Skip to content
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.

Sign up for GitHub

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

Jump to bottom

VAULT-6936 support bound service account namespace selector #218

Merged
merged 21 commits into from
Dec 12, 2023
Merged

VAULT-6936 support bound service account namespace selector #218

merged 21 commits into from
Dec 12, 2023

Conversation

thyton
Copy link
Contributor

@thyton thyton commented Nov 27, 2023

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)

make integration-test
cd integrationtest && INTEGRATION_TESTS=true CGO_ENABLED=0 KUBE_CONTEXT="kind-vault-plugin-auth-kubernetes" go test '-test.v' -count=1 -timeout=20m ./...
?   	github.com/hashicorp/vault-plugin-auth-kubernetes/integrationtest/k8s	[no test files]
=== RUN   TestSuccess
--- PASS: TestSuccess (0.48s)
=== RUN   TestSuccessWithTokenReviewerJwt
--- PASS: TestSuccessWithTokenReviewerJwt (0.10s)
=== RUN   TestSuccessWithNamespaceLabels
--- PASS: TestSuccessWithNamespaceLabels (0.11s)
=== RUN   TestFailWithBadTokenReviewerJwt
--- PASS: TestFailWithBadTokenReviewerJwt (0.11s)
=== RUN   TestUnauthorizedServiceAccountErrorCode
--- PASS: TestUnauthorizedServiceAccountErrorCode (0.10s)
=== RUN   TestAudienceValidation
=== RUN   TestAudienceValidation/config:_default,_JWT:_default
=== RUN   TestAudienceValidation/config:_default,_JWT:_a
=== RUN   TestAudienceValidation/config:_a,_JWT:_a
=== RUN   TestAudienceValidation/config:_a,_JWT:_b
=== RUN   TestAudienceValidation/config:_unset,_JWT:_default
=== RUN   TestAudienceValidation/config:_unset,_JWT:_a
--- PASS: TestAudienceValidation (0.61s)
    --- PASS: TestAudienceValidation/config:_default,_JWT:_default (0.10s)
    --- PASS: TestAudienceValidation/config:_default,_JWT:_a (0.09s)
    --- PASS: TestAudienceValidation/config:_a,_JWT:_a (0.11s)
    --- PASS: TestAudienceValidation/config:_a,_JWT:_b (0.09s)
    --- PASS: TestAudienceValidation/config:_unset,_JWT:_default (0.09s)
    --- PASS: TestAudienceValidation/config:_unset,_JWT:_a (0.10s)
PASS
ok  	github.com/hashicorp/vault-plugin-auth-kubernetes/integrationtest	2.077s

[ ] Backwards compatible

@benashz benashz self-requested a review November 27, 2023 19:05
benashz
benashz reviewed Nov 27, 2023
View reviewed changes
Copy link
Contributor

@benashz benashz left a comment •
edited
Loading

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.

namespace_validate.go Outdated Show resolved Hide resolved
namespace_validate.go Outdated Show resolved Hide resolved
namespace_validate.go Outdated Show resolved Hide resolved
path_config_test.go Show resolved Hide resolved
path_login.go Outdated Show resolved Hide resolved
namespace_validate.go Outdated Show resolved Hide resolved
namespace_validate.go Outdated Show resolved Hide resolved
namespace_validate.go Outdated Show resolved Hide resolved
namespace_validate.go Outdated Show resolved Hide resolved
namespace_validate.go Outdated Show resolved Hide resolved
@benashz benashz self-requested a review November 27, 2023 20:34
benashz
benashz reviewed Nov 28, 2023
View reviewed changes
Copy link
Contributor

@benashz benashz left a 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.

thyton and others added 10 commits December 4, 2023 06:50
@thyton @benashz
Update namespace_validate.go
ade9d2e 
Co-authored-by: Ben Ash <32777270+benashz@users.noreply.github.com>
@thyton @benashz
Update namespace_validate.go
37462d8 
Co-authored-by: Ben Ash <32777270+benashz@users.noreply.github.com>
@thyton @benashz
Update path_role_test.go
eec906c 
Co-authored-by: Ben Ash <32777270+benashz@users.noreply.github.com>
@thyton @benashz
Update path_role_test.go
44bc86a 
Co-authored-by: Ben Ash <32777270+benashz@users.noreply.github.com>
@thyton
address comments
4535b0c
@thyton
drop API from error message
67d9888
@thyton
rename variables
cc35bde
@thyton
parse error status from body on namespace retrieval failure
16f8757
@thyton
add a new line
ca554dd
@thyton
make fmt
0488570
@thyton
Copy link
Contributor Author

thyton commented Dec 5, 2023 •
edited
Loading

@benashz @tomhjp I appreciate the detailed first pass! I've addressed/followed up with all comments. It's ready for the next review whenever you have a chance.

@thyton thyton requested review from benashz and tomhjp December 5, 2023 16:45
@thyton thyton requested a review from kschoche December 7, 2023 15:57
benashz
benashz reviewed Dec 7, 2023
View reviewed changes
Copy link
Contributor

@benashz benashz left a 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.

integrationtest/integration_test.go Show resolved Hide resolved
namespace_validator.go Outdated Show resolved Hide resolved
path_login.go Outdated Show resolved Hide resolved
path_login.go Outdated Show resolved Hide resolved
path_login.go Outdated Show resolved Hide resolved
path_login_test.go Show resolved Hide resolved
path_role.go Show resolved Hide resolved
path_role.go Outdated Show resolved Hide resolved
path_role.go Outdated Show resolved Hide resolved
path_role.go Show resolved Hide resolved
@thyton thyton requested a review from benashz December 8, 2023 18:59
benashz
benashz reviewed Dec 8, 2023
View reviewed changes
path_login.go Outdated Show resolved Hide resolved
@thyton thyton mentioned this pull request Dec 11, 2023
Closed
benashz
benashz reviewed Dec 11, 2023
View reviewed changes
Copy link
Contributor

@benashz benashz left a 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"})
Copy link
Contributor

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.

benashz
benashz approved these changes Dec 12, 2023
View reviewed changes
Copy link
Contributor

@benashz benashz left a 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.

@thyton thyton merged commit 0f4be0b into main Dec 12, 2023
8 checks passed
@thyton thyton deleted the VAULT-6936-support-allowed-namespace-selector branch December 12, 2023 16:07
@thyton thyton mentioned this pull request Dec 12, 2023
Merged
thyton added a commit that referenced this pull request Dec 12, 2023
@thyton
add missing CHANGELOG entry for PR #218 (#220)
7bd7455
@mbenson
Copy link

mbenson commented Jan 8, 2024

When are we likely to see a new release incorporating this change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benashz benashz benashz approved these changes

@tomhjp tomhjp Awaiting requested review from tomhjp

@kschoche kschoche Awaiting requested review from kschoche

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@thyton @mbenson @tomhjp @benashz @f4z3r