Merge pull request #14291 from hashicorp/f-gh-13120-sso-various-small…

…-fixes

acl: three small fixes for CLI and state consistency

api/acl_test.go

@@ -330,7 +330,7 @@ func TestACLTokens_Info(t *testing.T) {
 				// Create an ACL role referencing the previously created
 				// policy.
 				role := ACLRole{
-					Name:     "acl-role-api-test",
+					Name:     "acl-role-api-test-role-and-policy",
 					Policies: []*ACLRolePolicyLink{{Name: aclPolicy1.Name}},
 				}
 				aclRoleCreateResp, writeMeta, err := testClient.ACLRoles().Create(&role, nil)
@@ -341,7 +341,7 @@ func TestACLTokens_Info(t *testing.T) {
 
 				// Create a token with a role linking.
 				token := &ACLToken{
-					Name:     "token-with-role-link",
+					Name:     "token-with-role-and-policy-link",
 					Type:     "client",
 					Policies: []string{aclPolicy2.Name},
 					Roles:    []*ACLTokenRoleLink{{Name: role.Name}},

command/acl_bootstrap.go

@@ -198,7 +198,7 @@ func outputACLToken(ui cli.Ui, token *api.ACLToken) {
 
 func expiryTimeString(t *time.Time) string {
 	if t == nil || t.IsZero() {
-		return "<never>"
+		return "<none>"
 	}
 	return t.String()
 }

command/acl_bootstrap_test.go

@@ -36,7 +36,7 @@ func TestACLBootstrapCommand(t *testing.T) {
 
 	out := ui.OutputWriter.String()
 	assert.Contains(out, "Secret ID")
-	require.Contains(t, out, "Expiry Time  = <never>")
+	require.Contains(t, out, "Expiry Time  = <none>")
 }
 
 // If a bootstrap token has already been created, attempts to create more should
@@ -117,7 +117,7 @@ func TestACLBootstrapCommand_WithOperatorFileBootstrapToken(t *testing.T) {
 
 	out := ui.OutputWriter.String()
 	assert.Contains(t, out, mockToken.SecretID)
-	require.Contains(t, out, "Expiry Time  = <never>")
+	require.Contains(t, out, "Expiry Time  = <none>")
 }
 
 // Attempting to bootstrap the server with an invalid operator provided token in a file should

command/acl_role_create.go

@@ -44,7 +44,7 @@ ACL Create Options:
     A free form text description of the role that must not exceed 256
     characters.
 
-  -policy-name
+  -policy
     Specifies a policy to associate with the role identified by their name. This
     flag can be specified multiple times and must be specified at least once.
 
@@ -62,7 +62,7 @@ func (a *ACLRoleCreateCommand) AutocompleteFlags() complete.Flags {
 		complete.Flags{
 			"-name":        complete.PredictAnything,
 			"-description": complete.PredictAnything,
-			"-policy-name": complete.PredictAnything,
+			"-policy":      complete.PredictAnything,
 			"-json":        complete.PredictNothing,
 			"-t":           complete.PredictAnything,
 		})
@@ -86,7 +86,7 @@ func (a *ACLRoleCreateCommand) Run(args []string) int {
 	flags.Var((funcVar)(func(s string) error {
 		a.policyNames = append(a.policyNames, s)
 		return nil
-	}), "policy-name", "")
+	}), "policy", "")
 	flags.BoolVar(&a.json, "json", false, "")
 	flags.StringVar(&a.tmpl, "t", "", "")
 	if err := flags.Parse(args); err != nil {
@@ -107,7 +107,7 @@ func (a *ACLRoleCreateCommand) Run(args []string) int {
 		return 1
 	}
 	if len(a.policyNames) < 1 {
-		a.Ui.Error("At least one policy name must be specified using the -policy-name flag")
+		a.Ui.Error("At least one policy name must be specified using the -policy flag")
 		return 1
 	}
 

command/acl_role_create_test.go

@@ -47,7 +47,7 @@ func TestACLRoleCreateCommand_Run(t *testing.T) {
 	ui.ErrorWriter.Reset()
 
 	require.Equal(t, 1, cmd.Run([]string{"-address=" + url, `-name="foobar"`}))
-	require.Contains(t, ui.ErrorWriter.String(), "At least one policy name must be specified using the -policy-name flag")
+	require.Contains(t, ui.ErrorWriter.String(), "At least one policy name must be specified using the -policy flag")
 
 	ui.OutputWriter.Reset()
 	ui.ErrorWriter.Reset()
@@ -67,7 +67,7 @@ func TestACLRoleCreateCommand_Run(t *testing.T) {
 	// Create an ACL role.
 	args := []string{
 		"-address=" + url, "-token=" + rootACLToken.SecretID, "-name=acl-role-cli-test",
-		"-policy-name=acl-role-cli-test-policy", "-description=acl-role-all-the-things",
+		"-policy=acl-role-cli-test-policy", "-description=acl-role-all-the-things",
 	}
 	require.Equal(t, 0, cmd.Run(args))
 	s := ui.OutputWriter.String()

command/acl_role_update.go

@@ -45,7 +45,7 @@ Update Options:
     A free form text description of the role that must not exceed 256
     characters.
 
-  -policy-name
+  -policy
     Specifies a policy to associate with the role identified by their name. This
     flag can be specified multiple times.
 
@@ -70,7 +70,7 @@ func (a *ACLRoleUpdateCommand) AutocompleteFlags() complete.Flags {
 			"-name":        complete.PredictAnything,
 			"-description": complete.PredictAnything,
 			"-no-merge":    complete.PredictNothing,
-			"-policy-name": complete.PredictAnything,
+			"-policy":      complete.PredictAnything,
 			"-json":        complete.PredictNothing,
 			"-t":           complete.PredictAnything,
 		})
@@ -94,7 +94,7 @@ func (a *ACLRoleUpdateCommand) Run(args []string) int {
 	flags.Var((funcVar)(func(s string) error {
 		a.policyNames = append(a.policyNames, s)
 		return nil
-	}), "policy-name", "")
+	}), "policy", "")
 	flags.BoolVar(&a.noMerge, "no-merge", false, "")
 	flags.BoolVar(&a.json, "json", false, "")
 	flags.StringVar(&a.tmpl, "t", "", "")
@@ -140,7 +140,7 @@ func (a *ACLRoleUpdateCommand) Run(args []string) int {
 			return 1
 		}
 		if len(a.policyNames) < 1 {
-			a.Ui.Error("At least one policy name must be specified using the -policy-name flag")
+			a.Ui.Error("At least one policy name must be specified using the -policy flag")
 			return 1
 		}
 

command/acl_role_update_test.go

@@ -106,12 +106,12 @@ func TestACLRoleUpdateCommand_Run(t *testing.T) {
 	code = cmd.Run([]string{
 		"-address=" + url, "-token=" + rootACLToken.SecretID, "-no-merge", "-name=update-role-name", aclRole.ID})
 	require.Equal(t, 1, code)
-	require.Contains(t, ui.ErrorWriter.String(), "At least one policy name must be specified using the -policy-name flag")
+	require.Contains(t, ui.ErrorWriter.String(), "At least one policy name must be specified using the -policy flag")
 
 	// Update the role using no-merge with all required flags set.
 	code = cmd.Run([]string{
 		"-address=" + url, "-token=" + rootACLToken.SecretID, "-no-merge", "-name=update-role-name",
-		"-description=updated-description", "-policy-name=acl-role-cli-test-policy", aclRole.ID})
+		"-description=updated-description", "-policy=acl-role-cli-test-policy", aclRole.ID})
 	require.Equal(t, 0, code)
 	s = ui.OutputWriter.String()
 	require.Contains(t, s, fmt.Sprintf("ID           = %s", aclRole.ID))

command/acl_token_create_test.go

@@ -39,7 +39,7 @@ func TestACLTokenCreateCommand(t *testing.T) {
 	// Check the output
 	out := ui.OutputWriter.String()
 	require.Contains(t, out, "[foo]")
-	require.Contains(t, out, "Expiry Time  = <never>")
+	require.Contains(t, out, "Expiry Time  = <none>")
 
 	ui.OutputWriter.Reset()
 	ui.ErrorWriter.Reset()
@@ -49,7 +49,7 @@ func TestACLTokenCreateCommand(t *testing.T) {
 	require.Equal(t, 0, code)
 
 	out = ui.OutputWriter.String()
-	require.NotContains(t, out, "Expiry Time  = <never>")
+	require.NotContains(t, out, "Expiry Time  = <none>")
 }
 
 func Test_generateACLTokenRoleLinks(t *testing.T) {

nomad/acl_endpoint.go

@@ -1122,6 +1122,28 @@ func (a *ACL) UpsertRoles(
 			return structs.NewErrRPCCodedf(http.StatusBadRequest, "role %d invalid: %v", idx, err)
 		}
 
+		// If the caller has passed a role ID, this call is considered an
+		// update to an existing role. We should therefore ensure it is found
+		// within state. Otherwise, the call is considered a new creation, and
+		// we must ensure a role of the same name does not exist.
+		if role.ID == "" {
+			existingRole, err := stateSnapshot.GetACLRoleByName(nil, role.Name)
+			if err != nil {
+				return structs.NewErrRPCCodedf(http.StatusInternalServerError, "role lookup failed: %v", err)
+			}
+			if existingRole != nil {
+				return structs.NewErrRPCCodedf(http.StatusBadRequest, "role with name %s already exists", role.Name)
+			}
+		} else {
+			existingRole, err := stateSnapshot.GetACLRoleByID(nil, role.ID)
+			if err != nil {
+				return structs.NewErrRPCCodedf(http.StatusInternalServerError, "role lookup failed: %v", err)
+			}
+			if existingRole == nil {
+				return structs.NewErrRPCCodedf(http.StatusBadRequest, "cannot find role %s", role.ID)
+			}
+		}
+
 		policyNames := make(map[string]struct{})
 		var policiesLinks []*structs.ACLRolePolicyLink
 
@@ -1156,19 +1178,6 @@ func (a *ACL) UpsertRoles(
 		// Stored the potentially updated policy links within our role.
 		role.Policies = policiesLinks
 
-		// If the caller has passed a role ID, this call is considered an
-		// update to an existing role. We should therefore ensure it is found
-		// within state.
-		if role.ID != "" {
-			out, err := stateSnapshot.GetACLRoleByID(nil, role.ID)
-			if err != nil {
-				return structs.NewErrRPCCodedf(http.StatusBadRequest, "role lookup failed: %v", err)
-			}
-			if out == nil {
-				return structs.NewErrRPCCodedf(http.StatusBadRequest, "cannot find role %s", role.ID)
-			}
-		}
-
 		role.Canonicalize()
 		role.SetHash()
 	}

nomad/acl_endpoint_test.go

@@ -1998,6 +1998,22 @@ func TestACL_UpsertRoles(t *testing.T) {
 	err = msgpackrpc.CallWithCodec(codec, structs.ACLUpsertRolesRPCMethod, aclRoleReq5, &aclRoleResp5)
 	require.Error(t, err)
 	require.NotContains(t, err, "Permission denied")
+
+	// Try and create a role with a name that already exists within state.
+	aclRole3 := mock.ACLRole()
+	aclRole3.ID = ""
+	aclRole3.Name = aclRole1.Name
+
+	aclRoleReq6 := &structs.ACLRolesUpsertRequest{
+		ACLRoles: []*structs.ACLRole{aclRole3},
+		WriteRequest: structs.WriteRequest{
+			Region:    DefaultRegion,
+			AuthToken: aclRootToken.SecretID,
+		},
+	}
+	var aclRoleResp6 structs.ACLRolesUpsertResponse
+	err = msgpackrpc.CallWithCodec(codec, structs.ACLUpsertRolesRPCMethod, aclRoleReq6, &aclRoleResp6)
+	require.ErrorContains(t, err, fmt.Sprintf("role with name %s already exists", aclRole1.Name))
 }
 
 func TestACL_DeleteRolesByID(t *testing.T) {

nomad/state/state_store_acl.go

@@ -98,18 +98,52 @@ func (s *StateStore) upsertACLRoleTxn(
 		}
 	}
 
-	existing, err := txn.First(TableACLRoles, indexID, role.ID)
+	// This validation also happens within the RPC handler, but Raft latency
+	// could mean that by the time the state call is invoked, another Raft
+	// update has already written a role with the same name. We therefore need
+	// to check we are not trying to create a role with an existing name.
+	existingRaw, err := txn.First(TableACLRoles, indexName, role.Name)
 	if err != nil {
 		return false, fmt.Errorf("ACL role lookup failed: %v", err)
 	}
 
-	// Set up the indexes correctly to ensure existing indexes are maintained.
+	// Track our type asserted role, so we only need to do this once.
+	var existing *structs.ACLRole
+
+	// If we did not find an ACL Role within state with the same name, we need
+	// to check using the ID index as the operator might be performing an
+	// update on the role name.
+	//
+	// If we found an entry using the name index, we need to check that the ID
+	// matches the object within the request.
+	if existingRaw == nil {
+		existingRaw, err = txn.First(TableACLRoles, indexID, role.ID)
+		if err != nil {
+			return false, fmt.Errorf("ACL role lookup failed: %v", err)
+		}
+		if existingRaw != nil {
+			existing = existingRaw.(*structs.ACLRole)
+		}
+	} else {
+		existing = existingRaw.(*structs.ACLRole)
+		if existing.ID != role.ID {
+			return false, fmt.Errorf("ACL role with name %s already exists", role.Name)
+		}
+	}
+
+	// Depending on whether this is an initial create, or an update, we need to
+	// check and set certain parameters. The most important is to ensure any
+	// create index is carried over.
 	if existing != nil {
-		exist := existing.(*structs.ACLRole)
-		if exist.Equals(role) {
+
+		// If the role already exists, check whether the update contains any
+		// difference. If it doesn't, we can avoid a state update as wel as
+		// updates to any blocking queries.
+		if existing.Equals(role) {
 			return false, nil
 		}
-		role.CreateIndex = exist.CreateIndex
+
+		role.CreateIndex = existing.CreateIndex
 		role.ModifyIndex = index
 	} else {
 		role.CreateIndex = index

nomad/state/state_store_acl_test.go

@@ -1,6 +1,7 @@
 package state
 
 import (
+	"fmt"
 	"testing"
 	"time"
 
@@ -246,6 +247,15 @@ func TestStateStore_UpsertACLRoles(t *testing.T) {
 	replicatedACLRoleResp, err := testState.GetACLRoleByName(ws, replicatedACLRole.Name)
 	require.NoError(t, err)
 	must.Eq(t, replicatedACLRole.Hash, replicatedACLRoleResp.Hash)
+
+	// Try adding a new ACL role, which has a name clash with an existing
+	// entry.
+	dupRoleName := mock.ACLRole()
+	dupRoleName.Name = mockedACLRoles[0].Name
+
+	err = testState.UpsertACLRoles(structs.MsgTypeTestSetup, 50,
+		[]*structs.ACLRole{dupRoleName}, false)
+	require.ErrorContains(t, err, fmt.Sprintf("ACL role with name %s already exists", dupRoleName.Name))
 }
 
 func TestStateStore_ValidateACLRolePolicyLinks(t *testing.T) {