Skip to content

Commit

Permalink
Move validation from pipelinespec to pipelinerunspec when propaga…
Browse files Browse the repository at this point in the history 
…ting parameters

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 15, 2022
1 parent 4aa2379 commit 06a2887
Show file tree
Hide file tree
Showing 17 changed files with 306 additions and 38 deletions.
12 changes: 6 additions & 6 deletions pkg/apis/config/contexts.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ 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
// validatePropagatedVariables is used for deciding whether to validate or skip parameters and workspaces inside the contect.Context.
type validatePropagatedVariables string

// WithinSubstituted is used to note that it is calling within
// the context of a substitute variable operation.
Expand All @@ -37,12 +37,12 @@ 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)
// SkipValidationDueToPropagatedParametersAndWorkspaces sets the context to skip validation of parameters when embedded vs referenced to true or false.
func SkipValidationDueToPropagatedParametersAndWorkspaces(ctx context.Context, skip bool) context.Context {
return context.WithValue(ctx, validatePropagatedVariables("ValidatePropagatedParameterVariablesAndWorkspaces"), !skip)
}

// ValidateParameterVariablesAndWorkspaces indicates if validation of paramater variables and workspaces should be conducted.
func ValidateParameterVariablesAndWorkspaces(ctx context.Context) bool {
return ctx.Value(validateEmbeddedVariables("ValidateParameterVariablesAndWorkspaces")) == true
return ctx.Value(validatePropagatedVariables("ValidatePropagatedParameterVariablesAndWorkspaces")) == true
}
5 changes: 4 additions & 1 deletion pkg/apis/pipeline/v1/pipeline_types_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ func TestPipelineTask_ValidateRegularTask_Success(t *testing.T) {
cfg.FeatureFlags.EnableAPIFields = config.AlphaAPIFields
}
ctx = config.ToContext(ctx, cfg)
ctx = config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false)
err := tt.tasks.validateTask(ctx)
if err != nil {
t.Errorf("PipelineTask.validateTask() returned error for valid pipeline task: %v", err)
Expand Down Expand Up @@ -282,7 +283,8 @@ func TestPipelineTask_ValidateRegularTask_Failure(t *testing.T) {
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := tt.task.validateTask(context.Background())
ctx := config.SkipValidationDueToPropagatedParametersAndWorkspaces(context.Background(), false)
err := tt.task.validateTask(ctx)
if err == nil {
t.Error("PipelineTask.validateTask() did not return error for invalid pipeline task")
}
Expand Down Expand Up @@ -317,6 +319,7 @@ func TestPipelineTask_Validate_Failure(t *testing.T) {
if tt.wc != nil {
ctx = tt.wc(ctx)
}
ctx = config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false)
err := tt.p.Validate(ctx)
if err == nil {
t.Error("PipelineTask.Validate() did not return error for invalid pipeline task")
Expand Down
2 changes: 2 additions & 0 deletions pkg/apis/pipeline/v1/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/reconciler/pipeline/dag"
"github.com/tektoncd/pipeline/pkg/substitution"
Expand All @@ -38,6 +39,7 @@ func (p *Pipeline) Validate(ctx context.Context) *apis.FieldError {
if apis.IsInDelete(ctx) {
return nil
}
ctx = config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false)
return errs.Also(p.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec"))
}

Expand Down
20 changes: 16 additions & 4 deletions pkg/apis/pipeline/v1/pipeline_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,8 @@ func TestPipelineSpec_Validate_Failure(t *testing.T) {
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := tt.ps.Validate(context.Background())
ctx := config.SkipValidationDueToPropagatedParametersAndWorkspaces(context.Background(), false)
err := tt.ps.Validate(ctx)
if err == nil {
t.Errorf("PipelineSpec.Validate() did not return error for invalid pipelineSpec")
}
Expand All @@ -573,7 +574,8 @@ func TestPipelineSpec_Validate_Failure_CycleDAG(t *testing.T) {
Name: "bar", TaskRef: &TaskRef{Name: "bar-task"}, RunAfter: []string{"foo"},
}},
}
err := ps.Validate(context.Background())
ctx := config.SkipValidationDueToPropagatedParametersAndWorkspaces(context.Background(), false)
err := ps.Validate(ctx)
if err == nil {
t.Errorf("PipelineSpec.Validate() did not return error for invalid pipelineSpec: %s", name)
}
Expand Down Expand Up @@ -640,7 +642,8 @@ func TestValidatePipelineTasks_Failure(t *testing.T) {
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := ValidatePipelineTasks(context.Background(), tt.tasks, tt.finalTasks)
ctx := config.SkipValidationDueToPropagatedParametersAndWorkspaces(context.Background(), false)
err := ValidatePipelineTasks(ctx, tt.tasks, tt.finalTasks)
if err == nil {
t.Error("ValidatePipelineTasks() did not return error for invalid pipeline tasks")
}
Expand Down Expand Up @@ -1135,6 +1138,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 @@ -1419,6 +1423,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 @@ -1442,6 +1447,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 @@ -1463,6 +1469,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 @@ -1484,6 +1491,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 @@ -1511,10 +1519,14 @@ 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",
}}
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
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1/task_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +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)
ctx = config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false)
return errs.Also(t.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec"))
}

