diff --git a/pkg/apis/pipeline/v1/pipeline_validation.go b/pkg/apis/pipeline/v1/pipeline_validation.go index 216d424cf7e..96e8498fd30 100644 --- a/pkg/apis/pipeline/v1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1/pipeline_validation.go @@ -64,7 +64,7 @@ func (ps *PipelineSpec) Validate(ctx context.Context) (errs *apis.FieldError) { errs = errs.Also(validatePipelineWorkspacesUsage(ctx, ps.Workspaces, ps.Tasks).ViaField("tasks")) errs = errs.Also(validatePipelineWorkspacesUsage(ctx, ps.Workspaces, ps.Finally).ViaField("finally")) // Validate the pipeline's results - errs = errs.Also(validatePipelineResults(ps.Results, ps.Tasks)) + errs = errs.Also(validatePipelineResults(ps.Results, ps.Tasks, ps.Finally)) errs = errs.Also(validateTasksAndFinallySection(ps)) errs = errs.Also(validateFinalTasks(ps.Tasks, ps.Finally)) errs = errs.Also(validateWhenExpressions(ps.Tasks, ps.Finally)) @@ -242,8 +242,9 @@ func filter(arr []string, cond func(string) bool) []string { } // validatePipelineResults ensure that pipeline result variables are properly configured -func validatePipelineResults(results []PipelineResult, tasks []PipelineTask) (errs *apis.FieldError) { +func validatePipelineResults(results []PipelineResult, tasks []PipelineTask, finally []PipelineTask) (errs *apis.FieldError) { pipelineTaskNames := getPipelineTasksNames(tasks) + pipelineFinallyTaskNames := getPipelineTasksNames(finally) for idx, result := range results { expressions, ok := GetVarSubstitutionExpressionsForPipelineResult(result) if !ok { @@ -263,7 +264,7 @@ func validatePipelineResults(results []PipelineResult, tasks []PipelineTask) (er "value").ViaFieldIndex("results", idx)) } - if !taskContainsResult(result.Value.StringVal, pipelineTaskNames) { + if !taskContainsResult(result.Value.StringVal, pipelineTaskNames, pipelineFinallyTaskNames) { errs = errs.Also(apis.ErrInvalidValue("referencing a nonexistent task", "value").ViaFieldIndex("results", idx)) } @@ -284,16 +285,26 @@ func getPipelineTasksNames(pipelineTasks []PipelineTask) sets.String { // taskContainsResult ensures the result value is referenced within the // task names -func taskContainsResult(resultExpression string, pipelineTaskNames sets.String) bool { +func taskContainsResult(resultExpression string, pipelineTaskNames sets.String, pipelineFinallyTaskNames 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) { + value := stripVarSubExpression("$" + expression) + pipelineTaskName, _, _, _, err := parseExpression(value) + + if err != nil { + return false + } + + if strings.HasPrefix(value, "tasks") && !pipelineTaskNames.Has(pipelineTaskName) { return false } + if strings.HasPrefix(value, "finally") && !pipelineFinallyTaskNames.Has(pipelineTaskName) { + return false + } + } } return true diff --git a/pkg/apis/pipeline/v1/pipeline_validation_test.go b/pkg/apis/pipeline/v1/pipeline_validation_test.go index 3f7d57921f0..d871ce05b49 100644 --- a/pkg/apis/pipeline/v1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1/pipeline_validation_test.go @@ -18,6 +18,7 @@ package v1 import ( "context" + "fmt" "testing" "github.com/google/go-cmp/cmp" @@ -54,6 +55,24 @@ func TestPipeline_Validate_Success(t *testing.T) { }, }, wc: enableFeatures(t, []string{"enable-custom-tasks"}), + }, { + name: "pipelinetask custom task spec", + p: &Pipeline{ + ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, + Spec: PipelineSpec{ + Tasks: []PipelineTask{{Name: "foo", + TaskSpec: &EmbeddedTask{ + TypeMeta: runtime.TypeMeta{ + APIVersion: "example.dev/v0", + Kind: "Example", + }, + Spec: runtime.RawExtension{ + Raw: []byte(`{"field1":123,"field2":"value"}`), + }}, + }}, + }, + }, + wc: enableFeatures(t, []string{"enable-custom-tasks"}), }, { name: "do not validate spec on delete", p: &Pipeline{ @@ -434,7 +453,9 @@ func TestPipelineSpec_Validate_Failure(t *testing.T) { ps: &PipelineSpec{ Description: "this is an invalid pipeline with invalid pipeline task", Tasks: []PipelineTask{{ - + Name: "valid-pipeline-task", + TaskRef: &TaskRef{Name: "foo-task"}, + }, { Name: "invalid-pipeline-task", TaskRef: &TaskRef{Name: "foo-task"}, When: []WhenExpression{{ @@ -737,7 +758,7 @@ func TestValidatePipelineResults_Success(t *testing.T) { Description: "this is my pipeline result", Value: *NewStructuredValues("$(tasks.a-task.results.gitrepo.commit)"), }} - if err := validatePipelineResults(results, []PipelineTask{{Name: "a-task"}}); err != nil { + if err := validatePipelineResults(results, []PipelineTask{{Name: "a-task"}}, []PipelineTask{}); err != nil { t.Errorf("Pipeline.validatePipelineResults() returned error for valid pipeline: %s: %v", desc, err) } } @@ -748,7 +769,7 @@ func TestValidatePipelineResults_Failure(t *testing.T) { results []PipelineResult expectedError apis.FieldError }{{ - desc: "invalid pipeline result reference", + desc: "invalid pipeline task result reference", results: []PipelineResult{{ Name: "my-pipeline-result", Description: "this is my pipeline result", @@ -756,6 +777,15 @@ func TestValidatePipelineResults_Failure(t *testing.T) { }}, 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 finally result reference variable", + results: []PipelineResult{{ + Name: "my-pipeline-result", + Description: "this is my pipeline result", + Value: *NewStructuredValues("$(finally.a-task.results.output.key1.extra)"), + }}, + expectedError: *apis.ErrInvalidValue(`expected all of the expressions [finally.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{{ @@ -777,7 +807,7 @@ func TestValidatePipelineResults_Failure(t *testing.T) { apis.ErrInvalidValue("referencing a nonexistent task", "results[0].value")), }} for _, tt := range tests { - err := validatePipelineResults(tt.results, []PipelineTask{{Name: "a-task"}}) + err := validatePipelineResults(tt.results, []PipelineTask{{Name: "a-task"}}, []PipelineTask{}) if err == nil { t.Errorf("Pipeline.validatePipelineResults() did not return for invalid pipeline: %s", tt.desc) } @@ -814,7 +844,42 @@ func TestFinallyTaskResultsToPipelineResults_Success(t *testing.T) { }}, }}, }, - }}, + }}, { + name: "referencing existent finally task result", + p: &Pipeline{ + ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, + Spec: PipelineSpec{ + Results: []PipelineResult{{ + Name: "initialized", + Value: *NewStructuredValues("$(finally.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", + }}, + }}, + }}, + }, + }, + }, } for _, tt := range tests { @@ -876,6 +941,45 @@ func TestFinallyTaskResultsToPipelineResults_Failure(t *testing.T) { Message: `invalid value: referencing a nonexistent task`, Paths: []string{"spec.results[0].value"}, }, + }, { + desc: "referencing nonexistent finally task result", + p: &Pipeline{ + ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, + Spec: PipelineSpec{ + Results: []PipelineResult{{ + Name: "initialized", + Value: *NewStructuredValues("$(finally.nonexistent-task.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 { @@ -990,6 +1094,36 @@ func TestValidatePipelineParameterVariables_Success(t *testing.T) { Name: "a-param", Value: ParamValue{Type: ParamTypeString, StringVal: "$(input.workspace.$(params.baz))"}, }}, }}, + }, { + name: "valid array parameter variables in matrix", + params: []ParamSpec{{ + Name: "baz", Type: ParamTypeArray, Default: &ParamValue{Type: ParamTypeArray, ArrayVal: []string{"some", "default"}}, + }, { + Name: "foo-is-baz", Type: ParamTypeArray, + }}, + tasks: []PipelineTask{{ + Name: "bar", + TaskRef: &TaskRef{Name: "bar-task"}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"$(params.baz)", "and", "$(params.foo-is-baz)"}}, + }}}, + }}, + }, { + name: "valid star array parameter variables in matrix", + params: []ParamSpec{{ + Name: "baz", Type: ParamTypeArray, Default: &ParamValue{Type: ParamTypeArray, ArrayVal: []string{"some", "default"}}, + }, { + Name: "foo-is-baz", Type: ParamTypeArray, + }}, + tasks: []PipelineTask{{ + Name: "bar", + TaskRef: &TaskRef{Name: "bar-task"}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"$(params.baz[*])", "and", "$(params.foo-is-baz[*])"}}, + }}}, + }}, }, { name: "array param - using the whole variable as a param's value that is intended to be array type", params: []ParamSpec{{ @@ -1077,6 +1211,24 @@ func TestValidatePipelineParameterVariables_Success(t *testing.T) { }}, }}, }}, + }, { + name: "object param - using individual variables in matrix", + params: []ParamSpec{{ + Name: "myObject", + Type: ParamTypeObject, + Properties: map[string]PropertySpec{ + "key1": {Type: "string"}, + "key2": {Type: "string"}, + }, + }}, + tasks: []PipelineTask{{ + Name: "bar", + TaskRef: &TaskRef{Name: "bar-task"}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"$(params.myObject.key1)", "and", "$(params.myObject.key2)"}}, + }}}, + }}, }, { name: "object param - using the whole variable as a param's value that is intended to be object type", params: []ParamSpec{{ @@ -1396,6 +1548,56 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { Message: `parameter appears more than once`, Paths: []string{"params[baz]"}, }, + }, { + name: "invalid pipeline task with a matrix parameter which is missing from the param declarations", + tasks: []PipelineTask{{ + Name: "foo", + TaskRef: &TaskRef{Name: "foo-task"}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"$(params.does-not-exist)"}}, + }}}, + }}, + expectedError: apis.FieldError{ + Message: `non-existent variable in "$(params.does-not-exist)"`, + Paths: []string{"[0].matrix[a-param].value[0]"}, + }, + }, { + name: "invalid pipeline task with a matrix parameter combined with missing param from the param declarations", + params: []ParamSpec{{ + Name: "foo", Type: ParamTypeString, + }}, + tasks: []PipelineTask{{ + Name: "foo-task", + TaskRef: &TaskRef{Name: "foo-task"}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"$(params.foo)", "and", "$(params.does-not-exist)"}}, + }}}, + }}, + expectedError: apis.FieldError{ + Message: `non-existent variable in "$(params.does-not-exist)"`, + Paths: []string{"[0].matrix[a-param].value[2]"}, + }, + }, { + name: "invalid pipeline task with two matrix parameters and one of them missing from the param declarations", + params: []ParamSpec{{ + Name: "foo", Type: ParamTypeArray, + }}, + tasks: []PipelineTask{{ + Name: "foo-task", + TaskRef: &TaskRef{Name: "foo-task"}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"$(params.foo)"}}, + }, { + Name: "b-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"$(params.does-not-exist)"}}, + }}}, + }}, + expectedError: apis.FieldError{ + Message: `non-existent variable in "$(params.does-not-exist)"`, + Paths: []string{"[0].matrix[b-param].value[0]"}, + }, }, { name: "invalid object key in the input of the when expression", params: []ParamSpec{{ @@ -1516,6 +1718,31 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { Paths: []string{"[0].params[an-object-param].properties[url]"}, }, api: "alpha", + }, { + name: "invalid object key is used to provide values for matrix params", + params: []ParamSpec{{ + Name: "myObject", + Type: ParamTypeObject, + Properties: map[string]PropertySpec{ + "key1": {Type: "string"}, + "key2": {Type: "string"}, + }, + }}, + tasks: []PipelineTask{{ + Name: "foo-task", + TaskRef: &TaskRef{Name: "foo-task"}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"$(params.myObject.key1)"}}, + }, { + Name: "b-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"$(params.myObject.non-exist-key)"}}, + }}}, + }}, + expectedError: apis.FieldError{ + Message: `non-existent variable in "$(params.myObject.non-exist-key)"`, + Paths: []string{"[0].matrix[b-param].value[0]"}, + }, + api: "alpha", }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -1552,9 +1779,10 @@ func TestValidatePipelineWorkspacesDeclarations_Success(t *testing.T) { func TestValidatePipelineWorkspacesUsage_Success(t *testing.T) { tests := []struct { - name string - workspaces []PipelineWorkspaceDeclaration - tasks []PipelineTask + name string + workspaces []PipelineWorkspaceDeclaration + tasks []PipelineTask + skipValidation bool }{{ name: "unused pipeline spec workspaces do not cause an error", workspaces: []PipelineWorkspaceDeclaration{{ @@ -1565,6 +1793,7 @@ func TestValidatePipelineWorkspacesUsage_Success(t *testing.T) { tasks: []PipelineTask{{ Name: "foo", TaskRef: &TaskRef{Name: "foo"}, }}, + skipValidation: false, }, { name: "valid mapping pipeline-task workspace name with pipeline workspace name", workspaces: []PipelineWorkspaceDeclaration{{ @@ -1577,10 +1806,21 @@ func TestValidatePipelineWorkspacesUsage_Success(t *testing.T) { Workspace: "", }}, }}, + skipValidation: false, + }, { + name: "skip validating workspace usage", + workspaces: []PipelineWorkspaceDeclaration{{ + Name: "pipelineWorkspaceName", + }}, + tasks: []PipelineTask{{ + Name: "foo", TaskRef: &TaskRef{Name: "foo"}, + }}, + skipValidation: true, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - errs := validatePipelineWorkspacesUsage(context.Background(), tt.workspaces, tt.tasks).ViaField("tasks") + ctx := config.SkipValidationDueToPropagatedParametersAndWorkspaces(context.Background(), tt.skipValidation) + errs := validatePipelineWorkspacesUsage(ctx, tt.workspaces, tt.tasks).ViaField("tasks") if errs != nil { t.Errorf("Pipeline.validatePipelineWorkspacesUsage() returned error for valid pipeline workspaces: %v", errs) } @@ -2140,6 +2380,209 @@ func TestValidateFinalTasks_Failure(t *testing.T) { } } +func TestContextValid(t *testing.T) { + tests := []struct { + name string + tasks []PipelineTask + }{{ + name: "valid string context variable for Pipeline name", + tasks: []PipelineTask{{ + Name: "bar", + TaskRef: &TaskRef{Name: "bar-task"}, + Params: []Param{{ + Name: "a-param", Value: ParamValue{StringVal: "$(context.pipeline.name)"}, + }}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param-mat", Value: ParamValue{ArrayVal: []string{"$(context.pipeline.name)"}}, + }}}, + }}, + }, { + name: "valid string context variable for PipelineRun name", + tasks: []PipelineTask{{ + Name: "bar", + TaskRef: &TaskRef{Name: "bar-task"}, + Params: []Param{{ + Name: "a-param", Value: ParamValue{StringVal: "$(context.pipelineRun.name)"}, + }}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param-mat", Value: ParamValue{ArrayVal: []string{"$(context.pipelineRun.name)"}}, + }}}, + }}, + }, { + name: "valid string context variable for PipelineRun namespace", + tasks: []PipelineTask{{ + Name: "bar", + TaskRef: &TaskRef{Name: "bar-task"}, + Params: []Param{{ + Name: "a-param", Value: ParamValue{StringVal: "$(context.pipelineRun.namespace)"}, + }}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param-mat", Value: ParamValue{ArrayVal: []string{"$(context.pipelineRun.namespace)"}}, + }}}, + }}, + }, { + name: "valid string context variable for PipelineRun uid", + tasks: []PipelineTask{{ + Name: "bar", + TaskRef: &TaskRef{Name: "bar-task"}, + Params: []Param{{ + Name: "a-param", Value: ParamValue{StringVal: "$(context.pipelineRun.uid)"}, + }}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param-mat", Value: ParamValue{ArrayVal: []string{"$(context.pipelineRun.uid)"}}, + }}}, + }}, + }, { + name: "valid array context variables for Pipeline and PipelineRun names", + tasks: []PipelineTask{{ + Name: "bar", + TaskRef: &TaskRef{Name: "bar-task"}, + Params: []Param{{ + Name: "a-param", Value: ParamValue{ArrayVal: []string{"$(context.pipeline.name)", "and", "$(context.pipelineRun.name)"}}, + }}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param-mat", Value: ParamValue{ArrayVal: []string{"$(context.pipeline.name)", "and", "$(context.pipelineRun.name)"}}, + }}}, + }}, + }, { + name: "valid string context variable for PipelineTask retries", + tasks: []PipelineTask{{ + Name: "bar", + TaskRef: &TaskRef{Name: "bar-task"}, + Params: []Param{{ + Name: "a-param", Value: ParamValue{StringVal: "$(context.pipelineTask.retries)"}, + }}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param", Value: ParamValue{StringVal: "$(context.pipelineTask.retries)"}, + }}}, + }}, + }, { + name: "valid array context variable for PipelineTask retries", + tasks: []PipelineTask{{ + Name: "bar", + TaskRef: &TaskRef{Name: "bar-task"}, + Params: []Param{{ + Name: "a-param", Value: ParamValue{ArrayVal: []string{"$(context.pipelineTask.retries)"}}, + }}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param-mat", Value: ParamValue{ArrayVal: []string{"$(context.pipelineTask.retries)"}}, + }}}, + }}, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := validatePipelineContextVariables(tt.tasks); err != nil { + t.Errorf("Pipeline.validatePipelineContextVariables() returned error for valid pipeline context variables: %v", err) + } + }) + } +} + +func TestContextInvalid(t *testing.T) { + tests := []struct { + name string + tasks []PipelineTask + expectedError apis.FieldError + }{{ + name: "invalid string context variable for pipeline", + tasks: []PipelineTask{{ + Name: "bar", + TaskRef: &TaskRef{Name: "bar-task"}, + Params: []Param{{ + Name: "a-param", Value: ParamValue{StringVal: "$(context.pipeline.missing)"}, + }}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param-foo", Value: ParamValue{ArrayVal: []string{"$(context.pipeline.missing-foo)"}}, + }}}, + }}, + expectedError: *apis.ErrGeneric("").Also(&apis.FieldError{ + Message: `non-existent variable in "$(context.pipeline.missing)"`, + Paths: []string{"value"}, + }).Also(&apis.FieldError{ + Message: `non-existent variable in "$(context.pipeline.missing-foo)"`, + Paths: []string{"value"}, + }), + }, { + name: "invalid string context variable for pipelineRun", + tasks: []PipelineTask{{ + Name: "bar", + TaskRef: &TaskRef{Name: "bar-task"}, + Params: []Param{{ + Name: "a-param", Value: ParamValue{StringVal: "$(context.pipelineRun.missing)"}, + }}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param-foo", Value: ParamValue{ArrayVal: []string{"$(context.pipelineRun.missing-foo)"}}, + }}}, + }}, + expectedError: *apis.ErrGeneric("").Also(&apis.FieldError{ + Message: `non-existent variable in "$(context.pipelineRun.missing)"`, + Paths: []string{"value"}, + }).Also(&apis.FieldError{ + Message: `non-existent variable in "$(context.pipelineRun.missing-foo)"`, + Paths: []string{"value"}, + }), + }, { + name: "invalid string context variable for pipelineTask", + tasks: []PipelineTask{{ + Name: "bar", + TaskRef: &TaskRef{Name: "bar-task"}, + Params: []Param{{ + Name: "a-param", Value: ParamValue{StringVal: "$(context.pipelineTask.missing)"}, + }}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param-foo", Value: ParamValue{ArrayVal: []string{"$(context.pipelineTask.missing-foo)"}}, + }}}, + }}, + expectedError: *apis.ErrGeneric("").Also(&apis.FieldError{ + Message: `non-existent variable in "$(context.pipelineTask.missing)"`, + Paths: []string{"value"}, + }).Also(&apis.FieldError{ + Message: `non-existent variable in "$(context.pipelineTask.missing-foo)"`, + Paths: []string{"value"}, + }), + }, { + name: "invalid array context variables for pipeline, pipelineTask and pipelineRun", + tasks: []PipelineTask{{ + Name: "bar", + TaskRef: &TaskRef{Name: "bar-task"}, + Params: []Param{{ + Name: "a-param", Value: ParamValue{ArrayVal: []string{"$(context.pipeline.missing)", "$(context.pipelineTask.missing)", "$(context.pipelineRun.missing)"}}, + }}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param", Value: ParamValue{ArrayVal: []string{"$(context.pipeline.missing-foo)", "$(context.pipelineTask.missing-foo)", "$(context.pipelineRun.missing-foo)"}}, + }}}, + }}, + expectedError: *apis.ErrGeneric(`non-existent variable in "$(context.pipeline.missing)"`, "value"). + Also(apis.ErrGeneric(`non-existent variable in "$(context.pipelineRun.missing)"`, "value")). + Also(apis.ErrGeneric(`non-existent variable in "$(context.pipelineTask.missing)"`, "value")). + Also(apis.ErrGeneric(`non-existent variable in "$(context.pipeline.missing-foo)"`, "value")). + Also(apis.ErrGeneric(`non-existent variable in "$(context.pipelineRun.missing-foo)"`, "value")). + Also(apis.ErrGeneric(`non-existent variable in "$(context.pipelineTask.missing-foo)"`, "value")), + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validatePipelineContextVariables(tt.tasks) + if err == nil { + t.Errorf("Pipeline.validatePipelineContextVariables() did not return error for invalid pipeline parameters: %s", tt.tasks[0].Params) + } + if d := cmp.Diff(tt.expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" { + t.Errorf("PipelineSpec.Validate() errors diff %s", diff.PrintWantGot(d)) + } + }) + } +} + func TestPipelineTasksExecutionStatus(t *testing.T) { tests := []struct { name string @@ -2372,6 +2815,347 @@ func TestPipelineTasksExecutionStatus(t *testing.T) { } } +// TestMatrixIncompatibleAPIVersions exercises validation of matrix +// that requires alpha feature gate version in order to work. +func TestMatrixIncompatibleAPIVersions(t *testing.T) { + tests := []struct { + name string + requiredVersion string + spec PipelineSpec + }{{ + name: "matrix requires alpha - check tasks", + requiredVersion: "alpha", + spec: PipelineSpec{ + Tasks: PipelineTaskList{{ + Name: "a-task", + TaskRef: &TaskRef{Name: "a-task"}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}}, + }}}, + }}, + }, + }, { + name: "matrix requires alpha - check finally tasks", + requiredVersion: "alpha", + spec: PipelineSpec{ + Tasks: PipelineTaskList{{ + Name: "a-task", + TaskRef: &TaskRef{Name: "a-task"}, + }}, + Finally: PipelineTaskList{{ + Name: "b-task", + TaskRef: &TaskRef{Name: "b-task"}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}}, + }}}, + }}, + }, + }} + versions := []string{"alpha", "stable"} + for _, tt := range tests { + for _, version := range versions { + testName := fmt.Sprintf("(using %s) %s", version, tt.name) + t.Run(testName, func(t *testing.T) { + ps := tt.spec + featureFlags, _ := config.NewFeatureFlagsFromMap(map[string]string{ + "enable-api-fields": version, + "embedded-status": "minimal", + }) + defaults := &config.Defaults{ + DefaultMaxMatrixCombinationsCount: 4, + } + cfg := &config.Config{ + FeatureFlags: featureFlags, + Defaults: defaults, + } + + ctx := config.ToContext(context.Background(), cfg) + + ps.SetDefaults(ctx) + ctx = config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false) + err := ps.Validate(ctx) + + if tt.requiredVersion != version && err == nil { + t.Fatalf("no error received even though version required is %q while feature gate is %q", tt.requiredVersion, version) + } + + if tt.requiredVersion == version && err != nil { + t.Fatalf("error received despite required version and feature gate matching %q: %v", version, err) + } + }) + } + } +} + +func Test_validateMatrix(t *testing.T) { + tests := []struct { + name string + tasks []PipelineTask + wantErrs *apis.FieldError + }{{ + name: "parameter in both matrix and params", + tasks: PipelineTaskList{{ + Name: "a-task", + TaskRef: &TaskRef{Name: "a-task"}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "foobar", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}}, + }}}, + Params: []Param{{ + Name: "foobar", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}}, + }}, + }}, + wantErrs: apis.ErrMultipleOneOf("[0].matrix[foobar]", "[0].params[foobar]"), + }, { + name: "parameters unique in matrix and params", + tasks: PipelineTaskList{{ + Name: "a-task", + TaskRef: &TaskRef{Name: "a-task"}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "foobar", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}}, + }}}, + Params: []Param{{ + Name: "barfoo", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"bar", "foo"}}, + }}, + }}, + }, { + name: "parameters in matrix are strings", + tasks: PipelineTaskList{{ + Name: "a-task", + TaskRef: &TaskRef{Name: "a-task"}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "foo", Value: ParamValue{Type: ParamTypeString, StringVal: "foo"}, + }, { + Name: "bar", Value: ParamValue{Type: ParamTypeString, StringVal: "bar"}, + }}}, + }, { + Name: "b-task", + TaskRef: &TaskRef{Name: "b-task"}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "baz", Value: ParamValue{Type: ParamTypeString, StringVal: "baz"}, + }}}, + }}, + wantErrs: &apis.FieldError{ + Message: "invalid value: parameters of type array only are allowed in matrix", + Paths: []string{"[0].matrix[foo]", "[0].matrix[bar]", "[1].matrix[baz]"}, + }, + }, { + name: "parameters in matrix are arrays", + tasks: PipelineTaskList{{ + Name: "a-task", + TaskRef: &TaskRef{Name: "a-task"}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "foobar", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}}, + }, { + Name: "barfoo", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"bar", "foo"}}, + }}}, + }}, + }, { + name: "parameters in matrix contain results references", + tasks: PipelineTaskList{{ + Name: "a-task", + TaskRef: &TaskRef{Name: "a-task"}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"$(tasks.foo-task.results.a-result)"}}, + }}}, + }, { + Name: "b-task", + TaskRef: &TaskRef{Name: "b-task"}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "b-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"$(tasks.bar-task.results.b-result)"}}, + }}}, + }}, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + featureFlags, _ := config.NewFeatureFlagsFromMap(map[string]string{ + "enable-api-fields": "alpha", + "embedded-status": "minimal", + }) + defaults := &config.Defaults{ + DefaultMaxMatrixCombinationsCount: 4, + } + cfg := &config.Config{ + FeatureFlags: featureFlags, + Defaults: defaults, + } + + ctx := config.ToContext(context.Background(), cfg) + if d := cmp.Diff(tt.wantErrs.Error(), validateMatrix(ctx, tt.tasks).Error()); d != "" { + t.Errorf("validateMatrix() errors diff %s", diff.PrintWantGot(d)) + } + }) + } +} + +func Test_validateResultsFromMatrixedPipelineTasksNotConsumed(t *testing.T) { + tests := []struct { + name string + tasks []PipelineTask + finally []PipelineTask + wantErrs *apis.FieldError + }{{ + name: "results from matrixed task consumed in tasks through parameters", + tasks: PipelineTaskList{{ + Name: "a-task", + TaskRef: &TaskRef{Name: "a-task"}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}}, + }}}, + }, { + Name: "b-task", + TaskRef: &TaskRef{Name: "b-task"}, + Params: []Param{{ + Name: "b-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"$(tasks.a-task.results.a-result)"}}, + }}, + }}, + wantErrs: &apis.FieldError{ + Message: "invalid value: consuming results from matrixed task a-task is not allowed", + Paths: []string{"tasks[1]"}, + }, + }, { + name: "results from matrixed task consumed in finally through parameters", + tasks: PipelineTaskList{{ + Name: "a-task", + TaskRef: &TaskRef{Name: "a-task"}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}}, + }}}, + }}, + finally: PipelineTaskList{{ + Name: "b-task", + TaskRef: &TaskRef{Name: "b-task"}, + Params: []Param{{ + Name: "b-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"$(tasks.a-task.results.a-result)"}}, + }}, + }}, + wantErrs: &apis.FieldError{ + Message: "invalid value: consuming results from matrixed task a-task is not allowed", + Paths: []string{"finally[0]"}, + }, + }, { + name: "results from matrixed task consumed in tasks and finally through parameters", + tasks: PipelineTaskList{{ + Name: "a-task", + TaskRef: &TaskRef{Name: "a-task"}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}}, + }}}, + }, { + Name: "b-task", + TaskRef: &TaskRef{Name: "b-task"}, + Params: []Param{{ + Name: "b-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"$(tasks.a-task.results.a-result)"}}, + }}, + }}, + finally: PipelineTaskList{{ + Name: "c-task", + TaskRef: &TaskRef{Name: "c-task"}, + Params: []Param{{ + Name: "b-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"$(tasks.a-task.results.a-result)"}}, + }}, + }}, + wantErrs: &apis.FieldError{ + Message: "invalid value: consuming results from matrixed task a-task is not allowed", + Paths: []string{"tasks[1]", "finally[0]"}, + }, + }, { + name: "results from matrixed task consumed in tasks through when expressions", + tasks: PipelineTaskList{{ + Name: "a-task", + TaskRef: &TaskRef{Name: "a-task"}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}}, + }}}, + }, { + Name: "b-task", + TaskRef: &TaskRef{Name: "b-task"}, + When: WhenExpressions{{ + Input: "foo", + Operator: selection.In, + Values: []string{"$(tasks.a-task.results.a-result)"}, + }}, + }}, + wantErrs: &apis.FieldError{ + Message: "invalid value: consuming results from matrixed task a-task is not allowed", + Paths: []string{"tasks[1]"}, + }, + }, { + name: "results from matrixed task consumed in finally through when expressions", + tasks: PipelineTaskList{{ + Name: "a-task", + TaskRef: &TaskRef{Name: "a-task"}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}}, + }}}, + }}, + finally: PipelineTaskList{{ + Name: "b-task", + TaskRef: &TaskRef{Name: "b-task"}, + When: WhenExpressions{{ + Input: "$(tasks.a-task.results.a-result)", + Operator: selection.In, + Values: []string{"foo", "bar"}, + }}, + }}, + wantErrs: &apis.FieldError{ + Message: "invalid value: consuming results from matrixed task a-task is not allowed", + Paths: []string{"finally[0]"}, + }, + }, { + name: "results from matrixed task consumed in tasks and finally through when expressions", + tasks: PipelineTaskList{{ + Name: "a-task", + TaskRef: &TaskRef{Name: "a-task"}, + Matrix: &Matrix{ + Params: []Param{{ + Name: "a-param", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}}, + }}}, + }, { + Name: "b-task", + TaskRef: &TaskRef{Name: "b-task"}, + When: WhenExpressions{{ + Input: "$(tasks.a-task.results.a-result)", + Operator: selection.In, + Values: []string{"foo", "bar"}, + }}, + }}, + finally: PipelineTaskList{{ + Name: "c-task", + TaskRef: &TaskRef{Name: "c-task"}, + When: WhenExpressions{{ + Input: "foo", + Operator: selection.In, + Values: []string{"$(tasks.a-task.results.a-result)"}, + }}, + }}, + wantErrs: &apis.FieldError{ + Message: "invalid value: consuming results from matrixed task a-task is not allowed", + Paths: []string{"tasks[1]", "finally[0]"}, + }, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if d := cmp.Diff(tt.wantErrs.Error(), validateResultsFromMatrixedPipelineTasksNotConsumed(tt.tasks, tt.finally).Error()); d != "" { + t.Errorf("validateResultsFromMatrixedPipelineTasksNotConsumed() errors diff %s", diff.PrintWantGot(d)) + } + }) + } +} + func getTaskSpec() TaskSpec { return TaskSpec{ Steps: []Step{{ diff --git a/pkg/apis/pipeline/v1/pipelineref_validation.go b/pkg/apis/pipeline/v1/pipelineref_validation.go index 8fd971828cf..59d1d621635 100644 --- a/pkg/apis/pipeline/v1/pipelineref_validation.go +++ b/pkg/apis/pipeline/v1/pipelineref_validation.go @@ -18,6 +18,7 @@ package v1 import ( "context" + "fmt" "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/version" @@ -31,14 +32,37 @@ func (ref *PipelineRef) Validate(ctx context.Context) (errs *apis.FieldError) { return } - switch { - case ref.Resolver != "": - errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "resolver", config.AlphaAPIFields).ViaField("resolver")) - if ref.Name != "" { - errs = errs.Also(apis.ErrMultipleOneOf("name", "resolver")) + if ref.Resolver != "" || ref.Params != nil { + if ref.Resolver != "" { + errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "resolver", config.AlphaAPIFields).ViaField("resolver")) + if ref.Name != "" { + errs = errs.Also(apis.ErrMultipleOneOf("name", "resolver")) + } } - case ref.Name == "": + if ref.Params != nil { + errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "params", config.AlphaAPIFields).ViaField("params")) + if ref.Name != "" { + errs = errs.Also(apis.ErrMultipleOneOf("name", "params")) + } + if ref.Resolver == "" { + errs = errs.Also(apis.ErrMissingField("resolver")) + } + errs = errs.Also(ValidateParameters(ctx, ref.Params)) + errs = errs.Also(validateResolutionParamTypes(ref.Params).ViaField("params")) + } + } else if ref.Name == "" { errs = errs.Also(apis.ErrMissingField("name")) } return } + +func validateResolutionParamTypes(params []Param) (errs *apis.FieldError) { + for i, p := range params { + if p.Value.Type == ParamTypeArray || p.Value.Type == ParamTypeObject { + errs = errs.Also(apis.ErrGeneric(fmt.Sprintf("remote resolution parameter type must be %s, not %s", + string(ParamTypeString), string(p.Value.Type))).ViaIndex(i)) + } + } + + return errs +} diff --git a/pkg/apis/pipeline/v1/pipelineref_validation_test.go b/pkg/apis/pipeline/v1/pipelineref_validation_test.go index e97bd2fb603..03714cf3d2f 100644 --- a/pkg/apis/pipeline/v1/pipelineref_validation_test.go +++ b/pkg/apis/pipeline/v1/pipelineref_validation_test.go @@ -45,6 +45,23 @@ func TestPipelineRef_Invalid(t *testing.T) { }, }, wantErr: apis.ErrGeneric("resolver requires \"enable-api-fields\" feature gate to be \"alpha\" but it is \"stable\""), + }, { + name: "pipelineref params disallowed without alpha feature gate", + ref: &v1.PipelineRef{ + ResolverRef: v1.ResolverRef{ + Params: []v1.Param{}, + }, + }, + wantErr: apis.ErrMissingField("resolver").Also(apis.ErrGeneric("params requires \"enable-api-fields\" feature gate to be \"alpha\" but it is \"stable\"")), + }, { + name: "pipelineref params disallowed without resolver", + ref: &v1.PipelineRef{ + ResolverRef: v1.ResolverRef{ + Params: []v1.Param{}, + }, + }, + wantErr: apis.ErrMissingField("resolver"), + withContext: config.EnableAlphaAPIFields, }, { name: "pipelineref resolver disallowed in conjunction with pipelineref name", ref: &v1.PipelineRef{ @@ -55,6 +72,57 @@ func TestPipelineRef_Invalid(t *testing.T) { }, wantErr: apis.ErrMultipleOneOf("name", "resolver"), withContext: config.EnableAlphaAPIFields, + }, { + name: "pipelineref params disallowed in conjunction with pipelineref name", + ref: &v1.PipelineRef{ + Name: "bar", + ResolverRef: v1.ResolverRef{ + Params: []v1.Param{{ + Name: "foo", + Value: v1.ParamValue{ + Type: v1.ParamTypeString, + StringVal: "bar", + }, + }}, + }, + }, + wantErr: apis.ErrMultipleOneOf("name", "params").Also(apis.ErrMissingField("resolver")), + withContext: config.EnableAlphaAPIFields, + }, { + name: "pipelineref param array not allowed", + ref: &v1.PipelineRef{ + ResolverRef: v1.ResolverRef{ + Resolver: "some-resolver", + Params: []v1.Param{{ + Name: "foo", + Value: v1.ParamValue{ + Type: v1.ParamTypeArray, + ArrayVal: []string{"bar", "baz"}, + }, + }}, + }, + }, + wantErr: apis.ErrGeneric("remote resolution parameter type must be string, not array"). + Also(apis.ErrGeneric("resolver requires \"enable-api-fields\" feature gate to be \"alpha\" but it is \"stable\"")). + Also(apis.ErrGeneric("params requires \"enable-api-fields\" feature gate to be \"alpha\" but it is \"stable\"")), + }, { + name: "pipelineref param object not allowed", + ref: &v1.PipelineRef{ + ResolverRef: v1.ResolverRef{ + Resolver: "some-resolver", + Params: []v1.Param{{ + Name: "foo", + Value: v1.ParamValue{ + Type: v1.ParamTypeObject, + ObjectVal: map[string]string{"bar": "baz"}, + }, + }}, + }, + }, + wantErr: apis.ErrGeneric("object type parameter requires \"enable-api-fields\" feature gate to be \"alpha\" but it is \"stable\""). + Also(apis.ErrGeneric("remote resolution parameter type must be string, not object")). + Also(apis.ErrGeneric("resolver requires \"enable-api-fields\" feature gate to be \"alpha\" but it is \"stable\"")). + Also(apis.ErrGeneric("params requires \"enable-api-fields\" feature gate to be \"alpha\" but it is \"stable\"")), }} for _, tc := range tests { @@ -83,6 +151,22 @@ func TestPipelineRef_Valid(t *testing.T) { name: "alpha feature: valid resolver", ref: &v1.PipelineRef{ResolverRef: v1.ResolverRef{Resolver: "git"}}, wc: config.EnableAlphaAPIFields, + }, { + name: "alpha feature: valid resolver with params", + ref: &v1.PipelineRef{ResolverRef: v1.ResolverRef{Resolver: "git", Params: []v1.Param{{ + Name: "repo", + Value: v1.ParamValue{ + Type: v1.ParamTypeString, + StringVal: "https://github.com/tektoncd/pipeline.git", + }, + }, { + Name: "branch", + Value: v1.ParamValue{ + Type: v1.ParamTypeString, + StringVal: "baz", + }, + }}}}, + wc: config.EnableAlphaAPIFields, }} for _, ts := range tests { diff --git a/pkg/apis/pipeline/v1/task_validation.go b/pkg/apis/pipeline/v1/task_validation.go index 3406ea2108d..e1ec35ed4c4 100644 --- a/pkg/apis/pipeline/v1/task_validation.go +++ b/pkg/apis/pipeline/v1/task_validation.go @@ -204,7 +204,7 @@ func validateStep(ctx context.Context, s Step, names sets.String) (errs *apis.Fi if s.Script != "" { if len(s.Command) > 0 { errs = errs.Also(&apis.FieldError{ - Message: "script cannot be used with command", + Message: fmt.Sprintf("script cannot be used with command"), Paths: []string{"script"}, }) } @@ -241,11 +241,11 @@ func validateStep(ctx context.Context, s Step, names sets.String) (errs *apis.Fi } if s.OnError != "" { - if s.OnError != Continue && s.OnError != StopAndFail { + if !isParamRefs(string(s.OnError)) && s.OnError != Continue && s.OnError != StopAndFail { errs = errs.Also(&apis.FieldError{ - Message: fmt.Sprintf("invalid value: %v", s.OnError), + Message: fmt.Sprintf("invalid value: \"%v\"", s.OnError), Paths: []string{"onError"}, - Details: "Task step onError must be either continue or stopAndFail", + Details: "Task step onError must be either \"continue\" or \"stopAndFail\"", }) } } @@ -256,6 +256,7 @@ func validateStep(ctx context.Context, s Step, names sets.String) (errs *apis.Fi errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "windows script support", config.AlphaAPIFields).ViaField("script")) } } + // StdoutConfig is an alpha feature and will fail validation if it's used in a task spec // when the enable-api-fields feature gate is not "alpha". if s.StdoutConfig != nil { @@ -322,6 +323,7 @@ func (p ParamSpec) ValidateObjectType(ctx context.Context) *apis.FieldError { return apis.ErrMissingField(fmt.Sprintf("%s.properties", p.Name)) } } + invalidKeys := []string{} for key, propertySpec := range p.Properties { if propertySpec.Type != ParamTypeString { @@ -362,7 +364,6 @@ func ValidateParameterVariables(ctx context.Context, steps []Step, params []Para stringParameterNames.Insert(p.Name) } } - errs = errs.Also(validateNameFormat(stringParameterNames.Insert(arrayParameterNames.List()...), objectParamSpecs)) if config.ValidateParameterVariablesAndWorkspaces(ctx) == true { errs = errs.Also(validateVariables(ctx, steps, "params", allParameterNames)) @@ -543,6 +544,7 @@ func validateStepVariables(ctx context.Context, step Step, prefix string, vars s errs = errs.Also(validateTaskVariable(v.MountPath, prefix, vars).ViaField("MountPath").ViaFieldIndex("volumeMount", i)) errs = errs.Also(validateTaskVariable(v.SubPath, prefix, vars).ViaField("SubPath").ViaFieldIndex("volumeMount", i)) } + errs = errs.Also(validateTaskVariable(string(step.OnError), prefix, vars).ViaField("onError")) return errs } @@ -561,3 +563,9 @@ func validateTaskNoArrayReferenced(value, prefix string, arrayNames sets.String) func validateTaskArraysIsolated(value, prefix string, arrayNames sets.String) *apis.FieldError { return substitution.ValidateVariableIsolatedP(value, prefix, arrayNames) } + +// isParamRefs attempts to check if a specified string looks like it contains any parameter reference +// This is useful to make sure the specified value looks like a Parameter Reference before performing any strict validation +func isParamRefs(s string) bool { + return strings.HasPrefix(s, "$("+ParamsPrefix) +} diff --git a/pkg/apis/pipeline/v1/task_validation_test.go b/pkg/apis/pipeline/v1/task_validation_test.go index 95da35175c3..27c39ce915f 100644 --- a/pkg/apis/pipeline/v1/task_validation_test.go +++ b/pkg/apis/pipeline/v1/task_validation_test.go @@ -1340,6 +1340,7 @@ func TestStepAndSidecarWorkspaces(t *testing.T) { } ctx := config.EnableAlphaAPIFields(context.Background()) ts.SetDefaults(ctx) + ctx = config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, true) if err := ts.Validate(ctx); err != nil { t.Errorf("TaskSpec.Validate() = %v", err) } @@ -1413,48 +1414,76 @@ func TestStepAndSidecarWorkspacesErrors(t *testing.T) { func TestStepOnError(t *testing.T) { tests := []struct { name string + params []v1.ParamSpec steps []v1.Step expectedError *apis.FieldError }{{ - name: "valid step - valid onError usage - set to continue - alpha API", + name: "valid step - valid onError usage - set to continue", steps: []v1.Step{{ OnError: v1.Continue, Image: "image", Args: []string{"arg"}, }}, }, { - name: "valid step - valid onError usage - set to stopAndFail - alpha API", + name: "valid step - valid onError usage - set to stopAndFail", steps: []v1.Step{{ OnError: v1.StopAndFail, Image: "image", Args: []string{"arg"}, }}, }, { - name: "invalid step - onError set to invalid value - alpha API", + name: "valid step - valid onError usage - set to a task parameter", + params: []v1.ParamSpec{{ + Name: "CONTINUE", + Default: &v1.ParamValue{Type: v1.ParamTypeString, StringVal: string(v1.Continue)}, + }}, + steps: []v1.Step{{ + OnError: "$(params.CONTINUE)", + Image: "image", + Args: []string{"arg"}, + }}, + }, { + name: "invalid step - onError set to invalid value", steps: []v1.Step{{ OnError: "onError", Image: "image", Args: []string{"arg"}, }}, expectedError: &apis.FieldError{ - Message: "invalid value: onError", - Paths: []string{"onError"}, - Details: "Task step onError must be either continue or stopAndFail", + Message: fmt.Sprintf("invalid value: \"onError\""), + Paths: []string{"steps[0].onError"}, + Details: "Task step onError must be either \"continue\" or \"stopAndFail\"", + }, + }, { + name: "invalid step - invalid onError usage - set to a parameter which does not exist in the task", + steps: []v1.Step{{ + OnError: "$(params.CONTINUE)", + Image: "image", + Args: []string{"arg"}, + }}, + expectedError: &apis.FieldError{ + Message: "non-existent variable in \"$(params.CONTINUE)\"", + Paths: []string{"steps[0].onError"}, }, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ts := &v1.TaskSpec{ - Steps: tt.steps, + Params: tt.params, + Steps: tt.steps, } ctx := context.Background() ts.SetDefaults(ctx) - ctx = config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, true) + ctx = config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false) err := ts.Validate(ctx) if tt.expectedError == nil && err != nil { - t.Errorf("TaskSpec.Validate() = %v", err) - } else if tt.expectedError != nil && err == nil { - t.Errorf("TaskSpec.Validate() = %v", err) + t.Errorf("No error expected from TaskSpec.Validate() but got = %v", err) + } else if tt.expectedError != nil { + if err == nil { + t.Errorf("Expected error from TaskSpec.Validate() = %v, but got none", tt.expectedError) + } else if d := cmp.Diff(tt.expectedError.Error(), err.Error()); d != "" { + t.Errorf("returned error from TaskSpec.Validate() does not match with the expected error: %s", diff.PrintWantGot(d)) + } } }) } diff --git a/pkg/apis/pipeline/v1/taskref_validation.go b/pkg/apis/pipeline/v1/taskref_validation.go index 87f8bb4c54a..95bc05df142 100644 --- a/pkg/apis/pipeline/v1/taskref_validation.go +++ b/pkg/apis/pipeline/v1/taskref_validation.go @@ -18,7 +18,6 @@ package v1 import ( "context" - "fmt" "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/version" @@ -55,14 +54,3 @@ func (ref *TaskRef) Validate(ctx context.Context) (errs *apis.FieldError) { } return } - -func validateResolutionParamTypes(params []Param) (errs *apis.FieldError) { - for i, p := range params { - if p.Value.Type == ParamTypeArray || p.Value.Type == ParamTypeObject { - errs = errs.Also(apis.ErrGeneric(fmt.Sprintf("remote resolution parameter type must be %s, not %s", - string(ParamTypeString), string(p.Value.Type))).ViaIndex(i)) - } - } - - return errs -} diff --git a/pkg/apis/pipeline/v1/taskref_validation_test.go b/pkg/apis/pipeline/v1/taskref_validation_test.go index c0ddadf5bc4..88eaa52f9ad 100644 --- a/pkg/apis/pipeline/v1/taskref_validation_test.go +++ b/pkg/apis/pipeline/v1/taskref_validation_test.go @@ -42,13 +42,19 @@ func TestTaskRef_Valid(t *testing.T) { taskRef: &v1.TaskRef{ResolverRef: v1.ResolverRef{Resolver: "git"}}, wc: config.EnableAlphaAPIFields, }, { - name: "alpha feature: valid resolver with resource parameters", + name: "alpha feature: valid resolver with params", taskRef: &v1.TaskRef{ResolverRef: v1.ResolverRef{Resolver: "git", Params: []v1.Param{{ - Name: "repo", - Value: *v1.NewStructuredValues("https://github.com/tektoncd/pipeline.git"), + Name: "repo", + Value: v1.ParamValue{ + Type: v1.ParamTypeString, + StringVal: "https://github.com/tektoncd/pipeline.git", + }, }, { - Name: "branch", - Value: *v1.NewStructuredValues("baz"), + Name: "branch", + Value: v1.ParamValue{ + Type: v1.ParamTypeString, + StringVal: "baz", + }, }}}}, wc: config.EnableAlphaAPIFields, }} @@ -84,7 +90,7 @@ func TestTaskRef_Invalid(t *testing.T) { }, wantErr: apis.ErrGeneric("resolver requires \"enable-api-fields\" feature gate to be \"alpha\" but it is \"stable\""), }, { - name: "taskref resource disallowed without alpha feature gate", + name: "taskref params disallowed without alpha feature gate", taskRef: &v1.TaskRef{ ResolverRef: v1.ResolverRef{ Params: []v1.Param{}, @@ -92,7 +98,7 @@ func TestTaskRef_Invalid(t *testing.T) { }, wantErr: apis.ErrMissingField("resolver").Also(apis.ErrGeneric("params requires \"enable-api-fields\" feature gate to be \"alpha\" but it is \"stable\"")), }, { - name: "taskref resolver params disallowed without resolver", + name: "taskref params disallowed without resolver", taskRef: &v1.TaskRef{ ResolverRef: v1.ResolverRef{ Params: []v1.Param{}, @@ -111,13 +117,16 @@ func TestTaskRef_Invalid(t *testing.T) { wantErr: apis.ErrMultipleOneOf("name", "resolver"), wc: config.EnableAlphaAPIFields, }, { - name: "taskref resolver params disallowed in conjunction with taskref name", + name: "taskref params disallowed in conjunction with taskref name", taskRef: &v1.TaskRef{ Name: "bar", ResolverRef: v1.ResolverRef{ Params: []v1.Param{{ - Name: "foo", - Value: *v1.NewStructuredValues("bar"), + Name: "foo", + Value: v1.ParamValue{ + Type: v1.ParamTypeString, + StringVal: "bar", + }, }}, }, }, diff --git a/pkg/apis/pipeline/v1/taskrun_types.go b/pkg/apis/pipeline/v1/taskrun_types.go index 3daa0d0b2a8..534f50a9920 100644 --- a/pkg/apis/pipeline/v1/taskrun_types.go +++ b/pkg/apis/pipeline/v1/taskrun_types.go @@ -99,6 +99,8 @@ const ( // TaskRunCancelledByPipelineMsg indicates that the PipelineRun of which this // TaskRun was a part of has been cancelled. TaskRunCancelledByPipelineMsg TaskRunSpecStatusMessage = "TaskRun cancelled as the PipelineRun it belongs to has been cancelled." + // TaskRunCancelledByPipelineTimeoutMsg indicates that the TaskRun was cancelled because the PipelineRun running it timed out. + TaskRunCancelledByPipelineTimeoutMsg TaskRunSpecStatusMessage = "TaskRun cancelled as the PipelineRun it belongs to has timed out." ) // TaskRunDebug defines the breakpoint config for a particular TaskRun diff --git a/pkg/apis/pipeline/v1/taskrun_types_test.go b/pkg/apis/pipeline/v1/taskrun_types_test.go index 15683676dad..409446e89a5 100644 --- a/pkg/apis/pipeline/v1/taskrun_types_test.go +++ b/pkg/apis/pipeline/v1/taskrun_types_test.go @@ -123,7 +123,7 @@ func TestTaskRunIsDone(t *testing.T) { }, } if !tr.IsDone() { - t.Fatal("Expected pipelinerun status to be done") + t.Fatal("Expected taskrun status to be done") } } @@ -134,11 +134,7 @@ func TestTaskRunIsCancelled(t *testing.T) { }, } if !tr.IsCancelled() { - t.Fatal("Expected pipelinerun status to be cancelled") - } - expected := "" - if string(tr.Spec.StatusMessage) != expected { - t.Fatalf("Expected StatusMessage is %s but got %s", expected, tr.Spec.StatusMessage) + t.Fatal("Expected taskrun status to be cancelled") } }