-
Notifications
You must be signed in to change notification settings - Fork 33
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
Support HTTPQueryParamMatch as CEL routeRuleConditions #981
Conversation
Signed-off-by: Alex Snaps <[email protected]>
// from https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/advanced/attributes | ||
// request.query -> string : The query portion of the URL in the format of “name1=value1&name2=value2”. | ||
// query param, only consider the first in case of repetition, as per spec | ||
queryParams := make(map[gatewayapiv1.HTTPHeaderName]bool) |
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.
Dunno if that's idiomatic Go...
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.
its fine, I think you would find most use queryParams := map[gatewaypiv1.HTTPHeaderName]bool{}
but I don't think it is that important
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.
code looks ok, but will leave it to someone more familar with the impact of this to approve
Signed-off-by: Alex Snaps <[email protected]>
Signed-off-by: Alex Snaps <[email protected]>
Signed-off-by: Alex Snaps <[email protected]>
8a14ebe
to
61661d2
Compare
So I don't know how this makes math... but there is no way this introduces a regression in test coverage as it tests my additions as well as code that wasn't test before... Go home codecov, You're drunk! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #981 +/- ##
==========================================
- Coverage 76.15% 76.10% -0.05%
==========================================
Files 111 111
Lines 8986 8998 +12
==========================================
+ Hits 6843 6848 +5
- Misses 1852 1853 +1
- Partials 291 297 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
🏅
Depends on Kuadrant/wasm-shim#126 |
* Support HTTPQueryParamMatch as CEL routeRuleConditions Signed-off-by: Alex Snaps <[email protected]> * By default decodeQueryString will now ignore repeated params Signed-off-by: Alex Snaps <[email protected]> * By default queryMap will now ignore repeated params Signed-off-by: Alex Snaps <[email protected]> * Test for mapping `HTTPRouteMatch` to CEL Predicates Signed-off-by: Alex Snaps <[email protected]> --------- Signed-off-by: Alex Snaps <[email protected]>
No description provided.