Skip to content

Commit

Permalink
feat(webconnectivitylte): add more tests and comments (#1458)
Browse files Browse the repository at this point in the history 
This diff ensures that we do not lose any comment or test that we
removed in #1455 and rather aims
to ensure we have equivalent tests and comments.

Part of ooni/probe#2652.
  • Loading branch information
@bassosimone
bassosimone authored Jan 22, 2024
1 parent fbe3515 commit 0a5604a
Show file tree
Hide file tree
Showing 11 changed files with 211 additions and 47 deletions.
9 changes: 6 additions & 3 deletions internal/cmd/minipipeline/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ import (
"os"
"path/filepath"

"github.com/ooni/probe-cli/v3/internal/geoipx"
"github.com/ooni/probe-cli/v3/internal/minipipeline"
"github.com/ooni/probe-cli/v3/internal/model"
"github.com/ooni/probe-cli/v3/internal/must"
"github.com/ooni/probe-cli/v3/internal/runtimex"
)
Expand Down Expand Up @@ -56,8 +58,9 @@ func main() {
must.UnmarshalJSON(must.ReadFile(*measurementFlag), &parsed)

// generate and write observations
lookupper := model.GeoIPASNLookupperFunc(geoipx.LookupASN)
observationsPath := filepath.Join(*destdirFlag, *prefixFlag+"observations.json")
container := runtimex.Try1(minipipeline.IngestWebMeasurement(&parsed))
container := runtimex.Try1(minipipeline.IngestWebMeasurement(lookupper, &parsed))
mustWriteFileFn(observationsPath, must.MarshalAndIndentJSON(container, "", " "), 0600)

// generate and write classic observations
Expand All @@ -67,11 +70,11 @@ func main() {

// generate and write observations analysis
analysisPath := filepath.Join(*destdirFlag, *prefixFlag+"analysis.json")
analysis := minipipeline.AnalyzeWebObservationsWithLinearAnalysis(container)
analysis := minipipeline.AnalyzeWebObservationsWithLinearAnalysis(lookupper, container)
mustWriteFileFn(analysisPath, must.MarshalAndIndentJSON(analysis, "", " "), 0600)

// generate and write the classic analysis
classicAnalysisPath := filepath.Join(*destdirFlag, *prefixFlag+"analysis_classic.json")
analysisClassic := minipipeline.AnalyzeWebObservationsWithLinearAnalysis(containerClassic)
analysisClassic := minipipeline.AnalyzeWebObservationsWithLinearAnalysis(lookupper, containerClassic)
mustWriteFileFn(classicAnalysisPath, must.MarshalAndIndentJSON(analysisClassic, "", " "), 0600)
}
11 changes: 8 additions & 3 deletions internal/cmd/qatool/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ import (

"github.com/ooni/probe-cli/v3/internal/experiment/webconnectivitylte"
"github.com/ooni/probe-cli/v3/internal/experiment/webconnectivityqa"
"github.com/ooni/probe-cli/v3/internal/geoipx"
"github.com/ooni/probe-cli/v3/internal/minipipeline"
"github.com/ooni/probe-cli/v3/internal/model"
"github.com/ooni/probe-cli/v3/internal/must"
"github.com/ooni/probe-cli/v3/internal/runtimex"
)
Expand Down Expand Up @@ -73,8 +75,11 @@ func runWebConnectivityLTE(tc *webconnectivityqa.TestCase) {
var webMeasurement minipipeline.WebMeasurement
must.UnmarshalJSON(rawData, &webMeasurement)

// create the GeoIP ASN lookupper we're going to use
lookupper := model.GeoIPASNLookupperFunc(geoipx.LookupASN)

// ingest web measurement
observationsContainer := runtimex.Try1(minipipeline.IngestWebMeasurement(&webMeasurement))
observationsContainer := runtimex.Try1(minipipeline.IngestWebMeasurement(lookupper, &webMeasurement))

// serialize the observations
mustSerializeMkdirAllAndWriteFile(actualDestdir, "observations.json", observationsContainer)
Expand All @@ -86,13 +91,13 @@ func runWebConnectivityLTE(tc *webconnectivityqa.TestCase) {
mustSerializeMkdirAllAndWriteFile(actualDestdir, "observations_classic.json", observationsContainerClassic)

// analyze the observations
analysis := minipipeline.AnalyzeWebObservationsWithLinearAnalysis(observationsContainer)
analysis := minipipeline.AnalyzeWebObservationsWithLinearAnalysis(lookupper, observationsContainer)

// serialize the observations analysis
mustSerializeMkdirAllAndWriteFile(actualDestdir, "analysis.json", analysis)

// perform the classic analysis
analysisClassic := minipipeline.AnalyzeWebObservationsWithLinearAnalysis(observationsContainerClassic)
analysisClassic := minipipeline.AnalyzeWebObservationsWithLinearAnalysis(lookupper, observationsContainerClassic)

// serialize the classic analysis results
mustSerializeMkdirAllAndWriteFile(actualDestdir, "analysis_classic.json", analysisClassic)
Expand Down
13 changes: 7 additions & 6 deletions internal/experiment/webconnectivitylte/analysisclassic.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ package webconnectivitylte
//

import (
"github.com/ooni/probe-cli/v3/internal/geoipx"
"github.com/ooni/probe-cli/v3/internal/minipipeline"
"github.com/ooni/probe-cli/v3/internal/model"
"github.com/ooni/probe-cli/v3/internal/optional"
Expand All @@ -18,17 +19,17 @@ import (
// results that are backward compatible with Web Connectivity v0.4 while also
// procuding more fine-grained blocking flags.
func analysisEngineClassic(tk *TestKeys, logger model.Logger) {
tk.analysisClassic(logger)
tk.analysisClassic(model.GeoIPASNLookupperFunc(geoipx.LookupASN), logger)
}

func (tk *TestKeys) analysisClassic(logger model.Logger) {
func (tk *TestKeys) analysisClassic(lookupper model.GeoIPASNLookupper, logger model.Logger) {
// Since we run after all tasks have completed (or so we assume) we're
// not going to use any form of locking here.

// 1. produce web observations
container := minipipeline.NewWebObservationsContainer()
container.IngestDNSLookupEvents(tk.Queries...)
container.IngestTCPConnectEvents(tk.TCPConnect...)
container.IngestDNSLookupEvents(lookupper, tk.Queries...)
container.IngestTCPConnectEvents(lookupper, tk.TCPConnect...)
container.IngestTLSHandshakeEvents(tk.TLSHandshakes...)
container.IngestHTTPRoundTripEvents(tk.Requests...)

Expand All @@ -40,14 +41,14 @@ func (tk *TestKeys) analysisClassic(logger model.Logger) {
}

// 2. compute extended analysis flags
analysisExtMain(tk, container)
analysisExtMain(lookupper, tk, container)

// 3. filter observations to only include results collected by the
// system resolver, which approximates v0.4's results
classic := minipipeline.ClassicFilter(container)

// 3. produce a web observations analysis based on the web observations
woa := minipipeline.AnalyzeWebObservationsWithLinearAnalysis(classic)
woa := minipipeline.AnalyzeWebObservationsWithLinearAnalysis(lookupper, classic)

// 5. determine the DNS consistency
tk.DNSConsistency = analysisClassicDNSConsistency(woa)
Expand Down
87 changes: 87 additions & 0 deletions internal/experiment/webconnectivitylte/analysisclassic_test.go
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
package webconnectivitylte

import (
"encoding/json"
"os"
"path/filepath"
"testing"

"github.com/apex/log"
"github.com/ooni/probe-cli/v3/internal/mocks"
"github.com/ooni/probe-cli/v3/internal/model"
"github.com/ooni/probe-cli/v3/internal/runtimex"
"github.com/tailscale/hujson"
)

// This test is related to https://github.com/ooni/probe/issues/2517.
//
// It shows how blocking flags could vary depending on the ASN distribution based on
// a real case where we observed false positives caused by that.
func TestTestKeys_analysisDNSToplevel(t *testing.T) {

// testcase is a test case in this test
type testcase struct {
// name is the name of the test case
name string

// tkFile is the name of the JSONC file containing the test keys
tkFile string

// geoInfo contains a static mapping of geoip info
geoInfo map[string]*model.LocationASN

// expectBlockingFlags contains the expected BlockingFlags
expecteBlockingFlags int64
}

testcases := []testcase{{
name: "https://github.com/ooni/probe/issues/2517",
tkFile: filepath.Join("testdata", "20230706183840.201925_PK_webconnectivity_19f5e0d803cbaea7.jsonc"),
geoInfo: map[string]*model.LocationASN{
"172.224.19.10": {ASNumber: 36183, Organization: "Akamai Technologies, Inc."},
"172.224.19.5": {ASNumber: 36183, Organization: "Akamai Technologies, Inc."},
"172.224.19.9": {ASNumber: 36183, Organization: "Akamai Technologies, Inc."},
"17.248.248.101": {ASNumber: 714, Organization: "Apple Inc."},
"2a01:b740:a41:212::8": {ASNumber: 714, Organization: "Apple Inc."},
"2a01:b740:a41:212::7": {ASNumber: 714, Organization: "Apple Inc."},
"2a01:b740:a41:213::7": {ASNumber: 714, Organization: "Apple Inc."},
"172.224.19.3": {ASNumber: 36183, Organization: "Akamai Technologies, Inc."},
"172.224.19.12": {ASNumber: 36183, Organization: "Akamai Technologies, Inc."},
"17.248.248.103": {ASNumber: 714, Organization: "Apple Inc."},
"17.248.248.119": {ASNumber: 714, Organization: "Apple Inc."},
"2a01:b740:a41:213::5": {ASNumber: 714, Organization: "Apple Inc."},
"172.224.19.4": {ASNumber: 36183, Organization: "Akamai Technologies, Inc."},
"172.224.19.6": {ASNumber: 36183, Organization: "Akamai Technologies, Inc."},
"172.224.19.11": {ASNumber: 36183, Organization: "Akamai Technologies, Inc."},
"2a01:b740:a41:212::4": {ASNumber: 714, Organization: "Apple Inc."},
"172.224.19.7": {ASNumber: 36183, Organization: "Akamai Technologies, Inc."},
"17.248.248.117": {ASNumber: 714, Organization: "Apple Inc."},
"17.248.248.121": {ASNumber: 714, Organization: "Apple Inc."},
"2a01:b740:a41:212::5": {ASNumber: 714, Organization: "Apple Inc."},
"17.248.248.104": {ASNumber: 714, Organization: "Apple Inc."},
"2a01:b740:a41:213::9": {ASNumber: 714, Organization: "Apple Inc."},
"172.224.19.14": {ASNumber: 36183, Organization: "Akamai Technologies, Inc."},
"172.224.19.15": {ASNumber: 36183, Organization: "Akamai Technologies, Inc."},
"2a01:b740:a41:212::6": {ASNumber: 714, Organization: "Apple Inc."},
"172.224.19.17": {ASNumber: 36183, Organization: "Akamai Technologies, Inc."},
"172.224.19.13": {ASNumber: 36183, Organization: "Akamai Technologies, Inc."},
"17.248.248.105": {ASNumber: 714, Organization: "Apple Inc."},
"17.248.248.100": {ASNumber: 714, Organization: "Apple Inc."},
},
expecteBlockingFlags: AnalysisBlockingFlagDNSBlocking,
}}

for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
data := runtimex.Try1(os.ReadFile(tc.tkFile))
data = runtimex.Try1(hujson.Standardize(data))
var tk TestKeys
runtimex.Try0(json.Unmarshal(data, &tk))
log.SetLevel(log.DebugLevel)
tk.analysisClassic(mocks.NewGeoIPASNLookupper(tc.geoInfo), log.Log)
if tc.expecteBlockingFlags != tk.BlockingFlags {
t.Fatal("expected", tc.expecteBlockingFlags, "got", tk.BlockingFlags)
}
})
}
}
24 changes: 22 additions & 2 deletions internal/experiment/webconnectivitylte/analysisext.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,19 @@ import (
"strings"

"github.com/ooni/probe-cli/v3/internal/minipipeline"
"github.com/ooni/probe-cli/v3/internal/model"
)

// analysisExtMain computes the extended analysis.
//
// This function MUTATES the [*TestKeys].
func analysisExtMain(tk *TestKeys, container *minipipeline.WebObservationsContainer) {
func analysisExtMain(
lookupper model.GeoIPASNLookupper,
tk *TestKeys,
container *minipipeline.WebObservationsContainer,
) {
// compute the web analysis
analysis := minipipeline.AnalyzeWebObservationsWithoutLinearAnalysis(container)
analysis := minipipeline.AnalyzeWebObservationsWithoutLinearAnalysis(lookupper, container)

// prepare for emitting informational messages
var info strings.Builder
Expand Down Expand Up @@ -234,6 +239,16 @@ func analysisExtExpectedFailures(tk *TestKeys, analysis *minipipeline.WebAnalysi
// Also note that these cases ARE NOT MUTUALLY EXCLUSIVE meaning that these conditions
// could actually happen simultaneously in a bunch of cases.

// See https://github.com/ooni/probe/issues/2290 for further
// documentation about the issue we're solving here.
//
// It would be tempting to check specifically for NXDOMAIN here, but we
// know it is problematic do that. In fact, on Android the getaddrinfo
// resolver always returns EAI_NODATA on error, regardless of the actual
// error that may have occurred in the Android DNS backend.
//
// See https://github.com/ooni/probe/issues/2029 for more information
// on Android's getaddrinfo behavior.
if expected := analysis.DNSLookupExpectedFailure; expected.Len() > 0 {
tk.NullNullFlags |= AnalysisFlagNullNullExpectedDNSLookupFailure
fmt.Fprintf(
Expand All @@ -242,6 +257,10 @@ func analysisExtExpectedFailures(tk *TestKeys, analysis *minipipeline.WebAnalysi
)
}

// See https://explorer.ooni.org/measurement/20220911T105037Z_webconnectivity_IT_30722_n1_ruzuQ219SmIO9SrT?input=https://doh.centraleu.pi-dns.com/dns-query?dns=q80BAAABAAAAAAAAA3d3dwdleGFtcGxlA2NvbQAAAQAB
// for an example measurement with this behavior.
//
// See also https://github.com/ooni/probe/issues/2299.
if expected := analysis.TCPConnectExpectedFailure; expected.Len() > 0 {
tk.NullNullFlags |= AnalysisFlagNullNullExpectedTCPConnectFailure
fmt.Fprintf(
Expand All @@ -250,6 +269,7 @@ func analysisExtExpectedFailures(tk *TestKeys, analysis *minipipeline.WebAnalysi
)
}

// See https://github.com/ooni/probe/issues/2300.
if expected := analysis.TLSHandshakeExpectedFailure; expected.Len() > 0 {
tk.NullNullFlags |= AnalysisFlagNullNullExpectedTLSHandshakeFailure
fmt.Fprintf(
Expand Down
47 changes: 35 additions & 12 deletions internal/minipipeline/analysis.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package minipipeline
import (
"sort"

"github.com/ooni/probe-cli/v3/internal/model"
"github.com/ooni/probe-cli/v3/internal/optional"
"github.com/ooni/probe-cli/v3/internal/runtimex"
)
Expand Down Expand Up @@ -94,13 +95,14 @@ func NewLinearWebAnalysis(input *WebObservationsContainer) (output []*WebObserva

// AnalyzeWebObservationsWithoutLinearAnalysis generates a [*WebAnalysis] from a [*WebObservationsContainer]
// but avoids calling [NewLinearyAnalysis] to generate a linear analysis.
func AnalyzeWebObservationsWithoutLinearAnalysis(container *WebObservationsContainer) *WebAnalysis {
func AnalyzeWebObservationsWithoutLinearAnalysis(
lookupper model.GeoIPASNLookupper, container *WebObservationsContainer) *WebAnalysis {
analysis := &WebAnalysis{
ControlExpectations: container.ControlExpectations,
}

analysis.dnsComputeSuccessMetrics(container)
analysis.dnsComputeSuccessMetricsClassic(container)
analysis.dnsComputeSuccessMetrics(lookupper, container)
analysis.dnsComputeSuccessMetricsClassic(lookupper, container)
analysis.dnsComputeFailureMetrics(container)

analysis.tcpComputeMetrics(container)
Expand All @@ -113,8 +115,9 @@ func AnalyzeWebObservationsWithoutLinearAnalysis(container *WebObservationsConta

// AnalyzeWebObservationsWithLinearAnalysis generates a [*WebAnalysis] from a [*WebObservationsContainer]
// and ensures we also use [NewLinearyAnalysis] to generate a linear analysis.
func AnalyzeWebObservationsWithLinearAnalysis(container *WebObservationsContainer) *WebAnalysis {
analysis := AnalyzeWebObservationsWithoutLinearAnalysis(container)
func AnalyzeWebObservationsWithLinearAnalysis(
lookupper model.GeoIPASNLookupper, container *WebObservationsContainer) *WebAnalysis {
analysis := AnalyzeWebObservationsWithoutLinearAnalysis(lookupper, container)
analysis.Linear = NewLinearWebAnalysis(container)
return analysis
}
Expand Down Expand Up @@ -262,7 +265,8 @@ type WebAnalysis struct {
Linear []*WebObservation
}

func (wa *WebAnalysis) dnsComputeSuccessMetrics(c *WebObservationsContainer) {
func (wa *WebAnalysis) dnsComputeSuccessMetrics(
lookupper model.GeoIPASNLookupper, c *WebObservationsContainer) {
// fill the invalid set
var already Set[int64]
for _, obs := range c.DNSLookupSuccesses {
Expand Down Expand Up @@ -304,7 +308,7 @@ func (wa *WebAnalysis) dnsComputeSuccessMetrics(c *WebObservationsContainer) {
}

// this lookup is good if there is ASN intersection
if DNSDiffFindCommonASNsIntersection(measurement, control).Len() > 0 {
if DNSDiffFindCommonASNsIntersection(lookupper, measurement, control).Len() > 0 {
wa.DNSLookupSuccessWithValidAddress.Add(obs.DNSTransactionID.Unwrap())
continue
}
Expand Down Expand Up @@ -332,7 +336,8 @@ func (wa *WebAnalysis) dnsComputeSuccessMetrics(c *WebObservationsContainer) {
}
}

func (wa *WebAnalysis) dnsComputeSuccessMetricsClassic(c *WebObservationsContainer) {
func (wa *WebAnalysis) dnsComputeSuccessMetricsClassic(
lookupper model.GeoIPASNLookupper, c *WebObservationsContainer) {
var already Set[int64]

for _, obs := range c.DNSLookupSuccesses {
Expand Down Expand Up @@ -363,7 +368,7 @@ func (wa *WebAnalysis) dnsComputeSuccessMetricsClassic(c *WebObservationsContain
}

// this lookup is good if there is ASN intersection
if DNSDiffFindCommonASNsIntersection(measurement, control).Len() > 0 {
if DNSDiffFindCommonASNsIntersection(lookupper, measurement, control).Len() > 0 {
wa.DNSLookupSuccessWithValidAddressClassic.Add(obs.DNSTransactionID.Unwrap())
continue
}
Expand Down Expand Up @@ -421,7 +426,18 @@ func (wa *WebAnalysis) dnsComputeFailureMetrics(c *WebObservationsContainer) {
continue
}

// handle the case where both failed
// Handle the case where both failed.
//
// See https://github.com/ooni/probe/issues/2290 for further
// documentation about the issue we're solving here.
//
// It would be tempting to check specifically for NXDOMAIN here, but we
// know it is problematic do that. In fact, on Android the getaddrinfo
// resolver always returns EAI_NODATA on error, regardless of the actual
// error that may have occurred in the Android DNS backend.
//
// See https://github.com/ooni/probe/issues/2029 for more information
// on Android's getaddrinfo behavior.
if obs.DNSLookupFailure.Unwrap() != "" && obs.ControlDNSLookupFailure.Unwrap() != "" {
wa.DNSLookupExpectedFailure.Add(obs.DNSTransactionID.Unwrap())
continue
Expand Down Expand Up @@ -481,7 +497,12 @@ func (wa *WebAnalysis) tcpComputeMetrics(c *WebObservationsContainer) {

// handle the case where the control fails
if obs.ControlTCPConnectFailure.Unwrap() != "" {
// if also the probe failed mark this failure as expected
// If also the probe failed mark this failure as expected.
//
// See https://explorer.ooni.org/measurement/20220911T105037Z_webconnectivity_IT_30722_n1_ruzuQ219SmIO9SrT?input=https://doh.centraleu.pi-dns.com/dns-query?dns=q80BAAABAAAAAAAAA3d3dwdleGFtcGxlA2NvbQAAAQAB
// for an example measurement with this behavior.
//
// See also https://github.com/ooni/probe/issues/2299.
if obs.TCPConnectFailure.Unwrap() != "" {
wa.TCPConnectExpectedFailure.Add(obs.EndpointTransactionID.Unwrap())
}
Expand Down Expand Up @@ -540,7 +561,9 @@ func (wa *WebAnalysis) tlsComputeMetrics(c *WebObservationsContainer) {

// handle the case where the control fails
if obs.ControlTLSHandshakeFailure.Unwrap() != "" {
// if also the probe failed mark this failure as expected
// If also the probe failed mark this failure as expected.
//
// See https://github.com/ooni/probe/issues/2300.
if obs.TLSHandshakeFailure.Unwrap() != "" {
wa.TLSHandshakeExpectedFailure.Add(obs.EndpointTransactionID.Unwrap())
}
Expand Down
Loading

0 comments on commit 0a5604a

Please sign in to comment.