formatter: use AccessLogType::NotSet instead of default value (envoyp…

…roxy#27034) use AccessLogType::NotSet instead of default parameter value Signed-off-by: ohadvano <[email protected]>
pchaseh · Apr 28, 2023 · b1d1e2d · b1d1e2d
1 parent e6281a2
commit b1d1e2d
Show file tree

Hide file tree

Showing 22 changed files with 1,195 additions and 1,065 deletions.
diff --git a/envoy/formatter/substitution_formatter.h b/envoy/formatter/substitution_formatter.h
@@ -29,12 +29,12 @@ class Formatter {
    * @param local_reply_body supplies the local reply body.
    * @return std::string string containing the complete formatted substitution line.
    */
-  virtual std::string
-  format(const Http::RequestHeaderMap& request_headers,
-         const Http::ResponseHeaderMap& response_headers,
-         const Http::ResponseTrailerMap& response_trailers,
-         const StreamInfo::StreamInfo& stream_info, absl::string_view local_reply_body,
-         AccessLog::AccessLogType access_log_type = AccessLog::AccessLogType::NotSet) const PURE;
+  virtual std::string format(const Http::RequestHeaderMap& request_headers,
+                             const Http::ResponseHeaderMap& response_headers,
+                             const Http::ResponseTrailerMap& response_trailers,
+                             const StreamInfo::StreamInfo& stream_info,
+                             absl::string_view local_reply_body,
+                             AccessLog::AccessLogType access_log_type) const PURE;
 };
 
 using FormatterPtr = std::unique_ptr<Formatter>;
@@ -58,12 +58,12 @@ class FormatterProvider {
    * @return absl::optional<std::string> optional string containing a single value extracted from
    * the given headers/trailers/stream.
    */
-  virtual absl::optional<std::string>
-  format(const Http::RequestHeaderMap& request_headers,
-         const Http::ResponseHeaderMap& response_headers,
-         const Http::ResponseTrailerMap& response_trailers,
-         const StreamInfo::StreamInfo& stream_info, absl::string_view local_reply_body,
-         AccessLog::AccessLogType access_log_type = AccessLog::AccessLogType::NotSet) const PURE;
+  virtual absl::optional<std::string> format(const Http::RequestHeaderMap& request_headers,
+                                             const Http::ResponseHeaderMap& response_headers,
+                                             const Http::ResponseTrailerMap& response_trailers,
+                                             const StreamInfo::StreamInfo& stream_info,
+                                             absl::string_view local_reply_body,
+                                             AccessLog::AccessLogType access_log_type) const PURE;
   /**
    * Extract a value from the provided headers/trailers/stream, preserving the value's type.
    * @param request_headers supplies the request headers.
@@ -74,12 +74,12 @@ class FormatterProvider {
    * @return ProtobufWkt::Value containing a single value extracted from the given
    *         headers/trailers/stream.
    */
-  virtual ProtobufWkt::Value formatValue(
-      const Http::RequestHeaderMap& request_headers,
-      const Http::ResponseHeaderMap& response_headers,
-      const Http::ResponseTrailerMap& response_trailers, const StreamInfo::StreamInfo& stream_info,
-      absl::string_view local_reply_body,
-      AccessLog::AccessLogType access_log_type = AccessLog::AccessLogType::NotSet) const PURE;
+  virtual ProtobufWkt::Value formatValue(const Http::RequestHeaderMap& request_headers,
+                                         const Http::ResponseHeaderMap& response_headers,
+                                         const Http::ResponseTrailerMap& response_trailers,
+                                         const StreamInfo::StreamInfo& stream_info,
+                                         absl::string_view local_reply_body,
+                                         AccessLog::AccessLogType access_log_type) const PURE;
 };
 
 using FormatterProviderPtr = std::unique_ptr<FormatterProvider>;

diff --git a/source/common/formatter/substitution_formatter.h b/source/common/formatter/substitution_formatter.h
diff --git a/source/common/local_reply/local_reply.cc b/source/common/local_reply/local_reply.cc
@@ -37,8 +37,8 @@ class BodyFormatter {
               const Http::ResponseTrailerMap& response_trailers,
               const StreamInfo::StreamInfo& stream_info, std::string& body,
               absl::string_view& content_type) const {
-    body =
-        formatter_->format(request_headers, response_headers, response_trailers, stream_info, body);
+    body = formatter_->format(request_headers, response_headers, response_trailers, stream_info,
+                              body, AccessLog::AccessLogType::NotSet);
     content_type = content_type_;
   }
 

diff --git a/source/common/router/header_formatter.cc b/source/common/router/header_formatter.cc
@@ -59,7 +59,7 @@ parseSubstitutionFormatField(absl::string_view field_name,
     return formatter->format(*Http::StaticEmptyHeaders::get().request_headers,
                              *Http::StaticEmptyHeaders::get().response_headers,
                              *Http::StaticEmptyHeaders::get().response_trailers, stream_info,
-                             absl::string_view());
+                             absl::string_view(), AccessLog::AccessLogType::NotSet);
   };
 }
 

