fix(hosts): a failed DNS deletion must not orphan the record
Swallowing it and carrying on deleted the row that held the only reference to that record, publishing the machine's management address for good. The purge now fails instead — and is retried, since it re-reads the host and its other steps are idempotent — so a broken DNS provider becomes a visible failed job rather than a silent leak. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>feat/portal-design
parent
edf24257b5
commit
9f8659da32
|
|
@ -4,7 +4,6 @@ namespace App\Provisioning\Jobs;
|
||||||
|
|
||||||
use App\Models\Host;
|
use App\Models\Host;
|
||||||
use App\Services\Dns\HetznerDnsClient;
|
use App\Services\Dns\HetznerDnsClient;
|
||||||
use Illuminate\Support\Facades\Log;
|
|
||||||
use App\Models\VpnPeer;
|
use App\Models\VpnPeer;
|
||||||
use Illuminate\Bus\Queueable;
|
use Illuminate\Bus\Queueable;
|
||||||
use Illuminate\Contracts\Queue\ShouldQueue;
|
use Illuminate\Contracts\Queue\ShouldQueue;
|
||||||
|
|
@ -22,7 +21,16 @@ class PurgeHost implements ShouldQueue
|
||||||
{
|
{
|
||||||
use Dispatchable, InteractsWithQueue, Queueable, SerializesModels;
|
use Dispatchable, InteractsWithQueue, Queueable, SerializesModels;
|
||||||
|
|
||||||
public $tries = 1;
|
// Retried, because the DNS deletion below can fail for a minute and the job
|
||||||
|
// is re-entrant: it re-reads the host, already-deleted runs are simply gone,
|
||||||
|
// and the peer removal is idempotent.
|
||||||
|
public $tries = 3;
|
||||||
|
|
||||||
|
/** @return list<int> seconds between attempts */
|
||||||
|
public function backoff(): array
|
||||||
|
{
|
||||||
|
return [30, 120];
|
||||||
|
}
|
||||||
|
|
||||||
public $timeout = 2200;
|
public $timeout = 2200;
|
||||||
|
|
||||||
|
|
@ -50,13 +58,13 @@ class PurgeHost implements ShouldQueue
|
||||||
// has to happen first — otherwise the host's management address stays
|
// has to happen first — otherwise the host's management address stays
|
||||||
// published with nothing left to clean it up.
|
// published with nothing left to clean it up.
|
||||||
if (filled($host->dns_record_id)) {
|
if (filled($host->dns_record_id)) {
|
||||||
try {
|
// Deliberately NOT swallowed: the row about to be deleted holds the
|
||||||
app(HetznerDnsClient::class)->deleteRecord($host->dns_record_id);
|
// only reference to that record, so continuing past a failure would
|
||||||
} catch (\Throwable $e) {
|
// leave the machine's management address published for good with no
|
||||||
Log::warning('could not delete host DNS record', [
|
// way left to find it. Failing here lets the queue retry, and a
|
||||||
'host' => $host->uuid, 'record' => $host->dns_record_id, 'error' => $e->getMessage(),
|
// permanently broken DNS provider becomes a visible failed job
|
||||||
]);
|
// rather than a silent leak.
|
||||||
}
|
app(HetznerDnsClient::class)->deleteRecord($host->dns_record_id);
|
||||||
}
|
}
|
||||||
|
|
||||||
if (filled($host->wg_pubkey)) {
|
if (filled($host->wg_pubkey)) {
|
||||||
|
|
|
||||||
|
|
@ -16,6 +16,8 @@ class FakeHetznerDnsClient implements HetznerDnsClient
|
||||||
|
|
||||||
public bool $failUpsert = false;
|
public bool $failUpsert = false;
|
||||||
|
|
||||||
|
public bool $failDelete = false;
|
||||||
|
|
||||||
private int $counter = 1;
|
private int $counter = 1;
|
||||||
|
|
||||||
public function upsertRecord(string $fqdn, string $type, string $value): string
|
public function upsertRecord(string $fqdn, string $type, string $value): string
|
||||||
|
|
@ -31,6 +33,10 @@ class FakeHetznerDnsClient implements HetznerDnsClient
|
||||||
|
|
||||||
public function deleteRecord(string $recordId): void
|
public function deleteRecord(string $recordId): void
|
||||||
{
|
{
|
||||||
|
if ($this->failDelete) {
|
||||||
|
throw new RuntimeException('hetzner dns 5xx');
|
||||||
|
}
|
||||||
|
|
||||||
$this->records = array_filter($this->records, fn ($id) => $id !== $recordId);
|
$this->records = array_filter($this->records, fn ($id) => $id !== $recordId);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -317,3 +317,30 @@ it('takes the host DNS record with it when the host is purged', function () {
|
||||||
// the identifier needed to remove it deleted along with the row.
|
// the identifier needed to remove it deleted along with the row.
|
||||||
expect($s['dns']->records)->not->toHaveKey($fqdn);
|
expect($s['dns']->records)->not->toHaveKey($fqdn);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('keeps the host until its DNS record is really gone', function () {
|
||||||
|
$s = fakeServices();
|
||||||
|
app()->instance(App\Services\Dns\HetznerDnsClient::class, $s['dns']);
|
||||||
|
|
||||||
|
$host = App\Models\Host::factory()->active()->create([
|
||||||
|
'datacenter' => 'hel', 'wg_ip' => '10.66.0.60', 'dns_name' => null,
|
||||||
|
]);
|
||||||
|
$run = App\Models\ProvisioningRun::factory()->forHost($host)->create(['pipeline' => 'host']);
|
||||||
|
(new App\Provisioning\Steps\Host\RegisterHostDns($s['dns']))->execute($run);
|
||||||
|
|
||||||
|
$s['dns']->failDelete = true;
|
||||||
|
|
||||||
|
// The row carries the only reference to that record — deleting it anyway
|
||||||
|
// would publish the machine's management address for good.
|
||||||
|
expect(fn () => (new App\Provisioning\Jobs\PurgeHost($host->uuid))->handle())
|
||||||
|
->toThrow(RuntimeException::class);
|
||||||
|
|
||||||
|
expect(App\Models\Host::query()->whereKey($host->id)->exists())->toBeTrue();
|
||||||
|
|
||||||
|
// Once DNS is back, the purge completes.
|
||||||
|
$s['dns']->failDelete = false;
|
||||||
|
(new App\Provisioning\Jobs\PurgeHost($host->uuid))->handle();
|
||||||
|
|
||||||
|
expect(App\Models\Host::query()->whereKey($host->id)->exists())->toBeFalse()
|
||||||
|
->and($s['dns']->records)->toBe([]);
|
||||||
|
});
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue