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.

Sign up for GitHub

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

Jump to bottom

Enable DNSLink by default #558

Merged
merged 12 commits into from
Aug 30, 2018
Merged

Enable DNSLink by default #558

merged 12 commits into from
Aug 30, 2018

Conversation

lidel
Copy link
Member

@lidel lidel commented Aug 27, 2018 •
edited

Latest update: #558 (comment)

Let's make web more robust by making DNSLink ready for mainstream use!

This PR enables efficient DNSLink support by implementing additional lookup strategy from #548.
It also makes it the default for new installs of IPFS Companion.

Iteration 2

Applied changes from #558 (comment):

TL;DR

  • Website Owner
    • publish DNS TXT record and make sure X-Ipfs-Path HTTP header is returned along with resources. The header is already returned by HTTP Gateway provided by go-ipfs, just make sure it is not filtered out.
  • Companion Extension
    • execute DNSLink async lookup on cache miss and sync if X-Ipfs-Path found in onHeadersReceived of initial main request

screenshot_18

Key Changes

The dnslink (boolean flag) is replaced by dnslinkPolicy (select list)

My current plan is to simplify language around this and rename three states for DNSLink Support:

  • disabled
  • best-effort – check cache or (this is new) async lookup in the background to populate cache
  • enabled – check cache or do sync (blocking) lookup before sending any HTTP request

Note: if "Detect X-Ipfs-Path header" is enabled (and it will be enabled by default) it will trigger blocking lookup even if best-effort is picked.

To ensure DNSLink adoption all old users will be migrated from dnslink to dnslinkPolicy=best-effort (it can be later changed it in Preferences)

See /docs/dnslink.md for "Read more" explanation of each policy.

DNS TXT Lookup Result Cache

Switched to in-memory lru-cache with size 1000 and max-age 12h.

Detect X-Ipfs-Path (HTTP Header)

Detect X-Ipfs-Path and redirect to a valid /ipfs/ path
(or /ipns/ if dnslinkPolicy=best-effort is enabled as well).

See /docs/x-ipfs-path.md for why this is useful.

Recover from HTTP Errors

Let's talk about the worst case scenario for dnslinkPolicy=best-effort

  • Visiting a new website
    • DNSLink for FQDN is not in cache
    • remote HTTP server is offline, making it impossible to check if X-Ipfs-Path is present

Companion will detect connection error via onErrorOccurred and check if website can be loaded from IPFS (forced DNSLink lookup using FQDN from HTTP URL).

If DNSLink is present in DNS it is cached (to make all following requests instant by removing HTTP transport for FQDN) and website will be opened from IPFS in a new tab (We can't redirect from onErrorOccurred).

Demo: recovering from HTTP connection timeout (httpd is offline):

screenshot_11

Other

Known Issues

Iteration 1

[Collapsed outdated notes]

TL;DR

  • Website Owner
    • publish DNS TXT record and make sure X-Ipfs-Path HTTP header is returned along with resources. The header is already returned by HTTP Gateway provided by go-ipfs, just make sure it is not filtered out.
  • Companion Extension
    • execute DNSLink lookup on cache miss if X-Ipfs-Path found in onHeadersReceived of initial main request

screenshot_10

Key Changes

The dnslink (boolean flag) is replaced by dnslinkPolicy (select list)

It accepts three states described in /docs/dnslink.md:

  • false – same role as old dnslink=false (no DNSLink redirects)
  • detectIpfsPathHeader – the new default, performs DNS TXT lookup only if X-Ipfs-Path is found in onHeadersReceived of initial request
  • eagerDnsTxtLookup - true – same role as old dnslink=true (DNS TXT lookup for every new hostname)

DNS TXT Lookup Result Cache

Switched to in-memory lru-cache with size 1000 and max-age 12h.

Detect X-Ipfs-Path (HTTP Header)

Detect X-Ipfs-Path and redirect to a valid /ipfs/ path
(or /ipns/ if dnslinkPolicy=detectIpfsPathHeader is enabled as well).

See /docs/x-ipfs-path.md for why this is useful.

Recover from HTTP Errors

Let's talk about the worst case scenario for dnslinkPolicy=detectIpfsPathHeader

  • Visiting a new website
    • DNSLink for FQDN is not in cache
    • remote HTTP server is offline, making it impossible to check if X-Ipfs-Path is present

Companion will detect connection error via onErrorOccurred and check if website can be loaded from IPFS (forced DNSLink lookup using FQDN from HTTP URL).

If DNSLink is present in DNS it is cached (to make all following requests instant by removing HTTP transport for FQDN) and website will be opened from IPFS in a new tab (We can't redirect from onErrorOccurred).

Demo: recovering from HTTP connection timeout (httpd is offline):

screenshot_11

Other

Known Issues

cc @alanshaw @olizilla @lgierth @diasdavid @kyledrake – any thoughts before merging?
(I want to merge & ship to beta channel for initial feedback and community testing)

@lidel lidel mentioned this pull request Aug 27, 2018
Closed
@lidel
fix(dnslink): try to recover only from final errors
a433684 
This change ignores most of errors
and attempts to recover via dnslink only if error is in a safe set.

- browser vendors use different error codes
  and need to be added to the safe set
- dnslink recovery ignores non-whitelisted error codes
@lidel
fix(dnslink): could not resolve name (recursion limit exceeded)
4be3504 
Context:
ipfs/kubo#4293 (comment)
@lidel lidel mentioned this pull request Aug 27, 2018
Closed
@lidel
Copy link
Member Author

lidel commented Aug 29, 2018 •
edited

I gathered some initial feedback and I feel this needs more UX polish:

  • DNSLink policy names are too vague
  • Explaining policies raises even more questions
  • Always enabling X-Ipfs-Path with DNSLink does not help
    (both should be enabled on install, then decoupled)

My current plan is to simplify language around this and rename three states for DNSLink Support:

  • disabled
  • best-effort – check cache or (this is new) async lookup in the background to populate cache
  • enabled – check cache or do sync (blocking) lookup before sending any HTTP request

Note: if "Detect X-Ipfs-Path header" is enabled (and it will be enabled by default) it will trigger blocking lookup even if best-effort is picked.

Are those terms clear enough, or could we improve language even further?

@lidel
Copy link
Member Author

lidel commented Aug 29, 2018

Update: simplified options:

"best-effort" executes preloadDnslink(url) for every request in async+throttled manner, so it is a safe default even for users running with remote APIs over HTTP:

https://github.com/ipfs-shipyard/ipfs-companion/blob/21913bfe1c7f757402c338bd518c5083e0146225/add-on/src/lib/dnslink.js#L13-L14

@olizilla
Copy link
Member

olizilla commented Aug 30, 2018 •
edited

This is a good idea. I think the names could be used to inform people of the trade off.

  • Off
  • Check after http request
  • Check before http request

If you reframe this as the user facing idea of "Load IPFS hosted sites over IPFS where possible" rather than "Enable DNSLink" then it's a no-brainer to enable it by default.

lidel added a commit that referenced this pull request Aug 30, 2018
@lidel
docs: improved DNSLink policy names
1c62253 
#558 (comment)
@lidel
Copy link
Member Author

lidel commented Aug 30, 2018 •
edited

@olizilla thank you, those are serious improvements!

I feel now most of technical users won't need to "read more" to understand what is what:

screenshot_4

We can make it less technical after the dust settles down and we move it from Experiments to a regular Preferences section.

I'm gonna fast-track this to Beta to smoke-test it in the wild.
Refinements will go into new PRs.

@lidel lidel merged commit 4b9a6d5 into master Aug 30, 2018
@lidel lidel deleted the feat/x-ipfs-path branch August 30, 2018 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olizilla olizilla Awaiting requested review from olizilla

@alanshaw alanshaw Awaiting requested review from alanshaw

@fsdiogo fsdiogo Awaiting requested review from fsdiogo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Revisiting dnslink support with X-Ipfs-Path experimental DNSLINK support doesn't work with /ipns/ path
2 participants
@lidel @olizilla