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

Set cache headers for 404 and 400 to 0 #649

Merged
merged 5 commits into from
Oct 29, 2020

Conversation

ruflin
Copy link
Contributor

@ruflin ruflin commented Oct 29, 2020

Fastly caches 404 headers by default. This could cause issues when rolling out a new registry and a path was requested to early. This sets the cache time to 0.

Fastly docs: https://docs.fastly.com/en/guides/http-status-codes-cached-by-default

Fastly caches 404 headers by default. This could cause issues when rolling out a new registry and a path was requested to early. This sets the cache time to 0.

Fastly docs: https://docs.fastly.com/en/guides/http-status-codes-cached-by-default
@ruflin ruflin requested a review from mtojek October 29, 2020 08:01
@ruflin ruflin self-assigned this Oct 29, 2020
@elasticmachine
Copy link

elasticmachine commented Oct 29, 2020

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #649 updated]

  • Start Time: 2020-10-29T15:25:07.625+0000

  • Duration: 7 min 12 sec

Test stats 🧪

Test Results
Failed 0
Passed 161
Skipped 0
Total 161

handler.go Outdated
@@ -17,10 +17,12 @@ import (
var errResourceNotFound = errors.New("resource not found")

func notFoundError(w http.ResponseWriter, err error) {
cacheHeaders(w, 0)
Copy link
Contributor

@mtojek mtojek Oct 29, 2020

Choose a reason for hiding this comment

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

I think you need to change the internal implementation. The recommended for Cache-Control value for not cachable resources is:
Cache-Control: no-cache, no-store, must-revalidate

see: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control (Preventing caching)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm.. "private" is to inform the intermittent servers not to cache data, because they will be cached by browsers. To prevent caching on the browser level, AFAIK you need to add Max-Age: 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Referring to the Mozilla docs, Cache-Control: no-store should be sufficient at this point:

no-store
The response may not be stored in any cache. Although other directives may be set, this alone is the only directive you need in preventing cached responses on modern browsers. max-age=0 is already implied. Setting must-revalidate does not make sense because in order to go through revalidation you need the response to be stored in a cache, which no-store prevents.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fastly does not respect no-store:

Fastly does not currently respect no-store or no-cache directives. Including either or both of these in a Cache-Control header has no effect on Fastly's caching decision, unless you alter this behavior using custom VCL.

@ruflin
Copy link
Contributor Author

ruflin commented Oct 29, 2020

This now sets

Cache-Control: max-age=0
Cache-Control: "private, no-store"

This should work for browser and especially Fastly too.

@mtojek mtojek self-requested a review October 29, 2020 15:29
@ruflin ruflin merged commit cfb3165 into elastic:master Oct 29, 2020
@ruflin ruflin deleted the not-found-headers branch October 29, 2020 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants