Give better error when policy was created manually

In cases where a user creates a policy out-of-band with the same name as
one consul-k8s tries to update, give a better error.

This is definitely an edge case but has occurred when a user created the
cross-namespace-acl-policy manually when trying to work around another
issue. This will at least give a better error message in this case.

subcommand/server-acl-init/command_test.go

@@ -1097,6 +1097,69 @@ 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, fileCleanup := writeTempFile(t, bootToken)
+	defer fileCleanup()
+	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) {

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 deleted so consul-k8s can recreate it and manage it itself",
+					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.WaitForServiceIntentions(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 deleted so consul-k8s can recreate it and manage it itself",
+	)
+
+	// Check that the policy wasn't modified.
+	rereadPolicy, _, err := consul.ACL().PolicyRead(policy.ID, nil)
+	require.NoError(err)
+	require.Equal(policyDescription, rereadPolicy.Description)
+}