From fcbdfa393e98da6097325c2c992b25af983bd42c Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 20 Nov 2020 18:14:17 -0500 Subject: [PATCH 1/3] config: Use LiteralSource for some defaults Using the LiteralSource makes it much easier to find default values, because an IDE reports the location of a default. With an HCL string they are harder to discover. Also removes unnecessary mapstructure.Decodes of constant values. --- agent/config/builder.go | 2 +- agent/config/default.go | 57 ++++++++++++++++-------------------- agent/config/default_oss.go | 14 ++------- agent/config/runtime_test.go | 2 +- 4 files changed, 29 insertions(+), 46 deletions(-) diff --git a/agent/config/builder.go b/agent/config/builder.go index 48a2c55be..9ca6092ab 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -165,7 +165,7 @@ func NewBuilder(opts BuilderOpts) (*Builder, error) { Data: s, }) } - b.Tail = append(b.Tail, NonUserSource(), DefaultConsulSource(), OverrideEnterpriseSource(), DefaultVersionSource()) + b.Tail = append(b.Tail, NonUserSource(), DefaultConsulSource(), OverrideEnterpriseSource(), defaultVersionSource()) if b.boolVal(opts.DevMode) { b.Tail = append(b.Tail, DevConsulSource()) } diff --git a/agent/config/default.go b/agent/config/default.go index 72888250d..736ceeedb 100644 --- a/agent/config/default.go +++ b/agent/config/default.go @@ -1,13 +1,13 @@ package config 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. @@ -205,22 +205,24 @@ func NonUserSource() Source { } } -// VersionSource creates a config source for the version parameters. +// versionSource creates a config source for the version parameters. // This should be merged in the tail since these values are not // user configurable. -// TODO: return a LiteralSource (no decoding) instead of a FileSource -func VersionSource(rev, ver, verPre string) Source { - return FileSource{ - Name: "version", - Format: "hcl", - Data: fmt.Sprintf(`revision = %q version = %q version_prerelease = %q`, rev, ver, verPre), +func versionSource(rev, ver, verPre string) Source { + return LiteralSource{ + Name: "version", + Config: Config{ + Revision: &rev, + Version: &ver, + VersionPrerelease: &verPre, + }, } } -// DefaultVersionSource returns the version config source for the embedded +// defaultVersionSource returns the version config source for the embedded // version numbers. -func DefaultVersionSource() Source { - return VersionSource(version.GitCommit, version.Version, version.VersionPrerelease) +func defaultVersionSource() Source { + return versionSource(version.GitCommit, version.Version, version.VersionPrerelease) } // DefaultConsulSource returns the default configuration for the consul agent. @@ -254,27 +256,18 @@ func DefaultConsulSource() Source { // DevConsulSource returns the consul agent configuration for the dev mode. // This should be merged in the tail after the DefaultConsulSource. -// TODO: return a LiteralSource (no decoding) instead of a FileSource func DevConsulSource() Source { - return FileSource{ - Name: "consul-dev", - Format: "hcl", - Data: ` - consul = { - coordinate = { - update_period = "100ms" - } - raft = { - election_timeout = "52ms" - heartbeat_timeout = "35ms" - leader_lease_timeout = "20ms" - } - server = { - health_interval = "10ms" - } - } - `, - } + c := Config{} + c.Consul.Coordinate.UpdatePeriod = strPtr("100ms") + c.Consul.Raft.ElectionTimeout = strPtr("52ms") + c.Consul.Raft.HeartbeatTimeout = strPtr("35ms") + c.Consul.Raft.LeaderLeaseTimeout = strPtr("20ms") + c.Consul.Server.HealthInterval = strPtr("10ms") + return LiteralSource{Name: "consul-dev", Config: c} +} + +func strPtr(v string) *string { + return &v } func DefaultRuntimeConfig(hcl string) *RuntimeConfig { diff --git a/agent/config/default_oss.go b/agent/config/default_oss.go index 8f5d61aaa..57f52d927 100644 --- a/agent/config/default_oss.go +++ b/agent/config/default_oss.go @@ -5,22 +5,12 @@ package config // DefaultEnterpriseSource returns the consul agent configuration for enterprise mode. // These can be overridden by the user and therefore this source should be merged in the // head and processed before user configuration. -// TODO: return a LiteralSource (no decoding) instead of a FileSource func DefaultEnterpriseSource() Source { - return FileSource{ - Name: "enterprise-defaults", - Format: "hcl", - Data: ``, - } + return LiteralSource{Name: "enterprise-defaults"} } // OverrideEnterpriseSource returns the consul agent configuration for the enterprise mode. // This should be merged in the tail after the DefaultConsulSource. -// TODO: return a LiteralSource (no decoding) instead of a FileSource func OverrideEnterpriseSource() Source { - return FileSource{ - Name: "enterprise-overrides", - Format: "hcl", - Data: ``, - } + return LiteralSource{Name: "enterprise-overrides"} } diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index a989add2f..092e75b6a 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -7205,7 +7205,7 @@ func TestFullConfig(t *testing.T) { } b.Sources = append(b.Sources, FileSource{Name: "full." + format, Data: data, Format: format}) b.Tail = append(b.Tail, tail[format]...) - b.Tail = append(b.Tail, VersionSource("JNtPSav3", "R909Hblt", "ZT1JOQLn")) + b.Tail = append(b.Tail, versionSource("JNtPSav3", "R909Hblt", "ZT1JOQLn")) // construct the runtime config rt, err := b.Build() From 8e783fb37b8328e52b917296fafbd6110d644e24 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 20 Nov 2020 18:29:57 -0500 Subject: [PATCH 2/3] config: move testing shims to BuilderOpts And remove the devMode field from builder. This change helps make the Builder state more explicit by moving inputs to the BuilderOps struct, leaving only fields that can change during Builder.Build on the Builder struct. --- agent/config/builder.go | 24 +++++++----------------- agent/config/builder_test.go | 6 +++--- agent/config/flags.go | 10 ++++++++++ agent/config/runtime_test.go | 10 ++++------ 4 files changed, 24 insertions(+), 26 deletions(-) diff --git a/agent/config/builder.go b/agent/config/builder.go index 9ca6092ab..2eff8fbd8 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -92,8 +92,7 @@ func Load(opts BuilderOpts, extraHead Source, overrides ...Source) (*RuntimeConf // since not all pre-conditions have to be satisfied when performing // syntactical tests. type Builder struct { - // devMode stores the value of the -dev flag, and enables development mode. - devMode *bool + opts BuilderOpts // Head, Sources, and Tail are used to manage the order of the // config sources, as described in the comments above. @@ -105,15 +104,6 @@ type Builder struct { // parsing the configuration. Warnings []string - // hostname is a shim for testing, allowing tests to specify a replacement - // for os.Hostname. - hostname func() (string, 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. err error @@ -135,8 +125,8 @@ func NewBuilder(opts BuilderOpts) (*Builder, error) { } b := &Builder{ - devMode: opts.DevMode, - Head: []Source{DefaultSource(), DefaultEnterpriseSource()}, + opts: opts, + Head: []Source{DefaultSource(), DefaultEnterpriseSource()}, } if b.boolVal(opts.DevMode) { @@ -467,14 +457,14 @@ func (b *Builder) Build() (rt RuntimeConfig, err error) { switch { case ipaddr.IsAnyV4(advertiseAddr): addrtyp = "private IPv4" - detect = b.getPrivateIPv4 + detect = b.opts.getPrivateIPv4 if detect == nil { detect = ipaddr.GetPrivateIPv4 } case ipaddr.IsAnyV6(advertiseAddr): addrtyp = "public IPv6" - detect = b.getPublicIPv6 + detect = b.opts.getPublicIPv6 if detect == nil { detect = ipaddr.GetPublicIPv6 } @@ -994,7 +984,7 @@ func (b *Builder) Build() (rt RuntimeConfig, err error) { DataDir: dataDir, Datacenter: datacenter, DefaultQueryTime: b.durationVal("default_query_time", c.DefaultQueryTime), - DevMode: b.boolVal(b.devMode), + DevMode: b.boolVal(b.opts.DevMode), DisableAnonymousSignature: b.boolVal(c.DisableAnonymousSignature), DisableCoordinates: b.boolVal(c.DisableCoordinates), DisableHostNodeID: b.boolVal(c.DisableHostNodeID), @@ -1900,7 +1890,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.opts.hostname if fn == nil { fn = os.Hostname } diff --git a/agent/config/builder_test.go b/agent/config/builder_test.go index 9be12a4d3..430a959e8 100644 --- a/agent/config/builder_test.go +++ b/agent/config/builder_test.go @@ -172,13 +172,13 @@ func TestBuilder_BuildAndValidate_NodeName(t *testing.T) { } func patchBuilderShims(b *Builder) { - b.hostname = func() (string, error) { + b.opts.hostname = func() (string, error) { return "thehostname", nil } - b.getPrivateIPv4 = func() ([]*net.IPAddr, error) { + b.opts.getPrivateIPv4 = func() ([]*net.IPAddr, error) { return []*net.IPAddr{ipAddr("10.0.0.1")}, nil } - b.getPublicIPv6 = func() ([]*net.IPAddr, error) { + b.opts.getPublicIPv6 = func() ([]*net.IPAddr, error) { return []*net.IPAddr{ipAddr("dead:beef::1")}, nil } } diff --git a/agent/config/flags.go b/agent/config/flags.go index 77dc2abb3..155e24334 100644 --- a/agent/config/flags.go +++ b/agent/config/flags.go @@ -3,6 +3,7 @@ package config import ( "flag" "fmt" + "net" "time" ) @@ -26,6 +27,15 @@ type BuilderOpts struct { // HCL contains an arbitrary config in hcl format. HCL []string + + // hostname is a shim for testing, allowing tests to specify a replacement + // for os.Hostname. + hostname func() (string, 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) } // AddFlags adds the command line flags for the agent. diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index 092e75b6a..12307d641 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -4886,13 +4886,13 @@ func testConfig(t *testing.T, tests []configTest, dataDir string) { patchBuilderShims(b) if tt.hostname != nil { - b.hostname = tt.hostname + b.opts.hostname = tt.hostname } if tt.privatev4 != nil { - b.getPrivateIPv4 = tt.privatev4 + b.opts.getPrivateIPv4 = tt.privatev4 } if tt.publicv6 != nil { - b.getPublicIPv6 = tt.publicv6 + b.opts.getPublicIPv6 = tt.publicv6 } // read the source fragements @@ -4937,9 +4937,7 @@ 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 } + patchBuilderShims(x) expected, err := x.Build() if err != nil { t.Fatalf("build default failed: %s", err) From 1987a1eca08596f2b55213702113a6a8edcc5806 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 20 Nov 2020 19:17:12 -0500 Subject: [PATCH 3/3] config: remove unused const --- agent/config/config.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/agent/config/config.go b/agent/config/config.go index a6d4df843..40a1b3e0d 100644 --- a/agent/config/config.go +++ b/agent/config/config.go @@ -10,11 +10,6 @@ import ( "github.com/hashicorp/consul/lib/decode" ) -const ( - SerfLANKeyring = "serf/local.keyring" - SerfWANKeyring = "serf/remote.keyring" -) - // Source parses configuration from some source. type Source interface { // Source returns an identifier for the Source that can be used in error message