Skip to content

Commit

Permalink
[TEP-0075] Validate object task result variable
Browse files Browse the repository at this point in the history 
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.<taskName>.results.<resultName>` 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.<taskName>.results.<objectResultName>.<individualAttribute>`.

In this commit, we consider the object case in the validation webhook.
  • Loading branch information
@chuangw6
chuangw6 committed Jun 9, 2022
1 parent 4621a66 commit 0facd8d
Show file tree
Hide file tree
Showing 5 changed files with 126 additions and 16 deletions.
9 changes: 8 additions & 1 deletion pkg/apis/pipeline/v1beta1/openapi_generated.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 6 additions & 2 deletions pkg/apis/pipeline/v1beta1/pipeline_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1102,6 +1102,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)
Expand All @@ -1118,10 +1122,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"},
},
}, {
Expand Down
53 changes: 43 additions & 10 deletions pkg/apis/pipeline/v1beta1/resultref.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,15 @@ type ResultRef struct {
PipelineTask string `json:"pipelineTask"`
Result string `json:"result"`
ResultsIndex int `json:"resultsIndex"`
Property string `json:"property"`
}

const (
resultExpressionFormat = "tasks.<taskName>.results.<resultName>"
// Result expressions of the form <resultName>.<attribute> will be treated as object results.
// If a string result name contains a dot, brackets should be used to differentiate it from an object result.
// https://github.com/tektoncd/community/blob/main/teps/0075-object-param-and-result-types.md#collisions-with-builtin-variable-replacement
objectResultExpressionFormat = "tasks.<taskName>.results.<objectResultName>.<individualAttribute>"
// 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
Expand All @@ -55,7 +60,7 @@ var arrayIndexingRegex = regexp.MustCompile(arrayIndexing)
func NewResultRefs(expressions []string) []*ResultRef {
var resultRefs []*ResultRef
for _, expression := range expressions {
pipelineTask, result, index, err := parseExpression(expression)
pipelineTask, result, index, 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
Expand All @@ -64,6 +69,7 @@ func NewResultRefs(expressions []string) []*ResultRef {
PipelineTask: pipelineTask,
Result: result,
ResultsIndex: index,
Property: property,
})
}
}
Expand Down Expand Up @@ -101,6 +107,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
}
Expand Down Expand Up @@ -129,19 +140,41 @@ func stripVarSubExpression(expression string) string {
return strings.TrimSuffix(strings.TrimPrefix(expression, "$("), ")")
}

func parseExpression(substitutionExpression string) (string, string, int, error) {
// parseExpression parses "task name", "result name", "array index" (iff it's an array result) and "object key name" (iff it's an object result)
// Valid Example 1:
// - Input: tasks.myTask.results.aStringResult
// - Output: "myTask", "aStringResult", -1, "", nil
// Valid Example 2:
// - Input: tasks.myTask.results.anObjectResult.key1
// - Output: "myTask", "anObjectResult", 0, "key1", nil
// Valid Example 3:
// - Input: tasks.myTask.results.anArrayResult[1]
// - Output: "myTask", "anArrayResult", 1, "", nil
// Invalid Example 1:
// - Input: tasks.myTask.results.resultName.foo.bar
// - Output: "", "", 0, "", error
// TODO: may use regex for each type to handle possible reference formats
func parseExpression(substitutionExpression string) (string, string, int, string, error) {
subExpressions := strings.Split(substitutionExpression, ".")
if len(subExpressions) != 4 || subExpressions[0] != ResultTaskPart || subExpressions[2] != ResultResultPart {
return "", "", 0, fmt.Errorf("Must be of the form %q", resultExpressionFormat)

// For string result: tasks.<taskName>.results.<stringResultName>
// For array result: tasks.<taskName>.results.<arrayResultName>[index]
if len(subExpressions) == 4 && subExpressions[0] == ResultTaskPart && subExpressions[2] == ResultResultPart {
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
}

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
// For object type result: tasks.<taskName>.results.<objectResultName>.<individualAttribute>
if len(subExpressions) == 5 && subExpressions[0] == ResultTaskPart && subExpressions[2] == ResultResultPart {
return subExpressions[1], subExpressions[3], 0, subExpressions[4], nil
}
return subExpressions[1], subExpressions[3], 0, nil

return "", "", 0, "", 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
Expand Down
65 changes: 63 additions & 2 deletions pkg/apis/pipeline/v1beta1/resultref_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,17 @@ 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: "refer array indexing result",
param: v1beta1.Param{
Expand All @@ -62,6 +73,21 @@ func TestNewResultReference(t *testing.T) {
Result: "sumResult",
ResultsIndex: 1,
}},
}, {
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{
Expand All @@ -76,27 +102,35 @@ 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",
Result: "sumResult",
}, {
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",
Result: "sumResult",
}, {
PipelineTask: "sumTask2",
Result: "sumResult",
}, {
PipelineTask: "sumTask3",
Result: "sumResult",
Property: "key1",
}},
}, {
name: "first separator typo",
Expand All @@ -112,6 +146,13 @@ func TestNewResultReference(t *testing.T) {
Value: *v1beta1.NewArrayOrString("$(tasks.sumTasks.result.sumResult)"),
},
want: nil,
}, {
name: "more than 5 dot-separated components",
param: v1beta1.Param{
Name: "param",
Value: *v1beta1.NewArrayOrString("$(tasks.sumTasks.result.sumResult.key.extra)"),
},
want: nil,
}, {
name: "param substitution shouldn't be considered result ref",
param: v1beta1.Param{
Expand Down Expand Up @@ -209,6 +250,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)
Expand Down
7 changes: 6 additions & 1 deletion pkg/apis/pipeline/v1beta1/swagger.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1525,13 +1525,18 @@
"required": [
"pipelineTask",
"result",
"resultsIndex"
"resultsIndex",
"property"
],
"properties": {
"pipelineTask": {
"type": "string",
"default": ""
},
"property": {
"type": "string",
"default": ""
},
"result": {
"type": "string",
"default": ""
Expand Down

0 comments on commit 0facd8d

Please sign in to comment.