-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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.
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
sqlproxyccl: add proxy protocol listener #117241
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,9 +56,20 @@ func runStartSQLProxy(cmd *cobra.Command, args []string) (returnErr error) { | |
|
||
log.Infof(ctx, "New proxy with opts: %+v", proxyContext) | ||
|
||
proxyLn, err := net.Listen("tcp", proxyContext.ListenAddr) | ||
if err != nil { | ||
return err | ||
var proxyLn net.Listener | ||
if proxyContext.ListenAddr != "" { | ||
proxyLn, err = net.Listen("tcp", proxyContext.ListenAddr) | ||
if err != nil { | ||
return err | ||
} | ||
} | ||
|
||
var proxyProtocolLn net.Listener | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to Jay's request for tests. That may be easier if you move most of this logic into the pkg/ccl/sqlproxyccl package that contains the majority of the sql proxy code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've moved some logic around and added a test for how the main listener and the proxy protocol listener interact with the presence or absence of proxy headers. PTAL! |
||
if proxyContext.ProxyProtocolListenAddr != "" { | ||
proxyProtocolLn, err = net.Listen("tcp", proxyContext.ProxyProtocolListenAddr) | ||
if err != nil { | ||
return err | ||
} | ||
} | ||
|
||
metricsLn, err := net.Listen("tcp", proxyContext.MetricsAddress) | ||
|
@@ -84,15 +95,14 @@ func runStartSQLProxy(cmd *cobra.Command, args []string) (returnErr error) { | |
} | ||
|
||
if err := stopper.RunAsyncTask(ctx, "serve-proxy", func(ctx context.Context) { | ||
log.Infof(ctx, "proxy server listening at %s", proxyLn.Addr()) | ||
if err := server.Serve(ctx, proxyLn); err != nil { | ||
if err := server.ServeSQL(ctx, proxyLn, proxyProtocolLn); err != nil { | ||
errChan <- err | ||
} | ||
}); err != nil { | ||
return err | ||
} | ||
|
||
return waitForSignals(ctx, server, stopper, proxyLn, errChan) | ||
return waitForSignals(ctx, server, stopper, proxyLn, proxyProtocolLn, errChan) | ||
} | ||
|
||
func initLogging(cmd *cobra.Command) (ctx context.Context, stopper *stop.Stopper, err error) { | ||
|
@@ -110,6 +120,7 @@ func waitForSignals( | |
server *sqlproxyccl.Server, | ||
stopper *stop.Stopper, | ||
proxyLn net.Listener, | ||
proxyProtocolLn net.Listener, | ||
errChan chan error, | ||
) (returnErr error) { | ||
// Need to alias the signals if this has to run on non-unix OSes too. | ||
|
@@ -139,7 +150,12 @@ func waitForSignals( | |
// waiting for "shutdownConnectionTimeout" to elapse after which | ||
// open TCP connections will be forcefully closed so the server can stop | ||
log.Infof(ctx, "stopping tcp listener") | ||
_ = proxyLn.Close() | ||
if proxyLn != nil { | ||
_ = proxyLn.Close() | ||
} | ||
if proxyProtocolLn != nil { | ||
_ = proxyProtocolLn.Close() | ||
} | ||
select { | ||
case <-server.AwaitNoConnections(ctx): | ||
case <-time.After(shutdownConnectionTimeout): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The combination of flags here is a bit redundant. What do you think of making the existing --listen-addr flag as optional and adding a TODO to delete the require proxy protocol flag once we migrate everything over to --proxy-protocol?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!