Skip to content

Commit a9893c6

Browse files
committed
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.
1 parent 8f76a04 commit a9893c6

10 files changed

+166
-139
lines changed

pkg/apis/pipeline/v1/pipeline_validation.go

+46-33
Original file line numberDiff line numberDiff line change
@@ -69,8 +69,6 @@ func (ps *PipelineSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
6969
errs = errs.Also(ps.ValidateBetaFeaturesEnabledForParamArrayIndexing(ctx))
7070
// Validate the pipeline's workspaces.
7171
errs = errs.Also(validatePipelineWorkspacesDeclarations(ps.Workspaces))
72-
errs = errs.Also(validatePipelineWorkspacesUsage(ctx, ps.Workspaces, ps.Tasks).ViaField("tasks"))
73-
errs = errs.Also(validatePipelineWorkspacesUsage(ctx, ps.Workspaces, ps.Finally).ViaField("finally"))
7472
// Validate the pipeline's results
7573
errs = errs.Also(validatePipelineResults(ps.Results, ps.Tasks, ps.Finally))
7674
errs = errs.Also(validateTasksAndFinallySection(ps))
@@ -79,6 +77,13 @@ func (ps *PipelineSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
7977
errs = errs.Also(validateMatrix(ctx, ps.Tasks).ViaField("tasks"))
8078
errs = errs.Also(validateMatrix(ctx, ps.Finally).ViaField("finally"))
8179
errs = errs.Also(validateResultsFromMatrixedPipelineTasksNotConsumed(ps.Tasks, ps.Finally))
80+
// When propagating params and workspaces, params and workspaces used in the Pipeline spec may not be declared by the Pipeline.
81+
// Only perform this validation after all declared params and workspaces have been propagated.
82+
// TODO(#6647): Remove this flag and call this function in the reconciler instead
83+
if config.ValidateParameterVariablesAndWorkspaces(ctx) {
84+
errs = errs.Also(ps.validatePipelineParameterUsage())
85+
errs = errs.Also(ps.validatePipelineWorkspacesUsage())
86+
}
8287
return errs
8388
}
8489

@@ -297,12 +302,41 @@ func validatePipelineWorkspacesDeclarations(wss []PipelineWorkspaceDeclaration)
297302
return errs
298303
}
299304

300-
// validatePipelineWorkspacesUsage validates that all the referenced workspaces (by pipeline tasks) are specified in
301-
// the pipeline
302-
func validatePipelineWorkspacesUsage(ctx context.Context, wss []PipelineWorkspaceDeclaration, pts []PipelineTask) (errs *apis.FieldError) {
303-
if !config.ValidateParameterVariablesAndWorkspaces(ctx) {
304-
return nil
305+
// validatePipelineParameterUsage validates that parameters referenced in the Pipeline are declared by the Pipeline
306+
func (ps *PipelineSpec) validatePipelineParameterUsage() (errs *apis.FieldError) {
307+
errs = errs.Also(validatePipelineTaskParameterUsage(ps.Tasks, ps.Params).ViaField("tasks"))
308+
errs = errs.Also(validatePipelineTaskParameterUsage(ps.Finally, ps.Params).ViaField("finally"))
309+
return errs
310+
}
311+
312+
// validatePipelineTaskParameterUsage validates that parameters referenced in the Pipeline Tasks are declared by the Pipeline
313+
func validatePipelineTaskParameterUsage(tasks []PipelineTask, params ParamSpecs) (errs *apis.FieldError) {
314+
allParamNames := sets.NewString(params.getNames()...)
315+
_, arrayParams, objectParams := params.sortByType()
316+
arrayParamNames := sets.NewString(arrayParams.getNames()...)
317+
objectParameterNameKeys := map[string][]string{}
318+
for _, p := range objectParams {
319+
for k := range p.Properties {
320+
objectParameterNameKeys[p.Name] = append(objectParameterNameKeys[p.Name], k)
321+
}
305322
}
323+
errs = errs.Also(validatePipelineParametersVariables(tasks, "params", allParamNames, arrayParamNames, objectParameterNameKeys))
324+
for i, task := range tasks {
325+
errs = errs.Also(task.Params.validateDuplicateParameters().ViaFieldIndex("params", i))
326+
}
327+
return errs
328+
}
329+
330+
// validatePipelineWorkspacesUsage validates that workspaces referenced in the Pipeline are declared by the Pipeline
331+
func (ps *PipelineSpec) validatePipelineWorkspacesUsage() (errs *apis.FieldError) {
332+
errs = errs.Also(validatePipelineTasksWorkspacesUsage(ps.Workspaces, ps.Tasks).ViaField("tasks"))
333+
errs = errs.Also(validatePipelineTasksWorkspacesUsage(ps.Workspaces, ps.Finally).ViaField("finally"))
334+
return errs
335+
}
336+
337+
// validatePipelineTasksWorkspacesUsage validates that all the referenced workspaces (by pipeline tasks) are specified in
338+
// the pipeline
339+
func validatePipelineTasksWorkspacesUsage(wss []PipelineWorkspaceDeclaration, pts []PipelineTask) (errs *apis.FieldError) {
306340
workspaceNames := sets.NewString()
307341
for _, ws := range wss {
308342
workspaceNames.Insert(ws.Name)
@@ -316,37 +350,16 @@ func validatePipelineWorkspacesUsage(ctx context.Context, wss []PipelineWorkspac
316350

317351
// ValidatePipelineParameterVariables validates parameters with those specified by each pipeline task,
318352
// (1) it validates the type of parameter is either string or array (2) parameter default value matches
319-
// with the type of that param (3) ensures that the referenced param variable is defined is part of the param declarations
320-
func ValidatePipelineParameterVariables(ctx context.Context, tasks []PipelineTask, params []ParamSpec) (errs *apis.FieldError) {
321-
parameterNames := sets.NewString()
322-
arrayParameterNames := sets.NewString()
323-
objectParameterNameKeys := map[string][]string{}
324-
353+
// with the type of that param
354+
func ValidatePipelineParameterVariables(ctx context.Context, tasks []PipelineTask, params ParamSpecs) (errs *apis.FieldError) {
325355
// validates all the types within a slice of ParamSpecs
326356
errs = errs.Also(ValidateParameterTypes(ctx, params).ViaField("params"))
327-
328-
for _, p := range params {
329-
if parameterNames.Has(p.Name) {
330-
errs = errs.Also(apis.ErrGeneric("parameter appears more than once", "").ViaFieldKey("params", p.Name))
331-
}
332-
// Add parameter name to parameterNames, and to arrayParameterNames if type is array.
333-
parameterNames.Insert(p.Name)
334-
if p.Type == ParamTypeArray {
335-
arrayParameterNames.Insert(p.Name)
336-
}
337-
338-
if p.Type == ParamTypeObject {
339-
for k := range p.Properties {
340-
objectParameterNameKeys[p.Name] = append(objectParameterNameKeys[p.Name], k)
341-
}
342-
}
343-
}
344-
if config.ValidateParameterVariablesAndWorkspaces(ctx) {
345-
errs = errs.Also(validatePipelineParametersVariables(tasks, "params", parameterNames, arrayParameterNames, objectParameterNameKeys))
357+
errs = errs.Also(params.validateNoDuplicateNames())
358+
for i, task := range tasks {
359+
errs = errs.Also(task.Params.validateDuplicateParameters().ViaField("params").ViaIndex(i))
346360
}
347361
return errs
348362
}
349-
350363
func validatePipelineParametersVariables(tasks []PipelineTask, prefix string, paramNames sets.String, arrayParamNames sets.String, objectParamNameKeys map[string][]string) (errs *apis.FieldError) {
351364
for idx, task := range tasks {
352365
errs = errs.Also(validatePipelineParametersVariablesInTaskParameters(task.Params, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaIndex(idx))

pkg/apis/pipeline/v1/pipeline_validation_test.go

+3-18
Original file line numberDiff line numberDiff line change
@@ -1308,7 +1308,6 @@ func TestValidatePipelineDeclaredParameterUsage_Failure(t *testing.T) {
13081308
params []ParamSpec
13091309
tasks []PipelineTask
13101310
expectedError apis.FieldError
1311-
api string
13121311
}{{
13131312
name: "invalid pipeline task with a parameter which is missing from the param declarations",
13141313
tasks: []PipelineTask{{
@@ -1515,7 +1514,6 @@ func TestValidatePipelineDeclaredParameterUsage_Failure(t *testing.T) {
15151514
Message: `non-existent variable in "$(params.myObject.non-exist-key)"`,
15161515
Paths: []string{"[0].when[0].input"},
15171516
},
1518-
api: "alpha",
15191517
}, {
15201518
name: "invalid object key in the Values of the when expression",
15211519
params: []ParamSpec{{
@@ -1539,7 +1537,6 @@ func TestValidatePipelineDeclaredParameterUsage_Failure(t *testing.T) {
15391537
Message: `non-existent variable in "$(params.myObject.non-exist-key)"`,
15401538
Paths: []string{"[0].when[0].values"},
15411539
},
1542-
api: "alpha",
15431540
}, {
15441541
name: "invalid object key is used to provide values for array params",
15451542
params: []ParamSpec{{
@@ -1561,7 +1558,6 @@ func TestValidatePipelineDeclaredParameterUsage_Failure(t *testing.T) {
15611558
Message: `non-existent variable in "$(params.myObject.non-exist-key)"`,
15621559
Paths: []string{"[0].params[a-param].value[0]"},
15631560
},
1564-
api: "alpha",
15651561
}, {
15661562
name: "invalid object key is used to provide values for string params",
15671563
params: []ParamSpec{{
@@ -1583,7 +1579,6 @@ func TestValidatePipelineDeclaredParameterUsage_Failure(t *testing.T) {
15831579
Message: `non-existent variable in "$(params.myObject.non-exist-key)"`,
15841580
Paths: []string{"[0].params[a-param]"},
15851581
},
1586-
api: "alpha",
15871582
}, {
15881583
name: "invalid object key is used to provide values for object params",
15891584
params: []ParamSpec{{
@@ -1611,7 +1606,6 @@ func TestValidatePipelineDeclaredParameterUsage_Failure(t *testing.T) {
16111606
Message: `non-existent variable in "$(params.myObject.non-exist-key)"`,
16121607
Paths: []string{"[0].params[an-object-param].properties[url]"},
16131608
},
1614-
api: "alpha",
16151609
}, {
16161610
name: "invalid object key is used to provide values for matrix params",
16171611
params: []ParamSpec{{
@@ -1636,16 +1630,10 @@ func TestValidatePipelineDeclaredParameterUsage_Failure(t *testing.T) {
16361630
Message: `non-existent variable in "$(params.myObject.non-exist-key)"`,
16371631
Paths: []string{"[0].matrix.params[b-param].value[0]"},
16381632
},
1639-
api: "alpha",
16401633
}}
16411634
for _, tt := range tests {
16421635
t.Run(tt.name, func(t *testing.T) {
1643-
ctx := context.Background()
1644-
if tt.api == "alpha" {
1645-
ctx = config.EnableAlphaAPIFields(context.Background())
1646-
}
1647-
ctx = config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false)
1648-
err := ValidatePipelineParameterVariables(ctx, tt.tasks, tt.params)
1636+
err := validatePipelineTaskParameterUsage(tt.tasks, tt.params)
16491637
if err == nil {
16501638
t.Errorf("Pipeline.ValidatePipelineParameterVariables() did not return error for invalid pipeline parameters")
16511639
}
@@ -1831,7 +1819,6 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) {
18311819
for _, tt := range tests {
18321820
t.Run(tt.name, func(t *testing.T) {
18331821
ctx := context.Background()
1834-
ctx = config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false)
18351822
err := ValidatePipelineParameterVariables(ctx, tt.tasks, tt.params)
18361823
if err == nil {
18371824
t.Errorf("Pipeline.ValidatePipelineParameterVariables() did not return error for invalid pipeline parameters")
@@ -1900,8 +1887,7 @@ func TestValidatePipelineWorkspacesUsage_Success(t *testing.T) {
19001887
}}
19011888
for _, tt := range tests {
19021889
t.Run(tt.name, func(t *testing.T) {
1903-
ctx := config.SkipValidationDueToPropagatedParametersAndWorkspaces(context.Background(), tt.skipValidation)
1904-
errs := validatePipelineWorkspacesUsage(ctx, tt.workspaces, tt.tasks).ViaField("tasks")
1890+
errs := validatePipelineTasksWorkspacesUsage(tt.workspaces, tt.tasks).ViaField("tasks")
19051891
if errs != nil {
19061892
t.Errorf("Pipeline.validatePipelineWorkspacesUsage() returned error for valid pipeline workspaces: %v", errs)
19071893
}
@@ -2019,8 +2005,7 @@ func TestValidatePipelineWorkspacesUsage_Failure(t *testing.T) {
20192005
}}
20202006
for _, tt := range tests {
20212007
t.Run(tt.name, func(t *testing.T) {
2022-
ctx := config.SkipValidationDueToPropagatedParametersAndWorkspaces(context.Background(), false)
2023-
errs := validatePipelineWorkspacesUsage(ctx, tt.workspaces, tt.tasks).ViaField("tasks")
2008+
errs := validatePipelineTasksWorkspacesUsage(tt.workspaces, tt.tasks).ViaField("tasks")
20242009
if errs == nil {
20252010
t.Errorf("Pipeline.validatePipelineWorkspacesUsage() did not return error for invalid pipeline workspaces")
20262011
}

pkg/apis/pipeline/v1/pipelinerun_validation.go

+4-4
Original file line numberDiff line numberDiff line change
@@ -179,12 +179,12 @@ func (ps *PipelineRunSpec) validateInlineParameters(ctx context.Context) (errs *
179179
for _, pt := range ps.PipelineSpec.Tasks {
180180
if pt.TaskSpec != nil && pt.TaskSpec.Steps != nil {
181181
errs = errs.Also(ValidateParameterTypes(ctx, paramSpec))
182-
errs = errs.Also(ValidateParameterVariables(
183-
config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false), pt.TaskSpec.Steps, paramSpec))
182+
errs = errs.Also(ValidateParameterVariables(ctx, pt.TaskSpec.Steps, paramSpec))
183+
errs = errs.Also(validateUsageOfDeclaredParameters(ctx, pt.TaskSpec.Steps, paramSpec))
184184
}
185185
}
186-
errs = errs.Also(ValidatePipelineParameterVariables(
187-
config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false), ps.PipelineSpec.Tasks, paramSpec))
186+
errs = errs.Also(ValidatePipelineParameterVariables(ctx, ps.PipelineSpec.Tasks, paramSpec))
187+
errs = errs.Also(validatePipelineTaskParameterUsage(ps.PipelineSpec.Tasks, paramSpec))
188188
}
189189
return errs
190190
}

pkg/apis/pipeline/v1/task_validation.go

+28-14
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,12 @@ func (ts *TaskSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
7979

8080
return errs
8181
}
82+
// When propagating parameters, parameters used in the Task spec may not be declared by the Task.
83+
// Only perform this validation after all declared parameters have been propagated.
84+
// TODO(#6647): Remove this flag and call this function in the reconciler instead
85+
if config.ValidateParameterVariablesAndWorkspaces(ctx) {
86+
errs = errs.Also(validateUsageOfDeclaredParameters(ctx, ts.Steps, ts.Params))
87+
}
8288

8389
errs = errs.Also(ValidateVolumes(ts.Volumes).ViaField("volumes"))
8490
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) {
103109
return errs
104110
}
105111

112+
// validateUsageOfDeclaredParameters validates that all parameters referenced in the Task are declared by the Task.
113+
func validateUsageOfDeclaredParameters(ctx context.Context, steps []Step, params ParamSpecs) *apis.FieldError {
114+
var errs *apis.FieldError
115+
_, _, objectParams := params.sortByType()
116+
allParameterNames := sets.NewString(params.getNames()...)
117+
errs = errs.Also(validateVariables(ctx, steps, "params", allParameterNames))
118+
errs = errs.Also(validateObjectUsage(ctx, steps, objectParams))
119+
errs = errs.Also(validateObjectParamsHaveProperties(ctx, params))
120+
return errs
121+
}
122+
123+
// validateObjectParamsHaveProperties returns an error if any declared object params are missing properties
124+
func validateObjectParamsHaveProperties(ctx context.Context, params ParamSpecs) *apis.FieldError {
125+
var errs *apis.FieldError
126+
for _, p := range params {
127+
if p.Type == ParamTypeObject && p.Properties == nil {
128+
errs = errs.Also(apis.ErrMissingField(fmt.Sprintf("%s.properties", p.Name)))
129+
}
130+
}
131+
return errs
132+
}
133+
106134
func validateSidecarNames(sidecars []Sidecar) (errs *apis.FieldError) {
107135
for _, sc := range sidecars {
108136
if sc.Name == pipeline.ReservedResultsSidecarName {
@@ -338,14 +366,6 @@ func (p ParamSpec) ValidateType(ctx context.Context) *apis.FieldError {
338366
// definition of `properties` section and the type of a PropertySpec is allowed.
339367
// (Currently, only string is allowed)
340368
func (p ParamSpec) ValidateObjectType(ctx context.Context) *apis.FieldError {
341-
if p.Type == ParamTypeObject && p.Properties == nil {
342-
// If this we are not skipping validation checks due to propagated params
343-
// then properties field is required.
344-
if config.ValidateParameterVariablesAndWorkspaces(ctx) {
345-
return apis.ErrMissingField(fmt.Sprintf("%s.properties", p.Name))
346-
}
347-
}
348-
349369
invalidKeys := []string{}
350370
for key, propertySpec := range p.Properties {
351371
if propertySpec.Type != ParamTypeString {
@@ -370,13 +390,7 @@ func ValidateParameterVariables(ctx context.Context, steps []Step, params ParamS
370390
stringParams, arrayParams, objectParams := params.sortByType()
371391
stringParameterNames := sets.NewString(stringParams.getNames()...)
372392
arrayParameterNames := sets.NewString(arrayParams.getNames()...)
373-
allParameterNames := sets.NewString(params.getNames()...)
374-
375393
errs = errs.Also(validateNameFormat(stringParameterNames.Insert(arrayParameterNames.List()...), objectParams))
376-
if config.ValidateParameterVariablesAndWorkspaces(ctx) {
377-
errs = errs.Also(validateVariables(ctx, steps, "params", allParameterNames))
378-
errs = errs.Also(validateObjectUsage(ctx, steps, objectParams))
379-
}
380394
return errs.Also(validateArrayUsage(steps, "params", arrayParameterNames))
381395
}
382396

pkg/apis/pipeline/v1/taskrun_validation.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,8 @@ func (ts *TaskRunSpec) validateInlineParameters(ctx context.Context) (errs *apis
141141
}
142142
if ts.TaskSpec != nil && ts.TaskSpec.Steps != nil {
143143
errs = errs.Also(ValidateParameterTypes(ctx, paramSpec))
144-
errs = errs.Also(ValidateParameterVariables(config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false), ts.TaskSpec.Steps, paramSpec))
144+
errs = errs.Also(ValidateParameterVariables(ctx, ts.TaskSpec.Steps, paramSpec))
145+
errs = errs.Also(validateUsageOfDeclaredParameters(ctx, ts.TaskSpec.Steps, paramSpec))
145146
}
146147
return errs
147148
}

0 commit comments

Comments
 (0)