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 8cb7c26
Show file tree
Hide file tree
Showing 6 changed files with 217 additions and 20 deletions.
6 changes: 5 additions & 1 deletion pkg/apis/pipeline/v1beta1/pipeline_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"fmt"
"strings"

"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/validate"
"github.com/tektoncd/pipeline/pkg/list"
"github.com/tektoncd/pipeline/pkg/reconciler/pipeline/dag"
Expand Down Expand Up @@ -149,7 +150,10 @@ func validatePipelineParameterVariables(ctx context.Context, tasks []PipelineTas
}
}
}
return errs.Also(validatePipelineParametersVariables(tasks, "params", parameterNames, arrayParameterNames, objectParameterNameKeys))
if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields != "alpha" {
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
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
65 changes: 65 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 Expand Up @@ -135,6 +167,7 @@ func TestPipelineRun_Validate(t *testing.T) {
name string
pr v1beta1.PipelineRun
wc func(context.Context) context.Context
api string
}{{
name: "normal case",
pr: v1beta1.PipelineRun{
Expand Down Expand Up @@ -196,6 +229,35 @@ func TestPipelineRun_Validate(t *testing.T) {
},
},
},
}, {
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.pipeline-words[*])"},
}},
}},
}},
},
},
},
api: "alpha",
}, {
name: "pipelinerun pending",
pr: v1beta1.PipelineRun{
Expand Down Expand Up @@ -295,6 +357,9 @@ func TestPipelineRun_Validate(t *testing.T) {
for _, ts := range tests {
t.Run(ts.name, func(t *testing.T) {
ctx := context.Background()
if ts.api == "alpha" {
ctx = config.EnableAlphaAPIFields(context.Background())
}
if ts.wc != nil {
ctx = ts.wc(ctx)
}
Expand Down
33 changes: 16 additions & 17 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 @@ -571,30 +572,28 @@ func validateNameFormat(stringAndArrayParams sets.String, objectParams []ParamSp
}

func validateStepVariables(ctx context.Context, step Step, prefix string, vars sets.String) *apis.FieldError {
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 := validateTaskVariable(ctx, step.Name, prefix, vars).ViaField("name")
errs = errs.Also(validateTaskVariable(ctx, step.Image, prefix, vars).ViaField("image"))
errs = errs.Also(validateTaskVariable(ctx, step.WorkingDir, prefix, vars).ViaField("workingDir"))
errs = errs.Also(validateTaskVariable(ctx, step.Script, prefix, vars).ViaField("script"))
for i, cmd := range step.Command {
errs = errs.Also(validateTaskVariable(cmd, prefix, vars).ViaFieldIndex("command", i))
errs = errs.Also(validateTaskVariable(ctx, cmd, prefix, vars).ViaFieldIndex("command", i))
}
for i, arg := range step.Args {
errs = errs.Also(validateTaskVariable(arg, prefix, vars).ViaFieldIndex("args", i))
errs = errs.Also(validateTaskVariable(ctx, arg, prefix, vars).ViaFieldIndex("args", i))
}
for _, env := range step.Env {
errs = errs.Also(validateTaskVariable(env.Value, prefix, vars).ViaFieldKey("env", env.Name))
errs = errs.Also(validateTaskVariable(ctx, env.Value, prefix, vars).ViaFieldKey("env", env.Name))
}
for i, v := range step.VolumeMounts {
errs = errs.Also(validateTaskVariable(v.Name, prefix, vars).ViaField("name").ViaFieldIndex("volumeMount", i))
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(ctx, v.Name, prefix, vars).ViaField("name").ViaFieldIndex("volumeMount", i))
errs = errs.Also(validateTaskVariable(ctx, v.MountPath, prefix, vars).ViaField("MountPath").ViaFieldIndex("volumeMount", i))
errs = errs.Also(validateTaskVariable(ctx, v.SubPath, prefix, vars).ViaField("SubPath").ViaFieldIndex("volumeMount", i))
}
return errs
}

func validateTaskVariable(value, prefix string, vars sets.String) *apis.FieldError {
func validateTaskVariable(ctx context.Context, value, prefix string, vars sets.String) *apis.FieldError {
return substitution.ValidateVariableP(value, prefix, vars)
}

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
Loading

0 comments on commit 8cb7c26

Please sign in to comment.