envoyproxy/envoy

Add support for json_format_source to Access Logs

Open

#24,710 opened on Dec 29, 2022

View on GitHub
 (3 comments) (1 reaction) (1 assignee)C++ (5,373 forks)batch import
area/access_loghelp wanted

Repository metrics

Stars
 (27,997 stars)
PR merge metrics
 (Avg merge 8d) (378 merged PRs in 30d)

Description

Title: Add support for json_format_source to Access Logs

Description:

When running Envoy in a multi-tenant environment like Kubernetes, it is common to create one instance of the envoy.filters.network.http_connection_manager filter per Ingress resource. With this approach, most settings need to be configured for each Ingress resource. In large deployments, with thousands of Ingress resources, Envoy's configuration can get quite large. A big contributor to the configuration size is configuring an identical Access Logs format for each resource. Example:

"typed_json_format": {
  "response_code_details": "%RESPONSE_CODE_DETAILS%",
  "upstream_request_attempt_count": "%UPSTREAM_REQUEST_ATTEMPT_COUNT%",
  "downstream_remote_address": "%DOWNSTREAM_REMOTE_ADDRESS%",
  "rbac": "%DYNAMIC_METADATA(envoy.filters.http.rbac)%",
  "server": "%RESP(SERVER)%",
  "user_agent": "%REQ(USER-AGENT)%",
  "x_forwarded_for": "%REQ(X-FORWARDED-FOR)%",
  "downstream_peer_cert_valid_until": "%DOWNSTREAM_PEER_CERT_V_END%",
  "downstream_peer_cert_valid_start": "%DOWNSTREAM_PEER_CERT_V_START%",
  "request_duration": "%REQUEST_DURATION%",
  "downstream_tls_version": "%DOWNSTREAM_TLS_VERSION%",
  "downstream_peer_serial": "%DOWNSTREAM_PEER_SERIAL%",
  "downstream_peer_issuer": "%DOWNSTREAM_PEER_ISSUER%",
  "method": "%REQ(:METHOD)%",
  "response_flags": "%RESPONSE_FLAGS%",
  // ... dozens more fields here
}

Multiply this by thousands of envoy.filters.network.http_connection_manager independent filter configurations and the configuration (via http://admin/config_dump) size balloons very quickly.

The work done in #14276, added support for text_format_source, to be able to configure the text format via a datasource. The motivation for the work by @esmet was:

This change is motivated by https://github.com/envoyproxy/envoy/pull/14221 where we use a SubstitutionFormatString as a way to define custom HTTP response body rewrites.

Possibly incidentally, it opened the possibility to configure a custom text_format for access logs once, and to use the DataSource configuration as some sort of reference/pointer to a single "source", avoiding having to "copy" the custom format for each instance of the envoy.filters.network.http_connection_manager filter.

Unfortunately, as it stands, this is not usable when using json as the format for access logs, since if one attempts to put a JSON looking string in the text_format_source.inline_string, all JSON types but string are effectively lost.

Hence this Feature Request.

Exploring Options:

To validate the usefulness of such a feature, I created a small patch that uses the test_format_source + content_type to configure the JsonFormatter.

diff --git a/source/common/formatter/BUILD b/source/common/formatter/BUILD
index 202ad9d030..e4f7a196b8 100644
--- a/source/common/formatter/BUILD
+++ b/source/common/formatter/BUILD
@@ -25,6 +25,8 @@ envoy_cc_library(
         "//source/common/config:metadata_lib",
         "//source/common/grpc:common_lib",
         "//source/common/http:utility_lib",
+        "//source/common/protobuf:utility_lib",
+        "//source/common/http:headers_lib",
         "//source/common/protobuf:message_validator_lib",
         "//source/common/runtime:runtime_features_lib",
         "//source/common/stream_info:utility_lib",
diff --git a/source/common/formatter/substitution_format_string.cc b/source/common/formatter/substitution_format_string.cc
index 9eede2c4dd..e87b28cdda 100644
--- a/source/common/formatter/substitution_format_string.cc
+++ b/source/common/formatter/substitution_format_string.cc
@@ -4,8 +4,10 @@
 
 #include "source/common/config/datasource.h"
 #include "source/common/config/utility.h"
+#include "source/common/protobuf/utility.h"
 #include "source/common/formatter/substitution_formatter.h"
 #include "source/common/protobuf/message_validator_impl.h"
+#include "source/common/http/headers.h"
 
 namespace Envoy {
 namespace Formatter {
@@ -44,10 +46,17 @@ FormatterPtr SubstitutionFormatStringUtils::fromProtoConfig(
   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:
+  case envoy::config::core::v3::SubstitutionFormatString::FormatCase::kTextFormatSource: {
+    if (!config.content_type().empty() && config.content_type() == Http::Headers::get().ContentTypeValues.Json) {
+        std::string foo = Config::DataSource::read(config.text_format_source(), true, context.api());
+        ProtobufWkt::Struct struct_format;
+        MessageUtil::loadFromJson(foo, struct_format);
+        return std::make_unique<JsonFormatterImpl>(struct_format, true, config.omit_empty_values(), commands);
+    }
     return std::make_unique<FormatterImpl>(
         Config::DataSource::read(config.text_format_source(), true, context.api()), false,
         commands);
+  }
   case envoy::config::core::v3::SubstitutionFormatString::FormatCase::FORMAT_NOT_SET:
     PANIC_DUE_TO_PROTO_UNSET;
   }

which, with the following configuration:

      - name: envoy.filters.network.http_connection_manager
        typed_config:
          "@type": type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
          codec_type: AUTO
          stat_prefix: ingress_http
          access_log:
          - name: envoy.access_loggers.file
            typed_config:
              "@type": type.googleapis.com/envoy.extensions.access_loggers.file.v3.FileAccessLog
              path: "/dev/stdout"
              log_format:
                text_format_source:
                  filename: /path/to/access-logs-format.json
                content_type: application/json

yields the desired outcome: typed json access logs format configured centrally (in a file in this example).

It seems that introducing a json_format_source modeled after the text_format_source feels like a more sustainable approach than parsing the text_format_source differently based on the value of content_type, but changing the API is not required to produce the desired outcome.

Questions for Envoy maintainers

  1. Does this feel like a reasonably useful feature? (I can't find the Github issue where someone who works on Istio asked for something similar).
  2. Any sense of the impact of having to "open" the same datasource thousands of times? Not an issue for inline_string, inline_bytes or environment_variable, but possibly problematic for a Local filesystem data source.
  3. Would it be better to add this a static resource loaded once during bootstrap and referenced by the Access Logs filters?

Assuming this Feature Request is reasonable, I'd be happy to work on implementing whichever option is chosen.

Relevant Links:

Contributor guide