envoy: moving access log manager to statusor (envoyproxy#32202)

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]>
dynatrace-oss-contrib · Feb 9, 2024 · c50ebb7 · c50ebb7
1 parent 7e770f5
commit c50ebb7
Show file tree

Hide file tree

Showing 15 changed files with 117 additions and 82 deletions.
diff --git a/contrib/generic_proxy/filters/network/source/file_access_log.h b/contrib/generic_proxy/filters/network/source/file_access_log.h
@@ -20,7 +20,7 @@ template <class Context> class FileAccessLogBase : public AccessLog::InstanceBas
                     Formatter::FormatterBasePtr<Context>&& formatter,
                     AccessLog::AccessLogManager& log_manager)
       : filter_(std::move(filter)), formatter_(std::move(formatter)) {
-    log_file_ = log_manager.createAccessLog(access_log_file_info);
+    log_file_ = log_manager.createAccessLog(access_log_file_info).value();
   }
 
   void log(const Context& context, const StreamInfo::StreamInfo& stream_info) override {

diff --git a/envoy/access_log/access_log.h b/envoy/access_log/access_log.h
@@ -49,9 +49,9 @@ class AccessLogManager {
   /**
    * Create a new access log file managed by the access log manager.
    * @param file_info specifies the file to create/open.
-   * @return the opened file.
+   * @return the opened file or an error status.
    */
-  virtual AccessLogFileSharedPtr
+  virtual absl::StatusOr<AccessLogFileSharedPtr>
   createAccessLog(const Envoy::Filesystem::FilePathAndType& file_info) PURE;
 };
 

diff --git a/source/common/access_log/access_log_manager_impl.cc b/source/common/access_log/access_log_manager_impl.cc
@@ -12,6 +12,11 @@
 
 namespace Envoy {
 namespace AccessLog {
+namespace {
+static constexpr Filesystem::FlagSet default_flags{1 << Filesystem::File::Operation::Write |
+                                                   1 << Filesystem::File::Operation::Create |
+                                                   1 << Filesystem::File::Operation::Append};
+} // namespace
 
 AccessLogManagerImpl::~AccessLogManagerImpl() {
   for (auto& [log_key, log_file_ptr] : access_logs_) {
@@ -27,13 +32,20 @@ void AccessLogManagerImpl::reopen() {
   }
 }
 
-AccessLogFileSharedPtr
+absl::StatusOr<AccessLogFileSharedPtr>
 AccessLogManagerImpl::createAccessLog(const Filesystem::FilePathAndType& file_info) {
   auto file = api_.fileSystem().createFile(file_info);
   std::string file_name = file->path();
   if (access_logs_.count(file_name)) {
     return access_logs_[file_name];
   }
+
+  Api::IoCallBoolResult open_result = file->open(default_flags);
+  if (!open_result.return_value_) {
+    return absl::InvalidArgumentError(fmt::format("unable to open file '{}': {}", file_name,
+                                                  open_result.err_->getErrorDetails()));
+  }
+
   access_logs_[file_name] =
       std::make_shared<AccessLogFileImpl>(std::move(file), dispatcher_, lock_, file_stats_,
                                           file_flush_interval_msec_, api_.threadFactory());
@@ -52,24 +64,6 @@ AccessLogFileImpl::AccessLogFileImpl(Filesystem::FilePtr&& file, Event::Dispatch
       })),
       thread_factory_(thread_factory), flush_interval_msec_(flush_interval_msec), stats_(stats) {
   flush_timer_->enableTimer(flush_interval_msec_);
-  auto open_result = open();
-  if (!open_result.return_value_) {
-    throwEnvoyExceptionOrPanic(fmt::format("unable to open file '{}': {}", file_->path(),
-                                           open_result.err_->getErrorDetails()));
-  }
-}
-
-Filesystem::FlagSet AccessLogFileImpl::defaultFlags() {
-  static constexpr Filesystem::FlagSet default_flags{1 << Filesystem::File::Operation::Write |
-                                                     1 << Filesystem::File::Operation::Create |
-                                                     1 << Filesystem::File::Operation::Append};
-
-  return default_flags;
-}
-
-Api::IoCallBoolResult AccessLogFileImpl::open() {
-  Api::IoCallBoolResult result = file_->open(defaultFlags());
-  return result;
 }
 
 void AccessLogFileImpl::reopen() {
@@ -173,7 +167,7 @@ void AccessLogFileImpl::flushThreadFunc() {
         ASSERT(result.return_value_, fmt::format("unable to close file '{}': {}", file_->path(),
                                                  result.err_->getErrorDetails()));
       }
-      const Api::IoCallBoolResult open_result = open();
+      const Api::IoCallBoolResult open_result = file_->open(default_flags);
       if (!open_result.return_value_) {
         stats_.reopen_failed_.inc();
       } else {

diff --git a/source/common/access_log/access_log_manager_impl.h b/source/common/access_log/access_log_manager_impl.h
@@ -44,7 +44,8 @@ class AccessLogManagerImpl : public AccessLogManager, Logger::Loggable<Logger::I
 
   // AccessLog::AccessLogManager
   void reopen() override;
-  AccessLogFileSharedPtr createAccessLog(const Filesystem::FilePathAndType& file_info) override;
+  absl::StatusOr<AccessLogFileSharedPtr>
+  createAccessLog(const Filesystem::FilePathAndType& file_info) override;
 
 private:
   const std::chrono::milliseconds file_flush_interval_msec_;
@@ -84,12 +85,8 @@ class AccessLogFileImpl : public AccessLogFile {
 private:
   void doWrite(Buffer::Instance& buffer);
   void flushThreadFunc();
-  Api::IoCallBoolResult open();
   void createFlushStructures();
 
-  // return default flags set which used by open
-  static Filesystem::FlagSet defaultFlags();
-
   // Minimum size before the flush thread will be told to flush.
   static const uint64_t MIN_FLUSH_SIZE = 1024 * 64;
 

diff --git a/source/common/common/logger_delegates.cc b/source/common/common/logger_delegates.cc
@@ -12,8 +12,11 @@ namespace Logger {
 FileSinkDelegate::FileSinkDelegate(const std::string& log_path,
                                    AccessLog::AccessLogManager& log_manager,
                                    DelegatingLogSinkSharedPtr log_sink)
-    : SinkDelegate(log_sink), log_file_(log_manager.createAccessLog(Filesystem::FilePathAndType{
-                                  Filesystem::DestinationType::File, log_path})) {
+    : SinkDelegate(log_sink) {
+  auto file_or_error = log_manager.createAccessLog(
+      Filesystem::FilePathAndType{Filesystem::DestinationType::File, log_path});
+  THROW_IF_STATUS_NOT_OK(file_or_error, throw);
+  log_file_ = file_or_error.value();
   setDelegate();
 }
 

diff --git a/source/common/upstream/health_checker_event_logger.h b/source/common/upstream/health_checker_event_logger.h
@@ -31,9 +31,11 @@ class HealthCheckEventLoggerImpl : public HealthCheckEventLogger {
     // TODO(botengyao): Remove the file_ creation here into the file based health check
     // event sink. In this way you can remove the file_ based code from the createHealthCheckEvent
     if (!health_check_config.event_log_path().empty() /* deprecated */) {
-      file_ = context.serverFactoryContext().accessLogManager().createAccessLog(
+      auto file_or_error = context.serverFactoryContext().accessLogManager().createAccessLog(
           Filesystem::FilePathAndType{Filesystem::DestinationType::File,
                                       health_check_config.event_log_path()});
+      THROW_IF_STATUS_NOT_OK(file_or_error, throw);
+      file_ = file_or_error.value();
     }
     for (const auto& config : health_check_config.event_logger()) {
       auto& factory = Config::Utility::getAndCheckFactory<HealthCheckEventSinkFactory>(config);

diff --git a/source/common/upstream/outlier_detection_impl.h b/source/common/upstream/outlier_detection_impl.h
@@ -493,9 +493,12 @@ class EventLoggerImpl : public EventLogger {
 public:
   EventLoggerImpl(AccessLog::AccessLogManager& log_manager, const std::string& file_name,
                   TimeSource& time_source)
-      : file_(log_manager.createAccessLog(
-            Filesystem::FilePathAndType{Filesystem::DestinationType::File, file_name})),
-        time_source_(time_source) {}
+      : time_source_(time_source) {
+    auto file_or_error = log_manager.createAccessLog(
+        Filesystem::FilePathAndType{Filesystem::DestinationType::File, file_name});
+    THROW_IF_STATUS_NOT_OK(file_or_error, throw);
+    file_ = file_or_error.value();
+  }
 
   // Upstream::Outlier::EventLogger
   void logEject(const HostDescriptionConstSharedPtr& host, Detector& detector,

diff --git a/source/extensions/access_loggers/common/file_access_log_impl.cc b/source/extensions/access_loggers/common/file_access_log_impl.cc
@@ -9,7 +9,9 @@ FileAccessLog::FileAccessLog(const Filesystem::FilePathAndType& access_log_file_
                              AccessLog::FilterPtr&& filter, Formatter::FormatterPtr&& formatter,
                              AccessLog::AccessLogManager& log_manager)
     : ImplBase(std::move(filter)), formatter_(std::move(formatter)) {
-  log_file_ = log_manager.createAccessLog(access_log_file_info);
+  auto file_or_error = log_manager.createAccessLog(access_log_file_info);
+  THROW_IF_STATUS_NOT_OK(file_or_error, throw);
+  log_file_ = file_or_error.value();
 }
 
 void FileAccessLog::emitLog(const Formatter::HttpFormatterContext& context,

diff --git a/source/extensions/filters/network/mongo_proxy/proxy.cc b/source/extensions/filters/network/mongo_proxy/proxy.cc
@@ -37,8 +37,10 @@ using DynamicMetadataKeysSingleton = ConstSingleton<DynamicMetadataKeys>;
 AccessLog::AccessLog(const std::string& file_name, Envoy::AccessLog::AccessLogManager& log_manager,
                      TimeSource& time_source)
     : time_source_(time_source) {
-  file_ = log_manager.createAccessLog(
+  auto file_or_error = log_manager.createAccessLog(
       Filesystem::FilePathAndType{Filesystem::DestinationType::File, file_name});
+  THROW_IF_STATUS_NOT_OK(file_or_error, throw);
+  file_ = file_or_error.value();
 }
 
 void AccessLog::logMessage(const Message& message, bool full,

diff --git a/source/extensions/health_check/event_sinks/file/file_sink_impl.h b/source/extensions/health_check/event_sinks/file/file_sink_impl.h
@@ -13,9 +13,12 @@ class HealthCheckEventFileSink : public HealthCheckEventSink {
   explicit HealthCheckEventFileSink(
       const envoy::extensions::health_check::event_sinks::file::v3::HealthCheckEventFileSink&
           config,
-      AccessLog::AccessLogManager& log_manager)
-      : file_(log_manager.createAccessLog(Filesystem::FilePathAndType{
-            Filesystem::DestinationType::File, config.event_log_path()})) {}
+      AccessLog::AccessLogManager& log_manager) {
+    auto file_or_error = log_manager.createAccessLog(
+        Filesystem::FilePathAndType{Filesystem::DestinationType::File, config.event_log_path()});
+    THROW_IF_STATUS_NOT_OK(file_or_error, throw);
+    file_ = file_or_error.value();
+  }
 
   void log(envoy::data::core::v3::HealthCheckEvent event) override;
 

diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc
@@ -379,8 +379,10 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c
 
   if (!config.tlsKeyLogPath().empty()) {
     ENVOY_LOG(debug, "Enable tls key log");
-    tls_keylog_file_ = config.accessLogManager().createAccessLog(
+    auto file_or_error = config.accessLogManager().createAccessLog(
         Filesystem::FilePathAndType{Filesystem::DestinationType::File, config.tlsKeyLogPath()});
+    THROW_IF_STATUS_NOT_OK(file_or_error, throw);
+    tls_keylog_file_ = file_or_error.value();
     for (auto& context : tls_contexts_) {
       SSL_CTX* ctx = context.ssl_ctx_.get();
       ASSERT(ctx != nullptr);

diff --git a/test/common/access_log/access_log_impl_test.cc b/test/common/access_log/access_log_impl_test.cc
@@ -1131,13 +1131,13 @@ name: accesslog
   ON_CALL(context_.server_factory_context_, accessLogManager())
       .WillByDefault(ReturnRef(log_manager_));
   EXPECT_CALL(log_manager_, createAccessLog(_))
-      .WillOnce(Invoke(
-          [this](const Envoy::Filesystem::FilePathAndType& file_info) -> AccessLogFileSharedPtr {
-            EXPECT_EQ(file_info.path_, "");
-            EXPECT_EQ(file_info.file_type_, Filesystem::DestinationType::Stdout);
+      .WillOnce(Invoke([this](const Envoy::Filesystem::FilePathAndType& file_info)
+                           -> absl::StatusOr<AccessLogFileSharedPtr> {
+        EXPECT_EQ(file_info.path_, "");
+        EXPECT_EQ(file_info.file_type_, Filesystem::DestinationType::Stdout);
 
-            return file_;
-          }));
+        return file_;
+      }));
   EXPECT_NO_THROW(AccessLogFactory::fromProto(parseAccessLogFromV3Yaml(yaml), context_));
 }
 
@@ -1152,12 +1152,12 @@ name: accesslog
   ON_CALL(context_.server_factory_context_, accessLogManager())
       .WillByDefault(ReturnRef(log_manager_));
   EXPECT_CALL(log_manager_, createAccessLog(_))
-      .WillOnce(Invoke(
-          [this](const Envoy::Filesystem::FilePathAndType& file_info) -> AccessLogFileSharedPtr {
-            EXPECT_EQ(file_info.path_, "");
-            EXPECT_EQ(file_info.file_type_, Filesystem::DestinationType::Stderr);
-            return file_;
-          }));
+      .WillOnce(Invoke([this](const Envoy::Filesystem::FilePathAndType& file_info)
+                           -> absl::StatusOr<AccessLogFileSharedPtr> {
+        EXPECT_EQ(file_info.path_, "");
+        EXPECT_EQ(file_info.file_type_, Filesystem::DestinationType::Stderr);
+        return file_;
+      }));
   InstanceSharedPtr log = AccessLogFactory::fromProto(parseAccessLogFromV3Yaml(yaml), context_);
 }