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..f5708c8b5ee --- /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.TBD.project_id) + echo $(params.foo.project_id) +--- +kind: PipelineRun +apiVersion: tekton.dev/v1 +metadata: + name: test-pipelinerun +spec: + params: + - name: TBD + 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..710cb187373 --- /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.TBD.project_id) +--- +kind: TaskRun +apiVersion: tekton.dev/v1 +metadata: + name: example-tr +spec: + params: + - name: foo + value: + project_id: foo + digest: bar + - name: TBD + 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 2dc3b884f7d..a5b5e8ab7a7 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 = "TBD" ) // SupportedVerbs returns the operations that validation should be called for @@ -395,6 +396,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 37d7100bc56..d57f2bdca54 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 34be57243ae..ecde749df56 100644 --- a/pkg/apis/pipeline/v1/pipelinerun_types.go +++ b/pkg/apis/pipeline/v1/pipelinerun_types.go @@ -414,6 +414,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 86c277ca968..13efee03d05 100644 --- a/pkg/apis/pipeline/v1/task_validation.go +++ b/pkg/apis/pipeline/v1/task_validation.go @@ -132,6 +132,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)) @@ -643,6 +644,27 @@ func validateVariables(ctx context.Context, steps []Step, prefix string, vars se 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) + } + } + } + if len(objectKeys) > 0 { + // check if the object's key names are referenced correctly i.e. param.objectParam.key1 + err := validateVariables(ctx, steps, fmt.Sprintf("params\\.%s", ReservedParamName), objectKeys) + if err != nil { + return err + } + } + return nil +} + // ValidateNameFormat validates that the name format of all param types follows the rules func ValidateNameFormat(stringAndArrayParams sets.String, objectParams []ParamSpec) (errs *apis.FieldError) { // checking string or array name format diff --git a/pkg/apis/pipeline/v1/task_validation_test.go b/pkg/apis/pipeline/v1/task_validation_test.go index 6f38a2d38b0..1914210a87b 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) { diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation.go b/pkg/apis/pipeline/v1beta1/pipeline_validation.go index 7ee9ba354ed..082bfddf2fc 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 = "TBD" ) // SupportedVerbs returns the operations that validation should be called for @@ -414,6 +415,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/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 09ca54b53a5..3ddfd79ec2c 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -671,6 +671,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: %s", + pr.Namespace, pr.Name, err) + 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 85c78191d30..8f1e4f70b3a 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -4627,10 +4627,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 @@ -4638,7 +4638,7 @@ spec: taskRef: name: ref-task - name: c-task-matrixed - matrix: + matrix: params: - name: ref-p1 value: [v1, v2] @@ -4717,7 +4717,7 @@ spec: steps: - name: s1 image: alpine - script: | + script: | echo $(params.version) `)} prs := []*v1.PipelineRun{parse.MustParseV1PipelineRun(t, ` diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index e4e55371502..2a3438a8f8f 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -417,6 +417,12 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1.TaskRun) (*v1.TaskSpec, Kind: resources.GetTaskKind(tr), } + 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.TaskRunReason(v1.PipelineRunReasonParamKeyNotExistent.String()), validateErr) + return nil, nil, 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) diff --git a/test/default_params_test.go b/test/default_params_test.go new file mode 100644 index 00000000000..503102b1c97 --- /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.TBD.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: TBD + 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.TBD.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: TBD + 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.TBD.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: TBD + 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.TBD.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: TBD + 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.PipelineRunReasonParamKeyNotExistent.String(), trName), + ), "PipelineRunFailed", v1Version); err != nil { + t.Fatalf("Error waiting for TaskRun to finish with expected error: %s", err) + } +}