Coalesce directory management to Envoy::TestEnvironment (#9721)

- Drop std::[*]filesystem logic - Implement/tests for new Envoy::Filesystem::splitFileName - Remove unneeded TestEnvironment::createParentPath - Move/rename Envoy::TestUtility::createDirectory to Envoy::TestEnvironment::createPath - Move/rename Envoy::TestUtility::removeDirectory to Envoy::TestEnvironment::removePath - Move Envoy::TestUtility::renameFile to Envoy::TestEnvironment - Move Envoy::TestUtility::createSymlink to Envoy::TestEnvironment - Move AtomicFileUpdater from test_common utility to environment - Simplify Envoy::Filesystem::Watcher implementations using Filesystem - Clean up watcher_impl flavors for splitFileName API - Implement/new tests for Envoy::Filesystem::illegalPath for win32 - Implement watcher_impl for win32 [Initial development by Sam Smith] - Ensure top level test tempdir exists when requested Co-authored-by: Sunjay Bhatia <[email protected]> Co-authored-by: William A Rowe Jr <[email protected]> Co-authored-by: Sam Smith <[email protected]> Signed-off-by: William A Rowe Jr <[email protected]>
envoyproxy · Jan 26, 2020 · 1dc6e43 · 1dc6e43
1 parent 6e3d61a
commit 1dc6e43
Show file tree

Hide file tree

Showing 43 changed files with 959 additions and 258 deletions.
diff --git a/STYLE.md b/STYLE.md
@@ -4,7 +4,8 @@
   issues are taken care of automatically. The CircleCI tests will automatically check
   the code format and fail. There are make targets that can both check the format
   (check_format) as well as fix the code format for you (fix_format). Errors in
-  .clang-tidy are enforced while other warnings are suggestions.
+  .clang-tidy are enforced while other warnings are suggestions. Note that code and
+  comment blocks designated `clang-format off` must be closed with `clang-format on`.
   To run these checks locally, see [Support Tools](support/README.md).
 * Beyond code formatting, for the most part Envoy uses the
   [Google C++ style guidelines](https://google.github.io/styleguide/cppguide.html).

diff --git a/include/envoy/filesystem/filesystem.h b/include/envoy/filesystem/filesystem.h
@@ -65,6 +65,15 @@ class File {
 
 using FilePtr = std::unique_ptr<File>;
 
+/**
+ * Contains the result of splitting the file name and its parent directory from
+ * a given file path.
+ */
+struct PathSplitResult {
+  absl::string_view directory_;
+  absl::string_view file_;
+};
+
 /**
  * Abstraction for some basic filesystem operations
  */
@@ -102,6 +111,13 @@ class Instance {
    */
   virtual std::string fileReadToEnd(const std::string& path) PURE;
 
+  /**
+   * @path file path to split
+   * @return PathSplitResult containing the parent directory of the input path and the file name
+   * @note will throw an exception if path does not contain any path separator character
+   */
+  virtual PathSplitResult splitPathFromFilename(absl::string_view path) PURE;
+
   /**
    * Determine if the path is on a list of paths Envoy will refuse to access. This
    * is a basic sanity check for users, blacklisting some clearly bad paths. Paths
@@ -130,10 +146,6 @@ struct DirectoryEntry {
   }
 };
 
-/**
- * Abstraction for listing a directory.
- * TODO(sesmith177): replace with std::filesystem::directory_iterator once we move to C++17
- */
 class DirectoryIteratorImpl;
 class DirectoryIterator {
 public:

diff --git a/include/envoy/filesystem/watcher.h b/include/envoy/filesystem/watcher.h
@@ -8,6 +8,8 @@
 #include "envoy/common/platform.h"
 #include "envoy/common/pure.h"
 
+#include "absl/strings/string_view.h"
+
 namespace Envoy {
 namespace Filesystem {
 
@@ -31,7 +33,7 @@ class Watcher {
    * @param events supplies the events to watch.
    * @param cb supplies the callback to invoke when a change occurs.
    */
-  virtual void addWatch(const std::string& path, uint32_t events, OnChangedCb cb) PURE;
+  virtual void addWatch(absl::string_view path, uint32_t events, OnChangedCb cb) PURE;
 };
 
 using WatcherPtr = std::unique_ptr<Watcher>;

diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc
@@ -133,7 +133,7 @@ FileEventPtr DispatcherImpl::createFileEvent(int fd, FileReadyCb cb, FileTrigger
 
 Filesystem::WatcherPtr DispatcherImpl::createFilesystemWatcher() {
   ASSERT(isThreadSafe());
-  return Filesystem::WatcherPtr{new Filesystem::WatcherImpl(*this)};
+  return Filesystem::WatcherPtr{new Filesystem::WatcherImpl(*this, api_)};
 }
 
 Network::ListenerPtr DispatcherImpl::createListener(Network::SocketSharedPtr&& socket,

diff --git a/source/common/filesystem/BUILD b/source/common/filesystem/BUILD
@@ -79,6 +79,9 @@ envoy_cc_library(
         "//bazel:apple": [
             "kqueue/watcher_impl.cc",
         ],
+        "//bazel:windows_x86_64": [
+            "win32/watcher_impl.cc",
+        ],
         "//conditions:default": [
             "inotify/watcher_impl.cc",
         ],
@@ -87,6 +90,9 @@ envoy_cc_library(
         "//bazel:apple": [
             "kqueue/watcher_impl.h",
         ],
+        "//bazel:windows_x86_64": [
+            "win32/watcher_impl.h",
+        ],
         "//conditions:default": [
             "inotify/watcher_impl.h",
         ],
@@ -96,13 +102,21 @@ envoy_cc_library(
     ],
     strip_include_prefix = select({
         "//bazel:apple": "kqueue",
+        "//bazel:windows_x86_64": "win32",
         "//conditions:default": "inotify",
     }),
     deps = [
+        "//include/envoy/api:api_interface",
         "//include/envoy/event:dispatcher_interface",
         "//source/common/common:assert_lib",
         "//source/common/common:linked_object",
         "//source/common/common:minimal_logger_lib",
         "//source/common/common:utility_lib",
-    ],
+    ] + select({
+        "//bazel:windows_x86_64": [
+            "//source/common/api:os_sys_calls_lib",
+            "//source/common/common:thread_lib",
+        ],
+        "//conditions:default": [],
+    }),
 )
diff --git a/source/common/filesystem/directory.h b/source/common/filesystem/directory.h
@@ -2,6 +2,8 @@
 
 #include <string>
 
+#include "envoy/filesystem/filesystem.h"
+
 #include "common/filesystem/directory_iterator_impl.h"
 
 namespace Envoy {

diff --git a/source/common/filesystem/inotify/watcher_impl.cc b/source/common/filesystem/inotify/watcher_impl.cc
@@ -3,6 +3,7 @@
 #include <cstdint>
 #include <string>
 
+#include "envoy/api/api.h"
 #include "envoy/common/exception.h"
 #include "envoy/event/dispatcher.h"
 #include "envoy/event/file_event.h"
@@ -15,8 +16,8 @@
 namespace Envoy {
 namespace Filesystem {
 
-WatcherImpl::WatcherImpl(Event::Dispatcher& dispatcher)
-    : inotify_fd_(inotify_init1(IN_NONBLOCK)),
+WatcherImpl::WatcherImpl(Event::Dispatcher& dispatcher, Api::Api& api)
+    : api_(api), inotify_fd_(inotify_init1(IN_NONBLOCK)),
       inotify_event_(dispatcher.createFileEvent(
           inotify_fd_,
           [this](uint32_t events) -> void {
@@ -30,26 +31,21 @@ WatcherImpl::WatcherImpl(Event::Dispatcher& dispatcher)
 
 WatcherImpl::~WatcherImpl() { close(inotify_fd_); }
 
-void WatcherImpl::addWatch(const std::string& path, uint32_t events, OnChangedCb callback) {
+void 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.
-  size_t last_slash = path.rfind('/');
-  if (last_slash == std::string::npos) {
-    throw EnvoyException(absl::StrCat("invalid watch path ", path));
-  }
-
-  std::string directory = last_slash != 0 ? path.substr(0, last_slash) : "/";
-  std::string file = StringUtil::subspan(path, last_slash + 1, path.size());
+  const PathSplitResult result = api_.fileSystem().splitPathFromFilename(path);
 
   const uint32_t watch_mask = IN_MODIFY | IN_MOVED_TO;
-  int watch_fd = inotify_add_watch(inotify_fd_, directory.c_str(), watch_mask);
+  int watch_fd = inotify_add_watch(inotify_fd_, std::string(result.directory_).c_str(), watch_mask);
   if (watch_fd == -1) {
     throw EnvoyException(
         fmt::format("unable to add filesystem watch for file {}: {}", path, strerror(errno)));
   }
 
-  ENVOY_LOG(debug, "added watch for directory: '{}' file: '{}' fd: {}", directory, file, watch_fd);
-  callback_map_[watch_fd].watches_.push_back({file, events, callback});
+  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});
 }
 
 void WatcherImpl::onInotifyEvent() {

diff --git a/source/common/filesystem/inotify/watcher_impl.h b/source/common/filesystem/inotify/watcher_impl.h
@@ -5,6 +5,7 @@
 #include <string>
 #include <unordered_map>
 
+#include "envoy/api/api.h"
 #include "envoy/event/dispatcher.h"
 #include "envoy/filesystem/watcher.h"
 
@@ -20,11 +21,11 @@ namespace Filesystem {
  */
 class WatcherImpl : public Watcher, Logger::Loggable<Logger::Id::file> {
 public:
-  WatcherImpl(Event::Dispatcher& dispatcher);
+  WatcherImpl(Event::Dispatcher& dispatcher, Api::Api& api);
   ~WatcherImpl() override;
 
   // Filesystem::Watcher
-  void addWatch(const std::string& path, uint32_t events, OnChangedCb cb) override;
+  void addWatch(absl::string_view path, uint32_t events, OnChangedCb cb) override;
 
 private:
   struct FileWatch {
@@ -39,6 +40,7 @@ class WatcherImpl : public Watcher, Logger::Loggable<Logger::Id::file> {
 
   void onInotifyEvent();
 
+  Api::Api& api_;
   int inotify_fd_;
   Event::FileEventPtr inotify_event_;
   std::unordered_map<int, DirectoryWatch> callback_map_;

diff --git a/source/common/filesystem/kqueue/watcher_impl.cc b/source/common/filesystem/kqueue/watcher_impl.cc
@@ -16,44 +16,40 @@
 namespace Envoy {
 namespace Filesystem {
 
-WatcherImpl::WatcherImpl(Event::Dispatcher& dispatcher)
-    : queue_(kqueue()), kqueue_event_(dispatcher.createFileEvent(
-                            queue_,
-                            [this](uint32_t events) -> void {
-                              if (events & Event::FileReadyType::Read) {
-                                onKqueueEvent();
-                              }
-                            },
-                            Event::FileTriggerType::Edge, Event::FileReadyType::Read)) {}
+WatcherImpl::WatcherImpl(Event::Dispatcher& dispatcher, Api::Api& api)
+    : api_(api), queue_(kqueue()), kqueue_event_(dispatcher.createFileEvent(
+                                       queue_,
+                                       [this](uint32_t events) -> void {
+                                         if (events & Event::FileReadyType::Read) {
+                                           onKqueueEvent();
+                                         }
+                                       },
+                                       Event::FileTriggerType::Edge, Event::FileReadyType::Read)) {}
 
 WatcherImpl::~WatcherImpl() {
   close(queue_);
   watches_.clear();
 }
 
-void WatcherImpl::addWatch(const std::string& path, uint32_t events, Watcher::OnChangedCb cb) {
+void WatcherImpl::addWatch(absl::string_view path, uint32_t events, Watcher::OnChangedCb cb) {
   FileWatchPtr watch = addWatch(path, events, cb, false);
   if (watch == nullptr) {
     throw EnvoyException(absl::StrCat("invalid watch path ", path));
   }
 }
 
-WatcherImpl::FileWatchPtr WatcherImpl::addWatch(const std::string& path, uint32_t events,
+WatcherImpl::FileWatchPtr WatcherImpl::addWatch(absl::string_view path, uint32_t events,
                                                 Watcher::OnChangedCb cb, bool path_must_exist) {
   bool watching_dir = false;
-  int watch_fd = open(path.c_str(), O_SYMLINK);
+  std::string pathname(path);
+  int watch_fd = open(pathname.c_str(), O_SYMLINK);
   if (watch_fd == -1) {
     if (path_must_exist) {
       return nullptr;
     }
 
-    size_t last_slash = path.rfind('/');
-    if (last_slash == std::string::npos) {
-      return nullptr;
-    }
-
-    std::string directory = path.substr(0, last_slash);
-    watch_fd = open(directory.c_str(), 0);
+    watch_fd =
+        open(std::string(api_.fileSystem().splitPathFromFilename(path).directory_).c_str(), 0);
     if (watch_fd == -1) {
       return nullptr;
     }
@@ -63,7 +59,7 @@ WatcherImpl::FileWatchPtr WatcherImpl::addWatch(const std::string& path, uint32_
 
   FileWatchPtr watch(new FileWatch());
   watch->fd_ = watch_fd;
-  watch->file_ = path;
+  watch->file_ = pathname;
   watch->events_ = events;
   watch->callback_ = cb;
   watch->watching_dir_ = watching_dir;

diff --git a/source/common/filesystem/kqueue/watcher_impl.h b/source/common/filesystem/kqueue/watcher_impl.h
@@ -4,6 +4,7 @@
 #include <list>
 #include <string>
 
+#include "envoy/api/api.h"
 #include "envoy/event/dispatcher.h"
 #include "envoy/filesystem/watcher.h"
 
@@ -20,11 +21,11 @@ namespace Filesystem {
  */
 class WatcherImpl : public Watcher, Logger::Loggable<Logger::Id::file> {
 public:
-  WatcherImpl(Event::Dispatcher& dispatcher);
+  WatcherImpl(Event::Dispatcher& dispatcher, Api::Api& api);
   ~WatcherImpl();
 
   // Filesystem::Watcher
-  void addWatch(const std::string& path, uint32_t events, OnChangedCb cb) override;
+  void addWatch(absl::string_view path, uint32_t events, OnChangedCb cb) override;
 
 private:
   struct FileWatch : LinkedObject<FileWatch> {
@@ -40,10 +41,11 @@ class WatcherImpl : public Watcher, Logger::Loggable<Logger::Id::file> {
   using FileWatchPtr = std::shared_ptr<FileWatch>;
 
   void onKqueueEvent();
-  FileWatchPtr addWatch(const std::string& path, uint32_t events, Watcher::OnChangedCb cb,
+  FileWatchPtr addWatch(absl::string_view path, uint32_t events, Watcher::OnChangedCb cb,
                         bool pathMustExist);
   void removeWatch(FileWatchPtr& watch);
 
+  Api::Api& api_;
   int queue_;
   std::unordered_map<int, FileWatchPtr> watches_;
   Event::FileEventPtr kqueue_event_;

diff --git a/source/common/filesystem/posix/directory_iterator_impl.cc b/source/common/filesystem/posix/directory_iterator_impl.cc
@@ -55,7 +55,16 @@ FileType DirectoryIteratorImpl::fileType(const std::string& full_path) const {
 
   const Api::SysCallIntResult result = os_sys_calls_.stat(full_path.c_str(), &stat_buf);
   if (result.rc_ != 0) {
-    throw EnvoyException(fmt::format("unable to stat file: '{}'", full_path));
+    if (errno == ENOENT) {
+      // Special case. This directory entity is likely to be a symlink,
+      // but the reference is broken as the target could not be stat()'ed.
+      // If we confirm this with an lstat, treat this file entity as
+      // a regular file, which may be unlink()'ed.
+      if (::lstat(full_path.c_str(), &stat_buf) == 0 && S_ISLNK(stat_buf.st_mode)) {
+        return FileType::Regular;
+      }
+    }
+    throw EnvoyException(fmt::format("unable to stat file: '{}' ({})", full_path, errno));
   }
 
   if (S_ISDIR(stat_buf.st_mode)) {

diff --git a/source/common/filesystem/posix/filesystem_impl.cc b/source/common/filesystem/posix/filesystem_impl.cc
@@ -108,6 +108,19 @@ std::string InstanceImplPosix::fileReadToEnd(const std::string& path) {
   return file_string.str();
 }
 
+PathSplitResult InstanceImplPosix::splitPathFromFilename(absl::string_view path) {
+  size_t last_slash = path.rfind('/');
+  if (last_slash == std::string::npos) {
+    throw EnvoyException(fmt::format("invalid file path {}", path));
+  }
+  absl::string_view name = path.substr(last_slash + 1);
+  // truncate all trailing slashes, except root slash
+  if (last_slash == 0) {
+    ++last_slash;
+  }
+  return {path.substr(0, last_slash), name};
+}
+
 bool InstanceImplPosix::illegalPath(const std::string& path) {
   // Special case, allow /dev/fd/* access here so that config can be passed in a
   // file descriptor from a bootstrap script via exec. The reason we do this
@@ -141,7 +154,6 @@ bool InstanceImplPosix::illegalPath(const std::string& path) {
 }
 
 Api::SysCallStringResult InstanceImplPosix::canonicalPath(const std::string& path) {
-  // TODO(htuch): When we are using C++17, switch to std::filesystem::canonical.
   char* resolved_path = ::realpath(path.c_str(), nullptr);
   if (resolved_path == nullptr) {
     return {std::string(), errno};

diff --git a/source/common/filesystem/posix/filesystem_impl.h b/source/common/filesystem/posix/filesystem_impl.h
@@ -39,6 +39,7 @@ class InstanceImplPosix : public Instance {
   bool directoryExists(const std::string& path) override;
   ssize_t fileSize(const std::string& path) override;
   std::string fileReadToEnd(const std::string& path) override;
+  PathSplitResult splitPathFromFilename(absl::string_view path) override;
   bool illegalPath(const std::string& path) override;
 
 private: