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

containerd client needs a to_any() helper #362

Closed
estesp opened this issue Jan 9, 2025 · 3 comments · Fixed by #363
Closed

containerd client needs a to_any() helper #362

estesp opened this issue Jan 9, 2025 · 3 comments · Fixed by #363

Comments

@estesp
Copy link
Member

estesp commented Jan 9, 2025

In trying to use the transfer service in containerd, which uses opaque any->any interfaces for source and destination, I found that using the Rust containerd client failed as the types weren't recognized.

After some digging it appears that the containerd/typeurl package generates its notion of the "type_url" of a protobuf from the FullName entry. In Rust, prost generates both the fullname fn and the typeurl fn, but the typeurl starts with a '/' character, I think to meet the requirement that type Urls are supposed to contain at least one forward slash according to Google's protobuf documentation.

Given the use of FullName in Go in containerd/typeurl (ref the code here), I think a helper in the containerd client would make sense.

I came up with this code and tested it, and it works, but wanted feedback before I open a PR in case there is a better way to solve this. Using this helper, I can use the transfer service successfully when passing these Any types to the gRPC call via the transfer API.

fn to_any<T: Message + Name>(m: &T) -> Any {
    let mut anyt = Any::from_msg(m).unwrap();
    anyt.type_url = T::full_name();
    anyt
}

Ref: #282 (hopefully when complete, I can provide a nice example of using the transfer service to pull an image via a Rust client)

@mxpv
Copy link
Member

mxpv commented Jan 10, 2025

I dug prost crate a little bit more and found this.

Type URLs are generated by combining domain name and type's full name. We don't configure the domain part in rust extensions (but we can), so it's empty and hence all type urls start with /.

{domain_name}/{full_name}

In case of Go implementation, by default, typeurl library doesn't use any domains names, which is the case for the majority of proto types (unless you use typeurl.Register with a custom URL path).

Protobuf's well known types all start with type.googleapis.com/ (like type.googleapis.com/google.protobuf.Duration).

Adding domain prefix to containerd API types doesn't sound like a terrible idea to me and feels like the "right" way. So we could get containerd.io/containerd.services.version.v1.VersionResponse instead of containerd.services.version.v1.VersionResponse. This behavior would be more appropriate as its expected by default by any proto generator.

Though, given that containerd 2.0 is out, I'm not sure of compatibility consequences. So I don't mind adding a small helper on client's side as an easy path forward.

Possibly @dmcgowan or @kzys have more thoughts on this.

@estesp
Copy link
Member Author

estesp commented Jan 13, 2025

Adding domain prefix to containerd API types doesn't sound like a terrible idea to me and feels like the "right" way. So we could get containerd.io/containerd.services.version.v1.VersionResponse instead of containerd.services.version.v1.VersionResponse. This behavior would be more appropriate as its expected by default by any proto generator.

This makes sense to me to, but apparently we didn't in the early days of containerd, and I don't have any info on why/how those choices were made or if something was different given we were using that now deprecated gogo impl? Have no idea if that even matters to this discussion.. anyway.

Regardless, it does look like containerd/typeurl does return just the FullName() essentially for it's own computation of type URL, so at least for the "right now" solution, we need our Any types to also use the full_name() provided by prost, so at least they match.

I think anything more invasive about full type URLs would need yet-another major version boundary, unless we add some interim code for allowing either?

@dmcgowan thoughts?

@dmcgowan
Copy link
Member

Would it be possible in typeurl to support with and without domain on the lookup side and we can make it configurable on the register side? Considering these types go over the API, its not just a matter of a major version rev of containerd itself. We could be stricter for new API endpoints when they get added though. Either way, sounds like the client fix would be needed on this side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants