From 62bfd6b964997786613d1d882d0a3bb55d26a52b Mon Sep 17 00:00:00 2001 From: Antonin Bas Date: Mon, 1 Apr 2024 10:53:31 -0700 Subject: [PATCH] Fix possible race condition in SelfSignedCertProvider tests (#6166) I have seen some cases in Github CI where it took slightly over 2 seconds for the Secret update to be processed in TestSelfSignedCertProviderRotate. The test ended up failing. The most likely cause is the well-known race condition between List and Watch for informers when using the fake client. If the Secret Update happens concurrently with the Informer starting, right between the List and the Watch operations, we may lose the Update event. We call WaitForCacheSync to avoid race conditions. To be more conservative, we also increase the wait duration for assertions to 5 seconds. Signed-off-by: Antonin Bas --- .../certificate/selfsignedcert_provider.go | 1 + .../certificate/selfsignedcert_provider_test.go | 13 ++++++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/pkg/apiserver/certificate/selfsignedcert_provider.go b/pkg/apiserver/certificate/selfsignedcert_provider.go index 6074b2f205a..2ed9ccb53f9 100644 --- a/pkg/apiserver/certificate/selfsignedcert_provider.go +++ b/pkg/apiserver/certificate/selfsignedcert_provider.go @@ -150,6 +150,7 @@ func (p *selfSignedCertProvider) Run(ctx context.Context, workers int) { if p.secretInformer != nil { go p.secretInformer.Run(ctx.Done()) + cache.WaitForCacheSync(ctx.Done(), p.secretInformer.HasSynced) } // doesn't matter what workers say, only start one. diff --git a/pkg/apiserver/certificate/selfsignedcert_provider_test.go b/pkg/apiserver/certificate/selfsignedcert_provider_test.go index 2123f3ab307..ea72911ff3b 100644 --- a/pkg/apiserver/certificate/selfsignedcert_provider_test.go +++ b/pkg/apiserver/certificate/selfsignedcert_provider_test.go @@ -28,6 +28,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" genericoptions "k8s.io/apiserver/pkg/server/options" fakeclientset "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/tools/cache" certutil "k8s.io/client-go/util/cert" clocktesting "k8s.io/utils/clock/testing" @@ -124,6 +125,9 @@ func TestSelfSignedCertProviderRotate(t *testing.T) { }, gotSecret, "Secret doesn't match") go p.Run(ctx, 1) + // Wait for cache to have synced to make sure that no event is lost in case the Secret + // update happens between the List and Watch operations in the informer. + cache.WaitForCacheSync(ctx.Done(), p.secretInformer.HasSynced) // Update the Secret, it should update the serving one. gotSecret.Data[corev1.TLSCertKey] = testOneYearCert @@ -135,7 +139,7 @@ func TestSelfSignedCertProviderRotate(t *testing.T) { keyInFile, _ := os.ReadFile(p.secureServing.ServerCert.CertKey.KeyFile) assert.Equal(c, testOneYearCert, certInFile) assert.Equal(c, testOneYearKey, keyInFile) - }, 2*time.Second, 50*time.Millisecond) + }, 5*time.Second, 100*time.Millisecond) // Trigger a resync, nothing should change. p.enqueue() @@ -161,7 +165,7 @@ func TestSelfSignedCertProviderRotate(t *testing.T) { corev1.TLSCertKey: testOneYearCert, corev1.TLSPrivateKeyKey: testOneYearKey, }, gotSecret.Data, "Secret should not match") - }, 2*time.Second, 50*time.Millisecond) + }, 5*time.Second, 100*time.Millisecond) } func TestSelfSignedCertProviderRun(t *testing.T) { @@ -276,6 +280,9 @@ func TestSelfSignedCertProviderRun(t *testing.T) { } p := newTestSelfSignedCertProvider(t, client, tt.tlsSecretName, tt.minValidDuration, withGenerateSelfSignedCertKeyFn(generateSelfSignedCertKey)) go p.Run(ctx, 1) + if tt.tlsSecretName != "" { + cache.WaitForCacheSync(ctx.Done(), p.secretInformer.HasSynced) + } if tt.updatedSecret != nil { client.CoreV1().Secrets(tt.updatedSecret.Namespace).Update(ctx, tt.updatedSecret, metav1.UpdateOptions{}) } @@ -289,7 +296,7 @@ func TestSelfSignedCertProviderRun(t *testing.T) { keyInFile, _ := os.ReadFile(p.secureServing.ServerCert.CertKey.KeyFile) assert.Equal(c, tt.expectedCert, certInFile) assert.Equal(c, tt.expectedKey, keyInFile) - }, 2*time.Second, 50*time.Millisecond) + }, 5*time.Second, 100*time.Millisecond) }) } }