-
Notifications
You must be signed in to change notification settings - Fork 363
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
feat:support configuring xff trusted cidrs #4702
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4702 +/- ##
==========================================
+ Coverage 65.59% 65.62% +0.02%
==========================================
Files 211 211
Lines 31999 32034 +35
==========================================
+ Hits 20990 21022 +32
- Misses 9765 9768 +3
Partials 1244 1244 ☔ View full report in Codecov by Sentry. |
3ba436b
to
25f96f1
Compare
hey @rudrakhp can we add an e2e for this in this PR, or can you confirm this works by testing manually (and e2e can be added in a follow up) |
c145161
to
52831e6
Compare
@arkodg I tried to use original IP detection extension's |
1fde45a
to
5595e7d
Compare
@rudrakhp This is probably related to envoyproxy/envoy#34241 |
5154988
to
5c96805
Compare
Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>
5c96805
to
66d9135
Compare
According to HCM docs:
But due to known issues mentioned earlier, not able to move legacy num trusted hops to the new extension. Moving PR to draft till the envoy issue is resolved. |
To move this PR ahead, We probably could modify the existing RBAC e2e.
But I'm not sure which behavior is correct, the HCM xff_num_trusted_hops or the xff extension. |
Raised a PR envoyproxy/envoy#37780 to seek help from the Enovy maintainers. |
Not sure if that's the right thing to do, isn't it essentially changing behaviour for EGW users, probably breaking? |
What type of PR is this?
feat:support configuring xff trusted cidrs
What this PR does / why we need it:
Implement API introduced in #4500
Which issue(s) this PR fixes:
Refer #4489
Release Notes: No