webhook: transit engine use batch api calls for better performance #1697
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
BojanZelic
requested review from
bonifaido,
pbalogh-sa and
Rasek91
as code owners
BojanZelic
force-pushed
the
batch-decrypt-transit-secrets
branch
4 times, most recently
from
6b3c78e to
c07def4
Compare
BojanZelic
force-pushed
the
batch-decrypt-transit-secrets
branch
from
c07def4 to
b1271dd
Compare
NitriKx
referenced
this pull request
in NitriKx/bank-vaults
Jan 27, 2023
Since https://github.com/banzaicloud/bank-vaults/pull/1697 we are experimenting issues with `vault-env`: the generated environment has all the `vault:` env vars duplicated, one version in its original version and a second one with the secret instead of the token. I have noticed that the new `preprocessTransitSecrets` is calling `inject` even if nothing has been replaced with values from the transit cache. In case you are working with "maps" (ConfigMap, Secret) this does not matter as the "old" code will still overwrite the key with the secret. But in the case of `vault-env` you will have the same env var set twice and most the the time only the first occurence (without the secret) is taken into account.
This was referenced Jan 27, 2023
NitriKx
referenced
this pull request
in backmarket-oss/bank-vaults
Feb 1, 2023
Since https://github.com/banzaicloud/bank-vaults/pull/1697 we are experimenting issues with `vault-env`: the generated environment has all the `vault:` env vars duplicated, one version in its original version and a second one with the secret instead of the token. I have noticed that the new `preprocessTransitSecrets` is calling `inject` even if nothing has been replaced with values from the transit cache. In case you are working with "maps" (ConfigMap, Secret) this does not matter as the "old" code will still overwrite the key with the secret. But in the case of `vault-env` you will have the same env var set twice and most the the time only the first occurence (without the secret) is taken into account.
NitriKx
referenced
this pull request
in backmarket-oss/bank-vaults
Feb 2, 2023
fix: fix vault-env generated env which may contain duplicates Since https://github.com/banzaicloud/bank-vaults/pull/1697 we are experimenting issues with `vault-env`: the generated environment has all the `vault:` env vars duplicated, one version in its original version and a second one with the secret instead of the token. I have noticed that the new `preprocessTransitSecrets` is calling `inject` even if nothing has been replaced with values from the transit cache. In case you are working with "maps" (ConfigMap, Secret) this does not matter as the "old" code will still overwrite the key with the secret. But in the case of `vault-env` you will have the same env var set twice and most the the time only the first occurence (without the secret) is taken into account. build: try to use chart-releaser to publish our chart in GitHub pages build: only consider tags ending with `+bm` build: fix the registry when building the docker images build: fix yaml syntax build: use `-bm` suffix docker images and binaries (and `+bm` for helm charts) build: use chart-releaser to package the helm chart build: relies on versioning in the Charts.yaml to make chart-releaser happy
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.
What's in this PR?
This PR improves performance when using the transit engine with lots of secrets coming in from vault;
Why?
When you have lots of secrets that use the transit engine, the webhook is really slow to respond; This is because it's making N number of API calls to vault; We can leverage batch_input to send multiple secrets at once, reducing the overhead and improving the response time of the webhook.
For example - If I have a secret with 20 Values coming from vault... previously this would take about 1.5-2 seconds for the webhook to respond; After this PR since it's only making 1 API call, it takes < 250 ms
Additional context
I've tested it extensively with ConfigMaps, and Secrets; The code for secret mutation was simplified in order to share the logic for prefetching transit secrets;
Mutating Custom Resources or Pods still has the old behavior
Checklist