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

[Fleet][EPM] Assets responses will forward any EPR cache-control header #83680

Merged
merged 1 commit into from
Nov 19, 2020

Conversation

jfsiii
Copy link
Contributor

@jfsiii jfsiii commented Nov 18, 2020

closes #83631

Problem

Assets are served with a cache-control header that prevents any caching

Root cause

Likely from this code

cache: {
privacy: 'private',
otherwise: 'private, no-cache, no-store, must-revalidate',
},

Also based on these tests, it seems this is default/expected behavior

it('does not allow responses to be cached by default', async () => {
const { server: innerServer, createRouter } = await server.setup(setupDeps);
const router = createRouter('/');
router.get({ path: '/', validate: false, options: {} }, (context, req, res) => res.ok());
await server.start();
await supertest(innerServer.listener)
.get('/')
.expect('Cache-Control', 'private, no-cache, no-store, must-revalidate');
});

Proposed solution

Set the header via the response handler as shown in this test:

it('allows individual responses override the default cache-control header', async () => {
const { server: innerServer, createRouter } = await server.setup(setupDeps);
const router = createRouter('/');
router.get({ path: '/', validate: false, options: {} }, (context, req, res) =>
res.ok({
headers: {
'Cache-Control': 'public, max-age=1200',
},
})
);
await server.start();
await supertest(innerServer.listener).get('/').expect('Cache-Control', 'public, max-age=1200');
});

This PR

If this registry response contains a cache-control header, that value is included in the EPM response as well

In master, which points to epr-snapshot
Screen Shot 2020-11-18 at 12 33 47 PM
which matches https://epr-snapshot.elastic.co/package/apache/0.2.6/img/logo_apache.svg

or using epr.elastic.co,
Screen Shot 2020-11-18 at 12 31 56 PM
which matches https://epr.elastic.co/package/apache/0.2.6/img/logo_apache.svg

Checklist

Should I add any tests for this? What kind(s) if so?

@jfsiii jfsiii requested a review from ruflin November 18, 2020 19:05
@jfsiii jfsiii added release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v8.0.0 labels Nov 18, 2020
@jfsiii jfsiii marked this pull request as ready for review November 18, 2020 19:07
@jfsiii jfsiii requested a review from a team November 18, 2020 19:07
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

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

@ruflin
Copy link
Contributor

ruflin commented Nov 19, 2020

What happened exactly before this change? Every time the browser requested the image, Kibana requested it again from the CDN?

After installing a package, is the image served from the local package cache or also the CDN?

@jfsiii
Copy link
Contributor Author

jfsiii commented Nov 19, 2020

What happened exactly before this change? Every time the browser requested the image, Kibana requested it again from the CDN?

Yes. Every view caused the browser to make a request to Kibana, which then made a request to the Registry.

After installing a package, is the image served from the local package cache or also the CDN?

I'm not entirely clear on what you mean by "local package cache". The contents do not come from server memory if that's the question.

All files, regardless of installation status, are fetched from the Registry.

What's changed here is Kibana now includes the Registry response's cache header in the response to the browser. This way, the browser will use it's cache and rules to avoid even making a request to Kibana.

@jfsiii jfsiii self-assigned this Nov 19, 2020
@jfsiii jfsiii added the Team:Fleet Team label for Observability Data Collection Fleet team label Nov 19, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Team:Ingest Management)

@ruflin
Copy link
Contributor

ruflin commented Nov 19, 2020

Tested this locally and seems to do the tricky. Image loading is now much quicker and contains the cache headers.

@jfsiii jfsiii merged commit 514b50e into elastic:master Nov 19, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 19, 2020
* master: (60 commits)
  Forward any registry cache-control header for files (elastic#83680)
  Revert "[Alerting] Add `alert.updatedAt` field to represent date of last user edit (elastic#83578)"
  [Security Solution][Detections] Fix adding an action to detection rules (elastic#83722)
  Make expectSnapshot available in all functional test runs (elastic#82932)
  Skip failing cypress test
  Increase bulk request timeout during esArchiver load (elastic#83657)
  [data.search] Server-side background session service (elastic#81099)
  [maps] convert VectorStyleEditor to TS (elastic#83582)
  Revert "[App Search] Engine overview layout stub (elastic#83504)"
  Adding documentation for global action configuration options (elastic#83557)
  [Metrics UI] Optimizations for Snapshot and Inventory Metadata (elastic#83596)
  chore(NA): update lmdb store to v0.8.15 (elastic#83726)
  [App Search] Engine overview layout stub (elastic#83504)
  [Workplace Search] Update SourceIcon to match latest changes in ent-search (elastic#83714)
  [Enterprise Search] Rename React Router helpers (elastic#83718)
  [Maps] Add 'crossed' & 'exited' events to tracking alert (elastic#82463)
  Updating code-owners to use new core/app-services team names (elastic#83731)
  Add Managed label to data streams and a view switch for the table (elastic#83049)
  [Maps] Add query bar inputs to geo threshold alerts tracked points & boundaries (elastic#80871)
  fix(NA): search examples kibana version declaration (elastic#83182)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 19, 2020
* master: (60 commits)
  Forward any registry cache-control header for files (elastic#83680)
  Revert "[Alerting] Add `alert.updatedAt` field to represent date of last user edit (elastic#83578)"
  [Security Solution][Detections] Fix adding an action to detection rules (elastic#83722)
  Make expectSnapshot available in all functional test runs (elastic#82932)
  Skip failing cypress test
  Increase bulk request timeout during esArchiver load (elastic#83657)
  [data.search] Server-side background session service (elastic#81099)
  [maps] convert VectorStyleEditor to TS (elastic#83582)
  Revert "[App Search] Engine overview layout stub (elastic#83504)"
  Adding documentation for global action configuration options (elastic#83557)
  [Metrics UI] Optimizations for Snapshot and Inventory Metadata (elastic#83596)
  chore(NA): update lmdb store to v0.8.15 (elastic#83726)
  [App Search] Engine overview layout stub (elastic#83504)
  [Workplace Search] Update SourceIcon to match latest changes in ent-search (elastic#83714)
  [Enterprise Search] Rename React Router helpers (elastic#83718)
  [Maps] Add 'crossed' & 'exited' events to tracking alert (elastic#82463)
  Updating code-owners to use new core/app-services team names (elastic#83731)
  Add Managed label to data streams and a view switch for the table (elastic#83049)
  [Maps] Add query bar inputs to geo threshold alerts tracked points & boundaries (elastic#80871)
  fix(NA): search examples kibana version declaration (elastic#83182)
  ...
phillipb added a commit to phillipb/kibana that referenced this pull request Nov 19, 2020
…ode-details

* 'master' of github.com:elastic/kibana:
  fixed pagination in connectors list (elastic#83638)
  Forward any registry cache-control header for files (elastic#83680)
  Revert "[Alerting] Add `alert.updatedAt` field to represent date of last user edit (elastic#83578)"
  [Security Solution][Detections] Fix adding an action to detection rules (elastic#83722)
  Make expectSnapshot available in all functional test runs (elastic#82932)
  Skip failing cypress test
  Increase bulk request timeout during esArchiver load (elastic#83657)
  [data.search] Server-side background session service (elastic#81099)
  [maps] convert VectorStyleEditor to TS (elastic#83582)
  Revert "[App Search] Engine overview layout stub (elastic#83504)"
  Adding documentation for global action configuration options (elastic#83557)
  [Metrics UI] Optimizations for Snapshot and Inventory Metadata (elastic#83596)
  chore(NA): update lmdb store to v0.8.15 (elastic#83726)
  [App Search] Engine overview layout stub (elastic#83504)
  [Workplace Search] Update SourceIcon to match latest changes in ent-search (elastic#83714)
  [Enterprise Search] Rename React Router helpers (elastic#83718)
chrisronline pushed a commit to chrisronline/kibana that referenced this pull request Nov 19, 2020
closes elastic#83631 

### Problem
Assets are served with a `cache-control` header that prevents any caching
<img src="https://user-images.githubusercontent.com/640/99534379-517d2300-2975-11eb-8c05-4fb3f127c52b.png"/>

### Root cause
Likely from this code https://github.com/elastic/kibana/blob/2a365ff6329544465227e61141ded6fba8bb2c80/src/core/server/http/http_tools.ts#L40-L43

Also based on these tests, it seems this is default/expected behavior

https://github.com/elastic/kibana/blob/b3eefb97da8e712789b5c5d2eeae65c886ed8f64/src/core/server/http/integration_tests/router.test.ts#L510-L520

### Proposed solution
Set the header via the response handler as shown in this test:
https://github.com/elastic/kibana/blob/b3eefb97da8e712789b5c5d2eeae65c886ed8f64/src/core/server/http/integration_tests/router.test.ts#L522-L536

### This PR
If this registry response contains a `cache-control` header, that value is included in the EPM response as well

In `master`, which points to `epr-snapshot`
<img width="742" alt="Screen Shot 2020-11-18 at 12 33 47 PM" src="https://user-images.githubusercontent.com/57655/99568352-4fc75580-299d-11eb-962f-6ff28fa9510d.png">
which matches https://epr-snapshot.elastic.co/package/apache/0.2.6/img/logo_apache.svg

or using `epr.elastic.co`, 
<img width="781" alt="Screen Shot 2020-11-18 at 12 31 56 PM" src="https://user-images.githubusercontent.com/57655/99568350-4fc75580-299d-11eb-966e-f3489c13edb5.png">
which matches https://epr.elastic.co/package/apache/0.2.6/img/logo_apache.svg
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 23, 2020
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 83680 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

jfsiii pushed a commit that referenced this pull request Nov 24, 2020
closes #83631 

### Problem
Assets are served with a `cache-control` header that prevents any caching
<img src="https://user-images.githubusercontent.com/640/99534379-517d2300-2975-11eb-8c05-4fb3f127c52b.png"/>

### Root cause
Likely from this code https://github.com/elastic/kibana/blob/2a365ff6329544465227e61141ded6fba8bb2c80/src/core/server/http/http_tools.ts#L40-L43

Also based on these tests, it seems this is default/expected behavior

https://github.com/elastic/kibana/blob/b3eefb97da8e712789b5c5d2eeae65c886ed8f64/src/core/server/http/integration_tests/router.test.ts#L510-L520

### Proposed solution
Set the header via the response handler as shown in this test:
https://github.com/elastic/kibana/blob/b3eefb97da8e712789b5c5d2eeae65c886ed8f64/src/core/server/http/integration_tests/router.test.ts#L522-L536

### This PR
If this registry response contains a `cache-control` header, that value is included in the EPM response as well

In `master`, which points to `epr-snapshot`
<img width="742" alt="Screen Shot 2020-11-18 at 12 33 47 PM" src="https://user-images.githubusercontent.com/57655/99568352-4fc75580-299d-11eb-962f-6ff28fa9510d.png">
which matches https://epr-snapshot.elastic.co/package/apache/0.2.6/img/logo_apache.svg

or using `epr.elastic.co`, 
<img width="781" alt="Screen Shot 2020-11-18 at 12 31 56 PM" src="https://user-images.githubusercontent.com/57655/99568350-4fc75580-299d-11eb-966e-f3489c13edb5.png">
which matches https://epr.elastic.co/package/apache/0.2.6/img/logo_apache.svg
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Fleet] Slow image loading on 7.10
5 participants