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

Enable type names for Protocol Buffer message types to support Any decoding. #262

Merged
merged 5 commits into from
Apr 26, 2024

Conversation

tobz
Copy link
Contributor

@tobz tobz commented Apr 23, 2024

Context

When using something like EventsClient::subscribe, the Envelope value returned provides the event data (if any) behind prost_types::Any.

This value can be fallibly decoded to the underlying type (such as containerd_client::events::ContainerCreate for a container create event) by using Any::to_msg<M>, but only if M implements Name.

Currently, based on how tonic_build is used to do the codegen, the Name trait is not implemented for any of the generated message types, making decoding much harder to do.

Solution

This PR is a simple change that simply provides additional configuration in build.rs, enabling the generation of Name implementations for message types. Builder::compile_with_config simply re-applies any of its own configuration on top of the configuration value given, so we're essentially just starting with a base of "enable type Name generation" and whatever else comes after (via tonic_build::configure()...) gets applied on top.

@tobz tobz changed the title Enable type names for Protocol Buffer message types to support Any decoding. Enable type names for Protocol Buffer message types to support Any decoding. Apr 23, 2024
@github-actions github-actions bot added C-client Containerd client T-CI Changes in project's CI labels Apr 23, 2024
@jsturtevant
Copy link
Contributor

could you add an example in https://github.com/containerd/rust-extensions/tree/main/crates/client/examples that demonstrates this? It would also ensure we have a way to validate it.

@mxpv
Copy link
Member

mxpv commented Apr 24, 2024

It would also ensure we have a way to validate it.

I think a unit test with decoding should do it ?

Generally the PR is SGTM.

@tobz
Copy link
Contributor Author

tobz commented Apr 25, 2024

I pushed both a unit test and an example of listening for and decoding event payloads.

Admittedly, I have not yet tested the example.

@tobz
Copy link
Contributor Author

tobz commented Apr 26, 2024

Alright, I ended up testing this and it seems to work, but with one caveat.

You'll see in the example that I had to futz with the type URL field for the Any payloads. It appears that containerd generates non-compliant type URLs by not emitting a leading slash, so that a payload for the ContainerCreate event has a type URL of containerd.events.ContainerCreate, when prost generates the type URL as /containerd.events.ContainerCreate.

The Protocol Buffers definition for Any states that there must be at least one forward slash in the type URL, which seems to be what prost is following and adhering to... but practically, it also seems pointless to have if there's no actual leading URL, given how all of this stuff was based on the premise of querying some schema/type registry endpoint and handling it all automagically... so I can understand why containerd just omitted the leading slash.

Either way, the functionality does work, so long as we adjust the type URL to make prost happy. Hopefully that's sufficient.

Example output:

toby@consigliera:~/src/rust-extensions$ cargo run --example container_events
    Finished dev [unoptimized + debuginfo] target(s) in 0.12s
     Running `target/debug/examples/container_events`
container created: id=0f550a4e94ae12a8881cf017e14e3b505ea62b022f21876613490f99ce9549e6 payload=ContainerCreate { id: "0f550a4e94ae12a8881cf017e14e3b505ea62b022f21876613490f99ce9549e6", image: "", runtime: Some(Runtime { name: "io.containerd.runc.v2", options: Some(Any { type_url: "containerd.runc.v1.Options", value: [50, 4, 114, 117, 110, 99, 58, 28, 47, 118, 97, 114, 47, 114, 117, 110, 47, 100, 111, 99, 107, 101, 114, 47, 114, 117, 110, 116, 105, 109, 101, 45, 114, 117, 110, 99, 72, 1] }) }) }
container deleted: id=0f550a4e94ae12a8881cf017e14e3b505ea62b022f21876613490f99ce9549e6 payload=ContainerDelete { id: "0f550a4e94ae12a8881cf017e14e3b505ea62b022f21876613490f99ce9549e6" }

Copy link
Contributor

@jsturtevant jsturtevant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I tried this out and it is working.

LGTM

@mxpv mxpv added this pull request to the merge queue Apr 26, 2024
Merged via the queue into containerd:main with commit dcc8e39 Apr 26, 2024
18 checks passed
@tobz tobz deleted the tobz/prost-type-names branch April 26, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-client Containerd client T-CI Changes in project's CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants