LOG-9376: Enhance operator to deploy collector with SAR authn/z

[jcantrill]

Description

This PR:

• Modifies the operator to configure the collector metric endpoint to require valid permissions

Links

• https://redhat.atlassian.net/browse/LOG-9376
• Blocked by LOG-9375: Enhance the prometheus exporter with SAR auth strategy ViaQ/vector#265

cc @Clee2691 @vparfonov

/hold

[openshift-ci-robot]

@jcantrill: This pull request references LOG-9376 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 sub-task to target the "4.8.0" version, but no target version was set.

Details

In response to this:

Description

This PR:

• Modifies the operator to configure the collector metric endpoint to require valid permissions

Links

• https://redhat.atlassian.net/browse/LOG-9376
• Blocked by LOG-9375: Enhance the prometheus exporter with SAR auth strategy ViaQ/vector#265

cc @Clee2691 @vparfonov

/hold

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]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

[qodo-code-review]

Review Summary by Qodo

Enhance collector metrics endpoint with SAR authentication

✨ Enhancement

Walkthroughs

Description

• Add SAR authentication to collector metrics endpoint
• Create ClusterRoleBinding for metrics auth with system:auth-delegator
• Configure Vector prometheus exporter with auth strategy
• Update functional tests to validate metrics auth RBAC

Diagram

flowchart LR
  A["Collector Reconciliation"] -->|"Add metrics auth RBAC"| B["ReconcileMetricsAuthRBAC"]
  B -->|"Create ClusterRoleBinding"| C["system:auth-delegator Role"]
  D["Vector Config"] -->|"Configure auth"| E["PrometheusExporterAuth"]
  E -->|"SAR strategy"| F["Metrics Endpoint Auth"]
  G["Functional Tests"] -->|"Validate RBAC"| H["Token Review & Metrics Reader"]

Loading

File Changes

1. internal/controller/observability/collector.go ✨ Enhancement +6/-0

Add metrics auth RBAC reconciliation

• Add call to auth.ReconcileMetricsAuthRBAC in collector reconciliation
• Create ClusterRoleBinding to allow collector to validate bearer tokens
• Bind metrics auth to collector's ServiceAccount

internal/controller/observability/collector.go

---

2. internal/controller/observability/collector_test.go 🧪 Tests +10/-0

Test metrics auth ClusterRoleBinding creation

• Import rbacv1 package for RBAC testing
• Add test verification for metrics auth ClusterRoleBinding
• Validate binding references system:auth-delegator role
• Verify ServiceAccount subject configuration

internal/controller/observability/collector_test.go

---

3. internal/generator/vector/api/sinks/prometheus_export.go ✨ Enhancement +24/-5

Add authentication configuration to prometheus exporter

• Add PrometheusExporterAuth struct with SAR strategy support
• Define auth configuration fields: strategy, path, resource, verb, etc.
• Add Auth field to PrometheusExporter struct
• Support JSON, YAML, and TOML serialization

internal/generator/vector/api/sinks/prometheus_export.go

---

View more (3)

4. internal/generator/vector/output/metrics/prometheus.go ✨ Enhancement +5/-0

Configure SAR auth for prometheus metrics output

• Configure prometheus exporter with SAR authentication strategy
• Set auth path to /metrics and verb to get
• Initialize auth configuration in prometheus output builder

internal/generator/vector/output/metrics/prometheus.go

---

5. internal/runtime/rbac.go ✨ Enhancement +26/-0

Add RBAC helper functions for auth configuration

• Add NewNonResourceURLPolicyRule helper for non-resource URL policies
• Add NewClusterRoleRef helper to create ClusterRole references
• Add NewServiceAccountSubject helper for ServiceAccount subjects
• Support RBAC configuration for metrics authentication

internal/runtime/rbac.go

---

6. test/functional/metrics/metrics_test.go 🧪 Tests +46/-3

Add metrics auth RBAC setup to functional tests

• Import rbacv1 for RBAC resource creation
• Create ClusterRole for metrics reader with /metrics GET permission
• Create ClusterRoleBinding for metrics reader role
• Create ClusterRoleBinding for token review with system:auth-delegator
• Update curl commands to include bearer token authentication
• Add cleanup for cluster-scoped RBAC resources in AfterEach

test/functional/metrics/metrics_test.go

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. ClusterRoleBinding name collision ☑ 🐞 Bug ⛨ Security

Description

ReconcileCollector creates a cluster-scoped ClusterRoleBinding named "<clf.name>-metrics-auth", so
two ClusterLogForwarders with the same name in different namespaces will overwrite each other’s
binding subjects. This can break metrics authn/z and can grant system:auth-delegator to the wrong
ServiceAccount.

Code

internal/controller/observability/collector.go[R72-76]

+	// Add ClusterRoleBinding to allow the collector to validate bearer tokens for metrics endpoint
+	if err = auth.ReconcileMetricsAuthRBAC(context.Client, resourceNames.CommonName, context.Forwarder.Namespace, context.Forwarder.Spec.ServiceAccount.Name); err != nil {
+		log.V(3).Error(err, "auth.ReconcileMetricsAuthRBAC")
+		return
+	}

Evidence

The reconciler passes resourceNames.CommonName into ReconcileMetricsAuthRBAC; CommonName is just
clf.Name (no namespace), and ReconcileMetricsAuthRBAC formats the ClusterRoleBinding name from that
value. Because ClusterRoleBinding is cluster-scoped, the name must be globally unique; otherwise
reconcilers will fight by updating Subjects.

