Skip to content

[WIP] OCPCLOUD-3557: split capi-controllers and machine-api-migration - #622

Open
stefanonardo wants to merge 4 commits into
openshift:mainfrom
stefanonardo:OCPCLOUD-3557
Open

[WIP] OCPCLOUD-3557: split capi-controllers and machine-api-migration#622
stefanonardo wants to merge 4 commits into
openshift:mainfrom
stefanonardo:OCPCLOUD-3557

Conversation

@stefanonardo

@stefanonardo stefanonardo commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Split the single capi-controllers Deployment (two containers sharing one SA) into two independent Deployments with dedicated ServiceAccounts and least-privilege RBAC
  • Aligned machine-api-migration metrics port to :8443 (matching all other binaries)

Test plan

  • make build passes
  • make lint passes (0 issues)
  • make unit passes (pre-existing crdcompatibility failures excluded)
  • CI e2e tests pass with both pods running under separate SAs
  • Run audit2rbac on CI job audit logs to confirm zero 403s for both SAs

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added a dedicated Machine API migration controller with feature-gate support.
    • Added separate metrics service and monitoring for the migration controller.
  • Security
    • Separated controller permissions and tightened access for improved isolation.
    • Removed unnecessary pull-secret access.
  • Bug Fixes
    • Corrected controller metrics and health-check ports.
    • Updated network policies for both metrics endpoints.
  • Documentation
    • Documented separate service accounts and permissions.

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 9, 2026
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 9, 2026
@openshift-ci-robot

openshift-ci-robot commented Jul 9, 2026

Copy link
Copy Markdown

@stefanonardo: This pull request references OCPCLOUD-3557 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.

Details

In response to this:

Summary

  • Split the single capi-controllers Deployment (two containers sharing one SA) into two independent Deployments with dedicated ServiceAccounts and least-privilege RBAC
  • Aligned machine-api-migration metrics port to :8443 (matching all other binaries)

Test plan

  • make build passes
  • make lint passes (0 issues)
  • make unit passes (pre-existing crdcompatibility failures excluded)
  • CI e2e tests pass with both pods running under separate SAs
  • Run audit2rbac on CI job audit logs to confirm zero 403s for both SAs

🤖 Generated with Claude Code

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.

@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: e00fd0a4-fb5a-450b-a551-51566e8c0720

📥 Commits

Reviewing files that changed from the base of the PR and between daedb87 and 961bcaf.

📒 Files selected for processing (16)
  • capi-operator-manifests/default/manifests.yaml
  • cmd/capi-controllers/main.go
  • docs/rbac.md
  • manifests/0000_30_cluster-api_02_machine-api-migration-service-account.yaml
  • manifests/0000_30_cluster-api_03_machine-api-migration-rbac-roles.yaml
  • manifests/0000_30_cluster-api_03_rbac_roles.yaml
  • manifests/0000_30_cluster-api_04_machine-api-migration-rbac-bindings.yaml
  • manifests/0000_30_cluster-api_04_rbac_bindings.yaml
  • manifests/0000_30_cluster-api_10_capi-controllers-servicemonitor.yaml
  • manifests/0000_30_cluster-api_10_machine-api-migration-metrics-service.yaml
  • manifests/0000_30_cluster-api_10_machine-api-migration-servicemonitor.yaml
  • manifests/0000_30_cluster-api_10_metrics-service.yaml
  • manifests/0000_30_cluster-api_12_allow-ingress-to-metrics-operators.yaml
  • manifests/0000_30_cluster-api_14_allow-egress-operators.yaml
  • manifests/0000_30_cluster-api_17_machine-api-migration-deployment.yaml
  • ocp-manifests-input/default/capi-controllers-deployment.yaml
💤 Files with no reviewable changes (4)
  • cmd/capi-controllers/main.go
  • manifests/0000_30_cluster-api_04_rbac_bindings.yaml
  • manifests/0000_30_cluster-api_10_capi-controllers-servicemonitor.yaml
  • manifests/0000_30_cluster-api_03_rbac_roles.yaml
