Skip to content

ratelimit: use policy namespaced name as descriptor value#6801

Closed
zirain wants to merge 8 commits intoenvoyproxy:mainfrom
zirain:ratelimit-rule
Closed

ratelimit: use policy namespaced name as descriptor value#6801
zirain wants to merge 8 commits intoenvoyproxy:mainfrom
zirain:ratelimit-rule

Conversation

@zirain
Copy link
Copy Markdown
Member

@zirain zirain commented Aug 16, 2025

make the descriptor value( and the metric generated by ratelimit service) more valuable

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 18, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.09%. Comparing base (c5d6b03) to head (d5c1ce0).
⚠️ Report is 299 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6801      +/-   ##
==========================================
+ Coverage   71.06%   71.09%   +0.02%     
==========================================
  Files         229      229              
  Lines       40942    40945       +3     
==========================================
+ Hits        29097    29109      +12     
+ Misses      10127    10119       -8     
+ Partials     1718     1717       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@arkodg arkodg added this to the v1.6.0-rc.1 Release milestone Sep 11, 2025
@zirain zirain force-pushed the ratelimit-rule branch 3 times, most recently from 5e17861 to ba40ba7 Compare September 26, 2025 04:40
@zirain zirain changed the title [WIP] optimize ratelimit rule optimize ratelimit rule Sep 30, 2025
@zirain zirain changed the title optimize ratelimit rule ratelimit: use policy namespaced name as descriptor value Sep 30, 2025
@zirain zirain marked this pull request as ready for review September 30, 2025 16:17
@zirain zirain requested a review from a team as a code owner September 30, 2025 16:17
rudrakhp
rudrakhp previously approved these changes Oct 5, 2025
Signed-off-by: zirain <zirain2009@gmail.com>
Signed-off-by: zirain <zirain2009@gmail.com>
Signed-off-by: zirain <zirain2009@gmail.com>
Signed-off-by: zirain <zirain2009@gmail.com>
Signed-off-by: zirain <zirain2009@gmail.com>
Signed-off-by: zirain <zirain2009@gmail.com>
Signed-off-by: zirain <zirain2009@gmail.com>
Signed-off-by: zirain <zirain2009@gmail.com>
@github-actions
Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale label Nov 23, 2025
@github-actions github-actions bot removed the stale label Nov 23, 2025
@zirain
Copy link
Copy Markdown
Member Author

zirain commented Dec 7, 2025

wait envoyproxy/envoy#41339

@zirain
Copy link
Copy Markdown
Member Author

zirain commented Dec 26, 2025

with envoyproxy/ratelimit#1027, you can dump the descriptors to accesslog as following:

{
  ":authority": "172.18.255.206",
  "authorization": null,
  "bytes_received": 0,
  "bytes_sent": 0,
  "downstream_local_address": "10.244.2.18:10080",
  "downstream_remote_address": "172.18.0.1:57968",
  "duration": 1,
  "method": "GET",
  "protocol": "HTTP/1.1",
  "ratelimit": {
    "descriptors": [
      {
        "entries": [
          "httproute/default/http-ratelimit2/rule/0/match/0/*=httproute/default/http-ratelimit2/rule/0/match/0/*",
          "rule-0-match-0=rule-0-match-0"
        ]
      }
    ],
    "domain": "default/eg/http"
  },
  "request_headers_bytes": 223,
  "requested_server_name": null,
  "response_code": 429,
  "response_code_details": "request_rate_limited",
  "response_flags": "RL",
  "route_name": "httproute/default/http-ratelimit2/rule/0/match/0/*",
  "start_time": "2025-12-26T06:29:13.565Z",
  "upstream_cluster": "httproute/default/http-ratelimit2/rule/0",
  "upstream_duration": null,
  "upstream_host": null,
  "upstream_local_address": null,
  "upstream_transport_failure_reason": null,
  "user-agent": "curl/8.7.1",
  "x-envoy-origin-path": "/get",
  "x-envoy-upstream-service-time": null,
  "x-forwarded-for": "172.18.0.1",
  "x-request-id": "9d2e4b50-b7fd-476b-ae3e-ceeac22858fe"
}

but didn't known which policy was applied to the route.

@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Dec 26, 2025

thanks @zirain, looks like policy is needed in the descriptor, but we cannot replace route suffix, because the same policy can be applied to different routes

@zirain
Copy link
Copy Markdown
Member Author

zirain commented Dec 27, 2025

thanks @zirain, looks like policy is needed in the descriptor, but we cannot replace route suffix, because the same policy can be applied to different routes

Sorry, I didn't get your point, what's changed in this PR is the value of the first descriptor(it's same as the key before).

@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Dec 27, 2025

similar to #6801 (comment), can you share the after with this PR change

@zirain
Copy link
Copy Markdown
Member Author

zirain commented Dec 27, 2025

you can find the diff from the testdata, here is an example.

@zirain
Copy link
Copy Markdown
Member Author

zirain commented Dec 27, 2025

BTW, I think #7824 might be another option.
This patch introuduce a breaking change which may cause migration problem in a prod environment.

@zirain zirain closed this Jan 14, 2026
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