feat(proxy-wasm) add get_property, supporting Nginx variables

[hishamhm]

Implements the get_property call for the proxy-wasm SDK.

The actual properties themselves are not listed by the proxy-wasm specification, so they seem to be proxy-specific.

In this initial iteration, the only properties implemented are Nginx variables, obtained using the Nginx API via ngx_http_get_variable and hence exposed in the ngx.* property path.

The ABI for property path values (that is, how they should be parsed by a host implementation) is not specified in the proxy-wasm spec docs either, so I have followed an implementation compatible with the input produced by the proxy-wasm-rust-sdk, which expects from the user a path as a vector of strings (vec!["ngx", "pid"] for ngx.pid) and produces a byte string to the host by joining them using \0 bytes.

With regard to property support, we could mimic the support for many Envoy attributes which are exposed via get_property by mapping them to the equivalent Nginx data. The list is available here: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/advanced/attributes.html?highlight=attributes

However, the future of get_property and set_property is currently put into question: they are not included in the vNEXT docs, at least as of their state in 2020: see https://github.com/proxy-wasm/spec/pull/1/files#r472951567 — "I've dropped {get,set}_property from this iteration, since the current implementation is not really portable (it's an opaque pass-through to CEL), and I wanted to get a consensus on this MVP first... but we should definitely add something that can be standardized in its place as soon as this is merged.". More info about the current lack of standardization of properties
here: proxy-wasm/proxy-wasm-cpp-host#90

The Envoy properties seem to be the "de facto" properties for proxy-wasm and which properties are proxy-specific or proxy-independent are ill-defined (they just map to the Envoy internals), but on our side, we start from a clean slate by using the ngx namespace for entries that map 1-to-1 to Nginx values.

[codecov]

Codecov Report

Merging #103 (dcf1086) into main (7f20c12) will decrease coverage by 0.41%.
The diff coverage is 94.87%.

@@            Coverage Diff             @@
##             main     #103      +/-   ##
==========================================
- Coverage   86.28%   85.87%   -0.42%     
==========================================
  Files          34       33       -1     
  Lines        5426     5288     -138     
==========================================
- Hits         4682     4541     -141     
- Misses        744      747       +3     

Flag	Coverage Δ
unit	85.87% <94.87%> (-0.42%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/common/proxy_wasm/ngx_proxy_wasm_host.c	86.60% <94.59%> (+0.80%)	⬆️
src/http/proxy_wasm/ngx_http_proxy_wasm.h	100.00% <100.00%> (ø)
src/wasm/wasi/ngx_wasi_host.c	0.00% <0.00%> (-81.82%)	⬇️
src/wasm/vm/ngx_wavm_host.c	82.02% <0.00%> (-0.96%)	⬇️
src/wasm/wrt/ngx_wrt_v8.c

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f20c12...dcf1086. Read the comment docs.

[thibaultcha]

Awesome. Are you still planning on adding more tests?

[hishamhm]

Added the "property not found" test, coverage should be good now (apart from the usual memory failure lines)

[thibaultcha]

A thought here is we should have a quick glance at the available Envoy properties and assess if they expose relevant info one may need in the root context, such as $pid, $http_listeners, and other things one may need in background ticks. When we do implement an Envoy-compatibility layer for properties (it should be fun to do, we kinda have that for req/resp headers already!), each property will invoke its own handler computing its value. In the handler, we might have to retrieve some values from the http{} or stream{} subsystems, and we might have to create fake ngx_http_requests for the sake of invoking these handlers, if we need to (like we do for the sock_tcp http reader). Or we might write new ways of generating these values altogether in our own properties handlers if it makes more sense.

Anyway, where I am going with all this is to the statement on this test's line: I think we may have to give access to ngx_http_* calls to the root context, either later on (adding the Envoy layer), or perhaps sooner (if the Envoy properties aren't really useful for background ticks). We have potential to write something elegant and subsystem-agnostic, with some namespace detection and matching to subsystem, with a waterfall-like set of handlers (i.e. ngx.http.* | http{} handlers are provisioned a fake ngx_http_request, while ngx.stream.* | stream{} handlers provisioned a fake ngx_stream_request, ngx.wasm.* wasm{} handlers are given the VM, and so on for ngx.* or Envoy properties).

[hishamhm]

A thought here is we should have a quick glance at the available Envoy properties and assess if they expose relevant info one may need in the root context, such as $pid, $http_listeners, and other things one may need in background ticks. When we do implement an Envoy-compatibility layer for properties (it should be fun to do, we kinda have that for req/resp headers already!), each property will invoke its own handler computing its value.

Yes, I was thinking pretty much on the same lines as a follow-up step, after getting the basic PRs for get_property and set_property in. What I had in mind was to take a look at some existing real-world proxy-wasm filters and see which properties they use, to prioritize which ones to put on a first batch.

In the handler, we might have to retrieve some values from the http{} or stream{} subsystems, and we might have to create fake ngx_http_requests for the sake of invoking these handlers, if we need to (like we do for the sock_tcp http reader). Or we might write new ways of generating these values altogether in our own properties handlers if it makes more sense.

Ah, I didn't occur to me to create fake ngx_http_requests! But I think that for the kinds of valid data we would be getting with fake requests, it might indeed be easier to generate the data ourselves (e.g. just using getpid() to get the pid, etc.)

[hishamhm]

I'm going to agree to disagree with clippy on this one because I expect to add more cases to this match in the set_property PR

[thibaultcha]

Agreed, we can add a preprocessor flag to ignore this warning iirc.

[hishamhm]

The warning is not causing clippy to fail (and the codecov failure seems to be because it didn't upload the v8 stats correctly), so I'd say let's just merge as-is and it will sort itself out next week with the set_property PR.

[thibaultcha]

Hmmm the problem with such a test plan is that putting tests in isolation may fail for some tests, but not others. Typically all tests have the same amount of assertions for this reason, with "stub" assertions for fillers. Alternatively, many tests here check the response body, but could ignore it instead, so all tests could have 4 assertions.

[thibaultcha]

Touching this one up locally and merging it.