diff --git a/internal/engine/netx/dialer.go b/internal/engine/netx/dialer.go index d6f98260e0..90c712f9d7 100644 --- a/internal/engine/netx/dialer.go +++ b/internal/engine/netx/dialer.go @@ -13,6 +13,7 @@ import ( // NewDialer creates a new Dialer from the specified config. func NewDialer(config Config) model.Dialer { if config.FullResolver == nil { + // TODO(https://github.com/ooni/probe/issues/2121#issuecomment-1147424810) config.FullResolver = NewResolver(config) } logger := model.ValidLoggerOrDefault(config.Logger) diff --git a/internal/engine/netx/dnstransport.go b/internal/engine/netx/dnstransport.go index 3812e34d3a..f2b2814f2c 100644 --- a/internal/engine/netx/dnstransport.go +++ b/internal/engine/netx/dnstransport.go @@ -6,6 +6,7 @@ package netx // TODO(bassosimone): this code should be refactored to return // a DNSTransport rather than a model.Resolver. With this in mind, // I've named this file dnstransport.go. +// TODO(https://github.com/ooni/probe/issues/2121#issuecomment-1147424810) // import ( @@ -45,6 +46,8 @@ func NewDNSClient(config Config, URL string) (model.Resolver, error) { // with the option to override the default Hostname and SNI. func NewDNSClientWithOverrides(config Config, URL, hostOverride, SNIOverride, TLSVersion string) (model.Resolver, error) { + // We should split this function in smaller and testable units + // TODO(https://github.com/ooni/probe/issues/2121#issuecomment-1147424810) switch URL { case "doh://powerdns": URL = "https://doh.powerdns.org/" @@ -132,6 +135,10 @@ func makeValidEndpoint(URL *url.URL) (string, error) { if _, _, err := net.SplitHostPort(URL.Host); err == nil { return URL.Host, nil } + + // Here we should add a test case for when the host is empty + // TODO(https://github.com/ooni/probe/issues/2121#issuecomment-1147424810) + // The second step is to assume that appending the default port // to a host parsed by url.Parse should be giving us a valid // endpoint. The possibilities in fact are: diff --git a/internal/engine/netx/http.go b/internal/engine/netx/http.go index 4c5fa0ee29..9f202a1fb4 100644 --- a/internal/engine/netx/http.go +++ b/internal/engine/netx/http.go @@ -14,12 +14,15 @@ import ( // NewHTTPTransport creates a new HTTPRoundTripper from the given Config. func NewHTTPTransport(config Config) model.HTTPTransport { if config.Dialer == nil { + // TODO(https://github.com/ooni/probe/issues/2121#issuecomment-1147424810) config.Dialer = NewDialer(config) } if config.TLSDialer == nil { + // TODO(https://github.com/ooni/probe/issues/2121#issuecomment-1147424810) config.TLSDialer = NewTLSDialer(config) } if config.QUICDialer == nil { + // TODO(https://github.com/ooni/probe/issues/2121#issuecomment-1147424810) config.QUICDialer = NewQUICDialer(config) } tInfo := allTransportsInfo[config.HTTP3Enabled] @@ -30,7 +33,8 @@ func NewHTTPTransport(config Config) model.HTTPTransport { TLSDialer: config.TLSDialer, TLSConfig: config.TLSConfig, }) - // TODO(bassosimone): I am not super convinced by this code because it + // TODO(https://github.com/ooni/probe/issues/2121#issuecomment-1147424810): I am + // not super convinced by this code because it // seems we're currently counting bytes twice in some cases. I think we // should review how we're counting bytes and using netx currently. txp = config.ByteCounter.MaybeWrapHTTPTransport(txp) // WAI with ByteCounter == nil @@ -46,7 +50,7 @@ type httpTransportInfo struct { var allTransportsInfo = map[bool]httpTransportInfo{ false: { - Factory: newSystemTransport, + Factory: newHTTPTransport, TransportName: "tcp", }, true: { @@ -56,6 +60,8 @@ var allTransportsInfo = map[bool]httpTransportInfo{ } // httpTransportConfig contains configuration for constructing an HTTPTransport. +// +// All the fields in this structure MUST be initialized. type httpTransportConfig struct { Dialer model.Dialer Logger model.Logger @@ -66,13 +72,10 @@ type httpTransportConfig struct { // newHTTP3Transport creates a new HTTP3Transport instance. func newHTTP3Transport(config httpTransportConfig) model.HTTPTransport { - // Rationale for using NoLogger here: previously this code did - // not use a logger as well, so it's fine to keep it as is. return netxlite.NewHTTP3Transport(config.Logger, config.QUICDialer, config.TLSConfig) } -// newSystemTransport creates a new "system" HTTP transport. That is a transport -// using the Go standard library with custom dialer and TLS dialer. -func newSystemTransport(config httpTransportConfig) model.HTTPTransport { +// newHTTPTransport creates a new "system" HTTP transport. +func newHTTPTransport(config httpTransportConfig) model.HTTPTransport { return netxlite.NewHTTPTransport(config.Logger, config.Dialer, config.TLSDialer) } diff --git a/internal/engine/netx/quic.go b/internal/engine/netx/quic.go index 4e90cca38c..e48de760d6 100644 --- a/internal/engine/netx/quic.go +++ b/internal/engine/netx/quic.go @@ -14,7 +14,8 @@ func NewQUICDialer(config Config) model.QUICDialer { if config.FullResolver == nil { config.FullResolver = NewResolver(config) } - // TODO(bassosimone): we should count the bytes consumed by this QUIC dialer + // TODO(https://github.com/ooni/probe/issues/2121#issuecomment-1147424810): we + // should count the bytes consumed by this QUIC dialer ql := config.ReadWriteSaver.WrapQUICListener(netxlite.NewQUICListener()) logger := model.ValidLoggerOrDefault(config.Logger) return netxlite.NewQUICDialerWithResolver(ql, logger, config.FullResolver, config.Saver)