diff options
author | John Cai <jcai@gitlab.com> | 2023-02-09 20:34:36 +0300 |
---|---|---|
committer | John Cai <jcai@gitlab.com> | 2023-02-09 20:34:36 +0300 |
commit | 8fd20f981d3918e505b6c6a88ca6e734bebfbce9 (patch) | |
tree | e87b5ab54a73d9f162b34d641002d49640a2b82a | |
parent | 3775aeedcfa431d056648a3ef15ef2d0af46d21a (diff) | |
parent | 21e5a03bdd8c8b3845091d28a77ed890d23ed1ab (diff) |
Merge branch 'qmnguyen0711/add-dial-support-to-client-dials' into 'master'
dnsresolver: Support DNS scheme in client dial functions
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5367
Merged-by: John Cai <jcai@gitlab.com>
Approved-by: John Cai <jcai@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
-rw-r--r-- | client/dial_test.go | 114 | ||||
-rw-r--r-- | internal/dnsresolver/target.go | 30 | ||||
-rw-r--r-- | internal/dnsresolver/target_test.go | 42 | ||||
-rw-r--r-- | internal/gitaly/client/dial.go | 11 |
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( |