-
Notifications
You must be signed in to change notification settings - Fork 521
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
Add Rate-Limit xDS config server related control plane functionality #598
Conversation
envoyproxy#595 Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
@alecholmez could you please review this? |
Hi @alecholmez, What do you think is the best? Using ADS or having a separate RLS config service? |
Is there an xDS client for ADS that can be reused in the rate limit? So I do not want to implement an xDS client in the rate limit repo, just import from go-control-plane and use it. |
Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
c08a5a9
to
bcfa6c3
Compare
Sorry for the slow response times! I'll begin to look into this. I would actually like us to have RLS over ADS. That's the most common use case and if people begin to request that we supported a separated RLS service we can address that down the line. |
@@ -0,0 +1,551 @@ | |||
// Code generated by protoc-gen-go. DO NOT EDIT. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions on this generated pb file...
Where does this come from? How can we automate that it keeps up with changes from its source? If this is from the envoy data-plane mirror or the udpa repo, why is it receiving it's own folder instead of living under the envoy/
generated code folder.
Can you give me some background here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I planned to keep the source proto file in the rate limit service repository envoyproxy/ratelimit#371 since it contains the rate limit config message.
The pb file is generated on my local machine (definitely we have to automate it 🙂). The location to keep this pb file is just what I decided, we can move it to a proper location. Could you please help me on automating this pb file generation?
It is better if we can keep the source proto file in the rate limit repository, but I think it is not an issue to keep that in another repository.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay no worries let's definitely keep the proto file close to the implementation.
I think it's worth moving it. I'm about to travel for the day so I will think about the automation and where we want to keep the generated code.
please include this for java control plane too. |
…telimit-xds Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com> Conflicts: pkg/cache/v3/resource.go
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
Hi @alecholmez, Could you please run the failed build again and please review the PR? The issue with the build is fixed now: #625 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merging this since it's ready to go
@alecholmez, thank you very much for the support in getting this done. |
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>
Related issue
Fix #595
Proto files
PR: envoyproxy/ratelimit#368
Description
To configure envoy rate limit service via xDS