Skip to content
This repository has been archived by the owner on Jul 26, 2022. It is now read-only.

Cache Vault clients/tokens on a per-role&mountpoint basis. #488

Merged
merged 5 commits into from
Oct 4, 2020

Conversation

megakid
Copy link
Contributor

@megakid megakid commented Sep 19, 2020

Fixes #487

When you have a single k8s cluster with multiple namespaces and each ExternalSecret is defined with a very limited scoped vaultRole (limiting the vault data that role can read) the existing token caching causes issues.

This change caches each vaultClient against the vaultRole it's authenticated against and can therefore KES can read all the secrets it is meant to regardless of how many vaultRoles are defined across your ExternalSecrets

@pault28
Copy link

pault28 commented Sep 19, 2020

Lovely. This will solve the issue I am currently facing. Guessing other folks would want this as it follows vault policy per role.

@megakid megakid changed the title Cache Vault clients/tokens on a per-role basis. Cache Vault clients/tokens on a per-role&mountpoint basis. Sep 19, 2020
@Flydiverny
Copy link
Member

Some conflicts after #472 merge, mind resolving?

@megakid
Copy link
Contributor Author

megakid commented Sep 25, 2020

I can't explain why the E2E tests are failing here (they were passing before this merge and the changes are extremely minimal) whilst the unit tests are passing. Tried numerous things (will tidy up afterwards) but e2e still returning error code 1. Seems they're broken in the README.md update PR here too #490

By the way, we are running the pre-merge code on our production cluster with good outcomes.

@megakid megakid mentioned this pull request Sep 26, 2020
@Flydiverny
Copy link
Member

Yeah don't think E2E tests are related to this PR, seems to be failing everywhere but I cant really tell why <_<

@megakid
Copy link
Contributor Author

megakid commented Sep 26, 2020

Should I just tidy up the commits (rebase) in preparation for merge or do the e2e tests need sorting first?

@Flydiverny
Copy link
Member

I dont have the rights to merge without the E2E tests passing anyway so :) and all merges are squash as well. But I'll try pinging someone on slack.
E2E test ran fine locally so 🤷

@Flydiverny
Copy link
Member

E2E tests have been restored and should be working after a rebase :)

@megakid
Copy link
Contributor Author

megakid commented Sep 29, 2020

All good now 👍

Copy link
Member

@Flydiverny Flydiverny left a comment

Choose a reason for hiding this comment

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

LGTM

@Flydiverny Flydiverny merged commit ab36718 into external-secrets:master Oct 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Specifying unique vaultRole per external secrets result in permission denied for all but one
3 participants