build: update quic-go version #4385
Conversation
Update the quic-go version to v0.36.3. This is the last version that does not require Go 1.20 yet. We will upgrade the Go version in a follow up PR.
There was a problem hiding this comment.
Reviewed 15 of 15 files at r1.
Reviewable status: 10 of 17 files reviewed, 2 unresolved discussions (waiting on @oncilla)
pkg/snet/squic/net.go line 379 at r1 (raw file):
var err error session, err = quic.Dial(ctx, d.Conn, dst, tlsConfig, quicConfig)
Multiple Dial calls on the same PacketConn are no longer allowed. This panics. We need to use a quic.DialListener here (see migration guide in quic-go/quic-go#3727).
pkg/snet/squic/net.go line 379 at r1 (raw file):
var err error session, err = quic.Dial(ctx, d.Conn, dst, tlsConfig, quicConfig)
The SNI name is now (only) in tlsConfig.ServerName, which we don't set AFAICT. Is this legal, and compatible with older servers?
There was a problem hiding this comment.
Reviewable status: 10 of 17 files reviewed, 2 unresolved discussions (waiting on @matzf)
pkg/snet/squic/net.go line 379 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Multiple Dial calls on the same PacketConn are no longer allowed. This panics. We need to use a
quic.DialListenerhere (see migration guide in quic-go/quic-go#3727).
Done.
pkg/snet/squic/net.go line 379 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
The SNI name is now (only) in tlsConfig.ServerName, which we don't set AFAICT. Is this legal, and compatible with older servers?
Not sure, I need to double-check
There was a problem hiding this comment.
Reviewable status: 10 of 17 files reviewed, 2 unresolved discussions (waiting on @matzf)
pkg/snet/squic/net.go line 379 at r1 (raw file):
Previously, oncilla (Dominik Roos) wrote…
Not sure, I need to double-check
I was able to connect from new quic client to existing server without issues.
But, I did have InsecureSkipVerify enabled, so all bets are off.
I set the ServerName to be compatible with the old version for sure.
There was a problem hiding this comment.
Reviewable status: 10 of 17 files reviewed, 2 unresolved discussions (waiting on @matzf)
pkg/snet/squic/net.go line 379 at r1 (raw file):
Previously, oncilla (Dominik Roos) wrote…
I was able to connect from new quic client to existing server without issues.
But, I did have InsecureSkipVerify enabled, so all bets are off.I set the ServerName to be compatible with the old version for sure.
Hm, i need to still investigate why drkey test is failing. Most likely due to this .
There was a problem hiding this comment.
Reviewable status: 10 of 21 files reviewed, 2 unresolved discussions (waiting on @matzf)
pkg/snet/squic/net.go line 379 at r1 (raw file):
Previously, oncilla (Dominik Roos) wrote…
Hm, i need to still investigate why drkey test is failing. Most likely due to this .
💡 server names don't have ports 🤦
Should be good to go now.
There was a problem hiding this comment.
Reviewable status: 10 of 22 files reviewed, 2 unresolved discussions (waiting on @matzf)
pkg/snet/squic/net.go line 379 at r1 (raw file):
Previously, oncilla (Dominik Roos) wrote…
💡 server names don't have ports 🤦
Should be good to go now.
Hm, big topologies are still failing. A bit too often to declare it a flake
There was a problem hiding this comment.
Reviewable status: 10 of 24 files reviewed, 2 unresolved discussions (waiting on @matzf)
pkg/snet/squic/net.go line 379 at r1 (raw file):
Previously, oncilla (Dominik Roos) wrote…
Hm, big topologies are still failing. A bit too often to declare it a flake
0/20 vs 8/20
https://buildkite.com/scionproto/scion/builds/3683
https://buildkite.com/scionproto/scion/builds/3681
There was a problem hiding this comment.
Reviewable status: 10 of 22 files reviewed, 2 unresolved discussions (waiting on @matzf)
pkg/snet/squic/net.go line 379 at r1 (raw file):
Previously, oncilla (Dominik Roos) wrote…
0/20 vs 8/20
https://buildkite.com/scionproto/scion/builds/3683
https://buildkite.com/scionproto/scion/builds/3681
Turns out receiving an unexpected error leaves the transport in a semi broken state.
I think I will file an issue with upstream to get their opinion on the matter.
Currently, it is hard for the client to detect it is in a broken state.
There was a problem hiding this comment.
Reviewed 2 of 7 files at r2, 9 of 10 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @oncilla)
Update the quic-go version to v0.36.3. This is the last version that does not require Go 1.20 yet.
We will upgrade the Go version in a follow up PR.
This change is