Skip to content

Commit f655000

Browse files
vinceprim-messiah
andcommitted
[release-0.19] 🐛 Refactor certificate watcher to use polling, instead of fsnotify (#3023)
* Reestablish watch for the certificate paths * Remove fsnotify and use cached read watcher * Simplify return * 🐛Fix certwatcher test to be backwards compatible --------- Co-authored-by: Maxim Muzafarov <[email protected]> Signed-off-by: Vince Prignano <[email protected]>
1 parent 267b59e commit f655000

File tree

9 files changed

+110
-129
lines changed

9 files changed

+110
-129
lines changed

examples/scratch-env/go.mod

+1-2
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,7 @@ require (
1414
github.com/davecgh/go-spew v1.1.1 // 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
18-
github.com/go-logr/logr v1.4.1 // indirect
17+
github.com/go-logr/logr v1.4.2 // indirect
1918
github.com/go-logr/zapr v1.3.0 // indirect
2019
github.com/go-openapi/jsonpointer v0.19.6 // indirect
2120
github.com/go-openapi/jsonreference v0.20.2 // indirect

examples/scratch-env/go.sum

+2-4
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,8 @@ github.com/evanphx/json-patch v4.12.0+incompatible h1:4onqiflcdA9EOZ4RxV643DvftH
1212
github.com/evanphx/json-patch v4.12.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk=
1313
github.com/evanphx/json-patch/v5 v5.9.0 h1:kcBlZQbplgElYIlo/n1hJbls2z/1awpXxpRi0/FOJfg=
1414
github.com/evanphx/json-patch/v5 v5.9.0/go.mod h1:VNkHZ/282BpEyt/tObQO8s5CMPmYYq14uClGH4abBuQ=
15-
github.com/fsnotify/fsnotify v1.7.0 h1:8JEhPFa5W2WU7YfeZzPNqzMP6Lwt7L2715Ggo0nosvA=
16-
github.com/fsnotify/fsnotify v1.7.0/go.mod h1:40Bi/Hjc2AVfZrqy+aj+yEI+/bRxZnMJyTJwOpGvigM=
17-
github.com/go-logr/logr v1.4.1 h1:pKouT5E8xu9zeFC39JXRDukb6JFQPXM5p5I91188VAQ=
18-
github.com/go-logr/logr v1.4.1/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY=
15+
github.com/go-logr/logr v1.4.2 h1:6pFjapn8bFcIbiKo3XT4j/BhANplGihG6tvd+8rYgrY=
16+
github.com/go-logr/logr v1.4.2/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY=
1917
github.com/go-logr/zapr v1.3.0 h1:XGdV8XW8zdwFiwOA2Dryh1gj2KRQyOOoNmBy4EplIcQ=
2018
github.com/go-logr/zapr v1.3.0/go.mod h1:YKepepNBd1u/oyhd/yQmtjVXmm9uML4IXUgMOwR8/Gg=
2119
github.com/go-openapi/jsonpointer v0.19.6 h1:eCs3fxoIi3Wh6vtgmLTOjdhSpiqphQ+DaPn38N2ZdrE=

go.mod

+3-3
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,8 @@ go 1.22.0
55
require (
66
github.com/evanphx/json-patch v4.12.0+incompatible // Using v4 to match upstream
77
github.com/evanphx/json-patch/v5 v5.9.0
8-
github.com/fsnotify/fsnotify v1.7.0
9-
github.com/go-logr/logr v1.4.1
8+
github.com/fsnotify/fsnotify v1.7.0 // indirect
9+
github.com/go-logr/logr v1.4.2
1010
github.com/go-logr/zapr v1.3.0
1111
github.com/google/go-cmp v0.6.0
1212
github.com/google/gofuzz v1.2.0
@@ -38,7 +38,7 @@ require (
3838
github.com/cespare/xxhash/v2 v2.2.0 // indirect
3939
github.com/davecgh/go-spew v1.1.1 // indirect
4040
github.com/emicklei/go-restful/v3 v3.11.0 // indirect
41-
github.com/felixge/httpsnoop v1.0.3 // indirect
41+
github.com/felixge/httpsnoop v1.0.4 // indirect
4242
github.com/go-logr/stdr v1.2.2 // indirect
4343
github.com/go-openapi/jsonpointer v0.19.6 // indirect
4444
github.com/go-openapi/jsonreference v0.20.2 // indirect

go.sum

+4-4
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,13 @@ github.com/evanphx/json-patch v4.12.0+incompatible h1:4onqiflcdA9EOZ4RxV643DvftH
2121
github.com/evanphx/json-patch v4.12.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk=
2222
github.com/evanphx/json-patch/v5 v5.9.0 h1:kcBlZQbplgElYIlo/n1hJbls2z/1awpXxpRi0/FOJfg=
2323
github.com/evanphx/json-patch/v5 v5.9.0/go.mod h1:VNkHZ/282BpEyt/tObQO8s5CMPmYYq14uClGH4abBuQ=
24-
github.com/felixge/httpsnoop v1.0.3 h1:s/nj+GCswXYzN5v2DpNMuMQYe+0DDwt5WVCU6CWBdXk=
25-
github.com/felixge/httpsnoop v1.0.3/go.mod h1:m8KPJKqk1gH5J9DgRY2ASl2lWCfGKXixSwevea8zH2U=
24+
github.com/felixge/httpsnoop v1.0.4 h1:NFTV2Zj1bL4mc9sqWACXbQFVBBg2W3GPvqp8/ESS2Wg=
25+
github.com/felixge/httpsnoop v1.0.4/go.mod h1:m8KPJKqk1gH5J9DgRY2ASl2lWCfGKXixSwevea8zH2U=
2626
github.com/fsnotify/fsnotify v1.7.0 h1:8JEhPFa5W2WU7YfeZzPNqzMP6Lwt7L2715Ggo0nosvA=
2727
github.com/fsnotify/fsnotify v1.7.0/go.mod h1:40Bi/Hjc2AVfZrqy+aj+yEI+/bRxZnMJyTJwOpGvigM=
2828
github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A=
29-
github.com/go-logr/logr v1.4.1 h1:pKouT5E8xu9zeFC39JXRDukb6JFQPXM5p5I91188VAQ=
30-
github.com/go-logr/logr v1.4.1/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY=
29+
github.com/go-logr/logr v1.4.2 h1:6pFjapn8bFcIbiKo3XT4j/BhANplGihG6tvd+8rYgrY=
30+
github.com/go-logr/logr v1.4.2/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY=
3131
github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag=
3232
github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre4VKE=
3333
github.com/go-logr/zapr v1.3.0 h1:XGdV8XW8zdwFiwOA2Dryh1gj2KRQyOOoNmBy4EplIcQ=

pkg/certwatcher/certwatcher.go

+57-104
Original file line numberDiff line numberDiff line change
@@ -17,58 +17,55 @@ 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"
3027
"sigs.k8s.io/controller-runtime/pkg/certwatcher/metrics"
3128
logf "sigs.k8s.io/controller-runtime/pkg/internal/log"
3229
)
3330

3431
var log = logf.RuntimeLog.WithName("certwatcher")
3532

36-
// CertWatcher watches certificate and key files for changes. When either file
37-
// changes, it reads and parses both and calls an optional callback with the new
38-
// 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.
3939
type CertWatcher struct {
4040
sync.RWMutex
4141

4242
currentCert *tls.Certificate
43-
watcher *fsnotify.Watcher
43+
interval time.Duration
4444

4545
certPath string
4646
keyPath string
4747

48+
cachedKeyPEMBlock []byte
49+
4850
// callback is a function to be invoked when the certificate changes.
4951
callback func(tls.Certificate)
5052
}
5153

5254
// New returns a new CertWatcher watching the given certificate and key.
5355
func New(certPath, keyPath string) (*CertWatcher, error) {
54-
var err error
55-
5656
cw := &CertWatcher{
5757
certPath: certPath,
5858
keyPath: keyPath,
59+
interval: defaultWatchInterval,
5960
}
6061

61-
// Initial read of certificate and key.
62-
if err := cw.ReadCertificate(); err != nil {
63-
return nil, err
64-
}
65-
66-
cw.watcher, err = fsnotify.NewWatcher()
67-
if err != nil {
68-
return nil, err
69-
}
62+
return cw, cw.ReadCertificate()
63+
}
7064

71-
return cw, nil
65+
// WithWatchInterval sets the watch interval and returns the CertWatcher pointer
66+
func (cw *CertWatcher) WithWatchInterval(interval time.Duration) *CertWatcher {
67+
cw.interval = interval
68+
return cw
7269
}
7370

7471
// RegisterCallback registers a callback to be invoked when the certificate changes.
@@ -91,72 +88,64 @@ func (cw *CertWatcher) GetCertificate(_ *tls.ClientHelloInfo) (*tls.Certificate,
9188

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

11594
log.Info("Starting certificate watcher")
116-
117-
// Block until the context is done.
118-
<-ctx.Done()
119-
120-
return cw.watcher.Close()
121-
}
122-
123-
// Watch reads events from the watcher's channel and reacts to changes.
124-
func (cw *CertWatcher) Watch() {
12595
for {
12696
select {
127-
case event, ok := <-cw.watcher.Events:
128-
// Channel is closed.
129-
if !ok {
130-
return
97+
case <-ctx.Done():
98+
return nil
99+
case <-ticker.C:
100+
if err := cw.ReadCertificate(); err != nil {
101+
log.Error(err, "failed read certificate")
131102
}
103+
}
104+
}
105+
}
132106

133-
cw.handleEvent(event)
134-
135-
case err, ok := <-cw.watcher.Errors:
136-
// Channel is closed.
137-
if !ok {
138-
return
139-
}
107+
// updateCachedCertificate checks if the new certificate differs from the cache,
108+
// updates it and returns the result if it was updated or not
109+
func (cw *CertWatcher) updateCachedCertificate(cert *tls.Certificate, keyPEMBlock []byte) bool {
110+
cw.Lock()
111+
defer cw.Unlock()
140112

141-
log.Error(err, "certificate watch error")
142-
}
113+
if cw.currentCert != nil &&
114+
bytes.Equal(cw.currentCert.Certificate[0], cert.Certificate[0]) &&
115+
bytes.Equal(cw.cachedKeyPEMBlock, keyPEMBlock) {
116+
log.V(7).Info("certificate already cached")
117+
return false
143118
}
119+
cw.currentCert = cert
120+
cw.cachedKeyPEMBlock = keyPEMBlock
121+
return true
144122
}
145123

146124
// ReadCertificate reads the certificate and key files from disk, parses them,
147-
// and updates the current certificate on the watcher. If a callback is set, it
125+
// and updates the current certificate on the watcher if updated. If a callback is set, it
148126
// is invoked with the new certificate.
149127
func (cw *CertWatcher) ReadCertificate() error {
150128
metrics.ReadCertificateTotal.Inc()
151-
cert, err := tls.LoadX509KeyPair(cw.certPath, cw.keyPath)
129+
certPEMBlock, err := os.ReadFile(cw.certPath)
130+
if err != nil {
131+
metrics.ReadCertificateErrors.Inc()
132+
return err
133+
}
134+
keyPEMBlock, err := os.ReadFile(cw.keyPath)
152135
if err != nil {
153136
metrics.ReadCertificateErrors.Inc()
154137
return err
155138
}
156139

157-
cw.Lock()
158-
cw.currentCert = &cert
159-
cw.Unlock()
140+
cert, err := tls.X509KeyPair(certPEMBlock, keyPEMBlock)
141+
if err != nil {
142+
metrics.ReadCertificateErrors.Inc()
143+
return err
144+
}
145+
146+
if !cw.updateCachedCertificate(&cert, keyPEMBlock) {
147+
return nil
148+
}
160149

161150
log.Info("Updated current TLS certificate")
162151

@@ -170,39 +159,3 @@ func (cw *CertWatcher) ReadCertificate() error {
170159
}
171160
return nil
172161
}
173-
174-
func (cw *CertWatcher) handleEvent(event fsnotify.Event) {
175-
// Only care about events which may modify the contents of the file.
176-
if !(isWrite(event) || isRemove(event) || isCreate(event) || isChmod(event)) {
177-
return
178-
}
179-
180-
log.V(1).Info("certificate event", "event", event)
181-
182-
// If the file was removed or renamed, re-add the watch to the previous name
183-
if isRemove(event) || isChmod(event) {
184-
if err := cw.watcher.Add(event.Name); err != nil {
185-
log.Error(err, "error re-watching file")
186-
}
187-
}
188-
189-
if err := cw.ReadCertificate(); err != nil {
190-
log.Error(err, "error re-reading certificate")
191-
}
192-
}
193-
194-
func isWrite(event fsnotify.Event) bool {
195-
return event.Op.Has(fsnotify.Write)
196-
}
197-
198-
func isCreate(event fsnotify.Event) bool {
199-
return event.Op.Has(fsnotify.Create)
200-
}
201-
202-
func isRemove(event fsnotify.Event) bool {
203-
return event.Op.Has(fsnotify.Remove)
204-
}
205-
206-
func isChmod(event fsnotify.Event) bool {
207-
return event.Op.Has(fsnotify.Chmod)
208-
}

pkg/certwatcher/certwatcher_suite_test.go

+1
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
)

0 commit comments

Comments
 (0)