ads: support envoy filter local ratelimit. #859

yuanqijing · 2024-09-15T10:09:04Z

What type of PR is this?
support local rate limit.

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

codecov · 2024-09-15T10:49:50Z

Codecov Report

Attention: Patch coverage is 88.57143% with 4 lines in your changes missing coverage. Please review.

Project coverage is 52.17%. Comparing base (1f2940b) to head (6876096).
Report is 130 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/controller/ads/cache.go	0.00%	4 Missing ⚠️

Files with missing lines	Coverage Δ
pkg/controller/ads/extensions/local_ratelimit.go	`100.00% <100.00%> (ø)`
pkg/utils/test/bpf_map.go	`34.04% <100.00%> (-3.11%)`	⬇️
pkg/controller/ads/cache.go	`47.94% <0.00%> (+0.39%)`	⬆️

... and 69 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5b8028...6876096. Read the comment docs.

hzxuzhonghu · 2024-09-18T02:20:37Z

api/filter/ratelimit.proto

+  }
+*/
+message LocalRateLimit {
+  reserved 1 to 2;


why reserve?

reserve for
{
"stat_prefix": ...,
"status": {...},
}
should i start token_bucket with number 1, or keep the number tag ?

You can start from 1

hzxuzhonghu · 2024-09-18T02:21:26Z

api/filter/ratelimit.proto

+      "enable_x_ratelimit_headers": ...,
+      "vh_rate_limits": ...,
+      "always_consume_default_token_bucket": {...},
+      "rate_limited_as_resource_exhausted": ...


if other fields not supported, please remove them. Add once we support it

just remove the comments? i copy the definition from envoy api.

hzxuzhonghu

First we need a proposal to better understand this implement

hzxuzhonghu · 2024-09-18T02:24:47Z

bpf/kmesh/ads/include/local_ratelimit.h

+};
+
+struct ratelimit_value {
+    __u64 last_topup;


can you add a comment about what is this

yuanqijing · 2024-09-19T13:34:15Z

First we need a proposal to better understand this implement

hi i have created a proposal pr in #873

yuanqijing · 2024-09-19T14:52:35Z

@hzxuzhonghu hi, ratelimit use the same way to cut tcp connection as circuit breaker, it can be merged after #570.

hzxuzhonghu · 2024-09-20T04:33:04Z

bpf/kmesh/ads/include/local_ratelimit.h

+    __type(value, struct ratelimit_value);
+    __uint(max_entries, 1000);
+    __uint(map_flags, BPF_F_NO_PREALLOC);
+} ratelimit_map SEC(".maps");


please keep consistent with other map name.

bpf/kmesh/ads/include/local_ratelimit.h

hzxuzhonghu · 2024-10-28T06:33:39Z

In kmesh, we are a little different, we can only ratelimit on the client side now. So take the example here, though it is http.

https://istio.io/latest/docs/tasks/policy-enforcement/rate-limit/#local-rate-limit

apiVersion: networking.istio.io/v1alpha3
kind: EnvoyFilter
metadata:
  name: filter-local-ratelimit-svc
  namespace: istio-system
spec:
  workloadSelector:
    labels:
      app: productpage
  configPatches:
    - applyTo: NETWORK_FILTER
      match:
        context: SIDECAR_OUTBOUND

It should be applied on each client instance, not on all the instances of a node.

yuanqijing · 2024-10-28T06:43:27Z

In kmesh, we are a little different, we can only ratelimit on the client side now. So take the example here, though it is http.

https://istio.io/latest/docs/tasks/policy-enforcement/rate-limit/#local-rate-limit
apiVersion: networking.istio.io/v1alpha3
kind: EnvoyFilter
metadata:
  name: filter-local-ratelimit-svc
  namespace: istio-system
spec:
  workloadSelector:
    labels:
      app: productpage
  configPatches:
    - applyTo: NETWORK_FILTER
      match:
        context: SIDECAR_OUTBOUND
It should be applied on each client instance, not on all the instances of a node.

Understood, so the key point here is whether intances in a node share a token bucket.
option 1: client_A->Service_C: 5 token, client_B->Service_C: 6 token
option 2: Service_C: 11 token.

Envoy using option 2, but in kmesh we use option1 ? Is my understanding correct?

hzxuzhonghu · 2024-10-28T07:58:04Z

Envoy using option 2, but in kmesh we use option1 ? Is my understanding correct?

Envoy can use both, kmesh we can only use option 1

yuanqijing · 2024-10-28T08:59:48Z

Envoy using option 2, but in kmesh we use option1 ? Is my understanding correct?

Envoy can use both, kmesh we can only use option 1

I have added a new field to distinguish different pods:

struct ratelimit_key {
    union {
        struct {
            __u64 netns;  /* Network namespace. */
            __u32 ipv4;   /* Destination IPv4 address. */
            __u32 port;   /* Destination port. */
            __u32 family; /* Address family (e.g., AF_INET) */
        } sk_skb;
    } key;
};

yuanqijing · 2024-10-28T09:05:10Z

@hzxuzhonghu Hi I think there are 2 pending Issues:

When cleaning up the listener, also clean up the ratelimit configuration. Can I create another PR to fix this?
The concurrent issue here may not have been fully discussed yet.

hzxuzhonghu · 2024-10-28T09:26:19Z

When cleaning up the listener, also clean up the ratelimit configuration. Can I create another PR to fix this?

Yes, i think its ok

The concurrent issue here may not have been fully discussed yet.

@nlgwcy suggested using atomic update

yuanqijing · 2024-10-28T10:57:16Z

When cleaning up the listener, also clean up the ratelimit configuration. Can I create another PR to fix this?

Yes, i think its ok

The concurrent issue here may not have been fully discussed yet.

@nlgwcy suggested using atomic update

suggested atomic updates all addressed. #859 (comment) #859 (comment)

hzxuzhonghu · 2024-10-30T10:02:25Z

bpf/kmesh/ads/include/local_ratelimit.h

+    if (topup > 0) {
+        value->tokens += topup * settings->tokens_per_fill;
+        if (value->tokens > settings->max_tokens) {
+            value->tokens = settings->max_tokens;


This is not concurrent safe

can this ensue concurent safe ? cause only one progress can add the value.

topup = (now - value->last_topup) / settings->fill_interval; if (topup > 0) { topup_time = value->last_topup + topup * settings->fill_interval; if (__sync_bool_compare_and_swap(&value->last_topup, value->last_topup, topup_time)) { delta = topup * settings->tokens_per_fill; if (value->tokens + delta > settings->max_tokens) { delta = settings->max_tokens - value->tokens; } __sync_fetch_and_add(&value->tokens, delta); } }

@nlgwcy is an expert on this

@nlgwcy hi, could this approach be used to avoid concurrent issues?

hzxuzhonghu · 2024-10-30T10:02:45Z

bpf/kmesh/ads/include/local_ratelimit.h

+#include "listener/listener.pb-c.h"
+
+struct ratelimit_key {
+    union {


why use union here

Using a union in the ratelimit_key structure improves scalability. If we need to limit HTTP connections in the future, we can reuse the same key structure. For example, we can add a structure for HTTP connections like this:

struct ratelimit_key { union { struct { __u64 netns; /* Network namespace. */ __u32 ipv4; /* Destination IPv4 address. */ __u32 port; /* Destination port. */ __u32 family; /* Address family (e.g., AF_INET) */ } sk_skb; struct { __u32 http_method; /* HTTP method (e.g., GET, POST) */ __u32 http_version; /* HTTP version (e.g., 1.1, 2) */ __u32 http_port; /* HTTP port */ } http; // New HTTP connection related fields } key; };

nlgwcy · 2024-10-31T11:23:58Z

/lgtm

nlgwcy · 2024-10-31T11:28:36Z

bpf/kmesh/ads/include/local_ratelimit.h

+    if (rate_limit__check_and_take(&key, &settings)) {
+        BPF_LOG(INFO, FILTERCHAIN, "rate limit exceeded\n");
+// TODO: remove this after #570 merged.
+#ifndef MARK_REJECTED


It's not needed anymore.

kmesh-bot · 2024-10-31T12:12:22Z

New changes are detected. LGTM label has been removed.

lec-bit · 2024-10-31T12:28:47Z

/retest

nlgwcy · 2024-10-31T12:56:45Z

/approve

kmesh-bot · 2024-10-31T12:56:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nlgwcy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [nlgwcy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: yuan <[email protected]>

kmesh-bot added the do-not-merge/work-in-progress label Sep 15, 2024

kmesh-bot requested review from hzxuzhonghu and kwb0523 September 15, 2024 10:09

kmesh-bot added the size/L label Sep 15, 2024

yuanqijing force-pushed the ads/ratelimit branch 3 times, most recently from 5964c91 to 1c8d1bb Compare September 15, 2024 10:27

kmesh-bot added size/XL and removed size/L labels Sep 15, 2024

yuanqijing force-pushed the ads/ratelimit branch 2 times, most recently from 80747b0 to 17aa8b8 Compare September 15, 2024 10:34

kmesh-bot added size/XXL and removed size/XL labels Sep 15, 2024

yuanqijing force-pushed the ads/ratelimit branch from 17aa8b8 to e064980 Compare September 15, 2024 10:37

yuanqijing force-pushed the ads/ratelimit branch from b7ffc97 to d12d047 Compare September 15, 2024 10:54

yuanqijing changed the title ~~[WIP] ads: support envoy filter local ratelimit.~~ ads: support envoy filter local ratelimit. Sep 15, 2024

kmesh-bot removed the do-not-merge/work-in-progress label Sep 15, 2024

hzxuzhonghu reviewed Sep 18, 2024

View reviewed changes

yuanqijing force-pushed the ads/ratelimit branch 2 times, most recently from 917ece0 to 007baba Compare September 19, 2024 14:49

yuanqijing force-pushed the ads/ratelimit branch 2 times, most recently from cdf2d67 to 8b988d1 Compare September 19, 2024 15:16

hzxuzhonghu reviewed Sep 20, 2024

View reviewed changes

bpf/kmesh/ads/include/local_ratelimit.h Show resolved Hide resolved

yuanqijing force-pushed the ads/ratelimit branch from 8b988d1 to 6d4e280 Compare September 20, 2024 04:40

yuanqijing force-pushed the ads/ratelimit branch 3 times, most recently from 362207e to 6876096 Compare October 28, 2024 08:51

hzxuzhonghu reviewed Oct 30, 2024

View reviewed changes

kmesh-bot assigned nlgwcy Oct 31, 2024

kmesh-bot added the lgtm label Oct 31, 2024

nlgwcy reviewed Oct 31, 2024

View reviewed changes

yuanqijing force-pushed the ads/ratelimit branch from 6876096 to 3f0c4e9 Compare October 31, 2024 12:12

kmesh-bot removed the lgtm label Oct 31, 2024

yuanqijing force-pushed the ads/ratelimit branch 2 times, most recently from 0c4da26 to 6bc5c4e Compare October 31, 2024 12:50

kmesh-bot added the approved label Oct 31, 2024

yuanqijing force-pushed the ads/ratelimit branch from 6bc5c4e to 102ce34 Compare October 31, 2024 13:06

ads: support envoy filter local ratelimit.

694e917

Signed-off-by: yuan <[email protected]>

yuanqijing force-pushed the ads/ratelimit branch from 553f30b to 694e917 Compare October 31, 2024 14:07

nlgwcy merged commit 55d4e65 into kmesh-net:main Oct 31, 2024
6 of 8 checks passed

tacslon mentioned this pull request Nov 1, 2024

Revert "ads: support envoy filter local ratelimit." #1008

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ads: support envoy filter local ratelimit. #859

ads: support envoy filter local ratelimit. #859

yuanqijing commented Sep 15, 2024

codecov bot commented Sep 15, 2024 •

edited

Loading

hzxuzhonghu Sep 18, 2024

yuanqijing Sep 19, 2024

hzxuzhonghu Sep 20, 2024

yuanqijing Sep 20, 2024

hzxuzhonghu Sep 18, 2024

yuanqijing Sep 19, 2024

hzxuzhonghu left a comment

hzxuzhonghu Sep 18, 2024

yuanqijing Sep 19, 2024

yuanqijing commented Sep 19, 2024

yuanqijing commented Sep 19, 2024

hzxuzhonghu Sep 20, 2024

yuanqijing Sep 20, 2024

hzxuzhonghu commented Oct 28, 2024

yuanqijing commented Oct 28, 2024 •

edited

Loading

hzxuzhonghu commented Oct 28, 2024

yuanqijing commented Oct 28, 2024

yuanqijing commented Oct 28, 2024

hzxuzhonghu commented Oct 28, 2024

yuanqijing commented Oct 28, 2024

hzxuzhonghu Oct 30, 2024

yuanqijing Oct 30, 2024

hzxuzhonghu Oct 31, 2024

yuanqijing Oct 31, 2024

hzxuzhonghu Oct 30, 2024

yuanqijing Oct 30, 2024

nlgwcy commented Oct 31, 2024

nlgwcy Oct 31, 2024

kmesh-bot commented Oct 31, 2024

lec-bit commented Oct 31, 2024

nlgwcy commented Oct 31, 2024

kmesh-bot commented Oct 31, 2024

ads: support envoy filter local ratelimit. #859

ads: support envoy filter local ratelimit. #859

Conversation

yuanqijing commented Sep 15, 2024

codecov bot commented Sep 15, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzxuzhonghu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuanqijing commented Sep 19, 2024

yuanqijing commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzxuzhonghu commented Oct 28, 2024

yuanqijing commented Oct 28, 2024 • edited Loading

hzxuzhonghu commented Oct 28, 2024

yuanqijing commented Oct 28, 2024

yuanqijing commented Oct 28, 2024

hzxuzhonghu commented Oct 28, 2024

yuanqijing commented Oct 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlgwcy commented Oct 31, 2024

Choose a reason for hiding this comment

kmesh-bot commented Oct 31, 2024

lec-bit commented Oct 31, 2024

nlgwcy commented Oct 31, 2024

kmesh-bot commented Oct 31, 2024

codecov bot commented Sep 15, 2024 •

edited

Loading

yuanqijing commented Oct 28, 2024 •

edited

Loading