feat(iroh-dns-server)!: add dht fallback option #2188

rklaehn · 2024-04-15T15:44:52Z

Description

feat(iroh-dns-server): add dht fallback option

if we don't have a record, ask the DHT as a last resort.

The DHT entries need to go into a separate ttl cache so they are invalidated when they get too old.

Breaking changes

iroh_dns_server::config::Config struct has a new field mainline.

Notes & open questions

The issue with async in pkarr has been solved.

Note: caching will be integrated into pkarr in v2. Once that is out we might be able to remove the cache again and just use pkarr with an appropriate sized cache configured.

Change checklist

Self-review.
Documentation updates if relevant.
Tests if relevant.

Frando · 2024-04-15T23:41:53Z

It seems to work just fine for me?

published a record on mainline via pkarr:
https://app.pkarr.org/?pk=p9e6imo9qws9c3wr94o1rh5o9oc34aae1n4n1hdkdk9dy8fmxyoy

then running cargo run --bin iroh-dns-server on this branch, and now both of the following work:

dig @localhost -p 5300 _iroh.p9e6imo9qws9c3wr94o1rh5o9oc34aae1n4n1hdkdk9dy8fmxyoy. TXT
dig @localhost -p 5300 _iroh.p9e6imo9qws9c3wr94o1rh5o9oc34aae1n4n1hdkdk9dy8fmxyoy.irohdns.example TXT

and return

;; QUESTION SECTION:
;_iroh.p9e6imo9qws9c3wr94o1rh5o9oc34aae1n4n1hdkdk9dy8fmxyoy.irohdns.example. IN TXT
;; ANSWER SECTION:
_iroh.p9e6imo9qws9c3wr94o1rh5o9oc34aae1n4n1hdkdk9dy8fmxyoy.irohdns.example. 30 IN TXT "relay=https://publishedviamainline.example"

rklaehn · 2024-04-16T05:39:13Z

The issue that was confusing me was with subdomains. I don't understand why the order here is reversed:

;; QUESTION SECTION:
;iroh._relay_url.urfy9rxfmgd66bf5qb6c1d9xxjjm78og76zaerrqma5nsuqxw88o. IN TXT

;; ANSWER SECTION:
_relay_url.iroh.urfy9rxfmgd66bf5qb6c1d9xxjjm78og76zaerrqma5nsuqxw88o. 0    IN TXT "https://euw1-1.derp.iroh.network./"

The pkarr web ui shows _relay_url.iroh but I have to query it using iroh._relay_url.. But then I get back an answer containing _relay_url.iroh. as in the GUI. What's going on there?

Frando · 2024-04-16T08:54:42Z

I found the issue. It is a bug in iroh-dns-server. Will post a PR with a fix once I finished writing a test.

# Conflicts: # Cargo.lock

@rklaehn

## Description The order of labels when parsing a DNS query as pkarr name was reversed. This means all queries for second-level subdomains under a pkarr pubkey failed. * First commit is the actual fix. * Second commit adds a test for various record name and type variations * Third and forth commit improve the debug and info logging ## Notes & open questions Was discovered by @rklaehn in #2188 and fixes the wrong behavior observed there. ## Change checklist - [x] Self-review. - [x] Documentation updates if relevant. - [x] Tests if relevant.

rklaehn · 2024-04-16T10:44:26Z

iroh-dns-server/src/server.rs

-    let store = ZoneStore::persistent(Config::signed_packet_store_path()?)?;
+    let mut store = ZoneStore::persistent(Config::signed_packet_store_path()?)?;
+    if config.dht_fallback {
+        store = store.with_pkarr(Some(Arc::new(PkarrClient::default())));


Not sure about this one. Do a builder?

@rklaehn

* refactor: downloader * test: add test for concurrent progress reporting * chore: cleanup * refactor: rename some things for clarity * feat: limit concurrent dials per download * chore: cleanup * refactor: respect progress sender IDs to allow reusing the senders in subsequent operations * chore: fmt & clippy * cleanup: shared download progress * feat: handle tags in the downloader * fix: visibility * fixup * fixup * chore: clippy * fixup * refactor: make export a seperate operation from download * fixup after merge * refactor: remove id mapping * refactor: move BlobId up to the progress events * fix tests * cleanup * fix: invariants * fixes and cleanups * fix: nodes_have should only add to queued downloads * chore: fmt * chore: clippy! * fix: do not queue downloads with no providers * feat: set more than one node on download requests (n0-computer#2128) ## Description Based on n0-computer#2085 The downloader already has the feature to try multiple nodes for a single hash or hashseq. With n0-computer#2085 we expose the downloader over the RPC protocol, by adding a `DownloadMode { Queued, Direct }` enum to the `BlobDownloadRequest`. This PR modifies the `BlobDownloadRequest` to include a list of provider nodes for a hash instead of just a single node. For queued requests that go to the downloader, the nodes are just passed to the downloader. For direct requests, the nodes are tried in-order, until one succeeds. ## Notes & open questions  ## Change checklist - [ ] Self-review. - [ ] Documentation updates if relevant. - [ ] Tests if relevant. * fix: address review by divma * fix: remove now obsolete into_send method from ProgressSender * chore: remove unused code * fix: use Display for DownloadKind * fix: first round of changes after review * docs: downloader next_step * more review * more review * fix: only remove hashes from provider map if they are really not needed * refactor: move TagSet into util * refactor: store intents in request info * cleanup * refactor: no allocation in next_step and simpler code * fix: test after merge * refactor: better method * fix(iroh-bytes): do not log redundant file delete error (n0-computer#2199) ## Description fix(iroh-bytes): do not log when a file can not be deleted... because it is not there. this makes the delete op idempotent, and also helps in case the file was not there in the first place. Fixes n0-computer#2142 ## Notes & open questions  ## Change checklist - [ ] Self-review. - [ ] Documentation updates if relevant. - [ ] Tests if relevant. * fix(iroh-dns-server): fix bug in pkarr name parsing (n0-computer#2200) ## Description The order of labels when parsing a DNS query as pkarr name was reversed. This means all queries for second-level subdomains under a pkarr pubkey failed. * First commit is the actual fix. * Second commit adds a test for various record name and type variations * Third and forth commit improve the debug and info logging ## Notes & open questions Was discovered by @rklaehn in n0-computer#2188 and fixes the wrong behavior observed there. ## Change checklist - [x] Self-review. - [x] Documentation updates if relevant. - [x] Tests if relevant. --------- Co-authored-by: Franz Heinzmann (Frando) <[email protected]> Co-authored-by: Rüdiger Klaehn <[email protected]>

@rklaehn

* refactor: downloader * test: add test for concurrent progress reporting * chore: cleanup * refactor: rename some things for clarity * feat: limit concurrent dials per download * chore: cleanup * refactor: respect progress sender IDs to allow reusing the senders in subsequent operations * chore: fmt & clippy * cleanup: shared download progress * feat: handle tags in the downloader * fix: visibility * fixup * fixup * chore: clippy * fixup * refactor: make export a seperate operation from download * fixup after merge * refactor: remove id mapping * refactor: move BlobId up to the progress events * fix tests * cleanup * fix: invariants * fixes and cleanups * fix: nodes_have should only add to queued downloads * chore: fmt * chore: clippy! * fix: do not queue downloads with no providers * feat: set more than one node on download requests (n0-computer#2128) ## Description Based on n0-computer#2085 The downloader already has the feature to try multiple nodes for a single hash or hashseq. With n0-computer#2085 we expose the downloader over the RPC protocol, by adding a `DownloadMode { Queued, Direct }` enum to the `BlobDownloadRequest`. This PR modifies the `BlobDownloadRequest` to include a list of provider nodes for a hash instead of just a single node. For queued requests that go to the downloader, the nodes are just passed to the downloader. For direct requests, the nodes are tried in-order, until one succeeds. ## Notes & open questions  ## Change checklist - [ ] Self-review. - [ ] Documentation updates if relevant. - [ ] Tests if relevant. * fix: address review by divma * fix: remove now obsolete into_send method from ProgressSender * chore: remove unused code * fix: use Display for DownloadKind * fix: first round of changes after review * docs: downloader next_step * more review * more review * fix: only remove hashes from provider map if they are really not needed * refactor: move TagSet into util * refactor: store intents in request info * cleanup * refactor: no allocation in next_step and simpler code * fix: test after merge * refactor: better method * fix(iroh-bytes): do not log redundant file delete error (n0-computer#2199) ## Description fix(iroh-bytes): do not log when a file can not be deleted... because it is not there. this makes the delete op idempotent, and also helps in case the file was not there in the first place. Fixes n0-computer#2142 ## Notes & open questions  ## Change checklist - [ ] Self-review. - [ ] Documentation updates if relevant. - [ ] Tests if relevant. * fix(iroh-dns-server): fix bug in pkarr name parsing (n0-computer#2200) ## Description The order of labels when parsing a DNS query as pkarr name was reversed. This means all queries for second-level subdomains under a pkarr pubkey failed. * First commit is the actual fix. * Second commit adds a test for various record name and type variations * Third and forth commit improve the debug and info logging ## Notes & open questions Was discovered by @rklaehn in n0-computer#2188 and fixes the wrong behavior observed there. ## Change checklist - [x] Self-review. - [x] Documentation updates if relevant. - [x] Tests if relevant. --------- Co-authored-by: Franz Heinzmann (Frando) <[email protected]> Co-authored-by: Rüdiger Klaehn <[email protected]>

@rklaehn

* refactor: downloader * test: add test for concurrent progress reporting * chore: cleanup * refactor: rename some things for clarity * feat: limit concurrent dials per download * chore: cleanup * refactor: respect progress sender IDs to allow reusing the senders in subsequent operations * chore: fmt & clippy * cleanup: shared download progress * feat: handle tags in the downloader * fix: visibility * fixup * fixup * chore: clippy * fixup * refactor: make export a seperate operation from download * fixup after merge * refactor: remove id mapping * refactor: move BlobId up to the progress events * fix tests * cleanup * fix: invariants * fixes and cleanups * fix: nodes_have should only add to queued downloads * chore: fmt * chore: clippy! * fix: do not queue downloads with no providers * feat: set more than one node on download requests (n0-computer#2128) ## Description Based on n0-computer#2085 The downloader already has the feature to try multiple nodes for a single hash or hashseq. With n0-computer#2085 we expose the downloader over the RPC protocol, by adding a `DownloadMode { Queued, Direct }` enum to the `BlobDownloadRequest`. This PR modifies the `BlobDownloadRequest` to include a list of provider nodes for a hash instead of just a single node. For queued requests that go to the downloader, the nodes are just passed to the downloader. For direct requests, the nodes are tried in-order, until one succeeds. ## Notes & open questions  ## Change checklist - [ ] Self-review. - [ ] Documentation updates if relevant. - [ ] Tests if relevant. * fix: address review by divma * fix: remove now obsolete into_send method from ProgressSender * chore: remove unused code * fix: use Display for DownloadKind * fix: first round of changes after review * docs: downloader next_step * more review * more review * fix: only remove hashes from provider map if they are really not needed * refactor: move TagSet into util * refactor: store intents in request info * cleanup * refactor: no allocation in next_step and simpler code * fix: test after merge * refactor: better method * fix(iroh-bytes): do not log redundant file delete error (n0-computer#2199) ## Description fix(iroh-bytes): do not log when a file can not be deleted... because it is not there. this makes the delete op idempotent, and also helps in case the file was not there in the first place. Fixes n0-computer#2142 ## Notes & open questions  ## Change checklist - [ ] Self-review. - [ ] Documentation updates if relevant. - [ ] Tests if relevant. * fix(iroh-dns-server): fix bug in pkarr name parsing (n0-computer#2200) ## Description The order of labels when parsing a DNS query as pkarr name was reversed. This means all queries for second-level subdomains under a pkarr pubkey failed. * First commit is the actual fix. * Second commit adds a test for various record name and type variations * Third and forth commit improve the debug and info logging ## Notes & open questions Was discovered by @rklaehn in n0-computer#2188 and fixes the wrong behavior observed there. ## Change checklist - [x] Self-review. - [x] Documentation updates if relevant. - [x] Tests if relevant. --------- Co-authored-by: Franz Heinzmann (Frando) <[email protected]> Co-authored-by: Rüdiger Klaehn <[email protected]>

@rklaehn

* refactor: downloader * test: add test for concurrent progress reporting * chore: cleanup * refactor: rename some things for clarity * feat: limit concurrent dials per download * chore: cleanup * refactor: respect progress sender IDs to allow reusing the senders in subsequent operations * chore: fmt & clippy * cleanup: shared download progress * feat: handle tags in the downloader * fix: visibility * fixup * fixup * chore: clippy * fixup * refactor: make export a seperate operation from download * fixup after merge * refactor: remove id mapping * refactor: move BlobId up to the progress events * fix tests * cleanup * fix: invariants * fixes and cleanups * fix: nodes_have should only add to queued downloads * chore: fmt * chore: clippy! * fix: do not queue downloads with no providers * feat: set more than one node on download requests (n0-computer#2128) ## Description Based on n0-computer#2085 The downloader already has the feature to try multiple nodes for a single hash or hashseq. With n0-computer#2085 we expose the downloader over the RPC protocol, by adding a `DownloadMode { Queued, Direct }` enum to the `BlobDownloadRequest`. This PR modifies the `BlobDownloadRequest` to include a list of provider nodes for a hash instead of just a single node. For queued requests that go to the downloader, the nodes are just passed to the downloader. For direct requests, the nodes are tried in-order, until one succeeds. ## Notes & open questions  ## Change checklist - [ ] Self-review. - [ ] Documentation updates if relevant. - [ ] Tests if relevant. * fix: address review by divma * fix: remove now obsolete into_send method from ProgressSender * chore: remove unused code * fix: use Display for DownloadKind * fix: first round of changes after review * docs: downloader next_step * more review * more review * fix: only remove hashes from provider map if they are really not needed * refactor: move TagSet into util * refactor: store intents in request info * cleanup * refactor: no allocation in next_step and simpler code * fix: test after merge * refactor: better method * fix(iroh-bytes): do not log redundant file delete error (n0-computer#2199) ## Description fix(iroh-bytes): do not log when a file can not be deleted... because it is not there. this makes the delete op idempotent, and also helps in case the file was not there in the first place. Fixes n0-computer#2142 ## Notes & open questions  ## Change checklist - [ ] Self-review. - [ ] Documentation updates if relevant. - [ ] Tests if relevant. * fix(iroh-dns-server): fix bug in pkarr name parsing (n0-computer#2200) ## Description The order of labels when parsing a DNS query as pkarr name was reversed. This means all queries for second-level subdomains under a pkarr pubkey failed. * First commit is the actual fix. * Second commit adds a test for various record name and type variations * Third and forth commit improve the debug and info logging ## Notes & open questions Was discovered by @rklaehn in n0-computer#2188 and fixes the wrong behavior observed there. ## Change checklist - [x] Self-review. - [x] Documentation updates if relevant. - [x] Tests if relevant. --------- Co-authored-by: Franz Heinzmann (Frando) <[email protected]> Co-authored-by: Rüdiger Klaehn <[email protected]>

Frando

The code LGTM. Ideally this would include a test that runs a mainline DHT node locally so that we don't accidentally break things (and have a confirmation that things work). Not sure though if this is straightforward to do.

iroh-dns-server/src/store.rs

Frando · 2024-04-29T09:35:48Z

#2250 has a test for this new functionality.

## Description This adds a test for iroh-dns-server with mainline fallback resolution. Needed to rework the config structure a little to not duplicate code too much. ## Breaking Changes are documented in #2180 ## Notes & open questions  ## Change checklist - [x] Self-review. - [x] Documentation updates if relevant. - [x] Tests if relevant. - [x] All breaking changes documented.

## Description By merging #2188 we unknowingly made `iroh` bind a socket for a mainline DHT. Pkarr includes code for querying records from the bittorent mainline DHT behind a `dht` feature flag, which is disabled in `iroh-net` but with #2188 is enabled in `iroh-dns-server`, and due to feature unification in cargo workspaces this silently also enabled the feature in `iroh-net`. The DHT is not used because we only use the `relay_` methods of the pkarr client in iroh-net. It is binding a socket, but not doing anything else because it has no bootstrap node configured. Still, this is unfortunate - we are shipping code and binding a socket for a feature we don't (yet) use. From a cursory skim of the source code, this would still be the same with the recently released `pkarr` v2. We should check the docs on feature unification if there is a clean way around that. For now, this PR makes iroh not bind a socket for an unused mainline DHT client. Instead, we just use reqwest directly, which is the same as what `PkarrClient::relay_put` would do here. ## Breaking Changes  ## Notes & open questions  ## Change checklist - [x] Self-review. - [x] Documentation updates if relevant. - [x] Tests if relevant. - [x] All breaking changes documented.

@rklaehn

## Description The order of labels when parsing a DNS query as pkarr name was reversed. This means all queries for second-level subdomains under a pkarr pubkey failed. * First commit is the actual fix. * Second commit adds a test for various record name and type variations * Third and forth commit improve the debug and info logging ## Notes & open questions Was discovered by @rklaehn in #2188 and fixes the wrong behavior observed there. ## Change checklist - [x] Self-review. - [x] Documentation updates if relevant. - [x] Tests if relevant.

## Description feat(iroh-dns-server): add dht fallback option if we don't have a record, ask the DHT as a last resort. The DHT entries need to go into a separate ttl cache so they are invalidated when they get too old. ## Breaking changes `iroh_dns_server::config::Config` struct has a new field `mainline`. ## Notes & open questions The issue with async in pkarr has been solved. Note: caching will be integrated into pkarr in v2. Once that is out we might be able to remove the cache again and just use pkarr with an appropriate sized cache configured. ## Change checklist - [x] Self-review. - [x] Documentation updates if relevant. - [ ] Tests if relevant. --------- Co-authored-by: Franz Heinzmann <[email protected]>

rklaehn added 4 commits April 15, 2024 16:24

WIP

d695e2a

Wire up pkarr dht fallback

644ee36

WIP dht fallback for DNS

b035c42

WIP debugging

71a4b35

Merge branch 'main' into iroh-dns-mainline

7ca967b

# Conflicts: # Cargo.lock

Frando mentioned this pull request Apr 16, 2024

fix(iroh-dns-server): fix bug in pkarr name parsing #2200

Merged

3 tasks

rklaehn added 2 commits April 16, 2024 13:02

nicer init

7749101

Merge branch 'main' into iroh-dns-mainline

9c08251

rklaehn commented Apr 16, 2024

View reviewed changes

better logging

af2a9a2

rklaehn marked this pull request as ready for review April 16, 2024 11:28

adjust config files

5d008de

rklaehn force-pushed the iroh-dns-mainline branch from a261fa8 to 5d008de Compare April 16, 2024 11:29

make mainline a separate config section

a213362

rklaehn force-pushed the iroh-dns-mainline branch from 0802cf6 to a213362 Compare April 16, 2024 11:59

shut up clippy

a6cb172

rklaehn requested a review from Frando April 16, 2024 12:58

update pkarr

c73d6a0

rklaehn mentioned this pull request Apr 16, 2024

DNS server pubky/pkarr#39

Closed

Merge branch 'main' into iroh-dns-mainline

5b27aa0

Frando approved these changes Apr 29, 2024

View reviewed changes

iroh-dns-server/src/store.rs Outdated Show resolved Hide resolved

dignifiedquire added this to the v0.15.0 milestone Apr 29, 2024

PR review: better docs for with_pkarr

17ff6f6

rklaehn enabled auto-merge April 29, 2024 09:08

Merge branch 'main' into iroh-dns-mainline

749c389

rklaehn added this pull request to the merge queue Apr 29, 2024

Frando removed this pull request from the merge queue due to a manual request Apr 29, 2024

Frando self-requested a review April 29, 2024 09:36

Frando changed the title ~~feat(iroh-dns-server): add dht fallback option~~ feat(iroh-dns-server)!: add dht fallback option Apr 29, 2024

Frando mentioned this pull request Apr 29, 2024

test: add test for iroh-dns-server with mainline #2250

Merged

4 tasks

Frando added this pull request to the merge queue Apr 29, 2024

Merged via the queue into main with commit 0b0508b Apr 29, 2024
20 of 21 checks passed

dignifiedquire deleted the iroh-dns-mainline branch April 29, 2024 11:56

Frando mentioned this pull request May 15, 2024

fix: do not bind a mainline DHT socket #2296

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(iroh-dns-server)!: add dht fallback option #2188

feat(iroh-dns-server)!: add dht fallback option #2188

rklaehn commented Apr 15, 2024 •

edited by Frando

Loading

Frando commented Apr 15, 2024 •

edited

Loading

rklaehn commented Apr 16, 2024

Frando commented Apr 16, 2024

rklaehn Apr 16, 2024

Frando left a comment •

edited

Loading

Frando commented Apr 29, 2024

feat(iroh-dns-server)!: add dht fallback option #2188

feat(iroh-dns-server)!: add dht fallback option #2188

Conversation

rklaehn commented Apr 15, 2024 • edited by Frando Loading

Description

Breaking changes

Notes & open questions

Change checklist

Frando commented Apr 15, 2024 • edited Loading

rklaehn commented Apr 16, 2024

Frando commented Apr 16, 2024

rklaehn Apr 16, 2024

Choose a reason for hiding this comment

Frando left a comment • edited Loading

Choose a reason for hiding this comment

Frando commented Apr 29, 2024

rklaehn commented Apr 15, 2024 •

edited by Frando

Loading

Frando commented Apr 15, 2024 •

edited

Loading

Frando left a comment •

edited

Loading