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()) }