search

cockroachdb / errors

Go error library with error portability over the network
Apache License 2.0
2.07k stars 66 forks source link

protobuf: recompile using gogoproto 1.2 from CRDB master #64

Closed erikgrinaker closed 3 years ago

erikgrinaker commented 3 years ago

Generated Protobufs have been a mix of gogoproto 1.2 and 1.3 types, since different packages have been compiled with different Protobuf compilers. This was in part because Makefile.update-protos only covered errorspb/*.proto, with other Protobufs compiled ad hoc. This was problematic since CockroachDB currently uses gogoproto 1.2 and thus could not make use of the 1.3-generated types.

This patch changes Makefile.update-protos to compile all Protobufs in the repo, and recompiles all Protobufs using the current Protobuf compiler used in CockroachDB.

Note in particular that this downgrades generated Protobufs for extgrpc, exthttp, and grpc from gogoproto 1.3 to 1.2, which might be considered a breaking change.

This also addresses CVE-2021-3121.

Related to #63 and https://github.com/cockroachdb/cockroach/issues/56208.

This change is Reviewable

CLAassistant commented 3 years ago

CLA assistant check
All committers have signed the CLA.

erikgrinaker commented 3 years ago

why did so much change in go.sum?

It came along with gogoproto 1.3.2, see https://github.com/gogo/protobuf/commit/550e88954e617545f49920b752c154d72abf1d8d

knz commented 3 years ago

I don't get why this is needed. If the errors library was generating its code using gogo 1.3, it should be able to pick the bug fix for the skippy thing directly in the 1.3 compiler, so why the need to pull in the custom patched 1.2 compiler?

erikgrinaker commented 3 years ago

The initial motivation for this was to compile gRPC error Protobufs that would be compatible with CockroachDB, using version 2 structs (proto.GoGoProtoPackageIsVersion2). These were initially contributed in #14 by an external contributor who couldn't get the correct Protobuf compiler to work, and thus used GoGoProtoPackageIsVersion3.

Instructions in https://github.com/cockroachdb/errors/blob/master/Makefile.update-protos#L9-L24 say that we need to generate all Protobufs with CockroachDB's Protobuf compiler, so that's what I did. Most Protobufs seemed to be generated with an old gogo 1.2 compiler, but the gRPC stuff was compiled with 1.3, so I recompiled everything using the 1.2 compiler currently used by CRDB to clean things up.

tooolbox commented 3 years ago

Hm, tricky situation. I apologize for having not gone the extra mile to keep it v2 in the first place.

Seems like the takeaway is to pin the tooling to specific versions, ideally in a way that doesn't require checking out and building CRDB itself. I've now done this successfully for several different projects; will probably get together a PR to show how this could be done.

erikgrinaker commented 3 years ago

Hm, tricky situation. I apologize for having not gone the extra mile to keep it v2 in the first place.

No worries, we should've given you a hand with this!

Seems like the takeaway is to pin the tooling to specific versions, ideally in a way that doesn't require checking out and building CRDB itself. I've now done this successfully for several different projects; will probably get together a PR to show how this could be done.

Yeah, that would be great! Note that we use our own patched version of the 1.2 compiler, to backport security fixes and such. We're considering options for the whole Protobuf situation, now that gogo is no longer maintained, but there are a few complications.