diff --git a/source/common/router/header_formatter.h b/source/common/router/header_formatter.h
@@ -117,7 +117,8 @@ class HttpHeaderFormatterImpl : public HttpHeaderFormatter {
                            const Envoy::StreamInfo::StreamInfo& stream_info) const override {
     std::string buf;
     buf = formatter_->format(request_headers, response_headers,
-                             *Http::StaticEmptyHeaders::get().response_trailers, stream_info, "");
+                             *Http::StaticEmptyHeaders::get().response_trailers, stream_info, "",
+                             AccessLog::AccessLogType::NotSet);
     return buf;
   };
 

diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc
@@ -629,7 +629,7 @@ std::string TunnelingConfigHelperImpl::host(const StreamInfo::StreamInfo& stream
   return hostname_fmt_->format(*Http::StaticEmptyHeaders::get().request_headers,
                                *Http::StaticEmptyHeaders::get().response_headers,
                                *Http::StaticEmptyHeaders::get().response_trailers, stream_info,
-                               absl::string_view());
+                               absl::string_view(), AccessLog::AccessLogType::NotSet);
 }
 
 void TunnelingConfigHelperImpl::propagateResponseHeaders(

diff --git a/source/extensions/access_loggers/open_telemetry/substitution_formatter.h b/source/extensions/access_loggers/open_telemetry/substitution_formatter.h
@@ -33,7 +33,7 @@ class OpenTelemetryFormatter {
          const Http::ResponseHeaderMap& response_headers,
          const Http::ResponseTrailerMap& response_trailers,
          const StreamInfo::StreamInfo& stream_info, absl::string_view local_reply_body,
-         AccessLog::AccessLogType access_log_type = AccessLog::AccessLogType::NotSet) const;
+         AccessLog::AccessLogType access_log_type) const;
 
 private:
   struct OpenTelemetryFormatMapWrapper;
@@ -75,13 +75,13 @@ class OpenTelemetryFormatter {
   };
 
   // Methods for doing the actual formatting.
-  ::opentelemetry::proto::common::v1::AnyValue providersCallback(
-      const std::vector<Formatter::FormatterProviderPtr>& providers,
-      const Http::RequestHeaderMap& request_headers,
-      const Http::ResponseHeaderMap& response_headers,
-      const Http::ResponseTrailerMap& response_trailers, const StreamInfo::StreamInfo& stream_info,
-      absl::string_view local_reply_body,
-      AccessLog::AccessLogType access_log_type = AccessLog::AccessLogType::NotSet) const;
+  ::opentelemetry::proto::common::v1::AnyValue
+  providersCallback(const std::vector<Formatter::FormatterProviderPtr>& providers,
+                    const Http::RequestHeaderMap& request_headers,
+                    const Http::ResponseHeaderMap& response_headers,
+                    const Http::ResponseTrailerMap& response_trailers,
+                    const StreamInfo::StreamInfo& stream_info, absl::string_view local_reply_body,
+                    AccessLog::AccessLogType access_log_type) const;
   ::opentelemetry::proto::common::v1::AnyValue openTelemetryFormatMapCallback(
       const OpenTelemetryFormatter::OpenTelemetryFormatMapWrapper& format_map,
       const OpenTelemetryFormatMapVisitor& visitor) const;

diff --git a/source/extensions/filters/http/oauth2/filter.cc b/source/extensions/filters/http/oauth2/filter.cc
@@ -349,9 +349,9 @@ Http::FilterHeadersStatus OAuth2Filter::decodeHeaders(Http::RequestHeaderMap& he
   }
 
   Formatter::FormatterImpl formatter(config_->redirectUri());
-  const auto redirect_uri = formatter.format(headers, *Http::ResponseHeaderMapImpl::create(),
-                                             *Http::ResponseTrailerMapImpl::create(),
-                                             decoder_callbacks_->streamInfo(), "");
+  const auto redirect_uri = formatter.format(
+      headers, *Http::ResponseHeaderMapImpl::create(), *Http::ResponseTrailerMapImpl::create(),
+      decoder_callbacks_->streamInfo(), "", AccessLog::AccessLogType::NotSet);
   oauth_client_->asyncGetAccessToken(auth_code_, config_->clientId(), config_->clientSecret(),
                                      redirect_uri, config_->authType());
 
