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

v2 protobuf Registry #10978

Closed
5 tasks
Tracked by #11277
aaronc opened this issue Jan 19, 2022 · 2 comments
Closed
5 tasks
Tracked by #11277

v2 protobuf Registry #10978

aaronc opened this issue Jan 19, 2022 · 2 comments
Labels
C: Proto Proto definition and proto release

Comments

@aaronc
Copy link
Member

aaronc commented Jan 19, 2022

As we migrate to pulsar and the api module, we want to have a better way of registering protobuf information with the codec than what currently happens with the InterfaceRegistry.

The main design principle is to have everything the registry needs to know about declaratively specified in the .proto files themselves so that the registry only requires that a module register file descriptors.

The proposed design is here:

type ModuleRegistrar interface {
// RegisterFiles registers the protobuf file descriptors for a module. pinnedProtoImageFS should
// be an embedded FS which contains a file - image.bin.gz - generated by buf build, that contains
// the pinned FileDescriptorSet that this module was built against. This pinned FileDescriptorSet
// will be used when performing unknown field rejection for correct API compatibility. The
// file descriptors also passed into RegisterFiles are the protobuf files that the module
// "owns" and should define relevant Msg and Query servers as well as ORM tables. Instead of
// using the file descriptors passed in at runtime, RegisterFiles will used the same versions
// of those file descriptors stored in the pinned FileDescriptorSet, in case the binary for
// a module was compiled with a newer build of the proto files that the module was developed against.
// This feature will eventually allow a single binary to contain multiple versions of the same module
// and perform upgrades without a restart.
RegisterFiles(pinnedProtoImageFS embed.FS, fileDescriptors ...protoreflect.FileDescriptor) error
}

Things we want to take care with the registry include:

  • register interface implementations
  • validate msg signers
  • ensure correct proto paths
  • use pinned file descriptors for unknown field rejection and gRPC reflection
  • store pinned file descriptors in state and update them with chain upgrades (probably in x/upgrade)

We may want to consider a middleware design pattern for the registry where different extensions can perform a validation step for .proto options they use.

@fdymylja
Copy link
Contributor

fdymylja commented Jan 19, 2022

I have noticed that what happens in the sdk, and applications build with the sdk, is that protobuf files are not imported as they register themselves.

For example, gogoproto is imported everywhere in the sdk proto files as gogoproto/gogo.proto, but gogo.proto generated code registers itself as gogo.proto.

This causes a bug in reflection services in which files cannot be correctly resolved, and this yields to invalid field descriptors.

NOTE: this might be irrelevant for protov2, pending investigation.

@fdymylja
Copy link
Contributor

We need to modify the grpc reflection service to return pinned protofiles instead of the files from the global registry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Proto Proto definition and proto release
Projects
No open projects
Development

No branches or pull requests

3 participants