From fe42657aee2bc4c966df788c500ee1d3571f5bed Mon Sep 17 00:00:00 2001
From: qingliu <qingliu@alauda.io>
Date: Tue, 19 Mar 2024 14:08:06 +0800
Subject: [PATCH] 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.
---
.../pipelinerun/pipelinerun_test.go | 10 +++---
.../pipelinerun/resources/pipelineref.go | 4 +++
.../pipelinerun/resources/pipelineref_test.go | 22 ++++++++++---
pkg/reconciler/taskrun/resources/taskref.go | 5 +++
.../taskrun/resources/taskref_test.go | 16 ++++++---
test/parse/yaml.go | 33 +++++++++++++++++++
6 files changed, 75 insertions(+), 15 deletions(-)
diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go
index de293772183..b7c72df80ea 100644
--- a/pkg/reconciler/pipelinerun/pipelinerun_test.go
+++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go
@@ -15923,7 +15923,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
@@ -15947,7 +15947,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
@@ -16260,7 +16260,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
@@ -16284,7 +16284,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
@@ -16416,7 +16416,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
diff --git a/pkg/reconciler/pipelinerun/resources/pipelineref.go b/pkg/reconciler/pipelinerun/resources/pipelineref.go
index a6674f15483..71fd76669dd 100644
--- a/pkg/reconciler/pipelinerun/resources/pipelineref.go
+++ b/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
diff --git a/pkg/reconciler/pipelinerun/resources/pipelineref_test.go b/pkg/reconciler/pipelinerun/resources/pipelineref_test.go
index 93d01a52ecc..9b173bc49dc 100644
--- a/pkg/reconciler/pipelinerun/resources/pipelineref_test.go
+++ b/pkg/reconciler/pipelinerun/resources/pipelineref_test.go
@@ -316,7 +316,7 @@ 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{
@@ -324,7 +324,7 @@ func TestGetPipelineFunc_RemoteResolution(t *testing.T) {
"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{
@@ -332,7 +332,7 @@ func TestGetPipelineFunc_RemoteResolution(t *testing.T) {
"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{
@@ -340,7 +340,7 @@ func TestGetPipelineFunc_RemoteResolution(t *testing.T) {
"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) {
@@ -433,7 +433,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",
@@ -1315,9 +1315,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
diff --git a/pkg/reconciler/taskrun/resources/taskref.go b/pkg/reconciler/taskrun/resources/taskref.go
index 368ce8b78c7..7bddf0b0523 100644
--- a/pkg/reconciler/taskrun/resources/taskref.go
+++ b/pkg/reconciler/taskrun/resources/taskref.go
@@ -233,6 +233,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
@@ -251,6 +252,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
@@ -259,6 +261,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
diff --git a/pkg/reconciler/taskrun/resources/taskref_test.go b/pkg/reconciler/taskrun/resources/taskref_test.go
index e2d6ca79e24..1f81947dd95 100644
--- a/pkg/reconciler/taskrun/resources/taskref_test.go
+++ b/pkg/reconciler/taskrun/resources/taskref_test.go
@@ -1062,7 +1062,7 @@ 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{
@@ -1070,7 +1070,7 @@ func TestGetTaskFunc_RemoteResolution(t *testing.T) {
"apiVersion: tekton.dev/v1beta1",
taskYAMLString,
}, "\n"),
- wantTask: parse.MustParseV1Task(t, taskYAMLString),
+ wantTask: parse.MustParseV1TaskAndSetDefaults(t, taskYAMLString),
}, {
name: "v1 task",
taskYAML: strings.Join([]string{
@@ -1078,7 +1078,7 @@ func TestGetTaskFunc_RemoteResolution(t *testing.T) {
"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{
@@ -1086,7 +1086,7 @@ func TestGetTaskFunc_RemoteResolution(t *testing.T) {
"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) {
@@ -1189,7 +1189,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",
@@ -1899,6 +1899,12 @@ var taskYAMLString = `
metadata:
name: foo
spec:
+ params:
+ - name: array
+ # type: array
+ default:
+ - "bar"
+ - "bar"
steps:
- name: step1
image: ubuntu
diff --git a/test/parse/yaml.go b/test/parse/yaml.go
index 8b0a30e11c7..68bc16e14b5 100644
--- a/test/parse/yaml.go
+++ b/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()