plabayo / rama

modular service framework to move and transform network packets
https://ramaproxy.org
Apache License 2.0
187 stars 21 forks source link

vec impl for DnsResolver #345

Open parkma99 opened 3 weeks ago

parkma99 commented 3 weeks ago

Hi @GlenDC I have a try for #332 But I am not sure my implement is totally that issue asked. And also, thanks for your helping

parkma99 commented 3 weeks ago

Thanks @GlenDC , I make some change in recent commits. I would like to add some tests, can you give me some help?

GlenDC commented 3 weeks ago

Thanks @GlenDC , I make some change in recent commits. I would like to add some tests, can you give me some help?

Sure. What tests would you like to add? I can chip in if they are useful and how you might go on doing so.

parkma99 commented 3 weeks ago

Sure. What tests would you like to add? I can chip in if they are useful and how you might go on doing so.

Some demos for using DnsChainDomainResolve, just like in #332 mentioned

GlenDC commented 3 weeks ago

Sure. What tests would you like to add? I can chip in if they are useful and how you might go on doing so.

Some demos for using DnsChainDomainResolve, just like in #332 mentioned

Sure. For general advise you can check existing tests in rama. E.g. how to do multiple cases of same tests (test cases), or fact that you'll need to use tokio::test for async tests, etc...

Regarding these tests. Wouldn't overdo it. Again use your own judgement, but following cases come to my mind:

  1. test empty array/vec, will result in error but without inner errors (and that's ok)
  2. test [Ok, Err]
  3. test [Err, Ok]
  4. test [Ok, Ok]
  5. test [Err, Err, Ok]
  6. test [Err, Err]

2-6 for both vec/array I guess.

You can use the existing https://ramaproxy.org/docs/rama/dns/struct.DenyAllDns.html for your err case, and https://ramaproxy.org/docs/rama/dns/struct.InMemoryDns.html for the ok case. I wouldn't check on the actual inner errors, just the high level Ok vs Err is good enough :) And if ok of course do check if you get the expected IP. Make sure to test both IPv4 and IPv6.

Feel free to cut on testing where you see fit, logic is simple enough that I don't think we really need that much testing here, but given you seem motivated to do so, do feel free to go for it :)

parkma99 commented 3 weeks ago

Hi @GlenDC ,I still have no idea for testing, could you give me some example code , thanks

GlenDC commented 3 weeks ago

Hi @GlenDC ,I still have no idea for testing, could you give me some example code , thanks

Sure, in rama-dns/src/in_memory.rs there are already some example tests.

So to pick up some of the given example cases, you can start with:

#[cfg(test)]
mod tests {
    use std::net::{Ipv4Addr, Ipv6Addr};
    use rama_core::combinators::Either;

    use super::*;

    #[tokio::test]
    async fn test_empty_chain_vec() {
        let v = Vec::<DnsOverwrite>::new();
        assert!(v
            .ipv4_lookup(Domain::from_static("plabayo.tech"))
            .await
            .is_err());
        assert!(v
            .ipv6_lookup(Domain::from_static("plabayo.tech"))
            .await
            .is_err());
    }

    #[tokio::test]
    async fn test_empty_chain_array() {
        let a: [DnsOverwrite; 0] = [];
        assert!(a
            .ipv4_lookup(Domain::from_static("plabayo.tech"))
            .await
            .is_err());
        assert!(a
            .ipv6_lookup(Domain::from_static("plabayo.tech"))
            .await
            .is_err());
    }

    #[tokio::test]
    async fn test_chain_ok_err_ipv4() {
        let v = vec![
            Either::A(serde_html_form::from_str("example.com=127.0.0.1").unwrap()),
            Either::B(DenyAllDns::new()),
        ];
        assert!(
            v.ipv4_lookup(Domain::from_static("example.com"))
                .await
                .unwrap()
                .into_iter()
                .next()
                .unwrap(),
            Ipv4Addr::new(127, 0, 0, 1)
        );
        assert!(v
            .ipv6_lookup(Domain::from_static("example.com"))
            .await
            .is_err());
    }

    #[tokio::test]
    async fn test_chain_err_err_ok_ipv6() {
        // ... TODO
    }

    // TODO ...
}

Haven't tested the code or ran it so you might need to solve some issues in the code, but hopefully it gets the idea across. I certainly wouldn't test all possible combinations. Just try to combine a couple so that you more or less have tested some couple things. I'm not interested in full test coverage, so try to be minimal and smart about it, as this is also all code to maintain in the end. And thanks to how we write the code, e.g. using a macro for a single implementation, it is already more guaranteed that we have working code for both sequence implementations.