-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
nginx_proxy: Support HTTP/3 (QUIC) #4234
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
📝 WalkthroughWalkthroughAdds HTTP/3 (QUIC) support: bumps addon version to 3.15.0, exposes UDP port 443, detects/configures a quic_port at runtime, passes quic_port into the nginx template, conditions QUIC listeners and Alt‑Svc on the presence of quic_port, and updates docs, changelog, and translations. No other API/signature changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Addon as Addon metadata
participant s6run as s6-run script
participant Template as nginx.conf.gtpl renderer
participant nginx as nginx
Addon->>s6run: provide mapped ports (tcp/udp)
s6run->>s6run: QUIC_PORT = UDP 443 or null
s6run->>Template: render with JSON { port: <tcp>, quic_port: <number|null> }
Template->>nginx: write generated nginx.conf
nginx->>nginx: enable listeners based on quic_port presence
alt quic_port set
note right of nginx `#DFF2E1`: UDP QUIC listeners added\nlisten ... quic\nhttp3 on\nAlt-Svc header emitted
else quic_port null
note right of nginx `#F7F2E8`: No QUIC/HTTP3 configuration emitted
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
nginx_proxy/translations/en.yaml (1)
23-25: HTTP/3 labels look good; consider minor text polishThe new
http3configuration label and the443/udpport label are clear and consistent with the rest of the add-on.Two small optional tweaks:
- Consider aligning the description with the docs by mentioning “HTTP/3 over QUIC/UDP in addition to HTTP/2 and HTTP/1.1” for extra clarity.
- There is an existing typo in the
keyfiledescription (“Private Private Key File”) you might want to correct while touching this file.Also applies to: 37-37
nginx_proxy/DOCS.md (1)
41-43: Tighten http3 docs wording and fix minor typoThe new HTTP/3 documentation is clear but can be tightened and aligned with existing style:
- For the boolean flag, “If true” is clearer than “If specified”:
-If specified, configures Nginx to use HTTP/3 with QUIC/UDP in addition to HTTP/2 and HTTP/1.1; [for more information](https://nginx.org/en/docs/http/ngx_http_v3_module.html). -Make sure to enable UDP in your port forward or firewall in addition to TCP. +If true, configures Nginx to use HTTP/3 with QUIC/UDP in addition to HTTP/2 and HTTP/1.1; [for more information](https://nginx.org/en/docs/http/ngx_http_v3_module.html). +Enable UDP in your port forward or firewall in addition to TCP.
- Fix the spelling of “advertize” and slightly clarify the description of
http3.portas the advertised HTTP/3 port that must match the network port label:-Port to advertize with the `Alt-Svc 'h3=":443";` header. Must match the specified network HTTP/3 (QUIC) Port value. +Port to advertise with the `Alt-Svc 'h3=":443";` header. Must match the configured network **HTTP/3 (QUIC) Port** value.These changes keep the docs concise and more directive while matching the new configuration surface.
Also applies to: 85-93
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
nginx_proxy/CHANGELOG.md(1 hunks)nginx_proxy/DOCS.md(2 hunks)nginx_proxy/config.yaml(2 hunks)nginx_proxy/rootfs/etc/nginx/nginx.conf.gtpl(2 hunks)nginx_proxy/translations/en.yaml(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
*/**(html|markdown|md)
⚙️ CodeRabbit configuration file
*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
*/**(html|markdown|md): - Use bold to mark UI strings.
If "" are used to mark UI strings, replace them by bold.
Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Use sentence-style capitalization also in headings.
do not comment on HTML used for icons
Avoid flagging inline HTML for embedding videos in future reviews for this repository.
Files:
nginx_proxy/DOCS.mdnginx_proxy/config.yamlnginx_proxy/CHANGELOG.md
🔇 Additional comments (2)
nginx_proxy/config.yaml (1)
2-2: HTTP/3 options, port mapping, and schema are consistentThe manifest changes are internally consistent:
version: 3.15.0matches introducing a new feature and should pair with a3.15.0changelog entry.- The new
http3block (active: false,port: 443) cleanly exposes an opt-in flag and the Alt-Svc port.- Adding
443/udp: 443inportsaligns with the new QUIClisten 443 quicdirectives and the “HTTP/3 (QUIC) Port” label intranslations/en.yaml.- The
schema.http3definition (active: bool,port: int) matches the options structure and the way.options.http3is used innginx.conf.gtpl.No structural issues spotted here.
Also applies to: 25-27, 35-35, 43-45
nginx_proxy/rootfs/etc/nginx/nginx.conf.gtpl (1)
43-47: HTTP/3 configuration verified as correctThe NGINX HTTP/3 setup in the template is correct and aligns with current NGINX 1.28.x best practices. The pattern using
listen ... quic reuseportwithhttp3 on;is the current recommendation from official NGINX documentation, and bothquicandproxy_protocolare validlistendirective parameters.The conditional blocks at lines 43-47, 77-81, 86-90, and 97-99 properly implement HTTP/3 support with all directives correctly used.
897b04d to
11466d1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
nginx_proxy/config.yaml (1)
35-35: UDP 443 mapping default is reasonable but always exposedAdding
443/udp: 443ensures HTTP/3 works out of the box once enabled, but it also exposes UDP 443 even whenhttp3.activeis false. If you want a stricter default surface, consider defaulting this tonulland relying on docs to have users enable UDP when they turn on HTTP/3.nginx_proxy/rootfs/etc/nginx/nginx.conf.gtpl (1)
97-99: Alt-Svc wiring is correct; consider edge casesUsing
Alt-Svc 'h3=":{{ .options.http3.port }}"; ma=86400'correctly ties advertisement to the configured HTTP/3 port and only emits it when HTTP/3 is active. Ensure documentation clearly states thathttp3.portmust match the externally exposed UDP port, and consider addingalwaysif you want Alt-Svc on error responses as well (optional).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
nginx_proxy/CHANGELOG.md(1 hunks)nginx_proxy/DOCS.md(2 hunks)nginx_proxy/config.yaml(2 hunks)nginx_proxy/rootfs/etc/nginx/nginx.conf.gtpl(2 hunks)nginx_proxy/translations/en.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- nginx_proxy/translations/en.yaml
- nginx_proxy/CHANGELOG.md
- nginx_proxy/DOCS.md
🧰 Additional context used
📓 Path-based instructions (1)
*/**(html|markdown|md)
⚙️ CodeRabbit configuration file
*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
*/**(html|markdown|md): - Use bold to mark UI strings.
If "" are used to mark UI strings, replace them by bold.
Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Use sentence-style capitalization also in headings.
do not comment on HTML used for icons
Avoid flagging inline HTML for embedding videos in future reviews for this repository.
Files:
nginx_proxy/config.yaml
🔇 Additional comments (3)
nginx_proxy/config.yaml (2)
2-2: Version bump aligns with new functionalityVersion change to
3.15.0is appropriate for introducing HTTP/3 support; no issues from a config perspective.
25-27: http3 options/schema look consistent; verify upgrade behaviorThe
http3options and schema are consistent, and defaultingactive: falseis a safe way to gate the feature. Verify that on upgrade from pre-3.15.0 installs, the supervisor populates the nestedhttp3structure so that.options.http3.activeand.options.http3.portare always defined for the template.Also applies to: 43-45
nginx_proxy/rootfs/etc/nginx/nginx.conf.gtpl (1)
43-47: Conditional HTTP/3 listeners are wired consistentlyThe new
listen ... quicdirectives andhttp3 on;are consistently wrapped in{{- if .options.http3.active }}across the default, non‑proxy_protocol, and proxy_protocol servers, which cleanly gates QUIC support without affecting existing HTTP/1.1/2 behavior. Confirm that the nginx version in this add‑on is built with HTTP/3/QUIC support and that thequic reuseport proxy_protocolcombination matches the versioned nginx docs you target.Also applies to: 77-81, 86-90
|
Thank you for your PR, nice addition 🤩
It should be possible to read the port using |
| # shellcheck disable=SC2046 | ||
| JSON_CONF=$(jq --arg port $(bashio::core.port) \ | ||
| '({options: .}) + ({variables: {port: $port}})' \ | ||
| JSON_CONF=$(jq --argjson port $(bashio::core.port) --argjson quic_port $QUIC_PORT \ | ||
| '({options: .}) + ({variables: {port: $port, quic_port: $quic_port}})' \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With simple quotes the values are still consumed as numbers (since bash removes them). This eliminates the linter error:
| # shellcheck disable=SC2046 | |
| JSON_CONF=$(jq --arg port $(bashio::core.port) \ | |
| '({options: .}) + ({variables: {port: $port}})' \ | |
| JSON_CONF=$(jq --argjson port $(bashio::core.port) --argjson quic_port $QUIC_PORT \ | |
| '({options: .}) + ({variables: {port: $port, quic_port: $quic_port}})' \ | |
| JSON_CONF=$(jq --argjson port "$(bashio::core.port)" --argjson quic_port "$QUIC_PORT" \ | |
| '({options: .}) + ({variables: {port: $port, quic_port: $quic_port}})' \ |
| listen [::]:443 ssl; | ||
| http2 on; | ||
| {{- if .variables.quic_port }} | ||
| listen 443 quic reuseport; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is reuseport required here? We don't use it for the other instances. I'd expect a problem if that port is already used by some other process. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, but every example of NGINX and QUIC I found used the reuseport keyword.
Add option to nginx_proxy for HTTP/3 (QUIC) support over UDP.
Try it out here with this custom addon repository:
https://github.com/kmhallen/ha-addon-nginx_proxy_quic
HTTP/3 should enable more seamless roaming from WiFi to cellular because the connection ID feature enables a connection to continue when the client IP address changes.
I couldn't find any reliable online HTTP/3 test websites, but browser developer consoles can be used to verify if HTTP/3 is being used. Chromium based browsers seem to be more likely to use HTTP/3 than Firefox. I'm not sure about the Android and iOS companion apps.
The port is specified both in options and ports because I couldn't find a way to read the port value directly. The port is needed to advertise HTTP/3 support in the
Alt-Svcheader.Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.