From 222b662696f967001163074e88af931fb0ffa569 Mon Sep 17 00:00:00 2001 From: Yongxuan Zhang Date: Mon, 30 May 2022 21:43:03 +0000 Subject: [PATCH] add index validation --- pkg/apis/pipeline/v1beta1/resultref.go | 25 ++++-- pkg/apis/pipeline/v1beta1/resultref_test.go | 21 +++++ .../pipelinerun/resources/apply_test.go | 33 +++++++- .../resources/pipelinerunresolution.go | 6 +- .../resources/resultrefresolution.go | 16 +++- .../resources/resultrefresolution_test.go | 76 +++++++++++++++++++ 6 files changed, 164 insertions(+), 13 deletions(-) diff --git a/pkg/apis/pipeline/v1beta1/resultref.go b/pkg/apis/pipeline/v1beta1/resultref.go index 0a60a573b5f..13c9ab71695 100644 --- a/pkg/apis/pipeline/v1beta1/resultref.go +++ b/pkg/apis/pipeline/v1beta1/resultref.go @@ -19,6 +19,7 @@ package v1beta1 import ( "fmt" "regexp" + "strconv" "strings" ) @@ -26,6 +27,7 @@ import ( type ResultRef struct { PipelineTask string `json:"pipelineTask"` Result string `json:"result"` + ResultsIndex int `json:"resultsIndex"` } const ( @@ -37,15 +39,15 @@ const ( // TODO(#2462) use one regex across all substitutions // variableSubstitutionFormat matches format like $result.resultname, $result.resultname[int] and $result.resultname[*] variableSubstitutionFormat = `\$\([_a-zA-Z0-9.-]+(\.[_a-zA-Z0-9.-]+)*(\[([0-9])*\*?\])?\)` - // excludeArrayIndexing will replace all `[int]` and `[*]` for parseExpression to extract result name - excludeArrayIndexing = `\[([0-9])*\*?\]` + // arrayIndexing will match all `[int]` and `[*]` for parseExpression + arrayIndexing = `\[([0-9])*\*?\]` // ResultNameFormat Constant used to define the the regex Result.Name should follow ResultNameFormat = `^([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]$` ) var variableSubstitutionRegex = regexp.MustCompile(variableSubstitutionFormat) var resultNameFormatRegex = regexp.MustCompile(ResultNameFormat) -var excludeArrayIndexingRegex = regexp.MustCompile(excludeArrayIndexing) +var arrayIndexingRegex = regexp.MustCompile(arrayIndexing) // NewResultRefs extracts all ResultReferences from a param or a pipeline result. // If the ResultReference can be extracted, they are returned. Expressions which are not @@ -53,7 +55,7 @@ var excludeArrayIndexingRegex = regexp.MustCompile(excludeArrayIndexing) func NewResultRefs(expressions []string) []*ResultRef { var resultRefs []*ResultRef for _, expression := range expressions { - pipelineTask, result, err := parseExpression(expression) + pipelineTask, result, index, err := parseExpression(expression) // If the expression isn't a result but is some other expression, // parseExpression will return an error, in which case we just skip that expression, // since although it's not a result ref, it might be some other kind of reference @@ -61,6 +63,7 @@ func NewResultRefs(expressions []string) []*ResultRef { resultRefs = append(resultRefs, &ResultRef{ PipelineTask: pipelineTask, Result: result, + ResultsIndex: index, }) } } @@ -126,13 +129,19 @@ func stripVarSubExpression(expression string) string { return strings.TrimSuffix(strings.TrimPrefix(expression, "$("), ")") } -func parseExpression(substitutionExpression string) (string, string, error) { +func parseExpression(substitutionExpression string) (string, string, int, error) { subExpressions := strings.Split(substitutionExpression, ".") if len(subExpressions) != 4 || subExpressions[0] != ResultTaskPart || subExpressions[2] != ResultResultPart { - return "", "", fmt.Errorf("Must be of the form %q", resultExpressionFormat) + return "", "", 0, fmt.Errorf("Must be of the form %q", resultExpressionFormat) } - subExpressions[3] = excludeArrayIndexingRegex.ReplaceAllString(subExpressions[3], "") - return subExpressions[1], subExpressions[3], nil + + stringIdx := strings.TrimSuffix(strings.TrimPrefix(arrayIndexingRegex.FindString(subExpressions[3]), "["), "]") + subExpressions[3] = arrayIndexingRegex.ReplaceAllString(subExpressions[3], "") + if stringIdx != "" { + intIdx, _ := strconv.Atoi(stringIdx) + return subExpressions[1], subExpressions[3], intIdx, nil + } + return subExpressions[1], subExpressions[3], 0, nil } // PipelineTaskResultRefs walks all the places a result reference can be used diff --git a/pkg/apis/pipeline/v1beta1/resultref_test.go b/pkg/apis/pipeline/v1beta1/resultref_test.go index 37015ad5be6..2cffe1cdf22 100644 --- a/pkg/apis/pipeline/v1beta1/resultref_test.go +++ b/pkg/apis/pipeline/v1beta1/resultref_test.go @@ -41,6 +41,27 @@ func TestNewResultReference(t *testing.T) { PipelineTask: "sumTask", Result: "sumResult", }}, + },{ + name: "refer whole array result", + param: v1beta1.Param{ + Name: "param", + Value: *v1beta1.NewArrayOrString("$(tasks.sumTask.results.sumResult[*])"), + }, + want: []*v1beta1.ResultRef{{ + PipelineTask: "sumTask", + Result: "sumResult", + }}, + },{ + name: "refer array indexing result", + param: v1beta1.Param{ + Name: "param", + Value: *v1beta1.NewArrayOrString("$(tasks.sumTask.results.sumResult[1])"), + }, + want: []*v1beta1.ResultRef{{ + PipelineTask: "sumTask", + Result: "sumResult", + ResultsIndex: 1, + }}, }, { name: "substitution within string", param: v1beta1.Param{ diff --git a/pkg/reconciler/pipelinerun/resources/apply_test.go b/pkg/reconciler/pipelinerun/resources/apply_test.go index 6bb5987cb09..0b3aad9af1b 100644 --- a/pkg/reconciler/pipelinerun/resources/apply_test.go +++ b/pkg/reconciler/pipelinerun/resources/apply_test.go @@ -431,7 +431,38 @@ func TestApplyTaskResults_MinimalExpression(t *testing.T) { }}, }, }}, - }, { + }, { + name: "Test array indexing result substitution out of bound - params", + resolvedResultRefs: ResolvedResultRefs{{ + Value: *v1beta1.NewArrayOrString("arrayResultValueOne", "arrayResultValueTwo"), + ResultReference: v1beta1.ResultRef{ + PipelineTask: "aTask", + Result: "a.Result", + }, + FromTaskRun: "aTaskRun", + }}, + targets: PipelineRunState{{ + PipelineTask: &v1beta1.PipelineTask{ + Name: "bTask", + TaskRef: &v1beta1.TaskRef{Name: "bTask"}, + Params: []v1beta1.Param{{ + Name: "bParam", + Value: *v1beta1.NewArrayOrString(`$(tasks.aTask.results["a.Result"][3])`), + }}, + }, + }}, + want: PipelineRunState{{ + PipelineTask: &v1beta1.PipelineTask{ + Name: "bTask", + TaskRef: &v1beta1.TaskRef{Name: "bTask"}, + Params: []v1beta1.Param{{ + Name: "bParam", + // index validation is done in ResolveResultRefs() before ApplyTaskResults() + Value: *v1beta1.NewArrayOrString(`$(tasks.aTask.results["a.Result"][3])`), + }}, + }, + }}, + },{ name: "Test result substitution on minimal variable substitution expression - when expressions", resolvedResultRefs: ResolvedResultRefs{{ Value: *v1beta1.NewArrayOrString("aResultValue"), diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go index 617af19bb7b..6bca11983e7 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go @@ -306,8 +306,10 @@ func (t *ResolvedPipelineRunTask) skipBecauseResultReferencesAreMissing(facts *P if t.checkParentsDone(facts) && t.hasResultReferences() { resolvedResultRefs, pt, err := ResolveResultRefs(facts.State, PipelineRunState{t}) rprt := facts.State.ToMap()[pt] - if err != nil && (t.IsFinalTask(facts) || rprt.Skip(facts).SkippingReason == v1beta1.WhenExpressionsSkip) { - return true + if rprt != nil { + if err != nil && (t.IsFinalTask(facts) || rprt.Skip(facts).SkippingReason == v1beta1.WhenExpressionsSkip) { + return true + } } ApplyTaskResults(PipelineRunState{t}, resolvedResultRefs) facts.ResetSkippedCache() diff --git a/pkg/reconciler/pipelinerun/resources/resultrefresolution.go b/pkg/reconciler/pipelinerun/resources/resultrefresolution.go index f0a85ab3aae..bbb1b366bf5 100644 --- a/pkg/reconciler/pipelinerun/resources/resultrefresolution.go +++ b/pkg/reconciler/pipelinerun/resources/resultrefresolution.go @@ -43,7 +43,7 @@ func ResolveResultRef(pipelineRunState PipelineRunState, target *ResolvedPipelin if err != nil { return nil, pt, err } - return removeDup(resolvedResultRefs), "", nil + return validateArrayResultsIndex(removeDup(resolvedResultRefs)) } // ResolveResultRefs resolves any ResultReference that are found in the target ResolvedPipelineRunTask @@ -56,7 +56,19 @@ func ResolveResultRefs(pipelineRunState PipelineRunState, targets PipelineRunSta } allResolvedResultRefs = append(allResolvedResultRefs, resolvedResultRefs...) } - return removeDup(allResolvedResultRefs), "", nil + return validateArrayResultsIndex(removeDup(allResolvedResultRefs)) +} + +// validateArrayResultsIndex checks if the result array indexing reference is out of bound of the array size +func validateArrayResultsIndex(allResolvedResultRefs ResolvedResultRefs) (ResolvedResultRefs, string, error) { + for _, r := range allResolvedResultRefs { + if r.Value.Type == v1beta1.ParamTypeArray { + if r.ResultReference.ResultsIndex >= len(r.Value.ArrayVal) { + return nil, "", fmt.Errorf("Array Result Index %d for Task %s Result %s is out of bound of size %d", r.ResultReference.ResultsIndex, r.ResultReference.PipelineTask, r.ResultReference.Result, len(r.Value.ArrayVal)) + } + } + } + return allResolvedResultRefs, "", nil } // extractResultRefs resolves any ResultReference that are found in param or pipeline result diff --git a/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go b/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go index 327585cfd7f..1f856dff203 100644 --- a/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go +++ b/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go @@ -120,6 +120,58 @@ var pipelineRunState = PipelineRunState{{ Value: *v1beta1.NewArrayOrString("$(tasks.aCustomPipelineTask.results.aResult)"), }}, }, +}, { + TaskRunName: "cTaskRun", + TaskRun: &v1beta1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cTaskRun", + }, + Status: v1beta1.TaskRunStatus{ + Status: duckv1beta1.Status{ + Conditions: duckv1beta1.Conditions{successCondition}, + }, + TaskRunStatusFields: v1beta1.TaskRunStatusFields{ + TaskRunResults: []v1beta1.TaskRunResult{{ + Name: "cResult", + Value: *v1beta1.NewArrayOrString("arrayResultOne", "arrayResultTwo"), + }}, + }, + }, + }, + PipelineTask: &v1beta1.PipelineTask{ + Name: "cTask", + TaskRef: &v1beta1.TaskRef{Name: "cTask"}, + Params: []v1beta1.Param{{ + Name: "cParam", + Value: *v1beta1.NewArrayOrString("$(tasks.cTask.results.cResult[1])"), + }}, + }, +}, { + TaskRunName: "dTaskRun", + TaskRun: &v1beta1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dTaskRun", + }, + Status: v1beta1.TaskRunStatus{ + Status: duckv1beta1.Status{ + Conditions: duckv1beta1.Conditions{successCondition}, + }, + TaskRunStatusFields: v1beta1.TaskRunStatusFields{ + TaskRunResults: []v1beta1.TaskRunResult{{ + Name: "dResult", + Value: *v1beta1.NewArrayOrString("arrayResultOne", "arrayResultTwo"), + }}, + }, + }, + }, + PipelineTask: &v1beta1.PipelineTask{ + Name: "dTask", + TaskRef: &v1beta1.TaskRef{Name: "dTask"}, + Params: []v1beta1.Param{{ + Name: "dParam", + Value: *v1beta1.NewArrayOrString("$(tasks.dTask.results.dResult[3])"), + }}, + }, }} func TestTaskParamResolver_ResolveResultRefs(t *testing.T) { @@ -479,6 +531,30 @@ func TestResolveResultRefs(t *testing.T) { FromTaskRun: "aTaskRun", }}, wantErr: false, + }, { + name: "Test successful array result references resolution - params", + pipelineRunState: pipelineRunState, + targets: PipelineRunState{ + pipelineRunState[7], + }, + want: ResolvedResultRefs{{ + Value: *v1beta1.NewArrayOrString("arrayResultOne", "arrayResultTwo"), + ResultReference: v1beta1.ResultRef{ + PipelineTask: "cTask", + Result: "cResult", + ResultsIndex: 1, + }, + FromTaskRun: "cTaskRun", + }}, + wantErr: false, + }, { + name: "Test unsuccessful result references resolution - params", + pipelineRunState: pipelineRunState, + targets: PipelineRunState{ + pipelineRunState[8], + }, + want: nil, + wantErr: true, }, { name: "Test successful result references resolution - when expressions", pipelineRunState: pipelineRunState,