From 87f616fc3152a52c64b4d601f0a77049a3eb785a Mon Sep 17 00:00:00 2001 From: Kasra Bigdeli Date: Sun, 11 Aug 2024 15:49:31 -0700 Subject: [PATCH] Added nginx -t test and revert if the config is invalid. Fixed https://github.com/caprover/caprover/issues/660 --- src/api/ApiStatusCodes.ts | 1 + src/user/ServiceManager.ts | 64 +++++++++++++++++++++++++- src/user/system/CaptainManager.ts | 37 ++++++++++++++- src/user/system/LoadBalancerManager.ts | 28 +++++++++++ 4 files changed, 127 insertions(+), 3 deletions(-) diff --git a/src/api/ApiStatusCodes.ts b/src/api/ApiStatusCodes.ts index 905008c..fab8012 100644 --- a/src/api/ApiStatusCodes.ts +++ b/src/api/ApiStatusCodes.ts @@ -48,6 +48,7 @@ class ApiStatusCodes { static readonly STATUS_PASSWORD_BACK_OFF = 1113 static readonly STATUS_ERROR_OTP_REQUIRED = 1114 static readonly STATUS_ERROR_PRO_API_KEY_INVALIDATED = 1115 + static readonly STATUS_ERROR_NGINX_VALIDATION_FAILED = 1116 } export default ApiStatusCodes diff --git a/src/user/ServiceManager.ts b/src/user/ServiceManager.ts index 89127d0..4369ae5 100644 --- a/src/user/ServiceManager.ts +++ b/src/user/ServiceManager.ts @@ -613,6 +613,8 @@ class ServiceManager { let serviceName: string + let existingAppDefinition: IAppDef + const checkIfNodeIdExists = function (nodeIdToCheck: string) { return dockerApi.getNodesInfo().then(function (nodeInfo) { for (let i = 0; i < nodeInfo.length; i++) { @@ -704,6 +706,11 @@ class ServiceManager { } }) .then(function () { + return dataStore.getAppsDataStore().getAppDefinition(appName) + }) + .then(function (appDef) { + existingAppDefinition = appDef + return dataStore .getAppsDataStore() .updateAppDefinitionInDb( @@ -733,8 +740,61 @@ class ServiceManager { .then(function () { return self.ensureServiceInitedAndUpdated(appName) }) - .then(function () { - return self.reloadLoadBalancer() + .catch(function (error) { + if ( + error && + error.captainErrorType === + ApiStatusCodes.STATUS_ERROR_NGINX_VALIDATION_FAILED + ) { + // Revert back to the old definition because the nginx config is invalid + if (existingAppDefinition) { + Logger.d( + `nginx validation failed, reverting configs for: ${appName}` + ) + return dataStore + .getAppsDataStore() + .updateAppDefinitionInDb( + appName, + existingAppDefinition.description, + existingAppDefinition.instanceCount, + existingAppDefinition.captainDefinitionRelativeFilePath, + existingAppDefinition.envVars, + existingAppDefinition.volumes, + existingAppDefinition.tags || [], + existingAppDefinition.nodeId || '', + existingAppDefinition.notExposeAsWebApp, + existingAppDefinition.containerHttpPort || 80, + existingAppDefinition.httpAuth, + existingAppDefinition.forceSsl, + existingAppDefinition.ports, + existingAppDefinition.appPushWebhook + ?.repoInfo || { + repo: '', + branch: '', + user: '', + password: '', + }, + self.authenticator, + existingAppDefinition.customNginxConfig || '', + existingAppDefinition.redirectDomain || '', + existingAppDefinition.preDeployFunction || '', + existingAppDefinition.serviceUpdateOverride || + '', + existingAppDefinition.websocketSupport, + existingAppDefinition.appDeployTokenConfig || { + enabled: false, + } + ) + .then(function () { + self.reloadLoadBalancer() + }) + .then(function () { + throw error + }) + } + } + + throw error }) } diff --git a/src/user/system/CaptainManager.ts b/src/user/system/CaptainManager.ts index 5048840..1184ea4 100644 --- a/src/user/system/CaptainManager.ts +++ b/src/user/system/CaptainManager.ts @@ -780,12 +780,47 @@ class CaptainManager { setNginxConfig(baseConfig: string, captainConfig: string) { const self = this + let existingConfigs: { + baseConfig: { + byDefault: string + customValue: any + } + captainConfig: { + byDefault: string + customValue: any + } + } return Promise.resolve() .then(function () { + return self.dataStore.getNginxConfig() + }) + .then(function (configs) { + existingConfigs = configs return self.dataStore.setNginxConfig(baseConfig, captainConfig) }) .then(function () { - self.resetSelf() + return self.loadBalancerManager.rePopulateNginxConfigFile() + }) + .catch(function (error) { + if ( + error && + error.captainErrorType === + ApiStatusCodes.STATUS_ERROR_NGINX_VALIDATION_FAILED + ) { + Logger.d( + "Nginx validation failed. Reverting changes in system's nginx configs..." + ) + self.dataStore + .setNginxConfig( + existingConfigs.baseConfig.customValue, + existingConfigs.captainConfig.customValue + ) + + .then(function () { + return self.loadBalancerManager.rePopulateNginxConfigFile() + }) + } + throw error }) } diff --git a/src/user/system/LoadBalancerManager.ts b/src/user/system/LoadBalancerManager.ts index ca270d2..23c5e88 100644 --- a/src/user/system/LoadBalancerManager.ts +++ b/src/user/system/LoadBalancerManager.ts @@ -183,6 +183,34 @@ class LoadBalancerManager { .then(function () { return self.createRootConfFile() }) + .then(function () { + return self.dockerApi + .executeCommand(CaptainConstants.nginxServiceName, [ + 'nginx', + '-t', + ]) + .then(function (result) { + // nginx:1.24 + // Failed example: + // + // 2024/08/11 22:32:59 [emerg] 28#28: unknown directive "eventsxxx" in /etc/nginx/nginx.conf:8 + // nginx: [emerg] unknown directive "eventsxxx" in /etc/nginx/nginx.conf:8 + // nginx: configuration file /etc/nginx/nginx.conf test failed + + // Successful example: + // + // nginx: the configuration file /etc/nginx/nginx.conf syntax is ok + // nginx: configuration file /etc/nginx/nginx.conf test is successful + + if (result.indexOf('test is successful') < 0) { + throw ApiStatusCodes.createError( + ApiStatusCodes.STATUS_ERROR_NGINX_VALIDATION_FAILED, + result + ) + } + }) + }) + .then(function () { Logger.d('sendReloadSignal...') return self.dockerApi.sendSingleContainerKillHUP(