feat: recover from failed HTTP requests to third party gateways #783

colinfruit · 2019-10-06T19:06:44Z

This PR closes #640, enabling dead public gateways to be redirected to the default public gateway. Added as an experimental option, defaulting to false.

…-party gateways

colinfruit · 2019-10-06T19:09:49Z

add-on/src/lib/ipfs-request.js

+      }
+      // if error cannot be recovered via DNSLink
+      // direct the request to the public gateway
+      const recoverableViaPubGw = isRecoverableViaPubGw(request, state, ipfsPathValidator)


Including the redirect functionality was necessary in both onCompleted and onErrorOccurred, because of the different types of gateway failures. Many requests result in failure, others return 4xx or 5xx error codes.

lidel · 2019-10-11T14:22:51Z

Thank you for opening this @colinfruit!
Is it still wip, or ok to review?

colinfruit · 2019-10-12T00:11:16Z

Hi @lidel, the basic structure is fine to review. I'm working on tests, finding a clean way to stub promises (browser.tabs.query/create), and reviewing the error codes that were added, to make sure I have all that we want, and none that we don't want. Glad to hear your comments!

colinfruit · 2019-10-12T00:28:11Z

To elaborate a little more on the testing part of my work, I've been going through a couple of different libraries to see if any of them can promisify sinon-chrome. The only two that could theoretically work are then-chrome and chrome-promise. chrome-promise is no longer maintained, just bug fixes, and then-chrome is outdated and seemingly abandoned. The only other option afaik is stubbing the needed APIs manually. Has this been discussed before for this project, and is there a better way to accomplish this?

lidel · 2019-10-14T15:29:32Z

@colinfruit I stumbled upon similar problem with testing WebExtension APIs before. On top of the problem with abandoned libraries, sometimes Chromium and Firefox have slight changes in some APIs, making a generic library not feasible. In redirect tests I ended up writing bare minimim input-output/side-effect tests for modifyRequest.* methods with hardcoded input specific to Chromium and Firefox, to ensure we have regression tests for each vendor (example).

I think we should be fine with adding test/functional/lib/ipfs-request-gateway-recover.test.js with similar tests for modifyRequest.onCompleted and modifyRequest.onErrorOccurred (modelled after ipfs-request-gateway-redirect.test.js). If there are no vendor-specific APIs in onCompleted and onErrorOccurred, a single set of tests for both Firefox and Chrome will be enough.

Let me know if you got any questions, will be glad to help if I can :)

colinfruit · 2019-10-14T18:08:29Z

@lidel, that sounds good, except for the fact that it requires reworking the way the onErrorOccurred originally was written. The current setup doesn't return anything, so in my current test setup I am checking that browser.create was called with the expected arguments,

it('should redirect broken non-default public gateway IPFS request to public gateway', async function () {
      const request = url2request('https://nondefaultipfs.io/ipfs/QmYbZgeWE7y8HXkH8zbb8J9ddHQvp8LTqm6isL791eo14h', 500)
      await requestHandler.onCompleted(request)
      assert.ok(browser.tabs.create.withArgs({ url: 'https://ipfs.io/ipfs/QmYbZgeWE7y8HXkH8zbb8J9ddHQvp8LTqm6isL791eo14h', active: true, openerTabId: 12345 }).calledOnce, 'tabs.create should be called with IPFS default public gateway URL')
    })

I was under the impression that it would be bad practice to add a return value solely for the purposes of testing, but I'm happy to add them if you're OK with that?

colinfruit · 2019-10-14T21:31:52Z

@lidel just added my tests as a working example so it's easier to see what I am doing, will remove them if your previous comment is what you want. :)

colinfruit · 2019-10-14T22:02:39Z

test/functional/lib/ipfs-request-gateway-recover.test.js

