Merge #66369

66369: sqlproxy: fix some small issues and tidy up files and comments r=andy-kimball a=andy-kimball

See commit comments for details.

Co-authored-by: Andrew Kimball <[email protected]>

pkg/BUILD.bazel

@@ -26,7 +26,7 @@ ALL_TESTS = [
     "//pkg/ccl/sqlproxyccl/cache:cache_test",
     "//pkg/ccl/sqlproxyccl/denylist:denylist_test",
     "//pkg/ccl/sqlproxyccl/idle:idle_test",
-    "//pkg/ccl/sqlproxyccl/tenantdirsvr:tenantdirsvr_test",
+    "//pkg/ccl/sqlproxyccl/tenant:tenant_test",
     "//pkg/ccl/sqlproxyccl/throttler:throttler_test",
     "//pkg/ccl/sqlproxyccl:sqlproxyccl_test",
     "//pkg/ccl/storageccl/engineccl:engineccl_test",

pkg/ccl/sqlproxyccl/BUILD.bazel

@@ -38,6 +38,8 @@ go_library(
         "@com_github_cockroachdb_logtags//:logtags",
         "@com_github_jackc_pgproto3_v2//:pgproto3",
         "@org_golang_google_grpc//:go_default_library",
+        "@org_golang_google_grpc//codes",
+        "@org_golang_google_grpc//status",
     ],
 )
 
@@ -51,17 +53,13 @@ go_test(
         "proxy_handler_test.go",
         "server_test.go",
     ],
-    data = [
-        ":testserver.crt",
-        ":testserver.key",
-        ":testserver_config.cnf",
-    ],
+    data = [":testdata"],
     embed = [":sqlproxyccl"],
     deps = [
         "//pkg/base",
         "//pkg/ccl/kvccl/kvtenantccl",
         "//pkg/ccl/sqlproxyccl/denylist",
-        "//pkg/ccl/sqlproxyccl/tenant",
+        "//pkg/ccl/sqlproxyccl/tenantdirsvr",
         "//pkg/ccl/utilccl",
         "//pkg/roachpb",
         "//pkg/security",

pkg/ccl/sqlproxyccl/backend_dialer.go

@@ -42,8 +42,8 @@ var backendDial = func(
 	return conn, nil
 }
 
-// sslOverlay attempts to upgrade the PG connection to use SSL
-// if a tls.Config is specified..
+// sslOverlay attempts to upgrade the PG connection to use SSL if a tls.Config
+// is specified.
 func sslOverlay(conn net.Conn, tlsConfig *tls.Config) (net.Conn, error) {
 	if tlsConfig == nil {
 		return conn, nil

pkg/ccl/sqlproxyccl/cache/cache.go

@@ -39,7 +39,8 @@ type cappedConnCache struct {
 	}
 }
 
-// NewCappedConnCache returns a cache service that has a limit on the total entries.
+// NewCappedConnCache returns a cache service that has a limit on the total
+// entries.
 func NewCappedConnCache(maxMapSize int) ConnCache {
 	c := &cappedConnCache{
 		maxMapSize: maxMapSize,

pkg/ccl/sqlproxyccl/denylist/file.go

@@ -70,11 +70,12 @@ type Denylist struct {
 	ctx context.Context
 }
 
-// NewDenylistWithFile returns a new denylist that automatically watches updates to a file.
-// Note: this currently does not return an error. This is by design, since even if we trouble
-// initiating a denylist with file, we can always update the file with correct content during
-// runtime. We don't want sqlproxy fail to start just because there's something wrong with
-// contents of a denylist file.
+// NewDenylistWithFile returns a new denylist that automatically watches updates
+// to a file.
+// Note: this currently does not return an error. This is by design, since even
+// if we trouble initiating a denylist with file, we can always update the file
+// with correct content during runtime. We don't want sqlproxy fail to start
+// just because there's something wrong with contents of a denylist file.
 func NewDenylistWithFile(ctx context.Context, filename string, opts ...Option) *Denylist {
 	ret := &Denylist{
 		pollingInterval: defaultPollingInterval,
@@ -89,9 +90,10 @@ func NewDenylistWithFile(ctx context.Context, filename string, opts ...Option) *
 	}
 	err := ret.update(filename)
 	if err != nil {
-		// don't return just yet; sqlproxy should be able to carry on without a proper denylist
-		// and we still have a chance to recover.
-		// TODO(ye): add monitoring for failed updates; we don't want silent failures
+		// don't return just yet; sqlproxy should be able to carry on without a
+		// proper denylist and we still have a chance to recover.
+		// TODO(ye): add monitoring for failed updates; we don't want silent
+		// failures.
 		log.Errorf(ctx, "error when reading from file %s: %v", filename, err)
 	}
 
@@ -103,7 +105,8 @@ func NewDenylistWithFile(ctx context.Context, filename string, opts ...Option) *
 // Option allows configuration of a denylist service.
 type Option func(*Denylist)
 
-// WithPollingInterval specifies interval between polling for config file changes.
+// WithPollingInterval specifies interval between polling for config file
+// changes.
 func WithPollingInterval(d time.Duration) Option {
 	return func(dl *Denylist) {
 		dl.pollingInterval = d
@@ -163,7 +166,8 @@ func (dl *Denylist) Denied(entity DenyEntity) (*Entry, error) {
 func (dl *Denylist) watchForUpdate(filename string) {
 	go func() {
 		// TODO(ye): use notification via SIGHUP instead.
-		// TODO(ye): use inotify or similar mechanism for watching file updates instead of polling.
+		// TODO(ye): use inotify or similar mechanism for watching file updates
+		// instead of polling.
 		t := timeutil.NewTimer()
 		defer t.Stop()
 		for {

pkg/ccl/sqlproxyccl/denylist/service.go

@@ -10,8 +10,6 @@ package denylist
 
 import "strings"
 
-//go:generate mockgen -package=denylist -destination=mocks_generated.go -source=service.go . Service
-
 // Entry records the reason for putting an item on the denylist.
 // TODO(spaskob): add codes for different denial reasons.
 type Entry struct {

pkg/ccl/sqlproxyccl/frontend_admitter.go

@@ -16,11 +16,11 @@ import (
 )
 
 // frontendAdmit is the default implementation of a frontend admitter. It can
-// upgrade to an optional SSL connection, and will handle and verify
-// the startup message received from the PG SQL client.
-// The connection returned should never be nil in case of error. Depending
-// on whether the error happened before the connection was upgraded to TLS or not
-// it will either be the original or the TLS connection.
+// upgrade to an optional SSL connection, and will handle and verify the startup
+// message received from the PG SQL client. The connection returned should never
+// be nil in case of error. Depending on whether the error happened before the
+// connection was upgraded to TLS or not it will either be the original or the
+// TLS connection.
 var frontendAdmit = func(
 	conn net.Conn, incomingTLSConfig *tls.Config,
 ) (net.Conn, *pgproto3.StartupMessage, error) {
@@ -38,9 +38,9 @@ var frontendAdmit = func(
 		switch m.(type) {
 		case *pgproto3.SSLRequest:
 		case *pgproto3.CancelRequest:
-			// Ignore CancelRequest explicitly. We don't need to do this but it makes
-			// testing easier by avoiding a call to sendErrToClient on this path
-			// (which would confuse assertCtx).
+			// Ignore CancelRequest explicitly. We don't need to do this but it
+			// makes testing easier by avoiding a call to sendErrToClient on this
+			// path (which would confuse assertCtx).
 			return conn, nil, nil
 		default:
 			code := codeUnexpectedInsecureStartupMessage

pkg/ccl/sqlproxyccl/frontend_admitter_test.go

@@ -23,7 +23,7 @@ import (
 )
 
 func tlsConfig() (*tls.Config, error) {
-	cer, err := tls.LoadX509KeyPair("testserver.crt", "testserver.key")
+	cer, err := tls.LoadX509KeyPair("testdata/testserver.crt", "testdata/testserver.key")
 	if err != nil {
 		return nil, err
 	}

pkg/ccl/sqlproxyccl/idle/idle_disconnect_connection_test.go

@@ -82,7 +82,8 @@ func benchmarkSocketRead(timeout time.Duration, b *testing.B) {
 }
 
 // No statistically significant difference in a single roundtrip time between
-// using and not using deadline as implemented above. Both show the same value in my tests.
+// using and not using deadline as implemented above. Both show the same value
+// in my tests.
 // SocketReadWithDeadline-32     11.1µs ± 1%
 // SocketReadWithoutDeadline-32  11.0µs ± 3%
 func BenchmarkSocketReadWithoutDeadline(b *testing.B) {

pkg/ccl/sqlproxyccl/metrics.go

@@ -13,8 +13,7 @@ import (
 	"github.com/cockroachdb/errors"
 )
 
-// metrics contains pointers to the metrics for monitoring proxy
-// operations.
+// metrics contains pointers to the metrics for monitoring proxy operations.
 type metrics struct {
 	BackendDisconnectCount *metric.Counter
 	IdleDisconnectCount    *metric.Counter
@@ -112,8 +111,8 @@ func makeProxyMetrics() metrics {
 	}
 }
 
-// updateForError updates the metrics relevant for the type of the
-// error message.
+// updateForError updates the metrics relevant for the type of the error
+// message.
 func (metrics *metrics) updateForError(err error) {
 	if err == nil {
 		return

pkg/ccl/sqlproxyccl/proxy.go

@@ -87,10 +87,10 @@ func connectionCopy(crdbConn, conn net.Conn) error {
 	select {
 	// NB: when using pgx, we see a nil errIncoming first on clean connection
 	// termination. Using psql I see a nil errOutgoing first. I think the PG
-	// protocol stipulates sending a message to the server at which point
-	// the server closes the connection (errIncoming), but presumably the
-	// client gets to close the connection once it's sent that message,
-	// meaning either case is possible.
+	// protocol stipulates sending a message to the server at which point the
+	// server closes the connection (errIncoming), but presumably the client
+	// gets to close the connection once it's sent that message, meaning either
+	// case is possible.
 	case err := <-errIncoming:
 		if err == nil {
 			return nil