From c4b0824d260ae09943f8a3c6a17c87558193cc53 Mon Sep 17 00:00:00 2001 From: Yongxuan Zhang Date: Tue, 7 Feb 2023 23:49:42 +0000 Subject: [PATCH] add feature gate check for param array indexing Before this commit, if alpha feature gate is not enabled, the array indexing params will not be added to string replacements, thus will lead to non-existent variable error. This is confusing to users and doesn't provide correct guidance. This commit adds the check to the array indexing validation. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com --- pkg/reconciler/pipelinerun/pipelinerun.go | 7 +- .../pipelinerun/pipelinerun_test.go | 37 ++++ .../pipelinerun/resources/validate_params.go | 67 +++--- .../resources/validate_params_test.go | 42 +++- pkg/reconciler/taskrun/validate_resources.go | 199 +++++++++++++----- .../taskrun/validate_resources_test.go | 24 ++- 6 files changed, 276 insertions(+), 100 deletions(-) diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 799d2abd37d..212fbef6416 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -94,6 +94,9 @@ const ( // ReasonObjectParameterMissKeys indicates that the object param value provided from PipelineRun spec // misses some keys required for the object param declared in Pipeline spec. ReasonObjectParameterMissKeys = "ObjectParameterMissKeys" + // ReasonParamArrayIndexingInvalid indicates that the use of param array indexing is not under correct api fields feature gate + // or the array is out of bound. + ReasonParamArrayIndexingInvalid = "ParamArrayIndexingInvalid" // ReasonCouldntGetTask indicates that the reason for the failure status is that the // associated Pipeline's Tasks couldn't all be retrieved ReasonCouldntGetTask = "CouldntGetTask" @@ -509,8 +512,8 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get // Ensure that the array reference is not out of bound if err := resources.ValidateParamArrayIndex(ctx, pipelineSpec, pr); err != nil { // This Run has failed, so we need to mark it as failed and stop reconciling it - pr.Status.MarkFailed(ReasonObjectParameterMissKeys, - "PipelineRun %s/%s parameters is missing object keys required by Pipeline %s/%s's parameters: %s", + pr.Status.MarkFailed(ReasonParamArrayIndexingInvalid, + "PipelineRun %s/%s array indexing params fail validation by Pipeline %s/%s's parameters: %s", pr.Namespace, pr.Name, pr.Namespace, pipelineMeta.Name, err) return controller.NewPermanentError(err) } diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index e619df1aee9..40e0cd51f45 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -1061,6 +1061,22 @@ spec: name: a-task-that-needs-array-params `, v1beta1.ParamTypeArray)), parse.MustParseV1beta1Pipeline(t, fmt.Sprintf(` +metadata: + name: a-pipeline-with-array-indexing-params + namespace: foo +spec: + params: + - name: some-param + type: %s + tasks: + - name: some-task + taskRef: + name: a-task-that-needs-array-params + params: + - name: param + value: "$(params.some-param[2])" +`, v1beta1.ParamTypeArray)), + parse.MustParseV1beta1Pipeline(t, fmt.Sprintf(` metadata: name: a-pipeline-with-object-params namespace: foo @@ -1225,6 +1241,27 @@ spec: "Normal Started", "Warning Failed PipelineRun foo/pipeline-missing-object-param-keys parameters is missing object keys required by Pipeline foo/a-pipeline-with-object-params's parameters: PipelineRun missing object keys for parameters", }, + }, { + name: "invalid-pipeline-array-index-out-of-bound", + pipelineRun: parse.MustParseV1beta1PipelineRun(t, ` +metadata: + name: pipeline-param-array-out-of-bound + namespace: foo +spec: + pipelineRef: + name: a-pipeline-with-array-indexing-params + params: + - name: some-param + value: + - "a" + - "b" +`), + reason: ReasonParamArrayIndexingInvalid, + permanentError: true, + wantEvents: []string{ + "Normal Started", + "Warning Failed PipelineRun foo/pipeline-param-array-out-of-bound array indexing params fail validation by Pipeline foo/a-pipeline-with-array-indexing-params's parameters: non-existent param references:[$(params.some-param[2]", + }, }, { name: "invalid-embedded-pipeline-resources-bot-bound-shd-stop-reconciling", pipelineRun: parse.MustParseV1beta1PipelineRun(t, fmt.Sprintf(` diff --git a/pkg/reconciler/pipelinerun/resources/validate_params.go b/pkg/reconciler/pipelinerun/resources/validate_params.go index 363d18281c9..5c2da11fe28 100644 --- a/pkg/reconciler/pipelinerun/resources/validate_params.go +++ b/pkg/reconciler/pipelinerun/resources/validate_params.go @@ -20,11 +20,9 @@ import ( "context" "fmt" - "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/pkg/list" "github.com/tektoncd/pipeline/pkg/reconciler/taskrun" - "github.com/tektoncd/pipeline/pkg/substitution" "k8s.io/apimachinery/pkg/util/sets" ) @@ -91,40 +89,51 @@ func ValidateObjectParamRequiredKeys(pipelineParameters []v1beta1.ParamSpec, pip // ValidateParamArrayIndex validate if the array indexing param reference target is existent func ValidateParamArrayIndex(ctx context.Context, p *v1beta1.PipelineSpec, pr *v1beta1.PipelineRun) error { - cfg := config.FromContextOrDefaults(ctx) - if cfg.FeatureFlags.EnableAPIFields != config.AlphaAPIFields { - return nil - } - arrayParams := extractParamIndexes(p.Params, pr.Spec.Params) outofBoundParams := sets.String{} // collect all the references for i := range p.Tasks { - findInvalidParamArrayReferences(p.Tasks[i].Params, arrayParams, &outofBoundParams) + if err := findInvalidParamArrayReferences(ctx, p.Tasks[i].Params, arrayParams, &outofBoundParams); err != nil { + return err + } if p.Tasks[i].IsMatrixed() { - findInvalidParamArrayReferences(p.Tasks[i].Matrix.Params, arrayParams, &outofBoundParams) + if err := findInvalidParamArrayReferences(ctx, p.Tasks[i].Matrix.Params, arrayParams, &outofBoundParams); err != nil { + return err + } } for j := range p.Tasks[i].Workspaces { - findInvalidParamArrayReference(p.Tasks[i].Workspaces[j].SubPath, arrayParams, &outofBoundParams) + if err := taskrun.FindInvalidParamArrayReference(ctx, p.Tasks[i].Workspaces[j].SubPath, arrayParams, &outofBoundParams); err != nil { + return err + } } for _, wes := range p.Tasks[i].WhenExpressions { - findInvalidParamArrayReference(wes.Input, arrayParams, &outofBoundParams) + if err := taskrun.FindInvalidParamArrayReference(ctx, wes.Input, arrayParams, &outofBoundParams); err != nil { + return err + } for _, v := range wes.Values { - findInvalidParamArrayReference(v, arrayParams, &outofBoundParams) + if err := taskrun.FindInvalidParamArrayReference(ctx, v, arrayParams, &outofBoundParams); err != nil { + return err + } } } } for i := range p.Finally { - findInvalidParamArrayReferences(p.Finally[i].Params, arrayParams, &outofBoundParams) + if err := findInvalidParamArrayReferences(ctx, p.Finally[i].Params, arrayParams, &outofBoundParams); err != nil { + return err + } if p.Finally[i].IsMatrixed() { - findInvalidParamArrayReferences(p.Finally[i].Matrix.Params, arrayParams, &outofBoundParams) + if err := findInvalidParamArrayReferences(ctx, p.Finally[i].Matrix.Params, arrayParams, &outofBoundParams); err != nil { + return err + } } for _, wes := range p.Finally[i].WhenExpressions { for _, v := range wes.Values { - findInvalidParamArrayReference(v, arrayParams, &outofBoundParams) + if err := taskrun.FindInvalidParamArrayReference(ctx, v, arrayParams, &outofBoundParams); err != nil { + return err + } } } } @@ -171,28 +180,22 @@ func extractParamIndexes(defaults []v1beta1.ParamSpec, params []v1beta1.Param) m return arrayParams } -func findInvalidParamArrayReferences(params []v1beta1.Param, arrayParams map[string]int, outofBoundParams *sets.String) { +// findInvalidParamArrayReferences validates all the params' array indexing references and check if they are valid +func findInvalidParamArrayReferences(ctx context.Context, params []v1beta1.Param, arrayParams map[string]int, outofBoundParams *sets.String) error { for i := range params { - findInvalidParamArrayReference(params[i].Value.StringVal, arrayParams, outofBoundParams) + if err := taskrun.FindInvalidParamArrayReference(ctx, params[i].Value.StringVal, arrayParams, outofBoundParams); err != nil { + return err + } for _, v := range params[i].Value.ArrayVal { - findInvalidParamArrayReference(v, arrayParams, outofBoundParams) + if err := taskrun.FindInvalidParamArrayReference(ctx, v, arrayParams, outofBoundParams); err != nil { + return err + } } for _, v := range params[i].Value.ObjectVal { - findInvalidParamArrayReference(v, arrayParams, outofBoundParams) - } - } -} - -func findInvalidParamArrayReference(paramReference string, arrayParams map[string]int, outofBoundParams *sets.String) { - list := substitution.ExtractParamsExpressions(paramReference) - for _, val := range list { - indexString := substitution.ExtractIndexString(paramReference) - idx, _ := substitution.ExtractIndex(indexString) - v := substitution.TrimArrayIndex(val) - if paramLength, ok := arrayParams[v]; ok { - if idx >= paramLength { - outofBoundParams.Insert(val) + if err := taskrun.FindInvalidParamArrayReference(ctx, v, arrayParams, outofBoundParams); err != nil { + return err } } } + return nil } diff --git a/pkg/reconciler/pipelinerun/resources/validate_params_test.go b/pkg/reconciler/pipelinerun/resources/validate_params_test.go index 27ae5f48479..fa8d988fe3a 100644 --- a/pkg/reconciler/pipelinerun/resources/validate_params_test.go +++ b/pkg/reconciler/pipelinerun/resources/validate_params_test.go @@ -506,15 +506,12 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { } func TestValidateParamArrayIndex_invalid(t *testing.T) { - ctx := context.Background() - cfg := config.FromContextOrDefaults(ctx) - cfg.FeatureFlags.EnableAPIFields = config.AlphaAPIFields - ctx = config.ToContext(ctx, cfg) for _, tt := range []struct { - name string - original v1beta1.PipelineSpec - params []v1beta1.Param - expected error + name string + original v1beta1.PipelineSpec + params []v1beta1.Param + apifields string + expected error }{{ name: "single parameter reference out of bound", original: v1beta1.PipelineSpec{ @@ -698,11 +695,38 @@ func TestValidateParamArrayIndex_invalid(t *testing.T) { }, params: []v1beta1.Param{{Name: "second-param", Value: *v1beta1.NewStructuredValues("second-value", "second-value-again")}}, expected: fmt.Errorf("non-existent param references:[$(params.first-param[2]) $(params.second-param[3])]"), + }, { + name: "alpha gate not enabled", + original: v1beta1.PipelineSpec{ + Params: []v1beta1.ParamSpec{ + {Name: "first-param", Type: v1beta1.ParamTypeArray, Default: v1beta1.NewStructuredValues("default-value", "default-value-again")}, + {Name: "second-param", Type: v1beta1.ParamTypeArray}, + }, + Tasks: []v1beta1.PipelineTask{{ + Params: []v1beta1.Param{ + {Name: "first-task-first-param", Value: *v1beta1.NewStructuredValues("$(params.first-param[2])")}, + {Name: "first-task-second-param", Value: *v1beta1.NewStructuredValues("static value")}, + }, + Workspaces: []v1beta1.WorkspacePipelineTaskBinding{ + { + Name: "first-workspace", + Workspace: "first-workspace", + SubPath: "$(params.second-param[3])", + }, + }, + }}, + }, + params: []v1beta1.Param{{Name: "second-param", Value: *v1beta1.NewStructuredValues("second-value", "second-value-again")}}, + apifields: "stable", + expected: fmt.Errorf(`indexing into array param %s requires "enable-api-fields" feature gate to be "alpha"`, "$(params.first-param[2])"), }, } { tt := tt // capture range variable t.Run(tt.name, func(t *testing.T) { - t.Parallel() + ctx := context.Background() + if tt.apifields != "stable" { + ctx = config.ToContext(ctx, &config.Config{FeatureFlags: &config.FeatureFlags{EnableAPIFields: "alpha"}}) + } run := &v1beta1.PipelineRun{ Spec: v1beta1.PipelineRunSpec{ Params: tt.params, diff --git a/pkg/reconciler/taskrun/validate_resources.go b/pkg/reconciler/taskrun/validate_resources.go index 31d4ecc2382..4570104afa3 100644 --- a/pkg/reconciler/taskrun/validate_resources.go +++ b/pkg/reconciler/taskrun/validate_resources.go @@ -367,11 +367,6 @@ func missingKeysofObjectResults(tr *v1beta1.TaskRun, specResults []v1beta1.TaskR } func validateParamArrayIndex(ctx context.Context, params []v1beta1.Param, spec *v1beta1.TaskSpec) error { - cfg := config.FromContextOrDefaults(ctx) - if cfg.FeatureFlags.EnableAPIFields != config.AlphaAPIFields { - return nil - } - var defaults []v1beta1.ParamSpec if len(spec.Params) > 0 { defaults = append(defaults, spec.Params...) @@ -412,20 +407,26 @@ func validateParamArrayIndex(ctx context.Context, params []v1beta1.Param, spec * outofBoundParams := sets.String{} // Validate array param in steps fields. - validateStepsParamArrayIndexing(spec.Steps, arrayParams, &outofBoundParams) - + if err := validateStepsParamArrayIndexing(ctx, spec.Steps, arrayParams, &outofBoundParams); err != nil { + return err + } // Validate array param in StepTemplate fields. - validateStepsTemplateParamArrayIndexing(spec.StepTemplate, arrayParams, &outofBoundParams) - + if err := validateStepsTemplateParamArrayIndexing(ctx, spec.StepTemplate, arrayParams, &outofBoundParams); err != nil { + return err + } // Validate array param in build's volumes - validateVolumesParamArrayIndexing(spec.Volumes, arrayParams, &outofBoundParams) - + if err := validateVolumesParamArrayIndexing(ctx, spec.Volumes, arrayParams, &outofBoundParams); err != nil { + return err + } for _, v := range spec.Workspaces { - extractParamIndex(v.MountPath, arrayParams, &outofBoundParams) + if err := FindInvalidParamArrayReference(ctx, v.MountPath, arrayParams, &outofBoundParams); err != nil { + return err + } } - validateSidecarsParamArrayIndexing(spec.Sidecars, arrayParams, &outofBoundParams) - + if err := validateSidecarsParamArrayIndexing(ctx, spec.Sidecars, arrayParams, &outofBoundParams); err != nil { + return err + } if outofBoundParams.Len() > 0 { return fmt.Errorf("non-existent param references:%v", outofBoundParams.List()) } @@ -433,10 +434,18 @@ func validateParamArrayIndex(ctx context.Context, params []v1beta1.Param, spec * return nil } -func extractParamIndex(paramReference string, arrayParams map[string]int, outofBoundParams *sets.String) { +// FindInvalidParamArrayReference checks if the alpha feature gate is enabled when array +// indexing is used. And also find out of bound params +func FindInvalidParamArrayReference(ctx context.Context, paramReference string, arrayParams map[string]int, outofBoundParams *sets.String) error { list := substitution.ExtractParamsExpressions(paramReference) for _, val := range list { indexString := substitution.ExtractIndexString(paramReference) + if indexString != "" { + cfg := config.FromContextOrDefaults(ctx) + if cfg.FeatureFlags.EnableAPIFields != config.AlphaAPIFields { + return fmt.Errorf(`indexing into array param %s requires "enable-api-fields" feature gate to be "alpha"`, paramReference) + } + } idx, _ := substitution.ExtractIndex(indexString) v := substitution.TrimArrayIndex(val) if paramLength, ok := arrayParams[v]; ok { @@ -445,119 +454,197 @@ func extractParamIndex(paramReference string, arrayParams map[string]int, outofB } } } + return nil } -func validateStepsParamArrayIndexing(steps []v1beta1.Step, arrayParams map[string]int, outofBoundParams *sets.String) { +// validateStepsParamArrayIndexing validates array indexing params references in steps +func validateStepsParamArrayIndexing(ctx context.Context, steps []v1beta1.Step, arrayParams map[string]int, outofBoundParams *sets.String) error { for _, step := range steps { - extractParamIndex(step.Script, arrayParams, outofBoundParams) + if err := FindInvalidParamArrayReference(ctx, step.Script, arrayParams, outofBoundParams); err != nil { + return err + } container := step.ToK8sContainer() - validateContainerParamArrayIndexing(container, arrayParams, outofBoundParams) + if err := validateContainerParamArrayIndexing(ctx, container, arrayParams, outofBoundParams); err != nil { + return err + } } + return nil } -func validateStepsTemplateParamArrayIndexing(stepTemplate *v1beta1.StepTemplate, arrayParams map[string]int, outofBoundParams *sets.String) { +// validateStepsTemplateParamArrayIndexing validates array indexing params references in steps template +func validateStepsTemplateParamArrayIndexing(ctx context.Context, stepTemplate *v1beta1.StepTemplate, arrayParams map[string]int, outofBoundParams *sets.String) error { if stepTemplate == nil { - return + return nil } container := stepTemplate.ToK8sContainer() - validateContainerParamArrayIndexing(container, arrayParams, outofBoundParams) + return validateContainerParamArrayIndexing(ctx, container, arrayParams, outofBoundParams) } -func validateSidecarsParamArrayIndexing(sidecars []v1beta1.Sidecar, arrayParams map[string]int, outofBoundParams *sets.String) { +// validateSidecarsParamArrayIndexing validates array indexing params references in sidecars +func validateSidecarsParamArrayIndexing(ctx context.Context, sidecars []v1beta1.Sidecar, arrayParams map[string]int, outofBoundParams *sets.String) error { for _, s := range sidecars { - extractParamIndex(s.Script, arrayParams, outofBoundParams) + if err := FindInvalidParamArrayReference(ctx, s.Script, arrayParams, outofBoundParams); err != nil { + return err + } container := s.ToK8sContainer() - validateContainerParamArrayIndexing(container, arrayParams, outofBoundParams) + if err := validateContainerParamArrayIndexing(ctx, container, arrayParams, outofBoundParams); err != nil { + return err + } } + return nil } -func validateVolumesParamArrayIndexing(volumes []corev1.Volume, arrayParams map[string]int, outofBoundParams *sets.String) { +// validateVolumesParamArrayIndexing validates array indexing params references in volumes +func validateVolumesParamArrayIndexing(ctx context.Context, volumes []corev1.Volume, arrayParams map[string]int, outofBoundParams *sets.String) error { for i, v := range volumes { - extractParamIndex(v.Name, arrayParams, outofBoundParams) + if err := FindInvalidParamArrayReference(ctx, v.Name, arrayParams, outofBoundParams); err != nil { + return err + } if v.VolumeSource.ConfigMap != nil { - extractParamIndex(v.ConfigMap.Name, arrayParams, outofBoundParams) + if err := FindInvalidParamArrayReference(ctx, v.ConfigMap.Name, arrayParams, outofBoundParams); err != nil { + return err + } for _, item := range v.ConfigMap.Items { - extractParamIndex(item.Key, arrayParams, outofBoundParams) - extractParamIndex(item.Path, arrayParams, outofBoundParams) + if err := FindInvalidParamArrayReference(ctx, item.Key, arrayParams, outofBoundParams); err != nil { + return err + } + if err := FindInvalidParamArrayReference(ctx, item.Path, arrayParams, outofBoundParams); err != nil { + return err + } } } if v.VolumeSource.Secret != nil { - extractParamIndex(v.Secret.SecretName, arrayParams, outofBoundParams) + if err := FindInvalidParamArrayReference(ctx, v.Secret.SecretName, arrayParams, outofBoundParams); err != nil { + return err + } for _, item := range v.Secret.Items { - extractParamIndex(item.Key, arrayParams, outofBoundParams) - extractParamIndex(item.Path, arrayParams, outofBoundParams) + if err := FindInvalidParamArrayReference(ctx, item.Key, arrayParams, outofBoundParams); err != nil { + return err + } + if err := FindInvalidParamArrayReference(ctx, item.Path, arrayParams, outofBoundParams); err != nil { + return err + } } } if v.PersistentVolumeClaim != nil { - extractParamIndex(v.PersistentVolumeClaim.ClaimName, arrayParams, outofBoundParams) + if err := FindInvalidParamArrayReference(ctx, v.PersistentVolumeClaim.ClaimName, arrayParams, outofBoundParams); err != nil { + return err + } } if v.Projected != nil { for _, s := range volumes[i].Projected.Sources { if s.ConfigMap != nil { - extractParamIndex(s.ConfigMap.Name, arrayParams, outofBoundParams) + if err := FindInvalidParamArrayReference(ctx, s.ConfigMap.Name, arrayParams, outofBoundParams); err != nil { + return err + } } if s.Secret != nil { - extractParamIndex(s.Secret.Name, arrayParams, outofBoundParams) + if err := FindInvalidParamArrayReference(ctx, s.Secret.Name, arrayParams, outofBoundParams); err != nil { + return err + } } if s.ServiceAccountToken != nil { - extractParamIndex(s.ServiceAccountToken.Audience, arrayParams, outofBoundParams) + if err := FindInvalidParamArrayReference(ctx, s.ServiceAccountToken.Audience, arrayParams, outofBoundParams); err != nil { + return err + } } } } if v.CSI != nil { if v.CSI.NodePublishSecretRef != nil { - extractParamIndex(v.CSI.NodePublishSecretRef.Name, arrayParams, outofBoundParams) + if err := FindInvalidParamArrayReference(ctx, v.CSI.NodePublishSecretRef.Name, arrayParams, outofBoundParams); err != nil { + return err + } } if v.CSI.VolumeAttributes != nil { for _, value := range v.CSI.VolumeAttributes { - extractParamIndex(value, arrayParams, outofBoundParams) + if err := FindInvalidParamArrayReference(ctx, value, arrayParams, outofBoundParams); err != nil { + return err + } } } } } + return nil } -func validateContainerParamArrayIndexing(c *corev1.Container, arrayParams map[string]int, outofBoundParams *sets.String) { - extractParamIndex(c.Name, arrayParams, outofBoundParams) - extractParamIndex(c.Image, arrayParams, outofBoundParams) - extractParamIndex(string(c.ImagePullPolicy), arrayParams, outofBoundParams) +// validateContainerParamArrayIndexing validates array indexing params references in container +func validateContainerParamArrayIndexing(ctx context.Context, c *corev1.Container, arrayParams map[string]int, outofBoundParams *sets.String) error { + if err := FindInvalidParamArrayReference(ctx, c.Name, arrayParams, outofBoundParams); err != nil { + return err + } + if err := FindInvalidParamArrayReference(ctx, c.Image, arrayParams, outofBoundParams); err != nil { + return err + } + if err := FindInvalidParamArrayReference(ctx, string(c.ImagePullPolicy), arrayParams, outofBoundParams); err != nil { + return err + } for _, a := range c.Args { - extractParamIndex(a, arrayParams, outofBoundParams) + if err := FindInvalidParamArrayReference(ctx, a, arrayParams, outofBoundParams); err != nil { + return err + } } for ie, e := range c.Env { - extractParamIndex(e.Value, arrayParams, outofBoundParams) + if err := FindInvalidParamArrayReference(ctx, e.Value, arrayParams, outofBoundParams); err != nil { + return err + } if c.Env[ie].ValueFrom != nil { if e.ValueFrom.SecretKeyRef != nil { - extractParamIndex(e.ValueFrom.SecretKeyRef.LocalObjectReference.Name, arrayParams, outofBoundParams) - extractParamIndex(e.ValueFrom.SecretKeyRef.Key, arrayParams, outofBoundParams) + if err := FindInvalidParamArrayReference(ctx, e.ValueFrom.SecretKeyRef.LocalObjectReference.Name, arrayParams, outofBoundParams); err != nil { + return err + } + if err := FindInvalidParamArrayReference(ctx, e.ValueFrom.SecretKeyRef.Key, arrayParams, outofBoundParams); err != nil { + return err + } } if e.ValueFrom.ConfigMapKeyRef != nil { - extractParamIndex(e.ValueFrom.ConfigMapKeyRef.LocalObjectReference.Name, arrayParams, outofBoundParams) - extractParamIndex(e.ValueFrom.ConfigMapKeyRef.Key, arrayParams, outofBoundParams) + if err := FindInvalidParamArrayReference(ctx, e.ValueFrom.ConfigMapKeyRef.LocalObjectReference.Name, arrayParams, outofBoundParams); err != nil { + return err + } + if err := FindInvalidParamArrayReference(ctx, e.ValueFrom.ConfigMapKeyRef.Key, arrayParams, outofBoundParams); err != nil { + return err + } } } } for _, e := range c.EnvFrom { - extractParamIndex(e.Prefix, arrayParams, outofBoundParams) + if err := FindInvalidParamArrayReference(ctx, e.Prefix, arrayParams, outofBoundParams); err != nil { + return err + } if e.ConfigMapRef != nil { - extractParamIndex(e.ConfigMapRef.LocalObjectReference.Name, arrayParams, outofBoundParams) + if err := FindInvalidParamArrayReference(ctx, e.ConfigMapRef.LocalObjectReference.Name, arrayParams, outofBoundParams); err != nil { + return err + } } if e.SecretRef != nil { - extractParamIndex(e.SecretRef.LocalObjectReference.Name, arrayParams, outofBoundParams) + if err := FindInvalidParamArrayReference(ctx, e.SecretRef.LocalObjectReference.Name, arrayParams, outofBoundParams); err != nil { + return err + } } } - extractParamIndex(c.WorkingDir, arrayParams, outofBoundParams) + if err := FindInvalidParamArrayReference(ctx, c.WorkingDir, arrayParams, outofBoundParams); err != nil { + return err + } for _, cc := range c.Command { - extractParamIndex(cc, arrayParams, outofBoundParams) + if err := FindInvalidParamArrayReference(ctx, cc, arrayParams, outofBoundParams); err != nil { + return err + } } for _, v := range c.VolumeMounts { - extractParamIndex(v.Name, arrayParams, outofBoundParams) - extractParamIndex(v.MountPath, arrayParams, outofBoundParams) - extractParamIndex(v.SubPath, arrayParams, outofBoundParams) + if err := FindInvalidParamArrayReference(ctx, v.Name, arrayParams, outofBoundParams); err != nil { + return err + } + if err := FindInvalidParamArrayReference(ctx, v.MountPath, arrayParams, outofBoundParams); err != nil { + return err + } + if err := FindInvalidParamArrayReference(ctx, v.SubPath, arrayParams, outofBoundParams); err != nil { + return err + } } + return nil } diff --git a/pkg/reconciler/taskrun/validate_resources_test.go b/pkg/reconciler/taskrun/validate_resources_test.go index 19db70401af..682533cef62 100644 --- a/pkg/reconciler/taskrun/validate_resources_test.go +++ b/pkg/reconciler/taskrun/validate_resources_test.go @@ -952,6 +952,7 @@ func TestValidateParamArrayIndex(t *testing.T) { name string params []v1beta1.Param taskspec *v1beta1.TaskSpec + apifields string expectedError error }{{ name: "steps reference invalid", @@ -1120,11 +1121,32 @@ func TestValidateParamArrayIndex(t *testing.T) { }, }, expectedError: fmt.Errorf("non-existent param references:[%v]", "$(params.array-params[3])"), + }, { + name: "alpha gate not enabled", + params: []v1beta1.Param{{ + Name: "array-params", + Value: *v1beta1.NewStructuredValues("bar", "foo"), + }}, + taskspec: &v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{{ + Name: "array-params", + Default: v1beta1.NewStructuredValues("bar", "foo"), + }}, + Sidecars: []v1beta1.Sidecar{{ + Script: "$(params.array-params[3])", + }, + }, + }, + apifields: config.StableAPIFields, + expectedError: fmt.Errorf(`indexing into array param %s requires "enable-api-fields" feature gate to be "alpha"`, "$(params.array-params[3])"), }, } for _, tc := range tcs { t.Run(tc.name, func(t *testing.T) { - ctx := config.ToContext(context.Background(), &config.Config{FeatureFlags: &config.FeatureFlags{EnableAPIFields: "alpha"}}) + ctx := context.Background() + if tc.apifields != config.StableAPIFields { + ctx = config.ToContext(ctx, &config.Config{FeatureFlags: &config.FeatureFlags{EnableAPIFields: "alpha"}}) + } err := validateParamArrayIndex(ctx, tc.params, tc.taskspec) if d := cmp.Diff(tc.expectedError.Error(), err.Error()); d != "" { t.Errorf("validateParamArrayIndex() errors diff %s", diff.PrintWantGot(d))