diff --git a/pkg/certwatcher/certwatcher.go b/pkg/certwatcher/certwatcher.go index e54dddb..d15a824 100644 --- a/pkg/certwatcher/certwatcher.go +++ b/pkg/certwatcher/certwatcher.go @@ -4,7 +4,6 @@ import ( "bytes" "context" "crypto/tls" - "fmt" "os" "sync" "time" @@ -12,7 +11,6 @@ import ( "github.com/fsnotify/fsnotify" "github.com/krateoplatformops/unstructured-runtime/pkg/certwatcher/metrics" "github.com/krateoplatformops/unstructured-runtime/pkg/logging" - kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/wait" ) @@ -96,28 +94,15 @@ func (cw *CertWatcher) GetCertificate(_ *tls.ClientHelloInfo) (*tls.Certificate, } // Start starts the watch on the certificate and key files. +// +// The periodic poll below is the baseline mechanism and runs immediately; +// fsnotify is only an optimization for faster reaction. Adding the fsnotify +// watches is therefore done in the background and never blocks (or fails) the +// poll: a slow or failing watch setup must not disable certificate reloading. func (cw *CertWatcher) Start(ctx context.Context) error { log := cw.log - files := sets.New(cw.certPath, cw.keyPath) - - { - var watchErr error - if err := wait.PollUntilContextTimeout(ctx, 1*time.Second, 10*time.Second, true, func(ctx context.Context) (done bool, err error) { - for _, f := range files.UnsortedList() { - if err := cw.watcher.Add(f); err != nil { - watchErr = err - return false, nil //nolint:nilerr // We want to keep trying. - } - // We've added the watch, remove it from the set. - files.Delete(f) - } - return true, nil - }); err != nil { - return fmt.Errorf("failed to add watches: %w", kerrors.NewAggregate([]error{err, watchErr})) - } - } - go cw.Watch() + go cw.watchFiles(ctx) ticker := time.NewTicker(cw.interval) defer ticker.Stop() @@ -135,6 +120,34 @@ func (cw *CertWatcher) Start(ctx context.Context) error { } } +// watchFiles registers fsnotify watches for the certificate and key files, +// retrying until it succeeds or the context is cancelled, then runs the event +// loop. While the watches are not yet in place the watcher keeps working in +// poll-only mode (see Start). +func (cw *CertWatcher) watchFiles(ctx context.Context) { + log := cw.log + files := sets.New(cw.certPath, cw.keyPath) + + if err := wait.PollUntilContextCancel(ctx, 1*time.Second, true, func(ctx context.Context) (done bool, err error) { + for _, f := range files.UnsortedList() { + if err := cw.watcher.Add(f); err != nil { + log.Debug("waiting to add file watch", "file", f, "error", err.Error()) + return false, nil //nolint:nilerr // We want to keep trying. + } + // We've added the watch, remove it from the set. + files.Delete(f) + } + return true, nil + }); err != nil { + // Context cancelled before the watches were added; the poll loop in + // Start remains the active mechanism, so this is not fatal. + log.Debug("stopped adding file watches before completion", "error", err.Error()) + return + } + + cw.Watch() +} + // Watch reads events from the watcher's channel and reacts to changes. func (cw *CertWatcher) Watch() { log := cw.log diff --git a/pkg/certwatcher/certwatcher_test.go b/pkg/certwatcher/certwatcher_test.go index 739b73a..c6c5a3a 100644 --- a/pkg/certwatcher/certwatcher_test.go +++ b/pkg/certwatcher/certwatcher_test.go @@ -87,8 +87,9 @@ var _ = Describe("CertWatcher", func() { }) It("should reload currentCert when changed", func() { - // This test verifies fsnotify detects the cert change. So interval doesn't matter. - doneCh := startWatcher(10 * time.Second) + // fsnotify is the fast path; a short poll interval is a deterministic fallback, + // so this assertion does not depend on fsnotify delivery timing under load. + doneCh := startWatcher(1 * time.Second) called := atomic.Int64{} watcher.RegisterCallback(func(crt tls.Certificate) { called.Add(1) @@ -104,7 +105,7 @@ var _ = Describe("CertWatcher", func() { secondcert, _ := watcher.GetCertificate(nil) first := firstcert.PrivateKey.(*rsa.PrivateKey) return first.Equal(secondcert.PrivateKey) || firstcert.Leaf.SerialNumber == secondcert.Leaf.SerialNumber - }).ShouldNot(BeTrue()) + }, "10s", "1s").ShouldNot(BeTrue()) ctxCancel() Eventually(doneCh, "4s").Should(BeClosed()) @@ -112,8 +113,9 @@ var _ = Describe("CertWatcher", func() { }) It("should reload currentCert when changed with rename", func() { - // This test verifies fsnotify detects the cert change. So interval doesn't matter. - doneCh := startWatcher(10 * time.Second) + // fsnotify is the fast path; a short poll interval is a deterministic fallback, + // so this assertion does not depend on fsnotify delivery timing under load. + doneCh := startWatcher(1 * time.Second) called := atomic.Int64{} watcher.RegisterCallback(func(crt tls.Certificate) { called.Add(1) @@ -135,7 +137,7 @@ var _ = Describe("CertWatcher", func() { secondcert, _ := watcher.GetCertificate(nil) first := firstcert.PrivateKey.(*rsa.PrivateKey) return first.Equal(secondcert.PrivateKey) || firstcert.Leaf.SerialNumber == secondcert.Leaf.SerialNumber - }).ShouldNot(BeTrue()) + }, "10s", "1s").ShouldNot(BeTrue()) ctxCancel() Eventually(doneCh, "4s").Should(BeClosed()) @@ -200,8 +202,9 @@ var _ = Describe("CertWatcher", func() { }) It("should get updated on read certificate errors", func() { - // This test works with fsnotify, so interval doesn't matter. - doneCh := startWatcher(10 * time.Second) + // fsnotify is the fast path; the 1s poll is a deterministic fallback so the + // metric assertions below do not depend on fsnotify delivery timing under load. + doneCh := startWatcher(1 * time.Second) Eventually(func() error { readCertificateTotalAfter := testutil.ToFloat64(metrics.ReadCertificateTotal) @@ -210,7 +213,7 @@ var _ = Describe("CertWatcher", func() { } readCertificateTotalBefore = readCertificateTotalAfter return nil - }, "4s").Should(Succeed()) + }, "10s").Should(Succeed()) Expect(os.Remove(keyPath)).To(Succeed()) @@ -221,14 +224,14 @@ var _ = Describe("CertWatcher", func() { return fmt.Errorf("metric read certificate total expected at least: %v and got: %v", readCertificateTotalBefore+1.0, readCertificateTotalAfter) } return nil - }, "4s").Should(Succeed()) + }, "10s").Should(Succeed()) Eventually(func() error { readCertificateErrorsAfter := testutil.ToFloat64(metrics.ReadCertificateErrors) if readCertificateErrorsAfter < readCertificateErrorsBefore+1.0 { return fmt.Errorf("metric read certificate errors expected at least: %v and got: %v", readCertificateErrorsBefore+1.0, readCertificateErrorsAfter) } return nil - }, "4s").Should(Succeed()) + }, "10s").Should(Succeed()) ctxCancel() Eventually(doneCh, "4s").Should(BeClosed())