Change implementation of incoming-request.authority
#2684
Conversation
|
FYI: The CI failure is expected as this breaks one of the conformance tests. Once we're happy with the changes here, I will change the conformance test to match this behavior. |
crates/trigger-http/src/lib.rs
Outdated
| ) -> Result<Response<Body>>; | ||
| } | ||
|
|
||
| /// The definition of the `self` host for outbound requests. | ||
| #[derive(Clone)] | ||
| pub struct SelfRequestDefinition { |
There was a problem hiding this comment.
Could this perhaps be SelfRequestOrigin? "Definition" is pretty vague and I doubt future readers would have a reasonable intuition for what it stands for.
The origin concept of course technically includes the port, too; one way to address that would be to simply include it. But I think it'd also be fine to just add a comment saying that for our purposes that part is irrelevant, so left out.
There was a problem hiding this comment.
Authority does include port, either explicitly or implicitly via scheme. This struct could also alternatively wrap (or be replaced by) a Uri, i.e. a "self base URI".
There was a problem hiding this comment.
I renamed to SelfRequestOrigin and kept its fields (URI contains a lot of other stuff we don't need and I think it makes the code harder to read).
Always rely on the `Host` header for authority incoming-reques.authority. To determine where `self` requests should be routed, explicitly pass a `self_authority` parameter which is set to the listening address. Signed-off-by: Ryan Levick <[email protected]>
Signed-off-by: Ryan Levick <[email protected]>
Signed-off-by: Ryan Levick <[email protected]>
| builder = builder.header(*key, *value); | ||
| } | ||
|
|
||
| // TODO(rylev): convert body as well |
| /// The incoming request's URI is relative to the server, so we need to set the scheme and authority | ||
| fn set_req_uri(req: &mut Request<Body>, scheme: Scheme, addr: SocketAddr) -> Result<()> { | ||
| /// The incoming request's URI is relative to the server, so we need to set the scheme and authority. | ||
| /// The `Host` header is used to set the authority. This function will error if no `Host` header is |
There was a problem hiding this comment.
The
Hostheader is used to set the authority.
Should this also check the req.uri().authority()? I think it might be most correct to use either one, verifying that they are identical if both are present.
There was a problem hiding this comment.
I'm pretty sure that's right, based on hyperium/hyper#1612
|
Does this need another set of reviews, or is it good to merge? Also, surfacing the test failure: |
|
We can land this whenever we feel comfortable with the breaking change. One thought I had was that we'll need to bring these changes into the factors break. Instead of landing this in |
|
Given we know it's the right change, bringing it in together with the rest of the breaking changes from the factors work seems reasonable to me. |
|
Closing this in favor of the factors branch where #2723 was incorporated which addresses this issue. |
The guest will now see
incoming-request.authorityas whatever is set in the incoming request'sHostheader.Previously we were relying on the definition of
incoming-request.authoritynot being set by user input because outbound requests to the specialselfhost would be ultimately directed towards whatever was inincoming-request.authority. Now,selfrequests are specifically defined (with scheme and authority) in the host. The implementation currently sets theselfdefinition to be the same scheme as the incoming request and the authority is set to the listener address of the Spin web server.