Client Cache by Philip-NLnetLabs · Pull Request #275 · NLnetLabs/domain

Philip-NLnetLabs · 2024-02-19T20:30:00Z

No description provided.

Cargo.toml

src/net/client/mod.rs

src/net/client/cache.rs

src/utils/config.rs

src/net/client/cache.rs

ximon18 · 2024-02-28T22:40:14Z

src/net/client/cache.rs

+    C: Clock + Send + Sync + 'static,
+{
+    /// Create a new connection with default configuration parameters.
+    pub fn new_with_time(upstream: Upstream, clock: C) -> Self {


If kept this should be documented as being intended for internal test purposes only. Adopting #278 however would remove this issue by removing these with_time functions.

ximon18 · 2024-02-28T22:52:51Z

One thing not provided by this PR at present is any notion of cache metrics/statistics. I also don't see any obvious support on the underlying moka crate for this, so presumably one would have to wrap internal calls in cache.rs to track the miss/hit ratio, for example. I don't see how the user of the cache in this crate could do this themselves without forking the code as the cache.get() call is deep inside library code, so users of this library would have to wait for us to add such support later or PR it themselves.

Add a config option to enable caching of truncated responses.

ximon18 · 2024-03-02T07:36:14Z

A thought:

Would it be worth making the cache get and insert interface a trait taken by the cache client, and making the current in memory moka cache one impl of it?

Then the same client could be used with an alternate backing cache impl.

This occurred to me because of a similar discussion about zone storage.

To fully support less local stores the interface would need to support async function calls I expect, but IIRC the moka cache is also used via an async interface.

That would also be a way to enable users to wrap the cache calls to gather usage metrics/statistics if that's what they need, without impacting performance when not wanting them.

- Import DefMinMax from PR #275 and use it to make stream server and connection limits configurable. - Remove the timeout on the network read operation as there is nothing in the RFC 7766 about it and it's unclear how to handle it or how it helps on top of the existing DNS idle time out. - Cleanup the idle timeout code naming and behaiour and use Tokio Instants which also makes the behaviour compatible with the existing `stop_service_test()` which uses `start_paused = true`. - When compiled in test mode detect incomplete reads due to async future cancellation. - For use by the test but also generally useful, add two metrics: num_received_requests and num_sent_responses. - Change the queueing of responses for sending so that it respects the queue limit rather than accumulating and endless number of Tokio tasks. - Don't return unused JoinHandles. - Accept but then drop connections beyond the max conncurrent TCP connections configured.

This PR adds experimental support for a client cache provided as a pass through transport

@hunts

Breaking changes * All types and functions referring to domain names have been changed from using the term “dname” to just “name.” For instance, `Dname` has become `Name`, `ToDname` has become `ToName`, and `ToDname::to_dname` has become `ToName::to_name`. ([#290]) * The `ToName` and `ToRelativeName` traits have been changed to have a pair of methods a la `try_to_name` and `to_name` for octets builders with limited and unlimited buffers, reflecting the pattern used elsewhere. ([#285]) * The types for IANA-registered parameters in `base::iana` have been changed from enums to a newtypes around their underlying integer type and associated constants for the registered values. (This was really always the better way to structure this.) ([#276], [#298]) * The `Txt` record data type now rejects empty record data as invalid. As a consequence `TxtBuilder` converts an empty builder into TXT record data consisting of one empty character string which requires `TxtBuilder::finish` to be able to return an error. ([#267]) * `Txt` record data serialization has been redesigned. It now serialized as a sequence of character strings. It also deserializes from such a sequence. If supported by the format, it alternatively deserializes from a string that is broken up into 255 octet chunks if necessary. ([#268]) * The text formatting for `CharStr` has been redesigned. The `Display` impl now uses a modified version of the representation format that doesn’t escape white space but also doesn’t enclose the string in quotes. Methods for explicitly formatting in quoted and unquoted presentation format are provided. ([#270]) * The `validate::RrsigExt` trait now accepts anything that impls `AsRef<Record<..>>` to allow the use of smart pointers. ([#288] by [@hunts]) * The stub resolver now uses the new client transports. This doesn’t change how it is used but does change how it queries the configured servers. ([#215]) * The sub resolver’s server configuration `Transport` type has been changed to be either `Transport::UdpTcp` for trying UDP and if that leads to a truncated answer try TCP and `Transport::Tcp` for only trying TCP. The stub resolver uses these accordingly now ([#296]) * Many error types have been changed from enums to structs that hide internal error details. Enums have been kept for errors where distinguishing variants might be meaningful for dealing with the error. ([#277]) * Renamed `Dnskey::is_zsk` to `is_zone_key`. ([#292]) * Split RRSIG timestamp handling from `Serial` into a new type `rdata::dnssec::Timestamp`. ([#294]) * Upgraded `octseq` to 0.5. ([#257]) * The minimum Rust version is now 1.70. ([#304]) New * Add impls for `AsRef<RelativeDname<[u8]>>` and `Borrow<RelativeDname<[u8]>>` to `RelativeDname<_>`. ([#251] by [@torin-carey]) * Added `name::Chain::fmt_with_dots` to format an absolute chained name with a final dot. ([#253]) * Added a new `ParseAnyRecordData` trait for record data types that can parse any type of record data. ([#256]) * Added implementations of `OctetsFrom` and `Debug` to `AllOptData` and the specific options types that didn’t have them yet. ([#257]) * Added missing ordering impls to `ZoneRecordData`, `AllRecordData`, `Opt`, and `SvcbRdata`. ([#293]) * Added `Name::reverse_from_addr` that creates a domain name for the reverse lookup of an IP address. ([#289]) * Added `OptBuilder::clone_from` to replace the OPT record with the content of another OPT record. ([#299]) * Added `Message::for_slice_ref` that returns a `Message<&[u8]>`. ([#300]) Bug fixes * Fixed the display implementation of `name::Chain<_, _>`. ([#253]) * Fixed the display implementation of `rdata::Txt<..>`. It now displays each embedded character string separately in quoted form. ([#259]) * Fixed the extended part returned by `OptRcode::to_parts` (it was shifted by 4 bits too many) and return all 12 bits for the `Int` variant in `OptRcode::to_int`. ([#258]) * Fixed a bug in the `inplace` zonefile parser that made it reject character string of length 255. ([#284]) Unstable features * Added the module `net::client` with experimental support for client message transport, i.e., sending of requests and receiving responses as well as caching of responses. This is gated by the `unstable-client-transport` feature. ([#215],[#275]) * Added the module `net::server` with experimental support for server transports, processing requests through a middleware chain and a service trait. This is gated by the `unstable-server-transport` feature. ([#274]) * Added the module `zonetree` providing basic traits representing a collection of zones and their data. The `zonetree::in_memory` module provides an in-memory implementation. The `zonetree::parsed` module provides a way to classify RRsets before inserting them into a tree. This is gated by the `unstable-zonetree` feature. ([#286])

WhyNotHugo · 2025-01-05T20:45:51Z

As a downstream consumer of this crate (for libdav and pimsync), the introduction of moka as a dependency is quite a burden: it substantially increases the amount of dependencies that I need to review, and total build time.

I'm mostly relying on domain for the stub resolver. If I understand correctly, moka is used in the context of DNSSEC validation, but the stub resolver doesn't currently implement DNSSEC validation.

My particular usage does a few DNS queries (often times 3-4 at startup for a process that then runs uninterrupted for days or weeks). Something simple and "slower" (e.g.: using RwLock<HashMap>) would probably not introduce any significant overhead for my use case. This is likely true for many other consumers.

Would it be worth making the cache get and insert interface a trait taken by the cache client, and making the current in memory moka cache one impl of it?

I think this would be a pretty apt solution. Downstream consumers can easily create a newtype aronud moka::cache::Cache or RwLock<HashMap>, or whatever is the best fit.

Philip-NLnetLabs · 2025-01-09T16:36:51Z

The client cache is mostly unrelated to DNSSEC validation. The cache does know about DNSSEC but is useful even without DNSSEC validation. There is no easy way around Moka. Moka has many nice features. Re-implementing them would essentially be a poor man's Moka.

That said, we could look into making the cache optional. The resolver currently does not use the cache.

WhyNotHugo · 2025-01-09T17:08:39Z

I tried making the cache optional by using C: Cache<Key, Arc<Value>> for a cache (the exact key and value types varies on each usage). This would allow consumers to provide any implementation; either moka or a very minimal lightweight one.

However, the Key type used for the cache is a private type in a few cases, so this required making the type public, as well as a few other invasive changes. I'm not convinced that the approach I have taken here is feasible.

Re-implementing them would essentially be a poor man's Moka.

I would find this very fitting for my use case.

That said, we could look into making the cache optional. The resolver currently does not use the cache.

That sounds good to me, but I don't think I'm familiar enough to try and execute that myself.

Philip-NLnetLabs · 2025-01-16T10:09:06Z

I created a PR that make the client cache optional. Could you please check if that works for you?

Philip-NLnetLabs requested a review from a team February 19, 2024 20:30