@@ -405,9 +405,9 @@ void OAuth2Filter::redirectToOAuthServer(Http::RequestHeaderMap& headers) const
           : Http::Utility::PercentEncoding::encode(state_path, ":/=&?");
 
   Formatter::FormatterImpl formatter(config_->redirectUri());
-  const auto redirect_uri = formatter.format(headers, *Http::ResponseHeaderMapImpl::create(),
-                                             *Http::ResponseTrailerMapImpl::create(),
-                                             decoder_callbacks_->streamInfo(), "");
+  const auto redirect_uri = formatter.format(
+      headers, *Http::ResponseHeaderMapImpl::create(), *Http::ResponseTrailerMapImpl::create(),
+      decoder_callbacks_->streamInfo(), "", AccessLog::AccessLogType::NotSet);
   const std::string escaped_redirect_uri =
       Runtime::runtimeFeatureEnabled("envoy.reloadable_features.oauth_use_url_encoding")
           ? Http::Utility::PercentEncoding::urlEncodeQueryParameter(redirect_uri)

diff --git a/source/extensions/filters/network/ratelimit/ratelimit.cc b/source/extensions/filters/network/ratelimit/ratelimit.cc
@@ -50,7 +50,8 @@ Config::applySubstitutionFormatter(StreamInfo::StreamInfo& stream_info) {
 
       std::string value = descriptor_entry.value_;
       value = formatter_it->get()->format(*request_headers_.get(), *response_headers_.get(),
-                                          *response_trailers_.get(), stream_info, value);
+                                          *response_trailers_.get(), stream_info, value,
+                                          AccessLog::AccessLogType::NotSet);
       formatter_it++;
       new_descriptor.entries_.push_back({descriptor_entry.key_, value});
     }

