From 8ddbe3c60614678b7572c2f965dea5968abe985f Mon Sep 17 00:00:00 2001 From: Vaibhav Yadav <64212458+vky5@users.noreply.github.com> Date: Mon, 12 Oct 2026 11:18:47 +0530 Subject: [PATCH] fix(compose): strip and resolve pre-deploy hook secrets like service env (#424) * 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 --- internal/machine/store/container.go | 3 + internal/machine/store/container_test.go | 67 +++++++++++++++ pkg/client/compose/secret.go | 77 ++++++++++++----- pkg/client/compose/secret_test.go | 84 +++++++++++++++++++ website/docs/3-concepts/8-secrets.md | 4 +- .../8-compose-file-reference/2-extensions.md | 14 ++-- 6 files changed, 217 insertions(+), 32 deletions(-) create mode 100644 internal/machine/store/container_test.go diff --git a/internal/machine/store/container.go b/internal/machine/store/container.go index 0fb6d18a..eacaea45 100644 --- a/internal/machine/store/container.go +++ b/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 { diff --git a/internal/machine/store/container_test.go b/internal/machine/store/container_test.go new file mode 100644 index 00000000..4627020b --- /dev/null +++ b/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) + }) +} diff --git a/pkg/client/compose/secret.go b/pkg/client/compose/secret.go index c0005a0b..78c75415 100644 --- a/pkg/client/compose/secret.go +++ b/pkg/client/compose/secret.go @@ -89,40 +89,53 @@ 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 { - if v == nil { - continue - } - secretName, ok := secretRefName(*v) - if !ok { - continue - } - value, err := resolve(secretName) - if err != nil { + // 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 } - service.Environment[k] = &value } } return nil } -// HasCommandSecretRefs reports whether any service environment references a secret that is resolved by running -// a command. +// 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 + } + secretName, ok := secretRefName(*v) + if !ok { + continue + } + value, err := resolve(secretName) + if err != nil { + return err + } + env[k] = &value + } + return nil +} + +// HasCommandSecretRefs reports whether any service environment, including a pre-deploy hook's environment, +// 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 v == nil { - continue - } - name, ok := secretRefName(*v) - if !ok { - continue - } - if secret, ok := project.Secrets[name]; ok && secret.Driver == secretExecDriver { + if envHasCommandSecretRef(service.Environment, project) { + return true + } + if hook, ok := service.Extensions[PreDeployHookExtensionKey].(PreDeployHook); ok { + if envHasCommandSecretRef(hook.Environment, project) { return true } } @@ -130,6 +143,24 @@ func HasCommandSecretRefs(project *types.Project) bool { 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 + } + name, ok := secretRefName(*v) + if !ok { + continue + } + if secret, ok := project.Secrets[name]; ok && secret.Driver == secretExecDriver { + return true + } + } + return false +} + // secretValue resolves a secret to its value depending on its source: an 'exec' driver command, // an environment variable, or a file. A single trailing newline ('\n' or '\r\n') is stripped from the command // output as command-line tools commonly append one. All other whitespace is kept. Environment and file values are diff --git a/pkg/client/compose/secret_test.go b/pkg/client/compose/secret_test.go index 1b719dc9..4ac54d81 100644 --- a/pkg/client/compose/secret_test.go +++ b/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 { diff --git a/website/docs/3-concepts/8-secrets.md b/website/docs/3-concepts/8-secrets.md index 297a09f0..3fdecfc2 100644 --- a/website/docs/3-concepts/8-secrets.md +++ b/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://` format. This tells Uncloud to - resolve the secret and set the environment variable to its value at deploy time. +2. Reference that secret value in a service's `environment` or `x-pre_deploy.environment` using the `secret://` + format. This tells Uncloud to resolve the secret and set the environment variable to its value at deploy time. ```yaml title="compose.yaml" services: diff --git a/website/docs/8-compose-file-reference/2-extensions.md b/website/docs/8-compose-file-reference/2-extensions.md index b7516cfb..11e8fe8f 100644 --- a/website/docs/8-compose-file-reference/2-extensions.md +++ b/website/docs/8-compose-file-reference/2-extensions.md @@ -105,13 +105,13 @@ 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) | -| `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`) | +| 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). Supports `secret://` 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`) | The hook container also gets `UNCLOUD_HOOK_PRE_DEPLOY=true` environment variable set automatically.