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

Rename agent_master ACL token in the API and CLI #11669

Merged
merged 7 commits into from
Dec 2, 2021
Merged

Rename agent_master ACL token in the API and CLI #11669

merged 7 commits into from
Dec 2, 2021

Conversation

boxofrad
Copy link
Contributor

Following on from #11665, this PR renames the agent master token in the API and CLI.

@boxofrad boxofrad requested a review from a team as a code owner November 26, 2021 16:36
@github-actions github-actions bot added theme/api Relating to the HTTP API interface theme/cli Flags and documentation for the CLI interface type/docs Documentation needs to be created/updated/clarified labels Nov 26, 2021
@boxofrad boxofrad force-pushed the boxofrad/rename-master-acl-tokens-api branch from c8f3fb3 to 4456ee7 Compare November 26, 2021 22:18
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging November 26, 2021 22:18 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging November 26, 2021 22:28 Inactive
@vercel vercel bot temporarily deployed to Preview – consul November 26, 2021 22:28 Inactive
@boxofrad boxofrad force-pushed the boxofrad/rename-master-acl-tokens-api branch from cfcf6bc to 73aab89 Compare November 26, 2021 22:30
@vercel vercel bot temporarily deployed to Preview – consul November 26, 2021 22:30 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging November 26, 2021 22:30 Inactive
@boxofrad boxofrad force-pushed the boxofrad/rename-master-acl-tokens branch from 9c2bc3c to 2d10e0d Compare November 26, 2021 22:32
@boxofrad boxofrad force-pushed the boxofrad/rename-master-acl-tokens-api branch from 73aab89 to 7b7db29 Compare November 26, 2021 22:35
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging November 26, 2021 22:35 Inactive
@vercel vercel bot temporarily deployed to Preview – consul November 26, 2021 22:35 Inactive
@boxofrad boxofrad force-pushed the boxofrad/rename-master-acl-tokens-api branch from 7b7db29 to 5984b76 Compare November 30, 2021 15:45
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging November 30, 2021 15:45 Inactive
@vercel vercel bot temporarily deployed to Preview – consul November 30, 2021 15:45 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging November 30, 2021 16:02 Inactive
@boxofrad boxofrad force-pushed the boxofrad/rename-master-acl-tokens-api branch from 9e8ddb3 to 9617729 Compare November 30, 2021 16:04
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging November 30, 2021 16:04 Inactive
Base automatically changed from boxofrad/rename-master-acl-tokens to main December 1, 2021 21:08
@boxofrad boxofrad force-pushed the boxofrad/rename-master-acl-tokens-api branch from 9617729 to 44d8abd Compare December 1, 2021 21:12
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging December 1, 2021 21:12 Inactive
dhiaayachi
dhiaayachi reviewed Dec 1, 2021
View reviewed changes
Copy link
Collaborator

@dhiaayachi dhiaayachi left a comment

Choose a reason for hiding this comment

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

Thank you for working on this @boxofrad!!
I added few comments about master mention in tests and deprecation notice.

agent/agent_endpoint_test.go
@@ -5308,14 +5308,23 @@ func TestAgent_Token(t *testing.T) {
effective: tokens{master: "M"},
},
{
name: "set master ",
name: "set master",
Copy link
Collaborator

Choose a reason for hiding this comment

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

While we are at it we should change this too, I see a lot of other references that can be changed in the test file

Copy link
Contributor

@dnephin dnephin Dec 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I believe the plan is to only focus on external references for now. We can cleanup all the internal stuff after the 1.11 release.

But I think in this particular case we would keep it, because it's testing the old name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, exactly that 👍🏻

I have a WIP branch that renames the struct fields etc. but in this case, we'd leave the test name the same.

agent/agent_endpoint.go
@@ -1444,7 +1444,7 @@ func (s *HTTPHandlers) AgentToken(resp http.ResponseWriter, req *http.Request) (
triggerAntiEntropySync = true
}

case "acl_agent_master_token", "agent_master":
case "acl_agent_master_token", "agent_master", "agent_recovery":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the plan to deprecate the acl_agent_master_token and agent_master naming in favour of agent_recovery? If so I don't see any deprecation notice for it in the doc.
It also would be nice to have those defined as const and have a deprecation notice for the ones we want to remove in the future.
What you think about that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. It's not super obvious from the diff, but the target we're switching on is a path suffix - so there's a deprecation notice in the API docs here 🙂

I'd be happy to move them into constants, but as they're only really used in this handler, I wonder if the indirection would be worth it, or make the code less clear?

dnephin
dnephin reviewed Dec 1, 2021
View reviewed changes
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Nice!

One comment about the API client library logic maybe not handling errors correctly.

api/agent.go
meta, status, err := a.updateTokenOnce(target, token, q)
if err != nil && status == 404 {
meta, _, err = a.updateTokenOnce(fallback, token, q)
func (a *Agent) updateTokenFallback(token string, q *WriteOptions, targets ...string) (*WriteMeta, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

varargs, nice! 🎉

@vercel vercel bot temporarily deployed to Preview – consul December 2, 2021 11:26 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging December 2, 2021 11:26 Inactive
dnephin
dnephin approved these changes Dec 2, 2021
View reviewed changes
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM!

@boxofrad boxofrad merged commit bf56a2c into main Dec 2, 2021
@boxofrad boxofrad deleted the boxofrad/rename-master-acl-tokens-api branch December 2, 2021 17:05
@hc-github-team-consul-core
Copy link
Collaborator

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/513279.

This was referenced Dec 6, 2021
Merged
Merged
Merged
@mikemorris mikemorris mentioned this pull request Dec 14, 2021
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnephin dnephin dnephin approved these changes

@dhiaayachi dhiaayachi Awaiting requested review from dhiaayachi

Assignees
No one assigned
Labels
theme/api Relating to the HTTP API interface theme/cli Flags and documentation for the CLI interface type/docs Documentation needs to be created/updated/clarified
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@boxofrad @hc-github-team-consul-core @dnephin @dhiaayachi