Skip to content

OCPBUGS-98329: Add aggregated cluster role for extended per-provider RBAC#623

Open
mdbooth wants to merge 5 commits into
openshift:mainfrom
openshift-cloud-team:OCPBUGS-98329
Open

OCPBUGS-98329: Add aggregated cluster role for extended per-provider RBAC#623
mdbooth wants to merge 5 commits into
openshift:mainfrom
openshift-cloud-team:OCPBUGS-98329

Conversation

@mdbooth

@mdbooth mdbooth commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Adds openshift-capi-installer:provider-permissions ClusterRole and binds it to capi-installer. This is an aggregated cluster rule which uses the label capi-operator.openshift.io/aggregate-to-installer. If a provider's manifests include any object which the default CAPIO RBAC does not cover they can specify their additional requirements in their own repo as a separate ClusterRole which aggregates to this one.

Also fixes a bug initialising boxcutter's tracking cache which turns missing RBAC into a panic.

Summary by CodeRabbit

  • New Features
    • Added installer provider RBAC using an aggregated, label-driven permission model, including a new cluster-scoped role and binding to the installer service account.
  • Bug Fixes
    • Improved installer managed tracking by ensuring correct REST mapping for cluster-scoped resources.
  • Documentation
    • Updated the provider contract with a new section detailing default installer permissions and how to extend them via the aggregated role pattern.
  • Tests
    • Increased test logging verbosity and refined machine sync readiness/sentinel workflow validations.

mdbooth added 2 commits July 10, 2026 15:54
Boxcutter expects the rest mapper to be available, and will panic if it
is not.
@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 10, 2026
@openshift-ci-robot

Copy link
Copy Markdown

@mdbooth: This pull request references Jira Issue OCPBUGS-98329, which is invalid:

  • expected the bug to target the "5.0.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Adds openshift-capi-installer:provider-permissions ClusterRole and binds it to capi-installer. This is an aggregated cluster rule which uses the label capi-operator.openshift.io/aggregate-to-installer. If a provider's manifests include any object which the default CAPIO RBAC does not cover they can specify their additional requirements in their own repo as a separate ClusterRole which aggregates to this one.

Also fixes a bug initialising boxcutter's tracking cache which turns missing RBAC into a panic.

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-robot openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Jul 10, 2026
@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown

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: 4b9f70a2-3d55-459c-a32f-65ddbec2c6f4

📥 Commits

Reviewing files that changed from the base of the PR and between d197f7f and c826684.

📒 Files selected for processing (1)
  • pkg/controllers/machinesync/machine_sync_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • pkg/controllers/machinesync/machine_sync_controller_test.go

Walkthrough

The installer now receives provider permissions through an aggregated ClusterRole and binding. Its tracking cache uses the controller-manager REST mapper, while controller tests increase logging verbosity and refine VAP synchronization setup.

Changes

Installer permissions and tracking

Layer / File(s) Summary
Aggregate and bind provider permissions
manifests/0000_30_cluster-api-operator_03_capi-installer-clusterrole.yaml, manifests/0000_30_cluster-api-operator_04_capi-installer-clusterrolebinding.yaml, docs/provider-contract.md
Adds the aggregated provider-permissions ClusterRole, binds it to the capi-installer ServiceAccount, and documents default and provider-extended permissions.
Configure tracking cache mapper
pkg/controllers/installer/installer_controller.go
Passes the controller-manager REST mapper into the tracking cache options.

Controller test setup

Layer / File(s) Summary
Increase test logging verbosity
pkg/controllers/installer/revision_reconciler_test.go, pkg/controllers/installer/suite_test.go, pkg/controllers/machinemigration/suite_test.go, pkg/controllers/machinesetmigration/suite_test.go, pkg/controllers/machinesetsync/suite_test.go, pkg/controllers/machinesync/suite_test.go
Changes test loggers from verbosity 0 to 4.
Refine cluster API VAP synchronization test
pkg/controllers/machinesync/machine_sync_controller_test.go
Applies sentinel VAP validation through an update, requires observed generation equality, and waits for sentinel CAPI machine synchronization before the rejected label update.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: damdo, theobarberbany

🚥 Pre-merge checks | ✅ 15
✅ Passed checks (15 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: adding an aggregated ClusterRole for provider-specific RBAC extensions.
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 Changed test files only adjust setup/logging; inspected Ginkgo titles and found only static literal names, with no dynamic or unstable values.
Test Structure And Quality ✅ Passed PASS: The new VAP setup stays in BeforeEach/AfterEach, uses the suite’s eventual timeout, and cleans up cluster-scoped VAP resources; no new multi-behavior It blocks were added.
Microshift Test Compatibility ✅ Passed The PR only adds steps inside an existing Ginkgo context; it doesn’t add an unguarded new test, and the new code uses admission policy APIs, not MicroShift-blocked OpenShift APIs.
Single Node Openshift (Sno) Test Compatibility ✅ Passed PASS: The only modified Ginkgo code is envtest/admission setup; it adds no node-count, topology, or scheduling assumptions, and no new tests.
Topology-Aware Scheduling Compatibility ✅ Passed Touched files are RBAC/doc/tracking-cache/test changes only; no nodeSelector, affinity, tolerations, PDB, or replica-topology logic was introduced.
Ote Binary Stdout Contract ✅ Passed Changed BeforeSuite setup only raises test logger verbosity and keeps output on GinkgoWriter; no new stdout writes or stdout-backed loggers were added.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed No new e2e cases or external connectivity were added; the test changes are existing envtest/controller blocks with no IPv4-only URL/IP-family assumptions in new code.
No-Weak-Crypto ✅ Passed No weak-crypto APIs, custom crypto, or non-constant-time secret comparisons appear in the changed code; the diff is only test-flow/logging updates.
Container-Privileges ✅ Passed PR adds only RBAC and a cache-mapper fix; the changed manifests contain no privileged/host* or allowPrivilegeEscalation settings.
No-Sensitive-Data-In-Logs ✅ Passed No new logs with secrets/PII were added; changes only raise test logger verbosity and add a REST mapper to the tracking cache.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@openshift-ci openshift-ci Bot requested review from damdo and theobarberbany July 10, 2026 15:55
@openshift-ci

openshift-ci Bot commented Jul 10, 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 racheljpg 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

@mdbooth

mdbooth commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

/pipeline auto

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification

The pipeline-auto label has been added to this PR. Second-stage tests will be triggered automatically when all first-stage tests pass.

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test e2e-aws-ovn
/test e2e-aws-ovn-serial-1of2
/test e2e-aws-ovn-serial-2of2
/test e2e-aws-capi-disconnected-techpreview
/test e2e-aws-capi-techpreview
/test e2e-aws-capi-techpreview-post-install
/test e2e-aws-ovn-techpreview
/test e2e-aws-ovn-techpreview-upgrade
/test e2e-azure-capi-techpreview
/test e2e-azure-ovn-techpreview
/test e2e-azure-ovn-techpreview-upgrade
/test e2e-gcp-capi-techpreview
/test e2e-gcp-ovn-techpreview
/test e2e-metal3-capi-techpreview
/test e2e-openstack-capi-techpreview
/test e2e-vsphere-capi-techpreview
/test regression-clusterinfra-aws-ipi-techpreview-capi

@mdbooth

mdbooth commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

/testwith openshift/cluster-capi-operator/main/e2e-metal3-capi-techpreview openshift/cluster-api-provider-metal3#86

@mdbooth

mdbooth commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Failed due to a typo in the featuregate name

/testwith openshift/cluster-capi-operator/main/e2e-metal3-capi-techpreview openshift/cluster-api-provider-metal3#86

@mdbooth

mdbooth commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

/testwith openshift/cluster-capi-operator/main/e2e-metal3-capi-techpreview openshift/cluster-api-provider-metal3#86

1 similar comment
@mdbooth

mdbooth commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

/testwith openshift/cluster-capi-operator/main/e2e-metal3-capi-techpreview openshift/cluster-api-provider-metal3#86

@mdbooth

mdbooth commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

For anybody keeping count: these are all infra capacity issues. Prow has not yet managed to execute this job at all. It has not failed.

/testwith openshift/cluster-capi-operator/main/e2e-metal3-capi-techpreview openshift/cluster-api-provider-metal3#86

@mdbooth

mdbooth commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

/testwith openshift/cluster-capi-operator/main/e2e-metal3-capi-techpreview openshift/cluster-api-provider-metal3#86

@mdbooth

mdbooth commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

/retest-required

@mdbooth

mdbooth commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

/test e2e-aws-capi-disconnected-techpreview e2e-metal3-capi-techpreview

@mdbooth

mdbooth commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

/jira refresh

@openshift-ci-robot openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Jul 14, 2026
@openshift-ci-robot

Copy link
Copy Markdown

@mdbooth: This pull request references Jira Issue OCPBUGS-98329, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (5.0.0) matches configured target version for branch (5.0.0)
  • bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)
Details

In response to this:

/jira refresh

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-robot

Copy link
Copy Markdown

@mdbooth: This pull request references Jira Issue OCPBUGS-98329, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (5.0.0) matches configured target version for branch (5.0.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)
Details

In response to this:

Adds openshift-capi-installer:provider-permissions ClusterRole and binds it to capi-installer. This is an aggregated cluster rule which uses the label capi-operator.openshift.io/aggregate-to-installer. If a provider's manifests include any object which the default CAPIO RBAC does not cover they can specify their additional requirements in their own repo as a separate ClusterRole which aggregates to this one.

Also fixes a bug initialising boxcutter's tracking cache which turns missing RBAC into a panic.

Summary by CodeRabbit

  • New Features

  • Added provider-managed installer RBAC via an aggregated cluster-scoped permission model.

  • Installer access is now assembled from approved aggregated roles using a standard label-based mechanism.

  • Bug Fixes

  • Improved managed tracking so installer discovery correctly respects REST mappings across cluster-scoped resources.

  • Documentation

  • Updated the provider contract with a new section detailing default installer permissions and how providers extend them via the aggregated role pattern.

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 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 `@docs/provider-contract.md`:
- Around line 17-27: Update the “Namespaced to openshift-capi-installer” heading
and its namespace reference in the provider contract documentation to use
openshift-cluster-api, matching the RBAC contract and manifests-gen behavior.
Leave the listed resources and aggregated ClusterRole guidance unchanged.
🪄 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: 1975620e-7073-4210-aaa8-6b5b61e59af2

📥 Commits

Reviewing files that changed from the base of the PR and between 5fbc7d6 and 316419f.

📒 Files selected for processing (4)
  • docs/provider-contract.md
  • manifests/0000_30_cluster-api-operator_03_capi-installer-clusterrole.yaml
  • manifests/0000_30_cluster-api-operator_04_capi-installer-clusterrolebinding.yaml
  • pkg/controllers/installer/installer_controller.go

Comment thread docs/provider-contract.md
@mdbooth

mdbooth commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Looks like another flake. I'll investigate it separately.

/test unit

@mdbooth

mdbooth commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

/testwith openshift/cluster-capi-operator/main/e2e-metal3-capi-techpreview openshift/cluster-api-provider-metal3#86

At level 0 we get almost no logs.

@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 `@pkg/controllers/machinesync/machine_sync_controller_test.go`:
- Around line 1487-1488: Insert a blank line between the test machine
construction statements and the following By(...) calls in the affected test
cases, including the sentinel CAPI machine and MAPI machine setup blocks, to
satisfy wsl_v5 whitespace requirements.
🪄 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: 04b8f212-fe73-4766-a130-c6c89b0ca663

📥 Commits

Reviewing files that changed from the base of the PR and between 316419f and d197f7f.

📒 Files selected for processing (7)
  • pkg/controllers/installer/revision_reconciler_test.go
  • pkg/controllers/installer/suite_test.go
  • pkg/controllers/machinemigration/suite_test.go
  • pkg/controllers/machinesetmigration/suite_test.go
  • pkg/controllers/machinesetsync/suite_test.go
  • pkg/controllers/machinesync/machine_sync_controller_test.go
  • pkg/controllers/machinesync/suite_test.go

Comment thread pkg/controllers/machinesync/machine_sync_controller_test.go
@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test e2e-aws-ovn
/test e2e-aws-ovn-serial-1of2
/test e2e-aws-ovn-serial-2of2
/test e2e-aws-capi-disconnected-techpreview
/test e2e-aws-capi-techpreview
/test e2e-aws-capi-techpreview-post-install
/test e2e-aws-ovn-techpreview
/test e2e-aws-ovn-techpreview-upgrade
/test e2e-azure-capi-techpreview
/test e2e-azure-ovn-techpreview
/test e2e-azure-ovn-techpreview-upgrade
/test e2e-gcp-capi-techpreview
/test e2e-gcp-ovn-techpreview
/test e2e-metal3-capi-techpreview
/test e2e-openstack-capi-techpreview
/test e2e-vsphere-capi-techpreview
/test regression-clusterinfra-aws-ipi-techpreview-capi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. pipeline-auto

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants