From 9bc3fd8097e13eecc28f6afe165bfb91bf55f42c Mon Sep 17 00:00:00 2001
From: Jerop <jerop@google.com>
Date: Thu, 3 Sep 2020 10:20:55 -0400
Subject: [PATCH] Refactor Pipeline Parameters Validation

Currently, pipeline parameters are only used in task parameters. we need
to use Pipeline Parameters in When Expressions as well. Trying to add the
new validation became ugly, so we're refactoring parameters validation so '
that we can easily add validation for parameters in when expressions.
---
 pkg/apis/pipeline/v1beta1/param_types.go      | 40 +++++++++++++++++++
 .../pipeline/v1beta1/pipeline_validation.go   | 36 ++---------------
 2 files changed, 44 insertions(+), 32 deletions(-)

diff --git a/pkg/apis/pipeline/v1beta1/param_types.go b/pkg/apis/pipeline/v1beta1/param_types.go
index 6588fac007f..875815ca760 100644
--- a/pkg/apis/pipeline/v1beta1/param_types.go
+++ b/pkg/apis/pipeline/v1beta1/param_types.go
@@ -22,6 +22,9 @@ import (
 	"fmt"
 
 	resource "github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1"
+	"github.com/tektoncd/pipeline/pkg/substitution"
+	"k8s.io/apimachinery/pkg/util/sets"
+	"knative.dev/pkg/apis"
 )
 
 // ParamSpec defines arbitrary parameters needed beyond typed inputs (such as
@@ -142,3 +145,40 @@ func NewArrayOrString(value string, values ...string) ArrayOrString {
 		StringVal: value,
 	}
 }
+
+func validatePipelineParametersVariablesInTaskParameters(params []Param, prefix string, paramNames sets.String, arrayParamNames sets.String) *apis.FieldError {
+	for _, param := range params {
+		if param.Value.Type == ParamTypeString {
+			if err := validateStringVariableInTaskParameters(fmt.Sprintf("[%s]", param.Name), param.Value.StringVal, prefix, paramNames, arrayParamNames); err != nil {
+				return err
+			}
+		} else {
+			for _, arrayElement := range param.Value.ArrayVal {
+				if err := validateArrayVariableInTaskParameters(fmt.Sprintf("[%s]", param.Name), arrayElement, prefix, paramNames, arrayParamNames); err != nil {
+					return err
+				}
+			}
+		}
+	}
+	return nil
+}
+
+func validateStringVariableInTaskParameters(name, value, prefix string, stringVars sets.String, arrayVars sets.String) *apis.FieldError {
+	if err := substitution.ValidateVariable(name, value, prefix, "task parameter", "pipelinespec.params", stringVars); err != nil {
+		return err
+	}
+	if err := substitution.ValidateVariableProhibited(name, value, prefix, "task parameter", "pipelinespec.params", arrayVars); err != nil {
+		return err
+	}
+	return nil
+}
+
+func validateArrayVariableInTaskParameters(name, value, prefix string, stringVars sets.String, arrayVars sets.String) *apis.FieldError {
+	if err := substitution.ValidateVariable(name, value, prefix, "task parameter", "pipelinespec.params", stringVars); err != nil {
+		return err
+	}
+	if err := substitution.ValidateVariableIsolated(name, value, prefix, "task parameter", "pipelinespec.params", arrayVars); err != nil {
+		return err
+	}
+	return nil
+}
diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation.go b/pkg/apis/pipeline/v1beta1/pipeline_validation.go
index a82544ec1cc..ee52521f21d 100644
--- a/pkg/apis/pipeline/v1beta1/pipeline_validation.go
+++ b/pkg/apis/pipeline/v1beta1/pipeline_validation.go
@@ -356,46 +356,18 @@ func validatePipelineParameterVariables(tasks []PipelineTask, params []ParamSpec
 		}
 	}
 
-	return validatePipelineVariables(tasks, "params", parameterNames, arrayParameterNames)
+	return validatePipelineParametersVariables(tasks, "params", parameterNames, arrayParameterNames)
 }
 
-func validatePipelineVariables(tasks []PipelineTask, prefix string, paramNames sets.String, arrayParamNames sets.String) *apis.FieldError {
+func validatePipelineParametersVariables(tasks []PipelineTask, prefix string, paramNames sets.String, arrayParamNames sets.String) *apis.FieldError {
 	for _, task := range tasks {
-		for _, param := range task.Params {
-			if param.Value.Type == ParamTypeString {
-				if err := validatePipelineVariable(fmt.Sprintf("param[%s]", param.Name), param.Value.StringVal, prefix, paramNames); err != nil {
-					return err
-				}
-				if err := validatePipelineNoArrayReferenced(fmt.Sprintf("param[%s]", param.Name), param.Value.StringVal, prefix, arrayParamNames); err != nil {
-					return err
-				}
-			} else {
-				for _, arrayElement := range param.Value.ArrayVal {
-					if err := validatePipelineVariable(fmt.Sprintf("param[%s]", param.Name), arrayElement, prefix, paramNames); err != nil {
-						return err
-					}
-					if err := validatePipelineArraysIsolated(fmt.Sprintf("param[%s]", param.Name), arrayElement, prefix, arrayParamNames); err != nil {
-						return err
-					}
-				}
-			}
+		if err := validatePipelineParametersVariablesInTaskParameters(task.Params, prefix, paramNames, arrayParamNames); err != nil {
+			return err
 		}
 	}
 	return nil
 }
 
-func validatePipelineVariable(name, value, prefix string, vars sets.String) *apis.FieldError {
-	return substitution.ValidateVariable(name, value, prefix, "task parameter", "pipelinespec.params", vars)
-}
-
-func validatePipelineNoArrayReferenced(name, value, prefix string, vars sets.String) *apis.FieldError {
-	return substitution.ValidateVariableProhibited(name, value, prefix, "task parameter", "pipelinespec.params", vars)
-}
-
-func validatePipelineArraysIsolated(name, value, prefix string, vars sets.String) *apis.FieldError {
-	return substitution.ValidateVariableIsolated(name, value, prefix, "task parameter", "pipelinespec.params", vars)
-}
-
 func validatePipelineContextVariables(tasks []PipelineTask) *apis.FieldError {
 	pipelineRunContextNames := sets.NewString().Insert(
 		"name",