Skip to content
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

probe-cli: Update to quic-go v0.37.3 #1161

Merged
merged 20 commits into from
Sep 8, 2023

Conversation

kelmenhorst
Copy link
Contributor

@kelmenhorst kelmenhorst commented Jun 19, 2023

Checklist

Description

This diff updates our QUIC code, mainly in netxlite, to quic-go v0.35.1, including the new API introduced in v0.35.0.
The migration has been done according to ooni/probe#2492.

Major changes:

  • Replaced call to quic.DialEarlyContext with call to quic.DialEarly (netxlite/quic.go).
  • Removed now unnecessary address string in DialEarly calls (netxlite/quic.go).
  • Replaced quic.Listener with *quic.Listener (simplequicping_test.go).
  • Adapted signature of HandshakeComplete such that it returns a channel rather than a context (quic code in netxlite, measurex, mocks).
  • Renamed QUICListener to UDPListener.

Attention: Tests using a failing MockSyscallConn (e.g. in measurexlite) fail, due to issues with closing the quic.Transport quic-go.

Update (07.08.2023):
Upgraded to quic-go 0.37.3 with the following changes:

  • quic.ConnectionState.TLS points directly to a tls.ConnectionState instead of pointing to a qtls.ConnectionState instance. Background: Starting with Go 1.21, quic-go will use the QUIC support provided by the standard library's TLS implementation. Thus, quic-go will no longer fork crypto/tls anymore.
  • changed signature of quic.Connection's ReceiveMessage method

@bassosimone
Copy link
Contributor

@kelmenhorst there are several merge conflicts, could you make sure they are resolved? Thank you!

@bassosimone bassosimone changed the title probe-cli: Update to quic-go v0.35.0 probe-cli: Update to quic-go v0.37.3 Sep 7, 2023
Copy link
Contributor

@bassosimone bassosimone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐳

@bassosimone
Copy link
Contributor

bassosimone commented Sep 7, 2023

However, it seems we probably need to do more work either in this pull request or elsewhere.

Here's how tests fail for me locally:

2023/09/07 14:30:11 failed to increase receive buffer size (wanted: 2048 kiB, got 0 kiB). See https://github.com/quic-go/quic-go/wiki/UDP-Buffer-Sizes for details.
panic: connection already exists

goroutine 323 [running]:
github.com/quic-go/quic-go.(*connMultiplexer).AddConn(0x140004a2000, {0x12a80c530?, 0x140004c20d0?})
	/Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/multiplexer.go:59 +0x198
github.com/quic-go/quic-go.(*Transport).init.func1()
	/Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/transport.go:220 +0x370
sync.(*Once).doSlow(0x102baf460?, 0x10026dbb0?)
	/Users/sbs/sdk/go1.20.6/src/sync/once.go:74 +0x104
sync.(*Once).Do(...)
	/Users/sbs/sdk/go1.20.6/src/sync/once.go:65
github.com/quic-go/quic-go.(*Transport).init(0x1400051e0e0?, 0x40?)
	/Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/transport.go:186 +0x58
github.com/quic-go/quic-go.(*Transport).ListenEarly(0x1400051e0e0, 0x14000273b00, 0xb0?)
	/Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/transport.go:138 +0xfc
github.com/quic-go/quic-go.ListenEarly({0x12a84a4b8?, 0x140004c20d0}, 0x556f01?, 0x14000273b00?)
	/Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/server.go:218 +0x7c
github.com/quic-go/quic-go/http3.glob..func1({0x12a84a4b8?, 0x140004c20d0?}, 0x0?, 0xf4240?)
	/Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/http3/server.go:27 +0x24
github.com/quic-go/quic-go/http3.(*Server).serveConn(0x140009796b0, 0x14000178480, {0x12a84a4b8, 0x140004c20d0})
	/Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/http3/server.go:319 +0x140
github.com/quic-go/quic-go/http3.(*Server).Serve(0x14000090340?, {0x12a84a4b8?, 0x140004c20d0?})
	/Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/http3/server.go:245 +0x34
created by github.com/ooni/probe-cli/v3/internal/netemx.(*http3Server).mustListenPortLocked
	/Users/sbs/src/github.com/ooni/probe-cli/internal/netemx/http3.go:106 +0x194
FAIL	github.com/ooni/probe-cli/v3/internal/experiment/webconnectivityqa	1.060s

I need to figure out what's the issue here.

Here's how to reproduce by only running the package that fails:

% go test -count 1 -v ./internal/experiment/webconnectivityqa

=== RUN   TestBadSSLConditions
=== RUN   TestBadSSLConditions/badSSLWithUnknownAuthorityWithConsistentDNS
2023/09/07 14:35:27 failed to increase receive buffer size (wanted: 2048 kiB, got 0 kiB). See https://github.com/quic-go/quic-go/wiki/UDP-Buffer-Sizes for details.
=== RUN   TestBadSSLConditions/badSSLWithExpiredCertificate
panic: connection already exists

