From c566da6bb41b6c43f3b9322ea0e30acf3e52856f Mon Sep 17 00:00:00 2001 From: qingliu Date: Tue, 9 Jan 2024 19:08:20 +0800 Subject: [PATCH] fix: ensure global podTemplate configuration is merged correctly fix #7551 Adjust the behavior of how global Pod templates are merged with TaskRun or PipelineRun. The `env` and `volumes` fields are merged by the `name` value in the array elements. --- docs/matrix.md | 2 +- docs/podtemplates.md | 10 +- docs/workspaces.md | 2 +- pkg/apis/pipeline/pod/template.go | 54 +++++++- pkg/apis/pipeline/pod/template_test.go | 170 +++++++++++++++++++++++++ 5 files changed, 225 insertions(+), 13 deletions(-) create mode 100644 pkg/apis/pipeline/pod/template_test.go diff --git a/docs/matrix.md b/docs/matrix.md index 8a9b024ac3e..7ecfaaaecf9 100644 --- a/docs/matrix.md +++ b/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 diff --git a/docs/podtemplates.md b/docs/podtemplates.md index cfd4f69245c..af0888d4bb9 100644 --- a/docs/podtemplates.md +++ b/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`.
+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. diff --git a/docs/workspaces.md b/docs/workspaces.md index 26b72f0e9f3..8cf7c578e3a 100644 --- a/docs/workspaces.md +++ b/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 diff --git a/pkg/apis/pipeline/pod/template.go b/pkg/apis/pipeline/pod/template.go index e9f75fa3b74..e551e6efdee 100644 --- a/pkg/apis/pipeline/pod/template.go +++ b/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 "" + } +} diff --git a/pkg/apis/pipeline/pod/template_test.go b/pkg/apis/pipeline/pod/template_test.go new file mode 100644 index 00000000000..64eaf704877 --- /dev/null +++ b/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) + } + }) + } +}