Skip to content

Commit

Permalink
refactor(enginenetx): introduce stats and make tactic a struct (#1291)
Browse files Browse the repository at this point in the history 
This diff refactors the code to introduce a stats interface and to make
the tactic a struct ~without methods attached.

We transform the tactic into a struct because we're planning on storing
the tactics on disk and loading them.

We need stats to track what is working and choose which is the best
tactic that we should employ.

In light of these two needs, it makes sense to transfer the reporting
methods that previously were part of the tactic interface to stats.

With this diff, we temporarily lost the possibility of choosing which
handshaker to use, which was not implemented anyway and needs some more
work to reason about how we'd store this information on disk.

Part of ooni/probe#2531
  • Loading branch information
@bassosimone
bassosimone authored Sep 21, 2023
1 parent 19f3787 commit 7110bb9
Show file tree
Hide file tree
Showing 5 changed files with 157 additions and 259 deletions.
14 changes: 6 additions & 8 deletions internal/enginenetx/httpsdialer_internal_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,13 @@ func TestHTTPSDialerTacticsEmitter(t *testing.T) {
wg: &sync.WaitGroup{},
}

var tactics []HTTPSDialerTactic
var tactics []*HTTPSDialerTactic
for idx := 0; idx < 255; idx++ {
tactics = append(tactics, &HTTPSDialerLoadableTacticWrapper{
Tactic: &HTTPSDialerLoadableTactic{
IPAddr: fmt.Sprintf("10.0.0.%d", idx),
InitialDelay: 0,
SNI: "www.example.com",
VerifyHostname: "www.example.com",
},
tactics = append(tactics, &HTTPSDialerTactic{
IPAddr: fmt.Sprintf("10.0.0.%d", idx),
InitialDelay: 0,
SNI: "www.example.com",
VerifyHostname: "www.example.com",
})
}

Expand Down
166 changes: 56 additions & 110 deletions internal/enginenetx/httpsdialer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,72 +10,56 @@ import (
"github.com/google/go-cmp/cmp"
"github.com/ooni/netem"
"github.com/ooni/probe-cli/v3/internal/enginenetx"
"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/testingx"
)

// Flags controlling when [httpsDialerPolicyCancelingContext] cancels the context
// Flags controlling when [httpsDialerCancelingContextStatsTracker] cancels the context
const (
httpsDialerPolicyCancelingContextOnStarting = 1 << iota
httpsDialerPolicyCancelingContextOnSuccess
httpsDialerCancelingContextStatsTrackerOnStarting = 1 << iota
httpsDialerCancelingContextStatsTrackerOnSuccess
)

// httpsDialerPolicyCancelingContext is an [enginenetsx.HTTPSDialerPolicy] with a cancel
// httpsDialerCancelingContextStatsTracker is an [enginenetx.HTTPSDialerStatsTracker] with a cancel
// function that causes the context to be canceled once we start dialing.
//
// This struct helps with testing [enginenetx.HTTPSDialer] is WAI when the context
// has been canceled and we correctly shutdown all goroutines.
type httpsDialerPolicyCancelingContext struct {
type httpsDialerCancelingContextStatsTracker struct {
cancel context.CancelFunc
flags int
policy enginenetx.HTTPSDialerPolicy
}

var _ enginenetx.HTTPSDialerPolicy = &httpsDialerPolicyCancelingContext{}
var _ enginenetx.HTTPSDialerStatsTracker = &httpsDialerCancelingContextStatsTracker{}

// LookupTactics implements enginenetx.HTTPSDialerPolicy.
func (p *httpsDialerPolicyCancelingContext) LookupTactics(ctx context.Context, domain string, reso model.Resolver) ([]enginenetx.HTTPSDialerTactic, error) {
tactics, err := p.policy.LookupTactics(ctx, domain, reso)
if err != nil {
return nil, err
// OnStarting implements enginenetx.HTTPSDialerStatsTracker.
func (st *httpsDialerCancelingContextStatsTracker) OnStarting(tactic *enginenetx.HTTPSDialerTactic) {
if (st.flags & httpsDialerCancelingContextStatsTrackerOnStarting) != 0 {
st.cancel()
}
var out []enginenetx.HTTPSDialerTactic
for _, tactic := range tactics {
out = append(out, &httpsDialerTacticCancelingContext{
HTTPSDialerTactic: tactic,
cancel: p.cancel,
flags: p.flags,
})
}
return out, nil
}

// Parallelism implements enginenetx.HTTPSDialerPolicy.
func (p *httpsDialerPolicyCancelingContext) Parallelism() int {
return p.policy.Parallelism()
// OnTCPConnectError implements enginenetx.HTTPSDialerStatsTracker.
func (*httpsDialerCancelingContextStatsTracker) OnTCPConnectError(ctx context.Context, tactic *enginenetx.HTTPSDialerTactic, err error) {
// nothing
}

// httpsDialerTacticCancelingContext is the tactic returned by [httpsDialerPolicyCancelingContext].
type httpsDialerTacticCancelingContext struct {
enginenetx.HTTPSDialerTactic
cancel context.CancelFunc
flags int
// OnTLSHandshakeError implements enginenetx.HTTPSDialerStatsTracker.
func (*httpsDialerCancelingContextStatsTracker) OnTLSHandshakeError(ctx context.Context, tactic *enginenetx.HTTPSDialerTactic, err error) {
// nothing
}

// OnStarting implements enginenetx.HTTPSDialerTactic.
func (t *httpsDialerTacticCancelingContext) OnStarting() {
if (t.flags & httpsDialerPolicyCancelingContextOnStarting) != 0 {
t.cancel()
}
// OnTLSVerifyError implements enginenetx.HTTPSDialerStatsTracker.
func (*httpsDialerCancelingContextStatsTracker) OnTLSVerifyError(ctz context.Context, tactic *enginenetx.HTTPSDialerTactic, err error) {
// nothing
}

// OnSuccess implements enginenetx.HTTPSDialerTactic.
func (t *httpsDialerTacticCancelingContext) OnSuccess() {
if (t.flags & httpsDialerPolicyCancelingContextOnSuccess) != 0 {
t.cancel()
// OnSuccess implements enginenetx.HTTPSDialerStatsTracker.
func (st *httpsDialerCancelingContextStatsTracker) OnSuccess(tactic *enginenetx.HTTPSDialerTactic) {
if (st.flags & httpsDialerCancelingContextStatsTrackerOnSuccess) != 0 {
st.cancel()
}
}

Expand All @@ -91,6 +75,9 @@ func TestHTTPSDialerWAI(t *testing.T) {
// policy is the dialer policy
policy enginenetx.HTTPSDialerPolicy

// stats is the stats tracker to use.
stats enginenetx.HTTPSDialerStatsTracker

// endpoint is the endpoint to connect to consisting of a domain
// name or IP address followed by a TCP port
endpoint string
Expand All @@ -113,6 +100,7 @@ func TestHTTPSDialerWAI(t *testing.T) {
name: "net.SplitHostPort failure",
short: true,
policy: &enginenetx.HTTPSDialerNullPolicy{},
stats: &enginenetx.HTTPSDialerNullStatsTracker{},
endpoint: "www.example.com", // note: here the port is missing
scenario: netemx.InternetScenario,
configureDPI: func(dpi *netem.DPIEngine) {
Expand All @@ -126,6 +114,7 @@ func TestHTTPSDialerWAI(t *testing.T) {
name: "hd.policy.LookupTactics failure",
short: true,
policy: &enginenetx.HTTPSDialerNullPolicy{},
stats: &enginenetx.HTTPSDialerNullStatsTracker{},
endpoint: "www.example.nonexistent:443", // note: the domain does not exist
scenario: netemx.InternetScenario,
configureDPI: func(dpi *netem.DPIEngine) {
Expand All @@ -140,6 +129,7 @@ func TestHTTPSDialerWAI(t *testing.T) {
name: "successful dial with multiple addresses",
short: true,
policy: &enginenetx.HTTPSDialerNullPolicy{},
stats: &enginenetx.HTTPSDialerNullStatsTracker{},
endpoint: "www.example.com:443",
scenario: []*netemx.ScenarioDomainAddresses{{
Domains: []string{
Expand All @@ -166,6 +156,7 @@ func TestHTTPSDialerWAI(t *testing.T) {
name: "with TCP connect errors",
short: true,
policy: &enginenetx.HTTPSDialerNullPolicy{},
stats: &enginenetx.HTTPSDialerNullStatsTracker{},
endpoint: "www.example.com:443",
scenario: []*netemx.ScenarioDomainAddresses{{
Domains: []string{
Expand Down Expand Up @@ -200,6 +191,7 @@ func TestHTTPSDialerWAI(t *testing.T) {
name: "with TLS handshake errors",
short: true,
policy: &enginenetx.HTTPSDialerNullPolicy{},
stats: &enginenetx.HTTPSDialerNullStatsTracker{},
endpoint: "www.example.com:443",
scenario: []*netemx.ScenarioDomainAddresses{{
Domains: []string{
Expand Down Expand Up @@ -230,6 +222,7 @@ func TestHTTPSDialerWAI(t *testing.T) {
name: "with a TLS certificate valid for ANOTHER domain",
short: true,
policy: &enginenetx.HTTPSDialerNullPolicy{},
stats: &enginenetx.HTTPSDialerNullStatsTracker{},
endpoint: "wrong.host.badssl.com:443",
scenario: []*netemx.ScenarioDomainAddresses{{
Domains: []string{
Expand All @@ -255,6 +248,7 @@ func TestHTTPSDialerWAI(t *testing.T) {
name: "with TLS certificate signed by an unknown authority",
short: true,
policy: &enginenetx.HTTPSDialerNullPolicy{},
stats: &enginenetx.HTTPSDialerNullStatsTracker{},
endpoint: "untrusted-root.badssl.com:443",
scenario: []*netemx.ScenarioDomainAddresses{{
Domains: []string{
Expand All @@ -280,6 +274,7 @@ func TestHTTPSDialerWAI(t *testing.T) {
name: "with expired TLS certificate",
short: true,
policy: &enginenetx.HTTPSDialerNullPolicy{},
stats: &enginenetx.HTTPSDialerNullStatsTracker{},
endpoint: "expired.badssl.com:443",
scenario: []*netemx.ScenarioDomainAddresses{{
Domains: []string{
Expand All @@ -302,12 +297,12 @@ func TestHTTPSDialerWAI(t *testing.T) {
// This is a corner case: what if the context is canceled after the DNS lookup
// but before we start dialing? Are we closing all goroutines and returning correctly?
{
name: "with context being canceled in OnStarting",
short: true,
policy: &httpsDialerPolicyCancelingContext{
name: "with context being canceled in OnStarting",
short: true,
policy: &enginenetx.HTTPSDialerNullPolicy{},
stats: &httpsDialerCancelingContextStatsTracker{
cancel: nil,
flags: httpsDialerPolicyCancelingContextOnStarting,
policy: &enginenetx.HTTPSDialerNullPolicy{},
flags: httpsDialerCancelingContextStatsTrackerOnStarting,
},
endpoint: "www.example.com:443",
scenario: []*netemx.ScenarioDomainAddresses{{
Expand All @@ -331,12 +326,12 @@ func TestHTTPSDialerWAI(t *testing.T) {
// This is another corner case: what happens if the context is canceled after we
// have a good connection but before we're able to report it to the caller?
{
name: "with context being canceled in OnSuccess for the first success",
short: true,
policy: &httpsDialerPolicyCancelingContext{
name: "with context being canceled in OnSuccess for the first success",
short: true,
policy: &enginenetx.HTTPSDialerNullPolicy{},
stats: &httpsDialerCancelingContextStatsTracker{
cancel: nil,
flags: httpsDialerPolicyCancelingContextOnSuccess,
policy: &enginenetx.HTTPSDialerNullPolicy{},
flags: httpsDialerCancelingContextStatsTrackerOnSuccess,
},
endpoint: "www.example.com:443",
scenario: []*netemx.ScenarioDomainAddresses{{
Expand Down Expand Up @@ -390,6 +385,7 @@ func TestHTTPSDialerWAI(t *testing.T) {
log.Log,
tc.policy,
resolver,
tc.stats,
unet,
)
defer dialer.CloseIdleConnections()
Expand All @@ -398,9 +394,9 @@ func TestHTTPSDialerWAI(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

// Possibly tell the httpsDialerPolicyCancelingContext about the cancel func
// Possibly tell the httpsDialerCancelingContextStatsTracker about the cancel func
// depending on which flags have been configured.
if p, ok := tc.policy.(*httpsDialerPolicyCancelingContext); ok {
if p, ok := tc.stats.(*httpsDialerCancelingContextStatsTracker); ok {
p.cancel = cancel
}

Expand Down Expand Up @@ -478,7 +474,7 @@ func TestLoadHTTPSDialerPolicy(t *testing.T) {
name: "with real serialized policy",
input: (func() []byte {
return runtimex.Try1(json.Marshal(&enginenetx.HTTPSDialerLoadablePolicy{
Domains: map[string][]*enginenetx.HTTPSDialerLoadableTactic{
Domains: map[string][]*enginenetx.HTTPSDialerTactic{
"api.ooni.io": {{
IPAddr: "162.55.247.208",
InitialDelay: 0,
Expand Down Expand Up @@ -510,7 +506,7 @@ func TestLoadHTTPSDialerPolicy(t *testing.T) {
})(),
expectErr: "",
expectedPolicy: &enginenetx.HTTPSDialerLoadablePolicy{
Domains: map[string][]*enginenetx.HTTPSDialerLoadableTactic{
Domains: map[string][]*enginenetx.HTTPSDialerTactic{
"api.ooni.io": {{
IPAddr: "162.55.247.208",
InitialDelay: 0,
Expand Down Expand Up @@ -568,68 +564,18 @@ func TestLoadHTTPSDialerPolicy(t *testing.T) {
}
}

func TestHTTPSDialerLoadableTacticWrapper(t *testing.T) {
t.Run("IPAddr", func(t *testing.T) {
expected := "10.0.0.1"
ldt := &enginenetx.HTTPSDialerLoadableTacticWrapper{
Tactic: &enginenetx.HTTPSDialerLoadableTactic{
IPAddr: expected,
},
}
if got := ldt.IPAddr(); got != expected {
t.Fatal("expected", expected, "got", got)
}
})

t.Run("InitialDelay", func(t *testing.T) {
expected := time.Millisecond
ldt := &enginenetx.HTTPSDialerLoadableTacticWrapper{
Tactic: &enginenetx.HTTPSDialerLoadableTactic{
InitialDelay: expected,
},
}
if got := ldt.InitialDelay(); got != expected {
t.Fatal("expected", expected, "got", got)
}
})

t.Run("SNI", func(t *testing.T) {
expected := "x.org"
ldt := &enginenetx.HTTPSDialerLoadableTacticWrapper{
Tactic: &enginenetx.HTTPSDialerLoadableTactic{
SNI: expected,
},
}
if got := ldt.SNI(); got != expected {
t.Fatal("expected", expected, "got", got)
}
})

func TestHTTPSDialerTactic(t *testing.T) {
t.Run("String", func(t *testing.T) {
expected := "&{IPAddr:162.55.247.208 InitialDelay:150ms SNI:www.example.com VerifyHostname:api.ooni.io}"
ldt := &enginenetx.HTTPSDialerLoadableTacticWrapper{
Tactic: &enginenetx.HTTPSDialerLoadableTactic{
IPAddr: "162.55.247.208",
InitialDelay: 150 * time.Millisecond,
SNI: "www.example.com",
VerifyHostname: "api.ooni.io",
},
expected := `{"IPAddr":"162.55.247.208","InitialDelay":150000000,"SNI":"www.example.com","VerifyHostname":"api.ooni.io"}`
ldt := &enginenetx.HTTPSDialerTactic{
IPAddr: "162.55.247.208",
InitialDelay: 150 * time.Millisecond,
SNI: "www.example.com",
VerifyHostname: "api.ooni.io",
}
got := ldt.String()
if diff := cmp.Diff(expected, got); diff != "" {
t.Fatal(diff)
}
})

t.Run("VerifyHostname", func(t *testing.T) {
expected := "x.org"
ldt := &enginenetx.HTTPSDialerLoadableTacticWrapper{
Tactic: &enginenetx.HTTPSDialerLoadableTactic{
VerifyHostname: expected,
},
}
if got := ldt.VerifyHostname(); got != expected {
t.Fatal("expected", expected, "got", got)
}
})
}
Loading

0 comments on commit 7110bb9

Please sign in to comment.