Skip to content

THRIFT-5996: connection check supports TLS sockets#3488

Merged
fishy merged 1 commit into
apache:masterfrom
murfffi:THRIFT-5996
May 16, 2026
Merged

THRIFT-5996: connection check supports TLS sockets#3488
fishy merged 1 commit into
apache:masterfrom
murfffi:THRIFT-5996

Conversation

@murfffi
Copy link
Copy Markdown
Contributor

@murfffi murfffi commented May 15, 2026

Client: go

The connection check first introduced in THRIFT-5214 now supports TLS connections using a technique from the code that initially inspired the check:
go-sql-driver/mysql#934

Testing done: new test with alongside the existing test of conn. check

@murfffi murfffi requested a review from fishy as a code owner May 15, 2026 16:33
@mergeable mergeable Bot added the golang patches related to go language label May 15, 2026
Comment thread lib/go/thrift/socket_unix_conn.go Outdated
Comment on lines +45 to +46
if connTLS, isTLS := rawConn.(*tls.Conn); isTLS {
rawConn = connTLS.NetConn()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the jira ticket your proposal was to do this before trying to get syscallConn. is there a reason you flipped it to this approach?

I like that approach better because that way you don't have the next line that rewrites syscallConn, ok which is very error-prone (e.g. if you used ok instead of isTLS on line 45 that's a bug that's hard to spot).

Copy link
Copy Markdown
Contributor Author

@murfffi murfffi May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review! I agree that the version is more convoluted and thus error prone .

I wondered if it is likely that, in the future, Go team makes *tls.Config implement syscall.Conn directly, possibly to utilize https://docs.kernel.org/networking/tls-offload.html on Linux.

I though about this more and I believe that if something drastic like that happens, the syscall code may not work and we should unwrap the connection anyway.

I changed it back to the variant in JIRA. Please take another look.

Copy link
Copy Markdown
Contributor Author

@murfffi murfffi May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed golang/go#44506 - tracking kernel TLS support in Go . It is in state Proposal-Accepted but not implemented. A couple of people implemented crypto/tls forks as PoC. I think the point still stands - there is no value to check if tls.Conn implements syscall.Conn.

Client: go

The connection check introduced in THRIFT-5214 now supports
TLS connections using a technique from the code that
initially inspired the check:
go-sql-driver/mysql#934

Testing done: new test with alongside the existing test of conn. check
@fishy fishy merged commit 6b02163 into apache:master May 16, 2026
98 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

golang patches related to go language

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants