From 724a281e1c9302c75ff630d15a4b53e69a35167a Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 20 Nov 2020 17:43:32 -0500 Subject: [PATCH] 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 | 15 ++++- agent/config/builder_oss.go | 104 ++++++++++++++----------------- agent/config/builder_oss_test.go | 77 ++++++----------------- agent/config/default.go | 3 +- agent/config/runtime_oss_test.go | 23 +++---- 5 files changed, 93 insertions(+), 129 deletions(-) diff --git a/agent/config/builder.go b/agent/config/builder.go index e4faa4d2d4..932da22b53 100644 --- a/agent/config/builder.go +++ b/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 - b.validateEnterpriseConfigKeys(&c2, md.Keys) + for _, err := range validateEnterpriseConfigKeys(&c2) { + 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 diff --git a/agent/config/builder_oss.go b/agent/config/builder_oss.go index 82a037994b..9e68a54609 100644 --- a/agent/config/builder_oss.go +++ b/agent/config/builder_oss.go @@ -4,47 +4,55 @@ package config import ( "fmt" - - "github.com/hashicorp/go-multierror" ) -var ( - enterpriseConfigMap map[string]func(*Config) = map[string]func(c *Config){ - "non_voting_server": func(c *Config) { - // to maintain existing compatibility we don't nullify the value - }, - "read_replica": func(c *Config) { - // to maintain existing compatibility we don't nullify the value - }, - "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 - }, +// 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 validateEnterpriseConfigKeys(config *Config) []error { + var result []error + add := func(k string) { + result = append(result, enterpriseConfigKeyError{key: k}) } -) + + 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 -} diff --git a/agent/config/builder_oss_test.go b/agent/config/builder_oss_test.go index e94fec68a2..d03ef165b6 100644 --- a/agent/config/builder_oss_test.go +++ b/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"}, + badKeys: []string{"read_replica (or the deprecated non_voting_server)"}, }, "segment": { config: Config{ SegmentName: &stringVal, }, - keys: []string{"segment"}, badKeys: []string{"segment"}, }, "segments": { config: Config{ - Segments: []Segment{ - {Name: &stringVal}, - }, + Segments: []Segment{{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{ - DisableUpgradeMigration: &boolVal, - }, + Autopilot: Autopilot{DisableUpgradeMigration: &boolVal}, }, - keys: []string{"autopilot.disable_upgrade_migration"}, badKeys: []string{"autopilot.disable_upgrade_migration"}, }, "dns_config.prefer_namespace": { config: Config{ - DNS: DNS{ - PreferNamespace: &boolVal, - }, + DNS: DNS{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{ - MSPDisableBootstrap: &boolVal, - }, + ACL: ACL{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{"non_voting_server", "read_replica", "segment"}, + badKeys: []string{"read_replica (or the deprecated non_voting_server)", "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) - if len(tcase.badKeys) > 0 { - require.Error(t, err) + var expected []error + for _, k := range tcase.badKeys { + expected = append(expected, enterpriseConfigKeyError{key: k}) + } + require.ElementsMatch(t, expected, errs) - multiErr, ok := err.(*multierror.Error) - require.True(t, ok) - - 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) + if tcase.check != nil { + tcase.check(t, &tcase.config) } }) } diff --git a/agent/config/default.go b/agent/config/default.go index 72888250d6..0220399dfb 100644 --- a/agent/config/default.go +++ b/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. diff --git a/agent/config/runtime_oss_test.go b/agent/config/runtime_oss_test.go index e3b51568bf..059d8223ff 100644 --- a/agent/config/runtime_oss_test.go +++ b/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 - -func init() { - for k := range enterpriseConfigMap { - if k == "non_voting_server" { - // this is an alias for "read_replica" so we shouldn't see it in warnings - continue - } - enterpriseConfigKeyWarnings = append(enterpriseConfigKeyWarnings, enterpriseConfigKeyError{key: k}.Error()) - } +var enterpriseConfigKeyWarnings = []string{ + enterpriseConfigKeyError{key: "read_replica (or the deprecated non_voting_server)"}.Error(), + enterpriseConfigKeyError{key: "segment"}.Error(), + enterpriseConfigKeyError{key: "segments"}.Error(), + enterpriseConfigKeyError{key: "autopilot.redundancy_zone_tag"}.Error(), + enterpriseConfigKeyError{key: "autopilot.upgrade_version_tag"}.Error(), + enterpriseConfigKeyError{key: "autopilot.disable_upgrade_migration"}.Error(), + enterpriseConfigKeyError{key: "dns_config.prefer_namespace"}.Error(), + enterpriseConfigKeyError{key: "acl.msp_disable_bootstrap"}.Error(), + enterpriseConfigKeyError{key: "acl.tokens.managed_service_provider"}.Error(), + enterpriseConfigKeyError{key: "audit"}.Error(), }