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: basic support for Ipfs-Path-Affinity from IPIP-462 #592

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lidel
Copy link
Member

@lidel lidel commented Mar 22, 2024

This PR aims to kick-off discussion how we would support IPIP-462 in boxo/gateway.

For more info and header semantics, see ipfs/specs#462

proposed scope

What I want to do for now is to add minimal code to start leveraging Ipfs-Path-Affinity hints within existing boxo/gateway codebase, so we cna deploy it to our gateways and allow clients like service-worker-gateway or ipfs-chromium to pass hint and retrieve content even when internal CID was not announced directly.

  • support percent-encoded values
  • limit to 3 hints per request
  • cancel work if original request finished
  • skip work that would not produce any benefit in routing (if path from hint is already cached it won't trigger any additional provider lookups)

future scope

We may formalize this by extending IPFSBackend with explicit place to pass this hint, and then wire it up into routing system of Kubo, but it feels more involved and would like to do that in follow-up, rather than block on it here.

lidel added 2 commits March 22, 2024 19:24
This is first stab at leveraging these hints withing existing
boxo/gateway codebase.

It is pretty blunt, but will enable smart clients fetching sub-DAGs
to work around any content routing gaps

For more info and header semantics see ipfs/specs#462
// Confirm it is a valid content path
affinityPath, err := path.NewPath(headerValue)
if err != nil {
logger.Debugw("skipping invalid Ipfs-Path-Affinity hint", "error", err)

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

Sensitive data returned by HTTP request headers
flows to a logging call.
Sensitive data returned by HTTP request headers
flows to a logging call.
// Skip duplicated work if immutable affinity hint is a subset of requested immutable contentPath
// (protect against broken clients that use affinity incorrectly)
if !contentPath.Mutable() && !affinityPath.Mutable() && strings.HasPrefix(contentPath.String(), affinityPath.String()) {
logger.Debugw("skipping redundant Ipfs-Path-Affinity hint", "affinity", affinityPath)

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

Sensitive data returned by HTTP request headers
flows to a logging call.
Sensitive data returned by HTTP request headers
flows to a logging call.
// Process hint in background without blocking response logic for contentPath
go func(contentPath path.Path, affinityPath path.Path, logger *zap.SugaredLogger) {
var immutableAffinityPath path.ImmutablePath
logger.Debugw("async processing of Ipfs-Path-Affinity hint", "affinity", affinityPath)

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

Sensitive data returned by HTTP request headers
flows to a logging call.
Sensitive data returned by HTTP request headers
flows to a logging call.
if affinityPath.Mutable() {
// Skip work if mutable affinity hint is a subset of mutable contentPath
if contentPath.Mutable() && strings.HasPrefix(contentPath.String(), affinityPath.String()) {
logger.Debugw("skipping redundant Ipfs-Path-Affinity hint", "affinity", affinityPath)

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

Sensitive data returned by HTTP request headers
flows to a logging call.
Sensitive data returned by HTTP request headers
flows to a logging call.
}
immutableAffinityPath, _, _, err = i.backend.ResolveMutable(r.Context(), affinityPath)
if err != nil {
logger.Debugw("error while resolving mutable Ipfs-Path-Affinity hint", "affinity", affinityPath, "error", err)

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

Sensitive data returned by HTTP request headers
flows to a logging call.
Sensitive data returned by HTTP request headers
flows to a logging call.
}
immutableAffinityPath, _, _, err = i.backend.ResolveMutable(r.Context(), affinityPath)
if err != nil {
logger.Debugw("error while resolving mutable Ipfs-Path-Affinity hint", "affinity", affinityPath, "error", err)

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

Sensitive data returned by HTTP request headers
flows to a logging call.
Sensitive data returned by HTTP request headers
flows to a logging call.
// original contentPath was received and returned to HTTP
// client before below get is done, the work is cancelled.

logger.Debugw("started async search for providers of Ipfs-Path-Affinity hint", "affinity", affinityPath)

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

Sensitive data returned by HTTP request headers
flows to a logging call.
Sensitive data returned by HTTP request headers
flows to a logging call.

logger.Debugw("started async search for providers of Ipfs-Path-Affinity hint", "affinity", affinityPath)
_, _, err = i.backend.GetBlock(r.Context(), immutableAffinityPath)
logger.Debugw("ended async search for providers of Ipfs-Path-Affinity hint", "affinity", affinityPath, "error", err)

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

Sensitive data returned by HTTP request headers
flows to a logging call.
Sensitive data returned by HTTP request headers
flows to a logging call.

logger.Debugw("started async search for providers of Ipfs-Path-Affinity hint", "affinity", affinityPath)
_, _, err = i.backend.GetBlock(r.Context(), immutableAffinityPath)
logger.Debugw("ended async search for providers of Ipfs-Path-Affinity hint", "affinity", affinityPath, "error", err)

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

Sensitive data returned by HTTP request headers
flows to a logging call.
Sensitive data returned by HTTP request headers
flows to a logging call.
_, _, err = i.backend.GetBlock(r.Context(), immutableAffinityPath)
logger.Debugw("ended async search for providers of Ipfs-Path-Affinity hint", "affinity", affinityPath, "error", err)
} else {
logger.Debugw("skipping Ipfs-Path-Affinity hint due to data being locally cached", "affinity", affinityPath)

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

Sensitive data returned by HTTP request headers
flows to a logging call.
Sensitive data returned by HTTP request headers
flows to a logging call.
Copy link

codecov bot commented Mar 23, 2024

Codecov Report

Attention: Patch coverage is 11.11111% with 64 lines in your changes are missing coverage. Please review.

Project coverage is 59.69%. Comparing base (b101ba0) to head (39a8bd7).

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #592      +/-   ##
==========================================
- Coverage   59.78%   59.69%   -0.10%     
==========================================
  Files         232      232              
  Lines       28124    28197      +73     
==========================================
+ Hits        16815    16833      +18     
- Misses       9837     9892      +55     
  Partials     1472     1472              
Files Coverage Δ
gateway/handler.go 70.63% <11.11%> (-7.65%) ⬇️

... and 15 files with indirect coverage changes

@@ -233,6 +233,8 @@ func (i *handler) getOrHeadHandler(w http.ResponseWriter, r *http.Request) {
return
}

i.handlePathAffinityHints(w, r, contentPath, logger)
Copy link
Member

Choose a reason for hiding this comment

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

What about only calling this if it's a request for raw blocks or CAR files as indicated here? Or do you want to extend the scope?

Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

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

It seems like a good initial version, as it will trigger the search for providers for the affinity path, which are likely the same providers that contain the direct block.

I pushed a quick fix for the gateway sharness tests.

@@ -233,6 +233,8 @@ func (i *handler) getOrHeadHandler(w http.ResponseWriter, r *http.Request) {
return
}

i.handlePathAffinityHints(w, r, contentPath, logger)

Copy link
Member

Choose a reason for hiding this comment

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

As someone not familiar with go nor this codebase, these variable names need work 😳

Copy link
Contributor

Choose a reason for hiding this comment

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

@SgtPooki In Go HTTP server request handlers, w and r are pretty much convention for http.ResponseWriter and *http.Request respectively. This is also seen the Go docs and examples.

logger.Debugw("async processing of Ipfs-Path-Affinity hint", "affinity", affinityPath)
if affinityPath.Mutable() {
// Skip work if mutable affinity hint is a subset of mutable contentPath
if contentPath.Mutable() && strings.HasPrefix(contentPath.String(), affinityPath.String()) {
Copy link
Member

Choose a reason for hiding this comment

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

Seems like hasPrefix check could be pulled out into a more semantic function since it's used at least twice.

// These optional hints are mostly useful for trustless block requests.
// See https://github.com/ipfs/specs/pull/462
func (i *handler) handlePathAffinityHints(w http.ResponseWriter, r *http.Request, contentPath path.Path, logger *zap.SugaredLogger) {
headerName := "Ipfs-Path-Affinity"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
headerName := "Ipfs-Path-Affinity"
const headerName = "Ipfs-Path-Affinity"

Comment on lines +969 to +984
if !i.backend.IsCached(r.Context(), immutableAffinityPath) {
// The intention of below code is to asynchronously preconnect
// gateway with providers of the affinityPath in
// Ipfs-Path-Affinity hint. Once connected, these peers can be
// asked directly (via mechanism like bitswap) for blocks
// related to main request for contentPath, and retrieve them,
// even when no other routing system had them announced. If
// original contentPath was received and returned to HTTP
// client before below get is done, the work is cancelled.

logger.Debugw("started async search for providers of Ipfs-Path-Affinity hint", "affinity", affinityPath)
_, _, err = i.backend.GetBlock(r.Context(), immutableAffinityPath)
logger.Debugw("ended async search for providers of Ipfs-Path-Affinity hint", "affinity", affinityPath, "error", err)
} else {
logger.Debugw("skipping Ipfs-Path-Affinity hint due to data being locally cached", "affinity", affinityPath)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: Instead of if .. else, check if cached and return if it is.

Suggested change
if !i.backend.IsCached(r.Context(), immutableAffinityPath) {
// The intention of below code is to asynchronously preconnect
// gateway with providers of the affinityPath in
// Ipfs-Path-Affinity hint. Once connected, these peers can be
// asked directly (via mechanism like bitswap) for blocks
// related to main request for contentPath, and retrieve them,
// even when no other routing system had them announced. If
// original contentPath was received and returned to HTTP
// client before below get is done, the work is cancelled.
logger.Debugw("started async search for providers of Ipfs-Path-Affinity hint", "affinity", affinityPath)
_, _, err = i.backend.GetBlock(r.Context(), immutableAffinityPath)
logger.Debugw("ended async search for providers of Ipfs-Path-Affinity hint", "affinity", affinityPath, "error", err)
} else {
logger.Debugw("skipping Ipfs-Path-Affinity hint due to data being locally cached", "affinity", affinityPath)
}
if i.backend.IsCached(r.Context(), immutableAffinityPath) {
logger.Debugw("skipping Ipfs-Path-Affinity hint due to data being locally cached", "affinity", affinityPath)
return
}
// The intention of below code is to asynchronously preconnect
// gateway with providers of the affinityPath in
// Ipfs-Path-Affinity hint. Once connected, these peers can be
// asked directly (via mechanism like bitswap) for blocks
// related to main request for contentPath, and retrieve them,
// even when no other routing system had them announced. If
// original contentPath was received and returned to HTTP
// client before below get is done, the work is cancelled.
logger.Debugw("started async search for providers of Ipfs-Path-Affinity hint", "affinity", affinityPath)
_, _, err = i.backend.GetBlock(r.Context(), immutableAffinityPath)
logger.Debugw("ended async search for providers of Ipfs-Path-Affinity hint", "affinity", affinityPath, "error", err)

Comment on lines +939 to +941
// Skip duplicated work if immutable affinity hint is a subset of requested immutable contentPath
// (protect against broken clients that use affinity incorrectly)
if !contentPath.Mutable() && !affinityPath.Mutable() && strings.HasPrefix(contentPath.String(), affinityPath.String()) {
Copy link
Contributor

@gammazero gammazero Jun 24, 2024

Choose a reason for hiding this comment

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

The comment is a little confusing about which is a prefix of the other.

if immutable affinity hint is a subset of requested immutable contentPath

It could mean that the contentPath should be the prefix, since the subset is a more specific (longer) path:

contentPath = "/top-level"
affinityPath = "/top-level/sub-path"

or it could me that the affinity path is a string subset of the longer content path:

contentPath = "/top-level/stuff/sub-stuff"
affinityPath = "/top-level/stuff"

@gammazero gammazero marked this pull request as draft October 29, 2024 17:51
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.

4 participants