From 2586857ce9f2bf7c9ff5f1f8a37abb5ae61243d8 Mon Sep 17 00:00:00 2001 From: Petr Pchelko Date: Mon, 10 Aug 2020 21:15:02 -0700 Subject: [PATCH 1/7] TEMP Signed-off-by: Petr Pchelko --- source/common/formatter/substitution_format_string.cc | 1 - source/common/formatter/substitution_formatter.h | 8 ++++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/source/common/formatter/substitution_format_string.cc b/source/common/formatter/substitution_format_string.cc index ec9e2db96865..57470b83412c 100644 --- a/source/common/formatter/substitution_format_string.cc +++ b/source/common/formatter/substitution_format_string.cc @@ -6,7 +6,6 @@ namespace Envoy { namespace Formatter { namespace { -absl::flat_hash_map convertJsonFormatToMap(const ProtobufWkt::Struct& json_format) { absl::flat_hash_map output; for (const auto& pair : json_format.fields()) { diff --git a/source/common/formatter/substitution_formatter.h b/source/common/formatter/substitution_formatter.h index 8336f3274f85..6a6ff800e67e 100644 --- a/source/common/formatter/substitution_formatter.h +++ b/source/common/formatter/substitution_formatter.h @@ -17,6 +17,14 @@ namespace Envoy { namespace Formatter { +// We actually need a map to formatters, we can pass the ProtobufStruct, no need to reformat it. +struct JsonMap; +using JsonMapPtr = std::unique_ptr; +using JsonMapValue = absl::variant; +struct JsonMap { + absl::flat_hash_map values_; +}; + /** * Access log format parser. */ From b15125c98ef715e0d6d0e11c247d9c6b495e11ec Mon Sep 17 00:00:00 2001 From: Petr Pchelko Date: Tue, 11 Aug 2020 14:17:39 -0700 Subject: [PATCH 2/7] Support nested objects in access logging json format Signed-off-by: Petr Pchelko --- .../observability/access_log/usage.rst | 2 +- docs/root/version_history/current.rst | 1 + .../formatter/substitution_format_string.cc | 23 +-- .../formatter/substitution_format_string.h | 6 - .../formatter/substitution_formatter.cc | 86 ++++---- .../common/formatter/substitution_formatter.h | 22 +- .../substitution_format_string_test.cc | 21 +- .../substitution_formatter_speed_test.cc | 26 +-- .../formatter/substitution_formatter_test.cc | 190 ++++++++++++------ 9 files changed, 220 insertions(+), 157 deletions(-) diff --git a/docs/root/configuration/observability/access_log/usage.rst b/docs/root/configuration/observability/access_log/usage.rst index 894c8ac61c36..329e071f0a5b 100644 --- a/docs/root/configuration/observability/access_log/usage.rst +++ b/docs/root/configuration/observability/access_log/usage.rst @@ -93,7 +93,7 @@ would be rendered as the number ``123``. Format dictionaries have the following restrictions: * The dictionary must map strings to strings (specifically, strings to command operators). Nesting - is not currently supported. + is supported. * When using the ``typed_json_format`` command operators will only produce typed output if the command operator is the only string that appears in the dictionary value. For example, ``"%DURATION%"`` will log a numeric duration value, but ``"%DURATION%.0"`` will log a string diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index f5b6217a1757..571c3cf39ec3 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -50,6 +50,7 @@ New Features ------------ * access log: added a :ref:`dynamic metadata filter` for access logs, which filters whether to log based on matching dynamic metadata. * access log: added support for :ref:`%DOWNSTREAM_PEER_FINGERPRINT_1% ` as a response flag. +* access log: added support for nested objects in :ref:`JSON logging mode `. * build: enable building envoy :ref:`arm64 images ` by buildx tool in x86 CI platform. * dynamic_forward_proxy: added :ref:`use_tcp_for_dns_lookups` option to use TCP for DNS lookups in order to match the DNS options for :ref:`Clusters`. * ext_authz filter: added support for emitting dynamic metadata for both :ref:`HTTP ` and :ref:`network ` filters. diff --git a/source/common/formatter/substitution_format_string.cc b/source/common/formatter/substitution_format_string.cc index 57470b83412c..465ab49b4db7 100644 --- a/source/common/formatter/substitution_format_string.cc +++ b/source/common/formatter/substitution_format_string.cc @@ -4,27 +4,6 @@ namespace Envoy { namespace Formatter { -namespace { - -convertJsonFormatToMap(const ProtobufWkt::Struct& json_format) { - absl::flat_hash_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."); - } - output.emplace(pair.first, pair.second.string_value()); - } - return output; -} - -} // namespace - -FormatterPtr -SubstitutionFormatStringUtils::createJsonFormatter(const ProtobufWkt::Struct& struct_format, - bool preserve_types) { - auto json_format_map = convertJsonFormatToMap(struct_format); - return std::make_unique(json_format_map, preserve_types); -} FormatterPtr SubstitutionFormatStringUtils::fromProtoConfig( const envoy::config::core::v3::SubstitutionFormatString& config) { @@ -32,7 +11,7 @@ FormatterPtr SubstitutionFormatStringUtils::fromProtoConfig( case envoy::config::core::v3::SubstitutionFormatString::FormatCase::kTextFormat: return std::make_unique(config.text_format()); case envoy::config::core::v3::SubstitutionFormatString::FormatCase::kJsonFormat: { - return createJsonFormatter(config.json_format(), true); + return std::make_unique(config.json_format(), true); } default: NOT_REACHED_GCOVR_EXCL_LINE; diff --git a/source/common/formatter/substitution_format_string.h b/source/common/formatter/substitution_format_string.h index 6d514cecc47d..f94c8574785e 100644 --- a/source/common/formatter/substitution_format_string.h +++ b/source/common/formatter/substitution_format_string.h @@ -20,12 +20,6 @@ class SubstitutionFormatStringUtils { */ static FormatterPtr fromProtoConfig(const envoy::config::core::v3::SubstitutionFormatString& config); - - /** - * Generate a Json formatter object from proto::Struct config - */ - static FormatterPtr createJsonFormatter(const ProtobufWkt::Struct& struct_format, - bool preserve_types); }; } // namespace Formatter diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc index 4a3bb8a90cf8..086b8c477cb3 100644 --- a/source/common/formatter/substitution_formatter.cc +++ b/source/common/formatter/substitution_formatter.cc @@ -49,6 +49,9 @@ const std::regex& getStartTimeNewlinePattern() { } const std::regex& getNewlinePattern() { CONSTRUCT_ON_FIRST_USE(std::regex, "\n"); } +template struct JsonFormatMapVisitor : Ts... { using Ts::operator()...; }; +template JsonFormatMapVisitor(Ts...) -> JsonFormatMapVisitor; + } // namespace const std::string SubstitutionFormatUtils::DEFAULT_FORMAT = @@ -109,14 +112,6 @@ std::string FormatterImpl::format(const Http::RequestHeaderMap& request_headers, return log_line; } -JsonFormatterImpl::JsonFormatterImpl( - const absl::flat_hash_map& format_mapping, bool preserve_types) - : preserve_types_(preserve_types) { - for (const auto& pair : format_mapping) { - json_output_format_.emplace(pair.first, SubstitutionFormatParser::parse(pair.second)); - } -} - std::string JsonFormatterImpl::format(const Http::RequestHeaderMap& request_headers, const Http::ResponseHeaderMap& response_headers, const Http::ResponseTrailerMap& response_trailers, @@ -129,37 +124,60 @@ std::string JsonFormatterImpl::format(const Http::RequestHeaderMap& request_head return absl::StrCat(log_line, "\n"); } +JsonFormatMap JsonFormatterImpl::toFormatMap(const ProtobufWkt::Struct& json_format) const { + auto output = std::make_unique>(); + for (const auto& pair : json_format.fields()) { + switch (pair.second.kind_case()) { + case ProtobufWkt::Value::kStringValue: + output->emplace(pair.first, SubstitutionFormatParser::parse(pair.second.string_value())); + break; + case ProtobufWkt::Value::kStructValue: + output->emplace(pair.first, toFormatMap(pair.second.struct_value())); + break; + default: + throw EnvoyException( + "Only string/object values are supported in the JSON access log format."); + } + } + return {std::move(output)}; +}; + ProtobufWkt::Struct JsonFormatterImpl::toStruct(const Http::RequestHeaderMap& request_headers, const Http::ResponseHeaderMap& response_headers, const Http::ResponseTrailerMap& response_trailers, const StreamInfo::StreamInfo& stream_info, absl::string_view local_reply_body) const { - ProtobufWkt::Struct output; - auto* fields = output.mutable_fields(); - for (const auto& pair : json_output_format_) { - const auto& providers = pair.second; - ASSERT(!providers.empty()); - - if (providers.size() == 1) { - const auto& provider = providers.front(); - const auto val = - preserve_types_ ? provider->formatValue(request_headers, response_headers, - response_trailers, stream_info, local_reply_body) - : ValueUtil::stringValue( - provider->format(request_headers, response_headers, - response_trailers, stream_info, local_reply_body)); - (*fields)[pair.first] = val; - } else { - // Multiple providers forces string output. - std::string str; - for (const auto& provider : providers) { - str += provider->format(request_headers, response_headers, response_trailers, stream_info, - local_reply_body); - } - (*fields)[pair.first] = ValueUtil::stringValue(str); - } - } - return output; + const std::function&)> + providers_callback = [&](const std::vector& providers) { + ASSERT(!providers.empty()); + if (providers.size() == 1) { + const auto& provider = providers.front(); + if (preserve_types_) { + return provider->formatValue(request_headers, response_headers, response_trailers, + stream_info, local_reply_body); + } + return ValueUtil::stringValue(provider->format( + request_headers, response_headers, response_trailers, stream_info, local_reply_body)); + } + // Multiple providers forces string output. + std::string str; + for (const auto& provider : providers) { + str += provider->format(request_headers, response_headers, response_trailers, stream_info, + local_reply_body); + } + return ValueUtil::stringValue(str); + }; + const std::function json_format_map_callback = + [&](const JsonFormatMap& format) { + ProtobufWkt::Struct output; + auto* fields = output.mutable_fields(); + for (const auto& pair : *format.value_) { + (*fields)[pair.first] = std::visit( + JsonFormatMapVisitor{json_format_map_callback, providers_callback}, pair.second); + } + return ValueUtil::structValue(output); + }; + return json_format_map_callback(json_output_format_).struct_value(); } void SubstitutionFormatParser::parseCommandHeader(const std::string& token, const size_t start, diff --git a/source/common/formatter/substitution_formatter.h b/source/common/formatter/substitution_formatter.h index 6a6ff800e67e..f6e8b56b43ca 100644 --- a/source/common/formatter/substitution_formatter.h +++ b/source/common/formatter/substitution_formatter.h @@ -17,14 +17,6 @@ namespace Envoy { namespace Formatter { -// We actually need a map to formatters, we can pass the ProtobufStruct, no need to reformat it. -struct JsonMap; -using JsonMapPtr = std::unique_ptr; -using JsonMapValue = absl::variant; -struct JsonMap { - absl::flat_hash_map values_; -}; - /** * Access log format parser. */ @@ -108,10 +100,17 @@ class FormatterImpl : public Formatter { std::vector providers_; }; +struct JsonFormatMap; +using JsonFormatMapValue = + absl::variant, const JsonFormatMap>; +struct JsonFormatMap { + std::unique_ptr> value_; +}; + class JsonFormatterImpl : public Formatter { public: - JsonFormatterImpl(const absl::flat_hash_map& format_mapping, - bool preserve_types); + JsonFormatterImpl(const ProtobufWkt::Struct& format_mapping, bool preserve_types) + : preserve_types_(preserve_types), json_output_format_(toFormatMap(format_mapping)) {} // Formatter::format std::string format(const Http::RequestHeaderMap& request_headers, @@ -122,13 +121,14 @@ class JsonFormatterImpl : public Formatter { private: const bool preserve_types_; - std::map> json_output_format_; + const JsonFormatMap json_output_format_; ProtobufWkt::Struct toStruct(const Http::RequestHeaderMap& request_headers, const Http::ResponseHeaderMap& response_headers, const Http::ResponseTrailerMap& response_trailers, const StreamInfo::StreamInfo& stream_info, absl::string_view local_reply_body) const; + JsonFormatMap toFormatMap(const ProtobufWkt::Struct& json_format) const; }; /** diff --git a/test/common/formatter/substitution_format_string_test.cc b/test/common/formatter/substitution_format_string_test.cc index 22e4a030d430..340326599d89 100644 --- a/test/common/formatter/substitution_format_string_test.cc +++ b/test/common/formatter/substitution_format_string_test.cc @@ -21,7 +21,8 @@ class SubstitutionFormatStringUtilsTest : public ::testing::Test { EXPECT_CALL(stream_info_, responseCode()).WillRepeatedly(Return(response_code)); } - Http::TestRequestHeaderMapImpl request_headers_{{":method", "GET"}, {":path", "/bar/foo"}}; + Http::TestRequestHeaderMapImpl request_headers_{ + {":method", "GET"}, {":path", "/bar/foo"}, {"content-type", "application/json"}}; Http::TestResponseHeaderMapImpl response_headers_; Http::TestResponseTrailerMapImpl response_trailers_; StreamInfo::MockStreamInfo stream_info_; @@ -54,6 +55,8 @@ TEST_F(SubstitutionFormatStringUtilsTest, TestFromProtoConfigJson) { text: "plain text" path: "%REQ(:path)%" code: "%RESPONSE_CODE%" + headers: + content-type: "%REQ(CONTENT-TYPE)%" )EOF"; TestUtility::loadFromYaml(yaml, config_); @@ -64,7 +67,10 @@ TEST_F(SubstitutionFormatStringUtilsTest, TestFromProtoConfigJson) { const std::string expected = R"EOF({ "text": "plain text", "path": "/bar/foo", - "code": 200 + "code": 200, + "headers": { + "content-type": "application/json" + } })EOF"; EXPECT_TRUE(TestUtility::jsonStringEqual(out_json, expected)); } @@ -78,18 +84,13 @@ TEST_F(SubstitutionFormatStringUtilsTest, TestInvalidConfigs) { R"( json_format: field: 200 -)", - R"( - json_format: - field: - nest_field: "value" )", }; for (const auto& yaml : invalid_configs) { TestUtility::loadFromYaml(yaml, config_); - EXPECT_THROW_WITH_MESSAGE(SubstitutionFormatStringUtils::fromProtoConfig(config_), - EnvoyException, - "Only string values are supported in the JSON access log format."); + EXPECT_THROW_WITH_MESSAGE( + SubstitutionFormatStringUtils::fromProtoConfig(config_), EnvoyException, + "Only string/object values are supported in the JSON access log format."); } } diff --git a/test/common/formatter/substitution_formatter_speed_test.cc b/test/common/formatter/substitution_formatter_speed_test.cc index fd2b6c7fe7a9..29e3edc41850 100644 --- a/test/common/formatter/substitution_formatter_speed_test.cc +++ b/test/common/formatter/substitution_formatter_speed_test.cc @@ -11,18 +11,20 @@ namespace Envoy { namespace { std::unique_ptr makeJsonFormatter(bool typed) { - absl::flat_hash_map JsonLogFormat = { - {"remote_address", "%DOWNSTREAM_REMOTE_ADDRESS_WITHOUT_PORT%"}, - {"start_time", "%START_TIME(%Y/%m/%dT%H:%M:%S%z %s)%"}, - {"method", "%REQ(:METHOD)%"}, - {"url", "%REQ(X-FORWARDED-PROTO)%://%REQ(:AUTHORITY)%%REQ(X-ENVOY-ORIGINAL-PATH?:PATH)%"}, - {"protocol", "%PROTOCOL%"}, - {"respoinse_code", "%RESPONSE_CODE%"}, - {"bytes_sent", "%BYTES_SENT%"}, - {"duration", "%DURATION%"}, - {"referer", "%REQ(REFERER)%"}, - {"user-agent", "%REQ(USER-AGENT)%"}}; - + ProtobufWkt::Struct JsonLogFormat; + const std::string format_yaml = R"EOF( + remote_address: '%DOWNSTREAM_REMOTE_ADDRESS_WITHOUT_PORT%' + start_time: '%START_TIME(%Y/%m/%dT%H:%M:%S%z %s)%' + method: '%REQ(:METHOD)%' + url: '%REQ(X-FORWARDED-PROTO)%://%REQ(:AUTHORITY)%%REQ(X-ENVOY-ORIGINAL-PATH?:PATH)%' + protocol: '%PROTOCOL%' + respoinse_code: '%RESPONSE_CODE%' + bytes_sent: '%BYTES_SENT%' + duration: '%DURATION%' + referer: '%REQ(REFERER)%' + user-agent: '%REQ(USER-AGENT)%' + )EOF"; + TestUtility::loadFromYaml(format_yaml, JsonLogFormat); return std::make_unique(JsonLogFormat, typed); } diff --git a/test/common/formatter/substitution_formatter_test.cc b/test/common/formatter/substitution_formatter_test.cc index 66ed458a2344..9e13eee62106 100644 --- a/test/common/formatter/substitution_formatter_test.cc +++ b/test/common/formatter/substitution_formatter_test.cc @@ -1523,8 +1523,11 @@ TEST(SubstitutionFormatterTest, JsonFormatterPlainStringTest) { absl::node_hash_map expected_json_map = { {"plain_string", "plain_string_value"}}; - absl::flat_hash_map key_mapping = { - {"plain_string", "plain_string_value"}}; + ProtobufWkt::Struct key_mapping; + TestUtility::loadFromYaml(R"EOF( + plain_string: plain_string_value + )EOF", + key_mapping); JsonFormatterImpl formatter(key_mapping, false); verifyJsonOutput( @@ -1532,6 +1535,44 @@ TEST(SubstitutionFormatterTest, JsonFormatterPlainStringTest) { expected_json_map); } +TEST(SubstitutionFormatterTest, JsonFormatterNestedObject) { + StreamInfo::MockStreamInfo stream_info; + Http::TestRequestHeaderMapImpl request_header; + Http::TestResponseHeaderMapImpl response_header; + Http::TestResponseTrailerMapImpl response_trailer; + std::string body; + + envoy::config::core::v3::Metadata metadata; + populateMetadataTestData(metadata); + absl::optional protocol = Http::Protocol::Http11; + EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); + + ProtobufWkt::Struct key_mapping; + TestUtility::loadFromYaml(R"EOF( + level_one: + level_two: + level_three: + plain_string: plain_string_value + protocol: '%PROTOCOL%' + )EOF", + key_mapping); + JsonFormatterImpl formatter(key_mapping, false); + + const std::string expected = R"EOF({ + "level_one": { + "level_two": { + "level_three": { + "plain_string": "plain_string_value", + "protocol": "HTTP/1.1" + } + } + } + })EOF"; + std::string out_json = + formatter.format(request_header, response_header, response_trailer, stream_info, body); + EXPECT_TRUE(TestUtility::jsonStringEqual(out_json, expected)); +} + TEST(SubstitutionFormatterTest, JsonFormatterSingleOperatorTest) { StreamInfo::MockStreamInfo stream_info; Http::TestRequestHeaderMapImpl request_header; @@ -1546,7 +1587,11 @@ TEST(SubstitutionFormatterTest, JsonFormatterSingleOperatorTest) { absl::node_hash_map expected_json_map = {{"protocol", "HTTP/1.1"}}; - absl::flat_hash_map key_mapping = {{"protocol", "%PROTOCOL%"}}; + ProtobufWkt::Struct key_mapping; + TestUtility::loadFromYaml(R"EOF( + protocol: '%PROTOCOL%' + )EOF", + key_mapping); JsonFormatterImpl formatter(key_mapping, false); verifyJsonOutput( @@ -1567,11 +1612,14 @@ TEST(SubstitutionFormatterTest, JsonFormatterNonExistentHeaderTest) { {"nonexistent_response_header", "-"}, {"some_response_header", "SOME_RESPONSE_HEADER"}}; - absl::flat_hash_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)%"}}; + ProtobufWkt::Struct key_mapping; + TestUtility::loadFromYaml(R"EOF( + protocol: '%PROTOCOL%' + some_request_header: '%REQ(some_request_header)%' + nonexistent_response_header: '%RESP(nonexistent_response_header)%' + some_response_header: '%RESP(some_response_header)%' + )EOF", + key_mapping); JsonFormatterImpl formatter(key_mapping, false); absl::optional protocol = Http::Protocol::Http11; @@ -1597,15 +1645,14 @@ TEST(SubstitutionFormatterTest, JsonFormatterAlternateHeaderTest) { {"response_absent_header_or_response_absent_header", "RESPONSE_PRESENT_HEADER"}, {"response_present_header_or_response_absent_header", "RESPONSE_PRESENT_HEADER"}}; - absl::flat_hash_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)%"}}; + ProtobufWkt::Struct key_mapping; + TestUtility::loadFromYaml(R"EOF( + 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)%' + )EOF", + key_mapping); JsonFormatterImpl formatter(key_mapping, false); absl::optional protocol = Http::Protocol::Http11; @@ -1633,11 +1680,13 @@ TEST(SubstitutionFormatterTest, JsonFormatterDynamicMetadataTest) { {"test_obj", "{\"inner_key\":\"inner_value\"}"}, {"test_obj.inner_key", "\"inner_value\""}}; - absl::flat_hash_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)%"}}; - + ProtobufWkt::Struct key_mapping; + TestUtility::loadFromYaml(R"EOF( + 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)%' + )EOF", + key_mapping); JsonFormatterImpl formatter(key_mapping, false); verifyJsonOutput( @@ -1657,11 +1706,13 @@ TEST(SubstitutionFormatterTest, JsonFormatterTypedDynamicMetadataTest) { EXPECT_CALL(stream_info, dynamicMetadata()).WillRepeatedly(ReturnRef(metadata)); EXPECT_CALL(Const(stream_info), dynamicMetadata()).WillRepeatedly(ReturnRef(metadata)); - absl::flat_hash_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)%"}}; - + ProtobufWkt::Struct key_mapping; + TestUtility::loadFromYaml(R"EOF( + 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)%' + )EOF", + key_mapping); JsonFormatterImpl formatter(key_mapping, true); const std::string json = @@ -1693,9 +1744,12 @@ TEST(SubstitutionFormatterTest, JsonFormatterFilterStateTest) { absl::node_hash_map expected_json_map = { {"test_key", "\"test_value\""}, {"test_obj", "{\"inner_key\":\"inner_value\"}"}}; - absl::flat_hash_map key_mapping = { - {"test_key", "%FILTER_STATE(test_key)%"}, {"test_obj", "%FILTER_STATE(test_obj)%"}}; - + ProtobufWkt::Struct key_mapping; + TestUtility::loadFromYaml(R"EOF( + test_key: '%FILTER_STATE(test_key)%' + test_obj: '%FILTER_STATE(test_obj)%' + )EOF", + key_mapping); JsonFormatterImpl formatter(key_mapping, false); verifyJsonOutput( @@ -1717,9 +1771,12 @@ TEST(SubstitutionFormatterTest, JsonFormatterTypedFilterStateTest) { StreamInfo::FilterState::StateType::ReadOnly); EXPECT_CALL(Const(stream_info), filterState()).Times(testing::AtLeast(1)); - absl::flat_hash_map key_mapping = { - {"test_key", "%FILTER_STATE(test_key)%"}, {"test_obj", "%FILTER_STATE(test_obj)%"}}; - + ProtobufWkt::Struct key_mapping; + TestUtility::loadFromYaml(R"EOF( + test_key: '%FILTER_STATE(test_key)%' + test_obj: '%FILTER_STATE(test_obj)%' + )EOF", + key_mapping); JsonFormatterImpl formatter(key_mapping, true); std::string json = @@ -1751,10 +1808,12 @@ TEST(SubstitutionFormatterTest, FilterStateSpeciferTest) { {"test_key_typed", "\"test_value By TYPED\""}, }; - absl::flat_hash_map key_mapping = { - {"test_key_plain", "%FILTER_STATE(test_key:PLAIN)%"}, - {"test_key_typed", "%FILTER_STATE(test_key:TYPED)%"}}; - + ProtobufWkt::Struct key_mapping; + TestUtility::loadFromYaml(R"EOF( + test_key_plain: '%FILTER_STATE(test_key:PLAIN)%' + test_key_typed: '%FILTER_STATE(test_key:TYPED)%' + )EOF", + key_mapping); JsonFormatterImpl formatter(key_mapping, false); verifyJsonOutput( @@ -1775,10 +1834,12 @@ TEST(SubstitutionFormatterTest, TypedFilterStateSpeciferTest) { StreamInfo::FilterState::StateType::ReadOnly); EXPECT_CALL(Const(stream_info), filterState()).Times(testing::AtLeast(1)); - absl::flat_hash_map key_mapping = { - {"test_key_plain", "%FILTER_STATE(test_key:PLAIN)%"}, - {"test_key_typed", "%FILTER_STATE(test_key:TYPED)%"}}; - + ProtobufWkt::Struct key_mapping; + TestUtility::loadFromYaml(R"EOF( + test_key_plain: '%FILTER_STATE(test_key:PLAIN)%' + test_key_typed: '%FILTER_STATE(test_key:TYPED)%' + )EOF", + key_mapping); JsonFormatterImpl formatter(key_mapping, true); std::string json = @@ -1804,10 +1865,12 @@ TEST(SubstitutionFormatterTest, FilterStateErrorSpeciferTest) { StreamInfo::FilterState::StateType::ReadOnly); // 'ABCDE' is error specifier. - absl::flat_hash_map key_mapping = { - {"test_key_plain", "%FILTER_STATE(test_key:ABCDE)%"}, - {"test_key_typed", "%FILTER_STATE(test_key:TYPED)%"}}; - + ProtobufWkt::Struct key_mapping; + TestUtility::loadFromYaml(R"EOF( + test_key_plain: '%FILTER_STATE(test_key:ABCDE)%' + test_key_typed: '%FILTER_STATE(test_key:TYPED)%' + )EOF", + key_mapping); EXPECT_THROW_WITH_MESSAGE(JsonFormatterImpl formatter(key_mapping, false), EnvoyException, "Invalid filter state serialize type, only support PLAIN/TYPED."); } @@ -1830,12 +1893,15 @@ TEST(SubstitutionFormatterTest, JsonFormatterStartTimeTest) { {"default", "2018-03-28T23:35:58.000Z"}, {"all_zeroes", "000000000.0.00.000"}}; - absl::flat_hash_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)%"}}; + ProtobufWkt::Struct key_mapping; + TestUtility::loadFromYaml(R"EOF( + 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)%' + )EOF", + key_mapping); JsonFormatterImpl formatter(key_mapping, false); verifyJsonOutput( @@ -1855,10 +1921,11 @@ TEST(SubstitutionFormatterTest, JsonFormatterMultiTokenTest) { absl::node_hash_map expected_json_map = { {"multi_token_field", "HTTP/1.1 plainstring SOME_REQUEST_HEADER SOME_RESPONSE_HEADER"}}; - absl::flat_hash_map key_mapping = { - {"multi_token_field", - "%PROTOCOL% plainstring %REQ(some_request_header)% %RESP(some_response_header)%"}}; - + ProtobufWkt::Struct key_mapping; + TestUtility::loadFromYaml(R"EOF( + multi_token_field: '%PROTOCOL% plainstring %REQ(some_request_header)% %RESP(some_response_header)%' + )EOF", + key_mapping); for (const bool preserve_types : {false, true}) { JsonFormatterImpl formatter(key_mapping, preserve_types); @@ -1896,12 +1963,13 @@ TEST(SubstitutionFormatterTest, JsonFormatterTypedTest) { StreamInfo::FilterState::StateType::ReadOnly); EXPECT_CALL(Const(stream_info), filterState()).Times(testing::AtLeast(1)); - absl::flat_hash_map key_mapping = { - {"request_duration", "%REQUEST_DURATION%"}, - {"request_duration_multi", "%REQUEST_DURATION%ms"}, - {"filter_state", "%FILTER_STATE(test_obj)%"}, - }; - + ProtobufWkt::Struct key_mapping; + TestUtility::loadFromYaml(R"EOF( + request_duration: '%REQUEST_DURATION%' + request_duration_multi: '%REQUEST_DURATION%ms' + filter_state: '%FILTER_STATE(test_obj)%' + )EOF", + key_mapping); JsonFormatterImpl formatter(key_mapping, true); const auto json = From f36fe9646bfe2f467a419a4cb8be0c2a265cb2ad Mon Sep 17 00:00:00 2001 From: Petr Pchelko Date: Tue, 11 Aug 2020 21:23:17 -0700 Subject: [PATCH 3/7] Revert removal of createJsonFormatter Signed-off-by: Petr Pchelko --- source/common/formatter/substitution_format_string.cc | 8 +++++++- source/common/formatter/substitution_format_string.h | 6 ++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/source/common/formatter/substitution_format_string.cc b/source/common/formatter/substitution_format_string.cc index 465ab49b4db7..a94b755b8689 100644 --- a/source/common/formatter/substitution_format_string.cc +++ b/source/common/formatter/substitution_format_string.cc @@ -5,13 +5,19 @@ namespace Envoy { namespace Formatter { +FormatterPtr +SubstitutionFormatStringUtils::createJsonFormatter(const ProtobufWkt::Struct& struct_format, + bool preserve_types) { + return std::make_unique(struct_format, preserve_types); +} + FormatterPtr SubstitutionFormatStringUtils::fromProtoConfig( const envoy::config::core::v3::SubstitutionFormatString& config) { switch (config.format_case()) { case envoy::config::core::v3::SubstitutionFormatString::FormatCase::kTextFormat: return std::make_unique(config.text_format()); case envoy::config::core::v3::SubstitutionFormatString::FormatCase::kJsonFormat: { - return std::make_unique(config.json_format(), true); + return createJsonFormatter(config.json_format(), true); } default: NOT_REACHED_GCOVR_EXCL_LINE; diff --git a/source/common/formatter/substitution_format_string.h b/source/common/formatter/substitution_format_string.h index f94c8574785e..6d514cecc47d 100644 --- a/source/common/formatter/substitution_format_string.h +++ b/source/common/formatter/substitution_format_string.h @@ -20,6 +20,12 @@ class SubstitutionFormatStringUtils { */ static FormatterPtr fromProtoConfig(const envoy::config::core::v3::SubstitutionFormatString& config); + + /** + * Generate a Json formatter object from proto::Struct config + */ + static FormatterPtr createJsonFormatter(const ProtobufWkt::Struct& struct_format, + bool preserve_types); }; } // namespace Formatter From 9b0df3f633ebe970bd41777e7d73d21dcdabb86d Mon Sep 17 00:00:00 2001 From: Petr Pchelko Date: Thu, 13 Aug 2020 15:18:58 -0700 Subject: [PATCH 4/7] Improve exception message, make JsonFormatMap private Signed-off-by: Petr Pchelko --- source/common/formatter/substitution_formatter.cc | 14 +++++++------- source/common/formatter/substitution_formatter.h | 14 +++++++------- .../formatter/substitution_format_string_test.cc | 2 +- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc index 086b8c477cb3..bb193bfc7f5b 100644 --- a/source/common/formatter/substitution_formatter.cc +++ b/source/common/formatter/substitution_formatter.cc @@ -124,8 +124,8 @@ std::string JsonFormatterImpl::format(const Http::RequestHeaderMap& request_head return absl::StrCat(log_line, "\n"); } -JsonFormatMap JsonFormatterImpl::toFormatMap(const ProtobufWkt::Struct& json_format) const { - auto output = std::make_unique>(); +JsonFormatterImpl::JsonFormatMap JsonFormatterImpl::toFormatMap(const ProtobufWkt::Struct& json_format) const { + auto output = std::make_unique>(); for (const auto& pair : json_format.fields()) { switch (pair.second.kind_case()) { case ProtobufWkt::Value::kStringValue: @@ -136,7 +136,7 @@ JsonFormatMap JsonFormatterImpl::toFormatMap(const ProtobufWkt::Struct& json_for break; default: throw EnvoyException( - "Only string/object values are supported in the JSON access log format."); + "Only string values or nested structs are supported in the JSON access log format."); } } return {std::move(output)}; @@ -167,13 +167,13 @@ ProtobufWkt::Struct JsonFormatterImpl::toStruct(const Http::RequestHeaderMap& re } return ValueUtil::stringValue(str); }; - const std::function json_format_map_callback = - [&](const JsonFormatMap& format) { + const std::function json_format_map_callback = + [&](const JsonFormatterImpl::JsonFormatMap& format) { ProtobufWkt::Struct output; auto* fields = output.mutable_fields(); + JsonFormatMapVisitor visitor{json_format_map_callback, providers_callback}; for (const auto& pair : *format.value_) { - (*fields)[pair.first] = std::visit( - JsonFormatMapVisitor{json_format_map_callback, providers_callback}, pair.second); + (*fields)[pair.first] = std::visit(visitor, pair.second); } return ValueUtil::structValue(output); }; diff --git a/source/common/formatter/substitution_formatter.h b/source/common/formatter/substitution_formatter.h index f6e8b56b43ca..1a2164294d96 100644 --- a/source/common/formatter/substitution_formatter.h +++ b/source/common/formatter/substitution_formatter.h @@ -100,13 +100,6 @@ class FormatterImpl : public Formatter { std::vector providers_; }; -struct JsonFormatMap; -using JsonFormatMapValue = - absl::variant, const JsonFormatMap>; -struct JsonFormatMap { - std::unique_ptr> value_; -}; - class JsonFormatterImpl : public Formatter { public: JsonFormatterImpl(const ProtobufWkt::Struct& format_mapping, bool preserve_types) @@ -120,6 +113,13 @@ class JsonFormatterImpl : public Formatter { absl::string_view local_reply_body) const override; private: + struct JsonFormatMap; + using JsonFormatMapValue = + absl::variant, const JsonFormatMap>; + struct JsonFormatMap { + std::unique_ptr> value_; + }; + const bool preserve_types_; const JsonFormatMap json_output_format_; diff --git a/test/common/formatter/substitution_format_string_test.cc b/test/common/formatter/substitution_format_string_test.cc index 340326599d89..9109999ea8ae 100644 --- a/test/common/formatter/substitution_format_string_test.cc +++ b/test/common/formatter/substitution_format_string_test.cc @@ -90,7 +90,7 @@ TEST_F(SubstitutionFormatStringUtilsTest, TestInvalidConfigs) { TestUtility::loadFromYaml(yaml, config_); EXPECT_THROW_WITH_MESSAGE( SubstitutionFormatStringUtils::fromProtoConfig(config_), EnvoyException, - "Only string/object values are supported in the JSON access log format."); + "Only string values or nested structs are supported in the JSON access log format."); } } From aed8788d6b20e1d4b89e3050eae4fedc096ecb14 Mon Sep 17 00:00:00 2001 From: Petr Pchelko Date: Thu, 13 Aug 2020 15:59:43 -0700 Subject: [PATCH 5/7] Fix format Signed-off-by: Petr Pchelko --- source/common/formatter/substitution_formatter.cc | 7 ++++--- source/common/formatter/substitution_formatter.h | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc index bb193bfc7f5b..4babd5be3635 100644 --- a/source/common/formatter/substitution_formatter.cc +++ b/source/common/formatter/substitution_formatter.cc @@ -124,7 +124,8 @@ std::string JsonFormatterImpl::format(const Http::RequestHeaderMap& request_head return absl::StrCat(log_line, "\n"); } -JsonFormatterImpl::JsonFormatMap JsonFormatterImpl::toFormatMap(const ProtobufWkt::Struct& json_format) const { +JsonFormatterImpl::JsonFormatMap +JsonFormatterImpl::toFormatMap(const ProtobufWkt::Struct& json_format) const { auto output = std::make_unique>(); for (const auto& pair : json_format.fields()) { switch (pair.second.kind_case()) { @@ -167,8 +168,8 @@ ProtobufWkt::Struct JsonFormatterImpl::toStruct(const Http::RequestHeaderMap& re } return ValueUtil::stringValue(str); }; - const std::function json_format_map_callback = - [&](const JsonFormatterImpl::JsonFormatMap& format) { + const std::function + json_format_map_callback = [&](const JsonFormatterImpl::JsonFormatMap& format) { ProtobufWkt::Struct output; auto* fields = output.mutable_fields(); JsonFormatMapVisitor visitor{json_format_map_callback, providers_callback}; diff --git a/source/common/formatter/substitution_formatter.h b/source/common/formatter/substitution_formatter.h index 1a2164294d96..1837c7770263 100644 --- a/source/common/formatter/substitution_formatter.h +++ b/source/common/formatter/substitution_formatter.h @@ -115,7 +115,7 @@ class JsonFormatterImpl : public Formatter { private: struct JsonFormatMap; using JsonFormatMapValue = - absl::variant, const JsonFormatMap>; + absl::variant, const JsonFormatMap>; struct JsonFormatMap { std::unique_ptr> value_; }; From 8e3ad2590f73619bbcbaa07f8ca8f975c58e72d9 Mon Sep 17 00:00:00 2001 From: Petr Pchelko Date: Wed, 19 Aug 2020 21:48:20 -0700 Subject: [PATCH 6/7] Add more comments and typedefs Signed-off-by: Petr Pchelko --- .../common/formatter/substitution_formatter.cc | 8 ++++---- source/common/formatter/substitution_formatter.h | 16 ++++++++++------ 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc index 4babd5be3635..934b2726d916 100644 --- a/source/common/formatter/substitution_formatter.cc +++ b/source/common/formatter/substitution_formatter.cc @@ -124,9 +124,9 @@ std::string JsonFormatterImpl::format(const Http::RequestHeaderMap& request_head return absl::StrCat(log_line, "\n"); } -JsonFormatterImpl::JsonFormatMap +JsonFormatterImpl::JsonFormatMapWrapper JsonFormatterImpl::toFormatMap(const ProtobufWkt::Struct& json_format) const { - auto output = std::make_unique>(); + auto output = std::make_unique(); for (const auto& pair : json_format.fields()) { switch (pair.second.kind_case()) { case ProtobufWkt::Value::kStringValue: @@ -168,8 +168,8 @@ ProtobufWkt::Struct JsonFormatterImpl::toStruct(const Http::RequestHeaderMap& re } return ValueUtil::stringValue(str); }; - const std::function - json_format_map_callback = [&](const JsonFormatterImpl::JsonFormatMap& format) { + const std::function + json_format_map_callback = [&](const JsonFormatterImpl::JsonFormatMapWrapper& format) { ProtobufWkt::Struct output; auto* fields = output.mutable_fields(); JsonFormatMapVisitor visitor{json_format_map_callback, providers_callback}; diff --git a/source/common/formatter/substitution_formatter.h b/source/common/formatter/substitution_formatter.h index 1837c7770263..290340b6c843 100644 --- a/source/common/formatter/substitution_formatter.h +++ b/source/common/formatter/substitution_formatter.h @@ -113,22 +113,26 @@ class JsonFormatterImpl : public Formatter { absl::string_view local_reply_body) const override; private: - struct JsonFormatMap; + struct JsonFormatMapWrapper; using JsonFormatMapValue = - absl::variant, const JsonFormatMap>; - struct JsonFormatMap { - std::unique_ptr> value_; + absl::variant, const JsonFormatMapWrapper>; + // Although not required for JSON, it is nice to have the order of properties + // preserved between the format and the log entry, thus std::map + using JsonFormatMap = std::map; + using JsonFormatMapPtr = std::unique_ptr; + struct JsonFormatMapWrapper{ + JsonFormatMapPtr value_; }; const bool preserve_types_; - const JsonFormatMap json_output_format_; + const JsonFormatMapWrapper json_output_format_; ProtobufWkt::Struct toStruct(const Http::RequestHeaderMap& request_headers, const Http::ResponseHeaderMap& response_headers, const Http::ResponseTrailerMap& response_trailers, const StreamInfo::StreamInfo& stream_info, absl::string_view local_reply_body) const; - JsonFormatMap toFormatMap(const ProtobufWkt::Struct& json_format) const; + JsonFormatMapWrapper toFormatMap(const ProtobufWkt::Struct& json_format) const; }; /** From cabef016c8fd4c8f7a90377daf9bc29c8d2b967e Mon Sep 17 00:00:00 2001 From: Petr Pchelko Date: Thu, 20 Aug 2020 06:41:47 -0700 Subject: [PATCH 7/7] Fix format Signed-off-by: Petr Pchelko --- source/common/formatter/substitution_formatter.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/formatter/substitution_formatter.h b/source/common/formatter/substitution_formatter.h index 290340b6c843..0907fa128a4f 100644 --- a/source/common/formatter/substitution_formatter.h +++ b/source/common/formatter/substitution_formatter.h @@ -117,10 +117,10 @@ class JsonFormatterImpl : public Formatter { using JsonFormatMapValue = absl::variant, const JsonFormatMapWrapper>; // Although not required for JSON, it is nice to have the order of properties - // preserved between the format and the log entry, thus std::map + // preserved between the format and the log entry, thus std::map. using JsonFormatMap = std::map; using JsonFormatMapPtr = std::unique_ptr; - struct JsonFormatMapWrapper{ + struct JsonFormatMapWrapper { JsonFormatMapPtr value_; };