From 8aa57d66e8d7d5c5382e2388b5fea1c905273c52 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:25:40 +0200 Subject: [PATCH] fix(previews): give each preview its own volume for external volumes Previews of Docker Compose applications used external volumes as written, so a preview mounted the production volume. Two database containers on the same data can corrupt it. - A preview uses {uuid}_-pr-N with a name-only declaration, the same name as v4.3.23, so old preview volumes are used again - Production still uses the external volume as written - Preview cleanup removes the preview volume, never the external one - Do not show the external volume warning for previews --- app/Models/LocalPersistentVolume.php | 8 +- bootstrap/helpers/parsers.php | 41 +++++--- .../ApplicationPreviewComposeCleanupTest.php | 3 +- tests/Feature/ComposeExternalVolumeTest.php | 95 +++++++++++++++---- 4 files changed, 112 insertions(+), 35 deletions(-) diff --git a/app/Models/LocalPersistentVolume.php b/app/Models/LocalPersistentVolume.php index 0ff3a1903f..cd251aa2d4 100644 --- a/app/Models/LocalPersistentVolume.php +++ b/app/Models/LocalPersistentVolume.php @@ -227,8 +227,9 @@ class LocalPersistentVolume extends BaseModel /** * The external Compose volume that this storage entry still replaces, or null. Before Coolify * used external volumes as written, the parsers gave them a generated name, for example - * "{uuid}_{volume}" or "{uuid}_{volume}-pr-{id}". While this storage entry exists, the parsers - * keep that name so that the resource keeps its data (see useComposeExternalVolumeAsWritten()). + * "{uuid}_{volume}". While this storage entry exists, the parsers keep that name so that the + * resource keeps its data (see useComposeExternalVolumeAsWritten()). A preview volume + * ("{uuid}_{volume}-pr-{id}") replaces nothing: a preview never uses the external volume. */ public function replacedExternalComposeVolume(): ?string { @@ -256,8 +257,7 @@ class LocalPersistentVolume extends BaseModel $resource instanceof Application => $resource->uuid.'_'.Str::slug($key, '-'), default => data_get($resource, 'service.uuid').'_'.Str::slug($key, '-'), }; - $isLegacyName = $legacyName !== $key && $this->name === $legacyName; - if ($isLegacyName || preg_match('/^'.preg_quote($legacyName, '/').'-pr-\d+$/', $this->name) === 1) { + if ($legacyName !== $key && $this->name === $legacyName) { return $key; } } diff --git a/bootstrap/helpers/parsers.php b/bootstrap/helpers/parsers.php index 8947b3964b..85cb6e2bf1 100644 --- a/bootstrap/helpers/parsers.php +++ b/bootstrap/helpers/parsers.php @@ -342,8 +342,9 @@ function isComposeExternalVolume(mixed $declaration): bool /** * Returns the top-level declaration of a volume source when the Compose file declares it as * external, and null for all other sources. The parsers use an external volume as written: no - * "{uuid}_" prefix, no preview suffix and no LocalPersistentVolume row, so that Coolify never - * removes a volume that it does not own. + * "{uuid}_" prefix and no LocalPersistentVolume row, so that Coolify never removes a volume that it + * does not own. A preview deployment never uses an external volume; it gets its own volume + * "{uuid}_{volume}-pr-{id}" like any other named volume. * * @param iterable $topLevelVolumes * @return array|null @@ -364,9 +365,11 @@ function composeExternalVolumeDeclaration(iterable $topLevelVolumes, string $sou /** * Tells if a parser uses the volume source as written because it is an external volume. * + * Only for production deployments: a preview deployment always gets its own volume. + * * Before Coolify used external volumes as written, the parsers renamed them like all other - * volumes (the old name, for example "{uuid}_{volume}" or "{uuid}_{volume}-pr-{id}"), so the - * resource wrote its data into the renamed volume. When the owner still has the storage entry + * volumes (the old name, for example "{uuid}_{volume}"), so the resource wrote its data into the + * renamed volume. When the owner still has the storage entry * with the old name, the parser must keep the old name, or the resource loses its data. Then * this function records a warning on the resource and returns false. When the user deletes that * storage entry, the next parse uses the external volume. @@ -431,7 +434,8 @@ function legacyApplicationRenamedVolumeDeclaration(string $name): array * Records a warning when a legacy Compose application (parser version 1 or 2) does not use an * external volume as written. These parsers keep the old volume name (see * legacyApplicationComposeVolumeName()), so the resource keeps its data. The parser version 1 keeps - * the name of a production volume, so it uses the external volume and gets no warning. + * the name of a production volume, so it uses the external volume and gets no warning. A preview + * always uses its own volume, so it gets no warning either. * * The volume names are not validated here, so that the legacy parsers keep their old behavior. The * warning shows the Docker volume name only when it is a literal, valid Docker volume name. @@ -440,6 +444,9 @@ function legacyApplicationRenamedVolumeDeclaration(string $name): array */ function warnLegacyApplicationComposeExternalVolume(Application $resource, iterable $topLevelVolumes, string $source, int $pull_request_id): void { + if ($pull_request_id !== 0) { + return; + } $declaration = collect($topLevelVolumes)->get($source); if (! isComposeExternalVolume($declaration)) { return; @@ -1331,6 +1338,7 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int } // Parse the rest of the services + $previewOwnExternalVolumes = collect([]); foreach ($services as $serviceName => $service) { $image = data_get_str($service, 'image'); $restart = data_get_str($service, 'restart', RESTART_MODE); @@ -1481,16 +1489,21 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int } } } elseif ($type->value() === 'volume') { - $legacyName = "{$uuid}_".Str::slug($source, '-'); - if ($isPullRequest) { - $legacyName = addPreviewDeploymentSuffix($legacyName, $pull_request_id); - } - if (useComposeExternalVolumeAsWritten($resource, $originalResource, $topLevel->get('volumes'), $source->value(), $legacyName)) { - // Previews share the external volume. It gets no row, so Coolify never removes it. + // A preview never mounts an external volume: two deployments that write into the same + // data directory (for example two databases) can corrupt it. The preview gets its own + // volume with the name that older Coolify versions gave it, so it keeps its data, and + // the preview cleanup removes it. + $isPreviewOwnExternalVolume = $isPullRequest + && composeExternalVolumeDeclaration($topLevel->get('volumes'), $source->value()) !== null; + if (! $isPullRequest && useComposeExternalVolumeAsWritten($resource, $originalResource, $topLevel->get('volumes'), $source->value(), "{$uuid}_".Str::slug($source, '-'))) { + // The external volume gets no row, so Coolify never removes it. $volumesParsed->put($index, $volume); continue; } + if ($isPreviewOwnExternalVolume) { + $previewOwnExternalVolumes->put($source->value(), true); + } $declaration = $topLevel->get('volumes')->get($source->value()); if (in_array(data_get($declaration, 'driver_opts.type'), ['cifs', 'nfs'], true)) { // Network volumes are used as written. @@ -1529,7 +1542,9 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int 'resource_type' => get_class($originalResource), ] ); - $topLevel->get('volumes')->put($name, composeRenamedVolumeDeclarationFor($declaration, $name, $persistentVolume, $isPullRequest)); + $topLevel->get('volumes')->put($name, $isPreviewOwnExternalVolume + ? ['name' => $name] + : composeRenamedVolumeDeclarationFor($declaration, $name, $persistentVolume, $isPullRequest)); } dispatch(new ServerFilesFromServerJob($originalResource)); $volumesParsed->put($index, $volume); @@ -2134,6 +2149,8 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int $parsedServices->put($serviceName, $payload); } + // The preview mounts its own volumes instead of these external volumes (see above), so it does not declare them. + $topLevel->put('volumes', $topLevel->get('volumes')->except($previewOwnExternalVolumes->keys()->all())); $topLevel->put('services', $parsedServices); $customOrder = ['services', 'volumes', 'networks', 'configs', 'secrets']; diff --git a/tests/Feature/ApplicationPreviewComposeCleanupTest.php b/tests/Feature/ApplicationPreviewComposeCleanupTest.php index 41c904d35c..dfb4f546cf 100644 --- a/tests/Feature/ApplicationPreviewComposeCleanupTest.php +++ b/tests/Feature/ApplicationPreviewComposeCleanupTest.php @@ -131,7 +131,8 @@ it('removes only the network and volumes that Coolify generated for the preview' expect($resourceCommands) ->toContain("sudo docker volume rm -f '{$this->uuid}_app-data-pr-42'") - ->not->toContain('shared-data-pr-42') + // A preview gets its own volume instead of the external one. + ->toContain("sudo docker volume rm -f '{$this->uuid}_shared-data-pr-42'") ->toContain("sudo docker network disconnect '{$this->previewNetwork}' coolify-proxy") ->toContain("sudo docker network rm '{$this->previewNetwork}'") ->not->toContain("'app-data'") diff --git a/tests/Feature/ComposeExternalVolumeTest.php b/tests/Feature/ComposeExternalVolumeTest.php index 9b97887a32..4577b9b2b5 100644 --- a/tests/Feature/ComposeExternalVolumeTest.php +++ b/tests/Feature/ComposeExternalVolumeTest.php @@ -304,18 +304,34 @@ describe('applicationParser', function () { ->and(persistentVolumeNames())->toBe([]); }); - it('shares the external volume with a preview deployment', function () { + it('gives a preview deployment its own volume instead of the external volume', function () { $application = externalVolumeApplication(EXTERNAL_VOLUME_SHORT_COMPOSE); $uuid = $application->uuid; $preview = externalVolumePreview($application); $compose = applicationParser($application, 42, $preview->id)->toArray(); - expect($compose['services']['web-pr-42']['volumes'])->toContain('shared-data:/data:ro') + expect($compose['services']['web-pr-42']['volumes'])->toContain("{$uuid}_shared-data-pr-42:/data:ro") ->toContain("{$uuid}_app-data-pr-42:/app") - ->and($compose['volumes']['shared-data'])->toBe(['external' => true]) - ->and($compose['volumes'])->not->toHaveKey("{$uuid}_shared-data-pr-42") - ->and(persistentVolumeNames())->toBe(["{$uuid}_app-data-pr-42"]); + ->not->toContain('shared-data:/data:ro') + ->and($compose['volumes']["{$uuid}_shared-data-pr-42"])->toBe(['name' => "{$uuid}_shared-data-pr-42"]) + ->and($compose['volumes'])->not->toHaveKey('shared-data') + ->and(persistentVolumeNames())->toBe(["{$uuid}_app-data-pr-42", "{$uuid}_shared-data-pr-42"]) + ->and($application->composeVolumeWarnings())->toBe([]); + }); + + it('gives a preview deployment its own volume instead of a named external volume in long syntax', function () { + $application = externalVolumeApplication(EXTERNAL_VOLUME_LONG_COMPOSE); + $uuid = $application->uuid; + $preview = externalVolumePreview($application); + + $compose = applicationParser($application, 42, $preview->id); + $sources = collect($compose->toArray()['services']['web-pr-42']['volumes'])->pluck('source')->all(); + + expect($sources)->toBe(["{$uuid}_shared-data-pr-42", "{$uuid}_app-data-pr-42"]) + ->and($compose->toArray()['volumes']["{$uuid}_shared-data-pr-42"])->toBe(['name' => "{$uuid}_shared-data-pr-42"]) + ->and(Yaml::dump(convertToArray($compose), 10, 2))->not->toContain('existing-shared-volume') + ->and($application->composeVolumeWarnings())->toBe([]); }); it('does not show a warning when the resource has no old storage entry', function () { @@ -354,21 +370,22 @@ describe('applicationParser', function () { ->and($application->composeVolumeWarnings())->toBe([legacyExternalVolumeWarning("{$uuid}_shared-data", 'existing-shared-volume')]); }); - it('keeps the old prefixed preview volume when the resource has its old storage entry', function () { + it('reuses the preview volume of an older Coolify version without a warning', function () { $application = externalVolumeApplication(EXTERNAL_VOLUME_SHORT_COMPOSE); $uuid = $application->uuid; $preview = externalVolumePreview($application); - legacyExternalVolumeRow($application, "{$uuid}_shared-data-pr-42"); + $oldVolume = legacyExternalVolumeRow($application, "{$uuid}_shared-data-pr-42"); $compose = applicationParser($application, 42, $preview->id)->toArray(); expect($compose['services']['web-pr-42']['volumes'])->toContain("{$uuid}_shared-data-pr-42:/data:ro") ->and($compose['volumes']["{$uuid}_shared-data-pr-42"])->toBe(['name' => "{$uuid}_shared-data-pr-42"]) + ->and(LocalPersistentVolume::find($oldVolume->id)?->name)->toBe("{$uuid}_shared-data-pr-42") ->and(persistentVolumeNames())->toBe(["{$uuid}_app-data-pr-42", "{$uuid}_shared-data-pr-42"]) - ->and($application->composeVolumeWarnings())->toBe([legacyExternalVolumeWarning("{$uuid}_shared-data-pr-42")]); + ->and($application->composeVolumeWarnings())->toBe([]); }); - it('uses the external volume in a preview when only the production volume has an old storage entry', function () { + it('gives a preview its own volume when only the production volume has an old storage entry', function () { $application = externalVolumeApplication(EXTERNAL_VOLUME_SHORT_COMPOSE); $uuid = $application->uuid; $preview = externalVolumePreview($application); @@ -376,7 +393,9 @@ describe('applicationParser', function () { $compose = applicationParser($application, 42, $preview->id)->toArray(); - expect($compose['services']['web-pr-42']['volumes'])->toContain('shared-data:/data:ro') + expect($compose['services']['web-pr-42']['volumes'])->toContain("{$uuid}_shared-data-pr-42:/data:ro") + ->not->toContain('shared-data:/data:ro') + ->not->toContain("{$uuid}_shared-data:/data:ro") ->and($application->composeVolumeWarnings())->toBe([]); }); @@ -532,7 +551,7 @@ describe('legacy parsers', function () { ->and($service->composeVolumeWarnings())->toBe([]); }); - it('keeps the old volume names of a legacy application and shows a warning when the name is not the external name', function (string $parsingVersion, int $pullRequestId, string $compose, string $expectedVolume, ?string $dockerVolume) { + it('keeps the old volume names of a legacy application and shows a warning when production does not use the external name', function (string $parsingVersion, int $pullRequestId, string $compose, string $expectedVolume, ?string $dockerVolume) { // Legacy Compose applications (parser versions 1 and 2) never stored their volumes, so Coolify // cannot tell which external volume already holds data: they always keep the old names. $application = externalVolumeApplication($compose, $parsingVersion); @@ -548,7 +567,8 @@ describe('legacy parsers', function () { ->and(array_keys($parsed['volumes'] ?? []))->toContain($expectedName) ->and($application->composeVolumeWarnings())->toBe( // Parser version 1 keeps the name of a production volume, so it uses the external volume. - $expectedName === 'shared-data' ? [] : [legacyParserExternalVolumeWarning($expectedName, $dockerVolume)] + // A preview always uses its own volume, so it gets no warning. + $pullRequestId !== 0 || $expectedName === 'shared-data' ? [] : [legacyParserExternalVolumeWarning($expectedName, $dockerVolume)] ); })->with([ 'v1 short' => ['1', 0, EXTERNAL_VOLUME_SHORT_COMPOSE, 'shared-data:/data:ro', null], @@ -572,8 +592,20 @@ describe('legacy parsers', function () { ->and($logEntries)->toBe([['Warning: '.legacyParserExternalVolumeWarning($keptName), 'stderr']]); })->with([ 'v2 production' => ['2', 0, '{uuid}-shared-data'], - 'v2 preview' => ['2', 42, '{uuid}-shared-data-pr-42'], - 'v1 preview' => ['1', 42, 'shared-data-pr-42'], + ]); + + it('writes no warning to the deployment log of a legacy application preview, which always uses its own volume', function (string $parsingVersion, string $keptName) { + $application = externalVolumeApplication(EXTERNAL_VOLUME_SHORT_COMPOSE, $parsingVersion); + $keptName = str_replace('{uuid}', $application->uuid, $keptName); + $preview = externalVolumePreview($application); + + [$composeFile, $logEntries] = parseExternalVolumeDeployment($application, 42, $preview); + + expect(data_get($composeFile, 'services.web-pr-42.volumes'))->toContain("{$keptName}:/data:ro") + ->and($logEntries)->toBe([]); + })->with([ + 'v2 preview' => ['2', '{uuid}-shared-data-pr-42'], + 'v1 preview' => ['1', 'shared-data-pr-42'], ]); it('writes no warning to the deployment log for a production deployment of a parser version 1 application', function () { @@ -842,8 +874,8 @@ describe('delete', function () { expect(LocalPersistentVolume::find($volume->id))->not->toBeNull(); }); - it('does not remove the external volume when a preview is deleted', function () { - $application = externalVolumeApplication(EXTERNAL_VOLUME_SHORT_COMPOSE); + it('removes the own volume of the preview, but not the external volume, when a preview is deleted', function (string $compose, string $externalVolume) { + $application = externalVolumeApplication($compose); $uuid = $application->uuid; $preview = externalVolumePreview($application); $commands = []; @@ -853,7 +885,23 @@ describe('delete', function () { $volumeCommands = collect($commands)->filter(fn (string $command): bool => str_contains($command, 'docker volume'))->implode("\n"); expect($volumeCommands)->toContain("docker volume rm -f '{$uuid}_app-data-pr-42'") - ->not->toContain('shared-data'); + ->toContain("docker volume rm -f '{$uuid}_shared-data-pr-42'") + ->not->toContain("'{$externalVolume}'"); + })->with([ + 'short syntax' => [EXTERNAL_VOLUME_SHORT_COMPOSE, 'shared-data'], + 'named external volume' => [EXTERNAL_VOLUME_LONG_COMPOSE, 'existing-shared-volume'], + ]); + + it('does not mark the own volume of a preview as an entry that replaces the external volume', function () { + $application = externalVolumeApplication(EXTERNAL_VOLUME_SHORT_COMPOSE); + $uuid = $application->uuid; + $preview = externalVolumePreview($application); + $productionEntry = legacyExternalVolumeRow($application, "{$uuid}_shared-data"); + applicationParser($application, 42, $preview->id); + $previewEntry = LocalPersistentVolume::query()->where('name', "{$uuid}_shared-data-pr-42")->firstOrFail(); + + expect($previewEntry->replacedExternalComposeVolume())->toBeNull() + ->and($productionEntry->replacedExternalComposeVolume())->toBe('shared-data'); }); it('removes the old prefixed preview volume, but not the external volume, when a preview is deleted', function () { @@ -886,9 +934,20 @@ describe('warnings', function () { ->and($logEntries)->toBe([['Warning: '.legacyExternalVolumeWarning(str_replace('{uuid}', $uuid, $oldName)), 'stderr']]); })->with([ 'production' => [0, '{uuid}_shared-data', '{uuid}_shared-data:/data:ro'], - 'preview' => [42, '{uuid}_shared-data-pr-42', '{uuid}_shared-data-pr-42:/data:ro'], ]); + it('writes no warning to the deployment log of a preview, which always uses its own volume', function () { + $application = externalVolumeApplication(EXTERNAL_VOLUME_SHORT_COMPOSE); + $uuid = $application->uuid; + $preview = externalVolumePreview($application); + legacyExternalVolumeRow($application, "{$uuid}_shared-data-pr-42"); + + [$composeFile, $logEntries] = parseExternalVolumeDeployment($application, 42, $preview); + + expect(data_get($composeFile, 'services.web-pr-42.volumes'))->toContain("{$uuid}_shared-data-pr-42:/data:ro") + ->and($logEntries)->toBe([]); + }); + it('writes no warning to the deployment log when the external volume is used as written', function () { $application = externalVolumeApplication(EXTERNAL_VOLUME_SHORT_COMPOSE);