filesystem: making fileReadToEnd take absl::status

Signed-off-by: Alyssa Wilk <[email protected]>

contrib/client_ssl_auth/filters/network/test/client_ssl_auth_test.cc

@@ -176,8 +176,11 @@ TEST_F(ClientSslAuthFilterTest, Ssl) {
   EXPECT_CALL(*interval_timer_, enableTimer(_, _));
   Http::ResponseMessagePtr message(new Http::ResponseMessageImpl(
       Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl{{":status", "200"}}}));
-  message->body().add(api_->fileSystem().fileReadToEnd(TestEnvironment::runfilesPath(
-      "contrib/client_ssl_auth/filters/network/test/test_data/vpn_response_1.json")));
+  message->body().add(
+      api_->fileSystem()
+          .fileReadToEnd(TestEnvironment::runfilesPath(
+              "contrib/client_ssl_auth/filters/network/test/test_data/vpn_response_1.json"))
+          .value());
   callbacks_->onSuccess(request_, std::move(message));
   EXPECT_EQ(1U,
             stats_store_

envoy/filesystem/BUILD

@@ -12,10 +12,10 @@ envoy_cc_library(
     name = "filesystem_interface",
     hdrs = ["filesystem.h"],
     deps = [
-         "@com_google_absl//absl/status:statusor",
         "//envoy/api:io_error_interface",
         "//envoy/api:os_sys_calls_interface",
         "//envoy/common:time_interface",
+        "@com_google_absl//absl/status:statusor",
     ],
 )
 

envoy/filesystem/filesystem.h

@@ -203,10 +203,10 @@ class Instance {
 
   /**
    * @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
+   * @return PathSplitResult containing the parent directory of the input path and the file name or
+   * an error status.
    */
-  virtual PathSplitResult splitPathFromFilename(absl::string_view path) PURE;
+  virtual absl::StatusOr<PathSplitResult> splitPathFromFilename(absl::string_view path) PURE;
 
   /**
    * Determine if the path is on a list of paths Envoy will refuse to access. This

source/common/config/datasource.cc

@@ -18,9 +18,10 @@ static constexpr uint32_t RetryCount = 1;
 std::string read(const envoy::config::core::v3::DataSource& source, bool allow_empty,
                  Api::Api& api) {
   std::string data;
+  absl::StatusOr<std::string> file_or_error;
   switch (source.specifier_case()) {
   case envoy::config::core::v3::DataSource::SpecifierCase::kFilename:
-    auto file_or_error = api.fileSystem().fileReadToEnd(path);
+    file_or_error = api.fileSystem().fileReadToEnd(source.filename());
     THROW_IF_STATUS_NOT_OK(file_or_error, throw);
     data = file_or_error.value();
     break;

source/common/filesystem/inotify/watcher_impl.cc

@@ -35,7 +35,9 @@ WatcherImpl::~WatcherImpl() { close(inotify_fd_); }
 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.
-  const PathSplitResult result = file_system_.splitPathFromFilename(path);
+  auto result_or_error = file_system_.splitPathFromFilename(path);
+  THROW_IF_STATUS_NOT_OK(result_or_error, throw);
+  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);

source/common/filesystem/kqueue/watcher_impl.cc

@@ -49,7 +49,9 @@ WatcherImpl::FileWatchPtr WatcherImpl::addWatch(absl::string_view path, uint32_t
       return nullptr;
     }
 
-    watch_fd = open(std::string(file_system_.splitPathFromFilename(path).directory_).c_str(), 0);
+    const auto result_or_error = file_system_.splitPathFromFilename(path);
+    THROW_IF_STATUS_NOT_OK(result_or_error, throw);
+    watch_fd = open(std::string(result_or_error.value().directory_).c_str(), 0);
     if (watch_fd == -1) {
       return nullptr;
     }

source/common/filesystem/posix/filesystem_impl.cc

@@ -137,9 +137,11 @@ static constexpr absl::optional<SystemTime> systemTimeFromTimespec(const struct
   return timespecToChrono(t);
 }
 
-static FileInfo infoFromStat(absl::string_view path, const struct stat& s, FileType type) {
-  return {
-      std::string{InstanceImplPosix().splitPathFromFilename(path).file_},
+static Api::IoCallResult<FileInfo> infoFromStat(absl::string_view path, const struct stat& s,
+                                                FileType type) {
+  auto result_or_error = InstanceImplPosix().splitPathFromFilename(path);
+  FileInfo ret = {
+      result_or_error.status().ok() ? std::string{result_or_error.value().file_} : "",
       s.st_size,
       type,
 #ifdef _DARWIN_FEATURE_64_BIT_INODE
@@ -152,9 +154,13 @@ static FileInfo infoFromStat(absl::string_view path, const struct stat& s, FileT
       systemTimeFromTimespec(s.st_mtim),
 #endif
   };
+  if (result_or_error.status().ok()) {
+    return resultSuccess(ret);
+  }
+  return resultFailure(ret, EINVAL);
 }
 
-static FileInfo infoFromStat(absl::string_view path, const struct stat& s) {
+static Api::IoCallResult<FileInfo> infoFromStat(absl::string_view path, const struct stat& s) {
   return infoFromStat(path, s, typeFromStat(s));
 }
 
@@ -164,7 +170,7 @@ Api::IoCallResult<FileInfo> FileImplPosix::info() {
   if (::fstat(fd_, &s) != 0) {
     return resultFailure<FileInfo>({}, errno);
   }
-  return resultSuccess(infoFromStat(path(), s));
+  return infoFromStat(path(), s);
 }
 
 Api::IoCallResult<FileInfo> InstanceImplPosix::stat(absl::string_view path) {
@@ -177,12 +183,12 @@ Api::IoCallResult<FileInfo> InstanceImplPosix::stat(absl::string_view path) {
         // but the reference is broken as the target could not be stat()'ed.
         // After confirming this with an lstat, treat this file entity as
         // a regular file, which may be unlink()'ed.
-        return resultSuccess(infoFromStat(path, s, FileType::Regular));
+        return infoFromStat(path, s, FileType::Regular);
       }
     }
     return resultFailure<FileInfo>({}, errno);
   }
-  return resultSuccess(infoFromStat(path, s));
+  return infoFromStat(path, s);
 }
 
 Api::IoCallBoolResult InstanceImplPosix::createPath(absl::string_view path) {
@@ -298,17 +304,17 @@ absl::StatusOr<std::string> InstanceImplPosix::fileReadToEnd(const std::string&
   return file_string.str();
 }
 
-PathSplitResult InstanceImplPosix::splitPathFromFilename(absl::string_view path) {
+absl::StatusOr<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));
+    return absl::InvalidArgumentError(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};
+  return PathSplitResult{path.substr(0, last_slash), name};
 }
 
 bool InstanceImplPosix::illegalPath(const std::string& path) {

source/common/filesystem/posix/filesystem_impl.h

@@ -59,7 +59,7 @@ class InstanceImplPosix : public Instance {
   bool directoryExists(const std::string& path) override;
   ssize_t fileSize(const std::string& path) override;
   absl::StatusOr<std::string> fileReadToEnd(const std::string& path) override;
-  PathSplitResult splitPathFromFilename(absl::string_view path) override;
+  absl::StatusOr<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;

source/common/filesystem/win32/filesystem_impl.cc

@@ -150,21 +150,26 @@ static FileType fileTypeFromAttributeData(const WIN32_FILE_ATTRIBUTE_DATA& data)
   return FileType::Regular;
 }
 
-static FileInfo fileInfoFromAttributeData(absl::string_view path,
-                                          const WIN32_FILE_ATTRIBUTE_DATA& data) {
+static Api::IoCallResult<FileInfo>
+fileInfoFromAttributeData(absl::string_view path, const WIN32_FILE_ATTRIBUTE_DATA& data) {
   absl::optional<uint64_t> sz;
   FileType type = fileTypeFromAttributeData(data);
   if (type == FileType::Regular) {
     sz = fileSizeFromAttributeData(data);
   }
-  return {
-      std::string{InstanceImplWin32().splitPathFromFilename(path).file_},
+  absl::StatusOr<PathSplitResult> result_or_error = InstanceImplWin32().splitPathFromFilename(path);
+  FileInfo ret{
+      result_or_error.ok() ? std::string{result_or_error.file_} : "",
       sz,
       type,
       systemTimeFromFileTime(data.ftCreationTime),
       systemTimeFromFileTime(data.ftLastAccessTime),
       systemTimeFromFileTime(data.ftLastWriteTime),
   };
+  if (result_or_error.status().ok()) {
+    return resultSuccess(ret);
+  }
+  return resultFailure(ret, ERROR_INVALID_DATA);
 }
 
 Api::IoCallResult<FileInfo> FileImplWin32::info() {
@@ -174,7 +179,7 @@ Api::IoCallResult<FileInfo> FileImplWin32::info() {
   if (!result) {
     return resultFailure<FileInfo>({}, ::GetLastError());
   }
-  return resultSuccess(fileInfoFromAttributeData(path(), data));
+  return fileInfoFromAttributeData(path(), data);
 }
 
 Api::IoCallResult<FileInfo> InstanceImplWin32::stat(absl::string_view path) {
@@ -184,7 +189,7 @@ Api::IoCallResult<FileInfo> InstanceImplWin32::stat(absl::string_view path) {
   if (!result) {
     return resultFailure<FileInfo>({}, ::GetLastError());
   }
-  return resultSuccess(fileInfoFromAttributeData(full_path, data));
+  return fileInfoFromAttributeData(full_path, data);
 }
 
 Api::IoCallBoolResult InstanceImplWin32::createPath(absl::string_view path) {
@@ -314,10 +319,10 @@ absl::StatusOr<std::string> InstanceImplWin32::fileReadToEnd(const std::string&
   return std::string(complete_buffer.begin(), complete_buffer.end());
 }
 
-PathSplitResult InstanceImplWin32::splitPathFromFilename(absl::string_view path) {
+absl::StatusOr<PathSplitResult> InstanceImplWin32::splitPathFromFilename(absl::string_view path) {
   size_t last_slash = path.find_last_of(":/\\");
   if (last_slash == std::string::npos) {
-    throw EnvoyException(fmt::format("invalid file path {}", path));
+    return absl::InvalidArgumentError(fmt::format("invalid file path {}", path));
   }
   absl::string_view name = path.substr(last_slash + 1);
   // Truncate all trailing slashes, but retain the entire

source/common/filesystem/win32/filesystem_impl.h

@@ -93,7 +93,7 @@ class InstanceImplWin32 : public Instance {
   bool directoryExists(const std::string& path) override;
   ssize_t fileSize(const std::string& path) override;
   absl::StatusOr<std::string> fileReadToEnd(const std::string& path) override;
-  PathSplitResult splitPathFromFilename(absl::string_view path) override;
+  absl::StatusOr<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;

source/common/filesystem/win32/watcher_impl.cc

@@ -55,7 +55,9 @@ void WatcherImpl::addWatch(absl::string_view path, uint32_t events, OnChangedCb
     return;
   }
 
-  const PathSplitResult result = file_system_.splitPathFromFilename(path);
+  const absl::StatusOr<PathSplitResult> result_or_error = file_system_.splitPathFromFilename(path);
+  THROW_IF_STATUS_NOT_OK(result_or_error, throw);
+  const PathSplitResult& result = result_or_error.value();
   // ReadDirectoryChangesW only has a Unicode version, so we need
   // to use wide strings here
   const std::wstring directory = wstring_converter_.from_bytes(std::string(result.directory_));

source/common/runtime/runtime_impl.cc

@@ -446,9 +446,9 @@ void DiskLayer::walkDirectory(const std::string& path, const std::string& prefix
 
       // Read the file and remove any comments. A comment is a line starting with a '#' character.
       // Comments are useful for placeholder files with no value.
-    auto file_or_error = api.fileSystem().fileReadToEnd(full_path);
-    THROW_IF_STATUS_NOT_OK(file_or_error, throw);
-      const std::string text_file{file_or_error.value};
+      auto file_or_error = api.fileSystem().fileReadToEnd(full_path);
+      THROW_IF_STATUS_NOT_OK(file_or_error, throw);
+      const std::string text_file{file_or_error.value()};
 
       const auto lines = StringUtil::splitToken(text_file, "\n");
       for (const auto& line : lines) {

source/common/secret/sds_api.cc

@@ -116,8 +116,9 @@ absl::Status SdsApi::onConfigUpdate(const std::vector<Config::DecodedResourceRef
         for (auto const& filename : files) {
           // Watch for directory instead of file. This allows users to do atomic renames
           // on directory level (e.g. Kubernetes secret update).
-          const auto result = api_.fileSystem().splitPathFromFilename(filename);
-          watcher_->addWatch(absl::StrCat(result.directory_, "/"),
+          const auto result_or_error = api_.fileSystem().splitPathFromFilename(filename);
+          THROW_IF_STATUS_NOT_OK(result_or_error, throw);
+          watcher_->addWatch(absl::StrCat(result_or_error.value().directory_, "/"),
                              Filesystem::Watcher::Events::MovedTo,
                              [this](uint32_t) { onWatchUpdate(); });
         }
@@ -167,7 +168,7 @@ SdsApi::SecretData SdsApi::secretData() { return secret_data_; }
 SdsApi::FileContentMap SdsApi::loadFiles() {
   FileContentMap files;
   for (auto const& filename : getDataSourceFilenames()) {
-    auto file_or_error = api.fileSystem().fileReadToEnd(path);
+    auto file_or_error = api_.fileSystem().fileReadToEnd(filename);
     THROW_IF_STATUS_NOT_OK(file_or_error, throw);
     files[filename] = file_or_error.value();
   }

source/extensions/compression/zstd/common/dictionary_manager.h

@@ -89,10 +89,8 @@ template <class T, size_t (*deleter)(T*), unsigned (*getDictId)(const T*)> class
                                    public ThreadLocal::ThreadLocalObject {};
 
   void onDictionaryUpdate(unsigned origin_id, const std::string& filename) {
-e auto file_or_error = api.fileSystem().fileReadToEnd(filename);
-  auto file_or_error = api.fileSystem().fileReadToEnd(filename);
-  THROW_IF_STATUS_NOT_OK(file_or_error, throw);
-  THROW_IF_STATUS_NOT_OK(file_or_error, throw);
+    auto file_or_error = api_.fileSystem().fileReadToEnd(filename);
+    THROW_IF_STATUS_NOT_OK(file_or_error, throw);
     const auto data = file_or_error.value();
     if (!data.empty()) {
       auto dictionary = DictionarySharedPtr(builder_(data.data(), data.length()));

source/extensions/filters/http/grpc_field_extraction/filter_config.cc

@@ -56,8 +56,8 @@ void FilterConfig::initDescriptorPool(Api::Api& api) {
 
   switch (descriptor_config.specifier_case()) {
   case envoy::config::core::v3::DataSource::SpecifierCase::kFilename: {
-    if (!descriptor_set.ParseFromString(
-            api.fileSystem().fileReadToEnd(descriptor_config.filename()))) {
+    auto file_or_error = api.fileSystem().fileReadToEnd(descriptor_config.filename());
+    if (!file_or_error.status().ok() || !descriptor_set.ParseFromString(file_or_error.value())) {
       throw Envoy::EnvoyException(fmt::format("unable to parse proto descriptor from file `{}`",
                                               descriptor_config.filename()));
     }

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc

@@ -121,13 +121,13 @@ JsonTranscoderConfig::JsonTranscoderConfig(
   switch (proto_config.descriptor_set_case()) {
   case envoy::extensions::filters::http::grpc_json_transcoder::v3::GrpcJsonTranscoder::
       DescriptorSetCase::kProtoDescriptor: {
-  auto file_or_error = api.fileSystem().fileReadToEnd(proto_config.proto_descriptor());
-  THROW_IF_STATUS_NOT_OK(file_or_error, throw);
+    auto file_or_error = api.fileSystem().fileReadToEnd(proto_config.proto_descriptor());
+    THROW_IF_STATUS_NOT_OK(file_or_error, throw);
     if (!descriptor_set.ParseFromString(file_or_error.value())) {
       throw EnvoyException("transcoding_filter: Unable to parse proto descriptor");
     }
     break;
-      }
+  }
   case envoy::extensions::filters::http::grpc_json_transcoder::v3::GrpcJsonTranscoder::
       DescriptorSetCase::kProtoDescriptorBin:
     if (!descriptor_set.ParseFromString(proto_config.proto_descriptor_bin())) {

source/extensions/key_value/file_based/config.cc

@@ -16,7 +16,7 @@ FileBasedKeyValueStore::FileBasedKeyValueStore(Event::Dispatcher& dispatcher,
     ENVOY_LOG(info, "File for key value store does not yet exist: {}", filename);
     return;
   }
-  auto file_or_error = api.fileSystem().fileReadToEnd(filename_);
+  auto file_or_error = file_system_.fileReadToEnd(filename_);
   THROW_IF_STATUS_NOT_OK(file_or_error, throw);
   const std::string contents = file_or_error.value();
   if (!parseContents(contents)) {

source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc

@@ -27,8 +27,8 @@ void InjectedResourceMonitor::updateResourceUsage(Server::ResourceUpdateCallback
   if (file_changed_) {
     file_changed_ = false;
     TRY_ASSERT_MAIN_THREAD {
-  auto file_or_error = api.fileSystem().fileReadToEnd(filename_);
-  THROW_IF_STATUS_NOT_OK(file_or_error, throw);
+      auto file_or_error = api_.fileSystem().fileReadToEnd(filename_);
+      THROW_IF_STATUS_NOT_OK(file_or_error, throw);
       const std::string contents = file_or_error.value();
       double pressure;
       if (absl::SimpleAtod(contents, &pressure)) {