From df16f807c5508ddd269a4be87086114864a607de Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Wed, 24 Mar 2021 11:52:02 -0400 Subject: [PATCH 01/12] access log: support formatter extensions from json_format This was missed in #14512. Signed-off-by: Raul Gutierrez Segales --- docs/root/version_history/current.rst | 1 + .../formatter/substitution_format_string.cc | 14 +++++--- .../formatter/substitution_formatter.cc | 11 ++++--- .../common/formatter/substitution_formatter.h | 10 +++--- .../substitution_format_string_test.cc | 33 +++++++++++++++++++ 5 files changed, 56 insertions(+), 13 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 0e9b565afbb12..abc0d0dce07f9 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -124,6 +124,7 @@ New Features * access log: added the new response flag `NC` for upstream cluster not found. The error flag is set when the http or tcp route is found for the request but the cluster is not available. * access log: added the :ref:`formatters ` extension point for custom formatters (command operators). * access log: added support for cross platform writing to :ref:`standard output ` and :ref:`standard error `. +* access log: extended the :ref:`formatters ` extension point to support json format. * access log: support command operator: %FILTER_CHAIN_NAME% for the downstream tcp and http request. * access log: support command operator: %REQUEST_HEADERS_BYTES%, %RESPONSE_HEADERS_BYTES%, and %RESPONSE_TRAILERS_BYTES%. * compression: add brotli :ref:`compressor ` and :ref:`decompressor `. diff --git a/source/common/formatter/substitution_format_string.cc b/source/common/formatter/substitution_format_string.cc index bddaac039a7f1..5cd4a2f22e355 100644 --- a/source/common/formatter/substitution_format_string.cc +++ b/source/common/formatter/substitution_format_string.cc @@ -12,7 +12,9 @@ namespace Formatter { FormatterPtr SubstitutionFormatStringUtils::createJsonFormatter(const ProtobufWkt::Struct& struct_format, bool preserve_types, bool omit_empty_values) { - return std::make_unique(struct_format, preserve_types, omit_empty_values); + std::vector commands; + return std::make_unique(struct_format, preserve_types, omit_empty_values, + commands); } FormatterPtr SubstitutionFormatStringUtils::fromProtoConfig( @@ -25,6 +27,9 @@ FormatterPtr SubstitutionFormatStringUtils::fromProtoConfig( throw EnvoyException(absl::StrCat("Formatter not found: ", formatter.name())); } auto parser = factory->createCommandParserFromProto(formatter.typed_config()); + if (!parser) { + throw EnvoyException(absl::StrCat("Failed to create command parser: ", formatter.name())); + } commands.push_back(std::move(parser)); } @@ -32,15 +37,16 @@ FormatterPtr SubstitutionFormatStringUtils::fromProtoConfig( case envoy::config::core::v3::SubstitutionFormatString::FormatCase::kTextFormat: return std::make_unique(config.text_format(), config.omit_empty_values(), commands); - case envoy::config::core::v3::SubstitutionFormatString::FormatCase::kJsonFormat: { - return createJsonFormatter(config.json_format(), true, config.omit_empty_values()); + case envoy::config::core::v3::SubstitutionFormatString::FormatCase::kJsonFormat: + return std::make_unique(config.json_format(), true, + config.omit_empty_values(), commands); case envoy::config::core::v3::SubstitutionFormatString::FormatCase::kTextFormatSource: return std::make_unique( Config::DataSource::read(config.text_format_source(), true, api), false, commands); - } default: NOT_REACHED_GCOVR_EXCL_LINE; } + return nullptr; } diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc index 2cc684ac0dd43..f3e669cc5ed4b 100644 --- a/source/common/formatter/substitution_formatter.cc +++ b/source/common/formatter/substitution_formatter.cc @@ -149,10 +149,11 @@ std::string JsonFormatterImpl::format(const Http::RequestHeaderMap& request_head } StructFormatter::StructFormatter(const ProtobufWkt::Struct& format_mapping, bool preserve_types, - bool omit_empty_values) + bool omit_empty_values, + const std::vector& commands) : omit_empty_values_(omit_empty_values), preserve_types_(preserve_types), empty_value_(omit_empty_values_ ? EMPTY_STRING : DefaultUnspecifiedValueString), - struct_output_format_(toFormatMapValue(format_mapping)) {} + commands_(commands), struct_output_format_(toFormatMapValue(format_mapping)) {} StructFormatter::StructFormatMapWrapper StructFormatter::toFormatMapValue(const ProtobufWkt::Struct& struct_format) const { @@ -177,7 +178,7 @@ StructFormatter::toFormatMapValue(const ProtobufWkt::Struct& struct_format) cons } } return {std::move(output)}; -}; +} StructFormatter::StructFormatListWrapper StructFormatter::toFormatListValue(const ProtobufWkt::ListValue& list_value_format) const { @@ -205,8 +206,8 @@ StructFormatter::toFormatListValue(const ProtobufWkt::ListValue& list_value_form std::vector StructFormatter::toFormatStringValue(const std::string& string_format) const { - return SubstitutionFormatParser::parse(string_format); -}; + return SubstitutionFormatParser::parse(string_format, commands_); +} ProtobufWkt::Value StructFormatter::providersCallback( const std::vector& providers, diff --git a/source/common/formatter/substitution_formatter.h b/source/common/formatter/substitution_formatter.h index d9e2f53b4661f..c758cb9c36d74 100644 --- a/source/common/formatter/substitution_formatter.h +++ b/source/common/formatter/substitution_formatter.h @@ -134,7 +134,7 @@ template StructFormatMapVisitorHelper(Ts...) -> StructFormatMapVis class StructFormatter { public: StructFormatter(const ProtobufWkt::Struct& format_mapping, bool preserve_types, - bool omit_empty_values); + bool omit_empty_values, const std::vector& commands); ProtobufWkt::Struct format(const Http::RequestHeaderMap& request_headers, const Http::ResponseHeaderMap& response_headers, @@ -189,16 +189,18 @@ class StructFormatter { const bool omit_empty_values_; const bool preserve_types_; const std::string empty_value_; + // Note: don't use this ref beyond the constructor. + const std::vector& commands_; const StructFormatMapWrapper struct_output_format_; -}; // namespace Formatter +}; using StructFormatterPtr = std::unique_ptr; class JsonFormatterImpl : public Formatter { public: JsonFormatterImpl(const ProtobufWkt::Struct& format_mapping, bool preserve_types, - bool omit_empty_values) - : struct_formatter_(format_mapping, preserve_types, omit_empty_values) {} + bool omit_empty_values, const std::vector& commands) + : struct_formatter_(format_mapping, preserve_types, omit_empty_values, commands) {} // Formatter::format std::string format(const Http::RequestHeaderMap& request_headers, diff --git a/test/common/formatter/substitution_format_string_test.cc b/test/common/formatter/substitution_format_string_test.cc index 37e83fe3caa9f..7806d3c9ec29b 100644 --- a/test/common/formatter/substitution_format_string_test.cc +++ b/test/common/formatter/substitution_format_string_test.cc @@ -135,5 +135,38 @@ TEST_F(SubstitutionFormatStringUtilsTest, TestFromProtoConfigFormatterExtensionU "Formatter not found: envoy.formatter.TestFormatterUnknown"); } +TEST_F(SubstitutionFormatStringUtilsTest, TestFromProtoConfigJsonWithExtension) { + TestCommandFactory factory; + Registry::InjectFactory command_register(factory); + + const std::string yaml = R"EOF( + json_format: + text: "plain text %COMMAND_EXTENSION()%" + path: "%REQ(:path)%" + code: "%RESPONSE_CODE%" + headers: + content-type: "%REQ(CONTENT-TYPE)%" + formatters: + - name: envoy.formatter.TestFormatter + typed_config: + "@type": type.googleapis.com/google.protobuf.StringValue +)EOF"; + TestUtility::loadFromYaml(yaml, config_); + + auto formatter = SubstitutionFormatStringUtils::fromProtoConfig(config_, context_.api()); + const auto out_json = formatter->format(request_headers_, response_headers_, response_trailers_, + stream_info_, body_); + + const std::string expected = R"EOF({ + "text": "plain text TestFormatter", + "path": "/bar/foo", + "code": 200, + "headers": { + "content-type": "application/json" + } +})EOF"; + EXPECT_TRUE(TestUtility::jsonStringEqual(out_json, expected)); +} + } // namespace Formatter } // namespace Envoy From 84c65ce1bff74230da80618259952522b787f304 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Wed, 24 Mar 2021 13:20:36 -0400 Subject: [PATCH 02/12] Re-add StructFormatter's prev constructor to avoid breakage Signed-off-by: Raul Gutierrez Segales --- source/common/formatter/substitution_formatter.cc | 12 +++++++++++- source/common/formatter/substitution_formatter.h | 9 +++++++-- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc index f3e669cc5ed4b..8e094f04a665b 100644 --- a/source/common/formatter/substitution_formatter.cc +++ b/source/common/formatter/substitution_formatter.cc @@ -155,6 +155,12 @@ StructFormatter::StructFormatter(const ProtobufWkt::Struct& format_mapping, bool empty_value_(omit_empty_values_ ? EMPTY_STRING : DefaultUnspecifiedValueString), commands_(commands), struct_output_format_(toFormatMapValue(format_mapping)) {} +StructFormatter::StructFormatter(const ProtobufWkt::Struct& format_mapping, bool preserve_types, + bool omit_empty_values) + : omit_empty_values_(omit_empty_values), preserve_types_(preserve_types), + empty_value_(omit_empty_values_ ? EMPTY_STRING : DefaultUnspecifiedValueString), + commands_(absl::nullopt), struct_output_format_(toFormatMapValue(format_mapping)) {} + StructFormatter::StructFormatMapWrapper StructFormatter::toFormatMapValue(const ProtobufWkt::Struct& struct_format) const { auto output = std::make_unique(); @@ -206,7 +212,11 @@ StructFormatter::toFormatListValue(const ProtobufWkt::ListValue& list_value_form std::vector StructFormatter::toFormatStringValue(const std::string& string_format) const { - return SubstitutionFormatParser::parse(string_format, commands_); + if (commands_.has_value()) { + return SubstitutionFormatParser::parse(string_format, commands_.value()); + } + + return SubstitutionFormatParser::parse(string_format); } ProtobufWkt::Value StructFormatter::providersCallback( diff --git a/source/common/formatter/substitution_formatter.h b/source/common/formatter/substitution_formatter.h index c758cb9c36d74..1c437760e3d4d 100644 --- a/source/common/formatter/substitution_formatter.h +++ b/source/common/formatter/substitution_formatter.h @@ -133,6 +133,8 @@ template StructFormatMapVisitorHelper(Ts...) -> StructFormatMapVis */ class StructFormatter { public: + StructFormatter(const ProtobufWkt::Struct& format_mapping, bool preserve_types, + bool omit_empty_values); StructFormatter(const ProtobufWkt::Struct& format_mapping, bool preserve_types, bool omit_empty_values, const std::vector& commands); @@ -189,8 +191,11 @@ class StructFormatter { const bool omit_empty_values_; const bool preserve_types_; const std::string empty_value_; - // Note: don't use this ref beyond the constructor. - const std::vector& commands_; + + // Note: don't use this ref outside of the constructor. + using CommandsRef = std::reference_wrapper>; + const absl::optional commands_; + const StructFormatMapWrapper struct_output_format_; }; From 6633a3de14d8027e5b54f286e48252d4efc2888a Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Wed, 24 Mar 2021 14:19:19 -0400 Subject: [PATCH 03/12] Keep the old JsonFormatterImpl constructor around Convenient for other users of the class. Signed-off-by: Raul Gutierrez Segales --- source/common/formatter/substitution_formatter.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/source/common/formatter/substitution_formatter.h b/source/common/formatter/substitution_formatter.h index 1c437760e3d4d..5030eb3d0a4ac 100644 --- a/source/common/formatter/substitution_formatter.h +++ b/source/common/formatter/substitution_formatter.h @@ -203,6 +203,9 @@ using StructFormatterPtr = std::unique_ptr; class JsonFormatterImpl : public Formatter { public: + JsonFormatterImpl(const ProtobufWkt::Struct& format_mapping, bool preserve_types, + bool omit_empty_values) + : struct_formatter_(format_mapping, preserve_types, omit_empty_values) {} JsonFormatterImpl(const ProtobufWkt::Struct& format_mapping, bool preserve_types, bool omit_empty_values, const std::vector& commands) : struct_formatter_(format_mapping, preserve_types, omit_empty_values, commands) {} From 6969e3437dd7a7fff367d1be41b7d1cabf0b89e6 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Wed, 24 Mar 2021 15:04:12 -0400 Subject: [PATCH 04/12] Dmitri's review * extend the existing test to use the extension in multiple parts of the JSON doc * add additional test for multiple extensions Signed-off-by: Raul Gutierrez Segales --- test/common/formatter/command_extension.cc | 37 +++++++++++++++ test/common/formatter/command_extension.h | 25 +++++++++++ .../substitution_format_string_test.cc | 45 ++++++++++++++++--- 3 files changed, 101 insertions(+), 6 deletions(-) diff --git a/test/common/formatter/command_extension.cc b/test/common/formatter/command_extension.cc index 4dd2faa346255..ccf8c9ce84512 100644 --- a/test/common/formatter/command_extension.cc +++ b/test/common/formatter/command_extension.cc @@ -41,5 +41,42 @@ ProtobufTypes::MessagePtr TestCommandFactory::createEmptyConfigProto() { std::string TestCommandFactory::name() const { return "envoy.formatter.TestFormatter"; } +absl::optional AdditionalFormatter::format(const Http::RequestHeaderMap&, + const Http::ResponseHeaderMap&, + const Http::ResponseTrailerMap&, + const StreamInfo::StreamInfo&, + absl::string_view) const { + return "AdditionalFormatter"; +} + +ProtobufWkt::Value AdditionalFormatter::formatValue(const Http::RequestHeaderMap&, + const Http::ResponseHeaderMap&, + const Http::ResponseTrailerMap&, + const StreamInfo::StreamInfo&, + absl::string_view) const { + return ValueUtil::stringValue(""); +} + +FormatterProviderPtr AdditionalCommandParser::parse(const std::string& token, size_t, + size_t) const { + if (absl::StartsWith(token, "ADDITIONAL_EXTENSION")) { + return std::make_unique(); + } + + return nullptr; +} + +CommandParserPtr AdditionalCommandFactory::createCommandParserFromProto(const Protobuf::Message&) { + return std::make_unique(); +} + +std::string AdditionalCommandFactory::configType() { return "google.protobuf.UInt32Value"; } + +ProtobufTypes::MessagePtr AdditionalCommandFactory::createEmptyConfigProto() { + return std::make_unique(); +} + +std::string AdditionalCommandFactory::name() const { return "envoy.formatter.AdditionalFormatter"; } + } // namespace Formatter } // namespace Envoy diff --git a/test/common/formatter/command_extension.h b/test/common/formatter/command_extension.h index 3afa5e0333747..e6247bde7cbb8 100644 --- a/test/common/formatter/command_extension.h +++ b/test/common/formatter/command_extension.h @@ -35,5 +35,30 @@ class TestCommandFactory : public CommandParserFactory { std::string name() const override; }; +class AdditionalFormatter : public FormatterProvider { +public: + // FormatterProvider + absl::optional format(const Http::RequestHeaderMap&, const Http::ResponseHeaderMap&, + const Http::ResponseTrailerMap&, const StreamInfo::StreamInfo&, + absl::string_view) const override; + ProtobufWkt::Value formatValue(const Http::RequestHeaderMap&, const Http::ResponseHeaderMap&, + const Http::ResponseTrailerMap&, const StreamInfo::StreamInfo&, + absl::string_view) const override; +}; + +class AdditionalCommandParser : public CommandParser { +public: + AdditionalCommandParser() = default; + FormatterProviderPtr parse(const std::string& token, size_t, size_t) const override; +}; + +class AdditionalCommandFactory : public CommandParserFactory { +public: + CommandParserPtr createCommandParserFromProto(const Protobuf::Message&) override; + std::string configType() override; + ProtobufTypes::MessagePtr createEmptyConfigProto() override; + std::string name() const override; +}; + } // namespace Formatter } // namespace Envoy diff --git a/test/common/formatter/substitution_format_string_test.cc b/test/common/formatter/substitution_format_string_test.cc index 7806d3c9ec29b..605bf6ee33024 100644 --- a/test/common/formatter/substitution_format_string_test.cc +++ b/test/common/formatter/substitution_format_string_test.cc @@ -142,10 +142,10 @@ TEST_F(SubstitutionFormatStringUtilsTest, TestFromProtoConfigJsonWithExtension) const std::string yaml = R"EOF( json_format: text: "plain text %COMMAND_EXTENSION()%" - path: "%REQ(:path)%" - code: "%RESPONSE_CODE%" + path: "%REQ(:path)% %COMMAND_EXTENSION()%" + code: "%RESPONSE_CODE% %COMMAND_EXTENSION()%" headers: - content-type: "%REQ(CONTENT-TYPE)%" + content-type: "%REQ(CONTENT-TYPE)% %COMMAND_EXTENSION()%" formatters: - name: envoy.formatter.TestFormatter typed_config: @@ -159,12 +159,45 @@ TEST_F(SubstitutionFormatStringUtilsTest, TestFromProtoConfigJsonWithExtension) const std::string expected = R"EOF({ "text": "plain text TestFormatter", - "path": "/bar/foo", - "code": 200, + "path": "/bar/foo TestFormatter", + "code": "200 TestFormatter", "headers": { - "content-type": "application/json" + "content-type": "application/json TestFormatter" } })EOF"; + + EXPECT_TRUE(TestUtility::jsonStringEqual(out_json, expected)); +} + +TEST_F(SubstitutionFormatStringUtilsTest, TestFromProtoConfigJsonWithMultipleExtensions) { + TestCommandFactory test_factory; + Registry::InjectFactory test_command_register(test_factory); + AdditionalCommandFactory additional_factory; + Registry::InjectFactory additional_command_register(additional_factory); + + const std::string yaml = R"EOF( + json_format: + text: "plain text %COMMAND_EXTENSION()%" + path: "%REQ(:path)% %ADDITIONAL_EXTENSION()%" + formatters: + - name: envoy.formatter.TestFormatter + typed_config: + "@type": type.googleapis.com/google.protobuf.StringValue + - name: envoy.formatter.AdditionalFormatter + typed_config: + "@type": type.googleapis.com/google.protobuf.UInt32Value +)EOF"; + TestUtility::loadFromYaml(yaml, config_); + + auto formatter = SubstitutionFormatStringUtils::fromProtoConfig(config_, context_.api()); + const auto out_json = formatter->format(request_headers_, response_headers_, response_trailers_, + stream_info_, body_); + + const std::string expected = R"EOF({ + "text": "plain text TestFormatter", + "path": "/bar/foo AdditionalFormatter", +})EOF"; + EXPECT_TRUE(TestUtility::jsonStringEqual(out_json, expected)); } From 90334351c50710959900a22a05fee1ed27e0253b Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Wed, 24 Mar 2021 15:08:21 -0400 Subject: [PATCH 05/12] Improve wording in the changelog Signed-off-by: Raul Gutierrez Segales --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 92a60b75a6e9e..e8011da04ffe6 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -125,7 +125,7 @@ New Features * access log: added the new response flag `NC` for upstream cluster not found. The error flag is set when the http or tcp route is found for the request but the cluster is not available. * access log: added the :ref:`formatters ` extension point for custom formatters (command operators). * access log: added support for cross platform writing to :ref:`standard output ` and :ref:`standard error `. -* access log: extended the :ref:`formatters ` extension point to support json format. +* access log: added support for json format in the :ref:`formatters ` extension point. * access log: support command operator: %FILTER_CHAIN_NAME% for the downstream tcp and http request. * access log: support command operator: %REQUEST_HEADERS_BYTES%, %RESPONSE_HEADERS_BYTES%, and %RESPONSE_TRAILERS_BYTES%. * compression: add brotli :ref:`compressor ` and :ref:`decompressor `. From c96edb358fb97e3e477e3c66946c63637b8339d8 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Thu, 25 Mar 2021 15:38:55 -0400 Subject: [PATCH 06/12] Nuke explicit default constructors TODO(rgs1): please add a clang-tidy check that flags these Signed-off-by: Raul Gutierrez Segales --- test/common/formatter/command_extension.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/common/formatter/command_extension.h b/test/common/formatter/command_extension.h index e6247bde7cbb8..ad4f61e6e59af 100644 --- a/test/common/formatter/command_extension.h +++ b/test/common/formatter/command_extension.h @@ -23,7 +23,6 @@ class TestFormatter : public FormatterProvider { class TestCommandParser : public CommandParser { public: - TestCommandParser() = default; FormatterProviderPtr parse(const std::string& token, size_t, size_t) const override; }; @@ -48,7 +47,6 @@ class AdditionalFormatter : public FormatterProvider { class AdditionalCommandParser : public CommandParser { public: - AdditionalCommandParser() = default; FormatterProviderPtr parse(const std::string& token, size_t, size_t) const override; }; From 18ea1462732bc8c909e7dcff95825e4509ad7f69 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Thu, 25 Mar 2021 16:13:01 -0400 Subject: [PATCH 07/12] Clean things up around handling commands_ Signed-off-by: Raul Gutierrez Segales --- source/common/formatter/substitution_formatter.cc | 14 ++++++++------ source/common/formatter/substitution_formatter.h | 4 ++-- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc index 8e094f04a665b..8ca53527c4548 100644 --- a/source/common/formatter/substitution_formatter.cc +++ b/source/common/formatter/substitution_formatter.cc @@ -153,7 +153,12 @@ StructFormatter::StructFormatter(const ProtobufWkt::Struct& format_mapping, bool const std::vector& commands) : omit_empty_values_(omit_empty_values), preserve_types_(preserve_types), empty_value_(omit_empty_values_ ? EMPTY_STRING : DefaultUnspecifiedValueString), - commands_(commands), struct_output_format_(toFormatMapValue(format_mapping)) {} + commands_(commands), struct_output_format_(toFormatMapValue(format_mapping)) { + // Note: to avoid copying commands we save the ref in an optional for toFormatMapValue and related + // methods to use. Once those are done we need to unset the optional to avoid accidentally + // accessing a dangling reference. + commands_ = absl::nullopt; +} StructFormatter::StructFormatter(const ProtobufWkt::Struct& format_mapping, bool preserve_types, bool omit_empty_values) @@ -212,11 +217,8 @@ StructFormatter::toFormatListValue(const ProtobufWkt::ListValue& list_value_form std::vector StructFormatter::toFormatStringValue(const std::string& string_format) const { - if (commands_.has_value()) { - return SubstitutionFormatParser::parse(string_format, commands_.value()); - } - - return SubstitutionFormatParser::parse(string_format); + std::vector commands; + return SubstitutionFormatParser::parse(string_format, commands_.value_or(commands)); } ProtobufWkt::Value StructFormatter::providersCallback( diff --git a/source/common/formatter/substitution_formatter.h b/source/common/formatter/substitution_formatter.h index 5030eb3d0a4ac..515311f69a073 100644 --- a/source/common/formatter/substitution_formatter.h +++ b/source/common/formatter/substitution_formatter.h @@ -192,9 +192,9 @@ class StructFormatter { const bool preserve_types_; const std::string empty_value_; - // Note: don't use this ref outside of the constructor. + // Note: this ref becomes invalid outside of the constructor, so we unset it after that. using CommandsRef = std::reference_wrapper>; - const absl::optional commands_; + absl::optional commands_; const StructFormatMapWrapper struct_output_format_; }; From 6e821c4f1d71412089dd9a31d80a4aa0fec0a172 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Thu, 25 Mar 2021 16:27:47 -0400 Subject: [PATCH 08/12] Add test for parser creation failure Signed-off-by: Raul Gutierrez Segales --- test/common/formatter/command_extension.cc | 12 +++++++++++ test/common/formatter/command_extension.h | 8 ++++++++ .../substitution_format_string_test.cc | 20 +++++++++++++++++++ 3 files changed, 40 insertions(+) diff --git a/test/common/formatter/command_extension.cc b/test/common/formatter/command_extension.cc index ccf8c9ce84512..724650d1811a4 100644 --- a/test/common/formatter/command_extension.cc +++ b/test/common/formatter/command_extension.cc @@ -78,5 +78,17 @@ ProtobufTypes::MessagePtr AdditionalCommandFactory::createEmptyConfigProto() { std::string AdditionalCommandFactory::name() const { return "envoy.formatter.AdditionalFormatter"; } +CommandParserPtr FailCommandFactory::createCommandParserFromProto(const Protobuf::Message&) { + return nullptr; +} + +std::string FailCommandFactory::configType() { return "google.protobuf.UInt64Value"; } + +ProtobufTypes::MessagePtr FailCommandFactory::createEmptyConfigProto() { + return std::make_unique(); +} + +std::string FailCommandFactory::name() const { return "envoy.formatter.FailFormatter"; } + } // namespace Formatter } // namespace Envoy diff --git a/test/common/formatter/command_extension.h b/test/common/formatter/command_extension.h index ad4f61e6e59af..2abba003d9b2c 100644 --- a/test/common/formatter/command_extension.h +++ b/test/common/formatter/command_extension.h @@ -58,5 +58,13 @@ class AdditionalCommandFactory : public CommandParserFactory { std::string name() const override; }; +class FailCommandFactory : public CommandParserFactory { +public: + CommandParserPtr createCommandParserFromProto(const Protobuf::Message&) override; + std::string configType() override; + ProtobufTypes::MessagePtr createEmptyConfigProto() override; + std::string name() const override; +}; + } // namespace Formatter } // namespace Envoy diff --git a/test/common/formatter/substitution_format_string_test.cc b/test/common/formatter/substitution_format_string_test.cc index 605bf6ee33024..7591fede2c0c4 100644 --- a/test/common/formatter/substitution_format_string_test.cc +++ b/test/common/formatter/substitution_format_string_test.cc @@ -119,6 +119,26 @@ TEST_F(SubstitutionFormatStringUtilsTest, TestFromProtoConfigFormatterExtension) response_trailers_, stream_info_, body_)); } +TEST_F(SubstitutionFormatStringUtilsTest, + TestFromProtoConfigFormatterExtensionFailsToCreateParser) { + FailCommandFactory fail_factory; + Registry::InjectFactory command_register(fail_factory); + + const std::string yaml = R"EOF( + text_format_source: + inline_string: "plain text" + formatters: + - name: envoy.formatter.TestFormatter + typed_config: + "@type": type.googleapis.com/google.protobuf.UInt64Value +)EOF"; + TestUtility::loadFromYaml(yaml, config_); + + EXPECT_THROW_WITH_MESSAGE(SubstitutionFormatStringUtils::fromProtoConfig(config_, context_.api()), + EnvoyException, + "Failed to create command parser: envoy.formatter.TestFormatter"); +} + TEST_F(SubstitutionFormatStringUtilsTest, TestFromProtoConfigFormatterExtensionUnknown) { const std::string yaml = R"EOF( text_format_source: From 7527a34b174470bf962fcd98de16ff80dd4223ad Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Thu, 25 Mar 2021 16:29:29 -0400 Subject: [PATCH 09/12] Drop changelog entry since the previous one still holds Signed-off-by: Raul Gutierrez Segales --- docs/root/version_history/current.rst | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index e8011da04ffe6..ab2542a5bbbff 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -125,7 +125,6 @@ New Features * access log: added the new response flag `NC` for upstream cluster not found. The error flag is set when the http or tcp route is found for the request but the cluster is not available. * access log: added the :ref:`formatters ` extension point for custom formatters (command operators). * access log: added support for cross platform writing to :ref:`standard output ` and :ref:`standard error `. -* access log: added support for json format in the :ref:`formatters ` extension point. * access log: support command operator: %FILTER_CHAIN_NAME% for the downstream tcp and http request. * access log: support command operator: %REQUEST_HEADERS_BYTES%, %RESPONSE_HEADERS_BYTES%, and %RESPONSE_TRAILERS_BYTES%. * compression: add brotli :ref:`compressor ` and :ref:`decompressor `. From c3da62e97294abb9468b2a33530cb93475205123 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Thu, 25 Mar 2021 16:34:59 -0400 Subject: [PATCH 10/12] Fix name Signed-off-by: Raul Gutierrez Segales --- test/common/formatter/substitution_format_string_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/common/formatter/substitution_format_string_test.cc b/test/common/formatter/substitution_format_string_test.cc index 7591fede2c0c4..31924dbd19f44 100644 --- a/test/common/formatter/substitution_format_string_test.cc +++ b/test/common/formatter/substitution_format_string_test.cc @@ -128,7 +128,7 @@ TEST_F(SubstitutionFormatStringUtilsTest, text_format_source: inline_string: "plain text" formatters: - - name: envoy.formatter.TestFormatter + - name: envoy.formatter.FailFormatter typed_config: "@type": type.googleapis.com/google.protobuf.UInt64Value )EOF"; @@ -136,7 +136,7 @@ TEST_F(SubstitutionFormatStringUtilsTest, EXPECT_THROW_WITH_MESSAGE(SubstitutionFormatStringUtils::fromProtoConfig(config_, context_.api()), EnvoyException, - "Failed to create command parser: envoy.formatter.TestFormatter"); + "Failed to create command parser: envoy.formatter.FailFormatter"); } TEST_F(SubstitutionFormatStringUtilsTest, TestFromProtoConfigFormatterExtensionUnknown) { From ba89dc8263509c0cdb4e8aef3d8f1ee381a69fa9 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Mon, 29 Mar 2021 16:54:02 -0400 Subject: [PATCH 11/12] Use a FormatBuilder helper class This avoids the dangers around a dangling reference. Signed-off-by: Raul Gutierrez Segales --- .../formatter/substitution_formatter.cc | 17 ++++++---------- .../common/formatter/substitution_formatter.h | 20 ++++++++++++------- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc index 8ca53527c4548..e591024f553d7 100644 --- a/source/common/formatter/substitution_formatter.cc +++ b/source/common/formatter/substitution_formatter.cc @@ -153,21 +153,16 @@ StructFormatter::StructFormatter(const ProtobufWkt::Struct& format_mapping, bool const std::vector& commands) : omit_empty_values_(omit_empty_values), preserve_types_(preserve_types), empty_value_(omit_empty_values_ ? EMPTY_STRING : DefaultUnspecifiedValueString), - commands_(commands), struct_output_format_(toFormatMapValue(format_mapping)) { - // Note: to avoid copying commands we save the ref in an optional for toFormatMapValue and related - // methods to use. Once those are done we need to unset the optional to avoid accidentally - // accessing a dangling reference. - commands_ = absl::nullopt; -} + struct_output_format_(FormatBuilder(commands).toFormatMapValue(format_mapping)) {} StructFormatter::StructFormatter(const ProtobufWkt::Struct& format_mapping, bool preserve_types, bool omit_empty_values) : omit_empty_values_(omit_empty_values), preserve_types_(preserve_types), empty_value_(omit_empty_values_ ? EMPTY_STRING : DefaultUnspecifiedValueString), - commands_(absl::nullopt), struct_output_format_(toFormatMapValue(format_mapping)) {} + struct_output_format_(FormatBuilder().toFormatMapValue(format_mapping)) {} StructFormatter::StructFormatMapWrapper -StructFormatter::toFormatMapValue(const ProtobufWkt::Struct& struct_format) const { +StructFormatter::FormatBuilder::toFormatMapValue(const ProtobufWkt::Struct& struct_format) const { auto output = std::make_unique(); for (const auto& pair : struct_format.fields()) { switch (pair.second.kind_case()) { @@ -191,8 +186,8 @@ StructFormatter::toFormatMapValue(const ProtobufWkt::Struct& struct_format) cons return {std::move(output)}; } -StructFormatter::StructFormatListWrapper -StructFormatter::toFormatListValue(const ProtobufWkt::ListValue& list_value_format) const { +StructFormatter::StructFormatListWrapper StructFormatter::FormatBuilder::toFormatListValue( + const ProtobufWkt::ListValue& list_value_format) const { auto output = std::make_unique(); for (const auto& value : list_value_format.values()) { switch (value.kind_case()) { @@ -216,7 +211,7 @@ StructFormatter::toFormatListValue(const ProtobufWkt::ListValue& list_value_form } std::vector -StructFormatter::toFormatStringValue(const std::string& string_format) const { +StructFormatter::FormatBuilder::toFormatStringValue(const std::string& string_format) const { std::vector commands; return SubstitutionFormatParser::parse(string_format, commands_.value_or(commands)); } diff --git a/source/common/formatter/substitution_formatter.h b/source/common/formatter/substitution_formatter.h index 515311f69a073..20daddb7e43c5 100644 --- a/source/common/formatter/substitution_formatter.h +++ b/source/common/formatter/substitution_formatter.h @@ -170,9 +170,19 @@ class StructFormatter { const std::function>; // Methods for building the format map. - std::vector toFormatStringValue(const std::string& string_format) const; - StructFormatMapWrapper toFormatMapValue(const ProtobufWkt::Struct& struct_format) const; - StructFormatListWrapper toFormatListValue(const ProtobufWkt::ListValue& list_value_format) const; + class FormatBuilder { + public: + FormatBuilder(const std::vector& commands) : commands_(commands) {} + FormatBuilder() : commands_(absl::nullopt) {} + std::vector toFormatStringValue(const std::string& string_format) const; + StructFormatMapWrapper toFormatMapValue(const ProtobufWkt::Struct& struct_format) const; + StructFormatListWrapper + toFormatListValue(const ProtobufWkt::ListValue& list_value_format) const; + + private: + using CommandsRef = std::reference_wrapper>; + const absl::optional commands_; + }; // Methods for doing the actual formatting. ProtobufWkt::Value providersCallback(const std::vector& providers, @@ -192,10 +202,6 @@ class StructFormatter { const bool preserve_types_; const std::string empty_value_; - // Note: this ref becomes invalid outside of the constructor, so we unset it after that. - using CommandsRef = std::reference_wrapper>; - absl::optional commands_; - const StructFormatMapWrapper struct_output_format_; }; From d40c4437cde90e166ec27b45e9764f0dab5183ac Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Tue, 30 Mar 2021 10:59:32 -0400 Subject: [PATCH 12/12] explicit Signed-off-by: Raul Gutierrez Segales --- 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 20daddb7e43c5..32d33eab4d3b5 100644 --- a/source/common/formatter/substitution_formatter.h +++ b/source/common/formatter/substitution_formatter.h @@ -172,8 +172,8 @@ class StructFormatter { // Methods for building the format map. class FormatBuilder { public: - FormatBuilder(const std::vector& commands) : commands_(commands) {} - FormatBuilder() : commands_(absl::nullopt) {} + explicit FormatBuilder(const std::vector& commands) : commands_(commands) {} + explicit FormatBuilder() : commands_(absl::nullopt) {} std::vector toFormatStringValue(const std::string& string_format) const; StructFormatMapWrapper toFormatMapValue(const ProtobufWkt::Struct& struct_format) const; StructFormatListWrapper