code review updates

subcommand/create-federation-secret/command.go

@@ -45,6 +45,15 @@ type Command struct {
 	// token in the secret.
 	flagExportReplicationToken bool
 	flagGossipKeyFile          string
+
+	// flagServerCACertFile is the location of the file containing the CA cert
+	// for servers. We also accept a -ca-file flag. This will point to a different
+	// file when auto-encrypt is enabled, otherwise it will point to the same file
+	// as -server-ca-cert-file.
+	// When auto-encrypt is enabled, the clients
+	// use a different CA than the servers (since they piggy-back on the Connect CA)
+	// and so when talking to our local client we need to use the CA cert passed
+	// via -ca-file, not the server CA.
 	flagServerCACertFile       string
 	flagServerCAKeyFile        string
 	flagResourcePrefix         string
@@ -87,7 +96,6 @@ func (c *Command) init() {
 	flags.Merge(c.flags, c.http.ClientFlags())
 	flags.Merge(c.flags, c.http.ServerFlags())
 	flags.Merge(c.flags, c.k8s.Flags())
-
 }
 
 // Run creates a Kubernetes secret with data needed by secondary datacenters
@@ -96,35 +104,7 @@ func (c *Command) init() {
 func (c *Command) Run(args []string) int {
 	c.once.Do(c.init)
 
-	// Validate flags.
-	if err := c.flags.Parse(args); err != nil {
-		return 1
-	}
-	if len(c.flags.Args()) > 0 {
-		c.UI.Error("Should have no non-flag arguments")
-		return 1
-	}
-	if c.flagResourcePrefix == "" {
-		c.UI.Error("-resource-prefix must be set")
-		return 1
-	}
-	if c.flagK8sNamespace == "" {
-		c.UI.Error("-k8s-namespace must be set")
-		return 1
-	}
-	if c.flagServerCACertFile == "" {
-		c.UI.Error("-server-ca-cert-file must be set")
-		return 1
-	}
-	if c.flagServerCAKeyFile == "" {
-		c.UI.Error("-server-ca-key-file must be set")
-		return 1
-	}
-	if c.flagMeshGatewayServiceName == "" {
-		c.UI.Error("-mesh-gateway-service-name must be set")
-		return 1
-	}
-	if err := c.validateCAFileFlag(); err != nil {
+	if err := c.validateFlags(args); err != nil {
 		c.UI.Error(err.Error())
 		return 1
 	}
@@ -159,18 +139,15 @@ func (c *Command) Run(args []string) int {
 			c.UI.Error(fmt.Sprintf("Error reading gossip encryption key file: %s", err))
 			return 1
 		}
+		if len(gossipKey) == 0 {
+			c.UI.Error(fmt.Sprintf("gossip key file %q was empty", c.flagGossipKeyFile))
+			return 1
+		}
 		federationSecret.Data[fedSecretGossipKey] = gossipKey
 		logger.Info("Gossip encryption key retrieved successfully")
 	}
 
-	// Add CA cert.
-	// Note: This CA cert may be different from the one passed into the -ca-file
-	// flag if auto-encrypt is enabled. When auto-encrypt is enabled, the clients
-	// use a different CA than the servers (since they piggy-back on the Connect CA).
-	// In this case, we need to include the server CA in our federation secret
-	// because federation is server to server communication but we need to use
-	// the auto-encrypt CA when we talk to our local Consul client to get data
-	// like the dc name and mesh gateway addresses.
+	// Add server CA cert.
 	logger.Info("Retrieving server CA cert data")
 	caCert, err := ioutil.ReadFile(c.flagServerCACertFile)
 	if err != nil {
@@ -180,7 +157,7 @@ func (c *Command) Run(args []string) int {
 	federationSecret.Data[fedSecretCACertKey] = caCert
 	logger.Info("Server CA cert retrieved successfully")
 
-	// Add CA key.
+	// Add server CA key.
 	logger.Info("Retrieving server CA key data")
 	caKey, err := ioutil.ReadFile(c.flagServerCAKeyFile)
 	if err != nil {
@@ -279,6 +256,34 @@ func (c *Command) Run(args []string) int {
 	return 0
 }
 
+func (c *Command) validateFlags(args []string) error {
+	if err := c.flags.Parse(args); err != nil {
+		return err
+	}
+	if len(c.flags.Args()) > 0 {
+		return errors.New("should have no non-flag arguments")
+	}
+	if c.flagResourcePrefix == "" {
+		return errors.New("-resource-prefix must be set")
+	}
+	if c.flagK8sNamespace == "" {
+		return errors.New("-k8s-namespace must be set")
+	}
+	if c.flagServerCACertFile == "" {
+		return errors.New("-server-ca-cert-file must be set")
+	}
+	if c.flagServerCAKeyFile == "" {
+		return errors.New("-server-ca-key-file must be set")
+	}
+	if c.flagMeshGatewayServiceName == "" {
+		return errors.New("-mesh-gateway-service-name must be set")
+	}
+	if err := c.validateCAFileFlag(); err != nil {
+		return err
+	}
+	return nil
+}
+
 // replicationToken waits for the ACL replication token Kubernetes secret to
 // be created and then returns it.
 func (c *Command) replicationToken(logger hclog.Logger) ([]byte, error) {
@@ -299,8 +304,8 @@ func (c *Command) replicationToken(logger hclog.Logger) ([]byte, error) {
 			logger.Warn("secret not yet created, retrying", "secret", secretName, "ns", c.flagK8sNamespace)
 			return errors.New("")
 		} else if err != nil {
-			logger.Error("error retrieving secret", "secret", secretName, "ns", c.flagK8sNamespace, "err", err)
-			return err
+			unrecoverableErr = err
+			return nil
 		}
 		var ok bool
 		token, ok = secret.Data[common.ACLTokenSecretKey]
@@ -413,21 +418,12 @@ func (c *Command) consulDatacenter(logger hclog.Logger) string {
 // validateCAFileFlag returns an error if the -ca-file flag (or its env var
 // CONSUL_CACERT) isn't set or the file it points to can't be read.
 func (c *Command) validateCAFileFlag() error {
-	// In order to ensure the -ca-file or CONSUL_CACERT env var is set, we
-	// need to use the MergeOntoConfig function and then inspect the config struct
-	// because we can't access that flag directly from c.http.
-	consulCfg := &api.Config{
-		TLSConfig: api.TLSConfig{},
-	}
-	c.http.MergeOntoConfig(consulCfg)
-	caFile := consulCfg.TLSConfig.CAFile
-	if caFile == "" {
-		caFile = os.Getenv("CONSUL_CACERT")
-	}
-	if caFile == "" {
+	cfg := api.DefaultConfig()
+	c.http.MergeOntoConfig(cfg)
+	if cfg.TLSConfig.CAFile == "" {
 		return errors.New("-ca-file or CONSUL_CACERT must be set")
 	}
-	_, err := ioutil.ReadFile(caFile)
+	_, err := ioutil.ReadFile(cfg.TLSConfig.CAFile)
 	if err != nil {
 		return fmt.Errorf("error reading CA file: %s", err)
 	}