c/ooni-probe-cli
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

tracex: prepare HTTP code for future refactoring (#778)

Browse Source 
The main issue I see inside tracex at the moment is that we
construct the HTTP measurement from separate events.

This is fragile because we cannot be sure that these events
belong to the same round trip. (Currently, they _are_ part
of the same round trip, but this is a fragile assumption and
it would be much more robust to dispose of it.)

To prepare for emitting a single event, it's imperative to
have two distinct fields for HTTP request and response headers,
which is the main contribution in this commit.

Then, we have a bunch of smaller changes including:

1. correctly naming 'response' the DNS response (instead of 'reply')

2. ensure we always use pointer receivers

Reference issue: https://github.com/ooni/probe/issues/2121
This commit is contained in:
Simone Basso 2022-06-01 15:20:28 +02:00 committed by GitHub
parent c740be987b
commit 66fd1569b8
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 69 additions and 68 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
internal/engine/netx/netx.go
View File

@ -187,11 +187,11 @@ func NewHTTPTransport(config Config) model.HTTPTransport {
txp = &netxlite.HTTPTransportLogger{Logger: config.Logger, HTTPTransport: txp} txp = &netxlite.HTTPTransportLogger{Logger: config.Logger, HTTPTransport: txp}
} }
if config.HTTPSaver != nil { if config.HTTPSaver != nil {
txp = tracex.SaverMetadataHTTPTransport{ txp = &tracex.SaverMetadataHTTPTransport{
HTTPTransport: txp, Saver: config.HTTPSaver} HTTPTransport: txp, Saver: config.HTTPSaver}
txp = tracex.SaverBodyHTTPTransport{ txp = &tracex.SaverBodyHTTPTransport{
HTTPTransport: txp, Saver: config.HTTPSaver} HTTPTransport: txp, Saver: config.HTTPSaver}
txp = tracex.SaverTransactionHTTPTransport{ txp = &tracex.SaverTransactionHTTPTransport{
HTTPTransport: txp, Saver: config.HTTPSaver} HTTPTransport: txp, Saver: config.HTTPSaver}
} }
return txp return txp

6
internal/engine/netx/netx_test.go
View File

