config: Use config fields to warn about enterprise settings

It is no safe to assumes that the mapstructure keys will contain all the keys because some config can be specified
with command line flags or literals.

This change allows us to remove the json marshal/unmarshal cycle for command line flags, which will allow
us to remove all of the hcl/json struct tags on config fields.

agent/config/builder.go

@@ -316,8 +316,9 @@ func (b *Builder) Build() (rt RuntimeConfig, err error) {
 			return RuntimeConfig{}, fmt.Errorf("failed to parse %v: %s", s.Source(), unusedErr)
 		}
 
-		// for now this is a soft failure that will cause warnings but not actual problems
+		for _, err := range validateEnterpriseConfigKeys(&c2) {
-		b.validateEnterpriseConfigKeys(&c2, md.Keys)
+			b.warn("%s", err)
+		}
 
 		// if we have a single 'check' or 'service' we need to add them to the
 		// list of checks and services first since we cannot merge them
@@ -1845,10 +1846,20 @@ func (b *Builder) stringValWithDefault(v *string, defaultVal string) string {
 	return *v
 }
 
+// Deprecated: use the stringVal() function instead of this Builder method. This
+// method is being left here temporarily as there are many callers. It will be
+// removed in the future.
 func (b *Builder) stringVal(v *string) string {
 	return b.stringValWithDefault(v, "")
 }
 
+func stringVal(v *string) string {
+	if v == nil {
+		return ""
+	}
+	return *v
+}
+
 func (b *Builder) float64ValWithDefault(v *float64, defaultVal float64) float64 {
 	if v == nil {
 		return defaultVal

agent/config/builder_oss.go

@@ -4,47 +4,55 @@ package config
 
 import (
 	"fmt"
-
-	"github.com/hashicorp/go-multierror"
 )
 
-var (
+// validateEnterpriseConfig is a function to validate the enterprise specific
-	enterpriseConfigMap map[string]func(*Config) = map[string]func(c *Config){
+// configuration items after Parsing but before merging into the overall
-		"non_voting_server": func(c *Config) {
+// configuration. The original intent is to use it to ensure that we warn
-			// to maintain existing compatibility we don't nullify the value
+// for enterprise configurations used in OSS.
-		},
+func validateEnterpriseConfigKeys(config *Config) []error {
-		"read_replica": func(c *Config) {
+	var result []error
-			// to maintain existing compatibility we don't nullify the value
+	add := func(k string) {
-		},
+		result = append(result, enterpriseConfigKeyError{key: k})
-		"segment": func(c *Config) {
-			// to maintain existing compatibility we don't nullify the value
-		},
-		"segments": func(c *Config) {
-			// to maintain existing compatibility we don't nullify the value
-		},
-		"autopilot.redundancy_zone_tag": func(c *Config) {
-			// to maintain existing compatibility we don't nullify the value
-		},
-		"autopilot.upgrade_version_tag": func(c *Config) {
-			// to maintain existing compatibility we don't nullify the value
-		},
-		"autopilot.disable_upgrade_migration": func(c *Config) {
-			// to maintain existing compatibility we don't nullify the value
-		},
-		"dns_config.prefer_namespace": func(c *Config) {
-			c.DNS.PreferNamespace = nil
-		},
-		"acl.msp_disable_bootstrap": func(c *Config) {
-			c.ACL.MSPDisableBootstrap = nil
-		},
-		"acl.tokens.managed_service_provider": func(c *Config) {
-			c.ACL.Tokens.ManagedServiceProvider = nil
-		},
-		"audit": func(c *Config) {
-			c.Audit = nil
-		},
 	}
-)
+
+	if config.ReadReplica != nil {
+		add(`read_replica (or the deprecated non_voting_server)`)
+	}
+	if stringVal(config.SegmentName) != "" {
+		add("segment")
+	}
+	if len(config.Segments) > 0 {
+		add("segments")
+	}
+	if stringVal(config.Autopilot.RedundancyZoneTag) != "" {
+		add("autopilot.redundancy_zone_tag")
+	}
+	if stringVal(config.Autopilot.UpgradeVersionTag) != "" {
+		add("autopilot.upgrade_version_tag")
+	}
+	if config.Autopilot.DisableUpgradeMigration != nil {
+		add("autopilot.disable_upgrade_migration")
+	}
+	if config.DNS.PreferNamespace != nil {
+		add("dns_config.prefer_namespace")
+		config.DNS.PreferNamespace = nil
+	}
+	if config.ACL.MSPDisableBootstrap != nil {
+		add("acl.msp_disable_bootstrap")
+		config.ACL.MSPDisableBootstrap = nil
+	}
+	if len(config.ACL.Tokens.ManagedServiceProvider) > 0 {
+		add("acl.tokens.managed_service_provider")
+		config.ACL.Tokens.ManagedServiceProvider = nil
+	}
+	if config.Audit != nil {
+		add("audit")
+		config.Audit = nil
+	}
+
+	return result
+}
 
 type enterpriseConfigKeyError struct {
 	key string
@@ -61,23 +69,3 @@ func (*Builder) BuildEnterpriseRuntimeConfig(_ *RuntimeConfig, _ *Config) error
 func (*Builder) validateEnterpriseConfig(_ RuntimeConfig) error {
 	return nil
 }
-
-// validateEnterpriseConfig is a function to validate the enterprise specific
-// configuration items after Parsing but before merging into the overall
-// configuration. The original intent is to use it to ensure that we warn
-// for enterprise configurations used in OSS.
-func (b *Builder) validateEnterpriseConfigKeys(config *Config, keys []string) error {
-	var err error
-
-	for _, k := range keys {
-		if unset, ok := enterpriseConfigMap[k]; ok {
-			keyErr := enterpriseConfigKeyError{key: k}
-
-			b.warn(keyErr.Error())
-			err = multierror.Append(err, keyErr)
-			unset(config)
-		}
-	}
-
-	return err
-}

agent/config/builder_oss_test.go

@@ -5,15 +5,13 @@ package config
 import (
 	"testing"
 
-	"github.com/hashicorp/go-multierror"
 	"github.com/stretchr/testify/require"
 )
 
-func TestBuilder_validateEnterpriseConfigKeys(t *testing.T) {
+func TestValidateEnterpriseConfigKeys(t *testing.T) {
 	// ensure that all the enterprise configurations
 	type testCase struct {
 		config  Config
-		keys    []string
 		badKeys []string
 		check   func(t *testing.T, c *Config)
 	}
@@ -22,34 +20,22 @@ func TestBuilder_validateEnterpriseConfigKeys(t *testing.T) {
 	stringVal := "string"
 
 	cases := map[string]testCase{
-		"non_voting_server": {
-			config: Config{
-				ReadReplica: &boolVal,
-			},
-			keys:    []string{"non_voting_server"},
-			badKeys: []string{"non_voting_server"},
-		},
 		"read_replica": {
 			config: Config{
 				ReadReplica: &boolVal,
 			},
-			keys:    []string{"read_replica"},
+			badKeys: []string{"read_replica (or the deprecated non_voting_server)"},
-			badKeys: []string{"read_replica"},
 		},
 		"segment": {
 			config: Config{
 				SegmentName: &stringVal,
 			},
-			keys:    []string{"segment"},
 			badKeys: []string{"segment"},
 		},
 		"segments": {
 			config: Config{
-				Segments: []Segment{
+				Segments: []Segment{{Name: &stringVal}},
-					{Name: &stringVal},
-				},
 			},
-			keys:    []string{"segments"},
 			badKeys: []string{"segments"},
 		},
 		"autopilot.redundancy_zone_tag": {
@@ -58,7 +44,6 @@ func TestBuilder_validateEnterpriseConfigKeys(t *testing.T) {
 					RedundancyZoneTag: &stringVal,
 				},
 			},
-			keys:    []string{"autopilot.redundancy_zone_tag"},
 			badKeys: []string{"autopilot.redundancy_zone_tag"},
 		},
 		"autopilot.upgrade_version_tag": {
@@ -67,25 +52,18 @@ func TestBuilder_validateEnterpriseConfigKeys(t *testing.T) {
 					UpgradeVersionTag: &stringVal,
 				},
 			},
-			keys:    []string{"autopilot.upgrade_version_tag"},
 			badKeys: []string{"autopilot.upgrade_version_tag"},
 		},
 		"autopilot.disable_upgrade_migration": {
 			config: Config{
-				Autopilot: Autopilot{
+				Autopilot: Autopilot{DisableUpgradeMigration: &boolVal},
-					DisableUpgradeMigration: &boolVal,
-				},
 			},
-			keys:    []string{"autopilot.disable_upgrade_migration"},
 			badKeys: []string{"autopilot.disable_upgrade_migration"},
 		},
 		"dns_config.prefer_namespace": {
 			config: Config{
-				DNS: DNS{
+				DNS: DNS{PreferNamespace: &boolVal},
-					PreferNamespace: &boolVal,
-				},
 			},
-			keys:    []string{"dns_config.prefer_namespace"},
 			badKeys: []string{"dns_config.prefer_namespace"},
 			check: func(t *testing.T, c *Config) {
 				require.Nil(t, c.DNS.PreferNamespace)
@@ -93,11 +71,8 @@ func TestBuilder_validateEnterpriseConfigKeys(t *testing.T) {
 		},
 		"acl.msp_disable_bootstrap": {
 			config: Config{
-				ACL: ACL{
+				ACL: ACL{MSPDisableBootstrap: &boolVal},
-					MSPDisableBootstrap: &boolVal,
-				},
 			},
-			keys:    []string{"acl.msp_disable_bootstrap"},
 			badKeys: []string{"acl.msp_disable_bootstrap"},
 			check: func(t *testing.T, c *Config) {
 				require.Nil(t, c.ACL.MSPDisableBootstrap)
@@ -116,7 +91,6 @@ func TestBuilder_validateEnterpriseConfigKeys(t *testing.T) {
 					},
 				},
 			},
-			keys:    []string{"acl.tokens.managed_service_provider"},
 			badKeys: []string{"acl.tokens.managed_service_provider"},
 			check: func(t *testing.T, c *Config) {
 				require.Empty(t, c.ACL.Tokens.ManagedServiceProvider)
@@ -127,39 +101,28 @@ func TestBuilder_validateEnterpriseConfigKeys(t *testing.T) {
 			config: Config{
 				ReadReplica: &boolVal,
 				SegmentName: &stringVal,
+				ACL:         ACL{Tokens: Tokens{AgentMaster: &stringVal}},
 			},
-			keys:    []string{"non_voting_server", "read_replica", "segment", "acl.tokens.agent_master"},
+			badKeys: []string{"read_replica (or the deprecated non_voting_server)", "segment"},
-			badKeys: []string{"non_voting_server", "read_replica", "segment"},
 		},
 	}
 
 	for name, tcase := range cases {
 		t.Run(name, func(t *testing.T) {
-			b := &Builder{}
+			errs := validateEnterpriseConfigKeys(&tcase.config)
+			if len(tcase.badKeys) == 0 {
+				require.Len(t, errs, 0)
+				return
+			}
 
-			err := b.validateEnterpriseConfigKeys(&tcase.config, tcase.keys)
+			var expected []error
-			if len(tcase.badKeys) > 0 {
+			for _, k := range tcase.badKeys {
-				require.Error(t, err)
+				expected = append(expected, enterpriseConfigKeyError{key: k})
+			}
+			require.ElementsMatch(t, expected, errs)
 
-				multiErr, ok := err.(*multierror.Error)
+			if tcase.check != nil {
-				require.True(t, ok)
+				tcase.check(t, &tcase.config)
-
-				var badKeys []string
-				for _, e := range multiErr.Errors {
-					if keyErr, ok := e.(enterpriseConfigKeyError); ok {
-						badKeys = append(badKeys, keyErr.key)
-						require.Contains(t, b.Warnings, keyErr.Error())
-					}
-				}
-
-				require.ElementsMatch(t, tcase.badKeys, badKeys)
-
-				if tcase.check != nil {
-					tcase.check(t, &tcase.config)
-				}
-
-			} else {
-				require.NoError(t, err)
 			}
 		})
 	}

agent/config/default.go

@@ -4,10 +4,11 @@ import (
 	"fmt"
 	"strconv"
 
+	"github.com/hashicorp/raft"
+
 	"github.com/hashicorp/consul/agent/checks"
 	"github.com/hashicorp/consul/agent/consul"
 	"github.com/hashicorp/consul/version"
-	"github.com/hashicorp/raft"
 )
 
 // DefaultSource is the default agent configuration.

agent/config/runtime_oss_test.go

@@ -12,16 +12,17 @@ var entTokenConfigSanitize = `"EnterpriseConfig": {},`
 
 func entFullRuntimeConfig(rt *RuntimeConfig) {}
 
-var enterpriseReadReplicaWarnings = []string{enterpriseConfigKeyError{key: "read_replica"}.Error()}
+var enterpriseReadReplicaWarnings = []string{enterpriseConfigKeyError{key: "read_replica (or the deprecated non_voting_server)"}.Error()}
 
-var enterpriseConfigKeyWarnings []string
+var enterpriseConfigKeyWarnings = []string{
-
+	enterpriseConfigKeyError{key: "read_replica (or the deprecated non_voting_server)"}.Error(),
-func init() {
+	enterpriseConfigKeyError{key: "segment"}.Error(),
-	for k := range enterpriseConfigMap {
+	enterpriseConfigKeyError{key: "segments"}.Error(),
-		if k == "non_voting_server" {
+	enterpriseConfigKeyError{key: "autopilot.redundancy_zone_tag"}.Error(),
-			// this is an alias for "read_replica" so we shouldn't see it in warnings
+	enterpriseConfigKeyError{key: "autopilot.upgrade_version_tag"}.Error(),
-			continue
+	enterpriseConfigKeyError{key: "autopilot.disable_upgrade_migration"}.Error(),
-		}
+	enterpriseConfigKeyError{key: "dns_config.prefer_namespace"}.Error(),
-		enterpriseConfigKeyWarnings = append(enterpriseConfigKeyWarnings, enterpriseConfigKeyError{key: k}.Error())
+	enterpriseConfigKeyError{key: "acl.msp_disable_bootstrap"}.Error(),
-	}
+	enterpriseConfigKeyError{key: "acl.tokens.managed_service_provider"}.Error(),
+	enterpriseConfigKeyError{key: "audit"}.Error(),
 }