these need to be swapped IMO. Passthrough is default, reject is option 1.

+1 for swapping, though per the big convo below I think this is going to get changed anyway.

Thanks for this nice writeup. Few thoughts on the design:

1. Why rely on the upstream being an envoy? why not make this generic such that Envoy will auto follow redirects from any upstream (HTTP 302 has the redirect URL). Typical deployments with Envoys at the edge are more likely to benefit from this option. Nginx has something similar I believe.
2. what is the idea behind the x-envoy-original-url header? Put another way, why rely on a header returned by an untrusted upstream envoy (x-envoy-original-url) instead of keeping track of the original url inside the caller Envoy itself? (may be this is what you meant but the docs are not clear).
3. The idea of rejecting a redirect and returning 500 seems a bit out of place in this "follow_redirects" option. In my mind, follow_redirects: on (aka handle) | off (aka passthrough,default) is different from 'on receiving a response code XYZ, send 500 upstream'. The latter is what you are doing with on301,send500. Couldn't this be accomplished with a simpler disallow_redirects option which if true will cause envoy to send 500 automatically.

1. Any upstream can send this. The header directs Envoy instances to do an internal redirect but there's no documented requirement the originator of the URL is an Envoy instance. I'll try to make that more clear.

2. I think you misread the docs.
   Redirects are initiated by an upstream sending the redirect command. Envoy then sends the x-envoy-original-url to the new upstream. No upstream ever sends x-envoy-original-url, and we protect against untrusted downstreams sending it. I'll add a workflow example

3. The use case I am most familiar with is sending the first request to an auth server, then redirecting to a content URL to serve the response. In this case it is often the case that is's unsafe to ever return the content URL to the user, as they can use it to bypass auth checks. As such I'm inclined to leave both the defaults and the sanity checks as returning 500. I could add a fail-open option (split handle into handle-or-fail or handle-or-pass-through) but realistically any failed internal redirect indicates a bug on the upstream or configuration and I'm not inclined to add extra complexity to more gracefully handle user failure :-P

1. I was talking about

