feat(certs): add cert watcher with fsnotify and SHA-256 polling#2561
Draft
fseldow wants to merge 1 commit into
Draft
feat(certs): add cert watcher with fsnotify and SHA-256 polling#2561fseldow wants to merge 1 commit into
fseldow wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a filesystem-backed trust store watcher so certificate providers can automatically reload cached certificates when files change.
Changes:
- Introduces
truststore.Watcherbased onfsnotifywith a polling fallback and debouncing. - Updates the filesystem keyprovider to keep an in-memory certificate cache that reloads on watch events.
- Adds/updates tests to cover watcher behaviors and certificate reload after file changes/removal.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| internal/verifier/truststore/watcher.go | New watcher implementation for detecting trust store changes (fsnotify + polling). |
| internal/verifier/truststore/watcher_test.go | New tests covering watcher construction, callback triggering, polling detection, and stop behavior. |
| internal/verifier/keyprovider/filesystemprovider/register.go | Adds watcher-backed certificate reloading with concurrency protection. |
| internal/verifier/keyprovider/filesystemprovider/register_test.go | Adjusts certificate tests and adds coverage for reload after file removal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+136
to
+154
| case event, ok := <-w.watcher.Events: | ||
| if !ok { | ||
| return | ||
| } | ||
| if event.Op&(fsnotify.Write|fsnotify.Create|fsnotify.Remove|fsnotify.Rename) == 0 { | ||
| continue | ||
| } | ||
| logrus.Debugf("trust store watcher event: %s %s", event.Op, event.Name) | ||
| if event.Op&(fsnotify.Remove|fsnotify.Rename) != 0 { | ||
| _ = w.watcher.Add(event.Name) | ||
| } | ||
| if debounceTimer != nil { | ||
| debounceTimer.Stop() | ||
| } | ||
| debounceTimer = time.AfterFunc(debounceInterval, func() { | ||
| logrus.Infof("trust store cert change detected: %s", event.Name) | ||
| w.callback() | ||
| w.snapshotHashes() | ||
| }) |
Comment on lines
+198
to
+217
| func (w *Watcher) checkPathChanged(path string) bool { | ||
| info, err := os.Stat(path) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| if info.IsDir() { | ||
| changed := false | ||
| _ = filepath.WalkDir(path, func(filePath string, d fs.DirEntry, err error) error { | ||
| if err != nil || d.IsDir() { | ||
| return nil | ||
| } | ||
| if w.fileHashChanged(filePath) { | ||
| changed = true | ||
| } | ||
| return nil | ||
| }) | ||
| return changed | ||
| } | ||
| return w.fileHashChanged(path) | ||
| } |
Comment on lines
+231
to
+260
| func (w *Watcher) snapshotHashes() { | ||
| w.mu.Lock() | ||
| defer w.mu.Unlock() | ||
|
|
||
| for _, path := range w.paths { | ||
| info, err := os.Stat(path) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| if info.IsDir() { | ||
| _ = filepath.WalkDir(path, func(filePath string, d fs.DirEntry, err error) error { | ||
| if err != nil || d.IsDir() { | ||
| return nil | ||
| } | ||
| data, err := os.ReadFile(filePath) | ||
| if err != nil { | ||
| return nil | ||
| } | ||
| w.hashes[filePath] = sha256.Sum256(data) | ||
| return nil | ||
| }) | ||
| continue | ||
| } | ||
| data, err := os.ReadFile(path) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| w.hashes[path] = sha256.Sum256(data) | ||
| } | ||
| } |
Comment on lines
+94
to
+113
| func (w *Watcher) AddPath(path string) error { | ||
| w.mu.Lock() | ||
| for _, existing := range w.paths { | ||
| if existing == path { | ||
| w.mu.Unlock() | ||
| return nil | ||
| } | ||
| } | ||
| w.mu.Unlock() | ||
|
|
||
| if err := w.addPath(path); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| w.mu.Lock() | ||
| w.paths = append(w.paths, path) | ||
| w.mu.Unlock() | ||
| w.snapshotHashes() | ||
| return nil | ||
| } |
Comment on lines
+65
to
+78
| watcher, err := verifiertruststore.NewWatcher(paths, func() { | ||
| if err := provider.reloadCertificates(); err != nil { | ||
| logrus.WithError(err).Error("failed to reload trust store certificates") | ||
| } | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create trust store watcher: %w", err) | ||
| } | ||
| if err := watcher.Start(); err != nil { | ||
| watcher.Stop() | ||
| return nil, fmt.Errorf("failed to start trust store watcher: %w", err) | ||
| } | ||
| provider.watcher = watcher | ||
| return provider, nil |
Comment on lines
+58
to
+66
| time.Sleep(100 * time.Millisecond) | ||
| if err := os.WriteFile(certFile, []byte("rotated-cert"), 0o644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| time.Sleep(3 * time.Second) | ||
| if calls.Load() == 0 { | ||
| t.Fatal("expected callback after cert write") | ||
| } |
Comment on lines
+92
to
94
| if len(certs) == 0 { | ||
| t.Fatalf("expected at least one certificate, got %d", len(certs)) | ||
| } |
Comment on lines
+105
to
107
| if len(certs) == 0 { | ||
| t.Fatalf("expected at least one certificate, got %d", len(certs)) | ||
| } |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2561 +/- ##
==========================================
+ Coverage 77.62% 77.77% +0.15%
==========================================
Files 105 106 +1
Lines 4657 4824 +167
==========================================
+ Hits 3615 3752 +137
- Misses 893 908 +15
- Partials 149 164 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: xinhl <xinhl@microsoft.com>
fseldow
force-pushed
the
xinhl/alpha2-cert-watcher
branch
from
26049cf to
107987d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing