diff --git a/lib/utilities/healthcheckHandler.js b/lib/utilities/healthcheckHandler.js index 35186299f8..6dd8c816dc 100644 --- a/lib/utilities/healthcheckHandler.js +++ b/lib/utilities/healthcheckHandler.js @@ -41,12 +41,7 @@ function writeResponse(res, error, log, results, cb) { * @return {undefined} */ function clientCheck(flightCheckOnStartUp, log, cb) { - const clients = [ - data, - metadata, - vault, - kms, - ]; + const clients = [data, metadata, vault, kms]; const clientTasks = []; clients.forEach(client => { if (typeof client.checkHealth === 'function') { @@ -56,6 +51,24 @@ function clientCheck(flightCheckOnStartUp, log, cb) { } }); async.parallel(clientTasks, (err, results) => { + // Error's message/name/stack are non-enumerable, so JSON.stringify + // would drop them and log "error":{}. Copy them onto a plain object. + results.forEach(clientResult => { + Object.keys(clientResult).forEach(k => { + const e = clientResult[k] && clientResult[k].error; + if (e instanceof Error) { + // eslint-disable-next-line no-param-reassign + clientResult[k].error = { + ...e, + message: e.message, + name: e.name, + ...(e.code !== undefined && { code: e.code }), + ...(e.statusCode !== undefined && { statusCode: e.statusCode }), + ...(e.$metadata !== undefined && { $metadata: e.$metadata }), + }; + } + }); + }); // obj will be an object of the healthcheck results of // every backends. No errors were returned directly to // async.parallel in order to complete the check, so a @@ -65,21 +78,27 @@ function clientCheck(flightCheckOnStartUp, log, cb) { // Each result in the array represents one client (data, metadata, vault, kms) // with its backend locations as keys. // If ALL backend/locations of ANY client have errors, the overall check fails. - const { obj, fail } = results.reduce((acc, clientResult) => { - Object.assign(acc.obj, clientResult); + const { obj, fail } = results.reduce( + (acc, clientResult) => { + Object.assign(acc.obj, clientResult); - // Check if ALL backends/locations of this client have errors - const keys = Object.keys(clientResult); - // eslint-disable-next-line no-param-reassign - acc.fail ||= keys.length > 0 && keys.every(k => - // if there is an error from an external backend, - // only return a 500 if it is on startup - // (flightCheckOnStartUp set to true) - clientResult[k].error && (flightCheckOnStartUp || !clientResult[k].external) - ); + // Check if ALL backends/locations of this client have errors + const keys = Object.keys(clientResult); + // eslint-disable-next-line no-param-reassign + acc.fail ||= + keys.length > 0 && + keys.every( + k => + // if there is an error from an external backend, + // only return a 500 if it is on startup + // (flightCheckOnStartUp set to true) + clientResult[k].error && (flightCheckOnStartUp || !clientResult[k].external), + ); - return acc; - }, { obj: {}, fail: false }); + return acc; + }, + { obj: {}, fail: false }, + ); if (fail) { return cb(errors.InternalError, obj); } @@ -106,8 +125,7 @@ function routeHandler(deep, req, res, log, statsClient, cb) { } function checkIP(clientIP) { - return ipCheck.ipMatchCidrList( - _config.healthChecks.allowFrom, clientIP); + return ipCheck.ipMatchCidrList(_config.healthChecks.allowFrom, clientIP); } /** @@ -139,8 +157,7 @@ function healthcheckHandler(clientIP, req, res, log, statsClient, deep) { if (!checkIP(clientIP)) { return healthcheckEndHandler(errors.AccessDenied, []); } - return routeHandler(deep, req, res, log, statsClient, - healthcheckEndHandler); + return routeHandler(deep, req, res, log, statsClient, healthcheckEndHandler); } module.exports = { diff --git a/tests/unit/healthchecks/clientCheck.js b/tests/unit/healthchecks/clientCheck.js index 2e514af8b4..f037b1be9a 100644 --- a/tests/unit/healthchecks/clientCheck.js +++ b/tests/unit/healthchecks/clientCheck.js @@ -1,6 +1,7 @@ const assert = require('assert'); const sinon = require('sinon'); const { errors } = require('arsenal'); +const { S3ServiceException } = require('@aws-sdk/client-s3'); const { clientCheck } = require('../../../lib/utilities/healthcheckHandler'); const { DummyRequestLogger } = require('../helpers'); @@ -11,6 +12,13 @@ const kms = require('../../../lib/kms/wrapper'); const log = new DummyRequestLogger(); +const serializedInternalError = { + InternalError: true, + message: 'InternalError', + name: 'Error', + code: 500, +}; + describe('clientCheck - failure detection logic', () => { let dataStub; let metadataStub; @@ -30,19 +38,27 @@ describe('clientCheck - failure detection logic', () => { }); it('should succeed when all backends are healthy', done => { - dataStub.callsFake((log, cb) => cb(null, { - 'sproxyd-loc1': { code: 200, message: 'OK' }, - 'sproxyd-loc2': { code: 200, message: 'OK' }, - })); - metadataStub.callsFake((log, cb) => cb(null, { - metadata: { code: 200, message: 'OK' }, - })); - vaultStub.callsFake((log, cb) => cb(null, { - vault: { code: 200, message: 'OK' }, - })); - kmsStub.callsFake((log, cb) => cb(null, { - kms: { code: 200, message: 'OK' }, - })); + dataStub.callsFake((log, cb) => + cb(null, { + 'sproxyd-loc1': { code: 200, message: 'OK' }, + 'sproxyd-loc2': { code: 200, message: 'OK' }, + }), + ); + metadataStub.callsFake((log, cb) => + cb(null, { + metadata: { code: 200, message: 'OK' }, + }), + ); + vaultStub.callsFake((log, cb) => + cb(null, { + vault: { code: 200, message: 'OK' }, + }), + ); + kmsStub.callsFake((log, cb) => + cb(null, { + kms: { code: 200, message: 'OK' }, + }), + ); clientCheck(false, log, (err, result) => { assert.ifError(err); @@ -58,26 +74,34 @@ describe('clientCheck - failure detection logic', () => { }); it('should fail when ALL backends of data client fail while metadata is healthy', done => { - dataStub.callsFake((log, cb) => cb(null, { - 'sproxyd-loc1': { error: errors.InternalError, code: 500 }, - 'sproxyd-loc2': { error: errors.InternalError, code: 500 }, - })); - metadataStub.callsFake((log, cb) => cb(null, { - metadata: { code: 200, message: 'OK' }, - })); - vaultStub.callsFake((log, cb) => cb(null, { - vault: { code: 200, message: 'OK' }, - })); - kmsStub.callsFake((log, cb) => cb(null, { - kms: { code: 200, message: 'OK' }, - })); + dataStub.callsFake((log, cb) => + cb(null, { + 'sproxyd-loc1': { error: errors.InternalError, code: 500 }, + 'sproxyd-loc2': { error: errors.InternalError, code: 500 }, + }), + ); + metadataStub.callsFake((log, cb) => + cb(null, { + metadata: { code: 200, message: 'OK' }, + }), + ); + vaultStub.callsFake((log, cb) => + cb(null, { + vault: { code: 200, message: 'OK' }, + }), + ); + kmsStub.callsFake((log, cb) => + cb(null, { + kms: { code: 200, message: 'OK' }, + }), + ); clientCheck(false, log, (err, result) => { assert(err); assert.strictEqual(err.InternalError, true); assert.deepStrictEqual(result, { - 'sproxyd-loc1': { error: errors.InternalError, code: 500 }, - 'sproxyd-loc2': { error: errors.InternalError, code: 500 }, + 'sproxyd-loc1': { error: serializedInternalError, code: 500 }, + 'sproxyd-loc2': { error: serializedInternalError, code: 500 }, metadata: { code: 200, message: 'OK' }, vault: { code: 200, message: 'OK' }, kms: { code: 200, message: 'OK' }, @@ -87,24 +111,32 @@ describe('clientCheck - failure detection logic', () => { }); it('should succeed when ONE data location fails but another is healthy', done => { - dataStub.callsFake((log, cb) => cb(null, { - 'sproxyd-loc1': { error: errors.InternalError, code: 500 }, - 'sproxyd-loc2': { code: 200, message: 'OK' }, - })); - metadataStub.callsFake((log, cb) => cb(null, { - metadata: { code: 200, message: 'OK' }, - })); - vaultStub.callsFake((log, cb) => cb(null, { - vault: { code: 200, message: 'OK' }, - })); - kmsStub.callsFake((log, cb) => cb(null, { - kms: { code: 200, message: 'OK' }, - })); + dataStub.callsFake((log, cb) => + cb(null, { + 'sproxyd-loc1': { error: errors.InternalError, code: 500 }, + 'sproxyd-loc2': { code: 200, message: 'OK' }, + }), + ); + metadataStub.callsFake((log, cb) => + cb(null, { + metadata: { code: 200, message: 'OK' }, + }), + ); + vaultStub.callsFake((log, cb) => + cb(null, { + vault: { code: 200, message: 'OK' }, + }), + ); + kmsStub.callsFake((log, cb) => + cb(null, { + kms: { code: 200, message: 'OK' }, + }), + ); clientCheck(false, log, (err, result) => { assert.ifError(err); assert.deepStrictEqual(result, { - 'sproxyd-loc1': { error: errors.InternalError, code: 500 }, + 'sproxyd-loc1': { error: serializedInternalError, code: 500 }, 'sproxyd-loc2': { code: 200, message: 'OK' }, metadata: { code: 200, message: 'OK' }, vault: { code: 200, message: 'OK' }, @@ -115,27 +147,35 @@ describe('clientCheck - failure detection logic', () => { }); it('should fail when ALL backends of multiple clients fail', done => { - dataStub.callsFake((log, cb) => cb(null, { - 'sproxyd-loc1': { error: errors.InternalError, code: 500 }, - 'sproxyd-loc2': { error: errors.InternalError, code: 500 }, - })); - metadataStub.callsFake((log, cb) => cb(null, { - metadata: { error: errors.InternalError, code: 500 }, - })); - vaultStub.callsFake((log, cb) => cb(null, { - vault: { code: 200, message: 'OK' }, - })); - kmsStub.callsFake((log, cb) => cb(null, { - kms: { code: 200, message: 'OK' }, - })); + dataStub.callsFake((log, cb) => + cb(null, { + 'sproxyd-loc1': { error: errors.InternalError, code: 500 }, + 'sproxyd-loc2': { error: errors.InternalError, code: 500 }, + }), + ); + metadataStub.callsFake((log, cb) => + cb(null, { + metadata: { error: errors.InternalError, code: 500 }, + }), + ); + vaultStub.callsFake((log, cb) => + cb(null, { + vault: { code: 200, message: 'OK' }, + }), + ); + kmsStub.callsFake((log, cb) => + cb(null, { + kms: { code: 200, message: 'OK' }, + }), + ); clientCheck(false, log, (err, result) => { assert(err); assert.strictEqual(err.InternalError, true); assert.deepStrictEqual(result, { - 'sproxyd-loc1': { error: errors.InternalError, code: 500 }, - 'sproxyd-loc2': { error: errors.InternalError, code: 500 }, - metadata: { error: errors.InternalError, code: 500 }, + 'sproxyd-loc1': { error: serializedInternalError, code: 500 }, + 'sproxyd-loc2': { error: serializedInternalError, code: 500 }, + metadata: { error: serializedInternalError, code: 500 }, vault: { code: 200, message: 'OK' }, kms: { code: 200, message: 'OK' }, }); @@ -145,15 +185,21 @@ describe('clientCheck - failure detection logic', () => { it('should succeed when client returns empty result', done => { dataStub.callsFake((log, cb) => cb(null, {})); - metadataStub.callsFake((log, cb) => cb(null, { - metadata: { code: 200, message: 'OK' }, - })); - vaultStub.callsFake((log, cb) => cb(null, { - vault: { code: 200, message: 'OK' }, - })); - kmsStub.callsFake((log, cb) => cb(null, { - kms: { code: 200, message: 'OK' }, - })); + metadataStub.callsFake((log, cb) => + cb(null, { + metadata: { code: 200, message: 'OK' }, + }), + ); + vaultStub.callsFake((log, cb) => + cb(null, { + vault: { code: 200, message: 'OK' }, + }), + ); + kmsStub.callsFake((log, cb) => + cb(null, { + kms: { code: 200, message: 'OK' }, + }), + ); clientCheck(false, log, (err, result) => { assert.ifError(err); @@ -167,34 +213,44 @@ describe('clientCheck - failure detection logic', () => { }); describe('external backend error handling', () => { - it('should NOT fail on external backend errors during normal operation ' + - '(flightCheckOnStartUp=false)', done => { - dataStub.callsFake((log, cb) => cb(null, { - 's3-backend': { error: errors.InternalError, code: 500, external: true }, - })); - metadataStub.callsFake((log, cb) => cb(null, { - metadata: { code: 200, message: 'OK' }, - })); - vaultStub.callsFake((log, cb) => cb(null, {})); - kmsStub.callsFake((log, cb) => cb(null, {})); + it( + 'should NOT fail on external backend errors during normal operation ' + '(flightCheckOnStartUp=false)', + done => { + dataStub.callsFake((log, cb) => + cb(null, { + 's3-backend': { error: errors.InternalError, code: 500, external: true }, + }), + ); + metadataStub.callsFake((log, cb) => + cb(null, { + metadata: { code: 200, message: 'OK' }, + }), + ); + vaultStub.callsFake((log, cb) => cb(null, {})); + kmsStub.callsFake((log, cb) => cb(null, {})); - clientCheck(false, log, (err, result) => { - assert.ifError(err); - assert.deepStrictEqual(result, { - 's3-backend': { error: errors.InternalError, code: 500, external: true }, - metadata: { code: 200, message: 'OK' }, + clientCheck(false, log, (err, result) => { + assert.ifError(err); + assert.deepStrictEqual(result, { + 's3-backend': { error: serializedInternalError, code: 500, external: true }, + metadata: { code: 200, message: 'OK' }, + }); + done(); }); - done(); - }); - }); + }, + ); it('should fail on external backend errors during startup (flightCheckOnStartUp=true)', done => { - dataStub.callsFake((log, cb) => cb(null, { - 's3-backend': { error: errors.InternalError, code: 500, external: true }, - })); - metadataStub.callsFake((log, cb) => cb(null, { - metadata: { code: 200, message: 'OK' }, - })); + dataStub.callsFake((log, cb) => + cb(null, { + 's3-backend': { error: errors.InternalError, code: 500, external: true }, + }), + ); + metadataStub.callsFake((log, cb) => + cb(null, { + metadata: { code: 200, message: 'OK' }, + }), + ); vaultStub.callsFake((log, cb) => cb(null, {})); kmsStub.callsFake((log, cb) => cb(null, {})); @@ -202,34 +258,75 @@ describe('clientCheck - failure detection logic', () => { assert(err); assert.strictEqual(err.InternalError, true); assert.deepStrictEqual(result, { - 's3-backend': { error: errors.InternalError, code: 500, external: true }, + 's3-backend': { error: serializedInternalError, code: 500, external: true }, metadata: { code: 200, message: 'OK' }, }); done(); }); }); - it('should succeed when external backend fails but internal backend is healthy ' + - '(flightCheckOnStartUp=false)', done => { - dataStub.callsFake((log, cb) => cb(null, { - 'sproxyd-loc1': { code: 200, message: 'OK' }, - 's3-backend': { error: errors.InternalError, code: 500, external: true }, - })); - metadataStub.callsFake((log, cb) => cb(null, { - metadata: { code: 200, message: 'OK' }, - })); + it( + 'should succeed when external backend fails but internal backend is healthy ' + + '(flightCheckOnStartUp=false)', + done => { + dataStub.callsFake((log, cb) => + cb(null, { + 'sproxyd-loc1': { code: 200, message: 'OK' }, + 's3-backend': { error: errors.InternalError, code: 500, external: true }, + }), + ); + metadataStub.callsFake((log, cb) => + cb(null, { + metadata: { code: 200, message: 'OK' }, + }), + ); + vaultStub.callsFake((log, cb) => cb(null, {})); + kmsStub.callsFake((log, cb) => cb(null, {})); + + clientCheck(false, log, (err, result) => { + assert.ifError(err); + assert.deepStrictEqual(result, { + 'sproxyd-loc1': { code: 200, message: 'OK' }, + 's3-backend': { error: serializedInternalError, code: 500, external: true }, + metadata: { code: 200, message: 'OK' }, + }); + done(); + }); + }, + ); + + it('should serialize AWS SDK S3ServiceException so message survives JSON.stringify', async () => { + const awsErr = new S3ServiceException({ + name: 'NoSuchBucket', + $fault: 'client', + $metadata: { httpStatusCode: 404, requestId: 'ABC123' }, + message: 'The specified bucket does not exist', + }); + assert.strictEqual(JSON.parse(JSON.stringify({ error: awsErr })).error.message, undefined); + + dataStub.callsFake((log, cb) => + cb(null, { + 'aws-loc': { error: awsErr, external: true }, + }), + ); + metadataStub.callsFake((log, cb) => + cb(null, { + metadata: { code: 200, message: 'OK' }, + }), + ); vaultStub.callsFake((log, cb) => cb(null, {})); kmsStub.callsFake((log, cb) => cb(null, {})); - clientCheck(false, log, (err, result) => { - assert.ifError(err); - assert.deepStrictEqual(result, { - 'sproxyd-loc1': { code: 200, message: 'OK' }, - 's3-backend': { error: errors.InternalError, code: 500, external: true }, - metadata: { code: 200, message: 'OK' }, - }); - done(); + const { err, result } = await new Promise(resolve => { + clientCheck(true, log, (err, result) => resolve({ err, result })); }); + assert(err); + + const roundTripped = JSON.parse(JSON.stringify(result)); + const loggedError = roundTripped['aws-loc'].error; + assert.strictEqual(loggedError.message, 'The specified bucket does not exist'); + assert.strictEqual(loggedError.name, 'NoSuchBucket'); + assert.deepStrictEqual(loggedError.$metadata, { httpStatusCode: 404, requestId: 'ABC123' }); }); }); });