config: unexport and resolve TODOs in config.Builder

- unexport testing shims, and document their purpose
- resolve a TODO by moving validation to NewBuilder and storing the one
  field that is used instead of all of Options
- create a slice with the correct size to avoid extra allocations

agent/config/builder.go

@@ -84,8 +84,8 @@ func Load(builderOpts BuilderOpts, extraHead Source, overrides ...Source) (*Runt
 // since not all pre-conditions have to be satisfied when performing
 // syntactical tests.
 type Builder struct {
-	// options contains the input values used to construct a RuntimeConfig
+	// devMode stores the value of the -dev flag, and enables development mode.
-	options BuilderOpts
+	devMode *bool
 
 	// Head, Sources, and Tail are used to manage the order of the
 	// config sources, as described in the comments above.
@@ -97,14 +97,14 @@ type Builder struct {
 	// parsing the configuration.
 	Warnings []string
 
-	// Hostname returns the hostname of the machine. If nil, os.Hostname
+	// hostname is a shim for testing, allowing tests to specify a replacement
-	// is called.
+	// for os.Hostname.
-	Hostname func() (string, error)
+	hostname func() (string, error)
 
-	// GetPrivateIPv4 and GetPublicIPv6 return suitable default addresses
+	// getPrivateIPv4 and getPublicIPv6 are shims for testing, allowing tests to
-	// for cases when the user doesn't supply them.
+	// specify a replacement for ipaddr.GetPrivateIPv4 and ipaddr.GetPublicIPv6.
-	GetPrivateIPv4 func() ([]*net.IPAddr, error)
+	getPrivateIPv4 func() ([]*net.IPAddr, error)
-	GetPublicIPv6  func() ([]*net.IPAddr, error)
+	getPublicIPv6  func() ([]*net.IPAddr, error)
 
 	// err contains the first error that occurred during
 	// building the runtime configuration.
@@ -113,6 +113,11 @@ type Builder struct {
 
 // NewBuilder returns a new configuration Builder from the BuilderOpts.
 func NewBuilder(opts BuilderOpts) (*Builder, error) {
+	configFormat := opts.ConfigFormat
+	if configFormat != "" && configFormat != "json" && configFormat != "hcl" {
+		return nil, fmt.Errorf("config: -config-format must be either 'hcl' or 'json'")
+	}
+
 	newSource := func(name string, v interface{}) Source {
 		b, err := json.MarshalIndent(v, "", "    ")
 		if err != nil {
@@ -122,7 +127,7 @@ func NewBuilder(opts BuilderOpts) (*Builder, error) {
 	}
 
 	b := &Builder{
-		options: opts,
+		devMode: opts.DevMode,
 		Head:    []Source{DefaultSource(), DefaultEnterpriseSource()},
 	}
 
@@ -281,14 +286,7 @@ func (b *Builder) BuildAndValidate() (RuntimeConfig, error) {
 // warnings can still contain deprecation or format warnings that should
 // be presented to the user.
 func (b *Builder) Build() (rt RuntimeConfig, err error) {
-	// TODO: move to NewBuilder to remove Builder.options field
+	srcs := make([]Source, 0, len(b.Head)+len(b.Sources)+len(b.Tail))
-	configFormat := b.options.ConfigFormat
-	if configFormat != "" && configFormat != "json" && configFormat != "hcl" {
-		return RuntimeConfig{}, fmt.Errorf("config: -config-format must be either 'hcl' or 'json'")
-	}
-
-	// build the list of config sources
-	var srcs []Source
 	srcs = append(srcs, b.Head...)
 	srcs = append(srcs, b.Sources...)
 	srcs = append(srcs, b.Tail...)
@@ -461,14 +459,14 @@ func (b *Builder) Build() (rt RuntimeConfig, err error) {
 		switch {
 		case ipaddr.IsAnyV4(advertiseAddr):
 			addrtyp = "private IPv4"
-			detect = b.GetPrivateIPv4
+			detect = b.getPrivateIPv4
 			if detect == nil {
 				detect = ipaddr.GetPrivateIPv4
 			}
 
 		case ipaddr.IsAnyV6(advertiseAddr):
 			addrtyp = "public IPv6"
-			detect = b.GetPublicIPv6
+			detect = b.getPublicIPv6
 			if detect == nil {
 				detect = ipaddr.GetPublicIPv6
 			}
@@ -956,7 +954,7 @@ func (b *Builder) Build() (rt RuntimeConfig, err error) {
 		DataDir:                                b.stringVal(c.DataDir),
 		Datacenter:                             datacenter,
 		DefaultQueryTime:                       b.durationVal("default_query_time", c.DefaultQueryTime),
-		DevMode:                                b.boolVal(b.options.DevMode),
+		DevMode:                                b.boolVal(b.devMode),
 		DisableAnonymousSignature:              b.boolVal(c.DisableAnonymousSignature),
 		DisableCoordinates:                     b.boolVal(c.DisableCoordinates),
 		DisableHostNodeID:                      b.boolVal(c.DisableHostNodeID),
@@ -1744,7 +1742,7 @@ func (b *Builder) tlsCipherSuites(name string, v *string) []uint16 {
 func (b *Builder) nodeName(v *string) string {
 	nodeName := b.stringVal(v)
 	if nodeName == "" {
-		fn := b.Hostname
+		fn := b.hostname
 		if fn == nil {
 			fn = os.Hostname
 		}

agent/config/runtime_test.go

@@ -48,7 +48,7 @@ type configTest struct {
 // should check one option at a time if possible and should use generic
 // values, e.g. 'a' or 1 instead of 'servicex' or 3306.
 
-func TestConfigFlagsAndEdgecases(t *testing.T) {
+func TestBuilder_BuildAndValide_ConfigFlagsAndEdgecases(t *testing.T) {
 	dataDir := testutil.TempDir(t, "consul")
 	defer os.RemoveAll(dataDir)
 
@@ -490,13 +490,6 @@ func TestConfigFlagsAndEdgecases(t *testing.T) {
 				writeFile(filepath.Join(dataDir, "conf"), []byte(`datacenter = "a"`))
 			},
 		},
-		{
-			desc: "-config-format invalid",
-			args: []string{
-				`-config-format=foobar`,
-			},
-			err: "-config-format must be either 'hcl' or 'json'",
-		},
 		{
 			desc: "-http-port",
 			args: []string{
@@ -4289,9 +4282,9 @@ func testConfig(t *testing.T, tests []configTest, dataDir string) {
 				}
 
 				// mock the hostname function unless a mock is provided
-				b.Hostname = tt.hostname
+				b.hostname = tt.hostname
-				if b.Hostname == nil {
+				if b.hostname == nil {
-					b.Hostname = func() (string, error) { return "nodex", nil }
+					b.hostname = func() (string, error) { return "nodex", nil }
 				}
 
 				// mock the ip address detection
@@ -4307,8 +4300,8 @@ func testConfig(t *testing.T, tests []configTest, dataDir string) {
 						return []*net.IPAddr{ipAddr("dead:beef::1")}, nil
 					}
 				}
-				b.GetPrivateIPv4 = privatev4
+				b.getPrivateIPv4 = privatev4
-				b.GetPublicIPv6 = publicv6
+				b.getPublicIPv6 = publicv6
 
 				// read the source fragements
 				for i, data := range srcs {
@@ -4354,9 +4347,9 @@ func testConfig(t *testing.T, tests []configTest, dataDir string) {
 				if err != nil {
 					t.Fatal(err)
 				}
-				x.Hostname = b.Hostname
+				x.hostname = b.hostname
-				x.GetPrivateIPv4 = func() ([]*net.IPAddr, error) { return []*net.IPAddr{ipAddr("10.0.0.1")}, nil }
+				x.getPrivateIPv4 = func() ([]*net.IPAddr, error) { return []*net.IPAddr{ipAddr("10.0.0.1")}, nil }
-				x.GetPublicIPv6 = func() ([]*net.IPAddr, error) { return []*net.IPAddr{ipAddr("dead:beef::1")}, nil }
+				x.getPublicIPv6 = func() ([]*net.IPAddr, error) { return []*net.IPAddr{ipAddr("dead:beef::1")}, nil }
 				expected, err := x.Build()
 				if err != nil {
 					t.Fatalf("build default failed: %s", err)
@@ -4370,6 +4363,12 @@ func testConfig(t *testing.T, tests []configTest, dataDir string) {
 	}
 }
 
+func TestNewBuilder_InvalidConfigFormat(t *testing.T) {
+	_, err := NewBuilder(BuilderOpts{ConfigFormat: "yaml"})
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "-config-format must be either 'hcl' or 'json'")
+}
+
 // TestFullConfig tests the conversion from a fully populated JSON or
 // HCL config file to a RuntimeConfig structure. All fields must be set
 // to a unique non-zero value.