Skip to content
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

feature regression: no longer possible to rewrite authority #16775

Closed
howardjohn opened this issue Jun 2, 2021 · 8 comments · Fixed by #17083
Closed

feature regression: no longer possible to rewrite authority #16775

howardjohn opened this issue Jun 2, 2021 · 8 comments · Fixed by #17083
Assignees
Labels
enhancement Feature requests. Not bugs or questions. investigate Potential bug that needs verification

Comments

@howardjohn
Copy link
Contributor

Title: no longer possible to rewrite authority

Description:
#14747 changed the behavior of header modifications that are allowed. Previously, we relied on this to rewrite the authority on a per-destination basis.

After the change, this is no longer possible. While we can do a rewrite at the route level, we cannot do it on a per-weighted-cluster basis, unlike all other headers.

For us this represents a breaking change, so we will likely set the runtime flag for now. But we want to understand a long term plan.

cc @alyssawilk

[optional Relevant Links:]
Istio specific issue: istio/istio#33226

@howardjohn howardjohn added enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Jun 2, 2021
@alyssawilk
Copy link
Contributor

Yeah, this was a bit of a bug, because it allowed rewrites specific for HTTP/1 and not HTTP/2 so would cause inconsistent behavior.

Would any of
"host_rewrite_literal": "...",
"auto_host_rewrite": "{...}",
"host_rewrite_header": "...",
"host_rewrite_path_regex": "{...}",

work for you?
https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#envoy-v3-api-field-config-route-v3-routeaction-regex-rewrite

If not happy to discuss the use case further.

@alyssawilk alyssawilk added investigate Potential bug that needs verification and removed triage Issue requires triage labels Jun 2, 2021
@howardjohn
Copy link
Contributor Author

We do use host_rewrite_literal, but the issue is it cannot be set inside a WeightedCluster_ClusterWeight. So we can set it at the RouteAction level but if we have multiple destinations we can only use one rewrite between all of them

@alyssawilk
Copy link
Contributor

ah, gotcha. So if host_rewrite_specifier were added to ClusterWeight that'd work for you?
cc @envoyproxy/api-shepherds too since I'd like their input as well.

Also you did catch this validation is runtime guarded, yes? Ideally you should just be able to flip the runtime guard back while we figure out a long term solution.

@alyssawilk alyssawilk self-assigned this Jun 2, 2021
@howardjohn
Copy link
Contributor Author

Yep, I was planning to set the runtime flag in the meantime. Thanks for adding that

ah, gotcha. So if host_rewrite_specifier were added to ClusterWeight that'd work for you?

Yep

@alyssawilk
Copy link
Contributor

@adisuissa @lizan thoughts?

@alyssawilk
Copy link
Contributor

alyssawilk commented Jun 8, 2021

@htuch can you comment on this?
We allow header tweaks in many places - if we're game for adding host rewrite to more places I can go do that.

@htuch
Copy link
Member

htuch commented Jun 9, 2021

I guess this is reasonable to ask for behavior wise. What I'm missing a bit is what are the set of hierarchically overridable things we want to make available more generally. We already have request/response headers to add, now we add host rewrite, what does the full picture here look like?

We don't need to boil the ocean, but at the same time, building an understandable/maintainable/consistent API should have some notion of what makes sense to have as hierarchically overridable. I have this feeling that if we had rewritten the API from scratch with these requirements, it would have a dedicated message that could be composed across levels.

htuch pushed a commit that referenced this issue Jun 24, 2021
Risk Level: low (config guarded addition)
Testing: new unit tests
Docs Changes: in API docs
Release Notes: inline
Fixes #16775

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
Copy link
Contributor

@howardjohn should be fixed - lmk if this doesn't work for all your use cases and I'll keep iterating :-)

chrisxrepo pushed a commit to chrisxrepo/envoy that referenced this issue Jul 8, 2021
Risk Level: low (config guarded addition)
Testing: new unit tests
Docs Changes: in API docs
Release Notes: inline
Fixes envoyproxy#16775

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: chris.xin <xinchuantao@qq.com>
leyao-daily pushed a commit to leyao-daily/envoy that referenced this issue Sep 30, 2021
Risk Level: low (config guarded addition)
Testing: new unit tests
Docs Changes: in API docs
Release Notes: inline
Fixes envoyproxy#16775

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests. Not bugs or questions. investigate Potential bug that needs verification
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants