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 <[email protected]>
tektoncd · Jun 26, 2023 · fa97071 · fa97071
1 parent e81308f
commit fa97071
Show file tree

Hide file tree

Showing 5 changed files with 157 additions and 8 deletions.
diff --git a/docs/pipelineruns.md b/docs/pipelineruns.md
@@ -919,8 +919,9 @@ spec:
           disktype: ssd
 ```
 
-If used with this `Pipeline`,  `build-task` will use the task specific `PodTemplate` (where `nodeSelector` has `disktype` equal to `ssd`).
-`PipelineTaskRunSpec` may also contain `StepOverrides` and `SidecarOverrides`; see
+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.
 
 The optional annotations and labels can be added under a `Metadata` field as for a specific running context.

diff --git a/pkg/apis/pipeline/v1/pipelinerun_types.go b/pkg/apis/pipeline/v1/pipelinerun_types.go
@@ -551,9 +551,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
@@ -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
@@ -582,9 +582,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
@@ -502,3 +502,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))
+				}
+			})
+		}
+	}
+}