feat: update dns-over-https default resolver #111

SgtPooki · 2023-11-02T20:30:21Z

Builds on top of #55 with the following
changes:

Rebase on main (pushed to feat!: support DNS over HTTPS and DNS-JSON over HTTPS #55 as well, so not in this diff)
no more ipfs.io/api/v0/dns queries, use dns-over-http-resolver instead
pull out common resolve logic from dns-resolvers/default.ts into /utils/dns.ts as resolveFn
move browser/node resolve logic split to dns-resolvers/resolver.ts and dns-resolvers/resolver.browser.ts
defaultResolver is now isomorphic so functionality (use of recursiveResolveDnslink) is the same in browser and nodejs.

Open Questions

Do we want to error if recursion is left and we hit MAX, or do we want to return the value we got so far? Should this be configurable (e.g. return value at MAX_RECURSIVE_DEPTH or throw)? https://github.com/ipfs/helia-ipns/pull/55/files#r1380758999
- Returning the value at MAX_RECURSIVE_DEPTH would allow users to specify lower recursive values and make their own decision whether to continue to resolve dnslink entries.

This also refactors `dns-resolvers/default.ts` so that the recursive logic of the `defaultResolver` will be shared between browser and nodejs consumers.

…-https-refactor

This fixes an error seen in CI when response.Answer.filter() was called and we got an error: TypeError: Cannot read properties of undefined (reading 'filter') at findDNSLinkAnswer (file:///home/runner/work/helia-ipns/helia-ipns/packages/ipns/src/utils/dns.ts:45:34) at ipfsPath (file:///home/runner/work/helia-ipns/helia-ipns/packages/ipns/src/utils/dns.ts:29:18) at httpQueue.add.signal (file:///home/runner/work/helia-ipns/helia-ipns/packages/ipns/src/dns-resolvers/dns-json-over-https.ts:71:22)

SgtPooki

self review

SgtPooki · 2023-11-02T23:55:09Z

packages/ipns/test/dns-resolvers.spec.ts

+      const makeCall = async (): Promise<void> => {
+        await resolver('ipfs.io', {
+          onProgress: (evt) => {
+            if (evt.type.includes('dnslink:cache')) {
+              usedCache = true
+            }
          }
+        })
+      }
+
+      for (let i = 0; i < 10; i++) {
+        // resolve again, should use the cache
+        // TTL can be pretty low which means this can be flaky if executed more slowly than the TTL
+        await makeCall()
+        if (usedCache) {
+          break
        }
-      })
+      }


fix for flaky test: https://github.com/ipfs/helia-ipns/actions/runs/6738699294/job/18318866259?pr=111#step:5:60

SgtPooki · 2023-11-02T23:55:27Z

packages/ipns/test/dns-resolvers.spec.ts

+    it(`${name} should abort if signal is aborted`, async () => {
+      const signal = AbortSignal.timeout(1)
+
+      await expect(resolver('ipfs.io', { signal })).to.eventually.be.rejected()
+    })


check that abort signals are being respected (was missing in coverage)

SgtPooki · 2023-11-02T23:56:18Z

packages/ipns/src/utils/dns.ts

