Automate generating Rate Limit Config gRPC pb files from source proto

[renuka-fernando]

Description

It is better to have an automated way to generate pb files for Rate Limit Config as discussed here #598 (comment).

The source files of the config reside in https://github.com/envoyproxy/ratelimit/blob/main/api/ratelimit/config/ratelimit/v3/rls_conf.proto.
Generated pb files https://github.com/envoyproxy/go-control-plane/tree/main/ratelimit

Related Issue

#595

[arkodg]

@renuka-fernando something similar to https://github.com/envoyproxy/envoy/blob/b03b18122f439b3bb53801c681aac663907fdcf0/.azure-pipelines/pipelines.yml#L276 might be required in envoyproxy/ratelimit

[renuka-fernando]

Thanks @arkodg.
CC @phlax, @lizan, @mattklein123

[phlax]

something similar to ...

apologies i missed this before - just flagging that the linked CI job is a bit of an anti pattern - it causes frequent races in CI where multiple jobs are trying to update at the same time

ive thought about it quite a bit but havent thought of a better way to do this so far

[renuka-fernando]

What is if we move the rls_conf.proto to envoyproxy/envoy repo?

[phlax]

sounds sensible to me but it needs api-sheps to look - perhaps raise a Pr and see what they say

[phlax]

ive thought about it quite a bit but havent thought of a better way to do this so far

just clarifying that the problem (iirc) more occurs with filter example as it is pushed more freqently i think - but the pattern is the same

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[github-actions]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.

[Ronnie-personal]

@phlax Is there anything I can help with? What are the options to resolve the issue?

[jpeach]

@phlax Is there anything I can help with? What are the options to resolve the issue?

My 2c is that generated protobuf bindings for rate limit should like in the ratelimit repository so they can be tagged and release-managed along with the ratelimit server itself. If you do that, then there's no need for complicated CI workflows.

[Ronnie-personal]

@phlax Is there anything I can help with? What are the options to resolve the issue?

My 2c is that generated protobuf bindings for rate limit should like in the ratelimit repository so they can be tagged and release-managed along with the ratelimit server itself. If you do that, then there's no need for complicated CI workflows.

Thanks for sharing your ideas. Currently, does rate limit repo have pipeline in place to generate any protobuf bindings?

[phlax]

im not familiar with the ratelimit repo - but checking it seems that it has some build/test ci (docker/docker_tests) https://github.com/envoyproxy/ratelimit/blob/main/Makefile

[renuka-fernando]

im not familiar with the ratelimit repo - but checking it seems that it has some build/test ci (docker/docker_tests) https://github.com/envoyproxy/ratelimit/blob/main/Makefile

AFAIK, those are for GitHub actions used for PR check validations.
https://github.com/envoyproxy/ratelimit/blob/278a7c2a9a18a2fe101ff02090c00bdc4904181f/.github/workflows/pullrequest.yaml#L22-L23

It is really great if it is possible to automate generating pb files for Rate Limit Config proto files.

cc @alecholmez

[arkodg]

4 decisions needed here

1. where does the source live (inout repo) - for now, lives in envoyproxy/ratelimit
2. where does the protobuf tooling live (buf, protoc-gen-go, protoc-gen-validate)
3. where does the generated pb files live (output repo) - for now, lives in envoyproxy/go-control-plane
4. how (& who) publishes & commits the generated pb files into the output repo

[dio]

@phlax I have a sketch here: https://github.com/envoyproxy/ratelimit/compare/main...dio:ratelimit:generated/sync?expand=1#diff-9378a8b24aa534a459299b517298bbae5c3865058af860aba203ff6e069d5123

The process is roughly as follows:

1. When triggered, for example: when we have post-commit (push to main) changes to the api/ratelimit/**/*.proto, or go-control-plane has a newer mirrored commit, or dispatched manually, periodically, etc.
2. Then, the script (./script/sync, which can be executed using make sync), checks out the envoyproxy/go-control-plane.
3. Having a local copy of envoyproxy/go-control-plane, we check for the last mirrored attempt, and get the corresponding envoyproxy/envoy SHA (e.g. from git log: 107d405 Mirrored from envoyproxy/envoy @ c745c1956cdaffa35d6f4933b57a431a2d2e9fc7), we know that it is synced with c745c1956cdaffa35d6f4933b57a431a2d2e9fc7 of envoyproxy/envoy repo.
4. Then, we scrape for rules_go, to find the right version of protoc-gen-go. ⚠️ rules_go uses the legacy version of protoc-gen-go (golang/protobuf), but compiles it with a newer google.golang.org/protobuf. Reference: https://github.com/bazelbuild/rules_go/blob/30943d1bd319323bee7663c8f3f57b33d401ec6b/go/private/repositories.bzl#L157-L162, proof:

   go-control-plane/envoy/admin/v3/certs.pb.go

   Line 3 in d264e51

   // protoc-gen-go v1.30.0

   , and the relevant patch: https://github.com/bazelbuild/rules_go/blob/30943d1bd319323bee7663c8f3f57b33d401ec6b/third_party/com_github_golang_protobuf-gazelle.patch. So we need to get the tarball of golang/protobuf (the version/tag is from rules_go), then do go get -u google.golang.org/protobuf@<version_from_rules_go>.
5. Then, by having the right version of protoc-gen-go we do buf generate --path ratelimit within the ratelimit's api directory. The output (*.pb.go), then can be saved inside the ratelimit repo and also copied to the local copy of go-control-plane repo (see: https://github.com/dio/ratelimit/blob/e22f37c8ace17c68c135375b60fe1b2bfc94b11a/api/buf.gen.yaml#L3-L8), and later a GHA job can pick it up and craft a PR to the go-control-plane (upstream) repo (hence testable), this requires PAT of envoy-bot (a PR sample to my fork: https://github.com/dio/go-control-plane/pull/3/files).

Notes:

1. The current (manually(?)) generated *.pb.go for ratelimit is using a different version of protoc-gen-go (

   go-control-plane/ratelimit/config/ratelimit/v3/rls_conf.pb.go

   Line 3 in d264e51

   // protoc-gen-go v1.25.0-devel

   ), while mostly it is fine, but probably we need to fix it.
2. The generated *.pb.go via buf will render // protoc (unknown) (example: https://github.com/dio/go-control-plane/blob/04732e11727420bac630a183b01ca947033cf1d6/ratelimit/config/ratelimit/v3/rls_conf.pb.go#L4) which I think is acceptable.

[jpeach]

I don't understand why we want a complicated CI mechanism to generate protobufs here. We could just generate and commit them to the ratelimit repository when that proto file changes. Then, no-one ever needs a new go-control-plane release just to pick up protobuf definitions, and the versions are always in sync by definition.

[jpeach]

Re-opening due to the discussion.

[dio]

True that. I agree that we can "just" generate it inside the ratelimit repo (check-in the generated *.pb.go) and don't need to sync it here (use it as a module instead).

[renuka-fernando]

Some points, AFAIR, the reasons to have Ratelimit pb files in the go-control-plane.

• We need to add StreamRlsConfigs https://github.com/envoyproxy/go-control-plane/blob/main/pkg/server/v3/server.go#L225 and GetResourceName as in https://github.com/envoyproxy/go-control-plane/blob/main/pkg/cache/v3/resource.go#L112 to enable for implementation of a Go xDS server for Ratelimit configurations (Communication between Ratelimit xDS server and Ratelimit service). Hence, rate limit pb files are required in the go-control-plane or they should be imported from Ratelimit.

  • Here is an example xDS server for Ratelimit configuration: https://github.com/envoyproxy/ratelimit/tree/main/examples/xds-sotw-config-server

• Ratelimit service should import go-control-plane pb files required for the gRPC communication between Envoy and Ratelimit service (the request made from Envoy to check whether the request is rate limited) https://github.com/envoyproxy/ratelimit/blob/965f0bc861da0d4478071aeca0a5f2c2202257b7/src/config/config_impl.go#L7.

To remove the cyclic dependencies we have to put the Ratelimit pb files in go-control-plane. So ratelimit is not a dependency of go-control-plane but go-control-plane is a dependency of ratelimit.

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[github-actions]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.