refactor(enginenetx): make static-policy API private (#1308)

Now that we've more or less reached the point where we wanted to be with
ooni/probe#2531, let's spend some time to
refactor the implementation, now that we know very well how it 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.

Here, in particular, we deal with the static-policy API. To this end, we
also need to more some tests around because they need internals.

Also, while there, add a top-level description to some files, so that
newcomers have an idea of what each file is about.

internal/enginenetx/beacons.go

@@ -1,5 +1,10 @@
 package enginenetx
 
+//
+// beacons policy - a policy where we treat some IP addresses as special for
+// some domains, bypassing DNS lookups and using custom SNIs
+//
+
 import (
 	"context"
 	"math/rand"

internal/enginenetx/network.go

@@ -1,5 +1,10 @@
 package enginenetx
 
+//
+// Network - the top-level object of this package, used by the
+// OONI engine to communicate with several backends
+//
+
 import (
 	"net/http"
 	"net/http/cookiejar"
@@ -141,7 +146,7 @@ func newHTTPSDialerPolicy(kvStore model.KeyValueStore, logger model.Logger, reso
 	}
 
 	// make sure we honor a user-provided policy
-	policy, err := NewHTTPSDialerStaticPolicy(kvStore, fallback)
+	policy, err := newStaticPolicy(kvStore, fallback)
 	if err != nil {
 		return fallback
 	}

internal/enginenetx/network_internal_test.go

@@ -1,12 +1,20 @@
 package enginenetx
 
 import (
+	"context"
+	"encoding/json"
 	"sync"
 	"testing"
 
+	"github.com/apex/log"
+	"github.com/google/go-cmp/cmp"
+	"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/model"
+	"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 TestNetworkUnit(t *testing.T) {
@@ -75,4 +83,88 @@ func TestNetworkUnit(t *testing.T) {
 			t.Fatal("did not call the transport's CloseIdleConnections")
 		}
 	})
+
+	t.Run("NewNetwork uses the correct HTTPSDialerPolicy", func(t *testing.T) {
+		// testcase is a test case run by this func
+		type testcase struct {
+			name         string
+			kvStore      func() model.KeyValueStore
+			expectStatus int
+			expectBody   []byte
+		}
+
+		cases := []testcase{
+			// Without a policy accessing www.example.com should lead to 200 as status
+			// code and the expected web page when we're using netem
+			{
+				name: "when there is no user-provided policy",
+				kvStore: func() model.KeyValueStore {
+					return &kvstore.Memory{}
+				},
+				expectStatus: 200,
+				expectBody:   []byte(netemx.ExampleWebPage),
+			},
+
+			// But we can create a policy that can land us on a different website (not the
+			// typical use case of the policy, but definitely demonstrating it works)
+			{
+				name: "when there's a user-provided policy",
+				kvStore: func() model.KeyValueStore {
+					policy := &staticPolicyRoot{
+						DomainEndpoints: map[string][]*HTTPSDialerTactic{
+							"www.example.com:443": {{
+								Address:        netemx.AddressApiOONIIo,
+								InitialDelay:   0,
+								Port:           "443",
+								SNI:            "www.example.com",
+								VerifyHostname: "api.ooni.io",
+							}},
+						},
+						Version: staticPolicyVersion,
+					}
+					rawPolicy := runtimex.Try1(json.Marshal(policy))
+					kvStore := &kvstore.Memory{}
+					runtimex.Try0(kvStore.Set(staticPolicyKey, rawPolicy))
+					return kvStore
+				},
+				expectStatus: 404,
+				expectBody:   []byte{},
+			},
+		}
+
+		for _, tc := range cases {
+			t.Run(tc.name, func(t *testing.T) {
+				env := netemx.MustNewScenario(netemx.InternetScenario)
+				defer env.Close()
+
+				env.Do(func() {
+					netx := NewNetwork(
+						bytecounter.New(),
+						tc.kvStore(),
+						log.Log,
+						nil, // proxy URL
+						netxlite.NewStdlibResolver(log.Log),
+					)
+					defer netx.Close()
+
+					client := netx.NewHTTPClient()
+					resp, err := client.Get("https://www.example.com/")
+					if err != nil {
+						t.Fatal(err)
+					}
+					defer resp.Body.Close()
+					if resp.StatusCode != tc.expectStatus {
+						t.Fatal("StatusCode: expected", tc.expectStatus, "got", resp.StatusCode)
+					}
+					data, err := netxlite.ReadAllContext(context.Background(), resp.Body)
+					if err != nil {
+						t.Fatal(err)
+					}
+					if diff := cmp.Diff(tc.expectBody, data); diff != "" {
+						t.Fatal(diff)
+					}
+				})
+			})
+		}
+	})
 }

internal/enginenetx/network_test.go

@@ -2,23 +2,19 @@ package enginenetx_test
 
 import (
 	"context"
-	"encoding/json"
 	"net"
 	"net/http"
 	"net/url"
 	"testing"
 	"time"
 
 	"github.com/apex/log"
-	"github.com/google/go-cmp/cmp"
 	"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/measurexlite"
-	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/netemx"
 	"github.com/ooni/probe-cli/v3/internal/netxlite"
-	"github.com/ooni/probe-cli/v3/internal/runtimex"
 	"github.com/ooni/probe-cli/v3/internal/testingsocks5"
 	"github.com/ooni/probe-cli/v3/internal/testingx"
 )
@@ -273,88 +269,4 @@ func TestNetworkQA(t *testing.T) {
 			t.Fatal("expected non-nil cookie jar")
 		}
 	})
