From d2dc85f7124be348da7b619cc2f584bc569cfdf5 Mon Sep 17 00:00:00 2001 From: IvanHunters Date: Fri, 7 Aug 2026 16:36:42 +0300 Subject: [PATCH] refactor(kube-ovn): reload webhook cert via a plain GetCertificate closure Collapse the certReloader (mtime tracking, serialized refresh, retry backoff, rate-limited logging, last-good-certificate fallback) into a GetCertificate callback that simply re-reads the key pair from disk on every handshake. The signature is unchanged, so main.go is untouched. The extra machinery guarded scenarios this deployment cannot produce: - Torn reads are unreachable: the cert/key files come from a plain Secret volume with no subPath, and Kubernetes' atomic writer swaps the whole ..data directory via a single symlink flip, so a reader always sees a complete old or complete new generation of both files. - The per-handshake LoadX509KeyPair cost is negligible next to the asymmetric crypto the handshake already performs, so no cache is needed. - The webhook configures no mTLS, so GetCertificate not refreshing client CA pools does not apply. If the mounted files genuinely become unreadable (an operator error, not a renewal) the handshake now fails loudly instead of silently serving a stale certificate. The core regression test is retained and remains non-vacuous: it fails against a static tls.Config.Certificates and passes with the reloading callback. Signed-off-by: IvanHunters (cherry picked from commit 8baa449f53f6410a348ce2e7e7ea89393f1c431d) --- .../images/kubeovn-webhook/cert.go | 182 ++++-------------- .../images/kubeovn-webhook/cert_test.go | 106 ---------- 2 files changed, 36 insertions(+), 252 deletions(-) diff --git a/packages/system/kubeovn-webhook/images/kubeovn-webhook/cert.go b/packages/system/kubeovn-webhook/images/kubeovn-webhook/cert.go index ebc3c3e65..1fcbf573b 100644 --- a/packages/system/kubeovn-webhook/images/kubeovn-webhook/cert.go +++ b/packages/system/kubeovn-webhook/images/kubeovn-webhook/cert.go @@ -1,155 +1,45 @@ package main -import ( - "crypto/tls" - "fmt" - "log" - "os" - "sync" - "time" -) +import "crypto/tls" -// certReloader keeps the webhook serving certificate in sync with the key pair -// files written by cert-manager. +// newReloadingTLSConfig returns a tls.Config that reloads the certificate and +// key from disk on every TLS handshake, so a cert-manager renewal of the mounted +// Secret is picked up by the running process without a restart. // -// The certificate is served through tls.Config.GetCertificate, so a cert-manager -// renewal of the backing Secret is picked up by the running process without a -// restart. Without this, the key pair is read exactly once at startup and cached -// for the lifetime of the process: after the certificate expires (roughly one year -// after install) the pod keeps presenting the expired certificate, the -// kube-apiserver's TLS call to the webhook fails, and because the -// MutatingWebhookConfiguration uses failurePolicy: Fail, every pod creation in -// tenant namespaces is rejected. -type certReloader struct { - certFile string - keyFile string - - // retryInterval bounds how often a failed reload (or a repeated stat failure) is - // retried, so a persistently broken file does not read/parse/log on every handshake. - retryInterval time.Duration - - mu sync.RWMutex - cert *tls.Certificate - loadedModTime time.Time - reloading bool - nextRetry time.Time - nextLog time.Time -} - -const defaultReloadRetryInterval = 30 * time.Second - -// newCertReloader loads the initial key pair and returns a reloader for it. -func newCertReloader(certFile, keyFile string) (*certReloader, error) { - cr := &certReloader{ - certFile: certFile, - keyFile: keyFile, - retryInterval: defaultReloadRetryInterval, - } - modTime, err := fileModTime(certFile) - if err != nil { - return nil, err - } - if err := cr.load(modTime); err != nil { - return nil, err - } - return cr, nil -} - -// fileModTime returns the modification time of the given file. -func fileModTime(name string) (time.Time, error) { - fi, err := os.Stat(name) - if err != nil { - return time.Time{}, err - } - return fi.ModTime(), nil -} - -// load reads the key pair and, on success, caches it tagged with modTime. On failure -// the cached certificate and its recorded mtime are left unchanged, so a transient -// failure on a valid file is retried rather than the renewal being abandoned. -func (cr *certReloader) load(modTime time.Time) error { - cert, err := tls.LoadX509KeyPair(cr.certFile, cr.keyFile) - if err != nil { - return err - } - cr.mu.Lock() - cr.cert = &cert - cr.loadedModTime = modTime - cr.mu.Unlock() - return nil -} - -// refresh reloads the key pair when the certificate file's modification time changes. +// Without this, the key pair is read exactly once at startup and cached for the +// lifetime of the process: after the certificate expires (roughly one year after +// install) the pod keeps presenting the expired certificate, the kube-apiserver's +// TLS call to the webhook fails, and because the MutatingWebhookConfiguration uses +// failurePolicy: Fail, every pod creation in tenant namespaces is rejected. // -// A failed reload keeps the last good certificate and does NOT advance the recorded -// mtime, so a transient error (fd exhaustion, a torn read) is retried, at most once per -// retryInterval, until it succeeds: a transient failure must never permanently pin an -// expiring certificate. A single reload is kept in flight so a burst of handshakes does -// not trigger a thundering herd of reads. -func (cr *certReloader) refresh() { - modTime, err := fileModTime(cr.certFile) - if err != nil { - cr.logRateLimited("certificate stat failed, keeping previous certificate: %v", err) - return - } - - cr.mu.Lock() - if modTime.Equal(cr.loadedModTime) || cr.reloading || time.Now().Before(cr.nextRetry) { - cr.mu.Unlock() - return - } - cr.reloading = true - cr.mu.Unlock() - - loadErr := cr.load(modTime) - - cr.mu.Lock() - cr.reloading = false - if loadErr == nil { - cr.nextRetry = time.Time{} - } else { - cr.nextRetry = time.Now().Add(cr.retryInterval) - } - cr.mu.Unlock() - - if loadErr != nil { - cr.logRateLimited("certificate reload failed, keeping previous certificate: %v", loadErr) - } -} - -// logRateLimited emits a log line at most once per retryInterval, so a persistently -// broken mount does not spam the log on every TLS handshake. -func (cr *certReloader) logRateLimited(format string, args ...any) { - cr.mu.Lock() - if time.Now().Before(cr.nextLog) { - cr.mu.Unlock() - return - } - cr.nextLog = time.Now().Add(cr.retryInterval) - cr.mu.Unlock() - log.Printf(format, args...) -} - -// GetCertificate is a tls.Config.GetCertificate callback. It refreshes the key pair -// when the certificate file changes and keeps serving the last good certificate if a -// reload fails. -func (cr *certReloader) GetCertificate(*tls.ClientHelloInfo) (*tls.Certificate, error) { - cr.refresh() - - cr.mu.RLock() - defer cr.mu.RUnlock() - if cr.cert == nil { - return nil, fmt.Errorf("no certificate loaded") - } - return cr.cert, nil -} - -// newReloadingTLSConfig builds a tls.Config that serves the key pair via a -// certReloader, so cert-manager renewals are honoured without a restart. +// tls.Config.GetCertificate is invoked on every handshake whenever Certificates is +// left unset, so re-reading the files there is enough; no watcher, mtime tracking +// or cache is needed: +// - The cert/key files are mounted from a plain Secret volume with no subPath. +// Kubernetes' atomic writer swaps the whole ..data directory via a single +// symlink flip, so a reader always sees a complete old or complete new +// generation of both files, never a torn mid-write mixture. A per-handshake +// LoadX509KeyPair therefore cannot observe a partial renewal. +// - The per-handshake cost (a few KB of file I/O plus a PEM/key parse) is +// negligible next to the asymmetric crypto the handshake already performs. +// - The webhook configures no mTLS (no ClientCAs), so GetCertificate's +// limitation of not refreshing client CA pools does not apply. +// +// If the mounted files genuinely become unreadable (Secret deleted, permissions +// broken: an operator error, not a normal renewal) the handshake fails loudly +// rather than silently serving a stale certificate. func newReloadingTLSConfig(certFile, keyFile string) (*tls.Config, error) { - cr, err := newCertReloader(certFile, keyFile) - if err != nil { + // Fail fast at startup if the initial key pair is missing or malformed. + if _, err := tls.LoadX509KeyPair(certFile, keyFile); err != nil { return nil, err } - return &tls.Config{GetCertificate: cr.GetCertificate}, nil + return &tls.Config{ + GetCertificate: func(*tls.ClientHelloInfo) (*tls.Certificate, error) { + cert, err := tls.LoadX509KeyPair(certFile, keyFile) + if err != nil { + return nil, err + } + return &cert, nil + }, + }, nil } diff --git a/packages/system/kubeovn-webhook/images/kubeovn-webhook/cert_test.go b/packages/system/kubeovn-webhook/images/kubeovn-webhook/cert_test.go index c3b1d4baf..2d39fe599 100644 --- a/packages/system/kubeovn-webhook/images/kubeovn-webhook/cert_test.go +++ b/packages/system/kubeovn-webhook/images/kubeovn-webhook/cert_test.go @@ -71,27 +71,6 @@ func setModTime(t *testing.T, name string, mtime time.Time) { } } -// certSerial returns the serial number of a *tls.Certificate leaf without mutating the -// shared value returned by GetCertificate (it parses into a local when Leaf is unset). -func certSerial(t *testing.T, c *tls.Certificate) int64 { - t.Helper() - if c == nil { - t.Fatal("nil certificate") - } - leaf := c.Leaf - if leaf == nil { - if len(c.Certificate) == 0 { - t.Fatal("certificate has no DER data") - } - parsed, err := x509.ParseCertificate(c.Certificate[0]) - if err != nil { - t.Fatalf("parse served cert: %v", err) - } - leaf = parsed - } - return leaf.SerialNumber.Int64() -} - // servedSerial completes a TLS handshake against addr and returns the serial number // of the leaf certificate the server actually presented. func servedSerial(t *testing.T, addr string) int64 { @@ -158,88 +137,3 @@ func TestReloadingTLSConfigServesRenewedCertificate(t *testing.T) { t.Fatalf("after renewal: expected renewed serial 2, got %d (certificate was not reloaded)", got) } } - -// TestCertReloaderKeepsLastGoodCertificate verifies that a failed reload (e.g. a torn -// read while the volume is updated) does not take the webhook down: it keeps serving -// the previously loaded certificate instead of erroring the handshake. -func TestCertReloaderKeepsLastGoodCertificate(t *testing.T) { - dir := t.TempDir() - certFile := filepath.Join(dir, "tls.crt") - keyFile := filepath.Join(dir, "tls.key") - - certA, keyA := genSelfSigned(t, 1) - writeKeyPair(t, certFile, keyFile, certA, keyA, time.Now().Add(-2*time.Second)) - - cr, err := newCertReloader(certFile, keyFile) - if err != nil { - t.Fatalf("newCertReloader: %v", err) - } - - // Corrupt the cert file and advance its mtime so a reload is attempted and fails. - if err := os.WriteFile(certFile, []byte("not a certificate"), 0o600); err != nil { - t.Fatalf("corrupt cert: %v", err) - } - setModTime(t, certFile, time.Now().Add(2*time.Second)) - - got, err := cr.GetCertificate(&tls.ClientHelloInfo{}) - if err != nil { - t.Fatalf("GetCertificate returned error instead of serving last good cert: %v", err) - } - if certSerial(t, got) != 1 { - t.Fatalf("expected to keep serving serial 1 after failed reload, got %d", certSerial(t, got)) - } -} - -// TestCertReloaderRetriesAfterTransientFailure is the regression test for the reloader -// abandoning a renewal on a transient error. A failed reload must NOT advance the -// recorded mtime, so once the (unchanged-mtime) file becomes readable the renewed -// certificate is still picked up. Against the code that advanced the mtime on failure, -// the renewal is permanently missed and this test fails. -func TestCertReloaderRetriesAfterTransientFailure(t *testing.T) { - dir := t.TempDir() - certFile := filepath.Join(dir, "tls.crt") - keyFile := filepath.Join(dir, "tls.key") - - certA, keyA := genSelfSigned(t, 1) - writeKeyPair(t, certFile, keyFile, certA, keyA, time.Now().Add(-2*time.Second)) - - cr, err := newCertReloader(certFile, keyFile) - if err != nil { - t.Fatalf("newCertReloader: %v", err) - } - cr.retryInterval = 0 // retry immediately, no backoff wait in the test - - // Renewal advances the mtime, but the first load fails (simulating a transient, - // content-independent error) by writing an unreadable cert at the new mtime. - renewMod := time.Now().Add(2 * time.Second) - if err := os.WriteFile(certFile, []byte("transiently unreadable"), 0o600); err != nil { - t.Fatalf("write bad cert: %v", err) - } - setModTime(t, certFile, renewMod) - - if got, gerr := cr.GetCertificate(&tls.ClientHelloInfo{}); gerr != nil { - t.Fatalf("GetCertificate errored during transient failure: %v", gerr) - } else if certSerial(t, got) != 1 { - t.Fatalf("during transient failure: expected to keep serial 1, got %d", certSerial(t, got)) - } - - // The renewed, valid certificate is now readable at the SAME mtime as the failed - // attempt. The reloader must still pick it up on retry. - certB, keyB := genSelfSigned(t, 2) - if err := os.WriteFile(certFile, certB, 0o600); err != nil { - t.Fatalf("write renewed cert: %v", err) - } - if err := os.WriteFile(keyFile, keyB, 0o600); err != nil { - t.Fatalf("write renewed key: %v", err) - } - setModTime(t, certFile, renewMod) - setModTime(t, keyFile, renewMod) - - got, gerr := cr.GetCertificate(&tls.ClientHelloInfo{}) - if gerr != nil { - t.Fatalf("GetCertificate errored after retry: %v", gerr) - } - if certSerial(t, got) != 2 { - t.Fatalf("after transient failure: expected renewed serial 2 on retry, got %d (mtime advanced on failure, renewal permanently missed)", certSerial(t, got)) - } -}