fix(iroh-dns-server): fix bug in pkarr name parsing #2200
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rklaehn
approved these changes
Apr 16, 2024
ppodolsky
added a commit
to izihawa/iroh
that referenced
this pull request
Apr 16, 2024
* 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 <!-- Any notes, remarks or open questions you have to make about the PR. --> ## 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 <!-- Any notes, remarks or open questions you have to make about the PR. --> ## 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]>
ppodolsky
added a commit
to izihawa/iroh
that referenced
this pull request
Apr 20, 2024
* 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 <!-- Any notes, remarks or open questions you have to make about the PR. --> ## 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 <!-- Any notes, remarks or open questions you have to make about the PR. --> ## 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]>
ppodolsky
added a commit
to izihawa/iroh
that referenced
this pull request
Apr 20, 2024
* 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 <!-- Any notes, remarks or open questions you have to make about the PR. --> ## 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 <!-- Any notes, remarks or open questions you have to make about the PR. --> ## 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]>
ppodolsky
added a commit
to izihawa/iroh
that referenced
this pull request
Apr 20, 2024
* 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 <!-- Any notes, remarks or open questions you have to make about the PR. --> ## 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 <!-- Any notes, remarks or open questions you have to make about the PR. --> ## 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]>
matheus23
pushed a commit
that referenced
this pull request
Nov 14, 2024
## 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Notes & open questions
Was discovered by @rklaehn in #2188 and fixes the wrong behavior observed there.
Change checklist