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.

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

GODRIVER-1879 Apply connectTimeoutMS to TLS handshake #594

Merged
merged 3 commits into from Mar 8, 2021

Conversation

divjotarora
Copy link
Collaborator

This PR modifies the connection code to apply the connectTimeoutMS option to both socket establishment and the TLS handshake. Previously, it was only applied to the former. To aid in testing, I also modified the existing tlsConnectionSource interface to return a new tlsConn interface rather than directly returning a concrete tls.Conn. This allows us to provide a custom implementation for TLS handshaking in our tests.

The alternate approach considered was to use tls.Dialer or tls.DialWithDialer. These aren't flexible enough because they require us to specify a net.Dialer as the dialer for the underlying network socket, but we actually accept a custom interface to dial connections, which defaults to net.Dialer but can be something else (e.g. we have seen this option used to dial TLS connections via openssl rather than crytp/tls).

@divjotarora divjotarora requested a review from iwysiu March 1, 2021 19:30
Copy link
Contributor

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

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

LGTM!

x/mongo/driver/topology/connection_test.go Show resolved Hide resolved
x/mongo/driver/topology/connection_options.go Show resolved Hide resolved
iwysiu
iwysiu approved these changes Mar 2, 2021
Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

Code changes LGTM, I just have a couple clarifying questions. Nicely done!

var dialCancel context.CancelFunc
if c.config.connectTimeout != 0 {
dialCtx, dialCancel = context.WithTimeout(handshakeCtx, c.config.connectTimeout)
defer dialCancel()
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to change. But IIUC, since this derived from handshakeCtx, it is not strictly necessary to cancel the dialCtx. Cancelling handshakeCtx would cancel any derived contexts. Is that right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have the same understanding. However, AFAIK it's good practice to defer all context.CancelFunc variables and this is happening once per connection handshake, so I don't think it's going to be prohibitive to do that here.

x/mongo/driver/topology/connection.go Show resolved Hide resolved
x/mongo/driver/topology/connection_test.go Show resolved Hide resolved
@divjotarora divjotarora merged commit 5c0f679 into mongodb:master Mar 8, 2021
15 checks passed
@divjotarora divjotarora deleted the godriver1879-tls-timeout branch March 8, 2021 02:40
This was referenced Mar 10, 2021
This was referenced Mar 15, 2021
faem pushed a commit to kubedb/mongo-go-driver that referenced this pull request Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants