fix: anomaly with android_dns_cache_no_data and inconsistent dns

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"))