DRIVERS-3632 Introduce Custom Callback logic in Configurable DNS - #1980
sleepyStick wants to merge 5 commits into
Conversation
| hostname has fewer than three `.` separated parts, the returned hostname MUST have at least one more domain level | ||
| than the SRV record hostname. | ||
| - When [`srvAllowedHostsSuffix`](#srvallowedhostssuffix) is configured, the returned host name MUST end in | ||
| `srvAllowedHostsSuffix` after normalization. |
There was a problem hiding this comment.
Suggest tightening:
MUST end in
.followed by thesrvAllowedHostsSuffixvalue.
|
|
||
| Configure a MongoClient with `srvAllowedHostsSuffix=.mongodb.com` and assert that the SRV | ||
| `mongodb+srv://blogs.mongodb.com` resolving to `CLUSTER.MONGODB.COM.` produces a seedlist containing | ||
| `cluster.mongodb.com`. |
There was a problem hiding this comment.
This asserts the seedlist stores the normalized host name, but Querying DNS only requires normalization "before validating returned hostnames". A driver that normalizes only for the comparison and seeds the DNS-returned string is conforming today and would fail this test. Test 7 works because the srvHostValidator signature explicitly requires the normalized form as the callback input but there's no equivalent sentence for the seedlist. Could we add a similar sentence to Querying DNS?
E.g.
Before validating returned hostnames, drivers MUST normalize them as follows, in this order:
- Any trailing
.MUST be stripped. For example,host.mydomain.net.becomeshost.mydomain.net.- The hostname MUST be converted to its A-label (Punycode) form.
- The hostname MUST be normalized to lowercase using ASCII case folding.
Drivers MUST use the normalized hostnames, rather than the hostnames exactly as returned by DNS, to populate the
seedlist.
|
|
||
| Run this test twice: once with a validator that returns `false` for every host name, and once with a validator that | ||
| raises an error for every host name. In both cases the driver MUST treat the returned host name as non-compliant, and | ||
| MUST NOT raise an error to the application or stop rescanning. |
There was a problem hiding this comment.
Suggest tightening so we don't imply reconfigure mongo client.
Run this test twice: once with a validator that returns
falsefor every host name, and once with a validator that
raises an error for every host name.
The validator's behavior MUST be controlled by state external to the validator (e.g. a flag the validator reads) so that it can be changed later in the test without reconfiguring the MongoClient.
In both cases the driver MUST treat the returned host name as non-compliant, and MUST NOT raise an error to the
application or stop rescanning.
|
|
||
| Then reconfigure the validator to return `true` for every host name, wait until `2*rescanSRVIntervalMS`, and assert that | ||
| rescanning was not stopped by the earlier failures: the final topology description MUST contain | ||
| `localhost.test.build.10gen.cc:27019` in addition to the two original hosts. |
There was a problem hiding this comment.
Suggest tightening
Then change the validator's behavior so that it returns
truefor every host name. Because the earlier rescans
obtained no verified hosts, the driver has temporarily set rescanSRVIntervalMS to heartbeatFrequencyMS, so the next
rescan occurs withinheartbeatFrequencyMS. Wait until2*heartbeatFrequencyMSand assert that rescanning was not
stopped by the earlier failures: the final topology description MUST containlocalhost.test.build.10gen.cc:27019in
addition to the two original hosts.
There was a problem hiding this comment.
Should we also add a prose test for the reserved single labels?
| ### 5. srvHostValidator accepts a host the default verification would reject | ||
|
|
||
| When `srvHostValidator` is configured, it replaces the default verification entirely, so a returned address the default | ||
| check would reject MUST be accepted if the validator returns `true`. | ||
|
|
||
| Configure a validator that returns `true` for every host name and assert that the SRV `mongodb+srv://blogs.mongodb.com` | ||
| resolving to `blogs.evil.com` produces a seedlist containing `blogs.evil.com`. |
There was a problem hiding this comment.
The default verification has two parts: (1) the returned host must share the SRV's {domainname}, and (2) when the SRV hostname has fewer than three . separated parts, the returned host must add at least one level.
This prose test currently only pins down part 1. Should we add another case that pins down part 2? something like mongodb+srv://mongo.corp resolving to mongo.corp
| The driver MUST report an error if any of `srvServiceName`, `srvMaxHosts`, or `srvAllowedHostsSuffix` URI options are | ||
| specified with a non-SRV URI (i.e. scheme other than `mongodb+srv`). The driver MUST allow specifying the | ||
| `srvServiceName`, `srvMaxHosts`, and `srvAllowedHostsSuffix` URI options with an SRV URI (i.e. `mongodb+srv` scheme). | ||
| While not a URI option, `srvHostValidator` also MUST only be allowed with the use of an SRV URI. |
There was a problem hiding this comment.
This MUST currently has no test. Should we add a prose test for it?
This also has the same open question as the mutual-exclusion error — when it surfaces isn't stated anywhere. Whatever timing sentence lands for the mutual-exclusion error should cover this check too.
| mongodb+srv://cluster.test.internal.example.com/?srvAllowedHostsSuffix=.internal.example.com | ||
| ``` | ||
|
|
||
| ### Rationale for `srvHostValidator` |
There was a problem hiding this comment.
Should we also add a rational entry for the allowlist?
…cords-for-mongos-discovery.md Co-authored-by: Adelin Owona <51498470+adelinowona@users.noreply.github.com>
…covery.md Co-authored-by: Adelin Owona <51498470+adelinowona@users.noreply.github.com>
| the program is compiled -- MUST skip this test. | ||
|
|
||
| Assert that configuring a MongoClient with a `srvHostValidator` that is not callable, such as the string | ||
| `"notacallable"`, throws a runtime error. |
There was a problem hiding this comment.
Is throwing a runtime error when the validator is not callable described in the spec?
| - When [`srvHostValidator`](#srvhostvalidator) is configured, the driver MUST pass each returned host name to the | ||
| validator and MUST treat the value it returns as the complete verdict: a returned host name is valid if and only if | ||
| the validator returns `true`. Drivers MUST NOT additionally apply the `{domainname}` check or the domain level | ||
| requirement described above, whether before or after calling the validator. |
There was a problem hiding this comment.
Can we add this to ensure all returned hosts are validated?
The driver MUST call the validator once for every returned host name, before any
srvMaxHostsselection is applied.
Please complete the following before merging:
pymongo pr: PYTHON-6080 Introduce Custom Callback logic in Configurable DNS mongo-python-driver#3051
clusters).