fix(helm): support authenticated ServiceMonitor scrapes - #30565
fix(helm): support authenticated ServiceMonitor scrapes#30565brtydse100 wants to merge 1 commit into
Conversation
Greptile SummaryThis PR adds opt-in bearer-token authentication for Prometheus Operator ServiceMonitor scrapes against the LiteLLM
Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| helm/litellm-helm/templates/servicemonitor.yaml | Adds an authorization.credentials block (Prometheus Operator v1 format) guarded by authSecret.enabled, with required validation on both name and key. Indentation aligns correctly with other endpoint fields. No fallback to master-key secret. |
| helm/litellm-helm/tests/servicemonitor_tests.yaml | New helm-unittest suite covering: default off, unauthenticated on, authenticated on, and both required-field validations. All cases are meaningful and correctly assert template failure for missing name/key. |
| helm/litellm-helm/values.yaml | Adds authSecret block nested under serviceMonitor with safe defaults (enabled: false, empty name and key). No existing defaults changed. |
| helm/litellm-helm/README.md | Documents the new serviceMonitor.authSecret.* values in the reference table and adds a usage example. Content is accurate and matches the template behaviour. |
| helm/litellm-helm/Chart.yaml | Bumps chart version from 1.1.1 to 1.1.2, which is the correct semver patch increment for a new opt-in feature. |
Reviews (7): Last reviewed commit: "fix(helm): support authenticated Service..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
PR overviewThis pull request updates the Helm deployment configuration and documentation to support authenticated ServiceMonitor scrapes. One security issue has been addressed, but the production deployment examples still explicitly enable plaintext HTTP. Users who copy these examples could expose bearer tokens and administrative credentials to a network-positioned attacker; the risk depends on deploying the example without adding TLS. Open issues (1)
Fixed/addressed: 1 · PR risk: 4/10 |
| # Production: provide an ACM cert. Without one, set allow_plaintext_alb = true | ||
| # (dev/trial only). | ||
| # acm_certificate_arn = "arn:aws:acm:us-west-2:111122223333:certificate/..." | ||
| allow_plaintext_alb = true |
There was a problem hiding this comment.
Low: Production deployment examples opt into plaintext HTTP
The module defaults fail closed unless TLS is configured, but this new production-oriented example explicitly sets allow_plaintext_alb = true and the GCP example does the same with allow_plaintext_lb = true on line 515. Users who copy the snippets deploy the proxy over HTTP, letting a network attacker capture LiteLLM bearer tokens and admin credentials; keep these flags commented out or show a certificate/domain placeholder instead.
|
Thanks for your contribution! A few things to get this ready:
We're also triggering a Greptile code review: |
Hey, |
|
Thanks for the PR! A couple of things to get this over the finish line:
Once those are addressed we'll take another look — appreciate the contribution! |
|
Ye, I checked the CI checks and from what i can tell they are nor failing because of my pr |
|
what's the current state of this? So this functionality in the helm-chart is needed. |
|
Hi, |
|
too many unnecessary changes in here, we have sonnet 5 already and a GCP template in the README. could you explain your problem and what the PR does to solve it ? |
|
Yes, the problem I encountered was that if you enabled the ServiceMonitor in the values.yaml in the helm chart, you woudln't receive any metrics from LiteLLM because of a new change that requires authentication in the form of a bearer token to access the /metrics endpoint. |
9803690 to
cd12c77
Compare
cd12c77 to
1c166dc
Compare
|
I rebased this PR onto the current
Could you please review it or assign an appropriate Helm maintainer? |
TLDR
Problem this solves:
/metricsauthorizationHow it solves it:
User Flow
Before: an operator enables the ServiceMonitor, but Prometheus receives 401 responses because the scrape has no authorization header
serviceMonitorand reference that Secret in Helm valuesAfter: the same configuration produces authenticated scrapes and Prometheus can collect LiteLLM metrics
serviceMonitorand reference that Secret in Helm valuesRelevant issues
Linear 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
Shared command:
Before (d0a815d)
After (cd12c77)
Type
Bug Fix
Documentation
Test
Caveats (if any)
Final Attestation