From 527e1a07076805cd0eddf016869b267ced570ab5 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Sat, 26 Jun 2021 18:11:47 +0200 Subject: [PATCH] refactor: move httptransport w/ logging to netxlite (#411) Part of https://github.com/ooni/probe/issues/1505 --- internal/engine/netx/dialer/dialer.go | 3 + internal/engine/netx/httptransport/logging.go | 50 -------- .../engine/netx/httptransport/logging_test.go | 78 ------------- internal/engine/netx/netx.go | 2 +- internal/engine/netx/netx_test.go | 4 +- internal/netxlite/http.go | 62 ++++++++++ internal/netxlite/http_test.go | 108 ++++++++++++++++++ internal/netxlite/logger.go | 3 + internal/netxmocks/http.go | 19 +++ internal/netxmocks/http_test.go | 38 ++++++ 10 files changed, 236 insertions(+), 131 deletions(-) delete mode 100644 internal/engine/netx/httptransport/logging.go delete mode 100644 internal/engine/netx/httptransport/logging_test.go create mode 100644 internal/netxlite/http.go create mode 100644 internal/netxlite/http_test.go create mode 100644 internal/netxmocks/http.go create mode 100644 internal/netxmocks/http_test.go diff --git a/internal/engine/netx/dialer/dialer.go b/internal/engine/netx/dialer/dialer.go index e436172..6f015c6 100644 --- a/internal/engine/netx/dialer/dialer.go +++ b/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. diff --git a/internal/engine/netx/httptransport/logging.go b/internal/engine/netx/httptransport/logging.go deleted file mode 100644 index 07a3bad..0000000 --- a/internal/engine/netx/httptransport/logging.go +++ /dev/null @@ -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{} diff --git a/internal/engine/netx/httptransport/logging_test.go b/internal/engine/netx/httptransport/logging_test.go deleted file mode 100644 index 6b0f47a..0000000 --- a/internal/engine/netx/httptransport/logging_test.go +++ /dev/null @@ -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() -} diff --git a/internal/engine/netx/netx.go b/internal/engine/netx/netx.go index ba2fd77..3dda34e 100644 --- a/internal/engine/netx/netx.go +++ b/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{ diff --git a/internal/engine/netx/netx_test.go b/internal/engine/netx/netx_test.go index 1f2d318..beba3c6 100644 --- a/internal/engine/netx/netx_test.go +++ b/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") } } diff --git a/internal/netxlite/http.go b/internal/netxlite/http.go new file mode 100644 index 0000000..189d5d0 --- /dev/null +++ b/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() +} diff --git a/internal/netxlite/http_test.go b/internal/netxlite/http_test.go new file mode 100644 index 0000000..5c27bd0 --- /dev/null +++ b/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") + } +} diff --git a/internal/netxlite/logger.go b/internal/netxlite/logger.go index 089a36d..526f391 100644 --- a/internal/netxlite/logger.go +++ b/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) } diff --git a/internal/netxmocks/http.go b/internal/netxmocks/http.go new file mode 100644 index 0000000..ac37938 --- /dev/null +++ b/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() +} diff --git a/internal/netxmocks/http_test.go b/internal/netxmocks/http_test.go new file mode 100644 index 0000000..535a21d --- /dev/null +++ b/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") + } +}