-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
net/http: add support for SETTINGS_ENABLE_CONNECT_PROTOCOL #53208
Comments
How do we handle a request sent on an HTTP/1 connection with a An alternative to adding a new field might be to set a |
My inclination is to return an error since my understanding of go's philosophy is that it is a one-way-to-do-things language and you can already set the
I like this approach because it feels a little weird for Protocol to be a top level field on such a common type when it will be so rarely used. I think I actually tried this when implementing support but ran into an error. I decided to propose a new field because psudoheaders are not really supposed to be set by the user, but I'm not so sure how good a justification this is. On reflection, maybe allowing people to set |
I don't have a strong feeling on a An argument for a We do have existing cases where you can set either a field in the |
The websocket handshake for HTTP/1.1 and HTTP/2 are sufficiently different that attempting this is probably not a good idea. There is a bunch of
Gotcha, that is good context and weakens my one-way-to-do-things argument. I think I'm still a little inclined towards allowing a |
I'd rather put |
Proposal SGTM with a |
Should I clean up the gerrit patch to use a |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
Change https://go.dev/cl/508238 mentions this issue: |
Hi @bradfitz @neild @ethanpailes What about store the The quick-go project currently using the |
any update? |
As far as I know I'm still waiting on review by the core go maintainers. |
This is waiting on proposal acceptance. I'll ping the proposal committee to add this to the set under consideration. The proposal, as I understand it: The HTTP/2 CONNECT method is used to convert a connection into a tunnel to a remote host. It takes an Extended CONNECT is an HTTP/2 extension (RFC 8441). When enabled, the the CONNECT can alternatively take a This proposal adds support for extended CONNECT to For servers, the proposal is to advertise extended CONNECT support. When a client makes an extended CONNECT request, the value of the For clients, Is that all correct? If so, I support this proposal, with one caveat that I think can be worked out while this works through proposal committee. A reason extended CONNECT needs to be negotiated is that it changes the meaning of the This applies to existing http handlers which process Does extended |
This proposal has been added to the active column of the proposals project |
I don't think it has to be opt-in but it probably needs a GODEBUG to provide a fallback when hitting endpoints or middleware that is buggy when extended CONNECTs are involved. http2xconnect=0 or something like that. |
No change in consensus, so accepted. 🎉 Proposal details are in #53208 (comment), plus we would need to add a GODEBUG. |
Sorry, I missed this question when you made it. I don't anticipate having the time to work on this myself in the near future, but I'd be happy to review your changes if you'd like to pick them up again. |
I made http2 servers advertise |
Late in cycle, we discovered an issue with the implementation of this proposal: https://go.dev/issue/71128 It turns out that browsers interpret a server advertising Extended CONNECT support as the server also advertising WebSocket-over-HTTP/2 support. This causes problems when net/http supports Extended CONNECT but the WebSocket server implementation does not. As a result, we have disabled Extended CONNECT by default for Go 1.24. It can still be enabled by setting Since enabling Extended CONNECT requires cooperation between the HTTP and WebSocket layers, I don't see any way to enable Extended CONNECT in a backwards-compatible way without providing a knob accessible to the WebSocket layer to enable it. I propose adding such a knob to
|
Pushing back to proposal process for a final check. |
Have all remaining concerns about this proposal been addressed? It turns out that browsers interpret a server advertising Extended CONNECT support as the server also advertising WebSocket-over-HTTP/2 support. This causes problems when net/http supports Extended CONNECT but the WebSocket server implementation does not. As a result, we have disabled Extended CONNECT by default for Go 1.24. It can still be enabled by setting Since enabling Extended CONNECT requires cooperation between the HTTP and WebSocket layers, I don't see any way to enable Extended CONNECT in a backwards-compatible way without providing a knob accessible to the WebSocket layer to enable it. I propose adding such a knob to
|
This proposal has been added to the active column of the proposals project |
Based on the discussion above, this proposal seems like a likely accept. It turns out that browsers interpret a server advertising Extended CONNECT support as the server also advertising WebSocket-over-HTTP/2 support. This causes problems when net/http supports Extended CONNECT but the WebSocket server implementation does not. As a result, we have disabled Extended CONNECT by default for Go 1.24. It can still be enabled by setting Since enabling Extended CONNECT requires cooperation between the HTTP and WebSocket layers, I don't see any way to enable Extended CONNECT in a backwards-compatible way without providing a knob accessible to the WebSocket layer to enable it. The proposed solution is to add such a knob to
|
No change in consensus, so accepted. 🎉 It turns out that browsers interpret a server advertising Extended CONNECT support as the server also advertising WebSocket-over-HTTP/2 support. This causes problems when net/http supports Extended CONNECT but the WebSocket server implementation does not. As a result, we have disabled Extended CONNECT by default for Go 1.24. It can still be enabled by setting Since enabling Extended CONNECT requires cooperation between the HTTP and WebSocket layers, I don't see any way to enable Extended CONNECT in a backwards-compatible way without providing a knob accessible to the WebSocket layer to enable it. The proposed solution is to add such a knob to
|
Overview
I propose extending the x/net/http2 and net/http packages to support the extended CONNECT protocol described in RFC 8441. Support for this will be always-on in the http server, and the http client will automatically detect the presence of support based on the settings flag and adjust the checks it applies to outgoing requests to allow the for the extended CONNECT protocol.
http.Request
will be extended with a newProtocol
member to allow users to set the:protocol
psudoheader.Public Interface Changes
http.Request
will gain a new member,Protocol
of type string.All other functionality can be achieved with existing interfaces.
Implementation
An example implementation of SETTINGS_ENABLE_CONNECT_PROTOCOL support for both the client and the server can be found on gerrit. This is just meant to be a proof of concept, not a fully production ready implementation. Rather than using a field on
http.Request
it uses a magic header calledHACK-HTTP2-Protocol
, but the intention is that this would be switched out forreq.Protocol
for real code.The implementation is pretty non-invasive. It consists of extending the code to allow the use of a new
:protocol
psudo header, sending the new settings flag during the initial handshake, and loosening some checks when the extended CONNECT protocol is in use.Probably the most complex thing about the implementation I was able to come up with is the fact that a CONNECT request coming from the client can now trigger a ping HTTP/2 frame if the server has not yet sent headers. This is required for an extended CONNECT to work if is the very first request on a given http2.Transport object.
Related Issues
#49918 contains some discussion about some of my initial implementation work and links to further context.
The text was updated successfully, but these errors were encountered: