Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ccl/sqlproxyccl: remove the idle monitor component #81174

Merged
merged 1 commit into from
May 16, 2022
Merged

ccl/sqlproxyccl: remove the idle monitor component #81174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion pkg/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ ALL_TESTS = [
"//pkg/ccl/spanconfigccl/spanconfigsqlwatcherccl:spanconfigsqlwatcherccl_test",
"//pkg/ccl/sqlproxyccl/balancer:balancer_test",
"//pkg/ccl/sqlproxyccl/denylist:denylist_test",
"//pkg/ccl/sqlproxyccl/idle:idle_test",
"//pkg/ccl/sqlproxyccl/interceptor:interceptor_test",
"//pkg/ccl/sqlproxyccl/tenant:tenant_test",
"//pkg/ccl/sqlproxyccl/throttler:throttler_test",
Expand Down
1 change: 0 additions & 1 deletion pkg/ccl/sqlproxyccl/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ go_library(
"//pkg/base",
"//pkg/ccl/sqlproxyccl/balancer",
"//pkg/ccl/sqlproxyccl/denylist",
"//pkg/ccl/sqlproxyccl/idle",
"//pkg/ccl/sqlproxyccl/interceptor",
"//pkg/ccl/sqlproxyccl/tenant",
"//pkg/ccl/sqlproxyccl/tenantdirsvr",
Expand Down
18 changes: 0 additions & 18 deletions pkg/ccl/sqlproxyccl/connector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,16 +72,6 @@ type connector struct {
// NOTE: This field is optional.
TLSConfig *tls.Config

// IdleMonitorWrapperFn is used to wrap the connection to the SQL pod with
// an idle monitor. If not specified, the raw connection to the SQL pod
// will be returned.
//
// In the case of connecting with an authentication phase, the connection
// will be wrapped before starting the authentication.
//
// NOTE: This field is optional.
IdleMonitorWrapperFn func(serverConn net.Conn) net.Conn

// Testing knobs for internal connector calls. If specified, these will
// be called instead of the actual logic.
testingKnobs struct {
Expand Down Expand Up @@ -112,10 +102,6 @@ func (c *connector) OpenTenantConnWithToken(
}
}()

if c.IdleMonitorWrapperFn != nil {
serverConn = c.IdleMonitorWrapperFn(serverConn)
}

// When we use token-based authentication, we will still get the initial
// connection data messages (e.g. ParameterStatus and BackendKeyData).
// Since this method is only used during connection migration (i.e. proxy
Expand Down Expand Up @@ -161,10 +147,6 @@ func (c *connector) OpenTenantConnWithAuth(
}
}()

if c.IdleMonitorWrapperFn != nil {
serverConn = c.IdleMonitorWrapperFn(serverConn)
}

// Perform user authentication for non-token-based auth methods. This will
// block until the server has authenticated the client.
if err := authenticate(clientConn, serverConn, throttleHook); err != nil {
Expand Down
115 changes: 0 additions & 115 deletions pkg/ccl/sqlproxyccl/connector_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,59 +152,6 @@ func TestConnector_OpenTenantConnWithToken(t *testing.T) {
_, ok := c.StartupMsg.Parameters[sessionRevivalTokenStartupParam]
require.False(t, ok)
})

t.Run("idle monitor wrapper is called", func(t *testing.T) {
var wrapperCalled bool
f := &forwarder{}
c := &connector{
StartupMsg: &pgproto3.StartupMessage{
Parameters: make(map[string]string),
},
IdleMonitorWrapperFn: func(crdbConn net.Conn) net.Conn {
wrapperCalled = true
return crdbConn
},
}

conn, _ := net.Pipe()
defer conn.Close()

var openCalled bool
c.testingKnobs.dialTenantCluster = func(
ctx context.Context, requester balancer.ConnectionHandle,
) (net.Conn, error) {
require.Equal(t, f, requester)
openCalled = true

// Validate that token is set.
str, ok := c.StartupMsg.Parameters[sessionRevivalTokenStartupParam]
require.True(t, ok)
require.Equal(t, token, str)

return conn, nil
}

var authCalled bool
defer testutils.TestingHook(
&readTokenAuthResult,
func(serverConn net.Conn) error {
authCalled = true
require.Equal(t, conn, serverConn)
return nil
},
)()

crdbConn, err := c.OpenTenantConnWithToken(ctx, f, token)
require.True(t, wrapperCalled)
require.True(t, openCalled)
require.True(t, authCalled)
require.NoError(t, err)
require.Equal(t, conn, crdbConn)

// Ensure that token is deleted.
_, ok := c.StartupMsg.Parameters[sessionRevivalTokenStartupParam]
require.False(t, ok)
})
}

