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

feat: directConnection adds unify behavior for replica set discovery #2349

Merged
merged 10 commits into from
May 15, 2020
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
24 changes: 19 additions & 5 deletions lib/connection_string.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ function matchesParentDomain(srvAddress, parentDomain) {
function parseSrvConnectionString(uri, options, callback) {
const result = URL.parse(uri, true);

if (options.directConnection || options.directconnection) {
Copy link
Member

Choose a reason for hiding this comment

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

is it possible to have both forms here? I was hoping we wouldn't be introducing more cases where we had to check the upper and lowercase version of URI options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

During testing it came in as both, I think it performs differently when in URL and passed in, this covers all our bases.

Screen Shot 2020-05-11 at 3 52 25 PM

Is there a helper I'm missing? It's also in CASE_TRANSLATION. 😫

Copy link
Member

Choose a reason for hiding this comment

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

so case translation comes into play in applyConnectionStringOption which is in turn called by parseQueryString. Your change here moves the call to parseSrvConnectionString to after parseQueryString is called, which means that we should be in a world where all known values are camelCased.

Copy link
Member

Choose a reason for hiding this comment

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

@reggi I think we're waiting on you here

return callback(new MongoParseError('directConnection not supported with SRV URI'));
}

if (result.hostname.split('.').length < 3) {
return callback(new MongoParseError('URI does not have hostname, domain name and tld'));
}
Expand Down Expand Up @@ -217,7 +221,8 @@ const CASE_TRANSLATION = {
tlscertificatekeyfile: 'tlsCertificateKeyFile',
tlscertificatekeyfilepassword: 'tlsCertificateKeyFilePassword',
wtimeout: 'wTimeoutMS',
j: 'journal'
j: 'journal',
directconnection: 'directConnection'
};

/**
Expand Down Expand Up @@ -594,10 +599,6 @@ function parseConnectionString(uri, options, callback) {
return callback(new MongoParseError('Invalid protocol provided'));
}

if (protocol === PROTOCOL_MONGODB_SRV) {
return parseSrvConnectionString(uri, options, callback);
}

const dbAndQuery = cap[4].split('?');
const db = dbAndQuery.length > 0 ? dbAndQuery[0] : null;
const query = dbAndQuery.length > 1 ? dbAndQuery[1] : null;
Expand All @@ -613,6 +614,12 @@ function parseConnectionString(uri, options, callback) {
return callback(parseError);
}

parsedOptions = Object.assign({}, parsedOptions, options);

if (protocol === PROTOCOL_MONGODB_SRV) {
return parseSrvConnectionString(uri, parsedOptions, callback);
}

const auth = { username: null, password: null, db: db && db !== '' ? qs.unescape(db) : null };
if (parsedOptions.auth) {
// maintain support for legacy options passed into `MongoClient`
Expand Down Expand Up @@ -706,6 +713,13 @@ function parseConnectionString(uri, options, callback) {
return callback(new MongoParseError('No hostname or hostnames provided in connection string'));
}

const directConnection = !!parsedOptions.directConnection;
if (directConnection && hosts.length !== 1) {
// If the option is set to true, the driver MUST validate that there is exactly one host given
// in the host list in the URI, and fail client creation otherwise.
return callback(new MongoParseError('directConnection option requires exactly one host'));
}

const result = {
hosts: hosts,
auth: auth.db || auth.username ? auth : null,
Expand Down
2 changes: 2 additions & 0 deletions lib/mongo_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ const { connect, validOptions } = require('./operations/connect');
* @param {boolean} [options.useNewUrlParser=true] Determines whether or not to use the new url parser. Enables the new, spec-compliant, url parser shipped in the core driver. This url parser fixes a number of problems with the original parser, and aims to outright replace that parser in the near future. Defaults to true, and must be explicitly set to false to use the legacy url parser.
* @param {DriverInfoOptions} [options.driverInfo] Allows a wrapping driver to amend the client metadata generated by the driver to include information about the wrapping driver
* @param {AutoEncrypter~AutoEncryptionOptions} [options.autoEncryption] Optionally enable client side auto encryption
* @param {boolean} [options.directConnection=false] Enable directConnection
Copy link
Member

Choose a reason for hiding this comment

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

can we improve this doc string? "Indicates that a client should directly connect to a node without attempting to discover its topology type" or something like that

* @returns {MongoClient} a MongoClient instance
*/
function MongoClient(url, options) {
Expand Down Expand Up @@ -388,6 +389,7 @@ MongoClient.prototype.isConnected = function(options) {
* @param {number} [options.numberOfRetries=5] The number of retries for a tailable cursor
* @param {boolean} [options.auto_reconnect=true] Enable auto reconnecting for single server instances
* @param {number} [options.minSize] If present, the connection pool will be initialized with minSize connections, and will never dip below minSize connections
* @param {boolean} [options.directConnection=false] Enable directConnection
Copy link
Member

Choose a reason for hiding this comment

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

same comment

* @param {MongoClient~connectCallback} [callback] The command result callback
* @returns {Promise<MongoClient>} returns Promise if no callback passed
*/
Expand Down
3 changes: 2 additions & 1 deletion lib/operations/connect.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,8 @@ const validOptionNames = [
'tlsCertificateKeyFilePassword',
'minHeartbeatFrequencyMS',
'heartbeatFrequencyMS',
'waitQueueTimeoutMS'
'waitQueueTimeoutMS',
'directConnection'
];

const ignoreOptionNames = ['native_parser'];
Expand Down
4 changes: 4 additions & 0 deletions lib/sdam/server_selection.js
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,10 @@ function readPreferenceServerSelector(readPreference) {
);
}

if (topologyDescription.type === TopologyType.Unknown) {
return [];
}

if (
topologyDescription.type === TopologyType.Single ||
topologyDescription.type === TopologyType.Sharded
Expand Down
28 changes: 24 additions & 4 deletions lib/sdam/topology.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,10 @@ class Topology extends EventEmitter {
* @param {number} [options.localThresholdMS=15] The size of the latency window for selecting among multiple suitable servers
* @param {number} [options.serverSelectionTimeoutMS=30000] How long to block for server selection before throwing an error
* @param {number} [options.heartbeatFrequencyMS=10000] The frequency with which topology updates are scheduled
* @param {boolean} [options.directConnection] will set topology to Single
* @param {string} [options.replicaSet] will set topology to ReplicaSetNoPrimary
Copy link
Member

Choose a reason for hiding this comment

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

replicaSet should be the only supported option, setName and rs_name can and should be removed in this major revision. Also I think the docstrings for all of these are a bit wanting, directConnection means "connect directly to the node without discovering its type" and "replicaSet" is the name of the replica set to connect to (used for validation during initial handshake as well). You can find decent descriptions of these in the URI Options spec

* @param {string} [options.setName] will set topology to ReplicaSetNoPrimary
* @param {string} [options.rs_name] will set topology to ReplicaSetNoPrimary
*/
constructor(seedlist, options) {
super();
Expand Down Expand Up @@ -136,7 +140,7 @@ class Topology extends EventEmitter {
}
});

const topologyType = topologyTypeFromSeedlist(seedlist, options);
const topologyType = topologyTypeFromOptions(options);
const topologyId = globalTopologyCounter++;
const serverDescriptions = seedlist.reduce((result, seed) => {
if (seed.domain_socket) seed.host = seed.domain_socket;
Expand Down Expand Up @@ -797,10 +801,26 @@ function parseStringSeedlist(seedlist) {
}));
}

function topologyTypeFromSeedlist(seedlist, options) {
/**
* Gets the TopologyType from the client options
*
* @param {object} options mongo client options
* @param {boolean} [options.directConnection] will set topology to Single
* @param {string} [options.replicaSet] will set topology to ReplicaSetNoPrimary
* @param {string} [options.setName] will set topology to ReplicaSetNoPrimary
* @param {string} [options.rs_name] will set topology to ReplicaSetNoPrimary
* @returns TopologyType
*/
function topologyTypeFromOptions(options) {
if (options.directConnection) {
return TopologyType.Single;
}

const replicaSet = options.replicaSet || options.setName || options.rs_name;
if (seedlist.length === 1 && !replicaSet) return TopologyType.Single;
if (replicaSet) return TopologyType.ReplicaSetNoPrimary;
if (replicaSet) {
return TopologyType.ReplicaSetNoPrimary;
}

return TopologyType.Unknown;
}

Expand Down
18 changes: 14 additions & 4 deletions lib/sdam/topology_description.js
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ class TopologyDescription {
}

if (topologyType === TopologyType.Unknown) {
if (serverType === ServerType.Standalone) {
if (serverType === ServerType.Standalone && this.servers.size !== 1) {
emadum marked this conversation as resolved.
Show resolved Hide resolved
serverDescriptions.delete(address);
} else {
topologyType = topologyTypeForServerType(serverType);
Expand Down Expand Up @@ -277,9 +277,19 @@ class TopologyDescription {
}

function topologyTypeForServerType(serverType) {
if (serverType === ServerType.Mongos) return TopologyType.Sharded;
if (serverType === ServerType.RSPrimary) return TopologyType.ReplicaSetWithPrimary;
return TopologyType.ReplicaSetNoPrimary;
switch (serverType) {
case ServerType.Standalone:
return TopologyType.Single;
case ServerType.Mongos:
return TopologyType.Sharded;
case ServerType.RSPrimary:
return TopologyType.ReplicaSetWithPrimary;
case ServerType.RSOther:
case ServerType.RSSecondary:
return TopologyType.ReplicaSetNoPrimary;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It didn't make sense to me to have the "default" be ReplicaSetNoPrimary so I swapped this, initial spec tests showed this worked.

Copy link
Member

Choose a reason for hiding this comment

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

nit: I don't find that using a switch here improves readability at all 🤷‍♂️ Maybe adding some line breaks in there would be useful, I just find it hard to visually track

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I generally don't prefer switch syntax, but i think @emadum's recommendation makes sense here it does help restrict what this function can do. It makes it more obvious that this function takes a ServerType and returns a TopologyType and no other logic is really allowed.

Copy link
Member

Choose a reason for hiding this comment

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

default Unknown works for me, I'm fine to resolve this comment

default:
return TopologyType.Unknown;
}
}

function compareObjectId(oid1, oid2) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{
"uri": "mongodb+srv://test3.test.build.10gen.cc/?directConnection=false",
"seeds": [
"localhost.test.build.10gen.cc:27017"
],
"hosts": [
"localhost:27017",
"localhost:27018",
"localhost:27019"
],
"options": {
"ssl": true
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
uri: "mongodb+srv://test3.test.build.10gen.cc/?directConnection=false"
seeds:
- localhost.test.build.10gen.cc:27017
hosts:
- localhost:27017
- localhost:27018
- localhost:27019
options:
ssl: true
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"uri": "mongodb+srv://test3.test.build.10gen.cc/?directConnection=true",
"seeds": [],
"hosts": [],
"error": true,
"comment": "Should fail because directConnection=true is incompatible with SRV URIs."
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
uri: "mongodb+srv://test3.test.build.10gen.cc/?directConnection=true"
seeds: []
hosts: []
error: true
comment: Should fail because directConnection=true is incompatible with SRV URIs.
2 changes: 2 additions & 0 deletions test/spec/server-discovery-and-monitoring/rs/compatible.yml
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
description: "Replica set member with large maxWireVersion"

uri: "mongodb://a,b/?replicaSet=rs"

phases: [
{
responses: [
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
description: "Replica set member and an unknown server"

uri: "mongodb://a,b/?replicaSet=rs"

phases: [
{
responses: [
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"description": "Discover arbiters",
"uri": "mongodb://a/?replicaSet=rs",
"description": "Discover arbiters with directConnection URI option",
"uri": "mongodb://a/?directConnection=false",
"phases": [
{
"responses": [
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
description: "Discover arbiters"
description: "Discover arbiters with directConnection URI option"

uri: "mongodb://a/?replicaSet=rs"
uri: "mongodb://a/?directConnection=false"

phases: [

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
{
"description": "Discover arbiters with replicaSet URI option",
"uri": "mongodb://a/?replicaSet=rs",
"phases": [
{
"responses": [
[
"a:27017",
{
"ok": 1,
"ismaster": true,
"hosts": [
"a:27017"
],
"arbiters": [
"b:27017"
],
"setName": "rs",
"minWireVersion": 0,
"maxWireVersion": 6
}
]
],
"outcome": {
"servers": {
"a:27017": {
"type": "RSPrimary",
"setName": "rs"
},
"b:27017": {
"type": "Unknown",
"setName": null
}
},
"topologyType": "ReplicaSetWithPrimary",
"logicalSessionTimeoutMinutes": null,
"setName": "rs"
}
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
description: "Discover arbiters with replicaSet URI option"

uri: "mongodb://a/?replicaSet=rs"

phases: [

{
responses: [

["a:27017", {

ok: 1,
ismaster: true,
hosts: ["a:27017"],
arbiters: ["b:27017"],
setName: "rs",
minWireVersion: 0,
maxWireVersion: 6
}]
],

outcome: {

servers: {

"a:27017": {

type: "RSPrimary",
setName: "rs"
},

"b:27017": {

type: "Unknown",
setName:
}
},
topologyType: "ReplicaSetWithPrimary",
logicalSessionTimeoutMinutes: null,
setName: "rs"
}
}
]
31 changes: 31 additions & 0 deletions test/spec/server-discovery-and-monitoring/rs/discover_ghost.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
{
"description": "Discover ghost with directConnection URI option",
"uri": "mongodb://b/?directConnection=false",
"phases": [
{
"responses": [
[
"b:27017",
{
"ok": 1,
"ismaster": false,
"isreplicaset": true,
"minWireVersion": 0,
"maxWireVersion": 6
}
]
],
"outcome": {
"servers": {
"b:27017": {
"type": "RSGhost",
"setName": null
}
},
"topologyType": "Unknown",
"logicalSessionTimeoutMinutes": null,
"setName": null
}
}
]
}
Loading