Add HTTP Retrieval check

[hsanjuan]

This PR adds an HTTP retrieval check (close #73)

• The multiaddresses of the peer are passed to a function that performs HTTP retrieval check for the CID
• If no multiaddresses are */http or cannot connect, the relevant errors are returned.
• If there is no support for HEAD, this is indicated
• We do not download the CID, we just perform a HEAD request and inform if the destination has the CID (200). Downloading the content opens us to a waste of bandwidth for a request that the user can perform themselves.

Related ipshipyard/roadmaps#12

[2color]

We do not download the CID, we just perform a HEAD request and inform if the destination has the CID (200). Downloading the content opens us to a waste of bandwidth for a request that the user can perform themselves.

Do Boxo/Rainbow/Kubo also try a HEAD request first before trying to download? Or just optimistically try a GET?

[hsanjuan]

Do Boxo/Rainbow/Kubo also try a HEAD request first before trying to download? Or just optimistically try a GET?

They do HEAD if the endpoint supports it.

[2color]

They do HEAD if the endpoint supports it.

How do they know if the endpoint supports without trying?

Will they try to download if the endpoint doesn't support HEAD?

[hsanjuan]

Some screenshots:

[hsanjuan]

They do HEAD if the endpoint supports it.

How do they know if the endpoint supports without trying?

Will they try to download if the endpoint doesn't support HEAD?

There is a first "Connect" step where we probe the peers/http addresses. We probe HEAD and if that doesn't work, GET. We request the identity CID. Whether the peer supports HEAD is stored in the peerstore. With that information, subsequent requests for WANT_HAVE are performed via HEAD if supported, if not, a GET requests is triggered.

In our case here, we don't trigger GET requests if HEAD is not supported (which we know after probing).

[hsanjuan]

(can be reviewed while I make tests)

[2color]

They do HEAD if the endpoint supports it.

How do they know if the endpoint supports without trying?
Will they try to download if the endpoint doesn't support HEAD?

There is a first "Connect" step where we probe the peers/http addresses. We probe HEAD and if that doesn't work, GET. We request the identity CID. Whether the peer supports HEAD is stored in the peerstore. With that information, subsequent requests for WANT_HAVE are performed via HEAD if supported, if not, a GET requests is triggered.

In our case here, we don't trigger GET requests if HEAD is not supported (which we know after probing).

From the large providers, do you know which already support HEAD?

[hsanjuan]

From the large providers, do you know which already support HEAD?

Pinata and Storacha do for sure.

[lidel]

@hsanjuan Will this be how we show that the endpoint is missing HTTPS or that TLS certificate validation failed?

We need to surface that in a clear way, to help storage providers fix their setups without bothering us for analysis.

[hsanjuan]

We don't care about certificate validity, but we do care about http2 (which doesn't get setup unless certificate is valid I think).

In that case, when no http/2 present, we would return an error which would be displayed here.

However we return a standard "no valid addresses" error. We would need to be more specific regarding the error, but that needs to be fixed in boxo.

[aschmahmann]

We don't care about certificate validity

I might be missing something but why not? Without valid certs you won't be accessible from a browser or by default from boxo, kubo, etc. so why wouldn't we care?

[hsanjuan]

hmm ok the browser, that thing... so we do care, sorry.

[2color]

When running a check with an HTTPS maddr I consistently get a 500 with context deadline exceeded.

cid: bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi
multiaddr: /dns4/dag.w3s.link/tcp/443/https/p2p/QmUA9D3H7HeCYsirB3KmPSvZh3dNXMZas6Lwgr4fv1HTTp
ipniIndexer: https://cid.contact
timeoutSeconds: 60
httpRetrieval: on

[hsanjuan]

When running a check with an HTTPS maddr I consistently get a 500 with context deadline exceeded.

cid: bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi
multiaddr: /dns4/dag.w3s.link/tcp/443/https/p2p/QmUA9D3H7HeCYsirB3KmPSvZh3dNXMZas6Lwgr4fv1HTTp
ipniIndexer: https://cid.contact
timeoutSeconds: 60
httpRetrieval: on

This is an issue with the existing code: it will timeout on DHT lookups and fail everything that comes after, including testing bitswap providers found via IPNI. In production the issue is not apparent because AcceleratedDHT is true, and the DHT lookup finishes quickly within the timeout. Fixing that warrants a bit of refactoring, merging of the two check paths (with and without one given multiaddr, it run the same checks) etc.

[2color]

This is an issue with the existing code: it will timeout on DHT lookups and fail everything that comes after, including testing bitswap providers found via IPNI. In production the issue is not apparent because AcceleratedDHT is true, and the DHT lookup finishes quickly within the timeout. Fixing that warrants a bit of refactoring, merging of the two check paths (with and without one given multiaddr, it run the same checks) etc.

It's strange that the with one maddr + cid check would fail because of a DHT lookup, causing it to not achieve it's main goal of checking retrievability from the peer. I just pushed 8745dff (feel free to revert) thinking we can reduce the chances of a timeout by doing more work concurrently (though it didn't help with this).

What do you think about only performing the DHT checks after or while we check HTTP retrievability (the primary purpose of this check), since it's not dependent on the results from the DHT/IPNI lookups, and is likely a cheaper check to run anyways. This would result in a useful result, even if we don't have the results for ProviderRecordFromPeerInDHT, ProviderRecordFromPeerInIPNI, PeerFoundInDHT (we'd need to adjust for a more graceful failure).

[hsanjuan]

probably 8745dff doesn't fix it. As the idea is also that DHT addresses are used to test retrieability later iirc. And I think this will still block.

My opinion is that:

• things work in production due to accelerated dht
• no need to make the code flow more complicated than it is and we can leave things as they are.
• these issues warrant a refactor, and if that happens, it is worth doing a big refactor so that the same flow is followed in all checks, and the given peer address is just a hint that gets added to everything else discovered.

[hsanjuan]

Otherwise:

• HTTP check can be done in parallel to the rest.
• DHT lookup should have its own timeout, so that other checks have time to proceed.

[2color]

I think that this working in production or with the accelerated DHT client ins't good reason to not fix this as part of this PR.

I don't have a strong opinion on how, but adding a separate timeout for the DHT lookups seems reasonable, given that for routable peers/cids, we should be able to get a response under 5 seconds. If we don't find it in that time window, the chances are pretty slim that waiting out the default 60 second timeout is going to yield any results.

Also, we don't even support HTTP providers yet in the DHT, so we're doing all of this work right now, blocking on the main purpose of this check.

[2color]

LGTM.

I pushed two fixes:

• Add separate timeout for dht peer lookup in peer-specific check
• make sure that when we don't filter out http providers from IPNI, when doing a peer-specific check (so that we can correctly render whether an http provider was found in the IPNI)