LOG-8978: Enhance log-file-metrics-exporter to only allow scraping of metrics from a secure endpoint

[Clee2691]

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]

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

/hold

[qodo-code-review]

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

Loading

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

---

[openshift-ci]

[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 assign cahartma for approval. 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

[qodo-code-review]

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]

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

[openshift-ci]

@Clee2691: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-target	ff56529	link	true	/test e2e-target

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.

[vparfonov]

/lgtm

[jcantrill]

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

[Clee2691]

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