fix: ensure default type for params in remote tasks to prevent pipeline failures

fix #7775

In the existing logic, resources used for ConvertTo should have default values set.
Otherwise, there could be issues with incorrect parameter types being set
(e.g., an array type being treated as a string type).

However, resources fetched from remote sources haven't undergone the SetDefaults
operation. If we directly invoke the ConvertTo operation, it might result in
erroneous outcomes.

For instance, a v1beta1 ClusterTask that undergoes a direct ConvertTo to convert
the resource into a v1 Task for validation might be mistakenly considered invalid.

Additionally, even if a v1beta1 Task passes validation, the process of converting
it to a v1 Task could still incorrectly set default parameter types, leading to
errors during execution.

Author: l-qing

pkg/reconciler/pipelinerun/pipelinerun_test.go

@@ -15918,7 +15918,7 @@ spec:
 func TestReconcile_verifyResolved_V1beta1Pipeline_NoError(t *testing.T) {
 	resolverName := "foobar"
 
-	ts := parse.MustParseV1beta1Task(t, `
+	ts := parse.MustParseV1beta1TaskAndSetDefaults(t, `
 metadata:
   name: test-task
   namespace: foo
@@ -15942,7 +15942,7 @@ spec:
 		t.Fatal("fail to marshal task", err)
 	}
 
-	ps := parse.MustParseV1beta1Pipeline(t, fmt.Sprintf(`
+	ps := parse.MustParseV1beta1PipelineAndSetDefaults(t, fmt.Sprintf(`
 metadata:
   name: test-pipeline
   namespace: foo
@@ -16252,7 +16252,7 @@ spec:
 func TestReconcile_verifyResolved_V1Pipeline_NoError(t *testing.T) {
 	resolverName := "foobar"
 
-	ts := parse.MustParseV1Task(t, `
+	ts := parse.MustParseV1TaskAndSetDefaults(t, `
 metadata:
   name: test-task
   namespace: foo
@@ -16276,7 +16276,7 @@ spec:
 		t.Fatal("fail to marshal task", err)
 	}
 
-	ps := parse.MustParseV1Pipeline(t, fmt.Sprintf(`
+	ps := parse.MustParseV1PipelineAndSetDefaults(t, fmt.Sprintf(`
 metadata:
   name: test-pipeline
   namespace: foo
@@ -16405,7 +16405,7 @@ func TestReconcile_verifyResolved_V1Pipeline_Error(t *testing.T) {
 	resolverName := "foobar"
 
 	// Case1: unsigned Pipeline refers to unsigned Task
-	unsignedTask := parse.MustParseV1beta1Task(t, `
+	unsignedTask := parse.MustParseV1beta1TaskAndSetDefaults(t, `
 metadata:
   name: test-task
   namespace: foo

pkg/reconciler/pipelinerun/resources/pipelineref.go

@@ -136,6 +136,7 @@ func resolvePipeline(ctx context.Context, resolver remote.Resolver, name string,
 func readRuntimeObjectAsPipeline(ctx context.Context, namespace string, obj runtime.Object, k8s kubernetes.Interface, tekton clientset.Interface, refSource *v1.RefSource, verificationPolicies []*v1alpha1.VerificationPolicy) (*v1.Pipeline, *trustedresources.VerificationResult, error) {
 	switch obj := obj.(type) {
 	case *v1beta1.Pipeline:
+		obj.SetDefaults(ctx)
 		// Verify the Pipeline once we fetch from the remote resolution, mutating, validation and conversion of the pipeline should happen after the verification, since signatures are based on the remote pipeline contents
 		vr := trustedresources.VerifyResource(ctx, obj, k8s, refSource, verificationPolicies)
 		// Issue a dry-run request to create the remote Pipeline, so that it can undergo validation from validating admission webhooks
@@ -154,6 +155,9 @@ func readRuntimeObjectAsPipeline(ctx context.Context, namespace string, obj runt
 		}
 		return p, &vr, nil
 	case *v1.Pipeline:
+		// This SetDefaults is currently not necessary, but for consistency, it is recommended to add it.
+		// Avoid forgetting to add it in the future when there is a v2 version, causing similar problems.
+		obj.SetDefaults(ctx)
 		vr := trustedresources.VerifyResource(ctx, obj, k8s, refSource, verificationPolicies)
 		// Issue a dry-run request to create the remote Pipeline, so that it can undergo validation from validating admission webhooks
 		// without actually creating the Pipeline on the cluster

pkg/reconciler/pipelinerun/resources/pipelineref_test.go

@@ -315,31 +315,31 @@ func TestGetPipelineFunc_RemoteResolution(t *testing.T) {
 			"apiVersion: tekton.dev/v1beta1",
 			pipelineYAMLString,
 		}, "\n"),
-		wantPipeline: parse.MustParseV1Pipeline(t, pipelineYAMLString),
+		wantPipeline: parse.MustParseV1PipelineAndSetDefaults(t, pipelineYAMLString),
 	}, {
 		name: "v1 pipeline",
 		pipelineYAML: strings.Join([]string{
 			"kind: Pipeline",
 			"apiVersion: tekton.dev/v1",
 			pipelineYAMLString,
 		}, "\n"),
-		wantPipeline: parse.MustParseV1Pipeline(t, pipelineYAMLString),
+		wantPipeline: parse.MustParseV1PipelineAndSetDefaults(t, pipelineYAMLString),
 	}, {
 		name: "v1 remote pipeline without defaults",
 		pipelineYAML: strings.Join([]string{
 			"kind: Pipeline",
 			"apiVersion: tekton.dev/v1",
 			pipelineYAMLStringWithoutDefaults,
 		}, "\n"),
-		wantPipeline: parse.MustParseV1Pipeline(t, pipelineYAMLStringWithoutDefaults),
+		wantPipeline: parse.MustParseV1PipelineAndSetDefaults(t, pipelineYAMLStringWithoutDefaults),
 	}, {
 		name: "v1 remote pipeline with different namespace",
 		pipelineYAML: strings.Join([]string{
 			"kind: Pipeline",
 			"apiVersion: tekton.dev/v1",
 			pipelineYAMLStringNamespaceFoo,
 		}, "\n"),
-		wantPipeline: parse.MustParseV1Pipeline(t, pipelineYAMLStringNamespaceFoo),
+		wantPipeline: parse.MustParseV1PipelineAndSetDefaults(t, pipelineYAMLStringNamespaceFoo),
 	}}
 	for _, tc := range testcases {
 		t.Run(tc.name, func(t *testing.T) {
@@ -432,7 +432,7 @@ func TestGetPipelineFunc_RemoteResolution_ReplacedParams(t *testing.T) {
 	ctx, clients := getFakePipelineClient(t)
 	cfg := config.FromContextOrDefaults(ctx)
 	ctx = config.ToContext(ctx, cfg)
-	pipeline := parse.MustParseV1Pipeline(t, pipelineYAMLString)
+	pipeline := parse.MustParseV1PipelineAndSetDefaults(t, pipelineYAMLString)
 	pipelineRef := &v1.PipelineRef{
 		ResolverRef: v1.ResolverRef{
 			Resolver: "git",
@@ -1307,9 +1307,21 @@ var pipelineYAMLString = `
 metadata:
   name: foo
 spec:
+  params:
+  - name: array
+    # type: array
+    default:
+      - "bar"
+      - "bar"
   tasks:
   - name: task1
     taskSpec:
+      params:
+      - name: array
+        # type: array
+        default:
+          - "bar"
+          - "bar"
       steps:
       - name: step1
         image: ubuntu

pkg/reconciler/taskrun/resources/taskref.go

@@ -192,6 +192,7 @@ func resolveStepAction(ctx context.Context, resolver remote.Resolver, name, name
 func readRuntimeObjectAsTask(ctx context.Context, namespace string, obj runtime.Object, k8s kubernetes.Interface, tekton clientset.Interface, refSource *v1.RefSource, verificationPolicies []*v1alpha1.VerificationPolicy) (*v1.Task, *trustedresources.VerificationResult, error) {
 	switch obj := obj.(type) {
 	case *v1beta1.Task:
+		obj.SetDefaults(ctx)
 		// Verify the Task once we fetch from the remote resolution, mutating, validation and conversion of the task should happen after the verification, since signatures are based on the remote task contents
 		vr := trustedresources.VerifyResource(ctx, obj, k8s, refSource, verificationPolicies)
 		// Issue a dry-run request to create the remote Task, so that it can undergo validation from validating admission webhooks
@@ -210,6 +211,7 @@ func readRuntimeObjectAsTask(ctx context.Context, namespace string, obj runtime.
 		}
 		return t, &vr, nil
 	case *v1beta1.ClusterTask:
+		obj.SetDefaults(ctx)
 		t, err := convertClusterTaskToTask(ctx, *obj)
 		// Issue a dry-run request to create the remote Task, so that it can undergo validation from validating admission webhooks
 		// without actually creating the Task on the cluster
@@ -218,6 +220,9 @@ func readRuntimeObjectAsTask(ctx context.Context, namespace string, obj runtime.
 		}
 		return t, nil, err
 	case *v1.Task:
+		// This SetDefaults is currently not necessary, but for consistency, it is recommended to add it.
+		// Avoid forgetting to add it in the future when there is a v2 version, causing similar problems.
+		obj.SetDefaults(ctx)
 		vr := trustedresources.VerifyResource(ctx, obj, k8s, refSource, verificationPolicies)
 		// Issue a dry-run request to create the remote Task, so that it can undergo validation from validating admission webhooks
 		// without actually creating the Task on the cluster

pkg/reconciler/taskrun/resources/taskref_test.go

@@ -807,31 +807,31 @@ func TestGetTaskFunc_RemoteResolution(t *testing.T) {
 			"apiVersion: tekton.dev/v1beta1",
 			taskYAMLString,
 		}, "\n"),
-		wantTask: parse.MustParseV1Task(t, taskYAMLString),
+		wantTask: parse.MustParseV1TaskAndSetDefaults(t, taskYAMLString),
 	}, {
 		name: "v1beta1 cluster task",
 		taskYAML: strings.Join([]string{
 			"kind: ClusterTask",
 			"apiVersion: tekton.dev/v1beta1",
 			taskYAMLString,
 		}, "\n"),
-		wantTask: parse.MustParseV1Task(t, taskYAMLString),
+		wantTask: parse.MustParseV1TaskAndSetDefaults(t, taskYAMLString),
 	}, {
 		name: "v1 task",
 		taskYAML: strings.Join([]string{
 			"kind: Task",
 			"apiVersion: tekton.dev/v1",
 			taskYAMLString,
 		}, "\n"),
-		wantTask: parse.MustParseV1Task(t, taskYAMLString),
+		wantTask: parse.MustParseV1TaskAndSetDefaults(t, taskYAMLString),
 	}, {
 		name: "v1 task without defaults",
 		taskYAML: strings.Join([]string{
 			"kind: Task",
 			"apiVersion: tekton.dev/v1",
 			remoteTaskYamlWithoutDefaults,
 		}, "\n"),
-		wantTask: parse.MustParseV1Task(t, remoteTaskYamlWithoutDefaults),
+		wantTask: parse.MustParseV1TaskAndSetDefaults(t, remoteTaskYamlWithoutDefaults),
 	}}
 	for _, tc := range testcases {
 		t.Run(tc.name, func(t *testing.T) {
@@ -934,7 +934,7 @@ func TestGetTaskFunc_RemoteResolution_ReplacedParams(t *testing.T) {
 	ctx := context.Background()
 	cfg := config.FromContextOrDefaults(ctx)
 	ctx = config.ToContext(ctx, cfg)
-	task := parse.MustParseV1Task(t, taskYAMLString)
+	task := parse.MustParseV1TaskAndSetDefaults(t, taskYAMLString)
 	taskRef := &v1.TaskRef{
 		ResolverRef: v1.ResolverRef{
 			Resolver: "git",
@@ -1639,6 +1639,12 @@ var taskYAMLString = `
 metadata:
   name: foo
 spec:
+  params:
+  - name: array
+    # type: array
+    default:
+      - "bar"
+      - "bar"
   steps:
   - name: step1
     image: ubuntu

test/parse/yaml.go

@@ -14,6 +14,7 @@
 package parse
 
 import (
+	"context"
 	"testing"
 
 	v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
@@ -67,6 +68,14 @@ kind: Task
 	return &task
 }
 
+// MustParseV1beta1TaskAndSetDefaults takes YAML and parses it into a *v1beta1.Task and sets defaults
+func MustParseV1beta1TaskAndSetDefaults(t *testing.T, yaml string) *v1beta1.Task {
+	t.Helper()
+	task := MustParseV1beta1Task(t, yaml)
+	task.SetDefaults(context.Background())
+	return task
+}
+
 // MustParseCustomRun takes YAML and parses it into a *v1beta1.CustomRun
 func MustParseCustomRun(t *testing.T, yaml string) *v1beta1.CustomRun {
 	t.Helper()
@@ -89,6 +98,14 @@ kind: Task
 	return &task
 }
 
+// MustParseV1TaskAndSetDefaults takes YAML and parses it into a *v1.Task and sets defaults
+func MustParseV1TaskAndSetDefaults(t *testing.T, yaml string) *v1.Task {
+	t.Helper()
+	task := MustParseV1Task(t, yaml)
+	task.SetDefaults(context.Background())
+	return task
+}
+
 // MustParseClusterTask takes YAML and parses it into a *v1beta1.ClusterTask
 func MustParseClusterTask(t *testing.T, yaml string) *v1beta1.ClusterTask {
 	t.Helper()
@@ -133,6 +150,14 @@ kind: Pipeline
 	return &pipeline
 }
 
+// MustParseV1beta1PipelineAndSetDefaults takes YAML and parses it into a *v1beta1.Pipeline and sets defaults
+func MustParseV1beta1PipelineAndSetDefaults(t *testing.T, yaml string) *v1beta1.Pipeline {
+	t.Helper()
+	p := MustParseV1beta1Pipeline(t, yaml)
+	p.SetDefaults(context.Background())
+	return p
+}
+
 // MustParseV1Pipeline takes YAML and parses it into a *v1.Pipeline
 func MustParseV1Pipeline(t *testing.T, yaml string) *v1.Pipeline {
 	t.Helper()
@@ -144,6 +169,14 @@ kind: Pipeline
 	return &pipeline
 }
 
+// MustParseV1PipelineAndSetDefaults takes YAML and parses it into a *v1.Pipeline and sets defaults
+func MustParseV1PipelineAndSetDefaults(t *testing.T, yaml string) *v1.Pipeline {
+	t.Helper()
+	p := MustParseV1Pipeline(t, yaml)
+	p.SetDefaults(context.Background())
+	return p
+}
+
 // MustParseVerificationPolicy takes YAML and parses it into a *v1alpha1.VerificationPolicy
 func MustParseVerificationPolicy(t *testing.T, yaml string) *v1alpha1.VerificationPolicy {
 	t.Helper()