-
Notifications
You must be signed in to change notification settings - Fork 232
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-425: Signaling Features on HTTP Gateways #425
base: main
Are you sure you want to change the base?
Conversation
lidel
commented
Jul 6, 2023
•
edited by hacdias
Loading
edited by hacdias
- merge IPIP-402 and IPIP-412 first (in order), this PR is based on top of them
- gateway conformance test
Initial version to kick-off discussion
3a0785a
to
0eacf6a
Compare
- `trustless-gateway` for :cite[trustless-gateway] | ||
- `path-proof` indicates support for returning parent blocks up to the terminus element | ||
- `car-version=1|2` indicates CAR support | ||
- `dag-scope=block|entity|all` from :cite[ipip-0402] | ||
- `entity-bytes` from :cite[ipip-0402], implies support for `dag-scope=entity` as well |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hannahhoward @alanshaw @olizilla I know you want a way for signaling support for partial cars and range requests between Lassie and .storage to avoid expensive feature-sniffing.
With this IPIP, signaling support for partial cars WITHOUT range requests woudl look like this:
Ipfs-Gateway-Features: dag-scope=block|entity|all
and if you add support for entity-bytes
at some point:
Ipfs-Gateway-Features: dag-scope=block|entity|all, entity-bytes
<!-- TODO do we want these too? | ||
- `multibase` list of prefixes, indicates which multibase encoding are supported in CIDs | ||
- `multihash` indicates which hash functions are supported in CIDs | ||
- `multicodec` indicates which codecs are supported in CIDs | ||
--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓
Thoughts on adding these or even more?
The need for list of supported IPLD codecs was raised by ipfs-chromium and Capyloon in #402 (comment), having the other two would not hurt too.
My only constraint would be to use numeric (decimal or hex) codes, to avoid problems if we ever do this again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @aschmahmann as you noted in 402:
Not specifically related to the trustless gateway, but if we're adding OPTIONS support we might want to be able to discover things like supported hash functions and IPLD codecs.
Would below work? is there a better way of representing this?
multihash=0x12|0x13|etc # identity + goodset from https://github.com/ipfs/boxo/blob/cfad09d7156efa2f09822d620cacb2423d884067/verifcid/validate.go#L17
multicodec=0x51|0x55|0x70|0x71|0x72|0x0129|0x0200 # based on boxo/gateway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
too much information, let's start simple for now
|
||
### Alternatives | ||
|
||
- Exposing the list of suported features via `GET |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that using OPTIONS
here makes more sense for this use case than using .well-known
. My argument is that this the HTTP Gateway API is an application-level protocol, and these are features for this application. .well-known
on the other hand is for metadata about an origin, not a specific application.
This is also why libp2p/specs#508 uses .well-known
for metadata on where application protocols are mounted. This is metadata about the origin.
- `dnslink-gateway` for :cite[dnslink-gateway] support based on `Host` header | ||
- `ipns` indicating :cite[ipns-record] support on `/ipns/` content paths | ||
- `dnslink` indicating [DNSLink](https://dnslink.dev) support on `/ipns/` content paths | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thing that could be useful is max block size:
- `max-block-size` to indicate what is the biggest block that can be retrieved by this gateway (default: 1MiB? 2MiB? -- whatever we currently have in boxo/gateway) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for writing this up. Makes sense and seems like a great step forward for letting nodes announce the functionality they support.
Rather than embedding all of this in the header, is there any precedent for putting it into a body with JSON? (Just was wondering in case easier to work with and avoids any max header length situations?)
that checks if the CORS protocol is understood and a server is aware using | ||
specific methods and headers. | ||
|
||
The `OPTIONS` request if often sent by web browser anyway, so we would not be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this oten a heavily cachable response so we'd be hitting the browser cache.
- requires additional HTTP GET, while in some cases HTTP OPTIONS is already | ||
sent (e.g., [browser's preflight | ||
requests](https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#preflighted_requests)), | ||
and would not introduce no overhead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(suggestion not working for me for some reason)
"and would not introduce overhead"
`.well-known` we need to make multiple GET requests for different | ||
`.well-known/..` paths, vs sending a single HTTP OPTIONS and getting all | ||
headers related to pre-existing path | ||
- HTTP headers can be added as the rrqueest passes via reverse proxies, CDNs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
request
- `dnslink-gateway` for :cite[dnslink-gateway] support based on `Host` header | ||
- `ipns` indicating :cite[ipns-record] support on `/ipns/` content paths | ||
- `dnslink` indicating [DNSLink](https://dnslink.dev) support on `/ipns/` content paths | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thing we could signal: if gateway is recursive (fetching content if not available locally) or not (cc @aschmahmann @Jorropo)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea but hold off until we have a real use-case for it reckon - unless you can conjure one now?
@@ -542,6 +542,46 @@ Optional, present in certain response types: | |||
non-executable binary response types are not used in `<script>` and `<style>` | |||
HTML tags. | |||
|
|||
### `Ipfs-Gateway-Features` (response header) | |||
|
|||
Optional, this header SHOULD be only returned in response to HTTP `OPTIONS` request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(just tuning into this PR now that I care about signalling, sorry!)
Whatever this header is, I think it should also be returned when a server doesn't implement a feature requested, either via Accept
header or other means and in that case the server should return a 415
. That way I can parse the list of Accept
content types, and be strict about the parameters, and if I don't find one that I fully support, return a 415
with this signal that says what I do support.
That way, a client can optimistically make a request, but be prepared to downgrade if it doesn't get what it wants. Rather than needing to OPTIONS
first, thereby saving a round-trip.
|
||
- `trustless-gateway` for :cite[trustless-gateway] | ||
- `path-proof` indicates support for returning parent blocks up to the terminus element | ||
- `car-version=1|2` indicates CAR support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- `car-version=1|2` indicates CAR support | |
- `car-version=1` indicates CAR support |
we should strictly do v1 for now, v2 doesn't make sense for this (yet), but it might be worth signalling this so we can also add 3
etc. when we get to it
- `dag-scope=block|entity|all` from :cite[ipip-0402] | ||
- `entity-bytes` from :cite[ipip-0402], implies support for `dag-scope=entity` as well | ||
- `car-block-order=dfs` from :cite[ipip-0412] | ||
- `car-block-dupes=y|n` from :cite[ipip-0412] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- `car-block-dupes=y|n` from :cite[ipip-0412] | |
- `dups=y|n` from :cite[ipip-0412] |
- `car-version=1|2` indicates CAR support | ||
- `dag-scope=block|entity|all` from :cite[ipip-0402] | ||
- `entity-bytes` from :cite[ipip-0402], implies support for `dag-scope=entity` as well | ||
- `car-block-order=dfs` from :cite[ipip-0412] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- `car-block-order=dfs` from :cite[ipip-0412] | |
- `order=dfs` from :cite[ipip-0412] |
I ~like this, but would rather its feature set be somewhat limited within the bounds of 412 plus anything else we can come up with a legitimate use-case for signalling. I don't really buy the multicodec thing yet—I know why it's a problem, but how does signalling it help at this stage? The tighter we scope it the easier it is to agree and ship, and I'd really like to turn this "ignore" into an error so evolution is easier: https://github.com/ipld/go-trustless-utils/blob/27d979b92be45fa189c9bef4a351e5cb02d95285/http/parse.go#L187-L189 |
My original thought for this was to just
But I also suppose there's things in here that are not content-type related. |
- `trustless-gateway` for :cite[trustless-gateway] | ||
- `path-proof` indicates support for returning parent blocks up to the terminus element |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- `trustless-gateway` for :cite[trustless-gateway] | |
- `path-proof` indicates support for returning parent blocks up to the terminus element | |
- `trustless-gateway` for :cite[trustless-gateway] | |
- `formats=car|raw` indicates whether CAR and/or raw block responses are supported | |
- `path-proof` indicates support for returning parent blocks up to the terminus element |