Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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