In-memory cache per pod for Vault clients and tokens #202
Conversation
This file contains 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
tomhjp
requested review from
swenson,
tvoran,
a team,
shore and
alvin-huang
and removed request for
a team
tvoran
approved these changes
Apr 20, 2023
Co-authored-by: Theron Voran <[email protected]>
tvoran
approved these changes
Apr 20, 2023
Co-authored-by: Theron Voran <[email protected]>
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR adds a Vault client cache, keyed on SecretProviderClass parameters + pod info, which ensures we re-use Vault client tokens until they expire. When combined with the Vault Agent sidecar deployed by default in the helm chart since 0.24.0, this reduces lease creation to the minimum needed per pod.
Fixes #150 and #90, and relates to #149 and #151.
I initially tried caching the k8s tokens in the first commit which would have been simpler, however, I switched away from this because the Vault token TTL is uncapped by the k8s token TTL, so if the k8s token TTL is shorter than the Vault token TTL, it would have caused unnecessarily early reauthentication. It's then a poor UX to communicate that k8s token TTLs need to be set to the same length as Vault token TTLs to avoid thrashing.
It's quite a large PR but a lot of the changes are just moving code from
provider.gointoauth/kubernetes.goandclient.go. The majority of new code is in theclientandclientcachepackages.