-
Notifications
You must be signed in to change notification settings - Fork 170
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
Feature: Add HTTP2 support. #1128
Conversation
69d676a
to
21e1b73
Compare
21e1b73
to
acbb3c9
Compare
Around test that I added in this feature: HTTP 2.0 termination: HTTP 2.0 full flow: All other policies should work as normal, no protocol or code issues that I can |
@@ -47,7 +38,7 @@ local function detect_http_client(endpoint) | |||
if proxy then -- use default client | |||
return | |||
else | |||
return http_ng_backend_phase[ngx.get_phase()] | |||
return http_ng_resty |
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.
It would be good to evaluate whether this change has any impact on performance.
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.
It's using lua coroutines and it's the recommended way in Openresty, it's safe to update.
@@ -738,7 +738,7 @@ GET /t | |||
--- request | |||
GET /t | |||
--- more_headers | |||
Content-Type: application/json | |||
X-Foo: application/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.
Why did this change?
|
||
function _M:rewrite(context) | ||
if ngx.var.server_protocol == "HTTP/2.0" then | ||
context.upstream_location_name = "@grpc_upstream" |
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.
Can you add a comment that references the file where this location is defined?
t/apicast-http2.t
Outdated
|
||
$ENV{APICAST_HTTPS_RANDOM_PORT} = Test::APIcast::get_random_port(); | ||
|
||
repeat_each(1); |
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.
If there's a reason why the tests cannot run twice, can you please document it?
5bcf6cf
to
e317f7a
Compare
This commit adds HTTP2 termination support to APIcast. Things that were changed: - ngx.location.capture: no longer used, it has no support to HTTP2, added support will be easy in C code, but HTTP client using cosockets is the prefered way from code maintainers and it has the same performance. - Integration test: Changed some of them that was not correctly defined. This commit only add support to terminate HTTP2 request, full HTTP2 support is not yet enabled. Signed-off-by: Eloy Coto <[email protected]>
This policy adds the option to enable HTTP2 responses in endpoints and ensure that the connection is working as expected. Signed-off-by: Eloy Coto <[email protected]>
This commits add HTTP2 integration test into APICast, the main change is that a new helper function was added into integration `t/helper/` to avoid to repeat code across multiple test. Another development dependency was added ("lua-http") to be able to execute http2 calls from openresty to verify that everything works. Signed-off-by: Eloy Coto <[email protected]>
Signed-off-by: Eloy Coto <[email protected]>
e317f7a
to
5ec2f8b
Compare
This commit adds HTTP2 termination support to APIcast.
Things that were changed:
ngx.location.capture: no longer used, it has no support to HTTP2,
added support will be easy in C code, but HTTP client using cosockets is
the prefered way from code maintainers and it has the same performance.
Integration test: Changed some of them that were not correctly defined.
This commit only add support to terminate HTTP2 request, full HTTP2
support is not yet enabled.