fix: do not fail PipelineRun when TaskRef reconciles with retryable err

With this change, no `ErrCouldntValidateObjectRetryable` returned by
DryRunValidate results in a PipelineRun being marked as failed.
Previously, the only errors which were actually retried were naming
conflicts, server-timeouts, timeouts, and too-many-requests.
Unknown errors were marked as "Retryable", however they still caused the
PipelineRun to fail since `resolutioncommon.IsErrTransient` did not
check if the err was of type `ErrCouldntValidateObjectRetryable`

Author: aThorp96

pkg/reconciler/pipelinerun/pipelinerun_test.go

@@ -8513,7 +8513,7 @@ func (prt PipelineRunTest) reconcileRun(namespace, pipelineRunName string, wantE
 			prt.Test.Fatalf("Expected an error to be returned by Reconcile, got nil instead")
 		}
 		if controller.IsPermanentError(reconcileError) != permanentError {
-			prt.Test.Fatalf("Expected the error to be permanent: %v but got: %v", permanentError, controller.IsPermanentError(reconcileError))
+			prt.Test.Fatalf("Expected the error to be permanent: %v but got: %v: %v", permanentError, controller.IsPermanentError(reconcileError), reconcileError)
 		}
 	} else if ok, _ := controller.IsRequeueKey(reconcileError); ok {
 		// This is normal, it happens for timeouts when we otherwise successfully reconcile.
@@ -9297,6 +9297,95 @@ spec:
 	th.CheckPipelineRunConditionStatusAndReason(t, updatedPipelineRun.Status, corev1.ConditionFalse, v1.PipelineRunReasonCouldntGetTask.String())
 }
 
+// TestReconcileWithFailingTaskResolver checks that a PipelineRun with a failing Resolver
+// field for a Task creates a ResolutionRequest object for that Resolver's type, and
+// that when the request fails, the PipelineRun fails.
+func TestReconcileWithTaskResolutionDryRunValidaitonErrors(t *testing.T) {
+	pr := parse.MustParseV1PipelineRun(t, `
+metadata:
+  name: pr
+  namespace: default
+spec:
+  pipelineSpec:
+    tasks:
+    - name: some-task
+      taskRef:
+        resolver: foobar
+  taskRunTemplate:
+    serviceAccountName: default
+`)
+	remoteTask := parse.MustParseV1Task(t, `
+metadata:
+  name: some-task
+  namespace: default
+`)
+	taskBytes, err := yaml.Marshal(remoteTask)
+	if err != nil {
+		t.Fatal("fail to marshal task", err)
+	}
+	for _, testCase := range []struct {
+		name             string
+		apiErr           error
+		isPermanentError bool
+	}{
+		{"leader change", errors.New("etcdserver: leader changed"), false},
+		{"kubeapi timeout", apierrors.NewTimeoutError("roses are red, tulips are long, this dang ol' request done took too long", 5), false},
+		{"kubeapi throttling", apierrors.NewTooManyRequestsError("over nine thousand requests"), false},
+		{"unknown error", errors.New("the tranformogrifier is not tranformogrifying"), false},
+		{"bad request", apierrors.NewBadRequest("you've thrown off the emperors groove"), true},
+		{"actual validation error", apierrors.NewInvalid(schema.GroupKind{Group: "tekton.dev/v1", Kind: "Task"}, "some-invalid-task", field.ErrorList{}), true},
+	} {
+		t.Run(testCase.name, func(t *testing.T) {
+			taskReq := getResolvedResolutionRequest(t, "foobar", taskBytes, "default", "pr-some-task")
+
+			d := test.Data{
+				PipelineRuns: []*v1.PipelineRun{pr},
+				ServiceAccounts: []*corev1.ServiceAccount{{
+					ObjectMeta: metav1.ObjectMeta{Name: pr.Spec.TaskRunTemplate.ServiceAccountName, Namespace: "foo"},
+				}},
+				ConfigMaps:         th.NewAlphaFeatureFlagsConfigMapInSlice(),
+				ResolutionRequests: []*resolutionv1beta1.ResolutionRequest{&taskReq},
+			}
+
+			prt := newPipelineRunTest(t, d)
+			defer prt.Cancel()
+			clients := prt.TestAssets.Clients
+
+			failDryRunCreation := true
+
+			clients.Pipeline.PrependReactor("create", "tasks", func(action ktesting.Action) (bool, runtime.Object, error) {
+				return failDryRunCreation, nil, testCase.apiErr
+			})
+
+			err := prt.TestAssets.Controller.Reconciler.Reconcile(prt.TestAssets.Ctx, "default/pr")
+			if err == nil {
+				t.Errorf("Expected error returned from reconcile after failing to cancel TaskRun but saw none!")
+			}
+
+			if testCase.isPermanentError && !controller.IsPermanentError(err) {
+				t.Errorf("Expected permanent error reconciling PipelineRun but got non-permanent error: %v", err)
+			} else if !testCase.isPermanentError && controller.IsPermanentError(err) {
+				t.Errorf("Expected non-permanent error reconciling PipelineRun but got permanent error: %v", err)
+			}
+
+			// Check that the PipelineRun is still running with correct error message
+			updatedPipelineRun, err := clients.Pipeline.TektonV1().PipelineRuns("default").Get(prt.TestAssets.Ctx, "pr", metav1.GetOptions{})
+			if err != nil {
+				t.Fatalf("Somehow had error getting reconciled run out of fake client: %s", err)
+			}
+
+			// The PipelineRun should not be cancelled since the error was retryable
+			condition := updatedPipelineRun.Status.GetCondition(apis.ConditionSucceeded)
+			if testCase.isPermanentError && condition.IsUnknown() {
+				t.Errorf("Expected PipelineRun to be failed but succeeded condition is %v", condition.Status)
+			}
+			if !testCase.isPermanentError && !condition.IsUnknown() {
+				t.Errorf("Expected PipelineRun to still be running but succeeded condition is %v", condition.Status)
+			}
+		})
+	}
+}
+
 // TestReconcileWithTaskResolver checks that a PipelineRun with a populated Resolver
 // field for a Task creates a ResolutionRequest object for that Resolver's type, and
 // that when the request is successfully resolved the PipelineRun begins running.

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go

@@ -54,11 +54,15 @@ type TaskSkipStatus struct {
 // TaskNotFoundError indicates that the resolution failed because a referenced Task couldn't be retrieved
 type TaskNotFoundError struct {
 	Name string
-	Msg  string
+	Err  error
 }
 
 func (e *TaskNotFoundError) Error() string {
-	return fmt.Sprintf("Couldn't retrieve Task %q: %s", e.Name, e.Msg)
+	return fmt.Sprintf("Couldn't retrieve Task %q: %s", e.Name, e.Err.Error())
+}
+
+func (e *TaskNotFoundError) Unwrap() error {
+	return e.Err
 }
 
 // ResolvedPipelineTask contains a PipelineTask and its associated child PipelineRun(s) (Pipelines-in-Pipelines), TaskRun(s) or CustomRuns, if they exist.
@@ -820,7 +824,7 @@ func resolveTask(
 				}
 				return rt, &TaskNotFoundError{
 					Name: name,
-					Msg:  err.Error(),
+					Err:  err,
 				}
 			default:
 				spec := t.Spec

pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go

@@ -30,6 +30,7 @@ import (
 	"github.com/tektoncd/pipeline/pkg/apis/config"
 	v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
+	"github.com/tektoncd/pipeline/pkg/reconciler/apiserver"
 	"github.com/tektoncd/pipeline/pkg/reconciler/pipeline/dag"
 	"github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources"
 	resolutioncommon "github.com/tektoncd/pipeline/pkg/resolution/common"
@@ -2746,16 +2747,32 @@ func TestResolvePipelineRun_TransientError(t *testing.T) {
 		TaskRef: &v1.TaskRef{Name: "task"},
 	}
 
-	getTask := getTaskFn(nil, apierrors.NewTimeoutError("some dang ol' timeout", 5))
-	getTaskRun := getTaskRunFn(nil)
-	pr := v1.PipelineRun{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: "pipelinerun",
-		},
+	testCases := []struct {
+		name         string
+		transientErr error
+	}{
+		{"kubeapi timeout error", apierrors.NewTimeoutError("some dang ol' timeout", 5)},
+		{"retryable resolution error", apiserver.ErrCouldntValidateObjectRetryable},
+		{"any knonw-retryable validation error", apiserver.ErrCouldntValidateObjectRetryable},
 	}
-	_, err := ResolvePipelineTask(t.Context(), pr, nopGetPipelineRun, getTask, getTaskRun, nopGetCustomRun, pt, nil)
-	if !resolutioncommon.IsErrTransient(err) {
-		t.Error("Transient error while getting Task did not result in a transient error")
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			getTask := getTaskFn(nil, testCase.transientErr)
+			getTaskRun := getTaskRunFn(nil)
+			pr := v1.PipelineRun{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "pipelinerun",
+				},
+			}
+			_, err := ResolvePipelineTask(t.Context(), pr, nopGetPipelineRun, getTask, getTaskRun, nopGetCustomRun, pt, nil)
+
+			// This is really testing is that if the getTask function give ResolvePipelineTask a transient error,
+			// ResolvePipielineTask does not swallow that error
+			if !resolutioncommon.IsErrTransient(err) {
+				t.Error("Transient error while getting Task did not result in a transient error")
+			}
+		})
 	}
 }
 

pkg/resolution/common/errors.go

@@ -23,6 +23,7 @@ import (
 	"slices"
 	"strings"
 
+	"github.com/tektoncd/pipeline/pkg/reconciler/apiserver"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 )
 
@@ -153,7 +154,7 @@ func ReasonError(err error) (string, error) {
 // IsErrTransient returns true if an error returned by GetTask/GetStepAction is retryable.
 func IsErrTransient(err error) bool {
 	switch {
-	case apierrors.IsConflict(err), apierrors.IsServerTimeout(err), apierrors.IsTimeout(err), apierrors.IsTooManyRequests(err):
+	case apierrors.IsConflict(err), apierrors.IsServerTimeout(err), apierrors.IsTimeout(err), apierrors.IsTooManyRequests(err), errors.Is(err, apiserver.ErrCouldntValidateObjectRetryable):
 		return true
 	default:
 		return slices.ContainsFunc([]string{errEtcdLeaderChange, context.DeadlineExceeded.Error()}, func(s string) bool {