From 56336f26a3516a3d6691beff01a8a9e82a0bf596 Mon Sep 17 00:00:00 2001 From: Christina Ying Wang Date: Tue, 21 Nov 2023 11:50:18 -0800 Subject: [PATCH 1/8] Separate routes & actions, add tests for GET /v2/device/vpn Change-type: patch Signed-off-by: Christina Ying Wang --- src/device-api/actions.ts | 13 ++++++++ src/device-api/v2.ts | 24 ++++++------- src/device-config.ts | 30 ++--------------- src/network.ts | 30 ++++++++++++++--- test/data/device-api-responses.json | 10 ------ test/integration/device-api/actions.spec.ts | 33 ++++++++++++++++++ test/integration/device-api/v2.spec.ts | 37 +++++++++++++++++++++ test/integration/network.spec.ts | 8 +++++ test/legacy/42-device-api-v2.spec.ts | 16 --------- 9 files changed, 130 insertions(+), 71 deletions(-) diff --git a/src/device-api/actions.ts b/src/device-api/actions.ts index 1a83e52d8..e6d8905f7 100644 --- a/src/device-api/actions.ts +++ b/src/device-api/actions.ts @@ -7,6 +7,7 @@ import * as deviceState from '../device-state'; import * as logger from '../logger'; import * as config from '../config'; import * as hostConfig from '../host-config'; +import { isVPNEnabled, isVPNActive } from '../network'; import * as applicationManager from '../compose/application-manager'; import { CompositionStepAction, @@ -411,3 +412,15 @@ export const patchHostConfig = async ( } await hostConfig.patch(conf, force); }; + +/** + * Get device VPN status + * Used by: + * - GET /v2/device/vpn + */ +export const getVPNStatus = async () => { + return { + enabled: await isVPNEnabled(), + connected: await isVPNActive(), + }; +}; diff --git a/src/device-api/v2.ts b/src/device-api/v2.ts index cd71b7f47..c8edd1514 100644 --- a/src/device-api/v2.ts +++ b/src/device-api/v2.ts @@ -11,7 +11,6 @@ import Volume from '../compose/volume'; import * as commitStore from '../compose/commit'; import * as config from '../config'; import * as db from '../db'; -import * as deviceConfig from '../device-config'; import * as logger from '../logger'; import * as images from '../compose/images'; import * as volumeManager from '../compose/volume-manager'; @@ -25,7 +24,6 @@ import { isBadRequestError, BadRequestError, } from '../lib/errors'; -import { isVPNActive } from '../network'; import { AuthorizedRequest } from './api-keys'; import { fromV2TargetState } from '../lib/legacy'; import * as actions from './actions'; @@ -511,18 +509,16 @@ router.get('/v2/device/tags', async (_req, res) => { } }); -router.get('/v2/device/vpn', async (_req, res) => { - const conf = await deviceConfig.getCurrent(); - // Build VPNInfo - const info = { - enabled: conf.SUPERVISOR_VPN_CONTROL === 'true', - connected: await isVPNActive(), - }; - // Return payload - return res.json({ - status: 'success', - vpn: info, - }); +router.get('/v2/device/vpn', async (_req, res, next) => { + try { + const vpnStatus = await actions.getVPNStatus(); + return res.json({ + status: 'success', + vpn: vpnStatus, + }); + } catch (e: unknown) { + next(e); + } }); router.get('/v2/cleanup-volumes', async (req: AuthorizedRequest, res) => { diff --git a/src/device-config.ts b/src/device-config.ts index 8c1315190..218185618 100644 --- a/src/device-config.ts +++ b/src/device-config.ts @@ -5,9 +5,8 @@ import { promises as fs } from 'fs'; import * as config from './config'; import * as db from './db'; import * as logger from './logger'; -import * as dbus from './lib/dbus'; +import * as network from './network'; import { EnvVarObject } from './types'; -import { UnitNotLoadedError } from './lib/errors'; import { checkInt, checkTruthy } from './lib/validation'; import log from './lib/supervisor-console'; import * as configUtils from './config/utils'; @@ -18,8 +17,6 @@ import { Odmdata } from './config/backends/odmdata'; import * as fsUtils from './lib/fs-utils'; import { pathOnRoot } from './lib/host-utils'; -const vpnServiceName = 'openvpn'; - // This indicates the file on the host /tmp directory that // marks the need for a reboot. Since reboot is only triggered for now // by some config changes, we leave this here for now. There is planned @@ -99,7 +96,7 @@ const actionExecutors: DeviceActionExecutors = { logger.logConfigChange(logValue); } try { - await setVPNEnabled(step.target); + await network.setVPNEnabled(step.target); if (!initial) { logger.logConfigChange(logValue, { success: true }); } @@ -304,7 +301,7 @@ export async function getCurrent(): Promise> { currentConf[envVarName] = confValue != null ? confValue.toString() : ''; } // Add VPN information - currentConf['SUPERVISOR_VPN_CONTROL'] = (await isVPNEnabled()) + currentConf['SUPERVISOR_VPN_CONTROL'] = (await network.isVPNEnabled()) ? 'true' : 'false'; // Get list of configurable backends @@ -694,27 +691,6 @@ export async function setBootConfig( } } -async function isVPNEnabled(): Promise { - try { - const activeState = await dbus.serviceActiveState(vpnServiceName); - return !['inactive', 'deactivating'].includes(activeState); - } catch (e: any) { - if (UnitNotLoadedError(e)) { - return false; - } - throw e; - } -} - -async function setVPNEnabled(value: string | boolean = true) { - const enable = checkTruthy(value); - if (enable) { - await dbus.startService(vpnServiceName); - } else { - await dbus.stopService(vpnServiceName); - } -} - function configTest(method: string, a: string, b: string): boolean { switch (method) { case 'bool': diff --git a/src/network.ts b/src/network.ts index 5fa448b38..ad1f6e2a1 100644 --- a/src/network.ts +++ b/src/network.ts @@ -4,12 +4,11 @@ import * as networkCheck from 'network-checker'; import * as os from 'os'; import * as url from 'url'; +import * as dbus from './lib/dbus'; import * as constants from './lib/constants'; -import { EEXIST } from './lib/errors'; -import { checkFalsey } from './lib/validation'; - +import { EEXIST, UnitNotLoadedError } from './lib/errors'; +import { checkFalsey, checkTruthy } from './lib/validation'; import blink = require('./lib/blink'); - import log from './lib/supervisor-console'; const networkPattern = { @@ -52,6 +51,29 @@ export async function isVPNActive(): Promise { return active; } +const vpnServiceName = 'openvpn'; + +export async function isVPNEnabled(): Promise { + try { + const activeState = await dbus.serviceActiveState(vpnServiceName); + return !['inactive', 'deactivating'].includes(activeState); + } catch (e: any) { + if (UnitNotLoadedError(e)) { + return false; + } + throw e; + } +} + +export async function setVPNEnabled(value: string | boolean = true) { + const enable = checkTruthy(value); + if (enable) { + await dbus.startService(vpnServiceName); + } else { + await dbus.stopService(vpnServiceName); + } +} + async function vpnStatusInotifyCallback(): Promise { isConnectivityCheckPaused = await isVPNActive(); } diff --git a/test/data/device-api-responses.json b/test/data/device-api-responses.json index 3347e488f..5b151c03c 100644 --- a/test/data/device-api-responses.json +++ b/test/data/device-api-responses.json @@ -1,16 +1,6 @@ { "V2": { "GET": { - "/device/vpn": { - "statusCode": 200, - "body": { - "status": "success", - "vpn": { - "enabled": true, - "connected": false - } - } - }, "/applications/1/state": { "statusCode": 200, "body": { diff --git a/test/integration/device-api/actions.spec.ts b/test/integration/device-api/actions.spec.ts index a5316896e..438168701 100644 --- a/test/integration/device-api/actions.spec.ts +++ b/test/integration/device-api/actions.spec.ts @@ -7,6 +7,7 @@ import { setTimeout } from 'timers/promises'; import * as deviceState from '~/src/device-state'; import * as config from '~/src/config'; import * as hostConfig from '~/src/host-config'; +import * as network from '~/src/network'; import * as deviceApi from '~/src/device-api'; import * as actions from '~/src/device-api/actions'; import * as TargetState from '~/src/device-state/target-state'; @@ -833,3 +834,35 @@ describe('patches host config', () => { ); }); }); + +describe('gets VPN status', () => { + let activeStub: SinonStub; + let enabledStub: SinonStub; + + before(() => { + // Stub external dependencies which are separately tested in network.spec.ts + activeStub = stub(network, 'isVPNActive'); + enabledStub = stub(network, 'isVPNEnabled'); + }); + + after(() => { + activeStub.restore(); + enabledStub.restore(); + }); + + it('returns VPN active and enabled statuses', async () => { + activeStub.resolves(true); + enabledStub.resolves(true); + expect(await actions.getVPNStatus()).to.deep.equal({ + enabled: true, + connected: true, + }); + + activeStub.resolves(false); + enabledStub.resolves(false); + expect(await actions.getVPNStatus()).to.deep.equal({ + enabled: false, + connected: false, + }); + }); +}); diff --git a/test/integration/device-api/v2.spec.ts b/test/integration/device-api/v2.spec.ts index 7c189c9c8..de27dc412 100644 --- a/test/integration/device-api/v2.spec.ts +++ b/test/integration/device-api/v2.spec.ts @@ -637,4 +637,41 @@ describe('device-api/v2', () => { .expect(503); }); }); + + describe('GET /v2/device/vpn', () => { + // Actions are tested elsewhere so we can stub the dependency here + let getVPNStatusStub: SinonStub; + before(() => { + getVPNStatusStub = stub(actions, 'getVPNStatus'); + }); + after(() => { + getVPNStatusStub.restore(); + }); + + it('responds with 200 and vpn status', async () => { + const vpnStatus = { + active: true, + connected: false, + }; + getVPNStatusStub.resolves(vpnStatus); + await request(api) + .get('/v2/device/vpn') + .set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`) + .expect(200) + .then(({ body }) => { + expect(body).to.deep.equal({ + status: 'success', + vpn: vpnStatus, + }); + }); + }); + + it('responds with 503 if an error occurred', async () => { + getVPNStatusStub.throws(new Error()); + await request(api) + .get('/v2/device/vpn') + .set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`) + .expect(503); + }); + }); }); diff --git a/test/integration/network.spec.ts b/test/integration/network.spec.ts index d7410a9cf..35d702d36 100644 --- a/test/integration/network.spec.ts +++ b/test/integration/network.spec.ts @@ -7,6 +7,7 @@ import { expect } from 'chai'; import Log from '~/lib/supervisor-console'; import * as network from '~/src/network'; import * as constants from '~/lib/constants'; +import * as dbus from '~/lib/dbus'; describe('network', () => { it('checks VPN connection status', async () => { @@ -33,4 +34,11 @@ describe('network', () => { // Restore file system await testFs.restore(); }); + + it('checks VPN enabled status', async () => { + await dbus.stopService('openvpn'); + expect(await network.isVPNEnabled()).to.equal(false); + await dbus.startService('openvpn'); + expect(await network.isVPNEnabled()).to.equal(true); + }); }); diff --git a/test/legacy/42-device-api-v2.spec.ts b/test/legacy/42-device-api-v2.spec.ts index 6d9dcd1a2..e54d055c3 100644 --- a/test/legacy/42-device-api-v2.spec.ts +++ b/test/legacy/42-device-api-v2.spec.ts @@ -72,22 +72,6 @@ describe('SupervisorAPI [V2 Endpoints]', () => { applicationManagerSpy.resetHistory(); }); - describe('GET /v2/device/vpn', () => { - it('returns information about VPN connection', async () => { - await request - .get('/v2/device/vpn') - .set('Accept', 'application/json') - .set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`) - .expect('Content-Type', /json/) - .expect(sampleResponses.V2.GET['/device/vpn'].statusCode) - .then((response) => { - expect(response.body).to.deep.equal( - sampleResponses.V2.GET['/device/vpn'].body, - ); - }); - }); - }); - describe('GET /v2/applications/:appId/state', () => { it('returns information about a SPECIFIC application', async () => { await request From 0b5e92b7c6c91f2d000a2c5d80ff4aeea87cbc6e Mon Sep 17 00:00:00 2001 From: Christina Ying Wang Date: Tue, 21 Nov 2023 12:18:56 -0800 Subject: [PATCH 2/8] Separate routes & actions, add tests for GET /v2/device/name Signed-off-by: Christina Ying Wang --- src/device-api/actions.ts | 17 ++++++++--- src/device-api/v2.ts | 16 ++++++---- test/integration/device-api/actions.spec.ts | 11 +++++++ test/integration/device-api/v2.spec.ts | 34 +++++++++++++++++++++ 4 files changed, 68 insertions(+), 10 deletions(-) diff --git a/src/device-api/actions.ts b/src/device-api/actions.ts index e6d8905f7..a93a6ba2f 100644 --- a/src/device-api/actions.ts +++ b/src/device-api/actions.ts @@ -349,7 +349,7 @@ export const getSingleContainerApp = async (appId: number) => { /** * Returns legacy device info, update status, and service status for a single-container application. * Used by: - * - GET /v1/device + * - GET /v1/device */ export const getLegacyDeviceState = async () => { const state = await deviceState.getLegacyState(); @@ -390,7 +390,7 @@ export const getLegacyDeviceState = async () => { /** * Get host config from the host-config module; Returns proxy config and hostname. * Used by: - * - GET /v1/device/host-config + * - GET /v1/device/host-config */ export const getHostConfig = async () => { return await hostConfig.get(); @@ -399,7 +399,7 @@ export const getHostConfig = async () => { /** * Patch host configs such as proxy config and hostname * Used by: - * - PATCH /v1/device/host-config + * - PATCH /v1/device/host-config */ export const patchHostConfig = async ( conf: Parameters[0], @@ -416,7 +416,7 @@ export const patchHostConfig = async ( /** * Get device VPN status * Used by: - * - GET /v2/device/vpn + * - GET /v2/device/vpn */ export const getVPNStatus = async () => { return { @@ -424,3 +424,12 @@ export const getVPNStatus = async () => { connected: await isVPNActive(), }; }; + +/** + * Get device name + * Used by: + * - GET /v2/device/name + */ +export const getDeviceName = async () => { + return await config.get('name'); +}; diff --git a/src/device-api/v2.ts b/src/device-api/v2.ts index c8edd1514..84444c1e9 100644 --- a/src/device-api/v2.ts +++ b/src/device-api/v2.ts @@ -485,12 +485,16 @@ router.get('/v2/state/status', async (req: AuthorizedRequest, res) => { }); }); -router.get('/v2/device/name', async (_req, res) => { - const deviceName = await config.get('name'); - res.json({ - status: 'success', - deviceName, - }); +router.get('/v2/device/name', async (_req, res, next) => { + try { + const deviceName = await actions.getDeviceName(); + return res.json({ + status: 'success', + deviceName, + }); + } catch (e) { + next(e); + } }); router.get('/v2/device/tags', async (_req, res) => { diff --git a/test/integration/device-api/actions.spec.ts b/test/integration/device-api/actions.spec.ts index 438168701..94b072dd8 100644 --- a/test/integration/device-api/actions.spec.ts +++ b/test/integration/device-api/actions.spec.ts @@ -866,3 +866,14 @@ describe('gets VPN status', () => { }); }); }); + +describe('gets device name', () => { + before(async () => { + await config.initialized(); + }); + + it('returns device name', async () => { + await config.set({ name: 'test' }); + expect(await actions.getDeviceName()).to.equal('test'); + }); +}); diff --git a/test/integration/device-api/v2.spec.ts b/test/integration/device-api/v2.spec.ts index de27dc412..892dc400c 100644 --- a/test/integration/device-api/v2.spec.ts +++ b/test/integration/device-api/v2.spec.ts @@ -674,4 +674,38 @@ describe('device-api/v2', () => { .expect(503); }); }); + + describe('GET /v2/device/name', () => { + // Actions are tested elsewhere so we can stub the dependency here + let getDeviceNameStub: SinonStub; + before(() => { + getDeviceNameStub = stub(actions, 'getDeviceName'); + }); + after(() => { + getDeviceNameStub.restore(); + }); + + it('responds with 200 and device name', async () => { + const deviceName = 'my-rpi4'; + getDeviceNameStub.resolves(deviceName); + await request(api) + .get('/v2/device/name') + .set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`) + .expect(200) + .then(({ body }) => { + expect(body).to.deep.equal({ + status: 'success', + deviceName, + }); + }); + }); + + it('responds with 503 if an error occurred', async () => { + getDeviceNameStub.throws(new Error()); + await request(api) + .get('/v2/device/name') + .set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`) + .expect(503); + }); + }); }); From 9c23b9946790344bf8db50fb42a83650c64b967e Mon Sep 17 00:00:00 2001 From: Christina Ying Wang Date: Tue, 21 Nov 2023 13:57:41 -0800 Subject: [PATCH 3/8] Separate routes & actions, add tests for /v2/device/tags Signed-off-by: Christina Ying Wang --- src/device-api/actions.ts | 15 +++++++++ src/device-api/v2.ts | 10 +++--- test/integration/device-api/actions.spec.ts | 17 +++++++++++ test/integration/device-api/v2.spec.ts | 34 +++++++++++++++++++++ 4 files changed, 70 insertions(+), 6 deletions(-) diff --git a/src/device-api/actions.ts b/src/device-api/actions.ts index a93a6ba2f..a949396c7 100644 --- a/src/device-api/actions.ts +++ b/src/device-api/actions.ts @@ -8,6 +8,7 @@ import * as logger from '../logger'; import * as config from '../config'; import * as hostConfig from '../host-config'; import { isVPNEnabled, isVPNActive } from '../network'; +import { fetchDeviceTags } from '../api-binder'; import * as applicationManager from '../compose/application-manager'; import { CompositionStepAction, @@ -433,3 +434,17 @@ export const getVPNStatus = async () => { export const getDeviceName = async () => { return await config.get('name'); }; + +/** + * Get device tags + * Used by: + * - GET /v2/device/tags + */ +export const getDeviceTags = async () => { + try { + return await fetchDeviceTags(); + } catch (e: unknown) { + log.error((e as Error).message ?? e); + throw e; + } +}; diff --git a/src/device-api/v2.ts b/src/device-api/v2.ts index 84444c1e9..a155a73d4 100644 --- a/src/device-api/v2.ts +++ b/src/device-api/v2.ts @@ -3,7 +3,6 @@ import type { Response, NextFunction } from 'express'; import * as _ from 'lodash'; import * as deviceState from '../device-state'; -import * as apiBinder from '../api-binder'; import * as applicationManager from '../compose/application-manager'; import { CompositionStepAction } from '../compose/composition-steps'; import { Service } from '../compose/service'; @@ -499,16 +498,15 @@ router.get('/v2/device/name', async (_req, res, next) => { router.get('/v2/device/tags', async (_req, res) => { try { - const tags = await apiBinder.fetchDeviceTags(); + const tags = await actions.getDeviceTags(); return res.json({ status: 'success', tags, }); - } catch (e: any) { - log.error(e); - res.status(500).json({ + } catch (e: unknown) { + return res.status(500).json({ status: 'failed', - message: e.message, + message: (e as Error).message ?? e, }); } }); diff --git a/test/integration/device-api/actions.spec.ts b/test/integration/device-api/actions.spec.ts index 94b072dd8..9a53ed05b 100644 --- a/test/integration/device-api/actions.spec.ts +++ b/test/integration/device-api/actions.spec.ts @@ -9,6 +9,7 @@ import * as config from '~/src/config'; import * as hostConfig from '~/src/host-config'; import * as network from '~/src/network'; import * as deviceApi from '~/src/device-api'; +import * as apiBinder from '~/src/api-binder'; import * as actions from '~/src/device-api/actions'; import * as TargetState from '~/src/device-state/target-state'; import { cleanupDocker } from '~/test-lib/docker-helper'; @@ -877,3 +878,19 @@ describe('gets device name', () => { expect(await actions.getDeviceName()).to.equal('test'); }); }); + +describe('gets device tags', () => { + let fetchDeviceTagsStub: SinonStub; + before(() => { + fetchDeviceTagsStub = stub(apiBinder, 'fetchDeviceTags'); + }); + after(() => { + fetchDeviceTagsStub.restore(); + }); + + it('returns device tags fetched from api-binder', async () => { + const fetchResponse = [{ id: 1, name: 'test', value: '' }]; + fetchDeviceTagsStub.resolves(fetchResponse); + expect(await actions.getDeviceTags()).to.deep.equal(fetchResponse); + }); +}); diff --git a/test/integration/device-api/v2.spec.ts b/test/integration/device-api/v2.spec.ts index 892dc400c..9610b8149 100644 --- a/test/integration/device-api/v2.spec.ts +++ b/test/integration/device-api/v2.spec.ts @@ -708,4 +708,38 @@ describe('device-api/v2', () => { .expect(503); }); }); + + describe('GET /v2/device/tags', () => { + // Actions are tested elsewhere so we can stub the dependency here + let getDeviceTagsStub: SinonStub; + before(() => { + getDeviceTagsStub = stub(actions, 'getDeviceTags'); + }); + after(() => { + getDeviceTagsStub.restore(); + }); + + it('responds with 200 and device tags', async () => { + const tags = { id: 1, name: 'test', value: '' }; + getDeviceTagsStub.resolves(tags); + await request(api) + .get('/v2/device/tags') + .set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`) + .expect(200) + .then(({ body }) => { + expect(body).to.deep.equal({ + status: 'success', + tags, + }); + }); + }); + + it('responds with 500 if an error occurred', async () => { + getDeviceTagsStub.throws(new Error()); + await request(api) + .get('/v2/device/tags') + .set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`) + .expect(500); + }); + }); }); From 6e277b794a0c3771476a9492fb76a40c50457612 Mon Sep 17 00:00:00 2001 From: Christina Ying Wang Date: Wed, 29 Nov 2023 14:50:10 -0800 Subject: [PATCH 4/8] Separate routes & actions, add tests for GET /v2/cleanup-volumes Signed-off-by: Christina Ying Wang --- src/compose/application-manager.ts | 19 +++++++++++++++ src/device-api/actions.ts | 16 ++++++++++++ src/device-api/v2.ts | 27 +++++++-------------- test/integration/device-api/actions.spec.ts | 16 ++++++++++++ test/integration/device-api/v2.spec.ts | 27 +++++++++++++++++++++ 5 files changed, 87 insertions(+), 18 deletions(-) diff --git a/src/compose/application-manager.ts b/src/compose/application-manager.ts index 051d65bb8..387166931 100644 --- a/src/compose/application-manager.ts +++ b/src/compose/application-manager.ts @@ -1031,3 +1031,22 @@ export async function getState() { } return state; } + +export async function removeOrphanedVolumes( + inScope: (id: number) => boolean = () => true, +) { + const targetState = await getTargetApps(); + // Get list of referenced volumes to keep + const referencedVolumes = Object.values(targetState) + // Don't include volumes out of scope + .filter((app) => inScope(app.id)) + .flatMap((app) => { + const [release] = Object.values(app.releases); + // Return a list of the volume names + return Object.keys(release?.volumes ?? {}).map((volumeName) => + Volume.generateDockerName(app.id, volumeName), + ); + }); + + await volumeManager.removeOrphanedVolumes(referencedVolumes); +} diff --git a/src/device-api/actions.ts b/src/device-api/actions.ts index a949396c7..e0d9ca06b 100644 --- a/src/device-api/actions.ts +++ b/src/device-api/actions.ts @@ -2,6 +2,7 @@ import * as _ from 'lodash'; import { getGlobalApiKey, refreshKey } from '.'; import * as messages from './messages'; +import { AuthorizedRequest } from './api-keys'; import * as eventTracker from '../event-tracker'; import * as deviceState from '../device-state'; import * as logger from '../logger'; @@ -448,3 +449,18 @@ export const getDeviceTags = async () => { throw e; } }; + +/** + * Clean up orphaned volumes + * Used by: + * - GET /v2/cleanup-volumes + */ +export const cleanupVolumes = async ( + withScope: AuthorizedRequest['auth']['isScoped'] = () => true, +) => { + // It's better practice to access engine functionality through application-manager + // than through volume-manager directly, as the latter should be an internal module + await applicationManager.removeOrphanedVolumes((id) => + withScope({ apps: [id] }), + ); +}; diff --git a/src/device-api/v2.ts b/src/device-api/v2.ts index a155a73d4..03e7bf5d0 100644 --- a/src/device-api/v2.ts +++ b/src/device-api/v2.ts @@ -6,13 +6,11 @@ import * as deviceState from '../device-state'; import * as applicationManager from '../compose/application-manager'; import { CompositionStepAction } from '../compose/composition-steps'; import { Service } from '../compose/service'; -import Volume from '../compose/volume'; import * as commitStore from '../compose/commit'; import * as config from '../config'; import * as db from '../db'; import * as logger from '../logger'; import * as images from '../compose/images'; -import * as volumeManager from '../compose/volume-manager'; import * as serviceManager from '../compose/service-manager'; import { spawnJournalctl } from '../lib/journald'; import log from '../lib/supervisor-console'; @@ -523,23 +521,16 @@ router.get('/v2/device/vpn', async (_req, res, next) => { } }); -router.get('/v2/cleanup-volumes', async (req: AuthorizedRequest, res) => { - const targetState = await applicationManager.getTargetApps(); - const referencedVolumes = Object.values(targetState) - // if this app isn't in scope of the request, do not cleanup it's volumes - .filter((app) => req.auth.isScoped({ apps: [app.id] })) - .flatMap((app) => { - const [release] = Object.values(app.releases); - // Return a list of the volume names - return Object.keys(release?.volumes ?? {}).map((volumeName) => - Volume.generateDockerName(app.id, volumeName), - ); +// This should be a POST but we have to keep it a GET for interface consistency +router.get('/v2/cleanup-volumes', async (req: AuthorizedRequest, res, next) => { + try { + await actions.cleanupVolumes(req.auth.isScoped); + return res.json({ + status: 'success', }); - - await volumeManager.removeOrphanedVolumes(referencedVolumes); - res.json({ - status: 'success', - }); + } catch (e: unknown) { + next(e); + } }); router.post('/v2/journal-logs', (req, res) => { diff --git a/test/integration/device-api/actions.spec.ts b/test/integration/device-api/actions.spec.ts index 9a53ed05b..13d09e2e8 100644 --- a/test/integration/device-api/actions.spec.ts +++ b/test/integration/device-api/actions.spec.ts @@ -12,6 +12,7 @@ import * as deviceApi from '~/src/device-api'; import * as apiBinder from '~/src/api-binder'; import * as actions from '~/src/device-api/actions'; import * as TargetState from '~/src/device-state/target-state'; +import * as applicationManager from '~/src/compose/application-manager'; import { cleanupDocker } from '~/test-lib/docker-helper'; import { exec } from '~/src/lib/fs-utils'; @@ -894,3 +895,18 @@ describe('gets device tags', () => { expect(await actions.getDeviceTags()).to.deep.equal(fetchResponse); }); }); + +describe('cleans up orphaned volumes', () => { + let removeOrphanedVolumes: SinonStub; + before(() => { + removeOrphanedVolumes = stub(applicationManager, 'removeOrphanedVolumes'); + }); + after(() => { + removeOrphanedVolumes.restore(); + }); + + it('cleans up orphaned volumes through application-manager', async () => { + await actions.cleanupVolumes(); + expect(removeOrphanedVolumes).to.have.been.calledOnce; + }); +}); diff --git a/test/integration/device-api/v2.spec.ts b/test/integration/device-api/v2.spec.ts index 9610b8149..bd4117fb3 100644 --- a/test/integration/device-api/v2.spec.ts +++ b/test/integration/device-api/v2.spec.ts @@ -742,4 +742,31 @@ describe('device-api/v2', () => { .expect(500); }); }); + + describe('GET /v2/cleanup-volumes', () => { + // Actions are tested elsewhere so we can stub the dependency here + let cleanupVolumesStub: SinonStub; + before(() => { + cleanupVolumesStub = stub(actions, 'cleanupVolumes'); + }); + after(() => { + cleanupVolumesStub.restore(); + }); + + it('responds with 200', async () => { + cleanupVolumesStub.resolves(); + await request(api) + .get('/v2/cleanup-volumes') + .set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`) + .expect(200); + }); + + it('responds with 503 if an error occurred', async () => { + cleanupVolumesStub.throws(new Error()); + await request(api) + .get('/v2/cleanup-volumes') + .set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`) + .expect(503); + }); + }); }); From 231fd221a2720b38079208e3347cd97d5f7f3ec2 Mon Sep 17 00:00:00 2001 From: Christina Ying Wang Date: Tue, 5 Dec 2023 13:14:39 -0800 Subject: [PATCH 5/8] Separate routes & actions, add tests for POST /v2/journal-logs Signed-off-by: Christina Ying Wang --- src/device-api/actions.ts | 10 +++ src/device-api/v2.ts | 58 ++++++++--------- src/lib/journald.ts | 31 ++++++--- src/logging/monitor.ts | 4 +- test/integration/device-api/actions.spec.ts | 31 ++++++++- test/integration/device-api/v2.spec.ts | 33 ++++++++++ test/integration/lib/journald.spec.ts | 71 +++++++++++++++++++++ 7 files changed, 195 insertions(+), 43 deletions(-) create mode 100644 test/integration/lib/journald.spec.ts diff --git a/src/device-api/actions.ts b/src/device-api/actions.ts index e0d9ca06b..f1a1f4fde 100644 --- a/src/device-api/actions.ts +++ b/src/device-api/actions.ts @@ -27,6 +27,7 @@ import { NotFoundError, BadRequestError, } from '../lib/errors'; +import { JournalctlOpts, spawnJournalctl } from '../lib/journald'; /** * Run an array of healthchecks, outputting whether all passed or not @@ -464,3 +465,12 @@ export const cleanupVolumes = async ( withScope({ apps: [id] }), ); }; + +/** + * Spawn a journalctl process with the given options + * Used by: + * - POST /v2/journal-logs + */ +export const getLogStream = (opts: JournalctlOpts) => { + return spawnJournalctl(opts); +}; diff --git a/src/device-api/v2.ts b/src/device-api/v2.ts index 03e7bf5d0..2fd62d088 100644 --- a/src/device-api/v2.ts +++ b/src/device-api/v2.ts @@ -12,7 +12,6 @@ import * as db from '../db'; import * as logger from '../logger'; import * as images from '../compose/images'; import * as serviceManager from '../compose/service-manager'; -import { spawnJournalctl } from '../lib/journald'; import log from '../lib/supervisor-console'; import supervisorVersion = require('../lib/supervisor-version'); import { checkInt, checkString, checkTruthy } from '../lib/validation'; @@ -533,34 +532,31 @@ router.get('/v2/cleanup-volumes', async (req: AuthorizedRequest, res, next) => { } }); -router.post('/v2/journal-logs', (req, res) => { - const all = checkTruthy(req.body.all); - const follow = checkTruthy(req.body.follow); - const count = checkInt(req.body.count, { positive: true }) || undefined; - const unit = req.body.unit; - const format = req.body.format || 'short'; - const containerId = req.body.containerId; - const since = req.body.since; - const until = req.body.until; - - const journald = spawnJournalctl({ - all, - follow, - count, - unit, - format, - containerId, - since, - until, - }); - res.status(200); - // We know stdout will be present - journald.stdout!.pipe(res); - res.on('close', () => { - journald.kill('SIGKILL'); - }); - journald.on('exit', () => { - journald.stdout!.unpipe(); - res.end(); - }); +router.post('/v2/journal-logs', (req, res, next) => { + try { + const opts = { + all: checkTruthy(req.body.all), + follow: checkTruthy(req.body.follow), + count: checkInt(req.body.count, { positive: true }), + unit: req.body.unit, + format: req.body.format, + containerId: req.body.containerId, + since: req.body.since, + until: req.body.until, + }; + + const journalProcess = actions.getLogStream(opts); + res.status(200); + + journalProcess.stdout.pipe(res); + res.on('close', () => { + journalProcess.kill('SIGKILL'); + }); + journalProcess.on('exit', () => { + journalProcess.stdout.unpipe(); + res.end(); + }); + } catch (e: unknown) { + next(e); + } }); diff --git a/src/lib/journald.ts b/src/lib/journald.ts index 6fb2f46dc..e56792d23 100644 --- a/src/lib/journald.ts +++ b/src/lib/journald.ts @@ -1,6 +1,7 @@ import { ChildProcess, spawn } from 'child_process'; import log from './supervisor-console'; +import { Readable } from 'stream'; /** * Given a date integer in ms, return in a format acceptable by journalctl. @@ -13,17 +14,24 @@ import log from './supervisor-console'; export const toJournalDate = (timestamp: number): string => new Date(timestamp).toISOString().replace(/T/, ' ').replace(/\..+$/, ''); -export function spawnJournalctl(opts: { +export interface JournalctlOpts { all: boolean; follow: boolean; - count?: number | 'all'; unit?: string; containerId?: string; - format: string; - filterString?: string; + count?: number; since?: string; until?: string; -}): ChildProcess { + format?: string; + matches?: string; +} + +// A journalctl process has a non-null stdout +export interface JournalctlProcess extends ChildProcess { + stdout: Readable; +} + +export function spawnJournalctl(opts: JournalctlOpts): JournalctlProcess { const args: string[] = []; if (opts.all) { args.push('-a'); @@ -52,10 +60,15 @@ export function spawnJournalctl(opts: { args.push(opts.until); } args.push('-o'); - args.push(opts.format); - - if (opts.filterString) { - args.push(opts.filterString); + if (opts.format != null) { + args.push(opts.format); + } else { + args.push('short'); + } + // Filter logs by space-seperated matches per + // journalctl interface of `journalctl [OPTIONS..] [MATCHES..]` + if (opts.matches) { + args.push(opts.matches); } log.debug('Spawning journalctl', args.join(' ')); diff --git a/src/logging/monitor.ts b/src/logging/monitor.ts index 0df36d9dc..0b46c4055 100644 --- a/src/logging/monitor.ts +++ b/src/logging/monitor.ts @@ -65,7 +65,7 @@ class LogMonitor { all: true, follow: true, format: 'json', - filterString: '_SYSTEMD_UNIT=balena.service', + matches: '_SYSTEMD_UNIT=balena.service', }, (row) => { if (row.CONTAINER_ID_FULL && this.containers[row.CONTAINER_ID_FULL]) { @@ -148,7 +148,7 @@ class LogMonitor { all: true, follow: false, format: 'json', - filterString: `CONTAINER_ID_FULL=${containerId}`, + matches: `CONTAINER_ID_FULL=${containerId}`, since: toJournalDate(lastSentTimestamp + 1), // increment to exclude last sent log }, (row) => this.handleRow(row), diff --git a/test/integration/device-api/actions.spec.ts b/test/integration/device-api/actions.spec.ts index 13d09e2e8..31cbc9800 100644 --- a/test/integration/device-api/actions.spec.ts +++ b/test/integration/device-api/actions.spec.ts @@ -15,7 +15,8 @@ import * as TargetState from '~/src/device-state/target-state'; import * as applicationManager from '~/src/compose/application-manager'; import { cleanupDocker } from '~/test-lib/docker-helper'; -import { exec } from '~/src/lib/fs-utils'; +import { exec } from '~/lib/fs-utils'; +import * as journald from '~/lib/journald'; export async function dbusSend( dest: string, @@ -910,3 +911,31 @@ describe('cleans up orphaned volumes', () => { expect(removeOrphanedVolumes).to.have.been.calledOnce; }); }); + +describe('spawns a journal process', () => { + // This action simply calls spawnJournalctl which we test in + // journald.spec.ts, so we can just stub it here + let spawnJournalctlStub: SinonStub; + before(() => { + spawnJournalctlStub = stub(journald, 'spawnJournalctl'); + }); + after(() => { + spawnJournalctlStub.restore(); + }); + + it('spawns a journal process through journald', async () => { + const opts = { + all: true, + follow: true, + unit: 'test-unit', + containerId: 'test-container-id', + count: 10, + since: '2019-01-01 00:00:00', + until: '2019-01-01 01:00:00', + format: 'json', + matches: '_SYSTEMD_UNIT=test-unit', + }; + await actions.getLogStream(opts); + expect(spawnJournalctlStub).to.have.been.calledOnceWith(opts); + }); +}); diff --git a/test/integration/device-api/v2.spec.ts b/test/integration/device-api/v2.spec.ts index bd4117fb3..42000b997 100644 --- a/test/integration/device-api/v2.spec.ts +++ b/test/integration/device-api/v2.spec.ts @@ -769,4 +769,37 @@ describe('device-api/v2', () => { .expect(503); }); }); + + describe('POST /v2/journal-logs', () => { + // Actions are tested elsewhere so we can stub the dependency here + let getLogStreamStub: SinonStub; + before(() => { + getLogStreamStub = stub(actions, 'getLogStream'); + }); + after(() => { + getLogStreamStub.restore(); + }); + + it('responds with 200 and pipes journal stdout to response', async () => { + getLogStreamStub.callThrough(); + + await request(api) + .post('/v2/journal-logs') + .set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`) + .expect(200) + .then(({ text }) => { + // journalctl in the sut service should be empty + // as we don't log to it during testing + expect(text).to.equal('-- No entries --\n'); + }); + }); + + it('responds with 503 if an error occurred', async () => { + getLogStreamStub.throws(new Error()); + await request(api) + .post('/v2/journal-logs') + .set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`) + .expect(503); + }); + }); }); diff --git a/test/integration/lib/journald.spec.ts b/test/integration/lib/journald.spec.ts new file mode 100644 index 000000000..ff35dc881 --- /dev/null +++ b/test/integration/lib/journald.spec.ts @@ -0,0 +1,71 @@ +import { expect } from 'chai'; +import { ChildProcess } from 'child_process'; + +import { toJournalDate, spawnJournalctl } from '~/src/lib/journald'; + +describe('lib/journald', () => { + describe('toJournalDate', () => { + it('should convert a timestamp in ms to a journalctl date', () => { + const journalDate = toJournalDate( + new Date('2019-01-01T00:00:00.000Z').getTime(), + ); + expect(journalDate).to.equal('2019-01-01 00:00:00'); + }); + }); + + describe('spawnJournalctl', () => { + it('should spawn a journalctl process with defaults', () => { + const journalProcess = spawnJournalctl({ + all: false, + follow: false, + }); + + expect(journalProcess).to.have.property('stdout'); + expect(journalProcess).to.be.instanceOf(ChildProcess); + expect(journalProcess) + .to.have.property('spawnargs') + .that.deep.equals(['journalctl', '-o', 'short']); + + journalProcess.kill('SIGKILL'); + }); + + it('should spawn a journalctl process with valid options', () => { + const journalProcess = spawnJournalctl({ + all: true, + follow: true, + unit: 'test-unit', + containerId: 'test-container', + count: 10, + since: '2019-01-01 00:00:00', + until: '2019-01-02 00:00:00', + format: 'json', + matches: '_SYSTEMD_UNIT=test-unit', + }); + + expect(journalProcess).to.have.property('stdout'); + expect(journalProcess).to.be.instanceOf(ChildProcess); + expect(journalProcess) + .to.have.property('spawnargs') + .that.deep.equals([ + 'journalctl', + '-a', + '--follow', + '-u', + 'test-unit', + '-t', + 'test-container', + '-n', + '10', + '-S', + '2019-01-01 00:00:00', + '-U', + '2019-01-02 00:00:00', + '-o', + 'json', + '_SYSTEMD_UNIT=test-unit', + ]); + + journalProcess.kill('SIGKILL'); + }); + }); +}); From 9443281dec0ec8b6ca6a93b820f43eb07059ac35 Mon Sep 17 00:00:00 2001 From: Christina Ying Wang Date: Tue, 5 Dec 2023 14:04:34 -0800 Subject: [PATCH 6/8] Separate routes & actions, add tests for GET /v2/version Signed-off-by: Christina Ying Wang --- src/config/functions.ts | 3 +-- src/device-api/actions.ts | 10 +++++++++ src/device-api/v2.ts | 16 ++++++++------ src/lib/request.ts | 3 +-- src/lib/supervisor-version.ts | 13 ++++++------ src/supervisor.ts | 6 +++--- test/integration/device-api/v2.spec.ts | 27 ++++++++++++++++++++++++ test/legacy/10-api-binder.spec.ts | 2 +- test/unit/device-api/actions.spec.ts | 7 ++++++ test/unit/lib/contracts.spec.ts | 2 +- test/unit/lib/supervisor-version.spec.ts | 9 ++++++++ 11 files changed, 76 insertions(+), 22 deletions(-) create mode 100644 test/unit/lib/supervisor-version.spec.ts diff --git a/src/config/functions.ts b/src/config/functions.ts index af3330cfc..374500be9 100644 --- a/src/config/functions.ts +++ b/src/config/functions.ts @@ -1,14 +1,13 @@ import * as _ from 'lodash'; import * as memoizee from 'memoizee'; -import supervisorVersion = require('../lib/supervisor-version'); - import * as config from '.'; import * as constants from '../lib/constants'; import * as osRelease from '../lib/os-release'; import * as macAddress from '../lib/mac-address'; import * as hostUtils from '../lib/host-utils'; import log from '../lib/supervisor-console'; +import { supervisorVersion } from '../lib/supervisor-version'; export const fnSchema = { version: () => { diff --git a/src/device-api/actions.ts b/src/device-api/actions.ts index f1a1f4fde..c4300491b 100644 --- a/src/device-api/actions.ts +++ b/src/device-api/actions.ts @@ -28,6 +28,7 @@ import { BadRequestError, } from '../lib/errors'; import { JournalctlOpts, spawnJournalctl } from '../lib/journald'; +import { supervisorVersion } from '../lib/supervisor-version'; /** * Run an array of healthchecks, outputting whether all passed or not @@ -474,3 +475,12 @@ export const cleanupVolumes = async ( export const getLogStream = (opts: JournalctlOpts) => { return spawnJournalctl(opts); }; + +/** + * Get version of running Supervisor + * Used by: + * - GET /v2/version + */ +export const getSupervisorVersion = () => { + return supervisorVersion; +}; diff --git a/src/device-api/v2.ts b/src/device-api/v2.ts index 2fd62d088..4d85e1286 100644 --- a/src/device-api/v2.ts +++ b/src/device-api/v2.ts @@ -13,7 +13,6 @@ import * as logger from '../logger'; import * as images from '../compose/images'; import * as serviceManager from '../compose/service-manager'; import log from '../lib/supervisor-console'; -import supervisorVersion = require('../lib/supervisor-version'); import { checkInt, checkString, checkTruthy } from '../lib/validation'; import { isNotFoundError, @@ -378,11 +377,16 @@ router.get('/v2/local/logs', async (_req, res) => { listenStream.pipe(res); }); -router.get('/v2/version', (_req, res) => { - res.status(200).json({ - status: 'success', - version: supervisorVersion, - }); +router.get('/v2/version', (_req, res, next) => { + try { + const supervisorVersion = actions.getSupervisorVersion(); + return res.status(200).json({ + status: 'success', + version: supervisorVersion, + }); + } catch (e: unknown) { + next(e); + } }); router.get('/v2/containerId', async (req: AuthorizedRequest, res) => { diff --git a/src/lib/request.ts b/src/lib/request.ts index cbd7c4fb8..e5d883197 100644 --- a/src/lib/request.ts +++ b/src/lib/request.ts @@ -5,8 +5,7 @@ import * as resumableRequestLib from 'resumable-request'; import * as constants from './constants'; import * as osRelease from './os-release'; - -import supervisorVersion = require('./supervisor-version'); +import { supervisorVersion } from './supervisor-version'; export { requestLib }; diff --git a/src/lib/supervisor-version.ts b/src/lib/supervisor-version.ts index 6d95b7972..fd1e3f020 100644 --- a/src/lib/supervisor-version.ts +++ b/src/lib/supervisor-version.ts @@ -1,10 +1,9 @@ -import * as _ from 'lodash'; - -import * as packageJson from '../../package.json'; -let version = packageJson.version; +import { version as packageJsonVersion } from '../../package.json'; +let supervisorVersion = packageJsonVersion; const tagExtra = process.env.SUPERVISOR_TAG_EXTRA; -if (!_.isEmpty(tagExtra)) { - version += '+' + tagExtra; +if (tagExtra != null) { + supervisorVersion += '+' + tagExtra; } -export = version; + +export { supervisorVersion }; diff --git a/src/supervisor.ts b/src/supervisor.ts index 03d5becfc..9f8c1ba41 100644 --- a/src/supervisor.ts +++ b/src/supervisor.ts @@ -12,7 +12,7 @@ import { initializeContractRequirements } from './lib/contracts'; import { normaliseLegacyDatabase } from './lib/legacy'; import * as osRelease from './lib/os-release'; import log from './lib/supervisor-console'; -import version = require('./lib/supervisor-version'); +import { supervisorVersion } from './lib/supervisor-version'; import * as avahi from './lib/avahi'; import * as firewall from './lib/firewall'; @@ -32,7 +32,7 @@ export class Supervisor { private api: SupervisorAPI; public async init() { - log.info(`Supervisor v${version} starting up...`); + log.info(`Supervisor v${supervisorVersion} starting up...`); await db.initialized(); await config.initialized(); @@ -43,7 +43,7 @@ export class Supervisor { const conf = await config.getMany(startupConfigFields); initializeContractRequirements({ - supervisorVersion: version, + supervisorVersion, deviceType: await config.get('deviceType'), deviceArch: await config.get('deviceArch'), l4tVersion: await osRelease.getL4tVersion(), diff --git a/test/integration/device-api/v2.spec.ts b/test/integration/device-api/v2.spec.ts index 42000b997..0dff7a3ef 100644 --- a/test/integration/device-api/v2.spec.ts +++ b/test/integration/device-api/v2.spec.ts @@ -13,6 +13,7 @@ import { NotFoundError, BadRequestError, } from '~/lib/errors'; +import { supervisorVersion } from '~/src/lib/supervisor-version'; // All routes that require Authorization are integration tests due to // the api-key module relying on the database. @@ -802,4 +803,30 @@ describe('device-api/v2', () => { .expect(503); }); }); + + describe('GET /v2/version', () => { + let getSupervisorVersionStub: SinonStub; + before(() => { + getSupervisorVersionStub = stub(actions, 'getSupervisorVersion'); + }); + after(() => { + getSupervisorVersionStub.restore(); + }); + + it('responds with 200 and Supervisor version', async () => { + getSupervisorVersionStub.callThrough(); + await request(api) + .get('/v2/version') + .set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`) + .expect(200, { status: 'success', version: supervisorVersion }); + }); + + it('responds with 503 if an error occurred', async () => { + getSupervisorVersionStub.throws(new Error()); + await request(api) + .get('/v2/version') + .set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`) + .expect(503); + }); + }); }); diff --git a/test/legacy/10-api-binder.spec.ts b/test/legacy/10-api-binder.spec.ts index 758d0ef47..b33669a9e 100644 --- a/test/legacy/10-api-binder.spec.ts +++ b/test/legacy/10-api-binder.spec.ts @@ -13,7 +13,7 @@ import { schema } from '~/src/config/schema'; import ConfigJsonConfigBackend from '~/src/config/configJson'; import * as TargetState from '~/src/device-state/target-state'; import * as ApiHelper from '~/lib/api-helper'; -import supervisorVersion = require('~/lib/supervisor-version'); +import { supervisorVersion } from '~/lib/supervisor-version'; import * as eventTracker from '~/src/event-tracker'; import * as constants from '~/lib/constants'; diff --git a/test/unit/device-api/actions.spec.ts b/test/unit/device-api/actions.spec.ts index 8a846faee..45cf7c8b8 100644 --- a/test/unit/device-api/actions.spec.ts +++ b/test/unit/device-api/actions.spec.ts @@ -3,6 +3,7 @@ import { spy, useFakeTimers, stub, SinonStub } from 'sinon'; import * as hostConfig from '~/src/host-config'; import * as actions from '~/src/device-api/actions'; +import { supervisorVersion } from '~/lib/supervisor-version'; import blink = require('~/lib/blink'); describe('device-api/actions', () => { @@ -67,4 +68,10 @@ describe('device-api/actions', () => { expect(await actions.getHostConfig()).to.deep.equal(conf); }); }); + + describe('gets Supervisor version', () => { + it('gets Supervisor version from package.json', () => { + expect(actions.getSupervisorVersion()).to.equal(supervisorVersion); + }); + }); }); diff --git a/test/unit/lib/contracts.spec.ts b/test/unit/lib/contracts.spec.ts index ee50b649b..594ee5843 100644 --- a/test/unit/lib/contracts.spec.ts +++ b/test/unit/lib/contracts.spec.ts @@ -3,7 +3,7 @@ import * as semver from 'semver'; import { SinonStub, stub } from 'sinon'; import * as osRelease from '~/lib/os-release'; -import supervisorVersion = require('~/lib/supervisor-version'); +import { supervisorVersion } from '~/lib/supervisor-version'; import * as fsUtils from '~/lib/fs-utils'; describe('lib/contracts', () => { diff --git a/test/unit/lib/supervisor-version.spec.ts b/test/unit/lib/supervisor-version.spec.ts new file mode 100644 index 000000000..da7ea62ad --- /dev/null +++ b/test/unit/lib/supervisor-version.spec.ts @@ -0,0 +1,9 @@ +import { expect } from 'chai'; +import { version as packageJsonVersion } from '~/src/../package.json'; +import { supervisorVersion } from '~/lib/supervisor-version'; + +describe('lib/supervisor-version', () => { + it('should return the version from package.json', () => { + expect(supervisorVersion).to.equal(packageJsonVersion); + }); +}); From e9535cd0ffba1d53f7d69cbc333b9fbc4bb1e320 Mon Sep 17 00:00:00 2001 From: Christina Ying Wang Date: Tue, 5 Dec 2023 15:56:21 -0800 Subject: [PATCH 7/8] Separate routes & actions, add tests for GET /v2/containerId Signed-off-by: Christina Ying Wang --- src/compose/application-manager.ts | 6 ++ src/device-api/actions.ts | 34 ++++++++++ src/device-api/v2.ts | 49 ++++++++------ .../compose/application-manager.spec.ts | 49 ++++++++++++++ test/integration/device-api/actions.spec.ts | 53 +++++++++++++++ test/integration/device-api/v2.spec.ts | 67 +++++++++++++++++++ 6 files changed, 236 insertions(+), 22 deletions(-) diff --git a/src/compose/application-manager.ts b/src/compose/application-manager.ts index 387166931..28baa135c 100644 --- a/src/compose/application-manager.ts +++ b/src/compose/application-manager.ts @@ -1050,3 +1050,9 @@ export async function removeOrphanedVolumes( await volumeManager.removeOrphanedVolumes(referencedVolumes); } + +export async function getAllServices( + inScope: (id: number) => boolean = () => true, +) { + return (await serviceManager.getAll()).filter((svc) => inScope(svc.appId)); +} diff --git a/src/device-api/actions.ts b/src/device-api/actions.ts index c4300491b..ee1aa7656 100644 --- a/src/device-api/actions.ts +++ b/src/device-api/actions.ts @@ -484,3 +484,37 @@ export const getLogStream = (opts: JournalctlOpts) => { export const getSupervisorVersion = () => { return supervisorVersion; }; + +/** + * Get the containerId(s) associated with a service. + * If no serviceName is provided, get all containerIds. + * Used by: + * - GET /v2/containerId + */ +export const getContainerIds = async ( + serviceName: string = '', + withScope: AuthorizedRequest['auth']['isScoped'] = () => true, +) => { + const services = await applicationManager.getAllServices((id) => + withScope({ apps: [id] }), + ); + + // Return all containerIds if no serviceName is provided + if (!serviceName) { + return services.reduce( + (svcToContainerIdMap, svc) => ({ + [svc.serviceName]: svc.containerId, + ...svcToContainerIdMap, + }), + {}, + ); + } + + // Otherwise, only return containerId of provided serviceNmae + const service = services.find((svc) => svc.serviceName === serviceName); + if (service != null) { + return service.containerId; + } else { + throw new Error(`Could not find service with name '${serviceName}'`); + } +}; diff --git a/src/device-api/v2.ts b/src/device-api/v2.ts index 4d85e1286..200c92e5b 100644 --- a/src/device-api/v2.ts +++ b/src/device-api/v2.ts @@ -389,33 +389,38 @@ router.get('/v2/version', (_req, res, next) => { } }); -router.get('/v2/containerId', async (req: AuthorizedRequest, res) => { - const services = (await serviceManager.getAll()).filter((service) => - req.auth.isScoped({ apps: [service.appId] }), - ); - - if (req.query.serviceName != null || req.query.service != null) { - const serviceName = req.query.serviceName || req.query.service; - const service = _.find(services, (svc) => svc.serviceName === serviceName); - if (service != null) { - res.status(200).json({ +router.get('/v2/containerId', async (req: AuthorizedRequest, res, next) => { + try { + // While technically query parameters support entering a query multiple times + // as in ?service=foo&service=bar, we don't explicitly support this, + // so only pass the serviceName / service if it's a string + let serviceQuery: string = ''; + const { serviceName, service } = req.query; + if (typeof serviceName === 'string' && !!serviceName) { + serviceQuery = serviceName; + } + if (typeof service === 'string' && !!service) { + serviceQuery = service; + } + const result = await actions.getContainerIds( + serviceQuery, + req.auth.isScoped, + ); + // Single containerId + if (typeof result === 'string') { + return res.status(200).json({ status: 'success', - containerId: service.containerId, + containerId: result, }); } else { - res.status(503).json({ - status: 'failed', - message: 'Could not find service with that name', + // Multiple containerIds + return res.status(200).json({ + status: 'success', + services: result, }); } - } else { - res.status(200).json({ - status: 'success', - services: _(services) - .keyBy('serviceName') - .mapValues('containerId') - .value(), - }); + } catch (e: unknown) { + next(e); } }); diff --git a/test/integration/compose/application-manager.spec.ts b/test/integration/compose/application-manager.spec.ts index 7683a0049..a4cfd2db1 100644 --- a/test/integration/compose/application-manager.spec.ts +++ b/test/integration/compose/application-manager.spec.ts @@ -7,6 +7,7 @@ import * as serviceManager from '~/src/compose/service-manager'; import Network from '~/src/compose/network'; import * as networkManager from '~/src/compose/network-manager'; import Volume from '~/src/compose/volume'; +import Service from '~/src/compose/service'; import * as config from '~/src/config'; import { createDockerImage } from '~/test-lib/docker-helper'; import { @@ -1600,4 +1601,52 @@ describe('compose/application-manager', () => { expect(steps).to.have.lengthOf(0); }); }); + + describe('getting all services', () => { + let getAllServices: sinon.SinonStub; + let testServices: Service[]; + before(async () => { + testServices = [ + await createService( + { + serviceName: 'one', + appId: 1, + }, + { state: { containerId: 'abc' } }, + ), + await createService( + { + serviceName: 'two', + appId: 2, + }, + { state: { containerId: 'def' } }, + ), + ]; + getAllServices = sinon + .stub(serviceManager, 'getAll') + .resolves(testServices); + }); + + after(() => { + getAllServices.restore(); + }); + + it('should get all services by default', async () => { + expect(await applicationManager.getAllServices()).to.deep.equal( + testServices, + ); + }); + + it('should get services scoped by appId', async () => { + expect( + await applicationManager.getAllServices((appId) => appId === 1), + ).to.deep.equal([testServices[0]]); + expect( + await applicationManager.getAllServices((appId) => appId === 2), + ).to.deep.equal([testServices[1]]); + expect( + await applicationManager.getAllServices((appId) => appId === 3), + ).to.deep.equal([]); + }); + }); }); diff --git a/test/integration/device-api/actions.spec.ts b/test/integration/device-api/actions.spec.ts index 31cbc9800..f4e0da70f 100644 --- a/test/integration/device-api/actions.spec.ts +++ b/test/integration/device-api/actions.spec.ts @@ -14,6 +14,7 @@ import * as actions from '~/src/device-api/actions'; import * as TargetState from '~/src/device-state/target-state'; import * as applicationManager from '~/src/compose/application-manager'; import { cleanupDocker } from '~/test-lib/docker-helper'; +import { createService } from '~/test-lib/state-helper'; import { exec } from '~/lib/fs-utils'; import * as journald from '~/lib/journald'; @@ -939,3 +940,55 @@ describe('spawns a journal process', () => { expect(spawnJournalctlStub).to.have.been.calledOnceWith(opts); }); }); + +describe('gets service container ids', () => { + // getAllServicesStub is tested in app manager tests + // so we can stub it here + let getAllServicesStub: SinonStub; + before(async () => { + getAllServicesStub = stub(applicationManager, 'getAllServices').resolves([ + await createService( + { + serviceName: 'one', + appId: 1, + }, + { state: { containerId: 'abc' } }, + ), + await createService( + { + serviceName: 'two', + appId: 2, + }, + { state: { containerId: 'def' } }, + ), + ]); + }); + after(() => { + getAllServicesStub.restore(); + }); + + it('gets all containerIds by default', async () => { + expect(await actions.getContainerIds()).to.deep.equal({ + one: 'abc', + two: 'def', + }); + }); + + it('gets a single containerId associated with provided service', async () => { + expect(await actions.getContainerIds('one')).to.deep.equal('abc'); + expect(await actions.getContainerIds('two')).to.deep.equal('def'); + }); + + it('errors if no containerId found associated with provided service', async () => { + try { + await actions.getContainerIds('three'); + expect.fail( + 'getContainerIds should throw for a nonexistent serviceName parameter', + ); + } catch (e: unknown) { + expect((e as Error).message).to.equal( + "Could not find service with name 'three'", + ); + } + }); +}); diff --git a/test/integration/device-api/v2.spec.ts b/test/integration/device-api/v2.spec.ts index 0dff7a3ef..57ab5b80d 100644 --- a/test/integration/device-api/v2.spec.ts +++ b/test/integration/device-api/v2.spec.ts @@ -829,4 +829,71 @@ describe('device-api/v2', () => { .expect(503); }); }); + + describe('GET /v2/containerId', () => { + let getContainerIdStub: SinonStub; + beforeEach(() => { + getContainerIdStub = stub(actions, 'getContainerIds'); + }); + afterEach(() => { + getContainerIdStub.restore(); + }); + + it('accepts query parameters if they are strings', async () => { + getContainerIdStub.resolves('test'); + await request(api) + .get('/v2/containerId?serviceName=one') + .set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`) + .expect(200, { status: 'success', containerId: 'test' }); + expect(getContainerIdStub.firstCall.args[0]).to.equal('one'); + + await request(api) + .get('/v2/containerId?service=two') + .set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`) + .expect(200, { status: 'success', containerId: 'test' }); + expect(getContainerIdStub.secondCall.args[0]).to.equal('two'); + }); + + it('ignores query parameters that are repeated', async () => { + getContainerIdStub.resolves('test'); + await request(api) + .get('/v2/containerId?serviceName=one&serviceName=two') + .set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`) + .expect(200, { status: 'success', containerId: 'test' }); + expect(getContainerIdStub.firstCall.args[0]).to.equal(''); + + await request(api) + .get('/v2/containerId?service=one&service=two') + .set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`) + .expect(200, { status: 'success', containerId: 'test' }); + expect(getContainerIdStub.secondCall.args[0]).to.equal(''); + }); + + it('responds with 200 and single containerId', async () => { + getContainerIdStub.resolves('test'); + await request(api) + .get('/v2/containerId') + .set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`) + .expect(200, { status: 'success', containerId: 'test' }); + }); + + it('responds with 200 and multiple containerIds', async () => { + getContainerIdStub.resolves({ one: 'abc', two: 'def' }); + await request(api) + .get('/v2/containerId') + .set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`) + .expect(200, { + status: 'success', + services: { one: 'abc', two: 'def' }, + }); + }); + + it('responds with 503 if an error occurred', async () => { + getContainerIdStub.throws(new Error()); + await request(api) + .get('/v2/containerId') + .set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`) + .expect(503); + }); + }); }); From 8112e51f4e8a1c09fa492771f35a266da047bb65 Mon Sep 17 00:00:00 2001 From: Christina Ying Wang Date: Wed, 6 Dec 2023 14:15:34 -0800 Subject: [PATCH 8/8] Separate routes & actions, add tests for GET /v2/local/device-info Signed-off-by: Christina Ying Wang --- src/device-api/actions.ts | 9 ++++++ src/device-api/v2.ts | 11 +++---- test/integration/device-api/actions.spec.ts | 22 +++++++++++++ test/integration/device-api/v2.spec.ts | 35 +++++++++++++++++++++ 4 files changed, 70 insertions(+), 7 deletions(-) diff --git a/src/device-api/actions.ts b/src/device-api/actions.ts index ee1aa7656..fd000f0e1 100644 --- a/src/device-api/actions.ts +++ b/src/device-api/actions.ts @@ -518,3 +518,12 @@ export const getContainerIds = async ( throw new Error(`Could not find service with name '${serviceName}'`); } }; + +/** + * Get device type & arch + * Used by: + * - GET /v2/local/device-info + */ +export const getDeviceInfo = async () => { + return await config.getMany(['deviceType', 'deviceArch']); +}; diff --git a/src/device-api/v2.ts b/src/device-api/v2.ts index 200c92e5b..9e85b8d9f 100644 --- a/src/device-api/v2.ts +++ b/src/device-api/v2.ts @@ -330,10 +330,7 @@ router.post('/v2/local/target-state', async (req, res) => { router.get('/v2/local/device-info', async (_req, res) => { try { - const { deviceType, deviceArch } = await config.getMany([ - 'deviceType', - 'deviceArch', - ]); + const { deviceType, deviceArch } = await actions.getDeviceInfo(); return res.status(200).json({ status: 'success', @@ -342,10 +339,10 @@ router.get('/v2/local/device-info', async (_req, res) => { deviceType, }, }); - } catch (e: any) { - res.status(500).json({ + } catch (e: unknown) { + return res.status(500).json({ status: 'failed', - message: e.message, + message: (e as Error).message ?? e, }); } }); diff --git a/test/integration/device-api/actions.spec.ts b/test/integration/device-api/actions.spec.ts index f4e0da70f..81fd11308 100644 --- a/test/integration/device-api/actions.spec.ts +++ b/test/integration/device-api/actions.spec.ts @@ -992,3 +992,25 @@ describe('gets service container ids', () => { } }); }); + +describe('gets device type and arch', () => { + let configGetManyStub: SinonStub; + before(() => { + // @ts-expect-error + configGetManyStub = stub(config, 'getMany').resolves({ + deviceType: 'test-type', + deviceArch: 'test-arch', + }); + }); + + after(() => { + configGetManyStub.restore(); + }); + + it('returns device type and arch', async () => { + expect(await actions.getDeviceInfo()).to.deep.equal({ + deviceType: 'test-type', + deviceArch: 'test-arch', + }); + }); +}); diff --git a/test/integration/device-api/v2.spec.ts b/test/integration/device-api/v2.spec.ts index 57ab5b80d..3500ecca3 100644 --- a/test/integration/device-api/v2.spec.ts +++ b/test/integration/device-api/v2.spec.ts @@ -896,4 +896,39 @@ describe('device-api/v2', () => { .expect(503); }); }); + + describe('GET /v2/local/device-info', () => { + let getDeviceInfoStub: SinonStub; + beforeEach(() => { + getDeviceInfoStub = stub(actions, 'getDeviceInfo'); + }); + afterEach(() => { + getDeviceInfoStub.restore(); + }); + + it('responds with 200 and device info', async () => { + getDeviceInfoStub.resolves({ + deviceArch: 'aarch64', + deviceType: 'raspberrypi4-64', + }); + await request(api) + .get('/v2/local/device-info') + .set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`) + .expect(200, { + status: 'success', + info: { + arch: 'aarch64', + deviceType: 'raspberrypi4-64', + }, + }); + }); + + it('responds with 500 if an error occurred', async () => { + getDeviceInfoStub.throws(new Error()); + await request(api) + .get('/v2/local/device-info') + .set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`) + .expect(500); + }); + }); });