From e351ed98032af5ada1a0e5889742a754f656a794 Mon Sep 17 00:00:00 2001 From: Christina Ying Wang Date: Wed, 26 Oct 2022 14:59:10 -0700 Subject: [PATCH] Use runHealthchecks action for GET /v1/healthy Signed-off-by: Christina Ying Wang --- src/device-api/actions.ts | 24 ++++++++++++ src/device-api/index.ts | 12 ++---- test/integration/device-api/v1.spec.ts | 53 ++++++++++++++++++++++++++ test/legacy/41-device-api-v1.spec.ts | 46 +--------------------- test/unit/device-api/actions.spec.ts | 25 ++++++++++++ 5 files changed, 107 insertions(+), 53 deletions(-) create mode 100644 src/device-api/actions.ts create mode 100644 test/integration/device-api/v1.spec.ts create mode 100644 test/unit/device-api/actions.spec.ts diff --git a/src/device-api/actions.ts b/src/device-api/actions.ts new file mode 100644 index 00000000..a947d708 --- /dev/null +++ b/src/device-api/actions.ts @@ -0,0 +1,24 @@ +import log from '../lib/supervisor-console'; + +/** + * Run an array of healthchecks, outputting whether all passed or not + * Used by: + * - GET /v1/healthy + */ +export const runHealthchecks = async ( + healthchecks: Array<() => Promise>, +) => { + const HEALTHCHECK_FAILURE = 'Healthcheck failed'; + + try { + const checks = await Promise.all(healthchecks.map((fn) => fn())); + if (checks.some((check) => !check)) { + throw new Error(HEALTHCHECK_FAILURE); + } + } catch { + log.error(HEALTHCHECK_FAILURE); + return false; + } + + return true; +}; diff --git a/src/device-api/index.ts b/src/device-api/index.ts index 67f196d0..f12f3529 100644 --- a/src/device-api/index.ts +++ b/src/device-api/index.ts @@ -3,6 +3,7 @@ import * as _ from 'lodash'; import * as middleware from './middleware'; import * as apiKeys from './api-keys'; +import * as actions from './actions'; import * as eventTracker from '../event-tracker'; import { reportCurrentState } from '../device-state'; import proxyvisor from '../proxyvisor'; @@ -43,15 +44,10 @@ export class SupervisorAPI { this.api.use(middleware.logging); this.api.get('/v1/healthy', async (_req, res) => { - try { - const healths = await Promise.all(this.healthchecks.map((fn) => fn())); - if (!_.every(healths)) { - log.error('Healthcheck failed'); - return res.status(500).send('Unhealthy'); - } + const isHealthy = await actions.runHealthchecks(this.healthchecks); + if (isHealthy) { return res.sendStatus(200); - } catch { - log.error('Healthcheck failed'); + } else { return res.status(500).send('Unhealthy'); } }); diff --git a/test/integration/device-api/v1.spec.ts b/test/integration/device-api/v1.spec.ts new file mode 100644 index 00000000..7d6a42fc --- /dev/null +++ b/test/integration/device-api/v1.spec.ts @@ -0,0 +1,53 @@ +import * as express from 'express'; +import { SinonStub, stub } from 'sinon'; +import * as request from 'supertest'; + +import * as config from '~/src/config'; +import * as deviceApi from '~/src/device-api'; +import * as actions from '~/src/device-api/actions'; +import * as v1 from '~/src/device-api/v1'; + +describe('device-api/v1', () => { + let api: express.Application; + + before(async () => { + await config.initialized(); + + // `api` is a private property on SupervisorAPI but + // passing it directly to supertest is easier than + // setting up an API listen port & timeout + api = new deviceApi.SupervisorAPI({ + routers: [v1.router], + healthchecks: [], + // @ts-expect-error + }).api; + }); + + describe('GET /v1/healthy', () => { + after(() => { + api = new deviceApi.SupervisorAPI({ + routers: [v1.router], + healthchecks: [], + // @ts-expect-error + }).api; + }); + + it('responds with 200 because all healthchecks pass', async () => { + api = new deviceApi.SupervisorAPI({ + routers: [v1.router], + healthchecks: [stub().resolves(true), stub().resolves(true)], + // @ts-expect-error + }).api; + await request(api).get('/v1/healthy').expect(200); + }); + + it('responds with 500 because some healthchecks did not pass', async () => { + api = new deviceApi.SupervisorAPI({ + routers: [v1.router], + healthchecks: [stub().resolves(false), stub().resolves(true)], + // @ts-expect-error + }).api; + await request(api).get('/v1/healthy').expect(500); + }); + }); +}); diff --git a/test/legacy/41-device-api-v1.spec.ts b/test/legacy/41-device-api-v1.spec.ts index 3b0cbf9c..10feb6f2 100644 --- a/test/legacy/41-device-api-v1.spec.ts +++ b/test/legacy/41-device-api-v1.spec.ts @@ -39,7 +39,6 @@ import App from '~/src/compose/app'; describe('SupervisorAPI [V1 Endpoints]', () => { let api: SupervisorAPI; - let healthCheckStubs: SinonStub[]; let targetStateCacheMock: SinonStub; const request = supertest( `http://127.0.0.1:${mockedAPI.mockedOptions.listenPort}`, @@ -87,15 +86,9 @@ describe('SupervisorAPI [V1 Endpoints]', () => { // Do not apply target state stub(deviceState, 'applyStep').resolves(); - // Stub health checks so we can modify them whenever needed - healthCheckStubs = [ - stub(apiBinder, 'healthcheck'), - stub(deviceState, 'healthcheck'), - ]; - // The mockedAPI contains stubs that might create unexpected results // See the module to know what has been stubbed - api = await mockedAPI.create(healthCheckStubs); + api = await mockedAPI.create([]); // Start test API await api.listen( @@ -120,8 +113,6 @@ describe('SupervisorAPI [V1 Endpoints]', () => { } } (deviceState.applyStep as SinonStub).restore(); - // Restore healthcheck stubs - healthCheckStubs.forEach((hc) => hc.restore()); // Remove any test data generated await mockedAPI.cleanUp(); targetStateCacheMock.restore(); @@ -179,41 +170,6 @@ describe('SupervisorAPI [V1 Endpoints]', () => { }); }); - describe('GET /v1/healthy', () => { - it('returns OK because all checks pass', async () => { - // Make all healthChecks pass - healthCheckStubs.forEach((hc) => hc.resolves(true)); - await request - .get('/v1/healthy') - .set('Accept', 'application/json') - .set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`) - .expect(sampleResponses.V1.GET['/healthy'].statusCode) - .then((response) => { - expect(response.body).to.deep.equal( - sampleResponses.V1.GET['/healthy'].body, - ); - expect(response.text).to.deep.equal( - sampleResponses.V1.GET['/healthy'].text, - ); - }); - }); - it('Fails because some checks did not pass', async () => { - healthCheckStubs.forEach((hc) => hc.resolves(false)); - await request - .get('/v1/healthy') - .set('Accept', 'application/json') - .expect(sampleResponses.V1.GET['/healthy [2]'].statusCode) - .then((response) => { - expect(response.body).to.deep.equal( - sampleResponses.V1.GET['/healthy [2]'].body, - ); - expect(response.text).to.deep.equal( - sampleResponses.V1.GET['/healthy [2]'].text, - ); - }); - }); - }); - describe('GET /v1/apps/:appId', () => { it('does not return information for an application when there is more than 1 container', async () => { await request diff --git a/test/unit/device-api/actions.spec.ts b/test/unit/device-api/actions.spec.ts new file mode 100644 index 00000000..20f1d8a4 --- /dev/null +++ b/test/unit/device-api/actions.spec.ts @@ -0,0 +1,25 @@ +import { expect } from 'chai'; + +import * as actions from '~/src/device-api/actions'; + +describe('device-api/actions', () => { + describe('runs healthchecks', () => { + const taskTrue = () => Promise.resolve(true); + const taskFalse = () => Promise.resolve(false); + const taskError = () => { + throw new Error(); + }; + + it('resolves true if all healthchecks pass', async () => { + expect(await actions.runHealthchecks([taskTrue, taskTrue])).to.be.true; + }); + + it('resolves false if any healthchecks throw an error or fail', async () => { + expect(await actions.runHealthchecks([taskTrue, taskFalse])).to.be.false; + expect(await actions.runHealthchecks([taskTrue, taskError])).to.be.false; + expect(await actions.runHealthchecks([taskFalse, taskError])).to.be.false; + expect(await actions.runHealthchecks([taskFalse, taskFalse])).to.be.false; + expect(await actions.runHealthchecks([taskError, taskError])).to.be.false; + }); + }); +});