access log: support formatter extensions from json_format (#15646)

This was missed in #14512.

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

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<JsonFormatterImpl>(struct_format, preserve_types, omit_empty_values);
+  std::vector<CommandParserPtr> commands;
+  return std::make_unique<JsonFormatterImpl>(struct_format, preserve_types, omit_empty_values,
+                                             commands);
 }
 
 FormatterPtr SubstitutionFormatStringUtils::fromProtoConfig(
@@ -25,22 +27,26 @@ 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));
   }
 
   switch (config.format_case()) {
   case envoy::config::core::v3::SubstitutionFormatString::FormatCase::kTextFormat:
     return std::make_unique<FormatterImpl>(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<JsonFormatterImpl>(config.json_format(), true,
+                                               config.omit_empty_values(), commands);
   case envoy::config::core::v3::SubstitutionFormatString::FormatCase::kTextFormatSource:
     return std::make_unique<FormatterImpl>(
         Config::DataSource::read(config.text_format_source(), true, api), false, commands);
-  }
   default:
     NOT_REACHED_GCOVR_EXCL_LINE;
   }
+
   return nullptr;
 }
 

source/common/formatter/substitution_formatter.cc

@@ -149,14 +149,21 @@ std::string JsonFormatterImpl::format(const Http::RequestHeaderMap& request_head
   return absl::StrCat(log_line, "\n");
 }
 
+StructFormatter::StructFormatter(const ProtobufWkt::Struct& format_mapping, bool preserve_types,
+                                 bool omit_empty_values,
+                                 const std::vector<CommandParserPtr>& commands)
+    : omit_empty_values_(omit_empty_values), preserve_types_(preserve_types),
+      empty_value_(omit_empty_values_ ? EMPTY_STRING : DefaultUnspecifiedValueString),
+      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),
-      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<StructFormatMap>();
   for (const auto& pair : struct_format.fields()) {
     switch (pair.second.kind_case()) {
@@ -178,10 +185,10 @@ 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<StructFormatList>();
   for (const auto& value : list_value_format.values()) {
     switch (value.kind_case()) {
@@ -205,9 +212,10 @@ StructFormatter::toFormatListValue(const ProtobufWkt::ListValue& list_value_form
 }
 
 std::vector<FormatterProviderPtr>
-StructFormatter::toFormatStringValue(const std::string& string_format) const {
-  return SubstitutionFormatParser::parse(string_format);
-};
+StructFormatter::FormatBuilder::toFormatStringValue(const std::string& string_format) const {
+  std::vector<CommandParserPtr> commands;
+  return SubstitutionFormatParser::parse(string_format, commands_.value_or(commands));
+}
 
 ProtobufWkt::Value StructFormatter::providersCallback(
     const std::vector<FormatterProviderPtr>& providers,

source/common/formatter/substitution_formatter.h

@@ -135,6 +135,8 @@ 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<CommandParserPtr>& commands);
 
   ProtobufWkt::Struct format(const Http::RequestHeaderMap& request_headers,
                              const Http::ResponseHeaderMap& response_headers,
@@ -168,9 +170,19 @@ class StructFormatter {
       const std::function<ProtobufWkt::Value(const StructFormatter::StructFormatListWrapper&)>>;
 
   // Methods for building the format map.
-  std::vector<FormatterProviderPtr> 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:
+    explicit FormatBuilder(const std::vector<CommandParserPtr>& commands) : commands_(commands) {}
+    explicit FormatBuilder() : commands_(absl::nullopt) {}
+    std::vector<FormatterProviderPtr> 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 std::vector<CommandParserPtr>>;
+    const absl::optional<CommandsRef> commands_;
+  };
 
   // Methods for doing the actual formatting.
   ProtobufWkt::Value providersCallback(const std::vector<FormatterProviderPtr>& providers,
@@ -189,8 +201,9 @@ class StructFormatter {
   const bool omit_empty_values_;
   const bool preserve_types_;
   const std::string empty_value_;
+
   const StructFormatMapWrapper struct_output_format_;
-}; // namespace Formatter
+};
 
 using StructFormatterPtr = std::unique_ptr<StructFormatter>;
 
@@ -199,6 +212,9 @@ class JsonFormatterImpl : public Formatter {
   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<CommandParserPtr>& commands)
+      : struct_formatter_(format_mapping, preserve_types, omit_empty_values, commands) {}
 
   // Formatter::format
   std::string format(const Http::RequestHeaderMap& request_headers,

test/common/formatter/command_extension.cc

@@ -41,5 +41,54 @@ ProtobufTypes::MessagePtr TestCommandFactory::createEmptyConfigProto() {
 
 std::string TestCommandFactory::name() const { return "envoy.formatter.TestFormatter"; }
 
+absl::optional<std::string> 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<AdditionalFormatter>();
+  }
+
+  return nullptr;
+}
+
+CommandParserPtr AdditionalCommandFactory::createCommandParserFromProto(const Protobuf::Message&) {
+  return std::make_unique<AdditionalCommandParser>();
+}
+
+std::string AdditionalCommandFactory::configType() { return "google.protobuf.UInt32Value"; }
+
+ProtobufTypes::MessagePtr AdditionalCommandFactory::createEmptyConfigProto() {
+  return std::make_unique<ProtobufWkt::UInt32Value>();
+}
+
+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<ProtobufWkt::UInt64Value>();
+}
+
+std::string FailCommandFactory::name() const { return "envoy.formatter.FailFormatter"; }
+
 } // namespace Formatter
 } // namespace Envoy

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;
 };
 
@@ -35,5 +34,37 @@ class TestCommandFactory : public CommandParserFactory {
   std::string name() const override;
 };
 
+class AdditionalFormatter : public FormatterProvider {
+public:
+  // FormatterProvider
+  absl::optional<std::string> format(const Http::RequestHeaderMap&, const Http::ResponseHeaderMap&,
+                                     const Http::ResponseTrailerMap&, const StreamInfo::StreamInfo&,
+                                     absl::string_view) 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:
+  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;
+};
+
+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

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<CommandParserFactory> command_register(fail_factory);
+
+  const std::string yaml = R"EOF(
+  text_format_source:
+    inline_string: "plain text"
+  formatters:
+    - name: envoy.formatter.FailFormatter
+      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.FailFormatter");
+}
+
 TEST_F(SubstitutionFormatStringUtilsTest, TestFromProtoConfigFormatterExtensionUnknown) {
   const std::string yaml = R"EOF(
   text_format_source:
@@ -135,5 +155,71 @@ TEST_F(SubstitutionFormatStringUtilsTest, TestFromProtoConfigFormatterExtensionU
                             "Formatter not found: envoy.formatter.TestFormatterUnknown");
 }
 
+TEST_F(SubstitutionFormatStringUtilsTest, TestFromProtoConfigJsonWithExtension) {
+  TestCommandFactory factory;
+  Registry::InjectFactory<CommandParserFactory> command_register(factory);
+
+  const std::string yaml = R"EOF(
+  json_format:
+    text: "plain text %COMMAND_EXTENSION()%"
+    path: "%REQ(:path)% %COMMAND_EXTENSION()%"
+    code: "%RESPONSE_CODE% %COMMAND_EXTENSION()%"
+    headers:
+      content-type: "%REQ(CONTENT-TYPE)% %COMMAND_EXTENSION()%"
+  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 TestFormatter",
+    "code": "200 TestFormatter",
+    "headers": {
+      "content-type": "application/json TestFormatter"
+    }
+})EOF";
+
+  EXPECT_TRUE(TestUtility::jsonStringEqual(out_json, expected));
+}
+
+TEST_F(SubstitutionFormatStringUtilsTest, TestFromProtoConfigJsonWithMultipleExtensions) {
+  TestCommandFactory test_factory;
+  Registry::InjectFactory<CommandParserFactory> test_command_register(test_factory);
+  AdditionalCommandFactory additional_factory;
+  Registry::InjectFactory<CommandParserFactory> 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));
+}
+
 } // namespace Formatter
 } // namespace Envoy