goroutine 305 [running]:
github.com/quic-go/quic-go.(*connMultiplexer).AddConn(0x14000526160, {0x12c6d8cb0?, 0x140006a8118?})
	/Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/multiplexer.go:59 +0x198
github.com/quic-go/quic-go.(*Transport).init.func1()
	/Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/transport.go:220 +0x370
sync.(*Once).doSlow(0x104bbb460?, 0x102279bb0?)
	/Users/sbs/sdk/go1.20.6/src/sync/once.go:74 +0x104
sync.(*Once).Do(...)
	/Users/sbs/sdk/go1.20.6/src/sync/once.go:65
github.com/quic-go/quic-go.(*Transport).init(0x140004460e0?, 0x50?)
	/Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/transport.go:186 +0x58
github.com/quic-go/quic-go.(*Transport).ListenEarly(0x140004460e0, 0x14000273b00, 0xb0?)
	/Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/transport.go:138 +0xfc
github.com/quic-go/quic-go.ListenEarly({0x12c8ebf98?, 0x140006a8118}, 0x3cdf01?, 0x14000273b00?)
	/Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/server.go:218 +0x7c
github.com/quic-go/quic-go/http3.glob..func1({0x12c8ebf98?, 0x140006a8118?}, 0x0?, 0xf4240?)
	/Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/http3/server.go:27 +0x24
github.com/quic-go/quic-go/http3.(*Server).serveConn(0x140006d0e70, 0x140006aac00, {0x12c8ebf98, 0x140006a8118})
	/Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/http3/server.go:319 +0x140
github.com/quic-go/quic-go/http3.(*Server).Serve(0x14000468b40?, {0x12c8ebf98?, 0x140006a8118?})
	/Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/http3/server.go:245 +0x34
created by github.com/ooni/probe-cli/v3/internal/netemx.(*http3Server).mustListenPortLocked
	/Users/sbs/src/github.com/ooni/probe-cli/internal/netemx/http3.go:106 +0x194
FAIL	github.com/ooni/probe-cli/v3/internal/experiment/webconnectivityqa	1.243s
FAIL

IIUC, there is some sort of "global" state here?

kelmenhorst added a commit that referenced this pull request Sep 8, 2023
This diff fixes a bug in the webconnectivityqa tests which we discovered due to the new quic-go API panicking (#1161 (comment)).

The bug was that we did not close the `QAEnv` in some of our webconnectivityqa tests.
Therefore, the QUIC Server would not close the underlying UDP socket
(`net.PacketConn`) and the local address of this socket would not be
removed from the QUIC multiplexer.
In the subsequent test case, the new quic-go API (see
#1161) would panic because the
local address of the server would still be registered in the global QUIC
multiplexer.
@bassosimone bassosimone merged commit 8d0db63 into ooni:master Sep 8, 2023
6 of 8 checks passed
Murphy-OrangeMud pushed a commit to Murphy-OrangeMud/probe-cli that referenced this pull request Feb 13, 2024
This diff fixes a bug in the webconnectivityqa tests which we discovered due to the new quic-go API panicking (ooni#1161 (comment)).

The bug was that we did not close the `QAEnv` in some of our webconnectivityqa tests.
Therefore, the QUIC Server would not close the underlying UDP socket
(`net.PacketConn`) and the local address of this socket would not be
removed from the QUIC multiplexer.
In the subsequent test case, the new quic-go API (see
ooni#1161) would panic because the
local address of the server would still be registered in the global QUIC
multiplexer.
Murphy-OrangeMud pushed a commit to Murphy-OrangeMud/probe-cli that referenced this pull request Feb 13, 2024
## Checklist

- [x] I have read the [contribution
guidelines](https://github.com/ooni/probe-cli/blob/master/CONTRIBUTING.md)

- [x] reference issue for this pull request:
ooni/probe#2492

## Description

This diff updates our QUIC code, mainly in netxlite, to quic-go v0.37.1,
including the new API introduced in v0.35.0.

The migration has been done according to ooni/probe#2492.

Major changes: 

* Replaced call to `quic.DialEarlyContext` with call to `quic.DialEarly` (netxlite/quic.go).

* Removed now unnecessary address string in `DialEarly` calls (netxlite/quic.go).

* Replaced `quic.Listener` with `*quic.Listener` (simplequicping_test.go).

* Adapted signature of `HandshakeComplete` such that it returns a
channel rather than a context (quic code in netxlite, measurex, mocks).

* Renamed `QUICListener` to `UDPListener`.

* `quic.ConnectionState.TLS` points directly to a `tls.ConnectionState`
instead of pointing to a `qtls.ConnectionState` instance. Background:
Starting with Go 1.21, quic-go will use the QUIC support provided by the
standard library's TLS implementation. Thus, quic-go will no longer fork
crypto/tls anymore.

* changed signature of `quic.Connection`'s `ReceiveMessage` method

---------

Co-authored-by: Simone Basso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants