luxolus/open-nomad
2
0
Fork 0
Code Issues 1 Pull requests Packages Releases Wiki Activity

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

Browse source 
acl: three small fixes for CLI and state consistency
This commit is contained in:
James Rasell 2022-08-25 12:05:32 +02:00 committed by GitHub
parent 9782d6d7ff 7a0798663d
commit 2293dcf35f
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
12 changed files with 106 additions and 37 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
api/acl_test.go
View file

@ -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}},

2
command/acl_bootstrap.go
View file

@ -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()
}

4
command/acl_bootstrap_test.go
View file

@ -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

8
command/acl_role_create.go
View file

@ -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
}

4
command/acl_role_create_test.go
View file

@ -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()

8
command/acl_role_update.go
View file

@ -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
}

4
command/acl_role_update_test.go
View file

@ -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))

4
command/acl_token_create_test.go
View file

@ -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) {

35
nomad/acl_endpoint.go
View file

@ -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()
}

16
nomad/acl_endpoint_test.go
View file

@ -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) {

44
nomad/state/state_store_acl.go
View file

@ -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

10
nomad/state/state_store_acl_test.go
View file

@ -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) {