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

feat(NODE-5754): allow auto select family options #4185

Merged
merged 3 commits into from
Aug 1, 2024
Merged

feat(NODE-5754): allow auto select family options #4185

merged 3 commits into from
Aug 1, 2024

Conversation

durran
Copy link
Member

@durran durran commented Jul 29, 2024

Description

Allows users to specify auto select family options passed to the underlying connections.

What is changing?

  • Adds autoSelectFamily option to the client which defaults to true.
  • Adds autoSelectFamilyAttemptTimeout option to the client with no default.
  • Adds tests to verify the options are passed through to regular and TLS connections.
Is there new documentation needed for these changes?

Add note in documentation about the options.

What is the motivation for this change?

NODE-5754

Release Highlight

Driver now supports auto selecting between IPv4 and IPv6 connections

For users on Node versions that support the autoSelectFamily and autoSelectFamilyAttemptTimeout options (Node 18.13+), they can now be provided to the MongoClient and will be passed through to socket creation. autoSelectFamily will default to true with autoSelectFamilyAttemptTimeout by default not defined. Example:

const client = new MongoClient(process.env.MONGODB_URI, { autoSelectFamilyAttemptTimeout: 100 });

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@durran durran force-pushed the NODE-5754 branch 9 times, most recently from a55afce to 1f17842 Compare July 30, 2024 18:51
@durran durran marked this pull request as ready for review July 31, 2024 12:09
@nbbeeken nbbeeken self-assigned this Jul 31, 2024
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jul 31, 2024
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

Changes LGTM! Can you fill out the release highlights? this is def a feature to show off 🎉

@durran
Copy link
Member Author

durran commented Jul 31, 2024

Changes LGTM! Can you fill out the release highlights? this is def a feature to show off 🎉

Ah yeah missed that. Updated now.

@durran durran requested a review from nbbeeken July 31, 2024 14:08
Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

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

Question: should we also be using autoSelectFamily when establishing sockets for KMS requests in the state machine?

@durran
Copy link
Member Author

durran commented Jul 31, 2024

Question: should we also be using autoSelectFamily when establishing sockets for KMS requests in the state machine?

Good question. I think the answer to that should be yes? If we agree I can add to this PR.

@baileympearson
Copy link
Contributor

@durran We could potentially consume makeSocket directly from the KMS fetching in the state machine. That's a nice improvement regardless. Then we'd automatically support the new option, so long as we pass it into the state machine.

want to spend a bit of time looking at that approach?

Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

I am just putting back my request changes state since you're working on the above, given the reactions I think we're all on board with expanding this to KMS.

Using makeSocket sounds great but you may run into more refactoring than is worth, I'm thinking about how connecting will be inside that function now and you may have to catch and re-throw in "interesting" ways, additionally, I am unsure if the socks5 behavior is precisely equivalent.

If none of that ends up being a concern GREAT! But if it is, as long as we can get the autoFamily option through we can rest this work there, and file a follow up to consolidate 🚀 Good catch @baileympearson

@durran
Copy link
Member Author

durran commented Jul 31, 2024

The refactor to use makeSocket did prove to be a bit more invasive for my liking, so I've left it out here and will create a ticket for that. PR has now been updated to pass through the new socket options to the state machine to use with KMS requests.

Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

couple of Qs

src/client-side-encryption/state_machine.ts Outdated Show resolved Hide resolved
src/client-side-encryption/state_machine.ts Outdated Show resolved Hide resolved
src/client-side-encryption/auto_encrypter.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

LGTM!

@nbbeeken nbbeeken requested review from baileympearson and removed request for baileympearson August 1, 2024 19:17
@nbbeeken nbbeeken merged commit 54efb7d into main Aug 1, 2024
29 checks passed
@nbbeeken nbbeeken deleted the NODE-5754 branch August 1, 2024 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Primary Review In Review with primary reviewer, not yet ready for team's eyes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants