[release-6.2] 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-9873

[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: 46803a25-4b28-4373-99b7-22c08cbbe725

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

• Enables delegated authentication and authorization for the LogFileMetricExporter TLS metrics
 endpoint.
• Grants token-based metrics access and exporter TokenReview/SubjectAccessReview permissions.
• Cleans up cluster-scoped RBAC and verifies authorized scraping with unit and end-to-end tests.

Diagram

sequenceDiagram
    participant SM as ServiceMonitor
    participant P as Prometheus
    participant E as LFME endpoint
    participant A as Auth-delegator RBAC
    participant K as Kubernetes API
    SM->>P: Configure HTTPS scrape
    P->>E: Bearer-token request
    E->>K: TokenReview and access review
    A->>K: Permit delegated reviews
    K-->>E: Authorization decision
    E-->>P: Authorized metrics

Loading

High-Level Assessment

The delegated Kubernetes authentication and authorization approach is appropriate: it reuses service-account tokens and RBAC instead of introducing separate credentials or weakening endpoint access. Network-only restriction and static shared tokens were implicitly avoided because they provide weaker identity-based control and operationally harder credential management.

Files changed (18) +426 / -33

Enhancement (5) +67 / -11

rbac.goReconcile exporter delegated-auth binding +32/-0

Reconcile exporter delegated-auth binding

• Adds helpers to create, reconcile, and delete a ClusterRoleBinding from the exporter service account to system:auth-delegator.

internal/auth/rbac.go

resource_names.goName metrics-auth binding +2/-0

Name metrics-auth binding

• Extends forwarder resource names with the cluster-scoped metrics authentication ClusterRoleBinding.

internal/factory/resource_names.go

factory.goEnable secure exporter metrics +1/-1

Enable secure exporter metrics

• Starts log-file-metric-exporter with secureMetrics enabled while preserving TLS profile configuration.

internal/metrics/logfilemetricexporter/factory.go

metric_exporter.goReconcile and clean up metrics RBAC +26/-6

Reconcile and clean up metrics RBAC

• Centralizes namespaced exporter resource names, provisions the auth-delegator binding, and deletes it during cleanup.

internal/metrics/logfilemetricexporter/metric_exporter.go

service_monitor.goSend Prometheus bearer token +6/-4

Send Prometheus bearer token

• Configures the ServiceMonitor endpoint to include Prometheus's service-account token in HTTPS scrapes.

internal/metrics/service_monitor.go

Bug fix (3) +37 / -14

logfilemetricsexporter_controller.goClean up exporter RBAC on deletion +6/-1

Clean up exporter RBAC on deletion

• Invokes exporter cleanup during LogFileMetricExporter deletion to remove cluster-scoped resources that owner references cannot collect.

internal/controller/logfilemetricsexporter/logfilemetricsexporter_controller.go

rbac.goHandle immutable ClusterRoleBinding role references +30/-13

Handle immutable ClusterRoleBinding role references

• Replaces CreateOrUpdate with explicit create, update, and delete-recreate behavior when immutable roleRef changes. Makes missing binding deletion a no-op.

internal/reconcile/rbac.go

service_account.goPreserve ServiceAccount finalizers +1/-0

Preserve ServiceAccount finalizers

• Propagates desired finalizers during ServiceAccount reconciliation.

internal/reconcile/service_account.go

Tests (7) +302 / -8

rbac_test.goTest delegated-auth binding shape +16/-0

Test delegated-auth binding shape

• Verifies the generated binding targets system:auth-delegator and the expected exporter service account.

internal/auth/rbac_test.go

factory_test.goTest secure exporter resources +20/-0

Test secure exporter resources

• Checks secure-metrics arguments, ServiceMonitor bearer-token configuration, and delegated-auth binding reconciliation.

internal/metrics/logfilemetricexporter/factory_test.go

metric_exporter_test.goTest metrics RBAC cleanup +25/-0

Test metrics RBAC cleanup

• Verifies cleanup removes the cluster-scoped metrics-auth binding and remains idempotent.

internal/metrics/logfilemetricexporter/metric_exporter_test.go

rbac_test.goTest immutable roleRef replacement +43/-0

Test immutable roleRef replacement

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

internal/reconcile/rbac_test.go

service_account_test.goTest ServiceAccount finalizer reconciliation +37/-0

Test ServiceAccount finalizer reconciliation

• Verifies desired ServiceAccount finalizers are retained after reconciliation.

internal/reconcile/service_account_test.go

lfme_test.goValidate authorized metrics scraping +58/-8

Validate authorized metrics scraping

• Adds end-to-end coverage for bearer-token access to exporter metrics and for Prometheus collection 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 acquire Prometheus credentials, query the Thanos route, and detect metric results for end-to-end tests.

test/helpers/prometheus/prometheus.go

Other (3) +20 / -0

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

Add bundled metrics-reader ClusterRole

• Adds the generated install manifest for read-only GET access to the non-resource /metrics endpoint.

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

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

Include metrics-reader RBAC

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

config/rbac/kustomization.yaml

metrics_reader_role.yamlDefine metrics-reader ClusterRole +9/-0

Define metrics-reader ClusterRole

• Defines cluster-scoped permission to GET /metrics for identities authorized to scrape metrics.

config/rbac/metrics_reader_role.yaml

[qodo-for-rh-openshift]

Code Review by Qodo

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

1. Finalizers overwritten on reconcile ✓ Resolved 🐞 Bug ☼ Reliability

Description

reconcile.ServiceAccount now assigns sa.Finalizers = desired.Finalizers, which will remove any
pre-existing finalizers on the ServiceAccount that were added by other controllers/admins. This can
break deletion/cleanup semantics for those controllers because their finalizers can be dropped on
the next reconciliation.

Code

internal/reconcile/service_account.go[19]

+		sa.Finalizers = desired.Finalizers

Relevance

●●● Strong

Direct finalizer replacement can remove other controllers’ finalizers; reliability findings on
resource reconciliation are typically actionable.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The reconciliation path explicitly replaces finalizers, and the desired SA for LFME only adds a
single finalizer; therefore any other finalizers present on the live SA would be dropped on update.

internal/reconcile/service_account.go[14-21]
internal/auth/service_account.go[19-28]

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.ServiceAccount` overwrites the entire `finalizers` slice with the desired object's finalizers. This can unintentionally remove finalizers that this controller does not own.

## Issue Context
`internal/auth/service_account.go` adds `metav1.FinalizerDeleteDependents` to the desired ServiceAccount for LFME, and reconciliation should ensure that finalizer exists, without deleting other finalizers.

## Fix Focus Areas
- internal/reconcile/service_account.go[14-21]
- internal/auth/service_account.go[19-28]

## Implementation guidance
- Do **not** assign `sa.Finalizers = desired.Finalizers`.
- Instead, add the desired finalizers to the existing object (dedupe), e.g.:
 - `for _, f := range desired.Finalizers { controllerutil.AddFinalizer(sa, f) }`
- Only remove finalizers that are explicitly owned by this controller (if applicable); otherwise preserve unknown finalizers.

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

---

2. E2E re-runs oc each poll 🐞 Bug ☼ Reliability

Description

The new e2e test calls prometheus.Query(...) inside an Eventually loop, and Query() shells out
to oc twice (create token + get route) on every poll. This adds avoidable latency and load to the
test and can increase flakiness if oc intermittently fails.

Code

test/e2e/logfilesmetricexporter/lfme_test.go[R113-116]

+		Eventually(func(g Gomega) {
+			response, err := prometheus.Query(`log_logged_bytes_total{namespace="openshift-logging"}`)
+			g.Expect(err).NotTo(HaveOccurred(), "Failed to query metric")
+			g.Expect(prometheus.HasResults(response)).To(BeTrue())

Relevance

●● Moderate

Performance concern is plausible, but history only weakly supports rejecting subjective e2e
structure changes.

PR-#3269

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The e2e test calls prometheus.Query(...) within Eventually, and the helper’s Query()
implementation executes oc commands on every invocation (token + route).

test/e2e/logfilesmetricexporter/lfme_test.go[104-118]
test/helpers/prometheus/prometheus.go[16-34]
test/helpers/prometheus/prometheus.go[81-91]

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 polling loop repeatedly calls `prometheus.Query()`, which re-fetches the Thanos host and creates a new token via `oc` every time.

## Issue Context
This is used inside an `Eventually(..., 5*time.Minute, 30*time.Second)` loop, so the `oc` calls are repeated across retries.

## Fix Focus Areas
- test/e2e/logfilesmetricexporter/lfme_test.go[112-118]
- test/helpers/prometheus/prometheus.go[81-91]

## Implementation guidance
- Resolve `host` and `token` once before entering `Eventually` (or add memoization in the helper).
- Use `prometheus.QueryPrometheus(host, token, query)` inside the polling loop.
- If token lifetime is a concern, refresh only on auth failures (e.g., 401/403), not every poll.

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

---

3. Test helper depends on bash 🐞 Bug ⚙ Maintainability

Description

test/helpers/prometheus.executeCmd runs exec.Command("bash", "-c", cmd), which makes e2e
execution depend on bash being present and introduces shell parsing/quoting concerns. Using
exec.Command("oc", ...) with explicit args would be more portable and easier to debug.

Code

test/helpers/prometheus/prometheus.go[R21-23]

+func executeCmd(cmd string) (string, error) {
+	result, err := exec.Command("bash", "-c", cmd).Output()
+	return string(result), err

Relevance

●● Moderate

Portability concern is reasonable, but no close accepted or rejected precedent exists for this
helper pattern.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The helper explicitly invokes bash to execute the command string, so environments lacking bash (or
with different shell behavior) would fail these e2e helpers.

test/helpers/prometheus/prometheus.go[16-24]

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 Prometheus test helper shells out via `bash -c`, which is less portable and hides stderr details.

## Issue Context
The helper only needs to execute two fixed `oc` commands.

## Fix Focus Areas
- test/helpers/prometheus/prometheus.go[16-34]

## Implementation guidance
- Replace `executeCmd(cmd string)` with explicit `exec.Command("oc", ...)` calls in `GetToken()` and `GetThanosHost()`.
- Consider capturing and returning stderr in errors to improve debugging.

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

[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