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

repo/fsrepo/migrations: verified HTTP migrations #10324

Merged
merged 2 commits into from
Feb 19, 2024
Merged

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Jan 29, 2024

Closes #9159. This changes the current HttpFetcher to use CAR files. Because we have /ipns/dist.ipfs.tech as default (even though not used), I also implemented DNSLink resolution, as well as IPNS record fetching and verification.

Go tests were updated such that the test server now uses a CAR file backed gateway. This CAR file is generated on the fly before the tests begin. The reasoning behind this versus a static CAR is that the migration file names depend on the platform that they are being run on.

You can also test this locally by spawning a repository, downgrading the version, and running the migrations:

# Build local version (this PR)
make build

# New Kubo repo
export IPFS_PATH=$(mktemp -d)
./cmd/ipfs/ipfs init

# Change version to older version
echo 13 > $IPFS_PATH/version

# Start daemon and accept migrations
./cmd/ipfs/ipfs daemon

@hacdias hacdias requested a review from a team as a code owner January 29, 2024 15:45
@hacdias hacdias marked this pull request as draft January 29, 2024 15:45
@hacdias hacdias self-assigned this Jan 30, 2024
@hacdias hacdias force-pushed the car-migrations branch 3 times, most recently from 192066a to e3f026d Compare January 30, 2024 10:51
@hacdias
Copy link
Member Author

hacdias commented Jan 30, 2024

I can't figure out a way of "fixing" the Docker test that checks if Docker downloads the correct version. The problem here is that uses the hardcoded distribution CIDs in the code. But I don't have that CAR file to just serve with socat. And the file would be massive nevertheless.

I would suggest removing this test unless someone has a very good idea.

@hacdias hacdias marked this pull request as ready for review January 30, 2024 10:52
@aschmahmann
Copy link
Contributor

aschmahmann commented Jan 30, 2024

@hacdias removing that test (or reworking it to not use docker) might be reasonable.

However, if you wanted to keep this test working roughly as is you might be able to work around the large CAR issue by setting the IPFS_DIST_PATH env var to the root CID of a small DAG that you can control + embed.

envIpfsDistPath = "IPFS_DIST_PATH"

@hacdias
Copy link
Member Author

hacdias commented Jan 30, 2024

@aschmahmann I think the test uses docker on purpose: the idea seems to be that the Docker image fetches the correct migration for the correct arch and platform. I will try what you said and see how it goes. Otherwise, I will just remove it.

@hacdias
Copy link
Member Author

hacdias commented Jan 30, 2024

@aschmahmann the environment variable seems to do the trick 😄

Comment on lines +196 to +197
func carStreamToFileBytes(ctx context.Context, r io.ReadCloser, imPath path.ImmutablePath) ([]byte, error) {
defer r.Close()
Copy link
Member Author

@hacdias hacdias Jan 30, 2024

Choose a reason for hiding this comment

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

I think we should extract this code into either of:

  1. As a Boxo example on "how to download a CAR from the gateway and verify its contents"; or
  2. Make a package that does it, or perhaps just inside gateway. There's a lot of "bootstrap" code here that feels a bit annoying to write and having an exported function that does this somewhere in Boxo could be very helpful. Maybe gateway.Fetcher?

Copy link
Member

Choose a reason for hiding this comment

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

Creating an example is good enough, but this is so popular use case we should have a high level boxo lib/API.

Unsure about putting this in gateway package, feels like something that deserves own.

Given we have https://www.npmjs.com/package/@helia/verified-fetch that basically is a similar abstraction, maybe we could have boxo/fetch and VerifiedFetch that works in similar way?

We don't have to do it now, ok to do example now + fill issue for the API.

@hacdias hacdias force-pushed the car-migrations branch 2 times, most recently from f575d30 to 6a92350 Compare February 7, 2024 09:58
@hacdias hacdias mentioned this pull request Feb 8, 2024
8 tasks
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.

lgtm, works as expected, thank you for making migrations fully trustless. ❤️
Merge when it feels appropriate.

ps. @hacdias I've been thinking about reusing the CAR fetch code, and turning it into a utility.

Filled issues for follow-up work in:

Doing these is a good opportunity to flesh out how Fetch API library in go should look like.

@hacdias hacdias merged commit 595e1ba into master Feb 19, 2024
14 checks passed
@hacdias hacdias deleted the car-migrations branch February 19, 2024 13:20
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.

HTTP fetch of fs-migrations should use CAR
3 participants