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); +});