From 8331a30d5cf6c9b51f2a2532acb6854cbca5ca69 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 24 Jan 2024 11:32:42 +0100 Subject: [PATCH] refactor(oohelperd,netmx): reduce construction diff to zero (#1467) This diff modifies how we construct oohelperd.Handler inside the oohelperd package and inside the netemx package such that we're now constructing using equivalent code. We know that construction is equivalent for HTTP clients because previosuly we made sure it was the case in https://github.com/ooni/probe-cli/pull/1464. So, the next step would be removing the custom construction code inside of netemx and always use oohelperd.NewHandler. In turn, by doing this, we would ensure we have the same `oohelperd` behavior for QA and production. In turn, with this guarantee, we can write QA tests that ensure we're correctly dealing with 127.0.0.1. The reference issue is https://github.com/ooni/probe/issues/1517. --- internal/cmd/oohelperd/main.go | 3 ++- internal/netemx/oohelperd.go | 21 ++++++--------------- internal/oohelperd/handler.go | 15 +++++++-------- internal/oohelperd/handler_test.go | 4 +++- 4 files changed, 18 insertions(+), 25 deletions(-) diff --git a/internal/cmd/oohelperd/main.go b/internal/cmd/oohelperd/main.go index f777157a1..fdc225d62 100644 --- a/internal/cmd/oohelperd/main.go +++ b/internal/cmd/oohelperd/main.go @@ -15,6 +15,7 @@ import ( "time" "github.com/apex/log" + "github.com/ooni/probe-cli/v3/internal/netxlite" "github.com/ooni/probe-cli/v3/internal/oohelperd" "github.com/ooni/probe-cli/v3/internal/runtimex" "github.com/ooni/probe-cli/v3/internal/version" @@ -92,7 +93,7 @@ func main() { mux := http.NewServeMux() // add the main oohelperd handler to the mux - mux.Handle("/", oohelperd.NewHandler()) + mux.Handle("/", oohelperd.NewHandler(log.Log, &netxlite.Netx{})) // create a listening server for serving ooniprobe requests srv := &http.Server{Addr: *apiEndpoint, Handler: mux} diff --git a/internal/netemx/oohelperd.go b/internal/netemx/oohelperd.go index fc22d5f56..793a7c19b 100644 --- a/internal/netemx/oohelperd.go +++ b/internal/netemx/oohelperd.go @@ -4,6 +4,7 @@ import ( "fmt" "net/http" + "github.com/apex/log" "github.com/ooni/netem" "github.com/ooni/probe-cli/v3/internal/logx" "github.com/ooni/probe-cli/v3/internal/model" @@ -20,12 +21,11 @@ var _ HTTPHandlerFactory = &OOHelperDFactory{} // NewHandler implements QAEnvHTTPHandlerFactory.NewHandler. func (f *OOHelperDFactory) NewHandler(env NetStackServerFactoryEnv, unet *netem.UNetStack) http.Handler { netx := &netxlite.Netx{Underlying: &netxlite.NetemUnderlyingNetworkAdapter{UNet: unet}} - handler := oohelperd.NewHandler() - - handler.BaseLogger = &logx.PrefixLogger{ + logger := &logx.PrefixLogger{ Prefix: fmt.Sprintf("%-16s", "TH_HANDLER"), - Logger: handler.BaseLogger, + Logger: log.Log, } + handler := oohelperd.NewHandler(logger, netx) handler.NewDialer = func(logger model.Logger) model.Dialer { return netx.NewDialerWithResolver(logger, netx.NewStdlibResolver(logger)) @@ -46,23 +46,14 @@ func (f *OOHelperDFactory) NewHandler(env NetStackServerFactoryEnv, unet *netem. handler.NewHTTPClient = func(logger model.Logger) model.HTTPClient { return oohelperd.NewHTTPClientWithTransportFactory( netx, logger, - func(netx *netxlite.Netx, dl model.DebugLogger, r model.Resolver) model.HTTPTransport { - dialer := netx.NewDialerWithResolver(dl, r) - tlsDialer := netxlite.NewTLSDialer(dialer, netx.NewTLSHandshakerStdlib(dl)) - // TODO(https://github.com/ooni/probe/issues/2534): NewHTTPTransport is QUIRKY but - // we probably don't care about using a QUIRKY function here - return netxlite.NewHTTPTransport(dl, dialer, tlsDialer) - }, + netxlite.NewHTTPTransportWithResolver, ) } handler.NewHTTP3Client = func(logger model.Logger) model.HTTPClient { return oohelperd.NewHTTPClientWithTransportFactory( netx, logger, - func(netx *netxlite.Netx, dl model.DebugLogger, r model.Resolver) model.HTTPTransport { - qd := netx.NewQUICDialerWithResolver(netx.NewUDPListener(), dl, r) - return netxlite.NewHTTP3Transport(dl, qd, nil) - }, + netxlite.NewHTTP3TransportWithResolver, ) } diff --git a/internal/oohelperd/handler.go b/internal/oohelperd/handler.go index 88e3c4a07..fe052f225 100644 --- a/internal/oohelperd/handler.go +++ b/internal/oohelperd/handler.go @@ -15,7 +15,6 @@ import ( "sync/atomic" "time" - "github.com/apex/log" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/netxlite" "github.com/ooni/probe-cli/v3/internal/runtimex" @@ -69,10 +68,9 @@ type Handler struct { var _ http.Handler = &Handler{} // NewHandler constructs the [handler]. -func NewHandler() *Handler { - netx := &netxlite.Netx{} +func NewHandler(logger model.Logger, netx *netxlite.Netx) *Handler { return &Handler{ - BaseLogger: log.Log, + BaseLogger: logger, CountRequests: &atomic.Int64{}, Indexer: &atomic.Int64{}, MaxAcceptableBody: MaxAcceptableBodySize, @@ -103,7 +101,9 @@ func NewHandler() *Handler { logger, ) }, - NewResolver: newResolver, + NewResolver: func(logger model.Logger) model.Resolver { + return newResolver(logger, netx) + }, NewTLSHandshaker: func(logger model.Logger) model.TLSHandshaker { return netx.NewTLSHandshakerStdlib(logger) }, @@ -202,11 +202,10 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, req *http.Request) { // newResolver creates a new [model.Resolver] suitable for serving // requests coming from ooniprobe clients. -func newResolver(logger model.Logger) model.Resolver { +func newResolver(logger model.Logger, netx *netxlite.Netx) 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. - netx := &netxlite.Netx{} resolver := netx.NewParallelDNSOverHTTPSResolver(logger, "https://dns.google/dns-query") return resolver } @@ -242,7 +241,7 @@ func NewHTTPClientWithTransportFactory( // So, it's better to consider this as a possible corner case. reso := netxlite.MaybeWrapWithBogonResolver( true, // enabled - newResolver(logger), + newResolver(logger, netx), ) // fix: We MUST set a cookie jar for measuring HTTP. See diff --git a/internal/oohelperd/handler_test.go b/internal/oohelperd/handler_test.go index 3b30fbb3c..64578ffaf 100644 --- a/internal/oohelperd/handler_test.go +++ b/internal/oohelperd/handler_test.go @@ -10,8 +10,10 @@ import ( "strings" "testing" + "github.com/apex/log" "github.com/ooni/probe-cli/v3/internal/mocks" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/netxlite" "github.com/ooni/probe-cli/v3/internal/version" ) @@ -186,7 +188,7 @@ func TestHandlerWorkingAsIntended(t *testing.T) { for _, expect := range expectations { t.Run(expect.name, func(t *testing.T) { // create handler and possibly override .Measure - handler := NewHandler() + handler := NewHandler(log.Log, &netxlite.Netx{}) if expect.measureFn != nil { handler.Measure = expect.measureFn }