feat: refactor gateway api to operate on higher level semantics #176

aschmahmann · 2023-02-16T14:54:47Z

Kubo PR that runs sharness tests: ipfs/kubo#9681 (sharness in this PR will fail, but MUST be green in the linked PR)

This is a start at a gateway refactor so that we operate on higher level semantics rather than lower level ones ipfs/kubo#173.

This enables us to have all the gateway HTTP code separate from whether the data was retrieved over Bitswap, via requesting the entire result as a CAR file, etc.

This probably could've been done while still keeping a bunch of the traversal code in the gateway (and collecting metrics like first block times, etc.). However, IMO this seemed unnecessary given that it should be the job of the API implementations to verify the data is correct and the gateway code's job to "HTTP-ify" it and record some higher level metrics.

The result is the a lot of the code has been inverted, since instead of the order being "get a bit -> verify -> get more -> verify..." we have "resolve mutable -> do top level request and get async response -> process async response and emit the correct headers".

Given that the implementations (e.g. of the BlockGateway or the interface-go-ipfs-core one) aren't built yet this is all obviously still in flux, but wanted to put something up in case people think this approach is nuts 😅. For this to all be usable I suspect we'll probably need to have some shared utilities so we don't replicate some of the annoying gateway traversal code multiple times (e.g. handling _redirects).

Also addresses:

Support DNSLink on ENS (move namesys defaults out of Kubo) ipfs-inactive/bifrost-gateway#60

Requirements:

~~- [ ] ipfs/go-unixfs#142~~ No longer needed.

Jorropo · 2023-02-16T15:31:10Z

gateway/gateway.go

@@ -17,34 +16,110 @@ type Config struct {
 	Headers map[string][]string
 }

-// API defines the minimal set of API services required for a gateway handler.
+// ImmutablePath TODO: This isn't great, as its just a signal and mutable paths fulfill the same interface
+type ImmutablePath interface {


Why interface ?

Was matching the other path types, doesn't have to be though. This was mostly a placeholder

gateway/gateway.go

lidel

(I've run out of time today, will try to do proper read tomorrow, for now dropping partial review with feedback on some TODOs and _redirects (i'd de-emphasize it, it is niche feature, can we move it back to the same place as ipfs-404 thingy? i got gut feeling entire thing will get way less noisy)

gateway/gateway.go

lidel · 2023-02-16T23:00:34Z

gateway/handler.go

+		logger.Debugw("serving ipns record", "path", contentPath)
+
+		i.addUserHeaders(w)                                 // ok, _now_ write user's headers.
+		w.Header().Set("X-Ipfs-Path", contentPath.String()) // TODO: Should we even set this?


for completeness, yes
in practice we could skip these on verifiable responses and nobody would be impacted
(these matter only for implicit "deserialize for me pls" GET done for website hosting)

gateway/handler.go

gateway/handler_defaults.go

…eway's problem. Implemented blocks gateway example

aschmahmann · 2023-02-26T08:19:26Z

Updated: pushed some updates here, still need to rebase on the gateway PRs that have happened in the meanwhile.

There are still some TODOs to be figured out (and I think I found a bug in files package on the way). Two bigger picture questions I had here are:

If/how we want to deal with sentinel errors so we know which HTTP Status Codes to return. Basically as we shuffle work out to the Gateway API implementations they now need to report back more than an opaque error so we know if it's a timeout/couldn't find the data, the data cannot (immutable)/does not (mutable) exist, internal processing errors (blockstore corruption returning the wrong data, etc.), or even "I'm busy go away" 429 errors.
As we shuffle more logic into Gateway API implementations I get more nervous about our testing situation and how much it currently relies on kubo sharness tests.

I'd probably feel better about this if we could get kubo leveraging a blocks gateway implementation that lives here. How unreasonable of an approach is this?

aschmahmann · 2023-02-26T08:23:33Z

files/readerfile.go

 func NewBytesFile(b []byte) File {
-	return &ReaderFile{"", NewReaderFile(bytes.NewReader(b)), nil, int64(len(b))}
+	return &ReaderFile{"", bytesReaderCloser{bytes.NewReader(b)}, nil, int64(len(b))}
+}
+
+// TODO: Is this the best way to fix this bug?
+// The bug is we want to be an io.ReadSeekCloser, but bytes.NewReader only gives a io.ReadSeeker and io.NopCloser
+// effectively removes the io.Seeker ability from the passed in io.Reader
+type bytesReaderCloser struct {
+	*bytes.Reader
+}
+
+func (b bytesReaderCloser) Close() error {
+	return nil


This seems like a bug and a small fix for it. I might be missing something though about why it was done this way.

gateway/gateway.go

aschmahmann · 2023-02-26T08:30:15Z

gateway/handler_defaults.go

+		}
+		defer data.Close()
+	case http.MethodGet:
+		gwMetadata, data, err = i.api.Get(ctx, imPath)


TODO: handle range requests, probably by copying the golang HTTP library's parser

# Conflicts: # gateway/gateway_test.go # gateway/handler.go # gateway/handler_block.go # gateway/handler_codec.go # gateway/handler_tar.go # gateway/handler_unixfs.go # gateway/handler_unixfs__redirects.go # gateway/handler_unixfs_dir.go # go.mod # go.sum

BigLep · 2023-02-28T18:18:33Z

2023-02-28 conversation:
This is a precursor for the graph work
Thins for 2023-02-28:
Catchup with other changes happening here.
Get it running
Get it to a place where Henrique can take W/Th or Adin gets back to it Friday

codecov · 2023-03-01T08:23:51Z

Codecov Report

Merging #176 (360b031) into main (5bbb21b) will decrease coverage by 0.11%.
The diff coverage is 36.28%.

@@            Coverage Diff             @@
##             main     ipfs/kubo#176      +/-   ##
==========================================
- Coverage   48.13%   48.02%   -0.11%     
==========================================
  Files         267      269       +2     
  Lines       32571    32982     +411     
==========================================
+ Hits        15678    15841     +163     
- Misses      15236    15483     +247     
- Partials     1657     1658       +1

Impacted Files	Coverage Δ
examples/gateway/car/main.go	`8.00% <0.00%> (ø)`
examples/gateway/proxy/main.go	`0.00% <0.00%> (ø)`
gateway/dns.go	`0.00% <0.00%> (ø)`
gateway/handler_block.go	`0.00% <0.00%> (ø)`
gateway/handler_car.go	`0.00% <0.00%> (ø)`
gateway/handler_codec.go	`0.00% <0.00%> (ø)`
gateway/handler_ipns_record.go	`0.00% <0.00%> (ø)`
gateway/handler_tar.go	`0.00% <0.00%> (ø)`
gateway/handler_defaults.go	`31.51% <31.51%> (ø)`
gateway/handler_unixfs__redirects.go	`37.09% <32.53%> (-2.25%)`	⬇️
... and 9 more

... and 14 files with indirect coverage changes

lidel

Gave it another pass, commented inline. (Only this PR, I did not look at sharness yet, I assumed we want the dust to settle first)

Highlights:

found some ways we could simplify+unify handling of path traversal errors
we may want to introduce ContentType at top level APIs to avoid forcing unnecessary sniffing and block fetches when we have explicit one in root node or CID codec
browser on often updated websites may hit an additional RTT around If-None-Match on /ipns/, but not sure if we should worry yet
general anxiety about huge refactor ;) 🫂

gateway/gateway.go

gateway/handler.go

gateway/handler_defaults.go

gateway/handler_unixfs__redirects.go

gateway/handler_unixfs_dir.go

… the Gateway API implementer

BigLep · 2023-03-21T20:20:03Z

2023-03-21 maintainer conversation: this is blocked from being merged until #206 lands.

# Conflicts: # examples/gateway/common/blocks.go # examples/go.mod # examples/go.sum # gateway/errors.go # gateway/gateway.go # gateway/gateway_test.go # gateway/handler.go # gateway/handler_car.go # gateway/handler_codec.go # gateway/handler_test.go # gateway/handler_unixfs.go # gateway/handler_unixfs__redirects.go # gateway/handler_unixfs_dir.go # go.mod # go.sum

BigLep · 2023-03-28T16:52:49Z

Blocked on Kubo sharness test passing in ipfs/kubo#9681 (needs rebase), which is blocked on ipfs/kubo#9746 landing.

hacdias · 2023-03-29T09:00:06Z

I rebased ipfs/kubo#9681 and everything is green 💚.

Context: ipfs/boxo#215 We use commit from ipfs/boxo#176 (comment) to unblock work on ipfs-inactive/bifrost-gateway#61

Context: ipfs/boxo#215 Used commits from ipfs/boxo#176 (comment) and filecoin-saturn/caboose#68 to unblock work on #61

lidel · 2023-03-31T12:16:57Z

