Skip to content

add x-envoy-original-host to keep original host #39617

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

Merged
merged 4 commits into from
May 29, 2025

Conversation

wbpcode
Copy link
Member

@wbpcode wbpcode commented May 24, 2025

Commit Message: add x-envoy-original-host to keep original host
Additional Description:

This PR added a x-envoy-original-host to keep original host value if the host rewriting happens.

Risk Level: low.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.

Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #39617 was opened by wbpcode.

see: more, trace.

@wbpcode
Copy link
Member Author

wbpcode commented May 24, 2025

This header is used to ensure that, when recreateStream happens, we can use the original host and path to re-match route and re-traverse the filter.

I am not sure if this could acceptable to community, so I created this PR first and get some response from the community.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine if it's only written when the host header is rewritten, but you should add to the docs where we explain all the other headers that are set as well as the release notes.

/wait

@wbpcode
Copy link
Member Author

wbpcode commented May 27, 2025

This seems fine if it's only written when the host header is rewritten, but you should add to the docs where we explain all the other headers that are set as well as the release notes.

/wait

Got it, I will add related docs and release note, thanks!

@wbpcode wbpcode force-pushed the dev-clean-up-router-2 branch from 9f78d7c to 29ce04f Compare May 27, 2025 16:34
Signed-off-by: wangbaiping(wbpcode) <[email protected]>
@wbpcode wbpcode force-pushed the dev-clean-up-router-2 branch from 29ce04f to f8662db Compare May 28, 2025 03:51
Comment on lines +598 to +600
// Finalize the request headers before the host selection.
route_entry_->finalizeRequestHeaders(headers, callbacks_->streamInfo(),
!config_->suppress_envoy_headers_);
Copy link
Member Author

@wbpcode wbpcode May 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving finalizeRequestHeaders here could avoid to expose the getRequestHostValue method and could avoid possible multiple host calculating/string copy/heap allocations, etc.

Another side effect is we could use this to effect upstream lb selection now (like the override host lb could select a specified host based on headers values). Although this can't be guaranteed because we have no constraint on the order of headers mutations and other operations in the router filter.

wbpcode added 2 commits May 28, 2025 07:33
Signed-off-by: wangbaiping(wbpcode) <[email protected]>
Signed-off-by: wangbaiping(wbpcode) <[email protected]>
@wbpcode wbpcode requested a review from Copilot May 28, 2025 10:00
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces support for preserving the original host header via a new header (x‑envoy‑original‑host) before any host rewriting takes place. Key changes include updates to header finalization logic (in functions such as Utility::updateAuthority and RouteEntryImplBase::finalizeRequestHeaders), modifications to tests to assert the presence and correctness of the new header, and corresponding API and documentation updates.

Reviewed Changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/mocks/router/mocks.cc Removed the mocking of getRequestHostValue to align with the new changes.
test/common/router/router_test.cc Added tests to verify that the original host is correctly preserved.
test/common/router/config_impl_test.cc Updated tests to assert x‑envoy‑original‑host values under different scenarios.
test/common/http/utility_test.cc Modified test cases for updateAuthority; one test loop uses a premature return.
source/common/router/upstream_request.cc Updated the call to updateAuthority with the new flag for suppressing envoy headers.
source/common/router/router.cc Adjusted header finalization and attempt count handling to support the new header logic.
source/common/router/delegating_route_impl.{h,cc} Removed the deprecated getRequestHostValue function.
source/common/router/config_impl.{h,cc} Changed the signature and behavior of header finalization functions.
source/common/http/utility.{h,cc} Updated updateAuthority to set x‑envoy‑original‑host and handle forwarded host appropriately.
source/common/http/conn_manager_utility.cc Ensured removal of the original host header from downstream requests.
envoy/router/router.h & envoy/http/header_map.h Updated API definitions to reflect the removal of getRequestHostValue and addition of the new header.
docs/root/configuration/http/http_filters/router_filter.rst Updated documentation for the new x‑envoy‑original‑host configuration.
changelogs/current.yaml & api/envoy/config/route/v3/route_components.proto Documented the feature addition and updated API references accordingly.

@wbpcode
Copy link
Member Author

wbpcode commented May 28, 2025

/retest

@wbpcode wbpcode merged commit 4453ce1 into envoyproxy:main May 29, 2025
26 checks passed
@wbpcode wbpcode deleted the dev-clean-up-router-2 branch May 29, 2025 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants