diff --git a/internal/measurex/http.go b/internal/measurex/http.go index 872449e..9d0e4d9 100644 --- a/internal/measurex/http.go +++ b/internal/measurex/http.go @@ -244,7 +244,7 @@ var ErrHTTPTooManyRedirects = errors.New("stopped after 10 redirects") func newHTTPClient(db WritableDB, cookiejar http.CookieJar, txp HTTPTransport, defaultErr error) HTTPClient { - return &httpClientErrWrapper{&http.Client{ + return netxlite.WrapHTTPClient(&http.Client{ Transport: txp, Jar: cookiejar, CheckRedirect: func(req *http.Request, via []*http.Request) error { @@ -260,19 +260,7 @@ func newHTTPClient(db WritableDB, cookiejar http.CookieJar, }) return err }, - }} -} - -type httpClientErrWrapper struct { - HTTPClient -} - -func (c *httpClientErrWrapper) Do(req *http.Request) (*http.Response, error) { - resp, err := c.HTTPClient.Do(req) - if err != nil { - err = netxlite.NewTopLevelGenericErrWrapper(err) - } - return resp, err + }) } // NewCookieJar is a convenience factory for creating an http.CookieJar diff --git a/internal/netxlite/classify.go b/internal/netxlite/classify.go index 56a4f4d..77dae38 100644 --- a/internal/netxlite/classify.go +++ b/internal/netxlite/classify.go @@ -38,6 +38,8 @@ 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. var errwrapper *ErrWrapper if errors.As(err, &errwrapper) { return errwrapper.Error() // we've already wrapped it @@ -143,6 +145,9 @@ const ( // If this classifier fails, it calls ClassifyGenericError // 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. var errwrapper *ErrWrapper if errors.As(err, &errwrapper) { return errwrapper.Error() // we've already wrapped it @@ -257,10 +262,14 @@ var ( // If this classifier fails, it calls ClassifyGenericError and // 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. var errwrapper *ErrWrapper if errors.As(err, &errwrapper) { return errwrapper.Error() // we've already wrapped it } + if errors.Is(err, ErrDNSBogon) { return FailureDNSBogonError // not in MK } @@ -281,10 +290,14 @@ func ClassifyResolverError(err error) string { // If this classifier fails, it calls ClassifyGenericError and // 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. var errwrapper *ErrWrapper if errors.As(err, &errwrapper) { return errwrapper.Error() // we've already wrapped it } + var x509HostnameError x509.HostnameError if errors.As(err, &x509HostnameError) { // Test case: https://wrong.host.badssl.com/ diff --git a/internal/netxlite/dnsoverhttps.go b/internal/netxlite/dnsoverhttps.go index 70bacfc..e9caa27 100644 --- a/internal/netxlite/dnsoverhttps.go +++ b/internal/netxlite/dnsoverhttps.go @@ -10,12 +10,6 @@ import ( "github.com/ooni/probe-cli/v3/internal/engine/httpheader" ) -// HTTPClient is an http.Client-like interface. -type HTTPClient interface { - Do(req *http.Request) (*http.Response, error) - CloseIdleConnections() -} - // DNSOverHTTPS is a DNS-over-HTTPS DNSTransport. type DNSOverHTTPS struct { // Client is the MANDATORY http client to use. diff --git a/internal/netxlite/errwrapper.go b/internal/netxlite/errwrapper.go index 9d604f8..c618399 100644 --- a/internal/netxlite/errwrapper.go +++ b/internal/netxlite/errwrapper.go @@ -1,6 +1,9 @@ package netxlite -import "encoding/json" +import ( + "encoding/json" + "errors" +) // ErrWrapper is our error wrapper for Go errors. The key objective of // this structure is to properly set Failure, which is also returned by @@ -78,7 +81,19 @@ type Classifier func(err error) string // // This function panics if classifier is nil, or operation // is the empty string or error 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 wrapped error. func NewErrWrapper(c Classifier, op string, err error) *ErrWrapper { + var wrapper *ErrWrapper + if errors.As(err, &wrapper) { + return &ErrWrapper{ + Failure: wrapper.Failure, + Operation: wrapper.Operation, + WrappedErr: err, + } + } if c == nil { panic("nil classifier") } @@ -97,6 +112,10 @@ func NewErrWrapper(c Classifier, op string, err error) *ErrWrapper { // NewTopLevelGenericErrWrapper wraps an error occurring at top // level using ClassifyGenericError as classifier. +// +// 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) } diff --git a/internal/netxlite/errwrapper_test.go b/internal/netxlite/errwrapper_test.go index 1fe8aa4..97cc645 100644 --- a/internal/netxlite/errwrapper_test.go +++ b/internal/netxlite/errwrapper_test.go @@ -3,6 +3,7 @@ package netxlite import ( "encoding/json" "errors" + "fmt" "io" "testing" @@ -101,6 +102,22 @@ func TestNewErrWrapper(t *testing.T) { t.Fatal("unexpected WrappedErr") } }) + + t.Run("when the underlying error is already a wrapped error", func(t *testing.T) { + ew := NewErrWrapper(classifySyscallError, ReadOperation, ECONNRESET) + var err1 error = ew + err2 := fmt.Errorf("cannot read: %w", err1) + ew2 := NewErrWrapper(ClassifyGenericError, TopLevelOperation, err2) + if ew2.Failure != ew.Failure { + t.Fatal("not the same failure") + } + if ew2.Operation != ew.Operation { + t.Fatal("not the same operation") + } + if ew2.WrappedErr != err2 { + t.Fatal("invalid underlying error") + } + }) } func TestNewTopLevelGenericErrWrapper(t *testing.T) { diff --git a/internal/netxlite/http.go b/internal/netxlite/http.go index 8ccbf06..fbd59d7 100644 --- a/internal/netxlite/http.go +++ b/internal/netxlite/http.go @@ -19,6 +19,19 @@ type HTTPTransport interface { CloseIdleConnections() } +// httpTransportErrWrapper is an HTTPTransport with error wrapping. +type httpTransportErrWrapper struct { + HTTPTransport +} + +func (txp *httpTransportErrWrapper) RoundTrip(req *http.Request) (*http.Response, error) { + resp, err := txp.HTTPTransport.RoundTrip(req) + if err != nil { + return nil, NewTopLevelGenericErrWrapper(err) + } + return resp, nil +} + // httpTransportLogger is an HTTPTransport with logging. type httpTransportLogger struct { // HTTPTransport is the underlying HTTP transport. @@ -141,12 +154,13 @@ func NewOOHTTPBaseTransport(dialer Dialer, tlsDialer TLSDialer) HTTPTransport { } } -// WrapHTTPTransport creates an HTTPTransport using the given logger. +// WrapHTTPTransport creates an HTTPTransport using the given logger +// and guarantees that returned errors are wrapped. // // This is a low level factory. Consider not using it directly. func WrapHTTPTransport(logger Logger, txp HTTPTransport) HTTPTransport { return &httpTransportLogger{ - HTTPTransport: txp, + HTTPTransport: &httpTransportErrWrapper{txp}, Logger: logger, } } @@ -250,3 +264,26 @@ func NewHTTPTransportStdlib(logger Logger) HTTPTransport { tlsDialer := NewTLSDialer(dialer, NewTLSHandshakerStdlib(logger)) return NewHTTPTransport(logger, dialer, tlsDialer) } + +// HTTPClient is an http.Client-like interface. +type HTTPClient interface { + Do(req *http.Request) (*http.Response, error) + CloseIdleConnections() +} + +// WrapHTTPClient wraps an HTTP client to add error wrapping capabilities. +func WrapHTTPClient(clnt HTTPClient) HTTPClient { + return &httpClientErrWrapper{clnt} +} + +type httpClientErrWrapper struct { + HTTPClient +} + +func (c *httpClientErrWrapper) Do(req *http.Request) (*http.Response, error) { + resp, err := c.HTTPClient.Do(req) + if err != nil { + return nil, NewTopLevelGenericErrWrapper(err) + } + return resp, nil +} diff --git a/internal/netxlite/http_test.go b/internal/netxlite/http_test.go index 2844d8a..8009f60 100644 --- a/internal/netxlite/http_test.go +++ b/internal/netxlite/http_test.go @@ -16,6 +16,49 @@ import ( "github.com/ooni/probe-cli/v3/internal/netxlite/mocks" ) +func TestHTTPTransportErrWrapper(t *testing.T) { + t.Run("RoundTrip", func(t *testing.T) { + t.Run("with failure", func(t *testing.T) { + txp := &httpTransportErrWrapper{ + HTTPTransport: &mocks.HTTPTransport{ + MockRoundTrip: func(req *http.Request) (*http.Response, error) { + return nil, io.EOF + }, + }, + } + resp, err := txp.RoundTrip(&http.Request{}) + var errWrapper *ErrWrapper + if !errors.As(err, &errWrapper) { + t.Fatal("the returned error is not an ErrWrapper") + } + if errWrapper.Failure != FailureEOFError { + t.Fatal("unexpected failure", errWrapper.Failure) + } + if resp != nil { + t.Fatal("expected nil response") + } + }) + + t.Run("with success", func(t *testing.T) { + expect := &http.Response{} + txp := &httpTransportErrWrapper{ + HTTPTransport: &mocks.HTTPTransport{ + MockRoundTrip: func(req *http.Request) (*http.Response, error) { + return expect, nil + }, + }, + } + resp, err := txp.RoundTrip(&http.Request{}) + if err != nil { + t.Fatal(err) + } + if resp != expect { + t.Fatal("not the expected response") + } + }) + }) +} + func TestHTTPTransportLogger(t *testing.T) { t.Run("RoundTrip", func(t *testing.T) { t.Run("with failure", func(t *testing.T) { @@ -198,7 +241,8 @@ func TestNewHTTPTransport(t *testing.T) { if logger.Logger != log.Log { t.Fatal("invalid logger") } - connectionsCloser := logger.HTTPTransport.(*httpTransportConnectionsCloser) + errWrapper := logger.HTTPTransport.(*httpTransportErrWrapper) + connectionsCloser := errWrapper.HTTPTransport.(*httpTransportConnectionsCloser) withReadTimeout := connectionsCloser.Dialer.(*httpDialerWithReadTimeout) if withReadTimeout.Dialer != d { t.Fatal("invalid dialer") @@ -412,3 +456,56 @@ func TestNewHTTPTransportStdlib(t *testing.T) { } txp.CloseIdleConnections() } + +func TestHTTPClientErrWrapper(t *testing.T) { + t.Run("Do", func(t *testing.T) { + t.Run("with failure", func(t *testing.T) { + clnt := &httpClientErrWrapper{ + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + return nil, io.EOF + }, + }, + } + resp, err := clnt.Do(&http.Request{}) + var errWrapper *ErrWrapper + if !errors.As(err, &errWrapper) { + t.Fatal("the returned error is not an ErrWrapper") + } + if errWrapper.Failure != FailureEOFError { + t.Fatal("unexpected failure", errWrapper.Failure) + } + if resp != nil { + t.Fatal("expected nil response") + } + }) + + t.Run("with success", func(t *testing.T) { + expect := &http.Response{} + clnt := &httpClientErrWrapper{ + HTTPClient: &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + return expect, nil + }, + }, + } + resp, err := clnt.Do(&http.Request{}) + if err != nil { + t.Fatal(err) + } + if resp != expect { + t.Fatal("not the expected response") + } + }) + }) +} + +func TestWrapHTTPClient(t *testing.T) { + origClient := &http.Client{} + wrapped := WrapHTTPClient(origClient) + errWrapper := wrapped.(*httpClientErrWrapper) + innerClient := errWrapper.HTTPClient.(*http.Client) + if innerClient != origClient { + t.Fatal("not the inner client we expected") + } +} diff --git a/internal/netxlite/iox.go b/internal/netxlite/iox.go index 710e1cb..53fa22b 100644 --- a/internal/netxlite/iox.go +++ b/internal/netxlite/iox.go @@ -27,9 +27,9 @@ func ReadAllContext(ctx context.Context, r io.Reader) ([]byte, error) { case data := <-datach: return data, nil case <-ctx.Done(): - return nil, ctx.Err() + return nil, NewTopLevelGenericErrWrapper(ctx.Err()) case err := <-errch: - return nil, err + return nil, NewTopLevelGenericErrWrapper(err) } } @@ -51,8 +51,8 @@ func CopyContext(ctx context.Context, dst io.Writer, src io.Reader) (int64, erro case count := <-countch: return count, nil case <-ctx.Done(): - return 0, ctx.Err() + return 0, NewTopLevelGenericErrWrapper(ctx.Err()) case err := <-errch: - return 0, err + return 0, NewTopLevelGenericErrWrapper(err) } } diff --git a/internal/netxlite/iox_test.go b/internal/netxlite/iox_test.go index d36d173..908fddf 100644 --- a/internal/netxlite/iox_test.go +++ b/internal/netxlite/iox_test.go @@ -36,6 +36,10 @@ func TestReadAllContext(t *testing.T) { if !errors.Is(err, expected) { t.Fatal("not the error we expected", err) } + var errWrapper *ErrWrapper + if !errors.As(err, &errWrapper) { + t.Fatal("the returned error is not wrapped") + } if len(out) != 0 { t.Fatal("not the expected number of bytes") } @@ -65,6 +69,10 @@ func TestReadAllContext(t *testing.T) { if !errors.Is(err, context.Canceled) { t.Fatal("not the error we expected", err) } + var errWrapper *ErrWrapper + if !errors.As(err, &errWrapper) { + t.Fatal("the returned error is not wrapped") + } if len(out) != 0 { t.Fatal("not the expected number of bytes") } @@ -90,6 +98,10 @@ func TestReadAllContext(t *testing.T) { if !errors.Is(err, context.Canceled) { t.Fatal("not the error we expected", err) } + var errWrapper *ErrWrapper + if !errors.As(err, &errWrapper) { + t.Fatal("the returned error is not wrapped") + } if len(out) != 0 { t.Fatal("not the expected number of bytes") } @@ -123,6 +135,10 @@ func TestCopyContext(t *testing.T) { if !errors.Is(err, expected) { t.Fatal("not the error we expected", err) } + var errWrapper *ErrWrapper + if !errors.As(err, &errWrapper) { + t.Fatal("the returned error is not wrapped") + } if out != 0 { t.Fatal("not the expected number of bytes") } @@ -152,6 +168,10 @@ func TestCopyContext(t *testing.T) { if !errors.Is(err, context.Canceled) { t.Fatal("not the error we expected", err) } + var errWrapper *ErrWrapper + if !errors.As(err, &errWrapper) { + t.Fatal("the returned error is not wrapped") + } if out != 0 { t.Fatal("not the expected number of bytes") } @@ -177,6 +197,10 @@ func TestCopyContext(t *testing.T) { if !errors.Is(err, context.Canceled) { t.Fatal("not the error we expected", err) } + var errWrapper *ErrWrapper + if !errors.As(err, &errWrapper) { + t.Fatal("the returned error is not wrapped") + } if out != 0 { t.Fatal("not the expected number of bytes") }