feat(proxy-wasm) allow setting ":status" header

[hishamhm]

Since #484, we now have an easy way to implement support for setting the ":status" pseudo-header.

[codecov[bot]]

Codecov Report

Merging #532 (be79467) into main (7ee4eb4) will decrease coverage by 0.00513%. The diff coverage is 94.33962%.

Additional details and impacted files

[](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/532?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) ```diff @@ Coverage Diff @@ ## main #532 +/- ## =================================================== - Coverage 90.06471% 90.05958% -0.00514% =================================================== Files 47 47 Lines 10045 10070 +25 =================================================== + Hits 9047 9069 +22 - Misses 998 1001 +3 ``` | [Files](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/532?dropdown=coverage&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\_host.c](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/532?src=pr&el=tree&filepath=src%2Fcommon%2Fproxy_wasm%2Fngx_proxy_wasm_host.c&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-c3JjL2NvbW1vbi9wcm94eV93YXNtL25neF9wcm94eV93YXNtX2hvc3QuYw==) | `94.28191% <97.72727%> (-0.01158%)` | :arrow_down: | | [src/common/proxy\_wasm/ngx\_proxy\_wasm\_maps.c](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/532?src=pr&el=tree&filepath=src%2Fcommon%2Fproxy_wasm%2Fngx_proxy_wasm_maps.c&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-c3JjL2NvbW1vbi9wcm94eV93YXNtL25neF9wcm94eV93YXNtX21hcHMuYw==) | `93.55742% <77.77778%> (-0.40811%)` | :arrow_down: | | [Flag](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/532/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/532/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | `90.14578% <97.91667%> (+0.01435%)` | :arrow_up: | | [valgrind](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/532/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | `78.16817% <63.41463%> (+0.00437%)` | :arrow_up: | 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.

[thibaultcha]

Before merging this (I have not made any changes yet locally), I think we should add context checks for set_response_header to better capture which phases the function can be called from (and :status be set from, particularly). These context checks go in t/03-proxy_wasm/hfuncs/contexts.

[hishamhm]

I think we should add context checks for set_response_header to better capture which phases the function can be called from (and :status be set from, particularly).

@thibaultcha How hard should the context checks be? In the current code, headers being set in the wrong context are soft errors, and bodies being set in the wrong context are hard errors.

For headers, functions such as ngx_proxy_wasm_hfuncs_replace_header_map_value and ngx_proxy_wasm_hfuncs_set_header_map_pairs return OK and just log an error when attempting to set response header when the headers are already sent.

For bodies, in ngx_proxy_wasm_hfuncs_set_buffer, attempting to set the response body or request body from the wrong place trigger a trap.

To clarify, by "add context checks" are you suggesting we turn the header checks mentioned above into hard errors?

[hishamhm]

@thibaultcha is this second commit 6014ff0 what you had in mind?

[thibaultcha]

@thibaultcha is this second commit 6014ff0 what you had in mind?

Yes, strengthen our context checks and formalize the behavior in each unsupported phase. Regarding the tests themselves, setting a response header in the request phases will not have any effect so I think we should catch usage there is what I meant, cutting some decisions where the behavior is "strange" or "undefined". For example, the new :status pseudo-header may work when used from the request phases(?), but that's different from pure response headers (or special response headers)... As this adds more behavior into the response headers, I think we should cover expected cases and make some decisions wherever we have margin (do we allow usage or not, how consistent are we between use-cases or sister APIs like set_body, etc...).

[hishamhm]

@thibaultcha The added context checks and tests should be ready for review now!

[thibaultcha]

Thanks! The code etc all looks good; I'll just reorganize the tests to try not to duplicate test cases between hfuncs/ and hfuncs/contexts (also the contexts tests are supposed to be non-functional - only test the return code - but maybe they should be to avoid confusion, I'll compare with the other suites).

[hishamhm]

Thanks! I did notice some duplication there, but when in doubt I added stuff to both, just in case. The main difference I could spot is that hfuncs/contexts tests are using context-checks which is testing at the extern "C" low-level (e.g proxy_set_buffer_bytes), and the ones in hfuncs/ are using proxy-wasm-rust-sdk (e.g. ctx.set_http_response_headers).