-
Notifications
You must be signed in to change notification settings - Fork 63
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
VAULT-6936 support bound service account namespace selector #218
Conversation
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.
Gave it a first pass. I think we may want to revisit the approach a bit. I had some questions around request parameter types JSON+YAML, the way that namespaces and namespace selector's are evaluated together.
Overall, it seems like the change is heading in a positive direction.
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.
Re-adding myself as a reviewer.
Co-authored-by: Ben Ash <[email protected]>
Co-authored-by: Ben Ash <[email protected]>
Co-authored-by: Ben Ash <[email protected]>
Co-authored-by: Ben Ash <[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 making the requested changes! I think we are almost there.
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.
Looking good! It would be good to add some tests for the methods provided by namespaceValidatorWrapper{}
, as well as what we covered during our sync.
path_login_test.go
Outdated
testName = "vault-auth" | ||
testUID = "d77f89bc-9055-11e7-a068-0800276d99bf" | ||
testMockTokenReviewFactory = mockTokenReviewFactory(testName, testNamespace, testUID) | ||
testMockNamespaceValidateFactory = mockNamespaceValidateFactory(map[string]string{"key": "value", "other": "label"}) |
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.
nit: maybe fold on the slice elements.
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.
LGTM! Thanks for making the extra changes.
When are we likely to see a new release incorporating this change? |
Overview
A high level description of the contribution, including:
What is the change? Support a label selector to define from which namespaces clients are allowed to authenticate with their ServiceAccounts.
Why is the change needed? This will add flexibility to bound namespace specification. A similar feature, allowed_kubernetes_namespace_selector, was also seen in Secret Engine.
How does this change affect the user experience (if at all)? No
Design of Change
How was this change implemented? The change is based on PR #182 and how allowed_kubernetes_namespace_selector was implemented.
Related Issues/Pull Requests
[ ] Issue #155
[ ] PR #182
Contributor Checklist
[ ] Add relevant docs to upstream Vault repository, or sufficient reasoning why docs won’t be added yet
Docs PR Link
[ ] Add output for any tests not ran in CI to the PR description (eg, acceptance tests)
[ ] Backwards compatible