fix: support MySQL driver’s conn check. #226
Conversation
dialer.go
Outdated
| // SyscallConn supports a connection check in the MySQL driver by delegating to | ||
| // the underlying non-TLS net.Conn. | ||
| func (i *instrumentedConn) SyscallConn() (syscall.RawConn, error) { | ||
| sconn := i.rawConn.(syscall.Conn) |
There was a problem hiding this comment.
Good question. I'll add a unit test to make sure we have support in either case.
There was a problem hiding this comment.
Looks like the MySQL driver doesn't have support for Windows here: https://github.com/go-sql-driver/mysql/blob/ad9fa14acdcf7d0533e7fbe58728f3d216213ade/conncheck_dummy.go#L9. What would your expectation be?
There was a problem hiding this comment.
I've updated this so it won't panic if the type assertion fails. Separately, this builds on Windows with the unit test, so I think we're good.
There was a problem hiding this comment.
I would just expect it not to panic. (Parity to go mysql should be good)
humm I don't think returning an error will work with go mysql's current implementation. it will always trigger driver.ErrBadConn.
func (mc *mysqlConn) writePacket(data []byte) error {
...
if err == nil && mc.cfg.CheckConnLiveness {
err = connCheck(conn)
}
if err != nil {
errLog.Print("closing bad idle connection: ", err)
mc.Close()
return driver.ErrBadConn
}
...
where connCheck():
func connCheck(conn net.Conn) error {
...
rawConn, err := sysConn.SyscallConn()
if err != nil {
return err
}
...
}
There was a problem hiding this comment.
I'll take another look at this and see what we can do. Thanks for all your help on this.
There was a problem hiding this comment.
It looks like the platforms that don't support the call won't use connCheck: https://github.com/go-sql-driver/mysql/blob/ad9fa14acdcf7d0533e7fbe58728f3d216213ade/conncheck_dummy.go#L16
So it seems like it's probably fine to return an error, since if it is being used something probably wrong?
There was a problem hiding this comment.
That’s my understanding as well. @Gentoli what do you think?
There was a problem hiding this comment.
Sorry I did not realize // +build is built-in to golang. Both with or without the check on the cast should work.
enocom
changed the title
This reverts commit 4b48e3b. This fix was incorrect. The connection to the Cloud SQL instance is a TLS connection. A recent version of Go made it possible to access the underlying net.Conn, but noted: "writing to or reading from this connection directly will corrupt the TLS session." [1] The change here made it possible for the MySQL driver to inadvertenly read from the underlying connection. [2] Instead, we should be attempting to read from the TLS connection without corrupting it. [1] https://pkg.go.dev/crypto/tls#Conn.NetConn. [2] https://github.com/go-sql-driver/mysql/blob/ad9fa14acdcf7d0533e7fbe58728f3d216213ade/conncheck.go#L34
3 participants
Fixes #225.