Expand Down
12 changes: 6 additions & 6 deletions pkg/apis/pipeline/v1/task_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ func TestTaskSpecValidatePropagatedParamsAndWorkspaces(t *testing.T) {
}
ctx := config.EnableAlphaAPIFields(context.Background())
ts.SetDefaults(ctx)
ctx = config.SetValidateParameterVariablesAndWorkspaces(ctx, false)
ctx = config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, true)
if err := ts.Validate(ctx); err != nil {
t.Errorf("TaskSpec.Validate() = %v", err)
}
Expand Down Expand Up @@ -1263,7 +1263,7 @@ func TestTaskSpecValidateError(t *testing.T) {
}
ctx := config.EnableAlphaAPIFields(context.Background())
ts.SetDefaults(ctx)
ctx = config.SetValidateParameterVariablesAndWorkspaces(ctx, true)
ctx = config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false)
err := ts.Validate(ctx)
if err == nil {
t.Fatalf("Expected an error, got nothing for %v", ts)
Expand Down Expand Up @@ -1368,7 +1368,7 @@ func TestStepAndSidecarWorkspacesErrors(t *testing.T) {

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

if tt.requiredVersion != version && err == nil {
Expand Down Expand Up @@ -1599,7 +1599,7 @@ func TestSubstitutedContext(t *testing.T) {
}
ctx := context.Background()
ts.SetDefaults(ctx)
ctx = config.SetValidateParameterVariablesAndWorkspaces(ctx, true)
ctx = config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false)
if tt.fields.SubstitutionContext {
ctx = config.WithinSubstituted(ctx)
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1beta1/cluster_task_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +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)
ctx = config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false)
return errs.Also(t.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec"))
}
6 changes: 5 additions & 1 deletion pkg/apis/pipeline/v1beta1/pipeline_types_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,7 @@ func TestPipelineTask_ValidateRegularTask_Success(t *testing.T) {
cfg.FeatureFlags.EnableTektonOCIBundles = true
}
ctx = config.ToContext(ctx, cfg)
ctx = config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false)
err := tt.tasks.validateTask(ctx)
if err != nil {
t.Errorf("PipelineTask.validateTask() returned error for valid pipeline task: %v", err)
Expand Down Expand Up @@ -344,7 +345,8 @@ func TestPipelineTask_ValidateRegularTask_Failure(t *testing.T) {
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := tt.task.validateTask(context.Background())
ctx := config.SkipValidationDueToPropagatedParametersAndWorkspaces(context.Background(), false)
err := tt.task.validateTask(ctx)
if err == nil {
t.Error("PipelineTask.validateTask() did not return error for invalid pipeline task")
}
Expand Down Expand Up @@ -390,6 +392,7 @@ func TestPipelineTask_Validate_Failure(t *testing.T) {
if tt.wc != nil {
ctx = tt.wc(ctx)
}
ctx = config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false)
err := tt.p.Validate(ctx)
if err == nil {
t.Error("PipelineTask.Validate() did not return error for invalid pipeline task")
Expand Down Expand Up @@ -735,6 +738,7 @@ func TestPipelineTaskList_Validate(t *testing.T) {
ctx = tt.wc(ctx)
}
taskNames := sets.String{}
ctx = config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false)
err := tt.tasks.Validate(ctx, taskNames, tt.path)
if tt.expectedError != nil && err == nil {
t.Error("PipelineTaskList.Validate() did not return error for invalid pipeline tasks")
Expand Down
2 changes: 2 additions & 0 deletions 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 All @@ -39,6 +40,7 @@ func (p *Pipeline) Validate(ctx context.Context) *apis.FieldError {
return nil
}
errs := validate.ObjectMetadata(p.GetObjectMeta()).ViaField("metadata")
ctx = config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false)
return errs.Also(p.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec"))
}

Expand Down
22 changes: 18 additions & 4 deletions pkg/apis/pipeline/v1beta1/pipeline_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -685,7 +685,8 @@ func TestPipelineSpec_Validate_Failure(t *testing.T) {
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := tt.ps.Validate(context.Background())
ctx := config.SkipValidationDueToPropagatedParametersAndWorkspaces(context.Background(), false)
err := tt.ps.Validate(ctx)
if err == nil {
t.Errorf("PipelineSpec.Validate() did not return error for invalid pipelineSpec")
}
Expand All @@ -705,7 +706,8 @@ func TestPipelineSpec_Validate_Failure_CycleDAG(t *testing.T) {
Name: "bar", TaskRef: &TaskRef{Name: "bar-task"}, RunAfter: []string{"foo"},
}},
}
err := ps.Validate(context.Background())
ctx := config.SkipValidationDueToPropagatedParametersAndWorkspaces(context.Background(), false)
err := ps.Validate(ctx)
if err == nil {
t.Errorf("PipelineSpec.Validate() did not return error for invalid pipelineSpec: %s", name)
}
Expand Down Expand Up @@ -772,7 +774,8 @@ func TestValidatePipelineTasks_Failure(t *testing.T) {
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := ValidatePipelineTasks(context.Background(), tt.tasks, tt.finalTasks)
ctx := config.SkipValidationDueToPropagatedParametersAndWorkspaces(context.Background(), false)
err := ValidatePipelineTasks(ctx, tt.tasks, tt.finalTasks)
if err == nil {
t.Error("ValidatePipelineTasks() did not return error for invalid pipeline tasks")
}
Expand Down Expand Up @@ -1673,6 +1676,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 @@ -2004,6 +2008,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 @@ -2027,6 +2032,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 @@ -2048,6 +2054,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 @@ -2069,6 +2076,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 @@ -2096,6 +2104,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 @@ -2119,10 +2128,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 Expand Up @@ -3270,6 +3283,7 @@ func TestMatrixIncompatibleAPIVersions(t *testing.T) {
ctx := config.ToContext(context.Background(), cfg)

ps.SetDefaults(ctx)
ctx = config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false)
err := ps.Validate(ctx)

if tt.requiredVersion != version && err == nil {
Expand Down
Loading

0 comments on commit 06a2887

Please sign in to comment.