ROSAENG-61803: feat: preserve service-set fields on update - #276
Conversation
…and codegen pipeline New standalone module at api/public/v2alpha1/ with generated passthrough types (HostedClusterSpecPassthrough, NodePoolSpecPassthrough), envelope types (Cluster, NodePool), configuration mirror types, and per-field markers for write-mode, visibility, and feature gates. Adds platform-api codegen packages: field metadata registry (120 fields), feature gate registry (6 gates), and conversion helpers. Adds Makefile codegen pipeline (make codegen) and updates verify/deps targets. v1alpha1 internal CRD types are unchanged. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The marker-scanner produced flat registry keys (e.g. "pausedUntil") with no root-type namespace, so fields with the same JSON name in different passthrough types silently overwrote each other with non-deterministic results. Prefix passthrough fields with their root type context (spec.hostedCluster.* / spec.nodePool.*) to match the paths that downstream consumers already construct. Add a verify-codegen Makefile target that re-runs the full codegen pipeline and fails on git diff, same pattern as verify-clientset, so CI catches stale generated code. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@cdoan1: This pull request references ROSAENG-61803 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cdoan1 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift-online/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (31)
🚧 Files skipped from review as they are similar to previous changes (29)
WalkthroughThe PR adds public v2alpha1 cluster and node pool APIs, passthrough code generation, field metadata, feature gates, conversion helpers, and request validation. Cluster and node pool handlers now validate fields and preserve platform-managed values during updates. ChangesPublic API and generated metadata
Managed fields and request enforcement
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (11)
hack/api-codegen/pkg/markers/scanner_test.go (2)
124-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo test covers
verbose=true.All three call sites pass
falsefor the newverboseparameter. The verbose path inMarkerScanner.logfandScanhas no coverage, so a nil-writer or format-string defect in that path stays unnoticed.Add one case that constructs the scanner with
verbose=trueand asserts the registry output is identical.🤖 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 `@hack/api-codegen/pkg/markers/scanner_test.go` at line 124, Add a test case alongside the existing NewScanner coverage that constructs the scanner with verbose=true, captures its registry output, and asserts it matches the established expected output. Exercise the verbose path through MarkerScanner.Scan and logf without changing the existing false-verbose cases.
155-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBroaden the collision guard beyond
pausedUntil.The assertion covers only the flat
pausedUntilkey. The generated registry inplatform-api/internal/codegen/registry/field_metadata.gostill contains flat duplicates for other scanned types, for example bothmaxPodsandkubelet.maxPods. A narrow guard does not catch that class.Assert that every key produced from these two roots is prefixed. This makes the test fail on any new unprefixed key, not just this one name.
♻️ Proposed assertion change
- // Verify no flat "pausedUntil" key exists (the old collision) - if _, found := scanner.Registry["pausedUntil"]; found { - t.Error("flat key \"pausedUntil\" should not exist; passthrough fields must be prefixed") - } + // Every passthrough field must be registered under a spec.* prefix. + for key := range scanner.Registry { + if !strings.HasPrefix(key, "spec.") { + t.Errorf("unprefixed registry key %q; passthrough fields must be prefixed", key) + } + }Add
"strings"to the import block.🤖 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 `@hack/api-codegen/pkg/markers/scanner_test.go` around lines 155 - 159, Broaden the collision check in the scanner test beyond the single "pausedUntil" key: iterate over every registry key produced from the two scanned roots and assert that each is prefixed, using strings.HasPrefix with the expected root prefixes. Add the strings import and retain the existing failure behavior for unprefixed keys.platform-api/internal/codegen/registry/field_metadata.go (1)
1-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep both registry outputs synchronized.
The generated Go and JSON registries under
hack/api-codegenandplatform-apidiffer.verify-codegenregenerates and checks only theplatform-apiregistry. Generate one shared registry or compare both outputs inverify-codegen.🤖 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 `@platform-api/internal/codegen/registry/field_metadata.go` around lines 1 - 22, Update verify-codegen and the registry generation flow around FieldRegistry so the hack/api-codegen and platform-api registry outputs remain synchronized. Either generate both from one shared registry source or make verify-codegen compare both generated outputs, including the Go and JSON representations, and fail when they differ.platform-api/internal/codegen/conversion/cluster.go (1)
34-40: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueEscape
clusterIDbefore joining the URL.
url.URL.JoinPath(clusterID)removes duplicate slashes but does not keep/or../insideclusterIDas one path segment. Useu.JoinPath(url.PathEscape(clusterID))and handle URL parse errors explicitly.🤖 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 `@platform-api/internal/codegen/conversion/cluster.go` around lines 34 - 40, Update RewriteCloudURLWithID to parse baseURL into a url.URL, explicitly handle any parse error, and construct the cloudUrl with u.JoinPath(url.PathEscape(clusterID)) so clusterID remains a single path segment. Preserve the existing nil-spec early return and assign the resulting URL string to spec["cloudUrl"].platform-api/pkg/validation/field_validator_test.go (2)
162-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for arrays and for unchanged platform-managed values.
TestFlattenToFieldPathsuses only nested objects, so it passes even thoughflattenMapnever descends into JSON arrays. Add a case with a list of objects to pin the intended behavior. Add a case in which an update resends an unchangedServiceSetvalue, becauseTestValidateUpdate_RejectsServiceSetFieldsonly exercises a changed value.💚 Proposed tests
func TestFlattenToFieldPaths_DescendsIntoArrays(t *testing.T) { spec := map[string]any{ "services": []any{ map[string]any{"service": "APIServer"}, }, } result := flattenToFieldPaths("spec", spec) if _, found := result["spec.services.service"]; !found { t.Error("expected list element fields to be flattened") } } func TestValidateUpdate_AllowsUnchangedServiceSetValue(t *testing.T) { v := newTestValidator(map[string]registry.FieldMeta{ "spec.accountId": {FieldPath: "spec.accountId", WriteMode: registry.ServiceSet}, }) existing := map[string]any{"accountId": "123"} updated := map[string]any{"accountId": "123"} if errs := v.ValidateUpdate(updated, existing, featuregate.Default); errs != nil { t.Errorf("expected no errors when platform-managed value is unchanged, got %v", errs) } }🤖 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 `@platform-api/pkg/validation/field_validator_test.go` around lines 162 - 185, Extend validation test coverage by adding an array-of-objects case to TestFlattenToFieldPaths that verifies nested list fields are flattened, and add a TestValidateUpdate_AllowsUnchangedServiceSetValue case using identical existing and updated platform-managed values. Keep the assertions focused on the expected flattened path and absence of validation errors.
87-89: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDo not index
errs[0]without a length check.Each test registers a single registry entry, so one error is expected today. If a case later produces zero errors,
errs[0]panics aftererrs == nilalready passed for a non-nil empty slice. Assert the length first, or search the slice for the expected field asTestValidateCreate_RejectsServiceSetFieldsdoes.Also applies to: 120-122
🤖 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 `@platform-api/pkg/validation/field_validator_test.go` around lines 87 - 89, The validation tests index errs without confirming an error exists, allowing a non-nil empty slice to panic. In the affected assertions and the corresponding check around TestValidateCreate_RejectsServiceSetFields, assert the expected error count before indexing, or search errs for the expected "spec.fips" field while preserving the existing validation expectations.platform-api/internal/codegen/featuregate/registry.go (1)
5-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueProtect the gate registry from accidental mutation.
HyperFleetFeatureGatesis an exported package-level map. Go maps are mutable, so any importing package can add or remove gates and change validation outcomes. Unexport the map and expose read-only accessors, or add a comment that states the map must never be modified after initialization.🤖 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 `@platform-api/internal/codegen/featuregate/registry.go` around lines 5 - 6, Protect the feature-gate registry declared as HyperFleetFeatureGates from external mutation by unexporting the map and providing read-only accessors for consumers, or explicitly documenting that it must not be modified after initialization if the exported map is retained. Ensure existing validation code uses the protected registry access path.hyperfleet-operator/api/v1alpha1/nodepool_types.go (1)
39-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNaming differs between the two specs for the same concept.
ClusterSpecusesInternalID/internalId.NodePoolSpecusesInternalPoolID/internalPoolId. Both describe "an internal platform identifier". Confirm this asymmetry is deliberate, because consumers and the generated field registry must handle two different key names for one concept.🤖 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 `@hyperfleet-operator/api/v1alpha1/nodepool_types.go` around lines 39 - 45, Align the node pool internal identifier naming with ClusterSpec by renaming NodePoolSpec’s InternalPoolID field and JSON key to InternalID and internalId, unless the API intentionally requires distinct concepts; update generated field-registry references and consumers consistently with the chosen contract.platform-api/pkg/handlers/cluster.go (1)
315-325: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
writeValidationErrorshelper in both handlers. The two functions have identical bodies and differ only in the error code string. Extract one shared helper that takes the code as a parameter, so the 422 response shape stays consistent as it evolves.
platform-api/pkg/handlers/cluster.go#L315-L325: replace the local helper with a call to the shared helper, passing"CLUSTERS-MGMT-VALIDATION-001".platform-api/pkg/handlers/nodepool.go#L272-L282: replace the local helper with a call to the shared helper, passing"NODEPOOLS-MGMT-VALIDATION-001".🤖 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 `@platform-api/pkg/handlers/cluster.go` around lines 315 - 325, Extract the duplicated validation-response logic into one shared helper accepting the HTTP writer, validation errors, and error code. In platform-api/pkg/handlers/cluster.go lines 315-325, remove the local writeValidationErrors implementation and call the shared helper with "CLUSTERS-MGMT-VALIDATION-001"; do the same in platform-api/pkg/handlers/nodepool.go lines 272-282 using "NODEPOOLS-MGMT-VALIDATION-001", preserving the existing 422 response shape.platform-api/internal/codegen/featuregate/types.go (1)
6-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that the iota order defines gate inclusion.
Includescompares stage ordinals.DevPreviewNoUpgradetherefore also enablesTechPreviewgates becauseTechPreviewis declared beforeDevPreview. Any new stage inserted in this const block changes gating behavior silently. Add a comment that states the required order, and add a unit test that asserts the inclusion matrix for the three feature sets.♻️ Proposed comment
const ( + // The declaration order is significant. FeatureSet.Includes compares stage + // ordinals, so a stage must be declared after every stage it subsumes. GA FeatureStage = iota TechPreview DevPreview )Also applies to: 51-53
🤖 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 `@platform-api/internal/codegen/featuregate/types.go` around lines 6 - 10, Document on the FeatureStage const block that declaration order is significant because Includes uses stage ordinals, and preserve the required GA, TechPreview, DevPreview ordering when adding stages. Add a unit test covering the inclusion matrix for all three feature sets, including DevPreviewNoUpgrade enabling TechPreview gates.hyperfleet-operator/api/v1alpha1/cluster_types.go (1)
40-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider validation markers for the new identifier fields.
CreatorARNuses+kubebuilder:validation:Pattern.AccountIDandInternalIDaccept any string, including very long values. AddMaxLengthand, forAccountID, a 12-digit pattern to keep the CRD schema self-describing and to bound stored data.Note also that the type doc states the owning AWS account is stored in the
hyperfleet.io/account-idlabel.spec.accountIdnow duplicates that value, so the operator needs one authoritative source.♻️ Proposed markers
// AccountID is the AWS account ID that owns this cluster (platform-managed). // +optional + // +kubebuilder:validation:MaxLength=12 + // +kubebuilder:validation:Pattern=`^[0-9]{12}$` AccountID string `json:"accountId,omitempty"` // InternalID is an internal platform identifier for this cluster (platform-managed). // +optional + // +kubebuilder:validation:MaxLength=253 InternalID string `json:"internalId,omitempty"`🤖 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 `@hyperfleet-operator/api/v1alpha1/cluster_types.go` around lines 40 - 46, Add CRD validation markers to AccountID and InternalID, bounding both fields with an appropriate MaxLength and restricting AccountID to exactly 12 digits like the existing CreatorARN validation style. Update the operator’s account lookup/reconciliation logic so hyperfleet.io/account-id is the single authoritative source rather than allowing spec.accountId to diverge.
🤖 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 `@api/public/v2alpha1/cluster_types.go`:
- Around line 72-77: Update the CreatorARN validation pattern to accept all
supported AWS partitions, including aws, aws-us-gov, and aws-cn, while
preserving ARN prefix validation. Confirm the platform’s intended partition
scope before widening the kubebuilder pattern.
- Around line 36-88: Update marker-scanner and verify-codegen to include
hostedclusterspec.passthrough.go in mirror synchronization checks, comparing
every generated HostedCluster field with the tracked spec.hostedCluster registry
entry set. Ensure the passthrough generation step detects and skips or rejects
duplicate declarations so each upstream field has exactly one registry entry.
In `@api/public/v2alpha1/configuration.go`:
- Around line 7-63: Update HostedClusterSpecPassthrough.Configuration to use the
local ClusterConfiguration mirror instead of
hypershiftv1beta1.ClusterConfiguration, and register its nested fields under the
spec.hostedCluster.configuration prefix rather than as an unprefixed root.
Ensure the registration preserves the existing nested metadata, including
service-set write modes.
In `@api/public/v2alpha1/nodepool_types.go`:
- Around line 36-69: Update MarkerScanner and the FieldValidator registry
construction to detect duplicate field paths or namespace keys within each
resource type, including collisions between ClusterSpec and NodePoolSpec paths
such as spec.<field>. Return a clear generation error instead of overwriting an
existing entry, and ensure map iteration order cannot change whether the
conflict is detected.
In `@Makefile`:
- Around line 329-355: Update the codegen pipeline around codegen-passthrough
and verify-codegen so passthrough-gen output becomes the source compiled by the
build rather than being moved only to the ignored .raw file. Preserve any
required transformation, write the resulting output to the checked-in
passthrough source, and add a git diff verification for that source so
verify-codegen fails when generated content differs.
In `@platform-api/pkg/handlers/cluster.go`:
- Around line 115-118: Update both cluster validation paths, including the flow
around ValidateCreate, to pass the configured or account-specific FeatureSet
instead of featuregate.Default. Ensure the same resolved feature set is used for
create and update validation so TechPreview and DevPreview fields are validated
according to the request’s account configuration.
- Around line 239-246: The restoration block in ApplyPlatformUpdateToClusterCR
must preserve every registry-managed ServiceSet field after replacing cr.Spec,
not just AccountID, InternalID, CreatorARN, and IssuerURL. Restore omitted
HostedCluster values such as infraID, dns, release, and services using the
resource-specific registry paths, while retaining the existing expiration
fallback; apply the equivalent complete restoration in the nodepool update flow.
In `@platform-api/pkg/handlers/nodepool.go`:
- Around line 102-105: Update PlatformCreateToNodePoolCR to populate the node
pool spec’s service-owned AccountID and InternalPoolID fields before creating
the CR, using the authoritative accountID and internal pool ID sources rather
than client-provided req.Spec values. Preserve validation that rejects
client-supplied managed fields and continue applying the account label.
In `@platform-api/pkg/validation/field_validator.go`:
- Around line 131-136: Update the ServiceSet handling in ValidateUpdate so
OperationUpdate permits a field value that is unchanged from the existing
object, while still rejecting changed or newly supplied platform-managed values.
Preserve the current rejection behavior for non-update operations, and add tests
covering unchanged ServiceSet fields during updates.
---
Nitpick comments:
In `@hack/api-codegen/pkg/markers/scanner_test.go`:
- Line 124: Add a test case alongside the existing NewScanner coverage that
constructs the scanner with verbose=true, captures its registry output, and
asserts it matches the established expected output. Exercise the verbose path
through MarkerScanner.Scan and logf without changing the existing false-verbose
cases.
- Around line 155-159: Broaden the collision check in the scanner test beyond
the single "pausedUntil" key: iterate over every registry key produced from the
two scanned roots and assert that each is prefixed, using strings.HasPrefix with
the expected root prefixes. Add the strings import and retain the existing
failure behavior for unprefixed keys.
In `@hyperfleet-operator/api/v1alpha1/cluster_types.go`:
- Around line 40-46: Add CRD validation markers to AccountID and InternalID,
bounding both fields with an appropriate MaxLength and restricting AccountID to
exactly 12 digits like the existing CreatorARN validation style. Update the
operator’s account lookup/reconciliation logic so hyperfleet.io/account-id is
the single authoritative source rather than allowing spec.accountId to diverge.
In `@hyperfleet-operator/api/v1alpha1/nodepool_types.go`:
- Around line 39-45: Align the node pool internal identifier naming with
ClusterSpec by renaming NodePoolSpec’s InternalPoolID field and JSON key to
InternalID and internalId, unless the API intentionally requires distinct
concepts; update generated field-registry references and consumers consistently
with the chosen contract.
In `@platform-api/internal/codegen/conversion/cluster.go`:
- Around line 34-40: Update RewriteCloudURLWithID to parse baseURL into a
url.URL, explicitly handle any parse error, and construct the cloudUrl with
u.JoinPath(url.PathEscape(clusterID)) so clusterID remains a single path
segment. Preserve the existing nil-spec early return and assign the resulting
URL string to spec["cloudUrl"].
In `@platform-api/internal/codegen/featuregate/registry.go`:
- Around line 5-6: Protect the feature-gate registry declared as
HyperFleetFeatureGates from external mutation by unexporting the map and
providing read-only accessors for consumers, or explicitly documenting that it
must not be modified after initialization if the exported map is retained.
Ensure existing validation code uses the protected registry access path.
In `@platform-api/internal/codegen/featuregate/types.go`:
- Around line 6-10: Document on the FeatureStage const block that declaration
order is significant because Includes uses stage ordinals, and preserve the
required GA, TechPreview, DevPreview ordering when adding stages. Add a unit
test covering the inclusion matrix for all three feature sets, including
DevPreviewNoUpgrade enabling TechPreview gates.
In `@platform-api/internal/codegen/registry/field_metadata.go`:
- Around line 1-22: Update verify-codegen and the registry generation flow
around FieldRegistry so the hack/api-codegen and platform-api registry outputs
remain synchronized. Either generate both from one shared registry source or
make verify-codegen compare both generated outputs, including the Go and JSON
representations, and fail when they differ.
In `@platform-api/pkg/handlers/cluster.go`:
- Around line 315-325: Extract the duplicated validation-response logic into one
shared helper accepting the HTTP writer, validation errors, and error code. In
platform-api/pkg/handlers/cluster.go lines 315-325, remove the local
writeValidationErrors implementation and call the shared helper with
"CLUSTERS-MGMT-VALIDATION-001"; do the same in
platform-api/pkg/handlers/nodepool.go lines 272-282 using
"NODEPOOLS-MGMT-VALIDATION-001", preserving the existing 422 response shape.
In `@platform-api/pkg/validation/field_validator_test.go`:
- Around line 162-185: Extend validation test coverage by adding an
array-of-objects case to TestFlattenToFieldPaths that verifies nested list
fields are flattened, and add a
TestValidateUpdate_AllowsUnchangedServiceSetValue case using identical existing
and updated platform-managed values. Keep the assertions focused on the expected
flattened path and absence of validation errors.
- Around line 87-89: The validation tests index errs without confirming an error
exists, allowing a non-nil empty slice to panic. In the affected assertions and
the corresponding check around TestValidateCreate_RejectsServiceSetFields,
assert the expected error count before indexing, or search errs for the expected
"spec.fips" field while preserving the existing validation expectations.
🪄 Autofix
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: Repository: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: be4d1c60-fe86-4642-9045-76e611092add
⛔ Files ignored due to path filters (4)
api/public/v2alpha1/go.sumis excluded by!**/*.sumapi/public/v2alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*hack/api-codegen/go.sumis excluded by!**/*.sumplatform-api/go.sumis excluded by!**/*.sum
📒 Files selected for processing (29)
.gitignoreMakefileapi/public/v2alpha1/cluster_types.goapi/public/v2alpha1/configuration.goapi/public/v2alpha1/go.modapi/public/v2alpha1/groupversion_info.goapi/public/v2alpha1/hostedclusterspec.passthrough.goapi/public/v2alpha1/nodepool_types.gohack/api-codegen/cmd/marker-scanner/main.gohack/api-codegen/go.modhack/api-codegen/pkg/markers/gated_writemode_test.gohack/api-codegen/pkg/markers/scanner.gohack/api-codegen/pkg/markers/scanner_test.gohack/api-codegen/pkg/markers/types.gohyperfleet-operator/api/v1alpha1/cluster_types.gohyperfleet-operator/api/v1alpha1/nodepool_types.gohyperfleet-operator/config/crd/bases/hyperfleet.io_clusters.yamlhyperfleet-operator/config/crd/bases/hyperfleet.io_nodepools.yamlplatform-api/Containerfileplatform-api/go.modplatform-api/internal/codegen/conversion/cluster.goplatform-api/internal/codegen/featuregate/registry.goplatform-api/internal/codegen/featuregate/types.goplatform-api/internal/codegen/registry/field_metadata.goplatform-api/internal/codegen/registry/field_metadata.jsonplatform-api/pkg/handlers/cluster.goplatform-api/pkg/handlers/nodepool.goplatform-api/pkg/validation/field_validator.goplatform-api/pkg/validation/field_validator_test.go
| // CreatorARN is the IAM ARN of the user who created this cluster. | ||
| // +k8s:openapi-gen=false | ||
| // +hyperfleet:write-mode=service-set | ||
| // +optional | ||
| // +kubebuilder:validation:Pattern=`^arn:aws:` | ||
| CreatorARN string `json:"creatorARN,omitempty"` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Anchor the ARN pattern to all supported AWS partitions.
The pattern ^arn:aws: accepts only the aws partition. AWS GovCloud ARNs use arn:aws-us-gov: and China ARNs use arn:aws-cn:. If the platform sets CreatorARN from a GovCloud caller, the API server rejects the object at admission. The field is service-set, so the rejection surfaces as a platform-side failure, not a customer input error.
Confirm the intended partition scope. If GovCloud is in scope, widen the pattern.
🔧 Proposed pattern change
- // +kubebuilder:validation:Pattern=`^arn:aws:`
+ // +kubebuilder:validation:Pattern=`^arn:aws(-[a-z]+)*:`
CreatorARN string `json:"creatorARN,omitempty"`📝 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.
| // CreatorARN is the IAM ARN of the user who created this cluster. | |
| // +k8s:openapi-gen=false | |
| // +hyperfleet:write-mode=service-set | |
| // +optional | |
| // +kubebuilder:validation:Pattern=`^arn:aws:` | |
| CreatorARN string `json:"creatorARN,omitempty"` | |
| // CreatorARN is the IAM ARN of the user who created this cluster. | |
| // +k8s:openapi-gen=false | |
| // +hyperfleet:write-mode=service-set | |
| // +optional | |
| // +kubebuilder:validation:Pattern=`^arn:aws(-[a-z]+)*:` | |
| CreatorARN string `json:"creatorARN,omitempty"` |
🤖 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 `@api/public/v2alpha1/cluster_types.go` around lines 72 - 77, Update the
CreatorARN validation pattern to accept all supported AWS partitions, including
aws, aws-us-gov, and aws-cn, while preserving ARN prefix validation. Confirm the
platform’s intended partition scope before widening the kubebuilder pattern.
There was a problem hiding this comment.
Skip. The CreatorARN field is service-set and hidden from the public API — customers cannot set or see it.
There was a problem hiding this comment.
@cdoan1 Acknowledged. I will skip this finding. CreatorARN is platform-managed and hidden from the public API.
✏️ Learnings added
Learnt from: cdoan1
Repo: openshift-online/rosa-hyperfleet-api PR: 276
File: api/public/v2alpha1/cluster_types.go:72-77
Timestamp: 2026-08-04T23:29:54.252Z
Learning: In `api/public/v2alpha1/cluster_types.go`, `ClusterSpec.CreatorARN` is a platform-managed service-set field and is hidden from the public API. Customers cannot set or view this field.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
| // NodePoolSpec defines the desired state of a NodePool. | ||
| type NodePoolSpec struct { | ||
| // DisplayName is a human-readable name for the node pool. | ||
| // +hyperfleet:write-mode=mutable | ||
| // +kubebuilder:validation:MaxLength=256 | ||
| // +optional | ||
| DisplayName string `json:"displayName,omitempty"` | ||
|
|
||
| // AutoRepair enables automatic repair of unhealthy nodes. | ||
| // +hyperfleet:write-mode=mutable | ||
| // +optional | ||
| AutoRepair *bool `json:"autoRepair,omitempty"` | ||
|
|
||
| // Labels are customer-defined labels applied to nodes. | ||
| // +hyperfleet:write-mode=mutable | ||
| // +optional | ||
| Labels map[string]string `json:"labels,omitempty"` | ||
|
|
||
| // AccountID identifies the customer account (platform-managed, hidden from API). | ||
| // +k8s:openapi-gen=false | ||
| // +hyperfleet:write-mode=service-set | ||
| // +optional | ||
| AccountID string `json:"accountId,omitempty"` | ||
|
|
||
| // InternalPoolID is an internal platform identifier (platform-managed, hidden). | ||
| // +k8s:openapi-gen=false | ||
| // +hyperfleet:write-mode=service-set | ||
| // +optional | ||
| InternalPoolID string `json:"internalPoolId,omitempty"` | ||
|
|
||
| // NodePool is the full HyperShift NodePoolSpec. | ||
| // +kubebuilder:validation:Required | ||
| NodePool hypershiftv1beta1.NodePoolSpec `json:"nodePool"` | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Does the scanner detect or error on duplicate registry keys?
rg -n --type=go -C6 'Registry\[' hack/api-codegen/pkg/markers/scanner.go
ast-grep outline hack/api-codegen/pkg/markers/scanner.go --items all
# Does the validator distinguish cluster vs nodepool when looking up spec.* keys?
fd -t f 'field_validator.go' --exec rg -n -C5 'FieldRegistry|Lookup|spec\.' {}Repository: openshift-online/rosa-hyperfleet-api
Length of output: 2504
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- scanner implementation ---'
sed -n '1,340p' hack/api-codegen/pkg/markers/scanner.go
printf '%s\n' '--- registry and validator files ---'
fd -t f . | rg '(^|/)(field_metadata\.go|field_validator\.go|registry|validator)'Repository: openshift-online/rosa-hyperfleet-api
Length of output: 10119
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- generated registry keys ---'
rg -n -C2 '"spec\.(accountId|displayName)|spec\.(hostedCluster|nodePool)\.(accountId|displayName)' \
platform-api/internal/codegen/registry/field_metadata.go \
hack/api-codegen/pkg/registry/field_metadata.go \
platform-api/internal/codegen/registry/field_metadata.json \
hack/api-codegen/pkg/registry/field_metadata.json
printf '%s\n' '--- validator lookup and path construction ---'
sed -n '1,280p' platform-api/pkg/validation/field_validator.go
rg -n -C4 'buildFieldPath|rootTypePrefix|HostedClusterSpecPassthrough|NodePoolSpecPassthrough|type (Cluster|NodePool).*Spec' \
platform-api hack/api-codegen apiRepository: openshift-online/rosa-hyperfleet-api
Length of output: 24610
Reject conflicting duplicate field paths in code generation.
ClusterSpec and NodePoolSpec both resolve fields to spec.<field>, and FieldValidator performs a flat registry lookup. MarkerScanner overwrites duplicate entries without an error while iterating root types from a map. Reject conflicting metadata or namespace registry keys by resource type.
🤖 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 `@api/public/v2alpha1/nodepool_types.go` around lines 36 - 69, Update
MarkerScanner and the FieldValidator registry construction to detect duplicate
field paths or namespace keys within each resource type, including collisions
between ClusterSpec and NodePoolSpec paths such as spec.<field>. Return a clear
generation error instead of overwriting an existing entry, and ensure map
iteration order cannot change whether the conflict is detected.
There was a problem hiding this comment.
defer: ROSAENG-64559
There was a problem hiding this comment.
@cdoan1, acknowledged. ROSAENG-64559 tracks this deferred finding.
You are interacting with an AI system.
Wire the generated FieldRegistry into cluster and nodepool handlers to enforce write-mode and feature-gate rules on create/update requests. - Service-set fields rejected if sent by customers (422) - Immutable fields rejected on update if changed (422) - Feature-gated fields rejected without the gate enabled (422) - Validation errors returned with per-field detail Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
6ceb151 to
a321d18
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
platform-api/pkg/validation/field_validator.go (1)
36-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid ignored return values from
strings.Builder.WriteString.
WriteStringreturns a nil error by contract, so this has no runtime failure risk. Usestrings.Jointo satisfy the error-handling rule and preserve the output.🤖 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 `@platform-api/pkg/validation/field_validator.go` around lines 36 - 40, Update the error formatting loop in the validation error method to use strings.Join over the individual err.Error() messages, preserving the existing indentation and newline output while avoiding ignored strings.Builder.WriteString return values.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.
Nitpick comments:
In `@platform-api/pkg/validation/field_validator.go`:
- Around line 36-40: Update the error formatting loop in the validation error
method to use strings.Join over the individual err.Error() messages, preserving
the existing indentation and newline output while avoiding ignored
strings.Builder.WriteString return values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ac48957e-d8ce-4de8-add8-65ad79bc1bd7
⛔ Files ignored due to path filters (4)
api/public/v2alpha1/go.sumis excluded by!**/*.sumapi/public/v2alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*hack/api-codegen/go.sumis excluded by!**/*.sumplatform-api/go.sumis excluded by!**/*.sum
📒 Files selected for processing (29)
.gitignoreMakefileapi/public/v2alpha1/cluster_types.goapi/public/v2alpha1/configuration.goapi/public/v2alpha1/go.modapi/public/v2alpha1/groupversion_info.goapi/public/v2alpha1/hostedclusterspec.passthrough.goapi/public/v2alpha1/nodepool_types.gohack/api-codegen/cmd/marker-scanner/main.gohack/api-codegen/go.modhack/api-codegen/pkg/markers/gated_writemode_test.gohack/api-codegen/pkg/markers/scanner.gohack/api-codegen/pkg/markers/scanner_test.gohack/api-codegen/pkg/markers/types.gohyperfleet-operator/api/v1alpha1/cluster_types.gohyperfleet-operator/api/v1alpha1/nodepool_types.gohyperfleet-operator/config/crd/bases/hyperfleet.io_clusters.yamlhyperfleet-operator/config/crd/bases/hyperfleet.io_nodepools.yamlplatform-api/Containerfileplatform-api/go.modplatform-api/internal/codegen/conversion/cluster.goplatform-api/internal/codegen/featuregate/registry.goplatform-api/internal/codegen/featuregate/types.goplatform-api/internal/codegen/registry/field_metadata.goplatform-api/internal/codegen/registry/field_metadata.jsonplatform-api/pkg/handlers/cluster.goplatform-api/pkg/handlers/nodepool.goplatform-api/pkg/validation/field_validator.goplatform-api/pkg/validation/field_validator_test.go
🚧 Files skipped from review as they are similar to previous changes (26)
- .gitignore
- platform-api/internal/codegen/featuregate/types.go
- hack/api-codegen/pkg/markers/gated_writemode_test.go
- hack/api-codegen/pkg/markers/types.go
- platform-api/internal/codegen/featuregate/registry.go
- hack/api-codegen/cmd/marker-scanner/main.go
- platform-api/Containerfile
- hyperfleet-operator/api/v1alpha1/cluster_types.go
- platform-api/go.mod
- platform-api/internal/codegen/registry/field_metadata.go
- hack/api-codegen/pkg/markers/scanner_test.go
- hack/api-codegen/go.mod
- hyperfleet-operator/config/crd/bases/hyperfleet.io_nodepools.yaml
- platform-api/internal/codegen/conversion/cluster.go
- hyperfleet-operator/config/crd/bases/hyperfleet.io_clusters.yaml
- hyperfleet-operator/api/v1alpha1/nodepool_types.go
- platform-api/pkg/handlers/nodepool.go
- api/public/v2alpha1/go.mod
- api/public/v2alpha1/nodepool_types.go
- api/public/v2alpha1/groupversion_info.go
- platform-api/pkg/validation/field_validator_test.go
- hack/api-codegen/pkg/markers/scanner.go
- platform-api/pkg/handlers/cluster.go
- api/public/v2alpha1/hostedclusterspec.passthrough.go
- api/public/v2alpha1/configuration.go
- api/public/v2alpha1/cluster_types.go
Go structs without omitempty serialize zero values to JSON, causing the field validator to reject fields the client never explicitly set. Skip ServiceSet enforcement when the field value is at its zero value. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add AccountID, InternalID to internal ClusterSpec; AccountID, InternalPoolID to internal NodePoolSpec - Restore all service-set fields after full spec replacement in cluster and nodepool update handlers - Add verbose mode to marker-scanner (make codegen VERBOSE=1) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ions PlatformCreateToClusterCR and PlatformCreateToNodePoolCR now set AccountID/InternalID/InternalPoolID from authoritative platform sources instead of relying on client-provided req.Spec values. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
12e0b20 to
8f04f71
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@cdoan1: The following test 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. |
2 participants
Description
Type of Change
Testing
Update a cluster and verify accountId, internalId, creatorARN, issuerURL are unchanged in the stored spec
Update a nodepool and verify accountId, internalPoolId are unchanged
Verify expirationTimestamp is only restored when the customer doesn't explicitly set it
Run make codegen VERBOSE=1 and confirm scanner logs field registration
Unit tests pass (
make test)Integration tests pass (if applicable)
Manual verification completed
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores