From a9893c6fe52841fb9c489d7ad284e85c212053e0 Mon Sep 17 00:00:00 2001 From: Lee Bernick Date: Fri, 12 May 2023 08:34:31 -0400 Subject: [PATCH] Refactor validation of propagated parameters and workspaces When using propagated parameters and workspaces, params/workspaces referenced in a Task or Pipeline are not necessarily declared in that Task/Pipeline. We currently skip some validation that checks whether referenced params/workspaces are declared based on sentinel values injected into the validation context. This commit consolidates validation logic that is gated behind this check. This refactoring will make it easier to remove these sentinel values and perform this validation only where it is supposed to happen. The only functional changes expected as a result of this commit are that we now return a multierror if multiple object parameter declarations are missing keys, instead of only returning an error for the first one. --- pkg/apis/pipeline/v1/pipeline_validation.go | 79 +++++++++++-------- .../pipeline/v1/pipeline_validation_test.go | 21 +---- .../pipeline/v1/pipelinerun_validation.go | 8 +- pkg/apis/pipeline/v1/task_validation.go | 42 ++++++---- pkg/apis/pipeline/v1/taskrun_validation.go | 3 +- .../pipeline/v1beta1/pipeline_validation.go | 78 ++++++++++-------- .../v1beta1/pipeline_validation_test.go | 21 +---- .../v1beta1/pipelinerun_validation.go | 8 +- pkg/apis/pipeline/v1beta1/task_validation.go | 42 ++++++---- .../pipeline/v1beta1/taskrun_validation.go | 3 +- 10 files changed, 166 insertions(+), 139 deletions(-) diff --git a/pkg/apis/pipeline/v1/pipeline_validation.go b/pkg/apis/pipeline/v1/pipeline_validation.go index 190a7a36cb3..4a426ebf0c3 100644 --- a/pkg/apis/pipeline/v1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1/pipeline_validation.go @@ -69,8 +69,6 @@ func (ps *PipelineSpec) Validate(ctx context.Context) (errs *apis.FieldError) { errs = errs.Also(ps.ValidateBetaFeaturesEnabledForParamArrayIndexing(ctx)) // Validate the pipeline's workspaces. errs = errs.Also(validatePipelineWorkspacesDeclarations(ps.Workspaces)) - 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, ps.Finally)) errs = errs.Also(validateTasksAndFinallySection(ps)) @@ -79,6 +77,13 @@ func (ps *PipelineSpec) Validate(ctx context.Context) (errs *apis.FieldError) { errs = errs.Also(validateMatrix(ctx, ps.Tasks).ViaField("tasks")) errs = errs.Also(validateMatrix(ctx, ps.Finally).ViaField("finally")) errs = errs.Also(validateResultsFromMatrixedPipelineTasksNotConsumed(ps.Tasks, ps.Finally)) + // When propagating params and workspaces, params and workspaces used in the Pipeline spec may not be declared by the Pipeline. + // Only perform this validation after all declared params and workspaces have been propagated. + // TODO(#6647): Remove this flag and call this function in the reconciler instead + if config.ValidateParameterVariablesAndWorkspaces(ctx) { + errs = errs.Also(ps.validatePipelineParameterUsage()) + errs = errs.Also(ps.validatePipelineWorkspacesUsage()) + } return errs } @@ -297,12 +302,41 @@ func validatePipelineWorkspacesDeclarations(wss []PipelineWorkspaceDeclaration) return errs } -// validatePipelineWorkspacesUsage validates that all the referenced workspaces (by pipeline tasks) are specified in -// the pipeline -func validatePipelineWorkspacesUsage(ctx context.Context, wss []PipelineWorkspaceDeclaration, pts []PipelineTask) (errs *apis.FieldError) { - if !config.ValidateParameterVariablesAndWorkspaces(ctx) { - return nil +// validatePipelineParameterUsage validates that parameters referenced in the Pipeline are declared by the Pipeline +func (ps *PipelineSpec) validatePipelineParameterUsage() (errs *apis.FieldError) { + errs = errs.Also(validatePipelineTaskParameterUsage(ps.Tasks, ps.Params).ViaField("tasks")) + errs = errs.Also(validatePipelineTaskParameterUsage(ps.Finally, ps.Params).ViaField("finally")) + return errs +} + +// validatePipelineTaskParameterUsage validates that parameters referenced in the Pipeline Tasks are declared by the Pipeline +func validatePipelineTaskParameterUsage(tasks []PipelineTask, params ParamSpecs) (errs *apis.FieldError) { + allParamNames := sets.NewString(params.getNames()...) + _, arrayParams, objectParams := params.sortByType() + arrayParamNames := sets.NewString(arrayParams.getNames()...) + objectParameterNameKeys := map[string][]string{} + for _, p := range objectParams { + for k := range p.Properties { + objectParameterNameKeys[p.Name] = append(objectParameterNameKeys[p.Name], k) + } } + errs = errs.Also(validatePipelineParametersVariables(tasks, "params", allParamNames, arrayParamNames, objectParameterNameKeys)) + for i, task := range tasks { + errs = errs.Also(task.Params.validateDuplicateParameters().ViaFieldIndex("params", i)) + } + return errs +} + +// validatePipelineWorkspacesUsage validates that workspaces referenced in the Pipeline are declared by the Pipeline +func (ps *PipelineSpec) validatePipelineWorkspacesUsage() (errs *apis.FieldError) { + errs = errs.Also(validatePipelineTasksWorkspacesUsage(ps.Workspaces, ps.Tasks).ViaField("tasks")) + errs = errs.Also(validatePipelineTasksWorkspacesUsage(ps.Workspaces, ps.Finally).ViaField("finally")) + return errs +} + +// validatePipelineTasksWorkspacesUsage validates that all the referenced workspaces (by pipeline tasks) are specified in +// the pipeline +func validatePipelineTasksWorkspacesUsage(wss []PipelineWorkspaceDeclaration, pts []PipelineTask) (errs *apis.FieldError) { workspaceNames := sets.NewString() for _, ws := range wss { workspaceNames.Insert(ws.Name) @@ -316,37 +350,16 @@ func validatePipelineWorkspacesUsage(ctx context.Context, wss []PipelineWorkspac // ValidatePipelineParameterVariables validates parameters with those specified by each pipeline task, // (1) it validates the type of parameter is either string or array (2) parameter default value matches -// with the type of that param (3) ensures that the referenced param variable is defined is part of the param declarations -func ValidatePipelineParameterVariables(ctx context.Context, tasks []PipelineTask, params []ParamSpec) (errs *apis.FieldError) { - parameterNames := sets.NewString() - arrayParameterNames := sets.NewString() - objectParameterNameKeys := map[string][]string{} - +// with the type of that param +func ValidatePipelineParameterVariables(ctx context.Context, tasks []PipelineTask, params ParamSpecs) (errs *apis.FieldError) { // validates all the types within a slice of ParamSpecs errs = errs.Also(ValidateParameterTypes(ctx, params).ViaField("params")) - - for _, p := range params { - if parameterNames.Has(p.Name) { - errs = errs.Also(apis.ErrGeneric("parameter appears more than once", "").ViaFieldKey("params", p.Name)) - } - // Add parameter name to parameterNames, and to arrayParameterNames if type is array. - parameterNames.Insert(p.Name) - if p.Type == ParamTypeArray { - arrayParameterNames.Insert(p.Name) - } - - if p.Type == ParamTypeObject { - for k := range p.Properties { - objectParameterNameKeys[p.Name] = append(objectParameterNameKeys[p.Name], k) - } - } - } - if config.ValidateParameterVariablesAndWorkspaces(ctx) { - errs = errs.Also(validatePipelineParametersVariables(tasks, "params", parameterNames, arrayParameterNames, objectParameterNameKeys)) + errs = errs.Also(params.validateNoDuplicateNames()) + for i, task := range tasks { + errs = errs.Also(task.Params.validateDuplicateParameters().ViaField("params").ViaIndex(i)) } return errs } - func validatePipelineParametersVariables(tasks []PipelineTask, prefix string, paramNames sets.String, arrayParamNames sets.String, objectParamNameKeys map[string][]string) (errs *apis.FieldError) { for idx, task := range tasks { errs = errs.Also(validatePipelineParametersVariablesInTaskParameters(task.Params, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaIndex(idx)) diff --git a/pkg/apis/pipeline/v1/pipeline_validation_test.go b/pkg/apis/pipeline/v1/pipeline_validation_test.go index fc6cdf644c8..a039914dba9 100644 --- a/pkg/apis/pipeline/v1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1/pipeline_validation_test.go @@ -1308,7 +1308,6 @@ func TestValidatePipelineDeclaredParameterUsage_Failure(t *testing.T) { params []ParamSpec tasks []PipelineTask expectedError apis.FieldError - api string }{{ name: "invalid pipeline task with a parameter which is missing from the param declarations", tasks: []PipelineTask{{ @@ -1515,7 +1514,6 @@ func TestValidatePipelineDeclaredParameterUsage_Failure(t *testing.T) { Message: `non-existent variable in "$(params.myObject.non-exist-key)"`, Paths: []string{"[0].when[0].input"}, }, - api: "alpha", }, { name: "invalid object key in the Values of the when expression", params: []ParamSpec{{ @@ -1539,7 +1537,6 @@ func TestValidatePipelineDeclaredParameterUsage_Failure(t *testing.T) { Message: `non-existent variable in "$(params.myObject.non-exist-key)"`, Paths: []string{"[0].when[0].values"}, }, - api: "alpha", }, { name: "invalid object key is used to provide values for array params", params: []ParamSpec{{ @@ -1561,7 +1558,6 @@ func TestValidatePipelineDeclaredParameterUsage_Failure(t *testing.T) { Message: `non-existent variable in "$(params.myObject.non-exist-key)"`, Paths: []string{"[0].params[a-param].value[0]"}, }, - api: "alpha", }, { name: "invalid object key is used to provide values for string params", params: []ParamSpec{{ @@ -1583,7 +1579,6 @@ func TestValidatePipelineDeclaredParameterUsage_Failure(t *testing.T) { Message: `non-existent variable in "$(params.myObject.non-exist-key)"`, Paths: []string{"[0].params[a-param]"}, }, - api: "alpha", }, { name: "invalid object key is used to provide values for object params", params: []ParamSpec{{ @@ -1611,7 +1606,6 @@ func TestValidatePipelineDeclaredParameterUsage_Failure(t *testing.T) { Message: `non-existent variable in "$(params.myObject.non-exist-key)"`, 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{{ @@ -1636,16 +1630,10 @@ func TestValidatePipelineDeclaredParameterUsage_Failure(t *testing.T) { Message: `non-existent variable in "$(params.myObject.non-exist-key)"`, Paths: []string{"[0].matrix.params[b-param].value[0]"}, }, - api: "alpha", }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ctx := context.Background() - if tt.api == "alpha" { - ctx = config.EnableAlphaAPIFields(context.Background()) - } - ctx = config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false) - err := ValidatePipelineParameterVariables(ctx, tt.tasks, tt.params) + err := validatePipelineTaskParameterUsage(tt.tasks, tt.params) if err == nil { t.Errorf("Pipeline.ValidatePipelineParameterVariables() did not return error for invalid pipeline parameters") } @@ -1831,7 +1819,6 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ctx := context.Background() - ctx = config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false) err := ValidatePipelineParameterVariables(ctx, tt.tasks, tt.params) if err == nil { t.Errorf("Pipeline.ValidatePipelineParameterVariables() did not return error for invalid pipeline parameters") @@ -1900,8 +1887,7 @@ func TestValidatePipelineWorkspacesUsage_Success(t *testing.T) { }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ctx := config.SkipValidationDueToPropagatedParametersAndWorkspaces(context.Background(), tt.skipValidation) - errs := validatePipelineWorkspacesUsage(ctx, tt.workspaces, tt.tasks).ViaField("tasks") + errs := validatePipelineTasksWorkspacesUsage(tt.workspaces, tt.tasks).ViaField("tasks") if errs != nil { t.Errorf("Pipeline.validatePipelineWorkspacesUsage() returned error for valid pipeline workspaces: %v", errs) } @@ -2019,8 +2005,7 @@ func TestValidatePipelineWorkspacesUsage_Failure(t *testing.T) { }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ctx := config.SkipValidationDueToPropagatedParametersAndWorkspaces(context.Background(), false) - errs := validatePipelineWorkspacesUsage(ctx, tt.workspaces, tt.tasks).ViaField("tasks") + errs := validatePipelineTasksWorkspacesUsage(tt.workspaces, tt.tasks).ViaField("tasks") if errs == nil { t.Errorf("Pipeline.validatePipelineWorkspacesUsage() did not return error for invalid pipeline workspaces") } diff --git a/pkg/apis/pipeline/v1/pipelinerun_validation.go b/pkg/apis/pipeline/v1/pipelinerun_validation.go index 6d63f804d32..a2057a14753 100644 --- a/pkg/apis/pipeline/v1/pipelinerun_validation.go +++ b/pkg/apis/pipeline/v1/pipelinerun_validation.go @@ -179,12 +179,12 @@ func (ps *PipelineRunSpec) validateInlineParameters(ctx context.Context) (errs * for _, pt := range ps.PipelineSpec.Tasks { if pt.TaskSpec != nil && pt.TaskSpec.Steps != nil { errs = errs.Also(ValidateParameterTypes(ctx, paramSpec)) - errs = errs.Also(ValidateParameterVariables( - config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false), pt.TaskSpec.Steps, paramSpec)) + errs = errs.Also(ValidateParameterVariables(ctx, pt.TaskSpec.Steps, paramSpec)) + errs = errs.Also(validateUsageOfDeclaredParameters(ctx, pt.TaskSpec.Steps, paramSpec)) } } - errs = errs.Also(ValidatePipelineParameterVariables( - config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false), ps.PipelineSpec.Tasks, paramSpec)) + errs = errs.Also(ValidatePipelineParameterVariables(ctx, ps.PipelineSpec.Tasks, paramSpec)) + errs = errs.Also(validatePipelineTaskParameterUsage(ps.PipelineSpec.Tasks, paramSpec)) } return errs } diff --git a/pkg/apis/pipeline/v1/task_validation.go b/pkg/apis/pipeline/v1/task_validation.go index 25fe4430b26..3a65fa9f20e 100644 --- a/pkg/apis/pipeline/v1/task_validation.go +++ b/pkg/apis/pipeline/v1/task_validation.go @@ -79,6 +79,12 @@ func (ts *TaskSpec) Validate(ctx context.Context) (errs *apis.FieldError) { return errs } + // When propagating parameters, parameters used in the Task spec may not be declared by the Task. + // Only perform this validation after all declared parameters have been propagated. + // TODO(#6647): Remove this flag and call this function in the reconciler instead + if config.ValidateParameterVariablesAndWorkspaces(ctx) { + errs = errs.Also(validateUsageOfDeclaredParameters(ctx, ts.Steps, ts.Params)) + } errs = errs.Also(ValidateVolumes(ts.Volumes).ViaField("volumes")) errs = errs.Also(validateDeclaredWorkspaces(ts.Workspaces, ts.Steps, ts.StepTemplate).ViaField("workspaces")) @@ -103,6 +109,28 @@ func (ts *TaskSpec) Validate(ctx context.Context) (errs *apis.FieldError) { return errs } +// validateUsageOfDeclaredParameters validates that all parameters referenced in the Task are declared by the Task. +func validateUsageOfDeclaredParameters(ctx context.Context, steps []Step, params ParamSpecs) *apis.FieldError { + var errs *apis.FieldError + _, _, objectParams := params.sortByType() + allParameterNames := sets.NewString(params.getNames()...) + errs = errs.Also(validateVariables(ctx, steps, "params", allParameterNames)) + errs = errs.Also(validateObjectUsage(ctx, steps, objectParams)) + errs = errs.Also(validateObjectParamsHaveProperties(ctx, params)) + return errs +} + +// validateObjectParamsHaveProperties returns an error if any declared object params are missing properties +func validateObjectParamsHaveProperties(ctx context.Context, params ParamSpecs) *apis.FieldError { + var errs *apis.FieldError + for _, p := range params { + if p.Type == ParamTypeObject && p.Properties == nil { + errs = errs.Also(apis.ErrMissingField(fmt.Sprintf("%s.properties", p.Name))) + } + } + return errs +} + func validateSidecarNames(sidecars []Sidecar) (errs *apis.FieldError) { for _, sc := range sidecars { if sc.Name == pipeline.ReservedResultsSidecarName { @@ -338,14 +366,6 @@ func (p ParamSpec) ValidateType(ctx context.Context) *apis.FieldError { // definition of `properties` section and the type of a PropertySpec is allowed. // (Currently, only string is allowed) func (p ParamSpec) ValidateObjectType(ctx context.Context) *apis.FieldError { - if p.Type == ParamTypeObject && p.Properties == nil { - // If this we are not skipping validation checks due to propagated params - // then properties field is required. - if config.ValidateParameterVariablesAndWorkspaces(ctx) { - return apis.ErrMissingField(fmt.Sprintf("%s.properties", p.Name)) - } - } - invalidKeys := []string{} for key, propertySpec := range p.Properties { if propertySpec.Type != ParamTypeString { @@ -370,13 +390,7 @@ func ValidateParameterVariables(ctx context.Context, steps []Step, params ParamS stringParams, arrayParams, objectParams := params.sortByType() stringParameterNames := sets.NewString(stringParams.getNames()...) arrayParameterNames := sets.NewString(arrayParams.getNames()...) - allParameterNames := sets.NewString(params.getNames()...) - errs = errs.Also(validateNameFormat(stringParameterNames.Insert(arrayParameterNames.List()...), objectParams)) - if config.ValidateParameterVariablesAndWorkspaces(ctx) { - errs = errs.Also(validateVariables(ctx, steps, "params", allParameterNames)) - errs = errs.Also(validateObjectUsage(ctx, steps, objectParams)) - } return errs.Also(validateArrayUsage(steps, "params", arrayParameterNames)) } diff --git a/pkg/apis/pipeline/v1/taskrun_validation.go b/pkg/apis/pipeline/v1/taskrun_validation.go index 055094bb6a1..a71b96ba42a 100644 --- a/pkg/apis/pipeline/v1/taskrun_validation.go +++ b/pkg/apis/pipeline/v1/taskrun_validation.go @@ -141,7 +141,8 @@ func (ts *TaskRunSpec) validateInlineParameters(ctx context.Context) (errs *apis } if ts.TaskSpec != nil && ts.TaskSpec.Steps != nil { errs = errs.Also(ValidateParameterTypes(ctx, paramSpec)) - errs = errs.Also(ValidateParameterVariables(config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false), ts.TaskSpec.Steps, paramSpec)) + errs = errs.Also(ValidateParameterVariables(ctx, ts.TaskSpec.Steps, paramSpec)) + errs = errs.Also(validateUsageOfDeclaredParameters(ctx, ts.TaskSpec.Steps, paramSpec)) } return errs } diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation.go b/pkg/apis/pipeline/v1beta1/pipeline_validation.go index 2112858b13c..33f27701af7 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation.go @@ -72,8 +72,6 @@ func (ps *PipelineSpec) Validate(ctx context.Context) (errs *apis.FieldError) { errs = errs.Also(validateExecutionStatusVariables(ps.Tasks, ps.Finally)) // Validate the pipeline's workspaces. errs = errs.Also(validatePipelineWorkspacesDeclarations(ps.Workspaces)) - 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, ps.Finally)) errs = errs.Also(validateTasksAndFinallySection(ps)) @@ -82,6 +80,13 @@ func (ps *PipelineSpec) Validate(ctx context.Context) (errs *apis.FieldError) { errs = errs.Also(validateMatrix(ctx, ps.Tasks).ViaField("tasks")) errs = errs.Also(validateMatrix(ctx, ps.Finally).ViaField("finally")) errs = errs.Also(validateResultsFromMatrixedPipelineTasksNotConsumed(ps.Tasks, ps.Finally)) + // When propagating params and workspaces, params and workspaces used in the Pipeline spec may not be declared by the Pipeline. + // Only perform this validation after all declared params and workspaces have been propagated. + // TODO(#6647): Remove this flag and call this function in the reconciler instead + if config.ValidateParameterVariablesAndWorkspaces(ctx) { + errs = errs.Also(ps.validatePipelineParameterUsage()) + errs = errs.Also(ps.validatePipelineWorkspacesUsage()) + } return errs } @@ -318,12 +323,41 @@ func validatePipelineWorkspacesDeclarations(wss []PipelineWorkspaceDeclaration) return errs } -// validatePipelineWorkspacesUsage validates that all the referenced workspaces (by pipeline tasks) are specified in -// the pipeline -func validatePipelineWorkspacesUsage(ctx context.Context, wss []PipelineWorkspaceDeclaration, pts []PipelineTask) (errs *apis.FieldError) { - if !config.ValidateParameterVariablesAndWorkspaces(ctx) { - return nil +// validatePipelineParameterUsage validates that parameters referenced in the Pipeline are declared by the Pipeline +func (ps *PipelineSpec) validatePipelineParameterUsage() (errs *apis.FieldError) { + errs = errs.Also(validatePipelineTaskParameterUsage(ps.Tasks, ps.Params).ViaField("tasks")) + errs = errs.Also(validatePipelineTaskParameterUsage(ps.Finally, ps.Params).ViaField("finally")) + return errs +} + +// validatePipelineTaskParameterUsage validates that parameters referenced in the Pipeline Tasks are declared by the Pipeline +func validatePipelineTaskParameterUsage(tasks []PipelineTask, params ParamSpecs) (errs *apis.FieldError) { + allParamNames := sets.NewString(params.getNames()...) + _, arrayParams, objectParams := params.sortByType() + arrayParamNames := sets.NewString(arrayParams.getNames()...) + objectParameterNameKeys := map[string][]string{} + for _, p := range objectParams { + for k := range p.Properties { + objectParameterNameKeys[p.Name] = append(objectParameterNameKeys[p.Name], k) + } + } + errs = errs.Also(validatePipelineParametersVariables(tasks, "params", allParamNames, arrayParamNames, objectParameterNameKeys)) + for i, task := range tasks { + errs = errs.Also(task.Params.validateDuplicateParameters().ViaFieldIndex("params", i)) } + return errs +} + +// validatePipelineWorkspacesUsage validates that Workspaces referenced in the Pipeline are declared by the Pipeline +func (ps *PipelineSpec) validatePipelineWorkspacesUsage() (errs *apis.FieldError) { + errs = errs.Also(validatePipelineTasksWorkspacesUsage(ps.Workspaces, ps.Tasks).ViaField("tasks")) + errs = errs.Also(validatePipelineTasksWorkspacesUsage(ps.Workspaces, ps.Finally).ViaField("finally")) + return errs +} + +// validatePipelineTasksWorkspacesUsage validates that all the referenced workspaces (by pipeline tasks) are specified in +// the pipeline +func validatePipelineTasksWorkspacesUsage(wss []PipelineWorkspaceDeclaration, pts []PipelineTask) (errs *apis.FieldError) { workspaceNames := sets.NewString() for _, ws := range wss { workspaceNames.Insert(ws.Name) @@ -337,33 +371,13 @@ func validatePipelineWorkspacesUsage(ctx context.Context, wss []PipelineWorkspac // ValidatePipelineParameterVariables validates parameters with those specified by each pipeline task, // (1) it validates the type of parameter is either string or array (2) parameter default value matches -// with the type of that param (3) ensures that the referenced param variable is defined is part of the param declarations -func ValidatePipelineParameterVariables(ctx context.Context, tasks []PipelineTask, params []ParamSpec) (errs *apis.FieldError) { - parameterNames := sets.NewString() - arrayParameterNames := sets.NewString() - objectParameterNameKeys := map[string][]string{} - +// with the type of that param +func ValidatePipelineParameterVariables(ctx context.Context, tasks []PipelineTask, params ParamSpecs) (errs *apis.FieldError) { // validates all the types within a slice of ParamSpecs errs = errs.Also(ValidateParameterTypes(ctx, params).ViaField("params")) - - for _, p := range params { - if parameterNames.Has(p.Name) { - errs = errs.Also(apis.ErrGeneric("parameter appears more than once", "").ViaFieldKey("params", p.Name)) - } - // Add parameter name to parameterNames, and to arrayParameterNames if type is array. - parameterNames.Insert(p.Name) - if p.Type == ParamTypeArray { - arrayParameterNames.Insert(p.Name) - } - - if p.Type == ParamTypeObject { - for k := range p.Properties { - objectParameterNameKeys[p.Name] = append(objectParameterNameKeys[p.Name], k) - } - } - } - if config.ValidateParameterVariablesAndWorkspaces(ctx) { - errs = errs.Also(validatePipelineParametersVariables(tasks, "params", parameterNames, arrayParameterNames, objectParameterNameKeys)) + errs = errs.Also(params.validateNoDuplicateNames()) + for i, task := range tasks { + errs = errs.Also(task.Params.validateDuplicateParameters().ViaField("params").ViaIndex(i)) } return errs } diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go index 369f7191505..3eb0298afb0 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go @@ -1351,7 +1351,6 @@ func TestValidatePipelineDeclaredParameterUsage_Failure(t *testing.T) { params []ParamSpec tasks []PipelineTask expectedError apis.FieldError - api string }{{ name: "invalid pipeline task with a parameter which is missing from the param declarations", tasks: []PipelineTask{{ @@ -1558,7 +1557,6 @@ func TestValidatePipelineDeclaredParameterUsage_Failure(t *testing.T) { Message: `non-existent variable in "$(params.myObject.non-exist-key)"`, Paths: []string{"[0].when[0].input"}, }, - api: "alpha", }, { name: "invalid object key in the Values of the when expression", params: []ParamSpec{{ @@ -1582,7 +1580,6 @@ func TestValidatePipelineDeclaredParameterUsage_Failure(t *testing.T) { Message: `non-existent variable in "$(params.myObject.non-exist-key)"`, Paths: []string{"[0].when[0].values"}, }, - api: "alpha", }, { name: "invalid object key is used to provide values for array params", params: []ParamSpec{{ @@ -1604,7 +1601,6 @@ func TestValidatePipelineDeclaredParameterUsage_Failure(t *testing.T) { Message: `non-existent variable in "$(params.myObject.non-exist-key)"`, Paths: []string{"[0].params[a-param].value[0]"}, }, - api: "alpha", }, { name: "invalid object key is used to provide values for string params", params: []ParamSpec{{ @@ -1626,7 +1622,6 @@ func TestValidatePipelineDeclaredParameterUsage_Failure(t *testing.T) { Message: `non-existent variable in "$(params.myObject.non-exist-key)"`, Paths: []string{"[0].params[a-param]"}, }, - api: "alpha", }, { name: "invalid object key is used to provide values for object params", params: []ParamSpec{{ @@ -1654,7 +1649,6 @@ func TestValidatePipelineDeclaredParameterUsage_Failure(t *testing.T) { Message: `non-existent variable in "$(params.myObject.non-exist-key)"`, 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{{ @@ -1679,16 +1673,10 @@ func TestValidatePipelineDeclaredParameterUsage_Failure(t *testing.T) { Message: `non-existent variable in "$(params.myObject.non-exist-key)"`, Paths: []string{"[0].matrix.params[b-param].value[0]"}, }, - api: "alpha", }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ctx := context.Background() - if tt.api == "alpha" { - ctx = config.EnableAlphaAPIFields(context.Background()) - } - ctx = config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false) - err := ValidatePipelineParameterVariables(ctx, tt.tasks, tt.params) + err := validatePipelineTaskParameterUsage(tt.tasks, tt.params) if err == nil { t.Errorf("Pipeline.ValidatePipelineParameterVariables() did not return error for invalid pipeline parameters") } @@ -1874,7 +1862,6 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ctx := context.Background() - ctx = config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false) err := ValidatePipelineParameterVariables(ctx, tt.tasks, tt.params) if err == nil { t.Errorf("Pipeline.ValidatePipelineParameterVariables() did not return error for invalid pipeline parameters") @@ -1943,8 +1930,7 @@ func TestValidatePipelineWorkspacesUsage_Success(t *testing.T) { }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ctx := config.SkipValidationDueToPropagatedParametersAndWorkspaces(context.Background(), tt.skipValidation) - errs := validatePipelineWorkspacesUsage(ctx, tt.workspaces, tt.tasks).ViaField("tasks") + errs := validatePipelineTasksWorkspacesUsage(tt.workspaces, tt.tasks).ViaField("tasks") if errs != nil { t.Errorf("Pipeline.validatePipelineWorkspacesUsage() returned error for valid pipeline workspaces: %v", errs) } @@ -2062,8 +2048,7 @@ func TestValidatePipelineWorkspacesUsage_Failure(t *testing.T) { }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ctx := config.SkipValidationDueToPropagatedParametersAndWorkspaces(context.Background(), false) - errs := validatePipelineWorkspacesUsage(ctx, tt.workspaces, tt.tasks).ViaField("tasks") + errs := validatePipelineTasksWorkspacesUsage(tt.workspaces, tt.tasks).ViaField("tasks") if errs == nil { t.Errorf("Pipeline.validatePipelineWorkspacesUsage() did not return error for invalid pipeline workspaces") } diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go b/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go index ed7ac5cce14..8146582ee4a 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go @@ -232,12 +232,12 @@ func (ps *PipelineRunSpec) validateInlineParameters(ctx context.Context) (errs * for _, pt := range ps.PipelineSpec.Tasks { if pt.TaskSpec != nil && pt.TaskSpec.Steps != nil { errs = errs.Also(ValidateParameterTypes(ctx, paramSpec)) - errs = errs.Also(ValidateParameterVariables( - config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false), pt.TaskSpec.Steps, paramSpec)) + errs = errs.Also(ValidateParameterVariables(ctx, pt.TaskSpec.Steps, paramSpec)) + errs = errs.Also(validateUsageOfDeclaredParameters(ctx, pt.TaskSpec.Steps, paramSpec)) } } - errs = errs.Also(ValidatePipelineParameterVariables( - config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false), ps.PipelineSpec.Tasks, paramSpec)) + errs = errs.Also(ValidatePipelineParameterVariables(ctx, ps.PipelineSpec.Tasks, paramSpec)) + errs = errs.Also(validatePipelineTaskParameterUsage(ps.PipelineSpec.Tasks, paramSpec)) } return errs } diff --git a/pkg/apis/pipeline/v1beta1/task_validation.go b/pkg/apis/pipeline/v1beta1/task_validation.go index 41768dbf9f8..2815647f3dd 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation.go +++ b/pkg/apis/pipeline/v1beta1/task_validation.go @@ -79,6 +79,12 @@ func (ts *TaskSpec) Validate(ctx context.Context) (errs *apis.FieldError) { return errs } + // When propagating parameters, parameters used in the Task spec may not be declared by the Task. + // Only perform this validation after all declared parameters have been propagated. + // TODO(#6647): Remove this flag and call this function in the reconciler instead + if config.ValidateParameterVariablesAndWorkspaces(ctx) { + errs = errs.Also(validateUsageOfDeclaredParameters(ctx, ts.Steps, ts.Params)) + } errs = errs.Also(ValidateVolumes(ts.Volumes).ViaField("volumes")) errs = errs.Also(validateDeclaredWorkspaces(ts.Workspaces, ts.Steps, ts.StepTemplate).ViaField("workspaces")) @@ -105,6 +111,28 @@ func (ts *TaskSpec) Validate(ctx context.Context) (errs *apis.FieldError) { return errs } +// validateUsageOfDeclaredParameters validates that all parameters referenced in the Task are declared by the Task. +func validateUsageOfDeclaredParameters(ctx context.Context, steps []Step, params ParamSpecs) *apis.FieldError { + var errs *apis.FieldError + _, _, objectParams := params.sortByType() + allParameterNames := sets.NewString(params.getNames()...) + errs = errs.Also(validateVariables(ctx, steps, "params", allParameterNames)) + errs = errs.Also(validateObjectUsage(ctx, steps, objectParams)) + errs = errs.Also(validateObjectParamsHaveProperties(ctx, params)) + return errs +} + +// validateObjectParamsHaveProperties returns an error if any declared object params are missing properties +func validateObjectParamsHaveProperties(ctx context.Context, params ParamSpecs) *apis.FieldError { + var errs *apis.FieldError + for _, p := range params { + if p.Type == ParamTypeObject && p.Properties == nil { + errs = errs.Also(apis.ErrMissingField(fmt.Sprintf("%s.properties", p.Name))) + } + } + return errs +} + func validateSidecarNames(sidecars []Sidecar) (errs *apis.FieldError) { for _, sc := range sidecars { if sc.Name == pipeline.ReservedResultsSidecarName { @@ -340,14 +368,6 @@ func (p ParamSpec) ValidateType(ctx context.Context) *apis.FieldError { // definition of `properties` section and the type of a PropertySpec is allowed. // (Currently, only string is allowed) func (p ParamSpec) ValidateObjectType(ctx context.Context) *apis.FieldError { - if p.Type == ParamTypeObject && p.Properties == nil { - // If this we are not skipping validation checks due to propagated params - // then properties field is required. - if config.ValidateParameterVariablesAndWorkspaces(ctx) { - return apis.ErrMissingField(fmt.Sprintf("%s.properties", p.Name)) - } - } - invalidKeys := []string{} for key, propertySpec := range p.Properties { if propertySpec.Type != ParamTypeString { @@ -372,13 +392,7 @@ func ValidateParameterVariables(ctx context.Context, steps []Step, params ParamS stringParams, arrayParams, objectParams := params.sortByType() stringParameterNames := sets.NewString(stringParams.getNames()...) arrayParameterNames := sets.NewString(arrayParams.getNames()...) - allParameterNames := sets.NewString(params.getNames()...) - errs = errs.Also(validateNameFormat(stringParameterNames.Insert(arrayParameterNames.List()...), objectParams)) - if config.ValidateParameterVariablesAndWorkspaces(ctx) { - errs = errs.Also(validateVariables(ctx, steps, "params", allParameterNames)) - errs = errs.Also(validateObjectUsage(ctx, steps, objectParams)) - } return errs.Also(validateArrayUsage(steps, "params", arrayParameterNames)) } diff --git a/pkg/apis/pipeline/v1beta1/taskrun_validation.go b/pkg/apis/pipeline/v1beta1/taskrun_validation.go index ef414612b43..f89dd878392 100644 --- a/pkg/apis/pipeline/v1beta1/taskrun_validation.go +++ b/pkg/apis/pipeline/v1beta1/taskrun_validation.go @@ -143,7 +143,8 @@ func (ts *TaskRunSpec) validateInlineParameters(ctx context.Context) (errs *apis } if ts.TaskSpec != nil && ts.TaskSpec.Steps != nil { errs = errs.Also(ValidateParameterTypes(ctx, paramSpec)) - errs = errs.Also(ValidateParameterVariables(config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false), ts.TaskSpec.Steps, paramSpec)) + errs = errs.Also(ValidateParameterVariables(ctx, ts.TaskSpec.Steps, paramSpec)) + errs = errs.Also(validateUsageOfDeclaredParameters(ctx, ts.TaskSpec.Steps, paramSpec)) } return errs }