From 7f5aa71a09d21a19e9c61e3d953477c27b6b991c Mon Sep 17 00:00:00 2001 From: Johan Kiviniemi Date: Sat, 31 Oct 2015 08:08:04 +0200 Subject: [PATCH] Gateway: Add Cache-Control/max-age to IPNS requests License: MIT Signed-off-by: Johan Kiviniemi --- core/corehttp/gateway_handler.go | 24 ++-- core/corehttp/gateway_test.go | 63 ++++++++-- test/sharness/t0110-gateway.sh | 191 +++++++++++++++++++++++++------ 3 files changed, 222 insertions(+), 56 deletions(-) diff --git a/core/corehttp/gateway_handler.go b/core/corehttp/gateway_handler.go index d9864c051465..74c1581d1ea9 100644 --- a/core/corehttp/gateway_handler.go +++ b/core/corehttp/gateway_handler.go @@ -111,7 +111,7 @@ func (i *gatewayHandler) getOrHeadHandler(w http.ResponseWriter, r *http.Request return } - nd, err := core.Resolve(ctx, i.node, path.Path(urlPath)) + nd, ttl, err := core.ResolveDetailed(ctx, i.node, path.Path(urlPath)) if err != nil { webError(w, "Path Resolve error", err, http.StatusBadRequest) return @@ -146,15 +146,15 @@ func (i *gatewayHandler) getOrHeadHandler(w http.ResponseWriter, r *http.Request return } - // set these headers _after_ the error, for we may just not have it - // and dont want the client to cache a 500 response... - // and only if it's /ipfs! - // TODO: break this out when we split /ipfs /ipns routes. + // 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? + setSuccessHeaders(w, ttl) + modtime := time.Now() if strings.HasPrefix(urlPath, ipfsPathPrefix) { w.Header().Set("Etag", etag) - w.Header().Set("Cache-Control", "public, max-age=29030400") - // set modtime to a really long time ago, since files are immutable and should stay cached modtime = time.Unix(1, 0) } @@ -446,6 +446,7 @@ func webError(w http.ResponseWriter, message string, err error, defaultCode int) } func webErrorWithCode(w http.ResponseWriter, message string, err error, code int) { + unsetSuccessHeaders(w) w.WriteHeader(code) log.Errorf("%s: %s", message, err) // TODO(cryptix): log errors until we have a better way to expose these (counter metrics maybe) fmt.Fprintf(w, "%s: %s", message, err) @@ -455,3 +456,12 @@ func webErrorWithCode(w http.ResponseWriter, message string, err error, code int func internalWebError(w http.ResponseWriter, err error) { webErrorWithCode(w, "internalWebError", err, http.StatusInternalServerError) } + +func setSuccessHeaders(w http.ResponseWriter, ttl time.Duration) { + w.Header().Set("Cache-Control", fmt.Sprintf("public, max-age=%d", int(ttl.Seconds()))) +} + +func unsetSuccessHeaders(w http.ResponseWriter) { + w.Header().Del("Cache-Control") + w.Header().Del("ETag") +} diff --git a/core/corehttp/gateway_test.go b/core/corehttp/gateway_test.go index 829a988329f5..41dc837d38aa 100644 --- a/core/corehttp/gateway_test.go +++ b/core/corehttp/gateway_test.go @@ -2,6 +2,7 @@ package corehttp import ( "errors" + "fmt" "io/ioutil" "net/http" "net/http/httptest" @@ -20,7 +21,12 @@ import ( testutil "github.com/ipfs/go-ipfs/util/testutil" ) -type mockNamesys map[string]path.Path +type mockNamesys map[string]mockEntry + +type mockEntry struct { + path path.Path + ttl time.Duration +} func (m mockNamesys) Resolve(ctx context.Context, name string) (path.Path, error) { p, _, err := m.ResolveDetailed(ctx, name) @@ -37,11 +43,11 @@ func (m mockNamesys) ResolveDetailed(ctx context.Context, name string) (path.Pat } func (m mockNamesys) ResolveNDetailed(ctx context.Context, name string, depth int) (path.Path, time.Duration, error) { - p, ok := m[name] + entry, ok := m[name] if !ok { return "", 0, namesys.ErrResolveFailed } - return p, 0, nil + return entry.path, entry.ttl, nil } func (m mockNamesys) Publish(ctx context.Context, name ci.PrivKey, value path.Path) error { @@ -124,21 +130,27 @@ func TestGatewayGet(t *testing.T) { if err != nil { t.Fatal(err) } - ns["/ipns/example.com"] = path.FromString("/ipfs/" + k) + kTTL := 10 * time.Minute + ns["/ipns/example.com"] = mockEntry{ + path: path.FromString("/ipfs/" + k), + ttl: kTTL, + } t.Log(ts.URL) + immutableTTL := namesys.ImmutableTTL // Need a pointer to the constant for _, test := range []struct { host string path string status int + ttl *time.Duration text string }{ - {"localhost:5001", "/", http.StatusNotFound, "404 page not found\n"}, - {"localhost:5001", "/" + k, http.StatusNotFound, "404 page not found\n"}, - {"localhost:5001", "/ipfs/" + k, http.StatusOK, "fnord"}, - {"localhost:5001", "/ipns/nxdomain.example.com", http.StatusBadRequest, "Path Resolve error: " + namesys.ErrResolveFailed.Error()}, - {"localhost:5001", "/ipns/example.com", http.StatusOK, "fnord"}, - {"example.com", "/", http.StatusOK, "fnord"}, + {"localhost:5001", "/", http.StatusNotFound, nil, "404 page not found\n"}, + {"localhost:5001", "/" + k, http.StatusNotFound, nil, "404 page not found\n"}, + {"localhost:5001", "/ipfs/" + k, http.StatusOK, &immutableTTL, "fnord"}, + {"localhost:5001", "/ipns/nxdomain.example.com", http.StatusBadRequest, nil, "Path Resolve error: " + namesys.ErrResolveFailed.Error()}, + {"localhost:5001", "/ipns/example.com", http.StatusOK, &kTTL, "fnord"}, + {"example.com", "/", http.StatusOK, &kTTL, "fnord"}, } { var c http.Client r, err := http.NewRequest("GET", ts.URL+test.path, nil) @@ -158,6 +170,7 @@ func TestGatewayGet(t *testing.T) { t.Errorf("got %d, expected %d from %s", resp.StatusCode, test.status, urlstr) continue } + checkCacheControl(t, urlstr, resp, test.ttl) body, err := ioutil.ReadAll(resp.Body) if err != nil { t.Fatalf("error reading response from %s: %s", urlstr, err) @@ -199,7 +212,11 @@ func TestIPNSHostnameRedirect(t *testing.T) { t.Fatal(err) } t.Logf("k: %s\n", k) - ns["/ipns/example.net"] = path.FromString("/ipfs/" + k.String()) + kTTL := 10 * time.Minute + ns["/ipns/example.net"] = mockEntry{ + path: path.FromString("/ipfs/" + k.String()), + ttl: kTTL, + } // make request to directory containing index.html req, err := http.NewRequest("GET", ts.URL+"/foo", nil) @@ -223,6 +240,7 @@ func TestIPNSHostnameRedirect(t *testing.T) { } else if hdr[0] != "/foo/" { t.Errorf("location header is %v, expected /foo/", hdr[0]) } + checkCacheControl(t, "http://example.net/foo", res, &kTTL) } func TestIPNSHostnameBacklinks(t *testing.T) { @@ -260,7 +278,11 @@ func TestIPNSHostnameBacklinks(t *testing.T) { t.Fatal(err) } t.Logf("k: %s\n", k) - ns["/ipns/example.net"] = path.FromString("/ipfs/" + k.String()) + kTTL := 10 * time.Minute + ns["/ipns/example.net"] = mockEntry{ + path: path.FromString("/ipfs/" + k.String()), + ttl: kTTL, + } // make request to directory listing req, err := http.NewRequest("GET", ts.URL+"/foo/", nil) @@ -292,6 +314,8 @@ func TestIPNSHostnameBacklinks(t *testing.T) { t.Fatalf("expected file in directory listing") } + checkCacheControl(t, "http://example.net/foo/", res, &kTTL) + // make request to directory listing req, err = http.NewRequest("GET", ts.URL, nil) if err != nil { @@ -322,6 +346,8 @@ func TestIPNSHostnameBacklinks(t *testing.T) { t.Fatalf("expected file in directory listing") } + checkCacheControl(t, "http://example.net/", res, &kTTL) + // make request to directory listing req, err = http.NewRequest("GET", ts.URL+"/foo/bar/", nil) if err != nil { @@ -351,4 +377,17 @@ func TestIPNSHostnameBacklinks(t *testing.T) { if !strings.Contains(s, "") { t.Fatalf("expected file in directory listing") } + + checkCacheControl(t, "http://example.net/foo/bar/", res, &kTTL) +} + +func checkCacheControl(t *testing.T, urlstr string, resp *http.Response, ttl *time.Duration) { + expCacheControl := "" + if ttl != nil { + expCacheControl = fmt.Sprintf("public, max-age=%d", int(ttl.Seconds())) + } + cacheControl := resp.Header.Get("Cache-Control") + if cacheControl != expCacheControl { + t.Errorf("unexpected Cache-Control header from %s: expected %q; got %q", urlstr, expCacheControl, cacheControl) + } } diff --git a/test/sharness/t0110-gateway.sh b/test/sharness/t0110-gateway.sh index c15832088484..9f54fb064961 100755 --- a/test/sharness/t0110-gateway.sh +++ b/test/sharness/t0110-gateway.sh @@ -22,59 +22,176 @@ apiport=$PORT_API # define a function to test a gateway, and do so for each port. # for now we check 5001 here as 5002 will be checked in gateway-writable. -test_expect_success "GET IPFS path succeeds" ' - echo "Hello Worlds!" >expected && - HASH=$(ipfs add -q expected) && - curl -sfo actual "http://127.0.0.1:$port/ipfs/$HASH" -' +TEST_COMMAND=test_expect_success -test_expect_success "GET IPFS path output looks good" ' - test_cmp expected actual && - rm actual -' +# Overwrites: actual.headers, actual. +test_run_curl() { + rm -f actual.headers actual + curl -s -D actual.headers -o actual "$@" +} -test_expect_success "GET IPFS path on API forbidden" ' - test_curl_resp_http_code "http://127.0.0.1:$apiport/ipfs/$HASH" "HTTP/1.1 403 Forbidden" -' +test_actual_headers_http_status() { + test_should_contain '^HTTP/1.1 '"$1"'.$' actual.headers +} + +test_get_max_age() { + perl -nwe 's/^Cache-Control:.* max-age=([0-9]+).*$/$1/ && print' "$1" +} -test_expect_success "GET IPFS directory path succeeds" ' - mkdir dir && - echo "12345" >dir/test && - ipfs add -r -q dir >actual && +# Overwrites: actual.max-age, expected.max-age. +test_expect_max_age() { + test_get_max_age actual.headers >actual.max-age + printf "%s\n" "$1" >expected.max-age + test_cmp expected.max-age actual.max-age || test_fsh cat actual.headers +} + +# Overwrites: actual.max-age, expected.max-age. +test_expect_no_max_age() { + test_get_max_age actual.headers >actual.max-age + : >expected.max-age + test_cmp expected.max-age actual.max-age || test_fsh cat actual.headers +} + +test_item() { + _item="$1" + _url="$2" + _http_status="$3" + _expected_output_file="$4" + _max_age="$5" + + "$TEST_COMMAND" "$_item: responds with $_http_status" ' + test_run_curl "$_url" && + test_actual_headers_http_status "$_http_status" + ' + + case "$_expected_output_file" in + # Make sure a skipped output test is conspicuous at the call site. + IGNORE_OUTPUT) ;; + *) + "$TEST_COMMAND" "$_item: output looks good" ' + test_cmp "$_expected_output_file" actual + ' + ;; + esac + + case "$_max_age" in + IGNORE_MAX_AGE) ;; + NO_MAX_AGE) + "$TEST_COMMAND" "$_item: has no Cache-Control max-age" ' + test_expect_no_max_age + ' + ;; + *) + "$TEST_COMMAND" "$_item: has Cache-Control max-age=$_max_age" ' + test_expect_max_age "$_max_age" + ' + ;; + esac +} + +IMMUTABLE_TTL=$((10*365*24*60*60)) +UNKNOWN_TTL=60 + +# DATA1, HASH1: a file +# DATA2, HASH2: a directory +# PEERID: the node ID +test_expect_success "Generating test environment" ' + DATA1=data1 && + echo "Hello Worlds!" >"$DATA1" && + HASH1=$(ipfs add -q "$DATA1") && + DATA2=data2 && + mkdir "$DATA2" && + echo "12345" >"$DATA2/test" && + mkdir "$DATA2/has-index-html" && + echo "index" >"$DATA2/has-index-html/index.html" && + ipfs add -r -q "$DATA2" >actual && HASH2=$(tail -n 1 actual) && - curl -sf "http://127.0.0.1:$port/ipfs/$HASH2" + PEERID=$(ipfs config Identity.PeerID) && + test_check_peerid "$PEERID" ' -test_expect_success "GET IPFS directory file succeeds" ' - curl -sfo actual "http://127.0.0.1:$port/ipfs/$HASH2/test" -' +test_item "GET IPFS path" "http://127.0.0.1:$port/ipfs/$HASH1" \ + "200 OK" "$DATA1" "$IMMUTABLE_TTL" -test_expect_success "GET IPFS directory file output looks good" ' - test_cmp dir/test actual -' +test_item "GET IPFS path on API" "http://127.0.0.1:$apiport/ipfs/$HASH1" \ + "403 Forbidden" IGNORE_OUTPUT NO_MAX_AGE -test_expect_success "GET IPFS non existent file returns code expected (404)" ' - test_curl_resp_http_code "http://127.0.0.1:$port/ipfs/$HASH2/pleaseDontAddMe" "HTTP/1.1 404 Not Found" +ITEM="GET IPFS directory path" +test_item "$ITEM" "http://127.0.0.1:$port/ipfs/$HASH2" \ + "200 OK" IGNORE_OUTPUT "$IMMUTABLE_TTL" +"$TEST_COMMAND" "$ITEM: output looks good" ' + test_should_contain "Index of /ipfs/$HASH2" actual ' -test_expect_failure "GET IPNS path succeeds" ' - ipfs name publish "$HASH" && - PEERID=$(ipfs config Identity.PeerID) && - test_check_peerid "$PEERID" && - curl -sfo actual "http://127.0.0.1:$port/ipns/$PEERID" +test_item "GET IPFS directory file" "http://127.0.0.1:$port/ipfs/$HASH2/test" \ + "200 OK" "$DATA2/test" "$IMMUTABLE_TTL" + +ITEM="GET IPFS directory path with index.html" +test_item "$ITEM" "http://127.0.0.1:$port/ipfs/$HASH2/has-index-html" \ + "302 Found" IGNORE_OUTPUT "$IMMUTABLE_TTL" +"$TEST_COMMAND" "$ITEM: redirects to path/" ' + test_should_contain "^Location: /ipfs/$HASH2/has-index-html/.\$" actual.headers ' -test_expect_failure "GET IPNS path output looks good" ' - test_cmp expected actual +test_item "GET IPFS directory path/ with index.html" \ + "http://127.0.0.1:$port/ipfs/$HASH2/has-index-html/" \ + "200 OK" "$DATA2/has-index-html/index.html" "$IMMUTABLE_TTL" + +test_item "GET IPFS non-existent file" \ + "http://127.0.0.1:$port/ipfs/$HASH2/pleaseDontAddMe" \ + "404 Not Found" IGNORE_OUTPUT NO_MAX_AGE + +# https://github.com/ipfs/go-ipfs/issues/1941 sharness suite: ipfs name +# publish: “Error: failed to find any peer in table” +TEST_COMMAND=test_expect_failure + +TTL=10 + +ITEM="GET IPNS path" +"$TEST_COMMAND" "$ITEM: IPNS publish with TTL $TTL succeeds" ' + ipfs name publish --ttl="${TTL}s" "$HASH1" ' +test_item "$ITEM" "http://127.0.0.1:$port/ipns/$PEERID" \ + "200 OK" "$DATA1" "$TTL" +test_timer_start EXPIRY_TIMER "${TTL}s" # The cache entry has expired when this finishes. -test_expect_success "GET invalid IPFS path errors" ' - test_must_fail curl -sf "http://127.0.0.1:$port/ipfs/12345" +ITEM="GET IPNS path again before cache expiry" +# Publish a new version now, the previous version should still be cached. The +# next item will resolve this version. +"$TEST_COMMAND" "$ITEM: IPNS publish with default TTL succeeds" ' + ipfs name publish "$HASH2" +' +go-sleep 1s # Ensure the following max-age is strictly less than TTL. +test_item "$ITEM" "http://127.0.0.1:$port/ipns/$PEERID" \ + "200 OK" "$DATA1" IGNORE_MAX_AGE +"$TEST_COMMAND" "$ITEM: has Cache-Control max-age between 0 and $TTL" ' + max_age="$(test_get_max_age actual.headers)" && + test "$max_age" -gt 0 && + test "$max_age" -lt "$TTL" || + test_fsh cat actual.headers ' -test_expect_success "GET invalid path errors" ' - test_must_fail curl -sf "http://127.0.0.1:$port/12345" +# Make sure the expiry timer is still running, otherwise the result might be +# wrong. If this fails, we will need to increase TTL above to give enough time +# for the tests. +test_expect_success "$ITEM: tests did not take too long" ' + test_timer_is_running "$EXPIRY_TIMER" ' +test_expect_success "$ITEM: previous version is no longer cached" ' + test_timer_wait "$EXPIRY_TIMER" +' + +test_item "GET IPNS path again after cache expiry" \ + "http://127.0.0.1:$port/ipns/$PEERID/test" \ + "200 OK" "$DATA2/test" "$UNKNOWN_TTL" + +TEST_COMMAND=test_expect_success + +test_item "GET invalid IPFS path" "http://127.0.0.1:$port/ipfs/12345" \ + "400 Bad Request" IGNORE_OUTPUT NO_MAX_AGE + +test_item "GET invalid root path" "http://127.0.0.1:$port/12345" \ + "404 Not Found" IGNORE_OUTPUT NO_MAX_AGE test_expect_success "GET /webui returns code expected" ' test_curl_resp_http_code "http://127.0.0.1:$apiport/webui" "HTTP/1.1 302 Found" "HTTP/1.1 301 Moved Permanently" @@ -103,7 +220,7 @@ test_expect_success "get IPFS directory file through readonly API succeeds" ' ' test_expect_success "get IPFS directory file through readonly API output looks good" ' - test_cmp dir/test actual + test_cmp "$DATA2/test" actual ' test_expect_success "refs IPFS directory file through readonly API succeeds" '