refactor: move httptransport w/ logging to netxlite (#411)

Part of https://github.com/ooni/probe/issues/1505

internal/engine/netx/dialer/dialer.go

@@ -25,6 +25,9 @@ type Resolver interface {
 type Logger interface {
 	// Debugf formats and emits a debug message.
 	Debugf(format string, v ...interface{})
+
+	// Debug emits a debug message.
+	Debug(msg string)
 }
 
 // Config contains the settings for New.

internal/engine/netx/httptransport/logging.go

@@ -1,50 +0,0 @@
-package httptransport
-
-import "net/http"
-
-// Logger is the logger assumed by this package
-type Logger interface {
-	Debugf(format string, v ...interface{})
-	Debug(message string)
-}
-
-// LoggingTransport is a logging transport
-type LoggingTransport struct {
-	RoundTripper
-	Logger Logger
-}
-
-// RoundTrip implements RoundTripper.RoundTrip
-func (txp LoggingTransport) RoundTrip(req *http.Request) (*http.Response, error) {
-	host := req.Host
-	if host == "" {
-		host = req.URL.Host
-	}
-	req.Header.Set("Host", host) // anticipate what Go would do
-	return txp.logTrip(req)
-}
-
-func (txp LoggingTransport) logTrip(req *http.Request) (*http.Response, error) {
-	txp.Logger.Debugf("> %s %s", req.Method, req.URL.String())
-	for key, values := range req.Header {
-		for _, value := range values {
-			txp.Logger.Debugf("> %s: %s", key, value)
-		}
-	}
-	txp.Logger.Debug(">")
-	resp, err := txp.RoundTripper.RoundTrip(req)
-	if err != nil {
-		txp.Logger.Debugf("< %s", err)
-		return nil, err
-	}
-	txp.Logger.Debugf("< %d", resp.StatusCode)
-	for key, values := range resp.Header {
-		for _, value := range values {
-			txp.Logger.Debugf("< %s: %s", key, value)
-		}
-	}
-	txp.Logger.Debug("<")
-	return resp, nil
-}
-
-var _ RoundTripper = LoggingTransport{}

internal/engine/netx/httptransport/logging_test.go

@@ -1,78 +0,0 @@
-package httptransport_test
-
-import (
-	"context"
-	"errors"
-	"io"
-	"net/http"
-	"net/url"
-	"strings"
-	"testing"
-
-	"github.com/apex/log"
-	"github.com/ooni/probe-cli/v3/internal/engine/netx/httptransport"
-	"github.com/ooni/probe-cli/v3/internal/iox"
-)
-
-func TestLoggingFailure(t *testing.T) {
-	txp := httptransport.LoggingTransport{
-		Logger: log.Log,
-		RoundTripper: httptransport.FakeTransport{
-			Err: io.EOF,
-		},
-	}
-	client := &http.Client{Transport: txp}
-	resp, err := client.Get("https://www.google.com")
-	if !errors.Is(err, io.EOF) {
-		t.Fatal("not the error we expected")
-	}
-	if resp != nil {
-		t.Fatal("expected nil response here")
-	}
-}
-
-func TestLoggingFailureWithNoHostHeader(t *testing.T) {
-	txp := httptransport.LoggingTransport{
-		Logger: log.Log,
-		RoundTripper: httptransport.FakeTransport{
-			Err: io.EOF,
-		},
-	}
-	req := &http.Request{
-		Header: http.Header{},
-		URL: &url.URL{
-			Scheme: "https",
-			Host:   "www.google.com",
-			Path:   "/",
-		},
-	}
-	resp, err := txp.RoundTrip(req)
-	if !errors.Is(err, io.EOF) {
-		t.Fatal("not the error we expected")
-	}
-	if resp != nil {
-		t.Fatal("expected nil response here")
-	}
-}
-
-func TestLoggingSuccess(t *testing.T) {
-	txp := httptransport.LoggingTransport{
-		Logger: log.Log,
-		RoundTripper: httptransport.FakeTransport{
-			Resp: &http.Response{
-				Body: io.NopCloser(strings.NewReader("")),
-				Header: http.Header{
-					"Server": []string{"antani/0.1.0"},
-				},
-				StatusCode: 200,
-			},
-		},
-	}
-	client := &http.Client{Transport: txp}
-	resp, err := client.Get("https://www.google.com")
-	if err != nil {
-		t.Fatal(err)
-	}
-	iox.ReadAllContext(context.Background(), resp.Body)
-	resp.Body.Close()
-}

internal/engine/netx/netx.go

@@ -231,7 +231,7 @@ func NewHTTPTransport(config Config) HTTPRoundTripper {
 			Counter: config.ByteCounter, RoundTripper: txp}
 	}
 	if config.Logger != nil {
-		txp = httptransport.LoggingTransport{Logger: config.Logger, RoundTripper: txp}
+		txp = &netxlite.HTTPTransportLogger{Logger: config.Logger, HTTPTransport: txp}
 	}
 	if config.HTTPSaver != nil {
 		txp = httptransport.SaverMetadataHTTPTransport{

internal/engine/netx/netx_test.go

@@ -491,14 +491,14 @@ func TestNewWithLogger(t *testing.T) {
 	if !ok {
 		t.Fatal("not the transport we expected")
 	}
-	ltxp, ok := uatxp.RoundTripper.(httptransport.LoggingTransport)
+	ltxp, ok := uatxp.RoundTripper.(*netxlite.HTTPTransportLogger)
 	if !ok {
 		t.Fatal("not the transport we expected")
 	}
 	if ltxp.Logger != log.Log {
 		t.Fatal("not the logger we expected")
 	}
-	if _, ok := ltxp.RoundTripper.(*http.Transport); !ok {
+	if _, ok := ltxp.HTTPTransport.(*http.Transport); !ok {
 		t.Fatal("not the transport we expected")
 	}
 }

internal/netxlite/http.go

@@ -0,0 +1,62 @@
+package netxlite
+
+import "net/http"
+
+// HTTPTransport is an http.Transport-like structure.
+type HTTPTransport interface {
+	// RoundTrip performs the HTTP round trip.
+	RoundTrip(req *http.Request) (*http.Response, error)
+
+	// CloseIdleConnections closes idle connections.
+	CloseIdleConnections()
+}
+
+// HTTPTransportLogger is an HTTPTransport with logging.
+type HTTPTransportLogger struct {
+	// HTTPTransport is the underlying HTTP transport.
+	HTTPTransport HTTPTransport
+
+	// Logger is the underlying logger.
+	Logger Logger
+}
+
+var _ HTTPTransport = &HTTPTransportLogger{}
+
+// RoundTrip implements HTTPTransport.RoundTrip.
+func (txp *HTTPTransportLogger) RoundTrip(req *http.Request) (*http.Response, error) {
+	host := req.Host
+	if host == "" {
+		host = req.URL.Host
+	}
+	req.Header.Set("Host", host) // anticipate what Go would do
+	return txp.logTrip(req)
+}
+
+// logTrip is an HTTP round trip with logging.
+func (txp *HTTPTransportLogger) logTrip(req *http.Request) (*http.Response, error) {
+	txp.Logger.Debugf("> %s %s", req.Method, req.URL.String())
+	for key, values := range req.Header {
+		for _, value := range values {
+			txp.Logger.Debugf("> %s: %s", key, value)
+		}
+	}
+	txp.Logger.Debug(">")
+	resp, err := txp.HTTPTransport.RoundTrip(req)
+	if err != nil {
+		txp.Logger.Debugf("< %s", err)
+		return nil, err
+	}
+	txp.Logger.Debugf("< %d", resp.StatusCode)
+	for key, values := range resp.Header {
+		for _, value := range values {
+			txp.Logger.Debugf("< %s: %s", key, value)
+		}
+	}
+	txp.Logger.Debug("<")
+	return resp, nil
+}
+
+// CloseIdleConnections implement HTTPTransport.CloseIdleConnections.
+func (txp *HTTPTransportLogger) CloseIdleConnections() {
+	txp.HTTPTransport.CloseIdleConnections()
+}

internal/netxlite/http_test.go

@@ -0,0 +1,108 @@
+package netxlite
+
+import (
+	"context"
+	"errors"
+	"io"
+	"net/http"
+	"net/url"
+	"strings"
+	"testing"
+
+	"github.com/apex/log"
+	"github.com/ooni/probe-cli/v3/internal/atomicx"
+	"github.com/ooni/probe-cli/v3/internal/iox"
+	"github.com/ooni/probe-cli/v3/internal/netxmocks"
+)
+
+func TestHTTPTransportLoggerFailure(t *testing.T) {
+	txp := &HTTPTransportLogger{
+		Logger: log.Log,
+		HTTPTransport: &netxmocks.HTTPTransport{
+			MockRoundTrip: func(req *http.Request) (*http.Response, error) {
+				return nil, io.EOF
+			},
+		},
+	}
+	client := &http.Client{Transport: txp}
+	resp, err := client.Get("https://www.google.com")
+	if !errors.Is(err, io.EOF) {
+		t.Fatal("not the error we expected")
+	}
+	if resp != nil {
+		t.Fatal("expected nil response here")
+	}
+}
+
+func TestHTTPTransportLoggerFailureWithNoHostHeader(t *testing.T) {
+	foundHost := &atomicx.Int64{}
+	txp := &HTTPTransportLogger{
+		Logger: log.Log,
+		HTTPTransport: &netxmocks.HTTPTransport{
+			MockRoundTrip: func(req *http.Request) (*http.Response, error) {
+				if req.Header.Get("Host") == "www.google.com" {
+					foundHost.Add(1)
+				}
+				return nil, io.EOF
+			},
+		},
+	}
+	req := &http.Request{
+		Header: http.Header{},
+		URL: &url.URL{
+			Scheme: "https",
+			Host:   "www.google.com",
+			Path:   "/",
+		},
+	}
+	resp, err := txp.RoundTrip(req)
+	if !errors.Is(err, io.EOF) {
+		t.Fatal("not the error we expected")
+	}
+	if resp != nil {
+		t.Fatal("expected nil response here")
+	}
+	if foundHost.Load() != 1 {
+		t.Fatal("host header was not added")
+	}
+}
+
+func TestHTTPTransportLoggerSuccess(t *testing.T) {
+	txp := &HTTPTransportLogger{
+		Logger: log.Log,
+		HTTPTransport: &netxmocks.HTTPTransport{
+			MockRoundTrip: func(req *http.Request) (*http.Response, error) {
+				return &http.Response{
+					Body: io.NopCloser(strings.NewReader("")),
+					Header: http.Header{
+						"Server": []string{"antani/0.1.0"},
+					},
+					StatusCode: 200,
+				}, nil
+			},
+		},
+	}
+	client := &http.Client{Transport: txp}
+	resp, err := client.Get("https://www.google.com")
+	if err != nil {
+		t.Fatal(err)
+	}
+	iox.ReadAllContext(context.Background(), resp.Body)
+	resp.Body.Close()
+}
+
+func TestHTTPTransportLoggerCloseIdleConnections(t *testing.T) {
+	calls := &atomicx.Int64{}
+	txp := &HTTPTransportLogger{
+		HTTPTransport: &netxmocks.HTTPTransport{
+			MockCloseIdleConnections: func() {
+				calls.Add(1)
+			},
+		},
+		Logger: log.Log,
+	}
+	txp.CloseIdleConnections()
+	if calls.Load() != 1 {
+		t.Fatal("not called")
+	}
+}

internal/netxlite/logger.go

@@ -4,4 +4,7 @@ package netxlite
 type Logger interface {
 	// Debugf formats and emits a debug message.
 	Debugf(format string, v ...interface{})
+
+	// Debug emits a debug message.
+	Debug(msg string)
 }

internal/netxmocks/http.go

@@ -0,0 +1,19 @@
+package netxmocks
+
+import "net/http"
+
+// HTTPTransport mocks netxlite.HTTPTransport.
+type HTTPTransport struct {
+	MockRoundTrip            func(req *http.Request) (*http.Response, error)
+	MockCloseIdleConnections func()
+}
+
+// RoundTrip calls MockRoundTrip.
+func (txp *HTTPTransport) RoundTrip(req *http.Request) (*http.Response, error) {
+	return txp.MockRoundTrip(req)
+}
+
+// CloseIdleConnections calls MockCloseIdleConnections.
+func (txp *HTTPTransport) CloseIdleConnections() {
+	txp.MockCloseIdleConnections()
+}

internal/netxmocks/http_test.go

@@ -0,0 +1,38 @@
+package netxmocks
+
+import (
+	"errors"
+	"net/http"
+	"testing"
+
+	"github.com/ooni/probe-cli/v3/internal/atomicx"
+)
+
+func TestHTTPTransportRoundTrip(t *testing.T) {
+	expected := errors.New("mocked error")
+	txp := &HTTPTransport{
+		MockRoundTrip: func(req *http.Request) (*http.Response, error) {
+			return nil, expected
+		},
+	}
+	resp, err := txp.RoundTrip(&http.Request{})
+	if !errors.Is(err, expected) {
+		t.Fatal("not the error we expected", err)
+	}
+	if resp != nil {
+		t.Fatal("expected nil response here")
+	}
+}
+
+func TestHTTPTransportCloseIdleConnections(t *testing.T) {
+	called := &atomicx.Int64{}
+	txp := &HTTPTransport{
+		MockCloseIdleConnections: func() {
+			called.Add(1)
+		},
+	}
+	txp.CloseIdleConnections()
+	if called.Load() != 1 {
+		t.Fatal("not called")
+	}
+}