diff --git a/docs/parity/parity-matrix.yaml b/docs/parity/parity-matrix.yaml index 0a9aa26..a2c08c4 100644 --- a/docs/parity/parity-matrix.yaml +++ b/docs/parity/parity-matrix.yaml @@ -1375,6 +1375,7 @@ initial_cases: current_status: match - id: build.buildkit-never-platform-failure + lock: feature-content-image lane: contract command: build priority: p1 @@ -1593,6 +1594,7 @@ initial_cases: current_status: match - id: exec.workspace-image-with-features-hello-success + lock: feature-content-image lane: runtime command: exec priority: p1 @@ -1606,6 +1608,7 @@ initial_cases: current_status: match - id: exec.workspace-dockerfile-with-features-hello-success + lock: feature-content-image lane: runtime command: exec priority: p1 @@ -1619,6 +1622,7 @@ initial_cases: current_status: match - id: exec.workspace-dockerfile-with-target-marker-success + lock: feature-content-image lane: runtime command: exec priority: p1 @@ -1632,6 +1636,7 @@ initial_cases: current_status: match - id: exec.workspace-compose-image-with-features-hello-success + lock: feature-content-image no_compose_isolation: true lane: runtime command: exec @@ -1971,6 +1976,7 @@ initial_cases: current_status: match - id: up.image-with-features-omit-config-remote-env-from-metadata-success + lock: feature-content-image lane: runtime command: up priority: p1 @@ -2009,6 +2015,7 @@ initial_cases: current_status: match - id: up.compose-image-with-features-omit-config-remote-env-from-metadata-success + lock: feature-content-image lane: runtime command: up priority: p1 @@ -2024,6 +2031,7 @@ initial_cases: notes: "Unblocked by pinning the harness setup with `docker pull --platform linux/amd64 redis:alpine` (the fixture's db service) before compose." - id: up.compose-dockerfile-with-features-omit-config-remote-env-from-metadata-success + lock: feature-content-image lane: runtime command: up priority: p1 @@ -2051,6 +2059,7 @@ initial_cases: current_status: match - id: up.image-metadata-container-env-success + lock: feature-content-image lane: runtime command: up priority: p1 @@ -2063,6 +2072,7 @@ initial_cases: current_status: match - id: up.image-with-features-success + lock: feature-content-image lane: runtime command: up priority: p1 @@ -2077,6 +2087,7 @@ initial_cases: current_status: match - id: up.image-with-git-feature-success + lock: feature-content-image lane: runtime command: up priority: p1 @@ -2182,6 +2193,7 @@ initial_cases: current_status: match - id: up.compose-dockerfile-with-features-success + lock: feature-content-image lane: runtime command: up priority: p1 @@ -2196,6 +2208,7 @@ initial_cases: notes: "Closed out by fixing the compose entrypoint generated by Go so that it uses real line breaks and keeps the container alive" - id: up.compose-dockerfile-with-features-reuse-stopped-container-with-persisted-overrides-success + lock: feature-content-image no_compose_isolation: true serial: true lane: runtime @@ -2213,6 +2226,7 @@ initial_cases: notes: "Reuse of a stopped compose container with persisted overrides. The divergence observed before (TS failed the second `docker compose up --no-recreate`) was an artifact of the setup with the old TS path; with the setup pointing to the TS submodule, Go and TS match." - id: up.compose-dockerfile-with-features-reuse-stopped-container-without-persisted-overrides-success + lock: feature-content-image no_compose_isolation: true serial: true lane: runtime @@ -2292,6 +2306,7 @@ initial_cases: current_status: match - id: up.compose-dockerfile-with-target-post-commands-success + lock: feature-content-image lane: runtime command: up priority: p1 @@ -2458,6 +2473,7 @@ initial_cases: current_status: match - id: build.image-metadata-local-features-success + lock: feature-content-image lane: runtime command: build priority: p1 @@ -2471,6 +2487,7 @@ initial_cases: current_status: match - id: build.skip-persisting-customizations-from-features-success + lock: feature-content-image lane: runtime command: build priority: p1 @@ -2484,6 +2501,7 @@ initial_cases: current_status: match - id: build.labels-success + lock: feature-content-image lane: runtime command: build priority: p0 @@ -2495,6 +2513,7 @@ initial_cases: current_status: match - id: build.local-feature-failure + lock: feature-content-image lane: runtime command: build priority: p0 @@ -2531,6 +2550,7 @@ initial_cases: current_status: match - id: build.dockerfile-multi-image-success + lock: feature-content-image lane: runtime command: build priority: p1 @@ -2607,6 +2627,7 @@ initial_cases: notes: "Passes in parity runtime by pinning DOCKER_DEFAULT_PLATFORM=linux/amd64 from the harness" - id: build.apply-build-options-success + lock: feature-content-image lane: runtime command: build priority: p1 @@ -2635,6 +2656,7 @@ initial_cases: notes: "Closed out using a valid cache source (`ubuntu:latest`); the nonexistent remote ref blocked TS before comparing Go." - id: build.image-metadata-container-env-success + lock: feature-content-image lane: runtime command: build priority: p1 @@ -2647,6 +2669,7 @@ initial_cases: current_status: match - id: build.compose-image-extra-cache-from-success + lock: feature-content-image lane: runtime command: build priority: p1 @@ -2746,6 +2769,7 @@ initial_cases: expect_infra_failure the harness compares exit codes instead of skipping via isInfraError." - id: build.compose-image-with-features-success + lock: feature-content-image lane: runtime command: build priority: p1 diff --git a/go.mod b/go.mod index 1c04109..0e5d44b 100644 --- a/go.mod +++ b/go.mod @@ -8,6 +8,7 @@ require ( github.com/Masterminds/semver/v3 v3.5.0 github.com/iancoleman/orderedmap v0.3.0 github.com/moby/buildkit v0.34.1 + github.com/moby/docker-image-spec v1.3.1 github.com/moby/moby/api v1.56.1 github.com/moby/moby/client v0.6.2 github.com/opencontainers/go-digest v1.0.0 @@ -17,6 +18,7 @@ require ( github.com/spin-stack/go-tools v0.0.0-20261025134518-0ee222410bc5 github.com/tailscale/hujson v0.0.0-20260727124030-b80ff77dac4f github.com/testcontainers/testcontainers-go v0.44.0 + go.uber.org/mock v0.6.0 golang.org/x/crypto v0.58.0 golang.org/x/net v0.61.0 gopkg.in/yaml.v3 v3.0.1 @@ -127,7 +129,6 @@ require ( github.com/mattn/go-runewidth v0.0.27 // indirect github.com/mitchellh/go-homedir v1.1.0 // indirect github.com/mitchellh/hashstructure/v2 v2.0.2 // indirect - github.com/moby/docker-image-spec v1.3.1 // indirect github.com/moby/go-archive v0.3.0 // indirect github.com/moby/patternmatcher v0.6.1 // indirect github.com/moby/sys/sequential v0.7.0 // indirect @@ -197,5 +198,6 @@ tool ( github.com/spin-stack/go-tools/cmd/testquality github.com/spin-stack/go-tools/cmd/versions github.com/spin-stack/go-tools/cmd/vulngate + go.uber.org/mock/mockgen golang.org/x/tools/cmd/deadcode ) diff --git a/go.sum b/go.sum index ea2970e..9e099ef 100644 --- a/go.sum +++ b/go.sum @@ -361,6 +361,8 @@ go.opentelemetry.io/otel/sdk/metric v1.46.0 h1:0piZ26EG4RBfebb2jhDH6ERCYHoVWduc3 go.opentelemetry.io/otel/sdk/metric v1.46.0/go.mod h1:I1PbKrdVc8Qu8HYVDNtqVIwLwjNrhsV/uFuxfwg8mO4= go.opentelemetry.io/otel/trace v1.46.0 h1:OULy7ccdJnZtJ0UDYFOIGaCmiWzJ8Vi2G/Rsu60qs1c= go.opentelemetry.io/otel/trace v1.46.0/go.mod h1:J7GAXweO77XSFkB/rmAqk9D6ihszhFjLU+d9WuUxDLI= +go.uber.org/mock v0.6.0 h1:hyF9dfmbgIX5EfOdasqLsWD6xqpNZlXblLB/Dbnwv3Y= +go.uber.org/mock v0.6.0/go.mod h1:KiVJ4BqZJaMj4svdfmHM0AUx4NJYO8ZNpPnZn1Z+BBU= go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= go.yaml.in/yaml/v3 v3.0.5 h1:N6y/pJk8buWs9NY5ERU2HSMfm+IuD/OtfdAnq6kESPw= go.yaml.in/yaml/v3 v3.0.5/go.mod h1:HVTZu1O7/Vkt2N+BFy8Zza+lnLsABggaTM2ZpNIGuKg= diff --git a/internal/cli/build.go b/internal/cli/build.go index 009aefd..dff3294 100644 --- a/internal/cli/build.go +++ b/internal/cli/build.go @@ -318,6 +318,7 @@ func prepareDockerfileBuild(cfg *config.DevContainer) (*dockerfileBuildPrep, err prep.StageName = stageName if modified != "" { tmpDockerfile := dockerfilePath + ".devcontainer.build" + // mutate-exempt: only the docker build this process starts reads the file, as its owner, and Cleanup removes it if err := os.WriteFile(tmpDockerfile, []byte(modified), 0644); err != nil { return nil, fmt.Errorf("write build dockerfile: %w", err) } diff --git a/internal/cli/build_auth.go b/internal/cli/build_auth.go index f5ffa32..e9fa873 100644 --- a/internal/cli/build_auth.go +++ b/internal/cli/build_auth.go @@ -54,11 +54,10 @@ func registryHostFromImageRef(ref string) string { if ref == "" { return "" } - i := strings.IndexByte(ref, '/') - if i < 0 { + first, _, found := strings.Cut(ref, "/") + if !found { return "" } - first := ref[:i] if first == "localhost" || strings.ContainsAny(first, ".:") { return first } diff --git a/internal/cli/build_repo2_held_test.go b/internal/cli/build_repo2_held_test.go new file mode 100644 index 0000000..5505316 --- /dev/null +++ b/internal/cli/build_repo2_held_test.go @@ -0,0 +1,50 @@ +package cli + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/devcontainers/cli/internal/config" +) + +// The Dockerfile written to name the final stage sits next to the user's with +// a copy of its .dockerignore, and Cleanup removes both and leaves the user's +// files alone. +func TestPrepareDockerfileBuildCleanupRemovesTheWrittenFiles(t *testing.T) { + dir := t.TempDir() + dockerfile := filepath.Join(dir, "Dockerfile") + if err := os.WriteFile(dockerfile, []byte("FROM alpine\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(dockerfile+".dockerignore", []byte("secret\n"), 0o644); err != nil { + t.Fatal(err) + } + prep, err := prepareDockerfileBuild(&config.DevContainer{ + ConfigFilePath: filepath.Join(dir, "devcontainer.json"), + Build: &config.Build{Dockerfile: "Dockerfile"}, + }) + if err != nil { + t.Fatal(err) + } + written, err := os.ReadFile(prep.Path) + if err != nil || !strings.Contains(string(written), "AS dev_container_auto_added_stage_label") { + t.Fatalf("written Dockerfile = %q, %v; want the final stage named", written, err) + } + if ignore, err := os.ReadFile(prep.Path + ".dockerignore"); err != nil || string(ignore) != "secret\n" { + t.Fatalf("written .dockerignore = %q, %v; want the user's copied", ignore, err) + } + prep.Cleanup() + for _, p := range []string{prep.Path, prep.Path + ".dockerignore"} { + if _, err := os.Stat(p); !errors.Is(err, os.ErrNotExist) { + t.Errorf("%s after Cleanup: %v, want it removed", p, err) + } + } + for _, p := range []string{dockerfile, dockerfile + ".dockerignore"} { + if _, err := os.Stat(p); err != nil { + t.Errorf("user's %s after Cleanup: %v", p, err) + } + } +} diff --git a/internal/cli/collection_package_held_test.go b/internal/cli/collection_package_held_test.go index 2164b0d..197e5fc 100644 --- a/internal/cli/collection_package_held_test.go +++ b/internal/cli/collection_package_held_test.go @@ -86,4 +86,3 @@ func TestPackageTemplateOptionalPathsShape(t *testing.T) { t.Errorf("collection lacks %s:\n%s", want, got) } } - diff --git a/internal/cli/disallowed.go b/internal/cli/disallowed.go index 693f70a..882a58a 100644 --- a/internal/cli/disallowed.go +++ b/internal/cli/disallowed.go @@ -27,7 +27,7 @@ import ( func cacheFolder() string { base := "devcontainercli" if runtime.GOOS == "linux" { - if u, err := user.Current(); err == nil && u.Username != "" { + if u, err := user.Current(); err == nil { base = "devcontainercli-" + u.Username } } diff --git a/internal/cli/image_build_info.go b/internal/cli/image_build_info.go index f0ff98f..eda2bbe 100644 --- a/internal/cli/image_build_info.go +++ b/internal/cli/image_build_info.go @@ -351,7 +351,7 @@ func (r *dockerfileResolver) replace(baseEnv map[string]string, str string, s *d b.WriteString(str[last:m[0]]) last = m[1] value := r.findValue(baseEnv, str[m[2]:m[3]], s, before) - varExp := m[4] // mutate-exempt: m[4] and m[5] are both -1 exactly when the :-word/:+word group is unmatched + varExp := m[4] // mutate-exempt: m[4] and m[5] are both -1 exactly when the :-word/:+word group is unmatched if varExp != -1 { // ${var:-word} / ${var:+word} option, word := str[m[6]], str[m[8]:m[9]] // the option is one character, - or + isSet := value != "" diff --git a/internal/cli/ociauth_held_test.go b/internal/cli/ociauth_held_test.go new file mode 100644 index 0000000..ade3562 --- /dev/null +++ b/internal/cli/ociauth_held_test.go @@ -0,0 +1,44 @@ +package cli + +import ( + "context" + "testing" + + "github.com/devcontainers/cli/internal/log" + "github.com/devcontainers/cli/internal/oci" +) + +// Storing the policy keeps the command context it is stored on: a cancelled +// command (Ctrl-C) must still cancel the registry traffic that reads the +// policy from the same context. A caller without a context gets one. +func TestWithOCIAuthPolicyKeepsTheContext(t *testing.T) { + policy, err := oci.NewAuthPolicy(true, nil, log.Null) + if err != nil { + t.Fatal(err) + } + parent, cancel := context.WithCancel(context.Background()) + ctx := withOCIAuthPolicy(parent, policy) + cancel() + if ctx.Err() == nil { + t.Error("cancelling the command context did not reach the context holding the policy") + } + if got := ociAuthPolicy(ctx); got != policy { + t.Errorf("ociAuthPolicy = %p, want the stored policy %p", got, policy) + } + //nolint:staticcheck // SA1012: a nil context is the case under test + if got := ociAuthPolicy(withOCIAuthPolicy(nil, policy)); got != policy { + t.Errorf("ociAuthPolicy without a parent context = %p, want %p", got, policy) + } +} + +// A context holding a nil policy reads as the default policy (hardening off, +// as the reference CLI defaults --oci-auth-hardening), never as nil. +func TestOCIAuthPolicyIgnoresAStoredNil(t *testing.T) { + policy := ociAuthPolicy(withOCIAuthPolicy(context.Background(), nil)) + if policy == nil { + t.Fatal("ociAuthPolicy returned nil") + } + if policy.Hardening() { + t.Error("the default policy must not harden") + } +} diff --git a/internal/cli/parity_lock_test.go b/internal/cli/parity_lock_test.go new file mode 100644 index 0000000..41359f0 --- /dev/null +++ b/internal/cli/parity_lock_test.go @@ -0,0 +1,76 @@ +package cli + +import ( + "os" + "path/filepath" + "regexp" + "slices" + "strings" + "testing" + + "gopkg.in/yaml.v3" +) + +var ( + parityWorkspaceFlag = regexp.MustCompile(`--workspace-folder\s+(src/test/configs/[^\s"';]+)`) + parityConfigFlag = regexp.MustCompile(`--config\s+(src/test/configs/[^\s"';]+)`) + nonEmptyFeaturesJSON = regexp.MustCompile(`"features"\s*:\s*\{\s*"`) +) + +// A case that builds Features without BuildKit goes through the shared +// dev_container_feature_content_temp image, so it must hold the +// feature-content-image lock; a case that does not, must not, so the lock +// serializes no more than it has to. +func TestParityMatrixLocksTheFeatureContentImage(t *testing.T) { + raw, err := os.ReadFile(filepath.Join("..", "..", "docs", "parity", "parity-matrix.yaml")) + if err != nil { + t.Fatal(err) + } + var matrix parityMatrix + if err := yaml.Unmarshal(raw, &matrix); err != nil { + t.Fatal(err) + } + locked := 0 + for _, tc := range matrix.InitialCases { + if _, ok := parityLocks[tc.Lock]; tc.Lock != "" && !ok { + t.Errorf("case %q names unknown lock %q", tc.ID, tc.Lock) + } + cmds := tc.TSCmd + "\n" + tc.SetupCmd + need := strings.Contains(cmds, "--buildkit=never") && slices.ContainsFunc(parityCaseConfigs(t, cmds), func(config string) bool { + data, err := os.ReadFile(filepath.Join("..", "..", config)) + if err != nil { + t.Fatalf("case %q: %v", tc.ID, err) + } + return nonEmptyFeaturesJSON.Match(data) + }) + switch has := tc.Lock == "feature-content-image"; { + case need && !has: + t.Errorf("case %q builds Features without BuildKit; add `lock: feature-content-image`", tc.ID) + case has && !need: + t.Errorf("case %q holds feature-content-image but builds no Features without BuildKit", tc.ID) + case has: + locked++ + } + } + if locked == 0 { + t.Error("no case holds feature-content-image") + } +} + +// parityCaseConfigs is every devcontainer.json the commands name, by +// --config or as a --workspace-folder's. +func parityCaseConfigs(t *testing.T, cmds string) []string { + var configs []string + for _, m := range parityConfigFlag.FindAllStringSubmatch(cmds, -1) { + configs = append(configs, m[1]) + } + for _, m := range parityWorkspaceFlag.FindAllStringSubmatch(cmds, -1) { + for _, c := range []string{".devcontainer/devcontainer.json", ".devcontainer.json"} { + if _, err := os.Stat(filepath.Join("..", "..", m[1], c)); err == nil { + configs = append(configs, filepath.Join(m[1], c)) + break + } + } + } + return configs +} diff --git a/internal/cli/parity_matrix_test.go b/internal/cli/parity_matrix_test.go index 61c34d7..f1912fc 100644 --- a/internal/cli/parity_matrix_test.go +++ b/internal/cli/parity_matrix_test.go @@ -68,6 +68,10 @@ type parityCase struct { // Serial forces a case to run sequentially (no t.Parallel). Used for the few // runtime cases that can't be isolated for parallel execution. Serial bool `yaml:"serial"` + // Lock names a Docker resource the case shares with other cases and cannot + // isolate; cases with the same lock run one at a time, still in parallel + // with every other case. See parityLocks. + Lock string `yaml:"lock"` // CompareHashes keeps sha256/hex digests UNSCRUBBED so they are compared // exactly between TS and Go. Set it for cases whose digests are deterministic // and meaningful (resolve-dependencies installOrder resource@digest, @@ -189,6 +193,16 @@ func writeParityReport(path string, snapshot map[parityOutcome][]string) error { return nil } +// parityLocks are the shared resources a case can name in `lock`. +// +// feature-content-image: without BuildKit, both CLIs build a Feature's files +// into the fixed tag dev_container_feature_content_temp (TS containerFeatures.ts +// getFeaturesBuildOptions) and build FROM it, so two such builds at once can +// take each other's Features. +var parityLocks = map[string]*sync.Mutex{ + "feature-content-image": {}, +} + // --- Main test --- func TestParityMatrix(t *testing.T) { @@ -314,6 +328,14 @@ func TestParityMatrix(t *testing.T) { outcome = paritySkippedArm64 t.Skip("arm64 experimental (set PARITY_ARM64=true to run; requires arm64 emulation)") } + if tc.Lock != "" { + mu, ok := parityLocks[tc.Lock] + if !ok { + t.Fatalf("unknown lock %q", tc.Lock) + } + mu.Lock() + defer mu.Unlock() + } perCaseTimeout := defaultTimeout if tc.Lane == "runtime" { perCaseTimeout = runtimeTimeout diff --git a/internal/cli/stop_repo2_held_test.go b/internal/cli/stop_repo2_held_test.go new file mode 100644 index 0000000..7f5fe36 --- /dev/null +++ b/internal/cli/stop_repo2_held_test.go @@ -0,0 +1,33 @@ +package cli + +import ( + "bytes" + "testing" + + "github.com/spf13/cobra" +) + +// The commands that take an optional positional path refuse a second one, as +// the TS CLI's yargs parser (.strict()) refuses arguments its commands do not +// declare; nothing is run. +func TestPathCommandsRefuseASecondPositional(t *testing.T) { + for name, newCmd := range map[string]func() *cobra.Command{ + "stop": newStopCmd, + "down": newDownCmd, + "open": newOpenCmd, + "features generate-docs": realFeaturesGenerateDocsCmd, + "templates generate-docs": realTemplatesGenerateDocsCmd, + "features test": realFeaturesTestCmd, + } { + cmd := newCmd() + cmd.SilenceUsage, cmd.SilenceErrors = true, true + var out bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&bytes.Buffer{}) + cmd.SetArgs([]string{t.TempDir(), t.TempDir()}) + err := cmd.ExecuteContext(t.Context()) + if want := "accepts at most 1 arg(s), received 2"; err == nil || err.Error() != want || out.Len() != 0 { + t.Errorf("%s with two paths: err = %v, stdout = %q; want %q and no output", name, err, out.String(), want) + } + } +} diff --git a/internal/cli/validate_repo_held_test.go b/internal/cli/validate_repo_held_test.go index 132cb18..e59d508 100644 --- a/internal/cli/validate_repo_held_test.go +++ b/internal/cli/validate_repo_held_test.go @@ -62,6 +62,9 @@ func TestValidateTerminalImplications(t *testing.T) { {80, 24, ""}, {80, 0, "Implications failed:\n terminal-columns -> terminal-rows"}, {0, 24, "Implications failed:\n terminal-rows -> terminal-columns"}, + {1, 0, "Implications failed:\n terminal-columns -> terminal-rows"}, + {0, 1, "Implications failed:\n terminal-rows -> terminal-columns"}, + {1, 1, ""}, } for _, c := range cases { err := validateTerminalImplications(c.cols, c.rows) diff --git a/internal/config/loader_held_test.go b/internal/config/loader_held_test.go new file mode 100644 index 0000000..ceaaffd --- /dev/null +++ b/internal/config/loader_held_test.go @@ -0,0 +1,60 @@ +package config + +import ( + "os" + "path/filepath" + "runtime" + "testing" +) + +// A workspace that is not in a repository mounts at /workspaces/ with +// no extra mount, even with the worktree common-dir mount enabled: TS +// getWorkspaceConfiguration finds no `.git` and adds nothing. +func TestComputeWorkspaceConfigOutsideARepository(t *testing.T) { + ws := filepath.Join(t.TempDir(), "plain") + if err := os.MkdirAll(ws, 0o755); err != nil { + t.Fatal(err) + } + wc := computeWorkspaceConfig(&Workspace{RootFolderPath: ws}, &DevContainer{}, true, true) + if len(wc.AdditionalMounts) != 0 || wc.WorkspaceFolder != "/workspaces/plain" { + t.Errorf("additionalMounts = %q, workspaceFolder = %q; want none and /workspaces/plain", wc.AdditionalMounts, wc.WorkspaceFolder) + } +} + +// The ceiling is compared as a clean path, so one written with a trailing +// separator still stops the walk before a `.git` above it. +func TestDetectGitRootUntilCleansTheCeiling(t *testing.T) { + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, ".git"), 0o755); err != nil { + t.Fatal(err) + } + ceiling := filepath.Join(root, "sub") + nested := filepath.Join(ceiling, "a") + if err := os.MkdirAll(nested, 0o755); err != nil { + t.Fatal(err) + } + if got := detectGitRootUntil(nested, ceiling+string(filepath.Separator)); got != "" { + t.Errorf("walk with ceiling %q found %q, want no repository", ceiling+string(filepath.Separator), got) + } +} + +// Off Windows a config's ${localEnv:NAME} is case-sensitive (TS +// replaceWithContext → lookupValue lowercases the name only when the platform +// is win32), so a name in another case than the host variable resolves to "". +func TestLoadResolvesLocalEnvCaseSensitivelyOffWindows(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("the host is Windows") + } + t.Setenv("GOOS_OVERRIDE", "") + t.Setenv("DC_HELD_CASE", "host") + ws := t.TempDir() + cfg := filepath.Join(ws, ".devcontainer", "devcontainer.json") + writeFile(t, cfg, `{"image":"x","containerEnv":{"EXACT":"${localEnv:DC_HELD_CASE}","LOWER":"${localEnv:dc_held_case}"}}`) + res, err := LoadDevContainerConfigWithMounts(ws, cfg, "", MountOptions{}) + if err != nil { + t.Fatal(err) + } + if env := res.Config.ContainerEnv; env["EXACT"] != "host" || env["LOWER"] != "" { + t.Errorf("containerEnv = %v, want EXACT=host and LOWER empty", env) + } +} diff --git a/internal/config/types_held_test.go b/internal/config/types_held_test.go new file mode 100644 index 0000000..19021e9 --- /dev/null +++ b/internal/config/types_held_test.go @@ -0,0 +1,69 @@ +package config + +import ( + "encoding/json" + "testing" +) + +// The build context is the legacy top-level "context", else build.context, else +// none (TS getDockerContextPath, spec-node/utils.ts: `'dockerFile' in config ? +// config.context : config.build.context`). +func TestBuildContextPrefersTheLegacyProperty(t *testing.T) { + for in, want := range map[string]string{ + `{"dockerFile":"D","context":"legacy","build":{"context":"build"}}`: "legacy", + `{"build":{"context":".."}}`: "..", + `{"build":{"dockerfile":"Dockerfile"}}`: "", + `{}`: "", + } { + var c DevContainer + if err := json.Unmarshal([]byte(in), &c); err != nil { + t.Fatal(err) + } + if got := c.BuildContext(); got != want { + t.Errorf("BuildContext(%s) = %q, want %q", in, got, want) + } + } +} + +// A lifecycle command marshals back as it was written - the string, array or +// object TS keeps as-is in the metadata label - and an absent one as null. +func TestLifecycleCommandMarshalsAsWritten(t *testing.T) { + for _, in := range []string{`"echo hi"`, `["echo","hi"]`, `{"a":"echo a"}`, `null`} { + var c LifecycleCommand + if err := json.Unmarshal([]byte(in), &c); err != nil { + t.Fatal(err) + } + got, err := json.Marshal(c) + if err != nil { + t.Fatal(err) + } + if string(got) != in { + t.Errorf("marshal(%s) = %s, want it unchanged", in, got) + } + } + if got, err := json.Marshal(LifecycleCommand{}); err != nil || string(got) != "null" { + t.Errorf("marshal of no command = %s, %v; want null", got, err) + } +} + +// A single dockerComposeFile marshals as the string, more than one as the array +// (the `string | string[]` shape the spec gives the property). +func TestStringOrStringsMarshalsOneAsAString(t *testing.T) { + for in, want := range map[string]string{ + `"a.yml"`: `"a.yml"`, + `["a.yml"]`: `"a.yml"`, + `["a.yml","b.yml"]`: `["a.yml","b.yml"]`, + } { + var s StringOrStrings + if err := json.Unmarshal([]byte(in), &s); err != nil { + t.Fatal(err) + } + got, err := json.Marshal(s) + if err != nil { + t.Fatal(err) + } + if string(got) != want { + t.Errorf("marshal(%s) = %s, want %s", in, got, want) + } + } +} diff --git a/internal/config/varsub.go b/internal/config/varsub.go index b992731..a5b6770 100644 --- a/internal/config/varsub.go +++ b/internal/config/varsub.go @@ -127,7 +127,6 @@ func substituteTags(s string, replace func(tag string) (string, bool, error)) (s return s, nil } var b strings.Builder - b.Grow(len(s) + len(s)/4) for { i := strings.Index(s, "${") if i < 0 { diff --git a/internal/config/varsub_held_test.go b/internal/config/varsub_held_test.go new file mode 100644 index 0000000..5ac7580 --- /dev/null +++ b/internal/config/varsub_held_test.go @@ -0,0 +1,51 @@ +package config + +import "testing" + +// An empty ${} is left as written and the tags after it are still resolved: TS +// matches /\$\{(.*?)\}/g, so "${}" is a match whose empty variable no case +// handles (replaceWithContext returns the match), and the scan goes on. +func TestResolveKeepsAnEmptyTagAndResolvesTheRest(t *testing.T) { + ctx := SubstitutionContext{HostSubContext: HostSubContext{LocalWorkspaceFolder: "/home/me/proj"}} + got, err := NewVariableResolver().Resolve(ctx, PhaseHost, "${}-${localWorkspaceFolderBasename}") + if err != nil { + t.Fatal(err) + } + if got != "${}-proj" { + t.Errorf("Resolve = %q, want ${}-proj", got) + } +} + +// ${containerEnv:NAME} matches the name case-insensitively only for a Windows +// platform (TS containerSubstitute: normalizeEnv lowercases the keys and +// lookupValue the name when isWindows); elsewhere a name in another case is +// missing and resolves to "". +func TestResolveContainerEnvIgnoresCaseOnlyOnWindows(t *testing.T) { + for platform, want := range map[string]string{"win32": "/bin", "linux": ""} { + ctx := SubstitutionContext{ + HostSubContext: HostSubContext{Platform: platform}, + ContainerEnv: map[string]string{"Path": "/bin"}, + } + got, err := NewVariableResolver().Resolve(ctx, PhaseContainer, "${containerEnv:PATH}") + if err != nil { + t.Fatal(err) + } + if got != want { + t.Errorf("%s: Resolve = %q, want %q", platform, got, want) + } + } +} + +// Only ${templateOption:NAME} with a single name is a template option: TS +// replaceTemplatedValues matches /\${templateOption:\s*(\w+?)\s*}/g, which +// neither a second argument nor another variable fits, so both stay as written. +func TestResolveTemplateOptionTakesOneName(t *testing.T) { + ctx := SubstitutionContext{TemplateOptions: map[string]string{"a": "x"}} + got, err := NewVariableResolver().Resolve(ctx, PhaseTemplate, "${templateOption:a}|${templateOption:a:b}|${other:a}") + if err != nil { + t.Fatal(err) + } + if want := "x|${templateOption:a:b}|${other:a}"; got != want { + t.Errorf("Resolve = %q, want %q", got, want) + } +} diff --git a/internal/docker/client_held_test.go b/internal/docker/client_held_test.go new file mode 100644 index 0000000..b4e2d31 --- /dev/null +++ b/internal/docker/client_held_test.go @@ -0,0 +1,16 @@ +package docker + +import ( + "slices" + "testing" +) + +// Every KEY=VALUE build secret becomes `--secret id=KEY,env=KEY` (the contract +// on BuildOptions.Secrets), a one-letter key included. +func TestBuildArgsPassesAOneLetterSecret(t *testing.T) { + args := (&Client{}).buildArgs(BuildOptions{UseBuildx: true, ContextPath: ".", Secrets: []string{"A=1"}}) + i := slices.Index(args, "--secret") + if i < 0 || i+1 >= len(args) || args[i+1] != "id=A,env=A" { + t.Errorf("args = %q, want --secret id=A,env=A", args) + } +} diff --git a/internal/docker/compose.go b/internal/docker/compose.go index 6fd7008..e045fb7 100644 --- a/internal/docker/compose.go +++ b/internal/docker/compose.go @@ -194,34 +194,19 @@ func (c *ComposeClient) Down(ctx context.Context, composeFiles []string, project return nil } -// SupportsAdditionalContexts returns true if compose version >= 2.17.0. +// SupportsAdditionalContexts returns true if compose version >= 2.17.0, which +// added additional_contexts (TS buildAndExtendDockerCompose: `version && +// !isEarlierVersion(version, [2, 17, 0])`). An unknown (empty) version is +// earlier than 2.17.0, so it reports false like TS. func (c *ComposeClient) SupportsAdditionalContexts() bool { - if len(c.Version) < 2 { - return false - } - // 2.17.0 added additional_contexts - if c.Version[0] > 2 { - return true - } - if c.Version[0] == 2 && c.Version[1] >= 17 { - return true - } - return false + return !isEarlierVersion(c.Version, []int{2, 17, 0}) } -// UsesNewProjectNames returns true if compose version >= 1.21.0 -// (allowed hyphens and underscores in project names). +// UsesNewProjectNames returns true if compose version >= 1.21.0 (allowed +// hyphens and underscores in project names), or when the version is unknown +// (TS useNewProjectName: optimistic default). func (c *ComposeClient) UsesNewProjectNames() bool { - if len(c.Version) < 2 { - return true // optimistic default - } - if c.Version[0] > 1 { - return true - } - if c.Version[0] == 1 && c.Version[1] >= 21 { - return true - } - return false + return len(c.Version) == 0 || !isEarlierVersion(c.Version, []int{1, 21, 0}) } func (c *ComposeClient) buildGlobalArgs(composeFiles []string, envFile string) []string { @@ -235,15 +220,15 @@ func (c *ComposeClient) buildGlobalArgs(composeFiles []string, envFile string) [ return args } -// parseComposeVersion extracts the first N[.N[.N]] version found in s (e.g. from -// "Docker Compose version v2.15.1"), up to three numeric components. +// parseComposeVersion extracts the first N[.N]* version found in s (e.g. from +// "Docker Compose version v2.15.1"), every numeric component like TS parseVersion. func parseComposeVersion(s string) []int { i := strings.IndexFunc(s, func(r rune) bool { return r >= '0' && r <= '9' }) if i < 0 { return nil } var version []int - for len(version) < 3 { + for { j := i for j < len(s) && s[j] >= '0' && s[j] <= '9' { j++ @@ -269,7 +254,7 @@ func ToProjectName(basename string, newProjectName bool) string { case newProjectName && (r == '_' || r == '-'): return r } - return -1 + return -1 // mutate-exempt: strings.Map drops the rune for any negative value }, strings.ToLower(basename)) } diff --git a/internal/docker/compose_held_test.go b/internal/docker/compose_held_test.go new file mode 100644 index 0000000..e8a5c3a --- /dev/null +++ b/internal/docker/compose_held_test.go @@ -0,0 +1,70 @@ +package docker + +import ( + "errors" + "slices" + "testing" + + "github.com/devcontainers/cli/internal/log" +) + +// Down reports a compose process that could not run and one that exited +// non-zero, and succeeds only on exit 0. +func TestDownFailsUnlessComposeExitsZero(t *testing.T) { + newClient := func(r *fakeRunner) *ComposeClient { + return &ComposeClient{Command: []string{"docker", "compose"}, Log: log.Null, Runner: r} + } + + ok := &fakeRunner{} + if err := newClient(ok).Down(t.Context(), []string{"a.yml"}, "proj", true); err != nil { + t.Fatalf("Down = %v, want nil", err) + } + if want := []string{"compose", "-f", "a.yml", "--project-name", "proj", "down", "--volumes"}; !slices.Equal(ok.gotArgs, want) { + t.Errorf("args = %q, want %q", ok.gotArgs, want) + } + + sentinel := errors.New("exec: docker: not found") + if err := newClient(&fakeRunner{err: sentinel}).Down(t.Context(), nil, "", false); !errors.Is(err, sentinel) { + t.Errorf("Down with a failed run = %v, want it to wrap %v", err, sentinel) + } + if err := newClient(&fakeRunner{code: 1, stderr: []byte("boom")}).Down(t.Context(), nil, "", false); err == nil { + t.Error("Down with exit 1 = nil, want an error") + } +} + +// TS parseVersion (spec-common/commonUtils.ts) keeps every numeric component. +func TestParseComposeVersionKeepsEveryComponent(t *testing.T) { + if got, want := parseComposeVersion("2.24.5.1"), []int{2, 24, 5, 1}; !slices.Equal(got, want) { + t.Errorf("parseComposeVersion = %v, want %v", got, want) + } +} + +// TS compares the whole version with isEarlierVersion, a missing component +// counting as 0: useNewProjectName (dockerCompose.ts) is +// !isEarlierVersion(version, [1, 21, 0]), optimistic when the version is +// unknown, and additional build contexts need !isEarlierVersion(version, +// [2, 17, 0]) (buildAndExtendDockerCompose). A one-component version is +// therefore 1.0.0, 2.0.0 or 3.0.0, not "unknown". +func TestComposeVersionGatesCountAMissingComponentAsZero(t *testing.T) { + for _, tc := range []struct { + version []int + newProjectNames, addlContexts bool + }{ + {nil, true, false}, + {[]int{1}, false, false}, + {[]int{1, 20}, false, false}, + {[]int{1, 21}, true, false}, + {[]int{2}, true, false}, + {[]int{2, 16, 9}, true, false}, + {[]int{2, 17}, true, true}, + {[]int{3}, true, true}, + } { + c := &ComposeClient{Version: tc.version} + if got := c.UsesNewProjectNames(); got != tc.newProjectNames { + t.Errorf("UsesNewProjectNames(%v) = %v, want %v", tc.version, got, tc.newProjectNames) + } + if got := c.SupportsAdditionalContexts(); got != tc.addlContexts { + t.Errorf("SupportsAdditionalContexts(%v) = %v, want %v", tc.version, got, tc.addlContexts) + } + } +} diff --git a/internal/docker/context_held_test.go b/internal/docker/context_held_test.go new file mode 100644 index 0000000..5961371 --- /dev/null +++ b/internal/docker/context_held_test.go @@ -0,0 +1,47 @@ +package docker + +import ( + "os" + "path/filepath" + "testing" +) + +// "default" is the Docker CLI's built-in context: its endpoint comes from the +// environment, never from the context store, even if a meta.json exists for it. +func TestResolveDockerContextHostIgnoresAStoredDefault(t *testing.T) { + dir := t.TempDir() + t.Setenv("DOCKER_CONFIG", dir) + t.Setenv("DOCKER_CONTEXT", "default") + writeContext(t, dir, "default", "tcp://10.0.0.9:2376") + if got := resolveDockerContextHost(); got != "" { + t.Errorf("host = %q, want empty for the default context", got) + } +} + +// A config.json without currentContext selects "default". +func TestActiveContextNameDefaultsWithoutCurrentContext(t *testing.T) { + dir := t.TempDir() + t.Setenv("DOCKER_CONTEXT", "") + if err := os.WriteFile(filepath.Join(dir, "config.json"), []byte(`{"auths":{}}`), 0o644); err != nil { + t.Fatal(err) + } + if got := activeContextName(dir); got != "default" { + t.Errorf("activeContextName = %q, want default", got) + } +} + +// Without DOCKER_CONFIG the store is ~/.docker, as for the Docker CLI. +func TestResolveDockerContextHostReadsTheHomeStore(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("DOCKER_CONFIG", "") + t.Setenv("DOCKER_CONTEXT", "") + dir := filepath.Join(home, ".docker") + writeContext(t, dir, "remote", "tcp://10.0.0.7:2376") + if err := os.WriteFile(filepath.Join(dir, "config.json"), []byte(`{"currentContext":"remote"}`), 0o644); err != nil { + t.Fatal(err) + } + if got := resolveDockerContextHost(); got != "tcp://10.0.0.7:2376" { + t.Errorf("host = %q, want tcp://10.0.0.7:2376 from ~/.docker", got) + } +} diff --git a/internal/docker/dockerfile.go b/internal/docker/dockerfile.go index ae7a840..779b76c 100644 --- a/internal/docker/dockerfile.go +++ b/internal/docker/dockerfile.go @@ -1,6 +1,8 @@ package docker import ( + "maps" + "slices" "strings" "github.com/moby/buildkit/frontend/dockerfile/instructions" @@ -78,7 +80,6 @@ func ExtractDockerfile(content string) *Dockerfile { } } - df.Stages = make([]Stage, 0, len(stages)) for i := range stages { s := &stages[i] st := Stage{From: From{Platform: s.Platform, Image: trimQuotes(s.BaseName), Label: s.Name}} @@ -163,13 +164,7 @@ func derefTrim(p *string) string { type mapEnv map[string]string func (m mapEnv) Get(k string) (string, bool) { v, ok := m[k]; return v, ok } -func (m mapEnv) Keys() []string { - ks := make([]string, 0, len(m)) - for k := range m { - ks = append(ks, k) - } - return ks -} +func (m mapEnv) Keys() []string { return slices.Collect(maps.Keys(m)) } // FindBaseImage resolves the base image for a target stage (or the last stage), // following FROM references to the root image. ${VAR} / ${VAR:+x} / @@ -225,7 +220,9 @@ func EnsureFinalStageName(content, defaultName string) (stageName string, modifi // on a parse error (e.g. a trailing inline comment the strict parser rejects // but the TS tolerates) or no name, fall through to the tolerant scan below. if res, err := parser.Parse(strings.NewReader(content)); err == nil { - if stages, _, perr := instructions.Parse(res.AST, nil); perr == nil && len(stages) > 0 { + // instructions.Parse returns no stages with its error, so an error lands in + // the tolerant scan like "no stages" does. + if stages, _, _ := instructions.Parse(res.AST, nil); len(stages) > 0 { if name := stages[len(stages)-1].Name; name != "" { return name, "" } @@ -236,7 +233,11 @@ func EnsureFinalStageName(content, defaultName string) (stageName string, modifi // it by hand (no regex). If it already has an "AS