sqlproxyccl: close proxy connections properly#57306
sqlproxyccl: close proxy connections properly#57306craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
The sqlproxy opens a connection to the client and backend and proxies data between them. The backend connection is never closed. Moreover the client connection in certain situations may not be closed properly as well. The client connection may be embedded in a tls connection whose `Close` method will never be called. Details: Proxy connection to the client is closed in `func (s *Server) Serve` https://github.com/cockroachdb/cockroach/blob/master/pkg/ccl/sqlproxyccl/server.go#L150 However the client connection, if the proxy is using TLS, is further embedded in `tls.Conn` in func (s *Server) Proxy here https://github.com/cockroachdb/cockroach/blob/master/pkg/ccl/sqlproxyccl/proxy.go#L117. `tls.Conn` has a more involved `Close` implementation which will not be called; see https://golang.org/src/crypto/tls/conn.go. In particular, readers/writers on this connection may not be notified of the connection closure potentially leading to never exiting the go routines at https://github.com/cockroachdb/cockroach/blob/master/pkg/ccl/sqlproxyccl/proxy.go#L204. Note that this fix does not prevent another party from calling `Close` on a copy of `proxyConn` which may lead again to `tls.Conn Close` not being called if proxy is using TLS. Release note: none.
|
This change is |
jbowens
left a comment
There was a problem hiding this comment.
In particular,
readers/writers on this connection may not be notified of the connection
closure potentially leading to never exiting the go routines at
Is this true? My understanding is that it's not possible to leak goroutines here, because the (*tls.Conn).Read call will block in the underlying TCP connection's Read call, which errors out if the connection is closed.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @jbowens and @petermattis)
What do you mean by true? Are you positive that this could never happen? Unless we have an appropriate test for this I wouldn't rely on it and would instead make sure we call the appropriate |
|
TFTRs! |
|
bors r=jbowens |
|
Build succeeded: |
jbowens
left a comment
There was a problem hiding this comment.
What do you mean by true? Are you positive that this could never happen? Unless we have an appropriate test for this I wouldn't rely on it and would instead make sure we call the appropriate Close method in all scenarios.
By true I mean literally that it's not possible to leak a goroutine here according to my understanding.
More tests are always a good idea :) #57385
Reviewable status:
Previously, the sqlproxy failed to close the incoming TCP connection if an error occurred in the frontend admitter. This leaked the connection server-side and left the client connection hanging. This adds back the deferred close of the incoming TCP connection (removed in cockroachdb#57306). The custom Conn type allows callers to call Close multiple times. Release note: none
58355: sqlproxyccl: always close the client connection r=darinpp a=jbowens Previously, the sqlproxy failed to close the incoming TCP connection if an error occurred in the frontend admitter. This leaked the connection server-side and left the client connection hanging. This adds back the deferred close of the incoming TCP connection (removed in #57306). The custom Conn type allows callers to call Close multiple times. Release note: none 58374: rfcs: new RFC for the eventlog modernization r=knz a=knz Link to the text of the RFC: https://github.com/knz/cockroach/blob/20201230-eventlog-rfc/docs/RFCS/20201230_eventlog_modernization.md Release note: None Co-authored-by: Jackson Owens <jackson@cockroachlabs.com> Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
Issue:
The sqlproxy opens a connection to the client and backend and proxies
data between them. The backend connection is never closed. Moreover the
client connection in certain situations may not be closed properly as
well because it may be embedded in a tls connection whose
Closemethod will never be called.
Details:
Proxy connection to the client is closed in
func (s *Server) Servehttps://github.com/cockroachdb/cockroach/blob/master/pkg/ccl/sqlproxyccl/server.go#L149
However the client connection, if the proxy is using TLS, is further
embedded in
tls.Connin func (s *Server) Proxy herehttps://github.com/cockroachdb/cockroach/blob/master/pkg/ccl/sqlproxyccl/proxy.go#L117.
tls.Connhas a more involvedCloseimplementation which will not becalled; see https://golang.org/src/crypto/tls/conn.go. In particular,
readers/writers on this connection may not be notified of the connection
closure potentially leading to never exiting the go routines at
https://github.com/cockroachdb/cockroach/blob/master/pkg/ccl/sqlproxyccl/proxy.go#L204.
Note that this fix does not prevent another party from calling
Closeon
proxyConnwhich may lead again totls.Conn Closenot beingcalled when then proxy is using TLS.
Release note: none.