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

Add cache headers. #132

Merged
merged 3 commits into from
Feb 1, 2017
Merged

Add cache headers. #132

merged 3 commits into from
Feb 1, 2017

Conversation

stephank
Copy link
Member

Fixes #41.

The approach here is to just set Cache-Control: no-cache, no-store for everything, except when overridden explicitly. The overrides are for static files, discovery and keys, each having a config var.

The discovery and keys resources don't set Last-Modified and Etag. I feel like it's more trouble than it's worth?

@stephank stephank force-pushed the featureCacheHeaders branch 2 times, most recently from c3330ae to 6e76fc4 Compare January 26, 2017 09:37
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 84f9770 on featureCacheHeaders into ** on master**.

Copy link
Member

@onli onli left a comment

Choose a reason for hiding this comment

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

Looks good. I agree that Etag and last-modified are not necessary to make this addition worthwhile.

=============== ==================== ================
static_ttl BROKER_STATIC_TTL 604800 (1 week)
discovery_ttl BROKER_DISCOVERY_TTL 604800 (1 week)
keys_ttl BROKER_KEYS_TTL 86400 (1 day)
Copy link
Member

Choose a reason for hiding this comment

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

Having the key cacheable is nice, though also problematic if combined with the key rotation of #121 (comment). If clients were to really respect the cache (which I doubt any client does, but let's assume) and the rotation happens during the time, validation should fail.

At first I wanted to suggest to you to document that problematic, but since key rotation is external to the broker, I wouldn't know where would be a good place for that. So in the end I'm just leaving this as a comment here.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling b73a6c4 on featureCacheHeaders into ** on master**.

@stephank stephank mentioned this pull request Feb 1, 2017
2 tasks
@stephank
Copy link
Member Author

stephank commented Feb 1, 2017

I created #134, so we won't forget! :)

@stephank stephank merged commit 94dabda into master Feb 1, 2017
@stephank stephank deleted the featureCacheHeaders branch February 1, 2017 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants