fix(compose): strip and resolve pre-deploy hook secrets like service env - #424
Open
vky5 wants to merge 1 commit into
Open
fix(compose): strip and resolve pre-deploy hook secrets like service env#424vky5 wants to merge 1 commit into
vky5 wants to merge 1 commit into
Conversation
…env (fixes psviderski#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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #422.
ServiceSpec.PreDeploy.Envdidn't get the same treatment asServiceSpec.Container.Envin two places.normaliseContainerForStorestripsConfig.EnvandServiceSpec.Container.Envbefore a container record goes into the replicated Corrosion store, but never touchedServiceSpec.PreDeploy.Env. So a secret inx-pre_deploy.environmentreplicated to every machine in the cluster, while the same value inenvironmentstayed local. Reproduced this on a 3-nodeucindcluster, details in #422.ResolveSecretshad the mirror problem: it only resolvedsecret://namerefs inservice.Environment, never in the pre-deploy hook's environment. Asecret://reference there just passed through as a literal string, so secrets never actually worked for pre-deploy hooks in the first place.This PR fixes both:
normaliseContainerForStorenow also nilsServiceSpec.PreDeploy.Env.ResolveSecretsnow resolves refs in the hook's environment too, through a sharedresolveEnvSecretshelper instead of duplicating the loop, so the next env-carrying field doesn't need a second copy of the same logic.HasCommandSecretRefsgets the same treatment for consistency (cosmetic, it only decides whether "Resolving secrets..." gets printed).container_test.go(there wasn't one for this package) pinning the stripping behavior, plus cases insecret_test.gofor hook secret resolution and the once-only resolution guarantee across both fields.Ran
go test ./internal/machine/store/... ./pkg/client/compose/... -race, and checked each new test actually fails without its fix by reverting one fix at a time before restoring. gofmt/vet/golangci-lint all clean.