Skip to content

Add gRPC proto def for rate-limit xDS Config communication#371

Closed
renuka-fernando wants to merge 4 commits intoenvoyproxy:mainfrom
renuka-fernando:xds-proto2
Closed

Add gRPC proto def for rate-limit xDS Config communication#371
renuka-fernando wants to merge 4 commits intoenvoyproxy:mainfrom
renuka-fernando:xds-proto2

Conversation

@renuka-fernando
Copy link
Copy Markdown
Contributor

@renuka-fernando renuka-fernando commented Oct 20, 2022

Description

Proto for xDS communication as in the discussion #201 (comment).
xDS implementation: #373

@mattklein123 mattklein123 self-assigned this Oct 20, 2022
@renuka-fernando renuka-fernando force-pushed the xds-proto2 branch 2 times, most recently from 4793ce9 to 2153c05 Compare October 20, 2022 15:16
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this. Can you add more comments so it's easier to understand how this is going to be used in the context of ADS? Thank you.

Comment thread api/ratelimit/config/ratelimit/v3/rls_conf.proto Outdated
Comment thread api/ratelimit/config/ratelimit/v3/rls_conf.proto Outdated
@renuka-fernando
Copy link
Copy Markdown
Contributor Author

I had a discussion with @alecholmez about adding this along with ADS. We can use ADS for this and there is no ADS client in go-control-plane hence, I will add an xDS client in this repo.

For writing the xDS server we can use go-control-plane and use Snapshots to update the rate limits. This PR envoyproxy/go-control-plane#598 will register the rate limit config as a known type in go-control-plane.

@mattklein123
Copy link
Copy Markdown
Member

I had a discussion with @alecholmez about adding this along with ADS. We can use ADS for this and there is no ADS client in go-control-plane hence, I will add an xDS client in this repo.

Please don't add the ADS client in this repo. I think it belongs in GCP so it can be used in other places.

@renuka-fernando
Copy link
Copy Markdown
Contributor Author

I had a discussion with @alecholmez about adding this along with ADS. We can use ADS for this and there is no ADS client in go-control-plane hence, I will add an xDS client in this repo.

Please don't add the ADS client in this repo. I think it belongs in GCP so it can be used in other places.

Used the ADS streaming client https://github.com/envoyproxy/go-control-plane/blob/129d0216a4efb985b22349fb45c1444d4f0e7900/envoy/service/discovery/v3/ads.pb.go#L228
Please find the xDS implementation in #373

Comment thread api/ratelimit/config/ratelimit/v3/rls_conf.proto
@renuka-fernando
Copy link
Copy Markdown
Contributor Author

Could you please retrigger the build? This is an intermitten issue, which will be fixed with #370

Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
…ratelimit service

Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine but one thing I'm wondering is can we share the protos from xds/envoy for some of the core structures?

@kkcmadhu
Copy link
Copy Markdown

this app prodvides an http endpoint /json to get the current rate limit as jason.
A HTTP endpoint to add /modify the current config will be great too..

irrespective of GRPC or HTTP, do we think we will need some kind of role support to control who can modify this config?

Even though ADS is used to configure rate limits, the relevant discvory service should be there according to envoyproxy/go-control-plane#598 (comment)

Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
@mattklein123 mattklein123 removed their assignment Jan 20, 2023
@renuka-fernando
Copy link
Copy Markdown
Contributor Author

Hi @mattklein123,

ADS Client implementation is merged envoyproxy/go-control-plane#604 and updated the go-control-plane version to reflect the client implementation. Could you please review this PR?

Thanks.

renuka-fernando added a commit to renuka-fernando/ratelimit that referenced this pull request Feb 7, 2023
Related to envoyproxy#371

Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
@renuka-fernando
Copy link
Copy Markdown
Contributor Author

Closing this PR. The proto file is included in #373 as a single commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants