feat(http): TLS support #2492
feat(http): TLS support #2492
Conversation
grahamking
requested review from
a team,
GuanLuo,
PeaBrane,
alec-flowers,
biswapanda,
ishandhanani,
jthomson04,
kkranen,
nnshah1,
paulhendricks,
piotrm-nvidia,
ptarasiewiczNV,
rmccorm4,
ryanolson,
tanmayv25,
tedzhouhk and
tmonty12
as code owners
WalkthroughAdds optional TLS/HTTPS support across the CLI, Python bindings, and HTTP service: new tls_cert_path and tls_key_path options are threaded from frontend/flags to LocalModel and HttpService, enabling TLS when both paths are provided. http_port handling is adjusted (non-optional in some layers). Docs updated; minor runtime debug assertion tweak. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FrontendCLI as Frontend CLI (Python)
participant PyBindings as EntrypointArgs (Py→Rust)
participant Builder as LocalModelBuilder
participant Model as LocalModel
participant HTTP as HttpService
User->>FrontendCLI: run with --http-port [--tls-cert-path] [--tls-key-path]
FrontendCLI->>PyBindings: EntrypointArgs(http_port, tls_cert_path?, tls_key_path?)
PyBindings->>Builder: http_port(u16), tls_cert_path(opt), tls_key_path(opt)
Builder->>Model: build() → LocalModel{http_port, tls_*}
Model->>HTTP: configure(enable_tls if both tls paths)
alt TLS enabled
HTTP->>HTTP: bind_rustls(addr, cert.pem, key.pem)
else HTTP only
HTTP->>HTTP: bind_tcp(addr)
end
HTTP-->>User: Serve requests (HTTP/HTTPS)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
lib/bindings/python/rust/llm/entrypoint.rs (2)
107-146: Defaulting http_port to 0 will bind a random ephemeral port.
http_port.unwrap_or(0)overrides the builder’s 8080 default and can start the server on an unpredictable port when the caller omits the argument. This will be very confusing for Python consumers not setting --http-port.Apply this diff to keep the expected default:
- http_port: http_port.unwrap_or(0), + // Match LocalModelBuilder's default (8080) when not provided from Python. + http_port: http_port.unwrap_or(8080),
107-146: Validate TLS args pairing at the Python boundary.Early validation provides better error messages to Python users and avoids surprising runtime behavior.
Apply this diff inside EntrypointArgs::new before constructing EntrypointArgs:
pub fn new( engine_type: EngineType, model_path: Option<PathBuf>, model_name: Option<String>, // e.g. "dyn://namespace.component.endpoint" model_config: Option<PathBuf>, endpoint_id: Option<String>, context_length: Option<u32>, template_file: Option<PathBuf>, router_config: Option<RouterConfig>, kv_cache_block_size: Option<u32>, http_port: Option<u16>, tls_cert_path: Option<PathBuf>, tls_key_path: Option<PathBuf>, extra_engine_args: Option<PathBuf>, ) -> PyResult<Self> { + if (tls_cert_path.is_some() && tls_key_path.is_none()) + || (tls_cert_path.is_none() && tls_key_path.is_some()) + { + return Err(pyo3::exceptions::PyValueError::new_err( + "tls_cert_path and tls_key_path must be provided together", + )); + }
🧹 Nitpick comments (6)
README.md (1)
121-123: Docs: add explicit HTTPS example and note about curl --insecure for self-signed certsMake it obvious how to run TLS and how to call it during local testing with a self-signed cert.
Apply this diff to improve clarity:
-# Start an OpenAI compatible HTTP server, a pre-processor (prompt templating and tokenization) and a router. -# Pass the TLS certificate and key paths to use HTTPS instead of HTTP. -python -m dynamo.frontend --http-port 8080 [--tls-cert-path cert.pem] [--tls-key-path key.pem] +# Start an OpenAI compatible HTTP/HTTPS server, a pre-processor (prompt templating and tokenization) and a router. +# Use the TLS flags to serve HTTPS. With a self-signed cert (e.g., local dev), use `curl -k/--insecure`. +python -m dynamo.frontend --http-port 8080 +python -m dynamo.frontend --http-port 8443 --tls-cert-path cert.pem --tls-key-path key.pemlib/runtime/src/component.rs (1)
280-286: Use eq_ignore_ascii_case to avoid allocations in debug_assertMinor, but avoids unnecessary lowercase allocations and reads cleaner. Only impacts debug builds.
Apply this diff:
- debug_assert_eq!( - hierarchies - .last() - .cloned() - .unwrap_or_default() - .to_lowercase(), - self.service_name().to_lowercase() - ); // it happens that in component, hierarchy and service name are the same + let last = hierarchies.last().map(|s| s.as_str()).unwrap_or(""); + debug_assert!( + last.eq_ignore_ascii_case(self.service_name().as_str()), + "it happens that in component, hierarchy and service name are the same" + );launch/dynamo-run/src/lib.rs (1)
42-45: Only apply TLS if both cert and key are present; error out otherwiseToday the builder receives whatever is present. Prefer a small guard here to avoid partially configured TLS leaking through (even if downstream validates later). Keeps the failure localized to the CLI entrypoint.
You can adopt this pattern near the builder setup:
// After setting http_port let cert = flags.tls_cert_path.take(); let key = flags.tls_key_path.take(); match (cert, key) { (Some(c), Some(k)) => { builder.tls_cert_path(Some(c)) .tls_key_path(Some(k)); } (None, None) => { /* no TLS */ } _ => { anyhow::bail!("Both --tls-cert-path and --tls-key-path must be provided together"); } }lib/llm/src/http/service/service_v2.rs (3)
201-203: Fix tracing field usage; currentaddressfield is empty.
tracing::info!(address, "...")records an empty field named address. Use structured values or drop the field.Preferred structured logging:
- tracing::info!(address, "Starting {protocol} service on: {address}"); + tracing::info!(protocol = protocol, address = %address, "Starting {protocol} service on: {address}");
235-244: TLS path lacks graceful shutdown; mirror the HTTP branch.On cancellation, the TLS server future is dropped without a graceful shutdown signal. Use axum_server::Handle to drain connections consistently.
Apply this diff:
- let server = axum_server::bind_rustls(addr, config).serve(router.into_make_service()); + let handle = axum_server::Handle::new(); + let server = axum_server::bind_rustls(addr, config) + .handle(handle.clone()) + .serve(router.into_make_service()); tokio::select! { result = server => { result.map_err(|e| anyhow::anyhow!("HTTPS server error: {}", e))?; } _ = observer.cancelled() => { - tracing::info!("HTTPS server shutdown requested"); + tracing::info!("HTTPS server shutdown requested"); + handle.graceful_shutdown(Some(Duration::from_secs(5))); } }
246-254: Avoid panic on bind failures; return an error instead.
panic!here will bring down the process. Prefer bubbling an error as in the TLS branch for consistent handling.- let listener = tokio::net::TcpListener::bind(addr) - .await - .unwrap_or_else(|_| panic!("could not bind to address: {address}")); + let listener = tokio::net::TcpListener::bind(addr) + .await + .map_err(|_| anyhow::anyhow!("could not bind to address: {address}"))?;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locklib/bindings/python/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
README.md(1 hunks)components/frontend/src/dynamo/frontend/main.py(3 hunks)launch/dynamo-run/src/flags.rs(1 hunks)launch/dynamo-run/src/lib.rs(2 hunks)lib/bindings/python/rust/llm/entrypoint.rs(4 hunks)lib/llm/Cargo.toml(1 hunks)lib/llm/src/entrypoint/input/http.rs(1 hunks)lib/llm/src/http/service/service_v2.rs(6 hunks)lib/llm/src/local_model.rs(7 hunks)lib/runtime/src/component.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
lib/llm/src/entrypoint/input/http.rs (3)
lib/llm/src/entrypoint.rs (1)
local_model(59-67)lib/llm/src/local_model.rs (4)
tls_cert_path(122-125)tls_cert_path(327-329)tls_key_path(127-130)tls_key_path(331-333)lib/llm/src/http/service/service_v2.rs (1)
builder(178-180)
lib/runtime/src/component.rs (2)
lib/tokens/src/lib.rs (1)
last(342-344)lib/llm/src/local_model.rs (1)
service_name(315-317)
components/frontend/src/dynamo/frontend/main.py (1)
lib/llm/src/local_model.rs (5)
default(63-82)tls_cert_path(122-125)tls_cert_path(327-329)tls_key_path(127-130)tls_key_path(331-333)
lib/bindings/python/rust/llm/entrypoint.rs (1)
lib/llm/src/local_model.rs (13)
model_path(86-89)model_name(91-94)model_config(96-99)endpoint_id(101-104)endpoint_id(350-352)context_length(106-109)kv_cache_block_size(112-115)http_port(117-120)http_port(323-325)tls_cert_path(122-125)tls_cert_path(327-329)tls_key_path(127-130)tls_key_path(331-333)
🔇 Additional comments (9)
lib/llm/Cargo.toml (1)
104-106: TLS deps selection looks correct and aligned with axum 0.7 ecosystemaxum-server 0.7 with the tls-rustls feature and rustls 0.23 are a good match for enabling HTTPS. No concerns here.
launch/dynamo-run/src/lib.rs (1)
23-23: mut Flags is appropriate to allow take() on OptionsThis enables moving out tls_cert_path/tls_key_path without cloning. No issues.
lib/llm/src/entrypoint/input/http.rs (1)
42-44: LGTM: Request template applied on the finalized builder.Threading the request template after resolving TLS vs. non-TLS builder is clean and avoids duplication.
components/frontend/src/dynamo/frontend/main.py (1)
212-216: LGTM: TLS flags are conditionally propagated.Passing pathlib.Path objects through kwargs to EntrypointArgs (PathBuf in Rust) is correct and keeps the flags optional.
lib/llm/src/local_model.rs (4)
117-130: API change: http_port now takes u16 (non-optional).The change simplifies callers but removes the ability to “reset to default” via None. Make sure all call sites pass an explicit port or rely on the builder default before overriding.
Would you like me to scan the codebase for any callers still using the old Option semantics?
198-206: LGTM: TLS fields are moved from builder to LocalModel in echo_full path.The take() usage avoids cloning and mirrors the main build path.
272-282: LGTM: TLS fields propagated in main build path.Consistent with the added getters; no lifetime issues.
323-333: LGTM: New getters for TLS paths and port are appropriate.Returning Option<&Path> is ergonomic and avoids allocation.
lib/bindings/python/rust/llm/entrypoint.rs (1)
171-176: LGTM: TLS paths and port plumbed into LocalModelBuilder.Cloning PathBufs here is fine; builder takes ownership.
While we expect the frontend to be deployed behind a web layer, we still want to support TLS: - The data center might require all internal communications to be encrypted. - For some deployments such as NIM a web layer is not practical. Make a self-signed certificate: ``` openssl genpkey -algorithm Ed25519 -out key.pem openssl req -new -x509 -key key.pem -days 1460 -out cert.pem -subj "/C=US/ST=CA/L=Santa Clara/O=LocalDev/OU=Dynamo/CN=example.com" ``` Start the frontend: ``` python -m dynamo.frontend --http-port 8443 --tls-cert-path /data/certs/cert.pem --tls-key-path /data/certs/key.pem ``` Remember to add `--insecure` to your curl request.
Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
While we expect the frontend to be deployed behind a web layer, we still want to support TLS:
Make a self-signed certificate:
Start the frontend:
Remember to add
--insecureto your curl request.Summary by CodeRabbit
New Features
Documentation