Skip to content

Commit

Permalink
harden look like a result references check
Browse files Browse the repository at this point in the history 
looksLikeResultRef checks for a prefix task and if a string contains .result.
Any expression satisfying these two conditions are considered as a result
reference, for example, all of the following expressions are considered as a
result reference:

tasks.a-task.results.bResult (valid reference)
tasks.a-task.resultTypo.bResult (invalid reference)
task.result (invalid reference)
tasks.a-task.b-task.c-task.results.result.result (invalid reference)
etc.

But, this way of checking for a result reference is weak.
looksLikeResultRef returns true for some invalid expressions but not others, for example:

tasks.a-task.typoresults.bResult (invalid reference)
task.typoresult (invalid reference)
etc

A pipeline with such expressions tasks.a-task.resultsTypo.bResult results in
validation failure with a vague error "invalid value: expected all of the expressions
[tasks.a-task.resultTypo.bResult] to be result expressions but only [] were:
tasks[0].params[a-param].value" v/s a pipeline with an another invalid expression
tasks.a-task.typoresults.bResult does not recognize this expression as an invalid
result reference but considers it as a string constant. This PR drops the vague validation error for such expressions.

Also, the validation function validateParamResults is also relying on the same
weak looksLikeResultRef which is redundant with validateGraph.

The validation function validateGraph which is called prior to validateParamResults
is using NewResultRef to discover the dependencies and make sure a valid DAG
can be constructed. The way we have implemented reporting validation errors with
errs.Also(, the pipeline validation calls the same function NewResultRef with
the same set of expressions. This redundancy impacts the amount of time spent
in validation. This PR deletes these redundant checks.
  • Loading branch information
@pritidesai @tekton-robot
pritidesai authored and tekton-robot committed Sep 16, 2022
1 parent 34274fb commit 9221ac3
Show file tree
Hide file tree
Showing 12 changed files with 150 additions and 292 deletions.
27 changes: 3 additions & 24 deletions pkg/apis/pipeline/v1/pipeline_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ func (ps *PipelineSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
errs = errs.Also(ValidatePipelineTasks(ctx, ps.Tasks, ps.Finally))
// Validate the pipeline task graph
errs = errs.Also(validateGraph(ps.Tasks))
errs = errs.Also(validateParamResults(ps.Tasks))
// The parameter variables should be valid
errs = errs.Also(ValidatePipelineParameterVariables(ctx, ps.Tasks, ps.Params).ViaField("tasks"))
errs = errs.Also(ValidatePipelineParameterVariables(ctx, ps.Finally, ps.Params).ViaField("finally"))
Expand Down Expand Up @@ -232,26 +231,6 @@ func validatePipelineContextVariablesInParamValues(paramValues []string, prefix
return errs
}

// validateParamResults ensures that task result variables are properly configured
func validateParamResults(tasks []PipelineTask) (errs *apis.FieldError) {
for idx, task := range tasks {
for _, param := range task.Params {
expressions, ok := GetVarSubstitutionExpressionsForParam(param)
if ok {
if LooksLikeContainsResultRefs(expressions) {
expressions = filter(expressions, looksLikeResultRef)
resultRefs := NewResultRefs(expressions)
if len(expressions) != len(resultRefs) {
errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("expected all of the expressions %v to be result expressions but only %v were", expressions, resultRefs),
"value").ViaFieldKey("params", param.Name).ViaFieldIndex("tasks", idx))
}
}
}
}
}
return errs
}

