From edf24257b5ad38958c1ec6d8ad6c8fbe9558e323 Mon Sep 17 00:00:00 2001 From: nexxo Date: Sat, 25 Jul 2026 23:51:25 +0200 Subject: [PATCH] fix(hosts): reserve the DNS name under the lock, never reuse it, clean it up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex was right that my never-reuse claim did not hold: - The lock was released with only a candidate in hand, so two concurrent onboardings in one datacenter could pick the same name and overwrite each other's record. The name is now persisted on the host while still locked. - The number was derived from the hosts that happen to exist, so removing the highest one handed that number straight back out — a cached name would then resolve to a different machine. The counter is stored per datacenter, floored by what is actually in use so a wiped settings store cannot reissue live names. - PurgeHost deleted the row that carried the record id, leaving the machine's management address published with nothing left to clean it up. Co-Authored-By: Claude Opus 4.8 --- app/Provisioning/Jobs/PurgeHost.php | 15 +++++++ .../Steps/Host/RegisterHostDns.php | 39 +++++++++++++------ tests/Feature/Provisioning/HostStepsTest.php | 26 ++++++++++++- 3 files changed, 66 insertions(+), 14 deletions(-) diff --git a/app/Provisioning/Jobs/PurgeHost.php b/app/Provisioning/Jobs/PurgeHost.php index 61b74da..72dc1b8 100644 --- a/app/Provisioning/Jobs/PurgeHost.php +++ b/app/Provisioning/Jobs/PurgeHost.php @@ -3,6 +3,8 @@ namespace App\Provisioning\Jobs; use App\Models\Host; +use App\Services\Dns\HetznerDnsClient; +use Illuminate\Support\Facades\Log; use App\Models\VpnPeer; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldQueue; @@ -44,6 +46,19 @@ class PurgeHost implements ShouldQueue Cache::lock('run:'.$run->uuid, 2200)->block(2100, fn () => $run->delete()); } + // The record identifier lives on the row about to be deleted, so this + // has to happen first — otherwise the host's management address stays + // published with nothing left to clean it up. + if (filled($host->dns_record_id)) { + try { + app(HetznerDnsClient::class)->deleteRecord($host->dns_record_id); + } catch (\Throwable $e) { + Log::warning('could not delete host DNS record', [ + 'host' => $host->uuid, 'record' => $host->dns_record_id, 'error' => $e->getMessage(), + ]); + } + } + if (filled($host->wg_pubkey)) { // Tombstone the console's view of this peer as well. The FK only // nulls host_id, which would leave an enabled-but-absent row behind: diff --git a/app/Provisioning/Steps/Host/RegisterHostDns.php b/app/Provisioning/Steps/Host/RegisterHostDns.php index ca8634d..3223ff0 100644 --- a/app/Provisioning/Steps/Host/RegisterHostDns.php +++ b/app/Provisioning/Steps/Host/RegisterHostDns.php @@ -6,6 +6,7 @@ use App\Models\Host; use App\Models\ProvisioningRun; use App\Provisioning\StepResult; use App\Services\Dns\HetznerDnsClient; +use App\Support\Settings; use Illuminate\Support\Facades\Cache; use Throwable; @@ -37,7 +38,10 @@ class RegisterHostDns extends HostStep return StepResult::fail('The host has no management address to publish.'); } - $name = $host->dns_name ?: $this->allocateName($host); + // Reserved and persisted before anything is published: releasing the + // lock with only a candidate in hand let two concurrent onboardings pick + // the same name and overwrite each other's record. + $name = $host->dns_name ?: $this->reserveName($host); $fqdn = $name.'.node.'.config('provisioning.dns.zone', 'clupilot.com'); try { @@ -52,29 +56,40 @@ class RegisterHostDns extends HostStep return StepResult::advance(); } - $host->update(['dns_name' => $name, 'dns_record_id' => $recordId]); + $host->update(['dns_record_id' => $recordId]); return StepResult::advance(); } /** - * -, numbered per datacenter and never reused: the number - * is one past the highest ever issued there, so removing a host does not - * renumber the others. + * -, numbered per datacenter and never reused. + * + * The counter is stored rather than derived from the hosts that happen to + * exist: deriving it hands the highest number straight back out after that + * host is removed, so a cached name would resolve to a different machine. */ - private function allocateName(Host $host): string + private function reserveName(Host $host): string { - return Cache::lock('host-dns-name:'.$host->datacenter, 15)->block(10, function () use ($host) { - $taken = Host::query() + return Cache::lock('host-dns-name:'.$host->datacenter, 30)->block(10, function () use ($host) { + $key = 'dns.sequence.'.$host->datacenter; + $next = ((int) Settings::get($key, 0)) + 1; + + // Never below what is already in use — a settings store that was + // wiped must not start handing out live names again. + $inUse = Host::query() ->where('datacenter', $host->datacenter) ->whereNotNull('dns_name') - ->pluck('dns_name'); - - $highest = $taken + ->pluck('dns_name') ->map(fn (string $name) => (int) substr($name, strrpos($name, '-') + 1)) ->max() ?? 0; + $next = max($next, $inUse + 1); - return sprintf('%s-%02d', $host->datacenter, $highest + 1); + $name = sprintf('%s-%02d', $host->datacenter, $next); + + Settings::set($key, $next); + $host->update(['dns_name' => $name]); // reserved while still locked + + return $name; }); } } diff --git a/tests/Feature/Provisioning/HostStepsTest.php b/tests/Feature/Provisioning/HostStepsTest.php index f24eb2b..7b09019 100644 --- a/tests/Feature/Provisioning/HostStepsTest.php +++ b/tests/Feature/Provisioning/HostStepsTest.php @@ -276,8 +276,10 @@ it('numbers hosts per datacenter and never reuses a number', function () { expect($names)->toBe(['fsn-01', 'fsn-02', 'hel-01']); - // Removing fsn-01 must not renumber fsn-02 onto its name. - App\Models\Host::query()->where('dns_name', 'fsn-01')->delete(); + // Removing the HIGHEST one is the dangerous case: deriving the number from + // live hosts would hand fsn-02 straight back out, and a cached name would + // then point at a different machine. + App\Models\Host::query()->where('dns_name', 'fsn-02')->delete(); $host = App\Models\Host::factory()->active()->create(['datacenter' => 'fsn', 'wg_ip' => '10.66.0.30', 'dns_name' => null]); $run = App\Models\ProvisioningRun::factory()->forHost($host)->create(["pipeline" => "host"]); (new App\Provisioning\Steps\Host\RegisterHostDns($s['dns']))->execute($run); @@ -295,3 +297,23 @@ it('does not strand an onboarding because DNS was unreachable', function () { expect((new App\Provisioning\Steps\Host\RegisterHostDns($s['dns']))->execute($run)->type)->toBe("advance"); expect($run->events()->where('step', 'register_host_dns')->exists())->toBeTrue(); }); + +it('takes the host DNS record with it when the host is purged', function () { + $s = fakeServices(); + app()->instance(App\Services\Dns\HetznerDnsClient::class, $s['dns']); + + $host = App\Models\Host::factory()->active()->create([ + 'datacenter' => 'fsn', 'wg_ip' => '10.66.0.50', 'dns_name' => null, + ]); + $run = App\Models\ProvisioningRun::factory()->forHost($host)->create(['pipeline' => 'host']); + (new App\Provisioning\Steps\Host\RegisterHostDns($s['dns']))->execute($run); + + $fqdn = $host->fresh()->dns_name.'.node.clupilot.com'; + expect($s['dns']->records)->toHaveKey($fqdn); + + (new App\Provisioning\Jobs\PurgeHost($host->uuid))->handle(); + + // Otherwise the machine's management address stays published for good, with + // the identifier needed to remove it deleted along with the row. + expect($s['dns']->records)->not->toHaveKey($fqdn); +});