From 885e2871f7790ee69d2e1f1b3ca469730f2737e6 Mon Sep 17 00:00:00 2001 From: Pierangelo Di Pilato Date: Fri, 26 Nov 2021 10:58:43 +0100 Subject: [PATCH 1/9] Add support for callback defaults Signed-off-by: Pierangelo Di Pilato --- testing/resource.go | 12 + .../defaulting/controller.go | 22 +- .../defaulting/defaulting.go | 102 +++++++- .../defaulting/defaulting_test.go | 221 ++++++++++++++++-- 4 files changed, 334 insertions(+), 23 deletions(-) diff --git a/testing/resource.go b/testing/resource.go index d562b7ce9d..1f749073e2 100644 --- a/testing/resource.go +++ b/testing/resource.go @@ -23,6 +23,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "knative.dev/pkg/apis" ) @@ -49,6 +50,7 @@ type ResourceSpec struct { FieldThatsImmutable string `json:"fieldThatsImmutable,omitempty"` FieldThatsImmutableWithDefault string `json:"fieldThatsImmutableWithDefault,omitempty"` FieldForCallbackValidation string `json:"fieldThatCallbackRejects,omitempty"` + FieldForCallbackDefaulting string `json:"fieldDefaultingCallback,omitempty"` } // GetGroupVersionKind returns the GroupVersionKind. @@ -56,6 +58,16 @@ func (r *Resource) GetGroupVersionKind() schema.GroupVersionKind { return SchemeGroupVersion.WithKind("Resource") } +// GetGroupVersionKindMeta returns the metav1.GroupVersionKind. +func (r *Resource) GetGroupVersionKindMeta() metav1.GroupVersionKind { + gvk := r.GroupVersionKind() + return metav1.GroupVersionKind{ + Group: gvk.Group, + Version: gvk.Version, + Kind: gvk.Kind, + } +} + // GetUntypedSpec returns the spec of the resource. func (r *Resource) GetUntypedSpec() interface{} { return r.Spec diff --git a/webhook/resourcesemantics/defaulting/controller.go b/webhook/resourcesemantics/defaulting/controller.go index a4a41d0bea..b50fcf8f41 100644 --- a/webhook/resourcesemantics/defaulting/controller.go +++ b/webhook/resourcesemantics/defaulting/controller.go @@ -29,6 +29,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/cache" + "knative.dev/pkg/controller" "knative.dev/pkg/system" "knative.dev/pkg/webhook" @@ -42,6 +43,7 @@ func NewAdmissionController( handlers map[schema.GroupVersionKind]resourcesemantics.GenericCRD, wc func(context.Context) context.Context, disallowUnknownFields bool, + callbacks ...map[schema.GroupVersionKind]Callback, ) *controller.Impl { client := kubeclient.Get(ctx) @@ -51,6 +53,19 @@ func NewAdmissionController( key := types.NamespacedName{Name: name} + // This not ideal, we are using a variadic argument to effectively make callbacks optional + // This allows this addition to be non-breaking to consumers of /pkg + // TODO: once all sub-repos have adopted this, we might move this back to a traditional param. + var unwrappedCallbacks map[schema.GroupVersionKind]Callback + switch len(callbacks) { + case 0: + unwrappedCallbacks = map[schema.GroupVersionKind]Callback{} + case 1: + unwrappedCallbacks = callbacks[0] + default: + panic("NewAdmissionController may not be called with multiple callback maps") + } + wh := &reconciler{ LeaderAwareFuncs: pkgreconciler.LeaderAwareFuncs{ // Have this reconciler enqueue our singleton whenever it becomes leader. @@ -60,9 +75,10 @@ func NewAdmissionController( }, }, - key: key, - path: path, - handlers: handlers, + key: key, + path: path, + handlers: handlers, + callbacks: unwrappedCallbacks, withContext: wc, disallowUnknownFields: disallowUnknownFields, diff --git a/webhook/resourcesemantics/defaulting/defaulting.go b/webhook/resourcesemantics/defaulting/defaulting.go index 504dcaa066..a65b6d2bce 100644 --- a/webhook/resourcesemantics/defaulting/defaulting.go +++ b/webhook/resourcesemantics/defaulting/defaulting.go @@ -25,16 +25,18 @@ import ( "github.com/gobuffalo/flect" "go.uber.org/zap" - jsonpatch "gomodules.xyz/jsonpatch/v2" + "gomodules.xyz/jsonpatch/v2" admissionv1 "k8s.io/api/admission/v1" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes" admissionlisters "k8s.io/client-go/listers/admissionregistration/v1" corelisters "k8s.io/client-go/listers/core/v1" + "knative.dev/pkg/apis" "knative.dev/pkg/apis/duck" "knative.dev/pkg/controller" @@ -56,9 +58,10 @@ type reconciler struct { webhook.StatelessAdmissionImpl pkgreconciler.LeaderAwareFuncs - key types.NamespacedName - path string - handlers map[schema.GroupVersionKind]resourcesemantics.GenericCRD + key types.NamespacedName + path string + handlers map[schema.GroupVersionKind]resourcesemantics.GenericCRD + callbacks map[schema.GroupVersionKind]Callback withContext func(context.Context) context.Context @@ -70,6 +73,34 @@ type reconciler struct { secretName string } +// CallbackFunc is the function to be invoked. +type CallbackFunc func(ctx context.Context, unstructured *unstructured.Unstructured) error + +// Callback is a generic function to be called by a consumer of defaulting. +type Callback struct { + // function is the callback to be invoked. + function CallbackFunc + + // supportedVerbs are the verbs supported for the callback. + // The function will only be called on these actions. + supportedVerbs map[webhook.Operation]struct{} +} + +// NewCallback creates a new callback function to be invoked on supported verbs. +func NewCallback(function func(context.Context, *unstructured.Unstructured) error, supportedVerbs ...webhook.Operation) Callback { + if function == nil { + panic("expected function, got nil") + } + m := make(map[webhook.Operation]struct{}) + for _, op := range supportedVerbs { + if _, has := m[op]; has { + panic("duplicate verbs not allowed") + } + m[op] = struct{}{} + } + return Callback{function: function, supportedVerbs: m} +} + var _ controller.Reconciler = (*reconciler)(nil) var _ pkgreconciler.LeaderAware = (*reconciler)(nil) var _ webhook.AdmissionController = (*reconciler)(nil) @@ -231,8 +262,18 @@ func (ac *reconciler) mutate(ctx context.Context, req *admissionv1.AdmissionRequ logger := logging.FromContext(ctx) handler, ok := ac.handlers[gvk] if !ok { - logger.Error("Unhandled kind: ", gvk) - return nil, fmt.Errorf("unhandled kind: %v", gvk) + if _, ok := ac.callbacks[gvk]; !ok { + logger.Error("Unhandled kind: ", gvk) + return nil, fmt.Errorf("unhandled kind: %v", gvk) + } + patches, err := ac.callback(ctx, gvk, req, duck.JSONPatch{}) + if err != nil { + logger.Errorw("Failed the callback defaulter", zap.Error(err)) + // Return the error message as-is to give the defaulter callback + // discretion over (our portion of) the message that the user sees. + return nil, err + } + return json.Marshal(patches) } // nil values denote absence of `old` (create) or `new` (delete) objects. @@ -302,6 +343,13 @@ func (ac *reconciler) mutate(ctx context.Context, req *admissionv1.AdmissionRequ return nil, err } + if patches, err = ac.callback(ctx, gvk, req, patches); err != nil { + logger.Errorw("Failed the callback defaulter", zap.Error(err)) + // Return the error message as-is to give the defaulter callback + // discretion over (our portion of) the message that the user sees. + return nil, err + } + // None of the validators will accept a nil value for newObj. if newObj == nil { return nil, errMissingNewObject @@ -329,6 +377,48 @@ func (ac *reconciler) setUserInfoAnnotations(ctx context.Context, patches duck.J return append(patches, patch...), nil } +func (ac *reconciler) callback(ctx context.Context, gvk schema.GroupVersionKind, req *admissionv1.AdmissionRequest, patches duck.JSONPatch) (duck.JSONPatch, error) { + oldBytes := req.OldObject.Raw + newBytes := req.Object.Raw + + // Setup context. + if len(oldBytes) != 0 { + ctx = apis.WithinUpdate(ctx, oldBytes) + } else { + ctx = apis.WithinCreate(ctx) + } + + // Get callback. + callback, ok := ac.callbacks[gvk] + if !ok { + return patches, nil + } + + // Check if request operation is a supported webhook operation. + if _, isSupported := callback.supportedVerbs[req.Operation]; !isSupported { + return patches, nil + } + + before := &unstructured.Unstructured{} + after := &unstructured.Unstructured{} + + // Get unstructured object. + if err := json.Unmarshal(newBytes, before); err != nil { + return nil, fmt.Errorf("cannot decode object: %w", err) + } + // Copy before in after unstructured objects. + before.DeepCopyInto(after) + + // Call callback passing after. + if err := callback.function(ctx, after); err != nil { + return patches, err + } + + // Create patches. + patch, err := duck.CreatePatch(before.Object, after.Object) + return append(patches, patch...), err +} + // roundTripPatch generates the JSONPatch that corresponds to round tripping the given bytes through // the Golang type (JSON -> Golang type -> JSON). Because it is not always true that // bytes == json.Marshal(json.Unmarshal(bytes)). diff --git a/webhook/resourcesemantics/defaulting/defaulting_test.go b/webhook/resourcesemantics/defaulting/defaulting_test.go index dd52307621..fca0a7d160 100644 --- a/webhook/resourcesemantics/defaulting/defaulting_test.go +++ b/webhook/resourcesemantics/defaulting/defaulting_test.go @@ -19,14 +19,19 @@ package defaulting import ( "context" "encoding/json" + "errors" + "fmt" "testing" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + // Injection stuff _ "knative.dev/pkg/client/injection/kube/client/fake" _ "knative.dev/pkg/client/injection/kube/informers/admissionregistration/v1/mutatingwebhookconfiguration/fake" _ "knative.dev/pkg/injection/clients/namespacedkube/informers/core/v1/secret/fake" - jsonpatch "gomodules.xyz/jsonpatch/v2" + "gomodules.xyz/jsonpatch/v2" admissionv1 "k8s.io/api/admission/v1" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" authenticationv1 "k8s.io/api/authentication/v1" @@ -78,6 +83,29 @@ var ( }: &InnerDefaultResource{}, } + callbacks = map[schema.GroupVersionKind]Callback{ + { + Group: "pkg.knative.dev", + Version: "v1alpha1", + Kind: "Resource", + }: NewCallback(resourceCallback, webhook.Create, webhook.Update, webhook.Delete), + { + Group: "pkg.knative.dev", + Version: "v1beta1", + Kind: "Resource", + }: NewCallback(resourceCallback, webhook.Create, webhook.Update, webhook.Delete), + { + Group: "pkg.knative.dev", + Version: "v1beta1", + Kind: "ResourceCallbackDefault", + }: NewCallback(resourceCallback, webhook.Create, webhook.Update, webhook.Delete), + { + Group: "pkg.knative.dev", + Version: "v1beta1", + Kind: "ResourceCallbackDefaultCreate", + }: NewCallback(resourceCallback, webhook.Create), + } + initialResourceWebhook = &admissionregistrationv1.MutatingWebhookConfiguration{ ObjectMeta: metav1.ObjectMeta{ Name: "webhook.knative.dev", @@ -218,10 +246,11 @@ func TestUnknownMetadataFieldSucceeds(t *testing.T) { func TestAdmitCreates(t *testing.T) { tests := []struct { - name string - setup func(context.Context, *Resource) - rejection string - patches []jsonpatch.JsonPatchOperation + name string + setup func(context.Context, *Resource) + rejection string + patches []jsonpatch.JsonPatchOperation + createRequestFunc func(ctx context.Context, t *testing.T, r *Resource) *admissionv1.AdmissionRequest }{{ name: "test simple creation (alpha, no diff)", setup: func(ctx context.Context, r *Resource) { @@ -323,6 +352,56 @@ func TestAdmitCreates(t *testing.T) { Path: "/metadata/annotations/pkg.knative.dev~1creator", Value: user1, }}, + }, { + name: "test simple creation (callback return error)", + setup: func(ctx context.Context, resource *Resource) { + resource.Spec.FieldForCallbackDefaulting = "no magic value" + }, + rejection: "no magic value", + }, { + name: "test simple creation (resource and callback defaults)", + setup: func(ctx context.Context, r *Resource) { + r.SetDefaults(ctx) + r.Spec.FieldForCallbackDefaulting = "magic value" + // THIS IS NOT WHO IS CREATING IT, IT IS LIES! + r.Annotations = map[string]string{ + "pkg.knative.dev/lastModifier": user2, + } + }, + patches: []jsonpatch.JsonPatchOperation{{ + Operation: "replace", + Path: "/spec/fieldDefaultingCallback", + Value: "I'm a default", + }, { + Operation: "replace", + Path: "/metadata/annotations/pkg.knative.dev~1lastModifier", + Value: user1, + }, { + Operation: "add", + Path: "/metadata/annotations/pkg.knative.dev~1creator", + Value: user1, + }}, + }, { + name: "test simple creation (only callback defaults)", + setup: func(ctx context.Context, r *Resource) { + r.TypeMeta.APIVersion = "pkg.knative.dev/v1beta1" + r.TypeMeta.Kind = "ResourceCallbackDefault" + r.Spec.FieldForCallbackDefaulting = "magic value" + // THIS IS NOT WHO IS CREATING IT, IT LIES! + r.Annotations = map[string]string{ + "pkg.knative.dev/lastModifier": user2, + } + }, + createRequestFunc: func(ctx context.Context, t *testing.T, r *Resource) *admissionv1.AdmissionRequest { + req := createCreateResource(ctx, t, r) + req.Kind = r.GetGroupVersionKindMeta() + return req + }, + patches: []jsonpatch.JsonPatchOperation{{ + Operation: "replace", + Path: "/spec/fieldDefaultingCallback", + Value: "I'm a default", + }}, }} for _, tc := range tests { @@ -336,7 +415,13 @@ func TestAdmitCreates(t *testing.T) { tc.setup(ctx, r) _, ac := newNonRunningTestResourceAdmissionController(t) - resp := ac.Admit(ctx, createCreateResource(ctx, t, r)) + var req *admissionv1.AdmissionRequest + if tc.createRequestFunc == nil { + req = createCreateResource(ctx, t, r) + } else { + req = tc.createRequestFunc(ctx, t, r) + } + resp := ac.Admit(ctx, req) if tc.rejection == "" { ExpectAllowed(t, resp) @@ -370,11 +455,13 @@ func createCreateResource(ctx context.Context, t *testing.T, r *Resource) *admis func TestAdmitUpdates(t *testing.T) { tests := []struct { - name string - setup func(context.Context, *Resource) - mutate func(context.Context, *Resource) - rejection string - patches []jsonpatch.JsonPatchOperation + name string + setup func(context.Context, *Resource) + mutate func(context.Context, *Resource) + createResourceFunc func(name string) *Resource + createUpdateResourceFunc func(ctx context.Context, t *testing.T, old, new *Resource) *admissionv1.AdmissionRequest + rejection string + patches []jsonpatch.JsonPatchOperation }{{ name: "test simple update (no diff)", setup: func(ctx context.Context, r *Resource) { @@ -385,6 +472,71 @@ func TestAdmitUpdates(t *testing.T) { // annotation doesn't change. }, patches: []jsonpatch.JsonPatchOperation{}, + }, { + name: "test simple update (callback defaults error)", + setup: func(ctx context.Context, r *Resource) { + r.SetDefaults(ctx) + }, + mutate: func(ctx context.Context, r *Resource) { + r.Spec.FieldForCallbackDefaulting = "no magic value" + }, + rejection: "no magic value", + }, { + name: "test simple update (callback defaults)", + setup: func(ctx context.Context, r *Resource) { + r.SetDefaults(ctx) + }, + mutate: func(ctx context.Context, r *Resource) { + r.Spec.FieldForCallbackDefaulting = "magic value" + }, + patches: []jsonpatch.JsonPatchOperation{{ + Operation: "replace", + Path: "/spec/fieldDefaultingCallback", + Value: "I'm a default", + }, { + + Operation: "replace", + Path: "/metadata/annotations/pkg.knative.dev~1lastModifier", + Value: user2, + }}, + }, { + name: "test simple update (callback defaults only)", + setup: func(ctx context.Context, r *Resource) { + r.TypeMeta.APIVersion = "pkg.knative.dev/v1beta1" + r.TypeMeta.Kind = "ResourceCallbackDefault" + r.Spec.FieldForCallbackDefaulting = "magic value" + r.SetDefaults(ctx) + }, + mutate: func(ctx context.Context, r *Resource) { + r.Spec.FieldForCallbackDefaulting = "magic value" + }, + createUpdateResourceFunc: func(ctx context.Context, t *testing.T, old, new *Resource) *admissionv1.AdmissionRequest { + req := createUpdateResource(ctx, t, old, new) + req.Kind = new.GetGroupVersionKindMeta() + return req + }, + patches: []jsonpatch.JsonPatchOperation{{ + Operation: "replace", + Path: "/spec/fieldDefaultingCallback", + Value: "I'm a default", + }}, + }, { + name: "test simple update (callback defaults only, operation not supported)", + setup: func(ctx context.Context, r *Resource) { + r.TypeMeta.APIVersion = "pkg.knative.dev/v1beta1" + r.TypeMeta.Kind = "ResourceCallbackDefaultCreate" + r.Spec.FieldForCallbackDefaulting = "magic value" + r.SetDefaults(ctx) + }, + mutate: func(ctx context.Context, r *Resource) { + r.Spec.FieldForCallbackDefaulting = "magic value" + }, + createUpdateResourceFunc: func(ctx context.Context, t *testing.T, old, new *Resource) *admissionv1.AdmissionRequest { + req := createUpdateResource(ctx, t, old, new) + req.Operation = admissionv1.Update + req.Kind = new.GetGroupVersionKindMeta() + return req + }, }, { name: "test simple update (update updater annotation)", setup: func(ctx context.Context, r *Resource) { @@ -428,7 +580,14 @@ func TestAdmitUpdates(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - old := CreateResource("a name") + name := "a name" + + var old *Resource + if tc.createResourceFunc == nil { + old = CreateResource(name) + } else { + old = tc.createResourceFunc(name) + } ctx := TestContextWithLogger(t) old.Annotations = map[string]string{ @@ -446,7 +605,15 @@ func TestAdmitUpdates(t *testing.T) { tc.mutate(ctx, new) _, ac := newNonRunningTestResourceAdmissionController(t) - resp := ac.Admit(ctx, createUpdateResource(ctx, t, old, new)) + + var req *admissionv1.AdmissionRequest + if tc.createUpdateResourceFunc == nil { + req = createUpdateResource(ctx, t, old, new) + } else { + req = tc.createUpdateResourceFunc(ctx, t, old, new) + } + + resp := ac.Admit(ctx, req) if tc.rejection == "" { ExpectAllowed(t, resp) @@ -511,6 +678,10 @@ func TestValidCreateResourceSucceedsWithRoundTripAndDefaultPatch(t *testing.T) { func createInnerDefaultResourceWithoutSpec(t *testing.T) []byte { t.Helper() r := InnerDefaultResource{ + TypeMeta: metav1.TypeMeta{ + Kind: "InnerDefaultResource", + APIVersion: fmt.Sprintf("%s/%s", SchemeGroupVersion.Group, SchemeGroupVersion.Version), + }, ObjectMeta: metav1.ObjectMeta{ Namespace: system.Namespace(), Name: "a name", @@ -543,5 +714,27 @@ func newTestResourceAdmissionController(t *testing.T) webhook.AdmissionControlle ctx, testResourceValidationName, testResourceValidationPath, handlers, func(ctx context.Context) context.Context { return ctx - }, true).Reconciler.(*reconciler) + }, true, callbacks).Reconciler.(*reconciler) +} + +func resourceCallback(_ context.Context, uns *unstructured.Unstructured) error { + var resource Resource + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(uns.UnstructuredContent(), &resource); err != nil { + return err + } + + if resource.Spec.FieldForCallbackDefaulting != "" { + if resource.Spec.FieldForCallbackDefaulting != "magic value" { + return errors.New(resource.Spec.FieldForCallbackDefaulting) + } + resource.Spec.FieldForCallbackDefaulting = "I'm a default" + } + + u, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&resource) + if err != nil { + return err + } + uns.Object = u + + return nil } From a8cc831a97389630b58da3192fcfa141a8060a25 Mon Sep 17 00:00:00 2001 From: Pierangelo Di Pilato Date: Fri, 3 Dec 2021 18:58:58 +0100 Subject: [PATCH 2/9] Put unstr object in ctx and set user info Signed-off-by: Pierangelo Di Pilato --- .../defaulting/defaulting.go | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/webhook/resourcesemantics/defaulting/defaulting.go b/webhook/resourcesemantics/defaulting/defaulting.go index a65b6d2bce..9860fe4225 100644 --- a/webhook/resourcesemantics/defaulting/defaulting.go +++ b/webhook/resourcesemantics/defaulting/defaulting.go @@ -381,12 +381,27 @@ func (ac *reconciler) callback(ctx context.Context, gvk schema.GroupVersionKind, oldBytes := req.OldObject.Raw newBytes := req.Object.Raw + before := &unstructured.Unstructured{} + after := &unstructured.Unstructured{} + + // Get unstructured object. + if err := json.Unmarshal(newBytes, before); err != nil { + return nil, fmt.Errorf("cannot decode object: %w", err) + } + // Copy before in after unstructured objects. + before.DeepCopyInto(after) + // Setup context. if len(oldBytes) != 0 { - ctx = apis.WithinUpdate(ctx, oldBytes) + if req.SubResource == "" { + ctx = apis.WithinUpdate(ctx, before) + } else { + ctx = apis.WithinSubResourceUpdate(ctx, before, req.SubResource) + } } else { ctx = apis.WithinCreate(ctx) } + ctx = apis.WithUserInfo(ctx, &req.UserInfo) // Get callback. callback, ok := ac.callbacks[gvk] @@ -399,16 +414,6 @@ func (ac *reconciler) callback(ctx context.Context, gvk schema.GroupVersionKind, return patches, nil } - before := &unstructured.Unstructured{} - after := &unstructured.Unstructured{} - - // Get unstructured object. - if err := json.Unmarshal(newBytes, before); err != nil { - return nil, fmt.Errorf("cannot decode object: %w", err) - } - // Copy before in after unstructured objects. - before.DeepCopyInto(after) - // Call callback passing after. if err := callback.function(ctx, after); err != nil { return patches, err From 7628c02389fecbb7d64ed03a87681f18bc9b090a Mon Sep 17 00:00:00 2001 From: Pierangelo Di Pilato Date: Fri, 3 Dec 2021 19:00:13 +0100 Subject: [PATCH 3/9] Move get callback at the top Signed-off-by: Pierangelo Di Pilato --- .../defaulting/defaulting.go | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/webhook/resourcesemantics/defaulting/defaulting.go b/webhook/resourcesemantics/defaulting/defaulting.go index 9860fe4225..f88dcf3974 100644 --- a/webhook/resourcesemantics/defaulting/defaulting.go +++ b/webhook/resourcesemantics/defaulting/defaulting.go @@ -378,6 +378,17 @@ func (ac *reconciler) setUserInfoAnnotations(ctx context.Context, patches duck.J } func (ac *reconciler) callback(ctx context.Context, gvk schema.GroupVersionKind, req *admissionv1.AdmissionRequest, patches duck.JSONPatch) (duck.JSONPatch, error) { + // Get callback. + callback, ok := ac.callbacks[gvk] + if !ok { + return patches, nil + } + + // Check if request operation is a supported webhook operation. + if _, isSupported := callback.supportedVerbs[req.Operation]; !isSupported { + return patches, nil + } + oldBytes := req.OldObject.Raw newBytes := req.Object.Raw @@ -403,17 +414,6 @@ func (ac *reconciler) callback(ctx context.Context, gvk schema.GroupVersionKind, } ctx = apis.WithUserInfo(ctx, &req.UserInfo) - // Get callback. - callback, ok := ac.callbacks[gvk] - if !ok { - return patches, nil - } - - // Check if request operation is a supported webhook operation. - if _, isSupported := callback.supportedVerbs[req.Operation]; !isSupported { - return patches, nil - } - // Call callback passing after. if err := callback.function(ctx, after); err != nil { return patches, err From b2524a0474735c5d189cc3b0ecadbcca26f1f0af Mon Sep 17 00:00:00 2001 From: Pierangelo Di Pilato Date: Fri, 3 Dec 2021 19:01:35 +0100 Subject: [PATCH 4/9] Panic when using delete verb Signed-off-by: Pierangelo Di Pilato --- webhook/resourcesemantics/defaulting/defaulting.go | 3 +++ webhook/resourcesemantics/defaulting/defaulting_test.go | 6 +++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/webhook/resourcesemantics/defaulting/defaulting.go b/webhook/resourcesemantics/defaulting/defaulting.go index f88dcf3974..4c1a677bd5 100644 --- a/webhook/resourcesemantics/defaulting/defaulting.go +++ b/webhook/resourcesemantics/defaulting/defaulting.go @@ -93,6 +93,9 @@ func NewCallback(function func(context.Context, *unstructured.Unstructured) erro } m := make(map[webhook.Operation]struct{}) for _, op := range supportedVerbs { + if op == webhook.Delete { + panic("Verb " + webhook.Delete + " not allowed") + } if _, has := m[op]; has { panic("duplicate verbs not allowed") } diff --git a/webhook/resourcesemantics/defaulting/defaulting_test.go b/webhook/resourcesemantics/defaulting/defaulting_test.go index fca0a7d160..8401c70ad2 100644 --- a/webhook/resourcesemantics/defaulting/defaulting_test.go +++ b/webhook/resourcesemantics/defaulting/defaulting_test.go @@ -88,17 +88,17 @@ var ( Group: "pkg.knative.dev", Version: "v1alpha1", Kind: "Resource", - }: NewCallback(resourceCallback, webhook.Create, webhook.Update, webhook.Delete), + }: NewCallback(resourceCallback, webhook.Create, webhook.Update), { Group: "pkg.knative.dev", Version: "v1beta1", Kind: "Resource", - }: NewCallback(resourceCallback, webhook.Create, webhook.Update, webhook.Delete), + }: NewCallback(resourceCallback, webhook.Create, webhook.Update), { Group: "pkg.knative.dev", Version: "v1beta1", Kind: "ResourceCallbackDefault", - }: NewCallback(resourceCallback, webhook.Create, webhook.Update, webhook.Delete), + }: NewCallback(resourceCallback, webhook.Create, webhook.Update), { Group: "pkg.knative.dev", Version: "v1beta1", From 500de574aeb2052e1cfcb7e2701c785d4cef658c Mon Sep 17 00:00:00 2001 From: Pierangelo Di Pilato Date: Fri, 3 Dec 2021 19:56:00 +0100 Subject: [PATCH 5/9] Split tests and add callback ctx tests Signed-off-by: Pierangelo Di Pilato --- testing/resource.go | 16 +- .../defaulting/defaulting_test.go | 261 +++++++++++++----- 2 files changed, 196 insertions(+), 81 deletions(-) diff --git a/testing/resource.go b/testing/resource.go index 1f749073e2..00cb427d96 100644 --- a/testing/resource.go +++ b/testing/resource.go @@ -44,13 +44,15 @@ var _ apis.Listable = (*Resource)(nil) // ResourceSpec represents test resource spec. type ResourceSpec struct { - FieldWithDefault string `json:"fieldWithDefault,omitempty"` - FieldWithContextDefault string `json:"fieldWithContextDefault,omitempty"` - FieldWithValidation string `json:"fieldWithValidation,omitempty"` - FieldThatsImmutable string `json:"fieldThatsImmutable,omitempty"` - FieldThatsImmutableWithDefault string `json:"fieldThatsImmutableWithDefault,omitempty"` - FieldForCallbackValidation string `json:"fieldThatCallbackRejects,omitempty"` - FieldForCallbackDefaulting string `json:"fieldDefaultingCallback,omitempty"` + FieldWithDefault string `json:"fieldWithDefault,omitempty"` + FieldWithContextDefault string `json:"fieldWithContextDefault,omitempty"` + FieldWithValidation string `json:"fieldWithValidation,omitempty"` + FieldThatsImmutable string `json:"fieldThatsImmutable,omitempty"` + FieldThatsImmutableWithDefault string `json:"fieldThatsImmutableWithDefault,omitempty"` + FieldForCallbackValidation string `json:"fieldThatCallbackRejects,omitempty"` + FieldForCallbackDefaulting string `json:"fieldDefaultingCallback,omitempty"` + FieldForCallbackDefaultingIsWithinUpdate bool `json:"fieldForIsWithinUpdate,omitempty"` + FieldForCallbackDefaultingUsername string `json:"FieldForCallbackDefaultingUsername,omitempty"` } // GetGroupVersionKind returns the GroupVersionKind. diff --git a/webhook/resourcesemantics/defaulting/defaulting_test.go b/webhook/resourcesemantics/defaulting/defaulting_test.go index 8401c70ad2..9a1f9e5f23 100644 --- a/webhook/resourcesemantics/defaulting/defaulting_test.go +++ b/webhook/resourcesemantics/defaulting/defaulting_test.go @@ -21,6 +21,7 @@ import ( "encoding/json" "errors" "fmt" + "reflect" "testing" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -380,6 +381,10 @@ func TestAdmitCreates(t *testing.T) { Operation: "add", Path: "/metadata/annotations/pkg.knative.dev~1creator", Value: user1, + }, { + Operation: "add", + Path: "/spec/FieldForCallbackDefaultingUsername", + Value: user1, }}, }, { name: "test simple creation (only callback defaults)", @@ -401,6 +406,10 @@ func TestAdmitCreates(t *testing.T) { Operation: "replace", Path: "/spec/fieldDefaultingCallback", Value: "I'm a default", + }, { + Operation: "add", + Path: "/spec/FieldForCallbackDefaultingUsername", + Value: user1, }}, }} @@ -455,13 +464,11 @@ func createCreateResource(ctx context.Context, t *testing.T, r *Resource) *admis func TestAdmitUpdates(t *testing.T) { tests := []struct { - name string - setup func(context.Context, *Resource) - mutate func(context.Context, *Resource) - createResourceFunc func(name string) *Resource - createUpdateResourceFunc func(ctx context.Context, t *testing.T, old, new *Resource) *admissionv1.AdmissionRequest - rejection string - patches []jsonpatch.JsonPatchOperation + name string + setup func(context.Context, *Resource) + mutate func(context.Context, *Resource) + rejection string + patches []jsonpatch.JsonPatchOperation }{{ name: "test simple update (no diff)", setup: func(ctx context.Context, r *Resource) { @@ -481,62 +488,6 @@ func TestAdmitUpdates(t *testing.T) { r.Spec.FieldForCallbackDefaulting = "no magic value" }, rejection: "no magic value", - }, { - name: "test simple update (callback defaults)", - setup: func(ctx context.Context, r *Resource) { - r.SetDefaults(ctx) - }, - mutate: func(ctx context.Context, r *Resource) { - r.Spec.FieldForCallbackDefaulting = "magic value" - }, - patches: []jsonpatch.JsonPatchOperation{{ - Operation: "replace", - Path: "/spec/fieldDefaultingCallback", - Value: "I'm a default", - }, { - - Operation: "replace", - Path: "/metadata/annotations/pkg.knative.dev~1lastModifier", - Value: user2, - }}, - }, { - name: "test simple update (callback defaults only)", - setup: func(ctx context.Context, r *Resource) { - r.TypeMeta.APIVersion = "pkg.knative.dev/v1beta1" - r.TypeMeta.Kind = "ResourceCallbackDefault" - r.Spec.FieldForCallbackDefaulting = "magic value" - r.SetDefaults(ctx) - }, - mutate: func(ctx context.Context, r *Resource) { - r.Spec.FieldForCallbackDefaulting = "magic value" - }, - createUpdateResourceFunc: func(ctx context.Context, t *testing.T, old, new *Resource) *admissionv1.AdmissionRequest { - req := createUpdateResource(ctx, t, old, new) - req.Kind = new.GetGroupVersionKindMeta() - return req - }, - patches: []jsonpatch.JsonPatchOperation{{ - Operation: "replace", - Path: "/spec/fieldDefaultingCallback", - Value: "I'm a default", - }}, - }, { - name: "test simple update (callback defaults only, operation not supported)", - setup: func(ctx context.Context, r *Resource) { - r.TypeMeta.APIVersion = "pkg.knative.dev/v1beta1" - r.TypeMeta.Kind = "ResourceCallbackDefaultCreate" - r.Spec.FieldForCallbackDefaulting = "magic value" - r.SetDefaults(ctx) - }, - mutate: func(ctx context.Context, r *Resource) { - r.Spec.FieldForCallbackDefaulting = "magic value" - }, - createUpdateResourceFunc: func(ctx context.Context, t *testing.T, old, new *Resource) *admissionv1.AdmissionRequest { - req := createUpdateResource(ctx, t, old, new) - req.Operation = admissionv1.Update - req.Kind = new.GetGroupVersionKindMeta() - return req - }, }, { name: "test simple update (update updater annotation)", setup: func(ctx context.Context, r *Resource) { @@ -582,12 +533,7 @@ func TestAdmitUpdates(t *testing.T) { t.Run(tc.name, func(t *testing.T) { name := "a name" - var old *Resource - if tc.createResourceFunc == nil { - old = CreateResource(name) - } else { - old = tc.createResourceFunc(name) - } + old := CreateResource(name) ctx := TestContextWithLogger(t) old.Annotations = map[string]string{ @@ -606,12 +552,167 @@ func TestAdmitUpdates(t *testing.T) { _, ac := newNonRunningTestResourceAdmissionController(t) - var req *admissionv1.AdmissionRequest - if tc.createUpdateResourceFunc == nil { - req = createUpdateResource(ctx, t, old, new) + req := createUpdateResource(ctx, t, old, new) + + resp := ac.Admit(ctx, req) + + if tc.rejection == "" { + ExpectAllowed(t, resp) + ExpectPatches(t, resp.Patch, tc.patches) } else { - req = tc.createUpdateResourceFunc(ctx, t, old, new) + ExpectFailsWith(t, resp, tc.rejection) } + }) + } +} + +func TestAdmitUpdatesCallback(t *testing.T) { + tests := []struct { + name string + setup func(context.Context, *Resource) + mutate func(context.Context, *Resource) + createResourceFunc func(name string) *Resource + createUpdateResourceFunc func(ctx context.Context, t *testing.T, old, new *Resource) *admissionv1.AdmissionRequest + rejection string + patches []jsonpatch.JsonPatchOperation + }{ + { + name: "test simple update (callback defaults error)", + setup: func(ctx context.Context, r *Resource) { + r.SetDefaults(ctx) + }, + mutate: func(ctx context.Context, r *Resource) { + r.Spec.FieldForCallbackDefaulting = "no magic value" + }, + rejection: "no magic value", + createUpdateResourceFunc: createUpdateResource, + }, { + name: "test simple update (callback defaults)", + setup: func(ctx context.Context, r *Resource) { + r.SetDefaults(ctx) + }, + mutate: func(ctx context.Context, r *Resource) { + r.Spec.FieldForCallbackDefaulting = "magic value" + }, + patches: []jsonpatch.JsonPatchOperation{{ + Operation: "replace", + Path: "/spec/fieldDefaultingCallback", + Value: "I'm a default", + }, { + + Operation: "replace", + Path: "/metadata/annotations/pkg.knative.dev~1lastModifier", + Value: user2, + }, { + Operation: "add", + Path: "/spec/fieldForIsWithinUpdate", + Value: true, + }, { + Operation: "add", + Path: "/spec/FieldForCallbackDefaultingUsername", + Value: user2, + }}, + createUpdateResourceFunc: createUpdateResource, + }, { + name: "test simple update (callback defaults only)", + setup: func(ctx context.Context, r *Resource) { + r.TypeMeta.APIVersion = "pkg.knative.dev/v1beta1" + r.TypeMeta.Kind = "ResourceCallbackDefault" + r.Spec.FieldForCallbackDefaulting = "magic value" + r.SetDefaults(ctx) + }, + mutate: func(ctx context.Context, r *Resource) { + r.Spec.FieldForCallbackDefaulting = "magic value" + }, + createUpdateResourceFunc: func(ctx context.Context, t *testing.T, old, new *Resource) *admissionv1.AdmissionRequest { + req := createUpdateResource(ctx, t, old, new) + req.Kind = new.GetGroupVersionKindMeta() + return req + }, + patches: []jsonpatch.JsonPatchOperation{{ + Operation: "replace", + Path: "/spec/fieldDefaultingCallback", + Value: "I'm a default", + }, { + Operation: "add", + Path: "/spec/fieldForIsWithinUpdate", + Value: true, + }, { + Operation: "add", + Path: "/spec/FieldForCallbackDefaultingUsername", + Value: user2, + }}, + }, { + name: "test simple update (callback defaults only, operation not supported)", + setup: func(ctx context.Context, r *Resource) { + r.TypeMeta.APIVersion = "pkg.knative.dev/v1beta1" + r.TypeMeta.Kind = "ResourceCallbackDefaultCreate" + r.Spec.FieldForCallbackDefaulting = "magic value" + r.SetDefaults(ctx) + }, + mutate: func(ctx context.Context, r *Resource) { + r.Spec.FieldForCallbackDefaulting = "magic value" + }, + createUpdateResourceFunc: func(ctx context.Context, t *testing.T, old, new *Resource) *admissionv1.AdmissionRequest { + req := createUpdateResource(ctx, t, old, new) + req.Operation = admissionv1.Update + req.Kind = new.GetGroupVersionKindMeta() + return req + }, + }, { + name: "test simple update (callback defaults)", + setup: func(ctx context.Context, r *Resource) { + r.SetDefaults(ctx) + }, + mutate: func(ctx context.Context, r *Resource) { + r.Spec.FieldForCallbackDefaulting = "magic value" + }, + patches: []jsonpatch.JsonPatchOperation{{ + Operation: "replace", + Path: "/spec/fieldDefaultingCallback", + Value: "I'm a default", + }, { + + Operation: "replace", + Path: "/metadata/annotations/pkg.knative.dev~1lastModifier", + Value: user2, + }, { + Operation: "add", + Path: "/spec/fieldForIsWithinUpdate", + Value: true, + }, { + Operation: "add", + Path: "/spec/FieldForCallbackDefaultingUsername", + Value: user2, + }}, + createUpdateResourceFunc: createUpdateResource, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + name := "a name" + + old := CreateResource(name) + ctx := TestContextWithLogger(t) + + old.Annotations = map[string]string{ + "pkg.knative.dev/creator": user1, + "pkg.knative.dev/lastModifier": user1, + } + + tc.setup(ctx, old) + + new := old.DeepCopy() + + // Mutate the resource using the update context as user2 + ctx = apis.WithUserInfo(apis.WithinUpdate(ctx, old), + &authenticationv1.UserInfo{Username: user2}) + tc.mutate(ctx, new) + + _, ac := newNonRunningTestResourceAdmissionController(t) + + req := tc.createUpdateResourceFunc(ctx, t, old, new) resp := ac.Admit(ctx, req) @@ -717,7 +818,7 @@ func newTestResourceAdmissionController(t *testing.T) webhook.AdmissionControlle }, true, callbacks).Reconciler.(*reconciler) } -func resourceCallback(_ context.Context, uns *unstructured.Unstructured) error { +func resourceCallback(ctx context.Context, uns *unstructured.Unstructured) error { var resource Resource if err := runtime.DefaultUnstructuredConverter.FromUnstructured(uns.UnstructuredContent(), &resource); err != nil { return err @@ -727,7 +828,19 @@ func resourceCallback(_ context.Context, uns *unstructured.Unstructured) error { if resource.Spec.FieldForCallbackDefaulting != "magic value" { return errors.New(resource.Spec.FieldForCallbackDefaulting) } + resource.Spec.FieldForCallbackDefaultingIsWithinUpdate = apis.IsInUpdate(ctx) + resource.Spec.FieldForCallbackDefaultingUsername = apis.GetUserInfo(ctx).Username resource.Spec.FieldForCallbackDefaulting = "I'm a default" + if apis.IsInUpdate(ctx) { + if apis.GetBaseline(ctx) == nil { + return fmt.Errorf("expected baseline object") + } + if v, ok := apis.GetBaseline(ctx).(*unstructured.Unstructured); !ok { + return fmt.Errorf("expected *unstructured.Unstructured, got %v", reflect.TypeOf(v)) + } + } else if !apis.IsInCreate(ctx) { + return fmt.Errorf("expected to have context within update or create") + } } u, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&resource) From 0bb0dbbb43b767a75e2c9fb3b5c05727bc69f20c Mon Sep 17 00:00:00 2001 From: Pierangelo Di Pilato Date: Mon, 6 Dec 2021 11:31:36 +0100 Subject: [PATCH 6/9] Set user info annotations Signed-off-by: Pierangelo Di Pilato --- .../defaulting/defaulting.go | 10 +++++-- .../defaulting/defaulting_test.go | 12 ++++++++ .../resourcesemantics/defaulting/user_info.go | 28 +++++++++++++++++++ 3 files changed, 47 insertions(+), 3 deletions(-) diff --git a/webhook/resourcesemantics/defaulting/defaulting.go b/webhook/resourcesemantics/defaulting/defaulting.go index 4c1a677bd5..cab420cc07 100644 --- a/webhook/resourcesemantics/defaulting/defaulting.go +++ b/webhook/resourcesemantics/defaulting/defaulting.go @@ -269,7 +269,7 @@ func (ac *reconciler) mutate(ctx context.Context, req *admissionv1.AdmissionRequ logger.Error("Unhandled kind: ", gvk) return nil, fmt.Errorf("unhandled kind: %v", gvk) } - patches, err := ac.callback(ctx, gvk, req, duck.JSONPatch{}) + patches, err := ac.callback(ctx, gvk, req, true /* shouldSetUserInfo */, duck.JSONPatch{}) if err != nil { logger.Errorw("Failed the callback defaulter", zap.Error(err)) // Return the error message as-is to give the defaulter callback @@ -346,7 +346,7 @@ func (ac *reconciler) mutate(ctx context.Context, req *admissionv1.AdmissionRequ return nil, err } - if patches, err = ac.callback(ctx, gvk, req, patches); err != nil { + if patches, err = ac.callback(ctx, gvk, req, false /* shouldSetUserInfo */, patches); err != nil { logger.Errorw("Failed the callback defaulter", zap.Error(err)) // Return the error message as-is to give the defaulter callback // discretion over (our portion of) the message that the user sees. @@ -380,7 +380,7 @@ func (ac *reconciler) setUserInfoAnnotations(ctx context.Context, patches duck.J return append(patches, patch...), nil } -func (ac *reconciler) callback(ctx context.Context, gvk schema.GroupVersionKind, req *admissionv1.AdmissionRequest, patches duck.JSONPatch) (duck.JSONPatch, error) { +func (ac *reconciler) callback(ctx context.Context, gvk schema.GroupVersionKind, req *admissionv1.AdmissionRequest, shouldSetUserInfo bool, patches duck.JSONPatch) (duck.JSONPatch, error) { // Get callback. callback, ok := ac.callbacks[gvk] if !ok { @@ -422,6 +422,10 @@ func (ac *reconciler) callback(ctx context.Context, gvk schema.GroupVersionKind, return patches, err } + if shouldSetUserInfo { + setUserInfoAnnotationsUnstructured(ctx, after, before, req) + } + // Create patches. patch, err := duck.CreatePatch(before.Object, after.Object) return append(patches, patch...), err diff --git a/webhook/resourcesemantics/defaulting/defaulting_test.go b/webhook/resourcesemantics/defaulting/defaulting_test.go index 9a1f9e5f23..949f9fbafe 100644 --- a/webhook/resourcesemantics/defaulting/defaulting_test.go +++ b/webhook/resourcesemantics/defaulting/defaulting_test.go @@ -410,6 +410,14 @@ func TestAdmitCreates(t *testing.T) { Operation: "add", Path: "/spec/FieldForCallbackDefaultingUsername", Value: user1, + }, { + Operation: "replace", + Path: "/metadata/annotations/pkg.knative.dev~1lastModifier", + Value: user1, + }, { + Operation: "add", + Path: "/metadata/annotations/pkg.knative.dev~1creator", + Value: user1, }}, }} @@ -630,6 +638,10 @@ func TestAdmitUpdatesCallback(t *testing.T) { return req }, patches: []jsonpatch.JsonPatchOperation{{ + Operation: "replace", + Path: "/metadata/annotations/pkg.knative.dev~1lastModifier", + Value: user2, + }, { Operation: "replace", Path: "/spec/fieldDefaultingCallback", Value: "I'm a default", diff --git a/webhook/resourcesemantics/defaulting/user_info.go b/webhook/resourcesemantics/defaulting/user_info.go index 36c1cfbd21..477e6678e7 100644 --- a/webhook/resourcesemantics/defaulting/user_info.go +++ b/webhook/resourcesemantics/defaulting/user_info.go @@ -19,8 +19,11 @@ package defaulting import ( "context" + admissionv1 "k8s.io/api/admission/v1" "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "knative.dev/pkg/apis" ) @@ -50,3 +53,28 @@ func setUserInfoAnnotations(ctx context.Context, resource apis.HasSpec, groupNam } } } + +// setUserInfoAnnotationsUnstructured sets creator and updater annotations on a resource. +func setUserInfoAnnotationsUnstructured(ctx context.Context, after *unstructured.Unstructured, before *unstructured.Unstructured, req *admissionv1.AdmissionRequest) { + if v, ok := after.Object["metadata"]; ok { + if metadata, ok := v.(map[string]interface{}); ok { + if v, ok := metadata["annotations"]; ok { + if annotations, ok := v.(map[string]interface{}); ok { + if apis.IsInUpdate(ctx) { + + if equality.Semantic.DeepEqual(before.UnstructuredContent(), after.UnstructuredContent()) { + return + } + + annotations[req.Resource.Group+apis.UpdaterAnnotationSuffix] = req.UserInfo.Username + return + } + + annotations[req.Resource.Group+apis.CreatorAnnotationSuffix] = req.UserInfo.Username + annotations[req.Resource.Group+apis.UpdaterAnnotationSuffix] = req.UserInfo.Username + } + } + + } + } +} From 4162f860bbcc75344ba140d9384bb9ca34adf264 Mon Sep 17 00:00:00 2001 From: Pierangelo Di Pilato Date: Wed, 22 Dec 2021 15:12:50 +0100 Subject: [PATCH 7/9] Register Webhook Rules from callbacks Signed-off-by: Pierangelo Di Pilato --- .../resourcesemantics/defaulting/defaulting.go | 10 ++++++++++ .../resourcesemantics/defaulting/table_test.go | 18 +++++++++++++++++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/webhook/resourcesemantics/defaulting/defaulting.go b/webhook/resourcesemantics/defaulting/defaulting.go index cab420cc07..a1ea57fa62 100644 --- a/webhook/resourcesemantics/defaulting/defaulting.go +++ b/webhook/resourcesemantics/defaulting/defaulting.go @@ -171,7 +171,17 @@ func (ac *reconciler) reconcileMutatingWebhook(ctx context.Context, caCert []byt logger := logging.FromContext(ctx) rules := make([]admissionregistrationv1.RuleWithOperations, 0, len(ac.handlers)) + gvks := make(map[schema.GroupVersionKind]struct{}, len(ac.handlers)+len(ac.callbacks)) for gvk := range ac.handlers { + gvks[gvk] = struct{}{} + } + for gvk := range ac.callbacks { + if _, ok := gvks[gvk]; !ok { + gvks[gvk] = struct{}{} + } + } + + for gvk := range gvks { plural := strings.ToLower(flect.Pluralize(gvk.Kind)) rules = append(rules, admissionregistrationv1.RuleWithOperations{ diff --git a/webhook/resourcesemantics/defaulting/table_test.go b/webhook/resourcesemantics/defaulting/table_test.go index 1c7fa107db..6f5177cf00 100644 --- a/webhook/resourcesemantics/defaulting/table_test.go +++ b/webhook/resourcesemantics/defaulting/table_test.go @@ -33,6 +33,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/wait" clientgotesting "k8s.io/client-go/testing" + "knative.dev/pkg/configmap" "knative.dev/pkg/controller" "knative.dev/pkg/ptr" @@ -91,6 +92,20 @@ func TestReconcile(t *testing.T) { APIVersions: []string{"v1alpha1"}, Resources: []string{"resources", "resources/status"}, }, + }, { + Operations: []admissionregistrationv1.OperationType{"CREATE", "UPDATE"}, + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"pkg.knative.dev"}, + APIVersions: []string{"v1beta1"}, + Resources: []string{"resourcecallbackdefaultcreates", "resourcecallbackdefaultcreates/status"}, + }, + }, { + Operations: []admissionregistrationv1.OperationType{"CREATE", "UPDATE"}, + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"pkg.knative.dev"}, + APIVersions: []string{"v1beta1"}, + Resources: []string{"resourcecallbackdefaults", "resourcecallbackdefaults/status"}, + }, }, { Operations: []admissionregistrationv1.OperationType{"CREATE", "UPDATE"}, Rule: admissionregistrationv1.Rule{ @@ -419,7 +434,8 @@ func TestReconcile(t *testing.T) { }, path: path, - handlers: handlers, + handlers: handlers, + callbacks: callbacks, client: kubeclient.Get(ctx), mwhlister: listers.GetMutatingWebhookConfigurationLister(), From 8cc119201d66322649ea955d151f74c31c04cb54 Mon Sep 17 00:00:00 2001 From: Pierangelo Di Pilato Date: Wed, 12 Jan 2022 09:26:45 +0100 Subject: [PATCH 8/9] Adapt unstructured objects to apis.HasSpec Signed-off-by: Pierangelo Di Pilato --- .../defaulting/defaulting.go | 2 +- .../resourcesemantics/defaulting/user_info.go | 39 +++++++++++-------- 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/webhook/resourcesemantics/defaulting/defaulting.go b/webhook/resourcesemantics/defaulting/defaulting.go index a1ea57fa62..a8a30fae13 100644 --- a/webhook/resourcesemantics/defaulting/defaulting.go +++ b/webhook/resourcesemantics/defaulting/defaulting.go @@ -433,7 +433,7 @@ func (ac *reconciler) callback(ctx context.Context, gvk schema.GroupVersionKind, } if shouldSetUserInfo { - setUserInfoAnnotationsUnstructured(ctx, after, before, req) + setUserInfoAnnotations(adaptUnstructuredHasSpecCtx(ctx, req), unstructuredHasSpec{after}, req.Resource.Group) } // Create patches. diff --git a/webhook/resourcesemantics/defaulting/user_info.go b/webhook/resourcesemantics/defaulting/user_info.go index 477e6678e7..fcd97a4561 100644 --- a/webhook/resourcesemantics/defaulting/user_info.go +++ b/webhook/resourcesemantics/defaulting/user_info.go @@ -51,30 +51,35 @@ func setUserInfoAnnotations(ctx context.Context, resource apis.HasSpec, groupNam annotations[groupName+apis.CreatorAnnotationSuffix] = ui.Username annotations[groupName+apis.UpdaterAnnotationSuffix] = ui.Username } + objectMetaAccessor.GetObjectMeta().SetAnnotations(annotations) } } -// setUserInfoAnnotationsUnstructured sets creator and updater annotations on a resource. -func setUserInfoAnnotationsUnstructured(ctx context.Context, after *unstructured.Unstructured, before *unstructured.Unstructured, req *admissionv1.AdmissionRequest) { - if v, ok := after.Object["metadata"]; ok { - if metadata, ok := v.(map[string]interface{}); ok { - if v, ok := metadata["annotations"]; ok { - if annotations, ok := v.(map[string]interface{}); ok { - if apis.IsInUpdate(ctx) { +type unstructuredHasSpec struct { + *unstructured.Unstructured +} - if equality.Semantic.DeepEqual(before.UnstructuredContent(), after.UnstructuredContent()) { - return - } +func (us unstructuredHasSpec) GetObjectMeta() metav1.Object { + return us.Unstructured +} - annotations[req.Resource.Group+apis.UpdaterAnnotationSuffix] = req.UserInfo.Username - return - } +var _ metav1.ObjectMetaAccessor = unstructuredHasSpec{} - annotations[req.Resource.Group+apis.CreatorAnnotationSuffix] = req.UserInfo.Username - annotations[req.Resource.Group+apis.UpdaterAnnotationSuffix] = req.UserInfo.Username - } - } +func (us unstructuredHasSpec) GetUntypedSpec() interface{} { + if s, ok := us.Unstructured.Object["spec"]; ok { + return s + } + return nil +} +func adaptUnstructuredHasSpecCtx(ctx context.Context, req *admissionv1.AdmissionRequest) context.Context { + if apis.IsInUpdate(ctx) { + b := apis.GetBaseline(ctx) + if apis.IsInStatusUpdate(ctx) { + ctx = apis.WithinSubResourceUpdate(ctx, unstructuredHasSpec{b.(*unstructured.Unstructured)}, req.SubResource) + } else { + ctx = apis.WithinUpdate(ctx, unstructuredHasSpec{b.(*unstructured.Unstructured)}) } } + return ctx } From b18660053f1b046ba2df9565813905397bc79206 Mon Sep 17 00:00:00 2001 From: Pierangelo Di Pilato Date: Wed, 12 Jan 2022 09:39:38 +0100 Subject: [PATCH 9/9] Change json tag name to match struct field name Signed-off-by: Pierangelo Di Pilato --- testing/resource.go | 6 ++--- .../defaulting/defaulting_test.go | 26 +++++++++---------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/testing/resource.go b/testing/resource.go index 00cb427d96..e26ca317e4 100644 --- a/testing/resource.go +++ b/testing/resource.go @@ -50,9 +50,9 @@ type ResourceSpec struct { FieldThatsImmutable string `json:"fieldThatsImmutable,omitempty"` FieldThatsImmutableWithDefault string `json:"fieldThatsImmutableWithDefault,omitempty"` FieldForCallbackValidation string `json:"fieldThatCallbackRejects,omitempty"` - FieldForCallbackDefaulting string `json:"fieldDefaultingCallback,omitempty"` - FieldForCallbackDefaultingIsWithinUpdate bool `json:"fieldForIsWithinUpdate,omitempty"` - FieldForCallbackDefaultingUsername string `json:"FieldForCallbackDefaultingUsername,omitempty"` + FieldForCallbackDefaulting string `json:"fieldForCallbackDefaulting,omitempty"` + FieldForCallbackDefaultingIsWithinUpdate bool `json:"fieldForCallbackDefaultingIsWithinUpdate,omitempty"` + FieldForCallbackDefaultingUsername string `json:"fieldForCallbackDefaultingUsername,omitempty"` } // GetGroupVersionKind returns the GroupVersionKind. diff --git a/webhook/resourcesemantics/defaulting/defaulting_test.go b/webhook/resourcesemantics/defaulting/defaulting_test.go index 949f9fbafe..fc8ba71ec7 100644 --- a/webhook/resourcesemantics/defaulting/defaulting_test.go +++ b/webhook/resourcesemantics/defaulting/defaulting_test.go @@ -371,7 +371,7 @@ func TestAdmitCreates(t *testing.T) { }, patches: []jsonpatch.JsonPatchOperation{{ Operation: "replace", - Path: "/spec/fieldDefaultingCallback", + Path: "/spec/fieldForCallbackDefaulting", Value: "I'm a default", }, { Operation: "replace", @@ -383,7 +383,7 @@ func TestAdmitCreates(t *testing.T) { Value: user1, }, { Operation: "add", - Path: "/spec/FieldForCallbackDefaultingUsername", + Path: "/spec/fieldForCallbackDefaultingUsername", Value: user1, }}, }, { @@ -404,11 +404,11 @@ func TestAdmitCreates(t *testing.T) { }, patches: []jsonpatch.JsonPatchOperation{{ Operation: "replace", - Path: "/spec/fieldDefaultingCallback", + Path: "/spec/fieldForCallbackDefaulting", Value: "I'm a default", }, { Operation: "add", - Path: "/spec/FieldForCallbackDefaultingUsername", + Path: "/spec/fieldForCallbackDefaultingUsername", Value: user1, }, { Operation: "replace", @@ -604,7 +604,7 @@ func TestAdmitUpdatesCallback(t *testing.T) { }, patches: []jsonpatch.JsonPatchOperation{{ Operation: "replace", - Path: "/spec/fieldDefaultingCallback", + Path: "/spec/fieldForCallbackDefaulting", Value: "I'm a default", }, { @@ -613,11 +613,11 @@ func TestAdmitUpdatesCallback(t *testing.T) { Value: user2, }, { Operation: "add", - Path: "/spec/fieldForIsWithinUpdate", + Path: "/spec/fieldForCallbackDefaultingIsWithinUpdate", Value: true, }, { Operation: "add", - Path: "/spec/FieldForCallbackDefaultingUsername", + Path: "/spec/fieldForCallbackDefaultingUsername", Value: user2, }}, createUpdateResourceFunc: createUpdateResource, @@ -643,15 +643,15 @@ func TestAdmitUpdatesCallback(t *testing.T) { Value: user2, }, { Operation: "replace", - Path: "/spec/fieldDefaultingCallback", + Path: "/spec/fieldForCallbackDefaulting", Value: "I'm a default", }, { Operation: "add", - Path: "/spec/fieldForIsWithinUpdate", + Path: "/spec/fieldForCallbackDefaultingIsWithinUpdate", Value: true, }, { Operation: "add", - Path: "/spec/FieldForCallbackDefaultingUsername", + Path: "/spec/fieldForCallbackDefaultingUsername", Value: user2, }}, }, { @@ -681,7 +681,7 @@ func TestAdmitUpdatesCallback(t *testing.T) { }, patches: []jsonpatch.JsonPatchOperation{{ Operation: "replace", - Path: "/spec/fieldDefaultingCallback", + Path: "/spec/fieldForCallbackDefaulting", Value: "I'm a default", }, { @@ -690,11 +690,11 @@ func TestAdmitUpdatesCallback(t *testing.T) { Value: user2, }, { Operation: "add", - Path: "/spec/fieldForIsWithinUpdate", + Path: "/spec/fieldForCallbackDefaultingIsWithinUpdate", Value: true, }, { Operation: "add", - Path: "/spec/FieldForCallbackDefaultingUsername", + Path: "/spec/fieldForCallbackDefaultingUsername", Value: user2, }}, createUpdateResourceFunc: createUpdateResource,