diff --git a/docs/variables.md b/docs/variables.md index ace4bd5a348..beab1c9bc9e 100644 --- a/docs/variables.md +++ b/docs/variables.md @@ -160,6 +160,7 @@ variable via `resources.inputs..` 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` | diff --git a/examples/v1beta1/pipelineruns/ignore-step-error.yaml b/examples/v1beta1/pipelineruns/ignore-step-error.yaml index ef08855bd80..c92a428d398 100644 --- a/examples/v1beta1/pipelineruns/ignore-step-error.yaml +++ b/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 diff --git a/pkg/apis/pipeline/v1beta1/task_validation.go b/pkg/apis/pipeline/v1beta1/task_validation.go index 335ecd52c49..1d20dc35b58 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation.go +++ b/pkg/apis/pipeline/v1beta1/task_validation.go @@ -242,8 +242,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) +} diff --git a/pkg/apis/pipeline/v1beta1/task_validation_test.go b/pkg/apis/pipeline/v1beta1/task_validation_test.go index 3d69ad6ae00..1b00616a33d 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/task_validation_test.go @@ -1424,24 +1424,36 @@ 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", @@ -1449,26 +1461,41 @@ func TestStepOnError(t *testing.T) { }}, 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) 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 diff --git a/pkg/container/step_replacements.go b/pkg/container/step_replacements.go index 31265a17e16..846b12b00fe 100644 --- a/pkg/container/step_replacements.go +++ b/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) } diff --git a/pkg/container/step_replacements_test.go b/pkg/container/step_replacements_test.go index 52a7420776c..05a7b805fb7 100644 --- a/pkg/container/step_replacements_test.go +++ b/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{ diff --git a/pkg/pod/entrypoint.go b/pkg/pod/entrypoint.go index e9d8767fa8b..e1e57d4ad98 100644 --- a/pkg/pod/entrypoint.go +++ b/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 { diff --git a/pkg/pod/entrypoint_test.go b/pkg/pod/entrypoint_test.go index 6f2167041f5..fd6a8370287 100644 --- a/pkg/pod/entrypoint_test.go +++ b/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) {