-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Changes remote exec KV read to call GetTokenForAgent(). #3283
Conversation
agent/remote_exec_test.go
Outdated
cfg.ACLDatacenter = "dc1" | ||
cfg.ACLAgentToken = "root" | ||
cfg.ACLDefaultPolicy = "deny" | ||
testRemoteExecGetSpec(t, cfg) |
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.
I am not sure if this is testing anything here. It is passing this config struct to testRemoteExecGetSpec
, but that doesn't seem to get passed to the test agent it creates there. What exactly is the intention of this TestRemoteExecGetSpec_ACLAgentToken
test case?
agent/remote_exec_test.go
Outdated
cfg.ACLDefaultPolicy = "deny" | ||
testRemoteExecGetSpec(t, cfg) | ||
} | ||
|
||
func testRemoteExecGetSpec(t *testing.T, c *Config) { | ||
a := NewTestAgent(t.Name(), nil) |
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.
Ah, maybe the second arg is not supposed to be nil but c instead?
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.
Oh good catch - yeah this must be working because ACLs aren't getting enabled for either case. This is supposed to make sure we can get the token from the ACL agent token, and the other test checks the ACL token. I'll pass that through and make sure it looks like it's properly using ACLs.
the acl_agent_token instead of the acl_token. Fixes #3160.
@preetapan did a rebase and updated that unit test, can you please take another look? |
The unhappy path test found a similar |
This lets it use the
acl_agent_token
instead of just theacl_token
. Docs were also updated accordingly.Fixes #3160.