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

IPIP-0388: Routing HTTP API Support for Querying Multiple Routers #388

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

aschmahmann
Copy link
Contributor

Adds support for querying for multiple routers via query parameter. cc @lidel @masih @guseggert

Marked as draft since it's point to the branch renaming delegated routing to routing v1

@aschmahmann aschmahmann force-pushed the feat/routing-v1-routers-param branch from e4f0cfd to ec8ea54 Compare March 20, 2023 17:44
routing/ROUTING_V1_HTTP.md Outdated Show resolved Hide resolved
Base automatically changed from feat/rename-delegated-routing to main March 30, 2023 17:34
@BigLep
Copy link
Contributor

BigLep commented Apr 19, 2023

2023-04-19 conversation:

  • Adin will check to see what is needed to get this out of draft
  • @lidel will surface to #bedrock / Rhea to ensure there is alignment on moving cid.contact to using this spec


This section contains a non-exhaustive list of known routers that may be supported by clients and servers.

- "ipfs-public-dht" - The IPFS Public DHT corresponding to the libp2p protocol ID `/ipfs/kad/1.0.0`
Copy link
Contributor

Choose a reason for hiding this comment

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

should we include / how should we name 'server's connected bitswap peers'?

Copy link
Member

@lidel lidel Jun 27, 2023

Choose a reason for hiding this comment

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

ipfs-bitswap-peers ?
I think it is ok to define some canonical names for popular things here, but the spec should be clear that it is at liberty of implementer to use other, opaque strings (for custom / future routing systems)

Copy link
Member

Choose a reason for hiding this comment

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

I've removed this list from the IPIP and added sentence that these are opaque strings controlled by implementation/instance.

routing/ROUTING_V1_HTTP.md Outdated Show resolved Hide resolved
Co-authored-by: Adin Schmahmann <[email protected]>
@lidel lidel changed the title Routing v1: Add support for multiple routers IPIP-0388: Routing v1 Support for Querying Multiple Routers Jun 27, 2023
@lidel lidel changed the title IPIP-0388: Routing v1 Support for Querying Multiple Routers IPIP-0388: Routing V1 HTTP API Support for Querying Multiple Routers Jun 27, 2023
@lidel lidel changed the title IPIP-0388: Routing V1 HTTP API Support for Querying Multiple Routers IPIP-0388: Routing HTTP API Support for Querying Multiple Routers Jun 27, 2023
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

+1, we need this, as noted in cc'd threads

GET /routing/v1/providers?routers=all (less ambiguous than *) sounds sensible, there is some UX gap around learning what routers are available, but grabby catch-all that enables all optional backend routers is already usefull.

In a spare moment this PR needs to be rebased to use specs.ipfs.tech, but for now some thoughts inline.


To select all routers use `?routers=*`. To determine the list of available routers see the `OPTIONS` API below

### `OPTIONS /routing/v1/providers`
Copy link
Member

@lidel lidel Jun 27, 2023

Choose a reason for hiding this comment

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

Some quick thoughts:

  • Learning about available routers via OPTIONS header is ok, but just for the record, it introduces some UX challenges:
    • can't paste link in address bar: need to pass custom method to curl -X OPTIONS, JS may need explicit entry in Access-Control-Expose-Headers for the value to be readable via JS.
  • Given the design decisions made when we abandoned Reframe and prioritized user-friendliness and human-readablinity of API, it may be a better UX and less headache to support listing available routers via GET and return them as JSON.

Alternative idea just to see if it resonates better:

Request: GET /routing/v1/routers (dedicated endpoint)

Response: JSON with list of items following router schema

{
  "Schema": "router",
  "Name": "ipfs-public-dht",
  "Default": false, // is it used when `?routers` is not passed
  "RoutingType": ["providers", "ipns", "peers"], // where can it be used
  "Details": { .. } // optional opaque JSON with router-type-specific values
}

Copy link
Member

Choose a reason for hiding this comment

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

Rebased this IPIP on top of latest spec, and replaced OPTIONS with GET /routing/v1/routers and a router schema above.

@BigLep BigLep mentioned this pull request Aug 3, 2023
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Rebased on top of latest /routing/v1 spec and applied changes from comments.
Main one is dedicated router schema used for returning enumeration results for GET /routing/v1/routers (and removal of HTTP OPTIONS).


This section contains a non-exhaustive list of known routers that may be supported by clients and servers.

- "ipfs-public-dht" - The IPFS Public DHT corresponding to the libp2p protocol ID `/ipfs/kad/1.0.0`
Copy link
Member

Choose a reason for hiding this comment

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

I've removed this list from the IPIP and added sentence that these are opaque strings controlled by implementation/instance.


To select all routers use `?routers=*`. To determine the list of available routers see the `OPTIONS` API below

### `OPTIONS /routing/v1/providers`
Copy link
Member

Choose a reason for hiding this comment

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

Rebased this IPIP on top of latest spec, and replaced OPTIONS with GET /routing/v1/routers and a router schema above.

@TorfinnOlsen
Copy link

added issue: ipni/storetheindex#2215 to backlog of IPNI repo cc @masih , @gammazero

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🏃 In Progress
Status: 🏃‍♀️ In Progress
Development

Successfully merging this pull request may close these issues.

6 participants