diff --git a/internal/netxlite/bogon.go b/internal/netxlite/bogon.go index 47a1c27..cbac027 100644 --- a/internal/netxlite/bogon.go +++ b/internal/netxlite/bogon.go @@ -18,7 +18,7 @@ import ( // function a non-IP address causes it to return true. func IsBogon(address string) bool { ip := net.ParseIP(address) - return ip == nil || isPrivate(address, ip) + return ip == nil || isBogon(address, ip) } // IsLoopback returns whether an IP address is loopback. Passing to this @@ -33,7 +33,7 @@ var ( bogons6 []*net.IPNet ) -func expandbogons(cidrs []string) (out []*net.IPNet) { +func expandBogons(cidrs []string) (out []*net.IPNet) { for _, cidr := range cidrs { _, block, err := net.ParseCIDR(cidr) runtimex.PanicOnError(err, "net.ParseCIDR failed") @@ -43,7 +43,7 @@ func expandbogons(cidrs []string) (out []*net.IPNet) { } func init() { - bogons4 = append(bogons4, expandbogons([]string{ + bogons4 = append(bogons4, expandBogons([]string{ // // List extracted from https://ipinfo.io/bogon // @@ -64,7 +64,7 @@ func init() { "240.0.0.0/4", // Reserved for future use "255.255.255.255/32", // Limited broadcast })...) - bogons6 = append(bogons6, expandbogons([]string{ + bogons6 = append(bogons6, expandBogons([]string{ // // List extracted from https://ipinfo.io/bogon // @@ -110,7 +110,10 @@ func init() { })...) } -func isPrivate(address string, ip net.IP) bool { +// isBogon implements IsBogon +func isBogon(address string, ip net.IP) bool { + // TODO(bassosimone): the following check is probably redundant given that these + // three checks are already included into the list of bogons. if ip.IsLoopback() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() { return true } diff --git a/internal/netxlite/classify.go b/internal/netxlite/classify.go index 6bb3d21..254ff61 100644 --- a/internal/netxlite/classify.go +++ b/internal/netxlite/classify.go @@ -1,5 +1,9 @@ package netxlite +// +// Mapping Go errors to OONI errors +// + import ( "context" "crypto/x509" @@ -38,8 +42,7 @@ func classifyGenericError(err error) string { // The list returned here matches the values used by MK unless // explicitly noted otherwise with a comment. - // QUIRK: we cannot remove this check as long as this function - // is exported and used independently from NewErrWrapper. + // Robustness: handle the case where we're passed a wrapped error. var errwrapper *ErrWrapper if errors.As(err, &errwrapper) { return errwrapper.Error() // we've already wrapped it @@ -146,8 +149,7 @@ const ( // and returns to the caller its return value. func classifyQUICHandshakeError(err error) string { - // QUIRK: we cannot remove this check as long as this function - // is exported and used independently from NewErrWrapper. + // Robustness: handle the case where we're passed a wrapped error. var errwrapper *ErrWrapper if errors.As(err, &errwrapper) { return errwrapper.Error() // we've already wrapped it @@ -269,8 +271,7 @@ var ( // returns to the caller its return value. func classifyResolverError(err error) string { - // QUIRK: we cannot remove this check as long as this function - // is exported and used independently from NewErrWrapper. + // Robustness: handle the case where we're passed a wrapped error. var errwrapper *ErrWrapper if errors.As(err, &errwrapper) { return errwrapper.Error() // we've already wrapped it @@ -303,8 +304,7 @@ func classifyResolverError(err error) string { // returns to the caller its return value. func classifyTLSHandshakeError(err error) string { - // QUIRK: we cannot remove this check as long as this function - // is exported and used independently from NewErrWrapper. + // Robustness: handle the case where we're passed a wrapped error. var errwrapper *ErrWrapper if errors.As(err, &errwrapper) { return errwrapper.Error() // we've already wrapped it diff --git a/internal/netxlite/dialer.go b/internal/netxlite/dialer.go index 4c1f754..43cade5 100644 --- a/internal/netxlite/dialer.go +++ b/internal/netxlite/dialer.go @@ -1,5 +1,9 @@ package netxlite +// +// Code for dialing TCP or UDP net.Conn-like connections +// + import ( "context" "errors" @@ -96,8 +100,8 @@ func (d *dialerSystem) CloseIdleConnections() { // dialerResolver combines dialing with domain name resolution. type dialerResolver struct { - model.Dialer - model.Resolver + Dialer model.Dialer + Resolver model.Resolver } var _ model.Dialer = &dialerResolver{} @@ -144,10 +148,10 @@ func (d *dialerResolver) CloseIdleConnections() { // dialerLogger is a Dialer with logging. type dialerLogger struct { // Dialer is the underlying dialer. - model.Dialer + Dialer model.Dialer - // Logger is the underlying logger. - model.DebugLogger + // DebugLogger is the underlying logger. + DebugLogger model.DebugLogger // operationSuffix is appended to the operation name. // @@ -194,15 +198,15 @@ func NewSingleUseDialer(conn net.Conn) model.Dialer { // dialerSingleUse is the Dialer returned by NewSingleDialer. type dialerSingleUse struct { - sync.Mutex + mu sync.Mutex conn net.Conn } var _ model.Dialer = &dialerSingleUse{} func (s *dialerSingleUse) DialContext(ctx context.Context, network string, addr string) (net.Conn, error) { - defer s.Unlock() - s.Lock() + defer s.mu.Unlock() + s.mu.Lock() if s.conn == nil { return nil, ErrNoConnReuse } @@ -218,7 +222,7 @@ func (s *dialerSingleUse) CloseIdleConnections() { // dialerErrWrapper is a dialer that performs error wrapping. The connection // returned by the DialContext function will also perform error wrapping. type dialerErrWrapper struct { - model.Dialer + Dialer model.Dialer } var _ model.Dialer = &dialerErrWrapper{} @@ -226,11 +230,15 @@ var _ model.Dialer = &dialerErrWrapper{} func (d *dialerErrWrapper) DialContext(ctx context.Context, network, address string) (net.Conn, error) { conn, err := d.Dialer.DialContext(ctx, network, address) if err != nil { - return nil, NewErrWrapper(classifyGenericError, ConnectOperation, err) + return nil, newErrWrapper(classifyGenericError, ConnectOperation, err) } return &dialerErrWrapperConn{Conn: conn}, nil } +func (d *dialerErrWrapper) CloseIdleConnections() { + d.Dialer.CloseIdleConnections() +} + // dialerErrWrapperConn is a net.Conn that performs error wrapping. type dialerErrWrapperConn struct { net.Conn @@ -241,7 +249,7 @@ var _ net.Conn = &dialerErrWrapperConn{} func (c *dialerErrWrapperConn) Read(b []byte) (int, error) { count, err := c.Conn.Read(b) if err != nil { - return 0, NewErrWrapper(classifyGenericError, ReadOperation, err) + return 0, newErrWrapper(classifyGenericError, ReadOperation, err) } return count, nil } @@ -249,7 +257,7 @@ func (c *dialerErrWrapperConn) Read(b []byte) (int, error) { func (c *dialerErrWrapperConn) Write(b []byte) (int, error) { count, err := c.Conn.Write(b) if err != nil { - return 0, NewErrWrapper(classifyGenericError, WriteOperation, err) + return 0, newErrWrapper(classifyGenericError, WriteOperation, err) } return count, nil } @@ -257,7 +265,7 @@ func (c *dialerErrWrapperConn) Write(b []byte) (int, error) { func (c *dialerErrWrapperConn) Close() error { err := c.Conn.Close() if err != nil { - return NewErrWrapper(classifyGenericError, CloseOperation, err) + return newErrWrapper(classifyGenericError, CloseOperation, err) } return nil } diff --git a/internal/netxlite/dialer_test.go b/internal/netxlite/dialer_test.go index 646e844..f421a1c 100644 --- a/internal/netxlite/dialer_test.go +++ b/internal/netxlite/dialer_test.go @@ -253,7 +253,7 @@ func TestDialerResolver(t *testing.T) { mu := &sync.Mutex{} errorsList := []error{ errors.New("a mocked error"), - NewErrWrapper( + newErrWrapper( classifyGenericError, CloseOperation, io.EOF, @@ -295,7 +295,7 @@ func TestDialerResolver(t *testing.T) { mu := &sync.Mutex{} errorsList := []error{ errors.New("a mocked error"), - NewErrWrapper( + newErrWrapper( classifyGenericError, CloseOperation, errors.New("antani"), diff --git a/internal/netxlite/dnsdecoder.go b/internal/netxlite/dnsdecoder.go index 22ef9fb..a557101 100644 --- a/internal/netxlite/dnsdecoder.go +++ b/internal/netxlite/dnsdecoder.go @@ -1,5 +1,9 @@ package netxlite +// +// Decode byte arrays to DNS messages +// + import ( "errors" diff --git a/internal/netxlite/dnsencoder.go b/internal/netxlite/dnsencoder.go index f5394be..91e3ba1 100644 --- a/internal/netxlite/dnsencoder.go +++ b/internal/netxlite/dnsencoder.go @@ -1,5 +1,9 @@ package netxlite +// +// Encode DNS queries to byte arrays +// + import ( "github.com/miekg/dns" "github.com/ooni/probe-cli/v3/internal/model" diff --git a/internal/netxlite/dnsoverhttps.go b/internal/netxlite/dnsoverhttps.go index 8c9cf49..7d1215a 100644 --- a/internal/netxlite/dnsoverhttps.go +++ b/internal/netxlite/dnsoverhttps.go @@ -1,5 +1,9 @@ package netxlite +// +// DNS-over-HTTPS transport +// + import ( "bytes" "context" diff --git a/internal/netxlite/dnsovertcp.go b/internal/netxlite/dnsovertcp.go index eeafe36..190b946 100644 --- a/internal/netxlite/dnsovertcp.go +++ b/internal/netxlite/dnsovertcp.go @@ -1,5 +1,9 @@ package netxlite +// +// DNS-over-{TCP,TLS} transport +// + import ( "context" "errors" diff --git a/internal/netxlite/dnsoverudp.go b/internal/netxlite/dnsoverudp.go index 8b336b0..4a79271 100644 --- a/internal/netxlite/dnsoverudp.go +++ b/internal/netxlite/dnsoverudp.go @@ -1,5 +1,9 @@ package netxlite +// +// DNS-over-UDP transport +// + import ( "context" "time" diff --git a/internal/netxlite/dnstransport.go b/internal/netxlite/dnstransport.go deleted file mode 100644 index 33539e6..0000000 --- a/internal/netxlite/dnstransport.go +++ /dev/null @@ -1 +0,0 @@ -package netxlite diff --git a/internal/netxlite/doc.go b/internal/netxlite/doc.go index c5d01ed..7a8f9cc 100644 --- a/internal/netxlite/doc.go +++ b/internal/netxlite/doc.go @@ -3,7 +3,7 @@ // This package is the basic networking building block that you // should be using every time you need networking. // -// It implements interfaces defined in the internal/model package. +// It implements interfaces defined in internal/model/netx.go. // // You should consider checking the tutorial explaining how to use this package // for network measurements: https://github.com/ooni/probe-cli/tree/master/internal/tutorial/netxlite. diff --git a/internal/netxlite/errwrapper.go b/internal/netxlite/errwrapper.go index 121ea2a..9af52c1 100644 --- a/internal/netxlite/errwrapper.go +++ b/internal/netxlite/errwrapper.go @@ -66,12 +66,12 @@ func (e *ErrWrapper) MarshalJSON() ([]byte, error) { return json.Marshal(e.Failure) } -// Classifier is the type of the function that maps a Go error +// classifier is the type of the function that maps a Go error // to a OONI failure string defined at // https://github.com/ooni/spec/blob/master/data-formats/df-007-errors.md. -type Classifier func(err error) string +type classifier func(err error) string -// NewErrWrapper creates a new ErrWrapper using the given +// newErrWrapper creates a new ErrWrapper using the given // classifier, operation name, and underlying error. // // This function panics if classifier is nil, or operation @@ -81,7 +81,7 @@ type Classifier func(err error) string // error wrapper will use the same classification string and // will determine whether to keep the major operation as documented // in the ErrWrapper.Operation documentation. -func NewErrWrapper(c Classifier, op string, err error) *ErrWrapper { +func newErrWrapper(c classifier, op string, err error) *ErrWrapper { var wrapper *ErrWrapper if errors.As(err, &wrapper) { return &ErrWrapper{ @@ -107,13 +107,15 @@ func NewErrWrapper(c Classifier, op string, err error) *ErrWrapper { } // NewTopLevelGenericErrWrapper wraps an error occurring at top -// level using ClassifyGenericError as classifier. +// level using a generic classifier as classifier. This is the +// function you should call when you suspect a given error hasn't +// already been wrapped. This function panics if err is nil. // // If the err argument has already been classified, the returned // error wrapper will use the same classification string and // failed operation of the original error. func NewTopLevelGenericErrWrapper(err error) *ErrWrapper { - return NewErrWrapper(classifyGenericError, TopLevelOperation, err) + return newErrWrapper(classifyGenericError, TopLevelOperation, err) } func classifyOperation(ew *ErrWrapper, operation string) string { diff --git a/internal/netxlite/errwrapper_test.go b/internal/netxlite/errwrapper_test.go index e4066d3..2960dcc 100644 --- a/internal/netxlite/errwrapper_test.go +++ b/internal/netxlite/errwrapper_test.go @@ -53,7 +53,7 @@ func TestNewErrWrapper(t *testing.T) { recovered.Add(1) } }() - NewErrWrapper(nil, CloseOperation, io.EOF) + newErrWrapper(nil, CloseOperation, io.EOF) }() if recovered.Load() != 1 { t.Fatal("did not panic") @@ -68,7 +68,7 @@ func TestNewErrWrapper(t *testing.T) { recovered.Add(1) } }() - NewErrWrapper(classifyGenericError, "", io.EOF) + newErrWrapper(classifyGenericError, "", io.EOF) }() if recovered.Load() != 1 { t.Fatal("did not panic") @@ -83,7 +83,7 @@ func TestNewErrWrapper(t *testing.T) { recovered.Add(1) } }() - NewErrWrapper(classifyGenericError, CloseOperation, nil) + newErrWrapper(classifyGenericError, CloseOperation, nil) }() if recovered.Load() != 1 { t.Fatal("did not panic") @@ -91,7 +91,7 @@ func TestNewErrWrapper(t *testing.T) { }) t.Run("otherwise, works as intended", func(t *testing.T) { - ew := NewErrWrapper(classifyGenericError, CloseOperation, io.EOF) + ew := newErrWrapper(classifyGenericError, CloseOperation, io.EOF) if ew.Failure != FailureEOFError { t.Fatal("unexpected failure") } @@ -104,10 +104,10 @@ func TestNewErrWrapper(t *testing.T) { }) t.Run("when the underlying error is already a wrapped error", func(t *testing.T) { - ew := NewErrWrapper(classifySyscallError, ReadOperation, ECONNRESET) + ew := newErrWrapper(classifySyscallError, ReadOperation, ECONNRESET) var err1 error = ew err2 := fmt.Errorf("cannot read: %w", err1) - ew2 := NewErrWrapper(classifyGenericError, HTTPRoundTripOperation, err2) + ew2 := newErrWrapper(classifyGenericError, HTTPRoundTripOperation, err2) if ew2.Failure != ew.Failure { t.Fatal("not the same failure") } diff --git a/internal/netxlite/http.go b/internal/netxlite/http.go index 1f3fe2c..d553fa7 100644 --- a/internal/netxlite/http.go +++ b/internal/netxlite/http.go @@ -1,5 +1,9 @@ package netxlite +// +// HTTP/1.1 and HTTP2 code +// + import ( "context" "errors" @@ -13,9 +17,11 @@ import ( // httpTransportErrWrapper is an HTTPTransport with error wrapping. type httpTransportErrWrapper struct { - model.HTTPTransport + HTTPTransport model.HTTPTransport } +var _ model.HTTPTransport = &httpTransportErrWrapper{} + func (txp *httpTransportErrWrapper) RoundTrip(req *http.Request) (*http.Response, error) { resp, err := txp.HTTPTransport.RoundTrip(req) if err != nil { @@ -24,10 +30,18 @@ func (txp *httpTransportErrWrapper) RoundTrip(req *http.Request) (*http.Response return resp, nil } +func (txp *httpTransportErrWrapper) CloseIdleConnections() { + txp.HTTPTransport.CloseIdleConnections() +} + +func (txp *httpTransportErrWrapper) Network() string { + return txp.HTTPTransport.Network() +} + // httpTransportLogger is an HTTPTransport with logging. type httpTransportLogger struct { // HTTPTransport is the underlying HTTP transport. - model.HTTPTransport + HTTPTransport model.HTTPTransport // Logger is the underlying logger. Logger model.DebugLogger @@ -62,12 +76,26 @@ func (txp *httpTransportLogger) CloseIdleConnections() { txp.HTTPTransport.CloseIdleConnections() } +func (txp *httpTransportLogger) Network() string { + return txp.HTTPTransport.Network() +} + // httpTransportConnectionsCloser is an HTTPTransport that // correctly forwards CloseIdleConnections calls. type httpTransportConnectionsCloser struct { - model.HTTPTransport - model.Dialer - model.TLSDialer + HTTPTransport model.HTTPTransport + Dialer model.Dialer + TLSDialer model.TLSDialer +} + +var _ model.HTTPTransport = &httpTransportConnectionsCloser{} + +func (txp *httpTransportConnectionsCloser) RoundTrip(req *http.Request) (*http.Response, error) { + return txp.HTTPTransport.RoundTrip(req) +} + +func (txp *httpTransportConnectionsCloser) Network() string { + return txp.HTTPTransport.Network() } // CloseIdleConnections forwards the CloseIdleConnections calls. @@ -148,7 +176,17 @@ func NewOOHTTPBaseTransport(dialer model.Dialer, tlsDialer model.TLSDialer) mode // stdlibTransport wraps oohttp.StdlibTransport to add .Network() type stdlibTransport struct { - *oohttp.StdlibTransport + StdlibTransport *oohttp.StdlibTransport +} + +var _ model.HTTPTransport = &stdlibTransport{} + +func (txp *stdlibTransport) CloseIdleConnections() { + txp.StdlibTransport.CloseIdleConnections() +} + +func (txp *stdlibTransport) RoundTrip(req *http.Request) (*http.Response, error) { + return txp.StdlibTransport.RoundTrip(req) } // Network implements HTTPTransport.Network. @@ -170,7 +208,13 @@ func WrapHTTPTransport(logger model.DebugLogger, txp model.HTTPTransport) model. // httpDialerWithReadTimeout enforces a read timeout for all HTTP // connections. See https://github.com/ooni/probe/issues/1609. type httpDialerWithReadTimeout struct { - model.Dialer + Dialer model.Dialer +} + +var _ model.Dialer = &httpDialerWithReadTimeout{} + +func (d *httpDialerWithReadTimeout) CloseIdleConnections() { + d.Dialer.CloseIdleConnections() } // DialContext implements Dialer.DialContext. @@ -186,7 +230,13 @@ func (d *httpDialerWithReadTimeout) DialContext( // httpTLSDialerWithReadTimeout enforces a read timeout for all HTTP // connections. See https://github.com/ooni/probe/issues/1609. type httpTLSDialerWithReadTimeout struct { - model.TLSDialer + TLSDialer model.TLSDialer +} + +var _ model.TLSDialer = &httpTLSDialerWithReadTimeout{} + +func (d *httpTLSDialerWithReadTimeout) CloseIdleConnections() { + d.TLSDialer.CloseIdleConnections() } // ErrNotTLSConn occur when an interface accepts a net.Conn but @@ -280,7 +330,7 @@ func WrapHTTPClient(clnt model.HTTPClient) model.HTTPClient { } type httpClientErrWrapper struct { - model.HTTPClient + HTTPClient model.HTTPClient } func (c *httpClientErrWrapper) Do(req *http.Request) (*http.Response, error) { @@ -290,3 +340,7 @@ func (c *httpClientErrWrapper) Do(req *http.Request) (*http.Response, error) { } return resp, nil } + +func (c *httpClientErrWrapper) CloseIdleConnections() { + c.HTTPClient.CloseIdleConnections() +} diff --git a/internal/netxlite/http3.go b/internal/netxlite/http3.go index 49cec12..0077f3a 100644 --- a/internal/netxlite/http3.go +++ b/internal/netxlite/http3.go @@ -1,5 +1,9 @@ package netxlite +// +// HTTP3 code +// + import ( "crypto/tls" "io" diff --git a/internal/netxlite/http_test.go b/internal/netxlite/http_test.go index e45cb01..873c6d1 100644 --- a/internal/netxlite/http_test.go +++ b/internal/netxlite/http_test.go @@ -252,19 +252,19 @@ func TestNewHTTPTransport(t *testing.T) { t.Fatal("invalid tls dialer") } stdlib := connectionsCloser.HTTPTransport.(*stdlibTransport) - if !stdlib.Transport.ForceAttemptHTTP2 { + if !stdlib.StdlibTransport.ForceAttemptHTTP2 { t.Fatal("invalid ForceAttemptHTTP2") } - if !stdlib.Transport.DisableCompression { + if !stdlib.StdlibTransport.DisableCompression { t.Fatal("invalid DisableCompression") } - if stdlib.Transport.MaxConnsPerHost != 1 { + if stdlib.StdlibTransport.MaxConnsPerHost != 1 { t.Fatal("invalid MaxConnPerHost") } - if stdlib.Transport.DialTLSContext == nil { + if stdlib.StdlibTransport.DialTLSContext == nil { t.Fatal("invalid DialTLSContext") } - if stdlib.Transport.DialContext == nil { + if stdlib.StdlibTransport.DialContext == nil { t.Fatal("invalid DialContext") } }) @@ -500,6 +500,20 @@ func TestHTTPClientErrWrapper(t *testing.T) { } }) }) + + t.Run("CloseIdleConnections", func(t *testing.T) { + var called bool + child := &mocks.HTTPClient{ + MockCloseIdleConnections: func() { + called = true + }, + } + clnt := &httpClientErrWrapper{child} + clnt.CloseIdleConnections() + if !called { + t.Fatal("not called") + } + }) } func TestNewHTTPClientStdlib(t *testing.T) { diff --git a/internal/netxlite/iox.go b/internal/netxlite/iox.go index 1ae617a..04dde36 100644 --- a/internal/netxlite/iox.go +++ b/internal/netxlite/iox.go @@ -1,5 +1,9 @@ package netxlite +// +// I/O extensions +// + import ( "context" "errors" diff --git a/internal/netxlite/iox_test.go b/internal/netxlite/iox_test.go index ebf7e2b..268b8dd 100644 --- a/internal/netxlite/iox_test.go +++ b/internal/netxlite/iox_test.go @@ -41,7 +41,7 @@ func TestReadAllContext(t *testing.T) { // // Note: Returning a wrapped error to ensure we address // https://github.com/ooni/probe/issues/1965 - return len(b), NewErrWrapper(classifyGenericError, + return len(b), newErrWrapper(classifyGenericError, ReadOperation, io.EOF) }, } @@ -171,7 +171,7 @@ func TestCopyContext(t *testing.T) { // // Note: Returning a wrapped error to ensure we address // https://github.com/ooni/probe/issues/1965 - return len(b), NewErrWrapper(classifyGenericError, + return len(b), newErrWrapper(classifyGenericError, ReadOperation, io.EOF) }, } diff --git a/internal/netxlite/legacy.go b/internal/netxlite/legacy.go index d213220..8d72f97 100644 --- a/internal/netxlite/legacy.go +++ b/internal/netxlite/legacy.go @@ -1,5 +1,9 @@ package netxlite +// +// Legacy code +// + // These vars export internal names to legacy ooni/probe-cli code. // // Deprecated: do not use these names in new code. diff --git a/internal/netxlite/operations.go b/internal/netxlite/operations.go index e675074..773659b 100644 --- a/internal/netxlite/operations.go +++ b/internal/netxlite/operations.go @@ -1,5 +1,9 @@ package netxlite +// +// Names of operations +// + // Operations that we measure. They are the possible values of // the ErrWrapper.Operation field. const ( diff --git a/internal/netxlite/parallelresolver.go b/internal/netxlite/parallelresolver.go index bf0e2cc..4b80eb8 100644 --- a/internal/netxlite/parallelresolver.go +++ b/internal/netxlite/parallelresolver.go @@ -1,7 +1,7 @@ package netxlite // -// Parallel resolver implementation +// Parallel DNS resolver implementation // import ( diff --git a/internal/netxlite/quic.go b/internal/netxlite/quic.go index 86a44e6..8bfe027 100644 --- a/internal/netxlite/quic.go +++ b/internal/netxlite/quic.go @@ -1,5 +1,9 @@ package netxlite +// +// QUIC implementation +// + import ( "context" "crypto/tls" @@ -340,7 +344,7 @@ func NewSingleUseQUICDialer(qconn quic.EarlyConnection) model.QUICDialer { // quicDialerSingleUse is the QUICDialer returned by NewSingleQUICDialer. type quicDialerSingleUse struct { - sync.Mutex + mu sync.Mutex qconn quic.EarlyConnection } @@ -351,8 +355,8 @@ func (s *quicDialerSingleUse) DialContext( ctx context.Context, network, addr string, tlsCfg *tls.Config, cfg *quic.Config) (quic.EarlyConnection, error) { var qconn quic.EarlyConnection - defer s.Unlock() - s.Lock() + defer s.mu.Unlock() + s.mu.Lock() if s.qconn == nil { return nil, ErrNoConnReuse } @@ -368,7 +372,7 @@ func (s *quicDialerSingleUse) CloseIdleConnections() { // quicListenerErrWrapper is a QUICListener that wraps errors. type quicListenerErrWrapper struct { // QUICListener is the underlying listener. - model.QUICListener + QUICListener model.QUICListener } var _ model.QUICListener = &quicListenerErrWrapper{} @@ -377,7 +381,7 @@ var _ model.QUICListener = &quicListenerErrWrapper{} func (qls *quicListenerErrWrapper) Listen(addr *net.UDPAddr) (model.UDPLikeConn, error) { pconn, err := qls.QUICListener.Listen(addr) if err != nil { - return nil, NewErrWrapper(classifyGenericError, QUICListenOperation, err) + return nil, newErrWrapper(classifyGenericError, QUICListenOperation, err) } return &quicErrWrapperUDPLikeConn{pconn}, nil } @@ -394,7 +398,7 @@ var _ model.UDPLikeConn = &quicErrWrapperUDPLikeConn{} func (c *quicErrWrapperUDPLikeConn) WriteTo(p []byte, addr net.Addr) (int, error) { count, err := c.UDPLikeConn.WriteTo(p, addr) if err != nil { - return 0, NewErrWrapper(classifyGenericError, WriteToOperation, err) + return 0, newErrWrapper(classifyGenericError, WriteToOperation, err) } return count, nil } @@ -403,7 +407,7 @@ func (c *quicErrWrapperUDPLikeConn) WriteTo(p []byte, addr net.Addr) (int, error func (c *quicErrWrapperUDPLikeConn) ReadFrom(b []byte) (int, net.Addr, error) { n, addr, err := c.UDPLikeConn.ReadFrom(b) if err != nil { - return 0, nil, NewErrWrapper(classifyGenericError, ReadFromOperation, err) + return 0, nil, newErrWrapper(classifyGenericError, ReadFromOperation, err) } return n, addr, nil } @@ -412,24 +416,30 @@ func (c *quicErrWrapperUDPLikeConn) ReadFrom(b []byte) (int, net.Addr, error) { func (c *quicErrWrapperUDPLikeConn) Close() error { err := c.UDPLikeConn.Close() if err != nil { - return NewErrWrapper(classifyGenericError, ReadFromOperation, err) + return newErrWrapper(classifyGenericError, ReadFromOperation, err) } return nil } // quicDialerErrWrapper is a dialer that performs quic err wrapping type quicDialerErrWrapper struct { - model.QUICDialer + QUICDialer model.QUICDialer } +var _ model.QUICDialer = &quicDialerErrWrapper{} + // DialContext implements ContextDialer.DialContext func (d *quicDialerErrWrapper) DialContext( ctx context.Context, network string, host string, tlsCfg *tls.Config, cfg *quic.Config) (quic.EarlyConnection, error) { qconn, err := d.QUICDialer.DialContext(ctx, network, host, tlsCfg, cfg) if err != nil { - return nil, NewErrWrapper( + return nil, newErrWrapper( classifyQUICHandshakeError, QUICHandshakeOperation, err) } return qconn, nil } + +func (d *quicDialerErrWrapper) CloseIdleConnections() { + d.QUICDialer.CloseIdleConnections() +} diff --git a/internal/netxlite/quirks.go b/internal/netxlite/quirks.go index 72d5fa8..3989a33 100644 --- a/internal/netxlite/quirks.go +++ b/internal/netxlite/quirks.go @@ -1,15 +1,17 @@ package netxlite +// +// This file contains weird stuff that we carried over from +// the original netx implementation and that we cannot remove +// or change without thinking about the consequences. +// + import ( "errors" "net" "strings" ) -// This file contains weird stuff that we carried over from -// the original netx implementation and that we cannot remove -// or change without thinking about the consequences. - // See https://github.com/ooni/probe/issues/1985 var errReduceErrorsEmptyList = errors.New("bug: reduceErrors given an empty list") diff --git a/internal/netxlite/quirks_test.go b/internal/netxlite/quirks_test.go index de62260..298d640 100644 --- a/internal/netxlite/quirks_test.go +++ b/internal/netxlite/quirks_test.go @@ -34,12 +34,12 @@ func TestQuirkReduceErrors(t *testing.T) { t.Run("multiple errors with meaningful ones", func(t *testing.T) { err1 := errors.New("mocked error #1") - err2 := NewErrWrapper( + err2 := newErrWrapper( classifyGenericError, CloseOperation, errors.New("antani"), ) - err3 := NewErrWrapper( + err3 := newErrWrapper( classifyGenericError, CloseOperation, ECONNREFUSED, diff --git a/internal/netxlite/resolver.go b/internal/netxlite/resolver.go index c81b24f..9a201b5 100644 --- a/internal/netxlite/resolver.go +++ b/internal/netxlite/resolver.go @@ -1,5 +1,9 @@ package netxlite +// +// DNS resolver +// + import ( "context" "errors" @@ -134,8 +138,8 @@ func (r *resolverSystem) LookupHTTPS( // resolverLogger is a resolver that emits events type resolverLogger struct { - model.Resolver - Logger model.DebugLogger + Resolver model.Resolver + Logger model.DebugLogger } var _ model.Resolver = &resolverLogger{} @@ -172,13 +176,27 @@ func (r *resolverLogger) LookupHTTPS( return https, nil } +func (r *resolverLogger) Address() string { + return r.Resolver.Address() +} + +func (r *resolverLogger) Network() string { + return r.Resolver.Network() +} + +func (r *resolverLogger) CloseIdleConnections() { + r.Resolver.CloseIdleConnections() +} + // resolverIDNA supports resolving Internationalized Domain Names. // // See RFC3492 for more information. type resolverIDNA struct { - model.Resolver + Resolver model.Resolver } +var _ model.Resolver = &resolverIDNA{} + func (r *resolverIDNA) LookupHost(ctx context.Context, hostname string) ([]string, error) { host, err := idna.ToASCII(hostname) if err != nil { @@ -196,12 +214,26 @@ func (r *resolverIDNA) LookupHTTPS( return r.Resolver.LookupHTTPS(ctx, host) } +func (r *resolverIDNA) Network() string { + return r.Resolver.Network() +} + +func (r *resolverIDNA) Address() string { + return r.Resolver.Address() +} + +func (r *resolverIDNA) CloseIdleConnections() { + r.Resolver.CloseIdleConnections() +} + // resolverShortCircuitIPAddr recognizes when the input hostname is an // IP address and returns it immediately to the caller. type resolverShortCircuitIPAddr struct { - model.Resolver + Resolver model.Resolver } +var _ model.Resolver = &resolverShortCircuitIPAddr{} + func (r *resolverShortCircuitIPAddr) LookupHost(ctx context.Context, hostname string) ([]string, error) { if net.ParseIP(hostname) != nil { return []string{hostname}, nil @@ -222,6 +254,18 @@ func (r *resolverShortCircuitIPAddr) LookupHTTPS(ctx context.Context, hostname s return r.Resolver.LookupHTTPS(ctx, hostname) } +func (r *resolverShortCircuitIPAddr) Network() string { + return r.Resolver.Network() +} + +func (r *resolverShortCircuitIPAddr) Address() string { + return r.Resolver.Address() +} + +func (r *resolverShortCircuitIPAddr) CloseIdleConnections() { + r.Resolver.CloseIdleConnections() +} + // IsIPv6 returns true if the given candidate is a valid IP address // representation and such representation is IPv6. func IsIPv6(candidate string) (bool, error) { @@ -271,7 +315,7 @@ func (r *nullResolver) LookupHTTPS( // resolverErrWrapper is a Resolver that knows about wrapping errors. type resolverErrWrapper struct { - model.Resolver + Resolver model.Resolver } var _ model.Resolver = &resolverErrWrapper{} @@ -279,7 +323,7 @@ var _ model.Resolver = &resolverErrWrapper{} func (r *resolverErrWrapper) LookupHost(ctx context.Context, hostname string) ([]string, error) { addrs, err := r.Resolver.LookupHost(ctx, hostname) if err != nil { - return nil, NewErrWrapper(classifyResolverError, ResolveOperation, err) + return nil, newErrWrapper(classifyResolverError, ResolveOperation, err) } return addrs, nil } @@ -288,7 +332,19 @@ func (r *resolverErrWrapper) LookupHTTPS( ctx context.Context, domain string) (*model.HTTPSSvc, error) { out, err := r.Resolver.LookupHTTPS(ctx, domain) if err != nil { - return nil, NewErrWrapper(classifyResolverError, ResolveOperation, err) + return nil, newErrWrapper(classifyResolverError, ResolveOperation, err) } return out, nil } + +func (r *resolverErrWrapper) Network() string { + return r.Resolver.Network() +} + +func (r *resolverErrWrapper) Address() string { + return r.Resolver.Address() +} + +func (r *resolverErrWrapper) CloseIdleConnections() { + r.Resolver.CloseIdleConnections() +} diff --git a/internal/netxlite/resolver_test.go b/internal/netxlite/resolver_test.go index e60ea26..6405aae 100644 --- a/internal/netxlite/resolver_test.go +++ b/internal/netxlite/resolver_test.go @@ -400,6 +400,30 @@ func TestResolverIDNA(t *testing.T) { } }) }) + + t.Run("Network", func(t *testing.T) { + child := &mocks.Resolver{ + MockNetwork: func() string { + return "x" + }, + } + r := &resolverIDNA{child} + if r.Network() != "x" { + t.Fatal("invalid network") + } + }) + + t.Run("Address", func(t *testing.T) { + child := &mocks.Resolver{ + MockAddress: func() string { + return "x" + }, + } + r := &resolverIDNA{child} + if r.Address() != "x" { + t.Fatal("invalid address") + } + }) } func TestResolverShortCircuitIPAddr(t *testing.T) { diff --git a/internal/netxlite/serialresolver.go b/internal/netxlite/serialresolver.go index 66ab153..be1f4bb 100644 --- a/internal/netxlite/serialresolver.go +++ b/internal/netxlite/serialresolver.go @@ -1,7 +1,7 @@ package netxlite // -// Serial resolver implementation +// Serial DNS resolver implementation // import ( @@ -20,7 +20,11 @@ import ( // // You should probably use NewSerialResolver to create a new instance. // -// Deprecated: please use ParallelResolver in new code. +// Deprecated: please use ParallelResolver in new code. We cannot +// remove this code as long as we use tracing for measuring. +// +// QUIRK: unlike the ParallelResolver, this resolver retries each +// query three times for soft errors. type SerialResolver struct { // Encoder is the MANDATORY encoder to use. Encoder model.DNSEncoder @@ -98,6 +102,8 @@ func (r *SerialResolver) LookupHTTPS( func (r *SerialResolver) lookupHostWithRetry( ctx context.Context, hostname string, qtype uint16) ([]string, error) { + // QUIRK: retrying has been there since the beginning so we need to + // keep it as long as we're using tracing for measuring. var errorslist []error for i := 0; i < 3; i++ { replies, err := r.lookupHostWithoutRetry(ctx, hostname, qtype) @@ -116,9 +122,9 @@ func (r *SerialResolver) lookupHostWithRetry( } r.NumTimeouts.Add(1) } - // bugfix: we MUST return one of the errors otherwise we confuse the + // QUIRK: we MUST return one of the errors otherwise we confuse the // mechanism in errwrap that classifies the root cause operation, since - // it would not be able to find a child with a major operation error + // it would not be able to find a child with a major operation error. return nil, errorslist[0] } diff --git a/internal/netxlite/tls.go b/internal/netxlite/tls.go index f0a2fa5..7984587 100644 --- a/internal/netxlite/tls.go +++ b/internal/netxlite/tls.go @@ -1,5 +1,9 @@ package netxlite +// +// TLS implementation +// + import ( "context" "crypto/tls" @@ -11,8 +15,12 @@ import ( oohttp "github.com/ooni/oohttp" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/runtimex" ) +// TODO(bassosimone): check whether there's now equivalent functionality +// inside the standard library allowing us to map numbers to names. + var ( tlsVersionString = map[uint16]string{ tls.VersionTLS10: "TLSv1", @@ -81,7 +89,8 @@ func NewDefaultCertPool() *x509.CertPool { pool := x509.NewCertPool() // Assumption: AppendCertsFromPEM cannot fail because we // have a test in certify_test.go that guarantees that - pool.AppendCertsFromPEM([]byte(pemcerts)) + ok := pool.AppendCertsFromPEM([]byte(pemcerts)) + runtimex.PanicIfFalse(ok, "pool.AppendCertsFromPEM failed") return pool } @@ -201,8 +210,8 @@ var defaultTLSHandshaker = &tlsHandshakerConfigurable{} // tlsHandshakerLogger is a TLSHandshaker with logging. type tlsHandshakerLogger struct { - model.TLSHandshaker - model.DebugLogger + TLSHandshaker model.TLSHandshaker + DebugLogger model.DebugLogger } var _ model.TLSHandshaker = &tlsHandshakerLogger{} @@ -313,7 +322,7 @@ func NewSingleUseTLSDialer(conn TLSConn) model.TLSDialer { // tlsDialerSingleUseAdapter adapts dialerSingleUse to // be a TLSDialer type rather than a Dialer type. type tlsDialerSingleUseAdapter struct { - model.Dialer + Dialer model.Dialer } var _ model.TLSDialer = &tlsDialerSingleUseAdapter{} @@ -323,9 +332,13 @@ func (d *tlsDialerSingleUseAdapter) DialTLSContext(ctx context.Context, network, return d.Dialer.DialContext(ctx, network, address) } +func (d *tlsDialerSingleUseAdapter) CloseIdleConnections() { + d.Dialer.CloseIdleConnections() +} + // tlsHandshakerErrWrapper wraps the returned error to be an OONI error type tlsHandshakerErrWrapper struct { - model.TLSHandshaker + TLSHandshaker model.TLSHandshaker } // Handshake implements TLSHandshaker.Handshake @@ -334,7 +347,7 @@ func (h *tlsHandshakerErrWrapper) Handshake( ) (net.Conn, tls.ConnectionState, error) { tlsconn, state, err := h.TLSHandshaker.Handshake(ctx, conn, config) if err != nil { - return nil, tls.ConnectionState{}, NewErrWrapper( + return nil, tls.ConnectionState{}, newErrWrapper( classifyTLSHandshakeError, TLSHandshakeOperation, err) } return tlsconn, state, nil diff --git a/internal/netxlite/tls_test.go b/internal/netxlite/tls_test.go index 2c17987..1fc8c47 100644 --- a/internal/netxlite/tls_test.go +++ b/internal/netxlite/tls_test.go @@ -487,6 +487,7 @@ func TestTLSDialer(t *testing.T) { func TestNewSingleUseTLSDialer(t *testing.T) { conn := &mocks.TLSConn{} d := NewSingleUseTLSDialer(conn) + defer d.CloseIdleConnections() outconn, err := d.DialTLSContext(context.Background(), "", "") if err != nil { t.Fatal(err) diff --git a/internal/netxlite/tproxy.go b/internal/netxlite/tproxy.go index f5cbf1b..db366f1 100644 --- a/internal/netxlite/tproxy.go +++ b/internal/netxlite/tproxy.go @@ -1,5 +1,9 @@ package netxlite +// +// Transparent proxy (for integration testing) +// + import ( "context" "net" diff --git a/internal/netxlite/utls.go b/internal/netxlite/utls.go index 5d668e2..a4ddc57 100644 --- a/internal/netxlite/utls.go +++ b/internal/netxlite/utls.go @@ -1,5 +1,9 @@ package netxlite +// +// Code to use yawning/utls or refraction-networking/utls +// + import ( "context" "crypto/tls"