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

feat(gateway): expose /routing/v1 server (opt-in) #9877

Merged
merged 14 commits into from
Aug 25, 2023
Merged

feat(gateway): expose /routing/v1 server (opt-in) #9877

merged 14 commits into from
Aug 25, 2023

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented May 12, 2023

Summary

This PR adds ability to expose Kubo's routing system as /routing/v1 on the gateway port.
This feature is disabled by default, requires opt-in via config (Gateway.ExposeRoutingAPI=true).

Closes #9875. Uses boxo/routing/http/server to implement the Routing v1 HTTP server in Kubo.

Details

@hacdias hacdias self-assigned this May 12, 2023
@hacdias hacdias force-pushed the issue/9875 branch 3 times, most recently from bf22d94 to 562d7f3 Compare May 15, 2023 09:02
core/corehttp/routing.go Outdated Show resolved Hide resolved
@hacdias hacdias marked this pull request as ready for review May 15, 2023 11:56
@hacdias hacdias requested review from lidel and a team as code owners May 15, 2023 11:56
core/corehttp/routing.go Outdated Show resolved Hide resolved
@hacdias hacdias force-pushed the issue/9875 branch 2 times, most recently from 5cec615 to a01c4f0 Compare May 29, 2023 08:08
@lidel lidel mentioned this pull request Jun 1, 2023
3 tasks
@hacdias hacdias force-pushed the issue/9875 branch 6 times, most recently from 0805ab6 to 5627805 Compare August 8, 2023 14:24
@hacdias hacdias requested a review from lidel August 8, 2023 14:41
@lidel lidel changed the title feat(gateway): expose routing v1 server via optional setting feat(gateway): expose /routing/v1 server (opt-in) Aug 21, 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.

Feedback after first pass, mainly ensuring we support legacy schemas, and avoid re-implementing PUTs in near future. Details inline.

docs/changelogs/v0.23.md Outdated Show resolved Hide resolved
docs/changelogs/v0.23.md Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
test/cli/routing_http_test.go Outdated Show resolved Hide resolved
test/cli/delegated_routing_http_test.go Show resolved Hide resolved
test/cli/delegated_routing_http_test.go Outdated Show resolved Hide resolved
test/cli/routing_http_test.go Outdated Show resolved Hide resolved
routing/delegated.go Outdated Show resolved Hide resolved
core/corehttp/routing.go Outdated Show resolved Hide resolved
@hacdias
Copy link
Member Author

hacdias commented Aug 25, 2023

@lidel I had completely missed the part of your comment on renaming get-ipns and put-ipns in the router configuration to find-ipns and provide-ipns. I do think we should solve this.

Get and Put for IPNS are historically related to it adhering to the routing API which uses routing.ValueStore, which uses Get/Put names in the code. At the same time, peer and providers have been called find and provide in other parts of the code. Therefore, that could be more consistent with remaining code, but not with the other methods of the routing v1.

I’m up to either:

  1. Change Kubo to find-ipns and provide-ipns; or
  2. Change Boxo to keep GetIPNS and PutIPNS - more consistent with Libp2p stack naming.

What do you think?

@hacdias hacdias requested a review from lidel August 25, 2023 10:49
@lidel
Copy link
Member

lidel commented Aug 25, 2023

@hacdias I'm leaning towards (2): keeping get-ipns and put-ipns in Kubo config and renaming to GetIPNS and PutIPNS here because

(Apologies for flip-flopping this so many times, not familiar with naming conventions here as much as I would like to, but (2) seems to make most sense given the information we have now)

@hacdias
Copy link
Member Author

hacdias commented Aug 25, 2023

@lidel agreed. Done both in Boxo and here. I think #9877 (comment) is the only missing thing to decide on to be able to merge Boxo and then this one.

Comment on lines +835 to +839
if cfg.Gateway.ExposeRoutingAPI.WithDefault(config.DefaultExposeRoutingAPI) {
for _, listener := range listeners {
fmt.Printf("Routing V1 API exposed at http://%s/routing/v1\n", listener.Addr())
}
}
Copy link
Member

Choose a reason for hiding this comment

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

ℹ️ we do this for WebUI URL, would not hurt here – makes it easy for user to eyeball if it is enabled, and copy URL for testing.

@hacdias hacdias enabled auto-merge (squash) August 25, 2023 15:20
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.

Thank you for including end-to-end in test/cli/delegated_routing_v1_http_proxy_test.go 👍

Since this is opt-in via Gateway.ExposeRoutingAPI, I think its good enough for giving it a try in Kubo 0.23

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Expose routing system under /routing/v1
4 participants