HYPERFLEET-1159 - feat: migrate cluster and nodepool to generic resource model#291
Conversation
…rce model Removes dedicated Cluster and NodePool types, DAOs, services, handlers, and plugins — all CRUD and status operations now go through the unified Resource pipeline. Adds resource labels support and resolves conflicts with HYPERFLEET-1154 (status handlers, force-delete adapter cleanup, hasActiveChildren helper) and HYPERFLEET-1157 (root endpoints, flat child routes).
|
[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 |
📝 WalkthroughWalkthroughThe change removes typed Cluster and NodePool runtime paths and registers both as generic entities. Configuration and schema validation now carry adapter, relationship, schema, search, and name-length metadata. Resource labels move from JSONB storage to a Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ResourceHandler
participant ResourceService
participant ResourceLabelDao
participant Database
Client->>ResourceHandler: create resource with labels
ResourceHandler->>ResourceService: validate and create resource
ResourceService->>Database: insert Resource
ResourceService->>ResourceLabelDao: ReplaceLabels
ResourceLabelDao->>Database: replace resource_labels rows
ResourceService-->>ResourceHandler: return resource
ResourceHandler-->>Client: return resource response
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
|
Risk Score: 5 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 11355 lines (>500) | +2 |
| Sensitive paths | cmd/ | +2 |
| Test coverage | Missing tests for: cmd/hyperfleet-api pkg/dao pkg/dao/mocks pkg/db pkg/db/migrations plugins/clusters plugins/entities plugins/nodePools plugins/resources test/factories | +1 |
Computed by hyperfleet-risk-scorer
|
@kuudori: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
charts/templates/configmap.yaml (1)
138-172: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winBump the Helm chart version for this rendered-config change.
This changes runtime configuration and entity schema output. No
Chart.yamlupdate is included, so a packaged chart can retain the old version while rendering different behavior. Bump the chart version and add the required changelog entry.As per path instructions, chart changes require a
Chart.yamlversion bump; the referenced Helm conventions also require an[Unreleased]changelog entry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/templates/configmap.yaml` around lines 138 - 172, Update Chart.yaml with an incremented chart version for the rendered configuration change, and add a corresponding [Unreleased] entry to the chart changelog describing the entity configuration/schema output update. Ensure both files are included with the template change.Source: Path instructions
🧹 Nitpick comments (5)
pkg/services/resource_test.go (2)
182-187: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMirror production label persistence in the mock.
The production DAO copies labels and sets
ResourceIDbefore inserting them. This mock stores the caller-owned slice unchanged, so labels can retain an emptyResourceIDand later mutations can alter the simulated persisted state.Proposed fix
- d.labels[resourceID] = labels + stored := make([]api.ResourceLabel, len(labels)) + copy(stored, labels) + for i := range stored { + stored[i].ResourceID = resourceID + } + d.labels[resourceID] = stored🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/resource_test.go` around lines 182 - 187, Update mockResourceLabelDao.ReplaceLabels to mirror production persistence: create a copied labels slice, set each copied label’s ResourceID to resourceID, and store the copy in d.labels instead of retaining the caller-owned slice.
236-248: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExpose the label DAO to test its failure path.
Both service fixtures discard the new
mockResourceLabelDao, so tests using them cannot setreplaceErr. The new label persistence path inCreateandPatchtherefore lacks fixture-level coverage for DAO error propagation. Return the label DAO from the fixture or add a dedicated failure fixture.As per path instructions, error paths should be tested, not just happy paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/resource_test.go` around lines 236 - 248, The test fixtures discard the label DAO, preventing failure-path tests for label persistence in ResourceService.Create and Patch. Update newTestResourceService and newTestResourceServiceWithAdapterStatus to retain and return the mockResourceLabelDao, or add a dedicated fixture exposing it, then use its replaceErr field in tests to verify DAO errors propagate.Source: Path instructions
pkg/api/resource_label.go (1)
10-14: 🚀 Performance & Scalability | 🔵 TrivialNo index for (key, value) lookup pattern.
resource_labelsPK is(resource_id, key), which doesn't help queries filtering bykey/valueindependent ofresource_id— exactly the access pattern used bylabels.<key> = '<value>'search. At scale this forces a full-table scan ofresource_labels. Consider a supporting index (e.g.,(key, value)).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/resource_label.go` around lines 10 - 14, Add a database index covering the key/value lookup pattern to the ResourceLabel model, using GORM index tags on Key and Value (or an equivalent composite index), while preserving the existing composite primary key and constraints.test/integration/resource_delete_test.go (1)
173-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest duplicates SQL instead of exercising the real DAO function.
TestExistsSoftDeletedByOwnerruns a hand-writtenSELECT EXISTS(...)query viadbSession.Raw(...)rather than calling the actualExistsSoftDeletedByOwnerDAO method the comment references. If the real implementation's WHERE clause changes, this test won't catch the drift. Call the production function directly (or move this into apkg/daounit/integration test that exercises it), and use the scenario-based naming convention rather than mirroring the internal function name.♻️ Suggested direction
// instead of re-deriving the SQL, invoke the DAO under test: exists, err := h.ResourceDao.ExistsSoftDeletedByOwner(t.Context(), []string{"Version"}, channel.ID) Expect(err).To(BeNil()) Expect(exists).To(BeTrue())As per path instructions for
**/*_test.go: "Test names describe the scenario, not the implementation."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/integration/resource_delete_test.go` around lines 173 - 220, Update TestExistsSoftDeletedByOwner to exercise h.ResourceDao.ExistsSoftDeletedByOwner directly instead of duplicating its SQL through dbSession.Raw; retain the positive multi-kind and negative-owner scenarios while asserting the returned value and error. Rename the test and subtest to describe the soft-deleted child detection scenario rather than the DAO method name.Source: Path instructions
pkg/handlers/resource_handler_test.go (1)
490-512: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the removed parent-not-found error path for
ListByOwner.The
"Parent not found"case was deleted, leaving only the happy path.ListByOwnerstill short-circuits on the parentGet(404), and that branch is now untested.As per path instructions (testing.md TEST-02): "Error paths SHOULD be tested, not just happy paths."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/handlers/resource_handler_test.go` around lines 490 - 512, Add a “Parent not found” table-driven test case alongside the existing success case in the ListByOwner handler tests, configuring the mock Get call for the parent resource to return a 404/not-found error and asserting the handler’s expected error status and response. Keep the existing ListByOwner expectation out of this case because execution should short-circuit after the failed parent lookup.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/api/presenters/resource.go`:
- Around line 128-140: convertLabelsToModel accepts an unbounded number of
labels and allocates the result slice before enforcing a limit. Reuse the
proposed maximum-labels constant from the resource-label validation logic, check
len(*labels) against it before allocation or iteration, and return the same
validation error used by resource_label.go when the limit is exceeded.
In `@pkg/api/resource_label.go`:
- Around line 20-31: Add a MaxLabelsPerResource limit and enforce it before
processing labels: update the request validation/conversion path, including
convertLabelsToModel, to reject maps exceeding the limit, and ensure
ReplaceLabels does not process unbounded entries. Reuse the limit consistently
across ValidateLabel-related validation and the resource presenter/DAO methods.
In `@pkg/db/migrations/202607010001_add_resource_labels.go`:
- Around line 12-24: The migration drops existing labels without preserving
them. In the migration function, backfill resource_labels from resources.labels
before ALTER TABLE removes the column, parsing each JSONB object into
resource_id, key, and value rows while handling empty or null labels safely; add
a corresponding down migration that recreates labels and restores data from
resource_labels if rollback is supported.
In `@pkg/db/migrations/migration_structs.go`:
- Around line 28-36: Update addResourceLabels() to backfill existing values from
resources.labels into resource_labels before dropping the column, using an
appropriate migration-safe data copy and preserving rollback behavior; only
execute the DROP COLUMN after the backfill completes successfully.
In `@pkg/handlers/name_validation_handler_test.go`:
- Around line 21-28: Update newNameValidationHandler to accept *testing.T, call
t.Helper() at the start, and import testing if needed. Modify all four call
sites to pass their test instance so failures are attributed to the calling
test.
In `@pkg/handlers/validation.go`:
- Around line 121-129: Name validation currently allows unlimited lengths when
maxLen is zero, despite the database cap. Update the validation logic in the
relevant name-validation function to enforce a hard maximum matching
Resource.Name’s 100-character limit whenever no configured maxLen exists, or
ensure every runtime entity descriptor supplies name_max_len; return the
existing validation error for overlong names.
In `@test/factories/clusters.go`:
- Around line 177-186: Replace direct updates of the removed Resource “labels”
column in NewClusterWithLabels and NewClusterWithStatusAndLabels with
ResourceLabelDao.ReplaceLabels using the created resource, then reload it with
Preload("Labels") so returned clusters contain their persisted labels.
---
Outside diff comments:
In `@charts/templates/configmap.yaml`:
- Around line 138-172: Update Chart.yaml with an incremented chart version for
the rendered configuration change, and add a corresponding [Unreleased] entry to
the chart changelog describing the entity configuration/schema output update.
Ensure both files are included with the template change.
---
Nitpick comments:
In `@pkg/api/resource_label.go`:
- Around line 10-14: Add a database index covering the key/value lookup pattern
to the ResourceLabel model, using GORM index tags on Key and Value (or an
equivalent composite index), while preserving the existing composite primary key
and constraints.
In `@pkg/handlers/resource_handler_test.go`:
- Around line 490-512: Add a “Parent not found” table-driven test case alongside
the existing success case in the ListByOwner handler tests, configuring the mock
Get call for the parent resource to return a 404/not-found error and asserting
the handler’s expected error status and response. Keep the existing ListByOwner
expectation out of this case because execution should short-circuit after the
failed parent lookup.
In `@pkg/services/resource_test.go`:
- Around line 182-187: Update mockResourceLabelDao.ReplaceLabels to mirror
production persistence: create a copied labels slice, set each copied label’s
ResourceID to resourceID, and store the copy in d.labels instead of retaining
the caller-owned slice.
- Around line 236-248: The test fixtures discard the label DAO, preventing
failure-path tests for label persistence in ResourceService.Create and Patch.
Update newTestResourceService and newTestResourceServiceWithAdapterStatus to
retain and return the mockResourceLabelDao, or add a dedicated fixture exposing
it, then use its replaceErr field in tests to verify DAO errors propagate.
In `@test/integration/resource_delete_test.go`:
- Around line 173-220: Update TestExistsSoftDeletedByOwner to exercise
h.ResourceDao.ExistsSoftDeletedByOwner directly instead of duplicating its SQL
through dbSession.Raw; retain the positive multi-kind and negative-owner
scenarios while asserting the returned value and error. Rename the test and
subtest to describe the soft-deleted child detection scenario rather than the
DAO method name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 751c12da-ea54-4dea-9986-ad41b5afe8b6
📒 Files selected for processing (84)
charts/templates/configmap.yamlcharts/values.yamlcmd/hyperfleet-api/environments/framework_test.gocmd/hyperfleet-api/main.goconfigs/config.yaml.examplepkg/api/cluster_types.gopkg/api/cluster_types_test.gopkg/api/node_pool_types.gopkg/api/node_pool_types_test.gopkg/api/presenters/cluster.gopkg/api/presenters/cluster_test.gopkg/api/presenters/node_pool.gopkg/api/presenters/node_pool_test.gopkg/api/presenters/presenter_test.gopkg/api/presenters/resource.gopkg/api/presenters/resource_test.gopkg/api/resource.gopkg/api/resource_label.gopkg/config/adapter.gopkg/config/adapter_test.gopkg/config/config.gopkg/config/dump.gopkg/config/loader.gopkg/config/loader_test.gopkg/dao/cluster.gopkg/dao/mocks/cluster.gopkg/dao/mocks/node_pool.gopkg/dao/node_pool.gopkg/dao/resource.gopkg/dao/resource_label.gopkg/db/migrations/202511111044_add_clusters.gopkg/db/migrations/202511111055_add_node_pools.gopkg/db/migrations/202601210001_add_conditions_gin_index.gopkg/db/migrations/202604160001_soft_delete_schema.gopkg/db/migrations/202604230001_add_nodepool_owner_deleted_index.gopkg/db/migrations/202605280001_remove_ready_condition.gopkg/db/migrations/202607010001_add_resource_labels.gopkg/db/migrations/migration_structs.gopkg/db/sql_helpers.gopkg/handlers/cluster.gopkg/handlers/cluster_nodepools.gopkg/handlers/cluster_nodepools_test.gopkg/handlers/cluster_status.gopkg/handlers/cluster_test.gopkg/handlers/name_validation_handler_test.gopkg/handlers/node_pool.gopkg/handlers/node_pool_test.gopkg/handlers/nodepool_status.gopkg/handlers/resource_handler.gopkg/handlers/resource_handler_test.gopkg/handlers/validation.gopkg/middleware/schema_validation.gopkg/middleware/schema_validation_test.gopkg/registry/descriptor.gopkg/registry/name_validation_test.gopkg/registry/registry.gopkg/services/adapter_status_validation.gopkg/services/cluster.gopkg/services/cluster_test.gopkg/services/generic.gopkg/services/generic_test.gopkg/services/node_pool.gopkg/services/node_pool_test.gopkg/services/resource.gopkg/services/resource_test.gopkg/services/status_helpers.gopkg/services/test_helpers_test.gopkg/services/util.gopkg/validators/schema_validator.gopkg/validators/schema_validator_test.goplugins/clusters/plugin.goplugins/entities/plugin.goplugins/nodePools/plugin.goplugins/resources/plugin.gotest/factories/clusters.gotest/factories/node_pools.gotest/integration/adapter_status_test.gotest/integration/channels_test.gotest/integration/clusters_test.gotest/integration/node_pools_test.gotest/integration/resource_delete_test.gotest/integration/resource_helpers.gotest/integration/transaction_test.gotest/integration/versions_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (41)
- pkg/api/presenters/node_pool.go
- pkg/dao/mocks/node_pool.go
- pkg/db/migrations/202605280001_remove_ready_condition.go
- pkg/handlers/cluster_nodepools.go
- pkg/db/migrations/202604230001_add_nodepool_owner_deleted_index.go
- pkg/config/adapter.go
- plugins/clusters/plugin.go
- pkg/api/presenters/cluster.go
- pkg/dao/cluster.go
- pkg/db/migrations/202511111044_add_clusters.go
- plugins/nodePools/plugin.go
- pkg/handlers/node_pool.go
- cmd/hyperfleet-api/main.go
- pkg/dao/mocks/cluster.go
- pkg/config/dump.go
- pkg/handlers/node_pool_test.go
- pkg/handlers/cluster_nodepools_test.go
- pkg/services/node_pool.go
- pkg/handlers/cluster_status.go
- pkg/db/migrations/202601210001_add_conditions_gin_index.go
- pkg/api/presenters/node_pool_test.go
- pkg/dao/node_pool.go
- pkg/api/presenters/cluster_test.go
- pkg/api/node_pool_types.go
- pkg/api/cluster_types.go
- pkg/services/cluster.go
- pkg/handlers/cluster_test.go
- pkg/db/migrations/202511111055_add_node_pools.go
- pkg/api/cluster_types_test.go
- pkg/config/adapter_test.go
- pkg/handlers/cluster.go
- pkg/handlers/nodepool_status.go
- pkg/api/node_pool_types_test.go
- cmd/hyperfleet-api/environments/framework_test.go
- pkg/db/migrations/202604160001_soft_delete_schema.go
- pkg/config/loader_test.go
- pkg/services/node_pool_test.go
- pkg/config/config.go
- pkg/config/loader.go
- pkg/services/cluster_test.go
- pkg/services/status_helpers.go
| func convertLabelsToModel(labels *map[string]string) ([]api.ResourceLabel, error) { | ||
| if labels == nil || len(*labels) == 0 { | ||
| return nil, nil | ||
| } | ||
| b, err := json.Marshal(*labels) | ||
| if err != nil { | ||
| return nil, err | ||
| result := make([]api.ResourceLabel, 0, len(*labels)) | ||
| for k, v := range *labels { | ||
| if err := api.ValidateLabel(k, v); err != nil { | ||
| return nil, err | ||
| } | ||
| result = append(result, api.ResourceLabel{Key: k, Value: v}) | ||
| } | ||
| return datatypes.JSON(b), nil | ||
| return result, nil | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Same unbounded-count concern as pkg/api/resource_label.go — no cap enforced here either.
convertLabelsToModel validates each label via ValidateLabel but places no ceiling on len(*labels) before allocating and returning the full slice. See the primary comment on pkg/api/resource_label.go for the proposed constant/guard; this is the call site where it should be enforced.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/api/presenters/resource.go` around lines 128 - 140, convertLabelsToModel
accepts an unbounded number of labels and allocates the result slice before
enforcing a limit. Reuse the proposed maximum-labels constant from the
resource-label validation logic, check len(*labels) against it before allocation
or iteration, and return the same validation error used by resource_label.go
when the limit is exceeded.
| func ValidateLabel(key, value string) error { | ||
| if key == "" { | ||
| return fmt.Errorf("label key cannot be empty") | ||
| } | ||
| if len(key) > MaxLabelKeyLen { | ||
| return fmt.Errorf("label key %q exceeds maximum length of %d", key, MaxLabelKeyLen) | ||
| } | ||
| if len(value) > MaxLabelValueLen { | ||
| return fmt.Errorf("label value for key %q exceeds maximum length of %d", key, MaxLabelValueLen) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm no existing count enforcement in handler validation layer.
rg -n -A10 'func validateLabels' pkg/handlers/validation.goRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 522
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== resource_label.go ==\n'
cat -n pkg/api/resource_label.go | sed -n '1,220p'
printf '\n== presenters/resource.go ==\n'
cat -n pkg/api/presenters/resource.go | sed -n '1,260p'
printf '\n== label-related usage ==\n'
rg -n 'ReplaceLabels|convertLabelsToModel|ValidateLabel|validateLabels|resource_labels|labels\.' pkg | sed -n '1,220p'Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 11827
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'MaxLabelsPerResource|label count|labels per resource|label.*count|count.*label' .Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 399
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'ReplaceLabels|DeleteLabels|CreateLabels|labels.*insert|labels.*delete|ON CONFLICT|DELETE FROM .*labels|INSERT INTO .*labels' pkg testRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 2096
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== presenters/resource.go (relevant sections) ==\n'
awk 'NR>=1 && NR<=260 {print NR ":" $0}' pkg/api/presenters/resource.go | sed -n '1,260p'
printf '\n== label validation helpers ==\n'
awk 'NR>=1 && NR<=260 {print NR ":" $0}' pkg/handlers/validation.go | sed -n '180,260p'
printf '\n== any schema limits on labels ==\n'
rg -n 'max.*label|label.*max|additionalProperties|map.*string.*string|resource_labels' openapi pkg config.yamlRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 18588
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== pkg/dao/resource_label.go ==\n'
cat -n pkg/dao/resource_label.go | sed -n '1,240p'
printf '\n== pkg/services/resource.go (label writes) ==\n'
cat -n pkg/services/resource.go | sed -n '90,170p'
printf '\n== label request schema ==\n'
rg -n 'labels.*map|string.*string|maxProperties|additionalProperties|ResourceCreateRequest|ResourcePatchRequest' pkg/api/openapi pkg/api -g'*.go' | sed -n '1,240p'Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 7668
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'type ResourceCreateRequest|type ResourcePatchRequest|Labels.*map\[string\]string|json:"labels"' pkg/api/openapi pkg/api -g'*.go' | sed -n '1,240p'Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 448
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' pkg/dao/resource_label.goRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 1396
Cap labels per resource to avoid CWE-400 DoS. ValidateLabel only checks key/value length; convertLabelsToModel still accepts an unbounded map, and ReplaceLabels deletes/inserts every entry. Add a MaxLabelsPerResource guard in the request validation/conversion path.
pkg/api/resource_label.go:20-31, pkg/api/presenters/resource.go:128-139, pkg/dao/resource_label.go:26-48
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/api/resource_label.go` around lines 20 - 31, Add a MaxLabelsPerResource
limit and enforce it before processing labels: update the request
validation/conversion path, including convertLabelsToModel, to reject maps
exceeding the limit, and ensure ReplaceLabels does not process unbounded
entries. Reuse the limit consistently across ValidateLabel-related validation
and the resource presenter/DAO methods.
| if err := tx.Exec(`CREATE TABLE IF NOT EXISTS resource_labels ( | ||
| resource_id VARCHAR(255) NOT NULL, | ||
| key VARCHAR(255) NOT NULL, | ||
| value VARCHAR(255) NOT NULL, | ||
| PRIMARY KEY (resource_id, key), | ||
| FOREIGN KEY (resource_id) REFERENCES resources(id) ON DELETE CASCADE | ||
| );`).Error; err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := tx.Exec(`ALTER TABLE resources DROP COLUMN IF EXISTS labels;`).Error; err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm prior labels usage and whether any backfill exists in the migration set.
rg -nP 'labels' pkg/db/migrations -g '!*_test.go' -C2
rg -nP 'resource_labels' pkg/db/migrations -C2Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 1995
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== migration file =="
cat -n pkg/db/migrations/202607010001_add_resource_labels.go
echo
echo "== search for any backfill / insert into resource_labels / rollback in migrations =="
rg -n "resource_labels|backfill|Rollback|DROP COLUMN IF EXISTS labels|INSERT INTO resource_labels|jsonb_each|labels" pkg/db/migrations -g '!*_test.go' -C 3
echo
echo "== search for runtime use of resource_labels and labels column =="
rg -n "resource_labels|\\.Labels|labels\\]" pkg -g '!pkg/api/openapi/**' -C 2Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 14494
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "Rollback:" pkg/db/migrations -g '!*_test.go' -C 2Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 173
pkg/db/migrations/202607010001_add_resource_labels.go:12-24 — Backfill resource_labels before dropping resources.labels
ALTER TABLE resources DROP COLUMN IF EXISTS labels deletes existing JSONB label data with no copy into resource_labels, so any resource already carrying labels loses them permanently. Add a backfill step before the drop, and include a down migration if reversal is needed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/db/migrations/202607010001_add_resource_labels.go` around lines 12 - 24,
The migration drops existing labels without preserving them. In the migration
function, backfill resource_labels from resources.labels before ALTER TABLE
removes the column, parsing each JSONB object into resource_id, key, and value
rows while handling empty or null labels safely; add a corresponding down
migration that recreates labels and restores data from resource_labels if
rollback is supported.
Source: Path instructions
| var MigrationList = []*gormigrate.Migration{ | ||
| // addEvents(), // REMOVED: Events table no longer used - no event-driven components | ||
| addClusters(), | ||
| addNodePools(), | ||
| addAdapterStatus(), | ||
| addConditionsGinIndex(), | ||
| addSoftDeleteSchema(), | ||
| addReconciledIndex(), | ||
| addNodePoolOwnerDeletedIndex(), | ||
| addDeletedTimeIndexes(), | ||
| addResources(), | ||
| removeReadyCondition(), | ||
| addResourceConditions(), | ||
| addResourceLabels(), | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm what the removed migrations did and whether resources migrations cover them.
fd -e go . pkg/db/migrations --exec grep -l . {} \; | sort
rg -nP 'func (addResources|addResourceConditions|addResourceLabels|addClusters|addNodePools|removeReadyCondition)\s*\(' pkg/db/migrations -C2
# Look for any DROP TABLE / data backfill of legacy typed tables.
rg -nP '\b(DROP TABLE|clusters|node_pools)\b' pkg/db/migrations -C2Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 5102
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== migration_structs.go =="
cat -n pkg/db/migrations/migration_structs.go
echo
echo "== resource migrations =="
for f in pkg/db/migrations/202605202128_add_resources.go \
pkg/db/migrations/202606290001_add_resource_conditions.go \
pkg/db/migrations/202607010001_add_resource_labels.go; do
echo "--- $f ---"
cat -n "$f"
echo
done
echo "== legacy typed migrations =="
for f in pkg/db/migrations/*clusters*go pkg/db/migrations/*nodepool*go pkg/db/migrations/*ready*go; do
[ -e "$f" ] || continue
echo "--- $f ---"
cat -n "$f"
echo
doneRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 6718
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== resource model / persistence references =="
rg -n --hidden -S 'resources\.labels|resource_labels|Labels\b|label' pkg test cmd plugins | head -n 300
echo
echo "== migration backfill/drop paths =="
rg -n --hidden -S 'DROP COLUMN IF EXISTS labels|INSERT INTO resource_labels|UPDATE resources SET labels|jsonb|resource_labels' pkg/db/migrations -C 3
echo
echo "== current resource schema readers/writers =="
rg -n --hidden -S 'resource_labels|labels' pkg/dao pkg/services pkg/handlers pkg/api/openapi -C 2 | head -n 300Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 47409
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== pkg/api/resource.go =="
cat -n pkg/api/resource.go
echo
echo "== package-wide writers to resources.labels column =="
rg -n --hidden -S 'Update\("labels"|Select\("labels"|Column\("labels"|labels\s*[:=]|DROP COLUMN IF EXISTS labels' pkg test -C 2Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 16001
Backfill resources.labels before dropping it. addResourceLabels() creates resource_labels and then runs ALTER TABLE resources DROP COLUMN IF EXISTS labels;, which discards any labels already stored in the old column with no backfill or rollback path (CWE-664). Copy existing rows first or keep the column until the data move is complete.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/db/migrations/migration_structs.go` around lines 28 - 36, Update
addResourceLabels() to backfill existing values from resources.labels into
resource_labels before dropping the column, using an appropriate migration-safe
data copy and preserving rollback behavior; only execute the DROP COLUMN after
the backfill completes successfully.
Source: Path instructions
| func newNameValidationHandler( | ||
| ctrl *gomock.Controller, | ||
| descriptor registry.EntityDescriptor, | ||
| ) (*ResourceHandler, *services.MockResourceService) { | ||
| mockResourceSvc := services.NewMockResourceService(ctrl) | ||
| handler := NewResourceHandler(descriptor, mockResourceSvc) | ||
| return handler, mockResourceSvc | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Mark newNameValidationHandler as a test helper.
Pass *testing.T into this helper, call t.Helper(), and update its four call sites. Otherwise failures inside the helper are reported against helper internals instead of the test scenario.
As per path instructions, **/*_test.go requires t.Helper() in test helper functions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/handlers/name_validation_handler_test.go` around lines 21 - 28, Update
newNameValidationHandler to accept *testing.T, call t.Helper() at the start, and
import testing if needed. Modify all four call sites to pass their test instance
so failures are attributed to the calling test.
Source: Path instructions
| // Check minimum length (0 = no constraint) | ||
| if minLen > 0 && len(name) < minLen { | ||
| return errors.Validation("%s must be at least %d characters", field, minLen) | ||
| } | ||
|
|
||
| // Check maximum length | ||
| if len(name) > maxLen { | ||
| // Check maximum length (0 = no constraint) | ||
| if maxLen > 0 && len(name) > maxLen { | ||
| return errors.Validation("%s must be at most %d characters", field, maxLen) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Do any descriptors omit NameMaxLen (defaults to 0 => unbounded)?
rg -nP 'NameMaxLen|NameMinLen' pkg/registry configs charts -C2
rg -nP 'Name\s+string.*size:100' pkg/api/resource.goRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 3067
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== validation.go =="
sed -n '1,220p' pkg/handlers/validation.go
echo
echo "== registry descriptor definition =="
sed -n '1,140p' pkg/registry/descriptor.go
echo
echo "== registry validation =="
sed -n '90,190p' pkg/registry/registry.go
echo
echo "== all descriptor registrations with name lengths =="
rg -n 'Register\(|NameMinLen|NameMaxLen' pkg plugins configs -C 2
echo
echo "== resource name column =="
sed -n '1,80p' pkg/api/resource.goRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 46080
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== all occurrences of name length fields =="
rg -n --hidden --glob '!**/*_test.go' --glob '!**/vendor/**' 'name_max_len|NameMaxLen|name_min_len|NameMinLen' .
echo
echo "== config files with entity descriptors =="
fd -HI 'config.yaml|config.yml|*.yaml|*.yml' . | sed -n '1,120p'
echo
echo "== likely runtime descriptor definitions =="
rg -n --hidden --glob '!**/*_test.go' --glob '!**/vendor/**' 'kind:|plural:|name_max_len:|name_min_len:' config* .claude .github plugins pkg openapiRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 1962
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== charts/templates/configmap.yaml around name length =="
sed -n '156,172p' charts/templates/configmap.yaml
echo
echo "== charts/values.yaml around entity defaults =="
sed -n '196,220p' charts/values.yaml
echo
echo "== configs/config.yaml.example around entity defaults =="
sed -n '104,130p' configs/config.yaml.exampleRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 2328
Cap name validation at the DB limit maxLen == 0 leaves runtime entity names unbounded at the API boundary (CWE-20). Resource.Name is still gorm:"size:100", and the shipped entity config leaves name_max_len unset for Channel, so overlong names will pass here and fail later as a 500 instead of a 400. Enforce a hard cap here or require every runtime descriptor to set name_max_len.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/handlers/validation.go` around lines 121 - 129, Name validation currently
allows unlimited lengths when maxLen is zero, despite the database cap. Update
the validation logic in the relevant name-validation function to enforce a hard
maximum matching Resource.Name’s 100-character limit whenever no configured
maxLen exists, or ensure every runtime entity descriptor supplies name_max_len;
return the existing validation error for overlong names.
| dbSession := dbFactory.New(context.Background()) | ||
| err = dbSession.Model(cluster).Update("labels", labelsJSON).Error | ||
| if err != nil { | ||
| if err := dbSession.Model(cluster).Update("labels", labelsJSON).Error; err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Reload to get updated values | ||
| if err := reloadCluster(dbSession, cluster); err != nil { | ||
| if err := reloadResource(dbSession, cluster); err != nil { | ||
| return nil, err | ||
| } | ||
| return cluster, nil | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm resources table has no `labels` column and labels live in resource_labels.
rg -nP 'labels' pkg/api/resource.go pkg/api/resource_label.go
rg -nP 'AddColumn|labels|resource_labels' pkg/db/migrations -C2
rg -nP 'func .*ReplaceLabels' pkg/dao/resource_label.go -C2
# Any other Update("labels", ...) call sites?
rg -nP 'Update\("labels"' -C1Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 1707
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- target file ---'
cat -n test/factories/clusters.go | sed -n '150,260p'
echo '--- resource model / association ---'
cat -n pkg/api/resource.go | sed -n '1,220p'
echo '--- resource_label model ---'
cat -n pkg/api/resource_label.go | sed -n '1,120p'
echo '--- migration dropping labels ---'
cat -n pkg/db/migrations/202607010001_add_resource_labels.go | sed -n '1,160p'
echo '--- resource label DAO ---'
cat -n pkg/dao/resource_label.go | sed -n '1,220p'
echo '--- reloadResource definition/usages ---'
rg -n "func reloadResource|reloadResource\\(" -C 3 test/factoriesRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 13819
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('test/factories/clusters.go')
text = p.read_text()
for needle in ['Update("labels"', 'reloadResource(', 'ReplaceLabels', 'Preload("Labels")']:
print(f'--- {needle} ---')
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
start=max(1,i-8); end=min(len(text.splitlines()),i+12)
for j in range(start, end+1):
print(f"{j}:{text.splitlines()[j-1]}")
print()
PYRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 5520
Persist labels through resource_labels, not resources.labels
Resource no longer has a labels column; the migration drops it, while labels now live in the resource_labels association. Update("labels", ...) will fail in both NewClusterWithLabels and NewClusterWithStatusAndLabels. Use ResourceLabelDao.ReplaceLabels (or association writes) and reload with Preload("Labels") so the factory returns populated labels.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/factories/clusters.go` around lines 177 - 186, Replace direct updates of
the removed Resource “labels” column in NewClusterWithLabels and
NewClusterWithStatusAndLabels with ResourceLabelDao.ReplaceLabels using the
created resource, then reload it with Preload("Labels") so returned clusters
contain their persisted labels.
1 participant
Summary
ResourceLabelmodel,resource_labelsmigration,ResourceLabelDao)hasActiveChildrenhelper) and HYPERFLEET-1157 (root endpoints, flat child routes)Test plan
make test)make lint— 0 issues)go vet+gofmtpass (make verify)make test-integration)/resourcesand/channels//versionsendpoints