Merge pull request #9452 from hashicorp/dnephin/config-tests-flags-and-edgecases

config: make TestBuilder_BuildAndValidate_ConfigFlagsAndEdgecases easier to work with

agent/config/builder.go

@@ -480,25 +480,7 @@ func (b *builder) Build() (rt RuntimeConfig, err error) {
 	advertiseAddr := makeIPAddr(b.expandFirstIP("advertise_addr", c.AdvertiseAddrLAN), bindAddr)
 
 	if ipaddr.IsAny(advertiseAddr) {
-
+		addrtyp, detect := advertiseAddrFunc(b.opts, advertiseAddr)
-		var addrtyp string
-		var detect func() ([]*net.IPAddr, error)
-		switch {
-		case ipaddr.IsAnyV4(advertiseAddr):
-			addrtyp = "private IPv4"
-			detect = b.opts.getPrivateIPv4
-			if detect == nil {
-				detect = ipaddr.GetPrivateIPv4
-			}
-
-		case ipaddr.IsAnyV6(advertiseAddr):
-			addrtyp = "public IPv6"
-			detect = b.opts.getPublicIPv6
-			if detect == nil {
-				detect = ipaddr.GetPublicIPv6
-			}
-		}
-
 		advertiseAddrs, err := detect()
 		if err != nil {
 			return RuntimeConfig{}, fmt.Errorf("Error detecting %s address: %s", addrtyp, err)
@@ -1150,6 +1132,27 @@ func (b *builder) Build() (rt RuntimeConfig, err error) {
 	return rt, nil
 }
 
+func advertiseAddrFunc(opts LoadOpts, advertiseAddr *net.IPAddr) (string, func() ([]*net.IPAddr, error)) {
+	switch {
+	case ipaddr.IsAnyV4(advertiseAddr):
+		fn := opts.getPrivateIPv4
+		if fn == nil {
+			fn = ipaddr.GetPrivateIPv4
+		}
+		return "private IPv4", fn
+
+	case ipaddr.IsAnyV6(advertiseAddr):
+		fn := opts.getPublicIPv6
+		if fn == nil {
+			fn = ipaddr.GetPublicIPv6
+		}
+		return "public IPv6", fn
+
+	default:
+		panic("unsupported net.IPAddr Type")
+	}
+}
+
 // reBasicName validates that a field contains only lower case alphanumerics,
 // underscore and dash and is non-empty.
 var reBasicName = regexp.MustCompile("^[a-z0-9_-]+$")

agent/config/builder_test.go

@@ -142,7 +142,7 @@ func TestBuilder_BuildAndValidate_NodeName(t *testing.T) {
 				DataDir:  pString("dir"),
 			},
 		})
-		patchBuilderShims(b)
+		patchLoadOptsShims(&b.opts)
 		require.NoError(t, err)
 		_, err = b.BuildAndValidate()
 		require.NoError(t, err)
@@ -175,15 +175,21 @@ func TestBuilder_BuildAndValidate_NodeName(t *testing.T) {
 	}
 }
 
-func patchBuilderShims(b *builder) {
+func patchLoadOptsShims(opts *LoadOpts) {
-	b.opts.hostname = func() (string, error) {
+	if opts.hostname == nil {
-		return "thehostname", nil
+		opts.hostname = func() (string, error) {
+			return "thehostname", nil
+		}
 	}
-	b.opts.getPrivateIPv4 = func() ([]*net.IPAddr, error) {
+	if opts.getPrivateIPv4 == nil {
-		return []*net.IPAddr{ipAddr("10.0.0.1")}, nil
+		opts.getPrivateIPv4 = func() ([]*net.IPAddr, error) {
+			return []*net.IPAddr{ipAddr("10.0.0.1")}, nil
+		}
 	}
-	b.opts.getPublicIPv6 = func() ([]*net.IPAddr, error) {
+	if opts.getPublicIPv6 == nil {
-		return []*net.IPAddr{ipAddr("dead:beef::1")}, nil
+		opts.getPublicIPv6 = func() ([]*net.IPAddr, error) {
+			return []*net.IPAddr{ipAddr("dead:beef::1")}, nil
+		}
 	}
 }
 

agent/config/segment_oss_test.go

@@ -3,6 +3,7 @@
 package config
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/hashicorp/consul/sdk/testutil"
@@ -11,16 +12,16 @@ import (
 func TestSegments(t *testing.T) {
 	dataDir := testutil.TempDir(t, "consul")
 
-	tests := []configTest{
+	tests := []testCase{
 		{
 			desc: "segment name not in OSS",
 			args: []string{
 				`-data-dir=` + dataDir,
 			},
-			json: []string{`{ "server": true, "segment": "a" }`},
+			json:        []string{`{ "server": true, "segment": "a" }`},
-			hcl:  []string{` server = true segment = "a" `},
+			hcl:         []string{` server = true segment = "a" `},
-			err:  `Network segments are not supported in this version of Consul`,
+			expectedErr: `Network segments are not supported in this version of Consul`,
-			warns: []string{
+			expectedWarnings: []string{
 				enterpriseConfigKeyError{key: "segment"}.Error(),
 			},
 		},
@@ -29,10 +30,10 @@ func TestSegments(t *testing.T) {
 			args: []string{
 				`-data-dir=` + dataDir,
 			},
-			json: []string{`{ "segments":[{ "name":"x" }] }`},
+			json:        []string{`{ "segments":[{ "name":"x" }] }`},
-			hcl:  []string{`segments = [{ name = "x" }]`},
+			hcl:         []string{`segments = [{ name = "x" }]`},
-			err:  `Port for segment "x" cannot be <= 0`,
+			expectedErr: `Port for segment "x" cannot be <= 0`,
-			warns: []string{
+			expectedWarnings: []string{
 				enterpriseConfigKeyError{key: "segments"}.Error(),
 			},
 		},
@@ -41,14 +42,19 @@ func TestSegments(t *testing.T) {
 			args: []string{
 				`-data-dir=` + dataDir,
 			},
-			json: []string{`{ "segments":[{ "name":"x", "port": 123 }] }`},
+			json:        []string{`{ "segments":[{ "name":"x", "port": 123 }] }`},
-			hcl:  []string{`segments = [{ name = "x" port = 123 }]`},
+			hcl:         []string{`segments = [{ name = "x" port = 123 }]`},
-			err:  `Network segments are not supported in this version of Consul`,
+			expectedErr: `Network segments are not supported in this version of Consul`,
-			warns: []string{
+			expectedWarnings: []string{
 				enterpriseConfigKeyError{key: "segments"}.Error(),
 			},
 		},
 	}
 
-	testConfig(t, tests, dataDir)
+	for _, tc := range tests {
+		for _, format := range []string{"json", "hcl"} {
+			name := fmt.Sprintf("%v_%v", tc.desc, format)
+			t.Run(name, tc.run(format, dataDir))
+		}
+	}
 }

contributing/checklist-adding-config-fields.md

@@ -39,11 +39,11 @@ There are four specific cases covered with increasing complexity:
  - [ ] **If** your new config field needed some validation as it's only valid in
    some cases or with some values (often true).
       - [ ] Add validation to Validate in `agent/config/builder.go`.
-      - [ ] Add a test case to the table test `TestConfigFlagsAndEdgeCases` in
+      - [ ] Add a test case to the table test `TestLoad_IntegrationWithFlags` in
         `agent/config/runtime_test.go`.
  - [ ] **If** your new config field needs a non-zero-value default.
       - [ ] Add that to `DefaultSource` in `agent/config/defaults.go`.
-      - [ ] Add a test case to the table test `TestConfigFlagsAndEdgeCases` in
+      - [ ] Add a test case to the table test `TestLoad_IntegrationWithFlags` in
         `agent/config/runtime_test.go`.
  - [ ] **If** your config should take effect on a reload/HUP.
       - [ ] Add necessary code to to trigger a safe (locked or atomic) update to
@@ -72,7 +72,7 @@ If the config field also needs a CLI flag, then follow these steps.
  - [ ] Add the new flag to `agent/config/flags.go`.
  - [ ] Add a test case to TestParseFlags in `agent/config/flag_test.go`.
  - [ ] Add a test case (or extend one if appropriate) to the table test
-   `TestConfigFlagsAndEdgeCases` in `agent/config/runtime_test.go` to ensure setting the
+   `TestLoad_IntegrationWithFlags` in `agent/config/runtime_test.go` to ensure setting the
    flag works.
  - [ ] Add flag (as well as config file) documentation to
    `website/source/docs/agent/options.html.md`.