internal/controller/observability/collector.go[72-76]
internal/factory/resource_names.go[33-48]
internal/auth/rbac.go[31-36]
internal/reconcile/rbac.go[46-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
The metrics auth ClusterRoleBinding name is derived from `ForwarderResourceNames.CommonName` which equals `clf.Name`, so it collides cluster-wide across namespaces.
### Issue Context
ClusterRoleBindings are cluster-scoped; using only `clf.Name` can cause subject overwrites and unintended privilege grants.
### Fix Focus Areas
- internal/controller/observability/collector.go[72-76]
- internal/auth/rbac.go[31-36]
- internal/factory/resource_names.go[33-48]
### Expected change
Generate the CRB name with namespace included (e.g., `cluster-logging-<namespace>-<clfName>-metrics-auth`), and use that consistently in tests and any callers.

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

---

2. Golden TOMLs missing auth block 🐞 Bug ≡ Correctness

Description

PrometheusOutput now always emits an auth block for the prometheus_exporter sink, but the embedded
golden TOML fixtures weren’t updated. The config generation tests that compare against these
fixtures will fail.

Code

internal/generator/vector/output/metrics/prometheus.go[R53-57]

+		s.Auth = &sinks.PrometheusExporterAuth{
+			Strategy: sinks.PrometheusExporterAuthStrategySar,
+			Path:     "/metrics",
+			Verb:     "get",
+		}

Evidence

The generator now sets s.Auth non-nil, which will serialize into TOML output. The full-config
generation tests compare the generated config to embedded TOML files; those fixtures currently
include the prometheus exporter sink and tls blocks but no auth block.

internal/generator/vector/output/metrics/prometheus.go[40-58]
internal/generator/vector/conf/conf_test.go[56-66]
internal/generator/vector/conf/complex.toml[840-851]
internal/generator/vector/conf/container.toml[423-434]

Agent prompt

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

## Issue description
Config generation now serializes a `[sinks.prometheus_output.auth]` block, but the golden TOML files used by config-generation tests don’t include it.
### Issue Context
`conf_test.go` compares generated config to embedded `*.toml` fixtures; adding `s.Auth` changes the generated TOML deterministically.
### Fix Focus Areas
- internal/generator/vector/output/metrics/prometheus.go[40-58]
- internal/generator/vector/conf/conf_test.go[56-66]
- internal/generator/vector/conf/complex.toml[840-851]
- internal/generator/vector/conf/complex_http_receiver.toml[895-910]
- internal/generator/vector/conf/container.toml[423-434]
### Expected change
Update each TOML fixture that includes `sinks.prometheus_output` to add:

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

---

3. Metrics auth RBAC not cleaned ☑ 🐞 Bug ⛨ Security

Description

The new metrics-auth ClusterRoleBinding is created/updated but never deleted when a
ClusterLogForwarder is removed, leaving a cluster-scoped binding to system:auth-delegator behind.
This can preserve tokenreview/SAR privileges for a ServiceAccount beyond the CLF lifecycle
(including after SA recreation).

Code

internal/controller/observability/collector.go[R72-76]

+	// Add ClusterRoleBinding to allow the collector to validate bearer tokens for metrics endpoint
+	if err = auth.ReconcileMetricsAuthRBAC(context.Client, resourceNames.CommonName, context.Forwarder.Namespace, context.Forwarder.Spec.ServiceAccount.Name); err != nil {
+		log.V(3).Error(err, "auth.ReconcileMetricsAuthRBAC")
+		return
+	}

Evidence

The controller exits reconciliation immediately when a CLF has a DeletionTimestamp, and there is no
corresponding delete call for the metrics-auth ClusterRoleBinding. Since the resource is
cluster-scoped and the CLF is namespace-scoped, it cannot be garbage-collected via ownership, so it
will remain unless explicitly removed.

internal/controller/observability/collector.go[72-76]
internal/controller/observability/clusterlogforwarder_controller.go[67-80]
internal/reconcile/rbac.go[72-76]
internal/auth/rbac.go[31-36]

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 operator creates a cluster-scoped `*-metrics-auth` ClusterRoleBinding but never removes it when the owning ClusterLogForwarder is deleted.
### Issue Context
`ClusterLogForwarderReconciler.Reconcile` returns early on deletion and does not clean up cluster-scoped RBAC. ClusterRoleBindings cannot be owned by namespace-scoped objects for GC cleanup.
### Fix Focus Areas
- internal/controller/observability/clusterlogforwarder_controller.go[67-80]
- internal/auth/rbac.go[31-36]
- internal/reconcile/rbac.go[72-76]
- internal/controller/observability/collector.go[72-76]
### Expected change
Add explicit cleanup for the metrics-auth ClusterRoleBinding when `DeletionTimestamp != nil` (and/or via a finalizer). Compute the CRB name using the same naming scheme as creation (ideally namespace-qualified per the collision fix), then call `reconcile.DeleteClusterRoleBinding(...)`, ignoring NotFound errors.

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

---

[jcantrill]

/label tide/merge-method-squash

[vparfonov]

How rename impact instaled cluster during upgrade?

[jcantrill]

This functionality is new in this release, added by @Clee2691 so it will not matter

[jcantrill]

/retest

[openshift-ci]

@jcantrill: The following tests 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/functional-target	fb217a4	link	true	/test functional-target
ci/prow/e2e-target	fb217a4	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.