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: no implicit filtering of proxied IPNI results #85

Merged
merged 5 commits into from
Oct 30, 2024
Merged

Conversation

lidel
Copy link
Member

@lidel lidel commented Oct 30, 2024

Problem

Because we don't have HTTP retrieval client in golang (boxo) yet, the DefaultProtocolFilter in boxo/routing/http/client does not include transport-ipfs-gateway-http yet.

This had unintended consequence at delegated-ipfs.dev because someguy as a proxy CLIENT to https://cid.contact/routing/v1, was running with default filters and as a client, was filtering transport-ipfs-gateway-http out.

Then, when acting as delegated routing SERVER, it no longer had them, which made these results unavailable to JS clients, even when they DO have HTTP support (e.g. SW gateway at https://inbrowser.link), even if they included it in explicit ?filter-protocols=unknown,transport-bitswap,transport-ipfs-gateway-http queries sent to delegated-ipfs.dev.

cc @2color @SgtPooki for visibility

Fix

This PR overrides implicit delegated routing CLIENT filters with explicit empty one.

This restores those IPNI results (from cid.contact), allowing clients that need them, to get them.

(clients that that do not need them, can still skip them by passing explicit filter-protocol list without this protocol)

Before (v0.5.2)

$ curl 'https://delegated-ipfs.dev/routing/v1/providers/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi' -H 'Accept: application/x-ndjson' -s | grep transport-ipfs-gateway-http
...

(no transport-ipfs-gateway-http results)

After (this PR, once merged will be v0.5.3)

$ go run . start
$ curl 'http://127.0.0.1:8190/routing/v1/providers/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi' -H 'Accept: application/x-ndjson' -s | grep transport-ipfs-gateway-http
{"Addrs":["/dns4/dag.w3s.link/tcp/443/https"],"ID":"QmUA9D3H7HeCYsirB3KmPSvZh3dNXMZas6Lwgr4fv1HTTp","Protocols":["transport-ipfs-gateway-http"],"Schema":"peer","transport-ipfs-gateway-http":"oBIA"}
{"Addrs":["/ip4/212.6.53.27/tcp/80/http"],"ID":"12D3KooWHEzPJNmo4shWendFFrxDNttYf8DW4eLC7M2JzuXHC1hE","Protocols":["transport-ipfs-gateway-http"],"Schema":"peer","transport-ipfs-gateway-http":"oBIA"}
{"Addrs":["/ip4/212.6.53.28/tcp/80/http"],"ID":"12D3KooWJ8YAF6DiRxrzcxoeUVjSANYxyxU55ruFgNvQB4EHibpG","Protocols":["transport-ipfs-gateway-http"],"Schema":"peer","transport-ipfs-gateway-http":"oBIA"}

Since we don't have HTTP retrieval client in golang (boxo) yet,
the default filter in boxo/routing/http/client
does not include transport-ipfs-gateway-http yet, which means someguy
as a proxy was filtering them out, making them unavailable to JS clients
that DO have HTTP support (e.g. SW gateway at inbrowser.link).

This overrides implicit filters with explicit ones that include
transport-ipfs-gateway-http restoring those IPNI results
from cid.contact.
@lidel lidel requested a review from 2color October 30, 2024 00:36
CHANGELOG.md Outdated Show resolved Hide resolved
server.go Outdated
Comment on lines 220 to 224
drclient.WithProtocolFilter([]string{
"unknown", // allow results without protocol list, allowing end user to do libp2p Identify probe to test them
"transport-bitswap",
"transport-ipfs-gateway-http",
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

For what someguy is doing (i.e. serving as a proxy) why not let all protocol through rather than limiting them?

e.g. why shouldn't GraphSync peers, or those from some new protocol show up here without being explicitly added?

Given that programmatic clients that want to use the data for content routing directly (e.g. helia) will send their own filters to be layered on top pre-filtering the data doesn't seem necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point about no filtering being a better default for someguy and ecosystem.
Disabled filtering in a2abe25.

But I've filled #86 to add support for setting implicit filters for deployments that would benefit from reduced default response sizes.

Copy link

Suggested version: 0.5.3

Comparing to: v0.5.2 (diff)

Changes in configuration file(s):

(empty)

gorelease says:

# diagnostics
required module github.com/gabriel-vasile/[email protected] retracted by module author: v1.4.4 had a test file detected as malicious by antivirus software. #575

# summary
Suggested version: v0.5.3

gocompat says:

HEAD is now at 94cc5f7 chore: release v0.5.2
Switched to branch 'main'
Your branch is up to date with 'origin/main'.

Cutting a Release (and modifying non-markdown files)

This PR is modifying both version.json and non-markdown files.
The Release Checker is not able to analyse files that are not checked in to main. This might cause the above analysis to be inaccurate.
Please consider performing all the code changes in a separate PR before cutting the release.

Automatically created GitHub Release

A draft GitHub Release has been created.
It is going to be published when this PR is merged.
You can modify its' body to include any release notes you wish to include with the release.

@lidel lidel changed the title fix: include transport-ipfs-gateway-http from IPNI fix: no implicit filtering of proxied IPNI results Oct 30, 2024
@lidel lidel merged commit 4903832 into main Oct 30, 2024
11 checks passed
@lidel lidel deleted the include-http-peers branch October 30, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants