[release-6.5] fix(lfme): set -secureMetrics=false on exporter daemonset

[Clee2691]

Description

This PR sets the -secureMetrics arg on the LogFileMetricExporter container to false.

/cc @vparfonov
/assign @jcantrill

Links

• JIRA: https://redhat.atlassian.net/browse/LOG-9872

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: e08d9103-6439-4a52-875b-b19cbe593eea

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-for-rh-openshift]

PR Summary by Qodo

Secure LogFileMetricExporter metrics endpoint

✨ Enhancement 🧪 Tests ⚙️ Configuration changes 🕐 40+ Minutes

AI Description

• Require authenticated, authorized access to LogFileMetricExporter metrics.
• Configure Prometheus scraping with its service-account bearer token.
• Add RBAC reconciliation and end-to-end coverage for secure metric collection.

Diagram

graph TD
  P["Prometheus"] --> S["ServiceMonitor"] --> E["LFME exporter"] --> K["Kubernetes auth API"]
  R["Auth-delegator binding"] --> E

Loading

High-Level Assessment

The approach uses the standard Kubernetes delegated-authentication model: grant the exporter system:auth-delegator, authorize /metrics readers through a dedicated ClusterRole, and have Prometheus present its service-account token. This is preferable to application-managed credentials or a custom authorization proxy because it preserves platform RBAC semantics and integrates directly with ServiceMonitor scraping.

Files changed (16) +356 / -41

Enhancement (5) +55 / -13

rbac.goReconcile delegated metrics-auth RBAC +32/-0

Reconcile delegated metrics-auth RBAC

• Adds helpers that bind the exporter service account to system:auth-delegator and remove the binding safely.

internal/auth/rbac.go

resource_names.goTrack metrics-auth binding names +9/-7

Track metrics-auth binding names

• Adds a resource-name field for the exporter metrics authentication ClusterRoleBinding.

internal/factory/resource_names.go

factory.goEnable secure exporter metrics +1/-1

Enable secure exporter metrics

• Passes the secureMetrics flag when launching the log-file-metric-exporter container.

internal/metrics/logfilemetricexporter/factory.go

metric_exporter.goReconcile exporter auth delegation +7/-1

Reconcile exporter auth delegation

• Creates a namespace-qualified metrics-auth binding name and reconciles it with exporter resources.

internal/metrics/logfilemetricexporter/metric_exporter.go

service_monitor.goAttach bearer token to metric scrapes +6/-4

Attach bearer token to metric scrapes

• Configures ServiceMonitor endpoints to present the Prometheus service-account token when scraping metrics.

internal/metrics/service_monitor.go

Bug fix (2) +29 / -18

rbac.goHandle immutable ClusterRoleBinding role references +28/-18

Handle immutable ClusterRoleBinding role references

• Reworks ClusterRoleBinding reconciliation to create missing bindings, update subjects, and recreate bindings when immutable roleRef changes. Deletion now ignores already-missing bindings.

internal/reconcile/rbac.go

service_account.goReconcile ServiceAccount finalizers +1/-0

Reconcile ServiceAccount finalizers

• Propagates desired finalizers while reconciling ServiceAccounts.

internal/reconcile/service_account.go

Tests (6) +260 / -8

rbac_test.goTest metrics-auth binding construction +16/-0

Test metrics-auth binding construction

• Verifies the delegated-auth ClusterRoleBinding targets system:auth-delegator and the expected service account.

internal/auth/rbac_test.go

factory_test.goVerify secured exporter resources +20/-0

Verify secured exporter resources

• Checks the secure metrics flag, ServiceMonitor bearer token, and delegated-auth ClusterRoleBinding during reconciliation.

internal/metrics/logfilemetricexporter/factory_test.go

rbac_test.goTest immutable role-reference replacement +28/-0

Test immutable role-reference replacement

• Verifies reconciliation deletes and recreates a ClusterRoleBinding when its role reference changes.

internal/reconcile/rbac_test.go

service_account_test.goTest ServiceAccount finalizer propagation +35/-0

