Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

feat: IPIP-402 based backpressure #160

Merged
merged 49 commits into from
Aug 15, 2023
Merged

feat: IPIP-402 based backpressure #160

merged 49 commits into from
Aug 15, 2023

Conversation

aschmahmann
Copy link
Contributor

@aschmahmann aschmahmann commented Jul 6, 2023

Relies on ipfs/boxo#369

7/31:

  • The error rates seem to have shifted (more 500s, fewer 502s, fewer 4xxs) as a result of issues here (i.e. not returning special error types to be picked up by boxo/gateway). However, the number of errors seem to be about the same
  • Saturn nodes are not properly implementing IPIP-402 byte ranges which makes this PR not usable with Saturn until that's resolved.

Other outstanding (non functionally mandatory) items:

  • Attempt to reduce code complexity by merging the selector-like pieces together
  • Add/decide on an error type for the gateway code to indicate that a given path traversal is impossible to satisfy

Note: This change makes the testing very heavily rely on code paths other than kubo sharness tests between

  1. Not using the same gateway backend implementation
  2. Leaning almost entirely on go-unixfsnode and go-ipld-prime rather than go-unixfs and go-ipld-format.

There have been bugs discovered in each, so being eagle eyed and reporting bugs is appreciated.

@BigLep
Copy link

BigLep commented Jul 11, 2023

2023-07-11 conversation:

@aschmahmann aschmahmann marked this pull request as ready for review July 25, 2023 00:45
Comment on lines 251 to 254
escapedPath := url.PathEscape(path.String())
escapedPath = strings.ReplaceAll(escapedPath, "%2F", "/")
paramsStr := paramsToString(params)
urlWithoutHost := fmt.Sprintf("%s?%s", escapedPath, paramsStr)
Copy link
Collaborator

@lidel lidel Jul 25, 2023

Choose a reason for hiding this comment

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

In the beginning we only had string content paths, and passed that to fetcher.
Now we have URL query params and we append them blindly to content path.

I am afraid mixing URL paths and query params with IPFS content paths will lead to ugly code, special handling like the one above, and even more bugs down the road.

We should clean this up in this PR.

Current state (iiuc)

I think the underlying source of problems is that we have a content path concatenated with URL query params – this is invalid, mixing abstractions, and will lead to more bugs, such as unnecessary escaping of entity-bytes values.

My understanding of "right thing" is the edge case where content path includes characters which look like percent-encoding, and we want to preserve that by double encoding:

In Kubo 0.21 this works:
http://localhost:8080/ipns/en.wikipedia-on-ipfs.org/I/Auditorio_de_Tenerife%252C_Santa_Cruz_de_Tenerife%252C_Espa%C3%B1a%252C_2012-12-15%252C_DD_02.jpg.webp?format=car&dag-scope=entity&entity-bytes=0:* → returns HTTP 301 → http://en.wikipedia-on-ipfs.org.ipns.localhost:8080/I/Auditorio_de_Tenerife%252C_Santa_Cruz_de_Tenerife%252C_Espa%C3%B1a%252C_2012-12-15%252C_DD_02.jpg.webp?format=car&dag-scope=entity&entity-bytes=0:*

We want the same to work in biforst-gateway.

Proposed fix

