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

Proposal: Check-in generated golang protos in envoy/api #8151

Closed
nareddyt opened this issue Sep 4, 2019 · 9 comments · Fixed by #8155
Closed

Proposal: Check-in generated golang protos in envoy/api #8151

nareddyt opened this issue Sep 4, 2019 · 9 comments · Fixed by #8155
Labels
api/v2 api/v3 Major version release @ end of Q3 2019 area/build design proposal Needs design doc/proposal before implementation enhancement Feature requests. Not bugs or questions.
Milestone

Comments

@nareddyt
Copy link
Contributor

nareddyt commented Sep 4, 2019

Hi envoy-api shepards,

Given the recent improvement to the golang APIs via #8003 and the upcoming goproto compatible version of go-control-plane via envoyproxy/go-control-plane#213, I wanted to propose another change: Is it possible that we could check in generate golang protos into the api/ folder?

Most golang code uses the built-in go build system instead of Bazel. This is a standard in the open-source golang community. This means that the generated golang protos (.pb.go files) must be checked into these repos, allowing consumers to depend upon them and build them using the go build system.

Currently, envoy's api repository does not have the generated golang protos. Instead, both go-control-plane/envoy and gprc-go/xds/internal/proto/envoy contain the generated envoy golang protos. This is a problem because:

  1. Each repo needs to manually update their generated protos everytime any protos in envoy are updated
  2. There is no single source-of-truth for generated protos. We could modify either go-control-plane or grpc-go to serve as the SOT, but it would bring in more dependencies than required. The ideal solution is to have the data-plane-api be the SOT.
  3. We recently split the UDPA apis into cncf/udpa. With this change, grpc-go was also forced to add generation for the UDPA protos. Here are the generated protos in grpc-go. Once again, this breaks the single source-of-truth.
  4. In the worst case, it may be impossible for external golang code to import both go-control-plane and grpc-go due to "duplicate proto type registered" errors. This would be extremely painful to solve.

Checking in generated files is OK for golang protos. Other than go-control-plane and grpc-go, some other repositories also follow the same structure for non-envoy protos:

  • census-instrumentation/opencensus-proto in the gen-go/ folder
  • googleapis/go-genproto in the googleapis/ folder

It would be ideal to have golang protos auto-generated and checked-in to the api/ folder whenever any of the underlying protos files are updated. Proto generation would occur with protoc using grpc and protoc-gen-validate.

I know that envoy primarily supports C++, but this change would help drive golang adoption for the control plane.

@htuch
Copy link
Member

htuch commented Sep 4, 2019

@nareddyt I get that this is the way that the Go proto ecosystem tends to work. Here is how I can see it being realistic for Envoy if you want this:

  1. We do this in a mirror repository, e.g. https://github.com/envoyproxy/data-plane-api. This avoid https://github.com/envoyproxy/envoy noise each time we do some API changes.
  2. We figure out how to do this with Bazel (not a parallel ad-hoc proto generation system).
  3. We have an OWNER for this who can do the original work and be an on-going support/maintenance person from the Go world.

If we have all the above satisfied, I think this would be totally reasonable and I'm supportive.

@htuch htuch added design proposal Needs design doc/proposal before implementation enhancement Feature requests. Not bugs or questions. api/v3 Major version release @ end of Q3 2019 labels Sep 4, 2019
@htuch
Copy link
Member

htuch commented Sep 4, 2019

CC other @envoyproxy/api-shepherds

@kyessenov
Copy link
Contributor

