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 @tekton-robot
chitrangpatel authored and tekton-robot committed Aug 11, 2022
1 parent 6902b8d commit 57aa030
Show file tree
Hide file tree
Showing 9 changed files with 353 additions and 12 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: 71 additions & 1 deletion 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 @@ -68,6 +82,58 @@ func TestTaskValidate(t *testing.T) {
}
}

func TestTaskSpecValidatePropagatedParamsAndWorkspaces(t *testing.T) {
type fields struct {
Params []v1.ParamSpec
Steps []v1.Step
StepTemplate *v1.StepTemplate
Workspaces []v1.WorkspaceDeclaration
Results []v1.TaskResult
}
tests := []struct {
name string
fields fields
}{{
name: "propagating params valid step with script",
fields: fields{
Steps: []v1.Step{{
Name: "propagatingparams",
Image: "my-image",
Script: `
#!/usr/bin/env bash
$(params.message)`,
}},
},
}, {
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)"},
}},
},
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ts := &v1.TaskSpec{
Params: tt.fields.Params,
Steps: tt.fields.Steps,
StepTemplate: tt.fields.StepTemplate,
Workspaces: tt.fields.Workspaces,
Results: tt.fields.Results,
}
ctx := config.EnableAlphaAPIFields(context.Background())
ts.SetDefaults(ctx)
ctx = config.SetValidateParameterVariablesAndWorkspaces(ctx, false)
if err := ts.Validate(ctx); err != nil {
t.Errorf("TaskSpec.Validate() = %v", err)
}
})
}
}

func TestTaskSpecValidate(t *testing.T) {
type fields struct {
Params []v1.ParamSpec
Expand Down Expand Up @@ -1197,6 +1263,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 +1368,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 +1420,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 +1514,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 +1599,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
75 changes: 74 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 @@ -67,6 +67,20 @@ func TestTaskValidate(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{Name: "task"},
},
wc: apis.WithinDelete,
}, {
name: "valid task",
t: &v1beta1.Task{
ObjectMeta: metav1.ObjectMeta{Name: "task"},
Spec: v1beta1.TaskSpec{
Steps: []v1beta1.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 @@ -82,6 +96,60 @@ func TestTaskValidate(t *testing.T) {
}
}

func TestTaskSpecValidatePropagatedParamsAndWorkspaces(t *testing.T) {
type fields struct {
Params []v1beta1.ParamSpec
Resources *v1beta1.TaskResources
Steps []v1beta1.Step
StepTemplate *v1beta1.StepTemplate
Workspaces []v1beta1.WorkspaceDeclaration
Results []v1beta1.TaskResult
}
tests := []struct {
name string
fields fields
}{{
name: "propagating params valid step with script",
fields: fields{
Steps: []v1beta1.Step{{
Name: "propagatingparams",
Image: "my-image",
Script: `
#!/usr/bin/env bash
$(params.message)`,
}},
},
}, {
name: "propagating params valid step with args",
fields: fields{
Steps: []v1beta1.Step{{
Name: "propagatingparams",
Image: "my-image",
Command: []string{"$(params.command)"},
Args: []string{"$(params.message)"},
}},
},
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ts := &v1beta1.TaskSpec{
Params: tt.fields.Params,
Resources: tt.fields.Resources,
Steps: tt.fields.Steps,
StepTemplate: tt.fields.StepTemplate,
Workspaces: tt.fields.Workspaces,
Results: tt.fields.Results,
}
ctx := config.EnableAlphaAPIFields(context.Background())
ts.SetDefaults(ctx)
ctx = config.SetValidateParameterVariablesAndWorkspaces(ctx, false)
if err := ts.Validate(ctx); err != nil {
t.Errorf("TaskSpec.Validate() = %v", err)
}
})
}
}

func TestTaskSpecValidate(t *testing.T) {
type fields struct {
Params []v1beta1.ParamSpec
Expand Down Expand Up @@ -1305,6 +1373,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 @@ -1352,6 +1421,7 @@ func TestStepAndSidecarWorkspaces(t *testing.T) {
}
ctx := config.EnableAlphaAPIFields(context.Background())
ts.SetDefaults(ctx)
ctx = config.SetValidateParameterVariablesAndWorkspaces(ctx, false)
if err := ts.Validate(ctx); err != nil {
t.Errorf("TaskSpec.Validate() = %v", err)
}
Expand Down Expand Up @@ -1409,6 +1479,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 @@ -1460,6 +1531,7 @@ func TestStepOnError(t *testing.T) {
}
ctx := context.Background()
ts.SetDefaults(ctx)
ctx = config.SetValidateParameterVariablesAndWorkspaces(ctx, true)
err := ts.Validate(ctx)
if tt.expectedError == nil && err != nil {
t.Errorf("TaskSpec.Validate() = %v", err)
Expand Down Expand Up @@ -1553,8 +1625,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 @@ -1638,6 +1710,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
Loading

0 comments on commit 57aa030

Please sign in to comment.