We need to separate content path from query params, it cna be done in two ways:

  • (A) pass ImmutablePath + params as url.Values (requires fetcher to construct URL correctly)
  • (B) create a detached (only path + query, no host) url.URL and pass that to fetcher (imo cleaner and less error prone – we control the source of truth, fetcher won't mangle it as it already has valid url.URL to execute, will only add hostname and protocol)

Rationale: ul.URL already takes care of escaping, we should reuse it as abstraction if possible

JS has dedicated functions for escaping full URI vs URI component:
encodeURIComponent('/ą/ę')%2F%C4%85%2F%C4%99
encodeURI('/ą/ę')/%C4%85/%C4%99

in GO we have url.PathEscape and url.QueryEscape instead of encodeURIComponent
and the url.URL type's String() func itself will act as encodeURI already, so no need for extra escaping.

And I think we can reuse it here, as it takes care of all escaping + a detached path is a valid ("relative") URL:

escapedPath := (&url.URL{Path: "/ą/ę"}).String() // → /%C4%85/%C4%99

@aschmahmann @willscott my preference would be to do proper cleanup and do (B) to remove number of moving pieces pased to fetcher – would it be ok?

I can help with code tomorrow if that helps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing a url object rather than a string here seems like a good idea to me. We should be clear about the contract of what's allowed/not in the URL though.

For example, if it's valid/invalid to put format=raw in the params or headers or if the Fetch function implies CAR. While it's somewhat obviously true now it doesn't have to be that way so the docs should clarify either way.

Copy link
Collaborator

@lidel lidel Jul 26, 2023

Choose a reason for hiding this comment

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

@aschmahmann I've pushed small refactor in 982535a that does not change the fetcher API (we still pass URL as string), but fixes the way we encode content paths. I also added explicit format=car to make it easier to debug and test.

I don't think we should block this PR on the fetcher API cleanup unless we really need to include it, and it does not seem to me like we need to solve the problem at hand: it seems to work as expected already.
Would it be ok to fill issue to do it as follow-up work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lidel yeah, I don't think we need to block on a refactor unless it's hindering functionality in some way. If this is enough for now and the refactor is cleanup + reducing headaches here then can definitely be a follow up.

Updating the top-level post with the areas currently under evaluation/investigation before merging.

lidel added a commit that referenced this pull request Jul 26, 2023
this implements part of #160 (comment)
that fixes the way we encode content paths, without changing the
interface of fetcher in caboose.
@@ -43,14 +43,20 @@ func (ps *proxyBlockStore) Fetch(ctx context.Context, path string, cb lib.DataCa
return err
}
goLog.Debugw("car fetch", "url", req.URL)
req.Header.Set("Accept", "application/vnd.ipld.car")
req.Header.Set("Accept", "application/vnd.ipld.car;order=dfs;dups=y")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: this resolves #177 for non-Saturn backends. Caboose/Saturn will have to change when they're ready.

@BigLep
Copy link

BigLep commented Aug 1, 2023

2023-08-01 conversation:

  • Impaired because Saturn doesn't support range requests
  • Should be able to test this against Kubo 0.22 (since it has 402 and 412 support)
  • We aren't falling back to block requests as often

Copy link
Collaborator

@lidel lidel Jul 27, 2023

Choose a reason for hiding this comment

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

TODO: check if we have tests for when CAR backend returns HTTP 502 or 504.
We want to ensure pass-through works and the same code is returned to end user.

.github/workflows/gateway-conformance.yml Outdated Show resolved Hide resolved
Comment on lines +200 to +205
case data.Data_Raw, data.Data_Metadata:
// TODO: for now, we decided to return nil here. The different implementations are inconsistent
// and UnixFS is not properly specified: https://github.com/ipfs/specs/issues/316.
// - Is Data_Raw different from Data_File?
// - Data_Metadata is handled differently in boxo/ipld/unixfs and go-unixfsnode.
return nil
Copy link
Collaborator

@lidel lidel Aug 2, 2023

Choose a reason for hiding this comment

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

💭 @aschmahmann do we know what was the old behavior in Kubo?
Unsure how popular these are, but if we don't have spec, we may want to derisk and not change behavior that has been in production for years.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We sort of ended up giving this a 🤷 anyhow here ipfs/boxo#303 (comment). We can revisit both, but don't think that needs to happen in this PR.

lib/graph_gateway.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

@aschmahmann I've enabled gateway-conformance tests with GRAPH_BACKEND=true on this PR and main branch (merged both #189 and #190). In the past we've only run conformance against the block-by-block backend, because the gw code paths were the same. With Rhea changes, we've diverged so much we need to run tests twice now.

Main branch is green, but around 200 checks are failing in this PR:

The number looks big, but it is most likely one or two bugs which impact a lot of tests.

Some quick thoughts (I did not dig too deep, just shallow pass over the log linked above):

  • seem to be mostly related to 404 / "not found" scenarios and related error handling:
    • sometimes returned code is 5XX instead of expected 404
    • index.html is not found in a few cases (no link named \"index.html\" under bafybeig6ka5mlwkl4subqhaiatalkcleo4jgnr3hqwvpmsqfca27cijp3i)
    • some _redirects file tests are failing because HTTP 502 is returned instead of expected 200|302 etc

@aschmahmann
Copy link
Contributor Author

@lidel thanks for noticing the discrepancies with the how the conformance tests were run 🙏 and getting that fixed up. Will poke around a little more to get to the bottom, but I suspect some of the failures are coming from kubo v0.22.0 (and any released version of boxo) not returning CAR blocks when the path doesn't exist and returning an error instead which .... is not great for the use case of tooling like bifrost-gateway but is being fixed in the version of boxo this PR relies on here.

Alright to swap out the action that pulls a released kubo for custom building a branch?

This is to handle a bug where if there is a CAR request for a non-existent path
an error is returned rather than a CAR that proves that the requested path cannot exist.
@aschmahmann aschmahmann merged commit 9288910 into main Aug 15, 2023
12 checks passed
@aschmahmann aschmahmann deleted the feat/backpressure branch August 15, 2023 17:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants