Skip to content

HYPERFLEET-619 - fix: Prevent duplicate nodepool names within a cluster #53

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 1 commit into from
Feb 16, 2026
Merged

HYPERFLEET-619 - fix: Prevent duplicate nodepool names within a cluster #53

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions pkg/db/migrations/202511111055_add_node_pools.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,13 @@ func addNodePools() *gormigrate.Migration {
return err
}

// Create unique index on (owner_id, name) to prevent duplicate nodepool names within a cluster
createUniqueIdxSQL := "CREATE UNIQUE INDEX IF NOT EXISTS idx_node_pools_owner_name " +
"ON node_pools(owner_id, name) WHERE deleted_at IS NULL;"
if err := tx.Exec(createUniqueIdxSQL).Error; err != nil {
return err
}
Comment on lines +66 to +71
Copy link

@coderabbitai coderabbitai bot Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Handle existing duplicate nodepool names before creating the unique index.

Line 66–69 will fail the migration if any existing (owner_id, name) duplicates exist among non-deleted rows, which can block upgrades on live clusters. Consider a preflight duplicate check with a clear error (or a cleanup step) before creating the index.

🧪 Suggested preflight check (with explicit error) 
 import (
+	"fmt"
 	"gorm.io/gorm"

 	"github.com/go-gormigrate/gormigrate/v2"
 )
@@
 			// Create unique index on (owner_id, name) to prevent duplicate nodepool names within a cluster
+			type dupRow struct {
+				OwnerID string
+				Name    string
+				Count   int64
+			}
+			var dupes []dupRow
+			if err := tx.Raw(`
+				SELECT owner_id, name, COUNT(*) AS count
+				FROM node_pools
+				WHERE deleted_at IS NULL
+				GROUP BY owner_id, name
+				HAVING COUNT(*) > 1;
+			`).Scan(&dupes).Error; err != nil {
+				return err
+			}
+			if len(dupes) > 0 {
+				return fmt.Errorf("duplicate nodepool names exist: %+v", dupes)
+			}
 			createUniqueIdxSQL := "CREATE UNIQUE INDEX IF NOT EXISTS idx_node_pools_owner_name " +
 				"ON node_pools(owner_id, name) WHERE deleted_at IS NULL;"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Create unique index on (owner_id, name) to prevent duplicate nodepool names within a cluster
createUniqueIdxSQL := "CREATE UNIQUE INDEX IF NOT EXISTS idx_node_pools_owner_name " +
"ON node_pools(owner_id, name) WHERE deleted_at IS NULL;"
if err := tx.Exec(createUniqueIdxSQL).Error; err != nil {
return err
}
import (
"fmt"
"gorm.io/gorm"
"github.com/go-gormigrate/gormigrate/v2"
)
// ... within the migration function ...
// Create unique index on (owner_id, name) to prevent duplicate nodepool names within a cluster
type dupRow struct {
OwnerID string
Name string
Count int64
}
var dupes []dupRow
if err := tx.Raw(`
SELECT owner_id, name, COUNT(*) AS count
FROM node_pools
WHERE deleted_at IS NULL
GROUP BY owner_id, name
HAVING COUNT(*) > 1;
`).Scan(&dupes).Error; err != nil {
return err
}
if len(dupes) > 0 {
return fmt.Errorf("duplicate nodepool names exist: %+v", dupes)
}
createUniqueIdxSQL := "CREATE UNIQUE INDEX IF NOT EXISTS idx_node_pools_owner_name " +
"ON node_pools(owner_id, name) WHERE deleted_at IS NULL;"
if err := tx.Exec(createUniqueIdxSQL).Error; err != nil {
return err
}
🤖 Prompt for AI Agents 
In `@pkg/db/migrations/202511111055_add_node_pools.go` around lines 66 - 71,
Before executing tx.Exec(createUniqueIdxSQL) to create
idx_node_pools_owner_name, run a preflight duplicate check against node_pools
for rows with deleted_at IS NULL (e.g., SELECT owner_id, name, COUNT(*) FROM
node_pools WHERE deleted_at IS NULL GROUP BY owner_id, name HAVING COUNT(*) > 1)
using the same tx; if the query returns any rows, return a clear error from the
migration that lists the offending owner_id/name (and counts) or perform an
explicit cleanup/deduplication step, and only proceed to execute
createUniqueIdxSQL when no duplicates are found.


