parameterize steps[].onError

In addition to the static values supported for onError, allow specifying
valid values as a task parameter for example, onError: $(params.CONTINUE).
Also, validate the specified value is supported otherwise return an error
for an invalid value.

docs/variables.md

@@ -160,6 +160,7 @@ variable via `resources.inputs.<resourceName>.<variableName>` or
 | `Task` | `spec.steps[].command` |
 | `Task` | `spec.steps[].args` |
 | `Task` | `spec.steps[].script` |
+| `Task` | `spec.steps[].onError` |
 | `Task` | `spec.steps[].env.value` |
 | `Task` | `spec.steps[].env.valuefrom.secretkeyref.name` |
 | `Task` | `spec.steps[].env.valuefrom.secretkeyref.key` |

examples/v1beta1/pipelineruns/ignore-step-error.yaml

@@ -4,14 +4,24 @@ metadata:
   generateName: pipelinerun-with-failing-step-
 spec:
   serviceAccountName: 'default'
+  params:
+    - name: CONTINUE
+      value: "continue"
   pipelineSpec:
+    params:
+      - name: CONTINUE
     tasks:
       - name: task1
+        params:
+          - name: CONTINUE
+            value: "$(params.CONTINUE)"
         taskSpec:
+          params:
+          - name: CONTINUE
           steps:
             # not really doing anything here, just a hurdle to test the "ignore step error"
             - image: alpine
-              onError: continue
+              onError: $(params.CONTINUE)
               name: exit-with-1
               script: |
                 exit 1

pkg/apis/pipeline/v1beta1/task_validation.go

@@ -243,8 +243,9 @@ func validateStep(ctx context.Context, s Step, names sets.String) (errs *apis.Fi
 		}
 	}
 
+	// validate static values in onError if specified - onError can only be set to continue or stopAndFail
 	if s.OnError != "" {
-		if s.OnError != "continue" && s.OnError != "stopAndFail" {
+		if !isParamRefs(s.OnError) && s.OnError != "continue" && s.OnError != "stopAndFail" {
 			errs = errs.Also(&apis.FieldError{
 				Message: fmt.Sprintf("invalid value: %v", s.OnError),
 				Paths:   []string{"onError"},
@@ -602,6 +603,7 @@ func validateStepVariables(ctx context.Context, step Step, prefix string, vars s
 		errs = errs.Also(validateTaskVariable(v.MountPath, prefix, vars).ViaField("MountPath").ViaFieldIndex("volumeMount", i))
 		errs = errs.Also(validateTaskVariable(v.SubPath, prefix, vars).ViaField("SubPath").ViaFieldIndex("volumeMount", i))
 	}
+	errs = errs.Also(validateTaskVariable(step.OnError, prefix, vars).ViaField("onError"))
 	return errs
 }
 
@@ -620,3 +622,9 @@ func validateTaskNoArrayReferenced(value, prefix string, arrayNames sets.String)
 func validateTaskArraysIsolated(value, prefix string, arrayNames sets.String) *apis.FieldError {
 	return substitution.ValidateVariableIsolatedP(value, prefix, arrayNames)
 }
+
+// isParamRefs attempts to check if a specified string looks like it contains any parameter reference
+// This is useful to make sure the specified value looks like a Parameter Reference before performing any strict validation
+func isParamRefs(s string) bool {
+	return strings.HasPrefix(s, "$("+ParamsPrefix)
+}

pkg/apis/pipeline/v1beta1/task_validation_test.go

@@ -1495,52 +1495,79 @@ func TestStepAndSidecarWorkspacesErrors(t *testing.T) {
 func TestStepOnError(t *testing.T) {
 	tests := []struct {
 		name          string
+		params        []v1beta1.ParamSpec
 		steps         []v1beta1.Step
 		expectedError *apis.FieldError
 	}{{
-		name: "valid step - valid onError usage - set to continue - alpha API",
+		name: "valid step - valid onError usage - set to continue",
 		steps: []v1beta1.Step{{
 			OnError: "continue",
 			Image:   "image",
 			Args:    []string{"arg"},
 		}},
 	}, {
-		name: "valid step - valid onError usage - set to stopAndFail - alpha API",
+		name: "valid step - valid onError usage - set to stopAndFail",
 		steps: []v1beta1.Step{{
 			OnError: "stopAndFail",
 			Image:   "image",
 			Args:    []string{"arg"},
 		}},
 	}, {
-		name: "invalid step - onError set to invalid value - alpha API",
+		name: "valid step - valid onError usage - set to a task parameter",
+		params: []v1beta1.ParamSpec{{
+			Name:    "CONTINUE",
+			Default: &v1beta1.ArrayOrString{Type: v1beta1.ParamTypeString, StringVal: "continue"},
+		}},
+		steps: []v1beta1.Step{{
+			OnError: "$(params.CONTINUE)",
+			Image:   "image",
+			Args:    []string{"arg"},
+		}},
+	}, {
+		name: "invalid step - onError set to invalid value",
 		steps: []v1beta1.Step{{
 			OnError: "onError",
 			Image:   "image",
 			Args:    []string{"arg"},
 		}},
 		expectedError: &apis.FieldError{
 			Message: fmt.Sprintf("invalid value: onError"),
-			Paths:   []string{"onError"},
+			Paths:   []string{"steps[0].onError"},
 			Details: "Task step onError must be either continue or stopAndFail",
 		},
+	}, {
+		name: "invalid step - invalid onError usage - set to a parameter which does not exist in the task",
+		steps: []v1beta1.Step{{
+			OnError: "$(params.CONTINUE)",
+			Image:   "image",
+			Args:    []string{"arg"},
+		}},
+		expectedError: &apis.FieldError{
+			Message: "non-existent variable in \"$(params.CONTINUE)\"",
+			Paths:   []string{"steps[0].onError"},
+		},
 	}}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			ts := &v1beta1.TaskSpec{
-				Steps: tt.steps,
+				Params: tt.params,
+				Steps:  tt.steps,
 			}
 			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)
-			} else if tt.expectedError != nil && err == nil {
-				t.Errorf("TaskSpec.Validate() = %v", err)
+				t.Errorf("No error expected from TaskSpec.Validate() but got = %v", err)
+			} else if tt.expectedError != nil {
+				if err == nil {
+					t.Errorf("Expected error from TaskSpec.Validate() = %v, but got none", tt.expectedError)
+				} else if d := cmp.Diff(tt.expectedError.Error(), err.Error()); d != "" {
+					t.Errorf("returned error from TaskSpec.Validate() does not match with the expected error: %s", diff.PrintWantGot(d))
+				}
 			}
 		})
 	}
-
 }
 
 // TestIncompatibleAPIVersions exercises validation of fields that

pkg/container/step_replacements.go

@@ -24,6 +24,7 @@ import (
 // ApplyStepReplacements applies variable interpolation on a Step.
 func ApplyStepReplacements(step *v1beta1.Step, stringReplacements map[string]string, arrayReplacements map[string][]string) {
 	step.Script = substitution.ApplyReplacements(step.Script, stringReplacements)
+	step.OnError = substitution.ApplyReplacements(step.OnError, stringReplacements)
 	if step.StdoutConfig != nil {
 		step.StdoutConfig.Path = substitution.ApplyReplacements(step.StdoutConfig.Path, stringReplacements)
 	}

pkg/container/step_replacements_test.go

@@ -42,6 +42,7 @@ func TestApplyStepReplacements(t *testing.T) {
 		Command:    []string{"$(array.replace.me)"},
 		Args:       []string{"$(array.replace.me)"},
 		WorkingDir: "$(replace.me)",
+		OnError:    "$(replace.me)",
 		EnvFrom: []corev1.EnvFromSource{{
 			ConfigMapRef: &corev1.ConfigMapEnvSource{
 				LocalObjectReference: corev1.LocalObjectReference{
@@ -92,6 +93,7 @@ func TestApplyStepReplacements(t *testing.T) {
 		Command:    []string{"val1", "val2"},
 		Args:       []string{"val1", "val2"},
 		WorkingDir: "replaced!",
+		OnError:    "replaced!",
 		EnvFrom: []corev1.EnvFromSource{{
 			ConfigMapRef: &corev1.ConfigMapEnvSource{
 				LocalObjectReference: corev1.LocalObjectReference{

pkg/pod/entrypoint.go

@@ -28,6 +28,7 @@ import (
 
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
+	"github.com/tektoncd/pipeline/pkg/entrypoint"
 	"gomodules.xyz/jsonpatch/v2"
 	corev1 "k8s.io/api/core/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
@@ -137,6 +138,10 @@ func orderContainers(commonExtraEntrypointArgs []string, steps []corev1.Containe
 		if taskSpec != nil {
 			if taskSpec.Steps != nil && len(taskSpec.Steps) >= i+1 {
 				if taskSpec.Steps[i].OnError != "" {
+					if taskSpec.Steps[i].OnError != entrypoint.ContinueOnError && taskSpec.Steps[i].OnError != entrypoint.FailOnError {
+						return nil, fmt.Errorf("task step onError must be either %s or %s but it is set to an invalid value %s",
+							entrypoint.ContinueOnError, entrypoint.FailOnError, taskSpec.Steps[i].OnError)
+					}
 					argsForEntrypoint = append(argsForEntrypoint, "-on_error", taskSpec.Steps[i].OnError)
 				}
 				if taskSpec.Steps[i].Timeout != nil {

pkg/pod/entrypoint_test.go

@@ -18,6 +18,7 @@ package pod
 
 import (
 	"context"
+	"errors"
 	"testing"
 	"time"
 
@@ -339,14 +340,6 @@ func TestEntryPointSingleResultsSingleStep(t *testing.T) {
 }
 
 func TestEntryPointOnError(t *testing.T) {
-	taskSpec := v1beta1.TaskSpec{
-		Steps: []v1beta1.Step{{
-			OnError: entrypoint.ContinueOnError,
-		}, {
-			OnError: entrypoint.FailOnError,
-		}},
-	}
-
 	steps := []corev1.Container{{
 		Name:    "failing-step",
 		Image:   "step-1",
@@ -357,42 +350,76 @@ func TestEntryPointOnError(t *testing.T) {
 		Command: []string{"cmd"},
 	}}
 
-	want := []corev1.Container{{
-		Name:    "failing-step",
-		Image:   "step-1",
-		Command: []string{entrypointBinary},
-		Args: []string{
-			"-wait_file", "/tekton/downward/ready",
-			"-wait_file_content",
-			"-post_file", "/tekton/run/0/out",
-			"-termination_path", "/tekton/termination",
-			"-step_metadata_dir", "/tekton/run/0/status",
-			"-on_error", "continue",
-			"-entrypoint", "cmd", "--",
+	for _, tc := range []struct {
+		desc           string
+		taskSpec       v1beta1.TaskSpec
+		wantContainers []corev1.Container
+		err            error
+	}{{
+		taskSpec: v1beta1.TaskSpec{
+			Steps: []v1beta1.Step{{
+				OnError: entrypoint.ContinueOnError,
+			}, {
+				OnError: entrypoint.FailOnError,
+			}},
 		},
-		VolumeMounts:           []corev1.VolumeMount{downwardMount},
-		TerminationMessagePath: "/tekton/termination",
+		wantContainers: []corev1.Container{{
+			Name:    "failing-step",
+			Image:   "step-1",
+			Command: []string{entrypointBinary},
+			Args: []string{
+				"-wait_file", "/tekton/downward/ready",
+				"-wait_file_content",
+				"-post_file", "/tekton/run/0/out",
+				"-termination_path", "/tekton/termination",
+				"-step_metadata_dir", "/tekton/run/0/status",
+				"-on_error", "continue",
+				"-entrypoint", "cmd", "--",
+			},
+			VolumeMounts:           []corev1.VolumeMount{downwardMount},
+			TerminationMessagePath: "/tekton/termination",
+		}, {
+			Name:    "passing-step",
+			Image:   "step-2",
+			Command: []string{entrypointBinary},
+			Args: []string{
+				"-wait_file", "/tekton/run/0/out",
+				"-post_file", "/tekton/run/1/out",
+				"-termination_path", "/tekton/termination",
+				"-step_metadata_dir", "/tekton/run/1/status",
+				"-on_error", "stopAndFail",
+				"-entrypoint", "cmd", "--",
+			},
+			TerminationMessagePath: "/tekton/termination",
+		}},
 	}, {
-		Name:    "passing-step",
-		Image:   "step-2",
-		Command: []string{entrypointBinary},
-		Args: []string{
-			"-wait_file", "/tekton/run/0/out",
-			"-post_file", "/tekton/run/1/out",
-			"-termination_path", "/tekton/termination",
-			"-step_metadata_dir", "/tekton/run/1/status",
-			"-on_error", "stopAndFail",
-			"-entrypoint", "cmd", "--",
+		taskSpec: v1beta1.TaskSpec{
+			Steps: []v1beta1.Step{{
+				OnError: "invalid-on-error",
+			}},
 		},
-		TerminationMessagePath: "/tekton/termination",
-	}}
-	got, err := orderContainers([]string{}, steps, &taskSpec, nil, true)
-	if err != nil {
-		t.Fatalf("orderContainers: %v", err)
-	}
-	if d := cmp.Diff(want, got); d != "" {
-		t.Errorf("Diff %s", diff.PrintWantGot(d))
+		err: errors.New("task step onError must be either continue or stopAndFail but it is set to an invalid value invalid-on-error"),
+	}} {
+		t.Run(tc.desc, func(t *testing.T) {
+			got, err := orderContainers([]string{}, steps, &tc.taskSpec, nil, true)
+			if len(tc.wantContainers) == 0 {
+				if err == nil {
+					t.Fatalf("expected an error for an invalid value for onError but received none")
+				}
+				if d := cmp.Diff(tc.err.Error(), err.Error()); d != "" {
+					t.Errorf("Diff %s", diff.PrintWantGot(d))
+				}
+			} else {
+				if err != nil {
+					t.Fatalf("orderContainers: %v", err)
+				}
+				if d := cmp.Diff(tc.wantContainers, got); d != "" {
+					t.Errorf("Diff %s", diff.PrintWantGot(d))
+				}
+			}
+		})
 	}
+
 }
 
 func TestEntryPointStepOutputConfigs(t *testing.T) {