-
Notifications
You must be signed in to change notification settings - Fork 5.5k
local_reply_config : support content-type in SubstitutionFormatString #13019
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
bfc45e4
84f641f
8d5f5fd
6207d6c
4a1fbe0
9ab7e4b
020eb52
48bd0ba
88e7856
0c9713b
db5486e
9dfaa86
95cc592
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,4 +65,12 @@ message SubstitutionFormatString { | |
| // empty string, so that empty values are omitted entirely. | ||
| // * for ``json_format`` the keys with null values are omitted in the output structure. | ||
| bool omit_empty_values = 3; | ||
|
|
||
| // Specify a content_type for text_format. This will be ignored for json_format | ||
| // | ||
| // .. code-block:: | ||
| // | ||
| // content_type: "text/html; charset=UTF-8" | ||
| // | ||
| string content_type = 4 [(validate.rules).string = {min_bytes: 1}]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need this validation since it is only valid for text_format?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also specify currently supported content type values are |
||
| } | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,6 +64,7 @@ New Features | |
| * access log: added support for :ref:`%DOWNSTREAM_PEER_FINGERPRINT_1% <config_access_log_format_response_flags>` as a response flag. | ||
| * access log: added support for nested objects in :ref:`JSON logging mode <config_access_log_format_dictionaries>`. | ||
| * access log: added :ref:`omit_empty_values<envoy_v3_api_field_config.core.v3.SubstitutionFormatString.omit_empty_values>` option to omit unset value from formatted log. | ||
| * access log: added :ref:`content_type<envoy_v3_api_field_config.core.v3.SubstitutionFormatString.content_type>` option to set content-type for textformat. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is applicable for local reply (not so for access log) - so may be we should release note it under local_reply config? |
||
| * build: enable building envoy :ref:`arm64 images <arm_binaries>` by buildx tool in x86 CI platform. | ||
| * cluster: added new :ref:`connection_pool_per_downstream_connection <envoy_v3_api_field_config.cluster.v3.Cluster.connection_pool_per_downstream_connection>` flag, which enable creation of a new connection pool for each downstream connection. | ||
| * decompressor filter: reports compressed and uncompressed bytes in trailers. | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| #include "common/formatter/substitution_format_string.h" | ||
|
|
||
| #include "common/formatter/substitution_formatter.h" | ||
| #include "common/http/headers.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Formatter { | ||
|
|
@@ -25,5 +26,18 @@ FormatterPtr SubstitutionFormatStringUtils::fromProtoConfig( | |
| return nullptr; | ||
| } | ||
|
|
||
| absl::string_view SubstitutionFormatStringUtils::getContentType( | ||
| const envoy::config::core::v3::SubstitutionFormatString& config) { | ||
| const std::string contentType = config.content_type(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Simplify this as follows as those are the only ones supported now ?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why can't we plumb through the content type directly, without having to rely on well-known values? |
||
| if (contentType == Http::Headers::get().ContentTypeValues.TextEventStream) | ||
| return Http::Headers::get().ContentTypeValues.TextEventStream; | ||
| else if (contentType == Http::Headers::get().ContentTypeValues.TextUtf8) | ||
| return Http::Headers::get().ContentTypeValues.TextUtf8; | ||
| else if (contentType == Http::Headers::get().ContentTypeValues.Html) | ||
| return Http::Headers::get().ContentTypeValues.Html; | ||
| else | ||
| return Http::Headers::get().ContentTypeValues.Text; | ||
| } | ||
|
|
||
| } // namespace Formatter | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,7 @@ class BodyFormatter { | |
| config.format_case() == | ||
| envoy::config::core::v3::SubstitutionFormatString::FormatCase::kJsonFormat | ||
| ? Http::Headers::get().ContentTypeValues.Json | ||
| : Http::Headers::get().ContentTypeValues.Text) {} | ||
| : Formatter::SubstitutionFormatStringUtils::getContentType(config)) {} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, given: https://github.com/envoyproxy/envoy/pull/13019/files#r488273786, here we can have: |
||
|
|
||
| void format(const Http::RequestHeaderMap& request_headers, | ||
| const Http::ResponseHeaderMap& response_headers, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: end with
.