router: unified header formatters#21932
Merged
mattklein123 merged 77 commits intoenvoyproxy:mainfrom Sep 22, 2022
Merged
Conversation
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Fixed all unit tests. Added translators for backwards compatibility. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
…_utils.cc. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Contributor
Author
|
/retest |
|
Retrying Azure Pipelines: |
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Contributor
Author
|
/retest |
|
Retrying Azure Pipelines: |
Member
|
Thanks. Great. |
Member
|
cc @mattklein123 I think this is ready for a final review. Some background: according to previous discussion, this PR will not change the method signature as far as possible to reduce impact to exist code tree (except some unavoidable updates). We will have a dedicated PR to unify all these interfaces after the runtime guard is expired. |
wbpcode
reviewed
Sep 20, 2022
Comment on lines
-579
to
-587
| %DOWNSTREAM_REMOTE_ADDRESS% | ||
| Remote address of the downstream connection. If the address is an IP address it includes both | ||
| address and port. | ||
|
|
||
| .. note:: | ||
|
|
||
| This may not be the physical remote address of the peer if the address has been inferred from | ||
| :ref:`Proxy Protocol filter <config_listener_filters_proxy_protocol>` or :ref:`x-forwarded-for | ||
| <config_http_conn_man_headers_x-forwarded-for>`. |
Member
There was a problem hiding this comment.
nit: should we just keep these old docs for now and removed after the runtime guard is expired? 🤔
Member
|
/assign @mattklein123 |
mattklein123
requested changes
Sep 21, 2022
Member
mattklein123
left a comment
There was a problem hiding this comment.
Sweet! A few small comments, thank you.
/wait
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
jmarantz
reviewed
Oct 5, 2022
wbpcode
added a commit
that referenced
this pull request
Sep 24, 2025
Commit Message: formatter: removed legacy router formatter support Additional Description: Three yeas ago, at #21932, we unified all the formatters to the substitution formatter. And we add a warn log for the legacy UPSTREAM_METADATA and DYNAMIC_METADATA. Now, I think it's time to remove it finally. This PR Removed legacy header formatter support for `%DYNAMIC_METADATA(["namespace", "key", ...])%` and `%UPSTREAM_METADATA(["namespace", "key", ...])%`. Please use `%DYNAMIC_METADATA(namespace:key:...])%` and `%UPSTREAM_METADATA(namespace:key:...])%` as alternatives. This change can be reverted temporarily by setting the runtime guard `envoy.reloadable_features.remove_legacy_route_formatter` to `false`. Risk Level: low. Testing: unit. Docs Changes: n/a. Release Notes: added. --------- Signed-off-by: WangBaiping <wbphub@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Commit Message:
unified formatters for access log and header
Additional Description:
At the core of this PR are changes to signature of just two methods:
HeaderFormatter::evaluateHeader- it used to take only stream_info as param. Now it also takes RequestHeaderMap and ResponseHeaderMap. This is required to directly map parameters to access log substitution formatters.Router::ResponseEntry::finalizeResponseHeaders- added RequestHeaderMap to the parameters.Remaining changes are around changes to those two methods.
Header formatters used to have internal parser and its own set of formatters. Now parsing is done by access log's parser and header formatting is done via access log's substitution formatters.
Runtime guard
envoy_reloadable_features_unified_header_formatterwas added to allow using the previous implementation of parser and formatters.New changes are fully backward compatible. However, header formatters for DYNAMIC_METADATA, UPSTREAM_METADATA and PER_REQUEST_STATE had a different format than access log's equivalent. Temporary translators have been added for those 3 formatters and will be removed when old format is deprecated.
All substitution formatters are allowed to be used. Some usage does not make sense though. For example, it is possible to modify request headers using a formatter which needs response headers, like ,%RESP(X-SERVED-BY)%. At the moment of modifying request headers, the response headers are not present yet and formatter produces empty string. It is left up for the administrator to select only formatters which make sense to use. This behaviour could be modified by adding attribute flags to each formatter indicating where formatters can be used (for example only in request headers, only in response headers or both), but it would come at some code complexity.
Risk Level: Med
Testing: Added unit tests. Manual testing with runtime guard on and off.
Docs Changes: Yes.
Release Notes: Yes
Platform Specific Features:
Runtime guard: envoy.reloadable_features.unified_header_formatter
Fixes #20389
Fixes #15036