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

fix(admin) avoid Lapis handler when handler doesn't exit explicitly #4077

Merged
merged 1 commit into from Dec 13, 2018

Conversation

Projects
None yet
3 participants
@hishamhm
Copy link
Member

hishamhm commented Dec 12, 2018

If an Admin API handler did not exit explicitly using kong.response.exit (or
ngx.exit), it would end up concatenating HTML produced by Lapis (for an
example of this behavior, remove kong.response.exit(200) from the collect
function in kong-prometheus-plugin 0.3.2).

This ensures that only the handler function executes without falling into
Lapis.

Includes a plugin fixture and a regression test.

@hishamhm hishamhm requested review from kikito and thibaultcha Dec 12, 2018

@hishamhm

This comment has been minimized.

Copy link
Member Author

hishamhm commented Dec 12, 2018

This was prompted by @hbagdi's issue with the prometheus plugin here: Kong/kong-plugin-prometheus#32

Once he removed kong.response.exit he started getting HTML produced by Lapis. I have an alternate branch that makes kong.response.exit work like ngx.exit in that case (which would fix his problems) but I don't think this is the right way to go: sending ngx.exit(200) there "works", but the argument is silently ignored because the headers and body have already been sent. Our behavior of failing kong.response.exit(200) is more correct in that regard.

However, I don't know what the handler method there in kong.api.init is supposed to be returning anyway (if anything). I'd like to check with someone more knowledgeable about the flow here if my solution there is correct.

@hishamhm

This comment has been minimized.

Copy link
Member Author

hishamhm commented Dec 12, 2018

@bungle The patch as-is is breaking a lot of tests, I think it is breaking the delayed-response mechanism. But the general idea is that, if the plugin handler in api.lua doesn't return explicitly (with kong.responses.exit or ngx.exit) the code is currently falling through to Lapis and producing HTML. We want it to not do it.

@hishamhm hishamhm force-pushed the fix/exit-admin-api branch from ade9da9 to f2e4c66 Dec 12, 2018

@bungle bungle force-pushed the fix/exit-admin-api branch 4 times, most recently from 6b69374 to 02181f6 Dec 13, 2018

fix(admin) avoid Lapis handler when handler doesn't exit explicitly
If an Admin API handler did not exit explicitly using `kong.response.exit` (or
`ngx.exit`), it would end up concatenating HTML produced by Lapis (for an
example of this behavior, remove `kong.response.exit(200)` from the `collect`
function in kong-prometheus-plugin 0.3.2).

This ensures that only the handler function executes without falling into
Lapis.

Includes a plugin fixture and a regression test.

@bungle bungle force-pushed the fix/exit-admin-api branch from 02181f6 to 5974bc4 Dec 13, 2018

@kikito

kikito approved these changes Dec 13, 2018

@hishamhm hishamhm merged commit cd7c67e into next Dec 13, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla All CLA requirements met.

@hishamhm hishamhm deleted the fix/exit-admin-api branch Dec 13, 2018

hbagdi added a commit to Kong/kong-plugin-prometheus that referenced this pull request Dec 14, 2018

fix: skip sending status code after body is sent
This commit fixes a bug where the plugin tries to send HTTP status code
after the response body is sent for `/metrics` endpoint.

This resulted in error logs in Kong.
A test has been added to ensure that no error logs are generated when
`/metrics` is scraped by Prometheus.

Another test has been added to ensure that the response body only
contains either metrics or comments describing the metrics.
This test is added to catch regression for
Kong/kong#4077, where Kong injected HTML
generated by the default Lapis handler.

Fixes #32

hbagdi added a commit to Kong/kong-plugin-prometheus that referenced this pull request Dec 14, 2018

fix: skip sending status code after body is sent
This commit fixes a bug where the plugin tries to send HTTP status code
after the response body is sent for `/metrics` endpoint.

This resulted in error logs in Kong.
A test has been added to ensure that no error logs are generated when
`/metrics` is scraped by Prometheus.

Another test has been added to ensure that the response body only
contains either metrics or comments describing the metrics.
This test is added to catch regression for
Kong/kong#4077, where Kong injected HTML
generated by the default Lapis handler.

Fixes #32

hbagdi added a commit to Kong/kong-plugin-prometheus that referenced this pull request Dec 14, 2018

fix: skip sending status code after body is sent
This commit fixes a bug where the plugin tries to send HTTP status code
after the response body is sent for `/metrics` endpoint.

This resulted in error logs in Kong.
A test has been added to ensure that no error logs are generated when
`/metrics` is scraped by Prometheus.

Another test has been added to ensure that the response body only
contains either metrics or comments describing the metrics.
This test is added to catch regression for
Kong/kong#4077, where Kong injected HTML
generated by the default Lapis handler.

Fixes #32 
From #33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.