Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(sdk): fix client tls connections #2223

Merged
merged 6 commits into from
Oct 8, 2024
Merged

feat(sdk): fix client tls connections #2223

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ab91e07
fix: fixed pools to add tls
QuantumExplorer Oct 8, 2024
2f60720
added root certificates
QuantumExplorer Oct 8, 2024
01282fb
assume http2
QuantumExplorer Oct 8, 2024
df80c15
fix
QuantumExplorer Oct 8, 2024
b400b0e
changed T to E
QuantumExplorer Oct 8, 2024
20f84ce
fix: old backon breaks tests
lklimek Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/rs-dapi-client/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ offline-testing = []

[dependencies]
backon = "0.5"
dapi-grpc = { path = "../dapi-grpc" }
dapi-grpc = { path = "../dapi-grpc", features = ["client"], default-features = false }
futures = "0.3.28"
http-serde = { version = "2.1", optional = true }
rand = { version = "0.8.5", features = ["small_rng"] }
Expand Down
12 changes: 7 additions & 5 deletions packages/rs-dapi-client/src/connection_pool.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,19 +67,21 @@ impl ConnectionPool {
/// * `prefix` - Prefix for the item in the pool. Used to distinguish between Core and Platform clients.
/// * `uri` - URI of the node.
/// * `settings` - Applied request settings.
pub fn get_or_create(
pub fn get_or_create<T>(
lklimek marked this conversation as resolved.
Show resolved Hide resolved
&self,
prefix: PoolPrefix,
uri: &Uri,
settings: Option<&AppliedRequestSettings>,
create: impl FnOnce() -> PoolItem,
) -> PoolItem {
create: impl FnOnce() -> Result<PoolItem, T>,
) -> Result<PoolItem, T> {
if let Some(cli) = self.get(prefix, uri, settings) {
return cli;
return Ok(cli);
}

let cli = create();
self.put(uri, settings, cli.clone());
if let Ok(cli) = &cli {
self.put(uri, settings, cli.clone());
}
cli
}

Expand Down
8 changes: 7 additions & 1 deletion packages/rs-dapi-client/src/dapi_client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,13 @@ impl DapiRequestExecutor for DapiClient {
address.uri().clone(),
&applied_settings,
&pool,
);
)
.map_err(|e| {
DapiClientError::<<R::Client as TransportClient>::Error>::Transport(
e,
address.clone(),
)
})?;

let response = transport_request
.execute_transport(&mut transport_client, &applied_settings)
Expand Down
4 changes: 2 additions & 2 deletions packages/rs-dapi-client/src/transport.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,12 @@ pub trait TransportClient: Send + Sized {
type Error: CanRetry + Send + Debug + Mockable;

/// Build client using node's url.
fn with_uri(uri: Uri, pool: &ConnectionPool) -> Self;
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>;
Comment on lines +54 to +61
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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.

  1. Ensure that all implementations of TransportClient have been updated to match the new signatures.
  2. Verify that all code using these methods now handles the Result return type correctly.
  3. 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

}
89 changes: 65 additions & 24 deletions packages/rs-dapi-client/src/transport/grpc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@
use super::{CanRetry, TransportClient, TransportRequest};
use crate::connection_pool::{ConnectionPool, PoolPrefix};
use crate::{request_settings::AppliedRequestSettings, RequestSettings};
use dapi_grpc::core::v0::core_client::CoreClient;

Check failure on line 8 in packages/rs-dapi-client/src/transport/grpc.rs

View workflow job for this annotation

GitHub Actions / Rust packages (rs-dapi-client) / Linting

failed to resolve: could not find `core` in `dapi_grpc` 

error[E0433]: failed to resolve: could not find `core` in `dapi_grpc`
 --> packages/rs-dapi-client/src/transport/grpc.rs:8:16
  |
8 | use dapi_grpc::core::v0::core_client::CoreClient;
  |                ^^^^ could not find `core` in `dapi_grpc`
  |
note: found an item that was configured out
 --> /home/ubuntu/actions-runner/_work/platform/platform/packages/dapi-grpc/src/lib.rs:4:9
  |
4 | pub mod core {
  |         ^^^^
  = note: the item is gated behind the `core` feature
use dapi_grpc::core::v0::{self as core_proto};

Check failure on line 9 in packages/rs-dapi-client/src/transport/grpc.rs

View workflow job for this annotation

GitHub Actions / Rust packages (rs-dapi-client) / Linting

unresolved import `dapi_grpc::core` 

error[E0432]: unresolved import `dapi_grpc::core`
 --> packages/rs-dapi-client/src/transport/grpc.rs:9:16
  |
9 | use dapi_grpc::core::v0::{self as core_proto};
  |                ^^^^ could not find `core` in `dapi_grpc`
  |
note: found an item that was configured out
 --> /home/ubuntu/actions-runner/_work/platform/platform/packages/dapi-grpc/src/lib.rs:4:9
  |
4 | pub mod core {
  |         ^^^^
  = note: the item is gated behind the `core` feature
use dapi_grpc::platform::v0::{self as platform_proto, platform_client::PlatformClient};
use dapi_grpc::tonic::transport::Uri;
use dapi_grpc::tonic::transport::{ClientTlsConfig, Uri};
use dapi_grpc::tonic::Streaming;
use dapi_grpc::tonic::{transport::Channel, IntoRequest};
use futures::{future::BoxFuture, FutureExt, TryFutureExt};
Expand All @@ -18,59 +18,100 @@
/// Core Client using gRPC transport.
pub type CoreGrpcClient = CoreClient<Channel>;

fn create_channel(uri: Uri, settings: Option<&AppliedRequestSettings>) -> Channel {
let mut builder = Channel::builder(uri);
fn create_channel(
uri: Uri,
settings: Option<&AppliedRequestSettings>,
) -> Result<Channel, dapi_grpc::tonic::transport::Error> {
let mut builder = Channel::builder(uri).tls_config(
ClientTlsConfig::new()
.with_native_roots()
.with_webpki_roots(),
)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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(),
)?;


if let Some(settings) = settings {
if let Some(timeout) = settings.connect_timeout {
builder = builder.connect_timeout(timeout);
}
}

builder.connect_lazy()
Ok(builder.connect_lazy())
}

impl TransportClient for PlatformGrpcClient {
type Error = dapi_grpc::tonic::Status;

fn with_uri(uri: Uri, pool: &ConnectionPool) -> Self {
pool.get_or_create(PoolPrefix::Platform, &uri, None, || {
Self::new(create_channel(uri.clone(), None)).into()
})
.into()
fn with_uri(uri: Uri, pool: &ConnectionPool) -> Result<Self, Self::Error> {
Ok(pool
.get_or_create(PoolPrefix::Platform, &uri, None, || {
match create_channel(uri.clone(), None) {
Ok(channel) => Ok(Self::new(channel).into()),
Err(e) => Err(dapi_grpc::tonic::Status::failed_precondition(format!(
"Channel creation failed: {}",
e
))),
}
Comment on lines +48 to +53
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

})?
.into())
}

fn with_uri_and_settings(
uri: Uri,
settings: &AppliedRequestSettings,
pool: &ConnectionPool,
) -> Self {
pool.get_or_create(PoolPrefix::Platform, &uri, Some(settings), || {
Self::new(create_channel(uri.clone(), Some(settings))).into()
})
.into()
) -> Result<Self, Self::Error> {
Ok(pool
.get_or_create(
PoolPrefix::Platform,
&uri,
Some(settings),
|| match create_channel(uri.clone(), Some(settings)) {
Ok(channel) => Ok(Self::new(channel).into()),
Err(e) => Err(dapi_grpc::tonic::Status::failed_precondition(format!(
"Channel creation failed: {}",
e
))),
},
)?
.into())
}
}

impl TransportClient for CoreGrpcClient {
type Error = dapi_grpc::tonic::Status;

fn with_uri(uri: Uri, pool: &ConnectionPool) -> Self {
pool.get_or_create(PoolPrefix::Core, &uri, None, || {
Self::new(create_channel(uri.clone(), None)).into()
})
.into()
fn with_uri(uri: Uri, pool: &ConnectionPool) -> Result<Self, Self::Error> {
Ok(pool
.get_or_create(PoolPrefix::Core, &uri, None, || {
match create_channel(uri.clone(), None) {
Ok(channel) => Ok(Self::new(channel).into()),
Err(e) => Err(dapi_grpc::tonic::Status::failed_precondition(format!(
"Channel creation failed: {}",
e
))),
}
})?
.into())
}

fn with_uri_and_settings(
uri: Uri,
settings: &AppliedRequestSettings,
pool: &ConnectionPool,
) -> Self {
pool.get_or_create(PoolPrefix::Core, &uri, Some(settings), || {
Self::new(create_channel(uri.clone(), Some(settings))).into()
})
.into()
) -> Result<Self, Self::Error> {
Ok(pool
.get_or_create(
PoolPrefix::Core,
&uri,
Some(settings),
|| match create_channel(uri.clone(), Some(settings)) {
Ok(channel) => Ok(Self::new(channel).into()),
Err(e) => Err(dapi_grpc::tonic::Status::failed_precondition(format!(
"Channel creation failed: {}",
e
))),
},
)?
.into())
}
}

Expand Down
13 changes: 11 additions & 2 deletions packages/rs-sdk/src/platform/types/evonode.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use futures::{FutureExt, TryFutureExt};
use rs_dapi_client::transport::{
AppliedRequestSettings, PlatformGrpcClient, TransportClient, TransportRequest,
};
use rs_dapi_client::{Address, ConnectionPool, RequestSettings};
use rs_dapi_client::{Address, ConnectionPool, DapiClientError, RequestSettings};
#[cfg(feature = "mocks")]
use serde::{Deserialize, Serialize};
use std::fmt::Debug;
Expand Down Expand Up @@ -74,7 +74,16 @@ impl TransportRequest for EvoNode {
// We also create a new client to use with this request, so that the user does not need to
// reconfigure SDK to use a single node.
let pool = ConnectionPool::new(1);
let mut client = Self::Client::with_uri_and_settings(uri.clone(), settings, &pool);
// We create a new client with the given URI and settings
let client_result = Self::Client::with_uri_and_settings(uri.clone(), settings, &pool);

// Handle the result manually to create a proper error response
let mut client = match client_result {
Ok(client) => client,
Err(e) => {
return async { Err(e) }.boxed();
}
};
let mut grpc_request = GetStatusRequest {
version: Some(get_status_request::Version::V0(GetStatusRequestV0 {})),
}
Expand Down
Loading