Merge pull request #8469 from hashicorp/dnephin/config-source

config: make Source an interface to avoid the marshal/unmarshal cycle in auto-config

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
 	}

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: `

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)

agent/auto-config/auto_config.go

@@ -2,7 +2,6 @@ package autoconf
 
 import (
 	"context"
-	"encoding/json"
 	"fmt"
 	"io/ioutil"
 	"net"
@@ -63,7 +62,7 @@ type AutoConfig struct {
 	certMonitor        CertMonitor
 	config             *config.RuntimeConfig
 	autoConfigResponse *pbautoconf.AutoConfigResponse
-	autoConfigData     string
+	autoConfigSource   config.Source
 	cancel             context.CancelFunc
 }
 
@@ -105,13 +104,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{
+	cfg, warnings, err := LoadConfig(ac.builderOpts, ac.autoConfigSource, ac.overrides...)
-		Name:   autoConfigFileName,
-		Format: "json",
-		Data:   ac.autoConfigData,
-	}
-
-	cfg, warnings, err := LoadConfig(ac.builderOpts, src, ac.overrides...)
 	if err != nil {
 		return cfg, err
 	}
@@ -496,8 +489,9 @@ func (ac *AutoConfig) generateCSR() (csr string, key string, err error) {
 func (ac *AutoConfig) update(resp *pbautoconf.AutoConfigResponse) error {
 	ac.autoConfigResponse = resp
 
-	if err := ac.updateConfigFromResponse(resp); err != nil {
+	ac.autoConfigSource = config.LiteralSource{
-		return err
+		Name:   autoConfigFileName,
+		Config: translateConfig(resp.Config),
 	}
 
 	if err := ac.updateTLSFromResponse(resp); err != nil {
@@ -507,20 +501,6 @@ func (ac *AutoConfig) update(resp *pbautoconf.AutoConfigResponse) error {
 	return nil
 }
 
-// updateConfigFromResponse is responsible for generating the JSON compatible with the
-// agent/config.Config struct
-func (ac *AutoConfig) updateConfigFromResponse(resp *pbautoconf.AutoConfigResponse) error {
-	// here we want to serialize the translated configuration for use in injecting into the normal
-	// configuration parsing chain.
-	conf, err := json.Marshal(translateConfig(resp.Config))
-	if err != nil {
-		return fmt.Errorf("failed to encode auto-config configuration as JSON: %w", err)
-	}
-
-	ac.autoConfigData = string(conf)
-	return nil
-}
-
 // updateTLSFromResponse will update the TLS certificate and roots in the shared
 // TLS configurator.
 func (ac *AutoConfig) updateTLSFromResponse(resp *pbautoconf.AutoConfigResponse) error {

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"}`,
@@ -148,7 +148,10 @@ func TestReadConfig(t *testing.T) {
 	// just testing that some auto config source gets injected
 	devMode := true
 	ac := AutoConfig{
-		autoConfigData: `{"node_name": "hobbiton"}`,
+		autoConfigSource: config.LiteralSource{
+			Name:   autoConfigFileName,
+			Config: config.Config{NodeName: stringPointer("hobbiton")},
+		},
 		builderOpts: config.BuilderOpts{
 			// putting this in dev mode so that the config validates
 			// without having to specify a data directory

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)
 	}
 

agent/auto-config/config_translate.go

@@ -3,6 +3,7 @@ package autoconf
 import (
 	"fmt"
 
+	"github.com/hashicorp/consul/agent/config"
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/proto"
 	"github.com/hashicorp/consul/proto/pbautoconf"
@@ -19,151 +20,84 @@ import (
 //
 // Why is this function not in the proto/pbconfig package? The answer, that
 // package cannot import the agent/config package without running into import cycles.
-//
+func translateConfig(c *pbconfig.Config) config.Config {
-// If this function is meant to output an agent/config.Config then why does it output
+	result := config.Config{
-// a map[string]interface{}? The answer is that our config and command line option
+		Datacenter:        &c.Datacenter,
-// parsing is messed up and it would require major changes to fix (we probably should
+		PrimaryDatacenter: &c.PrimaryDatacenter,
-// do them but not for the auto-config feature). To understand this we need to work
+		NodeName:          &c.NodeName,
-// backwards. What we want to be able to do is persist the config settings from an
-// auto-config response configuration to disk. We then want that configuration
-// to be able to be parsed with the normal configuration parser/builder. It sort of was
-// working with returning a filled out agent/config.Config but the problem was that
-// the struct has a lot of non-pointer struct members. Thus, JSON serializtion caused
-// these to always be emitted even if they contained no non-empty fields. The
-// configuration would then seem to parse okay, but in OSS we would get warnings for
-// setting a bunch of enterprise fields like "audit" at the top level. In an attempt
-// to quiet those warnings, I had converted all the existing non-pointer struct fields
-// to pointers. Then there were issues with the builder code expecting concrete values.
-// I could add nil checks **EVERYWHERE** in builder.go or take a different approach.
-// I then made a function utilizing github.com/mitchellh/reflectwalk to un-nil all the
-// struct pointers after parsing to prevent any nil pointer dereferences. At first
-// glance this seemed like it was going to work but then I saw that nearly all of the
-// tests in runtime_test.go were failing. The first issues was that we were not merging
-// pointers to struct fields properly. It was simply taking the new pointer if non-nil
-// and defaulting to the original. So I updated that code, to properly merge pointers
-// to structs. That fixed a bunch of tests but then there was another issue with
-// the runtime tests where it was emitting warnings for using consul enterprise only
-// configuration. After spending some time tracking this down it turns out that it
-// was coming from our CLI option parsing. Our CLI option parsing works by filling
-// in a agent/config.Config struct. Along the way when converting to pointers to
-// structs I had to add a call to that function to un-nil various pointers to prevent
-// the CLI from segfaulting. However this un-nil operation was causing the various
-// enterprise only keys to be materialized. Thus we were back to where we were before
-// the conversion to pointers to structs and mostly stuck.
-//
-// Therefore, this function will create a map[string]interface{} that should be
-// compatible with the agent/config.Config struct but where we can more tightly
-// control which fields are output. Its not a nice solution. It has a non-trivial
-// maintenance burden. In the long run we should unify the protobuf Config and
-// the normal agent/config.Config so that we can just serialize the protobuf version
-// without any translation. For now, this hack is necessary :(
-func translateConfig(c *pbconfig.Config) map[string]interface{} {
-	out := map[string]interface{}{
-		"datacenter":         c.Datacenter,
-		"primary_datacenter": c.PrimaryDatacenter,
-		"node_name":          c.NodeName,
-	}
-
 		// only output the SegmentName in the configuration if its non-empty
 		// this will avoid a warning later when parsing the persisted configuration
-	if c.SegmentName != "" {
+		SegmentName: stringPtrOrNil(c.SegmentName),
-		out["segment"] = c.SegmentName
 	}
 
-	// Translate Auto Encrypt settings
 	if a := c.AutoEncrypt; a != nil {
-		autoEncryptConfig := map[string]interface{}{
+		result.AutoEncrypt = config.AutoEncrypt{
-			"tls":       a.TLS,
+			TLS:      &a.TLS,
-			"allow_tls": a.AllowTLS,
+			DNSSAN:   a.DNSSAN,
+			IPSAN:    a.IPSAN,
+			AllowTLS: &a.AllowTLS,
+		}
 	}
 
-		if len(a.DNSSAN) > 0 {
-			autoEncryptConfig["dns_san"] = a.DNSSAN
-		}
-		if len(a.IPSAN) > 0 {
-			autoEncryptConfig["ip_san"] = a.IPSAN
-		}
-
-		out["auto_encrypt"] = autoEncryptConfig
-	}
-
-	// Translate all the ACL settings
 	if a := c.ACL; a != nil {
-		aclConfig := map[string]interface{}{
+		result.ACL = config.ACL{
-			"enabled":                  a.Enabled,
+			Enabled:                &a.Enabled,
-			"policy_ttl":               a.PolicyTTL,
+			PolicyTTL:              &a.PolicyTTL,
-			"role_ttl":                 a.RoleTTL,
+			RoleTTL:                &a.RoleTTL,
-			"token_ttl":                a.TokenTTL,
+			TokenTTL:               &a.TokenTTL,
-			"down_policy":              a.DownPolicy,
+			DownPolicy:             &a.DownPolicy,
-			"default_policy":           a.DefaultPolicy,
+			DefaultPolicy:          &a.DefaultPolicy,
-			"enable_key_list_policy":   a.EnableKeyListPolicy,
+			EnableKeyListPolicy:    &a.EnableKeyListPolicy,
-			"disabled_ttl":             a.DisabledTTL,
+			DisabledTTL:            &a.DisabledTTL,
-			"enable_token_persistence": a.EnableTokenPersistence,
+			EnableTokenPersistence: &a.EnableTokenPersistence,
 		}
 
 		if t := c.ACL.Tokens; t != nil {
-			var mspTokens []map[string]string
+			tokens := make([]config.ServiceProviderToken, 0, len(t.ManagedServiceProvider))
-
-			// create the slice of msp tokens if any
 			for _, mspToken := range t.ManagedServiceProvider {
-				mspTokens = append(mspTokens, map[string]string{
+				tokens = append(tokens, config.ServiceProviderToken{
-					"accessor_id": mspToken.AccessorID,
+					AccessorID: &mspToken.AccessorID,
-					"secret_id":   mspToken.SecretID,
+					SecretID:   &mspToken.SecretID,
 				})
 			}
 
-			tokenConfig := make(map[string]interface{})
+			result.ACL.Tokens = config.Tokens{
-
+				Master:                 stringPtrOrNil(t.Master),
-			if t.Master != "" {
+				Replication:            stringPtrOrNil(t.Replication),
-				tokenConfig["master"] = t.Master
+				AgentMaster:            stringPtrOrNil(t.AgentMaster),
+				Default:                stringPtrOrNil(t.Default),
+				Agent:                  stringPtrOrNil(t.Agent),
+				ManagedServiceProvider: tokens,
 			}
-			if t.Replication != "" {
-				tokenConfig["replication"] = t.Replication
 		}
-			if t.AgentMaster != "" {
-				tokenConfig["agent_master"] = t.AgentMaster
-			}
-			if t.Default != "" {
-				tokenConfig["default"] = t.Default
-			}
-			if t.Agent != "" {
-				tokenConfig["agent"] = t.Agent
-			}
-			if len(mspTokens) > 0 {
-				tokenConfig["managed_service_provider"] = mspTokens
 	}
 
-			aclConfig["tokens"] = tokenConfig
-		}
-		out["acl"] = aclConfig
-	}
-
-	// Translate the Gossip settings
 	if g := c.Gossip; g != nil {
-		out["retry_join"] = g.RetryJoinLAN
+		result.RetryJoinLAN = g.RetryJoinLAN
 
-		// Translate the Gossip Encryption settings
 		if e := c.Gossip.Encryption; e != nil {
-			out["encrypt"] = e.Key
+			result.EncryptKey = &e.Key
-			out["encrypt_verify_incoming"] = e.VerifyIncoming
+			result.EncryptVerifyIncoming = &e.VerifyIncoming
-			out["encrypt_verify_outgoing"] = e.VerifyOutgoing
+			result.EncryptVerifyOutgoing = &e.VerifyOutgoing
 		}
 	}
 
-	// Translate the Generic TLS settings
 	if t := c.TLS; t != nil {
-		out["verify_outgoing"] = t.VerifyOutgoing
+		result.VerifyOutgoing = &t.VerifyOutgoing
-		out["verify_server_hostname"] = t.VerifyServerHostname
+		result.VerifyServerHostname = &t.VerifyServerHostname
-		if t.MinVersion != "" {
+		result.TLSMinVersion = stringPtrOrNil(t.MinVersion)
-			out["tls_min_version"] = t.MinVersion
+		result.TLSCipherSuites = stringPtrOrNil(t.CipherSuites)
-		}
+		result.TLSPreferServerCipherSuites = &t.PreferServerCipherSuites
-		if t.CipherSuites != "" {
-			out["tls_cipher_suites"] = t.CipherSuites
-		}
-		out["tls_prefer_server_cipher_suites"] = t.PreferServerCipherSuites
 	}
 
-	return out
+	return result
+}
+
+func stringPtrOrNil(v string) *string {
+	if v == "" {
+		return nil
+	}
+	return &v
 }
 
 func extractSignedResponse(resp *pbautoconf.AutoConfigResponse) (*structs.SignedResponse, error) {

agent/auto-config/config_translate_test.go

@@ -1,7 +1,6 @@
 package autoconf
 
 import (
-	"encoding/json"
 	"testing"
 
 	"github.com/hashicorp/consul/agent/config"
@@ -17,7 +16,7 @@ func boolPointer(b bool) *bool {
 	return &b
 }
 
-func TestConfig_translateConfig(t *testing.T) {
+func TestTranslateConfig(t *testing.T) {
 	original := pbconfig.Config{
 		Datacenter:        "abc",
 		PrimaryDatacenter: "def",
@@ -71,7 +70,7 @@ func TestConfig_translateConfig(t *testing.T) {
 		},
 	}
 
-	expected := &config.Config{
+	expected := config.Config{
 		Datacenter:                  stringPointer("abc"),
 		PrimaryDatacenter:           stringPointer("def"),
 		NodeName:                    stringPointer("ghi"),
@@ -118,10 +117,5 @@ func TestConfig_translateConfig(t *testing.T) {
 	}
 
 	translated := translateConfig(&original)
-	data, err := json.Marshal(translated)
+	require.Equal(t, expected, translated)
-	require.NoError(t, err)
-
-	actual, _, err := config.Parse(string(data), "json")
-	require.NoError(t, err)
-	require.Equal(t, expected, &actual)
 }

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 {

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
-		}
+		case err != nil:
-		c2, md, err := Parse(s.Data, s.Format)
+			return RuntimeConfig{}, fmt.Errorf("failed to parse %v: %w", s.Source(), err)
-		if err != nil {
-			return RuntimeConfig{}, fmt.Errorf("Error parsing %s: %s", s.Name, 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

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"},
+		FileSource{Name: paths[0], Format: "hcl", Data: "content a"},
-		{Name: paths[1], Format: "json", Data: "content b"},
+		FileSource{Name: paths[1], Format: "json", Data: "content b"},
-		{Name: filepath.Join(paths[3], "a.hcl"), Format: "hcl", Data: "content a"},
+		FileSource{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: 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"},
+		FileSource{Name: paths[0], Format: "hcl", Data: "content a"},
-		{Name: paths[1], Format: "hcl", Data: "content b"},
+		FileSource{Name: paths[1], Format: "hcl", Data: "content b"},
-		{Name: paths[2], Format: "hcl", Data: "content c"},
+		FileSource{Name: paths[2], Format: "hcl", Data: "content c"},
-		{Name: filepath.Join(paths[3], "a.hcl"), Format: "hcl", Data: "content a"},
+		FileSource{Name: filepath.Join(paths[3], "a.hcl"), Format: "hcl", Data: "content a"},
-		{Name: filepath.Join(paths[3], "b.json"), Format: "hcl", Data: "content b"},
+		FileSource{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: filepath.Join(paths[3], "c.yaml"), Format: "hcl", Data: "content c"},
 	}
 	require.Equal(t, expected, b.Sources)
 }

agent/config/config.go

@@ -14,27 +14,51 @@ 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 (f FileSource) Source() string {
-func Parse(data string, format string) (c Config, md mapstructure.Metadata, err error) {
+	return f.Name
-	var raw map[string]interface{}
+}
-	switch format {
+
-	case "json":
+// Parse a config file in either JSON or HCL format.
-		err = json.Unmarshal([]byte(data), &raw)
+func (f FileSource) Parse() (Config, mapstructure.Metadata, error) {
-	case "hcl":
+	if f.Name == "" || f.Data == "" {
-		err = hcl.Decode(&raw, data)
+		return Config{}, mapstructure.Metadata{}, ErrNoData
-	default:
-		err = fmt.Errorf("invalid format: %s", format)
-	}
-	if err != nil {
-		return Config{}, mapstructure.Metadata{}, err
 	}
 
+	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,15 +73,29 @@ 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
 }
 
+// LiteralSource implements Source and returns an existing Config struct.
+type LiteralSource struct {
+	Name   string
+	Config Config
+}
+
+func (l LiteralSource) Source() string {
+	return l.Name
+}
+
+func (l LiteralSource) Parse() (Config, mapstructure.Metadata, error) {
+	return l.Config, mapstructure.Metadata{}, nil
+}
+
 // Cache is the tunning configuration for cache, values are optional
 type Cache struct {
 	// EntryFetchMaxBurst max burst size of RateLimit for a single cache entry

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: return a LiteralSource (no decoding) instead of a FileSource
-// 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.
 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 LiteralSource (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 LiteralSource (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 LiteralSource (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 LiteralSource (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 LiteralSource (no decoding) instead of a FileSource
 func DevConsulSource() Source {
-	return Source{
+	return FileSource{
 		Name:   "consul-dev",
 		Format: "hcl",
 		Data: `

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 LiteralSource (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 LiteralSource (no decoding) instead of a FileSource
 func OverrideEnterpriseSource() Source {
-	return Source{
+	return FileSource{
 		Name:   "enterprise-overrides",
 		Format: "hcl",
 		Data:   ``,

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"))
 

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: `

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)