Skip to content

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

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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
@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
@nbbeeken nbbeeken requested a review from dariakp September 15, 2022 18:47
@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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants