fix(helm): authenticate ServiceMonitor scrapes of the /metrics endpoint - #32360
fix(helm): authenticate ServiceMonitor scrapes of the /metrics endpoint#32360mubashir1osmani wants to merge 2 commits into
Conversation
Greptile SummaryThis PR fixes the Helm chart's
Confidence Score: 5/5Safe to merge; the change is confined to the Helm chart and fixes a broken default with a well-designed fail-fast guard. The template logic correctly handles all three credential modes and aborts rendering with a descriptive error when no mode is selected. The PR description includes concrete helm template and live-proxy proofs for each code path. The only gap is the absence of helm-unittest cases for the new servicemonitor.yaml branches, which is minor given the manual verification already provided. helm/litellm-helm/templates/servicemonitor.yaml — the new authSecret branching logic has no corresponding unit test file, unlike every other chart template.
|
| Filename | Overview |
|---|---|
| helm/litellm-helm/templates/servicemonitor.yaml | Adds authorization block to the ServiceMonitor endpoint with three paths (custom secret, master key, or fail-fast); logic is correct and the fail-fast default prevents silent 401 scrapes, but no unit tests cover the new branches |
| helm/litellm-helm/values.yaml | Adds authSecret block under serviceMonitor with sensible defaults (enabled: true, name/key empty, useMasterKey: false) that trigger the fail-fast guard when ServiceMonitor is enabled without credentials |
| helm/litellm-helm/Chart.yaml | Version bumped from 1.1.0 to 1.2.0 to reflect the new authSecret feature |
| helm/litellm-helm/README.md | Adds documentation rows for serviceMonitor.authSecret.* values to the README parameter table |
Reviews (2): Last reviewed commit: "fix(helm): require explicit metrics scra..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
…ulting to the master key
Relevant issues
Supersedes #30565, which fixes the same problem but was written against
deploy/charts/litellm-helmbefore the chart moved tohelm/litellm-helmin #32234, and carries unrelated README changes. This PR rebuilds the fix on the current chart location and addresses the review feedback left on that PRLinear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Before, at base commit 7d15f2f, the chart renders the scrape endpoint with no credentials:
so every scrape Prometheus makes is an unauthenticated GET of
/metrics/, which the proxy rejects sincerequire_auth_for_metrics_endpointdefaults to true. Reproduced against a live proxy on localhost (prometheus callback enabled, master keysk-1234):After, at a06517c, pointing the ServiceMonitor at a secret holding a scrape token renders bearer credentials:
and explicitly opting into the master key reuses the chart's masterkey secret, honoring
masterkeySecretName/masterkeySecretKeywhen set:Enabling the ServiceMonitor without choosing a credential fails at render time with instructions instead of silently deploying a broken scrape or handing Prometheus the admin key:
To prove the header the ServiceMonitor now sends is exactly what
/metricsneeds, a real completion through the live proxy followed by a scrape with that bearer token:The existing chart test suite still passes with the same invocation as the
helm_unit_test.ymlworkflow (helm unittest -f 'tests/*.yaml' helm/litellm-helm, 54 tests) andhelm lintis cleanType
🐛 Bug Fix
Changes
Enabling
serviceMonitor.enabledin the helm chart produces a ServiceMonitor whose scrapes always fail with 401 because the proxy now requires a bearer token on/metricsby default (require_auth_for_metrics_endpointdefaults to true inlitellm/__init__.py), so users get no metrics at allThe ServiceMonitor endpoint now sends
Authorization: Bearer <token>using the prometheus-operatorauthorizationfield (the modern replacement for the deprecatedbearerTokenSecret). A newserviceMonitor.authSecretvalues block controls where the token comes from:authSecret.name/authSecret.keyreference your own secret (a dedicated low privilege virtual key is recommended since whatever token Prometheus mounts can be replayed against the proxy), whileauthSecret.useMasterKey: trueexplicitly opts into scraping with the chart's master key secret, honoringmasterkeySecretName/masterkeySecretKeywith the same fallback logic asdeployment.yaml. There is deliberately no silent fallback to the master key; if the ServiceMonitor is enabled without choosing a credential, rendering fails with a message listing the options, and settingauthSecret.namewithoutauthSecret.keyfails rather than guessing a key.authSecret.enabled: falserestores the old credential-free endpoint for deployments that run withrequire_auth_for_metrics_endpoint: falseChart version bumped to 1.2.0 and the new options are documented in the chart README