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

Internal redirect interactions with filters (e.g, header_to_metadata) are confusing or undocumented #30441

Closed
sfc-gh-randerson opened this issue Oct 24, 2023 · 6 comments · Fixed by #30793
Labels
question Questions that are neither investigations, bugs, nor enhancements

Comments

@sfc-gh-randerson
Copy link

I have an internal redirect setup that works for the redirects, but I am unable to find a way to pass data from the redirect response to the followup (internal) request.

I think this is because http filters are not being processed due to the internal redirect, but I'm extremely confused at this point.

I have a route entry that looks like this:

                {
                    "match": {
                        "path": "/monitoring/queries",
                        "headers": [
                            {
                                "name": ":method",
                                "exactMatch": "GET"
                            }
                        ]
                    },
                    "route": {
                        "cluster": "local",
                        "timeout": "0s",
                        "idleTimeout": "50s",
                        "retryPolicy": {
                            "numRetries": 0
                        },
                        "internalRedirectPolicy": {
                            "maxInternalRedirects": 2,
                            "redirectResponseCodes": [
                                307
                            ],
                            "allowCrossSchemeRedirect": true
                        }
                    },
                    "typedPerFilterConfig": {
                        "type.googleapis.com/envoy.extensions.filters.http.header_to_metadata.v3.Config": {
                            "@type": "type.googleapis.com/envoy.extensions.filters.http.header_to_metadata.v3.Config",
                            "responseRules": [
                                {
                                    "header": "x-snowflake-redirect-count",
                                    "onHeaderPresent": {
                                        "metadataNamespace": "snowflake.md",
                                        "key": "redirectcount",
                                        "type": "NUMBER"
                                    }
                                }
                            ]
                        }
                    },
                    "requestHeadersToAdd": [
                        {
                            "header": {
                                "key": "x-snowflake-redirect-permitted",
                                "value": "true"
                            },
                            "appendAction": "OVERWRITE_IF_EXISTS_OR_ADD"
                        }
                    ]
                },

On the routes I'm redirecting to, I have:

                    "requestHeadersToAdd": [
                        {
                            "header": {
                                "key": "x-snowflake-redirect-count",
                                "value": "%DYNAMIC_METADATA(snowflake.md:redirectcount)%"
                            },
                            "appendAction": "OVERWRITE_IF_EXISTS_OR_ADD"
                        },

In addition, I have some Lua that runs as the default Lua configured on the listener:


function envoy_on_response(response_handle)
  local redirect_count = response_handle:headers():get("x-snowflake-redirect-count") or ""
  response_handle:logWarn("redir count = " .. redirect_count)
  local mdcount = response_handle:metadata():get("snowflake.md:redirectcount") or ""
  response_handle:logWarn("metadata redir count = " .. mdcount)
  local smdcount = response_handle:streamInfo():dynamicMetadata():get("snowflake.md:redirectcount") or ""
  response_handle:logWarn("strema metadata redir count = " .. smdcount)
end

This always logs empty strings and notably, doesn't log at all after the 307 redirect response, and seems to only trigger after "final" responses.

Is there some way to pass this data from the internal redirect response to the new internal redirect target?

(The way internal redirects interact with filters and other things isn't really documented as far as I can tell.)

Running 1.26.5 in production and 1.27.0 for local testing. (I should update that one.) Though other upgrades are possible if it'll help make this work.

I can try to pull together a more complete config if that would help, but I think this covers all the relevant details.

Aside: It would really be nice if the header_to_metadata filter would log something with --log-level=trace; it's very opaque right now.

@sfc-gh-randerson sfc-gh-randerson added the triage Issue requires triage label Oct 24, 2023
@alyssawilk alyssawilk added question Questions that are neither investigations, bugs, nor enhancements and removed triage Issue requires triage labels Oct 24, 2023
@alyssawilk
Copy link
Contributor

for the logging, feel free to send a PR with logging added and send it my way

For the way internal redirects work, ideally if you start with example.com/bar and redirect to example.com/foo, a new (internal) stream is created and it's as if the client sent the request to example.com/foo so the new stream will only go through foo filters. Any changes made to headers while processing bar filters will be maintained.

@sfc-gh-randerson
Copy link
Author

What filters does the response (the 3xx that triggers the internal redirect) go through?

(It doesn't seem to be triggering the header_to_metdata response_rules actions, for example.)

@sfc-gh-randerson
Copy link
Author

I read more closely; does this new internal stream copy over the metadata from the original stream?

@sfc-gh-randerson
Copy link
Author

Oh, I think you've actually answered my question back in #16777

It seems like there's no way (at the moment) of passing information between the 3xx internal redirect response and the subsequent request. Is that still true?
(I'm going to make some people very sad tomorrow, I think.)

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>
Copy link

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 Nov 24, 2023
@sfc-gh-randerson
Copy link
Author

Still working on the PR above.

no stalebot

@github-actions github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Nov 28, 2023
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants