From 28ae20e7c9a5fb94c1c4a4f24242c87b51fb0f98 Mon Sep 17 00:00:00 2001 From: Quan Zhang Date: Tue, 24 Oct 2023 11:26:42 -0400 Subject: [PATCH 1/2] [TEP-0144] Validate PipelineRun for Param Enum Part of [#7270][#7270]. In [TEP-0144][tep-0144] we proposed a new `enum` field to support built-in param input validation. This commit adds validation logic for PipelineRun against Param Enum /kind feature [#7270]: https://github.com/tektoncd/pipeline/issues/7270 [tep-0144]: https://github.com/tektoncd/community/blob/main/teps/0144-param-enum.md --- config/config-feature-flags.yaml | 1 - docs/additional-configs.md | 1 + docs/pipeline-api.md | 3 + docs/pipelineruns.md | 12 ++ docs/pipelines.md | 65 ++++++- docs/taskruns.md | 2 - docs/tasks.md | 2 - .../v1/pipelineruns/alpha/param-enum.yaml | 33 ++++ pkg/apis/pipeline/v1/pipelinerun_types.go | 2 + pkg/reconciler/pipelinerun/pipelinerun.go | 22 +++ .../pipelinerun/pipelinerun_test.go | 182 ++++++++++++++++++ 11 files changed, 318 insertions(+), 7 deletions(-) create mode 100644 examples/v1/pipelineruns/alpha/param-enum.yaml diff --git a/config/config-feature-flags.yaml b/config/config-feature-flags.yaml index 9911dd3fdb3..83c8ee98e01 100644 --- a/config/config-feature-flags.yaml +++ b/config/config-feature-flags.yaml @@ -127,5 +127,4 @@ data: # This feature is in preview mode and not implemented yet. Please check #7259 for updates. enable-step-actions: "false" # Setting this flag to "true" will enable the built-in param input validation via param enum. - # NOTE (#7270): this feature is still under development and not yet functional. enable-param-enum: "false" diff --git a/docs/additional-configs.md b/docs/additional-configs.md index 11b95952590..16a48b36950 100644 --- a/docs/additional-configs.md +++ b/docs/additional-configs.md @@ -317,6 +317,7 @@ Features currently in "alpha" are: | [Coschedule](./affinityassistants.md) | [TEP-0135](https://github.com/tektoncd/community/blob/main/teps/0135-coscheduling-pipelinerun-pods.md) | N/A |`coschedule` | | [keep pod on cancel](./taskruns.md#cancelling-a-taskrun) | N/A | v0.52 | keep-pod-on-cancel | | [CEL in WhenExpression](./taskruns.md#cancelling-a-taskrun) | [TEP-0145](https://github.com/tektoncd/community/blob/main/teps/0145-cel-in-whenexpression.md) | N/A | enable-cel-in-whenexpression | +| [Param Enum](./taskruns.md#parameter-enums) | [TEP-0144](https://github.com/tektoncd/community/blob/main/teps/0144-param-enum.md) | N/A | `enable-param-enum` | ### Beta Features diff --git a/docs/pipeline-api.md b/docs/pipeline-api.md index 379383bb642..03e1751d0c6 100644 --- a/docs/pipeline-api.md +++ b/docs/pipeline-api.md @@ -1997,6 +1997,9 @@ associated Pipeline is an invalid graph (a.k.a wrong order, cycle, …)

"InvalidMatrixParameterTypes"

ReasonInvalidMatrixParameterTypes indicates a matrix contains invalid parameter types

+

"InvalidParamValue"

+

PipelineRunReasonInvalidParamValue indicates that the PipelineRun Param input value is not allowed.

+

"InvalidTaskResultReference"

ReasonInvalidTaskResultReference indicates a task result was declared but was not initialized by that task

diff --git a/docs/pipelineruns.md b/docs/pipelineruns.md index a8ab9cf2187..43ef4d7675a 100644 --- a/docs/pipelineruns.md +++ b/docs/pipelineruns.md @@ -271,6 +271,18 @@ case is when your CI system autogenerates `PipelineRuns` and it has `Parameters` provide to all `PipelineRuns`. Because you can pass in extra `Parameters`, you don't have to go through the complexity of checking each `Pipeline` and providing only the required params. +#### Parameter Enums + +> :seedling: **Specifying `enum` is an [alpha](additional-configs.md#alpha-features) feature.** The `enable-param-enum` feature flag must be set to `"true"` to enable this feature. + +If a `Parameter` is guarded by `Enum` in the `Pipeline`, you can only provide `Parameter` values in the `PipelineRun` that are predefined in the `Param.Enum` in the `Pipeline`. The `PipelineRun` will fail with reason `InvalidParamValue` otherwise. + +Tekton will also the validate the `param` values passed to any referenced `Tasks` (vis `taskRef`) if `Enum` is specified for the `Task`. The `PipelineRun` will fail with reason `InvalidParamValue` if `Enum` validation is failed for any of the `PipelineTask`. + +You can also specify `Enum` for `PipelineRun` with an embedded `Pipeline`. The same param validation will be executed in this scenario. + +See more details in [Param.Enum](./pipelines.md#param-enum). + #### Propagated Parameters When using an inlined spec, parameters from the parent `PipelineRun` will be diff --git a/docs/pipelines.md b/docs/pipelines.md index e734ed1de73..6f2dd5c0e7f 100644 --- a/docs/pipelines.md +++ b/docs/pipelines.md @@ -274,9 +274,70 @@ spec: #### Param enum > :seedling: **Specifying `enum` is an [alpha](additional-configs.md#alpha-features) feature.** The `enable-param-enum` feature flag must be set to `"true"` to enable this feature. -> :seedling: This feature is WIP and not yet supported/implemented. Documentation to be completed. +Parameter declarations can include `enum` which is a predefine set of valid values that can be accepted by the `Pipeline` `Param`. For example, the valid/allowed values for `Param` "message" is bounded to `v1` and `v2`: -Parameter declarations can include `enum` which is a predefine set of valid values that can be accepted by the `Pipeline`. +``` yaml +apiVersion: tekton.dev/v1 +kind: Pipeline +metadata: + name: pipeline-param-enum +spec: + params: + - name: message + enum: ["v1", "v2"] + default: "v1" + tasks: + - name: task1 + params: + - name: message + value: $(params.message) + steps: + - name: build + image: bash:3.2 + script: | + echo "$(params.message)" +``` + +If the `Param` value passed in by `PipelineRun` is **NOT** in the predefined `enum` list, the `PipelineRun` will fail with reason `InvalidParamValue`. + +If a `PipelineTask` references a `Task` with `enum`, Tekton validates the **intersection** of enum specified in the referenced `Task` and the enum specified in the Pipeline `spec.params`. In the example below, the referenced `Task` accepts `v1` and `v2` as valid values, and the `Pipeline` accepts `v2` and `v3` as valid values. Only passing `v2` in the `PipelineRun` will lead to a sucessful execution. + +``` yaml +apiVersion: tekton.dev/v1 +kind: Task +metadata: + name: param-enum-demo +spec: + params: + - name: message + type: string + enum: ["v1", "v2"] + steps: + - name: build + image: bash:latest + script: | + echo "$(params.message)" +``` + +``` yaml +apiVersion: tekton.dev/v1 +kind: Pipeline +metadata: + name: pipeline-param-enum +spec: + params: + - name: message + enum: ["v2", "v3"] + tasks: + - name: task1 + params: + - name: message + value: $(params.message) + taskRef: + name: param-enum-demo +``` + +See usage in this [example](../examples/v1/pipelineruns/alpha/param-enum.yaml) ## Adding `Tasks` to the `Pipeline` diff --git a/docs/taskruns.md b/docs/taskruns.md index ba56b68ed1f..942d507c653 100644 --- a/docs/taskruns.md +++ b/docs/taskruns.md @@ -406,8 +406,6 @@ go through the complexity of checking each `Task` and providing only the require > :seedling: **Specifying `enum` is an [alpha](additional-configs.md#alpha-features) feature.** The `enable-param-enum` feature flag must be set to `"true"` to enable this feature. -> :seedling: This feature is WIP and not yet supported/implemented. Documentation to be completed. - If a `Parameter` is guarded by `Enum` in the `Task`, you can only provide `Parameter` values in the `TaskRun` that are predefined in the `Param.Enum` in the `Task`. The `TaskRun` will fail with reason `InvalidParamValue` otherwise. You can also specify `Enum` for [`TaskRun` with an embedded `Task`](#example-taskrun-with-an-embedded-task). The same param validation will be executed in this scenario. diff --git a/docs/tasks.md b/docs/tasks.md index d7a79f3feda..72e3f81f759 100644 --- a/docs/tasks.md +++ b/docs/tasks.md @@ -715,8 +715,6 @@ spec: #### Param enum > :seedling: **Specifying `enum` is an [alpha](additional-configs.md#alpha-features) feature.** The `enable-param-enum` feature flag must be set to `"true"` to enable this feature. -> :seedling: This feature is WIP and not yet supported/implemented. Documentation to be completed. - Parameter declarations can include `enum` which is a predefine set of valid values that can be accepted by the `Param`. For example, the valid/allowed values for `Param` "message" is bounded to `v1`, `v2` and `v3`: ``` yaml diff --git a/examples/v1/pipelineruns/alpha/param-enum.yaml b/examples/v1/pipelineruns/alpha/param-enum.yaml new file mode 100644 index 00000000000..39d57d967f4 --- /dev/null +++ b/examples/v1/pipelineruns/alpha/param-enum.yaml @@ -0,0 +1,33 @@ +apiVersion: tekton.dev/v1 +kind: Pipeline +metadata: + name: pipeline-param-enum +spec: + params: + - name: message + enum: ["v1", "v2", "v3"] + default: "v1" + tasks: + - name: task1 + params: + - name: message + value: $(params.message) + taskSpec: + params: + - name: message + steps: + - name: build + image: bash:3.2 + script: | + echo "$(params.message)" +--- +apiVersion: tekton.dev/v1 +kind: PipelineRun +metadata: + name: pipelinerun-param-enum +spec: + pipelineRef: + name: pipeline-param-enum + params: + - name: message + value: "v2" diff --git a/pkg/apis/pipeline/v1/pipelinerun_types.go b/pkg/apis/pipeline/v1/pipelinerun_types.go index 88aad636b89..724b47130a7 100644 --- a/pkg/apis/pipeline/v1/pipelinerun_types.go +++ b/pkg/apis/pipeline/v1/pipelinerun_types.go @@ -409,6 +409,8 @@ const ( PipelineRunReasonCreateRunFailed PipelineRunReason = "CreateRunFailed" // ReasonCELEvaluationFailed indicates the pipeline fails the CEL evaluation PipelineRunReasonCELEvaluationFailed PipelineRunReason = "CELEvaluationFailed" + // PipelineRunReasonInvalidParamValue indicates that the PipelineRun Param input value is not allowed. + PipelineRunReasonInvalidParamValue PipelineRunReason = "InvalidParamValue" ) func (t PipelineRunReason) String() string { diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index aac9fba25ba..5fc1a80be96 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -387,6 +387,18 @@ func (c *Reconciler) resolvePipelineState( return nil, controller.NewPermanentError(err) } } + + if config.FromContextOrDefaults(ctx).FeatureFlags.EnableParamEnum { + for _, tr := range resolvedTask.TaskRuns { + for _, cond := range tr.Status.Conditions { + if cond.Type == apis.ConditionSucceeded && cond.Status == corev1.ConditionFalse && cond.Reason == v1.TaskRunReasonInvalidParamValue { + err = fmt.Errorf("invalid param value in the referenced Task from PipelineTask \"%s\": %s", resolvedTask.PipelineTask.Name, cond.Message) + pr.Status.MarkFailed(v1.PipelineRunReasonInvalidParamValue.String(), err.Error()) + return nil, controller.NewPermanentError(err) + } + } + } + } pst = append(pst, resolvedTask) } return pst, nil @@ -487,6 +499,16 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1.PipelineRun, getPipel return controller.NewPermanentError(err) } + if config.FromContextOrDefaults(ctx).FeatureFlags.EnableParamEnum { + if err := taskrun.ValidateEnumParam(ctx, pr.Spec.Params, pipelineSpec.Params); err != nil { + logger.Errorf("PipelineRun %q Param Enum validation failed: %v", pr.Name, err) + pr.Status.MarkFailed(v1.PipelineRunReasonInvalidParamValue.String(), + "PipelineRun %s/%s parameters have invalid value: %s", + pr.Namespace, pr.Name, err) + return controller.NewPermanentError(err) + } + } + // Ensure that the keys of an object param declared in PipelineSpec are not missed in the PipelineRunSpec if err = resources.ValidateObjectParamRequiredKeys(pipelineSpec.Params, pr.Spec.Params); err != nil { // This Run has failed, so we need to mark it as failed and stop reconciling it diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index e9e50b529a4..9a86f1b2f52 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -4278,6 +4278,188 @@ spec: checkPipelineRunConditionStatusAndReason(t, pipelineRun, corev1.ConditionFalse, string(v1.PipelineRunReasonCELEvaluationFailed)) } +func TestReconcile_Pipeline_Level_Enum_Pass(t *testing.T) { + ps := []*v1.Pipeline{parse.MustParseV1Pipeline(t, ` +metadata: + name: test-pipeline-level-enum + namespace: foo +spec: + params: + - name: version + type: string + enum: ["v1", "v2"] + - name: tag + type: string + tasks: + - name: a-task + params: + - name: version + value: $(params.version) + - name: tag + value: $(params.tag) + taskSpec: + name: a-task + params: + - name: version + - name: tag + steps: + - name: s1 + image: alpine + script: | + echo $(params.version) + $(params.tag) +`)} + prs := []*v1.PipelineRun{parse.MustParseV1PipelineRun(t, ` +metadata: + name: test-pipeline-level-enum-run + namespace: foo +spec: + params: + - name: version + value: "v1" + - name: tag + value: "t1" + pipelineRef: + name: test-pipeline-level-enum +`)} + cms := []*corev1.ConfigMap{ + { + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.Namespace()}, + Data: map[string]string{ + "enable-param-enum": "true", + }, + }, + } + d := test.Data{ + PipelineRuns: prs, + Pipelines: ps, + ConfigMaps: cms, + } + prt := newPipelineRunTest(t, d) + defer prt.Cancel() + pipelineRun, _ := prt.reconcileRun("foo", "test-pipeline-level-enum-run", []string{}, false) + // PipelineRun in running status indicates the param enum has passed validation + checkPipelineRunConditionStatusAndReason(t, pipelineRun, corev1.ConditionUnknown, v1.PipelineRunReasonRunning.String()) +} + +func TestReconcile_Pipeline_Level_Enum_Failed(t *testing.T) { + ps := []*v1.Pipeline{parse.MustParseV1Pipeline(t, ` +metadata: + name: test-pipeline-level-enum + namespace: foo +spec: + params: + - name: version + type: string + enum: ["v1", "v2"] + tasks: + - name: a-task + taskSpec: + name: a-task + params: + - name: version + steps: + - name: s1 + image: alpine + script: | + echo $(params.version) +`)} + prs := []*v1.PipelineRun{parse.MustParseV1PipelineRun(t, ` +metadata: + name: test-pipeline-level-enum-run + namespace: foo +spec: + params: + - name: version + value: "v3" + pipelineRef: + name: test-pipeline-level-enum +`)} + cms := []*corev1.ConfigMap{ + { + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.Namespace()}, + Data: map[string]string{ + "enable-param-enum": "true", + }, + }, + } + d := test.Data{ + PipelineRuns: prs, + Pipelines: ps, + ConfigMaps: cms, + } + prt := newPipelineRunTest(t, d) + defer prt.Cancel() + pipelineRun, _ := prt.reconcileRun("foo", "test-pipeline-level-enum-run", []string{}, true) + checkPipelineRunConditionStatusAndReason(t, pipelineRun, corev1.ConditionFalse, string(v1.PipelineRunReasonInvalidParamValue)) +} + +func TestReconcile_PipelineTask_Level_Enum_Failed(t *testing.T) { + ps := []*v1.Pipeline{parse.MustParseV1Pipeline(t, ` +metadata: + name: test-pipelineTask-level-enum + namespace: foo +spec: + params: + - name: version + type: string + tasks: + - name: a-task + params: + - name: version + value: $(params.version) + taskSpec: + name: a-task + params: + - name: version + enum: ["v1", "v2"] + steps: + - name: s1 + image: alpine + script: | + echo $(params.version) +`)} + prs := []*v1.PipelineRun{parse.MustParseV1PipelineRun(t, ` +metadata: + name: test-pipelineTask-level-enum-run + namespace: foo +spec: + params: + - name: version + value: "v3" + pipelineRef: + name: test-pipelineTask-level-enum +`)} + + trs := []*v1.TaskRun{mustParseTaskRunWithObjectMeta(t, + taskRunObjectMeta("test-pipelineTask-level-enum-a-task", "foo", + "test-pipelineTask-level-enum-run", "test-pipelineTask-level-enum", "a-task", true), + ` +status: + conditions: + - status: "False" + type: Succeeded + reason: "InvalidParamValue" +`)} + cms := []*corev1.ConfigMap{ + { + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.Namespace()}, + Data: map[string]string{ + "enable-param-enum": "true", + }, + }, + } + d := test.Data{ + PipelineRuns: prs, + Pipelines: ps, + ConfigMaps: cms, + TaskRuns: trs, + } + prt := newPipelineRunTest(t, d) + defer prt.Cancel() + pipelineRun, _ := prt.reconcileRun("foo", "test-pipelineTask-level-enum-run", []string{}, true) + checkPipelineRunConditionStatusAndReason(t, pipelineRun, corev1.ConditionFalse, string(v1.PipelineRunReasonInvalidParamValue)) +} + // TestReconcileWithAffinityAssistantStatefulSet tests that given a pipelineRun with workspaces, // an Affinity Assistant StatefulSet is created for each PVC workspace and // that the Affinity Assistant names is propagated to TaskRuns. From ea1e7a7591843195cba4ce4165acce24947bd694 Mon Sep 17 00:00:00 2001 From: Quan Zhang Date: Sun, 12 Nov 2023 16:26:39 -0500 Subject: [PATCH 2/2] add subset validation --- .../v1/pipelineruns/alpha/param-enum.yaml | 29 +- pkg/reconciler/pipelinerun/pipelinerun.go | 44 ++- .../pipelinerun/pipelinerun_test.go | 87 ++++-- .../resources/pipelinerunresolution.go | 71 +++++ .../resources/pipelinerunresolution_test.go | 251 ++++++++++++++++++ 5 files changed, 431 insertions(+), 51 deletions(-) diff --git a/examples/v1/pipelineruns/alpha/param-enum.yaml b/examples/v1/pipelineruns/alpha/param-enum.yaml index 39d57d967f4..9f6758bc891 100644 --- a/examples/v1/pipelineruns/alpha/param-enum.yaml +++ b/examples/v1/pipelineruns/alpha/param-enum.yaml @@ -1,25 +1,34 @@ apiVersion: tekton.dev/v1 +kind: Task +metadata: + name: task-param-enum +spec: + params: + - name: message + type: string + enum: ["v1", "v2", "v3"] + steps: + - name: build + image: bash:latest + script: | + echo "$(params.message)" +--- +apiVersion: tekton.dev/v1 kind: Pipeline metadata: name: pipeline-param-enum spec: params: - name: message - enum: ["v1", "v2", "v3"] + enum: ["v1", "v2"] default: "v1" tasks: - name: task1 params: - name: message value: $(params.message) - taskSpec: - params: - - name: message - steps: - - name: build - image: bash:3.2 - script: | - echo "$(params.message)" + taskRef: + name: task-param-enum --- apiVersion: tekton.dev/v1 kind: PipelineRun @@ -30,4 +39,4 @@ spec: name: pipeline-param-enum params: - name: message - value: "v2" + value: "v1" diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 5fc1a80be96..654cb5ae76c 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -387,18 +387,6 @@ func (c *Reconciler) resolvePipelineState( return nil, controller.NewPermanentError(err) } } - - if config.FromContextOrDefaults(ctx).FeatureFlags.EnableParamEnum { - for _, tr := range resolvedTask.TaskRuns { - for _, cond := range tr.Status.Conditions { - if cond.Type == apis.ConditionSucceeded && cond.Status == corev1.ConditionFalse && cond.Reason == v1.TaskRunReasonInvalidParamValue { - err = fmt.Errorf("invalid param value in the referenced Task from PipelineTask \"%s\": %s", resolvedTask.PipelineTask.Name, cond.Message) - pr.Status.MarkFailed(v1.PipelineRunReasonInvalidParamValue.String(), err.Error()) - return nil, controller.NewPermanentError(err) - } - } - } - } pst = append(pst, resolvedTask) } return pst, nil @@ -543,6 +531,12 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1.PipelineRun, getPipel return controller.NewPermanentError(err) } + // Make a deep copy of the Pipeline and its Tasks before value substution. + // This is used to find referenced pipeline-level params at each PipelineTask when validate param enum subset requirement + originalPipeline := pipelineSpec.DeepCopy() + originalTasks := originalPipeline.Tasks + originalTasks = append(originalTasks, originalPipeline.Finally...) + // Apply parameter substitution from the PipelineRun pipelineSpec = resources.ApplyParameters(ctx, pipelineSpec, pr) pipelineSpec = resources.ApplyContexts(pipelineSpec, pipelineMeta.Name, pr) @@ -637,7 +631,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1.PipelineRun, getPipel pipelineRunFacts.TimeoutsState.PipelineTimeout = &pipelineTimeout } - for _, rpt := range pipelineRunFacts.State { + for i, rpt := range pipelineRunFacts.State { if !rpt.IsCustomTask() { err := taskrun.ValidateResolvedTask(ctx, rpt.PipelineTask.Params, rpt.PipelineTask.Matrix, rpt.ResolvedTask) if err != nil { @@ -645,6 +639,14 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1.PipelineRun, getPipel pr.Status.MarkFailed(v1.PipelineRunReasonFailedValidation.String(), err.Error()) return controller.NewPermanentError(err) } + + if config.FromContextOrDefaults(ctx).FeatureFlags.EnableParamEnum { + if err := resources.ValidateParamEnumSubset(originalTasks[i].Params, pipelineSpec.Params, rpt.ResolvedTask); err != nil { + logger.Errorf("Failed to validate pipelinerun %q with error %v", pr.Name, err) + pr.Status.MarkFailed(v1.PipelineRunReasonFailedValidation.String(), err.Error()) + return controller.NewPermanentError(err) + } + } } } @@ -886,10 +888,24 @@ func (c *Reconciler) createTaskRuns(ctx context.Context, rpt *resources.Resolved defer span.End() var taskRuns []*v1.TaskRun var matrixCombinations []v1.Params - if rpt.PipelineTask.IsMatrixed() { matrixCombinations = rpt.PipelineTask.Matrix.FanOut() } + // validate the param values meet resolved Task Param Enum requirements before creating TaskRuns + if config.FromContextOrDefaults(ctx).FeatureFlags.EnableParamEnum { + for i := range rpt.TaskRunNames { + var params v1.Params + if len(matrixCombinations) > i { + params = matrixCombinations[i] + } + params = append(params, rpt.PipelineTask.Params...) + if err := taskrun.ValidateEnumParam(ctx, params, rpt.ResolvedTask.TaskSpec.Params); err != nil { + err = fmt.Errorf("Invalid param value from PipelineTask \"%s\": %w", rpt.PipelineTask.Name, err) + pr.Status.MarkFailed(v1.PipelineRunReasonInvalidParamValue.String(), err.Error()) + return nil, controller.NewPermanentError(err) + } + } + } for i, taskRunName := range rpt.TaskRunNames { var params v1.Params if len(matrixCombinations) > i { diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 9a86f1b2f52..3a482553c00 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -4278,7 +4278,7 @@ spec: checkPipelineRunConditionStatusAndReason(t, pipelineRun, corev1.ConditionFalse, string(v1.PipelineRunReasonCELEvaluationFailed)) } -func TestReconcile_Pipeline_Level_Enum_Pass(t *testing.T) { +func TestReconcile_Enum_With_Matrix_Pass(t *testing.T) { ps := []*v1.Pipeline{parse.MustParseV1Pipeline(t, ` metadata: name: test-pipeline-level-enum @@ -4287,7 +4287,7 @@ spec: params: - name: version type: string - enum: ["v1", "v2"] + enum: ["v1"] - name: tag type: string tasks: @@ -4301,13 +4301,44 @@ spec: name: a-task params: - name: version + enum: ["v1", "v3"] - name: tag steps: - name: s1 image: alpine script: | echo $(params.version) + $(params.tag) + - name: b-task + params: + - name: ref-p1 + value: $(params.version) + - name: ref-p2 + value: "v3" + taskRef: + name: ref-task + - name: c-task-matrixed + matrix: + params: + - name: ref-p1 + value: [v1, v2] + - name: ref-p2 + value: [v3, v4] + taskRef: + name: ref-task +`)} + + refTasks := []*v1.Task{parse.MustParseV1Task(t, ` +metadata: + name: ref-task + namespace: foo +spec: + params: + - name: ref-p1 + enum: ["v1", "v2"] + - name: ref-p2 + enum: ["v3", "v4"] `)} + prs := []*v1.PipelineRun{parse.MustParseV1PipelineRun(t, ` metadata: name: test-pipeline-level-enum-run @@ -4330,6 +4361,7 @@ spec: }, } d := test.Data{ + Tasks: refTasks, PipelineRuns: prs, Pipelines: ps, ConfigMaps: cms, @@ -4341,7 +4373,7 @@ spec: checkPipelineRunConditionStatusAndReason(t, pipelineRun, corev1.ConditionUnknown, v1.PipelineRunReasonRunning.String()) } -func TestReconcile_Pipeline_Level_Enum_Failed(t *testing.T) { +func TestReconcile_Enum_Subset_Validation_Failed(t *testing.T) { ps := []*v1.Pipeline{parse.MustParseV1Pipeline(t, ` metadata: name: test-pipeline-level-enum @@ -4353,10 +4385,14 @@ spec: enum: ["v1", "v2"] tasks: - name: a-task + params: + - name: version + value: $(params.version) taskSpec: name: a-task params: - name: version + enum: ["v1", "v3"] steps: - name: s1 image: alpine @@ -4370,7 +4406,7 @@ metadata: spec: params: - name: version - value: "v3" + value: "v1" pipelineRef: name: test-pipeline-level-enum `)} @@ -4390,7 +4426,7 @@ spec: prt := newPipelineRunTest(t, d) defer prt.Cancel() pipelineRun, _ := prt.reconcileRun("foo", "test-pipeline-level-enum-run", []string{}, true) - checkPipelineRunConditionStatusAndReason(t, pipelineRun, corev1.ConditionFalse, string(v1.PipelineRunReasonInvalidParamValue)) + checkPipelineRunConditionStatusAndReason(t, pipelineRun, corev1.ConditionFalse, string(v1.PipelineRunReasonFailedValidation)) } func TestReconcile_PipelineTask_Level_Enum_Failed(t *testing.T) { @@ -4401,22 +4437,17 @@ metadata: spec: params: - name: version + enum: [v1] type: string tasks: - name: a-task params: - - name: version + - name: ref-version value: $(params.version) - taskSpec: - name: a-task - params: - - name: version - enum: ["v1", "v2"] - steps: - - name: s1 - image: alpine - script: | - echo $(params.version) + - name: ref-p2 + value: invalid + taskRef: + name: ref-task `)} prs := []*v1.PipelineRun{parse.MustParseV1PipelineRun(t, ` metadata: @@ -4425,21 +4456,23 @@ metadata: spec: params: - name: version - value: "v3" + value: "v1" pipelineRef: name: test-pipelineTask-level-enum `)} - trs := []*v1.TaskRun{mustParseTaskRunWithObjectMeta(t, - taskRunObjectMeta("test-pipelineTask-level-enum-a-task", "foo", - "test-pipelineTask-level-enum-run", "test-pipelineTask-level-enum", "a-task", true), - ` -status: - conditions: - - status: "False" - type: Succeeded - reason: "InvalidParamValue" + refTasks := []*v1.Task{parse.MustParseV1Task(t, ` +metadata: + name: ref-task + namespace: foo +spec: + params: + - name: ref-version + enum: ["v1", "v2"] + - name: ref-p2 + enum: ["v3", "v4"] `)} + cms := []*corev1.ConfigMap{ { ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.Namespace()}, @@ -4449,10 +4482,10 @@ status: }, } d := test.Data{ + Tasks: refTasks, PipelineRuns: prs, Pipelines: ps, ConfigMaps: cms, - TaskRuns: trs, } prt := newPipelineRunTest(t, d) defer prt.Cancel() diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go index 3de5edd6b41..62210247d3d 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go @@ -29,6 +29,7 @@ import ( "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources" "github.com/tektoncd/pipeline/pkg/remote" + "github.com/tektoncd/pipeline/pkg/substitution" kerrors "k8s.io/apimachinery/pkg/api/errors" "knative.dev/pkg/apis" "knative.dev/pkg/kmeta" @@ -808,3 +809,73 @@ func createResultsCacheMatrixedTaskRuns(rpt *ResolvedPipelineTask) (resultsCache } return resultsCache } + +// ValidateParamEnumSubset finds the referenced pipeline-level params in the resolved pipelineTask. +// It then validates if the referenced pipeline-level param enums are subsets of the resolved pipelineTask-level param enums +func ValidateParamEnumSubset(pipelineTaskParams []v1.Param, pipelineParamSpecs []v1.ParamSpec, rt *resources.ResolvedTask) error { + for _, p := range pipelineTaskParams { + // calculate referenced param enums + res, present, errString := substitution.ExtractVariablesFromString(p.Value.StringVal, "params") + if !present { + continue + } + if errString != "" { + return fmt.Errorf("unexpected error in ExtractVariablesFromString: %s", errString) + } + if len(res) != 1 { + return fmt.Errorf("unexpected resolved param in ExtractVariablesFromString, expect 1 but got %v", len(res)) + } + + // resolve pipeline-level and pipelineTask-level enums + paramName := substitution.TrimArrayIndex(res[0]) + pipelineParam := getParamFromName(paramName, pipelineParamSpecs) + resolvedTaskParam := getParamFromName(p.Name, rt.TaskSpec.Params) + + // param enum is only supported for string param type, + // we only validate the enum subset requirement for string typed param. + // If there is no task-level enum (allowing any value), any pipeline-level enum is allowed + if pipelineParam.Type != v1.ParamTypeString || len(resolvedTaskParam.Enum) == 0 { + return nil + } + + // if pipelin-level enum is empty (allowing any value) but task-level enum is not, it is not a "subset" + if len(pipelineParam.Enum) == 0 && len(resolvedTaskParam.Enum) > 0 { + return fmt.Errorf("pipeline param \"%s\" has no enum, but referenced in \"%s\" task has enums: %v", pipelineParam.Name, rt.TaskName, resolvedTaskParam.Enum) + } + + // validate if pipeline-level enum is a subset of pipelineTask-level enum + if isValid := isSubset(pipelineParam.Enum, resolvedTaskParam.Enum); !isValid { + return fmt.Errorf("pipeline param \"%s\" enum: %v is not a subset of the referenced in \"%s\" task param enum: %v", pipelineParam.Name, pipelineParam.Enum, rt.TaskName, resolvedTaskParam.Enum) + } + } + + return nil +} + +func isSubset(pipelineEnum, taskEnum []string) bool { + pipelineEnumMap := make(map[string]bool) + TaskEnumMap := make(map[string]bool) + for _, e := range pipelineEnum { + pipelineEnumMap[e] = true + } + for _, e := range taskEnum { + TaskEnumMap[e] = true + } + + for e := range pipelineEnumMap { + if !TaskEnumMap[e] { + return false + } + } + + return true +} + +func getParamFromName(name string, pss v1.ParamSpecs) v1.ParamSpec { + for _, ps := range pss { + if ps.Name == name { + return ps + } + } + return v1.ParamSpec{} +} diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go index cc6b38e5c5e..90118f30288 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go @@ -4999,3 +4999,254 @@ func TestEvaluateCEL_invalid(t *testing.T) { }) } } + +func TestValidateParamEnumSubset_Valid(t *testing.T) { + tcs := []struct { + name string + params []v1.Param + pipelinePs []v1.ParamSpec + rt *resources.ResolvedTask + }{ + { + name: "no enum - pass", + params: []v1.Param{ + { + Name: "resolved-task-p1", + Value: v1.ParamValue{ + StringVal: "$(params.p1)", + }, + }, + }, + pipelinePs: []v1.ParamSpec{ + { + Name: "p1", + Type: v1.ParamTypeString, + }, + }, + rt: &resources.ResolvedTask{ + TaskSpec: &v1.TaskSpec{ + Params: v1.ParamSpecs{ + { + Name: "resolved-task-p1", + }, + }, + }, + }, + }, { + name: "no task-level enum - pass", + params: []v1.Param{ + { + Name: "resolved-task-p1", + Value: v1.ParamValue{ + StringVal: "$(params.p1)", + }, + }, + }, + pipelinePs: []v1.ParamSpec{ + { + Name: "p1", + Type: v1.ParamTypeString, + Enum: []string{"v1", "v2"}, + }, + }, + rt: &resources.ResolvedTask{ + TaskSpec: &v1.TaskSpec{ + Params: v1.ParamSpecs{ + { + Name: "resolved-task-p1", + }, + }, + }, + }, + }, { + name: "task and pipeline level enum - pass", + params: []v1.Param{ + { + Name: "resolved-task-p1", + Value: v1.ParamValue{ + StringVal: "$(params.p1)", + }, + }, { + Name: "resolved-task-p2", + Value: v1.ParamValue{ + StringVal: "$(params.p2)", + }, + }, + }, + pipelinePs: []v1.ParamSpec{ + { + Name: "p1", + Type: v1.ParamTypeString, + Enum: []string{"v1", "v2"}, + }, + { + Name: "p2", + Type: v1.ParamTypeString, + Enum: []string{"v3", "v4"}, + }, + }, + rt: &resources.ResolvedTask{ + TaskSpec: &v1.TaskSpec{ + Params: v1.ParamSpecs{ + { + Name: "resolved-task-p1", + Enum: []string{"v1", "v2", "v3"}, + }, + { + Name: "resolved-task-p2", + Enum: []string{"v3", "v4"}, + }, + }, + }, + }, + }, { + name: "no referenced param enum - pass", + params: []v1.Param{ + { + Name: "resolved-task-p1", + Value: v1.ParamValue{ + StringVal: "v1", + }, + }, { + Name: "resolved-task-p2", + Value: v1.ParamValue{ + StringVal: "$(task1.results.r1)", + }, + }, + }, + }, { + name: "non-string typed param - pass", + params: []v1.Param{ + { + Name: "resolved-task-p1", + Value: v1.ParamValue{ + StringVal: "$(params.object1.key)", + }, + }, { + Name: "resolved-task-p2", + Value: v1.ParamValue{ + StringVal: "$(params.array1[0])", + }, + }, + }, + pipelinePs: []v1.ParamSpec{ + { + Name: "array1", + Type: v1.ParamTypeArray, + }, + { + Name: "object1", + Type: v1.ParamTypeObject, + }, + }, + rt: &resources.ResolvedTask{ + TaskSpec: &v1.TaskSpec{ + Params: v1.ParamSpecs{ + { + Name: "resolved-task-p1", + Enum: []string{"v1", "v2"}, + }, + { + Name: "resolved-task-p2", + Enum: []string{"v3", "v4"}, + }, + }, + }, + }, + }, + } + + for _, tc := range tcs { + if err := ValidateParamEnumSubset(tc.params, tc.pipelinePs, tc.rt); err != nil { + t.Errorf("unexpected error in ValidateParamEnumSubset: %s", err) + } + } +} + +func TestValidateParamEnumSubset_Invalid(t *testing.T) { + tcs := []struct { + name string + params []v1.Param + pipelinePs []v1.ParamSpec + rt *resources.ResolvedTask + wantErr error + }{{ + name: "pipeline enum is not a subset - fail", + params: []v1.Param{ + { + Name: "resolved-task-p1", + Value: v1.ParamValue{ + StringVal: "$(params.p1)", + }, + }, + }, + pipelinePs: []v1.ParamSpec{ + { + Name: "p1", + Type: v1.ParamTypeString, + Enum: []string{"v1", "v2"}, + }, + }, + rt: &resources.ResolvedTask{ + TaskName: "ref1", + TaskSpec: &v1.TaskSpec{ + Params: v1.ParamSpecs{ + { + Name: "resolved-task-p1", + Enum: []string{"v1", "v3"}, + }, + }, + }, + }, + wantErr: fmt.Errorf("pipeline param \"p1\" enum: [v1 v2] is not a subset of the referenced in \"ref1\" task param enum: [v1 v3]"), + }, { + name: "empty pipeline enum with non-empty task enum - fail", + params: []v1.Param{ + { + Name: "resolved-task-p1", + Value: v1.ParamValue{ + StringVal: "$(params.p1)", + }, + }, + }, + pipelinePs: []v1.ParamSpec{ + { + Name: "p1", + Type: v1.ParamTypeString, + }, + }, + rt: &resources.ResolvedTask{ + TaskName: "ref1", + TaskSpec: &v1.TaskSpec{ + Params: v1.ParamSpecs{ + { + Name: "resolved-task-p1", + Enum: []string{"v1", "v3"}, + }, + }, + }, + }, + wantErr: fmt.Errorf("pipeline param \"p1\" has no enum, but referenced in \"ref1\" task has enums: [v1 v3]"), + }, { + name: "invalid param syntax - failure", + params: []v1.Param{ + { + Name: "resolved-task-p1", + Value: v1.ParamValue{ + StringVal: "$(params.p1.aaa.bbb)", + }, + }, + }, + wantErr: fmt.Errorf("unexpected error in ExtractVariablesFromString: Invalid referencing of parameters in \"$(params.p1.aaa.bbb)\"! Only two dot-separated components after the prefix \"params\" are allowed."), + }} + + for _, tc := range tcs { + err := ValidateParamEnumSubset(tc.params, tc.pipelinePs, tc.rt) + if err == nil { + t.Errorf("expecting error in ValidateParamEnumSubset: %s, but got nil", err) + } + if d := cmp.Diff(tc.wantErr.Error(), err.Error()); d != "" { + t.Errorf("expecting error does not match in ValidateParamEnumSubset: %s", diff.PrintWantGot(d)) + } + } +}