diff --git a/pkg/pod/status.go b/pkg/pod/status.go index 80d141e049a..7c52ca4ce32 100644 --- a/pkg/pod/status.go +++ b/pkg/pod/status.go @@ -51,6 +51,10 @@ const ( // that taskrun failed runtime validation ReasonFailedValidation = "TaskRunValidationFailed" + // ReasonTaskFailedValidation indicated that the reason for failure status is + // that task failed runtime validation + ReasonTaskFailedValidation = "TaskValidationFailed" + // ReasonExceededResourceQuota indicates that the TaskRun failed to create a pod due to // a ResourceQuota in the namespace ReasonExceededResourceQuota = "ExceededResourceQuota" diff --git a/pkg/reconciler/apiserver/apiserver.go b/pkg/reconciler/apiserver/apiserver.go new file mode 100644 index 00000000000..8ed16c20b12 --- /dev/null +++ b/pkg/reconciler/apiserver/apiserver.go @@ -0,0 +1,80 @@ +package apiserver + +import ( + "context" + "errors" + "fmt" + + "github.com/google/uuid" + v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" + clientset "github.com/tektoncd/pipeline/pkg/client/clientset/versioned" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" +) + +var ( + ErrReferencedObjectValidationFailed = errors.New("validation failed for referenced object") + ErrCouldntValidateObjectRetryable = errors.New("retryable error validating referenced object") + ErrCouldntValidateObjectPermanent = errors.New("permanent error validating referenced object") +) + +// DryRunValidate validates the obj by issuing a dry-run create request for it in the given namespace. +// This allows validating admission webhooks to process the object without actually creating it. +// obj must be a v1/v1beta1 Task or Pipeline. +func DryRunValidate(ctx context.Context, namespace string, obj runtime.Object, tekton clientset.Interface) error { + dryRunObjName := uuid.NewString() // Use a randomized name for the Pipeline/Task in case there is already another Pipeline/Task of the same name + + switch obj := obj.(type) { + case *v1.Pipeline: + dryRunObj := obj.DeepCopy() + dryRunObj.Name = dryRunObjName + dryRunObj.Namespace = namespace // Make sure the namespace is the same as the PipelineRun + if _, err := tekton.TektonV1().Pipelines(namespace).Create(ctx, dryRunObj, metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}); err != nil { + return handleDryRunCreateErr(err, obj.Name) + } + case *v1beta1.Pipeline: + dryRunObj := obj.DeepCopy() + dryRunObj.Name = dryRunObjName + dryRunObj.Namespace = namespace // Make sure the namespace is the same as the PipelineRun + if _, err := tekton.TektonV1beta1().Pipelines(namespace).Create(ctx, dryRunObj, metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}); err != nil { + return handleDryRunCreateErr(err, obj.Name) + } + + case *v1.Task: + dryRunObj := obj.DeepCopy() + dryRunObj.Name = dryRunObjName + dryRunObj.Namespace = namespace // Make sure the namespace is the same as the TaskRun + if _, err := tekton.TektonV1().Tasks(namespace).Create(ctx, dryRunObj, metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}); err != nil { + return handleDryRunCreateErr(err, obj.Name) + } + case *v1beta1.Task: + dryRunObj := obj.DeepCopy() + dryRunObj.Name = dryRunObjName + dryRunObj.Namespace = namespace // Make sure the namespace is the same as the TaskRun + if _, err := tekton.TektonV1beta1().Tasks(namespace).Create(ctx, dryRunObj, metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}); err != nil { + return handleDryRunCreateErr(err, obj.Name) + } + default: + return fmt.Errorf("unsupported object GVK %s", obj.GetObjectKind().GroupVersionKind()) + } + return nil +} + +func handleDryRunCreateErr(err error, objectName string) error { + var errType error + switch { + case apierrors.IsBadRequest(err): // Object rejected by validating webhook + errType = ErrReferencedObjectValidationFailed + case apierrors.IsInvalid(err), apierrors.IsMethodNotSupported(err): + errType = ErrCouldntValidateObjectPermanent + case apierrors.IsTimeout(err), apierrors.IsServerTimeout(err), apierrors.IsTooManyRequests(err): + errType = ErrCouldntValidateObjectRetryable + default: + // Assume unknown errors are retryable + // Additional errors can be added to the switch statements as needed + errType = ErrCouldntValidateObjectRetryable + } + return fmt.Errorf("%w %s: %s", errType, objectName, err.Error()) +} diff --git a/pkg/reconciler/apiserver/apiserver_test.go b/pkg/reconciler/apiserver/apiserver_test.go new file mode 100644 index 00000000000..4a115248ef3 --- /dev/null +++ b/pkg/reconciler/apiserver/apiserver_test.go @@ -0,0 +1,141 @@ +package apiserver_test + +import ( + "context" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" + "github.com/tektoncd/pipeline/pkg/client/clientset/versioned/fake" + "github.com/tektoncd/pipeline/pkg/reconciler/apiserver" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/validation/field" + ktesting "k8s.io/client-go/testing" +) + +func TestDryRunCreate_Valid_DifferentGVKs(t *testing.T) { + tcs := []struct { + name string + obj runtime.Object + wantErr bool + }{{ + name: "v1 task", + obj: &v1.Task{}, + }, { + name: "v1beta1 task", + obj: &v1beta1.Task{}, + }, { + name: "v1 pipeline", + obj: &v1.Pipeline{}, + }, { + name: "v1beta1 pipeline", + obj: &v1beta1.Pipeline{}, + }, { + name: "unsupported gvk", + obj: &v1beta1.ClusterTask{}, + wantErr: true, + }} + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + tektonclient := fake.NewSimpleClientset() + err := apiserver.DryRunValidate(context.Background(), "default", tc.obj, tektonclient) + if (err != nil) != tc.wantErr { + t.Errorf("wantErr was %t but got err %v", tc.wantErr, err) + } + }) + } +} + +func TestDryRunCreate_Invalid_DifferentGVKs(t *testing.T) { + tcs := []struct { + name string + obj runtime.Object + wantErr error + }{{ + name: "v1 task", + obj: &v1.Task{}, + wantErr: apiserver.ErrReferencedObjectValidationFailed, + }, { + name: "v1beta1 task", + obj: &v1beta1.Task{}, + wantErr: apiserver.ErrReferencedObjectValidationFailed, + }, { + name: "v1 pipeline", + obj: &v1.Pipeline{}, + wantErr: apiserver.ErrReferencedObjectValidationFailed, + }, { + name: "v1beta1 pipeline", + obj: &v1beta1.Pipeline{}, + wantErr: apiserver.ErrReferencedObjectValidationFailed, + }, { + name: "unsupported gvk", + obj: &v1beta1.ClusterTask{}, + wantErr: cmpopts.AnyError, + }} + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + tektonclient := fake.NewSimpleClientset() + tektonclient.PrependReactor("create", "tasks", func(action ktesting.Action) (bool, runtime.Object, error) { + return true, nil, apierrors.NewBadRequest("bad request") + }) + tektonclient.PrependReactor("create", "pipelines", func(action ktesting.Action) (bool, runtime.Object, error) { + return true, nil, apierrors.NewBadRequest("bad request") + }) + err := apiserver.DryRunValidate(context.Background(), "default", tc.obj, tektonclient) + if d := cmp.Diff(tc.wantErr, err, cmpopts.EquateErrors()); d != "" { + t.Errorf("wrong error: %s", d) + } + }) + } +} + +func TestDryRunCreate_DifferentErrTypes(t *testing.T) { + tcs := []struct { + name string + webhookErr error + wantErr error + }{{ + name: "no error", + wantErr: nil, + }, { + name: "bad request", + webhookErr: apierrors.NewBadRequest("bad request"), + wantErr: apiserver.ErrReferencedObjectValidationFailed, + }, { + name: "invalid", + webhookErr: apierrors.NewInvalid(schema.GroupKind{Group: "tekton.dev/v1", Kind: "Task"}, "task", field.ErrorList{}), + wantErr: apiserver.ErrCouldntValidateObjectPermanent, + }, { + name: "not supported", + webhookErr: apierrors.NewMethodNotSupported(schema.GroupResource{}, "create"), + wantErr: apiserver.ErrCouldntValidateObjectPermanent, + }, { + name: "timeout", + webhookErr: apierrors.NewTimeoutError("timeout", 5), + wantErr: apiserver.ErrCouldntValidateObjectRetryable, + }, { + name: "server timeout", + webhookErr: apierrors.NewServerTimeout(schema.GroupResource{}, "create", 5), + wantErr: apiserver.ErrCouldntValidateObjectRetryable, + }, { + name: "too many requests", + webhookErr: apierrors.NewTooManyRequests("foo", 5), + wantErr: apiserver.ErrCouldntValidateObjectRetryable, + }} + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + tektonclient := fake.NewSimpleClientset() + tektonclient.PrependReactor("create", "tasks", func(action ktesting.Action) (bool, runtime.Object, error) { + return true, nil, tc.webhookErr + }) + err := apiserver.DryRunValidate(context.Background(), "default", &v1.Task{}, tektonclient) + if d := cmp.Diff(tc.wantErr, err, cmpopts.EquateErrors()); d != "" { + t.Errorf("wrong error: %s", d) + } + }) + } +} diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 5412cbfd77c..760b8b9cb9c 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -40,6 +40,7 @@ import ( resolutionutil "github.com/tektoncd/pipeline/pkg/internal/resolution" "github.com/tektoncd/pipeline/pkg/pipelinerunmetrics" tknreconciler "github.com/tektoncd/pipeline/pkg/reconciler" + "github.com/tektoncd/pipeline/pkg/reconciler/apiserver" "github.com/tektoncd/pipeline/pkg/reconciler/events" "github.com/tektoncd/pipeline/pkg/reconciler/events/cloudevent" "github.com/tektoncd/pipeline/pkg/reconciler/pipeline/dag" @@ -409,10 +410,10 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1.PipelineRun, getPipel message := fmt.Sprintf("PipelineRun %s/%s awaiting remote resource", pr.Namespace, pr.Name) pr.Status.MarkRunning(ReasonResolvingPipelineRef, message) return nil - case errors.Is(err, resources.ErrReferencedPipelineValidationFailed), errors.Is(err, resources.ErrCouldntValidatePipelinePermanent): + case errors.Is(err, apiserver.ErrReferencedObjectValidationFailed), errors.Is(err, apiserver.ErrCouldntValidateObjectPermanent): pr.Status.MarkFailed(ReasonFailedValidation, err.Error()) return controller.NewPermanentError(err) - case errors.Is(err, resources.ErrCouldntValidatePipelineRetryable): + case errors.Is(err, apiserver.ErrCouldntValidateObjectRetryable): return err case err != nil: logger.Errorf("Failed to determine Pipeline spec to use for pipelinerun %s: %v", pr.Name, err) diff --git a/pkg/reconciler/pipelinerun/resources/pipelineref.go b/pkg/reconciler/pipelinerun/resources/pipelineref.go index ab166b7adc0..12396d5631a 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelineref.go +++ b/pkg/reconciler/pipelinerun/resources/pipelineref.go @@ -21,28 +21,21 @@ import ( "errors" "fmt" - "github.com/google/uuid" v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" clientset "github.com/tektoncd/pipeline/pkg/client/clientset/versioned" + "github.com/tektoncd/pipeline/pkg/reconciler/apiserver" rprp "github.com/tektoncd/pipeline/pkg/reconciler/pipelinerun/pipelinespec" "github.com/tektoncd/pipeline/pkg/remote" "github.com/tektoncd/pipeline/pkg/remote/resolution" remoteresource "github.com/tektoncd/pipeline/pkg/resolution/resource" "github.com/tektoncd/pipeline/pkg/trustedresources" - apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/kubernetes" ) -var ( - ErrReferencedPipelineValidationFailed = errors.New("validation failed for referenced Pipeline") - ErrCouldntValidatePipelineRetryable = errors.New("retryable error validating referenced Pipeline") - ErrCouldntValidatePipelinePermanent = errors.New("permanent error validating referenced Pipeline") -) - // GetPipelineFunc is a factory function that will use the given PipelineRef to return a valid GetPipeline function that // looks up the pipeline. It uses as context a k8s client, tekton client, namespace, and service account name to return // the pipeline. It knows whether it needs to look in the cluster or in a remote location to fetch the reference. @@ -150,11 +143,8 @@ func readRuntimeObjectAsPipeline(ctx context.Context, namespace string, obj runt // Validation must happen before the v1beta1 Pipeline is converted into the storage version of the API, // since validation of beta features differs between v1 and v1beta1 // TODO(#6592): Decouple API versioning from feature versioning - dryRunObj := obj.DeepCopy() - dryRunObj.Name = uuid.NewString() // Use a randomized name for the Pipeline in case there is already another Pipeline of the same name - dryRunObj.Namespace = namespace // Make sure the namespace is the same as the PipelineRun - if _, err := tekton.TektonV1beta1().Pipelines(namespace).Create(ctx, dryRunObj, metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}); err != nil { - return nil, nil, handleDryRunCreateErr(err, obj.Name) + if err := apiserver.DryRunValidate(ctx, namespace, obj, tekton); err != nil { + return nil, nil, err } p := &v1.Pipeline{ TypeMeta: metav1.TypeMeta{ @@ -170,30 +160,10 @@ func readRuntimeObjectAsPipeline(ctx context.Context, namespace string, obj runt vr := trustedresources.VerifyResource(ctx, obj, k8s, refSource, verificationPolicies) // Issue a dry-run request to create the remote Pipeline, so that it can undergo validation from validating admission webhooks // without actually creating the Pipeline on the cluster - dryRunObj := obj.DeepCopy() - dryRunObj.Name = uuid.NewString() // Use a randomized name for the Pipeline in case there is already another Pipeline of the same name - dryRunObj.Namespace = namespace // Make sure the namespace is the same as the PipelineRun - if _, err := tekton.TektonV1().Pipelines(namespace).Create(ctx, dryRunObj, metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}); err != nil { - return nil, nil, handleDryRunCreateErr(err, obj.Name) + if err := apiserver.DryRunValidate(ctx, namespace, obj, tekton); err != nil { + return nil, nil, err } return obj, &vr, nil } return nil, nil, errors.New("resource is not a pipeline") } - -func handleDryRunCreateErr(err error, objectName string) error { - var errType error - switch { - case apierrors.IsBadRequest(err): // Pipeline rejected by validating webhook - errType = ErrReferencedPipelineValidationFailed - case apierrors.IsInvalid(err), apierrors.IsMethodNotSupported(err): - errType = ErrCouldntValidatePipelinePermanent - case apierrors.IsTimeout(err), apierrors.IsServerTimeout(err), apierrors.IsTooManyRequests(err): - errType = ErrCouldntValidatePipelineRetryable - default: - // Assume unknown errors are retryable - // Additional errors can be added to the switch statements as needed - errType = ErrCouldntValidatePipelineRetryable - } - return fmt.Errorf("%w %s: %s", errType, objectName, err.Error()) -} diff --git a/pkg/reconciler/pipelinerun/resources/pipelineref_test.go b/pkg/reconciler/pipelinerun/resources/pipelineref_test.go index 15bd6e5bd10..11597f1fc05 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelineref_test.go +++ b/pkg/reconciler/pipelinerun/resources/pipelineref_test.go @@ -36,6 +36,7 @@ import ( "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/pkg/client/clientset/versioned/fake" clientset "github.com/tektoncd/pipeline/pkg/client/clientset/versioned/fake" + "github.com/tektoncd/pipeline/pkg/reconciler/apiserver" "github.com/tektoncd/pipeline/pkg/reconciler/pipelinerun/resources" ttesting "github.com/tektoncd/pipeline/pkg/reconciler/testing" "github.com/tektoncd/pipeline/pkg/trustedresources" @@ -397,7 +398,7 @@ func TestGetPipelineFunc_RemoteResolution_ValidationFailure(t *testing.T) { }) resolvedPipeline, resolvedRefSource, _, err := fn(ctx, pipelineRef.Name) - if !errors.Is(err, resources.ErrReferencedPipelineValidationFailed) { + if !errors.Is(err, apiserver.ErrReferencedObjectValidationFailed) { t.Errorf("expected RemotePipelineValidationFailed error but got none") } if resolvedPipeline != nil { diff --git a/pkg/reconciler/taskrun/resources/taskref.go b/pkg/reconciler/taskrun/resources/taskref.go index 3b72d3c3a67..c33ec14c903 100644 --- a/pkg/reconciler/taskrun/resources/taskref.go +++ b/pkg/reconciler/taskrun/resources/taskref.go @@ -26,6 +26,7 @@ import ( "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" clientset "github.com/tektoncd/pipeline/pkg/client/clientset/versioned" + "github.com/tektoncd/pipeline/pkg/reconciler/apiserver" "github.com/tektoncd/pipeline/pkg/remote" "github.com/tektoncd/pipeline/pkg/remote/resolution" remoteresource "github.com/tektoncd/pipeline/pkg/resolution/resource" @@ -108,7 +109,7 @@ func GetTaskFunc(ctx context.Context, k8s kubernetes.Interface, tekton clientset replacedParams = append(replacedParams, tr.Params...) } resolver := resolution.NewResolver(requester, owner, string(tr.Resolver), trName, namespace, replacedParams) - return resolveTask(ctx, resolver, name, kind, k8s, verificationPolicies) + return resolveTask(ctx, resolver, name, namespace, kind, k8s, tekton, verificationPolicies) } default: @@ -127,14 +128,14 @@ func GetTaskFunc(ctx context.Context, k8s kubernetes.Interface, tekton clientset // An error is returned if the remoteresource doesn't work // A VerificationResult is returned if trusted resources is enabled, VerificationResult contains the result type and err. // or the returned data isn't a valid *v1beta1.Task. -func resolveTask(ctx context.Context, resolver remote.Resolver, name string, kind v1.TaskKind, k8s kubernetes.Interface, verificationPolicies []*v1alpha1.VerificationPolicy) (*v1.Task, *v1.RefSource, *trustedresources.VerificationResult, error) { +func resolveTask(ctx context.Context, resolver remote.Resolver, name, namespace string, kind v1.TaskKind, k8s kubernetes.Interface, tekton clientset.Interface, verificationPolicies []*v1alpha1.VerificationPolicy) (*v1.Task, *v1.RefSource, *trustedresources.VerificationResult, error) { // Because the resolver will only return references with the same kind (eg ClusterTask), this will ensure we // don't accidentally return a Task with the same name but different kind. obj, refSource, err := resolver.Get(ctx, strings.TrimSuffix(strings.ToLower(string(kind)), "s"), name) if err != nil { return nil, nil, nil, err } - taskObj, vr, err := readRuntimeObjectAsTask(ctx, obj, k8s, refSource, verificationPolicies) + taskObj, vr, err := readRuntimeObjectAsTask(ctx, namespace, obj, k8s, tekton, refSource, verificationPolicies) if err != nil { return nil, nil, nil, err } @@ -150,11 +151,18 @@ func resolveTask(ctx context.Context, resolver remote.Resolver, name string, kin // A VerificationResult is returned if trusted resources is enabled, VerificationResult contains the result type and err. // v1beta1 task will be verified by trusted resources if the feature is enabled // TODO(#5541): convert v1beta1 obj to v1 once we use v1 as the stored version -func readRuntimeObjectAsTask(ctx context.Context, obj runtime.Object, k8s kubernetes.Interface, refSource *v1.RefSource, verificationPolicies []*v1alpha1.VerificationPolicy) (*v1.Task, *trustedresources.VerificationResult, error) { +func readRuntimeObjectAsTask(ctx context.Context, namespace string, obj runtime.Object, k8s kubernetes.Interface, tekton clientset.Interface, refSource *v1.RefSource, verificationPolicies []*v1alpha1.VerificationPolicy) (*v1.Task, *trustedresources.VerificationResult, error) { switch obj := obj.(type) { case *v1beta1.Task: // Verify the Task once we fetch from the remote resolution, mutating, validation and conversion of the task should happen after the verification, since signatures are based on the remote task contents vr := trustedresources.VerifyResource(ctx, obj, k8s, refSource, verificationPolicies) + // Issue a dry-run request to create the remote Task, so that it can undergo validation from validating admission webhooks + // without actually creating the Task on the cluster. + // Validation must happen before converting the Task into the storage version of the API, + // since validation differs between API versions. + if err := apiserver.DryRunValidate(ctx, namespace, obj, tekton); err != nil { + return nil, nil, err + } t := &v1.Task{ TypeMeta: metav1.TypeMeta{ Kind: "Task", @@ -167,13 +175,18 @@ func readRuntimeObjectAsTask(ctx context.Context, obj runtime.Object, k8s kubern return t, &vr, nil case *v1beta1.ClusterTask: t, err := convertClusterTaskToTask(ctx, *obj) + // Issue a dry-run request to create the remote Task, so that it can undergo validation from validating admission webhooks + // without actually creating the Task on the cluster + if err := apiserver.DryRunValidate(ctx, namespace, t, tekton); err != nil { + return nil, nil, err + } return t, nil, err case *v1.Task: vr := trustedresources.VerifyResource(ctx, obj, k8s, refSource, verificationPolicies) - // Validation of beta fields must happen before the V1 Task is converted into the storage version of the API. - // TODO(#6592): Decouple API versioning from feature versioning - if err := obj.Spec.ValidateBetaFields(ctx); err != nil { - return nil, nil, fmt.Errorf("invalid Task %s: %w", obj.GetName(), err) + // Issue a dry-run request to create the remote Task, so that it can undergo validation from validating admission webhooks + // without actually creating the Task on the cluster + if err := apiserver.DryRunValidate(ctx, namespace, obj, tekton); err != nil { + return nil, nil, err } return obj, &vr, nil } diff --git a/pkg/reconciler/taskrun/resources/taskref_test.go b/pkg/reconciler/taskrun/resources/taskref_test.go index 55ea9684da0..ae3e52e4ca5 100644 --- a/pkg/reconciler/taskrun/resources/taskref_test.go +++ b/pkg/reconciler/taskrun/resources/taskref_test.go @@ -35,15 +35,18 @@ import ( "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/pkg/client/clientset/versioned/fake" + "github.com/tektoncd/pipeline/pkg/reconciler/apiserver" "github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources" "github.com/tektoncd/pipeline/pkg/trustedresources" "github.com/tektoncd/pipeline/test" "github.com/tektoncd/pipeline/test/diff" "github.com/tektoncd/pipeline/test/parse" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" fakek8s "k8s.io/client-go/kubernetes/fake" + ktesting "k8s.io/client-go/testing" "knative.dev/pkg/logging" ) @@ -485,14 +488,6 @@ func TestGetTaskFunc_RemoteResolution(t *testing.T) { taskYAMLString, }, "\n"), wantTask: parse.MustParseV1Task(t, taskYAMLString), - }, { - name: "v1beta1 task with beta features", - taskYAML: strings.Join([]string{ - "kind: Task", - "apiVersion: tekton.dev/v1beta1", - taskYAMLStringWithBetaFeatures, - }, "\n"), - wantTask: parse.MustParseV1Task(t, taskYAMLStringWithBetaFeatures), }, { name: "v1beta1 cluster task", taskYAML: strings.Join([]string{ @@ -509,14 +504,6 @@ func TestGetTaskFunc_RemoteResolution(t *testing.T) { taskYAMLString, }, "\n"), wantTask: parse.MustParseV1Task(t, taskYAMLString), - }, { - name: "v1 task with beta features", - taskYAML: strings.Join([]string{ - "kind: Task", - "apiVersion: tekton.dev/v1", - taskYAMLStringWithBetaFeatures, - }, "\n"), - wantErr: true, }, { name: "v1 task without defaults", taskYAML: strings.Join([]string{ @@ -537,7 +524,8 @@ func TestGetTaskFunc_RemoteResolution(t *testing.T) { ServiceAccountName: "default", }, } - fn := resources.GetTaskFunc(ctx, nil, nil, requester, tr, tr.Spec.TaskRef, "", "default", "default", nil /*VerificationPolicies*/) + tektonclient := fake.NewSimpleClientset() + fn := resources.GetTaskFunc(ctx, nil, tektonclient, requester, tr, tr.Spec.TaskRef, "", "default", "default", nil /*VerificationPolicies*/) resolvedTask, resolvedRefSource, _, err := fn(ctx, taskRef.Name) if tc.wantErr { @@ -561,6 +549,67 @@ func TestGetTaskFunc_RemoteResolution(t *testing.T) { } } +func TestGetTaskFunc_RemoteResolution_ValidationFailure(t *testing.T) { + ctx := context.Background() + cfg := config.FromContextOrDefaults(ctx) + ctx = config.ToContext(ctx, cfg) + taskRef := &v1.TaskRef{ResolverRef: v1.ResolverRef{Resolver: "git"}} + + testcases := []struct { + name string + taskYAML string + }{{ + name: "invalid v1beta1 task", + taskYAML: strings.Join([]string{ + "kind: Task", + "apiVersion: tekton.dev/v1beta1", + taskYAMLString, + }, "\n"), + }, { + name: "invalid v1beta1 clustertask", + taskYAML: strings.Join([]string{ + "kind: ClusterTask", + "apiVersion: tekton.dev/v1beta1", + taskYAMLString, + }, "\n"), + }, { + name: "invalid v1 task", + taskYAML: strings.Join([]string{ + "kind: Task", + "apiVersion: tekton.dev/v1", + taskYAMLString, + }, "\n"), + }} + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + resolved := test.NewResolvedResource([]byte(tc.taskYAML), nil /* annotations */, sampleRefSource.DeepCopy(), nil /* data error */) + requester := test.NewRequester(resolved, nil) + tektonclient := fake.NewSimpleClientset() + fn := resources.GetTaskFunc(ctx, nil, tektonclient, requester, &v1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default"}, + Spec: v1.TaskRunSpec{ + TaskRef: taskRef, + }, + }, taskRef, "trName", "default", "default", nil /*VerificationPolicies*/) + + tektonclient.PrependReactor("create", "tasks", func(action ktesting.Action) (bool, runtime.Object, error) { + return true, nil, apierrors.NewBadRequest("bad request") + }) + + resolvedTask, resolvedRefSource, _, err := fn(ctx, taskRef.Name) + if !errors.Is(err, apiserver.ErrReferencedObjectValidationFailed) { + t.Errorf("expected ReferencedObjectValidationFailed error but got none") + } + if resolvedTask != nil { + t.Errorf("expected nil Task but was %v", resolvedTask) + } + if resolvedRefSource != nil { + t.Errorf("expected nil refSource but was %s", resolvedRefSource) + } + }) + } +} + func TestGetTaskFunc_RemoteResolution_ReplacedParams(t *testing.T) { ctx := context.Background() cfg := config.FromContextOrDefaults(ctx) @@ -609,7 +658,8 @@ func TestGetTaskFunc_RemoteResolution_ReplacedParams(t *testing.T) { }}, }, } - fn := resources.GetTaskFunc(ctx, nil, nil, requester, tr, tr.Spec.TaskRef, "", "default", "default", nil /*VerificationPolicies*/) + tektonclient := fake.NewSimpleClientset() + fn := resources.GetTaskFunc(ctx, nil, tektonclient, requester, tr, tr.Spec.TaskRef, "", "default", "default", nil /*VerificationPolicies*/) resolvedTask, resolvedRefSource, _, err := fn(ctx, taskRef.Name) if err != nil { @@ -795,7 +845,7 @@ func TestGetTaskFunc_V1beta1Task_VerifyNoError(t *testing.T) { ServiceAccountName: "default", }, } - fn := resources.GetTaskFunc(ctx, k8sclient, tektonclient, tc.requester, tr, tr.Spec.TaskRef, "", "default", "default", tc.policies) + fn := resources.GetTaskFunc(ctx, k8sclient, tektonclient, tc.requester, tr, tr.Spec.TaskRef, "", "trusted-resources", "default", tc.policies) resolvedTask, refSource, vr, err := fn(ctx, taskRef.Name) @@ -923,7 +973,7 @@ func TestGetTaskFunc_V1beta1Task_VerifyError(t *testing.T) { ServiceAccountName: "default", }, } - fn := resources.GetTaskFunc(ctx, k8sclient, tektonclient, tc.requester, tr, tr.Spec.TaskRef, "", "default", "default", vps) + fn := resources.GetTaskFunc(ctx, k8sclient, tektonclient, tc.requester, tr, tr.Spec.TaskRef, "", "trusted-resources", "default", vps) _, _, vr, _ := fn(ctx, taskRef.Name) if !errors.Is(vr.Err, tc.expectedErr) { @@ -1274,20 +1324,6 @@ spec: echo "hello world!" ` -var taskYAMLStringWithBetaFeatures = ` -metadata: - name: foo -spec: - steps: - - name: step1 - image: ubuntu - script: | - echo "hello world!" - results: - - name: array-result - type: array -` - var remoteTaskYamlWithoutDefaults = ` metadata: name: simple diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index edc59e257f1..aa8becd044d 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -39,6 +39,7 @@ import ( resolutionutil "github.com/tektoncd/pipeline/pkg/internal/resolution" podconvert "github.com/tektoncd/pipeline/pkg/pod" tknreconciler "github.com/tektoncd/pipeline/pkg/reconciler" + "github.com/tektoncd/pipeline/pkg/reconciler/apiserver" "github.com/tektoncd/pipeline/pkg/reconciler/events" "github.com/tektoncd/pipeline/pkg/reconciler/events/cloudevent" "github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources" @@ -338,6 +339,11 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1.TaskRun) (*v1.TaskSpec, message := fmt.Sprintf("TaskRun %s/%s awaiting remote resource", tr.Namespace, tr.Name) tr.Status.MarkResourceOngoing(v1.TaskRunReasonResolvingTaskRef, message) return nil, nil, err + case errors.Is(err, apiserver.ErrReferencedObjectValidationFailed), errors.Is(err, apiserver.ErrCouldntValidateObjectPermanent): + tr.Status.MarkResourceFailed(podconvert.ReasonTaskFailedValidation, err) + return nil, nil, controller.NewPermanentError(err) + case errors.Is(err, apiserver.ErrCouldntValidateObjectRetryable): + return nil, nil, err case err != nil: logger.Errorf("Failed to determine Task spec to use for taskrun %s: %v", tr.Name, err) if resources.IsGetTaskErrTransient(err) { diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index de8392d3eec..17ea0678843 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -63,12 +63,15 @@ import ( "github.com/tektoncd/pipeline/test/parse" "go.opentelemetry.io/otel/trace" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" k8sapierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" k8sruntimeschema "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/validation/field" fakekubeclientset "k8s.io/client-go/kubernetes/fake" ktesting "k8s.io/client-go/testing" "k8s.io/client-go/tools/record" @@ -1919,6 +1922,93 @@ spec: } } +func TestReconcile_InvalidRemoteTask(t *testing.T) { + namespace := "foo" + trName := "test-task-run-success" + trs := []*v1.TaskRun{parse.MustParseV1TaskRun(t, ` +metadata: + name: test-task-run-success + namespace: foo +spec: + taskRef: + resolver: bar +`)} + ts := parse.MustParseV1Task(t, ` +metadata: + name: test-task + namespace: foo +spec: + steps: + - image: busybox + script: echo hello +`) + + taskBytes, err := yaml.Marshal(ts) + if err != nil { + t.Fatal("failed to marshal task", err) + } + taskReq := getResolvedResolutionRequest(t, "bar", taskBytes, "foo", trName) + + tcs := []struct { + name string + webhookErr error + wantPermanentErr bool + wantFailed bool + }{{ + name: "webhook validation fails: invalid object", + webhookErr: apierrors.NewBadRequest("bad request"), + wantPermanentErr: true, + wantFailed: true, + }, { + name: "webhook validation fails with permanent error", + webhookErr: apierrors.NewInvalid(schema.GroupKind{Group: "tekton.dev/v1", Kind: "TaskRun"}, "taskrun", field.ErrorList{}), + wantPermanentErr: true, + wantFailed: true, + }, { + name: "webhook validation fails: retryable", + webhookErr: apierrors.NewTimeoutError("timeout", 5), + wantPermanentErr: false, + wantFailed: false, + }} + for _, tc := range tcs { + d := test.Data{ + TaskRuns: trs, + ConfigMaps: []*corev1.ConfigMap{ + { + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.Namespace()}, + Data: map[string]string{ + "enable-api-fields": "beta", + }, + }, + }, + ResolutionRequests: []*resolutionv1beta1.ResolutionRequest{&taskReq}, + } + testAssets, cancel := getTaskRunController(t, d) + defer cancel() + c := testAssets.Controller + clients := testAssets.Clients + // Create an error when the Pipeline client attempts to create Tasks + clients.Pipeline.PrependReactor("create", "tasks", func(action ktesting.Action) (bool, runtime.Object, error) { + return true, nil, tc.webhookErr + }) + err = c.Reconciler.Reconcile(testAssets.Ctx, fmt.Sprintf("%s/%s", namespace, trName)) + if tc.wantPermanentErr != controller.IsPermanentError(err) { + t.Errorf("expected permanent error: %t but got %s", tc.wantPermanentErr, err) + } + reconciledRun, err := clients.Pipeline.TektonV1().TaskRuns(namespace).Get(testAssets.Ctx, trName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Somehow had error getting reconciled run out of fake client: %s", err) + } + + if tc.wantFailed && reconciledRun.Status.GetCondition(apis.ConditionSucceeded).Reason != podconvert.ReasonTaskFailedValidation { + t.Errorf("Expected TaskRun to have reason FailedValidation, but condition reason is %s", reconciledRun.Status.GetCondition(apis.ConditionSucceeded)) + } + if !tc.wantFailed && reconciledRun.Status.GetCondition(apis.ConditionSucceeded).IsFalse() { + t.Errorf("Expected TaskRun to not be failed but has condition status false") + } + } +} + func TestReconcileTaskRunWithPermanentError(t *testing.T) { noTaskRun := parse.MustParseV1TaskRun(t, ` metadata: