Always update auth method

.circleci/config.yml

@@ -6,8 +6,8 @@ executors:
       - image: circleci/golang:1.13
     environment:
       - TEST_RESULTS: /tmp/test-results # path to where test results are saved
-      - CONSUL_VERSION: 1.7.0 # Consul's OSS version to use in tests
-      - CONSUL_ENT_VERSION: 1.7.0+ent # Consul's enterprise version to use in tests
+      - CONSUL_VERSION: 1.8.0 # Consul's OSS version to use in tests
+      - CONSUL_ENT_VERSION: 1.8.0+ent # Consul's enterprise version to use in tests
 
 jobs:
   go-fmt-and-vet:

subcommand/server-acl-init/command.go

@@ -136,7 +136,7 @@ func (c *Command) init() {
 			"default namespace, specify the value in the form <GatewayName>.<ConsulNamespace>.")
 
 	c.flags.Var((*flags.AppendSliceValue)(&c.flagServerAddresses), "server-address",
-		"The IP, DNS name or the cloud auto-join string of the Consul server(s). If providing IPs or DNS names, may be specified multiple times."+
+		"The IP, DNS name or the cloud auto-join string of the Consul server(s). If providing IPs or DNS names, may be specified multiple times. "+
 			"At least one value is required.")
 	c.flags.UintVar(&c.flagServerPort, "server-port", 8500, "The HTTP or HTTPS port of the Consul server. Defaults to 8500.")
 	c.flags.StringVar(&c.flagConsulCACert, "consul-ca-cert", "",

subcommand/server-acl-init/command_test.go

@@ -680,9 +680,8 @@ func TestRun_ConnectInjectAuthMethod(t *testing.T) {
 	t.Parallel()
 
 	cases := map[string]struct {
-		flags          []string
-		expectedHost   string
-		expectedCACert string
+		flags        []string
+		expectedHost string
 	}{
 		"-create-inject-token flag": {
 			flags:        []string{"-create-inject-token"},
@@ -707,9 +706,6 @@ func TestRun_ConnectInjectAuthMethod(t *testing.T) {
 			defer testSvr.Stop()
 			caCert, jwtToken := setUpK8sServiceAccount(tt, k8s)
 			require := require.New(tt)
-			if c.expectedCACert != "" {
-				caCert = c.expectedCACert
-			}
 
 			// Run the command.
 			ui := cli.NewMockUi()
@@ -770,6 +766,90 @@ func TestRun_ConnectInjectAuthMethod(t *testing.T) {
 	}
 }
 
+// Test that when we provide a different k8s auth method parameters,
+// the auth method is updated.
+func TestRun_ConnectInjectAuthMethodUpdates(t *testing.T) {
+	t.Parallel()
+
+	k8s, testSvr := completeSetup(t)
+	defer testSvr.Stop()
+	caCert, jwtToken := setUpK8sServiceAccount(t, k8s)
+	require := require.New(t)
+
+	ui := cli.NewMockUi()
+	cmd := Command{
+		UI:        ui,
+		clientset: k8s,
+	}
+
+	bindingRuleSelector := "serviceaccount.name!=default"
+
+	// First, create an auth method using the defaults
+	responseCode := cmd.Run([]string{
+		"-resource-prefix=" + resourcePrefix,
+		"-k8s-namespace=" + ns,
+		"-server-address", strings.Split(testSvr.HTTPAddr, ":")[0],
+		"-server-port", strings.Split(testSvr.HTTPAddr, ":")[1],
+		"-create-inject-auth-method",
+		"-acl-binding-rule-selector=" + bindingRuleSelector,
+	})
+	require.Equal(0, responseCode, ui.ErrorWriter.String())
+
+	// Check that the auth method was created.
+	bootToken := getBootToken(t, k8s, resourcePrefix, ns)
+	consul, err := api.NewClient(&api.Config{
+		Address: testSvr.HTTPAddr,
+	})
+	require.NoError(err)
+	authMethodName := resourcePrefix + "-k8s-auth-method"
+	authMethod, _, err := consul.ACL().AuthMethodRead(authMethodName,
+		&api.QueryOptions{Token: bootToken})
+	require.NoError(err)
+	require.NotNil(authMethod)
+	require.Contains(authMethod.Config, "Host")
+	require.Equal(authMethod.Config["Host"], defaultKubernetesHost)
+	require.Contains(authMethod.Config, "CACert")
+	require.Equal(authMethod.Config["CACert"], caCert)
+	require.Contains(authMethod.Config, "ServiceAccountJWT")
+	require.Equal(authMethod.Config["ServiceAccountJWT"], jwtToken)
+
+	// Generate a new CA certificate
+	_, _, caCertPem, _, err := cert.GenerateCA("kubernetes")
+	require.NoError(err)
+
+	// Overwrite the default kubernetes api, service account token and CA cert
+	kubernetesHost := "https://kubernetes.example.com"
+	serviceAccountToken = "ZXlKaGJHY2lPaUpJVXpJMU5pSXNJblI1Y0NJNklrcFhWQ0o5LmV5SnpkV0lpT2lJeE1qTTBOVFkzT0Rrd0lpd2libUZ0WlNJNklrcHZhRzRnUkc5bElpd2lhV0YwSWpveE5URTJNak01TURJeWZRLlNmbEt4d1JKU01lS0tGMlFUNGZ3cE1lSmYzNlBPazZ5SlZfYWRRc3N3NWM="
+	serviceAccountCACert = base64.StdEncoding.EncodeToString([]byte(caCertPem))
+
+	// Create a new service account
+	updatedCACert, updatedJWTToken := setUpK8sServiceAccount(t, k8s)
+
+	// Run command again
+	responseCode = cmd.Run([]string{
+		"-resource-prefix=" + resourcePrefix,
+		"-k8s-namespace=" + ns,
+		"-server-address", strings.Split(testSvr.HTTPAddr, ":")[0],
+		"-server-port", strings.Split(testSvr.HTTPAddr, ":")[1],
+		"-acl-binding-rule-selector=" + bindingRuleSelector,
+		"-create-inject-auth-method",
+		"-inject-auth-method-host=" + kubernetesHost,
+	})
+	require.Equal(0, responseCode, ui.ErrorWriter.String())
+
+	// Check that the auth method has been updated
+	authMethod, _, err = consul.ACL().AuthMethodRead(authMethodName,
+		&api.QueryOptions{Token: bootToken})
+	require.NoError(err)
+	require.NotNil(authMethod)
+	require.Contains(authMethod.Config, "Host")
+	require.Equal(authMethod.Config["Host"], kubernetesHost)
+	require.Contains(authMethod.Config, "CACert")
+	require.Equal(authMethod.Config["CACert"], updatedCACert)
+	require.Contains(authMethod.Config, "ServiceAccountJWT")
+	require.Equal(authMethod.Config["ServiceAccountJWT"], updatedJWTToken)
+}
+
 // Test that ACL binding rules are updated if the rule selector changes.
 func TestRun_BindingRuleUpdates(t *testing.T) {
 	t.Parallel()
@@ -1766,45 +1846,50 @@ func generateServerCerts(t *testing.T) (string, string, string, func()) {
 // when the injector deployment is created. It returns the Service Account
 // CA Cert and JWT token.
 func setUpK8sServiceAccount(t *testing.T, k8s *fake.Clientset) (string, string) {
-	// Create Kubernetes Service.
-	_, err := k8s.CoreV1().Services(ns).Create(&v1.Service{
-		Spec: v1.ServiceSpec{
-			ClusterIP: "1.2.3.4",
-		},
-		ObjectMeta: metav1.ObjectMeta{
-			Name: "kubernetes",
-		},
-	})
-	require.NoError(t, err)
-
-	// Create ServiceAccount for the injector that the helm chart creates.
-	_, err = k8s.CoreV1().ServiceAccounts(ns).Create(&v1.ServiceAccount{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: resourcePrefix + "-connect-injector-authmethod-svc-account",
-		},
-		Secrets: []v1.ObjectReference{
-			{
-				Name: resourcePrefix + "-connect-injector-authmethod-svc-account",
+	// Create ServiceAccount for the kubernetes auth method if it doesn't exist,
+	// and update do nothing.
+	serviceAccountName := resourcePrefix + "-connect-injector-authmethod-svc-account"
+	sa, _ := k8s.CoreV1().ServiceAccounts(ns).Get(serviceAccountName, metav1.GetOptions{})
+	if sa == nil {
+		_, err := k8s.CoreV1().ServiceAccounts(ns).Create(&v1.ServiceAccount{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: serviceAccountName,
 			},
-		},
-	})
-	require.NoError(t, err)
+			Secrets: []v1.ObjectReference{
+				{
+					Name: resourcePrefix + "-connect-injector-authmethod-svc-account",
+				},
+			},
+		})
+		require.NoError(t, err)
+	}
 
 	// Create the ServiceAccount Secret.
 	caCertBytes, err := base64.StdEncoding.DecodeString(serviceAccountCACert)
 	require.NoError(t, err)
 	tokenBytes, err := base64.StdEncoding.DecodeString(serviceAccountToken)
 	require.NoError(t, err)
-	_, err = k8s.CoreV1().Secrets(ns).Create(&v1.Secret{
+
+	// Create a Kubernetes secret if it doesn't exist, otherwise update it
+	secretName := resourcePrefix + "-connect-injector-authmethod-svc-account"
+	secret := &v1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: resourcePrefix + "-connect-injector-authmethod-svc-account",
+			Name: secretName,
 		},
 		Data: map[string][]byte{
 			"ca.crt": caCertBytes,
 			"token":  tokenBytes,
 		},
-	})
-	require.NoError(t, err)
+	}
+	existingSecret, _ := k8s.CoreV1().Secrets(ns).Get(secretName, metav1.GetOptions{})
+	if existingSecret == nil {
+		_, err = k8s.CoreV1().Secrets(ns).Create(secret)
+		require.NoError(t, err)
+	} else {
+		_, err = k8s.CoreV1().Secrets(ns).Update(secret)
+		require.NoError(t, err)
+	}
+
 	return string(caCertBytes), string(tokenBytes)
 }
 

subcommand/server-acl-init/connect_inject.go

@@ -9,83 +9,58 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
+const defaultKubernetesHost = "https://kubernetes.default.svc"
+
 // configureConnectInject sets up auth methods so that connect injection will
 // work.
 func (c *Command) configureConnectInject(consulClient *api.Client) error {
 
 	authMethodName := c.withPrefix("k8s-auth-method")
 
-	// If not running namespaces, check if there's already an auth method.
-	// This means no changes need to be made to it. Binding rules should
-	// still be checked in case a user has updated their config.
-	var createAuthMethod bool
-	if !c.flagEnableNamespaces {
-		// Check if an auth method exists with the given name
-		err := c.untilSucceeds(fmt.Sprintf("checking if %s auth method exists", authMethodName),
-			func() error {
-				am, _, err := consulClient.ACL().AuthMethodRead(authMethodName, &api.QueryOptions{})
-				// This call returns nil if an AuthMethod does
-				// not exist with that name. This means we will
-				// need to create one.
-				if err == nil && am == nil {
-					createAuthMethod = true
-				}
-				return err
-			})
-		if err != nil {
-			return err
-		}
+	// Create the auth method template. This requires calls to the
+	// kubernetes environment.
+	authMethodTmpl, err := c.createAuthMethodTmpl(authMethodName)
+	if err != nil {
+		return err
 	}
 
-	// If namespaces are enabled, a namespace configuration change may need
-	// the auth method to be updated (as with a different mirroring prefix)
-	// or a new auth method created (if a new destination namespace is specified).
-	if c.flagEnableNamespaces || createAuthMethod {
-		// Create the auth method template. This requires calls to the
-		// kubernetes environment.
-		authMethodTmpl, err := c.createAuthMethodTmpl(authMethodName)
-		if err != nil {
-			return err
-		}
-
-		// Set up the auth method in the specific namespace if not mirroring
-		// If namespaces and mirroring are enabled, this is not necessary because
-		// the auth method will fall back to being created in the Consul `default`
-		// namespace automatically, as is necessary for mirroring.
-		// Note: if the config changes, an auth method will be created in the
-		// correct namespace, but the old auth method will not be removed.
-		writeOptions := api.WriteOptions{}
-		if c.flagEnableNamespaces && !c.flagEnableInjectK8SNSMirroring {
-			writeOptions.Namespace = c.flagConsulInjectDestinationNamespace
-
-			if c.flagConsulInjectDestinationNamespace != consulDefaultNamespace {
-				// If not the default namespace, check if it exists, creating it
-				// if necessary. The Consul namespace must exist for the AuthMethod
-				// to be created there.
-				err = c.untilSucceeds(fmt.Sprintf("checking or creating namespace %s",
-					c.flagConsulInjectDestinationNamespace),
-					func() error {
-						err := c.checkAndCreateNamespace(c.flagConsulInjectDestinationNamespace, consulClient)
-						return err
-					})
-				if err != nil {
+	// Set up the auth method in the specific namespace if not mirroring.
+	// If namespaces and mirroring are enabled, this is not necessary because
+	// the auth method will fall back to being created in the Consul `default`
+	// namespace automatically, as is necessary for mirroring.
+	// Note: if the config changes, an auth method will be created in the
+	// correct namespace, but the old auth method will not be removed.
+	writeOptions := api.WriteOptions{}
+	if c.flagEnableNamespaces && !c.flagEnableInjectK8SNSMirroring {
+		writeOptions.Namespace = c.flagConsulInjectDestinationNamespace
+
+		if c.flagConsulInjectDestinationNamespace != consulDefaultNamespace {
+			// If not the default namespace, check if it exists, creating it
+			// if necessary. The Consul namespace must exist for the AuthMethod
+			// to be created there.
+			err = c.untilSucceeds(fmt.Sprintf("checking or creating namespace %s",
+				c.flagConsulInjectDestinationNamespace),
+				func() error {
+					err := c.checkAndCreateNamespace(c.flagConsulInjectDestinationNamespace, consulClient)
 					return err
-				}
+				})
+			if err != nil {
+				return err
 			}
 		}
+	}
 
-		err = c.untilSucceeds(fmt.Sprintf("creating auth method %s", authMethodTmpl.Name),
-			func() error {
-				var err error
-				// `AuthMethodCreate` will also be able to update an existing
-				// AuthMethod based on the name provided. This means that any namespace
-				// configuration changes will correctly update the AuthMethod.
-				_, _, err = consulClient.ACL().AuthMethodCreate(&authMethodTmpl, &writeOptions)
-				return err
-			})
-		if err != nil {
+	err = c.untilSucceeds(fmt.Sprintf("creating auth method %s", authMethodTmpl.Name),
+		func() error {
+			var err error
+			// `AuthMethodCreate` will also be able to update an existing
+			// AuthMethod based on the name provided. This means that any
+			// configuration changes will correctly update the AuthMethod.
+			_, _, err = consulClient.ACL().AuthMethodCreate(&authMethodTmpl, &writeOptions)
 			return err
-		}
+		})
+	if err != nil {
+		return err
 	}
 
 	// Create the binding rule.
@@ -110,7 +85,7 @@ func (c *Command) configureConnectInject(consulClient *api.Client) error {
 	}
 
 	var existingRules []*api.ACLBindingRule
-	err := c.untilSucceeds(fmt.Sprintf("listing binding rules for auth method %s", authMethodName),
+	err = c.untilSucceeds(fmt.Sprintf("listing binding rules for auth method %s", authMethodName),
 		func() error {
 			var err error
 			existingRules, _, err = consulClient.ACL().BindingRuleList(authMethodName, &queryOptions)
@@ -186,7 +161,7 @@ func (c *Command) createAuthMethodTmpl(authMethodName string) (api.ACLAuthMethod
 		return api.ACLAuthMethod{}, err
 	}
 
-	kubernetesHost := "https://kubernetes.default.svc"
+	kubernetesHost := defaultKubernetesHost
 
 	// Check if custom auth method Host and CACert are provided
 	if c.flagInjectAuthMethodHost != "" {