ROSAENG-61805: feat: OpenAPI spec alignment with codegen pipeline - #279
ROSAENG-61805: feat: OpenAPI spec alignment with codegen pipeline#279cdoan1 wants to merge 7 commits into
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>
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>
openshift-ci-robot
added
the
jira/valid-reference
|
@cdoan1: This pull request references ROSAENG-61805 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
WalkthroughThis change adds public ChangesPublic API and platform integration
Runtime validation and managed fields
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant APIHandler
participant FieldValidator
participant FieldRegistry
participant CRStore
Client->>APIHandler: Submit Cluster or NodePool spec
APIHandler->>FieldValidator: Validate create or update
FieldValidator->>FieldRegistry: Read field metadata and feature gates
FieldRegistry-->>FieldValidator: Return validation rules
FieldValidator-->>APIHandler: Return validation errors or success
APIHandler->>CRStore: Persist spec and service-managed fields
CRStore-->>Client: Return resource response
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: 11
🧹 Nitpick comments (9)
api/public/v2alpha1/configuration.go (1)
150-168: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEmpty placeholder structs discard any data assigned to them.
Each placeholder type has no fields. A value set on
APIServer,Authentication,Network, or any sibling serializes as{}and loses its content. These fields areservice-setand hidden from the OpenAPI schema, so a customer cannot reach them today. If the service later populates them from HyperShift, the data will be dropped until the types gain fields. Add a short comment stating that these types must stay empty until the corresponding area is exposed.🤖 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/configuration.go` around lines 150 - 168, Add a concise comment above the placeholder configuration types explaining that they must remain empty until their corresponding configuration areas are exposed, preventing accidental population before API support exists. Keep all placeholder structs, including APIServerNetworkConfiguration, ClusterAuthentication, and the sibling types, unchanged.platform-api/pkg/clients/hyperfleetdb/convert_test.go (2)
41-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for overriding caller-supplied identity fields.
Both tests start from a spec where
AccountIDand the internal ID are empty, so the assertions pass whether the helper assigns the value or merely leaves the caller's value in place. The security-relevant behavior is the override: a caller may sendaccountIdin the request body, and the helper must replace it with the server-derived value. Seed the fixture spec with a foreign value and assert that the helper replaces it.💚 Proposed override assertions
Spec: &hyperfleetv1alpha1.ClusterSpec{ + AccountID: "attacker-supplied-account", + InternalID: "attacker-supplied-id", HostedCluster: hypershiftv1beta1.HostedClusterSpec{The existing assertions at lines 74-80 then prove that the helper overwrites both values.
Also applies to: 74-80
🤖 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/clients/hyperfleetdb/convert_test.go` around lines 41 - 47, Update the fixture used by the conversion test to seed Spec.AccountID and Spec.InternalPoolID with foreign caller-supplied values instead of leaving them empty. Keep the existing assertions around the conversion helper and ensure they verify both fields are replaced with the server-derived account and pool IDs, covering the override behavior.
28-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe two helpers take their string arguments in opposite orders.
PlatformCreateToNodePoolCRtakes(accountID, internalPoolID, req).PlatformCreateToClusterCRtakes(clusterID, accountID, req). Both leading parameters are plain strings, so a transposed call site compiles and silently assigns the account ID as the internal identifier. Align the parameter order across both helpers, or introduce distinct named string types so the compiler catches a swap.Also applies to: 65-65
🤖 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/clients/hyperfleetdb/convert_test.go` at line 28, Align the leading parameter order of PlatformCreateToNodePoolCR and PlatformCreateToClusterCR so both helpers use the same identifier ordering, then update their call sites including the cases in convert_test.go. Preserve the existing identifier values while ensuring account and pool/cluster IDs cannot be silently transposed; alternatively, introduce distinct named string types for these identifiers and apply them consistently.hyperfleet-operator/api/v1alpha1/cluster_types.go (1)
40-46: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPlatform-managed identifiers added without immutability rules. Both files add optional, platform-managed identifier fields that the conversion helpers set exactly once at creation. Neither field carries a CEL immutability rule, so an in-place patch can change the recorded owner while the
hyperfleet.io/account-idlabel that scoping queries use stays unchanged. The shared root cause is one missingXValidationrule per identifier field.
hyperfleet-operator/api/v1alpha1/cluster_types.go#L40-L46: add+kubebuilder:validation:XValidation:rule="oldSelf == '' || self == oldSelf"toAccountIDandInternalID, then regeneratehyperfleet.io_clusters.yaml.hyperfleet-operator/api/v1alpha1/nodepool_types.go#L39-L45: add the same rule toAccountIDandInternalPoolID, then regeneratehyperfleet.io_nodepools.yaml.🤖 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 the CEL immutability rule oldSelf == '' || self == oldSelf to AccountID and InternalID in hyperfleet-operator/api/v1alpha1/cluster_types.go, and to AccountID and InternalPoolID in hyperfleet-operator/api/v1alpha1/nodepool_types.go; then regenerate hyperfleet.io_clusters.yaml and hyperfleet.io_nodepools.yaml.hack/api-codegen/pkg/openapi/generator.go (2)
183-187: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnmapped definitions now skip hidden-field pruning silently.
filterHiddenFieldsnow continues whentypeToRegistryPrefixhas no entry for a definition. A new nested public type keeps its hidden properties in the generated schema, and the build stays green. The prefix map is maintained by hand, so this failure mode is easy to introduce.Report unmapped definitions instead of ignoring them. A cheap option is to collect the skipped names and return them so the caller can log or fail.
♻️ Suggested change to surface unmapped definitions
- for typeName, schema := range definitions { - prefix, ok := typeToRegistryPrefix[typeName] - if !ok { - continue - } - pruned := pruneHiddenProperties(&schema, prefix, hiddenPaths) + var unmapped []string + for typeName, schema := range definitions { + prefix, ok := typeToRegistryPrefix[typeName] + if !ok { + unmapped = append(unmapped, typeName) + continue + } + pruned := pruneHiddenProperties(&schema, prefix, hiddenPaths) definitions[typeName] = *pruned } + if len(unmapped) > 0 { + sort.Strings(unmapped) + fmt.Fprintf(os.Stderr, "warning: no registry prefix for definitions: %v\n", unmapped) + }🤖 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/openapi/generator.go` around lines 183 - 187, Update filterHiddenFields around the typeToRegistryPrefix lookup to collect definition names that have no registry prefix instead of silently continuing. Return the unmapped names alongside the pruning result so the caller can log or fail explicitly, while preserving pruning for mapped definitions.
244-249: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the duplicated passthrough type list.
The literal
[]string{"HostedClusterSpecPassthrough", "NodePoolSpecPassthrough"}appears twice in the same function. A future edit can update one list and miss the other.♻️ Suggested change
+// passthroughTypes are the definitions collapsed into shallow objects. +var passthroughTypes = []string{"HostedClusterSpecPassthrough", "NodePoolSpecPassthrough"} + func collapsePassthroughTypes(definitions map[string]apiextensionsv1.JSONSchemaProps) { // Mark passthrough types as accepting additional properties - for _, typeName := range []string{"HostedClusterSpecPassthrough", "NodePoolSpecPassthrough"} { + for _, typeName := range passthroughTypes {Also applies to: 269-286
🤖 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/openapi/generator.go` around lines 244 - 249, Extract the duplicated passthrough type names into a single local collection within the generator function, then reuse it in both loops, including the logic around HostedClusterSpecPassthrough and NodePoolSpecPassthrough. Ensure both code paths iterate the shared collection so future edits require changing the list only once.platform-api/go.mod (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThe service now depends on a
hack/tooling module at runtime.
platform-apiimportshack/api-codegen/pkg/registryinpkg/validation/field_validator.go. This makes build-time code-generation tooling part of the production dependency graph. The Containerfile already copies the wholehack/api-codegentree into the builder for this reason.Consider moving the generated field registry into a shared library module, for example
api/public/...or a dedicatedpkg/module, and keephack/for generators only. That keeps the dependency direction one-way.Also applies to: 24-24
🤖 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/go.mod` at line 6, Move the runtime field registry currently imported by pkg/validation/field_validator.go from hack/api-codegen/pkg/registry into a shared production library module such as api/public or a dedicated pkg module. Update field_validator.go and go.mod to use the new module path, while keeping hack/api-codegen limited to generation tooling and preserving the registry API used by validation.platform-api/pkg/handlers/cluster.go (1)
315-325: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShare the validation-error response helper.
NodePoolHandler.writeValidationErrorsinplatform-api/pkg/handlers/nodepool.gorepeats this body. Only the error code differs. Extract one helper that takes the code, and call it from both handlers.🤖 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 construction from ClusterHandler.writeValidationErrors and NodePoolHandler.writeValidationErrors into a shared helper accepting the validation error code. Update both handlers to call the helper with their respective codes while preserving the existing headers, status, response fields, and encoding behavior.platform-api/pkg/validation/field_validator_test.go (1)
148-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases that use the real registry and the read-modify-write flow.
Every test builds a synthetic registry, so none of them exercise
registry.FieldRegistry. The nestedServiceSetcontainer paths described in thefield_validator.gocomment stay untested.TestFlattenToFieldPathsalready shows the behavior: it asserts thatspec.hostedClusterappears as its own path.Add two cases:
NewFieldValidator()with a spec that sets onlyspec.hostedCluster.configuration.kubelet.maxPods. Assert no error.ValidateUpdatewhere the new spec repeats the existingServiceSetvalue unchanged. Assert no error.Also note that
errs[0]assumes a stable order.validateiterates a map, so the order is not defined once a test registry holds more than one matching field. Search the slice for the expected field instead.🤖 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 148 - 185, Expand the validator tests to use the real registry returned by NewFieldValidator, including a create case that sets only spec.hostedCluster.configuration.kubelet.maxPods and expects no error, plus a ValidateUpdate case where the new spec preserves the existing ServiceSet value and expects no error. Update error assertions to search errs for the expected field rather than relying on errs[0], since validate iterates an unordered map.
🤖 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 76-77: Update the validation pattern on CreatorARN to accept AWS
commercial, GovCloud, and China ARN partitions instead of only the `aws`
partition; preserve ARN-prefix validation while allowing `arn:aws-us-gov:` and
`arn:aws-cn:` callers.
In `@api/public/v2alpha1/go.mod`:
- Around line 5-29: Update the golang.org/x/text dependency in the go.mod
require block to version v0.39.0 or later, replacing v0.38.0 while preserving
the indirect dependency declaration.
In `@api/public/v2alpha1/hostedclusterspec.passthrough.go`:
- Around line 66-69: Update the passthrough generator inputs for AutoNode and
FIPS so both service-set fields are emitted as optional in the public request
schema. Replace the unsupported optionality representation with the
generator-recognized optional marker or JSON omission tag, rather than editing
generated OpenAPI output. Verify they are removed from required lists for all
affected cluster request operations.
In `@api/public/v2alpha1/nodepool_types.go`:
- Around line 66-68: Update NodePoolSpecPassthrough in
hostedclusterspec.passthrough.go to mark customer-settable fields such as
replicas, platform, and release with +k8s:openapi-gen=true instead of excluding
them, so the generated NodePoolSpecPassthrough schema exposes those properties
while preserving nodePool as required.
In `@hack/api-codegen/cmd/openapi-merge/main.go`:
- Around line 61-64: Make the schema merge fail when a requested definition is
absent instead of warning and continuing, and propagate that failure from the
main merge flow. Update insertSchemaBlock to report whether components.schemas
was found and whether insertion succeeded, then only count successful insertions
when it returns success. Add regression tests covering both missing generated
definitions and specifications without components.schemas.
In `@hack/api-codegen/pkg/registry/field_metadata.go`:
- Around line 581-584: Update the validation for
spec.hostedCluster.imageContentSources before the Mutable field update path
copies customer input into the cluster CR. Extend FieldValidator or the relevant
update flow to reject source and mirror registry hosts not on the approved
allowlist; otherwise change this field’s registry and source metadata to
platform-managed rather than Mutable.
In `@platform-api/openapi/openapi.yaml`:
- Around line 2260-2287: Update the ClusterCreateRequest description to remove
references to creatorARN, hostedCluster.release.image, and
hostedCluster.networking, retaining only the issuerURL behavior that remains
represented by HostedClusterSpecPassthrough. Ensure the documentation matches
the regenerated ClusterSpec schema.
- Around line 2280-2284: Update the OpenAPI schema generation around the listed
fields to exclude feature-gated properties from the default document, including
tags, serializeImagePulls, registryPullQPS, registryBurst, and
allowedKernelArguments. Use feature-set-specific schemas where supported, or
omit these properties from the default schema so it matches featuregate.Default
validation behavior.
In `@platform-api/pkg/handlers/cluster.go`:
- Around line 231-246: Fix the root cause in ApplyPlatformUpdateToClusterCR and
ApplyPlatformUpdateToNodePoolCR by merging only customer-writable fields or
deriving preserved fields from registry.FieldRegistry instead of replacing the
full spec; this must retain every ServiceSet field. In
platform-api/pkg/handlers/cluster.go lines 231-246, update the restoration/merge
to preserve the listed hostedCluster and cluster service-managed fields. In
platform-api/pkg/handlers/nodepool.go lines 198-208, likewise preserve the
listed nodePool ServiceSet fields alongside AccountID and InternalPoolID.
In `@platform-api/pkg/validation/field_validator.go`:
- Around line 158-172: Update flattenToFieldPaths to propagate serialization or
deserialization failures instead of returning nil, and update its callers,
including validate and the ValidateCreate/ValidateUpdate paths, to convert those
failures into an explicit validation error. Preserve normal field flattening for
successful conversions and ensure inspection failures never result in silent
validation success.
- Around line 131-156: Update the ServiceSet branch in the field-validation
function to compare values: on create reject only non-null values, and on update
reject only values that differ from the existing field, including clearing. For
object-valued container paths, skip the container-level rejection only when a
registered descendant is being validated; preserve descendant validation and
leave Immutable/Mutable behavior unchanged.
---
Nitpick comments:
In `@api/public/v2alpha1/configuration.go`:
- Around line 150-168: Add a concise comment above the placeholder configuration
types explaining that they must remain empty until their corresponding
configuration areas are exposed, preventing accidental population before API
support exists. Keep all placeholder structs, including
APIServerNetworkConfiguration, ClusterAuthentication, and the sibling types,
unchanged.
In `@hack/api-codegen/pkg/openapi/generator.go`:
- Around line 183-187: Update filterHiddenFields around the typeToRegistryPrefix
lookup to collect definition names that have no registry prefix instead of
silently continuing. Return the unmapped names alongside the pruning result so
the caller can log or fail explicitly, while preserving pruning for mapped
definitions.
- Around line 244-249: Extract the duplicated passthrough type names into a
single local collection within the generator function, then reuse it in both
loops, including the logic around HostedClusterSpecPassthrough and
NodePoolSpecPassthrough. Ensure both code paths iterate the shared collection so
future edits require changing the list only once.
In `@hyperfleet-operator/api/v1alpha1/cluster_types.go`:
- Around line 40-46: Add the CEL immutability rule oldSelf == '' || self ==
oldSelf to AccountID and InternalID in
hyperfleet-operator/api/v1alpha1/cluster_types.go, and to AccountID and
InternalPoolID in hyperfleet-operator/api/v1alpha1/nodepool_types.go; then
regenerate hyperfleet.io_clusters.yaml and hyperfleet.io_nodepools.yaml.
In `@platform-api/go.mod`:
- Line 6: Move the runtime field registry currently imported by
pkg/validation/field_validator.go from hack/api-codegen/pkg/registry into a
shared production library module such as api/public or a dedicated pkg module.
Update field_validator.go and go.mod to use the new module path, while keeping
hack/api-codegen limited to generation tooling and preserving the registry API
used by validation.
In `@platform-api/pkg/clients/hyperfleetdb/convert_test.go`:
- Around line 41-47: Update the fixture used by the conversion test to seed
Spec.AccountID and Spec.InternalPoolID with foreign caller-supplied values
instead of leaving them empty. Keep the existing assertions around the
conversion helper and ensure they verify both fields are replaced with the
server-derived account and pool IDs, covering the override behavior.
- Line 28: Align the leading parameter order of PlatformCreateToNodePoolCR and
PlatformCreateToClusterCR so both helpers use the same identifier ordering, then
update their call sites including the cases in convert_test.go. Preserve the
existing identifier values while ensuring account and pool/cluster IDs cannot be
silently transposed; alternatively, introduce distinct named string types for
these identifiers and apply them consistently.
In `@platform-api/pkg/handlers/cluster.go`:
- Around line 315-325: Extract the duplicated validation response construction
from ClusterHandler.writeValidationErrors and
NodePoolHandler.writeValidationErrors into a shared helper accepting the
validation error code. Update both handlers to call the helper with their
respective codes while preserving the existing headers, status, response fields,
and encoding behavior.
In `@platform-api/pkg/validation/field_validator_test.go`:
- Around line 148-185: Expand the validator tests to use the real registry
returned by NewFieldValidator, including a create case that sets only
spec.hostedCluster.configuration.kubelet.maxPods and expects no error, plus a
ValidateUpdate case where the new spec preserves the existing ServiceSet value
and expects no error. Update error assertions to search errs for the expected
field rather than relying on errs[0], since validate iterates an unordered map.
🪄 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: 82362022-3a04-41b5-8f0e-0ff6f9a4d2fe
⛔ 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 (35)
.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/cmd/openapi-merge/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.gohack/api-codegen/pkg/openapi/generator.gohack/api-codegen/pkg/openapi/generator_test.gohack/api-codegen/pkg/registry/field_metadata.gohack/api-codegen/pkg/registry/field_metadata.jsonhyperfleet-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/openapi/openapi.yamlplatform-api/pkg/clients/hyperfleetdb/convert.goplatform-api/pkg/clients/hyperfleetdb/convert_test.goplatform-api/pkg/handlers/cluster.goplatform-api/pkg/handlers/nodepool.goplatform-api/pkg/validation/field_validator.goplatform-api/pkg/validation/field_validator_test.go
| // +kubebuilder:validation:Pattern=`^arn:aws:` | ||
| CreatorARN string `json:"creatorARN,omitempty"` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restrict on ARN partition rejects GovCloud and China ARNs.
The pattern ^arn:aws: rejects arn:aws-us-gov: and arn:aws-cn: principal ARNs. If the service must run in those partitions, the create path silently fails validation for every caller there. Widen the pattern if non-commercial partitions are in scope.
🛠️ Proposed pattern widening
- // +kubebuilder:validation:Pattern=`^arn:aws:`
+ // +kubebuilder:validation:Pattern=`^arn:(aws|aws-cn|aws-us-gov):`
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.
| // +kubebuilder:validation:Pattern=`^arn:aws:` | |
| CreatorARN string `json:"creatorARN,omitempty"` | |
| // +kubebuilder:validation:Pattern=`^arn:(aws|aws-cn|aws-us-gov):` | |
| 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 76 - 77, Update the
validation pattern on CreatorARN to accept AWS commercial, GovCloud, and China
ARN partitions instead of only the `aws` partition; preserve ARN-prefix
validation while allowing `arn:aws-us-gov:` and `arn:aws-cn:` callers.
| require ( | ||
| github.com/openshift/api v0.0.0-20260416105050-3c6b218b8a80 | ||
| github.com/openshift/hypershift/api v0.0.0-20260625052409-9acec4759a16 | ||
| k8s.io/api v0.36.0 | ||
| k8s.io/apimachinery v0.36.0 | ||
| ) | ||
|
|
||
| require ( | ||
| github.com/fxamacker/cbor/v2 v2.9.2 // indirect | ||
| github.com/go-logr/logr v1.4.4 // indirect | ||
| github.com/json-iterator/go v1.1.12 // indirect | ||
| github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect | ||
| github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee // indirect | ||
| github.com/x448/float16 v0.8.4 // indirect | ||
| go.yaml.in/yaml/v2 v2.4.4 // indirect | ||
| golang.org/x/net v0.56.0 // indirect | ||
| golang.org/x/text v0.38.0 // indirect | ||
| gopkg.in/inf.v0 v0.9.1 // indirect | ||
| k8s.io/klog/v2 v2.140.0 // indirect | ||
| k8s.io/kube-openapi v0.0.0-20260317180543-43fb72c5454a // indirect | ||
| k8s.io/utils v0.0.0-20260707023825-cf1189d6abe3 // indirect | ||
| sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 // indirect | ||
| sigs.k8s.io/randfill v1.0.0 // indirect | ||
| sigs.k8s.io/structured-merge-diff/v6 v6.3.2 // indirect | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look up advisories for the direct and notable indirect dependencies.
set -uo pipefail
for pkg in \
"k8s.io/apimachinery" \
"k8s.io/api" \
"golang.org/x/net" \
"golang.org/x/text" \
"github.com/openshift/api"
do
echo "=== $pkg ==="
curl -s -X POST https://api.osv.dev/v1/query \
-d "{\"package\":{\"name\":\"$pkg\",\"ecosystem\":\"Go\"}}" \
| jq -r '.vulns[]? | "\(.id) \(.summary // "no summary")"' | head -20
doneRepository: openshift-online/rosa-hyperfleet-api
Length of output: 2768
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json
import urllib.request
deps = {
"github.com/openshift/api": "v0.0.0-20260416105050-3c6b218b8a80",
"github.com/openshift/hypershift/api": "v0.0.0-20260625052409-9acec4759a16",
"k8s.io/api": "v0.36.0",
"k8s.io/apimachinery": "v0.36.0",
"github.com/fxamacker/cbor/v2": "v2.9.2",
"github.com/go-logr/logr": "v1.4.4",
"github.com/json-iterator/go": "v1.1.12",
"github.com/modern-go/concurrent": "v0.0.0-20180306012644-bacd9c7ef1dd",
"github.com/modern-go/reflect2": "v1.0.3-0.20250322232337-35a7c28c31ee",
"github.com/x448/float16": "v0.8.4",
"go.yaml.in/yaml/v2": "v2.4.4",
"golang.org/x/net": "v0.56.0",
"golang.org/x/text": "v0.38.0",
"gopkg.in/inf.v0": "v0.9.1",
"k8s.io/klog/v2": "v2.140.0",
"k8s.io/kube-openapi": "v0.0.0-20260317180543-43fb72c5454a",
"k8s.io/utils": "v0.0.0-20260707023825-cf1189d6abe3",
"sigs.k8s.io/json": "v0.0.0-20250730193827-2d320260d730",
"sigs.k8s.io/randfill": "v1.0.0",
"sigs.k8s.io/structured-merge-diff/v6": "v6.3.2",
}
for name, version in deps.items():
body = json.dumps({
"package": {"name": name, "ecosystem": "Go"},
"version": version,
}).encode()
req = urllib.request.Request(
"https://api.osv.dev/v1/query",
data=body,
headers={"Content-Type": "application/json"},
)
try:
with urllib.request.urlopen(req, timeout=20) as response:
data = json.load(response)
vulns = data.get("vulns", [])
print(f"{name} {version}: " + (
"none" if not vulns else
"; ".join(f"{v['id']}: {v.get('summary', '')}" for v in vulns)
))
except Exception as exc:
print(f"{name} {version}: QUERY_ERROR {exc}")
PYRepository: openshift-online/rosa-hyperfleet-api
Length of output: 3931
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json, urllib.request
deps = {
"k8s.io/apimachinery":"v0.36.0",
"k8s.io/api":"v0.36.0",
"golang.org/x/net":"v0.56.0",
"golang.org/x/text":"v0.38.0",
"github.com/openshift/api":"v0.0.0-20260416105050-3c6b218b8a80",
"github.com/openshift/hypershift/api":"v0.0.0-20260625052409-9acec4759a16",
}
for name, version in deps.items():
req = urllib.request.Request(
"https://api.osv.dev/v1/query",
data=json.dumps({"package":{"name":name,"ecosystem":"Go"},"version":version}).encode(),
headers={"Content-Type":"application/json"},
)
with urllib.request.urlopen(req, timeout=20) as r:
vulns = json.load(r).get("vulns", [])
print(name, version, "NONE" if not vulns else
"\n ".join([""] + [f"{v['id']}: {v.get('summary','')}" for v in vulns]))
PYRepository: openshift-online/rosa-hyperfleet-api
Length of output: 2461
🏁 Script executed:
#!/bin/bash
set -euo pipefail
while read -r name version; do
payload=$(printf '{"package":{"name":"%s","ecosystem":"Go"},"version":"%s"}' "$name" "$version")
result=$(curl -skS --max-time 20 -X POST https://api.osv.dev/v1/query \
-H 'Content-Type: application/json' -d "$payload")
ids=$(printf '%s' "$result" | jq -r '[.vulns[]?.id] | join(", ")')
if [ -n "$ids" ]; then
printf '%s %s: %s\n' "$name" "$version" "$ids"
else
printf '%s %s: NONE\n' "$name" "$version"
fi
done <<'DEPS'
github.com/openshift/api v0.0.0-20260416105050-3c6b218b8a80
github.com/openshift/hypershift/api v0.0.0-20260625052409-9acec4759a16
k8s.io/api v0.36.0
k8s.io/apimachinery v0.36.0
github.com/fxamacker/cbor/v2 v2.9.2
github.com/go-logr/logr v1.4.4
github.com/json-iterator/go v1.1.12
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd
github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee
github.com/x448/float16 v0.8.4
go.yaml.in/yaml/v2 v2.4.4
golang.org/x/net v0.56.0
golang.org/x/text v0.38.0
gopkg.in/inf.v0 v0.9.1
k8s.io/klog/v2 v2.140.0
k8s.io/kube-openapi v0.0.0-20260317180543-43fb72c5454a
k8s.io/utils v0.0.0-20260707023825-cf1189d6abe3
sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730
sigs.k8s.io/randfill v1.0.0
sigs.k8s.io/structured-merge-diff/v6 v6.3.2
DEPSRepository: openshift-online/rosa-hyperfleet-api
Length of output: 1099
🏁 Script executed:
#!/bin/bash
set -euo pipefail
while read -r name version; do
payload=$(printf '{"package":{"name":"%s","ecosystem":"Go"},"version":"%s"}' "$name" "$version")
curl -skS --max-time 20 -X POST https://api.osv.dev/v1/query \
-H 'Content-Type: application/json' -d "$payload" |
jq -r --arg n "$name" --arg v "$version" '
if (.vulns // []) | length == 0
then "\($n) \($v): NONE"
else "\($n) \($v): " + ([.vulns[].id] | join(", "))
end'
done <<'DEPS'
github.com/openshift/api v0.0.0-20260416105050-3c6b218b8a80
github.com/openshift/hypershift/api v0.0.0-20260625052409-9acec4759a16
k8s.io/api v0.36.0
k8s.io/apimachinery v0.36.0
github.com/fxamacker/cbor/v2 v2.9.2
github.com/go-logr/logr v1.4.4
github.com/json-iterator/go v1.1.12
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd
github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee
github.com/x448/float16 v0.8.4
go.yaml.in/yaml/v2 v2.4.4
golang.org/x/net v0.56.0
golang.org/x/text v0.38.0
gopkg.in/inf.v0 v0.9.1
k8s.io/klog/v2 v2.140.0
k8s.io/kube-openapi v0.0.0-20260317180543-43fb72c5454a
k8s.io/utils v0.0.0-20260707023825-cf1189d6abe3
sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730
sigs.k8s.io/randfill v1.0.0
sigs.k8s.io/structured-merge-diff/v6 v6.3.2
DEPSRepository: openshift-online/rosa-hyperfleet-api
Length of output: 1099
🏁 Script executed:
#!/bin/bash
set -euo pipefail
curl -skS --max-time 20 https://api.osv.dev/v1/vulns/GO-2026-5970 |
jq '{
id,
summary,
details,
published,
modified,
aliases,
affected: [
.affected[] |
{
package: .package,
ranges: .ranges,
versions: .versions,
database_specific
}
],
references
}'Repository: openshift-online/rosa-hyperfleet-api
Length of output: 1203
🏁 Script executed:
#!/bin/bash
set -euo pipefail
curl -skS --max-time 20 https://api.osv.dev/v1/vulns/GO-2026-5970 |
jq '{id, summary, details, published, modified, aliases,
affected: [.affected[] | {package, ranges, versions, database_specific}],
references}'Repository: openshift-online/rosa-hyperfleet-api
Length of output: 1203
Dependency Vulnerability (CWE-835): Loop with Unreachable Exit Condition ('Infinite Loop')
Reachability path
● Entry
api/public/v2alpha1/hostedclusterspec.passthrough.go:1
github.com/openshift/api: Code generated by passthrough-gen. DO NOT EDIT.
│
▼
● Sink
api/public/v2alpha1/go.mod
Update golang.org/x/text to v0.39.0 or later.
Version v0.38.0 is affected by GO-2026-5970 (CVE-2026-56852), which can cause an infinite loop when norm.Iter processes invalid UTF-8 input.
🤖 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/go.mod` around lines 5 - 29, Update the golang.org/x/text
dependency in the go.mod require block to version v0.39.0 or later, replacing
v0.38.0 while preserving the indirect dependency declaration.
Source: Path instructions
| // autoNode specifies the configuration for automatic node provisioning and lifecycle management. | ||
| // +k8s:openapi-gen=true | ||
| // +hyperfleet:write-mode=service-set | ||
| AutoNode hypershiftv1beta1.AutoNode `json:"autoNode,omitzero"` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Service-set fields become required in the public request schema.
AutoNode and FIPS are both +hyperfleet:write-mode=service-set, so a customer must never be obliged to send them. The generator still emits them as required, because FIPS bool has no omitempty and the generator does not treat omitzero as optional. The downstream effect is visible in platform-api/openapi/openapi.yaml lines 3006-3008, where autoNode and fips appear under required. oasdiff reports this as a breaking change on POST /clusters, PUT /clusters/{id}, and PATCH /clusters/{id}: every existing client body now fails schema validation.
Fix the source markers rather than editing the generated YAML. Either mark these fields optional, or exclude service-set fields from the required list in the generator.
🐛 Proposed marker change in the passthrough generator input
// autoNode specifies the configuration for automatic node provisioning and lifecycle management.
// +k8s:openapi-gen=true
// +hyperfleet:write-mode=service-set
+ // +optional
AutoNode hypershiftv1beta1.AutoNode `json:"autoNode,omitzero"` // fips indicates whether this cluster's nodes will be running in FIPS mode.
// +k8s:openapi-gen=true
// +hyperfleet:write-mode=service-set
- FIPS bool `json:"fips"`
+ // +optional
+ FIPS *bool `json:"fips,omitempty"`Also applies to: 118-121
🤖 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/hostedclusterspec.passthrough.go` around lines 66 - 69,
Update the passthrough generator inputs for AutoNode and FIPS so both
service-set fields are emitted as optional in the public request schema. Replace
the unsupported optionality representation with the generator-recognized
optional marker or JSON omission tag, rather than editing generated OpenAPI
output. Verify they are removed from required lists for all affected cluster
request operations.
Source: Linters/SAST tools
| // NodePool is the full HyperShift NodePoolSpec. | ||
| // +kubebuilder:validation:Required | ||
| NodePool hypershiftv1beta1.NodePoolSpec `json:"nodePool"` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
nodePool is required but resolves to a schema with no properties.
Every field of NodePoolSpecPassthrough in api/public/v2alpha1/hostedclusterspec.passthrough.go lines 149-210 carries +k8s:openapi-gen=false. The generator therefore filters all of them out, and platform-api/openapi/openapi.yaml lines 3010-3013 emit NodePoolSpecPassthrough as an empty object with additionalProperties: true. A customer must send nodePool, but the published schema documents no field inside it and validates nothing.
Expose at least the customer-settable node pool fields, such as replicas, platform, and release, by setting +k8s:openapi-gen=true on them in the passthrough generator input.
🤖 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 66 - 68, Update
NodePoolSpecPassthrough in hostedclusterspec.passthrough.go to mark
customer-settable fields such as replicas, platform, and release with
+k8s:openapi-gen=true instead of excluding them, so the generated
NodePoolSpecPassthrough schema exposes those properties while preserving
nodePool as required.
| raw, ok := genDoc.Definitions[name] | ||
| if !ok { | ||
| log.Printf("warning: schema %q not found in generated output, skipping", name) | ||
| continue |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail when a requested schema is unavailable.
At Lines 61-64, a missing requested definition only logs a warning. At Lines 78-80, insertion is counted as successful even when insertSchemaBlock returns the unchanged specification. If any other schema merges, verify-openapi can pass with a stale or missing required schema.
Stop on a missing generated definition. Make insertSchemaBlock report whether it found components.schemas. Add regression tests for both failure cases.
Proposed fix
raw, ok := genDoc.Definitions[name]
if !ok {
- log.Printf("warning: schema %q not found in generated output, skipping", name)
- continue
+ log.Fatalf("schema %q not found in generated output", name)
}
updated, found := replaceSchemaBlock(result, name, yamlBlock)
- if found {
- result = updated
- merged++
- log.Printf("replaced schema: %s", name)
- } else {
- result = insertSchemaBlock(result, name, yamlBlock)
- merged++
- log.Printf("inserted schema: %s", name)
+ if !found {
+ updated, found = insertSchemaBlock(result, name, yamlBlock)
+ if !found {
+ log.Fatalf("components.schemas not found while inserting %q", name)
+ }
}
+ result = updated
+ merged++-func insertSchemaBlock(spec []byte, schemaName string, replacement []byte) []byte {
+func insertSchemaBlock(spec []byte, schemaName string, replacement []byte) ([]byte, bool) {
// ...
if schemasStart < 0 {
- return spec
+ return spec, false
}
// ...
- return buf.Bytes()
+ return buf.Bytes(), true
}Also applies to: 78-80, 158-161
🤖 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/cmd/openapi-merge/main.go` around lines 61 - 64, Make the
schema merge fail when a requested definition is absent instead of warning and
continuing, and propagate that failure from the main merge flow. Update
insertSchemaBlock to report whether components.schemas was found and whether
insertion succeeded, then only count successful insertions when it returns
success. Add regression tests covering both missing generated definitions and
specifications without components.schemas.
| snapshot := cr.Spec | ||
|
|
||
| if err := hyperfleetdb.ApplyPlatformUpdateToClusterCR(cr, &req); err != nil { | ||
| h.logger.Error("failed to merge cluster spec", "error", err) | ||
| h.writeError(w, http.StatusBadRequest, "CLUSTERS-MGMT-UPDATE-002", "Invalid cluster spec") | ||
| return | ||
| } | ||
|
|
||
| cr.Spec.HostedCluster.IssuerURL = existingIssuerURL | ||
| // Restore service-set fields wiped by the full spec replacement. | ||
| cr.Spec.AccountID = snapshot.AccountID | ||
| cr.Spec.InternalID = snapshot.InternalID | ||
| cr.Spec.CreatorARN = snapshot.CreatorARN | ||
| cr.Spec.HostedCluster.IssuerURL = snapshot.HostedCluster.IssuerURL | ||
| if cr.Spec.ExpirationTimestamp == nil { | ||
| cr.Spec.ExpirationTimestamp = existingExpiration | ||
| cr.Spec.ExpirationTimestamp = snapshot.ExpirationTimestamp | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Full spec replacement erases service-managed fields in both update handlers. ApplyPlatformUpdateToClusterCR and ApplyPlatformUpdateToNodePoolCR both assign cr.Spec = *req.Spec, so every field the customer omits becomes the zero value. Each handler then restores a short hand-written subset. ValidateUpdate cannot detect the loss, because an omitted field produces no flattened path. Hidden ServiceSet fields are never present in a customer request by design, so they are always wiped.
Fix the root cause once: derive the preserved set from registry.FieldRegistry, or merge only customer-writable fields onto the existing spec instead of replacing it.
platform-api/pkg/handlers/cluster.go#L231-L246: this block restores five fields. Also preserve the remainingServiceSetcluster fields, includingspec.hostedCluster.clusterID,infraID,pullSecret,sshKey,secretEncryption,serviceAccountSigningKey,release,controlPlaneRelease,platform,networking,dns, andservices.platform-api/pkg/handlers/nodepool.go#L198-L208: this block restoresAccountIDandInternalPoolIDonly. Also preservespec.nodePool.release,platform,management,replicas,taints,nodeLabels,tuningConfig,config, andosImageStream.
📍 Affects 2 files
platform-api/pkg/handlers/cluster.go#L231-L246(this comment)platform-api/pkg/handlers/nodepool.go#L198-L208
🤖 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 231 - 246, Fix the root
cause in ApplyPlatformUpdateToClusterCR and ApplyPlatformUpdateToNodePoolCR by
merging only customer-writable fields or deriving preserved fields from
registry.FieldRegistry instead of replacing the full spec; this must retain
every ServiceSet field. In platform-api/pkg/handlers/cluster.go lines 231-246,
update the restoration/merge to preserve the listed hostedCluster and cluster
service-managed fields. In platform-api/pkg/handlers/nodepool.go lines 198-208,
likewise preserve the listed nodePool ServiceSet fields alongside AccountID and
InternalPoolID.
| switch effectiveMode { | ||
| case registry.ServiceSet: | ||
| return &ValidationError{ | ||
| Field: fieldPath, | ||
| Reason: "field is platform-managed and cannot be set by customers", | ||
| } | ||
| case registry.Immutable: | ||
| if op == OperationUpdate && existingFields != nil { | ||
| oldVal, existsInOld := existingFields[fieldPath] | ||
| if existsInOld { | ||
| newVal := fields[fieldPath] | ||
| if !reflect.DeepEqual(oldVal, newVal) { | ||
| return &ValidationError{ | ||
| Field: fieldPath, | ||
| Reason: "field is immutable and cannot be changed after creation", | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
| case registry.Mutable: | ||
| return nil | ||
| default: | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm that non-hidden ServiceSet container paths exist in the registry and that Mutable leaves sit beneath them.
rg -n -A3 '"spec\.hostedCluster\.configuration(\.kubelet|\.machineConfig)?": \{' hack/api-codegen/pkg/registry/field_metadata.go
# Confirm the update contract replaces the whole spec.
rg -n -C4 'ApplyPlatformUpdateTo(Cluster|NodePool)CR' platform-api/pkg/clients/hyperfleetdb/convert.go
# Check whether GET responses expose those non-hidden ServiceSet fields to clients.
rg -n -C4 'ClusterCRToPlatform' platform-api/pkg/clients/hyperfleetdb/convert.goRepository: openshift-online/rosa-hyperfleet-api
Length of output: 2040
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- validator symbols ---'
ast-grep outline platform-api/pkg/validation/field_validator.go
printf '%s\n' '--- validator implementation ---'
sed -n '1,230p' platform-api/pkg/validation/field_validator.go
printf '%s\n' '--- registry entries ---'
sed -n '330,420p' hack/api-codegen/pkg/registry/field_metadata.go
sed -n '470,515p' hack/api-codegen/pkg/registry/field_metadata.go
printf '%s\n' '--- conversion implementation ---'
sed -n '1,115p' platform-api/pkg/clients/hyperfleetdb/convert.go
printf '%s\n' '--- relevant tests and call sites ---'
rg -n -C4 'Validate|flattenMap|ServiceSet|ApplyPlatformUpdateToClusterCR|ClusterCRToPlatform' platform-api hack/api-codegen --glob '*_test.go' --glob '*.go'Repository: openshift-online/rosa-hyperfleet-api
Length of output: 50393
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
import json
import re
from pathlib import Path
validator = Path("platform-api/pkg/validation/field_validator.go").read_text()
registry_text = Path("hack/api-codegen/pkg/registry/field_metadata.go").read_text()
convert = Path("platform-api/pkg/clients/hyperfleetdb/convert.go").read_text()
def registry_modes(text):
out = {}
pattern = re.compile(
r'"([^"]+)":\s*\{\s*'
r'FieldPath:\s*"[^"]+",\s*'
r'(?:(?:WriteMode:\s*(\w+))|(?:FieldPath:.*?\n\s*WriteMode:\s*(\w+)))',
re.S,
)
for match in pattern.finditer(text):
out[match.group(1)] = match.group(2) or match.group(3)
return out
modes = registry_modes(registry_text)
targets = [
"spec.hostedCluster.configuration",
"spec.hostedCluster.configuration.kubelet",
"spec.hostedCluster.configuration.kubelet.containerLogMaxFiles",
"spec.hostedCluster.configuration.kubelet.containerLogMaxSize",
"spec.hostedCluster.configuration.machineConfig",
"spec.hostedCluster.configuration.machineConfig.allowedKernelArguments",
"spec.hostedCluster.channel",
"spec.hostedCluster.fips",
"spec.hostedCluster.pausedUntil",
]
print("--- source facts ---")
print("flatten records container before recursion:", "result[path] = val" in validator and "flattenMap(path, nested, result)" in validator)
print("update replaces full spec:", "cr.Spec = *req.Spec" in convert)
print("GET conversion copies full spec:", "Spec: cr.Spec" in convert)
print("--- registry modes ---")
for target in targets:
print(target, modes.get(target, "<not found>"))
def flatten(value, prefix="spec", result=None):
if result is None:
result = {}
for key, val in value.items():
path = f"{prefix}.{key}" if prefix else key
result[path] = val
if isinstance(val, dict):
flatten(val, path, result)
return result
def current_service_set_errors(new, old=None):
new_fields = flatten(new)
old_fields = flatten(old) if old is not None else None
errors = []
for path, value in new_fields.items():
if modes.get(path) != "ServiceSet":
continue
if old_fields is not None and path in old_fields and old_fields[path] == value:
continue
errors.append(path)
return errors
print("--- current implementation behavior for representative inputs ---")
create = {"hostedCluster": {"configuration": {"kubelet": {"containerLogMaxFiles": 5}}}}
print("create mutable descendant errors:", [p for p in flatten(create) if modes.get(p) == "ServiceSet"])
existing = {"hostedCluster": {"channel": "stable", "fips": False, "pausedUntil": None}}
print("update unchanged visible ServiceSet errors:", current_service_set_errors(existing, existing))
print("update changed visible ServiceSet errors:", current_service_set_errors({"hostedCluster": {"channel": "fast"}}, existing))
PY
printf '%s\n' '--- exact registry target lines ---'
rg -n -A4 -B1 'configuration\.kubelet\.(containerLogMaxFiles|containerLogMaxSize)|configuration\.machineConfig\.allowedKernelArguments|hostedCluster\.(fips|pausedUntil)|hostedCluster\.channel' hack/api-codegen/pkg/registry/field_metadata.goRepository: openshift-online/rosa-hyperfleet-api
Length of output: 2705
Make ServiceSet validation value-aware.
flattenMap records container paths. The registry marks spec.hostedCluster.configuration, ...kubelet, and ...machineConfig as ServiceSet. Requests that set permitted descendants can therefore fail with "field is platform-managed".
ApplyPlatformUpdateToClusterCR replaces the full spec, and ClusterCRToPlatform returns the full spec. A read-modify-write request can resend unchanged visible ServiceSet fields and fail validation.
On create, reject a ServiceSet field only when its value is non-null. On update, reject it only when its value differs from the existing value, including clearing. Skip an object-valued container check only when a registered descendant validates its contents.
🤖 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 131 - 156,
Update the ServiceSet branch in the field-validation function to compare values:
on create reject only non-null values, and on update reject only values that
differ from the existing field, including clearing. For object-valued container
paths, skip the container-level rejection only when a registered descendant is
being validated; preserve descendant validation and leave Immutable/Mutable
behavior unchanged.
| func flattenToFieldPaths(v any) map[string]any { | ||
| data, err := json.Marshal(v) | ||
| if err != nil { | ||
| return nil | ||
| } | ||
|
|
||
| var m map[string]any | ||
| if err := json.Unmarshal(data, &m); err != nil { | ||
| return nil | ||
| } | ||
|
|
||
| result := make(map[string]any) | ||
| flattenMap("spec", m, result) | ||
| return result | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
flattenToFieldPaths fails open on a serialization error.
If json.Marshal or json.Unmarshal returns an error, the function returns nil. validate then iterates an empty map and reports no errors, so the request skips all write-mode checks. Validation must not pass silently when it cannot inspect the input.
Return an error, or return a sentinel that ValidateCreate and ValidateUpdate convert into a validation failure.
🛡️ Proposed direction
-func flattenToFieldPaths(v any) map[string]any {
+func flattenToFieldPaths(v any) (map[string]any, error) {
data, err := json.Marshal(v)
if err != nil {
- return nil
+ return nil, fmt.Errorf("marshaling spec: %w", err)
}
var m map[string]any
if err := json.Unmarshal(data, &m); err != nil {
- return nil
+ return nil, fmt.Errorf("spec is not a JSON object: %w", err)
}
result := make(map[string]any)
flattenMap("spec", m, result)
- return result
+ return result, nil
}🤖 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 158 - 172,
Update flattenToFieldPaths to propagate serialization or deserialization
failures instead of returning nil, and update its callers, including validate
and the ValidateCreate/ValidateUpdate paths, to convert those failures into an
explicit validation error. Preserve normal field flattening for successful
conversions and ensure inspection failures never result in silent validation
success.
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>
Generate typed OpenAPI schemas from Go public API types using controller-tools, filter hidden fields via the codegen registry, and merge into the handwritten OpenAPI spec. Switch passthrough spec to use local ClusterConfiguration type with hidden/visible markers so nested sub-configs are properly filtered. - Add openapi-gen and openapi-merge codegen tools - Wire $ref chains for cluster and nodepool schemas - Consolidate field registry to single source in hack/api-codegen - Add generate-openapi, verify-openapi, and swagger-ui make targets - Add regression test for nested configuration marker paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4c62450 to
7e36ead
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.
Actionable comments posted: 3
🤖 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 `@Makefile`:
- Line 383: Update the volume mount in the OpenAPI-related Makefile target to
use $(abspath $(OPENAPI_SPEC)) instead of prefixing OPENAPI_SPEC with $(CURDIR),
so both absolute and relative configured paths resolve to the intended file.
In `@platform-api/openapi/openapi.yaml`:
- Around line 2286-2306: Remove the newly added required constraints for
NodePoolSpec and the related hostedCluster, autoNode, and fips schema fields so
existing POST, PATCH, and PUT payloads remain valid. Preserve server-default
behavior for omitted fields, or only enforce them through a separately versioned
request contract with an established migration path.
In `@platform-api/pkg/validation/field_validator.go`:
- Around line 82-86: Update ValidateUpdate to compare immutable-field presence
in both old and new field sets, rejecting additions and removals as well as
changed values; do not skip registry entries absent from either set. Add
regression tests covering an immutable field being added and an existing
immutable field being omitted, while preserving current value-change validation.
🪄 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: 75dc6034-4a6b-449b-bdde-156fc9844beb
⛔ 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 (35)
.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/cmd/openapi-merge/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.gohack/api-codegen/pkg/openapi/generator.gohack/api-codegen/pkg/openapi/generator_test.gohack/api-codegen/pkg/registry/field_metadata.gohack/api-codegen/pkg/registry/field_metadata.jsonhyperfleet-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/openapi/openapi.yamlplatform-api/pkg/clients/hyperfleetdb/convert.goplatform-api/pkg/clients/hyperfleetdb/convert_test.goplatform-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 (29)
- hyperfleet-operator/api/v1alpha1/cluster_types.go
- .gitignore
- hack/api-codegen/go.mod
- hack/api-codegen/pkg/registry/field_metadata.json
- hack/api-codegen/cmd/marker-scanner/main.go
- platform-api/internal/codegen/featuregate/registry.go
- api/public/v2alpha1/go.mod
- hack/api-codegen/pkg/markers/gated_writemode_test.go
- hyperfleet-operator/config/crd/bases/hyperfleet.io_clusters.yaml
- hack/api-codegen/pkg/markers/types.go
- platform-api/Containerfile
- platform-api/internal/codegen/conversion/cluster.go
- hyperfleet-operator/api/v1alpha1/nodepool_types.go
- platform-api/internal/codegen/featuregate/types.go
- platform-api/pkg/clients/hyperfleetdb/convert.go
- hack/api-codegen/pkg/openapi/generator.go
- platform-api/go.mod
- hack/api-codegen/cmd/openapi-merge/main.go
- api/public/v2alpha1/hostedclusterspec.passthrough.go
- platform-api/pkg/handlers/cluster.go
- api/public/v2alpha1/cluster_types.go
- api/public/v2alpha1/configuration.go
- hack/api-codegen/pkg/markers/scanner.go
- hyperfleet-operator/config/crd/bases/hyperfleet.io_nodepools.yaml
- platform-api/pkg/handlers/nodepool.go
- hack/api-codegen/pkg/registry/field_metadata.go
- hack/api-codegen/pkg/openapi/generator_test.go
- hack/api-codegen/pkg/markers/scanner_test.go
- platform-api/pkg/clients/hyperfleetdb/convert_test.go
| @echo "Swagger UI available at http://localhost:$(SWAGGER_UI_PORT)" | ||
| $(CONTAINER_ENGINE) run --rm -p $(SWAGGER_UI_PORT):8080 \ | ||
| -e SWAGGER_JSON=/spec/openapi.yaml \ | ||
| -v $(CURDIR)/$(OPENAPI_SPEC):/spec/openapi.yaml:ro \ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Mount the configured OpenAPI file path.
Line 383 prefixes OPENAPI_SPEC with $(CURDIR). If OPENAPI_SPEC is absolute, the target mounts a repository-relative path instead of the configured file. Use $(abspath $(OPENAPI_SPEC)).
Proposed fix
- -v $(CURDIR)/$(OPENAPI_SPEC):/spec/openapi.yaml:ro \
+ -v $(abspath $(OPENAPI_SPEC)):/spec/openapi.yaml:ro \📝 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.
| -v $(CURDIR)/$(OPENAPI_SPEC):/spec/openapi.yaml:ro \ | |
| -v $(abspath $(OPENAPI_SPEC)):/spec/openapi.yaml:ro \ |
🤖 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 `@Makefile` at line 383, Update the volume mount in the OpenAPI-related
Makefile target to use $(abspath $(OPENAPI_SPEC)) instead of prefixing
OPENAPI_SPEC with $(CURDIR), so both absolute and relative configured paths
resolve to the intended file.
| - hostedCluster | ||
| type: object | ||
| description: | | ||
| NodePool specification following the hyperfleet-operator v1alpha1.NodePoolSpec | ||
| type. Contains a nested `nodePool` field that follows the HyperShift v1beta1 | ||
| NodePoolSpec schema. | ||
| NodePoolSpec: | ||
| description: NodePoolSpec defines the desired state of a NodePool. | ||
| properties: | ||
| nodePool: | ||
| type: object | ||
| description: | | ||
| HyperShift v1beta1 NodePoolSpec. Key fields include: | ||
| - platform: Cloud provider configuration (type, aws with instanceType, rootVolume) | ||
| - replicas: Number of worker nodes (default: 2) | ||
| - release: OpenShift release image | ||
| - management: Upgrade and repair configuration | ||
| additionalProperties: true | ||
|
|
||
| autoRepair: | ||
| description: AutoRepair enables automatic repair of unhealthy nodes. | ||
| type: boolean | ||
| displayName: | ||
| description: DisplayName is a human-readable name for the node pool. | ||
| maxLength: 256 | ||
| type: string | ||
| labels: | ||
| additionalProperties: | ||
| type: string | ||
| description: Labels are customer-defined labels applied to nodes. | ||
| type: object | ||
| nodePool: | ||
| $ref: '#/components/schemas/NodePoolSpecPassthrough' | ||
| required: | ||
| - nodePool |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve compatibility for existing request payloads.
These required entries make spec.hostedCluster, spec.nodePool, spec.hostedCluster.autoNode, and spec.hostedCluster.fips mandatory on existing endpoints. Existing POST, PATCH, and PUT clients can no longer submit payloads that were valid before this change.
Preserve the prior acceptance behavior with server defaults, or publish a versioned request contract and migration path before requiring these fields.
Also applies to: 3006-3008
🧰 Tools
🪛 oasdiff (1.27.0)
[error] 2286-2286: the request property spec/hostedCluster became required (POST /clusters, section: paths, fingerprint: e9b5374154a3)
(request-property-became-required)
[error] 2286-2286: the request property spec/hostedCluster became required (PATCH /clusters/{id}, section: paths, fingerprint: 6caa8081bdf1)
(request-property-became-required)
[error] 2286-2286: the request property spec/hostedCluster became required (PUT /clusters/{id}, section: paths, fingerprint: c3077119ca0e)
(request-property-became-required)
[error] 2306-2306: the request property spec/nodePool became required (POST /nodepools, section: paths, fingerprint: b8a7d7fc1a0b)
(request-property-became-required)
[error] 2306-2306: the request property spec/nodePool became required (PUT /nodepools/{id}, section: paths, fingerprint: 56ca69e5f5c6)
(request-property-became-required)
🤖 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/openapi/openapi.yaml` around lines 2286 - 2306, Remove the newly
added required constraints for NodePoolSpec and the related hostedCluster,
autoNode, and fips schema fields so existing POST, PATCH, and PUT payloads
remain valid. Preserve server-default behavior for omitted fields, or only
enforce them through a separately versioned request contract with an established
migration path.
Source: Linters/SAST tools
| for fieldPath := range fields { | ||
| meta, exists := v.registry[fieldPath] | ||
| if !exists { | ||
| continue | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect the update request and full-spec replacement contract.
rg -n -P -C 5 --type go \
'type\s+Cluster(Update|Create)Request\b|ValidateUpdate\s*\(|ApplyPlatformUpdateToClusterCR\s*\(' .
# Inspect JSON omission behavior for request and specification types.
rg -n -C 3 --type go 'json:".*omitempty|type\s+.*Spec\b' platform-api api
# Inspect immutable validation coverage.
rg -n -C 5 --type go 'TestValidateUpdate|Immutable' platform-api/pkg/validationRepository: openshift-online/rosa-hyperfleet-api
Length of output: 50392
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- validator ---'
cat -n platform-api/pkg/validation/field_validator.go | sed -n '1,210p'
printf '%s\n' '--- validator tests ---'
cat -n platform-api/pkg/validation/field_validator_test.go | sed -n '1,235p'
printf '%s\n' '--- cluster handler update ---'
cat -n platform-api/pkg/handlers/cluster.go | sed -n '180,255p'
printf '%s\n' '--- conversion ---'
cat -n platform-api/pkg/clients/hyperfleetdb/convert.go | sed -n '75,110p'
printf '%s\n' '--- request types ---'
cat -n platform-api/pkg/types/cluster.go | sed -n '20,40p'
printf '%s\n' '--- immutable markers ---'
rg -n -C 2 --type go '\+hyperfleet:write-mode=immutable' api platform-api/pkgRepository: openshift-online/rosa-hyperfleet-api
Length of output: 22565
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import json
import re
from pathlib import Path
validator = Path("platform-api/pkg/validation/field_validator.go").read_text()
print("--- validator implementation ---")
for start, end in [(65, 120), (120, 180)]:
lines = validator.splitlines()
for i in range(start - 1, min(end, len(lines))):
print(f"{i+1:4}: {lines[i]}")
print("--- relevant immutable fields ---")
for path in Path("api").rglob("*.go"):
text = path.read_text()
lines = text.splitlines()
for i, line in enumerate(lines):
if "+hyperfleet:write-mode=immutable" in line:
print(f"{path}:{i+1}: {line.strip()}")
for j in range(i + 1, min(i + 4, len(lines))):
print(f"{path}:{j+1}: {lines[j].strip()}")
print("--- request and update call sites ---")
for path in [Path("platform-api/pkg/types/cluster.go"),
Path("platform-api/pkg/handlers/cluster.go"),
Path("platform-api/pkg/clients/hyperfleetdb/convert.go")]:
lines = path.read_text().splitlines()
for i, line in enumerate(lines):
if any(term in line for term in [
"ClusterUpdateRequest", "ValidateUpdate",
"ApplyPlatformUpdateToClusterCR", "Decode",
]):
lo, hi = max(0, i - 4), min(len(lines), i + 8)
print(f"--- {path}:{i+1} ---")
for j in range(lo, hi):
print(f"{j+1:4}: {lines[j]}")
PY
printf '%s\n' '--- focused tests ---'
rg -n -C 6 --type go 'fips|systemReserved|kubeReserved|allowedKernelArguments|immutable' \
platform-api/pkg/validation api/public/v2alpha1Repository: openshift-online/rosa-hyperfleet-api
Length of output: 27202
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- flattening semantics ---'
cat -n platform-api/pkg/validation/field_validator.go | sed -n '205,255p'
printf '%s\n' '--- generated registry entries for immutable paths ---'
rg -n -C 3 --type go 'systemReserved|kubeReserved|allowedKernelArguments|spec\.fips|hostedCluster.*fips|machineConfig' \
hack platform-api api | head -n 240
printf '%s\n' '--- deterministic presence/replacement probe ---'
python3 - <<'PY'
import json
def flatten(spec):
out = {}
def visit(prefix, value):
if isinstance(value, dict):
out[prefix] = value
for key, child in value.items():
visit(f"{prefix}.{key}", child)
elif isinstance(value, list):
out[prefix] = value
else:
out[prefix] = value
for key, value in spec.items():
visit(f"spec.{key}", value)
return out
def immutable_allowed(new_spec, old_spec, path):
new_fields = flatten(new_spec)
old_fields = flatten(old_spec) if old_spec is not None else None
if path not in new_fields:
return True, "new path is not visited"
if old_fields is None or path not in old_fields:
return True, "old path is absent"
return new_fields[path] == old_fields[path], "values are compared"
cases = [
(
"add immutable field",
{"machineConfig": {"fips": False}},
{"machineConfig": {}},
"spec.machineConfig.fips",
),
(
"omit existing immutable field",
{"machineConfig": {}},
{"machineConfig": {"fips": True}},
"spec.machineConfig.fips",
),
]
for name, new_spec, old_spec, path in cases:
allowed, reason = immutable_allowed(new_spec, old_spec, path)
replaced = new_spec
print(json.dumps({
"case": name,
"path": path,
"new_paths": sorted(flatten(new_spec)),
"old_paths": sorted(flatten(old_spec)),
"validator_allows": allowed,
"reason": reason,
"replacement_spec": replaced,
}, sort_keys=True))
PYRepository: openshift-online/rosa-hyperfleet-api
Length of output: 20222
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
cat -n platform-api/pkg/validation/field_validator.go | sed -n '205,255p'
python3 - <<'PY'
import json
cases = [
("add", {"machineConfig": {"fips": False}}, {"machineConfig": {}}),
("remove", {"machineConfig": {}}, {"machineConfig": {"fips": True}}),
]
for name, new_spec, old_spec in cases:
def paths(obj, prefix="spec"):
result = {prefix}
for key, value in obj.items():
path = f"{prefix}.{key}"
result.add(path)
if isinstance(value, dict):
result |= paths(value, path)
return result
new_paths = paths(new_spec)
old_paths = paths(old_spec)
field = "spec.machineConfig.fips"
visits_field = field in new_paths
compares_old = visits_field and field in old_paths
print(json.dumps({
"case": name,
"new_paths": sorted(new_paths),
"old_paths": sorted(old_paths),
"validator_visits_immutable_field": visits_field,
"validator_compares_old_value": compares_old,
"full_replacement_result": new_spec,
}, sort_keys=True))
PYRepository: openshift-online/rosa-hyperfleet-api
Length of output: 1142
Reject immutable-field additions and removals during updates.
ValidateUpdate checks only paths in newFields and compares values only when the old path exists. Therefore, an update can add an absent immutable field or omit an existing immutable field. ApplyPlatformUpdateToClusterCR then replaces the full specification. Validate both presence changes and value changes, and add regression tests for both cases.
🤖 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 82 - 86, Update
ValidateUpdate to compare immutable-field presence in both old and new field
sets, rejecting additions and removals as well as changed values; do not skip
registry entries absent from either set. Add regression tests covering an
immutable field being added and an existing immutable field being omitted, while
preserving current value-change validation.
2 participants
Description
Type of Change
Testing
make test)Checklist
Summary by CodeRabbit
New Features
Bug Fixes