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

Replace gogo protobuf with golang protobuf #213

Closed
qiwzhang opened this issue Aug 15, 2019 · 21 comments · Fixed by #226
Closed

Replace gogo protobuf with golang protobuf #213

qiwzhang opened this issue Aug 15, 2019 · 21 comments · Fixed by #226
Assignees

Comments

@qiwzhang
Copy link

Certain environments can only use golang protobuf. This repo is using gogo which blocks its adaptions in these environments.

What does it take to replace gogo proto with golang proto?

@qiwzhang
Copy link
Author

@kyessenov ?

@kyessenov
Copy link
Contributor

It's certainly possible. The idea was to wait for the next-generation golang/protobuf v2 which is expected before the end of the year. Presumably, that would solve our performance issues that motivated the choice of gogo/protobuf.

We don't have resources to have two forks, one for gogo and another for golang. So the change is going to be breaking, and it needs to be done with care. Since golang/protobuf v2 will also break compatibility, we don't want to go through two breaking changes at once.

@qiwzhang
Copy link
Author

What is the resource issue for supporting two separate servers; one for gogo and another for golang?

For a new project without compatibility issue, and can only support golang/protobuf, it can start to use golang one. For Istio, it can stay with gogo one.

@kyessenov
Copy link
Contributor

If you want to help, we can create a branch for goproto here, and convert all code to use goproto there. This can stay non-default until goproto v2 lands.

@nareddyt
Copy link

I can work on this. I'll fork it, fix any incompatibilities, and then make a PR to a new branch.

@kyessenov
Copy link
Contributor

Awesome. You can use bazel to copy generated files from Envoy submodule if you prefer. We can call the branch "goproto" once it is ready.

@nareddyt
Copy link

I'm not sure if bazel building and copying would work well. Looking through the data-plane-api, some build files are missing the api_go_proto_library build rule to generate go protos. Perhaps all the protos that go-control-plane uses has go_proto_library rules, but I am not 100% sure.

Would you recommend using bazel, or modify the ./build/generate_protos script instead?

@kyessenov
Copy link
Contributor

I'm perfectly happy with build/generate_protos. The situation with go_proto_library in envoy deserves its own solution, so it should not block you.

@nareddyt
Copy link

nareddyt commented Aug 19, 2019

Hi Kuat,

I have most of the code converted over to use goproto instead of gogoproto. This is viewable on my fork.

However, I'm having trouble with protoc-gen-validate. It seems like none of the validation rules are being generated (even through the *.pb.validate.go files are being generated). An example is my route.pb.validate.go, where no validation is done on name or routes fields. This contrasts with the current route.pb.validate.go in this repo.

Any ideas why the generation isn't done? This is causing the unit tests in /pkg/cache/resource_test.go to fail. This is the snippet of code I'm using to generate the data-plane-api protos in my build/generate_protos.sh:

$protoc ${protocarg} ${path}/*.proto \
      --go_out=${goprotoarg}:. \
      --validate_out="lang=go:."

Note that I generate the protos with my local workspace, since the docker image in build/run_docker.sh does not have protoc-gen-go installed.

@kyessenov
Copy link
Contributor

I'm not sure why protoc-gen-validate fails here. Maybe try using the same pinned version that is picked in this repo? Alternatively, there's some debugging log that you could get out of protoc-gen-validate (https://github.com/envoyproxy/protoc-gen-validate/blob/master/vendor/github.com/lyft/protoc-gen-star/debug.go).

@nareddyt
Copy link

Thanks for the response! Turns out it was a version issue with protoc-gen-validate. I reset protoc-gen-validate to commit d6164de49109, and it worked.

@kyessenov
Copy link
Contributor

Branch https://github.com/envoyproxy/go-control-plane/tree/v2 is ready. @nareddyt can you take a glance to see if everything is alright?

@nareddyt
Copy link

nareddyt commented Sep 6, 2019

I took a quick look, LGTM. I'll take a deeper look tomorrow.

As we discussed offline, we will have to create seperate imports for GRPC services. For example, pkg/server/server.go should have 1 import for the ADS proto and another import for the ADS services. I'll make a PR tomorrow for that.

@kyessenov
Copy link
Contributor

Sure.
@mattklein123 or @lizan or any org admin - can we enable branch protection for v2 branch so we can start sending PRs there?

@lizan
Copy link
Member

lizan commented Sep 6, 2019

I'm not org admin, @envoyproxy/admins

@mattklein123
Copy link
Member

@kyessenov what is your plan exactly? Don't you want to auto-commit to master from Envoy CI?

@kyessenov
Copy link
Contributor

Yes, I do. I just want to prevent human commits to the branch. I thought that granting admin access to whoever authors the automated PRs (a bot?) to v2 branch would be sufficient.
Note that I hesitate making v2 the master since that would confuse go dependency tools. Maybe in a year when everyone uses go modules that would work.

@lizan
Copy link
Member

lizan commented Sep 7, 2019

A random thought, can we name that branch differently? “v2” is confusing enough since we will start Envoy API v3alpha, v3 etc very soon.

@kyessenov
Copy link
Contributor

kyessenov commented Sep 9, 2019

After thinking a bit more, I think we should just pull the plug and merge golang protobuf into master and drop "v2" from module name. This is less complicated than what I had in mind. There's just no nice way to go about it given that:

  • gogo and golang protobufs are incompatible;
  • gogo 1.2 and gogo 1.3 are not backwards compatible;
  • golang v1 and v2 will have breaking changes but will get assistance to help with migration.

So let's keep it at v0.x.y, not v2, and make breaking changes when it simplifies things.

@nareddyt
Copy link

nareddyt commented Sep 9, 2019

Just a few concerns with merging into master:

  • Why do we plan on keeping the major version number as 0? Wouldn't this break semantic versioning and confuse our consumers?
  • How will this change affect Istio? I was under the assumption that all of Istio uses gogo/proto instead of golang/proto for performance reasons. Is that something we should be worried about?

@kyessenov
Copy link
Contributor

  • Why do we plan on keeping the major version number as 0? Wouldn't this break semantic versioning and confuse our consumers?

Version number 0 does not follow semantic versioning. There is no promise of compatibility between minor/patch updates in v0.

  • How will this change affect Istio? I was under the assumption that all of Istio uses gogo/proto instead of golang/proto for performance reasons. Is that something we should be worried about?

Yes, but the performance requirements are not as stringent in istio as they used to be. We're not using xDS on the data path, so the main concern is memory/cpu cost in the control plane. It's difficult to evaluate that without making the transition. Longer term, istio is looking to move to standard protos. I think we can keep gogo branch around in case it takes longer to make the switch, but that would be a dead end and manually maintained.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants