Skip to content

Commit

Permalink
DryRunValidate returns the mutated object
Browse files Browse the repository at this point in the history 
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 #8071
  • Loading branch information
@chitrangpatel
chitrangpatel committed Jul 17, 2024
1 parent 0db5ca2 commit 8382ac9
Show file tree
Hide file tree
Showing 4 changed files with 134 additions and 74 deletions.
44 changes: 28 additions & 16 deletions pkg/reconciler/apiserver/apiserver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,57 +25,69 @@ 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) {
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)
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 {
Expand Down
42 changes: 27 additions & 15 deletions pkg/reconciler/apiserver/apiserver_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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)
}
})
}
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down
40 changes: 24 additions & 16 deletions pkg/reconciler/pipelinerun/resources/pipelineref.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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")
}
82 changes: 55 additions & 27 deletions pkg/reconciler/taskrun/resources/taskref.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand All @@ -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.
Expand All @@ -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")
}
Expand Down

0 comments on commit 8382ac9

Please sign in to comment.