Validate PipelineRun Parameters

While working on allowing PipelineRuns to provide extra parameters (#2513),
we found that providing extra parameters was unintentionally allowed. That's
because, currently, there's no validation that all parameters expected by
the Pipeline is provided by the PipelineRun.

In this PR, we add validation for PipelineRun parameters by generating a
list of provided parameters then iterating through the required parameters
to ensure they are in the list of provided parameters. Note that parameters
which have default values specified in Pipeline are not required.

In the validation, we still allow PipelineRuns to provide extra parameters.
If we disallow PipelineRuns from provides extra parameters, the Pipelines
would fail. As a result, systems that autogenerate PipelineRuns need to
lookat each pipeline to see what parameters they need so it can provide
only the required parameters. That means users would have to resort to
more complex designs to solve this issue, as further described in (#2513).
By allowing PipelineRuns to provide extra parameters, we make the process
of autogenerating of PipelineRuns simpler.

Fixes #2708.

docs/pipelineruns.md

@@ -186,6 +186,9 @@ to all `persistentVolumeClaims` generated internally.
 You can specify `Parameters` that you want to pass to the `Pipeline` during execution,
 including different values of the same parameter for different `Tasks` in the `Pipeline`.
 
+**Note:** You must specify all the `Parameters` that the `Pipeline` expects. Parameters 
+that have default values specified in Pipeline are not required to be provided by PipelineRun.
+
 For example:
 
 ```yaml
@@ -196,6 +199,10 @@ spec:
   - name: pl-param-y
     value: "500"
 ```
+You can pass in extra `Parameters` if needed depending on your use cases. An example use 
+case is when your CI system autogenerates `PipelineRuns` and it has `Parameters` it wants to 
+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.
 
 ### Specifying custom `ServiceAccount` credentials
 

examples/v1beta1/pipelineruns/pipelinerun-with-extra-params.yaml

@@ -0,0 +1,56 @@
+apiVersion: tekton.dev/v1beta1
+kind: Pipeline
+metadata:
+  name: pipeline-with-extra-params
+spec:
+  params:
+  - name: pl-param-x
+    type: string
+  - name: pl-param-y
+    type: string
+  tasks:
+  - name: add-params
+    taskRef:
+      name: add-params
+    params:
+    - name: a
+      value: "$(params.pl-param-x)"
+    - name: b
+      value: "$(params.pl-param-y)"
+---
+apiVersion: tekton.dev/v1beta1
+kind: Task
+metadata:
+  name: add-params
+  annotations:
+    description: |
+      A simple task that sums the two provided integers
+spec:
+  params:
+  - name: a
+    type: string
+    description: The first integer
+  - name: b
+    type: string
+    description: The second integer
+  steps:
+  - name: sum
+    image: bash:latest
+    script: |
+      #!/usr/bin/env bash
+      echo -n $(( "$(inputs.params.a)" + "$(inputs.params.b)" ))
+---
+apiVersion: tekton.dev/v1beta1
+kind: PipelineRun
+metadata:
+  generateName: pipelinerun-with-extra-params
+spec:
+  params:
+  - name: pl-param-x
+    value: "100"
+  - name: pl-param-y
+    value: "200"
+  - name: pl-param-z # the extra parameter
+    value: "300"
+  pipelineRef:
+    name: pipeline-with-extra-params

pkg/reconciler/pipelinerun/pipelinerun.go

@@ -80,6 +80,9 @@ const (
 	// ReasonCouldntGetCondition indicates that the reason for the failure status is that the
 	// associated Pipeline's Conditions couldn't all be retrieved
 	ReasonCouldntGetCondition = "CouldntGetCondition"
+	// ReasonParameterMissing indicates that the reason for the failure status is that the
+	// associated PipelineRun didn't provide all the required parameters
+	ReasonParameterMissing = "ParameterMissing"
 	// ReasonFailedValidation indicates that the reason for failure status is
 	// that pipelinerun failed runtime validation
 	ReasonFailedValidation = "PipelineValidationFailed"
@@ -315,6 +318,20 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err
 			pipelineMeta.Namespace, pr.Name, err)
 		return nil
 	}
+	// Ensure that the PipelineRun provides all the parameters required by the Pipeline
+	err = resources.ValidateRequiredParametersProvided(&pipelineSpec.Params, &pr.Spec.Params)
+	if err != nil {
+		// This Run has failed, so we need to mark it as failed and stop reconciling it
+		pr.Status.SetCondition(&apis.Condition{
+			Type:   apis.ConditionSucceeded,
+			Status: corev1.ConditionFalse,
+			Reason: ReasonParameterMissing,
+			Message: fmt.Sprintf("PipelineRun %s parameters is missing some parameters required by Pipeline %s's parameters: %s",
+				fmt.Sprintf("%s/%s", pr.Namespace, pr.Name), fmt.Sprintf("%s/%s", pr.Namespace, pipelineMeta.Name), err),
+		})
+		return nil
+
+	}
 
 	// Ensure that the parameters from the PipelineRun are overriding Pipeline parameters with the same type.
 	// Weird substitution issues can occur if this is not validated (ApplyParameters() does not verify type).

pkg/reconciler/pipelinerun/pipelinerun_test.go

@@ -434,6 +434,10 @@ func TestReconcile_InvalidPipelineRuns(t *testing.T) {
 			tb.PipelineTask("some-task", "a-task-that-needs-array-params")),
 			tb.PipelineRunParam("some-param", "stringval"),
 		)),
+		tb.PipelineRun("pipelinerun-missing-params", tb.PipelineRunNamespace("foo"), tb.PipelineRunSpec("", tb.PipelineRunPipelineSpec(
+			tb.PipelineParamSpec("some-param", v1beta1.ParamTypeString),
+			tb.PipelineTask("some-task", "a-task-that-needs-params")),
+		)),
 	}
 	d := test.Data{
 		Tasks:        ts,
@@ -491,6 +495,10 @@ func TestReconcile_InvalidPipelineRuns(t *testing.T) {
 			name:        "invalid-embedded-pipeline-mismatching-parameter-types",
 			pipelineRun: prs[10],
 			reason:      ReasonParameterTypeMismatch,
+		}, {
+			name:        "invalid-pipeline-run-missing-params-shd-stop-reconciling",
+			pipelineRun: prs[11],
+			reason:      ReasonParameterMissing,
 		},
 	}
 

pkg/reconciler/pipelinerun/resources/validate_params.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
+	"github.com/tektoncd/pipeline/pkg/list"
 )
 
 // ValidateParamTypesMatching validate that parameters in PipelineRun override corresponding parameters in Pipeline of the same type.
@@ -46,3 +47,29 @@ func ValidateParamTypesMatching(p *v1beta1.PipelineSpec, pr *v1beta1.PipelineRun
 	}
 	return nil
 }
+
+// ValidateRequiredParametersProvided validates that all the parameters expected by the Pipeline are provided by the PipelineRun.
+// Extra Parameters are allowed, the Pipeline will use the Parameters it needs and ignore the other Parameters.
+func ValidateRequiredParametersProvided(pipelineParameters *[]v1beta1.ParamSpec, pipelineRunParameters *[]v1beta1.Param) error {
+	// Build a list of parameter names declared in pr.
+	var providedParams []string
+	for _, param := range *pipelineRunParameters {
+		providedParams = append(providedParams, param.Name)
+	}
+
+	var requiredParams []string
+	for _, param := range *pipelineParameters {
+		if param.Default == nil { // include only parameters that don't have default values specified in the Pipeline
+			requiredParams = append(requiredParams, param.Name)
+		}
+	}
+
+	// Build a list of parameter names in p that are missing from pr.
+	missingParams := list.DiffLeft(requiredParams, providedParams)
+
+	// Return an error with the missing parameters' names, or return nil if there are none.
+	if len(missingParams) != 0 {
+		return fmt.Errorf("PipelineRun missing parameters: %s", missingParams)
+	}
+	return nil
+}

pkg/reconciler/pipelinerun/resources/validate_params_test.go

@@ -121,3 +121,104 @@ func TestValidateParamTypesMatching_Invalid(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateRequiredParametersProvided_Valid(t *testing.T) {
+	tcs := []struct {
+		name string
+		pp   []v1beta1.ParamSpec
+		prp  []v1beta1.Param
+	}{{
+		name: "required string params provided",
+		pp: []v1beta1.ParamSpec{
+			{
+				Name: "required-string-param",
+				Type: v1beta1.ParamTypeString,
+			},
+		},
+		prp: []v1beta1.Param{
+			{
+				Name:  "required-string-param",
+				Value: *tb.ArrayOrString("somestring"),
+			},
+		},
+	}, {
+		name: "required array params provided",
+		pp: []v1beta1.ParamSpec{
+			{
+				Name: "required-array-param",
+				Type: v1beta1.ParamTypeArray,
+			},
+		},
+		prp: []v1beta1.Param{
+			{
+				Name:  "required-array-param",
+				Value: *tb.ArrayOrString("another", "array"),
+			},
+		},
+	}, {
+		name: "string params provided in default",
+		pp: []v1beta1.ParamSpec{
+			{
+				Name:    "string-param",
+				Type:    v1beta1.ParamTypeString,
+				Default: tb.ArrayOrString("somedefault"),
+			},
+		},
+		prp: []v1beta1.Param{
+			{
+				Name:  "another-string-param",
+				Value: *tb.ArrayOrString("somestring"),
+			},
+		},
+	}}
+	for _, tc := range tcs {
+		t.Run(tc.name, func(t *testing.T) {
+			if err := ValidateRequiredParametersProvided(&tc.pp, &tc.prp); err != nil {
+				t.Errorf("Didn't expect to see error when validating valid PipelineRun parameters but got: %v", err)
+			}
+		})
+	}
+}
+
+func TestValidateRequiredParametersProvided_Invalid(t *testing.T) {
+	tcs := []struct {
+		name string
+		pp   []v1beta1.ParamSpec
+		prp  []v1beta1.Param
+	}{{
+		name: "required string param missing",
+		pp: []v1beta1.ParamSpec{
+			{
+				Name: "required-string-param",
+				Type: v1beta1.ParamTypeString,
+			},
+		},
+		prp: []v1beta1.Param{
+			{
+				Name:  "another-string-param",
+				Value: *tb.ArrayOrString("anotherstring"),
+			},
+		},
+	}, {
+		name: "required array param missing",
+		pp: []v1beta1.ParamSpec{
+			{
+				Name: "required-array-param",
+				Type: v1beta1.ParamTypeArray,
+			},
+		},
+		prp: []v1beta1.Param{
+			{
+				Name:  "another-array-param",
+				Value: *tb.ArrayOrString("anotherstring"),
+			},
+		},
+	}}
+	for _, tc := range tcs {
+		t.Run(tc.name, func(t *testing.T) {
+			if err := ValidateRequiredParametersProvided(&tc.pp, &tc.prp); err == nil {
+				t.Errorf("Expected to see error when validating invalid PipelineRun parameters but saw none")
+			}
+		})
+	}
+}