Skip to content

Commit

Permalink
Move validation from taskspec to taskrunspec when propagating par…
Browse files Browse the repository at this point in the history 
…ameters

Prior to this, propagating parameters only skipped webhook validation
for params defined in script. As a result, when users tried to propagate
params to other fields like `args` or `command`, etc. an webhook
validation was raised. This PR address this issue.
  • Loading branch information
@chitrangpatel
chitrangpatel committed Jul 15, 2022
1 parent 6542823 commit 6bd7552
Show file tree
Hide file tree
Showing 8 changed files with 204 additions and 11 deletions.
3 changes: 2 additions & 1 deletion pkg/apis/pipeline/v1beta1/pipeline_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,8 @@ func validatePipelineParameterVariables(ctx context.Context, tasks []PipelineTas
}
}
}
return errs.Also(validatePipelineParametersVariables(tasks, "params", parameterNames, arrayParameterNames, objectParameterNameKeys))
errs = errs.Also(validatePipelineParametersVariables(tasks, "params", parameterNames, arrayParameterNames, objectParameterNameKeys))
return errs
}

func validatePipelineParametersVariables(tasks []PipelineTask, prefix string, paramNames sets.String, arrayParamNames sets.String, objectParamNameKeys map[string][]string) (errs *apis.FieldError) {
Expand Down
12 changes: 11 additions & 1 deletion pkg/apis/pipeline/v1beta1/pipeline_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1590,6 +1590,7 @@ func TestValidatePipelineParameterVariables_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 @@ -1921,6 +1922,7 @@ func TestValidatePipelineParameterVariables_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 @@ -1944,6 +1946,7 @@ func TestValidatePipelineParameterVariables_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 @@ -1965,6 +1968,7 @@ func TestValidatePipelineParameterVariables_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 @@ -1986,6 +1990,7 @@ func TestValidatePipelineParameterVariables_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 @@ -2013,6 +2018,7 @@ func TestValidatePipelineParameterVariables_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 @@ -2036,10 +2042,14 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) {
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) {
ctx := config.EnableAlphaAPIFields(context.Background())
ctx := context.Background()
if tt.api == "alpha" {
ctx = config.EnableAlphaAPIFields(context.Background())
}
err := validatePipelineParameterVariables(ctx, tt.tasks, tt.params)
if err == nil {
t.Errorf("Pipeline.validatePipelineParameterVariables() did not return error for invalid pipeline parameters")
Expand Down
66 changes: 65 additions & 1 deletion pkg/apis/pipeline/v1beta1/pipelinerun_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,14 +107,78 @@ func (ps *PipelineRunSpec) Validate(ctx context.Context) (errs *apis.FieldError)
wsNames[ws.Name] = idx
}
}

errs = errs.Also(validateParameters(ctx, ps))
for idx, trs := range ps.TaskRunSpecs {
errs = errs.Also(validateTaskRunSpec(ctx, trs).ViaIndex(idx).ViaField("taskRunSpecs"))
}

return errs
}

func validateParameters(ctx context.Context, ps *PipelineRunSpec) (errs *apis.FieldError) {
var paramSpec []ParamSpec
if ps.Params == nil || ps.PipelineSpec == nil {
return errs
}
for _, p := range ps.Params {
pSpec := ParamSpec{
Name: p.Name,
Default: &p.Value,
}
paramSpec = append(paramSpec, pSpec)
}
for _, p := range ps.PipelineSpec.Params {
skip := false
for _, ps := range paramSpec {
if ps.Name == p.Name {
skip = true
break
}
}
if !skip {
paramSpec = append(paramSpec, p)
}
}
for _, pt := range ps.PipelineSpec.Tasks {
for _, p := range pt.Params {
skip := false
for _, ps := range paramSpec {
if ps.Name == p.Name {
skip = true
break
}
}
if !skip {
pSpec := ParamSpec{
Name: p.Name,
Default: &p.Value,
}
paramSpec = append(paramSpec, pSpec)
}
}
for _, p := range pt.TaskSpec.Params {
skip := false
for _, ps := range paramSpec {
if ps.Name == p.Name {
skip = true
break
}
}
if !skip {
paramSpec = append(paramSpec, p)
}
}
}
if ps.PipelineSpec != nil && ps.PipelineSpec.Tasks != nil {
for _, pt := range ps.PipelineSpec.Tasks {
if pt.TaskSpec != nil && pt.TaskSpec.Steps != nil {
errs = errs.Also(ValidateParameterVariables(ctx, pt.TaskSpec.Steps, paramSpec, false))
}
}
}
return errs
}

func validateSpecStatus(status PipelineRunSpecStatus) *apis.FieldError {
switch status {
case "":
Expand Down
32 changes: 32 additions & 0 deletions pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,38 @@ func TestPipelineRun_Invalid(t *testing.T) {
},
},
want: apis.ErrInvalidValue("PipelineRunCancell should be Cancelled, CancelledRunFinally, StoppedRunFinally or PipelineRunPending", "spec.status"),
}, {
name: "propagating params with pipelinespec and taskspec",
pr: v1beta1.PipelineRun{
ObjectMeta: metav1.ObjectMeta{
Name: "pipelinelinename",
},
Spec: v1beta1.PipelineRunSpec{
Params: []v1beta1.Param{{
Name: "pipeline-words",
Value: v1beta1.ArrayOrString{
ArrayVal: []string{"hello", "pipeline"},
},
}},
PipelineSpec: &v1beta1.PipelineSpec{
Tasks: []v1beta1.PipelineTask{{
Name: "echoit",
TaskSpec: &v1beta1.EmbeddedTask{TaskSpec: v1beta1.TaskSpec{
Steps: []v1beta1.Step{{
Name: "echo",
Image: "ubuntu",
Command: []string{"echo"},
Args: []string{"$(params.task-words[*])"},
}},
}},
}},
},
},
},
want: &apis.FieldError{
Message: `non-existent variable in "$(params.task-words[*])"`,
Paths: []string{"spec.pipelineSpec.tasks[0].taskSpec.steps[0].args[0], spec.steps[0].args[0]"},
},
}, {
name: "pipelinerun pending while running",
pr: v1beta1.PipelineRun{
Expand Down
13 changes: 6 additions & 7 deletions pkg/apis/pipeline/v1beta1/task_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ func (ts *TaskSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
errs = errs.Also(validateSteps(ctx, mergedSteps).ViaField("steps"))
errs = errs.Also(ts.Resources.Validate(ctx).ViaField("resources"))
errs = errs.Also(ValidateParameterTypes(ctx, ts.Params).ViaField("params"))
errs = errs.Also(ValidateParameterVariables(ctx, ts.Steps, ts.Params))
errs = errs.Also(ValidateParameterVariables(ctx, ts.Steps, ts.Params, true))
errs = errs.Also(ValidateResourcesVariables(ctx, ts.Steps, ts.Resources))
errs = errs.Also(validateTaskContextVariables(ctx, ts.Steps))
errs = errs.Also(validateResults(ctx, ts.Results).ViaField("results"))
Expand Down Expand Up @@ -329,7 +329,7 @@ func (p ParamSpec) ValidateObjectType() *apis.FieldError {
}

// ValidateParameterVariables validates all variables within a slice of ParamSpecs against a slice of Steps
func ValidateParameterVariables(ctx context.Context, steps []Step, params []ParamSpec) *apis.FieldError {
func ValidateParameterVariables(ctx context.Context, steps []Step, params []ParamSpec, skipVariableValidation bool) *apis.FieldError {
allParameterNames := sets.NewString()
stringParameterNames := sets.NewString()
arrayParameterNames := sets.NewString()
Expand All @@ -351,9 +351,10 @@ func ValidateParameterVariables(ctx context.Context, steps []Step, params []Para
stringParameterNames.Insert(p.Name)
}
}

errs = errs.Also(validateNameFormat(stringParameterNames.Insert(arrayParameterNames.List()...), objectParamSpecs))
errs = errs.Also(validateVariables(ctx, steps, "params", allParameterNames))
if !(skipVariableValidation && config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == "alpha") {
errs = errs.Also(validateVariables(ctx, steps, "params", allParameterNames))
}
errs = errs.Also(validateArrayUsage(steps, "params", arrayParameterNames))
errs = errs.Also(validateObjectDefault(objectParamSpecs))
return errs.Also(validateObjectUsage(ctx, steps, objectParamSpecs))
Expand Down Expand Up @@ -574,9 +575,7 @@ func validateStepVariables(ctx context.Context, step Step, prefix string, vars s
errs := validateTaskVariable(step.Name, prefix, vars).ViaField("name")
errs = errs.Also(validateTaskVariable(step.Image, prefix, vars).ViaField("image"))
errs = errs.Also(validateTaskVariable(step.WorkingDir, prefix, vars).ViaField("workingDir"))
if !(config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == "alpha" && prefix == "params") {
errs = errs.Also(validateTaskVariable(step.Script, prefix, vars).ViaField("script"))
}
errs = errs.Also(validateTaskVariable(step.Script, prefix, vars).ViaField("script"))
for i, cmd := range step.Command {
errs = errs.Also(validateTaskVariable(cmd, prefix, vars).ViaFieldIndex("command", i))
}
Expand Down
35 changes: 34 additions & 1 deletion pkg/apis/pipeline/v1beta1/task_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,26 @@ func TestTaskSpecValidate(t *testing.T) {
Image: "some-image",
},
},
}, {
name: "propagating params valid step with script",
fields: fields{
Steps: []v1beta1.Step{{
Image: "my-image",
Script: `
#!/usr/bin/env bash
$(params.message)`,
}},
},
}, {
name: "propagating params valid step with args",
fields: fields{
Steps: []v1beta1.Step{{
Name: "mystep",
Image: "my-image",
Command: []string{"$(params.command)"},
Args: []string{"$(params.message)"},
}},
},
}, {
name: "valid step with script",
fields: fields{
Expand Down Expand Up @@ -450,6 +470,7 @@ func TestTaskSpecValidateError(t *testing.T) {
name string
fields fields
expectedError apis.FieldError
api string
}{{
name: "empty spec",
expectedError: apis.FieldError{
Expand Down Expand Up @@ -593,6 +614,7 @@ func TestTaskSpecValidateError(t *testing.T) {
Paths: []string{"params"},
Details: "Object Names: \nMust only contain alphanumeric characters, hyphens (-), underscores (_) \nMust begin with a letter or an underscore (_)",
},
api: "alpha",
}, {
name: "duplicated param names",
fields: fields{
Expand Down Expand Up @@ -697,6 +719,7 @@ func TestTaskSpecValidateError(t *testing.T) {
Message: `"object" type does not match default value's type: "string"`,
Paths: []string{"params.task.type", "params.task.default.type"},
},
api: "alpha",
}, {
name: "the spec of object type parameter misses the definition of properties",
fields: fields{
Expand All @@ -708,6 +731,7 @@ func TestTaskSpecValidateError(t *testing.T) {
Steps: validSteps,
},
expectedError: *apis.ErrMissingField(fmt.Sprintf("params.task.properties")),
api: "alpha",
}, {
name: "PropertySpec type is set with unsupported type",
fields: fields{
Expand All @@ -726,6 +750,7 @@ func TestTaskSpecValidateError(t *testing.T) {
Message: fmt.Sprintf("The value type specified for these keys %v is invalid", []string{"key1"}),
Paths: []string{"params.task.properties"},
},
api: "alpha",
}, {
name: "keys defined in properties are missed in default",
fields: fields{
Expand All @@ -747,6 +772,7 @@ func TestTaskSpecValidateError(t *testing.T) {
Message: fmt.Sprintf("Required key(s) %s are missing in the value provider.", []string{"key2"}),
Paths: []string{"myobjectParam.properties", "myobjectParam.default"},
},
api: "alpha",
}, {
name: "invalid step",
fields: fields{
Expand Down Expand Up @@ -961,6 +987,7 @@ func TestTaskSpecValidateError(t *testing.T) {
Message: `variable type invalid in "$(params.gitrepo)"`,
Paths: []string{"steps[0].image"},
},
api: "alpha",
}, {
name: "object star used in a string field",
fields: fields{
Expand All @@ -983,6 +1010,7 @@ func TestTaskSpecValidateError(t *testing.T) {
Message: `variable type invalid in "$(params.gitrepo[*])"`,
Paths: []string{"steps[0].image"},
},
api: "alpha",
}, {
name: "object used in a field that can accept array type",
fields: fields{
Expand All @@ -1005,6 +1033,7 @@ func TestTaskSpecValidateError(t *testing.T) {
Message: `variable type invalid in "$(params.gitrepo)"`,
Paths: []string{"steps[0].args[0]"},
},
api: "alpha",
}, {
name: "object star used in a field that can accept array type",
fields: fields{
Expand All @@ -1027,6 +1056,7 @@ func TestTaskSpecValidateError(t *testing.T) {
Message: `variable type invalid in "$(params.gitrepo[*])"`,
Paths: []string{"steps[0].args[0]"},
},
api: "alpha",
}, {
name: "Inexistent param variable in volumeMount with existing",
fields: fields{
Expand Down Expand Up @@ -1299,7 +1329,10 @@ func TestTaskSpecValidateError(t *testing.T) {
Workspaces: tt.fields.Workspaces,
Results: tt.fields.Results,
}
ctx := config.EnableAlphaAPIFields(context.Background())
ctx := context.Background()
if tt.api == "alpha" {
ctx = config.EnableAlphaAPIFields(context.Background())
}
ts.SetDefaults(ctx)
err := ts.Validate(ctx)
if err == nil {
Expand Down
32 changes: 32 additions & 0 deletions pkg/apis/pipeline/v1beta1/taskrun_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ func (ts *TaskRunSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
}

errs = errs.Also(ValidateParameters(ctx, ts.Params).ViaField("params"))
errs = errs.Also(validateInlineParameters(ctx, ts).ViaField("params"))
errs = errs.Also(ValidateWorkspaceBindings(ctx, ts.Workspaces).ViaField("workspaces"))
errs = errs.Also(ts.Resources.Validate(ctx).ViaField("resources"))
if ts.Debug != nil {
Expand Down Expand Up @@ -118,6 +119,37 @@ func ValidateWorkspaceBindings(ctx context.Context, wb []WorkspaceBinding) (errs
return errs
}

func validateInlineParameters(ctx context.Context, ts *TaskRunSpec) (errs *apis.FieldError) {
params := ts.Params
var paramSpec []ParamSpec
if ts.Params == nil || ts.TaskSpec == nil {
return errs
}
for _, p := range params {
pSpec := ParamSpec{
Name: p.Name,
Default: &p.Value,
}
paramSpec = append(paramSpec, pSpec)
}
for _, p := range ts.TaskSpec.Params {
skip := false
for _, ps := range paramSpec {
if ps.Name == p.Name {
skip = true
break
}
}
if !skip {
paramSpec = append(paramSpec, p)
}
}
if ts.TaskSpec != nil && ts.TaskSpec.Steps != nil {
errs = errs.Also(ValidateParameterVariables(ctx, ts.TaskSpec.Steps, paramSpec, false))
}
return errs
}

// ValidateParameters makes sure the params for the Task are valid.
func ValidateParameters(ctx context.Context, params []Param) (errs *apis.FieldError) {
var names []string
Expand Down
Loading

0 comments on commit 6bd7552

Please sign in to comment.