diff --git a/changelogs/current.yaml b/changelogs/current.yaml index a23ee219a437..3747d944ac12 100644 --- a/changelogs/current.yaml +++ b/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 diff --git a/source/common/common/BUILD b/source/common/common/BUILD index 2f4558fb3c6c..ec85b28e5ba1 100644 --- a/source/common/common/BUILD +++ b/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", ], ) diff --git a/source/common/common/utility.cc b/source/common/common/utility.cc index 463683960a20..91dcd9d0f263 100644 --- a/source/common/common/utility.cc +++ b/source/common/common/utility.cc @@ -18,13 +18,13 @@ #include "source/common/common/hash.h" #include "source/common/singleton/const_singleton.h" -#include "absl/container/node_hash_map.h" #include "absl/strings/ascii.h" #include "absl/strings/match.h" #include "absl/strings/str_join.h" #include "absl/strings/str_replace.h" #include "absl/strings/str_split.h" #include "absl/time/time.h" +#include "re2/re2.h" #include "spdlog/spdlog.h" namespace Envoy { @@ -33,9 +33,11 @@ namespace { class SpecifierConstantValues { public: - // This captures three groups: subsecond-specifier, subsecond-specifier width and - // second-specifier. - const std::regex PATTERN{"(%([1-9])?f)|(%s)", std::regex::optimize}; + // This pattern contains three parts: + // 1. %[1-9*]?f: Envoy subseconds specifier with an optional width. + // 2. %s: Envoy seconds specifier. + // 3. %E[1-9*][Sf]: absl::FormatTime() seconds/subseconds specifier. + const re2::RE2 RE2_PATTERN{"(%[1-9*]?f|%s|%E[1-9*][Sf])"}; }; using SpecifierConstants = ConstSingleton; @@ -68,160 +70,240 @@ const std::string errorDetails(int error_code) { #endif } -std::string DateFormatter::fromTime(const SystemTime& time) const { - struct CachedTime { - // The string length of a number of seconds since the Epoch. E.g. for "1528270093", the length - // is 10. - size_t seconds_length; - - // A container object to hold a absl::FormatTime string, its timestamp (in seconds) and a list - // of position offsets for each specifier found in a format string. - struct Formatted { - // The resulted string after format string is passed to absl::FormatTime at a given point in - // time. - std::string str; - - // A timestamp (in seconds) when this object is created. - std::chrono::seconds epoch_time_seconds; - - // List of offsets for each specifier found in a format string. This is needed to compensate - // the position of each recorded specifier due to the possible size change of the previous - // segment (after being formatted). - SpecifierOffsets specifier_offsets; - }; - // A map is used to keep different formatted format strings at a given second. - absl::node_hash_map formatted; - }; - static thread_local CachedTime cached_time_utc; - static thread_local CachedTime cached_time_local; +std::string DateFormatter::fromTime(SystemTime time) const { + // A map is used to keep different formatted format strings at a given seconds. + static thread_local CachedTimes CACHE; + static thread_local CachedTimes CACHE_LOCAL; - const std::chrono::nanoseconds epoch_time_ns = - std::chrono::duration_cast(time.time_since_epoch()); + if (specifiers_.empty()) { + return {}; + } - const std::chrono::seconds epoch_time_seconds = - std::chrono::duration_cast(epoch_time_ns); + CachedTimes& cached_times = local_time_ ? CACHE_LOCAL : CACHE; + + const auto epoch_time_ss = + std::chrono::duration_cast(time.time_since_epoch()); + + const auto iter = cached_times.find(raw_format_string_, raw_format_hash_); + + if (iter == cached_times.end() || iter->second.epoch_time_seconds != epoch_time_ss) { + // No cached entry found for the given format string and time. - CachedTime& cached_time = local_time_ ? cached_time_local : cached_time_utc; - const auto& item = cached_time.formatted.find(raw_format_string_); - if (item == cached_time.formatted.end() || - item->second.epoch_time_seconds != epoch_time_seconds) { // Remove all the expired cached items. - for (auto it = cached_time.formatted.cbegin(); it != cached_time.formatted.cend();) { - if (it->second.epoch_time_seconds != epoch_time_seconds) { + for (auto it = cached_times.cbegin(); it != cached_times.cend();) { + if (it->second.epoch_time_seconds != epoch_time_ss) { auto next_it = std::next(it); - cached_time.formatted.erase(it); + cached_times.erase(it); it = next_it; } else { ++it; } } - const time_t current_time = std::chrono::system_clock::to_time_t(time); - - // Build a new formatted format string at current time. - CachedTime::Formatted formatted; - const std::string seconds_str = fmt::format_int(epoch_time_seconds.count()).str(); - formatted.str = - fromTimeAndPrepareSpecifierOffsets(current_time, formatted.specifier_offsets, seconds_str); - cached_time.seconds_length = seconds_str.size(); + auto new_iter = cached_times.emplace( + std::make_pair(raw_format_string_, formatTimeAndOffsets(time, epoch_time_ss))); - // Stamp the formatted string using the current epoch time in seconds, and then cache it in. - formatted.epoch_time_seconds = epoch_time_seconds; - cached_time.formatted.emplace(std::make_pair(raw_format_string_, formatted)); + ASSERT(new_iter.second); + return new_iter.first->second.formatted; } - const auto& formatted = cached_time.formatted.at(raw_format_string_); - ASSERT(specifiers_.size() == formatted.specifier_offsets.size()); + const auto& formatted = iter->second; + ASSERT(specifiers_.size() == formatted.offsets.size()); - // Copy the current cached formatted format string, then replace its subseconds part (when it has - // non-zero width) by correcting its position using prepared subseconds offsets. - std::string formatted_str = formatted.str; - std::string nanoseconds = fmt::format_int(epoch_time_ns.count()).str(); - // Special case handling for beginning of time, we should never need to do this outside of - // tests or a time machine. - if (nanoseconds.size() < 10) { - nanoseconds = std::string(10 - nanoseconds.size(), '0') + nanoseconds; - } - - for (size_t i = 0; i < specifiers_.size(); ++i) { - const auto& specifier = specifiers_.at(i); + // Copy the current cached formatted format string, then replace its subseconds specifier + // (when it has non-zero width) by correcting its position using prepared subseconds offsets. + std::string formatted_str = formatted.formatted; - // When specifier.width_ is zero, skip the replacement. This is the last segment or it has no - // specifier. - if (specifier.width_ > 0 && !specifier.second_) { - ASSERT(specifier.position_ + formatted.specifier_offsets.at(i) < formatted_str.size()); - formatted_str.replace(specifier.position_ + formatted.specifier_offsets.at(i), - specifier.width_, - nanoseconds.substr(cached_time.seconds_length, specifier.width_)); + for (size_t i = specifiers_.size(); i != 0; i--) { + if (specifiers_[i - 1].subsecondsSpecifier()) { + const auto offset = formatted.offsets[i - 1]; + const auto substr = specifiers_[i - 1].subsecondsToString(time); + formatted_str.replace(offset.offset, offset.length, substr); } } - - ASSERT(formatted_str.size() == formatted.str.size()); return formatted_str; } -void DateFormatter::parse(const std::string& format_string) { - std::string suffix = format_string; - std::smatch matched; - // "step" is the last specifier's position + the last specifier's width. It's not the current - // position in "format_string" because the length has changed. It is actually the index which - // points to the end of the last specifier in formatted string (generated in the future). - size_t step = 0; - while (regex_search(suffix, matched, SpecifierConstants::get().PATTERN)) { - // The std::smatch matched for (%([1-9])?f)|(%s): [all, subsecond-specifier, subsecond-specifier - // width, second-specifier]. - const std::string& width_specifier = matched[2]; - const std::string& second_specifier = matched[3]; - - // In the template string to be used in runtime substitution, the width is the number of - // characters to be replaced. - const size_t width = width_specifier.empty() ? 9 : width_specifier.at(0) - '0'; - - ASSERT(!suffix.empty()); - // This records matched position, the width of current subsecond pattern, and also the string - // segment before the matched position. These values will be used later at data path. - specifiers_.emplace_back( - second_specifier.empty() - ? Specifier(step + matched.position(), width, suffix.substr(0, matched.position())) - : Specifier(step + matched.position(), suffix.substr(0, matched.position()))); - step = specifiers_.back().position_ + specifiers_.back().width_; - suffix = matched.suffix(); +void DateFormatter::parse(absl::string_view format_string) { + absl::string_view format = format_string; + absl::string_view matched; + + // PartialMatch will return the leftmost-longest match. And the matched will be mutated + // to point to matched piece + while (re2::RE2::PartialMatch(format, SpecifierConstants::get().RE2_PATTERN, &matched)) { + ASSERT(!matched.empty()); + + // Get matched position based the matched and format view. For example: + // this-is-prefix-string-%s-this-is-suffix-string + // |------ prefix -------|| | + // | matched | + // |------------------ format ------------------| + const size_t prefix_length = matched.data() - format.data(); + + std::string prefix_string = std::string(format.substr(0, prefix_length)); + + Specifier::SpecifierType specifier{}; + uint8_t width{}; + + if (matched.size() == 2) { + // Handle the seconds specifier (%s) or subseconds specifier without width (%f). + + ASSERT(matched[1] == 's' || matched[1] == 'f'); + if (matched[1] == 's') { + specifier = Specifier::SpecifierType::Second; + } else { + specifier = Specifier::SpecifierType::EnvoySubsecond; + width = 9; + } + } else if (matched.size() == 3) { + // Handle subseconds specifier with width (%#f, %*f, '#' means number between 1-9, + // '*' is literal). + + ASSERT(matched[2] == 'f'); + specifier = Specifier::SpecifierType::EnvoySubsecond; + width = matched[1] == '*' ? std::numeric_limits::max() : matched[1] - '0'; + } else { + // To improve performance, we will cache the formatted string for every second. + // And when doing the runtime substitution, we will get the cached result and + // replace the subseconds part. + // In order to make sure the subseconds part of absl::FormatTime() can be replaced, + // we need to handle the subseconds specifier of absl::FormatTime() here. + + // Handle the absl subseconds specifier (%E#S, %E*S, %E#f, %E*f, '#' means number + // between 1-9, '*' is literal). + + ASSERT(matched.size() == 4); + ASSERT(matched[3] == 'S' || matched[3] == 'f'); + + if (matched[3] == 'S') { + // %E#S or %E*S includes the seconds, dot, and subseconds. For example, %E6S + // will output "30.654321". + // We will let the absl::FormatTime() to handle the seconds parts. So we need + // to add the seconds specifier to the prefix string to ensure the final + // formatted string is correct. + prefix_string.push_back('%'); + prefix_string.push_back('S'); + + specifier = Specifier::SpecifierType::AbslSubsecondS; + } else { + specifier = Specifier::SpecifierType::AbslSubsecondF; + } + + width = matched[2] == '*' ? std::numeric_limits::max() : matched[2] - '0'; + } + + if (!prefix_string.empty()) { + specifiers_.push_back(Specifier(prefix_string)); + } + + ASSERT(format.size() >= prefix_length + matched.size()); + ASSERT(specifier != Specifier::SpecifierType::String); + specifiers_.emplace_back(Specifier(specifier, width)); + format = format.substr(prefix_length + matched.size()); } // To capture the segment after the last specifier pattern of a format string by creating a zero // width specifier. E.g. %3f-this-is-the-last-%s-segment-%Y-until-this. - if (!suffix.empty()) { - Specifier specifier(step, 0, suffix); - specifiers_.emplace_back(specifier); + if (!format.empty()) { + specifiers_.emplace_back(Specifier(format)); + } +} + +std::string DateFormatter::Specifier::subsecondsToString(SystemTime time) const { + ASSERT(specifier_ > SpecifierType::Second); + ASSERT(width_ > 0); + + absl::string_view nanoseconds; + + std::string string_buffer; + fmt::format_int formatted(std::chrono::nanoseconds(time.time_since_epoch()).count()); + + // 1. Get the subseconds part of the formatted time. + if (formatted.size() < 9) { + // Special case. This should never happen in actual practice. + string_buffer = std::string(9 - formatted.size(), '0'); + string_buffer.append(formatted.data(), formatted.size()); + nanoseconds = string_buffer; + } else { + nanoseconds = absl::string_view(formatted.data() + formatted.size() - 9, 9); + } + ASSERT(nanoseconds.size() == 9); + + // 2. Calculate the actual width that will be used. + uint8_t width = width_; + if (width == std::numeric_limits::max()) { + // Dynamic width specifier. Remove trailing zeros. + if (const auto i = nanoseconds.find_last_not_of('0'); i != absl::string_view::npos) { + width = i + 1; + } else { + // This only happens for subseconds specifiers with dynamic width (%E*S, %E*f, %*f) + // and the subseconds are all zeros. + width = 0; + } } + + // 3. Handle the specifiers of %E#S and %E*S. The seconds part will be handled by + // absl::FormatTime() by string specifier. So we only need to return the dot and + // subseconds part. + if (specifier_ == SpecifierType::AbslSubsecondS) { + if (width == 0) { + // No subseconds and dot for %E*S in this case. + return {}; + } + + std::string output; + output.push_back('.'); // Add the dot. + output.append(nanoseconds.data(), width); + return output; + } + + // 4. Handle the specifiers of %E#f, %E*f, and %f, %#f, %*f. At least one subsecond digit + // will be returned for these specifiers even if the subseconds are all zeros and dynamic + // width is used. + return std::string(nanoseconds.substr(0, std::max(width, 1))); +} + +std::string DateFormatter::Specifier::toString(SystemTime time, + std::chrono::seconds epoch_time_seconds, + bool local) const { + switch (specifier_) { + case SpecifierType::String: + ASSERT(!string_.empty()); + // Handle the string first. It may be absl::FormatTime() format string. + if (absl_format_) { + return absl::FormatTime(string_, absl::FromChrono(time), + local ? absl::LocalTimeZone() : absl::UTCTimeZone()); + } else { + return string_; + } + case SpecifierType::Second: + // Handle the seconds specifier. + return fmt::format_int(epoch_time_seconds.count()).str(); + case SpecifierType::EnvoySubsecond: + case SpecifierType::AbslSubsecondS: + case SpecifierType::AbslSubsecondF: + // Handle the sub-seconds specifier. + return subsecondsToString(time); + } + + return {}; // Should never reach here. Make the gcc happy. } -std::string -DateFormatter::fromTimeAndPrepareSpecifierOffsets(time_t time, SpecifierOffsets& specifier_offsets, - const absl::string_view seconds_str) const { - std::string formatted_time; +DateFormatter::CacheableTime +DateFormatter::formatTimeAndOffsets(SystemTime time, + std::chrono::seconds epoch_time_seconds) const { + CacheableTime ret; + ret.epoch_time_seconds = epoch_time_seconds; + ret.formatted.reserve(64); + ret.offsets.reserve(specifiers_.size()); - int32_t previous = 0; - specifier_offsets.reserve(specifiers_.size()); for (const auto& specifier : specifiers_) { - std::string current_format = - absl::FormatTime(specifier.segment_, absl::FromTimeT(time), - local_time_ ? absl::LocalTimeZone() : absl::UTCTimeZone()); - absl::StrAppend(&formatted_time, current_format, - specifier.second_ ? seconds_str : std::string(specifier.width_, '?')); - - // This computes and saves offset of each specifier's pattern to correct its position after the - // previous string segment is formatted. An offset can be a negative value. - // - // If the current specifier is a second specifier (%s), it needs to be corrected by 2. - const int32_t offset = - (current_format.length() + (specifier.second_ ? (seconds_str.size() - 2) : 0)) - - specifier.segment_.size(); - specifier_offsets.emplace_back(previous + offset); - previous += offset; - } - - return formatted_time; + const auto offset = ret.formatted.size(); + ret.formatted.append(specifier.toString(time, epoch_time_seconds, local_time_)); + ret.offsets.push_back({offset, ret.formatted.size() - offset}); + } + + return ret; } std::string DateFormatter::now(TimeSource& time_source) const { diff --git a/source/common/common/utility.h b/source/common/common/utility.h index 872904b6a47a..47d1082da1cd 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -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,8 +46,9 @@ 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); } @@ -53,7 +56,7 @@ class DateFormatter { * @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; - 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::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::max(), full subseconds precision with dynamic width. + const uint8_t width_{}; }; - // This holds all specifiers found in a given format string. - std::vector 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 offsets; + }; + using CachedTimes = absl::node_hash_map; + + 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 specifiers_; }; /** diff --git a/source/common/config/utility.h b/source/common/config/utility.h index a3b83c808350..6c8e5a6ca8b5 100644 --- a/source/common/config/utility.h +++ b/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(); } /** diff --git a/source/common/filter/config_discovery_impl.h b/source/common/filter/config_discovery_impl.h index 652b6f54b8c5..1aaec8a0ab3d 100644 --- a/source/common/filter/config_discovery_impl.h +++ b/source/common/filter/config_discovery_impl.h @@ -213,8 +213,9 @@ class HttpDynamicFilterConfigProviderImpl auto* factory = Registry::FactoryRegistry::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::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"; } }; diff --git a/source/common/http/filter_chain_helper.h b/source/common/http/filter_chain_helper.h index 51852499317c..7b8df370b0d6 100644 --- a/source/common/http/filter_chain_helper.h +++ b/source/common/http/filter_chain_helper.h @@ -149,9 +149,9 @@ class FilterChainHelper : Logger::Loggable { 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 diff --git a/source/common/listener_manager/listener_manager_impl.cc b/source/common/listener_manager/listener_manager_impl.cc index d838975acd76..f0841e119d3e 100644 --- a/source/common/listener_manager/listener_manager_impl.cc +++ b/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()); diff --git a/source/common/singleton/threadsafe_singleton.h b/source/common/singleton/threadsafe_singleton.h index 3af33deb2548..cb5de79254bf 100644 --- a/source/common/singleton/threadsafe_singleton.h +++ b/source/common/singleton/threadsafe_singleton.h @@ -66,6 +66,9 @@ template 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 loader_; }; @@ -86,20 +89,16 @@ template class ScopedInjectableLoader { std::unique_ptr 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 StackedScopedInjectableLoader : - // To access the protected loader_. - protected InjectableSingleton { +// This class saves the singleton object and restore the original singleton at destroy. +template class StackedScopedInjectableLoaderForTest { public: - explicit StackedScopedInjectableLoader(std::unique_ptr&& instance) { - original_loader_ = InjectableSingleton::getExisting(); - InjectableSingleton::clear(); + explicit StackedScopedInjectableLoaderForTest(std::unique_ptr&& instance) { instance_ = std::move(instance); - InjectableSingleton::initialize(instance_.get()); + original_loader_ = InjectableSingleton::replaceForTest(instance_.get()); + } + ~StackedScopedInjectableLoaderForTest() { + InjectableSingleton::replaceForTest(original_loader_); } - ~StackedScopedInjectableLoader() { InjectableSingleton::loader_ = original_loader_; } private: std::unique_ptr instance_; diff --git a/test/common/common/utility_test.cc b/test/common/common/utility_test.cc index 3e9550a546d6..10e52d189f56 100644 --- a/test/common/common/utility_test.cc +++ b/test/common/common/utility_test.cc @@ -1,6 +1,7 @@ #include #include #include +#include #include #include #include @@ -1043,6 +1044,56 @@ TEST(DateFormatter, FromTime) { const SystemTime time2(std::chrono::seconds(0)); EXPECT_EQ("1970-01-01T00:00:00.000Z", DateFormatter("%Y-%m-%dT%H:%M:%S.000Z").fromTime(time2)); EXPECT_EQ("aaa00", DateFormatter(std::string(3, 'a') + "%H").fromTime(time2)); + + const SystemTime time3(std::chrono::milliseconds(1522796769321)); + EXPECT_EQ("2018-04-03T23:06:09.321Z", DateFormatter("%Y-%m-%dT%H:%M:%E3SZ").fromTime(time3)); + EXPECT_EQ("aaa23", DateFormatter(std::string(3, 'a') + "%H").fromTime(time1)); + const SystemTime time4(std::chrono::seconds(0)); + EXPECT_EQ("1970-01-01T00:00:00.000Z", DateFormatter("%Y-%m-%dT%H:%M:%E3SZ").fromTime(time4)); + EXPECT_EQ("aaa00", DateFormatter(std::string(3, 'a') + "%H").fromTime(time2)); + + const SystemTime time5(std::chrono::milliseconds(321)); + EXPECT_EQ("1970-01-01T00:00:00.321Z", DateFormatter("%Y-%m-%dT%H:%M:%E3SZ").fromTime(time5)); + EXPECT_EQ("aaa00", DateFormatter(std::string(3, 'a') + "%H").fromTime(time2)); +} + +TEST(DateFormatter, DateFormatterVsAbslFormatTime) { + const std::string format = "%Y-%m-%dT%H:%M:%E3SZ %Ez %E*z %E8S %E*S %E5f %E*f %E4Y %ET"; + + SystemTime zero_time(std::chrono::seconds(0)); + + EXPECT_EQ(DateFormatter(format).fromTime(zero_time), + absl::FormatTime(format, absl::FromChrono(zero_time), absl::UTCTimeZone())); + + std::mt19937 prng(1); + std::uniform_int_distribution distribution(-10, 20); + + SystemTime now = std::chrono::system_clock::now(); // NO_CHECK_FORMAT(real_time) + + for (size_t i = 0; i < 20; i++) { + auto time = now + std::chrono::milliseconds(static_cast(distribution(prng))); + // UTC time zone. + EXPECT_EQ(DateFormatter(format).fromTime(time), + absl::FormatTime(format, absl::FromChrono(time), absl::UTCTimeZone())); + + // Local time zone. + EXPECT_EQ(DateFormatter(format, true).fromTime(time), + absl::FormatTime(format, absl::FromChrono(time), absl::LocalTimeZone())); + } +} + +TEST(DateFormatter, HybridAbsl) { + const std::string format = "%Y-%m-%dT%H:%M:%E3SZ %E6S %E*S %E4f %E*f %S. %s %3f %6f %9f"; + + const SystemTime time1(std::chrono::seconds(1522796769) + std::chrono::microseconds(123450)); + + EXPECT_EQ( + "2018-04-03T23:06:09.123Z 09.123450 09.12345 1234 12345 09. 1522796769 123 123450 123450000", + DateFormatter(format).fromTime(time1)); + + const SystemTime time2(std::chrono::seconds(0)); + EXPECT_EQ("1970-01-01T00:00:00.000Z 00.000000 00 0000 0 00. 0 000 000000 000000000", + DateFormatter(format).fromTime(time2)); } TEST(DateFormatter, FromTimeLocalTimeZone) { @@ -1084,9 +1135,6 @@ TEST(DateFormatter, ParseLongString) { EXPECT_EQ(expected_output, output); } -// Verify that two DateFormatter patterns with the same ??? patterns but -// different format strings don't false share cache entries. This is a -// regression test for when they did. TEST(DateFormatter, FromTimeSameWildcard) { const SystemTime time1(std::chrono::seconds(1522796769) + std::chrono::milliseconds(142)); EXPECT_EQ("2018-04-03T23:06:09.000Z142", diff --git a/test/common/listener_manager/listener_manager_impl_test.cc b/test/common/listener_manager/listener_manager_impl_test.cc index bb81bb701d7a..d296966813b4 100644 --- a/test/common/listener_manager/listener_manager_impl_test.cc +++ b/test/common/listener_manager/listener_manager_impl_test.cc @@ -2604,7 +2604,8 @@ TEST_P(ListenerManagerImplTest, BindToPortEqualToFalse) { InSequence s; auto mock_interface = std::make_unique( std::vector{Network::Address::IpVersion::v4}); - StackedScopedInjectableLoader new_interface(std::move(mock_interface)); + StackedScopedInjectableLoaderForTest new_interface( + std::move(mock_interface)); ProdListenerComponentFactory real_listener_factory(server_); EXPECT_CALL(*worker_, start(_, _)); @@ -2643,7 +2644,8 @@ TEST_P(ListenerManagerImplTest, UpdateBindToPortEqualToFalse) { InSequence s; auto mock_interface = std::make_unique( std::vector{Network::Address::IpVersion::v4}); - StackedScopedInjectableLoader new_interface(std::move(mock_interface)); + StackedScopedInjectableLoaderForTest new_interface( + std::move(mock_interface)); ProdListenerComponentFactory real_listener_factory(server_); EXPECT_CALL(*worker_, start(_, _)); diff --git a/test/common/network/listen_socket_impl_test.cc b/test/common/network/listen_socket_impl_test.cc index 0bc6bd067bdc..769a62739177 100644 --- a/test/common/network/listen_socket_impl_test.cc +++ b/test/common/network/listen_socket_impl_test.cc @@ -229,7 +229,7 @@ TEST_P(ListenSocketImplTestTcp, SupportedIpFamilyVirtualSocketIsCreatedWithNoBsd auto any_address = version_ == Address::IpVersion::v4 ? Utility::getIpv4AnyAddress() : Utility::getIpv6AnyAddress(); - StackedScopedInjectableLoader new_interface(std::move(mock_interface)); + StackedScopedInjectableLoaderForTest new_interface(std::move(mock_interface)); { EXPECT_CALL(*mock_interface_ptr, socket(_, _, _)).Times(0); @@ -245,7 +245,7 @@ TEST_P(ListenSocketImplTestTcp, DeathAtUnSupportedIpFamilyListenSocket) { auto* mock_interface_ptr = mock_interface.get(); auto the_other_address = version_ == Address::IpVersion::v4 ? Utility::getIpv6AnyAddress() : Utility::getIpv4AnyAddress(); - StackedScopedInjectableLoader new_interface(std::move(mock_interface)); + StackedScopedInjectableLoaderForTest new_interface(std::move(mock_interface)); { EXPECT_CALL(*mock_interface_ptr, socket(_, _, _)).Times(0); EXPECT_CALL(*mock_interface_ptr, socket(_, _, _, _, _)).Times(0); diff --git a/test/extensions/filters/network/http_connection_manager/config_filter_chain_test.cc b/test/extensions/filters/network/http_connection_manager/config_filter_chain_test.cc index 96175f7e4aa4..9cc93b5749b9 100644 --- a/test/extensions/filters/network/http_connection_manager/config_filter_chain_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_filter_chain_test.cc @@ -335,12 +335,12 @@ TEST_F(FilterChainTest, CreateCustomUpgradeFilterChainWithRouterNotLast) { "\x1D" "encoder-decoder-buffer-filter"); - EXPECT_THROW_WITH_MESSAGE( - HttpConnectionManagerConfig(hcm_config, context_, date_provider_, - route_config_provider_manager_, - &scoped_routes_config_provider_manager_, tracer_manager_, - filter_config_provider_manager_, creation_status_), - EnvoyException, + HttpConnectionManagerConfig config(hcm_config, context_, date_provider_, + route_config_provider_manager_, + &scoped_routes_config_provider_manager_, tracer_manager_, + filter_config_provider_manager_, creation_status_); + EXPECT_EQ( + creation_status_.message(), "Error: terminal filter named envoy.filters.http.router of type envoy.filters.http.router " "must be the last filter in a http upgrade filter chain."); } diff --git a/test/integration/socket_interface_swap.cc b/test/integration/socket_interface_swap.cc index 9ea29fa7ad3b..7d29f0691a47 100644 --- a/test/integration/socket_interface_swap.cc +++ b/test/integration/socket_interface_swap.cc @@ -3,10 +3,8 @@ namespace Envoy { SocketInterfaceSwap::SocketInterfaceSwap(Network::Socket::Type socket_type) - : write_matcher_(std::make_shared(socket_type)) { - Envoy::Network::SocketInterfaceSingleton::clear(); - test_socket_interface_loader_ = std::make_unique( - std::make_unique( + : write_matcher_(std::make_shared(socket_type)), + test_socket_interface_loader_(std::make_unique( [write_matcher = write_matcher_](Envoy::Network::TestIoSocketHandle* io_handle) -> absl::optional { Api::IoErrorPtr error_override = write_matcher->returnConnectOverride(io_handle); @@ -28,8 +26,7 @@ SocketInterfaceSwap::SocketInterfaceSwap(Network::Socket::Type socket_type) }, [write_matcher = write_matcher_](Network::IoHandle::RecvMsgOutput& output) { write_matcher->readOverride(output); - })); -} + })) {} void SocketInterfaceSwap::IoHandleMatcher::setResumeWrites() { absl::MutexLock lock(&mutex_); diff --git a/test/integration/socket_interface_swap.h b/test/integration/socket_interface_swap.h index 57e82087de8f..1e16aaaf6156 100644 --- a/test/integration/socket_interface_swap.h +++ b/test/integration/socket_interface_swap.h @@ -116,15 +116,8 @@ class SocketInterfaceSwap { explicit SocketInterfaceSwap(Network::Socket::Type socket_type); - ~SocketInterfaceSwap() { - test_socket_interface_loader_.reset(); - Envoy::Network::SocketInterfaceSingleton::initialize(previous_socket_interface_); - } - - Envoy::Network::SocketInterface* const previous_socket_interface_{ - Envoy::Network::SocketInterfaceSingleton::getExisting()}; std::shared_ptr write_matcher_; - std::unique_ptr test_socket_interface_loader_; + StackedScopedInjectableLoaderForTest test_socket_interface_loader_; }; } // namespace Envoy