From 8cdd12ddb394e9f5a8aedd2c6b2089a6be684e7b Mon Sep 17 00:00:00 2001 From: qingliu 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 | 29 +++++++++++++++++++ 6 files changed, 71 insertions(+), 15 deletions(-) diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 71d1e776f1c..1e8d9a64a85 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/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 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 564a5160996..23717c6e9ae 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelineref_test.go +++ b/pkg/reconciler/pipelinerun/resources/pipelineref_test.go @@ -315,7 +315,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{ @@ -323,7 +323,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{ @@ -331,7 +331,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{ @@ -339,7 +339,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) { @@ -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 diff --git a/pkg/reconciler/taskrun/resources/taskref.go b/pkg/reconciler/taskrun/resources/taskref.go index 14317583240..9cec8346a9f 100644 --- a/pkg/reconciler/taskrun/resources/taskref.go +++ b/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 diff --git a/pkg/reconciler/taskrun/resources/taskref_test.go b/pkg/reconciler/taskrun/resources/taskref_test.go index 27379a2f94b..f2955d2b685 100644 --- a/pkg/reconciler/taskrun/resources/taskref_test.go +++ b/pkg/reconciler/taskrun/resources/taskref_test.go @@ -807,7 +807,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{ @@ -815,7 +815,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{ @@ -823,7 +823,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{ @@ -831,7 +831,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) { @@ -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 diff --git a/test/parse/yaml.go b/test/parse/yaml.go index 8b0a30e11c7..5ba3990e8e7 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,13 @@ 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 { + 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 +97,13 @@ 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 { + 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 +148,13 @@ 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 { + 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 +166,13 @@ 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 { + 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()