diff --git a/pkg/admission/configuration/mutating_webhook_manager.go b/pkg/admission/configuration/mutating_webhook_manager.go index daee67859..db88e1ade 100644 --- a/pkg/admission/configuration/mutating_webhook_manager.go +++ b/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/pkg/admission/configuration/mutating_webhook_manager_test.go b/pkg/admission/configuration/mutating_webhook_manager_test.go index 79666edb5..33aab7386 100644 --- a/pkg/admission/configuration/mutating_webhook_manager_test.go +++ b/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() + }) + } +}