+  // if the result is a CID, or depth is 1, we've reached the end of the recursion
+  // if depth is 1, another recursive call will be made, but it would throw.
+  // we could return if depth is 1 and allow users to handle, but that may be a breaking change
+  if (isIPFSCID) {


removed depth === 0 because that's not possible due to above check. see #55 (comment)

SgtPooki · 2023-11-02T23:56:28Z

packages/ipns/src/utils/dns.ts

+  // result is now a `/ipfs/<cid>` or `/ipns/<cid>` string
+  const domainOrCID = result.split('/')[2] // e.g. ["", "ipfs", "<cid>"]


just some comments

SgtPooki · 2023-11-02T23:56:41Z

packages/ipns/src/utils/dns.ts

@@ -59,7 +59,7 @@ export const findTTL = (domain: string, response: DNSResponse): number => {

 export const MAX_RECURSIVE_DEPTH = 32

-export const recursiveResolveDnslink = async (domain: string, depth: number, resolve: (domain: string, options: ResolveDnsLinkOptions) => Promise<string>, options: ResolveDnsLinkOptions = {}): Promise<string> => {
+export const recursiveResolveDnslink = async (domain: string, depth: number, resolve: DNSResolver, options: ResolveDnsLinkOptions = {}): Promise<string> => {


use the defined type

SgtPooki · 2023-11-03T00:02:33Z

packages/ipns/src/index.ts

@@ -167,15 +167,22 @@ export interface DNSResolver {
  (domain: string, options?: ResolveDnsLinkOptions): Promise<string>
 }

-export interface ResolveDNSOptions extends AbortOptions, ProgressOptions<ResolveDnsLinkProgressEvents> {
+export interface ResolveDNSOptions extends AbortOptions, ProgressOptions<ResolveProgressEvents | IPNSRoutingEvents | ResolveDnsLinkProgressEvents> {


resolveDns (which takes ResolveDNSOptions) ultimately calls #resolve, which is also called by resolve (which takes ResolveOptions).

This could be cleaned up but I'll leave this for @achingbrain to decide if he wants this cleaned up further. Ultimately, #resolve should take a superset of resolve and resolveDns option types.

SgtPooki · 2023-11-03T00:04:09Z

packages/ipns/src/dns-resolvers/resolver.ts

+    options.onProgress?.(new CustomProgressEvent<string>('dnslink:query', { detail: domain }))
+    const dnslinkRecord = await resolveFn(resolver, domain)
+
+    options.onProgress?.(new CustomProgressEvent<DNSResponse>('dnslink:answer', { detail: dnslinkRecord }))


dnslink onProgress events were previously not being used in the nodejs resolve function. see

helia-ipns/packages/ipns/src/dns-resolvers/default.ts

Lines 13 to 39 in b87b9e3

async function resolve (domain: string, options: AbortOptions = {}): Promise<string> {

const resolver = new Resolver()

const listener = (): void => {

resolver.cancel()

}

options.signal?.addEventListener('abort', listener)

try {

const DNSLINK_REGEX = /^dnslink=.+$/

const records = await resolver.resolveTxt(domain)

const dnslinkRecords = records.reduce((rs, r) => rs.concat(r), [])

.filter(record => DNSLINK_REGEX.test(record))

const dnslinkRecord = dnslinkRecords[0]

// we now have dns text entries as an array of strings

// only records passing the DNSLINK_REGEX text are included

if (dnslinkRecord == null) {

throw new CodeError(`No dnslink records found for domain: ${domain}`, 'ERR_DNSLINK_NOT_FOUND')

}

return dnslinkRecord

} finally {

options.signal?.removeEventListener('abort', listener)

}

}

packages/ipns/src/dns-resolvers/resolver.browser.ts

SgtPooki · 2023-11-03T00:06:43Z

packages/ipns/src/dns-resolvers/dns-over-https.ts

      return {
        name: a.name,
        type: txtType,
        TTL: a.ttl ?? ttl,
-        // @ts-expect-error we have already checked that a.data is not empty
        data: uint8ArrayToString(a.data[0])


The TS error was not that data was empty, but that a.data[0] === string | number | Buffer

We may need more smarts around this check

around the *above check of !Buffer.isBuffer(a.data[0])

SgtPooki · 2023-11-03T00:07:31Z

packages/ipns/src/dns-resolvers/dns-over-https.ts

+    TC: response.flag_tc ?? false,
+    RD: response.flag_rd ?? false,
+    RA: response.flag_ra ?? false,
+    AD: response.flag_ad ?? false,
+    CD: response.flag_cd ?? false,


See module augmentation type in dns-packet.d.ts

SgtPooki requested review from achingbrain and a team November 2, 2023 20:33

SgtPooki mentioned this pull request Nov 2, 2023

feat!: support DNS over HTTPS and DNS-JSON over HTTPS #55

Merged

SgtPooki added 2 commits November 2, 2023 13:41

feat: remove ipfs.io dns RPC calls

8cade68

This also refactors `dns-resolvers/default.ts` so that the recursive logic of the `defaultResolver` will be shared between browser and nodejs consumers.

deps(dev): bump aegir

a35be43

SgtPooki force-pushed the feat/support-dns-over-https-refactor branch from bdb49d0 to a35be43 Compare November 2, 2023 20:42

SgtPooki added 8 commits November 2, 2023 14:02

chore: address pr #55 comments

705fe3e

Merge branch 'feat/support-dns-over-https' into feat/support-dns-over…

856cc44

…-https-refactor

chore: ignore direnv and asdf dotfiles

26d9298

chore: fix dnsPacket type expected errors

dcfeff1

fix: answer data type expected error

7019059

fix: #resolve accepts correct options type

9b961ca

test(fix): race condition due to short TTL

2b790cd

SgtPooki commented Nov 3, 2023

View reviewed changes

chore: remove old code

0d7df51

SgtPooki merged commit 1834420 into feat/support-dns-over-https Dec 5, 2023
18 checks passed

SgtPooki deleted the feat/support-dns-over-https-refactor branch December 5, 2023 21:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update dns-over-https default resolver #111

feat: update dns-over-https default resolver #111

SgtPooki commented Nov 2, 2023 •

edited

Loading

SgtPooki left a comment

SgtPooki Nov 2, 2023

SgtPooki Nov 2, 2023

SgtPooki Nov 2, 2023

SgtPooki Nov 2, 2023

SgtPooki Nov 2, 2023

SgtPooki Nov 3, 2023

SgtPooki Nov 3, 2023

SgtPooki Nov 3, 2023

SgtPooki Nov 3, 2023

SgtPooki Nov 3, 2023

		// result is now a `/ipfs/<cid>` or `/ipns/<cid>` string
		const domainOrCID = result.split('/')[2] // e.g. ["", "ipfs", "<cid>"]

	async function resolve (domain: string, options: AbortOptions = {}): Promise<string> {
	const resolver = new Resolver()
	const listener = (): void => {
	resolver.cancel()
	}

	options.signal?.addEventListener('abort', listener)

	try {
	const DNSLINK_REGEX = /^dnslink=.+$/
	const records = await resolver.resolveTxt(domain)
	const dnslinkRecords = records.reduce((rs, r) => rs.concat(r), [])
	.filter(record => DNSLINK_REGEX.test(record))

	const dnslinkRecord = dnslinkRecords[0]

	// we now have dns text entries as an array of strings
	// only records passing the DNSLINK_REGEX text are included
	if (dnslinkRecord == null) {
	throw new CodeError(`No dnslink records found for domain: ${domain}`, 'ERR_DNSLINK_NOT_FOUND')
	}

	return dnslinkRecord
	} finally {
	options.signal?.removeEventListener('abort', listener)
	}
	}

feat: update dns-over-https default resolver #111

feat: update dns-over-https default resolver #111

Conversation

SgtPooki commented Nov 2, 2023 • edited Loading

Open Questions

SgtPooki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SgtPooki commented Nov 2, 2023 •

edited

Loading