Fix webhook accessors caching pattern

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]))
+			configurationAccessor := v.createValidatingWebhookAccessor(uid, c.Name, &c.Webhooks[i])
-			// Cache the new accessors
+			configurationAccessors = append(configurationAccessors, configurationAccessor)
-			v.configurationsCache.Store(c.Name, configurationAccessors)
-			accessors = append(accessors, configurationAccessors...)
 		}
+		accessors = append(accessors, configurationAccessors...)
+		v.configurationsCache.Store(c.Name, configurationAccessors)
 	}
 
 	return accessors

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
@@ -113,6 +121,7 @@ func TestGetValidatingWebhookConfigSmartReload(t *testing.T) {
 		// 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",
@@ -122,6 +131,7 @@ func TestGetValidatingWebhookConfigSmartReload(t *testing.T) {
 			},
 			numberOfCreations:             0,
 			numberOfRefreshes:             0,
+			finalNumberOfWebhookAccessors: 0,
 		},
 		{
 			name: "create configurations and no updates",
@@ -140,9 +150,10 @@ func TestGetValidatingWebhookConfigSmartReload(t *testing.T) {
 			},
 			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{
@@ -163,6 +174,7 @@ func TestGetValidatingWebhookConfigSmartReload(t *testing.T) {
 			},
 			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,
+			numberOfRefreshes:             4,
+			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 := 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.
@@ -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(

staging/src/k8s.io/apiserver/pkg/admission/configuration/webhook_cache.go

@@ -1 +0,0 @@
-package configuration

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