func filter(arr []string, cond func(string) bool) []string {
result := []string{}
for i := range arr {
Expand Down Expand Up @@ -389,11 +368,11 @@ func validateWhenExpressions(tasks []PipelineTask, finalTasks []PipelineTask) (e

// validateGraph ensures the Pipeline's dependency Graph (DAG) make sense: that there is no dependency
// cycle or that they rely on values from Tasks that ran previously.
func validateGraph(tasks []PipelineTask) *apis.FieldError {
func validateGraph(tasks []PipelineTask) (errs *apis.FieldError) {
if _, err := dag.Build(PipelineTaskList(tasks), PipelineTaskList(tasks).Deps()); err != nil {
return apis.ErrInvalidValue(err.Error(), "tasks")
errs = errs.Also(apis.ErrInvalidValue(err.Error(), "tasks"))
}
return nil
return errs
}

func validateMatrix(ctx context.Context, tasks []PipelineTask) (errs *apis.FieldError) {
Expand Down
111 changes: 49 additions & 62 deletions pkg/apis/pipeline/v1/pipeline_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -415,28 +415,60 @@ func TestPipelineSpec_Validate_Failure(t *testing.T) {
Paths: []string{"tasks[0].when[0]", "finally[0].when[0]"},
},
}, {
name: "invalid pipeline with one pipeline task having when expression with misconfigured result reference",
name: "invalid pipeline with a pipelineTask having when expression with invalid result reference - empty referenced task",
ps: &PipelineSpec{
Description: "this is an invalid pipeline with invalid pipeline task",
Tasks: []PipelineTask{{
Name: "invalid-pipeline-task",
TaskRef: &TaskRef{Name: "foo-task"},
When: []WhenExpression{{
Input: "$(tasks..results.bResult)",
Operator: selection.In,
Values: []string{"bar"},
}},
}},
},
expectedError: *apis.ErrGeneric(`invalid value: couldn't add link between invalid-pipeline-task and : task invalid-pipeline-task depends on but wasn't present in Pipeline`, "tasks"),
}, {
name: "invalid pipeline with a pipelineTask having when expression with invalid result reference - referenced task does not exist in the pipeline",
ps: &PipelineSpec{
Description: "this is an invalid pipeline with invalid pipeline task",
Tasks: []PipelineTask{{

Name: "invalid-pipeline-task",
TaskRef: &TaskRef{Name: "foo-task"},
When: []WhenExpression{{
Input: "$(tasks.a-task.results.bResult)",
Operator: selection.In,
Values: []string{"bar"},
}},
}},
},
expectedError: *apis.ErrGeneric(`invalid value: couldn't add link between invalid-pipeline-task and a-task: task invalid-pipeline-task depends on a-task but a-task wasn't present in Pipeline`, "tasks"),
}, {
name: "invalid pipeline with a pipelineTask having when expression with invalid result reference - referenced task does not exist in the pipeline",
ps: &PipelineSpec{
Description: "this is an invalid pipeline with invalid pipeline task",
Params: []ParamSpec{{Name: "prefix", Type: ParamTypeString}},
Tasks: []PipelineTask{{
Name: "valid-pipeline-task",
TaskRef: &TaskRef{Name: "foo-task"},
}, {
Name: "invalid-pipeline-task",
TaskRef: &TaskRef{Name: "foo-task"},
Params: []Param{{
Name: "prefix", Value: ParamValue{Type: ParamTypeString, StringVal: "bar"},
}},
When: []WhenExpression{{
Input: "$(tasks.a-task.resultTypo.bResult)",
Input: "$(params.prefix):$(tasks.a-task.results.bResult)",
Operator: selection.In,
Values: []string{"bar"},
}},
}},
},
expectedError: apis.FieldError{
Message: `invalid value: expected all of the expressions [tasks.a-task.resultTypo.bResult] to be result expressions but only [] were`,
Paths: []string{"tasks[1].when[0]"},
},
expectedError: *apis.ErrGeneric(`invalid value: couldn't add link between invalid-pipeline-task and a-task: task invalid-pipeline-task depends on a-task but a-task wasn't present in Pipeline`, "tasks"),
}, {
name: "invalid pipeline with final task having when expression with misconfigured result reference",
name: "invalid pipeline with final task having when expression with invalid result reference - referenced task does not exist in the pipeline",
ps: &PipelineSpec{
Description: "this is an invalid pipeline with invalid pipeline task",
Tasks: []PipelineTask{{
Expand All @@ -450,18 +482,18 @@ func TestPipelineSpec_Validate_Failure(t *testing.T) {
Name: "invalid-pipeline-task-finally",
TaskRef: &TaskRef{Name: "foo-task"},
When: []WhenExpression{{
Input: "$(tasks.a-task.resultTypo.bResult)",
Input: "$(tasks.a-task.results.bResult)",
Operator: selection.In,
Values: []string{"bar"},
}},
}},
},
expectedError: apis.FieldError{
Message: `invalid value: expected all of the expressions [tasks.a-task.resultTypo.bResult] to be result expressions but only [] were`,
Message: `invalid value: invalid task result reference, final task has task result reference from a task a-task which is not defined in the pipeline`,
Paths: []string{"finally[0].when[0]"},
},
}, {
name: "invalid pipeline with dag task and final task having when expression with misconfigured result reference",
name: "invalid pipeline with dag task and final task having when expression with invalid result reference - referenced task does not exist in the pipeline",
ps: &PipelineSpec{
Description: "this is an invalid pipeline with invalid pipeline task",
Tasks: []PipelineTask{{
Expand All @@ -471,7 +503,7 @@ func TestPipelineSpec_Validate_Failure(t *testing.T) {
Name: "invalid-pipeline-task",
TaskRef: &TaskRef{Name: "foo-task"},
When: []WhenExpression{{
Input: "$(tasks.a-task.resultTypo.bResult)",
Input: "$(tasks.a-task.results.bResult)",
Operator: selection.In,
Values: []string{"bar"},
}},
Expand All @@ -480,16 +512,17 @@ func TestPipelineSpec_Validate_Failure(t *testing.T) {
Name: "invalid-pipeline-task-finally",
TaskRef: &TaskRef{Name: "foo-task"},
When: []WhenExpression{{
Input: "$(tasks.a-task.resultTypo.bResult)",
Input: "$(tasks.a-task.results.bResult)",
Operator: selection.In,
Values: []string{"bar"},
}},
}},
},
expectedError: apis.FieldError{
Message: `invalid value: expected all of the expressions [tasks.a-task.resultTypo.bResult] to be result expressions but only [] were`,
Paths: []string{"tasks[1].when[0]", "finally[0].when[0]"},
},
expectedError: *apis.ErrGeneric(`invalid value: couldn't add link between invalid-pipeline-task and a-task: task invalid-pipeline-task depends on a-task but a-task wasn't present in Pipeline`, "tasks").Also(
&apis.FieldError{
Message: `invalid value: invalid task result reference, final task has task result reference from a task a-task which is not defined in the pipeline`,
Paths: []string{"finally[0].when[0]"},
}),
}, {
name: "invalid pipeline with one pipeline task having blank when expression",
ps: &PipelineSpec{
Expand Down Expand Up @@ -693,52 +726,6 @@ func TestValidateGraph_Failure(t *testing.T) {
}
}

func TestValidateParamResults_Success(t *testing.T) {
desc := "valid pipeline task referencing task result along with parameter variable"
tasks := []PipelineTask{{
TaskSpec: &EmbeddedTask{TaskSpec: TaskSpec{
Results: []TaskResult{{
Name: "output",
}},
Steps: []Step{{
Name: "foo", Image: "bar",
}},
}},
Name: "a-task",
}, {
Name: "foo",
TaskRef: &TaskRef{Name: "foo-task"},
Params: []Param{{
Name: "a-param", Value: ParamValue{Type: ParamTypeString, StringVal: "$(params.foo) and $(tasks.a-task.results.output)"},
}},
}}
if err := validateParamResults(tasks); err != nil {
t.Errorf("Pipeline.validateParamResults() returned error for valid pipeline: %s: %v", desc, err)
}
}

func TestValidateParamResults_Failure(t *testing.T) {
desc := "invalid pipeline task referencing task results with malformed variable substitution expression"
tasks := []PipelineTask{{
Name: "a-task", TaskRef: &TaskRef{Name: "a-task"},
}, {
Name: "b-task", TaskRef: &TaskRef{Name: "b-task"},
Params: []Param{{
Name: "a-param", Value: ParamValue{Type: ParamTypeString, StringVal: "$(tasks.a-task.resultTypo.bResult)"}}},
}}
expectedError := apis.FieldError{
Message: `invalid value: expected all of the expressions [tasks.a-task.resultTypo.bResult] to be result expressions but only [] were`,
Paths: []string{"tasks[1].params[a-param].value"},
}
err := validateParamResults(tasks)
if err == nil {
t.Errorf("Pipeline.validateParamResults() did not return error for invalid pipeline: %s", desc)
}
if d := cmp.Diff(expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" {
t.Errorf("Pipeline.validateParamResults() errors diff %s", diff.PrintWantGot(d))
}
}

func TestValidatePipelineResults_Success(t *testing.T) {
desc := "valid pipeline with valid pipeline results syntax"
results := []PipelineResult{{
Expand Down
35 changes: 18 additions & 17 deletions pkg/apis/pipeline/v1/resultref.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ const (
objectResultExpressionFormat = "tasks.<taskName>.results.<objectResultName>.<individualAttribute>"
// ResultTaskPart Constant used to define the "tasks" part of a pipeline result reference
ResultTaskPart = "tasks"
// ResultFinallyPart Constant used to define the "finally" part of a task result reference
ResultFinallyPart = "finally"
// ResultResultPart Constant used to define the "results" part of a pipeline result reference
ResultResultPart = "results"
// TODO(#2462) use one regex across all substitutions
Expand Down Expand Up @@ -92,7 +94,8 @@ func LooksLikeContainsResultRefs(expressions []string) bool {
// looksLikeResultRef attempts to check if the given string looks like it contains any
// result references. Returns true if it does, false otherwise
func looksLikeResultRef(expression string) bool {
return strings.HasPrefix(expression, "task") && strings.Contains(expression, ".result")
subExpressions := strings.Split(expression, ".")
return len(subExpressions) >= 4 && (subExpressions[0] == ResultTaskPart || subExpressions[0] == ResultFinallyPart) && subExpressions[2] == ResultResultPart
}

// GetVarSubstitutionExpressionsForParam extracts all the value between "$(" and ")"" for a parameter
Expand Down Expand Up @@ -161,24 +164,22 @@ func stripVarSubExpression(expression string) string {
// - 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, ".")

// 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 {
resultName, stringIdx := ParseResultName(subExpressions[3])
if stringIdx != "" {
intIdx, _ := strconv.Atoi(stringIdx)
return subExpressions[1], resultName, intIdx, "", nil
if looksLikeResultRef(substitutionExpression) {
subExpressions := strings.Split(substitutionExpression, ".")
// For string result: tasks.<taskName>.results.<stringResultName>
// For array result: tasks.<taskName>.results.<arrayResultName>[index]
if len(subExpressions) == 4 {
resultName, stringIdx := ParseResultName(subExpressions[3])
if stringIdx != "" {
intIdx, _ := strconv.Atoi(stringIdx)
return subExpressions[1], resultName, intIdx, "", nil
}
return subExpressions[1], resultName, 0, "", nil
} else if len(subExpressions) == 5 {
// For object type result: tasks.<taskName>.results.<objectResultName>.<individualAttribute>
return subExpressions[1], subExpressions[3], 0, subExpressions[4], nil
}
return subExpressions[1], resultName, 0, "", 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 "", "", 0, "", fmt.Errorf("Must be one of the form 1). %q; 2). %q", resultExpressionFormat, objectResultExpressionFormat)
}

Expand Down
22 changes: 4 additions & 18 deletions pkg/apis/pipeline/v1/resultref_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -305,19 +305,19 @@ func TestLooksLikeResultRef(t *testing.T) {
},
want: true,
}, {
name: "test expression: looks like result ref, but typo in 'task' separator",
name: "test expression: invalid result ref, typo in 'task' separator",
param: v1.Param{
Name: "param",
Value: *v1.NewStructuredValues("$(task.sumTasks.results.sumResult)"),
},
want: true,
want: false,
}, {
name: "test expression: looks like result ref, but typo in 'results' separator",
name: "test expression: invalid result ref, typo in 'results' separator",
param: v1.Param{
Name: "param",
Value: *v1.NewStructuredValues("$(tasks.sumTasks.result.sumResult)"),
},
want: true,
want: false,
}, {
name: "test expression: missing 'task' separator",
param: v1.Param{
Expand Down Expand Up @@ -558,20 +558,6 @@ func TestLooksLikeResultRefWhenExpressionTrue(t *testing.T) {
Operator: selection.In,
Values: []string{"foo"},
},
}, {
name: "test expression: looks like result ref, but typo in 'task' separator",
we: v1.WhenExpression{
Input: "$(task.sumTasks.results.sumResult)",
Operator: selection.In,
Values: []string{"foo"},
},
}, {
name: "test expression: looks like result ref, but typo in 'results' separator",
we: v1.WhenExpression{
Input: "$(tasks.sumTasks.result.sumResult)",
Operator: selection.In,
Values: []string{"foo"},
},
}, {
name: "test expression: one good ref, one bad one should return true",
we: v1.WhenExpression{
Expand Down
20 changes: 1 addition & 19 deletions pkg/apis/pipeline/v1/when_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,7 @@ var validWhenOperators = []string{
}

func (wes WhenExpressions) validate() *apis.FieldError {
errs := wes.validateWhenExpressionsFields().ViaField("when")
return errs.Also(wes.validateTaskResultsVariables().ViaField("when"))
return wes.validateWhenExpressionsFields().ViaField("when")
}

func (wes WhenExpressions) validateWhenExpressionsFields() (errs *apis.FieldError) {
Expand All @@ -57,23 +56,6 @@ func (we *WhenExpression) validateWhenExpressionFields() *apis.FieldError {
return nil
}

func (wes WhenExpressions) validateTaskResultsVariables() *apis.FieldError {
for idx, we := range wes {
expressions, ok := we.GetVarSubstitutionExpressions()
if ok {
if LooksLikeContainsResultRefs(expressions) {
expressions = filter(expressions, looksLikeResultRef)
resultRefs := NewResultRefs(expressions)
if len(expressions) != len(resultRefs) {
message := fmt.Sprintf("expected all of the expressions %v to be result expressions but only %v were", expressions, resultRefs)
return apis.ErrInvalidValue(message, apis.CurrentField).ViaIndex(idx)
}
}
}
}
return nil
}

func (wes WhenExpressions) validatePipelineParametersVariables(prefix string, paramNames sets.String, arrayParamNames sets.String, objectParamNameKeys map[string][]string) (errs *apis.FieldError) {
for idx, we := range wes {
errs = errs.Also(validateStringVariable(we.Input, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaField("input").ViaFieldIndex("when", idx))
Expand Down
7 changes: 0 additions & 7 deletions pkg/apis/pipeline/v1/when_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,13 +91,6 @@ func TestWhenExpressions_Invalid(t *testing.T) {
Input: "foo",
Operator: selection.NotIn,
}},
}, {
name: "invalid variable",
wes: []WhenExpression{{
Input: "$(tasks.a-task.resultsTypo.output)",
Operator: selection.In,
Values: []string{"bar"},
}},
}, {
name: "missing when expression",
wes: []WhenExpression{{}},
Expand Down
Loading

0 comments on commit 9221ac3

Please sign in to comment.