From 1f7d386ef21ef2f9e54c5bf2db27efa185ce0aec Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 26 Sep 2023 15:50:59 +0200 Subject: [PATCH] refactor(enginenetx): group by domain's endpoint (#1307) There's no point in grouping tactics by "www.example.com" for stats stored on disk and user-provided static policies. By grouping by domain alone, we'll end up with mixed tactics for different ports all being together. Which means that, code that needs to filter, needs to explicitly filter by port. If we used "www.example.com:443" as the group-by key, conversely, we would not need to write specialized filtering code to only get data for the correct port. So, let's do that and let's simplify future code that will need to get tactics known for work for a given domain _and_ port. Part of https://github.com/ooni/probe/issues/2531 --- internal/enginenetx/httpsdialercore.go | 5 ++ internal/enginenetx/httpsdialerstatic.go | 9 +- internal/enginenetx/httpsdialerstatic_test.go | 14 +-- internal/enginenetx/network_test.go | 4 +- internal/enginenetx/stats.go | 42 ++++----- internal/enginenetx/stats_test.go | 86 +++++++++---------- 6 files changed, 83 insertions(+), 77 deletions(-) diff --git a/internal/enginenetx/httpsdialercore.go b/internal/enginenetx/httpsdialercore.go index 141ccb3753..32b801b3a1 100644 --- a/internal/enginenetx/httpsdialercore.go +++ b/internal/enginenetx/httpsdialercore.go @@ -71,6 +71,11 @@ func (dt *HTTPSDialerTactic) Summary() string { return fmt.Sprintf("%v sni=%v verify=%v", net.JoinHostPort(dt.Address, dt.Port), dt.SNI, dt.VerifyHostname) } +// domainEndpointKey returns the domain's endpoint string key for storing into a map. +func (dt *HTTPSDialerTactic) domainEndpointKey() string { + return net.JoinHostPort(dt.VerifyHostname, dt.Port) +} + // HTTPSDialerPolicy describes the policy used by the [*HTTPSDialer]. type HTTPSDialerPolicy interface { // LookupTactics returns zero or more tactics for the given host and port. diff --git a/internal/enginenetx/httpsdialerstatic.go b/internal/enginenetx/httpsdialerstatic.go index e09a2f814e..3dfcc7790a 100644 --- a/internal/enginenetx/httpsdialerstatic.go +++ b/internal/enginenetx/httpsdialerstatic.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "net" "github.com/ooni/probe-cli/v3/internal/hujsonx" "github.com/ooni/probe-cli/v3/internal/model" @@ -65,12 +66,12 @@ func NewHTTPSDialerStaticPolicy( } // HTTPSDialerStaticPolicyVersion is the current version of the static policy file. -const HTTPSDialerStaticPolicyVersion = 2 +const HTTPSDialerStaticPolicyVersion = 3 // HTTPSDialerStaticPolicyRoot is the root of a statically loaded policy. type HTTPSDialerStaticPolicyRoot struct { - // Domains maps each domain to its policy. - Domains map[string][]*HTTPSDialerTactic + // DomainEndpoints maps each domain endpoint to its policies. + DomainEndpoints map[string][]*HTTPSDialerTactic // Version is the data structure version. Version int @@ -81,7 +82,7 @@ var _ HTTPSDialerPolicy = &HTTPSDialerStaticPolicy{} // LookupTactics implements HTTPSDialerPolicy. func (ldp *HTTPSDialerStaticPolicy) LookupTactics( ctx context.Context, domain string, port string) <-chan *HTTPSDialerTactic { - tactics, found := ldp.Root.Domains[domain] + tactics, found := ldp.Root.DomainEndpoints[net.JoinHostPort(domain, port)] if !found { return ldp.Fallback.LookupTactics(ctx, domain, port) } diff --git a/internal/enginenetx/httpsdialerstatic_test.go b/internal/enginenetx/httpsdialerstatic_test.go index 8a8295e716..d56bf2d131 100644 --- a/internal/enginenetx/httpsdialerstatic_test.go +++ b/internal/enginenetx/httpsdialerstatic_test.go @@ -57,15 +57,15 @@ func TestHTTPSDialerStaticPolicy(t *testing.T) { name: "with empty JSON", key: HTTPSDialerStaticPolicyKey, input: []byte(`{}`), - expectErr: "httpsdialerstatic.conf: wrong static policy version: expected=2 got=0", + expectErr: "httpsdialerstatic.conf: wrong static policy version: expected=3 got=0", expectedPolicy: nil, }, { name: "with real serialized policy", key: HTTPSDialerStaticPolicyKey, input: (func() []byte { return runtimex.Try1(json.Marshal(&HTTPSDialerStaticPolicyRoot{ - Domains: map[string][]*HTTPSDialerTactic{ - "api.ooni.io": {{ + DomainEndpoints: map[string][]*HTTPSDialerTactic{ + "api.ooni.io:443": {{ Address: "162.55.247.208", InitialDelay: 0, Port: "443", @@ -104,8 +104,8 @@ func TestHTTPSDialerStaticPolicy(t *testing.T) { expectedPolicy: &HTTPSDialerStaticPolicy{ Fallback: fallback, Root: &HTTPSDialerStaticPolicyRoot{ - Domains: map[string][]*HTTPSDialerTactic{ - "api.ooni.io": {{ + DomainEndpoints: map[string][]*HTTPSDialerTactic{ + "api.ooni.io:443": {{ Address: "162.55.247.208", InitialDelay: 0, Port: "443", @@ -181,8 +181,8 @@ func TestHTTPSDialerStaticPolicy(t *testing.T) { VerifyHostname: "api.ooni.io", } staticPolicyRoot := &HTTPSDialerStaticPolicyRoot{ - Domains: map[string][]*HTTPSDialerTactic{ - "api.ooni.io": {expectedTactic}, + DomainEndpoints: map[string][]*HTTPSDialerTactic{ + "api.ooni.io:443": {expectedTactic}, }, Version: HTTPSDialerStaticPolicyVersion, } diff --git a/internal/enginenetx/network_test.go b/internal/enginenetx/network_test.go index c2c71ad0af..80d9163f6e 100644 --- a/internal/enginenetx/network_test.go +++ b/internal/enginenetx/network_test.go @@ -301,8 +301,8 @@ func TestNetworkQA(t *testing.T) { name: "when there's a user-provided policy", kvStore: func() model.KeyValueStore { policy := &enginenetx.HTTPSDialerStaticPolicyRoot{ - Domains: map[string][]*enginenetx.HTTPSDialerTactic{ - "www.example.com": {{ + DomainEndpoints: map[string][]*enginenetx.HTTPSDialerTactic{ + "www.example.com:443": {{ Address: netemx.AddressApiOONIIo, InitialDelay: 0, Port: "443", diff --git a/internal/enginenetx/stats.go b/internal/enginenetx/stats.go index 30f3124e32..3f7ab8bab9 100644 --- a/internal/enginenetx/stats.go +++ b/internal/enginenetx/stats.go @@ -79,14 +79,14 @@ func (st *statsTactic) Clone() *statsTactic { } } -// statsDomain contains stats associated with a domain. -type statsDomain struct { +// statsDomainEndpoint contains stats associated with a domain endpoint. +type statsDomainEndpoint struct { Tactics map[string]*statsTactic } -// statsDomainRemoveOldEntries returns a copy of a [*statsDomain] with old entries removed. -func statsDomainRemoveOldEntries(input *statsDomain) (output *statsDomain) { - output = &statsDomain{ +// statsDomainEndpointRemoveOldEntries returns a copy of a [*statsDomainEndpoint] with old entries removed. +func statsDomainEndpointRemoveOldEntries(input *statsDomainEndpoint) (output *statsDomainEndpoint) { + output = &statsDomainEndpoint{ Tactics: map[string]*statsTactic{}, } oneWeek := 7 * 24 * time.Hour @@ -101,14 +101,14 @@ func statsDomainRemoveOldEntries(input *statsDomain) (output *statsDomain) { } // statsContainerVersion is the current version of [statsContainer]. -const statsContainerVersion = 4 +const statsContainerVersion = 5 // statsContainer is the root container for the stats. // // The zero value is invalid; construct using [newStatsContainer]. type statsContainer struct { - // Domains maps a domain name to its tactics. - Domains map[string]*statsDomain + // DomainEndpoints maps a domain endpoint to its tactics. + DomainEndpoints map[string]*statsDomainEndpoint // Version is the version of the container data format. Version int @@ -117,12 +117,12 @@ type statsContainer struct { // statsDomainRemoveOldEntries returns a copy of a [*statsContainer] with old entries removed. func statsContainerRemoveOldEntries(input *statsContainer) (output *statsContainer) { output = newStatsContainer() - for domain, inputStats := range input.Domains { - prunedStats := statsDomainRemoveOldEntries(inputStats) + for domainEpnt, inputStats := range input.DomainEndpoints { + prunedStats := statsDomainEndpointRemoveOldEntries(inputStats) if len(prunedStats.Tactics) <= 0 { continue } - output.Domains[domain] = prunedStats + output.DomainEndpoints[domainEpnt] = prunedStats } return } @@ -131,11 +131,11 @@ func statsContainerRemoveOldEntries(input *statsContainer) (output *statsContain // // At the name implies, this function MUST be called while holding the [*statsManager] mutex. func (c *statsContainer) GetStatsTacticLocked(tactic *HTTPSDialerTactic) (*statsTactic, bool) { - domainRecord, found := c.Domains[tactic.VerifyHostname] + domainEpntRecord, found := c.DomainEndpoints[tactic.domainEndpointKey()] if !found { return nil, false } - tacticRecord, found := domainRecord.Tactics[tactic.Summary()] + tacticRecord, found := domainEpntRecord.Tactics[tactic.Summary()] return tacticRecord, found } @@ -143,28 +143,28 @@ func (c *statsContainer) GetStatsTacticLocked(tactic *HTTPSDialerTactic) (*stats // // At the name implies, this function MUST be called while holding the [*statsManager] mutex. func (c *statsContainer) SetStatsTacticLocked(tactic *HTTPSDialerTactic, record *statsTactic) { - domainRecord, found := c.Domains[tactic.VerifyHostname] + domainEpntRecord, found := c.DomainEndpoints[tactic.domainEndpointKey()] if !found { - domainRecord = &statsDomain{ + domainEpntRecord = &statsDomainEndpoint{ Tactics: map[string]*statsTactic{}, } // make sure the map is initialized - if len(c.Domains) <= 0 { - c.Domains = make(map[string]*statsDomain) + if len(c.DomainEndpoints) <= 0 { + c.DomainEndpoints = make(map[string]*statsDomainEndpoint) } - c.Domains[tactic.VerifyHostname] = domainRecord + c.DomainEndpoints[tactic.domainEndpointKey()] = domainEpntRecord // fallthrough } - domainRecord.Tactics[tactic.Summary()] = record + domainEpntRecord.Tactics[tactic.Summary()] = record } // newStatsContainer creates a new empty [*statsContainer]. func newStatsContainer() *statsContainer { return &statsContainer{ - Domains: map[string]*statsDomain{}, - Version: statsContainerVersion, + DomainEndpoints: map[string]*statsDomainEndpoint{}, + Version: statsContainerVersion, } } diff --git a/internal/enginenetx/stats_test.go b/internal/enginenetx/stats_test.go index 732d70eb76..1945312c80 100644 --- a/internal/enginenetx/stats_test.go +++ b/internal/enginenetx/stats_test.go @@ -38,8 +38,8 @@ func TestNetworkCollectsStats(t *testing.T) { // expectErr is the expected error string expectErr string - // statsDomain is the domain to lookup inside the stats - statsDomain string + // statsDomainEpnt is the domain endpoint to lookup inside the stats + statsDomainEpnt string // statsTacticsSummary is the summary to lookup inside the stats // once we have used the statsDomain to get a record @@ -56,10 +56,10 @@ func TestNetworkCollectsStats(t *testing.T) { URL: "https://api.ooni.io/", initialPolicy: func() []byte { p0 := &HTTPSDialerStaticPolicyRoot{ - Domains: map[string][]*HTTPSDialerTactic{ + DomainEndpoints: 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": {{ + "api.ooni.io:443": {{ Address: netemx.AddressApiOONIIo, InitialDelay: 0, Port: "443", @@ -79,7 +79,7 @@ func TestNetworkCollectsStats(t *testing.T) { }) }, expectErr: `Get "https://api.ooni.io/": connection_refused`, - statsDomain: "api.ooni.io", + statsDomainEpnt: "api.ooni.io:443", statsTacticsSummary: "162.55.247.208:443 sni=www.example.com verify=api.ooni.io", expectStats: &statsTactic{ CountStarted: 1, @@ -108,10 +108,10 @@ func TestNetworkCollectsStats(t *testing.T) { URL: "https://api.ooni.io/", initialPolicy: func() []byte { p0 := &HTTPSDialerStaticPolicyRoot{ - Domains: map[string][]*HTTPSDialerTactic{ + DomainEndpoints: 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": {{ + "api.ooni.io:443": {{ Address: netemx.AddressApiOONIIo, InitialDelay: 0, Port: "443", @@ -130,7 +130,7 @@ func TestNetworkCollectsStats(t *testing.T) { }) }, expectErr: `Get "https://api.ooni.io/": connection_reset`, - statsDomain: "api.ooni.io", + statsDomainEpnt: "api.ooni.io:443", statsTacticsSummary: "162.55.247.208:443 sni=www.example.com verify=api.ooni.io", expectStats: &statsTactic{ CountStarted: 1, @@ -159,10 +159,10 @@ func TestNetworkCollectsStats(t *testing.T) { URL: "https://api.ooni.io/", initialPolicy: func() []byte { p0 := &HTTPSDialerStaticPolicyRoot{ - Domains: map[string][]*HTTPSDialerTactic{ + DomainEndpoints: 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": {{ + "api.ooni.io:443": {{ Address: netemx.AddressBadSSLCom, InitialDelay: 0, Port: "443", @@ -178,7 +178,7 @@ func TestNetworkCollectsStats(t *testing.T) { // nothing }, expectErr: `Get "https://api.ooni.io/": ssl_invalid_hostname`, - statsDomain: "api.ooni.io", + statsDomainEpnt: "api.ooni.io:443", statsTacticsSummary: "104.154.89.105:443 sni=untrusted-root.badssl.com verify=api.ooni.io", expectStats: &statsTactic{ CountStarted: 1, @@ -261,9 +261,9 @@ func TestNetworkCollectsStats(t *testing.T) { if err := json.Unmarshal(rawStats, &rootStats); err != nil { t.Fatal(err) } - tactics, good := rootStats.Domains[tc.statsDomain] + tactics, good := rootStats.DomainEndpoints[tc.statsDomainEpnt] if !good { - t.Fatalf("no such record for `%s`", tc.statsDomain) + t.Fatalf("no such record for `%s`", tc.statsDomainEpnt) } t.Logf("%+v", tactics) @@ -325,14 +325,14 @@ func TestLoadStatsContainer(t *testing.T) { input: func() []byte { return []byte(`{"Version":1}`) }, - expectErr: "httpsdialerstats.state: wrong stats container version: expected=4 got=1", + expectErr: "httpsdialerstats.state: wrong stats container version: expected=5 got=1", expectRoot: nil, }, { name: "on success including correct entries pruning", input: func() []byte { root := &statsContainer{ - Domains: map[string]*statsDomain{ - "api.ooni.io": { + DomainEndpoints: map[string]*statsDomainEndpoint{ + "api.ooni.io:443": { Tactics: map[string]*statsTactic{ "162.55.247.208:443 sni=www.example.com verify=api.ooni.io": { CountStarted: 4, @@ -384,7 +384,7 @@ func TestLoadStatsContainer(t *testing.T) { }, }, }, - "www.kernel.org": { // this whole entry should be skipped because it's too old + "www.kernel.org:443": { // this whole entry should be skipped because it's too old Tactics: map[string]*statsTactic{ "162.55.247.208:443 sni=www.example.com verify=www.kernel.org": { CountStarted: 4, @@ -419,8 +419,8 @@ func TestLoadStatsContainer(t *testing.T) { }, expectErr: "", expectRoot: &statsContainer{ - Domains: map[string]*statsDomain{ - "api.ooni.io": { + DomainEndpoints: map[string]*statsDomainEndpoint{ + "api.ooni.io:443": { Tactics: map[string]*statsTactic{ "162.55.247.208:443 sni=www.example.com verify=api.ooni.io": { CountStarted: 4, @@ -504,8 +504,8 @@ func TestStatsManagerCallbacks(t *testing.T) { { name: "OnTCPConnectError with ctx.Error() != nil", initialRoot: &statsContainer{ - Domains: map[string]*statsDomain{ - "api.ooni.io": { + DomainEndpoints: map[string]*statsDomainEndpoint{ + "api.ooni.io:443": { Tactics: map[string]*statsTactic{ "162.55.247.208:443 sni=www.example.com verify=api.ooni.io": { CountStarted: 1, @@ -534,8 +534,8 @@ func TestStatsManagerCallbacks(t *testing.T) { }, expectWarnf: 0, expectRoot: &statsContainer{ - Domains: map[string]*statsDomain{ - "api.ooni.io": { + DomainEndpoints: map[string]*statsDomainEndpoint{ + "api.ooni.io:443": { Tactics: map[string]*statsTactic{ "162.55.247.208:443 sni=www.example.com verify=api.ooni.io": { CountStarted: 1, @@ -553,8 +553,8 @@ func TestStatsManagerCallbacks(t *testing.T) { { name: "OnTCPConnectError when we are missing the stats record for the domain", initialRoot: &statsContainer{ - Domains: map[string]*statsDomain{}, - Version: statsContainerVersion, + DomainEndpoints: map[string]*statsDomainEndpoint{}, + Version: statsContainerVersion, }, do: func(stats *statsManager) { ctx := context.Background() @@ -572,8 +572,8 @@ func TestStatsManagerCallbacks(t *testing.T) { }, expectWarnf: 1, expectRoot: &statsContainer{ - Domains: map[string]*statsDomain{}, - Version: statsContainerVersion, + DomainEndpoints: map[string]*statsDomainEndpoint{}, + Version: statsContainerVersion, }, }, @@ -581,8 +581,8 @@ func TestStatsManagerCallbacks(t *testing.T) { { name: "OnTLSHandshakeError with ctx.Error() != nil", initialRoot: &statsContainer{ - Domains: map[string]*statsDomain{ - "api.ooni.io": { + DomainEndpoints: map[string]*statsDomainEndpoint{ + "api.ooni.io:443": { Tactics: map[string]*statsTactic{ "162.55.247.208:443 sni=www.example.com verify=api.ooni.io": { CountStarted: 1, @@ -611,8 +611,8 @@ func TestStatsManagerCallbacks(t *testing.T) { }, expectWarnf: 0, expectRoot: &statsContainer{ - Domains: map[string]*statsDomain{ - "api.ooni.io": { + DomainEndpoints: map[string]*statsDomainEndpoint{ + "api.ooni.io:443": { Tactics: map[string]*statsTactic{ "162.55.247.208:443 sni=www.example.com verify=api.ooni.io": { CountStarted: 1, @@ -630,8 +630,8 @@ func TestStatsManagerCallbacks(t *testing.T) { { name: "OnTLSHandshakeError when we are missing the stats record for the domain", initialRoot: &statsContainer{ - Domains: map[string]*statsDomain{}, - Version: statsContainerVersion, + DomainEndpoints: map[string]*statsDomainEndpoint{}, + Version: statsContainerVersion, }, do: func(stats *statsManager) { ctx := context.Background() @@ -649,8 +649,8 @@ func TestStatsManagerCallbacks(t *testing.T) { }, expectWarnf: 1, expectRoot: &statsContainer{ - Domains: map[string]*statsDomain{}, - Version: statsContainerVersion, + DomainEndpoints: map[string]*statsDomainEndpoint{}, + Version: statsContainerVersion, }, }, @@ -658,8 +658,8 @@ func TestStatsManagerCallbacks(t *testing.T) { { name: "OnTLSVerifyError when we are missing the stats record for the domain", initialRoot: &statsContainer{ - Domains: map[string]*statsDomain{}, - Version: statsContainerVersion, + DomainEndpoints: map[string]*statsDomainEndpoint{}, + Version: statsContainerVersion, }, do: func(stats *statsManager) { tactic := &HTTPSDialerTactic{ @@ -675,8 +675,8 @@ func TestStatsManagerCallbacks(t *testing.T) { }, expectWarnf: 1, expectRoot: &statsContainer{ - Domains: map[string]*statsDomain{}, - Version: statsContainerVersion, + DomainEndpoints: map[string]*statsDomainEndpoint{}, + Version: statsContainerVersion, }, }, @@ -684,8 +684,8 @@ func TestStatsManagerCallbacks(t *testing.T) { { name: "OnSuccess when we are missing the stats record for the domain", initialRoot: &statsContainer{ - Domains: map[string]*statsDomain{}, - Version: statsContainerVersion, + DomainEndpoints: map[string]*statsDomainEndpoint{}, + Version: statsContainerVersion, }, do: func(stats *statsManager) { tactic := &HTTPSDialerTactic{ @@ -700,8 +700,8 @@ func TestStatsManagerCallbacks(t *testing.T) { }, expectWarnf: 1, expectRoot: &statsContainer{ - Domains: map[string]*statsDomain{}, - Version: statsContainerVersion, + DomainEndpoints: map[string]*statsDomainEndpoint{}, + Version: statsContainerVersion, }, }, }