diff --git a/source/extensions/filters/network/generic_proxy/BUILD b/source/extensions/filters/network/generic_proxy/BUILD index c4759e9bf8f25..7a81689cea4b2 100644 --- a/source/extensions/filters/network/generic_proxy/BUILD +++ b/source/extensions/filters/network/generic_proxy/BUILD @@ -23,11 +23,13 @@ envoy_cc_library( ":route_lib", ":stats_lib", ":tracing_lib", + "//envoy/access_log:access_log_interface", "//envoy/network:filter_interface", "//envoy/server:factory_context_interface", "//envoy/stats:timespan_interface", "//source/common/common:linked_object", "//source/common/common:minimal_logger_lib", + "//source/common/formatter:substitution_formatter_lib", "//source/common/stats:timespan_lib", "//source/common/stream_info:stream_info_lib", "//source/common/tracing:tracer_config_lib", @@ -146,20 +148,6 @@ envoy_cc_library( ], ) -envoy_cc_library( - name = "file_access_log_lib", - hdrs = [ - "file_access_log.h", - ], - deps = [ - "//envoy/access_log:access_log_config_interface", - "//envoy/access_log:access_log_interface", - "//source/common/common:utility_lib", - "//source/common/formatter:substitution_format_string_lib", - "@envoy_api//envoy/extensions/access_loggers/file/v3:pkg_cc_proto", - ], -) - envoy_cc_library( name = "access_log_lib", srcs = [ @@ -169,8 +157,12 @@ envoy_cc_library( "access_log.h", ], deps = [ - ":file_access_log_lib", + "//envoy/access_log:access_log_config_interface", + "//envoy/formatter:substitution_formatter_interface", + "//source/common/config:utility_lib", "//source/extensions/filters/network/generic_proxy/interface:stream_interface", + "@envoy_api//envoy/extensions/access_loggers/file/v3:pkg_cc_proto", + "@envoy_api//envoy/extensions/filters/network/generic_proxy/v3:pkg_cc_proto", ], # Ensure this factory in the source is always linked in. alwayslink = 1, @@ -231,6 +223,7 @@ envoy_cc_library( ], deps = [ ":route_interface", + "//envoy/access_log:access_log_config_interface", "//envoy/tracing:trace_config_interface", "//envoy/tracing:tracer_interface", "//source/extensions/filters/network/generic_proxy:access_log_lib", diff --git a/source/extensions/filters/network/generic_proxy/access_log.cc b/source/extensions/filters/network/generic_proxy/access_log.cc index a96df953494d8..b946f30275620 100644 --- a/source/extensions/filters/network/generic_proxy/access_log.cc +++ b/source/extensions/filters/network/generic_proxy/access_log.cc @@ -1,7 +1,10 @@ #include "source/extensions/filters/network/generic_proxy/access_log.h" +#include "envoy/extensions/filters/network/generic_proxy/v3/generic_proxy.pb.h" #include "envoy/registry/registry.h" +#include "source/common/config/utility.h" + namespace Envoy { namespace Extensions { namespace NetworkFilters { @@ -29,26 +32,20 @@ ProtobufWkt::Value StringValueFormatterProvider::formatValueWithContext( absl::optional GenericStatusCodeFormatterProvider::formatWithContext(const FormatterContext& context, const StreamInfo::StreamInfo&) const { - if (context.response_ == nullptr) { - return absl::nullopt; - } - - const int code = context.response_->status().code(); + CHECK_DATA_OR_RETURN(context, response_, absl::nullopt); + const int code = checked_data->response_->status().code(); return std::to_string(code); } ProtobufWkt::Value GenericStatusCodeFormatterProvider::formatValueWithContext(const FormatterContext& context, const StreamInfo::StreamInfo&) const { - if (context.response_ == nullptr) { - return ValueUtil::nullValue(); - } - - const int code = context.response_->status().code(); + CHECK_DATA_OR_RETURN(context, response_, ValueUtil::nullValue()); + const int code = checked_data->response_->status().code(); return ValueUtil::numberValue(code); } -class SimpleCommandParser : public CommandParser { +class GenericProxyCommandParser : public Formatter::CommandParser { public: using ProviderFunc = std::function max_length)>; @@ -75,10 +72,8 @@ class SimpleCommandParser : public CommandParser { return std::make_unique( [](const FormatterContext& context, const StreamInfo::StreamInfo&) -> absl::optional { - if (context.request_) { - return std::string(context.request_->method()); - } - return absl::nullopt; + CHECK_DATA_OR_RETURN(context, request_, absl::nullopt); + return std::string(checked_data->request_->method()); }); }}, {"HOST", @@ -86,10 +81,8 @@ class SimpleCommandParser : public CommandParser { return std::make_unique( [](const FormatterContext& context, const StreamInfo::StreamInfo&) -> absl::optional { - if (context.request_) { - return std::string(context.request_->host()); - } - return absl::nullopt; + CHECK_DATA_OR_RETURN(context, request_, absl::nullopt); + return std::string(checked_data->request_->host()); }); }}, {"PATH", @@ -97,10 +90,8 @@ class SimpleCommandParser : public CommandParser { return std::make_unique( [](const FormatterContext& context, const StreamInfo::StreamInfo&) -> absl::optional { - if (context.request_) { - return std::string(context.request_->path()); - } - return absl::nullopt; + CHECK_DATA_OR_RETURN(context, request_, absl::nullopt); + return std::string(checked_data->request_->path()); }); }}, {"PROTOCOL", @@ -108,10 +99,8 @@ class SimpleCommandParser : public CommandParser { return std::make_unique( [](const FormatterContext& context, const StreamInfo::StreamInfo&) -> absl::optional { - if (context.request_) { - return std::string(context.request_->protocol()); - } - return absl::nullopt; + CHECK_DATA_OR_RETURN(context, request_, absl::nullopt); + return std::string(checked_data->request_->protocol()); }); }}, {"REQUEST_PROPERTY", @@ -120,10 +109,9 @@ class SimpleCommandParser : public CommandParser { [key = std::string(command_arg)]( const FormatterContext& context, const StreamInfo::StreamInfo&) -> absl::optional { - if (!context.request_) { - return absl::nullopt; - } - auto optional_view = context.request_->get(key); + CHECK_DATA_OR_RETURN(context, request_, absl::nullopt); + + auto optional_view = checked_data->request_->get(key); if (!optional_view.has_value()) { return absl::nullopt; } @@ -136,10 +124,8 @@ class SimpleCommandParser : public CommandParser { [key = std::string(command_arg)]( const FormatterContext& context, const StreamInfo::StreamInfo&) -> absl::optional { - if (!context.response_) { - return absl::nullopt; - } - auto optional_view = context.response_->get(key); + CHECK_DATA_OR_RETURN(context, response_, absl::nullopt); + auto optional_view = checked_data->response_->get(key); if (!optional_view.has_value()) { return absl::nullopt; } @@ -154,18 +140,9 @@ class SimpleCommandParser : public CommandParser { } }; -class DefaultBuiltInCommandParserFactory : public BuiltInCommandParserFactory { -public: - std::string name() const override { return "envoy.built_in_formatters.generic_poxy.default"; } - - // BuiltInCommandParserFactory - CommandParserPtr createCommandParser() const override { - return std::make_unique(); - } -}; - -REGISTER_FACTORY(DefaultBuiltInCommandParserFactory, BuiltInCommandParserFactory); -REGISTER_FACTORY(FileAccessLogFactory, AccessLogInstanceFactory); +Formatter::CommandParserPtr createGenericProxyCommandParser() { + return std::make_unique(); +} } // namespace GenericProxy } // namespace NetworkFilters diff --git a/source/extensions/filters/network/generic_proxy/access_log.h b/source/extensions/filters/network/generic_proxy/access_log.h index 99c69fc567f65..9ebb528789109 100644 --- a/source/extensions/filters/network/generic_proxy/access_log.h +++ b/source/extensions/filters/network/generic_proxy/access_log.h @@ -1,6 +1,9 @@ #pragma once -#include "source/extensions/filters/network/generic_proxy/file_access_log.h" +#include "envoy/access_log/access_log_config.h" +#include "envoy/extensions/access_loggers/file/v3/file.pb.h" +#include "envoy/formatter/substitution_formatter.h" + #include "source/extensions/filters/network/generic_proxy/interface/stream.h" namespace Envoy { @@ -8,34 +11,24 @@ namespace Extensions { namespace NetworkFilters { namespace GenericProxy { -struct FormatterContext { +struct FormatterContextExtension : public Formatter::Context::Extension { + FormatterContextExtension(const RequestHeaderFrame* request, const ResponseHeaderFrame* response) + : request_(request), response_(response) {} + FormatterContextExtension() = default; + const RequestHeaderFrame* request_{}; const ResponseHeaderFrame* response_{}; - - static constexpr absl::string_view category() { return "generic_proxy"; } }; -// Formatter for generic proxy. -using Formatter = Formatter::FormatterBase; -using FormatterProvider = Envoy::Formatter::FormatterProviderBase; -using FormatterProviderPtr = std::unique_ptr; -using CommandParser = Envoy::Formatter::CommandParserBase; -using CommandParserPtr = Envoy::Formatter::CommandParserBasePtr; -using CommandParserFactory = Envoy::Formatter::CommandParserFactoryBase; -using BuiltInCommandParserFactory = - Envoy::Formatter::BuiltInCommandParserFactoryBase; +#define CHECK_DATA_OR_RETURN(context, field, return_value) \ + const auto checked_data = context.typedExtension(); \ + if (!checked_data.has_value() || checked_data->field == nullptr) { \ + return return_value; \ + } -// Access log for generic proxy. -using AccessLogFilter = AccessLog::FilterBase; -using AccessLogFilterPtr = std::unique_ptr; -using AccessLogFilterFactory = AccessLog::ExtensionFilterFactoryBase; -using AccessLogInstance = AccessLog::InstanceBase; -using AccessLogInstanceSharedPtr = std::shared_ptr; -using AccessLogInstanceFactory = AccessLog::AccessLogInstanceFactoryBase; - -// File access log for generic proxy. -using FileAccessLog = FileAccessLogBase; -using FileAccessLogFactory = FileAccessLogFactoryBase; +using FormatterProvider = Formatter::FormatterProvider; +using FormatterProviderPtr = std::unique_ptr; +using FormatterContext = Formatter::Context; class StringValueFormatterProvider : public FormatterProvider { public: @@ -69,6 +62,8 @@ class GenericStatusCodeFormatterProvider : public FormatterProvider { const StreamInfo::StreamInfo&) const override; }; +Formatter::CommandParserPtr createGenericProxyCommandParser(); + } // namespace GenericProxy } // namespace NetworkFilters } // namespace Extensions diff --git a/source/extensions/filters/network/generic_proxy/config.cc b/source/extensions/filters/network/generic_proxy/config.cc index 5daaf4fab5975..a922cb83dbaa3 100644 --- a/source/extensions/filters/network/generic_proxy/config.cc +++ b/source/extensions/filters/network/generic_proxy/config.cc @@ -2,48 +2,15 @@ #include "source/common/access_log/access_log_impl.h" #include "source/common/tracing/tracer_manager_impl.h" +#include "source/extensions/filters/network/generic_proxy/access_log.h" #include "source/extensions/filters/network/generic_proxy/rds.h" #include "source/extensions/filters/network/generic_proxy/rds_impl.h" -#include "access_log.h" - namespace Envoy { namespace Extensions { namespace NetworkFilters { namespace GenericProxy { -template -static AccessLog::FilterBasePtr -accessLogFilterFromProto(const envoy::config::accesslog::v3::AccessLogFilter& config, - Server::Configuration::FactoryContext& context) { - if (!config.has_extension_filter()) { - ExceptionUtil::throwEnvoyException( - "Access log filter: only extension filter is supported by non-HTTP access loggers."); - } - - auto& factory = - Config::Utility::getAndCheckFactory>( - config.extension_filter()); - return factory.createFilter(config.extension_filter(), context); -} - -template -static AccessLog::InstanceBaseSharedPtr -accessLoggerFromProto(const envoy::config::accesslog::v3::AccessLog& config, - Server::Configuration::FactoryContext& context) { - AccessLog::FilterBasePtr filter; - if (config.has_filter()) { - filter = accessLogFilterFromProto(config.filter(), context); - } - - auto& factory = - Config::Utility::getAndCheckFactory>( - config); - ProtobufTypes::MessagePtr message = Config::Utility::translateToFactoryConfig( - config, context.messageValidationVisitor(), factory); - - return factory.createAccessLogInstance(*message, std::move(filter), context); -} SINGLETON_MANAGER_REGISTRATION(generic_route_config_provider_manager); SINGLETON_MANAGER_REGISTRATION(generic_proxy_code_or_flag_stats); @@ -156,10 +123,12 @@ Factory::createFilterFactoryFromProtoTyped(const ProxyConfig& proto_config, } // Access log configuration. - std::vector access_logs; + std::vector access_logs; for (const auto& access_log : proto_config.access_log()) { - AccessLogInstanceSharedPtr current_access_log = - accessLoggerFromProto(access_log, context); + std::vector command_parsers; + command_parsers.push_back(createGenericProxyCommandParser()); + AccessLog::InstanceSharedPtr current_access_log = + AccessLog::AccessLogFactory::fromProto(access_log, context, std::move(command_parsers)); access_logs.push_back(current_access_log); } diff --git a/source/extensions/filters/network/generic_proxy/file_access_log.h b/source/extensions/filters/network/generic_proxy/file_access_log.h deleted file mode 100644 index 4110fc9e793af..0000000000000 --- a/source/extensions/filters/network/generic_proxy/file_access_log.h +++ /dev/null @@ -1,123 +0,0 @@ -#pragma once - -#include "envoy/access_log/access_log.h" -#include "envoy/access_log/access_log_config.h" -#include "envoy/extensions/access_loggers/file/v3/file.pb.h" -#include "envoy/extensions/access_loggers/file/v3/file.pb.validate.h" - -#include "source/common/common/utility.h" -#include "source/common/formatter/substitution_format_string.h" - -namespace Envoy { -namespace Extensions { -namespace NetworkFilters { -namespace GenericProxy { - -template class FileAccessLogBase : public AccessLog::InstanceBase { -public: - FileAccessLogBase(const Filesystem::FilePathAndType& access_log_file_info, - AccessLog::FilterBasePtr&& filter, - Formatter::FormatterBasePtr&& formatter, - AccessLog::AccessLogManager& log_manager) - : filter_(std::move(filter)), formatter_(std::move(formatter)) { - log_file_ = log_manager.createAccessLog(access_log_file_info).value(); - } - - void log(const Context& context, const StreamInfo::StreamInfo& stream_info) override { - if (filter_ != nullptr && !filter_->evaluate(context, stream_info)) { - return; - } - log_file_->write(formatter_->formatWithContext(context, stream_info)); - } - -private: - AccessLog::AccessLogFileSharedPtr log_file_; - AccessLog::FilterBasePtr filter_; - Formatter::FormatterBasePtr formatter_; -}; - -template -class FileAccessLogFactoryBase : public AccessLog::AccessLogInstanceFactoryBase { -public: - FileAccessLogFactoryBase() - : name_(fmt::format("envoy.{}.access_loggers.file", Context::category())) {} - - AccessLog::InstanceBaseSharedPtr createAccessLogInstance( - const Protobuf::Message& config, AccessLog::FilterBasePtr&& filter, - Server::Configuration::FactoryContext& context, - std::vector>&& command_parsers = {}) override { - const auto& typed_config = MessageUtil::downcastAndValidate< - const envoy::extensions::access_loggers::file::v3::FileAccessLog&>( - config, context.messageValidationVisitor()); - - Formatter::FormatterBasePtr formatter; - - switch (typed_config.access_log_format_case()) { - case envoy::extensions::access_loggers::file::v3::FileAccessLog::AccessLogFormatCase::kFormat: - if (typed_config.format().empty()) { - formatter = getDefaultFormatter(); - } else { - envoy::config::core::v3::SubstitutionFormatString sff_config; - sff_config.mutable_text_format_source()->set_inline_string(typed_config.format()); - formatter = THROW_OR_RETURN_VALUE( - Formatter::SubstitutionFormatStringUtils::fromProtoConfig( - sff_config, context, std::move(command_parsers)), - Formatter::FormatterBasePtr); - } - break; - case envoy::extensions::access_loggers::file::v3::FileAccessLog::AccessLogFormatCase:: - kJsonFormat: - formatter = Formatter::SubstitutionFormatStringUtils::createJsonFormatter( - typed_config.json_format(), false, false, false, command_parsers); - break; - case envoy::extensions::access_loggers::file::v3::FileAccessLog::AccessLogFormatCase:: - kTypedJsonFormat: { - envoy::config::core::v3::SubstitutionFormatString sff_config; - *sff_config.mutable_json_format() = typed_config.typed_json_format(); - formatter = - THROW_OR_RETURN_VALUE(Formatter::SubstitutionFormatStringUtils::fromProtoConfig( - sff_config, context, std::move(command_parsers)), - Formatter::FormatterBasePtr); - break; - } - case envoy::extensions::access_loggers::file::v3::FileAccessLog::AccessLogFormatCase:: - kLogFormat: - formatter = - THROW_OR_RETURN_VALUE(Formatter::SubstitutionFormatStringUtils::fromProtoConfig( - typed_config.log_format(), context, std::move(command_parsers)), - Formatter::FormatterBasePtr); - break; - case envoy::extensions::access_loggers::file::v3::FileAccessLog::AccessLogFormatCase:: - ACCESS_LOG_FORMAT_NOT_SET: - formatter = getDefaultFormatter(); - break; - } - if (formatter == nullptr) { - ExceptionUtil::throwEnvoyException( - "Access log: no format and no default format for file access log"); - } - - Filesystem::FilePathAndType file_info{Filesystem::DestinationType::File, typed_config.path()}; - return std::make_shared>( - file_info, std::move(filter), std::move(formatter), - context.serverFactoryContext().accessLogManager()); - } - - ProtobufTypes::MessagePtr createEmptyConfigProto() override { - return ProtobufTypes::MessagePtr{ - new envoy::extensions::access_loggers::file::v3::FileAccessLog()}; - } - - std::string name() const override { return name_; } - -protected: - virtual Formatter::FormatterBasePtr getDefaultFormatter() const { return nullptr; } - -private: - const std::string name_; -}; - -} // namespace GenericProxy -} // namespace NetworkFilters -} // namespace Extensions -} // namespace Envoy diff --git a/source/extensions/filters/network/generic_proxy/proxy.cc b/source/extensions/filters/network/generic_proxy/proxy.cc index fad6c85444dbc..9edccec994006 100644 --- a/source/extensions/filters/network/generic_proxy/proxy.cc +++ b/source/extensions/filters/network/generic_proxy/proxy.cc @@ -6,6 +6,7 @@ #include "envoy/network/connection.h" #include "source/common/config/utility.h" +#include "source/common/formatter/substitution_formatter.h" #include "source/common/protobuf/protobuf.h" #include "source/common/stream_info/stream_info_impl.h" #include "source/extensions/filters/network/generic_proxy/interface/filter.h" @@ -625,8 +626,10 @@ void ActiveStream::completeStream(absl::optional re } for (const auto& access_log : parent_.config_->accessLogs()) { - const FormatterContext context{request_header_frame_.get(), response_header_frame_.get()}; - access_log->log(context, stream_info_); + const FormatterContextExtension context_extension(request_header_frame_.get(), + response_header_frame_.get()); + Formatter::Context context; + access_log->log(context.setExtension(context_extension), stream_info_); } // TODO(wbpcode): use ranges to simplify the code. diff --git a/source/extensions/filters/network/generic_proxy/proxy.h b/source/extensions/filters/network/generic_proxy/proxy.h index 27c1f63b9d587..4a80b98aa30ac 100644 --- a/source/extensions/filters/network/generic_proxy/proxy.h +++ b/source/extensions/filters/network/generic_proxy/proxy.h @@ -4,6 +4,7 @@ #include #include +#include "envoy/access_log/access_log.h" #include "envoy/config/core/v3/extension.pb.h" #include "envoy/extensions/filters/network/generic_proxy/v3/generic_proxy.pb.h" #include "envoy/extensions/filters/network/generic_proxy/v3/generic_proxy.pb.validate.h" @@ -63,7 +64,7 @@ class FilterConfigImpl : public FilterConfig { Rds::RouteConfigProviderSharedPtr route_config_provider, std::vector factories, Tracing::TracerSharedPtr tracer, Tracing::ConnectionManagerTracingConfigPtr tracing_config, - std::vector&& access_logs, + AccessLog::InstanceSharedPtrVector&& access_logs, const CodeOrFlags& code_or_flags, Envoy::Server::Configuration::FactoryContext& context) : stat_prefix_(stat_prefix), @@ -89,9 +90,7 @@ class FilterConfigImpl : public FilterConfig { } GenericFilterStats& stats() override { return stats_; } const CodeOrFlags& codeOrFlags() const override { return code_or_flags_; } - const std::vector& accessLogs() const override { - return access_logs_; - } + const AccessLog::InstanceSharedPtrVector& accessLogs() const override { return access_logs_; } // FilterChainFactory void createFilterChain(FilterChainManager& manager) override { @@ -118,7 +117,7 @@ class FilterConfigImpl : public FilterConfig { Tracing::TracerSharedPtr tracer_; Tracing::ConnectionManagerTracingConfigPtr tracing_config_; - std::vector access_logs_; + std::vector access_logs_; TimeSource& time_source_; }; diff --git a/source/extensions/filters/network/generic_proxy/proxy_config.h b/source/extensions/filters/network/generic_proxy/proxy_config.h index a91a35068dd67..5b754c9bfec69 100644 --- a/source/extensions/filters/network/generic_proxy/proxy_config.h +++ b/source/extensions/filters/network/generic_proxy/proxy_config.h @@ -1,5 +1,6 @@ #pragma once +#include "envoy/access_log/access_log_config.h" #include "envoy/tracing/trace_config.h" #include "envoy/tracing/tracer.h" @@ -62,7 +63,7 @@ class FilterConfig : public FilterChainFactory { /** * @return const std::vector& access logs. */ - virtual const std::vector& accessLogs() const PURE; + virtual const AccessLog::InstanceSharedPtrVector& accessLogs() const PURE; }; } // namespace GenericProxy diff --git a/test/extensions/filters/network/generic_proxy/BUILD b/test/extensions/filters/network/generic_proxy/BUILD index ac070b2c8b372..1d1a360ea3671 100644 --- a/test/extensions/filters/network/generic_proxy/BUILD +++ b/test/extensions/filters/network/generic_proxy/BUILD @@ -49,8 +49,11 @@ envoy_cc_test( rbe_pool = "6gig", deps = [ ":fake_codec_lib", + "//source/common/access_log:access_log_lib", "//source/common/buffer:buffer_lib", "//source/common/formatter:formatter_extension_lib", + "//source/common/formatter:substitution_format_string_lib", + "//source/extensions/access_loggers/file:config", "//source/extensions/filters/network/generic_proxy:proxy_lib", "//test/extensions/filters/network/generic_proxy/mocks:codec_mocks", "//test/extensions/filters/network/generic_proxy/mocks:filter_mocks", @@ -70,6 +73,7 @@ envoy_cc_test( deps = [ ":fake_codec_lib", "//source/common/buffer:buffer_lib", + "//source/extensions/access_loggers/file:config", "//source/extensions/filters/network/generic_proxy:config", "//source/extensions/filters/network/generic_proxy/router:config", "//source/extensions/tracers/zipkin:config", @@ -140,6 +144,7 @@ envoy_cc_test( rbe_pool = "6gig", deps = [ ":fake_codec_lib", + "//source/common/formatter:substitution_formatter_lib", "//source/extensions/filters/network/generic_proxy:access_log_lib", "//test/mocks/stream_info:stream_info_mocks", ], diff --git a/test/extensions/filters/network/generic_proxy/access_log_test.cc b/test/extensions/filters/network/generic_proxy/access_log_test.cc index 04426bdb0667a..1c694cdb448d1 100644 --- a/test/extensions/filters/network/generic_proxy/access_log_test.cc +++ b/test/extensions/filters/network/generic_proxy/access_log_test.cc @@ -1,3 +1,4 @@ +#include "source/common/formatter/substitution_formatter.h" #include "source/extensions/filters/network/generic_proxy/access_log.h" #include "test/extensions/filters/network/generic_proxy/fake_codec.h" @@ -12,165 +13,203 @@ namespace GenericProxy { namespace { TEST(GenericStatusCodeFormatterProviderTest, GenericStatusCodeFormatterProviderTest) { - FormatterContext context; + FormatterContextExtension context; GenericStatusCodeFormatterProvider formatter; StreamInfo::MockStreamInfo stream_info; - EXPECT_EQ(formatter.formatWithContext(context, stream_info), absl::nullopt); - EXPECT_TRUE(formatter.formatValueWithContext(context, stream_info).has_null_value()); + EXPECT_EQ(formatter.formatWithContext(Formatter::Context().setExtension(context), stream_info), + absl::nullopt); + EXPECT_TRUE( + formatter.formatValueWithContext(Formatter::Context().setExtension(context), stream_info) + .has_null_value()); FakeStreamCodecFactory::FakeResponse response; response.status_ = {1234, false}; context.response_ = &response; - EXPECT_EQ(formatter.formatWithContext(context, stream_info).value(), "1234"); - EXPECT_EQ(formatter.formatValueWithContext(context, stream_info).number_value(), 1234.0); + EXPECT_EQ( + formatter.formatWithContext(Formatter::Context().setExtension(context), stream_info).value(), + "1234"); + EXPECT_EQ( + formatter.formatValueWithContext(Formatter::Context().setExtension(context), stream_info) + .number_value(), + 1234.0); } TEST(StringValueFormatterProviderTest, StringValueFormatterProviderTest) { { - FormatterContext context; + FormatterContextExtension context; StringValueFormatterProvider formatter( - [](const FormatterContext& context, + [](const Formatter::Context& context, const StreamInfo::StreamInfo&) -> absl::optional { - if (context.request_ == nullptr) { - return absl::nullopt; - } - return std::string(context.request_->path()); + CHECK_DATA_OR_RETURN(context, request_, absl::nullopt); + return std::string(checked_data->request_->path()); }, 9); StreamInfo::MockStreamInfo stream_info; - EXPECT_EQ(formatter.formatWithContext(context, stream_info), absl::nullopt); - EXPECT_TRUE(formatter.formatValueWithContext(context, stream_info).has_null_value()); + EXPECT_EQ(formatter.formatWithContext(Formatter::Context().setExtension(context), stream_info), + absl::nullopt); + EXPECT_TRUE( + formatter.formatValueWithContext(Formatter::Context().setExtension(context), stream_info) + .has_null_value()); FakeStreamCodecFactory::FakeRequest request; request.path_ = "ANYTHING"; context.request_ = &request; - EXPECT_EQ(formatter.formatWithContext(context, stream_info).value(), "ANYTHING"); - EXPECT_EQ(formatter.formatValueWithContext(context, stream_info).string_value(), "ANYTHING"); + EXPECT_EQ(formatter.formatWithContext(Formatter::Context().setExtension(context), stream_info) + .value(), + "ANYTHING"); + EXPECT_EQ( + formatter.formatValueWithContext(Formatter::Context().setExtension(context), stream_info) + .string_value(), + "ANYTHING"); request.path_ = "ANYTHING_LONGER_THAN_9"; - EXPECT_EQ(formatter.formatWithContext(context, stream_info).value(), "ANYTHING_"); - EXPECT_EQ(formatter.formatValueWithContext(context, stream_info).string_value(), "ANYTHING_"); + EXPECT_EQ(formatter.formatWithContext(Formatter::Context().setExtension(context), stream_info) + .value(), + "ANYTHING_"); + EXPECT_EQ( + formatter.formatValueWithContext(Formatter::Context().setExtension(context), stream_info) + .string_value(), + "ANYTHING_"); } } TEST(AccessLogFormatterTest, AccessLogFormatterTest) { + std::vector commands; + commands.push_back(createGenericProxyCommandParser()); + { // Test for %METHOD%. - FormatterContext context; - auto formatter = *Envoy::Formatter::FormatterBaseImpl::create("%METHOD%"); + FormatterContextExtension context; + auto formatter = *Envoy::Formatter::FormatterImpl::create("%METHOD%", false, commands); StreamInfo::MockStreamInfo stream_info; - EXPECT_EQ(formatter->formatWithContext(context, stream_info), "-"); + EXPECT_EQ(formatter->formatWithContext(Formatter::Context().setExtension(context), stream_info), + "-"); FakeStreamCodecFactory::FakeRequest request; request.method_ = "FAKE_METHOD"; context.request_ = &request; - EXPECT_EQ(formatter->formatWithContext(context, stream_info), "FAKE_METHOD"); + EXPECT_EQ(formatter->formatWithContext(Formatter::Context().setExtension(context), stream_info), + "FAKE_METHOD"); } { // Test for %HOST%. - FormatterContext context; - auto formatter = *Envoy::Formatter::FormatterBaseImpl::create("%HOST%"); + FormatterContextExtension context; + auto formatter = *Envoy::Formatter::FormatterImpl::create("%HOST%", false, commands); StreamInfo::MockStreamInfo stream_info; - EXPECT_EQ(formatter->formatWithContext(context, stream_info), "-"); + EXPECT_EQ(formatter->formatWithContext(Formatter::Context().setExtension(context), stream_info), + "-"); FakeStreamCodecFactory::FakeRequest request; request.host_ = "FAKE_HOST"; context.request_ = &request; - EXPECT_EQ(formatter->formatWithContext(context, stream_info), "FAKE_HOST"); + EXPECT_EQ(formatter->formatWithContext(Formatter::Context().setExtension(context), stream_info), + "FAKE_HOST"); } { // Test for %PATH%. - FormatterContext context; - auto formatter = *Envoy::Formatter::FormatterBaseImpl::create("%PATH%"); + FormatterContextExtension context; + auto formatter = *Envoy::Formatter::FormatterImpl::create("%PATH%", false, commands); StreamInfo::MockStreamInfo stream_info; - EXPECT_EQ(formatter->formatWithContext(context, stream_info), "-"); + EXPECT_EQ(formatter->formatWithContext(Formatter::Context().setExtension(context), stream_info), + "-"); FakeStreamCodecFactory::FakeRequest request; request.path_ = "FAKE_PATH"; context.request_ = &request; - EXPECT_EQ(formatter->formatWithContext(context, stream_info), "FAKE_PATH"); + EXPECT_EQ(formatter->formatWithContext(Formatter::Context().setExtension(context), stream_info), + "FAKE_PATH"); } { // Test for %PROTOCOL%. - FormatterContext context; - auto formatter = *Envoy::Formatter::FormatterBaseImpl::create("%PROTOCOL%"); + FormatterContextExtension context; + auto formatter = *Envoy::Formatter::FormatterImpl::create("%PROTOCOL%", false, commands); StreamInfo::MockStreamInfo stream_info; - EXPECT_EQ(formatter->formatWithContext(context, stream_info), "-"); + EXPECT_EQ(formatter->formatWithContext(Formatter::Context().setExtension(context), stream_info), + "-"); FakeStreamCodecFactory::FakeRequest request; request.protocol_ = "FAKE_PROTOCOL"; context.request_ = &request; - EXPECT_EQ(formatter->formatWithContext(context, stream_info), "FAKE_PROTOCOL"); + EXPECT_EQ(formatter->formatWithContext(Formatter::Context().setExtension(context), stream_info), + "FAKE_PROTOCOL"); } { // Test for %REQUEST_PROPERTY%. - FormatterContext context; - auto formatter = *Envoy::Formatter::FormatterBaseImpl::create( - "%REQUEST_PROPERTY(FAKE_KEY)%"); + FormatterContextExtension context; + auto formatter = + *Envoy::Formatter::FormatterImpl::create("%REQUEST_PROPERTY(FAKE_KEY)%", false, commands); StreamInfo::MockStreamInfo stream_info; - EXPECT_EQ(formatter->formatWithContext(context, stream_info), "-"); + EXPECT_EQ(formatter->formatWithContext(Formatter::Context().setExtension(context), stream_info), + "-"); FakeStreamCodecFactory::FakeRequest request; context.request_ = &request; - EXPECT_EQ(formatter->formatWithContext(context, stream_info), "-"); + EXPECT_EQ(formatter->formatWithContext(Formatter::Context().setExtension(context), stream_info), + "-"); request.data_["FAKE_KEY"] = "FAKE_VALUE"; - EXPECT_EQ(formatter->formatWithContext(context, stream_info), "FAKE_VALUE"); + EXPECT_EQ(formatter->formatWithContext(Formatter::Context().setExtension(context), stream_info), + "FAKE_VALUE"); } { // Test for %RESPONSE_PROPERTY%. - FormatterContext context; - auto formatter = *Envoy::Formatter::FormatterBaseImpl::create( - "%RESPONSE_PROPERTY(FAKE_KEY)%"); + FormatterContextExtension context; + auto formatter = + *Envoy::Formatter::FormatterImpl::create("%RESPONSE_PROPERTY(FAKE_KEY)%", false, commands); StreamInfo::MockStreamInfo stream_info; - EXPECT_EQ(formatter->formatWithContext(context, stream_info), "-"); + EXPECT_EQ(formatter->formatWithContext(Formatter::Context().setExtension(context), stream_info), + "-"); FakeStreamCodecFactory::FakeResponse response; context.response_ = &response; - EXPECT_EQ(formatter->formatWithContext(context, stream_info), "-"); + EXPECT_EQ(formatter->formatWithContext(Formatter::Context().setExtension(context), stream_info), + "-"); response.data_["FAKE_KEY"] = "FAKE_VALUE"; - EXPECT_EQ(formatter->formatWithContext(context, stream_info), "FAKE_VALUE"); + EXPECT_EQ(formatter->formatWithContext(Formatter::Context().setExtension(context), stream_info), + "FAKE_VALUE"); } { // Test for %GENERIC_RESPONSE_CODE%. - FormatterContext context; + FormatterContextExtension context; auto formatter = - *Envoy::Formatter::FormatterBaseImpl::create("%GENERIC_RESPONSE_CODE%"); + *Envoy::Formatter::FormatterImpl::create("%GENERIC_RESPONSE_CODE%", false, commands); StreamInfo::MockStreamInfo stream_info; - EXPECT_EQ(formatter->formatWithContext(context, stream_info), "-"); + EXPECT_EQ(formatter->formatWithContext(Formatter::Context().setExtension(context), stream_info), + "-"); FakeStreamCodecFactory::FakeResponse response; response.status_ = {-1234, false}; context.response_ = &response; - EXPECT_EQ(formatter->formatWithContext(context, stream_info), "-1234"); + EXPECT_EQ(formatter->formatWithContext(Formatter::Context().setExtension(context), stream_info), + "-1234"); } } diff --git a/test/extensions/filters/network/generic_proxy/config_test.cc b/test/extensions/filters/network/generic_proxy/config_test.cc index 7197e810c3542..d21737c819798 100644 --- a/test/extensions/filters/network/generic_proxy/config_test.cc +++ b/test/extensions/filters/network/generic_proxy/config_test.cc @@ -496,10 +496,11 @@ TEST(BasicFilterConfigTest, TestConfigurationWithAccessLogAndLogFilter1) { EXPECT_CALL(codec_factory_config, createCodecFactory(_, _)) .WillOnce(Return(testing::ByMove(std::move(mock_codec_factory)))); - EXPECT_THROW_WITH_MESSAGE( - { auto status_or = factory.createFilterFactoryFromProto(config, factory_context); }, - EnvoyException, - "Access log filter: only extension filter is supported by non-HTTP access loggers."); + Network::FilterFactoryCb cb = + factory.createFilterFactoryFromProto(config, factory_context).value(); + EXPECT_NE(nullptr, cb); + NiceMock filter_manager; + cb(filter_manager); } TEST(BasicFilterConfigTest, TestConfigurationWithAccessLogAndLogFilter2) { @@ -539,7 +540,7 @@ TEST(BasicFilterConfigTest, TestConfigurationWithAccessLogAndLogFilter2) { Registry::InjectFactory registration(codec_factory_config); FakeAccessLogExtensionFilterFactory fake_access_log_extension_filter_factory; - Registry::InjectFactory registration_log( + Registry::InjectFactory registration_log( fake_access_log_extension_filter_factory); NiceMock factory_context; diff --git a/test/extensions/filters/network/generic_proxy/fake_codec.h b/test/extensions/filters/network/generic_proxy/fake_codec.h index 3c0b25bf9d3b6..1f9cd32f56378 100644 --- a/test/extensions/filters/network/generic_proxy/fake_codec.h +++ b/test/extensions/filters/network/generic_proxy/fake_codec.h @@ -2,6 +2,8 @@ #include +#include "envoy/access_log/access_log_config.h" + #include "source/common/buffer/buffer_impl.h" #include "source/extensions/filters/network/generic_proxy/access_log.h" #include "source/extensions/filters/network/generic_proxy/interface/codec.h" @@ -421,17 +423,17 @@ class FakeStreamCodecFactoryConfig : public CodecFactoryConfig { std::string name() const override { return "envoy.generic_proxy.codecs.fake"; } }; -class FakeAccessLogExtensionFilter : public AccessLogFilter { - bool evaluate(const FormatterContext&, const StreamInfo::StreamInfo&) const override { +class FakeAccessLogExtensionFilter : public AccessLog::Filter { + bool evaluate(const Formatter::Context&, const StreamInfo::StreamInfo&) const override { return true; } }; -class FakeAccessLogExtensionFilterFactory : public AccessLogFilterFactory { +class FakeAccessLogExtensionFilterFactory : public AccessLog::ExtensionFilterFactory { public: // AccessLogFilterFactory - AccessLogFilterPtr createFilter(const envoy::config::accesslog::v3::ExtensionFilter&, - Server::Configuration::FactoryContext&) override { + AccessLog::FilterPtr createFilter(const envoy::config::accesslog::v3::ExtensionFilter&, + Server::Configuration::FactoryContext&) override { return std::make_unique(); } diff --git a/test/extensions/filters/network/generic_proxy/proxy_test.cc b/test/extensions/filters/network/generic_proxy/proxy_test.cc index 31900ba301418..f9addf64e52ae 100644 --- a/test/extensions/filters/network/generic_proxy/proxy_test.cc +++ b/test/extensions/filters/network/generic_proxy/proxy_test.cc @@ -2,7 +2,10 @@ #include #include +#include "source/common/access_log/access_log_impl.h" +#include "source/common/formatter/substitution_format_string.h" #include "source/common/tracing/tracer_manager_impl.h" +#include "source/extensions/access_loggers/common/file_access_log_impl.h" #include "source/extensions/filters/network/generic_proxy/proxy.h" #include "test/extensions/filters/network/generic_proxy/fake_codec.h" @@ -45,7 +48,7 @@ class MockRouteConfigProvider : public Rds::RouteConfigProvider { class FilterConfigTest : public testing::Test { public: - void initializeFilterConfig(bool with_tracing = false, AccessLogInstanceSharedPtr logger = {}, + void initializeFilterConfig(bool with_tracing = false, AccessLog::InstanceSharedPtr logger = {}, bool allow_no_decoder_filter = false) { if (with_tracing) { tracer_ = std::make_shared>(); @@ -93,7 +96,7 @@ class FilterConfigTest : public testing::Test { mock_route_entry_ = std::make_shared>(); - std::vector access_logs; + std::vector access_logs; if (logger) { access_logs.push_back(logger); } @@ -104,16 +107,18 @@ class FilterConfigTest : public testing::Test { factory_context_); } - AccessLogInstanceSharedPtr loggerFormFormat(const std::string& format = DEFAULT_LOG_FORMAT) { - envoy::config::core::v3::SubstitutionFormatString sff_config; - sff_config.mutable_text_format_source()->set_inline_string(format); - auto formatter = - *Envoy::Formatter::SubstitutionFormatStringUtils::fromProtoConfig( - sff_config, factory_context_); - - return std::make_shared( - Filesystem::FilePathAndType{}, nullptr, std::move(formatter), - factory_context_.server_factory_context_.accessLogManager()); + AccessLog::InstanceSharedPtr loggerFormFormat(const std::string& format = DEFAULT_LOG_FORMAT) { + envoy::extensions::access_loggers::file::v3::FileAccessLog file_log_config; + file_log_config.mutable_log_format()->mutable_text_format_source()->set_inline_string(format); + file_log_config.set_path("/fake/log/path"); + envoy::config::accesslog::v3::AccessLog config; + config.mutable_typed_config()->PackFrom(file_log_config); + config.set_name("file"); + + std::vector command_parsers; + command_parsers.push_back(createGenericProxyCommandParser()); + return AccessLog::AccessLogFactory::fromProto(config, factory_context_, + std::move(command_parsers)); } NiceMock factory_context_; @@ -189,7 +194,7 @@ TEST_F(FilterConfigTest, CodecFactory) { class FilterTest : public FilterConfigTest { public: - void initializeFilter(bool with_tracing = false, AccessLogInstanceSharedPtr logger = {}, + void initializeFilter(bool with_tracing = false, AccessLog::InstanceSharedPtr logger = {}, bool allow_no_decoder_filter = false) { FilterConfigTest::initializeFilterConfig(with_tracing, logger, allow_no_decoder_filter); diff --git a/tools/base/requirements.txt b/tools/base/requirements.txt index 5b0faf771eeb8..f7febf2414b98 100644 --- a/tools/base/requirements.txt +++ b/tools/base/requirements.txt @@ -1150,9 +1150,9 @@ pyflakes==3.2.0 \ --hash=sha256:1c61603ff154621fb2a9172037d84dca3500def8c8b630657d1701f026f8af3f \ --hash=sha256:84b5be138a2dfbb40689ca07e2152deb896a65c3a3e24c251c5c62489568074a # via flake8 -pygithub==2.5.0 \ - --hash=sha256:b0b635999a658ab8e08720bdd3318893ff20e2275f6446fcf35bf3f44f2c0fd2 \ - --hash=sha256:e1613ac508a9be710920d26eb18b1905ebd9926aa49398e88151c1b526aad3cf +pygithub==2.6.1 \ + --hash=sha256:6f2fa6d076ccae475f9fc392cc6cdbd54db985d4f69b8833a28397de75ed6ca3 \ + --hash=sha256:b5c035392991cca63959e9453286b41b54d83bf2de2daa7d7ff7e4312cebf3bf # via -r requirements.in pygments==2.18.0 \ --hash=sha256:786ff802f32e91311bff3889f6e9a86e81505fe99f2735bb6d60ae0c5004f199 \