From f05748e04c4a15429eee1c978b44d6579d36ece5 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 25 Oct 2023 10:15:48 +0200 Subject: [PATCH] refactor(dslx): prepare for making HTTPRequest stateless (#1380) - avoid storing state for creating an http.Request in httpRequestFunc - avoid using methods for constructing an http.Request where we can use pure functions instead - update and adapt the unit test suite Reference issue: https://github.com/ooni/probe/issues/2612 --- internal/dslx/http_test.go | 236 +++++++++++++++++++++++++++++-------- internal/dslx/httpcore.go | 126 +++++++------------- 2 files changed, 227 insertions(+), 135 deletions(-) diff --git a/internal/dslx/http_test.go b/internal/dslx/http_test.go index e95ca14e0f..d8ea71bdcd 100644 --- a/internal/dslx/http_test.go +++ b/internal/dslx/http_test.go @@ -17,57 +17,193 @@ import ( "github.com/ooni/probe-cli/v3/internal/model" ) -/* -Test cases: -- Get httpRequestFunc with options -- Apply httpRequestFunc: - - with EOF - - with invalid method - - with port-less address - - with success (https) - - with success (http) - - with header options -*/ -func TestHTTPRequest(t *testing.T) { - t.Run("Get httpRequestFunc with options", func(t *testing.T) { - rt := NewMinimalRuntime(model.DiscardLogger, time.Now()) - f := HTTPRequest(rt, +func TestHTTPNewRequest(t *testing.T) { + t.Run("without any option and with domain", func(t *testing.T) { + ctx := context.Background() + conn := &HTTPTransport{ + Address: "130.192.91.211:443", + Domain: "example.com", + Network: "tcp", + Scheme: "https", + TLSNegotiatedProtocol: "h2", + Trace: nil, + Transport: nil, + } + + req, err := httpNewRequest(ctx, conn, model.DiscardLogger) + if err != nil { + t.Fatal(err) + } + + if req.URL.Scheme != "https" { + t.Fatal("unexpected req.URL.Scheme", req.URL.Scheme) + } + if req.URL.Host != "example.com" { + t.Fatal("unexpected req.URL.Host", req.URL.Host) + } + if req.URL.Path != "/" { + t.Fatal("unexpected req.URL.Path", req.URL.Path) + } + if req.Method != "GET" { + t.Fatal("unexpected req.Method", req.Method) + } + if req.Host != "example.com" { + t.Fatal("unexpected req.Host", req.Host) + } + headers := http.Header{ + "Host": {"example.com"}, + } + if diff := cmp.Diff(headers, req.Header); diff != "" { + t.Fatal(diff) + } + }) + + t.Run("without any option, without domain but with standard port", func(t *testing.T) { + ctx := context.Background() + conn := &HTTPTransport{ + Address: "130.192.91.211:443", + Domain: "", + Network: "tcp", + Scheme: "https", + TLSNegotiatedProtocol: "h2", + Trace: nil, + Transport: nil, + } + + req, err := httpNewRequest(ctx, conn, model.DiscardLogger) + if err != nil { + t.Fatal(err) + } + + if req.URL.Scheme != "https" { + t.Fatal("unexpected req.URL.Scheme", req.URL.Scheme) + } + if req.URL.Host != "130.192.91.211" { + t.Fatal("unexpected req.URL.Host", req.URL.Host) + } + if req.URL.Path != "/" { + t.Fatal("unexpected req.URL.Path", req.URL.Path) + } + if req.Method != "GET" { + t.Fatal("unexpected req.Method", req.Method) + } + if req.Host != "130.192.91.211" { + t.Fatal("unexpected req.Host", req.Host) + } + headers := http.Header{ + "Host": {"130.192.91.211"}, + } + if diff := cmp.Diff(headers, req.Header); diff != "" { + t.Fatal(diff) + } + }) + + t.Run("without any option, without domain but with nonstandard port", func(t *testing.T) { + ctx := context.Background() + conn := &HTTPTransport{ + Address: "130.192.91.211:443", + Domain: "", + Network: "tcp", + Scheme: "http", + TLSNegotiatedProtocol: "h2", + Trace: nil, + Transport: nil, + } + + req, err := httpNewRequest(ctx, conn, model.DiscardLogger) + if err != nil { + t.Fatal(err) + } + + if req.URL.Scheme != "http" { + t.Fatal("unexpected req.URL.Scheme", req.URL.Scheme) + } + if req.URL.Host != "130.192.91.211:443" { + t.Fatal("unexpected req.URL.Host", req.URL.Host) + } + if req.URL.Path != "/" { + t.Fatal("unexpected req.URL.Path", req.URL.Path) + } + if req.Method != "GET" { + t.Fatal("unexpected req.Method", req.Method) + } + if req.Host != "130.192.91.211:443" { + t.Fatal("unexpected req.Host", req.Host) + } + headers := http.Header{ + "Host": {"130.192.91.211:443"}, + } + if diff := cmp.Diff(headers, req.Header); diff != "" { + t.Fatal(diff) + } + }) + + t.Run("with all options", func(t *testing.T) { + ctx := context.Background() + conn := &HTTPTransport{ + Address: "130.192.91.211:443", + Domain: "example.com", + Network: "tcp", + Scheme: "https", + TLSNegotiatedProtocol: "h2", + Trace: nil, + Transport: nil, + } + + options := []HTTPRequestOption{ HTTPRequestOptionAccept("text/html"), HTTPRequestOptionAcceptLanguage("de"), - HTTPRequestOptionHost("host"), + HTTPRequestOptionHost("www.x.org"), HTTPRequestOptionMethod("PUT"), HTTPRequestOptionReferer("https://example.com/"), HTTPRequestOptionURLPath("/path/to/example"), HTTPRequestOptionUserAgent("Mozilla/5.0 Gecko/geckotrail Firefox/firefoxversion"), - ) - var requestFunc *httpRequestFunc - var ok bool - if requestFunc, ok = f.(*httpRequestFunc); !ok { - t.Fatal("unexpected type. Expected: tlsHandshakeFunc") } - if requestFunc.Accept != "text/html" { - t.Fatalf("unexpected %s, expected %s, got %s", "Accept", "text/html", requestFunc.Accept) + + req, err := httpNewRequest(ctx, conn, model.DiscardLogger, options...) + if err != nil { + t.Fatal(err) + } + + if req.URL.Scheme != "https" { + t.Fatal("unexpected req.URL.Scheme", req.URL.Scheme) } - if requestFunc.AcceptLanguage != "de" { - t.Fatalf("unexpected %s, expected %s, got %s", "AcceptLanguage", "de", requestFunc.AcceptLanguage) + if req.URL.Host != "www.x.org" { + t.Fatal("unexpected req.URL.Host", req.URL.Host) } - if requestFunc.Host != "host" { - t.Fatalf("unexpected %s, expected %s, got %s", "Host", "host", requestFunc.Host) + if req.URL.Path != "/path/to/example" { + t.Fatal("unexpected req.URL.Path", req.URL.Path) } - if requestFunc.Method != "PUT" { - t.Fatalf("unexpected %s, expected %s, got %s", "Method", "PUT", requestFunc.Method) + if req.Method != "PUT" { + t.Fatal("unexpected req.Method", req.Method) } - if requestFunc.Referer != "https://example.com/" { - t.Fatalf("unexpected %s, expected %s, got %s", "Referer", "https://example.com/", requestFunc.Referer) + if req.Host != "www.x.org" { + t.Fatal("unexpected req.Host", req.Host) } - if requestFunc.URLPath != "/path/to/example" { - t.Fatalf("unexpected %s, expected %s, got %s", "URLPath", "example/to/path", requestFunc.URLPath) + headers := http.Header{ + "Accept": {"text/html"}, + "Accept-Language": {"de"}, + "Host": {"www.x.org"}, + "Referer": {"https://example.com/"}, + "User-Agent": {"Mozilla/5.0 Gecko/geckotrail Firefox/firefoxversion"}, } - if requestFunc.UserAgent != "Mozilla/5.0 Gecko/geckotrail Firefox/firefoxversion" { - t.Fatalf("unexpected %s, expected %s, got %s", "UserAgent", "Mozilla/5.0 Gecko/geckotrail Firefox/firefoxversion", requestFunc.UserAgent) + if diff := cmp.Diff(headers, req.Header); diff != "" { + t.Fatal(diff) } }) +} +/* +Test cases: +- Apply httpRequestFunc: + - with EOF + - with invalid method + - with port-less address + - with success (https) + - with success (http) + - with header options +*/ +func TestHTTPRequest(t *testing.T) { t.Run("Apply httpRequestFunc", func(t *testing.T) { mockResponse := &http.Response{ Status: "expected", @@ -115,20 +251,20 @@ func TestHTTPRequest(t *testing.T) { } }) - t.Run("with invalid method", func(t *testing.T) { + t.Run("with invalid domain", func(t *testing.T) { httpTransport := HTTPTransport{ Address: "1.2.3.4:567", + Domain: "\t", // invalid domain Network: "tcp", Scheme: "https", Trace: trace, Transport: goodTransport, } - httpRequest := &httpRequestFunc{ - Method: "€", - } + rt := NewMinimalRuntime(model.DiscardLogger, time.Now()) + httpRequest := HTTPRequest(rt) res := httpRequest.Apply(context.Background(), &httpTransport) - if res.Error == nil || !strings.HasPrefix(res.Error.Error(), "net/http: invalid method") { - t.Fatal("not the error we expected") + if res.Error == nil || !strings.HasPrefix(res.Error.Error(), `parse "https://%09/": invalid URL escape "%09"`) { + t.Fatal("not the error we expected", res.Error) } if res.State.HTTPResponse != nil { t.Fatal("expected nil request here") @@ -238,15 +374,15 @@ func TestHTTPRequest(t *testing.T) { Trace: trace, Transport: goodTransport, } - httpRequest := &httpRequestFunc{ - Accept: "text/html", - AcceptLanguage: "de", - Host: "host", - Referer: "https://example.org", - Rt: NewMinimalRuntime(model.DiscardLogger, time.Now()), - URLPath: "/path/to/example", - UserAgent: "Mozilla/5.0 Gecko/geckotrail Firefox/firefoxversion", - } + rt := NewMinimalRuntime(model.DiscardLogger, time.Now()) + httpRequest := HTTPRequest(rt, + HTTPRequestOptionAccept("text/html"), + HTTPRequestOptionAcceptLanguage("de"), + HTTPRequestOptionHost("host"), + HTTPRequestOptionReferer("https://example.org"), + HTTPRequestOptionURLPath("/path/to/example"), + HTTPRequestOptionUserAgent("Mozilla/5.0 Gecko/geckotrail Firefox/firefoxversion"), + ) res := httpRequest.Apply(context.Background(), &httpTransport) if res.Error != nil { t.Fatal("unexpected error") diff --git a/internal/dslx/httpcore.go b/internal/dslx/httpcore.go index 08ff04eb2b..a2dc1f91ae 100644 --- a/internal/dslx/httpcore.go +++ b/internal/dslx/httpcore.go @@ -48,91 +48,71 @@ type HTTPTransport struct { } // HTTPRequestOption is an option you can pass to HTTPRequest. -type HTTPRequestOption func(*httpRequestFunc) +type HTTPRequestOption func(req *http.Request) // HTTPRequestOptionAccept sets the Accept header. func HTTPRequestOptionAccept(value string) HTTPRequestOption { - return func(hrf *httpRequestFunc) { - hrf.Accept = value + return func(req *http.Request) { + req.Header.Set("Accept", value) } } // HTTPRequestOptionAcceptLanguage sets the Accept header. func HTTPRequestOptionAcceptLanguage(value string) HTTPRequestOption { - return func(hrf *httpRequestFunc) { - hrf.AcceptLanguage = value + return func(req *http.Request) { + req.Header.Set("Accept-Language", value) } } // HTTPRequestOptionHost sets the Host header. func HTTPRequestOptionHost(value string) HTTPRequestOption { - return func(hrf *httpRequestFunc) { - hrf.Host = value + return func(req *http.Request) { + req.URL.Host = value + req.Host = value } } // HTTPRequestOptionHost sets the request method. func HTTPRequestOptionMethod(value string) HTTPRequestOption { - return func(hrf *httpRequestFunc) { - hrf.Method = value + return func(req *http.Request) { + req.Method = value } } // HTTPRequestOptionReferer sets the Referer header. func HTTPRequestOptionReferer(value string) HTTPRequestOption { - return func(hrf *httpRequestFunc) { - hrf.Referer = value + return func(req *http.Request) { + req.Header.Set("Referer", value) } } // HTTPRequestOptionURLPath sets the URL path. func HTTPRequestOptionURLPath(value string) HTTPRequestOption { - return func(hrf *httpRequestFunc) { - hrf.URLPath = value + return func(req *http.Request) { + req.URL.Path = value } } // HTTPRequestOptionUserAgent sets the UserAgent header. func HTTPRequestOptionUserAgent(value string) HTTPRequestOption { - return func(hrf *httpRequestFunc) { - hrf.UserAgent = value + return func(req *http.Request) { + req.Header.Set("User-Agent", value) } } // HTTPRequest issues an HTTP request using a transport and returns a response. func HTTPRequest(rt Runtime, options ...HTTPRequestOption) Func[*HTTPTransport, *Maybe[*HTTPResponse]] { - f := &httpRequestFunc{Rt: rt} - for _, option := range options { - option(f) - } + f := &httpRequestFunc{Options: options, Rt: rt} return f } // httpRequestFunc is the Func returned by HTTPRequest. type httpRequestFunc struct { - // Accept is the OPTIONAL accept header. - Accept string - - // AcceptLanguage is the OPTIONAL accept-language header. - AcceptLanguage string - - // Host is the OPTIONAL host header. - Host string - - // Method is the OPTIONAL method. - Method string - - // Referer is the OPTIONAL referer header. - Referer string + // Options contains the options. + Options []HTTPRequestOption // Rt is the MANDATORY runtime. Rt Runtime - - // URLPath is the OPTIONAL URL path. - URLPath string - - // UserAgent is the OPTIONAL user-agent header. - UserAgent string } // Apply implements Func. @@ -150,7 +130,7 @@ func (f *httpRequestFunc) Apply( ) // create HTTP request - req, err := f.newHTTPRequest(ctx, input) + req, err := httpNewRequest(ctx, input, f.Rt.Logger(), f.Options...) if err == nil { // start the operation logger @@ -165,7 +145,7 @@ func (f *httpRequestFunc) Apply( ) // perform HTTP transaction and collect the related observations - resp, body, observations, err = f.do(ctx, input, req) + resp, body, observations, err = httpRoundTrip(ctx, input, req) // stop the operation logger ol.Stop(err) @@ -191,68 +171,50 @@ func (f *httpRequestFunc) Apply( } } -func (f *httpRequestFunc) newHTTPRequest( - ctx context.Context, input *HTTPTransport) (*http.Request, error) { +// httpNewRequest is a convenience function for creating a new request. +func httpNewRequest( + ctx context.Context, input *HTTPTransport, logger model.Logger, options ...HTTPRequestOption) (*http.Request, error) { + // create the default HTTP request URL := &url.URL{ Scheme: input.Scheme, Opaque: "", User: nil, - Host: f.urlHost(input), - Path: f.urlPath(), + Host: httpNewURLHost(input, logger), + Path: "/", RawPath: "", ForceQuery: false, RawQuery: "", Fragment: "", RawFragment: "", } - - method := "GET" - if f.Method != "" { - method = f.Method - } - - req, err := http.NewRequestWithContext(ctx, method, URL.String(), nil) + req, err := http.NewRequestWithContext(ctx, "GET", URL.String(), nil) if err != nil { return nil, err } - if v := f.Host; v != "" { - req.Host = v - } else { - // Go would use URL.Host as "Host" header anyways in case we leave req.Host empty. - // We already set it here so that we can use req.Host for logging. - req.Host = URL.Host + // Go would use URL.Host as "Host" header anyways in case we leave req.Host empty. + // We already set it here so that we can use req.Host for logging. + req.Host = URL.Host + + // apply the user-specified options + for _, option := range options { + option(req) } + // req.Header["Host"] is ignored by Go but we want to have it in the measurement // to reflect what we think has been sent as HTTP headers. req.Header.Set("Host", req.Host) - - if v := f.Accept; v != "" { - req.Header.Set("Accept", v) - } - - if v := f.AcceptLanguage; v != "" { - req.Header.Set("Accept-Language", v) - } - - if v := f.Referer; v != "" { - req.Header.Set("Referer", v) - } - - if v := f.UserAgent; v != "" { // not setting means using Go's default - req.Header.Set("User-Agent", v) - } - return req, nil } -func (f *httpRequestFunc) urlHost(input *HTTPTransport) string { +// httpNewURLHost computes the URL host to use. +func httpNewURLHost(input *HTTPTransport, logger model.Logger) string { if input.Domain != "" { return input.Domain } addr, port, err := net.SplitHostPort(input.Address) if err != nil { - f.Rt.Logger().Warnf("httpRequestFunc: cannot SplitHostPort for input.Address") + logger.Warnf("httpRequestFunc: cannot SplitHostPort for input.Address") return input.Address } switch { @@ -265,14 +227,8 @@ func (f *httpRequestFunc) urlHost(input *HTTPTransport) string { } } -func (f *httpRequestFunc) urlPath() string { - if f.URLPath != "" { - return f.URLPath - } - return "/" -} - -func (f *httpRequestFunc) do( +// httpRoundTrip performs the actual HTTP round trip +func httpRoundTrip( ctx context.Context, input *HTTPTransport, req *http.Request,