Skip to content

fix: certwatcher reloads in poll-only mode when fsnotify setup is slow/fails #59

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
matteogastaldello merged 2 commits into from
Jun 26, 2026
Merged

fix: certwatcher reloads in poll-only mode when fsnotify setup is slow/fails #59

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d1cff7d
test: make certwatcher tests deterministic instead of relying on fsno…
matteogastaldello Jun 18, 2026
267f71a
fix: run certificate poll independently of fsnotify watch setup
matteogastaldello Jun 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 34 additions & 21 deletions pkg/certwatcher/certwatcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,13 @@ import (
"bytes"
"context"
"crypto/tls"
"fmt"
"os"
"sync"
"time"

"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"
)
Expand Down Expand Up @@ -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()
Expand All @@ -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
Expand Down
25 changes: 14 additions & 11 deletions pkg/certwatcher/certwatcher_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -104,16 +105,17 @@ 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())
Expect(called.Load()).To(BeNumerically(">=", 1))
})

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)
Expand All @@ -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())
Expand Down Expand Up @@ -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)
Expand All @@ -210,7 +213,7 @@ var _ = Describe("CertWatcher", func() {
}
readCertificateTotalBefore = readCertificateTotalAfter
return nil
}, "4s").Should(Succeed())
}, "10s").Should(Succeed())

Expect(os.Remove(keyPath)).To(Succeed())

Expand All @@ -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())
Expand Down
Loading