Merge pull request #5158 from hashicorp/f-1157-validate-node-meta-var…

…iables

added validation on client metadata keys

CHANGELOG.md

@@ -7,6 +7,9 @@ __BACKWARDS INCOMPATIBILITIES:__
  * client: Task config interpolation requires names to be valid identifiers
    (`node.region` or `NOMAD_DC`). Interpolating other variables requires a new
    indexing syntax: `env[".invalid.identifier."]`. [[GH-4843](https://github.com/hashicorp/nomad/issues/4843)]
+ * client: Node metadata variables must have valid identifiers, whether 
+   specified in the config file (`client.meta` stanza) or on the command line 
+   (`-meta`). [[GH-5158](https://github.com/hashicorp/nomad/pull/5158)]
 
 IMPROVEMENTS:
  * core: Added advertise address to client node meta data [[GH-4390](https://github.com/hashicorp/nomad/issues/4390)]

command/agent/command.go

@@ -15,17 +15,18 @@ import (
 	"syscall"
 	"time"
 
-	metrics "github.com/armon/go-metrics"
+	"github.com/armon/go-metrics"
 	"github.com/armon/go-metrics/circonus"
 	"github.com/armon/go-metrics/datadog"
 	"github.com/armon/go-metrics/prometheus"
 	"github.com/hashicorp/consul/lib"
-	checkpoint "github.com/hashicorp/go-checkpoint"
-	discover "github.com/hashicorp/go-discover"
-	gsyslog "github.com/hashicorp/go-syslog"
+	"github.com/hashicorp/go-checkpoint"
+	"github.com/hashicorp/go-discover"
+	"github.com/hashicorp/go-syslog"
 	"github.com/hashicorp/logutils"
-	flaghelper "github.com/hashicorp/nomad/helper/flag-helpers"
-	gatedwriter "github.com/hashicorp/nomad/helper/gated-writer"
+	"github.com/hashicorp/nomad/helper"
+	"github.com/hashicorp/nomad/helper/flag-helpers"
+	"github.com/hashicorp/nomad/helper/gated-writer"
 	"github.com/hashicorp/nomad/nomad/structs/config"
 	"github.com/hashicorp/nomad/version"
 	"github.com/mitchellh/cli"
@@ -193,7 +194,6 @@ func (c *Command) readConfig() *Config {
 				c.Ui.Error(fmt.Sprintf("Error parsing Client.Meta value: %v", kv))
 				return nil
 			}
-
 			cmdConfig.Client.Meta[parts[0]] = parts[1]
 		}
 	}
@@ -257,6 +257,27 @@ func (c *Command) readConfig() *Config {
 		}
 	}
 
+	// Default the plugin directory to be under that of the data directory if it
+	// isn't explicitly specified.
+	if config.PluginDir == "" && config.DataDir != "" {
+		config.PluginDir = filepath.Join(config.DataDir, "plugins")
+	}
+
+	if !c.isValidConfig(config) {
+		return nil
+	}
+
+	return config
+}
+
+func (c *Command) isValidConfig(config *Config) bool {
+
+	// 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 false
+	}
+
 	// 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.
@@ -266,34 +287,17 @@ func (c *Command) readConfig() *Config {
 		}
 	}
 
-	// Default the plugin directory to be under that of the data directory if it
-	// isn't explicitly specified.
-	if config.PluginDir == "" && config.DataDir != "" {
-		config.PluginDir = filepath.Join(config.DataDir, "plugins")
-	}
-
-	if dev {
-		// Skip validation for dev mode
-		return config
-	}
-
 	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 {
 			c.Ui.Warn("WARNING: keyring exists but -encrypt given, using keyring")
 		}
 	}
 
-	// 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
-	}
-
 	// Verify the paths are absolute.
 	dirs := map[string]string{
 		"data-dir":   config.DataDir,
@@ -308,35 +312,49 @@ 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
+		}
+	}
+
+	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
+			}
 		}
 	}
 
+	if config.DevMode {
+		// Skip the rest of the validation for dev mode
+		return true
+	}
+
 	// 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
 	// alloc-dir and state-dir are set.
 	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
 		}
 	}
 
 	// 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"
 
@@ -48,6 +49,18 @@ func TestCommand_Args(t *testing.T) {
 			[]string{"-client", "-alloc-dir="},
 			"Must specify the state, alloc dir, and plugin dir if data-dir is omitted.",
 		},
+		{
+			[]string{"-client", "-data-dir=" + tmpDir, "-meta=invalid..key=inaccessible-value"},
+			"Invalid Client.Meta key: invalid..key",
+		},
+		{
+			[]string{"-client", "-data-dir=" + tmpDir, "-meta=.invalid=inaccessible-value"},
+			"Invalid Client.Meta key: .invalid",
+		},
+		{
+			[]string{"-client", "-data-dir=" + tmpDir, "-meta=invalid.=inaccessible-value"},
+			"Invalid Client.Meta key: invalid.",
+		},
 	}
 	for _, tc := range tcases {
 		// Make a new command. We preemptively close the shutdownCh
@@ -76,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)
+		}
+	}
+}

helper/funcs.go

@@ -15,6 +15,11 @@ import (
 // validUUID is used to check if a given string looks like a UUID
 var validUUID = regexp.MustCompile(`(?i)^[\da-f]{8}-[\da-f]{4}-[\da-f]{4}-[\da-f]{4}-[\da-f]{12}$`)
 
+// validInterpVarKey matches valid dotted variable names for interpolation. The
+// string must begin with one or more non-dot characters which may be followed
+// by sequences containing a dot followed by a one or more non-dot characters.
+var validInterpVarKey = regexp.MustCompile(`^[^.]+(\.[^.]+)*$`)
+
 // IsUUID returns true if the given string is a valid UUID.
 func IsUUID(str string) bool {
 	const uuidLen = 36
@@ -25,6 +30,14 @@ func IsUUID(str string) bool {
 	return validUUID.MatchString(str)
 }
 
+// IsValidInterpVariable returns true if a valid dotted variable names for
+// interpolation. The string must begin with one or more non-dot characters
+// which may be followed by sequences containing a dot followed by a one or more
+// non-dot characters.
+func IsValidInterpVariable(str string) bool {
+	return validInterpVarKey.MatchString(str)
+}
+
 // HashUUID takes an input UUID and returns a hashed version of the UUID to
 // ensure it is well distributed.
 func HashUUID(input string) (output string, hashed bool) {