refactored config validation into a new method, modified Meta.Client

tests appropriately

command/agent/command.go

@@ -195,12 +195,6 @@ func (c *Command) readConfig() *Config {
 				c.Ui.Error(fmt.Sprintf("Error parsing Client.Meta value: %v", kv))
 				return nil
 			}
-
-			if !helper.IsValidInterpVariable(parts[0]) {
-				c.Ui.Error(fmt.Sprintf("Invalid Client.Meta key: %v", parts[0]))
-				return nil
-			}
-
 			cmdConfig.Client.Meta[parts[0]] = parts[1]
 		}
 	}
@@ -264,15 +258,6 @@ func (c *Command) readConfig() *Config {
 		}
 	}
 
-	// Set up the TLS configuration properly if we have one.
-	// XXX chelseakomlo: set up a TLSConfig New method which would wrap
-	// constructor-type actions like this.
-	if config.TLSConfig != nil && !config.TLSConfig.IsEmpty() {
-		if err := config.TLSConfig.SetChecksum(); err != nil {
-			c.Ui.Error(fmt.Sprintf("WARNING: Error when parsing TLS configuration: %v", err))
-		}
-	}
-
 	// Default the plugin directory to be under that of the data directory if it
 	// isn't explicitly specified.
 	if config.PluginDir == "" && config.DataDir != "" {
@@ -284,10 +269,28 @@ func (c *Command) readConfig() *Config {
 		return config
 	}
 
+	if !c.isValidConfig(config) {
+		return nil
+	}
+
+	return config
+}
+
+func (c *Command) isValidConfig(config *Config) bool {
+	// Set up the TLS configuration properly if we have one.
+	// XXX chelseakomlo: set up a TLSConfig New method which would wrap
+	// constructor-type actions like this.
+	if config.TLSConfig != nil && !config.TLSConfig.IsEmpty() {
+		if err := config.TLSConfig.SetChecksum(); err != nil {
+			c.Ui.Error(fmt.Sprintf("WARNING: Error when parsing TLS configuration: %v", err))
+			return false
+		}
+	}
+
 	if config.Server.EncryptKey != "" {
 		if _, err := config.Server.EncryptBytes(); err != nil {
 			c.Ui.Error(fmt.Sprintf("Invalid encryption key: %s", err))
-			return nil
+			return false
 		}
 		keyfile := filepath.Join(config.DataDir, serfKeyring)
 		if _, err := os.Stat(keyfile); err == nil {
@@ -298,7 +301,7 @@ func (c *Command) readConfig() *Config {
 	// Check that the server is running in at least one mode.
 	if !(config.Server.Enabled || config.Client.Enabled) {
 		c.Ui.Error("Must specify either server, client or dev mode for the agent.")
-		return nil
+		return false
 	}
 
 	// Verify the paths are absolute.
@@ -315,14 +318,14 @@ func (c *Command) readConfig() *Config {
 
 		if !filepath.IsAbs(dir) {
 			c.Ui.Error(fmt.Sprintf("%s must be given as an absolute path: got %v", k, dir))
-			return nil
+			return false
 		}
 	}
 
 	// Ensure that we have the directories we need to run.
 	if config.Server.Enabled && config.DataDir == "" {
 		c.Ui.Error("Must specify data directory")
-		return nil
+		return false
 	}
 
 	// The config is valid if the top-level data-dir is set or if both
@@ -330,20 +333,29 @@ func (c *Command) readConfig() *Config {
 	if config.Client.Enabled && config.DataDir == "" {
 		if config.Client.AllocDir == "" || config.Client.StateDir == "" || config.PluginDir == "" {
 			c.Ui.Error("Must specify the state, alloc dir, and plugin dir if data-dir is omitted.")
-			return nil
+			return false
+		}
+	}
+
+	if config.Client.Enabled {
+		for k := range config.Client.Meta {
+			if !helper.IsValidInterpVariable(k) {
+				c.Ui.Error(fmt.Sprintf("Invalid Client.Meta key: %v", k))
+				return false
+			}
 		}
 	}
 
 	// Check the bootstrap flags
 	if config.Server.BootstrapExpect > 0 && !config.Server.Enabled {
 		c.Ui.Error("Bootstrap requires server mode to be enabled")
-		return nil
+		return false
 	}
 	if config.Server.BootstrapExpect == 1 {
 		c.Ui.Error("WARNING: Bootstrap mode enabled! Potentially unsafe operation.")
 	}
 
-	return config
+	return true
 }
 
 // setupLoggers is used to setup the logGate, logWriter, and our logOutput

command/agent/command_test.go

@@ -3,6 +3,7 @@ package agent
 import (
 	"io/ioutil"
 	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 
@@ -88,3 +89,56 @@ func TestCommand_Args(t *testing.T) {
 		}
 	}
 }
+
+func TestCommand_MetaConfigValidation(t *testing.T) {
+	tmpDir, err := ioutil.TempDir("", "nomad")
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+	defer os.RemoveAll(tmpDir)
+
+	tcases := []string{
+		"foo..invalid",
+		".invalid",
+		"invalid.",
+	}
+	for _, tc := range tcases {
+		configFile := filepath.Join(tmpDir, "conf1.hcl")
+		err = ioutil.WriteFile(configFile, []byte(`client{
+			enabled = true
+			meta = {
+				"valid" = "yes"
+				"`+tc+`" = "kaboom!"
+				"nested.var" = "is nested"
+				"deeply.nested.var" = "is deeply nested"
+			}
+    	}`), 0600)
+		if err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		// Make a new command. We preemptively close the shutdownCh
+		// so that the command exits immediately instead of blocking.
+		ui := new(cli.MockUi)
+		shutdownCh := make(chan struct{})
+		close(shutdownCh)
+		cmd := &Command{
+			Version:    version.GetVersion(),
+			Ui:         ui,
+			ShutdownCh: shutdownCh,
+		}
+
+		// To prevent test failures on hosts whose hostname resolves to
+		// a loopback address, we must append a bind address
+		args := []string{"-client", "-data-dir=" + tmpDir, "-config=" + configFile, "-bind=169.254.0.1"}
+		if code := cmd.Run(args); code != 1 {
+			t.Fatalf("args: %v\nexit: %d\n", args, code)
+		}
+
+		expect := "Invalid Client.Meta key: " + tc
+		out := ui.ErrorWriter.String()
+		if !strings.Contains(out, expect) {
+			t.Fatalf("expect to find %q\n\n%s", expect, out)
+		}
+	}
+}

command/agent/config_parse.go

@@ -437,12 +437,6 @@ func parseClient(result **ClientConfig, list *ast.ObjectList) error {
 				return err
 			}
 		}
-
-		for k := range config.Meta {
-			if !helper.IsValidInterpVariable(k) {
-				return fmt.Errorf("invalid Client.Meta key: %v", k)
-			}
-		}
 	}
 
 	// Parse out chroot_env fields. These are in HCL as a list so we need to

command/agent/config_test.go

@@ -7,7 +7,6 @@ import (
 	"os"
 	"path/filepath"
 	"reflect"
-	"strings"
 	"testing"
 	"time"
 
@@ -419,30 +418,6 @@ func TestConfig_ParseConfigFile(t *testing.T) {
 	}
 }
 
-func TestConfig_ParseInvalidClientMeta(t *testing.T) {
-	tcases := []string{
-		"foo..invalid",
-		".invalid",
-		"invalid.",
-	}
-
-	for _, tc := range tcases {
-		reader := strings.NewReader(fmt.Sprintf(`client{
-			enabled = true
-			meta = {
-				"valid" = "yes"
-				"` + tc + `" = "kaboom!"
-				"nested.var" = "is nested"
-				"deeply.nested.var" = "is deeply nested"
-			}
-    	}`))
-
-		if _, err := ParseConfig(reader); err == nil {
-			t.Fatalf("expected load error, got nothing")
-		}
-	}
-}
-
 func TestConfig_LoadConfigDir(t *testing.T) {
 	// Fails if the dir doesn't exist.
 	if _, err := LoadConfigDir("/unicorns/leprechauns"); err == nil {