-
Notifications
You must be signed in to change notification settings - Fork 55
OLS-3546 Dynamically reconcile metrics-reader ClusterRoleBinding namespace #1852
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
0d5def0
675a061
4b5b529
f8d424f
058bc74
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,4 +14,4 @@ subjects: | |
|
|
||
| - kind: ServiceAccount | ||
| name: metrics-reader | ||
| namespace: openshift-lightspeed | ||
| namespace: system | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ import ( | |
| "context" | ||
| "fmt" | ||
| "os" | ||
| "reflect" | ||
| "time" | ||
|
|
||
| "github.com/openshift/lightspeed-operator/internal/controller/reconciler" | ||
|
|
@@ -71,6 +72,10 @@ func ReconcileAppServerResources(r reconciler.Reconciler, ctx context.Context, o | |
| Name: "reconcile Metrics Reader Secret", | ||
| Task: reconcileMetricsReaderSecret, | ||
| }, | ||
| { | ||
| Name: "reconcile Metrics Reader ClusterRoleBinding", | ||
| Task: reconcileMetricsReaderClusterRoleBinding, | ||
| }, | ||
| { | ||
| Name: "reconcile App NetworkPolicy", | ||
| Task: reconcileAppServerNetworkPolicy, | ||
|
|
@@ -405,6 +410,75 @@ func reconcileMetricsReaderSecret(r reconciler.Reconciler, ctx context.Context, | |
| return nil | ||
| } | ||
|
|
||
| // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterrolebindings,verbs=get;create;update | ||
| func reconcileMetricsReaderClusterRoleBinding(r reconciler.Reconciler, ctx context.Context, cr *olsv1alpha1.OLSConfig) error { | ||
| if os.Getenv("LOCAL_DEV_MODE") == "true" { | ||
| r.GetLogger().Info("Skipping metrics reader ClusterRoleBinding reconciliation in LOCAL_DEV_MODE") | ||
| return nil | ||
| } | ||
|
|
||
| desired, err := generateMetricsReaderClusterRoleBinding(r, cr) | ||
| if err != nil { | ||
| return fmt.Errorf("%s: %w", utils.ErrGenerateMetricsReaderCRB, err) | ||
| } | ||
|
|
||
| found := &rbacv1.ClusterRoleBinding{} | ||
| err = r.Get(ctx, client.ObjectKey{Name: desired.Name}, found) | ||
| if err != nil && errors.IsNotFound(err) { | ||
| r.GetLogger().Info("creating metrics reader ClusterRoleBinding", "ClusterRoleBinding", desired.Name) | ||
| err = r.Create(ctx, desired) | ||
| if err != nil { | ||
| return fmt.Errorf("%s: %w", utils.ErrCreateMetricsReaderCRB, err) | ||
| } | ||
| return nil | ||
| } else if err != nil { | ||
| return fmt.Errorf("%s: %w", utils.ErrGetMetricsReaderCRB, err) | ||
| } | ||
|
|
||
| // RoleRef is immutable in Kubernetes, so a mismatch can only be fixed by | ||
| // deleting and recreating the ClusterRoleBinding. | ||
| if found.RoleRef != desired.RoleRef { | ||
| r.GetLogger().Info("recreating metrics reader ClusterRoleBinding due to RoleRef mismatch", | ||
| "ClusterRoleBinding", found.Name, "foundRoleRef", found.RoleRef, "desiredRoleRef", desired.RoleRef) | ||
| if err := r.Delete(ctx, found); err != nil { | ||
| return fmt.Errorf("%s: %w", utils.ErrDeleteMetricsReaderCRB, err) | ||
| } | ||
| if err := r.Create(ctx, desired); err != nil { | ||
| return fmt.Errorf("%s: %w", utils.ErrCreateMetricsReaderCRB, err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| needsUpdate := false | ||
| if !reflect.DeepEqual(found.Subjects, desired.Subjects) { | ||
| found.Subjects = desired.Subjects | ||
| needsUpdate = true | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Upgrade path: OwnerReferences and Labels not adopted When upgrading an existing install, the CRB was created by kustomize and has no After the update, the CRB on the cluster still has no owner reference. This breaks:
Suggested fix — also reconcile OwnerReferences and Labels in the update path: needsUpdate := false
if !reflect.DeepEqual(found.Subjects, desired.Subjects) {
found.Subjects = desired.Subjects
needsUpdate = true
}
if !reflect.DeepEqual(found.OwnerReferences, desired.OwnerReferences) {
found.OwnerReferences = desired.OwnerReferences
needsUpdate = true
}
if !reflect.DeepEqual(found.Labels, desired.Labels) {
found.Labels = desired.Labels
needsUpdate = true
}Additionally, if found.RoleRef != desired.RoleRef {
// RoleRef is immutable; must delete and recreate
if err := r.Delete(ctx, found); err != nil {
return fmt.Errorf("failed to delete CRB with mismatched RoleRef: %w", err)
}
return r.Create(ctx, desired)
} |
||
|
|
||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| if !reflect.DeepEqual(found.OwnerReferences, desired.OwnerReferences) { | ||
| found.OwnerReferences = desired.OwnerReferences | ||
| needsUpdate = true | ||
| } | ||
|
|
||
| if !reflect.DeepEqual(found.Labels, desired.Labels) { | ||
| found.Labels = desired.Labels | ||
| needsUpdate = true | ||
| } | ||
|
|
||
| if needsUpdate { | ||
| r.GetLogger().Info("updating metrics reader ClusterRoleBinding", | ||
| "ClusterRoleBinding", found.Name, "namespace", r.GetNamespace()) | ||
| err = r.Update(ctx, found) | ||
| if err != nil { | ||
| return fmt.Errorf("%s: %w", utils.ErrUpdateMetricsReaderCRB, err) | ||
| } | ||
| } else { | ||
| r.GetLogger().Info("metrics reader ClusterRoleBinding reconciled", "ClusterRoleBinding", found.Name) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| func reconcileServiceMonitor(r reconciler.Reconciler, ctx context.Context, cr *olsv1alpha1.OLSConfig) error { | ||
| sm, err := GenerateServiceMonitor(r, cr) | ||
| if err != nil { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use
utils.DefaultLabels()for consistent labeling.The labels in this
ClusterRoleBindingare hardcoded. As per coding guidelines, useutils.DefaultLabels()for consistent labeling in asset generation functions.🤖 Prompt for AI Agents
Source: Coding guidelines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.