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

How do I include internal redirect response headers in the subsequent request? #16777

Closed
majames opened this issue Jun 2, 2021 · 7 comments · Fixed by #30793
Closed

How do I include internal redirect response headers in the subsequent request? #16777

majames opened this issue Jun 2, 2021 · 7 comments · Fixed by #30793
Labels
question Questions that are neither investigations, bugs, nor enhancements stale stalebot believes this issue/PR has not been touched recently

Comments

@majames
Copy link

majames commented Jun 2, 2021

How do I include internal redirect response headers in the subsequent request?

Description

I posted this question on Stack Overflow, but I also see that GitHub issues can be used for questions so I'm asking it here as well 🙂

Is it currently possible to include a subset of the 302 redirect response headers in the subsequent request Envoy sends when the redirect is handled internally? If not, does anyone have a good suggestion for a workaround for doing this?

@majames majames added the triage Issue requires triage label Jun 2, 2021
@junr03 junr03 added question Questions that are neither investigations, bugs, nor enhancements and removed triage Issue requires triage labels Jun 3, 2021
@junr03
Copy link
Member

junr03 commented Jun 3, 2021

Per my reading of the redirect docs:

The altered request headers will then have a new route selected, be sent through a new filter chain, and then shipped upstream with all of the normal Envoy request sanitization taking place.

Filters would have access to modify the redirected request headers. However, as far as I can read the chain would not have access to the redirect response headers.

cc @alyssawilk do you have any suggestions here?

@alyssawilk
Copy link
Contributor

There's no way to do this with Envoy's redirect utils.
We could potentially add this as config and code to the Envoy code or you could handle the redirect in a custom filter.

@majames
Copy link
Author

majames commented Jun 7, 2021

@alyssawilk thanks for the response! It'd be great if Envoy supported adding the redirect response headers to the subsequent request!

you could handle the redirect in a custom filter

Do you mind adding a little bit more detail to this please? From my understanding you're suggesting:

  1. I remove the internal redirect handling configuration from my Envoy config
  2. And, instead, use a Lua response handler instead. The response handler would check if the response is a 302 and, if it is, initiate a response_handle:httpCall() to the targeted upstream which includes the headers of interest from the original response. The response from this httpCall() would then be passed through to the downstream service via the response_handle:headers() and response_handle:body():setBytes() APIs

@alyssawilk
Copy link
Contributor

ah, I was thinking more of a C++ filter which called recreateStream much as the router filter does today for internal redirects. sorry I'm not terribly familiar with Envoy's lua fitler so I can't speak to how well that option would work.

@majames
Copy link
Author

majames commented Jun 7, 2021

Gotcha. I've never heard of a C++ filter or recreateStream... do you have docs you could link me about these please? 🙏

@github-actions
Copy link

github-actions bot commented Jul 8, 2021

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 8, 2021
@github-actions
Copy link

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.

pugmajere added a commit to Snowflake-Labs/envoy that referenced this issue Nov 20, 2023
This adds a new (repeated) field in the internal redirect policy,
"response_headers_to_preserve".  When set, the headers named there
will be copied from the response that triggers an internal redirect
into the request that follows.

This allows some limited information passing through the internal
redirect system.

The current system is faithful to the idea that internal redirects are
purely a latency optimization, and should behave similarly to if the
redirect had been passed to the downstream user-agent. This does
violate that idea.

Other proxies, such as Nginx, have a much more flexible way of
handling internal redirects that allows a fair bit of information
passing like this. This should allow implementations to adopt Envoy
that are using this kind of information passing, with reduced needs to
rearchitect.

Fixes: envoyproxy#30441
Fixes: envoyproxy#16777

Signed-off-by: Ryan Anderson <ryan.anderson@snowflake.com>
Signed-off-by: Ryan Anderson <ryan@michonline.com>
alyssawilk pushed a commit that referenced this issue Dec 7, 2023
…30793)

* internal redirects: Support passing headers from response to request

This adds a new (repeated) field in the internal redirect policy,
"response_headers_to_preserve".  When set, the headers named there
will be copied from the response that triggers an internal redirect
into the request that follows.

This allows some limited information passing through the internal
redirect system.

The current system is faithful to the idea that internal redirects are
purely a latency optimization, and should behave similarly to if the
redirect had been passed to the downstream user-agent. This does
violate that idea.

Other proxies, such as Nginx, have a much more flexible way of
handling internal redirects that allows a fair bit of information
passing like this. This should allow implementations to adopt Envoy
that are using this kind of information passing, with reduced needs to
rearchitect.

Fixes: #30441
Fixes: #16777

Signed-off-by: Ryan Anderson <ryan.anderson@snowflake.com>
Signed-off-by: Ryan Anderson <ryan@michonline.com>

* Switching loops to references

Signed-off-by: Ryan Anderson <ryan@michonline.com>

* Clarify that downstream filters will not run

Signed-off-by: Ryan Anderson <ryan@michonline.com>

* Use a vector of LowerCaseStrings

Signed-off-by: Ryan Anderson <ryan@michonline.com>

* Format fixes

Signed-off-by: Ryan Anderson <ryan@michonline.com>

* Fully qualify 'downstream_'

Signed-off-by: Ryan Anderson <ryan@michonline.com>

* Rename from ..._to_preserve to ..._to_copy

Signed-off-by: Ryan Anderson <ryan@michonline.com>

* Reject configs that specify HTTP/2 style headers or Host

Signed-off-by: Ryan Anderson <ryan@michonline.com>

* Fight with clang-tidy by hand

Signed-off-by: Ryan Anderson <ryan@michonline.com>

* Fixup bad doc references

Signed-off-by: Ryan Anderson <ryan@michonline.com>

* punctuation

Signed-off-by: Ryan Anderson <ryan@michonline.com>

* More doc fixups

Signed-off-by: Ryan Anderson <ryan@michonline.com>

* Add a small comment about request_headers_to_copy_

Signed-off-by: Ryan Anderson <ryan@michonline.com>

* Rip out the complicated header copying/restore logic and replace

This removes the existing specialized save/restore logic in favor of
just copying every header into another map, updating the original map
with the necessary changes, and then restoring the whole thing later on.

Signed-off-by: Ryan Anderson <ryan@michonline.com>

* Use copyFrom() instead of doing it by hand

Signed-off-by: Ryan Anderson <ryan@michonline.com>

* Return a reference instead of copying

Signed-off-by: Ryan Anderson <ryan@michonline.com>

* Deauto things

Signed-off-by: Ryan Anderson <ryan@michonline.com>

* fight with clang-format

Signed-off-by: Ryan Anderson <ryan@michonline.com>

* Just use copyFrom()

Signed-off-by: Ryan Anderson <ryan@michonline.com>

---------

Signed-off-by: Ryan Anderson <ryan.anderson@snowflake.com>
Signed-off-by: Ryan Anderson <ryan@michonline.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Questions that are neither investigations, bugs, nor enhancements stale stalebot believes this issue/PR has not been touched recently
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants