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

CDN: Adds support for serving assets over a CDN #30691

Merged
merged 17 commits into from Feb 1, 2021
Merged

CDN: Adds support for serving assets over a CDN #30691

merged 17 commits into from Feb 1, 2021

Conversation

torkelo
Copy link
Member

@torkelo torkelo commented Jan 27, 2021

Closes #6844

Adds option to load js & css from cdn path. Tested by starting a local nginx with a custom hostname and copying everything under public to it.

This does not currently impact loading plugins, they are still loaded from instance.

It does impact the content_security_policy, if enabled it needs to be modified allow the cdn. Not sure how exactly.

TODO

  • How to make it apply to all images

Screenshot from 2021-01-27 22-39-34

@torkelo torkelo requested a review from a team as a code owner January 27, 2021 21:44
@torkelo torkelo requested review from a team, papagian, jessabe, hugohaggmark and peterholmberg and removed request for a team January 27, 2021 21:44
@torkelo torkelo changed the title CDN: Initial poc support for serving assets over a CDN CDN: Adds support for serving assets over a CDN Jan 27, 2021
@@ -1,4 +1,6 @@
import app from './app';
declare let __webpack_public_path__: string;
__webpack_public_path__ = window.public_cdn_path;
Copy link
Member

Choose a reason for hiding this comment

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

is this used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes this is the key part of telling webpack where to load files

Copy link
Member Author

Choose a reason for hiding this comment

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

Without this all the dynamic webpack chunks are loaded from localhost

Copy link
Contributor

@hugohaggmark hugohaggmark left a comment

Choose a reason for hiding this comment

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

I think the code looks ok, but I'm not very familiar with all the details.

@torkelo torkelo added this to In Review in Frontend Platform Backlog via automation Jan 28, 2021
@torkelo torkelo added this to the 7.5.0 milestone Jan 28, 2021
Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Awesome 👍

Other things to consider referencing CDN might be email templates for example.

Another thing to take into consideration would be to not having to update the CDN path config each time you upgrade Grafana - I guess this partly will be solved by the build hashes, but thinking about 10 Grafana releases would currently require 10x all the files with hashes in the same CDN folder, unless CDN path config is not changed on upgrade. Maybe would make sense to always enforce/include version number/commit in the CDN path?

@@ -1297,6 +1298,7 @@ func readServerSettings(iniFile *ini.File, cfg *Cfg) error {
return err
}

cfg.CDNPath = valueAsString(server, "cdn_path", "")
Copy link
Member Author

Choose a reason for hiding this comment

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

@marefr think here we should probably add Grafana version string to the path (so v7.4.0-beta1) for example. (or in index.go) so that on the cdn each published release is under different paths , the config path you set in the ini file is just the root cdn path

Copy link
Contributor

Choose a reason for hiding this comment

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

using a hash would work with master builds and pre-releases as well. That would be nice.

Copy link
Member

Choose a reason for hiding this comment

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

So maybe both version and hash then? Or only using hash for pre-releases and version for stable ones, not sure how hard that would be.

Copy link
Member Author

@torkelo torkelo Jan 28, 2021

Choose a reason for hiding this comment

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

The asset files are already hashed, this should just add version folder. Master builds already build a number of some sort I think

Copy link
Member

Choose a reason for hiding this comment

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

True, makes sense

@torkelo
Copy link
Member Author

torkelo commented Jan 29, 2021

@bergquist @marefr added edition & and BuildVersion to the url as well so paths becomes

https://cdn.grafana.com/oss/v7.5.0-11124pre
https://cdn.grafana.com/enterprise/v7.5.0-11124pre

if the cdn_path is set to https://cdn.grafana.com depending on what verison & edition grafana is

@torkelo
Copy link
Member Author

torkelo commented Jan 29, 2021

Added one final thing, also segrate master builds into a separate path for easier clean-up. So it will be

/oss
  /v7.4.0-beta1
  /v7.4.0
  /master
     /v7.5.0-121pre
/enterprise
 .... 

Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Awesome. This is great 👍 Spotted some minor problems, see comments. .ini files and documentation needs update as well

pkg/setting/setting.go Outdated Show resolved Hide resolved
pkg/setting/setting_test.go Show resolved Hide resolved
@torkelo torkelo requested a review from a team as a code owner January 29, 2021 11:51
@torkelo torkelo requested review from oddlittlebird, achatterjee-grafana and osg-grafana and removed request for a team January 29, 2021 11:51
@torkelo
Copy link
Member Author

torkelo commented Jan 29, 2021

@marefr pushed changes, added error handling in the read config stage (of the url parsing), changed the property name from cdn_path to cd_url as it's a url not just path. Added docs

Copy link
Contributor

@achatterjee-grafana achatterjee-grafana left a comment

Choose a reason for hiding this comment

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

Added some copy-edit suggestions for the doc content.

public/views/index-template.html Show resolved Hide resolved
public/views/index-template.html Outdated Show resolved Hide resolved
docs/sources/administration/configuration.md Outdated Show resolved Hide resolved
docs/sources/administration/configuration.md Outdated Show resolved Hide resolved
torkelo and others added 3 commits January 29, 2021 16:25
Co-authored-by: achatterjee-grafana <70489351+achatterjee-grafana@users.noreply.github.com>
Co-authored-by: achatterjee-grafana <70489351+achatterjee-grafana@users.noreply.github.com>
Copy link
Contributor

@achatterjee-grafana achatterjee-grafana left a comment

Choose a reason for hiding this comment

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

Added the suggestion to fix a typo that got missed earlier.


Specify a full HTTP URL address to the root of your Grafana CDN assets. Grafana will add edition and version paths.

For example, given a cdn url like `https://cdn.myserver.com` grafana will try to load a javascript file from
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
For example, given a cdn url like `https://cdn.myserver.com` grafana will try to load a javascript file from
For example, given a CDN URL like `https://cdn.myserver.com` grafana will try to load a javascript file from

Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

LGTM

@torkelo torkelo merged commit c04bc80 into master Feb 1, 2021
Frontend Platform Backlog automation moved this from In Review to Done Feb 1, 2021
@torkelo torkelo deleted the cdn-support branch February 1, 2021 09:13
@torkelo torkelo modified the milestones: 7.5.0, 7.4.0-stable Feb 1, 2021
@torkelo torkelo added the old backport v7.4.x Mark PR for automatic backport to v7.4.x label Feb 1, 2021
grafanabot pushed a commit that referenced this pull request Feb 1, 2021
* CDN: Initial poc support for serving assets over a CDN

* Minor fix

* added build path and test

* fix lint error

* Added edition to cdn path

* Move master builds to a separate path

* Added error handling for the url parsing, changed setting name, and added docs

* Updated sample.ini

* Some property renames

* updated

* Minor update to html

* index template improvements

* Update docs/sources/administration/configuration.md

Co-authored-by: achatterjee-grafana <70489351+achatterjee-grafana@users.noreply.github.com>

* Update docs/sources/administration/configuration.md

Co-authored-by: achatterjee-grafana <70489351+achatterjee-grafana@users.noreply.github.com>

* Added ContentDeliveryPrefix to Licence service

* updated docs

* Updated test mock

Co-authored-by: achatterjee-grafana <70489351+achatterjee-grafana@users.noreply.github.com>
(cherry picked from commit c04bc80)
torkelo added a commit that referenced this pull request Feb 1, 2021
* CDN: Initial poc support for serving assets over a CDN

* Minor fix

* added build path and test

* fix lint error

* Added edition to cdn path

* Move master builds to a separate path

* Added error handling for the url parsing, changed setting name, and added docs

* Updated sample.ini

* Some property renames

* updated

* Minor update to html

* index template improvements

* Update docs/sources/administration/configuration.md

Co-authored-by: achatterjee-grafana <70489351+achatterjee-grafana@users.noreply.github.com>

* Update docs/sources/administration/configuration.md

Co-authored-by: achatterjee-grafana <70489351+achatterjee-grafana@users.noreply.github.com>

* Added ContentDeliveryPrefix to Licence service

* updated docs

* Updated test mock

Co-authored-by: achatterjee-grafana <70489351+achatterjee-grafana@users.noreply.github.com>
(cherry picked from commit c04bc80)

Co-authored-by: Torkel Ödegaard <torkel@grafana.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog old backport v7.4.x Mark PR for automatic backport to v7.4.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Support serving static assets of CDN
6 participants