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 ooni/probe#2531

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.

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)
 	}

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,
 		}

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",

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,40 +131,40 @@ 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
 }
 
 // SetStatsTacticLocked sets the tactic record for the given the given [*statsTactic] instance.
 //
 // 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,
 	}
 }