From 8382ac9761bcdf43927afe9266406f209d3cb248 Mon Sep 17 00:00:00 2001 From: Chitrang Patel Date: Tue, 9 Jul 2024 15:16:54 -0400 Subject: [PATCH] DryRunValidate returns the mutated object Prior to this, when validating remotely resolved resources, we performed a dry run. When a dry run is performed, the resource is passed through the mutating webhook admission controllers followed by the validating webhook controllers. We only validated the object and inserted the spec of the original object instead of the mutated object. This means that if users have mutating webhook controllers on their cluster then remotely resolved resources are not mutated when passed to the taskrun/pipelinerun. This PR fixes that. Fixes https://github.com/tektoncd/pipeline/issues/8071 --- pkg/reconciler/apiserver/apiserver.go | 44 ++++++---- pkg/reconciler/apiserver/apiserver_test.go | 42 ++++++---- .../pipelinerun/resources/pipelineref.go | 40 +++++---- pkg/reconciler/taskrun/resources/taskref.go | 82 +++++++++++++------ 4 files changed, 134 insertions(+), 74 deletions(-) diff --git a/pkg/reconciler/apiserver/apiserver.go b/pkg/reconciler/apiserver/apiserver.go index 6367d6da719..336774a5706 100644 --- a/pkg/reconciler/apiserver/apiserver.go +++ b/pkg/reconciler/apiserver/apiserver.go @@ -25,7 +25,7 @@ var ( // 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 { +func DryRunValidate(ctx context.Context, namespace string, obj runtime.Object, tekton clientset.Interface) (runtime.Object, 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) { @@ -33,49 +33,61 @@ func DryRunValidate(ctx context.Context, namespace string, obj runtime.Object, t 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) + mutatedObj, err := tekton.TektonV1().Pipelines(namespace).Create(ctx, dryRunObj, metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}) + if err != nil { + return nil, handleDryRunCreateErr(err, obj.Name) } + return mutatedObj, nil 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) + mutatedObj, err := tekton.TektonV1beta1().Pipelines(namespace).Create(ctx, dryRunObj, metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}) + if err != nil { + return nil, handleDryRunCreateErr(err, obj.Name) } - + return mutatedObj, nil 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) + mutatedObj, err := tekton.TektonV1().Tasks(namespace).Create(ctx, dryRunObj, metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}) + if err != nil { + return nil, handleDryRunCreateErr(err, obj.Name) } + return mutatedObj, nil 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) + mutatedObj, err := tekton.TektonV1beta1().Tasks(namespace).Create(ctx, dryRunObj, metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}) + if err != nil { + return nil, handleDryRunCreateErr(err, obj.Name) } + return mutatedObj, nil case *v1alpha1.StepAction: dryRunObj := obj.DeepCopy() dryRunObj.Name = dryRunObjName dryRunObj.Namespace = namespace // Make sure the namespace is the same as the StepAction - if _, err := tekton.TektonV1alpha1().StepActions(namespace).Create(ctx, dryRunObj, metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}); err != nil { - return handleDryRunCreateErr(err, obj.Name) + mutatedObj, err := tekton.TektonV1alpha1().StepActions(namespace).Create(ctx, dryRunObj, metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}) + if err != nil { + return nil, handleDryRunCreateErr(err, obj.Name) } + return mutatedObj, nil + case *v1beta1.StepAction: dryRunObj := obj.DeepCopy() dryRunObj.Name = dryRunObjName dryRunObj.Namespace = namespace // Make sure the namespace is the same as the StepAction - if _, err := tekton.TektonV1beta1().StepActions(namespace).Create(ctx, dryRunObj, metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}); err != nil { - return handleDryRunCreateErr(err, obj.Name) + mutatedObj, err := tekton.TektonV1beta1().StepActions(namespace).Create(ctx, dryRunObj, metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}) + if err != nil { + return nil, handleDryRunCreateErr(err, obj.Name) } + return mutatedObj, nil + default: - return fmt.Errorf("unsupported object GVK %s", obj.GetObjectKind().GroupVersionKind()) + return nil, fmt.Errorf("unsupported object GVK %s", obj.GetObjectKind().GroupVersionKind()) } - return nil } func handleDryRunCreateErr(err error, objectName string) error { diff --git a/pkg/reconciler/apiserver/apiserver_test.go b/pkg/reconciler/apiserver/apiserver_test.go index 1ed2aa3eac6..06f91abd8c5 100644 --- a/pkg/reconciler/apiserver/apiserver_test.go +++ b/pkg/reconciler/apiserver/apiserver_test.go @@ -12,6 +12,7 @@ import ( "github.com/tektoncd/pipeline/pkg/client/clientset/versioned/fake" "github.com/tektoncd/pipeline/pkg/reconciler/apiserver" apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/validation/field" @@ -23,36 +24,47 @@ func TestDryRunCreate_Valid_DifferentGVKs(t *testing.T) { name string obj runtime.Object wantErr bool + wantObj runtime.Object }{{ - name: "v1 task", - obj: &v1.Task{}, + name: "v1 task", + obj: &v1.Task{}, + wantObj: &v1.Task{}, }, { - name: "v1beta1 task", - obj: &v1beta1.Task{}, + name: "v1beta1 task", + obj: &v1beta1.Task{}, + wantObj: &v1beta1.Task{}, }, { - name: "v1 pipeline", - obj: &v1.Pipeline{}, + name: "v1 pipeline", + obj: &v1.Pipeline{}, + wantObj: &v1.Pipeline{}, }, { - name: "v1beta1 pipeline", - obj: &v1beta1.Pipeline{}, + name: "v1beta1 pipeline", + obj: &v1beta1.Pipeline{}, + wantObj: &v1beta1.Pipeline{}, }, { - name: "v1alpha1 stepaction", - obj: &v1alpha1.StepAction{}, + name: "v1alpha1 stepaction", + obj: &v1alpha1.StepAction{}, + wantObj: &v1alpha1.StepAction{}, }, { - name: "v1beta1 stepaction", - obj: &v1beta1.StepAction{}, + name: "v1beta1 stepaction", + obj: &v1beta1.StepAction{}, + wantObj: &v1beta1.StepAction{}, }, { name: "unsupported gvk", obj: &v1beta1.ClusterTask{}, wantErr: true, + wantObj: nil, }} for _, tc := range tcs { t.Run(tc.name, func(t *testing.T) { tektonclient := fake.NewSimpleClientset() - err := apiserver.DryRunValidate(context.Background(), "default", tc.obj, tektonclient) + mutatedObj, 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) } + if d := cmp.Diff(tc.wantObj, mutatedObj, cmpopts.IgnoreFields(metav1.ObjectMeta{}, "Name", "Namespace")); d != "" { + t.Errorf("wrong object: %s", d) + } }) } } @@ -103,7 +115,7 @@ func TestDryRunCreate_Invalid_DifferentGVKs(t *testing.T) { tektonclient.PrependReactor("create", "stepactions", func(action ktesting.Action) (bool, runtime.Object, error) { return true, nil, apierrors.NewBadRequest("bad request") }) - err := apiserver.DryRunValidate(context.Background(), "default", tc.obj, tektonclient) + _, 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) } @@ -150,7 +162,7 @@ func TestDryRunCreate_DifferentErrTypes(t *testing.T) { 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) + _, 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/resources/pipelineref.go b/pkg/reconciler/pipelinerun/resources/pipelineref.go index ccbcdbe79cb..17ae1cb0047 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelineref.go +++ b/pkg/reconciler/pipelinerun/resources/pipelineref.go @@ -156,21 +156,26 @@ func readRuntimeObjectAsPipeline(ctx context.Context, namespace string, obj runt obj.ObjectMeta.OwnerReferences = nil // Verify the Pipeline once we fetch from the remote resolution, mutating, validation and conversion of the pipeline should happen after the verification, since signatures are based on the remote pipeline contents 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. - if err := apiserver.DryRunValidate(ctx, namespace, obj, tekton); err != nil { + // 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 + o, err := apiserver.DryRunValidate(ctx, namespace, obj, tekton) + if err != nil { return nil, nil, err } - p := &v1.Pipeline{ - TypeMeta: metav1.TypeMeta{ - Kind: "Pipeline", - APIVersion: "tekton.dev/v1", - }, - } - if err := obj.ConvertTo(ctx, p); err != nil { - return nil, nil, fmt.Errorf("failed to convert v1beta1 obj %s into v1 Pipeline", obj.GetObjectKind().GroupVersionKind().String()) + switch mutatedPipeline := o.(type) { + case *v1beta1.Pipeline: + mutatedPipeline.ObjectMeta = obj.ObjectMeta + p := &v1.Pipeline{ + TypeMeta: metav1.TypeMeta{ + Kind: "Pipeline", + APIVersion: "tekton.dev/v1", + }, + } + if err := mutatedPipeline.ConvertTo(ctx, p); err != nil { + return nil, nil, fmt.Errorf("failed to convert v1beta1 obj %s into v1 Pipeline", mutatedPipeline.GetObjectKind().GroupVersionKind().String()) + } + return p, &vr, nil } - return p, &vr, nil case *v1.Pipeline: // Cleanup object from things we don't care about // FIXME: extract this in a function @@ -179,12 +184,15 @@ func readRuntimeObjectAsPipeline(ctx context.Context, namespace string, obj runt // Avoid forgetting to add it in the future when there is a v2 version, causing similar problems. obj.SetDefaults(ctx) 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 - if err := apiserver.DryRunValidate(ctx, namespace, obj, tekton); err != nil { + o, err := apiserver.DryRunValidate(ctx, namespace, obj, tekton) + if err != nil { return nil, nil, err } - return obj, &vr, nil + switch mutatedPipeline := o.(type) { + case *v1.Pipeline: + mutatedPipeline.ObjectMeta = obj.ObjectMeta + return mutatedPipeline, &vr, nil + } } return nil, nil, errors.New("resource is not a pipeline") } diff --git a/pkg/reconciler/taskrun/resources/taskref.go b/pkg/reconciler/taskrun/resources/taskref.go index 6a5ad08945b..a42cabbf690 100644 --- a/pkg/reconciler/taskrun/resources/taskref.go +++ b/pkg/reconciler/taskrun/resources/taskref.go @@ -235,29 +235,39 @@ func resolveStepAction(ctx context.Context, resolver remote.Resolver, name, name // Cleanup object from things we don't care about // FIXME: extract this in a function obj.ObjectMeta.OwnerReferences = nil - if err := apiserver.DryRunValidate(ctx, namespace, obj, tekton); err != nil { + o, err := apiserver.DryRunValidate(ctx, namespace, obj, tekton) + if err != nil { return nil, nil, err } - return obj, refSource, nil + switch mutatedStepAction := o.(type) { + case *v1beta1.StepAction: + mutatedStepAction.ObjectMeta = obj.ObjectMeta + return mutatedStepAction, refSource, nil + } case *v1alpha1.StepAction: obj.SetDefaults(ctx) // Cleanup object from things we don't care about // FIXME: extract this in a function obj.ObjectMeta.OwnerReferences = nil - if err := apiserver.DryRunValidate(ctx, namespace, obj, tekton); err != nil { - return nil, nil, err - } - v1BetaStepAction := v1beta1.StepAction{ - TypeMeta: metav1.TypeMeta{ - Kind: "StepAction", - APIVersion: "tekton.dev/v1beta1", - }, - } - err := obj.ConvertTo(ctx, &v1BetaStepAction) + o, err := apiserver.DryRunValidate(ctx, namespace, obj, tekton) if err != nil { return nil, nil, err } - return &v1BetaStepAction, refSource, nil + switch mutatedStepAction := o.(type) { + case *v1alpha1.StepAction: + mutatedStepAction.ObjectMeta = obj.ObjectMeta + v1BetaStepAction := v1beta1.StepAction{ + TypeMeta: metav1.TypeMeta{ + Kind: "StepAction", + APIVersion: "tekton.dev/v1beta1", + }, + } + err := mutatedStepAction.ConvertTo(ctx, &v1BetaStepAction) + if err != nil { + return nil, nil, err + } + return &v1BetaStepAction, refSource, nil + } } return nil, nil, errors.New("resource is not a StepAction") } @@ -282,31 +292,44 @@ func readRuntimeObjectAsTask(ctx context.Context, namespace string, obj runtime. 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. - if err := apiserver.DryRunValidate(ctx, namespace, obj, tekton); err != nil { + o, err := apiserver.DryRunValidate(ctx, namespace, obj, tekton) + if err != nil { return nil, nil, err } - t := &v1.Task{ - TypeMeta: metav1.TypeMeta{ - Kind: "Task", - APIVersion: "tekton.dev/v1", - }, - } - if err := obj.ConvertTo(ctx, t); err != nil { - return nil, nil, fmt.Errorf("failed to convert obj %s into Pipeline", obj.GetObjectKind().GroupVersionKind().String()) + switch mutatedTask := o.(type) { + case *v1beta1.Task: + t := &v1.Task{ + TypeMeta: metav1.TypeMeta{ + Kind: "Task", + APIVersion: "tekton.dev/v1", + }, + } + mutatedTask.ObjectMeta = obj.ObjectMeta + if err := mutatedTask.ConvertTo(ctx, t); err != nil { + return nil, nil, fmt.Errorf("failed to convert obj %s into Pipeline", mutatedTask.GetObjectKind().GroupVersionKind().String()) + } + return t, &vr, nil } - return t, &vr, nil case *v1beta1.ClusterTask: obj.SetDefaults(ctx) // Cleanup object from things we don't care about // FIXME: extract this in a function obj.ObjectMeta.OwnerReferences = nil t, err := convertClusterTaskToTask(ctx, *obj) + if err != nil { + return nil, nil, 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, t, tekton); err != nil { + o, err := apiserver.DryRunValidate(ctx, namespace, t, tekton) + if err != nil { return nil, nil, err } - return t, nil, err + switch mutatedTask := o.(type) { + case *v1.Task: + mutatedTask.ObjectMeta = obj.ObjectMeta + return mutatedTask, nil, nil + } case *v1.Task: // This SetDefaults is currently not necessary, but for consistency, it is recommended to add it. // Avoid forgetting to add it in the future when there is a v2 version, causing similar problems. @@ -317,10 +340,15 @@ func readRuntimeObjectAsTask(ctx context.Context, namespace string, obj runtime. 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 - if err := apiserver.DryRunValidate(ctx, namespace, obj, tekton); err != nil { + o, err := apiserver.DryRunValidate(ctx, namespace, obj, tekton) + if err != nil { return nil, nil, err } - return obj, &vr, nil + switch mutatedTask := o.(type) { + case *v1.Task: + mutatedTask.ObjectMeta = obj.ObjectMeta + return mutatedTask, &vr, nil + } } return nil, nil, errors.New("resource is not a task") }