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

Cache secrets and concurrent decryption #790

Merged
merged 1 commit into from
Aug 7, 2019

Conversation

travisgroth
Copy link
Contributor

Related to #782 and #444

  • Allows concurrent decryption of different secrets files
  • Caches decrypted secrets by original file path and returns decrypted results from memory
  • Secrets being run through an instance of helmexec will be cached and run as fast as possible concurrently

NB: This particular PR doesn't make all calls to secrets cached and concurrent. Environment Secrets in particular seem to not be evaluated with a ScatterGather(), and doesn't use the same helmexec instance as other parts of the code, so it doesn't take advantage of these changes. Some reworking of the plumbing there would be needed.

- Caches decrypted secrets by original file path and returns decrypted results from memory
@travisgroth travisgroth changed the title Cache secrets and allow concurrent decryption Cache secrets and concurrent decryption Aug 6, 2019
@travisgroth travisgroth marked this pull request as ready for review August 6, 2019 22:41
@mumoshu
Copy link
Collaborator

mumoshu commented Aug 7, 2019

I think we had a hacky workaround to use helm-secrets for decrypting environment secrets. Perhaps that's one of gotchas? Anyways, I'm fine with just sharing helmexec within and outside of scatterGather, if that works.

Copy link
Collaborator

@mumoshu mumoshu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read helm.decryptedSecretMutex is for a wider and shorter lock, that is taken until we take a more granular and longer "per-secret" lock. And we now have a in-memory cache of decrypted files paired with mutex's for granular locking, in decryptedSecrets. This does seem to do what it states.

LGTM! Great job. I'm impressed by your work, @travisgroth 🎉 🙏

@mumoshu mumoshu merged commit 6baad71 into roboll:master Aug 7, 2019
@travisgroth
Copy link
Contributor Author

Thanks! Also, your read on the methodology is accurate. Should be least possible lock contention.

I think we had a hacky workaround to use helm-secrets for decrypting environment secrets. Perhaps that's one of gotchas? Anyways, I'm fine with just sharing helmexec within and outside of scatterGather, if that works.

👍 i'm still poking around. I plan on doing some follow up PRs.

@travisgroth travisgroth deleted the feat/secrets-speedup branch August 7, 2019 16:27
This was referenced Mar 15, 2021
mumoshu pushed a commit that referenced this pull request Mar 23, 2021
Fixes issue introduced in #790: the order of secrets merged is not defined now, leading to unpredictable results in `helmfile apply`.
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

Successfully merging this pull request may close these issues.

2 participants