diff --git a/.eslintrc.js b/.eslintrc.js index 3d8565a..7162332 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -19,6 +19,11 @@ module.exports = { 'no-case-declarations': 0, 'no-inner-declarations': 0, + //`new require('x')` parses as `new require` followed by a plain + //`.x()` call, and throws under module systems whose `require` is not + //constructible (e.g. Jest). + 'no-new-require': 'error', + //global rules 'indent': [ 'error', diff --git a/lib/addMethod/addMethodREST.js b/lib/addMethod/addMethodREST.js index 9b7d13e..cf0aca8 100644 --- a/lib/addMethod/addMethodREST.js +++ b/lib/addMethod/addMethodREST.js @@ -14,6 +14,7 @@ var validateNotExpects = require('./validateNotExpects'); var formatResponse = require('./formatResponse'); var addMethodFunction = require('./addMethodFunction'); +var resolveKeepAliveAgent = require('./keepAliveAgent'); module.exports = function (methodName, config, afterHeadersFunction) { var threadneedle = this; @@ -29,15 +30,11 @@ module.exports = function (methodName, config, afterHeadersFunction) { return; } - // TCP keep-alive shorter than 350 sec to prevent AWS NAT idle timeout: https://repost.aws/knowledge-center/lambda-vpc-timeout - const keepAliveAgent = new require('http').Agent({ keepAlive: true, keepAliveMsecs: 349000 }); - // All methods should default to no timeout, maximising the chance // of success. needle.defaults({ open_timeout: 0, - read_timeout: 0, - agent: keepAliveAgent + read_timeout: 0 }); // Validate the input. Errors will be `throw`n if there's anything @@ -222,6 +219,13 @@ module.exports = function (methodName, config, afterHeadersFunction) { // console.log(options); logger.info(methodName + ': running ' + method + ' request', request); + // Attach the keep-alive agent, unless the caller supplied their own. + request.options = request.options || {}; + request.options.agent = resolveKeepAliveAgent( + request.options, + globalize.disableKeepAliveAgent.call(threadneedle, config) + ); + // Run a different method for get to not include data switch (method) { case 'get': diff --git a/lib/addMethod/globalize/disableKeepAliveAgent.js b/lib/addMethod/globalize/disableKeepAliveAgent.js new file mode 100644 index 0000000..3056d64 --- /dev/null +++ b/lib/addMethod/globalize/disableKeepAliveAgent.js @@ -0,0 +1,34 @@ +/* +* Whether to skip the keep-alive agent. On by default; a method or a connector's +* globals can set `disableKeepAliveAgent: true`, and a method can set it back to +* `false` where its connector disabled it wholesale. +* +* A blanket `globals: false` does not discard this, unlike other globals. The +* flag gets set because keep-alive breaks a service, and `globals: false` is +* common on auth endpoints for unrelated reasons - silently re-enabling the +* agent there would defeat the point. Naming the key does discard it, since +* that is a deliberate statement about this flag rather than collateral. +* +* Hence checking `globals` directly rather than through `localOnly`, which +* cannot tell the blanket opt-out from the per-key one. +*/ +const _ = require('lodash'); + +module.exports = function (config) { + + if (_.isBoolean(config.disableKeepAliveAgent)) { + return config.disableKeepAliveAgent; + } + + //`globals: { disableKeepAliveAgent: false }` - drop the global, leaving the default + if (_.get(config, 'globals.disableKeepAliveAgent') === false) { + return false; + } + + if (_.isBoolean(this._globalOptions.disableKeepAliveAgent)) { + return this._globalOptions.disableKeepAliveAgent; + } + + return false; + +}; diff --git a/lib/addMethod/globalize/index.js b/lib/addMethod/globalize/index.js index fd1e37a..496d898 100644 --- a/lib/addMethod/globalize/index.js +++ b/lib/addMethod/globalize/index.js @@ -9,5 +9,6 @@ module.exports = { notExpects: require('./notExpects'), afterSuccess: require('./afterSuccess'), afterHeaders: require('./afterHeaders'), - afterFailure: require('./afterFailure') + afterFailure: require('./afterFailure'), + disableKeepAliveAgent: require('./disableKeepAliveAgent') }; diff --git a/lib/addMethod/keepAliveAgent.js b/lib/addMethod/keepAliveAgent.js new file mode 100644 index 0000000..4cc54ce --- /dev/null +++ b/lib/addMethod/keepAliveAgent.js @@ -0,0 +1,94 @@ +/* +* TCP keep-alive, so a call still waiting on a slow API is not dropped by the +* AWS NAT gateway's 350 second idle timeout. +* See https://repost.aws/knowledge-center/lambda-vpc-timeout +* +* Not HTTP keep-alive: Node's `keepAlive: true` pools connections between +* requests and only arms the socket once it returns to the pool, after the +* response - too late. So this agent does not pool. It exists only to arm the +* socket as it is created. +* +* Switch off with `disableKeepAliveAgent` - see globalize/disableKeepAliveAgent.js +*/ +const http = require('http'); +const https = require('https'); + +const logger = require('../logger'); + +/* +* Idle time before the first probe, and the gap between probes thereafter, since +* an answered probe resets the timer. Under the gateway's 350 seconds with +* margin, so one probe per idle period is enough. +*/ +const KEEPALIVE_DELAY = 300000; + +/* +* Real agents, used only for their `createConnection`, so TLS setup stays Node's +* job. These are shared, so `maxCachedSessions: 0` keeps them stateless: this +* agent extends http.Agent, so the TLS session key is computed by +* `http.Agent.getName` and omits `rejectUnauthorized`, `ca` and the rest. A +* session opened by a lax request would then be offered to a strict one for the +* same host. Node still verifies the certificate on resumption, so that is not +* a bypass today - but nothing here needs session reuse, and this way it cannot +* become one. +*/ +const factories = { + 'http:': new http.Agent({ keepAlive: false }), + 'https:': new https.Agent({ keepAlive: false, maxCachedSessions: 0 }) +}; + +class KeepAliveAgent extends http.Agent { + + constructor (options) { + super(options); + + /* + Node only rejects a protocol mismatch when the agent declares one, and + this agent serves both. Declaring none is also what keeps redirects + working, since needle reuses the same agent for the redirect target. + */ + this.protocol = undefined; + } + + createConnection (options, callback) { + /* + needle sets this per request: the proxy's protocol when proxying, else + the target's. It dispatches anything but `https:` through http, so the + fallback mirrors that - a TLS socket there would break the request + rather than secure it, since needle would still be speaking http. + */ + const factory = ( factories[options.protocol] || factories['http:'] ); + + const socket = factory.createConnection(options, callback); + socket.setKeepAlive(true, KEEPALIVE_DELAY); //set SO_KEEPALIVE + + return socket; + } + +} + +//A caller's own `agent` is returned untouched, so `agent: false` stays an opt-out +module.exports = function resolveKeepAliveAgent (options, disabled) { + + if (options.agent !== undefined) { + return options.agent; + } + + //`disableKeepAliveAgent`, set on the method or the connector's globals + if (disabled === true) { + logger.info('keep-alive agent disabled by configuration; not attaching one'); + return undefined; + } + + /* + One per request, not shared: needle writes TLS options (`rejectUnauthorized`, + `cert`, `key`, ...) onto `agent.options`, so sharing would leak one method's + TLS settings into later requests. Agents open no sockets, so this is free. + */ + return new KeepAliveAgent({ keepAlive: false }); + +}; + +module.exports.KEEPALIVE_DELAY = KEEPALIVE_DELAY; +module.exports.KeepAliveAgent = KeepAliveAgent; +module.exports.factories = factories; diff --git a/package-lock.json b/package-lock.json index f9e8182..d53c447 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@trayio/threadneedle", - "version": "1.19.0-beta.0", + "version": "1.19.1", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@trayio/threadneedle", - "version": "1.19.0-beta.0", + "version": "1.19.1", "license": "MIT", "dependencies": { "@trayio/needle": "^4.0.0", diff --git a/package.json b/package.json index 6566e36..8e4acbf 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@trayio/threadneedle", - "version": "1.19.0", + "version": "1.19.1", "description": "A framework for simplifying working with HTTP-based APIs.", "main": "lib/index.js", "directories": { diff --git a/tests/addMethodREST_test.js b/tests/addMethodREST_test.js index 122c109..e67f245 100644 --- a/tests/addMethodREST_test.js +++ b/tests/addMethodREST_test.js @@ -109,8 +109,7 @@ describe('#addMethodREST', function () { }); after(function (done) { - server.close(); - done(); + server.close(done); }); var threadneedle; diff --git a/tests/globalize_test.js b/tests/globalize_test.js index 50c3259..90d3708 100644 --- a/tests/globalize_test.js +++ b/tests/globalize_test.js @@ -2067,4 +2067,83 @@ describe('#globalize', function () { }); + describe('#disableKeepAliveAgent', function () { + + it('should be enabled by default', function () { + const sample = { _globalOptions: {} }; + + assert.strictEqual( + globalize.disableKeepAliveAgent.call(sample, {}), + false + ); + }); + + it('should allow a method to disable it', function () { + const sample = { _globalOptions: {} }; + + assert.strictEqual( + globalize.disableKeepAliveAgent.call(sample, { disableKeepAliveAgent: true }), + true + ); + }); + + it('should allow a connector to disable it for every method', function () { + const sample = { _globalOptions: { disableKeepAliveAgent: true } }; + + assert.strictEqual( + globalize.disableKeepAliveAgent.call(sample, {}), + true + ); + }); + + it('should let a method re-enable it where the connector disabled it', function () { + const sample = { _globalOptions: { disableKeepAliveAgent: true } }; + + assert.strictEqual( + globalize.disableKeepAliveAgent.call(sample, { disableKeepAliveAgent: false }), + false + ); + }); + + it('should ignore non-boolean values', function () { + const sample = { _globalOptions: {} }; + + assert.strictEqual( + globalize.disableKeepAliveAgent.call(sample, { disableKeepAliveAgent: 'true' }), + false + ); + }); + + //Unlike other globals - a switch set because keep-alive breaks a service + it('should not be discarded by `globals: false`', function () { + const sample = { _globalOptions: { disableKeepAliveAgent: true } }; + + assert.strictEqual( + globalize.disableKeepAliveAgent.call(sample, { globals: false }), + true + ); + }); + + //Naming the key is deliberate, unlike a blanket `globals: false` + it('should be discarded when the method names it in `globals`', function () { + const sample = { _globalOptions: { disableKeepAliveAgent: true } }; + + assert.strictEqual( + globalize.disableKeepAliveAgent.call(sample, { globals: { disableKeepAliveAgent: false } }), + false + ); + }); + + it('should still apply where a method names other keys in `globals`', function () { + const sample = { _globalOptions: { disableKeepAliveAgent: true } }; + + assert.strictEqual( + globalize.disableKeepAliveAgent.call(sample, { globals: { before: false } }), + true + ); + }); + + }); + + }); diff --git a/tests/keepAliveAgent_test.js b/tests/keepAliveAgent_test.js new file mode 100644 index 0000000..4cc2e26 --- /dev/null +++ b/tests/keepAliveAgent_test.js @@ -0,0 +1,140 @@ +var assert = require('assert'); +var http = require('http'); +var https = require('https'); +var net = require('net'); + +var resolveKeepAliveAgent = require('../lib/addMethod/keepAliveAgent'); + + +describe('#keepAliveAgent', function () { + + describe('Caller overrides', function () { + + it('should return a caller supplied agent untouched', function () { + var mine = new https.Agent({ keepAlive: true }); + assert.strictEqual(resolveKeepAliveAgent({ agent: mine }), mine); + }); + + it('should pass `false` through, so opting out remains possible', function () { + assert.strictEqual(resolveKeepAliveAgent({ agent: false }), false); + }); + + it('should attach an agent when none was supplied', function () { + assert.ok(resolveKeepAliveAgent({}) instanceof http.Agent); + }); + + }); + + describe('Disabling', function () { + + it('should not attach an agent when disabled', function () { + assert.strictEqual(resolveKeepAliveAgent({}, true), undefined); + }); + + it('should attach an agent when explicitly not disabled', function () { + assert.ok(resolveKeepAliveAgent({}, false) instanceof http.Agent); + }); + + it('should attach an agent when nothing was said either way', function () { + assert.ok(resolveKeepAliveAgent({}, undefined) instanceof http.Agent); + }); + + it('should still honour a caller supplied agent when disabled', function () { + var mine = new https.Agent({ keepAlive: true }); + assert.strictEqual(resolveKeepAliveAgent({ agent: mine }, true), mine); + }); + + }); + + describe('Protocol handling', function () { + + /* + Declaring no protocol is what lets one agent serve both, and so what + keeps cross-protocol redirects working. If a Node upgrade changes that + check, this test says so. + */ + it('should declare no protocol, so either is accepted', function () { + assert.strictEqual(resolveKeepAliveAgent({}).protocol, undefined); + }); + + function connectionFor (protocol) { + var agent = resolveKeepAliveAgent({}); + var chosen = null; + + var originals = {}; + Object.keys(resolveKeepAliveAgent.factories).forEach(function (key) { + originals[key] = resolveKeepAliveAgent.factories[key].createConnection; + resolveKeepAliveAgent.factories[key].createConnection = function () { + chosen = key; + return new net.Socket(); + }; + }); + + try { + agent.createConnection({ protocol: protocol, host: 'example.com', port: 443 }, function () {}); + } finally { + Object.keys(originals).forEach(function (key) { + resolveKeepAliveAgent.factories[key].createConnection = originals[key]; + }); + } + + return chosen; + } + + it('should open a tls connection for an https request', function () { + assert.strictEqual(connectionFor('https:'), 'https:'); + }); + + it('should open a plain connection for an http request', function () { + assert.strictEqual(connectionFor('http:'), 'http:'); + }); + + //needle dispatches anything but `https:` through http, so this must match + it('should fall back to a plain connection for any other protocol', function () { + assert.strictEqual(connectionFor(undefined), 'http:'); + }); + + }); + + describe('Agent configuration', function () { + + it('should not pool connections, leaving socket lifetime unchanged', function () { + assert.strictEqual(resolveKeepAliveAgent({}).keepAlive, false); + }); + + it('should arm keep-alive with usable margin inside the NAT idle timeout', function () { + //AWS NAT gateways drop connections that sit idle for 350 seconds + var NAT_IDLE_TIMEOUT = 350000; + var REQUIRED_MARGIN = 30000; + + assert.ok( + resolveKeepAliveAgent.KEEPALIVE_DELAY <= NAT_IDLE_TIMEOUT - REQUIRED_MARGIN, + 'a probe must land early enough to reset the gateway timer, not just before it' + ); + }); + + /* + The shared https factory keys its TLS session cache with + `http.Agent.getName`, which omits TLS options, so a lax session would be + offered to a strict request for the same host. Nothing here needs + session reuse. + */ + it('should not cache TLS sessions across requests', function () { + assert.strictEqual(resolveKeepAliveAgent.factories['https:'].maxCachedSessions, 0); + }); + + it('should return a new agent per request', function () { + assert.notStrictEqual(resolveKeepAliveAgent({}), resolveKeepAliveAgent({})); + }); + + //needle writes TLS options onto `agent.options`, so sharing would leak them + it('should not share TLS options between requests', function () { + var first = resolveKeepAliveAgent({}); + first.options.rejectUnauthorized = false; + + assert.notStrictEqual(resolveKeepAliveAgent({}).options.rejectUnauthorized, false); + }); + + }); + +}); diff --git a/tests/keepAlive_integration_test.js b/tests/keepAlive_integration_test.js new file mode 100644 index 0000000..d7a1e62 --- /dev/null +++ b/tests/keepAlive_integration_test.js @@ -0,0 +1,326 @@ +var assert = require('assert'); +var http = require('http'); +var net = require('net'); + +var { randString } = require('../lib/utils/mout'); +var resolveKeepAliveAgent = require('../lib/addMethod/keepAliveAgent'); +var ThreadNeedle = require('../'); + + +describe('#keepAlive integration', function () { + + var server; + var host; + var closedPort; + + //Records every setKeepAlive call made while these tests run + var armed; + var originalSetKeepAlive; + + before(function (done) { + originalSetKeepAlive = net.Socket.prototype.setKeepAlive; + net.Socket.prototype.setKeepAlive = function (enable, delay) { + armed.push({ enable: enable, delay: delay, at: Date.now() }); + return originalSetKeepAlive.apply(this, arguments); + }; + + server = http.createServer(); + + server.listen(0, function () { + host = 'http://localhost:' + server.address().port; + + /* + A port reserved then released, so requests to it are refused. Lets us + check protocol handling without a TLS server, and so without a key. + */ + var scout = net.createServer(); + scout.listen(0, function () { + closedPort = scout.address().port; + scout.close(done); + }); + }); + }); + + after(function (done) { + net.Socket.prototype.setKeepAlive = originalSetKeepAlive; + server.close(done); + }); + + beforeEach(function () { + armed = []; + }); + + function errorCodeOf (error) { + return ( ( error.body && ( error.body.code || error.body.errno ) ) || error.code ); + } + + describe('Protocol handling', function () { + + /* + The rest of the suite runs against http, which is how an http-only agent + went unnoticed. Node checks the agent's protocol before opening a socket, + so reaching the network at all is what proves this. + */ + it('should reach the network on an https request rather than reject the agent', function (done) { + var name = randString(10); + var threadneedle = new ThreadNeedle(); + + threadneedle.addMethod(name, { + method: 'get', + url: 'https://127.0.0.1:' + closedPort + '/' + name + }); + + threadneedle[name]({}).done(function () { + done(new Error('nothing should be listening on this port')); + }, function (error) { + assert.strictEqual(errorCodeOf(error), 'ECONNREFUSED'); + done(); + }); + }); + + /* + needle reuses the same agent for the redirect target, so an agent fixed + to one protocol threw here - uncaught, taking the process down. Several + connectors follow redirects, and http-client exposes it as a user option. + */ + it('should follow a redirect that switches protocol', function (done) { + var name = randString(10); + var threadneedle = new ThreadNeedle(); + + server.once('request', function (req, res) { + res.writeHead(302, { location: 'https://127.0.0.1:' + closedPort + '/moved' }); + res.end(); + }); + + threadneedle.addMethod(name, { + method: 'get', + url: host + '/' + name, + options: { follow_max: 3 } + }); + + threadneedle[name]({}).done(function () { + done(new Error('nothing should be listening on the redirect target')); + }, function (error) { + //Reaching the target at all is the point - it fails on the refused connection + assert.strictEqual(errorCodeOf(error), 'ECONNREFUSED'); + done(); + }); + }); + + /* + needle connects to the proxy, not the target, so the socket's protocol is + the proxy's. Picking from `options.protocol` handles that for free. + */ + it('should connect over the proxy\'s protocol, not the target\'s', function (done) { + var name = randString(10); + var threadneedle = new ThreadNeedle(); + + //The plain http server above stands in for an http proxy + server.once('request', function (req, res) { + res.writeHead(200, { 'content-type': 'text/plain' }); + res.end('via proxy'); + }); + + threadneedle.addMethod(name, { + method: 'get', + url: 'https://example.com/' + name, + expects: 200, + options: { proxy: host } + }); + + threadneedle[name]({}).done(function (result) { + assert.strictEqual(result.body, 'via proxy'); + assert.strictEqual(armed[0].delay, resolveKeepAliveAgent.KEEPALIVE_DELAY); + done(); + }, done); + }); + + /* + Pins the failure the tests above rely on: if Node stopped rejecting a + mismatched agent, they would quietly lose their teeth. + */ + it('should show that a mismatched agent is what breaks such a request', function (done) { + var name = randString(10); + var threadneedle = new ThreadNeedle(); + + threadneedle.addMethod(name, { + method: 'get', + url: 'https://127.0.0.1:' + closedPort + '/' + name, + //An http agent on an https url, as 1.19.0 installed globally + options: { agent: function () { return new http.Agent({ keepAlive: false }); } } + }); + + threadneedle[name]({}).done(function () { + done(new Error('a mismatched agent should not have connected')); + }, function (error) { + assert.strictEqual(errorCodeOf(error), 'ERR_INVALID_PROTOCOL'); + done(); + }); + }); + + }); + + describe('Arming', function () { + + /* + The point of the fix: Node's own `keepAliveMsecs` is applied only once a + socket returns to the pool, after the response, which is too late. + */ + it('should arm keep-alive before the response arrives', function (done) { + var name = randString(10); + var threadneedle = new ThreadNeedle(); + var armedBeforeServerReplied; + + server.once('request', function (req, res) { + armedBeforeServerReplied = armed.length > 0; + //Hold the request open, as a slow API would + setTimeout(function () { + res.writeHead(200); + res.end('ok'); + }, 150); + }); + + threadneedle.addMethod(name, { + method: 'get', + url: host + '/' + name, + expects: 200 + }); + + threadneedle[name]({}).done(function () { + assert.strictEqual( + armedBeforeServerReplied, true, + 'keep-alive should be armed while the request is still in flight' + ); + assert.strictEqual(armed[0].enable, true); + assert.strictEqual(armed[0].delay, resolveKeepAliveAgent.KEEPALIVE_DELAY); + done(); + }, done); + }); + + }); + + describe('Disabling', function () { + + function respondOk () { + server.once('request', function (req, res) { + res.writeHead(200); + res.end('ok'); + }); + } + + it('should attach no agent when a method disables it', function (done) { + var name = randString(10); + var threadneedle = new ThreadNeedle(); + respondOk(); + + threadneedle.addMethod(name, { + method: 'get', + url: host + '/' + name, + expects: 200, + disableKeepAliveAgent: true + }); + + threadneedle[name]({}).done(function () { + assert.strictEqual(armed.length, 0, 'no keep-alive should have been armed'); + done(); + }, done); + }); + + it('should attach no agent when the connector disables it globally', function (done) { + var name = randString(10); + var threadneedle = new ThreadNeedle(); + threadneedle.global({ disableKeepAliveAgent: true }); + respondOk(); + + threadneedle.addMethod(name, { + method: 'get', + url: host + '/' + name, + expects: 200 + }); + + threadneedle[name]({}).done(function () { + assert.strictEqual(armed.length, 0, 'no keep-alive should have been armed'); + done(); + }, done); + }); + + it('should let a method re-enable it where the connector disabled it', function (done) { + var name = randString(10); + var threadneedle = new ThreadNeedle(); + threadneedle.global({ disableKeepAliveAgent: true }); + respondOk(); + + threadneedle.addMethod(name, { + method: 'get', + url: host + '/' + name, + expects: 200, + disableKeepAliveAgent: false + }); + + threadneedle[name]({}).done(function () { + assert.strictEqual(armed[0].delay, resolveKeepAliveAgent.KEEPALIVE_DELAY); + done(); + }, done); + }); + + }); + + describe('Caller overrides', function () { + + it('should use an agent supplied by the method instead of its own', function (done) { + var name = randString(10); + var threadneedle = new ThreadNeedle(); + var used = false; + + class RecordingAgent extends http.Agent { + createConnection (options, callback) { + used = true; + return super.createConnection(options, callback); + } + } + var mine = new RecordingAgent({ keepAlive: false }); + + server.once('request', function (req, res) { + res.writeHead(200); + res.end('ok'); + }); + + threadneedle.addMethod(name, { + method: 'get', + url: host + '/' + name, + expects: 200, + //Substitution keeps what a function returns, so the agent's prototype survives + options: { agent: function () { return mine; } } + }); + + threadneedle[name]({}).done(function () { + assert.strictEqual(used, true, 'the method\'s own agent should have been used'); + done(); + }, done); + }); + + it('should let a method opt out of keep-alive with `agent: false`', function (done) { + var name = randString(10); + var threadneedle = new ThreadNeedle(); + + server.once('request', function (req, res) { + res.writeHead(200); + res.end('ok'); + }); + + threadneedle.addMethod(name, { + method: 'get', + url: host + '/' + name, + expects: 200, + options: { agent: function () { return false; } } + }); + + threadneedle[name]({}).done(function () { + assert.strictEqual(armed.length, 0, 'no keep-alive should have been armed'); + done(); + }, done); + }); + + }); + +});