-
Notifications
You must be signed in to change notification settings - Fork 347
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
api: Add disableMergeSlash and escapedSlashesAction to ClientTrafficPolicy #2384
Conversation
…e expected behavior for path URIs with escaped slashes. Signed-off-by: Lior Okman <lior.okman@sap.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2384 +/- ##
==========================================
+ Coverage 64.65% 64.67% +0.01%
==========================================
Files 114 115 +1
Lines 16618 16844 +226
==========================================
+ Hits 10745 10894 +149
- Misses 5194 5255 +61
- Partials 679 695 +16 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Lior Okman <lior.okman@sap.com>
-1 on this setting, since its against the recommendations in https://www.envoyproxy.io/docs/envoy/latest/configuration/best_practices/edge#configuring-envoy-as-an-edge-proxy |
The With regards to the
In order to do so, it needs to be exposed in In our use-case, we have some upstream servers that are not RFC 3986 compliant and we need to keep the encoded slashes as they are. I think the current defaults are good, and I would definitely keep the current settings as defaults. But I think it does make sense to make these flags available for cases where the defaults don't work. |
A few other OSS ingress controllers also allow controlling the
|
thanks for added context @liorokman, some thoughts
|
These parameters are already grouped under a top level
Changing this to a
I think there's also a hidden issue here that when the route is a gRPC route the better value for this configuration key would probably be |
api/v1alpha1/pathsettings_types.go
Outdated
// proxies, Envoy and upstream server. | ||
// The “httpN.downstream_rq_redirected_with_normalized_path” counter is incremented | ||
// for each redirected request. | ||
UnescapeRedirect PathEscapedSlashAction = "UnescapeRedirect" |
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.
prefer UnescapeAndRedirect
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.
Renamed
api/v1alpha1/pathsettings_types.go
Outdated
// UnescapeForward unescapes %2F and %5C sequences and forwards the request. | ||
// Note: this option should not be enabled if intermediaries perform path based access | ||
// control as it may lead to path confusion vulnerabilities. | ||
UnescapeForward PathEscapedSlashAction = "UnescapeForward" |
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.
prefer UnescapeAndForward
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.
Renamed
thanks for the detailed comments @liorokman reg default setting for |
* Renamed UnescapeForward to UnescapeAndForward Signed-off-by: Lior Okman <lior.okman@sap.com>
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 thanks for adding the extra details in the PR !
What this PR does / why we need it:
This PR adds settings into
ClientTrafficPolicy
to control slashes in the incoming request path URI.The following settings are added:
The proposed defaults are the current EnvoyGateway behavior - so not setting these parameters will not cause any change to the current behavior.
These settings map to the EnvoyProxy HTTP Connection Manager configuration
path_with_escaped_slashes_action
andmerge_slashes
keys.Which issue(s) this PR fixes:
These parameters were proposed in issue #1846 .