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

Gateway: Fetch recursive DAG as CAR #304

Merged
merged 6 commits into from
Oct 10, 2022

Conversation

Frando
Copy link
Member

@Frando Frando commented Oct 7, 2022

Hi!
I was looking on how to use iroh from other codebases, and a bit of a precursor to a writable gateway, this PR adds functionality to fetch a DAG recursively into a CAR file. This was already stubbed in the gateway but not implemented.

The implementation works similar to the existing recursive loaders. However to save the cost of reserializing, I added a new OutRaw struct in the resolver for this (because the Out struct in the resolver currently only keeps the deserialized representation for UnixFS nodes). Currently, the nodes are still fully deserialized anyways for link scraping but that would be fixed with n0-computer/beetle#162.

What's still missing is, I think, a configurable upper limit on DAG sizes that can be requested from the Gateway to avoid DoS attacks. Also misses tests, I can add some if the PR seems on the right track to you.

let mut cids = VecDeque::new();
let this = self.clone();
async_stream::try_stream! {
let root_block = this.resolve_root(&root).await.map(|(cid, loaded)| OutRaw::from_loaded(cid, loaded));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the same code as resolve_recursive effectively, other than mapping things differently, maybe this could be abstracted by passing a closer to a shared function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good. I implemented an abstraction to be shared between the two resolve_recursive functions. resolve_recursive_with_path doesn't yet use it because there's additional logic around keeping the path. Could add it to the generic one, if always maintaining the path isn't considered too expensive (don't think it would matter).

Copy link
Member Author

@Frando Frando Oct 9, 2022

Choose a reason for hiding this comment

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

@dignifiedquire I've looked into having the remaining resolve_recursive_with_path also use the resolve_recursive_mapped function from the latest commit in this PR. I made it work but with the cost of a couple of additional clones of the path even if it's not being used. See this commit (not on this PR): Frando@91c0440
I wasn't sure if it's worth it and couldn't find a way to avoid the clones (because of lifetime issues due to async move in the closure)

@b5 b5 assigned Frando Oct 8, 2022
@b5 b5 added the c-gateway label Oct 8, 2022
Copy link
Collaborator

@Arqu Arqu left a comment

Choose a reason for hiding this comment

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

Gateway code looks good to me. Once digs comments are resolved, I'm happy to have this merged.

@Frando
Copy link
Member Author

Frando commented Oct 9, 2022

I added a test as well. @Arqu because there's not any integration tests in the gateway yet maybe review if the testing approach is correct.

@Arqu
Copy link
Collaborator

Arqu commented Oct 10, 2022

Lovely, thank you. The testing story is a bit grim currently besides some smaller bits. There's a pending issue I have to tackle to get tests going but will have to wait a little bit longer.

@Arqu Arqu merged commit 0dc698e into n0-computer:main Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants