From 6c7425b1252059815ff3fa52dfa1ba2f1215b809 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Fri, 13 Nov 2026 01:22:26 -0300 Subject: [PATCH 1/2] fix(features): resolve Features in the configuration's declaration order The reference builds its Feature list from Object.keys(config.features) followed by the --additional-features ids the configuration does not name, so integer-like ids come first, ascending, and the others keep their declaration order. Go sorted the ids, and features resolve-dependencies took Go map order. That decides which Feature is processed first: the first failing Feature reported, the order of the log lines and of the dependency graph's worklist. The loader now records the order of the features keys across the configuration and its override, mergeAdditionalFeatures appends the additional ids after them, and every resolution (up, build, read-configuration, upgrade, features resolve-dependencies) follows it. build no longer checks every id for a legacy one before resolving: the reference reports the first failing Feature in that order, after the image pull. Co-Authored-By: Claude Opus 5.5 --- docs/migration.md | 7 +++ docs/parity/parity-matrix.yaml | 52 ++++++++++++++++ internal/cli/build.go | 17 +---- internal/cli/build_held_test.go | 8 +-- internal/cli/compose_up.go | 2 +- internal/cli/feature_install.go | 21 ++----- internal/cli/feature_install_held_test.go | 2 +- internal/cli/feature_install_test.go | 10 +-- internal/cli/feature_order_test.go | 75 +++++++++++++++++++++++ internal/cli/features_resolve_deps.go | 2 +- internal/cli/helpers.go | 7 ++- internal/cli/helpers_held_test.go | 13 ++++ internal/cli/outdated.go | 2 +- internal/cli/pure_helpers_test.go | 3 +- internal/cli/read_configuration.go | 2 +- internal/cli/up.go | 1 + internal/config/key_order_test.go | 22 +++++++ internal/config/loader.go | 11 ++-- internal/config/types.go | 1 + internal/features/resolve_test.go | 22 ++++++- internal/features/types.go | 16 +++-- 21 files changed, 238 insertions(+), 58 deletions(-) create mode 100644 internal/cli/feature_order_test.go diff --git a/docs/migration.md b/docs/migration.md index eef2e28..2267ed2 100644 --- a/docs/migration.md +++ b/docs/migration.md @@ -208,6 +208,13 @@ runs the official CLI is unaffected: - Only a registry whose host is `localhost` is reached over plain HTTP. Earlier releases also used plain HTTP for `127.0.0.1`, which is now addressed over HTTPS like any other host. Refer to a local plain-HTTP registry as `localhost:`. +- Features are resolved in the order the configuration declares them, as the + official CLI resolves them: integer-like ids such as `"9"` first, ascending, + then the others in declaration order, followed by the `--additional-features` + ids the configuration does not name. When several Features fail, the one + reported is the first in that order; earlier releases reported the first by + sorted id. `build` no longer reports a legacy Feature id before trying the + Features declared ahead of it, or before pulling the image. ## Rollback diff --git a/docs/parity/parity-matrix.yaml b/docs/parity/parity-matrix.yaml index 3c114b1..98ae708 100644 --- a/docs/parity/parity-matrix.yaml +++ b/docs/parity/parity-matrix.yaml @@ -975,6 +975,58 @@ initial_cases: current_status: match notes: "With the configuration at the workspace root (.devcontainer.json), './feat' resolves outside the .devcontainer folder, which processFeatureIdentifier refuses; build reports the plain Error buildDependencyGraph throws as its message: {\"outcome\":\"error\",\"message\":\"ERR: Feature './feat' could not be processed. You may not have permission ...\",\"description\":\"An error occurred building the container.\"}, exit 1. Go used to build the Feature." + - id: features.resolve-dependencies-declaration-order-success + lane: contract + command: features + priority: p1 + docker_required: false + setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE/.devcontainer/zeta $WORKSPACE/.devcontainer/alpha $WORKSPACE/.devcontainer/mid $WORKSPACE/.devcontainer/beta $WORKSPACE/.devcontainer/omega; printf '%s' '{\"id\":\"zeta\",\"version\":\"1.0.0\"}' > $WORKSPACE/.devcontainer/zeta/devcontainer-feature.json; printf '%s' '{\"id\":\"alpha\",\"version\":\"1.0.0\"}' > $WORKSPACE/.devcontainer/alpha/devcontainer-feature.json; printf '%s' '{\"id\":\"mid\",\"version\":\"1.0.0\"}' > $WORKSPACE/.devcontainer/mid/devcontainer-feature.json; printf '%s' '{\"id\":\"beta\",\"version\":\"1.0.0\"}' > $WORKSPACE/.devcontainer/beta/devcontainer-feature.json; printf '%s' '{\"id\":\"omega\",\"version\":\"1.0.0\"}' > $WORKSPACE/.devcontainer/omega/devcontainer-feature.json; printf '%s' '{\"image\":\"alpine:3\",\"features\":{\"./zeta\":{},\"./alpha\":{},\"./mid\":{},\"./beta\":{},\"./omega\":{}}}' > $WORKSPACE/.devcontainer/devcontainer.json; echo WORKSPACE=$WORKSPACE" + cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}" + ts_cmd: "features resolve-dependencies --workspace-folder ${WORKSPACE}" + asserts: [exit_code, stdout_text] + class: features + current_status: match + notes: "userFeaturesToArray (containerFeaturesConfiguration.ts) lists the configuration's Features in Object.keys order, and the mermaid graph lists the dependency graph's worklist in that order: ./zeta, ./alpha, ./mid, ./beta, ./omega. Go used to take them in Go map order." + + - id: build.features-first-failure-in-declaration-order-error + lane: runtime + command: build + priority: p1 + docker_required: true + setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE/.devcontainer $WORKSPACE/zeta; printf '%s' '{\"image\":\"alpine:3\",\"features\":{\"../zeta\":{},\"zz\":{}}}' > $WORKSPACE/.devcontainer/devcontainer.json; echo WORKSPACE=$WORKSPACE" + cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}" + ts_cmd: "build --workspace-folder ${WORKSPACE}" + asserts: [exit_code, stdout_normalized] + class: features + current_status: match + notes: "Features are processed in Object.keys order and the first that fails is reported: '../zeta' is refused (outside .devcontainer) before the legacy id 'zz' is classified, so build reports ERR: Feature '../zeta' could not be processed. Go used to check every id for a legacy one before resolving any Feature and reported 'Legacy feature 'zz' not supported.'." + + - id: build.features-integer-like-ids-first-error + lane: runtime + command: build + priority: p1 + docker_required: true + setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE/.devcontainer; printf '%s' '{\"image\":\"alpine:3\",\"features\":{\"../zeta\":{},\"10\":{},\"9\":{}}}' > $WORKSPACE/.devcontainer/devcontainer.json; echo WORKSPACE=$WORKSPACE" + cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}" + ts_cmd: "build --workspace-folder ${WORKSPACE}" + asserts: [exit_code, stdout_normalized] + class: features + current_status: match + notes: "Object.keys lists integer-like keys first, ascending, so '9' is processed first and build reports 'Legacy feature '9' not supported.'. Go used to sort the ids and reported '10'." + + - id: build.additional-features-after-config-error + lane: runtime + command: build + priority: p1 + docker_required: true + setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE/.devcontainer $WORKSPACE/zeta $WORKSPACE/alpha; printf '%s' '{\"image\":\"alpine:3\",\"features\":{\"../zeta\":{}}}' > $WORKSPACE/.devcontainer/devcontainer.json; echo WORKSPACE=$WORKSPACE" + cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}" + ts_cmd: "build --workspace-folder ${WORKSPACE} --additional-features '{\"../alpha\":{}}'" + asserts: [exit_code, stdout_normalized] + class: features + current_status: match + notes: "userFeaturesToArray appends the --additional-features ids after the configuration's, so '../zeta' is processed and refused first. Go used to sort the merged ids and reported '../alpha'." + - id: up.update-uid-remote-user-from-image-metadata-success lane: runtime command: up diff --git a/internal/cli/build.go b/internal/cli/build.go index 13b395f..914141b 100644 --- a/internal/cli/build.go +++ b/internal/cli/build.go @@ -12,7 +12,6 @@ import ( "github.com/devcontainers/cli/internal/config" "github.com/devcontainers/cli/internal/docker" coreerrors "github.com/devcontainers/cli/internal/errors" - "github.com/devcontainers/cli/internal/features" "github.com/devcontainers/cli/internal/imagemeta" "github.com/devcontainers/cli/internal/log" "github.com/devcontainers/cli/internal/stringify" @@ -329,10 +328,6 @@ func (r *buildRunner) buildImage(ctx context.Context, cfg *config.DevContainer, return nil, fmt.Errorf("no image specified in devcontainer.json") } - if err := checkLegacyFeatures(cfg.Features); err != nil { - return nil, err - } - logger.Write(fmt.Sprintf("Using image: %s", cfg.Image), log.LevelInfo) folderImage := folderImageName(resolvePath(opts.workspaceFolder)) @@ -456,17 +451,6 @@ func imageBuildFor(cfg *config.DevContainer, opts *buildOpts) imageBuild { return imageAsIs } -func checkLegacyFeatures(feats map[string]any) error { - userFeatures, _ := features.UpdateDeprecatedFeaturesIntoOptions(sortedUserFeatures(feats, nil)) - for _, f := range userFeatures { - id, _ := features.BackwardCompatibleID(f.UserFeatureID) - if features.ClassifyID(id) == features.SourceLegacyShorthand { - return &coreerrors.ContainerError{Description: fmt.Sprintf(msgLegacyFeature, id)} - } - } - return nil -} - func imageNamesOr(names []string, fallback string) []string { if len(names) == 0 { return []string{fallback} @@ -693,6 +677,7 @@ func (r *buildRunner) featureBuildOptions(cfg *config.DevContainer, hostSub *con Output: opts.output, ContainerEnv: cfg.ContainerEnv, ContainerEnvOrder: cfg.ContainerEnvOrder, + FeatureOrder: cfg.FeatureOrder, FeatureOptionOrder: cfg.FeatureOptionOrder, FrozenLockfile: opts.experimentalFrozenLockfile, NoLockfile: opts.noLockfile, diff --git a/internal/cli/build_held_test.go b/internal/cli/build_held_test.go index 9337ae7..3b347d7 100644 --- a/internal/cli/build_held_test.go +++ b/internal/cli/build_held_test.go @@ -106,14 +106,14 @@ func TestImageBuildFor(t *testing.T) { } } -func TestCheckLegacyFeatures(t *testing.T) { - for _, id := range []string{"node", "golang", "gradle", "ghcr.io/devcontainers/features/node:1", "./local-feature", "owner/repo/feature"} { - if err := checkLegacyFeatures(map[string]any{id: true}); err != nil { +func TestResolveFeatureIDRefusesLegacyIDs(t *testing.T) { + for _, id := range []string{"node", "golang", "ghcr.io/devcontainers/features/node:1", "./local-feature", "owner/repo/feature"} { + if _, _, err := resolveFeatureID(log.Null, id); err != nil { t.Errorf("%s: unexpected error %v", id, err) } } for _, id := range []string{"not-a-feature", "not-a-feature:1", "node:1", "node:18", "gradle:1"} { - err := checkLegacyFeatures(map[string]any{id: true}) + _, _, err := resolveFeatureID(log.Null, id) if err == nil || !strings.Contains(err.Error(), "Legacy feature '"+id+"' not supported") { t.Errorf("%s: error = %v, want the legacy feature error naming it", id, err) } diff --git a/internal/cli/compose_up.go b/internal/cli/compose_up.go index 82663ab..abe6d1d 100644 --- a/internal/cli/compose_up.go +++ b/internal/cli/compose_up.go @@ -274,7 +274,7 @@ func (r *upRunner) composeFeatureSets(ctx context.Context, cfg *config.DevContai return nil, "", err } } - res, err := fetchFeatureSetsWithOrder(ctx, r.log, cfg.Features, cfg.FeatureOptionOrder, localFeatureRoots{filepath.Dir(cfg.ConfigFilePath), opts.workspaceFolder}, lockfile, cfg.OverrideFeatureInstallOrder) + res, err := fetchFeatureSetsWithOrder(ctx, r.log, cfg.Features, cfg.FeatureOrder, cfg.FeatureOptionOrder, localFeatureRoots{filepath.Dir(cfg.ConfigFilePath), opts.workspaceFolder}, lockfile, cfg.OverrideFeatureInstallOrder) if err != nil { return nil, "", err } diff --git a/internal/cli/feature_install.go b/internal/cli/feature_install.go index 4f75788..e8fe53b 100644 --- a/internal/cli/feature_install.go +++ b/internal/cli/feature_install.go @@ -18,7 +18,6 @@ import ( "path/filepath" "regexp" "slices" - "sort" "strconv" "strings" "time" @@ -48,6 +47,7 @@ type FeatureBuildOptions struct { CABundle string ContainerEnv map[string]string ContainerEnvOrder []string + FeatureOrder []string FeatureOptionOrder map[string][]string SkipPersistCustoms bool FeaturesBasePath string @@ -67,16 +67,8 @@ type fetchFeatureResult struct { TmpDir string } -func fetchFeatureSets(ctx context.Context, logger log.Logger, featuresCfg map[string]any, optionOrder map[string][]string, local localFeatureRoots) (*fetchFeatureResult, error) { - return fetchFeatureSetsWithOrder(ctx, logger, featuresCfg, optionOrder, local, nil, nil) -} - -func sortedUserFeatures(featuresCfg map[string]any, optionOrder map[string][]string) []features.DevContainerFeature { - userFeatures := features.UserFeaturesToArray(featuresCfg, optionOrder) - sort.Slice(userFeatures, func(i, j int) bool { - return userFeatures[i].UserFeatureID < userFeatures[j].UserFeatureID - }) - return userFeatures +func fetchFeatureSets(ctx context.Context, logger log.Logger, featuresCfg map[string]any, order []string, optionOrder map[string][]string, local localFeatureRoots) (*fetchFeatureResult, error) { + return fetchFeatureSetsWithOrder(ctx, logger, featuresCfg, order, optionOrder, local, nil, nil) } type localFeatureRoots struct { @@ -113,13 +105,12 @@ func createFeaturesFolder() (string, error) { } } -func fetchFeatureSetsWithOrder(ctx context.Context, logger log.Logger, featuresCfg map[string]any, optionOrder map[string][]string, local localFeatureRoots, lockfile *features.Lockfile, overrideOrder []string) (*fetchFeatureResult, error) { +func fetchFeatureSetsWithOrder(ctx context.Context, logger log.Logger, featuresCfg map[string]any, order []string, optionOrder map[string][]string, local localFeatureRoots, lockfile *features.Lockfile, overrideOrder []string) (*fetchFeatureResult, error) { if len(featuresCfg) == 0 { return nil, nil } - userFeatures := sortedUserFeatures(featuresCfg, optionOrder) - userFeatures, warnings := features.UpdateDeprecatedFeaturesIntoOptions(userFeatures) + userFeatures, warnings := features.UpdateDeprecatedFeaturesIntoOptions(features.UserFeaturesToArray(featuresCfg, order, optionOrder)) for _, w := range warnings { logger.Write(w, log.LevelWarning) } @@ -588,7 +579,7 @@ func fetchBuildFeatures(ctx context.Context, logger log.Logger, featuresCfg map[ return nil, err } } - result, err := fetchFeatureSetsWithOrder(ctx, logger, featuresCfg, fbOpts.FeatureOptionOrder, localFeatureRoots{fbOpts.FeaturesBasePath, fbOpts.WorkspaceFolder}, lockfile, fbOpts.OverrideFeatureInstallOrder) + result, err := fetchFeatureSetsWithOrder(ctx, logger, featuresCfg, fbOpts.FeatureOrder, fbOpts.FeatureOptionOrder, localFeatureRoots{fbOpts.FeaturesBasePath, fbOpts.WorkspaceFolder}, lockfile, fbOpts.OverrideFeatureInstallOrder) if err != nil { return nil, err } diff --git a/internal/cli/feature_install_held_test.go b/internal/cli/feature_install_held_test.go index 5cfcb8d..820487f 100644 --- a/internal/cli/feature_install_held_test.go +++ b/internal/cli/feature_install_held_test.go @@ -429,7 +429,7 @@ func TestTarballFeatureIDUsesTheURLPath(t *testing.T) { func TestFetchFeatureSetsReportsTheFirstSortedFailure(t *testing.T) { for range 5 { - _, err := fetchFeatureSets(t.Context(), log.Null, map[string]any{"./b-missing": true, "./a-missing": true, "./c-missing": true}, nil, devcontainerRoots(t)) + _, err := fetchFeatureSets(t.Context(), log.Null, map[string]any{"./b-missing": true, "./a-missing": true, "./c-missing": true}, nil, nil, devcontainerRoots(t)) if err == nil || !strings.Contains(err.Error(), "'./a-missing'") { t.Fatalf("err = %v, want the failure of ./a-missing", err) } diff --git a/internal/cli/feature_install_test.go b/internal/cli/feature_install_test.go index eb48c14..7fd4206 100644 --- a/internal/cli/feature_install_test.go +++ b/internal/cli/feature_install_test.go @@ -148,7 +148,7 @@ func TestFetchFeatureSets(t *testing.T) { t.Run(tt.name, func(t *testing.T) { roots := devcontainerRoots(t) tt.setup(t, roots.configDir) - result, err := fetchFeatureSets(t.Context(), log.Null, tt.entries, nil, roots) + result, err := fetchFeatureSets(t.Context(), log.Null, tt.entries, nil, nil, roots) if result != nil && result.TmpDir != "" { defer os.RemoveAll(result.TmpDir) } @@ -232,7 +232,7 @@ func TestFetchFeatureSets_StagingLayout(t *testing.T) { if err := os.WriteFile(filepath.Join(dirB, "install.sh"), []byte("#!/bin/sh\n"), 0o755); err != nil { t.Fatal(err) } - result, err := fetchFeatureSets(t.Context(), log.Null, map[string]any{"./dir-a": true}, nil, roots) + result, err := fetchFeatureSets(t.Context(), log.Null, map[string]any{"./dir-a": true}, nil, nil, roots) if err != nil { t.Fatal(err) } @@ -396,7 +396,7 @@ func TestFetchFeatureSetsRefusesLocalFeaturesOutsideDevcontainer(t *testing.T) { t.Run(tc.id, func(t *testing.T) { var logs bytes.Buffer logger := log.New(log.Options{Level: log.LevelInfo, Format: "text", Writer: &logs}) - result, err := fetchFeatureSets(t.Context(), logger, map[string]any{tc.id: true}, nil, roots) + result, err := fetchFeatureSets(t.Context(), logger, map[string]any{tc.id: true}, nil, nil, roots) if result != nil { t.Fatalf("result = %+v, want none", result) } @@ -420,7 +420,7 @@ func TestFetchFeatureSetsRefusesLocalFeaturesOutsideDevcontainer(t *testing.T) { func TestFetchFeatureSetsMovesDeprecatedFeaturesIntoOptions(t *testing.T) { var logs bytes.Buffer logger := log.New(log.Options{Level: log.LevelInfo, Format: "text", Writer: &logs}) - result, err := fetchFeatureSets(t.Context(), logger, map[string]any{"gradle": map[string]any{}, "node:18": map[string]any{}}, nil, devcontainerRoots(t)) + result, err := fetchFeatureSets(t.Context(), logger, map[string]any{"gradle": map[string]any{}, "node:18": map[string]any{}}, nil, nil, devcontainerRoots(t)) if result != nil || err == nil || err.Error() != fmt.Sprintf(msgLegacyFeature, "node:18") { t.Fatalf("result = %+v, err = %v; want the legacy Feature error for node:18", result, err) } @@ -480,7 +480,7 @@ func TestFetchedFeaturesKeepTheDeclaredOrder(t *testing.T) { write("dep", `{"id": "dep", "version": "1.0.0"}`) result, err := fetchFeatureSets(t.Context(), log.Null, - map[string]any{"./sdk": map[string]any{"zz": "1", "version": "2", "aa": "3"}}, + map[string]any{"./sdk": map[string]any{"zz": "1", "version": "2", "aa": "3"}}, nil, map[string][]string{"./sdk": {"zz", "version", "aa"}}, roots) if err != nil { t.Fatal(err) diff --git a/internal/cli/feature_order_test.go b/internal/cli/feature_order_test.go new file mode 100644 index 0000000..be28c4f --- /dev/null +++ b/internal/cli/feature_order_test.go @@ -0,0 +1,75 @@ +package cli + +import ( + "path/filepath" + "strings" + "testing" +) + +func TestReadConfigurationReportsTheFirstFeatureInDeclarationOrder(t *testing.T) { + legacy9 := "Legacy feature '9' not supported." + refusedZeta := "ERR: Feature '../zeta' could not be processed." + for name, tc := range map[string]struct { + features, override, additional string + want string + }{ + "declaration order": {features: `{"../zeta":{},"../alpha":{}}`, want: refusedZeta}, + "integer-like ids first": {features: `{"../zeta":{},"10":{},"9":{}}`, want: legacy9}, + "override adds a Feature after": {features: `{"../zeta":{}}`, override: `{"features":{"../alpha":{}}}`, want: refusedZeta}, + "override integer-like id first": {features: `{"../zeta":{}}`, override: `{"features":{"9":{}}}`, want: legacy9}, + "additional Features after config": {features: `{"../zeta":{}}`, additional: `{"9":{},"../alpha":{}}`, want: refusedZeta}, + "additional integer-like id first": {features: `{"../zeta":{}}`, additional: `{"../alpha":{},"10":{},"9":{}}`, want: refusedZeta}, + } { + t.Run(name, func(t *testing.T) { + startFakeEngine(t, nil) + files := map[string]string{".devcontainer/devcontainer.json": `{"image":"alpine:3","features":` + tc.features + `}`} + if tc.override != "" { + files["override.json"] = tc.override + } + ws := writeConfigFixture(t, files) + args := []string{"--workspace-folder", ws, "--include-features-configuration"} + if tc.override != "" { + args = append(args, "--override-config", filepath.Join(ws, "override.json")) + } + if tc.additional != "" { + args = append(args, "--additional-features", tc.additional) + } + stdout, err := readConfigurationStdout(args...) + if err == nil || !strings.HasPrefix(err.Error(), tc.want) || stdout != "" { + t.Fatalf("stdout = %q, err = %v; want %q", stdout, err, tc.want) + } + }) + } +} + +func TestReadConfigurationReportsAnAdditionalIntegerLikeIDAfterTheConfigs(t *testing.T) { + startFakeEngine(t, nil) + ws := writeConfigFixture(t, map[string]string{".devcontainer/devcontainer.json": `{"image":"alpine:3","features":{"zz":{}}}`}) + _, err := readConfigurationStdout("--workspace-folder", ws, "--include-features-configuration", "--additional-features", `{"9":{}}`) + if err == nil || !strings.HasPrefix(err.Error(), "Legacy feature 'zz' not supported.") { + t.Fatalf("err = %v; want the config's legacy Feature zz first", err) + } +} + +func TestFeaturesResolveDependenciesListsFeaturesInDeclarationOrder(t *testing.T) { + ids := []string{"zeta", "alpha", "mid", "beta", "omega"} + local := map[string]string{} + var decl []string + for _, id := range ids { + local[id] = `{"id":"` + id + `","version":"1.0.0"}` + decl = append(decl, `"./`+id+`":{}`) + } + ws, _ := resolveDepsWorkspace(t, "{"+strings.Join(decl, ",")+"}", local) + out, err := runResolveDeps(t, "--workspace-folder", ws) + if err != nil { + t.Fatal(err) + } + prev := -1 + for _, id := range ids { + i := strings.Index(out, "[./"+id+"<") + if i <= prev { + t.Fatalf("mermaid nodes not in declaration order %v:\n%s", ids, out) + } + prev = i + } +} diff --git a/internal/cli/features_resolve_deps.go b/internal/cli/features_resolve_deps.go index 059c26a..0f33917 100644 --- a/internal/cli/features_resolve_deps.go +++ b/internal/cli/features_resolve_deps.go @@ -38,7 +38,7 @@ func realFeaturesResolveDepsCmd() *cobra.Command { } cfg := loadResult.Config - userFeatures := features.UserFeaturesToArray(cfg.Features, cfg.FeatureOptionOrder) + userFeatures := features.UserFeaturesToArray(cfg.Features, cfg.FeatureOrder, cfg.FeatureOptionOrder) if len(userFeatures) == 0 { logger.Write(fmt.Sprintf("Could not parse features object in configuration '%s'", cfg.ConfigFilePath), log.LevelError) return &coreerrors.ExitCodeError{Code: 1} diff --git a/internal/cli/helpers.go b/internal/cli/helpers.go index 305650d..ff2dced 100644 --- a/internal/cli/helpers.go +++ b/internal/cli/helpers.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/devcontainers/cli/internal/config" + "github.com/devcontainers/cli/internal/features" "github.com/devcontainers/cli/internal/jsonc" "github.com/devcontainers/cli/internal/product" ) @@ -24,10 +25,12 @@ func mergeAdditionalFeatures(cfg *config.DevContainer, additionalFeaturesJSON st cfg.Features = make(map[string]any) } + cfg.FeatureOrder = features.PropertyOrder(cfg.Features, cfg.FeatureOrder) additionalOnly := make(map[string]bool) - for k, v := range additional { + for _, k := range features.PropertyOrder(additional, jsonc.MemberNames([]byte(additionalFeaturesJSON))) { if _, exists := cfg.Features[k]; !exists { - cfg.Features[k] = v + cfg.Features[k] = additional[k] + cfg.FeatureOrder = append(cfg.FeatureOrder, k) additionalOnly[k] = true if names := jsonc.MemberNames([]byte(additionalFeaturesJSON), k); len(names) > 0 { if cfg.FeatureOptionOrder == nil { diff --git a/internal/cli/helpers_held_test.go b/internal/cli/helpers_held_test.go index 1598e8b..bf10a6d 100644 --- a/internal/cli/helpers_held_test.go +++ b/internal/cli/helpers_held_test.go @@ -77,3 +77,16 @@ func TestMergeAdditionalFeaturesRecordsASingleOptionWithoutPriorOrder(t *testing t.Errorf("FeatureOptionOrder[./g] = %v, want [only]", got) } } + +func TestMergeAdditionalFeaturesAppendsTheirOrderAfterTheConfigs(t *testing.T) { + cfg := &config.DevContainer{ + Features: map[string]any{"./b": true, "./a": true, "./u": true}, + FeatureOrder: []string{"./b", "./a"}, + } + if _, err := mergeAdditionalFeatures(cfg, `{"./d": true, "7": true, "./a": true, "./c": true}`); err != nil { + t.Fatal(err) + } + if want := []string{"./b", "./a", "./u", "7", "./d", "./c"}; !slices.Equal(cfg.FeatureOrder, want) { + t.Errorf("FeatureOrder = %v, want %v", cfg.FeatureOrder, want) + } +} diff --git a/internal/cli/outdated.go b/internal/cli/outdated.go index ad2c180..0d87327 100644 --- a/internal/cli/outdated.go +++ b/internal/cli/outdated.go @@ -447,7 +447,7 @@ func newUpgradeCmd() *cobra.Command { lockfilePath := cfg.ConfigFilePath - res, err := fetchFeatureSetsWithOrder(cmd.Context(), logger, cfg.Features, cfg.FeatureOptionOrder, localFeatureRoots{filepath.Dir(lockfilePath), ws}, nil, nil) + res, err := fetchFeatureSetsWithOrder(cmd.Context(), logger, cfg.Features, cfg.FeatureOrder, cfg.FeatureOptionOrder, localFeatureRoots{filepath.Dir(lockfilePath), ws}, nil, nil) if err != nil { return err } diff --git a/internal/cli/pure_helpers_test.go b/internal/cli/pure_helpers_test.go index 88a62ef..1e74518 100644 --- a/internal/cli/pure_helpers_test.go +++ b/internal/cli/pure_helpers_test.go @@ -12,6 +12,7 @@ import ( "github.com/devcontainers/cli/internal/config" "github.com/devcontainers/cli/internal/features" + "github.com/devcontainers/cli/internal/log" "github.com/devcontainers/cli/internal/oci" ) @@ -258,7 +259,7 @@ func TestBuildArgsAndOptionsFromConfig(t *testing.T) { } func TestCommandErrorOutputKeepsAContainerError(t *testing.T) { - legacy := checkLegacyFeatures(map[string]any{"myfeature": true}) + _, _, legacy := resolveFeatureID(log.Null, "myfeature") out := commandErrorOutput(fmt.Errorf("install features: %w", legacy), "An error occurred setting up the container") want := fmt.Sprintf(msgLegacyFeature, "myfeature") if out.Description != want || out.Message != want { diff --git a/internal/cli/read_configuration.go b/internal/cli/read_configuration.go index 70e3b2f..d621967 100644 --- a/internal/cli/read_configuration.go +++ b/internal/cli/read_configuration.go @@ -183,7 +183,7 @@ func runReadConfiguration(ctx context.Context, out Output, opts *readConfigOpts) if needsFeaturesConfiguration(opts, containerID, result) { lgr := log.New(log.Options{Level: log.ParseLevel(opts.logLevel), Format: opts.logFormat, Writer: logDst}) var featErr error - featResult, featErr = fetchFeatureSets(ctx, lgr, result.Config.Features, result.Config.FeatureOptionOrder, localFeatureRoots{filepath.Dir(result.Config.ConfigFilePath), workspaceFolder}) + featResult, featErr = fetchFeatureSets(ctx, lgr, result.Config.Features, result.Config.FeatureOrder, result.Config.FeatureOptionOrder, localFeatureRoots{filepath.Dir(result.Config.ConfigFilePath), workspaceFolder}) if featErr != nil { return readConfigurationFailure(lgr, featErr) } diff --git a/internal/cli/up.go b/internal/cli/up.go index be390a6..65e48ac 100644 --- a/internal/cli/up.go +++ b/internal/cli/up.go @@ -1170,6 +1170,7 @@ func (r *upRunner) featureBuildOptions(cfg *config.DevContainer, loadResult *con HostSub: &loadResult.HostSub, ContainerEnv: cfg.ContainerEnv, ContainerEnvOrder: cfg.ContainerEnvOrder, + FeatureOrder: cfg.FeatureOrder, FeatureOptionOrder: cfg.FeatureOptionOrder, AdditionalMetadata: []imagemeta.Entry{configMetadataEntry(cfg, false)}, LabelMetadata: []imagemeta.Entry{r.configLabel}, diff --git a/internal/config/key_order_test.go b/internal/config/key_order_test.go index 4ab7aa3..14bb3d9 100644 --- a/internal/config/key_order_test.go +++ b/internal/config/key_order_test.go @@ -48,6 +48,9 @@ func TestLoadKeepsContainerEnvAndFeatureOptionOrder(t *testing.T) { if want := []string{"ZED", "ALPHA", "MID", "ZED"}; !slices.Equal(cfg.ContainerEnvOrder, want) { t.Errorf("ContainerEnvOrder = %v, want %v", cfg.ContainerEnvOrder, want) } + if want := []string{"./f", "./g", "./h"}; !slices.Equal(cfg.FeatureOrder, want) { + t.Errorf("FeatureOrder = %v, want %v", cfg.FeatureOrder, want) + } want := map[string][]string{"./f": {"zz", "aa", "mm"}, "./h": {"b", "a"}} if len(cfg.FeatureOptionOrder) != len(want) { t.Errorf("FeatureOptionOrder = %v, want %v", cfg.FeatureOptionOrder, want) @@ -80,3 +83,22 @@ func TestLoadKeepsBuildArgsOrder(t *testing.T) { t.Errorf("BuildArgsOrder = %v, want %v", res.Config.BuildArgsOrder, want) } } + +func TestLoadOrdersFeatureIDsAsJavaScriptEnumeratesThem(t *testing.T) { + dir := t.TempDir() + base := filepath.Join(dir, "devcontainer.json") + override := filepath.Join(dir, "override.json") + if err := os.WriteFile(base, []byte(`{"image": "i", "features": {"./zeta": {}, "10": {}, "./alpha": {}, "2": {}}}`), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(override, []byte(`{"features": {"./beta": {}, "./zeta": {}, "1": {}}}`), 0o644); err != nil { + t.Fatal(err) + } + res, err := LoadDevContainerConfig(dir, base, override) + if err != nil { + t.Fatal(err) + } + if want := []string{"1", "2", "10", "./zeta", "./alpha", "./beta"}; !slices.Equal(res.Config.FeatureOrder, want) { + t.Errorf("FeatureOrder = %v, want %v", res.Config.FeatureOrder, want) + } +} diff --git a/internal/config/loader.go b/internal/config/loader.go index ba201aa..864840b 100644 --- a/internal/config/loader.go +++ b/internal/config/loader.go @@ -13,6 +13,7 @@ import ( "runtime" "strings" + "github.com/devcontainers/cli/internal/features" "github.com/devcontainers/cli/internal/jsonc" "github.com/devcontainers/cli/internal/pfs" ) @@ -211,7 +212,7 @@ func LoadDevContainerConfigWithMounts(workspaceFolder, configPath, overrideConfi } config.ConfigFilePath = configFilePath - config.ContainerEnvOrder, config.FeatureOptionOrder, config.BuildArgsOrder = declaredOrder(docs) + config.ContainerEnvOrder, config.FeatureOrder, config.FeatureOptionOrder, config.BuildArgsOrder = declaredOrder(docs) return &LoadResult{ Config: &config, @@ -223,11 +224,13 @@ func LoadDevContainerConfigWithMounts(workspaceFolder, configPath, overrideConfi }, nil } -func declaredOrder(docs [][]byte) (containerEnv []string, featureOptions map[string][]string, buildArgs []string) { +func declaredOrder(docs [][]byte) (containerEnv, featureIDs []string, featureOptions map[string][]string, buildArgs []string) { for _, data := range docs { containerEnv = append(containerEnv, jsonc.MemberNames(data, "containerEnv")...) buildArgs = append(buildArgs, jsonc.MemberNames(data, "build", "args")...) - for _, id := range jsonc.MemberNames(data, "features") { + ids := jsonc.MemberNames(data, "features") + featureIDs = append(featureIDs, ids...) + for _, id := range ids { if names := jsonc.MemberNames(data, "features", id); len(names) > 0 { if featureOptions == nil { featureOptions = map[string][]string{} @@ -236,7 +239,7 @@ func declaredOrder(docs [][]byte) (containerEnv []string, featureOptions map[str } } } - return containerEnv, featureOptions, buildArgs + return containerEnv, features.KeyOrder(featureIDs), featureOptions, buildArgs } func deepMergeConfig(base, override map[string]any) map[string]any { diff --git a/internal/config/types.go b/internal/config/types.go index 3eea9fa..15de5a9 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -58,6 +58,7 @@ type DevContainer struct { DevPort any `json:"devPort,omitempty"` ContainerEnvOrder []string `json:"-"` + FeatureOrder []string `json:"-"` FeatureOptionOrder map[string][]string `json:"-"` BuildArgsOrder []string `json:"-"` } diff --git a/internal/features/resolve_test.go b/internal/features/resolve_test.go index 30b4b73..e62334d 100644 --- a/internal/features/resolve_test.go +++ b/internal/features/resolve_test.go @@ -109,14 +109,32 @@ func TestUserFeaturesToArray(t *testing.T) { "ghcr.io/devcontainers/features/go:1": map[string]any{"version": "1.21"}, "ghcr.io/devcontainers/features/node:1": true, } - arr := UserFeaturesToArray(features, nil) + arr := UserFeaturesToArray(features, nil, nil) if len(arr) != 2 { t.Errorf("len = %d, want 2", len(arr)) } } +func TestUserFeaturesToArrayFollowsTheOrderThenSortsTheRest(t *testing.T) { + features := map[string]any{"./z": true, "./a": 1, "./m": "x", "./b": false} + arr := UserFeaturesToArray(features, []string{"./z", "./gone", "./m", "./z"}, map[string][]string{"./m": {"o"}}) + var ids []string + for _, f := range arr { + ids = append(ids, f.UserFeatureID) + if f.Options != features[f.UserFeatureID] { + t.Errorf("%s options = %v, want %v", f.UserFeatureID, f.Options, features[f.UserFeatureID]) + } + } + if want := []string{"./z", "./m", "./a", "./b"}; !slices.Equal(ids, want) { + t.Errorf("ids = %v, want %v", ids, want) + } + if !slices.Equal(arr[1].OptionOrder, []string{"o"}) || arr[0].OptionOrder != nil { + t.Errorf("option order = %v, %v", arr[0].OptionOrder, arr[1].OptionOrder) + } +} + func TestUserFeaturesToArray_Nil(t *testing.T) { - arr := UserFeaturesToArray(nil, nil) + arr := UserFeaturesToArray(nil, nil, nil) if arr != nil { t.Errorf("expected nil, got %v", arr) } diff --git a/internal/features/types.go b/internal/features/types.go index f1df351..7441f32 100644 --- a/internal/features/types.go +++ b/internal/features/types.go @@ -322,14 +322,22 @@ type DevContainerFeature struct { OptionOrder []string } -// optionOrder maps a Feature id to the member names of its options object -// in devcontainer.json; an id it lacks has its options in sorted order. -func UserFeaturesToArray(features map[string]any, optionOrder map[string][]string) []DevContainerFeature { +// UserFeaturesToArray lists features in order, then the ids order does not +// name, sorted. optionOrder maps a Feature id to the member names of its +// options object in devcontainer.json; an id it lacks has its options in +// sorted order. +func UserFeaturesToArray(features map[string]any, order []string, optionOrder map[string][]string) []DevContainerFeature { if features == nil { return nil } result := make([]DevContainerFeature, 0, len(features)) - for id, opts := range features { + seen := make(map[string]bool, len(features)) + for _, id := range slices.Concat(order, slices.Sorted(maps.Keys(features))) { + opts, ok := features[id] + if !ok || seen[id] { + continue + } + seen[id] = true result = append(result, DevContainerFeature{ UserFeatureID: id, Options: opts, From 4217127b488a9a02d6f51e018073cd06138c1dfc Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Fri, 13 Nov 2026 01:22:35 -0300 Subject: [PATCH 2/2] fix(features)!: stop treating http:// Feature ids as tarball URLs The reference's getFeatureIdType takes only https:// ids as a direct tarball. An http:// id falls through to the OCI classification, where getRef rejects it without any request; the GitHub Releases fallback then refuses an id that is not owner/repo/feature, logs "Could not resolve Feature ''", and the dependency graph fails with "ERR: Feature '' could not be processed. You may not have permission ...". Go downloaded the tarball over plain HTTP. Any reference the OCI parser rejects and that fallback refuses now leaves the Feature unprocessed the same way, after the reference's warning and error lines. A reference the fallback would fetch from GitHub Releases keeps failing with the parse error, since this CLI does not implement that fallback. BREAKING CHANGE: an http:// Feature id no longer downloads a tarball; build, up, read-configuration and upgrade exit 1 with "ERR: Feature '' could not be processed. ...". Serve the tarball over https:// (NODE_EXTRA_CA_CERTS or SSL_CERT_FILE can name a private CA) or publish the Feature to an OCI registry. Co-Authored-By: Claude Opus 5.5 --- docs/migration.md | 7 ++ docs/parity/parity-matrix.yaml | 13 ++++ internal/cli/compose_up_held_test.go | 5 +- internal/cli/feature_deps.go | 2 +- internal/cli/feature_install.go | 23 ++++++- internal/cli/feature_install_held_test.go | 15 ++++- internal/cli/feature_order_test.go | 67 +++++++++++++++++++ internal/cli/feature_tarball_test.go | 6 +- internal/features/resolve.go | 3 +- internal/features/resolve_test.go | 2 +- .../features/testdata/ts-oracle/legacy.json | 7 ++ internal/features/ts_oracle_test.go | 4 ++ tools/tsoracle/oracle.ts | 2 +- 13 files changed, 141 insertions(+), 15 deletions(-) diff --git a/docs/migration.md b/docs/migration.md index 2267ed2..6a954b1 100644 --- a/docs/migration.md +++ b/docs/migration.md @@ -208,6 +208,13 @@ runs the official CLI is unaffected: - Only a registry whose host is `localhost` is reached over plain HTTP. Earlier releases also used plain HTTP for `127.0.0.1`, which is now addressed over HTTPS like any other host. Refer to a local plain-HTTP registry as `localhost:`. +- Only an `https://` Feature id is a tarball URL. An `http://` id is now handled + as the official CLI handles it: as a registry reference it cannot parse, so + `build`, `up`, `read-configuration` and `upgrade` exit 1 with `ERR: Feature + '' could not be processed. ...` without downloading anything. Earlier + releases downloaded the tarball over plain HTTP. Serve the tarball over HTTPS + (with `NODE_EXTRA_CA_CERTS` or `SSL_CERT_FILE` naming a private CA if needed), + or publish the Feature to an OCI registry. - Features are resolved in the order the configuration declares them, as the official CLI resolves them: integer-like ids such as `"9"` first, ascending, then the others in declaration order, followed by the `--additional-features` diff --git a/docs/parity/parity-matrix.yaml b/docs/parity/parity-matrix.yaml index 98ae708..692cb4c 100644 --- a/docs/parity/parity-matrix.yaml +++ b/docs/parity/parity-matrix.yaml @@ -1027,6 +1027,19 @@ initial_cases: current_status: match notes: "userFeaturesToArray appends the --additional-features ids after the configuration's, so '../zeta' is processed and refused first. Go used to sort the merged ids and reported '../alpha'." + - id: build.features-http-id-not-a-tarball-error + lane: runtime + command: build + priority: p1 + docker_required: true + setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE/.devcontainer; printf '%s' '{\"image\":\"alpine:3\",\"features\":{\"http://127.0.0.1:9/devcontainer-feature-a.tgz\":{}}}' > $WORKSPACE/.devcontainer/devcontainer.json; echo WORKSPACE=$WORKSPACE" + cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}" + ts_cmd: "build --workspace-folder ${WORKSPACE}" + asserts: [exit_code, stdout_normalized] + class: features + current_status: match + notes: "getFeatureIdType takes only https:// as a tarball URL; an http:// id goes to the OCI path, where getRef rejects it, then to the GitHub Releases fallback, which refuses an id that is not owner/repo/feature. buildDependencyGraph throws and build reports ERR: Feature 'http://...' could not be processed. You may not have permission ... without any request. Go used to download it as a tarball and reported the connection error." + - id: up.update-uid-remote-user-from-image-metadata-success lane: runtime command: up diff --git a/internal/cli/compose_up_held_test.go b/internal/cli/compose_up_held_test.go index 404a488..e041b8b 100644 --- a/internal/cli/compose_up_held_test.go +++ b/internal/cli/compose_up_held_test.go @@ -9,7 +9,6 @@ import ( "errors" "io" "net/http" - "net/http/httptest" "os" "path/filepath" "regexp" @@ -685,9 +684,7 @@ func TestComposeFeatureSets(t *testing.T) { }) t.Run("lockfile pins a tarball", func(t *testing.T) { tgz := featureTarball(t, "tarred") - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Write(tgz) })) - defer srv.Close() - uri := srv.URL + "/devcontainer-feature-tarred.tgz" + uri := serveTrustedTLS(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Write(tgz) })) + "/devcontainer-feature-tarred.tgz" for _, noLockfile := range []bool{false, true} { h := newComposeHarness(t) h.r.opts.noLockfile = noLockfile diff --git a/internal/cli/feature_deps.go b/internal/cli/feature_deps.go index f3f340f..b867996 100644 --- a/internal/cli/feature_deps.go +++ b/internal/cli/feature_deps.go @@ -73,7 +73,7 @@ func newMetadataProcessFeature(client oci.Registry, logger log.Logger, local loc default: ref, err := oci.ParseRef(resolvedID) if err != nil { - return nil, fmt.Errorf("ERR: Feature '%s' could not be processed. %w", id, err) + return nil, unparseableFeatureRef(logger, id, resolvedID, err) } if lockfile != nil { if integrity := lockfile.Features[resolvedID].Integrity; integrity != "" { diff --git a/internal/cli/feature_install.go b/internal/cli/feature_install.go index e8fe53b..c10bc1a 100644 --- a/internal/cli/feature_install.go +++ b/internal/cli/feature_install.go @@ -297,7 +297,7 @@ func processInstallFeature( default: ref, err := oci.ParseRef(resolvedID) if err != nil { - return nil, fmt.Errorf("ERR: Feature '%s' could not be processed. %w", id, err) + return nil, unparseableFeatureRef(logger, id, resolvedID, err) } userRef := ref @@ -986,6 +986,27 @@ func isPlainHTTPURL(u string) bool { return parsed.Scheme == "http" || parsed.Hostname() == "localhost" } +var githubReleaseFeatureName = regexp.MustCompile(`^[a-zA-Z0-9_-]*$`) + +// unparseableFeatureRef returns nil, after logging why, for a reference that +// TS's GitHub Releases fallback refuses, which leaves the Feature +// unprocessed; this CLI lacks that fallback, so it fails on any other. +func unparseableFeatureRef(logger log.Logger, id, resolvedID string, parseErr error) error { + at := strings.Split(resolvedID, "@") + var refusal string + switch parts := strings.Split(at[0], "/"); { + case len(at) > 2: + refusal = "Parse error. Use the '@' symbol only to designate a version tag." + case len(parts) != 3 || slices.Contains(parts, "") || !githubReleaseFeatureName.MatchString(parts[2]): + refusal = fmt.Sprintf("Could not resolve Feature '%s'. Ensure the Feature is published and accessible from your current environment.", resolvedID) + default: + return fmt.Errorf("ERR: Feature '%s' could not be processed. %w", id, parseErr) + } + logger.Write(fmt.Sprintf("Could not resolve Feature manifest for '%s'. If necessary, provide registry credentials with 'docker login '.", resolvedID), log.LevelWarning) + logger.Write(refusal, log.LevelError) + return nil +} + func resolveFeatureID(logger log.Logger, id string) (string, features.SourceType, error) { resolvedID, warning := features.BackwardCompatibleID(id) if warning != "" { diff --git a/internal/cli/feature_install_held_test.go b/internal/cli/feature_install_held_test.go index 820487f..5fad5f7 100644 --- a/internal/cli/feature_install_held_test.go +++ b/internal/cli/feature_install_held_test.go @@ -5,6 +5,7 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" + "encoding/pem" "errors" "fmt" "io" @@ -337,11 +338,23 @@ func TestProcessInstallFeatureOCIErrors(t *testing.T) { func serveBytes(t *testing.T, body []byte, status int) string { t.Helper() - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + return serveTrustedTLS(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(status) w.Write(body) })) +} + +func serveTrustedTLS(t *testing.T, h http.Handler) string { + t.Helper() + srv := httptest.NewTLSServer(h) t.Cleanup(srv.Close) + caFile := filepath.Join(t.TempDir(), "ca.pem") + certPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: srv.Certificate().Raw}) + if err := os.WriteFile(caFile, certPEM, 0o600); err != nil { + t.Fatal(err) + } + t.Setenv("NODE_EXTRA_CA_CERTS", "") + t.Setenv("SSL_CERT_FILE", caFile) return srv.URL } diff --git a/internal/cli/feature_order_test.go b/internal/cli/feature_order_test.go index be28c4f..c6c764c 100644 --- a/internal/cli/feature_order_test.go +++ b/internal/cli/feature_order_test.go @@ -1,9 +1,16 @@ package cli import ( + "bytes" + "net/http" + "net/http/httptest" "path/filepath" "strings" + "sync/atomic" "testing" + + "github.com/devcontainers/cli/internal/features" + "github.com/devcontainers/cli/internal/log" ) func TestReadConfigurationReportsTheFirstFeatureInDeclarationOrder(t *testing.T) { @@ -73,3 +80,63 @@ func TestFeaturesResolveDependenciesListsFeaturesInDeclarationOrder(t *testing.T prev = i } } + +func TestPlainHTTPFeatureIDIsNotATarball(t *testing.T) { + var requests atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests.Add(1) + w.Write(featureTgzBytes(t, map[string]string{"devcontainer-feature.json": `{"id":"x"}`})) + })) + t.Cleanup(srv.Close) + id := srv.URL + "/devcontainer-feature-x.tgz" + wantLogs := []string{ + "Could not resolve Feature manifest for '" + id + "'. If necessary, provide registry credentials with 'docker login '.\n", + "Could not resolve Feature '" + id + "'. Ensure the Feature is published and accessible from your current environment.\n", + } + + for name, process := range map[string]func(log.Logger) (*features.Set, error){ + "install": func(l log.Logger) (*features.Set, error) { + return processInstallFeature(l, nil, &features.FNode{UserFeatureID: id}, localFeatureRoots{}, nil, stageDirs(t)) + }, + "metadata": func(l log.Logger) (*features.Set, error) { + return newMetadataProcessFeature(nil, l, localFeatureRoots{}, nil)(&features.FNode{UserFeatureID: id}) + }, + } { + t.Run(name, func(t *testing.T) { + var logs bytes.Buffer + set, err := process(log.New(log.Options{Level: log.LevelInfo, Format: "text", Writer: &logs})) + if set != nil || err != nil { + t.Fatalf("set, err = %+v, %v; want neither", set, err) + } + for _, line := range wantLogs { + if !strings.Contains(logs.String(), line) { + t.Errorf("logs = %q, want %q", logs.String(), line) + } + } + }) + } + if n := requests.Load(); n != 0 { + t.Errorf("%d requests to the http:// URL, want none", n) + } +} + +func TestUnparseableFeatureReferences(t *testing.T) { + for id, want := range map[string]string{ + "owner/repo/feat@1@2": "Parse error. Use the '@' symbol only to designate a version tag.", + "owner/repo/fe.at@bad": "Could not resolve Feature 'owner/repo/fe.at@bad'. Ensure the Feature is published and accessible from your current environment.", + "owner/repo/sub/feat@v1": "Could not resolve Feature 'owner/repo/sub/feat@v1'. Ensure the Feature is published and accessible from your current environment.", + "owner//feat@v1": "Could not resolve Feature 'owner//feat@v1'. Ensure the Feature is published and accessible from your current environment.", + } { + var logs bytes.Buffer + set, err := newMetadataProcessFeature(nil, log.New(log.Options{Level: log.LevelInfo, Format: "text", Writer: &logs}), localFeatureRoots{}, nil)(&features.FNode{UserFeatureID: id}) + if set != nil || err != nil { + t.Errorf("%s: set, err = %+v, %v; want neither", id, set, err) + } + if !strings.HasSuffix(logs.String(), want+"\n") { + t.Errorf("%s: logs = %q, want them to end with %q", id, logs.String(), want) + } + } + if _, err := newMetadataProcessFeature(nil, log.Null, localFeatureRoots{}, nil)(&features.FNode{UserFeatureID: "owner/repo/feat@v1"}); err == nil || !strings.HasPrefix(err.Error(), "ERR: Feature 'owner/repo/feat@v1' could not be processed. ") { + t.Errorf("err = %v; want the reference's parse failure", err) + } +} diff --git a/internal/cli/feature_tarball_test.go b/internal/cli/feature_tarball_test.go index df978cd..6a89f65 100644 --- a/internal/cli/feature_tarball_test.go +++ b/internal/cli/feature_tarball_test.go @@ -214,11 +214,9 @@ func TestDownloadFeatureTarball_2xxAccepted(t *testing.T) { } func TestDirectTarballDigestCheckedAgainstTheLockfile(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + uri := serveTrustedTLS(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Write([]byte("not-a-real-tarball")) - })) - defer srv.Close() - uri := srv.URL + "/feature.tgz" + })) + "/feature.tgz" sum := sha256.Sum256([]byte("not-a-real-tarball")) digest := "sha256:" + hex.EncodeToString(sum[:]) diff --git a/internal/features/resolve.go b/internal/features/resolve.go index bd72815..1797742 100644 --- a/internal/features/resolve.go +++ b/internal/features/resolve.go @@ -21,8 +21,7 @@ func ClassifyID(id string) SourceType { if !strings.ContainsAny(id, `/\`) { return SourceLegacyShorthand } - // TS takes only https:// as a tarball URL. - if strings.HasPrefix(id, "http://") || strings.HasPrefix(id, "https://") { + if strings.HasPrefix(id, "https://") { return SourceDirectTarball } if strings.HasPrefix(id, "./") || strings.HasPrefix(id, "../") || strings.HasPrefix(id, "/") { diff --git a/internal/features/resolve_test.go b/internal/features/resolve_test.go index e62334d..fbfb71d 100644 --- a/internal/features/resolve_test.go +++ b/internal/features/resolve_test.go @@ -22,7 +22,7 @@ func TestClassifyFeatureID(t *testing.T) { {"owner/feat", SourceOCI}, {`a\b`, SourceOCI}, {"https://example.com/feature.tgz", SourceDirectTarball}, - {"http://example.com/feature.tgz", SourceDirectTarball}, + {"http://example.com/feature.tgz", SourceOCI}, {"./local-feature", SourceLocalPath}, {"../sibling-feature", SourceLocalPath}, {"/abs/feature", SourceLocalPath}, diff --git a/internal/features/testdata/ts-oracle/legacy.json b/internal/features/testdata/ts-oracle/legacy.json index 4aea475..14bdb1a 100644 --- a/internal/features/testdata/ts-oracle/legacy.json +++ b/internal/features/testdata/ts-oracle/legacy.json @@ -476,6 +476,13 @@ ], "result": "direct-tarball" }, + { + "fn": "getFeatureIdType", + "args": [ + "http://example.com/devcontainer-feature-node.tgz" + ], + "result": "github-repo" + }, { "fn": "getFeatureIdType", "args": [ diff --git a/internal/features/ts_oracle_test.go b/internal/features/ts_oracle_test.go index 1a30e1e..0b60e1f 100644 --- a/internal/features/ts_oracle_test.go +++ b/internal/features/ts_oracle_test.go @@ -6,6 +6,7 @@ import ( "strings" "testing" + "github.com/devcontainers/cli/internal/oci" "github.com/devcontainers/cli/internal/tsoracle/oracletest" ) @@ -117,6 +118,9 @@ func TestTSOracleLegacyFeatureIDs(t *testing.T) { case SourceLocalPath: return "file-path", nil } + if _, err := oci.ParseRef(id); err != nil { + return "github-repo", nil + } return "oci", nil }, }) diff --git a/tools/tsoracle/oracle.ts b/tools/tsoracle/oracle.ts index 3358472..97e0359 100644 --- a/tools/tsoracle/oracle.ts +++ b/tools/tsoracle/oracle.ts @@ -79,7 +79,7 @@ const fns: Record any> = { return { userFeatures: updateDeprecatedFeaturesIntoOptions(userFeatures, capturingOutput(log)), log }; }, // Only ids it classifies without a registry request: legacy ids (it - // throws), tarball URLs and local paths. + // throws), tarball URLs, local paths and references getRef rejects. getFeatureIdType: async (id: string) => (await getFeatureIdType({ output: nullOutput, env: {} } as any, id, undefined)).type, // spec-node/containerFeatures.ts getSafeId: (s: string) => getSafeId(s),