Test ServiceAccount finalizer propagation

• Adds coverage confirming reconciled ServiceAccounts retain desired finalizers.

internal/reconcile/service_account_test.go

lfme_test.goValidate authenticated metrics scraping +58/-8

Validate authenticated metrics scraping

• Replaces unauthenticated curl coverage with authorized bearer-token scraping. Adds a Thanos query assertion that Prometheus collects exporter metrics through the ServiceMonitor.

test/e2e/logfilesmetricexporter/lfme_test.go

prometheus.goAdd Thanos query test helper +103/-0

Add Thanos query test helper

• Adds helper functions to obtain Prometheus credentials and query the Thanos API for e2e metric assertions.

test/helpers/prometheus/prometheus.go

Other (3) +12 / -2

cluster-logging-operator-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yamlAdd packaged metrics-reader ClusterRole +10/-0

Add packaged metrics-reader ClusterRole

• Adds the bundle manifest for the operator-specific ClusterRole that grants GET access to /metrics.

bundle/manifests/cluster-logging-operator-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml

kustomization.yamlInclude metrics-reader RBAC +1/-1

Include metrics-reader RBAC

• Enables the metrics-reader ClusterRole in the RBAC kustomization output.

config/rbac/kustomization.yaml

metrics_reader_role.yamlNamespace the metrics-reader role name +1/-1

Namespace the metrics-reader role name

• Renames the ClusterRole to cluster-logging-operator-metrics-reader to avoid a generic shared name.

config/rbac/metrics_reader_role.yaml

[qodo-for-rh-openshift]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Orphaned auth-delegator binding ✓ Resolved 🐞 Bug ⛨ Security

Description

LogFileMetricExporter reconciliation now creates a cluster-scoped ClusterRoleBinding to
system:auth-delegator, but the LFME controller short-circuits when the CR is being deleted and
performs no cleanup, so this binding can persist indefinitely. If a ServiceAccount with the same
name/namespace is later recreated, it will immediately regain auth-delegator privileges via the
stale binding.

Code

internal/metrics/logfilemetricexporter/metric_exporter.go[R55-58]

+	if err := auth.ReconcileMetricsAuthRBAC(requestClient, resNames.MetricsAuthClusterRoleBinding, lfmeInstance.Namespace, resNames.ServiceAccount); err != nil {
+		log.Error(err, "logfilemetricexporter.ReconcileMetricsAuthRBAC")
+		return err
+	}

Relevance

●●● Strong

Security cleanup omission is directly relevant; stale auth-delegator binding can restore privileges
after ServiceAccount recreation.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The LFME reconciler always provisions a metrics-auth ClusterRoleBinding, but the LFME controller
exits early during deletion and never calls any delete helper, so the cluster-scoped binding is left
behind.

internal/metrics/logfilemetricexporter/metric_exporter.go[37-59]
internal/auth/rbac.go[115-145]
internal/controller/logfilemetricsexporter/logfilemetricsexporter_controller.go[52-70]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Reconcile()` now creates a cluster-scoped ClusterRoleBinding granting `system:auth-delegator` to the LFME ServiceAccount, but there is no deletion-time cleanup path; the controller returns immediately when `DeletionTimestamp != nil`.

### Issue Context
Because the binding is cluster-scoped and created without owner references, it can outlive the LFME CR and continue to apply to any future ServiceAccount recreated with the same identity.

### Fix Focus Areas
- internal/controller/logfilemetricsexporter/logfilemetricsexporter_controller.go[67-70]
- internal/auth/rbac.go[135-145]
- internal/metrics/logfilemetricexporter/metric_exporter.go[37-58]

### Implementation sketch
- Add a finalizer to the LFME CR.
- In the controller reconcile loop, when `DeletionTimestamp != nil`, delete cluster-scoped resources created for LFME (at minimum the metrics-auth ClusterRoleBinding via `auth.DeleteMetricsAuthRBAC(...)`), then remove the finalizer to allow deletion to complete.
- Ensure the delete logic is idempotent (NotFound ignored).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Wrong e2e CRB cleanup ✓ Resolved 🐞 Bug ☼ Reliability

Description

The LFME e2e test cleanup deletes clusterrolebinding logfilesmetricexporter-metrics-auth, but the
operator now creates cluster-logging-<namespace>-logfilesmetricexporter-metrics-auth, so the real
binding is not removed. This can leave residual cluster-scoped RBAC between test runs and
contaminate subsequent e2e executions.

Code

test/e2e/logfilesmetricexporter/lfme_test.go[R66-70]

+		// Delete the metrics auth ClusterRoleBinding
+		// The LFME reconciles the ClusterRoleBinding and ClusterRole for metrics auth
+		e2e.AddCleanup(func() error {
+			return oc.Literal().From("oc delete --ignore-not-found clusterrolebinding %s", metricsAuthRoleName).Output()
+		})

Relevance

●●● Strong

Cleanup uses a demonstrably different generated CRB name, leaving cluster-scoped test residue
between runs.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test deletes a short CRB name, while the reconciler constructs a different name that includes
the namespace and cluster-logging- prefix.

test/e2e/logfilesmetricexporter/lfme_test.go[57-70]
internal/metrics/logfilemetricexporter/metric_exporter.go[37-43]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The e2e cleanup computes and deletes a metrics-auth ClusterRoleBinding name that no longer matches the operator-created name.

### Issue Context
Operator naming: `cluster-logging-<namespace>-<sa>-metrics-auth`.
Test cleanup naming: `<sa>-metrics-auth`.

### Fix Focus Areas
- test/e2e/logfilesmetricexporter/lfme_test.go[57-70]
- internal/metrics/logfilemetricexporter/metric_exporter.go[37-43]

### Implementation sketch
- Update the e2e test to compute the CRB name using the same format as production code (include the namespace and `cluster-logging-` prefix), or centralize the name generation in a helper reused by both.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. CRB delete-create race ⊘ Outdated 🐞 Bug ☼ Reliability

Description

When a ClusterRoleBinding.roleRef changes, reconciliation deletes the existing binding and
immediately creates a new one with the same name in the same call. This can fail in real clusters if
the delete has not fully completed yet, causing transient reconcile failures during roleRef
migrations.

Code

internal/reconcile/rbac.go[R72-75]

+			return err
+		}
+		log.V(3).Info("Recreating clusterRoleBinding", "name", name)
+		return k8sClient.Create(context.TODO(), desired)

Relevance

●● Moderate

Race concern is technically plausible, but no close accepted or rejected repository precedent was
found.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new reconciliation logic explicitly calls Delete() and then Create() for the same name when
existing.RoleRef != desired.RoleRef, which is the risky sequence.

internal/reconcile/rbac.go[56-81]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`reconcile.ClusterRoleBinding()` deletes and then immediately recreates a ClusterRoleBinding when `roleRef` changes.

### Issue Context
Kubernetes deletion is asynchronous; attempting to `Create()` the same name immediately after `Delete()` can intermittently fail (e.g., name still exists while deletion is pending), producing avoidable reconcile errors.

### Fix Focus Areas
- internal/reconcile/rbac.go[68-80]

### Implementation sketch
- After issuing `Delete()`, return `nil` (or a sentinel error that the controller treats as requeue) and let the next reconcile create it once it becomes NotFound.
- Alternatively, after delete: poll `Get()` until NotFound (bounded), then create; or handle `AlreadyExists` from `Create()` by returning nil and relying on the next reconcile.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

⚠️ Tickets: not configured — ticket URL found in PR but could not be fetched — check ticket provider credentials

✅ Compliance rules (platform): 9 rules

Tip of the day

💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[vparfonov]

/lgtm

[vparfonov]

/lgtm cancel

[Clee2691]

/hold

[openshift-ci]

@Clee2691: all tests passed!

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

[jcantrill]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Clee2691, jcantrill

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [jcantrill]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jcantrill]

/lgtm

[jcantrill]

/label verified

[jcantrill]

/hold cancel