feat(helm): make extra ingress paths configurable in the componentized chart - #35700
feat(helm): make extra ingress paths configurable in the componentized chart#35700yassin-berriai wants to merge 1 commit into
Conversation
|
|
96e7b69 to
4a10d6e
Compare
Greptile SummaryThe componentized Helm chart now supports configurable ingress paths targeting the gateway, backend, or UI.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the current guard rejects root entries before rendering for Prefix, Exact, and ImplementationSpecific, resolving both previous findings.
|
| Filename | Overview |
|---|---|
| helm/litellm/templates/ingress.yaml | Adds extra-path routing and validation; the unconditional root-path guard fully resolves both previously reported ingress failures. |
| helm/litellm/tests/ingress_extra_paths_tests.yaml | Covers routing defaults, service selection, ordering, malformed configuration, built-in duplicates, and all three root path types. |
| helm/litellm/values.yaml | Defines and documents the additive ingress.extraPaths configuration with safe defaults. |
Reviews (4): Last reviewed commit: "feat(helm): make extra ingress paths con..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
a16d553 to
02b4057
Compare
|
@greptileai please review the current head You were right, and the fix went further than the root case. An entry at The chart now rejects any entry whose An entry that differs only by Seven new cases cover it, one per built-in family plus the allowed different-pathType case. Six mutants were run against them and each was killed: dropping the guard, emptying the key seed, skipping the UI paths, skipping the gateway prefixes, and two variants of keying on Note CI is red for a reason unrelated to this diff, which contains no Python. |
02b4057 to
ef38acb
Compare
|
@greptileai please review the current head Rebased onto current staging, which was failing |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…d chart
The componentized chart's Ingress renders a fixed path set: the UI paths, a
hardcoded gateway prefix list mirroring gateway/routes/allowlist.py, and a `/`
Prefix catch-all to the backend. values.yaml exposes no path knob, so an
operator who needs a route the chart does not know about has to fork the
template.
That prefix list is a snapshot of the data plane at release time and drifts
from it. `/watsonx` is in GATEWAY_PATH_PREFIXES with no ingress rule and no
backend prefix, so `/watsonx/*` falls into the catch-all, reaches the backend,
and 404s. A provider passthrough prefix added upstream after the chart version
an operator runs has the same shape, and a custom
general_settings.pass_through_endpoints route has a path only the operator
knows, so the chart can never ship a rule for it at all.
ingress.extraPaths takes a list of {path, service, pathType} entries rendered
in addition to the built-in paths, never in place of them. They render after
every built-in path and before the backend catch-all, and an entry repeating a
path the chart already routes is rejected: duplicates within one rule are
resolved by position or by controller-specific tie breaking, so an entry at
`/` Prefix would swallow the whole backend management API rather than add to
it. `service` picks the component Service — gateway by default, or backend or
ui — so an operator does not have to reconstruct release-templated Service
names, and `pathType` defaults to Prefix. A non-mapping entry, an entry with
no path, an unknown service, an unknown pathType, and a duplicate each abort
the render naming the offending index rather than emitting an Ingress that
misroutes traffic.
The duplicate check needs the built-in paths as data, so the UI paths move
from six literal YAML blocks into a $uiPaths list rendered by one range, and
their explanatory comments move from the rendered manifest into template
comments. The Ingress object this produces is unchanged: the same 92 paths in
the same order with the same backends, verified by parsing both renders.
The target component still answers only the paths its own route allowlist
keeps, so this makes a route routable, not servable.
ef38acb to
fe451d7
Compare
|
@greptileai please review the current head Right again, and the general form is stronger than the case you named. A root entry is dead under every pathType, not just Non-root entries are unaffected; the exact-pair duplicate check still governs those, and an entry differing from a built-in only by pathType is still allowed there since it is a genuinely distinct rule. Three cases cover the root, one per pathType, and two mutants were run against them: dropping the root guard, and exempting Separately, |
3 participants
TLDR
Problem this solves:
/watsonxis unreachable through this chart todaypass_through_endpointspaths can never ship a ruleHow it solves it:
ingress.extraPathslist invalues.yamlservicepicks gateway (default), backend, or uipathTypedefaults to PrefixRelevant issues
Linear ticket
Resolves LIT-5135
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
Live cluster: the routing actually changes
kind + ingress-nginx, running this chart with
ealen/echo-serverstanding in for the three component images so the pods come up and answer the chart's own probes. echo-server echoesenvironment.HOSTNAME, which is the pod name, and pod names carry-gateway-/-backend-/-ui-, so the response says which component served the request.Cluster legs were captured at
a16d553bf3; the head is nowfe451d70aa, which changed only the extra-path validation and moved the built-in UI paths from six literal blocks into one range. The Ingress object rendered from these values is identical at every commit on the branch, verified by parsing each render and comparing the objects, so the capture stands at the head.values-base.yaml(PORTmatches each chart containerPort: gateway 4000, backend 4001, ui 3000):values-extrapaths.yaml, exercising all three selectors, with/watsonxdeliberately omittingpathTypeso the run also proves thePrefixdefault:1. Cluster and controller
2. Install without
extraPathsAll three target paths fall into the
/catch-all and land on the backend, which serves none of them3. Upgrade with
extraPaths/watsonxrenderedpathType: Prefixwithout being asked, which is the documented default4. Same matrix, after
/watsonxmoved backend to gateway and/brand.txtmoved backend to ui. Every built-in path is identical to the before state, on the same pods, which never restarted (0 restarts, 38safter the upgrade)5. The backend selector needs a second form of evidence
service: backendtargets the same component the catch-all already targets, so a response cannot tell "matched my rule" from "fell through". The controller's generated config can: each entry gets its own location block with the right upstream.Combined with the before-leg
grep -creturning 0, that is the backend entry demonstrably taking effect rather than being absorbed by the catch-all6. Cleanup
$ kind delete cluster --name litellm-ingress-extrapathsWhat this run does not prove
ingress-nginx matches by longest prefix, so
/watsonxPrefix beats the/catch-all wherever it sits in the list. This run proves the routing delta and all three service selectors; it does not prove the manifest-position argument, which is specific to controllers that honour manifest order and is covered by the helm-unittest ordering assertions instead.Render aborts on a bad entry
Captured at
fe451d70aa:Type
🆕 New Feature
Changes
helm/litellm/templates/ingress.yamlrenders a fixed path set: the UI paths, a hardcoded$gatewayPrefixeslist mirroringgateway/routes/allowlist.py, and a/Prefix catch-all to the backend.values.yamlexposed no path knob, so an operator needing a route the chart does not know about had to fork the template.That prefix list is a snapshot of the data plane at chart release time and drifts from it.
/watsonxis inGATEWAY_PATH_PREFIXES(gateway/routes/allowlist.py:110, backing the real/watsonx/{endpoint:path}passthrough route) but has no ingress rule and no backend prefix, so/watsonx/*falls into the catch-all, reaches the backend, and 404s. A provider passthrough prefix added upstream after the chart version an operator runs has the same shape, and a customgeneral_settings.pass_through_endpointsroute has a path only the operator knows, so the chart can never ship a rule for it at all.ingress.extraPathstakes a list of{path, service, pathType}entries rendered in addition to the built-in paths, never in place of them. They render after every built-in path and before the backend catch-all, and two classes of entry are rejected at render time rather than shipped. An entry repeating apath+pathTypethe chart already routes would be resolved by position or by controller-specific tie breaking, so an entry at/Prefix would swallow the whole backend management API rather than add to it. And any entry at/is rejected whatever itspathType, because the chart routes the root in both directions already (Exact to ui, Prefix to backend): every request either matches the Exact rule or falls into the Prefix one, and no rule at/can be longer or more specific than those, so such an entry renders and can never capture a request. An entry that differs from a built-in only bypathTypeis still allowed elsewhere, since that is a distinct Kubernetes rule.servicepicks the component Service (gatewayby default, orbackendorui) so an operator does not have to reconstruct release-templated Service names, andpathTypedefaults toPrefix. A non-mapping entry, an entry with no path, an unknown service, an unknown pathType, and a duplicate each abort the render naming the offending index rather than emitting an Ingress that misroutes traffic. Leading-slash validation is deliberately left to the API server, which enforces it per pathType and would be the authority a chart-side copy could drift from.The duplicate check needs the built-in paths as data, so the six UI paths move from literal YAML blocks into a
$uiPathslist rendered by one range, and their explanatory comments move from the rendered manifest into template comments. The key set derives from that list plus$gatewayPrefixesplus/test|Exactand/|Prefix, so a built-in path added later extends the check automatically. The Ingress object is unchanged: the same 92 paths in the same order with the same backends, verified by parsing the render at every commit on this branch and comparing the objects.Where position matters at all: the AWS Load Balancer Controller this chart targets sorts Exact paths first and Prefix paths longest-first, and keeps ImplementationSpecific paths in manifest order, which is what the existing
/*.txtrule already depends on.The target component still answers only the paths its own route allowlist keeps, so this makes a route routable, not servable. Wiring a custom
pass_through_endpointspath through the gateway's route trim is separate work.Tests are
helm/litellm/tests/ingress_extra_paths_tests.yaml, run by the existinghelm_unit_testworkflow. Seventeen cases covering the additive contract, position relative to the catch-all, both selectors and their defaults, the component ports, every render abort, one built-in path per family rejected as a duplicate, the root rejected under all three pathTypes, and the different-pathType case that must still be allowed. Fifteen mutants were run against them and each was killed: flipping the defaultservice, flipping the defaultpathType, dropping each of the six guards, exemptingImplementationSpecificfrom the root guard, emptying the built-in key seed, skipping the UI paths or the gateway prefixes when building the key set, two forms of keying onpathalone, moving the extras below the catch-all, and reverting the template to the branch point. The chart's other 71 tests still pass unchangedosv-scanis red on a file this diff does not touch:cryptography 48.0.1inuv.lock, wheregit diff base...HEAD -- uv.lockis empty. It is a newly published advisory against the version pinned on staging, it fails the same way on #35690, and #35686 merged with it red. It needs a dependency bump of its own rather than anything hereDocs companion: BerriAI/litellm-docs#745, to merge after this
Final Attestation