From 0aed5a515551f2d0f8f7d510844cb3997c4467fb Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Thu, 12 Nov 2026 22:03:47 -0300 Subject: [PATCH] fix(features): keep host variables raw in the metadata label A Feature's mounts were resolved on the build host before the image's devcontainer.metadata label was written, so the image carried that host's paths (${localEnv:HOME} became /home/) and resolved them the same way on every other host. The metadata substitution also wrote its result back through mounts and maps it shared with the label's entries, so in compose it rewrote base-image mounts in the label too. As the reference CLI does, the label now keeps each Feature's values as the Feature declares them, and up resolves them on the host it runs on, from a copy: for single-container and compose, where Feature lifecycle commands with host variables used to run unresolved, and in read-configuration's merged configuration, which printed them raw. Only the Dockerfile ENV line for a Feature's containerEnv stays resolved at build time (upstream #308). Three runtime parity cases compare the label, the compose labels and the merged configuration with the reference. Co-Authored-By: Claude Opus 5.5 --- docs/parity/parity-matrix.yaml | 42 +++++++++++++++++++ internal/cli/compose_up.go | 4 +- internal/cli/compose_up_held_test.go | 29 +++++++++++++ internal/cli/e2e_fixes_test.go | 18 +++++++- internal/cli/feature_hostsub_test.go | 4 +- internal/cli/feature_install.go | 15 +------ .../cli/feature_install_nodocker_held_test.go | 7 +++- internal/cli/metadata_label.go | 23 ++++++++-- internal/cli/metadata_label_test.go | 23 ++++++++++ internal/cli/read_configuration.go | 3 ++ internal/cli/up.go | 2 +- 11 files changed, 146 insertions(+), 24 deletions(-) diff --git a/docs/parity/parity-matrix.yaml b/docs/parity/parity-matrix.yaml index 6f848ce..4a04b3a 100644 --- a/docs/parity/parity-matrix.yaml +++ b/docs/parity/parity-matrix.yaml @@ -1093,6 +1093,48 @@ initial_cases: current_status: match notes: "The compose service's Features Dockerfile (appended to `FROM AS dev_container_auto_added_stage_label`) has the same structure as the image build's: builtin env, consecutiveId folders, install wrapper, the Feature's containerEnv and, like TS getFeaturesBuildOptions, the config's containerEnv as ENV lines (CFG_VAR). Go used the _dev_container_feature_ layout and left the config's containerEnv out of the compose image." + - id: build.feature-host-variables-raw-label-success + 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/f; printf '%s' '{\"id\":\"f\",\"version\":\"1.0.0\",\"mounts\":[{\"source\":\"${localEnv:HOME}\",\"target\":\"/parity-home\",\"type\":\"bind\"}],\"postCreateCommand\":\"echo ${localEnv:HOME}\"}' > $WORKSPACE/.devcontainer/f/devcontainer-feature.json; printf '#!/bin/sh\\ntrue\\n' > $WORKSPACE/.devcontainer/f/install.sh; chmod +x $WORKSPACE/.devcontainer/f/install.sh; printf '%s' '{\"image\":\"alpine:3\",\"features\":{\"./f\":{}}}' > $WORKSPACE/.devcontainer/devcontainer.json; echo WORKSPACE=$WORKSPACE" + ts_cmd: "build --workspace-folder ${WORKSPACE} --image-name parity-${PARITY_CASE_ID}-${PARITY_SIDE}" + verify_cmd: "docker image inspect -f '{{index .Config.Labels \"devcontainer.metadata\"}}' ${IMAGE_NAME}" + cleanup_cmd: "docker rmi -f parity-${PARITY_CASE_ID}-${PARITY_SIDE} >/dev/null 2>&1; rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; true" + asserts: [exit_code, stdout_normalized] + class: build-runtime + current_status: match + notes: "TS getDevcontainerMetadata writes the Feature's raw values to the devcontainer.metadata label (imageMetadata.ts uses .raw), so a Feature mount and lifecycle command keep ${localEnv:HOME} and are resolved on the host that runs `up`. Go resolved the Feature's mounts before building the label, baking the build host's HOME into the image." + + - id: up.compose-feature-host-variables-raw-label-success + lane: runtime + command: up + priority: p1 + docker_required: true + setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE/.devcontainer/f; printf '%s' '{\"id\":\"f\",\"version\":\"1.0.0\",\"mounts\":[{\"source\":\"${localEnv:HOME}\",\"target\":\"/parity-home\",\"type\":\"bind\"}],\"postCreateCommand\":\"echo ${localEnv:HOME}\"}' > $WORKSPACE/.devcontainer/f/devcontainer-feature.json; printf '#!/bin/sh\\ntrue\\n' > $WORKSPACE/.devcontainer/f/install.sh; chmod +x $WORKSPACE/.devcontainer/f/install.sh; printf 'services:\\n app:\\n image: alpine:3\\n command: sleep infinity\\n' > $WORKSPACE/.devcontainer/docker-compose.yml; printf '%s' '{\"dockerComposeFile\":\"docker-compose.yml\",\"service\":\"app\",\"workspaceFolder\":\"/\",\"features\":{\"./f\":{}}}' > $WORKSPACE/.devcontainer/devcontainer.json; echo WORKSPACE=$WORKSPACE" + ts_cmd: "up --workspace-folder ${WORKSPACE} --skip-post-create --update-remote-user-uid-default never --id-label parity.case=${PARITY_CASE_ID}-${PARITY_SIDE}" + verify_cmd: "docker inspect -f '{{index .Config.Labels \"devcontainer.metadata\"}}' ${CONTAINER_ID}; docker image inspect -f '{{index .Config.Labels \"devcontainer.metadata\"}}' $(docker inspect -f '{{.Image}}' ${CONTAINER_ID}); docker inspect -f '{{range .Mounts}}{{.Source}}->{{.Destination}} {{end}}' ${CONTAINER_ID}" + cleanup_cmd: "docker compose --project-name ${COMPOSE_PROJECT_NAME} -f ${WORKSPACE}/.devcontainer/docker-compose.yml down -t 0 >/dev/null 2>&1; rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; true" + asserts: [exit_code, stdout_normalized] + class: up-runtime + current_status: match + notes: "The compose service's image and container labels keep the Feature's ${localEnv:HOME} raw, as TS writes them; the bind mount the container gets is resolved on this host (TS getDevcontainerMetadata's featureRaw.map(substitute)). Go resolved the Feature's mounts before building the label." + + - id: read-configuration.feature-host-variables-merged-success + lane: runtime + command: read-configuration + priority: p1 + docker_required: true + setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE/.devcontainer/f; printf '%s' '{\"id\":\"f\",\"version\":\"1.0.0\",\"mounts\":[{\"source\":\"${localEnv:HOME}\",\"target\":\"/parity-home\",\"type\":\"bind\"}],\"postCreateCommand\":\"echo ${localEnv:HOME}\"}' > $WORKSPACE/.devcontainer/f/devcontainer-feature.json; printf '#!/bin/sh\\ntrue\\n' > $WORKSPACE/.devcontainer/f/install.sh; chmod +x $WORKSPACE/.devcontainer/f/install.sh; printf '%s' '{\"image\":\"alpine:3\",\"features\":{\"./f\":{}}}' > $WORKSPACE/.devcontainer/devcontainer.json; echo WORKSPACE=$WORKSPACE" + ts_cmd: "read-configuration --workspace-folder ${WORKSPACE} --include-merged-configuration" + verify_cmd: "true" + cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}" + asserts: [exit_code, stdout_normalized] + class: merged-configuration + current_status: match + notes: "Without a container, TS getDevcontainerMetadata resolves the Features' raw entries with the configuration's host substitution, so mergedConfiguration lists the Feature's mount and postCreateCommand with ${localEnv:HOME} resolved. Go listed them raw." + - id: read-configuration.container-id-missing-error lane: runtime command: read-configuration diff --git a/internal/cli/compose_up.go b/internal/cli/compose_up.go index bf999da..630d738 100644 --- a/internal/cli/compose_up.go +++ b/internal/cli/compose_up.go @@ -399,11 +399,11 @@ func (r *upRunner) buildAndExtendCompose(ctx context.Context, s composeStart, pe featureEntries = append(featureEntries, featureMetadataEntry(fs, s.skipPersistCustoms)) } labelMetadata := slices.Concat(baseEntries, featureEntries, []imagemeta.Entry{r.configLabel}) - metadata := slices.Clone(baseEntries) + metadata := slices.Concat(baseEntries, featureEntries) if err := r.substitute(&metadata); err != nil { return nil, fmt.Errorf("substitute the image's metadata: %w", err) } - metadata = slices.Concat(metadata, featureEntries, []imagemeta.Entry{configMetadataEntry(cfg, false)}) + metadata = append(metadata, configMetadataEntry(cfg, false)) result := &composeExtendResult{imageMetadata: metadata} userSyntax := "" diff --git a/internal/cli/compose_up_held_test.go b/internal/cli/compose_up_held_test.go index d9a0a16..9619468 100644 --- a/internal/cli/compose_up_held_test.go +++ b/internal/cli/compose_up_held_test.go @@ -1278,3 +1278,32 @@ func TestBuildAndExtendComposeForTheBuildCommand(t *testing.T) { } } } + +func TestBuildAndExtendComposeKeepsFeatureHostVariablesRawInTheLabel(t *testing.T) { + h := newComposeHarness(t) + h.write("vars/devcontainer-feature.json", `{"id":"vars","version":"1.0.0","containerEnv":{"FE":"${localEnv:SRC}/env"},"mounts":[{"source":"${localEnv:SRC}","target":"/m","type":"bind"}],"postCreateCommand":"echo ${localEnv:SRC}"}`) + h.write("vars/install.sh", "#!/bin/sh\n") + s := h.start(&config.DevContainer{Features: map[string]any{"./vars": map[string]any{}}}, map[string]any{"image": "alpine:3"}) + s.loadResult.HostSub = config.HostSubContext{Env: map[string]string{"SRC": "/host/src"}} + h.r.substitute = hostMetadataSubstitution(s.loadResult.HostSub, s.idLabels) + res, err := h.r.buildAndExtendCompose(context.Background(), s, h.persisted, "", true) + if err != nil { + t.Fatal(err) + } + content := readFile(t, overrideDockerfile(t, readFile(t, res.overrideFiles[0]))) + for _, want := range []string{`\"source\":\"\${localEnv:SRC}\"`, `\"postCreateCommand\":\"echo \${localEnv:SRC}\"`, `ENV FE="/host/src/env"`} { + if !strings.Contains(content, want) { + t.Errorf("Dockerfile lacks %s:\n%s", want, content) + } + } + if strings.Contains(content, `\"source\":\"/host/src\"`) { + t.Errorf("the label carries the build host's mount source:\n%s", content) + } + feature := res.imageMetadata[1] + if got := feature.Mounts[0].(map[string]any)["source"]; got != "/host/src" { + t.Errorf("the container's Feature mount source = %v, want /host/src", got) + } + if feature.PostCreateCommand != "echo /host/src" { + t.Errorf("the container's Feature postCreateCommand = %v, want echo /host/src", feature.PostCreateCommand) + } +} diff --git a/internal/cli/e2e_fixes_test.go b/internal/cli/e2e_fixes_test.go index 3abd1b1..e1fb61e 100644 --- a/internal/cli/e2e_fixes_test.go +++ b/internal/cli/e2e_fixes_test.go @@ -44,6 +44,11 @@ func TestE2E_FeatureUserHomeAndLocalEnv(t *testing.T) { t.Skip("Docker not available") } t.Setenv("E2E_LOCALENV_MARKER", "resolved-localenv-1234") + mountSrc := t.TempDir() + if err := os.WriteFile(filepath.Join(mountSrc, "marker"), []byte("mounted-from-localenv"), 0o644); err != nil { + t.Fatal(err) + } + t.Setenv("E2E_LOCALENV_MOUNT", mountSrc) ws := writeFixture(t, map[string]string{ ".devcontainer/devcontainer.json": `{ @@ -55,7 +60,8 @@ func TestE2E_FeatureUserHomeAndLocalEnv(t *testing.T) { "id": "localfeat", "version": "1.0.0", "name": "localfeat", - "containerEnv": { "MY_LOCAL_ENV": "${localEnv:E2E_LOCALENV_MARKER}" } + "containerEnv": { "MY_LOCAL_ENV": "${localEnv:E2E_LOCALENV_MARKER}" }, + "mounts": [{ "source": "${localEnv:E2E_LOCALENV_MOUNT}", "target": "/e2e-localenv-mount", "type": "bind" }] }`, ".devcontainer/localfeat/install.sh": "#!/bin/sh\nset -e\n" + `echo "REMOTE_USER_HOME=[$_REMOTE_USER_HOME]" > /e2e-feature-marker` + "\n", @@ -75,6 +81,16 @@ func TestE2E_FeatureUserHomeAndLocalEnv(t *testing.T) { if got := dockerExec(t, id, "printenv MY_LOCAL_ENV"); got != "resolved-localenv-1234" { t.Errorf("#308 Feature containerEnv ${localEnv} = %q, want resolved-localenv-1234", got) } + if got := dockerExec(t, id, "cat /e2e-localenv-mount/marker"); got != "mounted-from-localenv" { + t.Errorf("Feature mount ${localEnv} not resolved on the host running up: %q", got) + } + out, err := exec.Command("sh", "-c", `docker image inspect -f '{{index .Config.Labels "devcontainer.metadata"}}' "$(docker inspect -f '{{.Image}}' `+id+`)"`).CombinedOutput() + if err != nil { + t.Fatalf("inspect the container's image: %v\n%s", err, out) + } + if label := string(out); !strings.Contains(label, `"source":"${localEnv:E2E_LOCALENV_MOUNT}"`) || strings.Contains(label, mountSrc) { + t.Errorf("image label = %s, want the Feature's mount source raw", label) + } } func TestE2E_RemoveExistingBeforeInitialize(t *testing.T) { diff --git a/internal/cli/feature_hostsub_test.go b/internal/cli/feature_hostsub_test.go index 574bc67..08311c9 100644 --- a/internal/cli/feature_hostsub_test.go +++ b/internal/cli/feature_hostsub_test.go @@ -31,7 +31,7 @@ func TestSubstituteFeatureHostVars(t *testing.T) { if got := f.ContainerEnv["WS"]; got != "/home/me/proj/x" { t.Errorf("containerEnv WS = %q, want /home/me/proj/x", got) } - if got := f.Mounts[0].(string); got != "source=/home/me/.ssh,target=/root/.ssh,type=bind" { - t.Errorf("mount = %q, want resolved localEnv:HOME", got) + if got := f.Mounts[0].(string); got != "source=${localEnv:HOME}/.ssh,target=/root/.ssh,type=bind" { + t.Errorf("mount = %q, want it raw", got) } } diff --git a/internal/cli/feature_install.go b/internal/cli/feature_install.go index bea61bc..20287bd 100644 --- a/internal/cli/feature_install.go +++ b/internal/cli/feature_install.go @@ -548,10 +548,8 @@ func engineAtLeast23(ctx context.Context, engine *docker.EngineClient) bool { } // substituteFeatureHostVars resolves host-side variables (${localEnv:…}, -// ${localWorkspaceFolder}, …) in each Feature's containerEnv values and mounts, -// so a Feature mount like "${localEnv:HOME}/.cfg" reaches Docker resolved — -// matching how the config's own containerEnv/mounts are substituted (upstream -// #308, which only substituted devcontainer.json's own values). +// ${localWorkspaceFolder}, …) in each Feature's containerEnv, which the +// Features Dockerfile writes as ENV lines (upstream #308). func substituteFeatureHostVars(sets []*features.Set, hs config.HostSubContext) error { resolver := config.NewVariableResolver() ctx := config.SubstitutionContext{HostSubContext: hs} @@ -567,15 +565,6 @@ func substituteFeatureHostVars(sets []*features.Set, hs config.HostSubContext) e f.ContainerEnv[k] = s } } - if len(f.Mounts) > 0 { - resolved, err := resolver.Resolve(ctx, config.PhaseHost, f.Mounts) - if err != nil { - return fmt.Errorf("resolve Feature %s mounts: %w", f.ID, err) - } - if m, ok := resolved.([]any); ok { - f.Mounts = m - } - } } } return nil diff --git a/internal/cli/feature_install_nodocker_held_test.go b/internal/cli/feature_install_nodocker_held_test.go index dd3861f..334793d 100644 --- a/internal/cli/feature_install_nodocker_held_test.go +++ b/internal/cli/feature_install_nodocker_held_test.go @@ -21,7 +21,7 @@ func TestExtendImageWithFeaturesResolvesFeatureHostVars(t *testing.T) { if err := os.MkdirAll(dir, 0o755); err != nil { t.Fatal(err) } - if err := os.WriteFile(filepath.Join(dir, "devcontainer-feature.json"), []byte(`{"id":"envy","version":"1.0.0","containerEnv":{"FROM_HOST":"${localEnv:HOST_VALUE}"}}`), 0o644); err != nil { + if err := os.WriteFile(filepath.Join(dir, "devcontainer-feature.json"), []byte(`{"id":"envy","version":"1.0.0","containerEnv":{"FROM_HOST":"${localEnv:HOST_VALUE}"},"mounts":[{"source":"${localEnv:HOST_VALUE}","target":"/m","type":"bind"}]}`), 0o644); err != nil { t.Fatal(err) } if err := os.WriteFile(filepath.Join(dir, "install.sh"), []byte("#!/bin/sh\n"), 0o755); err != nil { @@ -53,9 +53,12 @@ func TestExtendImageWithFeaturesResolvesFeatureHostVars(t *testing.T) { if err != nil { t.Fatalf("extended Dockerfile: %v (calls %v)", err, r.calls) } - if !strings.Contains(string(data), "resolved-on-host") || strings.Contains(string(data), "localEnv") { + if !strings.Contains(string(data), `ENV FROM_HOST="resolved-on-host"`) { t.Errorf("extended Dockerfile = %s, want FROM_HOST resolved from the host", data) } + if !strings.Contains(string(data), `\"source\":\"\${localEnv:HOST_VALUE}\"`) || strings.Contains(string(data), `\"source\":\"resolved-on-host\"`) { + t.Errorf("extended Dockerfile = %s, want the label's Feature mount raw", data) + } } func TestExtendImageWithFeaturesBuildContext(t *testing.T) { diff --git a/internal/cli/metadata_label.go b/internal/cli/metadata_label.go index 343789a..8f05cf5 100644 --- a/internal/cli/metadata_label.go +++ b/internal/cli/metadata_label.go @@ -53,13 +53,30 @@ type metadataSubstitution func(entries *[]imagemeta.Entry) error func hostMetadataSubstitution(hostSub config.HostSubContext, idLabels []string) metadataSubstitution { return func(entries *[]imagemeta.Entry) error { + // Resolve a copy: decoding into *entries would write through the + // mounts, commands and maps they share with the raw entries that + // become the devcontainer.metadata label. + data, err := json.Marshal(*entries) + if err != nil { + return fmt.Errorf("copy metadata entries: %w", err) + } + var resolved []imagemeta.Entry + if err := json.Unmarshal(data, &resolved); err != nil { + return fmt.Errorf("copy metadata entries: %w", err) + } resolver := config.NewVariableResolver() ctx := config.SubstitutionContext{HostSubContext: hostSub} if idLabels == nil { - return resolver.ResolveInto(ctx, config.PhaseHost, entries) + err = resolver.ResolveInto(ctx, config.PhaseHost, &resolved) + } else { + ctx.DevContainerID = config.ComputeDevContainerID(config.EnvMap(idLabels)) + err = resolver.BeforeContainerInto(ctx, &resolved) + } + if err != nil { + return err } - ctx.DevContainerID = config.ComputeDevContainerID(config.EnvMap(idLabels)) - return resolver.BeforeContainerInto(ctx, entries) + *entries = resolved + return nil } } diff --git a/internal/cli/metadata_label_test.go b/internal/cli/metadata_label_test.go index dd69fca..8cae016 100644 --- a/internal/cli/metadata_label_test.go +++ b/internal/cli/metadata_label_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "errors" "reflect" + "slices" "testing" "github.com/devcontainers/cli/internal/config" @@ -141,3 +142,25 @@ func TestHostMetadataSubstitution(t *testing.T) { t.Errorf("with id labels: ID = %q, want %q", entries[0].ContainerEnv["ID"], want) } } + +func TestHostMetadataSubstitutionLeavesTheRawEntriesRaw(t *testing.T) { + raw := []imagemeta.Entry{{ + ID: "f", + Mounts: []any{map[string]any{"source": "${localEnv:SRC}", "target": "/m", "type": "bind"}}, + PostCreateCommand: []any{"echo", "${localEnv:SRC}"}, + Customizations: map[string]any{"x": "${localEnv:SRC}"}, + }} + label := imagemeta.GenerateMetadataLabel(raw) + resolved := slices.Clone(raw) + for _, idLabels := range [][]string{nil, {"devcontainer.local_folder=/ws"}} { + if err := hostMetadataSubstitution(config.HostSubContext{Env: map[string]string{"SRC": "/host/src"}}, idLabels)(&resolved); err != nil { + t.Fatal(err) + } + if got := resolved[0].Mounts[0].(map[string]any)["source"]; got != "/host/src" { + t.Errorf("idLabels %v: resolved mount source = %v, want /host/src", idLabels, got) + } + if got := imagemeta.GenerateMetadataLabel(raw); got != label { + t.Errorf("idLabels %v: the raw entries changed:\n got %s\nwant %s", idLabels, got, label) + } + } +} diff --git a/internal/cli/read_configuration.go b/internal/cli/read_configuration.go index 7afcba4..e632c0e 100644 --- a/internal/cli/read_configuration.go +++ b/internal/cli/read_configuration.go @@ -210,6 +210,9 @@ func runReadConfiguration(ctx context.Context, out Output, opts *readConfigOpts) for _, fs := range featResult.FeatureSets { featureEntries = append(featureEntries, featureMetadataEntry(fs, false)) } + if err := hostMetadataSubstitution(result.HostSub, nil)(&featureEntries); err != nil { + return readConfigurationFailure(lgr, fmt.Errorf("substitute the Features' metadata: %w", err)) + } } entries, entriesErr := readConfigurationMetadata(append(base, featureEntries...), featureEntries, containerCfg, result, opts.idLabels, workspaceFolder, lgr) if entriesErr != nil { diff --git a/internal/cli/up.go b/internal/cli/up.go index 43b687e..05fa4d0 100644 --- a/internal/cli/up.go +++ b/internal/cli/up.go @@ -305,7 +305,7 @@ func runUp(ctx context.Context, out Output, opts *upOpts) error { return writeErrorResult(out, fmt.Sprintf("apply workspace pre-container substitutions: %v", err)) } run.idLabels = idLabels - run.substitute = func(entries *[]imagemeta.Entry) error { return resolver.BeforeContainerInto(idCtx, entries) } + run.substitute = hostMetadataSubstitution(loadResult.HostSub, idLabels) if run.configLabel, err = configLabelEntry(loadResult.RawConfig, opts.omitConfigRemoteEnvFromMeta); err != nil { return writeErrorResult(out, err.Error()) }