envoyproxy / envoyproxy/envoy

Add support for json_format_source to Access Logs

Open
#24,710 3 comments 1 reaction 1 assignee Claimed by @pims View on GitHub
area/access_log help wanted
Dominant language
C++
Stars
28.9k
Forks
5.6k
Avg merge
1d 22h
Merged PRs (30d)
430

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:

```json
"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
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(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(struct_format, true, config.omit_empty_values(), commands);
+ }
return std::make_unique(
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:
```yaml
- 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*:

* https://www.envoyproxy.io/docs/envoy/v1.23.1/api-v3/config/core/v3/substitution_format_string.proto#envoy-v3-api-msg-config-core-v3-substitutionformatstring
* https://github.com/envoyproxy/envoy/blob/v1.23.1/api/envoy/config/core/v3/substitution_format_string.proto#L90

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.