Add smart reload for `MutatingWebhooks`

Kubernetes-commit: 1eb60939fe5eb4c1394e5d93ee2d00b5894e9e73

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]
+	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{}
@@ -49,8 +59,28 @@ func NewMutatingWebhookConfigurationManager(f informers.SharedInformerFactory) g
 
 	handle, _ := informer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
 		AddFunc: func(_ interface{}) { manager.lazy.Notify() },
-		UpdateFunc: func(_, _ interface{}) { manager.lazy.Notify() },
+		UpdateFunc: func(old, new interface{}) {
-		DeleteFunc: func(_ interface{}) { manager.lazy.Notify() },
+			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
 }

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()
+		})
+	}
+}