From 32d36d0dd4dc712a612cb55ca5d4d0ddcdf04f58 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 21 Dec 2020 13:46:41 -0500 Subject: [PATCH] config: replace calls to config.NewBuilder with config.Load This is another incremental change to reduce config loading to a single small interface. All calls to NewBuilder can be replaced with Load. --- agent/config/agent_limits_test.go | 42 ------------------------------- agent/config/builder_test.go | 33 ++++++++++++++++++++++++ agent/config/default.go | 12 --------- agent/config/runtime_test.go | 4 +-- agent/dns_test.go | 25 ++++++++++++------ agent/local/state_test.go | 20 ++++++++++----- agent/testagent.go | 23 +++++++---------- command/services/config.go | 14 ++++------- command/services/config_test.go | 15 +++-------- command/validate/validate.go | 8 ++---- 10 files changed, 86 insertions(+), 110 deletions(-) delete mode 100644 agent/config/agent_limits_test.go diff --git a/agent/config/agent_limits_test.go b/agent/config/agent_limits_test.go deleted file mode 100644 index 1512f4e8ca..0000000000 --- a/agent/config/agent_limits_test.go +++ /dev/null @@ -1,42 +0,0 @@ -package config - -import ( - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestBuildAndValidate_HTTPMaxConnsPerClientExceedsRLimit(t *testing.T) { - hcl := ` - limits{ - # We put a very high value to be sure to fail - # This value is more than max on Windows as well - http_max_conns_per_client = 16777217 - }` - b, err := NewBuilder(LoadOpts{}) - assert.NoError(t, err) - testsrc := FileSource{ - Name: "test", - Format: "hcl", - Data: ` - ae_interval = "1m" - data_dir="/tmp/00000000001979" - bind_addr = "127.0.0.1" - advertise_addr = "127.0.0.1" - datacenter = "dc1" - bootstrap = true - server = true - node_id = "00000000001979" - node_name = "Node-00000000001979" - `, - } - b.Head = append(b.Head, testsrc) - b.Tail = append(b.Tail, DefaultConsulSource(), DevConsulSource()) - b.Tail = append(b.Head, FileSource{Name: "hcl", Format: "hcl", Data: hcl}) - - _, validationError := b.BuildAndValidate() - if validationError == nil { - assert.Fail(t, "Config should not be valid") - } - assert.Contains(t, validationError.Error(), "but limits.http_max_conns_per_client: 16777217 needs at least 16777237") -} diff --git a/agent/config/builder_test.go b/agent/config/builder_test.go index e038fbcdd5..52da30f540 100644 --- a/agent/config/builder_test.go +++ b/agent/config/builder_test.go @@ -10,6 +10,7 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -185,3 +186,35 @@ func patchBuilderShims(b *Builder) { return []*net.IPAddr{ipAddr("dead:beef::1")}, nil } } + +func TestLoad_HTTPMaxConnsPerClientExceedsRLimit(t *testing.T) { + hcl := ` + limits{ + # We put a very high value to be sure to fail + # This value is more than max on Windows as well + http_max_conns_per_client = 16777217 + }` + + opts := LoadOpts{ + DefaultConfig: FileSource{ + Name: "test", + Format: "hcl", + Data: ` + ae_interval = "1m" + data_dir="/tmp/00000000001979" + bind_addr = "127.0.0.1" + advertise_addr = "127.0.0.1" + datacenter = "dc1" + bootstrap = true + server = true + node_id = "00000000001979" + node_name = "Node-00000000001979" + `, + }, + HCL: []string{hcl}, + } + + _, err := Load(opts) + require.Error(t, err) + assert.Contains(t, err.Error(), "but limits.http_max_conns_per_client: 16777217 needs at least 16777237") +} diff --git a/agent/config/default.go b/agent/config/default.go index e416ec6312..d2c2a2e404 100644 --- a/agent/config/default.go +++ b/agent/config/default.go @@ -269,15 +269,3 @@ func DevConsulSource() Source { func strPtr(v string) *string { return &v } - -func DefaultRuntimeConfig(hcl string) *RuntimeConfig { - b, err := NewBuilder(LoadOpts{HCL: []string{hcl}}) - if err != nil { - panic(err) - } - rt, err := b.BuildAndValidate() - if err != nil { - panic(err) - } - return &rt -} diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index b42e8eb888..ed718a6b88 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -4996,8 +4996,8 @@ func assertDeepEqual(t *testing.T, x, y interface{}, opts ...cmp.Option) { } } -func TestNewBuilder_InvalidConfigFormat(t *testing.T) { - _, err := NewBuilder(LoadOpts{ConfigFormat: "yaml"}) +func TestLoad_InvalidConfigFormat(t *testing.T) { + _, err := Load(LoadOpts{ConfigFormat: "yaml"}) require.Error(t, err) require.Contains(t, err.Error(), "-config-format must be either 'hcl' or 'json'") } diff --git a/agent/dns_test.go b/agent/dns_test.go index 2b6ff25cba..fdc70118f2 100644 --- a/agent/dns_test.go +++ b/agent/dns_test.go @@ -10,6 +10,10 @@ import ( "testing" "time" + "github.com/hashicorp/serf/coordinate" + "github.com/miekg/dns" + "github.com/stretchr/testify/require" + "github.com/hashicorp/consul/agent/config" agentdns "github.com/hashicorp/consul/agent/dns" "github.com/hashicorp/consul/agent/structs" @@ -17,9 +21,6 @@ import ( "github.com/hashicorp/consul/lib" "github.com/hashicorp/consul/sdk/testutil/retry" "github.com/hashicorp/consul/testrpc" - "github.com/hashicorp/serf/coordinate" - "github.com/miekg/dns" - "github.com/stretchr/testify/require" ) const ( @@ -6664,7 +6665,7 @@ func TestDNS_trimUDPResponse_NoTrim(t *testing.T) { }, } - cfg := config.DefaultRuntimeConfig(`data_dir = "a" bind_addr = "127.0.0.1"`) + cfg := loadRuntimeConfig(t, `data_dir = "a" bind_addr = "127.0.0.1"`) if trimmed := trimUDPResponse(req, resp, cfg.DNSUDPAnswerLimit); trimmed { t.Fatalf("Bad %#v", *resp) } @@ -6698,7 +6699,7 @@ func TestDNS_trimUDPResponse_NoTrim(t *testing.T) { func TestDNS_trimUDPResponse_TrimLimit(t *testing.T) { t.Parallel() - cfg := config.DefaultRuntimeConfig(`data_dir = "a" bind_addr = "127.0.0.1"`) + cfg := loadRuntimeConfig(t, `data_dir = "a" bind_addr = "127.0.0.1"`) req, resp, expected := &dns.Msg{}, &dns.Msg{}, &dns.Msg{} for i := 0; i < cfg.DNSUDPAnswerLimit+1; i++ { @@ -6736,9 +6737,17 @@ func TestDNS_trimUDPResponse_TrimLimit(t *testing.T) { } } +func loadRuntimeConfig(t *testing.T, hcl string) *config.RuntimeConfig { + t.Helper() + result, err := config.Load(config.LoadOpts{HCL: []string{hcl}}) + require.NoError(t, err) + require.Len(t, result.Warnings, 0) + return result.RuntimeConfig +} + func TestDNS_trimUDPResponse_TrimSize(t *testing.T) { t.Parallel() - cfg := config.DefaultRuntimeConfig(`data_dir = "a" bind_addr = "127.0.0.1"`) + cfg := loadRuntimeConfig(t, `data_dir = "a" bind_addr = "127.0.0.1"`) req, resp := &dns.Msg{}, &dns.Msg{} for i := 0; i < 100; i++ { @@ -6791,7 +6800,7 @@ func TestDNS_trimUDPResponse_TrimSize(t *testing.T) { func TestDNS_trimUDPResponse_TrimSizeEDNS(t *testing.T) { t.Parallel() - cfg := config.DefaultRuntimeConfig(`data_dir = "a" bind_addr = "127.0.0.1"`) + cfg := loadRuntimeConfig(t, `data_dir = "a" bind_addr = "127.0.0.1"`) req, resp := &dns.Msg{}, &dns.Msg{} @@ -7094,7 +7103,7 @@ func TestDNS_syncExtra(t *testing.T) { func TestDNS_Compression_trimUDPResponse(t *testing.T) { t.Parallel() - cfg := config.DefaultRuntimeConfig(`data_dir = "a" bind_addr = "127.0.0.1"`) + cfg := loadRuntimeConfig(t, `data_dir = "a" bind_addr = "127.0.0.1"`) req, m := dns.Msg{}, dns.Msg{} trimUDPResponse(&req, &m, cfg.DNSUDPAnswerLimit) diff --git a/agent/local/state_test.go b/agent/local/state_test.go index 9e0a13630c..a8fc83be24 100644 --- a/agent/local/state_test.go +++ b/agent/local/state_test.go @@ -1773,7 +1773,7 @@ func TestAgent_ServiceTokens(t *testing.T) { tokens := new(token.Store) tokens.UpdateUserToken("default", token.TokenSourceConfig) - cfg := config.DefaultRuntimeConfig(`bind_addr = "127.0.0.1" data_dir = "dummy"`) + cfg := loadRuntimeConfig(t, `bind_addr = "127.0.0.1" data_dir = "dummy"`) l := local.NewState(agent.LocalConfig(cfg), nil, tokens) l.TriggerSyncChanges = func() {} @@ -1797,12 +1797,20 @@ func TestAgent_ServiceTokens(t *testing.T) { } } +func loadRuntimeConfig(t *testing.T, hcl string) *config.RuntimeConfig { + t.Helper() + result, err := config.Load(config.LoadOpts{HCL: []string{hcl}}) + require.NoError(t, err) + require.Len(t, result.Warnings, 0) + return result.RuntimeConfig +} + func TestAgent_CheckTokens(t *testing.T) { t.Parallel() tokens := new(token.Store) tokens.UpdateUserToken("default", token.TokenSourceConfig) - cfg := config.DefaultRuntimeConfig(`bind_addr = "127.0.0.1" data_dir = "dummy"`) + cfg := loadRuntimeConfig(t, `bind_addr = "127.0.0.1" data_dir = "dummy"`) l := local.NewState(agent.LocalConfig(cfg), nil, tokens) l.TriggerSyncChanges = func() {} @@ -1827,7 +1835,7 @@ func TestAgent_CheckTokens(t *testing.T) { func TestAgent_CheckCriticalTime(t *testing.T) { t.Parallel() - cfg := config.DefaultRuntimeConfig(`bind_addr = "127.0.0.1" data_dir = "dummy"`) + cfg := loadRuntimeConfig(t, `bind_addr = "127.0.0.1" data_dir = "dummy"`) l := local.NewState(agent.LocalConfig(cfg), nil, new(token.Store)) l.TriggerSyncChanges = func() {} @@ -1891,7 +1899,7 @@ func TestAgent_CheckCriticalTime(t *testing.T) { func TestAgent_AddCheckFailure(t *testing.T) { t.Parallel() - cfg := config.DefaultRuntimeConfig(`bind_addr = "127.0.0.1" data_dir = "dummy"`) + cfg := loadRuntimeConfig(t, `bind_addr = "127.0.0.1" data_dir = "dummy"`) l := local.NewState(agent.LocalConfig(cfg), nil, new(token.Store)) l.TriggerSyncChanges = func() {} @@ -1914,7 +1922,7 @@ func TestAgent_AliasCheck(t *testing.T) { t.Parallel() require := require.New(t) - cfg := config.DefaultRuntimeConfig(`bind_addr = "127.0.0.1" data_dir = "dummy"`) + cfg := loadRuntimeConfig(t, `bind_addr = "127.0.0.1" data_dir = "dummy"`) l := local.NewState(agent.LocalConfig(cfg), nil, new(token.Store)) l.TriggerSyncChanges = func() {} @@ -1965,7 +1973,7 @@ func TestAgent_AliasCheck_ServiceNotification(t *testing.T) { t.Parallel() require := require.New(t) - cfg := config.DefaultRuntimeConfig(`bind_addr = "127.0.0.1" data_dir = "dummy"`) + cfg := loadRuntimeConfig(t, `bind_addr = "127.0.0.1" data_dir = "dummy"`) l := local.NewState(agent.LocalConfig(cfg), nil, new(token.Store)) l.TriggerSyncChanges = func() {} diff --git a/agent/testagent.go b/agent/testagent.go index be44fca1f2..c58c6a6c5d 100644 --- a/agent/testagent.go +++ b/agent/testagent.go @@ -410,8 +410,7 @@ func NodeID() string { return id } -// TestConfig returns a unique default configuration for testing an -// agent. +// TestConfig returns a unique default configuration for testing an agent. func TestConfig(logger hclog.Logger, sources ...config.Source) *config.RuntimeConfig { nodeID := NodeID() testsrc := config.FileSource{ @@ -437,29 +436,25 @@ func TestConfig(logger hclog.Logger, sources ...config.Source) *config.RuntimeCo `, } - b, err := config.NewBuilder(config.LoadOpts{}) - if err != nil { - panic("NewBuilder failed: " + err.Error()) + opts := config.LoadOpts{ + DefaultConfig: testsrc, + Overrides: sources, } - b.Head = append(b.Head, testsrc) - b.Tail = append(b.Tail, config.DefaultConsulSource(), config.DevConsulSource()) - b.Tail = append(b.Tail, sources...) - - cfg, err := b.BuildAndValidate() + r, err := config.Load(opts) if err != nil { - panic("Error building config: " + err.Error()) + panic("config.Load failed: " + err.Error()) } - - for _, w := range b.Warnings { + for _, w := range r.Warnings { logger.Warn(w) } + cfg := r.RuntimeConfig // Effectively disables the delay after root rotation before requesting CSRs // to make test deterministic. 0 results in default jitter being applied but a // tiny delay is effectively thre same. cfg.ConnectTestCALeafRootChangeSpread = 1 * time.Nanosecond - return &cfg + return cfg } // TestACLConfig returns a default configuration for testing an agent diff --git a/command/services/config.go b/command/services/config.go index a12fc3d2f1..41341a379b 100644 --- a/command/services/config.go +++ b/command/services/config.go @@ -19,26 +19,22 @@ func ServicesFromFiles(ui cli.Ui, files []string) ([]*api.AgentServiceRegistrati // configuration. devMode doesn't set any services by default so this // is okay since we only look at services. devMode := true - b, err := config.NewBuilder(config.LoadOpts{ + r, err := config.Load(config.LoadOpts{ ConfigFiles: files, DevMode: &devMode, }) if err != nil { return nil, err } - - cfg, err := b.BuildAndValidate() - if err != nil { - return nil, err - } - for _, w := range b.Warnings { + for _, w := range r.Warnings { ui.Warn(w) } // The services are now in "structs.ServiceDefinition" form and we need // them in "api.AgentServiceRegistration" form so do the conversion. - result := make([]*api.AgentServiceRegistration, 0, len(cfg.Services)) - for _, svc := range cfg.Services { + services := r.RuntimeConfig.Services + result := make([]*api.AgentServiceRegistration, 0, len(services)) + for _, svc := range services { apiSvc, err := serviceToAgentService(svc) if err != nil { return nil, err diff --git a/command/services/config_test.go b/command/services/config_test.go index 7f31ac26fe..ccf0d36228 100644 --- a/command/services/config_test.go +++ b/command/services/config_test.go @@ -15,18 +15,11 @@ import ( // We depend on this behavior for ServiesFromFiles so we want to fail // tests if that ever changes. func TestDevModeHasNoServices(t *testing.T) { - t.Parallel() - require := require.New(t) - devMode := true - b, err := config.NewBuilder(config.LoadOpts{ - DevMode: &devMode, - }) - require.NoError(err) - - cfg, err := b.BuildAndValidate() - require.NoError(err) - require.Empty(cfg.Services) + result, err := config.Load(config.LoadOpts{DevMode: &devMode}) + require.NoError(t, err) + require.Len(t, result.Warnings, 0) + require.Len(t, result.RuntimeConfig.Services, 0) } func TestStructsToAgentService(t *testing.T) { diff --git a/command/validate/validate.go b/command/validate/validate.go index 40fecf713e..67370b792c 100644 --- a/command/validate/validate.go +++ b/command/validate/validate.go @@ -52,17 +52,13 @@ func (c *cmd) Run(args []string) int { return 1 } - b, err := config.NewBuilder(config.LoadOpts{ConfigFiles: configFiles, ConfigFormat: c.configFormat}) + result, err := config.Load(config.LoadOpts{ConfigFiles: configFiles, ConfigFormat: c.configFormat}) if err != nil { c.UI.Error(fmt.Sprintf("Config validation failed: %v", err.Error())) return 1 } - if _, err := b.BuildAndValidate(); err != nil { - c.UI.Error(fmt.Sprintf("Config validation failed: %v", err.Error())) - return 1 - } if !c.quiet { - for _, w := range b.Warnings { + for _, w := range result.Warnings { c.UI.Warn(w) } c.UI.Output("Configuration is valid!")