OCPBUGS-98219: Move RBAC manifests to the services that own them#7029
OCPBUGS-98219: Move RBAC manifests to the services that own them#7029pacevedom wants to merge 3 commits into
Conversation
openshift-ci-robot
added
jira/severity-important
|
@pacevedom: This pull request references Jira Issue OCPBUGS-98219, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
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:
WalkthroughControllers now apply startup resources in an ordered sequence. Cluster-policy-controller applies RBAC and waits for namespace security allocation before readiness, infrastructure services depend on it and no longer apply default RBAC, and kube-controller-manager applies namespaces and CSR approver resources step by step. ChangesController startup sequencing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ServiceManager
participant ClusterPolicyController
participant KubernetesAPI
participant InfrastructureServicesManager
ServiceManager->>ClusterPolicyController: Register and start
ClusterPolicyController->>KubernetesAPI: Apply RBAC resources
ClusterPolicyController->>KubernetesAPI: Poll default namespace security annotation
KubernetesAPI-->>ClusterPolicyController: Return UID range annotation
ClusterPolicyController-->>InfrastructureServicesManager: Signal readiness
InfrastructureServicesManager->>KubernetesAPI: Apply PriorityClass
InfrastructureServicesManager->>InfrastructureServicesManager: Start components
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@pacevedom: This pull request references Jira Issue OCPBUGS-98219, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pacevedom The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/controllers/kube-controller-manager.go (1)
119-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap each apply failure with the asset phase.
The sequence is clearer now, but bare returns make startup failures harder to triage. Match the nearby controller pattern by adding step-specific context.
Suggested fix
}, kubeconfigPath); err != nil { - return err + return fmt.Errorf("failed to apply kube-controller-manager namespaces: %w", err) } if err := assets.ApplyClusterRoles(ctx, []string{ "controllers/kube-controller-manager/csr_approver_clusterrole.yaml", }, kubeconfigPath); err != nil { - return err + return fmt.Errorf("failed to apply kube-controller-manager cluster roles: %w", err) } - return assets.ApplyClusterRoleBindings(ctx, []string{ + if err := assets.ApplyClusterRoleBindings(ctx, []string{ "controllers/kube-controller-manager/csr_approver_clusterrolebinding.yaml", - }, kubeconfigPath) + }, kubeconfigPath); err != nil { + return fmt.Errorf("failed to apply kube-controller-manager cluster role bindings: %w", err) + } + return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controllers/kube-controller-manager.go` around lines 119 - 128, The startup path in kube-controller-manager currently returns raw errors from assets.ApplyClusterRoles and assets.ApplyClusterRoleBindings, which makes it harder to tell which asset phase failed. Update the error handling in the controller setup flow to wrap each failure with step-specific context, following the same pattern used by nearby controller code, so the returned error clearly identifies the apply phase and the failing asset group.
🤖 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/cluster-policy-controller.go`:
- Line 105: The RBAC bootstrap in applyClusterPolicyControllerRBAC is using the
controller runtime kubeconfig, which makes startup depend on the controller
already having elevated permissions. Update the call site in
cluster-policy-controller startup to pass config.KubeAdmin instead of
s.kubeconfig so the ClusterRoles/Bindings are created with the admin kubeconfig.
---
Nitpick comments:
In `@pkg/controllers/kube-controller-manager.go`:
- Around line 119-128: The startup path in kube-controller-manager currently
returns raw errors from assets.ApplyClusterRoles and
assets.ApplyClusterRoleBindings, which makes it harder to tell which asset phase
failed. Update the error handling in the controller setup flow to wrap each
failure with step-specific context, following the same pattern used by nearby
controller code, so the returned error clearly identifies the apply phase and
the failing asset group.
🪄 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: 1cee39be-b597-485c-9d36-88701c2107fb
📒 Files selected for processing (3)
pkg/controllers/cluster-policy-controller.gopkg/controllers/infra-services-controller.gopkg/controllers/kube-controller-manager.go
💤 Files with no reviewable changes (1)
- pkg/controllers/infra-services-controller.go
coderabbitai
Bot
added
the
ready-for-human-review
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 `@pkg/controllers/kube-controller-manager.go`:
- Around line 114-128: The error wrapper in applyFn is stale because it now
covers namespace, ClusterRole, and ClusterRoleBinding application via
assets.ApplyNamespaces, assets.ApplyClusterRoles, and
assets.ApplyClusterRoleBindings. Update the wrapping message near applyFn so it
accurately describes the full set of resources being applied, instead of saying
only “openshift namespaces,” to avoid misattributing RBAC failures during
triage.
🪄 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: 956d4fab-469d-42c5-8362-0ccd93fe7587
📒 Files selected for processing (3)
pkg/controllers/cluster-policy-controller.gopkg/controllers/infra-services-controller.gopkg/controllers/kube-controller-manager.go
💤 Files with no reviewable changes (1)
- pkg/controllers/infra-services-controller.go
|
/retest |
The cluster-policy-controller RBAC was applied by infrastructure-services-manager, forcing cluster-policy-controller to depend on it. This created a deadlock on restart: infrastructure-services-manager creates deployments whose SCC admission blocks for up to 13s waiting for namespace UID-range annotations, but those annotations are set by the namespace-security-allocation-controller inside cluster-policy-controller, which cannot start until infrastructure-services-manager finishes. On a fresh boot this was masked because target namespaces did not exist yet and the deployment create timed out at the client side, allowing a retry after the annotations were set. On restart the namespaces already exist, so the deployment create goes straight to admission and the deadlock is exposed. Move each service's RBAC into its own Run method: - cluster-policy-controller applies its 6 RBAC manifests and drops the infrastructure-services-manager dependency, restoring the original dependency on kube-apiserver only - kube-controller-manager applies its CSR approver RBAC alongside its own namespaces - infrastructure-services-manager no longer applies RBAC for other services Bug: https://issues.redhat.com/browse/OCPBUGS-98219
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controllers/kube-controller-manager.go (1)
114-131: 📐 Maintainability & Code Quality | 🔵 TrivialAdd Robot coverage for the new KCM RBAC startup path.
Add a Robot test under
test/suites/that restarts MicroShift and asserts the CSR approver ClusterRole and ClusterRoleBinding are applied duringkube-controller-managerstartup.🤖 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 `@pkg/controllers/kube-controller-manager.go` around lines 114 - 131, Add Robot coverage for the new kube-controller-manager RBAC startup path by creating a test under test/suites/ that restarts MicroShift and verifies the csr_approver_clusterrole and csr_approver_clusterrolebinding are present after startup. Use the existing kube-controller-manager startup flow in kube-controller-manager.go as the behavior under test, and assert the resources applied via the same names used by assets.ApplyClusterRoles and assets.ApplyClusterRoleBindings.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.
Nitpick comments:
In `@pkg/controllers/kube-controller-manager.go`:
- Around line 114-131: Add Robot coverage for the new kube-controller-manager
RBAC startup path by creating a test under test/suites/ that restarts MicroShift
and verifies the csr_approver_clusterrole and csr_approver_clusterrolebinding
are present after startup. Use the existing kube-controller-manager startup flow
in kube-controller-manager.go as the behavior under test, and assert the
resources applied via the same names used by assets.ApplyClusterRoles and
assets.ApplyClusterRoleBindings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3c5d054b-0593-44f6-8e94-52d65c426997
📒 Files selected for processing (3)
pkg/controllers/cluster-policy-controller.gopkg/controllers/infra-services-controller.gopkg/controllers/kube-controller-manager.go
💤 Files with no reviewable changes (1)
- pkg/controllers/infra-services-controller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controllers/cluster-policy-controller.go
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/test e2e-aws-tests-arm |
The previous commit broke the circular dependency but left a race: infrastructure-services-manager could still submit deployment creates before cluster-policy-controller had annotated namespaces with openshift.io/sa.scc.uid-range, depending on CPU speed. Start the controller in a goroutine and poll the default namespace for the UID range annotation before signaling ready. Add cluster-policy-controller as a dependency of infrastructure-services-manager so that deployment creates are guaranteed to pass SCC admission regardless of system load. Bug: https://issues.redhat.com/browse/OCPBUGS-98219
coderabbitai
Bot
removed
the
ready-for-human-review
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controllers/cluster-policy-controller.go (1)
155-172: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo bounded timeout — readiness can hang indefinitely.
PollUntilContextCancelonly returns when the parentctxis cancelled or the annotation appears. Ifnamespace-security-allocation-controllernever annotatesdefault,Runnever closesready, stalling dependents (infrastructure-services-manager) until shutdown with no diagnostic. Consider deriving a boundedcontext.WithTimeoutand returning a clear error on expiry. The swallowedGeterror (return false, nil) is fine for polling, but logging it at high verbosity would aid triage.🤖 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 `@pkg/controllers/cluster-policy-controller.go` around lines 155 - 172, The readiness check in waitForNamespaceSecurityAllocation can hang forever because PollUntilContextCancel only stops on parent cancellation or when the annotation appears. Update this helper to derive a bounded context with timeout, pass it into the polling loop, and return a clear timeout error if the default namespace never gets securityv1.UIDRangeAnnotation. Keep the current retry behavior for kubeClient.CoreV1().Namespaces().Get, but consider logging the Get error at high verbosity for easier triage.Source: Path instructions
🤖 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.
Nitpick comments:
In `@pkg/controllers/cluster-policy-controller.go`:
- Around line 155-172: The readiness check in waitForNamespaceSecurityAllocation
can hang forever because PollUntilContextCancel only stops on parent
cancellation or when the annotation appears. Update this helper to derive a
bounded context with timeout, pass it into the polling loop, and return a clear
timeout error if the default namespace never gets securityv1.UIDRangeAnnotation.
Keep the current retry behavior for kubeClient.CoreV1().Namespaces().Get, but
consider logging the Get error at high verbosity for easier triage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b3ce4221-bf19-4bfc-aff7-2459d65520da
📒 Files selected for processing (2)
pkg/controllers/cluster-policy-controller.gopkg/controllers/infra-services-controller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controllers/infra-services-controller.go
…ager The service manager requires dependencies to be registered before the services that reference them. Since infrastructure-services-manager now depends on cluster-policy-controller, the registration order must be swapped. Bug: https://issues.redhat.com/browse/OCPBUGS-98219
|
/test ocp-full-conformance-rhel-eus |
|
/retest |
1 similar comment
|
/retest |
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@pacevedom: This PR has been marked as verified by 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. |
|
/retest |
|
@pacevedom: The following tests 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. |
|
|
||
| type ClusterPolicyController struct { | ||
| run func(context.Context) error | ||
| applyRBAC func(context.Context) error |
There was a problem hiding this comment.
why is applyRbac a func literal? It should be an unexported package func to reduce complexity.
| openshiftcontrolplanev1 "github.com/openshift/api/openshiftcontrolplane/v1" | ||
| securityv1 "github.com/openshift/api/security/v1" | ||
| clusterpolicycontroller "github.com/openshift/cluster-policy-controller/pkg/cmd/cluster-policy-controller" | ||
| "github.com/openshift/library-go/pkg/controller/controllercmd" | ||
| "github.com/openshift/library-go/pkg/operator/events" | ||
| "github.com/openshift/microshift/pkg/assets" | ||
| "github.com/openshift/microshift/pkg/config" | ||
| corev1 "k8s.io/api/core/v1" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| unstructuredv1 "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | ||
| "k8s.io/apimachinery/pkg/runtime" | ||
| "k8s.io/apimachinery/pkg/runtime/serializer" | ||
| "k8s.io/apimachinery/pkg/util/wait" | ||
| "k8s.io/client-go/kubernetes" | ||
| "k8s.io/client-go/tools/clientcmd" | ||
| klog "k8s.io/klog/v2" | ||
| "k8s.io/utils/clock" |
There was a problem hiding this comment.
Please refactor to match openshift import conventions
|
/retest |
3 participants
Context
When the el98-lrel@multi-config-standard2 scenario was added to test TLS 1.3 configuration on ARM, it exposed a startup failure on restart: MicroShift would stop, apply a TLS config
drop-in, start again, and crash with admission plugin "PodSecurity" failed to complete validation in 13s. The same failure was reported in OCPBUGS-33701 (closed as "Not a Bug") when
changing memoryLimitMB on ARM.
What's actually happening
The SCC admission plugin has two sequential polling loops when validating a deployment create:
That annotation is set by namespace-security-allocation-controller, which runs inside cluster-policy-controller. But cluster-policy-controller depends on infrastructure-services-manager —
the same service that's trying to create the deployments. This is a deadlock.
On fresh boot it's masked: the target namespaces don't exist yet, so the deployment CREATE hits a client-side HTTP timeout (~1.4s), the handler retries, and by then
cluster-policy-controller has started and annotated the namespace. On restart the namespaces already exist, so the CREATE goes straight to SCC admission, the 10s poll exhausts, and
MicroShift exits.
How the dependency got inverted
cluster-policy-controller was originally introduced (3a28b7a, July 2022) with only kube-apiserver as a dependency. Its RBAC manifests (6 ClusterRoles/ClusterRoleBindings for the
namespace-security-allocation and PSA label syncer controllers) were placed in infrastructure-services-manager's applyDefaultRBACs alongside unrelated kube-controller-manager RBAC.
Later (84ee036, September 2023), cluster-policy-controller was failing on startup because its RBAC didn't exist yet. Rather than moving the RBAC into the service that owns it, the fix
was to add infrastructure-services-manager as a dependency — creating the circular dependency.
What this PR does
Move RBAC to owning services: applyDefaultRBACs was a grab bag applying RBAC for multiple services. Each service now applies its own RBAC in its Run method:
Add a real readiness check to cluster-policy-controller: the old code had close(ready) // todo — signaling ready before doing any work. Now it starts the controller in a goroutine and
polls until the default namespace has the openshift.io/sa.scc.uid-range annotation, proving the namespace-security-allocation-controller is actually running.
Add cluster-policy-controller as a dependency of infrastructure-services-manager: this guarantees the annotations exist before any deployment creates, regardless of CPU speed or system
load. The dependency is safe because cluster-policy-controller now only depends on kube-apiserver — no cycle.
Startup performance
The hidden cost of the old design was always there — on every fresh boot, infrastructure-services-manager took ~22s because the SCC admission was silently polling for 10s on each
deployment create waiting for annotations that hadn't been set yet. With the fix, it takes ~3.4s.
Measured on same-arch x86 e2e-aws-tests, el98-src@standard-suite1 (two baselines from PRs #7015 and #7027 vs this PR):
Summary by CodeRabbit