initial commit for DNS local resolver #262

jaytaph · 2023-11-14T20:42:17Z

This is the initial commit for the DNS resolver component.
In consists of a single entrypoint which returns a DNSEntry. It is up to the caller to actually extract the correct ips it wants (either the ipv4() or ipv6().

There are 3 resolvers that are called in sequence until one of them returns a hit:

caching resolver
local table resolver
remote resolver

The caching resolver has a fixed size cache that will retrieve quickly a dns record if it is available.
The local table resolver contains a list of items that will be checked against (including wildcards). This allows to override hosts just like an /etc/host file or dnsmasq would do
The remote resolver calls hickory-dns, which will resolve the query.

It is not 100% finished, but the major work is done. As soon as we have the config store in place, we can use that for configuration purposes.

Another things left to do is expiring of DNS records.

    Finished test [unoptimized + debuginfo] target(s) in 0.08s
     Running unittests src/lib.rs (target/debug/deps/gosub_engine-761d2bb186cfbf97)
2023-11-17T21:57:37.971Z INFO  [gosub_engine::dns] Resolving example.org for Ipv4
2023-11-17T21:57:37.971Z DEBUG [gosub_engine::dns] Trying resolver: cache resolver
2023-11-17T21:57:37.971Z DEBUG [gosub_engine::dns] Trying resolver: local table resolver
2023-11-17T21:57:37.971Z TRACE [gosub_engine::dns::local] example.org: not found in local table
2023-11-17T21:57:37.971Z DEBUG [gosub_engine::dns] Trying resolver: remote resolver
2023-11-17T21:57:37.971Z TRACE [gosub_engine::dns::remote] example.org: resolving with ["ipv4"]
2023-11-17T21:57:37.977Z TRACE [gosub_engine::dns::remote] example.org: found ipv4 address 93.184.216.34
2023-11-17T21:57:37.978Z DEBUG [gosub_engine::dns] Found entry DnsEntry { domain: "example.org", ips: [93.184.216.34], has_ipv4: true, has_ipv6: false, iter: 0, expires: 0 }
2023-11-17T21:57:37.978Z TRACE [gosub_engine::dns::cache] example.org: announcing to cache
2023-11-17T21:57:37.978Z TRACE [gosub_engine::dns::cache] adding new entry to cache
ipv4: 93.184.216.34
Took 6626 microseconds.
2023-11-17T21:57:37.978Z INFO  [gosub_engine::dns] Resolving example.org for Ipv6
2023-11-17T21:57:37.978Z DEBUG [gosub_engine::dns] Trying resolver: cache resolver
2023-11-17T21:57:37.978Z TRACE [gosub_engine::dns::cache] example.org: no ipv6 addresses found in entry
2023-11-17T21:57:37.978Z DEBUG [gosub_engine::dns] Trying resolver: local table resolver
2023-11-17T21:57:37.978Z TRACE [gosub_engine::dns::local] example.org: not found in local table
2023-11-17T21:57:37.978Z DEBUG [gosub_engine::dns] Trying resolver: remote resolver
2023-11-17T21:57:37.978Z TRACE [gosub_engine::dns::remote] example.org: resolving with ["ipv6"]
2023-11-17T21:57:37.986Z TRACE [gosub_engine::dns::remote] example.org: found ipv6 address 2606:2800:220:1:248:1893:25c8:1946
2023-11-17T21:57:37.986Z DEBUG [gosub_engine::dns] Found entry DnsEntry { domain: "example.org", ips: [2606:2800:220:1:248:1893:25c8:1946], has_ipv4: false, has_ipv6: true, iter: 0, expires: 0 }
2023-11-17T21:57:37.986Z TRACE [gosub_engine::dns::cache] example.org: announcing to cache
2023-11-17T21:57:37.986Z TRACE [gosub_engine::dns::cache] example.org: updating existing entry to cache
2023-11-17T21:57:37.986Z TRACE [gosub_engine::dns::cache] current entries: [93.184.216.34]
2023-11-17T21:57:37.986Z TRACE [gosub_engine::dns::cache] new entries: [2606:2800:220:1:248:1893:25c8:1946]
2023-11-17T21:57:37.986Z TRACE [gosub_engine::dns::cache] merged entries: [93.184.216.34, 2606:2800:220:1:248:1893:25c8:1946]
ipv6: 2606:2800:220:1:248:1893:25c8:1946
Took 8253 microseconds.
2023-11-17T21:57:37.986Z INFO  [gosub_engine::dns] Resolving example.org for Ipv4
2023-11-17T21:57:37.986Z DEBUG [gosub_engine::dns] Trying resolver: cache resolver
2023-11-17T21:57:37.986Z TRACE [gosub_engine::dns::cache] example.org: found in cache with correct resolve type
2023-11-17T21:57:37.986Z DEBUG [gosub_engine::dns] Found entry DnsEntry { domain: "example.org", ips: [93.184.216.34, 2606:2800:220:1:248:1893:25c8:1946], has_ipv4: true, has_ipv6: true, iter: 0, expires: 0 }
2023-11-17T21:57:37.986Z TRACE [gosub_engine::dns::cache] example.org: announcing to cache
2023-11-17T21:57:37.986Z TRACE [gosub_engine::dns::cache] example.org: updating existing entry to cache
2023-11-17T21:57:37.986Z TRACE [gosub_engine::dns::cache] current entries: [93.184.216.34, 2606:2800:220:1:248:1893:25c8:1946]
2023-11-17T21:57:37.986Z TRACE [gosub_engine::dns::cache] new entries: [93.184.216.34, 2606:2800:220:1:248:1893:25c8:1946]
2023-11-17T21:57:37.986Z TRACE [gosub_engine::dns::cache] merged entries: [93.184.216.34, 2606:2800:220:1:248:1893:25c8:1946]
ipv4: 93.184.216.34
Took 57 microseconds.
2023-11-17T21:57:37.986Z INFO  [gosub_engine::dns] Resolving example.org for Both
2023-11-17T21:57:37.986Z DEBUG [gosub_engine::dns] Trying resolver: cache resolver
2023-11-17T21:57:37.986Z TRACE [gosub_engine::dns::cache] example.org: found in cache with correct resolve type
2023-11-17T21:57:37.986Z DEBUG [gosub_engine::dns] Found entry DnsEntry { domain: "example.org", ips: [93.184.216.34, 2606:2800:220:1:248:1893:25c8:1946], has_ipv4: true, has_ipv6: true, iter: 0, expires: 0 }
2023-11-17T21:57:37.986Z TRACE [gosub_engine::dns::cache] example.org: announcing to cache
2023-11-17T21:57:37.986Z TRACE [gosub_engine::dns::cache] example.org: updating existing entry to cache
2023-11-17T21:57:37.986Z TRACE [gosub_engine::dns::cache] current entries: [93.184.216.34, 2606:2800:220:1:248:1893:25c8:1946]
2023-11-17T21:57:37.986Z TRACE [gosub_engine::dns::cache] new entries: [93.184.216.34, 2606:2800:220:1:248:1893:25c8:1946]
2023-11-17T21:57:37.986Z TRACE [gosub_engine::dns::cache] merged entries: [93.184.216.34, 2606:2800:220:1:248:1893:25c8:1946]
ipv4: 93.184.216.34
ipv6: 2606:2800:220:1:248:1893:25c8:1946
Took 45 microseconds.

src/dns.rs

src/dns/local.rs

src/dns.rs

src/dns/local.rs

Sharktheone · 2023-11-15T17:04:40Z

src/dns.rs

+
+    /// Retrieves the next ipv4 address in the round robin list
+    fn next_ipv4(&mut self) -> IpAddr {
+        let entry = self.ipv4.get(self.rr_ipv4_ptr).expect("Invalid round robin pointer");


I think, the expect shouldn't be here. This error is recoverable (just ignore the ip ptr), so maybe just use a Result or an Option / "silently" recover the error may be better.

src/dns/local.rs

Sharktheone · 2023-11-15T17:20:38Z

src/dns/local.rs

-            return Some(*ips.get(0).unwrap());
-        }
+        // Fetch the actual entry in the entries list
+        let found_entry = self.entries.get_mut(matched_entry.expect("No idx found")).expect("No entry found");


Here again, don't use expect, I mean this function already returns a Result. Maybe it would be nice to implement the functionality for the ? syntax. If this is not already done, i can do this if you want.

jaytaph · 2023-11-15T18:07:00Z

src/dns.rs

+/// and a last-used timestamp. When the entry is expired, the entry is found to be stale and should
+/// be refreshed by the resolver. When expired == 0, the entry is considered to be a local override
+/// and should not be refreshed by the resolver.
+/// @todo: Does that information belong here? Or should it be in the cache layer?


as stated in the todo: i'm not sure if both expired and last-used should be part of the ip entry. I think the only place where ttl / expiration is important, would be the caching layer. This means the resolver should return a tuple (ipaddress, ttl) so the layer above can cache it accordingly. Time last used may not be needed.

jaytaph · 2023-11-15T18:07:57Z

src/dns.rs

+
+/// A DNS entry is a simple domain name to IP address mapping
+#[derive(Default)]
+struct DNSEntry {


The DNS entry now consists of a domain name, and 2 lists of ip addresses, the ipv4 andipv6. It now contains the roundrobin pointers as well. The whole round robin is encapsulated in the DNSEntry so we can simply do next_ipv4() and not need to worry about it from the outside world.

jaytaph · 2023-11-15T18:08:43Z

src/dns.rs

+
+/// Type of DNS resolution
+#[derive(Debug, PartialEq, Clone)]
+pub enum ResolveType {


I don't know exactly who will be in charge of this, but somewhere we must decide if we resolve the ipv4 or ipv6 of a domain. This is the ResolveType we can add to the resolve() function.

jaytaph · 2023-11-15T18:09:36Z

src/dns/cache.rs

+use std::collections::HashMap;
+use crate::dns::DNSEntry;
+
+struct Cache {


The cache has a fixed size of max_entries elements and uses a lru strategy for evicting old dns entries. It's not yet complete implemented, but the lru seems to work according to the tests

jaytaph · 2023-11-15T18:10:55Z

src/dns/local.rs

+    /// Entries in the local override table.
+    entries: HashMap<String, DNSEntry>,
+    /// Domaintree is a hierarchical lookup tree for quick scanning of (wildcard) domains
+    tree: DomainLookupTree,


The wildcard matcher has been replaced by the tree lookup crate. It will match deeper wildcards better (*.example.org would match foo.bar.baz.example.org, and we don't want that).

Also, we now do not have to do a difficult process of matching both specific and wildcard matching.

jaytaph · 2023-11-15T18:11:40Z

src/dns/local.rs

+            return Ok(found_entry.next_ipv6());
+        }
+        if resolve_type == ResolveType::Both {
+            // IPv6 gets precedence over IPv4


Could be configurable by "dns.prefer.ipv6.over.ipv4" maybe

jaytaph · 2023-11-15T18:12:16Z

src/dns/local.rs

+    /// and the domain is: foo.example.org, then the most specific match is foo.example.org
+    ///
+    /// if the domain is test.foo.example.org, .foo.example.org is the best match
+    fn find_most_specific_match(&self, entries: Vec<usize>) -> usize {


we can remove this func

jaytaph · 2023-11-15T18:13:38Z

src/types.rs

    Query(String),
+
+    #[error("dns: generic error: {0}")]


I like the way we can simply create errors and return them in Result<> , but should we add them all inside a generic enum like this? Wouldn't we want the errors to be per component? (src/dns/errors.rs, src/html5/errors.rs etc)

I think it'd make sense to make them component specific now that the list is growing (and will grow more in the future.) I remember this was added maybe a month or two back when there were only 2-3 error types, but now I'd be in favour to move groups of them into their own components

I'm guessing the rule of thumb on when to create a new error type and when to stick with an existing type is when you want to match against a specific kind of error and not other errors. So the number of error types becomes the number of specific kinds of error you need to match on in high-level application code.

The matching in error handling code is what is important here. I can imagine needing a total of three kinds of error in all, or 20, depending on what kind of error handling is needed.

emwalker · 2023-11-16T01:18:19Z

src/dns.rs

+    // Round robin pointer in case we need to round robin
+    rr_ipv4_ptr: usize,
+    // Round robin pointer in case we need to round robin
+    rr_ipv6_ptr: usize,


Should this be an enum? Also, curious why we aren't making use of a simple wrapper around an existing Rust library. This looks more low-level than I was anticipating.

We are using hickory dns in the remote resolver. I'll draw up a diagram to give you an idea of the architecture (its not too complicated)

jaytaph · 2023-11-18T12:34:22Z

src/dns.rs

+    ips: Vec<IpAddr>,
+
+    /// True when the ips list has ipv4 addresses
+    has_ipv4: bool,


an entry has either resolved ipv4 addresses, resolver ipv6 addresses or both. WHen we have a record in cache, but we want to have ipv6 addresses for that record, we can see if these are already resolved for this record.

jaytaph · 2023-11-18T12:34:47Z

src/dns.rs

+    // Internal iterator pointer
+    iter: usize,
+    /// expiry time after epoch
+    expires: u64,


We don't use the expires time yet. This will be taken from a TTL that is returned by the remote resolver.

jaytaph · 2023-11-18T12:35:46Z

src/dns/cache.rs

+
+    /// When a domain is resolved, it will be announced to all resolvers. This cache resolver
+    /// will store it into the cache.
+    fn announce(&mut self, domain: &str, entry: &DnsEntry) {


Each resolver has an "announce", which will be called when a new dns entry has been found. It is only used by the cache resolver to store this in cache.

emwalker

It would be nice if you would update the iterator code I left a note on. But I also don't want to hold up this PR. I suspect some of the details will change later.

This code makes me nervous, because the network stack exposes a large surface area for security attacks. But I can also see why it's needed. I do wonder whether there's already some canonical Rust crates out there that do some or all of this. In the Rust ecosystem, semi-official Rust crates (e.g., serde) seem to play a bigger role than third-party libraries in other language ecosystems.

emwalker · 2023-11-18T14:11:06Z

src/dns.rs

+    fn next(&mut self) -> Option<Self::Item> {
+        if self.iter >= self.ips.len() {
+            // reset iterator at the end
+            self.iter = 0;


This cleanup was confusing for me, and now I see what the source of the confusion is. Iterators should be implemented as separate structs that fall out of scope / are discarded after they're exhausted. So a cleanup / reset step shouldn't be needed. See TreeIterator for an example of this pattern. If there's a cleanup step, it suggests a new struct needs to be added.

But in this case, a separate struct for the iterator isn't needed, because you can get an iterator over the items you want through other means:

diff --git a/src/dns.rs b/src/dns.rs index 36b6a6d..cb2b42e 100644 --- a/src/dns.rs +++ b/src/dns.rs @@ -70,24 +70,9 @@ impl DnsEntry { fn ipv6(&self) -> Vec<IpAddr> { self.ips.iter().filter(|x| x.is_ipv6()).copied().collect() } -} - -impl Iterator for DnsEntry { - type Item = IpAddr; - - /// With next() you can simply iterate over all the ips in the list - fn next(&mut self) -> Option<Self::Item> { - if self.iter >= self.ips.len() { - // reset iterator at the end - self.iter = 0; - - return None; - } - - let ip = self.ips[self.iter]; - self.iter += 1; - Some(ip) + fn iter(&self) -> impl Iterator<Item = &IpAddr> { + self.ips.iter() } }

emwalker · 2023-11-18T14:12:31Z

src/dns.rs

+//     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+//         write!(f, "dns resolver")
+//     }
+// }


emwalker · 2023-11-18T14:13:52Z

src/dns.rs

+        info!("Resolving {} for {:?}", domain, resolve_type);
+
+        for resolver in self.resolvers.iter_mut() {
+            debug!("Trying resolver: {}", resolver.name());


Nice use of the log crate.

emwalker · 2023-11-18T14:16:39Z

src/dns/remote.rs

+            }
+            ResolveType::Both => {
+                ip_types.push("ipv6");
+                ip_types.push("ipv4");


It strikes me as unusual that we're using a static string here as a type marker.

I guess we can just use the ResolverType itself and if it is Both we Push ResolverType::Ipv4 and ReolverType::Ipv6

emwalker · 2023-11-18T14:18:28Z

src/types.rs

    Query(String),
+
+    #[error("dns: generic error: {0}")]


I'm guessing the rule of thumb on when to create a new error type and when to stick with an existing type is when you want to match against a specific kind of error and not other errors. So the number of error types becomes the number of specific kinds of error you need to match on in high-level application code.

The matching in error handling code is what is important here. I can imagine needing a total of three kinds of error in all, or 20, depending on what kind of error handling is needed.

Sharktheone

Looks fine to me, just some simple things. But you can ignore them if you want.

Sharktheone · 2023-11-18T16:40:43Z

src/dns.rs

+            return None;
+        }
+
+        let ip = self.ips[self.iter];


Suggested change

let ip = self.ips[self.iter];

let ip = self.ips[self.iter]; //we can be safe this won't panic because we already checked if we are out of bounds

Maybe add a comment here. I always suspect something indexing with [] to panic, so to add a note here is maybe nice. But feel free to ignore

Sharktheone · 2023-11-18T16:44:01Z

src/dns.rs

+
+impl Dns {
+    pub fn new() -> Self {
+        let mut resolvers: Vec<Box<dyn DnsResolver>> = vec![];


Suggested change

let mut resolvers: Vec<Box<dyn DnsResolver>> = vec![];

let mut resolvers: Vec<Box<dyn DnsResolver>> = Vec::with_capacity(3);

Allocate the memory for the vec directly at the creation (Yes, I know, you'd like to make such things later on, but I think we forget such things if we don't do it now)

I would not worry about this too much. It's not used in any thight loops, and the first push will do an allocation for 4 elements anyway.
So instead of with_capacity(3), you get a with_capacity(4), which is ok enough.

Sharktheone · 2023-11-18T17:00:43Z

src/dns/remote.rs

+            }
+            ResolveType::Both => {
+                ip_types.push("ipv6");
+                ip_types.push("ipv4");


I guess we can just use the ResolverType itself and if it is Both we Push ResolverType::Ipv4 and ReolverType::Ipv6

src/dns.rs

emwalker · 2023-11-19T14:00:29Z

src/dns.rs

+
+    fn iter(&self) -> impl Iterator<Item = &IpAddr> {
+        self.ips.iter()
+    }


After I thought about it more, I realized you might have been wanting to use the iterator trait as a state machine for implementing round-robin. If so, I think something other than trait Iterator would be used for this, e.g., a custom method that updates the state to the next ip address, possibly in a new struct that this struct delegates to.

i've actually removed the round robin inside the dns entries because it ended up a bit too complex. I leave it up to the caller to deal with round robin. There might be other ways they want to use the set of resolved ips: when one fails, use another, etc..

jaytaph commented Nov 14, 2023

View reviewed changes

src/dns.rs Outdated Show resolved Hide resolved

jaytaph commented Nov 14, 2023

View reviewed changes

src/dns/local.rs Outdated Show resolved Hide resolved

Sharktheone requested changes Nov 14, 2023

View reviewed changes

src/dns.rs Outdated Show resolved Hide resolved

src/dns.rs Outdated Show resolved Hide resolved

src/dns/local.rs Outdated Show resolved Hide resolved

Sharktheone requested changes Nov 15, 2023

View reviewed changes

jaytaph marked this pull request as draft November 15, 2023 18:02

jaytaph commented Nov 15, 2023

View reviewed changes

emwalker reviewed Nov 16, 2023

View reviewed changes

jaytaph force-pushed the dns branch 2 times, most recently from 6281c9c to 95080b2 Compare November 18, 2023 12:23

jaytaph marked this pull request as ready for review November 18, 2023 12:25

initial commit for DNS local resolver

2135ff2

jaytaph force-pushed the dns branch from 95080b2 to 2135ff2 Compare November 18, 2023 12:29

jaytaph commented Nov 18, 2023

View reviewed changes

jaytaph requested review from emwalker, Sharktheone, Kiyoshika, neuodev and a team and removed request for Sharktheone November 18, 2023 12:49

Kiyoshika approved these changes Nov 18, 2023

View reviewed changes

jaytaph mentioned this pull request Nov 18, 2023

Make errors component specific now that the list is growing #265

Closed

emwalker approved these changes Nov 18, 2023

View reviewed changes

Sharktheone approved these changes Nov 18, 2023

View reviewed changes

fixed some suggestions

2e435cb

Sharktheone approved these changes Nov 19, 2023

View reviewed changes

jaytaph merged commit c7e9625 into main Nov 19, 2023
4 checks passed

jaytaph deleted the dns branch November 19, 2023 10:45

emwalker reviewed Nov 19, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initial commit for DNS local resolver #262

initial commit for DNS local resolver #262

jaytaph commented Nov 14, 2023 •

edited

Loading

Sharktheone Nov 15, 2023

Sharktheone Nov 15, 2023

jaytaph Nov 15, 2023

jaytaph Nov 15, 2023

jaytaph Nov 15, 2023

jaytaph Nov 15, 2023

jaytaph Nov 15, 2023

jaytaph Nov 15, 2023

jaytaph Nov 15, 2023

jaytaph Nov 15, 2023

Kiyoshika Nov 18, 2023

emwalker Nov 18, 2023

emwalker Nov 16, 2023

jaytaph Nov 16, 2023

jaytaph Nov 18, 2023

jaytaph Nov 18, 2023

jaytaph Nov 18, 2023

emwalker left a comment •

edited

Loading

emwalker Nov 18, 2023

emwalker Nov 18, 2023

jaytaph Nov 19, 2023

emwalker Nov 18, 2023

emwalker Nov 18, 2023

Sharktheone Nov 18, 2023

jaytaph Nov 19, 2023

emwalker Nov 18, 2023

Sharktheone left a comment

Sharktheone Nov 18, 2023

jaytaph Nov 19, 2023

Sharktheone Nov 18, 2023

jaytaph Nov 19, 2023

Sharktheone Nov 18, 2023

emwalker Nov 19, 2023

jaytaph Nov 19, 2023

	let ip = self.ips[self.iter];
	let ip = self.ips[self.iter]; //we can be safe this won't panic because we already checked if we are out of bounds

	let mut resolvers: Vec<Box<dyn DnsResolver>> = vec![];
	let mut resolvers: Vec<Box<dyn DnsResolver>> = Vec::with_capacity(3);

initial commit for DNS local resolver #262

initial commit for DNS local resolver #262

Conversation

jaytaph commented Nov 14, 2023 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emwalker left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sharktheone 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

Choose a reason for hiding this comment

jaytaph commented Nov 14, 2023 •

edited

Loading

emwalker left a comment •

edited

Loading