From 68b29251efb7edac4fbc8def1809d2a8561511a3 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 9 Feb 2024 19:40:19 +0100 Subject: [PATCH] fix(webconnectivitylte): include client_resolver (#1504) Closes https://github.com/ooni/probe/issues/2676 --- .../experiment/webconnectivitylte/measurer.go | 5 ++ internal/webconnectivityqa/checker.go | 38 ++++++++++- internal/webconnectivityqa/checker_test.go | 63 +++++++++++++++++++ internal/webconnectivityqa/success.go | 14 +++-- 4 files changed, 114 insertions(+), 6 deletions(-) diff --git a/internal/experiment/webconnectivitylte/measurer.go b/internal/experiment/webconnectivitylte/measurer.go index 4368d912e..8bac97733 100644 --- a/internal/experiment/webconnectivitylte/measurer.go +++ b/internal/experiment/webconnectivitylte/measurer.go @@ -78,6 +78,11 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { tk := NewTestKeys() measurement.TestKeys = tk + // make sure we add the ClientResolver field + // + // See https://github.com/ooni/probe/issues/2676 + tk.ClientResolver = sess.ResolverIP() + // create variables required to run parallel tasks idGenerator := &atomic.Int64{} wg := &sync.WaitGroup{} diff --git a/internal/webconnectivityqa/checker.go b/internal/webconnectivityqa/checker.go index dd878059f..744a1e167 100644 --- a/internal/webconnectivityqa/checker.go +++ b/internal/webconnectivityqa/checker.go @@ -2,12 +2,12 @@ package webconnectivityqa import ( "errors" + "fmt" "strings" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/must" "github.com/ooni/probe-cli/v3/internal/netxlite" - "github.com/ooni/probe-cli/v3/internal/x/dslx" ) // Checker checks whether a measurement is correct. @@ -26,6 +26,10 @@ var ErrCheckerNoReadWriteEvents = errors.New("no read or write events") // ErrCheckerUnexpectedWebConnectivityVersion indicates that the version is unexpected var ErrCheckerUnexpectedWebConnectivityVersion = errors.New("unexpected Web Connectivity version") +type readWriteEventsExistentialCheckerTestKeys struct { + NetworkEvents []*model.ArchivalNetworkEvent `json:"network_events"` +} + // Check implements Checker. func (*ReadWriteEventsExistentialChecker) Check(mx *model.Measurement) error { // we don't care about v0.4 @@ -39,7 +43,7 @@ func (*ReadWriteEventsExistentialChecker) Check(mx *model.Measurement) error { } // serialize and reparse the test keys - var tk *dslx.Observations + var tk *readWriteEventsExistentialCheckerTestKeys must.UnmarshalJSON(must.MarshalJSON(mx.TestKeys), &tk) // count the read/write events @@ -59,3 +63,33 @@ func (*ReadWriteEventsExistentialChecker) Check(mx *model.Measurement) error { } return nil } + +// ClientResolverCorrectnessChecker checks whether the client_resolver field +// inside of the test_keys has been correctly configured. +type ClientResolverCorrectnessChecker struct{} + +var _ Checker = &ClientResolverCorrectnessChecker{} + +type clientResolverCorrectnessCheckerTestKeys struct { + ClientResolver string `json:"client_resolver"` +} + +// ErrCheckerInvalidClientResolver indicates that the client_resolver field is invalid. +var ErrCheckerInvalidClientResolver = errors.New("invalid client_resolver field") + +// Check implements Checker. +func (*ClientResolverCorrectnessChecker) Check(mx *model.Measurement) error { + var tk *clientResolverCorrectnessCheckerTestKeys + must.UnmarshalJSON(must.MarshalJSON(mx.TestKeys), &tk) + + if mx.ResolverIP != tk.ClientResolver { + return fmt.Errorf( + "%w: expected '%s', got '%s'", + ErrCheckerInvalidClientResolver, + mx.ResolverIP, + tk.ClientResolver, + ) + } + + return nil +} diff --git a/internal/webconnectivityqa/checker_test.go b/internal/webconnectivityqa/checker_test.go index 7d16f23bf..2283cadcb 100644 --- a/internal/webconnectivityqa/checker_test.go +++ b/internal/webconnectivityqa/checker_test.go @@ -3,12 +3,14 @@ package webconnectivityqa_test import ( "context" "errors" + "fmt" "testing" "github.com/ooni/probe-cli/v3/internal/experiment/webconnectivitylte" "github.com/ooni/probe-cli/v3/internal/mocks" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/must" + "github.com/ooni/probe-cli/v3/internal/netemx" "github.com/ooni/probe-cli/v3/internal/optional" "github.com/ooni/probe-cli/v3/internal/webconnectivityqa" ) @@ -110,3 +112,64 @@ func TestReadWriteEventsExistentialChecker(t *testing.T) { }) } } + +func TestClientResolverCorrectnessChecker(t *testing.T) { + type testcase struct { + name string + tk string + expect error + } + + cases := []testcase{{ + name: "with correct value", + tk: fmt.Sprintf(`{"client_resolver":"%s"}`, netemx.ISPResolverAddress), + expect: nil, + }, { + name: "with empty", + tk: `{"client_resolver":""}`, + expect: fmt.Errorf( + "%w: expected '%s', got ''", + webconnectivityqa.ErrCheckerInvalidClientResolver, + netemx.ISPResolverAddress, + ), + }, { + name: "with different value", + tk: `{"client_resolver":"10.0.0.1"}`, + expect: fmt.Errorf( + "%w: expected '%s', got '%s'", + webconnectivityqa.ErrCheckerInvalidClientResolver, + netemx.ISPResolverAddress, + "10.0.0.1", + ), + }} + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var tks map[string]any + must.UnmarshalJSON([]byte(tc.tk), &tks) + + meas := &model.Measurement{ + TestKeys: tks, + ResolverIP: netemx.ISPResolverAddress, + } + + err := (&webconnectivityqa.ClientResolverCorrectnessChecker{}).Check(meas) + + switch { + case tc.expect == nil && err == nil: + return + + case tc.expect == nil && err != nil: + t.Fatal("expected", tc.expect, "got", err) + + case tc.expect != nil && err == nil: + t.Fatal("expected", tc.expect, "got", err) + + case tc.expect != nil && err != nil: + if err.Error() != tc.expect.Error() { + t.Fatal("expected", tc.expect, "got", err) + } + } + }) + } +} diff --git a/internal/webconnectivityqa/success.go b/internal/webconnectivityqa/success.go index 44db7a785..8d30ddbbd 100644 --- a/internal/webconnectivityqa/success.go +++ b/internal/webconnectivityqa/success.go @@ -1,5 +1,13 @@ package webconnectivityqa +var successCheckers = []Checker{ + // See https://github.com/ooni/probe/issues/2674 + &ReadWriteEventsExistentialChecker{}, + + // See https://github.com/ooni/probe/issues/2676 + &ClientResolverCorrectnessChecker{}, +} + // successWithHTTP ensures we can successfully measure an HTTP URL. func successWithHTTP() *TestCase { return &TestCase{ @@ -20,10 +28,7 @@ func successWithHTTP() *TestCase { Accessible: true, Blocking: false, }, - Checkers: []Checker{ - // See https://github.com/ooni/probe/issues/2674 - &ReadWriteEventsExistentialChecker{}, - }, + Checkers: successCheckers, } } @@ -47,5 +52,6 @@ func successWithHTTPS() *TestCase { Accessible: true, Blocking: false, }, + Checkers: successCheckers, } }