Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor validation of propagated parameters and workspaces #6660

Merged
merged 1 commit into from
May 16, 2023
+166 −139
Merged

Refactor validation of propagated parameters and workspaces #6660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 46 additions & 33 deletions pkg/apis/pipeline/v1/pipeline_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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
}

Expand Down Expand Up @@ -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)
Expand All @@ -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))
Expand Down
21 changes: 3 additions & 18 deletions pkg/apis/pipeline/v1/pipeline_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{{
Expand Down Expand Up @@ -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{{
Expand All @@ -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{{
Expand All @@ -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{{
Expand All @@ -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{{
Expand Down Expand Up @@ -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{{
Expand All @@ -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")
}
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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")
}
Expand Down
8 changes: 4 additions & 4 deletions pkg/apis/pipeline/v1/pipelinerun_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
42 changes: 28 additions & 14 deletions pkg/apis/pipeline/v1/task_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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))
}

Expand Down
3 changes: 2 additions & 1 deletion pkg/apis/pipeline/v1/taskrun_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
Loading