Skip to content

Commit

Permalink
fix: ensure default type for params in remote tasks to prevent pipeli…
Browse files Browse the repository at this point in the history
…ne 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.
  • Loading branch information
l-qing committed Apr 23, 2024
1 parent 4fe52d4 commit 34fcb87
Show file tree
Hide file tree
Showing 6 changed files with 75 additions and 15 deletions.
10 changes: 5 additions & 5 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions pkg/reconciler/pipelinerun/resources/pipelineref.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
22 changes: 17 additions & 5 deletions pkg/reconciler/pipelinerun/resources/pipelineref_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -316,31 +316,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) {
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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
Expand Down
5 changes: 5 additions & 0 deletions pkg/reconciler/taskrun/resources/taskref.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down
16 changes: 11 additions & 5 deletions pkg/reconciler/taskrun/resources/taskref_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1062,31 +1062,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) {
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -1899,6 +1899,12 @@ var taskYAMLString = `
metadata:
name: foo
spec:
params:
- name: array
# type: array
default:
- "bar"
- "bar"
steps:
- name: step1
image: ubuntu
Expand Down
33 changes: 33 additions & 0 deletions test/parse/yaml.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package parse

import (
"context"
"testing"

v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
Expand Down Expand Up @@ -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()
Expand All @@ -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()
Expand Down Expand Up @@ -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()
Expand All @@ -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()
Expand Down

0 comments on commit 34fcb87

Please sign in to comment.