Kong / ngx_wasm_module

Nginx + WebAssembly
Apache License 2.0
80 stars 7 forks source link

feat(proxy-wasm) support 'send_local_response' during on_response_headers #484

Closed thibaultcha closed 7 months ago

thibaultcha commented 7 months ago

Replaces #470 by implementing the feature at the Proxy-Wasm host layer.

thibaultcha commented 7 months ago

@hishamhm I recall you were running some tests with various branches for this feature, would you give it another try with this branch?

codecov[bot] commented 7 months ago

Codecov Report

Merging #484 (f551c4a) into main (e8e9a59) will decrease coverage by 0.08867%. The diff coverage is 89.06250%.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/484/graphs/tree.svg?width=650&height=150&src=pr&token=T0PT2Q9IAN&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong)](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/484?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) ```diff @@ Coverage Diff @@ ## main #484 +/- ## =================================================== - Coverage 90.07548% 89.98681% -0.08867% =================================================== Files 46 46 Lines 9804 9857 +53 =================================================== + Hits 8831 8870 +39 - Misses 973 987 +14 ``` | [Files](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/484?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | Coverage Δ | | |---|---|---| | [src/common/proxy\_wasm/ngx\_proxy\_wasm.c](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/484?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-c3JjL2NvbW1vbi9wcm94eV93YXNtL25neF9wcm94eV93YXNtLmM=) | `92.64019% <100.00000%> (-0.74824%)` | :arrow_down: | | [src/http/ngx\_http\_wasm\_filter\_module.c](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/484?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-c3JjL2h0dHAvbmd4X2h0dHBfd2FzbV9maWx0ZXJfbW9kdWxlLmM=) | `92.89617% <100.00000%> (-0.88914%)` | :arrow_down: | | [src/http/ngx\_http\_wasm\_module.c](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/484?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-c3JjL2h0dHAvbmd4X2h0dHBfd2FzbV9tb2R1bGUuYw==) | `96.00939% <100.00000%> (+0.00938%)` | :arrow_up: | | [src/wasm/ngx\_wasm\_util.c](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/484?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-c3JjL3dhc20vbmd4X3dhc21fdXRpbC5j) | `92.27468% <100.00000%> (+0.03329%)` | :arrow_up: | | [src/common/proxy\_wasm/ngx\_proxy\_wasm\_host.c](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/484?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-c3JjL2NvbW1vbi9wcm94eV93YXNtL25neF9wcm94eV93YXNtX2hvc3QuYw==) | `94.27793% <90.90909%> (-0.11197%)` | :arrow_down: | | [src/http/ngx\_http\_wasm\_local\_response.c](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/484?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-c3JjL2h0dHAvbmd4X2h0dHBfd2FzbV9sb2NhbF9yZXNwb25zZS5j) | `85.47009% <66.66667%> (-1.08454%)` | :arrow_down: | | [src/http/ngx\_http\_wasm\_util.c](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/484?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-c3JjL2h0dHAvbmd4X2h0dHBfd2FzbV91dGlsLmM=) | `86.77686% <84.21053%> (+0.07165%)` | :arrow_up: | | [Flag](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/484/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | Coverage Δ | | |---|---|---| | [unit](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/484/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | `90.04964% <87.93103%> (-0.10707%)` | :arrow_down: | | [valgrind](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/484/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | `77.79134% <41.66667%> (-0.23321%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#carryforward-flags-in-the-pull-request-comment) to find out more.
hishamhm commented 7 months ago

@thibaultcha I gave it a try, but running the latest Kong master with the same filter I had compiled earlier and this branch is giving me "name resolution failed" responses:

HTTP/1.1 503 Service Temporarily Unavailable
Connection: keep-alive
Content-Length: 91
Content-Type: application/json; charset=utf-8
Date: Mon, 29 Jan 2024 21:48:27 GMT
Server: kong/3.6.0
X-Kong-Request-Id: daf81a78af4a005d961a3a106237643a
X-Kong-Response-Latency: 1

{
    "message": "name resolution failed",
    "request_id": "daf81a78af4a005d961a3a106237643a"
}

It's the end of my day here now, but I'll take another look at this tomorrow morning. Might be something silly on my end. (I already ran it with KONG_DNS_NO_SYNC=on just in case but it made no difference.)

thibaultcha commented 7 months ago

Hmmm that is strange, this should not rely to the DNS resolver at least not directly... Not sure what could be going on. I am spending some cycles on mlcache since it is being used by the new Gateway DNS resolver, but perhaps this needs some more work.

thibaultcha commented 7 months ago

Is your filter using a dispatch call prior to the response being produced? Because if so then it can be the same bug Damon experienced last week. If so, then dns_no_sync alone is not enough, you will also need this commit. Or producing a response without a hostname dispatch call.

hishamhm commented 7 months ago

Update: the name resolution issue was a misconfiguration on my end, you can ignore that.

The current status is that I'm getting the following:

2024/01/30 16:49:34 [error] 851930#0: *20552 [proxy-wasm] bad "on_response_headers" return action: "PAUSE" while reading response header from upstream, client: 127.0.0.1, server: kong, request: "GET /status/406 HTTP/1.1", upstream: "http://127.0.0.1:8080/status/406", host: "localhost:8000", request_id: "0c7b7a3e68611b7306574b4fa49d703c"

In both my version and upstream's version of the filter, ActionPause is called by OnHttpResponseHeaders (usually via ctx.handleInterruption), after a local response is produced.

This ends up causing a 500 error in a test case where a 403 (set by said local response) was wanted.

thibaultcha commented 7 months ago

@hishamhm Ok, that second commit should fix this!

hishamhm commented 7 months ago

@thibaultcha just tested the latest version of the PR, and yes, it does pass all of the e2e-example.sh tests running my fork of the coraza-waf filter :tada: