diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation.go b/pkg/apis/pipeline/v1beta1/pipeline_validation.go index c96a86c5cd5..e2808f64182 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation.go +++ b/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 { @@ -269,11 +270,42 @@ func validatePipelineResults(results []PipelineResult) (errs *apis.FieldError) { "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.) - $(task2.)" + 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") diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go index 583cb3f742d..d3743031648 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go +++ b/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) } } @@ -1124,10 +1124,8 @@ func TestValidatePipelineResults_Failure(t *testing.T) { Description: "this is my pipeline result", Value: "$(tasks.a-task.results.output.key1.extra)", }}, - expectedError: apis.FieldError{ - 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"}, - }, + expectedError: *apis.ErrInvalidValue(`expected all of the expressions [tasks.a-task.results.output.key1.extra] to be result expressions but only [] were`, "results[0].value").Also( + apis.ErrInvalidValue("referencing a nonexistent task", "results[0].value")), }, { desc: "invalid pipeline result value with static string", results: []PipelineResult{{ @@ -1152,7 +1150,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 +1160,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