Skip to content

Commit

Permalink
always update acl policy if it exists (#2392)
Browse files Browse the repository at this point in the history
* always update acl policy if it exists

* added changelog

* added unit test

* fix typo

* added some additional assertions to test

* refactored create_or_update unit test
  • Loading branch information
aahel authored Jun 27, 2023
1 parent c83ce0c commit 95af4c7
Show file tree
Hide file tree
Showing 3 changed files with 97 additions and 31 deletions.
3 changes: 3 additions & 0 deletions .changelog/2392.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
control-plane: Always update ACL policies upon upgrade
```
57 changes: 26 additions & 31 deletions control-plane/subcommand/server-acl-init/create_or_update.go
Original file line number Diff line number Diff line change
Expand Up @@ -315,42 +315,37 @@ func (c *Command) createOrUpdateACLPolicy(policy api.ACLPolicy, consulClient *ap
// Allowing the Consul node name to be configurable also requires any sync
// policy to be updated in case the node name has changed.
if isPolicyExistsErr(err, policy.Name) {
if c.flagEnableNamespaces || c.flagSyncCatalog {
c.log.Info(fmt.Sprintf("Policy %q already exists, updating", policy.Name))
c.log.Info(fmt.Sprintf("Policy %q already exists, updating", policy.Name))

// The policy ID is required in any PolicyUpdate call, so first we need to
// get the existing policy to extract its ID.
existingPolicies, _, err := consulClient.ACL().PolicyList(&api.QueryOptions{})
if err != nil {
return err
}

// Find the policy that matches our name and description
// and that's the ID we need
for _, existingPolicy := range existingPolicies {
if existingPolicy.Name == policy.Name && existingPolicy.Description == policy.Description {
policy.ID = existingPolicy.ID
}
}
// The policy ID is required in any PolicyUpdate call, so first we need to
// get the existing policy to extract its ID.
existingPolicies, _, err := consulClient.ACL().PolicyList(&api.QueryOptions{})
if err != nil {
return err
}

// This shouldn't happen, because we're looking for a policy
// only after we've hit a `Policy already exists` error.
// The only time it might happen is if a user has manually created a policy
// with this name but used a different description. In this case,
// we don't want to overwrite the policy so we just error.
if policy.ID == "" {
return fmt.Errorf("policy found with name %q but not with expected description %q; "+
"if this policy was created manually it must be renamed to something else because this name is reserved by consul-k8s",
policy.Name, policy.Description)
// Find the policy that matches our name and description
// and that's the ID we need
for _, existingPolicy := range existingPolicies {
if existingPolicy.Name == policy.Name && existingPolicy.Description == policy.Description {
policy.ID = existingPolicy.ID
}
}

// Update the policy now that we've found its ID
_, _, err = consulClient.ACL().PolicyUpdate(&policy, &api.WriteOptions{})
return err
} else {
c.log.Info(fmt.Sprintf("Policy %q already exists, skipping update", policy.Name))
return nil
// This shouldn't happen, because we're looking for a policy
// only after we've hit a `Policy already exists` error.
// The only time it might happen is if a user has manually created a policy
// with this name but used a different description. In this case,
// we don't want to overwrite the policy so we just error.
if policy.ID == "" {
return fmt.Errorf("policy found with name %q but not with expected description %q; "+
"if this policy was created manually it must be renamed to something else because this name is reserved by consul-k8s",
policy.Name, policy.Description)
}

// Update the policy now that we've found its ID
_, _, err = consulClient.ACL().PolicyUpdate(&policy, &api.WriteOptions{})
return err
}
return err
}
Expand Down
68 changes: 68 additions & 0 deletions control-plane/subcommand/server-acl-init/create_or_update_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,3 +70,71 @@ func TestCreateOrUpdateACLPolicy_ErrorsIfDescriptionDoesNotMatch(t *testing.T) {
require.NoError(err)
require.Equal(policyDescription, rereadPolicy.Description)
}

func TestCreateOrUpdateACLPolicy(t *testing.T) {
require := require.New(t)
ui := cli.NewMockUi()
k8s := fake.NewSimpleClientset()
cmd := Command{
UI: ui,
clientset: k8s,
log: hclog.NewNullLogger(),
}
cmd.init()
// Start Consul.
bootToken := "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"
svr, err := testutil.NewTestServerConfigT(t, func(c *testutil.TestServerConfig) {
c.ACL.Enabled = true
c.ACL.Tokens.InitialManagement = bootToken
})
require.NoError(err)
defer svr.Stop()
svr.WaitForLeader(t)

// Get a Consul client.
consul, err := api.NewClient(&api.Config{
Address: svr.HTTPAddr,
Token: bootToken,
})
require.NoError(err)
connectInjectRule, err := cmd.injectRules()
require.NoError(err)
aclReplRule, err := cmd.aclReplicationRules()
require.NoError(err)
policyDescription := "policy-description"
policyName := "policy-name"
cases := []struct {
Name string
PolicyDescription string
PolicyName string
Rules string
}{
{
Name: "create",
PolicyDescription: policyDescription,
PolicyName: policyName,
Rules: connectInjectRule,
},
{
Name: "update",
PolicyDescription: policyDescription,
PolicyName: policyName,
Rules: aclReplRule,
},
}
for _, tt := range cases {
t.Run(tt.Name, func(t *testing.T) {
err = cmd.createOrUpdateACLPolicy(api.ACLPolicy{
Name: tt.PolicyName,
Description: tt.PolicyDescription,
Rules: tt.Rules,
}, consul)
require.Nil(err)
policy, _, err := consul.ACL().PolicyReadByName(tt.PolicyName, nil)
require.Nil(err)
require.Equal(tt.Rules, policy.Rules)
require.Equal(tt.PolicyName, policy.Name)
require.Equal(tt.PolicyDescription, policy.Description)
})
}
}

0 comments on commit 95af4c7

Please sign in to comment.