From f30249a995ba2bafbb38feac505a95401dfd6504 Mon Sep 17 00:00:00 2001 From: Emma Munley Date: Thu, 9 Feb 2023 13:25:18 -0500 Subject: [PATCH] Added Validation: Matrix parameters cannot be empty arrays Added a check in Pipeline validation to verify that matrix parameters are not just arrays, but that they are not empty arrays. This won't change existing functionality/capabilities, but will get rid of the panic in the reconciler. --- docs/matrix.md | 27 ++++++++++--------- pkg/apis/pipeline/v1/param_types.go | 3 +++ pkg/apis/pipeline/v1/pipeline_types_test.go | 15 +++++++++++ pkg/apis/pipeline/v1beta1/param_types.go | 3 +++ .../pipeline/v1beta1/pipeline_types_test.go | 15 +++++++++++ .../clientset/versioned/fake/register.go | 14 +++++----- .../clientset/versioned/scheme/register.go | 14 +++++----- .../clientset/versioned/fake/register.go | 14 +++++----- .../clientset/versioned/scheme/register.go | 14 +++++----- .../clientset/versioned/fake/register.go | 14 +++++----- .../clientset/versioned/scheme/register.go | 14 +++++----- 11 files changed, 92 insertions(+), 55 deletions(-) diff --git a/docs/matrix.md b/docs/matrix.md index d4f991651a0..1a8e1a2d96c 100644 --- a/docs/matrix.md +++ b/docs/matrix.md @@ -24,7 +24,7 @@ weight: 11 ## Overview `Matrix` is used to fan out `Tasks` in a `Pipeline`. This doc will explain the details of `matrix` support in -Tekton. +Tekton. Documentation for specifying `Matrix` in a `Pipeline`: - [Specifying `Matrix` in `Tasks`](pipelines.md#specifying-matrix-in-pipelinetasks) @@ -40,12 +40,12 @@ A `Matrix` supports the following features: * [Concurrency Control](#concurrency-control) * [Parameters](#parameters) * [Context Variables](#context-variables) -* [Results](#results) +* [Results](#results) ### Concurrency Control The default maximum count of `TaskRuns` or `Runs` from a given `Matrix` is **256**. To customize the maximum count of -`TaskRuns` or `Runs` generated from a given `Matrix`, configure the `default-max-matrix-combinations-count` in +`TaskRuns` or `Runs` generated from a given `Matrix`, configure the `default-max-matrix-combinations-count` in [config defaults](/config/config-defaults.yaml). When a `Matrix` in `PipelineTask` would generate more than the maximum `TaskRuns` or `Runs`, the `Pipeline` validation would fail. @@ -69,6 +69,7 @@ The `Matrix` will take `Parameters` of type `"array"` only, which will be suppli `PipelineTask` by substituting `Parameters` of type `"string"` in the underlying `Task`. The names of the `Parameters` in the `Matrix` must match the names of the `Parameters` in the underlying `Task` that they will be substituting. +Note: The `Paramaters` of type `array` cannot be empty. In the example below, the *test* `Task` takes *browser* and *platform* `Parameters` of type `"string"`. A `Pipeline` used to fan out the `Task` using `Matrix` would have two `Parameters` @@ -92,7 +93,7 @@ spec: default: - chrome - safari - - firefox + - firefox tasks: - name: fetch-repository taskRef: @@ -110,7 +111,7 @@ spec: ... ``` -A `Parameter` can be passed to either the `matrix` or `params` field, not both. +A `Parameter` can be passed to either the `matrix` or `params` field, not both. For further details on specifying `Parameters` in the `Pipeline` and passing them to `PipelineTasks`, see [documentation](pipelines.md#specifying-parameters). @@ -150,7 +151,7 @@ spec: ### Context Variables -Similarly to the `Parameters` in the `Params` field, the `Parameters` in the `Matrix` field will accept +Similarly to the `Parameters` in the `Params` field, the `Parameters` in the `Matrix` field will accept [context variables](variables.md) that will be substituted, including: * `PipelineRun` name, namespace and uid @@ -161,7 +162,7 @@ Similarly to the `Parameters` in the `Params` field, the `Parameters` in the `Ma #### Specifying Results in a Matrix -Consuming `Results` from previous `TaskRuns` or `Runs` in a `Matrix`, which would dynamically generate +Consuming `Results` from previous `TaskRuns` or `Runs` in a `Matrix`, which would dynamically generate `TaskRuns` or `Runs` from the fanned out `PipelineTask`, is supported. Producing `Results` in from a `PipelineTask` with a `Matrix` is not yet supported - see [further details](#results-from-fanned-out-pipelinetasks). @@ -203,7 +204,7 @@ tasks: Consuming `Results` from fanned out `PipelineTasks` will not be in the supported in the initial iteration of `Matrix`. Supporting consuming `Results` from fanned out `PipelineTasks` will be revisited after array -and object `Results` are supported. +and object `Results` are supported. ## Fan Out @@ -487,7 +488,7 @@ status: name: platforms-and-browsers taskRef: apiVersion: cel.tekton.dev/v1alpha1 - kind: CEL + kind: CEL startTime: "2022-06-28T20:49:40Z" completionTime: "2022-06-28T20:49:41Z" conditions: @@ -495,7 +496,7 @@ status: message: 'Tasks Completed: 1 (Failed: 0, Cancelled 0), Skipped: 0' reason: Succeeded status: "True" - type: Succeeded + type: Succeeded childReferences: - apiVersion: tekton.dev/v1alpha1 kind: Run @@ -533,11 +534,11 @@ status: ## Retries -The `retries` field is used to specify the number of times a `PipelineTask` should be retried when its `TaskRun` or -`Run` fails, see the [documentation][retries] for further details. When a `PipelineTask` is fanned out using `Matrix`, +The `retries` field is used to specify the number of times a `PipelineTask` should be retried when its `TaskRun` or +`Run` fails, see the [documentation][retries] for further details. When a `PipelineTask` is fanned out using `Matrix`, a given `TaskRun` or `Run` executed will be retried as much as the field in the `retries` field of the `PipelineTask`. -For example, the `PipelineTask` in this `PipelineRun` will be fanned out into three `TaskRuns` each of which will be +For example, the `PipelineTask` in this `PipelineRun` will be fanned out into three `TaskRuns` each of which will be retried once: ```yaml diff --git a/pkg/apis/pipeline/v1/param_types.go b/pkg/apis/pipeline/v1/param_types.go index 2269c0896c5..51e2b63a161 100644 --- a/pkg/apis/pipeline/v1/param_types.go +++ b/pkg/apis/pipeline/v1/param_types.go @@ -331,6 +331,9 @@ func validateParametersInTaskMatrix(matrix *Matrix) (errs *apis.FieldError) { if param.Value.Type != ParamTypeArray { errs = errs.Also(apis.ErrInvalidValue("parameters of type array only are allowed in matrix", "").ViaFieldKey("matrix", param.Name)) } + if param.Value.Type == ParamTypeArray && len(param.Value.ArrayVal) == 0 { + errs = errs.Also(apis.ErrInvalidValue("empty arrays not allowed as parameters in matrix", "").ViaFieldKey("matrix", param.Name)) + } } } return errs diff --git a/pkg/apis/pipeline/v1/pipeline_types_test.go b/pkg/apis/pipeline/v1/pipeline_types_test.go index b1dc6f904ea..3f36793d18f 100644 --- a/pkg/apis/pipeline/v1/pipeline_types_test.go +++ b/pkg/apis/pipeline/v1/pipeline_types_test.go @@ -645,6 +645,21 @@ func TestPipelineTask_validateMatrix(t *testing.T) { Name: "barfoo", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"bar", "foo"}}, }}}, }, + }, { + name: "parameters in matrix are not empty arrays", + pt: &PipelineTask{ + Name: "task", + Matrix: &Matrix{ + Params: []Param{{ + Name: "foobar", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{}}, + }, { + Name: "barfoo", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{}}, + }}}, + }, + wantErrs: &apis.FieldError{ + Message: "invalid value: empty arrays not allowed as parameters in matrix", + Paths: []string{"matrix[barfoo]", "matrix[foobar]"}, + }, }, { name: "parameters in matrix contain results references", pt: &PipelineTask{ diff --git a/pkg/apis/pipeline/v1beta1/param_types.go b/pkg/apis/pipeline/v1beta1/param_types.go index 959d6b7ca71..fe936915969 100644 --- a/pkg/apis/pipeline/v1beta1/param_types.go +++ b/pkg/apis/pipeline/v1beta1/param_types.go @@ -325,6 +325,9 @@ func validateParametersInTaskMatrix(matrix *Matrix) (errs *apis.FieldError) { if param.Value.Type != ParamTypeArray { errs = errs.Also(apis.ErrInvalidValue("parameters of type array only are allowed in matrix", "").ViaFieldKey("matrix", param.Name)) } + if param.Value.Type == ParamTypeArray && len(param.Value.ArrayVal) == 0 { + errs = errs.Also(apis.ErrInvalidValue("empty arrays not allowed as parameters in matrix", "").ViaFieldKey("matrix", param.Name)) + } } } return errs diff --git a/pkg/apis/pipeline/v1beta1/pipeline_types_test.go b/pkg/apis/pipeline/v1beta1/pipeline_types_test.go index 0e8693cb623..a59f7a0f6bc 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_types_test.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_types_test.go @@ -831,6 +831,21 @@ func TestPipelineTask_validateMatrix(t *testing.T) { Name: "barfoo", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"bar", "foo"}}, }}}, }, + }, { + name: "parameters in matrix are not empty arrays", + pt: &PipelineTask{ + Name: "task", + Matrix: &Matrix{ + Params: []Param{{ + Name: "foobar", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{}}, + }, { + Name: "barfoo", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{}}, + }}}, + }, + wantErrs: &apis.FieldError{ + Message: "invalid value: empty arrays not allowed as parameters in matrix", + Paths: []string{"matrix[barfoo]", "matrix[foobar]"}, + }, }, { name: "parameters in matrix contain results references", pt: &PipelineTask{ diff --git a/pkg/client/clientset/versioned/fake/register.go b/pkg/client/clientset/versioned/fake/register.go index 951cec59e8f..27907d4e65f 100644 --- a/pkg/client/clientset/versioned/fake/register.go +++ b/pkg/client/clientset/versioned/fake/register.go @@ -41,14 +41,14 @@ var localSchemeBuilder = runtime.SchemeBuilder{ // AddToScheme adds all types of this clientset into the given scheme. This allows composition // of clientsets, like in: // -// import ( -// "k8s.io/client-go/kubernetes" -// clientsetscheme "k8s.io/client-go/kubernetes/scheme" -// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" -// ) +// import ( +// "k8s.io/client-go/kubernetes" +// clientsetscheme "k8s.io/client-go/kubernetes/scheme" +// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" +// ) // -// kclientset, _ := kubernetes.NewForConfig(c) -// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) +// kclientset, _ := kubernetes.NewForConfig(c) +// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) // // After this, RawExtensions in Kubernetes types will serialize kube-aggregator types // correctly. diff --git a/pkg/client/clientset/versioned/scheme/register.go b/pkg/client/clientset/versioned/scheme/register.go index 21c3e532dfd..ffc8f29c1f7 100644 --- a/pkg/client/clientset/versioned/scheme/register.go +++ b/pkg/client/clientset/versioned/scheme/register.go @@ -41,14 +41,14 @@ var localSchemeBuilder = runtime.SchemeBuilder{ // AddToScheme adds all types of this clientset into the given scheme. This allows composition // of clientsets, like in: // -// import ( -// "k8s.io/client-go/kubernetes" -// clientsetscheme "k8s.io/client-go/kubernetes/scheme" -// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" -// ) +// import ( +// "k8s.io/client-go/kubernetes" +// clientsetscheme "k8s.io/client-go/kubernetes/scheme" +// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" +// ) // -// kclientset, _ := kubernetes.NewForConfig(c) -// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) +// kclientset, _ := kubernetes.NewForConfig(c) +// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) // // After this, RawExtensions in Kubernetes types will serialize kube-aggregator types // correctly. diff --git a/pkg/client/resolution/clientset/versioned/fake/register.go b/pkg/client/resolution/clientset/versioned/fake/register.go index 70d792b35d6..8469024ac97 100644 --- a/pkg/client/resolution/clientset/versioned/fake/register.go +++ b/pkg/client/resolution/clientset/versioned/fake/register.go @@ -39,14 +39,14 @@ var localSchemeBuilder = runtime.SchemeBuilder{ // AddToScheme adds all types of this clientset into the given scheme. This allows composition // of clientsets, like in: // -// import ( -// "k8s.io/client-go/kubernetes" -// clientsetscheme "k8s.io/client-go/kubernetes/scheme" -// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" -// ) +// import ( +// "k8s.io/client-go/kubernetes" +// clientsetscheme "k8s.io/client-go/kubernetes/scheme" +// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" +// ) // -// kclientset, _ := kubernetes.NewForConfig(c) -// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) +// kclientset, _ := kubernetes.NewForConfig(c) +// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) // // After this, RawExtensions in Kubernetes types will serialize kube-aggregator types // correctly. diff --git a/pkg/client/resolution/clientset/versioned/scheme/register.go b/pkg/client/resolution/clientset/versioned/scheme/register.go index 9fe0d86a62e..19e5de5b4c6 100644 --- a/pkg/client/resolution/clientset/versioned/scheme/register.go +++ b/pkg/client/resolution/clientset/versioned/scheme/register.go @@ -39,14 +39,14 @@ var localSchemeBuilder = runtime.SchemeBuilder{ // AddToScheme adds all types of this clientset into the given scheme. This allows composition // of clientsets, like in: // -// import ( -// "k8s.io/client-go/kubernetes" -// clientsetscheme "k8s.io/client-go/kubernetes/scheme" -// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" -// ) +// import ( +// "k8s.io/client-go/kubernetes" +// clientsetscheme "k8s.io/client-go/kubernetes/scheme" +// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" +// ) // -// kclientset, _ := kubernetes.NewForConfig(c) -// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) +// kclientset, _ := kubernetes.NewForConfig(c) +// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) // // After this, RawExtensions in Kubernetes types will serialize kube-aggregator types // correctly. diff --git a/pkg/client/resource/clientset/versioned/fake/register.go b/pkg/client/resource/clientset/versioned/fake/register.go index 78bbf3de2ae..e33dea7aa03 100644 --- a/pkg/client/resource/clientset/versioned/fake/register.go +++ b/pkg/client/resource/clientset/versioned/fake/register.go @@ -37,14 +37,14 @@ var localSchemeBuilder = runtime.SchemeBuilder{ // AddToScheme adds all types of this clientset into the given scheme. This allows composition // of clientsets, like in: // -// import ( -// "k8s.io/client-go/kubernetes" -// clientsetscheme "k8s.io/client-go/kubernetes/scheme" -// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" -// ) +// import ( +// "k8s.io/client-go/kubernetes" +// clientsetscheme "k8s.io/client-go/kubernetes/scheme" +// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" +// ) // -// kclientset, _ := kubernetes.NewForConfig(c) -// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) +// kclientset, _ := kubernetes.NewForConfig(c) +// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) // // After this, RawExtensions in Kubernetes types will serialize kube-aggregator types // correctly. diff --git a/pkg/client/resource/clientset/versioned/scheme/register.go b/pkg/client/resource/clientset/versioned/scheme/register.go index 2eb6b537e5a..972999462fa 100644 --- a/pkg/client/resource/clientset/versioned/scheme/register.go +++ b/pkg/client/resource/clientset/versioned/scheme/register.go @@ -37,14 +37,14 @@ var localSchemeBuilder = runtime.SchemeBuilder{ // AddToScheme adds all types of this clientset into the given scheme. This allows composition // of clientsets, like in: // -// import ( -// "k8s.io/client-go/kubernetes" -// clientsetscheme "k8s.io/client-go/kubernetes/scheme" -// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" -// ) +// import ( +// "k8s.io/client-go/kubernetes" +// clientsetscheme "k8s.io/client-go/kubernetes/scheme" +// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" +// ) // -// kclientset, _ := kubernetes.NewForConfig(c) -// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) +// kclientset, _ := kubernetes.NewForConfig(c) +// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) // // After this, RawExtensions in Kubernetes types will serialize kube-aggregator types // correctly.