Skip to content

DRIVERS-3162 Add test for SRV hostname validation when resolver and r…#1787

Open
aclark4life wants to merge 3 commits into
mongodb:masterfrom
aclark4life:DRIVERS-3162
Open

DRIVERS-3162 Add test for SRV hostname validation when resolver and r…#1787
aclark4life wants to merge 3 commits into
mongodb:masterfrom
aclark4life:DRIVERS-3162

Conversation

@aclark4life

@aclark4life aclark4life commented Apr 22, 2025

Copy link
Copy Markdown

DRIVERS-3162

Driver PR:

Please complete the following before merging:

  • Update changelog.
  • Test changes in at least one language driver.
  • Test these changes against all server versions and topologies (including standalone, replica set, sharded
    clusters, and serverless).

@aclark4life
aclark4life requested a review from a team as a code owner April 22, 2025 01:31
@aclark4life
aclark4life requested review from dariakp and removed request for a team April 22, 2025 01:31
@dariakp
dariakp requested a review from ShaneHarvey April 22, 2025 13:13

### 5. Do not throw when return address is identical to SRV hostname and SRV hostname has three or more `.` separated parts

- the SRV `mongodb+srv://blogs.mongodb.com` resolving to `blogs.mongodb.com`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we add the case where mongodb+srv://blogs.mongodb.com resolves to test.blogs.mongodb.com? Or is that already tested elsewhere?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Not sure. We're adding the 5th test because the previous 3 tests confirm that an error is raised. Similar to the first test in which we confirm there is no error, this test confirms there is no error. IIUC, a response of test.blogs.mongodb.com is valid because the domains match, but the hostnames are not identical so we're not looking for this particular success in this test case.

@aclark4life
aclark4life requested a review from ShaneHarvey April 29, 2025 13:19
@aclark4life

Copy link
Copy Markdown
Author

@dariakp @ShaneHarvey Just noticed this stalled, what's next (if you happen to recall, else I'll review!) ?

@dariakp

dariakp commented Sep 15, 2025

Copy link
Copy Markdown
Contributor

@aclark4life The tests are pretty straightforward here and I think the only thing we were waiting on was dev prod; as Shane is the eng lead on the ticket I'll defer to his judgment on what's left to finish this spec change - you have my tentative approval modulo lint issues. It's a good idea to include a link to the driver PR/CI in the description though, for future reference.

@ShaneHarvey

Copy link
Copy Markdown
Contributor

@aclark4life these changes LGTM but https://jira.mongodb.org/browse/DEVPROD-17419 added the real SRV hosts so we can actually add JSON spec tests for these two cases. Should we update this PR to remove the prose test and add the spec test files? JSON files are easier for drivers to implement so that would be my preference.

vector-of-bool
vector-of-bool previously approved these changes Jul 6, 2026

@vector-of-bool vector-of-bool left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@aclark4life
aclark4life enabled auto-merge (squash) July 6, 2026 18:46
@Jibola
Jibola disabled auto-merge July 8, 2026 20:29
@dariakp
dariakp requested review from a team and sleepyStick and removed request for a team and ShaneHarvey July 8, 2026 20:32
- the SRV `mongodb+srv://mongo.local` resolving to `test_1.my_hostmongo.local`
- the SRV `mongodb+srv://blogs.mongodb.com` resolving to `cluster.testmongodb.com`

### 5. Do not throw when return address is identical to SRV hostname and SRV hostname has three or more `.` separated parts

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sorry if this is silly and i'm missing something but why cant this be a unified test?

(If i'm reading the PYTHON-3527's PR correctly, I see that JSON files were added and I do believe they're in the same format as the other unified tests in this spec?)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

sorry if this is silly and i'm missing something but why cant this be a unified test?

(If i'm reading the PYTHON-3527's PR correctly, I see that JSON files were added and I do believe they're in the same format as the other unified tests in this spec?)

Assume you mean PYTHON-5327! Also can you explain what that means for this PR? Add unit tests here? @blink1073 suggested something similar … also note there are two PRs for this in the driver, listed here.

@sleepyStick sleepyStick Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah yes, sorry about the typo, but i was referring to the identical-hostnames-valid-four-parts.json and identical-hostnames-valid-three-parts.json‎ files specifically. I believe they're JSON spec tests and thus should be added to this repo (and are the same as the prose test you've described?)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

and removed from pymongo or … ?

@aclark4life aclark4life Jul 9, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good call @sleepyStick thanks!

Removed prose test case and added JSON/YAML spec test files matching the ones from the driver PR:

  • replica-set/identical-hostnames-valid-three-parts.{json,yml} (mongodb+srv://build.10gen.cc)
  • replica-set/identical-hostnames-valid-four-parts.{json,yml} (mongodb+srv://test.build.10gen.cc)

Documented two new SRV records in the DNS records table in the README (created in DEVPROD-17419 and fixed in DEVPROD-34211) so the tests are reproducible.

These can now be removed from pymongo and consumed from the spec repo like the other JSON tests. 🎉

@aclark4life

aclark4life commented Jul 9, 2026

Copy link
Copy Markdown
Author

@aclark4life The tests are pretty straightforward here and I think the only thing we were waiting on was dev prod; as Shane is the eng lead on the ticket I'll defer to his judgment on what's left to finish this spec change - you have my tentative approval modulo lint issues. It's a good idea to include a link to the driver PR/CI in the description though, for future reference.

@dariakp Thanks and done!

@aclark4life
aclark4life requested a review from a team as a code owner July 9, 2026 18:46
@aclark4life
aclark4life requested a review from qingyang-hu July 9, 2026 18:46
@sleepyStick

Copy link
Copy Markdown
Contributor

A tad unrelated but it seems like the PYTHON-4575 jira ticket is actually split from DRIVERS-2922? So maybe there's only one python PR associated with this driver ticket?

@aclark4life
aclark4life requested a review from sleepyStick July 9, 2026 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants