auto-config: Avoid the marshal/unmarshal cycle in auto-config

Use a LiteralConfig and return a config.Config from translate.

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.FileSource{
-		Name:   autoConfigFileName,
-		Format: "json",
-		Data:   ac.autoConfigData,
-	}
-
-	cfg, warnings, err := LoadConfig(ac.builderOpts, src, ac.overrides...)
+	cfg, warnings, err := LoadConfig(ac.builderOpts, ac.autoConfigSource, 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 {
-		return err
+	ac.autoConfigSource = config.LiteralSource{
+		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

@@ -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/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.
-//
-// If this function is meant to output an agent/config.Config then why does it output
-// a map[string]interface{}? The answer is that our config and command line option
-// parsing is messed up and it would require major changes to fix (we probably should
-// do them but not for the auto-config feature). To understand this we need to work
-// 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,
+func translateConfig(c *pbconfig.Config) config.Config {
+	result := config.Config{
+		Datacenter:        &c.Datacenter,
+		PrimaryDatacenter: &c.PrimaryDatacenter,
+		NodeName:          &c.NodeName,
+		// only output the SegmentName in the configuration if its non-empty
+		// this will avoid a warning later when parsing the persisted configuration
+		SegmentName: stringPtrOrNil(c.SegmentName),
 	}
 
-	// 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 != "" {
-		out["segment"] = c.SegmentName
-	}
-
-	// Translate Auto Encrypt settings
 	if a := c.AutoEncrypt; a != nil {
-		autoEncryptConfig := map[string]interface{}{
-			"tls":       a.TLS,
-			"allow_tls": a.AllowTLS,
+		result.AutoEncrypt = config.AutoEncrypt{
+			TLS:      &a.TLS,
+			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{}{
-			"enabled":                  a.Enabled,
-			"policy_ttl":               a.PolicyTTL,
-			"role_ttl":                 a.RoleTTL,
-			"token_ttl":                a.TokenTTL,
-			"down_policy":              a.DownPolicy,
-			"default_policy":           a.DefaultPolicy,
-			"enable_key_list_policy":   a.EnableKeyListPolicy,
-			"disabled_ttl":             a.DisabledTTL,
-			"enable_token_persistence": a.EnableTokenPersistence,
+		result.ACL = config.ACL{
+			Enabled:                &a.Enabled,
+			PolicyTTL:              &a.PolicyTTL,
+			RoleTTL:                &a.RoleTTL,
+			TokenTTL:               &a.TokenTTL,
+			DownPolicy:             &a.DownPolicy,
+			DefaultPolicy:          &a.DefaultPolicy,
+			EnableKeyListPolicy:    &a.EnableKeyListPolicy,
+			DisabledTTL:            &a.DisabledTTL,
+			EnableTokenPersistence: &a.EnableTokenPersistence,
 		}
 
 		if t := c.ACL.Tokens; t != nil {
-			var mspTokens []map[string]string
-
-			// create the slice of msp tokens if any
+			tokens := make([]config.ServiceProviderToken, 0, len(t.ManagedServiceProvider))
 			for _, mspToken := range t.ManagedServiceProvider {
-				mspTokens = append(mspTokens, map[string]string{
-					"accessor_id": mspToken.AccessorID,
-					"secret_id":   mspToken.SecretID,
+				tokens = append(tokens, config.ServiceProviderToken{
+					AccessorID: &mspToken.AccessorID,
+					SecretID:   &mspToken.SecretID,
 				})
 			}
 
-			tokenConfig := make(map[string]interface{})
-
-			if t.Master != "" {
-				tokenConfig["master"] = t.Master
+			result.ACL.Tokens = config.Tokens{
+				Master:                 stringPtrOrNil(t.Master),
+				Replication:            stringPtrOrNil(t.Replication),
+				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
-			out["encrypt_verify_incoming"] = e.VerifyIncoming
-			out["encrypt_verify_outgoing"] = e.VerifyOutgoing
+			result.EncryptKey = &e.Key
+			result.EncryptVerifyIncoming = &e.VerifyIncoming
+			result.EncryptVerifyOutgoing = &e.VerifyOutgoing
 		}
 	}
 
-	// Translate the Generic TLS settings
 	if t := c.TLS; t != nil {
-		out["verify_outgoing"] = t.VerifyOutgoing
-		out["verify_server_hostname"] = t.VerifyServerHostname
-		if t.MinVersion != "" {
-			out["tls_min_version"] = t.MinVersion
-		}
-		if t.CipherSuites != "" {
-			out["tls_cipher_suites"] = t.CipherSuites
-		}
-		out["tls_prefer_server_cipher_suites"] = t.PreferServerCipherSuites
+		result.VerifyOutgoing = &t.VerifyOutgoing
+		result.VerifyServerHostname = &t.VerifyServerHostname
+		result.TLSMinVersion = stringPtrOrNil(t.MinVersion)
+		result.TLSCipherSuites = stringPtrOrNil(t.CipherSuites)
+		result.TLSPreferServerCipherSuites = &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,15 +117,5 @@ func TestConfig_translateConfig(t *testing.T) {
 	}
 
 	translated := translateConfig(&original)
-	data, err := json.Marshal(translated)
-	require.NoError(t, err)
-
-	src := config.FileSource{
-		Name:   "test",
-		Format: "json",
-		Data:   string(data),
-	}
-	actual, _, err := src.Parse()
-	require.NoError(t, err)
-	require.Equal(t, expected, &actual)
+	require.Equal(t, expected, translated)
 }

agent/config/config.go

@@ -39,7 +39,6 @@ func (f FileSource) Source() string {
 
 // 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
 	}
@@ -83,6 +82,20 @@ func (f FileSource) Parse() (Config, mapstructure.Metadata, error) {
 	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,7 +12,7 @@ import (
 
 // DefaultSource is the default agent configuration.
 // This needs to be merged first in the head.
-// TODO: return a rawSource (no decoding) instead of a FileSource
+// TODO: return a LiteralSource (no decoding) instead of a FileSource
 func DefaultSource() Source {
 	cfg := consul.DefaultConfig()
 	serfLAN := cfg.SerfLANConfig.MemberlistConfig
@@ -129,7 +129,7 @@ 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
+// TODO: return a LiteralSource (no decoding) instead of a FileSource
 func DevSource() Source {
 	return FileSource{
 		Name:   "dev",
@@ -170,7 +170,7 @@ 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
+// TODO: return a LiteralSource (no decoding) instead of a FileSource
 func NonUserSource() Source {
 	return FileSource{
 		Name:   "non-user",
@@ -203,7 +203,7 @@ 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
+// TODO: return a LiteralSource (no decoding) instead of a FileSource
 func VersionSource(rev, ver, verPre string) Source {
 	return FileSource{
 		Name:   "version",
@@ -220,7 +220,7 @@ 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
+// TODO: return a LiteralSource (no decoding) instead of a FileSource
 func DefaultConsulSource() Source {
 	cfg := consul.DefaultConfig()
 	raft := cfg.RaftConfig
@@ -249,7 +249,7 @@ 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
+// TODO: return a LiteralSource (no decoding) instead of a FileSource
 func DevConsulSource() Source {
 	return FileSource{
 		Name:   "consul-dev",

agent/config/default_oss.go

@@ -5,7 +5,7 @@ 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
+// TODO: return a LiteralSource (no decoding) instead of a FileSource
 func DefaultEnterpriseSource() Source {
 	return FileSource{
 		Name:   "enterprise-defaults",
@@ -16,7 +16,7 @@ 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
+// TODO: return a LiteralSource (no decoding) instead of a FileSource
 func OverrideEnterpriseSource() Source {
 	return FileSource{
 		Name:   "enterprise-overrides",