always update acl policy if it exists (#2392)

* 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

.changelog/2392.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+control-plane: Always update ACL policies upon upgrade
+```

control-plane/subcommand/server-acl-init/create_or_update.go

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

control-plane/subcommand/server-acl-init/create_or_update_test.go

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