Skip to content

Commit

Permalink
add feature gate check for param array indexing
Browse files Browse the repository at this point in the history 
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 [email protected]
  • Loading branch information
@Yongxuanzhang
Yongxuanzhang committed Feb 8, 2023
1 parent 1e45bed commit 6b384cb
Show file tree
Hide file tree
Showing 5 changed files with 279 additions and 99 deletions.
2 changes: 1 addition & 1 deletion pkg/reconciler/pipelinerun/pipelinerun.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get
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",
"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)
}
Expand Down
77 changes: 45 additions & 32 deletions pkg/reconciler/pipelinerun/resources/validate_params.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -91,40 +89,59 @@ 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)
err := findInvalidParamArrayReferences(ctx, p.Tasks[i].Params, arrayParams, &outofBoundParams)
if err != nil {
return err
}
if p.Tasks[i].IsMatrixed() {
findInvalidParamArrayReferences(p.Tasks[i].Matrix.Params, arrayParams, &outofBoundParams)
err := findInvalidParamArrayReferences(ctx, p.Tasks[i].Matrix.Params, arrayParams, &outofBoundParams)
if err != nil {
return err
}
}
for j := range p.Tasks[i].Workspaces {
findInvalidParamArrayReference(p.Tasks[i].Workspaces[j].SubPath, arrayParams, &outofBoundParams)
err := taskrun.FindInvalidParamArrayReference(ctx, p.Tasks[i].Workspaces[j].SubPath, arrayParams, &outofBoundParams)
if err != nil {
return err
}
}
for _, wes := range p.Tasks[i].WhenExpressions {
findInvalidParamArrayReference(wes.Input, arrayParams, &outofBoundParams)
err := taskrun.FindInvalidParamArrayReference(ctx, wes.Input, arrayParams, &outofBoundParams)
if err != nil {
return err
}
for _, v := range wes.Values {
findInvalidParamArrayReference(v, arrayParams, &outofBoundParams)
err := taskrun.FindInvalidParamArrayReference(ctx, v, arrayParams, &outofBoundParams)
if err != nil {
return err
}
}
}
}

for i := range p.Finally {
findInvalidParamArrayReferences(p.Finally[i].Params, arrayParams, &outofBoundParams)
err := findInvalidParamArrayReferences(ctx, p.Finally[i].Params, arrayParams, &outofBoundParams)
if err != nil {
return err
}
if p.Finally[i].IsMatrixed() {
findInvalidParamArrayReferences(p.Finally[i].Matrix.Params, arrayParams, &outofBoundParams)
err := findInvalidParamArrayReferences(ctx, p.Finally[i].Matrix.Params, arrayParams, &outofBoundParams)
if err != nil {
return err
}
}
for _, wes := range p.Finally[i].WhenExpressions {
for _, v := range wes.Values {
findInvalidParamArrayReference(v, arrayParams, &outofBoundParams)
err := taskrun.FindInvalidParamArrayReference(ctx, v, arrayParams, &outofBoundParams)
if err != nil {
return err
}
}
}
}
Expand Down Expand Up @@ -171,28 +188,24 @@ func extractParamIndexes(defaults []v1beta1.ParamSpec, params []v1beta1.Param) m
return arrayParams
}

func findInvalidParamArrayReferences(params []v1beta1.Param, arrayParams map[string]int, outofBoundParams *sets.String) {
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)
err := taskrun.FindInvalidParamArrayReference(ctx, params[i].Value.StringVal, arrayParams, outofBoundParams)
if err != nil {
return err
}
for _, v := range params[i].Value.ArrayVal {
findInvalidParamArrayReference(v, arrayParams, outofBoundParams)
err := taskrun.FindInvalidParamArrayReference(ctx, v, arrayParams, outofBoundParams)
if 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)
err := taskrun.FindInvalidParamArrayReference(ctx, v, arrayParams, outofBoundParams)
if err != nil {
return err
}
}
}
return nil
}
42 changes: 33 additions & 9 deletions pkg/reconciler/pipelinerun/resources/validate_params_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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,
Expand Down
Loading

0 comments on commit 6b384cb

Please sign in to comment.