feat(helm): add a ServiceMonitor for the controller metrics endpoint

[younsl]

Description

This adds native ServiceMonitor support to the kagent Helm chart so controller metrics are collected by Prometheus Operator out of the box.

This is the follow up that #1803 deferred. That PR landed the metrics Service and the scrape RBAC and closed #1369, but left the ServiceMonitor out of scope. #1802 carried one and was closed as a duplicate of #1803, so the ServiceMonitor half of #1369 was never reviewed on its own and no template for it exists in the chart today. This PR is additive on top of what #1803 merged and changes none of it.

Today controller.metrics.enabled provisions the metrics Service and the ClusterRoles for authenticated scrapes, but not the resource that makes Prometheus actually scrape them. Every user running kube-prometheus-stack has to hand write a ServiceMonitor outside the chart and keep its port name and namespace in sync with the chart by hand. The bundled kagent-tools subchart already ships one, so this closes the gap for the controller.

New values live under controller.metrics.serviceMonitor, disabled by default and gated on controller.metrics.enabled. Interval, scrape timeout, honorLabels, relabelings, metric relabelings, extra labels, annotations and a namespace override are exposed. With secure serving on, the endpoint defaults to https with the ServiceAccount token and insecureSkipVerify, which matches the self signed certificate the controller serves, and both are overridable.

The metrics Service port name moved into a shared helper used by both the Service and the ServiceMonitor endpoint, so the two cannot drift when secure serving is flipped. Rendered output for existing installs is unchanged.

Changelog

Added an optional Prometheus Operator ServiceMonitor for the controller metrics endpoint, configurable under controller.metrics.serviceMonitor.

Testing

1. helm unittest helm/kagent passes with 298 tests, 15 of them new in helm/kagent/tests/controller-servicemonitor_test.yaml.
2. Nothing renders unless both controller.metrics.enabled and controller.metrics.serviceMonitor.enabled are set, and the bindAddress disable sentinel still suppresses it.
3. Rendered the secure and plaintext variants with helm template, then validated them against a live cluster with kubectl apply --dry-run=server so the manifest is checked against the real ServiceMonitor CRD rather than only the chart.

Additional Notes

Rendering the ServiceMonitor does not authorize the scrape on its own. With secure serving, the metrics reader ClusterRole still has to be bound to the Prometheus ServiceAccount, which the values comment calls out.

The chart does not gate on the monitoring.coreos.com API being present, matching what kagent-tools does. The explicit opt in flag is the gate, so a missing CRD fails loudly at install time instead of silently dropping the resource during helm template.

[EItanya]

Do you think this can be added as an extraObject in you chart instead?

[younsl]

Good question, and one I should answer carefully since I am the one who added extraObjects in #2174 and argued there that PrometheusRule belongs in it. I still think that. I would put a ServiceMonitor on the other side of that line, and here is my reasoning.

extraObjects works well when the content of the resource is org-specific and independent of the chart: ExternalSecret, NetworkPolicy, Istio Sidecar, PrometheusRule. This ServiceMonitor is a pointer at a Service the chart itself renders, so its endpoint is not user content but derived state. The port name (https vs http-metrics), scheme, tlsConfig and bearerTokenFile all follow controller.metrics.secureServing, the selector has to match kagent.controller.selectorLabels, and it has to stay unrendered under the METRICS_BIND_ADDRESS=0 sentinel. A copy in extraObjects has to hardcode those, and if someone later flips secureServing the scrape fails quietly: the manifest still applies, Prometheus just drops the target. That is the one thing I could not solve outside the chart, so this PR routes the port name through a shared helper that the Service and the ServiceMonitor both use.

For context on scope, #1803 landed the Service and the RBAC and left this piece for a follow-up, with the same value path it suggested (controller.metrics.serviceMonitor.enabled). This PR is meant to be that follow-up, additive and changing nothing that #1803 shipped. #1369 asked for both halves, so the ServiceMonitor half is the part still outstanding. @danielorbach @Daniel-Vaz, a sanity check from you would help here: is this the shape you had in mind?

Two smaller notes. The kagent-tools chart already has servicemonitor.yaml under tools.metrics.servicemonitor.*, so this would make the controller consistent with it. And there is precedent in the chart for opt-in resources that reference external CRDs, ui-httproute.yaml and openshift-route.yaml, if the Prometheus Operator dependency is the concern. It is disabled by default, so rendered output for existing installs is unchanged and helm unittest covers that.

Happy to go smaller if the values surface is the issue: I can trim to what kagent-tools exposes (enabled, interval, scrapeTimeout, labels) and drop relabelings, metricRelabelings, annotations and the namespace override. And if you would still rather this live outside the chart, I am fine closing it and sending just the shared port-name helper plus a documented ServiceMonitor example in the extraObjects block of values.yaml, so at least the port name and scheme have one reference. Whichever you prefer.

[danielorbach]

@younsl I'd advise against extraObjects for this one, even though I think it's the right answer for most things people want to bolt on.

It comes down to who owns the contents. An ExternalSecret or a NetworkPolicy is yours: you write it, you know what belongs in it. A ServiceMonitor is really just a pointer at a Service this chart renders, and nearly every field in it is decided by controller.metrics.secureServing: the port name, the scheme, whether there's a token, whether TLS verification gets skipped. Hand someone a copy to keep in their values and it's correct the day they paste it. Then somebody flips secureServing next quarter, the manifest still applies clean, and Prometheus quietly stops having a target. No error anywhere. A template in the chart just doesn't have that failure mode, because it re-renders on every upgrade.

I'm the one who punted this in #1803, and I was too cautious about dragging Prometheus Operator in. Nothing gets dragged in while the flag defaults to false, and openshift-route.yaml and ui-httproute.yaml already point at CRDs we don't ship.

In my personal opinion for this PR: controller.metrics.serviceMonitor.* is indeed what I now have in mind. Five things I'd want ticked off, and you've already done four:

• off by default, existing installs render identically
• the port name defined once and shared by the Service and the endpoint, no stray https or http-metrics literals left anywhere
• secureServing drives the whole scrape, so flipping it alone gives you a working scrape with nothing else to touch. I didn't spot a test for that pairing, just the secure and plaintext cases on their own
• bindAddress: "0" kills the ServiceMonitor the same way it kills the Service
• turning it on gives you a scrape that works, not a 403

That last one's the real gap. In #1803 I bound metrics-auth-role but left metrics-reader unbound becase I had no name to put in it. The auth role goes on the controller's own ServiceAccount, which we create, so that one writes itself. The reader role has to go on whatever ServiceAccount Prometheus runs as, and the chart had no idea whether that's prometheus-k8s or kube-prometheus-stack-prometheus or something homegrown in a namespace we never touch.

Rendering a ServiceMonitor changes that, because now the user is telling us they run Prometheus Operator. So ask them for the ServiceAccount and render the ClusterRoleBinding when they hand you one. Blank means unbound, exactly like today, so nobody's existing setup shifts. Without it, enabled: true with secureServing on drops a target into Prometheus that can never be scraped, and all anyone sees is a silent 403 behind a manifest that applied fine. A comment in values.yaml won't necessarily catch that.

One last thing, and probably outside the scope of this pull-request: we're on track for three spellings of this in one umbrella: controller.metrics.serviceMonitor.* here, tools.metrics.servicemonitor.* in kagent-tools, and kmcp with no ServiceMonitor at all plus a hardcoded https in its metrics Service. If @EItanya and the maintainers all settle on one way of spelling it, I'll be happy to apply it. I'm just not at a position to make that choice, so I try to not invent a fourth.

[younsl]

Both CI failures are pre-existing on main, not from these chart changes. The same two jobs fail on main HEAD.