From 8e9fd46d62aa63400aac65d9132161a68e23b5d5 Mon Sep 17 00:00:00 2001
From: Quan Zhang <zhangquan@google.com>
Date: Mon, 11 Dec 2023 13:26:55 -0500
Subject: [PATCH] Fix enum validation with multiple param references

Fixes [#7476][7476]. TEP-0144 requires that the pipeline-level `enum` must be a subset of referenced task-level `enum`.

Prior to this commit, the enum subset validation logic assumes that a task-level param only referenced only one pipeline-level `enum`,
and does not support scenario where multiple pipeline-level `enums` are referenced (e.g., "$(params.p1) and $(params.p2)").

This commit adds the handling logic for such compound references, skipping the subset validation in this scenario as there is no directly associated
params at pipeline level.

/kind bug

[7476]: https://github.com/tektoncd/pipeline/issues/7476
---
 docs/pipelines.md                             | 29 +++++++++++++++--
 .../resources/pipelinerunresolution.go        |  9 +++---
 .../resources/pipelinerunresolution_test.go   | 32 +++++++++++++++++++
 3 files changed, 63 insertions(+), 7 deletions(-)

diff --git a/docs/pipelines.md b/docs/pipelines.md
index db366edec02..fe67e39fbf5 100644
--- a/docs/pipelines.md
+++ b/docs/pipelines.md
@@ -301,8 +301,10 @@ spec:
 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`, the `enums` specified in the Pipeline `spec.params` (pipeline-level `enum`) must be
-a **subset** of the `enums` specified in the referenced `Task` (task-level `enum`). Note that an empty pipeline-level `enum` is invalid 
-in this scenario since an empty `enum` set indicates a "universal set" which allows all possible values. In the below example, the referenced `Task` accepts `v1` and `v2` as valid values, the `Pipeline` further restricts the valid values to `v1`.
+a **subset** of the `enums` specified in the referenced `Task` (task-level `enum`). An empty pipeline-level `enum` is invalid 
+in this scenario since an empty `enum` set indicates a "universal set" which allows all possible values. The same rules apply to `Pipelines` with embbeded `Tasks`. 
+
+In the below example, the referenced `Task` accepts `v1` and `v2` as valid values, the `Pipeline` further restricts the valid value to `v1`.
 
 ``` yaml
 apiVersion: tekton.dev/v1
@@ -339,6 +341,29 @@ spec:
       name: param-enum-demo
 ```
 
+Note that this subset restriction only applies to the task-level `params` with a **direct single** reference to pipeline-level `params`. If a task-level `param` references multiple pipeline-level `params`, the subset validation is not applied.
+
+``` yaml
+apiVersion: tekton.dev/v1
+kind: Pipeline
+...
+spec:
+  params:
+  - name: message1
+    enum: ["v1"]
+  - name: message2
+    enum: ["v2"]
+  tasks:
+  - name: task1
+    params:
+      - name: message
+        value: "$(params.message1) and $(params.message2)"
+    taskSpec:
+      params: message
+      enum: [...] # the message enum is not required to be a subset of message1 or message2
+    ...
+```
+
 Tekton validates user-provided values in a `PipelineRun` against the `enum` specified in the `PipelineSpec.params`. Tekton also validates
 any resolved `param` value against the `enum` specified in each `PipelineTask` before creating the `TaskRun`.
 
diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go
index 93a9899dbdd..4d8b0e486bd 100644
--- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go
+++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go
@@ -818,14 +818,13 @@ func ValidateParamEnumSubset(pipelineTaskParams []v1.Param, pipelineParamSpecs [
 	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))
+
+		// if multiple params are extracted, that means the task-level param is a compounded value, skip subset validation
+		if !present || len(res) > 1 {
+			continue
 		}
 
 		// resolve pipeline-level and pipelineTask-level enums
diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go
index 90118f30288..b765ff591ae 100644
--- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go
+++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go
@@ -5153,6 +5153,38 @@ func TestValidateParamEnumSubset_Valid(t *testing.T) {
 					},
 				},
 			},
+		}, {
+			name: "compound task param with enum - pass",
+			params: []v1.Param{
+				{
+					Name: "resolved-task-p1",
+					Value: v1.ParamValue{
+						StringVal: "$(params.p1) and $(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{"e1", "e2"},
+						},
+					},
+				},
+			},
 		},
 	}