RLP v1beta2 #230
Merged
RLP v1beta2 #230
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alexsnaps
commented
Aug 11, 2023
alexsnaps
commented
Aug 11, 2023
| "github.com/kuadrant/kuadrant-operator/pkg/common" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" |
There was a problem hiding this comment.
We don't need that gatewayapiv1alpha2 do we?
There was a problem hiding this comment.
I believe we do for TargetRef gatewayapiv1alpha2.PolicyTargetReference:
I dont believe this is available in v1beta1 unless I'm missing something 🤔
Codecov Report
@@ Coverage Diff @@
## main #230 +/- ##
==========================================
- Coverage 63.39% 63.24% -0.16%
==========================================
Files 33 33
Lines 3112 3224 +112
==========================================
+ Hits 1973 2039 +66
- Misses 962 1007 +45
- Partials 177 178 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
KevFan
force-pushed
the
rlp-v2-rebase
branch
3 times, most recently
from
ce34ba9
to
654344c
Compare
Closed
--------- Co-authored-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
* reconciliation of route selectors for RLPs targeting HTTPRoutes * Change `common.Contains(slice []string, target string)` to any comparable type (using generics) * Define a `common.HTTPRouteRuleSelector` type that checks whether a `HTTPRouteMatch` selects a `HTTPRouteRule` or not * Define a `rlptools.HTTPRouteRulesFromRouteSelector` function that returns all rules from a HTTPRoute that a route selector selects (using `common.HTTPRouteRuleSelector`) * Modify `rlptools.conditionsFromLimit` to use `rlptools.HTTPRouteRulesFromRouteSelector` and generate wasm conditions for each combination of selected route rule match and hostname * Ensure all generated route conditions include a pattern for the hostnames (route selectors’, HTTPRoute’s or Gateway’s) * Ensure all generated route conditions include the user-defined `when` conditions * add tests for generating a wasmplugin config out of a complex httproute and complex ratelimitpolicy The httproute declares 2 rules and 2 hostnames. The RLP declares 2 limits: - one limit declares one counter, targets one of the httprouterules, for one hostname only, with an additional 'when' condition and counter qualifier; - the other limit declares 2 counters, targets the other httprouterule, for all hostnames, and states no additional 'when' condition nor counter qualifier. * generate wasm rules only for routeSelector-filtered hostnames that belong to the targeted route * fix: pattern expression for hostnames with the right operator - introduces 'endswith' pattern operator * fix: compute wasm rules only for the applicable hostnames Passes a list of hostnames into the call to the function that generates the wasm rules for a given RLP, notoriously to compute the conditions when to apply each limit. The list of hostnames is the domain intersection between the hostnames specified in the HTTPRoute and its parent gateway for which the wasm config is being built. The route selection considers this as a boundary, so it doesn’t generate rules for the wrong gateway/hostname. Fixes the computation of conditions when no HTTPRouteRule matches the route selectors. * do not generate wasm condition patterns for the hostname when all apply * do not repeat RLPs for each hostname within the wasm config * `rlptools.HTTPRouteRulesFromRouteSelector` moved to `api/v1beta2/route_selectors.go` along with the `RouteSelector` type itself + no longer injecting a separate list of hostnames for avoiding building wasm rules for hostnames that do not apply, as this did not seem to belong to the route selector logic anyway. * reconciliation of route selectors for RLPs targeting Gateways Get all HTTPRoutes that are children of the Gateway and call the generation of the set of wasm rules (conditions and data for each limit) for each HTTPRoute * match the gateway hostnames instead of the httproute ones when building the wasm rules for rlps that target a gateway * refactor: make HTTPRouteRuleSelector.Selects return sooner if the HTTP method does not match * refactor: ensure wasm rule conditions are generated out of selected HTTPRouteRules in the same order as the selectors select the rules * Prevent the usage of routeSelectors in RLPs that target a Gateway * do not generate conditions for gateway policy wasm rules from httproutes that have a rlp of its own and avoid building wasmplugins when there are no rules to apply * avoid adding rlps without any rules to the wasm config
Fixes the reconciliation of the Limitador CR, pairing it with the reconciled wasm config (WasmPlugin CR), so - rate limit definitions won't be duplicated in the Limitador CR; - limits can be defined crossing gateways and hostnames and yet be treated as the same limit in Limitador (case of simple RLPs that target HTTPRoutes with multiple Gateway parent refs)
… in the format: limit.<limit-name>__<hash>, where <limit-name> is sanitised to include only characters allowed by Limitador for the identifiers and <hash> is generated out of the original limit name to avoid breaking uniqueness of the name after sanitisation.
For some reason after rebase, there is some dependency issue. This was fixed by updating the istio dep to https://github.com/istio/istio/releases/tag/1.17.5
guicassolato
approved these changes
Aug 17, 2023
There was a problem hiding this comment.
I've left a minor comment although I don't think it's critical.
In general, LGTM. Verification steps also working fine.
Thanks for putting this together!
|
To others interested in revieweing this PR: git diff remotes/origin/rlp-v2-update-main..remotes/KevFan/rlp-v2-rebase |
Co-authored-by: Guilherme Cassolato <guicassolato@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I did a merge myself, and came up with some differences, will leave comments inline
Closes #156