fix: anomaly with android_dns_cache_no_data and inconsistent dns (#1211)

## Checklist

- [x] I have read the [contribution
guidelines](https://github.com/ooni/probe-cli/blob/master/CONTRIBUTING.md)
- [x] reference issue for this pull request:
ooni/probe#2499
- [x] if you changed anything related to how experiments work and you
need to reflect these changes in the ooni/spec repository, please link
to the related ooni/spec pull request: N/A
- [x] if you changed code inside an experiment, make sure you bump its
version number

## Summary

This diff changes the `summary.go` algorithm of Web Connectivity v0.4 to
handle `android_dns_cache_no_data` as an anomaly when the DNS is
inconsistent. We continue handling `dns_nxdomain_error` as an anomaly
when the DNS is inconsistent, as demonstrated by the fact that the
corresponding netem test is still passing.

This diff also bumps the version number to v0.4.3. Version v0.4.2 did
not handle this case, which caused measurements to be marked as failed
as documented by ooni/probe#2499.

This diff is also related to ooni/probe#2029,
in the sense that it is slightly improving our analysis results when the
is an NXDOMAIN error (even if it's masked by Android's DNS cache
behavior).

While there, add empty lines to improve the code readability.

internal/experiment/webconnectivity/dnsanalysis.go

@@ -34,12 +34,14 @@ func DNSAnalysis(URL *url.URL, measurement DNSLookupResult,
 	control ControlResponse) (out DNSAnalysisResult) {
 	// 0. start assuming it's not consistent
 	out.DNSConsistency = &DNSInconsistent
+
 	// 1. flip to consistent if we're targeting an IP address because the
 	// control will actually return dns_name_error in this case.
 	if net.ParseIP(URL.Hostname()) != nil {
 		out.DNSConsistency = &DNSConsistent
 		return
 	}
+
 	// 2. flip to consistent if the failures are compatible
 	if measurement.Failure != nil && control.DNS.Failure != nil {
 		switch *control.DNS.Failure {
@@ -57,6 +59,7 @@ func DNSAnalysis(URL *url.URL, measurement DNSLookupResult,
 		}
 		return
 	}
+
 	// 3. flip to consistent if measurement and control returned IP addresses
 	// that belong to the same Autonomous System(s).
 	//
@@ -85,6 +88,7 @@ func DNSAnalysis(URL *url.URL, measurement DNSLookupResult,
 			return
 		}
 	}
+
 	// 4. when ASN lookup failed (unlikely), check whether
 	// there is overlap in the returned IP addresses
 	ipmap := make(map[string]int)
@@ -101,6 +105,7 @@ func DNSAnalysis(URL *url.URL, measurement DNSLookupResult,
 			return
 		}
 	}
+
 	// 5. conclude that measurement and control are inconsistent
 	return
 }

internal/experiment/webconnectivity/summary.go

@@ -100,6 +100,7 @@ func Summarize(tk *TestKeys) (out Summary) {
 	defer func() {
 		out.Blocking = DetermineBlocking(out)
 	}()
+
 	var (
 		accessible   = true
 		inaccessible = false
@@ -108,6 +109,7 @@ func Summarize(tk *TestKeys) (out Summary) {
 		httpFailure  = "http-failure"
 		tcpIP        = "tcp_ip"
 	)
+
 	// If the measurement was for an HTTPS website and the HTTP experiment
 	// succeeded, then either there is a compromised CA in our pool (which is
 	// certifi-go), or there is transparent proxying, or we are actually
@@ -119,11 +121,13 @@ func Summarize(tk *TestKeys) (out Summary) {
 		out.Status |= StatusSuccessSecure
 		return
 	}
+
 	// If we couldn't contact the control, we cannot do much more here.
 	if tk.ControlFailure != nil {
 		out.Status |= StatusAnomalyControlUnreachable
 		return
 	}
+
 	// If DNS failed with NXDOMAIN and the control DNS is consistent, then it
 	// means this website does not exist anymore. We need to include the weird
 	// cache failure on Android into this analysis because that failure means
@@ -142,15 +146,33 @@ func Summarize(tk *TestKeys) (out Summary) {
 		out.Status |= StatusSuccessNXDOMAIN | StatusExperimentDNS
 		return
 	}
-	// Otherwise, if DNS failed with NXDOMAIN, it's DNS based blocking.
-	// TODO(bassosimone): do we wanna include other errors here? Like timeout?
-	if tk.DNSExperimentFailure != nil &&
-		*tk.DNSExperimentFailure == netxlite.FailureDNSNXDOMAINError {
+
+	// Web Connectivity's analysis algorithm up until v0.4.2 gave priority to checking for http-diff
+	// over saying that there's "dns" blocking when the DNS is inconsistent.
+	//
+	// In v0.4.3, we want to address https://github.com/ooni/probe/issues/2499 while still
+	// trying to preserve the original spirit of the v0.4.2 analysis algorithm.
+	//
+	// To this end, we _only_ flag anomaly if the following happens:
+	//
+	// 1. the DNS is inconsistent; and
+	//
+	// 2. the probe's DNS lookup has failed with dns_nxdomain_error or android_dns_cache_no_data.
+	//
+	// By using this algorithm, we narrow the scope and impact of this change but we are, at
+	// the same time, able to catch cases such as the one mentioned by the issue above.
+	//
+	// A more aggressive approach would flag as "dns" blocking any inconsistent result but
+	// that would depart quite a lot from the behavior of v0.4.2.
+	if tk.DNSConsistency != nil && *tk.DNSConsistency == DNSInconsistent &&
+		tk.DNSExperimentFailure != nil && (*tk.DNSExperimentFailure == netxlite.FailureDNSNXDOMAINError ||
+		*tk.DNSExperimentFailure == netxlite.FailureAndroidDNSCacheNoData) {
 		out.Accessible = &inaccessible
 		out.BlockingReason = &dns
 		out.Status |= StatusAnomalyDNS | StatusExperimentDNS
 		return
 	}
+
 	// If we tried to connect more than once and never succedeed and we were
 	// able to measure DNS consistency, then we can conclude something.
 	if tk.TCPConnectAttempts > 0 && tk.TCPConnectSuccesses <= 0 && tk.DNSConsistency != nil {

internal/experiment/webconnectivity/webconnectivity.go

@@ -15,7 +15,7 @@ import (
 
 const (
 	testName    = "web_connectivity"
-	testVersion = "0.4.2"
+	testVersion = "0.4.3"
 )
 
 // Config contains the experiment config.

internal/experiment/webconnectivity/webconnectivity_test.go

@@ -21,7 +21,7 @@ func TestNewExperimentMeasurer(t *testing.T) {
 	if measurer.ExperimentName() != "web_connectivity" {
 		t.Fatal("unexpected name")
 	}
-	if measurer.ExperimentVersion() != "0.4.2" {
+	if measurer.ExperimentVersion() != "0.4.3" {
 		t.Fatal("unexpected version")
 	}
 }

internal/experiment/webconnectivityqa/dnsblocking.go

@@ -9,7 +9,7 @@ import (
 func dnsBlockingAndroidDNSCacheNoData() *TestCase {
 	return &TestCase{
 		Name:  "dnsBlockingAndroidDNSCacheNoData",
-		Flags: TestCaseFlagNoV04, // see https://github.com/ooni/probe-cli/pull/1211
+		Flags: 0,
 		Input: "https://www.example.com/",
 		Configure: func(env *netemx.QAEnv) {
 			// make sure the env knows we want to emulate our getaddrinfo wrapper behavior
@@ -23,8 +23,9 @@ func dnsBlockingAndroidDNSCacheNoData() *TestCase {
 		ExpectTestKeys: &testKeys{
 			DNSExperimentFailure: "android_dns_cache_no_data",
 			DNSConsistency:       "inconsistent",
-			XDNSFlags:            2,  // AnalysisDNSUnexpectedFailure
-			XBlockingFlags:       33, // analysisFlagDNSBlocking | analysisFlagSuccess
+			XStatus:              2080, // StatusExperimentDNS | StatusAnomalyDNS
+			XDNSFlags:            2,    // AnalysisDNSUnexpectedFailure
+			XBlockingFlags:       33,   // analysisFlagDNSBlocking | analysisFlagSuccess
 			Accessible:           false,
 			Blocking:             "dns",
 		},

internal/experiment/webconnectivityqa/run_test.go

@@ -78,7 +78,7 @@ func TestRunTestCase(t *testing.T) {
 				return "web_connectivity"
 			},
 			MockExperimentVersion: func() string {
-				return "0.4.2"
+				return "0.4.3"
 			},
 			MockRun: func(ctx context.Context, args *model.ExperimentArgs) error {
 				args.Measurement.TestKeys = &testKeys{}
@@ -107,7 +107,7 @@ func TestRunTestCase(t *testing.T) {
 				return "web_connectivity"
 			},
 			MockExperimentVersion: func() string {
-				return "0.4.2"
+				return "0.4.3"
 			},
 			MockRun: func(ctx context.Context, args *model.ExperimentArgs) error {
 				args.Measurement.TestKeys = &testKeys{
@@ -139,7 +139,7 @@ func TestRunTestCase(t *testing.T) {
 				return "web_connectivity"
 			},
 			MockExperimentVersion: func() string {
-				return "0.4.2"
+				return "0.4.3"
 			},
 			MockRun: func(ctx context.Context, args *model.ExperimentArgs) error {
 				args.Measurement.TestKeys = &testKeys{
@@ -173,7 +173,7 @@ func TestRunTestCase(t *testing.T) {
 				return "web_connectivity"
 			},
 			MockExperimentVersion: func() string {
-				return "0.4.2"
+				return "0.4.3"
 			},
 			MockRun: func(ctx context.Context, args *model.ExperimentArgs) error {
 				args.Measurement.TestKeys = &testKeys{

internal/experiment/webconnectivityqa/testkeys.go

@@ -69,7 +69,7 @@ func compareTestKeys(expected, got *testKeys) error {
 	}
 
 	switch got.XExperimentVersion {
-	case "0.4.2":
+	case "0.4.3":
 		// ignore the fields that are specific to LTE
 		options = append(options, cmpopts.IgnoreFields(testKeys{}, "XDNSFlags", "XBlockingFlags", "XNullNullFlags"))