Skip to content

Commit

Permalink
Validation for Finally Task Results not being referenced in Pipeline …
Browse files Browse the repository at this point in the history
…Results

Prior to this commit, there was no validation for Finally
Task Results being referenced to Pipeline Results.

This commit creates validation for that case by making
sure all the Finally Task Results reference a `task_name`
within the Pipeline Results.

Fixes bug [#4923](#4923)

/kind bug
/cc @jerop
  • Loading branch information
Varun Singhai committed Jun 21, 2022
1 parent 3147117 commit ac6d11f
Show file tree
Hide file tree
Showing 3 changed files with 95 additions and 6 deletions.
31 changes: 28 additions & 3 deletions pkg/apis/pipeline/v1beta1/pipeline_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ func (ps *PipelineSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
errs = errs.Also(validatePipelineWorkspacesUsage(ps.Workspaces, ps.Tasks).ViaField("tasks"))
errs = errs.Also(validatePipelineWorkspacesUsage(ps.Workspaces, ps.Finally).ViaField("finally"))
// Validate the pipeline's results
errs = errs.Also(validatePipelineResults(ps.Results))
errs = errs.Also(validatePipelineResults(ps.Results, ps.Tasks))
errs = errs.Also(validateTasksAndFinallySection(ps))
errs = errs.Also(validateFinalTasks(ps.Tasks, ps.Finally))
errs = errs.Also(validateWhenExpressions(ps.Tasks, ps.Finally))
Expand Down Expand Up @@ -249,7 +249,8 @@ func filter(arr []string, cond func(string) bool) []string {
}

// validatePipelineResults ensure that pipeline result variables are properly configured
func validatePipelineResults(results []PipelineResult) (errs *apis.FieldError) {
func validatePipelineResults(results []PipelineResult, tasks []PipelineTask) (errs *apis.FieldError) {
taskSet := convertTaskToTaskSet(tasks)
for idx, result := range results {
expressions, ok := GetVarSubstitutionExpressionsForPipelineResult(result)
if !ok {
Expand All @@ -265,15 +266,39 @@ func validatePipelineResults(results []PipelineResult) (errs *apis.FieldError) {
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),
return errs.Also(apis.ErrInvalidValue(fmt.Sprintf("expected all of the expressions %v to be result expressions but only %v were", expressions, resultRefs),
"value").ViaFieldIndex("results", idx))
}

if !TaskContainsResult(result.Value, taskSet) {
return errs.Also(apis.ErrInvalidValue("referencing a nonexistent task",
"value").ViaFieldIndex("results", idx))
}
}

return errs
}

// put task names in a set
func convertTaskToTaskSet(tasks []PipelineTask) sets.String {
taskSet := make(sets.String)
for _, val := range tasks {
taskSet.Insert(val.Name)
}

return taskSet
}

// TaskContainsResult ensures the result value is referenced within the
// task names
func TaskContainsResult(value string, taskSet sets.String) bool {
value = value[2 : len(value)-1]
task_name, _, _, _, _ := parseExpression(value)

ok := taskSet.Has(task_name)
return ok
}

func validateTasksAndFinallySection(ps *PipelineSpec) *apis.FieldError {
if len(ps.Finally) != 0 && len(ps.Tasks) == 0 {
return apis.ErrInvalidValue(fmt.Sprintf("spec.tasks is empty but spec.finally has %d tasks", len(ps.Finally)), "finally")
Expand Down
68 changes: 66 additions & 2 deletions pkg/apis/pipeline/v1beta1/pipeline_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1107,7 +1107,7 @@ func TestValidatePipelineResults_Success(t *testing.T) {
Description: "this is my pipeline result",
Value: "$(tasks.a-task.results.gitrepo.commit)",
}}
if err := validatePipelineResults(results); err != nil {
if err := validatePipelineResults(results, []PipelineTask{{Name: "a-task"}}); err != nil {
t.Errorf("Pipeline.validatePipelineResults() returned error for valid pipeline: %s: %v", desc, err)
}
}
Expand Down Expand Up @@ -1152,7 +1152,7 @@ func TestValidatePipelineResults_Failure(t *testing.T) {
},
}}
for _, tt := range tests {
err := validatePipelineResults(tt.results)
err := validatePipelineResults(tt.results, []PipelineTask{})
if err == nil {
t.Errorf("Pipeline.validatePipelineResults() did not return for invalid pipeline: %s", tt.desc)
}
Expand All @@ -1162,6 +1162,70 @@ func TestValidatePipelineResults_Failure(t *testing.T) {
}
}

func TestFinallyTaskResultsToPipelineResults_Failure(t *testing.T) {
tests := []struct {
desc string
p *Pipeline
expectedError apis.FieldError
wc func(context.Context) context.Context
}{{
desc: "invalid propagation of finally task results from pipeline results",
p: &Pipeline{
ObjectMeta: metav1.ObjectMeta{Name: "pipeline"},
Spec: PipelineSpec{
Results: []PipelineResult{{
Name: "initialized",
Value: "$(tasks.check-git-commit.results.init)",
}},
Tasks: []PipelineTask{{
Name: "clone-app-repo",
TaskSpec: &EmbeddedTask{TaskSpec: TaskSpec{
Results: []TaskResult{{
Name: "current-date-unix-timestamp",
Type: "string",
}},
Steps: []Step{{
Name: "foo", Image: "bar",
}},
}},
}},
Finally: []PipelineTask{{
Name: "check-git-commit",
TaskSpec: &EmbeddedTask{TaskSpec: TaskSpec{
Results: []TaskResult{{
Name: "init",
Type: "string",
}},
Steps: []Step{{
Name: "foo2", Image: "bar",
}},
}},
}},
},
},
expectedError: apis.FieldError{
Message: `invalid value: referencing a nonexistent task`,
Paths: []string{"spec.results[0].value"},
},
}}

for _, tt := range tests {
t.Run(tt.desc, func(t *testing.T) {
ctx := context.Background()
if tt.wc != nil {
ctx = tt.wc(ctx)
}
err := tt.p.Validate(ctx)
if err == nil {
t.Errorf("Pipeline.validatePipelineResults() did not return for invalid pipeline: %s", tt.desc)
}
if d := cmp.Diff(tt.expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" {
t.Errorf("Pipeline.validateParamResults() errors diff %s", diff.PrintWantGot(d))
}
})
}
}

func TestValidatePipelineParameterVariables_Success(t *testing.T) {
tests := []struct {
name string
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/pipelinerun/pipelinerun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6659,7 +6659,7 @@ spec:
t.Errorf("expected invalid task reference condition but saw: %v", cond1)
}

if cond2.Reason != ReasonInvalidTaskResultReference {
if cond2.Reason != ReasonFailedValidation {
t.Errorf("expected invalid task reference condition but saw: %v", cond2)
}

Expand Down

0 comments on commit ac6d11f

Please sign in to comment.