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 all 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) {
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
6 changes: 4 additions & 2 deletions lib/mongo_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ const { connect, validOptions } = require('./operations/connect');
* @param {number} [options.reconnectInterval=1000] Server will wait # milliseconds between retries
* @param {boolean} [options.ha=true] Control if high availability monitoring runs for Replicaset or Mongos proxies
* @param {number} [options.haInterval=10000] The High availability period for replicaset inquiry
* @param {string} [options.replicaSet=undefined] The Replicaset set name
* @param {string} [options.replicaSet] The name of the replica set to connect to
* @param {number} [options.secondaryAcceptableLatencyMS=15] Cutoff latency point in MS for Replicaset member selection
* @param {number} [options.acceptableLatencyMS=15] Cutoff latency point in MS for Mongos proxies selection
* @param {boolean} [options.connectWithNoPrimary=false] Sets if the driver should connect even if no primary is available
Expand Down Expand Up @@ -142,6 +142,7 @@ const { connect, validOptions } = require('./operations/connect');
* @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.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 {boolean} [options.directConnection=false] Whether to connect to the deployment in Single topology.
* @param {AutoEncrypter~AutoEncryptionOptions} [options.autoEncryption] Optionally enable client side auto encryption
* @returns {MongoClient} a MongoClient instance
*/
Expand Down Expand Up @@ -352,7 +353,7 @@ MongoClient.prototype.isConnected = function(options) {
* @param {number} [options.reconnectInterval=1000] Server will wait # milliseconds between retries
* @param {boolean} [options.ha=true] Control if high availability monitoring runs for Replicaset or Mongos proxies
* @param {number} [options.haInterval=10000] The High availability period for replicaset inquiry
* @param {string} [options.replicaSet=undefined] The Replicaset set name
* @param {string} [options.replicaSet] The name of the replica set to connect to
* @param {number} [options.secondaryAcceptableLatencyMS=15] Cutoff latency point in MS for Replicaset member selection
* @param {number} [options.acceptableLatencyMS=15] Cutoff latency point in MS for Mongos proxies selection
* @param {boolean} [options.connectWithNoPrimary=false] Sets if the driver should connect even if no primary is available
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] Whether to connect to the deployment in Single topology.
* @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
25 changes: 20 additions & 5 deletions lib/sdam/topology.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,8 @@ 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] Indicates that a client should directly connect to a node without attempting to discover its topology type
* @param {string} [options.replicaSet] The name of the replica set to connect to
*/
constructor(seedlist, options) {
super();
Expand Down Expand Up @@ -136,7 +138,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 +799,23 @@ function parseStringSeedlist(seedlist) {
}));
}

function topologyTypeFromSeedlist(seedlist, options) {
const replicaSet = options.replicaSet || options.setName || options.rs_name;
if (seedlist.length === 1 && !replicaSet) return TopologyType.Single;
if (replicaSet) return TopologyType.ReplicaSetNoPrimary;
/**
* Gets the TopologyType from the client options
*
* @param {object} options mongo client options
* @param {boolean} [options.directConnection] Indicates that a client should directly connect to a node without attempting to discover its topology type
* @param {string} [options.replicaSet] The name of the replica set to connect to
* @returns TopologyType
*/
function topologyTypeFromOptions(options) {
if (options.directConnection) {
return TopologyType.Single;
}

if (options.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