diff --git a/agent/auto-config/auto_config.go b/agent/auto-config/auto_config.go index 9294e9aa4c..b99e6e89b2 100644 --- a/agent/auto-config/auto_config.go +++ b/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 { diff --git a/agent/auto-config/auto_config_test.go b/agent/auto-config/auto_config_test.go index 52625e3204..f132c4b4f8 100644 --- a/agent/auto-config/auto_config_test.go +++ b/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 diff --git a/agent/auto-config/config_translate.go b/agent/auto-config/config_translate.go index ff02a63262..b7d04d5f61 100644 --- a/agent/auto-config/config_translate.go +++ b/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) { diff --git a/agent/auto-config/config_translate_test.go b/agent/auto-config/config_translate_test.go index a7586c5c54..18a4270a88 100644 --- a/agent/auto-config/config_translate_test.go +++ b/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) } diff --git a/agent/config/config.go b/agent/config/config.go index c0b8a67027..3e0710380b 100644 --- a/agent/config/config.go +++ b/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 diff --git a/agent/config/default.go b/agent/config/default.go index cdbb32f0d2..347bc09c28 100644 --- a/agent/config/default.go +++ b/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", diff --git a/agent/config/default_oss.go b/agent/config/default_oss.go index fa991c4a49..8f5d61aaaf 100644 --- a/agent/config/default_oss.go +++ b/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",