-
-
Notifications
You must be signed in to change notification settings - Fork 147
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 using Istio Envoy #674
Add Rate Limit using Istio Envoy #674
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #674 +/- ##
==========================================
- Coverage 48.96% 48.81% -0.15%
==========================================
Files 69 69
Lines 10090 10127 +37
==========================================
+ Hits 4941 4944 +3
- Misses 4631 4657 +26
- Partials 518 526 +8 ☔ View full report in Codecov by Sentry. |
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.
Thank you for your contribution.
I think it will be good to have the opportunity to demonstrate and explain this feature (on weekly sync or personal meeting).
Leaving two major suggestions below:
- Since we have many manifests in single directory
templates
, how about seperating manifests for each service by directory like below?
- templates
- yorkie
- deployment.yaml
- istio
- ingress.yaml
- ratelimit.yaml
- How about adding
enable
/disable
value invalues.yaml
to enable and disable rate limiting base on their needs?
ratelimit:
enabled: false
build/charts/yorkie-cluster/templates/ratelimit-envoy-filter.yaml
Outdated
Show resolved
Hide resolved
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.
Thank you for applying my feedbacks.
I left few more comments below.
How about setting requestsPerUnit
to 10000
for now?
Considering total request metric in #590 and total request metric of this month(2023.11.01 ~ 2023.11.23
), I think setting 10000
requests per minute will do.
(total request metric of #590, approximately 10000+
per minute)
(highest total request metric between 2023.11.01
and 2023.11.23
, approximately 8000-
per minute)
build/charts/yorkie-cluster/templates/istio/ratelimit/config.yaml
Outdated
Show resolved
Hide resolved
build/charts/yorkie-cluster/templates/istio/ratelimit/config.yaml
Outdated
Show resolved
Hide resolved
build/charts/yorkie-cluster/templates/istio/ratelimit/deployment.yaml
Outdated
Show resolved
Hide resolved
build/charts/yorkie-cluster/templates/istio/ratelimit/redis/deployment.yaml
Outdated
Show resolved
Hide resolved
build/charts/yorkie-cluster/templates/istio/ratelimit/redis/deployment.yaml
Outdated
Show resolved
Hide resolved
build/charts/yorkie-cluster/templates/istio/ratelimit/redis/service.yaml
Outdated
Show resolved
Hide resolved
build/charts/yorkie-cluster/templates/istio/ratelimit/redis/service.yaml
Outdated
Show resolved
Hide resolved
build/charts/yorkie-cluster/templates/istio/ratelimit/service.yaml
Outdated
Show resolved
Hide resolved
@joonhyukchoi After you finish your revision, please re-request review for further progress. |
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.
LGTM! It works perfectly!
FYI, I could not find references that global rate limit is done per source ip (seems like it is default setting in global rate limit). For local rate limit, we can use remote_address
key to rate limit per source ip. So we need to check if global rate limit works per source ip after we apply this changes in our cluster.
What this PR does / why we need it:
To limit a large number of requests, such as in a DDoS attack, we can use Istio(envoy)'s rate limit in K8s ingress.
Which issue(s) this PR fixes:
Fixes #590
Special notes for your reviewer:
I have tested on local K8s environment with yorkie-cluster Helm chart with minikube, and it worked as expected.
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist: