fix: ensure global podTemplate configuration is merged correctly

docs/matrix.md

@@ -176,7 +176,7 @@ data:
   ...
 ```
 
-For more information, see [installation customizations](install.md#customizing-basic-execution-parameters).
+For more information, see [installation customizations](./additional-configs.md#customizing-basic-execution-parameters).
 
 ## Parameters
 

docs/podtemplates.md

@@ -13,11 +13,11 @@ configuration that Tekton can use as "boilerplate" for a Pod that runs your `Tas
 You can specify a Pod template for `TaskRuns` and `PipelineRuns`. In the template, you can specify custom values for fields governing
 the execution of individual `Tasks` or for all `Tasks` executed by a given `PipelineRun`.
 
-You also have the option to define a global Pod template [in your Tekton config](./install.md#customizing-basic-execution-parameters) using the key `default-pod-template`.
-However, this global template is going to be merged with any templates
-you specify in your `TaskRuns` and `PipelineRuns`. Any field that is
-present in both the global template and the `TaskRun`'s or
+You also have the option to define a global Pod template [in your Tekton config](./additional-configs.md#customizing-basic-execution-parameters) using the key `default-pod-template`.
+However, this global template is going to be merged with any templates you specify in your `TaskRuns` and `PipelineRuns`.<br>
+Except for the `env` and `volumes` fields, other fields that exist in both the global template and the `TaskRun`'s or
 `PipelineRun`'s template will be taken from the `TaskRun` or `PipelineRun`.
+The `env` and `volumes` fields are merged by the `name` value in the array elements. If the item's `name` is the same, the item from `TaskRun` or `PipelineRun` will be used.
 
 See the following for examples of specifying a Pod template:
 - [Specifying a Pod template for a `TaskRun`](./taskruns.md#specifying-a-pod-template)
@@ -33,7 +33,7 @@ The supported fields are: `tolerations`, `nodeSelector`, and
 `imagePullSecrets` (see the table below for more details).
 
 Similarily to Pod templates, you have the option to define a global affinity
-assistant Pod template [in your Tekton config](./install.md#customizing-basic-execution-parameters)
+assistant Pod template [in your Tekton config](./additional-configs.md#customizing-basic-execution-parameters)
 using the key `default-affinity-assistant-pod-template`. The merge strategy is
 the same as the one described above.
 

docs/workspaces.md

@@ -255,7 +255,7 @@ a `Task` requires but which are not provided by the `TaskRun` will be bound with
 
 The configuration for the default `Workspace Binding` is added to the `config-defaults` `ConfigMap`, under
 the `default-task-run-workspace-binding` key. For an example, see the [Customizing basic execution
-parameters](./install.md#customizing-basic-execution-parameters) section of the install doc.
+parameters](./additional-configs.md#customizing-basic-execution-parameters) section of the install doc.
 
 **Note:** the default configuration is used for any _required_ `Workspace` declared by a `Task`. Optional
 `Workspaces` are not populated with the default binding. This is because a `Task's` behaviour will typically

pkg/apis/pipeline/pod/template.go

@@ -172,12 +172,10 @@ func MergePodTemplateWithDefault(tpl, defaultTpl *PodTemplate) *PodTemplate {
 		return defaultTpl
 	default:
 		// Otherwise, merge fields
-		if tpl.Env == nil {
-			tpl.Env = defaultTpl.Env
-		}
 		if tpl.NodeSelector == nil {
 			tpl.NodeSelector = defaultTpl.NodeSelector
 		}
+		tpl.Env = mergeByName(defaultTpl.Env, tpl.Env)
 		if tpl.Tolerations == nil {
 			tpl.Tolerations = defaultTpl.Tolerations
 		}
@@ -187,9 +185,7 @@ func MergePodTemplateWithDefault(tpl, defaultTpl *PodTemplate) *PodTemplate {
 		if tpl.SecurityContext == nil {
 			tpl.SecurityContext = defaultTpl.SecurityContext
 		}
-		if tpl.Volumes == nil {
-			tpl.Volumes = defaultTpl.Volumes
-		}
+		tpl.Volumes = mergeByName(defaultTpl.Volumes, tpl.Volumes)
 		if tpl.RuntimeClassName == nil {
 			tpl.RuntimeClassName = defaultTpl.RuntimeClassName
 		}
@@ -254,3 +250,49 @@ func MergeAAPodTemplateWithDefault(tpl, defaultTpl *AAPodTemplate) *AAPodTemplat
 		return tpl
 	}
 }
+
+// mergeByName merges two slices of items with names based on the getName
+// function, giving priority to the items in the override slice.
+func mergeByName[T any](base, overrides []T) []T {
+	if len(overrides) == 0 {
+		return base
+	}
+
+	// create a map to store the exist names in the override slice
+	exists := make(map[string]struct{})
+	merged := make([]T, 0, len(base)+len(overrides))
+
+	// append the items in the override slice
+	for _, item := range overrides {
+		name := getName(item)
+		if name != "" { // name should not be empty, if empty, ignore
+			merged = append(merged, item)
+			exists[name] = struct{}{}
+		}
+	}
+
+	// append the items in the base slice if they have a different name
+	for _, item := range base {
+		name := getName(item)
+		if name != "" { // name should not be empty, if empty, ignore
+			if _, found := exists[name]; !found {
+				merged = append(merged, item)
+			}
+		}
+	}
+
+	return merged
+}
+
+// getName returns the name of the given item, or an empty string if the item
+// is not a supported type.
+func getName(item interface{}) string {
+	switch item := item.(type) {
+	case corev1.EnvVar:
+		return item.Name
+	case corev1.Volume:
+		return item.Name
+	default:
+		return ""
+	}
+}

pkg/apis/pipeline/pod/template_test.go

@@ -0,0 +1,170 @@
+/*
+Copyright 2024 The Tekton Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package pod
+
+import (
+	"reflect"
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+)
+
+func TestMergeByName(t *testing.T) {
+	type testCase struct {
+		name      string
+		base      []interface{}
+		overrides []interface{}
+		expected  []interface{}
+	}
+
+	testCases := []testCase{
+		{
+			name:      "empty overrides",
+			base:      []interface{}{corev1.EnvVar{Name: "foo", Value: "bar"}},
+			overrides: []interface{}{},
+			expected:  []interface{}{corev1.EnvVar{Name: "foo", Value: "bar"}},
+		},
+		{
+			name:      "empty base",
+			base:      []interface{}{},
+			overrides: []interface{}{corev1.EnvVar{Name: "foo", Value: "bar"}},
+			expected:  []interface{}{corev1.EnvVar{Name: "foo", Value: "bar"}},
+		},
+		{
+			name:      "same name",
+			base:      []interface{}{corev1.EnvVar{Name: "foo", Value: "bar"}},
+			overrides: []interface{}{corev1.EnvVar{Name: "foo", Value: "baz"}},
+			expected:  []interface{}{corev1.EnvVar{Name: "foo", Value: "baz"}},
+		},
+		{
+			name:      "different name",
+			base:      []interface{}{corev1.EnvVar{Name: "foo", Value: "bar"}},
+			overrides: []interface{}{corev1.EnvVar{Name: "bar", Value: "baz"}},
+			expected:  []interface{}{corev1.EnvVar{Name: "bar", Value: "baz"}, corev1.EnvVar{Name: "foo", Value: "bar"}},
+		},
+		{
+			name:      "different volume name",
+			base:      []interface{}{corev1.Volume{Name: "foo", VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}}},
+			overrides: []interface{}{corev1.Volume{Name: "bar", VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}}},
+			expected: []interface{}{
+				corev1.Volume{Name: "bar", VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}},
+				corev1.Volume{Name: "foo", VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}},
+			},
+		},
+		{
+			name:      "unsupported type",
+			base:      []interface{}{corev1.EnvVar{Name: "foo", Value: "bar"}},
+			overrides: []interface{}{42},
+			expected:  []interface{}{corev1.EnvVar{Name: "foo", Value: "bar"}},
+		},
+		{
+			name:      "empty name",
+			base:      []interface{}{corev1.EnvVar{Name: "", Value: "bar"}},
+			overrides: []interface{}{corev1.EnvVar{Name: "", Value: "bar"}},
+			expected:  []interface{}{},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			result := mergeByName(tc.base, tc.overrides)
+			if !reflect.DeepEqual(result, tc.expected) {
+				t.Errorf("mergeByName(%v, %v) = %v, want %v", tc.base, tc.overrides, result, tc.expected)
+			}
+		})
+	}
+}
+
+func TestMergePodTemplateWithDefault(t *testing.T) {
+	type testCase struct {
+		name       string
+		tpl        *PodTemplate
+		defaultTpl *PodTemplate
+		expected   *PodTemplate
+	}
+
+	testCases := []testCase{
+		{
+			name: "defaultTpl is nil",
+			tpl: &PodTemplate{
+				NodeSelector: map[string]string{"foo": "bar"},
+			},
+			defaultTpl: nil,
+			expected: &PodTemplate{
+				NodeSelector: map[string]string{"foo": "bar"},
+			},
+		},
+		{
+			name: "tpl is nil",
+			tpl:  nil,
+			defaultTpl: &PodTemplate{
+				NodeSelector: map[string]string{"foo": "bar"},
+			},
+			expected: &PodTemplate{
+				NodeSelector: map[string]string{"foo": "bar"},
+			},
+		},
+		{
+			name: "override default env",
+			tpl: &PodTemplate{
+				Env: []corev1.EnvVar{{Name: "foo", Value: "bar"}},
+			},
+			defaultTpl: &PodTemplate{
+				Env: []corev1.EnvVar{{Name: "foo", Value: "baz"}},
+			},
+			expected: &PodTemplate{
+				Env: []corev1.EnvVar{{Name: "foo", Value: "bar"}},
+			},
+		},
+		{
+			name: "merge envs",
+			tpl: &PodTemplate{
+				Env: []corev1.EnvVar{{Name: "foo", Value: "bar"}},
+			},
+			defaultTpl: &PodTemplate{
+				Env: []corev1.EnvVar{{Name: "bar", Value: "bar"}},
+			},
+			expected: &PodTemplate{
+				Env: []corev1.EnvVar{
+					{Name: "foo", Value: "bar"},
+					{Name: "bar", Value: "bar"},
+				},
+			},
+		},
+		{
+			name: "update host network",
+			tpl: &PodTemplate{
+				HostNetwork: false,
+			},
+			defaultTpl: &PodTemplate{
+				HostNetwork: true,
+			},
+			expected: &PodTemplate{
+				HostNetwork: true,
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			result := MergePodTemplateWithDefault(tc.tpl, tc.defaultTpl)
+			if !reflect.DeepEqual(result, tc.expected) {
+				t.Errorf("mergeByName(%v, %v) = %v, want %v", tc.tpl, tc.defaultTpl, result, tc.expected)
+			}
+		})
+	}
+}