From 25b21f8997eb9c9a87ba26e0c2cb1cd094092d69 Mon Sep 17 00:00:00 2001 From: Iryna Shustava Date: Tue, 23 Jun 2020 15:35:30 -0700 Subject: [PATCH 1/4] Always update auth method. Previously, we would only update auth method in Consul if Consul namespaces are enabled. This was necessary to make sure that if namespace configuration changes, it is reflected in the auth method. However, running Consul servers externally is another use case why it's important to update the auth method. That is because if you run a Helm install, then uninstall and install again, the auth method saved in Consul will be out-of-date: it will have the service account JWT token saved in the auth method refers to the service account token that has been deleted from Kubernetes when you ran helm uninstall. This change proposes to always update the auth method to solve for the external servers use case. --- .circleci/config.yml | 4 +- subcommand/server-acl-init/command.go | 2 +- subcommand/server-acl-init/command_test.go | 149 +++++++++++++++---- subcommand/server-acl-init/connect_inject.go | 107 +++++-------- 4 files changed, 161 insertions(+), 101 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index dbecb73c24..4e51677a4d 100644 --- a/.circleci/config.yml +++ b/.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: diff --git a/subcommand/server-acl-init/command.go b/subcommand/server-acl-init/command.go index 872afb1610..e75e9f908f 100644 --- a/subcommand/server-acl-init/command.go +++ b/subcommand/server-acl-init/command.go @@ -136,7 +136,7 @@ func (c *Command) init() { "default namespace, specify the value in the form ..") 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", "", diff --git a/subcommand/server-acl-init/command_test.go b/subcommand/server-acl-init/command_test.go index a86ebbd5dd..a6b205b791 100644 --- a/subcommand/server-acl-init/command_test.go +++ b/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) } diff --git a/subcommand/server-acl-init/connect_inject.go b/subcommand/server-acl-init/connect_inject.go index dec66d6114..8e54dede9e 100644 --- a/subcommand/server-acl-init/connect_inject.go +++ b/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 != "" { From 363deaadc9ee562a5b3d8750674410a62a5e59c7 Mon Sep 17 00:00:00 2001 From: Iryna Shustava Date: Tue, 23 Jun 2020 15:48:47 -0700 Subject: [PATCH 2/4] Switch back consul 1.7 because it looks like our unit tests need to be fixed first --- .circleci/config.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 4e51677a4d..dbecb73c24 100644 --- a/.circleci/config.yml +++ b/.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.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 + - 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 jobs: go-fmt-and-vet: From 81cdb4699818405036979c9ce739b7a6a0aa7d95 Mon Sep 17 00:00:00 2001 From: Iryna Shustava Date: Wed, 24 Jun 2020 13:01:14 -0700 Subject: [PATCH 3/4] Add docs to defaultKubernetesHost constant --- subcommand/server-acl-init/connect_inject.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/subcommand/server-acl-init/connect_inject.go b/subcommand/server-acl-init/connect_inject.go index 8e54dede9e..a51d25ee37 100644 --- a/subcommand/server-acl-init/connect_inject.go +++ b/subcommand/server-acl-init/connect_inject.go @@ -9,6 +9,10 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +// We use the default Kubernetes service as the default host +// for the auth method created in Consul. +// This is recommended as described here: +// https://kubernetes.io/docs/tasks/access-application-cluster/access-cluster/#accessing-the-api-from-a-pod const defaultKubernetesHost = "https://kubernetes.default.svc" // configureConnectInject sets up auth methods so that connect injection will From b657de31224239d7e60cb188aea58beae161f14a Mon Sep 17 00:00:00 2001 From: Iryna Shustava Date: Wed, 24 Jun 2020 13:04:55 -0700 Subject: [PATCH 4/4] Fix a couple of comments --- subcommand/server-acl-init/command_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/subcommand/server-acl-init/command_test.go b/subcommand/server-acl-init/command_test.go index a6b205b791..59f5483e2b 100644 --- a/subcommand/server-acl-init/command_test.go +++ b/subcommand/server-acl-init/command_test.go @@ -819,6 +819,7 @@ func TestRun_ConnectInjectAuthMethodUpdates(t *testing.T) { // Overwrite the default kubernetes api, service account token and CA cert kubernetesHost := "https://kubernetes.example.com" + // This token is the base64 encoded example token from jwt.io serviceAccountToken = "ZXlKaGJHY2lPaUpJVXpJMU5pSXNJblI1Y0NJNklrcFhWQ0o5LmV5SnpkV0lpT2lJeE1qTTBOVFkzT0Rrd0lpd2libUZ0WlNJNklrcHZhRzRnUkc5bElpd2lhV0YwSWpveE5URTJNak01TURJeWZRLlNmbEt4d1JKU01lS0tGMlFUNGZ3cE1lSmYzNlBPazZ5SlZfYWRRc3N3NWM=" serviceAccountCACert = base64.StdEncoding.EncodeToString([]byte(caCertPem)) @@ -1847,7 +1848,7 @@ func generateServerCerts(t *testing.T) (string, string, string, func()) { // CA Cert and JWT token. func setUpK8sServiceAccount(t *testing.T, k8s *fake.Clientset) (string, string) { // Create ServiceAccount for the kubernetes auth method if it doesn't exist, - // and update do nothing. + // otherwise, do nothing. serviceAccountName := resourcePrefix + "-connect-injector-authmethod-svc-account" sa, _ := k8s.CoreV1().ServiceAccounts(ns).Get(serviceAccountName, metav1.GetOptions{}) if sa == nil {