+
+  before(function () {
+    global.URL = URL
+    browser.tabs = { ...browser.tabs, query: sinon.stub().resolves([{ id: 20 }]) }


Otherwise this query will throw an error.

lidel

This is great, thank you @colinfruit!

The current setup doesn't return anything, so in my current test setup I am checking that browser.create was called with the expected arguments

The way you wrote tests is absolutely correct. Most of request APIs are input-output based, but ones related to error recovery need to produce side-effect (new tab). All good.

A few final asks before we merge this:

Let's frame language and names around "recovering from failed HTTP requests". Public gateway is a way of recovering, but its just an implementation detail that may change in the future while most of this code remains. This means we want to:
- rename recoverViaPublicGateway (config option) to something like recoverFailedRequests
- rename funcitons and variables from recoverableViaPubGw to recoverable
Would it be possible to recover from broken subdomain gateway links?
- example: https://bafybeiemxf5abjwjbikoz4mc3a3dla6ual3jsgpdr4cjr3oz3evfyavhwq.ipfs.broken-example.com/wiki/ fails at DNS lookup stage, would be cool to recover from this by redirecting subdomain-based gateways to https://bafybeiemxf5abjwjbikoz4mc3a3dla6ual3jsgpdr4cjr3oz3evfyavhwq.ipfs.dweb.link/wiki/
- This is not a blocker, if you feel it requires additional work, we can add this in a separate PR.

add-on/_locales/en/messages.json

add-on/src/lib/options.js

add-on/_locales/en/messages.json

lidel · 2019-10-16T14:39:45Z

add-on/src/lib/ipfs-request.js

+        recoverableErrorCodes.has(request.statusCode)
+      if (recoverableViaPubGw) {
+        const redirectUrl = ipfsPathValidator.resolveToPublicUrl(request.url, state.pubGwURLString)
+        const redirect = { redirectUrl }


can we drop redirect and use redirectUrl directly?

I feel like this is the cleanest way to do this. Without it we would have to worry about the dnslink redirect, and do something like const redirectUrl = redirect ? redirect.redirectUrl : null to be able to use this with the same createTabWithURL function.

add-on/src/lib/ipfs-request.js

test/functional/lib/ipfs-request-gateway-recover.test.js

colinfruit · 2019-10-17T02:44:22Z

thanks for the review, @lidel, resolved most of your suggestions. It would be cool to recover broken sub-domains. Maybe a separate issue to track this would be good? I'm glad to take a look at it if I have some time.

lidel

Thank you! LGTM, excited about this!
I'll fill a new issue for subdomains shortly.

This change builds on top of #783 and: - adds support for recovery from DNS lookup failures - enables recovery for all HTTP Codes >= 400 - recovers `.eth` DNS failures bu reopening website on EthDNS gateway at `.eth.link` - simplifies some code paths and adds more tests Motivation: When a third-party IPFS gateway is discontinued or censored at DNS level, the IPFS Companion should retry request using currently active gateway set by the user (public or local). We also want to recover in situation when website with DNSLink has a valid DNS `TXT` record, but HTTP behind `A` record is down or unreachable. *Note*: right now the only real use for DNS recovery is support of .eth TLD via .eth.link gateway, however in the future this could provide means of working around DNS-based censorship (eg. by executing DNSLink lookups over libp2p as a fallback). closes #678, closes #640

colinfruit added 2 commits October 6, 2019 14:00

recover dead public gateways to public gateway url

7111845

add experimental option to enable recover of broken requests to third…

3420a55

…-party gateways

colinfruit commented Oct 6, 2019

View reviewed changes

colinfruit changed the title ~~#640 - Recover dead public gateway links~~ WIP feat: Recover dead public gateway links Oct 6, 2019

colinfruit changed the title ~~WIP feat: Recover dead public gateway links~~ [WIP] feat: Recover dead public gateway links Oct 7, 2019

add tests for request recovery via public gateway

642a8ec

colinfruit commented Oct 14, 2019

View reviewed changes

lidel requested changes Oct 16, 2019

View reviewed changes

lidel mentioned this pull request Oct 17, 2019

feat: recover from DNS failures, support DNSLink for .eth #797

Merged

3 tasks

colinfruit added 5 commits October 16, 2019 22:03

rename recoverViaPublicGateway => recoverFailedHttpRequests

05a0090

update recovery tests

7298d05

rename recoverableViaPubGw => recoverable

3a89f1e

update messages

ff43eff

update log message on recovering failed request

6d6f0cc

colinfruit changed the title ~~[WIP] feat: Recover dead public gateway links~~ feat: Recover dead public gateway links Oct 17, 2019

modify failed http request description to match style of other messages

3998391

lidel approved these changes Oct 17, 2019

View reviewed changes

lidel changed the title ~~feat: Recover dead public gateway links~~ feat: recover from failed HTTP requests to third party gateways Oct 17, 2019

lidel merged commit 614da95 into ipfs:master Oct 17, 2019

lidel mentioned this pull request Dec 5, 2019

Disable default redirect of DNSLink websites if it breaks Origin #701

Closed

github-actions bot mentioned this pull request Feb 16, 2023

chore(main): release ipfs-companion 2.23.0 #1159

Closed

github-actions bot mentioned this pull request Sep 21, 2023

chore(main): release 3.0.1 #1288

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: recover from failed HTTP requests to third party gateways #783

feat: recover from failed HTTP requests to third party gateways #783

colinfruit commented Oct 6, 2019 •

edited

Loading

colinfruit Oct 6, 2019

lidel commented Oct 11, 2019 •

edited

Loading

colinfruit commented Oct 12, 2019

colinfruit commented Oct 12, 2019 •

edited

Loading

lidel commented Oct 14, 2019 •

edited

Loading

colinfruit commented Oct 14, 2019 •

edited

Loading

colinfruit commented Oct 14, 2019

colinfruit Oct 14, 2019 •

edited

Loading

lidel left a comment •

edited

Loading

lidel Oct 16, 2019

colinfruit Oct 17, 2019 •

edited

Loading

colinfruit commented Oct 17, 2019

lidel left a comment •

edited

Loading

feat: recover from failed HTTP requests to third party gateways #783

feat: recover from failed HTTP requests to third party gateways #783

Conversation

colinfruit commented Oct 6, 2019 • edited Loading

colinfruit Oct 6, 2019

Choose a reason for hiding this comment

lidel commented Oct 11, 2019 • edited Loading

colinfruit commented Oct 12, 2019

colinfruit commented Oct 12, 2019 • edited Loading

lidel commented Oct 14, 2019 • edited Loading

colinfruit commented Oct 14, 2019 • edited Loading

colinfruit commented Oct 14, 2019

colinfruit Oct 14, 2019 • edited Loading

Choose a reason for hiding this comment

lidel left a comment • edited Loading

Choose a reason for hiding this comment

lidel Oct 16, 2019

Choose a reason for hiding this comment

colinfruit Oct 17, 2019 • edited Loading

Choose a reason for hiding this comment

colinfruit commented Oct 17, 2019

lidel left a comment • edited Loading

Choose a reason for hiding this comment

colinfruit commented Oct 6, 2019 •

edited

Loading

lidel commented Oct 11, 2019 •

edited

Loading

colinfruit commented Oct 12, 2019 •

edited

Loading

lidel commented Oct 14, 2019 •

edited

Loading

colinfruit commented Oct 14, 2019 •

edited

Loading

colinfruit Oct 14, 2019 •

edited

Loading

lidel left a comment •

edited

Loading

colinfruit Oct 17, 2019 •

edited

Loading

lidel left a comment •

edited

Loading