From 535a5d3e009bffe9b1ddc238b08c1e61fa5569de Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 5 Jul 2022 19:10:39 +0200 Subject: [PATCH] refactor(oohelperd): flatten package hierarchy (#834) In https://github.com/ooni/probe-cli/pull/832's initial diff, I mentioned it would be cool to flatten oohelperd's hier. I'm doing this now, and just for the master branch. This diff is mostly a mechanical refactoring with very light and apparently rather safe manual changes. --- .../{internal/webconnectivity => }/dns.go | 33 ++++++++---- .../webconnectivity => }/dns_test.go | 6 +-- .../webconnectivity => }/fake_test.go | 2 +- .../webconnectivity.go => handler.go} | 35 ++++++++---- ...ebconnectivity_test.go => handler_test.go} | 6 +-- .../{internal/webconnectivity => }/http.go | 45 ++++++++++------ .../webconnectivity => }/http_test.go | 10 ++-- .../cmd/oohelperd/{oohelperd.go => main.go} | 13 +++-- .../{oohelperd_test.go => main_test.go} | 0 .../{internal/webconnectivity => }/measure.go | 53 ++++++++++--------- .../webconnectivity => }/tcpconnect.go | 44 +++++++++------ .../webconnectivity => }/tcpconnect_test.go | 2 +- 12 files changed, 151 insertions(+), 98 deletions(-) rename internal/cmd/oohelperd/{internal/webconnectivity => }/dns.go (72%) rename internal/cmd/oohelperd/{internal/webconnectivity => }/dns_test.go (97%) rename internal/cmd/oohelperd/{internal/webconnectivity => }/fake_test.go (99%) rename internal/cmd/oohelperd/{internal/webconnectivity/webconnectivity.go => handler.go} (56%) rename internal/cmd/oohelperd/{internal/webconnectivity/webconnectivity_test.go => handler_test.go} (97%) rename internal/cmd/oohelperd/{internal/webconnectivity => }/http.go (74%) rename internal/cmd/oohelperd/{internal/webconnectivity => }/http_test.go (95%) rename internal/cmd/oohelperd/{oohelperd.go => main.go} (80%) rename internal/cmd/oohelperd/{oohelperd_test.go => main_test.go} (100%) rename internal/cmd/oohelperd/{internal/webconnectivity => }/measure.go (57%) rename internal/cmd/oohelperd/{internal/webconnectivity => }/tcpconnect.go (60%) rename internal/cmd/oohelperd/{internal/webconnectivity => }/tcpconnect_test.go (97%) diff --git a/internal/cmd/oohelperd/internal/webconnectivity/dns.go b/internal/cmd/oohelperd/dns.go similarity index 72% rename from internal/cmd/oohelperd/internal/webconnectivity/dns.go rename to internal/cmd/oohelperd/dns.go index c759fda..7f71e83 100644 --- a/internal/cmd/oohelperd/internal/webconnectivity/dns.go +++ b/internal/cmd/oohelperd/dns.go @@ -1,4 +1,8 @@ -package webconnectivity +package main + +// +// DNS measurements +// import ( "context" @@ -13,20 +17,27 @@ import ( // newfailure is a convenience shortcut to save typing var newfailure = tracex.NewFailure -// CtrlDNSResult is the result of the DNS check performed by +// ctrlDNSResult is the result of the DNS check performed by // the Web Connectivity test helper. -type CtrlDNSResult = webconnectivity.ControlDNSResult +type ctrlDNSResult = webconnectivity.ControlDNSResult -// DNSConfig configures the DNS check. -type DNSConfig struct { - Domain string +// dnsConfig configures the DNS check. +type dnsConfig struct { + // Domain is the MANDATORY domain to resolve. + Domain string + + // NewResolver is the MANDATORY factory to create a new resolver. NewResolver func() model.Resolver - Out chan CtrlDNSResult - Wg *sync.WaitGroup + + // Out is the channel where we publish the results. + Out chan ctrlDNSResult + + // Wg allows to synchronize with the parent. + Wg *sync.WaitGroup } -// DNSDo performs the DNS check. -func DNSDo(ctx context.Context, config *DNSConfig) { +// dnsDo performs the DNS check. +func dnsDo(ctx context.Context, config *dnsConfig) { defer config.Wg.Done() reso := config.NewResolver() defer reso.CloseIdleConnections() @@ -35,7 +46,7 @@ func DNSDo(ctx context.Context, config *DNSConfig) { addrs = []string{} // fix: the old test helper did that } failure := dnsMapFailure(newfailure(err)) - config.Out <- CtrlDNSResult{Failure: failure, Addrs: addrs} + config.Out <- ctrlDNSResult{Failure: failure, Addrs: addrs} } // dnsMapFailure attempts to map netxlite failures to the strings diff --git a/internal/cmd/oohelperd/internal/webconnectivity/dns_test.go b/internal/cmd/oohelperd/dns_test.go similarity index 97% rename from internal/cmd/oohelperd/internal/webconnectivity/dns_test.go rename to internal/cmd/oohelperd/dns_test.go index c2eb1eb..9823eb2 100644 --- a/internal/cmd/oohelperd/internal/webconnectivity/dns_test.go +++ b/internal/cmd/oohelperd/dns_test.go @@ -1,4 +1,4 @@ -package webconnectivity +package main import ( "context" @@ -67,7 +67,7 @@ func Test_dnsMapFailure(t *testing.T) { func TestDNSDo(t *testing.T) { t.Run("returns non-nil addresses list on nxdomin", func(t *testing.T) { ctx := context.Background() - config := &DNSConfig{ + config := &dnsConfig{ Domain: "antani.ooni.org", NewResolver: func() model.Resolver { return &mocks.Resolver{ @@ -83,7 +83,7 @@ func TestDNSDo(t *testing.T) { Wg: &sync.WaitGroup{}, } config.Wg.Add(1) - DNSDo(ctx, config) + dnsDo(ctx, config) config.Wg.Wait() resp := <-config.Out if resp.Addrs == nil || len(resp.Addrs) != 0 { diff --git a/internal/cmd/oohelperd/internal/webconnectivity/fake_test.go b/internal/cmd/oohelperd/fake_test.go similarity index 99% rename from internal/cmd/oohelperd/internal/webconnectivity/fake_test.go rename to internal/cmd/oohelperd/fake_test.go index e43bba0..0f93128 100644 --- a/internal/cmd/oohelperd/internal/webconnectivity/fake_test.go +++ b/internal/cmd/oohelperd/fake_test.go @@ -1,4 +1,4 @@ -package webconnectivity +package main import ( "context" diff --git a/internal/cmd/oohelperd/internal/webconnectivity/webconnectivity.go b/internal/cmd/oohelperd/handler.go similarity index 56% rename from internal/cmd/oohelperd/internal/webconnectivity/webconnectivity.go rename to internal/cmd/oohelperd/handler.go index 1bd9ab1..691f893 100644 --- a/internal/cmd/oohelperd/internal/webconnectivity/webconnectivity.go +++ b/internal/cmd/oohelperd/handler.go @@ -1,4 +1,8 @@ -package webconnectivity +package main + +// +// HTTP handler +// import ( "encoding/json" @@ -12,15 +16,25 @@ import ( "github.com/ooni/probe-cli/v3/internal/version" ) -// Handler implements the Web Connectivity test helper HTTP API. -type Handler struct { +// handler implements the Web Connectivity test helper HTTP API. +type handler struct { + // MaxAcceptableBody is the MANDATORY maximum acceptable response body. MaxAcceptableBody int64 - NewClient func() model.HTTPClient - NewDialer func() model.Dialer - NewResolver func() model.Resolver + + // NewClient is the MANDATORY factory to create a new HTTPClient. + NewClient func() model.HTTPClient + + // NewDialer is the MANDATORY factory to create a new Dialer. + NewDialer func() model.Dialer + + // NewResolver is the MANDATORY factory for creating a new resolver. + NewResolver func() model.Resolver } -func (h Handler) ServeHTTP(w http.ResponseWriter, req *http.Request) { +var _ http.Handler = &handler{} + +// ServeHTTP implements http.Handler.ServeHTTP. +func (h *handler) ServeHTTP(w http.ResponseWriter, req *http.Request) { w.Header().Add("Server", fmt.Sprintf( "oohelperd/%s ooniprobe-engine/%s", version.Version, version.Version, )) @@ -34,19 +48,18 @@ func (h Handler) ServeHTTP(w http.ResponseWriter, req *http.Request) { w.WriteHeader(400) return } - var creq CtrlRequest + var creq ctrlRequest if err := json.Unmarshal(data, &creq); err != nil { w.WriteHeader(400) return } - measureConfig := MeasureConfig(h) - cresp, err := Measure(req.Context(), measureConfig, &creq) + cresp, err := measure(req.Context(), h, &creq) if err != nil { w.WriteHeader(400) return } // We assume that the following call cannot fail because it's a - // clearly serializable data structure. + // clearly-serializable data structure. data, err = json.Marshal(cresp) runtimex.PanicOnError(err, "json.Marshal failed") w.Header().Add("Content-Type", "application/json") diff --git a/internal/cmd/oohelperd/internal/webconnectivity/webconnectivity_test.go b/internal/cmd/oohelperd/handler_test.go similarity index 97% rename from internal/cmd/oohelperd/internal/webconnectivity/webconnectivity_test.go rename to internal/cmd/oohelperd/handler_test.go index d7f88f0..e523826 100644 --- a/internal/cmd/oohelperd/internal/webconnectivity/webconnectivity_test.go +++ b/internal/cmd/oohelperd/handler_test.go @@ -1,4 +1,4 @@ -package webconnectivity +package main import ( "context" @@ -50,7 +50,7 @@ const requestWithoutDomainName = `{ }` func TestWorkingAsIntended(t *testing.T) { - handler := Handler{ + handler := &handler{ MaxAcceptableBody: 1 << 24, NewClient: func() model.HTTPClient { return http.DefaultClient @@ -148,7 +148,7 @@ func TestWorkingAsIntended(t *testing.T) { func TestHandlerWithRequestBodyReadingError(t *testing.T) { expected := errors.New("mocked error") - handler := Handler{MaxAcceptableBody: 1 << 24} + handler := handler{MaxAcceptableBody: 1 << 24} rw := NewFakeResponseWriter() req := &http.Request{ Method: "POST", diff --git a/internal/cmd/oohelperd/internal/webconnectivity/http.go b/internal/cmd/oohelperd/http.go similarity index 74% rename from internal/cmd/oohelperd/internal/webconnectivity/http.go rename to internal/cmd/oohelperd/http.go index ed39f2c..d177d3c 100644 --- a/internal/cmd/oohelperd/internal/webconnectivity/http.go +++ b/internal/cmd/oohelperd/http.go @@ -1,4 +1,8 @@ -package webconnectivity +package main + +// +// HTTP measurements +// import ( "context" @@ -13,26 +17,37 @@ import ( "github.com/ooni/probe-cli/v3/internal/tracex" ) -// CtrlHTTPResponse is the result of the HTTP check performed by +// ctrlHTTPResponse is the result of the HTTP check performed by // the Web Connectivity test helper. -type CtrlHTTPResponse = webconnectivity.ControlHTTPRequestResult +type ctrlHTTPResponse = webconnectivity.ControlHTTPRequestResult -// HTTPConfig configures the HTTP check. -type HTTPConfig struct { - Headers map[string][]string +// httpConfig configures the HTTP check. +type httpConfig struct { + // Headers is OPTIONAL and contains the request headers we should set. + Headers map[string][]string + + // MaxAcceptableBody is MANDATORY and specifies the maximum acceptable body size. MaxAcceptableBody int64 - NewClient func() model.HTTPClient - Out chan CtrlHTTPResponse - URL string - Wg *sync.WaitGroup + + // NewClient is the MANDATORY factory to create a new client. + NewClient func() model.HTTPClient + + // Out is the MANDATORY channel where we'll post results. + Out chan ctrlHTTPResponse + + // URL is the MANDATORY URL to measure. + URL string + + // Wg is MANDATORY and allows synchronizing with parent. + Wg *sync.WaitGroup } -// HTTPDo performs the HTTP check. -func HTTPDo(ctx context.Context, config *HTTPConfig) { +// httpDo performs the HTTP check. +func httpDo(ctx context.Context, config *httpConfig) { defer config.Wg.Done() req, err := http.NewRequestWithContext(ctx, "GET", config.URL, nil) if err != nil { - config.Out <- CtrlHTTPResponse{ // fix: emit -1 like the old test helper does + config.Out <- ctrlHTTPResponse{ // fix: emit -1 like the old test helper does BodyLength: -1, Failure: httpMapFailure(err), StatusCode: -1, @@ -54,7 +69,7 @@ func HTTPDo(ctx context.Context, config *HTTPConfig) { defer clnt.CloseIdleConnections() resp, err := clnt.Do(req) if err != nil { - config.Out <- CtrlHTTPResponse{ // fix: emit -1 like old test helper does + config.Out <- ctrlHTTPResponse{ // fix: emit -1 like old test helper does BodyLength: -1, Failure: httpMapFailure(err), StatusCode: -1, @@ -69,7 +84,7 @@ func HTTPDo(ctx context.Context, config *HTTPConfig) { } reader := &io.LimitedReader{R: resp.Body, N: config.MaxAcceptableBody} data, err := netxlite.ReadAllContext(ctx, reader) - config.Out <- CtrlHTTPResponse{ + config.Out <- ctrlHTTPResponse{ BodyLength: int64(len(data)), Failure: httpMapFailure(err), StatusCode: int64(resp.StatusCode), diff --git a/internal/cmd/oohelperd/internal/webconnectivity/http_test.go b/internal/cmd/oohelperd/http_test.go similarity index 95% rename from internal/cmd/oohelperd/internal/webconnectivity/http_test.go rename to internal/cmd/oohelperd/http_test.go index 0c97a14..c51d8f8 100644 --- a/internal/cmd/oohelperd/internal/webconnectivity/http_test.go +++ b/internal/cmd/oohelperd/http_test.go @@ -1,4 +1,4 @@ -package webconnectivity +package main import ( "context" @@ -15,9 +15,9 @@ import ( func TestHTTPDoWithInvalidURL(t *testing.T) { ctx := context.Background() wg := new(sync.WaitGroup) - httpch := make(chan CtrlHTTPResponse, 1) + httpch := make(chan ctrlHTTPResponse, 1) wg.Add(1) - go HTTPDo(ctx, &HTTPConfig{ + go httpDo(ctx, &httpConfig{ Headers: nil, MaxAcceptableBody: 1 << 24, NewClient: func() model.HTTPClient { @@ -39,9 +39,9 @@ func TestHTTPDoWithHTTPTransportFailure(t *testing.T) { expected := errors.New("mocked error") ctx := context.Background() wg := new(sync.WaitGroup) - httpch := make(chan CtrlHTTPResponse, 1) + httpch := make(chan ctrlHTTPResponse, 1) wg.Add(1) - go HTTPDo(ctx, &HTTPConfig{ + go httpDo(ctx, &httpConfig{ Headers: nil, MaxAcceptableBody: 1 << 24, NewClient: func() model.HTTPClient { diff --git a/internal/cmd/oohelperd/oohelperd.go b/internal/cmd/oohelperd/main.go similarity index 80% rename from internal/cmd/oohelperd/oohelperd.go rename to internal/cmd/oohelperd/main.go index 71c9f89..245562f 100644 --- a/internal/cmd/oohelperd/oohelperd.go +++ b/internal/cmd/oohelperd/main.go @@ -1,4 +1,4 @@ -// Command oohelperd contains the Web Connectivity test helper. +// Command oohelperd implements the Web Connectivity test helper. package main import ( @@ -9,7 +9,6 @@ import ( "time" "github.com/apex/log" - "github.com/ooni/probe-cli/v3/internal/cmd/oohelperd/internal/webconnectivity" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/netxlite" ) @@ -27,7 +26,7 @@ func init() { srvctx, srvcancel = context.WithCancel(context.Background()) } -func newresolver() model.Resolver { +func newResolver() model.Resolver { // Implementation note: pin to a specific resolver so we don't depend upon the // default resolver configured by the box. Also, use an encrypted transport thus // we're less vulnerable to any policy implemented by the box's provider. @@ -54,15 +53,15 @@ func main() { func testableMain() { mux := http.NewServeMux() - mux.Handle("/", webconnectivity.Handler{ + mux.Handle("/", &handler{ MaxAcceptableBody: maxAcceptableBody, NewClient: func() model.HTTPClient { - return netxlite.NewHTTPClientWithResolver(log.Log, newresolver()) + return netxlite.NewHTTPClientWithResolver(log.Log, newResolver()) }, NewDialer: func() model.Dialer { - return netxlite.NewDialerWithResolver(log.Log, newresolver()) + return netxlite.NewDialerWithResolver(log.Log, newResolver()) }, - NewResolver: newresolver, + NewResolver: newResolver, }) srv := &http.Server{Addr: *endpoint, Handler: mux} srvwg.Add(1) diff --git a/internal/cmd/oohelperd/oohelperd_test.go b/internal/cmd/oohelperd/main_test.go similarity index 100% rename from internal/cmd/oohelperd/oohelperd_test.go rename to internal/cmd/oohelperd/main_test.go diff --git a/internal/cmd/oohelperd/internal/webconnectivity/measure.go b/internal/cmd/oohelperd/measure.go similarity index 57% rename from internal/cmd/oohelperd/internal/webconnectivity/measure.go rename to internal/cmd/oohelperd/measure.go index d7f3158..5035642 100644 --- a/internal/cmd/oohelperd/internal/webconnectivity/measure.go +++ b/internal/cmd/oohelperd/measure.go @@ -1,4 +1,8 @@ -package webconnectivity +package main + +// +// Top-level measurement algorithm +// import ( "context" @@ -7,60 +11,54 @@ import ( "sync" "github.com/ooni/probe-cli/v3/internal/engine/experiment/webconnectivity" - "github.com/ooni/probe-cli/v3/internal/model" ) type ( - // CtrlRequest is the request sent to the test helper - CtrlRequest = webconnectivity.ControlRequest + // ctrlRequest is the request sent to the test helper + ctrlRequest = webconnectivity.ControlRequest - // CtrlResponse is the response from the test helper - CtrlResponse = webconnectivity.ControlResponse + // ctrlResponse is the response from the test helper + ctrlResponse = webconnectivity.ControlResponse ) -// MeasureConfig contains configuration for Measure. -type MeasureConfig struct { - MaxAcceptableBody int64 - NewClient func() model.HTTPClient - NewDialer func() model.Dialer - NewResolver func() model.Resolver -} - -// Measure performs the measurement described by the request and +// measure performs the measurement described by the request and // returns the corresponding response or an error. -func Measure(ctx context.Context, config MeasureConfig, creq *CtrlRequest) (*CtrlResponse, error) { +func measure(ctx context.Context, config *handler, creq *ctrlRequest) (*ctrlResponse, error) { // parse input for correctness URL, err := url.Parse(creq.HTTPRequest) if err != nil { return nil, err } + wg := &sync.WaitGroup{} + // dns: start - wg := new(sync.WaitGroup) - dnsch := make(chan CtrlDNSResult, 1) + dnsch := make(chan ctrlDNSResult, 1) if net.ParseIP(URL.Hostname()) == nil { wg.Add(1) - go DNSDo(ctx, &DNSConfig{ + go dnsDo(ctx, &dnsConfig{ Domain: URL.Hostname(), NewResolver: config.NewResolver, Out: dnsch, Wg: wg, }) } + // tcpconnect: start - tcpconnch := make(chan TCPResultPair, len(creq.TCPConnect)) + tcpconnch := make(chan tcpResultPair, len(creq.TCPConnect)) for _, endpoint := range creq.TCPConnect { wg.Add(1) - go TCPDo(ctx, &TCPConfig{ + go tcpDo(ctx, &tcpConfig{ Endpoint: endpoint, NewDialer: config.NewDialer, Out: tcpconnch, Wg: wg, }) } + // http: start - httpch := make(chan CtrlHTTPResponse, 1) + httpch := make(chan ctrlHTTPResponse, 1) wg.Add(1) - go HTTPDo(ctx, &HTTPConfig{ + go httpDo(ctx, &httpConfig{ Headers: creq.HTTPRequestHeaders, MaxAcceptableBody: config.MaxAcceptableBody, NewClient: config.NewClient, @@ -68,25 +66,28 @@ func Measure(ctx context.Context, config MeasureConfig, creq *CtrlRequest) (*Ctr URL: creq.HTTPRequest, Wg: wg, }) + // wait for measurement steps to complete wg.Wait() + // assemble response - cresp := new(CtrlResponse) + cresp := new(ctrlResponse) select { case cresp.DNS = <-dnsch: default: // we need to emit a non-nil Addrs to match exactly // the behavior of the legacy TH - cresp.DNS = CtrlDNSResult{ + cresp.DNS = ctrlDNSResult{ Failure: nil, Addrs: []string{}, } } cresp.HTTPRequest = <-httpch - cresp.TCPConnect = make(map[string]CtrlTCPResult) + cresp.TCPConnect = make(map[string]ctrlTCPResult) for len(cresp.TCPConnect) < len(creq.TCPConnect) { tcpconn := <-tcpconnch cresp.TCPConnect[tcpconn.Endpoint] = tcpconn.Result } + return cresp, nil } diff --git a/internal/cmd/oohelperd/internal/webconnectivity/tcpconnect.go b/internal/cmd/oohelperd/tcpconnect.go similarity index 60% rename from internal/cmd/oohelperd/internal/webconnectivity/tcpconnect.go rename to internal/cmd/oohelperd/tcpconnect.go index 21afa26..8611893 100644 --- a/internal/cmd/oohelperd/internal/webconnectivity/tcpconnect.go +++ b/internal/cmd/oohelperd/tcpconnect.go @@ -1,4 +1,8 @@ -package webconnectivity +package main + +// +// TCP connect measurements +// import ( "context" @@ -9,25 +13,35 @@ import ( "github.com/ooni/probe-cli/v3/internal/netxlite" ) -// CtrlTCPResult is the result of the TCP check performed by the test helper. -type CtrlTCPResult = webconnectivity.ControlTCPConnectResult +// ctrlTCPResult is the result of the TCP check performed by the test helper. +type ctrlTCPResult = webconnectivity.ControlTCPConnectResult -// TCPResultPair contains the endpoint and the corresponding result. -type TCPResultPair struct { +// tcpResultPair contains the endpoint and the corresponding result. +type tcpResultPair struct { + // Endpoint is the endpoint we measured. Endpoint string - Result CtrlTCPResult + + // Result contains the results. + Result ctrlTCPResult } -// TCPConfig configures the TCP connect check. -type TCPConfig struct { - Endpoint string +// tcpConfig configures the TCP connect check. +type tcpConfig struct { + // Endpoint is the MANDATORY endpoint to connect to. + Endpoint string + + // NewDialer is the MANDATORY factory for creating a new dialer. NewDialer func() model.Dialer - Out chan TCPResultPair - Wg *sync.WaitGroup + + // Out is the MANDATORY where we'll post the TCP measurement results. + Out chan tcpResultPair + + // Wg is MANDATORY and is used to sync with the parent. + Wg *sync.WaitGroup } -// TCPDo performs the TCP check. -func TCPDo(ctx context.Context, config *TCPConfig) { +// tcpDo performs the TCP check. +func tcpDo(ctx context.Context, config *tcpConfig) { defer config.Wg.Done() dialer := config.NewDialer() defer dialer.CloseIdleConnections() @@ -35,9 +49,9 @@ func TCPDo(ctx context.Context, config *TCPConfig) { if conn != nil { conn.Close() } - config.Out <- TCPResultPair{ + config.Out <- tcpResultPair{ Endpoint: config.Endpoint, - Result: CtrlTCPResult{ + Result: ctrlTCPResult{ Failure: tcpMapFailure(newfailure(err)), Status: err == nil, }, diff --git a/internal/cmd/oohelperd/internal/webconnectivity/tcpconnect_test.go b/internal/cmd/oohelperd/tcpconnect_test.go similarity index 97% rename from internal/cmd/oohelperd/internal/webconnectivity/tcpconnect_test.go rename to internal/cmd/oohelperd/tcpconnect_test.go index bb2fb30..a7f596a 100644 --- a/internal/cmd/oohelperd/internal/webconnectivity/tcpconnect_test.go +++ b/internal/cmd/oohelperd/tcpconnect_test.go @@ -1,4 +1,4 @@ -package webconnectivity +package main import ( "testing"