fix(admin) support HTTP/2 when requesting /status #8690
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
dndx
reviewed
Apr 15, 2022
dndx
reviewed
Apr 18, 2022
chronolaw
reviewed
Apr 22, 2022
chronolaw
reviewed
Apr 22, 2022
dndx
reviewed
Apr 22, 2022
dndx
previously approved these changes
Apr 22, 2022
|
Approved with minor improvement. |
|
@outsinre We need some tests inside https://github.com/Kong/kong/tree/master/t/01-pdk/10-nginx as well. |
:8444/status does not support HTTP/2 due to "ngx.location.capture". Now use LuaJIR ffi to retrieve Nginx status data. The "/nginx_status" location block is left in nginx-kong.conf as other components may depend on it.
The "prometheus" plugin now depends 'ngx.location.capture' to retrieve /metrics through Admin API. However, 'ngx.location.capture' does not support http2. 1. Use LuaJIR "ffi" to support http2. 2. Remove the "/nginx_status" location blocks from 'nginx-kong.conf' template. A reminder; the Status API, natively, disallows http2!
We use ffi method to get metrics/status of Kong.
To avoid code duplication. Provide a PDK function reporting Kong connections and requests metrics at Admin API :8444/status, :8444/metrics and Status API :8100/metrics. Update 'prometheus' plugin to invoke the PDK function.
Explain the return values of kong.nginx.get_statistics() function in details. Use local variables as many as possible to avoid function calls.
Append test-nginx unit for "kong.nginx.get_statistics()" and busted unit for plugin "prometheus". The two units test "/metrics" and "/status" http2 support.
Built-in variables like 'ngx.var.connections_active' may be "nil" during the busted tasks. Now exposing connections_active, connections_reading, connections_writing, connections_waiting by 'ffi' directly. Add busted task to test prometheus metrics against status API. Remove '/nginx_status' from prometheus fixtures. Add 'pdk.nginx.get_statistics' to test-nginx 'phase_checks.t'
|
Seems all checks passed. |
kikito
approved these changes
May 4, 2022
| end) | ||
|
|
||
| end) | ||
| end |
There was a problem hiding this comment.
Spacing: newline at the end of file.
dndx
approved these changes
May 5, 2022
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
:8444/status does not support HTTP/2 due to
"ngx.location.capture". Now use LuaJIT ffi to retrieve Nginx status
data.
The "/nginx_status" location block is left in nginx-kong.conf as other
components may depend on it.