argoproj / argo-workflows

Workflow Engine for Kubernetes
https://argo-workflows.readthedocs.io/
Apache License 2.0
14.82k stars 3.17k forks source link

Replace `gogoprotobuf` related dependencies. #11053

Open isubasinghe opened 1 year ago

isubasinghe commented 1 year ago

Summary

We need to replace gogoprotobuf dependencies with some other tool. The reason we need to do this is because this package is deprecated. See this

List of alarming bugs: Invalid Type OOM Invalid Type (Possibly related to previous) Memory Leak Index out of range Bad Performance for growslice?

~This is going to be a herculean effort to migrate off, unfortunately.~

isubasinghe commented 1 year ago

https://www.youtube.com/watch?v=HTIltI0NuNg

maxsxu commented 1 year ago

Thanks for sharing this news. Wondering any alternatives currently?

isubasinghe commented 1 year ago

@maxsxu Unfortunately I am not sure, I don't think any straight forward alternatives exist though.

ryancurrah commented 1 year ago

We at CrowdStrike wrote https://github.com/CrowdStrike/csproto to migrate off of gogoprotobuf. The migration guide is here that covers almost step by step what you need to do https://github.com/CrowdStrike/csproto/blob/main/docs/migration_guide.md.

The library solves 2 problems:

  1. Transitioning off of Gogo to not Gogo: Csproto has a wrapper around all 3 popular golang proto libraries (gogo/proto, Google v1, Google v2) and have a wrapper that can unmarshal/marshal any of the 3.
  2. A custom decoder and encoder: Csproto has a custom encoder/decoder that allows for up to 40% faster decoding/encoding then google or gogo libraries.
isubasinghe commented 1 year ago

Thanks a lot @ryancurrah, glad to know we have an option here at least.

ryancurrah commented 1 year ago

Even if you don't use csproto the migration guide covers a lot of steps we needed to do to successfully migrate away.

JPZ13 commented 1 year ago

@isubasinghe - given how unwieldy the tooling is around protobufs generally and how there are similar projects that don't use protobufs, do you think it would be a good idea or feasible to remove the protobufs entirely?

ryancurrah commented 1 year ago

Not to detract from @JPZ13 comment but the protobuf tooling is getting better.

A lot of organizations are adopting https://buf.build which is making life easier for using protobufs.

isubasinghe commented 1 year ago

Hmm I still do think @JPZ13 makes a valid point, even if the tooling is better, the question of "do we need protobuf's in the first place" is a fair question imo. I think we can get away with something much simpler, adding a few ms of latency per request will not impact Workflows in any noticeable way.

isubasinghe commented 1 year ago

@JPZ13 with regards to feasibility, I think there would be quite a lot of refactoring to be done, more work than to migrate into something like csproto. I think it's something that would at least take a couple of months to completely refactor. That being said, dropping all these external deps and sticking only to golang stdlib would be a nice win.

JPZ13 commented 1 year ago

In that case, maybe we migrate to csproto in the short term and set a goal to remove protobufs entirely over the next year

agilgur5 commented 2 months ago

A lot of organizations are adopting https://buf.build which is making life easier for using protobufs.

Yea Buf's definitely made the ecosystem a lot more usable. The schema registry, an easier to use compiler, and protovalidate are all great tools off the top of my head. protovalidate's use of CEL also makes cross-language validation a lot easier (though WASM might be even easier, albeit with more overhead).

And protovalidate could be particularly useful for Argo to add in-schema validation (esp. as we have various bugs due to inconsistent validation in all the different Argo components, partly due to a lack of validating admission webhook -- see also https://github.com/argoproj/argo-workflows/issues/6781#issuecomment-1984910847 #13503 et al).

Protobuf tools are still a little hard to discover though (no central place to search for them); like I recently stumbled upon the xcontroller example repo that pulls from istio/tools to do more codegen of CRDs and Go types (I think some of our existing/legacy tooling partially overlaps, but not entirely, as I believe we don't use protobufs at all for the Controller, see below).

Also there are competing formats as "protobuf successors" too, like Cap'n Proto (which is awesome in many ways, such as nearly implementing dependent types in C++ via template metaprogramming) and Flatbuffers, the former from the creator of OSS Protobuf (after leaving Google) and the latter also from Google 🙃

That being said, dropping all these external deps and sticking only to golang stdlib would be a nice win.

I would definitely prefer less tooling over more (better) tooling. Though we do have an existing need for at least some codegen based off schemas already:

  1. The k8s CRD, which generates OpenAPI v2-based CRDs from Go types using kube-openapi
  2. The Server's API spec, which currently generates an OpenAPI v2 schema from protobufs using protoc-gen-swagger (from gRPC Gateway v1; see also https://github.com/argoproj/argo-workflows/issues/12565#issuecomment-2026688627 and #12851)

The SDKs are then all generated off the OpenAPI schema. While the Go SDK could re-use Go types and Go validation logic, I don't believe it does currently (all generated). The CLI already does this, however. The other language SDKs cannot do this, and the UI currently duplicates logic for that (with its own TS typings as well (mostly in models/ but also a bit in services/)).

Also gRPC requires protobufs, but we've long considered removing gRPC support entirely as not worth the maintenance overhead (it pre-dates Alex C even. also Alex C thought gRPC and protobufs were too much of a maintenance hassle too).

But I would consider the SDKs and gRPC more "nice-to-have" than needed, since both are lacking in various ways and ripe for replacement (or have better tools already, e.g. Hera for Python). But protovalidate and Buf tooling would help make the SDKs more usable.

Technically we can do the completely necessary bits without protobufs and that may be well worthwhile. We'd just have to tackle SDK validation separately

I think there would be quite a lot of refactoring to be done, more work than to migrate into something like csproto. I think it's something that would at least take a couple of months to completely refactor.

If it's only used in the API (I believe it is), this actually might be simpler. We just rip out all the protobuf and gRPC, and keep the generated Go types (and eventually improve on those manually) and the Server OpenAPI schema (and would need to move that to a new generator before the next API change -- that one's a little harder, but maybe not too much if we can combine the right tooling properly)

Also removing things is just so much more fun 😄 and less complex