Protobuf: cleanup both golang/protobuf and gogo/protobuf

[ahrtr]

What would you like to be added?

Both github.com/golang/protobuf and github.com/gogo/protobuf are deprecated.

• github.com/golang/protobuf is the first version of Go protocol buffer API. Deprecated.
• github.com/gogo/protobuf is actually a fork of github.com/golang/protobuf with extra code generation features. Deprecated.
• google.golang.org/protobuf is the second version of the Go protocol buffer API. Active.

In short, google.golang.org/protobuf is the future, so we should cleanup both github.com/golang/protobuf and github.com/gogo/protobuf, and only use google.golang.org/protobuf.

Why is this needed?

Same as above

cc @ptabor @serathius @spzala @dims @liggitt

[ahrtr]

This seems like a big change, please let me know if anyone has any comments or concerns.

[ajityagaty]

@ahrtr - Has it been decided to make the change? If so, can I take a swing at this?

[ahrtr]

@ajityagaty Thanks for your interest on this.

There is no any feedback from other maintainers yet, but I think we must do this because both github.com/golang/protobuf and github.com/gogo/protobuf are deprected, it's just a matter of time.

Since it's a big change, so please evaluate the impact and provide a detailed plan firstly. Afterwards, we can update the functional_test in the first step.

[ahrtr]

any updates on this? @ajityagaty

[ajityagaty]

@ahrtr - Apologies! I have not been able to look at this yet as I have been overwhelmed at work. When I briefly looked at the work that is required, I realized that I am lacking context of how all components are dependent on this change. So, I think I would be better off taking a smaller piece of work.

[ahrtr]

@ahrtr - Apologies! I have not been able to look at this yet as I have been overwhelmed at work. When I briefly looked at the work that is required, I realized that I am lacking context of how all components are dependent on this change. So, I think I would be better off taking a smaller piece of work.

Thanks for the feedback. No worries.

[ahrtr]

Unfortunately, I do not get time to work on this so far, so unassigned to me for now.

Notes:

1. It's an important but not urgent task. But I expect this can be finished in 2023 before we release 3.6.0.
2. This isn't a trivial change, it needs deep dive and huge effort.
3. It would be great if anyone with strong protobuf background can help.

[pchan]

@ahrtr I can look at this problem.

Since it's a big change, so please evaluate the impact and provide a detailed plan firstly. Afterwards, we can update the functional_test in the first step.

I have started an initial analysis in this document. I will update it as I add findings.

[Azanul]

@ahrtr @pchan Is this being worked on or can I take a look into this?

[ahrtr]

@ahrtr @pchan Is this being worked on or can I take a look into this?

Thanks @Azanul . AFAIK, two other people may be also interested in this issue. @pchan @fuweid. Could you please update the progress?

[fuweid]

@ahrtr @pchan Is this being worked on or can I take a look into this?

Thanks @Azanul . AFAIK, two other people may be also interested in this issue. @pchan @fuweid. Could you please update the progress?

I'm trying to upgrade the grpc-gateway version right now. Still working on the task right now and will file pr in this week. Thanks

Updated: NOTE: The grpc-gateway change is related to pb update.

[ahrtr]

Thanks @fuweid for the update. Please feel free to let other contributors know if you need any help.

[ahrtr]

More context:

• GoGo Protobuf looking for new ownership gogo/protobuf#691
• https://groups.google.com/g/kubernetes-sig-api-machinery/c/tcwFubV9Boo
• https://www.youtube.com/watch?v=HTIltI0NuNg

[marefr]

Based on findings/discussions in #17892 the conclusion was basically that we need to migrate away from these deprecated packages to google.golang.org/protobuf to be able to bump certain dependencies and be able to make use of standards they provide. At the same time there might not be bandwidth available to work on this. To kick things of at least, I took some time looking into how etcd are using gogo/protobuf as a first start to get a better understanding what's involved making this change, what it might affect and as a way to get some discussions going.

