Reject embedded Tasks with APIVersion and/or Kind specified

fixes #4543 If custom tasks are enabled and either of `PipelineTask.TaskSpec.APIVersion` or `PipelineTask.TaskSpec.Kind` are specified, the `PipelineTask` will be treated as a custom task. This is still the case if an actual embedded `Task` is specified in `PipelineTask.TaskSpec.TaskSpec`, so we should be failing validation for those cases. Signed-off-by: Andrew Bayer <[email protected]>
tektoncd · Jun 28, 2022 · 88ea861 · 88ea861
1 parent c6ff981
commit 88ea861
Show file tree

Hide file tree

Showing 4 changed files with 166 additions and 1 deletion.
diff --git a/docs/pipelines.md b/docs/pipelines.md
@@ -291,7 +291,7 @@ spec:
 
  Your `Pipeline` definition must reference at least one [`Task`](tasks.md).
 Each `Task` within a `Pipeline` must have a [valid](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names)
-`name` and a `taskRef`. For example:
+`name` and a `taskRef` or a `taskSpec`. For example:
 
 ```yaml
 tasks:
@@ -300,6 +300,19 @@ tasks:
       name: build-push
 ```
 
+or
+
+```yaml
+tasks:
+  - name: say-hello
+    taskSpec:
+      steps:
+      - image: ubuntu
+        script: echo 'hello there'
+```
+
+Note that any `task` specified in `taskSpec` will be the same version as the `Pipeline`.
+
 ### Specifying `Resources` in `PipelineTasks`
 
 You can use [`PipelineResources`](#specifying-resources) as inputs and outputs for `Tasks`

diff --git a/pkg/apis/pipeline/v1beta1/pipeline_types.go b/pkg/apis/pipeline/v1beta1/pipeline_types.go
@@ -321,6 +321,26 @@ func (pt *PipelineTask) validateMatrixCombinationsCount(ctx context.Context) (er
 	return errs
 }
 
+func (pt PipelineTask) validateEmbeddedOrType() (errs *apis.FieldError) {
+	// Reject cases where APIVersion and/or Kind are specified alongside an embedded Task.
+	// We determine if this is an embedded Task by checking of TaskSpec.TaskSpec.Steps has items.
+	if pt.TaskSpec != nil && len(pt.TaskSpec.TaskSpec.Steps) > 0 {
+		if pt.TaskSpec.APIVersion != "" {
+			errs = errs.Also(&apis.FieldError{
+				Message: "taskSpec.apiVersion cannot be specified when using taskSpec.steps",
+				Paths:   []string{"taskSpec.apiVersion"},
+			})
+		}
+		if pt.TaskSpec.Kind != "" {
+			errs = errs.Also(&apis.FieldError{
+				Message: "taskSpec.kind cannot be specified when using taskSpec.steps",
+				Paths:   []string{"taskSpec.kind"},
+			})
+		}
+	}
+	return
+}
+
 // GetMatrixCombinationsCount returns the count of combinations of Parameters generated from the Matrix in PipelineTask.
 func (pt *PipelineTask) GetMatrixCombinationsCount() int {
 	if len(pt.Matrix) == 0 {
@@ -464,6 +484,9 @@ func (pt PipelineTask) ValidateName() *apis.FieldError {
 // calls the validation routine based on the type of the task
 func (pt PipelineTask) Validate(ctx context.Context) (errs *apis.FieldError) {
 	errs = errs.Also(pt.validateRefOrSpec())
+
+	errs = errs.Also(pt.validateEmbeddedOrType())
+
 	cfg := config.FromContextOrDefaults(ctx)
 	// If EnableCustomTasks feature flag is on, validate custom task specifications
 	// pipeline task having taskRef with APIVersion is classified as custom task

diff --git a/pkg/apis/pipeline/v1beta1/pipeline_types_test.go b/pkg/apis/pipeline/v1beta1/pipeline_types_test.go
@@ -899,3 +899,90 @@ func TestPipelineTask_GetMatrixCombinationsCount(t *testing.T) {
 		})
 	}
 }
+
+func TestPipelineTask_ValidateEmbeddedOrType(t *testing.T) {
+	testCases := []struct {
+		name          string
+		pt            PipelineTask
+		expectedError *apis.FieldError
+	}{
+		{
+			name: "just apiVersion and kind",
+			pt: PipelineTask{
+				TaskSpec: &EmbeddedTask{
+					TypeMeta: runtime.TypeMeta{
+						APIVersion: "something",
+						Kind:       "whatever",
+					},
+				},
+			},
+		}, {
+			name: "just steps",
+			pt: PipelineTask{
+				TaskSpec: &EmbeddedTask{
+					TaskSpec: TaskSpec{
+						Steps: []Step{{
+							Name:  "foo",
+							Image: "bar",
+						}},
+					},
+				},
+			},
+		}, {
+			name: "apiVersion and steps",
+			pt: PipelineTask{
+				TaskSpec: &EmbeddedTask{
+					TypeMeta: runtime.TypeMeta{
+						APIVersion: "something",
+					},
+					TaskSpec: TaskSpec{
+						Steps: []Step{{
+							Name:  "foo",
+							Image: "bar",
+						}},
+					},
+				},
+			},
+			expectedError: &apis.FieldError{
+				Message: "taskSpec.apiVersion cannot be specified when using taskSpec.steps",
+				Paths:   []string{"taskSpec.apiVersion"},
+			},
+		}, {
+			name: "kind and steps",
+			pt: PipelineTask{
+				TaskSpec: &EmbeddedTask{
+					TypeMeta: runtime.TypeMeta{
+						Kind: "something",
+					},
+					TaskSpec: TaskSpec{
+						Steps: []Step{{
+							Name:  "foo",
+							Image: "bar",
+						}},
+					},
+				},
+			},
+			expectedError: &apis.FieldError{
+				Message: "taskSpec.kind cannot be specified when using taskSpec.steps",
+				Paths:   []string{"taskSpec.kind"},
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			err := tc.pt.validateEmbeddedOrType()
+			if err == nil && tc.expectedError != nil {
+				t.Fatalf("PipelineTask.validateEmbeddedOrType() did not return expected error '%s'", tc.expectedError.Error())
+			}
+			if err != nil {
+				if tc.expectedError == nil {
+					t.Fatalf("PipelineTask.validateEmbeddedOrType() returned unexpected error '%s'", err.Error())
+				}
+				if d := cmp.Diff(tc.expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" {
+					t.Errorf("PipelineTask.validateEmbeddedOrType() errors diff %s", diff.PrintWantGot(d))
+				}
+			}
+		})
+	}
+}
diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go
@@ -727,6 +727,48 @@ func TestValidatePipelineTasks_Failure(t *testing.T) {
 			Message: `expected exactly one, got both`,
 			Paths:   []string{"tasks[1].name"},
 		},
+	}, {
+		name: "apiVersion with steps",
+		tasks: []PipelineTask{{
+			Name: "foo",
+			TaskSpec: &EmbeddedTask{
+				TypeMeta: runtime.TypeMeta{
+					APIVersion: "tekton.dev/v1beta1",
+				},
+				TaskSpec: TaskSpec{
+					Steps: []Step{{
+						Name:  "some-step",
+						Image: "some-image",
+					}},
+				},
+			},
+		}},
+		finalTasks: nil,
+		expectedError: apis.FieldError{
+			Message: "taskSpec.apiVersion cannot be specified when using taskSpec.steps",
+			Paths:   []string{"tasks[0].taskSpec.apiVersion"},
+		},
+	}, {
+		name: "kind with steps",
+		tasks: []PipelineTask{{
+			Name: "foo",
+			TaskSpec: &EmbeddedTask{
+				TypeMeta: runtime.TypeMeta{
+					Kind: "Task",
+				},
+				TaskSpec: TaskSpec{
+					Steps: []Step{{
+						Name:  "some-step",
+						Image: "some-image",
+					}},
+				},
+			},
+		}},
+		finalTasks: nil,
+		expectedError: apis.FieldError{
+			Message: "taskSpec.kind cannot be specified when using taskSpec.steps",
+			Paths:   []string{"tasks[0].taskSpec.kind"},
+		},
 	}}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {