Gateway: Add Cache-Control/max-age and ETag to IPNS requests #1921

ion1 · 2015-10-31T10:27:45Z

Based on #1887 “cache ipns entries to speed things up a little”.

Addresses https://github.com/ipfs/go-ipfs/issues/1818.

This is my first time learning Go, I hope I’m doing things somewhat correctly.

jbenet · 2015-11-02T23:24:48Z

core/corehttp/gateway_handler.go

-	// and only if it's /ipfs!
-	// TODO: break this out when we split /ipfs /ipns routes.
+	w.Header().Set("ETag", etag)
+	w.Header().Set("Cache-Control", fmt.Sprintf("public, max-age=%d", int(res.TTL.Seconds())))


nope, errors still happen below. this is trickty! -- maybe the error functions should clear this header to be sure.

ensure it's /ipfs/ route. code evolves!

ion1 · 2015-11-03T01:07:25Z

Added code to unset Cache-Control and ETag in the webError functions.
Checked that all error responses in getOrHeadHandler go through those functions.
Added a comment reminding to unset the headers in error responses if not using the functions.

jbenet · 2015-11-03T01:10:25Z

Ok, this LGTM to me. @ion1 last thing is we should test some of this using sharness tests, so that the api tests are carried to other impls eventually.

ion1 · 2015-11-04T01:49:12Z

Added sharness tests.
Noticed that we weren’t following the RFC for ETags. Quote marks around the tag are mandatory. Fixed.

The sharness tests involving IPNS were test_expect_failure because ipfs name publish fails with Error: failed to find any peer in table.

All the new IPNS tests in the PR are test_expect_successes at the moment, and I verified them to succeed in a modified environment where they have access to the swarm.

Should I leave them as they are (which leaves the test suite failing until the publish problem is fixed) or change them all to test_expect_failures?

ion1 · 2015-11-04T02:02:29Z

I created #1941, changed them to test_expect_failures and added a comment with a link to the issue.

jbenet · 2015-11-08T15:15:48Z

@ion1 Looking much better! some comments above.

ion1 · 2015-11-08T20:57:37Z

@jbenet, thanks for your input.

Done:

Make sure the exact same commands are tested while offline and while online whereever possible in t0160-resolve.sh.
- Added test_expect_success_1941 to test-lib.sh as a workaround to sharness suite: ipfs name publish: “Error: failed to find any peer in table” #1941 to facilitate using the same test code for the offline and the online cases. Once sharness suite: ipfs name publish: “Error: failed to find any peer in table” #1941 is fixed, test_expect_success_1941 can be globally replaced with just test_expect_success.
Rename ResolveDetailed as ResolveWithTTL.
Retain the named return values in gateway_test.go.
Make test_item more readable (I hope) in t0110-gateway.sh.
Do not use E[Tt]ag, just do a case-insensitive match.

ion1 · 2015-11-08T21:09:01Z

The Travis failure does not seem to have anything to do with my changes.

jbenet · 2015-11-10T06:45:45Z

core/corehttp/gateway_handler.go

+	// Remember to unset the Cache-Control/ETag headers before error
+	// responses!  The webError functions unset them automatically.
+	// TODO: Consider letting clients cache 404s.  Are there cases where
+	// that would hurt?


yes let's not cache 404s anytime soon.

It’s unclear to me where it would hurt, though. Given a successfully resolved /ipfs/QmFoo, the knowledge that “/ipfs/QmFoo/fileA exists” should be as cacheable as “/ipfs/QmFoo/fileB does not exist” for example. QmFoo is immutable after all.

The same applies to /ipns/QmBar. The record’s TTL dictates that the knowledge that “/ipns/QmBar/fileA exists” can be cached for just as long as that “/ipns/QmBar/fileB does not exist”.

i think @ion1 has a point here, but its going to be difficult to do it right. A 404 should only be cached if its for a link under a valid resolved hash. A 'could not find this hash on the network' 404 should never be cached.

The code should already return a TTL of 0 for all network failures.

jbenet · 2015-11-10T06:57:07Z

@ion1 thanks -- more CR. and rebase to keep abreast of recent merges.

Closes ipfs#1934. License: MIT Signed-off-by: Johan Kiviniemi <[email protected]>

License: MIT Signed-off-by: Johan Kiviniemi <[email protected]>

Closes ipfs/go-ipfs#1818. License: MIT Signed-off-by: Johan Kiviniemi <[email protected]>

ion1 · 2015-11-13T18:27:06Z

@jbenet, thanks for your input.

Done:

Rebase.
Add util/infduration with the constructors InfiniteDuration() and FiniteDuration(d), use it in the TTL code.

I’m looking forward to your response to my comment about test_expect_success_1941. I’d like to emphasize that pending a fix for #1941, the tests running ipfs name publish while the daemon is running with zero peers will fail unconditionally.

jbenet · 2015-11-14T14:17:34Z

test/sharness/t0100-name.sh

@@ -10,12 +10,14 @@ test_description="Test ipfs repo operations"

 test_init_ipfs

+for TTL_PARAMS in "" "--ttl=0s" "--ttl=1h"; do


wow the rest of the file is in a for loop without indentation. didnt even catch this the first time. please dont do that.

do this instead:

somefunc() { TTL=$1 all the tests here } for TTL in "" "--ttl=0s" "--ttl=1h"; do somefunc $TTL done

whyrusleeping · 2015-11-16T17:48:16Z

@ion1 since this PR touches a lot of files, I'd prefer if we put this on 0.4.0. Give rebasing it a shot, let me know how it goes.

whyrusleeping · 2015-11-16T17:51:49Z

also, the infinite duration stuff (which i'm still not convinced is better than just using -1 as a special value) should go in thirdparty, i'm working on shrinking the util package.

whyrusleeping · 2016-01-29T17:42:37Z

@ion1 update here?

ion1 · 2016-01-29T22:14:50Z

@whyrusleeping: I have been waiting for a fix for #1941 which prevents the tests in this PR from working. I will rebase this and address the other remaining comments at that point.

whyrusleeping · 2017-03-24T17:15:05Z

Has been idle for about a year. Closing for now. Please do feel free to reopen this as necessary. Closing old PRs just helps us more easily see what needs immediate attention

jbenet added the backlog label Oct 31, 2015

ion1 force-pushed the feat/ipns-cache-resolve branch from f008ecb to e905204 Compare October 31, 2015 16:24

ion1 mentioned this pull request Nov 1, 2015

cache ipns entries to speed things up a little #1887

Merged

ion1 force-pushed the feat/ipns-cache-resolve branch 8 times, most recently from 6e96a45 to 85b8962 Compare November 1, 2015 22:12

ion1 mentioned this pull request Nov 2, 2015

Sprint Oct 26 ipfs/team-mgmt#41

Closed

47 tasks

ion1 force-pushed the feat/ipns-cache-resolve branch 4 times, most recently from 338a8ac to 763370f Compare November 2, 2015 23:14

jbenet reviewed Nov 2, 2015
View reviewed changes

ion1 force-pushed the feat/ipns-cache-resolve branch from 763370f to 42060d8 Compare November 3, 2015 01:04

ion1 force-pushed the feat/ipns-cache-resolve branch from 42060d8 to deb7184 Compare November 4, 2015 01:39

ion1 force-pushed the feat/ipns-cache-resolve branch from deb7184 to 2765be6 Compare November 4, 2015 02:00

ion1 force-pushed the feat/ipns-cache-resolve branch 6 times, most recently from c0b09e0 to b020415 Compare November 5, 2015 00:50

ion1 force-pushed the feat/ipns-cache-resolve branch from 6ee526f to f44ebdb Compare November 8, 2015 20:56

jbenet reviewed Nov 10, 2015
View reviewed changes

ion1 added 6 commits November 13, 2015 03:08

sharness tests for caching and --ttl

7e828de

Closes ipfs#1934. License: MIT Signed-off-by: Johan Kiviniemi <[email protected]>

name publish: Forbid negative --ttl

52e567a

License: MIT Signed-off-by: Johan Kiviniemi <[email protected]>

routing: Refactor EOL computation out of cacheSet; sanity check for EOL

9fee188

License: MIT Signed-off-by: Johan Kiviniemi <[email protected]>

Make Resolver return TTL information

9338b40

License: MIT Signed-off-by: Johan Kiviniemi <[email protected]>

Gateway: Add Cache-Control/max-age to IPNS requests

274c261

License: MIT Signed-off-by: Johan Kiviniemi <[email protected]>

Gateway: Add ETag to IPNS requests

e312125

Closes ipfs/go-ipfs#1818. License: MIT Signed-off-by: Johan Kiviniemi <[email protected]>

ion1 force-pushed the feat/ipns-cache-resolve branch from f44ebdb to e312125 Compare November 13, 2015 18:15

jbenet reviewed Nov 14, 2015
View reviewed changes

This was referenced Nov 16, 2015

Sprint Nov 16 ipfs/team-mgmt#55

Closed

Sprint Nov 9 ipfs/team-mgmt#51

Closed

ghost added the topic/gateway Topic gateway label Dec 22, 2015

daviddias removed the backlog label Jan 2, 2016

Kubuxu mentioned this pull request Feb 2, 2016

resolving a node's own /ipns entry: trade-offs between consistency and partition tolerance #2278

Closed

whyrusleeping closed this Mar 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gateway: Add Cache-Control/max-age and ETag to IPNS requests #1921

Gateway: Add Cache-Control/max-age and ETag to IPNS requests #1921

ion1 commented Oct 31, 2015

jbenet Nov 2, 2015

jbenet Nov 2, 2015

ion1 commented Nov 3, 2015

jbenet commented Nov 3, 2015

ion1 commented Nov 4, 2015

ion1 commented Nov 4, 2015

jbenet commented Nov 8, 2015

ion1 commented Nov 8, 2015

ion1 commented Nov 8, 2015

jbenet Nov 10, 2015

ion1 Nov 13, 2015

whyrusleeping Nov 13, 2015

ion1 Nov 13, 2015

jbenet commented Nov 10, 2015

ion1 commented Nov 13, 2015

jbenet Nov 14, 2015

whyrusleeping commented Nov 16, 2015

whyrusleeping commented Nov 16, 2015

whyrusleeping commented Jan 29, 2016

ion1 commented Jan 29, 2016

whyrusleeping commented Mar 24, 2017

		@@ -10,12 +10,14 @@ test_description="Test ipfs repo operations"

		test_init_ipfs

		for TTL_PARAMS in "" "--ttl=0s" "--ttl=1h"; do

Gateway: Add Cache-Control/max-age and ETag to IPNS requests #1921

Gateway: Add Cache-Control/max-age and ETag to IPNS requests #1921

Conversation

ion1 commented Oct 31, 2015

jbenet Nov 2, 2015

Choose a reason for hiding this comment

jbenet Nov 2, 2015

Choose a reason for hiding this comment

ion1 commented Nov 3, 2015

jbenet commented Nov 3, 2015

ion1 commented Nov 4, 2015

ion1 commented Nov 4, 2015

jbenet commented Nov 8, 2015

ion1 commented Nov 8, 2015

ion1 commented Nov 8, 2015

jbenet Nov 10, 2015

Choose a reason for hiding this comment

ion1 Nov 13, 2015

Choose a reason for hiding this comment

whyrusleeping Nov 13, 2015

Choose a reason for hiding this comment

ion1 Nov 13, 2015

Choose a reason for hiding this comment

jbenet commented Nov 10, 2015

ion1 commented Nov 13, 2015

jbenet Nov 14, 2015

Choose a reason for hiding this comment

whyrusleeping commented Nov 16, 2015

whyrusleeping commented Nov 16, 2015

whyrusleeping commented Jan 29, 2016

ion1 commented Jan 29, 2016

whyrusleeping commented Mar 24, 2017