From 38441ed36f182ff361d1746fb1d321b2a645f6f9 Mon Sep 17 00:00:00 2001 From: Luke Kysow <1034429+lkysow@users.noreply.github.com> Date: Mon, 9 Dec 2019 12:59:49 -0800 Subject: [PATCH 1/3] Support fullnameOverride In the Helm chart there is an undocumented value that allows users to override the prefix we usually apply to the resources. The default prefix is -consul. This change adds a flag to the server-acl-init command (the only command that is affected by the prefix) called -resource-prefix. If set, we will use the correct prefix, both for finding resources that we expect to be created by the helm chart, e.g. Consul servers, and for creating our own resources, e.g. Kubernetes secrets. --- subcommand/server-acl-init/command.go | 26 +++- subcommand/server-acl-init/command_test.go | 167 ++++++++++++++------- 2 files changed, 134 insertions(+), 59 deletions(-) diff --git a/subcommand/server-acl-init/command.go b/subcommand/server-acl-init/command.go index 03e46b4477..86f2c3b06c 100644 --- a/subcommand/server-acl-init/command.go +++ b/subcommand/server-acl-init/command.go @@ -28,6 +28,7 @@ type Command struct { flags *flag.FlagSet k8s *k8sflags.K8SFlags flagReleaseName string + flagResourcePrefix string flagReplicas int flagNamespace string flagAllowDNS bool @@ -54,6 +55,8 @@ func (c *Command) init() { c.flags = flag.NewFlagSet("", flag.ContinueOnError) c.flags.StringVar(&c.flagReleaseName, "release-name", "", "Name of Consul Helm release") + c.flags.StringVar(&c.flagResourcePrefix, "resource-prefix", "", + "Prefix to use for Kubernetes resources. If not set, will default to ${release-name}-consul.") c.flags.IntVar(&c.flagReplicas, "expected-replicas", 1, "Number of expected Consul server replicas") c.flags.StringVar(&c.flagNamespace, "k8s-namespace", "", @@ -115,6 +118,10 @@ func (c *Command) Run(args []string) int { c.UI.Error(fmt.Sprintf("%q is not a valid timeout: %s", c.flagTimeout, err)) return 1 } + if c.flagReleaseName == "" { + c.UI.Error("-release-name must be set") + return 1 + } var cancel context.CancelFunc c.cmdTimeout, cancel = context.WithTimeout(context.Background(), timeout) // The context will only ever be intentionally ended by the timeout. @@ -140,7 +147,7 @@ func (c *Command) Run(args []string) int { } // Wait if there's a rollout of servers. - ssName := c.flagReleaseName + "-consul-server" + ssName := c.withPrefix("server") err = c.untilSucceeds(fmt.Sprintf("waiting for rollout of statefulset %s", ssName), func() error { ss, err := c.clientset.AppsV1().StatefulSets(c.flagNamespace).Get(ssName, metav1.GetOptions{}) if err != nil { @@ -158,7 +165,7 @@ func (c *Command) Run(args []string) int { } // Check if we've already been bootstrapped. - bootTokenSecretName := fmt.Sprintf("%s-consul-bootstrap-acl-token", c.flagReleaseName) + bootTokenSecretName := c.withPrefix("bootstrap-acl-token") bootstrapToken, err := c.getBootstrapToken(logger, bootTokenSecretName) if err != nil { logger.Error(fmt.Sprintf("Unexpected error looking for preexisting bootstrap Secret: %s", err)) @@ -496,7 +503,7 @@ func (c *Command) setServerTokens(logger hclog.Logger, consulClient *api.Client, // policy and then writes the token to a Kubernetes secret. func (c *Command) createACL(name, rules string, consulClient *api.Client, logger hclog.Logger) error { // Check if the secret already exists, if so, we assume the ACL has already been created. - secretName := fmt.Sprintf("%s-consul-%s-acl-token", c.flagReleaseName, name) + secretName := c.withPrefix(name + "-acl-token") _, err := c.clientset.CoreV1().Secrets(c.flagNamespace).Get(secretName, metav1.GetOptions{}) if err == nil { logger.Info(fmt.Sprintf("Secret %q already exists", secretName)) @@ -598,7 +605,7 @@ func (c *Command) configureDNSPolicies(logger hclog.Logger, consulClient *api.Cl func (c *Command) configureConnectInject(logger hclog.Logger, consulClient *api.Client) error { // First, check if there's already an acl binding rule. If so, then this // work is already done. - authMethodName := fmt.Sprintf("%s-consul-k8s-auth-method", c.flagReleaseName) + authMethodName := c.withPrefix("k8s-auth-method") var existingRules []*api.ACLBindingRule err := c.untilSucceeds(fmt.Sprintf("listing binding rules for auth method %s", authMethodName), func() error { @@ -627,7 +634,7 @@ func (c *Command) configureConnectInject(logger hclog.Logger, consulClient *api. // Get the Secret name for the auth method ServiceAccount. var authMethodServiceAccount *apiv1.ServiceAccount - saName := fmt.Sprintf("%s-consul-connect-injector-authmethod-svc-account", c.flagReleaseName) + saName := c.withPrefix("connect-injector-authmethod-svc-account") err = c.untilSucceeds(fmt.Sprintf("getting %s ServiceAccount", saName), func() error { var err error @@ -714,6 +721,15 @@ func (c *Command) untilSucceeds(opName string, op func() error, logger hclog.Log return nil } +// withPrefix returns the name of resource with the correct prefix based +// on the -release-name or -resource-prefix flags. +func (c *Command) withPrefix(resource string) string { + if c.flagResourcePrefix != "" { + return fmt.Sprintf("%s-%s", c.flagResourcePrefix, resource) + } + return fmt.Sprintf("%s-consul-%s", c.flagReleaseName, resource) +} + // isNoLeaderErr returns true if err is due to trying to call the // bootstrap ACLs API when there is no leader elected. func isNoLeaderErr(err error) bool { diff --git a/subcommand/server-acl-init/command_test.go b/subcommand/server-acl-init/command_test.go index 59bed480a2..1fb4b6842a 100644 --- a/subcommand/server-acl-init/command_test.go +++ b/subcommand/server-acl-init/command_test.go @@ -22,10 +22,22 @@ import ( var ns = "default" var releaseName = "release-name" +var resourcePrefix = "release-name-consul" + +func TestRun_ReleaseNameFlagNotSet(t *testing.T) { + ui := cli.NewMockUi() + cmd := Command{ + UI: ui, + } + cmd.init() + responseCode := cmd.Run([]string{}) + require.Equal(t, 1, responseCode, ui.ErrorWriter.String()) + require.Contains(t, ui.ErrorWriter.String(), "-release-name must be set") +} func TestRun_Defaults(t *testing.T) { t.Parallel() - k8s, testAgent := completeSetup(t) + k8s, testAgent := completeSetup(t, resourcePrefix) defer testAgent.Shutdown() require := require.New(t) @@ -38,13 +50,14 @@ func TestRun_Defaults(t *testing.T) { cmd.init() responseCode := cmd.Run([]string{ "-release-name=" + releaseName, + "-resource-prefix=" + resourcePrefix, "-k8s-namespace=" + ns, "-expected-replicas=1", }) require.Equal(0, responseCode, ui.ErrorWriter.String()) // Test that the bootstrap kube secret is created. - bootToken := getBootToken(t, k8s, releaseName) + bootToken := getBootToken(t, k8s, resourcePrefix) // Check that it has the right policies. consul := testAgent.Client() @@ -72,38 +85,73 @@ func TestRun_Defaults(t *testing.T) { } // Test the different flags that should create tokens and save them as -// Kubernetes secrets. +// Kubernetes secrets. We also test using the -resource-prefix flag +// to ensure the secrets are created with the right prefix. func TestRun_Tokens(t *testing.T) { t.Parallel() cases := map[string]struct { - Flag string - TokenName string + TokenFlag string + ResourcePrefixFlag string + TokenName string + SecretName string }{ "client token": { - "-create-client-token", - "client", + TokenFlag: "-create-client-token", + ResourcePrefixFlag: "", + TokenName: "client", + SecretName: "release-name-consul-client-acl-token", + }, + "client token -resource-prefix": { + TokenFlag: "-create-client-token", + ResourcePrefixFlag: "my-prefix", + TokenName: "client", + SecretName: "my-prefix-client-acl-token", }, "catalog-sync token": { - "-create-sync-token", - "catalog-sync", + TokenFlag: "-create-sync-token", + ResourcePrefixFlag: "", + TokenName: "catalog-sync", + SecretName: "release-name-consul-catalog-sync-acl-token", + }, + "catalog-sync token -resource-prefix": { + TokenFlag: "-create-sync-token", + ResourcePrefixFlag: "my-prefix", + TokenName: "catalog-sync", + SecretName: "my-prefix-catalog-sync-acl-token", }, "enterprise-license token": { - "-create-enterprise-license-token", - "enterprise-license", + TokenFlag: "-create-enterprise-license-token", + ResourcePrefixFlag: "", + TokenName: "enterprise-license", + SecretName: "release-name-consul-enterprise-license-acl-token", }, - "snapshot-agent token": { - "-create-snapshot-agent-token", - "client-snapshot-agent", + "enterprise-license token -resource-prefix": { + TokenFlag: "-create-enterprise-license-token", + ResourcePrefixFlag: "my-prefix", + TokenName: "enterprise-license", + SecretName: "my-prefix-enterprise-license-acl-token", }, "mesh-gateway token": { - "-create-mesh-gateway-token", - "mesh-gateway", + TokenFlag: "-create-mesh-gateway-token", + ResourcePrefixFlag: "", + TokenName: "mesh-gateway", + SecretName: "release-name-consul-mesh-gateway-acl-token", + }, + "mesh-gateway token -resource-prefix": { + TokenFlag: "-create-mesh-gateway-token", + ResourcePrefixFlag: "my-prefix", + TokenName: "mesh-gateway", + SecretName: "my-prefix-mesh-gateway-acl-token", }, } - for name, c := range cases { - t.Run(name, func(t *testing.T) { - k8s, testAgent := completeSetup(t) + for testName, c := range cases { + t.Run(testName, func(t *testing.T) { + prefix := c.ResourcePrefixFlag + if c.ResourcePrefixFlag == "" { + prefix = releaseName + "-consul" + } + k8s, testAgent := completeSetup(t, prefix) defer testAgent.Shutdown() require := require.New(t) @@ -118,13 +166,16 @@ func TestRun_Tokens(t *testing.T) { "-release-name=" + releaseName, "-k8s-namespace=" + ns, "-expected-replicas=1", - c.Flag, + c.TokenFlag, + } + if c.ResourcePrefixFlag != "" { + cmdArgs = append(cmdArgs, "-resource-prefix="+c.ResourcePrefixFlag) } responseCode := cmd.Run(cmdArgs) require.Equal(0, responseCode, ui.ErrorWriter.String()) // Check that the client policy was created. - bootToken := getBootToken(t, k8s, releaseName) + bootToken := getBootToken(t, k8s, prefix) consul := testAgent.Client() policies, _, err := consul.ACL().PolicyList(&api.QueryOptions{Token: bootToken}) require.NoError(err) @@ -135,10 +186,10 @@ func TestRun_Tokens(t *testing.T) { break } } + require.True(found, "%s-token policy was not found", c.TokenName) // Test that the token was created as a Kubernetes Secret. - require.True(found, "%s-token policy was not found", c.TokenName) - tokenSecret, err := k8s.CoreV1().Secrets(ns).Get(fmt.Sprintf("%s-consul-%s-acl-token", releaseName, c.TokenName), metav1.GetOptions{}) + tokenSecret, err := k8s.CoreV1().Secrets(ns).Get(c.SecretName, metav1.GetOptions{}) require.NoError(err) require.NotNil(tokenSecret) token, ok := tokenSecret.Data["token"] @@ -150,7 +201,7 @@ func TestRun_Tokens(t *testing.T) { require.Equal(c.TokenName+"-token", tokenData.Policies[0].Name) // Test that if the same command is run again, it doesn't error. - t.Run(name+"-retried", func(t *testing.T) { + t.Run(testName+"-retried", func(t *testing.T) { ui := cli.NewMockUi() cmd := Command{ UI: ui, @@ -166,7 +217,7 @@ func TestRun_Tokens(t *testing.T) { func TestRun_AllowDNS(t *testing.T) { t.Parallel() - k8s, testAgent := completeSetup(t) + k8s, testAgent := completeSetup(t, resourcePrefix) defer testAgent.Shutdown() require := require.New(t) @@ -179,6 +230,7 @@ func TestRun_AllowDNS(t *testing.T) { cmd.init() cmdArgs := []string{ "-release-name=" + releaseName, + "-resource-prefix=" + resourcePrefix, "-k8s-namespace=" + ns, "-expected-replicas=1", "-allow-dns", @@ -187,7 +239,7 @@ func TestRun_AllowDNS(t *testing.T) { require.Equal(0, responseCode, ui.ErrorWriter.String()) // Check that the dns policy was created. - bootToken := getBootToken(t, k8s, releaseName) + bootToken := getBootToken(t, k8s, resourcePrefix) consul := testAgent.Client() policies, _, err := consul.ACL().PolicyList(&api.QueryOptions{Token: bootToken}) require.NoError(err) @@ -220,7 +272,7 @@ func TestRun_AllowDNS(t *testing.T) { func TestRun_ConnectInjectToken(t *testing.T) { t.Parallel() - k8s, testAgent := completeSetup(t) + k8s, testAgent := completeSetup(t, resourcePrefix) defer testAgent.Shutdown() require := require.New(t) @@ -238,11 +290,11 @@ func TestRun_ConnectInjectToken(t *testing.T) { // Create ServiceAccount for the injector that the helm chart creates. _, err = k8s.CoreV1().ServiceAccounts(ns).Create(&v1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{ - Name: releaseName + "-consul-connect-injector-authmethod-svc-account", + Name: resourcePrefix + "-connect-injector-authmethod-svc-account", }, Secrets: []v1.ObjectReference{ { - Name: releaseName + "-consul-connect-injector-authmethod-svc-accohndbv", + Name: resourcePrefix + "-connect-injector-authmethod-svc-accohndbv", }, }, }) @@ -255,7 +307,7 @@ func TestRun_ConnectInjectToken(t *testing.T) { require.NoError(err) _, err = k8s.CoreV1().Secrets(ns).Create(&v1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: releaseName + "-consul-connect-injector-authmethod-svc-accohndbv", + Name: resourcePrefix + "-connect-injector-authmethod-svc-accohndbv", }, Data: map[string][]byte{ "ca.crt": caCertBytes, @@ -274,6 +326,7 @@ func TestRun_ConnectInjectToken(t *testing.T) { bindingRuleSelector := "serviceaccount.name!=default" cmdArgs := []string{ "-release-name=" + releaseName, + "-resource-prefix=" + resourcePrefix, "-k8s-namespace=" + ns, "-expected-replicas=1", "-create-inject-token", @@ -283,9 +336,9 @@ func TestRun_ConnectInjectToken(t *testing.T) { require.Equal(0, responseCode, ui.ErrorWriter.String()) // Check that the auth method was created. - bootToken := getBootToken(t, k8s, releaseName) + bootToken := getBootToken(t, k8s, resourcePrefix) consul := testAgent.Client() - authMethodName := releaseName + "-consul-k8s-auth-method" + authMethodName := resourcePrefix + "-k8s-auth-method" authMethod, _, err := consul.ACL().AuthMethodRead(authMethodName, &api.QueryOptions{Token: bootToken}) require.NoError(err) @@ -359,6 +412,7 @@ func TestRun_DelayedServerPods(t *testing.T) { go func() { responseCode = cmd.Run([]string{ "-release-name=" + releaseName, + "-resource-prefix=" + resourcePrefix, "-k8s-namespace=" + ns, "-expected-replicas=1", }) @@ -375,7 +429,7 @@ func TestRun_DelayedServerPods(t *testing.T) { pods := k8s.CoreV1().Pods(ns) _, err = pods.Create(&v1.Pod{ ObjectMeta: metav1.ObjectMeta{ - Name: releaseName + "-consul-server-0", + Name: resourcePrefix + "-server-0", Labels: map[string]string{ "component": "server", "app": "consul", @@ -402,7 +456,7 @@ func TestRun_DelayedServerPods(t *testing.T) { require.NoError(err) _, err = k8s.AppsV1().StatefulSets(ns).Create(&appv1.StatefulSet{ ObjectMeta: metav1.ObjectMeta{ - Name: releaseName + "-consul-server", + Name: resourcePrefix + "-server", Labels: map[string]string{ "component": "server", "app": "consul", @@ -426,7 +480,7 @@ func TestRun_DelayedServerPods(t *testing.T) { } // Test that the bootstrap kube secret is created. - getBootToken(t, k8s, releaseName) + getBootToken(t, k8s, resourcePrefix) // Test that the expected API calls were made. require.Equal([]APICall{ @@ -488,7 +542,7 @@ func TestRun_InProgressDeployment(t *testing.T) { pods := k8s.CoreV1().Pods(ns) _, err = pods.Create(&v1.Pod{ ObjectMeta: metav1.ObjectMeta{ - Name: releaseName + "-consul-server-0", + Name: resourcePrefix + "-server-0", Labels: map[string]string{ "component": "server", "app": "consul", @@ -515,7 +569,7 @@ func TestRun_InProgressDeployment(t *testing.T) { require.NoError(err) _, err = k8s.AppsV1().StatefulSets(ns).Create(&appv1.StatefulSet{ ObjectMeta: metav1.ObjectMeta{ - Name: releaseName + "-consul-server", + Name: resourcePrefix + "-server", Labels: map[string]string{ "component": "server", "app": "consul", @@ -543,6 +597,7 @@ func TestRun_InProgressDeployment(t *testing.T) { go func() { responseCode = cmd.Run([]string{ "-release-name=" + releaseName, + "-resource-prefix=" + resourcePrefix, "-k8s-namespace=" + ns, "-expected-replicas=1", }) @@ -557,7 +612,7 @@ func TestRun_InProgressDeployment(t *testing.T) { time.Sleep(time.Duration(delay) * time.Millisecond) _, err = k8s.AppsV1().StatefulSets(ns).Update(&appv1.StatefulSet{ ObjectMeta: metav1.ObjectMeta{ - Name: releaseName + "-consul-server", + Name: resourcePrefix + "-server", Labels: map[string]string{ "component": "server", "app": "consul", @@ -581,7 +636,7 @@ func TestRun_InProgressDeployment(t *testing.T) { } // Test that the bootstrap kube secret is created. - getBootToken(t, k8s, releaseName) + getBootToken(t, k8s, resourcePrefix) // Test that the expected API calls were made. require.Equal([]APICall{ @@ -658,7 +713,7 @@ func TestRun_NoLeader(t *testing.T) { pods := k8s.CoreV1().Pods(ns) _, err = pods.Create(&v1.Pod{ ObjectMeta: metav1.ObjectMeta{ - Name: releaseName + "-consul-server-0", + Name: resourcePrefix + "-server-0", Labels: map[string]string{ "component": "server", "app": "consul", @@ -686,7 +741,7 @@ func TestRun_NoLeader(t *testing.T) { // Create Consul server Statefulset. _, err = k8s.AppsV1().StatefulSets(ns).Create(&appv1.StatefulSet{ ObjectMeta: metav1.ObjectMeta{ - Name: releaseName + "-consul-server", + Name: resourcePrefix + "-server", Labels: map[string]string{ "component": "server", "app": "consul", @@ -712,6 +767,7 @@ func TestRun_NoLeader(t *testing.T) { go func() { responseCode = cmd.Run([]string{ "-release-name=" + releaseName, + "-resource-prefix=" + resourcePrefix, "-k8s-namespace=" + ns, "-expected-replicas=1", }) @@ -726,7 +782,7 @@ func TestRun_NoLeader(t *testing.T) { } // Test that the bootstrap kube secret is created. - getBootToken(t, k8s, releaseName) + getBootToken(t, k8s, resourcePrefix) // Test that the expected API calls were made. require.Equal([]APICall{ @@ -812,7 +868,7 @@ func TestRun_ClientTokensRetry(t *testing.T) { pods := k8s.CoreV1().Pods(ns) _, err = pods.Create(&v1.Pod{ ObjectMeta: metav1.ObjectMeta{ - Name: releaseName + "-consul-server-0", + Name: resourcePrefix + "-server-0", Labels: map[string]string{ "component": "server", "app": "consul", @@ -840,7 +896,7 @@ func TestRun_ClientTokensRetry(t *testing.T) { // Create the server statefulset. _, err = k8s.AppsV1().StatefulSets(ns).Create(&appv1.StatefulSet{ ObjectMeta: metav1.ObjectMeta{ - Name: releaseName + "-consul-server", + Name: resourcePrefix + "-server", Labels: map[string]string{ "component": "server", "app": "consul", @@ -863,6 +919,7 @@ func TestRun_ClientTokensRetry(t *testing.T) { cmd.init() responseCode := cmd.Run([]string{ "-release-name=" + releaseName, + "-resource-prefix=" + resourcePrefix, "-k8s-namespace=" + ns, "-expected-replicas=1", }) @@ -938,7 +995,7 @@ func TestRun_AlreadyBootstrapped(t *testing.T) { pods := k8s.CoreV1().Pods(ns) _, err = pods.Create(&v1.Pod{ ObjectMeta: metav1.ObjectMeta{ - Name: releaseName + "-consul-server-0", + Name: resourcePrefix + "-server-0", Labels: map[string]string{ "component": "server", "app": "consul", @@ -966,7 +1023,7 @@ func TestRun_AlreadyBootstrapped(t *testing.T) { // Create the server statefulset. _, err = k8s.AppsV1().StatefulSets(ns).Create(&appv1.StatefulSet{ ObjectMeta: metav1.ObjectMeta{ - Name: releaseName + "-consul-server", + Name: resourcePrefix + "-server", Labels: map[string]string{ "component": "server", "app": "consul", @@ -983,7 +1040,7 @@ func TestRun_AlreadyBootstrapped(t *testing.T) { // Create the bootstrap secret. _, err = k8s.CoreV1().Secrets(ns).Create(&v1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: releaseName + "-consul-bootstrap-acl-token", + Name: resourcePrefix + "-bootstrap-acl-token", }, Data: map[string][]byte{ "token": []byte("old-token"), @@ -1000,13 +1057,14 @@ func TestRun_AlreadyBootstrapped(t *testing.T) { cmd.init() responseCode := cmd.Run([]string{ "-release-name=" + releaseName, + "-resource-prefix=" + resourcePrefix, "-k8s-namespace=" + ns, "-expected-replicas=1", }) require.Equal(0, responseCode, ui.ErrorWriter.String()) // Test that the Secret is the same. - secret, err := k8s.CoreV1().Secrets(ns).Get(releaseName+"-consul-bootstrap-acl-token", metav1.GetOptions{}) + secret, err := k8s.CoreV1().Secrets(ns).Get(resourcePrefix+"-bootstrap-acl-token", metav1.GetOptions{}) require.NoError(err) require.Contains(secret.Data, "token") require.Equal("old-token", string(secret.Data["token"])) @@ -1038,6 +1096,7 @@ func TestRun_Timeout(t *testing.T) { cmd.init() responseCode := cmd.Run([]string{ "-release-name=" + releaseName, + "-resource-prefix=" + resourcePrefix, "-k8s-namespace=" + ns, "-expected-replicas=1", "-timeout=500ms", @@ -1045,8 +1104,8 @@ func TestRun_Timeout(t *testing.T) { require.Equal(1, responseCode, ui.ErrorWriter.String()) } -// Set up test consul agent and kubernetes clusters with -func completeSetup(t *testing.T) (*fake.Clientset, *agent.TestAgent) { +// Set up test consul agent and kubernetes cluster. +func completeSetup(t *testing.T, prefix string) (*fake.Clientset, *agent.TestAgent) { require := require.New(t) k8s := fake.NewSimpleClientset() @@ -1064,7 +1123,7 @@ func completeSetup(t *testing.T) (*fake.Clientset, *agent.TestAgent) { // Create Consul server Pod. _, err = k8s.CoreV1().Pods(ns).Create(&v1.Pod{ ObjectMeta: metav1.ObjectMeta{ - Name: releaseName + "-consul-server-0", + Name: prefix + "-server-0", Labels: map[string]string{ "component": "server", "app": "consul", @@ -1093,7 +1152,7 @@ func completeSetup(t *testing.T) (*fake.Clientset, *agent.TestAgent) { // Create Consul server Statefulset. _, err = k8s.AppsV1().StatefulSets(ns).Create(&appv1.StatefulSet{ ObjectMeta: metav1.ObjectMeta{ - Name: releaseName + "-consul-server", + Name: prefix + "-server", Labels: map[string]string{ "component": "server", "app": "consul", @@ -1111,8 +1170,8 @@ func completeSetup(t *testing.T) (*fake.Clientset, *agent.TestAgent) { // getBootToken gets the bootstrap token from the Kubernetes secret. It will // cause a test failure if the Secret doesn't exist or is malformed. -func getBootToken(t *testing.T, k8s *fake.Clientset, releaseName string) string { - bootstrapSecret, err := k8s.CoreV1().Secrets(ns).Get(fmt.Sprintf("%s-consul-bootstrap-acl-token", releaseName), metav1.GetOptions{}) +func getBootToken(t *testing.T, k8s *fake.Clientset, prefix string) string { + bootstrapSecret, err := k8s.CoreV1().Secrets(ns).Get(fmt.Sprintf("%s-bootstrap-acl-token", prefix), metav1.GetOptions{}) require.NoError(t, err) require.NotNil(t, bootstrapSecret) bootToken, ok := bootstrapSecret.Data["token"] From 36d2c97af4d5047a536fa9ed2497435a5dcd341a Mon Sep 17 00:00:00 2001 From: Luke Kysow <1034429+lkysow@users.noreply.github.com> Date: Thu, 12 Dec 2019 14:38:20 -0800 Subject: [PATCH 2/3] Update subcommand/server-acl-init/command.go Co-Authored-By: Iryna Shustava --- subcommand/server-acl-init/command.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/subcommand/server-acl-init/command.go b/subcommand/server-acl-init/command.go index 86f2c3b06c..1fe7c3a92e 100644 --- a/subcommand/server-acl-init/command.go +++ b/subcommand/server-acl-init/command.go @@ -56,7 +56,7 @@ func (c *Command) init() { c.flags.StringVar(&c.flagReleaseName, "release-name", "", "Name of Consul Helm release") c.flags.StringVar(&c.flagResourcePrefix, "resource-prefix", "", - "Prefix to use for Kubernetes resources. If not set, will default to ${release-name}-consul.") + "Prefix to use for Kubernetes resources. If not set, the \"-consul\" prefix is used, where is the value set by the -release-name flag.") c.flags.IntVar(&c.flagReplicas, "expected-replicas", 1, "Number of expected Consul server replicas") c.flags.StringVar(&c.flagNamespace, "k8s-namespace", "", From 9c6ee55ebe4132db2cc005c2dfdc27d8ab1685c3 Mon Sep 17 00:00:00 2001 From: Luke Kysow <1034429+lkysow@users.noreply.github.com> Date: Fri, 13 Dec 2019 17:23:04 -0800 Subject: [PATCH 3/3] Add new -server-label-selector flag This flag removes the implicit reliance on the -release-name and -resource-prefix flags for selecting the Consul server statefulsets pods. With this flag, users explicitly specify the label selector used to identify the server statefulset pods. The statefulset itself is still selected via name since it's missing the component=server label that the Pods have. The deprecated -release-name flag is still supported. If using the -resource-prefix flag, we also require the new -server-label-selector flag. --- subcommand/server-acl-init/command.go | 44 ++++-- subcommand/server-acl-init/command_test.go | 170 +++++++++++++-------- 2 files changed, 139 insertions(+), 75 deletions(-) diff --git a/subcommand/server-acl-init/command.go b/subcommand/server-acl-init/command.go index 1fe7c3a92e..be098769ae 100644 --- a/subcommand/server-acl-init/command.go +++ b/subcommand/server-acl-init/command.go @@ -28,6 +28,7 @@ type Command struct { flags *flag.FlagSet k8s *k8sflags.K8SFlags flagReleaseName string + flagServerLabelSelector string flagResourcePrefix string flagReplicas int flagNamespace string @@ -54,7 +55,9 @@ type Command struct { func (c *Command) init() { c.flags = flag.NewFlagSet("", flag.ContinueOnError) c.flags.StringVar(&c.flagReleaseName, "release-name", "", - "Name of Consul Helm release") + "Name of Consul Helm release. Deprecated: Use -server-label-selector=component=server,app=consul,release= instead") + c.flags.StringVar(&c.flagServerLabelSelector, "server-label-selector", "", + "Selector (label query) to select Consul server statefulset pods, supports '=', '==', and '!='. (e.g. -l key1=value1,key2=value2)") c.flags.StringVar(&c.flagResourcePrefix, "resource-prefix", "", "Prefix to use for Kubernetes resources. If not set, the \"-consul\" prefix is used, where is the value set by the -release-name flag.") c.flags.IntVar(&c.flagReplicas, "expected-replicas", 1, @@ -118,10 +121,23 @@ func (c *Command) Run(args []string) int { c.UI.Error(fmt.Sprintf("%q is not a valid timeout: %s", c.flagTimeout, err)) return 1 } - if c.flagReleaseName == "" { - c.UI.Error("-release-name must be set") + if c.flagReleaseName != "" && c.flagServerLabelSelector != "" { + c.UI.Error("-release-name and -server-label-selector cannot both be set") return 1 } + if c.flagServerLabelSelector != "" && c.flagResourcePrefix == "" { + c.UI.Error("if -server-label-selector is set -resource-prefix must also be set") + return 1 + } + if c.flagReleaseName == "" && c.flagServerLabelSelector == "" { + c.UI.Error("-release-name or -server-label-selector must be set") + return 1 + } + // If only the -release-name is set, we use it as the label selector. + if c.flagReleaseName != "" { + c.flagServerLabelSelector = fmt.Sprintf("app=consul,component=server,release=%s", c.flagReleaseName) + } + var cancel context.CancelFunc c.cmdTimeout, cancel = context.WithTimeout(context.Background(), timeout) // The context will only ever be intentionally ended by the timeout. @@ -149,15 +165,19 @@ func (c *Command) Run(args []string) int { // Wait if there's a rollout of servers. ssName := c.withPrefix("server") err = c.untilSucceeds(fmt.Sprintf("waiting for rollout of statefulset %s", ssName), func() error { - ss, err := c.clientset.AppsV1().StatefulSets(c.flagNamespace).Get(ssName, metav1.GetOptions{}) + // Note: We can't use the -server-label-selector flag to find the statefulset + // because in older versions of consul-helm it wasn't labeled with + // component: server. We also can't drop that label because it's required + // for targeting the right server Pods. + statefulset, err := c.clientset.AppsV1().StatefulSets(c.flagNamespace).Get(ssName, metav1.GetOptions{}) if err != nil { return err } - if ss.Status.CurrentRevision == ss.Status.UpdateRevision { + if statefulset.Status.CurrentRevision == statefulset.Status.UpdateRevision { return nil } return fmt.Errorf("rollout is in progress (CurrentRevision=%s UpdateRevision=%s)", - ss.Status.CurrentRevision, ss.Status.UpdateRevision) + statefulset.Status.CurrentRevision, statefulset.Status.UpdateRevision) }, logger) if err != nil { logger.Error(err.Error()) @@ -296,15 +316,14 @@ func (c *Command) getConsulServers(logger hclog.Logger, n int) ([]podAddr, error var serverPods *apiv1.PodList err := c.untilSucceeds("discovering Consul server pods", func() error { - labelSelector := fmt.Sprintf("component=server, app=consul, release=%s", c.flagReleaseName) var err error - serverPods, err = c.clientset.CoreV1().Pods(c.flagNamespace).List(metav1.ListOptions{LabelSelector: labelSelector}) + serverPods, err = c.clientset.CoreV1().Pods(c.flagNamespace).List(metav1.ListOptions{LabelSelector: c.flagServerLabelSelector}) if err != nil { return err } if len(serverPods.Items) == 0 { - return fmt.Errorf("no server pods with labels %q found", labelSelector) + return fmt.Errorf("no server pods with labels %q found", c.flagServerLabelSelector) } if len(serverPods.Items) < n { @@ -664,7 +683,7 @@ func (c *Command) configureConnectInject(logger hclog.Logger, consulClient *api. // Now we're ready to set up Consul's auth method. authMethodTmpl := api.ACLAuthMethod{ Name: authMethodName, - Description: fmt.Sprintf("Consul %s default Kubernetes AuthMethod", c.flagReleaseName), + Description: "Kubernetes AuthMethod", Type: "kubernetes", Config: map[string]interface{}{ "Host": fmt.Sprintf("https://%s:443", kubeSvc.Spec.ClusterIP), @@ -685,7 +704,7 @@ func (c *Command) configureConnectInject(logger hclog.Logger, consulClient *api. // Create the binding rule. abr := api.ACLBindingRule{ - Description: fmt.Sprintf("Consul %s default binding rule", c.flagReleaseName), + Description: "Kubernetes binding rule", AuthMethod: authMethod.Name, BindType: api.BindingRuleBindTypeService, BindName: "${serviceaccount.name}", @@ -727,6 +746,9 @@ func (c *Command) withPrefix(resource string) string { if c.flagResourcePrefix != "" { return fmt.Sprintf("%s-%s", c.flagResourcePrefix, resource) } + // This is to support an older version of the Helm chart that only specified + // the -release-name flag. We ensure that this is set if -resource-prefix + // is not set when parsing the flags. return fmt.Sprintf("%s-consul-%s", c.flagReleaseName, resource) } diff --git a/subcommand/server-acl-init/command_test.go b/subcommand/server-acl-init/command_test.go index 1fb4b6842a..4cfc744ae3 100644 --- a/subcommand/server-acl-init/command_test.go +++ b/subcommand/server-acl-init/command_test.go @@ -24,81 +24,115 @@ var ns = "default" var releaseName = "release-name" var resourcePrefix = "release-name-consul" -func TestRun_ReleaseNameFlagNotSet(t *testing.T) { - ui := cli.NewMockUi() - cmd := Command{ - UI: ui, +func TestRun_FlagValidation(t *testing.T) { + cases := []struct { + Flags []string + ExpErr string + }{ + { + Flags: []string{}, + ExpErr: "-release-name or -server-label-selector must be set", + }, + { + Flags: []string{"-release-name=name", "-server-label-selector=hi"}, + ExpErr: "-release-name and -server-label-selector cannot both be set", + }, + { + Flags: []string{"-server-label-selector=hi"}, + ExpErr: "if -server-label-selector is set -resource-prefix must also be set", + }, + } + + for _, c := range cases { + t.Run(c.ExpErr, func(t *testing.T) { + ui := cli.NewMockUi() + cmd := Command{ + UI: ui, + } + responseCode := cmd.Run(c.Flags) + require.Equal(t, 1, responseCode, ui.ErrorWriter.String()) + require.Contains(t, ui.ErrorWriter.String(), c.ExpErr) + }) } - cmd.init() - responseCode := cmd.Run([]string{}) - require.Equal(t, 1, responseCode, ui.ErrorWriter.String()) - require.Contains(t, ui.ErrorWriter.String(), "-release-name must be set") } +// Test what happens if no extra flags were set (i.e. the defaults apply). +// We test with both the deprecated -release-name and the new -server-label-selector +// flags. func TestRun_Defaults(t *testing.T) { t.Parallel() - k8s, testAgent := completeSetup(t, resourcePrefix) - defer testAgent.Shutdown() - require := require.New(t) + for _, flags := range [][]string{ + {"-release-name=" + releaseName}, + { + "-server-label-selector=component=server,app=consul,release=" + releaseName, + "-resource-prefix=" + resourcePrefix, + }, + } { + t.Run(flags[0], func(t *testing.T) { + k8s, testAgent := completeSetup(t, resourcePrefix) + defer testAgent.Shutdown() + require := require.New(t) - // Run the command. - ui := cli.NewMockUi() - cmd := Command{ - UI: ui, - clientset: k8s, - } - cmd.init() - responseCode := cmd.Run([]string{ - "-release-name=" + releaseName, - "-resource-prefix=" + resourcePrefix, - "-k8s-namespace=" + ns, - "-expected-replicas=1", - }) - require.Equal(0, responseCode, ui.ErrorWriter.String()) + // Run the command. + ui := cli.NewMockUi() + cmd := Command{ + UI: ui, + clientset: k8s, + } + args := append([]string{ + "-k8s-namespace=" + ns, + "-expected-replicas=1", + }, flags...) + responseCode := cmd.Run(args) + require.Equal(0, responseCode, ui.ErrorWriter.String()) - // Test that the bootstrap kube secret is created. - bootToken := getBootToken(t, k8s, resourcePrefix) + // Test that the bootstrap kube secret is created. + bootToken := getBootToken(t, k8s, resourcePrefix) - // Check that it has the right policies. - consul := testAgent.Client() - tokenData, _, err := consul.ACL().TokenReadSelf(&api.QueryOptions{Token: bootToken}) - require.NoError(err) - require.Equal("global-management", tokenData.Policies[0].Name) + // Check that it has the right policies. + consul := testAgent.Client() + tokenData, _, err := consul.ACL().TokenReadSelf(&api.QueryOptions{Token: bootToken}) + require.NoError(err) + require.Equal("global-management", tokenData.Policies[0].Name) - // Check that the agent policy was created. - policies, _, err := consul.ACL().PolicyList(&api.QueryOptions{Token: bootToken}) - require.NoError(err) - found := false - for _, p := range policies { - if p.Name == "agent-token" { - found = true - break - } - } - require.True(found, "agent-token policy was not found") + // Check that the agent policy was created. + policies, _, err := consul.ACL().PolicyList(&api.QueryOptions{Token: bootToken}) + require.NoError(err) + found := false + for _, p := range policies { + if p.Name == "agent-token" { + found = true + break + } + } + require.True(found, "agent-token policy was not found") - // We should also test that the server's token was updated, however I - // couldn't find a way to test that with the test agent. Instead we test - // that in another test when we're using an httptest server instead of - // the test agent and we can assert that the /v1/agent/token/agent - // endpoint was called. + // We should also test that the server's token was updated, however I + // couldn't find a way to test that with the test agent. Instead we test + // that in another test when we're using an httptest server instead of + // the test agent and we can assert that the /v1/agent/token/agent + // endpoint was called. + }) + } } // Test the different flags that should create tokens and save them as -// Kubernetes secrets. We also test using the -resource-prefix flag -// to ensure the secrets are created with the right prefix. +// Kubernetes secrets. We test using the -release-name flag vs using the +// -resource-prefix flag. func TestRun_Tokens(t *testing.T) { t.Parallel() cases := map[string]struct { TokenFlag string ResourcePrefixFlag string + ReleaseNameFlag string TokenName string SecretName string }{ - "client token": { + "client token -release-name": { TokenFlag: "-create-client-token", ResourcePrefixFlag: "", + ReleaseNameFlag: "release-name", TokenName: "client", SecretName: "release-name-consul-client-acl-token", }, @@ -108,9 +142,10 @@ func TestRun_Tokens(t *testing.T) { TokenName: "client", SecretName: "my-prefix-client-acl-token", }, - "catalog-sync token": { + "catalog-sync token -release-name": { TokenFlag: "-create-sync-token", ResourcePrefixFlag: "", + ReleaseNameFlag: "release-name", TokenName: "catalog-sync", SecretName: "release-name-consul-catalog-sync-acl-token", }, @@ -120,9 +155,10 @@ func TestRun_Tokens(t *testing.T) { TokenName: "catalog-sync", SecretName: "my-prefix-catalog-sync-acl-token", }, - "enterprise-license token": { + "enterprise-license token -release-name": { TokenFlag: "-create-enterprise-license-token", ResourcePrefixFlag: "", + ReleaseNameFlag: "release-name", TokenName: "enterprise-license", SecretName: "release-name-consul-enterprise-license-acl-token", }, @@ -132,15 +168,17 @@ func TestRun_Tokens(t *testing.T) { TokenName: "enterprise-license", SecretName: "my-prefix-enterprise-license-acl-token", }, - "mesh-gateway token": { + "mesh-gateway token -release-name": { TokenFlag: "-create-mesh-gateway-token", ResourcePrefixFlag: "", + ReleaseNameFlag: "release-name", TokenName: "mesh-gateway", SecretName: "release-name-consul-mesh-gateway-acl-token", }, "mesh-gateway token -resource-prefix": { TokenFlag: "-create-mesh-gateway-token", ResourcePrefixFlag: "my-prefix", + ReleaseNameFlag: "release-name", TokenName: "mesh-gateway", SecretName: "my-prefix-mesh-gateway-acl-token", }, @@ -163,13 +201,17 @@ func TestRun_Tokens(t *testing.T) { } cmd.init() cmdArgs := []string{ - "-release-name=" + releaseName, "-k8s-namespace=" + ns, "-expected-replicas=1", c.TokenFlag, } if c.ResourcePrefixFlag != "" { - cmdArgs = append(cmdArgs, "-resource-prefix="+c.ResourcePrefixFlag) + // If using the -resource-prefix flag, we expect the -server-label-selector + // flag to also be set. + labelSelector := fmt.Sprintf("release=%s,component=server,app=consul", releaseName) + cmdArgs = append(cmdArgs, "-resource-prefix="+c.ResourcePrefixFlag, "-server-label-selector="+labelSelector) + } else { + cmdArgs = append(cmdArgs, "-release-name="+c.ReleaseNameFlag) } responseCode := cmd.Run(cmdArgs) require.Equal(0, responseCode, ui.ErrorWriter.String()) @@ -229,7 +271,7 @@ func TestRun_AllowDNS(t *testing.T) { } cmd.init() cmdArgs := []string{ - "-release-name=" + releaseName, + "-server-label-selector=component=server,app=consul,release=" + releaseName, "-resource-prefix=" + resourcePrefix, "-k8s-namespace=" + ns, "-expected-replicas=1", @@ -325,7 +367,7 @@ func TestRun_ConnectInjectToken(t *testing.T) { cmd.init() bindingRuleSelector := "serviceaccount.name!=default" cmdArgs := []string{ - "-release-name=" + releaseName, + "-server-label-selector=component=server,app=consul,release=" + releaseName, "-resource-prefix=" + resourcePrefix, "-k8s-namespace=" + ns, "-expected-replicas=1", @@ -411,7 +453,7 @@ func TestRun_DelayedServerPods(t *testing.T) { var responseCode int go func() { responseCode = cmd.Run([]string{ - "-release-name=" + releaseName, + "-server-label-selector=component=server,app=consul,release=" + releaseName, "-resource-prefix=" + resourcePrefix, "-k8s-namespace=" + ns, "-expected-replicas=1", @@ -596,7 +638,7 @@ func TestRun_InProgressDeployment(t *testing.T) { var responseCode int go func() { responseCode = cmd.Run([]string{ - "-release-name=" + releaseName, + "-server-label-selector=component=server,app=consul,release=" + releaseName, "-resource-prefix=" + resourcePrefix, "-k8s-namespace=" + ns, "-expected-replicas=1", @@ -766,7 +808,7 @@ func TestRun_NoLeader(t *testing.T) { var responseCode int go func() { responseCode = cmd.Run([]string{ - "-release-name=" + releaseName, + "-server-label-selector=component=server,app=consul,release=" + releaseName, "-resource-prefix=" + resourcePrefix, "-k8s-namespace=" + ns, "-expected-replicas=1", @@ -918,7 +960,7 @@ func TestRun_ClientTokensRetry(t *testing.T) { } cmd.init() responseCode := cmd.Run([]string{ - "-release-name=" + releaseName, + "-server-label-selector=component=server,app=consul,release=" + releaseName, "-resource-prefix=" + resourcePrefix, "-k8s-namespace=" + ns, "-expected-replicas=1", @@ -1056,7 +1098,7 @@ func TestRun_AlreadyBootstrapped(t *testing.T) { } cmd.init() responseCode := cmd.Run([]string{ - "-release-name=" + releaseName, + "-server-label-selector=component=server,app=consul,release=" + releaseName, "-resource-prefix=" + resourcePrefix, "-k8s-namespace=" + ns, "-expected-replicas=1", @@ -1095,7 +1137,7 @@ func TestRun_Timeout(t *testing.T) { } cmd.init() responseCode := cmd.Run([]string{ - "-release-name=" + releaseName, + "-server-label-selector=component=server,app=consul,release=" + releaseName, "-resource-prefix=" + resourcePrefix, "-k8s-namespace=" + ns, "-expected-replicas=1",