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

gateway: post-extraction GO API cleanup #61

Closed
2 tasks done
hacdias opened this issue Jan 26, 2023 · 13 comments · Fixed by #145
Closed
2 tasks done

gateway: post-extraction GO API cleanup #61

hacdias opened this issue Jan 26, 2023 · 13 comments · Fixed by #145
Labels
need/triage Needs initial labeling and prioritization topic/gateway Issues related to HTTP Gateway

Comments

@hacdias
Copy link
Member

hacdias commented Jan 26, 2023

We're currently working on extracting the Gateway code from kubo to go-libipfs (ipfs/kubo#8524). After the extraction, we should improve the code, such that it is more useable. There are currently a few points that, in my opinion, make the Gateway handler harder to use than it should:

  1. It requires more APIs than necessary.
    • From NodeAPI.Block(), we only use Get and Stat, but we are requiring the full interface (Get, Put, Rm, Stat).
    • From NodeAPI.Dag(), we only use Get, but we are requiring the full APIDagService interface, which contains a lot more functions.
  2. The handler requires both an online and offline APIs. The offline API is only used once with the Dag().Get function. Perhaps we could add an option to that method Offline: true or LocalOnly: true or NoFetch: true instead. This would allow us to only require the consumers of the Gateway code to provide one Node API.

Things to do:

@hacdias hacdias added the need/triage Needs initial labeling and prioritization label Jan 26, 2023
@lidel lidel changed the title Gateway Extraction and Improvement Gateway GO API Improvement Jan 30, 2023
@lidel
Copy link
Member

lidel commented Jan 30, 2023

cc @willscott @aschmahmann @iand

@willscott
Copy link
Contributor

when looking through this yesterday, a couple notes here as well:

  • the gateway code contains both WritableGateway and standard pathing gateway code. Being able to separate those would help make it clear that lots of the unixfs api is distinct only to the writable part.
  • The current car handler is requesting individual blocks from the Dag API, rather than the car itself (e.g. the traversal is happening locally in this gateway library) - we need to root+selector to be understandable over the API
  • It would be great to combine the path resolution logic (what is the actual root CID) with the fetch in the API rather than have them require 2 round trips of the API

@Jorropo
Copy link
Contributor

Jorropo commented Jan 31, 2023

we need to root+selector to be understandable over the API

You can use selectors here if you want but I plan to rip them out next week, reification and ADL layering is far too complex for what it's worth, they have no good multithreading support and they lead stuff like #56.

  • The current car handler is requesting individual blocks from the Dag API, rather than the car itself (e.g. the traversal is happening locally in this gateway library) - we need to root+selector to be understandable over the API
  • It would be great to combine the path resolution logic (what is the actual root CID) with the fetch in the API rather than have them require 2 round trips of the API

I already have an issue tracking this here: #139
My current plans is to merge the internal IPSL APIs and to split the IPSL compiler in a new undefined PR that have an undefined merge time.

@willscott
Copy link
Contributor

@Jorropo - i'm worried about that plan/timing.

selectors are what we have available for queries back to filecoin nodes for at least 6 months - if gateways start asking for a incompatible format we don't have support for that at either the saturn or at the SP level and it will make the current gateway transition take way longer.

@aschmahmann
Copy link
Contributor

aschmahmann commented Jan 31, 2023

@willscott: when @Jorropo says API he means "internal Go API" not "HTTP API". What happens here has no bearing on GraphSync and Selectors being available from Filecoin SPs from a Rhea/Saturn perspective.

What this API is going to look like and how we can use it to make various types of network requests is still TBD. I suspect this is the kind of thing we'll need to evaluate in a PR.

I wouldn't be surprised if the Gateway code's API was higher level than either IPSL or Selectors (closer to what the Gateway code actually needs) such that people can plug in whatever processing APIs they need. Given the current atmosphere around graph descriptors I don't see IPLD Selectors or IPSL being used over the wire for an HTTP API

@willscott
Copy link
Contributor

  • I think @hannahhoward, who is DRI for implementing the translation between whatever this HTTP API is and actually resolving what's being asked needs to be a partial owner in crafting what this API looks like.
  • There are a lot of ways that this could grow in scope, so I'm mostly looking for re-assurance that there's something not-very-ambitious that is what's being intended for the first iteration of this.

@aschmahmann
Copy link
Contributor

aschmahmann commented Jan 31, 2023

I'm happy to have Hannah involved, her feedback would certainly be welcome. Ideally any HTTP API ends up as an IPIP to the Gateway spec which is going to have broader reach and therefore broader review/feedback.

FYI my recollection of the last time we discussed this was that Hannah wanted the people working on the new gateways binary to negotiate this API with Saturn and then she would negotiate the APIs at the Saturn-L1 <-> Lassie boundary.

Note: this HTTP API is a separate issue and task from this one though.

@hannahhoward
Copy link
Contributor

hannahhoward commented Jan 31, 2023

Hey!

I'm not totally clear how this concerns me. The GET /ipfs/... HTTP endpoint call from path gateway spec as it stands is something we can convert to a selector in Lassie (even with the surprising support for non-unixfs stuff). Do we expect there to be something else coming?

Otherwise I'm not sure how this concerns our work. Tinker with your internals as you please. I don't expect to be using this code directly in Lassie, I don't think.

@hannahhoward
Copy link
Contributor

Maybe I lack context on the larger libipfs migration though? Is the idea that we might want to use this inside of Lassie?

@Jorropo
Copy link
Contributor

Jorropo commented Jan 31, 2023

Maybe I lack context on the larger libipfs migration though? Is the idea that we might want to use this inside of Lassie?

Yes but probably not all of it.
This is now used by Lotus: https://github.com/filecoin-project/lotus/blob/b8e7262b14ed45c58160a5dad2e67ee374e0ec23/go.mod#L98

@hannahhoward
Copy link
Contributor

What is IPSL?

Please let's not address "the right way to express graph queries" as a todo item here.

@Jorropo
Copy link
Contributor

Jorropo commented Jan 31, 2023

@hannahhoward a simple to use tree selection interface for new internal (go API between packages) Kubo API.
This will allow new clients (RAPIDE, ...) to receive requests for complete dags instead of many GetBlock calls.

That happen to be serializable and deserializable with some compiler but this wont get merged now, and maybe never. We don't need this for anything now.

Please let's not address "the right way to express graph queries" as a todo item here.

I don't claim it's the right way, I just claim it's easier to use and do less (which is a positive imo), while still covering almost all uses.

@lidel
Copy link
Member

lidel commented Feb 1, 2023

It is getting noisy, so to clarify:
this issue is for initial cleanup of the GO API of the go-libipfs/gateway library.

We want to remove its dependency on Kubo's internals and other legacy, but without rewriting the entire thing, so its easier to use in the bifrost-gateway binary. With that in mind, I've dropped the initial review in #145 (review) and filled #149 for subdomain support.

TBD-HTTP-API-for-fetching-partial-DAGs will be a separate issue. Currently discussing MVP set of "predefined graph queries" for existing, real world request types with Adin. When we have something worth sharing we will ping y'all on that for sure 👍 but it will be a separate issue/notion. 🙏

@lidel lidel changed the title Gateway GO API Improvement gateway: post-extraction GO API cleanup Feb 2, 2023
@lidel lidel closed this as completed in #145 Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/triage Needs initial labeling and prioritization topic/gateway Issues related to HTTP Gateway
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants