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

TypeURL behavior is inconsistent with MarshalAny #47

Open
jsternberg opened this issue Jul 31, 2024 · 1 comment · May be fixed by #48
Open

TypeURL behavior is inconsistent with MarshalAny #47

jsternberg opened this issue Jul 31, 2024 · 1 comment · May be fixed by #48

Comments

@jsternberg
Copy link

The TypeURL function will return a typeurl that was either registered with this module or with protobuf. When you obtain the TypeURL, the priority of the choices goes:

  • github.com/containerd/typeurl/v2
  • google.golang.org/protobuf/proto
  • github.com/gogo/protobuf/proto

On the other hand, the MarshalAny function has a different behavior that's inconsistent with this lookup order. Instead, it has the following lookup order:

  • google.golang.org/protobuf/proto
  • github.com/gogo/protobuf/proto
  • github.com/containerd/typeurl/v2

Even though it has this lookup order internally, it uses TypeURL to fill in the typeurl for the any protobuf message. This causes inconsistent behavior when a type implements multiple methods of serialization. If a type has been registered with this library using Register and it is also a proto.Message, it will be given the typeurl that was registered, but it will be serialized with the protobuf format. When the other side tries to deserialize the message, it will get confused and use the wrong deserialization method.

This behavior is accidentally relied upon in buildkit. In buildkit, the message is serialized by using TypeURL and then manually invoking json.Marshal here instead of using MarshalAny. But, it is deserialized with UnmarshalAny. If you change the serialization to use MarshalAny it becomes broken because it uses the wrong TypeURL.

I am not sure what the correct order should be. My thought is that this package should prefer protobuf over the JSON representation, but as long as it is consistent, then I think it's fine. If the order is changed so protobuf serialization is preferred for TypeURL, buildkit will also need to be updated.

@dmcgowan
Copy link
Member

// Register a type with a base URL for JSON marshaling.

We should probably prefer JSON if the type is manually registered, otherwise proto can already be preferred by just not registering it with typeurl

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.

2 participants