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: nft.storage naive gateway implementation #908

Merged
merged 3 commits into from
Jan 14, 2022

Conversation

vasco-santos
Copy link
Contributor

@vasco-santos vasco-santos commented Dec 7, 2021

This PR adds a naive implementation of the gateway as a CF Worker using subdomains, where it simply forwards requests to the IPFS public gateway for now as part of #823

It includes:

  • worker implementation with ESM
  • test setup with miniflare
  • actions for tests / release

ACM was configure in the TLS setup for the nft.storage domain, so that we can do:

  • bafkreidchi5c4c3kwr5rpkvvwnjz3lh44xi2y2lnbldehwmpplgynigidm.ipfs.nft.storage
  • bafkreidchi5c4c3kwr5rpkvvwnjz3lh44xi2y2lnbldehwmpplgynigidm.ipfs-staging.nft.storage
  • bafkreidchi5c4c3kwr5rpkvvwnjz3lh44xi2y2lnbldehwmpplgynigidm.ipfs-staging.nft.storage/path

Besides follow up work tracked on #823 needs:

  • follow up PR with sentry configured

Closes #867

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Dec 7, 2021

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: c673d98
Status: ✅  Deploy successful!
Preview URL: https://50cc1366.nft-storage-1at.pages.dev

View logs

@vasco-santos vasco-santos force-pushed the feat/gateway-implementation branch 2 times, most recently from 1961d0b to 30e7ad1 Compare December 7, 2021 11:11
account_id = "fffa4b4363a7e5250af8357087263b3a" # Protocol Labs CF account
zone_id = "fc6cb51dbc2d0b9a729eae6a302a49c9" # nft.storage zone
route = "*gateway.nft.storage/*"
vars = { IPFS_GATEWAY = "https://ipfs.io" }
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: We will need to setup multiple gateways, either we need to hardcode multiple here or we will need to use a string array and parse it. Leaving for when needed

@vasco-santos vasco-santos marked this pull request as ready for review December 7, 2021 11:25
@vasco-santos vasco-santos requested a review from alanshaw December 7, 2021 11:28
@vasco-santos vasco-santos mentioned this pull request Dec 8, 2021
1 task
@vasco-santos vasco-santos force-pushed the feat/gateway-implementation branch 3 times, most recently from 002a852 to e7f01c9 Compare December 9, 2021 14:32
@vasco-santos vasco-santos force-pushed the feat/gateway-implementation branch 2 times, most recently from 5787f43 to 19c5f81 Compare December 22, 2021 12:17
Copy link
Contributor

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

❤️ Awesome work so far!

packages/gateway/README.md Outdated Show resolved Hide resolved
packages/gateway/README.md Outdated Show resolved Hide resolved
packages/gateway/package.json Outdated Show resolved Hide resolved
packages/gateway/package.json Outdated Show resolved Hide resolved
packages/gateway/scripts/cli.js Outdated Show resolved Hide resolved
packages/gateway/src/index.js Show resolved Hide resolved
packages/gateway/src/index.js Outdated Show resolved Hide resolved
export function getCidFromSubdomainUrl(url) {
// Replace "ipfs-staging" by "ipfs" if needed
const nUrl = url.replace('ipfs-staging', 'ipfs')
const splitUrl = nUrl.split('.ipfs.')
Copy link
Contributor

@alanshaw alanshaw Jan 6, 2022

Choose a reason for hiding this comment

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

At this point is splitUrl not ['https://bafy...', 'nft.storage']? splitUrl[0] will fail to parse as a CID.

Copy link
Contributor Author

@vasco-santos vasco-santos Jan 6, 2022

Choose a reason for hiding this comment

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

Ok, the parameter name was unfortunate, it is the hostname:

getCidFromSubdomainUrl(url.hostname)

So, I guess just rename the parameter should be enough. Maybe better to send URL instead and get the hostname inside the function

packages/gateway/src/utils/json-response.js Outdated Show resolved Hide resolved
"pretest": "npm run build ",
"test": "npm-run-all -p -r mock:ipfs.io test:worker",
"test:worker": "ava --verbose test/*.spec.js",
"mock:ipfs.io": "smoke -p 9081 test/mocks/ipfs.io"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we spin up an IPFS node instead? We can use the IPFS HTTP API to add content that we want to fetch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's do this in the follow up PR (Race), I just commented to remember: #961 (review)

That's because I need to have tests where I can manipulate who is faster. In that PR it will be easier to do that, perhaps using a real ipfs node + smoke tests for other gateways

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes fine to do in follow up PR 😄

That's because I need to have tests where I can manipulate who is faster. In that PR it will be easier to do that, perhaps using a real ipfs node + smoke tests for other gateways

Maybe just a simple proxy server that adds a delay? I find smoke to have quite a big overhead in terms of knowing what to call the file, what the response object should look like, how to get requests parsed correctly etc.

@vasco-santos vasco-santos force-pushed the feat/gateway-implementation branch from 3d046c4 to 5bd3fa9 Compare January 6, 2022 16:13
@vasco-santos vasco-santos requested a review from alanshaw January 6, 2022 16:21
@vasco-santos vasco-santos force-pushed the feat/gateway-implementation branch from 5bd3fa9 to b13a6ae Compare January 14, 2022 14:11
Copy link
Contributor

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

A few minor tidyup things but this is good to go otherwise!

packages/gateway/src/error-handler.js Outdated Show resolved Hide resolved
packages/gateway/src/error-handler.js Show resolved Hide resolved
packages/gateway/src/utils/cid.js Outdated Show resolved Hide resolved
packages/gateway/src/utils/cid.js Outdated Show resolved Hide resolved
packages/gateway/src/errors.js Outdated Show resolved Hide resolved
packages/gateway/src/utils/cid.js Outdated Show resolved Hide resolved
packages/gateway/src/utils/cid.js Outdated Show resolved Hide resolved
packages/gateway/src/errors.js Outdated Show resolved Hide resolved
@vasco-santos vasco-santos force-pushed the feat/gateway-implementation branch from 2e0632f to c673d98 Compare January 14, 2022 15:58
@vasco-santos vasco-santos merged commit 119d948 into main Jan 14, 2022
@vasco-santos vasco-santos deleted the feat/gateway-implementation branch January 14, 2022 16:16
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.

gateway.nft.storage - CF Worker
2 participants