Automatic merge of gitlab-org/gitaly master

4 files changed, 177 insertions, 20 deletions

diff --git a/client/dial_test.go b/client/dial_test.go
index db34bf9d4..bc05b1b75 100644
--- a/client/dial_test.go
+++ b/client/dial_test.go
@@ -656,7 +656,32 @@ func TestHealthCheckDialer(t *testing.T) {
 	require.NoError(t, cc.Close())
 }
 
-func TestWithGitalyDNSResolver_successfully(t *testing.T) {
+var dialFuncs = []struct {
+	name string
+	dial func(*testing.T, string, []grpc.DialOption) (*grpc.ClientConn, error)
+}{
+	{
+		name: "Dial",
+		dial: func(t *testing.T, rawAddress string, connOpts []grpc.DialOption) (*grpc.ClientConn, error) {
+			return Dial(rawAddress, connOpts)
+		},
+	},
+	{
+		name: "DialContext",
+		dial: func(t *testing.T, rawAddress string, connOpts []grpc.DialOption) (*grpc.ClientConn, error) {
+			return DialContext(testhelper.Context(t), rawAddress, connOpts)
+		},
+	},
+	{
+		name: "DialSidechannel",
+		dial: func(t *testing.T, rawAddress string, connOpts []grpc.DialOption) (*grpc.ClientConn, error) {
+			sr := NewSidechannelRegistry(testhelper.NewDiscardingLogEntry(t))
+			return DialSidechannel(testhelper.Context(t), rawAddress, sr, connOpts)
+		},
+	},
+}
+
+func TestWithGitalyDNSResolver_resolvableDomain(t *testing.T) {
 	t.Parallel()
 
 	serverURL := startFakeGitalyServer(t)
@@ -671,11 +696,50 @@ func TestWithGitalyDNSResolver_successfully(t *testing.T) {
 	}).Start()
 
 	// This scheme uses our DNS resolver
-	target := fmt.Sprintf("dns://%s/grpc.test:%s", dnsServer.Addr(), serverPort)
-	conn, err := grpc.Dial(
+	url := fmt.Sprintf("dns://%s/grpc.test:%s", dnsServer.Addr(), serverPort)
+	for _, dialFunc := range dialFuncs {
+		dialFunc := dialFunc
+		t.Run(fmt.Sprintf("dial via %s, url = %s", dialFunc.name, url), func(t *testing.T) {
+			t.Parallel()
+			verifyDNSConnection(t, dialFunc.dial, url)
+		})
+	}
+}
+
+func TestWithGitalyDNSResolver_loopbackAddresses(t *testing.T) {
+	t.Parallel()
+
+	serverURL := startFakeGitalyServer(t)
+	_, port, err := net.SplitHostPort(serverURL)
+	require.NoError(t, err)
+
+	urls := []string{
+		fmt.Sprintf("dns:///%s", serverURL),
+		fmt.Sprintf("dns:%s", serverURL),
+		fmt.Sprintf("dns:///localhost:%s", port),
+		fmt.Sprintf("dns:localhost:%s", port),
+	}
+
+	for _, url := range urls {
+		for _, dialFunc := range dialFuncs {
+			url := url
+			dialFunc := dialFunc
+			t.Run(fmt.Sprintf("dial via %s, url = %s", dialFunc.name, url), func(t *testing.T) {
+				t.Parallel()
+				verifyDNSConnection(t, dialFunc.dial, url)
+			})
+		}
+	}
+}
+
+func verifyDNSConnection(t *testing.T, dial func(*testing.T, string, []grpc.DialOption) (*grpc.ClientConn, error), target string) {
+	conn, err := dial(
+		t,
 		target,
-		grpc.WithTransportCredentials(insecure.NewCredentials()),
-		WithGitalyDNSResolver(DefaultDNSResolverBuilderConfig()),
+		[]grpc.DialOption{
+			grpc.WithTransportCredentials(insecure.NewCredentials()),
+			WithGitalyDNSResolver(DefaultDNSResolverBuilderConfig()),
+		},
 	)
 	require.NoError(t, err)
 	defer testhelper.MustClose(t, conn)
@@ -690,23 +754,33 @@ func TestWithGitalyDNSResolver_successfully(t *testing.T) {
 func TestWithGitalyDNSResolver_zeroAddresses(t *testing.T) {
 	t.Parallel()
 
-	dnsServer := testhelper.NewFakeDNSServer(t).WithHandler(dns.TypeA, func(host string) []string {
-		return nil
-	}).Start()
+	for _, dialFunc := range dialFuncs {
+		dialFunc := dialFunc
+		t.Run(fmt.Sprintf("dial via %s", dialFunc.name), func(t *testing.T) {
+			t.Parallel()
 
-	// This scheme uses our DNS resolver
-	target := fmt.Sprintf("dns://%s/grpc.test:50051", dnsServer.Addr())
-	conn, err := grpc.Dial(
-		target,
-		grpc.WithTransportCredentials(insecure.NewCredentials()),
-		WithGitalyDNSResolver(DefaultDNSResolverBuilderConfig()),
-	)
-	require.NoError(t, err)
-	defer testhelper.MustClose(t, conn)
+			dnsServer := testhelper.NewFakeDNSServer(t).WithHandler(dns.TypeA, func(host string) []string {
+				return nil
+			}).Start()
+
+			// This scheme uses our DNS resolver
+			target := fmt.Sprintf("dns://%s/grpc.test:50051", dnsServer.Addr())
+			conn, err := dialFunc.dial(
+				t,
+				target,
+				[]grpc.DialOption{
+					grpc.WithTransportCredentials(insecure.NewCredentials()),
+					WithGitalyDNSResolver(DefaultDNSResolverBuilderConfig()),
+				},
+			)
+			require.NoError(t, err)
+			defer testhelper.MustClose(t, conn)
 
-	client := gitalypb.NewCommitServiceClient(conn)
-	_, err = client.FindCommit(testhelper.Context(t), &gitalypb.FindCommitRequest{})
-	require.Equal(t, err, status.Error(codes.Unavailable, "name resolver error: produced zero addresses"))
+			client := gitalypb.NewCommitServiceClient(conn)
+			_, err = client.FindCommit(testhelper.Context(t), &gitalypb.FindCommitRequest{})
+			require.Equal(t, err, status.Error(codes.Unavailable, "last resolver error: produced zero addresses"))
+		})
+	}
 }
 
 type fakeCommitServer struct {
diff --git a/internal/dnsresolver/target.go b/internal/dnsresolver/target.go
index 3ae9c86be..6975d0272 100644
--- a/internal/dnsresolver/target.go
+++ b/internal/dnsresolver/target.go
@@ -3,10 +3,40 @@ package dnsresolver
 import (
 	"context"
 	"net"
+	"net/url"
+	"strings"
 
 	"gitlab.com/gitlab-org/gitaly/v15/internal/structerr"
 )
 
+// ValidateURL validates Gitaly address URL having dns scheme. The URL follows three forms:
+// * dns://authority-port:authority-host/host:port
+// * dns:///host:port
+// * dns:host:port
+// Either form, the real address is the URL's path
+func ValidateURL(rawAddress string) error {
+	if rawAddress == "" {
+		return structerr.New("empty address")
+	}
+
+	uri, err := url.Parse(rawAddress)
+	if err != nil {
+		return structerr.New("fail to parse address: %w", err)
+	}
+
+	if uri.Scheme != "dns" {
+		return structerr.New("unexpected scheme: %s", uri.Scheme)
+	}
+
+	path := uri.Path
+	if path == "" {
+		// When "//" part is stripped
+		path = uri.Opaque
+	}
+	_, _, err = parseTarget(strings.TrimPrefix(path, "/"), "50051")
+	return err
+}
+
 // parseTarget takes the user input target string and default port, returns formatted host and port info.
 // This is a shameless copy of built-in gRPC dns resolver, because we don't want to have any
 // inconsistency between our resolver and dns resolver.
diff --git a/internal/dnsresolver/target_test.go b/internal/dnsresolver/target_test.go
index 7b97ded41..e4333d535 100644
--- a/internal/dnsresolver/target_test.go
+++ b/internal/dnsresolver/target_test.go
@@ -49,6 +49,48 @@ func TestFindDNSLookup_validAuthority(t *testing.T) {
 	require.Equal(t, []string{"1.2.3.4"}, addrs)
 }
 
+func TestValidateTarget(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		target      string
+		expectedErr error
+	}{
+		{target: "dns:google.com"},
+		{target: "dns:///google.com"},
+		{target: "dns://1.1.1.1/google.com"},
+		{target: "dns:google.com:50051"},
+		{target: "dns:///google.com:50051"},
+		{target: "dns://1.1.1.1:53/google.com:50051"},
+		{target: "dns:[2001:0db8:85a3:0000:0000:8a2e:0370:7334]"},
+		{target: "dns:///[2001:0db8:85a3:0000:0000:8a2e:0370:7334]"},
+		{target: "dns://1.1.1.1:53/[2001:0db8:85a3:0000:0000:8a2e:0370:7334]"},
+		{target: "dns:[2001:0db8:85a3:0000:0000:8a2e:0370:7334]:50051"},
+		{target: "dns:///[2001:0db8:85a3:0000:0000:8a2e:0370:7334]:50051"},
+		{target: "dns://1.1.1.1:53/[2001:0db8:85a3:0000:0000:8a2e:0370:7334]:50051"},
+		{target: "dns:[fe80::1ff:fe23:4567:890a]:50051"},
+		{target: "dns:///[fe80::1ff:fe23:4567:890a]:50051"},
+		{target: "dns://1.1.1.1:53/[fe80::1ff:fe23:4567:890a]:50051"},
+		{
+			target:      "dns:[fe80::1ff:fe23:4567:890a]:",
+			expectedErr: structerr.New("dns resolver: missing port after port-separator colon"),
+		},
+		{
+			target:      "tcp://[fe80::1ff:fe23:4567:890a]",
+			expectedErr: structerr.New("unexpected scheme: tcp"),
+		},
+		{
+			target:      "",
+			expectedErr: structerr.New("empty address"),
+		},
+	}
+	for _, tc := range tests {
+		t.Run(fmt.Sprintf("target: %s", tc.target), func(t *testing.T) {
+			require.Equal(t, tc.expectedErr, ValidateURL(tc.target))
+		})
+	}
+}
+
 func TestParseTarget(t *testing.T) {
 	t.Parallel()
 
diff --git a/internal/gitaly/client/dial.go b/internal/gitaly/client/dial.go
index 285056a36..1858e7310 100644
--- a/internal/gitaly/client/dial.go
+++ b/internal/gitaly/client/dial.go
@@ -8,6 +8,7 @@ import (
 	"net/url"
 	"time"
 
+	"gitlab.com/gitlab-org/gitaly/v15/internal/dnsresolver"
 	gitalyx509 "gitlab.com/gitlab-org/gitaly/v15/internal/x509"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	grpccorrelation "gitlab.com/gitlab-org/labkit/correlation/grpc"
@@ -26,6 +27,7 @@ const (
 	tcpConnection
 	tlsConnection
 	unixConnection
+	dnsConnection
 )
 
 func getConnectionType(rawAddress string) connectionType {
@@ -41,6 +43,8 @@ func getConnectionType(rawAddress string) connectionType {
 		return unixConnection
 	case "tcp":
 		return tcpConnection
+	case "dns":
+		return dnsConnection
 	default:
 		return invalidConnection
 	}
@@ -91,6 +95,13 @@ func Dial(ctx context.Context, rawAddress string, connOpts []grpc.DialOption, ha
 			return nil, fmt.Errorf("failed to extract host for 'tcp' connection: %w", err)
 		}
 
+	case dnsConnection:
+		err = dnsresolver.ValidateURL(rawAddress)
+		if err != nil {
+			return nil, fmt.Errorf("failed to parse target for 'dns' connection: %w", err)
+		}
+		canonicalAddress = rawAddress // DNS Resolver will handle this
+
 	case unixConnection:
 		canonicalAddress = rawAddress // This will be overridden by the custom dialer...
 		connOpts = append(