// Add foreign key constraint to clusters
addFKSQL := `
ALTER TABLE node_pools
Expand All @@ -83,6 +90,9 @@ func addNodePools() *gormigrate.Migration {
}

// Drop indexes
if err := tx.Exec("DROP INDEX IF EXISTS idx_node_pools_owner_name;").Error; err != nil {
return err
}
if err := tx.Exec("DROP INDEX IF EXISTS idx_node_pools_owner_id;").Error; err != nil {
return err
}
Expand Down
53 changes: 51 additions & 2 deletions test/integration/node_pools_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ import (
"github.com/openshift-hyperfleet/hyperfleet-api/test"
)

const (
kindNodePool = "NodePool"
)

// TestNodePoolGet is disabled because GET /nodepools/{id} is not in the OpenAPI spec
// The API only supports:
// - GET /api/hyperfleet/v1/nodepools (list all nodepools)
Expand Down Expand Up @@ -58,7 +62,7 @@ func TestNodePoolPost(t *testing.T) {
Expect(err).NotTo(HaveOccurred())

// POST responses per openapi spec: 201, 409, 500
kind := "NodePool"
kind := kindNodePool
nodePoolInput := openapi.NodePoolCreateRequest{
Kind: &kind,
Name: "test-name",
Expand Down Expand Up @@ -190,7 +194,7 @@ func TestGetNodePoolByClusterIdAndNodePoolId(t *testing.T) {
Expect(err).NotTo(HaveOccurred())

// Create a nodepool for this cluster using the API
kind := "NodePool"
kind := kindNodePool
nodePoolInput := openapi.NodePoolCreateRequest{
Kind: &kind,
Name: "test-np-get",
Expand Down Expand Up @@ -319,3 +323,48 @@ func TestNodePoolPost_WrongKind(t *testing.T) {
Expect(ok).To(BeTrue())
Expect(detail).To(ContainSubstring("kind must be 'NodePool'"))
}

// TestNodePoolDuplicateNames tests that duplicate nodepool names within a cluster are rejected
func TestNodePoolDuplicateNames(t *testing.T) {
h, client := test.RegisterIntegration(t)

account := h.NewRandAccount()
ctx := h.NewAuthenticatedContext(account)

// Create a cluster first
cluster, err := h.Factories.NewClusters(h.NewID())
Expect(err).NotTo(HaveOccurred())

// Create first nodepool with a specific name
kind := kindNodePool
nodePoolInput := openapi.NodePoolCreateRequest{
Kind: &kind,
Name: "test-duplicate",
Spec: map[string]interface{}{"test": "spec"},
}

resp, err := client.CreateNodePoolWithResponse(
ctx, cluster.ID, openapi.CreateNodePoolJSONRequestBody(nodePoolInput), test.WithAuthToken(ctx),
)
Expect(err).NotTo(HaveOccurred())
Expect(resp.StatusCode()).To(Equal(http.StatusCreated))

// Create second nodepool with the same name in the same cluster
resp, err = client.CreateNodePoolWithResponse(
ctx, cluster.ID, openapi.CreateNodePoolJSONRequestBody(nodePoolInput), test.WithAuthToken(ctx),
)
Expect(err).NotTo(HaveOccurred())
Expect(resp.StatusCode()).
Copy link
Contributor

@rafabene rafabene Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider also asserting the response body for the 409 case, not just the status code. Per the
HyperFleet error model standard (RFC 9457 Problem Details), the response should include code:
"HYPERFLEET-CNF-001", a meaningful detail message, and the standard type/title fields.
Validating the body ensures the error message is user-friendly and prevents regressions in the
error format.

For example:

  // After asserting StatusCode == 409:
  Expect(resp.JSON409).NotTo(BeNil())
  // or parse the body and check:
  // Expect(problemDetail.Code).To(Equal("HYPERFLEET-CNF-001"))
  // Expect(problemDetail.Detail).To(ContainSubstring("already exists"))

Copy link
Contributor Author

@Mischulee Mischulee Feb 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assertions to validate the error response body added.

To(Equal(http.StatusConflict), "Expected 409 Conflict for duplicate nodepool name in same cluster")

Expect(resp.ApplicationproblemJSONDefault).NotTo(BeNil(), "Expected response body to be present")
problemDetail := resp.ApplicationproblemJSONDefault

Expect(*problemDetail.Code).To(Equal("HYPERFLEET-CNF-001"), "Expected conflict error code")
Expect(problemDetail.Type).To(Equal("https://api.hyperfleet.io/errors/conflict"), "Expected conflict error type")
Expect(problemDetail.Title).To(Equal("Resource Conflict"), "Expected conflict error title")

Expect(problemDetail.Detail).NotTo(BeNil(), "Expected error detail to be present")
Expect(*problemDetail.Detail).To(ContainSubstring("already exists"),
"Expected error detail to mention that resource already exists")
}