OCPCLOUD-3347: Add RBAC for machine-approver to read APIServer TLS profile#7802
OCPCLOUD-3347: Add RBAC for machine-approver to read APIServer TLS profile#7802bryan-cox wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
WalkthroughAdds RBAC reconciliation for the machine-approver: hostedcluster controller now ensures a ClusterRole and ClusterRoleBinding granting get/list/watch on config.openshift.io/apiservers to the Changes
Sequence Diagram(s)sequenceDiagram
participant HC as HostedCluster Reconciler
participant K8s as Kubernetes API (RBAC)
participant HCP as HostedControlPlane namespace / ServiceAccount
HC->>HC: check DisableMachineManagement annotation
alt machine management enabled
HC->>K8s: Ensure ClusterRole (get/list/watch apiservers)
K8s-->>HC: ClusterRole created/updated
HC->>K8s: Ensure ClusterRoleBinding -> subject: SA in HCP
K8s-->>HC: ClusterRoleBinding created/updated
note right of HCP: ServiceAccount referenced (may be created elsewhere)
else disabled
HC-->>HC: skip RBAC reconciliation
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
approved
bryan-cox
force-pushed
the
OCPBUGS-machine-approver-rbac
branch
from
7719314 to
dda83db
Compare
…PIServer TLS profile The cluster-machine-approver binary now calls FetchAPIServerTLSProfile() at startup using its in-cluster (management cluster) client, which requires get/list/watch access to the cluster-scoped apiservers.config.openshift.io resource. This was introduced by openshift/cluster-machine-approver PR #286. The existing machine-approver RBAC in HyperShift is a namespace-scoped Role that only grants access to cluster.x-k8s.io/machines. Since apiservers is cluster-scoped, a ClusterRole and ClusterRoleBinding are needed. Following the existing pattern used by the PKI operator and KubeVirt CSI driver, the cluster-scoped RBAC is created by the hypershift-operator's HostedCluster controller, which has the necessary permissions. Changes: - Add reconcileMachineApproverClusterRBAC to the HostedCluster controller creating a ClusterRole and ClusterRoleBinding per HostedCluster - Gate RBAC creation on DisableMachineManagement annotation, matching the CPO's machine-approver component predicate - Add MachineApproverServiceAccount helper in cpomanifests for consistency with PKIOperatorServiceAccount and KubevirtCSIDriverInfraSA Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bryan-cox
force-pushed
the
OCPBUGS-machine-approver-rbac
branch
from
dda83db to
e3662b5
Compare
openshift-ci
bot
added
the
area/control-plane-operator
bryan-cox
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@bryan-cox: This pull request references OCPCLOUD-3347 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 "4.22.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. |
|
@bryan-cox: This pull request references OCPCLOUD-3347 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 "4.22.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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
control-plane-operator/controllers/hostedcontrolplane/manifests/machineapprover.go (1)
8-15: LGTM — consider extracting the name literal as a package-level constant.The helper is correctly minimal and follows the established manifest-helper pattern in this package. Optionally, extracting
"machine-approver"as a named constant would keep all references (ClusterRole/ClusterRoleBinding name prefix, RBAC subject, service-account name) consistent and refactor-safe. Note that a duplicate constantComponentName = "machine-approver"already exists incontrol-plane-operator/controllers/hostedcontrolplane/v2/machine_approver/component.go; consolidating by defining the constant in themanifestspackage and having the component reference it would eliminate duplication.♻️ Proposed optional refactor
package manifests import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +const MachineApproverName = "machine-approver" + func MachineApproverServiceAccount(ns string) *corev1.ServiceAccount { return &corev1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{ - Name: "machine-approver", + Name: MachineApproverName, Namespace: ns, }, } }Then reference
manifests.MachineApproverNamewherever"machine-approver"appears, and updatev2/machine_approver/component.goto import and use the constant from manifests to eliminate duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/manifests/machineapprover.go` around lines 8 - 15, Extract the literal "machine-approver" into a package-level constant (e.g., MachineApproverName) in the manifests package and replace the literal in MachineApproverServiceAccount (and any other manifest helpers in the same package) with that constant; then update control-plane-operator/controllers/hostedcontrolplane/v2/machine_approver/component.go to import the manifests package and use manifests.MachineApproverName instead of its local ComponentName to eliminate the duplicate constant and keep ClusterRole/ClusterRoleBinding names, RBAC subjects, and service-account name consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/manifests/machineapprover.go`:
- Around line 8-15: Extract the literal "machine-approver" into a package-level
constant (e.g., MachineApproverName) in the manifests package and replace the
literal in MachineApproverServiceAccount (and any other manifest helpers in the
same package) with that constant; then update
control-plane-operator/controllers/hostedcontrolplane/v2/machine_approver/component.go
to import the manifests package and use manifests.MachineApproverName instead of
its local ComponentName to eliminate the duplicate constant and keep
ClusterRole/ClusterRoleBinding names, RBAC subjects, and service-account name
consistent.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
control-plane-operator/controllers/hostedcontrolplane/manifests/machineapprover.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
|
@bryan-cox: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
This was the issue openshift/cluster-machine-approver#286 (review) |
3 participants
Summary
Add cluster-scoped RBAC for the machine-approver to read
apiservers.config.openshift.io, required after openshift/cluster-machine-approver PR #286 introducedFetchAPIServerTLSProfile().Problem
The
cluster-machine-approverbinary now callsFetchAPIServerTLSProfile()at startup using its in-cluster (management cluster) client, which readsapiservers.config.openshift.io/cluster. HyperShift's existing machine-approver RBAC is a namespace-scopedRolegranting onlycluster.x-k8s.io/machinesaccess. Sinceapiserversis cluster-scoped, the machine-approver crashes with:This breaks all HyperShift e2e tests using 4.22 CI nightlies built after 2026-02-25T08:01Z (evidence).
Changes
reconcileMachineApproverClusterRBACin the HostedCluster controller: creates aClusterRoleandClusterRoleBinding(named<hcp-namespace>-machine-approver) grantingget,list,watchonapiserversinconfig.openshift.ioDisableMachineManagementguard: skips RBAC creation when the annotation is set, matching the CPO's machine-approver component predicateMachineApproverServiceAccounthelper: newcpomanifests.MachineApproverServiceAccount()for consistency withPKIOperatorServiceAccount()andKubevirtCSIDriverInfraSA()Follows the existing pattern used by the PKI operator and KubeVirt CSI driver for cluster-scoped RBAC created by the hypershift-operator (since the CPO lacks cluster-scoped permissions).
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit