engine: reduce usage of quirky netxlite code #2534
Assignees
Labels
bug
Something isn't working
ooni/probe-engine
priority/high
techdebt
This issue describes technical debt
Comments
bassosimone
added
bug
bassosimone
added a commit
to ooni/probe-cli
that referenced
this issue
Sep 14, 2023
This diff isolates and annotates netxlite quirky functions such that ooni/probe#2534 will be easier. This work is also useful to ooni/probe#2531. While there, commit workaround for issue ooni/probe#2535.
bassosimone
added a commit
to ooni/probe-cli
that referenced
this issue
Oct 9, 2023
By using netxlite.NewHTTPTransportWithOptions: 1. we remove an unnecessary usage of the quirky HTTP transport used for measuring by ./legacy/netx et al (removing such unnecessary usages is ooni/probe#2534); 2. we enable using HTTP/HTTPS proxies in miniooni and ooniprobe. Closes ooni/probe#1955.
bassosimone
added a commit
to ooni/probe-cli
that referenced
this issue
Oct 9, 2023
By using netxlite.NewHTTPTransportWithOptions in `./internal/engineresolver/factory.go`: 1. we remove an unnecessary usage of the quirky HTTP transport used for measuring by ./legacy/netx et al (removing such unnecessary usages is ooni/probe#2534); 2. we enable using HTTP/HTTPS proxies in miniooni and ooniprobe. Closes ooni/probe#1955.
bassosimone
added a commit
to ooni/probe-cli
that referenced
this issue
Jan 24, 2024
This diff modifies oohelperd and netemx to ensure we construct
equivalent HTTPTransports.
Let's show that this is actually the case.
Let's start from the HTTP/1.1 and HTTP2 transport.
This is what `oohelperd` does after this diff:
```Go
NewHTTPClient: func(logger model.Logger) model.HTTPClient {
// TODO(ooni/probe#2534): the NewHTTPTransportWithResolver has QUIRKS and
// we should evaluate whether we can avoid using it here
return NewHTTPClientWithTransportFactory(
logger,
netxlite.NewHTTPTransportWithResolver,
)
},
```
This is what `netemx` does after this diff:
```Go
handler.NewHTTPClient = func(logger model.Logger) model.HTTPClient {
return oohelperd.NewHTTPClientWithTransportFactory(
logger,
func(dl model.DebugLogger, r model.Resolver) model.HTTPTransport {
dialer := netx.NewDialerWithResolver(dl, r)
tlsDialer := netxlite.NewTLSDialer(dialer, netx.NewTLSHandshakerStdlib(dl))
// TODO(ooni/probe#2534): NewHTTPTransport is QUIRKY but
// we probably don't care about using a QUIRKY function here
return netxlite.NewHTTPTransport(dl, dialer, tlsDialer)
},
)
}
```
We're using the same (now public) `NewHTTPClientWithTransportFactory`
function. So, what remains to be done to reach a QED is to show that
this code called by `oohelperd`:
```Go
return NewHTTPClientWithTransportFactory(
logger,
netxlite.NewHTTPTransportWithResolver,
)
```
is equivalent to this code called by `netemx`:
```Go
func(dl model.DebugLogger, r model.Resolver) model.HTTPTransport {
dialer := netx.NewDialerWithResolver(dl, r)
tlsDialer := netxlite.NewTLSDialer(dialer, netx.NewTLSHandshakerStdlib(dl))
// TODO(ooni/probe#2534): NewHTTPTransport is QUIRKY but
// we probably don't care about using a QUIRKY function here
return netxlite.NewHTTPTransport(dl, dialer, tlsDialer)
},
```
This is evident if we expand `netxlite.NewHTTPTransportWithResolver`,
whose implementation is:
```Go
func NewHTTPTransportWithResolver(logger model.DebugLogger, reso model.Resolver) model.HTTPTransport {
dialer := NewDialerWithResolver(logger, reso)
thx := NewTLSHandshakerStdlib(logger)
tlsDialer := NewTLSDialer(dialer, thx)
return NewHTTPTransport(logger, dialer, tlsDialer)
}
```
in fact, the following lines of code called from `oohelperd`:
```Go
dialer := NewDialerWithResolver(logger, reso)
thx := NewTLSHandshakerStdlib(logger)
tlsDialer := NewTLSDialer(dialer, thx)
return NewHTTPTransport(logger, dialer, tlsDialer)
```
are equivalent to this `netemx` code:
```Go
dialer := netx.NewDialerWithResolver(dl, r)
tlsDialer := netxlite.NewTLSDialer(dialer, netx.NewTLSHandshakerStdlib(dl))
// TODO(ooni/probe#2534): NewHTTPTransport is QUIRKY but
// we probably don't care about using a QUIRKY function here
return netxlite.NewHTTPTransport(dl, dialer, tlsDialer)
```
Modulo the fact that `netemx` code is using methods of the
`*netxlite.Netx` structure rather than bare functions.
Let's now inspect how we construct HTTP3.
This is what `oohelperd` does:
```Go
NewHTTP3Client: func(logger model.Logger) model.HTTPClient {
return NewHTTPClientWithTransportFactory(
logger,
netxlite.NewHTTP3TransportWithResolver,
)
},
```
This is what `netemx` does:
```Go
handler.NewHTTP3Client = func(logger model.Logger) model.HTTPClient {
return oohelperd.NewHTTPClientWithTransportFactory(
logger,
func(dl model.DebugLogger, r model.Resolver) model.HTTPTransport {
qd := netx.NewQUICDialerWithResolver(netx.NewUDPListener(), dl, r)
return netxlite.NewHTTP3Transport(dl, qd, nil)
},
)
}
```
Because we're using the same `NewHTTPClientWithTransportFactory`
factory, we need to show that `oohelperd`'s
```Go
return NewHTTPClientWithTransportFactory(
logger,
netxlite.NewHTTP3TransportWithResolver,
)
```
is equivalent to `netemx`'s
```Go
return oohelperd.NewHTTPClientWithTransportFactory(
logger,
func(dl model.DebugLogger, r model.Resolver) model.HTTPTransport {
qd := netx.NewQUICDialerWithResolver(netx.NewUDPListener(), dl, r)
return netxlite.NewHTTP3Transport(dl, qd, nil)
},
)
```
To show that we need to expand `NewHTTP3TransportWithResolver`, which
reads:
```Go
func NewHTTP3TransportWithResolver(logger model.DebugLogger, reso model.Resolver) model.HTTPTransport {
qd := NewQUICDialerWithResolver(NewUDPListener(), logger, reso)
return NewHTTP3Transport(logger, qd, nil)
}
```
And then we can conclude that we're good because the code invoked by
`oohelperd`:
```Go
qd := NewQUICDialerWithResolver(NewUDPListener(), logger, reso)
return NewHTTP3Transport(logger, qd, nil)
```
is equivalent to the code invoked by `netemx`:
```Go
qd := netx.NewQUICDialerWithResolver(netx.NewUDPListener(), dl, r)
return netxlite.NewHTTP3Transport(dl, qd, nil)
```
modulo the fact that `netemx` is using methods defined by the `netx`
object.
Extracted from #1462.
Part of ooni/probe#2652.
Part of ooni/probe#1517.
Murphy-OrangeMud
pushed a commit
to Murphy-OrangeMud/probe-cli
that referenced
this issue
Feb 13, 2024
This diff isolates and annotates netxlite quirky functions such that ooni/probe#2534 will be easier. This work is also useful to ooni/probe#2531. While there, commit workaround for issue ooni/probe#2535.
Murphy-OrangeMud
pushed a commit
to Murphy-OrangeMud/probe-cli
that referenced
this issue
Feb 13, 2024
By using netxlite.NewHTTPTransportWithOptions in `./internal/engineresolver/factory.go`: 1. we remove an unnecessary usage of the quirky HTTP transport used for measuring by ./legacy/netx et al (removing such unnecessary usages is ooni/probe#2534); 2. we enable using HTTP/HTTPS proxies in miniooni and ooniprobe. Closes ooni/probe#1955.
Murphy-OrangeMud
pushed a commit
to Murphy-OrangeMud/probe-cli
that referenced
this issue
Feb 13, 2024
This diff modifies oohelperd and netemx to ensure we construct
equivalent HTTPTransports.
Let's show that this is actually the case.
Let's start from the HTTP/1.1 and HTTP2 transport.
This is what `oohelperd` does after this diff:
```Go
NewHTTPClient: func(logger model.Logger) model.HTTPClient {
// TODO(ooni/probe#2534): the NewHTTPTransportWithResolver has QUIRKS and
// we should evaluate whether we can avoid using it here
return NewHTTPClientWithTransportFactory(
logger,
netxlite.NewHTTPTransportWithResolver,
)
},
```
This is what `netemx` does after this diff:
```Go
handler.NewHTTPClient = func(logger model.Logger) model.HTTPClient {
return oohelperd.NewHTTPClientWithTransportFactory(
logger,
func(dl model.DebugLogger, r model.Resolver) model.HTTPTransport {
dialer := netx.NewDialerWithResolver(dl, r)
tlsDialer := netxlite.NewTLSDialer(dialer, netx.NewTLSHandshakerStdlib(dl))
// TODO(ooni/probe#2534): NewHTTPTransport is QUIRKY but
// we probably don't care about using a QUIRKY function here
return netxlite.NewHTTPTransport(dl, dialer, tlsDialer)
},
)
}
```
We're using the same (now public) `NewHTTPClientWithTransportFactory`
function. So, what remains to be done to reach a QED is to show that
this code called by `oohelperd`:
```Go
return NewHTTPClientWithTransportFactory(
logger,
netxlite.NewHTTPTransportWithResolver,
)
```
is equivalent to this code called by `netemx`:
```Go
func(dl model.DebugLogger, r model.Resolver) model.HTTPTransport {
dialer := netx.NewDialerWithResolver(dl, r)
tlsDialer := netxlite.NewTLSDialer(dialer, netx.NewTLSHandshakerStdlib(dl))
// TODO(ooni/probe#2534): NewHTTPTransport is QUIRKY but
// we probably don't care about using a QUIRKY function here
return netxlite.NewHTTPTransport(dl, dialer, tlsDialer)
},
```
This is evident if we expand `netxlite.NewHTTPTransportWithResolver`,
whose implementation is:
```Go
func NewHTTPTransportWithResolver(logger model.DebugLogger, reso model.Resolver) model.HTTPTransport {
dialer := NewDialerWithResolver(logger, reso)
thx := NewTLSHandshakerStdlib(logger)
tlsDialer := NewTLSDialer(dialer, thx)
return NewHTTPTransport(logger, dialer, tlsDialer)
}
```
in fact, the following lines of code called from `oohelperd`:
```Go
dialer := NewDialerWithResolver(logger, reso)
thx := NewTLSHandshakerStdlib(logger)
tlsDialer := NewTLSDialer(dialer, thx)
return NewHTTPTransport(logger, dialer, tlsDialer)
```
are equivalent to this `netemx` code:
```Go
dialer := netx.NewDialerWithResolver(dl, r)
tlsDialer := netxlite.NewTLSDialer(dialer, netx.NewTLSHandshakerStdlib(dl))
// TODO(ooni/probe#2534): NewHTTPTransport is QUIRKY but
// we probably don't care about using a QUIRKY function here
return netxlite.NewHTTPTransport(dl, dialer, tlsDialer)
```
Modulo the fact that `netemx` code is using methods of the
`*netxlite.Netx` structure rather than bare functions.
Let's now inspect how we construct HTTP3.
This is what `oohelperd` does:
```Go
NewHTTP3Client: func(logger model.Logger) model.HTTPClient {
return NewHTTPClientWithTransportFactory(
logger,
netxlite.NewHTTP3TransportWithResolver,
)
},
```
This is what `netemx` does:
```Go
handler.NewHTTP3Client = func(logger model.Logger) model.HTTPClient {
return oohelperd.NewHTTPClientWithTransportFactory(
logger,
func(dl model.DebugLogger, r model.Resolver) model.HTTPTransport {
qd := netx.NewQUICDialerWithResolver(netx.NewUDPListener(), dl, r)
return netxlite.NewHTTP3Transport(dl, qd, nil)
},
)
}
```
Because we're using the same `NewHTTPClientWithTransportFactory`
factory, we need to show that `oohelperd`'s
```Go
return NewHTTPClientWithTransportFactory(
logger,
netxlite.NewHTTP3TransportWithResolver,
)
```
is equivalent to `netemx`'s
```Go
return oohelperd.NewHTTPClientWithTransportFactory(
logger,
func(dl model.DebugLogger, r model.Resolver) model.HTTPTransport {
qd := netx.NewQUICDialerWithResolver(netx.NewUDPListener(), dl, r)
return netxlite.NewHTTP3Transport(dl, qd, nil)
},
)
```
To show that we need to expand `NewHTTP3TransportWithResolver`, which
reads:
```Go
func NewHTTP3TransportWithResolver(logger model.DebugLogger, reso model.Resolver) model.HTTPTransport {
qd := NewQUICDialerWithResolver(NewUDPListener(), logger, reso)
return NewHTTP3Transport(logger, qd, nil)
}
```
And then we can conclude that we're good because the code invoked by
`oohelperd`:
```Go
qd := NewQUICDialerWithResolver(NewUDPListener(), logger, reso)
return NewHTTP3Transport(logger, qd, nil)
```
is equivalent to the code invoked by `netemx`:
```Go
qd := netx.NewQUICDialerWithResolver(netx.NewUDPListener(), dl, r)
return netxlite.NewHTTP3Transport(dl, qd, nil)
```
modulo the fact that `netemx` is using methods defined by the `netx`
object.
Extracted from ooni#1462.
Part of ooni/probe#2652.
Part of ooni/probe#1517.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
There are quirks inside netxlite that we cannot change because that would break ./legacy/netx. These quirks are true technical debt in the sense that constrain how we can change netxlite code. So, the general idea to ameliorate the situation is to try to annotate all the places that depend on quirky functions and see if we can substitute non quirky functions in there.
The text was updated successfully, but these errors were encountered: