Skip to content

Commit

Permalink
validate execution status variable
Browse files Browse the repository at this point in the history 
Adding param validation while accessing execution status along with
any other param or extra string
  • Loading branch information
@pritidesai
pritidesai committed Jan 13, 2021
1 parent 23b37ac commit b5853ab
Show file tree
Hide file tree
Showing 5 changed files with 113 additions and 71 deletions.
3 changes: 0 additions & 3 deletions docs/pipelines.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -854,9 +854,6 @@ This kind of variable can have any one of the values from the following table:

For an end-to-end example, see [`status` in a `PipelineRun`](../examples/v1beta1/pipelineruns/pipelinerun-task-execution-status.yaml).

**Note:** `$(tasks.<pipelineTask>.status)` is instantiated and available at runtime and must be used as a param value
as is without concatenating it with any other param or string, for example, this kind of usage is not validated/supported
`task status is $(tasks.<pipelineTask>.status)`.

### Known Limitations

Expand Down
64 changes: 46 additions & 18 deletions pkg/apis/pipeline/v1beta1/pipeline_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -291,40 +291,68 @@ func validatePipelineContextVariables(tasks []PipelineTask) *apis.FieldError {
return errs.Also(validatePipelineContextVariablesInParamValues(paramValues, "context\\.pipeline", pipelineContextNames))
}

func validateExecutionStatusVariables(tasks []PipelineTask, finallyTasks []PipelineTask) (errs *apis.FieldError) {
// creating a list of pipelineTask names to validate tasks.<name>.status
pipelineRunTasksContextNames := sets.String{}
// validate dag pipeline tasks, task params can not access execution status of any other task
// dag tasks cannot have param value as $(tasks.pipelineTask.status)
func validateExecutionStatusVariablesInTasks(tasks []PipelineTask) (errs *apis.FieldError) {
for idx, t := range tasks {
for _, param := range t.Params {
// validate dag pipeline tasks not accessing execution status of other pipeline task
// retrieve a list of substitution expression from a param
if ps, ok := GetVarSubstitutionExpressionsForParam(param); ok {
for _, p := range ps {
if strings.HasPrefix(p, "tasks.") && strings.HasSuffix(p, ".status") {
errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("pipeline tasks can not refer to execution status of any other pipeline task"),
"value").ViaFieldKey("params", param.Name).ViaFieldIndex("tasks", idx))
// validate tasks.pipelineTask.status if this expression is not a result reference
if !LooksLikeContainsResultRefs(ps) {
for _, p := range ps {
// check for a prefix and suffix
if strings.HasPrefix(p, "tasks.") && strings.HasSuffix(p, ".status") {
errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("pipeline tasks can not refer to execution status of any other pipeline task"),
"value").ViaFieldKey("params", param.Name).ViaFieldIndex("tasks", idx))
}
}
}
}
}
}
return errs
}

// validate finally tasks accessing execution status of a dag task specified in the pipeline
// $(tasks.pipelineTask.status) is invalid if pipelineTask is not defined as a dag task
func validateExecutionStatusVariablesInFinally(tasks []PipelineTask, finally []PipelineTask) (errs *apis.FieldError) {
// creating a list of pipelineTask names to validate tasks.<name>.status
pipelineRunTasksContextNames := sets.String{}
for _, t := range tasks {
pipelineRunTasksContextNames.Insert(t.Name)
}

// validate finally tasks accessing execution status of a dag task specified in the pipeline
var paramValues []string
for _, t := range finallyTasks {
for idx, t := range finally {
for _, param := range t.Params {
paramValues = append(paramValues, param.Value.StringVal)
paramValues = append(paramValues, param.Value.ArrayVal...)
}
}
for _, paramValue := range paramValues {
if strings.HasPrefix(stripVarSubExpression(paramValue), "tasks.") && strings.HasSuffix(stripVarSubExpression(paramValue), ".status") {
errs = errs.Also(substitution.ValidateVariablePS(paramValue, "tasks", "status", pipelineRunTasksContextNames).ViaField("value"))
// retrieve a list of substitution expression from a param
if ps, ok := GetVarSubstitutionExpressionsForParam(param); ok {
// validate tasks.pipelineTask.status if this expression is not a result reference
if !LooksLikeContainsResultRefs(ps) {
for _, p := range ps {
// check for a prefix and suffix
if strings.HasPrefix(p, "tasks.") && strings.HasSuffix(p, ".status") {
pt := strings.TrimSuffix(strings.TrimPrefix(p, "tasks."), ".status")
// report an error if the task name does not exist in the list of dag tasks
if !pipelineRunTasksContextNames.Has(pt) {
errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("pipeline task %s is not defined in the pipeline", pt),
"value").ViaFieldKey("params", param.Name).ViaFieldIndex("finally", idx))
}
}
}
}
}
}
}
return errs
}

func validateExecutionStatusVariables(tasks []PipelineTask, finallyTasks []PipelineTask) (errs *apis.FieldError) {
errs = errs.Also(validateExecutionStatusVariablesInTasks(tasks))
errs = errs.Also(validateExecutionStatusVariablesInFinally(tasks, finallyTasks))
return errs
}

