refactor: fully move IDNAResolver to netxlite (#433)

We started doing this in https://github.com/ooni/probe-cli/pull/432.

This work is part of https://github.com/ooni/probe/issues/1733.

internal/cmd/oohelperd/internal/nwcth/measure.go

@@ -91,6 +91,6 @@ func newResolver() netxlite.Resolver {
 	childResolver, err := netx.NewDNSClient(netx.Config{Logger: log.Log}, "doh://google")
 	runtimex.PanicOnError(err, "NewDNSClient failed")
 	var r netxlite.Resolver = childResolver
-	r = &netxlite.IDNAResolver{Resolver: r}
+	r = &netxlite.ResolverIDNA{Resolver: r}
 	return r
 }

internal/engine/experiment/websteps/dns.go

@@ -21,7 +21,7 @@ func DNSDo(ctx context.Context, config DNSConfig) ([]string, error) {
 		childResolver, err := netx.NewDNSClient(netx.Config{Logger: log.Log}, "doh://google")
 		runtimex.PanicOnError(err, "NewDNSClient failed")
 		var resolver netxlite.Resolver = childResolver
-		resolver = &netxlite.IDNAResolver{Resolver: resolver}
+		resolver = &netxlite.ResolverIDNA{Resolver: resolver}
 	}
 	return config.Resolver.LookupHost(ctx, config.Domain)
 }

internal/engine/netx/netx.go

@@ -139,7 +139,7 @@ func NewResolver(config Config) Resolver {
 	if config.ResolveSaver != nil {
 		r = resolver.SaverResolver{Resolver: r, Saver: config.ResolveSaver}
 	}
-	return resolver.IDNAResolver{Resolver: r}
+	return &resolver.IDNAResolver{Resolver: r}
 }
 
 // NewDialer creates a new Dialer from the specified config

internal/engine/netx/netx_test.go

@@ -20,7 +20,7 @@ import (
 
 func TestNewResolverVanilla(t *testing.T) {
 	r := netx.NewResolver(netx.Config{})
-	ir, ok := r.(resolver.IDNAResolver)
+	ir, ok := r.(*resolver.IDNAResolver)
 	if !ok {
 		t.Fatal("not the resolver we expected")
 	}
@@ -44,7 +44,7 @@ func TestNewResolverSpecificResolver(t *testing.T) {
 			// not initialized because it doesn't matter in this context
 		},
 	})
-	ir, ok := r.(resolver.IDNAResolver)
+	ir, ok := r.(*resolver.IDNAResolver)
 	if !ok {
 		t.Fatal("not the resolver we expected")
 	}
