-
Notifications
You must be signed in to change notification settings - Fork 845
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
chore(deps): bump go to 1.20 #945
Conversation
(fix #888) |
@DevinCarr Hello Devin, thank you for your time. Would you kindly review this pull request? I appreciate your help. :) |
36ac974
to
a1a378a
Compare
lgtm. Didn't check |
As I have mentioned before in the PR, we can't accept these changes without the following commit either applied to a new fork of quic-go or ideally merged upstream to quic-go: Make max datagram frame size configurable: chungthuang/quic-go/commit/40b85c87 More specifically, you removed this line which we rely on: https://github.com/cloudflare/cloudflared/pull/945/files#diff-d22dda08284a6d0f67f2db14172dfa24bc8afd69bf40d47b7aa5e3ebede00cd5L603 We have not had the time yet to tackle this change since there was some API surface changes happening in quic-go that don't make the application of the commit just a rebase like it was previously. |
I don't think that's necessary anymore. It was meant to fix the problem at quic-go/quic-go#3273, and it's resolved upsteam: quic-go/quic-go#3276. What do you think? |
It was not meant to address the Windows MTU discoverability issue, that change was needed to set the max datagram size as defined in https://www.rfc-editor.org/rfc/rfc9221.html. It looks like there have been some changes in quic-go that make it easier to coordinate this change, but we still require this change to set the MaxDatagramFrameSize to 1350: https://github.com/cloudflare/cloudflared/blob/master/quic/param_unix.go#L6 I have started with a baseline of changes to add this to quic-go, but I haven't had the time yet to test it completely: DevinCarr/quic-go@d1f4eda. Once this change is added to either a fork or merged upstream, we can evaluate the quic-go upgrade and go 1.20 changes to cloudflared. |
I have merged in the quic-go changes (9426b60) that should unblock the rest of this PR if you wish to restart it with just upgrading to go 1.20. Otherwise, I can close this and do it in the next couple of days. We are still not on upstream quic-go but a fork with our applied patch. We have also started a conversation to see if we can get our patch changes into upstream quic-go as the changes should be RFC compliant for quic datagrams. quic-go/quic-go#3300 Thanks for your initial efforts to get this started! |
- update all dependencies - switch to devincarr/quic-go - drop go 1.18 as quic-go no longer supports it
@DevinCarr Hi Devin, I've made the changes as per your suggestions. Appreciate your efforts in addressing this upstream! |
Well, hope this won't take forever to get merged. |
github.com/urfave/cli/v2 v2.3.0 | ||
github.com/prometheus/client_golang v1.15.1 | ||
github.com/prometheus/client_model v0.4.0 | ||
github.com/quic-go/quic-go v0.34.0 |
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.
To support go 1.20 and upcoming go 1.21 (currently at rc4) bump to 0.37.1:
github.com/quic-go/quic-go v0.34.0 | |
github.com/quic-go/quic-go v0.37.1 |
Of course some additional changes may be needed in addition to the usual go mod tidy
and go mod vendor
.
See https://github.com/quic-go/quic-go/releases/tag/v0.37.1 and https://github.com/quic-go/quic-go/releases/tag/v0.37.0
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.
waiting https://github.com/DevinCarr/quic-go & https://github.com/cloudflare/qtls-pq to update/upstream
We are going to address this internally because it requires some more careful details due to QUIC-GO. For now, we will close this. |
Review needed!