Support fullnameOverride #174

lkysow · 2019-12-09T21:02:36Z

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 two flags to the
server-acl-init command (the only command that is affected by the
prefix): -resource-prefix and -server-label-selector.

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.

The flags are backwards compatible. Users can upgrade consul-k8s and not require any changes to their consul-helm version. There is a corresponding consul-helm update that uses these flags. That is only required if users want to take advantage of this bugfix (which now fully supports the name override values).

Fixes Incorrect assumption of StatefulSet name in server-acl-init #165
Corresponding consul-helm PR: Support global.name consul-helm#307

lkysow · 2019-12-10T00:39:59Z

subcommand/server-acl-init/command.go

@@ -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")


If this isn't set, we won't find the Consul servers since we do a lookup by label so best to give an error.

lkysow · 2019-12-10T00:42:38Z

subcommand/server-acl-init/command_test.go

@@ -22,10 +22,22 @@ import (

 var ns = "default"
 var releaseName = "release-name"
+var resourcePrefix = "release-name-consul"


I've updated the tests to use both the -release-name and -resource-prefix flags since the corresponding change to the helm chart will use both flags and I thought it best to match the Helm chart usage.

I've also updated one test to not use the -resource-prefix flag, to test that behaviour (should default to release-name-consul). That's important because if someone upgrades consul-k8s and not the Helm chart, they won't be setting that -resource-prefix flag.

lkysow · 2019-12-10T00:43:13Z

subcommand/server-acl-init/command_test.go

+		TokenFlag          string
+		ResourcePrefixFlag string
+		TokenName          string
+		SecretName         string


I expanded these cases to include whether we set -resource-prefix or not

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 <helm release name>-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.

ishustava

Luke, this seems like a good change to me. I've added a couple of thoughts. I don't think they should block this PR though.

ishustava · 2019-12-12T19:54:39Z

subcommand/server-acl-init/command.go

@@ -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")


Could we look up the stateful set using label selectors, similarly to how we're doing it with Pods? That way we don't have to assume the naming convention of the server stateful set used in the chart.

yes, great idea

subcommand/server-acl-init/command.go

ishustava · 2019-12-12T22:13:04Z

subcommand/server-acl-init/command.go

@@ -28,6 +28,7 @@ type Command struct {
 	flags                        *flag.FlagSet
 	k8s                          *k8sflags.K8SFlags
 	flagReleaseName              string


If we always set the resource prefix flag through the helm chart, then the only thing this flag is used for is selecting the server pods. Would it make sense to always require -resource-prefix and change this flag to take a label selector?

The reason I'm suggesting it is that I think we are making a few hidden assumptions in this command about how resources are named in the helm chart, and it seems like a leaky abstraction.

Ooh, yeah I like that.

Co-Authored-By: Iryna Shustava <ishustava@users.noreply.github.com>

lkysow · 2019-12-12T23:24:00Z

I'm gonna have to re-jig some of the tests to take account for the label-selector.

lkysow · 2019-12-14T01:27:24Z

@ishustava I've updated the PR by adding the new -server-label-selector flag as suggested. I like how it removes that implicit reliance on the server name and its labels.

As a result, I've had to stop putting the release name in the description of the different tokens. This doesn't cause any issues with duplication because the token names themselves in Consul require uniqueness not the descriptions and the names are the same as they were previously.

ishustava

Looks great! Thank you for making the changes @lkysow!

ishustava · 2019-12-17T17:17:55Z

subcommand/server-acl-init/command.go

 	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{})
+		statefulsets, err := c.clientset.AppsV1().StatefulSets(c.flagNamespace).List(metav1.ListOptions{LabelSelector: c.flagServerLabelSelector})


@lkysow I realized that this will require changes to the helm chart. Currently the labels for the server pods and the server stateful set are different, so we can't use the same label selector.

Good catch, I've moved back to using the name of the statefulset.

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.

lkysow force-pushed the full-name-override branch 2 times, most recently from b669f05 to 49cdd48 Compare December 10, 2019 00:39

lkysow commented Dec 10, 2019

View reviewed changes

lkysow force-pushed the full-name-override branch from 49cdd48 to 38441ed Compare December 10, 2019 00:44

lkysow mentioned this pull request Dec 10, 2019

Support global.name hashicorp/consul-helm#307

Merged

lkysow requested a review from a team December 10, 2019 00:59

lkysow added the type/bug Something isn't working label Dec 11, 2019

ishustava approved these changes Dec 12, 2019

View reviewed changes

Update subcommand/server-acl-init/command.go

36d2c97

Co-Authored-By: Iryna Shustava <ishustava@users.noreply.github.com>

ishustava approved these changes Dec 16, 2019

View reviewed changes

ishustava reviewed Dec 17, 2019

View reviewed changes

lkysow force-pushed the full-name-override branch 2 times, most recently from 679fe9e to 21cdfb5 Compare December 17, 2019 18:12

lkysow force-pushed the full-name-override branch from 21cdfb5 to 9c6ee55 Compare December 17, 2019 18:18

lkysow merged commit e5f0bc7 into master Dec 17, 2019

lkysow deleted the full-name-override branch December 17, 2019 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support fullnameOverride #174

Support fullnameOverride #174

lkysow commented Dec 9, 2019 •

edited

Loading

lkysow Dec 10, 2019

lkysow Dec 10, 2019

lkysow Dec 10, 2019

ishustava left a comment

ishustava Dec 12, 2019

lkysow Dec 12, 2019

ishustava Dec 12, 2019

lkysow Dec 12, 2019

lkysow commented Dec 12, 2019

lkysow commented Dec 14, 2019

ishustava left a comment

ishustava Dec 17, 2019

lkysow Dec 17, 2019

Support fullnameOverride #174

Support fullnameOverride #174

Conversation

lkysow commented Dec 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishustava left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkysow commented Dec 12, 2019

lkysow commented Dec 14, 2019

ishustava left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkysow commented Dec 9, 2019 •

edited

Loading