Skip to content

Commit

Permalink
fix: skip config validation when using connector
Browse files Browse the repository at this point in the history 
This changeset fixes a problem in the original custom `connector`
factory method implementation that required the `server` config property
to be defined even though its value is ignored.

Removing the validation when `config.options.connector` is defined fixes
the problem.

Ref: #1540
Fixes: #1541
Signed-off-by: Ruy Adorno <ruyadorno@google.com>
  • Loading branch information
@ruyadorno
ruyadorno committed Jul 24, 2023
1 parent b78df14 commit 634c947
Show file tree
Hide file tree
Showing 3 changed files with 144 additions and 20 deletions.
58 changes: 42 additions & 16 deletions src/connection.ts
View file
Expand Up @@ -331,7 +331,7 @@ interface ErrorWithCode extends Error {
}

interface InternalConnectionConfig {
server: string;
server: undefined | string;
authentication: DefaultAuthentication | NtlmAuthentication | AzureActiveDirectoryPasswordAuthentication | AzureActiveDirectoryMsiAppServiceAuthentication | AzureActiveDirectoryMsiVmAuthentication | AzureActiveDirectoryAccessTokenAuthentication | AzureActiveDirectoryServicePrincipalSecret | AzureActiveDirectoryDefaultAuthentication;
options: InternalConnectionOptions;
}
Expand Down Expand Up @@ -1061,7 +1061,7 @@ class Connection extends EventEmitter {
throw new TypeError('The "config" argument is required and must be of type Object.');
}