I agree that the generated go protobufs need to be published somewhere to avoid bifurcation in go/envoy ecosystem. I was going to add a script to mirror generated go files from bazel to a repo (something like https://github.com/kythe/kythe/blob/master/kythe/proto/generate_go_protobufs.py). The question would remain about versioning scheme. I also had work in progress to move gogo ad-hoc script into bazel world.

@nareddyt
Copy link
Contributor Author

nareddyt commented Sep 5, 2019

Thanks for the quick reply @htuch. Some of my thoughts:

  1. That makes sense. If these could be auto-generated from any changes to envoy/api but checked into only envoyproxy/data-plane-api, that would be perfect.
  2. With api: organize go_proto_libraries #8003 merged it, it should be possible to generate golang protos in the correct directory structure using Bazel. I can work with Kuat on a prototype if needed.
  3. I can help maintain the setup (if needed), but I am new to the golang ecosystem. I'd certainly need help doing so, and help with the initial setup of this.

@mattklein123
Copy link
Member

I'm strongly in favor of the solution that @htuch proposes which is to mirror the Go protos on every master commit into a new repo. I think this should be pretty easy and would be great for the community. I know we would use this at Lyft and I was actually discussing this with @lita and @ryancox today. It's possible we could help out with this also.

@kyessenov
Copy link
Contributor

It should be possible to create a new branch in go-control-plane (v2?) to host the standard protos. The import path would be a bit strange, e.g. github.com/envoyproxy/go-control-plane/v2/envoy/api/v2/core.

@kyessenov
Copy link
Contributor

PR #8163 deleted all gogoproto annotations to help with separating go/gogo proto builds.

@nareddyt
Copy link
Contributor Author

Status Update:

@kyessenov: Should this issue be closed now, or will you close it after the CI change?

@mattklein123 mattklein123 added this to the 1.12.0 milestone Sep 11, 2019
@mattklein123
Copy link
Member

Let's close this after we get CI auto-merge working.

htuch pushed a commit that referenced this issue Sep 19, 2019
Adds a script to create a go module from the generated protobufs as part of #8151.
The module appears to build with the following module declaration:

module github.com/envoyproxy/data-plane-api/api

go 1.12

require (
        github.com/census-instrumentation/opencensus-proto v0.2.1
        github.com/envoyproxy/protoc-gen-validate v0.1.0
        github.com/gogo/protobuf v1.3.0
        github.com/golang/protobuf v1.3.2
        github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4
        google.golang.org/genproto v0.0.0-20190819201941-24fa4b261c55
        google.golang.org/grpc v1.23.0
)
Add CI automation to trigger the script after the merge to master in envoyproxy.

Risk Level: low
Testing: local build
Docs Changes: none
Release Notes: none

Fixes #8151 

Signed-off-by: Kuat Yessenov <kuat@google.com>
danzh2010 pushed a commit to danzh2010/envoy that referenced this issue Sep 24, 2019
Adds a script to create a go module from the generated protobufs as part of envoyproxy#8151.
The module appears to build with the following module declaration:

module github.com/envoyproxy/data-plane-api/api

go 1.12

require (
        github.com/census-instrumentation/opencensus-proto v0.2.1
        github.com/envoyproxy/protoc-gen-validate v0.1.0
        github.com/gogo/protobuf v1.3.0
        github.com/golang/protobuf v1.3.2
        github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4
        google.golang.org/genproto v0.0.0-20190819201941-24fa4b261c55
        google.golang.org/grpc v1.23.0
)
Add CI automation to trigger the script after the merge to master in envoyproxy.

Risk Level: low
Testing: local build
Docs Changes: none
Release Notes: none

Fixes envoyproxy#8151 

Signed-off-by: Kuat Yessenov <kuat@google.com>
danzh2010 pushed a commit to danzh2010/envoy that referenced this issue Oct 4, 2019
Adds a script to create a go module from the generated protobufs as part of envoyproxy#8151.
The module appears to build with the following module declaration:

module github.com/envoyproxy/data-plane-api/api

go 1.12

require (
        github.com/census-instrumentation/opencensus-proto v0.2.1
        github.com/envoyproxy/protoc-gen-validate v0.1.0
        github.com/gogo/protobuf v1.3.0
        github.com/golang/protobuf v1.3.2
        github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4
        google.golang.org/genproto v0.0.0-20190819201941-24fa4b261c55
        google.golang.org/grpc v1.23.0
)
Add CI automation to trigger the script after the merge to master in envoyproxy.

Risk Level: low
Testing: local build
Docs Changes: none
Release Notes: none

Fixes envoyproxy#8151 

Signed-off-by: Kuat Yessenov <kuat@google.com>
danzh2010 pushed a commit to danzh2010/envoy that referenced this issue Oct 4, 2019
Adds a script to create a go module from the generated protobufs as part of envoyproxy#8151.
The module appears to build with the following module declaration:

module github.com/envoyproxy/data-plane-api/api

go 1.12

require (
        github.com/census-instrumentation/opencensus-proto v0.2.1
        github.com/envoyproxy/protoc-gen-validate v0.1.0
        github.com/gogo/protobuf v1.3.0
        github.com/golang/protobuf v1.3.2
        github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4
        google.golang.org/genproto v0.0.0-20190819201941-24fa4b261c55
        google.golang.org/grpc v1.23.0
)
Add CI automation to trigger the script after the merge to master in envoyproxy.

Risk Level: low
Testing: local build
Docs Changes: none
Release Notes: none

Fixes envoyproxy#8151 

Signed-off-by: Kuat Yessenov <kuat@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api/v2 api/v3 Major version release @ end of Q3 2019 area/build design proposal Needs design doc/proposal before implementation enhancement Feature requests. Not bugs or questions.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants