feat(nginx): add optional HTTP/3 (QUIC) support - #457
Conversation
|
Warning Review limit reached
Next review available in: 32 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe image defaults ChangesHTTP/3 support
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
The nginx-unprivileged base image is already built with --with-http_v3_module, but nothing enabled it. Adds an HTTP3 env var (default off) that, when set to "on", adds a QUIC listener alongside the existing SSL listener, turns on http3, enables quic_retry, and advertises support via the Alt-Svc header. Off by default since QUIC needs the SSL_PORT published as UDP too, which existing deployments won't have done.
Publishes SSL_PORT on the existing verification container and checks the Alt-Svc header is absent by default. Adds a second, HTTP3=on container on one matrix leg (nginx-debian-writable-latest, amd64) to confirm the QUIC listener starts and Alt-Svc advertises h3.
27f610c to
85f9384
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nginx/docker-entrypoint.d/96-update-http3-config.sh`:
- Line 11: Update the HTTP3 configuration generation around HTTP3_CONFIG to
define HTTP3_ALT_SVC_PORT with SSL_PORT as its default and use it in the Alt-Svc
advertisement, while keeping the listen directive bound to SSL_PORT. Set
HTTP3_ALT_SVC_PORT to 8444 in the opt-in workflow and assert that the generated
Alt-Svc header advertises port 8444.
In `@README.md`:
- Line 258: Update the README’s HTTP3 configuration entry to describe HTTP/3
independently, explicitly stating that enabling HTTP3 does not enable HTTP/2 and
that HTTP2=off keeps HTTP/2 unavailable over TCP; refer readers to the HTTP2
setting for HTTP/2 control.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a22477b7-a002-4ae1-a592-7bf260ba95cf
📒 Files selected for processing (6)
.github/workflows/verifyimage.ymlREADME.mdnginx/Dockerfilenginx/Dockerfile-alpinenginx/docker-entrypoint.d/96-update-http3-config.shnginx/templates/conf.d/default.conf.template
…rify HTTP2/HTTP3 independence in docs Adds HTTP3_ALT_SVC_PORT (defaults to SSL_PORT) so the Alt-Svc header advertises the externally reachable port when it differs from the container's internal SSL_PORT (e.g. -p 8444:8443/udp). The CI opt-in test now exercises this remapped-port case directly. Also clarifies in the README that HTTP3 and HTTP2 are independent settings; enabling one doesn't affect the other.
| HTTP3_CONFIG="listen ${SSL_PORT} quic reuseport; http3 on; quic_retry on; add_header Alt-Svc 'h3=\":${HTTP3_ALT_SVC_PORT}\"; ma=86400' always;" | ||
| fi | ||
|
|
||
| sed -i.bak -r 's#HTTP3_CONFIG#'"${HTTP3_CONFIG}"'#' /etc/nginx/conf.d/default.conf |
There was a problem hiding this comment.
| sed -i.bak -r 's#HTTP3_CONFIG#'"${HTTP3_CONFIG}"'#' /etc/nginx/conf.d/default.conf | |
| sed -ir 's#HTTP3_CONFIG#'"${HTTP3_CONFIG}"'#' /etc/nginx/conf.d/default.conf |
|
|
||
| server { | ||
| listen ${SSL_PORT} ssl; | ||
| HTTP3_CONFIG |
There was a problem hiding this comment.
Does this really work??
| HTTP3_CONFIG | |
| ${HTTP3_CONFIG} |
2 participants
Summary
HTTP3env var (defaultoff) to the nginx image. When set toon, adds a QUIC listener onSSL_PORT, enableshttp3/quic_retry, and advertises support via theAlt-Svcheader.nginxinc/nginx-unprivilegedbase image is already compiled with--with-http_v3_module(confirmed againstnginx/pkg-oss's Debian and Alpine build configs) — this PR only wires up config, no rebuild needed. Our own build stage rebuilds nginx from source only to produce the ModSecurity/headers-more dynamic modules; the running binary is untouched upstream.SSL_PORTas UDP too (e.g.-p 8443:8443/udp), which existing deployments won't have done.verifyimage.yml): publishesSSL_PORTon the existing verification container and asserts noAlt-Svcheader by default; starts a second container withHTTP3=onon one matrix leg (nginx-debian-writable-latest,linux/amd64) and asserts the QUIC listener starts andAlt-Svcadvertisesh3.Test plan
actionlint/shellcheckclean on the new entrypoint script and CI changes (2 pre-existing shellcheck warnings inverifyimage.ymlpredate this change)zizmorclean onverifyimage.ymlsedsubstitution in96-update-http3-config.shfor bothHTTP3=onandHTTP3=offHTTP3=onand serves theAlt-SvcheaderAI Disclosure
Per the project's AI-Assisted Contribution Policy:
nginx/pkg-ossbuild configs directly rather than assuming), theHTTP3env var and96-update-http3-config.shentrypoint script (following the existingPROXY_SSL_CONFIGplaceholder pattern), template/Dockerfile/README changes, and the CI verification steps inverifyimage.yml.Dockerfilebuild to confirm the running image uses the unmodified upstream nginx binary. Verified thesed-based config substitution locally for both on/off states. Ranactionlint,shellcheck, andzizmoragainst changed files. The actual CI run (container boot +Alt-Svccheck) is marked unverified in the test plan above pending this PR's own CI run.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
HTTP3=onsetting.Bug Fixes