Skip to content

Commit 1402969

Browse files
m-messiahk8s-infra-cherrypick-robot
authored andcommitted
Remove fsnotify and use cached read watcher
1 parent e83745a commit 1402969

File tree

8 files changed

+70
-139
lines changed

8 files changed

+70
-139
lines changed

examples/scratch-env/go.mod

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ require (
1414
github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect
1515
github.com/emicklei/go-restful/v3 v3.11.0 // indirect
1616
github.com/evanphx/json-patch/v5 v5.9.0 // indirect
17-
github.com/fsnotify/fsnotify v1.7.0 // indirect
1817
github.com/fxamacker/cbor/v2 v2.7.0 // indirect
1918
github.com/go-logr/logr v1.4.2 // indirect
2019
github.com/go-logr/zapr v1.3.0 // indirect

examples/scratch-env/go.sum

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,6 @@ github.com/evanphx/json-patch v0.5.2 h1:xVCHIVMUu1wtM/VkR9jVZ45N3FhZfYMMYGorLCR8
1313
github.com/evanphx/json-patch v0.5.2/go.mod h1:ZWS5hhDbVDyob71nXKNL0+PWn6ToqBHMikGIFbs31qQ=
1414
github.com/evanphx/json-patch/v5 v5.9.0 h1:kcBlZQbplgElYIlo/n1hJbls2z/1awpXxpRi0/FOJfg=
1515
github.com/evanphx/json-patch/v5 v5.9.0/go.mod h1:VNkHZ/282BpEyt/tObQO8s5CMPmYYq14uClGH4abBuQ=
16-
github.com/fsnotify/fsnotify v1.7.0 h1:8JEhPFa5W2WU7YfeZzPNqzMP6Lwt7L2715Ggo0nosvA=
17-
github.com/fsnotify/fsnotify v1.7.0/go.mod h1:40Bi/Hjc2AVfZrqy+aj+yEI+/bRxZnMJyTJwOpGvigM=
1816
github.com/fxamacker/cbor/v2 v2.7.0 h1:iM5WgngdRBanHcxugY4JySA0nk1wZorNOpTgCMedv5E=
1917
github.com/fxamacker/cbor/v2 v2.7.0/go.mod h1:pxXPTn3joSm21Gbwsv0w9OSA2y1HFR9qXEeXQVeNoDQ=
2018
github.com/go-logr/logr v1.4.2 h1:6pFjapn8bFcIbiKo3XT4j/BhANplGihG6tvd+8rYgrY=

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ go 1.22.0
44

55
require (
66
github.com/evanphx/json-patch/v5 v5.9.0
7-
github.com/fsnotify/fsnotify v1.7.0
87
github.com/go-logr/logr v1.4.2
98
github.com/go-logr/zapr v1.3.0
109
github.com/google/go-cmp v0.6.0
@@ -40,6 +39,7 @@ require (
4039
github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect
4140
github.com/emicklei/go-restful/v3 v3.11.0 // indirect
4241
github.com/felixge/httpsnoop v1.0.4 // indirect
42+
github.com/fsnotify/fsnotify v1.7.0 // indirect
4343
github.com/fxamacker/cbor/v2 v2.7.0 // indirect
4444
github.com/go-logr/stdr v1.2.2 // indirect
4545
github.com/go-openapi/jsonpointer v0.19.6 // indirect

pkg/certwatcher/certwatcher.go

Lines changed: 57 additions & 125 deletions
Original file line numberDiff line numberDiff line change
@@ -17,61 +17,62 @@ limitations under the License.
1717
package certwatcher
1818

1919
import (
20+
"bytes"
2021
"context"
2122
"crypto/tls"
22-
"fmt"
23+
"os"
2324
"sync"
2425
"time"
2526

26-
"github.com/fsnotify/fsnotify"
27-
kerrors "k8s.io/apimachinery/pkg/util/errors"
28-
"k8s.io/apimachinery/pkg/util/sets"
29-
"k8s.io/apimachinery/pkg/util/wait"
30-
3127
"sigs.k8s.io/controller-runtime/pkg/certwatcher/metrics"
3228
logf "sigs.k8s.io/controller-runtime/pkg/internal/log"
3329
)
3430

3531
var log = logf.RuntimeLog.WithName("certwatcher")
3632

37-
// CertWatcher watches certificate and key files for changes. When either file
38-
// changes, it reads and parses both and calls an optional callback with the new
39-
// certificate.
33+
const defaultWatchInterval = 10 * time.Second
34+
35+
// CertWatcher watches certificate and key files for changes.
36+
// It always returns the cached version,
37+
// but periodically reads and parses certificate and key for changes
38+
// and calls an optional callback with the new certificate.
4039
type CertWatcher struct {
4140
sync.RWMutex
4241

4342
currentCert *tls.Certificate
44-
watcher *fsnotify.Watcher
43+
interval time.Duration
4544

4645
certPath string
4746
keyPath string
4847

48+
cachedKeyPEMBlock []byte
49+
4950
// callback is a function to be invoked when the certificate changes.
5051
callback func(tls.Certificate)
5152
}
5253

5354
// New returns a new CertWatcher watching the given certificate and key.
5455
func New(certPath, keyPath string) (*CertWatcher, error) {
55-
var err error
56-
5756
cw := &CertWatcher{
5857
certPath: certPath,
5958
keyPath: keyPath,
59+
interval: defaultWatchInterval,
6060
}
6161

6262
// Initial read of certificate and key.
6363
if err := cw.ReadCertificate(); err != nil {
6464
return nil, err
6565
}
6666

67-
cw.watcher, err = fsnotify.NewWatcher()
68-
if err != nil {
69-
return nil, err
70-
}
71-
7267
return cw, nil
7368
}
7469

70+
// WithWatchInterval sets the watch interval and returns the CertWatcher pointer
71+
func (cw *CertWatcher) WithWatchInterval(interval time.Duration) *CertWatcher {
72+
cw.interval = interval
73+
return cw
74+
}
75+
7576
// RegisterCallback registers a callback to be invoked when the certificate changes.
7677
func (cw *CertWatcher) RegisterCallback(callback func(tls.Certificate)) {
7778
cw.Lock()
@@ -92,97 +93,64 @@ func (cw *CertWatcher) GetCertificate(_ *tls.ClientHelloInfo) (*tls.Certificate,
9293

9394
// Start starts the watch on the certificate and key files.
9495
func (cw *CertWatcher) Start(ctx context.Context) error {
95-
files := sets.New(cw.certPath, cw.keyPath)
96-
97-
{
98-
var watchErr error
99-
if err := wait.PollUntilContextTimeout(ctx, 1*time.Second, 10*time.Second, true, func(ctx context.Context) (done bool, err error) {
100-
for _, f := range files.UnsortedList() {
101-
if err := cw.watcher.Add(f); err != nil {
102-
watchErr = err
103-
return false, nil //nolint:nilerr // We want to keep trying.
104-
}
105-
// We've added the watch, remove it from the set.
106-
files.Delete(f)
107-
}
108-
return true, nil
109-
}); err != nil {
110-
return fmt.Errorf("failed to add watches: %w", kerrors.NewAggregate([]error{err, watchErr}))
111-
}
112-
}
113-
114-
go cw.Watch()
96+
ticker := time.NewTicker(cw.interval)
97+
defer ticker.Stop()
11598

11699
log.Info("Starting certificate watcher")
117-
118-
// Block until the context is done.
119-
<-ctx.Done()
120-
121-
return cw.watcher.Close()
122-
}
123-
124-
func (cw *CertWatcher) ensureAllFilesAreWatched() {
125-
watchList := sets.New(cw.watcher.WatchList()...)
126-
difference := sets.New(cw.certPath, cw.keyPath).Difference(watchList)
127-
if difference.Len() == 0 {
128-
return
129-
}
130-
131-
for _, missingWatchPath := range difference.UnsortedList() {
132-
log.V(1).Info("re-adding missing watch", "path", missingWatchPath)
133-
if err := cw.watcher.Add(missingWatchPath); err != nil {
134-
log.Error(err, "failed to add watch", "path", missingWatchPath)
135-
return
136-
}
137-
}
138-
139-
log.V(1).Info("all files are watched again", "list", cw.watcher.WatchList())
140-
141-
if err := cw.ReadCertificate(); err != nil {
142-
log.Error(err, "error re-reading certificate")
143-
}
144-
}
145-
146-
// Watch reads events from the watcher's channel and reacts to changes.
147-
func (cw *CertWatcher) Watch() {
148-
watcherHealthTimer := time.NewTicker(time.Second)
149100
for {
150101
select {
151-
case event, ok := <-cw.watcher.Events:
152-
// Channel is closed.
153-
if !ok {
154-
return
102+
case <-ctx.Done():
103+
return nil
104+
case <-ticker.C:
105+
if err := cw.ReadCertificate(); err != nil {
106+
log.Error(err, "failed read certificate")
155107
}
108+
}
109+
}
110+
}
156111

157-
cw.handleEvent(event)
158-
159-
case err, ok := <-cw.watcher.Errors:
160-
// Channel is closed.
161-
if !ok {
162-
return
163-
}
112+
// updateCachedCertificate checks if the new certificate differs from the cache,
113+
// updates it and returns the result if it was updated or not
114+
func (cw *CertWatcher) updateCachedCertificate(cert *tls.Certificate, keyPEMBlock []byte) bool {
115+
cw.Lock()
116+
defer cw.Unlock()
164117

165-
log.Error(err, "certificate watch error")
166-
case <-watcherHealthTimer.C:
167-
cw.ensureAllFilesAreWatched()
168-
}
118+
if cw.currentCert != nil &&
119+
bytes.Equal(cw.currentCert.Certificate[0], cert.Certificate[0]) &&
120+
bytes.Equal(cw.cachedKeyPEMBlock, keyPEMBlock) {
121+
log.V(7).Info("certificate already cached")
122+
return false
169123
}
124+
cw.currentCert = cert
125+
cw.cachedKeyPEMBlock = keyPEMBlock
126+
return true
170127
}
171128

172129
// ReadCertificate reads the certificate and key files from disk, parses them,
173-
// and updates the current certificate on the watcher. If a callback is set, it
130+
// and updates the current certificate on the watcher if updated. If a callback is set, it
174131
// is invoked with the new certificate.
175132
func (cw *CertWatcher) ReadCertificate() error {
176133
metrics.ReadCertificateTotal.Inc()
177-
cert, err := tls.LoadX509KeyPair(cw.certPath, cw.keyPath)
134+
certPEMBlock, err := os.ReadFile(cw.certPath)
135+
if err != nil {
136+
metrics.ReadCertificateErrors.Inc()
137+
return err
138+
}
139+
keyPEMBlock, err := os.ReadFile(cw.keyPath)
178140
if err != nil {
179141
metrics.ReadCertificateErrors.Inc()
180142
return err
181143
}
182144

183-
cw.Lock()
184-
cw.currentCert = &cert
185-
cw.Unlock()
145+
cert, err := tls.X509KeyPair(certPEMBlock, keyPEMBlock)
146+
if err != nil {
147+
metrics.ReadCertificateErrors.Inc()
148+
return err
149+
}
150+
151+
if !cw.updateCachedCertificate(&cert, keyPEMBlock) {
152+
return nil
153+
}
186154

187155
log.Info("Updated current TLS certificate")
188156

@@ -196,39 +164,3 @@ func (cw *CertWatcher) ReadCertificate() error {
196164
}
197165
return nil
198166
}
199-
200-
func (cw *CertWatcher) handleEvent(event fsnotify.Event) {
201-
// Only care about events which may modify the contents of the file.
202-
if !(isWrite(event) || isRemove(event) || isCreate(event) || isChmod(event)) {
203-
return
204-
}
205-
206-
log.V(1).Info("certificate event", "event", event)
207-
208-
// If the file was removed or renamed, re-add the watch to the previous name
209-
if isRemove(event) || isChmod(event) {
210-
if err := cw.watcher.Add(event.Name); err != nil {
211-
log.Error(err, "error re-watching file")
212-
}
213-
}
214-
215-
if err := cw.ReadCertificate(); err != nil {
216-
log.Error(err, "error re-reading certificate")
217-
}
218-
}
219-
220-
func isWrite(event fsnotify.Event) bool {
221-
return event.Op.Has(fsnotify.Write)
222-
}
223-
224-
func isCreate(event fsnotify.Event) bool {
225-
return event.Op.Has(fsnotify.Create)
226-
}
227-
228-
func isRemove(event fsnotify.Event) bool {
229-
return event.Op.Has(fsnotify.Remove)
230-
}
231-
232-
func isChmod(event fsnotify.Event) bool {
233-
return event.Op.Has(fsnotify.Chmod)
234-
}

pkg/certwatcher/certwatcher_suite_test.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222

2323
. "github.com/onsi/ginkgo/v2"
2424
. "github.com/onsi/gomega"
25+
2526
logf "sigs.k8s.io/controller-runtime/pkg/log"
2627
"sigs.k8s.io/controller-runtime/pkg/log/zap"
2728
)

pkg/certwatcher/certwatcher_test.go

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ var _ = Describe("CertWatcher", func() {
8181
go func() {
8282
defer GinkgoRecover()
8383
defer close(doneCh)
84-
Expect(watcher.Start(ctx)).To(Succeed())
84+
Expect(watcher.WithWatchInterval(time.Second).Start(ctx)).To(Succeed())
8585
}()
8686
// wait till we read first cert
8787
Eventually(func() error {
@@ -193,8 +193,8 @@ var _ = Describe("CertWatcher", func() {
193193

194194
Eventually(func() error {
195195
readCertificateTotalAfter := testutil.ToFloat64(metrics.ReadCertificateTotal)
196-
if readCertificateTotalAfter != readCertificateTotalBefore+1.0 {
197-
return fmt.Errorf("metric read certificate total expected: %v and got: %v", readCertificateTotalBefore+1.0, readCertificateTotalAfter)
196+
if readCertificateTotalAfter < readCertificateTotalBefore+1.0 {
197+
return fmt.Errorf("metric read certificate total expected at least: %v and got: %v", readCertificateTotalBefore+1.0, readCertificateTotalAfter)
198198
}
199199
return nil
200200
}, "4s").Should(Succeed())
@@ -208,8 +208,8 @@ var _ = Describe("CertWatcher", func() {
208208

209209
Eventually(func() error {
210210
readCertificateTotalAfter := testutil.ToFloat64(metrics.ReadCertificateTotal)
211-
if readCertificateTotalAfter != readCertificateTotalBefore+1.0 {
212-
return fmt.Errorf("metric read certificate total expected: %v and got: %v", readCertificateTotalBefore+1.0, readCertificateTotalAfter)
211+
if readCertificateTotalAfter < readCertificateTotalBefore+1.0 {
212+
return fmt.Errorf("metric read certificate total expected at least: %v and got: %v", readCertificateTotalBefore+1.0, readCertificateTotalAfter)
213213
}
214214
readCertificateTotalBefore = readCertificateTotalAfter
215215
return nil
@@ -220,15 +220,15 @@ var _ = Describe("CertWatcher", func() {
220220
// Note, we are checking two errors here, because os.Remove generates two fsnotify events: Chmod + Remove
221221
Eventually(func() error {
222222
readCertificateTotalAfter := testutil.ToFloat64(metrics.ReadCertificateTotal)
223-
if readCertificateTotalAfter != readCertificateTotalBefore+2.0 {
224-
return fmt.Errorf("metric read certificate total expected: %v and got: %v", readCertificateTotalBefore+2.0, readCertificateTotalAfter)
223+
if readCertificateTotalAfter < readCertificateTotalBefore+2.0 {
224+
return fmt.Errorf("metric read certificate total expected at least: %v and got: %v", readCertificateTotalBefore+2.0, readCertificateTotalAfter)
225225
}
226226
return nil
227227
}, "4s").Should(Succeed())
228228
Eventually(func() error {
229229
readCertificateErrorsAfter := testutil.ToFloat64(metrics.ReadCertificateErrors)
230-
if readCertificateErrorsAfter != readCertificateErrorsBefore+2.0 {
231-
return fmt.Errorf("metric read certificate errors expected: %v and got: %v", readCertificateErrorsBefore+2.0, readCertificateErrorsAfter)
230+
if readCertificateErrorsAfter < readCertificateErrorsBefore+2.0 {
231+
return fmt.Errorf("metric read certificate errors expected at least: %v and got: %v", readCertificateErrorsBefore+2.0, readCertificateErrorsAfter)
232232
}
233233
return nil
234234
}, "4s").Should(Succeed())

pkg/certwatcher/example_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ func Example() {
3939
panic(err)
4040
}
4141

42-
// Start goroutine with certwatcher running fsnotify against supplied certdir
42+
// Start goroutine with certwatcher running against supplied cert
4343
go func() {
4444
if err := watcher.Start(ctx); err != nil {
4545
panic(err)

pkg/certwatcher/metrics/metrics.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package metrics
1818

1919
import (
2020
"github.com/prometheus/client_golang/prometheus"
21+
2122
"sigs.k8s.io/controller-runtime/pkg/metrics"
2223
)
2324

0 commit comments

Comments
 (0)