set host header on self outbound requests #2298
Conversation
karthik2804
force-pushed
the
fix_self_outbound
branch
from
0a78427
to
41dee84
Compare
| @@ -490,6 +490,9 @@ impl OutboundWasiHttpHandler for HttpRuntimeData { | |||
| .parse() | |||
| // origin together with the path and query must be a valid URI | |||
| .unwrap(); | |||
| let host = format!("{}:{}", uri.host().unwrap(), uri.port().unwrap()); | |||
| let headers = request.request.headers_mut(); | |||
| headers.insert(HOST, HeaderValue::from_str(&host)?); | |||
There was a problem hiding this comment.
Same question about potentially overwriting an existing host header.
There was a problem hiding this comment.
We are only overwriting on self-requests.
There was a problem hiding this comment.
I understand that but what if some mad, bad person sends a self request and explicitly sets the host header on it. (I cannot imagine why they would ever do this... I just wonder if we should respect it if they do.)
There was a problem hiding this comment.
I will check if it is a case of overwriting always or only if the user explicitly sets it. In that case, we can check for its existence before setting it.
There was a problem hiding this comment.
By default, the host seems to be
host = Some(
":443",
)
Do we want to do something like
if let Some(":443") = headers.get("host") {
headers.insert(HOST, HeaderValue::from_str(&host)?);
}
There was a problem hiding this comment.
Egad! That host looks extremely weird, though I guess it's what we should expect given what we saw in the issue. It makes me think it's already getting set somewhere - and that makes me wonder if we can tweak wherever it's currently getting set rather than overwriting it later in the pipeline. It reminded me of the issue @dicej found (fermyon/spin-rust-sdk#8 (comment)) so there may be some subtle difference between main and RTM anyway...
Anyway, "user explicitly sets Host header in a self-request" certainly feels like a wildly obscure edge case. Given that it's not trivial to determine, I'm happy to punt on it for the time being. Thanks for the detailed investigation!
There was a problem hiding this comment.
I'll try to dig into where the header actually gets set. I will also update the rust SDK that the app uses to main instead of pinning it to v2.2 to see if that interaction differs.
There was a problem hiding this comment.
I did some digging and this originates from the parsing of the uri by hyper. The first request goes through fine because the host is set properly on the incoming request that we use that to construct the URL and then in the subsequent requests, it fails.
Signed-off-by: karthik2804 <[email protected]>
karthik2804
force-pushed
the
fix_self_outbound
branch
from
41dee84
to
891e8d3
Compare
|
The fix only seems to be needed for the |
Set the host header so that more than one level of
selfrequests work. Not entirely sure if this is the correct approach but fixes the test app I had callinga -> b -> cfixes #2296