From 8a4d292c8eaade94913c21ea91191b5c583d21cc Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 14 Aug 2020 19:17:44 -0400 Subject: [PATCH] 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 | 42 +++++++++++++++++------------------- agent/config/runtime_test.go | 31 +++++++++++++------------- 2 files changed, 35 insertions(+), 38 deletions(-) diff --git a/agent/config/builder.go b/agent/config/builder.go index 489c5cb72c..4b11a2e337 100644 --- a/agent/config/builder.go +++ b/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 - options BuilderOpts + // devMode stores the value of the -dev flag, and enables development mode. + 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 - // is called. - Hostname func() (string, error) + // hostname is a shim for testing, allowing tests to specify a replacement + // for os.Hostname. + hostname func() (string, error) - // GetPrivateIPv4 and GetPublicIPv6 return suitable default addresses - // for cases when the user doesn't supply them. - GetPrivateIPv4 func() ([]*net.IPAddr, error) - GetPublicIPv6 func() ([]*net.IPAddr, error) + // getPrivateIPv4 and getPublicIPv6 are shims for testing, allowing tests to + // specify a replacement for ipaddr.GetPrivateIPv4 and ipaddr.GetPublicIPv6. + getPrivateIPv4 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 - 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 := make([]Source, 0, len(b.Head)+len(b.Sources)+len(b.Tail)) 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 } diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index 62ee09e9a5..f6976bcd34 100644 --- a/agent/config/runtime_test.go +++ b/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 - if b.Hostname == nil { - b.Hostname = func() (string, error) { return "nodex", nil } + b.hostname = tt.hostname + if b.hostname == 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.GetPublicIPv6 = publicv6 + b.getPrivateIPv4 = privatev4 + 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.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.hostname = b.hostname + 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 } 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.