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