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

added wrapper type and todos #348

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

added wrapper type and todos #348

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2c05499
added wrapper type and todos
AnkurRathore Nov 4, 2024
ea1b51f
modified code to support ipv4 or ipv6 connection
AnkurRathore Dec 26, 2024
9e6e101
added function for iplookup and made enums in separate file
AnkurRathore Dec 27, 2024
f802b0f
added the required enum and made the required changes
AnkurRathore Dec 28, 2024
edd1b3a
added functionality to respect ip version preferences
AnkurRathore Jan 15, 2025
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
124 changes: 87 additions & 37 deletions rama-tcp/src/client/connect.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,46 @@ use tokio::{
},
};

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
enum IPModes {
Copy link
Member

Choose a reason for hiding this comment

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

Originally I wanted to go for a single IP Mode indeed, and work with wrapper types. However I think it makes more sense to define 2 enums:

enum DnsResolveIpMode
    dual (default)
    singleIpV4
    singleIpV6
    dualPreferIpv4

enum ConnectIpMode
    dual (default)
    ipv4
    ipv6

Don't think these belong in rama-tcp though. Probably more like in rama-net or so.

Copy link
Author

Choose a reason for hiding this comment

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

Do I need to put these in rama-net?

Copy link
Member

Choose a reason for hiding this comment

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

Yes as these are not specific to tcp. E.g. in a future version we'll also support udp. And other packages such as Dns might also make use of it for w/e. The common denominator is that it's all related to network protocols, as such rama-net.

You can put it in a file created as rama-net/src/mode.rs and just pub mod mode in the lib.rs file of that crate.

Copy link
Member

Choose a reason for hiding this comment

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

This is still to move to rama-net and ConnectIpMode is still to be used as well in the tcp connection part itself (unrelated from DnsResolveIpMode)

Dual,
SingleIpV4,
SingleIpV6,
DualPreferIpV4
}

impl Default for IPModes {
Copy link
Member

Choose a reason for hiding this comment

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

You can easier define this by using Default in the #[derive(..)] above and set #[default] above Dual,.

fn default() -> Self {
Self::Dual
}
}

//DNS Resolver
GlenDC marked this conversation as resolved.
Show resolved Hide resolved
#[derive(Clone)]
struct DnsResolveIpMode<D>{
resolver: D,
mode: IPModes
}

impl<D> DnsResolveIpMode<D>{
fn new(resolver:D, mode: IPModes) -> Self {
Self { resolver, mode}
}
}

struct ConnectIpMode<C>{
connector: C,
ip_mode: IPModes
}

impl<C>ConnectIpMode<C>{
fn new(connector: C, ip_mode: IPModes) -> Self {
Self {connector, ip_mode}
}
}



/// Trait used internally by [`tcp_connect`] and the `TcpConnector`
/// to actually establish the [`TcpStream`.]
pub trait TcpStreamConnector: Send + Sync + 'static {
Expand Down Expand Up @@ -154,8 +194,8 @@ async fn tcp_connect_inner<State, Dns, Connector>(
ctx: &Context<State>,
domain: Domain,
port: u16,
dns: Dns,
connector: Connector,
dns: DnsResolveIpMode<Dns>,
connector: ConnectIpMode<Connector>,
) -> Result<(TcpStream, SocketAddr), OpaqueError>
where
State: Clone + Send + Sync + 'static,
Expand All @@ -167,37 +207,41 @@ where
let connected = Arc::new(AtomicBool::new(false));
let sem = Arc::new(Semaphore::new(3));

// IPv6
let ipv6_tx = tx.clone();
let ipv6_domain = domain.clone();
let ipv6_connected = connected.clone();
let ipv6_sem = sem.clone();
ctx.spawn(tcp_connect_inner_branch(
dns.clone(),
connector.clone(),
IpKind::Ipv6,
ipv6_domain,
port,
ipv6_tx,
ipv6_connected,
ipv6_sem,
));

