From 65437ffef30f51bc5a68b9af13bf9e396986dcf4 Mon Sep 17 00:00:00 2001 From: Jakub Nyckowski Date: Wed, 9 Mar 2022 12:49:36 -0500 Subject: [PATCH] Update go-mysql package (#10984) Send COM_QUIT message when closing MySQL connection. --- go.mod | 2 +- go.sum | 4 +-- lib/srv/db/access_test.go | 32 ++++++++++++++++++- lib/srv/db/mysql/test.go | 25 +++++++++++++++ lib/srv/db/proxy_test.go | 4 ++- .../siddontang/go-mysql/client/conn.go | 5 +++ vendor/modules.txt | 2 +- 7 files changed, 68 insertions(+), 6 deletions(-) diff --git a/go.mod b/go.mod index 15a22024e4223..b55f0496634f5 100644 --- a/go.mod +++ b/go.mod @@ -213,6 +213,6 @@ replace ( github.com/coreos/go-oidc => github.com/gravitational/go-oidc v0.0.5 github.com/gogo/protobuf => github.com/gravitational/protobuf v1.3.2-0.20201123192827-2b9fcfaffcbf github.com/gravitational/teleport/api => ./api - github.com/siddontang/go-mysql v1.1.0 => github.com/gravitational/go-mysql v1.1.1-teleport.1 + github.com/siddontang/go-mysql v1.1.0 => github.com/gravitational/go-mysql v1.1.1-teleport.2 github.com/sirupsen/logrus => github.com/gravitational/logrus v1.4.4-0.20210817004754-047e20245621 ) diff --git a/go.sum b/go.sum index c362eae0cba3c..34dd452dab847 100644 --- a/go.sum +++ b/go.sum @@ -394,8 +394,8 @@ github.com/gravitational/configure v0.0.0-20180808141939-c3428bd84c23 h1:havbccu github.com/gravitational/configure v0.0.0-20180808141939-c3428bd84c23/go.mod h1:XL9nebvlfNVvRzRPWdDcWootcyA0l7THiH/A+W1233g= github.com/gravitational/form v0.0.0-20151109031454-c4048f792f70 h1:To76nCJtM3DI0mdq3nGLzXqTV1wNOJByxv01+u9/BxM= github.com/gravitational/form v0.0.0-20151109031454-c4048f792f70/go.mod h1:88hFR45MpUd23d2vNWE/dYtesU50jKsbz0I9kH7UaBY= -github.com/gravitational/go-mysql v1.1.1-teleport.1 h1:062V8u0juCyUvpYMdkYch8JDDw7wf5rdhKaIfhnojDg= -github.com/gravitational/go-mysql v1.1.1-teleport.1/go.mod h1:re0JQZ1Cy5dVlIDGq0YksfDIla/GRZlxqOoC0XPSSGE= +github.com/gravitational/go-mysql v1.1.1-teleport.2 h1:XZ36BZ7BgslA5ZCyCHjpc1wilFITThIH7cLcbLWKWzM= +github.com/gravitational/go-mysql v1.1.1-teleport.2/go.mod h1:re0JQZ1Cy5dVlIDGq0YksfDIla/GRZlxqOoC0XPSSGE= github.com/gravitational/go-oidc v0.0.5 h1:kxsCknoOZ+KqIAoYLLdHuQcvcc+SrQlnT7xxIM8oo6o= github.com/gravitational/go-oidc v0.0.5/go.mod h1:SevmOUNdOB0aD9BAIgjptZ6oHkKxMZZgA70nwPfgU/w= github.com/gravitational/kingpin v2.1.11-0.20190130013101-742f2714c145+incompatible h1:CfyZl3nyo9K5lLqOmqvl9/IElY1UCnOWKZiQxJ8HKdA= diff --git a/lib/srv/db/access_test.go b/lib/srv/db/access_test.go index 84b8ec2f7bbf3..6ea21c2bb95e4 100644 --- a/lib/srv/db/access_test.go +++ b/lib/srv/db/access_test.go @@ -337,6 +337,31 @@ func TestAccessMySQLChangeUser(t *testing.T) { require.Error(t, err) } +func TestMySQLCloseConnection(t *testing.T) { + ctx := context.Background() + testCtx := setupTestContext(ctx, t, withSelfHostedMySQL("mysql")) + go testCtx.startHandlingConnections() + + // Create user/role with the requested permissions. + testCtx.createUserAndRole(ctx, t, "alice", "admin", []string{"alice"}, []string{types.Wildcard}) + + // Connect to the database as this user. + mysqlConn, err := testCtx.mysqlClient("alice", "mysql", "alice") + require.NoError(t, err) + + _, err = mysqlConn.Execute("select 1") + require.NoError(t, err) + + // Close connection to DB proxy + err = mysqlConn.Close() + require.NoError(t, err) + + // DB proxy should close the DB connection and send COM_QUIT message. + require.Eventually(t, func() bool { + return testCtx.mysql["mysql"].db.ConnsClosed() + }, 2*time.Second, 100*time.Millisecond) +} + // TestAccessMySQLServerPacket verifies some edge-cases related to reading // wire packets sent by the MySQL server. func TestAccessMySQLServerPacket(t *testing.T) { @@ -355,6 +380,9 @@ func TestAccessMySQLServerPacket(t *testing.T) { // in a way that previously would cause our packet parsing logic to fail. _, err = mysqlConn.Execute("show tables") require.NoError(t, err) + + err = mysqlConn.Close() + require.NoError(t, err) } // TestGCPRequireSSL tests connecting to GCP Cloud SQL Postgres and MySQL @@ -1313,7 +1341,9 @@ func withSelfHostedMySQL(name string) withDatabaseOption { }) require.NoError(t, err) go mysqlServer.Serve() - t.Cleanup(func() { mysqlServer.Close() }) + t.Cleanup(func() { + require.NoError(t, mysqlServer.Close()) + }) database, err := types.NewDatabaseV3(types.Metadata{ Name: name, }, types.DatabaseSpecV3{ diff --git a/lib/srv/db/mysql/test.go b/lib/srv/db/mysql/test.go index 18b4ae22db6c7..520bdc8dc31de 100644 --- a/lib/srv/db/mysql/test.go +++ b/lib/srv/db/mysql/test.go @@ -19,6 +19,7 @@ package mysql import ( "crypto/tls" "net" + "sync" "sync/atomic" "github.com/gravitational/teleport/lib/defaults" @@ -62,6 +63,11 @@ type TestServer struct { tlsConfig *tls.Config log logrus.FieldLogger handler *testHandler + + // serverConnsMtx is a mutex that guards serverConns. + serverConnsMtx sync.Mutex + // serverConns holds all connections created by the server. + serverConns []*server.Conn } // NewTestServer returns a new instance of a test MySQL server. @@ -152,6 +158,11 @@ func (s *TestServer) handleConnection(conn net.Conn) error { if err != nil { return trace.Wrap(err) } + + s.serverConnsMtx.Lock() + s.serverConns = append(s.serverConns, serverConn) + s.serverConnsMtx.Unlock() + for { if serverConn.Closed() { return nil @@ -196,6 +207,20 @@ func (s *TestServer) Close() error { return s.listener.Close() } +// ConnsClosed returns true if all connections has been correctly closed (message COM_QUIT), false otherwise. +func (s *TestServer) ConnsClosed() bool { + s.serverConnsMtx.Lock() + defer s.serverConnsMtx.Unlock() + + for _, conn := range s.serverConns { + if !conn.Closed() { + return false + } + } + + return true +} + type testHandler struct { server.EmptyHandler log logrus.FieldLogger diff --git a/lib/srv/db/proxy_test.go b/lib/srv/db/proxy_test.go index db79c36b9873e..46fecc06618d9 100644 --- a/lib/srv/db/proxy_test.go +++ b/lib/srv/db/proxy_test.go @@ -171,7 +171,9 @@ func TestProxyClientDisconnectDueToLockInForce(t *testing.T) { Target: types.LockTarget{User: "alice"}, }) require.NoError(t, err) - testCtx.authServer.UpsertLock(ctx, lock) + + err = testCtx.authServer.UpsertLock(ctx, lock) + require.NoError(t, err) waitForEvent(t, testCtx, events.ClientDisconnectCode) err = mysql.Ping() diff --git a/vendor/github.com/siddontang/go-mysql/client/conn.go b/vendor/github.com/siddontang/go-mysql/client/conn.go index e26680f16a737..5d7845aa1fe7a 100644 --- a/vendor/github.com/siddontang/go-mysql/client/conn.go +++ b/vendor/github.com/siddontang/go-mysql/client/conn.go @@ -102,6 +102,11 @@ func (c *Conn) handshake() error { } func (c *Conn) Close() error { + if err := c.writeCommand(COM_QUIT); err != nil { + c.Conn.Close() + return errors.Trace(err) + } + return c.Conn.Close() } diff --git a/vendor/modules.txt b/vendor/modules.txt index a9e35dd0607c2..8a4e6aadb6d28 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -694,7 +694,7 @@ github.com/siddontang/go/sync2 ## explicit github.com/siddontang/go-log/log github.com/siddontang/go-log/loggers -# github.com/siddontang/go-mysql v1.1.0 => github.com/gravitational/go-mysql v1.1.1-teleport.1 +# github.com/siddontang/go-mysql v1.1.0 => github.com/gravitational/go-mysql v1.1.1-teleport.2 ## explicit; go 1.15 github.com/siddontang/go-mysql/client github.com/siddontang/go-mysql/mysql