Skip to content

Update cache-control header#62014

Merged
legrego merged 14 commits intoelastic:masterfrom
legrego:chore/no-cache
Apr 6, 2020
Merged

Update cache-control header#62014
legrego merged 14 commits intoelastic:masterfrom
legrego:chore/no-cache

Conversation

@legrego
Copy link
Copy Markdown
Member

@legrego legrego commented Mar 31, 2020

This updates the default Cache-Control header to respond with private, no-cache, no-store, must-revalidate. The previous default header value was no-cache. Individual routes can still choose to respond with a custom cache-control header if they wish to do so.

Resolves #8530
Resolves #58169

@botelastic botelastic bot added Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Team:AppArch labels Mar 31, 2020
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@legrego
Copy link
Copy Markdown
Member Author

legrego commented Mar 31, 2020

@elastic/kibana-app-arch why did botelastic tag this PR with Team:AppArch and Feature:ExpressionLanguage?

body,
headers: {
'content-type': contentType,
'cache-control': 'private, no-cache, no-store',
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

note: the default cache control headers will apply to these custom responses as well. The existing SAML and OIDC functional tests will continue to assert this.

@legrego
Copy link
Copy Markdown
Member Author

legrego commented Apr 1, 2020

@elasticmachine merge upstream

@legrego
Copy link
Copy Markdown
Member Author

legrego commented Apr 2, 2020

@elasticmachine merge upstream

@legrego legrego marked this pull request as ready for review April 2, 2020 14:21
@legrego legrego requested review from a team as code owners April 2, 2020 14:21
Copy link
Copy Markdown
Contributor

@watson watson left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@legrego legrego requested a review from a team as a code owner April 2, 2020 15:03
Connection: 'keep-alive',
'Transfer-Encoding': 'chunked',
'Cache-Control': 'no-cache',
'Cache-Control': 'private, no-cache, no-store, must-revalidate',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As it's now the default value, can't this line just be removed?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yup, good catch

Comment on lines +39 to +42
cache: {
privacy: 'private',
otherwise: 'private, no-cache, no-store, must-revalidate',
},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see you added server_integration tests for it, but maybe we should also add a test for this in src/core/server/http/integration_tests/router.test.ts to check that 1/ this is the default value and 2/ route handlers can still override it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good idea, will do!

*/

// eslint-disable-next-line import/no-default-export
export default function({ getService }) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NIT: file could probably be in TS instead of JS.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I explored this, but in order to get really any benefit from the conversion, we also need to convert all of the server_integration services as well, and the type definitions for supertest are out of date (and essentially EOL), which is the only real external dependency, so I decided not to do the conversion as part of this PR

@legrego legrego removed the request for review from a team April 3, 2020 17:14
@legrego
Copy link
Copy Markdown
Member Author

legrego commented Apr 6, 2020

@elasticmachine merge upstream

Copy link
Copy Markdown
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

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

AppArch change LGTM.

@mshustov
Copy link
Copy Markdown
Contributor

mshustov commented Apr 6, 2020

@legrego closes #8530 #58169 ?

@legrego
Copy link
Copy Markdown
Member Author

legrego commented Apr 6, 2020

@legrego closes #8530 #58169 ?

Yes it does, I'll update the PR description to link these

@kibanamachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@legrego legrego merged commit 5003179 into elastic:master Apr 6, 2020
@legrego legrego deleted the chore/no-cache branch April 6, 2020 14:19
legrego added a commit to legrego/kibana that referenced this pull request Apr 6, 2020
* update cache-control header

* update tests

* update test run config

* remove custom cache-control header for authentication resources

* address test flakiness

* address PR feedback

* revert changes to endpoint test

* revert changes for real this time

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
legrego added a commit that referenced this pull request Apr 6, 2020
* update cache-control header

* update tests

* update test run config

* remove custom cache-control header for authentication resources

* address test flakiness

* address PR feedback

* revert changes to endpoint test

* revert changes for real this time

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update our Cache-Control header Prevent caching for dynamic content

7 participants