diff --git a/CHANGELOG.md b/CHANGELOG.md index fe5fbcf950d..63773d7499b 100644 --- a/CHANGELOG.md +++ b/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)] diff --git a/command/agent/command.go b/command/agent/command.go index c9cdab3dfc5..c30f46caf8c 100644 --- a/command/agent/command.go +++ b/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,21 +287,10 @@ 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 { @@ -288,12 +298,6 @@ 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 - } - // Verify the paths are absolute. dirs := map[string]string{ "data-dir": config.DataDir, @@ -308,14 +312,28 @@ 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 @@ -323,20 +341,20 @@ 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 } } // 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 diff --git a/command/agent/command_test.go b/command/agent/command_test.go index ee39d7d3f22..eed6bba82ed 100644 --- a/command/agent/command_test.go +++ b/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) + } + } +} diff --git a/helper/funcs.go b/helper/funcs.go index d3ccfd8d40d..b9a77db968c 100644 --- a/helper/funcs.go +++ b/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) {