-
Notifications
You must be signed in to change notification settings - Fork 321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support fullnameOverride #174
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,8 @@ type Command struct { | |
flags *flag.FlagSet | ||
k8s *k8sflags.K8SFlags | ||
flagReleaseName string | ||
flagServerLabelSelector string | ||
flagResourcePrefix string | ||
flagReplicas int | ||
flagNamespace string | ||
flagAllowDNS bool | ||
|
@@ -53,7 +55,11 @@ 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=<release-name> 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 \"<release-name>-consul\" prefix is used, where <release-name> 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", "", | ||
|
@@ -115,6 +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.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. | ||
|
@@ -140,25 +163,29 @@ func (c *Command) Run(args []string) int { | |
} | ||
|
||
// Wait if there's a rollout of servers. | ||
ssName := c.flagReleaseName + "-consul-server" | ||
ssName := c.withPrefix("server") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, great idea |
||
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()) | ||
return 1 | ||
} | ||
|
||
// 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)) | ||
|
@@ -289,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 { | ||
|
@@ -496,7 +522,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 +624,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 +653,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 | ||
|
@@ -657,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), | ||
|
@@ -678,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}", | ||
|
@@ -714,6 +740,18 @@ 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) | ||
} | ||
// 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) | ||
} | ||
|
||
// 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 { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, yeah I like that.