if (typeof config.server !== 'string') {
if (typeof config.server !== 'string' && !config.options!.connector) {
throw new TypeError('The "config.server" property is required and must be of type string.');
}

Expand Down Expand Up @@ -1352,8 +1352,15 @@ class Connection extends EventEmitter {
if (typeof config.options.connector !== 'function') {
throw new TypeError('The "config.options.connector" property must be a function.');
}
if (config.server) {
throw new Error('Server and connector are mutually exclusive, but ' + config.server + ' and a connector function were provided');
}
if (config.options.port) {
throw new Error('Port and connector are mutually exclusive, but ' + config.options.port + ' and a connector function were provided');
}

this.config.options.connector = config.options.connector;
this.config.options.port = undefined;
}

if (config.options.cryptoCredentialsDetails !== undefined) {
Expand Down Expand Up @@ -1917,7 +1924,10 @@ class Connection extends EventEmitter {
initialiseConnection() {
const signal = this.createConnectTimer();

if (this.config.options.port) {
if (this.config.options.connector) {
// port and multiSubnetFailover are not used when using a custom connector
return this.connectOnPort(0, false, signal, this.config.options.connector);
} else if (this.config.options.port) {
return this.connectOnPort(this.config.options.port, this.config.options.multiSubnetFailover, signal, this.config.options.connector);
} else {
return instanceLookup({
Expand Down Expand Up @@ -1990,7 +2000,7 @@ class Connection extends EventEmitter {
return new TokenStreamParser(message, this.debug, handler, this.config.options);
}

socketHandlingForSendPreLogin(socket: net.Socket) {
socketHandlingForSendPreLogin(socket: net.Socket, customConnector: boolean) {
socket.on('error', (error) => { this.socketError(error); });
socket.on('close', () => { this.socketClose(); });
socket.on('end', () => { this.socketEnd(); });
Expand All @@ -2002,19 +2012,24 @@ class Connection extends EventEmitter {
this.socket = socket;

this.closed = false;
this.debug.log('connected to ' + this.config.server + ':' + this.config.options.port);
const message =
'connected to ' + this.config.server + ':' + this.config.options.port;
const customConnectorMessage =
'connected via custom connector';
this.debug.log(customConnector ? customConnectorMessage : message);

this.sendPreLogin();
this.transitionTo(this.STATE.SENT_PRELOGIN);
}

wrapWithTls(socket: net.Socket): Promise<tls.TLSSocket> {
return new Promise((resolve, reject) => {
const server = String(this.config.server);
const secureContext = tls.createSecureContext(this.secureContextOptions);
// If connect to an ip address directly,
// need to set the servername to an empty string
// if the user has not given a servername explicitly
const serverName = !net.isIP(this.config.server) ? this.config.server : '';
const serverName = !net.isIP(server) ? server : '';
const encryptOptions = {
host: this.config.server,
socket: socket,
Expand All @@ -2034,7 +2049,7 @@ class Connection extends EventEmitter {

connectOnPort(port: number, multiSubnetFailover: boolean, signal: AbortSignal, customConnector?: () => Promise<net.Socket>) {
const connectOpts = {
host: this.routingData ? this.routingData.server : this.config.server,
host: this.routingData ? this.routingData.server : String(this.config.server),
port: this.routingData ? this.routingData.port : port,
localAddress: this.config.options.localAddress
};
Expand All @@ -2055,7 +2070,7 @@ class Connection extends EventEmitter {
}
}

this.socketHandlingForSendPreLogin(socket);
this.socketHandlingForSendPreLogin(socket, Boolean(customConnector));
})().catch((err) => {
this.clearConnectTimer();

Expand Down Expand Up @@ -2137,8 +2152,10 @@ class Connection extends EventEmitter {
// otherwise, leave the message empty.
const routingMessage = this.routingData ? ` (redirected from ${this.config.server}${hostPostfix})` : '';
const message = `Failed to connect to ${server}${port}${routingMessage} in ${this.config.options.connectTimeout}ms`;
this.debug.log(message);
this.emit('connect', new ConnectionError(message, 'ETIMEOUT'));
const customConnectorMessage = `Failed to connect using custom connector in ${this.config.options.connectTimeout}ms`;
const errMessage = this.config.options.connector ? customConnectorMessage : message;
this.debug.log(errMessage);
this.emit('connect', new ConnectionError(errMessage, 'ETIMEOUT'));
this.connectTimer = undefined;
this.dispatchEvent('connectTimeout');
}
Expand Down Expand Up @@ -2273,8 +2290,10 @@ class Connection extends EventEmitter {
// otherwise, leave the message empty.
const routingMessage = this.routingData ? ` (redirected from ${this.config.server}${hostPostfix})` : '';
const message = `Failed to connect to ${server}${port}${routingMessage} - ${error.message}`;
this.debug.log(message);
this.emit('connect', new ConnectionError(message, 'ESOCKET'));
const customConnectorMessage = `Failed to connect using custom connector - ${error.message}`;
const errMessage = this.config.options.connector ? customConnectorMessage : message;
this.debug.log(errMessage);
this.emit('connect', new ConnectionError(errMessage, 'ESOCKET'));
} else {
const message = `Connection lost - ${error.message}`;
this.debug.log(message);
Expand All @@ -2299,15 +2318,21 @@ class Connection extends EventEmitter {
* @private
*/
socketClose() {
this.debug.log('connection to ' + this.config.server + ':' + this.config.options.port + ' closed');
const message = 'connection to ' + this.config.server + ':' + this.config.options.port + ' closed';
const customConnectorMessage = 'connection closed';
this.debug.log(this.config.options.connector ? customConnectorMessage : message);
if (this.state === this.STATE.REROUTING) {
this.debug.log('Rerouting to ' + this.routingData!.server + ':' + this.routingData!.port);
const message = 'Rerouting to ' + this.routingData!.server + ':' + this.routingData!.port;
const customConnectorMessage = 'Rerouting';
this.debug.log(this.config.options.connector ? customConnectorMessage : message);

this.dispatchEvent('reconnect');
} else if (this.state === this.STATE.TRANSIENT_FAILURE_RETRY) {
const server = this.routingData ? this.routingData.server : this.config.server;
const port = this.routingData ? this.routingData.port : this.config.options.port;
this.debug.log('Retry after transient failure connecting to ' + server + ':' + port);
const message = 'Retry after transient failure connecting to ' + server + ':' + port;
const customConnectorMessage = 'Retry after transient failure connecting';
this.debug.log(this.config.options.connector ? customConnectorMessage : message);

this.dispatchEvent('retry');
} else {
Expand Down Expand Up @@ -3254,7 +3279,8 @@ Connection.prototype.STATE = {

try {
this.transitionTo(this.STATE.SENT_TLSSSLNEGOTIATION);
await this.messageIo.startTls(this.secureContextOptions, this.config.options.serverName ? this.config.options.serverName : this.routingData?.server ?? this.config.server, this.config.options.trustServerCertificate);
const serverName = this.config.options.serverName ? this.config.options.serverName : String(this.routingData?.server ?? this.config.server);
await this.messageIo.startTls(this.secureContextOptions, serverName, this.config.options.trustServerCertificate);
} catch (err: any) {
return this.socketError(err);
}
Expand Down
4 changes: 2 additions & 2 deletions src/instance-lookup.ts
View file
Expand Up @@ -14,7 +14,7 @@ const MYSTERY_HEADER_LENGTH = 3;
type LookupFunction = (hostname: string, options: dns.LookupAllOptions, callback: (err: NodeJS.ErrnoException | null, addresses: dns.LookupAddress[]) => void) => void;

// Most of the functionality has been determined from from jTDS's MSSqlServerInfo class.
export async function instanceLookup(options: { server: string, instanceName: string, timeout?: number, retries?: number, port?: number, lookup?: LookupFunction, signal: AbortSignal }) {
export async function instanceLookup(options: { server: undefined | string, instanceName: string, timeout?: number, retries?: number, port?: number, lookup?: LookupFunction, signal: AbortSignal }) {
const server = options.server;
if (typeof server !== 'string') {
throw new TypeError('Invalid arguments: "server" must be a string');
Expand Down Expand Up @@ -57,7 +57,7 @@ export async function instanceLookup(options: { server: string, instanceName: st
try {
response = await withTimeout(timeout, async (signal) => {
const request = Buffer.from([0x02]);
return await sendMessage(options.server, port, lookup, signal, request);
return await sendMessage(String(options.server), port, lookup, signal, request);
}, signal);
} catch (err) {
// If the current attempt timed out, continue with the next
Expand Down
102 changes: 100 additions & 2 deletions test/unit/custom-connector.js
View file
Expand Up @@ -28,7 +28,6 @@ describe('custom connector', function() {
const host = server.address().address;
const port = server.address().port;
const connection = new Connection({
server: host,
options: {
connector: async () => {
customConnectorCalled = true;
Expand All @@ -37,7 +36,6 @@ describe('custom connector', function() {
port,
});
},
port
},
});

Expand All @@ -59,4 +57,104 @@ describe('custom connector', function() {

connection.connect();
});

it('connection timeout using a custom connector', function(done) {
const host = server.address().address;
const port = server.address().port;
const connection = new Connection({
options: {
connectTimeout: 10,
connector: async () => {
return net.connect({
host,
port,
});
},
},
});

// times out since no server response is defined
connection.connect((err) => {
assert.strictEqual(
err.code,
'ETIMEOUT',
'should emit timeout error code'
);
assert.strictEqual(
err.message,
'Failed to connect using custom connector in 10ms',
'should emit expected custom connector timeout error msg'
);

done();
});
});

it('should emit socket error custom connector msg', function(done) {
const connection = new Connection({
options: {
connector: async () => {
throw new Error('ERR');
},
},
});

connection.connect((err) => {
assert.strictEqual(
err.code,
'ESOCKET',
'should emit expected error code'
);
assert.strictEqual(
err.message,
'Failed to connect using custom connector - ERR',
'should emit expected custom connector error msg'
);
done();
});
});

it('should only accept functions', function(done) {
assert.throws(() => {
new Connection({
options: {
connector: 'foo',
},
});
}, Error, 'The "config.options.connector" property must be a function.');
done();
});

it('should not allow setting both server and connector options', function(done) {
assert.throws(() => {
new Connection({
server: '0.0.0.0',
options: {
connector: async () => {},
},
});
}, Error, 'Server and connector are mutually exclusive, but 0.0.0.0 and a connector function were provided');
done();
});

it('should not allow setting both port and connector options', function(done) {
assert.throws(() => {
new Connection({
options: {
connector: async () => {},
port: 8080,
},
});
}, Error, 'Port and connector are mutually exclusive, but 8080 and a connector function were provided');
done();
});

it('should require server config option if custom connector is undefined', function(done) {
assert.throws(() => {
new Connection({
options: { port: 8080 },
});
}, TypeError, 'The "config.server" property is required and must be of type string.');
done();
});
});

0 comments on commit 634c947

Please sign in to comment.