From 7b9bce2d083b82222086553b65bd40e750d00d8e Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Wed, 20 Mar 2024 10:18:56 -0400 Subject: [PATCH] config: fix `client.template` config merging with defaults (#20165) When loading the client configuration, the user-specified `client.template` block was not properly merged with the default values. As a result, if the user set any `client.template` field, all the other field defaulted to their zero values instead of the documented defaults. This changeset: * Adds the missing `Merge` method for the client template config and ensures it's called. * Makes a single source of truth for the default template configuration, instead of two different constructors. * Extends the tests to cover the merge of a partial block better. Fixes: https://github.com/hashicorp/nomad/issues/20164 --- .changelog/20165.txt | 3 + client/config/config.go | 102 +++++++++++++++------ command/agent/agent.go | 2 +- command/agent/config.go | 11 +-- command/agent/config_test.go | 166 +++++++++++++++++++++++++---------- 5 files changed, 201 insertions(+), 83 deletions(-) create mode 100644 .changelog/20165.txt diff --git a/.changelog/20165.txt b/.changelog/20165.txt new file mode 100644 index 000000000..4e01872fb --- /dev/null +++ b/.changelog/20165.txt @@ -0,0 +1,3 @@ +```release-note:bug +template: Fixed a bug where a partial `client.template` block would cause defaults for unspecified fields to be ignored +``` diff --git a/client/config/config.go b/client/config/config.go index 2b32e62d1..13db700b5 100644 --- a/client/config/config.go +++ b/client/config/config.go @@ -424,6 +424,28 @@ type ClientTemplateConfig struct { NomadRetry *RetryConfig `hcl:"nomad_retry,optional"` } +func DefaultTemplateConfig() *ClientTemplateConfig { + return &ClientTemplateConfig{ + FunctionDenylist: DefaultTemplateFunctionDenylist, + DisableSandbox: false, + BlockQueryWaitTime: pointer.Of(5 * time.Minute), // match Consul default + MaxStale: pointer.Of(DefaultTemplateMaxStale), // match Consul default + Wait: &WaitConfig{ + Min: pointer.Of(5 * time.Second), + Max: pointer.Of(4 * time.Minute), + }, + ConsulRetry: &RetryConfig{ + Attempts: pointer.Of(0), // unlimited + }, + VaultRetry: &RetryConfig{ + Attempts: pointer.Of(0), // unlimited + }, + NomadRetry: &RetryConfig{ + Attempts: pointer.Of(0), // unlimited + }, + } +} + // Copy returns a deep copy of a ClientTemplateConfig func (c *ClientTemplateConfig) Copy() *ClientTemplateConfig { if c == nil { @@ -485,6 +507,50 @@ func (c *ClientTemplateConfig) IsEmpty() bool { c.NomadRetry.IsEmpty() } +func (c *ClientTemplateConfig) Merge(o *ClientTemplateConfig) *ClientTemplateConfig { + if c == nil { + return o + } + + result := *c + if o == nil { + return &result + } + + if o.FunctionDenylist != nil { + result.FunctionDenylist = slices.Clone(o.FunctionDenylist) + } + if o.FunctionBlacklist != nil { + result.FunctionBlacklist = slices.Clone(o.FunctionBlacklist) + } + + if o.DisableSandbox { + result.DisableSandbox = true + } + + result.MaxStale = pointer.Merge(result.MaxStale, o.MaxStale) + result.BlockQueryWaitTime = pointer.Merge(result.BlockQueryWaitTime, o.BlockQueryWaitTime) + + if o.Wait != nil { + result.Wait = c.Wait.Merge(o.Wait) + } + if o.WaitBounds != nil { + result.WaitBounds = c.WaitBounds.Merge(o.WaitBounds) + } + + if o.ConsulRetry != nil { + result.ConsulRetry = c.ConsulRetry.Merge(o.ConsulRetry) + } + if o.VaultRetry != nil { + result.VaultRetry = c.VaultRetry.Merge(o.VaultRetry) + } + if o.NomadRetry != nil { + result.NomadRetry = c.NomadRetry.Merge(o.NomadRetry) + } + + return &result +} + // WaitConfig is mirrored from templateconfig.WaitConfig because we need to handle // the HCL conversion which happens in agent.ParseConfigFile // NOTE: Since Consul Template requires pointers, this type uses pointers to fields @@ -790,33 +856,15 @@ func DefaultConfig() *Config { GCMaxAllocs: 50, NoHostUUID: true, DisableRemoteExec: false, - TemplateConfig: &ClientTemplateConfig{ - FunctionDenylist: DefaultTemplateFunctionDenylist, - DisableSandbox: false, - BlockQueryWaitTime: pointer.Of(5 * time.Minute), // match Consul default - MaxStale: pointer.Of(DefaultTemplateMaxStale), // match Consul default - Wait: &WaitConfig{ - Min: pointer.Of(5 * time.Second), - Max: pointer.Of(4 * time.Minute), - }, - ConsulRetry: &RetryConfig{ - Attempts: pointer.Of(0), // unlimited - }, - VaultRetry: &RetryConfig{ - Attempts: pointer.Of(0), // unlimited - }, - NomadRetry: &RetryConfig{ - Attempts: pointer.Of(0), // unlimited - }, - }, - RPCHoldTimeout: 5 * time.Second, - CNIPath: "/opt/cni/bin", - CNIConfigDir: "/opt/cni/config", - CNIInterfacePrefix: "eth", - HostNetworks: map[string]*structs.ClientHostNetworkConfig{}, - CgroupParent: "nomad.slice", // SETH todo - MaxDynamicPort: structs.DefaultMinDynamicPort, - MinDynamicPort: structs.DefaultMaxDynamicPort, + TemplateConfig: DefaultTemplateConfig(), + RPCHoldTimeout: 5 * time.Second, + CNIPath: "/opt/cni/bin", + CNIConfigDir: "/opt/cni/config", + CNIInterfacePrefix: "eth", + HostNetworks: map[string]*structs.ClientHostNetworkConfig{}, + CgroupParent: "nomad.slice", // SETH todo + MaxDynamicPort: structs.DefaultMinDynamicPort, + MinDynamicPort: structs.DefaultMaxDynamicPort, Users: &UsersConfig{ MinDynamicUser: 80_000, MaxDynamicUser: 89_999, diff --git a/command/agent/agent.go b/command/agent/agent.go index 2a64b3c97..9e589fd50 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -766,7 +766,7 @@ func convertClientConfig(agentConfig *Config) (*clientconfig.Config, error) { conf.DisableRemoteExec = agentConfig.Client.DisableRemoteExec if agentConfig.Client.TemplateConfig != nil { - conf.TemplateConfig = agentConfig.Client.TemplateConfig.Copy() + conf.TemplateConfig = conf.TemplateConfig.Merge(agentConfig.Client.TemplateConfig) } hvMap := make(map[string]*structs.ClientHostVolumeConfig, len(agentConfig.Client.HostVolumes)) diff --git a/command/agent/config.go b/command/agent/config.go index c0b9b9187..3e09f3f94 100644 --- a/command/agent/config.go +++ b/command/agent/config.go @@ -1281,10 +1281,6 @@ func DevConfig(mode *devModeConfig) *Config { conf.Client.GCDiskUsageThreshold = 99 conf.Client.GCInodeUsageThreshold = 99 conf.Client.GCMaxAllocs = 50 - conf.Client.TemplateConfig = &client.ClientTemplateConfig{ - FunctionDenylist: client.DefaultTemplateFunctionDenylist, - DisableSandbox: false, - } conf.Client.Options[fingerprint.TightenNetworkTimeoutsConfig] = "true" conf.Client.BindWildcardDefaultHostNetwork = true conf.Client.NomadServiceDiscovery = pointer.Of(true) @@ -1353,10 +1349,7 @@ func DefaultConfig() *Config { RetryInterval: 30 * time.Second, RetryMaxAttempts: 0, }, - TemplateConfig: &client.ClientTemplateConfig{ - FunctionDenylist: client.DefaultTemplateFunctionDenylist, - DisableSandbox: false, - }, + TemplateConfig: client.DefaultTemplateConfig(), BindWildcardDefaultHostNetwork: true, CNIPath: "/opt/cni/bin", CNIConfigDir: "/opt/cni/config", @@ -2293,7 +2286,7 @@ func (a *ClientConfig) Merge(b *ClientConfig) *ClientConfig { } if b.TemplateConfig != nil { - result.TemplateConfig = b.TemplateConfig + result.TemplateConfig = result.TemplateConfig.Merge(b.TemplateConfig) } // Add the servers diff --git a/command/agent/config_test.go b/command/agent/config_test.go index 43c3189aa..6c8c1d463 100644 --- a/command/agent/config_test.go +++ b/command/agent/config_test.go @@ -323,8 +323,17 @@ func TestConfig_Merge(t *testing.T) { MaxKillTimeout: "50s", DisableRemoteExec: false, TemplateConfig: &client.ClientTemplateConfig{ - FunctionDenylist: client.DefaultTemplateFunctionDenylist, - DisableSandbox: false, + FunctionDenylist: client.DefaultTemplateFunctionDenylist, + DisableSandbox: false, + BlockQueryWaitTime: pointer.Of(5 * time.Minute), + MaxStale: pointer.Of(client.DefaultTemplateMaxStale), + Wait: &client.WaitConfig{ + Min: pointer.Of(5 * time.Second), + Max: pointer.Of(4 * time.Minute), + }, + ConsulRetry: &client.RetryConfig{Attempts: pointer.Of(0)}, + VaultRetry: &client.RetryConfig{Attempts: pointer.Of(0)}, + NomadRetry: &client.RetryConfig{Attempts: pointer.Of(0)}, }, Reserved: &Resources{ CPU: 15, @@ -1451,56 +1460,121 @@ func TestEventBroker_Parse(t *testing.T) { func TestConfig_LoadConsulTemplateConfig(t *testing.T) { ci.Parallel(t) - defaultConfig := DefaultConfig() - // Test that loading without template config didn't create load errors - agentConfig, err := LoadConfig("test-resources/minimal_client.hcl") - require.NoError(t, err) + t.Run("minimal client expect defaults", func(t *testing.T) { + defaultConfig := DefaultConfig() + agentConfig, err := LoadConfig("test-resources/minimal_client.hcl") + must.NoError(t, err) + agentConfig = defaultConfig.Merge(agentConfig) + must.Eq(t, defaultConfig.Client.TemplateConfig, agentConfig.Client.TemplateConfig) + }) - // Test loading with this config didn't create load errors - agentConfig, err = LoadConfig("test-resources/client_with_template.hcl") - require.NoError(t, err) + t.Run("client config with nil function denylist", func(t *testing.T) { + defaultConfig := DefaultConfig() + agentConfig, err := LoadConfig("test-resources/client_with_function_denylist_nil.hcl") + must.NoError(t, err) + agentConfig = defaultConfig.Merge(agentConfig) - agentConfig = defaultConfig.Merge(agentConfig) + templateConfig := agentConfig.Client.TemplateConfig + must.Len(t, 2, templateConfig.FunctionDenylist) + }) - clientAgent := Agent{config: agentConfig} - clientConfig, err := clientAgent.clientConfig() - require.NoError(t, err) + t.Run("client config with basic template", func(t *testing.T) { + defaultConfig := DefaultConfig() + agentConfig, err := LoadConfig("test-resources/client_with_basic_template.hcl") + must.NoError(t, err) + agentConfig = defaultConfig.Merge(agentConfig) - templateConfig := clientConfig.TemplateConfig + templateConfig := agentConfig.Client.TemplateConfig - // Make sure all fields to test are set - require.NotNil(t, templateConfig.BlockQueryWaitTime) - require.NotNil(t, templateConfig.MaxStale) - require.NotNil(t, templateConfig.Wait) - require.NotNil(t, templateConfig.WaitBounds) - require.NotNil(t, templateConfig.ConsulRetry) - require.NotNil(t, templateConfig.VaultRetry) - require.NotNil(t, templateConfig.NomadRetry) + // check explicit overrides + must.Eq(t, true, templateConfig.DisableSandbox) + must.Len(t, 0, templateConfig.FunctionDenylist) + + // check all the complex defaults + must.Eq(t, 87600*time.Hour, *templateConfig.MaxStale) + must.Eq(t, 5*time.Minute, *templateConfig.BlockQueryWaitTime) + + // Wait + must.NotNil(t, templateConfig.Wait) + must.Eq(t, 5*time.Second, *templateConfig.Wait.Min) + must.Eq(t, 4*time.Minute, *templateConfig.Wait.Max) + + // WaitBounds + must.Nil(t, templateConfig.WaitBounds) + + // Consul Retry + must.NotNil(t, templateConfig.ConsulRetry) + must.Eq(t, 0, *templateConfig.ConsulRetry.Attempts) + must.Nil(t, templateConfig.ConsulRetry.Backoff) + must.Nil(t, templateConfig.ConsulRetry.MaxBackoff) + + // Vault Retry + must.NotNil(t, templateConfig.VaultRetry) + must.Eq(t, 0, *templateConfig.VaultRetry.Attempts) + must.Nil(t, templateConfig.VaultRetry.Backoff) + must.Nil(t, templateConfig.VaultRetry.MaxBackoff) + + // Nomad Retry + must.NotNil(t, templateConfig.NomadRetry) + must.Eq(t, 0, *templateConfig.NomadRetry.Attempts) + must.Nil(t, templateConfig.NomadRetry.Backoff) + must.Nil(t, templateConfig.NomadRetry.MaxBackoff) + }) + + t.Run("client config with full template block", func(t *testing.T) { + defaultConfig := DefaultConfig() + + agentConfig, err := LoadConfig("test-resources/client_with_template.hcl") + must.NoError(t, err) + + agentConfig = defaultConfig.Merge(agentConfig) + + clientAgent := Agent{config: agentConfig} + clientConfig, err := clientAgent.clientConfig() + must.NoError(t, err) + + templateConfig := clientConfig.TemplateConfig + + // Make sure all fields to test are set + must.NotNil(t, templateConfig.BlockQueryWaitTime) + must.NotNil(t, templateConfig.MaxStale) + must.NotNil(t, templateConfig.Wait) + must.NotNil(t, templateConfig.WaitBounds) + must.NotNil(t, templateConfig.ConsulRetry) + must.NotNil(t, templateConfig.VaultRetry) + must.NotNil(t, templateConfig.NomadRetry) + + // Direct properties + must.Eq(t, 300*time.Second, *templateConfig.MaxStale) + must.Eq(t, 90*time.Second, *templateConfig.BlockQueryWaitTime) + + // Wait + must.Eq(t, 2*time.Second, *templateConfig.Wait.Min) + must.Eq(t, 60*time.Second, *templateConfig.Wait.Max) + + // WaitBounds + must.Eq(t, 2*time.Second, *templateConfig.WaitBounds.Min) + must.Eq(t, 60*time.Second, *templateConfig.WaitBounds.Max) + + // Consul Retry + must.NotNil(t, templateConfig.ConsulRetry) + must.Eq(t, 5, *templateConfig.ConsulRetry.Attempts) + must.Eq(t, 5*time.Second, *templateConfig.ConsulRetry.Backoff) + must.Eq(t, 10*time.Second, *templateConfig.ConsulRetry.MaxBackoff) + + // Vault Retry + must.NotNil(t, templateConfig.VaultRetry) + must.Eq(t, 10, *templateConfig.VaultRetry.Attempts) + must.Eq(t, 15*time.Second, *templateConfig.VaultRetry.Backoff) + must.Eq(t, 20*time.Second, *templateConfig.VaultRetry.MaxBackoff) + + // Nomad Retry + must.NotNil(t, templateConfig.NomadRetry) + must.Eq(t, 15, *templateConfig.NomadRetry.Attempts) + must.Eq(t, 20*time.Second, *templateConfig.NomadRetry.Backoff) + must.Eq(t, 25*time.Second, *templateConfig.NomadRetry.MaxBackoff) + }) - // Direct properties - require.Equal(t, 300*time.Second, *templateConfig.MaxStale) - require.Equal(t, 90*time.Second, *templateConfig.BlockQueryWaitTime) - // Wait - require.Equal(t, 2*time.Second, *templateConfig.Wait.Min) - require.Equal(t, 60*time.Second, *templateConfig.Wait.Max) - // WaitBounds - require.Equal(t, 2*time.Second, *templateConfig.WaitBounds.Min) - require.Equal(t, 60*time.Second, *templateConfig.WaitBounds.Max) - // Consul Retry - require.NotNil(t, templateConfig.ConsulRetry) - require.Equal(t, 5, *templateConfig.ConsulRetry.Attempts) - require.Equal(t, 5*time.Second, *templateConfig.ConsulRetry.Backoff) - require.Equal(t, 10*time.Second, *templateConfig.ConsulRetry.MaxBackoff) - // Vault Retry - require.NotNil(t, templateConfig.VaultRetry) - require.Equal(t, 10, *templateConfig.VaultRetry.Attempts) - require.Equal(t, 15*time.Second, *templateConfig.VaultRetry.Backoff) - require.Equal(t, 20*time.Second, *templateConfig.VaultRetry.MaxBackoff) - // Nomad Retry - require.NotNil(t, templateConfig.NomadRetry) - require.Equal(t, 15, *templateConfig.NomadRetry.Attempts) - require.Equal(t, 20*time.Second, *templateConfig.NomadRetry.Backoff) - require.Equal(t, 25*time.Second, *templateConfig.NomadRetry.MaxBackoff) } func TestConfig_LoadConsulTemplate_FunctionDenylist(t *testing.T) {