-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[HTTP] Add support for configuring a CDN (part I) #169408
[HTTP] Add support for configuring a CDN (part I) #169408
Conversation
…hings for clarity
Pinging @elastic/kibana-core (Team:Core) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a great foundation to start iterating on. Something we should explore with operations is how to write a smoke test for this as a bug here would be high impact. Would be great if once we've published assets we could spin up Kibana and make sure it can render from the CDN.
default_src: [hostname], | ||
font_src: [hostname], | ||
img_src: [hostname], | ||
script_src: [hostname], | ||
style_src: [hostname], | ||
worker_src: [hostname], | ||
connect_src: [hostname], | ||
frame_src: [hostname], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elastic/kibana-security IIRC you've always reviewed changes to our CSP so would be good to get a +1 from you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few remarks and question, but overall LGTM
export type CspAdditionalConfig = Pick< | ||
Partial<CspConfigType>, | ||
| 'connect_src' | ||
| 'default_src' | ||
| 'font_src' | ||
| 'frame_src' | ||
| 'img_src' | ||
| 'script_src' | ||
| 'style_src' | ||
| 'worker_src' | ||
>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: I would just have gone with Partial<CspConfigType>
, but what you did is probably better 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I had that initially, but there is other funky stuff in there and I wanted scope the overrides to just the policy directives
public static from(input: Input = {}) { | ||
return new CdnConfig(input); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: FMI, what's the upside of the private constructor + static creator? (or maybe that's just how we're already doing with other http sub configs?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good q, I slightly refactored this, bc I realised I was using Input in both the constructor and in this state creator. The constructor know uses it's own, separated args as they are IMO easier to use in the constructor.
But yeah, not massive benefit, as you point out.
packages/core/http/core-http-server-internal/src/static_assets.ts
Outdated
Show resolved
Hide resolved
// TODO: Make this load as part of static assets! | ||
translationsUrl: `${basePath}/translations/${i18n.getLocale()}.json`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ (but good luck 😄)
default_src: [hostname], | ||
font_src: [hostname], | ||
img_src: [hostname], | ||
script_src: [hostname], | ||
style_src: [hostname], | ||
worker_src: [hostname], | ||
connect_src: [hostname], | ||
frame_src: [hostname], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you verified that Kibana requires all of these declarations for the CDN to function? A few initial thoughts:
- We don't supply a
default_src
today, and I don't think the CDN should be the first to do so, unless there's a compelling reason. - Will Kibana host frames whose src points to the CDN? If not, then we likely don't need
frame_src
. - This CDN will (I assume) host static assets. It seems unlikely that
connect_src
would be required. - Will the CDN be hosting scripts used by web workers? If not, then
worker_src
is unlikely to be required. I don't know where we're using working within Kibana today, so this is a genuine question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All valid points :) there is some amount of unknown, since I don't know what all will be served from the CDN, but:
- Sounds good to me!
- I don't know, but we can remove for now
- I believe we do have script based loading of certain assets
- We will be hosting Monaco editor's worker scripts on the CDN, yes
Do we think falling back to local assets if the CDN is down is worthwhile? |
Sounds worthwhile to me, probably as an enhancement to this first iteration. There are two failure scenarios I can think of:
To address both of these are you imagining something like this: https://stackoverflow.com/a/2607534 Perhaps webpack already has something for this? |
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Public APIs missing exports
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: |
`${regularBundlePath}/kbn-ui-shared-deps-src/${UiSharedDepsSrc.cssDistFilename}`, | ||
`${basePath}/ui/legacy_light_theme.min.css`, | ||
`${bundlesHref}/kbn-ui-shared-deps-src/${UiSharedDepsSrc.cssDistFilename}`, | ||
`${baseHref}/ui/legacy_light_theme.min.css`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we considered putting this route behind the build number? Behind a CDN I'm not sure how would push updates through to these files.
Closes #169427 Adds a new build step `createCdnAssets` that will create an archive `kibana-<version>-cdn-assets.tar.gz` with static assets organized using the request structure of the kibana client. - By default CDN assets are created - Adding the flag `node scripts/build --skip-cdn-assets` will skip creation - `ci:build-cdn-assets` can be used to create and upload the archive for testing Testing: see #169408. Builds are available in the artifacts tab on the `Build Distribution` step. 1) Extract builds 2) ``` python3 -m http.server -b localhost -d kibana-8.12.0-SNAPSHOT-cdn-assets 8000 ``` 3) ``` echo 'server.cdn.url: http://localhost:8000' >> kibana-8.12.0-SNAPSHOT/config/kibana.yml ``` 4) ``` kibana-8.12.0-SNAPSHOT/bin/kibana ```
## Summary Follow up of [first CDN PR](#169408). Primary focus is replacing our build nr with SHA that allows cache busting and maintains anti-collision properties. ## How to test Start Kibana as usual navigating around the app with the network tab open in your browser of choice. Keep an eye out for any asset loading errors. It's tricky to test every possible asset since there are many permutations, but generally navigating around Kibana should work exactly as it did before regarding loading bundles and assets. ## Notes * did a high-level audit of usages of `buildNum` in `packages`, `src` and `x-pack` adding comments where appropriate. * In non-distributable builds (like dev) static asset paths will be prefixed with `XXXXXXXXXXXX` instead of Node's `Number.MAX_SAFE_INTEGER` * Added some validation to ensure the CDN url is of the expected form: nothing trailing the pathname ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### Risk Matrix | Risk | Probability | Severity | Mitigation/Notes | |---------------------------|-------------|----------|-------------------------| | We break some first or third party dependencies on existing asset routes | Med | High | Attempting to mitgate by serving static assets from both old and new paths where paths have updated to include the build SHA. Additioanlly: it is very bad practice to rely on the values of the static paths, but someone might be | | Cache-busting is more aggressive | High | Low | Unlikely to be a big problem, but we are not scoping more static assets to a SHA and so every new Kibana release will require clients to, for example, download fonts again. | ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <[email protected]>
…ic#175898) ## Summary Follow up of [first CDN PR](elastic#169408). Primary focus is replacing our build nr with SHA that allows cache busting and maintains anti-collision properties. ## How to test Start Kibana as usual navigating around the app with the network tab open in your browser of choice. Keep an eye out for any asset loading errors. It's tricky to test every possible asset since there are many permutations, but generally navigating around Kibana should work exactly as it did before regarding loading bundles and assets. ## Notes * did a high-level audit of usages of `buildNum` in `packages`, `src` and `x-pack` adding comments where appropriate. * In non-distributable builds (like dev) static asset paths will be prefixed with `XXXXXXXXXXXX` instead of Node's `Number.MAX_SAFE_INTEGER` * Added some validation to ensure the CDN url is of the expected form: nothing trailing the pathname ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### Risk Matrix | Risk | Probability | Severity | Mitigation/Notes | |---------------------------|-------------|----------|-------------------------| | We break some first or third party dependencies on existing asset routes | Med | High | Attempting to mitgate by serving static assets from both old and new paths where paths have updated to include the build SHA. Additioanlly: it is very bad practice to rely on the values of the static paths, but someone might be | | Cache-busting is more aggressive | High | Low | Unlikely to be a big problem, but we are not scoping more static assets to a SHA and so every new Kibana release will require clients to, for example, download fonts again. | ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <[email protected]>
…ic#175898) ## Summary Follow up of [first CDN PR](elastic#169408). Primary focus is replacing our build nr with SHA that allows cache busting and maintains anti-collision properties. ## How to test Start Kibana as usual navigating around the app with the network tab open in your browser of choice. Keep an eye out for any asset loading errors. It's tricky to test every possible asset since there are many permutations, but generally navigating around Kibana should work exactly as it did before regarding loading bundles and assets. ## Notes * did a high-level audit of usages of `buildNum` in `packages`, `src` and `x-pack` adding comments where appropriate. * In non-distributable builds (like dev) static asset paths will be prefixed with `XXXXXXXXXXXX` instead of Node's `Number.MAX_SAFE_INTEGER` * Added some validation to ensure the CDN url is of the expected form: nothing trailing the pathname ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### Risk Matrix | Risk | Probability | Severity | Mitigation/Notes | |---------------------------|-------------|----------|-------------------------| | We break some first or third party dependencies on existing asset routes | Med | High | Attempting to mitgate by serving static assets from both old and new paths where paths have updated to include the build SHA. Additioanlly: it is very bad practice to rely on the values of the static paths, but someone might be | | Cache-busting is more aggressive | High | Low | Unlikely to be a big problem, but we are not scoping more static assets to a SHA and so every new Kibana release will require clients to, for example, download fonts again. | ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <[email protected]>
Adds support for setting `server.cdn.url` in our docker containers. Implemented at #169408.
Adds support for setting `server.cdn.url` in our docker containers. Implemented at elastic#169408. (cherry picked from commit 023f5d6)
# Backport This will backport the following commits from `main` to `8.13`: - [[bin/kibana-docker] Add server.cdn.url (#177491)](#177491) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Jon","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-02-22T01:05:08Z","message":"[bin/kibana-docker] Add server.cdn.url (#177491)\n\nAdds support for setting `server.cdn.url` in our docker containers.\r\nImplemented at https://github.com/elastic/kibana/pull/169408.","sha":"023f5d63345667819c363b597867d0b3c53ca488","branchLabelMapping":{"^v8.14.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Operations","release_note:skip","backport:prev-minor","v8.14.0"],"title":"[bin/kibana-docker] Add server.cdn.url","number":177491,"url":"https://github.com/elastic/kibana/pull/177491","mergeCommit":{"message":"[bin/kibana-docker] Add server.cdn.url (#177491)\n\nAdds support for setting `server.cdn.url` in our docker containers.\r\nImplemented at https://github.com/elastic/kibana/pull/169408.","sha":"023f5d63345667819c363b597867d0b3c53ca488"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.14.0","branchLabelMappingKey":"^v8.14.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/177491","number":177491,"mergeCommit":{"message":"[bin/kibana-docker] Add server.cdn.url (#177491)\n\nAdds support for setting `server.cdn.url` in our docker containers.\r\nImplemented at https://github.com/elastic/kibana/pull/169408.","sha":"023f5d63345667819c363b597867d0b3c53ca488"}}]}] BACKPORT--> Co-authored-by: Jon <[email protected]>
…ic#175898) ## Summary Follow up of [first CDN PR](elastic#169408). Primary focus is replacing our build nr with SHA that allows cache busting and maintains anti-collision properties. ## How to test Start Kibana as usual navigating around the app with the network tab open in your browser of choice. Keep an eye out for any asset loading errors. It's tricky to test every possible asset since there are many permutations, but generally navigating around Kibana should work exactly as it did before regarding loading bundles and assets. ## Notes * did a high-level audit of usages of `buildNum` in `packages`, `src` and `x-pack` adding comments where appropriate. * In non-distributable builds (like dev) static asset paths will be prefixed with `XXXXXXXXXXXX` instead of Node's `Number.MAX_SAFE_INTEGER` * Added some validation to ensure the CDN url is of the expected form: nothing trailing the pathname ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### Risk Matrix | Risk | Probability | Severity | Mitigation/Notes | |---------------------------|-------------|----------|-------------------------| | We break some first or third party dependencies on existing asset routes | Med | High | Attempting to mitgate by serving static assets from both old and new paths where paths have updated to include the build SHA. Additioanlly: it is very bad practice to rely on the values of the static paths, but someone might be | | Cache-busting is more aggressive | High | Low | Unlikely to be a big problem, but we are not scoping more static assets to a SHA and so every new Kibana release will require clients to, for example, download fonts again. | ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <[email protected]>
Adds support for setting `server.cdn.url` in our docker containers. Implemented at elastic#169408.
Summary
This PR prepares the Kibana core code for configuring and using a CDN to serve the vast majority of assets.
Goals
(1) Configure a CDN URL:
server.cdn.url
(2) Reach a new domain: updating our CSP to allow this
(3) Serve JS bundles, CSS, fonts via CDN
How to test
node ./scripts/build.js --release --skip-os-packages
), will output to./build/kibana
./etc/hosts
127.0.0.1 my.cdn.test
config/kibana.dev.yml
addingserver.cdn.url: "http://my.cdn.test:1772"
cd
intobuild/kibana
once the release build is donebuild/kibana
. For example:npx http-server -p 1772 --cors --gzip --brotli
-- this will be the "CDN" serveryarn start
and ESyarn es snapshot
404
s, CORs or CSP is almost definitely a bug in this code.Future work
should not be much effort)Screenshots
Checklist
Delete any items that are not applicable to this PR.
Risk Matrix
Delete this section if it is not applicable to this PR.
Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.
When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:
For maintainers