examples/gateway/car-file-gateway/libp2pio.car

@hacdias @aschmahmann afaik this file is not used anywhere, but increases repo size by +14MB. 🙈
Too late to force-push main to fix it, but can we at least remove it in separate PR so there is no confusing examples/gateway/car-file-gateway/ ?

If we need fixtures for tests, we should always put them in testdata/ directory, which has special meaning in go tooling

@lidel urgh, will do! Sorry missed that.

* refactor gateway api to operate on semantics to closer match user requests * rename gateway api interface to IPFSBackend * add blocks gateway implementation to the gateway package * refactored gateway examples * add default DNS resolvers * add default IPLD codecs --------- Co-authored-by: Henrique Dias <[email protected]> Co-authored-by: Marcin Rataj <[email protected]>

removed depecated and unused TTFB histograms. after #176 and #245 we now have api_call_duration_seconds histograms for higher resolution info per backend operation

feat: refactor gateway api to operate on higher level semantics

8ba923b

Jorropo reviewed Feb 16, 2023

View reviewed changes

lidel reviewed Feb 17, 2023

View reviewed changes

BigLep linked an issue Feb 17, 2023 that may be closed by this pull request

Refactor Gateway API so can extract out the request layer #173

Closed

fix: respond to comments

0ead6cd

BigLep assigned aschmahmann Feb 21, 2023

next iteration: moved _redirects and index.html back to being the gat…

8704fef

…eway's problem. Implemented blocks gateway example

aschmahmann commented Feb 26, 2023

View reviewed changes

rename GatewayMetadata -> ContentPathMetadata

0b6c1c5

aschmahmann commented Feb 26, 2023

View reviewed changes

gateway/gateway.go Outdated Show resolved Hide resolved

aschmahmann commented Feb 26, 2023

View reviewed changes

aschmahmann added 2 commits February 27, 2023 10:09

go mod tidy

9e32875

aschmahmann added 8 commits February 28, 2023 18:28

make tests mostly pass, includes change to go-unixfs

7d145b5

Merge branch 'main' into feat/gateway-refactor

ebbe85d

use shared BlocksGatway

8413c2b

fixup most errors, expand definition of NotFound

2b172e7

change error reporting to pass a test

461fca2

tmp: point at go-unixfs commit hash

8fca1ae

downgrade go-car+otel dependencies and tidy

2bba208

fix: bump go-unixfs dep that was missed previously

2631a5c

aschmahmann added 2 commits March 1, 2023 03:28

bump go-unixfs to support cid metadata in symlinks

6652355

fix it so we don't cache /ipns/somedir/index.html

3f135ce

This was referenced Mar 1, 2023

refactor: switch gateway code to new API from go-libipfs ipfs/kubo#9681

Merged

feat: cid metadata ipfs/go-unixfs#142

Closed

lidel reviewed Mar 2, 2023

View reviewed changes

feat(gateway): allow content-type for UnixFS data to be determined by…

bbdc88f

… the Gateway API implementer

hacdias requested a review from lidel March 21, 2023 10:58

fix: ensure basic ipld codecs are registered

1c4716f

BigLep mentioned this pull request Mar 21, 2023

Handle relationship between boxo and go-car and go-merkledag #218

Closed

5 tasks

hacdias mentioned this pull request Mar 22, 2023

Gateway: Possible Invalid 'X-Ipfs-Roots' Header #221

Closed

Jorropo mentioned this pull request Mar 25, 2023

New Unixfs implementations #224

Open

aschmahmann added 4 commits March 27, 2023 17:08

more boxo related fixups

b5beaa2

more boxo related fixups

2e4b7c3

cleanup variable name in example

ffa3aad

lidel mentioned this pull request Mar 29, 2023

feat: opt-in GRAPH_BACKEND ipfs-inactive/bifrost-gateway#61

Merged

2 tasks

Jorropo mentioned this pull request Mar 29, 2023

Gateway: too many CoreAPI.ResolvePath calls #233

Closed

Merge branch 'main' into feat/gateway-refactor

360b031

lidel added a commit to filecoin-saturn/caboose that referenced this pull request Mar 29, 2023

refactor: go-libipfs → boxo

83577ee

Context: ipfs/boxo#215 We use commit from ipfs/boxo#176 (comment) to unblock work on ipfs-inactive/bifrost-gateway#61

lidel mentioned this pull request Mar 29, 2023

feat: switch to boxo and fix CAR fetch timeouts filecoin-saturn/caboose#68

