HYPERFLEET-619 - fix: Prevent duplicate nodepool names within a cluster#53
HYPERFLEET-619 - fix: Prevent duplicate nodepool names within a cluster#53Mischulee wants to merge 1 commit intoopenshift-hyperfleet:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThis pull request adds validation to prevent duplicate node pool names within a cluster. The changes include adding a unique database constraint on Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/api/node_pool_types.go`:
- Line 17: Remove the redundant GORM uniqueIndex tags from the NodePool struct:
delete `uniqueIndex:idx_owner_name` from the Name field's struct tag and from
the OwnerID field's struct tag; retain the `index` tag on OwnerID if
single-column indexing is desired, since the canonical unique constraint is
defined in the migration `202602111400_add_nodepool_unique_name_constraint.go`
and migrations are the source of truth.
🧹 Nitpick comments (2)
test/integration/node_pools_test.go (1)
326-359: Good test coverage for the duplicate name conflict scenario.The test correctly verifies that duplicate names within the same cluster return a 409 Conflict. Consider adding additional test cases to ensure complete coverage of the uniqueness constraint:
- Cross-cluster duplicate names should succeed - Verify that the same name can be used in different clusters
- Name reuse after soft-delete - Verify that deleting a nodepool allows its name to be reused (tests the
WHERE deleted_at IS NULLpartial index)These would validate both the positive and edge-case behaviors of the constraint.
pkg/db/migrations/202602111400_add_nodepool_unique_name_constraint.go (1)
8-28: Consider pre-existing duplicate data before migration.If the database already contains duplicate (owner_id, name) pairs for active nodepools, this migration will fail. Consider adding a pre-migration check or documenting the remediation steps.
💡 Suggested improvement: Add a pre-check query
func addNodePoolUniqueNameConstraint() *gormigrate.Migration { return &gormigrate.Migration{ ID: "202602111400", Migrate: func(tx *gorm.DB) error { + // Check for existing duplicates before creating the index + var count int64 + checkSQL := `SELECT COUNT(*) FROM ( + SELECT owner_id, name FROM node_pools + WHERE deleted_at IS NULL + GROUP BY owner_id, name HAVING COUNT(*) > 1 + ) AS duplicates` + if err := tx.Raw(checkSQL).Scan(&count).Error; err != nil { + return err + } + if count > 0 { + return fmt.Errorf("cannot create unique constraint: %d duplicate (owner_id, name) pairs exist", count) + } + // Create unique index on (owner_id, name) to prevent duplicate nodepool names within a cluster createIndexSQL := "CREATE UNIQUE INDEX IF NOT EXISTS idx_node_pools_owner_name " + "ON node_pools(owner_id, name) WHERE deleted_at IS NULL;"Note: You would also need to add
"fmt"to the imports.
| @@ -0,0 +1,29 @@ | |||
| package migrations | |||
There was a problem hiding this comment.
Can we add this code to the existing add_node_pools file?
The reason being that we don't need to keep compatibility with older database schema yet, so it will be better to have less migration files at the begining.
There was a problem hiding this comment.
+1. As we have not yet exposed/released our framework, we are still at the first version, so we do not need the extra migration file, but instead, integrating in the current one is a better choice.
There was a problem hiding this comment.
Makes sense, fixed.
| @@ -0,0 +1,29 @@ | |||
| package migrations | |||
There was a problem hiding this comment.
+1. As we have not yet exposed/released our framework, we are still at the first version, so we do not need the extra migration file, but instead, integrating in the current one is a better choice.
pkg/api/node_pool_types.go
Outdated
|
|
||
| // Owner references (expanded) | ||
| OwnerID string `json:"owner_id" gorm:"size:255;not null;index"` | ||
| OwnerID string `json:"owner_id" gorm:"size:255;not null;index;uniqueIndex:idx_owner_name"` |
| ctx, cluster.ID, openapi.CreateNodePoolJSONRequestBody(nodePoolInput), test.WithAuthToken(ctx), | ||
| ) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(resp.StatusCode()). |
There was a problem hiding this comment.
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"))There was a problem hiding this comment.
Assertions to validate the error response body added.
992e80c to
5505cb7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/db/migrations/202511111055_add_node_pools.go`:
- Around line 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.
| // 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 | ||
| } |
There was a problem hiding this comment.
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.
| // 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.
| $ref: '#/components/schemas/NodePoolCreateResponse' | ||
| '400': | ||
| description: The server could not understand the request due to invalid syntax. | ||
| '409': |
There was a problem hiding this comment.
This openapi.yaml is a copy from the one we generate with the https://github.com/openshift-hyperfleet/hyperfleet-api-spec repository.
This means that if we don't make this change to that repository, the change will be lost the next time we update the api-spec and copy over this repository.
If we want this 409 to follow the schema, it may be a good opportunity to go to the api-spec and review the 409 responses in other cases, since improving the error handling in the schema is still something we have to work on.
So, I think we should either:
- Keep this change but also change the api-spec
- Discard this change here, and open a ticket to review the api-spec error schemas
https://issues.redhat.com/browse/HYPERFLEET-619
Summary by CodeRabbit
Release Notes