First I did a search to see if others have documented their migration and found https://thoughts.8-p.info/en/containerd-gogo-migration which includes some interesting details and links to PRs of their work.

gogo/protobuf usage investigation:

Extensions

https://github.com/gogo/protobuf/blob/master/extensions.md

option (gogoproto.marshaler_all) = true;

Generates Marshal and MarshalTo method for each message
https://pkg.go.dev/github.com/gogo/protobuf/plugin/marshalto?utm_source=godoc

Usage of Marshal:

contrib/raftexample/raft.go:169
etcdctl/ctlv3/command/printer_protobuf.go
etcdutl/etcdutl/printer_protobuf.go
etcdutl/snapshot/v3_snapshot.go
pkg/pbutil/pbutil_test.go
pkg/pbutil/pbutil.go
server/etcdserver/bootstrap.go (pbutil)
server/etcdserver/raft_test.go (pbutil)
server/etcdserver/server_test.go (pbutil)
server/etcdserver/server.go (pbutil)
server/etcdserver/v3_server.go
server/etcdserver/api/rafthttp/http_test.go (pbutil)
server/etcdserver/api/rafthttp/http.go
server/etcdserver/api/rafthttp/msg_codec.go (pbutil)
server/etcdserver/api/rafthttp/msgappv2_codec.go (pbutil)
server/etcdserver/api/rafthttp/pipeline.go (pbutil)
server/etcdserver/api/rafthttp/util_test.go
server/etcdserver/api/snap/snapshotter.go (pbutil and Marshal)
server/lease/leasehttp/http.go
server/proxy/grpcproxy/cache/store.go
server/storage/util.go (pbutil)
server/storage/mvcc/kvstore_test.go
server/storage/mvcc/kvstore_txn.go
server/storage/mvcc/kvstore.go
server/storage/mvcc/testutil/hash.go
server/storage/schema/alarm.go
server/storage/schema/auth_roles.go
server/storage/schema/auth_users.go
server/storage/schema/lease.go
server/storage/wal/encoder.go
server/storage/wal/version_test.go (pbutil)
server/storage/wal/wal_test.go (pbutil)
server/storage/wal/wal.go (pbutil)
server/storage/wal/testing/waltesting.go (pbutil)
tools/etcd-dump-db/backend.go
tools/etcd-dump-logs/etcd-dump-log_test.go (pbutil)

Usage of MarshalTo:

server/etcdserver/api/rafthttp/msgappv2_codec.go

option (gogoproto.unmarshaler_all) = true;

The unmarshal plugin generates a Unmarshal method for each message.
https://pkg.go.dev/github.com/gogo/protobuf/plugin/unmarshal?utm_source=godoc

Usage of Unmarshal:

contrib/raftexample/raft.go
pkg/pbutil/pbutil.go
server/etcdserver/server_test.go
server/etcdserver/api/rafthttp/http.go
server/etcdserver/api/rafthttp/msg_codec.go
server/etcdserver/api/rafthttp/util_test.go
server/etcdserver/api/snap/snapshotter.go
server/lease/leasehttp/http.go
server/storage/mvcc/kvstore_txn.go
server/storage/mvcc/kvstore.go
server/storage/mvcc/watchable_store.go
server/storage/mvcc/testutil/hash.go
server/storage/schema/alarm.go
server/storage/schema/auth_roles.go
server/storage/schema/auth_users.go
server/storage/schema/lease.go
server/storage/wal/decoder.go
server/storage/wal/version.go (pbutil)
tests/robustness/report/wal.go
tools/etcd-dump-db/backend.go
tools/etcd-dump-logs/main.go

option (gogoproto.sizer_all) = true;

generates a Size or ProtoSize method for each message. This is useful with the MarshalTo method generated by the marshalto plugin and the gogoproto.marshaler and gogoproto.marshaler_all extensions
https://pkg.go.dev/github.com/gogo/protobuf/plugin/size?utm_source=godoc

Usage of Size:

/Users/marcus/go/src/github.com/marefr/etcd/server/etcdserver/bootstrap.go
/Users/marcus/go/src/github.com/marefr/etcd/server/etcdserver/server.go
/Users/marcus/go/src/github.com/marefr/etcd/server/etcdserver/api/rafthttp/http.go
/Users/marcus/go/src/github.com/marefr/etcd/server/etcdserver/api/rafthttp/msg_codec.go
/Users/marcus/go/src/github.com/marefr/etcd/server/etcdserver/api/rafthttp/msgappv2_codec.go
/Users/marcus/go/src/github.com/marefr/etcd/server/etcdserver/api/rafthttp/pipeline.go
/Users/marcus/go/src/github.com/marefr/etcd/server/etcdserver/api/rafthttp/stream.go
/Users/marcus/go/src/github.com/marefr/etcd/server/etcdserver/api/rafthttp/transport.go
/Users/marcus/go/src/github.com/marefr/etcd/server/etcdserver/api/rafthttp/util_test.go
/Users/marcus/go/src/github.com/marefr/etcd/server/etcdserver/api/snap/message.go
/Users/marcus/go/src/github.com/marefr/etcd/server/etcdserver/api/v3rpc/interceptor.go
/Users/marcus/go/src/github.com/marefr/etcd/server/etcdserver/api/v3rpc/watch.go
/Users/marcus/go/src/github.com/marefr/etcd/server/etcdserver/txn/txn.go
/Users/marcus/go/src/github.com/marefr/etcd/server/etcdserver/txn/util.go
/Users/marcus/go/src/github.com/marefr/etcd/server/storage/wal/encoder.go
/Users/marcus/go/src/github.com/marefr/etcd/server/storage/wal/wal_bench_test.go
/Users/marcus/go/src/github.com/marefr/etcd/tests/e2e/watch_delay_test.go

option (gogoproto.goproto_getters_all) = false;

if false, the message is generated without get methods, this is useful when you would rather want to use face

Should behave as standard protobuf generation based on my knowledge.

option (gogoproto.goproto_enum_prefix_all) = false;

if false, generates the enum constant names without the messagetype prefix

Should behave as standard protobuf generation based on my knowledge.

gogoproto.nullable

if false, a field is generated without a pointer (see warning below).
Disabled for fields in api/etcdserverpb/etcdserver.proto and server/storage/wal/walpb/record.proto should behave as standard protobuf generation based on my knowledge.
Enabled for Request.Refresh field in api/etcdserverpb/etcdserver.proto, but not exactly sure how it's used?

Migrating from gogo/protobuf Marshal, MarshalTo, Unmarshal, Size:

By switching to google.golang.org/protobuf you don't get automatic generation of these methods in generated code for messages (Go structs).

Seems like server/etcdserver/api/v3rpc/grpc.go uses a custom codec, server/etcdserver/api/v3rpc/codec.go, which
uses proto.Marshal and proto.Unmarshal from github.com/golang/protobuf.

The google.golang.org/protobuf/proto package provides Marshal, Unmarshal and Size functions, however
it uses proto.Reflect (reflection during runtime) which might degrade performance/throughput.

The pbutil package mentioned above seems like it's used quite a lot so should be quite straightforward change that to use proto package instead for Marshal/Unmarshal.

Any direct thoughts/insights whether it's plausible switching to google.golang.org/protobuf/proto package and use reflection during runtime for Marshal/Unmarshal and/or what affect it might have given that server/etcdserver/api/v3rpc/grpc.go doesn't seem to use it for request/response encode/decoding?

One idea I had was as a first step try migrating to use github.com/google/protobuf/proto where possible and see what breaks/performance degrades. Thoughts?

References:

https://github.com/protocolbuffers/protobuf-go/blob/master/proto/decode.go
https://github.com/protocolbuffers/protobuf-go/blob/master/proto/encode.go
https://github.com/protocolbuffers/protobuf-go/blob/master/proto/size.go