From f0b921943b496d899f55fce0790af8242a198783 Mon Sep 17 00:00:00 2001 From: Maxim Muzafarov Date: Tue, 30 Jul 2024 14:57:50 +0100 Subject: [PATCH 1/2] Add certwatcher test for file rename --- pkg/certwatcher/certwatcher_suite_test.go | 2 +- pkg/certwatcher/certwatcher_test.go | 30 +++++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/pkg/certwatcher/certwatcher_suite_test.go b/pkg/certwatcher/certwatcher_suite_test.go index a44a968c89..2d0f677685 100644 --- a/pkg/certwatcher/certwatcher_suite_test.go +++ b/pkg/certwatcher/certwatcher_suite_test.go @@ -41,7 +41,7 @@ var _ = BeforeSuite(func() { }) var _ = AfterSuite(func() { - for _, file := range []string{certPath, keyPath} { + for _, file := range []string{certPath, keyPath, certPath + ".new", keyPath + ".new", certPath + ".old", keyPath + ".old"} { _ = os.Remove(file) } }) diff --git a/pkg/certwatcher/certwatcher_test.go b/pkg/certwatcher/certwatcher_test.go index 7e12e42679..1807d866f0 100644 --- a/pkg/certwatcher/certwatcher_test.go +++ b/pkg/certwatcher/certwatcher_test.go @@ -121,6 +121,36 @@ var _ = Describe("CertWatcher", func() { Expect(called.Load()).To(BeNumerically(">=", 1)) }) + It("should reload currentCert when changed with rename", func() { + doneCh := startWatcher() + called := atomic.Int64{} + watcher.RegisterCallback(func(crt tls.Certificate) { + called.Add(1) + Expect(crt.Certificate).ToNot(BeEmpty()) + }) + + firstcert, _ := watcher.GetCertificate(nil) + + err := writeCerts(certPath+".new", keyPath+".new", "192.168.0.2") + Expect(err).ToNot(HaveOccurred()) + + Expect(os.Link(certPath, certPath+".old")).To(Succeed()) + Expect(os.Rename(certPath+".new", certPath)).To(Succeed()) + + Expect(os.Link(keyPath, keyPath+".old")).To(Succeed()) + Expect(os.Rename(keyPath+".new", keyPath)).To(Succeed()) + + Eventually(func() bool { + secondcert, _ := watcher.GetCertificate(nil) + first := firstcert.PrivateKey.(*rsa.PrivateKey) + return first.Equal(secondcert.PrivateKey) + }).ShouldNot(BeTrue()) + + ctxCancel() + Eventually(doneCh, "4s").Should(BeClosed()) + Expect(called.Load()).To(BeNumerically(">=", 1)) + }) + Context("prometheus metric read_certificate_total", func() { var readCertificateTotalBefore float64 var readCertificateErrorsBefore float64 From f4ad8000a25f850545bb983cb9d1889e65e16b3b Mon Sep 17 00:00:00 2001 From: Maxim Muzafarov Date: Thu, 1 Aug 2024 16:57:22 +0100 Subject: [PATCH 2/2] Handle fsnotify.Chmod events as Removals --- pkg/certwatcher/certwatcher.go | 10 +++++++--- pkg/certwatcher/certwatcher_test.go | 9 +++++---- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/pkg/certwatcher/certwatcher.go b/pkg/certwatcher/certwatcher.go index 2b9b60d8d7..fe15fc0dd7 100644 --- a/pkg/certwatcher/certwatcher.go +++ b/pkg/certwatcher/certwatcher.go @@ -173,14 +173,14 @@ func (cw *CertWatcher) ReadCertificate() error { func (cw *CertWatcher) handleEvent(event fsnotify.Event) { // Only care about events which may modify the contents of the file. - if !(isWrite(event) || isRemove(event) || isCreate(event)) { + if !(isWrite(event) || isRemove(event) || isCreate(event) || isChmod(event)) { return } log.V(1).Info("certificate event", "event", event) - // If the file was removed, re-add the watch. - if isRemove(event) { + // If the file was removed or renamed, re-add the watch to the previous name + if isRemove(event) || isChmod(event) { if err := cw.watcher.Add(event.Name); err != nil { log.Error(err, "error re-watching file") } @@ -202,3 +202,7 @@ func isCreate(event fsnotify.Event) bool { func isRemove(event fsnotify.Event) bool { return event.Op.Has(fsnotify.Remove) } + +func isChmod(event fsnotify.Event) bool { + return event.Op.Has(fsnotify.Chmod) +} diff --git a/pkg/certwatcher/certwatcher_test.go b/pkg/certwatcher/certwatcher_test.go index 1807d866f0..1fb247581f 100644 --- a/pkg/certwatcher/certwatcher_test.go +++ b/pkg/certwatcher/certwatcher_test.go @@ -189,17 +189,18 @@ var _ = Describe("CertWatcher", func() { Expect(os.Remove(keyPath)).To(Succeed()) + // Note, we are checking two errors here, because os.Remove generates two fsnotify events: Chmod + Remove Eventually(func() error { readCertificateTotalAfter := testutil.ToFloat64(metrics.ReadCertificateTotal) - if readCertificateTotalAfter != readCertificateTotalBefore+1.0 { - return fmt.Errorf("metric read certificate total expected: %v and got: %v", readCertificateTotalBefore+1.0, readCertificateTotalAfter) + if readCertificateTotalAfter != readCertificateTotalBefore+2.0 { + return fmt.Errorf("metric read certificate total expected: %v and got: %v", readCertificateTotalBefore+2.0, readCertificateTotalAfter) } return nil }, "4s").Should(Succeed()) Eventually(func() error { readCertificateErrorsAfter := testutil.ToFloat64(metrics.ReadCertificateErrors) - if readCertificateErrorsAfter != readCertificateErrorsBefore+1.0 { - return fmt.Errorf("metric read certificate errors expected: %v and got: %v", readCertificateErrorsBefore+1.0, readCertificateErrorsAfter) + if readCertificateErrorsAfter != readCertificateErrorsBefore+2.0 { + return fmt.Errorf("metric read certificate errors expected: %v and got: %v", readCertificateErrorsBefore+2.0, readCertificateErrorsAfter) } return nil }, "4s").Should(Succeed())