func TestConnector_OpenTenantConnWithAuth(t *testing.T) {
Expand Down Expand Up @@ -334,68 +281,6 @@ func TestConnector_OpenTenantConnWithAuth(t *testing.T) {
require.False(t, sentToClient)
require.Equal(t, serverConn, crdbConn)
})

t.Run("idle monitor wrapper is called", func(t *testing.T) {
clientConn, _ := net.Pipe()
defer clientConn.Close()

serverConn, _ := net.Pipe()
defer serverConn.Close()

var wrapperCalled bool
f := &forwarder{}
c := &connector{
StartupMsg: &pgproto3.StartupMessage{
Parameters: map[string]string{
// Passing in a token should have no effect.
sessionRevivalTokenStartupParam: "foo",
},
},
IdleMonitorWrapperFn: func(crdbConn net.Conn) net.Conn {
wrapperCalled = true
return crdbConn
},
}

var openCalled bool
c.testingKnobs.dialTenantCluster = func(
ctx context.Context, requester balancer.ConnectionHandle,
) (net.Conn, error) {
require.Equal(t, f, requester)
openCalled = true

// Validate that token is not set.
_, ok := c.StartupMsg.Parameters[sessionRevivalTokenStartupParam]
require.False(t, ok)

return serverConn, nil
}

var authCalled bool
defer testutils.TestingHook(
&authenticate,
func(
client net.Conn,
server net.Conn,
throttleHook func(status throttler.AttemptStatus) error,
) error {
authCalled = true
require.Equal(t, clientConn, client)
require.NotNil(t, server)
require.Equal(t, reflect.ValueOf(dummyHook).Pointer(),
reflect.ValueOf(throttleHook).Pointer())
return nil
},
)()

crdbConn, sentToClient, err := c.OpenTenantConnWithAuth(ctx, f, clientConn, dummyHook)
require.True(t, openCalled)
require.True(t, wrapperCalled)
require.True(t, authCalled)
require.NoError(t, err)
require.False(t, sentToClient)
require.Equal(t, serverConn, crdbConn)
})
}

func TestConnector_dialTenantCluster(t *testing.T) {
Expand Down
8 changes: 0 additions & 8 deletions pkg/ccl/sqlproxyccl/error.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,6 @@ const (
// or vice versa.
codeUnexpectedInsecureStartupMessage

// codeSNIRoutingFailed indicates an error choosing a backend address based on
// the client's SNI header.
codeSNIRoutingFailed

// codeUnexpectedStartupMessage indicates an unexpected startup message
// received from the client after TLS negotiation.
codeUnexpectedStartupMessage
Expand Down Expand Up @@ -77,10 +73,6 @@ const (
// has expired and should be closed.
codeExpiredClientConnection

// codeIdleDisconnect indicates that the connection was disconnected for
// being idle for longer than the specified timeout.
codeIdleDisconnect

// codeUnavailable indicates that the backend SQL server exists but is not
// accepting connections. For example, a tenant cluster that has maxPods set to 0.
codeUnavailable
Expand Down
24 changes: 11 additions & 13 deletions pkg/ccl/sqlproxyccl/errorcode_string.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

29 changes: 0 additions & 29 deletions pkg/ccl/sqlproxyccl/idle/BUILD.bazel
View file
Open in desktop

This file was deleted.

Loading