containerd / containerd/typeurl

TypeURL behavior is inconsistent with MarshalAny

Open
#47 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
55
Forks
21
PR merge metrics
No merged PRs in 30d

Description

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](https://github.com/moby/buildkit/blob/master/util/grpcerrors/grpcerrors.go#L83-L91) instead of using `MarshalAny`. But, it is deserialized with [UnmarshalAny](https://github.com/moby/buildkit/blob/bc92b63b98aa0968614240082997483f6bf68cbe/util/grpcerrors/grpcerrors.go#L172). 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.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.