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

Add Rate-Limit xDS config server related control plane functionality #598

Merged
merged 4 commits into from
Jan 27, 2023

Conversation

renuka-fernando
Copy link
Contributor

Related issue

Fix #595

Proto files

PR: envoyproxy/ratelimit#368

Description

To configure envoy rate limit service via xDS

envoyproxy#595

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

@alecholmez could you please review this?

@renuka-fernando
Copy link
Contributor Author

Hi @alecholmez,

What do you think is the best? Using ADS or having a separate RLS config service?
I removed the RLS config service and commit d0a39a0 to this PR

@renuka-fernando
Copy link
Contributor Author

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

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

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@kkcmadhu
Copy link

please include this for java control plane too.

…telimit-xds

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

Conflicts:
	pkg/cache/v3/resource.go
@github-actions
Copy link

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

Hi @alecholmez,

Could you please run the failed build again and please review the PR? The issue with the build is fixed now: #625

pkg/server/v3/server.go Show resolved Hide resolved
Copy link
Contributor

@alecholmez alecholmez left a 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 alecholmez merged commit 29a4b73 into envoyproxy:main Jan 27, 2023
@renuka-fernando
Copy link
Contributor Author

@alecholmez, thank you very much for the support in getting this done.

renuka-fernando added a commit to renuka-fernando/ratelimit that referenced this pull request Jan 27, 2023
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include Rate-Limit xDS config server related control plane functionality
3 participants