Merge #66513

66513: sqlproxyccl: fix DNS resolution logic in backendLookupAddr r=andy-kimball a=jaylim-crl

Previously, we were using net.LookupAddr in backendLookupAddr, but that was
incorrect since [net.LookupAddr](https://golang.org/pkg/net/#LookupAddr) is meant to perform a reverse lookup. The
intention was to resolve the address (e.g. foo-bar-2-0.cockroachdb:80) to an
IP. This patch fixes that by using base.LookupAddr instead, which will resolve
the given address/host to an IP address.

Note that the tests added are not exhaustive since the code in
backendLookupAddr will eventually go away.

Release note: None

Co-authored-by: Jay Lim <[email protected]>

pkg/ccl/sqlproxyccl/BUILD.bazel

@@ -17,6 +17,7 @@ go_library(
     importpath = "github.com/cockroachdb/cockroach/pkg/ccl/sqlproxyccl",
     visibility = ["//visibility:public"],
     deps = [
+        "//pkg/base",
         "//pkg/ccl/sqlproxyccl/cache",
         "//pkg/ccl/sqlproxyccl/denylist",
         "//pkg/ccl/sqlproxyccl/idle",
@@ -48,6 +49,7 @@ go_test(
     size = "small",
     srcs = [
         "authentication_test.go",
+        "backend_dialer_test.go",
         "frontend_admitter_test.go",
         "main_test.go",
         "proxy_handler_test.go",

pkg/ccl/sqlproxyccl/backend_dialer.go

@@ -9,26 +9,45 @@
 package sqlproxyccl
 
 import (
+	"context"
 	"crypto/tls"
 	"encoding/binary"
+	"fmt"
 	"io"
 	"net"
 
+	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/errors"
 	"github.com/jackc/pgproto3/v2"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 )
 
 // backendLookupAddr looks up the given address using usual DNS resolution
 // mechanisms. It returns the first resolved address, or an error if the lookup
 // failed in some way. This can be overridden in tests.
-var backendLookupAddr = func(addr string) (string, error) {
-	names, err := net.LookupAddr(addr)
-	if err != nil || len(names) == 0 {
+var backendLookupAddr = func(ctx context.Context, addr string) (string, error) {
+	// Address must have a port. SplitHostPort returns an error if the address
+	// does not have a port.
+	host, port, err := net.SplitHostPort(addr)
+	if err != nil {
+		return "", err
+	}
+	if host == "" {
+		return "", fmt.Errorf("no host was provided for '%s'", addr)
+	}
+	if port == "" {
+		return "", fmt.Errorf("no port was provided for '%s'", addr)
+	}
+
+	// Note that LookupAddr might return an IPv6 address if no IPv4 addresses
+	// are found. We will punt on that since this function will go away soon,
+	// and we don't currently use IPv6.
+	ip, err := base.LookupAddr(ctx, net.DefaultResolver, host)
+	if err != nil {
 		// Assume that any errors are due to missing or mismatched tenant.
-		return "", status.Errorf(codes.NotFound, "DNS lookup failed for '%s': %v", addr, err)
+		return "", errors.Wrapf(err, "DNS lookup failed for '%s'", addr)
 	}
-	return names[0], nil
+
+	return net.JoinHostPort(ip, port), nil
 }
 
 // backendDial is an example backend dialer that does a TCP/IP connection

pkg/ccl/sqlproxyccl/backend_dialer_test.go

@@ -0,0 +1,63 @@
+// Copyright 2021 The Cockroach Authors.
+//
+// Licensed as a CockroachDB Enterprise file under the Cockroach Community
+// License (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+//     https://github.com/cockroachdb/cockroach/blob/master/licenses/CCL.txt
+
+package sqlproxyccl
+
+import (
+	"context"
+	"fmt"
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/testutils"
+	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+)
+
+func TestBackendLookupAddr(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	ctx := context.Background()
+
+	// Note that this list isn't exhaustive. We only cover the common cases.
+	// backendLookupAddr is only temporary, and will go away soon.
+	testData := []struct {
+		in          string
+		expected    string
+		expectedErr string
+	}{
+		// Invalid routing rules.
+		{"foobar", "", "missing port in address"},
+		{"foobar:", "", "no port was provided for 'foobar:'"},
+		{":80", "", "no host was provided for ':80'"},
+		{":", "", "no host was provided for ':'"},
+
+		// Invalid hosts.
+		{"nonexistent.example.com:26257", "", "no such host"},
+		{"333.333.333.333:26257", "", "no such host"},
+
+		// Valid hosts.
+		{"127.0.0.1:80", "127.0.0.1:80", ""},
+		{"localhost:80", "127.0.0.1:80", ""},
+	}
+	for i, test := range testData {
+		t.Run(fmt.Sprintf("%d/%s", i, test.in), func(t *testing.T) {
+			addr, err := backendLookupAddr(ctx, test.in)
+			if err != nil {
+				if !testutils.IsError(err, test.expectedErr) {
+					t.Fatalf("expected error %q, got %v", test.expectedErr, err)
+				}
+				return
+			}
+			if test.expectedErr != "" {
+				t.Fatalf("expected error %q, got success", test.expectedErr)
+			}
+			if addr != test.expected {
+				t.Fatalf("expected %q,\ngot %q", test.expected, addr)
+			}
+		})
+	}
+}

pkg/ccl/sqlproxyccl/proxy_handler.go

@@ -267,6 +267,7 @@ func (handler *proxyHandler) handle(ctx context.Context, incomingConn *proxyConn
 			}
 
 			// Remap error for external consumption.
+			log.Errorf(ctx, "could not retrieve outgoing address: %v", err.Error())
 			err = newErrorf(
 				codeParamsRoutingFailed, "cluster %s-%d not found", clusterName, tenID.ToUint64())
 			break
@@ -417,7 +418,7 @@ func (handler *proxyHandler) outgoingAddress(
 	addr := strings.ReplaceAll(
 		handler.RoutingRule, "{{clusterName}}", fmt.Sprintf("%s-%d", name, tenID.ToUint64()),
 	)
-	_, err := backendLookupAddr(addr)
+	_, err := backendLookupAddr(ctx, addr)
 	if err != nil {
 		return "", status.Error(codes.NotFound, err.Error())
 	}

pkg/ccl/sqlproxyccl/proxy_handler_test.go

@@ -90,7 +90,7 @@ func TestBackendDownRetry(t *testing.T) {
 	defer te.Close()
 
 	callCount := 0
-	defer testutils.TestingHook(&backendLookupAddr, func(addr string) (string, error) {
+	defer testutils.TestingHook(&backendLookupAddr, func(_ context.Context, addr string) (string, error) {
 		callCount++
 		if callCount >= 3 {
 			return "", errors.New("tenant not found")
@@ -559,7 +559,7 @@ func TestDirectoryConnect(t *testing.T) {
 	_, addr := newProxyServer(ctx, t, srv.Stopper(), opts)
 
 	t.Run("fallback when tenant not found", func(t *testing.T) {
-		defer testutils.TestingHook(&backendLookupAddr, func(addr string) (string, error) {
+		defer testutils.TestingHook(&backendLookupAddr, func(_ context.Context, addr string) (string, error) {
 			// Expect fallback.
 			require.Equal(t, srv.ServingSQLAddr(), addr)
 			return addr, nil
@@ -864,9 +864,9 @@ type tester struct {
 func newTester() *tester {
 	te := &tester{}
 
-	// Override default lookup function so that it does not use net.LookupAddr.
+	// Override default lookup function so that it does not use base.LookupAddr.
 	te.restoreBackendLookupAddr =
-		testutils.TestingHook(&backendLookupAddr, func(addr string) (string, error) {
+		testutils.TestingHook(&backendLookupAddr, func(ctx context.Context, addr string) (string, error) {
 			return addr, nil
 		})
 

pkg/cli/cliflags/flags_mt.go

@@ -50,8 +50,10 @@ var (
 	}
 
 	RoutingRule = FlagInfo{
-		Name:        "routing-rule",
-		Description: "Routing rule for incoming connections. Use '{{clusterName}}' for substitution.",
+		Name: "routing-rule",
+		Description: `
+Routing rule for incoming connections. Use '{{clusterName}}' for substitution.
+This rule must include the port of the SQL pod.`,
 	}
 
 	DirectoryAddr = FlagInfo{