Redirect handling is triggered by an upstream server sending a 302 response with an
x-envoy-internal-redirect header containing the fully qualified URL (http://host/path) to redirect
to

piece in the docs above. Which seems to indicate that the upstream service needs to be aware of Envoy being the Downstream server instead of using HTTP 302 <url>

3. I dont disagree with the use case. What I am stating is that

If the Envoy receiving this response is not configured for internal redirects, or is
explicitly configured to REJECT, the 302 will be converted into a 500 and the
x-envoy-internal-redirect will be removed, to prevent leaking potentially private URLs to untrusted
clients.

for the scenario above, isn't it sufficient to do the following:

1. allow_redirects which if set to true will cause envoy to accept and forward redirect responses to downstream. If set to false, will cause envoy to return a 500 to downstream without revealing any info about the redirect URL sent by the upstream.
2. follow_redirects: on|off. If allow_redirects is set to true and follow_redirects is on, then you follow any 302 response from upstream. If allow_redirects is true and follow_redirects is off, then you forward the 302 response as is to downstream. if allow_redirects is false, then follow_redirects setting will be ignored. Any 302 response from upstream will be turned into a HTTP 500 response (after scrubbing). It might be worthwhile to do (in follow ups) a follow_redirect_depth: N which controls the number of times we are willing to follow redirects (to avoid infinite loops, etc.).

ahhh, that totally makes more sense, my bad. So you'd prefer the originating proxy not need to be Envoy-aware, and Envoy intercept all 302s and use the location header? That's doable and I'm interested to hear if folks think it's a more compelling use case. My concern is twofold. first, intercepting redirects in general is dangerous: often if you're doing cross-domain redirects you're not going to get the correct cookies, or may forward cookies or other PII from one server which should have them to another which is not. Between that and URL leakage concerns I'd rather have the upstream do 302 capturing which is opt-out (you have to do something special to get this behavior) and fail-closed (you don't leak urls by accident) rather than this being something that people turn up without a lot of thought in the hopes of getting a latency boost skipping a client RTT.
We can almost achieve both both with an x-envoy-internal-redirect-fail-close header where default IR behavior was what you're asking for (intercepting 302), we pass-through by default or if #redirects > N (where N is 1 for now but we can always extend in a follow-up), but then the use case I care about doesn't work for multi-level proxying (if pass-through is default we can't allow an L2 envoy to pass a fail-closed URL to an L1 envoy without a configured option PASS_THROUGH_EVEN_FOR_FAIL_CLOSE which feels needlessly complicated)
Any thoughts on how to handle both cases gracefully?

We definitely want to be able to mix regular 302s and IR-302s on the same route so we really can't have the auto-redirect behavior. the best I can think of is to have explicit
handle / pass through for regular 302s and
handle / pass-through / reject for IR-302s

We could either have 2 full set of config, or handle the likely common permutations:
PASS_ALL_THROUGH (302 proxied, 302 IR proxied) <- allow handling IR at L1
HANDLE_EXPLICIT (302 proxied, 302-IR handled) <- what we mainly want
HANDLE_ALL (302 handled, 302-IR handled) <- what you're requesting?
REJECT_EXPLICIT (302 proxied, 302 IR rejected) <- default
It's more complicated than I'd like but I think it gets us both what we want, and with a docced up chart of what happens in what case I think it's doable.

I'm inclined to agree with @rshriram that offering opt-in 302 following is going to be a compelling feature. @alyssawilk the reason you need to differentiate "IR" from "regular" is really because you share a route in which you want to do both? That makes sense to me. I think I would be inclined to just have 2 different config options for "regular" and for header driven "IR" with clear docs. I think this would be the easiest to understand. With that said, I don't feel very strongly about single config vs. split. @rshriram thoughts?

between poor GH formatting and terminology mismatch, i am having a hard time decoding @alyssawilk 's use case for the multi-level envoy stuff.

Between that and URL leakage concerns I'd rather have the upstream do 302 capturing which is opt-out (you have to do something special to get this behavior) and fail-closed (you don't leak urls by accident) rather than this being something that people turn up without a lot of thought in the hopes of getting a latency boost skipping a client RTT.
We can almost achieve both both with an x-envoy-internal-redirect-fail-close header where default IR behavior was what you're asking for (intercepting 302), we pass-through by default or if #redirects > N (where N is 1 for now but we can always extend in a follow-up), but then the use case I care about doesn't work for multi-level proxying (if pass-through is default we can't allow an L2 envoy to pass a fail-closed URL to an L1 envoy without a configured option PASS_THROUGH_EVEN_FOR_FAIL_CLOSE which feels needlessly complicated)
Any thoughts on how to handle both cases gracefully?

Would it be possible to scribble a diagram on paper napkin and stick it in slack so that its easier to visualize the actors involved?

I think Matt's got it, and I agree with him that 2 configs is going to be less confusing than one.

I'll put together the change Matt and I thinks makes sense along with docs, and ask @rshriram to vet that they're substantially more clear than my github comments :-D

Ok, draft version up - docs are not cleaned up and now has E_INSUFFICIENT_INTEGRATION_TESTS too, but I'm hoping this is enough to clarify a bit for @rshriram . WDYT?

I think maybe you could use an EnumValue (?) WKT and then have different defaults?

Do you mind adding documentation for all the enum values so it's easier to review the options inline?

and general redirects

In the final change can you add x-envoy-internal-redirect to the HCM headers doc and cross link?

I think it might help to define somewhere in this doc a use case for an "internal redirect." This might help readers that might be confused as to why anyone would do this?

doesn't this mandate that the downstream has to be an Envoy? which won't work for the common case.

Ping on this. Can you move this into a warning/attention block?

ping on this nit, or just resolve if you don't want to

Can you add a sanitizing test for this in the unit tests for this file? I think there is already a test that covers sanitizing.

Worth having some specific unit tests for this new class?

nit: "once. Per-route..." (I think new sentence reads more clearly)

Nit: response to the original request.

Nit: missing ":" here and "." at the end of few sentences in the list.

Nit: missing ":" here and "." at the end of the sentences in the list.

s/woth body/with request body/

Do you want to restrict this to GET and HEAD requests? If not, then we should convert other requests to GET in convertRequestHeadersForInternalRedirect().

Nope - I want to add support for bodies, though in a follow-up (or series of follow-up) PRs.

302 redirects are historically turned into GET requests by the eyeballs and intercepting proxies, so the target of the redirection might simply not support the same method as the original request. I'd suggest converting redirects into GET (and leaving HEAD as-is) for 302 and 303 redirects, and keeping original method for 307 redirects.

As for supporting request bodies, I'm all for it (though it adds non-trivial amount of complexity), but if they are not supported as of this PR, then I'd still restrict internal redirects to GET and HEAD methods in this PR, and lift that restriction in one of the follow-up PRs that adds support for request bodies.

302 redirects are historically turned into GET requests by the eyeballs and intercepting proxies, so the target of the redirection might simply not support the same method as the original request. I'd suggest converting redirects into GET (and leaving HEAD as-is) for 302 and 303 redirects, and keeping original method for 307 redirects.

As for supporting request bodies, I'm all for it (though it adds non-trivial amount of complexity), but if they are not supported as of this PR, then I'd still restrict internal redirects to GET and HEAD methods in this PR, and lift that restriction in one of the follow-up PRs that adds support for request bodies.