From 33301aec543ad58cd342a8d04f52a6517e55bb9e Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Thu, 25 Dec 2025 19:44:24 +0100 Subject: [PATCH] webhook admission: avoid risk of flakes The unit test flaked at least once so far in the CI. Cannot be reproduced locally, though. === RUN TestGetMutatingWebhookConfigSmartReload/create_configuration_and_update_moar_of_them mutating_webhook_manager_test.go:238: Expected number of webhooks 4 received 0 mutating_webhook_manager_test.go:245: Expected number of creations 4 received 0 mutating_webhook_manager_test.go:266: Expected final number of webhooks 5 received 3 --- FAIL: TestGetMutatingWebhookConfigSmartReload/create_configuration_and_update_moar_of_them (2.00s) There were two potential root causes: - The watch must be set up completely before creating objects (a fake client-go limitation, fix separately). - The delay might be too small on a loaded system. By running the tests in a synctest bubble with synctest.Wait calls at the right places both problems are avoided. As a welcome side effect the test also completes faster. Before: ok k8s.io/apiserver/pkg/admission/configuration 21.036s After: ok k8s.io/apiserver/pkg/admission/configuration 4.029s 2m0s: 13801 runs so far, 0 failures --- .../mutating_webhook_manager_test.go | 147 +++++++++--------- .../validating_webhook_manager_test.go | 143 ++++++++--------- 2 files changed, 139 insertions(+), 151 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 801c4ecaa35..383d0084cd5 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 @@ -20,16 +20,17 @@ import ( "context" "reflect" "testing" - "time" + "testing/synctest" - "k8s.io/api/admissionregistration/v1" + v1 "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" ) -func TestGetMutatingWebhookConfig(t *testing.T) { +func TestGetMutatingWebhookConfig(t *testing.T) { synctest.Test(t, testGetMutatingWebhookConfig) } +func testGetMutatingWebhookConfig(t *testing.T) { // Build a test client that the admission plugin can use to look up the MutatingWebhookConfiguration client := fake.NewSimpleClientset() informerFactory := informers.NewSharedInformerFactory(client, 0) @@ -40,6 +41,9 @@ func TestGetMutatingWebhookConfig(t *testing.T) { informerFactory.Start(stop) informerFactory.WaitForCacheSync(stop) + // Wait for background activity to settle. + synctest.Wait() + // no configurations if configurations := manager.Webhooks(); len(configurations) != 0 { t.Errorf("expected empty webhooks, but got %v", configurations) @@ -55,19 +59,10 @@ func TestGetMutatingWebhookConfig(t *testing.T) { MutatingWebhookConfigurations(). Create(context.TODO(), webhookConfiguration, metav1.CreateOptions{}) - // Wait up to 10s for the notification to be delivered. - // (on my system this takes < 2ms) - startTime := time.Now() + // Wait for the notification to be delivered. + synctest.Wait() configurations := manager.Webhooks() - for len(configurations) == 0 { - if time.Since(startTime) > 10*time.Second { - break - } - time.Sleep(time.Millisecond) - configurations = manager.Webhooks() - } - // verify presence if len(configurations) == 0 { t.Errorf("expected non empty webhooks") } @@ -212,72 +207,72 @@ func TestGetMutatingWebhookConfigSmartReload(t *testing.T) { 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) + synctest.Test(t, 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(), - ) - } + // Create webhooks + for _, configurations := range tt.args.createWebhookConfigurations { + client. + AdmissionregistrationV1(). + MutatingWebhookConfigurations(). + Create(context.TODO(), configurations, metav1.CreateOptions{}) + } - // reset mock counter - fakeWebhookAccessorCreator.resetCounter() + synctest.Wait() + 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(), + ) + } - // 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), - ) - } + // reset mock counter + fakeWebhookAccessorCreator.resetCounter() - // 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() + // Update webhooks + for _, configurations := range tt.args.updateWebhookConfigurations { + client. + AdmissionregistrationV1(). + MutatingWebhookConfigurations(). + Update(context.TODO(), configurations, metav1.UpdateOptions{}) + } + + synctest.Wait() + 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() + }) }) } } 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 2f18f386bb4..9446f11b13e 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 @@ -20,16 +20,17 @@ import ( "context" "reflect" "testing" - "time" + "testing/synctest" - "k8s.io/api/admissionregistration/v1" + v1 "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" ) -func TestGetValidatingWebhookConfig(t *testing.T) { +func TestGetValidatingWebhookConfig(t *testing.T) { synctest.Test(t, testGetValidatingWebhookConfig) } +func testGetValidatingWebhookConfig(t *testing.T) { // Build a test client that the admission plugin can use to look up the ValidatingWebhookConfiguration client := fake.NewSimpleClientset() informerFactory := informers.NewSharedInformerFactory(client, 0) @@ -41,6 +42,7 @@ func TestGetValidatingWebhookConfig(t *testing.T) { informerFactory.WaitForCacheSync(stop) // no configurations + synctest.Wait() if configurations := manager.Webhooks(); len(configurations) != 0 { t.Errorf("expected empty webhooks, but got %v", configurations) } @@ -55,17 +57,9 @@ func TestGetValidatingWebhookConfig(t *testing.T) { ValidatingWebhookConfigurations(). Create(context.TODO(), webhookConfiguration, metav1.CreateOptions{}) - // Wait up to 10s for the notification to be delivered. - // (on my system this takes < 2ms) - startTime := time.Now() + // Wait for the notification to be delivered. + synctest.Wait() configurations := manager.Webhooks() - for len(configurations) == 0 { - if time.Since(startTime) > 10*time.Second { - break - } - time.Sleep(time.Millisecond) - configurations = manager.Webhooks() - } // verify presence if len(configurations) == 0 { @@ -212,72 +206,71 @@ func TestGetValidatingWebhookConfigSmartReload(t *testing.T) { 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) + synctest.Test(t, 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. - 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) - 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( - "Expected number of creations %d received %d", - tt.numberOfCreations, fakeWebhookAccessorCreator.calledNTimes(), - ) - } + // Create webhooks + for _, configurations := range tt.args.createWebhookConfigurations { + client. + AdmissionregistrationV1(). + ValidatingWebhookConfigurations(). + Create(context.TODO(), configurations, metav1.CreateOptions{}) + } - // reset mock counter - fakeWebhookAccessorCreator.resetCounter() + synctest.Wait() + 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( + "Expected number of creations %d received %d", + tt.numberOfCreations, fakeWebhookAccessorCreator.calledNTimes(), + ) + } - // 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) - webhooks = manager.Webhooks() - if tt.finalNumberOfWebhookAccessors != len(webhooks) { - t.Errorf("Expected final number of webhooks %d received %d", - tt.finalNumberOfWebhookAccessors, - len(webhooks), - ) - } + // reset mock counter + fakeWebhookAccessorCreator.resetCounter() - // 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() + // Update webhooks + for _, configurations := range tt.args.updateWebhookConfigurations { + client. + AdmissionregistrationV1(). + ValidatingWebhookConfigurations(). + Update(context.TODO(), configurations, metav1.UpdateOptions{}) + } + synctest.Wait() + 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() + }) }) } }