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

docs: add example of gateway that proxies to ?format=raw #151

Merged
merged 5 commits into from
Feb 6, 2023
Merged

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Feb 1, 2023

Closes #142. Depends on #147.

Since this is largely based on #147, I'm basing this on top of that. I reorganized the functions so I could re-use them. Part of the fetcher code is mostly based on https://github.com/filecoin-saturn/caboose.

I just have to see how I can pass-through DNSLink and IPNS requests for resolution.

@hacdias hacdias changed the title refactor: move gateway-car/ to gateway/car/ docs: add example of gateway that proxies to ?format=raw Feb 1, 2023
@hacdias hacdias self-assigned this Feb 1, 2023
@hacdias
Copy link
Member Author

hacdias commented Feb 1, 2023

Feedback welcome @lidel @aschmahmann. I'm not sure to which point IPNS is within the scope of this PR. We could use the raw keys, but we would still have to deal with DNSLink (#149).

@codecov
Copy link

codecov bot commented Feb 1, 2023

Codecov Report

Merging #151 (47519ff) into main (302b279) will decrease coverage by 0.28%.
The diff coverage is 12.22%.

❗ Current head 47519ff differs from pull request most recent head 4dec0c5. Consider uploading reports for the commit 4dec0c5 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #151      +/-   ##
==========================================
- Coverage   18.50%   18.23%   -0.28%     
==========================================
  Files          96       99       +3     
  Lines       10337    10537     +200     
==========================================
+ Hits         1913     1921       +8     
- Misses       8152     8342     +190     
- Partials      272      274       +2     
Impacted Files Coverage Δ
examples/gateway/car/main.go 14.63% <0.00%> (ø)
examples/gateway/proxy/blockstore.go 0.00% <0.00%> (ø)
examples/gateway/proxy/main.go 0.00% <0.00%> (ø)
examples/gateway/proxy/routing.go 0.00% <0.00%> (ø)
examples/gateway/common/blocks.go 38.97% <58.33%> (ø)

@lidel
Copy link
Member

lidel commented Feb 2, 2023

While it is fine to have this example without IPNS/DNSLink, the bifrost production binary will need it – I've wrote some thoughts ipfs-inactive/bifrost-gateway#5.

@hacdias perhaps you could implement a PoC here that uses HTTP RPC at https://node[0-2].delegate.ipfs.io/api/v0/resolve for /ipns/ resolution and /api/v0/routing/get for IPNS raw records?

Base automatically changed from issue/141 to main February 2, 2023 09:12
@hacdias hacdias force-pushed the issue/142 branch 3 times, most recently from 31241d6 to 40e6eb6 Compare February 2, 2023 11:56
@hacdias
Copy link
Member Author

hacdias commented Feb 2, 2023

@lidel I reworked and added IPNS resolution: DNSLink goes through normal DNS requests implemented in the namesys package, and then I added a proxy routing that does IPNS routing requests via ?format=ipns-record. Haven't added any tests yet, but it would be great to get some feedback.

@hacdias hacdias marked this pull request as ready for review February 2, 2023 11:58
@hacdias hacdias requested a review from lidel as a code owner February 2, 2023 11:58
Copy link
Member

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

Namesys with custom routing lgtm,
but for this to be an useful example it needs better error handling (details inline)

examples/gateway/blocks.go Outdated Show resolved Hide resolved
examples/gateway/proxy/README.md Outdated Show resolved Hide resolved
examples/gateway/proxy/README.md Outdated Show resolved Hide resolved
examples/gateway/proxy/README.md Outdated Show resolved Hide resolved
examples/gateway/proxy/blockstore.go Outdated Show resolved Hide resolved
examples/gateway/proxy/README.md Outdated Show resolved Hide resolved
examples/README.md Outdated Show resolved Hide resolved
examples/gateway/proxy/blockstore.go Show resolved Hide resolved
examples/gateway/proxy/routing.go Show resolved Hide resolved
Copy link
Member

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

@hacdias lgtm! small asks below, then feel free to merge (ok to add subdomains later) 👍

examples/gateway/proxy/README.md Show resolved Hide resolved
examples/gateway/blocks.go Outdated Show resolved Hide resolved
examples/gateway/proxy/README.md Outdated Show resolved Hide resolved
examples/gateway/proxy/README.md Outdated Show resolved Hide resolved
@hacdias
Copy link
Member Author

hacdias commented Feb 6, 2023

@lidel I updated the blockstore to add validation by default, which makes sense for this example, since we are proxying requests to a remote untrusted blockstore. I had missed that. I also added two smoke tests to ensure we get an HTTP error when the remote gives invalid content, and gives 200 and the right content when the content is indeed valid. I will merge once CI is green.

@hacdias hacdias merged commit a4e0934 into main Feb 6, 2023
@hacdias hacdias deleted the issue/142 branch February 6, 2023 08:48
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.

gateway: create example that proxies to requests to ?format=raw
2 participants