validate execution status variable

Adding param validation while accessing execution status along with
any other param or extra string. Also, avoiding task results validation as
it follows very similar pattern with $(tasks.taskname.results.status) where
"status" is a result of some task.

(cherry picked from commit 2ec2b86)

docs/pipelines.md

@@ -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
 

pkg/apis/pipeline/v1beta1/pipeline_types.go

@@ -272,6 +272,15 @@ func (l PipelineTaskList) Items() []dag.Task {
  return tasks
 }
 
+// Names returns a set of pipeline task names from the given list of pipeline tasks
+func (l PipelineTaskList) Names() sets.String {
+ names := sets.String{}
+ for _, pt := range l {
+ names.Insert(pt.Name)
+ }
+ return names
+}
+
 // PipelineTaskParam is used to provide arbitrary string parameters to a Task.
 type PipelineTaskParam struct {
  Name string `json:"name"`

pkg/apis/pipeline/v1beta1/pipeline_types_test.go

@@ -0,0 +1,41 @@
+/*
+Copyright 2021 The Tekton Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1beta1
+
+import (
+ "testing"
+
+ "github.com/tektoncd/pipeline/test/diff"
+
+ "github.com/google/go-cmp/cmp"
+
+ "k8s.io/apimachinery/pkg/util/sets"
+)
+
+func TestPipelineTaskList_Names(t *testing.T) {
+ tasks := []PipelineTask{
+ {Name: "task-1"},
+ {Name: "task-2"},
+ }
+ expectedTaskNames := sets.String{}
+ expectedTaskNames.Insert("task-1")
+ expectedTaskNames.Insert("task-2")
+ actualTaskNames := PipelineTaskList(tasks).Names()
+ if d := cmp.Diff(expectedTaskNames, actualTaskNames); d != "" {
+ t.Fatalf("Failed to get list of pipeline task names, diff: %s", diff.PrintWantGot(d))
+ }
+}

pkg/apis/pipeline/v1beta1/pipeline_validation.go

@@ -291,40 +291,72 @@ 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{}
+func containsExecutionStatusRef(p string) bool {
+ if strings.HasPrefix(p, "tasks.") && strings.HasSuffix(p, ".status") {
+ return true
+ }
+ return false
+}
+
+// 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 if it contains context variable accessing execution status - $(tasks.taskname.status)
+ if containsExecutionStatusRef(p) {
+ 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))
+ }
  }
  }
  }
  }
- pipelineRunTasksContextNames.Insert(t.Name)
  }
+ return errs
+}
 
- // validate finally tasks accessing execution status of a dag task specified in the pipeline
- var paramValues []string
- for _, t := range finallyTasks {
+// 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
+ ptNames := PipelineTaskList(tasks).Names()
+ 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 if it contains context variable accessing execution status - $(tasks.taskname.status)
+ if containsExecutionStatusRef(p) {
+ // strip tasks. and .status from tasks.taskname.status to further verify task name
+ 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 !ptNames.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"))

pkg/apis/pipeline/v1beta1/pipeline_validation_test.go

@@ -2134,6 +2134,39 @@ func TestPipelineTasksExecutionStatus(t *testing.T) {
  Name: "foo-status", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(tasks.foo.status)"},
  }},
  }},
+ }, {
+ name: "valid task result reference with status as a variable must not cause validation failure",
+ tasks: []PipelineTask{{
+ Name: "bar",
+ TaskRef: &TaskRef{Name: "bar-task"},
+ Params: []Param{{
+ Name: "foo-status", Value: ArrayOrString{Type: ParamTypeString, StringVal: "$(tasks.foo.results.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{{
@@ -2147,6 +2180,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{{
@@ -2169,7 +2215,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) {

pkg/substitution/substitution.go

@@ -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 {

pkg/substitution/substitution_test.go

@@ -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