feat(inference)!: replace managed routes with providers - #3194
feat(inference)!: replace managed routes with providers#3194varshaprasad96 wants to merge 3 commits into
Conversation
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
d08ae8d to
ead1b96
Compare
politerealism
left a comment
politerealism
left a comment
There was a problem hiding this comment.
Automated code review (full diff review at high effort). Overall this is a well-executed refactor — the CLI service/settings module extraction, SigV4/provider-native inference logic, and the openshell-router crate removal all check out clean across independent verification passes (including a full dangling-reference sweep across Rust crates, proto, Go/TS/Python SDKs, docs, and skills). One finding below is a real policy/security regression that should be addressed before merge; the rest are minor.
Also flagging (no inline anchor, file fully deleted): e2e/python/test_inference_routing.py was deleted with no Python-side replacement — the new provider-native E2E coverage landed only in e2e/rust/tests/host_gateway_alias.rs. If that's the intended coverage split, worth a one-line note in the PR description; otherwise this is a coverage gap for the 'Docker-backed provider-native inference E2E' checklist item.
| binaries: [] | ||
| credentials: [aws_access_key_id, aws_secret_access_key, aws_session_token] | ||
| endpoints: | ||
| - host: "bedrock-runtime.*.amazonaws.com" |
There was a problem hiding this comment.
Correctness/security — silently widens egress for existing aws-bedrock attachments.
The old aws-bedrock profile was intentionally non-egress-granting (endpoints: []) for bridge-fronted operators. This change gives it real, enforced Bedrock endpoints with read-write access. Because ProviderTypeProfile is resolved live from the compiled-in catalog rather than pinned per-provider at attach time (see provider_profile_endpoints_are_active in crates/openshell-server/src/grpc/provider.rs, which returns true unconditionally for any builtin profile other than openai/anthropic), any operator with a pre-existing aws-bedrock provider attachment will have their sandboxes' network policy silently widened the moment the gateway upgrades — no re-consent step, and it's not called out in the 'Migration from Managed Inference Routes' doc section.
Could this ship with an explicit migration note (and ideally a policy diff surfaced to the operator on upgrade) rather than a transparent widening?
| let (base_url_keys, default_base_url): (&[&str], &str) = match profile.id.as_str() { | ||
| "openai" => (&["OPENAI_BASE_URL"], "https://api.openai.com/v1"), | ||
| "anthropic" => (&["ANTHROPIC_BASE_URL"], "https://api.anthropic.com"), | ||
| _ => return true, | ||
| }; |
There was a problem hiding this comment.
Minor maintainability nit: OPENAI_BASE_URL/ANTHROPIC_BASE_URL and their canonical default URLs are hardcoded here as inline literals rather than sourced from the profile's own YAML definition, and the fallback arm (_ => return true) is exactly what makes the aws-bedrock finding above possible — any builtin profile not explicitly listed here is always treated as 'active'. Worth deriving this from the profile data (or at least adding a comment noting new builtin profiles with real endpoints must be added here) so a future profile with real endpoints doesn't silently bypass this check the same way.
| if let Some(pos) = spec.rfind(':') { | ||
| let addr = &spec[..pos]; | ||
| let port_str = &spec[pos + 1..]; | ||
| if let Ok(port) = port_str.parse::<u16>() { | ||
| if addr.is_empty() { | ||
| return Err(miette::miette!("bind address is required before ':'")); | ||
| } | ||
| return Ok((addr.to_string(), port)); | ||
| } | ||
| } |
There was a problem hiding this comment.
parse_tcp_forward_spec splits on the last : to separate an optional bind address from the port but doesn't strip IPv6 brackets. openshell service forward --local '[::1]:9090' ... yields bind_addr = "[::1]" (brackets included), and TcpListener::bind(("[::1]", 9090)) fails because Rust's (&str, u16) tuple parser doesn't accept bracket-wrapped IPv6 literals — so IPv6 loopback/bind addresses can't be used with this flag today.
| if let Some(pos) = spec.rfind(':') { | |
| let addr = &spec[..pos]; | |
| let port_str = &spec[pos + 1..]; | |
| if let Ok(port) = port_str.parse::<u16>() { | |
| if addr.is_empty() { | |
| return Err(miette::miette!("bind address is required before ':'")); | |
| } | |
| return Ok((addr.to_string(), port)); | |
| } | |
| } | |
| if let Some(pos) = spec.rfind(':') { | |
| let addr = &spec[..pos]; | |
| let port_str = &spec[pos + 1..]; | |
| if let Ok(port) = port_str.parse::<u16>() { | |
| let addr = addr | |
| .strip_prefix('[') | |
| .and_then(|a| a.strip_suffix(']')) | |
| .unwrap_or(addr); | |
| if addr.is_empty() { | |
| return Err(miette::miette!("bind address is required before ':'")); | |
| } | |
| return Ok((addr.to_string(), port)); | |
| } | |
| } |
|
Please coordinate with maintainers before picking up issues, this is a duplicate of #3195 |
3 participants
Summary
Remove the managed inference-route stack now that provider v2 supplies native provider endpoints, and fold the next service/settings CLI module extraction into the same clean replacement branch. This keeps the #2304 refactor aligned with the architecture established by #3172.
Related Issue
Closes #3172
Relates to #2304
Supersedes #3188
Changes
/v1base URL, correct provider attach and custom endpoint guidance, and remove stale managed-inference references.inference.local.Testing
mise run pre-commitpassesmise run cipassesmise run go:cipassesmise run e2enot run locally; the affected Docker-backed scenario passedChecklist