Skip to content

Commit

Permalink
Fix possible race condition in SelfSignedCertProvider tests (antrea-i…
Browse files Browse the repository at this point in the history
…o#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 <[email protected]>
  • Loading branch information
antoninbas authored Apr 1, 2024
1 parent b9e19f4 commit 62bfd6b
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 3 deletions.
1 change: 1 addition & 0 deletions pkg/apiserver/certificate/selfsignedcert_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
13 changes: 10 additions & 3 deletions pkg/apiserver/certificate/selfsignedcert_provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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
Expand All @@ -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()
Expand All @@ -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) {
Expand Down Expand Up @@ -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{})
}
Expand All @@ -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)
})
}
}

0 comments on commit 62bfd6b

Please sign in to comment.