From 032f28b3e306a2769d3a076ed4e05ba01881a49f Mon Sep 17 00:00:00 2001 From: Chuang Wang Date: Mon, 16 May 2022 10:41:12 -0700 Subject: [PATCH] [TEP-0075] Validate object task result variable Prior to this commit, when providing param value with task result variable, it only allowed using the task result as whole in the format of `tasks..results.` since result can be only of type string previously. As we are adding support for object type result, we need to support using the result variable of object type in the format of `tasks..results..`. In this commit, we consider the object case in the validation webhook. --- .../pipeline/v1beta1/openapi_generated.go | 9 ++- .../v1beta1/pipeline_validation_test.go | 8 ++- pkg/apis/pipeline/v1beta1/resultref.go | 30 ++++++++-- pkg/apis/pipeline/v1beta1/resultref_test.go | 58 ++++++++++++++++++- pkg/apis/pipeline/v1beta1/swagger.json | 7 ++- 5 files changed, 101 insertions(+), 11 deletions(-) diff --git a/pkg/apis/pipeline/v1beta1/openapi_generated.go b/pkg/apis/pipeline/v1beta1/openapi_generated.go index 146dd506b98..926e280f0d1 100644 --- a/pkg/apis/pipeline/v1beta1/openapi_generated.go +++ b/pkg/apis/pipeline/v1beta1/openapi_generated.go @@ -3036,8 +3036,15 @@ func schema_pkg_apis_pipeline_v1beta1_ResultRef(ref common.ReferenceCallback) co Format: "", }, }, + "property": { + SchemaProps: spec.SchemaProps{ + Default: "", + Type: []string{"string"}, + Format: "", + }, + }, }, - Required: []string{"pipelineTask", "result"}, + Required: []string{"pipelineTask", "result", "property"}, }, }, } diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go index 6a6e27bccac..500bfe699fe 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go @@ -1197,6 +1197,10 @@ func TestValidatePipelineResults_Success(t *testing.T) { Name: "my-pipeline-result", Description: "this is my pipeline result", Value: "$(tasks.a-task.results.output)", + }, { + Name: "my-pipeline-object-result", + Description: "this is my pipeline result", + Value: "$(tasks.a-task.results.gitrepo.commit)", }} if err := validatePipelineResults(results); err != nil { t.Errorf("Pipeline.validatePipelineResults() returned error for valid pipeline: %s: %v", desc, err) @@ -1208,10 +1212,10 @@ func TestValidatePipelineResults_Failure(t *testing.T) { results := []PipelineResult{{ Name: "my-pipeline-result", Description: "this is my pipeline result", - Value: "$(tasks.a-task.results.output.output)", + Value: "$(tasks.a-task.results.output.key1.extra)", }} expectedError := apis.FieldError{ - Message: `invalid value: expected all of the expressions [tasks.a-task.results.output.output] to be result expressions but only [] were`, + Message: `invalid value: expected all of the expressions [tasks.a-task.results.output.key1.extra] to be result expressions but only [] were`, Paths: []string{"results[0].value"}, } err := validatePipelineResults(results) diff --git a/pkg/apis/pipeline/v1beta1/resultref.go b/pkg/apis/pipeline/v1beta1/resultref.go index d74e2bcd38f..676908ff384 100644 --- a/pkg/apis/pipeline/v1beta1/resultref.go +++ b/pkg/apis/pipeline/v1beta1/resultref.go @@ -26,10 +26,15 @@ import ( type ResultRef struct { PipelineTask string `json:"pipelineTask"` Result string `json:"result"` + Property string `json:"property"` } const ( resultExpressionFormat = "tasks..results." + // To avoid conflicts with standalone names that include dots in it, + // TEP-0080 added support for using bracket notation [] for such names instead of dot notation. + // see more details in TEP-0080 and TEP-0075 regarding this. + objectResultExpressionFormat = "tasks..results.." // ResultTaskPart Constant used to define the "tasks" part of a pipeline result reference ResultTaskPart = "tasks" // ResultResultPart Constant used to define the "results" part of a pipeline result reference @@ -49,7 +54,7 @@ var resultNameFormatRegex = regexp.MustCompile(ResultNameFormat) func NewResultRefs(expressions []string) []*ResultRef { var resultRefs []*ResultRef for _, expression := range expressions { - pipelineTask, result, err := parseExpression(expression) + pipelineTask, result, property, 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 @@ -57,6 +62,7 @@ func NewResultRefs(expressions []string) []*ResultRef { resultRefs = append(resultRefs, &ResultRef{ PipelineTask: pipelineTask, Result: result, + Property: property, }) } } @@ -94,6 +100,11 @@ func GetVarSubstitutionExpressionsForParam(param Param) ([]string, bool) { case ParamTypeString: // string type allExpressions = append(allExpressions, validateString(param.Value.StringVal)...) + case ParamTypeObject: + // object type + for _, value := range param.Value.ObjectVal { + allExpressions = append(allExpressions, validateString(value)...) + } default: return nil, false } @@ -122,12 +133,21 @@ func stripVarSubExpression(expression string) string { return strings.TrimSuffix(strings.TrimPrefix(expression, "$("), ")") } -func parseExpression(substitutionExpression string) (string, string, error) { +func parseExpression(substitutionExpression string) (string, string, string, 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) + + // TODO (@yongxuanzhang): consider array type result here tasks..results.[*] and indexing tasks..results.[0] + // For string type result: tasks..results. + if len(subExpressions) == 4 && subExpressions[0] == ResultTaskPart && subExpressions[2] == ResultResultPart { + return subExpressions[1], subExpressions[3], "", nil } - return subExpressions[1], subExpressions[3], nil + + // For object type result: tasks..results.. + if len(subExpressions) == 5 && subExpressions[0] == ResultTaskPart && subExpressions[2] == ResultResultPart { + return subExpressions[1], subExpressions[3], subExpressions[4], nil + } + + return "", "", "", fmt.Errorf("Must be one of the form 1). %q; 2). %q", resultExpressionFormat, objectResultExpressionFormat) } // 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..34255515350 100644 --- a/pkg/apis/pipeline/v1beta1/resultref_test.go +++ b/pkg/apis/pipeline/v1beta1/resultref_test.go @@ -41,6 +41,32 @@ func TestNewResultReference(t *testing.T) { PipelineTask: "sumTask", Result: "sumResult", }}, + }, { + name: "Test valid expression with single object result property", + param: v1beta1.Param{ + Name: "param", + Value: *v1beta1.NewArrayOrString("$(tasks.sumTask.results.sumResult.key1)"), + }, + want: []*v1beta1.ResultRef{{ + PipelineTask: "sumTask", + Result: "sumResult", + Property: "key1", + }}, + }, { + name: "Test valid expression with multiple object result properties", + param: v1beta1.Param{ + Name: "param", + Value: *v1beta1.NewArrayOrString("$(tasks.sumTask.results.imageresult.digest), and another one $(tasks.sumTask.results.imageresult.tag)"), + }, + want: []*v1beta1.ResultRef{{ + PipelineTask: "sumTask", + Result: "imageresult", + Property: "digest", + }, { + PipelineTask: "sumTask", + Result: "imageresult", + Property: "tag", + }}, }, { name: "substitution within string", param: v1beta1.Param{ @@ -55,7 +81,7 @@ func TestNewResultReference(t *testing.T) { name: "multiple substitution", param: v1beta1.Param{ Name: "param", - Value: *v1beta1.NewArrayOrString("$(tasks.sumTask1.results.sumResult) and another $(tasks.sumTask2.results.sumResult)"), + Value: *v1beta1.NewArrayOrString("$(tasks.sumTask1.results.sumResult) and another $(tasks.sumTask2.results.sumResult), last one $(tasks.sumTask3.results.sumResult.key1)"), }, want: []*v1beta1.ResultRef{{ PipelineTask: "sumTask1", @@ -63,12 +89,16 @@ func TestNewResultReference(t *testing.T) { }, { PipelineTask: "sumTask2", Result: "sumResult", + }, { + PipelineTask: "sumTask3", + Result: "sumResult", + Property: "key1", }}, }, { name: "multiple substitution with param", param: v1beta1.Param{ Name: "param", - Value: *v1beta1.NewArrayOrString("$(params.param) $(tasks.sumTask1.results.sumResult) and another $(tasks.sumTask2.results.sumResult)"), + Value: *v1beta1.NewArrayOrString("$(params.param) $(tasks.sumTask1.results.sumResult) and another $(tasks.sumTask2.results.sumResult), last one $(tasks.sumTask3.results.sumResult.key1)"), }, want: []*v1beta1.ResultRef{{ PipelineTask: "sumTask1", @@ -76,6 +106,10 @@ func TestNewResultReference(t *testing.T) { }, { PipelineTask: "sumTask2", Result: "sumResult", + }, { + PipelineTask: "sumTask3", + Result: "sumResult", + Property: "key1", }}, }, { name: "first separator typo", @@ -188,6 +222,26 @@ func TestHasResultReference(t *testing.T) { PipelineTask: "sumTask2", Result: "sumResult2", }}, + }, { + name: "Test valid expression in object", + param: v1beta1.Param{ + Name: "param", + Value: *v1beta1.NewObject(map[string]string{ + "key1": "$(tasks.sumTask1.results.sumResult1)", + "key2": "$(tasks.sumTask2.results.sumResult2) and another one $(tasks.sumTask3.results.sumResult3)", + "key3": "no ref here", + }), + }, + wantRef: []*v1beta1.ResultRef{{ + PipelineTask: "sumTask1", + Result: "sumResult1", + }, { + PipelineTask: "sumTask2", + Result: "sumResult2", + }, { + PipelineTask: "sumTask3", + Result: "sumResult3", + }}, }} { t.Run(tt.name, func(t *testing.T) { expressions, ok := v1beta1.GetVarSubstitutionExpressionsForParam(tt.param) diff --git a/pkg/apis/pipeline/v1beta1/swagger.json b/pkg/apis/pipeline/v1beta1/swagger.json index c1e8ea1f3e7..e8ee8a3153e 100644 --- a/pkg/apis/pipeline/v1beta1/swagger.json +++ b/pkg/apis/pipeline/v1beta1/swagger.json @@ -1711,13 +1711,18 @@ "type": "object", "required": [ "pipelineTask", - "result" + "result", + "property" ], "properties": { "pipelineTask": { "type": "string", "default": "" }, + "property": { + "type": "string", + "default": "" + }, "result": { "type": "string", "default": ""