Merge pull request #2328 from caprover/removed-captain-prefixes

Removing the **captain** prefix for service and volume names

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() {

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)

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,

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 {

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<any>
+) {
+    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<string> = {}
 
     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(', ')}`)

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<string>) {
         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<boolean> = {}
+
                 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)

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 =

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.'
+        )
+    })
+})

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([])
+    })
+})