Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potentially nondeterministic rendering of state secrets #1855

Closed
yurrriq opened this issue May 19, 2021 · 6 comments
Closed

Potentially nondeterministic rendering of state secrets #1855

yurrriq opened this issue May 19, 2021 · 6 comments

Comments

@yurrriq
Copy link
Contributor

yurrriq commented May 19, 2021

Providing a MFE here would be difficult, but we seem to be encountering nondeterministic rendering of state secrets (:rofl:).

With a setup like the following, where defaults.yaml and overrides.yaml have some of the same keys, we seem to sometimes end up with the value from defaults.yaml and other times from overrides.yaml.

environments:
  example:
    secrets:
    - secrets/defaults.yaml
    - secrets/example/overrides.yaml

The same pattern with values works fine, so perhaps this has something to do with secrets. It also seems to work ok with AWS KMS as the key store, but not always with our self-hosted Hashicorp Vault as the key store. The sops/vault --debug logs (with HELM_SECRETS_DRIVER_ARGS=--verbose too) seem to imply everything worked fine, and I do see the decrypted values from both defaults.yaml and overrides.yaml.

Does that make sense? Any idea what's going on here and how we might prevent the nondeterminism?

@yurrriq yurrriq changed the title Potentially nondeterministic rendering of state values/secrets Potentially nondeterministic rendering of state secrets May 19, 2021
@yurrriq
Copy link
Contributor Author

yurrriq commented May 19, 2021

Grasping at more straws:

  • Maybe the time it takes to decrypt defaults.yaml and overrides.yaml varies and thus determines the order/precedence.
  • Maybe mergo doesn't work how I think it does.
  • Maybe secrets are parsed concurrently.

@yurrriq
Copy link
Contributor Author

yurrriq commented May 19, 2021

Comparing the logs of a successful run vs unsuccessful, we've noticed that the environment secrets files are processed in a nondeterministic order, which results in differing precedence in the merge.

@mumoshu
Copy link
Collaborator

mumoshu commented May 19, 2021

@yurrriq Hey! Thanks for reporting.

That's interesting... Helmfile does decrypt environment secrets concurrently, but it also sorts results by ids so that the result is deterministic and merging happens in the order of definitions.

sortedSecrets[result.id] = result

That said, I cant' think of any potential bug that may result in secrets being processed in nondeterministic order.

Or perhaps helm-secrets has some limitation preventing it from being run concurrently? 🤔 I have never heard about such limitation though.

@yurrriq
Copy link
Contributor Author

yurrriq commented May 20, 2021

I first noticed it in GitLab CI jobs, but was able to reproduce it locally today. Two runs of helmfile with the same values etc showed it decrypting the two secret files in reverse order, and then giving precedence to one or the other. We noticed it in a helmfile diff run, in case that makes a difference...

Here's some more info:

λ helmfile --version
helmfile version v0.138.7

λ helm version --short
v3.5.3+g041ce5a

λ helm plugin list
NAME   	VERSION  	DESCRIPTION                                                                  
diff   	3.1.3    	Preview helm upgrade changes as a diff                                       
secrets	3.7.0-dev	This plugin provides secrets values encryption for Helm charts secure storing

λ sops --version
sops 3.7.1 (latest)

λ vault -version
Vault v1.7.1 (cgo)

If you think it'd be helpful, I could try to purge any secrets from the log files and email them to you or something.

@yurrriq
Copy link
Contributor Author

yurrriq commented May 20, 2021

Looks like this was fixed in #1726, and we just need to update to 0.139.x. I'll do that and report back. 🤞

@yurrriq
Copy link
Contributor Author

yurrriq commented May 20, 2021

Confirmed that #1726 fixed it. Many thanks, @astorath and @mumoshu!

@yurrriq yurrriq closed this as completed May 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants