From 38980ebb4c1bd7f3e6b68be7401d7ae6e63a4d46 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 10 Aug 2020 12:46:28 -0400 Subject: [PATCH] config: Make Source an interface This will allow us to accept config from auto-config without needing to go through a serialziation cycle. --- agent/agent.go | 2 +- agent/agent_endpoint_test.go | 4 +- agent/agent_test.go | 8 +-- agent/auto-config/auto_config.go | 2 +- agent/auto-config/auto_config_test.go | 4 +- agent/auto-config/builder.go | 2 +- agent/auto-config/config_translate_test.go | 7 ++- agent/config/agent_limits_test.go | 4 +- agent/config/builder.go | 21 ++++---- agent/config/builder_test.go | 20 ++++---- agent/config/config.go | 57 ++++++++++++++++------ agent/config/default.go | 21 ++++---- agent/config/default_oss.go | 6 ++- agent/config/runtime_test.go | 16 +++--- agent/testagent.go | 4 +- agent/testagent_test.go | 2 +- 16 files changed, 108 insertions(+), 72 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 74440efd6e..385b5d8f44 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -433,7 +433,7 @@ func New(options ...AgentOption) (*Agent, error) { } // parse the configuration and handle the error/warnings - config, warnings, err := autoconf.LoadConfig(flat.builderOpts, config.Source{}, flat.overrides...) + config, warnings, err := autoconf.LoadConfig(flat.builderOpts, nil, flat.overrides...) if err != nil { return nil, err } diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index a2b96e9693..d7d8f94b41 100644 --- a/agent/agent_endpoint_test.go +++ b/agent/agent_endpoint_test.go @@ -1332,7 +1332,7 @@ func TestAgent_Reload(t *testing.T) { t.Fatal("missing redis service") } - cfg2 := TestConfig(testutil.Logger(t), config.Source{ + cfg2 := TestConfig(testutil.Logger(t), config.FileSource{ Name: "reload", Format: "hcl", Data: ` @@ -1466,7 +1466,7 @@ func TestAgent_ReloadDoesNotTriggerWatch(t *testing.T) { }) // Let's take almost the same config - cfg2 := TestConfig(testutil.Logger(t), config.Source{ + cfg2 := TestConfig(testutil.Logger(t), config.FileSource{ Name: "reload", Format: "hcl", Data: ` diff --git a/agent/agent_test.go b/agent/agent_test.go index 9b2cbb4e13..2865d5e0c6 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -3669,7 +3669,7 @@ func TestAgent_ReloadConfigOutgoingRPCConfig(t *testing.T) { key_file = "../test/key/ourdomain.key" verify_server_hostname = true ` - c := TestConfig(testutil.Logger(t), config.Source{Name: t.Name(), Format: "hcl", Data: hcl}) + c := TestConfig(testutil.Logger(t), config.FileSource{Name: t.Name(), Format: "hcl", Data: hcl}) require.NoError(t, a.reloadConfigInternal(c)) tlsConf = a.tlsConfigurator.OutgoingRPCConfig() require.False(t, tlsConf.InsecureSkipVerify) @@ -3699,7 +3699,7 @@ func TestAgent_ReloadConfigAndKeepChecksStatus(t *testing.T) { require.Equal(t, "passing", check.Status, "check %q is wrong", id) } - c := TestConfig(testutil.Logger(t), config.Source{Name: t.Name(), Format: "hcl", Data: hcl}) + c := TestConfig(testutil.Logger(t), config.FileSource{Name: t.Name(), Format: "hcl", Data: hcl}) require.NoError(t, a.reloadConfigInternal(c)) // After reload, should be passing directly (no critical state) for id, check := range a.State.Checks(nil) { @@ -3738,7 +3738,7 @@ func TestAgent_ReloadConfigIncomingRPCConfig(t *testing.T) { key_file = "../test/key/ourdomain.key" verify_server_hostname = true ` - c := TestConfig(testutil.Logger(t), config.Source{Name: t.Name(), Format: "hcl", Data: hcl}) + c := TestConfig(testutil.Logger(t), config.FileSource{Name: t.Name(), Format: "hcl", Data: hcl}) require.NoError(t, a.reloadConfigInternal(c)) tlsConf, err = tlsConf.GetConfigForClient(nil) require.NoError(t, err) @@ -3767,7 +3767,7 @@ func TestAgent_ReloadConfigTLSConfigFailure(t *testing.T) { data_dir = "` + dataDir + `" verify_incoming = true ` - c := TestConfig(testutil.Logger(t), config.Source{Name: t.Name(), Format: "hcl", Data: hcl}) + c := TestConfig(testutil.Logger(t), config.FileSource{Name: t.Name(), Format: "hcl", Data: hcl}) require.Error(t, a.reloadConfigInternal(c)) tlsConf, err := tlsConf.GetConfigForClient(nil) require.NoError(t, err) diff --git a/agent/auto-config/auto_config.go b/agent/auto-config/auto_config.go index bf2c348e93..9294e9aa4c 100644 --- a/agent/auto-config/auto_config.go +++ b/agent/auto-config/auto_config.go @@ -105,7 +105,7 @@ func New(config *Config) (*AutoConfig, error) { // ReadConfig will parse the current configuration and inject any // auto-config sources if present into the correct place in the parsing chain. func (ac *AutoConfig) ReadConfig() (*config.RuntimeConfig, error) { - src := config.Source{ + src := config.FileSource{ Name: autoConfigFileName, Format: "json", Data: ac.autoConfigData, diff --git a/agent/auto-config/auto_config_test.go b/agent/auto-config/auto_config_test.go index 4583d803b5..52625e3204 100644 --- a/agent/auto-config/auto_config_test.go +++ b/agent/auto-config/auto_config_test.go @@ -126,12 +126,12 @@ func TestLoadConfig(t *testing.T) { DevMode: &devMode, } - cfg, warnings, err := LoadConfig(builderOpts, config.Source{ + cfg, warnings, err := LoadConfig(builderOpts, config.FileSource{ Name: "test", Format: "hcl", Data: `node_name = "hobbiton"`, }, - config.Source{ + config.FileSource{ Name: "overrides", Format: "json", Data: `{"check_reap_interval": "1ms"}`, diff --git a/agent/auto-config/builder.go b/agent/auto-config/builder.go index 56feb268a3..05eaae8868 100644 --- a/agent/auto-config/builder.go +++ b/agent/auto-config/builder.go @@ -13,7 +13,7 @@ func LoadConfig(builderOpts config.BuilderOpts, extraHead config.Source, overrid return nil, nil, err } - if extraHead.Data != "" { + if extraHead != nil { b.Head = append(b.Head, extraHead) } diff --git a/agent/auto-config/config_translate_test.go b/agent/auto-config/config_translate_test.go index 6ce49474df..a7586c5c54 100644 --- a/agent/auto-config/config_translate_test.go +++ b/agent/auto-config/config_translate_test.go @@ -121,7 +121,12 @@ func TestConfig_translateConfig(t *testing.T) { data, err := json.Marshal(translated) require.NoError(t, err) - actual, _, err := config.Parse(string(data), "json") + src := config.FileSource{ + Name: "test", + Format: "json", + Data: string(data), + } + actual, _, err := src.Parse() require.NoError(t, err) require.Equal(t, expected, &actual) } diff --git a/agent/config/agent_limits_test.go b/agent/config/agent_limits_test.go index eb1b6fd348..4a91f09ea3 100644 --- a/agent/config/agent_limits_test.go +++ b/agent/config/agent_limits_test.go @@ -16,7 +16,7 @@ func TestBuildAndValidate_HTTPMaxConnsPerClientExceedsRLimit(t *testing.T) { }` b, err := NewBuilder(BuilderOpts{}) assert.NoError(t, err) - testsrc := Source{ + testsrc := FileSource{ Name: "test", Format: "hcl", Data: ` @@ -33,7 +33,7 @@ func TestBuildAndValidate_HTTPMaxConnsPerClientExceedsRLimit(t *testing.T) { } b.Head = append(b.Head, testsrc) b.Tail = append(b.Tail, DefaultConsulSource(), DevConsulSource()) - b.Tail = append(b.Head, Source{Name: "hcl", Format: "hcl", Data: hcl}) + b.Tail = append(b.Head, FileSource{Name: "hcl", Format: "hcl", Data: hcl}) _, validationError := b.BuildAndValidate() if validationError == nil { diff --git a/agent/config/builder.go b/agent/config/builder.go index 0cf85ebe09..4f8bb5d033 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -93,7 +93,7 @@ func NewBuilder(opts BuilderOpts) (*Builder, error) { if err != nil { panic(err) } - return Source{Name: name, Format: "json", Data: string(b)} + return FileSource{Name: name, Format: "json", Data: string(b)} } b := &Builder{ @@ -121,7 +121,7 @@ func NewBuilder(opts BuilderOpts) (*Builder, error) { } b.Tail = append(b.Tail, newSource("flags.values", values)) for i, s := range opts.HCL { - b.Tail = append(b.Tail, Source{ + b.Tail = append(b.Tail, FileSource{ Name: fmt.Sprintf("flags-%d.hcl", i), Format: "hcl", Data: s, @@ -207,12 +207,12 @@ func (b *Builder) sourcesFromPath(path string, format string) ([]Source, error) func newSourceFromFile(path string, format string) (Source, error) { data, err := ioutil.ReadFile(path) if err != nil { - return Source{}, fmt.Errorf("config: failed to read %s: %s", path, err) + return nil, fmt.Errorf("config: failed to read %s: %s", path, err) } if format == "" { format = formatFromFileExtension(path) } - return Source{Name: path, Data: string(data), Format: format}, nil + return FileSource{Name: path, Data: string(data), Format: format}, nil } // shouldParse file determines whether the file to be read is of a supported extension @@ -271,12 +271,13 @@ func (b *Builder) Build() (rt RuntimeConfig, err error) { // parse the config sources into a configuration var c Config for _, s := range srcs { - if s.Name == "" || s.Data == "" { + + c2, md, err := s.Parse() + switch { + case err == ErrNoData: continue - } - c2, md, err := Parse(s.Data, s.Format) - if err != nil { - return RuntimeConfig{}, fmt.Errorf("Error parsing %s: %s", s.Name, err) + case err != nil: + return RuntimeConfig{}, fmt.Errorf("failed to parse %v: %w", s.Source(), err) } var unusedErr error @@ -289,7 +290,7 @@ func (b *Builder) Build() (rt RuntimeConfig, err error) { } } if unusedErr != nil { - return RuntimeConfig{}, fmt.Errorf("Error parsing %s: %s", s.Name, unusedErr) + 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 diff --git a/agent/config/builder_test.go b/agent/config/builder_test.go index a2eddaf75d..2bdf643cbd 100644 --- a/agent/config/builder_test.go +++ b/agent/config/builder_test.go @@ -38,10 +38,10 @@ func TestNewBuilder_PopulatesSourcesFromConfigFiles(t *testing.T) { require.NoError(t, err) expected := []Source{ - {Name: paths[0], Format: "hcl", Data: "content a"}, - {Name: paths[1], Format: "json", Data: "content b"}, - {Name: filepath.Join(paths[3], "a.hcl"), Format: "hcl", Data: "content a"}, - {Name: filepath.Join(paths[3], "b.json"), Format: "json", Data: "content b"}, + FileSource{Name: paths[0], Format: "hcl", Data: "content a"}, + FileSource{Name: paths[1], Format: "json", Data: "content b"}, + FileSource{Name: filepath.Join(paths[3], "a.hcl"), Format: "hcl", Data: "content a"}, + FileSource{Name: filepath.Join(paths[3], "b.json"), Format: "json", Data: "content b"}, } require.Equal(t, expected, b.Sources) require.Len(t, b.Warnings, 2) @@ -54,12 +54,12 @@ func TestNewBuilder_PopulatesSourcesFromConfigFiles_WithConfigFormat(t *testing. require.NoError(t, err) expected := []Source{ - {Name: paths[0], Format: "hcl", Data: "content a"}, - {Name: paths[1], Format: "hcl", Data: "content b"}, - {Name: paths[2], Format: "hcl", Data: "content c"}, - {Name: filepath.Join(paths[3], "a.hcl"), Format: "hcl", Data: "content a"}, - {Name: filepath.Join(paths[3], "b.json"), Format: "hcl", Data: "content b"}, - {Name: filepath.Join(paths[3], "c.yaml"), Format: "hcl", Data: "content c"}, + FileSource{Name: paths[0], Format: "hcl", Data: "content a"}, + FileSource{Name: paths[1], Format: "hcl", Data: "content b"}, + FileSource{Name: paths[2], Format: "hcl", Data: "content c"}, + FileSource{Name: filepath.Join(paths[3], "a.hcl"), Format: "hcl", Data: "content a"}, + FileSource{Name: filepath.Join(paths[3], "b.json"), Format: "hcl", Data: "content b"}, + FileSource{Name: filepath.Join(paths[3], "c.yaml"), Format: "hcl", Data: "content c"}, } require.Equal(t, expected, b.Sources) } diff --git a/agent/config/config.go b/agent/config/config.go index eaa10d62a1..c0b8a67027 100644 --- a/agent/config/config.go +++ b/agent/config/config.go @@ -14,27 +14,52 @@ const ( SerfWANKeyring = "serf/remote.keyring" ) -type Source struct { +// Source parses configuration from some source. +type Source interface { + // Source returns an identifier for the Source that can be used in error message + Source() string + // Parse a configuration and return the result. + Parse() (Config, mapstructure.Metadata, error) +} + +// ErrNoData indicates to Builder.Build that the source contained no data, and +// it can be skipped. +var ErrNoData = fmt.Errorf("config source contained no data") + +// FileSource implements Source and parses a config from a file. +type FileSource struct { Name string Format string Data string } -// Parse parses a config fragment in either JSON or HCL format. -func Parse(data string, format string) (c Config, md mapstructure.Metadata, err error) { - var raw map[string]interface{} - switch format { - case "json": - err = json.Unmarshal([]byte(data), &raw) - case "hcl": - err = hcl.Decode(&raw, data) - default: - err = fmt.Errorf("invalid format: %s", format) - } - if err != nil { - return Config{}, mapstructure.Metadata{}, err +func (f FileSource) Source() string { + return f.Name +} + +// Parse a config file in either JSON or HCL format. +func (f FileSource) Parse() (Config, mapstructure.Metadata, error) { + // TODO: remove once rawSource is used instead of a FileSource with no data. + if f.Name == "" || f.Data == "" { + return Config{}, mapstructure.Metadata{}, ErrNoData } + var raw map[string]interface{} + var err error + var md mapstructure.Metadata + switch f.Format { + case "json": + err = json.Unmarshal([]byte(f.Data), &raw) + case "hcl": + err = hcl.Decode(&raw, f.Data) + default: + err = fmt.Errorf("invalid format: %s", f.Format) + } + if err != nil { + return Config{}, md, err + } + + var c Config d, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ DecodeHook: mapstructure.ComposeDecodeHookFunc( // decode.HookWeakDecodeFromSlice is only necessary when reading from @@ -49,10 +74,10 @@ func Parse(data string, format string) (c Config, md mapstructure.Metadata, err Result: &c, }) if err != nil { - return Config{}, mapstructure.Metadata{}, err + return Config{}, md, err } if err := d.Decode(raw); err != nil { - return Config{}, mapstructure.Metadata{}, err + return Config{}, md, err } return c, md, nil diff --git a/agent/config/default.go b/agent/config/default.go index 380cf31aef..cdbb32f0d2 100644 --- a/agent/config/default.go +++ b/agent/config/default.go @@ -12,9 +12,7 @@ import ( // DefaultSource is the default agent configuration. // This needs to be merged first in the head. -// todo(fs): The values are sourced from multiple sources. -// todo(fs): IMO, this should be the definitive default for all configurable values -// todo(fs): and whatever is in here should clobber every default value. Hence, no sourcing. +// TODO: return a rawSource (no decoding) instead of a FileSource func DefaultSource() Source { cfg := consul.DefaultConfig() serfLAN := cfg.SerfLANConfig.MemberlistConfig @@ -25,7 +23,7 @@ func DefaultSource() Source { // acl stanza for now we need to be able to detect the new entries not being set (not // just set to the defaults here) so that we can use the old entries. So the true // default still needs to reside in the original config values - return Source{ + return FileSource{ Name: "default", Format: "hcl", Data: ` @@ -131,8 +129,9 @@ func DefaultSource() Source { // DevSource is the additional default configuration for dev mode. // This should be merged in the head after the default configuration. +// TODO: return a rawSource (no decoding) instead of a FileSource func DevSource() Source { - return Source{ + return FileSource{ Name: "dev", Format: "hcl", Data: ` @@ -171,8 +170,9 @@ func DevSource() Source { // NonUserSource contains the values the user cannot configure. // This needs to be merged in the tail. +// TODO: return a rawSource (no decoding) instead of a FileSource func NonUserSource() Source { - return Source{ + return FileSource{ Name: "non-user", Format: "hcl", Data: ` @@ -203,8 +203,9 @@ func NonUserSource() Source { // 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 rawSource (no decoding) instead of a FileSource func VersionSource(rev, ver, verPre string) Source { - return Source{ + return FileSource{ Name: "version", Format: "hcl", Data: fmt.Sprintf(`revision = %q version = %q version_prerelease = %q`, rev, ver, verPre), @@ -219,10 +220,11 @@ func DefaultVersionSource() Source { // DefaultConsulSource returns the default configuration for the consul agent. // This should be merged in the tail since these values are not user configurable. +// TODO: return a rawSource (no decoding) instead of a FileSource func DefaultConsulSource() Source { cfg := consul.DefaultConfig() raft := cfg.RaftConfig - return Source{ + return FileSource{ Name: "consul", Format: "hcl", Data: ` @@ -247,8 +249,9 @@ 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 rawSource (no decoding) instead of a FileSource func DevConsulSource() Source { - return Source{ + return FileSource{ Name: "consul-dev", Format: "hcl", Data: ` diff --git a/agent/config/default_oss.go b/agent/config/default_oss.go index 1c675f3aee..fa991c4a49 100644 --- a/agent/config/default_oss.go +++ b/agent/config/default_oss.go @@ -5,8 +5,9 @@ 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 rawSource (no decoding) instead of a FileSource func DefaultEnterpriseSource() Source { - return Source{ + return FileSource{ Name: "enterprise-defaults", Format: "hcl", Data: ``, @@ -15,8 +16,9 @@ func DefaultEnterpriseSource() Source { // OverrideEnterpriseSource returns the consul agent configuration for the enterprise mode. // This should be merged in the tail after the DefaultConsulSource. +// TODO: return a rawSource (no decoding) instead of a FileSource func OverrideEnterpriseSource() Source { - return Source{ + return FileSource{ Name: "enterprise-overrides", Format: "hcl", Data: ``, diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index 448bb98a24..62ee09e9a5 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -1584,7 +1584,7 @@ func TestConfigFlagsAndEdgecases(t *testing.T) { args: []string{`-data-dir=` + dataDir}, json: []string{`this is not JSON`}, hcl: []string{`*** 0123 this is not HCL`}, - err: "Error parsing", + err: "failed to parse", }, { desc: "datacenter is lower-cased", @@ -4312,14 +4312,14 @@ func testConfig(t *testing.T, tests []configTest, dataDir string) { // read the source fragements for i, data := range srcs { - b.Sources = append(b.Sources, Source{ + b.Sources = append(b.Sources, FileSource{ Name: fmt.Sprintf("src-%d.%s", i, format), Format: format, Data: data, }) } for i, data := range tails { - b.Tail = append(b.Tail, Source{ + b.Tail = append(b.Tail, FileSource{ Name: fmt.Sprintf("tail-%d.%s", i, format), Format: format, Data: data, @@ -5727,7 +5727,7 @@ func TestFullConfig(t *testing.T) { tail := map[string][]Source{ "json": { - { + FileSource{ Name: "tail.non-user.json", Format: "json", Data: ` @@ -5746,7 +5746,7 @@ func TestFullConfig(t *testing.T) { "sync_coordinate_rate_target": 137.81 }`, }, - { + FileSource{ Name: "tail.consul.json", Format: "json", Data: ` @@ -5770,7 +5770,7 @@ func TestFullConfig(t *testing.T) { }, }, "hcl": { - { + FileSource{ Name: "tail.non-user.hcl", Format: "hcl", Data: ` @@ -5788,7 +5788,7 @@ func TestFullConfig(t *testing.T) { sync_coordinate_rate_target = 137.81 `, }, - { + FileSource{ Name: "tail.consul.hcl", Format: "hcl", Data: ` @@ -6525,7 +6525,7 @@ func TestFullConfig(t *testing.T) { if err != nil { t.Fatalf("NewBuilder: %s", err) } - b.Sources = append(b.Sources, Source{Name: "full." + format, Data: data, Format: format}) + 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")) diff --git a/agent/testagent.go b/agent/testagent.go index 1704e3a075..d68ae26ec6 100644 --- a/agent/testagent.go +++ b/agent/testagent.go @@ -212,7 +212,7 @@ func (a *TestAgent) Start(t *testing.T) (err error) { hclDataDir, }, }), - WithOverrides(config.Source{ + WithOverrides(config.FileSource{ Name: "test-overrides", Format: "hcl", Data: a.Overrides}, @@ -466,7 +466,7 @@ func NodeID() string { // agent. func TestConfig(logger hclog.Logger, sources ...config.Source) *config.RuntimeConfig { nodeID := NodeID() - testsrc := config.Source{ + testsrc := config.FileSource{ Name: "test", Format: "hcl", Data: ` diff --git a/agent/testagent_test.go b/agent/testagent_test.go index 12be557a39..39e91a2a69 100644 --- a/agent/testagent_test.go +++ b/agent/testagent_test.go @@ -13,7 +13,7 @@ func TestDefaultConfig(t *testing.T) { t.Run("", func(t *testing.T) { t.Parallel() var c config.Config - data := config.DefaultSource().Data + data := config.DefaultSource().(config.FileSource).Data hcl.Decode(&c, data) hcl.Decode(&c, data) hcl.Decode(&c, data)