From 5a10c732e25e10117169e8b63860d4621345f8e9 Mon Sep 17 00:00:00 2001 From: Ronald Ekambi Date: Thu, 16 Feb 2023 11:41:04 -0500 Subject: [PATCH 1/7] Deprecate merge-policies and add options add-policy-name/add-policy-id to improve CLI token update command --- command/acl/token/update/token_update.go | 47 ++++++++++++++++--- website/content/commands/acl/token/update.mdx | 16 +++++-- 2 files changed, 53 insertions(+), 10 deletions(-) diff --git a/command/acl/token/update/token_update.go b/command/acl/token/update/token_update.go index 4f168e927b8..3c38163c5aa 100644 --- a/command/acl/token/update/token_update.go +++ b/command/acl/token/update/token_update.go @@ -27,28 +27,28 @@ type cmd struct { tokenAccessorID string policyIDs []string + addPolicyIDs []string policyNames []string + addPolicyNames []string roleIDs []string roleNames []string serviceIdents []string nodeIdents []string description string - mergePolicies bool mergeRoles bool mergeServiceIdents bool mergeNodeIdents bool showMeta bool format string - tokenID string // DEPRECATED + mergePolicies bool // DEPRECATED + tokenID string // DEPRECATED } 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 "+ @@ -61,8 +61,12 @@ func (c *cmd) init() { 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") + c.flags.Var((*flags.AppendSliceValue)(&c.addPolicyIDs), "add-policy-id", "ID of a "+ + "policy to use for this token. Appends this policy to 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") + c.flags.Var((*flags.AppendSliceValue)(&c.addPolicyNames), "add-policy-name", "Name of a "+ + "policy to use for this token. Appends this policy to 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") c.flags.Var((*flags.AppendSliceValue)(&c.roleNames), "role-name", "Name of a "+ @@ -87,8 +91,9 @@ 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. Merge the new policies "+ + "with the existing policies. Use -add-policy-id or -add-policy-name instead.") } func (c *cmd) Run(args []string) int { @@ -148,6 +153,9 @@ func (c *cmd) Run(args []string) int { } if c.mergePolicies { + c.UI.Warn("merge-policies is deprecated and will be removed in future consul version. " + + "This is being replaced by `add-policy-name` and `add-policy-id`.") + for _, policyName := range c.policyNames { found := false for _, link := range t.Policies { @@ -184,7 +192,32 @@ func (c *cmd) Run(args []string) int { } } } else { - t.Policies = nil + + hasAddPolicyFields := len(c.addPolicyNames) > 0 || len(c.addPolicyIDs) > 0 + hasPolicyFields := len(c.policyIDs) > 0 || len(c.policyNames) > 0 + + if hasPolicyFields && hasAddPolicyFields { + c.UI.Error("Cannot specified both add-policy-id/add-policy-name and policy-id/policy-name") + return 1 + } + + if !hasAddPolicyFields { + // c.UI.Warn("Overwriting policies with new specified policies") + t.Policies = nil + } else { + for _, addedPolicyName := range c.addPolicyNames { + t.Policies = append(t.Policies, &api.ACLTokenPolicyLink{Name: addedPolicyName}) + } + + for _, addedPolicyId := range c.addPolicyIDs { + policyID, err := acl.GetPolicyIDFromPartial(client, addedPolicyId) + if err != nil { + c.UI.Error(fmt.Sprintf("Error resolving policy ID %s: %v", policyID, err)) + return 1 + } + t.Policies = append(t.Policies, &api.ACLTokenPolicyLink{ID: policyID}) + } + } for _, policyName := range c.policyNames { // We could resolve names to IDs here but there isn't any reason why its would be better diff --git a/website/content/commands/acl/token/update.mdx b/website/content/commands/acl/token/update.mdx index 28158e6db79..36a575fc725 100644 --- a/website/content/commands/acl/token/update.mdx +++ b/website/content/commands/acl/token/update.mdx @@ -36,7 +36,10 @@ 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. + +~> This is deprecated and will be removed in future consul version. Use `add-policy-id` or `add-policy-name` +instead. - `-merge-roles` - Merge the new roles with the existing roles. @@ -49,9 +52,16 @@ 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. Overwrites existing policies. May be specified multiple times. + +~> `-policy-id` and `-policy-name` will completely overwrite existing policies. Use `-add-policy-id` or `-add-policy-name` if you +are trying to append more policies to your existing token policies. + +- `add-policy-id=` - ID of policy to be added for this token. Added to existing policies. May be specified multiple times. -- `-policy-name=` - Name of a policy to use for this token. May be specified multiple times. +- `-add-policy-name=` - Name of a policy to be added for this token. Added to existing policies. May be specified multiple times. - `-role-id=` - ID of a role to use for this token. May be specified multiple times. From dbf3a1941057d25894a28ff4ac965d6dec523f74 Mon Sep 17 00:00:00 2001 From: Ronald Ekambi Date: Thu, 16 Feb 2023 11:43:58 -0500 Subject: [PATCH 2/7] fix some code alignment issues --- command/acl/token/update/token_update.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/command/acl/token/update/token_update.go b/command/acl/token/update/token_update.go index 3c38163c5aa..73a95e59ee9 100644 --- a/command/acl/token/update/token_update.go +++ b/command/acl/token/update/token_update.go @@ -201,10 +201,7 @@ func (c *cmd) Run(args []string) int { return 1 } - if !hasAddPolicyFields { - // c.UI.Warn("Overwriting policies with new specified policies") - t.Policies = nil - } else { + if hasAddPolicyFields { for _, addedPolicyName := range c.addPolicyNames { t.Policies = append(t.Policies, &api.ACLTokenPolicyLink{Name: addedPolicyName}) } @@ -217,6 +214,9 @@ func (c *cmd) Run(args []string) int { } t.Policies = append(t.Policies, &api.ACLTokenPolicyLink{ID: policyID}) } + } else { + // c.UI.Warn("Overwriting policies with new specified policies") + t.Policies = nil } for _, policyName := range c.policyNames { From 29a84c48ebb6321c5034c2f39e5b616eb54b91e3 Mon Sep 17 00:00:00 2001 From: Ronald Ekambi Date: Thu, 16 Feb 2023 17:04:10 -0500 Subject: [PATCH 3/7] deprecate merge-roles fields --- command/acl/token/update/token_update.go | 48 ++++++++++++++++--- website/content/commands/acl/token/update.mdx | 2 +- 2 files changed, 43 insertions(+), 7 deletions(-) diff --git a/command/acl/token/update/token_update.go b/command/acl/token/update/token_update.go index 73a95e59ee9..d6843cf493f 100644 --- a/command/acl/token/update/token_update.go +++ b/command/acl/token/update/token_update.go @@ -31,26 +31,27 @@ type cmd struct { policyNames []string addPolicyNames []string roleIDs []string + addRoleIDs []string roleNames []string + addRoleNames []string serviceIdents []string nodeIdents []string description string - mergeRoles bool mergeServiceIdents bool mergeNodeIdents bool showMeta bool format string - mergePolicies bool // DEPRECATED - 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.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 "+ @@ -71,6 +72,10 @@ func (c *cmd) init() { "role to use for this token. 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") + c.flags.Var((*flags.AppendSliceValue)(&c.addRoleIDs), "add-role-id", "ID of a "+ + "role to add to this token. Appends this role to existing roles. May be specified multiple times") + c.flags.Var((*flags.AppendSliceValue)(&c.addRoleNames), "add-role-name", "Name of a "+ + "role to add to this token. Appends this role to 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,...") @@ -94,6 +99,8 @@ func (c *cmd) init() { c.flags.StringVar(&c.tokenID, "id", "", "DEPRECATED. Use -accessor-id instead.") c.flags.BoolVar(&c.mergePolicies, "merge-policies", false, "Deprecated. Merge the new policies "+ "with the existing policies. Use -add-policy-id or -add-policy-name instead.") + c.flags.BoolVar(&c.mergeRoles, "merge-roles", false, "Deprecated. Merge the new roles "+ + "with the existing roles. Use -add-role-id or -add-role-name instead.") } func (c *cmd) Run(args []string) int { @@ -236,6 +243,9 @@ func (c *cmd) Run(args []string) int { } if c.mergeRoles { + c.UI.Warn("merge-roles is deprecated and will be removed in future consul version. " + + "This is being replaced by `add-role-name` and `add-role-id`.") + for _, roleName := range c.roleNames { found := false for _, link := range t.Roles { @@ -272,7 +282,33 @@ func (c *cmd) Run(args []string) int { } } } else { - t.Roles = nil + hasAddRoleFields := len(c.addRoleNames) > 0 || len(c.addRoleIDs) > 0 + hasRoleFields := len(c.roleIDs) > 0 || len(c.roleNames) > 0 + + if hasRoleFields && hasAddRoleFields { + c.UI.Error("Cannot specified both add-role-id/add-role-name and role-id/role-name") + return 1 + } + + if hasAddRoleFields { + for _, roleName := range c.addRoleNames { + // 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.addRoleIDs { + roleID, err := acl.GetRoleIDFromPartial(client, roleID) + if err != nil { + c.UI.Error(fmt.Sprintf("Error resolving role ID %s: %v", roleID, err)) + return 1 + } + t.Roles = append(t.Roles, &api.ACLTokenRoleLink{ID: roleID}) + } + } else { + // c.UI.Warn("Overwriting policies with new specified policies") + t.Roles = nil + } for _, roleName := range c.roleNames { // We could resolve names to IDs here but there isn't any reason why its would be better diff --git a/website/content/commands/acl/token/update.mdx b/website/content/commands/acl/token/update.mdx index 36a575fc725..c2caed58945 100644 --- a/website/content/commands/acl/token/update.mdx +++ b/website/content/commands/acl/token/update.mdx @@ -59,7 +59,7 @@ instead. ~> `-policy-id` and `-policy-name` will completely overwrite existing policies. Use `-add-policy-id` or `-add-policy-name` if you are trying to append more policies to your existing token policies. -- `add-policy-id=` - ID of policy to be added for this token. Added to existing policies. May be specified multiple times. +- `-add-policy-id=` - ID of policy to be added for this token. Added to existing policies. May be specified multiple times. - `-add-policy-name=` - Name of a policy to be added for this token. Added to existing policies. May be specified multiple times. From 63d2fb4d913698df9352aa0788e2a84fe07bdbb0 Mon Sep 17 00:00:00 2001 From: Ronald Ekambi Date: Tue, 21 Feb 2023 08:48:03 -0500 Subject: [PATCH 4/7] [NET-1723] deduplicate code --- .changelog/16288.txt | 8 ++ command/acl/token/update/token_update.go | 76 +++++++------------ website/content/commands/acl/token/update.mdx | 24 ++++-- 3 files changed, 53 insertions(+), 55 deletions(-) create mode 100644 .changelog/16288.txt diff --git a/.changelog/16288.txt b/.changelog/16288.txt new file mode 100644 index 00000000000..dfbdc26de95 --- /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 favour of: `-add-policy-id`, `-add-policy-name`, `-add-role-name`, and `-add-role-id`. +``` + +```release-note:improvement +cli: added `-add-policy-id`, `-add-policy-name`, `-add-role-name`, and `-add-role-id` flags to the `consul token update` command. +These flags will allow updates to 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 d6843cf493f..03029de6314 100644 --- a/command/acl/token/update/token_update.go +++ b/command/acl/token/update/token_update.go @@ -63,19 +63,19 @@ func (c *cmd) init() { c.flags.Var((*flags.AppendSliceValue)(&c.policyIDs), "policy-id", "ID of a "+ "policy to use for this token. May be specified multiple times") c.flags.Var((*flags.AppendSliceValue)(&c.addPolicyIDs), "add-policy-id", "ID of a "+ - "policy to use for this token. Appends this policy to existing policies. May be specified multiple times") + "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") c.flags.Var((*flags.AppendSliceValue)(&c.addPolicyNames), "add-policy-name", "Name of a "+ - "policy to use for this token. Appends this policy to existing policies. May be specified multiple times") + "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") c.flags.Var((*flags.AppendSliceValue)(&c.roleNames), "role-name", "Name of a "+ "role to use for this token. May be specified multiple times") c.flags.Var((*flags.AppendSliceValue)(&c.addRoleIDs), "add-role-id", "ID of a "+ - "role to add to this token. Appends this role to existing roles. May be specified multiple times") + "role to add to this token. The token retains existing roles. May be specified multiple times") c.flags.Var((*flags.AppendSliceValue)(&c.addRoleNames), "add-role-name", "Name of a "+ - "role to add to this token. Appends this role to existing roles. May be specified multiple times") + "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,...") @@ -97,10 +97,10 @@ func (c *cmd) init() { // Deprecations c.flags.StringVar(&c.tokenID, "id", "", "DEPRECATED. Use -accessor-id instead.") - c.flags.BoolVar(&c.mergePolicies, "merge-policies", false, "Deprecated. Merge the new policies "+ - "with the existing policies. Use -add-policy-id or -add-policy-name instead.") - c.flags.BoolVar(&c.mergeRoles, "merge-roles", false, "Deprecated. Merge the new roles "+ - "with the existing roles. Use -add-role-id or -add-role-name instead.") + c.flags.BoolVar(&c.mergePolicies, "merge-policies", false, "DEPRECATED. "+ + "Use -add-policy-id or -add-policy-name instead.") + c.flags.BoolVar(&c.mergeRoles, "merge-roles", false, "DEPRECATED. "+ + "Use -add-role-id or -add-role-name instead.") } func (c *cmd) Run(args []string) int { @@ -160,8 +160,8 @@ func (c *cmd) Run(args []string) int { } if c.mergePolicies { - c.UI.Warn("merge-policies is deprecated and will be removed in future consul version. " + - "This is being replaced by `add-policy-name` and `add-policy-id`.") + c.UI.Warn("merge-policies is deprecated and will be removed in future Consul version. " + + "Use `add-policy-name` and `add-policy-id` instead.") for _, policyName := range c.policyNames { found := false @@ -204,35 +204,27 @@ func (c *cmd) Run(args []string) int { hasPolicyFields := len(c.policyIDs) > 0 || len(c.policyNames) > 0 if hasPolicyFields && hasAddPolicyFields { - c.UI.Error("Cannot specified both add-policy-id/add-policy-name and policy-id/policy-name") + c.UI.Error("Cannot specify both add-policy-id/add-policy-name and policy-id/policy-name") return 1 } - if hasAddPolicyFields { - for _, addedPolicyName := range c.addPolicyNames { - t.Policies = append(t.Policies, &api.ACLTokenPolicyLink{Name: addedPolicyName}) - } + policyIDs := c.addPolicyIDs + policyNames := c.addPolicyNames - for _, addedPolicyId := range c.addPolicyIDs { - policyID, err := acl.GetPolicyIDFromPartial(client, addedPolicyId) - if err != nil { - c.UI.Error(fmt.Sprintf("Error resolving policy ID %s: %v", policyID, err)) - return 1 - } - t.Policies = append(t.Policies, &api.ACLTokenPolicyLink{ID: policyID}) - } - } else { - // c.UI.Warn("Overwriting policies with new specified policies") + if hasPolicyFields { + policyIDs = c.policyIDs + policyNames = c.policyNames t.Policies = nil + // c.UI.Warn("Overwriting policies with new specified policies") } - for _, policyName := range c.policyNames { + 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)) @@ -243,8 +235,8 @@ func (c *cmd) Run(args []string) int { } if c.mergeRoles { - c.UI.Warn("merge-roles is deprecated and will be removed in future consul version. " + - "This is being replaced by `add-role-name` and `add-role-id`.") + c.UI.Warn("merge-roles is deprecated and will be removed in future Consul version. " + + "Use `add-role-name` and `add-role-id` instead.") for _, roleName := range c.roleNames { found := false @@ -286,37 +278,27 @@ func (c *cmd) Run(args []string) int { hasRoleFields := len(c.roleIDs) > 0 || len(c.roleNames) > 0 if hasRoleFields && hasAddRoleFields { - c.UI.Error("Cannot specified both add-role-id/add-role-name and role-id/role-name") + c.UI.Error("Cannot specify both add-role-id/add-role-name and role-id/role-name") return 1 } - if hasAddRoleFields { - for _, roleName := range c.addRoleNames { - // 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}) - } + roleNames := c.addRoleNames + roleIDs := c.addRoleIDs - for _, roleID := range c.addRoleIDs { - roleID, err := acl.GetRoleIDFromPartial(client, roleID) - if err != nil { - c.UI.Error(fmt.Sprintf("Error resolving role ID %s: %v", roleID, err)) - return 1 - } - t.Roles = append(t.Roles, &api.ACLTokenRoleLink{ID: roleID}) - } - } else { + if hasRoleFields { + roleNames = c.roleNames + roleIDs = c.roleIDs // c.UI.Warn("Overwriting policies with new specified policies") t.Roles = nil } - for _, roleName := range c.roleNames { + 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/website/content/commands/acl/token/update.mdx b/website/content/commands/acl/token/update.mdx index c2caed58945..75ed89d9e98 100644 --- a/website/content/commands/acl/token/update.mdx +++ b/website/content/commands/acl/token/update.mdx @@ -38,10 +38,13 @@ Usage: `consul acl token update [options]` - `-merge-policies` - Deprecated. Merge the new policies with the existing policies. -~> This is deprecated and will be removed in future consul version. Use `add-policy-id` or `add-policy-name` +~> This is deprecated and will be removed in future Consul version. Use `add-policy-id` or `add-policy-name` instead. -- `-merge-roles` - Merge the new roles with the existing roles. +- `-merge-roles` - Deprecated. Merge the new roles with the existing roles. + +~> This is deprecated and will be removed in future Consul version. Use `add-role-id` or `add-role-name` +instead. - `-merge-service-identities` - Merge the new service identities with the existing service identities. @@ -56,16 +59,21 @@ instead. - `-policy-name=` - Name of a policy to use for this token. Overwrites existing policies. May be specified multiple times. -~> `-policy-id` and `-policy-name` will completely overwrite existing policies. Use `-add-policy-id` or `-add-policy-name` if you -are trying to append more policies to your existing token policies. +~> `-policy-id` and `-policy-name` will completely overwrite existing token policies. Use `-add-policy-id` or `-add-policy-name` to retain existing policies. + +- `-add-policy-id=` - ID of policy to be added for this token. The token retains existing policies. May be specified multiple times. + +- `-add-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. -- `-add-policy-id=` - ID of policy to be added for this token. Added to existing policies. May be specified multiple times. +- `-role-name=` - Name of a role to use for this token. Overwrites existing roles. May be specified multiple times. -- `-add-policy-name=` - Name of a policy to be added for this token. Added to existing policies. May be specified multiple times. +~> `-role-id` and `-role-name` will completely overwrite existing policies. Use `-add-role-id` or `-add-role-name` to retain the existing roles. -- `-role-id=` - ID of a role to use for this token. May be specified multiple times. +- `-add-role-id=` - ID of a role to add to this token. The token retains existing roles. May be specified multiple times. -- `-role-name=` - Name of a role to use for this token. May be specified multiple times. +- `-add-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 From f4048866f1137dbfd6b3dea0d93bedcbadc07fb0 Mon Sep 17 00:00:00 2001 From: Ronald Ekambi Date: Tue, 21 Feb 2023 12:11:06 -0500 Subject: [PATCH 5/7] add some basic tests --- command/acl/token/update/token_update.go | 2 -- command/acl/token/update/token_update_test.go | 26 +++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/command/acl/token/update/token_update.go b/command/acl/token/update/token_update.go index 03029de6314..8df986faa35 100644 --- a/command/acl/token/update/token_update.go +++ b/command/acl/token/update/token_update.go @@ -215,7 +215,6 @@ func (c *cmd) Run(args []string) int { policyIDs = c.policyIDs policyNames = c.policyNames t.Policies = nil - // c.UI.Warn("Overwriting policies with new specified policies") } for _, policyName := range policyNames { @@ -288,7 +287,6 @@ func (c *cmd) Run(args []string) int { if hasRoleFields { roleNames = c.roleNames roleIDs = c.roleIDs - // c.UI.Warn("Overwriting policies with new specified policies") t.Roles = nil } diff --git a/command/acl/token/update/token_update_test.go b/command/acl/token/update/token_update_test.go index 541d1e22539..65eb0496ec1 100644 --- a/command/acl/token/update/token_update_test.go +++ b/command/acl/token/update/token_update_test.go @@ -122,6 +122,19 @@ func TestTokenUpdateCommand(t *testing.T) { require.Len(t, token.Policies, 1) }) + // update with add-policy-name + t.Run("add-policy-name", func(t *testing.T) { + token := run(t, []string{ + "-http-addr=" + a.HTTPAddr(), + "-accessor-id=" + token.AccessorID, + "-token=root", + "-add-policy-name=" + policy.Name, + "-description=test token", + }) + + require.Len(t, token.Policies, 1) + }) + // update with policy by id t.Run("policy-id", func(t *testing.T) { token := run(t, []string{ @@ -135,6 +148,19 @@ func TestTokenUpdateCommand(t *testing.T) { require.Len(t, token.Policies, 1) }) + // update with add-policy-id + t.Run("add-policy-id", func(t *testing.T) { + token := run(t, []string{ + "-http-addr=" + a.HTTPAddr(), + "-accessor-id=" + token.AccessorID, + "-token=root", + "-add-policy-id=" + policy.ID, + "-description=test token", + }) + + require.Len(t, token.Policies, 1) + }) + // update with no description shouldn't delete the current description t.Run("merge-description", func(t *testing.T) { token := run(t, []string{ From e7c0a45492ede85c5745351f35df498765c1c54e Mon Sep 17 00:00:00 2001 From: Ronald Ekambi Date: Wed, 22 Feb 2023 14:47:56 -0500 Subject: [PATCH 6/7] rename to append and update tests --- .changelog/16288.txt | 4 +- command/acl/token/update/token_update.go | 56 ++++++------ command/acl/token/update/token_update_test.go | 88 +++++++++++++++---- website/content/commands/acl/token/update.mdx | 16 ++-- 4 files changed, 112 insertions(+), 52 deletions(-) diff --git a/.changelog/16288.txt b/.changelog/16288.txt index dfbdc26de95..3df376b9713 100644 --- a/.changelog/16288.txt +++ b/.changelog/16288.txt @@ -1,8 +1,8 @@ ```release-note:deprecation -cli: Deprecate the `-merge-policies` and `-merge-roles` flags from the `consul token update` command in favour of: `-add-policy-id`, `-add-policy-name`, `-add-role-name`, and `-add-role-id`. +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 `-add-policy-id`, `-add-policy-name`, `-add-role-name`, and `-add-role-id` flags to the `consul token update` command. +cli: added `-append-policy-id`, `-append-policy-name`, `-append-role-name`, and `-append-role-id` flags to the `consul token update` command. These flags will allow updates to 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 8df986faa35..87047390577 100644 --- a/command/acl/token/update/token_update.go +++ b/command/acl/token/update/token_update.go @@ -27,13 +27,13 @@ type cmd struct { tokenAccessorID string policyIDs []string - addPolicyIDs []string + appendPolicyIDs []string policyNames []string - addPolicyNames []string + appendPolicyNames []string roleIDs []string - addRoleIDs []string + appendRoleIDs []string roleNames []string - addRoleNames []string + appendRoleNames []string serviceIdents []string nodeIdents []string description string @@ -61,20 +61,20 @@ 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") - c.flags.Var((*flags.AppendSliceValue)(&c.addPolicyIDs), "add-policy-id", "ID of a "+ + "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") - c.flags.Var((*flags.AppendSliceValue)(&c.addPolicyNames), "add-policy-name", "Name of a "+ + "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") - c.flags.Var((*flags.AppendSliceValue)(&c.addRoleIDs), "add-role-id", "ID of a "+ + "role to use for this token. Overwrites existing rolees. 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.addRoleNames), "add-role-name", "Name of a "+ + 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 "+ @@ -98,9 +98,9 @@ func (c *cmd) init() { // Deprecations c.flags.StringVar(&c.tokenID, "id", "", "DEPRECATED. Use -accessor-id instead.") c.flags.BoolVar(&c.mergePolicies, "merge-policies", false, "DEPRECATED. "+ - "Use -add-policy-id or -add-policy-name instead.") + "Use -append-policy-id or -append-policy-name instead.") c.flags.BoolVar(&c.mergeRoles, "merge-roles", false, "DEPRECATED. "+ - "Use -add-role-id or -add-role-name instead.") + "Use -append-role-id or -append-role-name instead.") } func (c *cmd) Run(args []string) int { @@ -160,8 +160,8 @@ func (c *cmd) Run(args []string) int { } if c.mergePolicies { - c.UI.Warn("merge-policies is deprecated and will be removed in future Consul version. " + - "Use `add-policy-name` and `add-policy-id` instead.") + 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 @@ -200,16 +200,18 @@ func (c *cmd) Run(args []string) int { } } else { - hasAddPolicyFields := len(c.addPolicyNames) > 0 || len(c.addPolicyIDs) > 0 + 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 specify both add-policy-id/add-policy-name and policy-id/policy-name") + 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.addPolicyIDs - policyNames := c.addPolicyNames + policyIDs := c.appendPolicyIDs + policyNames := c.appendPolicyNames if hasPolicyFields { policyIDs = c.policyIDs @@ -234,8 +236,8 @@ func (c *cmd) Run(args []string) int { } if c.mergeRoles { - c.UI.Warn("merge-roles is deprecated and will be removed in future Consul version. " + - "Use `add-role-name` and `add-role-id` instead.") + 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 @@ -273,16 +275,18 @@ func (c *cmd) Run(args []string) int { } } } else { - hasAddRoleFields := len(c.addRoleNames) > 0 || len(c.addRoleIDs) > 0 + hasAddRoleFields := len(c.appendRoleNames) > 0 || len(c.appendRoleIDs) > 0 hasRoleFields := len(c.roleIDs) > 0 || len(c.roleNames) > 0 if hasRoleFields && hasAddRoleFields { - c.UI.Error("Cannot specify both add-role-id/add-role-name and role-id/role-name") + 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.addRoleNames - roleIDs := c.addRoleIDs + roleNames := c.appendRoleNames + roleIDs := c.appendRoleIDs if hasRoleFields { roleNames = c.roleNames diff --git a/command/acl/token/update/token_update_test.go b/command/acl/token/update/token_update_test.go index 65eb0496ec1..a410b61a8dc 100644 --- a/command/acl/token/update/token_update_test.go +++ b/command/acl/token/update/token_update_test.go @@ -122,55 +122,111 @@ func TestTokenUpdateCommand(t *testing.T) { require.Len(t, token.Policies, 1) }) - // update with add-policy-name - t.Run("add-policy-name", func(t *testing.T) { + // update with policy by id + t.Run("policy-id", func(t *testing.T) { token := run(t, []string{ "-http-addr=" + a.HTTPAddr(), "-accessor-id=" + token.AccessorID, "-token=root", - "-add-policy-name=" + policy.Name, + "-policy-id=" + policy.ID, "-description=test token", }) require.Len(t, token.Policies, 1) }) - // update with policy by id - t.Run("policy-id", func(t *testing.T) { + // update with no description shouldn't delete the current description + t.Run("merge-description", func(t *testing.T) { token := run(t, []string{ "-http-addr=" + a.HTTPAddr(), "-accessor-id=" + token.AccessorID, "-token=root", - "-policy-id=" + policy.ID, - "-description=test token", + "-policy-name=" + policy.Name, }) - require.Len(t, token.Policies, 1) + require.Equal(t, "test token", token.Description) }) +} + +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() - // update with add-policy-id - t.Run("add-policy-id", func(t *testing.T) { + 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) + + 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", - "-add-policy-id=" + policy.ID, + "-append-policy-name=" + secondPolicy.Name, "-description=test token", }) - require.Len(t, token.Policies, 1) + require.Len(t, token.Policies, 2) }) - // update with no description shouldn't delete the current description - t.Run("merge-description", func(t *testing.T) { + // 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", - "-policy-name=" + policy.Name, + "-append-policy-id=" + secondPolicy.ID, + "-description=test token", }) - require.Equal(t, "test token", token.Description) + require.Len(t, token.Policies, 2) }) } diff --git a/website/content/commands/acl/token/update.mdx b/website/content/commands/acl/token/update.mdx index 75ed89d9e98..68e5ffe48a1 100644 --- a/website/content/commands/acl/token/update.mdx +++ b/website/content/commands/acl/token/update.mdx @@ -38,12 +38,12 @@ Usage: `consul acl token update [options]` - `-merge-policies` - Deprecated. Merge the new policies with the existing policies. -~> This is deprecated and will be removed in future Consul version. Use `add-policy-id` or `add-policy-name` +~> 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 future Consul version. Use `add-role-id` or `add-role-name` +~> 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. @@ -59,21 +59,21 @@ instead. - `-policy-name=` - Name of a policy to use for this token. Overwrites existing policies. May be specified multiple times. -~> `-policy-id` and `-policy-name` will completely overwrite existing token policies. Use `-add-policy-id` or `-add-policy-name` to retain existing policies. +~> `-policy-id` and `-policy-name` will completely overwrite existing token policies. Use `-append-policy-id` or `-append-policy-name` to retain existing policies. -- `-add-policy-id=` - ID of policy to be added for this token. The token retains existing policies. 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. -- `-add-policy-name=` - Name of a 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 completely overwrite existing policies. Use `-add-role-id` or `-add-role-name` to retain the existing roles. +~> `-role-id` and `-role-name` will completely overwrite existing roles. Use `-append-role-id` or `-append-role-name` to retain the existing roles. -- `-add-role-id=` - ID of a role to add to this token. The token retains existing roles. May be specified multiple times. +- `-append-role-id=` - ID of a role to add to this token. The token retains existing roles. May be specified multiple times. -- `-add-role-name=` - Name 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 From 5a37d37179b104e9e60b11474261b86c900a0406 Mon Sep 17 00:00:00 2001 From: Ronald Ekambi Date: Wed, 1 Mar 2023 09:35:07 -0500 Subject: [PATCH 7/7] Fix potential flakey tests and update ux to remove 'completely' + typo fixes --- .changelog/16288.txt | 2 +- command/acl/token/update/token_update.go | 2 +- command/acl/token/update/token_update_test.go | 11 +++++++++-- website/content/commands/acl/token/update.mdx | 4 ++-- 4 files changed, 13 insertions(+), 6 deletions(-) diff --git a/.changelog/16288.txt b/.changelog/16288.txt index 3df376b9713..5e2820ec27d 100644 --- a/.changelog/16288.txt +++ b/.changelog/16288.txt @@ -4,5 +4,5 @@ cli: Deprecate the `-merge-policies` and `-merge-roles` flags from the `consul t ```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 will allow updates to token's policies/roles without having to override them completely. +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 87047390577..6cb529edd45 100644 --- a/command/acl/token/update/token_update.go +++ b/command/acl/token/update/token_update.go @@ -71,7 +71,7 @@ func (c *cmd) init() { c.flags.Var((*flags.AppendSliceValue)(&c.roleIDs), "role-id", "ID of a "+ "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. Overwrites existing rolees. 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 "+ diff --git a/command/acl/token/update/token_update_test.go b/command/acl/token/update/token_update_test.go index a410b61a8dc..bd11d1d8e3f 100644 --- a/command/acl/token/update/token_update_test.go +++ b/command/acl/token/update/token_update_test.go @@ -190,6 +190,13 @@ func TestTokenUpdateCommandWithAppend(t *testing.T) { ) 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) @@ -222,11 +229,11 @@ func TestTokenUpdateCommandWithAppend(t *testing.T) { "-http-addr=" + a.HTTPAddr(), "-accessor-id=" + token.AccessorID, "-token=root", - "-append-policy-id=" + secondPolicy.ID, + "-append-policy-id=" + thirdPolicy.ID, "-description=test token", }) - require.Len(t, token.Policies, 2) + require.Len(t, token.Policies, 3) }) } diff --git a/website/content/commands/acl/token/update.mdx b/website/content/commands/acl/token/update.mdx index 68e5ffe48a1..19441e1020b 100644 --- a/website/content/commands/acl/token/update.mdx +++ b/website/content/commands/acl/token/update.mdx @@ -59,7 +59,7 @@ instead. - `-policy-name=` - Name of a policy to use for this token. Overwrites existing policies. May be specified multiple times. -~> `-policy-id` and `-policy-name` will completely overwrite existing token policies. Use `-append-policy-id` or `-append-policy-name` to retain existing policies. +~> `-policy-id` and `-policy-name` will overwrite existing token policies. Use `-append-policy-id` or `-append-policy-name` to retain existing policies. - `-append-policy-id=` - ID of policy to be added for this token. The token retains existing policies. May be specified multiple times. @@ -69,7 +69,7 @@ instead. - `-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 completely overwrite existing roles. Use `-append-role-id` or `-append-role-name` to retain the existing roles. +~> `-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.