fix(namesys): handling of multiple DNSLink TXT records #510
Conversation
This file contains hidden or 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
Codecov Report
@@ Coverage Diff @@
## main #510 +/- ##
==========================================
- Coverage 65.52% 65.49% -0.04%
==========================================
Files 207 204 -3
Lines 25553 25469 -84
==========================================
- Hits 16743 16680 -63
+ Misses 7336 7319 -17
+ Partials 1474 1470 -4
|
lidel
approved these changes
Nov 10, 2023
Member
There was a problem hiding this comment.
LGTM. Removing undocumented legacy lookup is ok. People had multiple years to migrate to _dnslink, and the error message will be clear:
Error: DNSLink lookup for "google.com." failed: could not resolve name: DNSLink lookup could not find a TXT record (https://docs.ipfs.tech/concepts/dnslink/)
Added positive test for valid duplicates that we don't care about (making sure that is ignored correctly – we don't want to gatekeep DNSLink, people should be able to use it for non-IPFS namespaces if they want)
Tested it end-to-end against latest Kubo and works as expected:
$ dig +short TXT _dnslink.invalid-duplicate-dnslink-test.example.com.
"dnslink=/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR"
"dnslink=/ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi"
$ ipfs resolve /ipns/invalid-duplicate-dnslink-test.example.com
Error: DNSLink lookup for "invalid-duplicate-dnslink-test.example.com." failed: could not resolve name: DNSLink lookup returned more than one IPFS content path; ask domain owner to remove duplicate TXT records
$ dig +short TXT _dnslink.valid-duplicate-dnslink-test.example.com.
"dnslink=/foo/bar"
"dnslink=/ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi"
$ ipfs resolve /ipns/valid-duplicate-dnslink-test.example.com
/ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi
lidel
changed the title
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.
This addresses #507. I made individual commits to be easy to follow.
I removed the legacy support for DNSLink entries at the root of the domain. This made it easier to plug in the errors from the DNS. My issue was that if both
rootChandsubChreturned an error, and the error was different, which one to choose? ProbablysubCh, but that still felt fragile. I can maintain legacy support if we want it, but that will result in a bit more work.I also added two new errors to the package, to make it easier to test against specific types of errors.