-
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
Rename master
and agent_master
ACL tokens in the config file format
#11665
Conversation
🤔 This PR has changes in the |
162b746
to
efe4b21
Compare
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.
Looks great, nice work!
One suggestion below for making the deprecated fields a little more explicit even though we can't use the DeprecatedConfig struct. Also happy to chat about other options if you think there is a better way.
I think any of that could be done as a follow up, so nothing blocking a merge I can see.
26622a8
to
9c2bc3c
Compare
9c2bc3c
to
2d10e0d
Compare
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.
@boxofrad : thanks for taking this on! I proposed some changes that make the docs easier to understand for users on Consul 1.4 - 1.10. Let me know what you think.
Thanks @jkirschner-hashicorp, that's much better. Let me know what you think now? |
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 the changes, @boxofrad!
I added a few small comments to consider before merging, but have given my approval.
@@ -16,7 +16,7 @@ the [ACL tutorial](https://learn.hashicorp.com/tutorials/consul/access-control-s | |||
## Bootstrap ACLs | |||
|
|||
This endpoint does a special one-time bootstrap of the ACL system, making the first | |||
management token if the [`acl.tokens.master`](/docs/agent/options#acl_tokens_master) | |||
management token if the [`acl.tokens.initial_management`](/docs/agent/options#acl_tokens_initial_management) |
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.
Is the intent that, if someone is on Consul 1.11, they will follow this link, see
This is available in Consul 1.11 and later. In prior versions, use
acl.tokens.master
and then find out the correct (previous) name that way?
I imagine we've had this problem before (and will have new instances of this problem in the future) when we rename things... do we mention every name everywhere? Or mention just the most recent name in most docs, but link to information about old names?
[Long term, this might be best solved by having versioned docs pages. But that's well outside the scope of this PR :) ]
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.
Yep! In this case, it's a bit more onerous because we've already renamed the token before (acl_master_token
⇒ acl.tokens.master
⇒ acl.tokens.initial_management
) so users on very old versions have a bit of a trail to follow.
do we mention every name everywhere? Or mention just the most recent name in most docs
That seems to be the case, yeah. For example, the current ACL System page mentions acl.tokens.agent_master
, not acl_agent_master_token
.
- `master` ((#acl_tokens_master)) **Deprecated in Consul 1.11. Use the | ||
[`acl.tokens.initial_management`](#acl_tokens_initial_management) field 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.
For someone on Consul <1.11, I'm not sure this text makes it clear that the initial_management
token is a direct replacement (a rename), so the docs for initial_management
apply here, too.
Perhaps this? Let me know what you think.
- `master` ((#acl_tokens_master)) **Deprecated in Consul 1.11. Use the | |
[`acl.tokens.initial_management`](#acl_tokens_initial_management) field instead.** | |
- `master` ((#acl_tokens_master)) **Renamed in Consul 1.11 to | |
[`acl.tokens.initial_management`](#acl_tokens_initial_management).** |
Or if we must include the word deprecated...
- `master` ((#acl_tokens_master)) **Deprecated in Consul 1.11. Use the | |
[`acl.tokens.initial_management`](#acl_tokens_initial_management) field instead.** | |
- `master` ((#acl_tokens_master)) **Deprecated in Consul 1.11: renamed to | |
[`acl.tokens.initial_management`](#acl_tokens_initial_management).** |
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 agree, rename is clearer 👍🏻
This should only be used by operators during outages, regular ACL tokens should normally | ||
be used by applications. | ||
|
||
- `agent_master` ((#acl_tokens_agent_master)) **Deprecated in Consul 1.11. Use the |
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.
Same comment as above (mention that it was renamed)
🍒 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/512839. |
This is a first step towards removing this non-inclusive language. In subsequent PRs we will update the naming in the API, the
consul acl set-agent-token
command, the auto-config API, and internal references, struct field names etc.