ROSAENG-64580: feat: move api types from ./hyperfleet-operator/api to ./api - #283
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@cdoan1: This pull request references ROSAENG-64580 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. |
openshift-ci
Bot
added
the
approved
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe standalone CRD API module moves from ChangesAPI module relocation and integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 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 |
|
/test on-demand-e2e |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
Makefile (1)
292-313: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit
generate-clientsetinto helper targets.
generate-clientsethas 21 recipe lines. This exceeds the configured checkmake maximum of five lines. Extract the client generation and each Wire mode into separate phony targets. Keepgenerate-clientsetas their aggregate target.🤖 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` around lines 292 - 313, Split the generate-clientset recipe into separate phony helper targets for client generation, Wire mappings, and Wire wrappers, moving each command into its corresponding target. Keep generate-clientset as an aggregate target that depends on all three helpers, and ensure the helper targets preserve their existing command arguments and dependency behavior.Source: Linters/SAST tools
🤖 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 @.tekton/rosa-hyperfleet-operator-pull-request.yaml:
- Line 38: Add the api/*** pathChanged condition to the operator trigger
expression in .tekton/rosa-hyperfleet-operator-pull-request.yaml at lines 38-38,
and make the same change in .tekton/rosa-hyperfleet-operator-push.yaml at lines
35-35, preserving the existing conditions.
In `@api/go.mod`:
- Line 1: Update the import example in clientset/docs/architecture.md to replace
the stale hyperfleet-operator path with
github.com/openshift-online/rosa-hyperfleet-api/api/v1alpha1, matching the
module path declared in go.mod.
In `@api/v1alpha1/manifest_types.go`:
- Around line 41-44: In the manifest reconciliation flow around
extractResourceMeta and applyItems, validate that each resource’s derived target
identity (group, version, resource, namespace, and name) is unique before
appending applyItems or launching concurrent upserts. Reject the manifest with a
clear validation error on duplicates, and ensure duplicate entries do not
contribute to AppliedResources or watched-resource ReadDesire IDs.
- Around line 53-56: Add validation for Resource in ResourceTemplate so only
safe Kubernetes resource-name path segments are accepted before values reach
TargetItem.Resource. Prefer an allow-list or kubebuilder Pattern that rejects
slash, dot-segment, query, and fragment delimiters while preserving the existing
MinLength constraint.
In `@api/v1alpha1/placement_types.go`:
- Around line 40-42: Make PlacementSpec.ManagementCluster immutable after
creation, matching the existing immutability constraint used by ClusterName. Add
the appropriate CEL validation or equivalent schema rule on ManagementCluster so
creation remains allowed but subsequent patch/update attempts that change its
value are rejected.
---
Nitpick comments:
In `@Makefile`:
- Around line 292-313: Split the generate-clientset recipe into separate phony
helper targets for client generation, Wire mappings, and Wire wrappers, moving
each command into its corresponding target. Keep generate-clientset as an
aggregate target that depends on all three helpers, and ensure the helper
targets preserve their existing command arguments and dependency behavior.
🪄 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: 87d6362e-d965-4023-ae53-29a03950e786
⛔ Files ignored due to path filters (8)
api/go.sumis excluded by!**/*.sumapi/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*clientset/generated/fake/register.gois excluded by!**/generated/**clientset/generated/scheme/register.gois excluded by!**/generated/**clientset/generated/typed/v1alpha1/internalversion/cluster.gois excluded by!**/generated/**clientset/generated/typed/v1alpha1/internalversion/fake/fake_cluster.gois excluded by!**/generated/**clientset/generated/typed/v1alpha1/internalversion/fake/fake_nodepool.gois excluded by!**/generated/**clientset/generated/typed/v1alpha1/internalversion/nodepool.gois excluded by!**/generated/**
📒 Files selected for processing (56)
.tekton/rosa-hyperfleet-api-pull-request.yaml.tekton/rosa-hyperfleet-api-push.yaml.tekton/rosa-hyperfleet-operator-pull-request.yaml.tekton/rosa-hyperfleet-operator-push.yamlCLAUDE.mdMakefileREADME.mdapi/go.modapi/v1alpha1/cluster_types.goapi/v1alpha1/groupversion_info.goapi/v1alpha1/install/install.goapi/v1alpha1/managementcluster_types.goapi/v1alpha1/manifest_types.goapi/v1alpha1/nodepool_types.goapi/v1alpha1/placement_types.goclientset/go.modclientset/wrappers/wire_wrappers_generated.goclientset/wrappers/wrappers_test.gohyperfleet-operator/Containerfilehyperfleet-operator/PROJECThyperfleet-operator/cmd/manager/main.gohyperfleet-operator/go.modhyperfleet-operator/internal/controller/cluster_controller.gohyperfleet-operator/internal/controller/cluster_controller_test.gohyperfleet-operator/internal/controller/manifest_controller.gohyperfleet-operator/internal/controller/manifest_controller_test.gohyperfleet-operator/internal/controller/nodepool_controller.gohyperfleet-operator/internal/controller/nodepool_controller_test.gohyperfleet-operator/internal/controller/placement_controller.gohyperfleet-operator/internal/controller/placement_controller_test.gohyperfleet-operator/internal/controller/suite_test.gohyperfleet-operator/internal/dynamo/statusstream/manager.gohyperfleet-operator/internal/render/cluster.gohyperfleet-operator/internal/render/cluster_test.gohyperfleet-operator/internal/render/nodepool.gohyperfleet-operator/internal/render/nodepool_test.gohyperfleet-operator/test/cluster_test.gohyperfleet-operator/test/helpers_test.gohyperfleet-operator/test/manifest_test.gohyperfleet-operator/test/suite_test.goplatform-api/Containerfileplatform-api/go.modplatform-api/pkg/clients/hyperfleetdb/client.goplatform-api/pkg/clients/hyperfleetdb/client_test.goplatform-api/pkg/clients/hyperfleetdb/convert.goplatform-api/pkg/clients/hyperfleetdb/convert_test.goplatform-api/pkg/handlers/cluster_test.goplatform-api/pkg/handlers/management_cluster.goplatform-api/pkg/handlers/zoa_test.goplatform-api/pkg/types/cluster.goplatform-api/pkg/types/nodepool.goplatform-api/pkg/zoa/jobbuilder.goplatform-api/pkg/zoa/reconciler.goplatform-api/pkg/zoa/reconciler_test.gotest/e2e-sdk/sdk_sanity_test.gotest/go.mod
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
🧹 Nitpick comments (1)
Makefile (1)
292-313: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit
generate-clientsetinto helper targets.
generate-clientsethas 21 recipe lines. This exceeds the configured checkmake maximum of five lines. Extract the client generation and each Wire mode into separate phony targets. Keepgenerate-clientsetas their aggregate target.🤖 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` around lines 292 - 313, Split the generate-clientset recipe into separate phony helper targets for client generation, Wire mappings, and Wire wrappers, moving each command into its corresponding target. Keep generate-clientset as an aggregate target that depends on all three helpers, and ensure the helper targets preserve their existing command arguments and dependency behavior.Source: Linters/SAST tools
🤖 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 @.tekton/rosa-hyperfleet-operator-pull-request.yaml:
- Line 38: Add the api/*** pathChanged condition to the operator trigger
expression in .tekton/rosa-hyperfleet-operator-pull-request.yaml at lines 38-38,
and make the same change in .tekton/rosa-hyperfleet-operator-push.yaml at lines
35-35, preserving the existing conditions.
In `@api/go.mod`:
- Line 1: Update the import example in clientset/docs/architecture.md to replace
the stale hyperfleet-operator path with
github.com/openshift-online/rosa-hyperfleet-api/api/v1alpha1, matching the
module path declared in go.mod.
In `@api/v1alpha1/manifest_types.go`:
- Around line 41-44: In the manifest reconciliation flow around
extractResourceMeta and applyItems, validate that each resource’s derived target
identity (group, version, resource, namespace, and name) is unique before
appending applyItems or launching concurrent upserts. Reject the manifest with a
clear validation error on duplicates, and ensure duplicate entries do not
contribute to AppliedResources or watched-resource ReadDesire IDs.
- Around line 53-56: Add validation for Resource in ResourceTemplate so only
safe Kubernetes resource-name path segments are accepted before values reach
TargetItem.Resource. Prefer an allow-list or kubebuilder Pattern that rejects
slash, dot-segment, query, and fragment delimiters while preserving the existing
MinLength constraint.
In `@api/v1alpha1/placement_types.go`:
- Around line 40-42: Make PlacementSpec.ManagementCluster immutable after
creation, matching the existing immutability constraint used by ClusterName. Add
the appropriate CEL validation or equivalent schema rule on ManagementCluster so
creation remains allowed but subsequent patch/update attempts that change its
value are rejected.
---
Nitpick comments:
In `@Makefile`:
- Around line 292-313: Split the generate-clientset recipe into separate phony
helper targets for client generation, Wire mappings, and Wire wrappers, moving
each command into its corresponding target. Keep generate-clientset as an
aggregate target that depends on all three helpers, and ensure the helper
targets preserve their existing command arguments and dependency behavior.
🪄 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: 87d6362e-d965-4023-ae53-29a03950e786
⛔ Files ignored due to path filters (8)
api/go.sumis excluded by!**/*.sumapi/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*clientset/generated/fake/register.gois excluded by!**/generated/**clientset/generated/scheme/register.gois excluded by!**/generated/**clientset/generated/typed/v1alpha1/internalversion/cluster.gois excluded by!**/generated/**clientset/generated/typed/v1alpha1/internalversion/fake/fake_cluster.gois excluded by!**/generated/**clientset/generated/typed/v1alpha1/internalversion/fake/fake_nodepool.gois excluded by!**/generated/**clientset/generated/typed/v1alpha1/internalversion/nodepool.gois excluded by!**/generated/**
📒 Files selected for processing (56)
.tekton/rosa-hyperfleet-api-pull-request.yaml.tekton/rosa-hyperfleet-api-push.yaml.tekton/rosa-hyperfleet-operator-pull-request.yaml.tekton/rosa-hyperfleet-operator-push.yamlCLAUDE.mdMakefileREADME.mdapi/go.modapi/v1alpha1/cluster_types.goapi/v1alpha1/groupversion_info.goapi/v1alpha1/install/install.goapi/v1alpha1/managementcluster_types.goapi/v1alpha1/manifest_types.goapi/v1alpha1/nodepool_types.goapi/v1alpha1/placement_types.goclientset/go.modclientset/wrappers/wire_wrappers_generated.goclientset/wrappers/wrappers_test.gohyperfleet-operator/Containerfilehyperfleet-operator/PROJECThyperfleet-operator/cmd/manager/main.gohyperfleet-operator/go.modhyperfleet-operator/internal/controller/cluster_controller.gohyperfleet-operator/internal/controller/cluster_controller_test.gohyperfleet-operator/internal/controller/manifest_controller.gohyperfleet-operator/internal/controller/manifest_controller_test.gohyperfleet-operator/internal/controller/nodepool_controller.gohyperfleet-operator/internal/controller/nodepool_controller_test.gohyperfleet-operator/internal/controller/placement_controller.gohyperfleet-operator/internal/controller/placement_controller_test.gohyperfleet-operator/internal/controller/suite_test.gohyperfleet-operator/internal/dynamo/statusstream/manager.gohyperfleet-operator/internal/render/cluster.gohyperfleet-operator/internal/render/cluster_test.gohyperfleet-operator/internal/render/nodepool.gohyperfleet-operator/internal/render/nodepool_test.gohyperfleet-operator/test/cluster_test.gohyperfleet-operator/test/helpers_test.gohyperfleet-operator/test/manifest_test.gohyperfleet-operator/test/suite_test.goplatform-api/Containerfileplatform-api/go.modplatform-api/pkg/clients/hyperfleetdb/client.goplatform-api/pkg/clients/hyperfleetdb/client_test.goplatform-api/pkg/clients/hyperfleetdb/convert.goplatform-api/pkg/clients/hyperfleetdb/convert_test.goplatform-api/pkg/handlers/cluster_test.goplatform-api/pkg/handlers/management_cluster.goplatform-api/pkg/handlers/zoa_test.goplatform-api/pkg/types/cluster.goplatform-api/pkg/types/nodepool.goplatform-api/pkg/zoa/jobbuilder.goplatform-api/pkg/zoa/reconciler.goplatform-api/pkg/zoa/reconciler_test.gotest/e2e-sdk/sdk_sanity_test.gotest/go.mod
🛑 Comments failed to post (3)
api/v1alpha1/manifest_types.go (2)
41-44: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject duplicate resource targets before concurrent upserts.
MinItems=1does not enforce uniqueness. Inhyperfleet-operator/internal/controller/manifest_controller.go, Line 106 through Line 133 derives adocIDfor each resource and starts concurrentUpsertApplyDesirecalls. Two entries with the same(group, version, resource, namespace, name)therefore race on the same key. The persisted content becomes nondeterministic, andAppliedResourcescounts duplicate entries.Reject duplicate targets after
extractResourceMetaand before appendingapplyItemsor starting the goroutines. This also prevents duplicateReadDesireIDs for watched resources.Proposed controller check
docID := dynamo.NewDocumentID(scopedTaskKey, group, version, res.Resource, namespace, name) +if _, exists := currentDocIDs[docID]; exists { + return ctrl.Result{}, fmt.Errorf("duplicate resource target %s/%s", res.Resource, name) +} currentDocIDs[docID] = struct{}{}🤖 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/v1alpha1/manifest_types.go` around lines 41 - 44, In the manifest reconciliation flow around extractResourceMeta and applyItems, validate that each resource’s derived target identity (group, version, resource, namespace, and name) is unique before appending applyItems or launching concurrent upserts. Reject the manifest with a clear validation error on duplicates, and ensure duplicate entries do not contribute to AppliedResources or watched-resource ReadDesire IDs.
53-56: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash set -euo pipefail # Expect: Resource is constrained to one Kubernetes resource segment before URL construction. rg -n -C 8 'TargetItem\.Resource|ResourceReference|NewDocumentID|Resource\(' --glob '*.go' .Repository: openshift-online/rosa-hyperfleet-api
Length of output: 50393
🏁 Script executed:
# Search for validation/canonicalization of Resource field rg -n 'Resource\s*=|resource\s*=' --glob '*.go' . | head -40Repository: openshift-online/rosa-hyperfleet-api
Length of output: 1165
🏁 Script executed:
# Look at manifest_controller.go to find where Resource is used rg -n -A 5 'TargetItem.*Resource' hyperfleet-operator/internal/controller/manifest_controller.goRepository: openshift-online/rosa-hyperfleet-api
Length of output: 759
🏁 Script executed:
# Check for CRD validation rules on Resource field rg -n -B 5 -A 5 'validation:' api/v1alpha1/manifest_types.go | grep -A 10 -B 10 'Resource'Repository: openshift-online/rosa-hyperfleet-api
Length of output: 1584
🏁 Script executed:
# Search for how Resource field is used after being assigned to TargetItem rg -n 'TargetItem' hyperfleet-operator/internal/controller/manifest_controller.go -A 20 | head -60Repository: openshift-online/rosa-hyperfleet-api
Length of output: 2212
🏁 Script executed:
# Look for pattern validation or enum constraints on Resource field rg -n 'pattern:|enum:' api/v1alpha1/manifest_types.goRepository: openshift-online/rosa-hyperfleet-api
Length of output: 174
🏁 Script executed:
# Check if there's validation in the controller reconcile logic rg -n -B 3 -A 10 'res\.Resource' hyperfleet-operator/internal/controller/manifest_controller.go | head -80Repository: openshift-online/rosa-hyperfleet-api
Length of output: 3312
🏁 Script executed:
# Search for actual REST path construction using Resource rg -n -i 'format.*path|rest.*path|api.*path|resource.*path' --glob '*.go' hyperfleet-operator/ 2>/dev/null | head -20Repository: openshift-online/rosa-hyperfleet-api
Length of output: 309
🏁 Script executed:
# Check what Kubernetes resource names are allowed (look for kubebuilder validation elsewhere) rg -n 'resource.*validation|Resource.*validation' --glob '*.go' api/v1alpha1/ | head -20Repository: openshift-online/rosa-hyperfleet-api
Length of output: 174
🏁 Script executed:
# Look at the complete ResourceTemplate struct and nearby validation rules cat -n api/v1alpha1/manifest_types.go | sed -n '46,65p'Repository: openshift-online/rosa-hyperfleet-api
Length of output: 1356
🏁 Script executed:
# Check if there's any validation of Resource string format in the controller rg -n 'strings\.|validation|Regexp|regex' hyperfleet-operator/internal/controller/manifest_controller.go | head -20Repository: openshift-online/rosa-hyperfleet-api
Length of output: 225
🏁 Script executed:
# Look for kube-applier-aws package or imports to understand downstream usage rg -n 'kube-applier|kubeapplier' --glob '*.go' . | head -30Repository: openshift-online/rosa-hyperfleet-api
Length of output: 4020
🏁 Script executed:
# Check what validation Kubernetes CRD typically uses for resource names rg -n 'Pattern|pattern' api/v1alpha1/ --glob '*.go' | head -20Repository: openshift-online/rosa-hyperfleet-api
Length of output: 345
🏁 Script executed:
# Look at the dynamo types and ResourceReference to see if validation happens there cat -n hyperfleet-operator/internal/dynamo/types.go | head -40Repository: openshift-online/rosa-hyperfleet-api
Length of output: 1635
🏁 Script executed:
# Search for any allow-list or validation of Kubernetes resource names in the codebase rg -n 'configmap|deployment|secret|service' --glob '*.go' . | grep -i 'allow\|valid\|check' | head -20Repository: openshift-online/rosa-hyperfleet-api
Length of output: 2437
🏁 Script executed:
# Check if manifest_controller does ANY validation before using Resource rg -n -B 5 -A 15 'func.*manifest_controller\.go|func.*Reconcile' hyperfleet-operator/internal/controller/manifest_controller.go | head -80Repository: openshift-online/rosa-hyperfleet-api
Length of output: 3568
🏁 Script executed:
# Search for what valid Kubernetes resource names look like in validation rg -n 'DNS1123|alphanumeric|lowercase|resource.*name' --glob '*.go' . | head -20Repository: openshift-online/rosa-hyperfleet-api
Length of output: 2882
🏁 Script executed:
# Check if there's any external kube-applier code or documentation about REST path construction find . -name '*applier*' -o -name '*kubeapplier*' | head -10Repository: openshift-online/rosa-hyperfleet-api
Length of output: 174
🏁 Script executed:
# Look for how Kubernetes itself validates resource names rg -n 'QualifiedNameMaxLength|DNS1123Subdomain|validResourceName' --glob '*.go' . | head -10Repository: openshift-online/rosa-hyperfleet-api
Length of output: 174
🏁 Script executed:
# Check if the ResourceReference type has any validation on the Resource field rg -n -B 5 -A 10 'type ResourceReference\|struct.*Resource' hyperfleet-operator/internal/dynamo/ --glob '*.go'Repository: openshift-online/rosa-hyperfleet-api
Length of output: 174
🏁 Script executed:
# Check actual REST API path construction patterns in platform-api rg -n 'fmt.Sprintf.*api|path.Join|PathPrefix' platform-api/pkg/handlers/ --glob '*.go' | head -20Repository: openshift-online/rosa-hyperfleet-api
Length of output: 174
🏁 Script executed:
# Verify if there are any input validation rules on plural resource names rg -n 'Plural|plural' api/v1alpha1/ --glob '*.go' -A 3 -B 3 | head -40Repository: openshift-online/rosa-hyperfleet-api
Length of output: 1813
🏁 Script executed:
# Look at the comments around manifest_types.go to understand the full context cat -n api/v1alpha1/manifest_types.go | sed -n '25,75p'Repository: openshift-online/rosa-hyperfleet-api
Length of output: 2753
Path Traversal (CWE-22): Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal')
Reachability: External
Reachability path
● Entry platform-api/pkg/zoa/jobbuilder.go:45 ResourceTemplate │ ▼ ● Sink api/v1alpha1/manifest_types.goAdd validation to
Resourcefield to prevent path traversal in REST paths.The
Resourcefield has only aMinLength=1check. The comments at lines 47–52 state that kube-applier-aws uses this value to construct Kubernetes REST paths. The controller at lines 121–127 copiesResourcedirectly toTargetItem.Resourcewithout a path-segment check.If the downstream builder accepts
/,..,., or query/fragment delimiters, a Manifest creator can redirect requests to unintended API endpoints. Add aPatternvalidation rule to reject path traversal sequences, or add an explicit controller check that validatesResourceagainst the set of allowed Kubernetes resource names before passing it downstream. Per CWE-22, validate at trust boundaries with allow-lists.🤖 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/v1alpha1/manifest_types.go` around lines 53 - 56, Add validation for Resource in ResourceTemplate so only safe Kubernetes resource-name path segments are accepted before values reach TargetItem.Resource. Prefer an allow-list or kubebuilder Pattern that rejects slash, dot-segment, query, and fragment delimiters while preserving the existing MinLength constraint.Source: Path instructions
api/v1alpha1/placement_types.go (1)
40-42: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the controller that creates or reconciles Placement resources. ast-grep outline hyperfleet-operator/internal/controller/placement_controller.go --items all # Trace ManagementCluster validation and Placement writes. rg -n -C 6 --glob '*.go' \ '\bManagementCluster\b|PlacementSpec|\.Create\(|\.Update\(|Validate(Create|Update)' \ api hyperfleet-operator platform-api # Inspect RBAC or admission policy that grants Placement write access. rg -n -C 4 --glob '*.yaml' --glob '*.yml' \ 'placements|Placement|managementclusters|ManagementCluster' .Repository: openshift-online/rosa-hyperfleet-api
Length of output: 50393
🏁 Script executed:
# Inspect the exact usage of ManagementCluster in cluster_controller.go around lines 126-128 sed -n '120,135p' hyperfleet-operator/internal/controller/cluster_controller.go # Check how ManagementCluster is used when writing to Dynamo rg -n -A 5 -B 5 'SpecsPrefix|StatusPrefix' hyperfleet-operator/internal/controller/cluster_controller.go # Look for any RBAC or webhook validation on Placement create/update fd -e yaml -e yml . | xargs rg -l 'placement|Placement' | head -20Repository: openshift-online/rosa-hyperfleet-api
Length of output: 2523
🏁 Script executed:
# Check the placement_controller.go to see how ManagementCluster is set and if there's validation sed -n '1,150p' hyperfleet-operator/internal/controller/placement_controller.go # Check for ValidateCreate/ValidateUpdate webhooks rg -n 'ValidateCreate|ValidateUpdate|Validate' api/v1alpha1/placement_types.go # Inspect the RBAC rules for placements cat hyperfleet-operator/config/rbac/placement_editor_role.yaml cat hyperfleet-operator/config/rbac/placement_admin_role.yaml cat hyperfleet-operator/config/rbac/role.yaml | grep -A 10 -B 2 'placement\|Placement'Repository: openshift-online/rosa-hyperfleet-api
Length of output: 7307
🏁 Script executed:
# Check for webhook validation in placement_types.go cat api/v1alpha1/placement_types.go | head -100 # Look for ValidatingWebhookConfiguration rg -n 'ValidatingWebhookConfiguration|webhooks' hyperfleet-operator/config --glob '*.yaml' --glob '*.yml' # Check if there's a selectManagementCluster implementation and what values are allowed rg -n -A 20 'selectManagementCluster' hyperfleet-operator/internal/controller/placement_controller.go # Inspect how ManagementCluster references are validated rg -n 'ManagementCluster' api/v1alpha1/managementcluster_types.goRepository: openshift-online/rosa-hyperfleet-api
Length of output: 6444
Authorization Bypass (CWE-862): Missing Authorization
Reachability: External
Reachability path
● Entry hyperfleet-operator/internal/controller/cluster_controller_test.go:128 PlacementSpec │ ▼ ● Sink api/v1alpha1/placement_types.goEnforce ManagementCluster immutability or validate against authorized resources.
PlacementSpec.ManagementClusteraccepts any nonempty string and can be modified after creation via Placementpatch/updateoperations.ClusterReconcileruses this value directly to select DynamoDB table prefixes athyperfleet-operator/internal/controller/cluster_controller.golines 126–128. A principal with Placement editor RBAC can route cluster desires to an arbitrary management cluster scope.Prevent post-creation modification of
ManagementClusterby adding an immutability constraint (likeClusterNamehas), or enforce webhook validation that confirms the value references an authorized ManagementCluster resource.🤖 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/v1alpha1/placement_types.go` around lines 40 - 42, Make PlacementSpec.ManagementCluster immutable after creation, matching the existing immutability constraint used by ClusterName. Add the appropriate CEL validation or equivalent schema rule on ManagementCluster so creation remains allowed but subsequent patch/update attempts that change its value are rejected.Source: Path instructions
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@clientset/docs/architecture.md`:
- Line 22: Update the architecture documentation sentence describing client-gen
to call api/v1alpha1 the “API package” instead of the “operator API package,”
preserving the rest of the explanation and type-generation details.
🪄 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: 2ab7505f-cc0c-4679-b532-a2f71959f098
📒 Files selected for processing (4)
.tekton/rosa-hyperfleet-operator-pull-request.yaml.tekton/rosa-hyperfleet-operator-push.yamlclientset/docs/architecture.mdhyperfleet-operator/Containerfile
🚧 Files skipped from review as they are similar to previous changes (1)
- hyperfleet-operator/Containerfile
…el api/ Module path changes from .../hyperfleet-operator/api to .../api. Updates all go.mod replace directives, Go imports, Makefile targets, Tekton pipelines, Containerfiles, and documentation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
6aac62e to
05c58c3
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: 1
🧹 Nitpick comments (1)
api/v1alpha1/placement_types.go (1)
1-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
apimodule in the lint target.Makefileomitsapi, soapi/v1alpha1/placement_types.gois not checked by the standard lint target. Add theapimodule to that target..golangci.ymldoes not define alogcheckplugin.🤖 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/v1alpha1/placement_types.go` around lines 1 - 23, Add the api module to the standard lint target in the Makefile so files such as the Placement types in package v1alpha1 are included; do not add or configure a logcheck plugin in .golangci.yml.Source: Coding guidelines
🤖 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 287: Update the Makefile’s manifests/generate code-generation commands to
execute from the valid Go module directory `api`, so controller-gen can resolve
the API package paths without relying on a root go.mod or go.work; preserve the
existing CRD output location and generation behavior.
---
Nitpick comments:
In `@api/v1alpha1/placement_types.go`:
- Around line 1-23: Add the api module to the standard lint target in the
Makefile so files such as the Placement types in package v1alpha1 are included;
do not add or configure a logcheck plugin in .golangci.yml.
🪄 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: 35e5c298-a3aa-4226-8882-15a68003bd75
⛔ Files ignored due to path filters (8)
api/go.sumis excluded by!**/*.sumapi/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*clientset/generated/fake/register.gois excluded by!**/generated/**clientset/generated/scheme/register.gois excluded by!**/generated/**clientset/generated/typed/v1alpha1/internalversion/cluster.gois excluded by!**/generated/**clientset/generated/typed/v1alpha1/internalversion/fake/fake_cluster.gois excluded by!**/generated/**clientset/generated/typed/v1alpha1/internalversion/fake/fake_nodepool.gois excluded by!**/generated/**clientset/generated/typed/v1alpha1/internalversion/nodepool.gois excluded by!**/generated/**
📒 Files selected for processing (57)
.tekton/rosa-hyperfleet-api-pull-request.yaml.tekton/rosa-hyperfleet-api-push.yaml.tekton/rosa-hyperfleet-operator-pull-request.yaml.tekton/rosa-hyperfleet-operator-push.yamlCLAUDE.mdMakefileREADME.mdapi/go.modapi/v1alpha1/cluster_types.goapi/v1alpha1/groupversion_info.goapi/v1alpha1/install/install.goapi/v1alpha1/managementcluster_types.goapi/v1alpha1/manifest_types.goapi/v1alpha1/nodepool_types.goapi/v1alpha1/placement_types.goclientset/docs/architecture.mdclientset/go.modclientset/wrappers/wire_wrappers_generated.goclientset/wrappers/wrappers_test.gohyperfleet-operator/Containerfilehyperfleet-operator/PROJECThyperfleet-operator/cmd/manager/main.gohyperfleet-operator/go.modhyperfleet-operator/internal/controller/cluster_controller.gohyperfleet-operator/internal/controller/cluster_controller_test.gohyperfleet-operator/internal/controller/manifest_controller.gohyperfleet-operator/internal/controller/manifest_controller_test.gohyperfleet-operator/internal/controller/nodepool_controller.gohyperfleet-operator/internal/controller/nodepool_controller_test.gohyperfleet-operator/internal/controller/placement_controller.gohyperfleet-operator/internal/controller/placement_controller_test.gohyperfleet-operator/internal/controller/suite_test.gohyperfleet-operator/internal/dynamo/statusstream/manager.gohyperfleet-operator/internal/render/cluster.gohyperfleet-operator/internal/render/cluster_test.gohyperfleet-operator/internal/render/nodepool.gohyperfleet-operator/internal/render/nodepool_test.gohyperfleet-operator/test/cluster_test.gohyperfleet-operator/test/helpers_test.gohyperfleet-operator/test/manifest_test.gohyperfleet-operator/test/suite_test.goplatform-api/Containerfileplatform-api/go.modplatform-api/pkg/clients/hyperfleetdb/client.goplatform-api/pkg/clients/hyperfleetdb/client_test.goplatform-api/pkg/clients/hyperfleetdb/convert.goplatform-api/pkg/clients/hyperfleetdb/convert_test.goplatform-api/pkg/handlers/cluster_test.goplatform-api/pkg/handlers/management_cluster.goplatform-api/pkg/handlers/zoa_test.goplatform-api/pkg/types/cluster.goplatform-api/pkg/types/nodepool.goplatform-api/pkg/zoa/jobbuilder.goplatform-api/pkg/zoa/reconciler.goplatform-api/pkg/zoa/reconciler_test.gotest/e2e-sdk/sdk_sanity_test.gotest/go.mod
🚧 Files skipped from review as they are similar to previous changes (55)
- hyperfleet-operator/internal/controller/nodepool_controller.go
- platform-api/pkg/zoa/jobbuilder.go
- platform-api/pkg/zoa/reconciler_test.go
- platform-api/pkg/handlers/cluster_test.go
- clientset/go.mod
- clientset/wrappers/wrappers_test.go
- hyperfleet-operator/PROJECT
- platform-api/pkg/clients/hyperfleetdb/client.go
- .tekton/rosa-hyperfleet-api-pull-request.yaml
- api/go.mod
- platform-api/pkg/handlers/zoa_test.go
- hyperfleet-operator/cmd/manager/main.go
- hyperfleet-operator/internal/controller/manifest_controller.go
- hyperfleet-operator/internal/controller/cluster_controller_test.go
- hyperfleet-operator/internal/controller/suite_test.go
- hyperfleet-operator/internal/render/cluster_test.go
- api/v1alpha1/groupversion_info.go
- hyperfleet-operator/internal/controller/placement_controller.go
- README.md
- test/go.mod
- test/e2e-sdk/sdk_sanity_test.go
- CLAUDE.md
- hyperfleet-operator/go.mod
- hyperfleet-operator/internal/render/nodepool.go
- hyperfleet-operator/test/cluster_test.go
- .tekton/rosa-hyperfleet-api-push.yaml
- .tekton/rosa-hyperfleet-operator-push.yaml
- hyperfleet-operator/test/suite_test.go
- platform-api/pkg/clients/hyperfleetdb/client_test.go
- .tekton/rosa-hyperfleet-operator-pull-request.yaml
- hyperfleet-operator/internal/render/nodepool_test.go
- hyperfleet-operator/internal/controller/cluster_controller.go
- platform-api/pkg/types/nodepool.go
- platform-api/pkg/types/cluster.go
- hyperfleet-operator/internal/controller/nodepool_controller_test.go
- platform-api/pkg/handlers/management_cluster.go
- platform-api/Containerfile
- platform-api/pkg/clients/hyperfleetdb/convert.go
- hyperfleet-operator/Containerfile
- hyperfleet-operator/internal/dynamo/statusstream/manager.go
- platform-api/pkg/zoa/reconciler.go
- clientset/docs/architecture.md
- hyperfleet-operator/internal/controller/placement_controller_test.go
- api/v1alpha1/install/install.go
- platform-api/pkg/clients/hyperfleetdb/convert_test.go
- api/v1alpha1/manifest_types.go
- hyperfleet-operator/test/helpers_test.go
- hyperfleet-operator/test/manifest_test.go
- hyperfleet-operator/internal/render/cluster.go
- clientset/wrappers/wire_wrappers_generated.go
- platform-api/go.mod
- hyperfleet-operator/internal/controller/manifest_controller_test.go
- api/v1alpha1/managementcluster_types.go
- api/v1alpha1/nodepool_types.go
- api/v1alpha1/cluster_types.go
|
/test on-demand-e2e |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cdoan1, typeid 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 |
844e7fe
into
openshift-online:main
3 participants
Description
Module path changes from .../hyperfleet-operator/api to .../api. Updates all go.mod replace directives, Go imports, Makefile targets, Tekton pipelines, Containerfiles, and documentation.
Type of Change
Testing
make test)Checklist
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores