Custom task without api version return validation error

This commit closes #6459. For a customtask reference in a pipelinetask, if the Kind is non-empty and not "Task" or "ClusterTask", then it should be a Custom task and api version should be set. If not then validation webhook should return error. TaskRun's taskRef validation will be handled separately in #6557 Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com
tektoncd · Apr 20, 2023 · d3fc30d · d3fc30d
1 parent 50fa5f1
commit d3fc30d
Show file tree

Hide file tree

Showing 7 changed files with 87 additions and 5 deletions.
diff --git a/docs/pipelines.md b/docs/pipelines.md
@@ -1555,6 +1555,8 @@ spec:
 
 This creates a `Run/CustomRun` of a custom task of type `Example` in the `example.dev` API group with the version `v1alpha1`.
 
+Validation error will be returned if `apiVersion` or `kind` is missing.
+
 You can also specify the `name` of a custom task resource object previously defined in the cluster.
 
 ```yaml

diff --git a/pkg/apis/pipeline/v1/pipeline_types_test.go b/pkg/apis/pipeline/v1/pipeline_types_test.go
@@ -311,7 +311,7 @@ func TestPipelineTask_Validate_Failure(t *testing.T) {
 		expectedError apis.FieldError
 		wc            func(context.Context) context.Context
 	}{{
-		name: "invalid custom task without Kind",
+		name: "custom task reference in taskref missing apiversion Kind",
 		p: PipelineTask{
 			Name:    "invalid-custom-task",
 			TaskRef: &TaskRef{APIVersion: "example.com"},
@@ -320,7 +320,26 @@ func TestPipelineTask_Validate_Failure(t *testing.T) {
 			Message: `invalid value: custom task ref must specify kind`,
 			Paths:   []string{"taskRef.kind"},
 		},
-	}}
+	}, {
+		name: "custom task reference in taskspec missing kind",
+		p: PipelineTask{Name: "foo", TaskSpec: &EmbeddedTask{
+			TypeMeta: runtime.TypeMeta{
+				APIVersion: "example.com",
+			}}},
+		expectedError: *apis.ErrInvalidValue("custom task spec must specify kind", "taskSpec.kind"),
+	}, {
+		name:          "custom task reference in taskref missing apiversion",
+		p:             PipelineTask{Name: "foo", TaskRef: &TaskRef{Kind: "Example", Name: ""}},
+		expectedError: *apis.ErrInvalidValue("custom task ref must specify apiVersion", "taskRef.apiVersion"),
+	}, {
+		name: "custom task reference in taskspec missing apiversion",
+		p: PipelineTask{Name: "foo", TaskSpec: &EmbeddedTask{
+			TypeMeta: runtime.TypeMeta{
+				Kind: "Example",
+			}}},
+		expectedError: *apis.ErrInvalidValue("custom task spec must specify apiVersion", "taskSpec.apiVersion"),
+	},
+	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			ctx := context.Background()

diff --git a/pkg/apis/pipeline/v1/pipeline_validation.go b/pkg/apis/pipeline/v1/pipeline_validation.go
@@ -126,11 +126,20 @@ func (pt PipelineTask) Validate(ctx context.Context) (errs *apis.FieldError) {
 	errs = errs.Also(pt.validateRefOrSpec())
 
 	errs = errs.Also(pt.validateEmbeddedOrType())
-
+	// taskKinds contains the kinds when the apiVersion is not set, they are not custom tasks,
+	// if apiVersion is set they are custom tasks.
+	taskKinds := map[TaskKind]bool{
+		"":                 true,
+		NamespacedTaskKind: true,
+	}
 	// Pipeline task having taskRef/taskSpec with APIVersion is classified as custom task
 	switch {
+	case pt.TaskRef != nil && !taskKinds[pt.TaskRef.Kind]:
+		errs = errs.Also(pt.validateCustomTask())
 	case pt.TaskRef != nil && pt.TaskRef.APIVersion != "":
 		errs = errs.Also(pt.validateCustomTask())
+	case pt.TaskSpec != nil && !taskKinds[TaskKind(pt.TaskSpec.Kind)]:
+		errs = errs.Also(pt.validateCustomTask())
 	case pt.TaskSpec != nil && pt.TaskSpec.APIVersion != "":
 		errs = errs.Also(pt.validateCustomTask())
 	default:

diff --git a/pkg/apis/pipeline/v1/pipeline_validation_test.go b/pkg/apis/pipeline/v1/pipeline_validation_test.go
@@ -69,6 +69,14 @@ func TestPipeline_Validate_Success(t *testing.T) {
 				}},
 			},
 		},
+	}, {
+		name: "valid Task without apiversion",
+		p: &Pipeline{
+			ObjectMeta: metav1.ObjectMeta{Name: "pipeline"},
+			Spec: PipelineSpec{
+				Tasks: []PipelineTask{{Name: "foo", TaskRef: &TaskRef{Name: "bar", Kind: NamespacedTaskKind}}},
+			},
+		},
 	}}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

diff --git a/pkg/apis/pipeline/v1beta1/pipeline_types_test.go b/pkg/apis/pipeline/v1beta1/pipeline_types_test.go
@@ -339,7 +339,7 @@ func TestPipelineTask_Validate_Failure(t *testing.T) {
 		expectedError apis.FieldError
 		wc            func(context.Context) context.Context
 	}{{
-		name: "invalid custom task without Kind",
+		name: "custom task reference in taskref missing apiversion Kind",
 		p: PipelineTask{
 			Name:    "invalid-custom-task",
 			TaskRef: &TaskRef{APIVersion: "example.com"},
@@ -348,6 +348,24 @@ func TestPipelineTask_Validate_Failure(t *testing.T) {
 			Message: `invalid value: custom task ref must specify kind`,
 			Paths:   []string{"taskRef.kind"},
 		},
+	}, {
+		name: "custom task reference in taskspec missing kind",
+		p: PipelineTask{Name: "foo", TaskSpec: &EmbeddedTask{
+			TypeMeta: runtime.TypeMeta{
+				APIVersion: "example.com",
+			}}},
+		expectedError: *apis.ErrInvalidValue("custom task spec must specify kind", "taskSpec.kind"),
+	}, {
+		name:          "custom task reference in taskref missing apiversion",
+		p:             PipelineTask{Name: "foo", TaskRef: &TaskRef{Kind: "Example", Name: ""}},
+		expectedError: *apis.ErrInvalidValue("custom task ref must specify apiVersion", "taskRef.apiVersion"),
+	}, {
+		name: "custom task reference in taskspec missing apiversion",
+		p: PipelineTask{Name: "foo", TaskSpec: &EmbeddedTask{
+			TypeMeta: runtime.TypeMeta{
+				Kind: "Example",
+			}}},
+		expectedError: *apis.ErrInvalidValue("custom task spec must specify apiVersion", "taskSpec.apiVersion"),
 	}, {
 		name: "invalid bundle without bundle name",
 		p: PipelineTask{

diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation.go b/pkg/apis/pipeline/v1beta1/pipeline_validation.go
@@ -134,12 +134,22 @@ func (pt PipelineTask) Validate(ctx context.Context) (errs *apis.FieldError) {
 	if pt.Resources != nil {
 		errs = errs.Also(apis.ErrDisallowedFields("resources"))
 	}
-
+	// taskKinds contains the kinds when the apiVersion is not set, they are not custom tasks,
+	// if apiVersion is set they are custom tasks.
+	taskKinds := map[TaskKind]bool{
+		"":                 true,
+		NamespacedTaskKind: true,
+		ClusterTaskKind:    true,
+	}
 	cfg := config.FromContextOrDefaults(ctx)
 	// Pipeline task having taskRef/taskSpec with APIVersion is classified as custom task
 	switch {
+	case pt.TaskRef != nil && !taskKinds[pt.TaskRef.Kind]:
+		errs = errs.Also(pt.validateCustomTask())
 	case pt.TaskRef != nil && pt.TaskRef.APIVersion != "":
 		errs = errs.Also(pt.validateCustomTask())
+	case pt.TaskSpec != nil && !taskKinds[TaskKind(pt.TaskSpec.Kind)]:
+		errs = errs.Also(pt.validateCustomTask())
 	case pt.TaskSpec != nil && pt.TaskSpec.APIVersion != "":
 		errs = errs.Also(pt.validateCustomTask())
 		// If EnableTektonOCIBundles feature flag is on, validate bundle specifications

diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go
@@ -71,6 +71,22 @@ func TestPipeline_Validate_Success(t *testing.T) {
 				}},
 			},
 		},
+	}, {
+		name: "valid Task without apiversion",
+		p: &Pipeline{
+			ObjectMeta: metav1.ObjectMeta{Name: "pipeline"},
+			Spec: PipelineSpec{
+				Tasks: []PipelineTask{{Name: "foo", TaskRef: &TaskRef{Name: "bar", Kind: NamespacedTaskKind}}},
+			},
+		},
+	}, {
+		name: "valid Cluster Task without apiversion",
+		p: &Pipeline{
+			ObjectMeta: metav1.ObjectMeta{Name: "pipeline"},
+			Spec: PipelineSpec{
+				Tasks: []PipelineTask{{Name: "foo", TaskRef: &TaskRef{Name: "task", Kind: ClusterTaskKind}}},
+			},
+		},
 	}}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {