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

@@ -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
+```

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{
+		TemplateConfig:          DefaultTemplateConfig(),
-			FunctionDenylist:   DefaultTemplateFunctionDenylist,
+		RPCHoldTimeout:          5 * time.Second,
-			DisableSandbox:     false,
+		CNIPath:                 "/opt/cni/bin",
-			BlockQueryWaitTime: pointer.Of(5 * time.Minute),         // match Consul default
+		CNIConfigDir:            "/opt/cni/config",
-			MaxStale:           pointer.Of(DefaultTemplateMaxStale), // match Consul default
+		CNIInterfacePrefix:      "eth",
-			Wait: &WaitConfig{
+		HostNetworks:            map[string]*structs.ClientHostNetworkConfig{},
-				Min: pointer.Of(5 * time.Second),
+		CgroupParent:            "nomad.slice", // SETH todo
-				Max: pointer.Of(4 * time.Minute),
+		MaxDynamicPort:          structs.DefaultMinDynamicPort,
-			},
+		MinDynamicPort:          structs.DefaultMaxDynamicPort,
-			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,
 		Users: &UsersConfig{
 			MinDynamicUser: 80_000,
 			MaxDynamicUser: 89_999,

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

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{
+			TemplateConfig:                 client.DefaultTemplateConfig(),
-				FunctionDenylist: client.DefaultTemplateFunctionDenylist,
-				DisableSandbox:   false,
-			},
 			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

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,
+				FunctionDenylist:   client.DefaultTemplateFunctionDenylist,
-				DisableSandbox:   false,
+				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()
+	t.Run("minimal client expect defaults", func(t *testing.T) {
-	// Test that loading without template config didn't create load errors
+		defaultConfig := DefaultConfig()
-	agentConfig, err := LoadConfig("test-resources/minimal_client.hcl")
+		agentConfig, err := LoadConfig("test-resources/minimal_client.hcl")
-	require.NoError(t, err)
+		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
+	t.Run("client config with nil function denylist", func(t *testing.T) {
-	agentConfig, err = LoadConfig("test-resources/client_with_template.hcl")
+		defaultConfig := DefaultConfig()
-	require.NoError(t, err)
+		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}
+	t.Run("client config with basic template", func(t *testing.T) {
-	clientConfig, err := clientAgent.clientConfig()
+		defaultConfig := DefaultConfig()
-	require.NoError(t, err)
+		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
+		// check explicit overrides
-	require.NotNil(t, templateConfig.BlockQueryWaitTime)
+		must.Eq(t, true, templateConfig.DisableSandbox)
-	require.NotNil(t, templateConfig.MaxStale)
+		must.Len(t, 0, templateConfig.FunctionDenylist)
-	require.NotNil(t, templateConfig.Wait)
+
-	require.NotNil(t, templateConfig.WaitBounds)
+		// check all the complex defaults
-	require.NotNil(t, templateConfig.ConsulRetry)
+		must.Eq(t, 87600*time.Hour, *templateConfig.MaxStale)
-	require.NotNil(t, templateConfig.VaultRetry)
+		must.Eq(t, 5*time.Minute, *templateConfig.BlockQueryWaitTime)
-	require.NotNil(t, templateConfig.NomadRetry)
+
+		// 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) {