diff --git a/source/extensions/filters/network/redis_proxy/router_impl.cc b/source/extensions/filters/network/redis_proxy/router_impl.cc
@@ -110,11 +110,11 @@ void PrefixRoutes::formatKey(std::string& key, std::string redis_key_formatter)
   auto providers = Formatter::SubstitutionFormatParser::parse(redis_key_formatter);
   std::string formatted_key;
   for (Formatter::FormatterProviderPtr& provider : providers) {
-    auto provider_formatted_key =
-        provider->formatValue(*Http::StaticEmptyHeaders::get().request_headers,
-                              *Http::StaticEmptyHeaders::get().response_headers,
-                              *Http::StaticEmptyHeaders::get().response_trailers,
-                              callbacks_->connection().streamInfo(), absl::string_view());
+    auto provider_formatted_key = provider->formatValue(
+        *Http::StaticEmptyHeaders::get().request_headers,
+        *Http::StaticEmptyHeaders::get().response_headers,
+        *Http::StaticEmptyHeaders::get().response_trailers, callbacks_->connection().streamInfo(),
+        absl::string_view(), AccessLog::AccessLogType::NotSet);
 
     if (provider_formatted_key.has_string_value()) {
       formatted_key = formatted_key + provider_formatted_key.string_value();

diff --git a/source/extensions/formatter/req_without_query/req_without_query.cc b/source/extensions/formatter/req_without_query/req_without_query.cc
@@ -29,8 +29,7 @@ ReqWithoutQuery::ReqWithoutQuery(const std::string& main_header,
 absl::optional<std::string>
 ReqWithoutQuery::format(const Http::RequestHeaderMap& request, const Http::ResponseHeaderMap&,
                         const Http::ResponseTrailerMap&, const StreamInfo::StreamInfo&,
-                        absl::string_view,
-                        AccessLog::AccessLogType = AccessLog::AccessLogType::NotSet) const {
+                        absl::string_view, AccessLog::AccessLogType) const {
   const Http::HeaderEntry* header = findHeader(request);
   if (!header) {
     return absl::nullopt;
@@ -42,11 +41,11 @@ ReqWithoutQuery::format(const Http::RequestHeaderMap& request, const Http::Respo
   return val;
 }
 
-ProtobufWkt::Value
-ReqWithoutQuery::formatValue(const Http::RequestHeaderMap& request, const Http::ResponseHeaderMap&,
-                             const Http::ResponseTrailerMap&, const StreamInfo::StreamInfo&,
-                             absl::string_view,
-                             AccessLog::AccessLogType = AccessLog::AccessLogType::NotSet) const {
+ProtobufWkt::Value ReqWithoutQuery::formatValue(const Http::RequestHeaderMap& request,
+                                                const Http::ResponseHeaderMap&,
+                                                const Http::ResponseTrailerMap&,
+                                                const StreamInfo::StreamInfo&, absl::string_view,
+                                                AccessLog::AccessLogType) const {
   const Http::HeaderEntry* header = findHeader(request);
   if (!header) {
     return ValueUtil::nullValue();

diff --git a/source/extensions/http/custom_response/local_response_policy/local_response_policy.cc b/source/extensions/http/custom_response/local_response_policy/local_response_policy.cc
@@ -45,8 +45,8 @@ void LocalResponsePolicy::formatBody(const Envoy::Http::RequestHeaderMap& reques
 
   if (formatter_) {
     formatter_->format(request_headers, response_headers,
-                       *Envoy::Http::StaticEmptyHeaders::get().response_trailers, stream_info,
-                       body);
+                       *Envoy::Http::StaticEmptyHeaders::get().response_trailers, stream_info, body,
+                       AccessLog::AccessLogType::NotSet);
   }
 }
 

diff --git a/source/extensions/matching/actions/format_string/config.cc b/source/extensions/matching/actions/format_string/config.cc
@@ -15,10 +15,10 @@ namespace FormatString {
 const Network::FilterChain*
 ActionImpl::get(const Server::Configuration::FilterChainsByName& filter_chains_by_name,
                 const StreamInfo::StreamInfo& info) const {
-  const std::string name =
-      formatter_->format(*Http::StaticEmptyHeaders::get().request_headers,
-                         *Http::StaticEmptyHeaders::get().response_headers,
-                         *Http::StaticEmptyHeaders::get().response_trailers, info, "");
+  const std::string name = formatter_->format(*Http::StaticEmptyHeaders::get().request_headers,
+                                              *Http::StaticEmptyHeaders::get().response_headers,
+                                              *Http::StaticEmptyHeaders::get().response_trailers,
+                                              info, "", AccessLog::AccessLogType::NotSet);
   const auto chain_match = filter_chains_by_name.find(name);
   if (chain_match != filter_chains_by_name.end()) {
     return chain_match->second.get();

diff --git a/test/common/formatter/command_extension.h b/test/common/formatter/command_extension.h
@@ -13,14 +13,12 @@ namespace Formatter {
 class TestFormatter : public FormatterProvider {
 public:
   // FormatterProvider
-  absl::optional<std::string>
-  format(const Http::RequestHeaderMap&, const Http::ResponseHeaderMap&,
-         const Http::ResponseTrailerMap&, const StreamInfo::StreamInfo&, absl::string_view,
-         AccessLog::AccessLogType = AccessLog::AccessLogType::NotSet) const override;
-  ProtobufWkt::Value
-  formatValue(const Http::RequestHeaderMap&, const Http::ResponseHeaderMap&,
-              const Http::ResponseTrailerMap&, const StreamInfo::StreamInfo&, absl::string_view,
-              AccessLog::AccessLogType = AccessLog::AccessLogType::NotSet) const override;
+  absl::optional<std::string> format(const Http::RequestHeaderMap&, const Http::ResponseHeaderMap&,
+                                     const Http::ResponseTrailerMap&, const StreamInfo::StreamInfo&,
+                                     absl::string_view, AccessLog::AccessLogType) const override;
+  ProtobufWkt::Value formatValue(const Http::RequestHeaderMap&, const Http::ResponseHeaderMap&,
+                                 const Http::ResponseTrailerMap&, const StreamInfo::StreamInfo&,
+                                 absl::string_view, AccessLog::AccessLogType) const override;
 };
 
 class TestCommandParser : public CommandParser {
@@ -40,14 +38,12 @@ class TestCommandFactory : public CommandParserFactory {
 class AdditionalFormatter : public FormatterProvider {
 public:
   // FormatterProvider
-  absl::optional<std::string>
-  format(const Http::RequestHeaderMap&, const Http::ResponseHeaderMap&,
-         const Http::ResponseTrailerMap&, const StreamInfo::StreamInfo&, absl::string_view,
-         AccessLog::AccessLogType = AccessLog::AccessLogType::NotSet) const override;
-  ProtobufWkt::Value
-  formatValue(const Http::RequestHeaderMap&, const Http::ResponseHeaderMap&,
-              const Http::ResponseTrailerMap&, const StreamInfo::StreamInfo&, absl::string_view,
-              AccessLog::AccessLogType = AccessLog::AccessLogType::NotSet) const override;
+  absl::optional<std::string> format(const Http::RequestHeaderMap&, const Http::ResponseHeaderMap&,
+                                     const Http::ResponseTrailerMap&, const StreamInfo::StreamInfo&,
+                                     absl::string_view, AccessLog::AccessLogType) const override;
+  ProtobufWkt::Value formatValue(const Http::RequestHeaderMap&, const Http::ResponseHeaderMap&,
+                                 const Http::ResponseTrailerMap&, const StreamInfo::StreamInfo&,
+                                 absl::string_view, AccessLog::AccessLogType) const override;
 };
 
 class AdditionalCommandParser : public CommandParser {

diff --git a/test/common/formatter/substitution_format_string_test.cc b/test/common/formatter/substitution_format_string_test.cc
@@ -51,7 +51,7 @@ TEST_F(SubstitutionFormatStringUtilsTest, TestFromProtoConfigText) {
   auto formatter = SubstitutionFormatStringUtils::fromProtoConfig(config_, context_);
   EXPECT_EQ("plain text, path=/bar/foo, code=200",
             formatter->format(request_headers_, response_headers_, response_trailers_, stream_info_,
-                              body_));
+                              body_, AccessLog::AccessLogType::NotSet));
 }
 
 TEST_F(SubstitutionFormatStringUtilsTest, TestFromProtoConfigJson) {
@@ -67,7 +67,7 @@ TEST_F(SubstitutionFormatStringUtilsTest, TestFromProtoConfigJson) {
 
   auto formatter = SubstitutionFormatStringUtils::fromProtoConfig(config_, context_);
   const auto out_json = formatter->format(request_headers_, response_headers_, response_trailers_,
-                                          stream_info_, body_);
+                                          stream_info_, body_, AccessLog::AccessLogType::NotSet);
 
   const std::string expected = R"EOF({
     "text": "plain text",
@@ -111,8 +111,9 @@ TEST_F(SubstitutionFormatStringUtilsTest, TestFromProtoConfigFormatterExtension)
   TestUtility::loadFromYaml(yaml, config_);
 
   auto formatter = SubstitutionFormatStringUtils::fromProtoConfig(config_, context_);
-  EXPECT_EQ("plain text TestFormatter", formatter->format(request_headers_, response_headers_,
-                                                          response_trailers_, stream_info_, body_));
+  EXPECT_EQ("plain text TestFormatter",
+            formatter->format(request_headers_, response_headers_, response_trailers_, stream_info_,
+                              body_, AccessLog::AccessLogType::NotSet));
 }
 
 TEST_F(SubstitutionFormatStringUtilsTest,
@@ -171,7 +172,7 @@ TEST_F(SubstitutionFormatStringUtilsTest, TestFromProtoConfigJsonWithExtension)
 
   auto formatter = SubstitutionFormatStringUtils::fromProtoConfig(config_, context_);
   const auto out_json = formatter->format(request_headers_, response_headers_, response_trailers_,
-                                          stream_info_, body_);
+                                          stream_info_, body_, AccessLog::AccessLogType::NotSet);
 
   const std::string expected = R"EOF({
     "text": "plain text TestFormatter",
@@ -207,7 +208,7 @@ TEST_F(SubstitutionFormatStringUtilsTest, TestFromProtoConfigJsonWithMultipleExt
 
   auto formatter = SubstitutionFormatStringUtils::fromProtoConfig(config_, context_);
   const auto out_json = formatter->format(request_headers_, response_headers_, response_trailers_,
-                                          stream_info_, body_);
+                                          stream_info_, body_, AccessLog::AccessLogType::NotSet);
 
   const std::string expected = R"EOF({
     "text": "plain text TestFormatter",

diff --git a/test/common/formatter/substitution_formatter_fuzz_test.cc b/test/common/formatter/substitution_formatter_fuzz_test.cc
@@ -24,7 +24,7 @@ DEFINE_PROTO_FUZZER(const test::common::substitution::TestCase& input) {
         Fuzz::fromStreamInfo(input.stream_info(), time_system);
     for (const auto& it : formatters) {
       it->format(request_headers, response_headers, response_trailers, *stream_info,
-                 absl::string_view());
+                 absl::string_view(), AccessLog::AccessLogType::NotSet);
     }
     ENVOY_LOG_MISC(trace, "Success");
   } catch (const EnvoyException& e) {