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 gogoprotobuf related dependencies. #11053

Open
isubasinghe opened this issue May 8, 2023 · 12 comments
Open

Replace gogoprotobuf related dependencies. #11053

isubasinghe opened this issue May 8, 2023 · 12 comments
Labels
area/api Argo Server API go Pull requests that update Go dependencies type/dependencies PRs and issues specific to updating dependencies type/feature Feature request

Comments

@isubasinghe
Copy link
Member

isubasinghe commented May 8, 2023

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 isubasinghe added the type/feature Feature request label May 8, 2023
@isubasinghe
Copy link
Member Author

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

@maxsxu
Copy link
Contributor

maxsxu commented May 9, 2023

Thanks for sharing this news. Wondering any alternatives currently?

@isubasinghe
Copy link
Member Author

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

@ryancurrah
Copy link
Contributor

ryancurrah commented May 18, 2023

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
Copy link
Member Author

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

@ryancurrah
Copy link
Contributor

ryancurrah commented May 18, 2023

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

@JPZ13
Copy link
Member

JPZ13 commented May 23, 2023

@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
Copy link
Contributor

ryancurrah commented May 24, 2023

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
Copy link
Member Author

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
Copy link
Member Author

isubasinghe commented May 25, 2023

@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
Copy link
Member

JPZ13 commented May 26, 2023

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 agilgur5 added type/dependencies PRs and issues specific to updating dependencies go Pull requests that update Go dependencies labels Sep 1, 2023
@agilgur5
Copy link

agilgur5 commented Jun 24, 2024

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 #6781 (comment) #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 Update outdated k8s go packages to 0.26+ #12565 (comment) and Update from OpenAPI v2 to OpenAPI v3 #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

@agilgur5 agilgur5 added the area/api Argo Server API label Aug 12, 2024
@agilgur5 agilgur5 changed the title Replace gogoprotobuf related dependencies. Replace gogoprotobuf related dependencies. Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Argo Server API go Pull requests that update Go dependencies type/dependencies PRs and issues specific to updating dependencies type/feature Feature request
Projects
None yet
Development

No branches or pull requests

5 participants