fix(netemx): address issues with quic-go/quic-go (ooni#1228)

This diff fixes ooni/probe#2527. The comment I originally wrote in the code to explain the issue read: > I am wondering whether I am not fully understanding how quic-go/quic-go works. It turns out this comment was true. I was assuming http3.Server was closing the listener, however, that was not the case. I noticed this issue when reading again netemx/qaenv.go. So, it all boiled down to PEBCAK 😬. ## 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#2527 - [x] if you changed anything related to how experiments work and you need to reflect these changes in the ooni/spec repository, please link to the related ooni/spec pull request: N/A - [x] if you changed code inside an experiment, make sure you bump its version number: N/A
Murphy-OrangeMud · Feb 13, 2024 · 93c189d · 93c189d
1 parent 8cd3456
commit 93c189d
Show file tree

Hide file tree

Showing 8 changed files with 40 additions and 75 deletions.
diff --git a/internal/netemx/example_test.go b/internal/netemx/example_test.go
@@ -371,7 +371,7 @@ func Example_oohelperdWithInternetScenario() {
  })
 
  // Output:
- // {"tcp_connect":{"93.184.216.34:443":{"status":true,"failure":null}},"tls_handshake":{"93.184.216.34:443":{"server_name":"www.example.com","status":true,"failure":null}},"quic_handshake":{},"http_request":{"body_length":194,"discovered_h3_endpoint":"","failure":null,"title":"Default Web Page","headers":{"Content-Length":"194","Content-Type":"text/html; charset=utf-8","Date":"Thu, 24 Aug 2023 14:35:29 GMT"},"status_code":200},"http3_request":null,"dns":{"failure":null,"addrs":["93.184.216.34"]},"ip_info":{"93.184.216.34":{"asn":15133,"flags":11}}}
+ // {"tcp_connect":{"93.184.216.34:443":{"status":true,"failure":null}},"tls_handshake":{"93.184.216.34:443":{"server_name":"www.example.com","status":true,"failure":null}},"quic_handshake":{},"http_request":{"body_length":194,"discovered_h3_endpoint":"www.example.com:443","failure":null,"title":"Default Web Page","headers":{"Alt-Svc":"h3=\":443\"","Content-Length":"194","Content-Type":"text/html; charset=utf-8","Date":"Thu, 24 Aug 2023 14:35:29 GMT"},"status_code":200},"http3_request":null,"dns":{"failure":null,"addrs":["93.184.216.34"]},"ip_info":{"93.184.216.34":{"asn":15133,"flags":11}}}
 }
 
 // This example shows how the [InternetScenario] defines a GeoIP service like Ubuntu's one.

diff --git a/internal/netemx/http3.go b/internal/netemx/http3.go
@@ -103,7 +103,11 @@ func (srv *http3Server) mustListenPortLocked(handler http.Handler, ipAddr net.IP
  }
  go srvr.Serve(listener)
 
- // make sure we track the server (the .Serve method will close the
- // listener once we close the server itself)
+ // make sure we track and close the listener: assuming the server was closing the
+ // listener seems to be the root cause of https://github.com/ooni/probe/issues/2527
+ // and closing the listener completely fixes the issue.
+ srv.closers = append(srv.closers, listener)
+
+ // make sure we track the server
  srv.closers = append(srv.closers, srvr)
 }
