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.

synced v1 with v1beta1
  • Loading branch information
@chitrangpatel
chitrangpatel committed Aug 8, 2022
1 parent e8ee879 commit 3e446f0
Show file tree
Hide file tree
Showing 9 changed files with 315 additions and 15 deletions.
13 changes: 13 additions & 0 deletions pkg/apis/config/contexts.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ import (
// isSubstituted is used for associating the parameter substitution inside the context.Context.
type isSubstituted struct{}

// validateEmbeddedVariables is used for deciding whether to validate or skip parameters and workspaces inside the contect.Context.
type validateEmbeddedVariables string

// WithinSubstituted is used to note that it is calling within
// the context of a substitute variable operation.
func WithinSubstituted(ctx context.Context) context.Context {
Expand All @@ -33,3 +36,13 @@ func WithinSubstituted(ctx context.Context) context.Context {
func IsSubstituted(ctx context.Context) bool {
return ctx.Value(isSubstituted{}) != nil
}

// SetValidateParameterVariablesAndWorkspaces sets the context to skip validation of parameters when embedded vs referenced to true or false.
func SetValidateParameterVariablesAndWorkspaces(ctx context.Context, validate bool) context.Context {
return context.WithValue(ctx, validateEmbeddedVariables("ValidateParameterVariablesAndWorkspaces"), validate)
}

// ValidateParameterVariablesAndWorkspaces indicates if validation of paramater variables and workspaces should be conducted.
func ValidateParameterVariablesAndWorkspaces(ctx context.Context) bool {
return ctx.Value(validateEmbeddedVariables("ValidateParameterVariablesAndWorkspaces")) == true
}
9 changes: 5 additions & 4 deletions pkg/apis/pipeline/v1/task_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ func (t *Task) Validate(ctx context.Context) *apis.FieldError {
return nil
}
errs := validate.ObjectMetadata(t.GetObjectMeta()).ViaField("metadata")
ctx = config.SetValidateParameterVariablesAndWorkspaces(ctx, true)
return errs.Also(t.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec"))
}

Expand Down Expand Up @@ -361,7 +362,9 @@ func ValidateParameterVariables(ctx context.Context, steps []Step, params []Para
}

errs = errs.Also(validateNameFormat(stringParameterNames.Insert(arrayParameterNames.List()...), objectParamSpecs))
errs = errs.Also(validateVariables(ctx, steps, "params", allParameterNames))
if config.ValidateParameterVariablesAndWorkspaces(ctx) == true {
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 @@ -563,9 +566,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
72 changes: 69 additions & 3 deletions pkg/apis/pipeline/v1/task_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,20 @@ func TestTaskValidate(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{Name: "task"},
},
wc: apis.WithinDelete,
}, {
name: "valid task",
t: &v1.Task{
ObjectMeta: metav1.ObjectMeta{Name: "task"},
Spec: v1.TaskSpec{
Steps: []v1.Step{{
Name: "my-step",
Image: "my-image",
Script: `
#!/usr/bin/env bash
echo hello`,
}},
},
},
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand All @@ -77,8 +91,9 @@ func TestTaskSpecValidate(t *testing.T) {
Results []v1.TaskResult
}
tests := []struct {
name string
fields fields
name string
fields fields
validateParameterVariablesAndWorkspaces bool
}{{
name: "unnamed steps",
fields: fields{
Expand All @@ -88,6 +103,7 @@ func TestTaskSpecValidate(t *testing.T) {
Image: "myotherimage",
}},
},
validateParameterVariablesAndWorkspaces: true,
}, {
name: "valid params type implied",
fields: fields{
Expand All @@ -98,6 +114,7 @@ func TestTaskSpecValidate(t *testing.T) {
}},
Steps: validSteps,
},
validateParameterVariablesAndWorkspaces: true,
}, {
name: "valid params type explicit",
fields: fields{
Expand Down Expand Up @@ -129,6 +146,7 @@ func TestTaskSpecValidate(t *testing.T) {
}},
Steps: validSteps,
},
validateParameterVariablesAndWorkspaces: true,
}, {
name: "valid template variable",
fields: fields{
Expand All @@ -144,6 +162,7 @@ func TestTaskSpecValidate(t *testing.T) {
WorkingDir: "/foo/bar/src/",
}},
},
validateParameterVariablesAndWorkspaces: true,
}, {
name: "valid array template variable",
fields: fields{
Expand All @@ -162,6 +181,7 @@ func TestTaskSpecValidate(t *testing.T) {
WorkingDir: "/foo/bar/src/",
}},
},
validateParameterVariablesAndWorkspaces: true,
}, {
name: "valid object template variable",
fields: fields{
Expand All @@ -180,6 +200,7 @@ func TestTaskSpecValidate(t *testing.T) {
WorkingDir: "/foo/bar/src/",
}},
},
validateParameterVariablesAndWorkspaces: true,
}, {
name: "valid star array template variable",
fields: fields{
Expand All @@ -198,6 +219,7 @@ func TestTaskSpecValidate(t *testing.T) {
WorkingDir: "/foo/bar/src/",
}},
},
validateParameterVariablesAndWorkspaces: true,
}, {
name: "valid path variable for legacy credential helper (aka creds-init)",
fields: fields{
Expand All @@ -207,6 +229,7 @@ func TestTaskSpecValidate(t *testing.T) {
Args: []string{"$(credentials.path)"},
}},
},
validateParameterVariablesAndWorkspaces: true,
}, {
name: "step template included in validation",
fields: fields{
Expand All @@ -219,6 +242,30 @@ func TestTaskSpecValidate(t *testing.T) {
Image: "some-image",
},
},
validateParameterVariablesAndWorkspaces: true,
}, {
name: "propagating params valid step with script",
fields: fields{
Steps: []v1.Step{{
Name: "propagatingparams",
Image: "my-image",
Script: `
#!/usr/bin/env bash
$(params.message)`,
}},
},
validateParameterVariablesAndWorkspaces: false,
}, {
name: "propagating params valid step with args",
fields: fields{
Steps: []v1.Step{{
Name: "propagatingparams",
Image: "my-image",
Command: []string{"$(params.command)"},
Args: []string{"$(params.message)"},
}},
},
validateParameterVariablesAndWorkspaces: false,
}, {
name: "valid step with script",
fields: fields{
Expand All @@ -229,6 +276,7 @@ func TestTaskSpecValidate(t *testing.T) {
hello world`,
}},
},
validateParameterVariablesAndWorkspaces: true,
}, {
name: "valid step with parameterized script",
fields: fields{
Expand All @@ -244,6 +292,7 @@ func TestTaskSpecValidate(t *testing.T) {
hello $(params.baz)`,
}},
},
validateParameterVariablesAndWorkspaces: true,
}, {
name: "valid step with script and args",
fields: fields{
Expand All @@ -255,6 +304,7 @@ func TestTaskSpecValidate(t *testing.T) {
hello $1`,
}},
},
validateParameterVariablesAndWorkspaces: true,
}, {
name: "valid step with volumeMount under /tekton/home",
fields: fields{
Expand All @@ -266,6 +316,7 @@ func TestTaskSpecValidate(t *testing.T) {
}},
}},
},
validateParameterVariablesAndWorkspaces: true,
}, {
name: "valid workspace",
fields: fields{
Expand All @@ -279,6 +330,7 @@ func TestTaskSpecValidate(t *testing.T) {
MountPath: "some/path",
}},
},
validateParameterVariablesAndWorkspaces: true,
}, {
name: "valid result",
fields: fields{
Expand All @@ -291,6 +343,7 @@ func TestTaskSpecValidate(t *testing.T) {
Description: "my great result",
}},
},
validateParameterVariablesAndWorkspaces: true,
}, {
name: "valid result type string",
fields: fields{
Expand All @@ -304,6 +357,7 @@ func TestTaskSpecValidate(t *testing.T) {
Description: "my great result",
}},
},
validateParameterVariablesAndWorkspaces: true,
}, {
name: "valid result type array",
fields: fields{
Expand All @@ -317,6 +371,7 @@ func TestTaskSpecValidate(t *testing.T) {
Description: "my great result",
}},
},
validateParameterVariablesAndWorkspaces: true,
}, {
name: "valid result type object",
fields: fields{
Expand All @@ -334,6 +389,7 @@ func TestTaskSpecValidate(t *testing.T) {
},
}},
},
validateParameterVariablesAndWorkspaces: true,
}, {
name: "valid task name context",
fields: fields{
Expand All @@ -345,6 +401,7 @@ func TestTaskSpecValidate(t *testing.T) {
hello "$(context.task.name)"`,
}},
},
validateParameterVariablesAndWorkspaces: true,
}, {
name: "valid task retry count context",
fields: fields{
Expand All @@ -356,6 +413,7 @@ func TestTaskSpecValidate(t *testing.T) {
retry count "$(context.task.retry-count)"`,
}},
},
validateParameterVariablesAndWorkspaces: true,
}, {
name: "valid taskrun name context",
fields: fields{
Expand All @@ -367,6 +425,7 @@ func TestTaskSpecValidate(t *testing.T) {
hello "$(context.taskRun.name)"`,
}},
},
validateParameterVariablesAndWorkspaces: true,
}, {
name: "valid taskrun uid context",
fields: fields{
Expand All @@ -378,6 +437,7 @@ func TestTaskSpecValidate(t *testing.T) {
hello "$(context.taskRun.uid)"`,
}},
},
validateParameterVariablesAndWorkspaces: true,
}, {
name: "valid context",
fields: fields{
Expand All @@ -389,6 +449,7 @@ func TestTaskSpecValidate(t *testing.T) {
hello "$(context.taskRun.namespace)"`,
}},
},
validateParameterVariablesAndWorkspaces: true,
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand All @@ -401,6 +462,7 @@ func TestTaskSpecValidate(t *testing.T) {
}
ctx := config.EnableAlphaAPIFields(context.Background())
ts.SetDefaults(ctx)
ctx = config.SetValidateParameterVariablesAndWorkspaces(ctx, tt.validateParameterVariablesAndWorkspaces)
if err := ts.Validate(ctx); err != nil {
t.Errorf("TaskSpec.Validate() = %v", err)
}
Expand Down Expand Up @@ -1197,6 +1259,7 @@ func TestTaskSpecValidateError(t *testing.T) {
}
ctx := config.EnableAlphaAPIFields(context.Background())
ts.SetDefaults(ctx)
ctx = config.SetValidateParameterVariablesAndWorkspaces(ctx, true)
err := ts.Validate(ctx)
if err == nil {
t.Fatalf("Expected an error, got nothing for %v", ts)
Expand Down Expand Up @@ -1301,6 +1364,7 @@ func TestStepAndSidecarWorkspacesErrors(t *testing.T) {

ctx := config.EnableAlphaAPIFields(context.Background())
ts.SetDefaults(ctx)
ctx = config.SetValidateParameterVariablesAndWorkspaces(ctx, false)
err := ts.Validate(ctx)
if err == nil {
t.Fatalf("Expected an error, got nothing for %v", ts)
Expand Down Expand Up @@ -1352,6 +1416,7 @@ func TestStepOnError(t *testing.T) {
}
ctx := context.Background()
ts.SetDefaults(ctx)
ctx = config.SetValidateParameterVariablesAndWorkspaces(ctx, false)
err := ts.Validate(ctx)
if tt.expectedError == nil && err != nil {
t.Errorf("TaskSpec.Validate() = %v", err)
Expand Down Expand Up @@ -1445,8 +1510,8 @@ func TestIncompatibleAPIVersions(t *testing.T) {
if version == "alpha" {
ctx = config.EnableAlphaAPIFields(ctx)
}

ts.SetDefaults(ctx)
ctx = config.SetValidateParameterVariablesAndWorkspaces(ctx, false)
err := ts.Validate(ctx)

if tt.requiredVersion != version && err == nil {
Expand Down Expand Up @@ -1530,6 +1595,7 @@ func TestSubstitutedContext(t *testing.T) {
}
ctx := context.Background()
ts.SetDefaults(ctx)
ctx = config.SetValidateParameterVariablesAndWorkspaces(ctx, true)
if tt.fields.SubstitutionContext {
ctx = config.WithinSubstituted(ctx)
}
Expand Down
2 changes: 2 additions & 0 deletions pkg/apis/pipeline/v1beta1/cluster_task_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package v1beta1
import (
"context"

"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/validate"
"knative.dev/pkg/apis"
)
Expand All @@ -31,5 +32,6 @@ func (t *ClusterTask) Validate(ctx context.Context) *apis.FieldError {
return nil
}
errs := validate.ObjectMetadata(t.GetObjectMeta()).ViaField("metadata")
ctx = config.SetValidateParameterVariablesAndWorkspaces(ctx, true)
return errs.Also(t.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec"))
}
10 changes: 5 additions & 5 deletions pkg/apis/pipeline/v1beta1/task_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ func (t *Task) Validate(ctx context.Context) *apis.FieldError {
return nil
}
errs := validate.ObjectMetadata(t.GetObjectMeta()).ViaField("metadata")
ctx = config.SetValidateParameterVariablesAndWorkspaces(ctx, true)
return errs.Also(t.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec"))
}

Expand Down Expand Up @@ -362,9 +363,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 config.ValidateParameterVariablesAndWorkspaces(ctx) == true {
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 @@ -585,9 +587,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
Loading

0 comments on commit 3e446f0

Please sign in to comment.