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 broken for IPNS entries #1150

Closed
dylanPowers opened this issue Apr 27, 2015 · 17 comments · Fixed by #1191 or #1219
Closed

Gateway broken for IPNS entries #1150

dylanPowers opened this issue Apr 27, 2015 · 17 comments · Fixed by #1191 or #1219
Assignees
Labels
exp/expert Having worked on the specific codebase is important kind/bug A bug in existing code (including security flaws) topic/gateway Topic gateway

Comments

@dylanPowers
Copy link
Member

Go to http://localhost:8080/ipns/QmSJ5B27yQe9bDLmwqF4FxJpBBcpxMnK9uJwX8qbQUyycN

Result:

Path Resolve error: multihash length inconsistent: &{124  130 [210]}

ipfs name resolve QmSJ5B27yQe9bDLmwqF4FxJpBBcpxMnK9uJwX8qbQUyycN on the other hand works fine.

@whyrusleeping

@jbenet
Copy link
Member

jbenet commented Apr 27, 2015

@dylanPowers is that on master? it works for me.

@dylanPowers
Copy link
Member Author

Yes master after the new dnslink feature

@jbenet
Copy link
Member

jbenet commented Apr 27, 2015

ah

@dylanPowers
Copy link
Member Author

Specifically I'm seeing it on 196c6aa

@whyrusleeping
Copy link
Member

This is because we need to use core.Resolve in core/corehttp/gateway_handler.go.

I attempted to make the fix, but i have no idea whats going on in there.

@whyrusleeping whyrusleeping added kind/bug A bug in existing code (including security flaws) codereview topic/gateway Topic gateway and removed codereview labels Apr 27, 2015
@cryptix
Copy link
Contributor

cryptix commented Apr 28, 2015

Some logging reviled that resolveNamePath somehow prepends the resolved path with /ipfsagain. Slicing it off (p=p[5:]) makes it work. I'll look into a cleaner fix. Looks like resolveNamePath can go away with the use of core.Resolve.

15:30:57.440 ERROR core/serve: resolveNamePath "/ipns/QmSJ5B27yQe9bDLmwqF4FxJpBBcpxMnK9uJwX8qbQUyycN" resolved to "/ipfs/ipfs/QmNR6fGuSxmUcAPyG9iuhFoew9f7L7mcU6ydB2VsCZo2mD"  gateway_handler.go:89
15:30:57.440 ERROR core/serve: debug: after i.resolveNamePath on /ipfs/QmNR6fGuSxmUcAPyG9iuhFoew9f7L7mcU6ydB2VsCZo2mD gateway_handler.go:99
15:30:57.440 ERROR core/serve: resolved: &{Links:[0xc20843f940 0xc20843f9c0 0xc20843fa40 0xc20843fac0 0xc20843fb40 0xc20843fbc0 0xc20843fc40 0xc20843fcc0 0xc20843fd40] Data:[8 1] encoded:[] cached:[]} gateway_handler.go:106

@cryptix
Copy link
Contributor

cryptix commented Apr 28, 2015

This makes it work for me:

 func (i *gatewayHandler) ResolvePath(ctx context.Context, p string) (*dag.Node, string, error) {
-   p, err := i.resolveNamePath(ctx, p)
+   node, err := core.Resolve(i.node, path.Path(p))
    if err != nil {
        return nil, "", err
    }

-   node, err := i.node.Resolver.ResolvePath(path.Path(p))
-   if err != nil {
-       return nil, "", err
-   }
-   return node, p, err
+   return node, p, nil
 }

but this would ignore the ctx. @whyrusleeping is this some old style of passing the context around or is this a problem?

I'm not sure what this cancels are for. I guess, the idea is to cancel the related swarm communications after the http request is done?

@whyrusleeping
Copy link
Member

@cryptix good point on the context cancelling, I think that we could honestly make core.Resolve take a context. Its probably a good idea anyways

@cryptix
Copy link
Contributor

cryptix commented Apr 30, 2015

Agreed that core.Resolve should get the relevant context again. Currently it gets one from the node but a user (like the http endpoint trying to resolve the requested path) might want to wrap that context into a timeout or cancel so the internal context is basically useless.

@jbenet
Copy link
Member

jbenet commented May 1, 2015

arg-- this bug bit me in a demo for Brewster at the Internet Archive-- can we get it fixed asap?

I'm not sure what this cancels are for. I guess, the idea is to cancel the related swarm communications after the http request is done?

yes, cancel all long ops kicked off by this http request.

Agreed that core.Resolve should get the relevant context again. Currently it gets one from the node but a user (like the http endpoint trying to resolve the requested path) might want to wrap that context into a timeout or cancel so the internal context is basically useless.

👍

@cryptix that solution LGTM

@cryptix
Copy link
Contributor

cryptix commented May 1, 2015

that solution LGTM

@jbenet OK, nice! Just was waiting for the green light on this. I started refactoring gateway_http.go but I won't be able to finish it until tomorrow, Sunday at worst.

@wking
Copy link
Contributor

wking commented May 5, 2015

On Tue, Apr 28, 2015 at 06:39:16AM -0700, Henry wrote:

Some logging reviled that resolveNamePath somehow prepends the
resolved path with /ipfsagain.

I was looking through namesys last night to get references for the
current IPFS-default for dnslink 1, and I'm pretty sure the /ipfs/
injection is coming from 2. It seems like there's some parallel
high-level resolver stuff (e.g. core.Resolve and some corehttp stuff
and namesys), but at least for the dnslink path this /ipfs/ injection
seems to undo the earlier /ipfs/ or /ipns/ prefix removal here 3.

@wking
Copy link
Contributor

wking commented May 5, 2015

On Tue, May 05, 2015 at 07:27:55AM -0700, W. Trevor King wrote:

On Tue, Apr 28, 2015 at 06:39:16AM -0700, Henry wrote:

Some logging reviled that resolveNamePath somehow prepends the
resolved path with /ipfsagain.

I was looking through namesys last night to get references for the
current IPFS-default for dnslink 1, and I'm pretty sure the /ipfs/
injection is coming from [2].

[2]: https://github.com/ipfs/go-ipfs/blob/9a5bb52fcd247f8d6789290680469029b70d7a34/path/path.go#L28

@whyrusleeping cleared this up on IRC, pointing out that ParsePath is
just checking the syntax, and not doing any actual resolving 1.

@jbenet jbenet self-assigned this May 10, 2015
@jbenet
Copy link
Member

jbenet commented May 10, 2015

I think the sharness tests should test the IPNS part of the gateway.

@jbenet jbenet removed the status/in-progress In progress label May 10, 2015
@jbenet jbenet reopened this May 10, 2015
@jbenet
Copy link
Member

jbenet commented May 10, 2015

We should test this to make sure it works.

@jbenet
Copy link
Member

jbenet commented May 10, 2015

Ok looks like this is fixed! (confirm @dylanPowers ?)

@jbenet jbenet closed this as completed May 10, 2015
@dylanPowers
Copy link
Member Author

Works! So nice to get this back operating 👍

@RichardLitt RichardLitt added the exp/expert Having worked on the specific codebase is important label Feb 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exp/expert Having worked on the specific codebase is important kind/bug A bug in existing code (including security flaws) topic/gateway Topic gateway
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants