From cf8d6f38665c20e9d641ff5f508514f42010ed38 Mon Sep 17 00:00:00 2001 From: Tin Date: Tue, 27 Oct 2026 14:06:08 -0700 Subject: [PATCH] fix(ui): stop login loops in split deployments --- helm/litellm/templates/ingress.yaml | 18 +++++++------ .../tests/ingress_controller_tests.yaml | 27 +++++++++++++++++++ .../tests/ingress_extra_paths_tests.yaml | 14 +++++----- .../image/test_ui_image_serves_offline.py | 19 ++++++++++--- ui/nginx.conf | 12 +++++++-- 5 files changed, 70 insertions(+), 20 deletions(-) diff --git a/helm/litellm/templates/ingress.yaml b/helm/litellm/templates/ingress.yaml index 22ff8a95566..00c11ab22a1 100644 --- a/helm/litellm/templates/ingress.yaml +++ b/helm/litellm/templates/ingress.yaml @@ -122,7 +122,9 @@ at "/" Prefix would swallow the whole backend management API) instead of adding to it. */}} -{{- $builtinPathKeys := list "/test|Exact" "/debug/memory/summary|Exact" "/|Prefix" -}} +{{- $backendCatchAllPath := ternary "/*" "/" (eq $controller "alb") -}} +{{- $backendCatchAllType := ternary "ImplementationSpecific" "Prefix" (eq $controller "alb") -}} +{{- $builtinPathKeys := list "/test|Exact" "/debug/memory/summary|Exact" (printf "%s|%s" $backendCatchAllPath $backendCatchAllType) -}} {{- if $hasComponentHosts }} apiVersion: networking.k8s.io/v1 kind: Ingress @@ -265,10 +267,10 @@ spec: precedence over a default, and before the backend catch-all. Position only decides the match on controllers that honour manifest order: the AWS Load Balancer Controller (ingress.controller=alb) - sorts Exact paths first and Prefix paths longest-first, but keeps - ImplementationSpecific paths in manifest order, which is what the - /*.txt rule above already depends on. ingress-nginx ignores order - and serves the longest matching location. + sorts all Prefix paths ahead of ImplementationSpecific paths. Both + /*.txt and the ALB catch-all use ImplementationSpecific so their + manifest order is preserved. ingress-nginx serves the longest + matching location. */}} {{- range $idx, $extra := .Values.ingress.extraPaths }} {{- if not (kindIs "map" $extra) }} @@ -288,7 +290,7 @@ spec: {{- end }} {{- $pathType := include "litellm.ingress.pathType" (dict "controller" $controller "path" $extra.path "pathType" $requestedPathType) }} {{- if eq $extra.path "/" }} - {{- fail (printf "ingress.extraPaths[%d]: path / is already routed in both directions, Exact to ui and Prefix to backend, so no pathType leaves a request for an entry here to capture" $idx) }} + {{- fail (printf "ingress.extraPaths[%d]: path / is already routed to ui, with unmatched paths routed to backend" $idx) }} {{- end }} {{- if has (printf "%s|%s" $extra.path $pathType) $builtinPathKeys }} {{- fail (printf "ingress.extraPaths[%d]: path %s with pathType %s is already routed by this chart, and a duplicate would take it over rather than add to it" $idx $extra.path $pathType) }} @@ -302,8 +304,8 @@ spec: number: {{ $target.port }} {{- end }} # --- Catch-all → backend (management API: /key/*, /user/*, /team/*, ...) --- - - path: / - pathType: Prefix + - path: {{ $backendCatchAllPath | quote }} + pathType: {{ $backendCatchAllType }} backend: service: name: {{ $backendName }} diff --git a/helm/litellm/tests/ingress_controller_tests.yaml b/helm/litellm/tests/ingress_controller_tests.yaml index aa30db3c9c1..59be37dfe1b 100644 --- a/helm/litellm/tests/ingress_controller_tests.yaml +++ b/helm/litellm/tests/ingress_controller_tests.yaml @@ -38,6 +38,33 @@ tests: name: RELEASE-NAME-litellm-ui port: number: 3000 + - equal: + path: spec.rules[0].http.paths[-1] + value: + path: /* + pathType: ImplementationSpecific + backend: + service: + name: RELEASE-NAME-litellm-backend + port: + number: 4001 + - notContains: + path: spec.rules[0].http.paths + content: + path: / + pathType: Prefix + any: true + + - it: rejects an extra wildcard that would take over the ALB backend catch-all + set: + ingress.enabled: true + ingress.extraPaths: + - path: /* + pathType: ImplementationSpecific + service: ui + asserts: + - failedTemplate: + errorMessage: "ingress.extraPaths[0]: path /* with pathType ImplementationSpecific is already routed by this chart, and a duplicate would take it over rather than add to it" - it: renders no dotted Exact or Prefix path for ingress-nginx, whose admission webhook rejects them set: diff --git a/helm/litellm/tests/ingress_extra_paths_tests.yaml b/helm/litellm/tests/ingress_extra_paths_tests.yaml index 1305af15ae2..27ed26da4c0 100644 --- a/helm/litellm/tests/ingress_extra_paths_tests.yaml +++ b/helm/litellm/tests/ingress_extra_paths_tests.yaml @@ -11,8 +11,8 @@ tests: - equal: path: spec.rules[0].http.paths[-1] value: - path: / - pathType: Prefix + path: /* + pathType: ImplementationSpecific backend: service: name: RELEASE-NAME-litellm-backend @@ -48,8 +48,8 @@ tests: - equal: path: spec.rules[0].http.paths[-1] value: - path: / - pathType: Prefix + path: /* + pathType: ImplementationSpecific backend: service: name: RELEASE-NAME-litellm-backend @@ -218,7 +218,7 @@ tests: service: gateway asserts: - failedTemplate: - errorMessage: "ingress.extraPaths[0]: path / is already routed in both directions, Exact to ui and Prefix to backend, so no pathType leaves a request for an entry here to capture" + errorMessage: "ingress.extraPaths[0]: path / is already routed to ui, with unmatched paths routed to backend" - it: rejects a root entry that would take over the UI root set: @@ -229,7 +229,7 @@ tests: service: gateway asserts: - failedTemplate: - errorMessage: "ingress.extraPaths[0]: path / is already routed in both directions, Exact to ui and Prefix to backend, so no pathType leaves a request for an entry here to capture" + errorMessage: "ingress.extraPaths[0]: path / is already routed to ui, with unmatched paths routed to backend" # A root ImplementationSpecific entry duplicates no built-in pair, so the # duplicate check alone would admit it. It is still dead: the built-in @@ -244,7 +244,7 @@ tests: service: gateway asserts: - failedTemplate: - errorMessage: "ingress.extraPaths[0]: path / is already routed in both directions, Exact to ui and Prefix to backend, so no pathType leaves a request for an entry here to capture" + errorMessage: "ingress.extraPaths[0]: path / is already routed to ui, with unmatched paths routed to backend" - it: rejects an entry that would take over a UI prefix set: diff --git a/tests/integration/database/image/test_ui_image_serves_offline.py b/tests/integration/database/image/test_ui_image_serves_offline.py index 5ff68effd7f..ab9da9c469a 100644 --- a/tests/integration/database/image/test_ui_image_serves_offline.py +++ b/tests/integration/database/image/test_ui_image_serves_offline.py @@ -23,6 +23,7 @@ import subprocess import time import uuid from collections.abc import Iterator +from typing import Final import pytest @@ -86,11 +87,13 @@ def _is_running(container: str) -> bool: ) -def _probe(network: str, container: str, path: str) -> "subprocess.CompletedProcess[str]": +def _probe( + network: str, container: str, path: str, write_out: str = "%{http_code}" +) -> "subprocess.CompletedProcess[str]": return _docker( "run", "--rm", "--network", network, CURL_IMAGE, "--silent", "--show-error", "--max-time", "10", - "--output", "/dev/null", "--write-out", "%{http_code}", + "--output", "/dev/null", "--write-out", write_out, f"http://{container}:{UI_PORT}{path}", check=False, ) @@ -124,9 +127,19 @@ def test_ui_serves_as_arbitrary_uid_read_only(ui_container: tuple[str, str]) -> f"{ARBITRARY_UID}.\n{_container_logs(container)}" ) - for path in ("/", "/ui", "/ui/login"): + for path in ("/ui", "/ui/", "/ui/login"): page = _probe(network, container, path) assert page.stdout.strip() == "200", ( f"GET {path} returned {page.stdout.strip()!r} as uid {ARBITRARY_UID}.\n" f"{_container_logs(container)}" ) + + root: Final = _probe(network, container, "/?page=logs&limit=10", "%{http_code} %header{location}") + assert root.stdout.strip() == "307 /ui/?page=logs&limit=10", root.stderr + + for path in ("/index.txt", "/ui.txt", "/ui/index.txt", "/ui/home/index.txt", "/ui/login/index.txt"): + payload: Final = _probe(network, container, f"{path}?_rsc=regression", "%{http_code} %{content_type}") + assert payload.stdout.strip() == "200 text/plain", f"GET {path}: {payload.stdout!r}\n{payload.stderr}" + + missing: Final = _probe(network, container, "/ui/missing-route/index.txt") + assert missing.stdout.strip() == "404", missing.stderr diff --git a/ui/nginx.conf b/ui/nginx.conf index dd43defcde9..f7acbdb7db8 100644 --- a/ui/nginx.conf +++ b/ui/nginx.conf @@ -91,6 +91,12 @@ http { # /ui/.txt would otherwise be rewritten to HTML and break RSC # for nested routes. A genuinely missing payload must 404 (the # router degrades to a hard navigation); never fall back to HTML. + location = /ui.txt { + try_files /index.txt =404; + } + location ~ ^/ui/(.+\.txt)$ { + try_files /$1 =404; + } location ~ \.txt$ { try_files $uri =404; } @@ -107,8 +113,10 @@ http { try_files /$1.html /$1/index.html /index.html =404; } - # `/` is handy for direct-debug port-forwards. - location = / { try_files /index.html =404; } + location = / { + absolute_redirect off; + return 307 /ui/$is_args$args; + } # Anything else (API calls etc.) returns 404 from the UI's # perspective. A reverse proxy in front of this image routes the