From cf21a9c86b3fa387bef9cc6c52d1fb90adb9c1cb Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Fri, 13 Nov 2026 01:42:50 -0300 Subject: [PATCH 1/2] fix(cli): let --secrets-file values win over remoteEnv in hooks The reference runs every lifecycle hook with {...remoteEnv, ...secrets}, where remoteEnv is the probed env, then --remote-env, then the config's remoteEnv. up and run-user-commands put the secrets before both remoteEnv sources, so a remoteEnv entry of the same name hid the secret. Co-Authored-By: Claude Opus 5.5 --- docs/parity/parity-matrix.yaml | 28 +++++++++++++++++++++ internal/cli/run_user_commands.go | 22 ++++------------ internal/cli/run_user_commands_held_test.go | 16 ------------ internal/cli/setup.go | 2 +- internal/cli/up.go | 9 +++---- internal/cli/up_held_test.go | 10 +++++--- 6 files changed, 45 insertions(+), 42 deletions(-) diff --git a/docs/parity/parity-matrix.yaml b/docs/parity/parity-matrix.yaml index 0307aa9..22e7dd8 100644 --- a/docs/parity/parity-matrix.yaml +++ b/docs/parity/parity-matrix.yaml @@ -3061,6 +3061,20 @@ initial_cases: class: lifecycle-runtime current_status: match + - id: run-user-commands.secrets-override-remote-env-success + lane: runtime + command: run-user-commands + priority: p1 + docker_required: true + ts_cmd: "run-user-commands --workspace-folder ${WORKSPACE} --secrets-file ${SECRETS_FILE} --remote-env TOKEN=cli --remote-env CONFIG_WINS=cli --id-label parity.case=${PARITY_CASE_ID}-${PARITY_SIDE}" + setup_cmd: "WORKSPACE=/tmp/parity-secrets-env-run-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf ${WORKSPACE}; mkdir -p ${WORKSPACE}; printf '%s' '{\"image\":\"alpine:3.20\",\"overrideCommand\":true,\"remoteEnv\":{\"TOKEN\":\"config\",\"CONFIG_WINS\":\"config\"},\"postCreateCommand\":\"printenv TOKEN CONFIG_WINS > env.postCreateCommand.testMarker\"}' > ${WORKSPACE}/.devcontainer.json; SECRETS_FILE=${WORKSPACE}/test-secrets-temp.json; printf '%s' '{\"TOKEN\":\"SecretValue1\"}' > ${SECRETS_FILE}; if [ \"${PARITY_SIDE}\" = \"ts\" ]; then ${PARITY_CLI_TS} up --workspace-folder ${WORKSPACE} --skip-post-create --id-label parity.case=${PARITY_CASE_ID}-${PARITY_SIDE} --remove-existing-container >/dev/null; else ${PARITY_CLI_GO} up --workspace-folder ${WORKSPACE} --skip-post-create --id-label parity.case=${PARITY_CASE_ID}-${PARITY_SIDE} --remove-existing-container >/dev/null; fi; echo WORKSPACE=$WORKSPACE; echo SECRETS_FILE=$SECRETS_FILE" + verify_cmd: "cat ${WORKSPACE}/env.postCreateCommand.testMarker" + cleanup_cmd: "IDS=$(docker ps -aq --filter label=parity.case=${PARITY_CASE_ID}-${PARITY_SIDE}); [ -n \"$IDS\" ] && docker rm -f $IDS >/dev/null 2>&1 || true; rm -rf ${WORKSPACE}" + asserts: [exit_code, stdout_normalized] + class: lifecycle-runtime + current_status: match + notes: "Same precedence as up.secrets-override-remote-env-success: run-user-commands hands runLifecycleHooks the probed remoteEnv and the secrets, and each hook runs with {...remoteEnv, ...secrets}." + - id: run-user-commands.workspace-subfolder-config-success lane: runtime command: run-user-commands @@ -3794,6 +3808,20 @@ initial_cases: current_status: match arm64_required: true + - id: up.secrets-override-remote-env-success + lane: runtime + command: up + priority: p1 + docker_required: true + ts_cmd: "up --workspace-folder ${WORKSPACE} --secrets-file ${SECRETS_FILE} --remote-env TOKEN=cli --remote-env CONFIG_WINS=cli --id-label parity.case=${PARITY_CASE_ID}-${PARITY_SIDE} --remove-existing-container" + setup_cmd: "WORKSPACE=/tmp/parity-secrets-env-up-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf ${WORKSPACE}; mkdir -p ${WORKSPACE}; printf '%s' '{\"image\":\"alpine:3.20\",\"overrideCommand\":true,\"remoteEnv\":{\"TOKEN\":\"config\",\"CONFIG_WINS\":\"config\"},\"postCreateCommand\":\"printenv TOKEN CONFIG_WINS > env.postCreateCommand.testMarker\"}' > ${WORKSPACE}/.devcontainer.json; SECRETS_FILE=${WORKSPACE}/test-secrets-temp.json; printf '%s' '{\"TOKEN\":\"SecretValue1\"}' > ${SECRETS_FILE}; echo WORKSPACE=$WORKSPACE; echo SECRETS_FILE=$SECRETS_FILE" + verify_cmd: "cat ${WORKSPACE}/env.postCreateCommand.testMarker" + cleanup_cmd: "docker rm -f ${CONTAINER_ID} >/dev/null 2>&1 || true; rm -rf ${WORKSPACE}" + asserts: [exit_code, stdout_normalized] + class: up-runtime + current_status: match + notes: "Lifecycle hooks see {...remoteEnv, ...secrets} (injectHeadless.ts runLifecycleCommand), where remoteEnv is the probed env, then --remote-env, then the config's remoteEnv (probeRemoteEnv): a --secrets-file value beats both remoteEnv sources of the same name, and the config's remoteEnv beats --remote-env. Go put the secrets before both, so remoteEnv won." + - id: up.workspace-secrets-success lane: runtime command: up diff --git a/internal/cli/run_user_commands.go b/internal/cli/run_user_commands.go index c01ef2b..ee986ad 100644 --- a/internal/cli/run_user_commands.go +++ b/internal/cli/run_user_commands.go @@ -222,7 +222,11 @@ func runUserCommandsInContainer(ctx context.Context, out Output, opts *runUserCo remoteUser := lifecycleRemoteUser(merged, cfg) probed := lifecycle.ProbedRemoteEnv(ctx, engine, logger, containerID, remoteUser, lifecycleUserEnvProbe(opts.defaultUserEnvProbe, cfg), opts.containerSessionDataFolder) - remoteEnv := lifecycleRemoteEnv(probed, opts.secrets.env(), opts.remoteEnvs, cfg) + var cfgRemoteEnv map[string]*string + if cfg != nil { + cfgRemoteEnv = cfg.RemoteEnv + } + remoteEnv := hookRemoteEnv(probed, opts.remoteEnvs, cfgRemoteEnv, opts.secrets.env()) shellServer := lifecycle.NewShellServer(ctx, engine, containerID, remoteUser, remoteEnv...) defer shellServer.Close() @@ -280,19 +284,3 @@ func lifecycleUserEnvProbe(defaultProbe string, cfg *config.DevContainer) lifecy return lifecycle.UserEnvProbeStrategy(defaultProbe) } -func lifecycleRemoteEnv(probed map[string]string, secrets, remoteEnvs []string, cfg *config.DevContainer) []string { - var env []string - for k, v := range probed { - env = append(env, fmt.Sprintf("%s=%s", k, v)) - } - env = append(env, secrets...) - env = append(env, remoteEnvs...) - if cfg != nil { - for k, v := range cfg.RemoteEnv { - if v != nil { - env = append(env, fmt.Sprintf("%s=%s", k, *v)) - } - } - } - return env -} diff --git a/internal/cli/run_user_commands_held_test.go b/internal/cli/run_user_commands_held_test.go index 4c577c7..8b629d5 100644 --- a/internal/cli/run_user_commands_held_test.go +++ b/internal/cli/run_user_commands_held_test.go @@ -3,7 +3,6 @@ package cli import ( "os" "path/filepath" - "slices" "strings" "testing" @@ -183,18 +182,3 @@ func TestLifecycleUserEnvProbe(t *testing.T) { } } } - -func TestLifecycleRemoteEnvOrder(t *testing.T) { - probed := map[string]string{"PATH": "/probed"} - secrets := []string{"TOKEN=s"} - cli := []string{"PATH=/cli"} - value := "/config" - cfg := &config.DevContainer{RemoteEnv: map[string]*string{"PATH": &value, "UNSET": nil}} - - if got, want := lifecycleRemoteEnv(probed, secrets, cli, cfg), []string{"PATH=/probed", "TOKEN=s", "PATH=/cli", "PATH=/config"}; !slices.Equal(got, want) { - t.Errorf("with config: %v, want %v", got, want) - } - if got, want := lifecycleRemoteEnv(probed, secrets, cli, nil), []string{"PATH=/probed", "TOKEN=s", "PATH=/cli"}; !slices.Equal(got, want) { - t.Errorf("without config: %v, want %v", got, want) - } -} diff --git a/internal/cli/setup.go b/internal/cli/setup.go index 3627aef..6f1b0e1 100644 --- a/internal/cli/setup.go +++ b/internal/cli/setup.go @@ -159,7 +159,7 @@ func setUpContainer(ctx context.Context, out Output, engine *docker.EngineClient } probeStrategy := lifecycle.UserEnvProbeStrategy(cmp.Or(cfgProbe, opts.defaultUserEnvProbe)) probed := lifecycle.ProbedRemoteEnv(ctx, engine, logger, containerID, merged.RemoteUser, probeStrategy, opts.containerSessionDataFolder) - shellServer := lifecycle.NewShellServer(ctx, engine, containerID, merged.RemoteUser, hookRemoteEnv(probed, opts.remoteEnvs, cfgRemoteEnv)...) + shellServer := lifecycle.NewShellServer(ctx, engine, containerID, merged.RemoteUser, hookRemoteEnv(probed, opts.remoteEnvs, cfgRemoteEnv, nil)...) defer shellServer.Close() executor := &lifecycle.ShellExecutor{Server: shellServer, Log: logger} diff --git a/internal/cli/up.go b/internal/cli/up.go index b230f6a..99c3fad 100644 --- a/internal/cli/up.go +++ b/internal/cli/up.go @@ -986,8 +986,7 @@ func (r *upRunner) runLifecycleForUp(ctx context.Context, containerID string, cf logger, engine, opts := r.log, r.engine, r.opts probeStrategy := lifecycle.UserEnvProbeStrategy(cmp.Or(cfg.UserEnvProbe, opts.defaultUserEnvProbe)) probed := lifecycle.ProbedRemoteEnv(ctx, engine, logger, containerID, remoteUser, probeStrategy, opts.containerSessionDataFolder) - extra := append(opts.secrets.env(), opts.remoteEnvs...) - mergedRemoteEnv := hookRemoteEnv(probed, extra, cfg.RemoteEnv) + mergedRemoteEnv := hookRemoteEnv(probed, opts.remoteEnvs, cfg.RemoteEnv, opts.secrets.env()) shellServer := lifecycle.NewShellServer(ctx, engine, containerID, remoteUser, mergedRemoteEnv...) defer shellServer.Close() @@ -1033,18 +1032,18 @@ func (r *upRunner) checkContainerSubstitution(inspectResp container.InspectRespo return checkContainerSubstitution(containerEnv, cfg.ConfigFilePath, raw, imagemeta.MergeConfiguration(entries)) } -func hookRemoteEnv(probed map[string]string, extra []string, cfgRemoteEnv map[string]*string) []string { +func hookRemoteEnv(probed map[string]string, remoteEnvs []string, cfgRemoteEnv map[string]*string, secrets []string) []string { var env []string for k, v := range probed { env = append(env, fmt.Sprintf("%s=%s", k, v)) } - env = append(env, extra...) + env = append(env, remoteEnvs...) for k, v := range cfgRemoteEnv { if v != nil { env = append(env, fmt.Sprintf("%s=%s", k, *v)) } } - return env + return append(env, secrets...) } func hookContainerError(hookErr error, containerID, fallback string) *coreerrors.ContainerError { diff --git a/internal/cli/up_held_test.go b/internal/cli/up_held_test.go index eba0384..96e9b63 100644 --- a/internal/cli/up_held_test.go +++ b/internal/cli/up_held_test.go @@ -467,12 +467,16 @@ func TestParseComposeServiceBuildReadsTheServicesBuild(t *testing.T) { func TestHookRemoteEnvOrdersLaterSourcesLast(t *testing.T) { got := hookRemoteEnv( map[string]string{"P": "probed"}, - []string{"S=secret", "R=cli"}, - map[string]*string{"C": strPtr("config"), "UNSET": nil}, + []string{"R=cli", "TOKEN=cli"}, + map[string]*string{"TOKEN": strPtr("config"), "UNSET": nil}, + []string{"TOKEN=secret"}, ) - if want := []string{"P=probed", "S=secret", "R=cli", "C=config"}; !reflect.DeepEqual(got, want) { + if want := []string{"P=probed", "R=cli", "TOKEN=cli", "TOKEN=config", "TOKEN=secret"}; !reflect.DeepEqual(got, want) { t.Errorf("env = %q, want %q", got, want) } + if got, want := hookRemoteEnv(nil, []string{"R=cli"}, nil, nil), []string{"R=cli"}; !reflect.DeepEqual(got, want) { + t.Errorf("without config or secrets: env = %q, want %q", got, want) + } } func TestDropRanHooksAsksEveryMarkerAndDropsWhatRan(t *testing.T) { From 33d147765b4178e44b9959ff447ad34adbedac28 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Fri, 13 Nov 2026 01:42:50 -0300 Subject: [PATCH 2/2] fix(cli): report a failed --remove-existing-container removal up ignored the error removing the existing container and went on. The reference fails up with "An error occurred setting up the container." and the message "Command failed: rm -f " (with the container ID for a Compose configuration), exit 1. The removal is now logged as the reference's start/stop pair, "Removing Existing Container" ("Removing existing container." for Compose), instead of an info line. Co-Authored-By: Claude Opus 5.5 --- internal/cli/up.go | 41 +++++++++++++++++++- internal/cli/up_held_test.go | 72 ++++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 2 deletions(-) diff --git a/internal/cli/up.go b/internal/cli/up.go index 99c3fad..3296a1d 100644 --- a/internal/cli/up.go +++ b/internal/cli/up.go @@ -321,8 +321,9 @@ func runUp(ctx context.Context, out Output, opts *upOpts) error { existingIDs, _ := engine.ListContainers(ctx, true, idLabels) if len(existingIDs) > 0 && opts.removeExisting { - logger.Write(fmt.Sprintf("Removing existing container %s...", existingIDs[0]), log.LevelInfo) - _ = engine.RemoveContainer(ctx, existingIDs[0]) + if err := removeExistingContainer(ctx, engine, logger, cfg, opts.dockerPath, existingIDs[0]); err != nil { + return writeErrorJSON(out, coreerrors.ToErrorOutput(err)) + } existingIDs = nil } @@ -1046,6 +1047,42 @@ func hookRemoteEnv(probed map[string]string, remoteEnvs []string, cfgRemoteEnv m return append(env, secrets...) } +type containerRemover interface { + RemoveContainer(ctx context.Context, id string) error +} + +type containerRemoveError struct { + dockerPath string + id string + err error +} + +func (e *containerRemoveError) Error() string { + return fmt.Sprintf("Command failed: %s rm -f %s", e.dockerPath, e.id) +} + +func (e *containerRemoveError) Unwrap() error { return e.err } + +func removeExistingContainer(ctx context.Context, engine containerRemover, logger log.Logger, cfg *config.DevContainer, dockerPath, id string) error { + text := "Removing Existing Container" + if cfg.IsComposeConfig() { + text = "Removing existing container." + } + start := logger.Start(text) + if err := engine.RemoveContainer(ctx, id); err != nil { + removeErr := &coreerrors.ContainerError{ + Description: "An error occurred setting up the container.", + OriginalError: &containerRemoveError{dockerPath: cmp.Or(dockerPath, "docker"), id: id, err: err}, + } + if cfg.IsComposeConfig() { + removeErr.ContainerID = id + } + return removeErr + } + logger.Stop(text, start) + return nil +} + func hookContainerError(hookErr error, containerID, fallback string) *coreerrors.ContainerError { he, ok := errors.AsType[*lifecycle.HookError](hookErr) if !ok { diff --git a/internal/cli/up_held_test.go b/internal/cli/up_held_test.go index 96e9b63..a5a4450 100644 --- a/internal/cli/up_held_test.go +++ b/internal/cli/up_held_test.go @@ -23,6 +23,7 @@ import ( ocispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/devcontainers/cli/internal/config" + coreerrors "github.com/devcontainers/cli/internal/errors" "github.com/devcontainers/cli/internal/docker" "github.com/devcontainers/cli/internal/imagemeta" "github.com/devcontainers/cli/internal/lifecycle" @@ -656,3 +657,74 @@ func TestUpFeatureBuildOptionsCarryTheContainerEnv(t *testing.T) { t.Errorf("ContainerEnv = %v, want the config's %v", got, cfg.ContainerEnv) } } + +type fakeRemover struct { + removed []string + err error +} + +func (f *fakeRemover) RemoveContainer(_ context.Context, id string) error { + f.removed = append(f.removed, id) + return f.err +} + +func TestRemoveExistingContainer(t *testing.T) { + daemonErr := errors.New("removal of container abc is already in progress") + image := &config.DevContainer{} + compose := &config.DevContainer{DockerComposeFile: config.StringOrStrings{"docker-compose.yml"}} + for _, tc := range []struct { + name string + cfg *config.DevContainer + dockerPath string + removeErr error + wantEvents []string + wantErr *coreerrors.ErrorOutput + }{ + {"image config", image, "", nil, []string{"start:Removing Existing Container", "stop:Removing Existing Container"}, nil}, + {"compose config", compose, "", nil, []string{"start:Removing existing container.", "stop:Removing existing container."}, nil}, + {"image config fails", image, "", daemonErr, []string{"start:Removing Existing Container"}, &coreerrors.ErrorOutput{ + Outcome: "error", Message: "Command failed: docker rm -f abc", Description: "An error occurred setting up the container.", + }}, + {"compose config fails", compose, "/usr/bin/podman", daemonErr, []string{"start:Removing existing container."}, &coreerrors.ErrorOutput{ + Outcome: "error", Message: "Command failed: /usr/bin/podman rm -f abc", Description: "An error occurred setting up the container.", ContainerID: "abc", + }}, + } { + t.Run(tc.name, func(t *testing.T) { + var logged bytes.Buffer + logger := log.New(log.Options{Writer: &logged, Format: "json", Level: log.LevelTrace}) + engine := &fakeRemover{err: tc.removeErr} + + err := removeExistingContainer(context.Background(), engine, logger, tc.cfg, tc.dockerPath, "abc") + + if !slices.Equal(engine.removed, []string{"abc"}) { + t.Errorf("removed %q, want [abc]", engine.removed) + } + var events []string + for line := range strings.Lines(logged.String()) { + var e log.Event + if err := json.Unmarshal([]byte(line), &e); err != nil { + t.Fatalf("log line %q: %v", line, err) + } + events = append(events, e.Type+":"+e.Text) + } + if !slices.Equal(events, tc.wantEvents) { + t.Errorf("events = %q, want %q", events, tc.wantEvents) + } + if tc.wantErr == nil { + if err != nil { + t.Fatalf("err = %v, want nil", err) + } + return + } + if err == nil { + t.Fatal("err = nil, want the removal failure") + } + if got := coreerrors.ToErrorOutput(err); !reflect.DeepEqual(got, *tc.wantErr) { + t.Errorf("error output = %+v, want %+v", got, *tc.wantErr) + } + if !errors.Is(err, daemonErr) { + t.Errorf("err = %v, want it to wrap the daemon error", err) + } + }) + } +}