-
Notifications
You must be signed in to change notification settings - Fork 404
Conversation
@silasbw could you please take a look if you have the time? |
907f9c2
to
4548af4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @moolen, sorry for the delay. It's been vacation season over here.
Looks, good. One question.
lib/poller.js
Outdated
* @param {Object} namespace namespace object | ||
* @param {Object} descriptor secret descriptor | ||
*/ | ||
async _isPermitted (namespace, descriptor) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do external secrets users need to add annotations to their namespaces if they're using the assume role feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they don't have to, it's optional. I added a test to verify all testcases.
Signed-off-by: Moritz Johner <[email protected]>
add option to restrict the range of assumed roles by specifying an regular expression on a namespace annotation Signed-off-by: Moritz Johner <[email protected]>
Signed-off-by: Moritz Johner <[email protected]>
747a457
to
7c1f646
Compare
@silasbw i addressed the issues and rebased, PTAL again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few nits
Signed-off-by: Moritz Johner <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
Thanks for this feature, do we have a tentative date when this can be released? |
Hi, I got the impression from the docs in the See below:
I'm running external-secrets 1.5.0. Do we need to wait on a release for this feature to be usable? |
* feat: add option to assume role when retrieving secrets Signed-off-by: Moritz Johner <[email protected]> * feat: restrict iam roles per namespace add option to restrict the range of assumed roles by specifying an regular expression on a namespace annotation Signed-off-by: Moritz Johner <[email protected]> * chore: add test to verify assume-role access control * docs: add policy for secrets manager * docs: add assume-role limits per ns Signed-off-by: Moritz Johner <[email protected]> * docs: fix spelling Signed-off-by: Moritz Johner <[email protected]> * chore: remove stupid code
* feat: add option to assume role when retrieving secrets Signed-off-by: Moritz Johner <[email protected]> * feat: restrict iam roles per namespace add option to restrict the range of assumed roles by specifying an regular expression on a namespace annotation Signed-off-by: Moritz Johner <[email protected]> * chore: add test to verify assume-role access control * docs: add policy for secrets manager * docs: add assume-role limits per ns Signed-off-by: Moritz Johner <[email protected]> * docs: fix spelling Signed-off-by: Moritz Johner <[email protected]> * chore: remove stupid code
This PR solves #143
It adds an option to specify a
roleArn
on aExternalSecret
CRD. The app will assume a role before retrieving the secret.I tested it using
localstack
and AWS.