func validatePipelineContextVariablesInParamValues(paramValues []string, prefix string, contextNames sets.String) (errs *apis.FieldError) {
for _, paramValue := range paramValues {
errs = errs.Also(substitution.ValidateVariableP(paramValue, prefix, contextNames).ViaField("value"))
Expand Down
68 changes: 67 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 @@ -2134,6 +2134,30 @@ func TestPipelineTasksExecutionStatus(t *testing.T) {
Name: "foo-status", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(tasks.foo.status)"},
}},
}},
}, {
name: "valid variable concatenated with extra string in finally accessing pipelineTask status",
tasks: []PipelineTask{{
Name: "foo",
}},
finalTasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "foo-status", Value: ArrayOrString{Type: ParamTypeString, StringVal: "Execution status of foo is $(tasks.foo.status)."},
}},
}},
}, {
name: "valid variable concatenated with other param in finally accessing pipelineTask status",
tasks: []PipelineTask{{
Name: "foo",
}},
finalTasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "foo-status", Value: ArrayOrString{Type: ParamTypeString, StringVal: "Execution status of $(tasks.taskname) is $(tasks.foo.status)."},
}},
}},
}, {
name: "invalid string variable in dag task accessing pipelineTask status",
tasks: []PipelineTask{{
Expand All @@ -2147,6 +2171,19 @@ func TestPipelineTasksExecutionStatus(t *testing.T) {
Message: `invalid value: pipeline tasks can not refer to execution status of any other pipeline task`,
Paths: []string{"tasks[0].params[bar-status].value"},
},
}, {
name: "invalid variable concatenated with extra string in dag task accessing pipelineTask status",
tasks: []PipelineTask{{
Name: "foo",
TaskRef: &TaskRef{Name: "foo-task"},
Params: []Param{{
Name: "bar-status", Value: ArrayOrString{Type: ParamTypeString, StringVal: "Execution status of bar is $(tasks.bar.status)"},
}},
}},
expectedError: apis.FieldError{
Message: `invalid value: pipeline tasks can not refer to execution status of any other pipeline task`,
Paths: []string{"tasks[0].params[bar-status].value"},
},
}, {
name: "invalid array variable in dag task accessing pipelineTask status",
tasks: []PipelineTask{{
Expand All @@ -2169,7 +2206,36 @@ func TestPipelineTasksExecutionStatus(t *testing.T) {
Name: "notask-status", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(tasks.notask.status)"},
}},
}},
expectedError: *apis.ErrGeneric(`non-existent variable in "$(tasks.notask.status)"`, "value"),
expectedError: apis.FieldError{
Message: `invalid value: pipeline task notask is not defined in the pipeline`,
Paths: []string{"finally[0].params[notask-status].value"},
},
}, {
name: "invalid variable concatenated with extra string in finally accessing missing pipelineTask status",
finalTasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "notask-status", Value: ArrayOrString{Type: ParamTypeString, StringVal: "Execution status of notask is $(tasks.notask.status)."},
}},
}},
expectedError: apis.FieldError{
Message: `invalid value: pipeline task notask is not defined in the pipeline`,
Paths: []string{"finally[0].params[notask-status].value"},
},
}, {
name: "invalid variable concatenated with other params in finally accessing missing pipelineTask status",
finalTasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "notask-status", Value: ArrayOrString{Type: ParamTypeString, StringVal: "Execution status of $(tasks.taskname) is $(tasks.notask.status)."},
}},
}},
expectedError: apis.FieldError{
Message: `invalid value: pipeline task notask is not defined in the pipeline`,
Paths: []string{"finally[0].params[notask-status].value"},
},
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down
16 changes: 0 additions & 16 deletions pkg/substitution/substitution.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,22 +60,6 @@ func ValidateVariableP(value, prefix string, vars sets.String) *apis.FieldError
return nil
}

func ValidateVariablePS(value, prefix string, suffix string, vars sets.String) *apis.FieldError {
if vs, present := extractVariablesFromString(value, prefix); present {
for _, v := range vs {
v = strings.TrimSuffix(v, suffix)
if !vars.Has(v) {
return &apis.FieldError{
Message: fmt.Sprintf("non-existent variable in %q", value),
// Empty path is required to make the `ViaField`, … work
Paths: []string{""},
}
}
}
}
return nil
}

// Verifies that variables matching the relevant string expressions do not reference any of the names present in vars.
func ValidateVariableProhibited(name, value, prefix, locationName, path string, vars sets.String) *apis.FieldError {
if vs, present := extractVariablesFromString(value, prefix); present {
Expand Down
33 changes: 0 additions & 33 deletions pkg/substitution/substitution_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,39 +116,6 @@ func TestValidateVariables(t *testing.T) {
}
}

func TestValidateVariablePS(t *testing.T) {
type args struct {
paramValue string
vars sets.String
}
for _, tc := range []struct {
name string
paramValue string
vars sets.String
expectedError *apis.FieldError
}{{
name: "valid pipeline task in variable",
paramValue: "--flag=$(tasks.task1.status)",
vars: sets.NewString("task1"),
expectedError: nil,
}, {
name: "undefined pipeline task",
paramValue: "--flag=$(tasks.task1.status)",
vars: sets.NewString("foo"),
expectedError: &apis.FieldError{
Message: `non-existent variable in "--flag=$(tasks.task1.status)"`,
Paths: []string{""},
},
}} {
t.Run(tc.name, func(t *testing.T) {
got := substitution.ValidateVariablePS(tc.paramValue, "tasks", "status", tc.vars)
if d := cmp.Diff(got, tc.expectedError, cmp.AllowUnexported(apis.FieldError{})); d != "" {
t.Errorf("ValidateVariablePS() error did not match expected error for %s: %s", tc.name, diff.PrintWantGot(d))
}
})
}
}

func TestApplyReplacements(t *testing.T) {
type args struct {
input string
Expand Down

0 comments on commit b5853ab

Please sign in to comment.