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. This commit also gathers
all the errors in `Pipeline Results` validation instead
of exiting on the first error. The reason is that if
multiple references exist to non-existant tasks, this
function will only return an error message about one of them,
forcing the person to fix the errors one by one.

Fixes bug [#4923](#4923)

/kind bug
/cc @jerop
  • Loading branch information
Varun Singhai committed Jul 12, 2022
1 parent ff69adb commit 0b068fb
Show file tree
Hide file tree
Showing 2 changed files with 154 additions and 19 deletions.
40 changes: 36 additions & 4 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 @@ -255,16 +255,17 @@ 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 {
return errs.Also(apis.ErrInvalidValue("expected pipeline results to be task result expressions but no expressions were found",
errs = errs.Also(apis.ErrInvalidValue("expected pipeline results to be task result expressions but no expressions were found",
"value").ViaFieldIndex("results", idx))
}

if !LooksLikeContainsResultRefs(expressions) {
return errs.Also(apis.ErrInvalidValue("expected pipeline results to be task result expressions but an invalid expressions was found",
errs = errs.Also(apis.ErrInvalidValue("expected pipeline results to be task result expressions but an invalid expressions was found",
"value").ViaFieldIndex("results", idx))
}

Expand All @@ -275,11 +276,42 @@ func validatePipelineResults(results []PipelineResult) (errs *apis.FieldError) {
"value").ViaFieldIndex("results", idx))
}

if !taskContainsResult(result.Value.StringVal, pipelineTaskNames) {
errs = 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
133 changes: 118 additions & 15 deletions pkg/apis/pipeline/v1beta1/pipeline_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1149,7 +1149,7 @@ func TestValidatePipelineResults_Success(t *testing.T) {
Description: "this is my pipeline result",
Value: *NewArrayOrString("$(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 @@ -1166,44 +1166,147 @@ func TestValidatePipelineResults_Failure(t *testing.T) {
Description: "this is my pipeline result",
Value: *NewArrayOrString("$(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{{
Name: "my-pipeline-result",
Description: "this is my pipeline result",
Value: *NewArrayOrString("foo.bar"),
}},
expectedError: apis.FieldError{
Message: `invalid value: expected pipeline results to be task result expressions but no expressions were found`,
Paths: []string{"results[0].value"},
},
expectedError: *apis.ErrInvalidValue(`expected pipeline results to be task result expressions but an invalid expressions was found`, "results[0].value").Also(
apis.ErrInvalidValue(`expected pipeline results to be task result expressions but no expressions were found`, "results[0].value")).Also(
apis.ErrInvalidValue(`referencing a nonexistent task`, "results[0].value")),
}, {
desc: "invalid pipeline result value with invalid expression",
results: []PipelineResult{{
Name: "my-pipeline-result",
Description: "this is my pipeline result",
Value: *NewArrayOrString("$(foo.bar)"),
}},
expectedError: apis.FieldError{
Message: `invalid value: expected pipeline results to be task result expressions but an invalid expressions was found`,
Paths: []string{"results[0].value"},
},
expectedError: *apis.ErrInvalidValue(`expected pipeline results to be task result expressions but an invalid expressions was found`, "results[0].value").Also(
apis.ErrInvalidValue("referencing a nonexistent task", "results[0].value")),
}}
for _, tt := range tests {
err := validatePipelineResults(tt.results)
err := validatePipelineResults(tt.results, []PipelineTask{{Name: "a-task"}})
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))
t.Errorf("Pipeline.validatePipelineResults() errors diff %s", diff.PrintWantGot(d))
}
}
}

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: *NewArrayOrString("$(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: *NewArrayOrString("$(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.finallyTaskResultsToPipelineResults() 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.finallyTaskResultsToPipelineResults() errors diff %s", diff.PrintWantGot(d))
}
})
}
}

func TestValidatePipelineParameterVariables_Success(t *testing.T) {
tests := []struct {
name string
Expand Down

0 comments on commit 0b068fb

Please sign in to comment.