-
-	t.Run("NewNetwork uses the correct HTTPSDialerPolicy", func(t *testing.T) {
-		// testcase is a test case run by this func
-		type testcase struct {
-			name         string
-			kvStore      func() model.KeyValueStore
-			expectStatus int
-			expectBody   []byte
-		}
-
-		cases := []testcase{
-			// Without a policy accessing www.example.com should lead to 200 as status
-			// code and the expected web page when we're using netem
-			{
-				name: "when there is no user-provided policy",
-				kvStore: func() model.KeyValueStore {
-					return &kvstore.Memory{}
-				},
-				expectStatus: 200,
-				expectBody:   []byte(netemx.ExampleWebPage),
-			},
-
-			// But we can create a policy that can land us on a different website (not the
-			// typical use case of the policy, but definitely demonstrating it works)
-			{
-				name: "when there's a user-provided policy",
-				kvStore: func() model.KeyValueStore {
-					policy := &enginenetx.HTTPSDialerStaticPolicyRoot{
-						DomainEndpoints: map[string][]*enginenetx.HTTPSDialerTactic{
-							"www.example.com:443": {{
-								Address:        netemx.AddressApiOONIIo,
-								InitialDelay:   0,
-								Port:           "443",
-								SNI:            "www.example.com",
-								VerifyHostname: "api.ooni.io",
-							}},
-						},
-						Version: enginenetx.HTTPSDialerStaticPolicyVersion,
-					}
-					rawPolicy := runtimex.Try1(json.Marshal(policy))
-					kvStore := &kvstore.Memory{}
-					runtimex.Try0(kvStore.Set(enginenetx.HTTPSDialerStaticPolicyKey, rawPolicy))
-					return kvStore
-				},
-				expectStatus: 404,
-				expectBody:   []byte{},
-			},
-		}
-
-		for _, tc := range cases {
-			t.Run(tc.name, func(t *testing.T) {
-				env := netemx.MustNewScenario(netemx.InternetScenario)
-				defer env.Close()
-
-				env.Do(func() {
-					netx := enginenetx.NewNetwork(
-						bytecounter.New(),
-						tc.kvStore(),
-						log.Log,
-						nil, // proxy URL
-						netxlite.NewStdlibResolver(log.Log),
-					)
-					defer netx.Close()
-
-					client := netx.NewHTTPClient()
-					resp, err := client.Get("https://www.example.com/")
-					if err != nil {
-						t.Fatal(err)
-					}
-					defer resp.Body.Close()
-					if resp.StatusCode != tc.expectStatus {
-						t.Fatal("StatusCode: expected", tc.expectStatus, "got", resp.StatusCode)
-					}
-					data, err := netxlite.ReadAllContext(context.Background(), resp.Body)
-					if err != nil {
-						t.Fatal(err)
-					}
-					if diff := cmp.Diff(tc.expectBody, data); diff != "" {
-						t.Fatal(diff)
-					}
-				})
-			})
-		}
-	})
 }

internal/enginenetx/httpsdialerstatic.go → internal/enginenetx/static.go

@@ -1,5 +1,13 @@
 package enginenetx
 
+//
+// static policy - the possibility of loading a static policy from a JSON
+// document named `httpsdialerstatic.conf` in $OONI_HOME/engine that contains
+// a specific policy for TLS dialing for specific endpoints.
+//
+// This policy helps a lot with exploration and experimentation.
+//
+
 import (
 	"context"
 	"errors"
@@ -10,77 +18,77 @@ import (
 	"github.com/ooni/probe-cli/v3/internal/model"
 )
 
-// HTTPSDialerStaticPolicy is an [HTTPSDialerPolicy] incorporating verbatim
+// staticPolicy is an [HTTPSDialerPolicy] incorporating verbatim
 // a static policy loaded from the engine's key-value store.
 //
 // This policy is very useful for exploration and experimentation.
-type HTTPSDialerStaticPolicy struct {
+type staticPolicy struct {
 	// Fallback is the fallback policy in case the static one does not
 	// contain a rule for a specific domain.
 	Fallback HTTPSDialerPolicy
 
 	// Root is the root of the statically loaded policy.
-	Root *HTTPSDialerStaticPolicyRoot
+	Root *staticPolicyRoot
 }
 
-// HTTPSDialerStaticPolicyKey is the kvstore key used to retrieve the static policy.
-const HTTPSDialerStaticPolicyKey = "httpsdialerstatic.conf"
+// staticPolicyKey is the kvstore key used to retrieve the static policy.
+const staticPolicyKey = "httpsdialerstatic.conf"
 
-// errDialerStaticPolicyWrongVersion means that the static policy document has the wrong version number.
-var errDialerStaticPolicyWrongVersion = errors.New("wrong static policy version")
+// errStaticPolicyWrongVersion means that the static policy document has the wrong version number.
+var errStaticPolicyWrongVersion = errors.New("wrong static policy version")
 
-// NewHTTPSDialerStaticPolicy attempts to constructs a static policy using a given fallback
+// newStaticPolicy attempts to constructs a static policy using a given fallback
 // policy and either returns a good policy or an error. The typical error case is the one
 // in which there's no httpsDialerStaticPolicyKey in the key-value store.
-func NewHTTPSDialerStaticPolicy(
-	kvStore model.KeyValueStore, fallback HTTPSDialerPolicy) (*HTTPSDialerStaticPolicy, error) {
+func newStaticPolicy(
+	kvStore model.KeyValueStore, fallback HTTPSDialerPolicy) (*staticPolicy, error) {
 	// attempt to read the static policy bytes from the kvstore
-	data, err := kvStore.Get(HTTPSDialerStaticPolicyKey)
+	data, err := kvStore.Get(staticPolicyKey)
 	if err != nil {
 		return nil, err
 	}
 
 	// attempt to parse the static policy using human-readable JSON
-	var root HTTPSDialerStaticPolicyRoot
+	var root staticPolicyRoot
 	if err := hujsonx.Unmarshal(data, &root); err != nil {
 		return nil, err
 	}
 
 	// make sure the version is OK
-	if root.Version != HTTPSDialerStaticPolicyVersion {
+	if root.Version != staticPolicyVersion {
 		err := fmt.Errorf(
 			"%s: %w: expected=%d got=%d",
-			HTTPSDialerStaticPolicyKey,
-			errDialerStaticPolicyWrongVersion,
-			HTTPSDialerStaticPolicyVersion,
+			staticPolicyKey,
+			errStaticPolicyWrongVersion,
+			staticPolicyVersion,
 			root.Version,
 		)
 		return nil, err
 	}
 
-	out := &HTTPSDialerStaticPolicy{
+	out := &staticPolicy{
 		Fallback: fallback,
 		Root:     &root,
 	}
 	return out, nil
 }
 
-// HTTPSDialerStaticPolicyVersion is the current version of the static policy file.
-const HTTPSDialerStaticPolicyVersion = 3
+// staticPolicyVersion is the current version of the static policy file.
+const staticPolicyVersion = 3
 
-// HTTPSDialerStaticPolicyRoot is the root of a statically loaded policy.
-type HTTPSDialerStaticPolicyRoot struct {
+// staticPolicyRoot is the root of a statically loaded policy.
+type staticPolicyRoot struct {
 	// DomainEndpoints maps each domain endpoint to its policies.
 	DomainEndpoints map[string][]*HTTPSDialerTactic
 
 	// Version is the data structure version.
 	Version int
 }
 
-var _ HTTPSDialerPolicy = &HTTPSDialerStaticPolicy{}
+var _ HTTPSDialerPolicy = &staticPolicy{}
 
 // LookupTactics implements HTTPSDialerPolicy.
-func (ldp *HTTPSDialerStaticPolicy) LookupTactics(
+func (ldp *staticPolicy) LookupTactics(
 	ctx context.Context, domain string, port string) <-chan *HTTPSDialerTactic {
 	tactics, found := ldp.Root.DomainEndpoints[net.JoinHostPort(domain, port)]
 	if !found {