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

Backend: Adds support for HTTP/2 #18358

Merged
merged 3 commits into from
Aug 16, 2019
Merged

Backend: Adds support for HTTP/2 #18358

merged 3 commits into from
Aug 16, 2019

Conversation

kaydelaney
Copy link
Contributor

@kaydelaney kaydelaney commented Aug 2, 2019

Based on #13105

Ciphers chosen based on mozilla's recommendations: https://wiki.mozilla.org/Security/Server_Side_TLS

@kaydelaney kaydelaney self-assigned this Aug 2, 2019
@kaydelaney kaydelaney added this to Under review in Backend Platform Backlog via automation Aug 2, 2019
@gotjosh gotjosh self-requested a review August 2, 2019 14:13
@marefr
Copy link
Member

marefr commented Aug 6, 2019

Can you elaborate a bit on why Grafana should support HTTP/2?

@kaydelaney
Copy link
Contributor Author

@marefr It works nicely with some of the work I've been doing improving Grafana's performance through webpack (parallel downloads, etc.), but also I think it's nice to have the option for users who might have some specific use-case in mind.

MinVersion: tls.VersionTLS12,
PreferServerCipherSuites: false,
CipherSuites: []uint16{
tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
Copy link
Contributor

Choose a reason for hiding this comment

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

I only tagged myself as a reviewer - as I wanted to understand why the cipher-suite is so narrow. I have not been up to speed with the state of HTTP/2 but thinking whenever this would introduce compatibility issues for existing installations.

I understand we don't want it to be too broad due to security risks, but three seems a bit on the shorter end of the stick.

I don't need a direct answer if we have reasons let's just document them as part of the PR description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason in particular as to why it's so narrow - purely a consequence of this PR being based on #13105. I left it unchanged as I wanted to hear from others if they thought, say, Mozilla's recommendations re: preferred ciphers should be followed before committing it to code.

Copy link
Member

Choose a reason for hiding this comment

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

For reference I think https://wiki.mozilla.org/Security/Server_Side_TLS is the Mozilla recommendation

@marefr
Copy link
Member

marefr commented Aug 7, 2019

@kaydelaney the reason I'm a bit restrictive here is that we did close the other PR and there are no feature request opened for this. I'm also not aware of the potential performance improvements you've been looking into and how highly requested/needed those are - maybe something for the frontend team to discuss?

As a sidenote, one interesting thing to investigate before HTTP/2 would be #6844 since a lot of users could get a much better Grafana experience of that.

@torkelo do you have any other feedback regarding HTTP/2?

@marefr marefr removed this from Under review in Backend Platform Backlog Aug 7, 2019
@kaydelaney
Copy link
Contributor Author

kaydelaney commented Aug 7, 2019

@marefr To provide some data, here are some results from the work I've been doing splitting the code into more, smaller bundles (some of which are lazy-loaded).
Using Slow 4G and 4x CPU throttle:
Screenshot 2019-08-07 at 14 46 15

Certain statistics are notably improved by the introduction of HTTP/2, and I can imagine they will only be improved further by more aggressive code-splitting in the future

@ryantxu
Copy link
Member

ryantxu commented Aug 7, 2019

Also HTTP/2 gives us interesting options for streaming

@torkelo
Copy link
Member

torkelo commented Aug 7, 2019

Looks good to me!

Just a thought is TLS always required for HTTP2?

@kaydelaney
Copy link
Contributor Author

@torkelo so, according to the standard, technically not, but in practice yes since all major implementations exclusively support HTTP/2 over TLS.

@kaydelaney kaydelaney marked this pull request as ready for review August 15, 2019 09:21
Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

This looks good to me, @marefr what do you think?

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.

Looks good and the impact of the code is quite limited so I approve 👍

Requested a minor fix, see comment. There are also some documentation here that needs to be updated before this can be merged:

conf/defaults.ini Show resolved Hide resolved
@kaydelaney kaydelaney requested a review from marefr August 16, 2019 14:07
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

@kaydelaney kaydelaney merged commit fb0cec5 into master Aug 16, 2019
@kaydelaney kaydelaney deleted the http2_support branch August 16, 2019 15:06
ryantxu added a commit to ryantxu/grafana that referenced this pull request Aug 16, 2019
* grafana/master:
  Docker: switch docker image to alpine base with phantomjs support (grafana#18468)
  Backend: Adds support for HTTP/2 (grafana#18358)
  Explore: Fixes error when switching from prometheus to loki data sources (grafana#18599)
  TimePicker: Set time to to 23:59:59 when setting To time using calendar (grafana#18595)
  Prometheus: Return labels in query results (grafana#18535)
  Docs: Update changelog and docs for annotation region change (grafana#18593)
  Refactor: move KeyValue and deprecation warning to @grafana/data (grafana#18582)
  Annotations: use a single row to represent a region (grafana#17673)
  Docs: Update upgrading guide (grafana#18547)
  Docs: Adds tests requirement to bugs checklist (grafana#18576)
ryantxu added a commit to ryantxu/grafana that referenced this pull request Aug 16, 2019
* grafana/master:
  Docker: switch docker image to alpine base with phantomjs support (grafana#18468)
  Backend: Adds support for HTTP/2 (grafana#18358)
  Explore: Fixes error when switching from prometheus to loki data sources (grafana#18599)
  TimePicker: Set time to to 23:59:59 when setting To time using calendar (grafana#18595)
  Prometheus: Return labels in query results (grafana#18535)
  Docs: Update changelog and docs for annotation region change (grafana#18593)
  Refactor: move KeyValue and deprecation warning to @grafana/data (grafana#18582)
  Annotations: use a single row to represent a region (grafana#17673)
  Docs: Update upgrading guide (grafana#18547)
  Docs: Adds tests requirement to bugs checklist (grafana#18576)
ryantxu added a commit to ryantxu/grafana that referenced this pull request Aug 18, 2019
…rame

* grafana/master:
  SingleStat: add a gauge migration call to action button in the editor (grafana#18604)
  Build: update revive dependency (grafana#18585)
  LDAP: multildap + ldap integration (grafana#18588)
  Docker: switch docker image to alpine base with phantomjs support (grafana#18468)
  Backend: Adds support for HTTP/2 (grafana#18358)
  Explore: Fixes error when switching from prometheus to loki data sources (grafana#18599)
  TimePicker: Set time to to 23:59:59 when setting To time using calendar (grafana#18595)
  Prometheus: Return labels in query results (grafana#18535)
  Docs: Update changelog and docs for annotation region change (grafana#18593)
  Refactor: move KeyValue and deprecation warning to @grafana/data (grafana#18582)
  Annotations: use a single row to represent a region (grafana#17673)
  Docs: Update upgrading guide (grafana#18547)
  Docs: Adds tests requirement to bugs checklist (grafana#18576)
ryantxu added a commit to ryantxu/grafana that referenced this pull request Aug 18, 2019
* grafana/master:
  SingleStat: add a gauge migration call to action button in the editor (grafana#18604)
  Build: update revive dependency (grafana#18585)
  LDAP: multildap + ldap integration (grafana#18588)
  Docker: switch docker image to alpine base with phantomjs support (grafana#18468)
  Backend: Adds support for HTTP/2 (grafana#18358)
ryantxu added a commit to ryantxu/grafana that referenced this pull request Aug 19, 2019
* grafana/master: (202 commits)
  Azure Monitor and Log Analytics converted and separated into components (grafana#18259)
  Rewrite user profile edit to react (grafana#17917)
  Docs: remove codecov badge (grafana#18623)
  Prometheus: Prevents panel editor crash when switching to Prometheus datasource (grafana#18616)
  Chore: Rename Popper to Popover (grafana#18543)
  SingleStat: add a gauge migration call to action button in the editor (grafana#18604)
  Build: update revive dependency (grafana#18585)
  LDAP: multildap + ldap integration (grafana#18588)
  Docker: switch docker image to alpine base with phantomjs support (grafana#18468)
  Backend: Adds support for HTTP/2 (grafana#18358)
  Explore: Fixes error when switching from prometheus to loki data sources (grafana#18599)
  TimePicker: Set time to to 23:59:59 when setting To time using calendar (grafana#18595)
  Prometheus: Return labels in query results (grafana#18535)
  Docs: Update changelog and docs for annotation region change (grafana#18593)
  Refactor: move KeyValue and deprecation warning to @grafana/data (grafana#18582)
  Annotations: use a single row to represent a region (grafana#17673)
  Docs: Update upgrading guide (grafana#18547)
  Docs: Adds tests requirement to bugs checklist (grafana#18576)
  DataFrame: convert from row based to a columnar value format (grafana#18391)
  Packages: Temporarily skip canary releases if packages build fail (grafana#18577)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants