From ce4d7e4b9d81efc4e67006702231072b3cab5900 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 8 Nov 2021 17:05:42 -0500 Subject: [PATCH 01/30] feat(NODE-3467): implement srvMaxHosts, srvServiceName, and rescanSrvIntervalMS options --- .mocharc.json | 2 +- package-lock.json | 57 +++- package.json | 3 +- src/connection_string.ts | 64 ++++- src/mongo_client.ts | 15 + src/operations/connect.ts | 10 +- src/sdam/srv_polling.ts | 26 +- src/sdam/topology.ts | 13 +- src/sdam/topology_description.ts | 51 +++- src/utils.ts | 56 +++- test/functional/connection_leak_detector.js | 61 ---- test/functional/uri_options_spec.test.js | 52 +++- .../spec_prose.test.ts | 262 ++++++++++++++++++ test/tools/mock.js | 6 +- test/tools/utils.js | 3 + test/unit/connection_string.test.js | 136 +++++---- test/unit/utils.test.js | 31 ++- 17 files changed, 668 insertions(+), 180 deletions(-) delete mode 100644 test/functional/connection_leak_detector.js create mode 100644 test/integration/polling-srv-records-for-mongos-discovery/spec_prose.test.ts diff --git a/.mocharc.json b/.mocharc.json index 41b4e1feed..fad554261e 100644 --- a/.mocharc.json +++ b/.mocharc.json @@ -5,8 +5,8 @@ "ts" ], "require": [ - "ts-node/register", "source-map-support/register", + "ts-node/register", "test/tools/runner/chai-addons", "test/tools/runner/circular-dep-hack" ], diff --git a/package-lock.json b/package-lock.json index a584122759..5fd51c0f57 100644 --- a/package-lock.json +++ b/package-lock.json @@ -24,6 +24,7 @@ "@types/node": "^16.10.3", "@types/saslprep": "^1.0.1", "@types/semver": "^7.3.8", + "@types/sinon": "^10.0.6", "@types/whatwg-url": "^8.2.1", "@typescript-eslint/eslint-plugin": "^4.33.0", "@typescript-eslint/parser": "^4.33.0", @@ -46,7 +47,7 @@ "prettier": "^2.4.1", "rimraf": "^3.0.2", "semver": "^7.3.5", - "sinon": "^11.1.2", + "sinon": "^12.0.1", "sinon-chai": "^3.7.0", "source-map-support": "^0.5.20", "standard-version": "^9.3.1", @@ -1040,6 +1041,15 @@ "integrity": "sha512-D/2EJvAlCEtYFEYmmlGwbGXuK886HzyCc3nZX/tkFTQdEU8jZDAgiv08P162yB17y4ZXZoq7yFAnW4GDBb9Now==", "dev": true }, + "node_modules/@types/sinon": { + "version": "10.0.6", + "resolved": "https://registry.npmjs.org/@types/sinon/-/sinon-10.0.6.tgz", + "integrity": "sha512-6EF+wzMWvBNeGrfP3Nx60hhx+FfwSg1JJBLAAP/IdIUq0EYkqCYf70VT3PhuhPX9eLD+Dp+lNdpb/ZeHG8Yezg==", + "dev": true, + "dependencies": { + "@sinonjs/fake-timers": "^7.1.0" + } + }, "node_modules/@types/webidl-conversions": { "version": "6.1.1", "resolved": "https://registry.npmjs.org/@types/webidl-conversions/-/webidl-conversions-6.1.1.tgz", @@ -6020,13 +6030,13 @@ } }, "node_modules/sinon": { - "version": "11.1.2", - "resolved": "https://registry.npmjs.org/sinon/-/sinon-11.1.2.tgz", - "integrity": "sha512-59237HChms4kg7/sXhiRcUzdSkKuydDeTiamT/jesUVHshBgL8XAmhgFo0GfK6RruMDM/iRSij1EybmMog9cJw==", + "version": "12.0.1", + "resolved": "https://registry.npmjs.org/sinon/-/sinon-12.0.1.tgz", + "integrity": "sha512-iGu29Xhym33ydkAT+aNQFBINakjq69kKO6ByPvTsm3yyIACfyQttRTP03aBP/I8GfhFmLzrnKwNNkr0ORb1udg==", "dev": true, "dependencies": { "@sinonjs/commons": "^1.8.3", - "@sinonjs/fake-timers": "^7.1.2", + "@sinonjs/fake-timers": "^8.1.0", "@sinonjs/samsam": "^6.0.2", "diff": "^5.0.0", "nise": "^5.1.0", @@ -6047,6 +6057,15 @@ "sinon": ">=4.0.0" } }, + "node_modules/sinon/node_modules/@sinonjs/fake-timers": { + "version": "8.1.0", + "resolved": "https://registry.npmjs.org/@sinonjs/fake-timers/-/fake-timers-8.1.0.tgz", + "integrity": "sha512-OAPJUAtgeINhh/TAlUID4QTs53Njm7xzddaVlEs/SXwgtiD1tW22zAB/W1wdqfrpmikgaWQ9Fw6Ws+hsiRm5Vg==", + "dev": true, + "dependencies": { + "@sinonjs/commons": "^1.7.0" + } + }, "node_modules/slash": { "version": "3.0.0", "resolved": "https://registry.npmjs.org/slash/-/slash-3.0.0.tgz", @@ -8334,6 +8353,15 @@ "integrity": "sha512-D/2EJvAlCEtYFEYmmlGwbGXuK886HzyCc3nZX/tkFTQdEU8jZDAgiv08P162yB17y4ZXZoq7yFAnW4GDBb9Now==", "dev": true }, + "@types/sinon": { + "version": "10.0.6", + "resolved": "https://registry.npmjs.org/@types/sinon/-/sinon-10.0.6.tgz", + "integrity": "sha512-6EF+wzMWvBNeGrfP3Nx60hhx+FfwSg1JJBLAAP/IdIUq0EYkqCYf70VT3PhuhPX9eLD+Dp+lNdpb/ZeHG8Yezg==", + "dev": true, + "requires": { + "@sinonjs/fake-timers": "^7.1.0" + } + }, "@types/webidl-conversions": { "version": "6.1.1", "resolved": "https://registry.npmjs.org/@types/webidl-conversions/-/webidl-conversions-6.1.1.tgz", @@ -12089,17 +12117,28 @@ } }, "sinon": { - "version": "11.1.2", - "resolved": "https://registry.npmjs.org/sinon/-/sinon-11.1.2.tgz", - "integrity": "sha512-59237HChms4kg7/sXhiRcUzdSkKuydDeTiamT/jesUVHshBgL8XAmhgFo0GfK6RruMDM/iRSij1EybmMog9cJw==", + "version": "12.0.1", + "resolved": "https://registry.npmjs.org/sinon/-/sinon-12.0.1.tgz", + "integrity": "sha512-iGu29Xhym33ydkAT+aNQFBINakjq69kKO6ByPvTsm3yyIACfyQttRTP03aBP/I8GfhFmLzrnKwNNkr0ORb1udg==", "dev": true, "requires": { "@sinonjs/commons": "^1.8.3", - "@sinonjs/fake-timers": "^7.1.2", + "@sinonjs/fake-timers": "^8.1.0", "@sinonjs/samsam": "^6.0.2", "diff": "^5.0.0", "nise": "^5.1.0", "supports-color": "^7.2.0" + }, + "dependencies": { + "@sinonjs/fake-timers": { + "version": "8.1.0", + "resolved": "https://registry.npmjs.org/@sinonjs/fake-timers/-/fake-timers-8.1.0.tgz", + "integrity": "sha512-OAPJUAtgeINhh/TAlUID4QTs53Njm7xzddaVlEs/SXwgtiD1tW22zAB/W1wdqfrpmikgaWQ9Fw6Ws+hsiRm5Vg==", + "dev": true, + "requires": { + "@sinonjs/commons": "^1.7.0" + } + } } }, "sinon-chai": { diff --git a/package.json b/package.json index d24788b494..fe686c85aa 100644 --- a/package.json +++ b/package.json @@ -47,6 +47,7 @@ "@types/node": "^16.10.3", "@types/saslprep": "^1.0.1", "@types/semver": "^7.3.8", + "@types/sinon": "^10.0.6", "@types/whatwg-url": "^8.2.1", "@typescript-eslint/eslint-plugin": "^4.33.0", "@typescript-eslint/parser": "^4.33.0", @@ -69,7 +70,7 @@ "prettier": "^2.4.1", "rimraf": "^3.0.2", "semver": "^7.3.5", - "sinon": "^11.1.2", + "sinon": "^12.0.1", "sinon-chai": "^3.7.0", "source-map-support": "^0.5.20", "standard-version": "^9.3.1", diff --git a/src/connection_string.ts b/src/connection_string.ts index c188e2df85..01f09ecb65 100644 --- a/src/connection_string.ts +++ b/src/connection_string.ts @@ -34,7 +34,13 @@ import { PromiseProvider } from './promise_provider'; import { Encrypter } from './encrypter'; import { Compressor, CompressorName } from './cmap/wire_protocol/compression'; -const VALID_TXT_RECORDS = ['authSource', 'replicaSet', 'loadBalanced']; +const VALID_TXT_RECORDS = [ + 'authSource', + 'replicaSet', + 'loadBalanced', + 'srvMaxHosts', + 'srvServiceName' +]; const LB_SINGLE_HOST_ERROR = 'loadBalanced option only supported with a single host in the URI'; const LB_REPLICA_SET_ERROR = 'loadBalanced option not supported with a replicaSet option'; @@ -75,7 +81,7 @@ export function resolveSRVRecord(options: MongoOptions, callback: Callback { + dns.resolveSrv(`_${options.srvServiceName}._tcp.${lookupAddress}`, (err, addresses) => { if (err) return callback(err); if (addresses.length === 0) { @@ -116,12 +122,22 @@ export function resolveSRVRecord(options: MongoOptions, callback: Callback txtRecordOptions.get(option) === '')) { + return callback(new MongoParseError('Cannot have empty URI params in DNS TXT Record')); + } + const source = txtRecordOptions.get('authSource') ?? undefined; const replicaSet = txtRecordOptions.get('replicaSet') ?? undefined; const loadBalanced = txtRecordOptions.get('loadBalanced') ?? undefined; - - if (source === '' || replicaSet === '') { - return callback(new MongoParseError('Cannot have empty URI params in DNS TXT Record')); + const srvMaxHostsString = txtRecordOptions.get('srvMaxHosts') ?? undefined; + // const srvServiceName = txtRecordOptions.get('srvServiceName') ?? undefined; + + if (srvMaxHostsString) { + try { + options.srvMaxHosts = getUint('srvMaxHosts', txtRecordOptions.get('srvMaxHosts')); + } catch (error) { + return callback(error); + } } if (!options.userSpecifiedAuthSource && source) { @@ -132,6 +148,14 @@ export function resolveSRVRecord(options: MongoOptions, callback: Callback([ ...urlOptions.keys(), ...objectOptions.keys(), @@ -366,6 +394,14 @@ export function parseOptions( setOption(mongoOptions, key, descriptor, values); } + if ( + typeof mongoOptions.srvMaxHosts === 'number' && + mongoOptions.srvMaxHosts !== 0 && + typeof mongoOptions.replicaSet === 'string' + ) { + throw new MongoParseError('Cannot combine replicaSet option and maxSrvHosts'); + } + if (mongoOptions.credentials) { const isGssapi = mongoOptions.credentials.mechanism === AuthMechanism.MONGODB_GSSAPI; const isX509 = mongoOptions.credentials.mechanism === AuthMechanism.MONGODB_X509; @@ -439,6 +475,10 @@ function validateLoadBalancedOptions( if (mongoOptions.directConnection) { return new MongoParseError(LB_DIRECT_CONNECTION_ERROR); } + + if (mongoOptions.srvHost && mongoOptions.srvMaxHosts) { + return new MongoParseError('Cannot limit srv hosts with loadBalanced enabled'); + } } } @@ -902,6 +942,7 @@ export const OPTIONS = { replicaSet: { type: 'string' }, + rescanSrvIntervalMS: { type: 'uint', default: 60000 }, retryReads: { default: true, type: 'boolean' @@ -924,6 +965,13 @@ export const OPTIONS = { default: 0, type: 'uint' }, + srvMaxHosts: { + type: 'uint' + }, + srvServiceName: { + type: 'string', + default: 'mongodb' + }, ssl: { target: 'tls', type: 'boolean' diff --git a/src/mongo_client.ts b/src/mongo_client.ts index b57563f51d..6e7dfba8fe 100644 --- a/src/mongo_client.ts +++ b/src/mongo_client.ts @@ -132,6 +132,18 @@ export interface MongoClientOptions extends BSONSerializeOptions, SupportedNodeC compressors?: CompressorName[] | string; /** An integer that specifies the compression level if using zlib for network compression. */ zlibCompressionLevel?: 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | undefined; + /** The maximum number of hosts to connect to when using an srv connection string */ + srvMaxHosts?: number; + /** + * Modifies the srv URI to look like: + * + * `_{srvServiceName}._tcp.{hostname}.{domainname}` + * + * Querying this DNS URI is expected to respond with SRV records + */ + srvServiceName?: string; + /** Frequency with which to scan SRV record changes */ + rescanSrvIntervalMS?: number; /** The maximum number of connections in the connection pool. */ maxPoolSize?: number; /** The minimum number of connections in the connection pool. */ @@ -643,9 +655,12 @@ export interface MongoOptions | 'retryWrites' | 'serverSelectionTimeoutMS' | 'socketTimeoutMS' + | 'srvMaxHosts' + | 'srvServiceName' | 'tlsAllowInvalidCertificates' | 'tlsAllowInvalidHostnames' | 'tlsInsecure' + | 'rescanSrvIntervalMS' | 'waitQueueTimeoutMS' | 'zlibCompressionLevel' > diff --git a/src/operations/connect.ts b/src/operations/connect.ts index 7a1241130b..ddd52c3e90 100644 --- a/src/operations/connect.ts +++ b/src/operations/connect.ts @@ -1,7 +1,7 @@ import { MongoRuntimeError, MongoInvalidArgumentError } from '../error'; import { Topology, TOPOLOGY_EVENTS } from '../sdam/topology'; import { resolveSRVRecord } from '../connection_string'; -import type { Callback } from '../utils'; +import { Callback, shuffle } from '../utils'; import type { MongoClient, MongoOptions } from '../mongo_client'; import { CMAP_EVENTS } from '../cmap/connection_pool'; import { APM_EVENTS } from '../cmap/connection'; @@ -51,7 +51,13 @@ export function connect( if (typeof options.srvHost === 'string') { return resolveSRVRecord(options, (err, hosts) => { if (err || !hosts) return callback(err); - for (const [index, host] of hosts.entries()) { + + const selectedHosts = + options.srvMaxHosts === 0 || options.srvMaxHosts >= hosts.length + ? hosts + : shuffle(hosts, options.srvMaxHosts); + + for (const [index, host] of selectedHosts.entries()) { options.hosts[index] = host; } diff --git a/src/sdam/srv_polling.ts b/src/sdam/srv_polling.ts index 119bcc597b..956d170b63 100644 --- a/src/sdam/srv_polling.ts +++ b/src/sdam/srv_polling.ts @@ -29,18 +29,16 @@ export class SrvPollingEvent { this.srvRecords = srvRecords; } - addresses(): Map { - return new Map( - this.srvRecords.map(record => { - const host = new HostAddress(`${record.name}:${record.port}`); - return [host.toString(), host]; - }) - ); + hostnames(): Set { + return new Set(this.srvRecords.map(r => HostAddress.fromSrvRecord(r).toString())); } } /** @internal */ export interface SrvPollerOptions extends LoggerOptions { + rescanSrvIntervalMS?: number; + srvServiceName: string; + srvMaxHosts: number; srvHost: string; heartbeatFrequencyMS: number; } @@ -58,6 +56,8 @@ export class SrvPoller extends TypedEventEmitter { logger: Logger; haMode: boolean; generation: number; + srvMaxHosts: number; + srvServiceName: string; _timeout?: NodeJS.Timeout; /** @event */ @@ -71,8 +71,10 @@ export class SrvPoller extends TypedEventEmitter { } this.srvHost = options.srvHost; - this.rescanSrvIntervalMS = 60000; - this.heartbeatFrequencyMS = options.heartbeatFrequencyMS || 10000; + this.srvMaxHosts = options.srvMaxHosts; + this.srvServiceName = options.srvServiceName; + this.rescanSrvIntervalMS = options.rescanSrvIntervalMS ?? 60000; + this.heartbeatFrequencyMS = options.heartbeatFrequencyMS ?? 10000; this.logger = new Logger('srvPoller', options); this.haMode = false; @@ -82,7 +84,7 @@ export class SrvPoller extends TypedEventEmitter { } get srvAddress(): string { - return `_mongodb._tcp.${this.srvHost}`; + return `_${this.srvServiceName}._tcp.${this.srvHost}`; } get intervalMS(): number { @@ -143,13 +145,13 @@ export class SrvPoller extends TypedEventEmitter { } const finalAddresses: dns.SrvRecord[] = []; - srvRecords.forEach((record: dns.SrvRecord) => { + for (const record of srvRecords) { if (matchesParentDomain(record.name, this.srvHost)) { finalAddresses.push(record); } else { this.parentDomainMismatch(record); } - }); + } if (!finalAddresses.length) { this.failure('No valid addresses found at host'); diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index fa2551faa5..448f9b7c5a 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -140,6 +140,9 @@ export interface TopologyPrivate { /** @public */ export interface TopologyOptions extends BSONSerializeOptions, ServerOptions { + srvMaxHosts: number; + srvServiceName: string; + rescanSrvIntervalMS: number; hosts: HostAddress[]; retryWrites: boolean; retryReads: boolean; @@ -339,7 +342,10 @@ export class Topology extends TypedEventEmitter { options.srvPoller ?? new SrvPoller({ heartbeatFrequencyMS: this.s.heartbeatFrequencyMS, - srvHost: options.srvHost + srvHost: options.srvHost, + srvMaxHosts: options.srvMaxHosts, + srvServiceName: options.srvServiceName, + rescanSrvIntervalMS: options.rescanSrvIntervalMS }); this.on(Topology.TOPOLOGY_DESCRIPTION_CHANGED, this.s.detectShardedTopology); @@ -363,7 +369,10 @@ export class Topology extends TypedEventEmitter { private detectSrvRecords(ev: SrvPollingEvent) { const previousTopologyDescription = this.s.description; - this.s.description = this.s.description.updateFromSrvPollingEvent(ev); + this.s.description = this.s.description.updateFromSrvPollingEvent( + ev, + this.s.options.srvMaxHosts + ); if (this.s.description === previousTopologyDescription) { // Nothing changed, so return return; diff --git a/src/sdam/topology_description.ts b/src/sdam/topology_description.ts index 60ad5e689f..7036a2966e 100644 --- a/src/sdam/topology_description.ts +++ b/src/sdam/topology_description.ts @@ -4,6 +4,7 @@ import { TopologyType, ServerType } from './common'; import type { ObjectId, Document } from '../bson'; import type { SrvPollingEvent } from './srv_polling'; import { MongoError, MongoRuntimeError } from '../error'; +import { shuffle } from '../utils'; // constants related to compatibility checks const MIN_SUPPORTED_SERVER_VERSION = WIRE_CONSTANTS.MIN_SUPPORTED_SERVER_VERSION; @@ -139,23 +140,51 @@ export class TopologyDescription { * Returns a new TopologyDescription based on the SrvPollingEvent * @internal */ - updateFromSrvPollingEvent(ev: SrvPollingEvent): TopologyDescription { - const newAddresses = ev.addresses(); - const serverDescriptions = new Map(this.servers); - for (const address of this.servers.keys()) { - if (newAddresses.has(address)) { - newAddresses.delete(address); - } else { - serverDescriptions.delete(address); + updateFromSrvPollingEvent(ev: SrvPollingEvent, srvMaxHosts: number): TopologyDescription { + /** The SRV addresses defines the set of addresses we should be using */ + const incomingHostnames = ev.hostnames(); + const currentHostnames = new Set(this.servers.keys()); + + const hostnamesToRemove = new Set(); + for (const hostname of currentHostnames) { + if (!incomingHostnames.has(hostname)) { + // If the SRV Records no longer include this hostname + // we have to stop using it + hostnamesToRemove.add(hostname); + } + } + + const hostnamesToAdd = new Set(); + for (const hostname of incomingHostnames) { + if (!currentHostnames.has(hostname)) { + // There are new hosts in the SRV Record! + hostnamesToAdd.add(hostname); } } - if (serverDescriptions.size === this.servers.size && newAddresses.size === 0) { + if (hostnamesToAdd.size === 0 && hostnamesToRemove.size === 0) { + // No new hosts to add and none to remove return this; } - for (const [address, host] of newAddresses) { - serverDescriptions.set(address, new ServerDescription(host)); + const serverDescriptions = new Map(this.servers); + for (const removedHost of hostnamesToRemove) { + serverDescriptions.delete(removedHost); + } + + if (hostnamesToAdd.size > 0) { + if (srvMaxHosts === 0) { + // Add all! + for (const hostToAdd of hostnamesToAdd) { + serverDescriptions.set(hostToAdd, new ServerDescription(hostToAdd)); + } + } else if (serverDescriptions.size < srvMaxHosts) { + // Add only the amount needed to get us back to srvMaxHosts + const selectedHosts = shuffle(hostnamesToAdd, srvMaxHosts - serverDescriptions.size); + for (const selectedHostToAdd of selectedHosts) { + serverDescriptions.set(selectedHostToAdd, new ServerDescription(selectedHostToAdd)); + } + } } return new TopologyDescription( diff --git a/src/utils.ts b/src/utils.ts index eb275e862d..bef5251420 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -28,6 +28,7 @@ import type { CommandOperationOptions, OperationParent } from './operations/comm import { ReadPreference } from './read_preference'; import { URL } from 'url'; import { MAX_SUPPORTED_WIRE_VERSION } from './cmap/wire_protocol/constants'; +import type { SrvRecord } from 'dns'; /** * MongoDB Driver style callback @@ -1095,8 +1096,9 @@ export function isSuperset(set: Set | any[], subset: Set | any[]): boo return true; } -export function setDifference(setA: Iterable, setB: Iterable): Set { - const difference = new Set(setA); +/** Returns the items that are uniquely in setA */ +export function setDifference(setA: Iterable, setB: Iterable): Set { + const difference = new Set(setA); for (const elem of setB) { difference.delete(elem); } @@ -1319,6 +1321,23 @@ export class HostAddress { Object.freeze(this); } + equals(other: HostAddress): boolean { + return ( + this.host === other.host && + this.port === other.port && + this.socketPath === other.socketPath && + this.isIPv6 === other.isIPv6 + ); + } + + [Symbol.for('nodejs.util.inspect.custom')](): string { + return this.inspect(); + } + + inspect(): string { + return `new HostAddress('${this.toString(true)}')`; + } + /** * @param ipv6Brackets - optionally request ipv6 bracket notation required for connection strings */ @@ -1335,6 +1354,10 @@ export class HostAddress { static fromString(s: string): HostAddress { return new HostAddress(s); } + + static fromSrvRecord({ name, port }: SrvRecord): HostAddress { + return HostAddress.fromString(`${name}:${port}`); + } } export const DEFAULT_PK_FACTORY = { @@ -1405,3 +1428,32 @@ export function parsePackageVersion({ version }: { version: string }): { const [major, minor, patch] = version.split('.').map((n: string) => Number.parseInt(n, 10)); return { major, minor, patch }; } + +/** + * Fisher–Yates Shuffle + * + * Reference: https://bost.ocks.org/mike/shuffle/ + * @param sequence - items to be shuffled + * @param limit - the number of ite + */ +export function shuffle(sequence: Iterable, limit = 0): Array { + const items = Array.isArray(sequence) ? sequence : Array.from(sequence); + + if (limit > items.length) { + throw new MongoInvalidArgumentError('Limit must be less than the number of items'); + } + + let remainingItemsToShuffle = items.length; + while (remainingItemsToShuffle !== 0) { + // Pick a remaining element + const randomIndex = Math.floor(Math.random() * remainingItemsToShuffle); + remainingItemsToShuffle -= 1; + + // And swap it with the current element + const swapHold = items[remainingItemsToShuffle]; + items[remainingItemsToShuffle] = items[randomIndex]; + items[randomIndex] = swapHold; + } + + return limit === 0 ? items : items.slice(0, limit); +} diff --git a/test/functional/connection_leak_detector.js b/test/functional/connection_leak_detector.js deleted file mode 100644 index 38b3fa44bf..0000000000 --- a/test/functional/connection_leak_detector.js +++ /dev/null @@ -1,61 +0,0 @@ -'use strict'; - -// Disabled for now b/c it conflicts with session leak tests - -// before(function() { -// this.client = this.configuration.newClient({}, { maxPoolSize: 1 }); - -// return Promise.resolve() -// .then(() => this.client.connect()) -// .then(() => { -// this.adminDb = this.client.db(this.configuration.db).admin(); -// return this.adminDb.serverStatus(); -// }) -// .then(serverStatus => { -// this._currentConnections = serverStatus.connections.current; -// this._connectionChangedTests = []; -// }); -// }); - -// beforeEach(function() { -// return Promise.resolve() -// .then(() => this.adminDb.serverStatus()) -// .then(serverStatus => { -// this._currentConnections = serverStatus.connections.current; -// }); -// }); - -// afterEach(function() { -// return Promise.resolve() -// .then(() => this.adminDb.serverStatus()) -// .then(serverStatus => { -// const currentConnections = serverStatus.connections.current; -// if (this._currentConnections !== currentConnections) { -// console.log('connections: ', this._currentConnections, '-->', currentConnections); -// this._connectionChangedTests.push({ -// test: this.currentTest, -// previous: this._currentConnections, -// current: currentConnections -// }); -// } - -// this._currentConnections = currentConnections; -// }); -// }); - -// after(function() { -// return this.client.close().then(() => { -// if (this._connectionChangedTests.length) { -// console.group('The following tests had unstable connection counts:'); -// console.log('| previous | current | name |'); -// console.log('| -------- | ---- | ---- |'); -// this._connectionChangedTests.forEach(({ test, previous, current }) => { -// const name = test.fullTitle(); -// previous = previous.toString(10).padStart(8); -// current = current.toString(10).padStart(4); -// console.log(`| ${previous} | ${current} | ${name} |`); -// }); -// console.groupEnd(); -// } -// }); -// }); diff --git a/test/functional/uri_options_spec.test.js b/test/functional/uri_options_spec.test.js index e9e8f135fc..c8dc73f825 100644 --- a/test/functional/uri_options_spec.test.js +++ b/test/functional/uri_options_spec.test.js @@ -1,16 +1,18 @@ 'use strict'; -const chai = require('chai'); -const expect = chai.expect; -chai.use(require('chai-subset')); +const { expect } = require('chai'); +const { promisify } = require('util'); +require('chai').use(require('chai-subset')); -const { parseOptions } = require('../../src/connection_string'); +const { parseOptions, resolveSRVRecord } = require('../../src/connection_string'); const { loadSpecTests } = require('../spec'); describe('URI Options (spec)', function () { - loadSpecTests('uri-options').forEach(suite => { + const uriSpecs = loadSpecTests('uri-options'); + + uriSpecs.forEach(suite => { describe(suite.name, () => { - suite.tests.forEach(test => { + for (const test of suite.tests) { const itFn = test.warning ? it.skip : it; itFn(`${test.description}`, function () { @@ -24,10 +26,46 @@ describe('URI Options (spec)', function () { expect(options).to.containSubset(test.options); } } catch (err) { + if (test.warning === false || test.valid === true) { + // This test case is supposed to pass + this.skip(); + } expect(err).to.be.an.instanceof(Error); } }); - }); + } }); }); + + describe('srvMaxHost manual testing', function () { + const srvMaxHostTests = uriSpecs.find(testFolder => testFolder.name === 'srv-options').tests; + + for (const test of srvMaxHostTests) { + it(test.description, async function () { + let thrownError; + let driverOptions; + let hosts; + try { + driverOptions = parseOptions(test.uri); + hosts = await promisify(resolveSRVRecord)(driverOptions); + } catch (error) { + thrownError = error; + } + + if (test.valid === false || test.warning === true) { + // We implement warnings as errors + expect(thrownError).to.exist; + expect(hosts).to.not.exist; + return; // Nothing more to test... + } + + expect(thrownError).to.not.exist; + expect(driverOptions).to.exist; + + for (const [testOptionKey, testOptionValue] of Object.entries(test.options)) { + expect(driverOptions).to.have.property(testOptionKey, testOptionValue); + } + }); + } + }); }); diff --git a/test/integration/polling-srv-records-for-mongos-discovery/spec_prose.test.ts b/test/integration/polling-srv-records-for-mongos-discovery/spec_prose.test.ts new file mode 100644 index 0000000000..a32e6519e1 --- /dev/null +++ b/test/integration/polling-srv-records-for-mongos-discovery/spec_prose.test.ts @@ -0,0 +1,262 @@ +import * as path from 'path'; +import * as dns from 'dns'; +import * as sinon from 'sinon'; +import { expect } from 'chai'; +import { MongoClient } from '../../../src'; +import { sleep } from '../../tools/utils'; +import { it } from 'mocha'; +import * as mock from '../../tools/mock'; + +/* + The SRV Prose Tests make use of the following REAL DNS records. + We use sinon to replace the results from resolveSrv to test hostname removals and insertions. + + The prose tests assume you have a 4 node sharded cluster running on ports: + [27017, 27018, 27019, 27020] + + Record TTL Class Address + localhost.test.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.test3.test.build.10gen.cc. 86400 IN SRV 27017 localhost.test.build.10gen.cc. +*/ + +// const makeSrvRecordName = (host, serviceName = 'mongodb') => `_${serviceName}._tcp.${host}`; +// const dnsResolveSrvAsync = promisify(dns.resolveSrv); +const srvRecord = (name, port) => ({ name, port, weight: 0, priority: 0 }); +interface ShardedClusterMocks { + mongoses: mock.MockServer[]; + readonly srvRecords: dns.SrvRecord[]; +} + +describe(path.basename(__dirname), () => { + describe('prose tests', () => { + // TODO(): Make use of the shared driver's DNS records + // const SRV_CONNECTION_STRING_T1 = 'mongodb+srv://test1.test.build.10gen.cc'; + // const SRV_CONNECTION_STRING_t3 = 'mongodb+srv://test3.test.build.10gen.cc'; + const SRV_CONNECTION_STRING = 'mongodb+srv://my.fancy.prose.tests'; + let shardedCluster: ShardedClusterMocks; + let resolveSrvStub: sinon.SinonStub; + let lookupStub: sinon.SinonStub; + let client: MongoClient; + let dnsRecordsForTest1; + //let dnsRecordsForTest3; + + beforeEach(async () => { + const mongoses = [ + await mock.createServer(2000), + await mock.createServer(2001), + await mock.createServer(2002), + await mock.createServer(2003) + ]; + + const srvRecords = mongoses.map(s => srvRecord('localhost.my.fancy.prose.tests', s.port)); + + shardedCluster = { mongoses, srvRecords }; + + for (const mongos of shardedCluster.mongoses) { + mongos.setMessageHandler(request => { + const document = request.document; + + if (document.ismaster || document.hello) { + request.reply({ ...mock.HELLO, msg: 'isdbgrid' }); + } + }); + } + }); + + afterEach(async () => { + await mock.cleanup(); + }); + + before(async () => { + // const srvTest1 = makeSrvRecordName('test1.test.build.10gen.cc'); + // const srvTest3 = makeSrvRecordName('test3.test.build.10gen.cc'); + dnsRecordsForTest1 = [ + { name: 'localhost.my.fancy.prose.tests', port: 2000 }, + { name: 'localhost.my.fancy.prose.tests', port: 2001 } + ]; // await dnsResolveSrvAsync(srvTest1); + // dnsRecordsForTest3 = await dnsResolveSrvAsync(srvTest3); + }); + + afterEach(async () => { + if (resolveSrvStub) { + resolveSrvStub.restore(); + resolveSrvStub = undefined; + } + if (lookupStub) { + lookupStub.restore(); + lookupStub = undefined; + } + if (client) { + await client.close(); + } + }); + + function makeStubs({ + replacementRecords = undefined, + mockLimit = 0, + srvServiceName = 'mongodb' + }) { + let initialDNSLookup = true; + const mockRecords = shardedCluster.srvRecords; + replacementRecords ??= mockRecords; + // for some reason the stub needs to be set up here. + // first call is for the driver initial connection + // second call will check the poller + resolveSrvStub = sinon.stub(dns, 'resolveSrv').callsFake((address, callback) => { + expect(address).to.equal(`_${srvServiceName}._tcp.my.fancy.prose.tests`); + if (initialDNSLookup) { + initialDNSLookup = false; + const recordsToUse = mockRecords.slice( + 0, + mockLimit === 0 ? mockRecords.length : mockLimit + ); + + return process.nextTick(callback, null, recordsToUse); + } + process.nextTick(callback, null, replacementRecords); + }); + + lookupStub = sinon.stub(dns, 'lookup').callsFake((...args) => { + const hostname = args[0]; + const options = typeof args[1] === 'object' ? args[1] : {}; + const callback = args[args.length - 1] as (err: null, address: string, family: 4) => void; + + if (hostname.includes('my.fancy.prose.tests')) { + return process.nextTick(() => { + callback(null, '127.0.0.1', 4); + }); + } + + const { wrappedMethod: lookup } = lookupStub; + lookup(hostname, options, callback); + }); + } + + it('10 - All DNS records are selected (srvMaxHosts = 0)', async () => { + const replacementRecords = [ + { name: 'localhost.my.fancy.prose.tests', port: 2000, weight: 0, priority: 0 }, + { name: 'localhost.my.fancy.prose.tests', port: 2002, weight: 0, priority: 0 }, + { name: 'localhost.my.fancy.prose.tests', port: 2003, weight: 0, priority: 0 } + ]; + + makeStubs({ replacementRecords, mockLimit: 2 }); + + client = new MongoClient(SRV_CONNECTION_STRING, { + tls: false, + srvMaxHosts: 0, + serverSelectionTimeoutMS: 5000, + rescanSrvIntervalMS: 60 + }); + await client.connect(); + + const selectedHosts = client.topology.s.seedlist; + expect(selectedHosts).to.have.lengthOf(dnsRecordsForTest1.length); + expect(selectedHosts.map(({ host }) => host)).to.deep.equal( + dnsRecordsForTest1.map(({ name }) => name) + ); + + await sleep(2 * client.topology.s.srvPoller.intervalMS); + + const polledServerAddresses = Array.from(client.topology.description.servers.keys()); + polledServerAddresses.sort(); + expect(polledServerAddresses).to.deep.equal( + replacementRecords.map(({ name, port }) => `${name}:${port}`) + ); + }); + + it('11 - All DNS records are selected (srvMaxHosts >= records)', async () => { + const replacementRecords = [ + { name: 'localhost.my.fancy.prose.tests', port: 2002, weight: 0, priority: 0 }, + { name: 'localhost.my.fancy.prose.tests', port: 2003, weight: 0, priority: 0 } + ]; + + makeStubs({ replacementRecords, mockLimit: 2 }); + + client = new MongoClient(SRV_CONNECTION_STRING, { + tls: false, + srvMaxHosts: 2, + serverSelectionTimeoutMS: 5000, + rescanSrvIntervalMS: 60 + }); + await client.connect(); + + const selectedHosts = client.topology.s.seedlist; + expect(selectedHosts).to.have.lengthOf(2); + expect(selectedHosts.map(({ host }) => host)).to.deep.equal( + dnsRecordsForTest1.map(({ name }) => name) + ); + + await sleep(2 * client.topology.s.srvPoller.intervalMS); + + const polledServerAddresses = Array.from(client.topology.description.servers.keys()); + polledServerAddresses.sort(); + expect(polledServerAddresses).to.deep.equal( + replacementRecords.map(({ name, port }) => `${name}:${port}`) + ); + }); + + it('12 - New DNS records are randomly selected (srvMaxHosts > 0)', async () => { + const replacementRecords = [ + { name: 'localhost.my.fancy.prose.tests', port: 2000, weight: 0, priority: 0 }, + { name: 'localhost.my.fancy.prose.tests', port: 2002, weight: 0, priority: 0 }, + { name: 'localhost.my.fancy.prose.tests', port: 2003, weight: 0, priority: 0 } + ]; + + makeStubs({ replacementRecords, mockLimit: 2 }); + + client = new MongoClient(SRV_CONNECTION_STRING, { + tls: false, + srvMaxHosts: 2, + serverSelectionTimeoutMS: 5000, + rescanSrvIntervalMS: 60 + }); + await client.connect(); + + const selectedHosts = client.topology.s.seedlist; + expect(selectedHosts).to.have.lengthOf(2); + expect(selectedHosts.map(({ host }) => host)).to.deep.equal( + dnsRecordsForTest1.map(({ name }) => name) + ); + + await sleep(2 * client.topology.s.srvPoller.intervalMS); + + const polledServerAddresses = Array.from(client.topology.description.servers.keys()); + polledServerAddresses.sort(); + // Only two addresses, one should remain the original 2000, + // while the other will be one of 2002 or 2003 + expect(polledServerAddresses).to.have.lengthOf(2); + expect(polledServerAddresses.find(addr => addr.includes('2000'))).to.be.a('string'); + expect(polledServerAddresses).satisfies( + addresses => + // If you want proof, comment one of these conditions out, and run the test a few times + // you should see it pass and fail at random + typeof addresses.find(addr => addr.includes('2002')) === 'string' || + typeof addresses.find(addr => addr.includes('2003')) === 'string' + ); + }); + + it('13 - DNS record with custom service name can be found', async () => { + client = new MongoClient(SRV_CONNECTION_STRING, { + tls: false, + srvServiceName: 'myFancySrvServiceName', + serverSelectionTimeoutMS: 5000, + rescanSrvIntervalMS: 60 + }); + + makeStubs({ srvServiceName: 'myFancySrvServiceName' }); + + await client.connect(); + + await sleep(2 * client.topology.s.srvPoller.intervalMS); + + const resolveSrvCalls = resolveSrvStub.getCalls(); + expect(resolveSrvCalls).to.have.lengthOf(2); + expect(resolveSrvCalls[0].args[0]).includes('myFancySrvServiceName'); + expect(resolveSrvCalls[1].args[0]).include('myFancySrvServiceName'); + }); + }); +}); diff --git a/test/tools/mock.js b/test/tools/mock.js index 72dede7826..c13ec860cc 100644 --- a/test/tools/mock.js +++ b/test/tools/mock.js @@ -49,9 +49,9 @@ const { HostAddress } = require('../../src/utils'); /** * Make a mock mongodb server. * - * @param {number} port - port number - * @param {string} host - address - * @param {object} options - options + * @param {number} [port] - port number + * @param {string} [host] - address + * @param {object} [options] - options * @returns {Promise} */ function createServer(port, host, options) { diff --git a/test/tools/utils.js b/test/tools/utils.js index b5101ed046..f55e624a27 100644 --- a/test/tools/utils.js +++ b/test/tools/utils.js @@ -337,7 +337,10 @@ const runLater = (fn, ms) => { }); }; +const sleep = ms => new Promise(resolve => setTimeout(resolve, ms)); + module.exports = { + sleep, runLater, ejson, EventCollector, diff --git a/test/unit/connection_string.test.js b/test/unit/connection_string.test.js index 647599c64e..5c766a9fbc 100644 --- a/test/unit/connection_string.test.js +++ b/test/unit/connection_string.test.js @@ -2,6 +2,7 @@ const fs = require('fs'); const path = require('path'); +const { promisify } = require('util'); const { MongoParseError, MongoDriverError, MongoInvalidArgumentError } = require('../../src/error'); const { loadSpecTests } = require('../spec'); const { parseOptions, resolveSRVRecord } = require('../../src/connection_string'); @@ -224,8 +225,8 @@ describe('Connection String', function () { expect(options.srvHost).to.equal('test1.test.build.10gen.cc'); }); - SPEC_PATHS.forEach(function (folder) { - describe('spec tests', function () { + for (const folder of SPEC_PATHS) { + describe(`spec tests ${folder}`, function () { const specPath = path.join(__dirname, '../spec', 'initial-dns-seedlist-discovery', folder); const testFiles = fs .readdirSync(specPath) @@ -233,72 +234,87 @@ describe('Connection String', function () { .map(x => [x, fs.readFileSync(path.join(specPath, x), 'utf8')]) .map(x => [path.basename(x[0], '.json'), JSON.parse(x[1])]); - testFiles.forEach(test => { - if (!test[1].comment) { - test[1].comment = test[0]; + for (const [fileName, test] of testFiles) { + if (!test.comment) { + test.comment = fileName; } - const comment = test[1].comment; - const skipTest = comment.search(/^(srvMaxHosts|srv-service-name)/) > -1; - const maybeIt = skipTest ? it.skip : it; - // TODO: NODE-3467: Implement maxSrvHosts work - maybeIt(comment, { + const comment = test.comment; + it(comment, { metadata: { requires: { topology: ['single'] } }, - test: function (done) { + async test() { + let thrownError; + let options; + let hosts; try { - const options = parseOptions(test[1].uri); - resolveSRVRecord(options, (err, result) => { - if (test[1].error) { - expect(err).to.exist; - expect(result).to.not.exist; - } else { - expect(err).to.not.exist; - expect(result).to.exist; - // Implicit SRV options must be set. - expect(options.directConnection).to.be.false; - const testOptions = test[1].options; - if (testOptions && 'tls' in testOptions) { - expect(options).to.have.property('tls', testOptions.tls); - } else if (testOptions && 'ssl' in testOptions) { - expect(options).to.have.property('tls', testOptions.ssl); - } else { - expect(options.tls).to.be.true; - } - if (testOptions && testOptions.replicaSet) { - expect(options).to.have.property('replicaSet', testOptions.replicaSet); - } - if (testOptions && testOptions.authSource) { - expect(options).to.have.property('credentials'); - expect(options.credentials.source).to.equal(testOptions.authSource); - } - if (testOptions && testOptions.loadBalanced) { - expect(options).to.have.property('loadBalanced', testOptions.loadBalanced); - } - if ( - test[1].parsed_options && - test[1].parsed_options.user && - test[1].parsed_options.password - ) { - expect(options.credentials.username).to.equal(test[1].parsed_options.user); - expect(options.credentials.password).to.equal( - test[1].parsed_options.password - ); - } - } - done(); - }); + options = parseOptions(test.uri); + hosts = await promisify(resolveSRVRecord)(options); } catch (error) { - if (test[1].error) { - expect(error).to.exist; - done(); - } else { - throw error; - } + thrownError = error; + } + + if (test.error) { + expect(thrownError).to.exist; + expect(hosts).to.not.exist; + return; // Nothing more to test... + } + + expect(thrownError).to.not.exist; + expect(options).to.exist; + + // Implicit SRV options must be set. + expect(options.directConnection).to.be.false; + const testOptions = test.options; + if (testOptions && 'tls' in testOptions) { + expect(options).to.have.property('tls', testOptions.tls); + } else if (testOptions && 'ssl' in testOptions) { + expect(options).to.have.property('tls', testOptions.ssl); + } else { + expect(options.tls).to.be.true; + } + if (testOptions && testOptions.replicaSet) { + expect(options).to.have.property('replicaSet', testOptions.replicaSet); } + if (testOptions && testOptions.authSource) { + expect(options).to.have.property('credentials'); + expect(options.credentials.source).to.equal(testOptions.authSource); + } + if (testOptions && testOptions.loadBalanced) { + expect(options).to.have.property('loadBalanced', testOptions.loadBalanced); + } + if (test.parsed_options && test.parsed_options.user && test.parsed_options.password) { + expect(options.credentials.username).to.equal(test.parsed_options.user); + expect(options.credentials.password).to.equal(test.parsed_options.password); + } + + // TODO(): srvMaxHost limiting happens in the callback given to resolveSRVRecord inside the driver + // How can we test this in unit test? + + // if (options.srvHost && comment.includes('srvMaxHosts')) { + // if (typeof test.numSeeds === 'number' && typeof test.numHosts === 'number') { + // // Driver should limit the hosts to a random selection, so we just assert counts + // expect(hosts).to.have.lengthOf(test.numSeeds); + // expect(options.srvMaxHosts).to.be.lessThan(hosts.length); + // const selectedHosts = shuffle(hosts, options.srvMaxHosts); + // expect(selectedHosts).to.have.lengthOf(test.numHosts); + // } else { + // // Tests that can assert exact host lists + // const hostsAsStrings = hosts.map(h => h.toString()); + // expect(hostsAsStrings).to.have.lengthOf(test.seeds.length); + // expect(hostsAsStrings, 'test.seeds').to.deep.equal(test.seeds); + + // if (test.options.srvMaxHosts !== 0) { + // expect(hostsAsStrings).to.have.lengthOf(test.options.srvMaxHosts); + // } else { + // expect(hostsAsStrings).to.have.lengthOf(test.hosts.length); + // } + // // expect(hostsAsStrings, 'test.hosts').to.deep.equal(test.hosts); + // } + // } } }); - }); + } }); - }); + } }); }); diff --git a/test/unit/utils.test.js b/test/unit/utils.test.js index bee1f80e13..e41f4d44d5 100644 --- a/test/unit/utils.test.js +++ b/test/unit/utils.test.js @@ -3,10 +3,12 @@ const { eachAsync, executeLegacyOperation, makeInterruptibleAsyncInterval, - BufferPool + BufferPool, + shuffle } = require('../../src/utils'); const { expect } = require('chai'); const sinon = require('sinon'); +const { MongoInvalidArgumentError } = require('../../src/error'); describe('utils', function () { context('eachAsync', function () { @@ -456,4 +458,31 @@ describe('utils', function () { }); }); }); + + describe('shuffler function', () => { + it('should support iterables', function () { + // Kind of an implicit test, we should not throw/crash here. + const input = new Set(['a', 'b', 'c']); + const output = shuffle(input); + expect(Array.isArray(output)).to.be.true; + }); + it('should give a random subset if limit is less than the input length', () => { + const input = ['a', 'b', 'c', 'd', 'e']; + const output = shuffle(input, 3); + expect(output).to.have.lengthOf(3); + }); + + it('should give a random shuffling of the input', () => { + const input = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']; + const output = shuffle(input); + // Of course it is possible a shuffle returns exactly the same as the input + // but it is so improbable it is worth the flakiness in my opinion + expect(output).to.not.deep.equal(input); + expect(output).to.have.lengthOf(input.length); + }); + + it('should throw if limit is larger than input size', () => { + expect(() => shuffle([], 100)).to.throw(MongoInvalidArgumentError); + }); + }); }); From 632124621aef9d0f631e0d8d797c57a0820aaac8 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 9 Nov 2021 12:12:32 -0500 Subject: [PATCH 02/30] fix: unit tests --- src/connection_string.ts | 5 ++++- src/utils.ts | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/connection_string.ts b/src/connection_string.ts index 01f09ecb65..b1f0a386ed 100644 --- a/src/connection_string.ts +++ b/src/connection_string.ts @@ -338,8 +338,11 @@ export function parseOptions( if (isSRV) { // SRV turns on TLS by default, but users can override and turn it off - if (!objectOptions.has('tls') && !urlOptions.has('ssl')) { + const noUserSpecifiedTLS = !objectOptions.has('tls') && !urlOptions.has('tls'); + const noUserSpecifiedSSL = !objectOptions.has('ssl') && !urlOptions.has('ssl'); + if (noUserSpecifiedTLS && noUserSpecifiedSSL) { objectOptions.set('tls', true); + objectOptions.set('ssl', true); } } diff --git a/src/utils.ts b/src/utils.ts index bef5251420..242df6a441 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -1437,7 +1437,7 @@ export function parsePackageVersion({ version }: { version: string }): { * @param limit - the number of ite */ export function shuffle(sequence: Iterable, limit = 0): Array { - const items = Array.isArray(sequence) ? sequence : Array.from(sequence); + const items = Array.from(sequence); // shallow copy in order to never shuffle the input if (limit > items.length) { throw new MongoInvalidArgumentError('Limit must be less than the number of items'); From 52b3377fef3040982aeb4e547690386ca2fedb61 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 9 Nov 2021 13:11:55 -0500 Subject: [PATCH 03/30] fix: integration tests --- src/sdam/srv_polling.ts | 4 ++-- src/sdam/topology_description.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/sdam/srv_polling.ts b/src/sdam/srv_polling.ts index 956d170b63..f2a09b2448 100644 --- a/src/sdam/srv_polling.ts +++ b/src/sdam/srv_polling.ts @@ -71,8 +71,8 @@ export class SrvPoller extends TypedEventEmitter { } this.srvHost = options.srvHost; - this.srvMaxHosts = options.srvMaxHosts; - this.srvServiceName = options.srvServiceName; + this.srvMaxHosts = options.srvMaxHosts ?? 0; + this.srvServiceName = options.srvServiceName ?? 'mongodb'; this.rescanSrvIntervalMS = options.rescanSrvIntervalMS ?? 60000; this.heartbeatFrequencyMS = options.heartbeatFrequencyMS ?? 10000; this.logger = new Logger('srvPoller', options); diff --git a/src/sdam/topology_description.ts b/src/sdam/topology_description.ts index 7036a2966e..00edfa5321 100644 --- a/src/sdam/topology_description.ts +++ b/src/sdam/topology_description.ts @@ -140,7 +140,7 @@ export class TopologyDescription { * Returns a new TopologyDescription based on the SrvPollingEvent * @internal */ - updateFromSrvPollingEvent(ev: SrvPollingEvent, srvMaxHosts: number): TopologyDescription { + updateFromSrvPollingEvent(ev: SrvPollingEvent, srvMaxHosts = 0): TopologyDescription { /** The SRV addresses defines the set of addresses we should be using */ const incomingHostnames = ev.hostnames(); const currentHostnames = new Set(this.servers.keys()); From 676f8f38a6800d058bb6964fa0932ba82ddba945 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 9 Nov 2021 16:55:04 -0500 Subject: [PATCH 04/30] wip --- src/connection_string.ts | 16 ++++++---------- src/operations/connect.ts | 15 +++++++++++---- src/sdam/srv_polling.ts | 27 ++++++++++++++++++++++++++- src/sdam/topology.ts | 5 ++++- 4 files changed, 47 insertions(+), 16 deletions(-) diff --git a/src/connection_string.ts b/src/connection_string.ts index b1f0a386ed..f036a276f9 100644 --- a/src/connection_string.ts +++ b/src/connection_string.ts @@ -34,13 +34,7 @@ import { PromiseProvider } from './promise_provider'; import { Encrypter } from './encrypter'; import { Compressor, CompressorName } from './cmap/wire_protocol/compression'; -const VALID_TXT_RECORDS = [ - 'authSource', - 'replicaSet', - 'loadBalanced', - 'srvMaxHosts', - 'srvServiceName' -]; +const VALID_TXT_RECORDS = ['authSource', 'replicaSet', 'loadBalanced', 'srvMaxHosts']; const LB_SINGLE_HOST_ERROR = 'loadBalanced option only supported with a single host in the URI'; const LB_REPLICA_SET_ERROR = 'loadBalanced option not supported with a replicaSet option'; @@ -69,7 +63,10 @@ function matchesParentDomain(srvAddress: string, parentDomain: string): boolean * @param uri - The connection string to parse * @param options - Optional user provided connection string options */ -export function resolveSRVRecord(options: MongoOptions, callback: Callback): void { +export function resolveSRVRecord( + options: MongoOptions, + callback: Callback<{ hosts: HostAddress[]; records: dns.SrvRecord[] }> +): void { if (typeof options.srvHost !== 'string') { return callback(new MongoAPIError('Option "srvHost" must not be empty')); } @@ -130,7 +127,6 @@ export function resolveSRVRecord(options: MongoOptions, callback: Callback { - if (err || !hosts) return callback(err); + return resolveSRVRecord(options, (err, res) => { + if (err || !res) return callback(err); + + const { hosts, records } = res; const selectedHosts = options.srvMaxHosts === 0 || options.srvMaxHosts >= hosts.length @@ -61,7 +64,11 @@ export function connect( options.hosts[index] = host; } - return createTopology(mongoClient, options, connectCallback); + return createTopology( + mongoClient, + { ...options, initialSrvResults: records }, + connectCallback + ); }); } @@ -70,7 +77,7 @@ export function connect( function createTopology( mongoClient: MongoClient, - options: MongoOptions, + options: MongoOptions & { initialSrvResults?: dns.SrvRecord[] }, callback: Callback ) { // Create the topology diff --git a/src/sdam/srv_polling.ts b/src/sdam/srv_polling.ts index f2a09b2448..402800215e 100644 --- a/src/sdam/srv_polling.ts +++ b/src/sdam/srv_polling.ts @@ -32,6 +32,21 @@ export class SrvPollingEvent { hostnames(): Set { return new Set(this.srvRecords.map(r => HostAddress.fromSrvRecord(r).toString())); } + + equals(other: SrvPollingEvent): boolean { + return ( + this.srvRecords.length !== 0 && + other.srvRecords.length !== 0 && + this.srvRecords.length === other.srvRecords.length && + this.srvRecords.every( + (record, index) => + record.name === other.srvRecords[index].name && + record.port === other.srvRecords[index].port && + record.weight === other.srvRecords[index].weight && + record.priority === other.srvRecords[index].priority + ) + ); + } } /** @internal */ @@ -41,6 +56,8 @@ export interface SrvPollerOptions extends LoggerOptions { srvMaxHosts: number; srvHost: string; heartbeatFrequencyMS: number; + + initialSrvResults?: dns.SrvRecord[]; } /** @internal */ @@ -58,6 +75,7 @@ export class SrvPoller extends TypedEventEmitter { generation: number; srvMaxHosts: number; srvServiceName: string; + lastSrvPollingEvent?: SrvPollingEvent; _timeout?: NodeJS.Timeout; /** @event */ @@ -75,6 +93,9 @@ export class SrvPoller extends TypedEventEmitter { this.srvServiceName = options.srvServiceName ?? 'mongodb'; this.rescanSrvIntervalMS = options.rescanSrvIntervalMS ?? 60000; this.heartbeatFrequencyMS = options.heartbeatFrequencyMS ?? 10000; + this.lastSrvPollingEvent = Array.isArray(options.initialSrvResults) + ? new SrvPollingEvent(options.initialSrvResults) + : undefined; this.logger = new Logger('srvPoller', options); this.haMode = false; @@ -116,7 +137,11 @@ export class SrvPoller extends TypedEventEmitter { success(srvRecords: dns.SrvRecord[]): void { this.haMode = false; this.schedule(); - this.emit(SrvPoller.SRV_RECORD_DISCOVERY, new SrvPollingEvent(srvRecords)); + const event = new SrvPollingEvent(srvRecords); + if (!this.lastSrvPollingEvent?.equals(event)) { + this.emit(SrvPoller.SRV_RECORD_DISCOVERY, event); + } + this.lastSrvPollingEvent = event; } failure(message: string, obj?: NodeJS.ErrnoException): void { diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index 448f9b7c5a..a8cad4c469 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -1,4 +1,5 @@ import Denque = require('denque'); +import type * as dns from 'dns'; import { ReadPreference, ReadPreferenceLike } from '../read_preference'; import { compareTopologyVersion, ServerDescription } from './server_description'; import { TopologyDescription } from './topology_description'; @@ -143,6 +144,7 @@ export interface TopologyOptions extends BSONSerializeOptions, ServerOptions { srvMaxHosts: number; srvServiceName: string; rescanSrvIntervalMS: number; + initialSrvResults?: dns.SrvRecord[]; hosts: HostAddress[]; retryWrites: boolean; retryReads: boolean; @@ -345,7 +347,8 @@ export class Topology extends TypedEventEmitter { srvHost: options.srvHost, srvMaxHosts: options.srvMaxHosts, srvServiceName: options.srvServiceName, - rescanSrvIntervalMS: options.rescanSrvIntervalMS + rescanSrvIntervalMS: options.rescanSrvIntervalMS, + initialSrvResults: options.initialSrvResults }); this.on(Topology.TOPOLOGY_DESCRIPTION_CHANGED, this.s.detectShardedTopology); From 787977335f19698b567264877332ea109e40ceb8 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 9 Nov 2021 17:33:53 -0500 Subject: [PATCH 05/30] test: fix up shuffle unit tests --- src/utils.ts | 11 +---------- test/unit/utils.test.js | 41 ++++++++++++++++++++++++++++++++++++++--- 2 files changed, 39 insertions(+), 13 deletions(-) diff --git a/src/utils.ts b/src/utils.ts index 242df6a441..ba21241347 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -1321,15 +1321,6 @@ export class HostAddress { Object.freeze(this); } - equals(other: HostAddress): boolean { - return ( - this.host === other.host && - this.port === other.port && - this.socketPath === other.socketPath && - this.isIPv6 === other.isIPv6 - ); - } - [Symbol.for('nodejs.util.inspect.custom')](): string { return this.inspect(); } @@ -1440,7 +1431,7 @@ export function shuffle(sequence: Iterable, limit = 0): Array { const items = Array.from(sequence); // shallow copy in order to never shuffle the input if (limit > items.length) { - throw new MongoInvalidArgumentError('Limit must be less than the number of items'); + throw new MongoRuntimeError('Limit must be less than the number of items'); } let remainingItemsToShuffle = items.length; diff --git a/test/unit/utils.test.js b/test/unit/utils.test.js index e41f4d44d5..bd0b2b25de 100644 --- a/test/unit/utils.test.js +++ b/test/unit/utils.test.js @@ -8,7 +8,7 @@ const { } = require('../../src/utils'); const { expect } = require('chai'); const sinon = require('sinon'); -const { MongoInvalidArgumentError } = require('../../src/error'); +const { MongoRuntimeError } = require('../../src/error'); describe('utils', function () { context('eachAsync', function () { @@ -466,13 +466,34 @@ describe('utils', function () { const output = shuffle(input); expect(Array.isArray(output)).to.be.true; }); + + it('should not mutate the original input', function () { + const input = Object.freeze(['a', 'b', 'c', 'd', 'e']); + const output = shuffle(input); // This will throw if shuffle tries to edit the input + expect(output).to.not.deep.equal(input); + expect(output).to.have.lengthOf(input.length); + }); + it('should give a random subset if limit is less than the input length', () => { const input = ['a', 'b', 'c', 'd', 'e']; const output = shuffle(input, 3); expect(output).to.have.lengthOf(3); }); - it('should give a random shuffling of the input', () => { + const input = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']; + for (let n = 1; n <= input.length; n++) { + it(`should give a random subset of length ${n}`, function () { + const output = shuffle(input, n); + if (n > 2) { + // This expectation fails more at n values below 3 + // sparing us the flake + expect(output).to.not.deep.equal(input.slice(0, n)); + } + expect(output).to.have.lengthOf(n); + }); + } + + it('should give a random shuffling of the entire input when no limit provided', () => { const input = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']; const output = shuffle(input); // Of course it is possible a shuffle returns exactly the same as the input @@ -481,8 +502,22 @@ describe('utils', function () { expect(output).to.have.lengthOf(input.length); }); + it('should handle empty array', function () { + expect(shuffle([])).to.deep.equal([]); + expect(shuffle([], 0)).to.deep.equal([]); + }); + + it('should handle limit set to 0', function () { + expect(shuffle(['a', 'b'])).to.have.lengthOf(2); + expect(shuffle(['a', 'b'], 0)).to.have.lengthOf(2); + }); + + it('should throw if limit is greater than zero and empty array', function () { + expect(() => shuffle([], 2)).to.throw(MongoRuntimeError); + }); + it('should throw if limit is larger than input size', () => { - expect(() => shuffle([], 100)).to.throw(MongoInvalidArgumentError); + expect(() => shuffle(['a', 'b'], 3)).to.throw(MongoRuntimeError); }); }); }); From 1d09e183aa0c3c621ab122ef7a963fc6daf4e1e9 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 10 Nov 2021 13:30:37 -0500 Subject: [PATCH 06/30] fix: shuffle tests round 2 undo srv event saving --- src/connection_string.ts | 7 ++-- src/operations/connect.ts | 14 +++----- src/sdam/srv_polling.ts | 12 +------ src/sdam/topology.ts | 4 +-- src/utils.ts | 2 +- test/unit/utils.test.js | 70 +++++++++++++++++++++++++++------------ 6 files changed, 57 insertions(+), 52 deletions(-) diff --git a/src/connection_string.ts b/src/connection_string.ts index f036a276f9..bd159207f2 100644 --- a/src/connection_string.ts +++ b/src/connection_string.ts @@ -63,10 +63,7 @@ function matchesParentDomain(srvAddress: string, parentDomain: string): boolean * @param uri - The connection string to parse * @param options - Optional user provided connection string options */ -export function resolveSRVRecord( - options: MongoOptions, - callback: Callback<{ hosts: HostAddress[]; records: dns.SrvRecord[] }> -): void { +export function resolveSRVRecord(options: MongoOptions, callback: Callback): void { if (typeof options.srvHost !== 'string') { return callback(new MongoAPIError('Option "srvHost" must not be empty')); } @@ -162,7 +159,7 @@ export function resolveSRVRecord( } } - callback(undefined, { hosts: hostAddresses, records: addresses }); + callback(undefined, hostAddresses); }); }); } diff --git a/src/operations/connect.ts b/src/operations/connect.ts index 5d74c25f70..3364db0a62 100644 --- a/src/operations/connect.ts +++ b/src/operations/connect.ts @@ -50,10 +50,8 @@ export function connect( }; if (typeof options.srvHost === 'string') { - return resolveSRVRecord(options, (err, res) => { - if (err || !res) return callback(err); - - const { hosts, records } = res; + return resolveSRVRecord(options, (err, hosts) => { + if (err || !hosts) return callback(err); const selectedHosts = options.srvMaxHosts === 0 || options.srvMaxHosts >= hosts.length @@ -64,11 +62,7 @@ export function connect( options.hosts[index] = host; } - return createTopology( - mongoClient, - { ...options, initialSrvResults: records }, - connectCallback - ); + return createTopology(mongoClient, options, connectCallback); }); } @@ -77,7 +71,7 @@ export function connect( function createTopology( mongoClient: MongoClient, - options: MongoOptions & { initialSrvResults?: dns.SrvRecord[] }, + options: MongoOptions, callback: Callback ) { // Create the topology diff --git a/src/sdam/srv_polling.ts b/src/sdam/srv_polling.ts index 402800215e..ed745c8a6e 100644 --- a/src/sdam/srv_polling.ts +++ b/src/sdam/srv_polling.ts @@ -56,8 +56,6 @@ export interface SrvPollerOptions extends LoggerOptions { srvMaxHosts: number; srvHost: string; heartbeatFrequencyMS: number; - - initialSrvResults?: dns.SrvRecord[]; } /** @internal */ @@ -75,7 +73,6 @@ export class SrvPoller extends TypedEventEmitter { generation: number; srvMaxHosts: number; srvServiceName: string; - lastSrvPollingEvent?: SrvPollingEvent; _timeout?: NodeJS.Timeout; /** @event */ @@ -93,9 +90,6 @@ export class SrvPoller extends TypedEventEmitter { this.srvServiceName = options.srvServiceName ?? 'mongodb'; this.rescanSrvIntervalMS = options.rescanSrvIntervalMS ?? 60000; this.heartbeatFrequencyMS = options.heartbeatFrequencyMS ?? 10000; - this.lastSrvPollingEvent = Array.isArray(options.initialSrvResults) - ? new SrvPollingEvent(options.initialSrvResults) - : undefined; this.logger = new Logger('srvPoller', options); this.haMode = false; @@ -137,11 +131,7 @@ export class SrvPoller extends TypedEventEmitter { success(srvRecords: dns.SrvRecord[]): void { this.haMode = false; this.schedule(); - const event = new SrvPollingEvent(srvRecords); - if (!this.lastSrvPollingEvent?.equals(event)) { - this.emit(SrvPoller.SRV_RECORD_DISCOVERY, event); - } - this.lastSrvPollingEvent = event; + this.emit(SrvPoller.SRV_RECORD_DISCOVERY, new SrvPollingEvent(srvRecords)); } failure(message: string, obj?: NodeJS.ErrnoException): void { diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index a8cad4c469..2054782321 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -144,7 +144,6 @@ export interface TopologyOptions extends BSONSerializeOptions, ServerOptions { srvMaxHosts: number; srvServiceName: string; rescanSrvIntervalMS: number; - initialSrvResults?: dns.SrvRecord[]; hosts: HostAddress[]; retryWrites: boolean; retryReads: boolean; @@ -347,8 +346,7 @@ export class Topology extends TypedEventEmitter { srvHost: options.srvHost, srvMaxHosts: options.srvMaxHosts, srvServiceName: options.srvServiceName, - rescanSrvIntervalMS: options.rescanSrvIntervalMS, - initialSrvResults: options.initialSrvResults + rescanSrvIntervalMS: options.rescanSrvIntervalMS }); this.on(Topology.TOPOLOGY_DESCRIPTION_CHANGED, this.s.detectShardedTopology); diff --git a/src/utils.ts b/src/utils.ts index ba21241347..4d2a5a7540 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -1425,7 +1425,7 @@ export function parsePackageVersion({ version }: { version: string }): { * * Reference: https://bost.ocks.org/mike/shuffle/ * @param sequence - items to be shuffled - * @param limit - the number of ite + * @param limit - if nonzero shuffle will slice the randomized array e.g, `.slice(0, limit)` */ export function shuffle(sequence: Iterable, limit = 0): Array { const items = Array.from(sequence); // shallow copy in order to never shuffle the input diff --git a/test/unit/utils.test.js b/test/unit/utils.test.js index bd0b2b25de..4763482aa5 100644 --- a/test/unit/utils.test.js +++ b/test/unit/utils.test.js @@ -10,8 +10,8 @@ const { expect } = require('chai'); const sinon = require('sinon'); const { MongoRuntimeError } = require('../../src/error'); -describe('utils', function () { - context('eachAsync', function () { +describe('driver utils', function () { + context('eachAsync function', function () { it('should callback with an error', function (done) { eachAsync( [{ error: false }, { error: true }], @@ -329,7 +329,7 @@ describe('utils', function () { }); }); - context('BufferPool', function () { + context('BufferPool class', function () { it('should report the correct length', function () { const buffer = new BufferPool(); buffer.append(Buffer.from([0, 1])); @@ -416,7 +416,7 @@ describe('utils', function () { }); }); - context('executeLegacyOperation', function () { + context('executeLegacyOperation function', function () { it('should call callback with errors on throw errors, and rethrow error', function () { const expectedError = new Error('THIS IS AN ERROR'); let callbackError, caughtError; @@ -459,7 +459,7 @@ describe('utils', function () { }); }); - describe('shuffler function', () => { + describe('shuffle function', () => { it('should support iterables', function () { // Kind of an implicit test, we should not throw/crash here. const input = new Set(['a', 'b', 'c']); @@ -474,24 +474,43 @@ describe('utils', function () { expect(output).to.have.lengthOf(input.length); }); - it('should give a random subset if limit is less than the input length', () => { - const input = ['a', 'b', 'c', 'd', 'e']; - const output = shuffle(input, 3); - expect(output).to.have.lengthOf(3); + it(`should give a random subset of length input.length - 1`, function () { + const input = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']; + const output = shuffle(input, input.length - 1); + expect(output).to.not.deep.equal(input); + expect(output).to.have.lengthOf(input.length - 1); }); - const input = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']; - for (let n = 1; n <= input.length; n++) { - it(`should give a random subset of length ${n}`, function () { - const output = shuffle(input, n); - if (n > 2) { - // This expectation fails more at n values below 3 - // sparing us the flake - expect(output).to.not.deep.equal(input.slice(0, n)); - } - expect(output).to.have.lengthOf(n); - }); - } + it(`should give a random subset of length input.length`, function () { + const input = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']; + const output = shuffle(input, input.length); + expect(output).to.not.deep.equal(input); + expect(output).to.have.lengthOf(input.length); + }); + + it(`should always return the same element when input is one item`, function () { + const input = ['a']; + for (let i = 0; i < 10; i++) { + const output = shuffle(input); + expect(output).to.deep.equal(input); + } + for (let i = 0; i < 10; i++) { + const output = shuffle(input, 1); // and with limit + expect(output).to.deep.equal(input); + } + }); + + it(`should return a different item on every call of limit 1`, function () { + const input = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']; + const outputs = new Set(); + for (let i = 0; i < 5; i++) { + const output = shuffle(input, 1); + expect(output).to.have.lengthOf(1); + outputs.add(output[0]); + } + // Of the 5 shuffles we got at least 2 unique random items, this is to avoid flakiness + expect(outputs.size).is.greaterThanOrEqual(2); + }); it('should give a random shuffling of the entire input when no limit provided', () => { const input = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']; @@ -501,8 +520,14 @@ describe('utils', function () { expect(output).to.not.deep.equal(input); expect(output).to.have.lengthOf(input.length); }); + it('should give a random shuffling of the entire input when limit is explicitly set to 0', () => { + const input = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']; + const output = shuffle(input, 0); + expect(output).to.not.deep.equal(input); + expect(output).to.have.lengthOf(input.length); + }); - it('should handle empty array', function () { + it('should handle empty array if limit is unspecified or 0', function () { expect(shuffle([])).to.deep.equal([]); expect(shuffle([], 0)).to.deep.equal([]); }); @@ -514,6 +539,7 @@ describe('utils', function () { it('should throw if limit is greater than zero and empty array', function () { expect(() => shuffle([], 2)).to.throw(MongoRuntimeError); + expect(() => shuffle([], 1)).to.throw(MongoRuntimeError); }); it('should throw if limit is larger than input size', () => { From f7c304d2a99c9c7e74e251b6c366fc684fc13552 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 10 Nov 2021 14:31:05 -0500 Subject: [PATCH 07/30] Apply suggestions from code review Co-authored-by: Daria Pardue --- test/unit/utils.test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/unit/utils.test.js b/test/unit/utils.test.js index 4763482aa5..38f303fee9 100644 --- a/test/unit/utils.test.js +++ b/test/unit/utils.test.js @@ -474,14 +474,14 @@ describe('driver utils', function () { expect(output).to.have.lengthOf(input.length); }); - it(`should give a random subset of length input.length - 1`, function () { + it(`should give a random subset of length equal to limit when limit is less than the input length`, function () { const input = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']; const output = shuffle(input, input.length - 1); expect(output).to.not.deep.equal(input); expect(output).to.have.lengthOf(input.length - 1); }); - it(`should give a random subset of length input.length`, function () { + it(`should give a random shuffling of the entire input when limit is equal to input length`, function () { const input = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']; const output = shuffle(input, input.length); expect(output).to.not.deep.equal(input); @@ -500,7 +500,7 @@ describe('driver utils', function () { } }); - it(`should return a different item on every call of limit 1`, function () { + it(`should return a random item on every call of limit 1`, function () { const input = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']; const outputs = new Set(); for (let i = 0; i < 5; i++) { From eca1ee4620b510f5914097b816b09df44d77e32b Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 10 Nov 2021 14:33:29 -0500 Subject: [PATCH 08/30] test: remove dupe test --- test/unit/utils.test.js | 5 ----- 1 file changed, 5 deletions(-) diff --git a/test/unit/utils.test.js b/test/unit/utils.test.js index 38f303fee9..76dd991de8 100644 --- a/test/unit/utils.test.js +++ b/test/unit/utils.test.js @@ -532,11 +532,6 @@ describe('driver utils', function () { expect(shuffle([], 0)).to.deep.equal([]); }); - it('should handle limit set to 0', function () { - expect(shuffle(['a', 'b'])).to.have.lengthOf(2); - expect(shuffle(['a', 'b'], 0)).to.have.lengthOf(2); - }); - it('should throw if limit is greater than zero and empty array', function () { expect(() => shuffle([], 2)).to.throw(MongoRuntimeError); expect(() => shuffle([], 1)).to.throw(MongoRuntimeError); From 471dc61d099f93f8461af31f4f42e8ea0fe282dc Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 10 Nov 2021 14:35:54 -0500 Subject: [PATCH 09/30] fix: remove unused equals method --- src/sdam/srv_polling.ts | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/src/sdam/srv_polling.ts b/src/sdam/srv_polling.ts index ed745c8a6e..f2a09b2448 100644 --- a/src/sdam/srv_polling.ts +++ b/src/sdam/srv_polling.ts @@ -32,21 +32,6 @@ export class SrvPollingEvent { hostnames(): Set { return new Set(this.srvRecords.map(r => HostAddress.fromSrvRecord(r).toString())); } - - equals(other: SrvPollingEvent): boolean { - return ( - this.srvRecords.length !== 0 && - other.srvRecords.length !== 0 && - this.srvRecords.length === other.srvRecords.length && - this.srvRecords.every( - (record, index) => - record.name === other.srvRecords[index].name && - record.port === other.srvRecords[index].port && - record.weight === other.srvRecords[index].weight && - record.priority === other.srvRecords[index].priority - ) - ); - } } /** @internal */ From 0b3527547134d198b143f7d618d8a0ee13f5b708 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 10 Nov 2021 14:39:31 -0500 Subject: [PATCH 10/30] docs: improve limit description --- src/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils.ts b/src/utils.ts index 4d2a5a7540..b271a890da 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -1425,7 +1425,7 @@ export function parsePackageVersion({ version }: { version: string }): { * * Reference: https://bost.ocks.org/mike/shuffle/ * @param sequence - items to be shuffled - * @param limit - if nonzero shuffle will slice the randomized array e.g, `.slice(0, limit)` + * @param limit - Defaults to `0`. If nonzero shuffle will slice the randomized array e.g, `.slice(0, limit)` otherwise will return the entire randomized array. */ export function shuffle(sequence: Iterable, limit = 0): Array { const items = Array.from(sequence); // shallow copy in order to never shuffle the input From 5daa3ce501b9ed7e6bf3d22f95976da25de79c88 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 10 Nov 2021 14:41:34 -0500 Subject: [PATCH 11/30] fix: lint --- src/operations/connect.ts | 1 - src/sdam/topology.ts | 1 - 2 files changed, 2 deletions(-) diff --git a/src/operations/connect.ts b/src/operations/connect.ts index 3364db0a62..ddd52c3e90 100644 --- a/src/operations/connect.ts +++ b/src/operations/connect.ts @@ -1,4 +1,3 @@ -import type * as dns from 'dns'; import { MongoRuntimeError, MongoInvalidArgumentError } from '../error'; import { Topology, TOPOLOGY_EVENTS } from '../sdam/topology'; import { resolveSRVRecord } from '../connection_string'; diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index 2054782321..448f9b7c5a 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -1,5 +1,4 @@ import Denque = require('denque'); -import type * as dns from 'dns'; import { ReadPreference, ReadPreferenceLike } from '../read_preference'; import { compareTopologyVersion, ServerDescription } from './server_description'; import { TopologyDescription } from './topology_description'; From 382ad4dd0fed0f80c1479a155d6f1b1c2242927b Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 10 Nov 2021 15:36:08 -0500 Subject: [PATCH 12/30] fix: remove rescan option and drop TXT record option logic --- src/connection_string.ts | 22 ++++---------- src/mongo_client.ts | 3 -- src/sdam/topology.ts | 4 +-- .../spec_prose.test.ts | 29 +++++++++++-------- 4 files changed, 24 insertions(+), 34 deletions(-) diff --git a/src/connection_string.ts b/src/connection_string.ts index bd159207f2..9377dbe638 100644 --- a/src/connection_string.ts +++ b/src/connection_string.ts @@ -34,7 +34,7 @@ import { PromiseProvider } from './promise_provider'; import { Encrypter } from './encrypter'; import { Compressor, CompressorName } from './cmap/wire_protocol/compression'; -const VALID_TXT_RECORDS = ['authSource', 'replicaSet', 'loadBalanced', 'srvMaxHosts']; +const VALID_TXT_RECORDS = ['authSource', 'replicaSet', 'loadBalanced']; const LB_SINGLE_HOST_ERROR = 'loadBalanced option only supported with a single host in the URI'; const LB_REPLICA_SET_ERROR = 'loadBalanced option not supported with a replicaSet option'; @@ -123,15 +123,6 @@ export function resolveSRVRecord(options: MongoOptions, callback: Callback diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index 448f9b7c5a..1acb946b04 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -142,7 +142,6 @@ export interface TopologyPrivate { export interface TopologyOptions extends BSONSerializeOptions, ServerOptions { srvMaxHosts: number; srvServiceName: string; - rescanSrvIntervalMS: number; hosts: HostAddress[]; retryWrites: boolean; retryReads: boolean; @@ -344,8 +343,7 @@ export class Topology extends TypedEventEmitter { heartbeatFrequencyMS: this.s.heartbeatFrequencyMS, srvHost: options.srvHost, srvMaxHosts: options.srvMaxHosts, - srvServiceName: options.srvServiceName, - rescanSrvIntervalMS: options.rescanSrvIntervalMS + srvServiceName: options.srvServiceName }); this.on(Topology.TOPOLOGY_DESCRIPTION_CHANGED, this.s.detectShardedTopology); diff --git a/test/integration/polling-srv-records-for-mongos-discovery/spec_prose.test.ts b/test/integration/polling-srv-records-for-mongos-discovery/spec_prose.test.ts index a32e6519e1..967c944120 100644 --- a/test/integration/polling-srv-records-for-mongos-discovery/spec_prose.test.ts +++ b/test/integration/polling-srv-records-for-mongos-discovery/spec_prose.test.ts @@ -41,9 +41,18 @@ describe(path.basename(__dirname), () => { let resolveSrvStub: sinon.SinonStub; let lookupStub: sinon.SinonStub; let client: MongoClient; + let clock: sinon.SinonFakeTimers; let dnsRecordsForTest1; //let dnsRecordsForTest3; + before(() => { + clock = sinon.useFakeTimers(); + }); + + after(() => { + clock.restore(); + }); + beforeEach(async () => { const mongoses = [ await mock.createServer(2000), @@ -148,8 +157,7 @@ describe(path.basename(__dirname), () => { client = new MongoClient(SRV_CONNECTION_STRING, { tls: false, srvMaxHosts: 0, - serverSelectionTimeoutMS: 5000, - rescanSrvIntervalMS: 60 + serverSelectionTimeoutMS: 5000 }); await client.connect(); @@ -159,7 +167,7 @@ describe(path.basename(__dirname), () => { dnsRecordsForTest1.map(({ name }) => name) ); - await sleep(2 * client.topology.s.srvPoller.intervalMS); + clock.tick(2 * client.topology.s.srvPoller.intervalMS); const polledServerAddresses = Array.from(client.topology.description.servers.keys()); polledServerAddresses.sort(); @@ -179,8 +187,7 @@ describe(path.basename(__dirname), () => { client = new MongoClient(SRV_CONNECTION_STRING, { tls: false, srvMaxHosts: 2, - serverSelectionTimeoutMS: 5000, - rescanSrvIntervalMS: 60 + serverSelectionTimeoutMS: 5000 }); await client.connect(); @@ -190,7 +197,7 @@ describe(path.basename(__dirname), () => { dnsRecordsForTest1.map(({ name }) => name) ); - await sleep(2 * client.topology.s.srvPoller.intervalMS); + clock.tick(2 * client.topology.s.srvPoller.intervalMS); const polledServerAddresses = Array.from(client.topology.description.servers.keys()); polledServerAddresses.sort(); @@ -211,8 +218,7 @@ describe(path.basename(__dirname), () => { client = new MongoClient(SRV_CONNECTION_STRING, { tls: false, srvMaxHosts: 2, - serverSelectionTimeoutMS: 5000, - rescanSrvIntervalMS: 60 + serverSelectionTimeoutMS: 5000 }); await client.connect(); @@ -222,7 +228,7 @@ describe(path.basename(__dirname), () => { dnsRecordsForTest1.map(({ name }) => name) ); - await sleep(2 * client.topology.s.srvPoller.intervalMS); + clock.tick(2 * client.topology.s.srvPoller.intervalMS); const polledServerAddresses = Array.from(client.topology.description.servers.keys()); polledServerAddresses.sort(); @@ -243,15 +249,14 @@ describe(path.basename(__dirname), () => { client = new MongoClient(SRV_CONNECTION_STRING, { tls: false, srvServiceName: 'myFancySrvServiceName', - serverSelectionTimeoutMS: 5000, - rescanSrvIntervalMS: 60 + serverSelectionTimeoutMS: 5000 }); makeStubs({ srvServiceName: 'myFancySrvServiceName' }); await client.connect(); - await sleep(2 * client.topology.s.srvPoller.intervalMS); + clock.tick(2 * client.topology.s.srvPoller.intervalMS); const resolveSrvCalls = resolveSrvStub.getCalls(); expect(resolveSrvCalls).to.have.lengthOf(2); From 852fca3819ca0dd8bfc1f0e0ae86a3074faed93c Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 10 Nov 2021 16:55:19 -0500 Subject: [PATCH 13/30] fix: permit new options only on srv connection strings make use of timers and await IO --- src/connection_string.ts | 22 +++++++++++-------- .../spec_prose.test.ts | 15 +++++++++---- test/tools/utils.js | 7 ++++++ test/unit/mongo_client.test.js | 15 +++++++++++++ 4 files changed, 46 insertions(+), 13 deletions(-) diff --git a/src/connection_string.ts b/src/connection_string.ts index 9377dbe638..84baaa2e8d 100644 --- a/src/connection_string.ts +++ b/src/connection_string.ts @@ -137,7 +137,7 @@ export function resolveSRVRecord(options: MongoOptions, callback: Callback { let dnsRecordsForTest1; //let dnsRecordsForTest3; - before(() => { + beforeEach(() => { clock = sinon.useFakeTimers(); }); - after(() => { - clock.restore(); + afterEach(() => { + if (clock) { + clock.restore(); + clock = undefined; + } }); beforeEach(async () => { @@ -168,6 +171,7 @@ describe(path.basename(__dirname), () => { ); clock.tick(2 * client.topology.s.srvPoller.intervalMS); + await processTick(); const polledServerAddresses = Array.from(client.topology.description.servers.keys()); polledServerAddresses.sort(); @@ -198,6 +202,7 @@ describe(path.basename(__dirname), () => { ); clock.tick(2 * client.topology.s.srvPoller.intervalMS); + await processTick(); const polledServerAddresses = Array.from(client.topology.description.servers.keys()); polledServerAddresses.sort(); @@ -229,6 +234,7 @@ describe(path.basename(__dirname), () => { ); clock.tick(2 * client.topology.s.srvPoller.intervalMS); + await processTick(); const polledServerAddresses = Array.from(client.topology.description.servers.keys()); polledServerAddresses.sort(); @@ -257,6 +263,7 @@ describe(path.basename(__dirname), () => { await client.connect(); clock.tick(2 * client.topology.s.srvPoller.intervalMS); + // No need to await process tick, since we're not checking DNS lookups const resolveSrvCalls = resolveSrvStub.getCalls(); expect(resolveSrvCalls).to.have.lengthOf(2); diff --git a/test/tools/utils.js b/test/tools/utils.js index f55e624a27..985e1c5d78 100644 --- a/test/tools/utils.js +++ b/test/tools/utils.js @@ -339,7 +339,14 @@ const runLater = (fn, ms) => { const sleep = ms => new Promise(resolve => setTimeout(resolve, ms)); +/** + * If you are using sinon fake timers, it can end up blocking queued IO from running + * awaiting a nextTick call will allow the event loop to process Networking/FS callbacks + */ +const processTick = () => new Promise(resolve => process.nextTick(resolve)); + module.exports = { + processTick, sleep, runLater, ejson, diff --git a/test/unit/mongo_client.test.js b/test/unit/mongo_client.test.js index 47a7785c95..ec81ecd0f1 100644 --- a/test/unit/mongo_client.test.js +++ b/test/unit/mongo_client.test.js @@ -719,4 +719,19 @@ describe('MongoOptions', function () { expect(parse).to.throw(/URI/); }); }); + + it('srvServiceName and srvMaxHosts cannot be used on a non-srv connection string', () => { + expect(() => { + new MongoClient('mongodb://localhost?srvMaxHosts=2'); + }).to.throw(MongoParseError); + expect(() => { + new MongoClient('mongodb://localhost?srvServiceName=abc'); + }).to.throw(MongoParseError); + expect(() => { + new MongoClient('mongodb://localhost', { srvMaxHosts: 2 }); + }).to.throw(MongoParseError); + expect(() => { + new MongoClient('mongodb://localhost', { srvServiceName: 'abc' }); + }).to.throw(MongoParseError); + }); }); From 28567f830e545eb3c87d4f1dc545d5e63b51a196 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 11 Nov 2021 11:52:27 -0500 Subject: [PATCH 14/30] fix: address comments, fix option parsing errors, test naming --- src/connection_string.ts | 73 ++++++++++++++---------- src/mongo_client.ts | 2 +- src/sdam/srv_polling.ts | 3 +- src/sdam/topology_description.ts | 11 +--- src/utils.ts | 2 +- test/functional/uri_options_spec.test.js | 4 +- test/unit/utils.test.js | 8 +-- 7 files changed, 56 insertions(+), 47 deletions(-) diff --git a/src/connection_string.ts b/src/connection_string.ts index 84baaa2e8d..5958abf8c1 100644 --- a/src/connection_string.ts +++ b/src/connection_string.ts @@ -136,11 +136,7 @@ export function resolveSRVRecord(options: MongoOptions, callback: Callback 0) { return callback(new MongoParseError('Cannot combine replicaSet option with srvMaxHosts')); } @@ -259,10 +255,6 @@ export function parseOptions( const mongoOptions = Object.create(null); mongoOptions.hosts = isSRV ? [] : hosts.map(HostAddress.fromString); - if (isSRV) { - // SRV Record is resolved upon connecting - mongoOptions.srvHost = hosts[0]; - } const urlOptions = new CaseInsensitiveMap(); @@ -294,12 +286,6 @@ export function parseOptions( throw new MongoAPIError('URI cannot contain options with no value'); } - if (key.toLowerCase() === 'serverapi') { - throw new MongoParseError( - 'URI cannot contain `serverApi`, it can only be passed to the client' - ); - } - if (key.toLowerCase() === 'authsource' && urlOptions.has('authSource')) { // If authSource is an explicit key in the urlOptions we need to remove the implicit dbName urlOptions.delete('authSource'); @@ -314,13 +300,32 @@ export function parseOptions( Object.entries(options).filter(([, v]) => v != null) ); + // Validate options that can only be provided by one of uri or object + + if (urlOptions.has('serverApi')) { + throw new MongoParseError( + 'URI cannot contain `serverApi`, it can only be passed to the client' + ); + } + if (objectOptions.has('loadBalanced')) { throw new MongoParseError('loadBalanced is only a valid option in the URI'); } - const allOptions = new CaseInsensitiveMap(); + // SRV connection string validations + + const nonZeroSrvMaxHosts = + objectOptions.get('srvMaxHosts') > 0 || urlOptions.get('srvMaxHosts') > 0; + + const srvSpecificOptionsSpecified = + objectOptions.has('srvMaxHosts') || + urlOptions.has('srvMaxHosts') || + objectOptions.has('srvServiceName') || + urlOptions.has('srvServiceName'); if (isSRV) { + // SRV Record is resolved upon connecting + mongoOptions.srvHost = hosts[0]; // SRV turns on TLS by default, but users can override and turn it off const noUserSpecifiedTLS = !objectOptions.has('tls') && !urlOptions.has('tls'); const noUserSpecifiedSSL = !objectOptions.has('ssl') && !urlOptions.has('ssl'); @@ -328,8 +333,27 @@ export function parseOptions( objectOptions.set('tls', true); objectOptions.set('ssl', true); } + } else { + if (srvSpecificOptionsSpecified) { + throw new MongoParseError( + 'Cannot use maxSrvHosts nor srvServiceName with a non-srv connection string' + ); + } + } + + if (nonZeroSrvMaxHosts) { + if (urlOptions.has('loadBalanced')) { + throw new MongoParseError('Cannot use srvMaxHosts option with loadBalanced'); + } + if (urlOptions.has('replicaSet') || objectOptions.has('replicaSet')) { + throw new MongoParseError('Cannot use srvMaxHosts option with replicaSet'); + } } + // All option collection + + const allOptions = new CaseInsensitiveMap(); + const allKeys = new Set([ ...urlOptions.keys(), ...objectOptions.keys(), @@ -375,17 +399,7 @@ export function parseOptions( ); } - if ( - isSRV === false && - (objectOptions.has('srvMaxHosts') || - urlOptions.has('srvMaxHosts') || - objectOptions.has('srvServiceName') || - urlOptions.has('srvServiceName')) - ) { - throw new MongoParseError( - 'Cannot use maxSrvHosts nor srvServiceName with a non-srv connection string' - ); - } + // Option parsing and setting for (const [key, descriptor] of Object.entries(OPTIONS)) { const values = allOptions.get(key); @@ -467,7 +481,7 @@ function validateLoadBalancedOptions( return new MongoParseError(LB_DIRECT_CONNECTION_ERROR); } - if (mongoOptions.srvHost && mongoOptions.srvMaxHosts) { + if (mongoOptions.srvHost && mongoOptions.srvMaxHosts > 0) { return new MongoParseError('Cannot limit srv hosts with loadBalanced enabled'); } } @@ -956,7 +970,8 @@ export const OPTIONS = { type: 'uint' }, srvMaxHosts: { - type: 'uint' + type: 'uint', + default: 0 }, srvServiceName: { type: 'string', diff --git a/src/mongo_client.ts b/src/mongo_client.ts index 9fe42230b4..7e4ccfef9b 100644 --- a/src/mongo_client.ts +++ b/src/mongo_client.ts @@ -132,7 +132,7 @@ export interface MongoClientOptions extends BSONSerializeOptions, SupportedNodeC compressors?: CompressorName[] | string; /** An integer that specifies the compression level if using zlib for network compression. */ zlibCompressionLevel?: 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | undefined; - /** The maximum number of hosts to connect to when using an srv connection string */ + /** The maximum number of hosts to connect to when using an srv connection string, a setting of `0` means unlimited hosts */ srvMaxHosts?: number; /** * Modifies the srv URI to look like: diff --git a/src/sdam/srv_polling.ts b/src/sdam/srv_polling.ts index f2a09b2448..aaa0f7fea2 100644 --- a/src/sdam/srv_polling.ts +++ b/src/sdam/srv_polling.ts @@ -36,7 +36,6 @@ export class SrvPollingEvent { /** @internal */ export interface SrvPollerOptions extends LoggerOptions { - rescanSrvIntervalMS?: number; srvServiceName: string; srvMaxHosts: number; srvHost: string; @@ -73,7 +72,7 @@ export class SrvPoller extends TypedEventEmitter { this.srvHost = options.srvHost; this.srvMaxHosts = options.srvMaxHosts ?? 0; this.srvServiceName = options.srvServiceName ?? 'mongodb'; - this.rescanSrvIntervalMS = options.rescanSrvIntervalMS ?? 60000; + this.rescanSrvIntervalMS = 60000; this.heartbeatFrequencyMS = options.heartbeatFrequencyMS ?? 10000; this.logger = new Logger('srvPoller', options); diff --git a/src/sdam/topology_description.ts b/src/sdam/topology_description.ts index 00edfa5321..4d48dfc451 100644 --- a/src/sdam/topology_description.ts +++ b/src/sdam/topology_description.ts @@ -145,8 +145,11 @@ export class TopologyDescription { const incomingHostnames = ev.hostnames(); const currentHostnames = new Set(this.servers.keys()); + const hostnamesToAdd = new Set(incomingHostnames); const hostnamesToRemove = new Set(); for (const hostname of currentHostnames) { + // filter hostnamesToAdd (made from incomingHostnames) down to what is *not* present in currentHostnames + hostnamesToAdd.delete(hostname); if (!incomingHostnames.has(hostname)) { // If the SRV Records no longer include this hostname // we have to stop using it @@ -154,14 +157,6 @@ export class TopologyDescription { } } - const hostnamesToAdd = new Set(); - for (const hostname of incomingHostnames) { - if (!currentHostnames.has(hostname)) { - // There are new hosts in the SRV Record! - hostnamesToAdd.add(hostname); - } - } - if (hostnamesToAdd.size === 0 && hostnamesToRemove.size === 0) { // No new hosts to add and none to remove return this; diff --git a/src/utils.ts b/src/utils.ts index b271a890da..fc09452833 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -1435,7 +1435,7 @@ export function shuffle(sequence: Iterable, limit = 0): Array { } let remainingItemsToShuffle = items.length; - while (remainingItemsToShuffle !== 0) { + while (remainingItemsToShuffle > 1) { // Pick a remaining element const randomIndex = Math.floor(Math.random() * remainingItemsToShuffle); remainingItemsToShuffle -= 1; diff --git a/test/functional/uri_options_spec.test.js b/test/functional/uri_options_spec.test.js index c8dc73f825..51365381b6 100644 --- a/test/functional/uri_options_spec.test.js +++ b/test/functional/uri_options_spec.test.js @@ -10,7 +10,7 @@ const { loadSpecTests } = require('../spec'); describe('URI Options (spec)', function () { const uriSpecs = loadSpecTests('uri-options'); - uriSpecs.forEach(suite => { + for (const suite of uriSpecs) { describe(suite.name, () => { for (const test of suite.tests) { const itFn = test.warning ? it.skip : it; @@ -35,7 +35,7 @@ describe('URI Options (spec)', function () { }); } }); - }); + } describe('srvMaxHost manual testing', function () { const srvMaxHostTests = uriSpecs.find(testFolder => testFolder.name === 'srv-options').tests; diff --git a/test/unit/utils.test.js b/test/unit/utils.test.js index 76dd991de8..9be0a65414 100644 --- a/test/unit/utils.test.js +++ b/test/unit/utils.test.js @@ -11,7 +11,7 @@ const sinon = require('sinon'); const { MongoRuntimeError } = require('../../src/error'); describe('driver utils', function () { - context('eachAsync function', function () { + context('eachAsync()', function () { it('should callback with an error', function (done) { eachAsync( [{ error: false }, { error: true }], @@ -329,7 +329,7 @@ describe('driver utils', function () { }); }); - context('BufferPool class', function () { + context('new BufferPool()', function () { it('should report the correct length', function () { const buffer = new BufferPool(); buffer.append(Buffer.from([0, 1])); @@ -416,7 +416,7 @@ describe('driver utils', function () { }); }); - context('executeLegacyOperation function', function () { + context('executeLegacyOperation()', function () { it('should call callback with errors on throw errors, and rethrow error', function () { const expectedError = new Error('THIS IS AN ERROR'); let callbackError, caughtError; @@ -459,7 +459,7 @@ describe('driver utils', function () { }); }); - describe('shuffle function', () => { + describe('shuffle()', () => { it('should support iterables', function () { // Kind of an implicit test, we should not throw/crash here. const input = new Set(['a', 'b', 'c']); From 00a39d1c48afc483ab6998e3995374d7ddbf52e2 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 11 Nov 2021 13:05:01 -0500 Subject: [PATCH 15/30] fix: LB connection string assertion --- src/connection_string.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/connection_string.ts b/src/connection_string.ts index 5958abf8c1..cab56588d4 100644 --- a/src/connection_string.ts +++ b/src/connection_string.ts @@ -342,7 +342,7 @@ export function parseOptions( } if (nonZeroSrvMaxHosts) { - if (urlOptions.has('loadBalanced')) { + if (urlOptions.get('loadBalanced') === true) { throw new MongoParseError('Cannot use srvMaxHosts option with loadBalanced'); } if (urlOptions.has('replicaSet') || objectOptions.has('replicaSet')) { From 12a72ab397429fcecb935ea6b39b9babdb867f39 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 11 Nov 2021 14:29:48 -0500 Subject: [PATCH 16/30] feat: super algorithm enhancements O(-1) speeds --- src/utils.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/utils.ts b/src/utils.ts index fc09452833..af2f94c551 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -1435,7 +1435,8 @@ export function shuffle(sequence: Iterable, limit = 0): Array { } let remainingItemsToShuffle = items.length; - while (remainingItemsToShuffle > 1) { + const lowerBound = limit === 0 ? 1 : items.length - limit; + while (remainingItemsToShuffle > lowerBound) { // Pick a remaining element const randomIndex = Math.floor(Math.random() * remainingItemsToShuffle); remainingItemsToShuffle -= 1; @@ -1446,5 +1447,5 @@ export function shuffle(sequence: Iterable, limit = 0): Array { items[randomIndex] = swapHold; } - return limit === 0 ? items : items.slice(0, limit); + return limit === 0 ? items : items.slice(lowerBound); } From 5fa6550d5238dd27e3c05780202a83165a97277c Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 12 Nov 2021 10:50:26 -0500 Subject: [PATCH 17/30] fix: shuffle lowerBound logic, test for srvServiceName length error --- src/connection_string.ts | 5 +---- src/utils.ts | 4 ++-- test/functional/uri_options_spec.test.js | 8 ++++--- test/unit/mongo_client.test.js | 28 +++++++++++++++++++++++- 4 files changed, 35 insertions(+), 10 deletions(-) diff --git a/src/connection_string.ts b/src/connection_string.ts index cab56588d4..72a4605298 100644 --- a/src/connection_string.ts +++ b/src/connection_string.ts @@ -286,10 +286,7 @@ export function parseOptions( throw new MongoAPIError('URI cannot contain options with no value'); } - if (key.toLowerCase() === 'authsource' && urlOptions.has('authSource')) { - // If authSource is an explicit key in the urlOptions we need to remove the implicit dbName - urlOptions.delete('authSource'); - } + if (!urlOptions.has(key)) { urlOptions.set(key, values); diff --git a/src/utils.ts b/src/utils.ts index af2f94c551..2b58c96205 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -1435,7 +1435,7 @@ export function shuffle(sequence: Iterable, limit = 0): Array { } let remainingItemsToShuffle = items.length; - const lowerBound = limit === 0 ? 1 : items.length - limit; + const lowerBound = limit % items.length === 0 ? 1 : items.length - limit; while (remainingItemsToShuffle > lowerBound) { // Pick a remaining element const randomIndex = Math.floor(Math.random() * remainingItemsToShuffle); @@ -1447,5 +1447,5 @@ export function shuffle(sequence: Iterable, limit = 0): Array { items[randomIndex] = swapHold; } - return limit === 0 ? items : items.slice(lowerBound); + return limit === 0 || limit === items.length ? items : items.slice(lowerBound); } diff --git a/test/functional/uri_options_spec.test.js b/test/functional/uri_options_spec.test.js index 51365381b6..dd34322331 100644 --- a/test/functional/uri_options_spec.test.js +++ b/test/functional/uri_options_spec.test.js @@ -5,11 +5,13 @@ const { promisify } = require('util'); require('chai').use(require('chai-subset')); const { parseOptions, resolveSRVRecord } = require('../../src/connection_string'); +const { MongoParseError } = require('../../src/error'); const { loadSpecTests } = require('../spec'); describe('URI Options (spec)', function () { const uriSpecs = loadSpecTests('uri-options'); + // FIXME(NODE-3738): URI tests do not correctly assert whether they error or not for (const suite of uriSpecs) { describe(suite.name, () => { for (const test of suite.tests) { @@ -27,10 +29,10 @@ describe('URI Options (spec)', function () { } } catch (err) { if (test.warning === false || test.valid === true) { - // This test case is supposed to pass + // This test is supposed to not throw an error, we skip here for now (NODE-3738) this.skip(); } - expect(err).to.be.an.instanceof(Error); + expect(err).to.be.an.instanceof(MongoParseError); } }); } @@ -54,7 +56,7 @@ describe('URI Options (spec)', function () { if (test.valid === false || test.warning === true) { // We implement warnings as errors - expect(thrownError).to.exist; + expect(thrownError).to.be.instanceOf(MongoParseError); expect(hosts).to.not.exist; return; // Nothing more to test... } diff --git a/test/unit/mongo_client.test.js b/test/unit/mongo_client.test.js index ec81ecd0f1..d269cd5667 100644 --- a/test/unit/mongo_client.test.js +++ b/test/unit/mongo_client.test.js @@ -2,8 +2,9 @@ const os = require('os'); const fs = require('fs'); const { expect } = require('chai'); +const { promisify } = require('util'); const { getSymbolFrom } = require('../tools/utils'); -const { parseOptions } = require('../../src/connection_string'); +const { parseOptions, resolveSRVRecord } = require('../../src/connection_string'); const { ReadConcern } = require('../../src/read_concern'); const { WriteConcern } = require('../../src/write_concern'); const { ReadPreference } = require('../../src/read_preference'); @@ -734,4 +735,29 @@ describe('MongoOptions', function () { new MongoClient('mongodb://localhost', { srvServiceName: 'abc' }); }).to.throw(MongoParseError); }); + + it('srvServiceName should error if it is too long', async () => { + let thrownError; + let options; + try { + options = parseOptions('mongodb+srv://localhost.a.com', { srvServiceName: 'a'.repeat(255) }); + await promisify(resolveSRVRecord)(options); + } catch (error) { + thrownError = error; + } + expect(thrownError).to.have.property('code', 'EBADNAME'); + }); + + it('srvServiceName should not error if it greater than 15 characters', async () => { + let thrownError; + let options; + try { + options = parseOptions('mongodb+srv://localhost.a.com', { srvServiceName: 'a'.repeat(16) }); + await promisify(resolveSRVRecord)(options); + } catch (error) { + thrownError = error; + } + // Nothing wrong with the name, just DNE + expect(thrownError).to.have.property('code', 'ENOTFOUND'); + }); }); From 537ec451ae2d073776158f9f0738941881042544 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 12 Nov 2021 13:12:29 -0500 Subject: [PATCH 18/30] or -> nor Co-authored-by: Daria Pardue --- src/connection_string.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/connection_string.ts b/src/connection_string.ts index 72a4605298..12e2983181 100644 --- a/src/connection_string.ts +++ b/src/connection_string.ts @@ -333,7 +333,7 @@ export function parseOptions( } else { if (srvSpecificOptionsSpecified) { throw new MongoParseError( - 'Cannot use maxSrvHosts nor srvServiceName with a non-srv connection string' + 'Cannot use srvMaxHosts or srvServiceName with a non-srv connection string' ); } } From 0db7bb8c5df387a1d13836fb49e2bca3db0f522e Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 15 Nov 2021 11:24:12 -0500 Subject: [PATCH 19/30] fix: address comments except for connection_string tests --- src/connection_string.ts | 96 +++++++++++++---------------- src/operations/connect.ts | 11 +--- src/sdam/topology.ts | 10 ++- test/unit/connection_string.test.js | 67 ++++++++++++-------- test/unit/mongo_client.test.js | 15 +++++ 5 files changed, 109 insertions(+), 90 deletions(-) diff --git a/src/connection_string.ts b/src/connection_string.ts index 12e2983181..10f450e79e 100644 --- a/src/connection_string.ts +++ b/src/connection_string.ts @@ -92,7 +92,7 @@ export function resolveSRVRecord(options: MongoOptions, callback: Callback v != null) ); @@ -309,44 +312,6 @@ export function parseOptions( throw new MongoParseError('loadBalanced is only a valid option in the URI'); } - // SRV connection string validations - - const nonZeroSrvMaxHosts = - objectOptions.get('srvMaxHosts') > 0 || urlOptions.get('srvMaxHosts') > 0; - - const srvSpecificOptionsSpecified = - objectOptions.has('srvMaxHosts') || - urlOptions.has('srvMaxHosts') || - objectOptions.has('srvServiceName') || - urlOptions.has('srvServiceName'); - - if (isSRV) { - // SRV Record is resolved upon connecting - mongoOptions.srvHost = hosts[0]; - // SRV turns on TLS by default, but users can override and turn it off - const noUserSpecifiedTLS = !objectOptions.has('tls') && !urlOptions.has('tls'); - const noUserSpecifiedSSL = !objectOptions.has('ssl') && !urlOptions.has('ssl'); - if (noUserSpecifiedTLS && noUserSpecifiedSSL) { - objectOptions.set('tls', true); - objectOptions.set('ssl', true); - } - } else { - if (srvSpecificOptionsSpecified) { - throw new MongoParseError( - 'Cannot use srvMaxHosts or srvServiceName with a non-srv connection string' - ); - } - } - - if (nonZeroSrvMaxHosts) { - if (urlOptions.get('loadBalanced') === true) { - throw new MongoParseError('Cannot use srvMaxHosts option with loadBalanced'); - } - if (urlOptions.has('replicaSet') || objectOptions.has('replicaSet')) { - throw new MongoParseError('Cannot use srvMaxHosts option with replicaSet'); - } - } - // All option collection const allOptions = new CaseInsensitiveMap(); @@ -439,25 +404,47 @@ export function parseOptions( if (options.promiseLibrary) PromiseProvider.set(options.promiseLibrary); - if (mongoOptions.directConnection && typeof mongoOptions.srvHost === 'string') { - throw new MongoAPIError('SRV URI does not support directConnection'); - } - - const lbError = validateLoadBalancedOptions(hosts, mongoOptions); + const lbError = validateLoadBalancedOptions(hosts, mongoOptions, isSRV); if (lbError) { throw lbError; } + if (mongoClient && mongoOptions.autoEncryption) { + Encrypter.checkForMongoCrypt(); + mongoOptions.encrypter = new Encrypter(mongoClient, uri, options); + mongoOptions.autoEncrypter = mongoOptions.encrypter.autoEncrypter; + } + + // Potential SRV Overrides and SRV connection string validations - // Potential SRV Overrides mongoOptions.userSpecifiedAuthSource = objectOptions.has('authSource') || urlOptions.has('authSource'); mongoOptions.userSpecifiedReplicaSet = objectOptions.has('replicaSet') || urlOptions.has('replicaSet'); - if (mongoClient && mongoOptions.autoEncryption) { - Encrypter.checkForMongoCrypt(); - mongoOptions.encrypter = new Encrypter(mongoClient, uri, options); - mongoOptions.autoEncrypter = mongoOptions.encrypter.autoEncrypter; + if (isSRV) { + // SRV Record is resolved upon connecting + mongoOptions.srvHost = hosts[0]; + + if (mongoOptions.directConnection) { + throw new MongoAPIError('SRV URI does not support directConnection'); + } + + if (mongoOptions.srvMaxHosts > 0 && typeof mongoOptions.replicaSet === 'string') { + throw new MongoParseError('Cannot use srvMaxHosts option with replicaSet'); + } + + // SRV turns on TLS by default, but users can override and turn it off + const noUserSpecifiedTLS = !objectOptions.has('tls') && !urlOptions.has('tls'); + const noUserSpecifiedSSL = !objectOptions.has('ssl') && !urlOptions.has('ssl'); + if (noUserSpecifiedTLS && noUserSpecifiedSSL) { + mongoOptions.tls = true; + } + } else { + if (mongoOptions.srvMaxHosts > 0 || mongoOptions.srvServiceName !== 'mongodb') { + throw new MongoParseError( + 'Cannot use srvMaxHosts or srvServiceName with a non-srv connection string' + ); + } } return mongoOptions; @@ -465,7 +452,8 @@ export function parseOptions( function validateLoadBalancedOptions( hosts: HostAddress[] | string[], - mongoOptions: MongoOptions + mongoOptions: MongoOptions, + isSrv: boolean ): MongoParseError | undefined { if (mongoOptions.loadBalanced) { if (hosts.length > 1) { @@ -478,7 +466,7 @@ function validateLoadBalancedOptions( return new MongoParseError(LB_DIRECT_CONNECTION_ERROR); } - if (mongoOptions.srvHost && mongoOptions.srvMaxHosts > 0) { + if (isSrv && mongoOptions.srvMaxHosts > 0) { return new MongoParseError('Cannot limit srv hosts with loadBalanced enabled'); } } diff --git a/src/operations/connect.ts b/src/operations/connect.ts index ddd52c3e90..9abb9be864 100644 --- a/src/operations/connect.ts +++ b/src/operations/connect.ts @@ -1,7 +1,7 @@ import { MongoRuntimeError, MongoInvalidArgumentError } from '../error'; import { Topology, TOPOLOGY_EVENTS } from '../sdam/topology'; import { resolveSRVRecord } from '../connection_string'; -import { Callback, shuffle } from '../utils'; +import type { Callback } from '../utils'; import type { MongoClient, MongoOptions } from '../mongo_client'; import { CMAP_EVENTS } from '../cmap/connection_pool'; import { APM_EVENTS } from '../cmap/connection'; @@ -52,15 +52,6 @@ export function connect( return resolveSRVRecord(options, (err, hosts) => { if (err || !hosts) return callback(err); - const selectedHosts = - options.srvMaxHosts === 0 || options.srvMaxHosts >= hosts.length - ? hosts - : shuffle(hosts, options.srvMaxHosts); - - for (const [index, host] of selectedHosts.entries()) { - options.hosts[index] = host; - } - return createTopology(mongoClient, options, connectCallback); }); } diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index 1acb946b04..bf66bd87cb 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -27,7 +27,8 @@ import { HostAddress, ns, emitWarning, - EventEmitterWithState + EventEmitterWithState, + shuffle } from '../utils'; import { TopologyType, @@ -292,8 +293,13 @@ export class Topology extends TypedEventEmitter { const topologyType = topologyTypeFromOptions(options); const topologyId = globalTopologyCounter++; + const selectedHosts = + options.srvMaxHosts === 0 || options.srvMaxHosts >= seedlist.length + ? seedlist + : shuffle(seedlist, options.srvMaxHosts); + const serverDescriptions = new Map(); - for (const hostAddress of seedlist) { + for (const hostAddress of selectedHosts) { serverDescriptions.set(hostAddress.toString(), new ServerDescription(hostAddress)); } diff --git a/test/unit/connection_string.test.js b/test/unit/connection_string.test.js index 5c766a9fbc..7dd2ec7dde 100644 --- a/test/unit/connection_string.test.js +++ b/test/unit/connection_string.test.js @@ -2,12 +2,15 @@ const fs = require('fs'); const path = require('path'); +const dns = require('dns'); const { promisify } = require('util'); const { MongoParseError, MongoDriverError, MongoInvalidArgumentError } = require('../../src/error'); const { loadSpecTests } = require('../spec'); const { parseOptions, resolveSRVRecord } = require('../../src/connection_string'); const { AuthMechanism } = require('../../src/cmap/auth/defaultAuthProviders'); const { expect } = require('chai'); +const { Topology } = require('../../src/sdam/topology'); +const { HostAddress } = require('../../src/utils'); // NOTE: These are cases we could never check for unless we write our own // url parser. The node parser simply won't let these through, so we @@ -287,30 +290,46 @@ describe('Connection String', function () { expect(options.credentials.password).to.equal(test.parsed_options.password); } - // TODO(): srvMaxHost limiting happens in the callback given to resolveSRVRecord inside the driver - // How can we test this in unit test? - - // if (options.srvHost && comment.includes('srvMaxHosts')) { - // if (typeof test.numSeeds === 'number' && typeof test.numHosts === 'number') { - // // Driver should limit the hosts to a random selection, so we just assert counts - // expect(hosts).to.have.lengthOf(test.numSeeds); - // expect(options.srvMaxHosts).to.be.lessThan(hosts.length); - // const selectedHosts = shuffle(hosts, options.srvMaxHosts); - // expect(selectedHosts).to.have.lengthOf(test.numHosts); - // } else { - // // Tests that can assert exact host lists - // const hostsAsStrings = hosts.map(h => h.toString()); - // expect(hostsAsStrings).to.have.lengthOf(test.seeds.length); - // expect(hostsAsStrings, 'test.seeds').to.deep.equal(test.seeds); - - // if (test.options.srvMaxHosts !== 0) { - // expect(hostsAsStrings).to.have.lengthOf(test.options.srvMaxHosts); - // } else { - // expect(hostsAsStrings).to.have.lengthOf(test.hosts.length); - // } - // // expect(hostsAsStrings, 'test.hosts').to.deep.equal(test.hosts); - // } - // } + // srvMaxHost limiting happens in the topology constructor + if (options.srvHost && comment.includes('srvMaxHosts')) { + return this.skip(); // REMOVE ME!!!! + // const topology = new Topology(hosts, options); + // const initialSeedlist = hosts.map(h => h.toString()); + // const selectedHosts = Array.from(topology.s.description.servers.keys()); + + // if (typeof test.numSeeds === 'number') { + // // numSeeds: the expected number of initial seeds discovered from the SRV record. + // expect(initialSeedlist).to.have.lengthOf(test.numSeeds); + // } + // if (typeof test.numHosts === 'number') { + // // numHosts: the expected number of hosts discovered once SDAM completes a scan. + // // (In our case, its the Topology constructor, but not actual SDAM) + // expect(selectedHosts).to.have.lengthOf(test.numHosts); + // } + + // if (Array.isArray(test.seeds)) { + // // verify that the set of hosts in the client's initial seedlist + // // matches the list in seeds + // expect(initialSeedlist).to.deep.equal(test.seeds); + // } + // if (Array.isArray(test.hosts)) { + // // verify that the set of ServerDescriptions in the client's TopologyDescription + // // eventually matches the list in hosts + // const actualAddresses = await Promise.all( + // selectedHosts + // .map(async hn => await promisify(dns.lookup)(HostAddress.fromString(hn).host)) + // .map(async (addr, i) => { + // let address = (await addr).address; + // address = address === '127.0.0.1' ? 'localhost' : address; + // return HostAddress.fromString( + // `${address}:${HostAddress.fromString(selectedHosts[i]).port}` + // ).toString(); + // }) + // ); + + // expect(actualAddresses).to.deep.equal(test.hosts); + // } + } } }); } diff --git a/test/unit/mongo_client.test.js b/test/unit/mongo_client.test.js index d269cd5667..52b6d02225 100644 --- a/test/unit/mongo_client.test.js +++ b/test/unit/mongo_client.test.js @@ -721,6 +721,21 @@ describe('MongoOptions', function () { }); }); + it('srvMaxHosts cannot be combined with LB or ReplicaSet', () => { + expect(() => { + new MongoClient('mongodb+srv://localhost?srvMaxHosts=2&replicaSet=repl'); + }).to.throw(MongoParseError, 'Cannot use srvMaxHosts option with replicaSet'); + expect(() => { + new MongoClient('mongodb+srv://localhost?srvMaxHosts=2&loadBalanced=true'); + }).to.throw(MongoParseError, 'Cannot limit srv hosts with loadBalanced enabled'); + expect(() => { + new MongoClient('mongodb+srv://localhost', { srvMaxHosts: 2, replicaSet: 'blah' }); + }).to.throw(MongoParseError, 'Cannot use srvMaxHosts option with replicaSet'); + expect(() => { + new MongoClient('mongodb+srv://localhost?loadBalanced=true', { srvMaxHosts: 2 }); + }).to.throw(MongoParseError, 'Cannot limit srv hosts with loadBalanced enabled'); + }); + it('srvServiceName and srvMaxHosts cannot be used on a non-srv connection string', () => { expect(() => { new MongoClient('mongodb://localhost?srvMaxHosts=2'); From 42a09422428382639647952e84f0b9c0dcf4ddd3 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 15 Nov 2021 14:20:57 -0500 Subject: [PATCH 20/30] fix: whoops broke host gathering, fixed now --- src/operations/connect.ts | 4 ++++ test/unit/connection_string.test.js | 6 +++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/operations/connect.ts b/src/operations/connect.ts index 9abb9be864..586dbdfd6a 100644 --- a/src/operations/connect.ts +++ b/src/operations/connect.ts @@ -52,6 +52,10 @@ export function connect( return resolveSRVRecord(options, (err, hosts) => { if (err || !hosts) return callback(err); + for (let index = 0; index < hosts.length; index++) { + options.hosts[index] = hosts[index]; + } + return createTopology(mongoClient, options, connectCallback); }); } diff --git a/test/unit/connection_string.test.js b/test/unit/connection_string.test.js index 7dd2ec7dde..ce3aa6daf3 100644 --- a/test/unit/connection_string.test.js +++ b/test/unit/connection_string.test.js @@ -2,15 +2,15 @@ const fs = require('fs'); const path = require('path'); -const dns = require('dns'); const { promisify } = require('util'); const { MongoParseError, MongoDriverError, MongoInvalidArgumentError } = require('../../src/error'); const { loadSpecTests } = require('../spec'); const { parseOptions, resolveSRVRecord } = require('../../src/connection_string'); const { AuthMechanism } = require('../../src/cmap/auth/defaultAuthProviders'); const { expect } = require('chai'); -const { Topology } = require('../../src/sdam/topology'); -const { HostAddress } = require('../../src/utils'); +// const dns = require('dns'); +// const { Topology } = require('../../src/sdam/topology'); +// const { HostAddress } = require('../../src/utils'); // NOTE: These are cases we could never check for unless we write our own // url parser. The node parser simply won't let these through, so we From 0db9494d7d078283b10e65859195b9ae111afaf0 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 15 Nov 2021 14:53:17 -0500 Subject: [PATCH 21/30] suggestions! Co-authored-by: Daria Pardue --- src/utils.ts | 2 +- test/unit/mongo_client.test.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/utils.ts b/src/utils.ts index 2b58c96205..cb5de9b020 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -1447,5 +1447,5 @@ export function shuffle(sequence: Iterable, limit = 0): Array { items[randomIndex] = swapHold; } - return limit === 0 || limit === items.length ? items : items.slice(lowerBound); + return limit % items.length === 0 ? items : items.slice(lowerBound); } diff --git a/test/unit/mongo_client.test.js b/test/unit/mongo_client.test.js index 52b6d02225..3762258b5f 100644 --- a/test/unit/mongo_client.test.js +++ b/test/unit/mongo_client.test.js @@ -721,7 +721,7 @@ describe('MongoOptions', function () { }); }); - it('srvMaxHosts cannot be combined with LB or ReplicaSet', () => { + it('srvMaxHosts > 0 cannot be combined with LB or ReplicaSet', () => { expect(() => { new MongoClient('mongodb+srv://localhost?srvMaxHosts=2&replicaSet=repl'); }).to.throw(MongoParseError, 'Cannot use srvMaxHosts option with replicaSet'); @@ -763,7 +763,7 @@ describe('MongoOptions', function () { expect(thrownError).to.have.property('code', 'EBADNAME'); }); - it('srvServiceName should not error if it greater than 15 characters', async () => { + it('srvServiceName should not error if it is greater than 15 characters as long as the DNS query limit is not surpassed', async () => { let thrownError; let options; try { From 60e3c75b460fd2c7d9b9589e2f06f4cf14999c13 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 15 Nov 2021 17:48:47 -0500 Subject: [PATCH 22/30] move tests into correct places, update initial seed list testing WIP --- src/connection_string.ts | 8 +- src/operations/connect.ts | 5 +- ...nitial_dns_seedlist_discovery.spec.test.ts | 152 ++++++++++ .../spec_prose.test.ts | 274 ------------------ test/unit/connection_string.test.js | 118 +------- test/unit/mongo_client.test.js | 9 + ...records_for_mongos_discovery.prose.test.ts | 258 +++++++++++++++++ 7 files changed, 429 insertions(+), 395 deletions(-) create mode 100644 test/integration/initial-dns-seedlist-discovery/initial_dns_seedlist_discovery.spec.test.ts delete mode 100644 test/integration/polling-srv-records-for-mongos-discovery/spec_prose.test.ts create mode 100644 test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts diff --git a/src/connection_string.ts b/src/connection_string.ts index 10f450e79e..ed897000dc 100644 --- a/src/connection_string.ts +++ b/src/connection_string.ts @@ -440,7 +440,13 @@ export function parseOptions( mongoOptions.tls = true; } } else { - if (mongoOptions.srvMaxHosts > 0 || mongoOptions.srvServiceName !== 'mongodb') { + const userSpecifiedSrvOptions = + urlOptions.has('srvMaxHosts') || + objectOptions.has('srvMaxHosts') || + urlOptions.has('srvServiceName') || + objectOptions.has('srvServiceName'); + + if (userSpecifiedSrvOptions) { throw new MongoParseError( 'Cannot use srvMaxHosts or srvServiceName with a non-srv connection string' ); diff --git a/src/operations/connect.ts b/src/operations/connect.ts index 586dbdfd6a..7a1241130b 100644 --- a/src/operations/connect.ts +++ b/src/operations/connect.ts @@ -51,9 +51,8 @@ export function connect( if (typeof options.srvHost === 'string') { return resolveSRVRecord(options, (err, hosts) => { if (err || !hosts) return callback(err); - - for (let index = 0; index < hosts.length; index++) { - options.hosts[index] = hosts[index]; + for (const [index, host] of hosts.entries()) { + options.hosts[index] = host; } return createTopology(mongoClient, options, connectCallback); diff --git a/test/integration/initial-dns-seedlist-discovery/initial_dns_seedlist_discovery.spec.test.ts b/test/integration/initial-dns-seedlist-discovery/initial_dns_seedlist_discovery.spec.test.ts new file mode 100644 index 0000000000..2496fba48c --- /dev/null +++ b/test/integration/initial-dns-seedlist-discovery/initial_dns_seedlist_discovery.spec.test.ts @@ -0,0 +1,152 @@ +import * as fs from 'fs'; +import * as path from 'path'; +import * as dns from 'dns'; +import { expect } from 'chai'; +import { promisify } from 'util'; +import { HostAddress } from '../../../src/utils'; +import { MongoClient } from '../../../src'; + +function makeTest(test, topology) { + let client; + + afterEach(async () => { + if (client) { + await client.close(); + client = undefined; + } + }); + + it(test.comment, async function () { + return this.skip(); // FIXME + if (topology === 'replica-set' && this.configuration.topologyType !== 'ReplicaSetWithPrimary') { + return this.skip(); + } + + if (topology === 'sharded' && this.configuration.topologyType !== 'sharded') { + return this.skip(); + } + + if (topology === 'load-balanced' && this.configuration.topologyType !== 'load-balanced') { + return this.skip(); + } + + let thrownError; + try { + client = new MongoClient(test.uri, { serverSelectionTimeoutMS: 2000, tls: false }); + await client.connect(); + } catch (error) { + thrownError = error; + } + + if (test.error) { + expect(thrownError).to.exist; + return; // Nothing more to test... + } + + const options = client.options; + const hosts = Array.from(client.topology.s.description.servers.keys()); + + expect(thrownError).to.not.exist; + expect(options).to.exist; + + // Implicit SRV options must be set. + expect(options.directConnection).to.be.false; + const testOptions = test.options; + if (testOptions && 'tls' in testOptions) { + expect(options).to.have.property('tls', testOptions.tls); + } else if (testOptions && 'ssl' in testOptions) { + expect(options).to.have.property('tls', testOptions.ssl); + } else { + expect(options.tls).to.be.true; + } + if (testOptions && testOptions.replicaSet) { + expect(options).to.have.property('replicaSet', testOptions.replicaSet); + } + if (testOptions && testOptions.authSource) { + expect(options).to.have.property('credentials'); + expect(options.credentials.source).to.equal(testOptions.authSource); + } + if (testOptions && testOptions.loadBalanced) { + expect(options).to.have.property('loadBalanced', testOptions.loadBalanced); + } + if (test.parsed_options && test.parsed_options.user && test.parsed_options.password) { + expect(options.credentials.username).to.equal(test.parsed_options.user); + expect(options.credentials.password).to.equal(test.parsed_options.password); + } + + // srvMaxHost limiting happens in the topology constructor + if (options.srvHost && test.comment.includes('srvMaxHosts')) { + const initialSeedlist = hosts.map(h => h.toString()); + const selectedHosts = Array.from(topology.s.description.servers.keys()); + + if (typeof test.numSeeds === 'number') { + // numSeeds: the expected number of initial seeds discovered from the SRV record. + expect(initialSeedlist).to.have.lengthOf(test.numSeeds); + } + if (typeof test.numHosts === 'number') { + // numHosts: the expected number of hosts discovered once SDAM completes a scan. + // (In our case, its the Topology constructor, but not actual SDAM) + expect(selectedHosts).to.have.lengthOf(test.numHosts); + } + + if (Array.isArray(test.seeds)) { + // verify that the set of hosts in the client's initial seedlist + // matches the list in seeds + expect(initialSeedlist).to.deep.equal(test.seeds); + } + if (Array.isArray(test.hosts)) { + // verify that the set of ServerDescriptions in the client's TopologyDescription + // eventually matches the list in hosts + const actualAddresses = await Promise.all( + selectedHosts + .map(async hn => await promisify(dns.lookup)(HostAddress.fromString(hn).host)) + .map(async (addr, i) => { + let address = (await addr).address; + address = address === '127.0.0.1' ? 'localhost' : address; + return HostAddress.fromString( + `${address}:${HostAddress.fromString(selectedHosts[i]).port}` + ).toString(); + }) + ); + + expect(actualAddresses).to.deep.equal(test.hosts); + } + } + }); +} + +function readTestFilesFor(topology) { + const specPath = path.join(__dirname, '../../spec', 'initial-dns-seedlist-discovery', topology); + const testFiles = fs + .readdirSync(specPath) + .filter(x => x.indexOf('.json') !== -1) + .map(x => [x, fs.readFileSync(path.join(specPath, x), 'utf8')]) + .map(x => { + const test = JSON.parse(x[1]); + const fileName = path.basename(x[0], '.json'); + if (!test.comment) { + test.comment = fileName; + } + return [fileName, test]; + }); + return testFiles; +} + +/** + * 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 (corresponding to the script in drivers-evergreen-tools). + * 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: + */ +describe('Initial DNS Seedlist Discovery', () => { + for (const topology of ['replica-set', 'load-balanced', 'sharded']) { + describe(topology, function () { + const testFiles = readTestFilesFor(topology); + for (const [, test] of testFiles) { + makeTest(test, topology); + } + }); + } +}); diff --git a/test/integration/polling-srv-records-for-mongos-discovery/spec_prose.test.ts b/test/integration/polling-srv-records-for-mongos-discovery/spec_prose.test.ts deleted file mode 100644 index 7c239b1975..0000000000 --- a/test/integration/polling-srv-records-for-mongos-discovery/spec_prose.test.ts +++ /dev/null @@ -1,274 +0,0 @@ -import * as path from 'path'; -import * as dns from 'dns'; -import * as sinon from 'sinon'; -import { expect } from 'chai'; -import { MongoClient } from '../../../src'; -import { processTick } from '../../tools/utils'; -import { it } from 'mocha'; -import * as mock from '../../tools/mock'; - -/* - The SRV Prose Tests make use of the following REAL DNS records. - We use sinon to replace the results from resolveSrv to test hostname removals and insertions. - - The prose tests assume you have a 4 node sharded cluster running on ports: - [27017, 27018, 27019, 27020] - - Record TTL Class Address - localhost.test.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.test3.test.build.10gen.cc. 86400 IN SRV 27017 localhost.test.build.10gen.cc. -*/ - -// const makeSrvRecordName = (host, serviceName = 'mongodb') => `_${serviceName}._tcp.${host}`; -// const dnsResolveSrvAsync = promisify(dns.resolveSrv); -const srvRecord = (name, port) => ({ name, port, weight: 0, priority: 0 }); -interface ShardedClusterMocks { - mongoses: mock.MockServer[]; - readonly srvRecords: dns.SrvRecord[]; -} - -describe(path.basename(__dirname), () => { - describe('prose tests', () => { - // TODO(): Make use of the shared driver's DNS records - // const SRV_CONNECTION_STRING_T1 = 'mongodb+srv://test1.test.build.10gen.cc'; - // const SRV_CONNECTION_STRING_t3 = 'mongodb+srv://test3.test.build.10gen.cc'; - const SRV_CONNECTION_STRING = 'mongodb+srv://my.fancy.prose.tests'; - let shardedCluster: ShardedClusterMocks; - let resolveSrvStub: sinon.SinonStub; - let lookupStub: sinon.SinonStub; - let client: MongoClient; - let clock: sinon.SinonFakeTimers; - let dnsRecordsForTest1; - //let dnsRecordsForTest3; - - beforeEach(() => { - clock = sinon.useFakeTimers(); - }); - - afterEach(() => { - if (clock) { - clock.restore(); - clock = undefined; - } - }); - - beforeEach(async () => { - const mongoses = [ - await mock.createServer(2000), - await mock.createServer(2001), - await mock.createServer(2002), - await mock.createServer(2003) - ]; - - const srvRecords = mongoses.map(s => srvRecord('localhost.my.fancy.prose.tests', s.port)); - - shardedCluster = { mongoses, srvRecords }; - - for (const mongos of shardedCluster.mongoses) { - mongos.setMessageHandler(request => { - const document = request.document; - - if (document.ismaster || document.hello) { - request.reply({ ...mock.HELLO, msg: 'isdbgrid' }); - } - }); - } - }); - - afterEach(async () => { - await mock.cleanup(); - }); - - before(async () => { - // const srvTest1 = makeSrvRecordName('test1.test.build.10gen.cc'); - // const srvTest3 = makeSrvRecordName('test3.test.build.10gen.cc'); - dnsRecordsForTest1 = [ - { name: 'localhost.my.fancy.prose.tests', port: 2000 }, - { name: 'localhost.my.fancy.prose.tests', port: 2001 } - ]; // await dnsResolveSrvAsync(srvTest1); - // dnsRecordsForTest3 = await dnsResolveSrvAsync(srvTest3); - }); - - afterEach(async () => { - if (resolveSrvStub) { - resolveSrvStub.restore(); - resolveSrvStub = undefined; - } - if (lookupStub) { - lookupStub.restore(); - lookupStub = undefined; - } - if (client) { - await client.close(); - } - }); - - function makeStubs({ - replacementRecords = undefined, - mockLimit = 0, - srvServiceName = 'mongodb' - }) { - let initialDNSLookup = true; - const mockRecords = shardedCluster.srvRecords; - replacementRecords ??= mockRecords; - // for some reason the stub needs to be set up here. - // first call is for the driver initial connection - // second call will check the poller - resolveSrvStub = sinon.stub(dns, 'resolveSrv').callsFake((address, callback) => { - expect(address).to.equal(`_${srvServiceName}._tcp.my.fancy.prose.tests`); - if (initialDNSLookup) { - initialDNSLookup = false; - const recordsToUse = mockRecords.slice( - 0, - mockLimit === 0 ? mockRecords.length : mockLimit - ); - - return process.nextTick(callback, null, recordsToUse); - } - process.nextTick(callback, null, replacementRecords); - }); - - lookupStub = sinon.stub(dns, 'lookup').callsFake((...args) => { - const hostname = args[0]; - const options = typeof args[1] === 'object' ? args[1] : {}; - const callback = args[args.length - 1] as (err: null, address: string, family: 4) => void; - - if (hostname.includes('my.fancy.prose.tests')) { - return process.nextTick(() => { - callback(null, '127.0.0.1', 4); - }); - } - - const { wrappedMethod: lookup } = lookupStub; - lookup(hostname, options, callback); - }); - } - - it('10 - All DNS records are selected (srvMaxHosts = 0)', async () => { - const replacementRecords = [ - { name: 'localhost.my.fancy.prose.tests', port: 2000, weight: 0, priority: 0 }, - { name: 'localhost.my.fancy.prose.tests', port: 2002, weight: 0, priority: 0 }, - { name: 'localhost.my.fancy.prose.tests', port: 2003, weight: 0, priority: 0 } - ]; - - makeStubs({ replacementRecords, mockLimit: 2 }); - - client = new MongoClient(SRV_CONNECTION_STRING, { - tls: false, - srvMaxHosts: 0, - serverSelectionTimeoutMS: 5000 - }); - await client.connect(); - - const selectedHosts = client.topology.s.seedlist; - expect(selectedHosts).to.have.lengthOf(dnsRecordsForTest1.length); - expect(selectedHosts.map(({ host }) => host)).to.deep.equal( - dnsRecordsForTest1.map(({ name }) => name) - ); - - clock.tick(2 * client.topology.s.srvPoller.intervalMS); - await processTick(); - - const polledServerAddresses = Array.from(client.topology.description.servers.keys()); - polledServerAddresses.sort(); - expect(polledServerAddresses).to.deep.equal( - replacementRecords.map(({ name, port }) => `${name}:${port}`) - ); - }); - - it('11 - All DNS records are selected (srvMaxHosts >= records)', async () => { - const replacementRecords = [ - { name: 'localhost.my.fancy.prose.tests', port: 2002, weight: 0, priority: 0 }, - { name: 'localhost.my.fancy.prose.tests', port: 2003, weight: 0, priority: 0 } - ]; - - makeStubs({ replacementRecords, mockLimit: 2 }); - - client = new MongoClient(SRV_CONNECTION_STRING, { - tls: false, - srvMaxHosts: 2, - serverSelectionTimeoutMS: 5000 - }); - await client.connect(); - - const selectedHosts = client.topology.s.seedlist; - expect(selectedHosts).to.have.lengthOf(2); - expect(selectedHosts.map(({ host }) => host)).to.deep.equal( - dnsRecordsForTest1.map(({ name }) => name) - ); - - clock.tick(2 * client.topology.s.srvPoller.intervalMS); - await processTick(); - - const polledServerAddresses = Array.from(client.topology.description.servers.keys()); - polledServerAddresses.sort(); - expect(polledServerAddresses).to.deep.equal( - replacementRecords.map(({ name, port }) => `${name}:${port}`) - ); - }); - - it('12 - New DNS records are randomly selected (srvMaxHosts > 0)', async () => { - const replacementRecords = [ - { name: 'localhost.my.fancy.prose.tests', port: 2000, weight: 0, priority: 0 }, - { name: 'localhost.my.fancy.prose.tests', port: 2002, weight: 0, priority: 0 }, - { name: 'localhost.my.fancy.prose.tests', port: 2003, weight: 0, priority: 0 } - ]; - - makeStubs({ replacementRecords, mockLimit: 2 }); - - client = new MongoClient(SRV_CONNECTION_STRING, { - tls: false, - srvMaxHosts: 2, - serverSelectionTimeoutMS: 5000 - }); - await client.connect(); - - const selectedHosts = client.topology.s.seedlist; - expect(selectedHosts).to.have.lengthOf(2); - expect(selectedHosts.map(({ host }) => host)).to.deep.equal( - dnsRecordsForTest1.map(({ name }) => name) - ); - - clock.tick(2 * client.topology.s.srvPoller.intervalMS); - await processTick(); - - const polledServerAddresses = Array.from(client.topology.description.servers.keys()); - polledServerAddresses.sort(); - // Only two addresses, one should remain the original 2000, - // while the other will be one of 2002 or 2003 - expect(polledServerAddresses).to.have.lengthOf(2); - expect(polledServerAddresses.find(addr => addr.includes('2000'))).to.be.a('string'); - expect(polledServerAddresses).satisfies( - addresses => - // If you want proof, comment one of these conditions out, and run the test a few times - // you should see it pass and fail at random - typeof addresses.find(addr => addr.includes('2002')) === 'string' || - typeof addresses.find(addr => addr.includes('2003')) === 'string' - ); - }); - - it('13 - DNS record with custom service name can be found', async () => { - client = new MongoClient(SRV_CONNECTION_STRING, { - tls: false, - srvServiceName: 'myFancySrvServiceName', - serverSelectionTimeoutMS: 5000 - }); - - makeStubs({ srvServiceName: 'myFancySrvServiceName' }); - - await client.connect(); - - clock.tick(2 * client.topology.s.srvPoller.intervalMS); - // No need to await process tick, since we're not checking DNS lookups - - const resolveSrvCalls = resolveSrvStub.getCalls(); - expect(resolveSrvCalls).to.have.lengthOf(2); - expect(resolveSrvCalls[0].args[0]).includes('myFancySrvServiceName'); - expect(resolveSrvCalls[1].args[0]).include('myFancySrvServiceName'); - }); - }); -}); diff --git a/test/unit/connection_string.test.js b/test/unit/connection_string.test.js index ce3aa6daf3..e17276e9c2 100644 --- a/test/unit/connection_string.test.js +++ b/test/unit/connection_string.test.js @@ -1,16 +1,10 @@ 'use strict'; -const fs = require('fs'); -const path = require('path'); -const { promisify } = require('util'); const { MongoParseError, MongoDriverError, MongoInvalidArgumentError } = require('../../src/error'); const { loadSpecTests } = require('../spec'); -const { parseOptions, resolveSRVRecord } = require('../../src/connection_string'); +const { parseOptions } = require('../../src/connection_string'); const { AuthMechanism } = require('../../src/cmap/auth/defaultAuthProviders'); const { expect } = require('chai'); -// const dns = require('dns'); -// const { Topology } = require('../../src/sdam/topology'); -// const { HostAddress } = require('../../src/utils'); // NOTE: These are cases we could never check for unless we write our own // url parser. The node parser simply won't let these through, so we @@ -29,8 +23,6 @@ const skipTests = [ 'Deprecated (or unknown) options are ignored if replacement exists' ]; -const SPEC_PATHS = ['load-balanced', 'replica-set', 'sharded']; - describe('Connection String', function () { it('should not support auth passed with user', function () { const optionsWithUser = { @@ -227,113 +219,5 @@ describe('Connection String', function () { expect(options.dbName).to.equal('somedb'); expect(options.srvHost).to.equal('test1.test.build.10gen.cc'); }); - - for (const folder of SPEC_PATHS) { - describe(`spec tests ${folder}`, function () { - const specPath = path.join(__dirname, '../spec', 'initial-dns-seedlist-discovery', folder); - const testFiles = fs - .readdirSync(specPath) - .filter(x => x.indexOf('.json') !== -1) - .map(x => [x, fs.readFileSync(path.join(specPath, x), 'utf8')]) - .map(x => [path.basename(x[0], '.json'), JSON.parse(x[1])]); - - for (const [fileName, test] of testFiles) { - if (!test.comment) { - test.comment = fileName; - } - - const comment = test.comment; - it(comment, { - metadata: { requires: { topology: ['single'] } }, - async test() { - let thrownError; - let options; - let hosts; - try { - options = parseOptions(test.uri); - hosts = await promisify(resolveSRVRecord)(options); - } catch (error) { - thrownError = error; - } - - if (test.error) { - expect(thrownError).to.exist; - expect(hosts).to.not.exist; - return; // Nothing more to test... - } - - expect(thrownError).to.not.exist; - expect(options).to.exist; - - // Implicit SRV options must be set. - expect(options.directConnection).to.be.false; - const testOptions = test.options; - if (testOptions && 'tls' in testOptions) { - expect(options).to.have.property('tls', testOptions.tls); - } else if (testOptions && 'ssl' in testOptions) { - expect(options).to.have.property('tls', testOptions.ssl); - } else { - expect(options.tls).to.be.true; - } - if (testOptions && testOptions.replicaSet) { - expect(options).to.have.property('replicaSet', testOptions.replicaSet); - } - if (testOptions && testOptions.authSource) { - expect(options).to.have.property('credentials'); - expect(options.credentials.source).to.equal(testOptions.authSource); - } - if (testOptions && testOptions.loadBalanced) { - expect(options).to.have.property('loadBalanced', testOptions.loadBalanced); - } - if (test.parsed_options && test.parsed_options.user && test.parsed_options.password) { - expect(options.credentials.username).to.equal(test.parsed_options.user); - expect(options.credentials.password).to.equal(test.parsed_options.password); - } - - // srvMaxHost limiting happens in the topology constructor - if (options.srvHost && comment.includes('srvMaxHosts')) { - return this.skip(); // REMOVE ME!!!! - // const topology = new Topology(hosts, options); - // const initialSeedlist = hosts.map(h => h.toString()); - // const selectedHosts = Array.from(topology.s.description.servers.keys()); - - // if (typeof test.numSeeds === 'number') { - // // numSeeds: the expected number of initial seeds discovered from the SRV record. - // expect(initialSeedlist).to.have.lengthOf(test.numSeeds); - // } - // if (typeof test.numHosts === 'number') { - // // numHosts: the expected number of hosts discovered once SDAM completes a scan. - // // (In our case, its the Topology constructor, but not actual SDAM) - // expect(selectedHosts).to.have.lengthOf(test.numHosts); - // } - - // if (Array.isArray(test.seeds)) { - // // verify that the set of hosts in the client's initial seedlist - // // matches the list in seeds - // expect(initialSeedlist).to.deep.equal(test.seeds); - // } - // if (Array.isArray(test.hosts)) { - // // verify that the set of ServerDescriptions in the client's TopologyDescription - // // eventually matches the list in hosts - // const actualAddresses = await Promise.all( - // selectedHosts - // .map(async hn => await promisify(dns.lookup)(HostAddress.fromString(hn).host)) - // .map(async (addr, i) => { - // let address = (await addr).address; - // address = address === '127.0.0.1' ? 'localhost' : address; - // return HostAddress.fromString( - // `${address}:${HostAddress.fromString(selectedHosts[i]).port}` - // ).toString(); - // }) - // ); - - // expect(actualAddresses).to.deep.equal(test.hosts); - // } - } - } - }); - } - }); - } }); }); diff --git a/test/unit/mongo_client.test.js b/test/unit/mongo_client.test.js index 3762258b5f..51d5fe7cfe 100644 --- a/test/unit/mongo_client.test.js +++ b/test/unit/mongo_client.test.js @@ -734,12 +734,21 @@ describe('MongoOptions', function () { expect(() => { new MongoClient('mongodb+srv://localhost?loadBalanced=true', { srvMaxHosts: 2 }); }).to.throw(MongoParseError, 'Cannot limit srv hosts with loadBalanced enabled'); + + // These should not throw. + new MongoClient('mongodb+srv://localhost?srvMaxHosts=0&replicaSet=repl'); + new MongoClient('mongodb+srv://localhost', { srvMaxHosts: 0, replicaSet: 'blah' }); + new MongoClient('mongodb+srv://localhost?srvMaxHosts=0&loadBalanced=true'); + new MongoClient('mongodb+srv://localhost?loadBalanced=true', { srvMaxHosts: 0 }); }); it('srvServiceName and srvMaxHosts cannot be used on a non-srv connection string', () => { expect(() => { new MongoClient('mongodb://localhost?srvMaxHosts=2'); }).to.throw(MongoParseError); + expect(() => { + new MongoClient('mongodb://localhost?srvMaxHosts=0'); + }).to.throw(MongoParseError); expect(() => { new MongoClient('mongodb://localhost?srvServiceName=abc'); }).to.throw(MongoParseError); diff --git a/test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts b/test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts new file mode 100644 index 0000000000..cf67b49ac0 --- /dev/null +++ b/test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts @@ -0,0 +1,258 @@ +import * as dns from 'dns'; +import * as sinon from 'sinon'; +import { expect } from 'chai'; +import { MongoClient } from '../../src'; +import { processTick } from '../tools/utils'; +import { it } from 'mocha'; +import * as mock from '../tools/mock'; + +/* + The SRV Prose Tests make use of the following REAL DNS records. + We use sinon to replace the results from resolveSrv to test hostname removals and insertions. + + The prose tests assume you have a 4 node sharded cluster running on ports: + [27017, 27018, 27019, 27020] + + Record TTL Class Address + localhost.test.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.test3.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. +*/ + +const srvRecord = (name, port) => ({ name, port, weight: 0, priority: 0 }); +interface ShardedClusterMocks { + mongoses: mock.MockServer[]; + readonly srvRecords: dns.SrvRecord[]; +} + +// TODO(): Make use of the shared driver's DNS records + +describe('Polling Srv Records for Mongos Discovery', () => { + const SRV_CONNECTION_STRING = 'mongodb+srv://test.mock.test.build.10gen.cc'; + let shardedCluster: ShardedClusterMocks; + let resolveSrvStub: sinon.SinonStub; + let lookupStub: sinon.SinonStub; + let client: MongoClient; + let clock: sinon.SinonFakeTimers; + const initialRecords = [ + { name: 'localhost.test.mock.test.build.10gen.cc', port: 2017 }, + { name: 'localhost.test.mock.test.build.10gen.cc', port: 2018 } + ]; + + beforeEach(() => { + clock = sinon.useFakeTimers(); + }); + + afterEach(() => { + if (clock) { + clock.restore(); + clock = undefined; + } + }); + + beforeEach(async () => { + const mongoses = [ + await mock.createServer(2017), + await mock.createServer(2018), + await mock.createServer(2019), + await mock.createServer(2020) + ]; + + const srvRecords = mongoses.map(s => + srvRecord('localhost.test.mock.test.build.10gen.cc', s.port) + ); + + shardedCluster = { mongoses, srvRecords }; + + for (const mongos of shardedCluster.mongoses) { + mongos.setMessageHandler(request => { + const document = request.document; + + if (document.ismaster || document.hello) { + request.reply({ ...mock.HELLO, msg: 'isdbgrid' }); + } + }); + } + }); + + afterEach(async () => { + await mock.cleanup(); + }); + + afterEach(async () => { + if (resolveSrvStub) { + resolveSrvStub.restore(); + resolveSrvStub = undefined; + } + if (lookupStub) { + lookupStub.restore(); + lookupStub = undefined; + } + if (client) { + await client.close(); + client = undefined; + } + }); + + function makeStubs({ + initialRecords = undefined, + replacementRecords = undefined, + srvServiceName = 'mongodb' + }) { + let initialDNSLookup = true; + const mockRecords = shardedCluster.srvRecords; + replacementRecords ??= mockRecords; + initialRecords ??= mockRecords; + // for some reason the stub needs to be set up here. + // first call is for the driver initial connection + // second call will check the poller + resolveSrvStub = sinon.stub(dns, 'resolveSrv').callsFake((address, callback) => { + expect(address).to.equal(`_${srvServiceName}._tcp.test.mock.test.build.10gen.cc`); + if (initialDNSLookup) { + initialDNSLookup = false; + return process.nextTick(callback, null, initialRecords); + } + process.nextTick(callback, null, replacementRecords); + }); + + lookupStub = sinon.stub(dns, 'lookup').callsFake((...args) => { + const hostname = args[0]; + const options = typeof args[1] === 'object' ? args[1] : {}; + const callback = args[args.length - 1] as (err: null, address: string, family: 4) => void; + + if (hostname.includes('test.mock.test.build.10gen.cc')) { + return process.nextTick(callback, null, '127.0.0.1', 4); + } + + const { wrappedMethod: lookup } = lookupStub; + lookup(hostname, options, callback); + }); + } + + it('10 - All DNS records are selected (srvMaxHosts = 0)', async () => { + const replacementRecords = [ + { name: 'localhost.test.mock.test.build.10gen.cc', port: 2017, weight: 0, priority: 0 }, + { name: 'localhost.test.mock.test.build.10gen.cc', port: 2019, weight: 0, priority: 0 }, + { name: 'localhost.test.mock.test.build.10gen.cc', port: 2020, weight: 0, priority: 0 } + ]; + + makeStubs({ initialRecords, replacementRecords }); + + client = new MongoClient(SRV_CONNECTION_STRING, { + tls: false, // Need to turn off the automatic TLS turn on with SRV connection strings + srvMaxHosts: 0, + serverSelectionTimeoutMS: 5000 // This is just to make the test fail in a nice amount of time + }); + await client.connect(); + + const selectedHosts = client.topology.s.seedlist; + expect(selectedHosts).to.have.lengthOf(initialRecords.length); + expect(selectedHosts.map(({ host }) => host)).to.deep.equal( + initialRecords.map(({ name }) => name) + ); + + clock.tick(2 * client.topology.s.srvPoller.rescanSrvIntervalMS); + await processTick(); + + const polledServerAddresses = Array.from(client.topology.description.servers.keys()); + polledServerAddresses.sort(); + expect(polledServerAddresses).to.deep.equal( + replacementRecords.map(({ name, port }) => `${name}:${port}`) + ); + }); + + it('11 - All DNS records are selected (srvMaxHosts >= records)', async () => { + const replacementRecords = [ + { name: 'localhost.test.mock.test.build.10gen.cc', port: 2019, weight: 0, priority: 0 }, + { name: 'localhost.test.mock.test.build.10gen.cc', port: 2020, weight: 0, priority: 0 } + ]; + + makeStubs({ initialRecords, replacementRecords }); + + client = new MongoClient(SRV_CONNECTION_STRING, { + tls: false, + srvMaxHosts: 2, + serverSelectionTimeoutMS: 5000 + }); + await client.connect(); + + const selectedHosts = client.topology.s.seedlist; + expect(selectedHosts).to.have.lengthOf(2); + expect(selectedHosts.map(({ host }) => host)).to.deep.equal( + initialRecords.map(({ name }) => name) + ); + + clock.tick(2 * client.topology.s.srvPoller.rescanSrvIntervalMS); + await processTick(); + + const polledServerAddresses = Array.from(client.topology.description.servers.keys()); + polledServerAddresses.sort(); + expect(polledServerAddresses).to.deep.equal( + replacementRecords.map(({ name, port }) => `${name}:${port}`) + ); + }); + + it('12 - New DNS records are randomly selected (srvMaxHosts > 0)', async () => { + const replacementRecords = [ + { name: 'localhost.test.mock.test.build.10gen.cc', port: 2017, weight: 0, priority: 0 }, + { name: 'localhost.test.mock.test.build.10gen.cc', port: 2019, weight: 0, priority: 0 }, + { name: 'localhost.test.mock.test.build.10gen.cc', port: 2020, weight: 0, priority: 0 } + ]; + + makeStubs({ initialRecords, replacementRecords }); + + client = new MongoClient(SRV_CONNECTION_STRING, { + tls: false, + srvMaxHosts: 2, + serverSelectionTimeoutMS: 5000 + }); + await client.connect(); + + const selectedHosts = client.topology.s.seedlist; + expect(selectedHosts).to.have.lengthOf(2); + expect(selectedHosts.map(({ host }) => host)).to.deep.equal( + initialRecords.map(({ name }) => name) + ); + + clock.tick(2 * client.topology.s.srvPoller.rescanSrvIntervalMS); + await processTick(); + + const polledServerAddresses = Array.from(client.topology.description.servers.keys()); + polledServerAddresses.sort(); + // Only two addresses, one should remain the original 2017, + // while the other will be one of 2019 or 2020 + expect(polledServerAddresses).to.have.lengthOf(2); + expect(polledServerAddresses.find(addr => addr.includes('2017'))).to.be.a('string'); + expect(polledServerAddresses).satisfies( + addresses => + // If you want proof, comment one of these conditions out, and run the test a few times + // you should see it pass and fail at random + typeof addresses.find(addr => addr.includes('2019')) === 'string' || + typeof addresses.find(addr => addr.includes('2020')) === 'string' + ); + }); + + it('13 - DNS record with custom service name can be found', async () => { + client = new MongoClient(SRV_CONNECTION_STRING, { + tls: false, + srvServiceName: 'myFancySrvServiceName', + serverSelectionTimeoutMS: 5000 + }); + + makeStubs({ srvServiceName: 'myFancySrvServiceName' }); + + await client.connect(); + + clock.tick(2 * client.topology.s.srvPoller.rescanSrvIntervalMS); + // No need to await process tick, since we're not checking DNS lookups + + const resolveSrvCalls = resolveSrvStub.getCalls(); + expect(resolveSrvCalls).to.have.lengthOf(2); + expect(resolveSrvCalls[0].args[0]).includes('myFancySrvServiceName'); + expect(resolveSrvCalls[1].args[0]).include('myFancySrvServiceName'); + }); +}); From 34bc0d41329f4a88bd97548dce626ac2fe13e63f Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 15 Nov 2021 18:23:23 -0500 Subject: [PATCH 23/30] add ticket mention --- .../initial_dns_seedlist_discovery.spec.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/initial-dns-seedlist-discovery/initial_dns_seedlist_discovery.spec.test.ts b/test/integration/initial-dns-seedlist-discovery/initial_dns_seedlist_discovery.spec.test.ts index 2496fba48c..7526727e4b 100644 --- a/test/integration/initial-dns-seedlist-discovery/initial_dns_seedlist_discovery.spec.test.ts +++ b/test/integration/initial-dns-seedlist-discovery/initial_dns_seedlist_discovery.spec.test.ts @@ -17,7 +17,7 @@ function makeTest(test, topology) { }); it(test.comment, async function () { - return this.skip(); // FIXME + return this.skip(); // FIXME(NODE-3757): These tests require specific environment setups, also the error cases need better assertions if (topology === 'replica-set' && this.configuration.topologyType !== 'ReplicaSetWithPrimary') { return this.skip(); } From beb74c5fd1ac07f34cfc057badc4610aef31ca46 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 15 Nov 2021 18:34:24 -0500 Subject: [PATCH 24/30] clarify records --- .../polling_srv_records_for_mongos_discovery.prose.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts b/test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts index cf67b49ac0..6f56869dde 100644 --- a/test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts +++ b/test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts @@ -8,9 +8,10 @@ import * as mock from '../tools/mock'; /* The SRV Prose Tests make use of the following REAL DNS records. + CURRENTLY, WE DO NOT USE THESE. We have stubbed the methods to build our own fake data for testing. We use sinon to replace the results from resolveSrv to test hostname removals and insertions. - The prose tests assume you have a 4 node sharded cluster running on ports: + The actual spec prose assumes you have a 4 node sharded cluster running on ports: [27017, 27018, 27019, 27020] Record TTL Class Address From 0e453195bc4893241fc4dcf51a001ba8177c1c74 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 15 Nov 2021 18:36:19 -0500 Subject: [PATCH 25/30] remove comment --- test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts b/test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts index 6f56869dde..b96ee1a27a 100644 --- a/test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts +++ b/test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts @@ -108,7 +108,6 @@ describe('Polling Srv Records for Mongos Discovery', () => { const mockRecords = shardedCluster.srvRecords; replacementRecords ??= mockRecords; initialRecords ??= mockRecords; - // for some reason the stub needs to be set up here. // first call is for the driver initial connection // second call will check the poller resolveSrvStub = sinon.stub(dns, 'resolveSrv').callsFake((address, callback) => { From d9e8f285753ed913e50103a2bae211e6a98ea228 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 15 Nov 2021 18:46:30 -0500 Subject: [PATCH 26/30] prevent mutation --- .../polling_srv_records_for_mongos_discovery.prose.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts b/test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts index b96ee1a27a..596f0da158 100644 --- a/test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts +++ b/test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts @@ -39,10 +39,10 @@ describe('Polling Srv Records for Mongos Discovery', () => { let lookupStub: sinon.SinonStub; let client: MongoClient; let clock: sinon.SinonFakeTimers; - const initialRecords = [ + const initialRecords = Object.freeze([ { name: 'localhost.test.mock.test.build.10gen.cc', port: 2017 }, { name: 'localhost.test.mock.test.build.10gen.cc', port: 2018 } - ]; + ]); beforeEach(() => { clock = sinon.useFakeTimers(); From eec372e449d7637062e6055fa0cc388c13af7616 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 16 Nov 2021 09:10:37 -0500 Subject: [PATCH 27/30] test: add object option test, clean up assertions --- test/unit/mongo_client.test.js | 3 +++ ...v_records_for_mongos_discovery.prose.test.ts | 17 +++++++++++++---- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/test/unit/mongo_client.test.js b/test/unit/mongo_client.test.js index 51d5fe7cfe..e5ffce996b 100644 --- a/test/unit/mongo_client.test.js +++ b/test/unit/mongo_client.test.js @@ -749,6 +749,9 @@ describe('MongoOptions', function () { expect(() => { new MongoClient('mongodb://localhost?srvMaxHosts=0'); }).to.throw(MongoParseError); + expect(() => { + new MongoClient('mongodb://localhost', { srvMaxHosts: 0 }); + }).to.throw(MongoParseError); expect(() => { new MongoClient('mongodb://localhost?srvServiceName=abc'); }).to.throw(MongoParseError); diff --git a/test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts b/test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts index 596f0da158..1807ee0102 100644 --- a/test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts +++ b/test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts @@ -226,24 +226,33 @@ describe('Polling Srv Records for Mongos Discovery', () => { // Only two addresses, one should remain the original 2017, // while the other will be one of 2019 or 2020 expect(polledServerAddresses).to.have.lengthOf(2); - expect(polledServerAddresses.find(addr => addr.includes('2017'))).to.be.a('string'); + expect(polledServerAddresses).to.include('localhost.test.mock.test.build.10gen.cc:2017'); expect(polledServerAddresses).satisfies( addresses => // If you want proof, comment one of these conditions out, and run the test a few times // you should see it pass and fail at random - typeof addresses.find(addr => addr.includes('2019')) === 'string' || - typeof addresses.find(addr => addr.includes('2020')) === 'string' + addresses.includes('localhost.test.mock.test.build.10gen.cc:2019') || + addresses.includes('localhost.test.mock.test.build.10gen.cc:2020') ); }); it('13 - DNS record with custom service name can be found', async () => { + const replacementRecords = [ + { name: 'localhost.test.mock.test.build.10gen.cc', port: 2019, weight: 0, priority: 0 }, + { name: 'localhost.test.mock.test.build.10gen.cc', port: 2020, weight: 0, priority: 0 } + ]; + client = new MongoClient(SRV_CONNECTION_STRING, { tls: false, srvServiceName: 'myFancySrvServiceName', serverSelectionTimeoutMS: 5000 }); - makeStubs({ srvServiceName: 'myFancySrvServiceName' }); + makeStubs({ + initialRecords: [initialRecords[0]], + replacementRecords, + srvServiceName: 'myFancySrvServiceName' + }); await client.connect(); From 7465308a97ec1daec77835f989fa40a55c3f8586 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 16 Nov 2021 09:11:39 -0500 Subject: [PATCH 28/30] call makeStubs first in test 13 --- ...ng_srv_records_for_mongos_discovery.prose.test.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts b/test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts index 1807ee0102..5a2e7f3408 100644 --- a/test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts +++ b/test/unit/polling_srv_records_for_mongos_discovery.prose.test.ts @@ -242,18 +242,18 @@ describe('Polling Srv Records for Mongos Discovery', () => { { name: 'localhost.test.mock.test.build.10gen.cc', port: 2020, weight: 0, priority: 0 } ]; - client = new MongoClient(SRV_CONNECTION_STRING, { - tls: false, - srvServiceName: 'myFancySrvServiceName', - serverSelectionTimeoutMS: 5000 - }); - makeStubs({ initialRecords: [initialRecords[0]], replacementRecords, srvServiceName: 'myFancySrvServiceName' }); + client = new MongoClient(SRV_CONNECTION_STRING, { + tls: false, + srvServiceName: 'myFancySrvServiceName', + serverSelectionTimeoutMS: 5000 + }); + await client.connect(); clock.tick(2 * client.topology.s.srvPoller.rescanSrvIntervalMS); From ad521aad3dd0648e053c8ee46418a75a27667576 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 16 Nov 2021 11:45:54 -0500 Subject: [PATCH 29/30] fix: check for nullish srvMaxHosts --- src/sdam/topology.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index bf66bd87cb..5a9b8f54a5 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -294,7 +294,9 @@ export class Topology extends TypedEventEmitter { const topologyId = globalTopologyCounter++; const selectedHosts = - options.srvMaxHosts === 0 || options.srvMaxHosts >= seedlist.length + options.srvMaxHosts == null || + options.srvMaxHosts === 0 || + options.srvMaxHosts >= seedlist.length ? seedlist : shuffle(seedlist, options.srvMaxHosts); From 09b30d650cd1548d995c2ba730fc51d7e7d7eb68 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 16 Nov 2021 12:56:47 -0500 Subject: [PATCH 30/30] fix: increase the input size --- test/unit/utils.test.js | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/test/unit/utils.test.js b/test/unit/utils.test.js index 9be0a65414..15ba77841e 100644 --- a/test/unit/utils.test.js +++ b/test/unit/utils.test.js @@ -468,21 +468,22 @@ describe('driver utils', function () { }); it('should not mutate the original input', function () { - const input = Object.freeze(['a', 'b', 'c', 'd', 'e']); + const input = Object.freeze(['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j']); const output = shuffle(input); // This will throw if shuffle tries to edit the input + expect(output === input).to.be.false; expect(output).to.not.deep.equal(input); expect(output).to.have.lengthOf(input.length); }); it(`should give a random subset of length equal to limit when limit is less than the input length`, function () { - const input = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']; + const input = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j']; const output = shuffle(input, input.length - 1); expect(output).to.not.deep.equal(input); expect(output).to.have.lengthOf(input.length - 1); }); it(`should give a random shuffling of the entire input when limit is equal to input length`, function () { - const input = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']; + const input = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j']; const output = shuffle(input, input.length); expect(output).to.not.deep.equal(input); expect(output).to.have.lengthOf(input.length); @@ -501,7 +502,7 @@ describe('driver utils', function () { }); it(`should return a random item on every call of limit 1`, function () { - const input = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']; + const input = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j']; const outputs = new Set(); for (let i = 0; i < 5; i++) { const output = shuffle(input, 1); @@ -513,7 +514,7 @@ describe('driver utils', function () { }); it('should give a random shuffling of the entire input when no limit provided', () => { - const input = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']; + const input = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j']; const output = shuffle(input); // Of course it is possible a shuffle returns exactly the same as the input // but it is so improbable it is worth the flakiness in my opinion @@ -521,7 +522,7 @@ describe('driver utils', function () { expect(output).to.have.lengthOf(input.length); }); it('should give a random shuffling of the entire input when limit is explicitly set to 0', () => { - const input = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']; + const input = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j']; const output = shuffle(input, 0); expect(output).to.not.deep.equal(input); expect(output).to.have.lengthOf(input.length);