LOG-8978: Enhance log-file-metrics-exporter to only allow scraping of metrics from a secure endpoint by Clee2691 · Pull Request #3247 · openshift/cluster-logging-operator

Clee2691 · 2026-04-08T21:12:33Z

Description

Adds RBAC for the LFME ServiceAccount to perform TokenReview and SubjectAccessReview API calls, enabling
server-side bearer token validation on the metrics endpoint
- This could be removed when LOG-8977: Secure the metrics endpoint to restrict scraping to autorized client #3238 merges (Potentially WIP)
Passes -secureMetrics flag to the LFME binary so the auth middleware is active
Configures the ServiceMonitor to include the Prometheus bearer token when scraping
Updates the e2e test to authenticate with a bearer token

Should land #3238 first before this PR

This PR also requires changes in the LFME for metrics to be secured.
See: ViaQ/log-file-metric-exporter#42

/cc @cahartma @vparfonov
/assign @jcantrill

Links

JIRA: https://redhat.atlassian.net/browse/LOG-8978

openshift-ci-robot · 2026-04-08T21:12:39Z

@Clee2691: This pull request references LOG-8978 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.8.0" version, but no target version was set.

Details

In response to this:

Description

Adds RBAC for the LFME ServiceAccount to perform TokenReview and SubjectAccessReview API calls, enabling
server-side bearer token validation on the metrics endpoint

This could be removed when LOG-8977: Secure the metrics endpoint to restrict scraping to autorized client #3238 merges (Potentially WIP)

Passes -secureMetrics flag to the LFME binary so the auth middleware is active

Configures the ServiceMonitor to include the Prometheus bearer token when scraping

Updates the e2e test to authenticate with a bearer token

Should land #3238 first before this PR

/cc @cahartma @vparfonov
/assign @jcantrill

Links

JIRA: https://redhat.atlassian.net/browse/LOG-8978

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 openshift-eng/jira-lifecycle-plugin repository.

Clee2691 · 2026-04-08T21:12:40Z

/hold

qodo-code-review · 2026-04-08T21:12:53Z

Review Summary by Qodo

Secure log-file-metrics-exporter metrics endpoint with bearer token authentication

✨ Enhancement 🧪 Tests

Walkthroughs

Description

• Add RBAC for metrics endpoint authentication via TokenReview and SubjectAccessReview
• Enable secure metrics flag in log-file-metric-exporter binary
• Configure ServiceMonitor to include Prometheus bearer token authentication
• Update e2e tests to validate secure metrics endpoint with bearer token

Diagram

flowchart LR
  RBAC["RBAC Configuration<br/>TokenReview & SubjectAccessReview"]
  Flag["Secure Metrics Flag<br/>-secureMetrics"]
  ServiceMonitor["ServiceMonitor<br/>Bearer Token"]
  E2E["E2E Tests<br/>Token Validation"]
  
  RBAC --> Flag
  Flag --> ServiceMonitor
  ServiceMonitor --> E2E

File Changes

1. internal/auth/rbac.go ✨ Enhancement +50/-0

Add metrics endpoint RBAC reconciliation functions

• Add ReconcileMetricsRBAC() function to reconcile metrics authentication RBAC
• Create NewMetricsAuthClusterRole() with permissions for tokenreviews and subjectaccessreviews
• Create NewMetricsAuthClusterRoleBinding() to bind metrics auth role to service account

internal/auth/rbac.go

2. internal/metrics/logfilemetricexporter/factory.go ✨ Enhancement +1/-1

Enable secure metrics flag in exporter binary
• Add -secureMetrics flag to log-file-metric-exporter command arguments
internal/metrics/logfilemetricexporter/factory.go

3. internal/metrics/logfilemetricexporter/metric_exporter.go ✨ Enhancement +5/-0

Invoke metrics RBAC reconciliation in exporter
• Call ReconcileMetricsRBAC() during reconciliation to set up metrics authentication
internal/metrics/logfilemetricexporter/metric_exporter.go

View more (5)

4. internal/metrics/logfilemetricexporter/metric_exporter_test.go 🧪 Tests +22/-0

Test metrics RBAC and bearer token configuration

• Add import for rbacv1 package
• Verify BearerTokenFile is set in ServiceMonitor endpoint
• Test metrics auth ClusterRole exists with correct authentication and authorization rules
• Test metrics auth ClusterRoleBinding exists and binds to correct service account

internal/metrics/logfilemetricexporter/metric_exporter_test.go

5. internal/metrics/service_monitor.go ✨ Enhancement +6/-4

Configure ServiceMonitor with bearer token authentication
• Add prometheusBearerTokenFile constant for Prometheus service account token path
• Configure ServiceMonitor endpoint with BearerTokenFile for authentication
internal/metrics/service_monitor.go

6. internal/metrics/service_monitor_test.go 🧪 Tests +3/-0

Test ServiceMonitor bearer token configuration
• Verify BearerTokenFile is correctly set to Kubernetes service account token path
internal/metrics/service_monitor_test.go

7. internal/reconcile/rbac.go ✨ Enhancement +13/-0

Add ClusterRole reconciliation function
• Add ClusterRole() function to reconcile ClusterRole resources with desired rules
internal/reconcile/rbac.go

8. test/e2e/logfilesmetricexporter/lfme_test.go 🧪 Tests +24/-8

Update e2e test for secure metrics endpoint authentication

• Reorganize imports for better readability
• Update test name to reflect secure metrics endpoint validation
• Create ClusterRoleBinding to grant metrics access to service account
• Modify curl command to include bearer token from service account
• Verify metrics endpoint is accessible only with valid bearer token

test/e2e/logfilesmetricexporter/lfme_test.go

qodo-code-review · 2026-04-08T21:12:56Z

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX Issues (0)

🐞\ ☼ Reliability (2)

1. E2E assumes metrics ClusterRole 🐞 ☼

Description

The LFME e2e test binds to ClusterRole "cluster-logging-operator-metrics-reader" without ensuring it
exists, and this repo’s metrics reader role is not installed by default (it’s commented out in
config/rbac/kustomization.yaml). In environments where that ClusterRole is absent, `oc create
clusterrolebinding ... --clusterrole=...` will fail and the e2e will fail before scraping metrics.

Code

test/e2e/logfilesmetricexporter/lfme_test.go[R55-68]

+		metricsRoleName := "cluster-logging-operator-metrics-reader"
+		metricsBindingName := fmt.Sprintf("%s-metrics-reader", constants.LogfilesmetricexporterName)
+		e2e.AddCleanup(func() error {
+			return oc.Literal().From("oc delete --ignore-not-found clusterrolebinding %s", metricsBindingName).Output()
+		})
+
		err = createLFME(validCR)
		Expect(err).ToNot(HaveOccurred())
		Expect(e2e.WaitForDaemonSet(constants.OpenshiftNS, constants.LogfilesmetricexporterName)).To(Succeed())

-		args := []string{"-k", "-s", fmt.Sprintf("https://%s.%s.svc:2112/metrics", constants.LogfilesmetricexporterName, constants.OpenshiftNS)}
+		By("creating a ClusterRoleBinding for the service account to allow access to metrics")
+		_, err = oc.Literal().From("oc create clusterrolebinding %s --clusterrole=%s --serviceaccount=%s:%s",
+			metricsBindingName, metricsRoleName, constants.OpenshiftNS, constants.LogfilesmetricexporterName).Run()
+		Expect(err).ToNot(HaveOccurred(), "Failed to create ClusterRoleBinding")

Evidence

The e2e test hard-codes ClusterRole "cluster-logging-operator-metrics-reader" and creates a
ClusterRoleBinding to it. In this repo, the only metrics reader ClusterRole manifest is named
"metrics-reader" and it is not included in the default RBAC kustomization (commented out), so the
test now depends on cluster preconfiguration outside this codebase.

test/e2e/logfilesmetricexporter/lfme_test.go[51-68]
config/rbac/metrics_reader_role.yaml[1-9]
config/rbac/kustomization.yaml[13-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 LFME e2e test creates a ClusterRoleBinding to a hard-coded ClusterRole name ("cluster-logging-operator-metrics-reader") but does not create/ensure that ClusterRole exists. This makes the test environment-dependent and can fail when the cluster isn’t pre-seeded with that ClusterRole.

### Issue Context
The repo contains a `metrics-reader` ClusterRole manifest, but it is commented out in `config/rbac/kustomization.yaml`, so it’s not guaranteed to exist in test clusters.

### Fix Focus Areas
- test/e2e/logfilesmetricexporter/lfme_test.go[51-68]
- config/rbac/metrics_reader_role.yaml[1-9]
- config/rbac/kustomization.yaml[13-24]

### What to change
- Make the e2e test create (and cleanup) the required ClusterRole before creating the ClusterRoleBinding (e.g., `oc apply -f -` with an inline YAML, or `oc create clusterrole ... --verb=get --non-resource-url=/metrics`), or
- Change the test to use a ClusterRole that is guaranteed to exist in the test environment (and document that dependency explicitly).

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

2. E2E exec shell mismatch 🐞 ☼

Description

The LFME e2e test execs sh -c into the LFME pod, but the LFME DaemonSet container is configured to
run with /bin/bash. If the LFME image includes bash but not sh, the test will fail to exec and
never reach the curl scrape.

Code

test/e2e/logfilesmetricexporter/lfme_test.go[R71-76]

+		curlCmd := fmt.Sprintf(`curl -s -k -H "Authorization: Bearer $(cat /var/run/secrets/kubernetes.io/serviceaccount/token)" %s`, metricsURL)
+
		err = wait.PollUntilContextTimeout(context.TODO(), time.Second, time.Second*30, true, func(context.Context) (done bool, err error) {
-			out, err := oc.Exec().WithNamespace(constants.OpenshiftNS).Pod(fmt.Sprintf("ds/%s", constants.LogfilesmetricexporterName)).WithCmd("curl", args...).Run()
+			out, err := oc.Exec().WithNamespace(constants.OpenshiftNS).
+				Pod(fmt.Sprintf("ds/%s", constants.LogfilesmetricexporterName)).
+				WithCmd("sh", "-c", curlCmd).Run()

Evidence
The DaemonSet container configuration in this repo explicitly uses /bin/bash, indicating bash is
the expected shell in the image. The e2e test uses sh -c, which is an unnecessary assumption and
can fail when sh is not present (or not at /bin/sh).
test/e2e/logfilesmetricexporter/lfme_test.go[70-77]
internal/metrics/logfilemetricexporter/factory.go[109-118]

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 test uses `WithCmd("sh", "-c", ...)` while the LFME container is configured around `/bin/bash`. This can break the test if `/bin/sh` is not available in the container.

### Issue Context
`internal/metrics/logfilemetricexporter/factory.go` sets the container `Command` to `/bin/bash`, so using `bash -c` in the exec aligns with the image contract already established by the operator.

### Fix Focus Areas
- test/e2e/logfilesmetricexporter/lfme_test.go[70-77]
- internal/metrics/logfilemetricexporter/factory.go[109-118]

### What to change
- Replace `WithCmd("sh", "-c", curlCmd)` with `WithCmd("/bin/bash", "-c", curlCmd)` (or `WithCmd("bash", "-c", curlCmd)` if that’s the convention elsewhere in tests).

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

ⓘ The new review experience is currently in Beta. Learn more

openshift-ci-robot · 2026-04-08T22:10:34Z

@Clee2691: This pull request references LOG-8978 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.8.0" version, but no target version was set.

Details

In response to this:

Description

Adds RBAC for the LFME ServiceAccount to perform TokenReview and SubjectAccessReview API calls, enabling
server-side bearer token validation on the metrics endpoint

This could be removed when LOG-8977: Secure the metrics endpoint to restrict scraping to autorized client #3238 merges (Potentially WIP)

Passes -secureMetrics flag to the LFME binary so the auth middleware is active

Configures the ServiceMonitor to include the Prometheus bearer token when scraping

Updates the e2e test to authenticate with a bearer token

Should land #3238 first before this PR

This PR also requires changes in the LFME for metrics to be secured.
See: ViaQ/log-file-metric-exporter#42

/cc @cahartma @vparfonov
/assign @jcantrill

Links

JIRA: https://redhat.atlassian.net/browse/LOG-8978

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 openshift-eng/jira-lifecycle-plugin repository.

vparfonov · 2026-04-09T09:13:02Z

/lgtm

jcantrill · 2026-04-14T20:22:12Z

+// It creates:
+// - A ClusterRole allowing tokenreviews and subjectaccessreviews (for server-side token validation)
+// - A ClusterRoleBinding binding the above role to the service account
+func ReconcileMetricsRBAC(k8sClient client.Client, name, saNamespace, saName string) error {


Do we need to create these cluster roles or can we defer to OLM to create them from the CSV since the operator will require the same permissions. I thought this was part of the manifest deployment but I do not recall which SA we use for LFME

We do not need to create these clusterroles and I only have this here for tests to work. I've noted in the PR description that his can be removed when #3238 is merged and then we only need to reconcile the clusterrolebinding

@jcantrill We will need to reconcile the clusterrole because the one from the manifests are bundled into the CSV and the cluster-logging-operator-metrics-auth clusterrole is not generated as part of the manifests.

vparfonov · 2026-04-17T10:12:25Z

/test e2e-target

openshift-ci · 2026-04-22T20:01:13Z

New changes are detected. LGTM label has been removed.

openshift-ci · 2026-04-22T20:01:33Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Clee2691
Once this PR has been reviewed and has the lgtm label, please ask for approval from jcantrill. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

Clee2691 · 2026-04-23T15:09:06Z

/retest

Clee2691 · 2026-04-23T19:26:51Z

/hold cancel

vparfonov · 2026-04-24T11:00:45Z

/test e2e-target

Clee2691 · 2026-04-24T20:59:33Z

/retest

… metrics from a secure endpoint

Clee2691 · 2026-04-25T16:08:34Z

/retest

openshift-ci · 2026-04-25T17:26:19Z

@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 · 2026-04-22T20:22:56Z

 }

+// ReconcileMetricsAuthRBAC reconciles the ClusterRoleBinding for the metrics auth role
+func ReconcileMetricsAuthRBAC(k8sClient client.Client, name, saNamespace, saName string) error {


I'm not certain we need to create this rolebinding. Have we tested without it? This binding is only required for clients that are not prometheus AFAIK and I do not expect us to need to cater to those.

My testing of the CLO metrics endpoint where I'm inspecting in cluster metrics still shows operator metrics being received without any specfic bidings.

The metrics-auth binding is for the server side, not the client side. It grants the LFME's ServiceAccount permission to create TokenReview and SubjectAccessReview resources so it can validate incoming bearer tokens. Without it, the LFME can't authenticate any scraper, including Prometheus. The operator metrics work because the operator's ServiceAccount already has this binding via config/rbac/metrics_auth_role_binding.yaml. It is in the CSV of the operator.

jcantrill · 2026-04-22T20:23:58Z

 		return err
 	}

+	if err := auth.ReconcileMetricsAuthRBAC(requestClient, resNames.CommonName, lfmeInstance.Namespace, resNames.ServiceAccount); err != nil {


Same here. I don't think this is necessary as prometheus discovers the endpoints to scrape through the servicemonitor. Please verify

Again this is for the server not the client (prometheus)

jcantrill · 2026-04-22T20:26:01Z

 	return err
 }

+func ClusterRole(k8sClient client.Client, desired *rbacv1.ClusterRole) error {


We have to be careful about cluster objects as in the past there was an issue with finalizers and ownerrefs which would cause the API server to hang. It was some issue with these objects not being namespaced but being owned by namespaced objects

There is a default ClusterRole: system:auth-delegator that actually provides both the tokenreview and subjectaccessreview. We could bind that in order to reduce cluster scoped resources. However, we would need to ensure proper garbage collection of the CRB... somehow.

I think the only other option here to avoid ownership issues is to add the metrics-auth CRB to the manifests. It would be available when no LFME exists but it would be managed by OLM.

Thoughts?

openshift-ci Bot assigned jcantrill Apr 8, 2026

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 8, 2026

openshift-ci Bot requested review from cahartma and vparfonov April 8, 2026 21:12

openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 8, 2026

Clee2691 mentioned this pull request Apr 8, 2026

LOG-8978: Add bearer token authentication for metrics endpoint ViaQ/log-file-metric-exporter#42

Merged

4 tasks

openshift-ci Bot assigned vparfonov Apr 9, 2026

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 9, 2026

jcantrill reviewed Apr 14, 2026

View reviewed changes

Clee2691 mentioned this pull request Apr 15, 2026

LOG-8977: Secure the metrics endpoint to restrict scraping to autorized client #3238

Merged

Clee2691 force-pushed the LOG-8978 branch from ff56529 to 9e31e67 Compare April 22, 2026 20:01

openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2026

jcantrill added the release/6.6 label Apr 22, 2026

Clee2691 force-pushed the LOG-8978 branch from 9e31e67 to 3c8bd6c Compare April 23, 2026 19:21

openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 23, 2026

Clee2691 force-pushed the LOG-8978 branch from 3c8bd6c to 0f567ff Compare April 23, 2026 19:38

LOG-8978: Enhance log-file-metrics-exporter to only allow scraping of…

684de6e

… metrics from a secure endpoint

Clee2691 force-pushed the LOG-8978 branch from 0f567ff to 684de6e Compare April 24, 2026 22:09

jcantrill reviewed Apr 27, 2026

View reviewed changes

Conversation

Clee2691 commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Links

Uh oh!

openshift-ci-robot commented Apr 8, 2026 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Links

Uh oh!

Clee2691 commented Apr 8, 2026

Uh oh!

qodo-code-review Bot commented Apr 8, 2026

Review Summary by Qodo

Walkthroughs

File Changes

Uh oh!

qodo-code-review Bot commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

openshift-ci-robot commented Apr 8, 2026 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Links

Uh oh!

vparfonov commented Apr 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vparfonov commented Apr 17, 2026

Uh oh!

openshift-ci Bot commented Apr 22, 2026

Uh oh!

openshift-ci Bot commented Apr 22, 2026

Uh oh!

Clee2691 commented Apr 23, 2026

Uh oh!

Clee2691 commented Apr 23, 2026

Uh oh!

vparfonov commented Apr 24, 2026

Uh oh!

Clee2691 commented Apr 24, 2026

Uh oh!

Clee2691 commented Apr 25, 2026

Uh oh!

openshift-ci Bot commented Apr 25, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Clee2691 Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Clee2691 commented Apr 8, 2026 •

edited

Loading

openshift-ci-robot commented Apr 8, 2026 •

edited by openshift-ci Bot

Loading

qodo-code-review Bot commented Apr 8, 2026 •

edited

Loading

openshift-ci-robot commented Apr 8, 2026 •

edited by openshift-ci Bot

Loading

Clee2691 Apr 27, 2026 •

edited

Loading