filesystem: move addWatch to statusor (envoyproxy#32820)

Risk Level: low Testing: updated tests Docs Changes: n/a Release Notes: n/a envoyproxy/envoy-mobile#176 Signed-off-by: Alyssa Wilk <[email protected]>
ohadvano · Mar 19, 2024 · d3c90ed · d3c90ed
1 parent 7fec609
commit d3c90ed
Show file tree

Hide file tree

Showing 22 changed files with 121 additions and 68 deletions.
diff --git a/envoy/filesystem/watcher.h b/envoy/filesystem/watcher.h
@@ -8,6 +8,7 @@
 #include "envoy/common/platform.h"
 #include "envoy/common/pure.h"
 
+#include "absl/status/statusor.h"
 #include "absl/strings/string_view.h"
 
 namespace Envoy {
@@ -35,8 +36,9 @@ class Watcher {
    *        for the given directory.
    * @param events supplies the events to watch.
    * @param cb supplies the callback to invoke when a change occurs.
+   * @return a failure status if the file does not exist
    */
-  virtual void addWatch(absl::string_view path, uint32_t events, OnChangedCb cb) PURE;
+  virtual absl::Status addWatch(absl::string_view path, uint32_t events, OnChangedCb cb) PURE;
 };
 
 using WatcherPtr = std::unique_ptr<Watcher>;

diff --git a/source/common/config/watched_directory.cc b/source/common/config/watched_directory.cc
@@ -6,8 +6,9 @@ namespace Config {
 WatchedDirectory::WatchedDirectory(const envoy::config::core::v3::WatchedDirectory& config,
                                    Event::Dispatcher& dispatcher) {
   watcher_ = dispatcher.createFilesystemWatcher();
-  watcher_->addWatch(absl::StrCat(config.path(), "/"), Filesystem::Watcher::Events::MovedTo,
-                     [this](uint32_t) { cb_(); });
+  THROW_IF_NOT_OK(watcher_->addWatch(absl::StrCat(config.path(), "/"),
+                                     Filesystem::Watcher::Events::MovedTo,
+                                     [this](uint32_t) { cb_(); }));
 }
 
 } // namespace Config

diff --git a/source/common/filesystem/inotify/watcher_impl.cc b/source/common/filesystem/inotify/watcher_impl.cc
@@ -32,23 +32,24 @@ WatcherImpl::WatcherImpl(Event::Dispatcher& dispatcher, Filesystem::Instance& fi
 
 WatcherImpl::~WatcherImpl() { close(inotify_fd_); }
 
-void WatcherImpl::addWatch(absl::string_view path, uint32_t events, OnChangedCb callback) {
+absl::Status WatcherImpl::addWatch(absl::string_view path, uint32_t events, OnChangedCb callback) {
   // Because of general inotify pain, we always watch the directory that the file lives in,
   // and then synthetically raise per file events.
   auto result_or_error = file_system_.splitPathFromFilename(path);
-  THROW_IF_STATUS_NOT_OK(result_or_error, throw);
+  RETURN_IF_STATUS_NOT_OK(result_or_error);
   const PathSplitResult result = result_or_error.value();
 
   const uint32_t watch_mask = IN_MODIFY | IN_MOVED_TO;
   int watch_fd = inotify_add_watch(inotify_fd_, std::string(result.directory_).c_str(), watch_mask);
   if (watch_fd == -1) {
-    throwEnvoyExceptionOrPanic(
+    return absl::InvalidArgumentError(
         fmt::format("unable to add filesystem watch for file {}: {}", path, errorDetails(errno)));
   }
 
   ENVOY_LOG(debug, "added watch for directory: '{}' file: '{}' fd: {}", result.directory_,
             result.file_, watch_fd);
   callback_map_[watch_fd].watches_.push_back({std::string(result.file_), events, callback});
+  return absl::OkStatus();
 }
 
 void WatcherImpl::onInotifyEvent() {

diff --git a/source/common/filesystem/inotify/watcher_impl.h b/source/common/filesystem/inotify/watcher_impl.h
@@ -26,7 +26,7 @@ class WatcherImpl : public Watcher, Logger::Loggable<Logger::Id::file> {
   ~WatcherImpl() override;
 
   // Filesystem::Watcher
-  void addWatch(absl::string_view path, uint32_t events, OnChangedCb cb) override;
+  absl::Status addWatch(absl::string_view path, uint32_t events, OnChangedCb cb) override;
 
 private:
   struct FileWatch {

diff --git a/source/common/filesystem/kqueue/watcher_impl.cc b/source/common/filesystem/kqueue/watcher_impl.cc
@@ -32,11 +32,13 @@ WatcherImpl::~WatcherImpl() {
   watches_.clear();
 }
 
-void WatcherImpl::addWatch(absl::string_view path, uint32_t events, Watcher::OnChangedCb cb) {
+absl::Status WatcherImpl::addWatch(absl::string_view path, uint32_t events,
+                                   Watcher::OnChangedCb cb) {
   FileWatchPtr watch = addWatch(path, events, cb, false);
   if (watch == nullptr) {
-    throwEnvoyExceptionOrPanic(absl::StrCat("invalid watch path ", path));
+    return absl::InvalidArgumentError(absl::StrCat("invalid watch path ", path));
   }
+  return absl::OkStatus();
 }
 
 WatcherImpl::FileWatchPtr WatcherImpl::addWatch(absl::string_view path, uint32_t events,

diff --git a/source/common/filesystem/kqueue/watcher_impl.h b/source/common/filesystem/kqueue/watcher_impl.h
@@ -27,7 +27,7 @@ class WatcherImpl : public Watcher, Logger::Loggable<Logger::Id::file> {
   ~WatcherImpl();
 
   // Filesystem::Watcher
-  void addWatch(absl::string_view path, uint32_t events, OnChangedCb cb) override;
+  absl::Status addWatch(absl::string_view path, uint32_t events, OnChangedCb cb) override;
 
 private:
   struct FileWatch : LinkedObject<FileWatch> {

diff --git a/source/common/filesystem/win32/watcher_impl.cc b/source/common/filesystem/win32/watcher_impl.cc
@@ -50,13 +50,13 @@ WatcherImpl::~WatcherImpl() {
   ::CloseHandle(thread_exit_event_);
 }
 
-void WatcherImpl::addWatch(absl::string_view path, uint32_t events, OnChangedCb cb) {
+absl::Status WatcherImpl::addWatch(absl::string_view path, uint32_t events, OnChangedCb cb) {
   if (path == Platform::null_device_path) {
-    return;
+    return absl::OkStatus();
   }
 
   const absl::StatusOr<PathSplitResult> result_or_error = file_system_.splitPathFromFilename(path);
-  THROW_IF_STATUS_NOT_OK(result_or_error, throw);
+  RETURN_IF_STATUS_NOT_OK(result_or_error);
   const PathSplitResult& result = result_or_error.value();
   // ReadDirectoryChangesW only has a Unicode version, so we need
   // to use wide strings here
@@ -67,7 +67,7 @@ void WatcherImpl::addWatch(absl::string_view path, uint32_t events, OnChangedCb
       directory.c_str(), GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
       nullptr, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OVERLAPPED, NULL);
   if (dir_handle == INVALID_HANDLE_VALUE) {
-    throw EnvoyException(
+    return absl::InvalidArgumentError(
         fmt::format("unable to open directory {}: {}", result.directory_, GetLastError()));
   }
   std::string fii_key(sizeof(FILE_ID_INFO), '\0');
@@ -108,6 +108,7 @@ void WatcherImpl::addWatch(absl::string_view path, uint32_t events, OnChangedCb
 
   callback_map_[fii_key]->watches_.push_back({file, events, cb});
   ENVOY_LOG(debug, "added watch for file '{}' in directory '{}'", result.file_, result.directory_);
+  return absl::OkStatus();
 }
 
 void WatcherImpl::onDirectoryEvent() {

diff --git a/source/common/filesystem/win32/watcher_impl.h b/source/common/filesystem/win32/watcher_impl.h
@@ -31,7 +31,7 @@ class WatcherImpl : public Watcher, Logger::Loggable<Logger::Id::file> {
   ~WatcherImpl();
 
   // Filesystem::Watcher
-  void addWatch(absl::string_view path, uint32_t events, OnChangedCb cb) override;
+  absl::Status addWatch(absl::string_view path, uint32_t events, OnChangedCb cb) override;
 
 private:
   static void issueFirstRead(ULONG_PTR param);

diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc
@@ -576,8 +576,12 @@ LoaderImpl::LoaderImpl(Event::Dispatcher& dispatcher, ThreadLocal::SlotAllocator
       if (watcher_ == nullptr) {
         watcher_ = dispatcher.createFilesystemWatcher();
       }
-      watcher_->addWatch(layer.disk_layer().symlink_root(), Filesystem::Watcher::Events::MovedTo,
-                         [this](uint32_t) -> void { THROW_IF_NOT_OK(loadNewSnapshot()); });
+      creation_status = watcher_->addWatch(
+          layer.disk_layer().symlink_root(), Filesystem::Watcher::Events::MovedTo,
+          [this](uint32_t) -> void { THROW_IF_NOT_OK(loadNewSnapshot()); });
+      if (!creation_status.ok()) {
+        return;
+      }
       break;
     case envoy::config::bootstrap::v3::RuntimeLayer::LayerSpecifierCase::kRtdsLayer:
       subscriptions_.emplace_back(

diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc
@@ -122,9 +122,9 @@ absl::Status SdsApi::onConfigUpdate(const std::vector<Config::DecodedResourceRef
           // on directory level (e.g. Kubernetes secret update).
           const auto result_or_error = api_.fileSystem().splitPathFromFilename(filename);
           RETURN_IF_STATUS_NOT_OK(result_or_error);
-          watcher_->addWatch(absl::StrCat(result_or_error.value().directory_, "/"),
-                             Filesystem::Watcher::Events::MovedTo,
-                             [this](uint32_t) { onWatchUpdate(); });
+          RETURN_IF_NOT_OK(watcher_->addWatch(absl::StrCat(result_or_error.value().directory_, "/"),
+                                              Filesystem::Watcher::Events::MovedTo,
+                                              [this](uint32_t) { onWatchUpdate(); }));
         }
       } else {
         watcher_.reset(); // Destroy the old watch if any

diff --git a/source/extensions/compression/zstd/common/dictionary_manager.h b/source/extensions/compression/zstd/common/dictionary_manager.h
@@ -44,9 +44,9 @@ template <class T, size_t (*deleter)(T*), unsigned (*getDictId)(const T*)> class
           envoy::config::core::v3::DataSource::SpecifierCase::kFilename) {
         is_watch_added = true;
         const auto& filename = source.filename();
-        watcher_->addWatch(
+        THROW_IF_NOT_OK(watcher_->addWatch(
             filename, Filesystem::Watcher::Events::Modified | Filesystem::Watcher::Events::MovedTo,
-            [this, id, filename](uint32_t) { onDictionaryUpdate(id, filename); });
+            [this, id, filename](uint32_t) { onDictionaryUpdate(id, filename); }));
       }
     }
 

diff --git a/source/extensions/config_subscription/filesystem/filesystem_subscription_impl.cc b/source/extensions/config_subscription/filesystem/filesystem_subscription_impl.cc
@@ -27,11 +27,12 @@ FilesystemSubscriptionImpl::FilesystemSubscriptionImpl(
       stats_(stats), api_(api), validation_visitor_(validation_visitor) {
   if (!path_config_source.has_watched_directory()) {
     file_watcher_ = dispatcher.createFilesystemWatcher();
-    file_watcher_->addWatch(path_, Filesystem::Watcher::Events::MovedTo, [this](uint32_t) {
-      if (started_) {
-        refresh();
-      }
-    });
+    THROW_IF_NOT_OK(
+        file_watcher_->addWatch(path_, Filesystem::Watcher::Events::MovedTo, [this](uint32_t) {
+          if (started_) {
+            refresh();
+          }
+        }));
   } else {
     directory_watcher_ =
         std::make_unique<WatchedDirectory>(path_config_source.watched_directory(), dispatcher);

diff --git a/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc b/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc
@@ -17,8 +17,8 @@ InjectedResourceMonitor::InjectedResourceMonitor(
     Server::Configuration::ResourceMonitorFactoryContext& context)
     : filename_(config.filename()),
       watcher_(context.mainThreadDispatcher().createFilesystemWatcher()), api_(context.api()) {
-  watcher_->addWatch(filename_, Filesystem::Watcher::Events::MovedTo,
-                     [this](uint32_t) { onFileChanged(); });
+  THROW_IF_NOT_OK(watcher_->addWatch(filename_, Filesystem::Watcher::Events::MovedTo,
+                                     [this](uint32_t) { onFileChanged(); }));
 }
 
 void InjectedResourceMonitor::onFileChanged() { file_changed_ = true; }

diff --git a/test/common/config/watched_directory_test.cc b/test/common/config/watched_directory_test.cc
@@ -7,6 +7,7 @@
 
 #include "gtest/gtest.h"
 
+using testing::DoAll;
 using testing::Return;
 using testing::SaveArg;
 
@@ -21,7 +22,7 @@ TEST(WatchedDirectory, All) {
   EXPECT_CALL(dispatcher, createFilesystemWatcher_()).WillOnce(Return(watcher));
   Filesystem::Watcher::OnChangedCb cb;
   EXPECT_CALL(*watcher, addWatch("foo/bar/", Filesystem::Watcher::Events::MovedTo, _))
-      .WillOnce(SaveArg<2>(&cb));
+      .WillOnce(DoAll(SaveArg<2>(&cb), Return(absl::OkStatus())));
   WatchedDirectory wd(config, dispatcher);
   bool called = false;
   wd.setCallback([&called] { called = true; });

diff --git a/test/common/filesystem/watcher_impl_test.cc b/test/common/filesystem/watcher_impl_test.cc
@@ -47,11 +47,14 @@ TEST_F(WatcherImplTest, All) {
 
   WatchCallback callback;
   EXPECT_CALL(callback, called(Watcher::Events::MovedTo)).Times(2);
-  watcher->addWatch(TestEnvironment::temporaryPath("envoy_test/watcher_link"),
-                    Watcher::Events::MovedTo, [&](uint32_t events) -> void {
-                      callback.called(events);
-                      dispatcher_->exit();
-                    });
+  ASSERT_TRUE(watcher
+                  ->addWatch(TestEnvironment::temporaryPath("envoy_test/watcher_link"),
+                             Watcher::Events::MovedTo,
+                             [&](uint32_t events) -> void {
+                               callback.called(events);
+                               dispatcher_->exit();
+                             })
+                  .ok());
   TestEnvironment::renameFile(TestEnvironment::temporaryPath("envoy_test/watcher_new_link"),
                               TestEnvironment::temporaryPath("envoy_test/watcher_link"));
   dispatcher_->run(Event::Dispatcher::RunType::Block);
@@ -75,11 +78,14 @@ TEST_F(WatcherImplTest, Create) {
   { std::ofstream file(TestEnvironment::temporaryPath("envoy_test/watcher_target")); }
 
   WatchCallback callback;
-  watcher->addWatch(TestEnvironment::temporaryPath("envoy_test/watcher_link"),
-                    Watcher::Events::MovedTo, [&](uint32_t events) -> void {
-                      callback.called(events);
-                      dispatcher_->exit();
-                    });
+  ASSERT_TRUE(watcher
+                  ->addWatch(TestEnvironment::temporaryPath("envoy_test/watcher_link"),
+                             Watcher::Events::MovedTo,
+                             [&](uint32_t events) -> void {
+                               callback.called(events);
+                               dispatcher_->exit();
+                             })
+                  .ok());
 
   { std::ofstream file(TestEnvironment::temporaryPath("envoy_test/other_file")); }
   dispatcher_->run(Event::Dispatcher::RunType::NonBlock);
@@ -99,11 +105,14 @@ TEST_F(WatcherImplTest, Modify) {
   std::ofstream file(TestEnvironment::temporaryPath("envoy_test/watcher_target"));
 
   WatchCallback callback;
-  watcher->addWatch(TestEnvironment::temporaryPath("envoy_test/watcher_target"),
-                    Watcher::Events::Modified, [&](uint32_t events) -> void {
-                      callback.called(events);
-                      dispatcher_->exit();
-                    });
+  ASSERT_TRUE(watcher
+                  ->addWatch(TestEnvironment::temporaryPath("envoy_test/watcher_target"),
+                             Watcher::Events::Modified,
+                             [&](uint32_t events) -> void {
+                               callback.called(events);
+                               dispatcher_->exit();
+                             })
+                  .ok());
   dispatcher_->run(Event::Dispatcher::RunType::NonBlock);
 
   file << "text" << std::flush;
@@ -115,13 +124,14 @@ TEST_F(WatcherImplTest, Modify) {
 TEST_F(WatcherImplTest, BadPath) {
   Filesystem::WatcherPtr watcher = dispatcher_->createFilesystemWatcher();
 
-  EXPECT_THROW(
-      watcher->addWatch("this_is_not_a_file", Watcher::Events::MovedTo, [&](uint32_t) -> void {}),
-      EnvoyException);
+  EXPECT_FALSE(
+      watcher->addWatch("this_is_not_a_file", Watcher::Events::MovedTo, [&](uint32_t) -> void {})
+          .ok());
 
-  EXPECT_THROW(watcher->addWatch("this_is_not_a_dir/file", Watcher::Events::MovedTo,
-                                 [&](uint32_t) -> void {}),
-               EnvoyException);
+  EXPECT_FALSE(
+      watcher
+          ->addWatch("this_is_not_a_dir/file", Watcher::Events::MovedTo, [&](uint32_t) -> void {})
+          .ok());
 }
 
 TEST_F(WatcherImplTest, ParentDirectoryRemoved) {
@@ -132,9 +142,11 @@ TEST_F(WatcherImplTest, ParentDirectoryRemoved) {
   WatchCallback callback;
   EXPECT_CALL(callback, called(testing::_)).Times(0);
 
-  watcher->addWatch(TestEnvironment::temporaryPath("envoy_test_empty/watcher_link"),
-                    Watcher::Events::MovedTo,
-                    [&](uint32_t events) -> void { callback.called(events); });
+  ASSERT_TRUE(watcher
+                  ->addWatch(TestEnvironment::temporaryPath("envoy_test_empty/watcher_link"),
+                             Watcher::Events::MovedTo,
+                             [&](uint32_t events) -> void { callback.called(events); })
+                  .ok());
 
   int rc = rmdir(TestEnvironment::temporaryPath("envoy_test_empty").c_str());
   EXPECT_EQ(0, rc);
@@ -146,9 +158,9 @@ TEST_F(WatcherImplTest, RootDirectoryPath) {
   Filesystem::WatcherPtr watcher = dispatcher_->createFilesystemWatcher();
 
 #ifndef WIN32
-  EXPECT_NO_THROW(watcher->addWatch("/", Watcher::Events::MovedTo, [&](uint32_t) -> void {}));
+  EXPECT_TRUE(watcher->addWatch("/", Watcher::Events::MovedTo, [&](uint32_t) -> void {}).ok());
 #else
-  EXPECT_NO_THROW(watcher->addWatch("c:\\", Watcher::Events::MovedTo, [&](uint32_t) -> void {}));
+  EXPECT_TRUE(watcher->addWatch("c:\\", Watcher::Events::MovedTo, [&](uint32_t) -> void {}).ok());
 #endif
 }
 
@@ -169,11 +181,14 @@ TEST_F(WatcherImplTest, SymlinkAtomicRename) {
 
   WatchCallback callback;
   EXPECT_CALL(callback, called(Watcher::Events::MovedTo));
-  watcher->addWatch(TestEnvironment::temporaryPath("envoy_test/"), Watcher::Events::MovedTo,
-                    [&](uint32_t events) -> void {
-                      callback.called(events);
-                      dispatcher_->exit();
-                    });
+  ASSERT_TRUE(watcher
+                  ->addWatch(TestEnvironment::temporaryPath("envoy_test/"),
+                             Watcher::Events::MovedTo,
+                             [&](uint32_t events) -> void {
+                               callback.called(events);
+                               dispatcher_->exit();
+                             })
+                  .ok());
 
   TestEnvironment::createPath(TestEnvironment::temporaryPath("envoy_test/..timestamp2"));
   { std::ofstream file(TestEnvironment::temporaryPath("envoy_test/..timestamp2/watched_file")); }

diff --git a/test/common/runtime/runtime_impl_test.cc b/test/common/runtime/runtime_impl_test.cc
@@ -55,6 +55,7 @@ class LoaderImplTest : public testing::Test {
               Invoke([this](absl::string_view path, uint32_t, Filesystem::Watcher::OnChangedCb cb) {
                 EXPECT_EQ(path, expected_watch_root_);
                 on_changed_cbs_.emplace_back(cb);
+                return absl::OkStatus();
               }));
       return mock_watcher;
     }));
@@ -97,6 +98,7 @@ class DiskLoaderImplTest : public LoaderImplTest {
     absl::Status creation_status;
     loader_ = std::make_unique<LoaderImpl>(dispatcher_, tls_, layered_runtime, local_info_, store_,
                                            generator_, validation_visitor_, *api_, creation_status);
+    THROW_IF_NOT_OK(creation_status);
   }
 
   void write(const std::string& path, const std::string& value) {
@@ -329,6 +331,19 @@ TEST_F(DiskLoaderImplTest, OverrideFolderDoesNotExist) {
   EXPECT_EQ(1, store_.counter("runtime.override_dir_not_exists").value());
 }
 
+TEST_F(DiskLoaderImplTest, FileDoesNotExist) {
+  EXPECT_CALL(dispatcher_, createFilesystemWatcher_()).WillRepeatedly(InvokeWithoutArgs([] {
+    Filesystem::MockWatcher* mock_watcher = new NiceMock<Filesystem::MockWatcher>();
+    EXPECT_CALL(*mock_watcher, addWatch(_, Filesystem::Watcher::Events::MovedTo, _))
+        .WillRepeatedly(Return(absl::InvalidArgumentError("file does not exist")));
+    return mock_watcher;
+  }));
+
+  EXPECT_THROW_WITH_MESSAGE(
+      run("test/common/runtime/test_data/current", "envoy_override_does_not_exist"), EnvoyException,
+      "file does not exist");
+}
+
 TEST_F(DiskLoaderImplTest, PercentHandling) {
   setup();
   run("test/common/runtime/test_data/current", "envoy_override");