Improve ux around ACL token to help users avoid overwriting node/service identities (#16506)

* Deprecate merge-node-identities and merge-service-identities flags * added tests for node identities changes * added changelog file and docs
2023-03-06 16:00:39 +01:00 · 2023-03-06 16:00:39 +01:00 · bf501a337b
parent fc232326a0
commit bf501a337b
5 changed files with 125 additions and 29 deletions
--- a/.changelog/16506.txt
+++ b/.changelog/16506.txt
@ -0,0 +1,8 @@
+```release-note:deprecation
+cli: Deprecate the `-merge-node-identites` and `-merge-service-identities` flags from the `consul token update` command in favor of: `-append-node-identity` and `-append-service-identity`.
+```
+
+```release-note:improvement
+cli: added `-append-service-identity` and `-append-node-identity` flags to the `consul token update` command.
+These flags allow updates to a token's node identities/service identities without having to override them.  
+```
--- a/command/acl/token/update/token_update.go
+++ b/command/acl/token/update/token_update.go
@ -25,37 +25,35 @@ type cmd struct {
 	http  *flags.HTTPFlags
 	help  string

-	tokenAccessorID    string
-	policyIDs          []string
-	appendPolicyIDs    []string
-	policyNames        []string
-	appendPolicyNames  []string
-	roleIDs            []string
-	appendRoleIDs      []string
-	roleNames          []string
-	appendRoleNames    []string
-	serviceIdents      []string
-	nodeIdents         []string
-	description        string
-	mergeServiceIdents bool
-	mergeNodeIdents    bool
-	showMeta           bool
-	format             string
+	tokenAccessorID     string
+	policyIDs           []string
+	appendPolicyIDs     []string
+	policyNames         []string
+	appendPolicyNames   []string
+	roleIDs             []string
+	appendRoleIDs       []string
+	roleNames           []string
+	appendRoleNames     []string
+	serviceIdents       []string
+	nodeIdents          []string
+	appendNodeIdents    []string
+	appendServiceIdents []string
+	description         string
+	showMeta            bool
+	format              string

 	// DEPRECATED
-	mergeRoles    bool
-	mergePolicies bool
-	tokenID       string
+	mergeServiceIdents bool
+	mergeNodeIdents    bool
+	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.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 "+
-		"with the existing node identities")
 	c.flags.StringVar(&c.tokenAccessorID, "accessor-id", "", "The Accessor ID of the token to update. "+
 		"It may be specified as a unique ID prefix but will error if the prefix "+
 		"matches multiple token Accessor IDs")
@ -79,9 +77,15 @@ func (c *cmd) init() {
 	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,...")
+	c.flags.Var((*flags.AppendSliceValue)(&c.appendServiceIdents), "append-service-identity", "Name of a "+
+		"service identity to use for this token. This token retains existing service identities. May be specified"+
+		"multiple times. Format is the SERVICENAME or SERVICENAME:DATACENTER1,DATACENTER2,...")
 	c.flags.Var((*flags.AppendSliceValue)(&c.nodeIdents), "node-identity", "Name of a "+
 		"node identity to use for this token. May be specified multiple times. Format is "+
 		"NODENAME:DATACENTER")
+	c.flags.Var((*flags.AppendSliceValue)(&c.appendNodeIdents), "append-node-identity", "Name of a "+
+		"node identity to use for this token. This token retains existing node identities. May be "+
+		"specified multiple times. Format is NODENAME:DATACENTER")
 	c.flags.StringVar(
 		&c.format,
 		"format",
@ -101,6 +105,10 @@ func (c *cmd) init() {
 		"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.")
+	c.flags.BoolVar(&c.mergeServiceIdents, "merge-service-identities", false, "DEPRECATED. "+
+		"Use -append-service-identity instead.")
+	c.flags.BoolVar(&c.mergeNodeIdents, "merge-node-identities", false, "DEPRECATED. "+
+		"Use -append-node-identity instead.")
 }

 func (c *cmd) Run(args []string) int {
@ -147,13 +155,38 @@ func (c *cmd) Run(args []string) int {
 		t.Description = c.description
 	}

+	hasAppendServiceFields := len(c.appendServiceIdents) > 0
+	hasServiceFields := len(c.serviceIdents) > 0
+	if hasAppendServiceFields && hasServiceFields {
+		c.UI.Error("Cannot combine the use of service-identity flag with append-service-identity. " +
+			"To set or overwrite existing service identities, use -service-identity. " +
+			"To append to existing service identities, use -append-service-identity.")
+		return 1
+	}
+
 	parsedServiceIdents, err := acl.ExtractServiceIdentities(c.serviceIdents)
+	if hasAppendServiceFields {
+		parsedServiceIdents, err = acl.ExtractServiceIdentities(c.appendServiceIdents)
+	}
 	if err != nil {
 		c.UI.Error(err.Error())
 		return 1
 	}

+	hasAppendNodeFields := len(c.appendNodeIdents) > 0
+	hasNodeFields := len(c.nodeIdents) > 0
+
+	if hasAppendNodeFields && hasNodeFields {
+		c.UI.Error("Cannot combine the use of node-identity flag with append-node-identity. " +
+			"To set or overwrite existing node identities, use -node-identity. " +
+			"To append to existing node identities, use -append-node-identity.")
+		return 1
+	}
+
 	parsedNodeIdents, err := acl.ExtractNodeIdentities(c.nodeIdents)
+	if hasAppendNodeFields {
+		parsedNodeIdents, err = acl.ExtractNodeIdentities(c.appendNodeIdents)
+	}
 	if err != nil {
 		c.UI.Error(err.Error())
 		return 1
@ -310,7 +343,7 @@ func (c *cmd) Run(args []string) int {
 		}
 	}

-	if c.mergeServiceIdents {
+	if c.mergeServiceIdents || hasAppendServiceFields {
 		for _, svcid := range parsedServiceIdents {
 			found := -1
 			for i, link := range t.ServiceIdentities {
@ -330,7 +363,7 @@ func (c *cmd) Run(args []string) int {
 		t.ServiceIdentities = parsedServiceIdents
 	}

-	if c.mergeNodeIdents {
+	if c.mergeNodeIdents || hasAppendNodeFields {
 		for _, nodeid := range parsedNodeIdents {
 			found := false
 			for _, link := range t.NodeIdentities {
--- a/command/acl/token/update/token_update_test.go
+++ b/command/acl/token/update/token_update_test.go
@ -109,6 +109,22 @@ func TestTokenUpdateCommand(t *testing.T) {
 		require.ElementsMatch(t, expected, token.NodeIdentities)
 	})

+	// update with append-node-identity
+	t.Run("append-node-identity", func(t *testing.T) {
+
+		token := run(t, []string{
+			"-http-addr=" + a.HTTPAddr(),
+			"-accessor-id=" + token.AccessorID,
+			"-token=root",
+			"-append-node-identity=third:node",
+			"-description=test token",
+		})
+
+		require.Len(t, token.NodeIdentities, 3)
+		require.Equal(t, "third", token.NodeIdentities[2].NodeName)
+		require.Equal(t, "node", token.NodeIdentities[2].Datacenter)
+	})
+
 	// update with policy by name
 	t.Run("policy-name", func(t *testing.T) {
 		token := run(t, []string{
@ -135,6 +151,33 @@ func TestTokenUpdateCommand(t *testing.T) {
 		require.Len(t, token.Policies, 1)
 	})

+	// update with service-identity
+	t.Run("service-identity", func(t *testing.T) {
+		token := run(t, []string{
+			"-http-addr=" + a.HTTPAddr(),
+			"-accessor-id=" + token.AccessorID,
+			"-token=root",
+			"-service-identity=service:datapalace",
+			"-description=test token",
+		})
+
+		require.Len(t, token.ServiceIdentities, 1)
+		require.Equal(t, "service", token.ServiceIdentities[0].ServiceName)
+	})
+
+	// update with append-service-identity
+	t.Run("append-service-identity", func(t *testing.T) {
+		token := run(t, []string{
+			"-http-addr=" + a.HTTPAddr(),
+			"-accessor-id=" + token.AccessorID,
+			"-token=root",
+			"-append-service-identity=web",
+			"-description=test token",
+		})
+		require.Len(t, token.ServiceIdentities, 2)
+		require.Equal(t, "web", token.ServiceIdentities[1].ServiceName)
+	})
+
 	// update with no description shouldn't delete the current description
 	t.Run("merge-description", func(t *testing.T) {
 		token := run(t, []string{
--- a/website/content/commands/acl/policy/update.mdx
+++ b/website/content/commands/acl/policy/update.mdx
@ -49,6 +49,8 @@ Usage: `consul acl policy update [options] [args]`
  the value is a file path to load the rules from. `-` may also be given to
  indicate that the rules are available on stdin.

+~> Specifying `-rules` will overwrite existing rules.
+
 - `-valid-datacenter=<value>` - Datacenter that the policy should be valid within.
  This flag may be specified multiple times.

--- a/website/content/commands/acl/token/update.mdx
+++ b/website/content/commands/acl/token/update.mdx
@ -33,8 +33,9 @@ Usage: `consul acl token update [options]`
 - `-id=<string>` - The Accessor ID of the token to read. It may be specified as a
  unique ID prefix but will error if the prefix matches multiple token Accessor IDs

- `merge-node-identities` - Merge the new node identities with the existing node
+- `-merge-node-identities` - Deprecated. Merge the new node identities with the existing node
  identities.
+~> This is deprecated and will be removed in a future Consul version. Use `append-node-identity` instead.

 - `-merge-policies` - Deprecated. Merge the new policies with the existing policies.

@ -46,15 +47,20 @@ instead.
 ~> 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.
+- `-merge-service-identities` - Deprecated. Merge the new service identities with the existing service identities.
+
+~> This is deprecated and will be removed in a future Consul version. Use `append-service-identity` instead.

 - `-meta` - Indicates that token metadata such as the content hash and Raft indices should be
  shown for each entry.

- `-node-identity=<value>` - Name of a node identity to use for this role. May
+- `-node-identity=<value>` - Name of a node identity to use for this role. Overwrites existing node identity. May
  be specified multiple times. Format is `NODENAME:DATACENTER`. Added in Consul
  1.8.1.

+- `-append-node-identity=<value>` - Name of a node identity to add to this role. May
+  be specified multiple times. The token retains existing node identities. Format is `NODENAME:DATACENTER`.
+
 - `-policy-id=<value>` - ID of a policy to use for this token. Overwrites existing policies. May be specified multiple times.

 - `-policy-name=<value>` - Name of a policy to use for this token. Overwrites existing policies. May be specified multiple times.
@ -76,9 +82,13 @@ instead.
 - `-append-role-name=<value>` - Name of a role to add to this token. The token retains existing roles. May be specified multiple times.

 - `-service-identity=<value>` - Name of a service identity to use for this
-  token. May be specified multiple times. Format is the `SERVICENAME` or
+  token. Overwrites existing service identities. May be specified multiple times. Format is the `SERVICENAME` or
  `SERVICENAME:DATACENTER1,DATACENTER2,...`

+- `-append-service-identity=<value>` - Name of a service identity to add to this
+  token. May be specified multiple times. The token retains existing service identities.
+  Format is the `SERVICENAME` or `SERVICENAME:DATACENTER1,DATACENTER2,...`
+
 - `-format={pretty|json}` - Command output format. The default value is `pretty`.

 #### Enterprise Options