From cdcc1db700bef473715cf6621a01b17fb4734e8f Mon Sep 17 00:00:00 2001 From: Chris Baker Date: Tue, 8 Jan 2019 15:07:36 +0000 Subject: [PATCH] refactored config validation into a new method, modified Meta.Client tests appropriately --- command/agent/command.go | 56 +++++++++++++++++++++-------------- command/agent/command_test.go | 54 +++++++++++++++++++++++++++++++++ command/agent/config_parse.go | 6 ---- command/agent/config_test.go | 25 ---------------- 4 files changed, 88 insertions(+), 53 deletions(-) diff --git a/command/agent/command.go b/command/agent/command.go index 5740f8e91..8420a9cc2 100644 --- a/command/agent/command.go +++ b/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 diff --git a/command/agent/command_test.go b/command/agent/command_test.go index 68de02c5b..eed6bba82 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" @@ -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) + } + } +} diff --git a/command/agent/config_parse.go b/command/agent/config_parse.go index ed8e09dbb..641a10c2c 100644 --- a/command/agent/config_parse.go +++ b/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 diff --git a/command/agent/config_test.go b/command/agent/config_test.go index 90bc613b0..4c0d97f6e 100644 --- a/command/agent/config_test.go +++ b/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 {