added wrapper type and todos #348

AnkurRathore · 2024-11-04T13:29:02Z

@GlenDC I have attempted to add some wrapper types, and I have some doubts implementing them which I have put as TODO in the comments. I would need your guidance on this.

GlenDC

Let me know if my comments in this review are sufficient guidance or in case you need more detailed guidance. If the latter try to be more specific on where exactly you want my guidance with.

GlenDC · 2024-11-04T13:41:27Z

rama-tcp/src/client/connect.rs

+    DualPreferIpV4
+}
+
+impl Default for IPModes {


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

rama-tcp/src/client/connect.rs

GlenDC · 2024-11-04T13:43:35Z

rama-tcp/src/client/connect.rs

@@ -23,6 +23,49 @@ use tokio::{
    },
 };

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


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.

Do I need to put these in rama-net?

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.

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)

GlenDC · 2024-11-04T13:47:51Z

rama-tcp/src/client/connect.rs

+
+/*
+TODO - Using the wrapper types
+in the tcp_connect fn, update the dns type to use the DnsResolveIpMode


I think it's in async fn tcp_connect_inner<State, Dns, Connector>( that you'll want to check for DnsResolveIpMode.

And it's in the location where we actually establish a tcp stream that you'll check for ConnectIpMode.

Originally I was thinking to also automatically let one influence the other, but I think it makes more sense to just let the user worry about that. E.g. if user selects Ipv6-only for DNS and ipv4-only for Connect, then that obviously will fail, but that is up to the user to then fix. No point in trying to do anything about that, as the only reasonable thing to do would be to return some kind of error, so let's not bother with that as the current error will already be fine enough.

GlenDC · 2024-12-10T19:01:46Z

Hi @AnkurRathore , not that it's a blocker or urgent. But given this is already a month stale. I do want to at least check in.

Are you stuck?
Need guidance, mentoring, or help of any kind?
Or did you move one and want to hand this over to someone else?

EIther and everything is fine. And hope you're in good health and condition. Take care!

AnkurRathore · 2024-12-18T01:35:53Z

Hi @GlenDC ,
Apologies for the delay and thank you for checking in. I truly appreciate your patience.
There are two main reasons for the delay:

I’ve been dealing with some personal matters, which took up most of my time. I’m now getting back on track and regaining focus.
When I started reviewing the Rust codebase, I felt a bit overwhelmed and struggled with some of the language intricacies. To address this, I’ve been dedicating time to deepening my understanding of Rust.

I would still love to work on this, but I want to ensure I’m not impacting the release schedule. If you need this prioritized or handed over, I completely understand.
Thank you again for your understanding and support. I hope this clarifies my situation. Please let me know how you’d like me to proceed.

GlenDC · 2024-12-18T13:05:11Z

Understood. Do take care. For now I'll leave it with you then. It's not blocking any release schedule, as I still have plenty of other and much bigger fish to fry.

Should it still be left open in some weeks, I might re-assign it in case nothing changed by than. Either way good though, this is all voluntary. If you do feel ready by than to continue work on it, also good, I'll gladly support you with that. But otherwise also ok. Take care either way!

AnkurRathore · 2024-12-26T10:29:18Z

@GlenDC please review the latest changes. Let me know if I am on the correct path.

GlenDC · 2024-12-26T14:31:22Z

rama-tcp/src/client/connect.rs

-        ipv4_sem,
-    ));
+    match dns.mode {
+        DnsResolveIpMode::SingleIpV4 | DnsResolveIpMode::DualPreferIpV4 | DnsResovleIpMode::Dual =>{


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.

GlenDC · 2024-12-26T14:36:21Z

rama-tcp/src/client/connect.rs

    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 ) =>{


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

GlenDC · 2024-12-26T14:40:28Z

@GlenDC please review the latest changes. Let me know if I am on the correct path.

Hi @AnkurRathore great to see you back in action!!! Hope you're doing alright.

Yes, direction-wise this looks good. The DnsMode btw not only is about which IP version is supported, but also which one is prefered. Dual mode is the current only mode, where Ipv6 is preferred, but in case the Ipv4Preferred option is chosen you need to make sure that it is ipv4 which is prefered instead of the now only way ipv6, by swapping the delays.

Other than that, this is shaping up nicely, thx a lot!

AnkurRathore · 2024-12-26T15:02:32Z

@GlenDC I am doing good. Hope the same from your end.
I will go through the review comments.
Thanks.

AnkurRathore · 2024-12-27T09:34:24Z

@GlenDC I have made the changes as per the comments. Please review them.

GlenDC · 2024-12-27T12:29:08Z

rama-net/src/mode.rs

@@ -0,0 +1,43 @@
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)]
+/// Enum representing the IP modes that can be used by the DNS resolver.
+pub enum IPModes {


There are 2 enums required:

enum DnsResolveIpMode dual (default) singleIpV4 singleIpV6 dualPreferIpv4 enum ConnectIpMode dual (default) ipv4 ipv6

The struct DnsResolveIpMode can be removed as there should be no need to wrap the resolver, this would be bad for ergonomics and doesn't really solve anything.

The difference between the 2 enums is that:

ConnectIpMode is used only for establishing a connection

DnsResolveIpMode is used only for resolving a domain

these two are separate and do not influence each other.

if they are not present in the Context<S> of the incoming request we would resort to the default

DnsResolveIpMode has one more variant as there we also need to choose which one we are biased against, Ipv4 or Ipv6, where the latter is the default to help progress the adoption of ipv6.

Sorry for not catching this earlier. Seens I missed it in my initial scans of your code.

Do let me know here or on Discord if something is not clear in that set of requirements.

GlenDC · 2024-12-27T12:30:09Z

rama-tcp/src/lib.rs

@@ -19,4 +19,4 @@

 pub mod client;
 pub mod server;
-pub mod utils;
+pub mod utils;


You removed a whiteline here

GlenDC · 2024-12-27T12:31:09Z

@GlenDC I have made the changes as per the comments. Please review them.

Added the requested review, some comments that already existed are still unresolved as well. Do let me know if you have more questions, need more guidance or anything else. Here, in the original github ticket (#331) or on Discord.

AnkurRathore · 2024-12-28T05:42:34Z

@GlenDC addressed the required changes in enum. Please review

GlenDC · 2024-12-28T16:16:14Z

@GlenDC addressed the required changes in enum. Please review

Those changes look excellent. Do you need guidance for the remaining TODOs:

respect ip version preference when resolving dns
respect ipConnectMode when establishing a transport connection

AnkurRathore · 2024-12-30T07:16:54Z

@GlenDC addressed the required changes in enum. Please review

Those changes look excellent. Do you need guidance for the remaining TODOs:

respect ip version preference when resolving dns

respect ipConnectMode when establishing a transport connection

@GlenDC need your guidance on the remaining TODOs. Can you elaborate more on this.

GlenDC · 2024-12-30T09:30:55Z

@GlenDC need your guidance on the remaining TODOs. Can you elaborate more on this.

Sure thing

respect ip version preference when resolving dns

These sleep intervals would need to be swapped in case Ipv4 is prefered over ipv6

rama/rama-tcp/src/client/connect.rs

Lines 279 to 280 in 1e2acae

    
           IpKind::Ipv4 => Duration::from_micros((21 * 2 * index) as u64), 
        
           IpKind::Ipv6 => Duration::from_micros((15 * 2 * index) as u64),

respect ipConnectMode when establishing a transport connection

In these locations you would need to block connections with an error in case of an Ipv4 address but ipv4 not allowed, and same for ipv6:

rama/rama-tcp/src/client/connect.rs

Line 138 in 1e2acae

let stream = connector
rama/rama-tcp/src/client/connect.rs

Line 272 in 1e2acae

let sem = sem.clone();

added wrapper type and todos

2c05499

GlenDC requested changes Nov 4, 2024

View reviewed changes

modified code to support ipv4 or ipv6 connection

ea1b51f

GlenDC reviewed Dec 26, 2024

View reviewed changes

added function for iplookup and made enums in separate file

9e6e101

GlenDC reviewed Dec 27, 2024

View reviewed changes

added the required enum and made the required changes

f802b0f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added wrapper type and todos #348

added wrapper type and todos #348

AnkurRathore commented Nov 4, 2024

GlenDC left a comment

GlenDC Nov 4, 2024

GlenDC Nov 4, 2024

AnkurRathore Nov 6, 2024

GlenDC Nov 6, 2024

GlenDC Dec 26, 2024

GlenDC Nov 4, 2024

GlenDC commented Dec 10, 2024

AnkurRathore commented Dec 18, 2024

GlenDC commented Dec 18, 2024

AnkurRathore commented Dec 26, 2024

GlenDC Dec 26, 2024

GlenDC Dec 26, 2024 •

edited

Loading

GlenDC commented Dec 26, 2024

AnkurRathore commented Dec 26, 2024

AnkurRathore commented Dec 27, 2024

GlenDC Dec 27, 2024

GlenDC Dec 27, 2024

GlenDC commented Dec 27, 2024

AnkurRathore commented Dec 28, 2024

GlenDC commented Dec 28, 2024

AnkurRathore commented Dec 30, 2024

GlenDC commented Dec 30, 2024

added wrapper type and todos #348

Are you sure you want to change the base?

added wrapper type and todos #348

Conversation

AnkurRathore commented Nov 4, 2024

GlenDC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GlenDC commented Dec 10, 2024

AnkurRathore commented Dec 18, 2024

GlenDC commented Dec 18, 2024

AnkurRathore commented Dec 26, 2024

Choose a reason for hiding this comment

GlenDC Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

GlenDC commented Dec 26, 2024

AnkurRathore commented Dec 26, 2024

AnkurRathore commented Dec 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GlenDC commented Dec 27, 2024

AnkurRathore commented Dec 28, 2024

GlenDC commented Dec 28, 2024

AnkurRathore commented Dec 30, 2024

GlenDC commented Dec 30, 2024

GlenDC Dec 26, 2024 •

edited

Loading