From ce935cef55f7298e85843b3ca17ad13415e40e0b Mon Sep 17 00:00:00 2001 From: Frank Schroeder Date: Fri, 20 Oct 2017 12:02:05 +0200 Subject: [PATCH] config: return error on extra command line arguments (#3397) The `consul agent` command was ignoring extra command line arguments which can lead to confusion when the user has for example forgotten to add a dash in front of an argument or is not using an `=` when setting boolean flags to `true`. `-bootstrap true` is not the same as `-bootstrap=true`, for example. Since all command line flags are known and we don't expect unparsed arguments we can return an error. However, this may make it slightly more difficult in the future if we ever wanted to have these kinds of arguments. Fixes #3397 --- agent/config/builder.go | 6 ++++++ agent/config/flags.go | 12 ++---------- agent/config/flags_test.go | 15 ++++++++++----- agent/config/runtime_test.go | 6 +++++- command/agent/agent.go | 1 + command/agent/agent_test.go | 4 ++++ 6 files changed, 28 insertions(+), 16 deletions(-) diff --git a/agent/config/builder.go b/agent/config/builder.go index da5f4fb2b6..4458f0fe71 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -79,6 +79,12 @@ type Builder struct { // NewBuilder returns a new configuration builder based on the given command // line flags. func NewBuilder(flags Flags) (*Builder, error) { + // We expect all flags to be parsed and flags.Args to be empty. + // Therefore, we bail if we find unparsed args. + if len(flags.Args) > 0 { + return nil, fmt.Errorf("config: Unknown extra arguments: %v", flags.Args) + } + newSource := func(name string, v interface{}) Source { b, err := json.MarshalIndent(v, "", " ") if err != nil { diff --git a/agent/config/flags.go b/agent/config/flags.go index 739e289fa9..6e0d7e31cf 100644 --- a/agent/config/flags.go +++ b/agent/config/flags.go @@ -22,17 +22,9 @@ type Flags struct { // HCL contains an arbitrary config in hcl format. HCL []string -} -// ParseFlag parses the arguments into a Flags struct. -func ParseFlags(args []string) (Flags, error) { - var f Flags - fs := flag.NewFlagSet("agent", flag.ContinueOnError) - AddFlags(fs, &f) - if err := fs.Parse(args); err != nil { - return Flags{}, err - } - return f, nil + // Args contains the remaining unparsed flags. + Args []string } // AddFlags adds the command line flags for the agent. diff --git a/agent/config/flags_test.go b/agent/config/flags_test.go index 87aca32cc8..d7bb053701 100644 --- a/agent/config/flags_test.go +++ b/agent/config/flags_test.go @@ -1,6 +1,7 @@ package config import ( + "flag" "reflect" "strings" "testing" @@ -35,10 +36,6 @@ func TestParseFlags(t *testing.T) { args: []string{`-bootstrap=false`}, flags: Flags{Config: Config{Bootstrap: pBool(false)}}, }, - { - args: []string{`-bootstrap`, `true`}, - flags: Flags{Config: Config{Bootstrap: pBool(true)}}, - }, { args: []string{`-config-file`, `a`, `-config-dir`, `b`, `-config-file`, `c`, `-config-dir`, `d`}, flags: Flags{ConfigFiles: []string{"a", "b", "c", "d"}}, @@ -59,14 +56,22 @@ func TestParseFlags(t *testing.T) { args: []string{`-node-meta`, `a:b`, `-node-meta`, `c:d`}, flags: Flags{Config: Config{NodeMeta: map[string]string{"a": "b", "c": "d"}}}, }, + { + args: []string{`-bootstrap`, `true`}, + flags: Flags{Config: Config{Bootstrap: pBool(true)}, Args: []string{"true"}}, + }, } for _, tt := range tests { t.Run(strings.Join(tt.args, " "), func(t *testing.T) { - flags, err := ParseFlags(tt.args) + flags := Flags{} + fs := flag.NewFlagSet("", flag.ContinueOnError) + AddFlags(fs, &flags) + err := fs.Parse(tt.args) if got, want := err, tt.err; !reflect.DeepEqual(got, want) { t.Fatalf("got error %v want %v", got, want) } + flags.Args = fs.Args() if !verify.Values(t, "flag", flags, tt.flags) { t.FailNow() } diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index c82b959aac..9fcd608c8d 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -1886,10 +1886,14 @@ func testConfig(t *testing.T, tests []configTest, dataDir string) { t.Run(strings.Join(desc, ":"), func(t *testing.T) { // first parse the flags - flags, err := ParseFlags(tt.args) + flags := Flags{} + fs := flag.NewFlagSet("", flag.ContinueOnError) + AddFlags(fs, &flags) + err := fs.Parse(tt.args) if err != nil { t.Fatalf("ParseFlags failed: %s", err) } + flags.Args = fs.Args() // Then create a builder with the flags. b, err := NewBuilder(flags) diff --git a/command/agent/agent.go b/command/agent/agent.go index 44c60ff722..2528b84fda 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -90,6 +90,7 @@ func (c *cmd) readConfig() *config.RuntimeConfig { } return nil } + c.flagArgs.Args = c.flags.Args() b, err := config.NewBuilder(c.flagArgs) if err != nil { diff --git a/command/agent/agent_test.go b/command/agent/agent_test.go index 7a307c3eca..8ba836f26a 100644 --- a/command/agent/agent_test.go +++ b/command/agent/agent_test.go @@ -31,6 +31,10 @@ func TestConfigFail(t *testing.T) { args: []string{"agent", "-server", "-bind=10.0.0.1", "-datacenter="}, out: "==> datacenter cannot be empty\n", }, + { + args: []string{"agent", "-server", "-bind=10.0.0.1", "-datacenter=foo", "some-other-arg"}, + out: "==> config: Unknown extra arguments: [some-other-arg]\n", + }, { args: []string{"agent", "-server", "-bind=10.0.0.1"}, out: "==> data_dir cannot be empty\n",