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.

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

feat: optional audiences field on token create #24

Merged
merged 5 commits into from
Mar 10, 2023

Conversation

thyton
Copy link
Contributor

@thyton thyton commented Mar 7, 2023

Overview

This PR adds an option to set audiences for the k8s token created from the TokenCreate API

Design of Change

  • Add optional token_default_audiences field to the roles path backend
  • Add optional audiences field to the credentials path backend
  • Add audiences field to the credentials request payload
  • Add audiences param to func (c *client) createToken(ctx context.Context, namespace string, name string, ttl time.Duration)

@tvoran tvoran self-requested a review March 7, 2023 23:51
@thyton thyton force-pushed the VAULT-6292-k8s-se-support-audiences-on-token-create branch from 2aabfa6 to 0906f31 Compare March 8, 2023 00:23
Copy link
Member

@tvoran tvoran left a comment

Choose a reason for hiding this comment

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

This looks great! Left a thought about the tests, and I'm also wondering if we've thought about adding this as an option on the role endpoint as well? Something like token_default_audiences, that would be used if the audiences parameter isn't set in the call to the creds endpoint. Similar to how token_default_ttl and ttl are used between the role and creds endpoints.

Comment on lines 400 to 402
for _, audience := range expectedAudiences {
assert.Contains(t, aud, interface{}(audience))
}
Copy link
Member

Choose a reason for hiding this comment

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

I was playing around with this a bit, and noticed that if expectedAudiences is defined as a []interface{} type, then we could do a simpler assert.Equals() here, which would also allow us to check that the default aud is what we expect (https://kubernetes.default.svc.cluster.local).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had tried assert.Equal() before, and the actual audiences seemed to have a different order each test run. I'm not sure the cause for it

--- FAIL: TestCreds_audiences (0.16s)
    --- FAIL: TestCreds_audiences/default_to_token_default_audiences (0.01s)
        helpers.go:400: 
            	Error Trace:	/Users/thy.ton/go/src/github.com/hashicorp/vault-plugin-secrets-kubernetes/integrationtest/helpers.go:400
            	            				/Users/thy.ton/go/src/github.com/hashicorp/vault-plugin-secrets-kubernetes/integrationtest/creds_integration_test.go:197
            	Error:      	Not equal: 
            	            	expected: []interface {}{"foo", "bar"}
            	            	actual  : []interface {}{"bar", "foo"}
            	            	
            	            	Diff:
            	            	--- Expected
            	            	+++ Actual
            	            	@@ -1,4 +1,4 @@
            	            	 ([]interface {}) (len=2) {
            	            	- (string) (len=3) "foo",
            	            	- (string) (len=3) "bar"
            	            	+ (string) (len=3) "bar",
            	            	+ (string) (len=3) "foo"
            	            	 }
            	Test:       	TestCreds_audiences/default_to_token_default_audiences

Copy link
Member

Choose a reason for hiding this comment

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

assert.ElementsMatch might work then.

@thyton thyton force-pushed the VAULT-6292-k8s-se-support-audiences-on-token-create branch from 5dbbd31 to bb83ec4 Compare March 9, 2023 23:36
Copy link
Member

@tvoran tvoran left a comment

Choose a reason for hiding this comment

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

This is looking good, just a couple suggestions.

path_roles.go Outdated Show resolved Hide resolved
path_roles.go Outdated Show resolved Hide resolved
Copy link
Contributor

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

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

Looks great! +1 to Theron's suggestions

@thyton thyton force-pushed the VAULT-6292-k8s-se-support-audiences-on-token-create branch from 05b9fe0 to 1b2b7ef Compare March 10, 2023 19:28
@thyton thyton merged commit fb49b9f into main Mar 10, 2023
@thyton thyton deleted the VAULT-6292-k8s-se-support-audiences-on-token-create branch March 10, 2023 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants