From d4fe76dbc3519813719a138d2b6540fd8511d8d3 Mon Sep 17 00:00:00 2001 From: Yongxuan Zhang Date: Tue, 12 Dec 2023 18:38:37 +0000 Subject: [PATCH] add support for reserved default object param This commit adds support for reserved default object param.Users can reference to default object param values without needing to declaring them in remote Task&Pipeline. Validation err will be returned if referencing to non-existent object key. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com --- docs/pipeline-api.md | 6 + .../remote-pipeline-ref-reserved-param.yaml | 46 +++ .../alpha/remote-task-ref-reserved-param.yaml | 37 ++ pkg/apis/pipeline/v1/pipeline_validation.go | 10 +- .../pipeline/v1/pipeline_validation_test.go | 19 + pkg/apis/pipeline/v1/pipelinerun_types.go | 2 + pkg/apis/pipeline/v1/task_validation.go | 24 ++ pkg/apis/pipeline/v1/task_validation_test.go | 146 ++++++++ pkg/apis/pipeline/v1/taskrun_types.go | 2 + .../pipeline/v1beta1/pipeline_validation.go | 10 +- .../v1beta1/pipeline_validation_test.go | 19 + pkg/apis/pipeline/v1beta1/task_validation.go | 1 + .../pipeline/v1beta1/task_validation_test.go | 13 + pkg/reconciler/pipelinerun/pipelinerun.go | 14 + .../pipelinerun/pipelinerun_test.go | 67 +++- pkg/reconciler/taskrun/taskrun.go | 34 +- pkg/reconciler/taskrun/taskrun_test.go | 49 ++- test/default_params_test.go | 353 ++++++++++++++++++ 18 files changed, 817 insertions(+), 35 deletions(-) create mode 100644 examples/v1/pipelineruns/alpha/remote-pipeline-ref-reserved-param.yaml create mode 100644 examples/v1/taskruns/alpha/remote-task-ref-reserved-param.yaml create mode 100644 test/default_params_test.go diff --git a/docs/pipeline-api.md b/docs/pipeline-api.md index 4d09b848f60..f6ff6318217 100644 --- a/docs/pipeline-api.md +++ b/docs/pipeline-api.md @@ -2029,6 +2029,9 @@ misses some keys required for the object param declared in Pipeline spec.

ReasonParamArrayIndexingInvalid indicates that the use of param array indexing is not under correct api fields feature gate or the array is out of bound.

+

"ParamKeyNotExistent"

+

PipelineRunReasonParamKeyNotExistent indicates that the default object param doesn’t have the key which the param reference requires

+

"ParameterMissing"

ReasonParameterMissing indicates that the reason for the failure status is that the associated PipelineRun didn’t provide all the required parameters

@@ -5150,6 +5153,9 @@ TaskRuns failed due to reconciler/validation error should not use this reason.

"InvalidParamValue"

TaskRunReasonInvalidParamValue indicates that the TaskRun Param input value is not allowed.

+

"ParamKeyNotExistent"

+

TaskRunReasonParamKeyNotExistent indicates that the default object param doesn’t have the key which the param reference requires

+

"ResourceVerificationFailed"

TaskRunReasonResourceVerificationFailed indicates that the task fails the trusted resource verification, it could be the content has changed, signature is invalid or public key is invalid

diff --git a/examples/v1/pipelineruns/alpha/remote-pipeline-ref-reserved-param.yaml b/examples/v1/pipelineruns/alpha/remote-pipeline-ref-reserved-param.yaml new file mode 100644 index 00000000000..fd537dd22d0 --- /dev/null +++ b/examples/v1/pipelineruns/alpha/remote-pipeline-ref-reserved-param.yaml @@ -0,0 +1,46 @@ +apiVersion: tekton.dev/v1 +kind: Pipeline +metadata: + name: echo-message-pipeline +spec: + params: + - name: hello + properties: + project_id: + type: string + type: object + tasks: + - name: task1 + params: + - name: foo + value: + project_id: $(params.hello.project_id) + taskSpec: + params: + - name: foo + type: object + properties: + project_id: + type: string + steps: + - name: echo + image: ubuntu + script: | + #!/bin/bash + echo $(params.provider.project_id) + echo $(params.foo.project_id) +--- +kind: PipelineRun +apiVersion: tekton.dev/v1 +metadata: + name: test-pipelinerun +spec: + params: + - name: provider + value: + project_id: foo + - name: hello + value: + project_id: foo + pipelineRef: + name: echo-message-pipeline diff --git a/examples/v1/taskruns/alpha/remote-task-ref-reserved-param.yaml b/examples/v1/taskruns/alpha/remote-task-ref-reserved-param.yaml new file mode 100644 index 00000000000..a585d4a7ca1 --- /dev/null +++ b/examples/v1/taskruns/alpha/remote-task-ref-reserved-param.yaml @@ -0,0 +1,37 @@ +kind: Task +apiVersion: tekton.dev/v1 +metadata: + name: example-task +spec: + params: + - name: foo + type: object + properties: + project_id: + type: string + digest: + type: string + steps: + - image: ubuntu + name: echo + script: | + echo -n $(params.foo.project_id) + echo -n $(params.provider.project_id) +--- +kind: TaskRun +apiVersion: tekton.dev/v1 +metadata: + name: example-tr +spec: + params: + - name: foo + value: + project_id: foo + digest: bar + - name: provider + value: + project_id: foo + digest: bar + taskRef: + name: example-task + kind: task diff --git a/pkg/apis/pipeline/v1/pipeline_validation.go b/pkg/apis/pipeline/v1/pipeline_validation.go index 9c000c673e8..e576ff7b814 100644 --- a/pkg/apis/pipeline/v1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1/pipeline_validation.go @@ -37,10 +37,11 @@ var _ apis.Validatable = (*Pipeline)(nil) var _ resourcesemantics.VerbLimited = (*Pipeline)(nil) const ( - taskRef = "taskRef" - taskSpec = "taskSpec" - pipelineRef = "pipelineRef" - pipelineSpec = "pipelineSpec" + taskRef = "taskRef" + taskSpec = "taskSpec" + pipelineRef = "pipelineRef" + pipelineSpec = "pipelineSpec" + ReservedParamName = "provider" ) // SupportedVerbs returns the operations that validation should be called for @@ -389,6 +390,7 @@ func (ps *PipelineSpec) validatePipelineParameterUsage(ctx context.Context) (err // validatePipelineTaskParameterUsage validates that parameters referenced in the Pipeline Tasks are declared by the Pipeline func validatePipelineTaskParameterUsage(tasks []PipelineTask, params ParamSpecs) (errs *apis.FieldError) { allParamNames := sets.NewString(params.GetNames()...) + allParamNames.Insert(ReservedParamName) _, arrayParams, objectParams := params.SortByType() arrayParamNames := sets.NewString(arrayParams.GetNames()...) objectParameterNameKeys := map[string][]string{} diff --git a/pkg/apis/pipeline/v1/pipeline_validation_test.go b/pkg/apis/pipeline/v1/pipeline_validation_test.go index cf7344a7b35..7de62d96900 100644 --- a/pkg/apis/pipeline/v1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1/pipeline_validation_test.go @@ -18,6 +18,7 @@ package v1 import ( "context" + "fmt" "testing" "github.com/google/go-cmp/cmp" @@ -114,6 +115,24 @@ func TestPipeline_Validate_Success(t *testing.T) { }, }, }, + }, { + name: "valid reserved param", + wc: cfgtesting.EnableAlphaAPIFields, + p: &Pipeline{ + ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, + Spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "pipeline-task-use-reserved-param", + TaskSpec: &EmbeddedTask{TaskSpec: TaskSpec{ + Steps: []Step{{ + Name: "some-step", + Image: "some-image", + Script: fmt.Sprintf("echo $(params.%s.foo)", ReservedParamName), + }}, + }}, + }}, + }, + }, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/apis/pipeline/v1/pipelinerun_types.go b/pkg/apis/pipeline/v1/pipelinerun_types.go index 1cc3b2e6dbc..db40707afdb 100644 --- a/pkg/apis/pipeline/v1/pipelinerun_types.go +++ b/pkg/apis/pipeline/v1/pipelinerun_types.go @@ -417,6 +417,8 @@ const ( PipelineRunReasonCELEvaluationFailed PipelineRunReason = "CELEvaluationFailed" // PipelineRunReasonInvalidParamValue indicates that the PipelineRun Param input value is not allowed. PipelineRunReasonInvalidParamValue PipelineRunReason = "InvalidParamValue" + // PipelineRunReasonParamKeyNotExistent indicates that the default object param doesn't have the key which the param reference requires + PipelineRunReasonParamKeyNotExistent PipelineRunReason = "ParamKeyNotExistent" ) // PipelineTaskOnErrorAnnotation is used to pass the failure strategy to TaskRun pods from PipelineTask OnError field diff --git a/pkg/apis/pipeline/v1/task_validation.go b/pkg/apis/pipeline/v1/task_validation.go index 23f7178645e..6b71759e56e 100644 --- a/pkg/apis/pipeline/v1/task_validation.go +++ b/pkg/apis/pipeline/v1/task_validation.go @@ -126,6 +126,7 @@ func ValidateUsageOfDeclaredParameters(ctx context.Context, steps []Step, params var errs *apis.FieldError _, _, objectParams := params.SortByType() allParameterNames := sets.NewString(params.GetNames()...) + allParameterNames.Insert(ReservedParamName) errs = errs.Also(validateVariables(ctx, steps, "params", allParameterNames)) errs = errs.Also(validateObjectUsage(ctx, steps, objectParams)) errs = errs.Also(ValidateObjectParamsHaveProperties(ctx, params)) @@ -762,3 +763,26 @@ func ValidateStepResultsVariables(ctx context.Context, results []StepResult, scr errs = errs.Also(substitution.ValidateNoReferencesToUnknownVariables(script, "results", resultsNames).ViaField("script")) return errs } + +// Reserved Param doesn't have properties, so we need to check all the references if they are using a non-existent key from the param +func ValidateReservedParamReferenceMissingKeys(ctx context.Context, steps []Step, params Params) error { + objectKeys := sets.NewString() + // collect all the existent keys from the object reserved param. + for _, p := range params { + if p.Name == ReservedParamName && p.Value.Type == ParamTypeObject { + for key := range p.Value.ObjectVal { + objectKeys.Insert(key) + } + } + } + + // check if the object's key names are referenced correctly i.e. param.objectParam.key1 + if len(objectKeys) > 0 { + err := validateVariables(ctx, steps, fmt.Sprintf("params\\.%s", ReservedParamName), objectKeys) + if err != nil { + return err + } + } + + return nil +} diff --git a/pkg/apis/pipeline/v1/task_validation_test.go b/pkg/apis/pipeline/v1/task_validation_test.go index 27a830faa54..33c5f2e1040 100644 --- a/pkg/apis/pipeline/v1/task_validation_test.go +++ b/pkg/apis/pipeline/v1/task_validation_test.go @@ -61,6 +61,19 @@ func TestTaskValidate(t *testing.T) { }}, }, }, + }, { + name: "valid reserved param", + wc: cfgtesting.EnableAlphaAPIFields, + t: &v1.Task{ + ObjectMeta: metav1.ObjectMeta{Name: "task"}, + Spec: v1.TaskSpec{ + Steps: []v1.Step{{ + Name: "some-step", + Image: "some-image", + Script: fmt.Sprintf("echo $(params.%s.foo)", v1.ReservedParamName), + }}, + }, + }, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -2726,3 +2739,136 @@ func TestTaskSpecValidate_StepResults_Error(t *testing.T) { }) } } + +func TestValidateReservedParamReferenceMissingKeys_Success(t *testing.T) { + tests := []struct { + name string + steps []v1.Step + params []v1.Param + }{{ + name: "reference default param in all step fields", + steps: []v1.Step{{ + Name: fmt.Sprintf("echo $(params.%s.foo)", v1.ReservedParamName), + Image: fmt.Sprintf("$(params.%s.foo)", v1.ReservedParamName), + Args: []string{fmt.Sprintf("echo $(params.%s.foo)", v1.ReservedParamName)}, + Command: []string{fmt.Sprintf("echo $(params.%s.foo)", v1.ReservedParamName)}, + WorkingDir: fmt.Sprintf("echo $(params.%s.foo)", v1.ReservedParamName), + VolumeMounts: []corev1.VolumeMount{{ + Name: fmt.Sprintf("echo $(params.%s.foo)", v1.ReservedParamName), + }}, + Script: fmt.Sprintf("echo $(params.%s.foo)", v1.ReservedParamName), + }}, + params: []v1.Param{{ + Name: v1.ReservedParamName, + Value: *v1.NewObject(map[string]string{"foo": "bar"}), + }}, + }, { + name: "don't check non-reserved param", + steps: []v1.Step{{ + Name: fmt.Sprintf("echo $(params.%s.foo)", "non-reserved"), + Image: fmt.Sprintf("$(params.%s.foo)", "non-reserved"), + Args: []string{fmt.Sprintf("echo $(params.%s.foo)", "non-reserved")}, + Command: []string{fmt.Sprintf("echo $(params.%s.foo)", "non-reserved")}, + WorkingDir: fmt.Sprintf("echo $(params.%s.foo)", "non-reserved"), + VolumeMounts: []corev1.VolumeMount{{ + Name: fmt.Sprintf("echo $(params.%s.foo)", "non-reserved"), + }}, + Script: fmt.Sprintf("echo $(params.%s.foo)", "non-reserved"), + }}, + params: []v1.Param{{ + Name: v1.ReservedParamName, + Value: *v1.NewObject(map[string]string{"foo": "bar"}), + }}, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + err := v1.ValidateReservedParamReferenceMissingKeys(ctx, tt.steps, tt.params) + if err != nil { + t.Errorf("ValidateReservedParamReferenceMissingKeys() = %v", err) + } + }) + } +} + +func TestValidateReservedParamReferenceMissingKeys_Error(t *testing.T) { + tests := []struct { + name string + steps []v1.Step + params []v1.Param + }{{ + name: "reference non-existent key in name", + steps: []v1.Step{{ + Name: fmt.Sprintf("echo $(params.%s.%s)", v1.ReservedParamName, "non-existent"), + }}, + params: []v1.Param{{ + Name: v1.ReservedParamName, + Value: *v1.NewObject(map[string]string{"foo": "bar"}), + }}, + }, { + name: "reference non-existent key in image", + steps: []v1.Step{{ + Image: fmt.Sprintf("$(params.%s.%s)", v1.ReservedParamName, "non-existent"), + }}, + params: []v1.Param{{ + Name: v1.ReservedParamName, + Value: *v1.NewObject(map[string]string{"foo": "bar"}), + }}, + }, { + name: "reference non-existent key in args", + steps: []v1.Step{{ + Args: []string{fmt.Sprintf("echo $(params.%s.%s)", v1.ReservedParamName, "non-existent")}, + }}, + params: []v1.Param{{ + Name: v1.ReservedParamName, + Value: *v1.NewObject(map[string]string{"foo": "bar"}), + }}, + }, { + name: "reference non-existent key in command", + steps: []v1.Step{{ + Command: []string{fmt.Sprintf("echo $(params.%s.%s)", v1.ReservedParamName, "non-existent")}, + }}, + params: []v1.Param{{ + Name: v1.ReservedParamName, + Value: *v1.NewObject(map[string]string{"foo": "bar"}), + }}, + }, { + name: "reference non-existent key in workingdir", + steps: []v1.Step{{ + WorkingDir: fmt.Sprintf("echo $(params.%s.%s)", v1.ReservedParamName, "non-existent"), + }}, + params: []v1.Param{{ + Name: v1.ReservedParamName, + Value: *v1.NewObject(map[string]string{"foo": "bar"}), + }}, + }, { + name: "reference non-existent key in volume", + steps: []v1.Step{{ + VolumeMounts: []corev1.VolumeMount{{ + Name: fmt.Sprintf("echo $(params.%s.%s)", v1.ReservedParamName, "non-existent"), + }}, + }}, + params: []v1.Param{{ + Name: v1.ReservedParamName, + Value: *v1.NewObject(map[string]string{"foo": "bar"}), + }}, + }, { + name: "reference non-existent key in script", + steps: []v1.Step{{ + Script: fmt.Sprintf("echo $(params.%s.%s)", v1.ReservedParamName, "non-existent"), + }}, + params: []v1.Param{{ + Name: v1.ReservedParamName, + Value: *v1.NewObject(map[string]string{"foo": "bar"}), + }}, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + err := v1.ValidateReservedParamReferenceMissingKeys(ctx, tt.steps, tt.params) + if err == nil { + t.Errorf("expected error but got nil") + } + }) + } +} diff --git a/pkg/apis/pipeline/v1/taskrun_types.go b/pkg/apis/pipeline/v1/taskrun_types.go index 7c3cf232ee5..785001edb8b 100644 --- a/pkg/apis/pipeline/v1/taskrun_types.go +++ b/pkg/apis/pipeline/v1/taskrun_types.go @@ -205,6 +205,8 @@ const ( // TaskRunReasonFailureIgnored is the reason set when the Taskrun has failed due to pod execution error and the failure is ignored for the owning PipelineRun. // TaskRuns failed due to reconciler/validation error should not use this reason. TaskRunReasonFailureIgnored TaskRunReason = "FailureIgnored" + // TaskRunReasonParamKeyNotExistent indicates that the default object param doesn't have the key which the param reference requires + TaskRunReasonParamKeyNotExistent TaskRunReason = "ParamKeyNotExistent" ) func (t TaskRunReason) String() string { diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation.go b/pkg/apis/pipeline/v1beta1/pipeline_validation.go index 69d39e09546..f8fb98cdcc4 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation.go @@ -38,10 +38,11 @@ var _ apis.Validatable = (*Pipeline)(nil) var _ resourcesemantics.VerbLimited = (*Pipeline)(nil) const ( - taskRef = "taskRef" - taskSpec = "taskSpec" - pipelineRef = "pipelineRef" - pipelineSpec = "pipelineSpec" + taskRef = "taskRef" + taskSpec = "taskSpec" + pipelineRef = "pipelineRef" + pipelineSpec = "pipelineSpec" + ReservedParamName = "provider" ) // SupportedVerbs returns the operations that validation should be called for @@ -408,6 +409,7 @@ func (ps *PipelineSpec) validatePipelineParameterUsage(ctx context.Context) (err // validatePipelineTaskParameterUsage validates that parameters referenced in the Pipeline Tasks are declared by the Pipeline func validatePipelineTaskParameterUsage(tasks []PipelineTask, params ParamSpecs) (errs *apis.FieldError) { allParamNames := sets.NewString(params.getNames()...) + allParamNames.Insert(ReservedParamName) _, arrayParams, objectParams := params.sortByType() arrayParamNames := sets.NewString(arrayParams.getNames()...) objectParameterNameKeys := map[string][]string{} diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go index 01d6995f668..16a59967733 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go @@ -18,6 +18,7 @@ package v1beta1 import ( "context" + "fmt" "testing" "github.com/google/go-cmp/cmp" @@ -116,6 +117,24 @@ func TestPipeline_Validate_Success(t *testing.T) { }, }, }, + }, { + name: "valid reserved param", + wc: cfgtesting.EnableAlphaAPIFields, + p: &Pipeline{ + ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, + Spec: PipelineSpec{ + Tasks: []PipelineTask{{ + Name: "pipeline-task-use-reserved-param", + TaskSpec: &EmbeddedTask{TaskSpec: TaskSpec{ + Steps: []Step{{ + Name: "some-step", + Image: "some-image", + Script: fmt.Sprintf("echo $(params.%s.foo)", ReservedParamName), + }}, + }}, + }}, + }, + }, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/apis/pipeline/v1beta1/task_validation.go b/pkg/apis/pipeline/v1beta1/task_validation.go index c9a5e5a9ec9..303003d7164 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation.go +++ b/pkg/apis/pipeline/v1beta1/task_validation.go @@ -129,6 +129,7 @@ func ValidateUsageOfDeclaredParameters(ctx context.Context, steps []Step, params var errs *apis.FieldError _, _, objectParams := params.sortByType() allParameterNames := sets.NewString(params.getNames()...) + allParameterNames.Insert(ReservedParamName) errs = errs.Also(validateVariables(ctx, steps, "params", allParameterNames)) errs = errs.Also(validateObjectUsage(ctx, steps, objectParams)) errs = errs.Also(validateObjectParamsHaveProperties(ctx, params)) diff --git a/pkg/apis/pipeline/v1beta1/task_validation_test.go b/pkg/apis/pipeline/v1beta1/task_validation_test.go index bee77c71b69..de70ccc160e 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/task_validation_test.go @@ -65,6 +65,19 @@ func TestTaskValidate(t *testing.T) { }}, }, }, + }, { + name: "valid reserved param", + wc: cfgtesting.EnableAlphaAPIFields, + t: &v1beta1.Task{ + ObjectMeta: metav1.ObjectMeta{Name: "task"}, + Spec: v1beta1.TaskSpec{ + Steps: []v1beta1.Step{{ + Name: "some-step", + Image: "some-image", + Script: fmt.Sprintf("echo $(params.%s.foo)", v1.ReservedParamName), + }}, + }, + }, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index a6003474fd2..6d93f30e247 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -680,6 +680,20 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1.PipelineRun, getPipel } if pipelineRunFacts.State.IsBeforeFirstTaskRun() { + // check if the object's key names are referenced correctly i.e. param.objectParam.key1 + for _, prs := range pipelineRunState { + if prs.ResolvedTask == nil { + continue + } + if validateErr := v1.ValidateReservedParamReferenceMissingKeys(ctx, prs.ResolvedTask.TaskSpec.Steps, pr.Spec.Params); validateErr != nil { + logger.Errorf("PipelineRun %s params references error %v", pr.Name, validateErr) + pr.Status.MarkFailed(v1.PipelineRunReasonParamKeyNotExistent.String(), + "PipelineRun %s/%s doesn't has the references key: %v", + pr.Namespace, pr.Name, validateErr) + return controller.NewPermanentError(validateErr) + } + } + if err := resources.ValidatePipelineTaskResults(pipelineRunFacts.State); err != nil { logger.Errorf("Failed to resolve task result reference for %q with error %v", pr.Name, err) pr.Status.MarkFailed(v1.PipelineRunReasonInvalidTaskResultReference.String(), err.Error()) diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 1e82170957d..eaada7bf78c 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -4631,10 +4631,10 @@ spec: steps: - name: s1 image: alpine - script: | + script: | echo $(params.version) + $(params.tag) - name: b-task - params: + params: - name: ref-p1 value: $(params.version) - name: ref-p2 @@ -4642,7 +4642,7 @@ spec: taskRef: name: ref-task - name: c-task-matrixed - matrix: + matrix: params: - name: ref-p1 value: [v1, v2] @@ -4721,7 +4721,7 @@ spec: steps: - name: s1 image: alpine - script: | + script: | echo $(params.version) `)} prs := []*v1.PipelineRun{parse.MustParseV1PipelineRun(t, ` @@ -5707,7 +5707,7 @@ spec: serviceAccountName: test-sa-0 workspaces: - name: ws-1 - secret: + secret: secretName: $(tasks.a-task.results.aResult) `)}, expectedTr: mustParseTaskRunWithObjectMeta(t, @@ -5737,7 +5737,7 @@ spec: serviceAccountName: test-sa-0 workspaces: - name: ws-1 - projected: + projected: sources: - configMap: name: $(tasks.a-task.results.aResult) @@ -5752,10 +5752,10 @@ spec: kind: Task workspaces: - name: s1 - projected: + projected: sources: - configMap: - name: aResultValue + name: aResultValue `), }, { @@ -5771,7 +5771,7 @@ spec: serviceAccountName: test-sa-0 workspaces: - name: ws-1 - projected: + projected: sources: - secret: name: $(tasks.a-task.results.aResult) @@ -5786,10 +5786,10 @@ spec: kind: Task workspaces: - name: s1 - projected: + projected: sources: - secret: - name: aResultValue + name: aResultValue `), }, { @@ -5819,7 +5819,7 @@ spec: workspaces: - name: s1 csi: - driver: aResultValue + driver: aResultValue `), }, { @@ -5850,7 +5850,7 @@ spec: workspaces: - name: s1 csi: - nodePublishSecretRef: + nodePublishSecretRef: name: aResultValue `), }, @@ -17370,6 +17370,47 @@ func Test_runNextSchedulableTask(t *testing.T) { } } +func TestReconcileReservedParamsMissingKeys(t *testing.T) { + ps := []*v1.Pipeline{parse.MustParseV1Pipeline(t, ` +metadata: + name: echo-message-pipeline + namespace: foo +spec: + tasks: + - name: task1 + taskSpec: + steps: + - name: echo + image: ubuntu + script: | + #!/bin/bash + echo $(params.provider.wrongKey) +`)} + prs := []*v1.PipelineRun{parse.MustParseV1PipelineRun(t, ` +metadata: + name: test-pipeline-run-with-missing-preserved-param-key + namespace: foo +spec: + params: + - name: provider + value: + project_id: foo + pipelineRef: + name: echo-message-pipeline +`)} + + d := test.Data{ + PipelineRuns: prs, + Pipelines: ps, + } + prt := newPipelineRunTest(t, d) + defer prt.Cancel() + + wantEvents := []string{} + reconciledRun, _ := prt.reconcileRun("foo", "test-pipeline-run-with-missing-preserved-param-key", wantEvents, true) + checkPipelineRunConditionStatusAndReason(t, reconciledRun, corev1.ConditionFalse, v1.PipelineRunReasonParamKeyNotExistent.String()) +} + func getSignedV1Pipeline(unsigned *pipelinev1.Pipeline, signer signature.Signer, name string) (*pipelinev1.Pipeline, error) { signed := unsigned.DeepCopy() signed.Name = name diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 09ca1b7a151..89ed4fb0b92 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -422,37 +422,50 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1.TaskRun) (*v1.TaskSpec, TaskSpec: taskSpec, Kind: resources.GetTaskKind(tr), } + if err := c.validateTaskRun(ctx, tr, taskSpec, rtr); err != nil { + return nil, nil, err + } + return taskSpec, rtr, nil +} + +func (c *Reconciler) validateTaskRun(ctx context.Context, tr *v1.TaskRun, taskSpec *v1.TaskSpec, rtr *resources.ResolvedTask) error { + logger := logging.FromContext(ctx) + if validateErr := v1.ValidateReservedParamReferenceMissingKeys(ctx, taskSpec.Steps, tr.Spec.Params); validateErr != nil { + logger.Errorf("TaskRun %s params references error %v", tr.Name, validateErr) + tr.Status.MarkResourceFailed(v1.TaskRunReasonParamKeyNotExistent, validateErr) + return controller.NewPermanentError(validateErr) + } if err := validateTaskSpecRequestResources(taskSpec); err != nil { logger.Errorf("TaskRun %s taskSpec request resources are invalid: %v", tr.Name, err) tr.Status.MarkResourceFailed(v1.TaskRunReasonFailedValidation, err) - return nil, nil, controller.NewPermanentError(err) + return controller.NewPermanentError(err) } if err := ValidateResolvedTask(ctx, tr.Spec.Params, &v1.Matrix{}, rtr); err != nil { logger.Errorf("TaskRun %q resources are invalid: %v", tr.Name, err) tr.Status.MarkResourceFailed(v1.TaskRunReasonFailedValidation, err) - return nil, nil, controller.NewPermanentError(err) + return controller.NewPermanentError(err) } if config.FromContextOrDefaults(ctx).FeatureFlags.EnableParamEnum { if err := ValidateEnumParam(ctx, tr.Spec.Params, rtr.TaskSpec.Params); err != nil { logger.Errorf("TaskRun %q Param Enum validation failed: %v", tr.Name, err) tr.Status.MarkResourceFailed(v1.TaskRunReasonInvalidParamValue, err) - return nil, nil, controller.NewPermanentError(err) + return controller.NewPermanentError(err) } } if err := resources.ValidateParamArrayIndex(rtr.TaskSpec, tr.Spec.Params); err != nil { logger.Errorf("TaskRun %q Param references are invalid: %v", tr.Name, err) tr.Status.MarkResourceFailed(v1.TaskRunReasonFailedValidation, err) - return nil, nil, controller.NewPermanentError(err) + return controller.NewPermanentError(err) } if err := c.updateTaskRunWithDefaultWorkspaces(ctx, tr, taskSpec); err != nil { logger.Errorf("Failed to update taskrun %s with default workspace: %v", tr.Name, err) tr.Status.MarkResourceFailed(v1.TaskRunReasonFailedResolution, err) - return nil, nil, controller.NewPermanentError(err) + return controller.NewPermanentError(err) } var workspaceDeclarations []v1.WorkspaceDeclaration @@ -471,28 +484,27 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1.TaskRun) (*v1.TaskSpec, if err := workspace.ValidateBindings(ctx, workspaceDeclarations, tr.Spec.Workspaces); err != nil { logger.Errorf("TaskRun %q workspaces are invalid: %v", tr.Name, err) tr.Status.MarkResourceFailed(v1.TaskRunReasonFailedValidation, err) - return nil, nil, controller.NewPermanentError(err) + return controller.NewPermanentError(err) } aaBehavior, err := affinityassistant.GetAffinityAssistantBehavior(ctx) if err != nil { - return nil, nil, controller.NewPermanentError(err) + return controller.NewPermanentError(err) } if aaBehavior == affinityassistant.AffinityAssistantPerWorkspace { if err := workspace.ValidateOnlyOnePVCIsUsed(tr.Spec.Workspaces); err != nil { logger.Errorf("TaskRun %q workspaces incompatible with Affinity Assistant: %v", tr.Name, err) tr.Status.MarkResourceFailed(v1.TaskRunReasonFailedValidation, err) - return nil, nil, controller.NewPermanentError(err) + return controller.NewPermanentError(err) } } if err := validateOverrides(taskSpec, &tr.Spec); err != nil { logger.Errorf("TaskRun %q step or sidecar overrides are invalid: %v", tr.Name, err) tr.Status.MarkResourceFailed(v1.TaskRunReasonFailedValidation, err) - return nil, nil, controller.NewPermanentError(err) + return controller.NewPermanentError(err) } - - return taskSpec, rtr, nil + return nil } // `reconcile` creates the Pod associated to the TaskRun, and it pulls back status diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index c681f8ae04b..1efb5d37417 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -3480,7 +3480,7 @@ spec: value: ["taskrun", "array", "param"] - name: objectparam value: - key: taskrun object param + key: taskrun object param taskSpec: steps: - ref: @@ -3567,7 +3567,7 @@ spec: key: type: string default: - key: taskspec object param + key: taskspec object param steps: - ref: name: stepAction @@ -3625,7 +3625,7 @@ spec: params: - name: string-param type: string - default: "stepaction string param" + default: "stepaction string param" - name: array-param type: array default: @@ -6713,6 +6713,49 @@ func TestIsConcurrentModificationError(t *testing.T) { } } +func TestReconcileReservedParamsMissingKeys(t *testing.T) { + ts := []*v1.Task{parse.MustParseV1Task(t, ` +metadata: + name: echo-message-task + namespace: foo +spec: + steps: + - image: ubuntu + name: echo + script: | + echo -n $(params.provider.wrongKey) +`)} + trs := []*v1.TaskRun{parse.MustParseV1TaskRun(t, ` +metadata: + name: test-task-run-with-missing-preserved-param-key + namespace: foo +spec: + params: + - name: provider + value: + project_id: foo + taskRef: + name: echo-message-task +`)} + + d := test.Data{ + TaskRuns: trs, + Tasks: ts, + } + testAssets, cancel := getTaskRunController(t, d) + defer cancel() + createServiceAccount(t, testAssets, "default", trs[0].Namespace) + _ = testAssets.Controller.Reconciler.Reconcile(testAssets.Ctx, getRunName(trs[0])) + reconciledRun, err := testAssets.Clients.Pipeline.TektonV1().TaskRuns(trs[0].Namespace).Get(testAssets.Ctx, trs[0].Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("getting updated taskrun: %v", err) + } + condition := reconciledRun.Status.GetCondition(apis.ConditionSucceeded) + if condition.Type != apis.ConditionSucceeded || condition.Status != corev1.ConditionFalse || condition.Reason != v1.TaskRunReasonParamKeyNotExistent.String() { + t.Errorf("Expected TaskRun to fail with reason \"%s\" but it did not. Final conditions were:\n%#v", podconvert.ReasonResourceVerificationFailed, trs[0].Status.Conditions) + } +} + // getResolvedResolutionRequest is a helper function to return the ResolutionRequest and the data is filled with resourceBytes, // the ResolutionRequest's name is generated by resolverName, namespace and runName. func getResolvedResolutionRequest(t *testing.T, resolverName string, resourceBytes []byte, namespace string, runName string) resolutionv1beta1.ResolutionRequest { diff --git a/test/default_params_test.go b/test/default_params_test.go new file mode 100644 index 00000000000..a8b2ed27fcc --- /dev/null +++ b/test/default_params_test.go @@ -0,0 +1,353 @@ +//go:build e2e +// +build e2e + +/* + Copyright 2023 The Tekton Authors + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ + +package test + +import ( + "context" + "fmt" + "testing" + + v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" + "github.com/tektoncd/pipeline/test/parse" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + knativetest "knative.dev/pkg/test" + "knative.dev/pkg/test/helpers" +) + +func TestDefaultParamReferencedInPipeline_Success(t *testing.T) { + ctx := context.Background() + c, namespace := setup(ctx, t, clusterFeatureFlags) + + t.Parallel() + + knativetest.CleanupOnInterrupt(func() { tearDown(ctx, t, c, namespace) }, t.Logf) + defer tearDown(ctx, t, c, namespace) + + pipelineName := helpers.ObjectNameForTest(t) + examplePipeline := parse.MustParseV1Pipeline(t, fmt.Sprintf(` +apiVersion: tekton.dev/v1 +kind: Pipeline +metadata: + name: %s + namespace: %s +spec: + params: + - name: hello + properties: + project_id: + type: string + type: object + tasks: + - name: task1 + params: + - name: foo + value: + project_id: $(params.hello.project_id) + taskSpec: + params: + - name: foo + type: object + properties: + project_id: + type: string + steps: + - name: echo + image: ubuntu + script: | + #!/bin/bash + echo $(params.provider.project_id) + echo $(params.foo.project_id) +`, pipelineName, namespace)) + + _, err := c.V1PipelineClient.Create(ctx, examplePipeline, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Failed to create Pipeline `%s`: %s", pipelineName, err) + } + + prName := helpers.ObjectNameForTest(t) + + pipelineRun := parse.MustParseV1PipelineRun(t, fmt.Sprintf(` +metadata: + name: %s + namespace: %s +spec: + params: + - name: provider + value: + project_id: foo + - name: hello + value: + project_id: foo + pipelineRef: + resolver: cluster + params: + - name: kind + value: pipeline + - name: name + value: %s + - name: namespace + value: %s +`, prName, namespace, pipelineName, namespace)) + + _, err = c.V1PipelineRunClient.Create(ctx, pipelineRun, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Failed to create PipelineRun `%s`: %s", prName, err) + } + + t.Logf("Waiting for PipelineRun %s in namespace %s to complete", prName, namespace) + if err := WaitForPipelineRunState(ctx, c, prName, timeout, PipelineRunSucceed(prName), "PipelineRunSuccess", v1Version); err != nil { + t.Fatalf("Error waiting for PipelineRun %s to finish: %s", prName, err) + } +} + +func TestDefaultParamReferencedInTask_Success(t *testing.T) { + ctx := context.Background() + c, namespace := setup(ctx, t, clusterFeatureFlags) + + t.Parallel() + + knativetest.CleanupOnInterrupt(func() { tearDown(ctx, t, c, namespace) }, t.Logf) + defer tearDown(ctx, t, c, namespace) + + taskName := helpers.ObjectNameForTest(t) + exampleTask := parse.MustParseV1Task(t, fmt.Sprintf(` +apiVersion: tekton.dev/v1 +metadata: + name: %s + namespace: %s +spec: + params: + - name: foo + type: object + properties: + project_id: + type: string + steps: + - image: ubuntu + name: echo + script: | + echo -n $(params.foo.project_id) + echo -n $(params.provider.project_id) +`, taskName, namespace)) + + _, err := c.V1TaskClient.Create(ctx, exampleTask, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Failed to create Task `%s`: %s", taskName, err) + } + + trName := helpers.ObjectNameForTest(t) + + taskRun := parse.MustParseV1TaskRun(t, fmt.Sprintf(` +metadata: + name: %s + namespace: %s +spec: + params: + - name: provider + value: + project_id: foo + - name: foo + value: + project_id: foo + taskRef: + resolver: cluster + params: + - name: kind + value: task + - name: name + value: %s + - name: namespace + value: %s +`, trName, namespace, taskName, namespace)) + + _, err = c.V1TaskRunClient.Create(ctx, taskRun, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Failed to create TaskRun `%s`: %s", trName, err) + } + + t.Logf("Waiting for TaskRun %s in namespace %s to complete", trName, namespace) + if err := WaitForTaskRunState(ctx, c, trName, TaskRunSucceed(trName), "TaskRunSuccess", v1Version); err != nil { + t.Fatalf("Error waiting for TaskRun %s to finish: %s", trName, err) + } +} + +func TestDefaultParamReferencedInPipeline_Failure(t *testing.T) { + ctx := context.Background() + c, namespace := setup(ctx, t, clusterFeatureFlags) + + t.Parallel() + + knativetest.CleanupOnInterrupt(func() { tearDown(ctx, t, c, namespace) }, t.Logf) + defer tearDown(ctx, t, c, namespace) + + pipelineName := helpers.ObjectNameForTest(t) + examplePipeline := parse.MustParseV1Pipeline(t, fmt.Sprintf(` +apiVersion: tekton.dev/v1 +kind: Pipeline +metadata: + name: %s + namespace: %s +spec: + params: + - name: hello + properties: + project_id: + type: string + type: object + tasks: + - name: task1 + params: + - name: foo + value: + project_id: $(params.hello.project_id) + taskSpec: + params: + - name: foo + type: object + properties: + project_id: + type: string + steps: + - name: echo + image: ubuntu + script: | + #!/bin/bash + echo $(params.provider.not_existent) + echo $(params.foo.project_id) +`, pipelineName, namespace)) + + _, err := c.V1PipelineClient.Create(ctx, examplePipeline, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Failed to create Pipeline `%s`: %s", pipelineName, err) + } + + prName := helpers.ObjectNameForTest(t) + + pipelineRun := parse.MustParseV1PipelineRun(t, fmt.Sprintf(` +metadata: + name: %s + namespace: %s +spec: + params: + - name: provider + value: + project_id: foo + - name: hello + value: + project_id: foo + pipelineRef: + resolver: cluster + params: + - name: kind + value: pipeline + - name: name + value: %s + - name: namespace + value: %s +`, prName, namespace, pipelineName, namespace)) + + _, err = c.V1PipelineRunClient.Create(ctx, pipelineRun, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Failed to create PipelineRun `%s`: %s", prName, err) + } + + t.Logf("Waiting for PipelineRun %s in namespace %s to complete", prName, namespace) + if err := WaitForPipelineRunState(ctx, c, prName, timeout, + Chain( + FailedWithReason(v1.PipelineRunReasonParamKeyNotExistent.String(), prName), + ), "PipelineRunFailed", v1Version); err != nil { + t.Fatalf("Error waiting for PipelineRun to finish with expected error: %s", err) + } +} + +func TestDefaultParamReferencedInTask_Failure(t *testing.T) { + ctx := context.Background() + c, namespace := setup(ctx, t, clusterFeatureFlags) + + t.Parallel() + + knativetest.CleanupOnInterrupt(func() { tearDown(ctx, t, c, namespace) }, t.Logf) + defer tearDown(ctx, t, c, namespace) + + taskName := helpers.ObjectNameForTest(t) + exampleTask := parse.MustParseV1Task(t, fmt.Sprintf(` +apiVersion: tekton.dev/v1 +metadata: + name: %s + namespace: %s +spec: + params: + - name: foo + type: object + properties: + project_id: + type: string + steps: + - image: ubuntu + name: echo + script: | + echo -n $(params.foo.project_id) + echo -n $(params.provider.not-existent) +`, taskName, namespace)) + + _, err := c.V1TaskClient.Create(ctx, exampleTask, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Failed to create Task `%s`: %s", taskName, err) + } + + trName := helpers.ObjectNameForTest(t) + + taskRun := parse.MustParseV1TaskRun(t, fmt.Sprintf(` +metadata: + name: %s + namespace: %s +spec: + params: + - name: provider + value: + project_id: foo + - name: foo + value: + project_id: foo + taskRef: + resolver: cluster + params: + - name: kind + value: task + - name: name + value: %s + - name: namespace + value: %s +`, trName, namespace, taskName, namespace)) + + _, err = c.V1TaskRunClient.Create(ctx, taskRun, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Failed to create TaskRun `%s`: %s", trName, err) + } + + t.Logf("Waiting for TaskRun %s in namespace %s to complete", trName, namespace) + if err := WaitForTaskRunState(ctx, c, trName, + Chain( + FailedWithReason(v1.TaskRunReasonFailedValidation.String(), trName), + ), "PipelineRunFailed", v1Version); err != nil { + t.Fatalf("Error waiting for TaskRun to finish with expected error: %s", err) + } +}