From c898a26ba031298aa17abb81ddc1fa863a45019d Mon Sep 17 00:00:00 2001 From: Ronald Date: Wed, 1 Mar 2023 21:00:37 +0100 Subject: [PATCH] Improve ux to help users avoid overwriting fields of ACL tokens, roles and policies (#16288) * Deprecate merge-policies and add options add-policy-name/add-policy-id to improve CLI token update command * deprecate merge-roles fields * Fix potential flakey tests and update ux to remove 'completely' + typo fixes --- .changelog/16288.txt | 8 ++ command/acl/token/update/token_update.go | 91 +++++++++++++++---- command/acl/token/update/token_update_test.go | 89 ++++++++++++++++++ website/content/commands/acl/token/update.mdx | 30 ++++-- 4 files changed, 193 insertions(+), 25 deletions(-) create mode 100644 .changelog/16288.txt diff --git a/.changelog/16288.txt b/.changelog/16288.txt new file mode 100644 index 000000000..5e2820ec2 --- /dev/null +++ b/.changelog/16288.txt @@ -0,0 +1,8 @@ +```release-note:deprecation +cli: Deprecate the `-merge-policies` and `-merge-roles` flags from the `consul token update` command in favor of: `-append-policy-id`, `-append-policy-name`, `-append-role-name`, and `-append-role-id`. +``` + +```release-note:improvement +cli: added `-append-policy-id`, `-append-policy-name`, `-append-role-name`, and `-append-role-id` flags to the `consul token update` command. +These flags allow updates to a token's policies/roles without having to override them completely. +``` \ No newline at end of file diff --git a/command/acl/token/update/token_update.go b/command/acl/token/update/token_update.go index 4f168e927..6cb529edd 100644 --- a/command/acl/token/update/token_update.go +++ b/command/acl/token/update/token_update.go @@ -27,30 +27,31 @@ type cmd struct { tokenAccessorID string policyIDs []string + appendPolicyIDs []string policyNames []string + appendPolicyNames []string roleIDs []string + appendRoleIDs []string roleNames []string + appendRoleNames []string serviceIdents []string nodeIdents []string description string - mergePolicies bool - mergeRoles bool mergeServiceIdents bool mergeNodeIdents bool showMeta bool format string - tokenID string // DEPRECATED + // DEPRECATED + mergeRoles bool + mergePolicies bool + tokenID string } func (c *cmd) init() { c.flags = flag.NewFlagSet("", flag.ContinueOnError) c.flags.BoolVar(&c.showMeta, "meta", false, "Indicates that token metadata such "+ "as the content hash and raft indices should be shown for each entry") - c.flags.BoolVar(&c.mergePolicies, "merge-policies", false, "Merge the new policies "+ - "with the existing policies") - c.flags.BoolVar(&c.mergeRoles, "merge-roles", false, "Merge the new roles "+ - "with the existing roles") c.flags.BoolVar(&c.mergeServiceIdents, "merge-service-identities", false, "Merge the new service identities "+ "with the existing service identities") c.flags.BoolVar(&c.mergeNodeIdents, "merge-node-identities", false, "Merge the new node identities "+ @@ -60,13 +61,21 @@ func (c *cmd) init() { "matches multiple token Accessor IDs") c.flags.StringVar(&c.description, "description", "", "A description of the token") c.flags.Var((*flags.AppendSliceValue)(&c.policyIDs), "policy-id", "ID of a "+ - "policy to use for this token. May be specified multiple times") + "policy to use for this token. Overwrites existing policies. May be specified multiple times") + c.flags.Var((*flags.AppendSliceValue)(&c.appendPolicyIDs), "append-policy-id", "ID of a "+ + "policy to use for this token. The token retains existing policies. May be specified multiple times") c.flags.Var((*flags.AppendSliceValue)(&c.policyNames), "policy-name", "Name of a "+ - "policy to use for this token. May be specified multiple times") + "policy to use for this token. Overwrites existing policies. May be specified multiple times") + c.flags.Var((*flags.AppendSliceValue)(&c.appendPolicyNames), "append-policy-name", "Name of a "+ + "policy to add to this token. The token retains existing policies. May be specified multiple times") c.flags.Var((*flags.AppendSliceValue)(&c.roleIDs), "role-id", "ID of a "+ - "role to use for this token. May be specified multiple times") + "role to use for this token. Overwrites existing roles. May be specified multiple times") c.flags.Var((*flags.AppendSliceValue)(&c.roleNames), "role-name", "Name of a "+ - "role to use for this token. May be specified multiple times") + "role to use for this token. Overwrites existing roles. May be specified multiple times") + c.flags.Var((*flags.AppendSliceValue)(&c.appendRoleIDs), "append-role-id", "ID of a "+ + "role to add to this token. The token retains existing roles. May be specified multiple times") + c.flags.Var((*flags.AppendSliceValue)(&c.appendRoleNames), "append-role-name", "Name of a "+ + "role to add to this token. The token retains existing roles. May be specified multiple times") c.flags.Var((*flags.AppendSliceValue)(&c.serviceIdents), "service-identity", "Name of a "+ "service identity to use for this token. May be specified multiple times. Format is "+ "the SERVICENAME or SERVICENAME:DATACENTER1,DATACENTER2,...") @@ -87,8 +96,11 @@ func (c *cmd) init() { c.help = flags.Usage(help, c.flags) // Deprecations - c.flags.StringVar(&c.tokenID, "id", "", - "DEPRECATED. Use -accessor-id instead.") + c.flags.StringVar(&c.tokenID, "id", "", "DEPRECATED. Use -accessor-id instead.") + c.flags.BoolVar(&c.mergePolicies, "merge-policies", false, "DEPRECATED. "+ + "Use -append-policy-id or -append-policy-name instead.") + c.flags.BoolVar(&c.mergeRoles, "merge-roles", false, "DEPRECATED. "+ + "Use -append-role-id or -append-role-name instead.") } func (c *cmd) Run(args []string) int { @@ -148,6 +160,9 @@ func (c *cmd) Run(args []string) int { } if c.mergePolicies { + c.UI.Warn("merge-policies is deprecated and will be removed in a future Consul version. " + + "Use `append-policy-name` or `append-policy-id` instead.") + for _, policyName := range c.policyNames { found := false for _, link := range t.Policies { @@ -184,15 +199,33 @@ func (c *cmd) Run(args []string) int { } } } else { - t.Policies = nil - for _, policyName := range c.policyNames { + hasAddPolicyFields := len(c.appendPolicyNames) > 0 || len(c.appendPolicyIDs) > 0 + hasPolicyFields := len(c.policyIDs) > 0 || len(c.policyNames) > 0 + + if hasPolicyFields && hasAddPolicyFields { + c.UI.Error("Cannot combine the use of policy-id/policy-name flags with append- variants. " + + "To set or overwrite existing policies, use -policy-id or -policy-name. " + + "To append to existing policies, use -append-policy-id or -append-policy-name.") + return 1 + } + + policyIDs := c.appendPolicyIDs + policyNames := c.appendPolicyNames + + if hasPolicyFields { + policyIDs = c.policyIDs + policyNames = c.policyNames + t.Policies = nil + } + + for _, policyName := range policyNames { // We could resolve names to IDs here but there isn't any reason why its would be better // than allowing the agent to do it. t.Policies = append(t.Policies, &api.ACLTokenPolicyLink{Name: policyName}) } - for _, policyID := range c.policyIDs { + for _, policyID := range policyIDs { policyID, err := acl.GetPolicyIDFromPartial(client, policyID) if err != nil { c.UI.Error(fmt.Sprintf("Error resolving policy ID %s: %v", policyID, err)) @@ -203,6 +236,9 @@ func (c *cmd) Run(args []string) int { } if c.mergeRoles { + c.UI.Warn("merge-roles is deprecated and will be removed in a future Consul version. " + + "Use `append-role-name` or `append-role-id` instead.") + for _, roleName := range c.roleNames { found := false for _, link := range t.Roles { @@ -239,15 +275,32 @@ func (c *cmd) Run(args []string) int { } } } else { - t.Roles = nil + hasAddRoleFields := len(c.appendRoleNames) > 0 || len(c.appendRoleIDs) > 0 + hasRoleFields := len(c.roleIDs) > 0 || len(c.roleNames) > 0 - for _, roleName := range c.roleNames { + if hasRoleFields && hasAddRoleFields { + c.UI.Error("Cannot combine the use of role-id/role-name flags with append- variants. " + + "To set or overwrite existing roles, use -role-id or -role-name. " + + "To append to existing roles, use -append-role-id or -append-role-name.") + return 1 + } + + roleNames := c.appendRoleNames + roleIDs := c.appendRoleIDs + + if hasRoleFields { + roleNames = c.roleNames + roleIDs = c.roleIDs + t.Roles = nil + } + + for _, roleName := range roleNames { // We could resolve names to IDs here but there isn't any reason why its would be better // than allowing the agent to do it. t.Roles = append(t.Roles, &api.ACLTokenRoleLink{Name: roleName}) } - for _, roleID := range c.roleIDs { + for _, roleID := range roleIDs { roleID, err := acl.GetRoleIDFromPartial(client, roleID) if err != nil { c.UI.Error(fmt.Sprintf("Error resolving role ID %s: %v", roleID, err)) diff --git a/command/acl/token/update/token_update_test.go b/command/acl/token/update/token_update_test.go index 541d1e225..bd11d1d8e 100644 --- a/command/acl/token/update/token_update_test.go +++ b/command/acl/token/update/token_update_test.go @@ -148,6 +148,95 @@ func TestTokenUpdateCommand(t *testing.T) { }) } +func TestTokenUpdateCommandWithAppend(t *testing.T) { + if testing.Short() { + t.Skip("too slow for testing.Short") + } + + t.Parallel() + + a := agent.NewTestAgent(t, ` + primary_datacenter = "dc1" + acl { + enabled = true + tokens { + initial_management = "root" + } + }`) + + defer a.Shutdown() + testrpc.WaitForLeader(t, a.RPC, "dc1") + + // Create a policy + client := a.Client() + + policy, _, err := client.ACL().PolicyCreate( + &api.ACLPolicy{Name: "test-policy"}, + &api.WriteOptions{Token: "root"}, + ) + require.NoError(t, err) + + // create a token + token, _, err := client.ACL().TokenCreate( + &api.ACLToken{Description: "test", Policies: []*api.ACLTokenPolicyLink{{Name: policy.Name}}}, + &api.WriteOptions{Token: "root"}, + ) + require.NoError(t, err) + + //secondary policy + secondPolicy, _, policyErr := client.ACL().PolicyCreate( + &api.ACLPolicy{Name: "secondary-policy"}, + &api.WriteOptions{Token: "root"}, + ) + require.NoError(t, policyErr) + + //third policy + thirdPolicy, _, policyErr := client.ACL().PolicyCreate( + &api.ACLPolicy{Name: "third-policy"}, + &api.WriteOptions{Token: "root"}, + ) + require.NoError(t, policyErr) + + run := func(t *testing.T, args []string) *api.ACLToken { + ui := cli.NewMockUi() + cmd := New(ui) + + code := cmd.Run(append(args, "-format=json")) + require.Equal(t, 0, code) + require.Empty(t, ui.ErrorWriter.String()) + + var token api.ACLToken + require.NoError(t, json.Unmarshal(ui.OutputWriter.Bytes(), &token)) + return &token + } + + // update with append-policy-name + t.Run("append-policy-name", func(t *testing.T) { + token := run(t, []string{ + "-http-addr=" + a.HTTPAddr(), + "-accessor-id=" + token.AccessorID, + "-token=root", + "-append-policy-name=" + secondPolicy.Name, + "-description=test token", + }) + + require.Len(t, token.Policies, 2) + }) + + // update with append-policy-id + t.Run("append-policy-id", func(t *testing.T) { + token := run(t, []string{ + "-http-addr=" + a.HTTPAddr(), + "-accessor-id=" + token.AccessorID, + "-token=root", + "-append-policy-id=" + thirdPolicy.ID, + "-description=test token", + }) + + require.Len(t, token.Policies, 3) + }) +} + func TestTokenUpdateCommand_JSON(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") diff --git a/website/content/commands/acl/token/update.mdx b/website/content/commands/acl/token/update.mdx index 28158e6db..19441e102 100644 --- a/website/content/commands/acl/token/update.mdx +++ b/website/content/commands/acl/token/update.mdx @@ -36,9 +36,15 @@ Usage: `consul acl token update [options]` - `merge-node-identities` - Merge the new node identities with the existing node identities. -- `-merge-policies` - Merge the new policies with the existing policies. +- `-merge-policies` - Deprecated. Merge the new policies with the existing policies. -- `-merge-roles` - Merge the new roles with the existing roles. +~> This is deprecated and will be removed in a future Consul version. Use `append-policy-id` or `append-policy-name` +instead. + +- `-merge-roles` - Deprecated. Merge the new roles with the existing roles. + +~> This is deprecated and will be removed in a future Consul version. Use `append-role-id` or `append-role-name` +instead. - `-merge-service-identities` - Merge the new service identities with the existing service identities. @@ -49,13 +55,25 @@ Usage: `consul acl token update [options]` be specified multiple times. Format is `NODENAME:DATACENTER`. Added in Consul 1.8.1. -- `-policy-id=` - ID of a policy to use for this token. May be specified multiple times. +- `-policy-id=` - ID of a policy to use for this token. Overwrites existing policies. May be specified multiple times. -- `-policy-name=` - Name of a policy to use for this token. May be specified multiple times. +- `-policy-name=` - Name of a policy to use for this token. Overwrites existing policies. May be specified multiple times. -- `-role-id=` - ID of a role to use for this token. May be specified multiple times. +~> `-policy-id` and `-policy-name` will overwrite existing token policies. Use `-append-policy-id` or `-append-policy-name` to retain existing policies. -- `-role-name=` - Name of a role to use for this token. May be specified multiple times. +- `-append-policy-id=` - ID of policy to be added for this token. The token retains existing policies. May be specified multiple times. + +- `-append-policy-name=` - Name of a policy to be added for this token. The token retains existing policies. May be specified multiple times. + +- `-role-id=` - ID of a role to use for this token. Overwrites existing roles. May be specified multiple times. + +- `-role-name=` - Name of a role to use for this token. Overwrites existing roles. May be specified multiple times. + +~> `-role-id` and `-role-name` will overwrite existing roles. Use `-append-role-id` or `-append-role-name` to retain the existing roles. + +- `-append-role-id=` - ID of a role to add to this token. The token retains existing roles. May be specified multiple times. + +- `-append-role-name=` - Name of a role to add to this token. The token retains existing roles. May be specified multiple times. - `-service-identity=` - Name of a service identity to use for this token. May be specified multiple times. Format is the `SERVICENAME` or