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(pdk) allow kong.response.exit on header_filter with no body #4039

Merged
merged 3 commits into from Dec 5, 2018

Conversation

Projects
None yet
4 participants
@kikito
Copy link
Member

commented Nov 30, 2018

This fix on kong.response.exit fixes unintended failures on the response-ratelimiting plugin which were introduced when it was translated to the PDK.

I temporarily activated the response-ratelimiting specs on travis and they worked (although I had to to re-launch them many times, they are still quite flaky).

This PR also includes some doc comments on the pdk_phase_checking function, please verify that those comments are correct.

Show resolved Hide resolved t/Util.pm Outdated
Show resolved Hide resolved t/Util.pm Outdated
Show resolved Hide resolved t/Util.pm Outdated
Show resolved Hide resolved t/Util.pm
location /t {
proxy_pass http://unix:$TEST_NGINX_NXSOCK/nginx.sock;
set $upstream_uri '/t';
set $upstream_scheme 'http';

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Nov 30, 2018

Member

Are those two variables used in this test case?

@thibaultcha thibaultcha added this to the 1.0.0rc4 milestone Dec 1, 2018

@kikito kikito force-pushed the fix-response-ratelimiting-exit branch 3 times, most recently from e476171 to 49a5ffd Dec 1, 2018

@bungle

This comment has been minimized.

Copy link
Member

commented Dec 3, 2018

What does ngx.ctx.KONG_EXITED mean after this change? It was about to signal early exit. E.g. a plugin that short-circuited proxying (usually in access phase and I think never in response phases, e.g. after proxying).

@thibaultcha

This comment has been minimized.

Copy link
Member

commented Dec 3, 2018

What does ngx.ctx.KONG_EXITED mean after this change?

KONG_EXITED is true, and so is KONG_PROXIED. That is Kong indeed proxied the request (no short-circuit), but Kong produced the response itself (i.e. the response seen by the client is not that which was received from the upstream). That is where kong.response.get_source() loses some of its precisions, but I don't believe the control variables to be incorrectly set.

@kikito kikito force-pushed the fix-response-ratelimiting-exit branch 2 times, most recently from 7f702d5 to 9c7f773 Dec 5, 2018

@thibaultcha thibaultcha force-pushed the fix-response-ratelimiting-exit branch 3 times, most recently from 9d81304 to 805cb30 Dec 5, 2018

@thibaultcha thibaultcha force-pushed the fix-response-ratelimiting-exit branch from 805cb30 to 6aeee60 Dec 5, 2018

@thibaultcha
Copy link
Member

left a comment

LGTM with tweaks. Merging since the error is a known one being investigated and should not delay this merge.

@thibaultcha thibaultcha merged commit 3c6a252 into next Dec 5, 2018

1 of 3 checks passed

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

@thibaultcha thibaultcha deleted the fix-response-ratelimiting-exit branch Dec 5, 2018

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.