From bfe7744e71d28b89a8c6dae1c394d386063cc252 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Mon, 8 Oct 2018 17:55:22 -0700 Subject: [PATCH 01/51] Initial commit Signed-off-by: Aaltan Ahmad --- api/envoy/config/accesslog/v2/file.proto | 6 +- include/envoy/access_log/access_log.h | 15 ++ .../common/access_log/access_log_formatter.cc | 77 +++++-- .../common/access_log/access_log_formatter.h | 41 +++- source/common/router/header_formatter.h | 2 +- .../extensions/access_loggers/file/config.cc | 28 ++- .../extensions/access_loggers/file/config.h | 3 + .../access_log_formatter_fuzz_test.cc | 2 +- .../access_log/access_log_formatter_test.cc | 188 ++++++++++++++++++ .../common/access_log/access_log_impl_test.cc | 2 +- .../access_loggers/file/config_test.cc | 79 ++++++++ 11 files changed, 413 insertions(+), 30 deletions(-) diff --git a/api/envoy/config/accesslog/v2/file.proto b/api/envoy/config/accesslog/v2/file.proto index d1ca2d1e41514..cb348b88b4d87 100644 --- a/api/envoy/config/accesslog/v2/file.proto +++ b/api/envoy/config/accesslog/v2/file.proto @@ -4,6 +4,7 @@ package envoy.config.accesslog.v2; option go_package = "v2"; import "validate/validate.proto"; +import "google/protobuf/struct.proto"; // [#protodoc-title: File access log] @@ -17,5 +18,8 @@ message FileAccessLog { // Access log format. Envoy supports :ref:`custom access log formats // ` as well as a :ref:`default format // `. - string format = 2; + oneof access_log_format { + string format = 2; + google.protobuf.Struct json_format = 3; + } } diff --git a/include/envoy/access_log/access_log.h b/include/envoy/access_log/access_log.h index f2a86279a06d3..4af48fce96bbe 100644 --- a/include/envoy/access_log/access_log.h +++ b/include/envoy/access_log/access_log.h @@ -84,5 +84,20 @@ class Formatter { typedef std::unique_ptr FormatterPtr; +/** + * Interface for access log provider + */ +class FormatterProvider { +public: + virtual ~FormatterProvider() {} + + virtual std::string format(const Http::HeaderMap& request_headers, + const Http::HeaderMap& response_headers, + const Http::HeaderMap& response_trailers, + const RequestInfo::RequestInfo& request_info) const PURE; +}; + +typedef std::unique_ptr FormatterProviderPtr; + } // namespace AccessLog } // namespace Envoy diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc index d7ee60fe1c01c..74830f5fc9bff 100644 --- a/source/common/access_log/access_log_formatter.cc +++ b/source/common/access_log/access_log_formatter.cc @@ -13,6 +13,8 @@ #include "absl/strings/str_split.h" #include "fmt/format.h" +#include "rapidjson/prettywriter.h" +#include "rapidjson/stringbuffer.h" using Envoy::Config::Metadata; @@ -54,7 +56,7 @@ AccessLogFormatUtils::protocolToString(const absl::optional& pro } FormatterImpl::FormatterImpl(const std::string& format) { - formatters_ = AccessLogFormatParser::parse(format); + providers_ = AccessLogFormatParser::parse(format); } std::string FormatterImpl::format(const Http::HeaderMap& request_headers, @@ -64,14 +66,62 @@ std::string FormatterImpl::format(const Http::HeaderMap& request_headers, std::string log_line; log_line.reserve(256); - for (const FormatterPtr& formatter : formatters_) { + for (const FormatterProviderPtr& provider : providers_) { log_line += - formatter->format(request_headers, response_headers, response_trailers, stream_info); + provider->format(request_headers, response_headers, response_trailers, stream_info); } return log_line; } +JsonFormatterImpl::JsonFormatterImpl(const std::map& format_mapping) { + for (const auto& format_pair : format_mapping) { + auto providers = AccessLogFormatParser::parse(format_pair.second); + + // Enforce that each key only has one format specifier in it + if (providers.size() > 1) { + throw EnvoyException(fmt::format("More than one format specifier was provided in the JSON log format: {}", format_pair.second)); + } + + if (providers.size() < 1) { + throw EnvoyException(fmt::format("No format specifier was provided in the JSON log format: {}", format_pair.second)); + } + + json_output_format_.emplace(format_pair.first, std::move(providers[0])); + } +} + +std::string JsonFormatterImpl::format(const Http::HeaderMap& request_headers, + const Http::HeaderMap& response_headers, + const Http::HeaderMap& response_trailers, + const RequestInfo::RequestInfo& request_info) const { + rapidjson::StringBuffer strbuf; + rapidjson::Writer writer(strbuf); + + writer.StartObject(); + auto output_map = this->to_map(request_headers, response_headers, response_trailers, request_info); + for (auto it = output_map->begin(); it != output_map->end(); ++it) { + writer.Key(it->first.c_str()); + writer.String(it->second.c_str()); + } + writer.EndObject(); + return fmt::format("{}\n", strbuf.GetString()); +} + +std::unique_ptr> +JsonFormatterImpl::to_map(const Http::HeaderMap& request_headers, + const Http::HeaderMap& response_headers, + const Http::HeaderMap& response_trailers, + const RequestInfo::RequestInfo& request_info) const { + std::map output; + for (const auto& pair : json_output_format_) { + output.emplace( + pair.first, + pair.second->format(request_headers, response_headers, response_trailers, request_info)); + } + return std::make_unique>(output); +} + void AccessLogFormatParser::parseCommandHeader(const std::string& token, const size_t start, std::string& main_header, std::string& alternative_header, @@ -131,16 +181,17 @@ void AccessLogFormatParser::parseCommand(const std::string& token, const size_t } // TODO(derekargueta): #2967 - Rewrite AccessLogformatter with parser library & formal grammar -std::vector AccessLogFormatParser::parse(const std::string& format) { +std::vector AccessLogFormatParser::parse(const std::string& format) { std::string current_token; - std::vector formatters; + std::vector formatters; const std::string DYNAMIC_META_TOKEN = "DYNAMIC_METADATA("; const std::regex command_w_args_regex(R"EOF(%([A-Z]|_)+(\([^\)]*\))?(:[0-9]+)?(%))EOF"); for (size_t pos = 0; pos < format.length(); ++pos) { if (format[pos] == '%') { if (!current_token.empty()) { - formatters.emplace_back(new PlainStringFormatter(current_token)); + formatters.emplace_back( + FormatterProviderPtr{new PlainStringFormatter(current_token)}); current_token = ""; } @@ -164,7 +215,7 @@ std::vector AccessLogFormatParser::parse(const std::string& format parseCommandHeader(token, ReqParamStart, main_header, alternative_header, max_length); formatters.emplace_back( - new RequestHeaderFormatter(main_header, alternative_header, max_length)); + FormatterProviderPtr{new RequestHeaderFormatter(main_header, alternative_header, max_length)}); } else if (token.find("RESP(") == 0) { std::string main_header, alternative_header; absl::optional max_length; @@ -172,7 +223,7 @@ std::vector AccessLogFormatParser::parse(const std::string& format parseCommandHeader(token, RespParamStart, main_header, alternative_header, max_length); formatters.emplace_back( - new ResponseHeaderFormatter(main_header, alternative_header, max_length)); + FormatterProviderPtr{new ResponseHeaderFormatter(main_header, alternative_header, max_length)}); } else if (token.find("TRAILER(") == 0) { std::string main_header, alternative_header; absl::optional max_length; @@ -180,7 +231,7 @@ std::vector AccessLogFormatParser::parse(const std::string& format parseCommandHeader(token, TrailParamStart, main_header, alternative_header, max_length); formatters.emplace_back( - new ResponseTrailerFormatter(main_header, alternative_header, max_length)); + FormatterProviderPtr{new ResponseTrailerFormatter(main_header, alternative_header, max_length)}); } else if (token.find(DYNAMIC_META_TOKEN) == 0) { std::string filter_namespace; absl::optional max_length; @@ -188,7 +239,7 @@ std::vector AccessLogFormatParser::parse(const std::string& format const size_t start = DYNAMIC_META_TOKEN.size(); parseCommand(token, start, ":", filter_namespace, path, max_length); - formatters.emplace_back(new DynamicMetadataFormatter(filter_namespace, path, max_length)); + formatters.emplace_back(FormatterProviderPtr{new DynamicMetadataFormatter(filter_namespace, path, max_length)}); } else if (token.find("START_TIME") == 0) { const size_t parameters_length = pos + StartTimeParamStart + 1; const size_t parameters_end = command_end_position - parameters_length; @@ -196,9 +247,9 @@ std::vector AccessLogFormatParser::parse(const std::string& format const std::string args = token[StartTimeParamStart - 1] == '(' ? token.substr(StartTimeParamStart, parameters_end) : ""; - formatters.emplace_back(new StartTimeFormatter(args)); + formatters.emplace_back(FormatterProviderPtr{new StartTimeFormatter(args)}); } else { - formatters.emplace_back(new StreamInfoFormatter(token)); + formatters.emplace_back(FormatterProviderPtr{new StreamInfoFormatter(token)}); } pos = command_end_position; } else { @@ -207,7 +258,7 @@ std::vector AccessLogFormatParser::parse(const std::string& format } if (!current_token.empty()) { - formatters.emplace_back(new PlainStringFormatter(current_token)); + formatters.emplace_back(FormatterProviderPtr{new PlainStringFormatter(current_token)}); } return formatters; diff --git a/source/common/access_log/access_log_formatter.h b/source/common/access_log/access_log_formatter.h index 5bed621b5e770..6e70d623eea01 100644 --- a/source/common/access_log/access_log_formatter.h +++ b/source/common/access_log/access_log_formatter.h @@ -20,7 +20,7 @@ namespace AccessLog { */ class AccessLogFormatParser { public: - static std::vector parse(const std::string& format); + static std::vector parse(const std::string& format); private: /** @@ -95,14 +95,35 @@ class FormatterImpl : public Formatter { const StreamInfo::StreamInfo& stream_info) const override; private: - std::vector formatters_; + std::vector providers_; +}; + +class JsonFormatterImpl : public Formatter { +public: + JsonFormatterImpl(const std::map& format_mapping); + + // Formatter::format + std::string format(const Http::HeaderMap& request_headers, + const Http::HeaderMap& response_headers, + const Http::HeaderMap& response_trailers, + const RequestInfo::RequestInfo& request_info) const override; + + std::unique_ptr> to_map( + const Http::HeaderMap& request_headers, + const Http::HeaderMap& response_headers, + const Http::HeaderMap& response_trailers, + const RequestInfo::RequestInfo& request_info) const; + +private: + std::vector providers_; + std::map json_output_format_; }; /** * Formatter for string literal. It ignores headers and stream info and returns string by which it * was initialized. */ -class PlainStringFormatter : public Formatter { +class PlainStringFormatter : public FormatterProvider { public: PlainStringFormatter(const std::string& str); @@ -130,7 +151,7 @@ class HeaderFormatter { /** * Formatter based on request header. */ -class RequestHeaderFormatter : public Formatter, HeaderFormatter { +class RequestHeaderFormatter : public FormatterProvider, HeaderFormatter { public: RequestHeaderFormatter(const std::string& main_header, const std::string& alternative_header, absl::optional max_length); @@ -143,7 +164,7 @@ class RequestHeaderFormatter : public Formatter, HeaderFormatter { /** * Formatter based on the response header. */ -class ResponseHeaderFormatter : public Formatter, HeaderFormatter { +class ResponseHeaderFormatter : public FormatterProvider, HeaderFormatter { public: ResponseHeaderFormatter(const std::string& main_header, const std::string& alternative_header, absl::optional max_length); @@ -156,7 +177,7 @@ class ResponseHeaderFormatter : public Formatter, HeaderFormatter { /** * Formatter based on the response trailer. */ -class ResponseTrailerFormatter : public Formatter, HeaderFormatter { +class ResponseTrailerFormatter : public FormatterProvider, HeaderFormatter { public: ResponseTrailerFormatter(const std::string& main_header, const std::string& alternative_header, absl::optional max_length); @@ -170,7 +191,7 @@ class ResponseTrailerFormatter : public Formatter, HeaderFormatter { /** * Formatter based on the StreamInfo field. */ -class StreamInfoFormatter : public Formatter { +class StreamInfoFormatter : public FormatterProvider { public: StreamInfoFormatter(const std::string& field_name); @@ -201,12 +222,12 @@ class MetadataFormatter { /** * Formatter based on the DynamicMetadata from StreamInfo. */ -class DynamicMetadataFormatter : public Formatter, MetadataFormatter { +class DynamicMetadataFormatter : public FormatterProvider, MetadataFormatter { public: DynamicMetadataFormatter(const std::string& filter_namespace, const std::vector& path, absl::optional max_length); - // Formatter::format + // FormatterProvider::format std::string format(const Http::HeaderMap&, const Http::HeaderMap&, const Http::HeaderMap&, const StreamInfo::StreamInfo& stream_info) const override; }; @@ -214,7 +235,7 @@ class DynamicMetadataFormatter : public Formatter, MetadataFormatter { /** * Formatter */ -class StartTimeFormatter : public Formatter { +class StartTimeFormatter : public FormatterProvider { public: StartTimeFormatter(const std::string& format); std::string format(const Http::HeaderMap&, const Http::HeaderMap&, const Http::HeaderMap&, diff --git a/source/common/router/header_formatter.h b/source/common/router/header_formatter.h index f1b73621ad7e0..5e7ba71df7682 100644 --- a/source/common/router/header_formatter.h +++ b/source/common/router/header_formatter.h @@ -43,7 +43,7 @@ class StreamInfoHeaderFormatter : public HeaderFormatter { private: std::function field_extractor_; const bool append_; - std::unordered_map> start_time_formatters_; + std::unordered_map> start_time_formatters_; }; /** diff --git a/source/extensions/access_loggers/file/config.cc b/source/extensions/access_loggers/file/config.cc index 8490a72eccb04..79900d43f1c88 100644 --- a/source/extensions/access_loggers/file/config.cc +++ b/source/extensions/access_loggers/file/config.cc @@ -6,6 +6,7 @@ #include "common/access_log/access_log_formatter.h" #include "common/protobuf/protobuf.h" +#include "common/common/logger.h" #include "extensions/access_loggers/file/file_access_log_impl.h" #include "extensions/access_loggers/well_known_names.h" @@ -22,11 +23,21 @@ FileAccessLogFactory::createAccessLogInstance(const Protobuf::Message& config, const auto& fal_config = MessageUtil::downcastAndValidate(config); AccessLog::FormatterPtr formatter; - if (fal_config.format().empty()) { - formatter = AccessLog::AccessLogFormatUtils::defaultAccessLogFormatter(); + + if (fal_config.access_log_format_case() == envoy::config::accesslog::v2::FileAccessLog::kFormat + || fal_config.access_log_format_case() == envoy::config::accesslog::v2::FileAccessLog::ACCESS_LOG_FORMAT_NOT_SET) { + if (fal_config.format().empty()) { + formatter = AccessLog::AccessLogFormatUtils::defaultAccessLogFormatter(); + } else { + formatter.reset(new AccessLog::FormatterImpl(fal_config.format())); + } + } else if (fal_config.access_log_format_case() == envoy::config::accesslog::v2::FileAccessLog::kJsonFormat) { + const auto& json_format_map = this->convert_json_format_to_map(fal_config.json_format()); + formatter.reset(new AccessLog::JsonFormatterImpl(json_format_map)); } else { - formatter.reset(new AccessLog::FormatterImpl(fal_config.format())); + throw EnvoyException("Invalid access_log format provided"); } + return std::make_shared(fal_config.path(), std::move(filter), std::move(formatter), context.accessLogManager()); } @@ -37,6 +48,17 @@ ProtobufTypes::MessagePtr FileAccessLogFactory::createEmptyConfigProto() { std::string FileAccessLogFactory::name() const { return AccessLogNames::get().File; } +std::map FileAccessLogFactory::convert_json_format_to_map(google::protobuf::Struct json_format) { + std::map output; + for (const auto& pair : json_format.fields()) { + if (pair.second.kind_case() != google::protobuf::Value::kStringValue) { + throw EnvoyException("Only string values are supported in the JSON access log format."); + } + output.emplace(pair.first, pair.second.string_value()); + } + return output; +} + /** * Static registration for the file access log. @see RegisterFactory. */ diff --git a/source/extensions/access_loggers/file/config.h b/source/extensions/access_loggers/file/config.h index d3ebf58c352f8..3ce929d671e83 100644 --- a/source/extensions/access_loggers/file/config.h +++ b/source/extensions/access_loggers/file/config.h @@ -19,6 +19,9 @@ class FileAccessLogFactory : public Server::Configuration::AccessLogInstanceFact ProtobufTypes::MessagePtr createEmptyConfigProto() override; std::string name() const override; + +private: + std::map convert_json_format_to_map(google::protobuf::Struct config); }; } // namespace File diff --git a/test/common/access_log/access_log_formatter_fuzz_test.cc b/test/common/access_log/access_log_formatter_fuzz_test.cc index ec9332192e67d..2a9b6d7abb7a8 100644 --- a/test/common/access_log/access_log_formatter_fuzz_test.cc +++ b/test/common/access_log/access_log_formatter_fuzz_test.cc @@ -9,7 +9,7 @@ namespace Fuzz { DEFINE_PROTO_FUZZER(const test::common::access_log::TestCase& input) { try { - std::vector formatters = + std::vector formatters = AccessLog::AccessLogFormatParser::parse(input.format()); for (const auto& it : formatters) { it->format( diff --git a/test/common/access_log/access_log_formatter_test.cc b/test/common/access_log/access_log_formatter_test.cc index 762d6fcd9262c..ac8c4695317ec 100644 --- a/test/common/access_log/access_log_formatter_test.cc +++ b/test/common/access_log/access_log_formatter_test.cc @@ -382,6 +382,194 @@ TEST(AccessLogFormatterTest, startTimeFormatter) { } } +TEST(AccessLogFormatterTest, JsonFormatterTest) { + { + RequestInfo::MockRequestInfo request_info; + Http::TestHeaderMapImpl request_header{}; + Http::TestHeaderMapImpl response_header{}; + Http::TestHeaderMapImpl response_trailer{}; + + envoy::api::v2::core::Metadata metadata; + populateMetadataTestData(metadata); + absl::optional protocol = Http::Protocol::Http11; + EXPECT_CALL(request_info, protocol()).WillRepeatedly(Return(protocol)); + + std::map expected_json_map = { + {"plain_string", "plain_string_value"} + }; + + const std::map key_mapping = { + {"plain_string", "plain_string_value"} + }; + JsonFormatterImpl formatter(key_mapping); + + auto actual_json_map = formatter.to_map(request_header, response_header, response_trailer, request_info); + EXPECT_THAT(*actual_json_map, ::testing::ContainerEq(expected_json_map)); + + auto actual_string = formatter.format(request_header, response_header, response_trailer, request_info); + EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); + } + + { + RequestInfo::MockRequestInfo request_info; + Http::TestHeaderMapImpl request_header{}; + Http::TestHeaderMapImpl response_header{}; + Http::TestHeaderMapImpl response_trailer{}; + + envoy::api::v2::core::Metadata metadata; + populateMetadataTestData(metadata); + absl::optional protocol = Http::Protocol::Http11; + EXPECT_CALL(request_info, protocol()).WillRepeatedly(Return(protocol)); + + std::map expected_json_map = { + {"protocol", "HTTP/1.1"} + }; + + const std::map key_mapping = { + {"protocol", "%PROTOCOL%"} + }; + JsonFormatterImpl formatter(key_mapping); + + auto actual_json_map = formatter.to_map(request_header, response_header, response_trailer, request_info); + EXPECT_THAT(*actual_json_map, ::testing::ContainerEq(expected_json_map)); + + auto actual_string = formatter.format(request_header, response_header, response_trailer, request_info); + EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); + } + + { + RequestInfo::MockRequestInfo request_info; + Http::TestHeaderMapImpl request_header{{"some_request_header", "SOME_REQUEST_HEADER"}}; + Http::TestHeaderMapImpl response_header{{"some_response_header", "SOME_RESPONSE_HEADER"}}; + Http::TestHeaderMapImpl response_trailer{}; + + std::map expected_json_map = { + {"protocol", "HTTP/1.1"}, + {"some_request_header", "SOME_REQUEST_HEADER"}, + {"nonexistent_response_header", "-"}, + {"some_response_header", "SOME_RESPONSE_HEADER"} + }; + + const std::map key_mapping = { + {"protocol", "%PROTOCOL%"}, + {"some_request_header", "%REQ(some_request_header)%"}, + {"nonexistent_response_header", "%RESP(nonexistent_response_header)%"}, + {"some_response_header", "%RESP(some_response_header)%"} + }; + JsonFormatterImpl formatter(key_mapping); + + absl::optional protocol = Http::Protocol::Http11; + EXPECT_CALL(request_info, protocol()).WillRepeatedly(Return(protocol)); + + auto actual_json_map = formatter.to_map(request_header, response_header, response_trailer, request_info); + EXPECT_THAT(*actual_json_map, ::testing::ContainerEq(expected_json_map)); + + auto actual_string = formatter.format(request_header, response_header, response_trailer, request_info); + EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); + } + + { + RequestInfo::MockRequestInfo request_info; + Http::TestHeaderMapImpl request_header{{"request_present_header", "REQUEST_PRESENT_HEADER"}}; + Http::TestHeaderMapImpl response_header{{"response_present_header", "RESPONSE_PRESENT_HEADER"}}; + Http::TestHeaderMapImpl response_trailer{}; + + std::map expected_json_map = { + {"request_present_header_or_request_absent_header", "REQUEST_PRESENT_HEADER"}, + {"request_absent_header_or_request_present_header", "REQUEST_PRESENT_HEADER"}, + {"response_absent_header_or_response_absent_header", "RESPONSE_PRESENT_HEADER"}, + {"response_present_header_or_response_absent_header", "RESPONSE_PRESENT_HEADER"} + }; + + const std::map key_mapping = { + {"request_present_header_or_request_absent_header", "%REQ(request_present_header?request_absent_header)%"}, + {"request_absent_header_or_request_present_header", "%REQ(request_absent_header?request_present_header)%"}, + {"response_absent_header_or_response_absent_header", "%RESP(response_absent_header?response_present_header)%"}, + {"response_present_header_or_response_absent_header", "%RESP(response_present_header?response_absent_header)%"} + }; + JsonFormatterImpl formatter(key_mapping); + + absl::optional protocol = Http::Protocol::Http11; + EXPECT_CALL(request_info, protocol()).WillRepeatedly(Return(protocol)); + + auto actual_json_map = formatter.to_map(request_header, response_header, response_trailer, request_info); + EXPECT_THAT(*actual_json_map, ::testing::ContainerEq(expected_json_map)); + + auto actual_string = formatter.format(request_header, response_header, response_trailer, request_info); + EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); + } + + { + RequestInfo::MockRequestInfo request_info; + Http::TestHeaderMapImpl request_header{{"first", "GET"}, {":path", "/"}}; + Http::TestHeaderMapImpl response_header{{"second", "PUT"}, {"test", "test"}}; + Http::TestHeaderMapImpl response_trailer{{"third", "POST"}, {"test-2", "test-2"}}; + + envoy::api::v2::core::Metadata metadata; + populateMetadataTestData(metadata); + EXPECT_CALL(request_info, dynamicMetadata()).WillRepeatedly(ReturnRef(metadata)); + + std::map expected_json_map = { + {"test_key", "\"test_value\""}, + {"test_obj", "{\"inner_key\":\"inner_value\"}"}, + {"test_obj.inner_key", "\"inner_value\""} + }; + + const std::map key_mapping = { + {"test_key", "%DYNAMIC_METADATA(com.test:test_key)%"}, + {"test_obj", "%DYNAMIC_METADATA(com.test:test_obj)%"}, + {"test_obj.inner_key", "%DYNAMIC_METADATA(com.test:test_obj:inner_key)%"} + }; + + JsonFormatterImpl formatter(key_mapping); + + auto actual_json_map = formatter.to_map(request_header, response_header, response_trailer, request_info); + EXPECT_THAT(*actual_json_map, ::testing::ContainerEq(expected_json_map)); + + auto actual_string = formatter.format(request_header, response_header, response_trailer, request_info); + EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); + } + + { + RequestInfo::MockRequestInfo request_info; + Http::TestHeaderMapImpl request_header{}; + Http::TestHeaderMapImpl response_header{}; + Http::TestHeaderMapImpl response_trailer{}; + + time_t test_epoch = 1522280158; + SystemTime time = std::chrono::system_clock::from_time_t(test_epoch); + EXPECT_CALL(request_info, startTime()).WillRepeatedly(Return(time)); + + // Needed to take into account the behavior in non-GMT timezones. + struct tm time_val; + gmtime_r(&test_epoch, &time_val); + time_t expected_time_t = mktime(&time_val); + + std::map expected_json_map = { + {"simple_date", "2018/03/28"}, + {"test_time", fmt::format("{}", expected_time_t)}, + {"bad_format", "bad_format"}, + {"default", "2018-03-28T23:35:58.000Z"}, + {"all_zeroes", "000000000.0.00.000"} + }; + + const std::map key_mapping = { + {"simple_date", "%START_TIME(%Y/%m/%d)%"}, + {"test_time", "%START_TIME(%s)%"}, + {"bad_format", "%START_TIME(bad_format)%"}, + {"default", "%START_TIME%"}, + {"all_zeroes", "%START_TIME(%f.%1f.%2f.%3f)%"} + }; + JsonFormatterImpl formatter(key_mapping); + + auto actual_json_map = formatter.to_map(request_header, response_header, response_trailer, request_info); + EXPECT_THAT(*actual_json_map, ::testing::ContainerEq(expected_json_map)); + + auto actual_string = formatter.format(request_header, response_header, response_trailer, request_info); + EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); + } +} + TEST(AccessLogFormatterTest, CompositeFormatterSuccess) { StreamInfo::MockStreamInfo stream_info; Http::TestHeaderMapImpl request_header{{"first", "GET"}, {":path", "/"}}; diff --git a/test/common/access_log/access_log_impl_test.cc b/test/common/access_log/access_log_impl_test.cc index 25820484cc7d1..6f4418c4112fb 100644 --- a/test/common/access_log/access_log_impl_test.cc +++ b/test/common/access_log/access_log_impl_test.cc @@ -40,7 +40,7 @@ parseAccessLogFromJson(const std::string& json_string) { auto json_object_ptr = Json::Factory::loadFromString(json_string); Config::FilterJson::translateAccessLog(*json_object_ptr, access_log); return access_log; -} +} envoy::config::filter::accesslog::v2::AccessLog parseAccessLogFromV2Yaml(const std::string& yaml) { envoy::config::filter::accesslog::v2::AccessLog access_log; diff --git a/test/extensions/access_loggers/file/config_test.cc b/test/extensions/access_loggers/file/config_test.cc index 7bd56fea21107..3fb1593cae1f3 100644 --- a/test/extensions/access_loggers/file/config_test.cc +++ b/test/extensions/access_loggers/file/config_test.cc @@ -73,6 +73,85 @@ TEST(FileAccessLogConfigTest, FileAccessLogTest) { EXPECT_NE(nullptr, dynamic_cast(instance.get())); } +TEST(FileAccessLogConfigTest, FileAccessLogJsonTest) { + envoy::config::filter::accesslog::v2::AccessLog config; + + envoy::config::accesslog::v2::FileAccessLog fal_config; + fal_config.set_path("/dev/null"); + + // auto json_format2 = new envoy::config::accesslog::v2::FileAccessLogJsonFormat; + auto json_format = new google::protobuf::Struct; + google::protobuf::Value string_value; + string_value.set_string_value("%PROTOCOL%"); + (*json_format->mutable_fields())[std::string{"protocol"}] = string_value; + fal_config.set_allocated_json_format(json_format); + + EXPECT_EQ(fal_config.access_log_format_case(), envoy::config::accesslog::v2::FileAccessLog::kJsonFormat); + MessageUtil::jsonConvert(fal_config, *config.mutable_config()); + + NiceMock context; + EXPECT_THROW_WITH_MESSAGE(AccessLog::AccessLogFactory::fromProto(config, context), EnvoyException, + "Provided name for static registration lookup was empty."); + + config.set_name(AccessLogNames::get().File); + + AccessLog::InstanceSharedPtr log = AccessLog::AccessLogFactory::fromProto(config, context); + + EXPECT_NE(nullptr, log); + EXPECT_NE(nullptr, dynamic_cast(log.get())); + + config.set_name("INVALID"); + + EXPECT_THROW_WITH_MESSAGE(AccessLog::AccessLogFactory::fromProto(config, context), EnvoyException, + "Didn't find a registered implementation for name: 'INVALID'"); +} + +TEST(FileAccessLogConfigTest, FileAccessLogJsonConversionTest) { + { + envoy::config::filter::accesslog::v2::AccessLog config; + config.set_name(AccessLogNames::get().File); + envoy::config::accesslog::v2::FileAccessLog fal_config; + fal_config.set_path("/dev/null"); + + auto json_format = new google::protobuf::Struct; + google::protobuf::Value string_value; + string_value.set_bool_value(false); + (*json_format->mutable_fields())[std::string{"protocol"}] = string_value; + fal_config.set_allocated_json_format(json_format); + + MessageUtil::jsonConvert(fal_config, *config.mutable_config()); + NiceMock context; + + EXPECT_THROW_WITH_MESSAGE(AccessLog::AccessLogFactory::fromProto(config, context), EnvoyException, + "Only string values are supported in the JSON access log format."); + } + + { + envoy::config::filter::accesslog::v2::AccessLog config; + config.set_name(AccessLogNames::get().File); + envoy::config::accesslog::v2::FileAccessLog fal_config; + fal_config.set_path("/dev/null"); + + auto json_format = new google::protobuf::Struct; + auto nested_struct = new google::protobuf::Struct; + google::protobuf::Value string_value; + string_value.set_string_value(std::string{"some_nested_value"}); + (*nested_struct->mutable_fields())[std::string{"some_nested_key"}] = string_value; + + google::protobuf::Value struct_value; + struct_value.set_allocated_struct_value(nested_struct); + (*json_format->mutable_fields())[std::string{"top_level_key"}] = struct_value; + fal_config.set_allocated_json_format(json_format); + + MessageUtil::jsonConvert(fal_config, *config.mutable_config()); + NiceMock context; + + EXPECT_THROW_WITH_MESSAGE(AccessLog::AccessLogFactory::fromProto(config, context), EnvoyException, + "Only string values are supported in the JSON access log format."); + } +} + + } // namespace File } // namespace AccessLoggers } // namespace Extensions From 031377fdf199324a160c84a42b447ccc2bf884f2 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Tue, 9 Oct 2018 16:36:52 -0700 Subject: [PATCH 02/51] Minor fixes, remove a unique_ptr Signed-off-by: Aaltan Ahmad --- .../common/access_log/access_log_formatter.cc | 29 +++++++++---------- .../common/access_log/access_log_formatter.h | 9 +++--- .../access_log_formatter_fuzz_test.cc | 2 +- .../access_log/access_log_formatter_test.cc | 12 ++++---- 4 files changed, 25 insertions(+), 27 deletions(-) diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc index 74830f5fc9bff..eb955aa5bf3e4 100644 --- a/source/common/access_log/access_log_formatter.cc +++ b/source/common/access_log/access_log_formatter.cc @@ -75,19 +75,19 @@ std::string FormatterImpl::format(const Http::HeaderMap& request_headers, } JsonFormatterImpl::JsonFormatterImpl(const std::map& format_mapping) { - for (const auto& format_pair : format_mapping) { - auto providers = AccessLogFormatParser::parse(format_pair.second); + for (const auto& pair : format_mapping) { + auto providers = AccessLogFormatParser::parse(pair.second); // Enforce that each key only has one format specifier in it if (providers.size() > 1) { - throw EnvoyException(fmt::format("More than one format specifier was provided in the JSON log format: {}", format_pair.second)); + throw EnvoyException(fmt::format("More than one format specifier was provided in the JSON log format: {}", pair.second)); } if (providers.size() < 1) { - throw EnvoyException(fmt::format("No format specifier was provided in the JSON log format: {}", format_pair.second)); + throw EnvoyException(fmt::format("No format specifier was provided in the JSON log format: {}", pair.second)); } - json_output_format_.emplace(format_pair.first, std::move(providers[0])); + json_output_format_.emplace(pair.first, std::move(providers[0])); } } @@ -98,28 +98,27 @@ std::string JsonFormatterImpl::format(const Http::HeaderMap& request_headers, rapidjson::StringBuffer strbuf; rapidjson::Writer writer(strbuf); - writer.StartObject(); auto output_map = this->to_map(request_headers, response_headers, response_trailers, request_info); - for (auto it = output_map->begin(); it != output_map->end(); ++it) { - writer.Key(it->first.c_str()); - writer.String(it->second.c_str()); + writer.StartObject(); + for (const auto& pair : output_map) { + writer.Key(pair.first.c_str()); + writer.String(pair.second.c_str()); } writer.EndObject(); return fmt::format("{}\n", strbuf.GetString()); } -std::unique_ptr> -JsonFormatterImpl::to_map(const Http::HeaderMap& request_headers, - const Http::HeaderMap& response_headers, - const Http::HeaderMap& response_trailers, - const RequestInfo::RequestInfo& request_info) const { +std::map JsonFormatterImpl::to_map(const Http::HeaderMap& request_headers, + const Http::HeaderMap& response_headers, + const Http::HeaderMap& response_trailers, + const RequestInfo::RequestInfo& request_info) const { std::map output; for (const auto& pair : json_output_format_) { output.emplace( pair.first, pair.second->format(request_headers, response_headers, response_trailers, request_info)); } - return std::make_unique>(output); + return output; } void AccessLogFormatParser::parseCommandHeader(const std::string& token, const size_t start, diff --git a/source/common/access_log/access_log_formatter.h b/source/common/access_log/access_log_formatter.h index 6e70d623eea01..45a34846d1fb8 100644 --- a/source/common/access_log/access_log_formatter.h +++ b/source/common/access_log/access_log_formatter.h @@ -108,11 +108,10 @@ class JsonFormatterImpl : public Formatter { const Http::HeaderMap& response_trailers, const RequestInfo::RequestInfo& request_info) const override; - std::unique_ptr> to_map( - const Http::HeaderMap& request_headers, - const Http::HeaderMap& response_headers, - const Http::HeaderMap& response_trailers, - const RequestInfo::RequestInfo& request_info) const; + std::map to_map(const Http::HeaderMap& request_headers, + const Http::HeaderMap& response_headers, + const Http::HeaderMap& response_trailers, + const RequestInfo::RequestInfo& request_info) const; private: std::vector providers_; diff --git a/test/common/access_log/access_log_formatter_fuzz_test.cc b/test/common/access_log/access_log_formatter_fuzz_test.cc index 2a9b6d7abb7a8..9a6175912da4f 100644 --- a/test/common/access_log/access_log_formatter_fuzz_test.cc +++ b/test/common/access_log/access_log_formatter_fuzz_test.cc @@ -9,7 +9,7 @@ namespace Fuzz { DEFINE_PROTO_FUZZER(const test::common::access_log::TestCase& input) { try { - std::vector formatters = + std::vector formatters = AccessLog::AccessLogFormatParser::parse(input.format()); for (const auto& it : formatters) { it->format( diff --git a/test/common/access_log/access_log_formatter_test.cc b/test/common/access_log/access_log_formatter_test.cc index ac8c4695317ec..e265204a1ed6a 100644 --- a/test/common/access_log/access_log_formatter_test.cc +++ b/test/common/access_log/access_log_formatter_test.cc @@ -404,7 +404,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { JsonFormatterImpl formatter(key_mapping); auto actual_json_map = formatter.to_map(request_header, response_header, response_trailer, request_info); - EXPECT_THAT(*actual_json_map, ::testing::ContainerEq(expected_json_map)); + EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); auto actual_string = formatter.format(request_header, response_header, response_trailer, request_info); EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); @@ -431,7 +431,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { JsonFormatterImpl formatter(key_mapping); auto actual_json_map = formatter.to_map(request_header, response_header, response_trailer, request_info); - EXPECT_THAT(*actual_json_map, ::testing::ContainerEq(expected_json_map)); + EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); auto actual_string = formatter.format(request_header, response_header, response_trailer, request_info); EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); @@ -462,7 +462,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { EXPECT_CALL(request_info, protocol()).WillRepeatedly(Return(protocol)); auto actual_json_map = formatter.to_map(request_header, response_header, response_trailer, request_info); - EXPECT_THAT(*actual_json_map, ::testing::ContainerEq(expected_json_map)); + EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); auto actual_string = formatter.format(request_header, response_header, response_trailer, request_info); EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); @@ -493,7 +493,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { EXPECT_CALL(request_info, protocol()).WillRepeatedly(Return(protocol)); auto actual_json_map = formatter.to_map(request_header, response_header, response_trailer, request_info); - EXPECT_THAT(*actual_json_map, ::testing::ContainerEq(expected_json_map)); + EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); auto actual_string = formatter.format(request_header, response_header, response_trailer, request_info); EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); @@ -524,7 +524,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { JsonFormatterImpl formatter(key_mapping); auto actual_json_map = formatter.to_map(request_header, response_header, response_trailer, request_info); - EXPECT_THAT(*actual_json_map, ::testing::ContainerEq(expected_json_map)); + EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); auto actual_string = formatter.format(request_header, response_header, response_trailer, request_info); EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); @@ -563,7 +563,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { JsonFormatterImpl formatter(key_mapping); auto actual_json_map = formatter.to_map(request_header, response_header, response_trailer, request_info); - EXPECT_THAT(*actual_json_map, ::testing::ContainerEq(expected_json_map)); + EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); auto actual_string = formatter.format(request_header, response_header, response_trailer, request_info); EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); From 1c754a5dc6430bb8c9bb2540a78bbf1a2746f25e Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Tue, 9 Oct 2018 16:45:41 -0700 Subject: [PATCH 03/51] Run clang-format on source/common/access_log Signed-off-by: Aaltan Ahmad --- .../common/access_log/access_log_formatter.cc | 42 ++++++++++--------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc index eb955aa5bf3e4..668b06193cdd1 100644 --- a/source/common/access_log/access_log_formatter.cc +++ b/source/common/access_log/access_log_formatter.cc @@ -74,17 +74,20 @@ std::string FormatterImpl::format(const Http::HeaderMap& request_headers, return log_line; } -JsonFormatterImpl::JsonFormatterImpl(const std::map& format_mapping) { +JsonFormatterImpl::JsonFormatterImpl( + const std::map& format_mapping) { for (const auto& pair : format_mapping) { auto providers = AccessLogFormatParser::parse(pair.second); // Enforce that each key only has one format specifier in it if (providers.size() > 1) { - throw EnvoyException(fmt::format("More than one format specifier was provided in the JSON log format: {}", pair.second)); + throw EnvoyException(fmt::format( + "More than one format specifier was provided in the JSON log format: {}", pair.second)); } if (providers.size() < 1) { - throw EnvoyException(fmt::format("No format specifier was provided in the JSON log format: {}", pair.second)); + throw EnvoyException( + fmt::format("No format specifier was provided in the JSON log format: {}", pair.second)); } json_output_format_.emplace(pair.first, std::move(providers[0])); @@ -98,7 +101,8 @@ std::string JsonFormatterImpl::format(const Http::HeaderMap& request_headers, rapidjson::StringBuffer strbuf; rapidjson::Writer writer(strbuf); - auto output_map = this->to_map(request_headers, response_headers, response_trailers, request_info); + auto output_map = + this->to_map(request_headers, response_headers, response_trailers, request_info); writer.StartObject(); for (const auto& pair : output_map) { writer.Key(pair.first.c_str()); @@ -108,15 +112,13 @@ std::string JsonFormatterImpl::format(const Http::HeaderMap& request_headers, return fmt::format("{}\n", strbuf.GetString()); } -std::map JsonFormatterImpl::to_map(const Http::HeaderMap& request_headers, - const Http::HeaderMap& response_headers, - const Http::HeaderMap& response_trailers, - const RequestInfo::RequestInfo& request_info) const { +std::map JsonFormatterImpl::to_map( + const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, + const Http::HeaderMap& response_trailers, const RequestInfo::RequestInfo& request_info) const { std::map output; for (const auto& pair : json_output_format_) { - output.emplace( - pair.first, - pair.second->format(request_headers, response_headers, response_trailers, request_info)); + output.emplace(pair.first, pair.second->format(request_headers, response_headers, + response_trailers, request_info)); } return output; } @@ -189,8 +191,7 @@ std::vector AccessLogFormatParser::parse(const std::string for (size_t pos = 0; pos < format.length(); ++pos) { if (format[pos] == '%') { if (!current_token.empty()) { - formatters.emplace_back( - FormatterProviderPtr{new PlainStringFormatter(current_token)}); + formatters.emplace_back(FormatterProviderPtr{new PlainStringFormatter(current_token)}); current_token = ""; } @@ -213,24 +214,24 @@ std::vector AccessLogFormatParser::parse(const std::string parseCommandHeader(token, ReqParamStart, main_header, alternative_header, max_length); - formatters.emplace_back( - FormatterProviderPtr{new RequestHeaderFormatter(main_header, alternative_header, max_length)}); + formatters.emplace_back(FormatterProviderPtr{ + new RequestHeaderFormatter(main_header, alternative_header, max_length)}); } else if (token.find("RESP(") == 0) { std::string main_header, alternative_header; absl::optional max_length; parseCommandHeader(token, RespParamStart, main_header, alternative_header, max_length); - formatters.emplace_back( - FormatterProviderPtr{new ResponseHeaderFormatter(main_header, alternative_header, max_length)}); + formatters.emplace_back(FormatterProviderPtr{ + new ResponseHeaderFormatter(main_header, alternative_header, max_length)}); } else if (token.find("TRAILER(") == 0) { std::string main_header, alternative_header; absl::optional max_length; parseCommandHeader(token, TrailParamStart, main_header, alternative_header, max_length); - formatters.emplace_back( - FormatterProviderPtr{new ResponseTrailerFormatter(main_header, alternative_header, max_length)}); + formatters.emplace_back(FormatterProviderPtr{ + new ResponseTrailerFormatter(main_header, alternative_header, max_length)}); } else if (token.find(DYNAMIC_META_TOKEN) == 0) { std::string filter_namespace; absl::optional max_length; @@ -238,7 +239,8 @@ std::vector AccessLogFormatParser::parse(const std::string const size_t start = DYNAMIC_META_TOKEN.size(); parseCommand(token, start, ":", filter_namespace, path, max_length); - formatters.emplace_back(FormatterProviderPtr{new DynamicMetadataFormatter(filter_namespace, path, max_length)}); + formatters.emplace_back( + FormatterProviderPtr{new DynamicMetadataFormatter(filter_namespace, path, max_length)}); } else if (token.find("START_TIME") == 0) { const size_t parameters_length = pos + StartTimeParamStart + 1; const size_t parameters_end = command_end_position - parameters_length; From f604562781d8addaa7e6d567a4d38a48b48c6bad Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Tue, 9 Oct 2018 16:47:43 -0700 Subject: [PATCH 04/51] Run clang-format on source/extensions/access_loggers/file Signed-off-by: Aaltan Ahmad --- .../extensions/access_loggers/file/config.cc | 25 +++++++++++-------- .../extensions/access_loggers/file/config.h | 3 ++- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/source/extensions/access_loggers/file/config.cc b/source/extensions/access_loggers/file/config.cc index 79900d43f1c88..6ba74389ddef2 100644 --- a/source/extensions/access_loggers/file/config.cc +++ b/source/extensions/access_loggers/file/config.cc @@ -5,8 +5,8 @@ #include "envoy/server/filter_config.h" #include "common/access_log/access_log_formatter.h" -#include "common/protobuf/protobuf.h" #include "common/common/logger.h" +#include "common/protobuf/protobuf.h" #include "extensions/access_loggers/file/file_access_log_impl.h" #include "extensions/access_loggers/well_known_names.h" @@ -24,20 +24,22 @@ FileAccessLogFactory::createAccessLogInstance(const Protobuf::Message& config, MessageUtil::downcastAndValidate(config); AccessLog::FormatterPtr formatter; - if (fal_config.access_log_format_case() == envoy::config::accesslog::v2::FileAccessLog::kFormat - || fal_config.access_log_format_case() == envoy::config::accesslog::v2::FileAccessLog::ACCESS_LOG_FORMAT_NOT_SET) { - if (fal_config.format().empty()) { - formatter = AccessLog::AccessLogFormatUtils::defaultAccessLogFormatter(); - } else { - formatter.reset(new AccessLog::FormatterImpl(fal_config.format())); - } - } else if (fal_config.access_log_format_case() == envoy::config::accesslog::v2::FileAccessLog::kJsonFormat) { + if (fal_config.access_log_format_case() == envoy::config::accesslog::v2::FileAccessLog::kFormat || + fal_config.access_log_format_case() == + envoy::config::accesslog::v2::FileAccessLog::ACCESS_LOG_FORMAT_NOT_SET) { + if (fal_config.format().empty()) { + formatter = AccessLog::AccessLogFormatUtils::defaultAccessLogFormatter(); + } else { + formatter.reset(new AccessLog::FormatterImpl(fal_config.format())); + } + } else if (fal_config.access_log_format_case() == + envoy::config::accesslog::v2::FileAccessLog::kJsonFormat) { const auto& json_format_map = this->convert_json_format_to_map(fal_config.json_format()); formatter.reset(new AccessLog::JsonFormatterImpl(json_format_map)); } else { throw EnvoyException("Invalid access_log format provided"); } - + return std::make_shared(fal_config.path(), std::move(filter), std::move(formatter), context.accessLogManager()); } @@ -48,7 +50,8 @@ ProtobufTypes::MessagePtr FileAccessLogFactory::createEmptyConfigProto() { std::string FileAccessLogFactory::name() const { return AccessLogNames::get().File; } -std::map FileAccessLogFactory::convert_json_format_to_map(google::protobuf::Struct json_format) { +std::map +FileAccessLogFactory::convert_json_format_to_map(google::protobuf::Struct json_format) { std::map output; for (const auto& pair : json_format.fields()) { if (pair.second.kind_case() != google::protobuf::Value::kStringValue) { diff --git a/source/extensions/access_loggers/file/config.h b/source/extensions/access_loggers/file/config.h index 3ce929d671e83..652219440905b 100644 --- a/source/extensions/access_loggers/file/config.h +++ b/source/extensions/access_loggers/file/config.h @@ -21,7 +21,8 @@ class FileAccessLogFactory : public Server::Configuration::AccessLogInstanceFact std::string name() const override; private: - std::map convert_json_format_to_map(google::protobuf::Struct config); + std::map + convert_json_format_to_map(google::protobuf::Struct config); }; } // namespace File From cbdc6dd9301c8b0b6bbdc9a311475b3962273891 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Tue, 9 Oct 2018 16:51:33 -0700 Subject: [PATCH 05/51] Run clang-format on test/common/access_log Signed-off-by: Aaltan Ahmad --- .../access_log/access_log_formatter_test.cc | 153 +++++++++--------- .../common/access_log/access_log_impl_test.cc | 2 +- 2 files changed, 78 insertions(+), 77 deletions(-) diff --git a/test/common/access_log/access_log_formatter_test.cc b/test/common/access_log/access_log_formatter_test.cc index e265204a1ed6a..5da0453a95897 100644 --- a/test/common/access_log/access_log_formatter_test.cc +++ b/test/common/access_log/access_log_formatter_test.cc @@ -383,30 +383,29 @@ TEST(AccessLogFormatterTest, startTimeFormatter) { } TEST(AccessLogFormatterTest, JsonFormatterTest) { - { + { RequestInfo::MockRequestInfo request_info; Http::TestHeaderMapImpl request_header{}; Http::TestHeaderMapImpl response_header{}; Http::TestHeaderMapImpl response_trailer{}; - + envoy::api::v2::core::Metadata metadata; populateMetadataTestData(metadata); absl::optional protocol = Http::Protocol::Http11; EXPECT_CALL(request_info, protocol()).WillRepeatedly(Return(protocol)); - - std::map expected_json_map = { - {"plain_string", "plain_string_value"} - }; + + std::map expected_json_map = {{"plain_string", "plain_string_value"}}; const std::map key_mapping = { - {"plain_string", "plain_string_value"} - }; + {"plain_string", "plain_string_value"}}; JsonFormatterImpl formatter(key_mapping); - - auto actual_json_map = formatter.to_map(request_header, response_header, response_trailer, request_info); + + auto actual_json_map = + formatter.to_map(request_header, response_header, response_trailer, request_info); EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); - - auto actual_string = formatter.format(request_header, response_header, response_trailer, request_info); + + auto actual_string = + formatter.format(request_header, response_header, response_trailer, request_info); EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); } @@ -415,25 +414,23 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { Http::TestHeaderMapImpl request_header{}; Http::TestHeaderMapImpl response_header{}; Http::TestHeaderMapImpl response_trailer{}; - + envoy::api::v2::core::Metadata metadata; populateMetadataTestData(metadata); absl::optional protocol = Http::Protocol::Http11; EXPECT_CALL(request_info, protocol()).WillRepeatedly(Return(protocol)); - - std::map expected_json_map = { - {"protocol", "HTTP/1.1"} - }; - const std::map key_mapping = { - {"protocol", "%PROTOCOL%"} - }; + std::map expected_json_map = {{"protocol", "HTTP/1.1"}}; + + const std::map key_mapping = {{"protocol", "%PROTOCOL%"}}; JsonFormatterImpl formatter(key_mapping); - - auto actual_json_map = formatter.to_map(request_header, response_header, response_trailer, request_info); + + auto actual_json_map = + formatter.to_map(request_header, response_header, response_trailer, request_info); EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); - - auto actual_string = formatter.format(request_header, response_header, response_trailer, request_info); + + auto actual_string = + formatter.format(request_header, response_header, response_trailer, request_info); EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); } @@ -444,27 +441,27 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { Http::TestHeaderMapImpl response_trailer{}; std::map expected_json_map = { - {"protocol", "HTTP/1.1"}, - {"some_request_header", "SOME_REQUEST_HEADER"}, - {"nonexistent_response_header", "-"}, - {"some_response_header", "SOME_RESPONSE_HEADER"} - }; + {"protocol", "HTTP/1.1"}, + {"some_request_header", "SOME_REQUEST_HEADER"}, + {"nonexistent_response_header", "-"}, + {"some_response_header", "SOME_RESPONSE_HEADER"}}; const std::map key_mapping = { - {"protocol", "%PROTOCOL%"}, - {"some_request_header", "%REQ(some_request_header)%"}, - {"nonexistent_response_header", "%RESP(nonexistent_response_header)%"}, - {"some_response_header", "%RESP(some_response_header)%"} - }; + {"protocol", "%PROTOCOL%"}, + {"some_request_header", "%REQ(some_request_header)%"}, + {"nonexistent_response_header", "%RESP(nonexistent_response_header)%"}, + {"some_response_header", "%RESP(some_response_header)%"}}; JsonFormatterImpl formatter(key_mapping); absl::optional protocol = Http::Protocol::Http11; EXPECT_CALL(request_info, protocol()).WillRepeatedly(Return(protocol)); - auto actual_json_map = formatter.to_map(request_header, response_header, response_trailer, request_info); + auto actual_json_map = + formatter.to_map(request_header, response_header, response_trailer, request_info); EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); - auto actual_string = formatter.format(request_header, response_header, response_trailer, request_info); + auto actual_string = + formatter.format(request_header, response_header, response_trailer, request_info); EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); } @@ -475,27 +472,31 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { Http::TestHeaderMapImpl response_trailer{}; std::map expected_json_map = { - {"request_present_header_or_request_absent_header", "REQUEST_PRESENT_HEADER"}, - {"request_absent_header_or_request_present_header", "REQUEST_PRESENT_HEADER"}, - {"response_absent_header_or_response_absent_header", "RESPONSE_PRESENT_HEADER"}, - {"response_present_header_or_response_absent_header", "RESPONSE_PRESENT_HEADER"} - }; + {"request_present_header_or_request_absent_header", "REQUEST_PRESENT_HEADER"}, + {"request_absent_header_or_request_present_header", "REQUEST_PRESENT_HEADER"}, + {"response_absent_header_or_response_absent_header", "RESPONSE_PRESENT_HEADER"}, + {"response_present_header_or_response_absent_header", "RESPONSE_PRESENT_HEADER"}}; const std::map key_mapping = { - {"request_present_header_or_request_absent_header", "%REQ(request_present_header?request_absent_header)%"}, - {"request_absent_header_or_request_present_header", "%REQ(request_absent_header?request_present_header)%"}, - {"response_absent_header_or_response_absent_header", "%RESP(response_absent_header?response_present_header)%"}, - {"response_present_header_or_response_absent_header", "%RESP(response_present_header?response_absent_header)%"} - }; + {"request_present_header_or_request_absent_header", + "%REQ(request_present_header?request_absent_header)%"}, + {"request_absent_header_or_request_present_header", + "%REQ(request_absent_header?request_present_header)%"}, + {"response_absent_header_or_response_absent_header", + "%RESP(response_absent_header?response_present_header)%"}, + {"response_present_header_or_response_absent_header", + "%RESP(response_present_header?response_absent_header)%"}}; JsonFormatterImpl formatter(key_mapping); absl::optional protocol = Http::Protocol::Http11; EXPECT_CALL(request_info, protocol()).WillRepeatedly(Return(protocol)); - auto actual_json_map = formatter.to_map(request_header, response_header, response_trailer, request_info); + auto actual_json_map = + formatter.to_map(request_header, response_header, response_trailer, request_info); EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); - auto actual_string = formatter.format(request_header, response_header, response_trailer, request_info); + auto actual_string = + formatter.format(request_header, response_header, response_trailer, request_info); EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); } @@ -504,29 +505,29 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { Http::TestHeaderMapImpl request_header{{"first", "GET"}, {":path", "/"}}; Http::TestHeaderMapImpl response_header{{"second", "PUT"}, {"test", "test"}}; Http::TestHeaderMapImpl response_trailer{{"third", "POST"}, {"test-2", "test-2"}}; - + envoy::api::v2::core::Metadata metadata; populateMetadataTestData(metadata); EXPECT_CALL(request_info, dynamicMetadata()).WillRepeatedly(ReturnRef(metadata)); std::map expected_json_map = { - {"test_key", "\"test_value\""}, - {"test_obj", "{\"inner_key\":\"inner_value\"}"}, - {"test_obj.inner_key", "\"inner_value\""} - }; + {"test_key", "\"test_value\""}, + {"test_obj", "{\"inner_key\":\"inner_value\"}"}, + {"test_obj.inner_key", "\"inner_value\""}}; const std::map key_mapping = { - {"test_key", "%DYNAMIC_METADATA(com.test:test_key)%"}, - {"test_obj", "%DYNAMIC_METADATA(com.test:test_obj)%"}, - {"test_obj.inner_key", "%DYNAMIC_METADATA(com.test:test_obj:inner_key)%"} - }; + {"test_key", "%DYNAMIC_METADATA(com.test:test_key)%"}, + {"test_obj", "%DYNAMIC_METADATA(com.test:test_obj)%"}, + {"test_obj.inner_key", "%DYNAMIC_METADATA(com.test:test_obj:inner_key)%"}}; JsonFormatterImpl formatter(key_mapping); - auto actual_json_map = formatter.to_map(request_header, response_header, response_trailer, request_info); + auto actual_json_map = + formatter.to_map(request_header, response_header, response_trailer, request_info); EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); - - auto actual_string = formatter.format(request_header, response_header, response_trailer, request_info); + + auto actual_string = + formatter.format(request_header, response_header, response_trailer, request_info); EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); } @@ -545,27 +546,27 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { gmtime_r(&test_epoch, &time_val); time_t expected_time_t = mktime(&time_val); - std::map expected_json_map = { - {"simple_date", "2018/03/28"}, - {"test_time", fmt::format("{}", expected_time_t)}, - {"bad_format", "bad_format"}, - {"default", "2018-03-28T23:35:58.000Z"}, - {"all_zeroes", "000000000.0.00.000"} - }; + std::map expected_json_map = { + {"simple_date", "2018/03/28"}, + {"test_time", fmt::format("{}", expected_time_t)}, + {"bad_format", "bad_format"}, + {"default", "2018-03-28T23:35:58.000Z"}, + {"all_zeroes", "000000000.0.00.000"}}; const std::map key_mapping = { - {"simple_date", "%START_TIME(%Y/%m/%d)%"}, - {"test_time", "%START_TIME(%s)%"}, - {"bad_format", "%START_TIME(bad_format)%"}, - {"default", "%START_TIME%"}, - {"all_zeroes", "%START_TIME(%f.%1f.%2f.%3f)%"} - }; + {"simple_date", "%START_TIME(%Y/%m/%d)%"}, + {"test_time", "%START_TIME(%s)%"}, + {"bad_format", "%START_TIME(bad_format)%"}, + {"default", "%START_TIME%"}, + {"all_zeroes", "%START_TIME(%f.%1f.%2f.%3f)%"}}; JsonFormatterImpl formatter(key_mapping); - auto actual_json_map = formatter.to_map(request_header, response_header, response_trailer, request_info); + auto actual_json_map = + formatter.to_map(request_header, response_header, response_trailer, request_info); EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); - - auto actual_string = formatter.format(request_header, response_header, response_trailer, request_info); + + auto actual_string = + formatter.format(request_header, response_header, response_trailer, request_info); EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); } } diff --git a/test/common/access_log/access_log_impl_test.cc b/test/common/access_log/access_log_impl_test.cc index 6f4418c4112fb..25820484cc7d1 100644 --- a/test/common/access_log/access_log_impl_test.cc +++ b/test/common/access_log/access_log_impl_test.cc @@ -40,7 +40,7 @@ parseAccessLogFromJson(const std::string& json_string) { auto json_object_ptr = Json::Factory::loadFromString(json_string); Config::FilterJson::translateAccessLog(*json_object_ptr, access_log); return access_log; -} +} envoy::config::filter::accesslog::v2::AccessLog parseAccessLogFromV2Yaml(const std::string& yaml) { envoy::config::filter::accesslog::v2::AccessLog access_log; From 8db57a7e2e4b427adabea9569c1a695b4d5321a4 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Tue, 9 Oct 2018 16:57:13 -0700 Subject: [PATCH 06/51] Run clang-format on test/extensions/access_loggers/file and switch from google::protobuf to Protobuf Signed-off-by: Aaltan Ahmad --- .../access_loggers/file/config_test.cc | 40 ++++++++++--------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/test/extensions/access_loggers/file/config_test.cc b/test/extensions/access_loggers/file/config_test.cc index 3fb1593cae1f3..34df84a8efd86 100644 --- a/test/extensions/access_loggers/file/config_test.cc +++ b/test/extensions/access_loggers/file/config_test.cc @@ -1,6 +1,7 @@ #include "envoy/config/accesslog/v2/file.pb.h" #include "envoy/registry/registry.h" +#include "common/protobuf/protobuf.h" #include "common/access_log/access_log_impl.h" #include "extensions/access_loggers/file/config.h" @@ -75,18 +76,18 @@ TEST(FileAccessLogConfigTest, FileAccessLogTest) { TEST(FileAccessLogConfigTest, FileAccessLogJsonTest) { envoy::config::filter::accesslog::v2::AccessLog config; - + envoy::config::accesslog::v2::FileAccessLog fal_config; fal_config.set_path("/dev/null"); - - // auto json_format2 = new envoy::config::accesslog::v2::FileAccessLogJsonFormat; - auto json_format = new google::protobuf::Struct; - google::protobuf::Value string_value; + + auto json_format = new Protobuf::Struct; + Protobuf::Value string_value; string_value.set_string_value("%PROTOCOL%"); (*json_format->mutable_fields())[std::string{"protocol"}] = string_value; fal_config.set_allocated_json_format(json_format); - EXPECT_EQ(fal_config.access_log_format_case(), envoy::config::accesslog::v2::FileAccessLog::kJsonFormat); + EXPECT_EQ(fal_config.access_log_format_case(), + envoy::config::accesslog::v2::FileAccessLog::kJsonFormat); MessageUtil::jsonConvert(fal_config, *config.mutable_config()); NiceMock context; @@ -112,17 +113,18 @@ TEST(FileAccessLogConfigTest, FileAccessLogJsonConversionTest) { config.set_name(AccessLogNames::get().File); envoy::config::accesslog::v2::FileAccessLog fal_config; fal_config.set_path("/dev/null"); - - auto json_format = new google::protobuf::Struct; - google::protobuf::Value string_value; + + auto json_format = new Protobuf::Struct; + Protobuf::Value string_value; string_value.set_bool_value(false); (*json_format->mutable_fields())[std::string{"protocol"}] = string_value; fal_config.set_allocated_json_format(json_format); - + MessageUtil::jsonConvert(fal_config, *config.mutable_config()); NiceMock context; - EXPECT_THROW_WITH_MESSAGE(AccessLog::AccessLogFactory::fromProto(config, context), EnvoyException, + EXPECT_THROW_WITH_MESSAGE(AccessLog::AccessLogFactory::fromProto(config, context), + EnvoyException, "Only string values are supported in the JSON access log format."); } @@ -131,27 +133,27 @@ TEST(FileAccessLogConfigTest, FileAccessLogJsonConversionTest) { config.set_name(AccessLogNames::get().File); envoy::config::accesslog::v2::FileAccessLog fal_config; fal_config.set_path("/dev/null"); - - auto json_format = new google::protobuf::Struct; - auto nested_struct = new google::protobuf::Struct; - google::protobuf::Value string_value; + + auto json_format = new Protobuf::Struct; + auto nested_struct = new Protobuf::Struct; + Protobuf::Value string_value; string_value.set_string_value(std::string{"some_nested_value"}); (*nested_struct->mutable_fields())[std::string{"some_nested_key"}] = string_value; - google::protobuf::Value struct_value; + Protobuf::Value struct_value; struct_value.set_allocated_struct_value(nested_struct); (*json_format->mutable_fields())[std::string{"top_level_key"}] = struct_value; fal_config.set_allocated_json_format(json_format); - + MessageUtil::jsonConvert(fal_config, *config.mutable_config()); NiceMock context; - EXPECT_THROW_WITH_MESSAGE(AccessLog::AccessLogFactory::fromProto(config, context), EnvoyException, + EXPECT_THROW_WITH_MESSAGE(AccessLog::AccessLogFactory::fromProto(config, context), + EnvoyException, "Only string values are supported in the JSON access log format."); } } - } // namespace File } // namespace AccessLoggers } // namespace Extensions From 6af83d572f08435d08d978e87ad4cad861c694d8 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Tue, 9 Oct 2018 17:02:27 -0700 Subject: [PATCH 07/51] Run clang-format on include/envoy/access_log Signed-off-by: Aaltan Ahmad --- include/envoy/access_log/access_log.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/envoy/access_log/access_log.h b/include/envoy/access_log/access_log.h index 4af48fce96bbe..377e4346d0476 100644 --- a/include/envoy/access_log/access_log.h +++ b/include/envoy/access_log/access_log.h @@ -92,9 +92,9 @@ class FormatterProvider { virtual ~FormatterProvider() {} virtual std::string format(const Http::HeaderMap& request_headers, - const Http::HeaderMap& response_headers, - const Http::HeaderMap& response_trailers, - const RequestInfo::RequestInfo& request_info) const PURE; + const Http::HeaderMap& response_headers, + const Http::HeaderMap& response_trailers, + const RequestInfo::RequestInfo& request_info) const PURE; }; typedef std::unique_ptr FormatterProviderPtr; From bded5f2968e3a42baca04a0b2bbc26d1618b4940 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Tue, 9 Oct 2018 17:03:48 -0700 Subject: [PATCH 08/51] Update exception message in config load Signed-off-by: Aaltan Ahmad --- source/extensions/access_loggers/file/config.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/access_loggers/file/config.cc b/source/extensions/access_loggers/file/config.cc index 6ba74389ddef2..b630cee1552ad 100644 --- a/source/extensions/access_loggers/file/config.cc +++ b/source/extensions/access_loggers/file/config.cc @@ -37,7 +37,7 @@ FileAccessLogFactory::createAccessLogInstance(const Protobuf::Message& config, const auto& json_format_map = this->convert_json_format_to_map(fal_config.json_format()); formatter.reset(new AccessLog::JsonFormatterImpl(json_format_map)); } else { - throw EnvoyException("Invalid access_log format provided"); + throw EnvoyException("Invalid access_log format provided. Only 'format' and 'json_format' are supported."); } return std::make_shared(fal_config.path(), std::move(filter), std::move(formatter), From ffd335891a70c6779a4d8090f016930bdbad308d Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Tue, 9 Oct 2018 17:06:31 -0700 Subject: [PATCH 09/51] Switch to using ProtobufWkt Signed-off-by: Aaltan Ahmad --- source/extensions/access_loggers/file/config.cc | 4 ++-- .../access_loggers/file/config_test.cc | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/source/extensions/access_loggers/file/config.cc b/source/extensions/access_loggers/file/config.cc index b630cee1552ad..7ccd90b872017 100644 --- a/source/extensions/access_loggers/file/config.cc +++ b/source/extensions/access_loggers/file/config.cc @@ -51,10 +51,10 @@ ProtobufTypes::MessagePtr FileAccessLogFactory::createEmptyConfigProto() { std::string FileAccessLogFactory::name() const { return AccessLogNames::get().File; } std::map -FileAccessLogFactory::convert_json_format_to_map(google::protobuf::Struct json_format) { +FileAccessLogFactory::convert_json_format_to_map(ProtobufWkt::Struct json_format) { std::map output; for (const auto& pair : json_format.fields()) { - if (pair.second.kind_case() != google::protobuf::Value::kStringValue) { + if (pair.second.kind_case() != ProtobufWkt::Value::kStringValue) { throw EnvoyException("Only string values are supported in the JSON access log format."); } output.emplace(pair.first, pair.second.string_value()); diff --git a/test/extensions/access_loggers/file/config_test.cc b/test/extensions/access_loggers/file/config_test.cc index 34df84a8efd86..d5e3104d805c4 100644 --- a/test/extensions/access_loggers/file/config_test.cc +++ b/test/extensions/access_loggers/file/config_test.cc @@ -80,8 +80,8 @@ TEST(FileAccessLogConfigTest, FileAccessLogJsonTest) { envoy::config::accesslog::v2::FileAccessLog fal_config; fal_config.set_path("/dev/null"); - auto json_format = new Protobuf::Struct; - Protobuf::Value string_value; + auto json_format = new ProtobufWkt::Struct; + ProtobufWkt::Value string_value; string_value.set_string_value("%PROTOCOL%"); (*json_format->mutable_fields())[std::string{"protocol"}] = string_value; fal_config.set_allocated_json_format(json_format); @@ -114,8 +114,8 @@ TEST(FileAccessLogConfigTest, FileAccessLogJsonConversionTest) { envoy::config::accesslog::v2::FileAccessLog fal_config; fal_config.set_path("/dev/null"); - auto json_format = new Protobuf::Struct; - Protobuf::Value string_value; + auto json_format = new ProtobufWkt::Struct; + ProtobufWkt::Value string_value; string_value.set_bool_value(false); (*json_format->mutable_fields())[std::string{"protocol"}] = string_value; fal_config.set_allocated_json_format(json_format); @@ -134,13 +134,13 @@ TEST(FileAccessLogConfigTest, FileAccessLogJsonConversionTest) { envoy::config::accesslog::v2::FileAccessLog fal_config; fal_config.set_path("/dev/null"); - auto json_format = new Protobuf::Struct; - auto nested_struct = new Protobuf::Struct; - Protobuf::Value string_value; + auto json_format = new ProtobufWkt::Struct; + auto nested_struct = new ProtobufWkt::Struct; + ProtobufWkt::Value string_value; string_value.set_string_value(std::string{"some_nested_value"}); (*nested_struct->mutable_fields())[std::string{"some_nested_key"}] = string_value; - Protobuf::Value struct_value; + ProtobufWkt::Value struct_value; struct_value.set_allocated_struct_value(nested_struct); (*json_format->mutable_fields())[std::string{"top_level_key"}] = struct_value; fal_config.set_allocated_json_format(json_format); From 7c777d355c0e1d2823fd847a8866362e5dbeb440 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Wed, 10 Oct 2018 17:03:21 -0700 Subject: [PATCH 10/51] Fix some format and ProtobufWkt issues Signed-off-by: Aaltan Ahmad --- source/extensions/access_loggers/file/config.cc | 3 ++- source/extensions/access_loggers/file/config.h | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/source/extensions/access_loggers/file/config.cc b/source/extensions/access_loggers/file/config.cc index 7ccd90b872017..753382f541025 100644 --- a/source/extensions/access_loggers/file/config.cc +++ b/source/extensions/access_loggers/file/config.cc @@ -37,7 +37,8 @@ FileAccessLogFactory::createAccessLogInstance(const Protobuf::Message& config, const auto& json_format_map = this->convert_json_format_to_map(fal_config.json_format()); formatter.reset(new AccessLog::JsonFormatterImpl(json_format_map)); } else { - throw EnvoyException("Invalid access_log format provided. Only 'format' and 'json_format' are supported."); + throw EnvoyException( + "Invalid access_log format provided. Only 'format' and 'json_format' are supported."); } return std::make_shared(fal_config.path(), std::move(filter), std::move(formatter), diff --git a/source/extensions/access_loggers/file/config.h b/source/extensions/access_loggers/file/config.h index 652219440905b..95cf4cf8e08f2 100644 --- a/source/extensions/access_loggers/file/config.h +++ b/source/extensions/access_loggers/file/config.h @@ -22,7 +22,7 @@ class FileAccessLogFactory : public Server::Configuration::AccessLogInstanceFact private: std::map - convert_json_format_to_map(google::protobuf::Struct config); + convert_json_format_to_map(ProtobufWkt::Struct config); }; } // namespace File From ed1d0d8d7cbbe789ddc7e082e96bfffb00449877 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Wed, 10 Oct 2018 17:31:51 -0700 Subject: [PATCH 11/51] Adopt RequestInfo -> StreamInfo in the JSON logging code Signed-off-by: Aaltan Ahmad --- include/envoy/access_log/access_log.h | 2 +- .../common/access_log/access_log_formatter.cc | 8 ++-- .../common/access_log/access_log_formatter.h | 4 +- .../access_log_formatter_fuzz_test.cc | 2 +- .../access_log/access_log_formatter_test.cc | 48 +++++++++---------- 5 files changed, 32 insertions(+), 32 deletions(-) diff --git a/include/envoy/access_log/access_log.h b/include/envoy/access_log/access_log.h index 377e4346d0476..2d328a22f4c8d 100644 --- a/include/envoy/access_log/access_log.h +++ b/include/envoy/access_log/access_log.h @@ -94,7 +94,7 @@ class FormatterProvider { virtual std::string format(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, const Http::HeaderMap& response_trailers, - const RequestInfo::RequestInfo& request_info) const PURE; + const StreamInfo::StreamInfo& request_info) const PURE; }; typedef std::unique_ptr FormatterProviderPtr; diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc index 668b06193cdd1..98517827a6774 100644 --- a/source/common/access_log/access_log_formatter.cc +++ b/source/common/access_log/access_log_formatter.cc @@ -97,12 +97,12 @@ JsonFormatterImpl::JsonFormatterImpl( std::string JsonFormatterImpl::format(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, const Http::HeaderMap& response_trailers, - const RequestInfo::RequestInfo& request_info) const { + const StreamInfo::StreamInfo& stream_info) const { rapidjson::StringBuffer strbuf; rapidjson::Writer writer(strbuf); auto output_map = - this->to_map(request_headers, response_headers, response_trailers, request_info); + this->to_map(request_headers, response_headers, response_trailers, stream_info); writer.StartObject(); for (const auto& pair : output_map) { writer.Key(pair.first.c_str()); @@ -114,11 +114,11 @@ std::string JsonFormatterImpl::format(const Http::HeaderMap& request_headers, std::map JsonFormatterImpl::to_map( const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, - const Http::HeaderMap& response_trailers, const RequestInfo::RequestInfo& request_info) const { + const Http::HeaderMap& response_trailers, const StreamInfo::StreamInfo& stream_info) const { std::map output; for (const auto& pair : json_output_format_) { output.emplace(pair.first, pair.second->format(request_headers, response_headers, - response_trailers, request_info)); + response_trailers, stream_info)); } return output; } diff --git a/source/common/access_log/access_log_formatter.h b/source/common/access_log/access_log_formatter.h index 45a34846d1fb8..22d26ba20cfb6 100644 --- a/source/common/access_log/access_log_formatter.h +++ b/source/common/access_log/access_log_formatter.h @@ -106,12 +106,12 @@ class JsonFormatterImpl : public Formatter { std::string format(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, const Http::HeaderMap& response_trailers, - const RequestInfo::RequestInfo& request_info) const override; + const StreamInfo::StreamInfo& stream_info) const override; std::map to_map(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, const Http::HeaderMap& response_trailers, - const RequestInfo::RequestInfo& request_info) const; + const StreamInfo::StreamInfo& stream_info) const; private: std::vector providers_; diff --git a/test/common/access_log/access_log_formatter_fuzz_test.cc b/test/common/access_log/access_log_formatter_fuzz_test.cc index 9a6175912da4f..f6bcfb4bda8c0 100644 --- a/test/common/access_log/access_log_formatter_fuzz_test.cc +++ b/test/common/access_log/access_log_formatter_fuzz_test.cc @@ -9,7 +9,7 @@ namespace Fuzz { DEFINE_PROTO_FUZZER(const test::common::access_log::TestCase& input) { try { - std::vector formatters = + std::vector formatters = AccessLog::AccessLogFormatParser::parse(input.format()); for (const auto& it : formatters) { it->format( diff --git a/test/common/access_log/access_log_formatter_test.cc b/test/common/access_log/access_log_formatter_test.cc index 5da0453a95897..0abe5609e4ee8 100644 --- a/test/common/access_log/access_log_formatter_test.cc +++ b/test/common/access_log/access_log_formatter_test.cc @@ -384,7 +384,7 @@ TEST(AccessLogFormatterTest, startTimeFormatter) { TEST(AccessLogFormatterTest, JsonFormatterTest) { { - RequestInfo::MockRequestInfo request_info; + StreamInfo::MockStreamInfo stream_info; Http::TestHeaderMapImpl request_header{}; Http::TestHeaderMapImpl response_header{}; Http::TestHeaderMapImpl response_trailer{}; @@ -392,7 +392,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { envoy::api::v2::core::Metadata metadata; populateMetadataTestData(metadata); absl::optional protocol = Http::Protocol::Http11; - EXPECT_CALL(request_info, protocol()).WillRepeatedly(Return(protocol)); + EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); std::map expected_json_map = {{"plain_string", "plain_string_value"}}; @@ -401,16 +401,16 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { JsonFormatterImpl formatter(key_mapping); auto actual_json_map = - formatter.to_map(request_header, response_header, response_trailer, request_info); + formatter.to_map(request_header, response_header, response_trailer, stream_info); EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); auto actual_string = - formatter.format(request_header, response_header, response_trailer, request_info); + formatter.format(request_header, response_header, response_trailer, stream_info); EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); } { - RequestInfo::MockRequestInfo request_info; + StreamInfo::MockStreamInfo stream_info; Http::TestHeaderMapImpl request_header{}; Http::TestHeaderMapImpl response_header{}; Http::TestHeaderMapImpl response_trailer{}; @@ -418,7 +418,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { envoy::api::v2::core::Metadata metadata; populateMetadataTestData(metadata); absl::optional protocol = Http::Protocol::Http11; - EXPECT_CALL(request_info, protocol()).WillRepeatedly(Return(protocol)); + EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); std::map expected_json_map = {{"protocol", "HTTP/1.1"}}; @@ -426,16 +426,16 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { JsonFormatterImpl formatter(key_mapping); auto actual_json_map = - formatter.to_map(request_header, response_header, response_trailer, request_info); + formatter.to_map(request_header, response_header, response_trailer, stream_info); EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); auto actual_string = - formatter.format(request_header, response_header, response_trailer, request_info); + formatter.format(request_header, response_header, response_trailer, stream_info); EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); } { - RequestInfo::MockRequestInfo request_info; + StreamInfo::MockStreamInfo stream_info; Http::TestHeaderMapImpl request_header{{"some_request_header", "SOME_REQUEST_HEADER"}}; Http::TestHeaderMapImpl response_header{{"some_response_header", "SOME_RESPONSE_HEADER"}}; Http::TestHeaderMapImpl response_trailer{}; @@ -454,19 +454,19 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { JsonFormatterImpl formatter(key_mapping); absl::optional protocol = Http::Protocol::Http11; - EXPECT_CALL(request_info, protocol()).WillRepeatedly(Return(protocol)); + EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); auto actual_json_map = - formatter.to_map(request_header, response_header, response_trailer, request_info); + formatter.to_map(request_header, response_header, response_trailer, stream_info); EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); auto actual_string = - formatter.format(request_header, response_header, response_trailer, request_info); + formatter.format(request_header, response_header, response_trailer, stream_info); EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); } { - RequestInfo::MockRequestInfo request_info; + StreamInfo::MockStreamInfo stream_info; Http::TestHeaderMapImpl request_header{{"request_present_header", "REQUEST_PRESENT_HEADER"}}; Http::TestHeaderMapImpl response_header{{"response_present_header", "RESPONSE_PRESENT_HEADER"}}; Http::TestHeaderMapImpl response_trailer{}; @@ -489,26 +489,26 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { JsonFormatterImpl formatter(key_mapping); absl::optional protocol = Http::Protocol::Http11; - EXPECT_CALL(request_info, protocol()).WillRepeatedly(Return(protocol)); + EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); auto actual_json_map = - formatter.to_map(request_header, response_header, response_trailer, request_info); + formatter.to_map(request_header, response_header, response_trailer, stream_info); EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); auto actual_string = - formatter.format(request_header, response_header, response_trailer, request_info); + formatter.format(request_header, response_header, response_trailer, stream_info); EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); } { - RequestInfo::MockRequestInfo request_info; + StreamInfo::MockStreamInfo stream_info; Http::TestHeaderMapImpl request_header{{"first", "GET"}, {":path", "/"}}; Http::TestHeaderMapImpl response_header{{"second", "PUT"}, {"test", "test"}}; Http::TestHeaderMapImpl response_trailer{{"third", "POST"}, {"test-2", "test-2"}}; envoy::api::v2::core::Metadata metadata; populateMetadataTestData(metadata); - EXPECT_CALL(request_info, dynamicMetadata()).WillRepeatedly(ReturnRef(metadata)); + EXPECT_CALL(stream_info, dynamicMetadata()).WillRepeatedly(ReturnRef(metadata)); std::map expected_json_map = { {"test_key", "\"test_value\""}, @@ -523,23 +523,23 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { JsonFormatterImpl formatter(key_mapping); auto actual_json_map = - formatter.to_map(request_header, response_header, response_trailer, request_info); + formatter.to_map(request_header, response_header, response_trailer, stream_info); EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); auto actual_string = - formatter.format(request_header, response_header, response_trailer, request_info); + formatter.format(request_header, response_header, response_trailer, stream_info); EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); } { - RequestInfo::MockRequestInfo request_info; + StreamInfo::MockStreamInfo stream_info; Http::TestHeaderMapImpl request_header{}; Http::TestHeaderMapImpl response_header{}; Http::TestHeaderMapImpl response_trailer{}; time_t test_epoch = 1522280158; SystemTime time = std::chrono::system_clock::from_time_t(test_epoch); - EXPECT_CALL(request_info, startTime()).WillRepeatedly(Return(time)); + EXPECT_CALL(stream_info, startTime()).WillRepeatedly(Return(time)); // Needed to take into account the behavior in non-GMT timezones. struct tm time_val; @@ -562,11 +562,11 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { JsonFormatterImpl formatter(key_mapping); auto actual_json_map = - formatter.to_map(request_header, response_header, response_trailer, request_info); + formatter.to_map(request_header, response_header, response_trailer, stream_info); EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); auto actual_string = - formatter.format(request_header, response_header, response_trailer, request_info); + formatter.format(request_header, response_header, response_trailer, stream_info); EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); } } From 4c7e000ffc982b7afd6af7d41f8603d2900a032d Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Wed, 10 Oct 2018 17:35:03 -0700 Subject: [PATCH 12/51] More format fixes Signed-off-by: Aaltan Ahmad --- source/common/access_log/access_log_formatter.cc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc index 98517827a6774..08ffee09b2bb3 100644 --- a/source/common/access_log/access_log_formatter.cc +++ b/source/common/access_log/access_log_formatter.cc @@ -67,8 +67,7 @@ std::string FormatterImpl::format(const Http::HeaderMap& request_headers, log_line.reserve(256); for (const FormatterProviderPtr& provider : providers_) { - log_line += - provider->format(request_headers, response_headers, response_trailers, stream_info); + log_line += provider->format(request_headers, response_headers, response_trailers, stream_info); } return log_line; @@ -101,8 +100,7 @@ std::string JsonFormatterImpl::format(const Http::HeaderMap& request_headers, rapidjson::StringBuffer strbuf; rapidjson::Writer writer(strbuf); - auto output_map = - this->to_map(request_headers, response_headers, response_trailers, stream_info); + auto output_map = this->to_map(request_headers, response_headers, response_trailers, stream_info); writer.StartObject(); for (const auto& pair : output_map) { writer.Key(pair.first.c_str()); From 216814f7491325b8dcb2dd4d6678091380c96726 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Wed, 10 Oct 2018 17:38:08 -0700 Subject: [PATCH 13/51] More format fixes Signed-off-by: Aaltan Ahmad --- source/common/router/header_formatter.h | 3 ++- test/extensions/access_loggers/file/config_test.cc | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/source/common/router/header_formatter.h b/source/common/router/header_formatter.h index 5e7ba71df7682..0c87edc12b644 100644 --- a/source/common/router/header_formatter.h +++ b/source/common/router/header_formatter.h @@ -43,7 +43,8 @@ class StreamInfoHeaderFormatter : public HeaderFormatter { private: std::function field_extractor_; const bool append_; - std::unordered_map> start_time_formatters_; + std::unordered_map> + start_time_formatters_; }; /** diff --git a/test/extensions/access_loggers/file/config_test.cc b/test/extensions/access_loggers/file/config_test.cc index d5e3104d805c4..bc64de7f6ef3f 100644 --- a/test/extensions/access_loggers/file/config_test.cc +++ b/test/extensions/access_loggers/file/config_test.cc @@ -1,8 +1,8 @@ #include "envoy/config/accesslog/v2/file.pb.h" #include "envoy/registry/registry.h" -#include "common/protobuf/protobuf.h" #include "common/access_log/access_log_impl.h" +#include "common/protobuf/protobuf.h" #include "extensions/access_loggers/file/config.h" #include "extensions/access_loggers/file/file_access_log_impl.h" From 777bd96cb8762496d2bce73ee6db49403786ac8d Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Wed, 10 Oct 2018 17:45:11 -0700 Subject: [PATCH 14/51] More format fixes Signed-off-by: Aaltan Ahmad --- source/common/access_log/access_log_formatter.cc | 3 +-- source/common/access_log/access_log_impl.cc | 3 +-- source/common/access_log/access_log_manager_impl.cc | 4 ++-- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc index 08ffee09b2bb3..2c0b0d2cabf40 100644 --- a/source/common/access_log/access_log_formatter.cc +++ b/source/common/access_log/access_log_formatter.cc @@ -1,9 +1,8 @@ -#include "common/access_log/access_log_formatter.h" - #include #include #include +#include "common/access_log/access_log_formatter.h" #include "common/common/assert.h" #include "common/common/fmt.h" #include "common/common/utility.h" diff --git a/source/common/access_log/access_log_impl.cc b/source/common/access_log/access_log_impl.cc index aa335854eaa5e..7f85426449db3 100644 --- a/source/common/access_log/access_log_impl.cc +++ b/source/common/access_log/access_log_impl.cc @@ -1,5 +1,3 @@ -#include "common/access_log/access_log_impl.h" - #include #include @@ -11,6 +9,7 @@ #include "envoy/upstream/upstream.h" #include "common/access_log/access_log_formatter.h" +#include "common/access_log/access_log_impl.h" #include "common/common/assert.h" #include "common/common/utility.h" #include "common/config/utility.h" diff --git a/source/common/access_log/access_log_manager_impl.cc b/source/common/access_log/access_log_manager_impl.cc index 56933387b348b..e2d10c20861f9 100644 --- a/source/common/access_log/access_log_manager_impl.cc +++ b/source/common/access_log/access_log_manager_impl.cc @@ -1,7 +1,7 @@ -#include "common/access_log/access_log_manager_impl.h" - #include +#include "common/access_log/access_log_manager_impl.h" + namespace Envoy { namespace AccessLog { From 906421348d85a95cf531a2f48b6d20112b6a9432 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Wed, 10 Oct 2018 17:46:04 -0700 Subject: [PATCH 15/51] More format fixes - header reordering Signed-off-by: Aaltan Ahmad --- source/extensions/access_loggers/file/config.cc | 3 +-- source/extensions/access_loggers/file/file_access_log_impl.cc | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/source/extensions/access_loggers/file/config.cc b/source/extensions/access_loggers/file/config.cc index 753382f541025..1228aa5cabf3c 100644 --- a/source/extensions/access_loggers/file/config.cc +++ b/source/extensions/access_loggers/file/config.cc @@ -1,5 +1,3 @@ -#include "extensions/access_loggers/file/config.h" - #include "envoy/config/accesslog/v2/file.pb.validate.h" #include "envoy/registry/registry.h" #include "envoy/server/filter_config.h" @@ -8,6 +6,7 @@ #include "common/common/logger.h" #include "common/protobuf/protobuf.h" +#include "extensions/access_loggers/file/config.h" #include "extensions/access_loggers/file/file_access_log_impl.h" #include "extensions/access_loggers/well_known_names.h" diff --git a/source/extensions/access_loggers/file/file_access_log_impl.cc b/source/extensions/access_loggers/file/file_access_log_impl.cc index e2344d2a2806b..64bcf4b8b9e7b 100644 --- a/source/extensions/access_loggers/file/file_access_log_impl.cc +++ b/source/extensions/access_loggers/file/file_access_log_impl.cc @@ -1,7 +1,7 @@ -#include "extensions/access_loggers/file/file_access_log_impl.h" - #include "common/http/header_map_impl.h" +#include "extensions/access_loggers/file/file_access_log_impl.h" + namespace Envoy { namespace Extensions { namespace AccessLoggers { From 1d23323b7fc439dba6a8eaa2fe142feb536452a2 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Thu, 11 Oct 2018 13:50:16 -0700 Subject: [PATCH 16/51] Fix formatting issues Signed-off-by: Aaltan Ahmad --- source/common/access_log/access_log_formatter.cc | 3 ++- source/common/access_log/access_log_impl.cc | 3 ++- source/common/access_log/access_log_manager_impl.cc | 4 ++-- source/extensions/access_loggers/file/config.cc | 3 ++- source/extensions/access_loggers/file/file_access_log_impl.cc | 4 ++-- 5 files changed, 10 insertions(+), 7 deletions(-) diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc index 2c0b0d2cabf40..08ffee09b2bb3 100644 --- a/source/common/access_log/access_log_formatter.cc +++ b/source/common/access_log/access_log_formatter.cc @@ -1,8 +1,9 @@ +#include "common/access_log/access_log_formatter.h" + #include #include #include -#include "common/access_log/access_log_formatter.h" #include "common/common/assert.h" #include "common/common/fmt.h" #include "common/common/utility.h" diff --git a/source/common/access_log/access_log_impl.cc b/source/common/access_log/access_log_impl.cc index 7f85426449db3..aa335854eaa5e 100644 --- a/source/common/access_log/access_log_impl.cc +++ b/source/common/access_log/access_log_impl.cc @@ -1,3 +1,5 @@ +#include "common/access_log/access_log_impl.h" + #include #include @@ -9,7 +11,6 @@ #include "envoy/upstream/upstream.h" #include "common/access_log/access_log_formatter.h" -#include "common/access_log/access_log_impl.h" #include "common/common/assert.h" #include "common/common/utility.h" #include "common/config/utility.h" diff --git a/source/common/access_log/access_log_manager_impl.cc b/source/common/access_log/access_log_manager_impl.cc index e2d10c20861f9..56933387b348b 100644 --- a/source/common/access_log/access_log_manager_impl.cc +++ b/source/common/access_log/access_log_manager_impl.cc @@ -1,7 +1,7 @@ -#include - #include "common/access_log/access_log_manager_impl.h" +#include + namespace Envoy { namespace AccessLog { diff --git a/source/extensions/access_loggers/file/config.cc b/source/extensions/access_loggers/file/config.cc index 1228aa5cabf3c..753382f541025 100644 --- a/source/extensions/access_loggers/file/config.cc +++ b/source/extensions/access_loggers/file/config.cc @@ -1,3 +1,5 @@ +#include "extensions/access_loggers/file/config.h" + #include "envoy/config/accesslog/v2/file.pb.validate.h" #include "envoy/registry/registry.h" #include "envoy/server/filter_config.h" @@ -6,7 +8,6 @@ #include "common/common/logger.h" #include "common/protobuf/protobuf.h" -#include "extensions/access_loggers/file/config.h" #include "extensions/access_loggers/file/file_access_log_impl.h" #include "extensions/access_loggers/well_known_names.h" diff --git a/source/extensions/access_loggers/file/file_access_log_impl.cc b/source/extensions/access_loggers/file/file_access_log_impl.cc index 64bcf4b8b9e7b..e2344d2a2806b 100644 --- a/source/extensions/access_loggers/file/file_access_log_impl.cc +++ b/source/extensions/access_loggers/file/file_access_log_impl.cc @@ -1,7 +1,7 @@ -#include "common/http/header_map_impl.h" - #include "extensions/access_loggers/file/file_access_log_impl.h" +#include "common/http/header_map_impl.h" + namespace Envoy { namespace Extensions { namespace AccessLoggers { From cc0090e111e62f1b835de673f50c893a07203afa Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Thu, 11 Oct 2018 15:22:11 -0700 Subject: [PATCH 17/51] Add docs for JSON logging Signed-off-by: Aaltan Ahmad --- api/envoy/config/accesslog/v2/file.proto | 5 +- docs/root/configuration/access_log.rst | 106 +++++++++++++++++------ 2 files changed, 85 insertions(+), 26 deletions(-) diff --git a/api/envoy/config/accesslog/v2/file.proto b/api/envoy/config/accesslog/v2/file.proto index cb348b88b4d87..1ddf8cf364794 100644 --- a/api/envoy/config/accesslog/v2/file.proto +++ b/api/envoy/config/accesslog/v2/file.proto @@ -17,9 +17,12 @@ message FileAccessLog { // Access log format. Envoy supports :ref:`custom access log formats // ` as well as a :ref:`default format - // `. + // `. oneof access_log_format { + // Access log :ref:`format string` string format = 2; + + // Access log :ref:`format dictionary` google.protobuf.Struct json_format = 3; } } diff --git a/docs/root/configuration/access_log.rst b/docs/root/configuration/access_log.rst index ee26fe37c2ffc..3ca0e5b4751df 100644 --- a/docs/root/configuration/access_log.rst +++ b/docs/root/configuration/access_log.rst @@ -17,16 +17,93 @@ Access logs are configured as part of the :ref:`HTTP connection manager config Format rules ------------ -The access log format string contains either command operators or other characters interpreted as a -plain string. The access log formatter does not make any assumptions about a new line separator, so one +Access log formats contain command operators that extract the relevant data and insert it. +They support two formats: :ref:`"format strings" ` and +:ref:`"format dictionaries" `. In both cases, the command operators +are used to extract the relevant data, which is then inserted into the specified log format. +Only one access log format may be specified at a time. + +.. _config_access_log_format_strings: + +Format Strings +-------------- + +Format strings are plain strings, specified using the ``format`` key. They may contain +either command operators or other characters interpreted as a plain string. +The access log formatter does not make any assumptions about a new line separator, so one has to specified as part of the format string. See the :ref:`default format ` for an example. -Note that the access log line will contain a '-' character for every not set/empty value. -The same format strings are used by different types of access logs (such as HTTP and TCP). Some +.. _config_access_log_default_format: + +Default Format String +--------------------- + +If custom format string is not specified, Envoy uses the following default format: + +.. code-block:: none + + [%START_TIME%] "%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH)% %PROTOCOL%" + %RESPONSE_CODE% %RESPONSE_FLAGS% %BYTES_RECEIVED% %BYTES_SENT% %DURATION% + %RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)% "%REQ(X-FORWARDED-FOR)%" "%REQ(USER-AGENT)%" + "%REQ(X-REQUEST-ID)%" "%REQ(:AUTHORITY)%" "%UPSTREAM_HOST%"\n + +Example of the default Envoy access log format: + +.. code-block:: none + + [2016-04-15T20:17:00.310Z] "POST /api/v1/locations HTTP/2" 204 - 154 0 226 100 "10.0.35.28" + "nsq2http" "cc21d9b0-cf5c-432b-8c7e-98aeb7988cd2" "locations" "tcp://10.0.2.1:80" + +.. _config_access_log_format_dictionaries: + +Format Dictionaries +------------------- + +Format dictionaries are dictionaries that specify a structured access log output format, +specified using the ``json_format`` key. This allows logs to be output in a structured format +such as JSON. +Similar to format strings, command operators are evaluated and their values inserted into the format +dictionary to construct the log output. + +For example, with the following format provided in the configuration: + +.. code-block:: json + + { + "config": { + "json_format": { + "protocol": "%PROTOCOL%", + "duration": "%DURATION%", + "my_custom_header": "%REQ(MY_CUSTOM_HEADER)%" + } + } + } + +The following JSON object would be written to the log file: + +.. code-block:: json + + {"protocol": "HTTP/1.1", "duration": "123", "my_custom_header": "value_of_MY_CUSTOM_HEADER"} + +This allows you to specify a custom key for each command operator. + +Format dictionaries have the following restrictions: + +* The dictionary must map strings to strings (specifically, strings to command operators). Nesting is not currently supported. +* Each value in the dictionary can only contain one command operator. + + +Command Operators +----------------- + +Command operators are used to extract values that will be inserted into the access logs. +The same operators are used by different types of access logs (such as HTTP and TCP). Some fields may have slightly different meanings, depending on what type of log it is. Differences are noted. +Note that if a value is not set/empty, the logs will contain a '-' character. + The following command operators are supported: .. _config_access_log_format_start_time: @@ -232,24 +309,3 @@ The following command operators are supported: String value set on ssl connection socket for Server Name Indication (SNI) TCP String value set on ssl connection socket for Server Name Indication (SNI) - -.. _config_access_log_default_format: - -Default format --------------- - -If custom format is not specified, Envoy uses the following default format: - -.. code-block:: none - - [%START_TIME%] "%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH)% %PROTOCOL%" - %RESPONSE_CODE% %RESPONSE_FLAGS% %BYTES_RECEIVED% %BYTES_SENT% %DURATION% - %RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)% "%REQ(X-FORWARDED-FOR)%" "%REQ(USER-AGENT)%" - "%REQ(X-REQUEST-ID)%" "%REQ(:AUTHORITY)%" "%UPSTREAM_HOST%"\n - -Example of the default Envoy access log format: - -.. code-block:: none - - [2016-04-15T20:17:00.310Z] "POST /api/v1/locations HTTP/2" 204 - 154 0 226 100 "10.0.35.28" - "nsq2http" "cc21d9b0-cf5c-432b-8c7e-98aeb7988cd2" "locations" "tcp://10.0.2.1:80" From ccaab7cfdf054d0a2e849f734b2e72b59a2179e0 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Thu, 11 Oct 2018 15:52:52 -0700 Subject: [PATCH 18/51] Format fixes in file.proto Signed-off-by: Aaltan Ahmad --- api/envoy/config/accesslog/v2/file.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/envoy/config/accesslog/v2/file.proto b/api/envoy/config/accesslog/v2/file.proto index 1ddf8cf364794..a53c5aab1e467 100644 --- a/api/envoy/config/accesslog/v2/file.proto +++ b/api/envoy/config/accesslog/v2/file.proto @@ -17,7 +17,7 @@ message FileAccessLog { // Access log format. Envoy supports :ref:`custom access log formats // ` as well as a :ref:`default format - // `. + // `. oneof access_log_format { // Access log :ref:`format string` string format = 2; From 234a68305eb1add227a5c83f24691273e4e82c7f Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Thu, 11 Oct 2018 17:07:26 -0700 Subject: [PATCH 19/51] Update comments in access_log.h Signed-off-by: Aaltan Ahmad --- include/envoy/access_log/access_log.h | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/include/envoy/access_log/access_log.h b/include/envoy/access_log/access_log.h index 2d328a22f4c8d..505e0d3dc208f 100644 --- a/include/envoy/access_log/access_log.h +++ b/include/envoy/access_log/access_log.h @@ -71,6 +71,7 @@ typedef std::shared_ptr InstanceSharedPtr; /** * Interface for access log formatter. + * Formatters combine the output of FormatterProviders into a log output line. */ class Formatter { public: @@ -85,12 +86,17 @@ class Formatter { typedef std::unique_ptr FormatterPtr; /** - * Interface for access log provider + * Interface for access log provider. + * FormatterProviders extract information from a request. */ class FormatterProvider { public: virtual ~FormatterProvider() {} + /** + * Extract a value from the provided headers/trailers/stream. + * @return a string containing the extracted value. + */ virtual std::string format(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, const Http::HeaderMap& response_trailers, From 9f7838109afdb74e34356cc6e1cb798a84593a69 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Thu, 11 Oct 2018 17:21:00 -0700 Subject: [PATCH 20/51] Switch to unordered_map Signed-off-by: Aaltan Ahmad --- source/common/access_log/access_log_formatter.cc | 4 ++-- source/common/access_log/access_log_formatter.h | 2 +- test/common/access_log/access_log_formatter_test.cc | 12 ++++++------ 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc index 08ffee09b2bb3..9be87c3382560 100644 --- a/source/common/access_log/access_log_formatter.cc +++ b/source/common/access_log/access_log_formatter.cc @@ -110,10 +110,10 @@ std::string JsonFormatterImpl::format(const Http::HeaderMap& request_headers, return fmt::format("{}\n", strbuf.GetString()); } -std::map JsonFormatterImpl::to_map( +std::unordered_map JsonFormatterImpl::to_map( const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, const Http::HeaderMap& response_trailers, const StreamInfo::StreamInfo& stream_info) const { - std::map output; + std::unordered_map output; for (const auto& pair : json_output_format_) { output.emplace(pair.first, pair.second->format(request_headers, response_headers, response_trailers, stream_info)); diff --git a/source/common/access_log/access_log_formatter.h b/source/common/access_log/access_log_formatter.h index 22d26ba20cfb6..969d3cd6efb8c 100644 --- a/source/common/access_log/access_log_formatter.h +++ b/source/common/access_log/access_log_formatter.h @@ -108,7 +108,7 @@ class JsonFormatterImpl : public Formatter { const Http::HeaderMap& response_trailers, const StreamInfo::StreamInfo& stream_info) const override; - std::map to_map(const Http::HeaderMap& request_headers, + std::unordered_map to_map(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, const Http::HeaderMap& response_trailers, const StreamInfo::StreamInfo& stream_info) const; diff --git a/test/common/access_log/access_log_formatter_test.cc b/test/common/access_log/access_log_formatter_test.cc index 0abe5609e4ee8..7af850842ac5f 100644 --- a/test/common/access_log/access_log_formatter_test.cc +++ b/test/common/access_log/access_log_formatter_test.cc @@ -394,7 +394,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { absl::optional protocol = Http::Protocol::Http11; EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); - std::map expected_json_map = {{"plain_string", "plain_string_value"}}; + std::unordered_map expected_json_map = {{"plain_string", "plain_string_value"}}; const std::map key_mapping = { {"plain_string", "plain_string_value"}}; @@ -420,7 +420,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { absl::optional protocol = Http::Protocol::Http11; EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); - std::map expected_json_map = {{"protocol", "HTTP/1.1"}}; + std::unordered_map expected_json_map = {{"protocol", "HTTP/1.1"}}; const std::map key_mapping = {{"protocol", "%PROTOCOL%"}}; JsonFormatterImpl formatter(key_mapping); @@ -440,7 +440,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { Http::TestHeaderMapImpl response_header{{"some_response_header", "SOME_RESPONSE_HEADER"}}; Http::TestHeaderMapImpl response_trailer{}; - std::map expected_json_map = { + std::unordered_map expected_json_map = { {"protocol", "HTTP/1.1"}, {"some_request_header", "SOME_REQUEST_HEADER"}, {"nonexistent_response_header", "-"}, @@ -471,7 +471,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { Http::TestHeaderMapImpl response_header{{"response_present_header", "RESPONSE_PRESENT_HEADER"}}; Http::TestHeaderMapImpl response_trailer{}; - std::map expected_json_map = { + std::unordered_map expected_json_map = { {"request_present_header_or_request_absent_header", "REQUEST_PRESENT_HEADER"}, {"request_absent_header_or_request_present_header", "REQUEST_PRESENT_HEADER"}, {"response_absent_header_or_response_absent_header", "RESPONSE_PRESENT_HEADER"}, @@ -510,7 +510,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { populateMetadataTestData(metadata); EXPECT_CALL(stream_info, dynamicMetadata()).WillRepeatedly(ReturnRef(metadata)); - std::map expected_json_map = { + std::unordered_map expected_json_map = { {"test_key", "\"test_value\""}, {"test_obj", "{\"inner_key\":\"inner_value\"}"}, {"test_obj.inner_key", "\"inner_value\""}}; @@ -546,7 +546,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { gmtime_r(&test_epoch, &time_val); time_t expected_time_t = mktime(&time_val); - std::map expected_json_map = { + std::unordered_map expected_json_map = { {"simple_date", "2018/03/28"}, {"test_time", fmt::format("{}", expected_time_t)}, {"bad_format", "bad_format"}, From f2b1173d85e8d2fc915bc7afd3b4c087b4f19dd5 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Thu, 11 Oct 2018 17:24:28 -0700 Subject: [PATCH 21/51] Case change convertJsonFormatToMap Signed-off-by: Aaltan Ahmad --- source/extensions/access_loggers/file/config.cc | 4 ++-- source/extensions/access_loggers/file/config.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/source/extensions/access_loggers/file/config.cc b/source/extensions/access_loggers/file/config.cc index 753382f541025..02a531abfe45a 100644 --- a/source/extensions/access_loggers/file/config.cc +++ b/source/extensions/access_loggers/file/config.cc @@ -34,7 +34,7 @@ FileAccessLogFactory::createAccessLogInstance(const Protobuf::Message& config, } } else if (fal_config.access_log_format_case() == envoy::config::accesslog::v2::FileAccessLog::kJsonFormat) { - const auto& json_format_map = this->convert_json_format_to_map(fal_config.json_format()); + const auto& json_format_map = this->convertJsonFormatToMap(fal_config.json_format()); formatter.reset(new AccessLog::JsonFormatterImpl(json_format_map)); } else { throw EnvoyException( @@ -52,7 +52,7 @@ ProtobufTypes::MessagePtr FileAccessLogFactory::createEmptyConfigProto() { std::string FileAccessLogFactory::name() const { return AccessLogNames::get().File; } std::map -FileAccessLogFactory::convert_json_format_to_map(ProtobufWkt::Struct json_format) { +FileAccessLogFactory::convertJsonFormatToMap(ProtobufWkt::Struct json_format) { std::map output; for (const auto& pair : json_format.fields()) { if (pair.second.kind_case() != ProtobufWkt::Value::kStringValue) { diff --git a/source/extensions/access_loggers/file/config.h b/source/extensions/access_loggers/file/config.h index 95cf4cf8e08f2..1e4a51fc14615 100644 --- a/source/extensions/access_loggers/file/config.h +++ b/source/extensions/access_loggers/file/config.h @@ -22,7 +22,7 @@ class FileAccessLogFactory : public Server::Configuration::AccessLogInstanceFact private: std::map - convert_json_format_to_map(ProtobufWkt::Struct config); + convertJsonFormatToMap(ProtobufWkt::Struct config); }; } // namespace File From 633365a18ad88a5fb59e201ab49d270cfc090f62 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Thu, 11 Oct 2018 17:42:28 -0700 Subject: [PATCH 22/51] Describe some test cases Signed-off-by: Aaltan Ahmad --- test/extensions/access_loggers/file/config_test.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/extensions/access_loggers/file/config_test.cc b/test/extensions/access_loggers/file/config_test.cc index bc64de7f6ef3f..4bc586b6de382 100644 --- a/test/extensions/access_loggers/file/config_test.cc +++ b/test/extensions/access_loggers/file/config_test.cc @@ -109,6 +109,7 @@ TEST(FileAccessLogConfigTest, FileAccessLogJsonTest) { TEST(FileAccessLogConfigTest, FileAccessLogJsonConversionTest) { { + // Make sure we fail if you set a bool value in the format dictionary envoy::config::filter::accesslog::v2::AccessLog config; config.set_name(AccessLogNames::get().File); envoy::config::accesslog::v2::FileAccessLog fal_config; @@ -129,6 +130,7 @@ TEST(FileAccessLogConfigTest, FileAccessLogJsonConversionTest) { } { + // Make sure we fail if you set a nested Struct value in the format dictionary envoy::config::filter::accesslog::v2::AccessLog config; config.set_name(AccessLogNames::get().File); envoy::config::accesslog::v2::FileAccessLog fal_config; From d700a6402bb437d39fac6b4ea0a5a7155b91deda Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Fri, 12 Oct 2018 13:17:46 -0700 Subject: [PATCH 23/51] Remove new usage from config_test Signed-off-by: Aaltan Ahmad --- .../access_loggers/file/config_test.cc | 23 ++++++++----------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/test/extensions/access_loggers/file/config_test.cc b/test/extensions/access_loggers/file/config_test.cc index 4bc586b6de382..e656aeec3c3e9 100644 --- a/test/extensions/access_loggers/file/config_test.cc +++ b/test/extensions/access_loggers/file/config_test.cc @@ -80,11 +80,11 @@ TEST(FileAccessLogConfigTest, FileAccessLogJsonTest) { envoy::config::accesslog::v2::FileAccessLog fal_config; fal_config.set_path("/dev/null"); - auto json_format = new ProtobufWkt::Struct; ProtobufWkt::Value string_value; string_value.set_string_value("%PROTOCOL%"); + + auto json_format = fal_config.mutable_json_format(); (*json_format->mutable_fields())[std::string{"protocol"}] = string_value; - fal_config.set_allocated_json_format(json_format); EXPECT_EQ(fal_config.access_log_format_case(), envoy::config::accesslog::v2::FileAccessLog::kJsonFormat); @@ -115,11 +115,10 @@ TEST(FileAccessLogConfigTest, FileAccessLogJsonConversionTest) { envoy::config::accesslog::v2::FileAccessLog fal_config; fal_config.set_path("/dev/null"); - auto json_format = new ProtobufWkt::Struct; - ProtobufWkt::Value string_value; - string_value.set_bool_value(false); - (*json_format->mutable_fields())[std::string{"protocol"}] = string_value; - fal_config.set_allocated_json_format(json_format); + ProtobufWkt::Value bool_value; + bool_value.set_bool_value(false); + auto json_format = fal_config.mutable_json_format(); + (*json_format->mutable_fields())[std::string{"protocol"}] = bool_value; MessageUtil::jsonConvert(fal_config, *config.mutable_config()); NiceMock context; @@ -135,17 +134,15 @@ TEST(FileAccessLogConfigTest, FileAccessLogJsonConversionTest) { config.set_name(AccessLogNames::get().File); envoy::config::accesslog::v2::FileAccessLog fal_config; fal_config.set_path("/dev/null"); - - auto json_format = new ProtobufWkt::Struct; - auto nested_struct = new ProtobufWkt::Struct; + ProtobufWkt::Value string_value; string_value.set_string_value(std::string{"some_nested_value"}); - (*nested_struct->mutable_fields())[std::string{"some_nested_key"}] = string_value; ProtobufWkt::Value struct_value; - struct_value.set_allocated_struct_value(nested_struct); + (*struct_value.mutable_struct_value()->mutable_fields())[std::string{"some_nested_key"}] = string_value; + + auto json_format = fal_config.mutable_json_format(); (*json_format->mutable_fields())[std::string{"top_level_key"}] = struct_value; - fal_config.set_allocated_json_format(json_format); MessageUtil::jsonConvert(fal_config, *config.mutable_config()); NiceMock context; From b8a19d83ceca7f4ebbed27d37397e03832dd1020 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Fri, 12 Oct 2018 13:47:33 -0700 Subject: [PATCH 24/51] Use protobuf to serialize json output, instead of rapidjson Signed-off-by: Aaltan Ahmad --- .../common/access_log/access_log_formatter.cc | 20 +++++++++---------- .../access_log/access_log_formatter_test.cc | 2 +- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc index 9be87c3382560..81807f106a4d5 100644 --- a/source/common/access_log/access_log_formatter.cc +++ b/source/common/access_log/access_log_formatter.cc @@ -13,8 +13,6 @@ #include "absl/strings/str_split.h" #include "fmt/format.h" -#include "rapidjson/prettywriter.h" -#include "rapidjson/stringbuffer.h" using Envoy::Config::Metadata; @@ -97,17 +95,19 @@ std::string JsonFormatterImpl::format(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, const Http::HeaderMap& response_trailers, const StreamInfo::StreamInfo& stream_info) const { - rapidjson::StringBuffer strbuf; - rapidjson::Writer writer(strbuf); - auto output_map = this->to_map(request_headers, response_headers, response_trailers, stream_info); - writer.StartObject(); + + ProtobufWkt::Struct output_struct; for (const auto& pair : output_map) { - writer.Key(pair.first.c_str()); - writer.String(pair.second.c_str()); + Protobuf::Value string_value; + string_value.set_string_value(pair.second); + (*output_struct.mutable_fields())[pair.first] = string_value; } - writer.EndObject(); - return fmt::format("{}\n", strbuf.GetString()); + + std::string log_line; + ProtobufUtil::MessageToJsonString(output_struct, &log_line); + + return log_line; } std::unordered_map JsonFormatterImpl::to_map( diff --git a/test/common/access_log/access_log_formatter_test.cc b/test/common/access_log/access_log_formatter_test.cc index 7af850842ac5f..f1490aa99f3ba 100644 --- a/test/common/access_log/access_log_formatter_test.cc +++ b/test/common/access_log/access_log_formatter_test.cc @@ -452,7 +452,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { {"nonexistent_response_header", "%RESP(nonexistent_response_header)%"}, {"some_response_header", "%RESP(some_response_header)%"}}; JsonFormatterImpl formatter(key_mapping); - + absl::optional protocol = Http::Protocol::Http11; EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); From 64822356179af68d05f68597172f88ece3f6c925 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Fri, 12 Oct 2018 13:55:46 -0700 Subject: [PATCH 25/51] Check error value after serializing to JSON Signed-off-by: Aaltan Ahmad --- source/common/access_log/access_log_formatter.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc index 81807f106a4d5..b440692134b78 100644 --- a/source/common/access_log/access_log_formatter.cc +++ b/source/common/access_log/access_log_formatter.cc @@ -105,7 +105,10 @@ std::string JsonFormatterImpl::format(const Http::HeaderMap& request_headers, } std::string log_line; - ProtobufUtil::MessageToJsonString(output_struct, &log_line); + auto conversion_status = ProtobufUtil::MessageToJsonString(output_struct, &log_line); + if (!conversion_status.ok()) { + throw EnvoyException(fmt::format("Unable to convert access log to json: {}", conversion_status.ToString())); + } return log_line; } From 53e066b8634306caa09b962aa556e33582d2367e Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Fri, 12 Oct 2018 13:56:41 -0700 Subject: [PATCH 26/51] Use ProtobufWkt Signed-off-by: Aaltan Ahmad --- source/common/access_log/access_log_formatter.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc index b440692134b78..3301ed8258c28 100644 --- a/source/common/access_log/access_log_formatter.cc +++ b/source/common/access_log/access_log_formatter.cc @@ -99,7 +99,7 @@ std::string JsonFormatterImpl::format(const Http::HeaderMap& request_headers, ProtobufWkt::Struct output_struct; for (const auto& pair : output_map) { - Protobuf::Value string_value; + ProtobufWkt::Value string_value; string_value.set_string_value(pair.second); (*output_struct.mutable_fields())[pair.first] = string_value; } From 195c1be4f6e7bbbfc9894a9d522953f39ee26bdf Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Fri, 12 Oct 2018 13:58:18 -0700 Subject: [PATCH 27/51] Formatting fix Signed-off-by: Aaltan Ahmad --- source/common/access_log/access_log_formatter.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc index 3301ed8258c28..7c95d9257714f 100644 --- a/source/common/access_log/access_log_formatter.cc +++ b/source/common/access_log/access_log_formatter.cc @@ -107,7 +107,8 @@ std::string JsonFormatterImpl::format(const Http::HeaderMap& request_headers, std::string log_line; auto conversion_status = ProtobufUtil::MessageToJsonString(output_struct, &log_line); if (!conversion_status.ok()) { - throw EnvoyException(fmt::format("Unable to convert access log to json: {}", conversion_status.ToString())); + throw EnvoyException( + fmt::format("Unable to convert access log to json: {}", conversion_status.ToString())); } return log_line; From 7dfd3165338cdc2feefbd536b39f90d9d1df9441 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Fri, 12 Oct 2018 14:03:05 -0700 Subject: [PATCH 28/51] Rename to_map to toMap Signed-off-by: Aaltan Ahmad --- source/common/access_log/access_log_formatter.cc | 4 ++-- source/common/access_log/access_log_formatter.h | 7 +++---- test/common/access_log/access_log_formatter_test.cc | 12 ++++++------ 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc index 7c95d9257714f..3abc58bc4ccb3 100644 --- a/source/common/access_log/access_log_formatter.cc +++ b/source/common/access_log/access_log_formatter.cc @@ -95,7 +95,7 @@ std::string JsonFormatterImpl::format(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, const Http::HeaderMap& response_trailers, const StreamInfo::StreamInfo& stream_info) const { - auto output_map = this->to_map(request_headers, response_headers, response_trailers, stream_info); + auto output_map = this->toMap(request_headers, response_headers, response_trailers, stream_info); ProtobufWkt::Struct output_struct; for (const auto& pair : output_map) { @@ -114,7 +114,7 @@ std::string JsonFormatterImpl::format(const Http::HeaderMap& request_headers, return log_line; } -std::unordered_map JsonFormatterImpl::to_map( +std::unordered_map JsonFormatterImpl::toMap( const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, const Http::HeaderMap& response_trailers, const StreamInfo::StreamInfo& stream_info) const { std::unordered_map output; diff --git a/source/common/access_log/access_log_formatter.h b/source/common/access_log/access_log_formatter.h index 969d3cd6efb8c..b148f5c023052 100644 --- a/source/common/access_log/access_log_formatter.h +++ b/source/common/access_log/access_log_formatter.h @@ -108,10 +108,9 @@ class JsonFormatterImpl : public Formatter { const Http::HeaderMap& response_trailers, const StreamInfo::StreamInfo& stream_info) const override; - std::unordered_map to_map(const Http::HeaderMap& request_headers, - const Http::HeaderMap& response_headers, - const Http::HeaderMap& response_trailers, - const StreamInfo::StreamInfo& stream_info) const; + std::unordered_map + toMap(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, + const Http::HeaderMap& response_trailers, const StreamInfo::StreamInfo& stream_info) const; private: std::vector providers_; diff --git a/test/common/access_log/access_log_formatter_test.cc b/test/common/access_log/access_log_formatter_test.cc index f1490aa99f3ba..5080260494928 100644 --- a/test/common/access_log/access_log_formatter_test.cc +++ b/test/common/access_log/access_log_formatter_test.cc @@ -401,7 +401,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { JsonFormatterImpl formatter(key_mapping); auto actual_json_map = - formatter.to_map(request_header, response_header, response_trailer, stream_info); + formatter.toMap(request_header, response_header, response_trailer, stream_info); EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); auto actual_string = @@ -426,7 +426,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { JsonFormatterImpl formatter(key_mapping); auto actual_json_map = - formatter.to_map(request_header, response_header, response_trailer, stream_info); + formatter.toMap(request_header, response_header, response_trailer, stream_info); EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); auto actual_string = @@ -457,7 +457,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); auto actual_json_map = - formatter.to_map(request_header, response_header, response_trailer, stream_info); + formatter.toMap(request_header, response_header, response_trailer, stream_info); EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); auto actual_string = @@ -492,7 +492,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); auto actual_json_map = - formatter.to_map(request_header, response_header, response_trailer, stream_info); + formatter.toMap(request_header, response_header, response_trailer, stream_info); EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); auto actual_string = @@ -523,7 +523,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { JsonFormatterImpl formatter(key_mapping); auto actual_json_map = - formatter.to_map(request_header, response_header, response_trailer, stream_info); + formatter.toMap(request_header, response_header, response_trailer, stream_info); EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); auto actual_string = @@ -562,7 +562,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { JsonFormatterImpl formatter(key_mapping); auto actual_json_map = - formatter.to_map(request_header, response_header, response_trailer, stream_info); + formatter.toMap(request_header, response_header, response_trailer, stream_info); EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); auto actual_string = From 9d0ab166366781d68ac5269fc5066e1ff5fc400e Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Fri, 12 Oct 2018 14:03:38 -0700 Subject: [PATCH 29/51] Formatting fixes Signed-off-by: Aaltan Ahmad --- source/common/access_log/access_log_formatter.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/access_log/access_log_formatter.h b/source/common/access_log/access_log_formatter.h index b148f5c023052..c95fc6e259840 100644 --- a/source/common/access_log/access_log_formatter.h +++ b/source/common/access_log/access_log_formatter.h @@ -110,7 +110,7 @@ class JsonFormatterImpl : public Formatter { std::unordered_map toMap(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, - const Http::HeaderMap& response_trailers, const StreamInfo::StreamInfo& stream_info) const; + const Http::HeaderMap& response_trailers, const StreamInfo::StreamInfo& stream_info) const; private: std::vector providers_; From 6dc68696d0c0e90352083f3a82fe05993fa7bc38 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Fri, 12 Oct 2018 14:04:15 -0700 Subject: [PATCH 30/51] Formatting fixes Signed-off-by: Aaltan Ahmad --- source/extensions/access_loggers/file/config.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/extensions/access_loggers/file/config.h b/source/extensions/access_loggers/file/config.h index 1e4a51fc14615..c3804de0b73ea 100644 --- a/source/extensions/access_loggers/file/config.h +++ b/source/extensions/access_loggers/file/config.h @@ -21,8 +21,7 @@ class FileAccessLogFactory : public Server::Configuration::AccessLogInstanceFact std::string name() const override; private: - std::map - convertJsonFormatToMap(ProtobufWkt::Struct config); + std::map convertJsonFormatToMap(ProtobufWkt::Struct config); }; } // namespace File From 258a03dcb97c2ea61c485cc9e0690487209acff5 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Fri, 12 Oct 2018 14:05:15 -0700 Subject: [PATCH 31/51] Formatting fixes Signed-off-by: Aaltan Ahmad --- test/common/access_log/access_log_formatter_test.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/common/access_log/access_log_formatter_test.cc b/test/common/access_log/access_log_formatter_test.cc index 5080260494928..fba084dbc39a7 100644 --- a/test/common/access_log/access_log_formatter_test.cc +++ b/test/common/access_log/access_log_formatter_test.cc @@ -394,7 +394,8 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { absl::optional protocol = Http::Protocol::Http11; EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); - std::unordered_map expected_json_map = {{"plain_string", "plain_string_value"}}; + std::unordered_map expected_json_map = { + {"plain_string", "plain_string_value"}}; const std::map key_mapping = { {"plain_string", "plain_string_value"}}; @@ -452,7 +453,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { {"nonexistent_response_header", "%RESP(nonexistent_response_header)%"}, {"some_response_header", "%RESP(some_response_header)%"}}; JsonFormatterImpl formatter(key_mapping); - + absl::optional protocol = Http::Protocol::Http11; EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); From 76bc028b4d955e1da07cd24c1592a728b20bc5a5 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Fri, 12 Oct 2018 14:05:44 -0700 Subject: [PATCH 32/51] Formatting fixes Signed-off-by: Aaltan Ahmad --- test/extensions/access_loggers/file/config_test.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/extensions/access_loggers/file/config_test.cc b/test/extensions/access_loggers/file/config_test.cc index e656aeec3c3e9..af320097386f3 100644 --- a/test/extensions/access_loggers/file/config_test.cc +++ b/test/extensions/access_loggers/file/config_test.cc @@ -134,13 +134,14 @@ TEST(FileAccessLogConfigTest, FileAccessLogJsonConversionTest) { config.set_name(AccessLogNames::get().File); envoy::config::accesslog::v2::FileAccessLog fal_config; fal_config.set_path("/dev/null"); - + ProtobufWkt::Value string_value; string_value.set_string_value(std::string{"some_nested_value"}); ProtobufWkt::Value struct_value; - (*struct_value.mutable_struct_value()->mutable_fields())[std::string{"some_nested_key"}] = string_value; - + (*struct_value.mutable_struct_value()->mutable_fields())[std::string{"some_nested_key"}] = + string_value; + auto json_format = fal_config.mutable_json_format(); (*json_format->mutable_fields())[std::string{"top_level_key"}] = struct_value; From 7f10f1ae5aee3b9890fae3f7b6622e1da2f61761 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Fri, 12 Oct 2018 16:53:44 -0700 Subject: [PATCH 33/51] Kick CI Signed-off-by: Aaltan Ahmad From 034dffc8ab69130a5e0742a04a908a64f7f40451 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Mon, 15 Oct 2018 10:52:48 -0700 Subject: [PATCH 34/51] Add doxygen comments and rename a param Signed-off-by: Aaltan Ahmad --- include/envoy/access_log/access_log.h | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/include/envoy/access_log/access_log.h b/include/envoy/access_log/access_log.h index 505e0d3dc208f..d7082961729d3 100644 --- a/include/envoy/access_log/access_log.h +++ b/include/envoy/access_log/access_log.h @@ -77,6 +77,14 @@ class Formatter { public: virtual ~Formatter() {} + /** + * Return a formatted access log line. + * @param request_headers supplies thte request headers. + * @param response_headers supplies the response headers. + * @param response_trailes supplies the response trailers. + * @param stream_info supplies the stream info. + * @return a string containing the complete formatted access log line. + */ virtual std::string format(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, const Http::HeaderMap& response_trailers, @@ -95,12 +103,16 @@ class FormatterProvider { /** * Extract a value from the provided headers/trailers/stream. - * @return a string containing the extracted value. + * @param request_headers supplies thte request headers. + * @param response_headers supplies the response headers. + * @param response_trailes supplies the response trailers. + * @param stream_info supplies the stream info. + * @return a string containing a single value extracted from the provided headers/stream. */ virtual std::string format(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, const Http::HeaderMap& response_trailers, - const StreamInfo::StreamInfo& request_info) const PURE; + const StreamInfo::StreamInfo& stream_info) const PURE; }; typedef std::unique_ptr FormatterProviderPtr; From ee3f1dcdd89b091ae614a08350c5ca1036a58e38 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Mon, 15 Oct 2018 10:59:38 -0700 Subject: [PATCH 35/51] Fix spelling Signed-off-by: Aaltan Ahmad --- include/envoy/access_log/access_log.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/envoy/access_log/access_log.h b/include/envoy/access_log/access_log.h index d7082961729d3..e0d6582b022df 100644 --- a/include/envoy/access_log/access_log.h +++ b/include/envoy/access_log/access_log.h @@ -81,7 +81,7 @@ class Formatter { * Return a formatted access log line. * @param request_headers supplies thte request headers. * @param response_headers supplies the response headers. - * @param response_trailes supplies the response trailers. + * @param response_trailers supplies the response trailers. * @param stream_info supplies the stream info. * @return a string containing the complete formatted access log line. */ @@ -105,7 +105,7 @@ class FormatterProvider { * Extract a value from the provided headers/trailers/stream. * @param request_headers supplies thte request headers. * @param response_headers supplies the response headers. - * @param response_trailes supplies the response trailers. + * @param response_trailers supplies the response trailers. * @param stream_info supplies the stream info. * @return a string containing a single value extracted from the provided headers/stream. */ From 6273f4bb2324707e69e064931742e4f6396fe364 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Tue, 16 Oct 2018 15:09:11 -0700 Subject: [PATCH 36/51] Fix some wording Signed-off-by: Aaltan Ahmad --- include/envoy/access_log/access_log.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/include/envoy/access_log/access_log.h b/include/envoy/access_log/access_log.h index e0d6582b022df..5f04026a2aae0 100644 --- a/include/envoy/access_log/access_log.h +++ b/include/envoy/access_log/access_log.h @@ -71,7 +71,7 @@ typedef std::shared_ptr InstanceSharedPtr; /** * Interface for access log formatter. - * Formatters combine the output of FormatterProviders into a log output line. + * Formatters provide a complete access log output line for the given headers/trailers/stream. */ class Formatter { public: @@ -79,11 +79,11 @@ class Formatter { /** * Return a formatted access log line. - * @param request_headers supplies thte request headers. + * @param request_headers supplies the request headers. * @param response_headers supplies the response headers. * @param response_trailers supplies the response trailers. * @param stream_info supplies the stream info. - * @return a string containing the complete formatted access log line. + * @return std::string string containing the complete formatted access log line. */ virtual std::string format(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, @@ -95,7 +95,7 @@ typedef std::unique_ptr FormatterPtr; /** * Interface for access log provider. - * FormatterProviders extract information from a request. + * FormatterProviders extract information from the given headers/trailers/stream. */ class FormatterProvider { public: @@ -103,11 +103,11 @@ class FormatterProvider { /** * Extract a value from the provided headers/trailers/stream. - * @param request_headers supplies thte request headers. + * @param request_headers supplies the request headers. * @param response_headers supplies the response headers. * @param response_trailers supplies the response trailers. * @param stream_info supplies the stream info. - * @return a string containing a single value extracted from the provided headers/stream. + * @return std::string containing a single value extracted from the given headers/trailers/stream. */ virtual std::string format(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, From 588a28f99b7d19d9c0794c553c212da95391a1cb Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Tue, 16 Oct 2018 15:43:44 -0700 Subject: [PATCH 37/51] Make toMap private and compare parsed JSON in unit tests Signed-off-by: Aaltan Ahmad --- .../common/access_log/access_log_formatter.h | 8 +-- .../access_log/access_log_formatter_test.cc | 66 +++++++------------ 2 files changed, 28 insertions(+), 46 deletions(-) diff --git a/source/common/access_log/access_log_formatter.h b/source/common/access_log/access_log_formatter.h index c95fc6e259840..ed2d67201fb69 100644 --- a/source/common/access_log/access_log_formatter.h +++ b/source/common/access_log/access_log_formatter.h @@ -108,13 +108,13 @@ class JsonFormatterImpl : public Formatter { const Http::HeaderMap& response_trailers, const StreamInfo::StreamInfo& stream_info) const override; - std::unordered_map - toMap(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, - const Http::HeaderMap& response_trailers, const StreamInfo::StreamInfo& stream_info) const; - private: std::vector providers_; std::map json_output_format_; + + std::unordered_map + toMap(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, + const Http::HeaderMap& response_trailers, const StreamInfo::StreamInfo& stream_info) const; }; /** diff --git a/test/common/access_log/access_log_formatter_test.cc b/test/common/access_log/access_log_formatter_test.cc index fba084dbc39a7..ca93847c22434 100644 --- a/test/common/access_log/access_log_formatter_test.cc +++ b/test/common/access_log/access_log_formatter_test.cc @@ -401,13 +401,10 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { {"plain_string", "plain_string_value"}}; JsonFormatterImpl formatter(key_mapping); - auto actual_json_map = - formatter.toMap(request_header, response_header, response_trailer, stream_info); - EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); - - auto actual_string = - formatter.format(request_header, response_header, response_trailer, stream_info); - EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); + const auto parsed = Json::Factory::loadFromString(formatter.format(request_header, response_header, response_trailer, stream_info)); + for (const auto& pair : expected_json_map) { + EXPECT_EQ(parsed->getString(pair.first), pair.second); + } } { @@ -426,13 +423,10 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { const std::map key_mapping = {{"protocol", "%PROTOCOL%"}}; JsonFormatterImpl formatter(key_mapping); - auto actual_json_map = - formatter.toMap(request_header, response_header, response_trailer, stream_info); - EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); - - auto actual_string = - formatter.format(request_header, response_header, response_trailer, stream_info); - EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); + const auto parsed = Json::Factory::loadFromString(formatter.format(request_header, response_header, response_trailer, stream_info)); + for (const auto& pair : expected_json_map) { + EXPECT_EQ(parsed->getString(pair.first), pair.second); + } } { @@ -457,13 +451,10 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { absl::optional protocol = Http::Protocol::Http11; EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); - auto actual_json_map = - formatter.toMap(request_header, response_header, response_trailer, stream_info); - EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); - - auto actual_string = - formatter.format(request_header, response_header, response_trailer, stream_info); - EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); + const auto parsed = Json::Factory::loadFromString(formatter.format(request_header, response_header, response_trailer, stream_info)); + for (const auto& pair : expected_json_map) { + EXPECT_EQ(parsed->getString(pair.first), pair.second); + } } { @@ -492,13 +483,10 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { absl::optional protocol = Http::Protocol::Http11; EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); - auto actual_json_map = - formatter.toMap(request_header, response_header, response_trailer, stream_info); - EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); - - auto actual_string = - formatter.format(request_header, response_header, response_trailer, stream_info); - EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); + const auto parsed = Json::Factory::loadFromString(formatter.format(request_header, response_header, response_trailer, stream_info)); + for (const auto& pair : expected_json_map) { + EXPECT_EQ(parsed->getString(pair.first), pair.second); + } } { @@ -523,13 +511,10 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { JsonFormatterImpl formatter(key_mapping); - auto actual_json_map = - formatter.toMap(request_header, response_header, response_trailer, stream_info); - EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); - - auto actual_string = - formatter.format(request_header, response_header, response_trailer, stream_info); - EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); + const auto parsed = Json::Factory::loadFromString(formatter.format(request_header, response_header, response_trailer, stream_info)); + for (const auto& pair : expected_json_map) { + EXPECT_EQ(parsed->getString(pair.first), pair.second); + } } { @@ -562,13 +547,10 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { {"all_zeroes", "%START_TIME(%f.%1f.%2f.%3f)%"}}; JsonFormatterImpl formatter(key_mapping); - auto actual_json_map = - formatter.toMap(request_header, response_header, response_trailer, stream_info); - EXPECT_THAT(actual_json_map, ::testing::ContainerEq(expected_json_map)); - - auto actual_string = - formatter.format(request_header, response_header, response_trailer, stream_info); - EXPECT_NO_THROW(Json::Factory::loadFromString(actual_string)); + const auto parsed = Json::Factory::loadFromString(formatter.format(request_header, response_header, response_trailer, stream_info)); + for (const auto& pair : expected_json_map) { + EXPECT_EQ(parsed->getString(pair.first), pair.second); + } } } From 85e05f9bd5ae066c46928a445628cca9b4202c4d Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Tue, 16 Oct 2018 15:47:29 -0700 Subject: [PATCH 38/51] Log an error instead of throwing an exception on proto -> json conversion failure Signed-off-by: Aaltan Ahmad --- source/common/access_log/access_log_formatter.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc index 3abc58bc4ccb3..f3fc7ad8fa493 100644 --- a/source/common/access_log/access_log_formatter.cc +++ b/source/common/access_log/access_log_formatter.cc @@ -107,8 +107,7 @@ std::string JsonFormatterImpl::format(const Http::HeaderMap& request_headers, std::string log_line; auto conversion_status = ProtobufUtil::MessageToJsonString(output_struct, &log_line); if (!conversion_status.ok()) { - throw EnvoyException( - fmt::format("Unable to convert access log to json: {}", conversion_status.ToString())); + log_line = fmt::format("Error serializing access log to JSON: {}", conversion_status.ToString()); } return log_line; From a3e4410d5c83ade7ecdb7989807168990e8ab3bb Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Wed, 17 Oct 2018 11:44:11 -0700 Subject: [PATCH 39/51] Switch to unordered_map for config.cc Signed-off-by: Aaltan Ahmad --- source/common/access_log/access_log_formatter.cc | 3 ++- source/common/access_log/access_log_formatter.h | 3 ++- source/extensions/access_loggers/file/config.cc | 9 +++++---- source/extensions/access_loggers/file/config.h | 2 +- 4 files changed, 10 insertions(+), 7 deletions(-) diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc index f3fc7ad8fa493..ce2e2b389f7b1 100644 --- a/source/common/access_log/access_log_formatter.cc +++ b/source/common/access_log/access_log_formatter.cc @@ -4,6 +4,7 @@ #include #include + #include "common/common/assert.h" #include "common/common/fmt.h" #include "common/common/utility.h" @@ -72,7 +73,7 @@ std::string FormatterImpl::format(const Http::HeaderMap& request_headers, } JsonFormatterImpl::JsonFormatterImpl( - const std::map& format_mapping) { + std::unordered_map& format_mapping) { for (const auto& pair : format_mapping) { auto providers = AccessLogFormatParser::parse(pair.second); diff --git a/source/common/access_log/access_log_formatter.h b/source/common/access_log/access_log_formatter.h index ed2d67201fb69..2ab76c25b93cd 100644 --- a/source/common/access_log/access_log_formatter.h +++ b/source/common/access_log/access_log_formatter.h @@ -3,6 +3,7 @@ #include #include #include +#include #include "envoy/access_log/access_log.h" #include "envoy/common/time.h" @@ -100,7 +101,7 @@ class FormatterImpl : public Formatter { class JsonFormatterImpl : public Formatter { public: - JsonFormatterImpl(const std::map& format_mapping); + JsonFormatterImpl(std::unordered_map& format_mapping); // Formatter::format std::string format(const Http::HeaderMap& request_headers, diff --git a/source/extensions/access_loggers/file/config.cc b/source/extensions/access_loggers/file/config.cc index 02a531abfe45a..673c46975184f 100644 --- a/source/extensions/access_loggers/file/config.cc +++ b/source/extensions/access_loggers/file/config.cc @@ -10,7 +10,8 @@ #include "extensions/access_loggers/file/file_access_log_impl.h" #include "extensions/access_loggers/well_known_names.h" - +#include + namespace Envoy { namespace Extensions { namespace AccessLoggers { @@ -34,7 +35,7 @@ FileAccessLogFactory::createAccessLogInstance(const Protobuf::Message& config, } } else if (fal_config.access_log_format_case() == envoy::config::accesslog::v2::FileAccessLog::kJsonFormat) { - const auto& json_format_map = this->convertJsonFormatToMap(fal_config.json_format()); + auto json_format_map = this->convertJsonFormatToMap(fal_config.json_format()); formatter.reset(new AccessLog::JsonFormatterImpl(json_format_map)); } else { throw EnvoyException( @@ -51,9 +52,9 @@ ProtobufTypes::MessagePtr FileAccessLogFactory::createEmptyConfigProto() { std::string FileAccessLogFactory::name() const { return AccessLogNames::get().File; } -std::map +std::unordered_map FileAccessLogFactory::convertJsonFormatToMap(ProtobufWkt::Struct json_format) { - std::map output; + std::unordered_map output; for (const auto& pair : json_format.fields()) { if (pair.second.kind_case() != ProtobufWkt::Value::kStringValue) { throw EnvoyException("Only string values are supported in the JSON access log format."); diff --git a/source/extensions/access_loggers/file/config.h b/source/extensions/access_loggers/file/config.h index c3804de0b73ea..7f3976adfc8be 100644 --- a/source/extensions/access_loggers/file/config.h +++ b/source/extensions/access_loggers/file/config.h @@ -21,7 +21,7 @@ class FileAccessLogFactory : public Server::Configuration::AccessLogInstanceFact std::string name() const override; private: - std::map convertJsonFormatToMap(ProtobufWkt::Struct config); + std::unordered_map convertJsonFormatToMap(ProtobufWkt::Struct config); }; } // namespace File From 91b1b800bebaaf22202be91b08af3b52aac3217e Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Wed, 17 Oct 2018 11:59:13 -0700 Subject: [PATCH 40/51] Remove single token restriction Signed-off-by: Aaltan Ahmad --- .../common/access_log/access_log_formatter.cc | 14 +------ .../common/access_log/access_log_formatter.h | 2 +- .../access_log/access_log_formatter_test.cc | 37 ++++++++++++++++--- 3 files changed, 33 insertions(+), 20 deletions(-) diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc index ce2e2b389f7b1..1228a673e1bb2 100644 --- a/source/common/access_log/access_log_formatter.cc +++ b/source/common/access_log/access_log_formatter.cc @@ -76,19 +76,7 @@ JsonFormatterImpl::JsonFormatterImpl( std::unordered_map& format_mapping) { for (const auto& pair : format_mapping) { auto providers = AccessLogFormatParser::parse(pair.second); - - // Enforce that each key only has one format specifier in it - if (providers.size() > 1) { - throw EnvoyException(fmt::format( - "More than one format specifier was provided in the JSON log format: {}", pair.second)); - } - - if (providers.size() < 1) { - throw EnvoyException( - fmt::format("No format specifier was provided in the JSON log format: {}", pair.second)); - } - - json_output_format_.emplace(pair.first, std::move(providers[0])); + json_output_format_.emplace(pair.first, FormatterPtr{new FormatterImpl(pair.second)}); } } diff --git a/source/common/access_log/access_log_formatter.h b/source/common/access_log/access_log_formatter.h index 2ab76c25b93cd..820f1ae3822cc 100644 --- a/source/common/access_log/access_log_formatter.h +++ b/source/common/access_log/access_log_formatter.h @@ -111,7 +111,7 @@ class JsonFormatterImpl : public Formatter { private: std::vector providers_; - std::map json_output_format_; + std::map json_output_format_; std::unordered_map toMap(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, diff --git a/test/common/access_log/access_log_formatter_test.cc b/test/common/access_log/access_log_formatter_test.cc index ca93847c22434..073e49e2aedd2 100644 --- a/test/common/access_log/access_log_formatter_test.cc +++ b/test/common/access_log/access_log_formatter_test.cc @@ -397,7 +397,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { std::unordered_map expected_json_map = { {"plain_string", "plain_string_value"}}; - const std::map key_mapping = { + std::unordered_map key_mapping = { {"plain_string", "plain_string_value"}}; JsonFormatterImpl formatter(key_mapping); @@ -420,7 +420,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { std::unordered_map expected_json_map = {{"protocol", "HTTP/1.1"}}; - const std::map key_mapping = {{"protocol", "%PROTOCOL%"}}; + std::unordered_map key_mapping = {{"protocol", "%PROTOCOL%"}}; JsonFormatterImpl formatter(key_mapping); const auto parsed = Json::Factory::loadFromString(formatter.format(request_header, response_header, response_trailer, stream_info)); @@ -441,7 +441,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { {"nonexistent_response_header", "-"}, {"some_response_header", "SOME_RESPONSE_HEADER"}}; - const std::map key_mapping = { + std::unordered_map key_mapping = { {"protocol", "%PROTOCOL%"}, {"some_request_header", "%REQ(some_request_header)%"}, {"nonexistent_response_header", "%RESP(nonexistent_response_header)%"}, @@ -469,7 +469,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { {"response_absent_header_or_response_absent_header", "RESPONSE_PRESENT_HEADER"}, {"response_present_header_or_response_absent_header", "RESPONSE_PRESENT_HEADER"}}; - const std::map key_mapping = { + std::unordered_map key_mapping = { {"request_present_header_or_request_absent_header", "%REQ(request_present_header?request_absent_header)%"}, {"request_absent_header_or_request_present_header", @@ -504,7 +504,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { {"test_obj", "{\"inner_key\":\"inner_value\"}"}, {"test_obj.inner_key", "\"inner_value\""}}; - const std::map key_mapping = { + std::unordered_map key_mapping = { {"test_key", "%DYNAMIC_METADATA(com.test:test_key)%"}, {"test_obj", "%DYNAMIC_METADATA(com.test:test_obj)%"}, {"test_obj.inner_key", "%DYNAMIC_METADATA(com.test:test_obj:inner_key)%"}}; @@ -539,7 +539,7 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { {"default", "2018-03-28T23:35:58.000Z"}, {"all_zeroes", "000000000.0.00.000"}}; - const std::map key_mapping = { + std::unordered_map key_mapping = { {"simple_date", "%START_TIME(%Y/%m/%d)%"}, {"test_time", "%START_TIME(%s)%"}, {"bad_format", "%START_TIME(bad_format)%"}, @@ -554,6 +554,31 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { } } +TEST(AccessLogFormatterTest, JsonFormatterMultiTokenTest) { + { + StreamInfo::MockStreamInfo stream_info; + Http::TestHeaderMapImpl request_header{{"some_request_header", "SOME_REQUEST_HEADER"}}; + Http::TestHeaderMapImpl response_header{{"some_response_header", "SOME_RESPONSE_HEADER"}}; + Http::TestHeaderMapImpl response_trailer{}; + + std::unordered_map expected_json_map = { + {"multi_token_field", "HTTP/1.1 plainstring SOME_REQUEST_HEADER SOME_RESPONSE_HEADER"}}; + + std::unordered_map key_mapping = { + {"multi_token_field", "%PROTOCOL% plainstring %REQ(some_request_header)% %RESP(some_response_header)%"}}; + JsonFormatterImpl formatter(key_mapping); + + absl::optional protocol = Http::Protocol::Http11; + EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); + + const auto parsed = Json::Factory::loadFromString(formatter.format(request_header, response_header, response_trailer, stream_info)); + for (const auto& pair : expected_json_map) { + EXPECT_EQ(parsed->getString(pair.first), pair.second); + } + } + +} + TEST(AccessLogFormatterTest, CompositeFormatterSuccess) { StreamInfo::MockStreamInfo stream_info; Http::TestHeaderMapImpl request_header{{"first", "GET"}, {":path", "/"}}; From 0bf86f87cccbdae3090a04e6561339fb94b1d9b8 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Wed, 17 Oct 2018 12:00:07 -0700 Subject: [PATCH 41/51] Remove wording about one token limitation Signed-off-by: Aaltan Ahmad --- docs/root/configuration/access_log.rst | 2 -- 1 file changed, 2 deletions(-) diff --git a/docs/root/configuration/access_log.rst b/docs/root/configuration/access_log.rst index 3ca0e5b4751df..882d24e66da1b 100644 --- a/docs/root/configuration/access_log.rst +++ b/docs/root/configuration/access_log.rst @@ -91,8 +91,6 @@ This allows you to specify a custom key for each command operator. Format dictionaries have the following restrictions: * The dictionary must map strings to strings (specifically, strings to command operators). Nesting is not currently supported. -* Each value in the dictionary can only contain one command operator. - Command Operators ----------------- From 5aa43e2166266e6bfd47ba1dfc1bc8315cae6512 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Wed, 17 Oct 2018 12:01:00 -0700 Subject: [PATCH 42/51] Formatting Signed-off-by: Aaltan Ahmad --- source/common/access_log/access_log_formatter.cc | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc index 1228a673e1bb2..5985e8ae47dc2 100644 --- a/source/common/access_log/access_log_formatter.cc +++ b/source/common/access_log/access_log_formatter.cc @@ -4,7 +4,6 @@ #include #include - #include "common/common/assert.h" #include "common/common/fmt.h" #include "common/common/utility.h" @@ -72,8 +71,7 @@ std::string FormatterImpl::format(const Http::HeaderMap& request_headers, return log_line; } -JsonFormatterImpl::JsonFormatterImpl( - std::unordered_map& format_mapping) { +JsonFormatterImpl::JsonFormatterImpl(std::unordered_map& format_mapping) { for (const auto& pair : format_mapping) { auto providers = AccessLogFormatParser::parse(pair.second); json_output_format_.emplace(pair.first, FormatterPtr{new FormatterImpl(pair.second)}); @@ -96,7 +94,8 @@ std::string JsonFormatterImpl::format(const Http::HeaderMap& request_headers, std::string log_line; auto conversion_status = ProtobufUtil::MessageToJsonString(output_struct, &log_line); if (!conversion_status.ok()) { - log_line = fmt::format("Error serializing access log to JSON: {}", conversion_status.ToString()); + log_line = + fmt::format("Error serializing access log to JSON: {}", conversion_status.ToString()); } return log_line; From d8a51e93ef4e60491e7d5d237649f30a106c003b Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Wed, 17 Oct 2018 12:01:30 -0700 Subject: [PATCH 43/51] Formatting Signed-off-by: Aaltan Ahmad --- source/common/access_log/access_log_formatter.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/access_log/access_log_formatter.h b/source/common/access_log/access_log_formatter.h index 820f1ae3822cc..a1cf7e9ebc52d 100644 --- a/source/common/access_log/access_log_formatter.h +++ b/source/common/access_log/access_log_formatter.h @@ -2,8 +2,8 @@ #include #include -#include #include +#include #include "envoy/access_log/access_log.h" #include "envoy/common/time.h" @@ -112,7 +112,7 @@ class JsonFormatterImpl : public Formatter { private: std::vector providers_; std::map json_output_format_; - + std::unordered_map toMap(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, const Http::HeaderMap& response_trailers, const StreamInfo::StreamInfo& stream_info) const; From b87f2a4cb211d27ac710e9085ba1ff5f01bbb321 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Wed, 17 Oct 2018 12:04:39 -0700 Subject: [PATCH 44/51] Formatting Signed-off-by: Aaltan Ahmad --- .../extensions/access_loggers/file/config.cc | 5 ++-- .../access_log/access_log_formatter_test.cc | 25 ++++++++++++------- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/source/extensions/access_loggers/file/config.cc b/source/extensions/access_loggers/file/config.cc index 673c46975184f..8198232f76d03 100644 --- a/source/extensions/access_loggers/file/config.cc +++ b/source/extensions/access_loggers/file/config.cc @@ -1,5 +1,7 @@ #include "extensions/access_loggers/file/config.h" +#include + #include "envoy/config/accesslog/v2/file.pb.validate.h" #include "envoy/registry/registry.h" #include "envoy/server/filter_config.h" @@ -10,8 +12,7 @@ #include "extensions/access_loggers/file/file_access_log_impl.h" #include "extensions/access_loggers/well_known_names.h" -#include - + namespace Envoy { namespace Extensions { namespace AccessLoggers { diff --git a/test/common/access_log/access_log_formatter_test.cc b/test/common/access_log/access_log_formatter_test.cc index 073e49e2aedd2..88cff032c0985 100644 --- a/test/common/access_log/access_log_formatter_test.cc +++ b/test/common/access_log/access_log_formatter_test.cc @@ -401,7 +401,8 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { {"plain_string", "plain_string_value"}}; JsonFormatterImpl formatter(key_mapping); - const auto parsed = Json::Factory::loadFromString(formatter.format(request_header, response_header, response_trailer, stream_info)); + const auto parsed = Json::Factory::loadFromString( + formatter.format(request_header, response_header, response_trailer, stream_info)); for (const auto& pair : expected_json_map) { EXPECT_EQ(parsed->getString(pair.first), pair.second); } @@ -423,7 +424,8 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { std::unordered_map key_mapping = {{"protocol", "%PROTOCOL%"}}; JsonFormatterImpl formatter(key_mapping); - const auto parsed = Json::Factory::loadFromString(formatter.format(request_header, response_header, response_trailer, stream_info)); + const auto parsed = Json::Factory::loadFromString( + formatter.format(request_header, response_header, response_trailer, stream_info)); for (const auto& pair : expected_json_map) { EXPECT_EQ(parsed->getString(pair.first), pair.second); } @@ -451,7 +453,8 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { absl::optional protocol = Http::Protocol::Http11; EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); - const auto parsed = Json::Factory::loadFromString(formatter.format(request_header, response_header, response_trailer, stream_info)); + const auto parsed = Json::Factory::loadFromString( + formatter.format(request_header, response_header, response_trailer, stream_info)); for (const auto& pair : expected_json_map) { EXPECT_EQ(parsed->getString(pair.first), pair.second); } @@ -483,7 +486,8 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { absl::optional protocol = Http::Protocol::Http11; EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); - const auto parsed = Json::Factory::loadFromString(formatter.format(request_header, response_header, response_trailer, stream_info)); + const auto parsed = Json::Factory::loadFromString( + formatter.format(request_header, response_header, response_trailer, stream_info)); for (const auto& pair : expected_json_map) { EXPECT_EQ(parsed->getString(pair.first), pair.second); } @@ -511,7 +515,8 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { JsonFormatterImpl formatter(key_mapping); - const auto parsed = Json::Factory::loadFromString(formatter.format(request_header, response_header, response_trailer, stream_info)); + const auto parsed = Json::Factory::loadFromString( + formatter.format(request_header, response_header, response_trailer, stream_info)); for (const auto& pair : expected_json_map) { EXPECT_EQ(parsed->getString(pair.first), pair.second); } @@ -547,7 +552,8 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { {"all_zeroes", "%START_TIME(%f.%1f.%2f.%3f)%"}}; JsonFormatterImpl formatter(key_mapping); - const auto parsed = Json::Factory::loadFromString(formatter.format(request_header, response_header, response_trailer, stream_info)); + const auto parsed = Json::Factory::loadFromString( + formatter.format(request_header, response_header, response_trailer, stream_info)); for (const auto& pair : expected_json_map) { EXPECT_EQ(parsed->getString(pair.first), pair.second); } @@ -565,18 +571,19 @@ TEST(AccessLogFormatterTest, JsonFormatterMultiTokenTest) { {"multi_token_field", "HTTP/1.1 plainstring SOME_REQUEST_HEADER SOME_RESPONSE_HEADER"}}; std::unordered_map key_mapping = { - {"multi_token_field", "%PROTOCOL% plainstring %REQ(some_request_header)% %RESP(some_response_header)%"}}; + {"multi_token_field", + "%PROTOCOL% plainstring %REQ(some_request_header)% %RESP(some_response_header)%"}}; JsonFormatterImpl formatter(key_mapping); absl::optional protocol = Http::Protocol::Http11; EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); - const auto parsed = Json::Factory::loadFromString(formatter.format(request_header, response_header, response_trailer, stream_info)); + const auto parsed = Json::Factory::loadFromString( + formatter.format(request_header, response_header, response_trailer, stream_info)); for (const auto& pair : expected_json_map) { EXPECT_EQ(parsed->getString(pair.first), pair.second); } } - } TEST(AccessLogFormatterTest, CompositeFormatterSuccess) { From 6ef64568cd13179d2bb6b704d1a6119667171d7b Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Wed, 17 Oct 2018 12:09:56 -0700 Subject: [PATCH 45/51] use const auto Signed-off-by: Aaltan Ahmad --- source/common/access_log/access_log_formatter.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc index 5985e8ae47dc2..34391f5bec8d4 100644 --- a/source/common/access_log/access_log_formatter.cc +++ b/source/common/access_log/access_log_formatter.cc @@ -82,7 +82,7 @@ std::string JsonFormatterImpl::format(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, const Http::HeaderMap& response_trailers, const StreamInfo::StreamInfo& stream_info) const { - auto output_map = this->toMap(request_headers, response_headers, response_trailers, stream_info); + const auto output_map = this->toMap(request_headers, response_headers, response_trailers, stream_info); ProtobufWkt::Struct output_struct; for (const auto& pair : output_map) { @@ -92,7 +92,7 @@ std::string JsonFormatterImpl::format(const Http::HeaderMap& request_headers, } std::string log_line; - auto conversion_status = ProtobufUtil::MessageToJsonString(output_struct, &log_line); + const auto conversion_status = ProtobufUtil::MessageToJsonString(output_struct, &log_line); if (!conversion_status.ok()) { log_line = fmt::format("Error serializing access log to JSON: {}", conversion_status.ToString()); From 1ffc475743a1fbde8aa0a6e86247d717e10067c1 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Wed, 17 Oct 2018 12:10:23 -0700 Subject: [PATCH 46/51] Formatting Signed-off-by: Aaltan Ahmad --- source/common/access_log/access_log_formatter.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc index 34391f5bec8d4..738fa53503a42 100644 --- a/source/common/access_log/access_log_formatter.cc +++ b/source/common/access_log/access_log_formatter.cc @@ -82,7 +82,8 @@ std::string JsonFormatterImpl::format(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, const Http::HeaderMap& response_trailers, const StreamInfo::StreamInfo& stream_info) const { - const auto output_map = this->toMap(request_headers, response_headers, response_trailers, stream_info); + const auto output_map = + this->toMap(request_headers, response_headers, response_trailers, stream_info); ProtobufWkt::Struct output_struct; for (const auto& pair : output_map) { From e92a886904ade8bdc0a7fe6748a4606a22804ec0 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Thu, 18 Oct 2018 13:03:40 -0700 Subject: [PATCH 47/51] Break up tests and remove this-> Signed-off-by: Aaltan Ahmad --- .../common/access_log/access_log_formatter.cc | 2 +- .../access_log/access_log_formatter_test.cc | 37 ++++++++++++------- 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc index 738fa53503a42..67c3905ecebef 100644 --- a/source/common/access_log/access_log_formatter.cc +++ b/source/common/access_log/access_log_formatter.cc @@ -83,7 +83,7 @@ std::string JsonFormatterImpl::format(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_trailers, const StreamInfo::StreamInfo& stream_info) const { const auto output_map = - this->toMap(request_headers, response_headers, response_trailers, stream_info); + toMap(request_headers, response_headers, response_trailers, stream_info); ProtobufWkt::Struct output_struct; for (const auto& pair : output_map) { diff --git a/test/common/access_log/access_log_formatter_test.cc b/test/common/access_log/access_log_formatter_test.cc index 88cff032c0985..e16a35de9bd28 100644 --- a/test/common/access_log/access_log_formatter_test.cc +++ b/test/common/access_log/access_log_formatter_test.cc @@ -382,12 +382,12 @@ TEST(AccessLogFormatterTest, startTimeFormatter) { } } -TEST(AccessLogFormatterTest, JsonFormatterTest) { +TEST(AccessLogFormatterTest, JsonFormatterPlainStringTest) { { StreamInfo::MockStreamInfo stream_info; - Http::TestHeaderMapImpl request_header{}; - Http::TestHeaderMapImpl response_header{}; - Http::TestHeaderMapImpl response_trailer{}; + Http::TestHeaderMapImpl request_header; + Http::TestHeaderMapImpl response_header; + Http::TestHeaderMapImpl response_trailer; envoy::api::v2::core::Metadata metadata; populateMetadataTestData(metadata); @@ -407,12 +407,13 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { EXPECT_EQ(parsed->getString(pair.first), pair.second); } } - +} +TEST(AccessLogFormatterTest, JsonFormatterSingleOperatorTest) { { StreamInfo::MockStreamInfo stream_info; - Http::TestHeaderMapImpl request_header{}; - Http::TestHeaderMapImpl response_header{}; - Http::TestHeaderMapImpl response_trailer{}; + Http::TestHeaderMapImpl request_header; + Http::TestHeaderMapImpl response_header; + Http::TestHeaderMapImpl response_trailer; envoy::api::v2::core::Metadata metadata; populateMetadataTestData(metadata); @@ -430,12 +431,14 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { EXPECT_EQ(parsed->getString(pair.first), pair.second); } } +} +TEST(AccessLogFormatterTest, JsonFormatterNonExistentHeaderTest) { { StreamInfo::MockStreamInfo stream_info; Http::TestHeaderMapImpl request_header{{"some_request_header", "SOME_REQUEST_HEADER"}}; Http::TestHeaderMapImpl response_header{{"some_response_header", "SOME_RESPONSE_HEADER"}}; - Http::TestHeaderMapImpl response_trailer{}; + Http::TestHeaderMapImpl response_trailer; std::unordered_map expected_json_map = { {"protocol", "HTTP/1.1"}, @@ -459,12 +462,14 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { EXPECT_EQ(parsed->getString(pair.first), pair.second); } } +} +TEST(AccessLogFormatterTest, JsonFormatterAlternateHeaderTest) { { StreamInfo::MockStreamInfo stream_info; Http::TestHeaderMapImpl request_header{{"request_present_header", "REQUEST_PRESENT_HEADER"}}; Http::TestHeaderMapImpl response_header{{"response_present_header", "RESPONSE_PRESENT_HEADER"}}; - Http::TestHeaderMapImpl response_trailer{}; + Http::TestHeaderMapImpl response_trailer; std::unordered_map expected_json_map = { {"request_present_header_or_request_absent_header", "REQUEST_PRESENT_HEADER"}, @@ -492,7 +497,9 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { EXPECT_EQ(parsed->getString(pair.first), pair.second); } } +} +TEST(AccessLogFormatterTest, JsonFormatterDynamicMetadataTest) { { StreamInfo::MockStreamInfo stream_info; Http::TestHeaderMapImpl request_header{{"first", "GET"}, {":path", "/"}}; @@ -521,12 +528,14 @@ TEST(AccessLogFormatterTest, JsonFormatterTest) { EXPECT_EQ(parsed->getString(pair.first), pair.second); } } +} +TEST(AccessLogFormatterTest, JsonFormatterStartTimeTest) { { StreamInfo::MockStreamInfo stream_info; - Http::TestHeaderMapImpl request_header{}; - Http::TestHeaderMapImpl response_header{}; - Http::TestHeaderMapImpl response_trailer{}; + Http::TestHeaderMapImpl request_header; + Http::TestHeaderMapImpl response_header; + Http::TestHeaderMapImpl response_trailer; time_t test_epoch = 1522280158; SystemTime time = std::chrono::system_clock::from_time_t(test_epoch); @@ -565,7 +574,7 @@ TEST(AccessLogFormatterTest, JsonFormatterMultiTokenTest) { StreamInfo::MockStreamInfo stream_info; Http::TestHeaderMapImpl request_header{{"some_request_header", "SOME_REQUEST_HEADER"}}; Http::TestHeaderMapImpl response_header{{"some_response_header", "SOME_RESPONSE_HEADER"}}; - Http::TestHeaderMapImpl response_trailer{}; + Http::TestHeaderMapImpl response_trailer; std::unordered_map expected_json_map = { {"multi_token_field", "HTTP/1.1 plainstring SOME_REQUEST_HEADER SOME_RESPONSE_HEADER"}}; From 252df75fa591c2becaec3d390b990f13d0baa586 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Thu, 18 Oct 2018 13:07:14 -0700 Subject: [PATCH 48/51] Break up config tests into separate test cases Signed-off-by: Aaltan Ahmad --- test/extensions/access_loggers/file/config_test.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/extensions/access_loggers/file/config_test.cc b/test/extensions/access_loggers/file/config_test.cc index af320097386f3..5f5d421649c07 100644 --- a/test/extensions/access_loggers/file/config_test.cc +++ b/test/extensions/access_loggers/file/config_test.cc @@ -107,7 +107,7 @@ TEST(FileAccessLogConfigTest, FileAccessLogJsonTest) { "Didn't find a registered implementation for name: 'INVALID'"); } -TEST(FileAccessLogConfigTest, FileAccessLogJsonConversionTest) { +TEST(FileAccessLogConfigTest, FileAccessLogJsonWithBoolValueTest) { { // Make sure we fail if you set a bool value in the format dictionary envoy::config::filter::accesslog::v2::AccessLog config; @@ -127,7 +127,9 @@ TEST(FileAccessLogConfigTest, FileAccessLogJsonConversionTest) { EnvoyException, "Only string values are supported in the JSON access log format."); } +} +TEST(FileAccessLogConfigTest, FileAccessLogJsonWithNestedKeyTest) { { // Make sure we fail if you set a nested Struct value in the format dictionary envoy::config::filter::accesslog::v2::AccessLog config; From a060f50110f4122e46305786c8762688abd54c2b Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Thu, 18 Oct 2018 13:22:19 -0700 Subject: [PATCH 49/51] Remove std::string conversion Signed-off-by: Aaltan Ahmad --- test/extensions/access_loggers/file/config_test.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/extensions/access_loggers/file/config_test.cc b/test/extensions/access_loggers/file/config_test.cc index 5f5d421649c07..fddde642fca07 100644 --- a/test/extensions/access_loggers/file/config_test.cc +++ b/test/extensions/access_loggers/file/config_test.cc @@ -84,7 +84,7 @@ TEST(FileAccessLogConfigTest, FileAccessLogJsonTest) { string_value.set_string_value("%PROTOCOL%"); auto json_format = fal_config.mutable_json_format(); - (*json_format->mutable_fields())[std::string{"protocol"}] = string_value; + (*json_format->mutable_fields())["protocol"] = string_value; EXPECT_EQ(fal_config.access_log_format_case(), envoy::config::accesslog::v2::FileAccessLog::kJsonFormat); @@ -118,7 +118,7 @@ TEST(FileAccessLogConfigTest, FileAccessLogJsonWithBoolValueTest) { ProtobufWkt::Value bool_value; bool_value.set_bool_value(false); auto json_format = fal_config.mutable_json_format(); - (*json_format->mutable_fields())[std::string{"protocol"}] = bool_value; + (*json_format->mutable_fields())["protocol"] = bool_value; MessageUtil::jsonConvert(fal_config, *config.mutable_config()); NiceMock context; @@ -138,14 +138,14 @@ TEST(FileAccessLogConfigTest, FileAccessLogJsonWithNestedKeyTest) { fal_config.set_path("/dev/null"); ProtobufWkt::Value string_value; - string_value.set_string_value(std::string{"some_nested_value"}); + string_value.set_string_value("some_nested_value"); ProtobufWkt::Value struct_value; - (*struct_value.mutable_struct_value()->mutable_fields())[std::string{"some_nested_key"}] = + (*struct_value.mutable_struct_value()->mutable_fields())["some_nested_key"] = string_value; auto json_format = fal_config.mutable_json_format(); - (*json_format->mutable_fields())[std::string{"top_level_key"}] = struct_value; + (*json_format->mutable_fields())["top_level_key"] = struct_value; MessageUtil::jsonConvert(fal_config, *config.mutable_config()); NiceMock context; From e7ca4a79d516d103d13878d14723cb21100d44ae Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Thu, 18 Oct 2018 13:22:37 -0700 Subject: [PATCH 50/51] Refactor json map assertions into a helper Signed-off-by: Aaltan Ahmad --- .../access_log/access_log_formatter_test.cc | 292 ++++++++---------- 1 file changed, 135 insertions(+), 157 deletions(-) diff --git a/test/common/access_log/access_log_formatter_test.cc b/test/common/access_log/access_log_formatter_test.cc index e16a35de9bd28..1273135f0f6f3 100644 --- a/test/common/access_log/access_log_formatter_test.cc +++ b/test/common/access_log/access_log_formatter_test.cc @@ -382,191 +382,169 @@ TEST(AccessLogFormatterTest, startTimeFormatter) { } } +void verifyJsonOutput(std::string json_string, + std::unordered_map expected_map) { + const auto parsed = Json::Factory::loadFromString(json_string); + for (const auto& pair : expected_map) { + EXPECT_EQ(parsed->getString(pair.first), pair.second); + } +} + TEST(AccessLogFormatterTest, JsonFormatterPlainStringTest) { - { - StreamInfo::MockStreamInfo stream_info; - Http::TestHeaderMapImpl request_header; - Http::TestHeaderMapImpl response_header; - Http::TestHeaderMapImpl response_trailer; + StreamInfo::MockStreamInfo stream_info; + Http::TestHeaderMapImpl request_header; + Http::TestHeaderMapImpl response_header; + Http::TestHeaderMapImpl response_trailer; - envoy::api::v2::core::Metadata metadata; - populateMetadataTestData(metadata); - absl::optional protocol = Http::Protocol::Http11; - EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); + envoy::api::v2::core::Metadata metadata; + populateMetadataTestData(metadata); + absl::optional protocol = Http::Protocol::Http11; + EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); - std::unordered_map expected_json_map = { - {"plain_string", "plain_string_value"}}; + std::unordered_map expected_json_map = { + {"plain_string", "plain_string_value"}}; - std::unordered_map key_mapping = { - {"plain_string", "plain_string_value"}}; - JsonFormatterImpl formatter(key_mapping); + std::unordered_map key_mapping = { + {"plain_string", "plain_string_value"}}; + JsonFormatterImpl formatter(key_mapping); - const auto parsed = Json::Factory::loadFromString( - formatter.format(request_header, response_header, response_trailer, stream_info)); - for (const auto& pair : expected_json_map) { - EXPECT_EQ(parsed->getString(pair.first), pair.second); - } - } + verifyJsonOutput(formatter.format(request_header, response_header, response_trailer, stream_info), + expected_json_map); } TEST(AccessLogFormatterTest, JsonFormatterSingleOperatorTest) { - { - StreamInfo::MockStreamInfo stream_info; - Http::TestHeaderMapImpl request_header; - Http::TestHeaderMapImpl response_header; - Http::TestHeaderMapImpl response_trailer; + StreamInfo::MockStreamInfo stream_info; + Http::TestHeaderMapImpl request_header; + Http::TestHeaderMapImpl response_header; + Http::TestHeaderMapImpl response_trailer; - envoy::api::v2::core::Metadata metadata; - populateMetadataTestData(metadata); - absl::optional protocol = Http::Protocol::Http11; - EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); + envoy::api::v2::core::Metadata metadata; + populateMetadataTestData(metadata); + absl::optional protocol = Http::Protocol::Http11; + EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); - std::unordered_map expected_json_map = {{"protocol", "HTTP/1.1"}}; + std::unordered_map expected_json_map = {{"protocol", "HTTP/1.1"}}; - std::unordered_map key_mapping = {{"protocol", "%PROTOCOL%"}}; - JsonFormatterImpl formatter(key_mapping); + std::unordered_map key_mapping = {{"protocol", "%PROTOCOL%"}}; + JsonFormatterImpl formatter(key_mapping); - const auto parsed = Json::Factory::loadFromString( - formatter.format(request_header, response_header, response_trailer, stream_info)); - for (const auto& pair : expected_json_map) { - EXPECT_EQ(parsed->getString(pair.first), pair.second); - } - } + verifyJsonOutput(formatter.format(request_header, response_header, response_trailer, stream_info), + expected_json_map); } TEST(AccessLogFormatterTest, JsonFormatterNonExistentHeaderTest) { - { - StreamInfo::MockStreamInfo stream_info; - Http::TestHeaderMapImpl request_header{{"some_request_header", "SOME_REQUEST_HEADER"}}; - Http::TestHeaderMapImpl response_header{{"some_response_header", "SOME_RESPONSE_HEADER"}}; - Http::TestHeaderMapImpl response_trailer; - - std::unordered_map expected_json_map = { - {"protocol", "HTTP/1.1"}, - {"some_request_header", "SOME_REQUEST_HEADER"}, - {"nonexistent_response_header", "-"}, - {"some_response_header", "SOME_RESPONSE_HEADER"}}; - - std::unordered_map key_mapping = { - {"protocol", "%PROTOCOL%"}, - {"some_request_header", "%REQ(some_request_header)%"}, - {"nonexistent_response_header", "%RESP(nonexistent_response_header)%"}, - {"some_response_header", "%RESP(some_response_header)%"}}; - JsonFormatterImpl formatter(key_mapping); - - absl::optional protocol = Http::Protocol::Http11; - EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); - - const auto parsed = Json::Factory::loadFromString( - formatter.format(request_header, response_header, response_trailer, stream_info)); - for (const auto& pair : expected_json_map) { - EXPECT_EQ(parsed->getString(pair.first), pair.second); - } - } + StreamInfo::MockStreamInfo stream_info; + Http::TestHeaderMapImpl request_header{{"some_request_header", "SOME_REQUEST_HEADER"}}; + Http::TestHeaderMapImpl response_header{{"some_response_header", "SOME_RESPONSE_HEADER"}}; + Http::TestHeaderMapImpl response_trailer; + + std::unordered_map expected_json_map = { + {"protocol", "HTTP/1.1"}, + {"some_request_header", "SOME_REQUEST_HEADER"}, + {"nonexistent_response_header", "-"}, + {"some_response_header", "SOME_RESPONSE_HEADER"}}; + + std::unordered_map key_mapping = { + {"protocol", "%PROTOCOL%"}, + {"some_request_header", "%REQ(some_request_header)%"}, + {"nonexistent_response_header", "%RESP(nonexistent_response_header)%"}, + {"some_response_header", "%RESP(some_response_header)%"}}; + JsonFormatterImpl formatter(key_mapping); + + absl::optional protocol = Http::Protocol::Http11; + EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); + + verifyJsonOutput(formatter.format(request_header, response_header, response_trailer, stream_info), + expected_json_map); } TEST(AccessLogFormatterTest, JsonFormatterAlternateHeaderTest) { - { - StreamInfo::MockStreamInfo stream_info; - Http::TestHeaderMapImpl request_header{{"request_present_header", "REQUEST_PRESENT_HEADER"}}; - Http::TestHeaderMapImpl response_header{{"response_present_header", "RESPONSE_PRESENT_HEADER"}}; - Http::TestHeaderMapImpl response_trailer; - - std::unordered_map expected_json_map = { - {"request_present_header_or_request_absent_header", "REQUEST_PRESENT_HEADER"}, - {"request_absent_header_or_request_present_header", "REQUEST_PRESENT_HEADER"}, - {"response_absent_header_or_response_absent_header", "RESPONSE_PRESENT_HEADER"}, - {"response_present_header_or_response_absent_header", "RESPONSE_PRESENT_HEADER"}}; - - std::unordered_map key_mapping = { - {"request_present_header_or_request_absent_header", - "%REQ(request_present_header?request_absent_header)%"}, - {"request_absent_header_or_request_present_header", - "%REQ(request_absent_header?request_present_header)%"}, - {"response_absent_header_or_response_absent_header", - "%RESP(response_absent_header?response_present_header)%"}, - {"response_present_header_or_response_absent_header", - "%RESP(response_present_header?response_absent_header)%"}}; - JsonFormatterImpl formatter(key_mapping); - - absl::optional protocol = Http::Protocol::Http11; - EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); - - const auto parsed = Json::Factory::loadFromString( - formatter.format(request_header, response_header, response_trailer, stream_info)); - for (const auto& pair : expected_json_map) { - EXPECT_EQ(parsed->getString(pair.first), pair.second); - } - } + StreamInfo::MockStreamInfo stream_info; + Http::TestHeaderMapImpl request_header{{"request_present_header", "REQUEST_PRESENT_HEADER"}}; + Http::TestHeaderMapImpl response_header{{"response_present_header", "RESPONSE_PRESENT_HEADER"}}; + Http::TestHeaderMapImpl response_trailer; + + std::unordered_map expected_json_map = { + {"request_present_header_or_request_absent_header", "REQUEST_PRESENT_HEADER"}, + {"request_absent_header_or_request_present_header", "REQUEST_PRESENT_HEADER"}, + {"response_absent_header_or_response_absent_header", "RESPONSE_PRESENT_HEADER"}, + {"response_present_header_or_response_absent_header", "RESPONSE_PRESENT_HEADER"}}; + + std::unordered_map key_mapping = { + {"request_present_header_or_request_absent_header", + "%REQ(request_present_header?request_absent_header)%"}, + {"request_absent_header_or_request_present_header", + "%REQ(request_absent_header?request_present_header)%"}, + {"response_absent_header_or_response_absent_header", + "%RESP(response_absent_header?response_present_header)%"}, + {"response_present_header_or_response_absent_header", + "%RESP(response_present_header?response_absent_header)%"}}; + JsonFormatterImpl formatter(key_mapping); + + absl::optional protocol = Http::Protocol::Http11; + EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); + + verifyJsonOutput(formatter.format(request_header, response_header, response_trailer, stream_info), + expected_json_map); } TEST(AccessLogFormatterTest, JsonFormatterDynamicMetadataTest) { - { - StreamInfo::MockStreamInfo stream_info; - Http::TestHeaderMapImpl request_header{{"first", "GET"}, {":path", "/"}}; - Http::TestHeaderMapImpl response_header{{"second", "PUT"}, {"test", "test"}}; - Http::TestHeaderMapImpl response_trailer{{"third", "POST"}, {"test-2", "test-2"}}; + StreamInfo::MockStreamInfo stream_info; + Http::TestHeaderMapImpl request_header{{"first", "GET"}, {":path", "/"}}; + Http::TestHeaderMapImpl response_header{{"second", "PUT"}, {"test", "test"}}; + Http::TestHeaderMapImpl response_trailer{{"third", "POST"}, {"test-2", "test-2"}}; - envoy::api::v2::core::Metadata metadata; - populateMetadataTestData(metadata); - EXPECT_CALL(stream_info, dynamicMetadata()).WillRepeatedly(ReturnRef(metadata)); + envoy::api::v2::core::Metadata metadata; + populateMetadataTestData(metadata); + EXPECT_CALL(stream_info, dynamicMetadata()).WillRepeatedly(ReturnRef(metadata)); - std::unordered_map expected_json_map = { - {"test_key", "\"test_value\""}, - {"test_obj", "{\"inner_key\":\"inner_value\"}"}, - {"test_obj.inner_key", "\"inner_value\""}}; + std::unordered_map expected_json_map = { + {"test_key", "\"test_value\""}, + {"test_obj", "{\"inner_key\":\"inner_value\"}"}, + {"test_obj.inner_key", "\"inner_value\""}}; - std::unordered_map key_mapping = { - {"test_key", "%DYNAMIC_METADATA(com.test:test_key)%"}, - {"test_obj", "%DYNAMIC_METADATA(com.test:test_obj)%"}, - {"test_obj.inner_key", "%DYNAMIC_METADATA(com.test:test_obj:inner_key)%"}}; + std::unordered_map key_mapping = { + {"test_key", "%DYNAMIC_METADATA(com.test:test_key)%"}, + {"test_obj", "%DYNAMIC_METADATA(com.test:test_obj)%"}, + {"test_obj.inner_key", "%DYNAMIC_METADATA(com.test:test_obj:inner_key)%"}}; - JsonFormatterImpl formatter(key_mapping); + JsonFormatterImpl formatter(key_mapping); - const auto parsed = Json::Factory::loadFromString( - formatter.format(request_header, response_header, response_trailer, stream_info)); - for (const auto& pair : expected_json_map) { - EXPECT_EQ(parsed->getString(pair.first), pair.second); - } - } + verifyJsonOutput(formatter.format(request_header, response_header, response_trailer, stream_info), + expected_json_map); } TEST(AccessLogFormatterTest, JsonFormatterStartTimeTest) { - { - StreamInfo::MockStreamInfo stream_info; - Http::TestHeaderMapImpl request_header; - Http::TestHeaderMapImpl response_header; - Http::TestHeaderMapImpl response_trailer; - - time_t test_epoch = 1522280158; - SystemTime time = std::chrono::system_clock::from_time_t(test_epoch); - EXPECT_CALL(stream_info, startTime()).WillRepeatedly(Return(time)); - - // Needed to take into account the behavior in non-GMT timezones. - struct tm time_val; - gmtime_r(&test_epoch, &time_val); - time_t expected_time_t = mktime(&time_val); - - std::unordered_map expected_json_map = { - {"simple_date", "2018/03/28"}, - {"test_time", fmt::format("{}", expected_time_t)}, - {"bad_format", "bad_format"}, - {"default", "2018-03-28T23:35:58.000Z"}, - {"all_zeroes", "000000000.0.00.000"}}; - - std::unordered_map key_mapping = { - {"simple_date", "%START_TIME(%Y/%m/%d)%"}, - {"test_time", "%START_TIME(%s)%"}, - {"bad_format", "%START_TIME(bad_format)%"}, - {"default", "%START_TIME%"}, - {"all_zeroes", "%START_TIME(%f.%1f.%2f.%3f)%"}}; - JsonFormatterImpl formatter(key_mapping); - - const auto parsed = Json::Factory::loadFromString( - formatter.format(request_header, response_header, response_trailer, stream_info)); - for (const auto& pair : expected_json_map) { - EXPECT_EQ(parsed->getString(pair.first), pair.second); - } - } + StreamInfo::MockStreamInfo stream_info; + Http::TestHeaderMapImpl request_header; + Http::TestHeaderMapImpl response_header; + Http::TestHeaderMapImpl response_trailer; + + time_t test_epoch = 1522280158; + SystemTime time = std::chrono::system_clock::from_time_t(test_epoch); + EXPECT_CALL(stream_info, startTime()).WillRepeatedly(Return(time)); + + // Needed to take into account the behavior in non-GMT timezones. + struct tm time_val; + gmtime_r(&test_epoch, &time_val); + time_t expected_time_t = mktime(&time_val); + + std::unordered_map expected_json_map = { + {"simple_date", "2018/03/28"}, + {"test_time", fmt::format("{}", expected_time_t)}, + {"bad_format", "bad_format"}, + {"default", "2018-03-28T23:35:58.000Z"}, + {"all_zeroes", "000000000.0.00.000"}}; + + std::unordered_map key_mapping = { + {"simple_date", "%START_TIME(%Y/%m/%d)%"}, + {"test_time", "%START_TIME(%s)%"}, + {"bad_format", "%START_TIME(bad_format)%"}, + {"default", "%START_TIME%"}, + {"all_zeroes", "%START_TIME(%f.%1f.%2f.%3f)%"}}; + JsonFormatterImpl formatter(key_mapping); + + verifyJsonOutput(formatter.format(request_header, response_header, response_trailer, stream_info), + expected_json_map); } TEST(AccessLogFormatterTest, JsonFormatterMultiTokenTest) { From d0221c8356146b797bdf4e69e4ee8d7a1d839d04 Mon Sep 17 00:00:00 2001 From: Aaltan Ahmad Date: Thu, 18 Oct 2018 14:28:03 -0700 Subject: [PATCH 51/51] Formatting fixes Signed-off-by: Aaltan Ahmad --- source/common/access_log/access_log_formatter.cc | 3 +-- test/extensions/access_loggers/file/config_test.cc | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc index 67c3905ecebef..6e4e128e3e0c9 100644 --- a/source/common/access_log/access_log_formatter.cc +++ b/source/common/access_log/access_log_formatter.cc @@ -82,8 +82,7 @@ std::string JsonFormatterImpl::format(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers, const Http::HeaderMap& response_trailers, const StreamInfo::StreamInfo& stream_info) const { - const auto output_map = - toMap(request_headers, response_headers, response_trailers, stream_info); + const auto output_map = toMap(request_headers, response_headers, response_trailers, stream_info); ProtobufWkt::Struct output_struct; for (const auto& pair : output_map) { diff --git a/test/extensions/access_loggers/file/config_test.cc b/test/extensions/access_loggers/file/config_test.cc index fddde642fca07..14e35b40c72fe 100644 --- a/test/extensions/access_loggers/file/config_test.cc +++ b/test/extensions/access_loggers/file/config_test.cc @@ -141,8 +141,7 @@ TEST(FileAccessLogConfigTest, FileAccessLogJsonWithNestedKeyTest) { string_value.set_string_value("some_nested_value"); ProtobufWkt::Value struct_value; - (*struct_value.mutable_struct_value()->mutable_fields())["some_nested_key"] = - string_value; + (*struct_value.mutable_struct_value()->mutable_fields())["some_nested_key"] = string_value; auto json_format = fal_config.mutable_json_format(); (*json_format->mutable_fields())["top_level_key"] = struct_value;