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.

Sign up for GitHub

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

Jump to bottom

feat(core) enabled support for HTTP/2 #2541

Merged
merged 1 commit into from
Jun 22, 2017
Merged

feat(core) enabled support for HTTP/2 #2541

merged 1 commit into from
Jun 22, 2017

Conversation

subnetmarco
Copy link
Member

@subnetmarco subnetmarco commented May 17, 2017 •
edited
Loading

  • Added support for HTTP/2

To be merged after #2540.

Goes in hand with https://github.com/Mashape/kong-distributions/commit/5ca1bdd67e46587146f6669ae509c867806555dd and Kong/docs.konghq.com#421.

Notes: In NGINX, only HTTP/2 is only supported under https.

@subnetmarco subnetmarco mentioned this pull request May 17, 2017
Closed
@thibaultcha thibaultcha modified the milestone: 0.11.0 May 19, 2017
p0pr0ck5
p0pr0ck5 previously requested changes May 31, 2017
View reviewed changes
kong/templates/nginx_kong.lua Outdated
@@ -84,7 +84,7 @@ server {
access_log logs/access.log;

> if ssl then
listen ${{PROXY_LISTEN_SSL}} ssl;
listen ${{PROXY_LISTEN_SSL}} ssl http2;
Copy link
Contributor

Choose a reason for hiding this comment

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

lets make the http2 options configurable, even if it's enabled by default?

Copy link
Member Author

Choose a reason for hiding this comment

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

@p0pr0ck5 what would be the use-case? This is between Client <> Kong, not between Kong <> API.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I understand. It may be desirable that Kong operators do not want to expose clients to potential instabilities associated with the h2 implementation in Nginx, documented or otherwise.

@p0pr0ck5 p0pr0ck5 added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/please review labels May 31, 2017
@thibaultcha thibaultcha mentioned this pull request Jun 6, 2017
Merged
@subnetmarco
Copy link
Member Author

subnetmarco commented Jun 6, 2017 •
edited
Loading

New http2=on|off and admin_http2=on|off have been added. IMHO overkill for a stable feature of Nginx (since nothing prevents the user for using a custom template without HTTP/2 if he ends up in an edge-case).

@thibaultcha
Copy link
Member

thibaultcha commented Jun 6, 2017 •
edited
Loading

While rather stable in Nginx, it isn't in OpenResty, so it makes perfect sense to make this feature configurable.

@subnetmarco subnetmarco added pr/status/please review and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Jun 7, 2017
@p0pr0ck5
Copy link
Contributor

p0pr0ck5 commented Jun 9, 2017

@thefosk please remove the http2 directive from plaintext server blocks. Nginx currently cannot support h2c and HTTP/1.1 connections simultaneously. See: https://trac.nginx.org/nginx/ticket/816

@p0pr0ck5 p0pr0ck5 added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/please review labels Jun 20, 2017
@p0pr0ck5 p0pr0ck5 self-assigned this Jun 20, 2017
@p0pr0ck5 p0pr0ck5 changed the base branch from master to next June 21, 2017 16:12
@p0pr0ck5 p0pr0ck5 force-pushed the feat/http2 branch 2 times, most recently from 79b8026 to fcf5ce1 Compare June 21, 2017 16:17
@p0pr0ck5 p0pr0ck5 added pr/wip A work in progress PR opened to receive feedback and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Jun 21, 2017
@p0pr0ck5 p0pr0ck5 force-pushed the feat/http2 branch 3 times, most recently from bb49027 to 2e4b29f Compare June 21, 2017 22:05
@p0pr0ck5 p0pr0ck5 added pr/status/please review and removed pr/wip A work in progress PR opened to receive feedback labels Jun 21, 2017
@p0pr0ck5 p0pr0ck5 dismissed their stale review June 21, 2017 23:32

Handled, needs more eyes

@shashiranjan84
Copy link
Contributor

@p0pr0ck5 LGTM.

thibaultcha
thibaultcha reviewed Jun 22, 2017
View reviewed changes
kong.conf.default Outdated
@@ -136,6 +136,9 @@
# the SSL key for the `proxy_listen_ssl`
# address.

#http2 = off # Enable HTTP2 support for HTTPS traffic on the
Copy link
Member

Choose a reason for hiding this comment

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

All of our other configuration properties use the third person ("Enables")

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yep, thanks. Fixed!

@p0pr0ck5 p0pr0ck5 merged commit 4424146 into next Jun 22, 2017
@p0pr0ck5 p0pr0ck5 deleted the feat/http2 branch June 22, 2017 22:44
bungle added a commit that referenced this pull request Jul 12, 2017
@bungle
fix(conf) add the missing http2 (see #2541) to custom_nginx.template
7a93587
thibaultcha pushed a commit that referenced this pull request Jul 13, 2017
@bungle @thibaultcha
fix(conf) add the missing http2 (see #2541) to custom_nginx.template
96e33d1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@p0pr0ck5 p0pr0ck5 p0pr0ck5 left review comments

@thibaultcha thibaultcha thibaultcha left review comments

@shashiranjan84 shashiranjan84 shashiranjan84 approved these changes

Assignees

@p0pr0ck5 p0pr0ck5

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@subnetmarco @thibaultcha @p0pr0ck5 @shashiranjan84