fix(deploy): prepare replacement before stop-first cutover (#420)

* fix(compose): strip and resolve pre-deploy hook secrets like service env (fixes #422)

normaliseContainerForStore only cleared Config.Env and
ServiceSpec.Container.Env before writing a container record to the
replicated Corrosion store, leaving ServiceSpec.PreDeploy.Env in place.
A secret set via x-pre_deploy.environment therefore replicated to
every machine in the cluster, unlike the same value set via
environment.

ResolveSecrets had the same asymmetry in the other direction: it only
resolved secret://name references in service.Environment, so a
reference placed in the pre-deploy hook's environment passed through
unresolved as a literal string. HasCommandSecretRefs is extended the
same way for consistency.

Add tests pinning both behaviours.

* update docs that secret references are available for pre-deploy hooks

---------

Co-authored-by: Pasha Sviderski <me@psviderski.name>

internal/machine/store/container.go

@@ -96,6 +96,9 @@ func normaliseContainerForStore(ctr *api.ServiceContainer) {
 	// Remove the environment variables to avoid leaking secrets.
 	ctr.Config.Env = nil
 	ctr.ServiceSpec.Container.Env = nil
+	if ctr.ServiceSpec.PreDeploy != nil {
+		ctr.ServiceSpec.PreDeploy.Env = nil
+	}
 
 	// Docker returns Mounts in a non-deterministic order so sort them.
 	slices.SortFunc(ctr.Mounts, func(a, b container.MountPoint) int {

internal/machine/store/container_test.go

@@ -0,0 +1,67 @@
+package store
+
+import (
+	"testing"
+
+	"github.com/docker/docker/api/types/container"
+	"github.com/psviderski/uncloud/pkg/api"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestNormaliseContainerForStore_StripsEnv pins the behaviour that all fields carrying environment variables
+// are cleared before a container record is written to the replicated store, regardless of which part of the
+// spec they live in. See https://github.com/psviderski/uncloud/issues/422.
+func TestNormaliseContainerForStore_StripsEnv(t *testing.T) {
+	t.Parallel()
+
+	newContainer := func(preDeploy *api.PreDeployHook) api.ServiceContainer {
+		return api.ServiceContainer{
+			Container: api.Container{
+				InspectResponse: container.InspectResponse{
+					Config: &container.Config{
+						Env: []string{"CONTROL_TOKEN=control-value-aaa"},
+					},
+				},
+			},
+			ServiceSpec: api.ServiceSpec{
+				Container: api.ContainerSpec{
+					Env: api.EnvVars{"CONTROL_TOKEN": "control-value-aaa"},
+				},
+				PreDeploy: preDeploy,
+			},
+		}
+	}
+
+	t.Run("service with pre-deploy hook", func(t *testing.T) {
+		t.Parallel()
+
+		ctr := newContainer(&api.PreDeployHook{
+			Command: []string{"/bin/true"},
+			Env:     api.EnvVars{"HOOK_TOKEN": "hook-value-bbb"},
+		})
+
+		normaliseContainerForStore(&ctr)
+
+		assert.Nil(t, ctr.Config.Env, "Config.Env should be stripped")
+		assert.Nil(t, ctr.ServiceSpec.Container.Env, "ServiceSpec.Container.Env should be stripped")
+		require.NotNil(t, ctr.ServiceSpec.PreDeploy, "PreDeploy itself should be left in place")
+		assert.Nil(t, ctr.ServiceSpec.PreDeploy.Env, "ServiceSpec.PreDeploy.Env should be stripped")
+		// Non-env fields of the hook must survive the normalisation.
+		assert.Equal(t, []string{"/bin/true"}, ctr.ServiceSpec.PreDeploy.Command)
+	})
+
+	t.Run("service without pre-deploy hook", func(t *testing.T) {
+		t.Parallel()
+
+		ctr := newContainer(nil)
+
+		require.NotPanics(t, func() {
+			normaliseContainerForStore(&ctr)
+		})
+
+		assert.Nil(t, ctr.Config.Env)
+		assert.Nil(t, ctr.ServiceSpec.Container.Env)
+		assert.Nil(t, ctr.ServiceSpec.PreDeploy)
+	})
+}

pkg/client/compose/secret.go

@@ -89,9 +89,28 @@ func ResolveSecrets(ctx context.Context, project *types.Project) error {
 		return v, nil
 	}
 
-	// Resolve only secret references set as values for environment variables in enabled services.
 	for _, service := range project.Services {
-		for k, v := range service.Environment {
+		// Resolve secret references set as values for environment variables in the service.
+		if err := resolveEnvSecrets(service.Environment, resolve); err != nil {
+			return err
+		}
+
+		// Resolve secret references set as values for the pre-deploy hook's environment variables, if any.
+		if hook, ok := service.Extensions[PreDeployHookExtensionKey].(PreDeployHook); ok {
+			if err := resolveEnvSecrets(hook.Environment, resolve); err != nil {
+				return err
+			}
+		}
+	}
+
+	return nil
+}
+
+// resolveEnvSecrets resolves 'secret://name' references found as values in env to actual secret values using
+// resolve, setting each referenced variable to the secret's value in place. Variables that are not secret
+// references are left untouched.
+func resolveEnvSecrets(env types.MappingWithEquals, resolve func(string) (string, error)) error {
+	for k, v := range env {
 		if v == nil {
 			continue
 		}
@@ -103,18 +122,31 @@ func ResolveSecrets(ctx context.Context, project *types.Project) error {
 		if err != nil {
 			return err
 		}
-			service.Environment[k] = &value
+		env[k] = &value
 	}
-	}
-
 	return nil
 }
 
-// HasCommandSecretRefs reports whether any service environment references a secret that is resolved by running
+// HasCommandSecretRefs reports whether any service environment, including a pre-deploy hook's environment,
-// a command.
+// references a secret that is resolved by running a command.
 func HasCommandSecretRefs(project *types.Project) bool {
 	for _, service := range project.Services {
-		for _, v := range service.Environment {
+		if envHasCommandSecretRef(service.Environment, project) {
+			return true
+		}
+		if hook, ok := service.Extensions[PreDeployHookExtensionKey].(PreDeployHook); ok {
+			if envHasCommandSecretRef(hook.Environment, project) {
+				return true
+			}
+		}
+	}
+	return false
+}
+
+// envHasCommandSecretRef reports whether env contains a 'secret://name' reference to a secret resolved
+// by running a command.
+func envHasCommandSecretRef(env types.MappingWithEquals, project *types.Project) bool {
+	for _, v := range env {
 		if v == nil {
 			continue
 		}
@@ -126,7 +158,6 @@ func HasCommandSecretRefs(project *types.Project) bool {
 			return true
 		}
 	}
-	}
 	return false
 }
 

pkg/client/compose/secret_test.go

@@ -328,6 +328,74 @@ secrets:
 	assert.Equal(t, "run\n", string(runs), "command should run exactly once across services and repeated resolutions")
 }
 
+// TestResolveSecrets_PreDeployHook pins the behaviour that 'secret://name' references in the pre-deploy hook's
+// environment are resolved just like references in the service's own environment.
+// See https://github.com/psviderski/uncloud/issues/422.
+func TestResolveSecrets_PreDeployHook(t *testing.T) {
+	t.Parallel()
+
+	content := `
+services:
+  foo:
+    image: foo
+    environment:
+      CONTROL_TOKEN: secret://control_token
+    x-pre_deploy:
+      command: ["./migrate.sh"]
+      environment:
+        HOOK_TOKEN: secret://hook_token
+        PLAIN: hello
+secrets:
+  control_token:
+    x-command: printf control-value
+  hook_token:
+    x-command: printf hook-value
+`
+	project := loadProject(t, content)
+	require.NoError(t, ResolveSecrets(context.Background(), project))
+
+	assert.Equal(t, env(map[string]string{"CONTROL_TOKEN": "control-value"}), project.Services["foo"].Environment)
+
+	hook, ok := project.Services["foo"].Extensions[PreDeployHookExtensionKey].(PreDeployHook)
+	require.True(t, ok, "x-pre_deploy extension not found")
+	assert.Equal(t, env(map[string]string{"HOOK_TOKEN": "hook-value", "PLAIN": "hello"}), hook.Environment)
+}
+
+// TestResolveSecrets_PreDeployHook_ResolvedOnce verifies that a secret referenced from both the service's
+// environment and its pre-deploy hook's environment is only resolved once.
+func TestResolveSecrets_PreDeployHook_ResolvedOnce(t *testing.T) {
+	t.Parallel()
+
+	counter := filepath.Join(t.TempDir(), "runs")
+	content := fmt.Sprintf(`
+services:
+  foo:
+    image: foo
+    environment:
+      TOKEN: secret://token
+    x-pre_deploy:
+      command: ["./migrate.sh"]
+      environment:
+        TOKEN: secret://token
+secrets:
+  token:
+    x-command: "sh -c 'echo run >> %s; printf abc'"
+`, counter)
+
+	project := loadProject(t, content)
+	require.NoError(t, ResolveSecrets(context.Background(), project))
+
+	assert.Equal(t, "abc", *project.Services["foo"].Environment["TOKEN"])
+	hook, ok := project.Services["foo"].Extensions[PreDeployHookExtensionKey].(PreDeployHook)
+	require.True(t, ok, "x-pre_deploy extension not found")
+	assert.Equal(t, "abc", *hook.Environment["TOKEN"])
+
+	runs, err := os.ReadFile(counter)
+	require.NoError(t, err)
+	assert.Equal(t, "run\n", string(runs),
+		"command should run exactly once across the service and hook environments")
+}
+
 func TestHasCommandSecretRefs(t *testing.T) {
 	t.Parallel()
 
@@ -387,6 +455,22 @@ services:
 `,
 			want: false,
 		},
+		{
+			name: "command secret referenced from pre-deploy hook",
+			content: `
+services:
+  foo:
+    image: foo
+    x-pre_deploy:
+      command: ["./migrate.sh"]
+      environment:
+        TOKEN: secret://token
+secrets:
+  token:
+    x-command: printf abc
+`,
+			want: true,
+		},
 	}
 
 	for _, tt := range tests {

pkg/client/deploy/operation/container.go

@@ -173,6 +173,16 @@ type ReplaceContainerOperation struct {
 func (o *ReplaceContainerOperation) Execute(ctx context.Context, cli Client) error {
 	stopFirst := o.Order == api.UpdateOrderStopFirst
 
+	// Create the new container before stopping the old one. CreateContainer also pulls the image according to the
+	// service pull policy, so the stop-first downtime only includes stopping the old container and starting and
+	// monitoring the new one.
+	resp, err := cli.CreateContainer(ctx, o.ServiceID, o.Spec, o.MachineID)
+	if err != nil {
+		return fmt.Errorf("create new container: %w", err)
+	}
+	// Override event ID so StartContainer and WaitContainerHealthy update the same progress line as creation.
+	newCtx := cliprogress.WithEventID(ctx, cliprogress.NewContainerEventID(ctx, resp.Name, o.MachineName))
+
 	wasRunning := false
 	if stopFirst {
 		// Inspect the old container to remember its running state before stopping.
@@ -189,12 +199,6 @@ func (o *ReplaceContainerOperation) Execute(ctx context.Context, cli Client) err
 		}
 	}
 
-	resp, err := cli.CreateContainer(ctx, o.ServiceID, o.Spec, o.MachineID)
-	if err != nil {
-		return fmt.Errorf("create new container: %w", err)
-	}
-	// Override event ID so StartContainer and WaitContainerHealthy update the same progress line as creation.
-	newCtx := cliprogress.WithEventID(ctx, cliprogress.NewContainerEventID(ctx, resp.Name, o.MachineName))
 	if err = cli.StartContainer(newCtx, o.ServiceID, resp.ID); err != nil {
 		return fmt.Errorf("start new container: %w", err)
 	}

pkg/client/deploy/operation/container_test.go

@@ -0,0 +1,155 @@
+package operation
+
+import (
+	"context"
+	"errors"
+	"testing"
+
+	"github.com/docker/docker/api/types/container"
+	"github.com/docker/docker/api/types/volume"
+	"github.com/psviderski/uncloud/pkg/api"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestReplaceContainerOperation_StopFirstPreparation(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name      string
+		createErr error
+		wantErr   string
+		wantCalls []string
+	}{
+		{
+			name: "prepare before cutover",
+			wantCalls: []string{
+				"create:service",
+				"inspect:old",
+				"stop:old",
+				"start:new",
+				"wait:new",
+				"remove:old",
+			},
+		},
+		{
+			name:      "create failure does not interrupt old container",
+			createErr: errors.New("pull failed"),
+			wantErr:   "create new container: pull failed",
+			wantCalls: []string{"create:service"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			cli := &recordingClient{
+				oldContainerRunning: true,
+				createErr:           tt.createErr,
+			}
+			op := newStopFirstReplaceContainerOperation()
+
+			err := op.Execute(context.Background(), cli)
+			if tt.wantErr != "" {
+				require.ErrorContains(t, err, tt.wantErr)
+			} else {
+				require.NoError(t, err)
+			}
+			assert.Equal(t, tt.wantCalls, cli.calls)
+		})
+	}
+}
+
+func newStopFirstReplaceContainerOperation() *ReplaceContainerOperation {
+	return &ReplaceContainerOperation{
+		ServiceID:    "service-id",
+		Spec:         api.ServiceSpec{Name: "service"},
+		MachineID:    "machine-id",
+		MachineName:  "machine",
+		OldContainer: api.ServiceContainer{Container: api.Container{InspectResponse: container.InspectResponse{ContainerJSONBase: &container.ContainerJSONBase{ID: "old"}}}},
+		Order:        api.UpdateOrderStopFirst,
+	}
+}
+
+type recordingClient struct {
+	calls               []string
+	oldContainerRunning bool
+	createErr           error
+}
+
+func (c *recordingClient) CreateContainer(
+	_ context.Context, _ string, spec api.ServiceSpec, _ string,
+) (api.CreateContainerResponse, error) {
+	c.calls = append(c.calls, "create:"+spec.Name)
+	return api.CreateContainerResponse{
+		CreateResponse: container.CreateResponse{ID: "new"},
+		Name:           "new",
+	}, c.createErr
+}
+
+func (c *recordingClient) InspectContainer(
+	_ context.Context, _, containerID string,
+) (api.MachineServiceContainer, error) {
+	c.calls = append(c.calls, "inspect:"+containerID)
+	return api.MachineServiceContainer{
+		Container: api.ServiceContainer{
+			Container: api.Container{InspectResponse: container.InspectResponse{
+				ContainerJSONBase: &container.ContainerJSONBase{
+					ID:    containerID,
+					State: &container.State{Running: c.oldContainerRunning},
+				},
+			}},
+		},
+	}, nil
+}
+
+func (c *recordingClient) StartContainer(_ context.Context, _, containerID string) error {
+	c.calls = append(c.calls, "start:"+containerID)
+	return nil
+}
+
+func (c *recordingClient) StopContainer(
+	_ context.Context, _, containerID string, _ container.StopOptions,
+) error {
+	c.calls = append(c.calls, "stop:"+containerID)
+	return nil
+}
+
+func (c *recordingClient) RemoveContainer(
+	_ context.Context, _, containerID string, _ container.RemoveOptions,
+) error {
+	c.calls = append(c.calls, "remove:"+containerID)
+	return nil
+}
+
+func (c *recordingClient) CreatePreDeployHookContainer(
+	context.Context, string, api.ServiceSpec, string,
+) (api.CreateContainerResponse, error) {
+	panic("unexpected CreatePreDeployHookContainer call")
+}
+
+func (c *recordingClient) ExecContainer(context.Context, string, string, api.ExecOptions) (int, error) {
+	panic("unexpected ExecContainer call")
+}
+
+func (c *recordingClient) WaitContainerHealthy(
+	_ context.Context, _, containerID string, _ api.WaitContainerHealthyOptions,
+) error {
+	c.calls = append(c.calls, "wait:"+containerID)
+	return nil
+}
+
+func (c *recordingClient) CreateVolume(
+	context.Context, string, volume.CreateOptions,
+) (api.MachineVolume, error) {
+	panic("unexpected CreateVolume call")
+}
+
+func (c *recordingClient) ListVolumes(context.Context, *api.VolumeFilter) ([]api.MachineVolume, error) {
+	panic("unexpected ListVolumes call")
+}
+
+func (c *recordingClient) RemoveVolume(context.Context, string, string, bool) error {
+	panic("unexpected RemoveVolume call")
+}

website/docs/3-concepts/8-secrets.md

@@ -12,8 +12,8 @@ values at deploy time from your password manager, a cloud secrets store, or loca
 Two things go into your Compose file:
 
 1. Define a secret in the top-level `secrets` section. This tells Uncloud how to get the secret value.
-2. Reference that secret value in a service's `environment` using the `secret://<name>` format. This tells Uncloud to
+2. Reference that secret value in a service's `environment` or `x-pre_deploy.environment` using the `secret://<name>`
-   resolve the secret and set the environment variable to its value at deploy time.
+   format. This tells Uncloud to resolve the secret and set the environment variable to its value at deploy time.
 
 ```yaml title="compose.yaml"
 services:

website/docs/8-compose-file-reference/2-extensions.md

@@ -106,9 +106,9 @@ services:
 ### Attributes
 
 | Attribute     | Type                    | Default         | Description                                                                                                                                                                                                                                     |
-|---------------|-------------------------|-----------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+|---------------|-------------------------|-----------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
 | `command`     | string / list           | (required)      | The command to run in the hook container (same format as the service's [`command`](https://github.com/compose-spec/compose-spec/blob/main/05-services.md#command))                                                                              |
-| `environment` | map / list of KEY=VALUE | -               | Additional env vars that override or extend the service's [`environment`](https://github.com/compose-spec/compose-spec/blob/main/05-services.md#environment)       |
+| `environment` | map / list of KEY=VALUE | -               | Additional env vars that override or extend the service's [`environment`](https://github.com/compose-spec/compose-spec/blob/main/05-services.md#environment). Supports `secret://<name>` references. See [Secrets](../3-concepts/8-secrets.md). |
 | `privileged`  | bool                    | service's value | Override the service's [`privileged`](https://github.com/compose-spec/compose-spec/blob/main/05-services.md#privileged) mode                                                                                                                    |
 | `timeout`     | duration                | `5m`            | Max time to wait for the command to finish before killing it (e.g., `1m30s`, `30m`, `1h`)                                                                                                                                                       |
 | `user`        | string                  | service's value | Override the service's [`user`](https://github.com/compose-spec/compose-spec/blob/main/05-services.md#user) to run as (`user`, `UID`, `user:group`, or `UID:GID`)                                                                               |