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 29, 2022
1 parent 7d7d96f commit aa14f5c
Show file tree
Hide file tree
Showing 2 changed files with 146 additions and 8 deletions.
36 changes: 34 additions & 2 deletions pkg/apis/pipeline/v1beta1/pipeline_validation.go
View file
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) {
pipelineTaskNames := getPipelineTasksNames(tasks)
for idx, result := range results {
expressions, ok := GetVarSubstitutionExpressionsForPipelineResult(result)
if !ok {
Expand All @@ -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.<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")
Expand Down
118 changes: 112 additions & 6 deletions pkg/apis/pipeline/v1beta1/pipeline_validation_test.go
View file
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 All @@ -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{{
Expand All @@ -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)
}
Expand All @@ -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
Expand Down

0 comments on commit aa14f5c

Please sign in to comment.