Review of `main` by Frando · Pull Request #5 · n0-computer/iroh-dns-server

Frando · 2024-03-28T10:31:06Z

This contains the full diff of the empty repo against main for review purposes.

dignifiedquire · 2024-04-02T16:26:03Z

iroh-dns-server/src/http.rs

+        inc!(Metrics, http_requests_error);
+    }
+    //
+    // let labels = [


this is copypasted from the axum-telemetry example. opentelemetry and prometheus can associate counters and histograms to labels, which allows to e.g. see how many PUT vs GET requests occured, or the request durations for a specific path, etc. iroh-metrics does not support this at the moment. cc @Arqu - maybe you know more what we are missing to support this? Not a high priority atm I think, but would be nice to have also for metrics in iroh (e.g. attach document id to doc metrics to thus track transfer etc per doc in addition to global),

dignifiedquire

looks good for the most part, left some code style nits

Nuhvi · 2024-04-02T16:32:38Z

WDYT?

As far as I understand from researching this, Variant C is the most idiomatic, and I would definitely encourage it despite the size overhead because:

I found no convention for how to add multiple attributes in the same TXT, some use ; separators, but really I never saw multiple attributes in the wild yet.
TXT records are made of smaller parts that have 255 bytes limit, so if you make a longer TXT record, it will be split into multiple of these strings, and now you have to cross your fingers that all clients know how to join these and then parse attributes (see long_attributes, which ... ehm I added to simple-dns when I wanted to do Variant B with lots of data, so no guarantee that others will do the same), really not worth it.

rklaehn · 2024-04-02T20:08:39Z

"relay=https://derp.io",
"addr=10.0.0.2:3033",
"addr=1.2.3.4:3033"

So you are saying we could get into trouble when the total text in the record exceeds 255 bytes? I think that is unlikely to happen. It's just a few addrs and a derp url.

With the less efficient encoding of option C I would worry that we might reach the hard 1000 byte limit of a pkarr record.

Frando · 2024-04-02T22:08:51Z

iroh-dns-server/src/dns.rs

+}
+
+impl DnsServer {
+    /// Create a DNS server given some settings, a connection to the DB for DID-by-username lookups


Fix comment

Frando · 2024-04-02T22:10:50Z

iroh-dns-server/src/dns/node_authority.rs

+        serial: u32,
+    ) -> Result<Self> {
+        let origins = {
+            let mut o = Vec::with_capacity(additional_origins.len());


should be + 1

Nuhvi · 2024-04-03T05:41:54Z

"relay=https://derp.io",
"addr=10.0.0.2:3033",
"addr=1.2.3.4:3033"

So you are saying we could get into trouble when the total text in the record exceeds 255 bytes? I think that is unlikely to happen. It's just a few addrs and a derp url.

With the less efficient encoding of option C I would worry that we might reach the hard 1000 byte limit of a pkarr record.

It is your call, now you know the tradeoff. Maybe you don't care about other DNS clients parsing attributes correctly.

Frando · 2024-04-03T10:18:05Z

I did some tests:
For a hypothetical packet with one relay URL, one ipv4 and one ipv6 address (so three attributes in total), the bytes-encoded pkarr SignedPacket has a total size of:
variant B: 267 bytes
variant C: 291 bytes

A difference of 24 bytes. If I add another addr attribute, the difference is 36 bytes. So variant C has an overhead of 12 bytes per attribute. I thought this to be more, because each attribute creates a new record in variant C which includes the z32pubkey in the name. However! DNS supports a compression during encoding, which the simple_dns library used by pkarr applies, and thus identical labels are not repeated!

I also did some more reading:

There's RFC 1464 (from 1993!) which describes the attribute format of key=value (with an escaping scheme to allow including = in the key. It does not talk about multiple attributes under a single label, however it specifies that all characters after the =, without escaping, form the value of the attribute; so variant B would not comply to the standard
RFC 1464 does not talk about multiple attributes for a common label much, it does however say that A more complex routine could return attributes with multiple values, or several different attributes. which, I think, means that those would need to use either variant C or D to comply with the RFC
There's a RFC 5507 which says that the most idiomatic form would be to register a new RRType with IANA, however this is out of scope for us for now, and would need client support.

I also tried to find out how other protocols use DNS TXT records

RFC 6764: DNS-SD uses Variant B: Each key/value pair is encoded as its own constituent string within the DNS TXT record, in the form "key=value"
Some protocols append all strings in a TXT record to increase size of the value above 255 bytes,, thus would not allow variant B
The tool sdget supports both Variant B and Variant C

So I think both variant B and C will work fine for our usecase. I now have a slight tendency towards variant C:

12 bytes overhead per attribute seems acceptable
it composes best, because each attribute is independent of others?

Nuhvi · 2024-04-03T11:13:34Z

Yup, for some reason I took the compression for granted, and yes Pkarr encodes packets compressed, because obviously the 52 characters of the z32 tld can add up pretty quickly.

Thanks for the thorough research confirming my hand wavy suspicion that multiple attributes in a TXT is too dangerous

rklaehn · 2024-04-04T07:10:29Z

So I think both variant B and C will work fine for our usecase. I now have a slight tendency towards variant C:

12 bytes is less than I expected. So I guess there is not much difference between B and C. However, not quite sure what the benefit of C is. Either the packet contains the required info or not. What's the benefit of having this in separate attributes? Does this make command line usage more convenient?

Nuhvi · 2024-04-04T07:14:15Z

@rklaehn only that if you don't control the DNS parser (unlikely but hey if you embrace DNS, embrace it) then staying close to the idiomatic way help parsers out there read attributes better. As Frando mentioned, many parsers will just consider everything after the first = as one value.

Frando · 2024-04-04T18:56:31Z

I started to write a draft for how to document this, see n0-computer/iroh#2149 - let's take the design discussion over there!

Frando · 2024-04-10T12:05:07Z

We're going to move this into the iroh repo. Please take further discussion to n0-computer/iroh#2167

## Description Imports https://github.com/n0-computer/iroh-dns-server into this repo. See n0-computer/iroh-dns-server#5 for previous review/discussion. Now includes an integration smoke test in `iroh-dns-server/src/lib.rs`. ## Notes & open questions I *think* I addressed most review points that came up in the initial review. Prominently still open is: * The `redb` store is used from async context but only exposes a sync interface (redb default). I think this is fine for medium load. However for better performance we should reuse transactions, which likely means we need an actor on a separate thread, as we do in iroh-bytes and iroh-sync. ## Change checklist - [ ] Self-review. - [ ] Documentation updates if relevant. - [ ] Tests if relevant.

Frando added 30 commits February 20, 2024 20:25

initial commit

d110e0a

restructure

45cf8b4

progress

21252cb

feat: add A record for dns server

253c390

progress progress

c33a657

fix: add NS records

7cbdf77

fix: allow to set NS entry

263e2de

feat: cert cache

9a626fe

disable forwarder

42ac47a

refactor lib and make things nicer

35a6e78

move out sig0 experiments

ba6eb02

chore: cleanup

719e254

fix: make examples work

a7eb81e

refactor: better authority in dns server

60e38d2

deps: bump pkarr

8ea565a

refactor: move iroh-dns into iroh crate

f058ec5

fix docs

8dd53b5

fix

3eab3d8

fixes

4ac3c3e

chore: cargo-fix

fa0b116

chore: clippy fix

de2768f

chore: fmt

99178cf

refactor: cleanup code paths

2163959

refactor: be closer to pkarr, add persistence

203b3e6

refactor: resolve pkarr signed packets directly

3f68d44

fix: back to released pkarr

b32dff5

feat: rate limiting

702509a

feat: example for publish & resolve

456d9a9

change: only rate-limit pkarr put

e93a361

chore: fmt

2ceb1f4

dignifiedquire reviewed Apr 2, 2024

View reviewed changes

Frando commented Apr 2, 2024

View reviewed changes

Frando mentioned this pull request Apr 4, 2024

RFC: _iroh DNS records and pkarr signed packets n0-computer/iroh#2149

Closed

Frando added 14 commits April 8, 2024 21:21

feat: support multiple https domains

4592dbe

fix: fix and improve examples

bca7034

Merge remote-tracking branch 'origin/change-dev-port'

a2ce9df

Merge remote-tracking branch 'origin/create-data-dir'

1a0396c

fix: address review by dig

03b0c16

fixup

7fbc983

fix: examples

b0e0f0e

refactor: improve state handling

dbefd19

refactor: improve state handling

a70ef5a

refactor: better config layout and testability

1e47fcb

chore: cargo update

0326961

chore: clippy

2b500da

cleanup: docs and module structure

319f770

fix: readme

0ff3f68

Frando mentioned this pull request Apr 10, 2024

feat: pkarr relay with DNS server n0-computer/iroh#2167

Merged

3 tasks

Conversation

Frando commented Mar 28, 2024

Uh oh!

dignifiedquire Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

Frando Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

dignifiedquire left a comment

Choose a reason for hiding this comment

Uh oh!

Nuhvi commented Apr 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rklaehn commented Apr 2, 2024

Uh oh!

Frando Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

Frando Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

Nuhvi commented Apr 3, 2024

Uh oh!

Frando commented Apr 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nuhvi commented Apr 3, 2024

Uh oh!

rklaehn commented Apr 4, 2024

Uh oh!

Nuhvi commented Apr 4, 2024

Uh oh!

Frando commented Apr 4, 2024

Uh oh!

Frando commented Apr 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Nuhvi commented Apr 2, 2024 •

edited

Loading

Frando commented Apr 3, 2024 •

edited

Loading