From 74e107341d0fbd95ce32d7918bbc5defc09b4cff Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 26 Sep 2023 11:43:14 +0200 Subject: [PATCH] refactor(enginenetx): make stats API private (#1304) Now that we've more or less reached the point where we wanted to be with https://github.com/ooni/probe/issues/2531, let's spend some time to refactor the implementation, now that we know very well how to works, such that modifying it in the future would be easier. The first order of business here seems to hide implementation details and get rid of too many HTTPSDialer prefixes which only cause confusion when looking at the available structs. --- .../httpsdialerstats_internal_test.go | 416 ----------- internal/enginenetx/httpsdialerstats_test.go | 279 ------- internal/enginenetx/network.go | 4 +- internal/enginenetx/network_internal_test.go | 8 +- .../{httpsdialerstats.go => stats.go} | 111 ++- internal/enginenetx/stats_test.go | 683 ++++++++++++++++++ 6 files changed, 744 insertions(+), 757 deletions(-) delete mode 100644 internal/enginenetx/httpsdialerstats_internal_test.go delete mode 100644 internal/enginenetx/httpsdialerstats_test.go rename internal/enginenetx/{httpsdialerstats.go => stats.go} (60%) create mode 100644 internal/enginenetx/stats_test.go diff --git a/internal/enginenetx/httpsdialerstats_internal_test.go b/internal/enginenetx/httpsdialerstats_internal_test.go deleted file mode 100644 index c40b0e1f02..0000000000 --- a/internal/enginenetx/httpsdialerstats_internal_test.go +++ /dev/null @@ -1,416 +0,0 @@ -package enginenetx - -import ( - "context" - "encoding/json" - "errors" - "testing" - "time" - - "github.com/google/go-cmp/cmp" - "github.com/google/go-cmp/cmp/cmpopts" - "github.com/ooni/probe-cli/v3/internal/kvstore" - "github.com/ooni/probe-cli/v3/internal/mocks" - "github.com/ooni/probe-cli/v3/internal/runtimex" -) - -func TestLoadHTTPSDialerStatsRootContainer(t *testing.T) { - type testcase struct { - // name is the test case name - name string - - // input returns the bytes we should Set into the key-value store - input func() []byte - - // expectedErr is the expected error string or an empty string - expectErr string - - // expectRoot is the expected root container content - expectRoot *HTTPSDialerStatsRootContainer - } - - cases := []testcase{{ - name: "when the key-value store does not contain any data", - input: func() []byte { - // Note that returning nil causes the code to NOT set anything into the kvstore - return nil - }, - expectErr: "no such key", - expectRoot: nil, - }, { - name: "when we cannot parse the serialized JSON", - input: func() []byte { - return []byte(`{`) - }, - expectErr: "unexpected end of JSON input", - expectRoot: nil, - }, { - name: "with invalid version", - input: func() []byte { - return []byte(`{"Version":1}`) - }, - expectErr: "httpsdialerstats.state: wrong stats container version: expected=2 got=1", - expectRoot: nil, - }, { - name: "on success", - input: func() []byte { - root := &HTTPSDialerStatsRootContainer{ - Domains: map[string]*HTTPSDialerStatsTacticsContainer{ - "api.ooni.io": { - Tactics: map[string]*HTTPSDialerStatsTacticRecord{ - "162.55.247.208:443 sni=www.example.com verify=api.ooni.io": { - CountStarted: 4, - CountTCPConnectError: 1, - CountTLSHandshakeError: 1, - CountTLSVerificationError: 1, - CountSuccess: 1, - HistoTCPConnectError: map[string]int64{ - "connection_refused": 1, - }, - HistoTLSHandshakeError: map[string]int64{ - "generic_timeout_error": 1, - }, - HistoTLSVerificationError: map[string]int64{ - "ssl_invalid_hostname": 1, - }, - LastUpdated: time.Date(2023, 9, 25, 0, 0, 0, 0, time.UTC), - Tactic: &HTTPSDialerTactic{ - Endpoint: "162.55.247.208:443", - InitialDelay: 0, - SNI: "www.example.com", - VerifyHostname: "api.ooni.io", - }, - }, - }, - }, - }, - Version: HTTPSDialerStatsContainerVersion, - } - return runtimex.Try1(json.Marshal(root)) - }, - expectErr: "", - expectRoot: &HTTPSDialerStatsRootContainer{ - Domains: map[string]*HTTPSDialerStatsTacticsContainer{ - "api.ooni.io": { - Tactics: map[string]*HTTPSDialerStatsTacticRecord{ - "162.55.247.208:443 sni=www.example.com verify=api.ooni.io": { - CountStarted: 4, - CountTCPConnectError: 1, - CountTLSHandshakeError: 1, - CountTLSVerificationError: 1, - CountSuccess: 1, - HistoTCPConnectError: map[string]int64{ - "connection_refused": 1, - }, - HistoTLSHandshakeError: map[string]int64{ - "generic_timeout_error": 1, - }, - HistoTLSVerificationError: map[string]int64{ - "ssl_invalid_hostname": 1, - }, - LastUpdated: time.Date(2023, 9, 25, 0, 0, 0, 0, time.UTC), - Tactic: &HTTPSDialerTactic{ - Endpoint: "162.55.247.208:443", - InitialDelay: 0, - SNI: "www.example.com", - VerifyHostname: "api.ooni.io", - }, - }, - }, - }, - }, - Version: HTTPSDialerStatsContainerVersion, - }, - }} - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - kvStore := &kvstore.Memory{} - if input := tc.input(); len(input) > 0 { - if err := kvStore.Set(HTTPSDialerStatsKey, input); err != nil { - t.Fatal(err) - } - } - - root, err := loadHTTPSDialerStatsRootContainer(kvStore) - - switch { - case err == nil && tc.expectErr == "": - // all good - - case err != nil && tc.expectErr == "": - t.Fatal("expected", tc.expectErr, "but got", err.Error()) - - case err == nil && tc.expectErr != "": - t.Fatal("expected", tc.expectErr, "but got", err) - - case err != nil && tc.expectErr != "": - if tc.expectErr != err.Error() { - t.Fatal("expected", tc.expectErr, "but got", err.Error()) - } - } - - if diff := cmp.Diff(tc.expectRoot, root); diff != "" { - t.Fatal(diff) - } - }) - } -} - -func TestHTTPSDialerStatsManagerCallbacks(t *testing.T) { - type testcase struct { - name string - initialRoot *HTTPSDialerStatsRootContainer - do func(stats *HTTPSDialerStatsManager) - expectWarnf int - expectRoot *HTTPSDialerStatsRootContainer - } - - cases := []testcase{ - - // When TCP connect fails and the reason is a canceled context - { - name: "OnTCPConnectError with ctx.Error() != nil", - initialRoot: &HTTPSDialerStatsRootContainer{ - Domains: map[string]*HTTPSDialerStatsTacticsContainer{ - "api.ooni.io": { - Tactics: map[string]*HTTPSDialerStatsTacticRecord{ - "162.55.247.208:443 sni=www.example.com verify=api.ooni.io": { - CountStarted: 1, - }, - }, - }, - }, - Version: HTTPSDialerStatsContainerVersion, - }, - do: func(stats *HTTPSDialerStatsManager) { - ctx, cancel := context.WithCancel(context.Background()) - cancel() // immediately! - - tactic := &HTTPSDialerTactic{ - Endpoint: "162.55.247.208:443", - InitialDelay: 0, - SNI: "www.example.com", - VerifyHostname: "api.ooni.io", - } - err := errors.New("generic_timeout_error") - - stats.OnTCPConnectError(ctx, tactic, err) - }, - expectWarnf: 0, - expectRoot: &HTTPSDialerStatsRootContainer{ - Domains: map[string]*HTTPSDialerStatsTacticsContainer{ - "api.ooni.io": { - Tactics: map[string]*HTTPSDialerStatsTacticRecord{ - "162.55.247.208:443 sni=www.example.com verify=api.ooni.io": { - CountStarted: 1, - CountTCPConnectInterrupt: 1, - }, - }, - }, - }, - Version: HTTPSDialerStatsContainerVersion, - }, - }, - - // When TCP connect fails and we don't already have a policy record - { - name: "OnTCPConnectError when we are missing the stats record for the domain", - initialRoot: &HTTPSDialerStatsRootContainer{ - Domains: map[string]*HTTPSDialerStatsTacticsContainer{}, - Version: HTTPSDialerStatsContainerVersion, - }, - do: func(stats *HTTPSDialerStatsManager) { - ctx := context.Background() - - tactic := &HTTPSDialerTactic{ - Endpoint: "162.55.247.208:443", - InitialDelay: 0, - SNI: "www.example.com", - VerifyHostname: "api.ooni.io", - } - err := errors.New("generic_timeout_error") - - stats.OnTCPConnectError(ctx, tactic, err) - }, - expectWarnf: 1, - expectRoot: &HTTPSDialerStatsRootContainer{ - Domains: map[string]*HTTPSDialerStatsTacticsContainer{}, - Version: HTTPSDialerStatsContainerVersion, - }, - }, - - // When TLS handshake fails and the reason is a canceled context - { - name: "OnTLSHandshakeError with ctx.Error() != nil", - initialRoot: &HTTPSDialerStatsRootContainer{ - Domains: map[string]*HTTPSDialerStatsTacticsContainer{ - "api.ooni.io": { - Tactics: map[string]*HTTPSDialerStatsTacticRecord{ - "162.55.247.208:443 sni=www.example.com verify=api.ooni.io": { - CountStarted: 1, - }, - }, - }, - }, - Version: HTTPSDialerStatsContainerVersion, - }, - do: func(stats *HTTPSDialerStatsManager) { - ctx, cancel := context.WithCancel(context.Background()) - cancel() // immediately! - - tactic := &HTTPSDialerTactic{ - Endpoint: "162.55.247.208:443", - InitialDelay: 0, - SNI: "www.example.com", - VerifyHostname: "api.ooni.io", - } - err := errors.New("generic_timeout_error") - - stats.OnTLSHandshakeError(ctx, tactic, err) - }, - expectWarnf: 0, - expectRoot: &HTTPSDialerStatsRootContainer{ - Domains: map[string]*HTTPSDialerStatsTacticsContainer{ - "api.ooni.io": { - Tactics: map[string]*HTTPSDialerStatsTacticRecord{ - "162.55.247.208:443 sni=www.example.com verify=api.ooni.io": { - CountStarted: 1, - CountTLSHandshakeInterrupt: 1, - }, - }, - }, - }, - Version: HTTPSDialerStatsContainerVersion, - }, - }, - - // When TLS handshake fails and we don't already have a policy record - { - name: "OnTLSHandshakeError when we are missing the stats record for the domain", - initialRoot: &HTTPSDialerStatsRootContainer{ - Domains: map[string]*HTTPSDialerStatsTacticsContainer{}, - Version: HTTPSDialerStatsContainerVersion, - }, - do: func(stats *HTTPSDialerStatsManager) { - ctx := context.Background() - - tactic := &HTTPSDialerTactic{ - Endpoint: "162.55.247.208:443", - InitialDelay: 0, - SNI: "www.example.com", - VerifyHostname: "api.ooni.io", - } - err := errors.New("generic_timeout_error") - - stats.OnTLSHandshakeError(ctx, tactic, err) - }, - expectWarnf: 1, - expectRoot: &HTTPSDialerStatsRootContainer{ - Domains: map[string]*HTTPSDialerStatsTacticsContainer{}, - Version: HTTPSDialerStatsContainerVersion, - }, - }, - - // When TLS verification fails and we don't already have a policy record - { - name: "OnTLSVerifyError when we are missing the stats record for the domain", - initialRoot: &HTTPSDialerStatsRootContainer{ - Domains: map[string]*HTTPSDialerStatsTacticsContainer{}, - Version: HTTPSDialerStatsContainerVersion, - }, - do: func(stats *HTTPSDialerStatsManager) { - tactic := &HTTPSDialerTactic{ - Endpoint: "162.55.247.208:443", - InitialDelay: 0, - SNI: "www.example.com", - VerifyHostname: "api.ooni.io", - } - err := errors.New("generic_timeout_error") - - stats.OnTLSVerifyError(tactic, err) - }, - expectWarnf: 1, - expectRoot: &HTTPSDialerStatsRootContainer{ - Domains: map[string]*HTTPSDialerStatsTacticsContainer{}, - Version: HTTPSDialerStatsContainerVersion, - }, - }, - - // With success when we don't already have a policy record - { - name: "OnSuccess when we are missing the stats record for the domain", - initialRoot: &HTTPSDialerStatsRootContainer{ - Domains: map[string]*HTTPSDialerStatsTacticsContainer{}, - Version: HTTPSDialerStatsContainerVersion, - }, - do: func(stats *HTTPSDialerStatsManager) { - tactic := &HTTPSDialerTactic{ - Endpoint: "162.55.247.208:443", - InitialDelay: 0, - SNI: "www.example.com", - VerifyHostname: "api.ooni.io", - } - - stats.OnSuccess(tactic) - }, - expectWarnf: 1, - expectRoot: &HTTPSDialerStatsRootContainer{ - Domains: map[string]*HTTPSDialerStatsTacticsContainer{}, - Version: HTTPSDialerStatsContainerVersion, - }, - }, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - // configure the initial value of the stats - kvStore := &kvstore.Memory{} - if err := kvStore.Set(HTTPSDialerStatsKey, runtimex.Try1(json.Marshal(tc.initialRoot))); err != nil { - t.Fatal(err) - } - - // create logger counting the number Warnf invocations - var warnfCount int - logger := &mocks.Logger{ - MockWarnf: func(format string, v ...any) { - warnfCount++ - }, - } - - // create the stats manager - stats := NewHTTPSDialerStatsManager(kvStore, logger) - - // invoke the proper stats callback - tc.do(stats) - - // close the stats to trigger a kvstore write - if err := stats.Close(); err != nil { - t.Fatal(err) - } - - // extract the possibly modified stats from the kvstore - var root *HTTPSDialerStatsRootContainer - rawRoot, err := kvStore.Get(HTTPSDialerStatsKey) - if err != nil { - t.Fatal(err) - } - if err := json.Unmarshal(rawRoot, &root); err != nil { - t.Fatal(err) - } - - // make sure the stats are the ones we expect - diffOptions := []cmp.Option{ - cmpopts.IgnoreFields(HTTPSDialerStatsTacticRecord{}, "LastUpdated"), - } - if diff := cmp.Diff(tc.expectRoot, root, diffOptions...); diff != "" { - t.Fatal(diff) - } - - // make sure we logged if necessary - if tc.expectWarnf != warnfCount { - t.Fatal("expected", tc.expectWarnf, "got", warnfCount) - } - }) - } -} diff --git a/internal/enginenetx/httpsdialerstats_test.go b/internal/enginenetx/httpsdialerstats_test.go deleted file mode 100644 index 3ff5fdb5b2..0000000000 --- a/internal/enginenetx/httpsdialerstats_test.go +++ /dev/null @@ -1,279 +0,0 @@ -package enginenetx_test - -import ( - "encoding/json" - "net" - "testing" - "time" - - "github.com/apex/log" - "github.com/google/go-cmp/cmp" - "github.com/google/go-cmp/cmp/cmpopts" - "github.com/ooni/netem" - "github.com/ooni/probe-cli/v3/internal/bytecounter" - "github.com/ooni/probe-cli/v3/internal/enginenetx" - "github.com/ooni/probe-cli/v3/internal/kvstore" - "github.com/ooni/probe-cli/v3/internal/netemx" - "github.com/ooni/probe-cli/v3/internal/netxlite" - "github.com/ooni/probe-cli/v3/internal/runtimex" -) - -func TestHTTPSDialerCollectStats(t *testing.T) { - // testcase is a test case run by this function - type testcase struct { - // name is the test case name - name string - - // URL is the URL to GET - URL string - - // initialPolicy is the initial policy to configure into the key-value store - initialPolicy func() []byte - - // configureDPI is the function to configure DPI - configureDPI func(dpi *netem.DPIEngine) - - // expectErr is the expected error string - expectErr string - - // statsDomain is the domain to lookup inside the stats - statsDomain string - - // statsTacticsSummary is the summary to lookup inside the stats - // once we have used the statsDomain to get a record - statsTacticsSummary string - - // expectStats contains the expected record containing tactics stats - expectStats *enginenetx.HTTPSDialerStatsTacticRecord - } - - cases := []testcase{ - - { - name: "with TCP connect failure", - URL: "https://api.ooni.io/", - initialPolicy: func() []byte { - p0 := &enginenetx.HTTPSDialerStaticPolicyRoot{ - Domains: map[string][]*enginenetx.HTTPSDialerTactic{ - // This policy has a different SNI and VerifyHostname, which gives - // us confidence that the stats are using the latter - "api.ooni.io": {{ - Endpoint: net.JoinHostPort(netemx.AddressApiOONIIo, "443"), - InitialDelay: 0, - SNI: "www.example.com", - VerifyHostname: "api.ooni.io", - }}, - }, - Version: enginenetx.HTTPSDialerStaticPolicyVersion, - } - return runtimex.Try1(json.Marshal(p0)) - }, - configureDPI: func(dpi *netem.DPIEngine) { - dpi.AddRule(&netem.DPICloseConnectionForServerEndpoint{ - Logger: log.Log, - ServerIPAddress: netemx.AddressApiOONIIo, - ServerPort: 443, - }) - }, - expectErr: `Get "https://api.ooni.io/": connection_refused`, - statsDomain: "api.ooni.io", - statsTacticsSummary: "162.55.247.208:443 sni=www.example.com verify=api.ooni.io", - expectStats: &enginenetx.HTTPSDialerStatsTacticRecord{ - CountStarted: 1, - CountTCPConnectError: 1, - CountTLSHandshakeError: 0, - CountTLSVerificationError: 0, - CountSuccess: 0, - HistoTCPConnectError: map[string]int64{ - "connection_refused": 1, - }, - HistoTLSHandshakeError: map[string]int64{}, - HistoTLSVerificationError: map[string]int64{}, - LastUpdated: time.Time{}, - Tactic: &enginenetx.HTTPSDialerTactic{ - Endpoint: "162.55.247.208:443", - InitialDelay: 0, - SNI: "www.example.com", - VerifyHostname: "api.ooni.io", - }, - }, - }, - - { - name: "with TLS handshake failure", - URL: "https://api.ooni.io/", - initialPolicy: func() []byte { - p0 := &enginenetx.HTTPSDialerStaticPolicyRoot{ - Domains: map[string][]*enginenetx.HTTPSDialerTactic{ - // This policy has a different SNI and VerifyHostname, which gives - // us confidence that the stats are using the latter - "api.ooni.io": {{ - Endpoint: net.JoinHostPort(netemx.AddressApiOONIIo, "443"), - InitialDelay: 0, - SNI: "www.example.com", - VerifyHostname: "api.ooni.io", - }}, - }, - Version: enginenetx.HTTPSDialerStaticPolicyVersion, - } - return runtimex.Try1(json.Marshal(p0)) - }, - configureDPI: func(dpi *netem.DPIEngine) { - dpi.AddRule(&netem.DPIResetTrafficForTLSSNI{ - Logger: log.Log, - SNI: "www.example.com", - }) - }, - expectErr: `Get "https://api.ooni.io/": connection_reset`, - statsDomain: "api.ooni.io", - statsTacticsSummary: "162.55.247.208:443 sni=www.example.com verify=api.ooni.io", - expectStats: &enginenetx.HTTPSDialerStatsTacticRecord{ - CountStarted: 1, - CountTCPConnectError: 0, - CountTLSHandshakeError: 1, - CountTLSVerificationError: 0, - CountSuccess: 0, - HistoTCPConnectError: map[string]int64{}, - HistoTLSHandshakeError: map[string]int64{ - "connection_reset": 1, - }, - HistoTLSVerificationError: map[string]int64{}, - LastUpdated: time.Time{}, - Tactic: &enginenetx.HTTPSDialerTactic{ - Endpoint: "162.55.247.208:443", - InitialDelay: 0, - SNI: "www.example.com", - VerifyHostname: "api.ooni.io", - }, - }, - }, - - { - name: "with TLS verification failure", - URL: "https://api.ooni.io/", - initialPolicy: func() []byte { - p0 := &enginenetx.HTTPSDialerStaticPolicyRoot{ - Domains: map[string][]*enginenetx.HTTPSDialerTactic{ - // This policy has a different SNI and VerifyHostname, which gives - // us confidence that the stats are using the latter - "api.ooni.io": {{ - Endpoint: net.JoinHostPort(netemx.AddressBadSSLCom, "443"), - InitialDelay: 0, - SNI: "untrusted-root.badssl.com", - VerifyHostname: "api.ooni.io", - }}, - }, - Version: enginenetx.HTTPSDialerStaticPolicyVersion, - } - return runtimex.Try1(json.Marshal(p0)) - }, - configureDPI: func(dpi *netem.DPIEngine) { - // nothing - }, - expectErr: `Get "https://api.ooni.io/": ssl_invalid_hostname`, - statsDomain: "api.ooni.io", - statsTacticsSummary: "104.154.89.105:443 sni=untrusted-root.badssl.com verify=api.ooni.io", - expectStats: &enginenetx.HTTPSDialerStatsTacticRecord{ - CountStarted: 1, - CountTCPConnectError: 0, - CountTLSHandshakeError: 0, - CountTLSVerificationError: 1, - CountSuccess: 0, - HistoTCPConnectError: map[string]int64{}, - HistoTLSHandshakeError: map[string]int64{}, - HistoTLSVerificationError: map[string]int64{ - "ssl_invalid_hostname": 1, - }, - LastUpdated: time.Time{}, - Tactic: &enginenetx.HTTPSDialerTactic{ - Endpoint: "104.154.89.105:443", - InitialDelay: 0, - SNI: "untrusted-root.badssl.com", - VerifyHostname: "api.ooni.io", - }, - }, - }} - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - qa := netemx.MustNewScenario(netemx.InternetScenario) - defer qa.Close() - - // make sure we apply specific DPI rules - tc.configureDPI(qa.DPIEngine()) - - // create a memory key-value store where the engine will write stats that later we - // would be able to read to confirm we're collecting stats - kvStore := &kvstore.Memory{} - - initialPolicy := tc.initialPolicy() - t.Logf("initialPolicy: %s", string(initialPolicy)) - if err := kvStore.Set(enginenetx.HTTPSDialerStaticPolicyKey, initialPolicy); err != nil { - t.Fatal(err) - } - - qa.Do(func() { - byteCounter := bytecounter.New() - resolver := netxlite.NewStdlibResolver(log.Log) - - netx := enginenetx.NewNetwork(byteCounter, kvStore, log.Log, nil, resolver) - defer netx.Close() - - client := netx.NewHTTPClient() - - resp, err := client.Get(tc.URL) - - switch { - case err == nil && tc.expectErr == "": - // all good - - case err != nil && tc.expectErr == "": - t.Fatal("expected", tc.expectErr, "but got", err.Error()) - - case err == nil && tc.expectErr != "": - t.Fatal("expected", tc.expectErr, "but got", err) - - case err != nil && tc.expectErr != "": - if tc.expectErr != err.Error() { - t.Fatal("expected", tc.expectErr, "but got", err.Error()) - } - } - - if resp != nil { - defer resp.Body.Close() - } - }) - - // obtain the tactics container for the proper domain - rawStats, err := kvStore.Get(enginenetx.HTTPSDialerStatsKey) - if err != nil { - t.Fatal(err) - } - var rootStats enginenetx.HTTPSDialerStatsRootContainer - if err := json.Unmarshal(rawStats, &rootStats); err != nil { - t.Fatal(err) - } - tactics, good := rootStats.Domains[tc.statsDomain] - if !good { - t.Fatalf("no such record for `%s`", tc.statsDomain) - } - t.Logf("%+v", tactics) - - // we expect to see a single record - if len(tactics.Tactics) != 1 { - t.Fatal("expected a single tactic") - } - tactic, good := tactics.Tactics[tc.statsTacticsSummary] - if !good { - t.Fatalf("no such record for: %s", tc.statsTacticsSummary) - } - - diffOptions := []cmp.Option{ - cmpopts.IgnoreFields(enginenetx.HTTPSDialerStatsTacticRecord{}, "LastUpdated"), - } - if diff := cmp.Diff(tc.expectStats, tactic, diffOptions...); diff != "" { - t.Fatal(diff) - } - }) - } -} diff --git a/internal/enginenetx/network.go b/internal/enginenetx/network.go index dc5f51a737..1669a8bc38 100644 --- a/internal/enginenetx/network.go +++ b/internal/enginenetx/network.go @@ -15,7 +15,7 @@ import ( // Network is the network abstraction used by the OONI engine. type Network struct { reso model.Resolver - stats *HTTPSDialerStatsManager + stats *statsManager txp model.HTTPTransport } @@ -84,7 +84,7 @@ func NewNetwork( dialer := netxlite.NewDialerWithResolver(logger, resolver) // Create manager for keeping track of statistics - stats := NewHTTPSDialerStatsManager(kvStore, logger) + stats := newStatsManager(kvStore, logger) // Create a TLS dialer ONLY used for dialing TLS connections. This dialer will use // happy-eyeballs and possibly custom policies for dialing TLS connections. diff --git a/internal/enginenetx/network_internal_test.go b/internal/enginenetx/network_internal_test.go index 550add880b..741f183ed4 100644 --- a/internal/enginenetx/network_internal_test.go +++ b/internal/enginenetx/network_internal_test.go @@ -31,11 +31,11 @@ func TestNetworkUnit(t *testing.T) { // nothing }, }, - stats: &HTTPSDialerStatsManager{ + stats: &statsManager{ kvStore: &kvstore.Memory{}, logger: model.DiscardLogger, mu: sync.Mutex{}, - root: &HTTPSDialerStatsRootContainer{}, + root: &statsContainer{}, }, txp: expected, } @@ -56,11 +56,11 @@ func TestNetworkUnit(t *testing.T) { } netx := &Network{ reso: expected, - stats: &HTTPSDialerStatsManager{ + stats: &statsManager{ kvStore: &kvstore.Memory{}, logger: model.DiscardLogger, mu: sync.Mutex{}, - root: &HTTPSDialerStatsRootContainer{}, + root: &statsContainer{}, }, txp: &mocks.HTTPTransport{ MockCloseIdleConnections: func() { diff --git a/internal/enginenetx/httpsdialerstats.go b/internal/enginenetx/stats.go similarity index 60% rename from internal/enginenetx/httpsdialerstats.go rename to internal/enginenetx/stats.go index cbf16ad2d4..56fd97a335 100644 --- a/internal/enginenetx/httpsdialerstats.go +++ b/internal/enginenetx/stats.go @@ -12,8 +12,8 @@ import ( "github.com/ooni/probe-cli/v3/internal/runtimex" ) -// HTTPSDialerStatsTacticRecord keeps stats about an [HTTPSDialerTactic]. -type HTTPSDialerStatsTacticRecord struct { +// statsTactic keeps stats about an [*HTTPSDialerTactic]. +type statsTactic struct { // CountStarted counts the number of operations we started. CountStarted int64 @@ -51,30 +51,29 @@ type HTTPSDialerStatsTacticRecord struct { Tactic *HTTPSDialerTactic } -// HTTPSDialerStatsTacticsContainer contains tactics. -type HTTPSDialerStatsTacticsContainer struct { - // Tactic maps the summary of a tactic to the tactic record. - Tactics map[string]*HTTPSDialerStatsTacticRecord +// statsDomain contains stats associated with a domain. +type statsDomain struct { + Tactics map[string]*statsTactic } -// HTTPSDialerStatsContainerVersion is the current version of [HTTPSDialerStatsContainer]. -const HTTPSDialerStatsContainerVersion = 2 +// statsContainerVersion is the current version of [statsContainer]. +const statsContainerVersion = 2 -// HTTPSDialerStatsRootContainer is the root container for stats. +// statsContainer is the root container for stats. // -// The zero value is invalid; construct using [NewHTTPSDialerStatsRootContainer]. -type HTTPSDialerStatsRootContainer struct { +// The zero value is invalid; construct using [newStatsContainer]. +type statsContainer struct { // Domains maps a domain name to its tactics - Domains map[string]*HTTPSDialerStatsTacticsContainer + Domains map[string]*statsDomain // Version is the version of the container data format. Version int } -// Get returns the tactic record for the given [*HTTPSDialerTactic] instance. +// Get returns the tactic record for the given [*statsTactic] instance. // -// At the name implies, this function MUST be called while holding the [HTTPSDialerStatsManager] mutex. -func (c *HTTPSDialerStatsRootContainer) GetLocked(tactic *HTTPSDialerTactic) (*HTTPSDialerStatsTacticRecord, bool) { +// At the name implies, this function MUST be called while holding the [*statsManager] mutex. +func (c *statsContainer) GetLocked(tactic *HTTPSDialerTactic) (*statsTactic, bool) { domainRecord, found := c.Domains[tactic.VerifyHostname] if !found { return nil, false @@ -83,19 +82,19 @@ func (c *HTTPSDialerStatsRootContainer) GetLocked(tactic *HTTPSDialerTactic) (*H return tacticRecord, found } -// Set sets the tactic record for the given the given [*HTTPSDialerTactic] instance. +// Set sets the tactic record for the given the given [*statsTactic] instance. // -// At the name implies, this function MUST be called while holding the [HTTPSDialerStatsManager] mutex. -func (c *HTTPSDialerStatsRootContainer) SetLocked(tactic *HTTPSDialerTactic, record *HTTPSDialerStatsTacticRecord) { +// At the name implies, this function MUST be called while holding the [*statsManager] mutex. +func (c *statsContainer) SetLocked(tactic *HTTPSDialerTactic, record *statsTactic) { domainRecord, found := c.Domains[tactic.VerifyHostname] if !found { - domainRecord = &HTTPSDialerStatsTacticsContainer{ - Tactics: map[string]*HTTPSDialerStatsTacticRecord{}, + domainRecord = &statsDomain{ + Tactics: map[string]*statsTactic{}, } // make sure the map is initialized if len(c.Domains) <= 0 { - c.Domains = make(map[string]*HTTPSDialerStatsTacticsContainer) + c.Domains = make(map[string]*statsDomain) } c.Domains[tactic.VerifyHostname] = domainRecord @@ -104,20 +103,20 @@ func (c *HTTPSDialerStatsRootContainer) SetLocked(tactic *HTTPSDialerTactic, rec domainRecord.Tactics[tactic.Summary()] = record } -// NewHTTPSDialerStatsRootContainer creates a new empty [*HTTPSDialerStatsRootContainer]. -func NewHTTPSDialerStatsRootContainer() *HTTPSDialerStatsRootContainer { - return &HTTPSDialerStatsRootContainer{ - Domains: map[string]*HTTPSDialerStatsTacticsContainer{}, - Version: HTTPSDialerStatsContainerVersion, +// newStatsContainer creates a new empty [*statsContainer]. +func newStatsContainer() *statsContainer { + return &statsContainer{ + Domains: map[string]*statsDomain{}, + Version: statsContainerVersion, } } -// HTTPSDialerStatsManager implements [HTTPSDialerStatsTracker] by storing +// statsManager implements [HTTPSDialerStatsTracker] by storing // the relevant statistics in a [model.KeyValueStore]. // // The zero value of this structure is not ready to use; please, use the -// [NewHTTPSDialerStatsManager] factory to create a new instance. -type HTTPSDialerStatsManager struct { +// [newStatsManager] factory to create a new instance. +type statsManager struct { // kvStore is the key-value store we're using kvStore model.KeyValueStore @@ -128,36 +127,36 @@ type HTTPSDialerStatsManager struct { mu sync.Mutex // root is the root container for stats - root *HTTPSDialerStatsRootContainer + root *statsContainer } -// HTTPSDialerStatsKey is the key used in the key-value store to access the state. -const HTTPSDialerStatsKey = "httpsdialerstats.state" +// statsKey is the key used in the key-value store to access the state. +const statsKey = "httpsdialerstats.state" -// errDialerStatsContainerWrongVersion means that the stats container document has the wrong version number. -var errDialerStatsContainerWrongVersion = errors.New("wrong stats container version") +// errStatsContainerWrongVersion means that the stats container document has the wrong version number. +var errStatsContainerWrongVersion = errors.New("wrong stats container version") -// loadHTTPSDialerStatsRootContainer loads a state container from the given key-value store. -func loadHTTPSDialerStatsRootContainer(kvStore model.KeyValueStore) (*HTTPSDialerStatsRootContainer, error) { +// loadStatsContainer loads a state container from the given key-value store. +func loadStatsContainer(kvStore model.KeyValueStore) (*statsContainer, error) { // load data from the kvstore - data, err := kvStore.Get(HTTPSDialerStatsKey) + data, err := kvStore.Get(statsKey) if err != nil { return nil, err } // parse as JSON - var container HTTPSDialerStatsRootContainer + var container statsContainer if err := json.Unmarshal(data, &container); err != nil { return nil, err } // make sure the version is OK - if container.Version != HTTPSDialerStatsContainerVersion { + if container.Version != statsContainerVersion { err := fmt.Errorf( "%s: %w: expected=%d got=%d", - HTTPSDialerStatsKey, - errDialerStatsContainerWrongVersion, - HTTPSDialerStatsContainerVersion, + statsKey, + errStatsContainerWrongVersion, + statsContainerVersion, container.Version, ) return nil, err @@ -166,14 +165,14 @@ func loadHTTPSDialerStatsRootContainer(kvStore model.KeyValueStore) (*HTTPSDiale return &container, nil } -// NewHTTPSDialerStatsManager constructs a new instance of [*HTTPSDialerStatsManager]. -func NewHTTPSDialerStatsManager(kvStore model.KeyValueStore, logger model.Logger) *HTTPSDialerStatsManager { - root, err := loadHTTPSDialerStatsRootContainer(kvStore) +// newStatsManager constructs a new instance of [*statsManager]. +func newStatsManager(kvStore model.KeyValueStore, logger model.Logger) *statsManager { + root, err := loadStatsContainer(kvStore) if err != nil { - root = NewHTTPSDialerStatsRootContainer() + root = newStatsContainer() } - return &HTTPSDialerStatsManager{ + return &statsManager{ root: root, kvStore: kvStore, logger: logger, @@ -181,10 +180,10 @@ func NewHTTPSDialerStatsManager(kvStore model.KeyValueStore, logger model.Logger } } -var _ HTTPSDialerStatsTracker = &HTTPSDialerStatsManager{} +var _ HTTPSDialerStatsTracker = &statsManager{} // OnStarting implements HTTPSDialerStatsManager. -func (mt *HTTPSDialerStatsManager) OnStarting(tactic *HTTPSDialerTactic) { +func (mt *statsManager) OnStarting(tactic *HTTPSDialerTactic) { // get exclusive access defer mt.mu.Unlock() mt.mu.Lock() @@ -192,7 +191,7 @@ func (mt *HTTPSDialerStatsManager) OnStarting(tactic *HTTPSDialerTactic) { // get the record record, found := mt.root.GetLocked(tactic) if !found { - record = &HTTPSDialerStatsTacticRecord{ + record = &statsTactic{ CountStarted: 0, CountTCPConnectError: 0, CountTCPConnectInterrupt: 0, @@ -215,7 +214,7 @@ func (mt *HTTPSDialerStatsManager) OnStarting(tactic *HTTPSDialerTactic) { } // OnTCPConnectError implements HTTPSDialerStatsManager. -func (mt *HTTPSDialerStatsManager) OnTCPConnectError(ctx context.Context, tactic *HTTPSDialerTactic, err error) { +func (mt *statsManager) OnTCPConnectError(ctx context.Context, tactic *HTTPSDialerTactic, err error) { // get exclusive access defer mt.mu.Unlock() mt.mu.Lock() @@ -238,7 +237,7 @@ func (mt *HTTPSDialerStatsManager) OnTCPConnectError(ctx context.Context, tactic } // OnTLSHandshakeError implements HTTPSDialerStatsManager. -func (mt *HTTPSDialerStatsManager) OnTLSHandshakeError(ctx context.Context, tactic *HTTPSDialerTactic, err error) { +func (mt *statsManager) OnTLSHandshakeError(ctx context.Context, tactic *HTTPSDialerTactic, err error) { // get exclusive access defer mt.mu.Unlock() mt.mu.Lock() @@ -261,7 +260,7 @@ func (mt *HTTPSDialerStatsManager) OnTLSHandshakeError(ctx context.Context, tact } // OnTLSVerifyError implements HTTPSDialerStatsManager. -func (mt *HTTPSDialerStatsManager) OnTLSVerifyError(tactic *HTTPSDialerTactic, err error) { +func (mt *statsManager) OnTLSVerifyError(tactic *HTTPSDialerTactic, err error) { // get exclusive access defer mt.mu.Unlock() mt.mu.Lock() @@ -280,7 +279,7 @@ func (mt *HTTPSDialerStatsManager) OnTLSVerifyError(tactic *HTTPSDialerTactic, e } // OnSuccess implements HTTPSDialerStatsManager. -func (mt *HTTPSDialerStatsManager) OnSuccess(tactic *HTTPSDialerTactic) { +func (mt *statsManager) OnSuccess(tactic *HTTPSDialerTactic) { // get exclusive access defer mt.mu.Unlock() mt.mu.Lock() @@ -298,7 +297,7 @@ func (mt *HTTPSDialerStatsManager) OnSuccess(tactic *HTTPSDialerTactic) { } // Close implements io.Closer -func (mt *HTTPSDialerStatsManager) Close() error { +func (mt *statsManager) Close() error { // TODO(bassosimone): do we need to apply a "once" semantics to this method? // get exclusive access @@ -306,5 +305,5 @@ func (mt *HTTPSDialerStatsManager) Close() error { mt.mu.Lock() // write updated stats into the underlying key-value store - return mt.kvStore.Set(HTTPSDialerStatsKey, runtimex.Try1(json.Marshal(mt.root))) + return mt.kvStore.Set(statsKey, runtimex.Try1(json.Marshal(mt.root))) } diff --git a/internal/enginenetx/stats_test.go b/internal/enginenetx/stats_test.go new file mode 100644 index 0000000000..053c3136da --- /dev/null +++ b/internal/enginenetx/stats_test.go @@ -0,0 +1,683 @@ +package enginenetx + +import ( + "context" + "encoding/json" + "errors" + "net" + "testing" + "time" + + "github.com/apex/log" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/ooni/netem" + "github.com/ooni/probe-cli/v3/internal/bytecounter" + "github.com/ooni/probe-cli/v3/internal/kvstore" + "github.com/ooni/probe-cli/v3/internal/mocks" + "github.com/ooni/probe-cli/v3/internal/netemx" + "github.com/ooni/probe-cli/v3/internal/netxlite" + "github.com/ooni/probe-cli/v3/internal/runtimex" +) + +// This test ensures that a [*Network] created with [NewNetwork] collects stats. +func TestNetworkCollectsStats(t *testing.T) { + // testcase is a test case run by this function + type testcase struct { + // name is the test case name + name string + + // URL is the URL to GET + URL string + + // initialPolicy is the initial policy to configure into the key-value store + initialPolicy func() []byte + + // configureDPI is the function to configure DPI + configureDPI func(dpi *netem.DPIEngine) + + // expectErr is the expected error string + expectErr string + + // statsDomain is the domain to lookup inside the stats + statsDomain string + + // statsTacticsSummary is the summary to lookup inside the stats + // once we have used the statsDomain to get a record + statsTacticsSummary string + + // expectStats contains the expected record containing tactics stats + expectStats *statsTactic + } + + cases := []testcase{ + + { + name: "with TCP connect failure", + URL: "https://api.ooni.io/", + initialPolicy: func() []byte { + p0 := &HTTPSDialerStaticPolicyRoot{ + Domains: map[string][]*HTTPSDialerTactic{ + // This policy has a different SNI and VerifyHostname, which gives + // us confidence that the stats are using the latter + "api.ooni.io": {{ + Endpoint: net.JoinHostPort(netemx.AddressApiOONIIo, "443"), + InitialDelay: 0, + SNI: "www.example.com", + VerifyHostname: "api.ooni.io", + }}, + }, + Version: HTTPSDialerStaticPolicyVersion, + } + return runtimex.Try1(json.Marshal(p0)) + }, + configureDPI: func(dpi *netem.DPIEngine) { + dpi.AddRule(&netem.DPICloseConnectionForServerEndpoint{ + Logger: log.Log, + ServerIPAddress: netemx.AddressApiOONIIo, + ServerPort: 443, + }) + }, + expectErr: `Get "https://api.ooni.io/": connection_refused`, + statsDomain: "api.ooni.io", + statsTacticsSummary: "162.55.247.208:443 sni=www.example.com verify=api.ooni.io", + expectStats: &statsTactic{ + CountStarted: 1, + CountTCPConnectError: 1, + CountTLSHandshakeError: 0, + CountTLSVerificationError: 0, + CountSuccess: 0, + HistoTCPConnectError: map[string]int64{ + "connection_refused": 1, + }, + HistoTLSHandshakeError: map[string]int64{}, + HistoTLSVerificationError: map[string]int64{}, + LastUpdated: time.Time{}, + Tactic: &HTTPSDialerTactic{ + Endpoint: "162.55.247.208:443", + InitialDelay: 0, + SNI: "www.example.com", + VerifyHostname: "api.ooni.io", + }, + }, + }, + + { + name: "with TLS handshake failure", + URL: "https://api.ooni.io/", + initialPolicy: func() []byte { + p0 := &HTTPSDialerStaticPolicyRoot{ + Domains: map[string][]*HTTPSDialerTactic{ + // This policy has a different SNI and VerifyHostname, which gives + // us confidence that the stats are using the latter + "api.ooni.io": {{ + Endpoint: net.JoinHostPort(netemx.AddressApiOONIIo, "443"), + InitialDelay: 0, + SNI: "www.example.com", + VerifyHostname: "api.ooni.io", + }}, + }, + Version: HTTPSDialerStaticPolicyVersion, + } + return runtimex.Try1(json.Marshal(p0)) + }, + configureDPI: func(dpi *netem.DPIEngine) { + dpi.AddRule(&netem.DPIResetTrafficForTLSSNI{ + Logger: log.Log, + SNI: "www.example.com", + }) + }, + expectErr: `Get "https://api.ooni.io/": connection_reset`, + statsDomain: "api.ooni.io", + statsTacticsSummary: "162.55.247.208:443 sni=www.example.com verify=api.ooni.io", + expectStats: &statsTactic{ + CountStarted: 1, + CountTCPConnectError: 0, + CountTLSHandshakeError: 1, + CountTLSVerificationError: 0, + CountSuccess: 0, + HistoTCPConnectError: map[string]int64{}, + HistoTLSHandshakeError: map[string]int64{ + "connection_reset": 1, + }, + HistoTLSVerificationError: map[string]int64{}, + LastUpdated: time.Time{}, + Tactic: &HTTPSDialerTactic{ + Endpoint: "162.55.247.208:443", + InitialDelay: 0, + SNI: "www.example.com", + VerifyHostname: "api.ooni.io", + }, + }, + }, + + { + name: "with TLS verification failure", + URL: "https://api.ooni.io/", + initialPolicy: func() []byte { + p0 := &HTTPSDialerStaticPolicyRoot{ + Domains: map[string][]*HTTPSDialerTactic{ + // This policy has a different SNI and VerifyHostname, which gives + // us confidence that the stats are using the latter + "api.ooni.io": {{ + Endpoint: net.JoinHostPort(netemx.AddressBadSSLCom, "443"), + InitialDelay: 0, + SNI: "untrusted-root.badssl.com", + VerifyHostname: "api.ooni.io", + }}, + }, + Version: HTTPSDialerStaticPolicyVersion, + } + return runtimex.Try1(json.Marshal(p0)) + }, + configureDPI: func(dpi *netem.DPIEngine) { + // nothing + }, + expectErr: `Get "https://api.ooni.io/": ssl_invalid_hostname`, + statsDomain: "api.ooni.io", + statsTacticsSummary: "104.154.89.105:443 sni=untrusted-root.badssl.com verify=api.ooni.io", + expectStats: &statsTactic{ + CountStarted: 1, + CountTCPConnectError: 0, + CountTLSHandshakeError: 0, + CountTLSVerificationError: 1, + CountSuccess: 0, + HistoTCPConnectError: map[string]int64{}, + HistoTLSHandshakeError: map[string]int64{}, + HistoTLSVerificationError: map[string]int64{ + "ssl_invalid_hostname": 1, + }, + LastUpdated: time.Time{}, + Tactic: &HTTPSDialerTactic{ + Endpoint: "104.154.89.105:443", + InitialDelay: 0, + SNI: "untrusted-root.badssl.com", + VerifyHostname: "api.ooni.io", + }, + }, + }} + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + qa := netemx.MustNewScenario(netemx.InternetScenario) + defer qa.Close() + + // make sure we apply specific DPI rules + tc.configureDPI(qa.DPIEngine()) + + // create a memory key-value store where the engine will write stats that later we + // would be able to read to confirm we're collecting stats + kvStore := &kvstore.Memory{} + + initialPolicy := tc.initialPolicy() + t.Logf("initialPolicy: %s", string(initialPolicy)) + if err := kvStore.Set(HTTPSDialerStaticPolicyKey, initialPolicy); err != nil { + t.Fatal(err) + } + + qa.Do(func() { + byteCounter := bytecounter.New() + resolver := netxlite.NewStdlibResolver(log.Log) + + netx := NewNetwork(byteCounter, kvStore, log.Log, nil, resolver) + defer netx.Close() + + client := netx.NewHTTPClient() + + resp, err := client.Get(tc.URL) + + switch { + case err == nil && tc.expectErr == "": + // all good + + case err != nil && tc.expectErr == "": + t.Fatal("expected", tc.expectErr, "but got", err.Error()) + + case err == nil && tc.expectErr != "": + t.Fatal("expected", tc.expectErr, "but got", err) + + case err != nil && tc.expectErr != "": + if tc.expectErr != err.Error() { + t.Fatal("expected", tc.expectErr, "but got", err.Error()) + } + } + + if resp != nil { + defer resp.Body.Close() + } + }) + + // obtain the tactics container for the proper domain + rawStats, err := kvStore.Get(statsKey) + if err != nil { + t.Fatal(err) + } + var rootStats statsContainer + if err := json.Unmarshal(rawStats, &rootStats); err != nil { + t.Fatal(err) + } + tactics, good := rootStats.Domains[tc.statsDomain] + if !good { + t.Fatalf("no such record for `%s`", tc.statsDomain) + } + t.Logf("%+v", tactics) + + // we expect to see a single record + if len(tactics.Tactics) != 1 { + t.Fatal("expected a single tactic") + } + tactic, good := tactics.Tactics[tc.statsTacticsSummary] + if !good { + t.Fatalf("no such record for: %s", tc.statsTacticsSummary) + } + + diffOptions := []cmp.Option{ + cmpopts.IgnoreFields(statsTactic{}, "LastUpdated"), + } + if diff := cmp.Diff(tc.expectStats, tactic, diffOptions...); diff != "" { + t.Fatal(diff) + } + }) + } +} + +func TestLoadStatsContainer(t *testing.T) { + type testcase struct { + // name is the test case name + name string + + // input returns the bytes we should Set into the key-value store + input func() []byte + + // expectedErr is the expected error string or an empty string + expectErr string + + // expectRoot is the expected root container content + expectRoot *statsContainer + } + + cases := []testcase{{ + name: "when the key-value store does not contain any data", + input: func() []byte { + // Note that returning nil causes the code to NOT set anything into the kvstore + return nil + }, + expectErr: "no such key", + expectRoot: nil, + }, { + name: "when we cannot parse the serialized JSON", + input: func() []byte { + return []byte(`{`) + }, + expectErr: "unexpected end of JSON input", + expectRoot: nil, + }, { + name: "with invalid version", + input: func() []byte { + return []byte(`{"Version":1}`) + }, + expectErr: "httpsdialerstats.state: wrong stats container version: expected=2 got=1", + expectRoot: nil, + }, { + name: "on success", + input: func() []byte { + root := &statsContainer{ + Domains: map[string]*statsDomain{ + "api.ooni.io": { + Tactics: map[string]*statsTactic{ + "162.55.247.208:443 sni=www.example.com verify=api.ooni.io": { + CountStarted: 4, + CountTCPConnectError: 1, + CountTLSHandshakeError: 1, + CountTLSVerificationError: 1, + CountSuccess: 1, + HistoTCPConnectError: map[string]int64{ + "connection_refused": 1, + }, + HistoTLSHandshakeError: map[string]int64{ + "generic_timeout_error": 1, + }, + HistoTLSVerificationError: map[string]int64{ + "ssl_invalid_hostname": 1, + }, + LastUpdated: time.Date(2023, 9, 25, 0, 0, 0, 0, time.UTC), + Tactic: &HTTPSDialerTactic{ + Endpoint: "162.55.247.208:443", + InitialDelay: 0, + SNI: "www.example.com", + VerifyHostname: "api.ooni.io", + }, + }, + }, + }, + }, + Version: statsContainerVersion, + } + return runtimex.Try1(json.Marshal(root)) + }, + expectErr: "", + expectRoot: &statsContainer{ + Domains: map[string]*statsDomain{ + "api.ooni.io": { + Tactics: map[string]*statsTactic{ + "162.55.247.208:443 sni=www.example.com verify=api.ooni.io": { + CountStarted: 4, + CountTCPConnectError: 1, + CountTLSHandshakeError: 1, + CountTLSVerificationError: 1, + CountSuccess: 1, + HistoTCPConnectError: map[string]int64{ + "connection_refused": 1, + }, + HistoTLSHandshakeError: map[string]int64{ + "generic_timeout_error": 1, + }, + HistoTLSVerificationError: map[string]int64{ + "ssl_invalid_hostname": 1, + }, + LastUpdated: time.Date(2023, 9, 25, 0, 0, 0, 0, time.UTC), + Tactic: &HTTPSDialerTactic{ + Endpoint: "162.55.247.208:443", + InitialDelay: 0, + SNI: "www.example.com", + VerifyHostname: "api.ooni.io", + }, + }, + }, + }, + }, + Version: statsContainerVersion, + }, + }} + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + kvStore := &kvstore.Memory{} + if input := tc.input(); len(input) > 0 { + if err := kvStore.Set(statsKey, input); err != nil { + t.Fatal(err) + } + } + + root, err := loadStatsContainer(kvStore) + + switch { + case err == nil && tc.expectErr == "": + // all good + + case err != nil && tc.expectErr == "": + t.Fatal("expected", tc.expectErr, "but got", err.Error()) + + case err == nil && tc.expectErr != "": + t.Fatal("expected", tc.expectErr, "but got", err) + + case err != nil && tc.expectErr != "": + if tc.expectErr != err.Error() { + t.Fatal("expected", tc.expectErr, "but got", err.Error()) + } + } + + if diff := cmp.Diff(tc.expectRoot, root); diff != "" { + t.Fatal(diff) + } + }) + } +} + +func TestStatsManagerCallbacks(t *testing.T) { + type testcase struct { + name string + initialRoot *statsContainer + do func(stats *statsManager) + expectWarnf int + expectRoot *statsContainer + } + + cases := []testcase{ + + // When TCP connect fails and the reason is a canceled context + { + name: "OnTCPConnectError with ctx.Error() != nil", + initialRoot: &statsContainer{ + Domains: map[string]*statsDomain{ + "api.ooni.io": { + Tactics: map[string]*statsTactic{ + "162.55.247.208:443 sni=www.example.com verify=api.ooni.io": { + CountStarted: 1, + }, + }, + }, + }, + Version: statsContainerVersion, + }, + do: func(stats *statsManager) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() // immediately! + + tactic := &HTTPSDialerTactic{ + Endpoint: "162.55.247.208:443", + InitialDelay: 0, + SNI: "www.example.com", + VerifyHostname: "api.ooni.io", + } + err := errors.New("generic_timeout_error") + + stats.OnTCPConnectError(ctx, tactic, err) + }, + expectWarnf: 0, + expectRoot: &statsContainer{ + Domains: map[string]*statsDomain{ + "api.ooni.io": { + Tactics: map[string]*statsTactic{ + "162.55.247.208:443 sni=www.example.com verify=api.ooni.io": { + CountStarted: 1, + CountTCPConnectInterrupt: 1, + }, + }, + }, + }, + Version: statsContainerVersion, + }, + }, + + // When TCP connect fails and we don't already have a policy record + { + name: "OnTCPConnectError when we are missing the stats record for the domain", + initialRoot: &statsContainer{ + Domains: map[string]*statsDomain{}, + Version: statsContainerVersion, + }, + do: func(stats *statsManager) { + ctx := context.Background() + + tactic := &HTTPSDialerTactic{ + Endpoint: "162.55.247.208:443", + InitialDelay: 0, + SNI: "www.example.com", + VerifyHostname: "api.ooni.io", + } + err := errors.New("generic_timeout_error") + + stats.OnTCPConnectError(ctx, tactic, err) + }, + expectWarnf: 1, + expectRoot: &statsContainer{ + Domains: map[string]*statsDomain{}, + Version: statsContainerVersion, + }, + }, + + // When TLS handshake fails and the reason is a canceled context + { + name: "OnTLSHandshakeError with ctx.Error() != nil", + initialRoot: &statsContainer{ + Domains: map[string]*statsDomain{ + "api.ooni.io": { + Tactics: map[string]*statsTactic{ + "162.55.247.208:443 sni=www.example.com verify=api.ooni.io": { + CountStarted: 1, + }, + }, + }, + }, + Version: statsContainerVersion, + }, + do: func(stats *statsManager) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() // immediately! + + tactic := &HTTPSDialerTactic{ + Endpoint: "162.55.247.208:443", + InitialDelay: 0, + SNI: "www.example.com", + VerifyHostname: "api.ooni.io", + } + err := errors.New("generic_timeout_error") + + stats.OnTLSHandshakeError(ctx, tactic, err) + }, + expectWarnf: 0, + expectRoot: &statsContainer{ + Domains: map[string]*statsDomain{ + "api.ooni.io": { + Tactics: map[string]*statsTactic{ + "162.55.247.208:443 sni=www.example.com verify=api.ooni.io": { + CountStarted: 1, + CountTLSHandshakeInterrupt: 1, + }, + }, + }, + }, + Version: statsContainerVersion, + }, + }, + + // When TLS handshake fails and we don't already have a policy record + { + name: "OnTLSHandshakeError when we are missing the stats record for the domain", + initialRoot: &statsContainer{ + Domains: map[string]*statsDomain{}, + Version: statsContainerVersion, + }, + do: func(stats *statsManager) { + ctx := context.Background() + + tactic := &HTTPSDialerTactic{ + Endpoint: "162.55.247.208:443", + InitialDelay: 0, + SNI: "www.example.com", + VerifyHostname: "api.ooni.io", + } + err := errors.New("generic_timeout_error") + + stats.OnTLSHandshakeError(ctx, tactic, err) + }, + expectWarnf: 1, + expectRoot: &statsContainer{ + Domains: map[string]*statsDomain{}, + Version: statsContainerVersion, + }, + }, + + // When TLS verification fails and we don't already have a policy record + { + name: "OnTLSVerifyError when we are missing the stats record for the domain", + initialRoot: &statsContainer{ + Domains: map[string]*statsDomain{}, + Version: statsContainerVersion, + }, + do: func(stats *statsManager) { + tactic := &HTTPSDialerTactic{ + Endpoint: "162.55.247.208:443", + InitialDelay: 0, + SNI: "www.example.com", + VerifyHostname: "api.ooni.io", + } + err := errors.New("generic_timeout_error") + + stats.OnTLSVerifyError(tactic, err) + }, + expectWarnf: 1, + expectRoot: &statsContainer{ + Domains: map[string]*statsDomain{}, + Version: statsContainerVersion, + }, + }, + + // With success when we don't already have a policy record + { + name: "OnSuccess when we are missing the stats record for the domain", + initialRoot: &statsContainer{ + Domains: map[string]*statsDomain{}, + Version: statsContainerVersion, + }, + do: func(stats *statsManager) { + tactic := &HTTPSDialerTactic{ + Endpoint: "162.55.247.208:443", + InitialDelay: 0, + SNI: "www.example.com", + VerifyHostname: "api.ooni.io", + } + + stats.OnSuccess(tactic) + }, + expectWarnf: 1, + expectRoot: &statsContainer{ + Domains: map[string]*statsDomain{}, + Version: statsContainerVersion, + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + // configure the initial value of the stats + kvStore := &kvstore.Memory{} + if err := kvStore.Set(statsKey, runtimex.Try1(json.Marshal(tc.initialRoot))); err != nil { + t.Fatal(err) + } + + // create logger counting the number Warnf invocations + var warnfCount int + logger := &mocks.Logger{ + MockWarnf: func(format string, v ...any) { + warnfCount++ + }, + } + + // create the stats manager + stats := newStatsManager(kvStore, logger) + + // invoke the proper stats callback + tc.do(stats) + + // close the stats to trigger a kvstore write + if err := stats.Close(); err != nil { + t.Fatal(err) + } + + // extract the possibly modified stats from the kvstore + var root *statsContainer + rawRoot, err := kvStore.Get(statsKey) + if err != nil { + t.Fatal(err) + } + if err := json.Unmarshal(rawRoot, &root); err != nil { + t.Fatal(err) + } + + // make sure the stats are the ones we expect + diffOptions := []cmp.Option{ + cmpopts.IgnoreFields(statsTactic{}, "LastUpdated"), + } + if diff := cmp.Diff(tc.expectRoot, root, diffOptions...); diff != "" { + t.Fatal(diff) + } + + // make sure we logged if necessary + if tc.expectWarnf != warnfCount { + t.Fatal("expected", tc.expectWarnf, "got", warnfCount) + } + }) + } +}