-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
abort
directive returns http/500 on http/3
#5125
Comments
/cc @marten-seemann I think that |
That seems like a really simple fix. A PR would be appreciated :) |
Marking it as "good first issue", hopefully someone would like to take this on on our behalf 😁 |
I opened quic-go/quic-go#3572. @IndeedNotJames, do you want to contribute a fix? |
I wouldn't mind taking a crack at this, but might need some help. |
Please see the quic-go issue. It has all the details. |
quic-go/quic-go#3575 was merged. Thank you @shade34321 for your contribution. |
@shade34321 feel free to make a PR to Caddy updating the quic-go version once released, to make sure it fixes the issue in Caddy 👍 |
Ok, sounds good. |
Matt ended up bumping the quic-go version in #5120 |
But does https://github.com/lucas-clemente/quic-go/releases/tag/v0.29.2 contain the fix? 🤔 I don't see it in the diff. |
Ok, sounds good. I didn't realize they had updated the quic version. If it turns out that version doesn't have the fix just let me know and I'll keep an eye out for a release with my fix. Thanks! |
@mholt oh, I guess it doesn't. Looks like |
Wait no 😅 The current master returns the following:
(basically the same, but without |
Darn. @marten-seemann I guess this patch wasn't included, and unfortunately missed our 2.6.2 tag today. We can get it in the next one though. (And that's correct, Caddy 2.6.2 no longer advertises HTTP/3 over HTTP/3, as there is no need.) |
Indeed, quic-go v0.29.2 only contains the standard library crypto/tls fix, as well as a fix for a busy-looping which would occur under rare circumstances. quic-go/quic-go#3575 will be shipped as part of the v0.30 release. |
Should be fixed with the current master? |
Oh! Yes, we are now pinned to v0.31.0 which should be more than new enough to contain the fix. It will go out with 2.6.3. Thanks everyone! |
I'm afraid this didn't fix it 👀 Built against
|
@IndeedNotJames anything in the (QUIC) logs? |
I fixed it in the pr, which also fixes some of the difference between stdlib and http3. |
Thanks so much @WeidiDeng 😃 |
Fixed in the upstream. |
I think the easiest way to show what I mean by that is with an example:
The connection via
http/2
gets closed immediately, which is expected when using theabort
directive.Excerpt from the docs (permalink):
The same config on
http/3
however, yields a500 Internal Server Error
Running Caddy with
debug
doesn't return any meaningful insights this time, butQUIC_GO_LOG_LEVEL=INFO
(as documented in https://github.com/lucas-clemente/quic-go/wiki/Logging) does:The relevant code snippets seem to be:
caddy/modules/caddyhttp/staticresp.go
Lines 181 to 184 in 498f32b
My curl version, though browsers will run into that too and the curl version doesn't really matter:
A slightly modified version for plain http
yields
As far as I am aware, this has been the case since essentially ever.
I went as far back as
v2.4.6
, which might not seem that far. But I only stopped because my curl version dropped support for the old quic-draft versions.I use the
abort
directive for my wildcard domains (just like the example here) and only noticed it when I did a little typo, to which my browser greeted me with a blank http/500 page instead of the usual and expected connection reset.The text was updated successfully, but these errors were encountered: