Merge branch 'main' into metadata-matcher

Signed-off-by: Vikas Choudhary <[email protected]>

changelogs/current.yaml

@@ -100,6 +100,10 @@ new_features:
   change: |
     added %UPSTREAM_CLUSTER_RAW% access log formatter to log the original upstream cluster name, regardless of whether
     ``alt_stat_name`` is set.
+- area: formatter
+  change: |
+    Added full feature absl::FormatTime() support to the DateFormatter. This allows the timepoint formatters (like
+    ``%START_TIME%``) to use ``%E#S``, ``%E*S``, ``%E#f`` and ``%E*f`` to format the subsecond part of the timepoint.
 - area: sockets
   change: |
     Added socket ``type`` field for specifying a socket type to apply the socket option to under :ref:`SocketOption

source/common/common/BUILD

@@ -476,6 +476,7 @@ envoy_cc_library(
         "//envoy/common:interval_set_interface",
         "//envoy/common:time_interface",
         "//source/common/singleton:const_singleton",
+        "@com_googlesource_code_re2//:re2",
     ],
 )
 

source/common/common/utility.h

@@ -3,6 +3,7 @@
 #include <chrono>
 #include <cstdint>
 #include <ios>
+#include <limits>
 #include <set>
 #include <sstream>
 #include <string>
@@ -17,6 +18,7 @@
 
 #include "absl/container/flat_hash_map.h"
 #include "absl/container/flat_hash_set.h"
+#include "absl/container/node_hash_map.h"
 #include "absl/strings/string_view.h"
 
 namespace Envoy {
@@ -44,16 +46,17 @@ const std::string errorDetails(int error_code);
  */
 class DateFormatter {
 public:
-  DateFormatter(const std::string& format_string, bool local_time = false)
-      : raw_format_string_(format_string), local_time_(local_time) {
+  DateFormatter(absl::string_view format_string, bool local_time = false)
+      : raw_format_string_(format_string),
+        raw_format_hash_(CachedTimes::hasher{}(raw_format_string_)), local_time_(local_time) {
     parse(format_string);
   }
 
   /**
    * @return std::string representing the GMT/UTC time based on the input time,
    * or local zone time when local_time_ is true.
    */
-  std::string fromTime(const SystemTime& time) const;
+  std::string fromTime(SystemTime time) const;
 
   /**
    * @param time_source time keeping source.
@@ -67,48 +70,101 @@ class DateFormatter {
   const std::string& formatString() const { return raw_format_string_; }
 
 private:
-  void parse(const std::string& format_string);
-
-  using SpecifierOffsets = std::vector<int32_t>;
-  std::string fromTimeAndPrepareSpecifierOffsets(time_t time, SpecifierOffsets& specifier_offsets,
-                                                 const absl::string_view seconds_str) const;
+  void parse(absl::string_view format_string);
 
   // A container to hold a specifiers (%f, %Nf, %s) found in a format string.
-  struct Specifier {
-    // To build a subsecond-specifier.
-    Specifier(const size_t position, const size_t width, const std::string& segment)
-        : position_(position), width_(width), segment_(segment), second_(false) {}
+  class Specifier {
+  public:
+    enum class SpecifierType : uint8_t {
+      String = 0,     // means the specifier is a string specifier.
+      Second,         // means the specifier is a seconds specifier.
+      EnvoySubsecond, // subseconds specifiers %f, %#f and %*f that supported by Envoy.
+      AbslSubsecondS, // subseconds specifiers %E#S, %E*S.
+      AbslSubsecondF, // subseconds specifiers %E#f, %E*f.
+    };
+
+    // To build a seconds or subseconds specifier.
+    Specifier(SpecifierType specifier, uint8_t width) : specifier_(specifier), width_(width) {
+      // String specifier should call another constructor.
+      ASSERT(specifier != SpecifierType::String);
+
+      if (specifier == SpecifierType::Second) {
+        // Seconds specifier.
+        ASSERT(width == 0);
+      } else {
+        ASSERT(width >= 1);
+        ASSERT(width <= 9 || width == std::numeric_limits<uint8_t>::max());
+      }
+    }
 
-    // To build a second-specifier (%s), the number of characters to be replaced is always 2.
-    Specifier(const size_t position, const std::string& segment)
-        : position_(position), width_(2), segment_(segment), second_(true) {}
+    // To build a string specifier.
+    Specifier(absl::string_view string)
+        : string_(string), absl_format_(string_.find('%') != std::string::npos),
+          specifier_(SpecifierType::String) {
+      ASSERT(!string.empty());
+    }
 
-    // The position/index of a specifier in a format string.
-    const size_t position_;
+    // Format a time point based on the specifier.
+    std::string toString(SystemTime time, std::chrono::seconds epoch_time_seconds,
+                         bool local_time_zone) const;
 
-    // The width of a specifier, e.g. given %3f, the width is 3. If %f is set as the
-    // specifier, the width value should be 9 (the number of nanosecond digits).
-    const size_t width_;
+    // Format a time point based on the specifier. This should only be called for subseconds
+    // specifiers.
+    std::string subsecondsToString(SystemTime time) const;
 
-    // The string before the current specifier's position and after the previous found specifier. A
-    // segment may include absl::FormatTime accepted specifiers. E.g. given
+    /**
+     * @return bool whether the specifier is a subseconds specifier.
+     */
+    bool subsecondsSpecifier() const { return specifier_ > SpecifierType::Second; }
+
+  private:
+    // The format string before the current specifier's position and after the previous found
+    // specifier. The string may include absl::FormatTime accepted specifiers. E.g. given
     // "%3f-this-i%s-a-segment-%4f", the current specifier is "%4f" and the segment is
-    // "-this-i%s-a-segment-".
-    const std::string segment_;
+    // "-a-segment-".
+    const std::string string_;
+    // If the string_ contains absl::FormatTime accepted specifiers, this is true. Or string_
+    // will be treated as raw string.
+    const bool absl_format_{};
+
+    // Specifier type.
+    const SpecifierType specifier_{};
 
-    // As an indication that this specifier is a %s (expect to be replaced by seconds since the
-    // epoch).
-    const bool second_;
+    // The width of a sub-second specifier, e.g. given %3f, the width is 3. If %f is set as the
+    // specifier, the width value should be 9 (the number of nanosecond digits).
+    // The allowed values are:
+    // 0: only when the specifier is seconds specifier or string specifier.
+    // 1-9: fixed width of subseconds specifier.
+    // 255: std::numeric_limits<uint8_t>::max(), full subseconds precision with dynamic width.
+    const uint8_t width_{};
   };
 
-  // This holds all specifiers found in a given format string.
-  std::vector<Specifier> specifiers_;
+  // A struct to hold the offset and length of a specifier result in the format string.
+  struct SpecifierOffset {
+    size_t offset{};
+    size_t length{};
+  };
+
+  struct CacheableTime {
+    std::chrono::seconds epoch_time_seconds;
+
+    std::string formatted;
+    std::vector<SpecifierOffset> offsets;
+  };
+  using CachedTimes = absl::node_hash_map<std::string, const CacheableTime>;
+
+  CacheableTime formatTimeAndOffsets(SystemTime time,
+                                     std::chrono::seconds epoch_time_seconds) const;
 
   // This is the format string as supplied in configuration, e.g. "foo %3f bar".
   const std::string raw_format_string_;
+  const size_t raw_format_hash_{};
+
+  // Use local time zone instead of UTC if this is set to true.
+  const bool local_time_{};
 
-  // Whether use local time zone.
-  const bool local_time_;
+  // This holds all specifiers found in a given format string.
+  std::vector<Specifier> specifiers_;
 };
 
 /**

source/common/config/utility.h

@@ -418,21 +418,24 @@ class Utility {
    * @param filter_chain_type the type of filter chain.
    * @param is_terminal_filter true if the filter is designed to be terminal.
    * @param last_filter_in_current_config true if the filter is last in the configuration.
-   * @throws EnvoyException if there is a mismatch between design and configuration.
+   * @return a status indicating if there is a mismatch between design and configuration.
    */
-  static void validateTerminalFilters(const std::string& name, const std::string& filter_type,
-                                      const std::string& filter_chain_type, bool is_terminal_filter,
-                                      bool last_filter_in_current_config) {
+  static absl::Status validateTerminalFilters(const std::string& name,
+                                              const std::string& filter_type,
+                                              const std::string& filter_chain_type,
+                                              bool is_terminal_filter,
+                                              bool last_filter_in_current_config) {
     if (is_terminal_filter && !last_filter_in_current_config) {
-      ExceptionUtil::throwEnvoyException(
+      return absl::InvalidArgumentError(
           fmt::format("Error: terminal filter named {} of type {} must be the "
                       "last filter in a {} filter chain.",
                       name, filter_type, filter_chain_type));
     } else if (!is_terminal_filter && last_filter_in_current_config) {
-      ExceptionUtil::throwEnvoyException(fmt::format(
+      return absl::InvalidArgumentError(fmt::format(
           "Error: non-terminal filter named {} of type {} is the last filter in a {} filter chain.",
           name, filter_type, filter_chain_type));
     }
+    return absl::OkStatus();
   }
 
   /**

source/common/filter/config_discovery_impl.h

@@ -213,8 +213,9 @@ class HttpDynamicFilterConfigProviderImpl
     auto* factory =
         Registry::FactoryRegistry<NeutralHttpFilterConfigFactory>::getFactory(factory_name);
     const bool is_terminal_filter = factory->isTerminalFilterByProto(message, server_context_);
-    Config::Utility::validateTerminalFilters(config_name, factory_name, filter_chain_type_,
-                                             is_terminal_filter, last_filter_in_filter_chain_);
+    THROW_IF_NOT_OK(Config::Utility::validateTerminalFilters(config_name, factory_name,
+                                                             filter_chain_type_, is_terminal_filter,
+                                                             last_filter_in_filter_chain_));
   }
 
 private:
@@ -279,9 +280,9 @@ class DownstreamNetworkDynamicFilterConfigProviderImpl
         Registry::FactoryRegistry<NeutralNetworkFilterConfigFactory>::getFactory(factory_name);
     const bool is_terminal_filter =
         factory->isTerminalFilterByProto(message, this->server_context_);
-    Config::Utility::validateTerminalFilters(config_name, factory_name, this->filter_chain_type_,
-                                             is_terminal_filter,
-                                             this->last_filter_in_filter_chain_);
+    THROW_IF_NOT_OK(Config::Utility::validateTerminalFilters(
+        config_name, factory_name, this->filter_chain_type_, is_terminal_filter,
+        this->last_filter_in_filter_chain_));
   }
 };
 
@@ -668,8 +669,9 @@ class HttpFilterConfigProviderManagerImpl
   void validateFilters(const std::string& filter_config_name, const std::string& filter_type,
                        const std::string& filter_chain_type, bool is_terminal_filter,
                        bool last_filter_in_filter_chain) const override {
-    Config::Utility::validateTerminalFilters(filter_config_name, filter_type, filter_chain_type,
-                                             is_terminal_filter, last_filter_in_filter_chain);
+    THROW_IF_NOT_OK(Config::Utility::validateTerminalFilters(filter_config_name, filter_type,
+                                                             filter_chain_type, is_terminal_filter,
+                                                             last_filter_in_filter_chain));
   }
   const std::string getConfigDumpType() const override { return "ecds_filter_http"; }
 };
@@ -695,8 +697,9 @@ class UpstreamHttpFilterConfigProviderManagerImpl
   void validateFilters(const std::string& filter_config_name, const std::string& filter_type,
                        const std::string& filter_chain_type, bool is_terminal_filter,
                        bool last_filter_in_filter_chain) const override {
-    Config::Utility::validateTerminalFilters(filter_config_name, filter_type, filter_chain_type,
-                                             is_terminal_filter, last_filter_in_filter_chain);
+    THROW_IF_NOT_OK(Config::Utility::validateTerminalFilters(filter_config_name, filter_type,
+                                                             filter_chain_type, is_terminal_filter,
+                                                             last_filter_in_filter_chain));
   }
   const std::string getConfigDumpType() const override { return "ecds_filter_upstream_http"; }
 };
@@ -722,8 +725,9 @@ class NetworkFilterConfigProviderManagerImpl
   void validateFilters(const std::string& filter_config_name, const std::string& filter_type,
                        const std::string& filter_chain_type, bool is_terminal_filter,
                        bool last_filter_in_filter_chain) const override {
-    Config::Utility::validateTerminalFilters(filter_config_name, filter_type, filter_chain_type,
-                                             is_terminal_filter, last_filter_in_filter_chain);
+    THROW_IF_NOT_OK(Config::Utility::validateTerminalFilters(filter_config_name, filter_type,
+                                                             filter_chain_type, is_terminal_filter,
+                                                             last_filter_in_filter_chain));
   }
   const std::string getConfigDumpType() const override { return "ecds_filter_network"; }
 };

source/common/http/filter_chain_helper.h

@@ -149,9 +149,9 @@ class FilterChainHelper : Logger::Loggable<Logger::Id::config> {
     Http::FilterFactoryCb callback = callback_or_error.value();
     dependency_manager.registerFilter(factory->name(), *factory->dependencies());
     const bool is_terminal = factory->isTerminalFilterByProto(*message, server_context_);
-    Config::Utility::validateTerminalFilters(proto_config.name(), factory->name(),
-                                             filter_chain_type, is_terminal,
-                                             last_filter_in_current_config);
+    RETURN_IF_NOT_OK(Config::Utility::validateTerminalFilters(proto_config.name(), factory->name(),
+                                                              filter_chain_type, is_terminal,
+                                                              last_filter_in_current_config));
     auto filter_config_provider = filter_config_provider_manager_.createStaticFilterConfigProvider(
         {factory->name(), callback}, proto_config.name());
 #ifdef ENVOY_ENABLE_YAML

source/common/listener_manager/listener_manager_impl.cc

@@ -112,11 +112,11 @@ ProdListenerComponentFactory::createNetworkFilterFactoryListImpl(
 
     auto message = Config::Utility::translateToFactoryConfig(
         proto_config, filter_chain_factory_context.messageValidationVisitor(), factory);
-    Config::Utility::validateTerminalFilters(
+    RETURN_IF_NOT_OK(Config::Utility::validateTerminalFilters(
         filters[i].name(), factory.name(), "network",
         factory.isTerminalFilterByProto(*message,
                                         filter_chain_factory_context.serverFactoryContext()),
-        is_terminal);
+        is_terminal));
     auto callback_or_error =
         factory.createFilterFactoryFromProto(*message, filter_chain_factory_context);
     RETURN_IF_NOT_OK(callback_or_error.status());

source/common/singleton/threadsafe_singleton.h

@@ -66,6 +66,9 @@ template <class T> class InjectableSingleton {
   }
   static void clear() { loader_ = nullptr; }
 
+  // Atomically replace the value, returning the old value.
+  static T* replaceForTest(T* new_value) { return loader_.exchange(new_value); }
+
 protected:
   static std::atomic<T*> loader_;
 };
@@ -86,20 +89,16 @@ template <class T> class ScopedInjectableLoader {
   std::unique_ptr<T> instance_;
 };
 
-// This class saves the singleton object and restore the original singleton at destroy. This class
-// is not thread safe. It can be used in single thread test.
-template <class T>
-class StackedScopedInjectableLoader :
-    // To access the protected loader_.
-    protected InjectableSingleton<T> {
+// This class saves the singleton object and restore the original singleton at destroy.
+template <class T> class StackedScopedInjectableLoaderForTest {
 public:
-  explicit StackedScopedInjectableLoader(std::unique_ptr<T>&& instance) {
-    original_loader_ = InjectableSingleton<T>::getExisting();
-    InjectableSingleton<T>::clear();
+  explicit StackedScopedInjectableLoaderForTest(std::unique_ptr<T>&& instance) {
     instance_ = std::move(instance);
-    InjectableSingleton<T>::initialize(instance_.get());
+    original_loader_ = InjectableSingleton<T>::replaceForTest(instance_.get());
+  }
+  ~StackedScopedInjectableLoaderForTest() {
+    InjectableSingleton<T>::replaceForTest(original_loader_);
   }
-  ~StackedScopedInjectableLoader() { InjectableSingleton<T>::loader_ = original_loader_; }
 
 private:
   std::unique_ptr<T> instance_;