🚧 Files skipped from review as they are similar to previous changes (11)
  • manifests/0000_30_cluster-api_12_allow-ingress-to-metrics-operators.yaml
  • docs/rbac.md
  • capi-operator-manifests/default/manifests.yaml
  • manifests/0000_30_cluster-api_10_metrics-service.yaml
  • manifests/0000_30_cluster-api_02_machine-api-migration-service-account.yaml
  • manifests/0000_30_cluster-api_04_machine-api-migration-rbac-bindings.yaml
  • ocp-manifests-input/default/capi-controllers-deployment.yaml
  • manifests/0000_30_cluster-api_03_machine-api-migration-rbac-roles.yaml
  • manifests/0000_30_cluster-api_14_allow-egress-operators.yaml
  • manifests/0000_30_cluster-api_10_machine-api-migration-servicemonitor.yaml
  • manifests/0000_30_cluster-api_10_machine-api-migration-metrics-service.yaml

Walkthrough

The change separates machine-api-migration from capi-controllers into dedicated workload, ServiceAccount, RBAC, metrics, and network policy resources. It removes the migration sidecar and reduces capi-controllers permissions and cache configuration.

Changes

Machine API migration separation

Layer / File(s) Summary
Migration identity and RBAC
manifests/0000_30_cluster-api_02_*, manifests/0000_30_cluster-api_03_machine-api-migration-rbac-roles.yaml, manifests/0000_30_cluster-api_04_machine-api-migration-rbac-bindings.yaml, docs/rbac.md
Adds a feature-gated migration ServiceAccount, dedicated cluster-wide and namespaced roles, bindings, and RBAC documentation.
Controller workload and permission reduction
ocp-manifests-input/default/capi-controllers-deployment.yaml, capi-operator-manifests/default/manifests.yaml, cmd/capi-controllers/main.go, manifests/0000_30_cluster-api_03_rbac_roles.yaml, manifests/0000_30_cluster-api_04_rbac_bindings.yaml
Removes the migration sidecar and MachineSet cache entry, renames controller ports, reduces controller RBAC, and removes the pull-secret binding.
Dedicated migration deployment
manifests/0000_30_cluster-api_17_machine-api-migration-deployment.yaml
Adds the migration Deployment with health endpoints, TLS mounting, scheduling settings, resource requests, and the migration ServiceAccount.
Migration metrics and network access
manifests/0000_30_cluster-api_10_*, manifests/0000_30_cluster-api_12_allow-ingress-to-metrics-operators.yaml, manifests/0000_30_cluster-api_14_allow-egress-operators.yaml
Adds migration metrics Service and ServiceMonitor, updates controller metrics targeting, and updates metrics ingress and operator egress policies.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant machine-api-migration
  participant machine-api-migration-metrics
  participant ServiceMonitor
  machine-api-migration->>machine-api-migration-metrics: Expose HTTPS metrics on port 8443
  ServiceMonitor->>machine-api-migration-metrics: Select migration metrics Service
  ServiceMonitor->>machine-api-migration-metrics: Scrape metrics with TLS
Loading

Possibly related PRs

Suggested reviewers: mdbooth, radekmanak