@ -504,21 +504,21 @@ func TestNewWithSaver(t *testing.T) {
txp := netx.NewHTTPTransport(netx.Config{ txp := netx.NewHTTPTransport(netx.Config{
HTTPSaver: saver, HTTPSaver: saver,
}) })
stxptxp, ok := txp.(tracex.SaverTransactionHTTPTransport) stxptxp, ok := txp.(*tracex.SaverTransactionHTTPTransport)
if !ok { if !ok {
t.Fatal("not the transport we expected") t.Fatal("not the transport we expected")
} }
if stxptxp.Saver != saver { if stxptxp.Saver != saver {
t.Fatal("not the logger we expected") t.Fatal("not the logger we expected")
} }
sbtxp, ok := stxptxp.HTTPTransport.(tracex.SaverBodyHTTPTransport) sbtxp, ok := stxptxp.HTTPTransport.(*tracex.SaverBodyHTTPTransport)
if !ok { if !ok {
t.Fatal("not the transport we expected") t.Fatal("not the transport we expected")
} }
if sbtxp.Saver != saver { if sbtxp.Saver != saver {
t.Fatal("not the logger we expected") t.Fatal("not the logger we expected")
} }
smtxp, ok := sbtxp.HTTPTransport.(tracex.SaverMetadataHTTPTransport) smtxp, ok := sbtxp.HTTPTransport.(*tracex.SaverMetadataHTTPTransport)
if !ok { if !ok {
t.Fatal("not the transport we expected") t.Fatal("not the transport we expected")
} }

6
internal/engine/netx/tracex/archival.go
View File

@ -153,16 +153,16 @@ func newRequestList(begin time.Time, events []Event) []RequestEntry {
case *EventHTTPRequestMetadata: case *EventHTTPRequestMetadata:
entry.Request.Headers = make(map[string]MaybeBinaryValue) entry.Request.Headers = make(map[string]MaybeBinaryValue)
httpAddHeaders( httpAddHeaders(
ev.HTTPHeaders, &entry.Request.HeadersList, &entry.Request.Headers) ev.HTTPRequestHeaders, &entry.Request.HeadersList, &entry.Request.Headers)
entry.Request.Method = ev.HTTPMethod entry.Request.Method = ev.HTTPMethod
entry.Request.URL = ev.HTTPURL entry.Request.URL = ev.HTTPURL
entry.Request.Transport = ev.Transport entry.Request.Transport = ev.Transport
case *EventHTTPResponseMetadata: case *EventHTTPResponseMetadata:
entry.Response.Headers = make(map[string]MaybeBinaryValue) entry.Response.Headers = make(map[string]MaybeBinaryValue)
httpAddHeaders( httpAddHeaders(
ev.HTTPHeaders, &entry.Response.HeadersList, &entry.Response.Headers) ev.HTTPResponseHeaders, &entry.Response.HeadersList, &entry.Response.Headers)
entry.Response.Code = int64(ev.HTTPStatusCode) entry.Response.Code = int64(ev.HTTPStatusCode)
entry.Response.Locations = ev.HTTPHeaders.Values("Location") entry.Response.Locations = ev.HTTPResponseHeaders.Values("Location")
case *EventHTTPResponseBodySnapshot: case *EventHTTPResponseBodySnapshot:
entry.Response.Body.Value = string(ev.Data) entry.Response.Body.Value = string(ev.Data)
entry.Response.BodyIsTruncated = ev.DataIsTruncated entry.Response.BodyIsTruncated = ev.DataIsTruncated

10
internal/engine/netx/tracex/archival_test.go
View File

@ -149,13 +149,13 @@ func TestNewRequestList(t *testing.T) {
Data: []byte("deadbeef"), Data: []byte("deadbeef"),
DataIsTruncated: false, DataIsTruncated: false,
}}, &EventHTTPRequestMetadata{&EventValue{ }}, &EventHTTPRequestMetadata{&EventValue{
HTTPHeaders: http.Header{ HTTPRequestHeaders: http.Header{
"User-Agent": []string{"miniooni/0.1.0-dev"}, "User-Agent": []string{"miniooni/0.1.0-dev"},
}, },
HTTPMethod: "POST", HTTPMethod: "POST",
HTTPURL: "https://www.example.com/submit", HTTPURL: "https://www.example.com/submit",
}}, &EventHTTPResponseMetadata{&EventValue{ }}, &EventHTTPResponseMetadata{&EventValue{
HTTPHeaders: http.Header{ HTTPResponseHeaders: http.Header{
"Server": []string{"miniooni/0.1.0-dev"}, "Server": []string{"miniooni/0.1.0-dev"},
}, },
HTTPStatusCode: 200, HTTPStatusCode: 200,
@ -165,7 +165,7 @@ func TestNewRequestList(t *testing.T) {
}}, &EventHTTPTransactionDone{&EventValue{}}, &EventHTTPTransactionStart{&EventValue{ }}, &EventHTTPTransactionDone{&EventValue{}}, &EventHTTPTransactionStart{&EventValue{
Time: begin.Add(20 * time.Millisecond), Time: begin.Add(20 * time.Millisecond),
}}, &EventHTTPRequestMetadata{&EventValue{ }}, &EventHTTPRequestMetadata{&EventValue{
HTTPHeaders: http.Header{ HTTPRequestHeaders: http.Header{
"User-Agent": []string{"miniooni/0.1.0-dev"}, "User-Agent": []string{"miniooni/0.1.0-dev"},
}, },
HTTPMethod: "GET", HTTPMethod: "GET",
@ -234,13 +234,13 @@ func TestNewRequestList(t *testing.T) {
events: []Event{&EventHTTPTransactionStart{&EventValue{ events: []Event{&EventHTTPTransactionStart{&EventValue{
Time: begin.Add(10 * time.Millisecond), Time: begin.Add(10 * time.Millisecond),
}}, &EventHTTPRequestMetadata{&EventValue{ }}, &EventHTTPRequestMetadata{&EventValue{
HTTPHeaders: http.Header{ HTTPRequestHeaders: http.Header{
"User-Agent": []string{"miniooni/0.1.0-dev"}, "User-Agent": []string{"miniooni/0.1.0-dev"},
}, },
HTTPMethod: "GET", HTTPMethod: "GET",
HTTPURL: "https://www.example.com/", HTTPURL: "https://www.example.com/",
}}, &EventHTTPResponseMetadata{&EventValue{ }}, &EventHTTPResponseMetadata{&EventValue{
HTTPHeaders: http.Header{ HTTPResponseHeaders: http.Header{
"Server": []string{"miniooni/0.1.0-dev"}, "Server": []string{"miniooni/0.1.0-dev"},
"Location": []string{"https://x.example.com", "https://y.example.com"}, "Location": []string{"https://x.example.com", "https://y.example.com"},
}, },

49
internal/engine/netx/tracex/event.go
View File

@ -266,28 +266,29 @@ func (ev *EventWriteOperation) Name() string {
// Event is one of the events within a trace // Event is one of the events within a trace
type EventValue struct { type EventValue struct {
Addresses []string `json:",omitempty"` Addresses []string `json:",omitempty"`
Address string `json:",omitempty"` Address string `json:",omitempty"`
DNSQuery []byte `json:",omitempty"` DNSQuery []byte `json:",omitempty"`
DNSReply []byte `json:",omitempty"` DNSResponse []byte `json:",omitempty"`
DataIsTruncated bool `json:",omitempty"` DataIsTruncated bool `json:",omitempty"`
Data []byte `json:",omitempty"` Data []byte `json:",omitempty"`
Duration time.Duration `json:",omitempty"` Duration time.Duration `json:",omitempty"`
Err error `json:",omitempty"` Err error `json:",omitempty"`
HTTPHeaders http.Header `json:",omitempty"` HTTPMethod string `json:",omitempty"`
HTTPMethod string `json:",omitempty"` HTTPRequestHeaders http.Header `json:",omitempty"`
HTTPStatusCode int `json:",omitempty"` HTTPResponseHeaders http.Header `json:",omitempty"`
HTTPURL string `json:",omitempty"` HTTPStatusCode int `json:",omitempty"`
Hostname string `json:",omitempty"` HTTPURL string `json:",omitempty"`
NoTLSVerify bool `json:",omitempty"` Hostname string `json:",omitempty"`
NumBytes int `json:",omitempty"` NoTLSVerify bool `json:",omitempty"`
Proto string `json:",omitempty"` NumBytes int `json:",omitempty"`
TLSServerName string `json:",omitempty"` Proto string `json:",omitempty"`
TLSCipherSuite string `json:",omitempty"` TLSServerName string `json:",omitempty"`
TLSNegotiatedProto string `json:",omitempty"` TLSCipherSuite string `json:",omitempty"`
TLSNextProtos []string `json:",omitempty"` TLSNegotiatedProto string `json:",omitempty"`
TLSPeerCerts []*x509.Certificate `json:",omitempty"` TLSNextProtos []string `json:",omitempty"`
TLSVersion string `json:",omitempty"` TLSPeerCerts []*x509.Certificate `json:",omitempty"`
Time time.Time `json:",omitempty"` TLSVersion string `json:",omitempty"`
Transport string `json:",omitempty"` Time time.Time `json:",omitempty"`
Transport string `json:",omitempty"`
} }

32
internal/engine/netx/tracex/http.go
View File

@ -23,30 +23,30 @@ type SaverMetadataHTTPTransport struct {
} }
// RoundTrip implements RoundTripper.RoundTrip // RoundTrip implements RoundTripper.RoundTrip
func (txp SaverMetadataHTTPTransport) RoundTrip(req *http.Request) (*http.Response, error) { func (txp *SaverMetadataHTTPTransport) RoundTrip(req *http.Request) (*http.Response, error) {
txp.Saver.Write(&EventHTTPRequestMetadata{&EventValue{ txp.Saver.Write(&EventHTTPRequestMetadata{&EventValue{
HTTPHeaders: httpCloneHeaders(req), HTTPRequestHeaders: httpCloneRequestHeaders(req),
HTTPMethod: req.Method, HTTPMethod: req.Method,
HTTPURL: req.URL.String(), HTTPURL: req.URL.String(),
Transport: txp.HTTPTransport.Network(), Transport: txp.HTTPTransport.Network(),
Time: time.Now(), Time: time.Now(),
}}) }})
resp, err := txp.HTTPTransport.RoundTrip(req) resp, err := txp.HTTPTransport.RoundTrip(req)
if err != nil { if err != nil {
return nil, err return nil, err
} }
txp.Saver.Write(&EventHTTPResponseMetadata{&EventValue{ txp.Saver.Write(&EventHTTPResponseMetadata{&EventValue{
HTTPHeaders: resp.Header, HTTPResponseHeaders: resp.Header,
HTTPStatusCode: resp.StatusCode, HTTPStatusCode: resp.StatusCode,
Time: time.Now(), Time: time.Now(),
}}) }})
return resp, err return resp, err
} }
// httpCCloneHeaders returns a clone of the headers where we have // httpCloneRequestHeaders returns a clone of the headers where we have
// also set the host header, which normally is not set by // also set the host header, which normally is not set by
// golang until it serializes the request itself. // golang until it serializes the request itself.
func httpCloneHeaders(req *http.Request) http.Header { func httpCloneRequestHeaders(req *http.Request) http.Header {
header := req.Header.Clone() header := req.Header.Clone()
if req.Host != "" { if req.Host != "" {
header.Set("Host", req.Host) header.Set("Host", req.Host)
@ -64,7 +64,7 @@ type SaverTransactionHTTPTransport struct {
} }
// RoundTrip implements RoundTripper.RoundTrip // RoundTrip implements RoundTripper.RoundTrip
func (txp SaverTransactionHTTPTransport) RoundTrip(req *http.Request) (*http.Response, error) { func (txp *SaverTransactionHTTPTransport) RoundTrip(req *http.Request) (*http.Response, error) {
txp.Saver.Write(&EventHTTPTransactionStart{&EventValue{ txp.Saver.Write(&EventHTTPTransactionStart{&EventValue{
Time: time.Now(), Time: time.Now(),
}}) }})
@ -85,7 +85,7 @@ type SaverBodyHTTPTransport struct {
} }
// RoundTrip implements RoundTripper.RoundTrip // RoundTrip implements RoundTripper.RoundTrip
func (txp SaverBodyHTTPTransport) RoundTrip(req *http.Request) (*http.Response, error) { func (txp *SaverBodyHTTPTransport) RoundTrip(req *http.Request) (*http.Response, error) {
const defaultSnapSize = 1 << 17 const defaultSnapSize = 1 << 17
snapsize := defaultSnapSize snapsize := defaultSnapSize
if txp.SnapshotSize != 0 { if txp.SnapshotSize != 0 {
@ -134,6 +134,6 @@ type httpSaverReadCloser struct {
io.Reader io.Reader
} }
var _ model.HTTPTransport = SaverMetadataHTTPTransport{} var _ model.HTTPTransport = &SaverMetadataHTTPTransport{}
var _ model.HTTPTransport = SaverBodyHTTPTransport{} var _ model.HTTPTransport = &SaverBodyHTTPTransport{}
var _ model.HTTPTransport = SaverTransactionHTTPTransport{} var _ model.HTTPTransport = &SaverTransactionHTTPTransport{}

10
internal/engine/netx/tracex/http_test.go
View File

@ -44,7 +44,7 @@ func TestSaverMetadataSuccess(t *testing.T) {
if ev[0].Value().HTTPMethod != "GET" { if ev[0].Value().HTTPMethod != "GET" {
t.Fatal("unexpected Method") t.Fatal("unexpected Method")
} }
if len(ev[0].Value().HTTPHeaders) <= 0 { if len(ev[0].Value().HTTPRequestHeaders) <= 0 {
t.Fatal("unexpected Headers") t.Fatal("unexpected Headers")
} }
if ev[0].Value().HTTPURL != "https://www.google.com" { if ev[0].Value().HTTPURL != "https://www.google.com" {
@ -60,7 +60,7 @@ func TestSaverMetadataSuccess(t *testing.T) {
if ev[1].Value().HTTPStatusCode != 200 { if ev[1].Value().HTTPStatusCode != 200 {
t.Fatal("unexpected StatusCode") t.Fatal("unexpected StatusCode")
} }
if len(ev[1].Value().HTTPHeaders) <= 0 { if len(ev[1].Value().HTTPResponseHeaders) <= 0 {
t.Fatal("unexpected Headers") t.Fatal("unexpected Headers")
} }
if ev[1].Name() != "http_response_metadata" { if ev[1].Name() != "http_response_metadata" {
@ -99,7 +99,7 @@ func TestSaverMetadataFailure(t *testing.T) {
if ev[0].Value().HTTPMethod != "GET" { if ev[0].Value().HTTPMethod != "GET" {
t.Fatal("unexpected Method") t.Fatal("unexpected Method")
} }
if len(ev[0].Value().HTTPHeaders) <= 0 { if len(ev[0].Value().HTTPRequestHeaders) <= 0 {
t.Fatal("unexpected Headers") t.Fatal("unexpected Headers")
} }
if ev[0].Value().HTTPURL != "http://www.google.com" { if ev[0].Value().HTTPURL != "http://www.google.com" {
@ -394,7 +394,7 @@ func TestCloneHeaders(t *testing.T) {
}, },
Header: http.Header{}, Header: http.Header{},
} }
header := httpCloneHeaders(req) header := httpCloneRequestHeaders(req)
if header.Get("Host") != "www.example.com" { if header.Get("Host") != "www.example.com" {
t.Fatal("did not set Host header correctly") t.Fatal("did not set Host header correctly")
} }
@ -408,7 +408,7 @@ func TestCloneHeaders(t *testing.T) {
}, },
Header: http.Header{}, Header: http.Header{},
} }
header := httpCloneHeaders(req) header := httpCloneRequestHeaders(req)
if header.Get("Host") != "www.kernel.org" { if header.Get("Host") != "www.kernel.org" {
t.Fatal("did not set Host header correctly") t.Fatal("did not set Host header correctly")
} }

14
internal/engine/netx/tracex/resolver.go
View File

@ -118,13 +118,13 @@ func (txp *SaverDNSTransport) RoundTrip(
response, err := txp.DNSTransport.RoundTrip(ctx, query) response, err := txp.DNSTransport.RoundTrip(ctx, query)
stop := time.Now() stop := time.Now()
txp.Saver.Write(&EventDNSRoundTripDone{&EventValue{ txp.Saver.Write(&EventDNSRoundTripDone{&EventValue{
Address: txp.DNSTransport.Address(), Address: txp.DNSTransport.Address(),
DNSQuery: dnsMaybeQueryBytes(query), DNSQuery: dnsMaybeQueryBytes(query),
DNSReply: dnsMaybeResponseBytes(response), DNSResponse: dnsMaybeResponseBytes(response),
Duration: stop.Sub(start), Duration: stop.Sub(start),
Err: err, Err: err,
Proto: txp.DNSTransport.Network(), Proto: txp.DNSTransport.Network(),
Time: stop, Time: stop,
}}) }})
return response, err return response, err
} }

4
internal/engine/netx/tracex/resolver_test.go
View File

@ -145,7 +145,7 @@ func TestSaverDNSTransportFailure(t *testing.T) {
if !bytes.Equal(ev[1].Value().DNSQuery, rawQuery) { if !bytes.Equal(ev[1].Value().DNSQuery, rawQuery) {
t.Fatal("unexpected DNSQuery") t.Fatal("unexpected DNSQuery")
} }
if ev[1].Value().DNSReply != nil { if ev[1].Value().DNSResponse != nil {
t.Fatal("unexpected DNSReply") t.Fatal("unexpected DNSReply")
} }
if ev[1].Value().Duration <= 0 { if ev[1].Value().Duration <= 0 {
@ -210,7 +210,7 @@ func TestSaverDNSTransportSuccess(t *testing.T) {
if !bytes.Equal(ev[1].Value().DNSQuery, rawQuery) { if !bytes.Equal(ev[1].Value().DNSQuery, rawQuery) {
t.Fatal("unexpected DNSQuery") t.Fatal("unexpected DNSQuery")
} }
if !bytes.Equal(ev[1].Value().DNSReply, expected) { if !bytes.Equal(ev[1].Value().DNSResponse, expected) {
t.Fatal("unexpected DNSReply") t.Fatal("unexpected DNSReply")
} }
if ev[1].Value().Duration <= 0 { if ev[1].Value().Duration <= 0 {