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: add peer block filter option #8772

Closed
wants to merge 2 commits into from

Conversation

laurentsenta
Copy link
Contributor

This feature lets a user configure a filter to allow / deny bitswap requests according to the request's peer ID and the content id. For example, a user may use this option to implement a dynamic peer-based authorization.

Contributes to #8763,
Relates to ipfs/go-bitswap#549

This PR adds the plumbing required to pass the peer-block filter option from ipfs down to go-bitswap.

  • confirm with the team the way we pass the option is the right approach,
  • confirm with the team where we want to document this,
    • we could advertise this as an experimental feature,
    • provide an example / demo of the feature in the docs/examples folder (like in this demo)

@laurentsenta laurentsenta marked this pull request as draft March 8, 2022 12:54
@@ -40,6 +41,9 @@ type BuildCfg struct {
Routing libp2p.RoutingOption
Host libp2p.HostOption
Repo repo.Repo

// Bitswap
PeerBlockRequestFilter bitswap.PeerBlockRequestFilter
Copy link
Contributor Author

@laurentsenta laurentsenta Mar 8, 2022

Choose a reason for hiding this comment

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

I'm not sure which we prefer:

  1. Pass the PeerBlockRequestFilter function, shown here,
  2. Pass a generic bitswap.Option function.

(2) is more "obviously future-proof", but that'd mean we'd have two different ways to configure most values in the bitswap instance (json from cfg and code from buildcfg).

Access control is probably a cross-cutting concern that will eventually span multiple modules and protocols, not only bitswap. So by decoupling the access control config from the bitswap option, (1) might make it easier to build upon this feature.

@laurentsenta laurentsenta marked this pull request as ready for review March 8, 2022 16:06
@laurentsenta laurentsenta requested a review from aschmahmann March 8, 2022 16:06
@BigLep BigLep added this to the Best Effort Track milestone Mar 10, 2022
Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

Your code doesn't build:

core/node/bitswap.go:27:60: undefined: bitswap.PeerBlockRequestFilter
core/node/bitswap.go:45:24: undefined: bitswap.WithPeerBlockRequestFilter
core/node/builder.go:46:25: undefined: bitswap.PeerBlockRequestFilter

I guess you are waiting on a new version of bitswap with your changes merged.
I don't know if maintainers have feelings about this, but I like to create a temporary replace rule that I let maintainers remove (with the "maintainers are allowed to push to this branch" or alike option).
You can do this by running:

go mod edit -replace <target repo>=<your fork>@<your commit>

For example:

go mod edit -replace github.com/alanshaw/go-carbites=github.com/Jorropo/go-carbites@751dcbf09c8a387030238e4bd3169cc2516046f2

You can also add a // see <link to the blocking pull request> comment on the replace line in the go.mod.

That allows the CI tests to run and shows that your code is RFM (blocking PR aside).

Comment on lines +284 to +287
pbrf := bcfg.PeerBlockRequestFilter

return fx.Options(
fx.Provide(OnlineExchange(cfg, shouldBitswapProvide)),
fx.Provide(OnlineExchange(cfg, shouldBitswapProvide, pbrf)),
Copy link
Contributor

@Jorropo Jorropo Mar 10, 2022

Choose a reason for hiding this comment

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

I belive you inspired your pbrf variable from the line above, this code (shouldBitswapProvide) is a clarification variable, it translate !cfg.Experimental.StrategicProviding into a usefull thing we understand.
In your case you are turning a perfectly good bitswapConfig.PeerBlockRequestFilter into a meaningless without prior knowledge pbrf one.

Suggested change
pbrf := bcfg.PeerBlockRequestFilter
return fx.Options(
fx.Provide(OnlineExchange(cfg, shouldBitswapProvide)),
fx.Provide(OnlineExchange(cfg, shouldBitswapProvide, pbrf)),
return fx.Options(
fx.Provide(OnlineExchange(cfg, shouldBitswapProvide, bcfg.PeerBlockRequestFilter)),

@laurentsenta
Copy link
Contributor Author

Closing this after a sync chat with Adin, this might come back as a plugin later.

Thanks for the initial review @Jorropo, I won't update the PR but I'll keep your feedback in mind for future PRs.

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.

3 participants