Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion config/prometheus/clusterrole_binding.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,4 @@ subjects:

- kind: ServiceAccount
name: metrics-reader
namespace: openshift-lightspeed
namespace: system
27 changes: 27 additions & 0 deletions internal/controller/appserver/assets.go
Original file line number Diff line number Diff line change
Expand Up @@ -1004,6 +1004,33 @@ func GenerateMetricsReaderSecret(r reconciler.Reconciler, cr *olsv1alpha1.OLSCon
return secret, nil
}

func generateMetricsReaderClusterRoleBinding(r reconciler.Reconciler, cr *olsv1alpha1.OLSConfig) (*rbacv1.ClusterRoleBinding, error) {
rb := rbacv1.ClusterRoleBinding{
ObjectMeta: metav1.ObjectMeta{
Name: utils.MetricsReaderClusterRoleBindingName,
Labels: utils.GenerateAppServerSelectorLabels(),
},
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, fmt.Errorf("%s: %w", utils.ErrSetMetricsReaderCRBOwnerReference, err)
}

return &rb, nil
}

Comment on lines +1007 to +1033

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Contributor Author

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.

func serviceTLSProfileType(profileType configv1.TLSProfileType) string {
switch profileType {
case configv1.TLSProfileOldType:
Expand Down
13 changes: 13 additions & 0 deletions internal/controller/appserver/assets_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1661,6 +1661,19 @@ var _ = Describe("App server assets", func() {
Expect(secret.Type).To(Equal(corev1.SecretTypeServiceAccountToken))
})

It("should generate the metrics reader ClusterRoleBinding", func() {
rb, err := generateMetricsReaderClusterRoleBinding(testReconcilerInstance, cr)
Expect(err).NotTo(HaveOccurred())
Expect(rb.Name).To(Equal(utils.MetricsReaderClusterRoleBindingName))
Expect(rb.Labels).To(Equal(utils.GenerateAppServerSelectorLabels()))
Expect(rb.RoleRef.Kind).To(Equal("ClusterRole"))
Expect(rb.RoleRef.Name).To(Equal(utils.MetricsReaderClusterRoleName))
Expect(rb.Subjects).To(HaveLen(1))
Expect(rb.Subjects[0].Kind).To(Equal("ServiceAccount"))
Expect(rb.Subjects[0].Name).To(Equal(utils.MetricsReaderServiceAccountName))
Expect(rb.Subjects[0].Namespace).To(Equal(utils.OLSNamespaceDefault))
})

It("should generate the OLS prometheus rules", func() {
prometheusRule, err := GeneratePrometheusRule(testReconcilerInstance, cr)
Expect(err).NotTo(HaveOccurred())
Expand Down
50 changes: 50 additions & 0 deletions internal/controller/appserver/reconciler.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"context"
"fmt"
"os"
"reflect"
"time"

"github.com/openshift/lightspeed-operator/internal/controller/reconciler"
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -405,6 +410,51 @@ 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)
}

needsUpdate := false
if !reflect.DeepEqual(found.Subjects, desired.Subjects) {
found.Subjects = desired.Subjects
needsUpdate = true
}

Comment thread
coderabbitai[bot] marked this conversation as resolved.
if needsUpdate {
r.GetLogger().Info("updating metrics reader ClusterRoleBinding subject namespace",
"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
}

Comment thread
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 {
Expand Down
31 changes: 31 additions & 0 deletions internal/controller/appserver/reconciler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,37 @@ var _ = Describe("App server reconciliator", Ordered, func() {
Expect(err).NotTo(HaveOccurred())
})

It("should create a metrics reader ClusterRoleBinding with correct namespace", func() {
By("Get the metrics reader ClusterRoleBinding")
rb := &rbacv1.ClusterRoleBinding{}
err := k8sClient.Get(ctx, client.ObjectKey{Name: utils.MetricsReaderClusterRoleBindingName}, rb)
Expect(err).NotTo(HaveOccurred())
Expect(rb.Subjects).To(HaveLen(1))
Expect(rb.Subjects[0].Name).To(Equal(utils.MetricsReaderServiceAccountName))
Expect(rb.Subjects[0].Namespace).To(Equal(utils.OLSNamespaceDefault))
Expect(rb.RoleRef.Name).To(Equal(utils.MetricsReaderClusterRoleName))
})

It("should fix a metrics reader ClusterRoleBinding with wrong namespace", func() {
By("Patch the ClusterRoleBinding to have a wrong namespace")
rb := &rbacv1.ClusterRoleBinding{}
err := k8sClient.Get(ctx, client.ObjectKey{Name: utils.MetricsReaderClusterRoleBindingName}, rb)
Expect(err).NotTo(HaveOccurred())

rb.Subjects[0].Namespace = "wrong-namespace"
err = k8sClient.Update(ctx, rb)
Expect(err).NotTo(HaveOccurred())

By("Re-reconcile to fix the namespace")
err = ReconcileAppServer(testReconcilerInstance, ctx, cr)
Expect(err).NotTo(HaveOccurred())

By("Verify the namespace was corrected")
err = k8sClient.Get(ctx, client.ObjectKey{Name: utils.MetricsReaderClusterRoleBindingName}, rb)
Expect(err).NotTo(HaveOccurred())
Expect(rb.Subjects[0].Namespace).To(Equal(utils.OLSNamespaceDefault))
})

It("should create a prometheus rule", func() {
By("Get the prometheus rule")
pr := &monv1.PrometheusRule{}
Expand Down
4 changes: 4 additions & 0 deletions internal/controller/utils/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,10 @@ ssl_ca_file = '/etc/certs/cm-olspostgresca/service-ca.crt'
MetricsReaderServiceAccountTokenSecretName = "metrics-reader-token" // #nosec G101
// MetricsReaderServiceAccountName is the name of the service account for the metrics reader
MetricsReaderServiceAccountName = "lightspeed-operator-metrics-reader"
// MetricsReaderClusterRoleName is the name of the ClusterRole granting metrics read access
MetricsReaderClusterRoleName = "lightspeed-operator-ols-metrics-reader"
// MetricsReaderClusterRoleBindingName is the name of the ClusterRoleBinding for the metrics reader
MetricsReaderClusterRoleBindingName = "lightspeed-operator-ols-metrics-reader"
// OpenShiftMCPServerHTTPSPort is the standalone Service/container HTTPS port (service-ca TLS).
OpenShiftMCPServerHTTPSPort = 8443
// OpenShiftMCPServerDeploymentName is the standalone openshift-mcp-server Deployment.
Expand Down
5 changes: 5 additions & 0 deletions internal/controller/utils/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,11 @@ const (
ErrGetSARClusterRoleBinding = "failed to get SAR cluster role binding"
ErrGetServiceMonitor = "failed to get ServiceMonitor"
ErrGetMetricsReaderSecret = "failed to get metrics reader secret"
ErrGenerateMetricsReaderCRB = "failed to generate metrics reader cluster role binding"
ErrCreateMetricsReaderCRB = "failed to create metrics reader cluster role binding"
ErrGetMetricsReaderCRB = "failed to get metrics reader cluster role binding"
ErrUpdateMetricsReaderCRB = "failed to update metrics reader cluster role binding"
ErrSetMetricsReaderCRBOwnerReference = "failed to set metrics reader cluster role binding owner reference"
ErrGetPrometheusRule = "failed to get PrometheusRule"
ErrUpdateAPIConfigmap = "failed to update OLS configmap"
ErrUpdateAPIDeployment = "failed to update OLS deployment"
Expand Down