-
Notifications
You must be signed in to change notification settings - Fork 29
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
Document Proxy-Wasm ABI v0.2.1. #42
Conversation
Please take a look, I'm sure there are some errors, but it should be fairly complete. I'll wait a week or two before merging this to make sure all mistakes are caught and fixed. Afterwards, I'll push v0.1.0 and v0.2.0 for completeness, since it's going to be trivial to do those. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started making some comments, then realized they would be the same through the whole doc. Hopefully this gives a good idea of what may be helpful.
abi-versions/v0.2.1/README.md
Outdated
Called when the Wasm module is first loaded. | ||
|
||
|
||
### `proxy_on_memory_allocate` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe somewhere say that either proxy_on_memory_allocate
or malloc
may be absent (not exported by the guest), but not both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little tricky. One of those callbacks is required in the current Envoy implementation, but technically plugins don't need to export this if they don't retrieve strings/vectors/buffers/maps/properties from the host.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks for writing it!
- `SERIALIZATION_FAILURE` TODO | ||
- `INVALID_MEMORY_ACCESS` when `path_data`, `path_size`, | ||
`return_value_data` and/or `return_value_size` point to invalid | ||
memory address. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incomplete without a description of the encoding of the input / output I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, but I don't think that should hold up this PR; personally I'd be fine with it landing in a follow-on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I'm treating proxy_{get,set}_property
in v0.2.1 as a opaque pass-thru to CEL (hence the comment at the beginning of this section), and I was hoping not to document existing paths, since the set isn't stable across host implementations.
Maybe we could redirect users to the documentation in proxies for CEL attributes?
In any case, I think CEL makes sense for a lot of use-cases (e.g. when used in expressions, or to access unspecified objects like Envoy's filter state or dynamic metadata), but I don't think it works great as a generic property getter for predefined values.
Notably:
- Querying using strings vs enums is wasteful, and inconsistent with rest of the Proxy-Wasm ABI.
- The set isn't stable, so it's impossible to enforce it at the ABI layer, and we had issues with that in the past.
- There is a some overlap with existing functions (e.g. access to header maps), which is source of some confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it works great as a generic property getter for predefined values.
huge +1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM mod comments, acknowledging some TODOs could be addressed in later PRs.
- `SERIALIZATION_FAILURE` TODO | ||
- `INVALID_MEMORY_ACCESS` when `path_data`, `path_size`, | ||
`return_value_data` and/or `return_value_size` point to invalid | ||
memory address. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, but I don't think that should hold up this PR; personally I'd be fine with it landing in a follow-on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haven't reviewed all, but flushing the first comments. Thank you so much for your work! @PiotrSikora
- `SERIALIZATION_FAILURE` TODO | ||
- `INVALID_MEMORY_ACCESS` when `path_data`, `path_size`, | ||
`return_value_data` and/or `return_value_size` point to invalid | ||
memory address. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it works great as a generic property getter for predefined values.
huge +1
cc @spacewander @shukitchan as non-Envoy implementors |
* returns: | ||
- `i32 (`[`proxy_status_t`]`) status` | ||
|
||
Defines a new metric of type `metric_type` with a name (`name_data`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of questions about behaviors that are not specified here:
-
What happens if I give it a metric name that was previously defined? Do I get the preexisting id in
return_metric_id
?- What happens if I pass a pre-existing name but with a different
metric_type
?- Are the names namespaced by type and I get a new metric id?
- Do I get a
BAD_ARGUMENT
error? - Do I get the preexisting id and the type is ignored?
- Do I get the preexisting id and the type is updated?
- What happens if I pass a pre-existing name but with a different
-
This call creates a metric; how do I dispose of a metric? For other functions such as
proxy_set_property
setting the value to NULL can be assumed to mean "unset", but how to "undefine" and release memory from metrics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if I give it a metric name that was previously defined? Do I get the preexisting id in
return_metric_id
?
What happens if I pass a pre-existing name but with a different
metric_type
?
- Are the names namespaced by type and I get a new metric id?
- Do I get a
BAD_ARGUMENT
error?- Do I get the preexisting id and the type is ignored?
- Do I get the preexisting id and the type is updated?
The current behavior (i.e. that matching Envoy) is that there can be different metric types that use the same metric name.
Also, Envoy returns a new return_metric_id
for each call (even for the same metric name and type), but you should return the same metric ID for the same metric and type combination.
- This call creates a metric; how do I dispose of a metric? For other functions such as
proxy_set_property
setting the value to NULL can be assumed to mean "unset", but how to "undefine" and release memory from metrics?
Removing of various resources (including metrics) is not well supported in v0.2.1 (which this PR tries to document). In the future, you'd use proxy_delete_metric.
@PiotrSikora any update on when this might be merged? |
I'll try to wrap this this week. |
🎉 |
@PiotrSikora Is there anything blocking this? Reference documentation for |
Nothing major, I need to address some comments and do a minor cleanup. It's finally on top of my TODO list, so I should land it in a day or two. |
Signed-off-by: Piotr Sikora <code@piotrsikora.dev>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for creating this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you - my comments can be addressed in a follow-up.
Signed-off-by: Piotr Sikora <code@piotrsikora.dev>
Signed-off-by: Piotr Sikora <code@piotrsikora.dev>
Signed-off-by: Piotr Sikora <code@piotrsikora.dev>
Signed-off-by: Piotr Sikora <code@piotrsikora.dev>
Signed-off-by: Piotr Sikora <code@piotrsikora.dev>
Signed-off-by: Piotr Sikora <code@piotrsikora.dev>
Signed-off-by: Piotr Sikora <code@piotrsikora.dev>
Signed-off-by: Piotr Sikora <code@piotrsikora.dev>
Fixes #41.