Refactor proxy #814
Refactor proxy #814
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
LICENSE (1)
22-45: Excellent addition of attribution for incorporated code.The inclusion of the additional MIT License for code from the hyperium/hyper-util repository is a commendable practice. This ensures proper attribution and maintains legal compliance. The added license is correctly formatted and consistent with the original MIT License structure.
Consider maintaining a separate NOTICE or ATTRIBUTION file for third-party licenses if the project incorporates code from multiple sources in the future. This can help keep the main LICENSE file concise while still providing necessary attributions.
plane/src/proxy/request.rs (1)
41-75: Solid implementation with room for error handling improvement.The
get_and_maybe_remove_bearer_tokenfunction is well-implemented, correctly handling the extraction and processing of bearer tokens from URI parts. The distinction between static and dynamic tokens is a nice touch.However, there's an opportunity to improve error handling:
Consider replacing the
expectcall on line 71 with proper error handling usingResult. This would make the function more robust and allow the caller to handle potential errors gracefully. Here's a suggested modification:pub fn get_and_maybe_remove_bearer_token(parts: &mut uri::Parts) -> Result<Option<BearerToken>, uri::InvalidUri> { // ... (rest of the function remains the same) parts.path_and_query = Some( PathAndQuery::from_str(format!("/{}{}", path, query).as_str())? ); Ok(Some(token)) }This change would propagate any
InvalidUrierror to the caller, allowing for more flexible error handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- LICENSE (1 hunks)
- dynamic-proxy/src/graceful_shutdown.rs (1 hunks)
- dynamic-proxy/tests/common/websocket_echo_server.rs (1 hunks)
- dynamic-proxy/tests/graceful.rs (1 hunks)
- dynamic-proxy/tests/graceful_https.rs (1 hunks)
- plane/plane-tests/tests/common/mod.rs (1 hunks)
- plane/plane-tests/tests/common/websocket_echo_server.rs (1 hunks)
- plane/src/proxy/proxy_server.rs (1 hunks)
- plane/src/proxy/request.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- dynamic-proxy/src/graceful_shutdown.rs
- dynamic-proxy/tests/common/websocket_echo_server.rs
- dynamic-proxy/tests/graceful.rs
- dynamic-proxy/tests/graceful_https.rs
- plane/plane-tests/tests/common/mod.rs
- plane/plane-tests/tests/common/websocket_echo_server.rs
- plane/src/proxy/proxy_server.rs
🔇 Additional comments (2)
plane/src/proxy/request.rs (2)
14-37: LGTM! Comprehensive implementation with good test coverage.The
subdomain_from_hostfunction is well-implemented, handling various edge cases and providing clear error handling. The logic is efficient and the function is well-documented.Regarding the past review comment about unit tests, I can confirm that comprehensive unit tests have been added for this function (lines 84-138), covering various scenarios including no subdomains, valid subdomains, invalid suffixes, and port handling.
77-205: Excellent test coverage addressing previous concerns.The test suite is comprehensive and well-structured, covering a wide range of scenarios for both
subdomain_from_hostandget_and_maybe_remove_bearer_tokenfunctions. This addresses the previous review comments requesting unit tests.Key points:
- Tests for
subdomain_from_host(lines 84-138) cover various cases including no subdomains, valid subdomains, invalid suffixes, and port handling.- Tests for
get_and_maybe_remove_bearer_token(lines 141-205) cover different URI structures, static tokens, and edge cases.The tests are clear, concise, and provide good coverage of edge cases, which significantly enhances the reliability of the code.
There was a problem hiding this comment.
This is a masterpiece.
Left some more nit comments, responded to all the coderabbit comments that I felt were relevant, ignored the rest.
(Btw, best way to view comments is in the "Files changed" tab rather than "Conversation" which gets cluttered and confusing.)
dynamic-proxy/src/proxy.rs
Outdated
| let url = request.uri().to_string(); | ||
|
|
||
| let res = match tokio::time::timeout(self.timeout, self.client.request(request)).await { | ||
| Ok(Ok(res)) => res, | ||
| Err(_) => { | ||
| tracing::warn!(url, "Upstream request timed out."); | ||
| return Err(ProxyError::Timeout); | ||
| } | ||
| Ok(Err(e)) => { | ||
| tracing::error!(url, "Upstream request failed: {}", e); | ||
| return Err(ProxyError::RequestFailed(e.into())); | ||
| } | ||
| }; |
There was a problem hiding this comment.
@paulgb I think this is a valid point
| Ok(msg) => { | ||
| if let Message::Text(text) = msg { | ||
| if socket.send(Message::Text(text)).await.is_err() { | ||
| break; |
There was a problem hiding this comment.
Should we panic here as well?
There was a problem hiding this comment.
Did another once-over and bumped a couple comments so nothing's lost.
The one largest thing remaining is to consider using async-trait. From my code search I found ~7 instances of -> Self::Future where I think using the async keyword can keep the code more modern and easier to read. A few of those cases have suggestions from coderabbit. I'm sure Cursor can rewrite each of the instances for you very quickly.
| *res.status_mut() = StatusCode::SWITCHING_PROTOCOLS; | ||
| res.headers_mut() | ||
| .insert(UPGRADE, HeaderValue::from_static("websocket")); | ||
|
|
There was a problem hiding this comment.
@paulgb is this valid?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
plane/src/proxy/mod.rs (3)
113-115: LGTM: ProxyState and ProxyConnection changes look good.The use of
ArcforProxyStateand the creation ofProxyConnectionalign with the refactoring objectives.Consider adding a brief comment explaining why
_proxy_connectionis kept in scope but not used directly. This would improve code clarity for future maintainers.Also applies to: 118-124
126-147: LGTM: Refactored server creation logic looks good.The new server creation logic using
ServerWithHttpRedirectaligns well with the PR objectives. The conditional configuration based on the presence of an HTTPS port improves flexibility, and retaining the initial certificate wait ensures proper HTTPS setup.Consider adding error handling for the
ServerWithHttpRedirect::new()calls. While the?operator propagates errors, explicit handling could provide more informative error messages or recovery options.
153-155: LGTM: Simplified shutdown logic with graceful shutdown.The simplified shutdown logic using
graceful_shutdown_with_timeoutis a good improvement, aligning with the refactoring objectives and ensuring clean termination.Consider making the shutdown timeout configurable, either through a constant or as part of the
ProxyConfig. This would allow for easier adjustments based on deployment requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- plane/src/proxy/mod.rs (3 hunks)
🔇 Additional comments (4)
plane/src/proxy/mod.rs (4)
6-9: LGTM: Import and module changes align with refactoring objectives.The new imports from
dynamic_proxy::serverand the changes to module visibility are consistent with the PR's goal of refactoring the proxy and separating proxy logic from business logic.Also applies to: 12-13, 19-19, 22-22
42-43: Great job addressing the documentation issue!The added comments for
http_portandhttps_portimprove the struct's documentation. The explanation forhttp_portnow clearly describes its behavior whenhttps_portis provided, addressing the previous review comment.Also applies to: 46-47
Line range hint
1-155: LGTM: Removal of deprecated components.The removal of
ProxyMakeService,ShutdownSignal, andForwardableRequestInfoaligns with the PR's refactoring objectives. These components have been effectively replaced by the new server implementation usingServerWithHttpRedirect.
Line range hint
1-155: Great job on the proxy refactoring!The changes in this file align well with the PR objectives:
- The proxy logic has been substantially separated from the business logic of Plane.
- The new
dynamic_proxycrate is now used, allowing for decoupling of proxy dependency versions.- The refactoring sets the stage for future updates to
axumandreqwestcrates.The overall structure and maintainability of the code have been improved. Great work on this significant refactoring!
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- plane/src/proxy/proxy_server.rs (1 hunks)
🧰 Additional context used
📓 Learnings (1)
plane/src/proxy/proxy_server.rs (1)
Learnt from: michaelsilver PR: jamsocket/plane#814 File: plane/src/proxy/proxy_server.rs:154-158 Timestamp: 2024-09-30T22:50:58.691Z Learning: In the Plane project, using `std::sync::Mutex` in asynchronous code is acceptable when contention is low and the mutex is not held across `.await` calls.
🔇 Additional comments (3)
plane/src/proxy/proxy_server.rs (3)
25-38: Well-designed proxy state structuresThe
ProxyStateInnerandProxyStatestructs are well-designed for concurrent access. The use ofArcinProxyStateallows for safe sharing of the inner state across threads, which is crucial for a proxy server that may handle multiple connections simultaneously.
40-68: Appropriate implementation of ProxyStateThe
ProxyStateimplementation provides necessary functionality for creating and managing the proxy state. The use of atomic operations (AtomicBool) for managing the readiness state ensures thread-safe access, which is crucial in a concurrent environment.
250-261: Well-implemented status code to response conversionThe
status_code_to_responsefunction is concise and effectively creates a response with the given status code while applying general headers. It's a good utility function that promotes code reuse.
Will be rebased after #814 is merged, but otherwise ready for eyes.
This refactors the reverse proxy embedded in Plane.
The main goal of this is to update hyper support to the
1.xbranch. As side-goals, it:Rather than upgrading the entire
planeproject to the latest version of hyper, this PR introduces adynamic-proxycrate which contains the HTTP-level proxy implementation using the latest stable version of each of its dependencies. This allows us to decouple the dependency versions of the proxy from the rest of Plane.Once this is merged, the next step will be to update the
axumandreqwestscrates inplaneitself so that they use the latesthyper. Then, we can either keepdynamic-proxyas a separate crate and publish it (e.g. asplane-dynamic-proxy), or merge it back in to the mainplanecrate.Requirements matrix