From bcf1c7263790d5a9e2fb8d9a0f61a16e5e8fdf2a Mon Sep 17 00:00:00 2001 From: Jerop Date: Fri, 29 May 2020 17:25:04 -0400 Subject: [PATCH] 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 expected parameters to ensure they are in the list of provided parameters. 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 | 7 ++ .../pipelinerun-with-extra-params.yaml | 58 +++++++++++++ pkg/reconciler/pipelinerun/pipelinerun.go | 17 ++++ .../pipelinerun/pipelinerun_test.go | 8 ++ .../pipelinerun/resources/validate_params.go | 25 ++++++ .../resources/validate_params_test.go | 86 +++++++++++++++++++ 6 files changed, 201 insertions(+) create mode 100644 examples/v1beta1/pipelineruns/pipelinerun-with-extra-params.yaml diff --git a/docs/pipelineruns.md b/docs/pipelineruns.md index 3405b7e1830..c758c6f8c1e 100644 --- a/docs/pipelineruns.md +++ b/docs/pipelineruns.md @@ -185,6 +185,13 @@ 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`. +You must specify all the `Parameters` that the `Pipeline` expects. + +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. + For example: ```yaml diff --git a/examples/v1beta1/pipelineruns/pipelinerun-with-extra-params.yaml b/examples/v1beta1/pipelineruns/pipelinerun-with-extra-params.yaml new file mode 100644 index 00000000000..05c7e7b58b7 --- /dev/null +++ b/examples/v1beta1/pipelineruns/pipelinerun-with-extra-params.yaml @@ -0,0 +1,58 @@ +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/v1alpha1 +kind: Task +metadata: + name: add-params + annotations: + description: | + A simple task that sums the two provided integers +spec: + inputs: + 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/v1alpha1 +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 + \ No newline at end of file diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 43d9d621683..b9bffb2e115 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/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" + // ParameterMissing indicates that the reason for the failure status is that the + // associated PipelineRun didn't provide all the required parameters + ParameterMissing = "ParameterMissing" // ReasonFailedValidation indicates that the reason for failure status is // that pipelinerun failed runtime validation ReasonFailedValidation = "PipelineValidationFailed" @@ -440,6 +443,20 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) 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: ParameterMissing, + 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). diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index b3c4ea64f51..d8ec750bf7d 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -430,6 +430,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, @@ -487,6 +491,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: ParameterMissing, }, } diff --git a/pkg/reconciler/pipelinerun/resources/validate_params.go b/pkg/reconciler/pipelinerun/resources/validate_params.go index 80b4d61c0cf..2389e929829 100644 --- a/pkg/reconciler/pipelinerun/resources/validate_params.go +++ b/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,27 @@ 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 { + 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("pipeline run missing parameters: %s", missingParams) + } + return nil +} diff --git a/pkg/reconciler/pipelinerun/resources/validate_params_test.go b/pkg/reconciler/pipelinerun/resources/validate_params_test.go index c9a963cbbd2..7734970b8e8 100644 --- a/pkg/reconciler/pipelinerun/resources/validate_params_test.go +++ b/pkg/reconciler/pipelinerun/resources/validate_params_test.go @@ -121,3 +121,89 @@ 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-string-param", + Type: v1beta1.ParamTypeString, + }, + }, + prp: []v1beta1.Param{ + { + Name: "required-string-param", + Value: *tb.ArrayOrString("another", "array"), + }, + }, + }} + 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") + } + }) + } +}