From 7277870d2023b11fc36c93d41f1e48e4bb64535d Mon Sep 17 00:00:00 2001 From: Priti Desai Date: Mon, 19 Jun 2023 21:09:52 -0700 Subject: [PATCH] merge podTemplates instead of override The podTemplate can be specified for any given pipelineTask at the pipelineRun.spec.taskRunSpecs. The podTemplate can also be specified for the entire pipeline at the pipelineRun.spec.podTemplate. Before this commit, the podTemplates at the pipelineRun level were ignored if it was specified at the taskRunSpecs. This results in the settings specified at the pipelineRun level not included along with the pipelineTask specific configuration. For example, there could be a common setting at the pipelineRun level to set the User and Group for all the pipelineTasks and a pipelineTask can have its own diskType. Without this commit, the taskRun runs with a podTemplate with the specified diskType and no user or group configurations. Signed-off-by: Priti Desai --- docs/pipelineruns.md | 3 +- pkg/apis/pipeline/v1/pipelinerun_types.go | 6 +- .../pipeline/v1/pipelinerun_types_test.go | 75 +++++++++++++++++++ .../pipeline/v1beta1/pipelinerun_types.go | 6 +- .../v1beta1/pipelinerun_types_test.go | 73 ++++++++++++++++++ 5 files changed, 156 insertions(+), 7 deletions(-) diff --git a/docs/pipelineruns.md b/docs/pipelineruns.md index 6f8f1fa9c54..12ce004df97 100644 --- a/docs/pipelineruns.md +++ b/docs/pipelineruns.md @@ -896,7 +896,8 @@ spec: {{< /tab >}} {{< /tabs >}} -If used with this `Pipeline`, `build-task` will use the task specific `PodTemplate` (where `nodeSelector` has `disktype` equal to `ssd`). +If used with this `Pipeline`, `build-task` will use the task specific `PodTemplate` (where `nodeSelector` has `disktype` equal to `ssd`) +along with `securityContext` from the `pipelineRun.spec.podTemplate`. `PipelineTaskRunSpec` may also contain `StepSpecs` and `SidecarSpecs`; see [Overriding `Task` `Steps` and `Sidecars`](./taskruns.md#overriding-task-steps-and-sidecars) for more information. diff --git a/pkg/apis/pipeline/v1/pipelinerun_types.go b/pkg/apis/pipeline/v1/pipelinerun_types.go index 4f4e33b3a18..cc8ad99264f 100644 --- a/pkg/apis/pipeline/v1/pipelinerun_types.go +++ b/pkg/apis/pipeline/v1/pipelinerun_types.go @@ -555,9 +555,9 @@ func (pr *PipelineRun) GetTaskRunSpec(pipelineTaskName string) PipelineTaskRunSp } for _, task := range pr.Spec.TaskRunSpecs { if task.PipelineTaskName == pipelineTaskName { - if task.PodTemplate != nil { - s.PodTemplate = task.PodTemplate - } + // merge podTemplates specified in pipelineRun.spec.taskRunSpecs[].podTemplate and pipelineRun.spec.podTemplate + // with taskRunSpecs taking higher precedence + s.PodTemplate = pod.MergePodTemplateWithDefault(task.PodTemplate, s.PodTemplate) if task.ServiceAccountName != "" { s.ServiceAccountName = task.ServiceAccountName } diff --git a/pkg/apis/pipeline/v1/pipelinerun_types_test.go b/pkg/apis/pipeline/v1/pipelinerun_types_test.go index 8ca1e7b0fe1..07f06970940 100644 --- a/pkg/apis/pipeline/v1/pipelinerun_types_test.go +++ b/pkg/apis/pipeline/v1/pipelinerun_types_test.go @@ -401,3 +401,78 @@ func TestPipelineRunGetPodSpec(t *testing.T) { } } } + +func TestPipelineRun_GetTaskRunSpec(t *testing.T) { + user := int64(1000) + group := int64(2000) + fsGroup := int64(3000) + for _, tt := range []struct { + name string + pr *v1.PipelineRun + expectedPodTemplates map[string]*pod.PodTemplate + }{ + { + name: "pipelineRun Spec podTemplate and taskRunSpec pipelineTask podTemplate", + pr: &v1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{Name: "pr"}, + Spec: v1.PipelineRunSpec{ + TaskRunTemplate: v1.PipelineTaskRunTemplate{ + PodTemplate: &pod.Template{ + SecurityContext: &corev1.PodSecurityContext{ + RunAsUser: &user, + RunAsGroup: &group, + FSGroup: &fsGroup, + }, + }, + ServiceAccountName: "defaultSA", + }, + PipelineRef: &v1.PipelineRef{Name: "prs"}, + TaskRunSpecs: []v1.PipelineTaskRunSpec{{ + PipelineTaskName: "task-1", + ServiceAccountName: "task-1-service-account", + PodTemplate: &pod.Template{ + NodeSelector: map[string]string{ + "diskType": "ssd", + }, + }, + }, { + PipelineTaskName: "task-2", + ServiceAccountName: "task-2-service-account", + PodTemplate: &pod.Template{ + SchedulerName: "task-2-schedule", + }, + }}, + }, + }, + expectedPodTemplates: map[string]*pod.PodTemplate{ + "task-1": { + SecurityContext: &corev1.PodSecurityContext{ + RunAsUser: &user, + RunAsGroup: &group, + FSGroup: &fsGroup, + }, + NodeSelector: map[string]string{ + "diskType": "ssd", + }, + }, + "task-2": { + SecurityContext: &corev1.PodSecurityContext{ + RunAsUser: &user, + RunAsGroup: &group, + FSGroup: &fsGroup, + }, + SchedulerName: "task-2-schedule", + }, + }, + }, + } { + for taskName := range tt.expectedPodTemplates { + t.Run(tt.name, func(t *testing.T) { + s := tt.pr.GetTaskRunSpec(taskName) + if d := cmp.Diff(tt.expectedPodTemplates[taskName], s.PodTemplate); d != "" { + t.Error(diff.PrintWantGot(d)) + } + }) + } + } +} diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_types.go b/pkg/apis/pipeline/v1beta1/pipelinerun_types.go index 83bf2c22e1a..53c1f738c0f 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_types.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_types.go @@ -618,9 +618,9 @@ func (pr *PipelineRun) GetTaskRunSpec(pipelineTaskName string) PipelineTaskRunSp } for _, task := range pr.Spec.TaskRunSpecs { if task.PipelineTaskName == pipelineTaskName { - if task.TaskPodTemplate != nil { - s.TaskPodTemplate = task.TaskPodTemplate - } + // merge podTemplates specified in pipelineRun.spec.taskRunSpecs[].podTemplate and pipelineRun.spec.podTemplate + // with taskRunSpecs taking higher precedence + s.TaskPodTemplate = pod.MergePodTemplateWithDefault(task.TaskPodTemplate, s.TaskPodTemplate) if task.TaskServiceAccountName != "" { s.TaskServiceAccountName = task.TaskServiceAccountName } diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_types_test.go b/pkg/apis/pipeline/v1beta1/pipelinerun_types_test.go index 543861b3575..260235d1615 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_types_test.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_types_test.go @@ -662,3 +662,76 @@ func TestPipelineRunGetPodSpec(t *testing.T) { } } } + +func TestPipelineRun_GetTaskRunSpec(t *testing.T) { + user := int64(1000) + group := int64(2000) + fsGroup := int64(3000) + for _, tt := range []struct { + name string + pr *v1beta1.PipelineRun + expectedPodTemplates map[string]*pod.PodTemplate + }{ + { + name: "pipelineRun Spec podTemplate and taskRunSpec pipelineTask podTemplate", + pr: &v1beta1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{Name: "pr"}, + Spec: v1beta1.PipelineRunSpec{ + PodTemplate: &pod.Template{ + SecurityContext: &corev1.PodSecurityContext{ + RunAsUser: &user, + RunAsGroup: &group, + FSGroup: &fsGroup, + }, + }, + PipelineRef: &v1beta1.PipelineRef{Name: "prs"}, + ServiceAccountName: "defaultSA", + TaskRunSpecs: []v1beta1.PipelineTaskRunSpec{{ + PipelineTaskName: "task-1", + TaskServiceAccountName: "task-1-service-account", + TaskPodTemplate: &pod.Template{ + NodeSelector: map[string]string{ + "diskType": "ssd", + }, + }, + }, { + PipelineTaskName: "task-2", + TaskServiceAccountName: "task-2-service-account", + TaskPodTemplate: &pod.Template{ + SchedulerName: "task-2-schedule", + }, + }}, + }, + }, + expectedPodTemplates: map[string]*pod.PodTemplate{ + "task-1": { + SecurityContext: &corev1.PodSecurityContext{ + RunAsUser: &user, + RunAsGroup: &group, + FSGroup: &fsGroup, + }, + NodeSelector: map[string]string{ + "diskType": "ssd", + }, + }, + "task-2": { + SecurityContext: &corev1.PodSecurityContext{ + RunAsUser: &user, + RunAsGroup: &group, + FSGroup: &fsGroup, + }, + SchedulerName: "task-2-schedule", + }, + }, + }, + } { + for taskName := range tt.expectedPodTemplates { + t.Run(tt.name, func(t *testing.T) { + s := tt.pr.GetTaskRunSpec(taskName) + if d := cmp.Diff(tt.expectedPodTemplates[taskName], s.TaskPodTemplate); d != "" { + t.Error(diff.PrintWantGot(d)) + } + }) + } + } +}