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

Schema registry: how are duplicated proto definitions handled #933

Closed
howardjohn opened this issue Feb 11, 2022 · 5 comments
Closed

Schema registry: how are duplicated proto definitions handled #933

howardjohn opened this issue Feb 11, 2022 · 5 comments

Comments

@howardjohn
Copy link

Sorry this is more of a question than an issue - hopefully this is an ok avenue for this.

I was looking into the new BSR. It looks great, but prompted one concern. My understand is its highly desirable to have only a single definition of any given protobuf definition. Having multiple will bloat binaries (some like XDS and Kubernetes add ~10mb to binaries!), and depending on protobuf version/options will either spam warnings or panic at runtime if this happens.

So in the current state, most projects with protos will ship some go library with those protos. Will use https://github.com/envoyproxy/go-control-plane/ for example. BSR now also publishes a copy at https://buf.build/envoyproxy/envoy.

I am wondering how these can gracefully co-exist. If everything in the world switch to BSR for everything, it seems it would work great. However, even if we chose to migrate our own project's imports over to the BSR, we still have transient dependencies importing the old go-control-plane.

Is there any plans to improve this, reasons its not an issue, etc?

@bufdev
Copy link
Member

bufdev commented Feb 11, 2022

This is an interesting one - could we find 15-20 to chat? Send us an email dev@buf.build or message us on slack https://buf.build/links/slack, we should work through this together.

@howardjohn
Copy link
Author

FWIW a way to reproduce is to take http://github.com/istio/istio then apply this diff

diff --git a/pilot/cmd/pilot-discovery/main.go b/pilot/cmd/pilot-discovery/main.go
index 82396a9da7..fa0c4596c6 100644
--- a/pilot/cmd/pilot-discovery/main.go
+++ b/pilot/cmd/pilot-discovery/main.go
@@ -19,9 +19,11 @@ import (

        "istio.io/istio/pilot/cmd/pilot-discovery/app"
        "istio.io/pkg/log"
+       "go.buf.build/protocolbuffers/go/envoyproxy/envoy/envoy/admin/v3"
 )

 func main() {
+       _ = adminv3.ClientResourceStatus_ACKED
        log.EnableKlogWithCobra()
        rootCmd := app.NewRootCommand()
        if err := rootCmd.Execute(); err != nil {

Then go run ./pilot/cmd/pilot-discovery:

panic: proto: file "udpa/annotations/migrate.proto" is already registered
        previously from: "github.com/cncf/xds/go/udpa/annotations"
        currently from:  "go.buf.build/protocolbuffers/go/cncf/xds/udpa/annotations"
See https://developers.google.com/protocol-buffers/docs/reference/go/faq#namespace-conflict


goroutine 1 [running]:
google.golang.org/protobuf/reflect/protoregistry.glob..func1({0x53af240, 0x54ad198}, {0x53af240, 0xc000b79770})
        /usr/local/google/home/howardjohn/go/pkg/mod/google.golang.org/protobuf@v1.27.1/reflect/protoregistry/registry.go:54 +0x1f4
google.golang.org/protobuf/reflect/protoregistry.(*Files).RegisterFile(0xc00012e108, {0x54ad198, 0xc000814c40})
        /usr/local/google/home/howardjohn/go/pkg/mod/google.golang.org/protobuf@v1.27.1/reflect/protoregistry/registry.go:128 +0x399
google.golang.org/protobuf/internal/filedesc.Builder.Build({{0x45b8be2, 0x39}, {0x777a4a0, 0x394, 0x394}, 0x0, 0x3, 0x5, 0x0, {0x53f15f0, ...}, ...})
        /usr/local/google/home/howardjohn/go/pkg/mod/google.golang.org/protobuf@v1.27.1/internal/filedesc/build.go:113 +0x1d6
google.golang.org/protobuf/internal/filetype.Builder.Build({{{0x45b8be2, 0x39}, {0x777a4a0, 0x394, 0x394}, 0x0, 0x3, 0x5, 0x0, {0x0, ...}, ...}, ...})
        /usr/local/google/home/howardjohn/go/pkg/mod/google.golang.org/protobuf@v1.27.1/internal/filetype/build.go:139 +0x198
go.buf.build/protocolbuffers/go/cncf/xds/udpa/annotations.file_udpa_annotations_migrate_proto_init()
        /usr/local/google/home/howardjohn/go/pkg/mod/go.buf.build/protocolbuffers/go/cncf/xds@v1.1.10/udpa/annotations/migrate.pb.go:418 +0x1d8
go.buf.build/protocolbuffers/go/cncf/xds/udpa/annotations.init.0()
        /usr/local/google/home/howardjohn/go/pkg/mod/go.buf.build/protocolbuffers/go/cncf/xds@v1.1.10/udpa/annotations/migrate.pb.go:361 +0x17

Will reach out on slack

@jon-whit
Copy link

jon-whit commented May 9, 2022

I have this same issue @howardjohn. Did you guys come up with a strategy to resolve it?

I have a private Go module that defines its own buf module, but it also imports definitions from a public Go module that imports sources generated by the Buf Registry. There are namespace conflicts on overlapping modules just like this issue reports.

@amckinney
Copy link
Contributor

A workaround was outlined in #1125

@howardjohn
Copy link
Author

I don't think that resolves it @amckinney . That allows someone to override the dependencies, but doesn't provide a way to migrate an existing project AFAIK?

For example, go.buf.build/protocolbuffers/go/envoyproxy/envoy/ will essentially never be usable, because gRPC itself depends on https://github.com/envoyproxy/go-control-plane, and it will lead to duplicate protos

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

No branches or pull requests

4 participants