feat(sdk): fix client tls connections #2223
Conversation
WalkthroughThe pull request introduces changes to the Changes
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
packages/rs-sdk/src/platform/types/evonode.rs (1)
77-86: Improved error handling for client creation.The changes enhance the robustness of the
execute_transportmethod by explicitly handling potential errors during client creation. This is a good improvement that ensures errors are properly propagated.However, consider using the
?operator for more concise error handling:let mut client = Self::Client::with_uri_and_settings(uri.clone(), settings, &pool) .map_err(|e| async move { Err(e) }.boxed())?;This achieves the same result with slightly more idiomatic Rust code.
packages/rs-dapi-client/src/dapi_client.rs (1)
202-208: Improved error handling for transport client creation.The changes enhance error reporting by wrapping the transport error in a
DapiClientError::Transportvariant, which includes the address that caused the error. This improvement aligns well with the PR objective of fixing client TLS connections by providing more context for connection-related errors.For consistency with the error handling in the
execute_transportcall, consider using a closure for themap_erroperation:.map_err(|e| { DapiClientError::<<R::Client as TransportClient>::Error>::Transport( e, address.clone(), ) })?;This minor change would make the error handling style uniform throughout the method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- packages/rs-dapi-client/Cargo.toml (1 hunks)
- packages/rs-dapi-client/src/connection_pool.rs (1 hunks)
- packages/rs-dapi-client/src/dapi_client.rs (1 hunks)
- packages/rs-dapi-client/src/transport.rs (1 hunks)
- packages/rs-dapi-client/src/transport/grpc.rs (2 hunks)
- packages/rs-sdk/src/platform/types/evonode.rs (2 hunks)
🧰 Additional context used
🔇 Additional comments (12)
packages/rs-dapi-client/Cargo.toml (1)
23-23: LGTM! Verify build success with updated dependency.The change to the
dapi-grpcdependency looks good. It explicitly enables the "client" feature while disabling default features, which should help reduce the dependency footprint and ensure only necessary functionality is included.Potential impacts:
- Reduced compilation time and binary size.
- Only client-related functionality from
dapi-grpcwill be available.Please ensure that the build still succeeds with this change, as disabling default features could potentially break the build if any were implicitly relied upon. Run the following command to verify:
packages/rs-dapi-client/src/transport.rs (2)
58-61: Consistent error handling improvement inwith_uri_and_settingsmethod.The change to return
Result<Self, Self::Error>instead ofSelfaligns with the modification made to thewith_urimethod. This consistency in error handling across related methods is commendable.Please ensure that all implementations of the
TransportClienttrait have been updated to match this new signature. Run the following script to verify:#!/bin/bash # Description: Check for implementations of TransportClient::with_uri_and_settings # Test: Search for implementations of with_uri_and_settings. Expect: All implementations should return Result<Self, Self::Error> rg --type rust -e 'fn\s+with_uri_and_settings.*->.*Result<Self,\s*Self::Error>' -e 'impl.*TransportClient.*for'
54-54: Improved error handling inwith_urimethod.The change from returning
SelftoResult<Self, Self::Error>enhances error handling capabilities during client creation. This is a good practice in Rust and allows for more robust client initialization.Please ensure that all implementations of the
TransportClienttrait have been updated to match this new signature. Run the following script to verify:✅ Verification successful
Verified
TransportClient::with_urimethod signatures.All implementations of the
TransportClienttrait (PlatformGrpcClientandCoreGrpcClient) have been updated to returnResult<Self, Self::Error>.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for implementations of TransportClient::with_uri # Test: Search for implementations of with_uri. Expect: All implementations should return Result<Self, Self::Error> rg --type rust -e 'fn\s+with_uri.*->.*Result<Self,\s*Self::Error>' -e 'impl.*TransportClient.*for'Length of output: 293
Script:
#!/bin/bash # Description: Extract TransportClient::with_uri method signatures from grpc.rs # Extract 'with_uri' function signatures from grpc.rs rg --type rust 'impl\s+TransportClient\s+for\s+\w+\s+\{[^}]*fn\s+with_uri[^}]*\}' packages/rs-dapi-client/src/transport/grpc.rsLength of output: 129
Script:
#!/bin/bash # Description: Extract TransportClient::with_uri method signatures and their return types from grpc.rs # Find all 'fn with_uri' definitions and display their return types rg --type rust 'fn\s+with_uri\s*\([^)]*\)\s*->\s*Result<Self,\s*Self::Error>' packages/rs-dapi-client/src/transport/grpc.rsLength of output: 285
packages/rs-sdk/src/platform/types/evonode.rs (2)
13-13: LGTM: New import for error handling.The addition of
DapiClientErrorto the import list is appropriate for the enhanced error handling in theexecute_transportmethod.
Line range hint
1-118: Summary: Improved error handling in EvoNode implementation.The changes in this file focus on enhancing error handling in the
execute_transportmethod of theEvoNodestruct. The modifications improve the robustness of the code by explicitly handling potential errors during client creation. These changes align well with the PR objective of fixing client TLS connections.Key points:
- No changes were made to public interfaces or exported entities, maintaining backwards compatibility.
- The overall structure and functionality of the
EvoNodestruct remain intact.- The error handling improvements make the code more resilient to potential failures.
These changes contribute positively to the stability and reliability of the SDK.
packages/rs-dapi-client/src/connection_pool.rs (2)
70-76: Enhanced error handling inget_or_createmethodThe updated method signature of
get_or_createnow returns aResult<PoolItem, T>, allowing for proper error propagation when creating a new pool item. This change improves the robustness and reliability of the connection pool.
82-84:⚠️ Potential issueIncorrect arguments in
self.putmethod callIn the call to
self.puton line 83, theprefixargument is incorrectly included. Theputmethod expects parametersuri,settings, andvalue(thePoolItem), but notprefix. Includingprefixhere will cause a compilation error due to mismatched parameters.Apply the following diff to fix the error:
- self.put(prefix, uri, settings, cli.clone()); + self.put(uri, settings, cli.clone());Likely invalid or redundant comment.
packages/rs-dapi-client/src/transport/grpc.rs (5)
21-37: Enhanced error propagation increate_channelfunctionThe
create_channelfunction now returns aResult<Channel, Error>, allowing for proper error propagation during channel creation. This change enhances error handling and improves the robustness of the client initialization process.
43-54: Improved error handling inwith_urimethod ofPlatformGrpcClientThe
with_urimethod has been updated to returnResult<Self, Self::Error>, correctly handling potential errors during channel creation. This ensures that any issues encountered while establishing the channel are properly propagated and can be managed by the caller.
61-75: Improved error handling inwith_uri_and_settingsmethod ofPlatformGrpcClientThe method now returns
Result<Self, Self::Error>and handles errors appropriately when creating the channel with specific settings. This modification enhances robustness by ensuring that any failures during channel creation are accurately reported.
82-93: Improved error handling inwith_urimethod ofCoreGrpcClientThe
with_urimethod now returnsResult<Self, Self::Error>, properly handling potential errors during channel creation for the Core client. This change ensures that the initialization process is robust and errors are effectively communicated.
100-114: Improved error handling inwith_uri_and_settingsmethod ofCoreGrpcClientThe method has been updated to return
Result<Self, Self::Error>and now handles errors appropriately when creating the channel with applied settings. This enhances the client's resilience by allowing error propagation.
| fn with_uri(uri: Uri, pool: &ConnectionPool) -> Result<Self, Self::Error>; | ||
|
|
||
| /// Build client using node's url and [AppliedRequestSettings]. | ||
| fn with_uri_and_settings( | ||
| uri: Uri, | ||
| settings: &AppliedRequestSettings, | ||
| pool: &ConnectionPool, | ||
| ) -> Self; | ||
| ) -> Result<Self, Self::Error>; |
There was a problem hiding this comment.
💡 Codebase verification
Unhandled Result types detected in TransportClient method calls.
The recent changes to with_uri and with_uri_and_settings methods introduce Result<Self, Self::Error> returns, enhancing error handling capabilities. However, several usages of these methods do not handle the Result, which could lead to unhandled errors and potential application instability.
Please address the following instances to ensure proper error handling:
packages/rs-sdk/src/platform/types/evonode.rs: let client_result = Self::Client::with_uri_and_settings(uri.clone(), settings, &pool);packages/rs-dapi-client/src/dapi_client.rs: let mut transport_client = R::Client::with_uri_and_settings(packages/rs-dapi-client/src/transport/grpc.rs: fn with_uri(uri: Uri, pool: &ConnectionPool) -> Result<Self, Self::Error> {packages/rs-dapi-client/src/transport/grpc.rs: fn with_uri_and_settings(packages/rs-dapi-client/src/transport/grpc.rs: fn with_uri(uri: Uri, pool: &ConnectionPool) -> Result<Self, Self::Error> {packages/rs-dapi-client/src/transport/grpc.rs: fn with_uri_and_settings(
Ensure that each method call appropriately handles the Result using the ? operator, match statements, or other suitable error handling mechanisms.
🔗 Analysis chain
Potential breaking changes and testing recommendation.
The modifications to both with_uri and with_uri_and_settings methods improve error handling, which aligns with the PR objective of fixing client TLS connections. However, these changes may be breaking if not all implementations and usages have been updated accordingly.
- Ensure that all implementations of
TransportClienthave been updated to match the new signatures. - Verify that all code using these methods now handles the
Resultreturn type correctly. - Update and run all relevant tests to ensure the changes don't introduce regressions.
Run the following script to find potential usage of these methods that might need updating:
Please confirm that comprehensive testing has been performed to validate these changes across the entire codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find potential usages of TransportClient methods that might need updating
# Test: Search for usages of with_uri and with_uri_and_settings.
# Expect: All usages should handle the Result return type
rg --type rust -e 'with_uri\s*\(' -e 'with_uri_and_settings\s*\('
Length of output: 923
Script:
#!/bin/bash
# Description: Identify TransportClient method calls that do not handle the Result properly
# Test: Search for usages of with_uri and with_uri_and_settings without proper error handling
# Expect: All calls should handle the Result using '?' operator, 'match', or other error handling constructs
# Search for 'with_uri(' or 'with_uri_and_settings(' not followed by '?' or within a 'match' statement
rg --type rust -e 'with_uri\s*\(' -e 'with_uri_and_settings\s*\(' -A 2 | grep -B 2 -vE '\?|\bmatch\b'
Length of output: 2254
| .with_webpki_roots(), | ||
| )?; | ||
|
|
||
| if let Some(settings) = settings { | ||
| if let Some(timeout) = settings.connect_timeout { |
There was a problem hiding this comment.
Redundant TLS root certificates configuration
In the create_channel function, both with_native_roots() and with_webpki_roots() are called on the TLS configuration. These methods set the root certificate store, and calling both may lead to unintended behavior as they might overwrite each other. To ensure the TLS configuration is set correctly, consider using only one of these methods based on the desired root certificates source.
Apply this diff to fix the redundancy:
ClientTlsConfig::new()
- .with_native_roots()
.with_webpki_roots(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let mut builder = Channel::builder(uri).tls_config( | |
| ClientTlsConfig::new() | |
| .with_native_roots() | |
| .with_webpki_roots(), | |
| )?; | |
| let mut builder = Channel::builder(uri).tls_config( | |
| ClientTlsConfig::new() | |
| .with_webpki_roots(), | |
| )?; |
| e | ||
| ))), | ||
| } | ||
| })? | ||
| .into()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review the use of gRPC status code for error handling
When handling errors from create_channel, the code wraps the error using Status::failed_precondition. The FAILED_PRECONDITION status code indicates that the system is in an invalid state for the operation to execute. Since channel creation failure might be due to connectivity issues or misconfigurations, consider whether Status::unavailable or Status::internal might more accurately represent the error type.
Apply this diff if you decide to change the status code:
Err(e) => Err(dapi_grpc::tonic::Status::failed_precondition(format!(
- "Channel creation failed: {}",
+ "Channel is unavailable: {}",
e
))),Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/rs-dapi-client/src/transport/grpc.rs (4)
25-30: LGTM: Improved TLS configuration and channel creationThe changes enhance the security of the gRPC connections by adding TLS configuration. However, there's a minor redundancy in the TLS configuration that should be addressed.
Consider removing the redundant
with_native_roots()call as mentioned in a previous review:let mut builder = Channel::builder(uri).tls_config( ClientTlsConfig::new() - .with_native_roots() .with_webpki_roots() .assume_http2(true), )?;This change will ensure that only the WebPKI roots are used, avoiding any potential conflicts or unintended behavior.
Also applies to: 38-38
44-55: LGTM: Improved error handling inwith_urimethodThe changes enhance error handling in the
with_urimethod, properly propagating errors from channel creation. This improvement allows for better error management in the calling code.Consider using a more specific error status for channel creation failures. The current
failed_preconditionmight not accurately represent all possible error scenarios. A more appropriate status might beunavailablefor network-related issues orinternalfor unexpected errors:- Err(e) => Err(dapi_grpc::tonic::Status::failed_precondition(format!( + Err(e) => Err(dapi_grpc::tonic::Status::unavailable(format!( "Channel creation failed: {}", e ))),This change would provide more accurate error information to the caller.
62-76: LGTM: Consistent error handling inwith_uri_and_settingsmethodThe changes in the
with_uri_and_settingsmethod are consistent with those in thewith_urimethod, providing improved error handling and propagation.As suggested for the
with_urimethod, consider using a more specific error status for channel creation failures:- Err(e) => Err(dapi_grpc::tonic::Status::failed_precondition(format!( + Err(e) => Err(dapi_grpc::tonic::Status::unavailable(format!( "Channel creation failed: {}", e ))),This change would provide more accurate error information and maintain consistency with the
with_urimethod.
83-115: LGTM: Consistent updates toCoreGrpcClientimplementationThe changes to the
CoreGrpcClientimplementation are consistent with those made toPlatformGrpcClient, ensuring uniform error handling and connection management across both client types.For consistency with the suggested changes in
PlatformGrpcClient, consider updating the error status in bothwith_uriandwith_uri_and_settingsmethods:- Err(e) => Err(dapi_grpc::tonic::Status::failed_precondition(format!( + Err(e) => Err(dapi_grpc::tonic::Status::unavailable(format!( "Channel creation failed: {}", e ))),This change would provide more accurate error information and maintain consistency across all client implementations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/rs-dapi-client/src/transport/grpc.rs (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
packages/rs-dapi-client/src/transport/grpc.rs (2)
11-11: LGTM: Improved error handling increate_channelfunctionThe changes to the import statement and the
create_channelfunction signature enhance error handling. Returning aResulttype allows for better error propagation and handling in the calling code.Also applies to: 21-24
Line range hint
1-115: Overall assessment: Improved error handling and TLS configurationThe changes in this file significantly enhance the robustness of the gRPC transport layer by improving error handling and TLS configuration. The modifications are consistent across both
PlatformGrpcClientandCoreGrpcClient, ensuring uniform behavior.Key improvements:
- Better error propagation in the
create_channelfunction and client methods.- Enhanced TLS configuration for secure connections.
- Consistent use of the connection pool for efficient channel management.
Minor suggestions have been made to further refine error handling and remove redundant TLS configuration. Overall, these changes represent a solid improvement to the codebase.
Issue being fixed or feature implemented
Tls connections for clients were no longer working, this now fixes things.
What was done?
Added tls explicit configuration.
How Has This Been Tested?
Verified it was working
Breaking Changes
Fixes issues.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Summary by CodeRabbit
New Features
PlatformGrpcClient.Bug Fixes
Refactor
get_or_createmethod to support error handling during item creation.Chores
dapi-grpc, now including specific features.