From 99875b3fb73728caad3efb62556428b555ce02f4 Mon Sep 17 00:00:00 2001 From: Amine Date: Tue, 9 May 2023 16:47:11 -0500 Subject: [PATCH 01/11] Webhook Accessors Smart Recompilation Addresses https://github.com/kubernetes/kubernetes/issues/116588 This is an WIP patch trying to avoid recompiling CELs expressions when recreation Validating/Mutating WebhookAccessors. Maybe we should also concider using generatic.Controller from https://github.com/kubernetes/kubernetes/blob/5f59f449832e5206fe9b5fd7d9a43721c4c9ae44/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/internal/generic/controller.go --- .../validating_webhook_manager.go | 53 +++++++++++++++---- .../pkg/admission/plugin/webhook/accessors.go | 16 ++++++ 2 files changed, 60 insertions(+), 9 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go b/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go index f318b501293..4563984fc35 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go @@ -19,8 +19,9 @@ package configuration import ( "fmt" "sort" + "sync" - "k8s.io/api/admissionregistration/v1" + v1 "k8s.io/api/admissionregistration/v1" "k8s.io/apimachinery/pkg/labels" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apiserver/pkg/admission/plugin/webhook" @@ -33,9 +34,10 @@ import ( // validatingWebhookConfigurationManager collects the validating webhook objects so that they can be called. type validatingWebhookConfigurationManager struct { - lister admissionregistrationlisters.ValidatingWebhookConfigurationLister - hasSynced func() bool - lazy synctrack.Lazy[[]webhook.WebhookAccessor] + lister admissionregistrationlisters.ValidatingWebhookConfigurationLister + hasSynced func() bool + lazy synctrack.Lazy[[]webhook.WebhookAccessor] + needRefresh sync.Map // NOTE maybe use simple map and a sync.Mutex } var _ generic.Source = &validatingWebhookConfigurationManager{} @@ -48,15 +50,32 @@ func NewValidatingWebhookConfigurationManager(f informers.SharedInformerFactory) manager.lazy.Evaluate = manager.getConfiguration handle, _ := informer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ - AddFunc: func(_ interface{}) { manager.lazy.Notify() }, - UpdateFunc: func(_, _ interface{}) { manager.lazy.Notify() }, - DeleteFunc: func(_ interface{}) { manager.lazy.Notify() }, + AddFunc: func(_ interface{}) { manager.lazy.Notify() }, + UpdateFunc: func(old, new interface{}) { + //TODO: we can still dig deeper and figure out which ones of + // the webhooks have changed + whether CEL expressions changed + // etc... thinking about using reflect.. + obj := new.(*v1.ValidatingWebhookConfiguration) + // lock R+W + manager.needRefresh.Store(obj.GetName(), true) + manager.lazy.Notify() + }, + DeleteFunc: func(vwc interface{}) { + obj := vwc.(*v1.ValidatingWebhookConfiguration) + manager.needRefresh.Delete(obj.Name) + manager.lazy.Notify() + }, }) manager.hasSynced = handle.HasSynced return manager } +func (v *validatingWebhookConfigurationManager) configurationNeedRefresh(namespacedName string) bool { + _, ok := v.needRefresh.Load(namespacedName) + return ok +} + // Webhooks returns the merged ValidatingWebhookConfiguration. func (v *validatingWebhookConfigurationManager) Webhooks() []webhook.WebhookAccessor { out, err := v.lazy.Get() @@ -75,15 +94,31 @@ func (v *validatingWebhookConfigurationManager) getConfiguration() ([]webhook.We if err != nil { return []webhook.WebhookAccessor{}, err } - return mergeValidatingWebhookConfigurations(configurations), nil + return v.smartReloadValidatingWebhookConfigurations(configurations), nil } -func mergeValidatingWebhookConfigurations(configurations []*v1.ValidatingWebhookConfiguration) []webhook.WebhookAccessor { +func (v *validatingWebhookConfigurationManager) smartReloadValidatingWebhookConfigurations(configurations []*v1.ValidatingWebhookConfiguration) []webhook.WebhookAccessor { sort.SliceStable(configurations, ValidatingWebhookConfigurationSorter(configurations).ByName) accessors := []webhook.WebhookAccessor{} + cachedAccessors, _ := v.lazy.Get() +cfgLoop: for _, c := range configurations { + if !v.configurationNeedRefresh(c.Name) { + // Pick an already cached webhookAccessor + for _, ca := range cachedAccessors { + if ca.GetName() == c.Name { + accessors = append(accessors, ca) + v.needRefresh.Delete(c.Name) + continue cfgLoop + } + } + } + // webhook names are not validated for uniqueness, so we check for duplicates and // add a int suffix to distinguish between them + // + // NOTE: In `pkg/apis/admissionregistration/validation.go` webhook names are checked + // for uniqueness now. Is it safe to remove this? names := map[string]int{} for i := range c.Webhooks { n := c.Webhooks[i].Name diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go index 6c48418de61..b12c69e1c7d 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go @@ -216,6 +216,22 @@ func NewValidatingWebhookAccessor(uid, configurationName string, h *v1.Validatin return &validatingWebhookAccessor{uid: uid, configurationName: configurationName, ValidatingWebhook: h} } +// NOTE: we're thinking about adding a new NewValidatingWebhookAccessorWithOptions to allow +// recreating webhookAccessor with existing restClient, compiled CELs, selectors etc... to +// avoid remaking unnecessary expensive operations. +/* +type Options struct { + // rest client + // compiled CELs + // selectors... +} + +// NewValidatingWebhookAccessorWithOptions creates an accessor for a ValidatingWebhook. +func NewValidatingWebhookAccessorWithOptions(uid, configurationName string, h *v1.ValidatingWebhook, opt *Options) WebhookAccessor { + return &validatingWebhookAccessor{uid: uid, configurationName: configurationName, ValidatingWebhook: h} +} +*/ + type validatingWebhookAccessor struct { *v1.ValidatingWebhook uid string From c6f36e8702a9e90350c585298f1fc6e908699b12 Mon Sep 17 00:00:00 2001 From: Amine Date: Wed, 10 May 2023 15:09:15 -0500 Subject: [PATCH 02/11] Fix deadlock issue This patch fixes the deadlock issue by using a map to cache already initiated Webhooks instead of using `needRefresh` map. --- .../validating_webhook_manager.go | 36 ++++++++----------- 1 file changed, 15 insertions(+), 21 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go b/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go index 4563984fc35..34780bf1b22 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go @@ -34,10 +34,10 @@ import ( // validatingWebhookConfigurationManager collects the validating webhook objects so that they can be called. type validatingWebhookConfigurationManager struct { - lister admissionregistrationlisters.ValidatingWebhookConfigurationLister - hasSynced func() bool - lazy synctrack.Lazy[[]webhook.WebhookAccessor] - needRefresh sync.Map // NOTE maybe use simple map and a sync.Mutex + lister admissionregistrationlisters.ValidatingWebhookConfigurationLister + hasSynced func() bool + lazy synctrack.Lazy[[]webhook.WebhookAccessor] + configurationsCache sync.Map // NOTE maybe use simple map and a sync.Mutex } var _ generic.Source = &validatingWebhookConfigurationManager{} @@ -57,12 +57,12 @@ func NewValidatingWebhookConfigurationManager(f informers.SharedInformerFactory) // etc... thinking about using reflect.. obj := new.(*v1.ValidatingWebhookConfiguration) // lock R+W - manager.needRefresh.Store(obj.GetName(), true) + manager.configurationsCache.Delete(obj.GetName()) manager.lazy.Notify() }, DeleteFunc: func(vwc interface{}) { obj := vwc.(*v1.ValidatingWebhookConfiguration) - manager.needRefresh.Delete(obj.Name) + manager.configurationsCache.Delete(obj.Name) manager.lazy.Notify() }, }) @@ -71,11 +71,6 @@ func NewValidatingWebhookConfigurationManager(f informers.SharedInformerFactory) return manager } -func (v *validatingWebhookConfigurationManager) configurationNeedRefresh(namespacedName string) bool { - _, ok := v.needRefresh.Load(namespacedName) - return ok -} - // Webhooks returns the merged ValidatingWebhookConfiguration. func (v *validatingWebhookConfigurationManager) Webhooks() []webhook.WebhookAccessor { out, err := v.lazy.Get() @@ -100,18 +95,13 @@ func (v *validatingWebhookConfigurationManager) getConfiguration() ([]webhook.We func (v *validatingWebhookConfigurationManager) smartReloadValidatingWebhookConfigurations(configurations []*v1.ValidatingWebhookConfiguration) []webhook.WebhookAccessor { sort.SliceStable(configurations, ValidatingWebhookConfigurationSorter(configurations).ByName) accessors := []webhook.WebhookAccessor{} - cachedAccessors, _ := v.lazy.Get() cfgLoop: for _, c := range configurations { - if !v.configurationNeedRefresh(c.Name) { + cachedConfigurationAccessors, ok := v.configurationsCache.Load(c.Name) + if ok { // Pick an already cached webhookAccessor - for _, ca := range cachedAccessors { - if ca.GetName() == c.Name { - accessors = append(accessors, ca) - v.needRefresh.Delete(c.Name) - continue cfgLoop - } - } + accessors = append(accessors, cachedConfigurationAccessors.([]webhook.WebhookAccessor)...) + continue cfgLoop } // webhook names are not validated for uniqueness, so we check for duplicates and @@ -119,12 +109,16 @@ cfgLoop: // // NOTE: In `pkg/apis/admissionregistration/validation.go` webhook names are checked // for uniqueness now. Is it safe to remove this? + configurationAccessors := []webhook.WebhookAccessor{} names := map[string]int{} for i := range c.Webhooks { n := c.Webhooks[i].Name uid := fmt.Sprintf("%s/%s/%d", c.Name, n, names[n]) names[n]++ - accessors = append(accessors, webhook.NewValidatingWebhookAccessor(uid, c.Name, &c.Webhooks[i])) + configurationAccessors = append(configurationAccessors, webhook.NewValidatingWebhookAccessor(uid, c.Name, &c.Webhooks[i])) + // Cache the new accessors + v.configurationsCache.Store(c.Name, configurationAccessors) + accessors = append(accessors, configurationAccessors...) } } return accessors From 7d3d44af77679ed488b28dc839d02a8258fd3adc Mon Sep 17 00:00:00 2001 From: Amine Date: Fri, 12 May 2023 20:32:20 -0500 Subject: [PATCH 03/11] Add webhookAccessors smart reloads unit tests This patch adds few unit tests to assert that the webhook accessors are only recreate when they are update in the api-server. In order to test this feature we had to make few changes to wb manager that allows us to mock `NewValidatingWebhookAccessor` external function. --- .../validating_webhook_manager.go | 15 +- .../validating_webhook_manager_test.go | 174 ++++++++++++++++++ .../admission/configuration/webhook_cache.go | 1 + 3 files changed, 187 insertions(+), 3 deletions(-) create mode 100644 staging/src/k8s.io/apiserver/pkg/admission/configuration/webhook_cache.go diff --git a/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go b/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go index 34780bf1b22..cde2a577b13 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go @@ -32,12 +32,19 @@ import ( "k8s.io/client-go/tools/cache/synctrack" ) +type webhookAccessorCreator func(uid string, configurationName string, h *v1.ValidatingWebhook) webhook.WebhookAccessor + // validatingWebhookConfigurationManager collects the validating webhook objects so that they can be called. type validatingWebhookConfigurationManager struct { lister admissionregistrationlisters.ValidatingWebhookConfigurationLister hasSynced func() bool lazy synctrack.Lazy[[]webhook.WebhookAccessor] - configurationsCache sync.Map // NOTE maybe use simple map and a sync.Mutex + configurationsCache sync.Map + + // createValidatingWebhookAccessor is used to instantiate webhook accessors. + // This function is defined as field instead of a struct method to allow us + // to replace it with a mock during unit tests. + createValidatingWebhookAccessor webhookAccessorCreator } var _ generic.Source = &validatingWebhookConfigurationManager{} @@ -45,7 +52,8 @@ var _ generic.Source = &validatingWebhookConfigurationManager{} func NewValidatingWebhookConfigurationManager(f informers.SharedInformerFactory) generic.Source { informer := f.Admissionregistration().V1().ValidatingWebhookConfigurations() manager := &validatingWebhookConfigurationManager{ - lister: informer.Lister(), + lister: informer.Lister(), + createValidatingWebhookAccessor: webhook.NewValidatingWebhookAccessor, } manager.lazy.Evaluate = manager.getConfiguration @@ -115,12 +123,13 @@ cfgLoop: n := c.Webhooks[i].Name uid := fmt.Sprintf("%s/%s/%d", c.Name, n, names[n]) names[n]++ - configurationAccessors = append(configurationAccessors, webhook.NewValidatingWebhookAccessor(uid, c.Name, &c.Webhooks[i])) + configurationAccessors = append(configurationAccessors, v.createValidatingWebhookAccessor(uid, c.Name, &c.Webhooks[i])) // Cache the new accessors v.configurationsCache.Store(c.Name, configurationAccessors) accessors = append(accessors, configurationAccessors...) } } + return accessors } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager_test.go b/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager_test.go index dd28805f689..e90b79ddaaf 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager_test.go @@ -24,6 +24,7 @@ import ( "k8s.io/api/admissionregistration/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apiserver/pkg/admission/plugin/webhook" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes/fake" ) @@ -81,3 +82,176 @@ func TestGetValidatingWebhookConfig(t *testing.T) { } } } + +// mockCreateValidatingWebhookAccessor is a struct used to compute how many times +// the function webhook.NewValidatingWebhookAccessor is being called when refreshing +// webhookAccessors. +// +// NOTE: Maybe there some testing help that we can import and reuse instead. +type mockCreateValidatingWebhookAccessor struct { + numberOfCalls int +} + +func (mock *mockCreateValidatingWebhookAccessor) calledNTimes() int { return mock.numberOfCalls } +func (mock *mockCreateValidatingWebhookAccessor) resetCounter() { mock.numberOfCalls = 0 } +func (mock *mockCreateValidatingWebhookAccessor) incrementCounter() { mock.numberOfCalls++ } + +func (mock *mockCreateValidatingWebhookAccessor) fn(uid string, configurationName string, h *v1.ValidatingWebhook) webhook.WebhookAccessor { + mock.incrementCounter() + return webhook.NewValidatingWebhookAccessor(uid, configurationName, h) +} + +func TestGetValidatingWebhookConfigSmartReload(t *testing.T) { + type args struct { + createWebhookConfigurations []*v1.ValidatingWebhookConfiguration + updateWebhookConfigurations []*v1.ValidatingWebhookConfiguration + } + tests := []struct { + name string + args args + numberOfCreations int + // number of refreshes are number of times we pulled a webhook configuration + // from the cache without having to create new ones from scratch. + numberOfRefreshes int + }{ + { + name: "no creations and no updates", + args: args{ + nil, + nil, + }, + numberOfCreations: 0, + numberOfRefreshes: 0, + }, + { + name: "create configurations and no updates", + args: args{ + []*v1.ValidatingWebhookConfiguration{ + &v1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "webhook1"}, + Webhooks: []v1.ValidatingWebhook{{Name: "webhook1.1"}}, + }, + &v1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "webhook2"}, + Webhooks: []v1.ValidatingWebhook{{Name: "webhook2.1"}}, + }, + }, + nil, + }, + numberOfCreations: 2, + numberOfRefreshes: 0, + }, + { + name: "create configuration and update some of them", + args: args{ + []*v1.ValidatingWebhookConfiguration{ + &v1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "webhook3"}, + Webhooks: []v1.ValidatingWebhook{{Name: "webhook3.1"}}, + }, + &v1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "webhook4"}, + Webhooks: []v1.ValidatingWebhook{{Name: "webhook4.1"}}, + }, + }, + []*v1.ValidatingWebhookConfiguration{ + &v1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "webhook3"}, + Webhooks: []v1.ValidatingWebhook{{Name: "webhook3.1-updated"}}, + }, + }, + }, + numberOfCreations: 2, + numberOfRefreshes: 1, + }, + { + name: "create configuration and update moar of them", + args: args{ + []*v1.ValidatingWebhookConfiguration{ + &v1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "webhook5"}, + Webhooks: []v1.ValidatingWebhook{{Name: "webhook5.1"}, {Name: "webhook5.2"}}, + }, + &v1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "webhook6"}, + Webhooks: []v1.ValidatingWebhook{{Name: "webhook6.1"}}, + }, + &v1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "webhook7"}, + Webhooks: []v1.ValidatingWebhook{{Name: "webhook7.1"}, {Name: "webhook7.1"}}, + }, + }, + []*v1.ValidatingWebhookConfiguration{ + &v1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "webhook5"}, + Webhooks: []v1.ValidatingWebhook{{Name: "webhook5.1-updated"}}, + }, + &v1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "webhook7"}, + Webhooks: []v1.ValidatingWebhook{{Name: "webhook7.1-updated"}, {Name: "webhook7.2-updated"}}, + }, + }, + }, + numberOfCreations: 5, + numberOfRefreshes: 3, + }, + } + + client := fake.NewSimpleClientset() + informerFactory := informers.NewSharedInformerFactory(client, 0) + stop := make(chan struct{}) + defer close(stop) + + manager := NewValidatingWebhookConfigurationManager(informerFactory) + managerStructPtr := manager.(*validatingWebhookConfigurationManager) + fakeWebhookAccessorCreator := &mockCreateValidatingWebhookAccessor{} + managerStructPtr.createValidatingWebhookAccessor = fakeWebhookAccessorCreator.fn + + informerFactory.Start(stop) + informerFactory.WaitForCacheSync(stop) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Create webhooks + for _, configurations := range tt.args.createWebhookConfigurations { + client. + AdmissionregistrationV1(). + ValidatingWebhookConfigurations(). + Create(context.TODO(), configurations, metav1.CreateOptions{}) + } + // TODO use channels to wait for manager.createValidatingWebhookAccessor + // to be called instead of using time.Sleep + time.Sleep(1 * time.Second) + _ = manager.Webhooks() + // assert creations + if tt.numberOfCreations != fakeWebhookAccessorCreator.calledNTimes() { + t.Errorf( + "Expected number of creations %d received %d", + tt.numberOfCreations, fakeWebhookAccessorCreator.calledNTimes(), + ) + } + // reset mock counter + fakeWebhookAccessorCreator.resetCounter() + // Update webhooks + for _, configurations := range tt.args.updateWebhookConfigurations { + client. + AdmissionregistrationV1(). + ValidatingWebhookConfigurations(). + Update(context.TODO(), configurations, metav1.UpdateOptions{}) + } + // TODO use channels to wait for manager.createValidatingWebhookAccessor + // to be called instead of using time.Sleep + time.Sleep(1 * time.Second) + _ = manager.Webhooks() + // assert updates + if tt.numberOfRefreshes != fakeWebhookAccessorCreator.calledNTimes() { + t.Errorf( + "Expected number of refreshes %d received %d", + tt.numberOfRefreshes, fakeWebhookAccessorCreator.calledNTimes(), + ) + } + // reset mock counter for the next test cases + fakeWebhookAccessorCreator.resetCounter() + }) + } +} diff --git a/staging/src/k8s.io/apiserver/pkg/admission/configuration/webhook_cache.go b/staging/src/k8s.io/apiserver/pkg/admission/configuration/webhook_cache.go new file mode 100644 index 00000000000..49abac4d91d --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/admission/configuration/webhook_cache.go @@ -0,0 +1 @@ +package configuration From a01a8cb07e7bfe6dacadc51206ae4ef93d5f4352 Mon Sep 17 00:00:00 2001 From: Amine Date: Wed, 17 May 2023 10:54:17 -0500 Subject: [PATCH 04/11] Fix webhook accessors caching pattern --- .../validating_webhook_manager.go | 16 ++-- .../validating_webhook_manager_test.go | 78 ++++++++++++------- .../admission/configuration/webhook_cache.go | 1 - .../pkg/admission/plugin/webhook/accessors.go | 6 +- 4 files changed, 64 insertions(+), 37 deletions(-) delete mode 100644 staging/src/k8s.io/apiserver/pkg/admission/configuration/webhook_cache.go diff --git a/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go b/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go index cde2a577b13..ecb70a1232a 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go @@ -64,7 +64,6 @@ func NewValidatingWebhookConfigurationManager(f informers.SharedInformerFactory) // the webhooks have changed + whether CEL expressions changed // etc... thinking about using reflect.. obj := new.(*v1.ValidatingWebhookConfiguration) - // lock R+W manager.configurationsCache.Delete(obj.GetName()) manager.lazy.Notify() }, @@ -103,13 +102,12 @@ func (v *validatingWebhookConfigurationManager) getConfiguration() ([]webhook.We func (v *validatingWebhookConfigurationManager) smartReloadValidatingWebhookConfigurations(configurations []*v1.ValidatingWebhookConfiguration) []webhook.WebhookAccessor { sort.SliceStable(configurations, ValidatingWebhookConfigurationSorter(configurations).ByName) accessors := []webhook.WebhookAccessor{} -cfgLoop: for _, c := range configurations { cachedConfigurationAccessors, ok := v.configurationsCache.Load(c.Name) if ok { // Pick an already cached webhookAccessor accessors = append(accessors, cachedConfigurationAccessors.([]webhook.WebhookAccessor)...) - continue cfgLoop + continue } // webhook names are not validated for uniqueness, so we check for duplicates and @@ -117,17 +115,19 @@ cfgLoop: // // NOTE: In `pkg/apis/admissionregistration/validation.go` webhook names are checked // for uniqueness now. Is it safe to remove this? - configurationAccessors := []webhook.WebhookAccessor{} + // If we ever get rid of this, we can change the cache map value type to + // map[string]WebhookAccessor instead. Where keys will be of format "config.name/webhook.name" names := map[string]int{} + configurationAccessors := []webhook.WebhookAccessor{} for i := range c.Webhooks { n := c.Webhooks[i].Name uid := fmt.Sprintf("%s/%s/%d", c.Name, n, names[n]) names[n]++ - configurationAccessors = append(configurationAccessors, v.createValidatingWebhookAccessor(uid, c.Name, &c.Webhooks[i])) - // Cache the new accessors - v.configurationsCache.Store(c.Name, configurationAccessors) - accessors = append(accessors, configurationAccessors...) + configurationAccessor := v.createValidatingWebhookAccessor(uid, c.Name, &c.Webhooks[i]) + configurationAccessors = append(configurationAccessors, configurationAccessor) } + accessors = append(accessors, configurationAccessors...) + v.configurationsCache.Store(c.Name, configurationAccessors) } return accessors diff --git a/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager_test.go b/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager_test.go index e90b79ddaaf..435211e74d6 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager_test.go @@ -101,6 +101,14 @@ func (mock *mockCreateValidatingWebhookAccessor) fn(uid string, configurationNam return webhook.NewValidatingWebhookAccessor(uid, configurationName, h) } +func configurationTotalWebhooks(configurations []*v1.ValidatingWebhookConfiguration) int { + total := 0 + for _, configuration := range configurations { + total += len(configuration.Webhooks) + } + return total +} + func TestGetValidatingWebhookConfigSmartReload(t *testing.T) { type args struct { createWebhookConfigurations []*v1.ValidatingWebhookConfiguration @@ -112,7 +120,8 @@ func TestGetValidatingWebhookConfigSmartReload(t *testing.T) { numberOfCreations int // number of refreshes are number of times we pulled a webhook configuration // from the cache without having to create new ones from scratch. - numberOfRefreshes int + numberOfRefreshes int + finalNumberOfWebhookAccessors int }{ { name: "no creations and no updates", @@ -120,8 +129,9 @@ func TestGetValidatingWebhookConfigSmartReload(t *testing.T) { nil, nil, }, - numberOfCreations: 0, - numberOfRefreshes: 0, + numberOfCreations: 0, + numberOfRefreshes: 0, + finalNumberOfWebhookAccessors: 0, }, { name: "create configurations and no updates", @@ -138,11 +148,12 @@ func TestGetValidatingWebhookConfigSmartReload(t *testing.T) { }, nil, }, - numberOfCreations: 2, - numberOfRefreshes: 0, + numberOfCreations: 2, + numberOfRefreshes: 0, + finalNumberOfWebhookAccessors: 2, }, { - name: "create configuration and update some of them", + name: "create configurations and update some of them", args: args{ []*v1.ValidatingWebhookConfiguration{ &v1.ValidatingWebhookConfiguration{ @@ -161,8 +172,9 @@ func TestGetValidatingWebhookConfigSmartReload(t *testing.T) { }, }, }, - numberOfCreations: 2, - numberOfRefreshes: 1, + numberOfCreations: 2, + numberOfRefreshes: 1, + finalNumberOfWebhookAccessors: 2, }, { name: "create configuration and update moar of them", @@ -188,30 +200,29 @@ func TestGetValidatingWebhookConfigSmartReload(t *testing.T) { }, &v1.ValidatingWebhookConfiguration{ ObjectMeta: metav1.ObjectMeta{Name: "webhook7"}, - Webhooks: []v1.ValidatingWebhook{{Name: "webhook7.1-updated"}, {Name: "webhook7.2-updated"}}, + Webhooks: []v1.ValidatingWebhook{{Name: "webhook7.1-updated"}, {Name: "webhook7.2-updated"}, {Name: "webhook7.3"}}, }, }, }, - numberOfCreations: 5, - numberOfRefreshes: 3, + numberOfCreations: 5, + numberOfRefreshes: 4, + finalNumberOfWebhookAccessors: 5, }, } - client := fake.NewSimpleClientset() - informerFactory := informers.NewSharedInformerFactory(client, 0) - stop := make(chan struct{}) - defer close(stop) - - manager := NewValidatingWebhookConfigurationManager(informerFactory) - managerStructPtr := manager.(*validatingWebhookConfigurationManager) - fakeWebhookAccessorCreator := &mockCreateValidatingWebhookAccessor{} - managerStructPtr.createValidatingWebhookAccessor = fakeWebhookAccessorCreator.fn - - informerFactory.Start(stop) - informerFactory.WaitForCacheSync(stop) - for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + client := fake.NewSimpleClientset() + informerFactory := informers.NewSharedInformerFactory(client, 0) + stop := make(chan struct{}) + defer close(stop) + manager := NewValidatingWebhookConfigurationManager(informerFactory) + managerStructPtr := manager.(*validatingWebhookConfigurationManager) + fakeWebhookAccessorCreator := &mockCreateValidatingWebhookAccessor{} + managerStructPtr.createValidatingWebhookAccessor = fakeWebhookAccessorCreator.fn + informerFactory.Start(stop) + informerFactory.WaitForCacheSync(stop) + // Create webhooks for _, configurations := range tt.args.createWebhookConfigurations { client. @@ -222,7 +233,13 @@ func TestGetValidatingWebhookConfigSmartReload(t *testing.T) { // TODO use channels to wait for manager.createValidatingWebhookAccessor // to be called instead of using time.Sleep time.Sleep(1 * time.Second) - _ = manager.Webhooks() + webhooks := manager.Webhooks() + if configurationTotalWebhooks(tt.args.createWebhookConfigurations) != len(webhooks) { + t.Errorf("Expected number of webhooks %d received %d", + configurationTotalWebhooks(tt.args.createWebhookConfigurations), + len(webhooks), + ) + } // assert creations if tt.numberOfCreations != fakeWebhookAccessorCreator.calledNTimes() { t.Errorf( @@ -230,8 +247,10 @@ func TestGetValidatingWebhookConfigSmartReload(t *testing.T) { tt.numberOfCreations, fakeWebhookAccessorCreator.calledNTimes(), ) } + // reset mock counter fakeWebhookAccessorCreator.resetCounter() + // Update webhooks for _, configurations := range tt.args.updateWebhookConfigurations { client. @@ -242,7 +261,14 @@ func TestGetValidatingWebhookConfigSmartReload(t *testing.T) { // TODO use channels to wait for manager.createValidatingWebhookAccessor // to be called instead of using time.Sleep time.Sleep(1 * time.Second) - _ = manager.Webhooks() + webhooks = manager.Webhooks() + if tt.finalNumberOfWebhookAccessors != len(webhooks) { + t.Errorf("Expected final number of webhooks %d received %d", + tt.finalNumberOfWebhookAccessors, + len(webhooks), + ) + } + // assert updates if tt.numberOfRefreshes != fakeWebhookAccessorCreator.calledNTimes() { t.Errorf( diff --git a/staging/src/k8s.io/apiserver/pkg/admission/configuration/webhook_cache.go b/staging/src/k8s.io/apiserver/pkg/admission/configuration/webhook_cache.go deleted file mode 100644 index 49abac4d91d..00000000000 --- a/staging/src/k8s.io/apiserver/pkg/admission/configuration/webhook_cache.go +++ /dev/null @@ -1 +0,0 @@ -package configuration diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go index b12c69e1c7d..abea0f8d535 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go @@ -219,10 +219,13 @@ func NewValidatingWebhookAccessor(uid, configurationName string, h *v1.Validatin // NOTE: we're thinking about adding a new NewValidatingWebhookAccessorWithOptions to allow // recreating webhookAccessor with existing restClient, compiled CELs, selectors etc... to // avoid remaking unnecessary expensive operations. -/* + type Options struct { // rest client + // compiled CELs + namespaceSelector labels.Selector + // selectors... } @@ -230,7 +233,6 @@ type Options struct { func NewValidatingWebhookAccessorWithOptions(uid, configurationName string, h *v1.ValidatingWebhook, opt *Options) WebhookAccessor { return &validatingWebhookAccessor{uid: uid, configurationName: configurationName, ValidatingWebhook: h} } -*/ type validatingWebhookAccessor struct { *v1.ValidatingWebhook From aeefb762ece0f866e99def259d6714aa4deb6d31 Mon Sep 17 00:00:00 2001 From: Amine Date: Wed, 17 May 2023 18:42:56 -0500 Subject: [PATCH 05/11] Properly handle parameter in `shareInformer.DeleteFunc` --- .../validating_webhook_manager.go | 39 +++++++++++-------- .../pkg/admission/plugin/webhook/accessors.go | 18 --------- 2 files changed, 22 insertions(+), 35 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go b/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go index ecb70a1232a..f605eab4611 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go @@ -30,9 +30,11 @@ import ( admissionregistrationlisters "k8s.io/client-go/listers/admissionregistration/v1" "k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/cache/synctrack" + "k8s.io/klog/v2" ) -type webhookAccessorCreator func(uid string, configurationName string, h *v1.ValidatingWebhook) webhook.WebhookAccessor +// Type for test injection. +type validatingWebhookAccessorCreator func(uid string, configurationName string, h *v1.ValidatingWebhook) webhook.WebhookAccessor // validatingWebhookConfigurationManager collects the validating webhook objects so that they can be called. type validatingWebhookConfigurationManager struct { @@ -40,11 +42,10 @@ type validatingWebhookConfigurationManager struct { hasSynced func() bool lazy synctrack.Lazy[[]webhook.WebhookAccessor] configurationsCache sync.Map - // createValidatingWebhookAccessor is used to instantiate webhook accessors. - // This function is defined as field instead of a struct method to allow us - // to replace it with a mock during unit tests. - createValidatingWebhookAccessor webhookAccessorCreator + // This function is defined as field instead of a struct method to allow injection + // during tests + createValidatingWebhookAccessor validatingWebhookAccessorCreator } var _ generic.Source = &validatingWebhookConfigurationManager{} @@ -60,16 +61,25 @@ func NewValidatingWebhookConfigurationManager(f informers.SharedInformerFactory) handle, _ := informer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ AddFunc: func(_ interface{}) { manager.lazy.Notify() }, UpdateFunc: func(old, new interface{}) { - //TODO: we can still dig deeper and figure out which ones of - // the webhooks have changed + whether CEL expressions changed - // etc... thinking about using reflect.. obj := new.(*v1.ValidatingWebhookConfiguration) manager.configurationsCache.Delete(obj.GetName()) manager.lazy.Notify() }, - DeleteFunc: func(vwc interface{}) { - obj := vwc.(*v1.ValidatingWebhookConfiguration) - manager.configurationsCache.Delete(obj.Name) + DeleteFunc: func(obj interface{}) { + vwc, ok := obj.(*v1.ValidatingWebhookConfiguration) + if !ok { + tombstone, ok := obj.(cache.DeletedFinalStateUnknown) + if !ok { + klog.Errorf("Couldn't get object from tombstone %#v", obj) + return + } + vwc, ok = tombstone.Obj.(*v1.ValidatingWebhookConfiguration) + if !ok { + klog.Errorf("Tombstone contained object that is not expected %#v", obj) + return + } + } + manager.configurationsCache.Delete(vwc.Name) manager.lazy.Notify() }, }) @@ -87,7 +97,7 @@ func (v *validatingWebhookConfigurationManager) Webhooks() []webhook.WebhookAcce return out } -// HasSynced returns true if the initial set of mutating webhook configurations +// HasSynced returns true if the initial set of validating webhook configurations // has been loaded. func (v *validatingWebhookConfigurationManager) HasSynced() bool { return v.hasSynced() } @@ -112,11 +122,6 @@ func (v *validatingWebhookConfigurationManager) smartReloadValidatingWebhookConf // webhook names are not validated for uniqueness, so we check for duplicates and // add a int suffix to distinguish between them - // - // NOTE: In `pkg/apis/admissionregistration/validation.go` webhook names are checked - // for uniqueness now. Is it safe to remove this? - // If we ever get rid of this, we can change the cache map value type to - // map[string]WebhookAccessor instead. Where keys will be of format "config.name/webhook.name" names := map[string]int{} configurationAccessors := []webhook.WebhookAccessor{} for i := range c.Webhooks { diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go index abea0f8d535..6c48418de61 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go @@ -216,24 +216,6 @@ func NewValidatingWebhookAccessor(uid, configurationName string, h *v1.Validatin return &validatingWebhookAccessor{uid: uid, configurationName: configurationName, ValidatingWebhook: h} } -// NOTE: we're thinking about adding a new NewValidatingWebhookAccessorWithOptions to allow -// recreating webhookAccessor with existing restClient, compiled CELs, selectors etc... to -// avoid remaking unnecessary expensive operations. - -type Options struct { - // rest client - - // compiled CELs - namespaceSelector labels.Selector - - // selectors... -} - -// NewValidatingWebhookAccessorWithOptions creates an accessor for a ValidatingWebhook. -func NewValidatingWebhookAccessorWithOptions(uid, configurationName string, h *v1.ValidatingWebhook, opt *Options) WebhookAccessor { - return &validatingWebhookAccessor{uid: uid, configurationName: configurationName, ValidatingWebhook: h} -} - type validatingWebhookAccessor struct { *v1.ValidatingWebhook uid string From 1eb60939fe5eb4c1394e5d93ee2d00b5894e9e73 Mon Sep 17 00:00:00 2001 From: Amine Date: Wed, 17 May 2023 18:44:01 -0500 Subject: [PATCH 06/11] Add smart reload for `MutatingWebhooks` --- .../configuration/mutating_webhook_manager.go | 59 +++++- .../mutating_webhook_manager_test.go | 200 ++++++++++++++++++ 2 files changed, 250 insertions(+), 9 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go b/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go index daee6785991..db88e1ade31 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go @@ -19,6 +19,7 @@ package configuration import ( "fmt" "sort" + "sync" "k8s.io/api/admissionregistration/v1" "k8s.io/apimachinery/pkg/labels" @@ -29,13 +30,22 @@ import ( admissionregistrationlisters "k8s.io/client-go/listers/admissionregistration/v1" "k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/cache/synctrack" + "k8s.io/klog/v2" ) +// Type for test injection. +type mutatingWebhookAccessorCreator func(uid string, configurationName string, h *v1.MutatingWebhook) webhook.WebhookAccessor + // mutatingWebhookConfigurationManager collects the mutating webhook objects so that they can be called. type mutatingWebhookConfigurationManager struct { - lister admissionregistrationlisters.MutatingWebhookConfigurationLister - hasSynced func() bool - lazy synctrack.Lazy[[]webhook.WebhookAccessor] + lister admissionregistrationlisters.MutatingWebhookConfigurationLister + hasSynced func() bool + lazy synctrack.Lazy[[]webhook.WebhookAccessor] + configurationsCache sync.Map + // createMutatingWebhookAccessor is used to instantiate webhook accessors. + // This function is defined as field instead of a struct method to allow injection + // during tests + createMutatingWebhookAccessor mutatingWebhookAccessorCreator } var _ generic.Source = &mutatingWebhookConfigurationManager{} @@ -48,9 +58,29 @@ func NewMutatingWebhookConfigurationManager(f informers.SharedInformerFactory) g manager.lazy.Evaluate = manager.getConfiguration handle, _ := informer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ - AddFunc: func(_ interface{}) { manager.lazy.Notify() }, - UpdateFunc: func(_, _ interface{}) { manager.lazy.Notify() }, - DeleteFunc: func(_ interface{}) { manager.lazy.Notify() }, + AddFunc: func(_ interface{}) { manager.lazy.Notify() }, + UpdateFunc: func(old, new interface{}) { + obj := new.(*v1.MutatingWebhookConfiguration) + manager.configurationsCache.Delete(obj.GetName()) + manager.lazy.Notify() + }, + DeleteFunc: func(obj interface{}) { + vwc, ok := obj.(*v1.MutatingWebhookConfiguration) + if !ok { + tombstone, ok := obj.(cache.DeletedFinalStateUnknown) + if !ok { + klog.Errorf("Couldn't get object from tombstone %#v", obj) + return + } + vwc, ok = tombstone.Obj.(*v1.MutatingWebhookConfiguration) + if !ok { + klog.Errorf("Tombstone contained object that is not expected %#v", obj) + return + } + } + manager.configurationsCache.Delete(vwc.Name) + manager.lazy.Notify() + }, }) manager.hasSynced = handle.HasSynced @@ -75,25 +105,36 @@ func (m *mutatingWebhookConfigurationManager) getConfiguration() ([]webhook.Webh if err != nil { return []webhook.WebhookAccessor{}, err } - return mergeMutatingWebhookConfigurations(configurations), nil + return m.smartReloadMutatingWebhookConfigurations(configurations), nil } -func mergeMutatingWebhookConfigurations(configurations []*v1.MutatingWebhookConfiguration) []webhook.WebhookAccessor { +func (m *mutatingWebhookConfigurationManager) smartReloadMutatingWebhookConfigurations(configurations []*v1.MutatingWebhookConfiguration) []webhook.WebhookAccessor { // The internal order of webhooks for each configuration is provided by the user // but configurations themselves can be in any order. As we are going to run these // webhooks in serial, they are sorted here to have a deterministic order. sort.SliceStable(configurations, MutatingWebhookConfigurationSorter(configurations).ByName) accessors := []webhook.WebhookAccessor{} for _, c := range configurations { + cachedConfigurationAccessors, ok := m.configurationsCache.Load(c.Name) + if ok { + // Pick an already cached webhookAccessor + accessors = append(accessors, cachedConfigurationAccessors.([]webhook.WebhookAccessor)...) + continue + } + // webhook names are not validated for uniqueness, so we check for duplicates and // add a int suffix to distinguish between them names := map[string]int{} + configurationAccessors := []webhook.WebhookAccessor{} for i := range c.Webhooks { n := c.Webhooks[i].Name uid := fmt.Sprintf("%s/%s/%d", c.Name, n, names[n]) names[n]++ - accessors = append(accessors, webhook.NewMutatingWebhookAccessor(uid, c.Name, &c.Webhooks[i])) + configurationAccessor := m.createMutatingWebhookAccessor(uid, c.Name, &c.Webhooks[i]) + configurationAccessors = append(configurationAccessors, configurationAccessor) } + accessors = append(accessors, configurationAccessors...) + m.configurationsCache.Store(c.Name, configurationAccessors) } return accessors } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager_test.go b/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager_test.go index 79666edb51d..33aab738646 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager_test.go @@ -24,6 +24,7 @@ import ( "k8s.io/api/admissionregistration/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apiserver/pkg/admission/plugin/webhook" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes/fake" ) @@ -81,3 +82,202 @@ func TestGetMutatingWebhookConfig(t *testing.T) { } } } + +// mockCreateMutatingWebhookAccessor is a struct used to compute how many times +// the function webhook.NewMutatingWebhookAccessor is being called when refreshing +// webhookAccessors. +// +// NOTE: Maybe there some testing help that we can import and reuse instead. +type mockCreateMutatingWebhookAccessor struct { + numberOfCalls int +} + +func (mock *mockCreateMutatingWebhookAccessor) calledNTimes() int { return mock.numberOfCalls } +func (mock *mockCreateMutatingWebhookAccessor) resetCounter() { mock.numberOfCalls = 0 } +func (mock *mockCreateMutatingWebhookAccessor) incrementCounter() { mock.numberOfCalls++ } + +func (mock *mockCreateMutatingWebhookAccessor) fn(uid string, configurationName string, h *v1.MutatingWebhook) webhook.WebhookAccessor { + mock.incrementCounter() + return webhook.NewMutatingWebhookAccessor(uid, configurationName, h) +} + +func mutatingConfigurationTotalWebhooks(configurations []*v1.MutatingWebhookConfiguration) int { + total := 0 + for _, configuration := range configurations { + total += len(configuration.Webhooks) + } + return total +} + +func TestGetMutatingWebhookConfigSmartReload(t *testing.T) { + type args struct { + createWebhookConfigurations []*v1.MutatingWebhookConfiguration + updateWebhookConfigurations []*v1.MutatingWebhookConfiguration + } + tests := []struct { + name string + args args + numberOfCreations int + // number of refreshes are number of times we pulled a webhook configuration + // from the cache without having to create new ones from scratch. + numberOfRefreshes int + finalNumberOfWebhookAccessors int + }{ + { + name: "no creations and no updates", + args: args{ + nil, + nil, + }, + numberOfCreations: 0, + numberOfRefreshes: 0, + finalNumberOfWebhookAccessors: 0, + }, + { + name: "create configurations and no updates", + args: args{ + []*v1.MutatingWebhookConfiguration{ + &v1.MutatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "webhook1"}, + Webhooks: []v1.MutatingWebhook{{Name: "webhook1.1"}, {Name: "webhook1.2"}}, + }, + &v1.MutatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "webhook2"}, + Webhooks: []v1.MutatingWebhook{{Name: "webhook2.1"}, {Name: "webhook2.2"}}, + }, + }, + nil, + }, + numberOfCreations: 4, + numberOfRefreshes: 0, + finalNumberOfWebhookAccessors: 4, + }, + { + name: "create configurations and update some of them", + args: args{ + []*v1.MutatingWebhookConfiguration{ + &v1.MutatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "webhook3"}, + Webhooks: []v1.MutatingWebhook{{Name: "webhook3.1"}}, + }, + &v1.MutatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "webhook4"}, + Webhooks: []v1.MutatingWebhook{{Name: "webhook4.1"}}, + }, + }, + []*v1.MutatingWebhookConfiguration{ + &v1.MutatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "webhook4"}, + Webhooks: []v1.MutatingWebhook{{Name: "webhook4.1-updated"}, {Name: "webhook4.2"}}, + }, + }, + }, + numberOfCreations: 2, + numberOfRefreshes: 2, + finalNumberOfWebhookAccessors: 3, + }, + { + name: "create configuration and update moar of them", + args: args{ + []*v1.MutatingWebhookConfiguration{ + &v1.MutatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "webhook5"}, + Webhooks: []v1.MutatingWebhook{{Name: "webhook5.1"}, {Name: "webhook5.2"}}, + }, + &v1.MutatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "webhook6"}, + Webhooks: []v1.MutatingWebhook{{Name: "webhook6.1"}}, + }, + &v1.MutatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "webhook7"}, + Webhooks: []v1.MutatingWebhook{{Name: "webhook7.1"}}, + }, + }, + []*v1.MutatingWebhookConfiguration{ + &v1.MutatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "webhook6"}, + Webhooks: []v1.MutatingWebhook{{Name: "webhook6.1-updated"}}, + }, + &v1.MutatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "webhook7"}, + Webhooks: []v1.MutatingWebhook{{Name: "webhook7.1-updated"}, {Name: "webhook7.2"}}, + }, + }, + }, + numberOfCreations: 4, + numberOfRefreshes: 3, + finalNumberOfWebhookAccessors: 5, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + client := fake.NewSimpleClientset() + informerFactory := informers.NewSharedInformerFactory(client, 0) + stop := make(chan struct{}) + defer close(stop) + manager := NewMutatingWebhookConfigurationManager(informerFactory) + managerStructPtr := manager.(*mutatingWebhookConfigurationManager) + fakeWebhookAccessorCreator := &mockCreateMutatingWebhookAccessor{} + managerStructPtr.createMutatingWebhookAccessor = fakeWebhookAccessorCreator.fn + informerFactory.Start(stop) + informerFactory.WaitForCacheSync(stop) + + // Create webhooks + for _, configurations := range tt.args.createWebhookConfigurations { + client. + AdmissionregistrationV1(). + MutatingWebhookConfigurations(). + Create(context.TODO(), configurations, metav1.CreateOptions{}) + } + // TODO use channels to wait for manager.createMutatingWebhookAccessor + // to be called instead of using time.Sleep + time.Sleep(1 * time.Second) + webhooks := manager.Webhooks() + if mutatingConfigurationTotalWebhooks(tt.args.createWebhookConfigurations) != len(webhooks) { + t.Errorf("Expected number of webhooks %d received %d", + mutatingConfigurationTotalWebhooks(tt.args.createWebhookConfigurations), + len(webhooks), + ) + } + // assert creations + if tt.numberOfCreations != fakeWebhookAccessorCreator.calledNTimes() { + t.Errorf( + "Expected number of creations %d received %d", + tt.numberOfCreations, fakeWebhookAccessorCreator.calledNTimes(), + ) + } + + // reset mock counter + fakeWebhookAccessorCreator.resetCounter() + + // Update webhooks + for _, configurations := range tt.args.updateWebhookConfigurations { + client. + AdmissionregistrationV1(). + MutatingWebhookConfigurations(). + Update(context.TODO(), configurations, metav1.UpdateOptions{}) + } + // TODO use channels to wait for manager.createMutatingWebhookAccessor + // to be called instead of using time.Sleep + time.Sleep(1 * time.Second) + webhooks = manager.Webhooks() + if tt.finalNumberOfWebhookAccessors != len(webhooks) { + t.Errorf("Expected final number of webhooks %d received %d", + tt.finalNumberOfWebhookAccessors, + len(webhooks), + ) + } + + // assert updates + if tt.numberOfRefreshes != fakeWebhookAccessorCreator.calledNTimes() { + t.Errorf( + "Expected number of refreshes %d received %d", + tt.numberOfRefreshes, fakeWebhookAccessorCreator.calledNTimes(), + ) + } + // reset mock counter for the next test cases + fakeWebhookAccessorCreator.resetCounter() + }) + } +} From 747dbd9b6b72c21c94ddd17bd2c82bbb2ff583ad Mon Sep 17 00:00:00 2001 From: Amine Date: Wed, 17 May 2023 18:52:37 -0500 Subject: [PATCH 07/11] run `./hack/verify-gofmt.sh` --- .../mutating_webhook_manager_test.go | 20 +++++++++---------- .../validating_webhook_manager_test.go | 20 +++++++++---------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager_test.go b/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager_test.go index 33aab738646..8f9553f7812 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager_test.go @@ -137,11 +137,11 @@ func TestGetMutatingWebhookConfigSmartReload(t *testing.T) { name: "create configurations and no updates", args: args{ []*v1.MutatingWebhookConfiguration{ - &v1.MutatingWebhookConfiguration{ + { ObjectMeta: metav1.ObjectMeta{Name: "webhook1"}, Webhooks: []v1.MutatingWebhook{{Name: "webhook1.1"}, {Name: "webhook1.2"}}, }, - &v1.MutatingWebhookConfiguration{ + { ObjectMeta: metav1.ObjectMeta{Name: "webhook2"}, Webhooks: []v1.MutatingWebhook{{Name: "webhook2.1"}, {Name: "webhook2.2"}}, }, @@ -156,17 +156,17 @@ func TestGetMutatingWebhookConfigSmartReload(t *testing.T) { name: "create configurations and update some of them", args: args{ []*v1.MutatingWebhookConfiguration{ - &v1.MutatingWebhookConfiguration{ + { ObjectMeta: metav1.ObjectMeta{Name: "webhook3"}, Webhooks: []v1.MutatingWebhook{{Name: "webhook3.1"}}, }, - &v1.MutatingWebhookConfiguration{ + { ObjectMeta: metav1.ObjectMeta{Name: "webhook4"}, Webhooks: []v1.MutatingWebhook{{Name: "webhook4.1"}}, }, }, []*v1.MutatingWebhookConfiguration{ - &v1.MutatingWebhookConfiguration{ + { ObjectMeta: metav1.ObjectMeta{Name: "webhook4"}, Webhooks: []v1.MutatingWebhook{{Name: "webhook4.1-updated"}, {Name: "webhook4.2"}}, }, @@ -180,25 +180,25 @@ func TestGetMutatingWebhookConfigSmartReload(t *testing.T) { name: "create configuration and update moar of them", args: args{ []*v1.MutatingWebhookConfiguration{ - &v1.MutatingWebhookConfiguration{ + { ObjectMeta: metav1.ObjectMeta{Name: "webhook5"}, Webhooks: []v1.MutatingWebhook{{Name: "webhook5.1"}, {Name: "webhook5.2"}}, }, - &v1.MutatingWebhookConfiguration{ + { ObjectMeta: metav1.ObjectMeta{Name: "webhook6"}, Webhooks: []v1.MutatingWebhook{{Name: "webhook6.1"}}, }, - &v1.MutatingWebhookConfiguration{ + { ObjectMeta: metav1.ObjectMeta{Name: "webhook7"}, Webhooks: []v1.MutatingWebhook{{Name: "webhook7.1"}}, }, }, []*v1.MutatingWebhookConfiguration{ - &v1.MutatingWebhookConfiguration{ + { ObjectMeta: metav1.ObjectMeta{Name: "webhook6"}, Webhooks: []v1.MutatingWebhook{{Name: "webhook6.1-updated"}}, }, - &v1.MutatingWebhookConfiguration{ + { ObjectMeta: metav1.ObjectMeta{Name: "webhook7"}, Webhooks: []v1.MutatingWebhook{{Name: "webhook7.1-updated"}, {Name: "webhook7.2"}}, }, diff --git a/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager_test.go b/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager_test.go index 435211e74d6..18493a54f5f 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager_test.go @@ -137,11 +137,11 @@ func TestGetValidatingWebhookConfigSmartReload(t *testing.T) { name: "create configurations and no updates", args: args{ []*v1.ValidatingWebhookConfiguration{ - &v1.ValidatingWebhookConfiguration{ + { ObjectMeta: metav1.ObjectMeta{Name: "webhook1"}, Webhooks: []v1.ValidatingWebhook{{Name: "webhook1.1"}}, }, - &v1.ValidatingWebhookConfiguration{ + { ObjectMeta: metav1.ObjectMeta{Name: "webhook2"}, Webhooks: []v1.ValidatingWebhook{{Name: "webhook2.1"}}, }, @@ -156,17 +156,17 @@ func TestGetValidatingWebhookConfigSmartReload(t *testing.T) { name: "create configurations and update some of them", args: args{ []*v1.ValidatingWebhookConfiguration{ - &v1.ValidatingWebhookConfiguration{ + { ObjectMeta: metav1.ObjectMeta{Name: "webhook3"}, Webhooks: []v1.ValidatingWebhook{{Name: "webhook3.1"}}, }, - &v1.ValidatingWebhookConfiguration{ + { ObjectMeta: metav1.ObjectMeta{Name: "webhook4"}, Webhooks: []v1.ValidatingWebhook{{Name: "webhook4.1"}}, }, }, []*v1.ValidatingWebhookConfiguration{ - &v1.ValidatingWebhookConfiguration{ + { ObjectMeta: metav1.ObjectMeta{Name: "webhook3"}, Webhooks: []v1.ValidatingWebhook{{Name: "webhook3.1-updated"}}, }, @@ -180,25 +180,25 @@ func TestGetValidatingWebhookConfigSmartReload(t *testing.T) { name: "create configuration and update moar of them", args: args{ []*v1.ValidatingWebhookConfiguration{ - &v1.ValidatingWebhookConfiguration{ + { ObjectMeta: metav1.ObjectMeta{Name: "webhook5"}, Webhooks: []v1.ValidatingWebhook{{Name: "webhook5.1"}, {Name: "webhook5.2"}}, }, - &v1.ValidatingWebhookConfiguration{ + { ObjectMeta: metav1.ObjectMeta{Name: "webhook6"}, Webhooks: []v1.ValidatingWebhook{{Name: "webhook6.1"}}, }, - &v1.ValidatingWebhookConfiguration{ + { ObjectMeta: metav1.ObjectMeta{Name: "webhook7"}, Webhooks: []v1.ValidatingWebhook{{Name: "webhook7.1"}, {Name: "webhook7.1"}}, }, }, []*v1.ValidatingWebhookConfiguration{ - &v1.ValidatingWebhookConfiguration{ + { ObjectMeta: metav1.ObjectMeta{Name: "webhook5"}, Webhooks: []v1.ValidatingWebhook{{Name: "webhook5.1-updated"}}, }, - &v1.ValidatingWebhookConfiguration{ + { ObjectMeta: metav1.ObjectMeta{Name: "webhook7"}, Webhooks: []v1.ValidatingWebhook{{Name: "webhook7.1-updated"}, {Name: "webhook7.2-updated"}, {Name: "webhook7.3"}}, }, From 761016482da9e3febbbfe5ab3cf6c2b31692c1b0 Mon Sep 17 00:00:00 2001 From: Amine Date: Wed, 17 May 2023 22:44:18 -0500 Subject: [PATCH 08/11] Properly setup mutatingWebhookConfigurationManager{} --- .../pkg/admission/configuration/mutating_webhook_manager.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go b/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go index db88e1ade31..76c11f4a618 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go @@ -53,7 +53,8 @@ var _ generic.Source = &mutatingWebhookConfigurationManager{} func NewMutatingWebhookConfigurationManager(f informers.SharedInformerFactory) generic.Source { informer := f.Admissionregistration().V1().MutatingWebhookConfigurations() manager := &mutatingWebhookConfigurationManager{ - lister: informer.Lister(), + lister: informer.Lister(), + createMutatingWebhookAccessor: webhook.NewMutatingWebhookAccessor, } manager.lazy.Evaluate = manager.getConfiguration From 28b6c90696680c87cab04b539c0bca6af7d66bac Mon Sep 17 00:00:00 2001 From: Amine Date: Wed, 24 May 2023 13:50:50 -0500 Subject: [PATCH 09/11] Move DeleteFunc logging to level 2 --- .../pkg/admission/configuration/mutating_webhook_manager.go | 4 ++-- .../pkg/admission/configuration/validating_webhook_manager.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go b/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go index 76c11f4a618..851490c66a1 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go @@ -70,12 +70,12 @@ func NewMutatingWebhookConfigurationManager(f informers.SharedInformerFactory) g if !ok { tombstone, ok := obj.(cache.DeletedFinalStateUnknown) if !ok { - klog.Errorf("Couldn't get object from tombstone %#v", obj) + klog.V(2).Infof("Couldn't get object from tombstone %#v", obj) return } vwc, ok = tombstone.Obj.(*v1.MutatingWebhookConfiguration) if !ok { - klog.Errorf("Tombstone contained object that is not expected %#v", obj) + klog.V(2).Infof("Tombstone contained object that is not expected %#v", obj) return } } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go b/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go index f605eab4611..d57ce712129 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go @@ -70,12 +70,12 @@ func NewValidatingWebhookConfigurationManager(f informers.SharedInformerFactory) if !ok { tombstone, ok := obj.(cache.DeletedFinalStateUnknown) if !ok { - klog.Errorf("Couldn't get object from tombstone %#v", obj) + klog.V(2).Infof("Couldn't get object from tombstone %#v", obj) return } vwc, ok = tombstone.Obj.(*v1.ValidatingWebhookConfiguration) if !ok { - klog.Errorf("Tombstone contained object that is not expected %#v", obj) + klog.V(2).Infof("Tombstone contained object that is not expected %#v", obj) return } } From 0695853a3061ece0f602c1f267c82ced3f8c880d Mon Sep 17 00:00:00 2001 From: Amine Date: Wed, 12 Jul 2023 16:20:14 +0100 Subject: [PATCH 10/11] Improve naming and code comments --- .../configuration/mutating_webhook_manager.go | 11 ++++++++--- .../configuration/mutating_webhook_manager_test.go | 4 ++-- .../configuration/validating_webhook_manager.go | 9 +++++++-- .../configuration/validating_webhook_manager_test.go | 4 ++-- .../pkg/admission/plugin/webhook/accessors.go | 1 - 5 files changed, 19 insertions(+), 10 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go b/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go index 851490c66a1..a1b84780f2b 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go @@ -21,7 +21,7 @@ import ( "sort" "sync" - "k8s.io/api/admissionregistration/v1" + v1 "k8s.io/api/admissionregistration/v1" "k8s.io/apimachinery/pkg/labels" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apiserver/pkg/admission/plugin/webhook" @@ -106,10 +106,15 @@ func (m *mutatingWebhookConfigurationManager) getConfiguration() ([]webhook.Webh if err != nil { return []webhook.WebhookAccessor{}, err } - return m.smartReloadMutatingWebhookConfigurations(configurations), nil + return m.getMutatingWebhookConfigurations(configurations), nil } -func (m *mutatingWebhookConfigurationManager) smartReloadMutatingWebhookConfigurations(configurations []*v1.MutatingWebhookConfiguration) []webhook.WebhookAccessor { +// getMutatingWebhookConfigurations returns the webhook accessors for a given list of +// mutating webhook configurations. +// +// This function will, first, try to load the webhook accessors from the cache and avoid +// recreating them, which can be expessive (requiring CEL expression recompilation). +func (m *mutatingWebhookConfigurationManager) getMutatingWebhookConfigurations(configurations []*v1.MutatingWebhookConfiguration) []webhook.WebhookAccessor { // The internal order of webhooks for each configuration is provided by the user // but configurations themselves can be in any order. As we are going to run these // webhooks in serial, they are sorted here to have a deterministic order. diff --git a/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager_test.go b/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager_test.go index 8f9553f7812..801c4ecaa35 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager_test.go @@ -118,8 +118,8 @@ func TestGetMutatingWebhookConfigSmartReload(t *testing.T) { name string args args numberOfCreations int - // number of refreshes are number of times we pulled a webhook configuration - // from the cache without having to create new ones from scratch. + // number of refreshes are number of times we recrated a webhook accessor + // instead of pulling from the cache. numberOfRefreshes int finalNumberOfWebhookAccessors int }{ diff --git a/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go b/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go index d57ce712129..d9c11a9755b 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go @@ -106,10 +106,15 @@ func (v *validatingWebhookConfigurationManager) getConfiguration() ([]webhook.We if err != nil { return []webhook.WebhookAccessor{}, err } - return v.smartReloadValidatingWebhookConfigurations(configurations), nil + return v.getValidatingWebhookConfigurations(configurations), nil } -func (v *validatingWebhookConfigurationManager) smartReloadValidatingWebhookConfigurations(configurations []*v1.ValidatingWebhookConfiguration) []webhook.WebhookAccessor { +// getMutatingWebhookConfigurations returns the webhook accessors for a given list of +// mutating webhook configurations. +// +// This function will, first, try to load the webhook accessors from the cache and avoid +// recreating them, which can be expessive (requiring CEL expression recompilation). +func (v *validatingWebhookConfigurationManager) getValidatingWebhookConfigurations(configurations []*v1.ValidatingWebhookConfiguration) []webhook.WebhookAccessor { sort.SliceStable(configurations, ValidatingWebhookConfigurationSorter(configurations).ByName) accessors := []webhook.WebhookAccessor{} for _, c := range configurations { diff --git a/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager_test.go b/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager_test.go index 18493a54f5f..2f18f386bb4 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager_test.go @@ -118,8 +118,8 @@ func TestGetValidatingWebhookConfigSmartReload(t *testing.T) { name string args args numberOfCreations int - // number of refreshes are number of times we pulled a webhook configuration - // from the cache without having to create new ones from scratch. + // number of refreshes are number of times we recrated a webhook accessor + // instead of pulling from the cache. numberOfRefreshes int finalNumberOfWebhookAccessors int }{ diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go index 6c48418de61..88d8120f545 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go @@ -123,7 +123,6 @@ func (m *mutatingWebhookAccessor) GetRESTClient(clientManager *webhookutil.Clien return m.client, m.clientErr } -// TODO: graduation to beta: resolve the fact that we rebuild ALL items whenever ANY config changes in NewMutatingWebhookConfigurationManager and NewValidatingWebhookConfigurationManager ... now that we're doing CEL compilation, we probably want to avoid that func (m *mutatingWebhookAccessor) GetCompiledMatcher(compiler cel.FilterCompiler) matchconditions.Matcher { m.compileMatcher.Do(func() { expressions := make([]cel.ExpressionAccessor, len(m.MutatingWebhook.MatchConditions)) From 49d03468021e24434171fde5458df34f6a753a32 Mon Sep 17 00:00:00 2001 From: Amine Date: Thu, 13 Jul 2023 23:43:12 +0100 Subject: [PATCH 11/11] Pre-allocate webhook accessors arrays for mutating and validating webhooks --- .../admission/configuration/mutating_webhook_manager.go | 9 +++++++-- .../configuration/validating_webhook_manager.go | 9 +++++++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go b/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go index a1b84780f2b..3ecc00b74cb 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go @@ -119,7 +119,12 @@ func (m *mutatingWebhookConfigurationManager) getMutatingWebhookConfigurations(c // but configurations themselves can be in any order. As we are going to run these // webhooks in serial, they are sorted here to have a deterministic order. sort.SliceStable(configurations, MutatingWebhookConfigurationSorter(configurations).ByName) - accessors := []webhook.WebhookAccessor{} + size := 0 + for _, cfg := range configurations { + size += len(cfg.Webhooks) + } + accessors := make([]webhook.WebhookAccessor, 0, size) + for _, c := range configurations { cachedConfigurationAccessors, ok := m.configurationsCache.Load(c.Name) if ok { @@ -131,7 +136,7 @@ func (m *mutatingWebhookConfigurationManager) getMutatingWebhookConfigurations(c // webhook names are not validated for uniqueness, so we check for duplicates and // add a int suffix to distinguish between them names := map[string]int{} - configurationAccessors := []webhook.WebhookAccessor{} + configurationAccessors := make([]webhook.WebhookAccessor, 0, len(c.Webhooks)) for i := range c.Webhooks { n := c.Webhooks[i].Name uid := fmt.Sprintf("%s/%s/%d", c.Name, n, names[n]) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go b/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go index d9c11a9755b..b4233211770 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go @@ -116,7 +116,12 @@ func (v *validatingWebhookConfigurationManager) getConfiguration() ([]webhook.We // recreating them, which can be expessive (requiring CEL expression recompilation). func (v *validatingWebhookConfigurationManager) getValidatingWebhookConfigurations(configurations []*v1.ValidatingWebhookConfiguration) []webhook.WebhookAccessor { sort.SliceStable(configurations, ValidatingWebhookConfigurationSorter(configurations).ByName) - accessors := []webhook.WebhookAccessor{} + size := 0 + for _, cfg := range configurations { + size += len(cfg.Webhooks) + } + accessors := make([]webhook.WebhookAccessor, 0, size) + for _, c := range configurations { cachedConfigurationAccessors, ok := v.configurationsCache.Load(c.Name) if ok { @@ -128,7 +133,7 @@ func (v *validatingWebhookConfigurationManager) getValidatingWebhookConfiguratio // webhook names are not validated for uniqueness, so we check for duplicates and // add a int suffix to distinguish between them names := map[string]int{} - configurationAccessors := []webhook.WebhookAccessor{} + configurationAccessors := make([]webhook.WebhookAccessor, 0, len(c.Webhooks)) for i := range c.Webhooks { n := c.Webhooks[i].Name uid := fmt.Sprintf("%s/%s/%d", c.Name, n, names[n])