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

fix(NODE-4621): ipv6 address handling in HostAddress #3410

Merged
merged 4 commits into from Sep 16, 2022
Merged

Conversation

nbbeeken
Copy link
Contributor

@nbbeeken nbbeeken commented Sep 13, 2022

Description

NODE-4621

What is changing?

Always include brackets when getting the whole hostAddress string.

  • Ensure Connection instances have the correct formatted address for ipv6
  • HostAddress.toString no longer accepts a flag for disabling IPv6 brackets (they should never have been created without brackets)
  • Wrap the error thrown from new URL it was very unclear only printing: TypeError: Invalid URL prints the input hostname now and attaches the original error as a cause.
  • Fixed up a cursor test while debugging it, I can revert, it just happened to demonstrate an issue with bracketing
  • fixed type of this.configuration.options.hostAddresses to not be nullish
  • Added the --ipv6 flag to our mtools script
  • cmap spec runner improvement, print the original error stack and name, otherwise its hard to trace
  • fixed uri_spec_runner to handle new error wrapping logic, I can remove the TypeError handling now, thoughts?
  • Added unit tests to connection_string to demonstrate what does and does not work with IPv6 addresses
  • Notably: Added IPv6 integration tests that only run on Windows and ReplicaSet configs.
    • This is because Ubuntu 18.04 (our current default OS) does not have a hostname lookup for localhost to ::1 by default, Windows, does, and on macos we don't run replica_sets

What is the motivation for this change?

When a port exists (which always does for us) you must include brackets for the whole host address

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>
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@nbbeeken nbbeeken force-pushed the NODE-4621/ipv6 branch 4 times, most recently from b203159 to f94c18c Compare September 14, 2022 20:06
@nbbeeken nbbeeken marked this pull request as ready for review September 14, 2022 20:31
addaleax
addaleax previously approved these changes Sep 14, 2022
src/utils.ts Outdated Show resolved Hide resolved
test/integration/node-specific/ipv6.test.ts Show resolved Hide resolved
@dariakp dariakp self-assigned this Sep 15, 2022
@dariakp dariakp added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Sep 15, 2022
test/tools/uri_spec_runner.ts Outdated Show resolved Hide resolved
test/tools/cluster_setup.sh Show resolved Hide resolved
test/unit/connection_string.test.ts Outdated Show resolved Hide resolved
test/unit/connection_string.test.ts Outdated Show resolved Hide resolved
test/integration/node-specific/ipv6.test.ts Show resolved Hide resolved
test/integration/node-specific/ipv6.test.ts Show resolved Hide resolved
@dariakp dariakp added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Sep 15, 2022
@dariakp dariakp merged commit 5eb3978 into main Sep 16, 2022
@dariakp dariakp deleted the NODE-4621/ipv6 branch September 16, 2022 16:11
ZLY201 pushed a commit to ZLY201/node-mongodb-native that referenced this pull request Nov 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
3 participants