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

feat: experimental HTTP/3 support + optional HTTP/2 disabling #2278

Merged
merged 5 commits into from Dec 8, 2023
Merged

Conversation

buchdag
Copy link
Member

@buchdag buchdag commented Jul 18, 2023

@SchoNie I went the extra mile and opened a draft PR with what I got so far. It seems mostly functional to me, let me know if you think something is missing / should be done differently.

It indeed work when replacing proxy_set_header Host $http_host; with proxy_set_header Host $host;, should we also replace the other uses of $http_host beside this one ?

The PR still lacks documentation at this point, and I don't think it can be automatically tested beside parsing the rendered template (Python's request is not even capable of doing HTTP/2).

New labels on the proxied containers:

  • com.github.nginx-proxy.nginx-proxy.http2.enable:
    enable HTTP/2 support when set to true (default: true)
  • com.github.nginx-proxy.nginx-proxy.http3.enable:
    enable HTTP/3 support when set to true (default: false)

New environment variables on the proxy container:

  • ENABLE_HTTP2: globally enable HTTP/2 when set to true (default: true)
  • ENABLE_HTTP3: globally enable HTTP/3 when set to true (default: false)

closes #2170
closes #1055

@buchdag buchdag added status/pr-needs-docs This PR needs new or additional documentation type/feat PR for a new feature labels Jul 18, 2023
@buchdag buchdag self-assigned this Jul 18, 2023
@buchdag buchdag linked an issue Jul 18, 2023 that may be closed by this pull request
nginx.tmpl Outdated Show resolved Hide resolved
nginx.tmpl Show resolved Hide resolved
nginx.tmpl Show resolved Hide resolved
Copy link
Contributor

@SchoNie SchoNie left a comment

Choose a reason for hiding this comment

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

I think it is safe to change to $host there is only 1 instance left in the $http_x_forwarded_proto map.
I ran the tests with change and do not encounter any difference.

@buchdag
Copy link
Member Author

buchdag commented Jul 22, 2023

I re-organised the template a bit so that HTTP/3 related stuff is a bit more grouped together.

More importantly I moved the add-header statement for Alt-Svc from the location template to the server block to avoid overriding the HSTS header (thanks @Knapoc for spotting this in #2271).

I also removed the $http3 variable from logs because as explained in discussion above we already have the protocol logged through the $request variable.

And finally I replaced the last $http_host instance with $host.

@buchdag
Copy link
Member Author

buchdag commented Jul 27, 2023

@SchoNie @Knapoc is it okay if I add you as co-authors on relevant commits ?

@buchdag buchdag removed the status/pr-needs-docs This PR needs new or additional documentation label Jul 27, 2023
@SchoNie
Copy link
Contributor

SchoNie commented Jul 27, 2023

👍
I did create a few tests. I adjusted the hsts test to check for overridden headers.
And made soms basic http3 tests. Mostly to check for config syntax as there is not really a method to test by requests.

Will try to push later when at office and then you can improve or discard😅

cool this PR is getting along! Hopefully it helps more people to enable QUIC and maybe it encourages to use of this project as a early adopter.

@SchoNie
Copy link
Contributor

SchoNie commented Jul 28, 2023

HSTS override tests, http2 & http3 tests: SchoNie@618da60
Feel free to use, improve or discard.

@buchdag
Copy link
Member Author

buchdag commented Sep 4, 2023

@SchoNie sorry for the lack of activity on august, the tests looked interesting, do you still have them ?

@SchoNie
Copy link
Contributor

SchoNie commented Sep 11, 2023

@buchdag Yes. Still have the tests: 27cc611

buchdag and others added 5 commits December 8, 2023 00:48
Co-authored-by: Nicolas Duchon <nicolas.duchon@gmail.com>
Co-authored-by: Knapoc <Knapoc@users.noreply.github.com>
Co-authored-by: Nicolas Duchon <nicolas.duchon@gmail.com>
Co-authored-by: Niek <100143256+SchoNie@users.noreply.github.com>
Co-authored-by: Nicolas Duchon <nicolas.duchon@gmail.com>
Co-authored-by: Patrick Domack <patrickdk@patrickdk.com>
Co-authored-by: Nicolas Duchon <nicolas.duchon@gmail.com>
Co-authored-by: Niek <100143256+SchoNie@users.noreply.github.com>
@buchdag buchdag marked this pull request as ready for review December 7, 2023 23:59
@buchdag buchdag merged commit d05175d into main Dec 8, 2023
2 checks passed
@buchdag buchdag deleted the http3 branch December 8, 2023 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feat PR for a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QUIC and HTTP/3 Support Disable HTTP/2 http2 option
2 participants