v1 API uplift: Agent/Endpoint/Upstream replace Session/Tunnel/Builder#202
v1 API uplift: Agent/Endpoint/Upstream replace Session/Tunnel/Builder#202Copilot wants to merge 4 commits into
Conversation
Agent-Logs-Url: https://github.com/ngrok/ngrok-rust/sessions/e3eef50d-5dec-4058-8318-47b2c0827c0d Co-authored-by: jonstacks <6900888+jonstacks@users.noreply.github.com>
…k to v2 API Agent-Logs-Url: https://github.com/ngrok/ngrok-rust/sessions/e3eef50d-5dec-4058-8318-47b2c0827c0d Co-authored-by: jonstacks <6900888+jonstacks@users.noreply.github.com>
- Add explicit imports for v1 types (HttpTunnelBuilder, Scheme, Policy, etc.) from their internal module paths in crate::config, crate::tunnel, and crate::conn - Re-add pub(crate) endpoint builder methods (http_endpoint, tcp_endpoint, tls_endpoint, labeled_tunnel) to Session - Re-export OauthOptions via pub(crate) use in config module - Import TunnelBuilder, ForwarderBuilder, TunnelInfo, and ConnInfo traits needed for .listen(), .listen_and_forward(), .metadata(), .forwards_to(), and .remote_addr() methods Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: jonstacks <6900888+jonstacks@users.noreply.github.com>
…version to 1.0.0 Agent-Logs-Url: https://github.com/ngrok/ngrok-rust/sessions/e3eef50d-5dec-4058-8318-47b2c0827c0d Co-authored-by: jonstacks <6900888+jonstacks@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a breaking v1.0.0 redesign of the ngrok crate’s public API, replacing the v1 Session/protocol-specific tunnel builders with a smaller v2 surface centered on Agent, unified endpoint builders, and Upstream, plus updated docs/examples and an ffi feature.
Changes:
- Adds v2 API modules (
agent,endpoint,endpoint_builder,upstream,event,rpc_handler,default_agent) and re-exports them fromlib.rs. - Moves v1 configuration/session/tunnel/forwarder types behind
pub(crate)and updates internal imports accordingly. - Updates README, examples, changelog, and
cargo-doc-ngrokto the v2 API; bumps crate version to1.0.0and addsffifeature flag.
Reviewed changes
Copilot reviewed 30 out of 31 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| ngrok/src/upstream.rs | New v2 upstream configuration type. |
| ngrok/src/agent.rs | New v2 Agent/AgentBuilder implementation and session management. |
| ngrok/src/endpoint.rs | New v2 unified endpoint listener/forwarder types and Endpoint trait. |
| ngrok/src/endpoint_builder.rs | New unified endpoint builder and URL-scheme dispatch logic. |
| ngrok/src/default_agent.rs | New global default-agent convenience (listen/forward/default_agent). |
| ngrok/src/event.rs | New v2 agent event enum. |
| ngrok/src/rpc_handler.rs | New v2 RPC handler trait and method constants. |
| ngrok/src/ffi.rs | New ffi-gated type-erased wrappers for consumers. |
| ngrok/src/lib.rs | Rewires module visibility and v2 re-exports; updates prelude. |
| ngrok/src/session.rs | Restricts v1 entrypoints to pub(crate) and trims exports. |
| ngrok/src/tunnel.rs | Adjusts internal imports after module visibility changes. |
| ngrok/src/tunnel_ext.rs | Updates imports to internal module paths post-uplift. |
| ngrok/src/forwarder.rs | Updates imports to internal module paths post-uplift. |
| ngrok/src/internals/raw_session.rs | Fixes import path for internal Session. |
| ngrok/src/online_tests.rs | Updates test imports to internal module paths. |
| ngrok/src/config/common.rs | Updates imports for internalized session/tunnel usage. |
| ngrok/src/config/http.rs | Updates internal Session import path. |
| ngrok/src/config/tcp.rs | Updates internal Session import path. |
| ngrok/src/config/tls.rs | Updates internal Session import path. |
| ngrok/src/config/labeled.rs | Updates internal Session import path. |
| ngrok/examples/axum.rs | Migrates example to v2 listen().start(). |
| ngrok/examples/connect.rs | Migrates example to v2 Agent/EndpointListener. |
| ngrok/examples/labeled.rs | Updates example (now no longer labeled-specific). |
| ngrok/examples/mingrok.rs | Migrates minimal forwarder example to v2 forward(). |
| ngrok/examples/tls.rs | Migrates TLS example to v2 listen().url(...).start(). |
| ngrok/README.md | Updates quickstart/deps snippet to v1.0.0 and v2 usage. |
| ngrok/CHANGELOG.md | Adds v1.0.0 entry and migration guide. |
| ngrok/Cargo.toml | Bumps to 1.0.0; adds ffi feature flag. |
| Cargo.lock | Updates locked ngrok version to 1.0.0. |
| cargo-doc-ngrok/src/main.rs | Migrates to v2 Agent/listen().start(). |
| cargo-doc-ngrok/Cargo.toml | Updates ngrok dependency and features for v2. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| use crate::config::ProxyProto; | ||
|
|
||
| /// Configuration for the upstream (backend) service that traffic is forwarded to. | ||
| /// |
There was a problem hiding this comment.
Upstream is a public type, but it imports and exposes crate::config::ProxyProto (via the proxy_proto field/method). Since config is pub(crate) in lib.rs, this makes Upstream's public API reference a non-public type and will fail to compile for external users (and likely fail rustdoc doctests too). Consider either re-exporting ProxyProto as part of the new public API (e.g., pub use internals::proto::ProxyProto), or removing proxy_proto from Upstream / replacing it with a new public enum in the v2 surface.
| /// let upstream = Upstream::new("https://localhost:8443"); | ||
| /// | ||
| /// // With custom PROXY protocol | ||
| /// let upstream = Upstream::new("localhost:8080") | ||
| /// .proxy_proto(ngrok::config::ProxyProto::V2); | ||
| /// ``` |
There was a problem hiding this comment.
The doctest example refers to ngrok::config::ProxyProto, but config is now pub(crate) in lib.rs, so this example will not compile for crate users and may break rustdoc tests. Update the example to use the intended public path (or remove the example if ProxyProto is no longer part of the public API).
| pub fn default_agent() -> Agent { | ||
| // We can't use async here, so we try a non-blocking approach | ||
| // The agent is lazily initialized but build() is sync | ||
| let guard = DEFAULT_AGENT.blocking_lock(); | ||
| if let Some(ref agent) = *guard { | ||
| return agent.clone(); | ||
| } |
There was a problem hiding this comment.
default_agent() uses tokio::sync::Mutex::blocking_lock(). This can panic or block the executor if default_agent()/listen()/forward() are called from within an async runtime thread (which is the common usage pattern in a Tokio app). Consider using a non-async synchronization primitive for the global (e.g., std::sync::OnceLock/std::sync::Mutex/RwLock), or making the top-level convenience functions async so they can await the lock safely.
| let mut session_guard = self.inner.session.lock().await; | ||
| if session_guard.is_some() { | ||
| return Ok(()); | ||
| } | ||
| let config = self.inner.config.lock().await.clone(); | ||
| let session = build_session(&config).await?; | ||
| *session_guard = Some(session); |
There was a problem hiding this comment.
Agent::connect holds self.inner.session.lock().await across the build_session(&config).await call. Holding a Tokio mutex guard across an .await risks deadlocks and prevents other tasks from interacting with the agent while the network connection is in progress. Release the lock before awaiting (e.g., check state, drop guard, build session, then re-lock to store with a double-check).
| let mut session_guard = self.inner.session.lock().await; | |
| if session_guard.is_some() { | |
| return Ok(()); | |
| } | |
| let config = self.inner.config.lock().await.clone(); | |
| let session = build_session(&config).await?; | |
| *session_guard = Some(session); | |
| { | |
| let session_guard = self.inner.session.lock().await; | |
| if session_guard.is_some() { | |
| return Ok(()); | |
| } | |
| } | |
| let config = self.inner.config.lock().await.clone(); | |
| let session = build_session(&config).await?; | |
| let mut session_guard = self.inner.session.lock().await; | |
| if session_guard.is_none() { | |
| *session_guard = Some(session); | |
| } |
| let mut session_guard = self.inner.session.lock().await; | ||
| if let Some(mut session) = session_guard.take() { |
There was a problem hiding this comment.
Agent::disconnect calls session.close().await while still holding the self.inner.session mutex guard. This can block other operations and risks deadlock if close() needs to interact with agent state. Take the session out, drop the guard, then await the close.
| let mut session_guard = self.inner.session.lock().await; | |
| if let Some(mut session) = session_guard.take() { | |
| let session = { | |
| let mut session_guard = self.inner.session.lock().await; | |
| session_guard.take() | |
| }; | |
| if let Some(mut session) = session { |
| /// [`futures::Stream`] yielding [`Conn`](crate::conn::ConnInner) values | ||
| /// and provides the [`Endpoint`] trait for metadata access. |
There was a problem hiding this comment.
The EndpointListener docs claim the stream yields ConnInner values (Stream doc comment), but the actual Stream::Item is Result<EndpointConn, AcceptError>. Update the docs to match the public type (EndpointConn) and link to the correct type in the comment.
| /// [`futures::Stream`] yielding [`Conn`](crate::conn::ConnInner) values | |
| /// and provides the [`Endpoint`] trait for metadata access. | |
| /// [`futures::Stream`] yielding [`Result`]`<`[`EndpointConn`], [`AcceptError`]`>` | |
| /// values and provides the [`Endpoint`] trait for metadata access. |
| impl From<RpcError> for FfiError { | ||
| fn from(err: RpcError) -> Self { | ||
| use crate::internals::proto::Error; | ||
| FfiError { | ||
| code: err.error_code().map(|s| s.to_string()), | ||
| message: err.msg(), | ||
| } |
There was a problem hiding this comment.
From<RpcError> for FfiError imports crate::internals::proto::Error but never uses it, which triggers an unused import warning. Remove the import or use it (e.g., to extract an error code/message if needed).
| // .proxy_proto(ProxyProto::None) | ||
| .termination(CERT.into(), KEY.into()) | ||
| let mut listener = ngrok::listen() | ||
| .url("tls://0.0.0.0:0") |
There was a problem hiding this comment.
This example uses .url("tls://0.0.0.0:0"), but the v2 URL parser for tls:// only uses host_str() and ignores the port, and the underlying TLS endpoint config expects a domain rather than a host:port pair. This makes the example misleading and potentially non-functional. Consider switching to a domain-style TLS URL (e.g. tls://example.ngrok.app) or updating the builder/parser to explicitly support host:port for TLS if that’s intended.
| .url("tls://0.0.0.0:0") | |
| .url("tls://example.ngrok.app") |
| let mut listener = ngrok::listen() | ||
| .metadata("example tunnel metadata from rust") | ||
| .listen() | ||
| .start() | ||
| .await?; | ||
|
|
||
| println!("Labeled listener started!"); | ||
| println!("Listener started on: {}", listener.url()); |
There was a problem hiding this comment.
examples/labeled.rs no longer demonstrates labeled tunnels (it just calls ngrok::listen() with no labels), but the file name and surrounding context imply labeled-edge usage. Either update the example to demonstrate the v2 equivalent of labeled endpoints (if supported) or remove/rename the example to avoid confusion.
|
|
||
| Ok((scheme, host_port)) | ||
| } | ||
|
|
There was a problem hiding this comment.
The new URL parsing/dispatch logic in parse_endpoint_url/dispatch_listen is central to the v2 API behavior (scheme→protocol mapping, host/port handling, error messages), but this new module doesn’t appear to have unit tests. Adding tests for representative inputs (https/http/tcp/tls, invalid schemes, tls ports) would help prevent regressions during this breaking API transition.
| #[cfg(test)] | |
| mod tests { | |
| use super::parse_endpoint_url; | |
| #[test] | |
| fn parse_https_url_returns_scheme_and_domain() { | |
| let parsed = parse_endpoint_url("https://example.com").unwrap(); | |
| assert_eq!(parsed, ("https".to_string(), Some("example.com".to_string()))); | |
| } | |
| #[test] | |
| fn parse_http_url_returns_scheme_and_domain() { | |
| let parsed = parse_endpoint_url("http://example.com").unwrap(); | |
| assert_eq!(parsed, ("http".to_string(), Some("example.com".to_string()))); | |
| } | |
| #[test] | |
| fn parse_tcp_url_returns_scheme_and_host_port() { | |
| let parsed = parse_endpoint_url("tcp://example.com:1234").unwrap(); | |
| assert_eq!( | |
| parsed, | |
| ("tcp".to_string(), Some("example.com:1234".to_string())) | |
| ); | |
| } | |
| #[test] | |
| fn parse_tls_url_returns_scheme_and_domain_without_port() { | |
| let parsed = parse_endpoint_url("tls://example.com:443").unwrap(); | |
| assert_eq!(parsed, ("tls".to_string(), Some("example.com".to_string()))); | |
| } | |
| #[test] | |
| fn parse_invalid_scheme_returns_supported_schemes_error() { | |
| let err = parse_endpoint_url("udp://example.com:53").unwrap_err(); | |
| let msg = err.to_string(); | |
| assert!(msg.contains("unsupported endpoint URL scheme 'udp'")); | |
| assert!(msg.contains("Supported: https, http, tcp, tls")); | |
| } | |
| #[test] | |
| fn parse_malformed_url_returns_invalid_url_error() { | |
| let err = parse_endpoint_url("not a url").unwrap_err(); | |
| let msg = err.to_string(); | |
| assert!(msg.contains("invalid endpoint URL 'not a url'")); | |
| } | |
| } |
Transforms the
ngrokcrate public API from v1 (protocol-specific builders,Session/Tunneltypes) to a v2 design aligned with ngrok-go v2. The exported surface area shrinks from ~30 types to ~8 core types. Breaking change release → v1.0.0.New public API
Agent/AgentBuilder— replacesSession/SessionBuilder. Syncbuild(), asyncconnect(), auto-connects on first use.EndpointListener— unified replacement forHttpTunnel,TcpTunnel,TlsTunnel,LabeledTunnel. ImplementsStream<Item = Result<EndpointConn, AcceptError>>.EndpointForwarder— replacesForwarder<T>.EndpointListenBuilder/EndpointForwardBuilder— unified config; protocol inferred from URL scheme (https://,tcp://,tls://,http://).Upstream— forwarding target config.Endpointtrait — common metadata interface (id(),url(),protocol()).ngrok::listen(),ngrok::forward(),ngrok::default_agent()backed by a global agent initialized fromNGROK_AUTHTOKEN.ffifeature flag —FfiAgent,FfiEndpointOptions,FfiUpstream,FfiErrorfor FFI consumers.What moved to
pub(crate)Session,SessionBuilder,HttpTunnelBuilder,TcpTunnelBuilder,TlsTunnelBuilder,LabeledTunnelBuilder,Tunneltrait,TunnelInfo,EndpointInfo,EdgeInfo,Forwarder<T>,OauthOptions,OidcOptions,Policy,Scheme, and the entireconfigmodule. Internal plumbing retained for the v2 endpoint builder's URL→protocol dispatch.Example
Other changes
cargo-doc-ngrokupdated to v2 APIonline_tests.rsimports fixed to use internal module pathsmuxado/untouched