Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(gw): frugal DNSLink lookups on subdomains #493

Merged
merged 1 commit into from
Oct 19, 2023

Conversation

lidel
Copy link
Member

@lidel lidel commented Oct 18, 2023

This PR refactors the rushed ipfs-companion Manifest V3 fix from #462 to be more efficient and with better UX.

The old version was not very smart and wasted DNS lookup per every inlined DNSLink label (my-v--long-example-com) before retrying with un-inlined one (my.v-long.example.com).

This version is optimized to avoid unnecessary DNS lookups for both positive and negative scenarios.

It also ensures that in case of missing DNSLink, real FQDN (un-inlined domain name) is used in error path, which makes better UX AND simplifies the flow related to content blocking @hsanjuan is working on.

Before

Positive scenario

$ curl http://en-wikipedia--on--ipfs-org.ipns.localhost:8080 -H 'X-Forwarded-Proto: https'

DNS lookups made by gateway:

  1. en-wikipedia--on--ipfs-org (💢 we tried the unlikely inlined version for every name first, and it always failed)
  2. en.wikipedia-on-ipfs.org

Negative scenario (no DNSLink)

$ curl http://google-com.ipns.localhost:8080 -H 'X-Forwarded-Proto: https'
failed to resolve /ipns/google.com/: could not resolve name: _dnslink subdomain at "google-com." is missing a TXT record (https://docs.ipfs.tech/concepts/dnslink/)

💢 Note: error message is about inlined notation google-com and not the real domain google.com 👎

DNS lookups made by gateway:

  1. google-com (💢 less likely, but we checked it first)
  2. google.com (more likely, but we checked this second)

After

Positive scenario

$ curl http://google-com.ipns.localhost:8080 -H 'X-Forwarded-Proto: https'

DNS lookups made by gateway:

  1. en.wikipedia-on-ipfs.org (:green_circle: no wasted lookups)

Negative scenario (no DNSLink)

$ curl http://google-com.ipns.localhost:8080 -H 'X-Forwarded-Proto: https'
failed to resolve /ipns/google.com/: could not resolve name: _dnslink subdomain at "google.com." is missing a TXT record (https://docs.ipfs.tech/concepts/dnslink/)

🟢 Note: error message is about real domain google.com and not inlined notation google-com 👍

DNS lookups made by gateway:

  1. google.com (:green_circle: we try this first as it is more likely to have dnslink, as seen in positive scenario above)
  2. google-com (:green_circle: less likely, but we have to check to avoid false-negatives)

The old version was not very smart and wasted DNS lookup per
every inlined DNSLink label (my-v--long-example-com) before
retrying with un-inlined one (my.v-long.example.com).

This version is optimized to avoid unnecessary DNS lookup for
the golden path majority of inlined DNS names on subdomain will hit.

It also ensures that in case of missing DNSLink, the un-inlined domain
name is used in error path, which makes better UX.
@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

Merging #493 (c885582) into main (a50f784) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #493      +/-   ##
==========================================
- Coverage   65.85%   65.83%   -0.03%     
==========================================
  Files         205      205              
  Lines       25134    25152      +18     
==========================================
+ Hits        16553    16558       +5     
- Misses       7117     7131      +14     
+ Partials     1464     1463       -1     
Files Coverage Δ
gateway/hostname.go 71.21% <0.00%> (-3.33%) ⬇️

... and 9 files with indirect coverage changes

Copy link
Contributor

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

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

LGTM.

We may still want to have more specific error checking for hasDNSLinkRecord than just checking if the resolution error is nil to prevent accidentally falling through to check the non-fqdn name. However, this is lower priority since it's mostly covering edge cases in the less common scenario of non-fqdn DNSLinks

@hacdias hacdias merged commit a7e134e into main Oct 19, 2023
20 of 21 checks passed
@hacdias hacdias deleted the fix/inlined-dnslink-check-order branch October 19, 2023 09:06
@BigLep BigLep mentioned this pull request Nov 9, 2023
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants