From 4f446103f4b9256118434613a9f3c841eeb0a6f3 Mon Sep 17 00:00:00 2001 From: Christina Wang Date: Wed, 9 Feb 2022 02:41:13 +0000 Subject: [PATCH 1/2] Remove lockingIfNecessary in favor of updateLock.lock The functionality is pretty much the same, so we don't need the two functions in two different places. Signed-off-by: Christina Wang --- src/compose/application-manager.ts | 24 ++------- src/device-api/common.js | 13 ++--- src/lib/update-lock.ts | 82 +++++++++++++++++++----------- 3 files changed, 62 insertions(+), 57 deletions(-) diff --git a/src/compose/application-manager.ts b/src/compose/application-manager.ts index 2362e77b..844b0386 100644 --- a/src/compose/application-manager.ts +++ b/src/compose/application-manager.ts @@ -1,5 +1,6 @@ import * as express from 'express'; import * as _ from 'lodash'; +import StrictEventEmitter from 'strict-event-emitter-types'; import * as config from '../config'; import { transaction, Transaction } from '../db'; @@ -14,7 +15,7 @@ import { ContractViolationError, InternalInconsistencyError, } from '../lib/errors'; -import StrictEventEmitter from 'strict-event-emitter-types'; +import { lock } from '../lib/update-lock'; import App from './app'; import * as volumeManager from './volume-manager'; @@ -39,7 +40,6 @@ import { } from '../types/state'; import { checkTruthy, checkInt } from '../lib/validation'; import { Proxyvisor } from '../proxyvisor'; -import * as updateLock from '../lib/update-lock'; import { EventEmitter } from 'events'; type ApplicationManagerEventEmitter = StrictEventEmitter< @@ -90,7 +90,7 @@ export function resetTimeSpentFetching(value: number = 0) { } const actionExecutors = getExecutors({ - lockFn: lockingIfNecessary, + lockFn: lock, callbacks: { containerStarted: (id: string) => { containerStarted[id] = true; @@ -157,22 +157,6 @@ function reportCurrentState(data?: Partial) { events.emit('change', data ?? {}); } -export async function lockingIfNecessary( - appId: number, - { force = false, skipLock = false } = {}, - fn: () => Resolvable, -) { - if (skipLock) { - return fn(); - } - const lockOverride = (await config.get('lockOverride')) || force; - return updateLock.lock( - appId, - { force: lockOverride }, - fn as () => PromiseLike, - ); -} - export async function getRequiredSteps( targetApps: InstancedAppState, ignoreImages: boolean = false, @@ -359,7 +343,7 @@ export async function stopAll({ force = false, skipLock = false } = {}) { const services = await serviceManager.getAll(); await Promise.all( services.map(async (s) => { - return lockingIfNecessary(s.appId, { force, skipLock }, async () => { + return lock(s.appId, { force, skipLock }, async () => { await serviceManager.kill(s, { removeContainer: false, wait: true }); if (s.containerId) { delete containerStarted[s.containerId]; diff --git a/src/device-api/common.js b/src/device-api/common.js index 3f05e76f..bf12d542 100644 --- a/src/device-api/common.js +++ b/src/device-api/common.js @@ -1,21 +1,20 @@ import * as Bluebird from 'bluebird'; import * as _ from 'lodash'; -import { appNotFoundMessage } from '../lib/messages'; -import * as logger from '../logger'; +import * as logger from '../logger'; import * as deviceState from '../device-state'; import * as applicationManager from '../compose/application-manager'; import * as serviceManager from '../compose/service-manager'; import * as volumeManager from '../compose/volume-manager'; import { InternalInconsistencyError } from '../lib/errors'; +import { lock } from '../lib/update-lock'; +import { appNotFoundMessage } from '../lib/messages'; export async function doRestart(appId, force) { await deviceState.initialized; await applicationManager.initialized; - const { lockingIfNecessary } = applicationManager; - - return lockingIfNecessary(appId, { force }, () => + return lock(appId, { force }, () => deviceState.getCurrentState().then(function (currentState) { if (currentState.local.apps?.[appId] == null) { throw new InternalInconsistencyError( @@ -42,14 +41,12 @@ export async function doPurge(appId, force) { await deviceState.initialized; await applicationManager.initialized; - const { lockingIfNecessary } = applicationManager; - logger.logSystemMessage( `Purging data for app ${appId}`, { appId }, 'Purge data', ); - return lockingIfNecessary(appId, { force }, () => + return lock(appId, { force }, () => deviceState.getCurrentState().then(function (currentState) { const allApps = currentState.local.apps; diff --git a/src/lib/update-lock.ts b/src/lib/update-lock.ts index db1d24b2..2ac8d236 100644 --- a/src/lib/update-lock.ts +++ b/src/lib/update-lock.ts @@ -6,8 +6,14 @@ import * as path from 'path'; import * as Lock from 'rwlock'; import * as constants from './constants'; -import { ENOENT, EEXIST, UpdatesLockedError } from './errors'; +import { + ENOENT, + EEXIST, + UpdatesLockedError, + InternalInconsistencyError, +} from './errors'; import { getPathOnHost, pathExistsOnHost } from './fs-utils'; +import * as config from '../config'; type asyncLockFile = typeof lockFileLib & { unlockAsync(path: string): Bluebird; @@ -115,47 +121,65 @@ const lockExistsErrHandler = (err: Error, release: () => void) => { /** * Try to take the locks for an application. If force is set, it will remove * all existing lockfiles before performing the operation + * + * TODO: convert to native Promises. May require native implementation of Bluebird's dispose / using + * + * TODO: Remove skipLock as it's not a good interface. If lock is called it should try to take the lock + * without an option to skip. */ -export function lock( +export function lock( appId: number | null, - { force = false }: { force: boolean }, - fn: () => PromiseLike, -): Bluebird { + { force = false, skipLock = false }: { force: boolean; skipLock?: boolean }, + fn: () => Resolvable, +): Bluebird { + if (skipLock) { + return Bluebird.resolve(fn()); + } + const takeTheLock = () => { if (appId == null) { return; } - return writeLock(appId) - .tap((release: () => void) => { - const [lockDir] = getPathOnHost(lockPath(appId)); + return config + .get('lockOverride') + .then((lockOverride) => { + return writeLock(appId) + .tap((release: () => void) => { + const [lockDir] = getPathOnHost(lockPath(appId)); - return Bluebird.resolve(fs.readdir(lockDir)) - .catchReturn(ENOENT, []) - .mapSeries((serviceName) => { - return Bluebird.mapSeries( - lockFilesOnHost(appId, serviceName), - (tmpLockName) => { - return Bluebird.try(() => { - if (force) { - return lockFile.unlockAsync(tmpLockName); - } - }) - .then(() => lockFile.lockAsync(tmpLockName)) - .then(() => { - locksTaken[tmpLockName] = true; - }) - .catchReturn(ENOENT, undefined); - }, - ); + return Bluebird.resolve(fs.readdir(lockDir)) + .catchReturn(ENOENT, []) + .mapSeries((serviceName) => { + return Bluebird.mapSeries( + lockFilesOnHost(appId, serviceName), + (tmpLockName) => { + return Bluebird.try(() => { + if (force || lockOverride) { + return lockFile.unlockAsync(tmpLockName); + } + }) + .then(() => lockFile.lockAsync(tmpLockName)) + .then(() => { + locksTaken[tmpLockName] = true; + }) + .catchReturn(ENOENT, undefined); + }, + ); + }) + .catch((err) => lockExistsErrHandler(err, release)); }) - .catch((err) => lockExistsErrHandler(err, release)); + .disposer(dispose); }) - .disposer(dispose); + .catch((err) => { + throw new InternalInconsistencyError( + `Error getting lockOverride config value: ${err?.message ?? err}`, + ); + }); }; const disposer = takeTheLock(); if (disposer) { - return Bluebird.using(disposer, fn); + return Bluebird.using(disposer, fn as () => PromiseLike); } else { return Bluebird.resolve(fn()); } From 5f1a77da25b9d0bd07c2fbec85cb5deefae18884 Mon Sep 17 00:00:00 2001 From: Christina Wang Date: Wed, 9 Feb 2022 03:05:40 +0000 Subject: [PATCH 2/2] Add update lock check to PATCH /v1/device/host-config This is necessary with the changes as of balenaOS 2.82.6, which watches config.json and will restart balena-hostname and some other services automatically on file change. Change-type: patch Relates-to: #1876 Signed-off-by: Christina Wang --- docs/API.md | 15 +++++----- src/device-state.ts | 13 ++++++-- src/host-config.ts | 30 ++++++++++++------- test/41-device-api-v1.spec.ts | 56 +++++++++++++++++++++++++++++++++++ 4 files changed, 94 insertions(+), 20 deletions(-) diff --git a/docs/API.md b/docs/API.md index 30e07614..240b01cb 100644 --- a/docs/API.md +++ b/docs/API.md @@ -548,16 +548,16 @@ $ curl -X POST --header "Content-Type:application/json" \ > **Note:** on devices with supervisor version lower than 7.22.0, replace all `BALENA_` variables with `RESIN_`, e.g. `RESIN_SUPERVISOR_ADDRESS` instead of `BALENA_SUPERVISOR_ADDRESS`. -This endpoint allows setting some configuration values for the host OS. Currently it supports -proxy and hostname configuration. +This endpoint allows setting some configuration values for the host OS. Currently it supports proxy and hostname configuration. For proxy configuration, balenaOS 2.0.7 and higher provides a transparent proxy redirector (redsocks) that makes all connections be routed to a SOCKS or HTTP proxy. This endpoint allows services to modify these proxy settings at runtime. #### Request body -Is a JSON object with several optional fields. Proxy and hostname configuration go under a "network" key. If "proxy" or "hostname" are not present (undefined), those values will not be modified, so that a request can modify hostname -without changing proxy settings and viceversa. +A JSON object with several optional fields. Proxy and hostname configuration go under a `network` key. If `proxy` or `hostname` are not present (undefined), those values will not be modified, so that a request can modify hostname without changing proxy settings and vice versa. + +By default, with [balenaOS 2.82.6](https://github.com/balena-os/meta-balena/blob/master/CHANGELOG.md#v2826) or newer, host config PATCH requests will cause a balenaEngine restart. Therefore, with Supervisor v12.11.34 and newer, the PATCH request will respect the presence of update locks. Specify the `force` boolean (`false` by default) property in the request body to ignore this. ```json { @@ -570,7 +570,8 @@ without changing proxy settings and viceversa. "password": "password", "noProxy": [ "152.10.30.4", "253.1.1.0/16" ] }, - "hostname": "mynewhostname" + "hostname": "mynewhostname", + "force": true } } ``` @@ -581,10 +582,10 @@ guaranteed to work, especially if they block connections that the balena service Keep in mind that, even if transparent proxy redirection will take effect immediately after the API call (i.e. all new connections will go through the proxy), open connections will not be closed. So, if for example, the device has managed to connect to the balenaCloud VPN without the proxy, it will stay connected directly without trying to reconnect through the proxy, unless the connection breaks - any reconnection attempts will then go through the proxy. To force *all* connections to go through the proxy, the best way is to reboot the device (see the /v1/reboot endpoint). In most networks were no connections to the Internet can be made if not through a proxy, this should not be necessary (as there will be no open connections before configuring the proxy settings). -The "noProxy" setting for the proxy is an optional array of IP addresses/subnets that should not be routed through the +The `noProxy` setting for the proxy is an optional array of IP addresses/subnets that should not be routed through the proxy. Keep in mind that local/reserved subnets are already [excluded by balenaOS automatically](https://github.com/balena-os/meta-balena/blob/master/meta-balena-common/recipes-connectivity/balena-proxy-config/balena-proxy-config/balena-proxy-config). -If either "proxy" or "hostname" are null or empty values (i.e. `{}` for proxy or an empty string for hostname), they will be cleared to their default values (i.e. not using a proxy, and a hostname equal to the first 7 characters of the device's uuid, respectively). +If either `proxy` or `hostname` are null or empty values (i.e. `{}` for proxy or an empty string for hostname), they will be cleared to their default values (i.e. not using a proxy, and a hostname equal to the first 7 characters of the device's uuid, respectively). #### Examples: From an app container: diff --git a/src/device-state.ts b/src/device-state.ts index c695e29a..33ac3960 100644 --- a/src/device-state.ts +++ b/src/device-state.ts @@ -147,10 +147,19 @@ function createDeviceStateRouter() { const uuid = await config.get('uuid'); req.body.network.hostname = uuid?.slice(0, 7); } - - await hostConfig.patch(req.body); + const lockOverride = await config.get('lockOverride'); + await hostConfig.patch( + req.body, + validation.checkTruthy(req.body.force) || lockOverride, + ); res.status(200).send('OK'); } catch (err) { + // TODO: We should be able to throw err if it's UpdatesLockedError + // and the error middleware will handle it, but this doesn't work in + // the test environment. Fix this when fixing API tests. + if (err instanceof UpdatesLockedError) { + return res.status(423).send(err?.message ?? err); + } res.status(503).send(err?.message ?? err ?? 'Unknown error'); } }); diff --git a/src/host-config.ts b/src/host-config.ts index 94fcfe96..6706c245 100644 --- a/src/host-config.ts +++ b/src/host-config.ts @@ -7,8 +7,9 @@ import * as path from 'path'; import * as config from './config'; import * as constants from './lib/constants'; import * as dbus from './lib/dbus'; -import { ENOENT } from './lib/errors'; +import { ENOENT, InternalInconsistencyError } from './lib/errors'; import { writeFileAtomic, mkdirp, unlinkAll } from './lib/fs-utils'; +import * as updateLock from './lib/update-lock'; const redsocksHeader = stripIndent` base { @@ -214,15 +215,22 @@ export function get(): Bluebird { }); } -export function patch(conf: HostConfig): Bluebird { - const promises: Array> = []; - if (conf != null && conf.network != null) { - if (conf.network.proxy != null) { - promises.push(setProxy(conf.network.proxy)); - } - if (conf.network.hostname != null) { - promises.push(setHostname(conf.network.hostname)); - } +export async function patch(conf: HostConfig, force: boolean): Promise { + const appId = await config.get('applicationId'); + if (!appId) { + throw new InternalInconsistencyError('Could not find an appId'); } - return Bluebird.all(promises).return(); + + return updateLock.lock(appId, { force }, () => { + const promises: Array> = []; + if (conf != null && conf.network != null) { + if (conf.network.proxy != null) { + promises.push(setProxy(conf.network.proxy)); + } + if (conf.network.hostname != null) { + promises.push(setHostname(conf.network.hostname)); + } + } + return Bluebird.all(promises).return(); + }); } diff --git a/test/41-device-api-v1.spec.ts b/test/41-device-api-v1.spec.ts index 6de1e999..7bfff8e2 100644 --- a/test/41-device-api-v1.spec.ts +++ b/test/41-device-api-v1.spec.ts @@ -900,6 +900,62 @@ describe('SupervisorAPI [V1 Endpoints]', () => { restartServiceSpy.restore(); }); + it('prevents patch if update locks are present', async () => { + stub(updateLock, 'lock').callsFake((__, opts, fn) => { + if (opts.force) { + return Bluebird.resolve(fn()); + } + throw new UpdatesLockedError('Updates locked'); + }); + + await request + .patch('/v1/device/host-config') + .send({ network: { hostname: 'foobaz' } }) + .set('Accept', 'application/json') + .set('Authorization', `Bearer ${apiKeys.cloudApiKey}`) + .expect(423); + + expect(updateLock.lock).to.be.calledOnce; + (updateLock.lock as SinonStub).restore(); + + await request + .get('/v1/device/host-config') + .set('Accept', 'application/json') + .set('Authorization', `Bearer ${apiKeys.cloudApiKey}`) + .then((response) => { + expect(response.body.network.hostname).to.deep.equal( + 'foobardevice', + ); + }); + }); + + it('allows patch while update locks are present if force is in req.body', async () => { + stub(updateLock, 'lock').callsFake((__, opts, fn) => { + if (opts.force) { + return Bluebird.resolve(fn()); + } + throw new UpdatesLockedError('Updates locked'); + }); + + await request + .patch('/v1/device/host-config') + .send({ network: { hostname: 'foobaz' }, force: true }) + .set('Accept', 'application/json') + .set('Authorization', `Bearer ${apiKeys.cloudApiKey}`) + .expect(200); + + expect(updateLock.lock).to.be.calledOnce; + (updateLock.lock as SinonStub).restore(); + + await request + .get('/v1/device/host-config') + .set('Accept', 'application/json') + .set('Authorization', `Bearer ${apiKeys.cloudApiKey}`) + .then((response) => { + expect(response.body.network.hostname).to.deep.equal('foobaz'); + }); + }); + it('updates the hostname with provided string if string is not empty', async () => { // stub servicePartOf to throw exceptions for the new service names stub(dbus, 'servicePartOf').callsFake(