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.

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

nat support for sentinel connector #799

Merged
merged 5 commits into from
Mar 12, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
node_modules
*.cpuprofile
/test.js
/.idea
built

.vscode
22 changes: 20 additions & 2 deletions lib/connectors/SentinelConnector/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {createConnection, Socket} from 'net'
import {NatMap} from '../../cluster/ClusterOptions';
import {CONNECTION_CLOSED_ERROR_MSG, packObject, sample} from '../../utils'
import {connect as createTLSConnection, TLSSocket, SecureContextOptions} from 'tls'
import {ITcpConnectionOptions, isIIpcConnectionOptions} from '../StandaloneConnector'
Expand Down Expand Up @@ -30,6 +31,7 @@ interface ISentinelConnectionOptions extends ITcpConnectionOptions {
connectTimeout?: number
enableTLSForSentinelMode?: boolean
sentinelTLS?: SecureContextOptions
natMap: NatMap
}

export default class SentinelConnector extends AbstractConnector {
Expand All @@ -46,6 +48,10 @@ export default class SentinelConnector extends AbstractConnector {
throw new Error('Requires the name of master.')
}

if (this.options.natMap && Object.keys(this.options.natMap).length === 0) {
throw new Error('Empty natMap is not allowed.')
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should allow empty natMap ({}) to keep the same behavior as one in cluster: https://github.com/luin/ioredis/blob/master/lib/cluster/index.ts#L428.

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 for reply. Fixed.

}

this.sentinelIterator = new SentinelIterator(this.options.sentinels)
}

Expand Down Expand Up @@ -168,7 +174,10 @@ export default class SentinelConnector extends AbstractConnector {
if (err) {
return callback(err)
}
callback(null, Array.isArray(result) ? { host: result[0], port: Number(result[1]) } : null)

callback(null, this.sentinelNatResolve(
Array.isArray(result) ? { host: result[0], port: Number(result[1]) } : null
))
})
})
}
Expand All @@ -188,10 +197,19 @@ export default class SentinelConnector extends AbstractConnector {
slave.flags && !slave.flags.match(/(disconnected|s_down|o_down)/)
))

callback(null, selectPreferredSentinel(availableSlaves, this.options.preferredSlaves))
callback(null, this.sentinelNatResolve(
selectPreferredSentinel(availableSlaves, this.options.preferredSlaves)
))
})
}

sentinelNatResolve (item: ISentinelAddress) {
if (!item || !this.options.natMap)
return item;

return this.options.natMap[`${item.host}:${item.port}`] || item
}

private resolve (endpoint, callback: NodeCallback<ITcpConnectionOptions>): void {
if (typeof Redis === 'undefined') {
Redis = require('../../redis')
Expand Down
2 changes: 2 additions & 0 deletions lib/redis.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ var PromiseContainer = require('./promiseContainer');
* strings. This option is necessary when dealing with big numbers (exceed the [-2^53, +2^53] range).
* @param {boolean} [options.enableTLSForSentinelMode=false] - Whether to support the `tls` option
* when connecting to Redis via sentinel mode.
* @param {NatMap} [options.natMap=null] NAT map for sentinel connector.
* @extends [EventEmitter](http://nodejs.org/api/events.html#events_class_events_eventemitter)
* @extends Commander
* @example
Expand Down Expand Up @@ -172,6 +173,7 @@ Redis.defaultOptions = {
sentinelRetryStrategy: function (times) {
return Math.min(times * 10, 1000);
},
natMap: null,
enableTLSForSentinelMode: false,
// Status
password: null,
Expand Down
90 changes: 90 additions & 0 deletions test/functional/sentinel_nat.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
describe('sentinel_nat', function() {
it('connects to server as expected', function(done) {

var sentinel = new MockServer(27379, function (argv) {
if (argv[0] === 'sentinel' && argv[1] === 'get-master-addr-by-name') {
return ['127.0.0.1', '17380'];
}
})

var redis = new Redis({
sentinels: [
{ host: '127.0.0.1', port: '27379' }
],
natMap: {
'127.0.0.1:17380': {
host: 'localhost',
port: 6379,
}
},
name: 'master',
lazyConnect: true,
})

redis.connect(function(err) {
if (err) {
sentinel.disconnect(function() {})
return done(err)
}
sentinel.disconnect(done)
})
})

it('rejects connection if host is not defined in map', function(done) {
var sentinel = new MockServer(27379, function (argv) {
if (argv[0] === 'sentinel' && argv[1] === 'get-master-addr-by-name') {
return ['127.0.0.1', '17380']
}

if (argv[0] === 'sentinel' && argv[1] === 'sentinels' &&argv[2] === 'master') {
return ['127.0.0.1', '27379']
}
})

var redis = new Redis({
sentinels: [
{ host: '127.0.0.1', port: '27379' }
],
natMap: {
'127.0.0.1:17381': {
host: 'localhost',
port: 6379,
}
},
maxRetriesPerRequest: 1,
name: 'master',
lazyConnect: true,
})

redis
.connect()
.then(function() {
throw new Error("Should not call")
})
.catch(function(err) {
if (err.message === 'Connection is closed.') {
return done(null)
}
sentinel.disconnect(done)
})
})

it('throws \'Empty natMap is not allowed.\' when empty natMap was given', function(done) {
try {
new Redis({
sentinels: [
{ host: '127.0.0.1', port: '27379' }
],
natMap: {},
name: 'master',
})
} catch (error) {
if (error.message === 'Empty natMap is not allowed.') {
done(null)
} else {
done(new Error('Should not call'))
}
}
})

})