Skip to content
This repository has been archived by the owner on Feb 4, 2022. It is now read-only.

Commit

Permalink
feat(connection): attempt both ipv6 and ipv4 when no family entered (#…
Browse files Browse the repository at this point in the history 
…260)

If the user does not specify an IP family, we will attempt to
connect first via IPv6, and fall back to IPv4 if that fails

Fixes NODE-1222
  • Loading branch information
@daprahamian
daprahamian authored Dec 19, 2017
1 parent 867b080 commit 107bae5
Show file tree
Hide file tree
Showing 2 changed files with 236 additions and 78 deletions.
187 changes: 109 additions & 78 deletions lib/connection/connection.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -528,100 +528,131 @@ function merge(options1, options2) {
}
}

/**
* Connect
* @method
*/
Connection.prototype.connect = function(_options) {
var self = this;
_options = _options || {};
// Set the connections
if (connectionAccounting) addConnection(this.id, this);
// Check if we are overriding the promoteLongs
if (typeof _options.promoteLongs === 'boolean') {
self.responseOptions.promoteLongs = _options.promoteLongs;
self.responseOptions.promoteValues = _options.promoteValues;
self.responseOptions.promoteBuffers = _options.promoteBuffers;
function makeSSLConnection(self, _options) {
let sslOptions = {
socket: self.connection,
rejectUnauthorized: self.rejectUnauthorized
};

// Merge in options
merge(sslOptions, this.options);
merge(sslOptions, _options);

// Set options for ssl
if (self.ca) sslOptions.ca = self.ca;
if (self.crl) sslOptions.crl = self.crl;
if (self.cert) sslOptions.cert = self.cert;
if (self.key) sslOptions.key = self.key;
if (self.passphrase) sslOptions.passphrase = self.passphrase;

// Override checkServerIdentity behavior
if (self.checkServerIdentity === false) {
// Skip the identiy check by retuning undefined as per node documents
// https://nodejs.org/api/tls.html#tls_tls_connect_options_callback
sslOptions.checkServerIdentity = function() {
return undefined;
};
} else if (typeof self.checkServerIdentity === 'function') {
sslOptions.checkServerIdentity = self.checkServerIdentity;
}

// Set default sni servername to be the same as host
if (sslOptions.servername == null) {
sslOptions.servername = self.host;
}

// Attempt SSL connection
const connection = tls.connect(self.port, self.host, sslOptions, function() {
// Error on auth or skip
if (connection.authorizationError && self.rejectUnauthorized) {
return self.emit('error', connection.authorizationError, self, { ssl: true });
}

// Set socket timeout instead of connection timeout
connection.setTimeout(self.socketTimeout);
// We are done emit connect
self.emit('connect', self);
});
connection.setTimeout(self.connectionTimeout);

return connection;
}

function makeUnsecureConnection(self, family) {
// Create new connection instance
var connection_options;
let connection_options;
if (self.domainSocket) {
connection_options = { path: self.host };
} else {
connection_options = { port: self.port, host: self.host };
if (self.family !== void 0) {
connection_options.family = self.family;
}
connection_options.family = family;
}
self.connection = net.createConnection(connection_options);

// Set the options for the connection
self.connection.setKeepAlive(self.keepAlive, self.keepAliveInitialDelay);
self.connection.setTimeout(self.connectionTimeout);
self.connection.setNoDelay(self.noDelay);

// If we have ssl enabled
if (self.ssl) {
var sslOptions = {
socket: self.connection,
rejectUnauthorized: self.rejectUnauthorized
};
const connection = net.createConnection(connection_options);

// Merge in options
merge(sslOptions, this.options);
merge(sslOptions, _options);

// Set options for ssl
if (self.ca) sslOptions.ca = self.ca;
if (self.crl) sslOptions.crl = self.crl;
if (self.cert) sslOptions.cert = self.cert;
if (self.key) sslOptions.key = self.key;
if (self.passphrase) sslOptions.passphrase = self.passphrase;

// Override checkServerIdentity behavior
if (self.checkServerIdentity === false) {
// Skip the identiy check by retuning undefined as per node documents
// https://nodejs.org/api/tls.html#tls_tls_connect_options_callback
sslOptions.checkServerIdentity = function() {
return undefined;
};
} else if (typeof self.checkServerIdentity === 'function') {
sslOptions.checkServerIdentity = self.checkServerIdentity;
}

// Set default sni servername to be the same as host
if (sslOptions.servername == null) {
sslOptions.servername = self.host;
}
// Set the options for the connection
connection.setKeepAlive(self.keepAlive, self.keepAliveInitialDelay);
connection.setTimeout(self.connectionTimeout);
connection.setNoDelay(self.noDelay);

connection.once('connect', function() {
// Set socket timeout instead of connection timeout
connection.setTimeout(self.socketTimeout);
// Emit connect event
self.emit('connect', self);
});

return connection;
}

// Attempt SSL connection
self.connection = tls.connect(self.port, self.host, sslOptions, function() {
// Error on auth or skip
if (self.connection.authorizationError && self.rejectUnauthorized) {
return self.emit('error', self.connection.authorizationError, self, { ssl: true });
}

// Set socket timeout instead of connection timeout
self.connection.setTimeout(self.socketTimeout);
// We are done emit connect
self.emit('connect', self);
});
self.connection.setTimeout(self.connectionTimeout);
} else {
self.connection.once('connect', function() {
// Set socket timeout instead of connection timeout
self.connection.setTimeout(self.socketTimeout);
// Emit connect event
self.emit('connect', self);
});
}
function doConnect(self, family, _options, _errorHandler) {
self.connection = self.ssl
? makeSSLConnection(self, _options)
: makeUnsecureConnection(self, family);

// Add handlers for events
self.connection.once('error', errorHandler(self));
self.connection.once('error', _errorHandler);
self.connection.once('timeout', timeoutHandler(self));
self.connection.once('close', closeHandler(self));
self.connection.on('data', dataHandler(self));
}

/**
* Connect
* @method
*/
Connection.prototype.connect = function(_options) {
_options = _options || {};
// Set the connections
if (connectionAccounting) addConnection(this.id, this);
// Check if we are overriding the promoteLongs
if (typeof _options.promoteLongs === 'boolean') {
this.responseOptions.promoteLongs = _options.promoteLongs;
this.responseOptions.promoteValues = _options.promoteValues;
this.responseOptions.promoteBuffers = _options.promoteBuffers;
}

const _errorHandler = errorHandler(this);

if (this.family !== void 0) {
return doConnect(this, this.family, _options, _errorHandler);
}

return doConnect(this, 6, _options, err => {
if (this.logger.isDebug()) {
this.logger.debug(
f(
'connection %s for [%s:%s] errored out with [%s]',
this.id,
this.host,
this.port,
JSON.stringify(err)
)
);
}

return doConnect(this, 4, _options, _errorHandler);
});
};

/**
Expand Down
127 changes: 127 additions & 0 deletions test/tests/functional/connection_tests.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
'use strict';

const bson = require('bson');
const expect = require('chai').expect;
const mock = require('../../mock');
const Connection = require('../../../lib/connection/connection');

describe('Connection', function() {
const noop = () => {};
let server;
afterEach(() => mock.cleanup());

function testCase(name, options) {
const config = options.config;
const args = {
metadata: { requires: { topology: ['single'] } },
test: function(done) {
const connection = new Connection(
noop,
Object.assign(
{
bson,
port: server.port
},
config
)
);

const cleanup = err => {
connection.destroy();
done(err);
};

const errorHandler = options.error
? err => {
try {
options.error(err);
cleanup();
} catch (e) {
cleanup(e);
}
}
: cleanup;

const connectHandler = options.connect
? () => {
try {
options.connect(connection);
cleanup();
} catch (e) {
cleanup(e);
}
}
: () => {
cleanup(new Error('Expected test to not connect, but it connected successfully'));
};

connection.on('error', errorHandler);
connection.on('connect', connectHandler);
connection.connect();
}
};

if (options.skip) {
it.skip(name, args);
} else if (options.only) {
it.only(name, args);
} else {
it(name, args);
}
}

describe('IPv4', function() {
beforeEach(() => mock.createServer(0, '127.0.0.1').then(_server => (server = _server)));

testCase('should connect with no family', {
config: { host: 'localhost' },
connect: connection => {
expect(connection.connection.remotePort).to.equal(server.port);
expect(connection.connection.remoteFamily).to.equal('IPv4');
}
});

testCase('should connect with family=4', {
config: { host: 'localhost', family: 4 },
connect: connection => {
expect(connection.connection.remotePort).to.equal(server.port);
expect(connection.connection.remoteFamily).to.equal('IPv4');
}
});

testCase('should error with family=6', {
config: { host: 'localhost', family: 6 },
error: err => expect(err).to.be.an.instanceOf(Error)
});
});

describe('IPv6', function() {
beforeEach(() => mock.createServer(0, '::').then(_server => (server = _server)));

testCase('should connect with no family', {
config: { host: 'localhost' },
connect: connection => {
expect(connection.connection.remotePort).to.equal(server.port);
expect(connection.connection.remoteFamily).to.equal('IPv6');
}
});

// NOTE: this test is currently being skipped b/c of a "feature" in
// most operating systems where listening on an IPv6 port
// also listens on an IPv4 port. Don't want to spend time working around
// this. See https://github.com/nodejs/node/issues/9390 for more info.
testCase('should error with family=4', {
skip: true,
config: { host: 'localhost', family: 4 },
error: err => expect(err).to.be.an.instanceOf(Error)
});

testCase('should connect with family=6', {
config: { host: 'localhost', family: 6 },
connect: connection => {
expect(connection.connection.remotePort).to.equal(server.port);
expect(connection.connection.remoteFamily).to.equal('IPv6');
}
});
});
});

3 comments on commit 107bae5

@vkarpov15
Copy link
Member

Choose a reason for hiding this comment

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

I traced down Automattic/mongoose#6566 to this change. While I can understand the rationale for this change, perhaps we can try ipv4 and v6 in parallel rather than doing family: 6 first and falling back to family: 4 if that fails?

@mbroadst
Copy link
Member

@mbroadst mbroadst commented on 107bae5 Jul 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@vkarpov15 agreed, I think we should achieve this by implementing happy eyeballs rather than our current approach. I made NODE-1580 to track this.

@vkarpov15
Copy link
Member

Choose a reason for hiding this comment

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

Another possibility would be allowing specifying family in the connection string. It is non-standard, but would make it easier to switch between ipv4 for local vs ipv6 for prod or vice versa.

Please sign in to comment.