From c815aee0b51d94b9d924ee9c21c8ddbc9badb3ad Mon Sep 17 00:00:00 2001 From: nexxo Date: Sat, 1 Aug 2026 14:03:35 +0200 Subject: [PATCH] Fix-Runde 1: Vorabpruefung vor dem Unique-Index, zwei Praezisierungen, ein fehlender Test Migration: die Reihenfolge in up() stellte den unwiderruflichsten Schritt (app_settings loeschen, dns_name-Spalte weg) VOR den einzigen Schritt, der an vorhandenen Daten scheitern kann (unique('name') - hosts.name trug noch nie einen eindeutigen Index). Schlug der auf MariaDB fehl, war der Schaden nicht mehr rueckgaengig zu machen: DDL committet dort implizit, die Migration gilt aber mangels Eintrag in der Migrationstabelle als nicht gelaufen, und ein zweiter up()-Versuch stirbt an der bereits fehlenden dns_name-Spalte. Jetzt steht eine reine Vorabpruefung ganz am Anfang, die simuliert, was die Uebertragung schreiben wuerde, und mit einer RuntimeException abbricht, bevor irgendetwas angefasst ist; das Loeschen der app_settings-Zeilen steht jetzt hinter dem Unique-Index, nicht davor. Gegen echtes MariaDB geprueft, einschliesslich eines Laufs mit zwei absichtlich kollidierenden Hostnamen. HostStepsTest: Titel und ein Kommentar praezisiert - der Schritt vergibt den Namen nicht mehr, er veroeffentlicht ihn nur noch. HostNamingTest: ungenutzten Import entfernt (Pint), Rueckfall-Test fuer HostName::label() bei einem Code ohne gueltige Zeichen ergaenzt. Co-Authored-By: Claude Opus 5 --- ..._090000_clupilot_vergibt_die_hostnamen.php | 59 +++++++++++++++++-- tests/Feature/Admin/HostNamingTest.php | 11 +++- tests/Feature/Provisioning/HostStepsTest.php | 9 ++- 3 files changed, 69 insertions(+), 10 deletions(-) diff --git a/database/migrations/2026_08_04_090000_clupilot_vergibt_die_hostnamen.php b/database/migrations/2026_08_04_090000_clupilot_vergibt_die_hostnamen.php index 1eef863..e5fd3dd 100644 --- a/database/migrations/2026_08_04_090000_clupilot_vergibt_die_hostnamen.php +++ b/database/migrations/2026_08_04_090000_clupilot_vergibt_die_hostnamen.php @@ -28,6 +28,43 @@ return new class extends Migration { public function up(): void { + // Vorabprüfung, bevor irgendetwas angefasst wird — auch vor der + // Spalte weiter unten, die selbst schon DDL ist. `hosts.name` trug + // noch nie einen eindeutigen Index (siehe create_hosts_table): ein + // Host, der die DNS-Registrierung nie erreicht hat, behält seinen + // getippten Namen, und ein zweiter Anlauf nach einem gescheiterten + // Anlegen ist genau die Lage, in der derselbe Name zweimal getippt + // wird. Bricht `unique('name')` weiter unten deswegen ab, ist es zu + // spät: MariaDB committet DDL sofort und implizit, egal ob die + // Migration insgesamt als gelaufen gilt. Ohne Eintrag in der + // Migrationstabelle ruft niemand `down()` auf, und ein zweiter + // `up()`-Versuch stirbt sofort an der Spalte, die im ersten + // (fehlgeschlagenen) Anlauf schon angelegt wurde — Rettung dann nur + // noch von Hand per SQL. + // + // Deshalb wird hier SIMULIERT, was die Übertragung gleich schreiben + // würde (`dns_name`, wo gesetzt, sonst der unveränderte `name`), + // statt real zu schreiben und erst danach nachzusehen. Das deckt + // beide Quellen von Doppelten ab — zwei Hosts, die schon denselben + // getippten Namen tragen, UND ein getippter Name, der zufällig einem + // fremden `dns_name` gleicht — und lässt die Datenbank unangetastet, + // wenn es einen Treffer gibt: eine reine SELECT-Prüfung vor der + // ersten Schreiboperation, beliebig oft wiederholbar. + $duplicates = DB::table('hosts') + ->selectRaw("(CASE WHEN dns_name IS NOT NULL AND dns_name <> '' THEN dns_name ELSE name END) as future_name") + ->pluck('future_name') + ->countBy() + ->filter(fn (int $count) => $count > 1) + ->keys(); + + if ($duplicates->isNotEmpty()) { + throw new RuntimeException( + 'Mehrere Hosts würden nach dieser Migration denselben Namen tragen: ' + .$duplicates->implode(', ').'. Nichts wurde verändert — ' + .'die betroffenen Hosts von Hand umbenennen oder entfernen und die Migration erneut ausführen.' + ); + } + Schema::table('datacenters', function (Blueprint $table) { $table->unsignedInteger('next_host_number')->default(1)->after('code'); }); @@ -54,7 +91,10 @@ return new class extends Migration // Und was der alte Zähler schon ausgegeben HATTE. Ohne diesen Wert // ginge die Zusage „niemals wiederverwendet" beim Umzug verloren: // ein Host, der angelegt und wieder entfernt wurde, steht in keiner - // Zeile mehr, aber sein Name steht noch in den Protokollen. + // Zeile mehr, aber sein Name steht noch in den Protokollen. Die + // Zeile steht hier noch, weil `app_settings` erst ganz am Ende + // geleert wird (siehe dort) — an dieser Stelle ist sie also + // garantiert noch da. $carried = (int) (json_decode( (string) DB::table('app_settings')->where('key', 'dns.sequence.'.$label)->value('value'), true, @@ -64,10 +104,6 @@ return new class extends Migration ->update(['next_host_number' => max($inUse, $carried) + 1]); } - // Der alte Zähler geht mit. Eine tote Einstellung, die noch wie eine - // Quelle aussieht, ist genau das Problem, das diese Migration behebt. - DB::table('app_settings')->where('key', 'like', 'dns.sequence.%')->delete(); - // Getrennte Aufrufe: Index löschen, Spalte löschen und Index anlegen in // einem Blueprint bringt SQLite (Testlauf) durcheinander. Schema::table('hosts', function (Blueprint $table) { @@ -78,10 +114,21 @@ return new class extends Migration $table->dropColumn('dns_name'); }); - // Ein Riegel, kein Ersatz für den Zähler. + // Der Riegel — dank der Vorabprüfung oben ohne Überraschung, aber + // trotzdem der riskanteste Schritt dieser Migration: der einzige, der + // an vorhandenen DATEN scheitern kann statt nur am Schema. Alles, was + // danach noch aussteht (die Zeile unten), ist reine Aufräumarbeit an + // etwas, das nichts mehr referenziert — sie steht deshalb ZULETZT. Schema::table('hosts', function (Blueprint $table) { $table->unique('name'); }); + + // Der alte Zähler geht mit. Eine tote Einstellung, die noch wie eine + // Quelle aussieht, ist genau das Problem, das diese Migration behebt. + // Ganz am Ende, NACH dem Unique-Index: das ist der letzte wirklich + // zerstörerische Schritt hier, und er gehört hinter jeden Schritt, der + // noch scheitern könnte — nicht davor. + DB::table('app_settings')->where('key', 'like', 'dns.sequence.%')->delete(); } public function down(): void diff --git a/tests/Feature/Admin/HostNamingTest.php b/tests/Feature/Admin/HostNamingTest.php index dcee1a6..9dbe384 100644 --- a/tests/Feature/Admin/HostNamingTest.php +++ b/tests/Feature/Admin/HostNamingTest.php @@ -4,8 +4,9 @@ use App\Actions\StartHostOnboarding; use App\Livewire\Admin\HostCreate; use App\Models\Datacenter; use App\Models\Host; -use App\Provisioning\Jobs\PurgeHost; use App\Support\HostName; +use App\Support\HostTakeoverCommand; +use Illuminate\Database\QueryException; use Illuminate\Support\Facades\Queue; use Livewire\Livewire; @@ -106,7 +107,7 @@ it('lässt zwei Hosts nicht denselben Namen tragen', function () { onboard(); expect(fn () => Host::factory()->create(['name' => 'fsn-01'])) - ->toThrow(Illuminate\Database\QueryException::class); + ->toThrow(QueryException::class); }); it('gibt zwei ähnlich geschriebenen Rechenzentrums-Codes nicht denselben Namen', function () { @@ -120,6 +121,10 @@ it('gibt zwei ähnlich geschriebenen Rechenzentrums-Codes nicht denselben Namen' ->and(onboard('eu-west')->name)->toBe('eu-west-02'); }); +it('fällt auf eine gültige Bezeichnung zurück, wenn der Code keine hergibt', function () { + expect(HostName::label('---'))->toBe('node'); +}); + it('zählt ab hundert ohne Sonderfall weiter', function () { Datacenter::query()->where('code', 'fsn')->update(['next_host_number' => 100]); @@ -132,6 +137,6 @@ it('nennt den Host im DNS so, wie die Konsole ihn nennt', function () { $host = onboard(); expect(HostName::fqdn($host->name))->toBe('fsn-01.node.clupilot.com') - ->and(App\Support\HostTakeoverCommand::fqdnFor($host)) + ->and(HostTakeoverCommand::fqdnFor($host)) ->toBe(HostName::fqdn($host->name)); }); diff --git a/tests/Feature/Provisioning/HostStepsTest.php b/tests/Feature/Provisioning/HostStepsTest.php index e079e5e..4c7068a 100644 --- a/tests/Feature/Provisioning/HostStepsTest.php +++ b/tests/Feature/Provisioning/HostStepsTest.php @@ -1326,7 +1326,7 @@ it('marks the host active on completion', function () { ->and($host->fresh()->status)->toBe('active'); }); -it('gives the host a name that points at the tunnel, not at its public address', function () { +it('publishes the name it was given, and points it at the tunnel, not at the public address', function () { // The zone is pinned here rather than read from the machine's own .env. It // used to assert the literal clupilot.com, which was true until somebody // set CLUPILOT_DNS_ZONE to clupilot.cloud on their installation — and then @@ -1347,6 +1347,13 @@ it('gives the host a name that points at the tunnel, not at its public address', $result = (new RegisterHostDns($s['hostDns']))->execute($run); expect($result->type)->toBe('advance') + // Kein Beweis für den Schritt selbst — der Name steht schon vor + // dem Aufruf fest, also bliebe diese Zeile auch grün, wenn der + // Schritt ihn einfach nur unangetastet ließe. Sie ist ein + // Stolperdraht: bekäme RegisterHostDns wieder ein `$host->update([ + // 'name' => ...])` verpasst (wie früher `dns_name`), müsste ein + // ANDERER Wert hier auffallen, sobald jemand den Namen tatsächlich + // ändert. ->and($host->fresh()->name)->toBe('fsn-01'); // The management address: publishing a Proxmox host's public IP would hand