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

caddyhttp: Enable HTTP/3 by default #4707

Merged
merged 8 commits into from Aug 15, 2022
Merged

caddyhttp: Enable HTTP/3 by default #4707

merged 8 commits into from Aug 15, 2022

Conversation

mholt
Copy link
Member

@mholt mholt commented Apr 15, 2022

This PR enables HTTP/3 by default (#3833), and implements the RequireAddressValidation callback as recommended in #3055.

Also removes experimental_http3 options/configuration except it does leave it in the JSON config for now, but marks it as deprecated.

Still need to add a way to disable HTTP/3, probably through the use of the existing protocols configuration parameter.

We will likely merge this sometime after the v2.5.0 release.

@mholt mholt added in progress 🏃‍♂️ Being actively worked on under review 🧐 Review is pending before merging do not merge ⛔ Not ready yet! labels Apr 15, 2022
@mholt mholt self-assigned this Apr 15, 2022
@mholt
Copy link
Member Author

mholt commented Apr 15, 2022

@marten-seemann I went ahead and implemented a very simple load statistic (number of currently active requests). Maybe it's not great, but it's so dang cheap and simple that I want to start with it and see how it goes. Let me know if I'm on the right track in terms of implementation, and making sure I have the logic right for AcceptToken. Thanks!

Edit: Or, I wonder if the logic should be more like this (i.e. only use load statistic when it's a RetryToken specifically)?

AcceptToken: func(clientAddr net.Addr, token *quic.Token) bool {
	if token == nil {
		return false
	}
	if token.IsRetryToken && activeRequests != nil {
		highLoad := atomic.LoadInt64(activeRequests) > 1000 // TODO: make tunable
		return highLoad
	}
	return true
},

Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super excited to see this happening! Let me know if you need anything else, happy to help!

listeners.go Outdated Show resolved Hide resolved
@mholt mholt added this to the v2.6.0 milestone Jul 25, 2022
@mholt
Copy link
Member Author

mholt commented Aug 3, 2022

I've pushed a commit that polishes this feature a bit more:

  • Deprecate the server { protocol sub-option in global options, including allow_h2c and strict_sni_host.
  • Move strict_sni_host to sub-option of server
  • Add server { protocols sub-option, which allows setting which HTTP versions to enable: h1 h2 h2c h3 are allowed values.

This lets the user toggle precisely which HTTP versions they want to support, including HTTP/3-only, which is currently our oldest open issue: #1614. (Yay!)

Because the Go standard library does not let us disable HTTP/1.1, we would probably have to implement our own server to support only HTTP/2 (or h2c) without HTTP/1.1 fallback. I actually tried it:

// http2Server is a server that speaks only HTTP/2.
type http2Server struct {
	h1server *http.Server
	server   *http2.Server
	handler  http.Handler
	logger   *zap.Logger
}

func (h2 http2Server) Serve(ln net.Listener) error {
	h2.server = new(http2.Server)
	opts := &http2.ServeConnOpts{
		BaseConfig: h2.h1server,
		Handler:    h2.handler,
	}
	for {
		conn, err := ln.Accept()
		if err != nil {
			return err
		}
		go h2.serve(conn, opts)
	}
}

func (h2 http2Server) serve(conn net.Conn, opts *http2.ServeConnOpts) {
	defer func() {
		if r := recover(); r != nil {
			buf := make([]byte, 64<<10)
			buf = buf[:runtime.Stack(buf, false)]
			h2.logger.Error("panic",
				zap.String("remote", conn.RemoteAddr().String()),
				zap.Any("error", r),
				zap.String("stack", string(buf)))
		}
	}()
	defer conn.Close()
	h2.server.ServeConn(conn, opts)
}

but I could not get it to work. Curl complained, curl: (1) Received HTTP/0.9 when not allowed - which I don't understand, but I didn't spend much time looking into why. Anyway, this seems like a huge maintenance burden & undertaking, so I decided to not support HTTP/2-only servers for now. However, HTTP/1-only and HTTP/3-only configurations work fine!

This means HTTP/3 can be disabled, and we've made protocol configuration easy and consistent.


In summary: I believe have finished this feature, all except for the AcceptToken tweak mentioned above. (I would still like quic.defaultAcceptToken() either exported or always called even if we set our own. /cc @marten-seemann 😃)

Once that is taken care of, I think we can merge this and ship Caddy with HTTP/3 enabled by default.

@mholt mholt mentioned this pull request Aug 3, 2022
@TNQOYxNU
Copy link

TNQOYxNU commented Aug 3, 2022

but I could not get it to work. Curl complained, curl: (1) Received HTTP/0.9 when not allowed - which I don't understand, but did I spend much time looking into why.

try request with curl --http2-prior-knowledge, curl will try to do http upgrade from http1 first even with --http2 option. h2c server's response has no http1 header, will be treated as http0.9.
I tested the code, it works on my machine.

image

@mholt
Copy link
Member Author

mholt commented Aug 4, 2022

@TNQOYxNU Ah, that's good to know! Thank you. I will give that a try next time I revisit this (soon).

@mholt
Copy link
Member Author

mholt commented Aug 4, 2022

Linking this upstream issue as it is the last item remaining before we merge: quic-go/quic-go#3494 -- really appreciate how thoughtful Marten is being about it. 🙂

@bt90
Copy link
Contributor

bt90 commented Aug 10, 2022

quic-go/quic-go#2877 sounds like a possible bottleneck for servers with more traffic.

@mholt
Copy link
Member Author

mholt commented Aug 10, 2022

@bt90 Possibly! Although the upstream blockers appear to have been merged since then, so it's probably just a matter of time.

@mholt
Copy link
Member Author

mholt commented Aug 13, 2022

This branch is ready for final checks; I think we are ready to merge. Thank you @marten-seemann for your help and congrats on the progress!

@mholt mholt marked this pull request as ready for review August 13, 2022 17:35
@mholt mholt removed the in progress 🏃‍♂️ Being actively worked on label Aug 13, 2022
@mholt
Copy link
Member Author

mholt commented Aug 15, 2022

@TNQOYxNU I will go ahead and merge this as a starting point. If we have requests for HTTP/2-only servers that have a legitimate need, we can revisit that feature. But for now I'll just see how it goes. Thanks again for verifying that!

@mholt mholt changed the title Enable HTTP/3 by default caddyhttp: Enable HTTP/3 by default Aug 15, 2022
@mholt mholt merged commit c79c086 into master Aug 15, 2022
@mholt mholt deleted the http3-default branch August 15, 2022 18:02
@mholt mholt added feature ⚙️ New feature or request and removed under review 🧐 Review is pending before merging labels Aug 15, 2022
WilczynskiT pushed a commit to WilczynskiT/caddy that referenced this pull request Aug 17, 2022
@francislavoie francislavoie modified the milestones: v2.6.0, v2.6.0-beta.1 Aug 21, 2022
@mholt mholt modified the milestones: v2.6.0-beta.1, v2.6.0 Sep 13, 2022
toby-griffiths added a commit to toby-griffiths/api-platform that referenced this pull request Sep 21, 2022
The experimental_http3 server protocol option is now failing in
Caddyfile as the option has been removed.

HTTP/3 support is now standard.

See caddyserver/caddy#4707
dunglas pushed a commit to api-platform/api-platform that referenced this pull request Sep 21, 2022
* Removes experimental_http3 as now standard

The experimental_http3 server protocol option is now failing in
Caddyfile as the option has been removed.

HTTP/3 support is now standard.

See caddyserver/caddy#4707

* Removes entire server block & comment

This block is now, no longer required.
xiayesuifeng added a commit to xiayesuifeng/gopanel that referenced this pull request Sep 23, 2022
DarkKirb added a commit to DarkKirb/nixos-config that referenced this pull request Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ⚙️ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants