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

net/http: HTTP version selection API #67814

Open
Tracked by #67810
neild opened this issue Jun 4, 2024 · 10 comments
Open
Tracked by #67810

net/http: HTTP version selection API #67814

neild opened this issue Jun 4, 2024 · 10 comments

Comments

@neild
Copy link
Contributor

neild commented Jun 4, 2024

This issue is part of a project to move x/net/http2 into std: #67810

I propose adding a new mechanism for selecting what HTTP versions will be used by a net/http Server or Transport.

// A Protocol is a bitmask of HTTP protocols.
type Protocol uint64

// Contains reports whether p contains all protocols in v.
func (p Protocol) Contains(v Protocol) bool { return p&v != 0 }

const (
        // HTTP1 is the HTTP/1.0 and HTTP/1.1 protocols.
        // HTTP1 is supported on both unsecured TCP and secured TLS connections.
        HTTP1 Protocol = (1 << iota)

        // HTTP2 is the HTTP/2 protocol over a TLS connection.
        HTTP2
)

type Server { // contains unchanged fields
        // Protocols is the set of protocols accepted by the server.
        // If the set is empty, the default is usually HTTP/1 and HTTP/2.
        // The default is HTTP/1 only if TLSNextProto is non-nil
        // and does not contain an "h2" entry.
        Protocols Protocol
}

type Transport { // contains unchanged fields
        // Protocols is the set of protocols supported by the transport.
        // If zero, the default is usually HTTP/1 only.
        // The default is HTTP/1 and HTTP/2 if ForceAttemptHTTP2 is true,
        // or if TLSNextProto contains an "h2" entry.
        Protocols Protocol
}

Currently, by default

  • a net/http server listening on a TLS connection will accept both HTTP/1 and HTTP/2 requests;
  • a net/http transport will use HTTP/1 for https requests (never HTTP/2); and
  • net/http.DefaultTransport will use HTTP/2 when available and HTTP/1 otherwise for https requests.

Users may disable HTTP/2 support by setting Server.TLSNextProto or Transport.TLSNextProto to an empty map.

Users may enable HTTP/2 support on a transport by setting Transport.ForceAttemptHTTP2.

Users may disable HTTP/1 support by importing golang.org/x/net/http2 using an http2.Server or http2.Transport directly.

The net/http package does not currently directly support HTTP/3, but if and when it does, there will need to be a mechanism for enabling or disabling HTTP/3.

The existing APIs for selecting a protocol version are confusing, inconsistent, expose internal implementation details, and don't generalize well to additional protocol versions. The above proposal replaces them with a single, clear mechanism that allows for future expansion.

Example usage:

s := &http.Server{}
s.Protocols = HTTP1 // disable HTTP/2 support

tr := &http.Transport{}
tr.Protocols = HTTP1 | HTTP2 // enable HTTP/2 support
@neild
Copy link
Contributor Author

neild commented Jun 4, 2024

My initial proposal for this in #60746 configured protocols with an ordered []Protocol. After experimenting with implementation, I think that ordered protocol selection is too complicated. (It's not bad with just two protocols, but what happens if we support HTTP/3, someone asks for HTTP/2->HTTP/3->HTTP/1, and ALPN negotiation picks HTTP/1? Do we drop the connection and retry on HTTP/3?)

I'm not sure about the use of a bitmask to represent sets of protocols. It's simple and we're unlikely to ever run out of 64 bits, but perhaps it's a bit too simple? We could have an opaque type instead:

type Protocol int

// Protocols is a set of protocols.
type Protocols struct {}
func (p *Protocols) Add(Protocol)
func (p *Protocols) Remove(Protocol)
func (p Protocols) Contains(Protocol)
func (p Protocols) All() iter.Seq[Protocol]

@rsc
Copy link
Contributor

rsc commented Jun 12, 2024

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Jun 20, 2024

Does it have to be so general at all? An even more pedestrian API would be:

type Protocols struct { ... }
func (p *Protocols) HTTP1() bool
func (p *Protocols) HTTP2() bool
func (p *Protocols) HTTP3() bool
func (p *Protocols) SetHTTP1(ok bool)
func (p *Protocols) SetHTTP2(ok bool)
func (p *Protocols) SetHTTP3(ok bool)

It's not as cute but it doesn't paint us into any corners and it's very clear.

Thoughts?

@neild
Copy link
Contributor Author

neild commented Jun 20, 2024

Looks good to me.

@rsc
Copy link
Contributor

rsc commented Jun 27, 2024

Have all remaining concerns about this proposal been addressed?

The proposal is to add a new copyable type

type Protocols struct { ... }
func (p *Protocols) HTTP1() bool
func (p *Protocols) HTTP2() bool
func (p *Protocols) HTTP3() bool
func (p *Protocols) SetHTTP1(ok bool)
func (p *Protocols) SetHTTP2(ok bool)
func (p *Protocols) SetHTTP3(ok bool)

and then have both Server and Transport add a new field Protocols Protocols that sets which protocols can be used. The default if no protocols are set will be whatever the package deems appropriate.

@rsc
Copy link
Contributor

rsc commented Jul 25, 2024

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The proposal is to add a new copyable type

type Protocols struct { ... }
func (p *Protocols) HTTP1() bool
func (p *Protocols) HTTP2() bool
func (p *Protocols) HTTP3() bool
func (p *Protocols) SetHTTP1(ok bool)
func (p *Protocols) SetHTTP2(ok bool)
func (p *Protocols) SetHTTP3(ok bool)

and then have both Server and Transport add a new field Protocols Protocols that sets which protocols can be used. The default if no protocols are set will be whatever the package deems appropriate.

@rsc
Copy link
Contributor

rsc commented Jul 31, 2024

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The proposal is to add a new copyable type

type Protocols struct { ... }
func (p *Protocols) HTTP1() bool
func (p *Protocols) HTTP2() bool
func (p *Protocols) HTTP3() bool
func (p *Protocols) SetHTTP1(ok bool)
func (p *Protocols) SetHTTP2(ok bool)
func (p *Protocols) SetHTTP3(ok bool)

and then have both Server and Transport add a new field Protocols Protocols that sets which protocols can be used. The default if no protocols are set will be whatever the package deems appropriate.

@rsc rsc changed the title proposal: net/http: HTTP version selection API net/http: HTTP version selection API Jul 31, 2024
@rsc rsc modified the milestones: Proposal, Backlog Jul 31, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/607496 mentions this issue: net/http: add Protocols field to Server and Transport

@neild
Copy link
Contributor Author

neild commented Aug 21, 2024

I've implemented this in CL 607496. Implementation turned up a couple subtleties to the API:

Minor: The accepted proposal uses a pointer receiver for the accessor methods ((*Protocols).HTTP1). I don't see any reason not to use a value receiver for these. The above CL uses a value receiver.

More significant: Let's say you have an http.Server and want to disable HTTP/2. The obvious way to do this is:

s.Protocols.SetHTTP2(false)

This doesn't always work, however. If s.Protocols is zero-valued, SetHTTP2(false) is a no-op and the server continues to use the default protocol set. Or if s.Protocols contains only HTTP/2, SetHTTP2(false) removes the last protocol and the set reverts to a default.

I think that either the zero valued Protocols can be a default protocol set, or Protocols.Set* can take a bool parameter, but both is confusing.

One possibility is to make Server.Protocols and Transport.Protocols pointer values:

s.Protocols = new(http.Protocols) // no protocols, server is unusable
s.Protocols.SetHTTP1(true)        // server now supports HTTP/1
s.Protocols = nil                 // server uses default protocols again
s.Protocols.SetHTTP2(false)       // panic, s.Protocols is nil

Another might be to make Protocols additive-only:

s.Protocols = http.Protocols{} // default protocols
s.Protocols.EnableHTTP1()      // server now supports only HTTP/1

Or maybe we could go back to the very original version of this design and make the Protocols field an unordered slice:

s.Protocols = []http.Protocol{http.HTTP1, http.HTTP2} // HTTP/1 and HTTP/2
s.Protocols = []http.Protocol{http.HTTP2, http.HTTP1} // identical to the above; order does not matter

@neild
Copy link
Contributor Author

neild commented Aug 28, 2024

Perhaps:

// IsZero reports whether p is an unmodified value.
// It reports false if any Set has been called, even if p contains no protocols.
func (p Protocols) IsZero() bool

Now if the user calls s.Protocols.SetHTTP2(false) when s.Protocols is zero-valued, they have a server with no protocols enabled rather than one with the default set. This seems more understandable to me.

An alternative might be to unexport Server.Protocols and Transport.Protocols in favor of accessors (Server.SetProtocols, etc.), but currently Server and Transport have many exported configuration fields and no unexported ones. We could live with the inconsistency, but I'd much prefer to keep the Protocols field exported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

No branches or pull requests

3 participants