Fix Validation Error Merging StepTemplates with Step Ref

This PR fixes the validation error when using StepTemplates with Steps referencing StepActions. Fixes: #7981
tektoncd · May 24, 2024 · d6e2559 · d6e2559
1 parent 1f3b1b4
commit d6e2559
Show file tree

Hide file tree

Showing 7 changed files with 173 additions and 5 deletions.
diff --git a/examples/v1/taskruns/beta/stepactions-steptemplate.yaml b/examples/v1/taskruns/beta/stepactions-steptemplate.yaml
@@ -0,0 +1,21 @@
+apiVersion: tekton.dev/v1beta1
+kind: StepAction
+metadata:
+  name: step-action
+spec:
+  image: alpine
+  command: ["env"]
+---
+apiVersion: tekton.dev/v1
+kind: TaskRun
+metadata:
+  name: task-run
+spec:
+  taskSpec:
+    steps:
+      - ref:
+          name: step-action
+    stepTemplate:
+      env:
+        - name: foo
+          value: bar
diff --git a/pkg/apis/pipeline/v1/merge.go b/pkg/apis/pipeline/v1/merge.go
@@ -46,6 +46,11 @@ func MergeStepsWithStepTemplate(template *StepTemplate, steps []Step) ([]Step, e
 	}
 
 	for i, s := range steps {
+		// If the stepaction has not been fetched yet then do not merge.
+		// Skip over to the next one
+		if s.Ref != nil {
+			continue
+		}
 		merged := corev1.Container{}
 		err := mergeObjWithTemplateBytes(md, s.ToK8sContainer(), &merged)
 		if err != nil {

diff --git a/pkg/apis/pipeline/v1/merge_test.go b/pkg/apis/pipeline/v1/merge_test.go
@@ -145,7 +145,8 @@ func TestMergeStepsWithStepTemplate(t *testing.T) {
 			}},
 		}},
 		expected: []v1.Step{{
-			Command: []string{"/somecmd"}, Image: "some-image",
+			Command: []string{"/somecmd"},
+			Image:   "some-image",
 			OnError: "foo",
 			Results: []v1.StepResult{{
 				Name: "result",
@@ -155,11 +156,30 @@ func TestMergeStepsWithStepTemplate(t *testing.T) {
 			}},
 		}},
 	}, {
-		name: "ref-should-not-be-removed",
+		name: "step-ref-should-not-be-merged-with-steptemplate",
 		template: &v1.StepTemplate{
 			SecurityContext: &corev1.SecurityContext{
 				RunAsNonRoot: pointer.Bool(true),
 			},
+			VolumeMounts: []corev1.VolumeMount{{
+				Name:      "data",
+				MountPath: "/workspace/data",
+			}},
+			Env: []corev1.EnvVar{{
+				Name:  "KEEP_THIS",
+				Value: "A_VALUE",
+			}, {
+				Name: "SOME_KEY_1",
+				ValueFrom: &corev1.EnvVarSource{
+					SecretKeyRef: &corev1.SecretKeySelector{
+						Key:                  "A_KEY",
+						LocalObjectReference: corev1.LocalObjectReference{Name: "A_NAME"},
+					},
+				},
+			}, {
+				Name:  "SOME_KEY_2",
+				Value: "VALUE_2",
+			}},
 		},
 		steps: []v1.Step{{
 			Ref:     &v1.Ref{Name: "my-step-action"},
@@ -172,9 +192,6 @@ func TestMergeStepsWithStepTemplate(t *testing.T) {
 			}},
 		}},
 		expected: []v1.Step{{
-			SecurityContext: &corev1.SecurityContext{
-				RunAsNonRoot: pointer.Bool(true),
-			},
 			Ref:     &v1.Ref{Name: "my-step-action"},
 			OnError: "foo",
 			Results: []v1.StepResult{{

diff --git a/pkg/apis/pipeline/v1/task_validation_test.go b/pkg/apis/pipeline/v1/task_validation_test.go
@@ -30,6 +30,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/utils/pointer"
 	"knative.dev/pkg/apis"
 )
 
@@ -333,6 +334,41 @@ func TestTaskSpecValidate(t *testing.T) {
 				hello world`,
 			}},
 		},
+	}, {
+		name: "step template included in validation with stepaction",
+		fields: fields{
+			Steps: []v1.Step{{
+				Name: "astep",
+				Ref: &v1.Ref{
+					Name: "stepAction",
+				},
+			}},
+			StepTemplate: &v1.StepTemplate{
+				Image: "some-image",
+				SecurityContext: &corev1.SecurityContext{
+					RunAsNonRoot: pointer.Bool(true),
+				},
+				VolumeMounts: []corev1.VolumeMount{{
+					Name:      "data",
+					MountPath: "/workspace/data",
+				}},
+				Env: []corev1.EnvVar{{
+					Name:  "KEEP_THIS",
+					Value: "A_VALUE",
+				}, {
+					Name: "SOME_KEY_1",
+					ValueFrom: &corev1.EnvVarSource{
+						SecretKeyRef: &corev1.SecretKeySelector{
+							Key:                  "A_KEY",
+							LocalObjectReference: corev1.LocalObjectReference{Name: "A_NAME"},
+						},
+					},
+				}, {
+					Name:  "SOME_KEY_2",
+					Value: "VALUE_2",
+				}},
+			},
+		},
 	}, {
 		name: "valid step with parameterized script",
 		fields: fields{

diff --git a/pkg/apis/pipeline/v1beta1/merge.go b/pkg/apis/pipeline/v1beta1/merge.go
@@ -47,6 +47,11 @@ func MergeStepsWithStepTemplate(template *StepTemplate, steps []Step) ([]Step, e
 	}
 
 	for i, s := range steps {
+		// If the stepaction has not been fetched yet then do not merge.
+		// Skip over to the next one
+		if s.Ref != nil {
+			continue
+		}
 		merged := corev1.Container{}
 		err := mergeObjWithTemplateBytes(md, s.ToK8sContainer(), &merged)
 		if err != nil {

diff --git a/pkg/apis/pipeline/v1beta1/merge_test.go b/pkg/apis/pipeline/v1beta1/merge_test.go
@@ -20,10 +20,12 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
 	v1beta1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
 	"github.com/tektoncd/pipeline/test/diff"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
+	"k8s.io/utils/pointer"
 )
 
 func TestMergeStepsWithStepTemplate(t *testing.T) {
@@ -127,6 +129,52 @@ func TestMergeStepsWithStepTemplate(t *testing.T) {
 				MountPath: "/workspace/data",
 			}},
 		}},
+	}, {
+		name: "step-ref-should-not-be-merged-with-steptemplate",
+		template: &v1beta1.StepTemplate{
+			SecurityContext: &corev1.SecurityContext{
+				RunAsNonRoot: pointer.Bool(true),
+			},
+			VolumeMounts: []corev1.VolumeMount{{
+				Name:      "data",
+				MountPath: "/workspace/data",
+			}},
+			Env: []corev1.EnvVar{{
+				Name:  "KEEP_THIS",
+				Value: "A_VALUE",
+			}, {
+				Name: "SOME_KEY_1",
+				ValueFrom: &corev1.EnvVarSource{
+					SecretKeyRef: &corev1.SecretKeySelector{
+						Key:                  "A_KEY",
+						LocalObjectReference: corev1.LocalObjectReference{Name: "A_NAME"},
+					},
+				},
+			}, {
+				Name:  "SOME_KEY_2",
+				Value: "VALUE_2",
+			}},
+		},
+		steps: []v1beta1.Step{{
+			Ref:     &v1beta1.Ref{Name: "my-step-action"},
+			OnError: "foo",
+			Results: []v1.StepResult{{
+				Name: "result",
+			}},
+			Params: v1beta1.Params{{
+				Name: "param",
+			}},
+		}},
+		expected: []v1beta1.Step{{
+			Ref:     &v1beta1.Ref{Name: "my-step-action"},
+			OnError: "foo",
+			Results: []v1.StepResult{{
+				Name: "result",
+			}},
+			Params: v1beta1.Params{{
+				Name: "param",
+			}},
+		}},
 	}, {
 		name: "merge-env-by-step",
 		template: &v1beta1.StepTemplate{

diff --git a/pkg/apis/pipeline/v1beta1/task_validation_test.go b/pkg/apis/pipeline/v1beta1/task_validation_test.go
@@ -34,6 +34,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/utils/pointer"
 	"knative.dev/pkg/apis"
 )
 
@@ -337,6 +338,41 @@ func TestTaskSpecValidate(t *testing.T) {
 				hello world`,
 			}},
 		},
+	}, {
+		name: "step template included in validation with stepaction",
+		fields: fields{
+			Steps: []v1beta1.Step{{
+				Name: "astep",
+				Ref: &v1beta1.Ref{
+					Name: "stepAction",
+				},
+			}},
+			StepTemplate: &v1beta1.StepTemplate{
+				Image: "some-image",
+				SecurityContext: &corev1.SecurityContext{
+					RunAsNonRoot: pointer.Bool(true),
+				},
+				VolumeMounts: []corev1.VolumeMount{{
+					Name:      "data",
+					MountPath: "/workspace/data",
+				}},
+				Env: []corev1.EnvVar{{
+					Name:  "KEEP_THIS",
+					Value: "A_VALUE",
+				}, {
+					Name: "SOME_KEY_1",
+					ValueFrom: &corev1.EnvVarSource{
+						SecretKeyRef: &corev1.SecretKeySelector{
+							Key:                  "A_KEY",
+							LocalObjectReference: corev1.LocalObjectReference{Name: "A_NAME"},
+						},
+					},
+				}, {
+					Name:  "SOME_KEY_2",
+					Value: "VALUE_2",
+				}},
+			},
+		},
 	}, {
 		name: "valid step with parameterized script",
 		fields: fields{