🚥 Pre-merge checks | ✅ 14 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Topology-Aware Scheduling Compatibility ⚠️ Warning The new machine-api-migration Deployment requires node-role.kubernetes.io/control-plane, but no topology-aware logic exists; HyperShift hosted clusters lack these labels, so the pod remains Pending. Remove the unconditional control-plane nodeSelector or add topology-aware scheduling that supports External, SNO, TNF, and TNA topologies; validate with topology-specific CI.
✅ Passed checks (14 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: splitting capi-controllers and machine-api-migration into separate workloads.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed The PR changes no test files or Ginkgo title expressions; the complete diff contains no It, Describe, Context, When, or Entry calls.
Test Structure And Quality ✅ Passed The PR diff contains no Ginkgo test files or test-path changes, so these test-structure requirements are not applicable.
Microshift Test Compatibility ✅ Passed The full pull request diff adds no Ginkgo tests and changes no *_test.go or e2e files, so MicroShift test compatibility is not applicable.
Single Node Openshift (Sno) Test Compatibility ✅ Passed The PR diff adds no e2e or test files and no Ginkgo declarations; its only Go change removes a MachineSet cache entry.
Ote Binary Stdout Contract ✅ Passed The PR changes no OTE or e2e source. OTE main has no stdout writes, and its Ginkgo setup redirects GinkgoWriter to os.Stderr before suite setup.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed The committed diff changes only RBAC/docs YAML and cmd/capi-controllers/main.go; it adds no Ginkgo e2e tests or test declarations requiring IPv4 or external connectivity.
No-Weak-Crypto ✅ Passed The PR diff adds no MD5, SHA-1, DES, 3DES, RC4, Blowfish, or ECB usage; changed Go code only removes a cache entry, and TLS manifests contain no secret comparisons or custom crypto.
Container-Privileges ✅ Passed PR diff adds no privileged, hostPID, hostNetwork, hostIPC, SYS_ADMIN, allowPrivilegeEscalation, or runAsUser: 0 settings; both Deployments require restricted-v2.
No-Sensitive-Data-In-Logs ✅ Passed The PR adds no logging statements or sensitive values to logs; migration logging code is unchanged, and the new Deployment only sets diagnostics and standard log-on-error termination behavior.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@stefanonardo

Copy link
Copy Markdown
Contributor Author

/test e2e-aws-capi-techpreview

@openshift-ci
openshift-ci Bot requested review from RadekManak and mdbooth July 9, 2026 11:18
@openshift-ci

openshift-ci Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign mdbooth for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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 `@docs/rbac.md`:
- Around line 41-43: The RBAC summary for the `machine-api-migration` Role in
`openshift-cluster-api` is mislabeled as “(read)” even though the manifest
grants both read and write verbs. Update the description in `docs/rbac.md` to
reflect the actual permissions from
`0000_30_cluster-api_03_machine-api-migration-rbac-roles.yaml`, using the
`machine-api-migration` Role entry and its Machine/MachineSet permissions as the
reference point.

In `@manifests/0000_30_cluster-api_17_machine-api-migration-deployment.yaml`:
- Around line 26-69: The machine-api-migration Deployment container spec is
missing required hardening and health settings. Update the machine-api-migration
pod/container spec to add an explicit securityContext with
readOnlyRootFilesystem, allowPrivilegeEscalation disabled, and capabilities
dropping all, and set automountServiceAccountToken to false if the controller
does not need the token. Also add resource limits alongside the existing
requests, and define livenessProbe and readinessProbe for the healthz endpoint
exposed by the machine-api-migration container on port 9440.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: af553a64-1e21-4c5f-92f4-9de4822110af

📥 Commits

Reviewing files that changed from the base of the PR and between f2f0de3 and bd4c791.

📒 Files selected for processing (12)
  • docs/rbac.md
  • manifests/0000_30_cluster-api_02_machine-api-migration-service-account.yaml
  • manifests/0000_30_cluster-api_03_machine-api-migration-rbac-roles.yaml
  • manifests/0000_30_cluster-api_03_rbac_roles.yaml
  • manifests/0000_30_cluster-api_04_machine-api-migration-rbac-bindings.yaml
  • manifests/0000_30_cluster-api_10_capi-controllers-servicemonitor.yaml
  • manifests/0000_30_cluster-api_10_machine-api-migration-metrics-service.yaml
  • manifests/0000_30_cluster-api_10_machine-api-migration-servicemonitor.yaml
  • manifests/0000_30_cluster-api_10_metrics-service.yaml
  • manifests/0000_30_cluster-api_12_allow-ingress-to-metrics-operators.yaml
  • manifests/0000_30_cluster-api_17_deployment.yaml
  • manifests/0000_30_cluster-api_17_machine-api-migration-deployment.yaml
💤 Files with no reviewable changes (1)
  • manifests/0000_30_cluster-api_10_capi-controllers-servicemonitor.yaml

Comment thread docs/rbac.md
Comment on lines +26 to +69
spec:
serviceAccountName: machine-api-migration
containers:
- name: machine-api-migration
image: registry.ci.openshift.org/openshift:cluster-capi-operator
command:
- /machine-api-migration
args:
- --diagnostics-address=:8443
env:
- name: RELEASE_VERSION
value: "0.0.1-snapshot"
ports:
- containerPort: 8443
name: diagnostics
protocol: TCP
- containerPort: 9440
name: healthz
protocol: TCP
resources:
requests:
cpu: 10m
memory: 50Mi
terminationMessagePolicy: FallbackToLogsOnError
volumeMounts:
- name: metrics-cert
mountPath: /tmp/k8s-metrics-server/serving-certs
readOnly: true
nodeSelector:
node-role.kubernetes.io/control-plane: ""
priorityClassName: system-cluster-critical
restartPolicy: Always
tolerations:
- key: "node-role.kubernetes.io/master"
operator: "Exists"
effect: "NoSchedule"
- key: "node-role.kubernetes.io/control-plane"
operator: "Exists"
effect: "NoSchedule"
volumes:
- name: metrics-cert
secret:
defaultMode: 420
secretName: machine-api-migration-metrics-tls

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Add explicit securityContext, resource limits, and probes to the container spec.

This new Deployment lacks:

  • Pod/container securityContext (readOnlyRootFilesystem: true, allowPrivilegeEscalation: false, capabilities.drop: ["ALL"]). Note restricted-v2 SCC (referenced via the openshift.io/required-scc: restricted-v2 annotation) already drops all capabilities and disallows privilege escalation by default, but it does not enforce readOnlyRootFilesystem, so this must be set explicitly.
  • resources.limits (only requests are set).
  • livenessProbe/readinessProbe, despite exposing a healthz port on 9440.
  • automountServiceAccountToken: false (unless the controller genuinely needs the projected SA token via the pod itself rather than client libraries).

Static analysis flags the missing root-fs/security-context settings (Checkov CKV_K8S_20/CKV_K8S_23, Trivy KSV-0014/KSV-0118).

As per path instructions: "securityContext: runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation: false", "Drop ALL capabilities, add only what is required", "Resource limits (cpu, memory) on every container", "Liveness + readiness probes defined", "automountServiceAccountToken: false unless needed".

🛡️ Proposed fix
     spec:
       serviceAccountName: machine-api-migration
+      automountServiceAccountToken: false
       containers:
       - name: machine-api-migration
         image: registry.ci.openshift.org/openshift:cluster-capi-operator
         command:
         - /machine-api-migration
         args:
           - --diagnostics-address=:8443
         env:
         - name: RELEASE_VERSION
           value: "0.0.1-snapshot"
         ports:
         - containerPort: 8443
           name: diagnostics
           protocol: TCP
         - containerPort: 9440
           name: healthz
           protocol: TCP
         resources:
           requests:
             cpu: 10m
             memory: 50Mi
+          limits:
+            cpu: 100m
+            memory: 100Mi
+        securityContext:
+          allowPrivilegeEscalation: false
+          readOnlyRootFilesystem: true
+          capabilities:
+            drop:
+              - ALL
+        livenessProbe:
+          httpGet:
+            path: /healthz
+            port: healthz
+        readinessProbe:
+          httpGet:
+            path: /readyz
+            port: healthz
         terminationMessagePolicy: FallbackToLogsOnError
🧰 Tools
🪛 Checkov (3.3.2)

[medium] 2-69: Containers should not run with allowPrivilegeEscalation

(CKV_K8S_20)


[medium] 2-69: Minimize the admission of root containers

(CKV_K8S_23)

🪛 Trivy (0.69.3)

[error] 29-53: Root file system is not read-only

Container 'machine-api-migration' of Deployment 'machine-api-migration' should set 'securityContext.readOnlyRootFilesystem' to true

Rule: KSV-0014

Learn more

(IaC/Kubernetes)


[error] 29-53: Default security context configured

container machine-api-migration in openshift-cluster-api namespace is using the default security context

Rule: KSV-0118

Learn more

(IaC/Kubernetes)


[error] 26-69: Default security context configured

deployment machine-api-migration in openshift-cluster-api namespace is using the default security context, which allows root privileges

Rule: KSV-0118

Learn more

(IaC/Kubernetes)

🤖 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 `@manifests/0000_30_cluster-api_17_machine-api-migration-deployment.yaml`
around lines 26 - 69, The machine-api-migration Deployment container spec is
missing required hardening and health settings. Update the machine-api-migration
pod/container spec to add an explicit securityContext with
readOnlyRootFilesystem, allowPrivilegeEscalation disabled, and capabilities
dropping all, and set automountServiceAccountToken to false if the controller
does not need the token. Also add resource limits alongside the existing
requests, and define livenessProbe and readinessProbe for the healthz endpoint
exposed by the machine-api-migration container on port 9440.

Sources: Path instructions, Linters/SAST tools

@stefanonardo

Copy link
Copy Markdown
Contributor Author

/retest

@openshift-ci

openshift-ci Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

@stefanonardo: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-capi-techpreview bd4c791 link true /test e2e-aws-capi-techpreview

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
manifests/0000_30_cluster-api_10_machine-api-migration-metrics-service.yaml (1)

12-20: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Remove the ineffective targetPort mapping for this headless Service.

With clusterIP: None, Kubernetes ignores targetPort; the Service port must already match the pod’s listening port. Keep the migration listener on 8443 and omit targetPort, or use a non-headless Service if named port remapping is required. (kubernetes.io)

Suggested cleanup
   - name: machine-api-migration-metrics
     port: 8443
-    targetPort: diagnostics
🤖 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 `@manifests/0000_30_cluster-api_10_machine-api-migration-metrics-service.yaml`
around lines 12 - 20, Update the Service definition for
machine-api-migration-metrics by removing the targetPort mapping while retaining
port 8443 and clusterIP: None, so the headless Service uses the pod’s listening
port directly.
🤖 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 `@manifests/0000_30_cluster-api_17_machine-api-migration-deployment.yaml`:
- Around line 54-55: Remove the control-plane nodeSelector entry from the
Deployment manifest, leaving only topology-neutral scheduling rules so the
workload can schedule on HyperShift.

In `@ocp-manifests-input/default/capi-controllers-deployment.yaml`:
- Around line 39-42: Update the generated capi-operator manifests.yaml to
replace the outdated diagnostics-o and healthz-o port references with the
renamed diagnostics and healthz names, matching the source deployment manifest
while preserving the existing port configuration.

---

Nitpick comments:
In `@manifests/0000_30_cluster-api_10_machine-api-migration-metrics-service.yaml`:
- Around line 12-20: Update the Service definition for
machine-api-migration-metrics by removing the targetPort mapping while retaining
port 8443 and clusterIP: None, so the headless Service uses the pod’s listening
port directly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 128a92f8-b856-4904-bd2c-21119a1479de

📥 Commits

Reviewing files that changed from the base of the PR and between bd4c791 and deab05f.

📒 Files selected for processing (12)
  • docs/rbac.md
  • manifests/0000_30_cluster-api_02_machine-api-migration-service-account.yaml
  • manifests/0000_30_cluster-api_03_machine-api-migration-rbac-roles.yaml
  • manifests/0000_30_cluster-api_03_rbac_roles.yaml
  • manifests/0000_30_cluster-api_04_machine-api-migration-rbac-bindings.yaml
  • manifests/0000_30_cluster-api_10_capi-controllers-servicemonitor.yaml
  • manifests/0000_30_cluster-api_10_machine-api-migration-metrics-service.yaml
  • manifests/0000_30_cluster-api_10_machine-api-migration-servicemonitor.yaml
  • manifests/0000_30_cluster-api_10_metrics-service.yaml
  • manifests/0000_30_cluster-api_12_allow-ingress-to-metrics-operators.yaml
  • manifests/0000_30_cluster-api_17_machine-api-migration-deployment.yaml
  • ocp-manifests-input/default/capi-controllers-deployment.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
  • manifests/0000_30_cluster-api_02_machine-api-migration-service-account.yaml
  • manifests/0000_30_cluster-api_10_machine-api-migration-servicemonitor.yaml
  • manifests/0000_30_cluster-api_10_metrics-service.yaml
  • manifests/0000_30_cluster-api_10_capi-controllers-servicemonitor.yaml
  • manifests/0000_30_cluster-api_04_machine-api-migration-rbac-bindings.yaml
  • docs/rbac.md
  • manifests/0000_30_cluster-api_03_machine-api-migration-rbac-roles.yaml
  • manifests/0000_30_cluster-api_03_rbac_roles.yaml

Comment on lines +54 to +55
nodeSelector:
node-role.kubernetes.io/control-plane: ""

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Remove the control-plane node selector.

This hard requirement prevents the Deployment from scheduling on HyperShift, where these nodes are not available to this workload. Keep only topology-neutral scheduling rules unless this manifest is conditionally excluded for HyperShift.

Proposed fix
-      nodeSelector:
-        node-role.kubernetes.io/control-plane: ""

As per coding guidelines: “nodeSelector/affinity targeting control-plane nodes (breaks on HyperShift).”

📝 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.

Suggested change
nodeSelector:
node-role.kubernetes.io/control-plane: ""
🤖 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 `@manifests/0000_30_cluster-api_17_machine-api-migration-deployment.yaml`
around lines 54 - 55, Remove the control-plane nodeSelector entry from the
Deployment manifest, leaving only topology-neutral scheduling rules so the
workload can schedule on HyperShift.

Source: Coding guidelines

Comment thread ocp-manifests-input/default/capi-controllers-deployment.yaml

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@manifests/0000_30_cluster-api_14_allow-egress-operators.yaml`:
- Line 29: Replace the unrestricted egress rule in the machine-api-migration
egress policy with a port-based rule that allows only Kubernetes API server
access. Find the `- {}` entry in the machine-api-migration egress rules and
replace it with the same API-server port rule used by the compatibility operator
elsewhere in this manifest to ensure consistent and restrictive egress control.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 3d068657-10d3-4b28-8fa1-d67934bd8400

📥 Commits

Reviewing files that changed from the base of the PR and between 516ec55 and daedb87.

📒 Files selected for processing (5)
  • cmd/capi-controllers/main.go
  • docs/rbac.md
  • manifests/0000_30_cluster-api_03_rbac_roles.yaml
  • manifests/0000_30_cluster-api_04_rbac_bindings.yaml
  • manifests/0000_30_cluster-api_14_allow-egress-operators.yaml
💤 Files with no reviewable changes (3)
  • cmd/capi-controllers/main.go
  • manifests/0000_30_cluster-api_04_rbac_bindings.yaml
  • manifests/0000_30_cluster-api_03_rbac_roles.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
  • docs/rbac.md

operator: In
values:
- capi-controllers
- machine-api-migration

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n -C 10 \
  'allow-egress-operators|machine-api-migration|ServiceMonitor|https?://|RESTConfig|New.*Client|DialContext' \
  manifests cmd

Repository: openshift/cluster-capi-operator

Length of output: 50387


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- NetworkPolicy ---'
sed -n '1,80p' manifests/0000_30_cluster-api_14_allow-egress-operators.yaml

printf '%s\n' '--- migration entrypoint and controller wiring ---'
sed -n '150,280p' cmd/machine-api-migration/main.go
rg -n -g '*.go' \
  'NewForConfig|NewController|SetupWithManager|GetConfig|RESTMapper|rest\.|http\.|net\.|Dial|cloud|Provider|client\.New|Get\(|List\(|Watch\(' \
  cmd/machine-api-migration pkg/controllers/machinemigration pkg/controllers/machinesetmigration pkg/controllers/machinesetsync pkg/controllers/machinesync

Repository: openshift/cluster-capi-operator

Length of output: 41723


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- manager setup ---'
sed -n '60,155p' cmd/machine-api-migration/main.go

printf '%s\n' '--- direct non-test imports and network-like calls ---'
python3 - <<'PY'
from pathlib import Path
roots = [
    Path("cmd/machine-api-migration"),
    Path("pkg/controllers/machinemigration"),
    Path("pkg/controllers/machinesetmigration"),
    Path("pkg/controllers/machinesetsync"),
    Path("pkg/controllers/machinesync"),
]
for root in roots:
    for p in sorted(root.rglob("*.go")):
        if p.name.endswith("_test.go"):
            continue
        text = p.read_text()
        imports = []
        in_import = False
        for line in text.splitlines():
            s = line.strip()
            if s == "import (":
                in_import = True
                continue
            if in_import and s == ")":
                in_import = False
                continue
            if in_import or s.startswith("import "):
                imports.append(s)
        calls = [
            (i + 1, line.strip())
            for i, line in enumerate(text.splitlines())
            if any(x in line for x in (
                "http.", "https://", "net.", "Dial", "NewForConfig",
                "RESTClient", "HTTPClient", "url.", "tls.", "cloud",
            ))
        ]
        if imports or calls:
            print(f"\nFILE {p}")
            for line in imports:
                print("IMPORT", line)
            for line_no, line in calls:
                print(f"CALLLIKE {line_no}: {line}")
PY

printf '%s\n' '--- related egress policies ---'
for f in manifests/0000_30_cluster-api_13_allow-egress-controllers.yaml \
         manifests/0000_30_cluster-api_15_default-deny.yaml; do
  if test -f "$f"; then
    sed -n '1,120p' "$f"
  fi
done

Repository: openshift/cluster-capi-operator

Length of output: 19627


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- shared manager and infrastructure helpers ---'
rg -n -C 8 \
  'func InitOperatorConfig|func GetInfra|func GetCAPITypesForInfrastructure|NewManager|Webhook|healthz|metrics' \
  pkg cmd/machine-api-migration

printf '%s\n' '--- all NetworkPolicy egress rules in the repository ---'
python3 - <<'PY'
from pathlib import Path
for p in sorted(Path("manifests").glob("*.y*ml")):
    text = p.read_text()
    if "kind: NetworkPolicy" in text and ("egress:" in text or "policyTypes:" in text):
        print(f"\nFILE {p}")
        lines = text.splitlines()
        for i, line in enumerate(lines):
            if line.lstrip().startswith("egress:") or line.lstrip().startswith("policyTypes:"):
                print(f"{i+1}: {line}")
                for j in range(i + 1, min(i + 12, len(lines))):
                    if lines[j].startswith("apiVersion:") or lines[j].lstrip().startswith("podSelector:"):
                        break
                    print(f"{j+1}: {lines[j]}")
PY

Repository: openshift/cluster-capi-operator

Length of output: 50388


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- helper definitions ---'
rg -l 'func InitOperatorConfig|func GetInfra|func GetCAPITypesForInfrastructure' pkg cmd \
  | while read -r f; do
      printf '\nFILE %s\n' "$f"
      rg -n -A 35 -B 3 \
        'func InitOperatorConfig|func GetInfra|func GetCAPITypesForInfrastructure' "$f"
    done

printf '%s\n' '--- migration production files with HTTP or network packages ---'
python3 - <<'PY'
from pathlib import Path
needles = (
    '"net"', '"net/http"', '"net/url"', '"crypto/tls"',
    '"github.com/aws/aws-sdk-go', '"github.com/gophercloud',
    '"cloud.google.com"', '"google.golang.org/api"',
)
for p in sorted(Path("cmd/machine-api-migration").rglob("*.go")) + \
         sorted(Path("pkg/controllers").rglob("*.go")):
    if p.name.endswith("_test.go"):
        continue
    lines = p.read_text().splitlines()
    hits = [(i + 1, line.strip()) for i, line in enumerate(lines)
            if any(n in line for n in needles)]
    if hits:
        print(f"\nFILE {p}")
        for n, line in hits:
            print(f"{n}: {line}")
PY

printf '%s\n' '--- targeted policy files ---'
find manifests -maxdepth 1 -type f \( -name '*egress*.yaml' -o -name '*egress*.yml' \) -print \
  | sort \
  | while read -r f; do
      printf '\nFILE %s\n' "$f"
      sed -n '1,100p' "$f"
    done

Repository: openshift/cluster-capi-operator

Length of output: 11903


Restrict machine-api-migration egress to the Kubernetes API server.

The migration process only uses Kubernetes API clients. It has no external cloud-provider or network client. Replace the unrestricted - {} rule with the API-server port rule used by the compatibility operator.

🤖 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 `@manifests/0000_30_cluster-api_14_allow-egress-operators.yaml` at line 29,
Replace the unrestricted egress rule in the machine-api-migration egress policy
with a port-based rule that allows only Kubernetes API server access. Find the
`- {}` entry in the machine-api-migration egress rules and replace it with the
same API-server port rule used by the compatibility operator elsewhere in this
manifest to ensure consistent and restrictive egress control.

Comment thread docs/rbac.md
| `0000_30_cluster-api_03_rbac_roles.yaml` | Role `capi-controllers` | `openshift-cluster-api` | CAPI Cluster + infra cluster resources, secrets, pod self-read, events, leases |
| `0000_30_cluster-api_03_rbac_roles.yaml` | Role `capi-controllers` | `openshift-machine-api` | MAPI machines (read-only for InfraCluster), controlplanemachinesets (InfraCluster), secrets (read-only) |
| `0000_30_cluster-api_03_rbac_roles.yaml` | Role `capi-controllers-kube-system` | `kube-system` | Secrets (vSphere credentials) |
| `0000_30_cluster-api_03_rbac_roles.yaml` | Role `cluster-capi-operator-pull-secret` | `openshift-config` | Pull-secret read |

@simkam simkam Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cluster-capi-operator-pull-secret role and related resources removed. I can't find them used anywhere.

They were added in a61882d#diff-b259f954d26aaa43aa09f0fbc1c7af8f00d361b3122c70a2052b09ea91e5aaccR45

stefanonardo and others added 4 commits August 3, 2026 16:12
Separate the two binaries into independent Deployments with dedicated
ServiceAccounts and least-privilege RBAC.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants