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.

Sign up for GitHub

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

Jump to bottom

Allow valid SRV hostnames with less than 3 parts #1525

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+331 −193
Open

Allow valid SRV hostnames with less than 3 parts #1525

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
49ee4c4
Allow valid SRV hostnames with less than 3 parts
NathanQingyangXu Oct 4, 2024
1e918ad
change 'isShortSrvHost' to 'srvHostHasLessThanThreeParts' and remove …
NathanQingyangXu Oct 21, 2024
2cb640e
use ClusterFixture.sleep() as per code review comment
NathanQingyangXu Oct 21, 2024
540b89e
replace Mock annotation usage with method scope local Mockito.mock() …
NathanQingyangXu Oct 21, 2024
fb9feb5
improve the logic in DefaultDnsResolver to take into consideration th…
NathanQingyangXu Oct 21, 2024
8e10d43
fix check-style issue (testing method names should not contain '_' ch…
NathanQingyangXu Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions driver-core/src/main/com/mongodb/connection/ClusterSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
import static com.mongodb.assertions.Assertions.isTrueArgument;
import static com.mongodb.assertions.Assertions.notNull;
import static com.mongodb.internal.connection.ServerAddressHelper.createServerAddress;
import static java.lang.String.format;
import static java.util.Collections.singletonList;
import static java.util.Collections.unmodifiableList;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
Expand Down Expand Up @@ -607,11 +606,6 @@ private ClusterSettings(final Builder builder) {
if (builder.srvHost.contains(":")) {
throw new IllegalArgumentException("The srvHost can not contain a host name that specifies a port");
}

if (builder.srvHost.split("\\.").length < 3) {
throw new IllegalArgumentException(format("An SRV host name '%s' was provided that does not contain at least three parts. "
+ "It must contain a hostname, domain name and a top level domain.", builder.srvHost));
}
}

if (builder.hosts.size() > 1 && builder.requiredClusterType == ClusterType.STANDALONE) {
Expand Down
20 changes: 16 additions & 4 deletions driver-core/src/main/com/mongodb/internal/dns/DefaultDnsResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,19 @@ public DefaultDnsResolver(@Nullable final DnsClient dnsClient) {

The priority and weight are ignored, and we just concatenate the host (after removing the ending '.') and port with a
':' in between, as expected by ServerAddress.

It's required that the srvHost has at least three parts (e.g. foo.bar.baz) and that all of the resolved hosts have a parent
domain equal to the domain of the srvHost.
*/
@Override
public List<String> resolveHostFromSrvRecords(final String srvHost, final String srvServiceName) {
String srvHostDomain = srvHost.substring(srvHost.indexOf('.') + 1);
List<String> srvHostParts = asList(srvHost.split("\\."));

String srvHostDomain;
boolean isShortSrvHost = srvHostParts.size() < 3;
if (isShortSrvHost) {
srvHostDomain = srvHost; // when dot separated parts less than 3, domain name is the host per se
NathanQingyangXu marked this conversation as resolved.
Show resolved Hide resolved
} else {
srvHostDomain = srvHost.substring(srvHost.indexOf('.') + 1);
}

List<String> srvHostDomainParts = asList(srvHostDomain.split("\\."));
List<String> hosts = new ArrayList<>();
String resourceName = "_" + srvServiceName + "._tcp." + srvHost;
Expand All @@ -83,6 +89,12 @@ public List<String> resolveHostFromSrvRecords(final String srvHost, final String
for (String srvRecord : srvAttributeValues) {
String[] split = srvRecord.split(" ");
String resolvedHost = split[3].endsWith(".") ? split[3].substring(0, split[3].length() - 1) : split[3];
if (isShortSrvHost && resolvedHost.equals(srvHost)) {
throw new MongoConfigurationException(
format("The SRV host name '%s' has less than three parts and the resolved host '%s' is identical.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this (user-facing) error message discuss implementation details, for example the number of parts? The spec potentially suggests that this would be described as "...host name ... lacks a subdomain, and the resolved..."? I tried to compare against the other drivers, and I believe this corresponds to the following:

Node: https://github.com/mongodb/node-mongodb-native/pull/4197/files#diff-39b2554fd18da165b59a6351b1aafff3714e2a80c1435f2de9706355b4d32351R1181-R1185
Rust: https://github.com/mongodb/mongo-rust-driver/pull/1211/files#diff-32a39d356e1edde3d94581275b42e994ad1780ce4de8d6935ec5596fe4493ffdR39

However, I am not sure, because the error messages are different. It would have been useful if the spec specified the wording of the error messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This exception is mainly for this prose testing case: mongodb/specifications@8e9d768#diff-dfcf948e6a987b880ea85e1907d5b7ce8fb98c9dab752f42ccdaf1e656b8848fR30

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for referring to other driver's implementation. will investigate further.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as per the following code snippet, the exception message won't be exposed to end user but end up with a generic exception:

throw new MongoConfigurationException(format("Failed looking up SRV record for '%s'.", resourceName), e);

Copy link
Contributor Author

@NathanQingyangXu NathanQingyangXu Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I changed the logic to accommodate the possibility that 'resolvedHost' could be something like localhost (as in Prosetest case No.3: mongodb/specifications@8e9d768#diff-dfcf948e6a987b880ea85e1907d5b7ce8fb98c9dab752f42ccdaf1e656b8848fR30) which doesn't contain '.' separator and so it has no parent domain. After the code refactoring, no new exception or logic branch is required.

Copy link
Contributor

Choose a reason for hiding this comment

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

Were the prose tests passing previously? I thought you were testing against localhost.

exception message won't be exposed to end user

I believe all exception messages are exposed to the user. It may be in "caused by", but this is still visible.

no new exception or logic branch is required

I am not sure I follow the logic well enough to understand this - why is it that the other drivers have introduced 1 or 2 new messages, such as "Server record does not have at least one more domain level than parent URI", but we do not need to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, you are totally correct in that the exception will be exposed to end user as exception's cause. I missed that. Now after new code change (converging the newly -added logic branch to the existing only flow), we reuse the old same exception mesage as below:

throw new MongoConfigurationException(
                            format("The SRV host name '%s' resolved to a host '%s 'that is not in a sub-domain of the SRV host.",
                                    srvHost, resolvedHost));

It is simply another way to express the same meaning, from my understanding. Maybe it is desirable to maintain backward compatibility so I didn't change it.

srvHost, resolvedHost)
);
}
String resolvedHostDomain = resolvedHost.substring(resolvedHost.indexOf('.') + 1);
if (!sameParentDomain(srvHostDomainParts, resolvedHostDomain)) {
throw new MongoConfigurationException(
Expand Down
182 changes: 182 additions & 0 deletions driver-core/src/test/resources/initial-dns-seedlist-discovery/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,182 @@
# Initial DNS Seedlist Discovery tests

This directory contains platform-independent tests that drivers can use to prove their conformance to the Initial DNS
Seedlist Discovery spec.

## Prose Tests

For the following prose tests, it is assumed drivers are be able to stub DNS results to easily test invalid DNS
resolution results.

### 1. Allow SRVs with fewer than 3 `.` separated parts

When running validation on an SRV string before DNS resolution, do not throw a error due to number of SRV parts.

- `mongodb+srv://localhost`
- `mongodb+srv://mongo.local`

### 2. Throw when return address does not end with SRV domain

When given a returned address that does NOT end with the original SRV's domain name, throw a runtime error.

For this test, run each of the following cases:

- the SRV `mongodb+srv://localhost` resolving to `localhost.mongodb`
- the SRV `mongodb+srv://mongo.local` resolving to `test_1.evil.local`
- the SRV `mongodb+srv://blogs.mongodb.com` resolving to `blogs.evil.com`

Remember, the domain of an SRV with one or two `.` separated parts is the SRVs entire hostname.

### 3. Throw when return address is identical to SRV hostname

When given a returned address that is identical to the SRV hostname and the SRV hostname has fewer than three `.`
separated parts, throw a runtime error.

For this test, run each of the following cases:

- the SRV `mongodb+srv://localhost` resolving to `localhost`
- the SRV `mongodb+srv://mongo.local` resolving to `mongo.local`

### 4. Throw when return address does not contain `.` separating shared part of domain

When given a returned address that does NOT share the domain name of the SRV record because it's missing a `.`, throw a
runtime error.

For this test, run each of the following cases:

- the SRV `mongodb+srv://localhost` resolving to `test_1.cluster_1localhost`
- 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`

## Test Setup

The tests in the `replica-set` directory MUST be executed against a three-node replica set on localhost ports 27017,
27018, and 27019 with replica set name `repl0`.

The tests in the `load-balanced` directory MUST be executed against a load-balanced sharded cluster with the mongos
servers running on localhost ports 27017 and 27018 and `--loadBalancerPort` 27050 and 27051, respectively (corresponding
to the script in
[drivers-evergreen-tools](https://github.com/mongodb-labs/drivers-evergreen-tools/blob/master/.evergreen/run-load-balancer.sh)).
The load balancers, shard servers, and config servers may run on any open ports.

The tests in the `sharded` directory MUST be executed against a sharded cluster with the mongos servers running on
localhost ports 27017 and 27018. Shard servers and config servers may run on any open ports.

In all cases, the clusters MUST be started with SSL enabled.

To run the tests that accompany this spec, you need to configure the SRV and TXT records with a real name server. The
following records are required for these tests:

```
Record TTL Class Address
localhost.test.build.10gen.cc. 86400 IN A 127.0.0.1
localhost.sub.test.build.10gen.cc. 86400 IN A 127.0.0.1

Record TTL Class Port Target
_mongodb._tcp.test1.test.build.10gen.cc. 86400 IN SRV 27017 localhost.test.build.10gen.cc.
_mongodb._tcp.test1.test.build.10gen.cc. 86400 IN SRV 27018 localhost.test.build.10gen.cc.
_mongodb._tcp.test2.test.build.10gen.cc. 86400 IN SRV 27018 localhost.test.build.10gen.cc.
_mongodb._tcp.test2.test.build.10gen.cc. 86400 IN SRV 27019 localhost.test.build.10gen.cc.
_mongodb._tcp.test3.test.build.10gen.cc. 86400 IN SRV 27017 localhost.test.build.10gen.cc.
_mongodb._tcp.test5.test.build.10gen.cc. 86400 IN SRV 27017 localhost.test.build.10gen.cc.
_mongodb._tcp.test6.test.build.10gen.cc. 86400 IN SRV 27017 localhost.test.build.10gen.cc.
_mongodb._tcp.test7.test.build.10gen.cc. 86400 IN SRV 27017 localhost.test.build.10gen.cc.
_mongodb._tcp.test8.test.build.10gen.cc. 86400 IN SRV 27017 localhost.test.build.10gen.cc.
_mongodb._tcp.test10.test.build.10gen.cc. 86400 IN SRV 27017 localhost.test.build.10gen.cc.
_mongodb._tcp.test11.test.build.10gen.cc. 86400 IN SRV 27017 localhost.test.build.10gen.cc.
_mongodb._tcp.test12.test.build.10gen.cc. 86400 IN SRV 27017 localhost.build.10gen.cc.
_mongodb._tcp.test13.test.build.10gen.cc. 86400 IN SRV 27017 test.build.10gen.cc.
_mongodb._tcp.test14.test.build.10gen.cc. 86400 IN SRV 27017 localhost.not-test.build.10gen.cc.
_mongodb._tcp.test15.test.build.10gen.cc. 86400 IN SRV 27017 localhost.test.not-build.10gen.cc.
_mongodb._tcp.test16.test.build.10gen.cc. 86400 IN SRV 27017 localhost.test.build.not-10gen.cc.
_mongodb._tcp.test17.test.build.10gen.cc. 86400 IN SRV 27017 localhost.test.build.10gen.not-cc.
_mongodb._tcp.test18.test.build.10gen.cc. 86400 IN SRV 27017 localhost.sub.test.build.10gen.cc.
_mongodb._tcp.test19.test.build.10gen.cc. 86400 IN SRV 27017 localhost.evil.build.10gen.cc.
_mongodb._tcp.test19.test.build.10gen.cc. 86400 IN SRV 27017 localhost.test.build.10gen.cc.
_mongodb._tcp.test20.test.build.10gen.cc. 86400 IN SRV 27017 localhost.test.build.10gen.cc.
_mongodb._tcp.test21.test.build.10gen.cc. 86400 IN SRV 27017 localhost.test.build.10gen.cc.
_customname._tcp.test22.test.build.10gen.cc 86400 IN SRV 27017 localhost.test.build.10gen.cc.
_mongodb._tcp.test23.test.build.10gen.cc. 86400 IN SRV 8000 localhost.test.build.10gen.cc.
_mongodb._tcp.test24.test.build.10gen.cc. 86400 IN SRV 8000 localhost.test.build.10gen.cc.

Record TTL Class Text
test5.test.build.10gen.cc. 86400 IN TXT "replicaSet=repl0&authSource=thisDB"
test6.test.build.10gen.cc. 86400 IN TXT "replicaSet=repl0"
test6.test.build.10gen.cc. 86400 IN TXT "authSource=otherDB"
test7.test.build.10gen.cc. 86400 IN TXT "ssl=false"
test8.test.build.10gen.cc. 86400 IN TXT "authSource"
test10.test.build.10gen.cc. 86400 IN TXT "socketTimeoutMS=500"
test11.test.build.10gen.cc. 86400 IN TXT "replicaS" "et=rep" "l0"
test20.test.build.10gen.cc. 86400 IN TXT "loadBalanced=true"
test21.test.build.10gen.cc. 86400 IN TXT "loadBalanced=false"
test24.test.build.10gen.cc. 86400 IN TXT "loadBalanced=true"
```

Notes:

- `test4` is omitted deliberately to test what happens with no SRV record.
- `test9` is missing because it was deleted during the development of the tests.
- The missing `test.` sub-domain in the SRV record target for `test12` is deliberate.
- `test22` is used to test a custom service name (`customname`).
- `test23` and `test24` point to port 8000 (HAProxy) and are used for load-balanced tests.

In our tests we have used `localhost.test.build.10gen.cc` as the domain, and then configured
`localhost.test.build.10gen.cc` to resolve to 127.0.0.1.

You need to adapt the records shown above to replace `test.build.10gen.cc` with your own domain name, and update the
"uri" field in the YAML or JSON files in this directory with the actual domain.

## Test Format and Use

These YAML and JSON files contain the following fields:

- `uri`: a `mongodb+srv` connection string
- `seeds`: the expected set of initial seeds discovered from the SRV record
- `numSeeds`: the expected number of initial seeds discovered from the SRV record. This is mainly used to test
`srvMaxHosts`, since randomly selected hosts cannot be deterministically asserted.
- `hosts`: the discovered topology's list of hosts once SDAM completes a scan
- `numHosts`: the expected number of hosts discovered once SDAM completes a scan. This is mainly used to test
`srvMaxHosts`, since randomly selected hosts cannot be deterministically asserted.
- `options`: the parsed [URI options](../../uri-options/uri-options.md) as discovered from the
[Connection String](../../connection-string/connection-string-spec.md)'s "Connection Options" component and SRV
resolution (e.g. TXT records, implicit `tls` default).
- `parsed_options`: additional, parsed options from other
[Connection String](../../connection-string/connection-string-spec.md) components. This is mainly used for asserting
`UserInfo` (as `user` and `password`) and `Auth database` (as `auth_database`).
- `error`: indicates that the parsing of the URI, or the resolving or contents of the SRV or TXT records included
errors.
- `comment`: a comment to indicate why a test would fail.
- `ping`: if false, the test runner should not run a "ping" operation.

For each YAML file:

- Create a MongoClient initialized with the `mongodb+srv` connection string.
- Run a "ping" operation unless `ping` is false or `error` is true.

Assertions:

- If `seeds` is specified, drivers SHOULD verify that the set of hosts in the client's initial seedlist matches the list
in `seeds`. If `numSeeds` is specified, drivers SHOULD verify that the size of that set matches `numSeeds`.

- If `hosts` is specified, drivers MUST verify that the set of ServerDescriptions in the client's TopologyDescription
eventually matches the list in `hosts`. If `numHosts` is specified, drivers MUST verify that the size of that set
matches `numHosts`.

- If `options` is specified, drivers MUST verify each of the values under `options` match the MongoClient's parsed value
for that option. There may be other options parsed by the MongoClient as well, which a test does not verify.

- If `parsed_options` is specified, drivers MUST verify that each of the values under `parsed_options` match the
MongoClient's parsed value for that option. Supported values include, but are not limited to, `user` and `password`
(parsed from `UserInfo`) and `auth_database` (parsed from `Auth database`).

- If `error` is specified and `true`, drivers MUST verify that initializing the MongoClient throws an error. If `error`
is not specified or is `false`, both initializing the MongoClient and running a ping operation must succeed without
throwing any errors.

- If `ping` is not specified or `true`, drivers MUST verify that running a "ping" operation using the initialized
MongoClient succeeds. If `ping` is `false`, drivers MUST NOT run a "ping" operation.

> **Note:** These tests are expected to be run against MongoDB databases with and without authentication enabled. The
> "ping" operation does not require authentication so should succeed with URIs that contain no userinfo (i.e. no
> username and password). Tests with URIs that contain userinfo always set `ping` to `false` because some drivers will
> fail handshake on a connection if userinfo is provided but incorrect.
Loading