diff --git a/src/datastore/AppsDataStore.ts b/src/datastore/AppsDataStore.ts index 1d5deba..211e51e 100644 --- a/src/datastore/AppsDataStore.ts +++ b/src/datastore/AppsDataStore.ts @@ -26,7 +26,7 @@ import isValidPath = require('is-valid-path') const APP_DEFINITIONS = 'appDefinitions' -function isNameAllowed(name: string) { +export function isNameAllowed(name: string) { const isNameFormattingOk = !!name && name.length < 50 && @@ -34,7 +34,11 @@ function isNameAllowed(name: string) { /[a-z0-9]$/.test(name) && /^[a-z0-9\-]+$/.test(name) && name.indexOf('--') < 0 - return isNameFormattingOk && ['captain', 'registry'].indexOf(name) < 0 + return ( + isNameFormattingOk && + ['captain', 'registry'].indexOf(name) < 0 && + !name.startsWith('captain-') + ) } /** @@ -294,12 +298,20 @@ class AppsDataStore { }) } - getServiceName(appName: string) { - return `srv-${this.namepace}--${appName}` + getServiceName(appName: string, isLegacyAppName: boolean) { + if (isLegacyAppName) { + return `srv-${this.namepace}--${appName}` + } + + return `${appName}` } - getVolumeName(volumeName: string) { - return `${this.namepace}--${volumeName}` + getVolumeName(volumeName: string, isLegacyVolumeName: boolean) { + if (isLegacyVolumeName) { + return `${this.namepace}--${volumeName}` + } + + return volumeName } getAppDefinitions() { diff --git a/src/datastore/DataStore.ts b/src/datastore/DataStore.ts index 18e0b0d..dc25406 100644 --- a/src/datastore/DataStore.ts +++ b/src/datastore/DataStore.ts @@ -3,6 +3,7 @@ */ import Configstore = require('configstore') import fs = require('fs-extra') +import { IAppDefSaved } from '../models/AppDefinition' import { AutomatedCleanupConfigsCleaner, IAutomatedCleanupConfigs, @@ -77,6 +78,25 @@ const DEFAULT_NGINX_CONFIG_FOR_APP = fs .readFileSync(DEFAULT_NGINX_CONFIG_FOR_APP_PATH) .toString() +export function runDataStoreMigrations(data: Configstore) { + const schemaVersion = data.get('schemaVersion') as number | undefined + + if (schemaVersion && schemaVersion >= 2) { + return + } + + const appDefinitions = data.get('appDefinitions') + if (appDefinitions) { + Object.keys(appDefinitions).forEach((appName) => { + const appDef = appDefinitions[appName] as IAppDefSaved + appDef.isLegacyAppName = true + }) + data.set('appDefinitions', appDefinitions) + } + + data.set('schemaVersion', 2) +} + class DataStore { private encryptor: CaptainEncryptor private namespace: string @@ -98,6 +118,8 @@ class DataStore { } ) + runDataStoreMigrations(data) + this.data = data this.namespace = namespace this.data.set(NAMESPACE, namespace) diff --git a/src/docker/DockerApi.ts b/src/docker/DockerApi.ts index 15a0734..54624af 100644 --- a/src/docker/DockerApi.ts +++ b/src/docker/DockerApi.ts @@ -1439,8 +1439,9 @@ class DockerApi { // /var/lib/docker/volumes/YOUR_VOLUME_NAME/_data mts.push({ Source: - (namespace ? namespace + '--' : '') + - v.volumeName, + (namespace && appObject?.isLegacyAppName + ? namespace + '--' + : '') + v.volumeName, Target: v.containerPath, Type: VolumesTypes.VOLUME, ReadOnly: false, diff --git a/src/models/AppDefinition.ts b/src/models/AppDefinition.ts index 332059c..49322e4 100644 --- a/src/models/AppDefinition.ts +++ b/src/models/AppDefinition.ts @@ -84,6 +84,10 @@ export interface IAppDefinitionBase { envVars: IAppEnvVar[] versions: IAppVersion[] appDeployTokenConfig?: AppDeployTokenConfig + + // True for apps created before v1.15.0 + // non-existent for apps created on or after v1.15.0 + isLegacyAppName?: boolean } export interface IHttpAuth { diff --git a/src/routes/user/apps/appdefinition/AppDefinitionRouter.ts b/src/routes/user/apps/appdefinition/AppDefinitionRouter.ts index 87c0d82..de2ba6a 100644 --- a/src/routes/user/apps/appdefinition/AppDefinitionRouter.ts +++ b/src/routes/user/apps/appdefinition/AppDefinitionRouter.ts @@ -9,12 +9,27 @@ import { } from '../../../../handlers/users/apps/appdefinition/AppDefinitionHandler' import InjectionExtractor from '../../../../injection/InjectionExtractor' import { AppDeployTokenConfig } from '../../../../models/AppDefinition' +import { IHashMapGeneric } from '../../../../models/ICacheGeneric' import CaptainManager from '../../../../user/system/CaptainManager' import Logger from '../../../../utils/Logger' import Utils from '../../../../utils/Utils' const router = express.Router() +export function ensureAppsExist( + appNames: string[], + apps: IHashMapGeneric +) { + appNames.forEach((appName) => { + if (!Object.prototype.hasOwnProperty.call(apps, appName)) { + throw ApiStatusCodes.createError( + ApiStatusCodes.STATUS_ERROR_GENERIC, + `App (${appName}) could not be found. Make sure that you have created the app.` + ) + } + }) +} + // unused images router.get('/unusedImages', function (req, res, next) { return Promise.resolve() @@ -182,6 +197,8 @@ router.post('/register/', function (req, res, next) { router.post('/delete/', function (req, res, next) { const serviceManager = InjectionExtractor.extractUserFromInjected(res).user.serviceManager + const dataStore = + InjectionExtractor.extractUserFromInjected(res).user.dataStore const appName: string = req.body.appName const volumes: string[] = req.body.volumes || [] @@ -189,6 +206,7 @@ router.post('/delete/', function (req, res, next) { const appsToDelete: string[] = appNames.length ? appNames : [appName] Logger.d(`Deleting app started: ${appName}`) + const volumesToDelete: IHashMapGeneric = {} return Promise.resolve() .then(function () { @@ -199,14 +217,39 @@ router.post('/delete/', function (req, res, next) { ) } }) + .then(function () { + return dataStore.getAppsDataStore().getAppDefinitions() + }) + .then(function (apps) { + ensureAppsExist(appsToDelete, apps) + + appsToDelete.forEach((appNameToDelete) => { + const app = apps[appNameToDelete] + + const volumesForApp = app.volumes || [] + volumesForApp.forEach((volume) => { + const volumeName = volume.volumeName + if (!volumeName || volumes.indexOf(volumeName) < 0) { + return + } + + const physicalVolumeName = dataStore + .getAppsDataStore() + .getVolumeName(volumeName, !!app.isLegacyAppName) + volumesToDelete[physicalVolumeName] = volumeName + }) + }) + }) .then(function () { return serviceManager.removeApps(appsToDelete) }) .then(function () { - return Utils.getDelayedPromise(volumes.length ? 12000 : 0) + return Utils.getDelayedPromise( + Object.keys(volumesToDelete).length ? 12000 : 0 + ) }) .then(function () { - return serviceManager.removeVolsSafe(volumes) + return serviceManager.removeVolsSafe(volumesToDelete) }) .then(function (failedVolsToRemoved) { Logger.d(`Successfully deleted: ${appsToDelete.join(', ')}`) diff --git a/src/user/ServiceManager.ts b/src/user/ServiceManager.ts index 569b1e3..a6e5eb4 100644 --- a/src/user/ServiceManager.ts +++ b/src/user/ServiceManager.ts @@ -423,13 +423,11 @@ class ServiceManager { Logger.d(`Renaming app: ${oldAppName}`) const self = this - const oldServiceName = this.dataStore - .getAppsDataStore() - .getServiceName(oldAppName) const dockerApi = this.dockerApi const dataStore = this.dataStore let defaultSslOn = false + let oldServiceName: string return Promise.resolve() .then(function () { @@ -437,6 +435,9 @@ class ServiceManager { }) .then(function (appDef) { defaultSslOn = !!appDef.hasDefaultSubDomainSsl + oldServiceName = dataStore + .getAppsDataStore() + .getServiceName(oldAppName, !!appDef.isLegacyAppName) dataStore.getAppsDataStore().nameAllowedOrThrow(newAppName) @@ -473,14 +474,20 @@ class ServiceManager { const self = this const removeAppPromise = function (appName: string) { - const serviceName = self.dataStore - .getAppsDataStore() - .getServiceName(appName) const dockerApi = self.dockerApi const dataStore = self.dataStore + let serviceName: string return Promise.resolve() .then(function () { + return dataStore + .getAppsDataStore() + .getAppDefinition(appName) + }) + .then(function (appDef) { + serviceName = dataStore + .getAppsDataStore() + .getServiceName(appName, !!appDef.isLegacyAppName) return self.ensureNotBuilding(appName) }) .then(function () { @@ -515,7 +522,7 @@ class ServiceManager { return Promise.all(promises) } - removeVolsSafe(volumes: string[]) { + removeVolsSafe(volumes: IHashMapGeneric) { const dockerApi = this.dockerApi const dataStore = this.dataStore @@ -526,35 +533,42 @@ class ServiceManager { return dataStore.getAppsDataStore().getAppDefinitions() }) .then(function (apps) { - // Don't even try deleting volumes which are present in other app definitions + const physicalVolumesInUse: IHashMapGeneric = {} + Object.keys(apps).forEach((appName) => { const app = apps[appName] const volsInApp = app.volumes || [] - volsInApp.forEach((v) => { - const volName = v.volumeName - if (!volName) return - if (volumes.indexOf(volName) >= 0) { - volsFailedToDelete[volName] = true + volsInApp.forEach((volume) => { + const volumeName = volume.volumeName + if (!volumeName) { + return } + + const physicalVolumeName = dataStore + .getAppsDataStore() + .getVolumeName(volumeName, !!app.isLegacyAppName) + physicalVolumesInUse[physicalVolumeName] = true }) }) const volumesTryToDelete: string[] = [] - - volumes.forEach((v) => { - if (!volsFailedToDelete[v]) { - volumesTryToDelete.push( - dataStore.getAppsDataStore().getVolumeName(v) - ) + Object.keys(volumes).forEach((physicalVolumeName) => { + const logicalVolumeName = volumes[physicalVolumeName] + if (physicalVolumesInUse[physicalVolumeName]) { + volsFailedToDelete[logicalVolumeName] = true + } else { + volumesTryToDelete.push(physicalVolumeName) } }) return dockerApi.deleteVols(volumesTryToDelete) }) .then(function (failedVols) { - failedVols.forEach((v) => { - volsFailedToDelete[v] = true + failedVols.forEach((physicalVolumeName) => { + const logicalVolumeName = + volumes[physicalVolumeName] || physicalVolumeName + volsFailedToDelete[logicalVolumeName] = true }) return Object.keys(volsFailedToDelete) @@ -664,7 +678,7 @@ class ServiceManager { .then(function (app) { serviceName = dataStore .getAppsDataStore() - .getServiceName(appName) + .getServiceName(appName, !!app.isLegacyAppName) // After leaving this block, nodeId will be guaranteed to be NonNull if (app.hasPersistentData) { @@ -869,14 +883,18 @@ class ServiceManager { } getAppLogs(appName: string, encoding: string) { - const serviceName = this.dataStore - .getAppsDataStore() - .getServiceName(appName) - const dockerApi = this.dockerApi + const dataStore = this.dataStore + let serviceName: string return Promise.resolve() // .then(function () { + return dataStore.getAppsDataStore().getAppDefinition(appName) + }) + .then(function (appDef) { + serviceName = dataStore + .getAppsDataStore() + .getServiceName(appName, !!appDef.isLegacyAppName) return dockerApi.getLogForService( serviceName, CaptainConstants.configs.appLogSize, @@ -889,15 +907,12 @@ class ServiceManager { Logger.d(`Ensure service inited and Updated for: ${appName}`) const self = this - const serviceName = this.dataStore - .getAppsDataStore() - .getServiceName(appName) - let imageName: string | undefined const dockerApi = this.dockerApi const dataStore = this.dataStore let app: IAppDef let dockerAuthObject: DockerAuthObj | undefined + let serviceName: string return Promise.resolve() // .then(function () { @@ -905,6 +920,9 @@ class ServiceManager { }) .then(function (appFound) { app = appFound + serviceName = dataStore + .getAppsDataStore() + .getServiceName(appName, !!app.isLegacyAppName) Logger.d(`Check if service is running: ${serviceName}`) return dockerApi.isServiceRunningByName(serviceName) diff --git a/src/user/system/LoadBalancerManager.ts b/src/user/system/LoadBalancerManager.ts index dee5f68..d4d9c68 100644 --- a/src/user/system/LoadBalancerManager.ts +++ b/src/user/system/LoadBalancerManager.ts @@ -308,7 +308,7 @@ class LoadBalancerManager { const localDomain = dataStore .getAppsDataStore() - .getServiceName(appName) + .getServiceName(appName, !!webApp.isLegacyAppName) const forceSsl = !!webApp.forceSsl const websocketSupport = !!webApp.websocketSupport const nginxConfigTemplate = diff --git a/tests/AppDeletion.test.ts b/tests/AppDeletion.test.ts new file mode 100644 index 0000000..fa3ad4d --- /dev/null +++ b/tests/AppDeletion.test.ts @@ -0,0 +1,19 @@ +import { ensureAppsExist } from '../src/routes/user/apps/appdefinition/AppDefinitionRouter' + +describe('app deletion', () => { + test('rejects the entire request when any requested app is missing', () => { + expect(() => + ensureAppsExist(['existing-app', 'missing-app'], { + 'existing-app': {}, + }) + ).toThrow( + 'App (missing-app) could not be found. Make sure that you have created the app.' + ) + }) + + test('does not treat inherited object properties as existing apps', () => { + expect(() => ensureAppsExist(['constructor'], {})).toThrow( + 'App (constructor) could not be found. Make sure that you have created the app.' + ) + }) +}) diff --git a/tests/ServiceNamingMigration.test.ts b/tests/ServiceNamingMigration.test.ts new file mode 100644 index 0000000..f4896f2 --- /dev/null +++ b/tests/ServiceNamingMigration.test.ts @@ -0,0 +1,120 @@ +import configstore = require('configstore') +import AppsDataStore, { + isNameAllowed, +} from '../src/datastore/AppsDataStore' +import { runDataStoreMigrations } from '../src/datastore/DataStore' +import ServiceManager from '../src/user/ServiceManager' + +function createConfigStore(initialData: { [key: string]: any }) { + const data = { ...initialData } + return { + get: jest.fn((key: string) => data[key]), + set: jest.fn((key: string, value: any) => { + data[key] = value + }), + } as unknown as configstore +} + +describe('service and volume naming migration', () => { + test('marks existing apps as legacy when schemaVersion is missing', () => { + const appDefinitions = { + existingApp: {}, + } + const data = createConfigStore({ appDefinitions }) + + runDataStoreMigrations(data) + + expect(appDefinitions.existingApp).toEqual({ + isLegacyAppName: true, + }) + expect(data.set).toHaveBeenCalledWith('schemaVersion', 2) + }) + + test('does not mark apps created after schema version 2 as legacy', () => { + const appDefinitions = { + newApp: {}, + } + const data = createConfigStore({ + schemaVersion: 2, + appDefinitions, + }) + + runDataStoreMigrations(data) + + expect(appDefinitions.newApp).toEqual({}) + expect(data.set).not.toHaveBeenCalled() + }) + + test('reserves the captain service-name prefix', () => { + expect(isNameAllowed('captain-nginx')).toBe(false) + expect(isNameAllowed('captain-custom')).toBe(false) + expect(isNameAllowed('my-app')).toBe(true) + }) + + test('keeps legacy and new physical volumes distinct during deletion', async () => { + const appsDataStore = new AppsDataStore( + createConfigStore({}), + 'captain' + ) + jest.spyOn(appsDataStore, 'getAppDefinitions').mockResolvedValue({ + remainingLegacyApp: { + isLegacyAppName: true, + volumes: [ + { + volumeName: 'data', + containerPath: '/data', + }, + ], + } as any, + }) + + const deleteVols = jest.fn().mockResolvedValue([]) + const serviceManager = Object.create( + ServiceManager.prototype + ) as ServiceManager + ;(serviceManager as any).dataStore = { + getAppsDataStore: () => appsDataStore, + } + ;(serviceManager as any).dockerApi = { + deleteVols, + } + + const failedVolumes = await serviceManager.removeVolsSafe({ + 'captain--data': 'data', + data: 'data', + }) + + expect(deleteVols).toHaveBeenCalledWith(['data']) + expect(failedVolumes).toEqual(['data']) + }) + + test('deletes both physical volumes when neither remains in use', async () => { + const appsDataStore = new AppsDataStore( + createConfigStore({}), + 'captain' + ) + jest.spyOn(appsDataStore, 'getAppDefinitions').mockResolvedValue({}) + + const deleteVols = jest.fn().mockResolvedValue([]) + const serviceManager = Object.create( + ServiceManager.prototype + ) as ServiceManager + ;(serviceManager as any).dataStore = { + getAppsDataStore: () => appsDataStore, + } + ;(serviceManager as any).dockerApi = { + deleteVols, + } + + const failedVolumes = await serviceManager.removeVolsSafe({ + 'captain--data': 'data', + data: 'data', + }) + + expect(deleteVols).toHaveBeenCalledWith([ + 'captain--data', + 'data', + ]) + expect(failedVolumes).toEqual([]) + }) +})