Skip to content

Commit

Permalink
refactor(enginenetx): store address and port separately (#1306)
Browse files Browse the repository at this point in the history 
This diff modifies the `HTTPSDialerTactic` struct to store the address
and port separately rather than storing the endpoint as the
concatenation of the address and the port.

By storing these two fields separately, we're more flexible as we will
soon see with subsequent diffs.

Reference issue: ooni/probe#2531
  • Loading branch information
@bassosimone
bassosimone authored Sep 26, 2023
1 parent 98685c4 commit 9282659
Show file tree
Hide file tree
Showing 11 changed files with 92 additions and 63 deletions.
4 changes: 2 additions & 2 deletions internal/enginenetx/beacons.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package enginenetx
import (
"context"
"math/rand"
"net"
"time"
)

Expand Down Expand Up @@ -69,8 +68,9 @@ func (p *beaconsPolicy) tacticsForDomain(domain, port string) <-chan *HTTPSDiale
for _, ipAddr := range ipAddrs {
for _, sni := range snis {
out <- &HTTPSDialerTactic{
Endpoint: net.JoinHostPort(ipAddr, port),
Address: ipAddr,
InitialDelay: 0,
Port: port,
SNI: sni,
VerifyHostname: domain,
}
Expand Down
17 changes: 4 additions & 13 deletions internal/enginenetx/beacons_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package enginenetx
import (
"context"
"errors"
"net"
"testing"

"github.com/ooni/probe-cli/v3/internal/mocks"
Expand Down Expand Up @@ -56,14 +55,10 @@ func TestBeaconsPolicy(t *testing.T) {
for tactic := range tactics {
count++

host, port, err := net.SplitHostPort(tactic.Endpoint)
if err != nil {
t.Fatal(err)
}
if port != "443" {
if tactic.Port != "443" {
t.Fatal("the port should always be 443")
}
if host != "93.184.216.34" {
if tactic.Address != "93.184.216.34" {
t.Fatal("the host should always be 93.184.216.34")
}

Expand Down Expand Up @@ -101,14 +96,10 @@ func TestBeaconsPolicy(t *testing.T) {
for tactic := range tactics {
count++

host, port, err := net.SplitHostPort(tactic.Endpoint)
if err != nil {
t.Fatal(err)
}
if port != "443" {
if tactic.Port != "443" {
t.Fatal("the port should always be 443")
}
if host != "162.55.247.208" {
if tactic.Address != "162.55.247.208" {
t.Fatal("the host should always be 162.55.247.208")
}

Expand Down
8 changes: 5 additions & 3 deletions internal/enginenetx/httpsdialer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -435,10 +435,11 @@ func TestHTTPSDialerNetemQA(t *testing.T) {

func TestHTTPSDialerTactic(t *testing.T) {
t.Run("String", func(t *testing.T) {
expected := `{"Endpoint":"162.55.247.208:443","InitialDelay":150000000,"SNI":"www.example.com","VerifyHostname":"api.ooni.io"}`
expected := `{"Address":"162.55.247.208","InitialDelay":150000000,"Port":"443","SNI":"www.example.com","VerifyHostname":"api.ooni.io"}`
ldt := &enginenetx.HTTPSDialerTactic{
Endpoint: "162.55.247.208:443",
Address: "162.55.247.208",
InitialDelay: 150 * time.Millisecond,
Port: "443",
SNI: "www.example.com",
VerifyHostname: "api.ooni.io",
}
Expand All @@ -461,8 +462,9 @@ func TestHTTPSDialerTactic(t *testing.T) {
t.Run("Summary", func(t *testing.T) {
expected := `162.55.247.208:443 sni=www.example.com verify=api.ooni.io`
ldt := &enginenetx.HTTPSDialerTactic{
Endpoint: "162.55.247.208:443",
Address: "162.55.247.208",
InitialDelay: 150 * time.Millisecond,
Port: "443",
SNI: "www.example.com",
VerifyHostname: "api.ooni.io",
}
Expand Down
19 changes: 12 additions & 7 deletions internal/enginenetx/httpsdialercore.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,16 @@ import (

// HTTPSDialerTactic is a tactic to establish a TLS connection.
type HTTPSDialerTactic struct {
// Endpoint is the TCP endpoint to use for dialing.
Endpoint string
// Address is the IPv4/IPv6 address for dialing.
Address string

// InitialDelay is the time in nanoseconds after which
// you would like to start this policy.
InitialDelay time.Duration

// Port is the TCP port for dialing.
Port string

// SNI is the TLS ServerName to send over the wire.
SNI string

Expand All @@ -39,8 +42,9 @@ var _ fmt.Stringer = &HTTPSDialerTactic{}
// Clone makes a deep copy of this [HTTPSDialerTactic].
func (dt *HTTPSDialerTactic) Clone() *HTTPSDialerTactic {
return &HTTPSDialerTactic{
Endpoint: dt.Endpoint,
Address: dt.Address,
InitialDelay: dt.InitialDelay,
Port: dt.Port,
SNI: dt.SNI,
VerifyHostname: dt.VerifyHostname,
}
Expand All @@ -64,7 +68,7 @@ func (dt *HTTPSDialerTactic) String() string {
//
// The returned string contains the above fields separated by space.
func (dt *HTTPSDialerTactic) Summary() string {
return fmt.Sprintf("%v sni=%v verify=%v", dt.Endpoint, dt.SNI, dt.VerifyHostname)
return fmt.Sprintf("%v sni=%v verify=%v", net.JoinHostPort(dt.Address, dt.Port), dt.SNI, dt.VerifyHostname)
}

// HTTPSDialerPolicy describes the policy used by the [*HTTPSDialer].
Expand Down Expand Up @@ -281,9 +285,10 @@ func (hd *HTTPSDialer) dialTLS(
hd.stats.OnStarting(tactic)

// create dialer and establish TCP connection
ol := logx.NewOperationLogger(logger, "TCPConnect %s", tactic.Endpoint)
endpoint := net.JoinHostPort(tactic.Address, tactic.Port)
ol := logx.NewOperationLogger(logger, "TCPConnect %s", endpoint)
dialer := hd.netx.NewDialerWithoutResolver(logger)
tcpConn, err := dialer.DialContext(ctx, "tcp", tactic.Endpoint)
tcpConn, err := dialer.DialContext(ctx, "tcp", endpoint)
ol.Stop(err)

// handle a dialing error
Expand All @@ -304,7 +309,7 @@ func (hd *HTTPSDialer) dialTLS(
ol = logx.NewOperationLogger(
logger,
"TLSHandshake with %s SNI=%s ALPN=%v",
tactic.Endpoint,
endpoint,
tlsConfig.ServerName,
tlsConfig.NextProtos,
)
Expand Down
4 changes: 2 additions & 2 deletions internal/enginenetx/httpsdialernull.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package enginenetx

import (
"context"
"net"

"github.com/ooni/probe-cli/v3/internal/model"
"github.com/ooni/probe-cli/v3/internal/netxlite"
Expand Down Expand Up @@ -56,8 +55,9 @@ func (p *HTTPSDialerNullPolicy) LookupTactics(

for idx, addr := range addrs {
tactic := &HTTPSDialerTactic{
Endpoint: net.JoinHostPort(addr, port),
Address: addr,
InitialDelay: happyEyeballsDelay(idx),
Port: port,
SNI: domain,
VerifyHostname: domain,
}
Expand Down
7 changes: 5 additions & 2 deletions internal/enginenetx/httpsdialernull_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,11 @@ func TestHTTPSDialerNullPolicy(t *testing.T) {
for tactic := range tactics {
count++

if tactic.Endpoint != "130.192.91.211:443" {
t.Fatal("invalid endpoint")
if tactic.Address != "130.192.91.211" {
t.Fatal("invalid endpoint address")
}
if tactic.Port != "443" {
t.Fatal("invalid endpoint port")
}
if tactic.SNI != "130.192.91.211" {
t.Fatal("invalid SNI")
Expand Down
2 changes: 1 addition & 1 deletion internal/enginenetx/httpsdialerstatic.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func NewHTTPSDialerStaticPolicy(
}

// HTTPSDialerStaticPolicyVersion is the current version of the static policy file.
const HTTPSDialerStaticPolicyVersion = 1
const HTTPSDialerStaticPolicyVersion = 2

// HTTPSDialerStaticPolicyRoot is the root of a statically loaded policy.
type HTTPSDialerStaticPolicyRoot struct {
Expand Down
38 changes: 25 additions & 13 deletions internal/enginenetx/httpsdialerstatic_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func TestHTTPSDialerStaticPolicy(t *testing.T) {
name: "with empty JSON",
key: HTTPSDialerStaticPolicyKey,
input: []byte(`{}`),
expectErr: "httpsdialerstatic.conf: wrong static policy version: expected=1 got=0",
expectErr: "httpsdialerstatic.conf: wrong static policy version: expected=2 got=0",
expectedPolicy: nil,
}, {
name: "with real serialized policy",
Expand All @@ -66,28 +66,33 @@ func TestHTTPSDialerStaticPolicy(t *testing.T) {
return runtimex.Try1(json.Marshal(&HTTPSDialerStaticPolicyRoot{
Domains: map[string][]*HTTPSDialerTactic{
"api.ooni.io": {{
Endpoint: "162.55.247.208:443",
Address: "162.55.247.208",
InitialDelay: 0,
Port: "443",
SNI: "api.ooni.io",
VerifyHostname: "api.ooni.io",
}, {
Endpoint: "46.101.82.151:443",
Address: "46.101.82.151",
InitialDelay: 300 * time.Millisecond,
Port: "443",
SNI: "api.ooni.io",
VerifyHostname: "api.ooni.io",
}, {
Endpoint: "[2a03:b0c0:1:d0::ec4:9001]:443",
Address: "2a03:b0c0:1:d0::ec4:9001",
InitialDelay: 600 * time.Millisecond,
Port: "443",
SNI: "api.ooni.io",
VerifyHostname: "api.ooni.io",
}, {
Endpoint: "46.101.82.151:443",
Address: "46.101.82.151",
InitialDelay: 3000 * time.Millisecond,
Port: "443",
SNI: "www.example.com",
VerifyHostname: "api.ooni.io",
}, {
Endpoint: "[2a03:b0c0:1:d0::ec4:9001]:443",
Address: "2a03:b0c0:1:d0::ec4:9001",
InitialDelay: 3300 * time.Millisecond,
Port: "443",
SNI: "www.example.com",
VerifyHostname: "api.ooni.io",
}},
Expand All @@ -101,28 +106,33 @@ func TestHTTPSDialerStaticPolicy(t *testing.T) {
Root: &HTTPSDialerStaticPolicyRoot{
Domains: map[string][]*HTTPSDialerTactic{
"api.ooni.io": {{
Endpoint: "162.55.247.208:443",
Address: "162.55.247.208",
InitialDelay: 0,
Port: "443",
SNI: "api.ooni.io",
VerifyHostname: "api.ooni.io",
}, {
Endpoint: "46.101.82.151:443",
Address: "46.101.82.151",
InitialDelay: 300 * time.Millisecond,
Port: "443",
SNI: "api.ooni.io",
VerifyHostname: "api.ooni.io",
}, {
Endpoint: "[2a03:b0c0:1:d0::ec4:9001]:443",
Address: "2a03:b0c0:1:d0::ec4:9001",
InitialDelay: 600 * time.Millisecond,
Port: "443",
SNI: "api.ooni.io",
VerifyHostname: "api.ooni.io",
}, {
Endpoint: "46.101.82.151:443",
Address: "46.101.82.151",
InitialDelay: 3000 * time.Millisecond,
Port: "443",
SNI: "www.example.com",
VerifyHostname: "api.ooni.io",
}, {
Endpoint: "[2a03:b0c0:1:d0::ec4:9001]:443",
Address: "2a03:b0c0:1:d0::ec4:9001",
InitialDelay: 3300 * time.Millisecond,
Port: "443",
SNI: "www.example.com",
VerifyHostname: "api.ooni.io",
}},
Expand Down Expand Up @@ -164,8 +174,9 @@ func TestHTTPSDialerStaticPolicy(t *testing.T) {

t.Run("LookupTactics", func(t *testing.T) {
expectedTactic := &HTTPSDialerTactic{
Endpoint: "162.55.247.208:443",
Address: "162.55.247.208",
InitialDelay: 0,
Port: "443",
SNI: "www.example.com",
VerifyHostname: "api.ooni.io",
}
Expand Down Expand Up @@ -228,8 +239,9 @@ func TestHTTPSDialerStaticPolicy(t *testing.T) {
}

expect := []*HTTPSDialerTactic{{
Endpoint: "93.184.216.34:443",
Address: "93.184.216.34",
InitialDelay: 0,
Port: "443",
SNI: "www.example.com",
VerifyHostname: "www.example.com",
}}
Expand Down
3 changes: 2 additions & 1 deletion internal/enginenetx/network_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -303,8 +303,9 @@ func TestNetworkQA(t *testing.T) {
policy := &enginenetx.HTTPSDialerStaticPolicyRoot{
Domains: map[string][]*enginenetx.HTTPSDialerTactic{
"www.example.com": {{
Endpoint: net.JoinHostPort(netemx.AddressApiOONIIo, "443"),
Address: netemx.AddressApiOONIIo,
InitialDelay: 0,
Port: "443",
SNI: "www.example.com",
VerifyHostname: "api.ooni.io",
}},
Expand Down
2 changes: 1 addition & 1 deletion internal/enginenetx/stats.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ func statsDomainRemoveOldEntries(input *statsDomain) (output *statsDomain) {
}

// statsContainerVersion is the current version of [statsContainer].
const statsContainerVersion = 2
const statsContainerVersion = 4

// statsContainer is the root container for the stats.
//
Expand Down
Loading

0 comments on commit 9282659

Please sign in to comment.