From e838c5268b204540c792bb0b646f5eb87446337a Mon Sep 17 00:00:00 2001 From: Yongxuan Zhang Date: Thu, 16 Feb 2023 02:58:00 +0000 Subject: [PATCH] refactor param array indexing validation This commit refactors param array indexing validation to extract all references first, then validate if references are out of bound. Previously we traverse all references and collect invalid references there. The benefit of this refactoring is that it would be convenient for other validation to reuse these references. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com --- .../pipelinerun/resources/validate_params.go | 88 ++-------- pkg/reconciler/taskrun/validate_resources.go | 154 ++++++++++-------- 2 files changed, 102 insertions(+), 140 deletions(-) diff --git a/pkg/reconciler/pipelinerun/resources/validate_params.go b/pkg/reconciler/pipelinerun/resources/validate_params.go index 0b65d3683b9..5bf6559e6a6 100644 --- a/pkg/reconciler/pipelinerun/resources/validate_params.go +++ b/pkg/reconciler/pipelinerun/resources/validate_params.go @@ -24,8 +24,6 @@ import ( "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" ) // ValidateParamTypesMatching validate that parameters in PipelineRun override corresponding parameters in Pipeline of the same type. @@ -95,103 +93,49 @@ func ValidateParamArrayIndex(ctx context.Context, p *v1beta1.PipelineSpec, pr *v return nil } - arrayParams := extractParamIndexes(p.Params, pr.Spec.Params) - - outofBoundParams := sets.String{} + arrayParams := taskrun.ExtractParamArrayLengths(p.Params, pr.Spec.Params) + arrayIndexingParams := []string{} // collect all the references for i := range p.Tasks { - findInvalidParamArrayReferences(p.Tasks[i].Params, arrayParams, &outofBoundParams) + extractArrayIndexingReferencesFromParams(p.Tasks[i].Params, arrayParams, &arrayIndexingParams) if p.Tasks[i].IsMatrixed() { - findInvalidParamArrayReferences(p.Tasks[i].Matrix.Params, arrayParams, &outofBoundParams) + extractArrayIndexingReferencesFromParams(p.Tasks[i].Matrix.Params, arrayParams, &arrayIndexingParams) } for j := range p.Tasks[i].Workspaces { - findInvalidParamArrayReference(p.Tasks[i].Workspaces[j].SubPath, arrayParams, &outofBoundParams) + taskrun.ExtractArrayIndexingParamReference(p.Tasks[i].Workspaces[j].SubPath, arrayParams, &arrayIndexingParams) } for _, wes := range p.Tasks[i].WhenExpressions { - findInvalidParamArrayReference(wes.Input, arrayParams, &outofBoundParams) + taskrun.ExtractArrayIndexingParamReference(wes.Input, arrayParams, &arrayIndexingParams) for _, v := range wes.Values { - findInvalidParamArrayReference(v, arrayParams, &outofBoundParams) + taskrun.ExtractArrayIndexingParamReference(v, arrayParams, &arrayIndexingParams) } } } for i := range p.Finally { - findInvalidParamArrayReferences(p.Finally[i].Params, arrayParams, &outofBoundParams) + extractArrayIndexingReferencesFromParams(p.Finally[i].Params, arrayParams, &arrayIndexingParams) if p.Finally[i].IsMatrixed() { - findInvalidParamArrayReferences(p.Finally[i].Matrix.Params, arrayParams, &outofBoundParams) + extractArrayIndexingReferencesFromParams(p.Finally[i].Matrix.Params, arrayParams, &arrayIndexingParams) } for _, wes := range p.Finally[i].WhenExpressions { for _, v := range wes.Values { - findInvalidParamArrayReference(v, arrayParams, &outofBoundParams) + taskrun.ExtractArrayIndexingParamReference(v, arrayParams, &arrayIndexingParams) } } } - - if outofBoundParams.Len() > 0 { - return fmt.Errorf("non-existent param references:%v", outofBoundParams.List()) - } - return nil + return taskrun.ValidateOutofBoundArrayParams(arrayIndexingParams, arrayParams) } -func extractParamIndexes(defaults []v1beta1.ParamSpec, params []v1beta1.Param) map[string]int { - // Collect all array params - arrayParams := make(map[string]int) - - patterns := []string{ - "$(params.%s)", - "$(params[%q])", - "$(params['%s'])", - } - - // Collect array params lengths from defaults - for _, p := range defaults { - if p.Default != nil { - if p.Default.Type == v1beta1.ParamTypeArray { - for _, pattern := range patterns { - for i := 0; i < len(p.Default.ArrayVal); i++ { - arrayParams[fmt.Sprintf(pattern, p.Name)] = len(p.Default.ArrayVal) - } - } - } - } - } - - // Collect array params lengths from pipelinerun or taskrun - for _, p := range params { - if p.Value.Type == v1beta1.ParamTypeArray { - for _, pattern := range patterns { - for i := 0; i < len(p.Value.ArrayVal); i++ { - arrayParams[fmt.Sprintf(pattern, p.Name)] = len(p.Value.ArrayVal) - } - } - } - } - return arrayParams -} - -func findInvalidParamArrayReferences(params []v1beta1.Param, arrayParams map[string]int, outofBoundParams *sets.String) { +// extractArrayIndexingReferencesFromParams get all array indexing references from params +func extractArrayIndexingReferencesFromParams(params []v1beta1.Param, arrayParams map[string]int, arrayIndexingParams *[]string) { for i := range params { - findInvalidParamArrayReference(params[i].Value.StringVal, arrayParams, outofBoundParams) + taskrun.ExtractArrayIndexingParamReference(params[i].Value.StringVal, arrayParams, arrayIndexingParams) for _, v := range params[i].Value.ArrayVal { - findInvalidParamArrayReference(v, arrayParams, outofBoundParams) + taskrun.ExtractArrayIndexingParamReference(v, arrayParams, arrayIndexingParams) } 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) - } + taskrun.ExtractArrayIndexingParamReference(v, arrayParams, arrayIndexingParams) } } } diff --git a/pkg/reconciler/taskrun/validate_resources.go b/pkg/reconciler/taskrun/validate_resources.go index c4aac26e7a6..2b9415686c7 100644 --- a/pkg/reconciler/taskrun/validate_resources.go +++ b/pkg/reconciler/taskrun/validate_resources.go @@ -366,6 +366,7 @@ func missingKeysofObjectResults(tr *v1beta1.TaskRun, specResults []v1beta1.TaskR return findMissingKeys(neededKeys, providedKeys) } +// validateParamArrayIndex validate if the array indexing param reference target is existent func validateParamArrayIndex(ctx context.Context, params []v1beta1.Param, spec *v1beta1.TaskSpec) error { cfg := config.FromContextOrDefaults(ctx) if cfg.FeatureFlags.EnableAPIFields != config.AlphaAPIFields { @@ -377,7 +378,25 @@ func validateParamArrayIndex(ctx context.Context, params []v1beta1.Param, spec * defaults = append(defaults, spec.Params...) } // Collect all array params - arrayParams := make(map[string]int) + arrayParams := ExtractParamArrayLengths(defaults, params) + + // extract all array indexing references + arrayIndexingParams := []string{} + extractStepsParamArrayIndexing(spec.Steps, arrayParams, &arrayIndexingParams) + extractStepsTemplateParamArrayIndexing(spec.StepTemplate, arrayParams, &arrayIndexingParams) + extractVolumesParamArrayIndexing(spec.Volumes, arrayParams, &arrayIndexingParams) + for _, v := range spec.Workspaces { + ExtractArrayIndexingParamReference(v.MountPath, arrayParams, &arrayIndexingParams) + } + extractSidecarsParamArrayIndexing(spec.Sidecars, arrayParams, &arrayIndexingParams) + + return ValidateOutofBoundArrayParams(arrayIndexingParams, arrayParams) +} + +// ExtractParamArrayLengths extract and return the lengths of all array params +func ExtractParamArrayLengths(defaults []v1beta1.ParamSpec, params []v1beta1.Param) map[string]int { + // Collect all array params + arrayParamsLengths := make(map[string]int) patterns := []string{ "$(params.%s)", @@ -391,173 +410,172 @@ func validateParamArrayIndex(ctx context.Context, params []v1beta1.Param, spec * if p.Default.Type == v1beta1.ParamTypeArray { for _, pattern := range patterns { for i := 0; i < len(p.Default.ArrayVal); i++ { - arrayParams[fmt.Sprintf(pattern, p.Name)] = len(p.Default.ArrayVal) + arrayParamsLengths[fmt.Sprintf(pattern, p.Name)] = len(p.Default.ArrayVal) } } } } } - // Collect array params lengths from pipeline + // Collect array params lengths from params for _, p := range params { if p.Value.Type == v1beta1.ParamTypeArray { for _, pattern := range patterns { for i := 0; i < len(p.Value.ArrayVal); i++ { - arrayParams[fmt.Sprintf(pattern, p.Name)] = len(p.Value.ArrayVal) + arrayParamsLengths[fmt.Sprintf(pattern, p.Name)] = len(p.Value.ArrayVal) } } } } + return arrayParamsLengths +} +// ValidateOutofBoundArrayParams validates if the array indexing params are out of bound +func ValidateOutofBoundArrayParams(arrayIndexingParams []string, arrayParams map[string]int) error { outofBoundParams := sets.String{} - - // Validate array param in steps fields. - validateStepsParamArrayIndexing(spec.Steps, arrayParams, &outofBoundParams) - - // Validate array param in StepTemplate fields. - validateStepsTemplateParamArrayIndexing(spec.StepTemplate, arrayParams, &outofBoundParams) - - // Validate array param in build's volumes - validateVolumesParamArrayIndexing(spec.Volumes, arrayParams, &outofBoundParams) - - for _, v := range spec.Workspaces { - extractParamIndex(v.MountPath, arrayParams, &outofBoundParams) + for _, val := range arrayIndexingParams { + indexString := substitution.ExtractIndexString(val) + idx, _ := substitution.ExtractIndex(indexString) + v := substitution.TrimArrayIndex(val) + if paramLength, ok := arrayParams[v]; ok { + if idx >= paramLength { + outofBoundParams.Insert(val) + } + } } - - validateSidecarsParamArrayIndexing(spec.Sidecars, arrayParams, &outofBoundParams) - if outofBoundParams.Len() > 0 { return fmt.Errorf("non-existent param references:%v", outofBoundParams.List()) } - return nil } -func extractParamIndex(paramReference string, arrayParams map[string]int, outofBoundParams *sets.String) { +// ExtractArrayIndexingParamReference extract the array indexing references +func ExtractArrayIndexingParamReference(paramReference string, arrayParams map[string]int, arrayIndexingParams *[]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) - } + indexString := substitution.ExtractIndexString(val) + if indexString != "" { + *arrayIndexingParams = append(*arrayIndexingParams, val) } } } -func validateStepsParamArrayIndexing(steps []v1beta1.Step, arrayParams map[string]int, outofBoundParams *sets.String) { +// extractStepsParamArrayIndexing get all array indexing references from steps +func extractStepsParamArrayIndexing(steps []v1beta1.Step, arrayParams map[string]int, arrayIndexingParams *[]string) { for _, step := range steps { - extractParamIndex(step.Script, arrayParams, outofBoundParams) + ExtractArrayIndexingParamReference(step.Script, arrayParams, arrayIndexingParams) container := step.ToK8sContainer() - validateContainerParamArrayIndexing(container, arrayParams, outofBoundParams) + extractContainerParamArrayIndexing(container, arrayParams, arrayIndexingParams) } } -func validateStepsTemplateParamArrayIndexing(stepTemplate *v1beta1.StepTemplate, arrayParams map[string]int, outofBoundParams *sets.String) { +// extractStepsTemplateParamArrayIndexing get all array indexing references from StepsTemplate +func extractStepsTemplateParamArrayIndexing(stepTemplate *v1beta1.StepTemplate, arrayParams map[string]int, arrayIndexingParams *[]string) { if stepTemplate == nil { return } container := stepTemplate.ToK8sContainer() - validateContainerParamArrayIndexing(container, arrayParams, outofBoundParams) + extractContainerParamArrayIndexing(container, arrayParams, arrayIndexingParams) } -func validateSidecarsParamArrayIndexing(sidecars []v1beta1.Sidecar, arrayParams map[string]int, outofBoundParams *sets.String) { +// extractSidecarsParamArrayIndexing get all array indexing references from sidecars +func extractSidecarsParamArrayIndexing(sidecars []v1beta1.Sidecar, arrayParams map[string]int, arrayIndexingParams *[]string) { for _, s := range sidecars { - extractParamIndex(s.Script, arrayParams, outofBoundParams) + ExtractArrayIndexingParamReference(s.Script, arrayParams, arrayIndexingParams) container := s.ToK8sContainer() - validateContainerParamArrayIndexing(container, arrayParams, outofBoundParams) + extractContainerParamArrayIndexing(container, arrayParams, arrayIndexingParams) } } -func validateVolumesParamArrayIndexing(volumes []corev1.Volume, arrayParams map[string]int, outofBoundParams *sets.String) { +// extractVolumesParamArrayIndexing get all array indexing references from volumes +func extractVolumesParamArrayIndexing(volumes []corev1.Volume, arrayParams map[string]int, arrayIndexingParams *[]string) { for i, v := range volumes { - extractParamIndex(v.Name, arrayParams, outofBoundParams) + ExtractArrayIndexingParamReference(v.Name, arrayParams, arrayIndexingParams) if v.VolumeSource.ConfigMap != nil { - extractParamIndex(v.ConfigMap.Name, arrayParams, outofBoundParams) + ExtractArrayIndexingParamReference(v.ConfigMap.Name, arrayParams, arrayIndexingParams) for _, item := range v.ConfigMap.Items { - extractParamIndex(item.Key, arrayParams, outofBoundParams) - extractParamIndex(item.Path, arrayParams, outofBoundParams) + ExtractArrayIndexingParamReference(item.Key, arrayParams, arrayIndexingParams) + ExtractArrayIndexingParamReference(item.Path, arrayParams, arrayIndexingParams) } } if v.VolumeSource.Secret != nil { - extractParamIndex(v.Secret.SecretName, arrayParams, outofBoundParams) + ExtractArrayIndexingParamReference(v.Secret.SecretName, arrayParams, arrayIndexingParams) for _, item := range v.Secret.Items { - extractParamIndex(item.Key, arrayParams, outofBoundParams) - extractParamIndex(item.Path, arrayParams, outofBoundParams) + ExtractArrayIndexingParamReference(item.Key, arrayParams, arrayIndexingParams) + ExtractArrayIndexingParamReference(item.Path, arrayParams, arrayIndexingParams) } } if v.PersistentVolumeClaim != nil { - extractParamIndex(v.PersistentVolumeClaim.ClaimName, arrayParams, outofBoundParams) + ExtractArrayIndexingParamReference(v.PersistentVolumeClaim.ClaimName, arrayParams, arrayIndexingParams) } if v.Projected != nil { for _, s := range volumes[i].Projected.Sources { if s.ConfigMap != nil { - extractParamIndex(s.ConfigMap.Name, arrayParams, outofBoundParams) + ExtractArrayIndexingParamReference(s.ConfigMap.Name, arrayParams, arrayIndexingParams) } if s.Secret != nil { - extractParamIndex(s.Secret.Name, arrayParams, outofBoundParams) + ExtractArrayIndexingParamReference(s.Secret.Name, arrayParams, arrayIndexingParams) } if s.ServiceAccountToken != nil { - extractParamIndex(s.ServiceAccountToken.Audience, arrayParams, outofBoundParams) + ExtractArrayIndexingParamReference(s.ServiceAccountToken.Audience, arrayParams, arrayIndexingParams) } } } if v.CSI != nil { if v.CSI.NodePublishSecretRef != nil { - extractParamIndex(v.CSI.NodePublishSecretRef.Name, arrayParams, outofBoundParams) + ExtractArrayIndexingParamReference(v.CSI.NodePublishSecretRef.Name, arrayParams, arrayIndexingParams) } if v.CSI.VolumeAttributes != nil { for _, value := range v.CSI.VolumeAttributes { - extractParamIndex(value, arrayParams, outofBoundParams) + ExtractArrayIndexingParamReference(value, arrayParams, arrayIndexingParams) } } } } } -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) +// extractContainerParamArrayIndexing get all array indexing references from container +func extractContainerParamArrayIndexing(c *corev1.Container, arrayParams map[string]int, arrayIndexingParams *[]string) { + ExtractArrayIndexingParamReference(c.Name, arrayParams, arrayIndexingParams) + ExtractArrayIndexingParamReference(c.Image, arrayParams, arrayIndexingParams) + ExtractArrayIndexingParamReference(string(c.ImagePullPolicy), arrayParams, arrayIndexingParams) for _, a := range c.Args { - extractParamIndex(a, arrayParams, outofBoundParams) + ExtractArrayIndexingParamReference(a, arrayParams, arrayIndexingParams) } for ie, e := range c.Env { - extractParamIndex(e.Value, arrayParams, outofBoundParams) + ExtractArrayIndexingParamReference(e.Value, arrayParams, arrayIndexingParams) 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) + ExtractArrayIndexingParamReference(e.ValueFrom.SecretKeyRef.LocalObjectReference.Name, arrayParams, arrayIndexingParams) + ExtractArrayIndexingParamReference(e.ValueFrom.SecretKeyRef.Key, arrayParams, arrayIndexingParams) } if e.ValueFrom.ConfigMapKeyRef != nil { - extractParamIndex(e.ValueFrom.ConfigMapKeyRef.LocalObjectReference.Name, arrayParams, outofBoundParams) - extractParamIndex(e.ValueFrom.ConfigMapKeyRef.Key, arrayParams, outofBoundParams) + ExtractArrayIndexingParamReference(e.ValueFrom.ConfigMapKeyRef.LocalObjectReference.Name, arrayParams, arrayIndexingParams) + ExtractArrayIndexingParamReference(e.ValueFrom.ConfigMapKeyRef.Key, arrayParams, arrayIndexingParams) } } } for _, e := range c.EnvFrom { - extractParamIndex(e.Prefix, arrayParams, outofBoundParams) + ExtractArrayIndexingParamReference(e.Prefix, arrayParams, arrayIndexingParams) if e.ConfigMapRef != nil { - extractParamIndex(e.ConfigMapRef.LocalObjectReference.Name, arrayParams, outofBoundParams) + ExtractArrayIndexingParamReference(e.ConfigMapRef.LocalObjectReference.Name, arrayParams, arrayIndexingParams) } if e.SecretRef != nil { - extractParamIndex(e.SecretRef.LocalObjectReference.Name, arrayParams, outofBoundParams) + ExtractArrayIndexingParamReference(e.SecretRef.LocalObjectReference.Name, arrayParams, arrayIndexingParams) } } - extractParamIndex(c.WorkingDir, arrayParams, outofBoundParams) + ExtractArrayIndexingParamReference(c.WorkingDir, arrayParams, arrayIndexingParams) for _, cc := range c.Command { - extractParamIndex(cc, arrayParams, outofBoundParams) + ExtractArrayIndexingParamReference(cc, arrayParams, arrayIndexingParams) } for _, v := range c.VolumeMounts { - extractParamIndex(v.Name, arrayParams, outofBoundParams) - extractParamIndex(v.MountPath, arrayParams, outofBoundParams) - extractParamIndex(v.SubPath, arrayParams, outofBoundParams) + ExtractArrayIndexingParamReference(v.Name, arrayParams, arrayIndexingParams) + ExtractArrayIndexingParamReference(v.MountPath, arrayParams, arrayIndexingParams) + ExtractArrayIndexingParamReference(v.SubPath, arrayParams, arrayIndexingParams) } }