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.

Sign up for GitHub

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

Jump to bottom

gateway: use CID as an ETag strong validator #3869

Merged
merged 3 commits into from
Apr 20, 2017
Merged

gateway: use CID as an ETag strong validator #3869

merged 3 commits into from
Apr 20, 2017

Conversation

recmo
Copy link

@recmo recmo commented Apr 18, 2017

  • Always use the fully resolved CID from api.ResolveNode
    as the ETag (also for IPNS).

  • Format the result as a valid "Strong Validator"
    (double quotes around the encoded CID).

Fixes #3868

License: MIT
Signed-off-by: Remco Bloemen <remco@2π.com>

gateway: use CID as an ETag strong validator
8db6f86 
* Always use the fully resolved CID from api.ResolveNode
  as the ETag (also for IPNS).

* Format the result as a valid "Strong Validator"
  (double quotes around the encoded CID).

Fixes ipfs#3868

License: MIT
Signed-off-by: Remco Bloemen <remco@2π.com>
@recmo
Copy link
Author

recmo commented Apr 18, 2017

This implementation effectively calls api.ResolveNode twice. First as part of Unixfs().Cat(), and then directly. If it is a heavy operation, I can inline the Cat() function and save a call.

@ghost
Copy link

ghost commented Apr 18, 2017

Inlining Cat() would mean reintroducing the dependency on unixfs/io. We've been trying to trim down the gateway's dependencies so it can eventually be extracted to ipfs/go-ipfs-gateway.

The gateway's directory listings are another, already existing case of double-read.

Other options for making this more efficient:

  • Change Cat() signature to also return the resolved coreiface.Path, which includes the Cid -- mmmmh.
  • Add equivalent unixfs functions which accept an ipld.Node (from ResolveNode()) instead of a coreiface.Path. CatNode() for the ETag case, and LsNode() for the directory listings case.

I kinda like the latter, I can see these come in handy when working with ipld nodes, but not actually wanting to store them into ipfs just to work with them. We'd need to look at how this would play with other parts of the Core API (object, block, dag).

But I'm also fine with leaving the fix to double-reads to another PR.

@recmo
Copy link
Author

recmo commented Apr 18, 2017

I like the idea of passing ipld.Node to the unixfs functions. But I don't feel confident enough to change their APIs. I think I have a simpler solution, let me try something.

@recmo recmo force-pushed the fix/gateway/3868-etag branch 2 times, most recently from 0db1fe1 to 606f7c4 Compare April 18, 2017 16:46
@Kubuxu Kubuxu self-requested a review April 18, 2017 18:04
Kubuxu
Kubuxu reviewed Apr 18, 2017
View reviewed changes
core/corehttp/gateway_handler.go Outdated
defer dr.Close()
case coreiface.ErrIsDir:
dir = true
break
Copy link
Member

Choose a reason for hiding this comment

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

There is no need for break here.

Copy link
Author

Choose a reason for hiding this comment

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

It's gone now :)

@Kubuxu Kubuxu requested a review from a user April 18, 2017 19:30
Kubuxu
Kubuxu approved these changes Apr 18, 2017
View reviewed changes
Copy link
Member

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

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

I still want @lgierth to CR it

ghost
ghost suggested changes Apr 18, 2017
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Awesome, thanks a bunch! One tiny change, otherwise it LGTM 👍

core/corehttp/gateway_handler.go Outdated
case coreiface.ErrOffline:
if !i.node.OnlineMode() {
webError(w, "ipfs cat "+urlPath, err, http.StatusServiceUnavailable)
webError(w, "ipfs resolve "+urlPath, err, http.StatusServiceUnavailable)
Copy link

Choose a reason for hiding this comment

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

Actually the CLI equivalent of this is ipfs resolve -r :) Better get it right so that the few who actually paste this into a terminal don't get confused.

Copy link
Author

Choose a reason for hiding this comment

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

Done! Didn't know it was meant to paste in terminals—great feature :)

@ghost ghost added this to the Ipfs 0.4.9 milestone Apr 19, 2017
@ghost ghost added the topic/gateway Topic gateway label Apr 19, 2017
@Kubuxu
Copy link
Member

Kubuxu commented Apr 19, 2017

You've changed the error message but not the test and not it is failing.

gateway: re-use resolved path
a90f496 
Instead of resolving a node, we resolve a path. This resolved path
is then re-used for Cat and Ls. This way, a resolve operation is
only done once.

The error messages for a failed resolve is changed from `ipfs cat …`
to `ipfs resolve …` to better reflect the API calls. The test is
updated accordingly.

License: MIT
Signed-off-by: Remco Bloemen <remco@2π.com>
@recmo
Copy link
Author

recmo commented Apr 20, 2017

@Kubuxu Sorry, fixed it now!

@Kubuxu
Copy link
Member

Kubuxu commented Apr 20, 2017 •
edited
Loading

While trying to use this I get whole bunch of following errors: See next message

Looks like something is wrong with the core API, I will explore.
@recmo everything here looks good just something below is not behaving.

@Kubuxu
Copy link
Member

Kubuxu commented Apr 20, 2017 •
edited
Loading

And now it works.

Looks like it is combination of timeouts, short ipfs.io TTL and not so great connection. Not related to this PR.

@Kubuxu Kubuxu mentioned this pull request Apr 20, 2017
Closed
@Kubuxu
Copy link
Member

Kubuxu commented Apr 20, 2017

@recmo could you resolve the conflict by rebase. It should be quite easy. Sorry about that.

@Kubuxu
Copy link
Member

Kubuxu commented Apr 20, 2017

I was able to resolve the conflict via GH UI, not sure it was the best idea as it created merge commit.

@whyrusleeping whyrusleeping merged commit b3c20aa into ipfs:master Apr 20, 2017
@Kubuxu
Copy link
Member

Kubuxu commented Apr 20, 2017

@recmo thank you very much. It should significantly increase performance of non-local gateways and cache proxies.

@recmo
Copy link
Author

recmo commented Apr 20, 2017

Thanks for merging! This will help our deployment significantly.

hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023
@whyrusleeping
Merge pull request ipfs/kubo#3869 from Recmo/fix/gateway/3868-etag
04e40b1 
gateway: use CID as an ETag strong validator

This commit was moved from ipfs/kubo@b3c20aa
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kubuxu Kubuxu Kubuxu approved these changes

Assignees
No one assigned
Labels
topic/gateway Topic gateway
Projects
None yet
Milestone
Ipfs 0.4.9
Development

Successfully merging this pull request may close these issues.

ETag behaviour violates RFC7232 and related issues
3 participants
@recmo @Kubuxu @whyrusleeping