From 8c44801ee3a2f31210bf87fcd3d31c073a60603f Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 11 Oct 2023 15:22:47 -0700 Subject: [PATCH 1/6] Fix NGF fails to recover if conf files are unexpectedly removed --- internal/mode/static/nginx/file/manager.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/mode/static/nginx/file/manager.go b/internal/mode/static/nginx/file/manager.go index 560ab878af..10998418df 100644 --- a/internal/mode/static/nginx/file/manager.go +++ b/internal/mode/static/nginx/file/manager.go @@ -88,7 +88,11 @@ func NewManagerImpl(logger logr.Logger, osFileManager OSFileManager) *ManagerImp func (m *ManagerImpl) ReplaceFiles(files []File) error { for _, path := range m.lastWrittenPaths { if err := m.osFileManager.Remove(path); err != nil { - return fmt.Errorf("failed to delete file %q: %w", path, err) + if os.IsNotExist(err) { + m.logger.V(1).Info("file not found, failed to delete file %q: %w", path, err) + } else { + return fmt.Errorf("failed to delete file %q: %w", path, err) + } } m.logger.Info("deleted file", "path", path) From 78fe2475c0a604d46cc48fc50c48463b6d38a8f8 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Thu, 12 Oct 2023 11:07:11 -0700 Subject: [PATCH 2/6] Add test for file does not exist and adjust logging message --- internal/mode/static/manager.go | 1 + internal/mode/static/nginx/file/manager.go | 17 ++++++++----- .../mode/static/nginx/file/manager_test.go | 25 ++++++++++++++++--- 3 files changed, 34 insertions(+), 9 deletions(-) diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go index 90ec48275b..e62046118d 100644 --- a/internal/mode/static/manager.go +++ b/internal/mode/static/manager.go @@ -172,6 +172,7 @@ func StartManager(cfg config.Config) error { nginxFileMgr: file.NewManagerImpl( cfg.Logger.WithName("nginxFileManager"), file.NewStdLibOSFileManager(), + nil, ), nginxRuntimeMgr: ngxruntime.NewManagerImpl(ngxruntimeCollector), statusUpdater: statusUpdater, diff --git a/internal/mode/static/nginx/file/manager.go b/internal/mode/static/nginx/file/manager.go index 10998418df..1b4fcbdfe1 100644 --- a/internal/mode/static/nginx/file/manager.go +++ b/internal/mode/static/nginx/file/manager.go @@ -76,10 +76,11 @@ type ManagerImpl struct { } // NewManagerImpl creates a new NewManagerImpl. -func NewManagerImpl(logger logr.Logger, osFileManager OSFileManager) *ManagerImpl { +func NewManagerImpl(logger logr.Logger, osFileManager OSFileManager, lastWrittenPaths []string) *ManagerImpl { return &ManagerImpl{ - logger: logger, - osFileManager: osFileManager, + logger: logger, + osFileManager: osFileManager, + lastWrittenPaths: lastWrittenPaths, } } @@ -89,13 +90,17 @@ func (m *ManagerImpl) ReplaceFiles(files []File) error { for _, path := range m.lastWrittenPaths { if err := m.osFileManager.Remove(path); err != nil { if os.IsNotExist(err) { - m.logger.V(1).Info("file not found, failed to delete file %q: %w", path, err) + m.logger.V(1).Info( + "File not found when attempting to replace", + "path", path, + "error", err, + ) } else { return fmt.Errorf("failed to delete file %q: %w", path, err) } } - m.logger.Info("deleted file", "path", path) + m.logger.Info("Deleted file", "path", path) } // In some cases, NGINX reads files in runtime, like a JWK. If you remove such file, NGINX will fail @@ -110,7 +115,7 @@ func (m *ManagerImpl) ReplaceFiles(files []File) error { } m.lastWrittenPaths = append(m.lastWrittenPaths, file.Path) - m.logger.Info("wrote file", "path", file.Path) + m.logger.Info("Wrote file", "path", file.Path) } return nil diff --git a/internal/mode/static/nginx/file/manager_test.go b/internal/mode/static/nginx/file/manager_test.go index c6f52a526c..0d0f5872b3 100644 --- a/internal/mode/static/nginx/file/manager_test.go +++ b/internal/mode/static/nginx/file/manager_test.go @@ -60,7 +60,7 @@ var _ = Describe("EventHandler", func() { } BeforeAll(func() { - mgr = file.NewManagerImpl(zap.New(), file.NewStdLibOSFileManager()) + mgr = file.NewManagerImpl(zap.New(), file.NewStdLibOSFileManager(), nil) tmpDir = GinkgoT().TempDir() regular1 = file.File{ @@ -116,9 +116,28 @@ var _ = Describe("EventHandler", func() { }) }) + When("file does not exist", func() { + It("should not error", func() { + tmpDir := GinkgoT().TempDir() + mgr := file.NewManagerImpl( + zap.New(), + file.NewStdLibOSFileManager(), + []string{filepath.Join(tmpDir, "file-does-not-exist.md")}) + files := []file.File{ + { + Type: file.TypeRegular, + Path: filepath.Join(tmpDir, "regular-1.conf"), + Content: []byte("regular-1"), + }, + } + + Expect(mgr.ReplaceFiles(files)).ShouldNot(HaveOccurred()) + }) + }) + When("file type is not supported", func() { It("should panic", func() { - mgr := file.NewManagerImpl(zap.New(), nil) + mgr := file.NewManagerImpl(zap.New(), nil, nil) files := []file.File{ { @@ -155,7 +174,7 @@ var _ = Describe("EventHandler", func() { DescribeTable( "should return error on file IO error", func(fakeOSMgr *filefakes.FakeOSFileManager) { - mgr := file.NewManagerImpl(zap.New(), fakeOSMgr) + mgr := file.NewManagerImpl(zap.New(), fakeOSMgr, nil) // special case for Remove // to kick off removing, we need to successfully write files beforehand From 85c253646d4e4948a122c64f40c28e653b144ae3 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Thu, 12 Oct 2023 11:31:42 -0700 Subject: [PATCH 3/6] Adjust wording on log message --- internal/mode/static/nginx/file/manager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/mode/static/nginx/file/manager.go b/internal/mode/static/nginx/file/manager.go index 1b4fcbdfe1..0294a083e2 100644 --- a/internal/mode/static/nginx/file/manager.go +++ b/internal/mode/static/nginx/file/manager.go @@ -91,7 +91,7 @@ func (m *ManagerImpl) ReplaceFiles(files []File) error { if err := m.osFileManager.Remove(path); err != nil { if os.IsNotExist(err) { m.logger.V(1).Info( - "File not found when attempting to replace", + "File not found when attempting to delete", "path", path, "error", err, ) From 340cd8bfb92f7a168e046f9602cf04200b932606 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Thu, 12 Oct 2023 12:04:08 -0700 Subject: [PATCH 4/6] Refactor logging on file does not exist --- internal/mode/static/nginx/file/manager.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/mode/static/nginx/file/manager.go b/internal/mode/static/nginx/file/manager.go index 0294a083e2..e031e1c8cd 100644 --- a/internal/mode/static/nginx/file/manager.go +++ b/internal/mode/static/nginx/file/manager.go @@ -90,14 +90,14 @@ func (m *ManagerImpl) ReplaceFiles(files []File) error { for _, path := range m.lastWrittenPaths { if err := m.osFileManager.Remove(path); err != nil { if os.IsNotExist(err) { - m.logger.V(1).Info( + m.logger.Info( "File not found when attempting to delete", "path", path, "error", err, ) - } else { - return fmt.Errorf("failed to delete file %q: %w", path, err) + continue } + return fmt.Errorf("failed to delete file %q: %w", path, err) } m.logger.Info("Deleted file", "path", path) From 6b935feac6c7a43d1eb5690038819d92c5a8c2b2 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Thu, 12 Oct 2023 14:39:21 -0700 Subject: [PATCH 5/6] Remove lastWrittenPaths from NewManagerImpl and adjust tests --- internal/mode/static/manager.go | 1 - internal/mode/static/nginx/file/manager.go | 7 +++---- .../mode/static/nginx/file/manager_test.go | 18 ++++++++++-------- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go index e62046118d..90ec48275b 100644 --- a/internal/mode/static/manager.go +++ b/internal/mode/static/manager.go @@ -172,7 +172,6 @@ func StartManager(cfg config.Config) error { nginxFileMgr: file.NewManagerImpl( cfg.Logger.WithName("nginxFileManager"), file.NewStdLibOSFileManager(), - nil, ), nginxRuntimeMgr: ngxruntime.NewManagerImpl(ngxruntimeCollector), statusUpdater: statusUpdater, diff --git a/internal/mode/static/nginx/file/manager.go b/internal/mode/static/nginx/file/manager.go index e031e1c8cd..632a3af34f 100644 --- a/internal/mode/static/nginx/file/manager.go +++ b/internal/mode/static/nginx/file/manager.go @@ -76,11 +76,10 @@ type ManagerImpl struct { } // NewManagerImpl creates a new NewManagerImpl. -func NewManagerImpl(logger logr.Logger, osFileManager OSFileManager, lastWrittenPaths []string) *ManagerImpl { +func NewManagerImpl(logger logr.Logger, osFileManager OSFileManager) *ManagerImpl { return &ManagerImpl{ - logger: logger, - osFileManager: osFileManager, - lastWrittenPaths: lastWrittenPaths, + logger: logger, + osFileManager: osFileManager, } } diff --git a/internal/mode/static/nginx/file/manager_test.go b/internal/mode/static/nginx/file/manager_test.go index 0d0f5872b3..5f52162bd7 100644 --- a/internal/mode/static/nginx/file/manager_test.go +++ b/internal/mode/static/nginx/file/manager_test.go @@ -60,7 +60,7 @@ var _ = Describe("EventHandler", func() { } BeforeAll(func() { - mgr = file.NewManagerImpl(zap.New(), file.NewStdLibOSFileManager(), nil) + mgr = file.NewManagerImpl(zap.New(), file.NewStdLibOSFileManager()) tmpDir = GinkgoT().TempDir() regular1 = file.File{ @@ -118,11 +118,10 @@ var _ = Describe("EventHandler", func() { When("file does not exist", func() { It("should not error", func() { + fakeOSMgr := &filefakes.FakeOSFileManager{} tmpDir := GinkgoT().TempDir() - mgr := file.NewManagerImpl( - zap.New(), - file.NewStdLibOSFileManager(), - []string{filepath.Join(tmpDir, "file-does-not-exist.md")}) + mgr := file.NewManagerImpl(zap.New(), fakeOSMgr) + files := []file.File{ { Type: file.TypeRegular, @@ -131,13 +130,16 @@ var _ = Describe("EventHandler", func() { }, } - Expect(mgr.ReplaceFiles(files)).ShouldNot(HaveOccurred()) + Expect(mgr.ReplaceFiles(files)).ToNot(HaveOccurred()) + fakeOSMgr.RemoveReturns(os.ErrNotExist) + + Expect(mgr.ReplaceFiles(files)).ToNot(HaveOccurred()) }) }) When("file type is not supported", func() { It("should panic", func() { - mgr := file.NewManagerImpl(zap.New(), nil, nil) + mgr := file.NewManagerImpl(zap.New(), nil) files := []file.File{ { @@ -174,7 +176,7 @@ var _ = Describe("EventHandler", func() { DescribeTable( "should return error on file IO error", func(fakeOSMgr *filefakes.FakeOSFileManager) { - mgr := file.NewManagerImpl(zap.New(), fakeOSMgr, nil) + mgr := file.NewManagerImpl(zap.New(), fakeOSMgr) // special case for Remove // to kick off removing, we need to successfully write files beforehand From 06cdadf08de548ac3eb9d04cff53dd5b0ed860e2 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Fri, 13 Oct 2023 08:57:35 -0700 Subject: [PATCH 6/6] Add small review fixes --- internal/mode/static/nginx/file/manager_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/internal/mode/static/nginx/file/manager_test.go b/internal/mode/static/nginx/file/manager_test.go index 5f52162bd7..07f0478ae9 100644 --- a/internal/mode/static/nginx/file/manager_test.go +++ b/internal/mode/static/nginx/file/manager_test.go @@ -119,20 +119,19 @@ var _ = Describe("EventHandler", func() { When("file does not exist", func() { It("should not error", func() { fakeOSMgr := &filefakes.FakeOSFileManager{} - tmpDir := GinkgoT().TempDir() mgr := file.NewManagerImpl(zap.New(), fakeOSMgr) files := []file.File{ { Type: file.TypeRegular, - Path: filepath.Join(tmpDir, "regular-1.conf"), + Path: "regular-1.conf", Content: []byte("regular-1"), }, } Expect(mgr.ReplaceFiles(files)).ToNot(HaveOccurred()) - fakeOSMgr.RemoveReturns(os.ErrNotExist) + fakeOSMgr.RemoveReturns(os.ErrNotExist) Expect(mgr.ReplaceFiles(files)).ToNot(HaveOccurred()) }) })