diff --git a/internal/webconnectivityalgo/dnsoverhttps.go b/internal/webconnectivityalgo/dnsoverhttps.go index a096ce35a..6fa392e88 100644 --- a/internal/webconnectivityalgo/dnsoverhttps.go +++ b/internal/webconnectivityalgo/dnsoverhttps.go @@ -12,15 +12,17 @@ import ( "time" ) -// TODO(bassosimone): consider whether factoring out this code -// and storing the state on disk instead of using memory - // TODO(bassosimone): consider unifying somehow this code and // the systemresolver code (or maybe just the list of resolvers) // OpportunisticDNSOverHTTPSURLProvider allows to perform opportunistic DNS-over-HTTPS // measurements as part of Web Connectivity LTE. The zero value of this struct is not valid, // please use [NewOpportunisticDNSOverHTTPSURLProvider] to construct. +// +// Implementation note: this code uses memory to keep track of the resolvers and know +// when to perform the next opportunistic check. It seems pointless to use the disk since +// invocations of Web Connectivity typically consist of multiple URLs and therefore run +// for a few minutes. Hence, storing state on disk seems a bit overkill here. type OpportunisticDNSOverHTTPSURLProvider struct { // interval is the next interval after which to measure. interval time.Duration @@ -34,34 +36,58 @@ type OpportunisticDNSOverHTTPSURLProvider struct { // t is when we last run an opportunistic measurement. t time.Time + // timeNow is the function to get the current time. + timeNow func() time.Time + // urls contains the urls of known DoH services. urls []string } // NewOpportunisticDNSOverHTTPSURLProvider creates a new [*OpportunisticDNSOverHTTPSURLProvider]. func NewOpportunisticDNSOverHTTPSURLProvider(urls ...string) *OpportunisticDNSOverHTTPSURLProvider { - return &OpportunisticDNSOverHTTPSURLProvider{ + o := &OpportunisticDNSOverHTTPSURLProvider{ interval: 0, mu: &sync.Mutex{}, - rnd: rand.New(rand.NewSource(time.Now().UnixNano())), + rnd: nil, // configured below t: time.Time{}, + timeNow: time.Now, urls: urls, } + o.seed(o.timeNow()) // allow unit tests to reconfigure the seed we use + return o +} + +func (o *OpportunisticDNSOverHTTPSURLProvider) seed(t time.Time) { + o.rnd = rand.New(rand.NewSource(t.UnixNano())) } // MaybeNextURL returns the next URL to measure, if any. Our aim is to perform // periodic, opportunistic DoH measurements as part of Web Connectivity. func (o *OpportunisticDNSOverHTTPSURLProvider) MaybeNextURL() (string, bool) { - now := time.Now() + // obtain the current time + now := o.timeNow() + + // make sure there's mutual exclusion o.mu.Lock() defer o.mu.Unlock() - if o.t.IsZero() || now.Sub(o.t) > o.interval { + + // Make sure we run periodically but now always, since there is no point in + // always using DNS-over-HTTPS rather the aim is to opportunistically try using + // it so to collect data on whether it's actually WAI. + if len(o.urls) > 0 && (o.t.IsZero() || now.Sub(o.t) > o.interval) { + + // shuffle the list according to the selected random profile o.rnd.Shuffle(len(o.urls), func(i, j int) { o.urls[i], o.urls[j] = o.urls[j], o.urls[i] }) + + // register the current invocation and remember to run again later o.t = now o.interval = time.Duration(20+o.rnd.Uint32()%20) * time.Second + + // return the selected URL to the caller return o.urls[0], true } + return "", false } diff --git a/internal/webconnectivityalgo/dnsoverhttps_test.go b/internal/webconnectivityalgo/dnsoverhttps_test.go new file mode 100644 index 000000000..29e9264a6 --- /dev/null +++ b/internal/webconnectivityalgo/dnsoverhttps_test.go @@ -0,0 +1,150 @@ +package webconnectivityalgo + +import ( + "testing" + "time" + + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/runtimex" +) + +type testTimeProvider struct { + t0 time.Time + times []time.Duration + idx int +} + +func (ttp *testTimeProvider) timeNow() time.Time { + runtimex.Assert(ttp.idx < len(ttp.times), "out of bounds") + mockedTime := ttp.t0.Add(ttp.times[ttp.idx]) + ttp.idx++ + return mockedTime +} + +func TestOpportunisticDNSOverHTTPSURLProvider(t *testing.T) { + + // expectation is an expectation of a test case. + type expectation struct { + URL string + Good bool + } + + // testcase is a test case implemented by this testing function. + type testcase struct { + // name is the test case name. + name string + + // timeNow is the function to obtain time. In case it is zero, we're + // not goint to reconfigure the time fetching function. + timeNow func() time.Time + + // seed is the random seed or zero. In case it is zero, we're not + // going to reconfigure the random see we use. + seed time.Time + + // urls contains the URLs to use. + urls []string + + // expect contains the expectations. + expect []expectation + } + + // cases contains test cases. + cases := []testcase{{ + name: "without any URL", + timeNow: nil, + seed: time.Time{}, + urls: []string{}, + expect: []expectation{{ + URL: "", + Good: false, + }, { + URL: "", + Good: false, + }, { + URL: "", + Good: false, + }}, + }, { + name: "with a single URL we get it and then need to wait", + timeNow: (&testTimeProvider{ + t0: time.Date(2024, 2, 8, 9, 8, 7, 6, time.UTC), + times: []time.Duration{ + 0, // should return URL + 1 * time.Second, // too early to get another URL + 5 * time.Second, // ditto + }, + idx: 0, + }).timeNow, + seed: time.Date(2024, 2, 8, 9, 8, 7, 6, time.UTC), + urls: []string{ + "https://dns.google/dns-query", + }, + expect: []expectation{{ + URL: "https://dns.google/dns-query", + Good: true, + }, { + URL: "", + Good: false, + }, { + URL: "", + Good: false, + }}, + }, { + name: "with multiple URLs and long wait times we have shuffling", + timeNow: (&testTimeProvider{ + t0: time.Date(2024, 2, 8, 9, 8, 7, 6, time.UTC), + times: []time.Duration{ + 0, // should return URL + 60 * time.Minute, // ditto + 120 * time.Minute, // ditto + }, + idx: 0, + }).timeNow, + seed: time.Date(2024, 2, 8, 9, 8, 7, 6, time.UTC), + urls: []string{ + "https://dns.google/dns-query", + "https://cloudflare-dns.com/dns-query", + }, + expect: []expectation{{ + URL: "https://cloudflare-dns.com/dns-query", + Good: true, + }, { + URL: "https://dns.google/dns-query", + Good: true, + }, { + URL: "https://dns.google/dns-query", + Good: true, + }}, + }} + + // run test cases + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + oup := NewOpportunisticDNSOverHTTPSURLProvider(tc.urls...) + + // note: we need to reconfigure timeNow before resetting the seed + if tc.timeNow != nil { + oup.timeNow = tc.timeNow + } + if !tc.seed.IsZero() { + oup.seed(tc.seed) + } else { + oup.seed(oup.timeNow()) + } + + var got []expectation + for len(got) < len(tc.expect) { + url, good := oup.MaybeNextURL() + got = append(got, expectation{ + URL: url, + Good: good, + }) + } + + if diff := cmp.Diff(tc.expect, got); diff != "" { + t.Fatal(diff) + } + }) + } +}