From c9df088331eafebc2c885c176c3628912363665f Mon Sep 17 00:00:00 2001 From: Claudio Busse Date: Tue, 11 Aug 2026 10:03:39 +0200 Subject: [PATCH] Ensure unique hash4 DNS slug per cluster name at creation time Add a cross-account collision check in the platform-api Create handler that queries all clusters sharing the same name and verifies no existing cluster has a matching 4-character UUID prefix. On collision, the UUID is regenerated (up to 5 attempts). UUID generation is injectable for deterministic testing of collision and retry paths. Co-Authored-By: Claude Opus 4.6 --- .../migrations/002_cluster_dns_uniqueness.sql | 10 ++ hyperfleet-db/internal/schema/schema.go | 1 + .../docs/cluster-controller.md | 4 +- platform-api/pkg/handlers/cluster.go | 56 +++--- platform-api/pkg/handlers/cluster_test.go | 168 +++++++++++++++++- 5 files changed, 214 insertions(+), 25 deletions(-) create mode 100644 hyperfleet-db/internal/schema/migrations/002_cluster_dns_uniqueness.sql diff --git a/hyperfleet-db/internal/schema/migrations/002_cluster_dns_uniqueness.sql b/hyperfleet-db/internal/schema/migrations/002_cluster_dns_uniqueness.sql new file mode 100644 index 00000000..463478ee --- /dev/null +++ b/hyperfleet-db/internal/schema/migrations/002_cluster_dns_uniqueness.sql @@ -0,0 +1,10 @@ +-- DNS FQDNs include a hash4 slug (first 4 chars of internalId) to disambiguate +-- clusters sharing the same human-readable name across accounts. Two live +-- clusters with the same (name, hash4) would produce identical DNS records, +-- so we enforce uniqueness here as the atomic safety net behind the +-- platform-api's application-level collision check. +CREATE UNIQUE INDEX IF NOT EXISTS idx_cluster_name_hash4 + ON kubernetes_resources (name, (LEFT(spec->>'internalId', 4))) + WHERE gvk = 'hyperfleet.io/v1alpha1/Cluster' + AND deletion_timestamp IS NULL + AND spec->>'internalId' IS NOT NULL; diff --git a/hyperfleet-db/internal/schema/schema.go b/hyperfleet-db/internal/schema/schema.go index a3ca5568..548c3c7f 100644 --- a/hyperfleet-db/internal/schema/schema.go +++ b/hyperfleet-db/internal/schema/schema.go @@ -14,6 +14,7 @@ var migrationsFS embed.FS func Migrate(ctx context.Context, conn *pgx.Conn) error { files := []string{ "migrations/001_initial.sql", + "migrations/002_cluster_dns_uniqueness.sql", } for _, f := range files { sql, err := migrationsFS.ReadFile(f) diff --git a/hyperfleet-operator/docs/cluster-controller.md b/hyperfleet-operator/docs/cluster-controller.md index 3c141c21..c7421558 100644 --- a/hyperfleet-operator/docs/cluster-controller.md +++ b/hyperfleet-operator/docs/cluster-controller.md @@ -63,7 +63,7 @@ The controller generates 7 Kubernetes manifests, all scoped to namespace `cluste ### DNS and hash4 -The `hash4` value is the first 4 characters of the cluster ID (the CR name). It provides short, collision-resistant subdomains: +The `hash4` value is the first 4 characters of the cluster ID (a UUID). It provides short, unique subdomains that disambiguate clusters sharing the same human-readable name: - API server: `api.{clusterName}.{hash4}.{baseDomain}` - OAuth: `oauth.{clusterName}.{hash4}.{baseDomain}` @@ -72,6 +72,8 @@ The `hash4` value is the first 4 characters of the cluster ID (the CR name). It For example, cluster ID `abc12345` with name `my-cluster` and baseDomain `rosa.example.com` produces `api.my-cluster.abc1.rosa.example.com`. +**Uniqueness guarantee**: A PostgreSQL unique partial index (`idx_cluster_name_hash4`) enforces that no two live clusters with the same name share the same hash4 prefix. If a collision occurs during creation, the platform-api retries with a new UUID (up to 5 attempts). With 16^4 = 65,536 possible hex values per name, exhaustion is practically impossible. + ## Deletion Flow Deletion follows a strict ordering: NodePools first, then HostedCluster (so HyperShift can clean up workers and load balancers), then the namespace (cascading remaining resources). ApplyDesire specs are always removed before DeleteDesires are written to prevent kube-applier from racing and re-applying resources being deleted. diff --git a/platform-api/pkg/handlers/cluster.go b/platform-api/pkg/handlers/cluster.go index ac6ece76..7aa9e2dd 100644 --- a/platform-api/pkg/handlers/cluster.go +++ b/platform-api/pkg/handlers/cluster.go @@ -27,6 +27,7 @@ type ClusterHandler struct { defaultClusterExpiration time.Duration validator *validation.FieldValidator logger *slog.Logger + generateID func() string } // NewClusterHandler creates a new cluster handler @@ -37,6 +38,7 @@ func NewClusterHandler(db *hyperfleetdb.Client, oidcIssuerBaseURL string, defaul defaultClusterExpiration: defaultClusterExpiration, validator: validation.NewFieldValidator(), logger: logger, + generateID: func() string { return uuid.New().String() }, } } @@ -136,38 +138,46 @@ func (h *ClusterHandler) Create(w http.ResponseWriter, r *http.Request) { req.Spec.CreatorARN = callerARN } - clusterID := uuid.New().String() + clusterID := h.generateID() - h.logger.Info("creating cluster", "account_id", accountID, "cluster_name", req.Name, "cluster_id", clusterID) + const maxHash4Retries = 5 + for attempt := 0; attempt < maxHash4Retries; attempt++ { + h.logger.Info("creating cluster", "account_id", accountID, "cluster_name", req.Name, "cluster_id", clusterID) - cr, err := hyperfleetdb.PlatformCreateToClusterCR(clusterID, accountID, &req) - if err != nil { - h.logger.Error("failed to convert cluster spec", "error", err, "account_id", accountID) - h.writeError(w, http.StatusBadRequest, "CLUSTERS-MGMT-CREATE-002", "Invalid cluster spec") - return - } + cr, err := hyperfleetdb.PlatformCreateToClusterCR(clusterID, accountID, &req) + if err != nil { + h.logger.Error("failed to convert cluster spec", "error", err, "account_id", accountID) + h.writeError(w, http.StatusBadRequest, "CLUSTERS-MGMT-CREATE-002", "Invalid cluster spec") + return + } - if h.defaultClusterExpiration > 0 && cr.Spec.ExpirationTimestamp == nil { - expiry := metav1.NewTime(time.Now().Add(h.defaultClusterExpiration)) - cr.Spec.ExpirationTimestamp = &expiry - } + if h.defaultClusterExpiration > 0 && cr.Spec.ExpirationTimestamp == nil { + expiry := metav1.NewTime(time.Now().Add(h.defaultClusterExpiration)) + cr.Spec.ExpirationTimestamp = &expiry + } - if h.oidcIssuerBaseURL != "" { - cr.Spec.HostedCluster.IssuerURL = h.oidcIssuerBaseURL + "/" + clusterID - } + if h.oidcIssuerBaseURL != "" { + cr.Spec.HostedCluster.IssuerURL = h.oidcIssuerBaseURL + "/" + clusterID + } - if err := h.db.CreateCluster(ctx, accountID, cr); err != nil { - h.logger.Error("failed to create cluster", "error", err, "account_id", accountID) - if hyperfleetdb.IsAlreadyExists(err) { - h.writeError(w, http.StatusConflict, "CLUSTERS-MGMT-CREATE-003", "Cluster already exists") + if err := h.db.CreateCluster(ctx, accountID, cr); err != nil { + if hyperfleetdb.IsAlreadyExists(err) && attempt < maxHash4Retries-1 { + clusterID = h.generateID() + continue + } + h.logger.Error("failed to create cluster", "error", err, "account_id", accountID) + if hyperfleetdb.IsAlreadyExists(err) { + h.writeError(w, http.StatusInternalServerError, "CLUSTERS-MGMT-CREATE-007", "Unable to generate unique DNS identifier") + return + } + h.writeError(w, http.StatusInternalServerError, "CLUSTERS-MGMT-CREATE-003", "Failed to create cluster") return } - h.writeError(w, http.StatusInternalServerError, "CLUSTERS-MGMT-CREATE-003", "Failed to create cluster") + + cluster := hyperfleetdb.ClusterCRToPlatform(cr) + h.writeJSON(w, http.StatusCreated, cluster) return } - - cluster := hyperfleetdb.ClusterCRToPlatform(cr) - h.writeJSON(w, http.StatusCreated, cluster) } // Get handles GET /api/v0/clusters/{id} diff --git a/platform-api/pkg/handlers/cluster_test.go b/platform-api/pkg/handlers/cluster_test.go index cccd61d9..6b9eb06c 100644 --- a/platform-api/pkg/handlers/cluster_test.go +++ b/platform-api/pkg/handlers/cluster_test.go @@ -6,17 +6,22 @@ import ( "bytes" "context" "encoding/json" + "fmt" "log/slog" "net/http" "net/http/httptest" "os" + "sync" + "sync/atomic" "testing" - "time" "github.com/gorilla/mux" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" hyperfleetv1alpha1 "github.com/openshift-online/rosa-hyperfleet-api/api/v1alpha1" @@ -550,3 +555,164 @@ func TestClusterHandler_Create_SameNameDifferentAccount(t *testing.T) { t.Fatalf("expected 201 (same name in different account is allowed), got %d: %s", w.Code, w.Body.String()) } } + +func sequenceIDGen(ids ...string) func() string { + i := 0 + return func() string { + id := ids[i] + if i < len(ids)-1 { + i++ + } + return id + } +} + +func TestClusterHandler_Create_Hash4CollisionThenSuccess(t *testing.T) { + existing := testClusterCR("aaaa-existing", "test-cluster", "999999999999") + existing.Spec.InternalID = "aaaa-existing" + + scheme := newTestScheme() + innerFC := fake.NewClientBuilder().WithScheme(scheme).WithObjects(existing).Build() + fc := &hash4UniqueClient{Client: innerFC} + logger := slog.New(slog.NewTextHandler(os.Stdout, nil)) + handler := NewClusterHandler(hyperfleetdb.NewClientFrom(fc, logger), "https://oidc.example.com", 0, logger) + handler.generateID = sequenceIDGen("aaaa-1111-1111-1111", "cccc-2222-2222-2222") + + body, _ := json.Marshal(map[string]any{ + "name": "test-cluster", + "spec": map[string]any{}, + }) + + req := httptest.NewRequest(http.MethodPost, "/api/v0/clusters", bytes.NewReader(body)) + req = req.WithContext(testContext(testAccountID)) + + w := httptest.NewRecorder() + handler.Create(w, req) + + if w.Code != http.StatusCreated { + t.Fatalf("expected 201 after retry, got %d: %s", w.Code, w.Body.String()) + } + + var result map[string]any + _ = json.NewDecoder(w.Body).Decode(&result) + if id, ok := result["id"].(string); !ok || id != "cccc-2222-2222-2222" { + t.Errorf("expected cluster ID cccc-2222-2222-2222, got %v", result["id"]) + } +} + +func TestClusterHandler_Create_Hash4ExhaustedRetries(t *testing.T) { + existing := testClusterCR("aaaa-existing", "test-cluster", "999999999999") + existing.Spec.InternalID = "aaaa-existing" + + scheme := newTestScheme() + innerFC := fake.NewClientBuilder().WithScheme(scheme).WithObjects(existing).Build() + fc := &hash4UniqueClient{Client: innerFC} + logger := slog.New(slog.NewTextHandler(os.Stdout, nil)) + handler := NewClusterHandler(hyperfleetdb.NewClientFrom(fc, logger), "https://oidc.example.com", 0, logger) + handler.generateID = sequenceIDGen( + "aaaa-1111-1111-1111", + "aaaa-2222-2222-2222", + "aaaa-3333-3333-3333", + "aaaa-4444-4444-4444", + "aaaa-5555-5555-5555", + ) + + body, _ := json.Marshal(map[string]any{ + "name": "test-cluster", + "spec": map[string]any{}, + }) + + req := httptest.NewRequest(http.MethodPost, "/api/v0/clusters", bytes.NewReader(body)) + req = req.WithContext(testContext(testAccountID)) + + w := httptest.NewRecorder() + handler.Create(w, req) + + if w.Code != http.StatusInternalServerError { + t.Fatalf("expected 500 after exhausted retries, got %d: %s", w.Code, w.Body.String()) + } + + var errResp map[string]any + _ = json.NewDecoder(w.Body).Decode(&errResp) + if errResp["code"] != "CLUSTERS-MGMT-CREATE-007" { + t.Errorf("expected code CLUSTERS-MGMT-CREATE-007, got %v", errResp["code"]) + } +} + +// hash4UniqueClient wraps a client.Client to enforce hash4 uniqueness on +// Cluster creates, modeling the database's idx_cluster_name_hash4 unique index. +type hash4UniqueClient struct { + client.Client + mu sync.Mutex +} + +func (c *hash4UniqueClient) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + c.mu.Lock() + defer c.mu.Unlock() + + if cluster, ok := obj.(*hyperfleetv1alpha1.Cluster); ok { + if id := cluster.Spec.InternalID; len(id) >= 4 { + var list hyperfleetv1alpha1.ClusterList + if err := c.Client.List(ctx, &list); err != nil { + return err + } + for i := range list.Items { + existing := &list.Items[i] + if existing.Name == cluster.Name && + len(existing.Spec.InternalID) >= 4 && + existing.Spec.InternalID[:4] == id[:4] { + return apierrors.NewAlreadyExists( + schema.GroupResource{Resource: "clusters"}, cluster.Name) + } + } + } + } + return c.Client.Create(ctx, obj, opts...) +} + +func TestClusterHandler_Create_ConcurrentHash4Collision(t *testing.T) { + scheme := newTestScheme() + innerFC := fake.NewClientBuilder().WithScheme(scheme).Build() + fc := &hash4UniqueClient{Client: innerFC} + + logger := slog.New(slog.NewTextHandler(os.Stdout, nil)) + handler := NewClusterHandler(hyperfleetdb.NewClientFrom(fc, logger), "https://oidc.example.com", 0, logger) + + var callCount int64 + handler.generateID = func() string { + n := atomic.AddInt64(&callCount, 1) + return fmt.Sprintf("aaaa-%04d-0000-0000", n) + } + + var wg sync.WaitGroup + codes := make([]int, 2) + + for i := 0; i < 2; i++ { + wg.Add(1) + go func(idx int) { + defer wg.Done() + acct := fmt.Sprintf("account-%d", idx) + body, _ := json.Marshal(map[string]any{ + "name": "concurrent-cluster", + "spec": map[string]any{}, + }) + req := httptest.NewRequest(http.MethodPost, "/api/v0/clusters", bytes.NewReader(body)) + req = req.WithContext(testContext(acct)) + w := httptest.NewRecorder() + handler.Create(w, req) + codes[idx] = w.Code + }(i) + } + + wg.Wait() + + var created int + for _, code := range codes { + if code == http.StatusCreated { + created++ + } + } + if created != 1 { + t.Fatalf("expected exactly one 201 Created, got codes %v", codes) + } +}