Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

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

Merged
merged 16 commits into from
Dec 5, 2023

Conversation

achingbrain
Copy link
Member

Adds support for resoving DNSLink TXT entries from public DNS-Over-HTTPS servers (RFC 1035) and also DNS-JSON-Over-HTTPS since they are a bit kinder on the resulting browser bundle size.

Fixes #53

Adds support for resoving DNSLink TXT entries from public
DNS-Over-HTTPS servers (RFC 1035) and also DNS-JSON-Over-HTTPS
since they are a bit kinder on the resulting browser bundle size.

Fixes #53
@achingbrain achingbrain requested a review from lidel May 31, 2023 13:09
@achingbrain achingbrain mentioned this pull request May 31, 2023
15 tasks
@achingbrain achingbrain requested a review from a team May 31, 2023 16:47
Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments and questions, some minor changes (mostly more jsdoc) would improve things a bit.

packages/ipns/src/dns-resolvers/default.browser.ts Outdated Show resolved Hide resolved
return answer.TTL
}

export const MAX_RECURSIVE_DEPTH = 32
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be configurable?

packages/ipns/src/index.ts Outdated Show resolved Hide resolved
(domain: string, options?: ResolveDnsLinkOptions): Promise<string>
}

export interface ResolveDNSOptions extends AbortOptions, ProgressOptions<ResolveDnsLinkProgressEvents> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add MAX_RECURSIVE_DEPTH as a configurable option in here?

packages/ipns/src/utils/dns.ts Show resolved Hide resolved
packages/ipns/src/dns-resolvers/index.ts Show resolved Hide resolved
const txtType = 16

return {
Status: 0,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the status field? If so, what for? it's not part of rfc8427 nor rfc1035

packages/ipns/src/dns-resolvers/dns-over-https.ts Outdated Show resolved Hide resolved
packages/ipns/src/dns-resolvers/dns-over-https.ts Outdated Show resolved Hide resolved
Copy link

@whizzzkid whizzzkid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits

packages/ipns/src/dns-resolvers/default.browser.ts Outdated Show resolved Hide resolved
packages/ipns/src/dns-resolvers/default.browser.ts Outdated Show resolved Hide resolved
packages/ipns/src/dns-resolvers/default.ts Show resolved Hide resolved
packages/ipns/src/dns-resolvers/default.ts Outdated Show resolved Hide resolved
packages/ipns/src/dns-resolvers/default.ts Outdated Show resolved Hide resolved
packages/ipns/src/utils/dns.ts Outdated Show resolved Hide resolved
dnslinkRecord = await resolve(domain, options)
}

const result = dnslinkRecord.replace('dnslink=', '')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can return this here if depth === 0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically, depth inside this scope was already checked, and the depth===0 at the top would have checked if it was 0. The check below on line 94 is a duplicate and should never fire. So we should technically check if depth is 1, and we can return if we want to prevent erroring on further recursion to allow a user to do so if they wish?

@achingbrain what do you think

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it really depends on whether we want to allow infinite recursion and error if infinite recursion exists, or recurse as far as MAX and return what we have so far (user could then request to resolve that further if they want)

could be a config option.

packages/ipns/src/utils/dns.ts Outdated Show resolved Hide resolved
SgtPooki added a commit that referenced this pull request Nov 2, 2023
@wesbiggs
Copy link

Feature request: ability to "set and forget", i.e. provide an array of resolvers as the default option for all subsequent calls; check if this is present before falling back to defaultResolver. I think this is a common usage pattern and it would save the developer the trouble of having to pass the resolver or array of resolvers around. lmk if you want a PR on your branch.

@SgtPooki
Copy link
Member

SgtPooki commented Dec 5, 2023

Feature request: ability to "set and forget", i.e. provide an array of resolvers as the default option for all subsequent calls; check if this is present before falling back to defaultResolver. I think this is a common usage pattern and it would save the developer the trouble of having to pass the resolver or array of resolvers around. lmk if you want a PR on your branch.

see cac6ba9

@SgtPooki SgtPooki changed the title feat: support DNS over HTTPS and DNS-JSON over HTTPS feat!: support DNS over HTTPS and DNS-JSON over HTTPS Dec 5, 2023
@SgtPooki SgtPooki merged commit 2ac0e8b into main Dec 5, 2023
18 checks passed
@SgtPooki SgtPooki deleted the feat/support-dns-over-https branch December 5, 2023 23:19
github-actions bot pushed a commit that referenced this pull request Dec 5, 2023
## [@helia/ipns-v3.0.0](https://github.com/ipfs/helia-ipns/compare/@helia/ipns-v2.0.3...@helia/ipns-v3.0.0) (2023-12-05)

### ⚠ BREAKING CHANGES

* alters the options object passed to the `ipns` factory function

### Features

* support DNS over HTTPS and DNS-JSON over HTTPS ([#55](#55)) ([2ac0e8b](2ac0e8b))
@@ -60,7 +60,6 @@
"@libp2p/peer-id-factory": "^3.0.3",
"@libp2p/tcp": "^8.0.4",
"@libp2p/websockets": "^7.0.4",
"aegir": "^41.0.0",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be left in place. Each package in a monorepo should declare all of it's dependencies, otherwise we have some deps here, some deps there, and then it becomes hard to reason about where things are coming from and why.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Support DNS over HTTPS
6 participants