KEP-5589: Initial draft for gogo protobuf removal

[liggitt]

• One-line PR description: Initial draft for gogo protobuf removal

• Issue link: Remove gogo protobuf dependency for Kubernetes API types #5589

/sig api-machinery architecture
/cc @deads2k @jpbetz @dims

[serathius]

/cc @ahrtr
Might be relevant for etcd

[liggitt]

/cc @ahrtr

I'm not sure the approach taken here is particularly relevant to etcd and etcd-io/etcd#14533 ... etcd does treat the .proto file as the canonical API definition, and does fully generate go types from .proto without the wild Go-rewriting steps Kubernetes does.

[aojea]

LGTM modulo one comment

We have to get rid of this sword of Damocles hanging over the project

[pohly]

LGTM

[jpbetz]

Spoke with @liggitt earlier about this approach. It's pragmatic. I'm onboard.

/approve

/hold
For approval from @deads2k

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpbetz, liggitt, saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• keps/prod-readiness/OWNERS [jpbetz]
• keps/sig-api-machinery/OWNERS [jpbetz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ahrtr]

/cc @ahrtr

I'm not sure the approach taken here is particularly relevant to etcd and etcd-io/etcd#14533 ... etcd does treat the .proto file as the canonical API definition, and does fully generate go types from .proto without the wild Go-rewriting steps Kubernetes does.

etcd has some minor rewrite (converting v1 messages to v2), refer to https://github.com/etcd-io/etcd/blob/682d4fb5e281bb4f987e9350d5896d3bebdfadf4/scripts/genproto.sh#L177-L210

Could you point me to the rewrite & related doc Kubernetes does?

[ahrtr]

This makes perfect sense!

But it seems that introducing new interfaces might be unnecessarily complicate this KEP; and it might be better to consider it as a separate KEP.

[liggitt]

there's no new interfaces, we're referencing interfaces defined within gogo packages currently ... we're redefining those interfaces locally instead

[liggitt]

etcd has some minor rewrite (converting v1 messages to v2), refer to https://github.com/etcd-io/etcd/blob/682d4fb5e281bb4f987e9350d5896d3bebdfadf4/scripts/genproto.sh#L177-L210

Could you point me to the rewrite & related doc Kubernetes does?

Ours is here, after running the gogo generation step:
https://github.com/kubernetes/code-generator/blob/v0.34.0/cmd/go-to-protobuf/protobuf/cmd.go#L289-L293

The rewrite is defined here: https://github.com/kubernetes/code-generator/blob/master/cmd/go-to-protobuf/protobuf/parser.go#L80

[liggitt]

etcd has some minor rewrite (converting v1 messages to v2), refer to https://github.com/etcd-io/etcd/blob/682d4fb5e281bb4f987e9350d5896d3bebdfadf4/scripts/genproto.sh#L177-L210

that's unfortunate, I'm pretty sure gogo messages aren't fully compliant v2 messages

[ahrtr]

etcd has some minor rewrite (converting v1 messages to v2), refer to https://github.com/etcd-io/etcd/blob/682d4fb5e281bb4f987e9350d5896d3bebdfadf4/scripts/genproto.sh#L177-L210

that's unfortunate, I'm pretty sure gogo messages aren't fully compliant v2 messages

The rewrite is just to support REST API generated by grpc-gateway (so the core etcd's grpc API isn't impacted), and we have enough test cases to ensure nothing is broken.

Sorry to derail the discussion of the KEP.