diff --git a/app/Actions/Service/UpdateServiceApplicationFromApi.php b/app/Actions/Service/UpdateServiceApplicationFromApi.php index 7357368f9f..8065e11e05 100644 --- a/app/Actions/Service/UpdateServiceApplicationFromApi.php +++ b/app/Actions/Service/UpdateServiceApplicationFromApi.php @@ -3,6 +3,7 @@ namespace App\Actions\Service; use App\Models\ServiceApplication; +use App\Services\Dns\ManagedDnsRecordCleanup; use App\Support\ServiceComposeUrl; use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; @@ -12,6 +13,8 @@ class UpdateServiceApplicationFromApi public function execute(ServiceApplication $serviceApplication, Request $request, string $teamId, array $payload): ?JsonResponse { $forceDomainOverride = $request->boolean('force_domain_override'); + $dnsCleanup = app(ManagedDnsRecordCleanup::class); + $previousDnsHostnames = $dnsCleanup->hostnamesOf($serviceApplication->fresh() ?? $serviceApplication); if (array_key_exists('url', $payload)) { $urlRaw = $payload['url']; @@ -113,6 +116,7 @@ class UpdateServiceApplicationFromApi $serviceApplication->save(); $serviceApplication->refresh(); + $dnsCleanup->queueReleaseOfRemovedHostnames($serviceApplication, $previousDnsHostnames, (int) $teamId); updateCompose($serviceApplication); diff --git a/app/Enums/ManagedDnsDeletionResult.php b/app/Enums/ManagedDnsDeletionResult.php index a5b6311fd6..799a399320 100644 --- a/app/Enums/ManagedDnsDeletionResult.php +++ b/app/Enums/ManagedDnsDeletionResult.php @@ -19,8 +19,16 @@ enum ManagedDnsDeletionResult: string /** The provider could not be reached or rejected the request; retry later. */ case Failed = 'failed'; + /** Another change for the same hostname held the hostname lock; nothing was changed, retry later. */ + case Busy = 'busy'; + public function removedFromProvider(): bool { return $this === self::Deleted || $this === self::AlreadyGone; } + + public function shouldRetry(): bool + { + return $this === self::Failed || $this === self::Busy; + } } diff --git a/app/Http/Controllers/Api/ApplicationsController.php b/app/Http/Controllers/Api/ApplicationsController.php index f36bd48014..f721819976 100644 --- a/app/Http/Controllers/Api/ApplicationsController.php +++ b/app/Http/Controllers/Api/ApplicationsController.php @@ -23,6 +23,7 @@ use App\Models\SwarmDocker; use App\Rules\DockerImageFormat; use App\Rules\ValidGitBranch; use App\Rules\ValidGitRepositoryUrl; +use App\Services\Dns\ManagedDnsRecordCleanup; use App\Services\DockerImageParser; use App\Support\DomainPortOverrides; use App\Support\ValidationPatterns; @@ -2841,12 +2842,15 @@ class ApplicationsController extends Controller ], 409); } + $dnsCleanup = app(ManagedDnsRecordCleanup::class); + $previousDnsHostnames = $dnsCleanup->hostnamesOf($preview->fresh() ?? $preview); $preview->domain_port_overrides = $normalized['overrides']; $preview->fqdn = $portlessDomains; if ($isCompose) { $preview->docker_compose_domains = json_encode($dockerComposeDomains); } $preview->save(); + $dnsCleanup->queueReleaseOfRemovedHostnames($preview, $previousDnsHostnames, (int) $teamId); auditLog('api.application.preview_updated', [ 'team_id' => $teamId, @@ -3182,6 +3186,8 @@ class ApplicationsController extends Controller $this->authorize('deploy', $application); } + $dnsCleanup = app(ManagedDnsRecordCleanup::class); + $previousDnsHostnames = $dnsCleanup->hostnamesOf($application); $server = $application->destination->server; $allowedFields = ['name', 'description', 'is_static', 'is_spa', 'is_auto_deploy_enabled', 'is_force_https_enabled', 'is_preview_deployments_enabled', 'domains', 'noindex_domains', 'git_repository', 'git_branch', 'git_commit_sha', 'docker_registry_image_name', 'docker_registry_image_tag', 'build_pack', 'static_image', 'install_command', 'build_command', 'start_command', 'ports_exposes', 'ports_mappings', 'custom_network_aliases', 'base_directory', 'publish_directory', 'health_check_enabled', 'health_check_type', 'health_check_command', 'health_check_path', 'health_check_port', 'health_check_host', 'health_check_method', 'health_check_return_code', 'health_check_scheme', 'health_check_response_text', 'health_check_interval', 'health_check_timeout', 'health_check_retries', 'health_check_start_period', 'limits_memory', 'limits_memory_swap', 'limits_memory_swappiness', 'limits_memory_reservation', 'limits_cpus', 'limits_cpuset', 'limits_cpu_shares', 'custom_labels', 'custom_docker_run_options', 'post_deployment_command', 'post_deployment_command_container', 'pre_deployment_command', 'pre_deployment_command_container', 'watch_paths', 'manual_webhook_secret_github', 'manual_webhook_secret_gitlab', 'manual_webhook_secret_bitbucket', 'manual_webhook_secret_gitea', 'dockerfile_location', 'dockerfile_target_build', 'docker_compose_location', 'docker_compose_custom_start_command', 'docker_compose_custom_build_command', 'docker_compose_domains', 'redirect', 'instant_deploy', 'use_build_server', 'use_build_secrets', 'custom_nginx_configuration', 'is_http_basic_auth_enabled', 'http_basic_auth_username', 'http_basic_auth_password', 'connect_to_docker_network', 'force_domain_override', 'is_container_label_escape_enabled', 'is_preserve_repository_enabled', 'preview_url_template', 'max_restart_count', ...self::APPLICATION_SETTING_FIELDS]; @@ -3521,6 +3527,7 @@ class ApplicationsController extends Controller $application->custom_labels = str(implode('|coolify|', generateLabelsApplication($application)))->replace('|coolify|', "\n"); } $application->withoutAuditLogging(fn () => $application->save()); + $dnsCleanup->queueReleaseOfRemovedHostnames($application, $previousDnsHostnames, (int) $teamId); auditLog('api.application.updated', [ 'team_id' => $teamId, diff --git a/app/Http/Controllers/Api/ServicesController.php b/app/Http/Controllers/Api/ServicesController.php index fee94e874d..f78f2a0f3d 100644 --- a/app/Http/Controllers/Api/ServicesController.php +++ b/app/Http/Controllers/Api/ServicesController.php @@ -14,8 +14,10 @@ use App\Models\LocalPersistentVolume; use App\Models\Project; use App\Models\Server; use App\Models\Service; +use App\Models\ServiceApplication; use App\Models\StandaloneDocker; use App\Models\SwarmDocker; +use App\Services\Dns\ManagedDnsRecordCleanup; use App\Support\ValidationPatterns; use Illuminate\Database\Eloquent\Model; use Illuminate\Http\JsonResponse; @@ -114,6 +116,33 @@ class ServicesController extends Controller } } + /** + * Hostnames of every service application before an update, keyed by service application id. + * + * @return array> + */ + private function serviceApplicationDnsHostnames(Service $service): array + { + $cleanup = app(ManagedDnsRecordCleanup::class); + + return $service->applications()->get() + ->mapWithKeys(fn (ServiceApplication $application): array => [$application->id => $cleanup->hostnamesOf($application)]) + ->all(); + } + + /** + * Queues the release of managed DNS records for hostnames that a service application no longer uses. + * + * @param array> $previousHostnames + */ + private function queueReleaseOfRemovedServiceHostnames(Service $service, array $previousHostnames, int $teamId): void + { + $cleanup = app(ManagedDnsRecordCleanup::class); + $service->applications()->get() + ->filter(fn (ServiceApplication $application): bool => isset($previousHostnames[$application->id])) + ->each(fn (ServiceApplication $application) => $cleanup->queueReleaseOfRemovedHostnames($application, $previousHostnames[$application->id], $teamId)); + } + private function applyServiceUrls(Service $service, array $urlsArray, string $teamId, bool $forceDomainOverride = false): ?array { $errors = []; @@ -1271,26 +1300,30 @@ class ServicesController extends Controller if ($request->has('is_container_label_escape_enabled')) { $service->is_container_label_escape_enabled = $request->boolean('is_container_label_escape_enabled'); } + $previousDnsHostnames = $this->serviceApplicationDnsHostnames($service); $service->save(); $service->parse(); + $urlResult = null; if ($request->has('urls') && is_array($request->urls)) { $urlResult = $this->applyServiceUrls($service, $request->urls, $teamId, $request->boolean('force_domain_override')); - if ($urlResult !== null) { - if (isset($urlResult['errors'])) { - return response()->json([ - 'message' => 'Validation failed.', - 'errors' => $urlResult['errors'], - ], 422); - } - if (isset($urlResult['conflicts'])) { - return response()->json([ - 'message' => 'Domain conflicts detected. Use force_domain_override=true to proceed.', - 'conflicts' => $urlResult['conflicts'], - 'warning' => $urlResult['warning'], - ], 409); - } + } + // URLs of some containers can be saved before a later container fails, so release before any error response. + $this->queueReleaseOfRemovedServiceHostnames($service, $previousDnsHostnames, (int) $teamId); + if ($urlResult !== null) { + if (isset($urlResult['errors'])) { + return response()->json([ + 'message' => 'Validation failed.', + 'errors' => $urlResult['errors'], + ], 422); + } + if (isset($urlResult['conflicts'])) { + return response()->json([ + 'message' => 'Domain conflicts detected. Use force_domain_override=true to proceed.', + 'conflicts' => $urlResult['conflicts'], + 'warning' => $urlResult['warning'], + ], 409); } } diff --git a/app/Jobs/ConfigureDnsRecordJob.php b/app/Jobs/ConfigureDnsRecordJob.php index 4e0ea64718..f6d89ca531 100644 --- a/app/Jobs/ConfigureDnsRecordJob.php +++ b/app/Jobs/ConfigureDnsRecordJob.php @@ -5,6 +5,7 @@ namespace App\Jobs; use App\Events\DnsRecordConfigurationFinished; use App\Models\DnsProviderZone; use App\Services\Dns\CloudflareDnsProvider; +use Illuminate\Contracts\Cache\LockTimeoutException; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Relations\Relation; @@ -16,7 +17,10 @@ class ConfigureDnsRecordJob implements ShouldQueue { use Queueable, SerializesModels; - public int $tries = 1; + /** Only a busy hostname lock is retried; any other exception fails the job at once. */ + public int $tries = 3; + + public int $maxExceptions = 1; public int $timeout = 30; @@ -39,27 +43,36 @@ class ConfigureDnsRecordJob implements ShouldQueue try { $provider->createRecord($zone, $this->hostname, $this->content, $this->resource()); + } catch (LockTimeoutException) { + if ($this->attempts() < $this->tries) { + $this->release(10 * $this->attempts()); - DnsRecordConfigurationFinished::dispatch( - $this->teamId, - $this->resourceType, - $this->resourceId, - $this->hostname, - true, - $zone->integrationToken->name, - "DNS record added for {$this->hostname}.", - ); + return; + } + + $this->finished($zone, false, 'Another DNS change for this hostname is still in progress. Try again later.'); + + return; } catch (Throwable $exception) { - DnsRecordConfigurationFinished::dispatch( - $this->teamId, - $this->resourceType, - $this->resourceId, - $this->hostname, - false, - $zone->integrationToken->name, - $exception->getMessage(), - ); + $this->finished($zone, false, $exception->getMessage()); + + return; } + + $this->finished($zone, true, "DNS record added for {$this->hostname}."); + } + + private function finished(DnsProviderZone $zone, bool $successful, string $message): void + { + DnsRecordConfigurationFinished::dispatch( + $this->teamId, + $this->resourceType, + $this->resourceId, + $this->hostname, + $successful, + $zone->integrationToken->name, + $message, + ); } public function failed(?Throwable $exception): void diff --git a/app/Jobs/ReleaseRemovedDnsHostnamesJob.php b/app/Jobs/ReleaseRemovedDnsHostnamesJob.php new file mode 100644 index 0000000000..8090ff9a9f --- /dev/null +++ b/app/Jobs/ReleaseRemovedDnsHostnamesJob.php @@ -0,0 +1,77 @@ + $hostnames + */ + public function __construct( + public string $resourceType, + public int|string $resourceId, + public int $teamId, + public array $hostnames, + ) {} + + public function handle(ManagedDnsRecordCleanup $cleanup): void + { + $resource = $this->resource(); + if ($resource === null) { + // A deleted resource releases all of its records through ReleaseManagedDnsRecordsJob. + return; + } + + $retry = false; + foreach ($this->hostnames as $hostname) { + if ($cleanup->releaseHostname($resource, $hostname, $this->teamId)?->shouldRetry()) { + $retry = true; + } + } + if (! $retry) { + return; + } + + if ($this->attempts() < self::MAX_ATTEMPTS) { + $this->release(60 * $this->attempts()); + + return; + } + + Log::warning('Managed DNS records of removed hostnames could not be cleaned up; they were left in place.', [ + 'resource_type' => $this->resourceType, + 'resource_id' => $this->resourceId, + 'hostnames' => $this->hostnames, + ]); + } + + private function resource(): ?Model + { + $resourceClass = Relation::getMorphedModel($this->resourceType) ?? $this->resourceType; + if (! is_subclass_of($resourceClass, Model::class)) { + return null; + } + + return $resourceClass::query()->find($this->resourceId); + } +} diff --git a/app/Livewire/Concerns/InteractsWithDnsProviders.php b/app/Livewire/Concerns/InteractsWithDnsProviders.php index da47073011..2107e44984 100644 --- a/app/Livewire/Concerns/InteractsWithDnsProviders.php +++ b/app/Livewire/Concerns/InteractsWithDnsProviders.php @@ -9,6 +9,7 @@ use App\Models\DnsProviderZone; use App\Models\ManagedDnsRecord; use App\Services\Dns\CloudflareDnsProvider; use App\Services\Dns\ManagedDnsRecordCleanup; +use Illuminate\Contracts\Cache\LockTimeoutException; use Illuminate\Database\Eloquent\Model; trait InteractsWithDnsProviders @@ -50,8 +51,13 @@ trait InteractsWithDnsProviders return; } try { - $record = $cloudflare->createRecord($zone, $hostname, $content, $this->dnsResourceForHostname($hostname)); - $this->referenceDnsRecordFromAllResources($record, $hostname); + $cloudflare->createRecord( + $zone, + $hostname, + $content, + $this->dnsResourceForHostname($hostname), + $this->otherDnsResourcesForHostname($hostname), + ); $this->markDnsManaged($hostname, $zone->integrationToken->name); $this->dispatch('success', "DNS record created for {$hostname}."); $this->loadDnsProviderProposals(); @@ -59,6 +65,8 @@ trait InteractsWithDnsProviders $this->dnsProviderConflicts[$hostname.'|'.$zoneId] = [ 'record_id' => $e->providerRecordId, 'current' => $e->currentValue, 'proposed' => $e->proposedValue, ]; + } catch (LockTimeoutException) { + $this->dispatch('error', $this->dnsHostnameBusyMessage($hostname)); } catch (\Throwable $e) { $this->dispatch('error', $e->getMessage()); } @@ -149,18 +157,20 @@ trait InteractsWithDnsProviders return; } try { - $record = app(CloudflareDnsProvider::class)->replaceRecord( + app(CloudflareDnsProvider::class)->replaceRecord( $zone, (string) ($conflict['record_id'] ?? ''), $hostname, $content, $this->dnsResourceForHostname($hostname), (string) ($conflict['current'] ?? ''), + $this->otherDnsResourcesForHostname($hostname), ); - $this->referenceDnsRecordFromAllResources($record, $hostname); unset($this->dnsProviderConflicts[$key]); $this->dispatch('success', "DNS record replaced for {$hostname}."); $this->loadDnsProviderProposals(); + } catch (LockTimeoutException) { + $this->dispatch('error', $this->dnsHostnameBusyMessage($hostname)); } catch (\Throwable $e) { unset($this->dnsProviderConflicts[$key]); $this->dispatch('error', $e->getMessage()); @@ -241,27 +251,65 @@ trait InteractsWithDnsProviders } $result = app(ManagedDnsRecordCleanup::class)->releaseHostname($resource, $hostname, currentTeam()->id, $deleteRecord); + $this->reportManagedDnsRelease($result, 'removed'); + } + /** + * Captures the hostnames of a resource before a domain edit, for releaseManagedDnsForEditedDomains(). + * + * @return array + */ + protected function managedDnsHostnamesOf(Model $resource): array + { + return app(ManagedDnsRecordCleanup::class)->hostnamesOf($resource->fresh() ?? $resource); + } + + /** + * Releases the hostnames that the resource no longer uses after a domain edit, like a removed domain. + * An edit has no "also delete the DNS record" choice, so the safe default applies: only records that Coolify + * created and that no live resource uses any more are deleted; records changed outside Coolify stay. + * + * @param array $previousHostnames + */ + protected function releaseManagedDnsForEditedDomains(Model $resource, array $previousHostnames): void + { + $result = app(ManagedDnsRecordCleanup::class)->releaseRemovedHostnames($resource, $previousHostnames, currentTeam()->id); + $this->reportManagedDnsRelease($result, 'updated'); + } + + private function reportManagedDnsRelease(?ManagedDnsDeletionResult $result, string $change): void + { if ($result === ManagedDnsDeletionResult::ChangedExternally) { - $this->dispatch('warning', 'The domain was removed, but its DNS record changed externally and was left untouched.'); + $this->dispatch('warning', "The domain was {$change}, but its DNS record changed externally and was left untouched."); } elseif ($result === ManagedDnsDeletionResult::Failed) { - $this->dispatch('warning', 'The domain was removed, but its DNS record could not be deleted because Cloudflare did not respond. It was left untouched.'); + $this->dispatch('warning', "The domain was {$change}, but its DNS record could not be deleted because Cloudflare did not respond. It was left untouched."); + } elseif ($result === ManagedDnsDeletionResult::Busy) { + $this->dispatch('warning', "The domain was {$change}, but another DNS change for the same hostname was in progress. Its DNS record was left untouched."); } } + private function dnsHostnameBusyMessage(string $hostname): string + { + return "Another DNS change for {$hostname} is in progress. Try again in a few seconds."; + } + /** * Service domains can share a hostname across several service applications; each of them references the record. + * + * @return array */ - protected function referenceDnsRecordFromAllResources(ManagedDnsRecord $record, string $hostname): void + protected function otherDnsResourcesForHostname(string $hostname): array { if (property_exists($this, 'application') || ! property_exists($this, 'service') || $this->service === null) { - return; + return []; } $cleanup = app(ManagedDnsRecordCleanup::class); - $this->service->applications()->get() + + return $this->service->applications()->get() ->filter(fn (Model $application): bool => in_array(strtolower($hostname), $cleanup->hostnamesOf($application), true)) - ->each(fn (Model $application) => $record->addReference($application)); + ->values() + ->all(); } protected function authorizeDnsProviderChange(): void diff --git a/app/Livewire/Project/Application/Domains.php b/app/Livewire/Project/Application/Domains.php index a9b6bae5b8..7e9790f5d0 100644 --- a/app/Livewire/Project/Application/Domains.php +++ b/app/Livewire/Project/Application/Domains.php @@ -1564,10 +1564,12 @@ class Domains extends Component } $this->pendingAction = 'update'; + $previousDnsHostnames = $this->managedDnsHostnamesOf($this->application); if (! $this->saveDomainList($updated, $service, noindexDomains: $noindexDomains)) { return; } + $this->releaseManagedDnsForEditedDomains($this->application, $previousDnsHostnames); $this->resetDefaultLabels(); $this->forceSaveDomains = false; diff --git a/app/Livewire/Project/Application/General.php b/app/Livewire/Project/Application/General.php index 75742c8cd9..950bfc79d3 100644 --- a/app/Livewire/Project/Application/General.php +++ b/app/Livewire/Project/Application/General.php @@ -7,6 +7,7 @@ use App\Jobs\ApplicationDeploymentJob; use App\Livewire\Project\Service\Storage; use App\Models\Application; use App\Rules\ValidGitBranch; +use App\Services\Dns\ManagedDnsRecordCleanup; use App\Support\ValidationPatterns; use Exception; use Illuminate\Auth\Access\AuthorizationException; @@ -771,6 +772,8 @@ class General extends Component { try { $this->authorize('update', $this->application); + $dnsCleanup = app(ManagedDnsRecordCleanup::class); + $previousDnsHostnames = $dnsCleanup->hostnamesOf($this->application->fresh() ?? $this->application); $this->resetErrorBag(); @@ -911,6 +914,7 @@ class General extends Component $this->application->custom_labels = base64_encode($this->customLabels); $this->application->save(); $this->application->refresh(); + $dnsCleanup->queueReleaseOfRemovedHostnames($this->application, $previousDnsHostnames, currentTeam()->id); $this->syncData(); if ($oldPortsExposes !== $this->portsExposes) { $this->dispatch('applicationNetworkingUpdated')->to(InternalAccess::class); diff --git a/app/Livewire/Project/Application/PreviewDomains.php b/app/Livewire/Project/Application/PreviewDomains.php index 239a9d82e1..65ba5afba7 100644 --- a/app/Livewire/Project/Application/PreviewDomains.php +++ b/app/Livewire/Project/Application/PreviewDomains.php @@ -5,6 +5,7 @@ namespace App\Livewire\Project\Application; use App\Actions\Shared\CheckDomainDns; use App\Jobs\CheckDomainDnsJob; use App\Models\ApplicationPreview; +use App\Services\Dns\ManagedDnsRecordCleanup; use App\Support\DomainPortOverrides; use App\Support\DomainUrlParts; use App\Support\ValidationPatterns; @@ -446,6 +447,8 @@ class PreviewDomains extends Component private function persistDomains(): bool { + $dnsCleanup = app(ManagedDnsRecordCleanup::class); + $previousDnsHostnames = $dnsCleanup->hostnamesOf($this->preview->fresh() ?? $this->preview); if ($this->preview->application->build_pack === 'dockercompose') { try { $composeServices = $this->composeServices(failOnError: true); @@ -487,6 +490,7 @@ class PreviewDomains extends Component $this->domainRows[$index]['url'] = DomainPortOverrides::withoutPort($row['url']); } $this->preview->save(); + $dnsCleanup->queueReleaseOfRemovedHostnames($this->preview, $previousDnsHostnames, currentTeam()->id); $this->persistDnsStatuses(); $this->refreshDomains(); $this->dispatch('update_links'); diff --git a/app/Livewire/Project/Service/Domains.php b/app/Livewire/Project/Service/Domains.php index b078fb5e36..1a6dc91c7a 100644 --- a/app/Livewire/Project/Service/Domains.php +++ b/app/Livewire/Project/Service/Domains.php @@ -1322,11 +1322,14 @@ class Domains extends Component } } $this->pendingAction = 'update'; + $previousDnsHostnames = $this->managedDnsHostnamesOf($app); if (! $this->saveDomainListForApp($app, $updated, noindexDomains: $noindexDomains, redirect: $this->editingRedirect)) { return; } + $this->releaseManagedDnsForEditedDomains($app, $previousDnsHostnames); + $this->cancelEdit(); $this->dispatch('edit-domain-saved'); $this->forceSaveDomains = false; diff --git a/app/Livewire/Project/Service/Index.php b/app/Livewire/Project/Service/Index.php index adaaa6512a..dbba532261 100644 --- a/app/Livewire/Project/Service/Index.php +++ b/app/Livewire/Project/Service/Index.php @@ -9,6 +9,7 @@ use App\Models\Server; use App\Models\Service; use App\Models\ServiceApplication; use App\Models\ServiceDatabase; +use App\Services\Dns\ManagedDnsRecordCleanup; use App\Support\ValidationPatterns; use Illuminate\Foundation\Auth\Access\AuthorizesRequests; use Illuminate\Support\Collection; @@ -571,6 +572,7 @@ class Index extends Component { try { $persistedApplication = $this->serviceApplication->fresh(); + $previousDnsHostnames = app(ManagedDnsRecordCleanup::class)->hostnamesOf($persistedApplication); $previousEditableUrls = $persistedApplication->url; $previousFqdn = $persistedApplication->fqdn; $previousPortOverrides = $persistedApplication->domain_port_overrides; @@ -626,6 +628,7 @@ class Index extends Component $this->validate(); $this->serviceApplication->save(); $this->serviceApplication->refresh(); + app(ManagedDnsRecordCleanup::class)->queueReleaseOfRemovedHostnames($this->serviceApplication, $previousDnsHostnames, currentTeam()->id); $this->syncApplicationData(false); updateCompose($this->serviceApplication); if (str($this->serviceApplication->fqdn)->contains(',')) { diff --git a/app/Services/Dns/CloudflareDnsProvider.php b/app/Services/Dns/CloudflareDnsProvider.php index 12537d7b52..99a6e853a4 100644 --- a/app/Services/Dns/CloudflareDnsProvider.php +++ b/app/Services/Dns/CloudflareDnsProvider.php @@ -7,6 +7,7 @@ use App\Exceptions\DnsRecordConflictException; use App\Models\DnsProviderZone; use App\Models\IntegrationToken; use App\Models\ManagedDnsRecord; +use Illuminate\Contracts\Cache\LockTimeoutException; use Illuminate\Database\Eloquent\Model; use Illuminate\Http\Client\PendingRequest; use Illuminate\Support\Collection; @@ -20,6 +21,8 @@ class CloudflareDnsProvider /** @var array> */ private array $zoneCache = []; + public function __construct(private ManagedDnsHostnameLock $hostnameLock) {} + public function syncZones(IntegrationToken $token): int { unset($this->zoneCache[$token->team_id]); @@ -100,10 +103,33 @@ class CloudflareDnsProvider /** * Creates the record with Coolify's ownership comment, or references an existing record that already has the wanted content. * Existing records that Coolify did not create are only referenced and never marked as owned. + * Runs under the hostname lock and always checks the provider again, so a create that waited for a release + * creates the record again when the release deleted it. + * + * @param array $additionalResources Other resources that use the hostname and also reference the record. + * + * @throws LockTimeoutException when another change for the hostname does not finish in time */ - public function createRecord(DnsProviderZone $zone, string $hostname, string $content, ?Model $resource = null): ManagedDnsRecord + public function createRecord(DnsProviderZone $zone, string $hostname, string $content, ?Model $resource = null, array $additionalResources = []): ManagedDnsRecord { $hostname = strtolower(rtrim($hostname, '.')); + + return $this->hostnameLock->run( + (int) $zone->integrationToken->team_id, + $hostname, + function () use ($zone, $hostname, $content, $resource, $additionalResources): ManagedDnsRecord { + $record = $this->createRecordLocked($zone, $hostname, $content, $resource); + foreach ($additionalResources as $additionalResource) { + $record->addReference($additionalResource); + } + + return $record; + }, + ); + } + + private function createRecordLocked(DnsProviderZone $zone, string $hostname, string $content, ?Model $resource): ManagedDnsRecord + { $type = $this->recordType($content); $remoteRecords = $this->findRecords($zone, $hostname, $type); $matching = collect($remoteRecords)->first(fn (array $remote): bool => $remote['content'] === $content); @@ -149,6 +175,10 @@ class CloudflareDnsProvider /** * Points an existing (conflicting) record at the new content after explicit user confirmation. * Only the content changes: proxy status, TTL, comment and other settings stay as they are. + * + * @param array $additionalResources Other resources that use the hostname and also reference the record. + * + * @throws LockTimeoutException when another change for the hostname does not finish in time */ public function replaceRecord( DnsProviderZone $zone, @@ -157,8 +187,32 @@ class CloudflareDnsProvider string $content, ?Model $resource = null, ?string $expectedCurrent = null, + array $additionalResources = [], ): ManagedDnsRecord { $hostname = strtolower(rtrim($hostname, '.')); + + return $this->hostnameLock->run( + (int) $zone->integrationToken->team_id, + $hostname, + function () use ($zone, $recordId, $hostname, $content, $resource, $expectedCurrent, $additionalResources): ManagedDnsRecord { + $record = $this->replaceRecordLocked($zone, $recordId, $hostname, $content, $resource, $expectedCurrent); + foreach ($additionalResources as $additionalResource) { + $record->addReference($additionalResource); + } + + return $record; + }, + ); + } + + private function replaceRecordLocked( + DnsProviderZone $zone, + string $recordId, + string $hostname, + string $content, + ?Model $resource, + ?string $expectedCurrent, + ): ManagedDnsRecord { $type = $this->recordType($content); $remoteRecords = $this->findRecords($zone, $hostname, $type); if (count($remoteRecords) > 1) { diff --git a/app/Services/Dns/ManagedDnsHostnameLock.php b/app/Services/Dns/ManagedDnsHostnameLock.php new file mode 100644 index 0000000000..1afb002fe8 --- /dev/null +++ b/app/Services/Dns/ManagedDnsHostnameLock.php @@ -0,0 +1,41 @@ +ttlSeconds)->block($this->waitSeconds, $callback); + } +} diff --git a/app/Services/Dns/ManagedDnsRecordCleanup.php b/app/Services/Dns/ManagedDnsRecordCleanup.php index 32f112786b..06b1cecce4 100644 --- a/app/Services/Dns/ManagedDnsRecordCleanup.php +++ b/app/Services/Dns/ManagedDnsRecordCleanup.php @@ -3,11 +3,13 @@ namespace App\Services\Dns; use App\Enums\ManagedDnsDeletionResult; +use App\Jobs\ReleaseRemovedDnsHostnamesJob; use App\Models\Application; use App\Models\ApplicationPreview; use App\Models\ManagedDnsRecord; use App\Models\ManagedDnsRecordReference; use App\Models\ServiceApplication; +use Illuminate\Contracts\Cache\LockTimeoutException; use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Model; use Illuminate\Support\Collection; @@ -21,7 +23,69 @@ use Throwable; */ class ManagedDnsRecordCleanup { - public function __construct(private CloudflareDnsProvider $provider) {} + public function __construct( + private CloudflareDnsProvider $provider, + private ManagedDnsHostnameLock $hostnameLock, + ) {} + + /** + * Hostnames the resource used before a domain change (from hostnamesOf()) and no longer uses now. + * + * @param array $previousHostnames + * @return array + */ + public function removedHostnames(Model $resource, array $previousHostnames): array + { + $current = $this->hostnamesOf($resource->fresh() ?? $resource); + + return collect($previousHostnames) + ->map(fn (string $hostname): string => $this->normalizeHostname($hostname)) + ->reject(fn (string $hostname): bool => in_array($hostname, $current, true)) + ->unique() + ->values() + ->all(); + } + + /** + * Releases every hostname that the resource no longer uses after a domain edit. The same rules as for a removed + * domain apply: records still used by another live resource stay, and only owned records are deleted. + * + * @param array $previousHostnames + */ + public function releaseRemovedHostnames(Model $resource, array $previousHostnames, int $teamId, bool $deleteRecord = true): ?ManagedDnsDeletionResult + { + $worst = null; + foreach ($this->removedHostnames($resource, $previousHostnames) as $hostname) { + $worst = $this->worseResult($worst, $this->releaseHostname($resource, $hostname, $teamId, $deleteRecord)); + } + + return $worst; + } + + /** + * Queues the release of hostnames the resource no longer uses after a domain edit (API and other non-interactive + * paths). Nothing is queued when the resource references no managed record for a removed hostname. + * + * @param array $previousHostnames + */ + public function queueReleaseOfRemovedHostnames(Model $resource, array $previousHostnames, int $teamId): void + { + $removed = $this->removedHostnames($resource, $previousHostnames); + if ($removed === []) { + return; + } + + $referenced = ManagedDnsRecord::query() + ->where('team_id', $teamId) + ->whereIn('name', $removed) + ->whereHas('references', fn (Builder $query) => $this->whereResourceKeys($query, [[$resource->getMorphClass(), $resource->getKey()]])) + ->exists(); + if (! $referenced) { + return; + } + + ReleaseRemovedDnsHostnamesJob::dispatch($resource->getMorphClass(), $resource->getKey(), $teamId, $removed)->afterCommit(); + } /** * Releases the resource's reference after one of its URLs was removed. @@ -44,10 +108,7 @@ class ManagedDnsRecordCleanup $worst = null; foreach ($records as $record) { - $result = $this->release($record, $keys, $deleteRecord, $resource); - if ($result === ManagedDnsDeletionResult::ChangedExternally || $result === ManagedDnsDeletionResult::Failed) { - $worst = $worst === ManagedDnsDeletionResult::Failed ? $worst : $result; - } + $worst = $this->worseResult($worst, $this->release($record, $keys, $deleteRecord, $resource)); } return $worst; @@ -67,7 +128,7 @@ class ManagedDnsRecordCleanup $successful = true; foreach ($records as $record) { try { - if ($this->release($record, $keys, true) === ManagedDnsDeletionResult::Failed) { + if ($this->release($record, $keys, true)?->shouldRetry()) { $successful = false; } } catch (Throwable $e) { @@ -84,10 +145,41 @@ class ManagedDnsRecordCleanup } /** + * Runs under the hostname lock, so a concurrent create or reference for the same hostname either finishes before + * the usage re-check or starts after the record is gone. When the lock is not free in time, nothing changes and + * Busy is returned so the caller can retry. + * * @param array $releasingKeys + * @param Model|null $contextResource The releasing resource of a hostname release; it keeps the record when it uses the hostname again. */ public function release(ManagedDnsRecord $record, array $releasingKeys, bool $deleteRecord, ?Model $contextResource = null): ?ManagedDnsDeletionResult { + try { + return $this->hostnameLock->run( + (int) $record->team_id, + $record->name, + fn (): ?ManagedDnsDeletionResult => $this->releaseLocked($record, $releasingKeys, $deleteRecord, $contextResource), + ); + } catch (LockTimeoutException) { + $this->auditSkipped($record, 'hostname_busy'); + + return ManagedDnsDeletionResult::Busy; + } + } + + /** + * @param array $releasingKeys + */ + private function releaseLocked(ManagedDnsRecord $record, array $releasingKeys, bool $deleteRecord, ?Model $contextResource): ?ManagedDnsDeletionResult + { + $record = $record->fresh(); + if ($record === null) { + return null; + } + if ($contextResource !== null && in_array($record->name, $this->hostnamesOf($contextResource->fresh() ?? $contextResource), true)) { + return null; + } + $releasing = collect($releasingKeys)->map(fn (array $key): string => $key[0].'|'.$key[1]); $otherReferences = $record->references()->get() ->reject(fn (ManagedDnsRecordReference $reference): bool => $releasing->contains($reference->resource_type.'|'.$reference->resource_id)); @@ -184,6 +276,18 @@ class ManagedDnsRecordCleanup ->all(); } + private function worseResult(?ManagedDnsDeletionResult $current, ?ManagedDnsDeletionResult $next): ?ManagedDnsDeletionResult + { + $rank = fn (?ManagedDnsDeletionResult $result): int => match ($result) { + ManagedDnsDeletionResult::Failed => 3, + ManagedDnsDeletionResult::Busy => 2, + ManagedDnsDeletionResult::ChangedExternally => 1, + default => 0, + }; + + return $rank($next) > $rank($current) ? $next : $current; + } + private function teamIdOf(Model $resource): ?int { $teamId = match (true) { diff --git a/tests/Feature/ManagedDnsRecordEditReleaseTest.php b/tests/Feature/ManagedDnsRecordEditReleaseTest.php new file mode 100644 index 0000000000..7beff03e04 --- /dev/null +++ b/tests/Feature/ManagedDnsRecordEditReleaseTest.php @@ -0,0 +1,616 @@ +withoutVite(); + Queue::fake([CheckDomainDnsJob::class]); + config()->set('app.maintenance.store', 'array'); + config()->set('app.id', 'test-instance'); + + InstanceSettings::unguarded(fn () => InstanceSettings::updateOrCreate( + ['id' => 0], + ['id' => 0, 'is_dns_validation_enabled' => false, 'is_api_enabled' => true], + )); + + $this->team = Team::factory()->create(); + $this->user = User::factory()->create(); + $this->team->members()->attach($this->user->id, ['role' => 'owner']); + $this->actingAs($this->user); + session(['currentTeam' => $this->team]); + + $this->server = Server::factory()->create(['team_id' => $this->team->id, 'ip' => '203.0.113.10']); + $this->server->settings()->update(['is_reachable' => true, 'is_usable' => true]); + $this->destination = StandaloneDocker::withoutEvents(fn () => StandaloneDocker::firstOrCreate( + ['server_id' => $this->server->id, 'network' => 'coolify'], + ['uuid' => (string) Str::uuid(), 'name' => 'test-docker'], + )); + $this->project = Project::factory()->create(['team_id' => $this->team->id]); + $this->environment = Environment::factory()->create(['project_id' => $this->project->id]); + + $this->token = IntegrationToken::factory()->for($this->team)->create(['provider' => 'cloudflare', 'token' => 'secret']); + $this->zone = DnsProviderZone::factory()->for($this->token)->create(['provider_zone_id' => 'zone-1', 'name' => 'example.com']); +}); + +/** + * A hostname lock that runs a callback right before the next lock is taken, to simulate a concurrent change + * that happens between the release query and the lock. + */ +class InterleavingManagedDnsHostnameLock extends ManagedDnsHostnameLock +{ + public ?Closure $beforeNextRun = null; + + public function run(int $teamId, string $hostname, callable $callback): mixed + { + if ($this->beforeNextRun !== null) { + $interleaved = $this->beforeNextRun; + $this->beforeNextRun = null; + $interleaved(); + } + + return parent::run($teamId, $hostname, $callback); + } +} + +function editReleaseApiToken(User $user, Team $team, array $abilities = ['*']): string +{ + $plainTextToken = Str::random(40); + $token = $user->tokens()->create([ + 'name' => 'dns-edit-release-'.Str::random(6), + 'token' => hash('sha256', $plainTextToken), + 'abilities' => $abilities, + 'team_id' => $team->id, + ]); + auth()->logout(); + + return $token->getKey().'|'.$plainTextToken; +} + +function editReleaseService(object $test, string $fqdn): ServiceApplication +{ + $service = Service::factory()->create([ + 'server_id' => $test->server->id, + 'destination_id' => $test->destination->id, + 'destination_type' => $test->destination->getMorphClass(), + 'environment_id' => $test->environment->id, + 'docker_compose_raw' => "services:\n web:\n image: nginx:alpine\n", + ]); + + return ServiceApplication::create([ + 'uuid' => (string) Str::uuid(), 'service_id' => $service->id, 'name' => 'web', 'human_name' => 'Web', + 'image' => 'nginx:alpine', 'fqdn' => $fqdn, + ]); +} + +function editApplicationDomain(Application $application, string $newHost): void +{ + Livewire::test(Domains::class, ['application' => $application->fresh()]) + ->call('startEdit', 0) + ->set('editingDomainParts.scheme', 'https') + ->set('editingDomainParts.host', $newHost) + ->call('updateDomain') + ->assertHasNoErrors(); +} + +describe('UI application domain edit', function () { + test('an edited hostname releases and deletes its owned record', function () { + $cloudflare = fakeCloudflareDns(); + $application = createDnsTestApplication($this, 'https://app.example.com'); + $record = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'app.example.com', '203.0.113.10', $application); + + Livewire::test(Domains::class, ['application' => $application]) + ->call('startEdit', 0) + ->set('editingDomainParts.host', 'new.example.com') + ->call('updateDomain') + ->assertDispatched('success', 'Domain updated.') + ->assertNotDispatched('warning'); + + expect($application->fresh()->fqdn)->toBe('https://new.example.com') + ->and(sentDnsRequests('DELETE'))->toBe(1) + ->and($cloudflare['records'])->toBeEmpty() + ->and($record->fresh())->toBeNull(); + }); + + test('an edit that keeps the hostname keeps the record', function () { + $cloudflare = fakeCloudflareDns(); + $application = createDnsTestApplication($this, 'https://app.example.com'); + $record = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'app.example.com', '203.0.113.10', $application); + + Livewire::test(Domains::class, ['application' => $application]) + ->call('startEdit', 0) + ->set('editingDomainParts.scheme', 'http') + ->set('editingDomainParts.path', '/api') + ->call('updateDomain') + ->assertDispatched('success', 'Domain updated.'); + + expect($application->fresh()->fqdn)->toBe('http://app.example.com/api') + ->and(sentDnsRequests('DELETE'))->toBe(0) + ->and($cloudflare['records'])->toHaveCount(1) + ->and($record->references()->pluck('resource_id')->all())->toBe([$application->id]); + }); + + test('a hostname still used by another application of the team keeps the record', function () { + $cloudflare = fakeCloudflareDns(); + $application = createDnsTestApplication($this, 'https://app.example.com'); + $record = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'app.example.com', '203.0.113.10', $application); + $other = createDnsTestApplication($this, 'https://app.example.com/other'); + + editApplicationDomain($application, 'new.example.com'); + + expect(sentDnsRequests('DELETE'))->toBe(0) + ->and($cloudflare['records'])->toHaveCount(1) + ->and($record->references()->pluck('resource_id')->all())->toBe([$other->id]); + }); + + test('records of another team are never touched', function () { + $cloudflare = fakeCloudflareDns([[ + 'id' => 'record-foreign', 'type' => 'A', 'name' => 'app.example.com', 'content' => '203.0.113.10', 'comment' => null, + ]]); + $application = createDnsTestApplication($this, 'https://app.example.com'); + $otherToken = IntegrationToken::factory()->for(Team::factory()->create())->create(['provider' => 'cloudflare', 'token' => 'other']); + $otherZone = DnsProviderZone::factory()->for($otherToken)->create(['provider_zone_id' => 'zone-2', 'name' => 'example.com']); + $foreignRecord = ManagedDnsRecord::factory()->owned()->create([ + 'dns_provider_zone_id' => $otherZone->id, 'provider_record_id' => 'record-foreign', + 'type' => 'A', 'name' => 'app.example.com', 'content' => '203.0.113.10', + ]); + $foreignRecord->addReference($application); + + editApplicationDomain($application, 'new.example.com'); + + expect(Http::recorded())->toBeEmpty() + ->and($cloudflare['records'])->toHaveKey('record-foreign') + ->and($foreignRecord->fresh())->not->toBeNull() + ->and($foreignRecord->references()->count())->toBe(1); + }); + + test('a member without update permission cannot release a record by editing', function () { + $cloudflare = fakeCloudflareDns(); + $application = createDnsTestApplication($this, 'https://app.example.com'); + $record = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'app.example.com', '203.0.113.10', $application); + $this->team->members()->updateExistingPivot($this->user->id, ['role' => 'member']); + $this->actingAs($this->user->fresh()); + + Livewire::test(Domains::class, ['application' => $application->fresh()]) + ->set('editingIndex', 0) + ->set('editingDomainParts.host', 'new.example.com') + ->call('updateDomain') + ->assertNotDispatched('success'); + + expect($application->fresh()->fqdn)->toBe('https://app.example.com') + ->and(sentDnsRequests('DELETE'))->toBe(0) + ->and($cloudflare['records'])->toHaveCount(1) + ->and($record->fresh())->not->toBeNull(); + }); + + test('an adopted record is forgotten but never deleted', function () { + $cloudflare = fakeCloudflareDns([[ + 'id' => 'record-1', 'type' => 'A', 'name' => 'app.example.com', 'content' => '203.0.113.10', 'comment' => 'hand made', + ]]); + $application = createDnsTestApplication($this, 'https://app.example.com'); + $record = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'app.example.com', '203.0.113.10', $application); + + Livewire::test(Domains::class, ['application' => $application]) + ->call('startEdit', 0) + ->set('editingDomainParts.host', 'new.example.com') + ->call('updateDomain') + ->assertDispatched('success', 'Domain updated.') + ->assertNotDispatched('warning'); + + expect($record->owned)->toBeFalse() + ->and(sentDnsRequests('DELETE'))->toBe(0) + ->and($cloudflare['records'])->toHaveKey('record-1') + ->and($record->fresh())->toBeNull(); + }); + + test('a record changed outside Coolify stays and a warning is shown', function () { + $cloudflare = fakeCloudflareDns(); + $application = createDnsTestApplication($this, 'https://app.example.com'); + $record = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'app.example.com', '203.0.113.10', $application); + $records = $cloudflare['records']; + $records[$record->provider_record_id]['comment'] = 'production record'; + $cloudflare['records'] = $records; + + Livewire::test(Domains::class, ['application' => $application]) + ->call('startEdit', 0) + ->set('editingDomainParts.host', 'new.example.com') + ->call('updateDomain') + ->assertDispatched('warning', 'The domain was updated, but its DNS record changed externally and was left untouched.'); + + expect(sentDnsRequests('DELETE'))->toBe(0) + ->and($cloudflare['records'])->toHaveKey($record->provider_record_id) + ->and($record->fresh())->toBeNull(); + }); + + test('an unreachable provider keeps the record and shows a warning', function () { + fakeCloudflareDns(); + $application = createDnsTestApplication($this, 'https://app.example.com'); + $record = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'app.example.com', '203.0.113.10', $application); + Http::fake(fn () => throw new ConnectionException('Cloudflare is unreachable.')); + + Livewire::test(Domains::class, ['application' => $application]) + ->call('startEdit', 0) + ->set('editingDomainParts.host', 'new.example.com') + ->call('updateDomain') + ->assertDispatched('warning', 'The domain was updated, but its DNS record could not be deleted because Cloudflare did not respond. It was left untouched.'); + + expect($application->fresh()->fqdn)->toBe('https://new.example.com') + ->and($record->fresh())->not->toBeNull() + ->and($record->references()->count())->toBe(1); + }); +}); + +describe('UI service domain edit', function () { + test('an edited hostname releases and deletes its owned record', function () { + $cloudflare = fakeCloudflareDns(); + $serviceApplication = editReleaseService($this, 'https://app.example.com'); + $record = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'app.example.com', '203.0.113.10', $serviceApplication); + + Livewire::test(ServiceDomains::class, ['service' => $serviceApplication->service->fresh(['applications', 'server'])]) + ->call('startEdit', 0) + ->set('editingDomainParts.host', 'new.example.com') + ->call('updateDomain') + ->assertDispatched('success', 'Domain updated.') + ->assertNotDispatched('warning'); + + expect($serviceApplication->fresh()->fqdn)->toBe('https://new.example.com') + ->and(sentDnsRequests('DELETE'))->toBe(1) + ->and($cloudflare['records'])->toBeEmpty() + ->and($record->fresh())->toBeNull(); + }); + + test('a member without update permission cannot release a record by editing', function () { + $cloudflare = fakeCloudflareDns(); + $serviceApplication = editReleaseService($this, 'https://app.example.com'); + $record = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'app.example.com', '203.0.113.10', $serviceApplication); + $this->team->members()->updateExistingPivot($this->user->id, ['role' => 'member']); + $this->actingAs($this->user->fresh()); + + Livewire::test(ServiceDomains::class, ['service' => $serviceApplication->service->fresh(['applications', 'server'])]) + ->set('editingIndex', 0) + ->set('editingServiceApplicationId', $serviceApplication->id) + ->set('editingDomainParts.host', 'new.example.com') + ->call('updateDomain') + ->assertNotDispatched('success'); + + expect($serviceApplication->fresh()->fqdn)->toBe('https://app.example.com') + ->and(sentDnsRequests('DELETE'))->toBe(0) + ->and($cloudflare['records'])->toHaveCount(1) + ->and($record->fresh())->not->toBeNull(); + }); +}); + +describe('API domain edit', function () { + test('an application domain update releases the removed hostname', function () { + $cloudflare = fakeCloudflareDns(); + $application = createDnsTestApplication($this, 'https://app.example.com,https://keep.example.com'); + $provider = app(CloudflareDnsProvider::class); + $removed = $provider->createRecord($this->zone, 'app.example.com', '203.0.113.10', $application); + $kept = $provider->createRecord($this->zone, 'keep.example.com', '203.0.113.10', $application); + + $this->withToken(editReleaseApiToken($this->user, $this->team)) + ->patchJson("/api/v1/applications/{$application->uuid}", ['domains' => 'https://keep.example.com,https://new.example.com']) + ->assertOk(); + + expect($application->fresh()->fqdn)->toBe('https://keep.example.com,https://new.example.com') + ->and(sentDnsRequests('DELETE'))->toBe(1) + ->and($removed->fresh())->toBeNull() + ->and($kept->fresh())->not->toBeNull() + ->and($cloudflare['records'])->toHaveCount(1); + }); + + test('an application docker compose domain update releases the removed hostname', function () { + $cloudflare = fakeCloudflareDns(); + $application = createDnsTestApplication($this, 'https://unused.example.org'); + $application->update([ + 'build_pack' => 'dockercompose', + 'fqdn' => null, + 'docker_compose_raw' => "services:\n web:\n image: nginx:alpine\n api:\n image: nginx:alpine\n", + 'docker_compose_domains' => json_encode([ + 'web' => ['domain' => 'https://web.example.com'], + 'api' => ['domain' => 'https://api.example.com'], + ]), + ]); + $record = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'api.example.com', '203.0.113.10', $application); + + $this->withToken(editReleaseApiToken($this->user, $this->team)) + ->patchJson("/api/v1/applications/{$application->uuid}", ['docker_compose_domains' => [ + ['name' => 'web', 'domain' => 'https://web.example.com'], + ['name' => 'api', 'domain' => 'https://api2.example.com'], + ]]) + ->assertOk(); + + expect(sentDnsRequests('DELETE'))->toBe(1) + ->and($cloudflare['records'])->toBeEmpty() + ->and($record->fresh())->toBeNull(); + }); + + test('a service domain update releases the removed hostname', function (string $endpoint) { + $cloudflare = fakeCloudflareDns(); + $serviceApplication = editReleaseService($this, 'https://app.example.com'); + $record = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'app.example.com', '203.0.113.10', $serviceApplication); + $service = $serviceApplication->service; + [$path, $payload] = $endpoint === 'service' + ? ["/api/v1/services/{$service->uuid}", ['urls' => [['name' => 'web', 'url' => 'https://new.example.com']]]] + : ["/api/v1/services/{$service->uuid}/applications/{$serviceApplication->uuid}", ['url' => 'https://new.example.com']]; + + $this->withToken(editReleaseApiToken($this->user, $this->team))->patchJson($path, $payload)->assertOk(); + + expect($serviceApplication->fresh()->fqdn)->toBe('https://new.example.com') + ->and(sentDnsRequests('DELETE'))->toBe(1) + ->and($cloudflare['records'])->toBeEmpty() + ->and($record->fresh())->toBeNull(); + })->with(['service', 'service application']); + + test('a preview domain update releases the removed hostname', function () { + $cloudflare = fakeCloudflareDns(); + $application = createDnsTestApplication($this, 'https://main.example.org'); + $preview = ApplicationPreview::query()->create([ + 'application_id' => $application->id, 'pull_request_id' => 5, 'pull_request_html_url' => 'https://github.com/x/y/pull/5', + 'fqdn' => 'https://pr-5.example.com', + ]); + $record = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'pr-5.example.com', '203.0.113.10', $preview); + + $this->withToken(editReleaseApiToken($this->user, $this->team)) + ->patchJson("/api/v1/applications/{$application->uuid}/previews/5", ['domains' => 'https://pr-5-new.example.com']) + ->assertOk(); + + expect(sentDnsRequests('DELETE'))->toBe(1) + ->and($cloudflare['records'])->toBeEmpty() + ->and($record->fresh())->toBeNull(); + }); + + test('a member token cannot release a record by editing', function () { + $cloudflare = fakeCloudflareDns(); + $application = createDnsTestApplication($this, 'https://app.example.com'); + $record = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'app.example.com', '203.0.113.10', $application); + $this->team->members()->updateExistingPivot($this->user->id, ['role' => 'member']); + + $this->withToken(editReleaseApiToken($this->user->fresh(), $this->team)) + ->patchJson("/api/v1/applications/{$application->uuid}", ['domains' => 'https://new.example.com']) + ->assertForbidden(); + + expect($application->fresh()->fqdn)->toBe('https://app.example.com') + ->and(sentDnsRequests('DELETE'))->toBe(0) + ->and($cloudflare['records'])->toHaveCount(1) + ->and($record->fresh())->not->toBeNull(); + }); + + test('a token of another team cannot release a record by editing', function () { + $cloudflare = fakeCloudflareDns(); + $application = createDnsTestApplication($this, 'https://app.example.com'); + $record = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'app.example.com', '203.0.113.10', $application); + $otherTeam = Team::factory()->create(); + $otherUser = User::factory()->create(); + $otherTeam->members()->attach($otherUser->id, ['role' => 'owner']); + + $this->withToken(editReleaseApiToken($otherUser, $otherTeam)) + ->patchJson("/api/v1/applications/{$application->uuid}", ['domains' => 'https://new.example.com']) + ->assertNotFound(); + + expect(sentDnsRequests('DELETE'))->toBe(0) + ->and($cloudflare['records'])->toHaveCount(1) + ->and($record->fresh())->not->toBeNull(); + }); + + test('nothing is queued when the resource references no managed record', function () { + Queue::fake([ReleaseRemovedDnsHostnamesJob::class]); + fakeCloudflareDns(); + $application = createDnsTestApplication($this, 'https://app.example.com'); + + $this->withToken(editReleaseApiToken($this->user, $this->team)) + ->patchJson("/api/v1/applications/{$application->uuid}", ['domains' => 'https://new.example.com']) + ->assertOk(); + + Queue::assertNotPushed(ReleaseRemovedDnsHostnamesJob::class); + }); + + test('the queued release retries while cloudflare is unreachable', function () { + fakeCloudflareDns(); + $application = createDnsTestApplication($this, 'https://new.example.com'); + $record = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'app.example.com', '203.0.113.10', $application); + Http::fake(fn () => throw new ConnectionException('Cloudflare is unreachable.')); + + $job = (new ReleaseRemovedDnsHostnamesJob($application->getMorphClass(), $application->id, $this->team->id, ['app.example.com'])) + ->withFakeQueueInteractions(); + $job->handle(app(ManagedDnsRecordCleanup::class)); + + $job->assertReleased(60); + expect($record->fresh())->not->toBeNull(); + }); +}); + +describe('hostname lock', function () { + test('a release while the hostname lock is held keeps the record and warns', function () { + $cloudflare = fakeCloudflareDns(); + app()->instance(ManagedDnsHostnameLock::class, new ManagedDnsHostnameLock(waitSeconds: 0)); + $application = createDnsTestApplication($this, 'https://app.example.com'); + $record = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'app.example.com', '203.0.113.10', $application); + $heldLock = Cache::lock(ManagedDnsHostnameLock::key($this->team->id, 'app.example.com'), 60); + expect($heldLock->get())->toBeTrue(); + + Livewire::test(Domains::class, ['application' => $application]) + ->call('startEdit', 0) + ->set('editingDomainParts.host', 'new.example.com') + ->call('updateDomain') + ->assertDispatched('warning', 'The domain was updated, but another DNS change for the same hostname was in progress. Its DNS record was left untouched.'); + + expect(sentDnsRequests('DELETE'))->toBe(0) + ->and($cloudflare['records'])->toHaveCount(1) + ->and($record->fresh())->not->toBeNull() + ->and($record->references()->count())->toBe(1); + $heldLock->release(); + }); + + test('the release job of a deleted resource retries while the hostname lock is held', function () { + $cloudflare = fakeCloudflareDns(); + app()->instance(ManagedDnsHostnameLock::class, new ManagedDnsHostnameLock(waitSeconds: 0)); + $application = createDnsTestApplication($this, 'https://app.example.com'); + $record = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'app.example.com', '203.0.113.10', $application); + Application::withoutEvents(fn () => $application->delete()); + $heldLock = Cache::lock(ManagedDnsHostnameLock::key($this->team->id, 'app.example.com'), 60); + $heldLock->get(); + + $job = (new ReleaseManagedDnsRecordsJob($application->getMorphClass(), $application->id))->withFakeQueueInteractions(); + $job->handle(app(ManagedDnsRecordCleanup::class)); + + $job->assertReleased(60); + expect(sentDnsRequests('DELETE'))->toBe(0)->and($record->fresh())->not->toBeNull(); + + $heldLock->release(); + $retry = (new ReleaseManagedDnsRecordsJob($application->getMorphClass(), $application->id))->withFakeQueueInteractions(); + $retry->handle(app(ManagedDnsRecordCleanup::class)); + + $retry->assertNotReleased(); + expect(sentDnsRequests('DELETE'))->toBe(1) + ->and($cloudflare['records'])->toBeEmpty() + ->and($record->fresh())->toBeNull(); + }); + + test('a create while a release holds the lock is retried and then creates the record', function () { + fakeCloudflareDns(); + app()->instance(ManagedDnsHostnameLock::class, new ManagedDnsHostnameLock(waitSeconds: 0)); + $application = createDnsTestApplication($this, 'https://app.example.com'); + $heldLock = Cache::lock(ManagedDnsHostnameLock::key($this->team->id, 'app.example.com'), 60); + $heldLock->get(); + + $job = (new ConfigureDnsRecordJob($this->team->id, $this->zone->id, $application->getMorphClass(), $application->id, 'app.example.com', '203.0.113.10')) + ->withFakeQueueInteractions(); + $job->handle(app(CloudflareDnsProvider::class)); + + $job->assertReleased(10); + expect(sentDnsRequests('POST'))->toBe(0)->and(ManagedDnsRecord::query()->count())->toBe(0); + + $heldLock->release(); + $retry = (new ConfigureDnsRecordJob($this->team->id, $this->zone->id, $application->getMorphClass(), $application->id, 'app.example.com', '203.0.113.10')) + ->withFakeQueueInteractions(); + $retry->handle(app(CloudflareDnsProvider::class)); + + $retry->assertNotReleased(); + $record = ManagedDnsRecord::query()->sole(); + expect(sentDnsRequests('POST'))->toBe(1) + ->and($record->owned)->toBeTrue() + ->and($record->references()->pluck('resource_id')->all())->toBe([$application->id]); + }); + + test('a create from the UI while the lock is held shows a clear message', function () { + fakeCloudflareDns(); + app()->instance(ManagedDnsHostnameLock::class, new ManagedDnsHostnameLock(waitSeconds: 0)); + $application = createDnsTestApplication($this, 'https://app.example.com'); + $heldLock = Cache::lock(ManagedDnsHostnameLock::key($this->team->id, 'app.example.com'), 60); + $heldLock->get(); + + Livewire::test(Domains::class, ['application' => $application]) + ->call('createManagedDnsRecord', 'app.example.com', $this->zone->id) + ->assertDispatched('error', 'Another DNS change for app.example.com is in progress. Try again in a few seconds.'); + + expect(sentDnsRequests('POST'))->toBe(0)->and(ManagedDnsRecord::query()->count())->toBe(0); + $heldLock->release(); + }); + + test('a create that waited for a release creates the deleted record again', function () { + $cloudflare = fakeCloudflareDns(); + $lock = new InterleavingManagedDnsHostnameLock; + app()->instance(ManagedDnsHostnameLock::class, $lock); + $oldApplication = createDnsTestApplication($this, 'https://app.example.com'); + $oldRecord = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'app.example.com', '203.0.113.10', $oldApplication); + $oldApplication->update(['fqdn' => 'https://moved.example.com']); + $newApplication = createDnsTestApplication($this, 'https://unrelated.example.org'); + + // The release holds the lock first; the new application adds the hostname while the create waits for it. + $lock->beforeNextRun = function () use ($oldApplication, $newApplication): void { + app(ManagedDnsRecordCleanup::class)->releaseHostname($oldApplication, 'app.example.com', $this->team->id); + $newApplication->update(['fqdn' => 'https://app.example.com']); + }; + $newRecord = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'app.example.com', '203.0.113.10', $newApplication); + + expect(sentDnsRequests('DELETE'))->toBe(1) + ->and(sentDnsRequests('POST'))->toBe(2) + ->and($oldRecord->fresh())->toBeNull() + ->and($newRecord->fresh())->not->toBeNull() + ->and($newRecord->provider_record_id)->not->toBe($oldRecord->provider_record_id) + ->and($newRecord->owned)->toBeTrue() + ->and($newRecord->references()->pluck('resource_id')->all())->toBe([$newApplication->id]) + ->and($cloudflare['records'])->toHaveCount(1); + }); + + test('the release keeps the record when a new reference appears before the lock is taken', function () { + $cloudflare = fakeCloudflareDns(); + $lock = new InterleavingManagedDnsHostnameLock; + app()->instance(ManagedDnsHostnameLock::class, $lock); + $application = createDnsTestApplication($this, 'https://moved.example.com'); + $record = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'app.example.com', '203.0.113.10', $application); + $other = createDnsTestApplication($this, 'https://unrelated.example.org'); + + $lock->beforeNextRun = fn () => $record->addReference($other); + $result = app(ManagedDnsRecordCleanup::class)->releaseHostname($application, 'app.example.com', $this->team->id); + + expect($result)->toBeNull() + ->and(sentDnsRequests('DELETE'))->toBe(0) + ->and($cloudflare['records'])->toHaveCount(1) + ->and($record->references()->pluck('resource_id')->all())->toBe([$other->id]); + }); + + test('the release keeps the record when the resource uses the hostname again before the lock is taken', function () { + $cloudflare = fakeCloudflareDns(); + $lock = new InterleavingManagedDnsHostnameLock; + app()->instance(ManagedDnsHostnameLock::class, $lock); + $application = createDnsTestApplication($this, 'https://moved.example.com'); + $record = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'app.example.com', '203.0.113.10', $application); + + $lock->beforeNextRun = fn () => $application->update(['fqdn' => 'https://app.example.com']); + $result = app(ManagedDnsRecordCleanup::class)->releaseHostname($application, 'app.example.com', $this->team->id); + + expect($result)->toBeNull() + ->and(sentDnsRequests('DELETE'))->toBe(0) + ->and($cloudflare['records'])->toHaveCount(1) + ->and($record->references()->pluck('resource_id')->all())->toBe([$application->id]); + }); + + test('a release reports busy when the lock is not free in time', function () { + fakeCloudflareDns(); + app()->instance(ManagedDnsHostnameLock::class, new ManagedDnsHostnameLock(waitSeconds: 0)); + $application = createDnsTestApplication($this, 'https://moved.example.com'); + $record = app(CloudflareDnsProvider::class)->createRecord($this->zone, 'app.example.com', '203.0.113.10', $application); + $heldLock = Cache::lock(ManagedDnsHostnameLock::key($this->team->id, 'app.example.com'), 60); + $heldLock->get(); + + $result = app(ManagedDnsRecordCleanup::class)->releaseHostname($application, 'app.example.com', $this->team->id); + + expect($result)->toBe(ManagedDnsDeletionResult::Busy) + ->and($result->shouldRetry())->toBeTrue() + ->and($record->fresh())->not->toBeNull(); + $heldLock->release(); + }); +}); diff --git a/tests/Feature/ManagedDnsRecordOwnershipTest.php b/tests/Feature/ManagedDnsRecordOwnershipTest.php index 0047066997..5b892c692e 100644 --- a/tests/Feature/ManagedDnsRecordOwnershipTest.php +++ b/tests/Feature/ManagedDnsRecordOwnershipTest.php @@ -57,77 +57,6 @@ beforeEach(function () { $this->zone = DnsProviderZone::factory()->for($this->token)->create(['provider_zone_id' => 'zone-1', 'name' => 'example.com']); }); -/** - * Fakes the Cloudflare DNS record API with an in-memory record store. - * - * @param array> $records - */ -function fakeCloudflareDns(array $records = []): ArrayObject -{ - $state = new ArrayObject(['records' => collect($records)->keyBy('id')->all(), 'next' => 1]); - - Http::fake(function (Request $request) use ($state) { - if (! preg_match('#^https://api\.cloudflare\.com/client/v4/zones/[^/]+/dns_records(?:/([^/?]+))?(?:\?(.*))?$#', $request->url(), $matches)) { - return Http::response(['success' => false], 404); - } - $id = ($matches[1] ?? '') !== '' ? $matches[1] : null; - $records = $state['records']; - - if ($id === null && $request->method() === 'GET') { - parse_str($matches[2] ?? '', $query); - $result = collect($records)->filter(fn (array $record): bool => $record['name'] === ($query['name'] ?? null) - && $record['type'] === ($query['type'] ?? null))->values()->all(); - - return Http::response(['success' => true, 'result' => $result]); - } - if ($id === null && $request->method() === 'POST') { - $id = 'record-new-'.$state['next']; - $state['next']++; - $records[$id] = array_merge(['proxied' => false, 'ttl' => 1, 'comment' => null], $request->data(), ['id' => $id]); - $state['records'] = $records; - - return Http::response(['success' => true, 'result' => $records[$id]]); - } - if (! isset($records[$id])) { - return Http::response(['success' => false, 'errors' => [['code' => 81044, 'message' => 'Record does not exist.']]], 404); - } - if ($request->method() === 'PATCH') { - $records[$id] = array_merge($records[$id], $request->data()); - $state['records'] = $records; - } - if ($request->method() === 'DELETE') { - unset($records[$id]); - $state['records'] = $records; - - return Http::response(['success' => true, 'result' => ['id' => $id]]); - } - - return Http::response(['success' => true, 'result' => $records[$id]]); - }); - - return $state; -} - -function createDnsTestApplication(object $test, string $fqdn): Application -{ - $application = Application::factory()->create([ - 'environment_id' => $test->environment->id, - 'destination_id' => $test->destination->id, - 'destination_type' => $test->destination->getMorphClass(), - 'fqdn' => $fqdn, - 'build_pack' => 'nixpacks', - ]); - $application->settings()->update(['is_container_label_readonly_enabled' => true]); - - return $application->fresh(); -} - -function sentDnsRequests(string $method): int -{ - return Http::recorded(fn (Request $request) => $request->method() === $method - && str_contains($request->url(), 'api.cloudflare.com'))->count(); -} - test('a created record carries the ownership comment and is owned', function () { fakeCloudflareDns(); $application = createDnsTestApplication($this, 'https://app.example.com'); diff --git a/tests/Pest.php b/tests/Pest.php index acc38329e7..9ec1040ec7 100644 --- a/tests/Pest.php +++ b/tests/Pest.php @@ -27,6 +27,7 @@ uses(TestCase::class)->in('Feature', 'v4/Feature', 'v4/Browser', 'v5/Browser'); */ require_once __DIR__.'/Support/BrowserTestHelpers.php'; +require_once __DIR__.'/Support/ManagedDnsTestHelpers.php'; function remoteOutputSource(string $path): string { diff --git a/tests/Support/ManagedDnsTestHelpers.php b/tests/Support/ManagedDnsTestHelpers.php new file mode 100644 index 0000000000..af9aa6441d --- /dev/null +++ b/tests/Support/ManagedDnsTestHelpers.php @@ -0,0 +1,80 @@ +> $records + */ +function fakeCloudflareDns(array $records = []): ArrayObject +{ + $state = new ArrayObject(['records' => collect($records)->keyBy('id')->all(), 'next' => 1]); + + Http::fake(function (Request $request) use ($state) { + if (! preg_match('#^https://api\.cloudflare\.com/client/v4/zones/[^/]+/dns_records(?:/([^/?]+))?(?:\?(.*))?$#', $request->url(), $matches)) { + return Http::response(['success' => false], 404); + } + $id = ($matches[1] ?? '') !== '' ? $matches[1] : null; + $records = $state['records']; + + if ($id === null && $request->method() === 'GET') { + parse_str($matches[2] ?? '', $query); + $result = collect($records)->filter(fn (array $record): bool => $record['name'] === ($query['name'] ?? null) + && $record['type'] === ($query['type'] ?? null))->values()->all(); + + return Http::response(['success' => true, 'result' => $result]); + } + if ($id === null && $request->method() === 'POST') { + $id = 'record-new-'.$state['next']; + $state['next']++; + $records[$id] = array_merge(['proxied' => false, 'ttl' => 1, 'comment' => null], $request->data(), ['id' => $id]); + $state['records'] = $records; + + return Http::response(['success' => true, 'result' => $records[$id]]); + } + if (! isset($records[$id])) { + return Http::response(['success' => false, 'errors' => [['code' => 81044, 'message' => 'Record does not exist.']]], 404); + } + if ($request->method() === 'PATCH') { + $records[$id] = array_merge($records[$id], $request->data()); + $state['records'] = $records; + } + if ($request->method() === 'DELETE') { + unset($records[$id]); + $state['records'] = $records; + + return Http::response(['success' => true, 'result' => ['id' => $id]]); + } + + return Http::response(['success' => true, 'result' => $records[$id]]); + }); + + return $state; +} + +function createDnsTestApplication(object $test, string $fqdn): Application +{ + $application = Application::factory()->create([ + 'environment_id' => $test->environment->id, + 'destination_id' => $test->destination->id, + 'destination_type' => $test->destination->getMorphClass(), + 'fqdn' => $fqdn, + 'build_pack' => 'nixpacks', + ]); + $application->settings()->update(['is_container_label_readonly_enabled' => true]); + + return $application->fresh(); +} + +function sentDnsRequests(string $method): int +{ + return Http::recorded(fn (Request $request) => $request->method() === $method + && str_contains($request->url(), 'api.cloudflare.com'))->count(); +}