diff --git a/internal/netemx/http3_test.go b/internal/netemx/http3_test.go
@@ -13,27 +13,8 @@ import (
 
 func TestHTTP3ServerFactory(t *testing.T) {
  t.Run("when using the TLSConfig provided by netem", func(t *testing.T) {
- /*
- __ ________________________
- / \ / \__ ___/\_ _____/
- \ \/\/ / | | | __)
- \ / | | | \
- \__/\ / |____| \___ /
- \/ \/
-
- I originally wrote this test to use AddressWwwExampleCom and the test
- failed with generic_timeout_error. Now, instead, if I change it to use
- 10.55.56.57, the test is working as intended. I am wondering whether
- I am not fully understanding how quic-go/quic-go works.
-
- My (limited?) understanding: just a single test can use AddressWwwExampleCom
- and, if I use it in other tests, there are issues leading to timeouts.
-
- See https://github.com/ooni/probe/issues/2527.
- */
-
  env := MustNewQAEnv(
- QAEnvOptionNetStack("10.55.56.57", &HTTP3ServerFactory{
+ QAEnvOptionNetStack(AddressWwwExampleCom, &HTTP3ServerFactory{
  Factory: HTTPHandlerFactoryFunc(func(_ *netem.UNetStack) http.Handler {
  return ExampleWebPageHandler()
  }),
@@ -43,7 +24,7 @@ func TestHTTP3ServerFactory(t *testing.T) {
  )
  defer env.Close()
 
- env.AddRecordToAllResolvers("www.example.com", "", "10.55.56.57")
+ env.AddRecordToAllResolvers("www.example.com", "", AddressWwwExampleCom)
 
  env.Do(func() {
  client := netxlite.NewHTTP3ClientWithResolver(log.Log, netxlite.NewStdlibResolver(log.Log))
@@ -67,31 +48,12 @@ func TestHTTP3ServerFactory(t *testing.T) {
  })
 
  t.Run("when using an incompatible TLS config", func(t *testing.T) {
- /*
- __ ________________________
- / \ / \__ ___/\_ _____/
- \ \/\/ / | | | __)
- \ / | | | \
- \__/\ / |____| \___ /
- \/ \/
-
- I originally wrote this test to use AddressWwwExampleCom and the test
- failed with generic_timeout_error. Now, instead, if I change it to use
- 10.55.56.100, the test is working as intended. I am wondering whether
- I am not fully understanding how quic-go/quic-go works.
-
- My (limited?) understanding: just a single test can use AddressWwwExampleCom
- and, if I use it in other tests, there are issues leading to timeouts.
-
- See https://github.com/ooni/probe/issues/2527.
- */
-
  // we're creating a distinct MITM TLS config and we're using it, so we expect
  // that we're not able to verify certificates in client code
  mitmConfig := runtimex.Try1(netem.NewTLSMITMConfig())
 
  env := MustNewQAEnv(
- QAEnvOptionNetStack("10.55.56.100", &HTTP3ServerFactory{
+ QAEnvOptionNetStack(AddressWwwExampleCom, &HTTP3ServerFactory{
  Factory: HTTPHandlerFactoryFunc(func(_ *netem.UNetStack) http.Handler {
  return ExampleWebPageHandler()
  }),
@@ -101,7 +63,7 @@ func TestHTTP3ServerFactory(t *testing.T) {
  )
  defer env.Close()
 
- env.AddRecordToAllResolvers("www.example.com", "", "10.55.56.100")
+ env.AddRecordToAllResolvers("www.example.com", "", AddressWwwExampleCom)
 
  env.Do(func() {
  client := netxlite.NewHTTP3ClientWithResolver(log.Log, netxlite.NewStdlibResolver(log.Log))

diff --git a/internal/netemx/oohelperd.go b/internal/netemx/oohelperd.go
@@ -33,8 +33,6 @@ func (f *OOHelperDFactory) NewHandler(unet *netem.UNetStack) http.Handler {
  return netx.NewDialerWithResolver(logger, netx.NewStdlibResolver(logger))
  }
 
- // hard to test because of https://github.com/ooni/probe/issues/2527, which
- // makes tests become flaky and fragile in an instant
  handler.NewQUICDialer = func(logger model.Logger) model.QUICDialer {
  return netx.NewQUICDialerWithResolver(
  netx.NewQUICListener(),
@@ -59,8 +57,6 @@ func (f *OOHelperDFactory) NewHandler(unet *netem.UNetStack) http.Handler {
  }
  }
 
- // hard to test because of https://github.com/ooni/probe/issues/2527, which
- // makes tests become flaky and fragile in an instant
  handler.NewHTTP3Client = func(logger model.Logger) model.HTTPClient {
  cookieJar, _ := cookiejar.New(&cookiejar.Options{
  PublicSuffixList: publicsuffix.List,

diff --git a/internal/netemx/oohelperd_test.go b/internal/netemx/oohelperd_test.go
@@ -74,20 +74,38 @@ func TestOOHelperDHandler(t *testing.T) {
  Failure: nil,
  },
  },
- QUICHandshake: map[string]model.THTLSHandshakeResult{},
+ QUICHandshake: map[string]model.THTLSHandshakeResult{
+ "93.184.216.34:443": {
+ ServerName: "www.example.com",
+ Status: true,
+ Failure: nil,
+ },
+ },
  HTTPRequest: model.THHTTPRequestResult{
  BodyLength: 194,
- DiscoveredH3Endpoint: "",
+ DiscoveredH3Endpoint: "www.example.com:443",
  Failure: nil,
  Title: "Default Web Page",
  Headers: map[string]string{
+ "Alt-Svc": `h3=":443"`,
  "Content-Length": "194",
  "Content-Type": "text/html; charset=utf-8",
  "Date": "Thu, 24 Aug 2023 14:35:29 GMT",
  },
  StatusCode: 200,
  },
- HTTP3Request: nil,
+ HTTP3Request: &model.THHTTPRequestResult{
+ BodyLength: 194,
+ DiscoveredH3Endpoint: "",
+ Failure: nil,
+ Title: "Default Web Page",
+ Headers: map[string]string{
+ "Alt-Svc": `h3=":443"`,
+ "Content-Type": "text/html; charset=utf-8",
+ "Date": "Thu, 24 Aug 2023 14:35:29 GMT",
+ },
+ StatusCode: 200,
+ },
  DNS: model.THDNSResult{
  Failure: nil,
  Addrs: []string{"93.184.216.34"},

diff --git a/internal/netemx/qaenv_test.go b/internal/netemx/qaenv_test.go
@@ -136,29 +136,10 @@ func TestQAEnv(t *testing.T) {
  // If all of this works, it means we're using the userspace TCP/IP
  // stack exported by the [Environment] struct.
  t.Run("we can hijack HTTP3 requests", func(t *testing.T) {
- /*
- __ ________________________
- / \ / \__ ___/\_ _____/
- \ \/\/ / | | | __)
- \ / | | | \
- \__/\ / |____| \___ /
- \/ \/
-
- I originally wrote this test to use AddressWwwExampleCom and the test
- failed with generic_timeout_error. Now, instead, if I change it to use
- 10.55.56.101, the test is working as intended. I am wondering whether
- I am not fully understanding how quic-go/quic-go works.
-
- My (limited?) understanding: just a single test can use AddressWwwExampleCom
- and, if I use it in other tests, there are issues leading to timeouts.
-
- See https://github.com/ooni/probe/issues/2527.
- */
-
  // create QA env
  env := netemx.MustNewQAEnv(
  netemx.QAEnvOptionHTTPServer(
- "10.55.56.101",
+ netemx.AddressWwwExampleCom,
  netemx.ExampleWebPageHandlerFactory(),
  ),
  )
@@ -168,7 +149,7 @@ func TestQAEnv(t *testing.T) {
  env.AddRecordToAllResolvers(
  "www.example.com",
  "", // CNAME
- "10.55.56.101",
+ netemx.AddressWwwExampleCom,
  )
 
  env.Do(func() {

diff --git a/internal/netemx/scenario.go b/internal/netemx/scenario.go
@@ -145,6 +145,10 @@ func MustNewScenario(config []*ScenarioDomainAddresses) *QAEnv {
  Factory: sad.WebServerFactory,
  Ports: []int{443},
  TLSConfig: nil, // use netem's default
+ }, &HTTP3ServerFactory{
+ Factory: sad.WebServerFactory,
+ Ports: []int{443},
+ TLSConfig: nil, // use netem's default
  }))
  }
 

diff --git a/internal/netemx/web.go b/internal/netemx/web.go
@@ -26,7 +26,7 @@ const ExampleWebPage = `<!doctype html>
 // is www.example.{com,org} and redirecting to www. when the domain is example.{com,org}.
 func ExampleWebPageHandler() http.Handler {
  return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
- //w.Header().Add("Alt-Svc", `h3=":443"`) // see https://github.com/ooni/probe/issues/2527
+ w.Header().Add("Alt-Svc", `h3=":443"`)
  w.Header().Add("Date", "Thu, 24 Aug 2023 14:35:29 GMT")
 
  // According to Go documentation, the host header is removed from the
@@ -87,7 +87,7 @@ const Blockpage = `<!doctype html>
 func BlockpageHandlerFactory() HTTPHandlerFactory {
  return HTTPHandlerFactoryFunc(func(_ *netem.UNetStack) http.Handler {
  return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
- //w.Header().Add("Alt-Svc", `h3=":443"`) // see https://github.com/ooni/probe/issues/2527
+ w.Header().Add("Alt-Svc", `h3=":443"`)
  w.Header().Add("Date", "Thu, 24 Aug 2023 14:35:29 GMT")
  w.Write([]byte(Blockpage))
  })