@@ -66,7 +66,7 @@ func TestNewResolverWithBogonFilter(t *testing.T) {
 	r := netx.NewResolver(netx.Config{
 		BogonIsError: true,
 	})
-	ir, ok := r.(resolver.IDNAResolver)
+	ir, ok := r.(*resolver.IDNAResolver)
 	if !ok {
 		t.Fatal("not the resolver we expected")
 	}
@@ -92,7 +92,7 @@ func TestNewResolverWithLogging(t *testing.T) {
 	r := netx.NewResolver(netx.Config{
 		Logger: log.Log,
 	})
-	ir, ok := r.(resolver.IDNAResolver)
+	ir, ok := r.(*resolver.IDNAResolver)
 	if !ok {
 		t.Fatal("not the resolver we expected")
 	}
@@ -122,7 +122,7 @@ func TestNewResolverWithSaver(t *testing.T) {
 	r := netx.NewResolver(netx.Config{
 		ResolveSaver: saver,
 	})
-	ir, ok := r.(resolver.IDNAResolver)
+	ir, ok := r.(*resolver.IDNAResolver)
 	if !ok {
 		t.Fatal("not the resolver we expected")
 	}
@@ -151,7 +151,7 @@ func TestNewResolverWithReadWriteCache(t *testing.T) {
 	r := netx.NewResolver(netx.Config{
 		CacheResolutions: true,
 	})
-	ir, ok := r.(resolver.IDNAResolver)
+	ir, ok := r.(*resolver.IDNAResolver)
 	if !ok {
 		t.Fatal("not the resolver we expected")
 	}
@@ -182,7 +182,7 @@ func TestNewResolverWithPrefilledReadonlyCache(t *testing.T) {
 			"dns.google.com": {"8.8.8.8"},
 		},
 	})
-	ir, ok := r.(resolver.IDNAResolver)
+	ir, ok := r.(*resolver.IDNAResolver)
 	if !ok {
 		t.Fatal("not the resolver we expected")
 	}

internal/engine/netx/resolver/idna.go

@@ -1,34 +1,11 @@
 package resolver
 
 import (
-	"context"
+	"github.com/ooni/probe-cli/v3/internal/netxlite"
-
-	"golang.org/x/net/idna"
 )
 
 // IDNAResolver is to support resolving Internationalized Domain Names.
 // See RFC3492 for more information.
-type IDNAResolver struct {
+type IDNAResolver = netxlite.ResolverIDNA
-	Resolver
-}
 
-// LookupHost implements Resolver.LookupHost
+var _ Resolver = &IDNAResolver{}
-func (r IDNAResolver) LookupHost(ctx context.Context, hostname string) ([]string, error) {
-	host, err := idna.ToASCII(hostname)
-	if err != nil {
-		return nil, err
-	}
-	return r.Resolver.LookupHost(ctx, host)
-}
-
-// Network implements Resolver.Network.
-func (r IDNAResolver) Network() string {
-	return "idna"
-}
-
-// Address implements Resolver.Address.
-func (r IDNAResolver) Address() string {
-	return ""
-}
-
-var _ Resolver = IDNAResolver{}

internal/engine/netx/resolver/idna_test.go

@@ -1,76 +0,0 @@
-package resolver_test
-
-import (
-	"context"
-	"errors"
-	"strings"
-	"testing"
-
-	"github.com/google/go-cmp/cmp"
-	"github.com/ooni/probe-cli/v3/internal/engine/netx/resolver"
-)
-
-var ErrUnexpectedPunycode = errors.New("unexpected punycode value")
-
-type CheckIDNAResolver struct {
-	Addresses []string
-	Error     error
-	Expect    string
-}
-
-func (resolv CheckIDNAResolver) LookupHost(ctx context.Context, hostname string) ([]string, error) {
-	if resolv.Error != nil {
-		return nil, resolv.Error
-	}
-	if hostname != resolv.Expect {
-		return nil, ErrUnexpectedPunycode
-	}
-	return resolv.Addresses, nil
-}
-
-func (r CheckIDNAResolver) Network() string {
-	return "checkidna"
-}
-
-func (r CheckIDNAResolver) Address() string {
-	return ""
-}
-
-func TestIDNAResolverSuccess(t *testing.T) {
-	expectedIPs := []string{"77.88.55.66"}
-	resolv := resolver.IDNAResolver{Resolver: CheckIDNAResolver{
-		Addresses: expectedIPs,
-		Expect:    "xn--d1acpjx3f.xn--p1ai",
-	}}
-	addrs, err := resolv.LookupHost(context.Background(), "яндекс.рф")
-	if err != nil {
-		t.Fatal(err)
-	}
-	if diff := cmp.Diff(expectedIPs, addrs); diff != "" {
-		t.Fatal(diff)
-	}
-}
-
-func TestIDNAResolverFailure(t *testing.T) {
-	resolv := resolver.IDNAResolver{Resolver: CheckIDNAResolver{
-		Error: errors.New("we should not arrive here"),
-	}}
-	// See https://www.farsightsecurity.com/blog/txt-record/punycode-20180711/
-	addrs, err := resolv.LookupHost(context.Background(), "xn--0000h")
-	if err == nil || !strings.HasPrefix(err.Error(), "idna: invalid label") {
-		t.Fatal("not the error we expected")
-	}
-	if addrs != nil {
-		t.Fatal("expected no response here")
-	}
-}
-
-func TestIDNAResolverTransportOK(t *testing.T) {
-	resolv := resolver.IDNAResolver{Resolver: CheckIDNAResolver{}}
-	if resolv.Network() != "idna" {
-		t.Fatal("invalid network")
-	}
-	if resolv.Address() != "" {
-		t.Fatal("invalid address")
-	}
-}

internal/engine/netx/resolver/integration_test.go

@@ -44,8 +44,8 @@ func testresolverquickidna(t *testing.T, reso resolver.Resolver) {
 	if testing.Short() {
 		t.Skip("skip test in short mode")
 	}
-	reso = resolver.IDNAResolver{
+	reso = &resolver.IDNAResolver{
-		&netxlite.ResolverLogger{Logger: log.Log, Resolver: reso},
+		Resolver: &netxlite.ResolverLogger{Logger: log.Log, Resolver: reso},
 	}
 	addrs, err := reso.LookupHost(context.Background(), "яндекс.рф")
 	if err != nil {

internal/netxlite/resolver.go

@@ -83,14 +83,15 @@ func (r *ResolverLogger) Address() string {
 	return ""
 }
 
-// IDNAResolver is to support resolving Internationalized Domain Names.
+// ResolverIDNA supports resolving Internationalized Domain Names.
+//
 // See RFC3492 for more information.
-type IDNAResolver struct {
+type ResolverIDNA struct {
 	Resolver
 }
 
-// LookupHost implements Resolver.LookupHost
+// LookupHost implements Resolver.LookupHost.
-func (r *IDNAResolver) LookupHost(ctx context.Context, hostname string) ([]string, error) {
+func (r *ResolverIDNA) LookupHost(ctx context.Context, hostname string) ([]string, error) {
 	host, err := idna.ToASCII(hostname)
 	if err != nil {
 		return nil, err
@@ -99,7 +100,7 @@ func (r *IDNAResolver) LookupHost(ctx context.Context, hostname string) ([]strin
 }
 
 // Network implements Resolver.Network.
-func (r *IDNAResolver) Network() string {
+func (r *ResolverIDNA) Network() string {
 	if rn, ok := r.Resolver.(resolverNetworker); ok {
 		return rn.Network()
 	}
@@ -107,7 +108,7 @@ func (r *IDNAResolver) Network() string {
 }
 
 // Address implements Resolver.Address.
-func (r *IDNAResolver) Address() string {
+func (r *ResolverIDNA) Address() string {
 	if ra, ok := r.Resolver.(resolverAddresser); ok {
 		return ra.Address()
 	}

internal/netxlite/resolver_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"net"
+	"strings"
 	"testing"
 
 	"github.com/apex/log"
@@ -89,3 +90,64 @@ func TestResolverLoggerNoChildNetworkAddress(t *testing.T) {
 		t.Fatal("invalid Address")
 	}
 }
+
+func TestResolverIDNAWorksAsIntended(t *testing.T) {
+	expectedIPs := []string{"77.88.55.66"}
+	r := &ResolverIDNA{
+		Resolver: &netxmocks.Resolver{
+			MockLookupHost: func(ctx context.Context, domain string) ([]string, error) {
+				if domain != "xn--d1acpjx3f.xn--p1ai" {
+					return nil, errors.New("passed invalid domain")
+				}
+				return expectedIPs, nil
+			},
+		},
+	}
+	ctx := context.Background()
+	addrs, err := r.LookupHost(ctx, "яндекс.рф")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if diff := cmp.Diff(expectedIPs, addrs); diff != "" {
+		t.Fatal(diff)
+	}
+}
+
+func TestIDNAResolverWithInvalidPunycode(t *testing.T) {
+	r := &ResolverIDNA{Resolver: &netxmocks.Resolver{
+		MockLookupHost: func(ctx context.Context, domain string) ([]string, error) {
+			return nil, errors.New("should not happen")
+		},
+	}}
+	// See https://www.farsightsecurity.com/blog/txt-record/punycode-20180711/
+	ctx := context.Background()
+	addrs, err := r.LookupHost(ctx, "xn--0000h")
+	if err == nil || !strings.HasPrefix(err.Error(), "idna: invalid label") {
+		t.Fatal("not the error we expected")
+	}
+	if addrs != nil {
+		t.Fatal("expected no response here")
+	}
+}
+
+func TestResolverIDNAChildNetworkAddress(t *testing.T) {
+	r := &ResolverIDNA{
+		Resolver: DefaultResolver,
+	}
+	if v := r.Network(); v != "system" {
+		t.Fatal("invalid network", v)
+	}
+	if v := r.Address(); v != "" {
+		t.Fatal("invalid address", v)
+	}
+}
+
+func TestResolverIDNANoChildNetworkAddress(t *testing.T) {
+	r := &ResolverIDNA{}
+	if v := r.Network(); v != "idna" {
+		t.Fatal("invalid network", v)
+	}
+	if v := r.Address(); v != "" {
+		t.Fatal("invalid address", v)
+	}
+}