Validation for Finally Task Results not being referenced in Pipeline …

…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

pkg/apis/pipeline/v1beta1/pipeline_validation.go

@@ -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))
@@ -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) {
+	pipelineTaskNames := getPipelineTasksNames(tasks)
 	for idx, result := range results {
 		expressions, ok := GetVarSubstitutionExpressionsForPipelineResult(result)
 		if !ok {
@@ -265,15 +266,46 @@ 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, pipelineTaskNames) {
+			return errs.Also(apis.ErrInvalidValue("referencing a nonexistent task",
+				"value").ViaFieldIndex("results", idx))
+		}
 	}
 
 	return errs
 }
 
+// put task names in a set
+func getPipelineTasksNames(pipelineTasks []PipelineTask) sets.String {
+	pipelineTaskNames := make(sets.String)
+	for _, pipelineTask := range pipelineTasks {
+		pipelineTaskNames.Insert(pipelineTask.Name)
+	}
+
+	return pipelineTaskNames
+}
+
+// taskContainsResult ensures the result value is referenced within the
+// task names
+func taskContainsResult(resultExpression string, pipelineTaskNames sets.String) bool {
+	// split incase of multiple resultExpressions in the same result.Value string
+	// i.e "$(task.<task-name).result.<result-name>) - $(task2.<task2-name).result2.<result2-name>)"
+	split := strings.Split(resultExpression, "$")
+	for _, expression := range split {
+		if expression != "" {
+			pipelineTaskName, _, _, _, _ := parseExpression(stripVarSubExpression("$" + expression))
+			if !pipelineTaskNames.Has(pipelineTaskName) {
+				return false
+			}
+		}
+	}
+	return true
+}
+
 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")

pkg/apis/pipeline/v1beta1/pipeline_validation_test.go

@@ -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)
 	}
 }
@@ -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)
 		}
@@ -1162,6 +1162,114 @@ func TestValidatePipelineResults_Failure(t *testing.T) {
 	}
 }
 
+func TestFinallyTaskResultsToPipelineResults_Success(t *testing.T) {
+	tests := []struct {
+		name string
+		p    *Pipeline
+		wc   func(context.Context) context.Context
+	}{{
+		name: "valid pipeline with pipeline results",
+		p: &Pipeline{
+			ObjectMeta: metav1.ObjectMeta{Name: "pipeline"},
+			Spec: PipelineSpec{
+				Results: []PipelineResult{{
+					Name:  "initialized",
+					Value: "$(tasks.clone-app-repo.results.initialized)",
+				}},
+				Tasks: []PipelineTask{{
+					Name: "clone-app-repo",
+					TaskSpec: &EmbeddedTask{TaskSpec: TaskSpec{
+						Results: []TaskResult{{
+							Name: "initialized",
+							Type: "string",
+						}},
+						Steps: []Step{{
+							Name: "foo", Image: "bar",
+						}},
+					}},
+				}},
+			},
+		}},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, 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.finallyTaskResultsToPipelineResults() returned error for valid Pipeline: %v", err)
+			}
+		})
+	}
+}
+
+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