From 2e72a87f413cd2baa155663f0b787e6b61726fe6 Mon Sep 17 00:00:00 2001 From: Michael Sun Date: Wed, 2 Nov 2022 11:09:05 -0700 Subject: [PATCH 1/5] feat: add support for instance name Co-authored-by: mShan0 Co-authored-by: Arthur Schreiber --- src/connection.ts | 15 ++++++++------- src/prelogin-payload.ts | 28 ++++++++++++++++++---------- src/token/handler.ts | 8 ++++---- test/integration/connection-test.js | 25 +++++++++++++++++++++++++ test/unit/prelogin-payload-test.js | 7 ++++++- 5 files changed, 61 insertions(+), 22 deletions(-) diff --git a/src/connection.ts b/src/connection.ts index 353744dfb..42ccf963a 100644 --- a/src/connection.ts +++ b/src/connection.ts @@ -854,6 +854,7 @@ const CLEANUP_TYPE = { interface RoutingData { server: string; port: number; + instanceName: string | undefined; } /** @@ -1272,10 +1273,6 @@ class Connection extends EventEmitter { }; if (config.options) { - if (config.options.port && config.options.instanceName) { - throw new Error('Port and instanceName are mutually exclusive, but ' + config.options.port + ' and ' + config.options.instanceName + ' provided'); - } - if (config.options.abortTransactionOnError !== undefined) { if (typeof config.options.abortTransactionOnError !== 'boolean' && config.options.abortTransactionOnError !== null) { throw new TypeError('The "config.options.abortTransactionOnError" property must be of type string or null.'); @@ -1510,7 +1507,6 @@ class Connection extends EventEmitter { } this.config.options.instanceName = config.options.instanceName; - this.config.options.port = undefined; } if (config.options.isolationLevel !== undefined) { @@ -1561,7 +1557,6 @@ class Connection extends EventEmitter { } this.config.options.port = config.options.port; - this.config.options.instanceName = undefined; } if (config.options.readOnlyIntent !== undefined) { @@ -2241,7 +2236,8 @@ class Connection extends EventEmitter { const payload = new PreloginPayload({ encrypt: this.config.options.encrypt, - version: { major: Number(major), minor: Number(minor), build: Number(build), subbuild: 0 } + version: { major: Number(major), minor: Number(minor), build: Number(build), subbuild: 0 }, + instanceName: this.routingData?.instanceName ?? this.config.options.instanceName }); this.messageIo.sendMessage(TYPE.PRELOGIN, payload.data); @@ -3154,6 +3150,11 @@ Connection.prototype.STATE = { } const preloginPayload = new PreloginPayload(messageBuffer); + if (preloginPayload.instanceName !== undefined && preloginPayload.instanceName !== '\x00') { + this.emit('connect', new ConnectionError('Server instanceName does not match')); + return this.close(); + } + this.debug.payload(function() { return preloginPayload.toString(' '); }); diff --git a/src/prelogin-payload.ts b/src/prelogin-payload.ts index 77f1e6581..bbd5aa66f 100644 --- a/src/prelogin-payload.ts +++ b/src/prelogin-payload.ts @@ -48,6 +48,7 @@ interface Options { build: number; subbuild: number; }; + instanceName: string | undefined; } /* @@ -67,7 +68,7 @@ class PreloginPayload { encryption!: number; encryptionString!: string; - instance!: number; + instanceName!: string; threadId!: number; @@ -75,10 +76,10 @@ class PreloginPayload { marsString!: string; fedAuthRequired!: number; - constructor(bufferOrOptions: Buffer | Options = { encrypt: false, version: { major: 0, minor: 0, build: 0, subbuild: 0 } }) { + constructor(bufferOrOptions: Buffer | Options = { encrypt: false, version: { major: 0, minor: 0, build: 0, subbuild: 0 }, instanceName: undefined }) { if (bufferOrOptions instanceof Buffer) { this.data = bufferOrOptions; - this.options = { encrypt: false, version: { major: 0, minor: 0, build: 0, subbuild: 0 } }; + this.options = { encrypt: false, version: { major: 0, minor: 0, build: 0, subbuild: 0 }, instanceName: undefined }; } else { this.options = bufferOrOptions; this.createOptions(); @@ -90,7 +91,7 @@ class PreloginPayload { const options = [ this.createVersionOption(), this.createEncryptionOption(), - this.createInstanceOption(), + this.createInstanceNameOption(), this.createThreadIdOption(), this.createMarsOption(), this.createFedAuthOption() @@ -144,8 +145,11 @@ class PreloginPayload { }; } - createInstanceOption() { + createInstanceNameOption() { const buffer = new WritableTrackingBuffer(optionBufferSize); + if (this.options.instanceName !== undefined) { + buffer.writeString(this.options.instanceName, 'utf8'); + } buffer.writeUInt8(0x00); return { token: TOKEN.INSTOPT, @@ -193,7 +197,7 @@ class PreloginPayload { this.extractEncryption(dataOffset); break; case TOKEN.INSTOPT: - this.extractInstance(dataOffset); + this.extractInstanceName(dataOffset, dataLength); break; case TOKEN.THREADID: if (dataLength > 0) { @@ -226,8 +230,12 @@ class PreloginPayload { this.encryptionString = encryptByValue[this.encryption]; } - extractInstance(offset: number) { - this.instance = this.data.readUInt8(offset); + extractInstanceName(offset: number, dataLength: number) { + if (dataLength === 0) { + return; + } + + this.instanceName = this.data.toString('utf8', offset, offset + dataLength); } extractThreadId(offset: number) { @@ -245,11 +253,11 @@ class PreloginPayload { toString(indent = '') { return indent + 'PreLogin - ' + sprintf( - 'version:%d.%d.%d.%d, encryption:0x%02X(%s), instopt:0x%02X, threadId:0x%08X, mars:0x%02X(%s)', + 'version:%d.%d.%d.%d, encryption:0x%02X(%s), instanceName:%s, threadId:0x%08X, mars:0x%02X(%s)', this.version.major, this.version.minor, this.version.build, this.version.subbuild, this.encryption ? this.encryption : 0, this.encryptionString ? this.encryptionString : '', - this.instance ? this.instance : 0, + this.instanceName ? this.instanceName : '', this.threadId ? this.threadId : 0, this.mars ? this.mars : 0, this.marsString ? this.marsString : '' diff --git a/src/token/handler.ts b/src/token/handler.ts index 8d0088c9d..ca1e2d2bc 100644 --- a/src/token/handler.ts +++ b/src/token/handler.ts @@ -248,7 +248,7 @@ export class Login7TokenHandler extends TokenHandler { connection: Connection; fedAuthInfoToken: FedAuthInfoToken | undefined; - routingData: { server: string, port: number } | undefined; + routingData: { server: string, port: number, instanceName: string | undefined } | undefined; loginAckReceived = false; @@ -338,11 +338,11 @@ export class Login7TokenHandler extends TokenHandler { } onRoutingChange(token: RoutingEnvChangeToken) { - // Removes instance name attached to the redirect url. E.g., redirect.db.net\instance1 --> redirect.db.net - const [ server ] = token.newValue.server.split('\\'); + // Split the target into servername and instance name, e.g. redirect.db.net\instance1 --> redirect.db.net and instance1 + const [ server, instanceName ] = token.newValue.server.split('\\'); this.routingData = { - server, port: token.newValue.port + server, port: token.newValue.port, instanceName }; } diff --git a/test/integration/connection-test.js b/test/integration/connection-test.js index 00cc65701..70b5814d2 100644 --- a/test/integration/connection-test.js +++ b/test/integration/connection-test.js @@ -147,6 +147,31 @@ describe('Initiate Connect Test', function() { }); }); + it('should fail when connecting by port with wrong instance name', function(done) { + const config = getConfig(); + + config.options.instanceName = 'NonExistInstanceName'; + + if ((config.options != null ? config.options.port : undefined) == null) { + // Config says don't do this test (probably because ports are dynamic). + return this.skip(); + } + + const connection = new Connection(config); + + connection.connect((err) => { + assert.instanceOf(err, ConnectionError); + assert.strictEqual(err?.message, 'Server instanceName does not match'); + + done(); + }); + + connection.on('infoMessage', function(info) { + // console.log("#{info.number} : #{info.message}") + }); + }); + + it('should connect by instance name', function(done) { if (!getInstanceName()) { // Config says don't do this test (probably because SQL Server Browser is not available). diff --git a/test/unit/prelogin-payload-test.js b/test/unit/prelogin-payload-test.js index 56b8e5c78..6146ab061 100644 --- a/test/unit/prelogin-payload-test.js +++ b/test/unit/prelogin-payload-test.js @@ -8,7 +8,7 @@ function assertPayload(payload, encryptionString, { major, minor, build, subbuil assert.strictEqual(payload.version.subbuild, subbuild); assert.strictEqual(payload.encryptionString, encryptionString); - assert.strictEqual(payload.instance, 0); + assert.strictEqual(payload.instanceName, ''); assert.strictEqual(payload.threadId, 0); assert.strictEqual(payload.marsString, 'OFF'); assert.strictEqual(payload.fedAuthRequired, 1); @@ -25,6 +25,11 @@ describe('prelogin-payload-assert', function() { assertPayload(payload, 'ON', { major: 11, minor: 3, build: 2, subbuild: 0 }); }); + it('should accept an instance name', function() { + const payload = new PreloginPayload({ encrypt: true, version: { major: 11, minor: 3, build: 2, subbuild: 0 }, instanceName: 'MSSQLServer' }); + assert.strictEqual(payload.instanceName, 'MSSQLServer'); + }); + it('should create from buffer', function() { const payload = new PreloginPayload(); new PreloginPayload(payload.data); From c1b6583901adc63d46e55bca5d6006fc80724a56 Mon Sep 17 00:00:00 2001 From: Michael Sun Date: Tue, 8 Nov 2022 10:05:01 -0800 Subject: [PATCH 2/5] feat: fix the test failures --- src/prelogin-payload.ts | 1 - test/unit/prelogin-payload-test.js | 12 ++++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/prelogin-payload.ts b/src/prelogin-payload.ts index bbd5aa66f..87ce772fa 100644 --- a/src/prelogin-payload.ts +++ b/src/prelogin-payload.ts @@ -234,7 +234,6 @@ class PreloginPayload { if (dataLength === 0) { return; } - this.instanceName = this.data.toString('utf8', offset, offset + dataLength); } diff --git a/test/unit/prelogin-payload-test.js b/test/unit/prelogin-payload-test.js index 6146ab061..482626f83 100644 --- a/test/unit/prelogin-payload-test.js +++ b/test/unit/prelogin-payload-test.js @@ -1,14 +1,14 @@ const PreloginPayload = require('../../src/prelogin-payload'); const assert = require('chai').assert; -function assertPayload(payload, encryptionString, { major, minor, build, subbuild }) { +function assertPayload(payload, encryptionString, { major, minor, build, subbuild }, instanceName) { assert.strictEqual(payload.version.major, major); assert.strictEqual(payload.version.minor, minor); assert.strictEqual(payload.version.build, build); assert.strictEqual(payload.version.subbuild, subbuild); assert.strictEqual(payload.encryptionString, encryptionString); - assert.strictEqual(payload.instanceName, ''); + assert.strictEqual(payload.instanceName, instanceName); assert.strictEqual(payload.threadId, 0); assert.strictEqual(payload.marsString, 'OFF'); assert.strictEqual(payload.fedAuthRequired, 1); @@ -17,22 +17,22 @@ function assertPayload(payload, encryptionString, { major, minor, build, subbuil describe('prelogin-payload-assert', function() { it('should not encrypt', function() { const payload = new PreloginPayload(); - assertPayload(payload, 'NOT_SUP', { major: 0, minor: 0, build: 0, subbuild: 0 }); + assertPayload(payload, 'NOT_SUP', { major: 0, minor: 0, build: 0, subbuild: 0 }, '\u0000'); }); it('should encrypt', function() { const payload = new PreloginPayload({ encrypt: true, version: { major: 11, minor: 3, build: 2, subbuild: 0 } }); - assertPayload(payload, 'ON', { major: 11, minor: 3, build: 2, subbuild: 0 }); + assertPayload(payload, 'ON', { major: 11, minor: 3, build: 2, subbuild: 0 }, '\u0000'); }); it('should accept an instance name', function() { const payload = new PreloginPayload({ encrypt: true, version: { major: 11, minor: 3, build: 2, subbuild: 0 }, instanceName: 'MSSQLServer' }); - assert.strictEqual(payload.instanceName, 'MSSQLServer'); + assert.strictEqual(payload.instanceName, 'MSSQLServer\u0000'); }); it('should create from buffer', function() { const payload = new PreloginPayload(); new PreloginPayload(payload.data); - assertPayload(payload, 'NOT_SUP', { major: 0, minor: 0, build: 0, subbuild: 0 }); + assertPayload(payload, 'NOT_SUP', { major: 0, minor: 0, build: 0, subbuild: 0 }, '\u0000'); }); }); From eb9d1f93c68b6be655a170161fb0bab38f157ca0 Mon Sep 17 00:00:00 2001 From: Michael Sun Date: Wed, 9 Nov 2022 15:07:12 -0800 Subject: [PATCH 3/5] feat: fix some tests and added comments --- src/connection.ts | 12 +++++++++++- src/instance-lookup.ts | 5 +++++ src/sender.ts | 3 ++- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/connection.ts b/src/connection.ts index 4664c3f2f..6fb4a3e05 100644 --- a/src/connection.ts +++ b/src/connection.ts @@ -1254,7 +1254,7 @@ class Connection extends EventEmitter { maxRetriesOnTransientErrors: 3, multiSubnetFailover: false, packetSize: DEFAULT_PACKET_SIZE, - port: DEFAULT_PORT, + port: undefined, readOnlyIntent: false, requestTimeout: DEFAULT_CLIENT_REQUEST_TIMEOUT, rowCollectionOnDone: false, @@ -1549,6 +1549,8 @@ class Connection extends EventEmitter { } this.config.options.port = config.options.port; + } else if (!this.config.options.instanceName) { + this.config.options.port = DEFAULT_PORT; } if (config.options.readOnlyIntent !== undefined) { @@ -1878,15 +1880,23 @@ class Connection extends EventEmitter { initialiseConnection() { const signal = this.createConnectTimer(); + // If user provided both port and an instance name, + // the code should always use the port and skip the instance lookup if (this.config.options.port) { return this.connectOnPort(this.config.options.port, this.config.options.multiSubnetFailover, signal); } else { + // The instance lookup communicates with the server and gets all the + // available instance name and their corresponding ports. + // Then based on the provided instance name and the result ports, + // logic will try to find a match and connection to the instance by its port. return instanceLookup({ server: this.config.server, instanceName: this.config.options.instanceName!, timeout: this.config.options.connectTimeout, signal: signal }).then((port) => { + // If we get a port from the instance lookup process, the logic will try to connect + // using this port. process.nextTick(() => { this.connectOnPort(port, this.config.options.multiSubnetFailover, signal); }); diff --git a/src/instance-lookup.ts b/src/instance-lookup.ts index 1d6645644..e1076343a 100644 --- a/src/instance-lookup.ts +++ b/src/instance-lookup.ts @@ -57,6 +57,8 @@ export async function instanceLookup(options: { server: string, instanceName: st try { response = await withTimeout(timeout, async (signal) => { const request = Buffer.from([0x02]); + // This will send message to request a response that containing + // all instances available on the host return await sendMessage(options.server, port, lookup, signal, request); }, signal); } catch (err) { @@ -74,6 +76,9 @@ export async function instanceLookup(options: { server: string, instanceName: st } const message = response.toString('ascii', MYSTERY_HEADER_LENGTH); + // This function will try to parse out all the port information from the response + // Then compare the instance name while parsing the response, and if there is an + // instance name match, then return that port. const foundPort = parseBrowserResponse(message, instanceName); if (!foundPort) { diff --git a/src/sender.ts b/src/sender.ts index a33e2d741..58ac3f190 100644 --- a/src/sender.ts +++ b/src/sender.ts @@ -90,6 +90,7 @@ export async function sendMessage(host: string, port: number, lookup: LookupFunc }); }); } - + // This sends one or multiple IP addresses in parallel (for clustered SQL server setups) + // and it returns the first response it gets back from the SQL server browser agent return await sendInParallel(addresses, port, request, signal); } From 1b00bcce5494db9687ec6fc575c70fe0333f2d28 Mon Sep 17 00:00:00 2001 From: Michael Sun Date: Wed, 16 Nov 2022 09:55:02 -0800 Subject: [PATCH 4/5] feat: adding the additional tests --- test/unit/rerouting-test.js | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/test/unit/rerouting-test.js b/test/unit/rerouting-test.js index a793aa4be..b89fc0218 100644 --- a/test/unit/rerouting-test.js +++ b/test/unit/rerouting-test.js @@ -8,6 +8,8 @@ const Debug = require('../../src/debug'); const PreloginPayload = require('../../src/prelogin-payload'); const Message = require('../../src/message'); const WritableTrackingBuffer = require('../../src/tracking-buffer/writable-tracking-buffer'); +const StreamParser = require('../../src/token/stream-parser'); +const Login7TokenHandler = require('../../src/token/handler').Login7TokenHandler; function buildRoutingEnvChangeToken(hostname, port) { const valueBuffer = new WritableTrackingBuffer(0); @@ -251,10 +253,12 @@ describe('Connecting to a server that sends a re-routing information', function( const { value: message } = await messageIterator.next(); assert.strictEqual(message.type, 0x12); - const chunks = []; + let messageBuffer = Buffer.alloc(0); for await (const data of message) { - chunks.push(data); + messageBuffer = Buffer.concat([messageBuffer, data]); } + const preloginPayload = new PreloginPayload(messageBuffer); + assert.strictEqual(preloginPayload.instanceName, 'instanceNameA\u0000'); const responsePayload = new PreloginPayload({ encrypt: false, version: { major: 0, minor: 0, build: 0, subbuild: 0 } }); const responseMessage = new Message({ type: 0x12 }); @@ -367,7 +371,8 @@ describe('Connecting to a server that sends a re-routing information', function( server: routingServer.address().address, options: { port: routingServer.address().port, - encrypt: false + encrypt: false, + instanceName: 'instanceNameA' } }); @@ -381,4 +386,15 @@ describe('Connecting to a server that sends a re-routing information', function( connection.close(); } }); + + it('Test if routing data capture the correct instance name value', async function() { + const responseMessage = new Message({ type: 0x04 }); + responseMessage.end(buildRoutingEnvChangeToken(targetServer.address().address + '\\instanceNameA', targetServer.address().port)); + const parser = StreamParser.parseTokens(responseMessage, {}, {}); + const result = await parser.next(); + const handler = new Login7TokenHandler(new Connection({ server: 'servername' })); + handler[result.value.handlerName](result.value); + assert.strictEqual(handler.routingData.instanceName, 'instanceNameA'); + assert.isTrue((await parser.next()).done); + }); }); From 423c0a6f1fa98ad7e6e1408b78e964621933dd20 Mon Sep 17 00:00:00 2001 From: Michael Sun Date: Thu, 17 Nov 2022 09:42:44 -0800 Subject: [PATCH 5/5] feat: changes based on review comments --- test/unit/rerouting-test.js | 20 +++---------- .../token/env-change-token-parser-test.js | 28 +++++++++++++++++++ 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/test/unit/rerouting-test.js b/test/unit/rerouting-test.js index b89fc0218..cb4865822 100644 --- a/test/unit/rerouting-test.js +++ b/test/unit/rerouting-test.js @@ -8,8 +8,6 @@ const Debug = require('../../src/debug'); const PreloginPayload = require('../../src/prelogin-payload'); const Message = require('../../src/message'); const WritableTrackingBuffer = require('../../src/tracking-buffer/writable-tracking-buffer'); -const StreamParser = require('../../src/token/stream-parser'); -const Login7TokenHandler = require('../../src/token/handler').Login7TokenHandler; function buildRoutingEnvChangeToken(hostname, port) { const valueBuffer = new WritableTrackingBuffer(0); @@ -253,11 +251,12 @@ describe('Connecting to a server that sends a re-routing information', function( const { value: message } = await messageIterator.next(); assert.strictEqual(message.type, 0x12); - let messageBuffer = Buffer.alloc(0); + const chunks = []; for await (const data of message) { - messageBuffer = Buffer.concat([messageBuffer, data]); + chunks.push(data); } - const preloginPayload = new PreloginPayload(messageBuffer); + + const preloginPayload = new PreloginPayload(Buffer.concat(chunks)); assert.strictEqual(preloginPayload.instanceName, 'instanceNameA\u0000'); const responsePayload = new PreloginPayload({ encrypt: false, version: { major: 0, minor: 0, build: 0, subbuild: 0 } }); @@ -386,15 +385,4 @@ describe('Connecting to a server that sends a re-routing information', function( connection.close(); } }); - - it('Test if routing data capture the correct instance name value', async function() { - const responseMessage = new Message({ type: 0x04 }); - responseMessage.end(buildRoutingEnvChangeToken(targetServer.address().address + '\\instanceNameA', targetServer.address().port)); - const parser = StreamParser.parseTokens(responseMessage, {}, {}); - const result = await parser.next(); - const handler = new Login7TokenHandler(new Connection({ server: 'servername' })); - handler[result.value.handlerName](result.value); - assert.strictEqual(handler.routingData.instanceName, 'instanceNameA'); - assert.isTrue((await parser.next()).done); - }); }); diff --git a/test/unit/token/env-change-token-parser-test.js b/test/unit/token/env-change-token-parser-test.js index 43919e727..e96a20b0c 100644 --- a/test/unit/token/env-change-token-parser-test.js +++ b/test/unit/token/env-change-token-parser-test.js @@ -1,5 +1,8 @@ const StreamParser = require('../../../src/token/stream-parser'); const WritableTrackingBuffer = require('../../../src/tracking-buffer/writable-tracking-buffer'); +const { Connection } = require('../../../src/tedious'); +const Message = require('../../../src/message'); +const Login7TokenHandler = require('../../../src/token/handler').Login7TokenHandler; const assert = require('chai').assert; describe('Env Change Token Parser', () => { @@ -66,4 +69,29 @@ describe('Env Change Token Parser', () => { const result = await parser.next(); assert.isTrue(result.done); }); + + it('Test if routing data capture the correct instance name value', async function() { + const valueBuffer = new WritableTrackingBuffer(0); + valueBuffer.writeUInt8(0); // Protocol + valueBuffer.writeUInt16LE(1433); // Port + valueBuffer.writeUsVarchar('127.0.0.1\\instanceNameA', 'ucs-2'); + + const envValueDataBuffer = new WritableTrackingBuffer(0); + envValueDataBuffer.writeUInt8(20); // Type + envValueDataBuffer.writeUsVarbyte(valueBuffer.data); + envValueDataBuffer.writeUsVarbyte(Buffer.alloc(0)); + + const envChangeBuffer = new WritableTrackingBuffer(0); + envChangeBuffer.writeUInt8(0xE3); // TokenType + envChangeBuffer.writeUsVarbyte(envValueDataBuffer.data); // Length + EnvValueData + + const responseMessage = new Message({ type: 0x04 }); + responseMessage.end(envChangeBuffer.data); + const parser = StreamParser.parseTokens(responseMessage, {}, {}); + const result = await parser.next(); + const handler = new Login7TokenHandler(new Connection({ server: 'servername' })); + handler[result.value.handlerName](result.value); + assert.strictEqual(handler.routingData.instanceName, 'instanceNameA'); + assert.isTrue((await parser.next()).done); + }); });