// IPv4
let ipv4_tx = tx;
let ipv4_domain = domain.clone();
let ipv4_connected = connected.clone();
let ipv4_sem = sem;
ctx.spawn(tcp_connect_inner_branch(
dns,
connector,
IpKind::Ipv4,
ipv4_domain,
port,
ipv4_tx,
ipv4_connected,
ipv4_sem,
));
match dns.mode {
DnsResolveIpMode::SingleIpV4 | DnsResolveIpMode::DualPreferIpV4 | DnsResovleIpMode::Dual =>{
Copy link
Member

Choose a reason for hiding this comment

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

You probably want to provide utility methods to DnsResolveIpMode, such as:

fn ipv4_supported(&self) -> bool;
fn ipv6_supported(&self) -> bool;

Is same-same, but easier to have to update such statements only in a single loc.

//IPV4
ctx.spawn(tcp_connect_inner_branch(
dns.clone(),
Copy link
Member

Choose a reason for hiding this comment

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

You can add Copy derive trait to these mode Enums, then you don't need to Clone

connector.clone(),
IpKind::Ipv4,
domain.clone(),
port,
tx.clone(),
connected.clone(),
sem.clone(),
));
}
_ => {}

}

match dns.mode {
DnsResolveIpMode::SingleIpV6 | DnsResovleIpMode::Dual =>{
//IPV6
ctx.spawn(tcp_connect_inner_branch(
dns.clone(),
connector.clone(),
IpKind::Ipv6,
domain.clone(),
port,
tx.clone(),
connected.clone(),
sem.clone(),
));
}
_ => {}

}

// wait for the first connection to succeed,
// ignore the rest of the connections (sorry, but not sorry)
Expand Down Expand Up @@ -231,23 +275,29 @@ async fn tcp_connect_inner_branch<Dns, Connector>(
Dns: DnsResolver<Error: Into<BoxError>> + Clone,
Connector: TcpStreamConnector<Error: Into<BoxError> + Send + 'static> + Clone,
{

let ip_it = match ip_kind {
IpKind::Ipv4 => match dns.ipv4_lookup(domain).await {
IpKind::Ipv4 if matches!(dns.mode, DnsResolveIpMode::Dual |DnsResolveIpMode::SingleIpV4 |DnsResolveIpMode::DualPreferIpV4 ) =>{
Copy link
Member

@GlenDC GlenDC Dec 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I would suggest to do:

match ip_kind => {
     IpKind::Ipv4 => if dns.mode.ipv4_supported() {
           // ... logic
     } else {
          trace::debug ipv4 not supported by DnsMode: no connection can be established
     }

     IpKind::Ipv6 => if dns.mode.ipv6_supported() {
           // ... logic
     } else {
          trace::debug ipv6 not supported by DnsMode: no connection can be established
     }

This way we have a clear trace in our logs / spans

match dns.ipv4_lookup(domain).await {
Ok(ips) => Either::A(ips.into_iter().map(IpAddr::V4)),
Err(err) => {
let err = OpaqueError::from_boxed(err.into());
tracing::trace!(err = %err, "[{ip_kind:?}] failed to resolve domain to IPv4 addresses");
return;
}
},
IpKind::Ipv6 => match dns.ipv6_lookup(domain).await {
}
},
IpKind::Ipv6 if matches!(dns.mode, DnsResolveIpMode::Dual |DnsResolveIpMode::SingleIpV6)=> {
match dns.ipv6_lookup(domain).await {
Ok(ips) => Either::B(ips.into_iter().map(IpAddr::V6)),
Err(err) => {
let err = OpaqueError::from_boxed(err.into());
tracing::trace!(err = ?err, "[{ip_kind:?}] failed to resolve domain to IPv6 addresses");
return;
}
},
}
},
_ => return,
};

for (index, ip) in ip_it.enumerate() {
Expand Down