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

Implement ipni-sync http over libp2p #113

Merged
merged 29 commits into from
Aug 31, 2023
Merged

Implement ipni-sync http over libp2p #113

merged 29 commits into from
Aug 31, 2023

Conversation

gammazero
Copy link
Collaborator

@gammazero gammazero commented Aug 18, 2023

Serve ipnisync HTTP over libp2p

This PR makes use of the new libp2phttp functionality for serving and requesting ipnisync over libp2p.

The new publisher can publish ipnisync-http over libp2p if supplied with a stream host, over plain HTTP if supplied with a listen address, or both. The publisher also works as an HTTP handler to allow it to be used with existing HTTP listeners.

The sync client works with ipnisync-http over libp2p, plain HTTP, and legacy data-transfer sync. It is able to detect which protocol and transport to use with any provider, new or old.

Protocol negotiation

When a Syncer is created to sync with a specific advertisement publisher, it first uses libp2phttp to negotiate with the publisher whether to use HTTP with or without libp2p. If the publisher is an older publisher that does not support this negotiation, then the client tries to use plain HTTP or the legacy data-transfer/graphsync protocol depending on the publisher address.

If a publisher is newer, but uses an existing HTTP server, it can still support protocol negotiation by supporting the /.well-known/ HTTP endpoint. Here is an example of adding support for this. This is optional, and the cost for not supporting it is one additional HTTP round trip between the indexer and publisher per sync operation.

API Changes

This PR has some minor ipnisync API changes to be aware of.

Publisher API Changes:

  • The NewPublisher function does not take a single HTTP listen address as its first argument. It now takes a list of HTTP address as an option, WithHTTPListenAddrs, since HTTP listen addresses are not required when using a libp2p stream host or with an existing HTTP server.
  • The NewPublisher can be given a libp2p stream host to serve advertisements, by specifying the option WithStreamHost.
  • The dagsync/p2p/protocol/head package has moved to dagsync/dtsync/head since it is only used in dtsync. This should not affect any external applications.
  • The WithServer has been renamed to WithStartServer.
  • A new option, WithRequireTLS tells whether to require https or allow the publisher to serve non-secure http. Default is false, allowing non-secure HTTP.

Sync client API changes:

  • The NewSync function does not take an http.Client. Options are used to configure a default or retryable HTTP client.
  • New options are available to the NewSync function:
    • ClientAuthServerPeerID tells the sync client that it must authenticate the Server's PeerID.
    • ClientHTTPTimeout specifies a time limit for HTTP requests
    • ClientStreamHost specifies an optional stream based libp2p host
    • ClientHTTPRetry configures a retriable HTTP client
  • The NewSyncer function now takes a single peer.AddrInfo instead of a separate peer.ID and []multiaddr.Multiaddr arguments.

dagsync.Subscriber API changes:

  • NewSubscriber no longer has a HttpClient option. It now supports the following options:
    • HttpTimeout which is passed through to the the ClientHttpTimeout option
    • RetryableHTTPClient which is passed through to the ClientHTTPRetry option.

Use the new libp2phttp functionality for serving and requesting ipnisync over libp2p.
@codecov-commenter
Copy link

codecov-commenter commented Aug 18, 2023

Codecov Report

Patch coverage: 66.56% and project coverage change: +7.17% 🎉

Comparison is base (e0792fc) 54.03% compared to head (e6d63d9) 61.20%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #113      +/-   ##
==========================================
+ Coverage   54.03%   61.20%   +7.17%     
==========================================
  Files          67       57      -10     
  Lines        5341     4916     -425     
==========================================
+ Hits         2886     3009     +123     
+ Misses       2140     1581     -559     
- Partials      315      326      +11     
Files Changed Coverage Δ
announce/httpsender/sender.go 63.97% <ø> (ø)
dagsync/dtsync/head/head.go 74.22% <ø> (ø)
dagsync/dtsync/publisher.go 48.43% <ø> (ø)
dagsync/dtsync/syncer.go 81.94% <ø> (ø)
dagsync/option.go 41.40% <6.25%> (-3.82%) ⬇️
dagsync/ipnisync/sync.go 64.48% <55.65%> (-10.05%) ⬇️
dagsync/subscriber.go 67.29% <68.75%> (+0.84%) ⬆️
dagsync/ipnisync/publisher.go 67.21% <76.41%> (+5.02%) ⬆️
dagsync/test/util.go 83.13% <80.00%> (-6.26%) ⬇️
dagsync/ipnisync/option.go 89.23% <90.69%> (+1.73%) ⬆️

... and 11 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gammazero gammazero marked this pull request as ready for review August 19, 2023 03:29
@gammazero gammazero requested a review from rvagg August 19, 2023 03:44
@gammazero
Copy link
Collaborator Author

From @MarcoPolo in #103, here
I took a look at Frisbii, I think there's only one thing to add if you want to support the .well-known/libp2p endpoint. Which you probably should unless you expect every client to be able to learn about your protocols and their prefixes out-of-band (which is fine! just more work). The change is small since you already have go-libp2p as a dependency, only 4 lines of code. Here's the patch: https://gist.github.com/MarcoPolo/3fb6b479b25d8a8e43eaadc516c60c29.

- Change AsyncErr to Err in SyncFinished
- Move old p2p head client/server (legs protocol ID) into dtsync, since that is the only place it is used.
- Add tests
@gammazero gammazero requested a review from masih August 30, 2023 17:37
Copy link
Member

@masih masih left a comment

Choose a reason for hiding this comment

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

🚀

"github.com/multiformats/go-multihash"
)

const defaultHttpTimeout = 10 * time.Second
const ProtocolID = protocol.ID("/ipnisync/v1")
Copy link
Member

Choose a reason for hiding this comment

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

Too late to change this to /ipni/sync/v1?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would it be better to match the HTTP path, which is /ipni/v1/ad/ ?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure http path as libp2p protocol ID makes sense? That protocol exposes multiple paths right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only one path. As per spec the only path is /ipni/v1/ad/, which is followed by the resource head or the CID to fetch.

Copy link
Collaborator Author

@gammazero gammazero Aug 31, 2023

Choose a reason for hiding this comment

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

With the libp2phttp package, the protocol ID maps to a specific HTTP path, so it seems to make sense that they should be the same.

@MarcoPolo Recommendation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I decided to use the IPNIPath as the protocol ID. I provided an explanation in the comment here.

@gammazero gammazero merged commit eccd3d5 into main Aug 31, 2023
7 checks passed
@gammazero gammazero deleted the libp2phttp-ipnisync branch August 31, 2023 21:00
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