From 7e5d2aa537f7a166045144b8a99fab9c32a6c460 Mon Sep 17 00:00:00 2001 From: Benjamin Knofe-Vider Date: Fri, 3 Jul 2026 13:54:53 +0200 Subject: [PATCH] =?UTF-8?q?duckling:=20stop=20deriving=20the=20CR=20name?= =?UTF-8?q?=20=E2=80=94=20stored=20duckling=5Fname=20is=20authoritative?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Duckling CR name was derived from the org ID at every use site (lowercase transform + a hyphen-stripped legacy fallback on NotFound), with the stored duckling_name only consulted sometimes. Now the config store column (NOT NULL since 000012) is the single source of truth: - DucklingClient methods take the explicit CR name and use it verbatim; ducklingName()/legacyDucklingName() and the legacy getCR fallback are deleted. Create names the CR exactly what it is given. - The provisioner controller, both resolveDucklingStatus closures (worker activation + query-log writer), and the drift scanner pass the warehouse row's duckling_name (org-ID fallback only for zero-value fixtures; the column is NOT NULL in the DB). - Warehouse create stamps duckling_name = org_id verbatim (org IDs are validated lowercase DNS-1123 labels; no ToLower). Public provisioning API unchanged. - Admin UI drops the client-side ducklingName() derive helper and the hyphen-stripped matching in ducklingEntryFor; views render the stored field only. Verified beforehand that every prod warehouse's duckling_name exactly matches its live CR, so removing the legacy fallback strands nothing. --- controlplane/admin/ducklings_drift.go | 25 +- controlplane/admin/ui/src/lib/colors.test.ts | 13 +- controlplane/admin/ui/src/lib/format.ts | 22 +- controlplane/admin/ui/src/pages/OrgDetail.tsx | 10 +- controlplane/admin/ui/src/pages/Orgs.tsx | 14 +- controlplane/admin/ui/src/types/api.ts | 4 +- controlplane/configstore/models.go | 7 +- controlplane/configstore/store.go | 12 + controlplane/multitenant.go | 13 +- controlplane/provisioner/controller.go | 28 ++- .../provisioner/controller_analytics_test.go | 21 +- controlplane/provisioner/controller_test.go | 216 +++++++++++------- controlplane/provisioner/k8s_client.go | 126 ++++------ controlplane/provisioner/naming_test.go | 20 -- controlplane/provisioning/api.go | 10 +- querylog_writer_mode_kubernetes.go | 13 +- 16 files changed, 285 insertions(+), 269 deletions(-) diff --git a/controlplane/admin/ducklings_drift.go b/controlplane/admin/ducklings_drift.go index 160632fe..68a9e34b 100644 --- a/controlplane/admin/ducklings_drift.go +++ b/controlplane/admin/ducklings_drift.go @@ -6,7 +6,6 @@ import ( "context" "fmt" "net/http" - "strings" "time" "github.com/gin-gonic/gin" @@ -17,7 +16,9 @@ import ( // needs. Declared here so the admin package doesn't import provisioner; // *provisioner.DucklingClient satisfies it. type DucklingChecker interface { - CRStatus(ctx context.Context, orgID string) (present, ready bool, err error) + // CRStatus takes the Duckling CR name — the warehouse row's duckling_name, + // never a derived value. + CRStatus(ctx context.Context, name string) (present, ready bool, err error) ListCRNames(ctx context.Context) ([]string, error) } @@ -71,27 +72,23 @@ func (h *driftHandler) findDrift(c *gin.Context) { } entries := make([]driftEntry, 0) - // expected holds every CR name a warehouse legitimately maps to, so the - // orphan pass below doesn't flag a CR that simply uses the legacy - // (hyphen-stripped) name. - expected := make(map[string]struct{}, len(warehouses)*2) + // expected holds exactly the stored duckling_name of every warehouse row, + // so the orphan pass below flags any CR not claimed by a row. + expected := make(map[string]struct{}, len(warehouses)) for i := range warehouses { wh := warehouses[i] orgID := wh.OrgID + // The stored duckling_name is the authoritative CR name. The column is + // NOT NULL and backfilled; the org-ID fallback only covers legacy rows + // with an empty value (the CR name defaults to the org ID verbatim). name := wh.DucklingName if name == "" { - // Fallback for rows the backfill missed; mirrors - // provisioner.ducklingName (lowercased org ID). - name = strings.ToLower(orgID) + name = orgID } expected[name] = struct{}{} - // Also expect the canonical and legacy (hyphen-stripped) variants so - // pre-rename CRs aren't misreported as orphans. - expected[strings.ToLower(orgID)] = struct{}{} - expected[strings.ReplaceAll(strings.ToLower(orgID), "-", "")] = struct{}{} - present, ready, cerr := h.checker.CRStatus(ctx, orgID) + present, ready, cerr := h.checker.CRStatus(ctx, name) if cerr != nil { entries = append(entries, driftEntry{ Org: orgID, diff --git a/controlplane/admin/ui/src/lib/colors.test.ts b/controlplane/admin/ui/src/lib/colors.test.ts index 4a373488..76d35b34 100644 --- a/controlplane/admin/ui/src/lib/colors.test.ts +++ b/controlplane/admin/ui/src/lib/colors.test.ts @@ -21,13 +21,14 @@ describe("hashColor", () => { }); describe("ducklingEntryFor", () => { - const entries = { "acme-corp": 1, acmecorp: 2, custom: 3 }; + const entries = { "acme-corp": 1, custom: 3 }; - it("prefers the stored duckling_name, then canonical, then legacy", () => { + it("matches the stored duckling_name or the org name exactly — no derivation", () => { expect(ducklingEntryFor(entries, "Acme-Corp", "custom")).toBe(3); - expect(ducklingEntryFor(entries, "Acme-Corp")).toBe(1); - expect(ducklingEntryFor({ acmecorp: 2 }, "Acme-Corp")).toBe(2); - expect(ducklingEntryFor(entries, "unknown-org")).toBeUndefined(); - expect(ducklingEntryFor(undefined, "Acme-Corp")).toBeUndefined(); + expect(ducklingEntryFor(entries, "acme-corp", undefined)).toBe(1); + // No lowercasing or hyphen-stripping of the org name anymore. + expect(ducklingEntryFor(entries, "Acme-Corp", undefined)).toBeUndefined(); + expect(ducklingEntryFor(entries, "unknown-org", undefined)).toBeUndefined(); + expect(ducklingEntryFor(undefined, "acme-corp", "custom")).toBeUndefined(); }); }); diff --git a/controlplane/admin/ui/src/lib/format.ts b/controlplane/admin/ui/src/lib/format.ts index d996a9e8..cdb0053d 100644 --- a/controlplane/admin/ui/src/lib/format.ts +++ b/controlplane/admin/ui/src/lib/format.ts @@ -145,27 +145,17 @@ export function isZeroTime(ts: string | null | undefined): boolean { return !ts || ts.startsWith(ZERO_TIME); } -// The Duckling (managed warehouse) CR name is the org ID lowercased, hyphens -// preserved. -export function ducklingName(orgName: string): string { - return orgName.toLowerCase(); -} - -// Resolve an org's entry in a Duckling-CR-name-keyed map: the stored -// duckling_name wins, then the canonical (lowercased) org name, then the -// legacy hyphen-stripped variant pre-rename CRs still use (mirrors the drift -// finder's expected-name set). +// Resolve an org's entry in a Duckling-CR-name-keyed map by exact match only: +// the warehouse's stored duckling_name (authoritative, NOT NULL in the DB) +// wins, then the org name itself. No client-side derivation of CR names. export function ducklingEntryFor( entries: Record | undefined, orgName: string, - ducklingNameOverride?: string, + ducklingName: string | undefined, ): T | undefined { if (!entries) return undefined; - const canonical = ducklingName(orgName); - for (const name of [ducklingNameOverride, canonical, canonical.replaceAll("-", "")]) { - if (name && entries[name] !== undefined) return entries[name]; - } - return undefined; + if (ducklingName && entries[ducklingName] !== undefined) return entries[ducklingName]; + return entries[orgName]; } // A Duckling is broken/unhealthy when its warehouse row exists but the overall diff --git a/controlplane/admin/ui/src/pages/OrgDetail.tsx b/controlplane/admin/ui/src/pages/OrgDetail.tsx index 89252faf..b49aca97 100644 --- a/controlplane/admin/ui/src/pages/OrgDetail.tsx +++ b/controlplane/admin/ui/src/pages/OrgDetail.tsx @@ -21,7 +21,7 @@ import { DialogTitle, } from "@/components/ui/dialog"; import { ApiError } from "@/lib/api"; -import { ducklingBroken, ducklingEntryFor, ducklingName, fmtTime } from "@/lib/format"; +import { ducklingBroken, ducklingEntryFor, fmtTime } from "@/lib/format"; import { ShardBadge } from "@/components/ShardBadge"; import { useDeleteOrg, @@ -308,9 +308,9 @@ function WarehousePanel({ if (data) { setImage(data.image ?? ""); setVersion(data.ducklake_version ?? ""); - setDucklingNameInput(data.duckling_name || ducklingName(orgId)); + setDucklingNameInput(data.duckling_name ?? ""); } - }, [data, orgId]); + }, [data]); const notFound = error instanceof ApiError && error.status === 404; const missing = notFound || !data; @@ -337,7 +337,7 @@ function WarehousePanel({ const body: Partial = {}; if (image !== (data?.image ?? "")) body.image = image; if (version !== (data?.ducklake_version ?? "")) body.ducklake_version = version; - if (ducklingNameInput !== (data?.duckling_name || ducklingName(orgId))) { + if (ducklingNameInput !== (data?.duckling_name ?? "")) { body.duckling_name = ducklingNameInput; } if (Object.keys(body).length === 0) { @@ -364,7 +364,7 @@ function WarehousePanel({
Duckling
-
{data?.duckling_name || ducklingName(orgId)}
+
{data?.duckling_name ?? "—"}
{/* Live metadata-store assignment from the Duckling CR status — the cnpg shard the tenant's metadata actually lives on (the config diff --git a/controlplane/admin/ui/src/pages/Orgs.tsx b/controlplane/admin/ui/src/pages/Orgs.tsx index af2d950c..881d7019 100644 --- a/controlplane/admin/ui/src/pages/Orgs.tsx +++ b/controlplane/admin/ui/src/pages/Orgs.tsx @@ -12,7 +12,7 @@ import { StateBadge } from "@/components/StateBadge"; import { ShardBadge } from "@/components/ShardBadge"; import { EmptyState, ErrorState, TableSkeleton } from "@/components/states"; import { useDucklingDrift, useDucklingsMetadata, useOrgs } from "@/hooks/useApi"; -import { ducklingEntryFor, ducklingName, fmtInt } from "@/lib/format"; +import { ducklingEntryFor, fmtInt } from "@/lib/format"; import type { DucklingDrift, Org } from "@/types/api"; export function Orgs() { @@ -112,12 +112,12 @@ export function Orgs() { { id: "duckling", header: "Duckling", - accessorFn: (o) => (o.warehouse ? o.warehouse.duckling_name || ducklingName(o.name) : ""), - cell: ({ row }) => { - const o = row.original; - const name = o.warehouse ? o.warehouse.duckling_name || ducklingName(o.name) : ""; - return {name || "—"}; - }, + accessorFn: (o) => o.warehouse?.duckling_name ?? "", + cell: ({ row }) => ( + + {row.original.warehouse?.duckling_name || "—"} + + ), }, { id: "shard", diff --git a/controlplane/admin/ui/src/types/api.ts b/controlplane/admin/ui/src/types/api.ts index ea3021aa..55a1c1ae 100644 --- a/controlplane/admin/ui/src/types/api.ts +++ b/controlplane/admin/ui/src/types/api.ts @@ -132,8 +132,8 @@ export type WarehouseState = export interface ManagedWarehouse { org_id: string; - // The explicit Duckling CR name (provisioner-owned). May be "" on legacy rows - // that predate the field — callers fall back to ducklingName(org) in that case. + // The explicit Duckling CR name (provisioner-owned). Authoritative and always + // set — NOT NULL in the DB since migration 000012; no client-side derivation. duckling_name: string; image: string; ducklake_version: string; diff --git a/controlplane/configstore/models.go b/controlplane/configstore/models.go index ed93fe2b..6f398f5e 100644 --- a/controlplane/configstore/models.go +++ b/controlplane/configstore/models.go @@ -228,9 +228,10 @@ type ManagedWarehouse struct { Image string `gorm:"size:512" json:"image"` DuckLakeVersion string `gorm:"size:32" json:"ducklake_version"` - // DucklingName is the k8s Duckling CR name; canonical = lowercased org ID. - // Authoritative, not derived — stored explicitly so lookups stop guessing - // it from the org ID (see provisioner.ducklingName for the canonical form). + // DucklingName is THE authoritative k8s Duckling CR name — nothing in the + // control plane derives or re-derives it. On warehouse create it defaults + // to the org ID verbatim (org IDs are validated as lowercase DNS-1123 + // labels at the provisioning endpoint). DucklingName string `gorm:"size:255;not null" json:"duckling_name"` WarehouseDatabase ManagedWarehouseDatabase `gorm:"embedded;embeddedPrefix:warehouse_database_" json:"warehouse_database"` diff --git a/controlplane/configstore/store.go b/controlplane/configstore/store.go index cc25a772..29880b9f 100644 --- a/controlplane/configstore/store.go +++ b/controlplane/configstore/store.go @@ -650,6 +650,18 @@ func (cs *ConfigStore) ListWarehouses() ([]ManagedWarehouse, error) { return warehouses, nil } +// GetManagedWarehouse returns the managed-warehouse row for an org. Direct DB +// query (not snapshot-based); returns gorm.ErrRecordNotFound when the org has +// no warehouse row. Callers use it to resolve the authoritative duckling_name +// before talking to the Duckling CR API. +func (cs *ConfigStore) GetManagedWarehouse(orgID string) (*ManagedWarehouse, error) { + var warehouse ManagedWarehouse + if err := cs.db.First(&warehouse, "org_id = ?", orgID).Error; err != nil { + return nil, err + } + return &warehouse, nil +} + func (cs *ConfigStore) ListWarehousesByStates(states []ManagedWarehouseProvisioningState) ([]ManagedWarehouse, error) { var warehouses []ManagedWarehouse if err := cs.db.Where("state IN ?", states).Find(&warehouses).Error; err != nil { diff --git a/controlplane/multitenant.go b/controlplane/multitenant.go index 1e88ca0a..2692c9d8 100644 --- a/controlplane/multitenant.go +++ b/controlplane/multitenant.go @@ -247,7 +247,18 @@ func SetupMultiTenant( slog.Warn("Duckling client unavailable, will use config store for infrastructure details.", "error", dcErr) } else { resolveDucklingStatus = func(ctx context.Context, orgID string) (*provisioner.DucklingStatus, error) { - return dc.Get(ctx, orgID) + // The CR name is the warehouse row's duckling_name (authoritative, + // never derived). The column is NOT NULL and backfilled; the org-ID + // fallback only covers legacy in-flight rows with an empty value. + warehouse, err := store.GetManagedWarehouse(orgID) + if err != nil { + return nil, fmt.Errorf("resolve duckling name for org %q: %w", orgID, err) + } + name := warehouse.DucklingName + if name == "" { + name = orgID + } + return dc.Get(ctx, name) } } diff --git a/controlplane/provisioner/controller.go b/controlplane/provisioner/controller.go index ff2596cd..066f96e1 100644 --- a/controlplane/provisioner/controller.go +++ b/controlplane/provisioner/controller.go @@ -37,6 +37,18 @@ func captureProvisionFailed(w *configstore.ManagedWarehouse, reason string, upda analytics.Default().Capture("warehouse_provision_failed", w.OrgID, props) } +// ducklingCRName returns the Duckling CR name for a warehouse row: the stored +// duckling_name, which is authoritative (the control plane never derives it). +// The column is NOT NULL and backfilled in the database, so the org-ID +// fallback only covers zero-value test fixtures and legacy in-flight rows +// constructed in memory before the column existed. +func ducklingCRName(w *configstore.ManagedWarehouse) string { + if w.DucklingName != "" { + return w.DucklingName + } + return w.OrgID +} + // WarehouseStore is the subset of configstore.ConfigStore that the controller needs. type WarehouseStore interface { ListWarehousesByStates(states []configstore.ManagedWarehouseProvisioningState) ([]configstore.ManagedWarehouse, error) @@ -162,7 +174,7 @@ func (c *Controller) reconcilePending(ctx context.Context, w *configstore.Manage now := time.Now().UTC() // Check if a Duckling CR already exists (e.g., controller restart) - _, err := c.duckling.Get(ctx, w.OrgID) + _, err := c.duckling.Get(ctx, ducklingCRName(w)) if err == nil { // CR exists — transition directly to provisioning log.Info("Duckling CR already exists, transitioning to provisioning.") @@ -179,7 +191,7 @@ func (c *Controller) reconcilePending(ctx context.Context, w *configstore.Manage // Create the Duckling CR log.Info("Creating Duckling CR.", "pgbouncer_enabled", w.PgBouncer.Enabled) - if err := c.duckling.Create(ctx, w.OrgID, CreateOptions{ + if err := c.duckling.Create(ctx, ducklingCRName(w), CreateOptions{ MetadataStoreType: w.MetadataStore.Kind, PgBouncerEnabled: w.PgBouncer.Enabled, ExternalEndpoint: w.MetadataStore.Endpoint, @@ -231,7 +243,7 @@ func (c *Controller) reconcileProvisioning(ctx context.Context, w *configstore.M return } - status, err := c.duckling.Get(ctx, w.OrgID) + status, err := c.duckling.Get(ctx, ducklingCRName(w)) if err != nil { log.Warn("Failed to get Duckling CR status.", "error", err) return @@ -352,7 +364,7 @@ func (c *Controller) reconcileProvisioning(ctx context.Context, w *configstore.M func (c *Controller) reconcileReady(ctx context.Context, w *configstore.ManagedWarehouse) { log := slog.With("org", w.OrgID, "phase", "ready") - currentPgB, err := c.duckling.GetPgBouncerEnabled(ctx, w.OrgID) + currentPgB, err := c.duckling.GetPgBouncerEnabled(ctx, ducklingCRName(w)) if err != nil { if apierrors.IsNotFound(err) { // CR is gone but the warehouse is still marked Ready. Don't try @@ -367,7 +379,7 @@ func (c *Controller) reconcileReady(ctx context.Context, w *configstore.ManagedW if currentPgB != w.PgBouncer.Enabled { log.Info("PgBouncer drift detected, patching Duckling CR.", "desired", w.PgBouncer.Enabled, "current", currentPgB) - if err := c.duckling.SetPgBouncerEnabled(ctx, w.OrgID, w.PgBouncer.Enabled); err != nil { + if err := c.duckling.SetPgBouncerEnabled(ctx, ducklingCRName(w), w.PgBouncer.Enabled); err != nil { log.Warn("Failed to patch Duckling CR pgbouncer.enabled.", "error", err) } } @@ -401,7 +413,7 @@ func (c *Controller) reconcileBucketName(ctx context.Context, w *configstore.Man return } - current, err := c.duckling.GetDataStoreBucketName(ctx, w.OrgID) + current, err := c.duckling.GetDataStoreBucketName(ctx, ducklingCRName(w)) if err != nil { if apierrors.IsNotFound(err) { return @@ -417,7 +429,7 @@ func (c *Controller) reconcileBucketName(ctx context.Context, w *configstore.Man if current == "" { log.Info("Backfilling CP-owned bucket name onto Duckling CR.", "bucket", desired) - if err := c.duckling.SetDataStoreBucketName(ctx, w.OrgID, desired); err != nil { + if err := c.duckling.SetDataStoreBucketName(ctx, ducklingCRName(w), desired); err != nil { log.Warn("Failed to patch Duckling CR dataStore.bucketName.", "error", err) return } @@ -438,7 +450,7 @@ func (c *Controller) reconcileDeleting(ctx context.Context, w *configstore.Manag log := slog.With("org", w.OrgID, "phase", "deleting") log.Info("Deleting Duckling CR.") - if err := c.duckling.Delete(ctx, w.OrgID); err != nil { + if err := c.duckling.Delete(ctx, ducklingCRName(w)); err != nil { // Only proceed if the CR is already gone (NotFound). For other errors // (network, RBAC, etc.) we retry on the next reconcile pass to avoid // marking as deleted while AWS resources still exist. diff --git a/controlplane/provisioner/controller_analytics_test.go b/controlplane/provisioner/controller_analytics_test.go index b65acd21..a88b1855 100644 --- a/controlplane/provisioner/controller_analytics_test.go +++ b/controlplane/provisioner/controller_analytics_test.go @@ -80,6 +80,7 @@ func TestReconcileProvisioningSuccessEmitsEvent(t *testing.T) { fs := newFakeStore() fs.warehouses["org-ok"] = &configstore.ManagedWarehouse{ OrgID: "org-ok", + DucklingName: "org-ok", State: configstore.ManagedWarehouseStateProvisioning, CreatedAt: time.Now(), MetadataStore: configstore.ManagedWarehouseMetadataStore{Kind: configstore.MetadataStoreKindExternal}, @@ -89,7 +90,7 @@ func TestReconcileProvisioningSuccessEmitsEvent(t *testing.T) { cr := &unstructured.Unstructured{Object: map[string]interface{}{ "apiVersion": "k8s.posthog.com/v1alpha1", "kind": "Duckling", - "metadata": map[string]interface{}{"name": ducklingName("org-ok"), "namespace": ducklingNamespace}, + "metadata": map[string]interface{}{"name": "org-ok", "namespace": ducklingNamespace}, "status": map[string]interface{}{ "metadataStore": map[string]interface{}{ "type": "external", "endpoint": "rds.example", "password": "secret", "user": "postgres", "database": "postgres", @@ -148,6 +149,7 @@ func TestReconcileProvisioningTimeoutEmitsFailure(t *testing.T) { fs := newFakeStore() fs.warehouses["org-slow"] = &configstore.ManagedWarehouse{ OrgID: "org-slow", + DucklingName: "org-slow", State: configstore.ManagedWarehouseStateProvisioning, CreatedAt: time.Now().Add(-31 * time.Minute), MetadataStore: configstore.ManagedWarehouseMetadataStore{Kind: configstore.MetadataStoreKindCnpgShard}, @@ -180,6 +182,7 @@ func TestReconcileProvisioningCrossplaneFailureEmitsFailure(t *testing.T) { fs := newFakeStore() fs.warehouses["org-xp"] = &configstore.ManagedWarehouse{ OrgID: "org-xp", + DucklingName: "org-xp", State: configstore.ManagedWarehouseStateProvisioning, CreatedAt: time.Now().Add(-15 * time.Minute), // past the 10-min Crossplane grace, under the 30-min timeout MetadataStore: configstore.ManagedWarehouseMetadataStore{Kind: configstore.MetadataStoreKindExternal}, @@ -188,7 +191,7 @@ func TestReconcileProvisioningCrossplaneFailureEmitsFailure(t *testing.T) { cr := &unstructured.Unstructured{Object: map[string]interface{}{ "apiVersion": "k8s.posthog.com/v1alpha1", "kind": "Duckling", - "metadata": map[string]interface{}{"name": ducklingName("org-xp"), "namespace": ducklingNamespace}, + "metadata": map[string]interface{}{"name": "org-xp", "namespace": ducklingNamespace}, "status": map[string]interface{}{ "conditions": []interface{}{ map[string]interface{}{"type": "Synced", "status": "False", "message": "cannot create metadata store: InvalidParameterException"}, @@ -222,14 +225,15 @@ func TestReconcileDeletingSuccessEmitsEvent(t *testing.T) { dc, fakeK8s := newFakeDucklingClient() fs := newFakeStore() fs.warehouses["org-del"] = &configstore.ManagedWarehouse{ - OrgID: "org-del", - State: configstore.ManagedWarehouseStateDeleting, + OrgID: "org-del", + DucklingName: "org-del", + State: configstore.ManagedWarehouseStateDeleting, } ctx := context.Background() cr := &unstructured.Unstructured{Object: map[string]interface{}{ "apiVersion": "k8s.posthog.com/v1alpha1", "kind": "Duckling", - "metadata": map[string]interface{}{"name": ducklingName("org-del"), "namespace": ducklingNamespace}, + "metadata": map[string]interface{}{"name": "org-del", "namespace": ducklingNamespace}, }} if _, err := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Create(ctx, cr, metav1.CreateOptions{}); err != nil { t.Fatalf("create CR: %v", err) @@ -257,14 +261,15 @@ func TestReconcileDeletingFailureEmitsEvent(t *testing.T) { dc, fakeK8s := newFakeDucklingClient() fs := newFakeStore() fs.warehouses["org-stuck"] = &configstore.ManagedWarehouse{ - OrgID: "org-stuck", - State: configstore.ManagedWarehouseStateDeleting, + OrgID: "org-stuck", + DucklingName: "org-stuck", + State: configstore.ManagedWarehouseStateDeleting, } ctx := context.Background() cr := &unstructured.Unstructured{Object: map[string]interface{}{ "apiVersion": "k8s.posthog.com/v1alpha1", "kind": "Duckling", - "metadata": map[string]interface{}{"name": ducklingName("org-stuck"), "namespace": ducklingNamespace}, + "metadata": map[string]interface{}{"name": "org-stuck", "namespace": ducklingNamespace}, }} if _, err := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Create(ctx, cr, metav1.CreateOptions{}); err != nil { t.Fatalf("create CR: %v", err) diff --git a/controlplane/provisioner/controller_test.go b/controlplane/provisioner/controller_test.go index 7c507157..f5759041 100644 --- a/controlplane/provisioner/controller_test.go +++ b/controlplane/provisioner/controller_test.go @@ -117,8 +117,9 @@ func TestReconcilePendingCreatesCR(t *testing.T) { dc, fakeK8s := newFakeDucklingClient() fs := newFakeStore() fs.warehouses["org-a"] = &configstore.ManagedWarehouse{ - OrgID: "org-a", - State: configstore.ManagedWarehouseStatePending, + OrgID: "org-a", + DucklingName: "org-a", + State: configstore.ManagedWarehouseStatePending, MetadataStore: configstore.ManagedWarehouseMetadataStore{ Kind: configstore.MetadataStoreKindExternal, Endpoint: "ext.example.internal", @@ -133,7 +134,7 @@ func TestReconcilePendingCreatesCR(t *testing.T) { ctrl.reconcile(ctx) // Verify CR was created - cr, err := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(ctx, ducklingName("org-a"), metav1.GetOptions{}) + cr, err := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(ctx, "org-a", metav1.GetOptions{}) if err != nil { t.Fatalf("expected CR to exist: %v", err) } @@ -178,8 +179,9 @@ func TestReconcilePendingEmitsPgBouncerBlock(t *testing.T) { dc, fakeK8s := newFakeDucklingClient() fs := newFakeStore() fs.warehouses["org-pgb"] = &configstore.ManagedWarehouse{ - OrgID: "org-pgb", - State: configstore.ManagedWarehouseStatePending, + OrgID: "org-pgb", + DucklingName: "org-pgb", + State: configstore.ManagedWarehouseStatePending, MetadataStore: configstore.ManagedWarehouseMetadataStore{ Kind: configstore.MetadataStoreKindExternal, Endpoint: "ext.example.internal", @@ -193,7 +195,7 @@ func TestReconcilePendingEmitsPgBouncerBlock(t *testing.T) { ctx := context.Background() ctrl.reconcile(ctx) - cr, err := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(ctx, ducklingName("org-pgb"), metav1.GetOptions{}) + cr, err := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(ctx, "org-pgb", metav1.GetOptions{}) if err != nil { t.Fatalf("expected CR to exist: %v", err) } @@ -212,16 +214,17 @@ func TestReconcileReadyPatchesCRWhenPgBouncerFlippedOn(t *testing.T) { dc, fakeK8s := newFakeDucklingClient() fs := newFakeStore() fs.warehouses["org-flip"] = &configstore.ManagedWarehouse{ - OrgID: "org-flip", - State: configstore.ManagedWarehouseStateReady, - PgBouncer: configstore.ManagedWarehousePgBouncer{Enabled: true}, + OrgID: "org-flip", + DucklingName: "org-flip", + State: configstore.ManagedWarehouseStateReady, + PgBouncer: configstore.ManagedWarehousePgBouncer{Enabled: true}, } // Seed a CR whose spec still reflects the pre-flip world (no pgbouncer block). cr := &unstructured.Unstructured{Object: map[string]interface{}{ "apiVersion": "k8s.posthog.com/v1alpha1", "kind": "Duckling", "metadata": map[string]interface{}{ - "name": ducklingName("org-flip"), + "name": "org-flip", "namespace": ducklingNamespace, }, "spec": map[string]interface{}{ @@ -241,7 +244,7 @@ func TestReconcileReadyPatchesCRWhenPgBouncerFlippedOn(t *testing.T) { ctrl := NewControllerWithClient(fs, dc, time.Second) ctrl.reconcile(context.Background()) - got, err := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(context.Background(), ducklingName("org-flip"), metav1.GetOptions{}) + got, err := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(context.Background(), "org-flip", metav1.GetOptions{}) if err != nil { t.Fatalf("re-fetch CR: %v", err) } @@ -267,9 +270,10 @@ func TestReconcileReadyPatchesCRWhenPgBouncerFlippedOff(t *testing.T) { dc, fakeK8s := newFakeDucklingClient() fs := newFakeStore() fs.warehouses["org-off"] = &configstore.ManagedWarehouse{ - OrgID: "org-off", - State: configstore.ManagedWarehouseStateReady, - PgBouncer: configstore.ManagedWarehousePgBouncer{Enabled: false}, + OrgID: "org-off", + DucklingName: "org-off", + State: configstore.ManagedWarehouseStateReady, + PgBouncer: configstore.ManagedWarehousePgBouncer{Enabled: false}, } // Seed a CR that currently has pgbouncer enabled — expect it to be // patched back to false to match the config store. @@ -277,7 +281,7 @@ func TestReconcileReadyPatchesCRWhenPgBouncerFlippedOff(t *testing.T) { "apiVersion": "k8s.posthog.com/v1alpha1", "kind": "Duckling", "metadata": map[string]interface{}{ - "name": ducklingName("org-off"), + "name": "org-off", "namespace": ducklingNamespace, }, "spec": map[string]interface{}{ @@ -296,7 +300,7 @@ func TestReconcileReadyPatchesCRWhenPgBouncerFlippedOff(t *testing.T) { ctrl := NewControllerWithClient(fs, dc, time.Second) ctrl.reconcile(context.Background()) - got, err := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(context.Background(), ducklingName("org-off"), metav1.GetOptions{}) + got, err := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(context.Background(), "org-off", metav1.GetOptions{}) if err != nil { t.Fatalf("re-fetch CR: %v", err) } @@ -312,15 +316,16 @@ func TestReconcileReadyNoDriftDoesNotPatch(t *testing.T) { dc, fakeK8s := newFakeDucklingClient() fs := newFakeStore() fs.warehouses["org-sync"] = &configstore.ManagedWarehouse{ - OrgID: "org-sync", - State: configstore.ManagedWarehouseStateReady, - PgBouncer: configstore.ManagedWarehousePgBouncer{Enabled: true}, + OrgID: "org-sync", + DucklingName: "org-sync", + State: configstore.ManagedWarehouseStateReady, + PgBouncer: configstore.ManagedWarehousePgBouncer{Enabled: true}, } cr := &unstructured.Unstructured{Object: map[string]interface{}{ "apiVersion": "k8s.posthog.com/v1alpha1", "kind": "Duckling", "metadata": map[string]interface{}{ - "name": ducklingName("org-sync"), + "name": "org-sync", "namespace": ducklingNamespace, "resourceVersion": "42", }, @@ -339,7 +344,7 @@ func TestReconcileReadyNoDriftDoesNotPatch(t *testing.T) { ctrl := NewControllerWithClient(fs, dc, time.Second) ctrl.reconcile(context.Background()) - got, err := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(context.Background(), ducklingName("org-sync"), metav1.GetOptions{}) + got, err := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(context.Background(), "org-sync", metav1.GetOptions{}) if err != nil { t.Fatalf("re-fetch CR: %v", err) } @@ -352,9 +357,10 @@ func TestReconcileProvisioningAllReady(t *testing.T) { dc, fakeK8s := newFakeDucklingClient() fs := newFakeStore() fs.warehouses["org-b"] = &configstore.ManagedWarehouse{ - OrgID: "org-b", - State: configstore.ManagedWarehouseStateProvisioning, - CreatedAt: time.Now(), + OrgID: "org-b", + DucklingName: "org-b", + State: configstore.ManagedWarehouseStateProvisioning, + CreatedAt: time.Now(), } // Create a Duckling CR with all status fields populated @@ -363,7 +369,7 @@ func TestReconcileProvisioningAllReady(t *testing.T) { "apiVersion": "k8s.posthog.com/v1alpha1", "kind": "Duckling", "metadata": map[string]interface{}{ - "name": ducklingName("org-b"), + "name": "org-b", "namespace": ducklingNamespace, }, "status": map[string]interface{}{ @@ -437,9 +443,10 @@ func TestReconcileProvisioningProbeFailsKeepsProvisioning(t *testing.T) { dc, fakeK8s := newFakeDucklingClient() fs := newFakeStore() fs.warehouses["org-probe"] = &configstore.ManagedWarehouse{ - OrgID: "org-probe", - State: configstore.ManagedWarehouseStateProvisioning, - CreatedAt: time.Now(), + OrgID: "org-probe", + DucklingName: "org-probe", + State: configstore.ManagedWarehouseStateProvisioning, + CreatedAt: time.Now(), } cr := &unstructured.Unstructured{ @@ -447,7 +454,7 @@ func TestReconcileProvisioningProbeFailsKeepsProvisioning(t *testing.T) { "apiVersion": "k8s.posthog.com/v1alpha1", "kind": "Duckling", "metadata": map[string]interface{}{ - "name": ducklingName("org-probe"), + "name": "org-probe", "namespace": ducklingNamespace, }, "status": map[string]interface{}{ @@ -526,10 +533,11 @@ func TestReconcileProvisioningProbesPgBouncerWhenEnabled(t *testing.T) { dc, fakeK8s := newFakeDucklingClient() fs := newFakeStore() fs.warehouses["org-pgb"] = &configstore.ManagedWarehouse{ - OrgID: "org-pgb", - State: configstore.ManagedWarehouseStateProvisioning, - PgBouncer: configstore.ManagedWarehousePgBouncer{Enabled: true}, - CreatedAt: time.Now(), + OrgID: "org-pgb", + DucklingName: "org-pgb", + State: configstore.ManagedWarehouseStateProvisioning, + PgBouncer: configstore.ManagedWarehousePgBouncer{Enabled: true}, + CreatedAt: time.Now(), } cr := &unstructured.Unstructured{ @@ -537,7 +545,7 @@ func TestReconcileProvisioningProbesPgBouncerWhenEnabled(t *testing.T) { "apiVersion": "k8s.posthog.com/v1alpha1", "kind": "Duckling", "metadata": map[string]interface{}{ - "name": ducklingName("org-pgb"), + "name": "org-pgb", "namespace": ducklingNamespace, }, "status": map[string]interface{}{ @@ -587,8 +595,9 @@ func TestReconcileDeletingDeletesCR(t *testing.T) { dc, fakeK8s := newFakeDucklingClient() fs := newFakeStore() fs.warehouses["org-c"] = &configstore.ManagedWarehouse{ - OrgID: "org-c", - State: configstore.ManagedWarehouseStateDeleting, + OrgID: "org-c", + DucklingName: "org-c", + State: configstore.ManagedWarehouseStateDeleting, } ctx := context.Background() @@ -598,7 +607,7 @@ func TestReconcileDeletingDeletesCR(t *testing.T) { "apiVersion": "k8s.posthog.com/v1alpha1", "kind": "Duckling", "metadata": map[string]interface{}{ - "name": ducklingName("org-c"), + "name": "org-c", "namespace": ducklingNamespace, }, }, @@ -612,7 +621,7 @@ func TestReconcileDeletingDeletesCR(t *testing.T) { ctrl.reconcile(ctx) // Verify CR is gone - _, err = fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(ctx, ducklingName("org-c"), metav1.GetOptions{}) + _, err = fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(ctx, "org-c", metav1.GetOptions{}) if err == nil { t.Fatal("expected CR to be deleted") return @@ -630,8 +639,9 @@ func TestReconcileDeletingRetriesOnNonNotFoundError(t *testing.T) { dc, _ := newFakeDucklingClient() fs := newFakeStore() fs.warehouses["org-d"] = &configstore.ManagedWarehouse{ - OrgID: "org-d", - State: configstore.ManagedWarehouseStateDeleting, + OrgID: "org-d", + DucklingName: "org-d", + State: configstore.ManagedWarehouseStateDeleting, } ctx := context.Background() @@ -717,8 +727,9 @@ func TestParseDucklingStatusEmpty(t *testing.T) { func TestFakeStoreUpdateWarehouseState(t *testing.T) { fs := newFakeStore() fs.warehouses["org-x"] = &configstore.ManagedWarehouse{ - OrgID: "org-x", - State: configstore.ManagedWarehouseStatePending, + OrgID: "org-x", + DucklingName: "org-x", + State: configstore.ManagedWarehouseStatePending, } // CAS update should succeed @@ -756,8 +767,9 @@ func TestReconcilePendingCreatesCnpgShardCR(t *testing.T) { dc, fakeK8s := newFakeDucklingClient() fs := newFakeStore() fs.warehouses["org-cnpg"] = &configstore.ManagedWarehouse{ - OrgID: "org-cnpg", - State: configstore.ManagedWarehouseStatePending, + OrgID: "org-cnpg", + DucklingName: "org-cnpg", + State: configstore.ManagedWarehouseStatePending, MetadataStore: configstore.ManagedWarehouseMetadataStore{ Kind: configstore.MetadataStoreKindCnpgShard, }, @@ -768,7 +780,7 @@ func TestReconcilePendingCreatesCnpgShardCR(t *testing.T) { ctx := context.Background() ctrl.reconcile(ctx) - cr, err := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(ctx, ducklingName("org-cnpg"), metav1.GetOptions{}) + cr, err := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(ctx, "org-cnpg", metav1.GetOptions{}) if err != nil { t.Fatalf("expected CR to exist: %v", err) } @@ -811,7 +823,7 @@ func TestDucklingCreateCnpgShardRequiresDuckLake(t *testing.T) { if !strings.Contains(err.Error(), "requires ducklake enabled") { t.Fatalf("error = %v, want 'requires ducklake enabled'", err) } - if _, getErr := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(ctx, ducklingName("no-catalog"), metav1.GetOptions{}); getErr == nil { + if _, getErr := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(ctx, "no-catalog", metav1.GetOptions{}); getErr == nil { t.Error("CR should not have been created when validation failed") } } @@ -833,9 +845,10 @@ func TestReconcileProvisioningCnpgShardReadiness(t *testing.T) { dc, fakeK8s := newFakeDucklingClient() fs := newFakeStore() fs.warehouses["org-cs"] = &configstore.ManagedWarehouse{ - OrgID: "org-cs", - State: configstore.ManagedWarehouseStateProvisioning, - CreatedAt: time.Now(), + OrgID: "org-cs", + DucklingName: "org-cs", + State: configstore.ManagedWarehouseStateProvisioning, + CreatedAt: time.Now(), MetadataStore: configstore.ManagedWarehouseMetadataStore{ Kind: configstore.MetadataStoreKindCnpgShard, }, @@ -847,7 +860,7 @@ func TestReconcileProvisioningCnpgShardReadiness(t *testing.T) { "apiVersion": "k8s.posthog.com/v1alpha1", "kind": "Duckling", "metadata": map[string]interface{}{ - "name": ducklingName("org-cs"), + "name": "org-cs", "namespace": ducklingNamespace, }, "status": map[string]interface{}{ @@ -899,8 +912,9 @@ func TestReconcilePendingCreatesDuckLakeExternalCR(t *testing.T) { dc, fakeK8s := newFakeDucklingClient() fs := newFakeStore() fs.warehouses["org-dl"] = &configstore.ManagedWarehouse{ - OrgID: "org-dl", - State: configstore.ManagedWarehouseStatePending, + OrgID: "org-dl", + DucklingName: "org-dl", + State: configstore.ManagedWarehouseStatePending, MetadataStore: configstore.ManagedWarehouseMetadataStore{ Kind: configstore.MetadataStoreKindExternal, Endpoint: "rds.example.us-east-1.rds.amazonaws.com", @@ -920,7 +934,7 @@ func TestReconcilePendingCreatesDuckLakeExternalCR(t *testing.T) { ctx := context.Background() ctrl.reconcile(ctx) - cr, err := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(ctx, ducklingName("org-dl"), metav1.GetOptions{}) + cr, err := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(ctx, "org-dl", metav1.GetOptions{}) if err != nil { t.Fatalf("expected CR to exist: %v", err) } @@ -963,7 +977,7 @@ func TestDucklingCreateExternalRequiresFields(t *testing.T) { if err := dc.Create(ctx, "ext-bad", CreateOptions{MetadataStoreType: configstore.MetadataStoreKindExternal}); err == nil { t.Fatal("expected error creating external CR without endpoint/passwordAwsSecret") } - if _, getErr := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(ctx, ducklingName("ext-bad"), metav1.GetOptions{}); getErr == nil { + if _, getErr := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(ctx, "ext-bad", metav1.GetOptions{}); getErr == nil { t.Error("CR should not have been created when external validation failed") } } @@ -983,49 +997,79 @@ func TestDucklingCreateExternalDataStoreRequiresBucket(t *testing.T) { } } -// TestDucklingGetFallsBackToLegacyName verifies the backward-compat path: a CR -// created before ducklingName preserved hyphens (i.e. named with hyphens -// stripped) is still found when looked up by its hyphenated org ID. Without -// this, switching ducklingName to keep hyphens would orphan existing prod -// ducklings whose org IDs contain hyphens (e.g. UUID-named tenants). -func TestDucklingGetFallsBackToLegacyName(t *testing.T) { +// TestDucklingCreateUsesNameVerbatim locks in the naming contract: Create +// names the CR exactly the passed name — no lowercasing, hyphen-stripping, or +// any other derivation. The name comes from the warehouse row's duckling_name. +func TestDucklingCreateUsesNameVerbatim(t *testing.T) { dc, fakeK8s := newFakeDucklingClient() ctx := context.Background() - org := "018d351a-9ff7-0000-eaff-4628875ad045" - legacy := legacyDucklingName(org) // "018d351a9ff70000eaff4628875ad045" - if ducklingName(org) == legacy { - t.Fatal("test premise: hyphenated org id must differ from its legacy de-hyphenated name") + name := "018d351a-9ff7-0000-eaff-4628875ad045" + if err := dc.Create(ctx, name, CreateOptions{ + MetadataStoreType: configstore.MetadataStoreKindCnpgShard, + DuckLakeEnabled: true, + }); err != nil { + t.Fatalf("Create: %v", err) } - - // Seed a CR under the legacy (de-hyphenated) name, as the old code would have. - cr := &unstructured.Unstructured{Object: map[string]interface{}{ - "apiVersion": "k8s.posthog.com/v1alpha1", - "kind": "Duckling", - "metadata": map[string]interface{}{"name": legacy, "namespace": ducklingNamespace}, - "status": map[string]interface{}{"iamRoleArn": "arn:aws:iam::123:role/duckling-" + legacy}, - }} - if _, err := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Create(ctx, cr, metav1.CreateOptions{}); err != nil { - t.Fatalf("seed legacy CR: %v", err) + cr, err := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(ctx, name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("expected CR named exactly %q: %v", name, err) + } + if cr.GetName() != name { + t.Fatalf("CR name = %q, want %q verbatim", cr.GetName(), name) } - st, err := dc.Get(ctx, org) - if err != nil { - t.Fatalf("Get must fall back to the legacy de-hyphenated name, got: %v", err) + // Lookups, mutation, and delete all take the same exact name. + if _, err := dc.Get(ctx, name); err != nil { + t.Errorf("Get by exact name: %v", err) + } + if _, err := dc.GetPgBouncerEnabled(ctx, name); err != nil { + t.Errorf("GetPgBouncerEnabled by exact name: %v", err) } - if st.IAMRoleARN == "" { - t.Error("expected to parse the legacy CR's status") + if err := dc.Delete(ctx, name); err != nil { + t.Errorf("Delete by exact name: %v", err) } + if _, getErr := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(ctx, name, metav1.GetOptions{}); getErr == nil { + t.Error("CR should have been deleted") + } +} - // And pgbouncer reads + delete must resolve it too. - if _, err := dc.GetPgBouncerEnabled(ctx, org); err != nil { - t.Errorf("GetPgBouncerEnabled fallback: %v", err) +// TestReconcileUsesStoredDucklingName verifies the controller addresses the +// Duckling CR by the warehouse row's stored duckling_name — not anything +// derived from the org ID — across the create and delete flows. +func TestReconcileUsesStoredDucklingName(t *testing.T) { + dc, fakeK8s := newFakeDucklingClient() + fs := newFakeStore() + fs.warehouses["org-stored"] = &configstore.ManagedWarehouse{ + OrgID: "org-stored", + DucklingName: "custom-cr-name", + State: configstore.ManagedWarehouseStatePending, + MetadataStore: configstore.ManagedWarehouseMetadataStore{ + Kind: configstore.MetadataStoreKindCnpgShard, + }, + DuckLake: configstore.ManagedWarehouseDuckLake{Enabled: true}, } - if err := dc.Delete(ctx, org); err != nil { - t.Errorf("Delete fallback: %v", err) + + ctrl := NewControllerWithClient(fs, dc, time.Second) + ctx := context.Background() + ctrl.reconcile(ctx) + + // The CR must be created under the stored name, not the org ID. + if _, err := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(ctx, "custom-cr-name", metav1.GetOptions{}); err != nil { + t.Fatalf("expected CR under stored duckling_name: %v", err) + } + if _, err := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(ctx, "org-stored", metav1.GetOptions{}); err == nil { + t.Fatal("no CR should exist under the org ID when duckling_name differs") + } + + // Deletion resolves through the same stored name. + fs.warehouses["org-stored"].State = configstore.ManagedWarehouseStateDeleting + ctrl.reconcile(ctx) + if _, err := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(ctx, "custom-cr-name", metav1.GetOptions{}); err == nil { + t.Fatal("expected CR deleted via stored duckling_name") } - if _, getErr := fakeK8s.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(ctx, legacy, metav1.GetOptions{}); getErr == nil { - t.Error("legacy CR should have been deleted via the fallback") + if fs.warehouses["org-stored"].State != configstore.ManagedWarehouseStateDeleted { + t.Fatalf("expected deleted state, got %q", fs.warehouses["org-stored"].State) } } diff --git a/controlplane/provisioner/k8s_client.go b/controlplane/provisioner/k8s_client.go index 84fc607b..36c5fb11 100644 --- a/controlplane/provisioner/k8s_client.go +++ b/controlplane/provisioner/k8s_client.go @@ -79,20 +79,6 @@ func NewDucklingClientWithDynamic(client dynamic.Interface) *DucklingClient { return &DucklingClient{client: client} } -// ducklingName is the k8s/AWS resource name derived from an org ID, used for -// the Duckling CR, the IAM role (duckling-), the S3 bucket, etc. Org IDs -// are validated as DNS-1123 labels at -// provision time (lowercase alphanumerics + hyphens), so this only lowercases: -// hyphens are preserved, keeping in-cluster names human-readable and injective -// with the org ID. -// -// (It used to strip hyphens to keep per-org Aurora RDS cluster identifiers -// short, but the control plane no longer provisions per-org Aurora clusters, -// and stripping was lossy — "a-b" and "ab" collided.) -func ducklingName(orgID string) string { - return strings.ToLower(orgID) -} - // pgIdentSanitizeRe matches characters not allowed in an unquoted Postgres // identifier fragment. var pgIdentSanitizeRe = regexp.MustCompile(`[^a-z0-9_]`) @@ -109,15 +95,6 @@ func pgIdentSuffix(orgID string) string { return pgIdentSanitizeRe.ReplaceAllString(strings.ToLower(orgID), "_") } -// legacyDucklingName is the pre-hyphen-preservation transform (hyphens -// stripped). Retained ONLY so lookups can still find Duckling CRs created -// before ducklingName started preserving hyphens — e.g. a CR named -// "018d351a9ff70000eaff4628875ad045" for org "018d351a-9ff7-0000-eaff-...". -// New CRs are always created under ducklingName (hyphen-preserving). -func legacyDucklingName(orgID string) string { - return strings.ReplaceAll(strings.ToLower(orgID), "-", "") -} - // CreateOptions carries per-org knobs that shape the generated Duckling CR. type CreateOptions struct { // MetadataStoreType selects the Duckling's metadata-store backend. The @@ -147,10 +124,10 @@ type CreateOptions struct { DuckLakeEnabled bool } -// Create creates a Duckling CR for the given org. -func (d *DucklingClient) Create(ctx context.Context, orgID string, opts CreateOptions) error { - name := ducklingName(orgID) - +// Create creates a Duckling CR named exactly `name`. The name comes from the +// warehouse row's duckling_name (authoritative; the control plane never +// derives it) and is used verbatim. +func (d *DucklingClient) Create(ctx context.Context, name string, opts CreateOptions) error { var metadataStore map[string]interface{} switch opts.MetadataStoreType { case configstore.MetadataStoreKindCnpgShard: @@ -251,23 +228,10 @@ func (d *DucklingClient) Create(ctx context.Context, orgID string, opts CreateOp return nil } -// getCR fetches the org's Duckling CR, trying the current hyphen-preserving -// name first and falling back to the legacy de-hyphenated name for CRs created -// before ducklingName preserved hyphens. Returns the CR and the name it was -// found under (callers that mutate need the actual name). When neither exists, -// returns the current-name NotFound error so callers see the new scheme. -func (d *DucklingClient) getCR(ctx context.Context, orgID string) (*unstructured.Unstructured, string, error) { - name := ducklingName(orgID) - cr, err := d.client.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(ctx, name, metav1.GetOptions{}) - if err == nil { - return cr, name, nil - } - if legacy := legacyDucklingName(orgID); legacy != name && apierrors.IsNotFound(err) { - if lcr, lerr := d.client.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(ctx, legacy, metav1.GetOptions{}); lerr == nil { - return lcr, legacy, nil - } - } - return nil, name, err +// getCR fetches a Duckling CR by its exact name — the warehouse row's +// duckling_name. The control plane never derives or re-maps the name. +func (d *DucklingClient) getCR(ctx context.Context, name string) (*unstructured.Unstructured, error) { + return d.client.Resource(ducklingGVR).Namespace(ducklingNamespace).Get(ctx, name, metav1.GetOptions{}) } // ListCRNames returns the names of every Duckling CR in the namespace. Used by @@ -317,12 +281,12 @@ func (d *DucklingClient) CRMetadataStores(ctx context.Context) (map[string]CRMet return out, nil } -// CRStatus reports whether the org's Duckling CR exists and, if so, whether it +// CRStatus reports whether the named Duckling CR exists and, if so, whether it // is Ready — without treating absence as an error. A NotFound resolves to // (false, false, nil) so the drift finder can classify a missing CR rather than // surfacing a 500. Any other error is returned as-is. -func (d *DucklingClient) CRStatus(ctx context.Context, orgID string) (present bool, ready bool, err error) { - cr, _, gerr := d.getCR(ctx, orgID) +func (d *DucklingClient) CRStatus(ctx context.Context, name string) (present bool, ready bool, err error) { + cr, gerr := d.getCR(ctx, name) if gerr != nil { if apierrors.IsNotFound(gerr) { return false, false, nil @@ -336,35 +300,33 @@ func (d *DucklingClient) CRStatus(ctx context.Context, orgID string) (present bo return true, status.ReadyCondition, nil } -// Get fetches the Duckling CR and parses its status. -func (d *DucklingClient) Get(ctx context.Context, orgID string) (*DucklingStatus, error) { - cr, name, err := d.getCR(ctx, orgID) +// Get fetches the named Duckling CR and parses its status. +func (d *DucklingClient) Get(ctx context.Context, name string) (*DucklingStatus, error) { + cr, err := d.getCR(ctx, name) if err != nil { return nil, fmt.Errorf("get duckling CR %q: %w", name, err) } return parseDucklingStatus(cr) } -// Delete removes the Duckling CR for the given org. Resolves the legacy name so -// pre-rename CRs are still deletable. -func (d *DucklingClient) Delete(ctx context.Context, orgID string) error { - _, name, err := d.getCR(ctx, orgID) - if apierrors.IsNotFound(err) { - // Already gone — nothing to delete. (reconcileDeleting treats this as success.) - return err - } +// Delete removes the named Duckling CR. +func (d *DucklingClient) Delete(ctx context.Context, name string) error { if derr := d.client.Resource(ducklingGVR).Namespace(ducklingNamespace).Delete(ctx, name, metav1.DeleteOptions{}); derr != nil { + if apierrors.IsNotFound(derr) { + // Already gone — nothing to delete. (reconcileDeleting treats this as success.) + return derr + } return fmt.Errorf("delete duckling CR %q: %w", name, derr) } return nil } // GetPgBouncerEnabled reads spec.metadataStore.pgbouncer.enabled from the -// Duckling CR. Missing blocks (composition at an older schema, CR never +// named Duckling CR. Missing blocks (composition at an older schema, CR never // carried a pgbouncer section) are reported as false — same as an explicit // opt-out — so the caller just needs to compare against the desired value. -func (d *DucklingClient) GetPgBouncerEnabled(ctx context.Context, orgID string) (bool, error) { - cr, name, err := d.getCR(ctx, orgID) +func (d *DucklingClient) GetPgBouncerEnabled(ctx context.Context, name string) (bool, error) { + cr, err := d.getCR(ctx, name) if err != nil { return false, fmt.Errorf("get duckling CR %q: %w", name, err) } @@ -385,14 +347,10 @@ func (d *DucklingClient) GetPgBouncerEnabled(ctx context.Context, orgID string) } // SetPgBouncerEnabled patches spec.metadataStore.pgbouncer.enabled on the -// Duckling CR for the given org. Uses a JSON merge patch (RFC 7396) so the +// named Duckling CR. Uses a JSON merge patch (RFC 7396) so the // call is idempotent and only touches the pgbouncer block — sibling fields // under metadataStore (type, external) are left untouched. -func (d *DucklingClient) SetPgBouncerEnabled(ctx context.Context, orgID string, enabled bool) error { - _, name, err := d.getCR(ctx, orgID) - if err != nil { - return fmt.Errorf("resolve duckling CR for %q: %w", orgID, err) - } +func (d *DucklingClient) SetPgBouncerEnabled(ctx context.Context, name string, enabled bool) error { patch, err := json.Marshal(map[string]interface{}{ "spec": map[string]interface{}{ "metadataStore": map[string]interface{}{ @@ -414,12 +372,12 @@ func (d *DucklingClient) SetPgBouncerEnabled(ctx context.Context, orgID string, return nil } -// GetDataStoreBucketName reads spec.dataStore.bucketName from the Duckling CR. -// Empty (missing block / missing key) means the CR predates CP-owned naming -// and the composition is still deriving the name — the signal the backfill in -// reconcileReady uses to decide whether to patch. -func (d *DucklingClient) GetDataStoreBucketName(ctx context.Context, orgID string) (string, error) { - cr, name, err := d.getCR(ctx, orgID) +// GetDataStoreBucketName reads spec.dataStore.bucketName from the named +// Duckling CR. Empty (missing block / missing key) means the CR predates +// CP-owned naming and the composition is still deriving the name — the signal +// the backfill in reconcileReady uses to decide whether to patch. +func (d *DucklingClient) GetDataStoreBucketName(ctx context.Context, name string) (string, error) { + cr, err := d.getCR(ctx, name) if err != nil { return "", fmt.Errorf("get duckling CR %q: %w", name, err) } @@ -435,19 +393,15 @@ func (d *DucklingClient) GetDataStoreBucketName(ctx context.Context, orgID strin return bucket, nil } -// SetDataStoreBucketName patches spec.dataStore.bucketName on the Duckling CR. -// JSON merge patch (RFC 7396) so it's idempotent and only touches dataStore — -// the type field and any sibling under spec are left untouched. Used to backfill -// the CP-owned name onto ducklings created before this field existed (their -// spec.dataStore carries only {type: s3bucket}); the name supplied here is the -// same string the composition was already deriving into status, so the patch -// causes no bucket churn — it just moves the name from a derived output into a -// durable input so the composition stops deriving. -func (d *DucklingClient) SetDataStoreBucketName(ctx context.Context, orgID, bucket string) error { - _, name, err := d.getCR(ctx, orgID) - if err != nil { - return fmt.Errorf("resolve duckling CR for %q: %w", orgID, err) - } +// SetDataStoreBucketName patches spec.dataStore.bucketName on the named +// Duckling CR. JSON merge patch (RFC 7396) so it's idempotent and only touches +// dataStore — the type field and any sibling under spec are left untouched. +// Used to backfill the CP-owned name onto ducklings created before this field +// existed (their spec.dataStore carries only {type: s3bucket}); the name +// supplied here is the same string the composition was already deriving into +// status, so the patch causes no bucket churn — it just moves the name from a +// derived output into a durable input so the composition stops deriving. +func (d *DucklingClient) SetDataStoreBucketName(ctx context.Context, name, bucket string) error { patch, err := json.Marshal(map[string]interface{}{ "spec": map[string]interface{}{ "dataStore": map[string]interface{}{ diff --git a/controlplane/provisioner/naming_test.go b/controlplane/provisioner/naming_test.go index 9142297d..ebcd538f 100644 --- a/controlplane/provisioner/naming_test.go +++ b/controlplane/provisioner/naming_test.go @@ -4,26 +4,6 @@ package provisioner import "testing" -// TestDucklingNamePreservesHyphens locks in the post-fix behavior: k8s/AWS -// resource names keep hyphens (only lowercasing is applied), and the transform -// is injective — the regression being the old de-hyphenation where "a-b" and -// "ab" both collapsed to "ab". -func TestDucklingNamePreservesHyphens(t *testing.T) { - for in, want := range map[string]string{ - "ben-warehouse-cnpg": "ben-warehouse-cnpg", - "Ben-Warehouse": "ben-warehouse", - "team123": "team123", - "f47ac10b-58cc-4372-a567-0e02b2c3d479": "f47ac10b-58cc-4372-a567-0e02b2c3d479", - } { - if got := ducklingName(in); got != want { - t.Errorf("ducklingName(%q) = %q, want %q", in, got, want) - } - } - if ducklingName("a-b") == ducklingName("ab") { - t.Error("ducklingName must not collide \"a-b\" with \"ab\" (the old de-hyphenation bug)") - } -} - // TestPgIdentSuffixSanitizes verifies the Postgres-identifier transform maps // hyphens to underscores (PG identifiers can't be unquoted-hyphenated) and // stays injective for [a-z0-9-] inputs. diff --git a/controlplane/provisioning/api.go b/controlplane/provisioning/api.go index b99f605b..c265c610 100644 --- a/controlplane/provisioning/api.go +++ b/controlplane/provisioning/api.go @@ -5,7 +5,6 @@ import ( "fmt" "net/http" "regexp" - "strings" "time" "github.com/gin-gonic/gin" @@ -252,11 +251,10 @@ func (h *handler) provisionWarehouse(c *gin.Context) { warehouse := &configstore.ManagedWarehouse{ DataStore: ds, DuckLake: configstore.ManagedWarehouseDuckLake{Enabled: ducklakeEnabled}, - // Stamp the canonical Duckling CR name now so lookups never have to - // re-derive it. lower(orgID) mirrors provisioner.ducklingName (org IDs - // are validated DNS-1123 labels, so lowercasing is the whole transform); - // we inline it rather than import provisioner into this package. - DucklingName: strings.ToLower(orgID), + // Stamp the authoritative Duckling CR name: the org ID verbatim. Org IDs + // are validated as lowercase DNS-1123 labels at this endpoint, so no + // transform is needed — and nothing downstream ever derives the name. + DucklingName: orgID, } if warehouse.DucklingName == "" { c.JSON(http.StatusBadRequest, gin.H{"error": "duckling_name is required"}) diff --git a/querylog_writer_mode_kubernetes.go b/querylog_writer_mode_kubernetes.go index 2d7083fe..5cdbe758 100644 --- a/querylog_writer_mode_kubernetes.go +++ b/querylog_writer_mode_kubernetes.go @@ -186,7 +186,18 @@ func newQueryLogDuckLakeConfigResolver(ctx context.Context, cfg server.Config, r slog.Warn("Duckling client unavailable for query-log writer; using config store runtime only.", "error", err) } else { resolveDucklingStatus = func(ctx context.Context, orgID string) (*provisioner.DucklingStatus, error) { - return ducklingClient.Get(ctx, orgID) + // The CR name is the warehouse row's duckling_name (authoritative, + // never derived). The column is NOT NULL and backfilled; the org-ID + // fallback only covers legacy in-flight rows with an empty value. + warehouse, err := store.GetManagedWarehouse(orgID) + if err != nil { + return nil, fmt.Errorf("resolve duckling name for org %q: %w", orgID, err) + } + name := warehouse.DucklingName + if name == "" { + name = orgID + } + return ducklingClient.Get(ctx, name) } }