OLS-3546 Dynamically reconcile metrics-reader ClusterRoleBinding namespace - #1852
OLS-3546 Dynamically reconcile metrics-reader ClusterRoleBinding namespace#1852thoraxe wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThe change updates the OpenShift MCP service URL and CA configuration, adds metrics reader ClusterRoleBinding generation and reconciliation, changes the Prometheus binding namespace, and adds related tests and RBAC error constants. ChangesAppserver resource configuration and reconciliation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ReconcileAppServerResources
participant reconcileMetricsReaderClusterRoleBinding
participant KubernetesAPI
ReconcileAppServerResources->>reconcileMetricsReaderClusterRoleBinding: reconcile metrics reader ClusterRoleBinding
reconcileMetricsReaderClusterRoleBinding->>KubernetesAPI: get ClusterRoleBinding
KubernetesAPI-->>reconcileMetricsReaderClusterRoleBinding: existing or not found
reconcileMetricsReaderClusterRoleBinding->>KubernetesAPI: create or update desired subjects
KubernetesAPI-->>reconcileMetricsReaderClusterRoleBinding: reconciliation result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@internal/controller/appserver/assets.go`:
- Around line 998-1028: Update generateMetricsReaderClusterRoleBinding to
initialize its ObjectMeta.Labels with utils.DefaultLabels() instead of hardcoded
label values, preserving the existing resource name and all other
ClusterRoleBinding fields.
In `@internal/controller/appserver/reconciler.go`:
- Around line 444-460: Update the subject reconciliation logic around the
MetricsReaderServiceAccountName check to compare found.Subjects with
desired.Subjects using the repository’s standard semantic equality helper. When
they differ, replace found.Subjects with desired.Subjects and mark needsUpdate;
remove the separate subjectFound and namespace-only handling so the controller
consistently enforces the exact desired subject list.
- Around line 420-474: Add a kubebuilder RBAC marker in the reconciler’s
controller RBAC declarations granting get, create, and update permissions for
clusterrolebindings, covering the operations performed by
reconcileMetricsReaderClusterRoleBinding. Use the existing RBAC marker style and
preserve the current reconciliation logic.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 046f709e-8431-4d4e-9546-fc6937efe838
📒 Files selected for processing (7)
config/prometheus/clusterrole_binding.yamlinternal/controller/appserver/assets.gointernal/controller/appserver/assets_test.gointernal/controller/appserver/reconciler.gointernal/controller/appserver/reconciler_test.gointernal/controller/utils/constants.gointernal/controller/utils/errors.go
| func generateMetricsReaderClusterRoleBinding(r reconciler.Reconciler, cr *olsv1alpha1.OLSConfig) (*rbacv1.ClusterRoleBinding, error) { | ||
| rb := rbacv1.ClusterRoleBinding{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: utils.MetricsReaderClusterRoleBindingName, | ||
| Labels: map[string]string{ | ||
| "app.kubernetes.io/name": "clusterrolebinding", | ||
| "app.kubernetes.io/component": "metrics", | ||
| "app.kubernetes.io/part-of": "lightspeed-operator", | ||
| }, | ||
| }, | ||
| Subjects: []rbacv1.Subject{ | ||
| { | ||
| Kind: "ServiceAccount", | ||
| Name: utils.MetricsReaderServiceAccountName, | ||
| Namespace: r.GetNamespace(), | ||
| }, | ||
| }, | ||
| RoleRef: rbacv1.RoleRef{ | ||
| APIGroup: "rbac.authorization.k8s.io", | ||
| Kind: "ClusterRole", | ||
| Name: utils.MetricsReaderClusterRoleName, | ||
| }, | ||
| } | ||
|
|
||
| if err := controllerutil.SetControllerReference(cr, &rb, r.GetScheme()); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &rb, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use utils.DefaultLabels() for consistent labeling.
The labels in this ClusterRoleBinding are hardcoded. As per coding guidelines, use utils.DefaultLabels() for consistent labeling in asset generation functions.
🤖 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 `@internal/controller/appserver/assets.go` around lines 998 - 1028, Update
generateMetricsReaderClusterRoleBinding to initialize its ObjectMeta.Labels with
utils.DefaultLabels() instead of hardcoded label values, preserving the existing
resource name and all other ClusterRoleBinding fields.
Source: Coding guidelines
There was a problem hiding this comment.
No DefaultLabels() function exists in this codebase. The closest analog (generateSARClusterRoleBinding) has no labels at all. The labels here match the kustomize source for consistency with the OLM-created resource. No change needed.
openshift-ci
Bot
added
the
needs-rebase
a1df16f to
0d5def0
Compare
openshift-ci
Bot
removed
the
needs-rebase
- Use utils.GenerateAppServerSelectorLabels() instead of hardcoded labels in generateMetricsReaderClusterRoleBinding (consistent labeling per coding guidelines) - Add kubebuilder:rbac marker for clusterrolebindings get/create/update at the reconcileMetricsReaderClusterRoleBinding function - Extend CRB generation test to assert on Labels field Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/controller/appserver/reconciler_test.go (1)
455-484: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover a non-default operator namespace.
These assertions only exercise
utils.OLSNamespaceDefault; a hardcoded namespace regression would pass. Reconcile with a sharedTestReconcilerconfigured for a custom namespace and assert the generated subject uses it.As per coding guidelines, use shared controller test utilities and fixtures where appropriate, including
TestReconcilerandNewTestReconciler.🤖 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 `@internal/controller/appserver/reconciler_test.go` around lines 455 - 484, Update the metrics reader ClusterRoleBinding tests to use a shared TestReconciler configured with a non-default operator namespace via NewTestReconciler, rather than relying on utils.OLSNamespaceDefault. Reconcile using that fixture and assert both creation and namespace correction in the tests around the metrics reader ClusterRoleBinding cases, verifying the subject namespace matches the configured custom namespace.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 `@internal/controller/appserver/assets.go`:
- Around line 1027-1028: In the owner-reference setup within the metrics reader
reconciliation flow, add the shared ErrSetMetricsReaderCRBOwnerReference
constant to internal/controller/utils/errors.go, then wrap the error returned by
controllerutil.SetControllerReference using the project’s fmt.Errorf("%s: %w",
ErrConstant, err) convention before returning it.
---
Nitpick comments:
In `@internal/controller/appserver/reconciler_test.go`:
- Around line 455-484: Update the metrics reader ClusterRoleBinding tests to use
a shared TestReconciler configured with a non-default operator namespace via
NewTestReconciler, rather than relying on utils.OLSNamespaceDefault. Reconcile
using that fixture and assert both creation and namespace correction in the
tests around the metrics reader ClusterRoleBinding cases, verifying the subject
namespace matches the configured custom namespace.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 386cf600-b24a-40f0-84f0-2e7e743b4f46
📒 Files selected for processing (7)
config/prometheus/clusterrole_binding.yamlinternal/controller/appserver/assets.gointernal/controller/appserver/assets_test.gointernal/controller/appserver/reconciler.gointernal/controller/appserver/reconciler_test.gointernal/controller/utils/constants.gointernal/controller/utils/errors.go
| if err := controllerutil.SetControllerReference(cr, &rb, r.GetScheme()); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Wrap the owner-reference error with a shared constant.
Add ErrSetMetricsReaderCRBOwnerReference in internal/controller/utils/errors.go and wrap this failure before returning it.
Proposed fix
- if err := controllerutil.SetControllerReference(cr, &rb, r.GetScheme()); err != nil {
- return nil, err
+ if err := controllerutil.SetControllerReference(cr, &rb, r.GetScheme()); err != nil {
+ return nil, fmt.Errorf("%s: %w", utils.ErrSetMetricsReaderCRBOwnerReference, err)
}As per coding guidelines, “Wrap errors using fmt.Errorf("%s: %w", ErrConstant, err)”; as per path instructions, errors must use shared constants from internal/controller/utils/errors.go.
🤖 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 `@internal/controller/appserver/assets.go` around lines 1027 - 1028, In the
owner-reference setup within the metrics reader reconciliation flow, add the
shared ErrSetMetricsReaderCRBOwnerReference constant to
internal/controller/utils/errors.go, then wrap the error returned by
controllerutil.SetControllerReference using the project’s fmt.Errorf("%s: %w",
ErrConstant, err) convention before returning it.
Sources: Coding guidelines, Path instructions
|
@thoraxe: all tests passed! 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. |
1 participant
Summary
r.GetNamespace()so the subject namespace always matches the actual install namespacesystemplaceholder instead of the hardcodedopenshift-lightspeednamespaceFixes metrics scraping (403 Forbidden /
up == 0) when the operator is installed outsideopenshift-lightspeed(e.g. AppSRE'slightspeed-operatornamespace).Test plan
make test— 11 suites, 0 failures)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
system.