Add createPath to Filesystem abstraction. (envoyproxy#27052)

Add createPath to Filesystem abstraction Additional Description: createPath support is a prerequisite of file_system_http_cache's create_cache_path support. This is a revert of a revert. Now that mobile CI will run as part of the change, this should be safer to land when it passes than the two previous attempts. The revert is the first commit in this PR, so the changes that are new/fixes can be easily reviewed separately. Compared to the previous commit that broke mobile CI, this change performs posix file system operations with C style operating system calls, thereby avoiding the prior problem of C++17 filesystem functions failing to link. Signed-off-by: Raven Black <[email protected]>
pchaseh · May 1, 2023 · ecf3720 · ecf3720
1 parent 27be56a
commit ecf3720
Show file tree

Hide file tree

Showing 11 changed files with 107 additions and 2 deletions.
diff --git a/envoy/filesystem/filesystem.h b/envoy/filesystem/filesystem.h
@@ -174,6 +174,14 @@ class Instance {
    */
   virtual Api::IoCallResult<FileInfo> stat(absl::string_view path) PURE;
 
+  /**
+   * Attempts to create the given path, recursively if necessary.
+   * @return bool true if one or more directories was created and the path exists,
+   *         false if the path already existed, an error status if the path does
+   *         not exist after the call.
+   */
+  virtual Api::IoCallBoolResult createPath(absl::string_view path) PURE;
+
   /**
    * @return bool whether a directory exists on disk and can be opened for read.
    */

diff --git a/mobile/test/common/integration/base_client_integration_test.cc b/mobile/test/common/integration/base_client_integration_test.cc
@@ -206,7 +206,7 @@ void BaseClientIntegrationTest::createEnvoy() {
         << "Bootstrap config should not have `admin` configured in Envoy Mobile";
     builder_.setOverrideConfig(MessageUtil::getYamlStringFromMessage(config_helper_.bootstrap()));
   } else {
-    ENVOY_LOG_MISC(warn, "Using builder config and ignoring config modifiers.");
+    ENVOY_LOG_MISC(warn, "Using builder config and ignoring config modifiers");
   }
 
   absl::Notification engine_running;

diff --git a/source/common/filesystem/posix/filesystem_impl.cc b/source/common/filesystem/posix/filesystem_impl.cc
@@ -4,6 +4,7 @@
 #include <unistd.h>
 
 #include <cstdlib>
+#include <filesystem>
 #include <fstream>
 #include <iostream>
 #include <memory>
@@ -184,6 +185,37 @@ Api::IoCallResult<FileInfo> InstanceImplPosix::stat(absl::string_view path) {
   return resultSuccess(infoFromStat(path, s));
 }
 
+Api::IoCallBoolResult InstanceImplPosix::createPath(absl::string_view path) {
+  // Ideally we could just use std::filesystem::create_directories for this,
+  // identical to ../win32/filesystem_impl.cc, but the OS version used in mobile
+  // CI doesn't support it, so we have to do recursive path creation manually.
+  while (!path.empty() && path.back() == '/') {
+    path.remove_suffix(1);
+  }
+  if (directoryExists(std::string{path})) {
+    return resultSuccess(false);
+  }
+  absl::string_view subpath = path;
+  do {
+    size_t slashpos = subpath.find_last_of('/');
+    if (slashpos == absl::string_view::npos) {
+      return resultFailure(false, ENOENT);
+    }
+    subpath = subpath.substr(0, slashpos);
+  } while (!directoryExists(std::string{subpath}));
+  // We're now at the most-nested directory that already exists.
+  // Time to create paths recursively.
+  while (subpath != path) {
+    size_t slashpos = path.find_first_of('/', subpath.size() + 2);
+    subpath = (slashpos == absl::string_view::npos) ? path : path.substr(0, slashpos);
+    std::string dir_to_create{subpath};
+    if (mkdir(dir_to_create.c_str(), 0777)) {
+      return resultFailure(false, errno);
+    }
+  }
+  return resultSuccess(true);
+}
+
 FileImplPosix::FlagsAndMode FileImplPosix::translateFlag(FlagSet in) {
   int out = 0;
   mode_t mode = 0;

diff --git a/source/common/filesystem/posix/filesystem_impl.h b/source/common/filesystem/posix/filesystem_impl.h
@@ -62,6 +62,7 @@ class InstanceImplPosix : public Instance {
   PathSplitResult splitPathFromFilename(absl::string_view path) override;
   bool illegalPath(const std::string& path) override;
   Api::IoCallResult<FileInfo> stat(absl::string_view path) override;
+  Api::IoCallBoolResult createPath(absl::string_view path) override;
 
 private:
   Api::SysCallStringResult canonicalPath(const std::string& path);

diff --git a/source/common/filesystem/win32/filesystem_impl.cc b/source/common/filesystem/win32/filesystem_impl.cc
@@ -1,6 +1,7 @@
 #include <fcntl.h>
 
 #include <chrono>
+#include <filesystem>
 #include <fstream>
 #include <iostream>
 #include <sstream>
@@ -186,6 +187,15 @@ Api::IoCallResult<FileInfo> InstanceImplWin32::stat(absl::string_view path) {
   return resultSuccess(fileInfoFromAttributeData(full_path, data));
 }
 
+Api::IoCallBoolResult InstanceImplWin32::createPath(absl::string_view path) {
+  std::error_code ec;
+  while (!path.empty() && path.back() == '/') {
+    path.remove_suffix(1);
+  }
+  bool result = std::filesystem::create_directories(std::string{path}, ec);
+  return ec ? resultFailure(false, ec.value()) : resultSuccess(result);
+}
+
 FileImplWin32::FlagsAndMode FileImplWin32::translateFlag(FlagSet in) {
   DWORD access = 0;
   DWORD creation = OPEN_EXISTING;

diff --git a/source/common/filesystem/win32/filesystem_impl.h b/source/common/filesystem/win32/filesystem_impl.h
@@ -96,6 +96,7 @@ class InstanceImplWin32 : public Instance {
   PathSplitResult splitPathFromFilename(absl::string_view path) override;
   bool illegalPath(const std::string& path) override;
   Api::IoCallResult<FileInfo> stat(absl::string_view path) override;
+  Api::IoCallBoolResult createPath(absl::string_view path) override;
 };
 
 using FileImpl = FileImplWin32;

diff --git a/test/common/filesystem/filesystem_impl_test.cc b/test/common/filesystem/filesystem_impl_test.cc
@@ -361,6 +361,51 @@ TEST_F(FileSystemImplTest, StatOnDirectoryReturnsDirectoryType) {
   EXPECT_EQ(info_result.return_value_.file_type_, FileType::Directory);
 }
 
+TEST_F(FileSystemImplTest, CreatePathCreatesDirectoryAndReturnsSuccessForExistingDirectory) {
+  const std::string new_dir_path = TestEnvironment::temporaryPath("envoy_test_dir");
+  Api::IoCallBoolResult result = file_system_.createPath(new_dir_path);
+  Cleanup cleanup{[new_dir_path]() { TestEnvironment::removePath(new_dir_path); }};
+  EXPECT_TRUE(result.return_value_);
+  EXPECT_THAT(result.err_, ::testing::IsNull()) << result.err_->getErrorDetails();
+  // A bit awkwardly using file_system_.stat to test that the directory exists, because
+  // otherwise we might have to do windows/linux conditional code for this.
+  const Api::IoCallResult<FileInfo> info_result = file_system_.stat(new_dir_path);
+  EXPECT_THAT(info_result.err_, ::testing::IsNull()) << info_result.err_->getErrorDetails();
+  EXPECT_EQ(info_result.return_value_.name_, "envoy_test_dir");
+  EXPECT_EQ(info_result.return_value_.file_type_, FileType::Directory);
+  // Ensure it returns true if we 'create' an existing path too.
+  result = file_system_.createPath(new_dir_path);
+  EXPECT_FALSE(result.return_value_);
+  EXPECT_THAT(result.err_, ::testing::IsNull()) << result.err_->getErrorDetails();
+}
+
+TEST_F(FileSystemImplTest, CreatePathCreatesDirectoryGivenTrailingSlash) {
+  const std::string new_dir_path = TestEnvironment::temporaryPath("envoy_test_dir/");
+  const Api::IoCallBoolResult result = file_system_.createPath(new_dir_path);
+  Cleanup cleanup{[new_dir_path]() {
+    TestEnvironment::removePath(new_dir_path.substr(0, new_dir_path.size() - 1));
+  }};
+  EXPECT_TRUE(result.return_value_);
+  EXPECT_THAT(result.err_, ::testing::IsNull()) << result.err_->getErrorDetails();
+  // A bit awkwardly using file_system_.stat to test that the directory exists, because
+  // otherwise we might have to do windows/linux conditional code for this.
+  const Api::IoCallResult<FileInfo> info_result =
+      file_system_.stat(absl::string_view{new_dir_path}.substr(0, new_dir_path.size() - 1));
+  EXPECT_THAT(info_result.err_, ::testing::IsNull()) << info_result.err_->getErrorDetails();
+  EXPECT_EQ(info_result.return_value_.name_, "envoy_test_dir");
+  EXPECT_EQ(info_result.return_value_.file_type_, FileType::Directory);
+}
+
+TEST_F(FileSystemImplTest, CreatePathReturnsErrorOnNoPermissionToWriteDir) {
+#ifdef WIN32
+  GTEST_SKIP() << "It's hard to not have permission to create a directory on Windows";
+#endif
+  const std::string new_dir_path = "/should_fail_to_create_directory_in_root/x/y";
+  const Api::IoCallBoolResult result = file_system_.createPath(new_dir_path);
+  EXPECT_FALSE(result.return_value_);
+  EXPECT_THAT(result.err_, testing::Not(testing::IsNull()));
+}
+
 TEST_F(FileSystemImplTest, StatOnFileOpenOrClosedMeasuresTheExpectedValues) {
   const std::string file_path =
       TestEnvironment::writeStringToFileForTest("test_envoy", "0123456789");

diff --git a/test/mocks/filesystem/mocks.h b/test/mocks/filesystem/mocks.h
@@ -67,6 +67,7 @@ class MockInstance : public Instance {
   MOCK_METHOD(PathSplitResult, splitPathFromFilename, (absl::string_view));
   MOCK_METHOD(bool, illegalPath, (const std::string&));
   MOCK_METHOD(Api::IoCallResult<FileInfo>, stat, (absl::string_view));
+  MOCK_METHOD(Api::IoCallBoolResult, createPath, (absl::string_view));
 };
 
 class MockWatcher : public Watcher {

diff --git a/test/per_file_coverage.sh b/test/per_file_coverage.sh
@@ -10,7 +10,7 @@ declare -a KNOWN_LOW_COVERAGE=(
 "source/common/config:96.2"
 "source/common/crypto:88.1"
 "source/common/event:95.1" # Emulated edge events guards don't report LCOV
-"source/common/filesystem/posix:96.5" # FileReadToEndNotReadable keeps failing
+"source/common/filesystem/posix:96.2" # FileReadToEndNotReadable fails in some env; createPath can't test all failure branches.
 "source/common/http:96.3"
 "source/common/http/http2:95.0"
 "source/common/json:93.4"

diff --git a/test/test_common/file_system_for_test.cc b/test/test_common/file_system_for_test.cc
@@ -121,6 +121,11 @@ Api::IoCallResult<FileInfo> MemfileInstanceImpl::stat(absl::string_view path) {
   return file_system_->stat(path);
 }
 
+Api::IoCallBoolResult MemfileInstanceImpl::createPath(absl::string_view) {
+  // Creating an imaginary path is always successful.
+  return resultSuccess(true);
+}
+
 MemfileInstanceImpl::MemfileInstanceImpl()
     : file_system_{new InstanceImpl()}, use_memfiles_(false) {}
 

diff --git a/test/test_common/file_system_for_test.h b/test/test_common/file_system_for_test.h
@@ -42,6 +42,8 @@ class MemfileInstanceImpl : public Instance {
 
   Api::IoCallResult<FileInfo> stat(absl::string_view path) override;
 
+  Api::IoCallBoolResult createPath(absl::string_view path) override;
+
 private:
   friend class ScopedUseMemfiles;