Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: skip config validation when using connector #1542

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: skip config validation when using connector #1542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
634c947
fix: skip config validation when using connector
ruyadorno May 30, 2023
faff247
Merge branch 'master' into fix-config-validation-when-using-connector…
arthurschreiber Aug 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 42 additions & 16 deletions src/connection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
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.');
ruyadorno marked this conversation as resolved.
Show resolved Hide resolved
}

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a possibility that user proved both port and connector from config, the logic will go into the if statement instead of the else if and still work properly since the connector is passed into connectOnPort. Another case is, if use only pass in connector and logic will got into the else if and still call the connectOnPort.
Under both cases, with or without port passed into connectOnPort function, the connect socket here will be provided by the passed in customConnector, and the passed in port number should be ignored within connectOnPort.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks @MichaelSun90! these are also great examples of the kind of tests I can add to make codecov happy 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the connector check to take precedence just to err on the safe side but at the same time I made sure to set port to undefined during the validation phase of the constructor along with adding tests for the combinations of providing port/server/connector. Let me know if it looks better now 😊

} 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({
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is the place you need to change. We know here that this.config.server is not undefined in this else block (even if the type is declared as string | undefined), so we can tell TypeScript that this is the case by changing this.config.server to this.config.server!.

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
Open in desktop
Original file line number Diff line number Diff line change
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 }) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ruyadorno I think this change is not correct. Logically, the instanceLookup can not work without a hostname, so changing the type to allow undefined is not correct. Also, casting options.server to a string using String further down is also not right - if someone would call this method with undefined, it would try to send udp messages to the hostname "undefined". 😅

I think it's better to change the location where this method is called instead (will leave a separate comment for that).

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
Open in desktop
Original file line number Diff line number Diff line change
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();
});
});