PUT Whitelist and IPv6 toggle #48
Conversation
…d, and some minor fixups
Useful when dealing with whitelist
stephen-palmer
left a comment
stephen-palmer
left a comment
There was a problem hiding this comment.
Nice additions .. just some minor changes requested.
README.md
Outdated
| `-w`, `--workers <n>` | The number of worker threads to spawn. The default is 0. | ||
| `-m`, `--mirror [host:port]` | Mirror transactions to another cache server. Repeat this option ofr multiple mirrors. | ||
| `-m`, `--monitor-parent-process <n>` | Monitor a parent process and exit if it dies. | ||
| `--allow-ipv6` | Allow IPv6 connections if available. |
There was a problem hiding this comment.
I would prefer if this were in the config file - we need to move toward less discrepancy between CLI options and config file options.
To do so, I think a new top level section called "Server" would be appropriate:
Server
options:
allowIpv6: true
lib/server/server.js
Outdated
| options.mirror = [].concat(options.mirror); | ||
| this._mirrors = options.mirror.map(m => new TransactionMirror(m, cache)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Convert tabs to spaces (there are a few other places as well)
lib/server/command_processor.js
Outdated
| * @private | ||
| */ | ||
| _isWhitelisted(ip) { | ||
| if(this._whitelistEmpty){ |
There was a problem hiding this comment.
can be simplified:
return this._whitelistEmpty || this._putWhitelist.indexOf(ip) != -1
There was a problem hiding this comment.
It can be simplified further and maybe a little easier to understand if you use includes over an indexOf check:
return this._whitelistEmpty || this._putWhitelist.includes(ip);
lib/server/server.js
Outdated
| if(this.allowIpv6){ | ||
| this._server.listen(this.port, () => resolve()); | ||
| } | ||
| else{ |
There was a problem hiding this comment.
leave a space before open braces
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
lib/server/server.js
Outdated
| return new Promise(resolve => { | ||
| this._server.listen(this.port, () => resolve()); | ||
| if(this.allowIpv6){ | ||
| this._server.listen(this.port, () => resolve()); |
There was a problem hiding this comment.
If you are ignoring the resolution value of this promise, you can save an extra function call here and on line 96 by passing resolve by reference:
this._server.listen(this.port, resolve);this._server.listen(this.port, "0.0.0.0", resolve);
|
Looks like Travis doesn't support IPv6, bummer. I don't think there are any more tests I can reasonably add. Going to leave this as is unless you have any requests. |
3 participants
Added a PUT whitelist for configuring which machines are allowed to write to the cache server: #37
Also added the ability to toggle IPv6 on/off (defaulting to off). This is to avoid confusion when dealing with hybrid Ipv4-Ipv6 sockets as they convert IPv4 addresses to Ipv4-mapped Ipv6 addresses.
Relevant net library docs: https://nodejs.org/dist/latest-v4.x/docs/api/net.html#net_server_listen_port_hostname_backlog_callback