From 66fd1569b8ad0fa2907d441e78099ecb7d41d71c Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 1 Jun 2022 15:20:28 +0200 Subject: [PATCH] tracex: prepare HTTP code for future refactoring (#778) 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 --- internal/engine/netx/netx.go | 6 +-- internal/engine/netx/netx_test.go | 6 +-- internal/engine/netx/tracex/archival.go | 6 +-- internal/engine/netx/tracex/archival_test.go | 10 ++-- internal/engine/netx/tracex/event.go | 49 ++++++++++---------- internal/engine/netx/tracex/http.go | 32 ++++++------- internal/engine/netx/tracex/http_test.go | 10 ++-- internal/engine/netx/tracex/resolver.go | 14 +++--- internal/engine/netx/tracex/resolver_test.go | 4 +- 9 files changed, 69 insertions(+), 68 deletions(-) diff --git a/internal/engine/netx/netx.go b/internal/engine/netx/netx.go index 43b3a8f..3c3d68f 100644 --- a/internal/engine/netx/netx.go +++ b/internal/engine/netx/netx.go @@ -187,11 +187,11 @@ func NewHTTPTransport(config Config) model.HTTPTransport { txp = &netxlite.HTTPTransportLogger{Logger: config.Logger, HTTPTransport: txp} } if config.HTTPSaver != nil { - txp = tracex.SaverMetadataHTTPTransport{ + txp = &tracex.SaverMetadataHTTPTransport{ HTTPTransport: txp, Saver: config.HTTPSaver} - txp = tracex.SaverBodyHTTPTransport{ + txp = &tracex.SaverBodyHTTPTransport{ HTTPTransport: txp, Saver: config.HTTPSaver} - txp = tracex.SaverTransactionHTTPTransport{ + txp = &tracex.SaverTransactionHTTPTransport{ HTTPTransport: txp, Saver: config.HTTPSaver} } return txp diff --git a/internal/engine/netx/netx_test.go b/internal/engine/netx/netx_test.go index 3224857..c0e99ce 100644 --- a/internal/engine/netx/netx_test.go +++ b/internal/engine/netx/netx_test.go @@ -504,21 +504,21 @@ func TestNewWithSaver(t *testing.T) { txp := netx.NewHTTPTransport(netx.Config{ HTTPSaver: saver, }) - stxptxp, ok := txp.(tracex.SaverTransactionHTTPTransport) + stxptxp, ok := txp.(*tracex.SaverTransactionHTTPTransport) if !ok { t.Fatal("not the transport we expected") } if stxptxp.Saver != saver { t.Fatal("not the logger we expected") } - sbtxp, ok := stxptxp.HTTPTransport.(tracex.SaverBodyHTTPTransport) + sbtxp, ok := stxptxp.HTTPTransport.(*tracex.SaverBodyHTTPTransport) if !ok { t.Fatal("not the transport we expected") } if sbtxp.Saver != saver { t.Fatal("not the logger we expected") } - smtxp, ok := sbtxp.HTTPTransport.(tracex.SaverMetadataHTTPTransport) + smtxp, ok := sbtxp.HTTPTransport.(*tracex.SaverMetadataHTTPTransport) if !ok { t.Fatal("not the transport we expected") } diff --git a/internal/engine/netx/tracex/archival.go b/internal/engine/netx/tracex/archival.go index 6e81abd..8d4a4c8 100644 --- a/internal/engine/netx/tracex/archival.go +++ b/internal/engine/netx/tracex/archival.go @@ -153,16 +153,16 @@ func newRequestList(begin time.Time, events []Event) []RequestEntry { case *EventHTTPRequestMetadata: entry.Request.Headers = make(map[string]MaybeBinaryValue) httpAddHeaders( - ev.HTTPHeaders, &entry.Request.HeadersList, &entry.Request.Headers) + ev.HTTPRequestHeaders, &entry.Request.HeadersList, &entry.Request.Headers) entry.Request.Method = ev.HTTPMethod entry.Request.URL = ev.HTTPURL entry.Request.Transport = ev.Transport case *EventHTTPResponseMetadata: entry.Response.Headers = make(map[string]MaybeBinaryValue) 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.Locations = ev.HTTPHeaders.Values("Location") + entry.Response.Locations = ev.HTTPResponseHeaders.Values("Location") case *EventHTTPResponseBodySnapshot: entry.Response.Body.Value = string(ev.Data) entry.Response.BodyIsTruncated = ev.DataIsTruncated diff --git a/internal/engine/netx/tracex/archival_test.go b/internal/engine/netx/tracex/archival_test.go index 38f6906..213a7e6 100644 --- a/internal/engine/netx/tracex/archival_test.go +++ b/internal/engine/netx/tracex/archival_test.go @@ -149,13 +149,13 @@ func TestNewRequestList(t *testing.T) { Data: []byte("deadbeef"), DataIsTruncated: false, }}, &EventHTTPRequestMetadata{&EventValue{ - HTTPHeaders: http.Header{ + HTTPRequestHeaders: http.Header{ "User-Agent": []string{"miniooni/0.1.0-dev"}, }, HTTPMethod: "POST", HTTPURL: "https://www.example.com/submit", }}, &EventHTTPResponseMetadata{&EventValue{ - HTTPHeaders: http.Header{ + HTTPResponseHeaders: http.Header{ "Server": []string{"miniooni/0.1.0-dev"}, }, HTTPStatusCode: 200, @@ -165,7 +165,7 @@ func TestNewRequestList(t *testing.T) { }}, &EventHTTPTransactionDone{&EventValue{}}, &EventHTTPTransactionStart{&EventValue{ Time: begin.Add(20 * time.Millisecond), }}, &EventHTTPRequestMetadata{&EventValue{ - HTTPHeaders: http.Header{ + HTTPRequestHeaders: http.Header{ "User-Agent": []string{"miniooni/0.1.0-dev"}, }, HTTPMethod: "GET", @@ -234,13 +234,13 @@ func TestNewRequestList(t *testing.T) { events: []Event{&EventHTTPTransactionStart{&EventValue{ Time: begin.Add(10 * time.Millisecond), }}, &EventHTTPRequestMetadata{&EventValue{ - HTTPHeaders: http.Header{ + HTTPRequestHeaders: http.Header{ "User-Agent": []string{"miniooni/0.1.0-dev"}, }, HTTPMethod: "GET", HTTPURL: "https://www.example.com/", }}, &EventHTTPResponseMetadata{&EventValue{ - HTTPHeaders: http.Header{ + HTTPResponseHeaders: http.Header{ "Server": []string{"miniooni/0.1.0-dev"}, "Location": []string{"https://x.example.com", "https://y.example.com"}, }, diff --git a/internal/engine/netx/tracex/event.go b/internal/engine/netx/tracex/event.go index 1be3c81..cb5214b 100644 --- a/internal/engine/netx/tracex/event.go +++ b/internal/engine/netx/tracex/event.go @@ -266,28 +266,29 @@ func (ev *EventWriteOperation) Name() string { // Event is one of the events within a trace type EventValue struct { - Addresses []string `json:",omitempty"` - Address string `json:",omitempty"` - DNSQuery []byte `json:",omitempty"` - DNSReply []byte `json:",omitempty"` - DataIsTruncated bool `json:",omitempty"` - Data []byte `json:",omitempty"` - Duration time.Duration `json:",omitempty"` - Err error `json:",omitempty"` - HTTPHeaders http.Header `json:",omitempty"` - HTTPMethod string `json:",omitempty"` - HTTPStatusCode int `json:",omitempty"` - HTTPURL string `json:",omitempty"` - Hostname string `json:",omitempty"` - NoTLSVerify bool `json:",omitempty"` - NumBytes int `json:",omitempty"` - Proto string `json:",omitempty"` - TLSServerName string `json:",omitempty"` - TLSCipherSuite string `json:",omitempty"` - TLSNegotiatedProto string `json:",omitempty"` - TLSNextProtos []string `json:",omitempty"` - TLSPeerCerts []*x509.Certificate `json:",omitempty"` - TLSVersion string `json:",omitempty"` - Time time.Time `json:",omitempty"` - Transport string `json:",omitempty"` + Addresses []string `json:",omitempty"` + Address string `json:",omitempty"` + DNSQuery []byte `json:",omitempty"` + DNSResponse []byte `json:",omitempty"` + DataIsTruncated bool `json:",omitempty"` + Data []byte `json:",omitempty"` + Duration time.Duration `json:",omitempty"` + Err error `json:",omitempty"` + HTTPMethod string `json:",omitempty"` + HTTPRequestHeaders http.Header `json:",omitempty"` + HTTPResponseHeaders http.Header `json:",omitempty"` + HTTPStatusCode int `json:",omitempty"` + HTTPURL string `json:",omitempty"` + Hostname string `json:",omitempty"` + NoTLSVerify bool `json:",omitempty"` + NumBytes int `json:",omitempty"` + Proto string `json:",omitempty"` + TLSServerName string `json:",omitempty"` + TLSCipherSuite string `json:",omitempty"` + TLSNegotiatedProto string `json:",omitempty"` + TLSNextProtos []string `json:",omitempty"` + TLSPeerCerts []*x509.Certificate `json:",omitempty"` + TLSVersion string `json:",omitempty"` + Time time.Time `json:",omitempty"` + Transport string `json:",omitempty"` } diff --git a/internal/engine/netx/tracex/http.go b/internal/engine/netx/tracex/http.go index 4e68ea0..e4cce39 100644 --- a/internal/engine/netx/tracex/http.go +++ b/internal/engine/netx/tracex/http.go @@ -23,30 +23,30 @@ type SaverMetadataHTTPTransport struct { } // 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{ - HTTPHeaders: httpCloneHeaders(req), - HTTPMethod: req.Method, - HTTPURL: req.URL.String(), - Transport: txp.HTTPTransport.Network(), - Time: time.Now(), + HTTPRequestHeaders: httpCloneRequestHeaders(req), + HTTPMethod: req.Method, + HTTPURL: req.URL.String(), + Transport: txp.HTTPTransport.Network(), + Time: time.Now(), }}) resp, err := txp.HTTPTransport.RoundTrip(req) if err != nil { return nil, err } txp.Saver.Write(&EventHTTPResponseMetadata{&EventValue{ - HTTPHeaders: resp.Header, - HTTPStatusCode: resp.StatusCode, - Time: time.Now(), + HTTPResponseHeaders: resp.Header, + HTTPStatusCode: resp.StatusCode, + Time: time.Now(), }}) 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 // 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() if req.Host != "" { header.Set("Host", req.Host) @@ -64,7 +64,7 @@ type SaverTransactionHTTPTransport struct { } // 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{ Time: time.Now(), }}) @@ -85,7 +85,7 @@ type SaverBodyHTTPTransport struct { } // 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 snapsize := defaultSnapSize if txp.SnapshotSize != 0 { @@ -134,6 +134,6 @@ type httpSaverReadCloser struct { io.Reader } -var _ model.HTTPTransport = SaverMetadataHTTPTransport{} -var _ model.HTTPTransport = SaverBodyHTTPTransport{} -var _ model.HTTPTransport = SaverTransactionHTTPTransport{} +var _ model.HTTPTransport = &SaverMetadataHTTPTransport{} +var _ model.HTTPTransport = &SaverBodyHTTPTransport{} +var _ model.HTTPTransport = &SaverTransactionHTTPTransport{} diff --git a/internal/engine/netx/tracex/http_test.go b/internal/engine/netx/tracex/http_test.go index 36f1b0e..d64cacc 100644 --- a/internal/engine/netx/tracex/http_test.go +++ b/internal/engine/netx/tracex/http_test.go @@ -44,7 +44,7 @@ func TestSaverMetadataSuccess(t *testing.T) { if ev[0].Value().HTTPMethod != "GET" { t.Fatal("unexpected Method") } - if len(ev[0].Value().HTTPHeaders) <= 0 { + if len(ev[0].Value().HTTPRequestHeaders) <= 0 { t.Fatal("unexpected Headers") } if ev[0].Value().HTTPURL != "https://www.google.com" { @@ -60,7 +60,7 @@ func TestSaverMetadataSuccess(t *testing.T) { if ev[1].Value().HTTPStatusCode != 200 { t.Fatal("unexpected StatusCode") } - if len(ev[1].Value().HTTPHeaders) <= 0 { + if len(ev[1].Value().HTTPResponseHeaders) <= 0 { t.Fatal("unexpected Headers") } if ev[1].Name() != "http_response_metadata" { @@ -99,7 +99,7 @@ func TestSaverMetadataFailure(t *testing.T) { if ev[0].Value().HTTPMethod != "GET" { t.Fatal("unexpected Method") } - if len(ev[0].Value().HTTPHeaders) <= 0 { + if len(ev[0].Value().HTTPRequestHeaders) <= 0 { t.Fatal("unexpected Headers") } if ev[0].Value().HTTPURL != "http://www.google.com" { @@ -394,7 +394,7 @@ func TestCloneHeaders(t *testing.T) { }, Header: http.Header{}, } - header := httpCloneHeaders(req) + header := httpCloneRequestHeaders(req) if header.Get("Host") != "www.example.com" { t.Fatal("did not set Host header correctly") } @@ -408,7 +408,7 @@ func TestCloneHeaders(t *testing.T) { }, Header: http.Header{}, } - header := httpCloneHeaders(req) + header := httpCloneRequestHeaders(req) if header.Get("Host") != "www.kernel.org" { t.Fatal("did not set Host header correctly") } diff --git a/internal/engine/netx/tracex/resolver.go b/internal/engine/netx/tracex/resolver.go index aa45a45..db5afd3 100644 --- a/internal/engine/netx/tracex/resolver.go +++ b/internal/engine/netx/tracex/resolver.go @@ -118,13 +118,13 @@ func (txp *SaverDNSTransport) RoundTrip( response, err := txp.DNSTransport.RoundTrip(ctx, query) stop := time.Now() txp.Saver.Write(&EventDNSRoundTripDone{&EventValue{ - Address: txp.DNSTransport.Address(), - DNSQuery: dnsMaybeQueryBytes(query), - DNSReply: dnsMaybeResponseBytes(response), - Duration: stop.Sub(start), - Err: err, - Proto: txp.DNSTransport.Network(), - Time: stop, + Address: txp.DNSTransport.Address(), + DNSQuery: dnsMaybeQueryBytes(query), + DNSResponse: dnsMaybeResponseBytes(response), + Duration: stop.Sub(start), + Err: err, + Proto: txp.DNSTransport.Network(), + Time: stop, }}) return response, err } diff --git a/internal/engine/netx/tracex/resolver_test.go b/internal/engine/netx/tracex/resolver_test.go index 4dc7066..dd1eede 100644 --- a/internal/engine/netx/tracex/resolver_test.go +++ b/internal/engine/netx/tracex/resolver_test.go @@ -145,7 +145,7 @@ func TestSaverDNSTransportFailure(t *testing.T) { if !bytes.Equal(ev[1].Value().DNSQuery, rawQuery) { t.Fatal("unexpected DNSQuery") } - if ev[1].Value().DNSReply != nil { + if ev[1].Value().DNSResponse != nil { t.Fatal("unexpected DNSReply") } if ev[1].Value().Duration <= 0 { @@ -210,7 +210,7 @@ func TestSaverDNSTransportSuccess(t *testing.T) { if !bytes.Equal(ev[1].Value().DNSQuery, rawQuery) { t.Fatal("unexpected DNSQuery") } - if !bytes.Equal(ev[1].Value().DNSReply, expected) { + if !bytes.Equal(ev[1].Value().DNSResponse, expected) { t.Fatal("unexpected DNSReply") } if ev[1].Value().Duration <= 0 {