Give better error when policy was created manually

CHANGELOG.md

@@ -4,6 +4,9 @@ BUG FIXES:
 * CRDs: Fix issue where a `ServiceIntentions` resource could be continually resynced with Consul
   because Consul's internal representation had a different order for an array than the Kubernetes resource. [[GH-416](https://github.com/hashicorp/consul-k8s/pull/416)] 
 
+IMPROVEMENTS:
+* ACLs: give better error if policy that consul-k8s tries to update was created manually by user. [[GH-412](https://github.com/hashicorp/consul-k8s/pull/412)]
+
 ## 0.22.0 (December 21, 2020)
 
 BUG FIXES:

subcommand/server-acl-init/command_test.go

@@ -412,8 +412,7 @@ func TestRun_TokensReplicatedDC(t *testing.T) {
 	for _, c := range cases {
 		t.Run(c.TestName, func(t *testing.T) {
 			bootToken := "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"
-			tokenFile, fileCleanup := writeTempFile(t, bootToken)
-			defer fileCleanup()
+			tokenFile := writeTempFile(t, bootToken)
 
 			k8s, consul, secondaryAddr, cleanup := mockReplicatedSetup(t, bootToken)
 			setUpK8sServiceAccount(t, k8s, ns)
@@ -548,8 +547,7 @@ func TestRun_TokensWithProvidedBootstrapToken(t *testing.T) {
 	for _, c := range cases {
 		t.Run(c.TestName, func(t *testing.T) {
 			bootToken := "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"
-			tokenFile, fileCleanup := writeTempFile(t, bootToken)
-			defer fileCleanup()
+			tokenFile := writeTempFile(t, bootToken)
 
 			k8s, testAgent := completeBootstrappedSetup(t, bootToken)
 			setUpK8sServiceAccount(t, k8s, ns)
@@ -1097,6 +1095,68 @@ func TestRun_SyncPolicyUpdates(t *testing.T) {
 	}
 }
 
+// Test that we give an error if an ACL policy we were going to create
+// already exists but it has a different description than what consul-k8s
+// expected. In this case, it's likely that a user manually created an ACL
+// policy with the same name and so we want to error.
+// This test will test with the catalog sync policy but any policy
+// that we try to update will work for testing.
+func TestRun_ErrorsOnDuplicateACLPolicy(t *testing.T) {
+	t.Parallel()
+	require := require.New(t)
+
+	// Create Consul with ACLs already bootstrapped so that we can
+	// then seed it with our manually created policy.
+	bootToken := "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"
+	tokenFile := writeTempFile(t, bootToken)
+	k8s, testAgent := completeBootstrappedSetup(t, bootToken)
+	setUpK8sServiceAccount(t, k8s, ns)
+	defer testAgent.Stop()
+
+	consul, err := api.NewClient(&api.Config{
+		Address: testAgent.HTTPAddr,
+		Token:   bootToken,
+	})
+	require.NoError(err)
+
+	// Create the policy manually.
+	description := "not the expected description"
+	policy, _, err := consul.ACL().PolicyCreate(&api.ACLPolicy{
+		Name:        "catalog-sync-token",
+		Description: description,
+	}, nil)
+	require.NoError(err)
+
+	// Run the command.
+	ui := cli.NewMockUi()
+	cmd := Command{
+		UI:        ui,
+		clientset: k8s,
+	}
+	cmdArgs := []string{
+		"-timeout=1s",
+		"-k8s-namespace", ns,
+		"-bootstrap-token-file", tokenFile,
+		"-resource-prefix=" + resourcePrefix,
+		"-k8s-namespace=" + ns,
+		"-server-address", strings.Split(testAgent.HTTPAddr, ":")[0],
+		"-server-port", strings.Split(testAgent.HTTPAddr, ":")[1],
+		"-create-sync-token",
+	}
+	responseCode := cmd.Run(cmdArgs)
+
+	// We expect the command to time out.
+	require.Equal(1, responseCode)
+	// NOTE: Since the error is logged through the logger instead of the UI
+	// there's no good way to test that we logged the expected error however
+	// we also test this directly in create_or_update_test.go.
+
+	// Check that the policy wasn't modified.
+	rereadPolicy, _, err := consul.ACL().PolicyRead(policy.ID, nil)
+	require.NoError(err)
+	require.Equal(description, rereadPolicy.Description)
+}
+
 // Test that if the servers aren't available at first that bootstrap
 // still succeeds.
 func TestRun_DelayedServers(t *testing.T) {
@@ -1497,8 +1557,7 @@ func TestRun_SkipBootstrapping_WhenBootstrapTokenIsProvided(t *testing.T) {
 	k8s := fake.NewSimpleClientset()
 
 	bootToken := "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"
-	tokenFile, fileCleanup := writeTempFile(t, bootToken)
-	defer fileCleanup()
+	tokenFile := writeTempFile(t, bootToken)
 
 	type APICall struct {
 		Method string
@@ -1633,8 +1692,7 @@ func TestRun_ACLReplicationTokenValid(t *testing.T) {
 
 	// completeReplicatedSetup ran the command in our primary dc so now we
 	// need to run the command in our secondary dc.
-	tokenFile, cleanup := writeTempFile(t, aclReplicationToken)
-	defer cleanup()
+	tokenFile := writeTempFile(t, aclReplicationToken)
 	secondaryUI := cli.NewMockUi()
 	secondaryCmd := Command{
 		UI:        secondaryUI,
@@ -1683,8 +1741,7 @@ func TestRun_AnonPolicy_IgnoredWithReplication(t *testing.T) {
 	for _, flag := range cases {
 		t.Run(flag, func(t *testing.T) {
 			bootToken := "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"
-			tokenFile, fileCleanup := writeTempFile(t, bootToken)
-			defer fileCleanup()
+			tokenFile := writeTempFile(t, bootToken)
 			k8s, consul, serverAddr, cleanup := mockReplicatedSetup(t, bootToken)
 			setUpK8sServiceAccount(t, k8s, ns)
 			defer cleanup()
@@ -2128,15 +2185,20 @@ func policyExists(t require.TestingT, name string, client *api.Client) *api.ACLP
 	return policy
 }
 
-func writeTempFile(t *testing.T, contents string) (string, func()) {
+// writeTempFile writes contents to a temporary file and returns the file
+// name. It will remove the file once the test completes.
+func writeTempFile(t *testing.T, contents string) string {
 	t.Helper()
 	file, err := ioutil.TempFile("", "")
 	require.NoError(t, err)
 	_, err = file.WriteString(contents)
 	require.NoError(t, err)
-	return file.Name(), func() {
+
+	t.Cleanup(func() {
 		os.Remove(file.Name())
-	}
+	})
+
+	return file.Name()
 }
 
 var serviceAccountCACert = "LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSURDekNDQWZPZ0F3SUJBZ0lRS3pzN05qbDlIczZYYzhFWG91MjVoekFOQmdrcWhraUc5dzBCQVFzRkFEQXYKTVMwd0t3WURWUVFERXlRMU9XVTJaR00wTVMweU1EaG1MVFF3T1RVdFlUSTRPUzB4Wm1NM01EQmhZekZqWXpndwpIaGNOTVRrd05qQTNNVEF4TnpNeFdoY05NalF3TmpBMU1URXhOek14V2pBdk1TMHdLd1lEVlFRREV5UTFPV1UyClpHTTBNUzB5TURobUxUUXdPVFV0WVRJNE9TMHhabU0zTURCaFl6RmpZemd3Z2dFaU1BMEdDU3FHU0liM0RRRUIKQVFVQUE0SUJEd0F3Z2dFS0FvSUJBUURaakh6d3FvZnpUcEdwYzBNZElDUzdldXZmdWpVS0UzUEMvYXBmREFnQgo0anpFRktBNzgvOStLVUd3L2MvMFNIZVNRaE4rYThnd2xIUm5BejFOSmNmT0lYeTRkd2VVdU9rQWlGeEg4cGh0CkVDd2tlTk83ejhEb1Y4Y2VtaW5DUkhHamFSbW9NeHBaN2cycFpBSk5aZVB4aTN5MWFOa0ZBWGU5Z1NVU2RqUloKUlhZa2E3d2gyQU85azJkbEdGQVlCK3Qzdld3SjZ0d2pHMFR0S1FyaFlNOU9kMS9vTjBFMDFMekJjWnV4a04xawo4Z2ZJSHk3Yk9GQ0JNMldURURXLzBhQXZjQVByTzhETHFESis2TWpjM3I3K3psemw4YVFzcGIwUzA4cFZ6a2k1CkR6Ly84M2t5dTBwaEp1aWo1ZUI4OFY3VWZQWHhYRi9FdFY2ZnZyTDdNTjRmQWdNQkFBR2pJekFoTUE0R0ExVWQKRHdFQi93UUVBd0lDQkRBUEJnTlZIUk1CQWY4RUJUQURBUUgvTUEwR0NTcUdTSWIzRFFFQkN3VUFBNElCQVFCdgpRc2FHNnFsY2FSa3RKMHpHaHh4SjUyTm5SVjJHY0lZUGVOM1p2MlZYZTNNTDNWZDZHMzJQVjdsSU9oangzS21BCi91TWg2TmhxQnpzZWtrVHowUHVDM3dKeU0yT0dvblZRaXNGbHF4OXNGUTNmVTJtSUdYQ2Ezd0M4ZS9xUDhCSFMKdzcvVmVBN2x6bWozVFFSRS9XMFUwWkdlb0F4bjliNkp0VDBpTXVjWXZQMGhYS1RQQldsbnpJaWphbVU1MHIyWQo3aWEwNjVVZzJ4VU41RkxYL3Z4T0EzeTRyanBraldvVlFjdTFwOFRaclZvTTNkc0dGV3AxMGZETVJpQUhUdk9ICloyM2pHdWs2cm45RFVIQzJ4UGozd0NUbWQ4U0dFSm9WMzFub0pWNWRWZVE5MHd1c1h6M3ZURzdmaWNLbnZIRlMKeHRyNVBTd0gxRHVzWWZWYUdIMk8KLS0tLS1FTkQgQ0VSVElGSUNBVEUtLS0tLQo="

subcommand/server-acl-init/create_or_update.go

@@ -2,7 +2,6 @@ package serveraclinit
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"strings"
 
@@ -130,8 +129,13 @@ func (c *Command) createOrUpdateACLPolicy(policy api.ACLPolicy, consulClient *ap
 
 			// 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 errors.New("Unable to find existing ACL policy")
+				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

subcommand/server-acl-init/create_or_update_test.go

@@ -0,0 +1,68 @@
+package serveraclinit
+
+import (
+	"testing"
+
+	"github.com/hashicorp/consul/api"
+	"github.com/hashicorp/consul/sdk/testutil"
+	"github.com/hashicorp/go-hclog"
+	"github.com/mitchellh/cli"
+	"github.com/stretchr/testify/require"
+	"k8s.io/client-go/kubernetes/fake"
+)
+
+// Test that we give an error if an ACL policy we were going to create
+// already exists but it has a different description than what consul-k8s
+// expected. In this case, it's likely that a user manually created an ACL
+// policy with the same name and so we want to error.
+func TestCreateOrUpdateACLPolicy_ErrorsIfDescriptionDoesNotMatch(t *testing.T) {
+	require := require.New(t)
+	ui := cli.NewMockUi()
+	k8s := fake.NewSimpleClientset()
+	cmd := Command{
+		UI:                  ui,
+		clientset:           k8s,
+		log:                 hclog.NewNullLogger(),
+		flagCreateSyncToken: true,
+	}
+
+	// Start Consul.
+	bootToken := "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"
+	svr, err := testutil.NewTestServerConfigT(t, func(c *testutil.TestServerConfig) {
+		c.ACL.Enabled = true
+		c.ACL.Tokens.Master = bootToken
+	})
+	require.NoError(err)
+	svr.WaitForLeader(t)
+
+	// Get a Consul client.
+	consul, err := api.NewClient(&api.Config{
+		Address: svr.HTTPAddr,
+		Token:   bootToken,
+	})
+	require.NoError(err)
+
+	// Create the policy manually.
+	policyDescription := "not the expected description"
+	policyName := "policy-name"
+	policy, _, err := consul.ACL().PolicyCreate(&api.ACLPolicy{
+		Name:        policyName,
+		Description: policyDescription,
+	}, nil)
+	require.NoError(err)
+
+	// Now run the function.
+	err = cmd.createOrUpdateACLPolicy(api.ACLPolicy{
+		Name:        policyName,
+		Description: "expected description",
+	}, consul)
+	require.EqualError(err,
+		"policy found with name \"policy-name\" but not with expected description \"expected description\";"+
+			" if this policy was created manually it must be renamed to something else because this name is reserved by consul-k8s",
+	)
+
+	// Check that the policy wasn't modified.
+	rereadPolicy, _, err := consul.ACL().PolicyRead(policy.ID, nil)
+	require.NoError(err)
+	require.Equal(policyDescription, rereadPolicy.Description)
+}