Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(oohelperd,netmx): reduce construction diff to zero #1467

Merged
merged 1 commit into from
Jan 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion internal/cmd/oohelperd/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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}
Expand Down
21 changes: 6 additions & 15 deletions internal/netemx/oohelperd.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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))
Expand All @@ -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,
)
}

Expand Down
15 changes: 7 additions & 8 deletions internal/oohelperd/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)
},
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion internal/oohelperd/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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
}
Expand Down