Merged

7 tasks

lidel added a commit to ipfs-inactive/bifrost-gateway that referenced this pull request Mar 29, 2023

refactor: switch to boxo/gateway.NewBlocksGateway

955bcd5

Context: ipfs/boxo#215 Used commits from ipfs/boxo#176 (comment) and filecoin-saturn/caboose#68 to unblock work on #61

lidel added a commit to ipfs-inactive/bifrost-gateway that referenced this pull request Mar 29, 2023

refactor: switch to boxo/gateway.NewBlocksGateway

fd484ac

Context: ipfs/boxo#215 Used commits from ipfs/boxo#176 (comment) and filecoin-saturn/caboose#68 to unblock work on #61

aschmahmann merged commit b7d8da1 into main Mar 29, 2023

hacdias deleted the feat/gateway-refactor branch March 29, 2023 18:47

lidel reviewed Mar 31, 2023

View reviewed changes

hacdias mentioned this pull request Apr 5, 2023

refactor(gateway): consistent tracing names #261

Merged

lidel added a commit that referenced this pull request Apr 5, 2023

fix(gw): widen duration histograms and cleanup

b8d766f

removed depecated and unused TTFB histograms. after #176 and #245 we now have api_call_duration_seconds histograms for higher resolution info per backend operation

lidel mentioned this pull request Apr 5, 2023

fix(gw): widen duration histograms and cleanup #265

Merged

lidel added a commit that referenced this pull request Apr 5, 2023

fix(gw): widen duration histograms and cleanup

6e1d7a4

removed depecated and unused TTFB histograms. after #176 and #245 we now have api_call_duration_seconds histograms for higher resolution info per backend operation

lidel mentioned this pull request Apr 6, 2023

docs: preparing 0.20 changelog for release ipfs/kubo#9799

Merged

lidel mentioned this pull request Jul 19, 2023

fix: handle _redirects for If-None-Match headers #412

Merged

lidel mentioned this pull request Aug 29, 2023

Add tests for HTTP Range requests ipfs/gateway-conformance#154

Open

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: refactor gateway api to operate on higher level semantics #176

feat: refactor gateway api to operate on higher level semantics #176

aschmahmann commented Feb 16, 2023 •

edited

Loading

Jorropo Feb 16, 2023

aschmahmann Feb 17, 2023

lidel left a comment

lidel Feb 16, 2023

aschmahmann commented Feb 26, 2023

aschmahmann Feb 26, 2023

aschmahmann Feb 26, 2023 •

edited

Loading

BigLep commented Feb 28, 2023

codecov bot commented Mar 1, 2023 •

edited

Loading

lidel left a comment •

edited

Loading

BigLep commented Mar 21, 2023

BigLep commented Mar 28, 2023 •

edited by lidel

Loading

hacdias commented Mar 29, 2023

lidel Mar 31, 2023 •

edited

Loading

hacdias Mar 31, 2023

hacdias Mar 31, 2023

feat: refactor gateway api to operate on higher level semantics #176

feat: refactor gateway api to operate on higher level semantics #176

Conversation

aschmahmann commented Feb 16, 2023 • edited Loading

Jorropo Feb 16, 2023

Choose a reason for hiding this comment

aschmahmann Feb 17, 2023

Choose a reason for hiding this comment

lidel left a comment

Choose a reason for hiding this comment

lidel Feb 16, 2023

Choose a reason for hiding this comment

aschmahmann commented Feb 26, 2023

aschmahmann Feb 26, 2023

Choose a reason for hiding this comment

aschmahmann Feb 26, 2023 • edited Loading

Choose a reason for hiding this comment

BigLep commented Feb 28, 2023

codecov bot commented Mar 1, 2023 • edited Loading

Codecov Report

lidel left a comment • edited Loading

Choose a reason for hiding this comment

BigLep commented Mar 21, 2023

BigLep commented Mar 28, 2023 • edited by lidel Loading

hacdias commented Mar 29, 2023

lidel Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

hacdias Mar 31, 2023

Choose a reason for hiding this comment

hacdias Mar 31, 2023

Choose a reason for hiding this comment

aschmahmann commented Feb 16, 2023 •

edited

Loading

aschmahmann Feb 26, 2023 •

edited

Loading

codecov bot commented Mar 1, 2023 •

edited

Loading

lidel left a comment •

edited

Loading

BigLep commented Mar 28, 2023 •

edited by lidel

Loading

lidel Mar 31, 2023 •

edited

Loading