Merge pull request #3598 from hashicorp/issue-3397-error-with-extra-flags

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

@@ -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 {

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.

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()
 			}

agent/config/segments_oss_test.go

@@ -16,7 +16,7 @@ func TestSegments(t *testing.T) {
 	tests := []configTest{
 		{
 			desc: "segment name not in OSS",
-			flags: []string{
+			args: []string{
 				`-data-dir=` + dataDir,
 			},
 			json: []string{`{ "server": true, "segment": "a" }`},
@@ -25,7 +25,7 @@ func TestSegments(t *testing.T) {
 		},
 		{
 			desc: "segment port must be set",
-			flags: []string{
+			args: []string{
 				`-data-dir=` + dataDir,
 			},
 			json: []string{`{ "segments":[{ "name":"x" }] }`},
@@ -34,7 +34,7 @@ func TestSegments(t *testing.T) {
 		},
 		{
 			desc: "segments not in OSS",
-			flags: []string{
+			args: []string{
 				`-data-dir=` + dataDir,
 			},
 			json: []string{`{ "segments":[{ "name":"x", "port": 123 }] }`},

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 {

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",