From fec0420a8f39d0dea4674d61ea2822e9033de710 Mon Sep 17 00:00:00 2001 From: Guilherme Cassolato Date: Thu, 22 Jun 2023 19:40:31 +0200 Subject: [PATCH 01/15] reconciliation of route selectors for RLPs targeting HTTPRoutes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- .../ratelimitpolicy_controller_test.go | 17 +- controllers/ratelimitpolicy_wasm_plugins.go | 47 +- pkg/common/common.go | 34 +- pkg/common/common_test.go | 224 +++++++++ pkg/common/gatewayapi_utils.go | 111 +++++ pkg/common/gatewayapi_utils_test.go | 437 ++++++++++++++++++ pkg/rlptools/route_selectors.go | 27 ++ pkg/rlptools/route_selectors_test.go | 212 +++++++++ pkg/rlptools/wasm_utils.go | 141 +++--- pkg/rlptools/wasm_utils_test.go | 24 +- 10 files changed, 1180 insertions(+), 94 deletions(-) create mode 100644 pkg/rlptools/route_selectors.go create mode 100644 pkg/rlptools/route_selectors_test.go diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index 3933e154d..066ca6456 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -296,6 +296,11 @@ var _ = Describe("RateLimitPolicy controller", func() { Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), Value: "GET", }, + { + Selector: "request.host", + Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), + Value: "*.example.com", + }, }, }, }, @@ -411,7 +416,17 @@ var _ = Describe("RateLimitPolicy controller", func() { Hostnames: []string{"*"}, Rules: []wasm.Rule{ { - Conditions: nil, + Conditions: []wasm.Condition{ + { + AllOf: []wasm.PatternExpression{ + { + Selector: "request.host", + Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), + Value: "*", + }, + }, + }, + }, Data: []wasm.DataItem{ { Static: &wasm.StaticSpec{ diff --git a/controllers/ratelimitpolicy_wasm_plugins.go b/controllers/ratelimitpolicy_wasm_plugins.go index ef393cc19..33b897f1b 100644 --- a/controllers/ratelimitpolicy_wasm_plugins.go +++ b/controllers/ratelimitpolicy_wasm_plugins.go @@ -120,8 +120,7 @@ func (r *RateLimitPolicyReconciler) gatewayWASMPlugin(ctx context.Context, gw co } // returns nil when there is no rate limit policy to apply -func (r *RateLimitPolicyReconciler) wasmPluginConfig(ctx context.Context, - gw common.GatewayWrapper, rlpRefs []client.ObjectKey) (*wasm.WASMPlugin, error) { +func (r *RateLimitPolicyReconciler) wasmPluginConfig(ctx context.Context, gw common.GatewayWrapper, rlpRefs []client.ObjectKey) (*wasm.WASMPlugin, error) { logger, _ := logr.FromContext(ctx) routeRLPList := make([]*kuadrantv1beta2.RateLimitPolicy, 0) @@ -146,15 +145,23 @@ func (r *RateLimitPolicyReconciler) wasmPluginConfig(ctx context.Context, } wasmRulesByDomain := make(rlptools.WasmRulesByDomain) + var gwWasmRules []wasm.Rule if gwRLP != nil { - if len(gw.Hostnames()) == 0 { - // wildcard domain - wasmRulesByDomain["*"] = append(wasmRulesByDomain["*"], rlptools.WasmRules(gwRLP, nil)...) - } else { - for _, gwHostname := range gw.Hostnames() { - wasmRulesByDomain[gwHostname] = append(wasmRulesByDomain[gwHostname], rlptools.WasmRules(gwRLP, nil)...) - } + hostnames := gw.Hostnames() + if len(hostnames) == 0 { + hostnames = []string{"*"} + } + // FIXME(guicassolato): this is a hack until we start going through all the httproutes that are children of the gateway and build the rules for each httproute + route := &gatewayapiv1beta1.HTTPRoute{ + Spec: gatewayapiv1beta1.HTTPRouteSpec{ + Hostnames: common.Map(hostnames, func(h string) gatewayapiv1beta1.Hostname { return gatewayapiv1beta1.Hostname(h) }), + Rules: []gatewayapiv1beta1.HTTPRouteRule{{}}, + }, + } + gwWasmRules = rlptools.WasmRules(gwRLP, route) // FIXME(guicassolato): this is not correct. We need to go through all the httproutes that are children of the gateway and build the rules for each httproute instead + for _, gwHostname := range hostnames { + wasmRulesByDomain[gwHostname] = append(wasmRulesByDomain[gwHostname], gwWasmRules...) } } @@ -163,12 +170,11 @@ func (r *RateLimitPolicyReconciler) wasmPluginConfig(ctx context.Context, if err != nil { return nil, err } - // gateways limits merged with the route level limits - mergedGatewayActions := mergeRules(httpRouteRLP, gwRLP, httpRoute) + wasmRules := append(rlptools.WasmRules(httpRouteRLP, httpRoute), gwWasmRules...) // FIXME(guicassolato): there will be no need to merge gwRLP rules when targeting a gateway == shortcut for targeting all the routes of a gateway // routeLimits referenced by multiple hostnames for _, hostname := range httpRoute.Spec.Hostnames { - wasmRulesByDomain[string(hostname)] = append(wasmRulesByDomain[string(hostname)], mergedGatewayActions...) + wasmRulesByDomain[string(hostname)] = append(wasmRulesByDomain[string(hostname)], wasmRules...) } } @@ -178,10 +184,11 @@ func (r *RateLimitPolicyReconciler) wasmPluginConfig(ctx context.Context, } // One RateLimitPolicy per domain + // FIXME(guicassolato): Why do we map per domain? Is it so the wasm-shim can index the config per domain and improve perfomance in the data plane? If so, this will occasionally generate incongruent entries of domain keys combined with rules that have no relation with that domain. for domain, rules := range wasmRulesByDomain { rateLimitPolicy := wasm.RateLimitPolicy{ - Name: domain, - Domain: common.MarshallNamespace(gw.Key(), domain), + Name: domain, // FIXME(guicassolato): can't we use the name of the policy instead? + Domain: common.MarshallNamespace(gw.Key(), domain), // FIXME(guicassolato) https://github.com/Kuadrant/kuadrant-operator/issues/201 Service: common.KuadrantRateLimitClusterName, Hostnames: []string{domain}, Rules: rules, @@ -191,15 +198,3 @@ func (r *RateLimitPolicyReconciler) wasmPluginConfig(ctx context.Context, return wasmPlugin, nil } - -// merge operations currently implemented with list append operation -func mergeRules(routeRLP *kuadrantv1beta2.RateLimitPolicy, gwRLP *kuadrantv1beta2.RateLimitPolicy, route *gatewayapiv1beta1.HTTPRoute) []wasm.Rule { - routeRules := rlptools.WasmRules(routeRLP, route) - - if gwRLP == nil { - return routeRules - } - - // add gateway level actions - return append(routeRules, rlptools.WasmRules(gwRLP, nil)...) -} diff --git a/pkg/common/common.go b/pkg/common/common.go index 7c0e24bd7..8039ef203 100644 --- a/pkg/common/common.go +++ b/pkg/common/common.go @@ -87,7 +87,7 @@ func NamespacedNameToObjectKey(namespacedName, defaultNamespace string) client.O // Contains checks if the given target string is present in the slice of strings 'slice'. // It returns true if the target string is found in the slice, false otherwise. -func Contains(slice []string, target string) bool { +func Contains[T comparable](slice []T, target T) bool { for idx := range slice { if slice[idx] == target { return true @@ -96,6 +96,28 @@ func Contains(slice []string, target string) bool { return false } +// SameElements checks if the two slices contain the exact same elements. Order does not matter. +func SameElements[T comparable](s1, s2 []T) bool { + if len(s1) != len(s2) { + return false + } + for _, v := range s1 { + if !Contains(s2, v) { + return false + } + } + return true +} + +func Intersect[T comparable](slice1, slice2 []T) bool { + for _, item := range slice1 { + if Contains(slice2, item) { + return true + } + } + return false +} + func Find[T any](slice []T, match func(T) bool) (*T, bool) { for _, item := range slice { if match(item) { @@ -135,6 +157,16 @@ func ReverseSlice[T any](input []T) []T { return output } +func MapValues[T comparable, U any](m map[T]U) []U { + values := make([]U, len(m)) + i := 0 + for k := range m { + values[i] = m[k] + i++ + } + return values +} + // MergeMapStringString Merge desired into existing. // Not Thread-Safe. Does it matter? func MergeMapStringString(existing *map[string]string, desired map[string]string) bool { diff --git a/pkg/common/common_test.go b/pkg/common/common_test.go index cb093479b..ebea4a5d2 100644 --- a/pkg/common/common_test.go +++ b/pkg/common/common_test.go @@ -250,6 +250,212 @@ func TestContains(t *testing.T) { } } +func TestContainsWithInts(t *testing.T) { + testCases := []struct { + name string + slice []int + target int + expected bool + }{ + { + name: "when slice has one target item then return true", + slice: []int{1}, + target: 1, + expected: true, + }, + { + name: "when slice is empty then return false", + slice: []int{}, + target: 2, + expected: false, + }, + { + name: "when target is in a slice then return true", + slice: []int{1, 2, 3}, + target: 2, + expected: true, + }, + { + name: "when no target in a slice then return false", + slice: []int{1, 2, 3}, + target: 4, + expected: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if Contains(tc.slice, tc.target) != tc.expected { + t.Errorf("when slice=%v and target=%d, expected=%v, but got=%v", tc.slice, tc.target, tc.expected, !tc.expected) + } + }) + } +} + +func TestSameElements(t *testing.T) { + testCases := []struct { + name string + slice1 []string + slice2 []string + expected bool + }{ + { + name: "when slice1 and slice2 contain the same elements then return true", + slice1: []string{"test-gw1", "test-gw2", "test-gw3"}, + slice2: []string{"test-gw1", "test-gw2", "test-gw3"}, + expected: true, + }, + { + name: "when slice1 and slice2 contain unique elements then return false", + slice1: []string{"test-gw1", "test-gw2"}, + slice2: []string{"test-gw1", "test-gw3"}, + expected: false, + }, + { + name: "when both slices are empty then return true", + slice1: []string{}, + slice2: []string{}, + expected: true, + }, + { + name: "when both slices are nil then return true", + expected: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if SameElements(tc.slice1, tc.slice2) != tc.expected { + t.Errorf("when slice1=%v and slice2=%v, expected=%v, but got=%v", tc.slice1, tc.slice2, tc.expected, !tc.expected) + } + }) + } +} + +func TestIntersect(t *testing.T) { + testCases := []struct { + name string + slice1 []string + slice2 []string + expected bool + }{ + { + name: "when slice1 and slice2 have one common item then return true", + slice1: []string{"test-gw1", "test-gw2"}, + slice2: []string{"test-gw1", "test-gw3", "test-gw4"}, + expected: true, + }, + { + name: "when slice1 and slice2 have no common item then return false", + slice1: []string{"test-gw1", "test-gw2"}, + slice2: []string{"test-gw3", "test-gw4"}, + expected: false, + }, + { + name: "when slice1 is empty then return false", + slice1: []string{}, + slice2: []string{"test-gw3", "test-gw4"}, + expected: false, + }, + { + name: "when slice2 is empty then return false", + slice1: []string{"test-gw1", "test-gw2"}, + slice2: []string{}, + expected: false, + }, + { + name: "when both slices are empty then return false", + slice1: []string{}, + slice2: []string{}, + expected: false, + }, + { + name: "when slice1 is nil then return false", + slice2: []string{"test-gw3", "test-gw4"}, + expected: false, + }, + { + name: "when slice2 is nil then return false", + slice1: []string{"test-gw1", "test-gw2"}, + expected: false, + }, + { + name: "when both slices are nil then return false", + expected: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if Intersect(tc.slice1, tc.slice2) != tc.expected { + t.Errorf("when slice1=%v and slice2=%v, expected=%v, but got=%v", tc.slice1, tc.slice2, tc.expected, !tc.expected) + } + }) + } +} + +func TestIntersectWithInts(t *testing.T) { + testCases := []struct { + name string + slice1 []int + slice2 []int + expected bool + }{ + { + name: "when slice1 and slice2 have one common item then return true", + slice1: []int{1, 2}, + slice2: []int{1, 3, 4}, + expected: true, + }, + { + name: "when slice1 and slice2 have no common item then return false", + slice1: []int{1, 2}, + slice2: []int{3, 4}, + expected: false, + }, + { + name: "when slice1 is empty then return false", + slice1: []int{}, + slice2: []int{3, 4}, + expected: false, + }, + { + name: "when slice2 is empty then return false", + slice1: []int{1, 2}, + slice2: []int{}, + expected: false, + }, + { + name: "when both slices are empty then return false", + slice1: []int{}, + slice2: []int{}, + expected: false, + }, + { + name: "when slice1 is nil then return false", + slice2: []int{3, 4}, + expected: false, + }, + { + name: "when slice2 is nil then return false", + slice1: []int{1, 2}, + expected: false, + }, + { + name: "when both slices are nil then return false", + expected: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if Intersect(tc.slice1, tc.slice2) != tc.expected { + t.Errorf("when slice1=%v and slice2=%v, expected=%v, but got=%v", tc.slice1, tc.slice2, tc.expected, !tc.expected) + } + }) + } +} + func TestMap(t *testing.T) { slice1 := []int{1, 2, 3, 4} f1 := func(x int) int { return x + 1 } @@ -357,6 +563,24 @@ func TestReverseSlice(t *testing.T) { }) } +func TestMapValues(t *testing.T) { + t.Run("when given a map of strings to string then return a slice of the string values", func(t *testing.T) { + stos := map[string]string{"a": "foo", "b": "bar", "c": "baz"} + expected := []string{"foo", "bar", "baz"} + if r := MapValues(stos); len(r) != len(expected) || !SameElements(r, expected) { + t.Errorf("expected: %v; got: %v", expected, r) + } + }) + + t.Run("when given a map of strings to ints then return a slice of the int values", func(t *testing.T) { + stos := map[string]int{"a": 1, "b": 2, "c": 3} + expected := []int{1, 2, 3} + if r := MapValues(stos); len(r) != len(expected) || !SameElements(r, expected) { + t.Errorf("expected: %v; got: %v", expected, r) + } + }) +} + func TestMergeMapStringString(t *testing.T) { testCases := []struct { name string diff --git a/pkg/common/gatewayapi_utils.go b/pkg/common/gatewayapi_utils.go index a527f64d9..45f7b113d 100644 --- a/pkg/common/gatewayapi_utils.go +++ b/pkg/common/gatewayapi_utils.go @@ -91,6 +91,117 @@ func RulesFromHTTPRoute(route *gatewayapiv1beta1.HTTPRoute) []HTTPRouteRule { return rules } +type HTTPRouteRuleSelector struct { + *gatewayapiv1beta1.HTTPRouteMatch +} + +func (s *HTTPRouteRuleSelector) Selects(rule gatewayapiv1beta1.HTTPRouteRule) bool { + if s.HTTPRouteMatch == nil { + return true + } + + _, found := Find(rule.Matches, func(ruleMatch gatewayapiv1beta1.HTTPRouteMatch) bool { + // path + if s.Path != nil && !reflect.DeepEqual(s.Path, ruleMatch.Path) { + return false + } + + // headers + for _, header := range s.Headers { + if _, found := Find(ruleMatch.Headers, func(otherHeader gatewayapiv1beta1.HTTPHeaderMatch) bool { + return reflect.DeepEqual(header, otherHeader) + }); !found { + return false + } + } + + // query params + for _, param := range s.QueryParams { + if _, found := Find(ruleMatch.QueryParams, func(otherParam gatewayapiv1beta1.HTTPQueryParamMatch) bool { + return reflect.DeepEqual(param, otherParam) + }); !found { + return false + } + } + + // method + if s.Method != nil && !reflect.DeepEqual(s.Method, ruleMatch.Method) { + return false + } + + return true + }) + + return found +} + +// HTTPRouteRuleToString prints the matches of a HTTPRouteRule as string +func HTTPRouteRuleToString(rule gatewayapiv1beta1.HTTPRouteRule) string { + matches := Map(rule.Matches, HTTPRouteMatchToString) + return fmt.Sprintf("{matches:[%s]}", strings.Join(matches, ",")) +} + +func HTTPRouteMatchToString(match gatewayapiv1beta1.HTTPRouteMatch) string { + var patterns []string + if method := match.Method; method != nil { + patterns = append(patterns, fmt.Sprintf("method:%v", HTTPMethodToString(method))) + } + if path := match.Path; path != nil { + patterns = append(patterns, fmt.Sprintf("path:%s", HTTPPathMatchToString(path))) + } + if len(match.QueryParams) > 0 { + queryParams := Map(match.QueryParams, HTTPQueryParamMatchToString) + patterns = append(patterns, fmt.Sprintf("queryParams:[%s]", strings.Join(queryParams, ","))) + } + if len(match.Headers) > 0 { + headers := Map(match.Headers, HTTPHeaderMatchToString) + patterns = append(patterns, fmt.Sprintf("headers:[%s]", strings.Join(headers, ","))) + } + return fmt.Sprintf("{%s}", strings.Join(patterns, ",")) +} + +func HTTPPathMatchToString(path *gatewayapiv1beta1.HTTPPathMatch) string { + if path == nil { + return "*" + } + if path.Type != nil { + switch *path.Type { + case gatewayapiv1beta1.PathMatchExact: + return fmt.Sprintf("%s", *path.Value) + case gatewayapiv1beta1.PathMatchRegularExpression: + return fmt.Sprintf("~/%s/", *path.Value) + } + } + return fmt.Sprintf("%s*", *path.Value) +} + +func HTTPHeaderMatchToString(header gatewayapiv1beta1.HTTPHeaderMatch) string { + if header.Type != nil { + switch *header.Type { + case gatewayapiv1beta1.HeaderMatchRegularExpression: + return fmt.Sprintf("{%s:~/%s/}", header.Name, header.Value) + } + } + return fmt.Sprintf("{%s:%s}", header.Name, header.Value) +} + +func HTTPQueryParamMatchToString(queryParam gatewayapiv1beta1.HTTPQueryParamMatch) string { + if queryParam.Type != nil { + switch *queryParam.Type { + case gatewayapiv1beta1.QueryParamMatchRegularExpression: + return fmt.Sprintf("{%s:~/%s/}", queryParam.Name, queryParam.Value) + } + } + return fmt.Sprintf("{%s:%s}", queryParam.Name, queryParam.Value) +} + +func HTTPMethodToString(method *gatewayapiv1beta1.HTTPMethod) string { + if method == nil { + return "*" + } + return string(*method) +} + func GetNamespaceFromPolicyTargetRef(ctx context.Context, cli client.Client, policy KuadrantPolicy) (string, error) { targetRef := policy.GetTargetRef() gwNamespacedName := types.NamespacedName{Namespace: string(GetDefaultIfNil(targetRef.Namespace, policy.GetWrappedNamespace())), Name: string(targetRef.Name)} diff --git a/pkg/common/gatewayapi_utils_test.go b/pkg/common/gatewayapi_utils_test.go index d17ef8025..1f0668c22 100644 --- a/pkg/common/gatewayapi_utils_test.go +++ b/pkg/common/gatewayapi_utils_test.go @@ -241,6 +241,443 @@ func TestRulesFromHTTPRoute(t *testing.T) { } } +func TestHTTPRouteRuleSelectorSelects(t *testing.T) { + testCases := []struct { + name string + selector HTTPRouteRuleSelector + rule gatewayapiv1beta1.HTTPRouteRule + expected bool + }{ + { + name: "when the httproutrule contains the exact match then return true", + selector: HTTPRouteRuleSelector{ + HTTPRouteMatch: &gatewayapiv1beta1.HTTPRouteMatch{ + Method: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethodGet}[0], + Headers: []gatewayapiv1beta1.HTTPHeaderMatch{ + { + Type: &[]gatewayapiv1beta1.HeaderMatchType{gatewayapiv1beta1.HeaderMatchExact}[0], + Name: "someheader", + Value: "somevalue", + }, + }, + }, + }, + rule: gatewayapiv1beta1.HTTPRouteRule{ + Matches: []gatewayapiv1beta1.HTTPRouteMatch{ + { + Method: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethodGet}[0], + Headers: []gatewayapiv1beta1.HTTPHeaderMatch{ + { + Type: &[]gatewayapiv1beta1.HeaderMatchType{gatewayapiv1beta1.HeaderMatchExact}[0], + Name: "someheader", + Value: "somevalue", + }, + }, + }, + }, + }, + expected: true, + }, + { + name: "when the httproutrule contains the exact match and more then return true", + selector: HTTPRouteRuleSelector{ + HTTPRouteMatch: &gatewayapiv1beta1.HTTPRouteMatch{ + Method: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethodGet}[0], + }, + }, + rule: gatewayapiv1beta1.HTTPRouteRule{ + Matches: []gatewayapiv1beta1.HTTPRouteMatch{ + { + Method: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethodGet}[0], + Headers: []gatewayapiv1beta1.HTTPHeaderMatch{ + { + Type: &[]gatewayapiv1beta1.HeaderMatchType{gatewayapiv1beta1.HeaderMatchExact}[0], + Name: "someheader", + Value: "somevalue", + }, + }, + }, + }, + }, + expected: true, + }, + { + name: "when the httproutrule contains all the matching headers and more then return true", + selector: HTTPRouteRuleSelector{ + HTTPRouteMatch: &gatewayapiv1beta1.HTTPRouteMatch{ + Method: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethodGet}[0], + Headers: []gatewayapiv1beta1.HTTPHeaderMatch{ + { + Type: &[]gatewayapiv1beta1.HeaderMatchType{gatewayapiv1beta1.HeaderMatchExact}[0], + Name: "someheader", + Value: "somevalue", + }, + }, + }, + }, + rule: gatewayapiv1beta1.HTTPRouteRule{ + Matches: []gatewayapiv1beta1.HTTPRouteMatch{ + { + Method: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethodGet}[0], + Headers: []gatewayapiv1beta1.HTTPHeaderMatch{ + { + Type: &[]gatewayapiv1beta1.HeaderMatchType{gatewayapiv1beta1.HeaderMatchExact}[0], + Name: "someheader", + Value: "somevalue", + }, + { + Type: &[]gatewayapiv1beta1.HeaderMatchType{gatewayapiv1beta1.HeaderMatchRegularExpression}[0], + Name: "someotherheader", + Value: "someregex.*", + }, + }, + }, + }, + }, + expected: true, + }, + { + name: "when the httproutrule contains an inexact match then return false", + selector: HTTPRouteRuleSelector{ + HTTPRouteMatch: &gatewayapiv1beta1.HTTPRouteMatch{ + Method: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethodGet}[0], + Headers: []gatewayapiv1beta1.HTTPHeaderMatch{ + { + Type: &[]gatewayapiv1beta1.HeaderMatchType{gatewayapiv1beta1.HeaderMatchExact}[0], + Name: "someheader", + Value: "somevalue", + }, + }, + }, + }, + rule: gatewayapiv1beta1.HTTPRouteRule{ + Matches: []gatewayapiv1beta1.HTTPRouteMatch{ + { + Method: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethodPost}[0], + Headers: []gatewayapiv1beta1.HTTPHeaderMatch{ + { + Type: &[]gatewayapiv1beta1.HeaderMatchType{gatewayapiv1beta1.HeaderMatchExact}[0], + Name: "someheader", + Value: "somevalue", + }, + }, + }, + }, + }, + expected: false, + }, + { + name: "when the httproutrule is empty then return false", + rule: gatewayapiv1beta1.HTTPRouteRule{}, + selector: HTTPRouteRuleSelector{ + HTTPRouteMatch: &gatewayapiv1beta1.HTTPRouteMatch{ + Method: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethodGet}[0], + }, + }, + expected: false, + }, + { + name: "when the selector is empty then return true", + selector: HTTPRouteRuleSelector{}, + rule: gatewayapiv1beta1.HTTPRouteRule{ + Matches: []gatewayapiv1beta1.HTTPRouteMatch{ + { + Method: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethodGet}[0], + Headers: []gatewayapiv1beta1.HTTPHeaderMatch{ + { + Type: &[]gatewayapiv1beta1.HeaderMatchType{gatewayapiv1beta1.HeaderMatchExact}[0], + Name: "someheader", + Value: "somevalue", + }, + }, + }, + }, + }, + expected: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if r := tc.selector.Selects(tc.rule); r != tc.expected { + expectedStr := "" + resultStr := "not" + if !tc.expected { + expectedStr = "not" + resultStr = "" + } + t.Error("expected selector", HTTPRouteMatchToString(*tc.selector.HTTPRouteMatch), expectedStr, "to select rule", HTTPRouteRuleToString(tc.rule), "but it does", resultStr) + } + }) + } +} + +func TestHTTPPathMatchToString(t *testing.T) { + testCases := []struct { + name string + input *gatewayapiv1beta1.HTTPPathMatch + expected string + }{ + { + name: "exact path match", + input: &[]gatewayapiv1beta1.HTTPPathMatch{ + { + Type: &[]gatewayapiv1beta1.PathMatchType{gatewayapiv1beta1.PathMatchExact}[0], + Value: &[]string{"/foo"}[0], + }, + }[0], + expected: "/foo", + }, + { + name: "regex path match", + input: &[]gatewayapiv1beta1.HTTPPathMatch{ + { + Type: &[]gatewayapiv1beta1.PathMatchType{gatewayapiv1beta1.PathMatchRegularExpression}[0], + Value: &[]string{"^\\/foo.*"}[0], + }, + }[0], + expected: "~/^\\/foo.*/", + }, + { + name: "path prefix match", + input: &[]gatewayapiv1beta1.HTTPPathMatch{ + { + Type: &[]gatewayapiv1beta1.PathMatchType{gatewayapiv1beta1.PathMatchPathPrefix}[0], + Value: &[]string{"/foo"}[0], + }, + }[0], + expected: "/foo*", + }, + { + name: "path match with default type", + input: &[]gatewayapiv1beta1.HTTPPathMatch{ + { + Value: &[]string{"/foo"}[0], + }, + }[0], + expected: "/foo*", + }, + { + name: "nil path match", + input: nil, + expected: "*", + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if r := HTTPPathMatchToString(tc.input); r != tc.expected { + t.Errorf("expected: %s, got: %s", tc.expected, r) + } + }) + } +} + +func TestHTTPHeaderMatchToString(t *testing.T) { + testCases := []struct { + name string + input gatewayapiv1beta1.HTTPHeaderMatch + expected string + }{ + { + name: "exact header match", + input: gatewayapiv1beta1.HTTPHeaderMatch{ + Type: &[]gatewayapiv1beta1.HeaderMatchType{gatewayapiv1beta1.HeaderMatchExact}[0], + Name: "some-header", + Value: "foo", + }, + expected: "{some-header:foo}", + }, + { + name: "regex header match", + input: gatewayapiv1beta1.HTTPHeaderMatch{ + Type: &[]gatewayapiv1beta1.HeaderMatchType{gatewayapiv1beta1.HeaderMatchRegularExpression}[0], + Name: "some-header", + Value: "^foo.*", + }, + expected: "{some-header:~/^foo.*/}", + }, + { + name: "header match with default type", + input: gatewayapiv1beta1.HTTPHeaderMatch{ + Name: "some-header", + Value: "foo", + }, + expected: "{some-header:foo}", + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if r := HTTPHeaderMatchToString(tc.input); r != tc.expected { + t.Errorf("expected: %s, got: %s", tc.expected, r) + } + }) + } +} + +func TestHTTPQueryParamMatchToString(t *testing.T) { + testCases := []struct { + name string + input gatewayapiv1beta1.HTTPQueryParamMatch + expected string + }{ + { + name: "exact query param match", + input: gatewayapiv1beta1.HTTPQueryParamMatch{ + Type: &[]gatewayapiv1beta1.QueryParamMatchType{gatewayapiv1beta1.QueryParamMatchExact}[0], + Name: "some-param", + Value: "foo", + }, + expected: "{some-param:foo}", + }, + { + name: "regex query param match", + input: gatewayapiv1beta1.HTTPQueryParamMatch{ + Type: &[]gatewayapiv1beta1.QueryParamMatchType{gatewayapiv1beta1.QueryParamMatchRegularExpression}[0], + Name: "some-param", + Value: "^foo.*", + }, + expected: "{some-param:~/^foo.*/}", + }, + { + name: "query param match with default type", + input: gatewayapiv1beta1.HTTPQueryParamMatch{ + Name: "some-param", + Value: "foo", + }, + expected: "{some-param:foo}", + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if r := HTTPQueryParamMatchToString(tc.input); r != tc.expected { + t.Errorf("expected: %s, got: %s", tc.expected, r) + } + }) + } +} + +func TestHTTPMethodToString(t *testing.T) { + testCases := []struct { + input *gatewayapiv1beta1.HTTPMethod + expected string + }{ + { + input: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethodGet}[0], + expected: "GET", + }, + { + input: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethodHead}[0], + expected: "HEAD", + }, + { + input: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethodPost}[0], + expected: "POST", + }, + { + input: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethodPut}[0], + expected: "PUT", + }, + { + input: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethodPatch}[0], + expected: "PATCH", + }, + { + input: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethodDelete}[0], + expected: "DELETE", + }, + { + input: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethodConnect}[0], + expected: "CONNECT", + }, + { + input: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethodOptions}[0], + expected: "OPTIONS", + }, + { + input: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethodTrace}[0], + expected: "TRACE", + }, + { + input: nil, + expected: "*", + }, + } + for _, tc := range testCases { + if r := HTTPMethodToString(tc.input); r != tc.expected { + t.Errorf("expected: %s, got: %s", tc.expected, r) + } + } +} + +func TestHTTPRouteMatchToString(t *testing.T) { + match := gatewayapiv1beta1.HTTPRouteMatch{ + Path: &gatewayapiv1beta1.HTTPPathMatch{ + Type: &[]gatewayapiv1beta1.PathMatchType{gatewayapiv1beta1.PathMatchExact}[0], + Value: &[]string{"/foo"}[0], + }, + Method: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethodGet}[0], + QueryParams: []gatewayapiv1beta1.HTTPQueryParamMatch{ + { + Type: &[]gatewayapiv1beta1.QueryParamMatchType{gatewayapiv1beta1.QueryParamMatchRegularExpression}[0], + Name: "page", + Value: "\\d+", + }, + }, + } + + expected := "{method:GET,path:/foo,queryParams:[{page:~/\\d+/}]}" + + if r := HTTPRouteMatchToString(match); r != expected { + t.Errorf("expected: %s, got: %s", expected, r) + } + + match.Headers = []gatewayapiv1beta1.HTTPHeaderMatch{ + { + Name: "x-foo", + Value: "bar", + }, + } + + expected = "{method:GET,path:/foo,queryParams:[{page:~/\\d+/}],headers:[{x-foo:bar}]}" + + if r := HTTPRouteMatchToString(match); r != expected { + t.Errorf("expected: %s, got: %s", expected, r) + } +} + +func TestHTTPRouteRuleToString(t *testing.T) { + rule := gatewayapiv1beta1.HTTPRouteRule{} + + expected := "{matches:[]}" + + if r := HTTPRouteRuleToString(rule); r != expected { + t.Errorf("expected: %s, got: %s", expected, r) + } + + rule.Matches = []gatewayapiv1beta1.HTTPRouteMatch{ + { + Path: &gatewayapiv1beta1.HTTPPathMatch{ + Type: &[]gatewayapiv1beta1.PathMatchType{gatewayapiv1beta1.PathMatchExact}[0], + Value: &[]string{"/foo"}[0], + }, + Method: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethodGet}[0], + QueryParams: []gatewayapiv1beta1.HTTPQueryParamMatch{ + { + Type: &[]gatewayapiv1beta1.QueryParamMatchType{gatewayapiv1beta1.QueryParamMatchRegularExpression}[0], + Name: "page", + Value: "\\d+", + }, + }, + }, + } + + expected = "{matches:[{method:GET,path:/foo,queryParams:[{page:~/\\d+/}]}]}" + + if r := HTTPRouteRuleToString(rule); r != expected { + t.Errorf("expected: %s, got: %s", expected, r) + } +} + func TestGatewaysMissingPolicyRef(t *testing.T) { gwList := &gatewayapiv1beta1.GatewayList{ Items: []gatewayapiv1beta1.Gateway{ diff --git a/pkg/rlptools/route_selectors.go b/pkg/rlptools/route_selectors.go new file mode 100644 index 000000000..f8dc7e992 --- /dev/null +++ b/pkg/rlptools/route_selectors.go @@ -0,0 +1,27 @@ +package rlptools + +import ( + gatewayapiv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1" + + kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" + "github.com/kuadrant/kuadrant-operator/pkg/common" +) + +func HTTPRouteRulesFromRouteSelector(routeSelector kuadrantv1beta2.RouteSelector, route *gatewayapiv1beta1.HTTPRoute) []gatewayapiv1beta1.HTTPRouteRule { + rulesIndices := make(map[int]gatewayapiv1beta1.HTTPRouteRule, 0) + if len(routeSelector.Hostnames) > 0 && !common.Intersect(routeSelector.Hostnames, route.Spec.Hostnames) { + return nil + } + if len(routeSelector.Matches) == 0 { + return route.Spec.Rules + } + for _, routeSelectorMatch := range routeSelector.Matches { + for idx, rule := range route.Spec.Rules { + rs := common.HTTPRouteRuleSelector{HTTPRouteMatch: &routeSelectorMatch} + if rs.Selects(rule) { + rulesIndices[idx] = rule + } + } + } + return common.MapValues(rulesIndices) +} diff --git a/pkg/rlptools/route_selectors_test.go b/pkg/rlptools/route_selectors_test.go new file mode 100644 index 000000000..41eaf609d --- /dev/null +++ b/pkg/rlptools/route_selectors_test.go @@ -0,0 +1,212 @@ +//go:build unit + +package rlptools + +import ( + "fmt" + "reflect" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + gatewayapiv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1" + + kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" + "github.com/kuadrant/kuadrant-operator/pkg/common" +) + +func TestRouteSelectors(t *testing.T) { + gatewayHostnames := []gatewayapiv1beta1.Hostname{ + "*.toystore.com", + } + + gateway := &gatewayapiv1beta1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-gateway", + }, + } + + for _, hostname := range gatewayHostnames { + gateway.Spec.Listeners = append(gateway.Spec.Listeners, gatewayapiv1beta1.Listener{Hostname: &hostname}) + } + + route := &gatewayapiv1beta1.HTTPRoute{ + Spec: gatewayapiv1beta1.HTTPRouteSpec{ + CommonRouteSpec: gatewayapiv1beta1.CommonRouteSpec{ + ParentRefs: []gatewayapiv1beta1.ParentReference{ + { + Name: gatewayapiv1beta1.ObjectName(gateway.Name), + }, + }, + }, + Hostnames: []gatewayapiv1beta1.Hostname{"api.toystore.com"}, + Rules: []gatewayapiv1beta1.HTTPRouteRule{ + { + Matches: []gatewayapiv1beta1.HTTPRouteMatch{ + // get /toys* + { + Path: &gatewayapiv1beta1.HTTPPathMatch{ + Type: &[]gatewayapiv1beta1.PathMatchType{gatewayapiv1beta1.PathMatchPathPrefix}[0], + Value: &[]string{"/toy"}[0], + }, + Method: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethod("GET")}[0], + }, + // post /toys* + { + Path: &gatewayapiv1beta1.HTTPPathMatch{ + Type: &[]gatewayapiv1beta1.PathMatchType{gatewayapiv1beta1.PathMatchPathPrefix}[0], + Value: &[]string{"/toy"}[0], + }, + Method: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethod("POST")}[0], + }, + }, + }, + { + Matches: []gatewayapiv1beta1.HTTPRouteMatch{ + // /assets* + { + Path: &gatewayapiv1beta1.HTTPPathMatch{ + Type: &[]gatewayapiv1beta1.PathMatchType{gatewayapiv1beta1.PathMatchPathPrefix}[0], + Value: &[]string{"/assets"}[0], + }, + }, + }, + }, + }, + }, + } + + testCases := []struct { + name string + routeSelector kuadrantv1beta2.RouteSelector + route *gatewayapiv1beta1.HTTPRoute + expected []gatewayapiv1beta1.HTTPRouteRule + }{ + { + name: "empty route selector selects all HTTPRouteRules", + routeSelector: kuadrantv1beta2.RouteSelector{}, + route: route, + expected: route.Spec.Rules, + }, + { + name: "route selector selects the HTTPRouteRules whose set of HTTPRouteMatch is a perfect match", + routeSelector: kuadrantv1beta2.RouteSelector{ + Matches: []gatewayapiv1beta1.HTTPRouteMatch{ + { + Path: &gatewayapiv1beta1.HTTPPathMatch{ + Type: &[]gatewayapiv1beta1.PathMatchType{gatewayapiv1beta1.PathMatchPathPrefix}[0], + Value: &[]string{"/assets"}[0], + }, + }, + }, + }, + route: route, + expected: []gatewayapiv1beta1.HTTPRouteRule{route.Spec.Rules[1]}, + }, + { + name: "route selector selects the HTTPRouteRules whose set of HTTPRouteMatch contains at least one match", + routeSelector: kuadrantv1beta2.RouteSelector{ + Matches: []gatewayapiv1beta1.HTTPRouteMatch{ + { + Path: &gatewayapiv1beta1.HTTPPathMatch{ + Type: &[]gatewayapiv1beta1.PathMatchType{gatewayapiv1beta1.PathMatchPathPrefix}[0], + Value: &[]string{"/toy"}[0], + }, + Method: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethod("POST")}[0], + }, + }, + }, + route: route, + expected: []gatewayapiv1beta1.HTTPRouteRule{route.Spec.Rules[0]}, + }, + { + name: "route selector with missing part of a HTTPRouteMatch still selects the HTTPRouteRules that match", + routeSelector: kuadrantv1beta2.RouteSelector{ + Matches: []gatewayapiv1beta1.HTTPRouteMatch{ + { + Path: &gatewayapiv1beta1.HTTPPathMatch{ + Type: &[]gatewayapiv1beta1.PathMatchType{gatewayapiv1beta1.PathMatchPathPrefix}[0], + Value: &[]string{"/toy"}[0], + }, + }, + }, + }, + route: route, + expected: []gatewayapiv1beta1.HTTPRouteRule{route.Spec.Rules[0]}, + }, + { + name: "route selector selects no HTTPRouteRule when no criterion matches", + routeSelector: kuadrantv1beta2.RouteSelector{ + Matches: []gatewayapiv1beta1.HTTPRouteMatch{ + { + Path: &gatewayapiv1beta1.HTTPPathMatch{ + Type: &[]gatewayapiv1beta1.PathMatchType{gatewayapiv1beta1.PathMatchExact}[0], + Value: &[]string{"/toy"}[0], + }, + }, + }, + }, + route: route, + expected: []gatewayapiv1beta1.HTTPRouteRule{}, + }, + { + name: "route selector selects the HTTPRouteRules whose HTTPRoute's hostnames match the selector", + routeSelector: kuadrantv1beta2.RouteSelector{ + Hostnames: []gatewayapiv1beta1.Hostname{"api.toystore.com"}, + }, + route: route, + expected: route.Spec.Rules, + }, + { + name: "route selector selects the HTTPRouteRules whose HTTPRoute's hostnames match the selector additionally to other criteria", + routeSelector: kuadrantv1beta2.RouteSelector{ + Hostnames: []gatewayapiv1beta1.Hostname{"api.toystore.com"}, + Matches: []gatewayapiv1beta1.HTTPRouteMatch{ + { + Path: &gatewayapiv1beta1.HTTPPathMatch{ + Type: &[]gatewayapiv1beta1.PathMatchType{gatewayapiv1beta1.PathMatchPathPrefix}[0], + Value: &[]string{"/toy"}[0], + }, + }, + }, + }, + route: route, + expected: []gatewayapiv1beta1.HTTPRouteRule{route.Spec.Rules[0]}, + }, + { + name: "route selector does not select HTTPRouteRules whose HTTPRoute's hostnames do not match the selector", + routeSelector: kuadrantv1beta2.RouteSelector{ + Hostnames: []gatewayapiv1beta1.Hostname{"www.toystore.com"}, + }, + route: route, + expected: nil, + }, + { + name: "route selector does not select HTTPRouteRules whose HTTPRoute's hostnames do not match the selector even when other criteria match", + routeSelector: kuadrantv1beta2.RouteSelector{ + Hostnames: []gatewayapiv1beta1.Hostname{"www.toystore.com"}, + Matches: []gatewayapiv1beta1.HTTPRouteMatch{ + { + Path: &gatewayapiv1beta1.HTTPPathMatch{ + Type: &[]gatewayapiv1beta1.PathMatchType{gatewayapiv1beta1.PathMatchPathPrefix}[0], + Value: &[]string{"/toy"}[0], + }, + }, + }, + }, + route: route, + expected: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + rules := HTTPRouteRulesFromRouteSelector(tc.routeSelector, tc.route) + rulesToStringSlice := func(rules []gatewayapiv1beta1.HTTPRouteRule) []string { + return common.Map(common.Map(rules, common.HTTPRouteRuleToString), func(r string) string { return fmt.Sprintf("{%s}", r) }) + } + if !reflect.DeepEqual(rules, tc.expected) { + t.Errorf("expected %v, got %v", rulesToStringSlice(tc.expected), rulesToStringSlice(rules)) + } + }) + } +} diff --git a/pkg/rlptools/wasm_utils.go b/pkg/rlptools/wasm_utils.go index 8f66100b9..bb4630a3f 100644 --- a/pkg/rlptools/wasm_utils.go +++ b/pkg/rlptools/wasm_utils.go @@ -31,7 +31,6 @@ func WasmRules(rlp *kuadrantv1beta2.RateLimitPolicy, route *gatewayapiv1beta1.HT // 1 RLP limit <---> 1 WASM rule limitFullName := FullLimitName(rlp, limitName) rule := ruleFromLimit(limitFullName, &limit, route) - rules = append(rules, rule) } @@ -43,80 +42,100 @@ func ruleFromLimit(limitFullName string, limit *kuadrantv1beta2.Limit, route *ga return wasm.Rule{} } - return wasm.Rule{ - Conditions: conditionsFromLimit(limit, route), - Data: dataFromLimt(limitFullName, limit), + rule := wasm.Rule{} + + if conditions := conditionsFromLimit(limit, route); conditions != nil { + rule.Conditions = conditions } -} -func conditionsFromLimit(limit *kuadrantv1beta2.Limit, route *gatewayapiv1beta1.HTTPRoute) []wasm.Condition { - if limit == nil { - return make([]wasm.Condition, 0) + if data := dataFromLimt(limitFullName, limit); data != nil { + rule.Data = data } - // TODO(eastizle): review this implementation. This is a first naive implementation. - // The conditions must always be a subset of the route's matching rules. + return rule +} - conditions := make([]wasm.Condition, 0) +func conditionsFromLimit(limit *kuadrantv1beta2.Limit, route *gatewayapiv1beta1.HTTPRoute) []wasm.Condition { + routeConditions := make([]wasm.Condition, 0) - for routeSelectorIdx := range limit.RouteSelectors { - // TODO(eastizle): what if there are only Hostnames (i.e. empty "matches" list) - for matchIdx := range limit.RouteSelectors[routeSelectorIdx].Matches { - condition := wasm.Condition{ - AllOf: patternExpresionsFromMatch(&limit.RouteSelectors[routeSelectorIdx].Matches[matchIdx]), - } + if limit == nil { + return routeConditions + } - // merge hostnames expression in the same condition - for _, hostname := range limit.RouteSelectors[routeSelectorIdx].Hostnames { - condition.AllOf = append(condition.AllOf, patternExpresionFromHostname(hostname)) + // build conditions from the rules selected by the route selectors + for _, routeSelector := range limit.RouteSelectors { + for _, rule := range HTTPRouteRulesFromRouteSelector(routeSelector, route) { + hostnames := routeSelector.Hostnames // FIXME(guicassolato): it should be the intersection between routeSelector.Hostnames and route.Spec.Hostnames + if len(hostnames) == 0 { + hostnames = route.Spec.Hostnames } + routeConditions = append(routeConditions, conditionsFromRule(rule, hostnames)...) + } + } - conditions = append(conditions, condition) + // build conditions from the route if no route selectors are defined + if len(routeConditions) == 0 { + for _, rule := range route.Spec.Rules { + routeConditions = append(routeConditions, conditionsFromRule(rule, route.Spec.Hostnames)...) } } - if len(conditions) == 0 { - conditions = append(conditions, conditionsFromRoute(route)...) + if len(limit.When) == 0 { + return routeConditions } - // merge when expression in the same condition - // must be done after adding route level conditions when no route selector are available - // prevent conditions only filled with "when" definitions - for whenIdx := range limit.When { - for idx := range conditions { - conditions[idx].AllOf = append(conditions[idx].AllOf, patternExpresionFromWhen(limit.When[whenIdx])) + if len(routeConditions) > 0 { + // merge the 'when' conditions into each route level one + mergedConditions := make([]wasm.Condition, len(routeConditions)) + for _, when := range limit.When { + for idx := range routeConditions { + mergedCondition := routeConditions[idx] + mergedCondition.AllOf = append(mergedCondition.AllOf, patternExpresionFromWhen(when)) + mergedConditions[idx] = mergedCondition + } } + return mergedConditions } - return conditions -} - -func conditionsFromRoute(route *gatewayapiv1beta1.HTTPRoute) []wasm.Condition { - if route == nil { - return make([]wasm.Condition, 0) + // build conditions only from the 'when' field + whenConditions := make([]wasm.Condition, len(limit.When)) + for idx, when := range limit.When { + whenConditions[idx] = wasm.Condition{AllOf: []wasm.PatternExpression{patternExpresionFromWhen(when)}} } + return whenConditions +} - conditions := make([]wasm.Condition, 0) - - for ruleIdx := range route.Spec.Rules { - // One condition per match - for matchIdx := range route.Spec.Rules[ruleIdx].Matches { - conditions = append(conditions, wasm.Condition{ - AllOf: patternExpresionsFromMatch(&route.Spec.Rules[ruleIdx].Matches[matchIdx]), - }) +// conditionsFromRule builds a list of conditions from a rule and a list of hostnames +// each combination of a rule match and hostname yields one condition +// rules that specify no explicit match are assumed to match all request (i.e. implicit catch-all rule) +// empty list of hostnames yields a condition without a hostname pattern expression +func conditionsFromRule(rule gatewayapiv1beta1.HTTPRouteRule, hostnames []gatewayapiv1beta1.Hostname) (conditions []wasm.Condition) { + if len(rule.Matches) == 0 { + for _, hostname := range hostnames { + condition := wasm.Condition{AllOf: []wasm.PatternExpression{patternExpresionFromHostname(hostname)}} + conditions = append(conditions, condition) } + return } - return conditions -} + for _, match := range rule.Matches { + condition := wasm.Condition{AllOf: patternExpresionsFromMatch(match)} -func patternExpresionsFromMatch(match *gatewayapiv1beta1.HTTPRouteMatch) []wasm.PatternExpression { - // TODO(eastizle): only paths and methods implemented + if len(hostnames) > 0 { + for _, hostname := range hostnames { + mergedCondition := condition + mergedCondition.AllOf = append(mergedCondition.AllOf, patternExpresionFromHostname(hostname)) + conditions = append(conditions, mergedCondition) + } + continue + } - if match == nil { - return make([]wasm.PatternExpression, 0) + conditions = append(conditions, condition) } + return +} +func patternExpresionsFromMatch(match gatewayapiv1beta1.HTTPRouteMatch) []wasm.PatternExpression { expressions := make([]wasm.PatternExpression, 0) if match.Path != nil { @@ -127,6 +146,8 @@ func patternExpresionsFromMatch(match *gatewayapiv1beta1.HTTPRouteMatch) []wasm. expressions = append(expressions, patternExpresionFromMethod(*match.Method)) } + // TODO(eastizle): only paths and methods implemented + return expressions } @@ -162,29 +183,27 @@ func patternExpresionFromMethod(method gatewayapiv1beta1.HTTPMethod) wasm.Patter } } -func patternExpresionFromWhen(when kuadrantv1beta2.WhenCondition) wasm.PatternExpression { +func patternExpresionFromHostname(hostname gatewayapiv1beta1.Hostname) wasm.PatternExpression { return wasm.PatternExpression{ - Selector: when.Selector, - Operator: wasm.PatternOperator(when.Operator), - Value: when.Value, + Selector: "request.host", + Operator: "eq", // FIXME(guicassolato): won't work for wildcard domains + Value: string(hostname), } } -func patternExpresionFromHostname(hostname gatewayapiv1beta1.Hostname) wasm.PatternExpression { +func patternExpresionFromWhen(when kuadrantv1beta2.WhenCondition) wasm.PatternExpression { return wasm.PatternExpression{ - Selector: "request.host", - Operator: "eq", - Value: string(hostname), + Selector: when.Selector, + Operator: wasm.PatternOperator(when.Operator), + Value: when.Value, } } -func dataFromLimt(limitFullName string, limit *kuadrantv1beta2.Limit) []wasm.DataItem { +func dataFromLimt(limitFullName string, limit *kuadrantv1beta2.Limit) (data []wasm.DataItem) { if limit == nil { - return make([]wasm.DataItem, 0) + return } - data := make([]wasm.DataItem, 0) - // static key representing the limit data = append(data, wasm.DataItem{Static: &wasm.StaticSpec{Key: limitFullName, Value: "1"}}) diff --git a/pkg/rlptools/wasm_utils_test.go b/pkg/rlptools/wasm_utils_test.go index 573308caf..5717c3a42 100644 --- a/pkg/rlptools/wasm_utils_test.go +++ b/pkg/rlptools/wasm_utils_test.go @@ -19,7 +19,9 @@ import ( func TestWasmRules(t *testing.T) { httpRoute := &gatewayapiv1beta1.HTTPRoute{ Spec: gatewayapiv1beta1.HTTPRouteSpec{ - Hostnames: []gatewayapiv1beta1.Hostname{"*.example.com"}, + Hostnames: []gatewayapiv1beta1.Hostname{ + "*.example.com", + }, Rules: []gatewayapiv1beta1.HTTPRouteRule{ { Matches: []gatewayapiv1beta1.HTTPRouteMatch{ @@ -39,14 +41,21 @@ func TestWasmRules(t *testing.T) { t.Run("minimal RLP", func(subT *testing.T) { rlp := &kuadrantv1beta2.RateLimitPolicy{ TypeMeta: metav1.TypeMeta{ - Kind: "RateLimitPolicy", APIVersion: kuadrantv1beta2.GroupVersion.String()}, - ObjectMeta: metav1.ObjectMeta{Name: "rlpA", Namespace: "nsA"}, + Kind: "RateLimitPolicy", + APIVersion: kuadrantv1beta2.GroupVersion.String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "rlpA", + Namespace: "nsA", + }, Spec: kuadrantv1beta2.RateLimitPolicySpec{ Limits: map[string]kuadrantv1beta2.Limit{ - "l1": kuadrantv1beta2.Limit{ + "l1": { Rates: []kuadrantv1beta2.Rate{ { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + Limit: 1, + Duration: 3, + Unit: kuadrantv1beta2.TimeUnit("minute"), }, }, }, @@ -68,6 +77,11 @@ func TestWasmRules(t *testing.T) { Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), Value: "GET", }, + { + Selector: "request.host", + Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), + Value: "*.example.com", + }, }, }, }, From 19edca495da446935bd3bfb0e3e73281969c5cc6 Mon Sep 17 00:00:00 2001 From: Guilherme Cassolato Date: Sat, 24 Jun 2023 13:01:10 +0200 Subject: [PATCH 02/15] 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. --- .../ratelimitpolicy_controller_test.go | 576 ++++++++++++++---- pkg/common/common.go | 4 + 2 files changed, 448 insertions(+), 132 deletions(-) diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index 066ca6456..9f6cbf0b5 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -52,17 +52,7 @@ func testBuildBasicGateway(gwName, ns string) *gatewayapiv1beta1.Gateway { } } -func testBuildBasicHttpRoute(routeName, gwName, ns string, hostnamesStrSlice []string) *gatewayapiv1beta1.HTTPRoute { - tmpMatchPathPrefix := gatewayapiv1beta1.PathMatchPathPrefix - tmpMatchValue := "/toy" - tmpMatchMethod := gatewayapiv1beta1.HTTPMethod("GET") - gwNamespace := gatewayapiv1beta1.Namespace(ns) - - var hostnames []gatewayapiv1beta1.Hostname - for _, str := range hostnamesStrSlice { - hostnames = append(hostnames, gatewayapiv1beta1.Hostname(str)) - } - +func testBuildBasicHttpRoute(routeName, gwName, ns string, hostnames []string) *gatewayapiv1beta1.HTTPRoute { return &gatewayapiv1beta1.HTTPRoute{ TypeMeta: metav1.TypeMeta{ Kind: "HTTPRoute", @@ -78,78 +68,20 @@ func testBuildBasicHttpRoute(routeName, gwName, ns string, hostnamesStrSlice []s ParentRefs: []gatewayapiv1beta1.ParentReference{ { Name: gatewayapiv1beta1.ObjectName(gwName), - Namespace: &gwNamespace, + Namespace: common.Ptr(gatewayapiv1beta1.Namespace(ns)), }, }, }, - Hostnames: hostnames, + Hostnames: common.Map(hostnames, func(hostname string) gatewayapiv1beta1.Hostname { return gatewayapiv1beta1.Hostname(hostname) }), Rules: []gatewayapiv1beta1.HTTPRouteRule{ { Matches: []gatewayapiv1beta1.HTTPRouteMatch{ { Path: &gatewayapiv1beta1.HTTPPathMatch{ - Type: &tmpMatchPathPrefix, - Value: &tmpMatchValue, + Type: common.Ptr(gatewayapiv1beta1.PathMatchPathPrefix), + Value: common.Ptr("/toy"), }, - Method: &tmpMatchMethod, - }, - }, - }, - }, - }, - } -} - -func testBuildBasicRoutePolicy(policyName, ns, routeName string) *kuadrantv1beta2.RateLimitPolicy { - return &kuadrantv1beta2.RateLimitPolicy{ - TypeMeta: metav1.TypeMeta{ - Kind: "RateLimitPolicy", - APIVersion: kuadrantv1beta2.GroupVersion.String(), - }, - ObjectMeta: metav1.ObjectMeta{ - Name: policyName, - Namespace: ns, - }, - Spec: kuadrantv1beta2.RateLimitPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: gatewayapiv1beta1.Group("gateway.networking.k8s.io"), - Kind: "HTTPRoute", - Name: gatewayapiv1beta1.ObjectName(routeName), - }, - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), - }, - }, - }, - }, - }, - } -} - -func testBuildGatewayPolicy(policyName, ns, gwName string) *kuadrantv1beta2.RateLimitPolicy { - return &kuadrantv1beta2.RateLimitPolicy{ - TypeMeta: metav1.TypeMeta{ - Kind: "RateLimitPolicy", - APIVersion: kuadrantv1beta2.GroupVersion.String(), - }, - ObjectMeta: metav1.ObjectMeta{ - Name: policyName, - Namespace: ns, - }, - Spec: kuadrantv1beta2.RateLimitPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: gatewayapiv1beta1.Group("gateway.networking.k8s.io"), - Kind: "Gateway", - Name: gatewayapiv1beta1.ObjectName(gwName), - }, - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + Method: common.Ptr(gatewayapiv1beta1.HTTPMethod("GET")), }, }, }, @@ -190,48 +122,64 @@ var _ = Describe("RateLimitPolicy controller", func() { }, 15*time.Second, 5*time.Second).Should(BeTrue()) ApplyKuadrantCR(testNamespace) + + // Check Limitador Status is Ready + Eventually(func() bool { + limitador := &limitadorv1alpha1.Limitador{} + err := k8sClient.Get(context.Background(), client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace}, limitador) + if err != nil { + return false + } + if !meta.IsStatusConditionTrue(limitador.Status.Conditions, "Ready") { + return false + } + return true + }, time.Minute, 5*time.Second).Should(BeTrue()) } BeforeEach(beforeEachCallback) AfterEach(DeleteNamespaceCallback(&testNamespace)) - Context("Basic: RLP targeting HTTPRoute", func() { - It("check created resources", func() { - // Check Limitador Status is Ready - Eventually(func() bool { - limitador := &limitadorv1alpha1.Limitador{} - err := k8sClient.Get(context.Background(), client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace}, limitador) - if err != nil { - return false - } - if !meta.IsStatusConditionTrue(limitador.Status.Conditions, "Ready") { - return false - } - return true - }, time.Minute, 5*time.Second).Should(BeTrue()) - + Context("RLP targeting HTTPRoute", func() { + It("Creates all the resources for a basic HTTPRoute and RateLimitPolicy", func() { + // create httproute httpRoute := testBuildBasicHttpRoute(routeName, gwName, testNamespace, []string{"*.example.com"}) err := k8sClient.Create(context.Background(), httpRoute) Expect(err).ToNot(HaveOccurred()) - rlp := testBuildBasicRoutePolicy(rlpName, testNamespace, routeName) - rlpKey := client.ObjectKey{Name: rlpName, Namespace: testNamespace} + // create ratelimitpolicy + rlp := &kuadrantv1beta2.RateLimitPolicy{ + TypeMeta: metav1.TypeMeta{ + Kind: "RateLimitPolicy", + APIVersion: kuadrantv1beta2.GroupVersion.String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Name: rlpName, + Namespace: testNamespace, + }, + Spec: kuadrantv1beta2.RateLimitPolicySpec{ + TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ + Group: gatewayapiv1beta1.Group("gateway.networking.k8s.io"), + Kind: "HTTPRoute", + Name: gatewayapiv1beta1.ObjectName(routeName), + }, + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, + }, + }, + }, + }, + } err = k8sClient.Create(context.Background(), rlp) Expect(err).ToNot(HaveOccurred()) // Check RLP status is available - Eventually(func() bool { - existingRLP := &kuadrantv1beta2.RateLimitPolicy{} - err := k8sClient.Get(context.Background(), rlpKey, existingRLP) - if err != nil { - return false - } - if !meta.IsStatusConditionTrue(existingRLP.Status.Conditions, "Available") { - return false - } - - return true - }, time.Minute, 5*time.Second).Should(BeTrue()) + rlpKey := client.ObjectKeyFromObject(rlp) + Eventually(testRLPIsAvailable(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) // Check HTTPRoute direct back reference routeKey := client.ObjectKey{Name: routeName, Namespace: testNamespace} @@ -330,41 +278,390 @@ var _ = Describe("RateLimitPolicy controller", func() { Expect(existingGateway.GetAnnotations()).To(HaveKeyWithValue( common.RateLimitPoliciesBackRefAnnotation, string(serialized))) }) + + It("Creates the correct WasmPlugin for a complex HTTPRoute and a RateLimitPolicy", func() { + // create httproute + httpRoute := testBuildBasicHttpRoute(routeName, gwName, testNamespace, []string{"*.toystore.acme.com", "*.toystore.io"}) + httpRoute.Spec.Rules = []gatewayapiv1beta1.HTTPRouteRule{ + { + Matches: []gatewayapiv1beta1.HTTPRouteMatch{ + { // get /toys* + Path: &gatewayapiv1beta1.HTTPPathMatch{ + Type: common.Ptr(gatewayapiv1beta1.PathMatchPathPrefix), + Value: common.Ptr("/toys"), + }, + Method: common.Ptr(gatewayapiv1beta1.HTTPMethod("GET")), + }, + { // post /toys* + Path: &gatewayapiv1beta1.HTTPPathMatch{ + Type: common.Ptr(gatewayapiv1beta1.PathMatchPathPrefix), + Value: common.Ptr("/toys"), + }, + Method: common.Ptr(gatewayapiv1beta1.HTTPMethod("POST")), + }, + }, + }, + { + Matches: []gatewayapiv1beta1.HTTPRouteMatch{ + { // /assets* + Path: &gatewayapiv1beta1.HTTPPathMatch{ + Type: common.Ptr(gatewayapiv1beta1.PathMatchPathPrefix), + Value: common.Ptr("/assets"), + }, + }, + }, + }, + } + err := k8sClient.Create(context.Background(), httpRoute) + Expect(err).ToNot(HaveOccurred()) + + // create ratelimitpolicy + rlp := &kuadrantv1beta2.RateLimitPolicy{ + TypeMeta: metav1.TypeMeta{ + Kind: "RateLimitPolicy", + APIVersion: kuadrantv1beta2.GroupVersion.String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Name: rlpName, + Namespace: testNamespace, + }, + Spec: kuadrantv1beta2.RateLimitPolicySpec{ + TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ + Group: gatewayapiv1beta1.Group("gateway.networking.k8s.io"), + Kind: "HTTPRoute", + Name: gatewayapiv1beta1.ObjectName(routeName), + }, + Limits: map[string]kuadrantv1beta2.Limit{ + "toys": { + Rates: []kuadrantv1beta2.Rate{ + {Limit: 50, Duration: 1, Unit: kuadrantv1beta2.TimeUnit("minute")}, + }, + Counters: []kuadrantv1beta2.ContextSelector{"auth.identity.username"}, + RouteSelectors: []kuadrantv1beta2.RouteSelector{ + { // selects the 1st HTTPRouteRule (i.e. get|post /toys*) for one of the hostnames + Matches: []gatewayapiv1beta1.HTTPRouteMatch{ + { + Path: &gatewayapiv1beta1.HTTPPathMatch{ + Type: common.Ptr(gatewayapiv1beta1.PathMatchPathPrefix), + Value: common.Ptr("/toys"), + }, + }, + }, + Hostnames: []gatewayapiv1beta1.Hostname{"*.toystore.acme.com"}, + }, + }, + When: []kuadrantv1beta2.WhenCondition{ + { + Selector: "auth.identity.group", + Operator: kuadrantv1beta2.WhenConditionOperator("neq"), + Value: "admin", + }, + }, + }, + "assets": { + Rates: []kuadrantv1beta2.Rate{ + {Limit: 5, Duration: 1, Unit: kuadrantv1beta2.TimeUnit("minute")}, + {Limit: 100, Duration: 12, Unit: kuadrantv1beta2.TimeUnit("hour")}, + }, + RouteSelectors: []kuadrantv1beta2.RouteSelector{ + { // selects the 2nd HTTPRouteRule (i.e. /assets*) for all hostnames + Matches: []gatewayapiv1beta1.HTTPRouteMatch{ + { + Path: &gatewayapiv1beta1.HTTPPathMatch{ + Type: common.Ptr(gatewayapiv1beta1.PathMatchPathPrefix), + Value: common.Ptr("/assets"), + }, + }, + }, + }, + }, + }, + }, + }, + } + err = k8sClient.Create(context.Background(), rlp) + Expect(err).ToNot(HaveOccurred()) + + // Check RLP status is available + rlpKey := client.ObjectKeyFromObject(rlp) + Eventually(testRLPIsAvailable(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + + // Check wasm plugin + wpName := fmt.Sprintf("kuadrant-%s", gwName) + wasmPluginKey := client.ObjectKey{Name: wpName, Namespace: testNamespace} + existingWasmPlugin := &istioclientgoextensionv1alpha1.WasmPlugin{} + err = k8sClient.Get(context.Background(), wasmPluginKey, existingWasmPlugin) + // must exist + Expect(err).ToNot(HaveOccurred()) + existingWASMConfig, err := rlptools.WASMPluginFromStruct(existingWasmPlugin.Spec.PluginConfig) + Expect(err).ToNot(HaveOccurred()) + Expect(existingWASMConfig.FailureMode).To(Equal(wasm.FailureModeDeny)) + Expect(existingWASMConfig.RateLimitPolicies).To(HaveLen(2)) + // check the wasm rlp configs for each hostname in a verbose way because we cannot guarantee the order of the rules in each rlp config + // check wasm rlp config for the 1st hostname + wasmRLP, found := common.Find(existingWASMConfig.RateLimitPolicies, func(wasmRLP wasm.RateLimitPolicy) bool { + return wasmRLP.Name == "*.toystore.acme.com" + }) + Expect(found).To(BeTrue()) + Expect(wasmRLP.Domain).To(Equal(common.MarshallNamespace(client.ObjectKeyFromObject(gateway), "*.toystore.acme.com"))) + Expect(wasmRLP.Service).To(Equal(common.KuadrantRateLimitClusterName)) + Expect(wasmRLP.Hostnames).To(Equal([]string{"*.toystore.acme.com"})) + Expect(wasmRLP.Rules).To(ContainElement(wasm.Rule{ + Conditions: []wasm.Condition{ + { + AllOf: []wasm.PatternExpression{ + { + Selector: "request.url_path", + Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), + Value: "/toys", + }, + { + Selector: "request.method", + Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), + Value: "GET", + }, + { + Selector: "request.host", + Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), + Value: "*.toystore.acme.com", + }, + { + Selector: "auth.identity.group", + Operator: wasm.PatternOperator(kuadrantv1beta2.NotEqualOperator), + Value: "admin", + }, + }, + }, + { + AllOf: []wasm.PatternExpression{ + { + Selector: "request.url_path", + Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), + Value: "/toys", + }, + { + Selector: "request.method", + Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), + Value: "POST", + }, + { + Selector: "request.host", + Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), + Value: "*.toystore.acme.com", + }, + { + Selector: "auth.identity.group", + Operator: wasm.PatternOperator(kuadrantv1beta2.NotEqualOperator), + Value: "admin", + }, + }, + }, + }, + Data: []wasm.DataItem{ + { + Static: &wasm.StaticSpec{ + Key: fmt.Sprintf("%s/%s/toys", testNamespace, rlpName), + Value: "1", + }, + }, + { + Selector: &wasm.SelectorSpec{ + Selector: kuadrantv1beta2.ContextSelector("auth.identity.username"), + }, + }, + }, + })) + Expect(wasmRLP.Rules).To(ContainElement(wasm.Rule{ + Conditions: []wasm.Condition{ + { + AllOf: []wasm.PatternExpression{ + { + Selector: "request.url_path", + Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), + Value: "/assets", + }, + { + Selector: "request.host", + Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), + Value: "*.toystore.acme.com", + }, + }, + }, + { // FIXME(guicassolato): this condition should not be generated + AllOf: []wasm.PatternExpression{ + { + Selector: "request.url_path", + Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), + Value: "/assets", + }, + { + Selector: "request.host", + Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), + Value: "*.toystore.io", + }, + }, + }, + }, + Data: []wasm.DataItem{ + { + Static: &wasm.StaticSpec{ + Key: fmt.Sprintf("%s/%s/assets", testNamespace, rlpName), + Value: "1", + }, + }, + }, + })) + // check wasm rlp config for the 2nd hostname + wasmRLP, found = common.Find(existingWASMConfig.RateLimitPolicies, func(wasmRLP wasm.RateLimitPolicy) bool { + return wasmRLP.Name == "*.toystore.io" + }) + Expect(found).To(BeTrue()) + Expect(wasmRLP.Domain).To(Equal(common.MarshallNamespace(client.ObjectKeyFromObject(gateway), "*.toystore.io"))) + Expect(wasmRLP.Service).To(Equal(common.KuadrantRateLimitClusterName)) + Expect(wasmRLP.Hostnames).To(Equal([]string{"*.toystore.io"})) + Expect(wasmRLP.Rules).To(ContainElement(wasm.Rule{ // FIXME(guicassolato): this entire rule should not be generated + Conditions: []wasm.Condition{ + { + AllOf: []wasm.PatternExpression{ + { + Selector: "request.url_path", + Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), + Value: "/toys", + }, + { + Selector: "request.method", + Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), + Value: "GET", + }, + { + Selector: "request.host", + Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), + Value: "*.toystore.acme.com", + }, + { + Selector: "auth.identity.group", + Operator: wasm.PatternOperator(kuadrantv1beta2.NotEqualOperator), + Value: "admin", + }, + }, + }, + { + AllOf: []wasm.PatternExpression{ + { + Selector: "request.url_path", + Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), + Value: "/toys", + }, + { + Selector: "request.method", + Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), + Value: "POST", + }, + { + Selector: "request.host", + Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), + Value: "*.toystore.acme.com", + }, + { + Selector: "auth.identity.group", + Operator: wasm.PatternOperator(kuadrantv1beta2.NotEqualOperator), + Value: "admin", + }, + }, + }, + }, + Data: []wasm.DataItem{ + { + Static: &wasm.StaticSpec{ + Key: fmt.Sprintf("%s/%s/toys", testNamespace, rlpName), + Value: "1", + }, + }, + { + Selector: &wasm.SelectorSpec{ + Selector: kuadrantv1beta2.ContextSelector("auth.identity.username"), + }, + }, + }, + })) + Expect(wasmRLP.Rules).To(ContainElement(wasm.Rule{ + Conditions: []wasm.Condition{ + { // FIXME(guicassolato): this condition should not be generated + AllOf: []wasm.PatternExpression{ + { + Selector: "request.url_path", + Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), + Value: "/assets", + }, + { + Selector: "request.host", + Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), + Value: "*.toystore.acme.com", + }, + }, + }, + { + AllOf: []wasm.PatternExpression{ + { + Selector: "request.url_path", + Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), + Value: "/assets", + }, + { + Selector: "request.host", + Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), + Value: "*.toystore.io", + }, + }, + }, + }, + Data: []wasm.DataItem{ + { + Static: &wasm.StaticSpec{ + Key: fmt.Sprintf("%s/%s/assets", testNamespace, rlpName), + Value: "1", + }, + }, + }, + })) + }) }) - Context("Basic: RLP targeting Gateway", func() { - It("check created resources", func() { - // Check Limitador Status is Ready - Eventually(func() bool { - limitador := &limitadorv1alpha1.Limitador{} - err := k8sClient.Get(context.Background(), client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace}, limitador) - if err != nil { - return false - } - if !meta.IsStatusConditionTrue(limitador.Status.Conditions, "Ready") { - return false - } - return true - }, time.Minute, 5*time.Second).Should(BeTrue()) - - rlp := testBuildGatewayPolicy(rlpName, testNamespace, gwName) - rlpKey := client.ObjectKey{Name: rlpName, Namespace: testNamespace} + Context("RLP targeting Gateway", func() { + It("Creates all the resources for a basic Gateway and RateLimitPolicy", func() { + // create ratelimitpolicy + rlp := &kuadrantv1beta2.RateLimitPolicy{ + TypeMeta: metav1.TypeMeta{ + Kind: "RateLimitPolicy", + APIVersion: kuadrantv1beta2.GroupVersion.String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Name: rlpName, + Namespace: testNamespace, + }, + Spec: kuadrantv1beta2.RateLimitPolicySpec{ + TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ + Group: gatewayapiv1beta1.Group("gateway.networking.k8s.io"), + Kind: "Gateway", + Name: gatewayapiv1beta1.ObjectName(gwName), + }, + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, + }, + }, + }, + }, + } err := k8sClient.Create(context.Background(), rlp) Expect(err).ToNot(HaveOccurred()) // Check RLP status is available - Eventually(func() bool { - existingRLP := &kuadrantv1beta2.RateLimitPolicy{} - err := k8sClient.Get(context.Background(), rlpKey, existingRLP) - if err != nil { - return false - } - if !meta.IsStatusConditionTrue(existingRLP.Status.Conditions, "Available") { - return false - } - - return true - }, time.Minute, 5*time.Second).Should(BeTrue()) + rlpKey := client.ObjectKey{Name: rlpName, Namespace: testNamespace} + Eventually(testRLPIsAvailable(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) // Check Gateway direct back reference gwKey := client.ObjectKeyFromObject(gateway) @@ -453,3 +750,18 @@ var _ = Describe("RateLimitPolicy controller", func() { }) }) }) + +func testRLPIsAvailable(rlpKey client.ObjectKey) func() bool { + return func() bool { + existingRLP := &kuadrantv1beta2.RateLimitPolicy{} + err := k8sClient.Get(context.Background(), rlpKey, existingRLP) + if err != nil { + return false + } + if !meta.IsStatusConditionTrue(existingRLP.Status.Conditions, "Available") { + return false + } + + return true + } +} diff --git a/pkg/common/common.go b/pkg/common/common.go index 8039ef203..f8ac965fa 100644 --- a/pkg/common/common.go +++ b/pkg/common/common.go @@ -47,6 +47,10 @@ type KuadrantPolicy interface { GetRulesHostnames() []string } +func Ptr[T any](t T) *T { + return &t +} + // FetchEnv fetches the value of the environment variable with the specified key, // or returns the default value if the variable is not found or has an empty value. // If an error occurs during the lookup, the function returns the default value. From 3add050e45c501f86c4d6bcf9040e5c56e08a274 Mon Sep 17 00:00:00 2001 From: Guilherme Cassolato Date: Sun, 25 Jun 2023 10:22:14 +0200 Subject: [PATCH 03/15] generate wasm rules only for routeSelector-filtered hostnames that belong to the targeted route --- pkg/common/common.go | 16 +++++++++ pkg/common/common_test.go | 62 +++++++++++++++++++++++++++++++++ pkg/rlptools/wasm_utils.go | 6 ++-- pkg/rlptools/wasm_utils_test.go | 8 +++++ 4 files changed, 90 insertions(+), 2 deletions(-) diff --git a/pkg/common/common.go b/pkg/common/common.go index f8ac965fa..4330caeef 100644 --- a/pkg/common/common.go +++ b/pkg/common/common.go @@ -122,6 +122,22 @@ func Intersect[T comparable](slice1, slice2 []T) bool { return false } +func Intersection[T comparable](slice1, slice2 []T) []T { + smallerSlice := slice1 + largerSlice := slice2 + if len(slice1) > len(slice2) { + smallerSlice = slice2 + largerSlice = slice1 + } + var result []T + for _, item := range smallerSlice { + if Contains(largerSlice, item) { + result = append(result, item) + } + } + return result +} + func Find[T any](slice []T, match func(T) bool) (*T, bool) { for _, item := range slice { if match(item) { diff --git a/pkg/common/common_test.go b/pkg/common/common_test.go index ebea4a5d2..6ce458ed5 100644 --- a/pkg/common/common_test.go +++ b/pkg/common/common_test.go @@ -456,6 +456,68 @@ func TestIntersectWithInts(t *testing.T) { } } +func TestIntersection(t *testing.T) { + testCases := []struct { + name string + slice1 []string + slice2 []string + expected []string + }{ + { + name: "when slice1 and slice2 have one common item then return that item", + slice1: []string{"test-gw1", "test-gw2"}, + slice2: []string{"test-gw1", "test-gw3", "test-gw4"}, + expected: []string{"test-gw1"}, + }, + { + name: "when slice1 and slice2 have no common item then return nil", + slice1: []string{"test-gw1", "test-gw2"}, + slice2: []string{"test-gw3", "test-gw4"}, + expected: nil, + }, + { + name: "when slice1 is empty then return nil", + slice1: []string{}, + slice2: []string{"test-gw3", "test-gw4"}, + expected: nil, + }, + { + name: "when slice2 is empty then return nil", + slice1: []string{"test-gw1", "test-gw2"}, + slice2: []string{}, + expected: nil, + }, + { + name: "when both slices are empty then return nil", + slice1: []string{}, + slice2: []string{}, + expected: nil, + }, + { + name: "when slice1 is nil then return nil", + slice2: []string{"test-gw3", "test-gw4"}, + expected: nil, + }, + { + name: "when slice2 is nil then return nil", + slice1: []string{"test-gw1", "test-gw2"}, + expected: nil, + }, + { + name: "when both slices are nil then return nil", + expected: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if r := Intersection(tc.slice1, tc.slice2); !reflect.DeepEqual(r, tc.expected) { + t.Errorf("expected=%v; got=%v", tc.expected, r) + } + }) + } +} + func TestMap(t *testing.T) { slice1 := []int{1, 2, 3, 4} f1 := func(x int) int { return x + 1 } diff --git a/pkg/rlptools/wasm_utils.go b/pkg/rlptools/wasm_utils.go index bb4630a3f..34c8a48c1 100644 --- a/pkg/rlptools/wasm_utils.go +++ b/pkg/rlptools/wasm_utils.go @@ -65,8 +65,10 @@ func conditionsFromLimit(limit *kuadrantv1beta2.Limit, route *gatewayapiv1beta1. // build conditions from the rules selected by the route selectors for _, routeSelector := range limit.RouteSelectors { for _, rule := range HTTPRouteRulesFromRouteSelector(routeSelector, route) { - hostnames := routeSelector.Hostnames // FIXME(guicassolato): it should be the intersection between routeSelector.Hostnames and route.Spec.Hostnames - if len(hostnames) == 0 { + var hostnames []gatewayapiv1beta1.Hostname + if len(routeSelector.Hostnames) > 0 { + hostnames = common.Intersection(routeSelector.Hostnames, route.Spec.Hostnames) + } else { hostnames = route.Spec.Hostnames } routeConditions = append(routeConditions, conditionsFromRule(rule, hostnames)...) diff --git a/pkg/rlptools/wasm_utils_test.go b/pkg/rlptools/wasm_utils_test.go index 5717c3a42..f1c5206a1 100644 --- a/pkg/rlptools/wasm_utils_test.go +++ b/pkg/rlptools/wasm_utils_test.go @@ -58,6 +58,14 @@ func TestWasmRules(t *testing.T) { Unit: kuadrantv1beta2.TimeUnit("minute"), }, }, + RouteSelectors: []kuadrantv1beta2.RouteSelector{ + { + Hostnames: []gatewayapiv1beta1.Hostname{ + "*.example.com", + "myapp.apps.example.com", // ignored + }, + }, + }, }, }, }, From eb0fa8d2a08f465cea44324c74214a0eeb03ef78 Mon Sep 17 00:00:00 2001 From: Guilherme Cassolato Date: Wed, 28 Jun 2023 11:01:31 +0200 Subject: [PATCH 04/15] fix: pattern expression for hostnames with the right operator - introduces 'endswith' pattern operator --- api/v1beta2/ratelimitpolicy_types.go | 3 +- .../kuadrant.io_ratelimitpolicies.yaml | 1 + .../bases/kuadrant.io_ratelimitpolicies.yaml | 1 + .../ratelimitpolicy_controller_test.go | 52 ++++++++----------- pkg/rlptools/wasm_utils.go | 17 +++++- pkg/rlptools/wasm_utils_test.go | 4 +- 6 files changed, 42 insertions(+), 36 deletions(-) diff --git a/api/v1beta2/ratelimitpolicy_types.go b/api/v1beta2/ratelimitpolicy_types.go index 0c659bf87..fb50f7faf 100644 --- a/api/v1beta2/ratelimitpolicy_types.go +++ b/api/v1beta2/ratelimitpolicy_types.go @@ -40,13 +40,14 @@ import ( // +kubebuilder:validation:MaxLength=253 type ContextSelector string -// +kubebuilder:validation:Enum:=eq;neq;startswith;incl;excl;matches +// +kubebuilder:validation:Enum:=eq;neq;startswith;endswith;incl;excl;matches type WhenConditionOperator string const ( EqualOperator WhenConditionOperator = "eq" NotEqualOperator WhenConditionOperator = "neq" StartsWithOperator WhenConditionOperator = "startswith" + EndsWithOperator WhenConditionOperator = "endswith" IncludeOperator WhenConditionOperator = "incl" ExcludeOperator WhenConditionOperator = "excl" MatchesOperator WhenConditionOperator = "matches" diff --git a/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml b/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml index 2eeebd93a..af8a4c3dc 100644 --- a/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml +++ b/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml @@ -270,6 +270,7 @@ spec: - eq - neq - startswith + - endswith - incl - excl - matches diff --git a/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml b/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml index be4b708de..1c398ed41 100644 --- a/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml +++ b/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml @@ -269,6 +269,7 @@ spec: - eq - neq - startswith + - endswith - incl - excl - matches diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index 9f6cbf0b5..0c8f3c6d3 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -246,8 +246,8 @@ var _ = Describe("RateLimitPolicy controller", func() { }, { Selector: "request.host", - Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), - Value: "*.example.com", + Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), + Value: ".example.com", }, }, }, @@ -281,7 +281,7 @@ var _ = Describe("RateLimitPolicy controller", func() { It("Creates the correct WasmPlugin for a complex HTTPRoute and a RateLimitPolicy", func() { // create httproute - httpRoute := testBuildBasicHttpRoute(routeName, gwName, testNamespace, []string{"*.toystore.acme.com", "*.toystore.io"}) + httpRoute := testBuildBasicHttpRoute(routeName, gwName, testNamespace, []string{"*.toystore.acme.com", "api.toystore.io"}) httpRoute.Spec.Rules = []gatewayapiv1beta1.HTTPRouteRule{ { Matches: []gatewayapiv1beta1.HTTPRouteMatch{ @@ -422,8 +422,8 @@ var _ = Describe("RateLimitPolicy controller", func() { }, { Selector: "request.host", - Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), - Value: "*.toystore.acme.com", + Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), + Value: ".toystore.acme.com", }, { Selector: "auth.identity.group", @@ -446,8 +446,8 @@ var _ = Describe("RateLimitPolicy controller", func() { }, { Selector: "request.host", - Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), - Value: "*.toystore.acme.com", + Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), + Value: ".toystore.acme.com", }, { Selector: "auth.identity.group", @@ -482,8 +482,8 @@ var _ = Describe("RateLimitPolicy controller", func() { }, { Selector: "request.host", - Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), - Value: "*.toystore.acme.com", + Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), + Value: ".toystore.acme.com", }, }, }, @@ -497,7 +497,7 @@ var _ = Describe("RateLimitPolicy controller", func() { { Selector: "request.host", Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), - Value: "*.toystore.io", + Value: "api.toystore.io", }, }, }, @@ -513,12 +513,12 @@ var _ = Describe("RateLimitPolicy controller", func() { })) // check wasm rlp config for the 2nd hostname wasmRLP, found = common.Find(existingWASMConfig.RateLimitPolicies, func(wasmRLP wasm.RateLimitPolicy) bool { - return wasmRLP.Name == "*.toystore.io" + return wasmRLP.Name == "api.toystore.io" }) Expect(found).To(BeTrue()) - Expect(wasmRLP.Domain).To(Equal(common.MarshallNamespace(client.ObjectKeyFromObject(gateway), "*.toystore.io"))) + Expect(wasmRLP.Domain).To(Equal(common.MarshallNamespace(client.ObjectKeyFromObject(gateway), "api.toystore.io"))) Expect(wasmRLP.Service).To(Equal(common.KuadrantRateLimitClusterName)) - Expect(wasmRLP.Hostnames).To(Equal([]string{"*.toystore.io"})) + Expect(wasmRLP.Hostnames).To(Equal([]string{"api.toystore.io"})) Expect(wasmRLP.Rules).To(ContainElement(wasm.Rule{ // FIXME(guicassolato): this entire rule should not be generated Conditions: []wasm.Condition{ { @@ -535,8 +535,8 @@ var _ = Describe("RateLimitPolicy controller", func() { }, { Selector: "request.host", - Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), - Value: "*.toystore.acme.com", + Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), + Value: ".toystore.acme.com", }, { Selector: "auth.identity.group", @@ -559,8 +559,8 @@ var _ = Describe("RateLimitPolicy controller", func() { }, { Selector: "request.host", - Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), - Value: "*.toystore.acme.com", + Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), + Value: ".toystore.acme.com", }, { Selector: "auth.identity.group", @@ -595,8 +595,8 @@ var _ = Describe("RateLimitPolicy controller", func() { }, { Selector: "request.host", - Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), - Value: "*.toystore.acme.com", + Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), + Value: ".toystore.acme.com", }, }, }, @@ -610,7 +610,7 @@ var _ = Describe("RateLimitPolicy controller", func() { { Selector: "request.host", Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), - Value: "*.toystore.io", + Value: "api.toystore.io", }, }, }, @@ -713,17 +713,7 @@ var _ = Describe("RateLimitPolicy controller", func() { Hostnames: []string{"*"}, Rules: []wasm.Rule{ { - Conditions: []wasm.Condition{ - { - AllOf: []wasm.PatternExpression{ - { - Selector: "request.host", - Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), - Value: "*", - }, - }, - }, - }, + Conditions: nil, Data: []wasm.DataItem{ { Static: &wasm.StaticSpec{ diff --git a/pkg/rlptools/wasm_utils.go b/pkg/rlptools/wasm_utils.go index 34c8a48c1..f822255d7 100644 --- a/pkg/rlptools/wasm_utils.go +++ b/pkg/rlptools/wasm_utils.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "reflect" + "strings" _struct "github.com/golang/protobuf/ptypes/struct" istioclientgoextensionv1alpha1 "istio.io/client-go/pkg/apis/extensions/v1alpha1" @@ -114,6 +115,9 @@ func conditionsFromLimit(limit *kuadrantv1beta2.Limit, route *gatewayapiv1beta1. func conditionsFromRule(rule gatewayapiv1beta1.HTTPRouteRule, hostnames []gatewayapiv1beta1.Hostname) (conditions []wasm.Condition) { if len(rule.Matches) == 0 { for _, hostname := range hostnames { + if hostname == "*" { + continue + } condition := wasm.Condition{AllOf: []wasm.PatternExpression{patternExpresionFromHostname(hostname)}} conditions = append(conditions, condition) } @@ -125,6 +129,9 @@ func conditionsFromRule(rule gatewayapiv1beta1.HTTPRouteRule, hostnames []gatewa if len(hostnames) > 0 { for _, hostname := range hostnames { + if hostname == "*" { + continue + } mergedCondition := condition mergedCondition.AllOf = append(mergedCondition.AllOf, patternExpresionFromHostname(hostname)) conditions = append(conditions, mergedCondition) @@ -186,10 +193,16 @@ func patternExpresionFromMethod(method gatewayapiv1beta1.HTTPMethod) wasm.Patter } func patternExpresionFromHostname(hostname gatewayapiv1beta1.Hostname) wasm.PatternExpression { + value := string(hostname) + operator := "eq" + if strings.HasPrefix(value, "*.") { + operator = "endswith" + value = value[1:] + } return wasm.PatternExpression{ Selector: "request.host", - Operator: "eq", // FIXME(guicassolato): won't work for wildcard domains - Value: string(hostname), + Operator: wasm.PatternOperator(operator), + Value: string(value), } } diff --git a/pkg/rlptools/wasm_utils_test.go b/pkg/rlptools/wasm_utils_test.go index f1c5206a1..13d98b88e 100644 --- a/pkg/rlptools/wasm_utils_test.go +++ b/pkg/rlptools/wasm_utils_test.go @@ -87,8 +87,8 @@ func TestWasmRules(t *testing.T) { }, { Selector: "request.host", - Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), - Value: "*.example.com", + Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), + Value: ".example.com", }, }, }, From eb89b8d60e9a1576580d72bd661837749fa17d50 Mon Sep 17 00:00:00 2001 From: Guilherme Cassolato Date: Thu, 6 Jul 2023 14:53:50 +0200 Subject: [PATCH 05/15] fix: compute wasm rules only for the applicable hostnames MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- controllers/ratelimitpolicy_limits.go | 2 +- controllers/ratelimitpolicy_wasm_plugins.go | 28 +- pkg/common/common.go | 24 + pkg/common/common_test.go | 48 ++ pkg/common/gatewayapi_utils.go | 6 +- pkg/rlptools/route_selectors.go | 7 +- pkg/rlptools/route_selectors_test.go | 3 +- pkg/rlptools/wasm_utils.go | 72 +-- pkg/rlptools/wasm_utils_test.go | 473 ++++++++++++++++++-- 9 files changed, 566 insertions(+), 97 deletions(-) diff --git a/controllers/ratelimitpolicy_limits.go b/controllers/ratelimitpolicy_limits.go index f753c78e7..900cd8db9 100644 --- a/controllers/ratelimitpolicy_limits.go +++ b/controllers/ratelimitpolicy_limits.go @@ -155,7 +155,7 @@ func (r *RateLimitPolicyReconciler) gatewayLimits(ctx context.Context, limits["*"] = append(limits["*"], rlptools.ReadLimitsFromRLP(gwRLP)...) } else { for _, gwHostname := range gw.Hostnames() { - limits[gwHostname] = append(limits[gwHostname], rlptools.ReadLimitsFromRLP(gwRLP)...) + limits[string(gwHostname)] = append(limits[string(gwHostname)], rlptools.ReadLimitsFromRLP(gwRLP)...) } } } diff --git a/controllers/ratelimitpolicy_wasm_plugins.go b/controllers/ratelimitpolicy_wasm_plugins.go index 33b897f1b..02e88bee7 100644 --- a/controllers/ratelimitpolicy_wasm_plugins.go +++ b/controllers/ratelimitpolicy_wasm_plugins.go @@ -147,21 +147,22 @@ func (r *RateLimitPolicyReconciler) wasmPluginConfig(ctx context.Context, gw com wasmRulesByDomain := make(rlptools.WasmRulesByDomain) var gwWasmRules []wasm.Rule + gwHostnames := gw.Hostnames() + if len(gwHostnames) == 0 { + gwHostnames = []gatewayapiv1beta1.Hostname{"*"} + } + if gwRLP != nil { - hostnames := gw.Hostnames() - if len(hostnames) == 0 { - hostnames = []string{"*"} - } // FIXME(guicassolato): this is a hack until we start going through all the httproutes that are children of the gateway and build the rules for each httproute route := &gatewayapiv1beta1.HTTPRoute{ Spec: gatewayapiv1beta1.HTTPRouteSpec{ - Hostnames: common.Map(hostnames, func(h string) gatewayapiv1beta1.Hostname { return gatewayapiv1beta1.Hostname(h) }), + Hostnames: gwHostnames, Rules: []gatewayapiv1beta1.HTTPRouteRule{{}}, }, } - gwWasmRules = rlptools.WasmRules(gwRLP, route) // FIXME(guicassolato): this is not correct. We need to go through all the httproutes that are children of the gateway and build the rules for each httproute instead - for _, gwHostname := range hostnames { - wasmRulesByDomain[gwHostname] = append(wasmRulesByDomain[gwHostname], gwWasmRules...) + gwWasmRules = rlptools.WasmRules(gwRLP, route, gwHostnames) // FIXME(guicassolato): this is not correct. We need to go through all the httproutes that are children of the gateway and build the rules for each httproute instead + for _, gwHostname := range gwHostnames { + wasmRulesByDomain[string(gwHostname)] = append(wasmRulesByDomain[string(gwHostname)], gwWasmRules...) } } @@ -170,10 +171,17 @@ func (r *RateLimitPolicyReconciler) wasmPluginConfig(ctx context.Context, gw com if err != nil { return nil, err } + + // filter the route hostnames to only the ones that are children of the gateway + hostnames := common.FilterValidSubdomains(gwHostnames, httpRoute.Spec.Hostnames) + if len(hostnames) == 0 { // should never happen + hostnames = gwHostnames + } + // gateways limits merged with the route level limits - wasmRules := append(rlptools.WasmRules(httpRouteRLP, httpRoute), gwWasmRules...) // FIXME(guicassolato): there will be no need to merge gwRLP rules when targeting a gateway == shortcut for targeting all the routes of a gateway + wasmRules := append(rlptools.WasmRules(httpRouteRLP, httpRoute, hostnames), gwWasmRules...) // FIXME(guicassolato): there will be no need to merge gwRLP rules when targeting a gateway == shortcut for targeting all the routes of a gateway // routeLimits referenced by multiple hostnames - for _, hostname := range httpRoute.Spec.Hostnames { + for _, hostname := range hostnames { wasmRulesByDomain[string(hostname)] = append(wasmRulesByDomain[string(hostname)], wasmRules...) } } diff --git a/pkg/common/common.go b/pkg/common/common.go index 4330caeef..464a72a2c 100644 --- a/pkg/common/common.go +++ b/pkg/common/common.go @@ -156,6 +156,17 @@ func Map[T, U any](slice []T, f func(T) U) []U { return arr } +// Filter filters the input slice using the given predicate function and returns a new slice with the results. +func Filter[T any](slice []T, f func(T) bool) []T { + arr := make([]T, 0) + for _, e := range slice { + if f(e) { + arr = append(arr, e) + } + } + return arr +} + // SliceCopy copies the elements from the input slice into the output slice, and returns the output slice. func SliceCopy[T any](s1 []T) []T { s2 := make([]T, len(s1)) @@ -273,3 +284,16 @@ func ValidSubdomains(domains, subdomains []string) (bool, string) { } return true, "" } + +// FilterValidSubdomains returns every subdomain that is a subset of at least one of the (super) domains specified in the first argument. +func FilterValidSubdomains(domains, subdomains []gatewayapiv1beta1.Hostname) []gatewayapiv1beta1.Hostname { + arr := make([]gatewayapiv1beta1.Hostname, 0) + for _, subsubdomain := range subdomains { + if _, found := Find(domains, func(domain gatewayapiv1beta1.Hostname) bool { + return Name(subsubdomain).SubsetOf(Name(domain)) + }); found { + arr = append(arr, subsubdomain) + } + } + return arr +} diff --git a/pkg/common/common_test.go b/pkg/common/common_test.go index 6ce458ed5..50ba540d7 100644 --- a/pkg/common/common_test.go +++ b/pkg/common/common_test.go @@ -938,3 +938,51 @@ func TestHostnamesToStrings(t *testing.T) { }) } } + +func TestFilterValidSubdomains(t *testing.T) { + testCases := []struct { + name string + domains []gatewayapiv1beta1.Hostname + subdomains []gatewayapiv1beta1.Hostname + expected []gatewayapiv1beta1.Hostname + }{ + { + name: "when all subdomains are valid", + domains: []gatewayapiv1beta1.Hostname{"my-app.apps.io", "*.acme.com"}, + subdomains: []gatewayapiv1beta1.Hostname{"toystore.acme.com", "my-app.apps.io", "carstore.acme.com"}, + expected: []gatewayapiv1beta1.Hostname{"toystore.acme.com", "my-app.apps.io", "carstore.acme.com"}, + }, + { + name: "when some subdomains are valid and some are not", + domains: []gatewayapiv1beta1.Hostname{"my-app.apps.io", "*.acme.com"}, + subdomains: []gatewayapiv1beta1.Hostname{"toystore.acme.com", "my-app.apps.io", "other-app.apps.io"}, + expected: []gatewayapiv1beta1.Hostname{"toystore.acme.com", "my-app.apps.io"}, + }, + { + name: "when none of subdomains are valid", + domains: []gatewayapiv1beta1.Hostname{"my-app.apps.io", "*.acme.com"}, + subdomains: []gatewayapiv1beta1.Hostname{"other-app.apps.io"}, + expected: []gatewayapiv1beta1.Hostname{}, + }, + { + name: "when the set of super domains is empty", + domains: []gatewayapiv1beta1.Hostname{}, + subdomains: []gatewayapiv1beta1.Hostname{"toystore.acme.com"}, + expected: []gatewayapiv1beta1.Hostname{}, + }, + { + name: "when the set of subdomains is empty", + domains: []gatewayapiv1beta1.Hostname{"my-app.apps.io", "*.acme.com"}, + subdomains: []gatewayapiv1beta1.Hostname{}, + expected: []gatewayapiv1beta1.Hostname{}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if r := FilterValidSubdomains(tc.domains, tc.subdomains); !reflect.DeepEqual(r, tc.expected) { + t.Errorf("expected=%v; got=%v", tc.expected, r) + } + }) + } +} diff --git a/pkg/common/gatewayapi_utils.go b/pkg/common/gatewayapi_utils.go index 45f7b113d..b65e59464 100644 --- a/pkg/common/gatewayapi_utils.go +++ b/pkg/common/gatewayapi_utils.go @@ -485,15 +485,15 @@ func (g GatewayWrapper) DeletePolicy(policyKey client.ObjectKey) bool { } // Hostnames builds a list of hostnames from the listeners. -func (g GatewayWrapper) Hostnames() []string { - hostnames := make([]string, 0) +func (g GatewayWrapper) Hostnames() []gatewayapiv1beta1.Hostname { + hostnames := make([]gatewayapiv1beta1.Hostname, 0) if g.Gateway == nil { return hostnames } for idx := range g.Spec.Listeners { if g.Spec.Listeners[idx].Hostname != nil { - hostnames = append(hostnames, string(*g.Spec.Listeners[idx].Hostname)) + hostnames = append(hostnames, *g.Spec.Listeners[idx].Hostname) } } diff --git a/pkg/rlptools/route_selectors.go b/pkg/rlptools/route_selectors.go index f8dc7e992..1172cd6cc 100644 --- a/pkg/rlptools/route_selectors.go +++ b/pkg/rlptools/route_selectors.go @@ -7,9 +7,12 @@ import ( "github.com/kuadrant/kuadrant-operator/pkg/common" ) -func HTTPRouteRulesFromRouteSelector(routeSelector kuadrantv1beta2.RouteSelector, route *gatewayapiv1beta1.HTTPRoute) []gatewayapiv1beta1.HTTPRouteRule { +func HTTPRouteRulesFromRouteSelector(routeSelector kuadrantv1beta2.RouteSelector, route *gatewayapiv1beta1.HTTPRoute, hostnames []gatewayapiv1beta1.Hostname) []gatewayapiv1beta1.HTTPRouteRule { rulesIndices := make(map[int]gatewayapiv1beta1.HTTPRouteRule, 0) - if len(routeSelector.Hostnames) > 0 && !common.Intersect(routeSelector.Hostnames, route.Spec.Hostnames) { + if len(hostnames) == 0 { + hostnames = route.Spec.Hostnames + } + if len(routeSelector.Hostnames) > 0 && !common.Intersect(routeSelector.Hostnames, hostnames) { return nil } if len(routeSelector.Matches) == 0 { diff --git a/pkg/rlptools/route_selectors_test.go b/pkg/rlptools/route_selectors_test.go index 41eaf609d..19587f5a8 100644 --- a/pkg/rlptools/route_selectors_test.go +++ b/pkg/rlptools/route_selectors_test.go @@ -79,6 +79,7 @@ func TestRouteSelectors(t *testing.T) { name string routeSelector kuadrantv1beta2.RouteSelector route *gatewayapiv1beta1.HTTPRoute + hostnames []gatewayapiv1beta1.Hostname expected []gatewayapiv1beta1.HTTPRouteRule }{ { @@ -200,7 +201,7 @@ func TestRouteSelectors(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - rules := HTTPRouteRulesFromRouteSelector(tc.routeSelector, tc.route) + rules := HTTPRouteRulesFromRouteSelector(tc.routeSelector, tc.route, tc.hostnames) rulesToStringSlice := func(rules []gatewayapiv1beta1.HTTPRouteRule) []string { return common.Map(common.Map(rules, common.HTTPRouteRuleToString), func(r string) string { return fmt.Sprintf("{%s}", r) }) } diff --git a/pkg/rlptools/wasm_utils.go b/pkg/rlptools/wasm_utils.go index f822255d7..77d6f5903 100644 --- a/pkg/rlptools/wasm_utils.go +++ b/pkg/rlptools/wasm_utils.go @@ -21,8 +21,11 @@ var ( WASMFilterImageURL = common.FetchEnv("RELATED_IMAGE_WASMSHIM", "oci://quay.io/kuadrant/wasm-shim:latest") ) -// WasmRules computes WASM rules from the policy and the targeted Route (which can be nil when a gateway is targeted) -func WasmRules(rlp *kuadrantv1beta2.RateLimitPolicy, route *gatewayapiv1beta1.HTTPRoute) []wasm.Rule { +// WasmRules computes WASM rules from the policy and the targeted route. +// Pass a list of hostnames to ensure that only rules targeting these hostnames are computed; +// otherwise, the hostnames specified in the route will set the boundaries for the rules. +// This is useful to compute rules for a specific gateway. +func WasmRules(rlp *kuadrantv1beta2.RateLimitPolicy, route *gatewayapiv1beta1.HTTPRoute, hostnames []gatewayapiv1beta1.Hostname) []wasm.Rule { rules := make([]wasm.Rule, 0) if rlp == nil { return rules @@ -31,21 +34,23 @@ func WasmRules(rlp *kuadrantv1beta2.RateLimitPolicy, route *gatewayapiv1beta1.HT for limitName, limit := range rlp.Spec.Limits { // 1 RLP limit <---> 1 WASM rule limitFullName := FullLimitName(rlp, limitName) - rule := ruleFromLimit(limitFullName, &limit, route) - rules = append(rules, rule) + rule, err := ruleFromLimit(limitFullName, &limit, route, hostnames) + if err == nil { + rules = append(rules, rule) + } else { + // TODO(guicassolato): log the error + } } return rules } -func ruleFromLimit(limitFullName string, limit *kuadrantv1beta2.Limit, route *gatewayapiv1beta1.HTTPRoute) wasm.Rule { - if limit == nil { - return wasm.Rule{} - } - +func ruleFromLimit(limitFullName string, limit *kuadrantv1beta2.Limit, route *gatewayapiv1beta1.HTTPRoute, hostnames []gatewayapiv1beta1.Hostname) (wasm.Rule, error) { rule := wasm.Rule{} - if conditions := conditionsFromLimit(limit, route); conditions != nil { + if conditions, err := conditionsFromLimit(limit, route, hostnames); err != nil { + return rule, err + } else { rule.Conditions = conditions } @@ -53,38 +58,43 @@ func ruleFromLimit(limitFullName string, limit *kuadrantv1beta2.Limit, route *ga rule.Data = data } - return rule + return rule, nil } -func conditionsFromLimit(limit *kuadrantv1beta2.Limit, route *gatewayapiv1beta1.HTTPRoute) []wasm.Condition { +func conditionsFromLimit(limit *kuadrantv1beta2.Limit, route *gatewayapiv1beta1.HTTPRoute, hostnames []gatewayapiv1beta1.Hostname) ([]wasm.Condition, error) { + if limit == nil { + return nil, errors.New("limit should not be nil") + } + routeConditions := make([]wasm.Condition, 0) - if limit == nil { - return routeConditions + if len(hostnames) == 0 { + hostnames = route.Spec.Hostnames } - // build conditions from the rules selected by the route selectors - for _, routeSelector := range limit.RouteSelectors { - for _, rule := range HTTPRouteRulesFromRouteSelector(routeSelector, route) { - var hostnames []gatewayapiv1beta1.Hostname - if len(routeSelector.Hostnames) > 0 { - hostnames = common.Intersection(routeSelector.Hostnames, route.Spec.Hostnames) - } else { - hostnames = route.Spec.Hostnames + if len(limit.RouteSelectors) > 0 { + // build conditions from the rules selected by the route selectors + for _, routeSelector := range limit.RouteSelectors { + for _, rule := range HTTPRouteRulesFromRouteSelector(routeSelector, route, hostnames) { + hostnamesForCondition := hostnames + if len(routeSelector.Hostnames) > 0 { + hostnamesForCondition = common.Intersection(routeSelector.Hostnames, hostnames) + } + routeConditions = append(routeConditions, conditionsFromRule(rule, hostnamesForCondition)...) } - routeConditions = append(routeConditions, conditionsFromRule(rule, hostnames)...) } - } - - // build conditions from the route if no route selectors are defined - if len(routeConditions) == 0 { + if len(routeConditions) == 0 { + return nil, errors.New("cannot match any route rules, check for invalid route selectors in the policy") + } + } else { + // build conditions from the route if no route selectors are defined for _, rule := range route.Spec.Rules { - routeConditions = append(routeConditions, conditionsFromRule(rule, route.Spec.Hostnames)...) + routeConditions = append(routeConditions, conditionsFromRule(rule, hostnames)...) } } if len(limit.When) == 0 { - return routeConditions + return routeConditions, nil } if len(routeConditions) > 0 { @@ -97,7 +107,7 @@ func conditionsFromLimit(limit *kuadrantv1beta2.Limit, route *gatewayapiv1beta1. mergedConditions[idx] = mergedCondition } } - return mergedConditions + return mergedConditions, nil } // build conditions only from the 'when' field @@ -105,7 +115,7 @@ func conditionsFromLimit(limit *kuadrantv1beta2.Limit, route *gatewayapiv1beta1. for idx, when := range limit.When { whenConditions[idx] = wasm.Condition{AllOf: []wasm.PatternExpression{patternExpresionFromWhen(when)}} } - return whenConditions + return whenConditions, nil } // conditionsFromRule builds a list of conditions from a rule and a list of hostnames diff --git a/pkg/rlptools/wasm_utils_test.go b/pkg/rlptools/wasm_utils_test.go index 13d98b88e..0a7838d08 100644 --- a/pkg/rlptools/wasm_utils_test.go +++ b/pkg/rlptools/wasm_utils_test.go @@ -3,14 +3,16 @@ package rlptools import ( + "encoding/json" "reflect" "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" gatewayapiv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1" - "github.com/google/go-cmp/cmp" kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" + "github.com/kuadrant/kuadrant-operator/pkg/common" "github.com/kuadrant/kuadrant-operator/pkg/rlptools/wasm" ) @@ -21,6 +23,7 @@ func TestWasmRules(t *testing.T) { Spec: gatewayapiv1beta1.HTTPRouteSpec{ Hostnames: []gatewayapiv1beta1.Hostname{ "*.example.com", + "*.apps.example.internal", }, Rules: []gatewayapiv1beta1.HTTPRouteRule{ { @@ -38,79 +41,451 @@ func TestWasmRules(t *testing.T) { }, } - t.Run("minimal RLP", func(subT *testing.T) { - rlp := &kuadrantv1beta2.RateLimitPolicy{ - TypeMeta: metav1.TypeMeta{ - Kind: "RateLimitPolicy", - APIVersion: kuadrantv1beta2.GroupVersion.String(), - }, + catchAllHTTPRoute := &gatewayapiv1beta1.HTTPRoute{ + Spec: gatewayapiv1beta1.HTTPRouteSpec{ + Hostnames: []gatewayapiv1beta1.Hostname{"*"}, + }, + } + + rlp := func(name string, limits map[string]kuadrantv1beta2.Limit) *kuadrantv1beta2.RateLimitPolicy { + return &kuadrantv1beta2.RateLimitPolicy{ ObjectMeta: metav1.ObjectMeta{ - Name: "rlpA", - Namespace: "nsA", + Name: name, + Namespace: "my-app", }, Spec: kuadrantv1beta2.RateLimitPolicySpec{ - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, - Duration: 3, - Unit: kuadrantv1beta2.TimeUnit("minute"), + Limits: limits, + }, + } + } + + // a simple 50rps counter, for convinience, to be used in tests + counter50rps := kuadrantv1beta2.Rate{ + Limit: 50, + Duration: 1, + Unit: kuadrantv1beta2.TimeUnit("second"), + } + + testCases := []struct { + name string + rlp *kuadrantv1beta2.RateLimitPolicy + route *gatewayapiv1beta1.HTTPRoute + hostnames []gatewayapiv1beta1.Hostname + expectedRules []wasm.Rule + }{ + { + name: "minimal RLP", + rlp: rlp("minimal", map[string]kuadrantv1beta2.Limit{ + "50rps": { + Rates: []kuadrantv1beta2.Rate{counter50rps}, + }, + }), + route: httpRoute, + expectedRules: []wasm.Rule{ + { + Conditions: []wasm.Condition{ + { + AllOf: []wasm.PatternExpression{ + { + Selector: "request.url_path", + Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), + Value: "/toy", + }, + { + Selector: "request.method", + Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), + Value: "GET", + }, + { + Selector: "request.host", + Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), + Value: ".example.com", + }, }, }, - RouteSelectors: []kuadrantv1beta2.RouteSelector{ - { - Hostnames: []gatewayapiv1beta1.Hostname{ - "*.example.com", - "myapp.apps.example.com", // ignored + { + AllOf: []wasm.PatternExpression{ + { + Selector: "request.url_path", + Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), + Value: "/toy", + }, + { + Selector: "request.method", + Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), + Value: "GET", + }, + { + Selector: "request.host", + Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), + Value: ".apps.example.internal", }, }, }, }, + Data: []wasm.DataItem{ + { + Static: &wasm.StaticSpec{ + Key: "my-app/minimal/50rps", + Value: "1", + }, + }, + }, }, }, - } - - expectedRule := wasm.Rule{ - Conditions: []wasm.Condition{ + }, + { + name: "RLP with route selector based on hostname", + rlp: rlp("my-rlp", map[string]kuadrantv1beta2.Limit{ + "50rps-for-selected-hostnames": { + Rates: []kuadrantv1beta2.Rate{counter50rps}, + RouteSelectors: []kuadrantv1beta2.RouteSelector{ + { + Hostnames: []gatewayapiv1beta1.Hostname{ + "*.example.com", + "myapp.apps.example.com", // ignored + }, + }, + }, + }, + }), + route: httpRoute, + expectedRules: []wasm.Rule{ { - AllOf: []wasm.PatternExpression{ + Conditions: []wasm.Condition{ { - Selector: "request.url_path", - Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), - Value: "/toy", + AllOf: []wasm.PatternExpression{ + { + Selector: "request.url_path", + Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), + Value: "/toy", + }, + { + Selector: "request.method", + Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), + Value: "GET", + }, + { + Selector: "request.host", + Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), + Value: ".example.com", + }, + }, }, + }, + Data: []wasm.DataItem{ { - Selector: "request.method", - Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), - Value: "GET", + Static: &wasm.StaticSpec{ + Key: "my-app/my-rlp/50rps-for-selected-hostnames", + Value: "1", + }, }, + }, + }, + }, + }, + { + name: "RLP with route selector based on http route matches (full match)", + rlp: rlp("my-rlp", map[string]kuadrantv1beta2.Limit{ + "50rps-for-selected-route": { + Rates: []kuadrantv1beta2.Rate{counter50rps}, + RouteSelectors: []kuadrantv1beta2.RouteSelector{ { - Selector: "request.host", - Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), - Value: ".example.com", + Matches: []gatewayapiv1alpha2.HTTPRouteMatch{ + { + Path: &gatewayapiv1beta1.HTTPPathMatch{ + Type: &[]gatewayapiv1beta1.PathMatchType{gatewayapiv1beta1.PathMatchPathPrefix}[0], + Value: &[]string{"/toy"}[0], + }, + Method: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethod("GET")}[0], + }, + }, + }, + }, + }, + }), + route: httpRoute, + expectedRules: []wasm.Rule{ + { + Conditions: []wasm.Condition{ + { + AllOf: []wasm.PatternExpression{ + { + Selector: "request.url_path", + Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), + Value: "/toy", + }, + { + Selector: "request.method", + Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), + Value: "GET", + }, + { + Selector: "request.host", + Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), + Value: ".example.com", + }, + }, + }, + { + AllOf: []wasm.PatternExpression{ + { + Selector: "request.url_path", + Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), + Value: "/toy", + }, + { + Selector: "request.method", + Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), + Value: "GET", + }, + { + Selector: "request.host", + Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), + Value: ".apps.example.internal", + }, + }, + }, + }, + Data: []wasm.DataItem{ + { + Static: &wasm.StaticSpec{ + Key: "my-app/my-rlp/50rps-for-selected-route", + Value: "1", + }, }, }, }, }, - Data: []wasm.DataItem{ + }, + { + name: "RLP with route selector based on http route matches (partial match)", + rlp: rlp("my-rlp", map[string]kuadrantv1beta2.Limit{ + "50rps-for-selected-path": { + Rates: []kuadrantv1beta2.Rate{counter50rps}, + RouteSelectors: []kuadrantv1beta2.RouteSelector{ + { + Matches: []gatewayapiv1alpha2.HTTPRouteMatch{ + { + Path: &gatewayapiv1beta1.HTTPPathMatch{ + Type: &[]gatewayapiv1beta1.PathMatchType{gatewayapiv1beta1.PathMatchPathPrefix}[0], + Value: &[]string{"/toy"}[0], + }, + }, + }, + }, + }, + }, + }), + route: httpRoute, + expectedRules: []wasm.Rule{ { - Static: &wasm.StaticSpec{ - Key: "nsA/rlpA/l1", - Value: "1", + Conditions: []wasm.Condition{ + { + AllOf: []wasm.PatternExpression{ + { + Selector: "request.url_path", + Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), + Value: "/toy", + }, + { + Selector: "request.method", + Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), + Value: "GET", + }, + { + Selector: "request.host", + Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), + Value: ".example.com", + }, + }, + }, + { + AllOf: []wasm.PatternExpression{ + { + Selector: "request.url_path", + Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), + Value: "/toy", + }, + { + Selector: "request.method", + Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), + Value: "GET", + }, + { + Selector: "request.host", + Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), + Value: ".apps.example.internal", + }, + }, + }, + }, + Data: []wasm.DataItem{ + { + Static: &wasm.StaticSpec{ + Key: "my-app/my-rlp/50rps-for-selected-path", + Value: "1", + }, + }, }, }, }, - } + }, + { + name: "RLP with mismatching route selectors", + rlp: rlp("my-rlp", map[string]kuadrantv1beta2.Limit{ + "50rps-for-non-existent-route": { + Rates: []kuadrantv1beta2.Rate{counter50rps}, + RouteSelectors: []kuadrantv1beta2.RouteSelector{ + { + Matches: []gatewayapiv1alpha2.HTTPRouteMatch{ + { + Method: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethod("POST")}[0], + }, + }, + }, + }, + }, + }), + route: httpRoute, + expectedRules: []wasm.Rule{}, + }, + { + name: "HTTPRouteRules without rule matches", + rlp: rlp("my-rlp", map[string]kuadrantv1beta2.Limit{ + "50rps": { + Rates: []kuadrantv1beta2.Rate{counter50rps}, + }, + }), + route: catchAllHTTPRoute, + expectedRules: []wasm.Rule{ + { + Conditions: nil, + Data: []wasm.DataItem{ + { + Static: &wasm.StaticSpec{ + Key: "my-app/my-rlp/50rps", + Value: "1", + }, + }, + }, + }, + }, + }, + { + name: "RLP for when one of the hostnames in the httproute is from a different gateway", + rlp: rlp("my-rlp", map[string]kuadrantv1beta2.Limit{ + "50rps": { + Rates: []kuadrantv1beta2.Rate{counter50rps}, + }, + }), + route: httpRoute, + hostnames: []gatewayapiv1beta1.Hostname{"*.example.com"}, // intentionally excluding "*.apps.example.internal" + expectedRules: []wasm.Rule{ + { + Conditions: []wasm.Condition{ + { + AllOf: []wasm.PatternExpression{ + { + Selector: "request.url_path", + Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), + Value: "/toy", + }, + { + Selector: "request.method", + Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), + Value: "GET", + }, + { + Selector: "request.host", + Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), + Value: ".example.com", + }, + }, + }, + }, + Data: []wasm.DataItem{ + { + Static: &wasm.StaticSpec{ + Key: "my-app/my-rlp/50rps", + Value: "1", + }, + }, + }, + }, + }, + }, + { + name: "RLP with counter qualifier", + rlp: rlp("my-rlp", map[string]kuadrantv1beta2.Limit{ + "50rps-per-username": { + Rates: []kuadrantv1beta2.Rate{counter50rps}, + Counters: []kuadrantv1beta2.ContextSelector{"auth.identity.username"}, + }, + }), + route: catchAllHTTPRoute, + expectedRules: []wasm.Rule{ + { + Conditions: nil, + Data: []wasm.DataItem{ + { + Static: &wasm.StaticSpec{ + Key: "my-app/my-rlp/50rps-per-username", + Value: "1", + }, + }, + { + Selector: &wasm.SelectorSpec{ + Selector: "auth.identity.username", + }, + }, + }, + }, + }, + }, + } - rules := WasmRules(rlp, httpRoute) - if len(rules) != 1 { - subT.Errorf("expected 1 rule, got (%d)", len(rules)) - } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + computedRules := WasmRules(tc.rlp, tc.route, tc.hostnames) - if !reflect.DeepEqual(rules[0], expectedRule) { - diff := cmp.Diff(rules[0], expectedRule) - subT.Errorf("expected rule not found: %s", diff) - } - }) + if len(tc.expectedRules) != len(computedRules) { + t.Errorf("expected %d wasm rules, got (%d)", len(tc.expectedRules), len(computedRules)) + } + + for _, expectedRule := range tc.expectedRules { + if _, ruleFound := common.Find(computedRules, func(computedRule wasm.Rule) bool { + if len(expectedRule.Conditions) != len(computedRule.Conditions) { + return false + } + + // we cannot guarantee the order of the conditions, so we need to find each one + for _, expectedCondition := range expectedRule.Conditions { + if _, conditionFound := common.Find(computedRule.Conditions, func(computedCondition wasm.Condition) bool { + return reflect.DeepEqual(expectedCondition, computedCondition) + }); !conditionFound { + return false + } + } + + if len(expectedRule.Data) != len(computedRule.Data) { + return false + } + + // unlike the conditions, we can guarantee the order of the data items + for i := range expectedRule.Data { + if !reflect.DeepEqual(expectedRule.Data[i], computedRule.Data[i]) { + return false + } + } + + return true + }); !ruleFound { + expectedRuleJSON, _ := json.Marshal(expectedRule) + computedRulesJSON, _ := json.Marshal(computedRules) + t.Errorf("cannot find expected wasm rule: %s in %s", string(expectedRuleJSON), string(computedRulesJSON)) + } + } + }) + } } From a6b54fe4d7e3208c7995027daf79d6b301bd92f7 Mon Sep 17 00:00:00 2001 From: Guilherme Cassolato Date: Thu, 6 Jul 2023 20:34:18 +0200 Subject: [PATCH 06/15] do not generate wasm condition patterns for the hostname when all apply --- .../ratelimitpolicy_controller_test.go | 43 --------- pkg/rlptools/wasm_utils.go | 48 ++++++---- pkg/rlptools/wasm_utils_test.go | 93 ++----------------- 3 files changed, 39 insertions(+), 145 deletions(-) diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index 0c8f3c6d3..93b481ddc 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -244,11 +244,6 @@ var _ = Describe("RateLimitPolicy controller", func() { Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), Value: "GET", }, - { - Selector: "request.host", - Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), - Value: ".example.com", - }, }, }, }, @@ -480,25 +475,6 @@ var _ = Describe("RateLimitPolicy controller", func() { Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), Value: "/assets", }, - { - Selector: "request.host", - Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), - Value: ".toystore.acme.com", - }, - }, - }, - { // FIXME(guicassolato): this condition should not be generated - AllOf: []wasm.PatternExpression{ - { - Selector: "request.url_path", - Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), - Value: "/assets", - }, - { - Selector: "request.host", - Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), - Value: "api.toystore.io", - }, }, }, }, @@ -586,20 +562,6 @@ var _ = Describe("RateLimitPolicy controller", func() { })) Expect(wasmRLP.Rules).To(ContainElement(wasm.Rule{ Conditions: []wasm.Condition{ - { // FIXME(guicassolato): this condition should not be generated - AllOf: []wasm.PatternExpression{ - { - Selector: "request.url_path", - Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), - Value: "/assets", - }, - { - Selector: "request.host", - Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), - Value: ".toystore.acme.com", - }, - }, - }, { AllOf: []wasm.PatternExpression{ { @@ -607,11 +569,6 @@ var _ = Describe("RateLimitPolicy controller", func() { Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), Value: "/assets", }, - { - Selector: "request.host", - Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), - Value: "api.toystore.io", - }, }, }, }, diff --git a/pkg/rlptools/wasm_utils.go b/pkg/rlptools/wasm_utils.go index 77d6f5903..79a1c9da9 100644 --- a/pkg/rlptools/wasm_utils.go +++ b/pkg/rlptools/wasm_utils.go @@ -22,19 +22,23 @@ var ( ) // WasmRules computes WASM rules from the policy and the targeted route. -// Pass a list of hostnames to ensure that only rules targeting these hostnames are computed; +// Pass a list of target hostnames to ensure that only rules those are computed; // otherwise, the hostnames specified in the route will set the boundaries for the rules. // This is useful to compute rules for a specific gateway. -func WasmRules(rlp *kuadrantv1beta2.RateLimitPolicy, route *gatewayapiv1beta1.HTTPRoute, hostnames []gatewayapiv1beta1.Hostname) []wasm.Rule { +func WasmRules(rlp *kuadrantv1beta2.RateLimitPolicy, route *gatewayapiv1beta1.HTTPRoute, targetHostnames []gatewayapiv1beta1.Hostname) []wasm.Rule { rules := make([]wasm.Rule, 0) if rlp == nil { return rules } + if len(targetHostnames) == 0 { + targetHostnames = route.Spec.Hostnames + } + for limitName, limit := range rlp.Spec.Limits { // 1 RLP limit <---> 1 WASM rule limitFullName := FullLimitName(rlp, limitName) - rule, err := ruleFromLimit(limitFullName, &limit, route, hostnames) + rule, err := ruleFromLimit(limitFullName, &limit, route, targetHostnames) if err == nil { rules = append(rules, rule) } else { @@ -45,10 +49,10 @@ func WasmRules(rlp *kuadrantv1beta2.RateLimitPolicy, route *gatewayapiv1beta1.HT return rules } -func ruleFromLimit(limitFullName string, limit *kuadrantv1beta2.Limit, route *gatewayapiv1beta1.HTTPRoute, hostnames []gatewayapiv1beta1.Hostname) (wasm.Rule, error) { +func ruleFromLimit(limitFullName string, limit *kuadrantv1beta2.Limit, route *gatewayapiv1beta1.HTTPRoute, targetHostnames []gatewayapiv1beta1.Hostname) (wasm.Rule, error) { rule := wasm.Rule{} - if conditions, err := conditionsFromLimit(limit, route, hostnames); err != nil { + if conditions, err := conditionsFromLimit(limit, route, targetHostnames); err != nil { return rule, err } else { rule.Conditions = conditions @@ -61,26 +65,19 @@ func ruleFromLimit(limitFullName string, limit *kuadrantv1beta2.Limit, route *ga return rule, nil } -func conditionsFromLimit(limit *kuadrantv1beta2.Limit, route *gatewayapiv1beta1.HTTPRoute, hostnames []gatewayapiv1beta1.Hostname) ([]wasm.Condition, error) { +func conditionsFromLimit(limit *kuadrantv1beta2.Limit, route *gatewayapiv1beta1.HTTPRoute, targetHostnames []gatewayapiv1beta1.Hostname) ([]wasm.Condition, error) { if limit == nil { return nil, errors.New("limit should not be nil") } routeConditions := make([]wasm.Condition, 0) - if len(hostnames) == 0 { - hostnames = route.Spec.Hostnames - } - if len(limit.RouteSelectors) > 0 { // build conditions from the rules selected by the route selectors for _, routeSelector := range limit.RouteSelectors { - for _, rule := range HTTPRouteRulesFromRouteSelector(routeSelector, route, hostnames) { - hostnamesForCondition := hostnames - if len(routeSelector.Hostnames) > 0 { - hostnamesForCondition = common.Intersection(routeSelector.Hostnames, hostnames) - } - routeConditions = append(routeConditions, conditionsFromRule(rule, hostnamesForCondition)...) + hostnamesForConditions := hostnamesForConditions(targetHostnames, route, &routeSelector) + for _, rule := range HTTPRouteRulesFromRouteSelector(routeSelector, route, targetHostnames) { + routeConditions = append(routeConditions, conditionsFromRule(rule, hostnamesForConditions)...) } } if len(routeConditions) == 0 { @@ -89,7 +86,7 @@ func conditionsFromLimit(limit *kuadrantv1beta2.Limit, route *gatewayapiv1beta1. } else { // build conditions from the route if no route selectors are defined for _, rule := range route.Spec.Rules { - routeConditions = append(routeConditions, conditionsFromRule(rule, hostnames)...) + routeConditions = append(routeConditions, conditionsFromRule(rule, hostnamesForConditions(targetHostnames, route, nil))...) } } @@ -118,6 +115,22 @@ func conditionsFromLimit(limit *kuadrantv1beta2.Limit, route *gatewayapiv1beta1. return whenConditions, nil } +// hostnamesForConditions allows avoiding building conditions for hostnames that are excluded by the selector +// or when the hostname is irrelevant (i.e. matches all hostnames) +func hostnamesForConditions(targetHostnames []gatewayapiv1beta1.Hostname, route *gatewayapiv1beta1.HTTPRoute, routeSelector *kuadrantv1beta2.RouteSelector) []gatewayapiv1beta1.Hostname { + hostnames := targetHostnames + + if routeSelector != nil && len(routeSelector.Hostnames) > 0 { + hostnames = common.Intersection(routeSelector.Hostnames, hostnames) + } + + if common.SameElements(hostnames, targetHostnames) { + return []gatewayapiv1beta1.Hostname{"*"} + } + + return hostnames +} + // conditionsFromRule builds a list of conditions from a rule and a list of hostnames // each combination of a rule match and hostname yields one condition // rules that specify no explicit match are assumed to match all request (i.e. implicit catch-all rule) @@ -140,6 +153,7 @@ func conditionsFromRule(rule gatewayapiv1beta1.HTTPRouteRule, hostnames []gatewa if len(hostnames) > 0 { for _, hostname := range hostnames { if hostname == "*" { + conditions = append(conditions, condition) continue } mergedCondition := condition diff --git a/pkg/rlptools/wasm_utils_test.go b/pkg/rlptools/wasm_utils_test.go index 0a7838d08..1688aabdf 100644 --- a/pkg/rlptools/wasm_utils_test.go +++ b/pkg/rlptools/wasm_utils_test.go @@ -67,11 +67,11 @@ func TestWasmRules(t *testing.T) { } testCases := []struct { - name string - rlp *kuadrantv1beta2.RateLimitPolicy - route *gatewayapiv1beta1.HTTPRoute - hostnames []gatewayapiv1beta1.Hostname - expectedRules []wasm.Rule + name string + rlp *kuadrantv1beta2.RateLimitPolicy + route *gatewayapiv1beta1.HTTPRoute + targetHostnames []gatewayapiv1beta1.Hostname + expectedRules []wasm.Rule }{ { name: "minimal RLP", @@ -96,30 +96,6 @@ func TestWasmRules(t *testing.T) { Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), Value: "GET", }, - { - Selector: "request.host", - Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), - Value: ".example.com", - }, - }, - }, - { - AllOf: []wasm.PatternExpression{ - { - Selector: "request.url_path", - Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), - Value: "/toy", - }, - { - Selector: "request.method", - Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), - Value: "GET", - }, - { - Selector: "request.host", - Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), - Value: ".apps.example.internal", - }, }, }, }, @@ -220,30 +196,6 @@ func TestWasmRules(t *testing.T) { Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), Value: "GET", }, - { - Selector: "request.host", - Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), - Value: ".example.com", - }, - }, - }, - { - AllOf: []wasm.PatternExpression{ - { - Selector: "request.url_path", - Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), - Value: "/toy", - }, - { - Selector: "request.method", - Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), - Value: "GET", - }, - { - Selector: "request.host", - Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), - Value: ".apps.example.internal", - }, }, }, }, @@ -293,30 +245,6 @@ func TestWasmRules(t *testing.T) { Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), Value: "GET", }, - { - Selector: "request.host", - Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), - Value: ".example.com", - }, - }, - }, - { - AllOf: []wasm.PatternExpression{ - { - Selector: "request.url_path", - Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), - Value: "/toy", - }, - { - Selector: "request.method", - Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), - Value: "GET", - }, - { - Selector: "request.host", - Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), - Value: ".apps.example.internal", - }, }, }, }, @@ -379,8 +307,8 @@ func TestWasmRules(t *testing.T) { Rates: []kuadrantv1beta2.Rate{counter50rps}, }, }), - route: httpRoute, - hostnames: []gatewayapiv1beta1.Hostname{"*.example.com"}, // intentionally excluding "*.apps.example.internal" + route: httpRoute, + targetHostnames: []gatewayapiv1beta1.Hostname{"*.example.com"}, // intentionally excluding "*.apps.example.internal" expectedRules: []wasm.Rule{ { Conditions: []wasm.Condition{ @@ -396,11 +324,6 @@ func TestWasmRules(t *testing.T) { Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), Value: "GET", }, - { - Selector: "request.host", - Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), - Value: ".example.com", - }, }, }, }, @@ -447,7 +370,7 @@ func TestWasmRules(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - computedRules := WasmRules(tc.rlp, tc.route, tc.hostnames) + computedRules := WasmRules(tc.rlp, tc.route, tc.targetHostnames) if len(tc.expectedRules) != len(computedRules) { t.Errorf("expected %d wasm rules, got (%d)", len(tc.expectedRules), len(computedRules)) From aa52d24a204d9d429532cbae6709baa5dd7288f0 Mon Sep 17 00:00:00 2001 From: Guilherme Cassolato Date: Fri, 7 Jul 2023 13:21:04 +0200 Subject: [PATCH 07/15] do not repeat RLPs for each hostname within the wasm config --- .../ratelimitpolicy_controller_test.go | 128 ++---------------- controllers/ratelimitpolicy_wasm_plugins.go | 51 +++---- pkg/common/common.go | 8 +- 3 files changed, 38 insertions(+), 149 deletions(-) diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index 93b481ddc..b07a75455 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -225,10 +225,8 @@ var _ = Describe("RateLimitPolicy controller", func() { FailureMode: wasm.FailureModeDeny, RateLimitPolicies: []wasm.RateLimitPolicy{ { - Name: "*.example.com", - Domain: common.MarshallNamespace(client.ObjectKeyFromObject(gateway), "*.example.com"), - Service: common.KuadrantRateLimitClusterName, - Hostnames: []string{"*.example.com"}, + Name: rlpKey.String(), + Domain: fmt.Sprintf("%s/%s#%s", testNamespace, gwName, "*.example.com"), Rules: []wasm.Rule{ { Conditions: []wasm.Condition{ @@ -257,6 +255,8 @@ var _ = Describe("RateLimitPolicy controller", func() { }, }, }, + Hostnames: []string{"*.example.com"}, + Service: common.KuadrantRateLimitClusterName, }, }, })) @@ -391,17 +391,11 @@ var _ = Describe("RateLimitPolicy controller", func() { existingWASMConfig, err := rlptools.WASMPluginFromStruct(existingWasmPlugin.Spec.PluginConfig) Expect(err).ToNot(HaveOccurred()) Expect(existingWASMConfig.FailureMode).To(Equal(wasm.FailureModeDeny)) - Expect(existingWASMConfig.RateLimitPolicies).To(HaveLen(2)) - // check the wasm rlp configs for each hostname in a verbose way because we cannot guarantee the order of the rules in each rlp config - // check wasm rlp config for the 1st hostname - wasmRLP, found := common.Find(existingWASMConfig.RateLimitPolicies, func(wasmRLP wasm.RateLimitPolicy) bool { - return wasmRLP.Name == "*.toystore.acme.com" - }) - Expect(found).To(BeTrue()) - Expect(wasmRLP.Domain).To(Equal(common.MarshallNamespace(client.ObjectKeyFromObject(gateway), "*.toystore.acme.com"))) - Expect(wasmRLP.Service).To(Equal(common.KuadrantRateLimitClusterName)) - Expect(wasmRLP.Hostnames).To(Equal([]string{"*.toystore.acme.com"})) - Expect(wasmRLP.Rules).To(ContainElement(wasm.Rule{ + Expect(existingWASMConfig.RateLimitPolicies).To(HaveLen(1)) + wasmRLP := existingWASMConfig.RateLimitPolicies[0] + Expect(wasmRLP.Name).To(Equal(rlpKey.String())) + Expect(wasmRLP.Domain).To(Equal(fmt.Sprintf("%s/%s#%s", testNamespace, gwName, "*.toystore.acme.com"))) + Expect(wasmRLP.Rules).To(ContainElement(wasm.Rule{ // rule to activate the 'toys' limit defintion Conditions: []wasm.Condition{ { AllOf: []wasm.PatternExpression{ @@ -466,7 +460,7 @@ var _ = Describe("RateLimitPolicy controller", func() { }, }, })) - Expect(wasmRLP.Rules).To(ContainElement(wasm.Rule{ + Expect(wasmRLP.Rules).To(ContainElement(wasm.Rule{ // rule to activate the 'assets' limit defintion Conditions: []wasm.Condition{ { AllOf: []wasm.PatternExpression{ @@ -487,100 +481,8 @@ var _ = Describe("RateLimitPolicy controller", func() { }, }, })) - // check wasm rlp config for the 2nd hostname - wasmRLP, found = common.Find(existingWASMConfig.RateLimitPolicies, func(wasmRLP wasm.RateLimitPolicy) bool { - return wasmRLP.Name == "api.toystore.io" - }) - Expect(found).To(BeTrue()) - Expect(wasmRLP.Domain).To(Equal(common.MarshallNamespace(client.ObjectKeyFromObject(gateway), "api.toystore.io"))) + Expect(wasmRLP.Hostnames).To(Equal([]string{"*.toystore.acme.com", "api.toystore.io"})) Expect(wasmRLP.Service).To(Equal(common.KuadrantRateLimitClusterName)) - Expect(wasmRLP.Hostnames).To(Equal([]string{"api.toystore.io"})) - Expect(wasmRLP.Rules).To(ContainElement(wasm.Rule{ // FIXME(guicassolato): this entire rule should not be generated - Conditions: []wasm.Condition{ - { - AllOf: []wasm.PatternExpression{ - { - Selector: "request.url_path", - Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), - Value: "/toys", - }, - { - Selector: "request.method", - Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), - Value: "GET", - }, - { - Selector: "request.host", - Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), - Value: ".toystore.acme.com", - }, - { - Selector: "auth.identity.group", - Operator: wasm.PatternOperator(kuadrantv1beta2.NotEqualOperator), - Value: "admin", - }, - }, - }, - { - AllOf: []wasm.PatternExpression{ - { - Selector: "request.url_path", - Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), - Value: "/toys", - }, - { - Selector: "request.method", - Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), - Value: "POST", - }, - { - Selector: "request.host", - Operator: wasm.PatternOperator(kuadrantv1beta2.EndsWithOperator), - Value: ".toystore.acme.com", - }, - { - Selector: "auth.identity.group", - Operator: wasm.PatternOperator(kuadrantv1beta2.NotEqualOperator), - Value: "admin", - }, - }, - }, - }, - Data: []wasm.DataItem{ - { - Static: &wasm.StaticSpec{ - Key: fmt.Sprintf("%s/%s/toys", testNamespace, rlpName), - Value: "1", - }, - }, - { - Selector: &wasm.SelectorSpec{ - Selector: kuadrantv1beta2.ContextSelector("auth.identity.username"), - }, - }, - }, - })) - Expect(wasmRLP.Rules).To(ContainElement(wasm.Rule{ - Conditions: []wasm.Condition{ - { - AllOf: []wasm.PatternExpression{ - { - Selector: "request.url_path", - Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), - Value: "/assets", - }, - }, - }, - }, - Data: []wasm.DataItem{ - { - Static: &wasm.StaticSpec{ - Key: fmt.Sprintf("%s/%s/assets", testNamespace, rlpName), - Value: "1", - }, - }, - }, - })) }) }) @@ -664,10 +566,8 @@ var _ = Describe("RateLimitPolicy controller", func() { FailureMode: wasm.FailureModeDeny, RateLimitPolicies: []wasm.RateLimitPolicy{ { - Name: "*", - Domain: common.MarshallNamespace(client.ObjectKeyFromObject(gateway), "*"), - Service: common.KuadrantRateLimitClusterName, - Hostnames: []string{"*"}, + Name: rlpKey.String(), + Domain: fmt.Sprintf("%s/%s#%s", testNamespace, gwName, "*"), Rules: []wasm.Rule{ { Conditions: nil, @@ -681,6 +581,8 @@ var _ = Describe("RateLimitPolicy controller", func() { }, }, }, + Hostnames: []string{"*"}, + Service: common.KuadrantRateLimitClusterName, }, }, })) diff --git a/controllers/ratelimitpolicy_wasm_plugins.go b/controllers/ratelimitpolicy_wasm_plugins.go index 02e88bee7..06639d596 100644 --- a/controllers/ratelimitpolicy_wasm_plugins.go +++ b/controllers/ratelimitpolicy_wasm_plugins.go @@ -144,8 +144,10 @@ func (r *RateLimitPolicyReconciler) wasmPluginConfig(ctx context.Context, gw com } } - wasmRulesByDomain := make(rlptools.WasmRulesByDomain) - var gwWasmRules []wasm.Rule + wasmPlugin := &wasm.WASMPlugin{ + FailureMode: wasm.FailureModeDeny, + RateLimitPolicies: make([]wasm.RateLimitPolicy, 0), + } gwHostnames := gw.Hostnames() if len(gwHostnames) == 0 { @@ -153,17 +155,21 @@ func (r *RateLimitPolicyReconciler) wasmPluginConfig(ctx context.Context, gw com } if gwRLP != nil { - // FIXME(guicassolato): this is a hack until we start going through all the httproutes that are children of the gateway and build the rules for each httproute + // TODO(guicassolato): this is a hack until we start going through all the httproutes that are children of the gateway and build the rules for each httproute route := &gatewayapiv1beta1.HTTPRoute{ Spec: gatewayapiv1beta1.HTTPRouteSpec{ Hostnames: gwHostnames, Rules: []gatewayapiv1beta1.HTTPRouteRule{{}}, }, } - gwWasmRules = rlptools.WasmRules(gwRLP, route, gwHostnames) // FIXME(guicassolato): this is not correct. We need to go through all the httproutes that are children of the gateway and build the rules for each httproute instead - for _, gwHostname := range gwHostnames { - wasmRulesByDomain[string(gwHostname)] = append(wasmRulesByDomain[string(gwHostname)], gwWasmRules...) - } + + wasmPlugin.RateLimitPolicies = append(wasmPlugin.RateLimitPolicies, wasm.RateLimitPolicy{ + Name: client.ObjectKeyFromObject(gwRLP).String(), + Domain: common.MarshallNamespace(gw.Key(), string(gwHostnames[0])), // TODO(guicassolato): https://github.com/Kuadrant/kuadrant-operator/issues/201. Meanwhile, we are using the first hostname so it matches at least one set of limit definitions in the Limitador CR + Rules: rlptools.WasmRules(gwRLP, route, gwHostnames), + Hostnames: common.HostnamesToStrings(gwHostnames), // we might be listing more hostnames than needed due to route selectors hostnames possibly being more restrictive + Service: common.KuadrantRateLimitClusterName, + }) } for _, httpRouteRLP := range routeRLPList { @@ -174,34 +180,17 @@ func (r *RateLimitPolicyReconciler) wasmPluginConfig(ctx context.Context, gw com // filter the route hostnames to only the ones that are children of the gateway hostnames := common.FilterValidSubdomains(gwHostnames, httpRoute.Spec.Hostnames) - if len(hostnames) == 0 { // should never happen + if len(hostnames) == 0 { hostnames = gwHostnames } - // gateways limits merged with the route level limits - wasmRules := append(rlptools.WasmRules(httpRouteRLP, httpRoute, hostnames), gwWasmRules...) // FIXME(guicassolato): there will be no need to merge gwRLP rules when targeting a gateway == shortcut for targeting all the routes of a gateway - // routeLimits referenced by multiple hostnames - for _, hostname := range hostnames { - wasmRulesByDomain[string(hostname)] = append(wasmRulesByDomain[string(hostname)], wasmRules...) - } - } - - wasmPlugin := &wasm.WASMPlugin{ - FailureMode: wasm.FailureModeDeny, - RateLimitPolicies: make([]wasm.RateLimitPolicy, 0), - } - - // One RateLimitPolicy per domain - // FIXME(guicassolato): Why do we map per domain? Is it so the wasm-shim can index the config per domain and improve perfomance in the data plane? If so, this will occasionally generate incongruent entries of domain keys combined with rules that have no relation with that domain. - for domain, rules := range wasmRulesByDomain { - rateLimitPolicy := wasm.RateLimitPolicy{ - Name: domain, // FIXME(guicassolato): can't we use the name of the policy instead? - Domain: common.MarshallNamespace(gw.Key(), domain), // FIXME(guicassolato) https://github.com/Kuadrant/kuadrant-operator/issues/201 + wasmPlugin.RateLimitPolicies = append(wasmPlugin.RateLimitPolicies, wasm.RateLimitPolicy{ + Name: client.ObjectKeyFromObject(httpRouteRLP).String(), + Domain: common.MarshallNamespace(gw.Key(), string(hostnames[0])), // TODO(guicassolato): https://github.com/Kuadrant/kuadrant-operator/issues/201. Meanwhile, we are using the first hostname so it matches at least one set of limit definitions in the Limitador CR + Rules: rlptools.WasmRules(httpRouteRLP, httpRoute, hostnames), + Hostnames: common.HostnamesToStrings(hostnames), // we might be listing more hostnames than needed due to route selectors hostnames possibly being more restrictive Service: common.KuadrantRateLimitClusterName, - Hostnames: []string{domain}, - Rules: rules, - } - wasmPlugin.RateLimitPolicies = append(wasmPlugin.RateLimitPolicies, rateLimitPolicy) + }) } return wasmPlugin, nil diff --git a/pkg/common/common.go b/pkg/common/common.go index 464a72a2c..8ebe17fbf 100644 --- a/pkg/common/common.go +++ b/pkg/common/common.go @@ -255,11 +255,9 @@ func UnMarshallObjectKey(keyStr string) (client.ObjectKey, error) { // HostnamesToStrings converts []gatewayapi_v1alpha2.Hostname to []string func HostnamesToStrings(hostnames []gatewayapiv1beta1.Hostname) []string { - hosts := make([]string, len(hostnames)) - for i, h := range hostnames { - hosts[i] = string(h) - } - return hosts + return Map(hostnames, func(hostname gatewayapiv1beta1.Hostname) string { + return string(hostname) + }) } // ValidSubdomains returns (true, "") when every single subdomains item From 8206ffcd2b88a7b7c1d56014f76aeb8253eda32d Mon Sep 17 00:00:00 2001 From: Guilherme Cassolato Date: Fri, 7 Jul 2023 17:16:55 +0200 Subject: [PATCH 08/15] `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. --- api/v1beta2/ratelimitpolicy_types.go | 15 ------ api/v1beta2/route_selectors.go | 47 ++++++++++++++++++ .../v1beta2}/route_selectors_test.go | 26 +++++----- controllers/ratelimitpolicy_wasm_plugins.go | 9 ++-- pkg/rlptools/route_selectors.go | 30 ------------ pkg/rlptools/wasm_utils.go | 31 +++++------- pkg/rlptools/wasm_utils_test.go | 49 ++----------------- 7 files changed, 81 insertions(+), 126 deletions(-) create mode 100644 api/v1beta2/route_selectors.go rename {pkg/rlptools => api/v1beta2}/route_selectors_test.go (89%) delete mode 100644 pkg/rlptools/route_selectors.go diff --git a/api/v1beta2/ratelimitpolicy_types.go b/api/v1beta2/ratelimitpolicy_types.go index fb50f7faf..6744bae27 100644 --- a/api/v1beta2/ratelimitpolicy_types.go +++ b/api/v1beta2/ratelimitpolicy_types.go @@ -25,7 +25,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" - gatewayapiv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1" ) // EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN! @@ -84,20 +83,6 @@ type WhenCondition struct { Value string `json:"value"` } -// RouteSelector defines semantics for matching an HTTP request based on conditions -// https://gateway-api.sigs.k8s.io/v1alpha2/references/spec/#gateway.networking.k8s.io/v1beta1.HTTPRouteSpec -type RouteSelector struct { - // Hostnames defines a set of hostname that should match against the HTTP Host header to select a HTTPRoute to process the request - // https://gateway-api.sigs.k8s.io/v1alpha2/references/spec/#gateway.networking.k8s.io/v1beta1.HTTPRouteSpec - // +optional - Hostnames []gatewayapiv1beta1.Hostname `json:"hostnames,omitempty"` - - // Matches define conditions used for matching the rule against incoming HTTP requests. - // https://gateway-api.sigs.k8s.io/v1alpha2/references/spec/#gateway.networking.k8s.io/v1beta1.HTTPRouteSpec - // +optional - Matches []gatewayapiv1beta1.HTTPRouteMatch `json:"matches,omitempty"` -} - // Limit represents a complete rate limit configuration type Limit struct { // RouteSelectors defines semantics for matching an HTTP request based on conditions diff --git a/api/v1beta2/route_selectors.go b/api/v1beta2/route_selectors.go new file mode 100644 index 000000000..9e76e5d45 --- /dev/null +++ b/api/v1beta2/route_selectors.go @@ -0,0 +1,47 @@ +package v1beta2 + +import ( + gatewayapiv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1" + + "github.com/kuadrant/kuadrant-operator/pkg/common" +) + +// RouteSelector defines semantics for matching an HTTP request based on conditions +// https://gateway-api.sigs.k8s.io/v1alpha2/references/spec/#gateway.networking.k8s.io/v1beta1.HTTPRouteSpec +type RouteSelector struct { + // Hostnames defines a set of hostname that should match against the HTTP Host header to select a HTTPRoute to process the request + // https://gateway-api.sigs.k8s.io/v1alpha2/references/spec/#gateway.networking.k8s.io/v1beta1.HTTPRouteSpec + // +optional + Hostnames []gatewayapiv1beta1.Hostname `json:"hostnames,omitempty"` + + // Matches define conditions used for matching the rule against incoming HTTP requests. + // https://gateway-api.sigs.k8s.io/v1alpha2/references/spec/#gateway.networking.k8s.io/v1beta1.HTTPRouteSpec + // +optional + Matches []gatewayapiv1beta1.HTTPRouteMatch `json:"matches,omitempty"` +} + +// SelectRules returns, from a HTTPRoute, all HTTPRouteRules that either specify no HTTRouteMatches or that contain at +// least one HTTRouteMatch whose statements expressly include (partially or totally) the statements of at least one of +// the matches of the selector. If the selector does not specify any matches, then all HTTPRouteRules are selected. +// +// Additionally, if the selector specifies a non-empty list of hostnames, a non-empty intersection between the literal +// hostnames of the selector and set of hostnames specified in the HTTPRoute must be exist. Otherwise, the function +// returns nil. +func (s *RouteSelector) SelectRules(route *gatewayapiv1beta1.HTTPRoute) []gatewayapiv1beta1.HTTPRouteRule { + rulesIndices := make(map[int]gatewayapiv1beta1.HTTPRouteRule, 0) // using a map is an easy way to avoid repeated rules but it may not preserve the order + if len(s.Hostnames) > 0 && !common.Intersect(s.Hostnames, route.Spec.Hostnames) { + return nil + } + if len(s.Matches) == 0 { + return route.Spec.Rules + } + for _, routeSelectorMatch := range s.Matches { + for idx, rule := range route.Spec.Rules { + rs := common.HTTPRouteRuleSelector{HTTPRouteMatch: &routeSelectorMatch} + if rs.Selects(rule) { + rulesIndices[idx] = rule + } + } + } + return common.MapValues(rulesIndices) +} diff --git a/pkg/rlptools/route_selectors_test.go b/api/v1beta2/route_selectors_test.go similarity index 89% rename from pkg/rlptools/route_selectors_test.go rename to api/v1beta2/route_selectors_test.go index 19587f5a8..0d2e3194e 100644 --- a/pkg/rlptools/route_selectors_test.go +++ b/api/v1beta2/route_selectors_test.go @@ -1,6 +1,6 @@ //go:build unit -package rlptools +package v1beta2 import ( "fmt" @@ -10,7 +10,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" gatewayapiv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1" - kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" "github.com/kuadrant/kuadrant-operator/pkg/common" ) @@ -77,20 +76,19 @@ func TestRouteSelectors(t *testing.T) { testCases := []struct { name string - routeSelector kuadrantv1beta2.RouteSelector + routeSelector RouteSelector route *gatewayapiv1beta1.HTTPRoute - hostnames []gatewayapiv1beta1.Hostname expected []gatewayapiv1beta1.HTTPRouteRule }{ { name: "empty route selector selects all HTTPRouteRules", - routeSelector: kuadrantv1beta2.RouteSelector{}, + routeSelector: RouteSelector{}, route: route, expected: route.Spec.Rules, }, { name: "route selector selects the HTTPRouteRules whose set of HTTPRouteMatch is a perfect match", - routeSelector: kuadrantv1beta2.RouteSelector{ + routeSelector: RouteSelector{ Matches: []gatewayapiv1beta1.HTTPRouteMatch{ { Path: &gatewayapiv1beta1.HTTPPathMatch{ @@ -105,7 +103,7 @@ func TestRouteSelectors(t *testing.T) { }, { name: "route selector selects the HTTPRouteRules whose set of HTTPRouteMatch contains at least one match", - routeSelector: kuadrantv1beta2.RouteSelector{ + routeSelector: RouteSelector{ Matches: []gatewayapiv1beta1.HTTPRouteMatch{ { Path: &gatewayapiv1beta1.HTTPPathMatch{ @@ -121,7 +119,7 @@ func TestRouteSelectors(t *testing.T) { }, { name: "route selector with missing part of a HTTPRouteMatch still selects the HTTPRouteRules that match", - routeSelector: kuadrantv1beta2.RouteSelector{ + routeSelector: RouteSelector{ Matches: []gatewayapiv1beta1.HTTPRouteMatch{ { Path: &gatewayapiv1beta1.HTTPPathMatch{ @@ -136,7 +134,7 @@ func TestRouteSelectors(t *testing.T) { }, { name: "route selector selects no HTTPRouteRule when no criterion matches", - routeSelector: kuadrantv1beta2.RouteSelector{ + routeSelector: RouteSelector{ Matches: []gatewayapiv1beta1.HTTPRouteMatch{ { Path: &gatewayapiv1beta1.HTTPPathMatch{ @@ -151,7 +149,7 @@ func TestRouteSelectors(t *testing.T) { }, { name: "route selector selects the HTTPRouteRules whose HTTPRoute's hostnames match the selector", - routeSelector: kuadrantv1beta2.RouteSelector{ + routeSelector: RouteSelector{ Hostnames: []gatewayapiv1beta1.Hostname{"api.toystore.com"}, }, route: route, @@ -159,7 +157,7 @@ func TestRouteSelectors(t *testing.T) { }, { name: "route selector selects the HTTPRouteRules whose HTTPRoute's hostnames match the selector additionally to other criteria", - routeSelector: kuadrantv1beta2.RouteSelector{ + routeSelector: RouteSelector{ Hostnames: []gatewayapiv1beta1.Hostname{"api.toystore.com"}, Matches: []gatewayapiv1beta1.HTTPRouteMatch{ { @@ -175,7 +173,7 @@ func TestRouteSelectors(t *testing.T) { }, { name: "route selector does not select HTTPRouteRules whose HTTPRoute's hostnames do not match the selector", - routeSelector: kuadrantv1beta2.RouteSelector{ + routeSelector: RouteSelector{ Hostnames: []gatewayapiv1beta1.Hostname{"www.toystore.com"}, }, route: route, @@ -183,7 +181,7 @@ func TestRouteSelectors(t *testing.T) { }, { name: "route selector does not select HTTPRouteRules whose HTTPRoute's hostnames do not match the selector even when other criteria match", - routeSelector: kuadrantv1beta2.RouteSelector{ + routeSelector: RouteSelector{ Hostnames: []gatewayapiv1beta1.Hostname{"www.toystore.com"}, Matches: []gatewayapiv1beta1.HTTPRouteMatch{ { @@ -201,7 +199,7 @@ func TestRouteSelectors(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - rules := HTTPRouteRulesFromRouteSelector(tc.routeSelector, tc.route, tc.hostnames) + rules := tc.routeSelector.SelectRules(tc.route) rulesToStringSlice := func(rules []gatewayapiv1beta1.HTTPRouteRule) []string { return common.Map(common.Map(rules, common.HTTPRouteRuleToString), func(r string) string { return fmt.Sprintf("{%s}", r) }) } diff --git a/controllers/ratelimitpolicy_wasm_plugins.go b/controllers/ratelimitpolicy_wasm_plugins.go index 06639d596..76247331b 100644 --- a/controllers/ratelimitpolicy_wasm_plugins.go +++ b/controllers/ratelimitpolicy_wasm_plugins.go @@ -166,7 +166,7 @@ func (r *RateLimitPolicyReconciler) wasmPluginConfig(ctx context.Context, gw com wasmPlugin.RateLimitPolicies = append(wasmPlugin.RateLimitPolicies, wasm.RateLimitPolicy{ Name: client.ObjectKeyFromObject(gwRLP).String(), Domain: common.MarshallNamespace(gw.Key(), string(gwHostnames[0])), // TODO(guicassolato): https://github.com/Kuadrant/kuadrant-operator/issues/201. Meanwhile, we are using the first hostname so it matches at least one set of limit definitions in the Limitador CR - Rules: rlptools.WasmRules(gwRLP, route, gwHostnames), + Rules: rlptools.WasmRules(gwRLP, route), Hostnames: common.HostnamesToStrings(gwHostnames), // we might be listing more hostnames than needed due to route selectors hostnames possibly being more restrictive Service: common.KuadrantRateLimitClusterName, }) @@ -178,16 +178,17 @@ func (r *RateLimitPolicyReconciler) wasmPluginConfig(ctx context.Context, gw com return nil, err } - // filter the route hostnames to only the ones that are children of the gateway + // modifies (in memory) the list of hostnames specified in the route so we don't generate rules that only apply to other gateways hostnames := common.FilterValidSubdomains(gwHostnames, httpRoute.Spec.Hostnames) - if len(hostnames) == 0 { + if len(hostnames) == 0 { // it should only happen when the route specifies no hostnames hostnames = gwHostnames } + httpRoute.Spec.Hostnames = hostnames wasmPlugin.RateLimitPolicies = append(wasmPlugin.RateLimitPolicies, wasm.RateLimitPolicy{ Name: client.ObjectKeyFromObject(httpRouteRLP).String(), Domain: common.MarshallNamespace(gw.Key(), string(hostnames[0])), // TODO(guicassolato): https://github.com/Kuadrant/kuadrant-operator/issues/201. Meanwhile, we are using the first hostname so it matches at least one set of limit definitions in the Limitador CR - Rules: rlptools.WasmRules(httpRouteRLP, httpRoute, hostnames), + Rules: rlptools.WasmRules(httpRouteRLP, httpRoute), Hostnames: common.HostnamesToStrings(hostnames), // we might be listing more hostnames than needed due to route selectors hostnames possibly being more restrictive Service: common.KuadrantRateLimitClusterName, }) diff --git a/pkg/rlptools/route_selectors.go b/pkg/rlptools/route_selectors.go deleted file mode 100644 index 1172cd6cc..000000000 --- a/pkg/rlptools/route_selectors.go +++ /dev/null @@ -1,30 +0,0 @@ -package rlptools - -import ( - gatewayapiv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1" - - kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" - "github.com/kuadrant/kuadrant-operator/pkg/common" -) - -func HTTPRouteRulesFromRouteSelector(routeSelector kuadrantv1beta2.RouteSelector, route *gatewayapiv1beta1.HTTPRoute, hostnames []gatewayapiv1beta1.Hostname) []gatewayapiv1beta1.HTTPRouteRule { - rulesIndices := make(map[int]gatewayapiv1beta1.HTTPRouteRule, 0) - if len(hostnames) == 0 { - hostnames = route.Spec.Hostnames - } - if len(routeSelector.Hostnames) > 0 && !common.Intersect(routeSelector.Hostnames, hostnames) { - return nil - } - if len(routeSelector.Matches) == 0 { - return route.Spec.Rules - } - for _, routeSelectorMatch := range routeSelector.Matches { - for idx, rule := range route.Spec.Rules { - rs := common.HTTPRouteRuleSelector{HTTPRouteMatch: &routeSelectorMatch} - if rs.Selects(rule) { - rulesIndices[idx] = rule - } - } - } - return common.MapValues(rulesIndices) -} diff --git a/pkg/rlptools/wasm_utils.go b/pkg/rlptools/wasm_utils.go index 79a1c9da9..3dd71df19 100644 --- a/pkg/rlptools/wasm_utils.go +++ b/pkg/rlptools/wasm_utils.go @@ -22,23 +22,16 @@ var ( ) // WasmRules computes WASM rules from the policy and the targeted route. -// Pass a list of target hostnames to ensure that only rules those are computed; -// otherwise, the hostnames specified in the route will set the boundaries for the rules. -// This is useful to compute rules for a specific gateway. -func WasmRules(rlp *kuadrantv1beta2.RateLimitPolicy, route *gatewayapiv1beta1.HTTPRoute, targetHostnames []gatewayapiv1beta1.Hostname) []wasm.Rule { +func WasmRules(rlp *kuadrantv1beta2.RateLimitPolicy, route *gatewayapiv1beta1.HTTPRoute) []wasm.Rule { rules := make([]wasm.Rule, 0) if rlp == nil { return rules } - if len(targetHostnames) == 0 { - targetHostnames = route.Spec.Hostnames - } - for limitName, limit := range rlp.Spec.Limits { // 1 RLP limit <---> 1 WASM rule limitFullName := FullLimitName(rlp, limitName) - rule, err := ruleFromLimit(limitFullName, &limit, route, targetHostnames) + rule, err := ruleFromLimit(limitFullName, &limit, route) if err == nil { rules = append(rules, rule) } else { @@ -49,10 +42,10 @@ func WasmRules(rlp *kuadrantv1beta2.RateLimitPolicy, route *gatewayapiv1beta1.HT return rules } -func ruleFromLimit(limitFullName string, limit *kuadrantv1beta2.Limit, route *gatewayapiv1beta1.HTTPRoute, targetHostnames []gatewayapiv1beta1.Hostname) (wasm.Rule, error) { +func ruleFromLimit(limitFullName string, limit *kuadrantv1beta2.Limit, route *gatewayapiv1beta1.HTTPRoute) (wasm.Rule, error) { rule := wasm.Rule{} - if conditions, err := conditionsFromLimit(limit, route, targetHostnames); err != nil { + if conditions, err := conditionsFromLimit(limit, route); err != nil { return rule, err } else { rule.Conditions = conditions @@ -65,7 +58,7 @@ func ruleFromLimit(limitFullName string, limit *kuadrantv1beta2.Limit, route *ga return rule, nil } -func conditionsFromLimit(limit *kuadrantv1beta2.Limit, route *gatewayapiv1beta1.HTTPRoute, targetHostnames []gatewayapiv1beta1.Hostname) ([]wasm.Condition, error) { +func conditionsFromLimit(limit *kuadrantv1beta2.Limit, route *gatewayapiv1beta1.HTTPRoute) ([]wasm.Condition, error) { if limit == nil { return nil, errors.New("limit should not be nil") } @@ -75,8 +68,8 @@ func conditionsFromLimit(limit *kuadrantv1beta2.Limit, route *gatewayapiv1beta1. if len(limit.RouteSelectors) > 0 { // build conditions from the rules selected by the route selectors for _, routeSelector := range limit.RouteSelectors { - hostnamesForConditions := hostnamesForConditions(targetHostnames, route, &routeSelector) - for _, rule := range HTTPRouteRulesFromRouteSelector(routeSelector, route, targetHostnames) { + hostnamesForConditions := hostnamesForConditions(route, &routeSelector) + for _, rule := range routeSelector.SelectRules(route) { routeConditions = append(routeConditions, conditionsFromRule(rule, hostnamesForConditions)...) } } @@ -84,9 +77,9 @@ func conditionsFromLimit(limit *kuadrantv1beta2.Limit, route *gatewayapiv1beta1. return nil, errors.New("cannot match any route rules, check for invalid route selectors in the policy") } } else { - // build conditions from the route if no route selectors are defined + // build conditions from all rules if no route selectors are defined for _, rule := range route.Spec.Rules { - routeConditions = append(routeConditions, conditionsFromRule(rule, hostnamesForConditions(targetHostnames, route, nil))...) + routeConditions = append(routeConditions, conditionsFromRule(rule, hostnamesForConditions(route, nil))...) } } @@ -117,14 +110,14 @@ func conditionsFromLimit(limit *kuadrantv1beta2.Limit, route *gatewayapiv1beta1. // hostnamesForConditions allows avoiding building conditions for hostnames that are excluded by the selector // or when the hostname is irrelevant (i.e. matches all hostnames) -func hostnamesForConditions(targetHostnames []gatewayapiv1beta1.Hostname, route *gatewayapiv1beta1.HTTPRoute, routeSelector *kuadrantv1beta2.RouteSelector) []gatewayapiv1beta1.Hostname { - hostnames := targetHostnames +func hostnamesForConditions(route *gatewayapiv1beta1.HTTPRoute, routeSelector *kuadrantv1beta2.RouteSelector) []gatewayapiv1beta1.Hostname { + hostnames := route.Spec.Hostnames if routeSelector != nil && len(routeSelector.Hostnames) > 0 { hostnames = common.Intersection(routeSelector.Hostnames, hostnames) } - if common.SameElements(hostnames, targetHostnames) { + if common.SameElements(hostnames, route.Spec.Hostnames) { return []gatewayapiv1beta1.Hostname{"*"} } diff --git a/pkg/rlptools/wasm_utils_test.go b/pkg/rlptools/wasm_utils_test.go index 1688aabdf..f72064e92 100644 --- a/pkg/rlptools/wasm_utils_test.go +++ b/pkg/rlptools/wasm_utils_test.go @@ -67,11 +67,10 @@ func TestWasmRules(t *testing.T) { } testCases := []struct { - name string - rlp *kuadrantv1beta2.RateLimitPolicy - route *gatewayapiv1beta1.HTTPRoute - targetHostnames []gatewayapiv1beta1.Hostname - expectedRules []wasm.Rule + name string + rlp *kuadrantv1beta2.RateLimitPolicy + route *gatewayapiv1beta1.HTTPRoute + expectedRules []wasm.Rule }{ { name: "minimal RLP", @@ -300,44 +299,6 @@ func TestWasmRules(t *testing.T) { }, }, }, - { - name: "RLP for when one of the hostnames in the httproute is from a different gateway", - rlp: rlp("my-rlp", map[string]kuadrantv1beta2.Limit{ - "50rps": { - Rates: []kuadrantv1beta2.Rate{counter50rps}, - }, - }), - route: httpRoute, - targetHostnames: []gatewayapiv1beta1.Hostname{"*.example.com"}, // intentionally excluding "*.apps.example.internal" - expectedRules: []wasm.Rule{ - { - Conditions: []wasm.Condition{ - { - AllOf: []wasm.PatternExpression{ - { - Selector: "request.url_path", - Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), - Value: "/toy", - }, - { - Selector: "request.method", - Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), - Value: "GET", - }, - }, - }, - }, - Data: []wasm.DataItem{ - { - Static: &wasm.StaticSpec{ - Key: "my-app/my-rlp/50rps", - Value: "1", - }, - }, - }, - }, - }, - }, { name: "RLP with counter qualifier", rlp: rlp("my-rlp", map[string]kuadrantv1beta2.Limit{ @@ -370,7 +331,7 @@ func TestWasmRules(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - computedRules := WasmRules(tc.rlp, tc.route, tc.targetHostnames) + computedRules := WasmRules(tc.rlp, tc.route) if len(tc.expectedRules) != len(computedRules) { t.Errorf("expected %d wasm rules, got (%d)", len(tc.expectedRules), len(computedRules)) From de34731bd57380874ce742e15515c4ec25cfd1e7 Mon Sep 17 00:00:00 2001 From: Guilherme Cassolato Date: Mon, 10 Jul 2023 12:07:41 +0200 Subject: [PATCH 09/15] 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 --- .../ratelimitpolicy_controller_test.go | 119 +++++++++++++++++- controllers/ratelimitpolicy_wasm_plugins.go | 99 ++++++++------- pkg/reconcilers/targetref_reconciler.go | 29 +++++ 3 files changed, 195 insertions(+), 52 deletions(-) diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index b07a75455..ac8808e4c 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -488,6 +488,11 @@ var _ = Describe("RateLimitPolicy controller", func() { Context("RLP targeting Gateway", func() { It("Creates all the resources for a basic Gateway and RateLimitPolicy", func() { + // create httproute + httpRoute := testBuildBasicHttpRoute(routeName, gwName, testNamespace, []string{"*.example.com"}) + err := k8sClient.Create(context.Background(), httpRoute) + Expect(err).ToNot(HaveOccurred()) + // create ratelimitpolicy rlp := &kuadrantv1beta2.RateLimitPolicy{ TypeMeta: metav1.TypeMeta{ @@ -515,7 +520,7 @@ var _ = Describe("RateLimitPolicy controller", func() { }, }, } - err := k8sClient.Create(context.Background(), rlp) + err = k8sClient.Create(context.Background(), rlp) Expect(err).ToNot(HaveOccurred()) // Check RLP status is available @@ -567,10 +572,25 @@ var _ = Describe("RateLimitPolicy controller", func() { RateLimitPolicies: []wasm.RateLimitPolicy{ { Name: rlpKey.String(), - Domain: fmt.Sprintf("%s/%s#%s", testNamespace, gwName, "*"), + Domain: fmt.Sprintf("%s/%s#%s", testNamespace, gwName, "*.example.com"), Rules: []wasm.Rule{ { - Conditions: nil, + Conditions: []wasm.Condition{ + { + AllOf: []wasm.PatternExpression{ + { + Selector: "request.url_path", + Operator: wasm.PatternOperator(kuadrantv1beta2.StartsWithOperator), + Value: "/toy", + }, + { + Selector: "request.method", + Operator: wasm.PatternOperator(kuadrantv1beta2.EqualOperator), + Value: "GET", + }, + }, + }, + }, Data: []wasm.DataItem{ { Static: &wasm.StaticSpec{ @@ -581,7 +601,7 @@ var _ = Describe("RateLimitPolicy controller", func() { }, }, }, - Hostnames: []string{"*"}, + Hostnames: []string{"*.example.com"}, Service: common.KuadrantRateLimitClusterName, }, }, @@ -594,8 +614,97 @@ var _ = Describe("RateLimitPolicy controller", func() { refs := []client.ObjectKey{rlpKey} serialized, err := json.Marshal(refs) Expect(err).ToNot(HaveOccurred()) + Expect(existingGateway.GetAnnotations()).To(HaveKeyWithValue(common.RateLimitPoliciesBackRefAnnotation, string(serialized))) + }) + + It("Creates all the resources for a basic Gateway and RateLimitPolicy when missing a HTTPRoute attached to the Gateway", func() { + // create ratelimitpolicy + rlp := &kuadrantv1beta2.RateLimitPolicy{ + TypeMeta: metav1.TypeMeta{ + Kind: "RateLimitPolicy", + APIVersion: kuadrantv1beta2.GroupVersion.String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Name: rlpName, + Namespace: testNamespace, + }, + Spec: kuadrantv1beta2.RateLimitPolicySpec{ + TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ + Group: gatewayapiv1beta1.Group("gateway.networking.k8s.io"), + Kind: "Gateway", + Name: gatewayapiv1beta1.ObjectName(gwName), + }, + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, + }, + }, + }, + }, + } + err := k8sClient.Create(context.Background(), rlp) + Expect(err).ToNot(HaveOccurred()) + + // Check RLP status is available + rlpKey := client.ObjectKey{Name: rlpName, Namespace: testNamespace} + Eventually(testRLPIsAvailable(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + + // Check Gateway direct back reference + gwKey := client.ObjectKeyFromObject(gateway) + existingGateway := &gatewayapiv1beta1.Gateway{} + err = k8sClient.Get(context.Background(), gwKey, existingGateway) + // must exist + Expect(err).ToNot(HaveOccurred()) Expect(existingGateway.GetAnnotations()).To(HaveKeyWithValue( - common.RateLimitPoliciesBackRefAnnotation, string(serialized))) + common.RateLimitPolicyBackRefAnnotation, client.ObjectKeyFromObject(rlp).String())) + + // check limits + limitadorKey := client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace} + existingLimitador := &limitadorv1alpha1.Limitador{} + err = k8sClient.Get(context.Background(), limitadorKey, existingLimitador) + // must exist + Expect(err).ToNot(HaveOccurred()) + Expect(existingLimitador.Spec.Limits).To(ContainElements(limitadorv1alpha1.RateLimit{ + MaxValue: 1, + Seconds: 3 * 60, + Namespace: common.MarshallNamespace(client.ObjectKeyFromObject(gateway), "*"), + Conditions: []string{fmt.Sprintf("%s/%s/l1 == \"1\"", testNamespace, rlpName)}, + Variables: []string{}, + })) + + // Check envoy filter + efName := fmt.Sprintf("kuadrant-ratelimiting-cluster-%s", gwName) + efKey := client.ObjectKey{Name: efName, Namespace: testNamespace} + existingEF := &istioclientnetworkingv1alpha3.EnvoyFilter{} + err = k8sClient.Get(context.Background(), efKey, existingEF) + // must exist + Expect(err).ToNot(HaveOccurred()) + + // Check wasm plugin + wpName := fmt.Sprintf("kuadrant-%s", gwName) + wasmPluginKey := client.ObjectKey{Name: wpName, Namespace: testNamespace} + existingWasmPlugin := &istioclientgoextensionv1alpha1.WasmPlugin{} + err = k8sClient.Get(context.Background(), wasmPluginKey, existingWasmPlugin) + // must exist + Expect(err).ToNot(HaveOccurred()) + existingWASMConfig, err := rlptools.WASMPluginFromStruct(existingWasmPlugin.Spec.PluginConfig) + Expect(err).ToNot(HaveOccurred()) + Expect(existingWASMConfig).To(Equal(&wasm.WASMPlugin{ + FailureMode: wasm.FailureModeDeny, + RateLimitPolicies: []wasm.RateLimitPolicy{}, + })) + + // Check gateway back references + err = k8sClient.Get(context.Background(), gwKey, existingGateway) + // must exist + Expect(err).ToNot(HaveOccurred()) + refs := []client.ObjectKey{rlpKey} + serialized, err := json.Marshal(refs) + Expect(err).ToNot(HaveOccurred()) + Expect(existingGateway.GetAnnotations()).To(HaveKeyWithValue(common.RateLimitPoliciesBackRefAnnotation, string(serialized))) }) }) }) diff --git a/controllers/ratelimitpolicy_wasm_plugins.go b/controllers/ratelimitpolicy_wasm_plugins.go index 76247331b..f35009c0b 100644 --- a/controllers/ratelimitpolicy_wasm_plugins.go +++ b/controllers/ratelimitpolicy_wasm_plugins.go @@ -122,26 +122,46 @@ func (r *RateLimitPolicyReconciler) gatewayWASMPlugin(ctx context.Context, gw co // returns nil when there is no rate limit policy to apply func (r *RateLimitPolicyReconciler) wasmPluginConfig(ctx context.Context, gw common.GatewayWrapper, rlpRefs []client.ObjectKey) (*wasm.WASMPlugin, error) { logger, _ := logr.FromContext(ctx) + logger = logger.WithName("wasmPluginConfig").WithValues("gateway", gw.Key()) + + gwHostnames := gw.Hostnames() + if len(gwHostnames) == 0 { + gwHostnames = []gatewayapiv1beta1.Hostname{"*"} + } + + type routesPerRLP struct { + rlp kuadrantv1beta2.RateLimitPolicy + routes []gatewayapiv1beta1.HTTPRoute + } + rlps := make(map[string]*routesPerRLP, 0) - routeRLPList := make([]*kuadrantv1beta2.RateLimitPolicy, 0) - var gwRLP *kuadrantv1beta2.RateLimitPolicy for _, rlpKey := range rlpRefs { rlp := &kuadrantv1beta2.RateLimitPolicy{} err := r.Client().Get(ctx, rlpKey, rlp) - logger.V(1).Info("wasmPluginConfig", "get rlp", rlpKey, "err", err) + logger.V(1).Info("get rlp", "ratelimitpolicy", rlpKey, "err", err) if err != nil { return nil, err } + // target ref is a HTTPRoute if common.IsTargetRefHTTPRoute(rlp.Spec.TargetRef) { - routeRLPList = append(routeRLPList, rlp) - } else if common.IsTargetRefGateway(rlp.Spec.TargetRef) { - if gwRLP == nil { - gwRLP = rlp - } else { - return nil, fmt.Errorf("wasmPluginConfig: multiple gateway RLP found and only one expected. rlp keys: %v", rlpRefs) + httpRoute, err := r.FetchValidHTTPRoute(ctx, rlp.TargetKey()) + if err != nil { + return nil, err } + rlps[rlpKey.String()] = &routesPerRLP{rlp: *rlp, routes: []gatewayapiv1beta1.HTTPRoute{*httpRoute}} + continue } + + // target ref is a Gateway + if rlps[rlpKey.String()] != nil { + return nil, fmt.Errorf("wasmPluginConfig: multiple gateway RLP found and only one expected. rlp keys: %v", rlpRefs) + } + rlps[rlpKey.String()] = &routesPerRLP{rlp: *rlp, routes: r.FetchAcceptedGatewayHTTPRoutes(ctx, rlp.TargetKey())} + // should we reset the hostnames in the route with the hostnames of the gateway? otherwise the rlp that targets + // the gateway can only specify hostnames for route selection exactly as they are stated in the routes, not as they + // stated in the gateway, and this would be counterintuitive for the user, because, unlike other types of route + // selection (e.g. by path), the hostname is part of the gateway spec. } wasmPlugin := &wasm.WASMPlugin{ @@ -149,49 +169,34 @@ func (r *RateLimitPolicyReconciler) wasmPluginConfig(ctx context.Context, gw com RateLimitPolicies: make([]wasm.RateLimitPolicy, 0), } - gwHostnames := gw.Hostnames() - if len(gwHostnames) == 0 { - gwHostnames = []gatewayapiv1beta1.Hostname{"*"} - } - - if gwRLP != nil { - // TODO(guicassolato): this is a hack until we start going through all the httproutes that are children of the gateway and build the rules for each httproute - route := &gatewayapiv1beta1.HTTPRoute{ - Spec: gatewayapiv1beta1.HTTPRouteSpec{ - Hostnames: gwHostnames, - Rules: []gatewayapiv1beta1.HTTPRouteRule{{}}, - }, + if logger.V(1).Enabled() { + numRoutes := 0 + for _, rlp := range rlps { + numRoutes += len(rlp.routes) } - - wasmPlugin.RateLimitPolicies = append(wasmPlugin.RateLimitPolicies, wasm.RateLimitPolicy{ - Name: client.ObjectKeyFromObject(gwRLP).String(), - Domain: common.MarshallNamespace(gw.Key(), string(gwHostnames[0])), // TODO(guicassolato): https://github.com/Kuadrant/kuadrant-operator/issues/201. Meanwhile, we are using the first hostname so it matches at least one set of limit definitions in the Limitador CR - Rules: rlptools.WasmRules(gwRLP, route), - Hostnames: common.HostnamesToStrings(gwHostnames), // we might be listing more hostnames than needed due to route selectors hostnames possibly being more restrictive - Service: common.KuadrantRateLimitClusterName, - }) + logger.V(1).Info("build configs for rlps and routes", "#rlps", len(rlps), "#routes", numRoutes) } - for _, httpRouteRLP := range routeRLPList { - httpRoute, err := r.FetchValidHTTPRoute(ctx, httpRouteRLP.TargetKey()) - if err != nil { - return nil, err - } + for _, routesPerRLP := range rlps { + rlp := routesPerRLP.rlp + for _, httpRoute := range routesPerRLP.routes { + logger.V(1).Info("building config for rlp and route", "ratelimitpolicy", client.ObjectKeyFromObject(&rlp), "httproute", client.ObjectKeyFromObject(&httpRoute)) - // modifies (in memory) the list of hostnames specified in the route so we don't generate rules that only apply to other gateways - hostnames := common.FilterValidSubdomains(gwHostnames, httpRoute.Spec.Hostnames) - if len(hostnames) == 0 { // it should only happen when the route specifies no hostnames - hostnames = gwHostnames + // modifies (in memory) the list of hostnames specified in the route so we don't generate rules that only apply to other gateways + hostnames := common.FilterValidSubdomains(gwHostnames, httpRoute.Spec.Hostnames) + if len(hostnames) == 0 { // it should only happen when the route specifies no hostnames + hostnames = gwHostnames + } + httpRoute.Spec.Hostnames = hostnames + + wasmPlugin.RateLimitPolicies = append(wasmPlugin.RateLimitPolicies, wasm.RateLimitPolicy{ + Name: client.ObjectKeyFromObject(&rlp).String(), + Domain: common.MarshallNamespace(gw.Key(), string(hostnames[0])), // TODO(guicassolato): https://github.com/Kuadrant/kuadrant-operator/issues/201. Meanwhile, we are using the first hostname so it matches at least one set of limit definitions in the Limitador CR + Rules: rlptools.WasmRules(&rlp, &httpRoute), + Hostnames: common.HostnamesToStrings(hostnames), // we might be listing more hostnames than needed due to route selectors hostnames possibly being more restrictive + Service: common.KuadrantRateLimitClusterName, + }) } - httpRoute.Spec.Hostnames = hostnames - - wasmPlugin.RateLimitPolicies = append(wasmPlugin.RateLimitPolicies, wasm.RateLimitPolicy{ - Name: client.ObjectKeyFromObject(httpRouteRLP).String(), - Domain: common.MarshallNamespace(gw.Key(), string(hostnames[0])), // TODO(guicassolato): https://github.com/Kuadrant/kuadrant-operator/issues/201. Meanwhile, we are using the first hostname so it matches at least one set of limit definitions in the Limitador CR - Rules: rlptools.WasmRules(httpRouteRLP, httpRoute), - Hostnames: common.HostnamesToStrings(hostnames), // we might be listing more hostnames than needed due to route selectors hostnames possibly being more restrictive - Service: common.KuadrantRateLimitClusterName, - }) } return wasmPlugin, nil diff --git a/pkg/reconcilers/targetref_reconciler.go b/pkg/reconcilers/targetref_reconciler.go index 124114194..d5d3b5613 100644 --- a/pkg/reconcilers/targetref_reconciler.go +++ b/pkg/reconcilers/targetref_reconciler.go @@ -94,6 +94,35 @@ func (r *TargetRefReconciler) FetchValidTargetRef(ctx context.Context, targetRef return nil, fmt.Errorf("FetchValidTargetRef: targetRef (%v) to unknown network resource", targetRef) } +// FetchAcceptedGatewayHTTPRoutes returns the list of HTTPRoutes that have been accepted as children of a gateway. +func (r *TargetRefReconciler) FetchAcceptedGatewayHTTPRoutes(ctx context.Context, gwKey client.ObjectKey) (routeKeys []gatewayapiv1beta1.HTTPRoute) { + logger, _ := logr.FromContext(ctx) + logger = logger.WithName("FetchAcceptedGatewayHTTPRoutes").WithValues("gateway", gwKey) + + routeList := &gatewayapiv1beta1.HTTPRouteList{} + err := r.Client().List(ctx, routeList) + if err != nil { + logger.V(1).Info("failed to list httproutes", "err", err) + return + } + + for _, route := range routeList.Items { + routeParentStatus, found := common.Find(route.Status.RouteStatus.Parents, func(p gatewayapiv1beta1.RouteParentStatus) bool { + return *p.ParentRef.Kind == ("Gateway") && + ((p.ParentRef.Namespace == nil && route.GetNamespace() == gwKey.Namespace) || string(*p.ParentRef.Namespace) == gwKey.Namespace) && + string(p.ParentRef.Name) == gwKey.Name + }) + if found && meta.IsStatusConditionTrue(routeParentStatus.Conditions, "Accepted") { + logger.V(1).Info("found route attached to gateway", "httproute", client.ObjectKeyFromObject(&route)) + routeKeys = append(routeKeys, route) + continue + } + logger.V(1).Info("skipping route, not attached to gateway", "httproute", client.ObjectKeyFromObject(&route)) + } + + return +} + // TargetedGatewayKeys returns the list of gateways that are being referenced from the target. func (r *TargetRefReconciler) TargetedGatewayKeys(ctx context.Context, targetNetworkObject client.Object) []client.ObjectKey { switch obj := targetNetworkObject.(type) { From 90f963eb132ee2f8e323923bdfb0f1104166eeb1 Mon Sep 17 00:00:00 2001 From: Guilherme Cassolato Date: Mon, 10 Jul 2023 14:23:51 +0200 Subject: [PATCH 10/15] match the gateway hostnames instead of the httproute ones when building the wasm rules for rlps that target a gateway --- .../ratelimitpolicy_controller_test.go | 4 +- controllers/ratelimitpolicy_wasm_plugins.go | 80 ++++++++++--------- 2 files changed, 45 insertions(+), 39 deletions(-) diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index ac8808e4c..bb5fe1ce0 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -572,7 +572,7 @@ var _ = Describe("RateLimitPolicy controller", func() { RateLimitPolicies: []wasm.RateLimitPolicy{ { Name: rlpKey.String(), - Domain: fmt.Sprintf("%s/%s#%s", testNamespace, gwName, "*.example.com"), + Domain: fmt.Sprintf("%s/%s#%s", testNamespace, gwName, "*"), Rules: []wasm.Rule{ { Conditions: []wasm.Condition{ @@ -601,7 +601,7 @@ var _ = Describe("RateLimitPolicy controller", func() { }, }, }, - Hostnames: []string{"*.example.com"}, + Hostnames: []string{"*"}, Service: common.KuadrantRateLimitClusterName, }, }, diff --git a/controllers/ratelimitpolicy_wasm_plugins.go b/controllers/ratelimitpolicy_wasm_plugins.go index f35009c0b..32a05ad01 100644 --- a/controllers/ratelimitpolicy_wasm_plugins.go +++ b/controllers/ratelimitpolicy_wasm_plugins.go @@ -129,11 +129,11 @@ func (r *RateLimitPolicyReconciler) wasmPluginConfig(ctx context.Context, gw com gwHostnames = []gatewayapiv1beta1.Hostname{"*"} } - type routesPerRLP struct { - rlp kuadrantv1beta2.RateLimitPolicy - routes []gatewayapiv1beta1.HTTPRoute + type objects struct { + rlp kuadrantv1beta2.RateLimitPolicy + httpRoute gatewayapiv1beta1.HTTPRoute } - rlps := make(map[string]*routesPerRLP, 0) + rlps := make(map[string]*objects, 0) for _, rlpKey := range rlpRefs { rlp := &kuadrantv1beta2.RateLimitPolicy{} @@ -149,7 +149,7 @@ func (r *RateLimitPolicyReconciler) wasmPluginConfig(ctx context.Context, gw com if err != nil { return nil, err } - rlps[rlpKey.String()] = &routesPerRLP{rlp: *rlp, routes: []gatewayapiv1beta1.HTTPRoute{*httpRoute}} + rlps[rlpKey.String()] = &objects{rlp: *rlp, httpRoute: *httpRoute} continue } @@ -157,11 +157,28 @@ func (r *RateLimitPolicyReconciler) wasmPluginConfig(ctx context.Context, gw com if rlps[rlpKey.String()] != nil { return nil, fmt.Errorf("wasmPluginConfig: multiple gateway RLP found and only one expected. rlp keys: %v", rlpRefs) } - rlps[rlpKey.String()] = &routesPerRLP{rlp: *rlp, routes: r.FetchAcceptedGatewayHTTPRoutes(ctx, rlp.TargetKey())} - // should we reset the hostnames in the route with the hostnames of the gateway? otherwise the rlp that targets - // the gateway can only specify hostnames for route selection exactly as they are stated in the routes, not as they - // stated in the gateway, and this would be counterintuitive for the user, because, unlike other types of route - // selection (e.g. by path), the hostname is part of the gateway spec. + // build a single httproute with all rules from all httproutes attached to the gateway and the hostnames of the gateway. + // the hostnames of the gateway are used because, otherwise, the rlp that targets the gateway would have to specify + // hostnames for route selection exactly as they are stated in the httproutes, not as they stated in the gateway, + // and this would be counterintuitive for the user, because, unlike other types of route selection (e.g. by path), + // the hostnames are part of the gateway spec. + rules := make([]gatewayapiv1beta1.HTTPRouteRule, 0) + for _, route := range r.FetchAcceptedGatewayHTTPRoutes(ctx, rlp.TargetKey()) { + rules = append(rules, route.Spec.Rules...) + } + if len(rules) == 0 { + logger.V(1).Info("no httproutes attached to the targeted gateway, skipping wasm config for the rlp", "ratelimitpolicy", rlpKey) + continue + } + rlps[rlpKey.String()] = &objects{ + rlp: *rlp, + httpRoute: gatewayapiv1beta1.HTTPRoute{ + Spec: gatewayapiv1beta1.HTTPRouteSpec{ + Hostnames: gwHostnames, + Rules: rules, + }, + }, + } } wasmPlugin := &wasm.WASMPlugin{ @@ -169,34 +186,23 @@ func (r *RateLimitPolicyReconciler) wasmPluginConfig(ctx context.Context, gw com RateLimitPolicies: make([]wasm.RateLimitPolicy, 0), } - if logger.V(1).Enabled() { - numRoutes := 0 - for _, rlp := range rlps { - numRoutes += len(rlp.routes) - } - logger.V(1).Info("build configs for rlps and routes", "#rlps", len(rlps), "#routes", numRoutes) - } - - for _, routesPerRLP := range rlps { - rlp := routesPerRLP.rlp - for _, httpRoute := range routesPerRLP.routes { - logger.V(1).Info("building config for rlp and route", "ratelimitpolicy", client.ObjectKeyFromObject(&rlp), "httproute", client.ObjectKeyFromObject(&httpRoute)) - - // modifies (in memory) the list of hostnames specified in the route so we don't generate rules that only apply to other gateways - hostnames := common.FilterValidSubdomains(gwHostnames, httpRoute.Spec.Hostnames) - if len(hostnames) == 0 { // it should only happen when the route specifies no hostnames - hostnames = gwHostnames - } - httpRoute.Spec.Hostnames = hostnames - - wasmPlugin.RateLimitPolicies = append(wasmPlugin.RateLimitPolicies, wasm.RateLimitPolicy{ - Name: client.ObjectKeyFromObject(&rlp).String(), - Domain: common.MarshallNamespace(gw.Key(), string(hostnames[0])), // TODO(guicassolato): https://github.com/Kuadrant/kuadrant-operator/issues/201. Meanwhile, we are using the first hostname so it matches at least one set of limit definitions in the Limitador CR - Rules: rlptools.WasmRules(&rlp, &httpRoute), - Hostnames: common.HostnamesToStrings(hostnames), // we might be listing more hostnames than needed due to route selectors hostnames possibly being more restrictive - Service: common.KuadrantRateLimitClusterName, - }) + for rlpKey, objs := range rlps { + // modifies (in memory) the list of hostnames specified in the route so we don't generate rules that only apply to other gateways + // this is a no-op for the rlp that targets the gateway + httpRoute := objs.httpRoute + hostnames := common.FilterValidSubdomains(gwHostnames, httpRoute.Spec.Hostnames) + if len(hostnames) == 0 { // it should only happen when the route specifies no hostnames + hostnames = gwHostnames } + httpRoute.Spec.Hostnames = hostnames + + wasmPlugin.RateLimitPolicies = append(wasmPlugin.RateLimitPolicies, wasm.RateLimitPolicy{ + Name: rlpKey, + Domain: common.MarshallNamespace(gw.Key(), string(hostnames[0])), // TODO(guicassolato): https://github.com/Kuadrant/kuadrant-operator/issues/201. Meanwhile, we are using the first hostname so it matches at least one set of limit definitions in the Limitador CR + Rules: rlptools.WasmRules(&objs.rlp, &httpRoute), + Hostnames: common.HostnamesToStrings(hostnames), // we might be listing more hostnames than needed due to route selectors hostnames possibly being more restrictive + Service: common.KuadrantRateLimitClusterName, + }) } return wasmPlugin, nil From 5de719fbbbe6a2cfd58b9603be52b926971389d2 Mon Sep 17 00:00:00 2001 From: Guilherme Cassolato Date: Tue, 11 Jul 2023 10:14:19 +0200 Subject: [PATCH 11/15] refactor: make HTTPRouteRuleSelector.Selects return sooner if the HTTP method does not match --- pkg/common/gatewayapi_utils.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/common/gatewayapi_utils.go b/pkg/common/gatewayapi_utils.go index b65e59464..c13ba6964 100644 --- a/pkg/common/gatewayapi_utils.go +++ b/pkg/common/gatewayapi_utils.go @@ -106,6 +106,11 @@ func (s *HTTPRouteRuleSelector) Selects(rule gatewayapiv1beta1.HTTPRouteRule) bo return false } + // method + if s.Method != nil && !reflect.DeepEqual(s.Method, ruleMatch.Method) { + return false + } + // headers for _, header := range s.Headers { if _, found := Find(ruleMatch.Headers, func(otherHeader gatewayapiv1beta1.HTTPHeaderMatch) bool { @@ -124,11 +129,6 @@ func (s *HTTPRouteRuleSelector) Selects(rule gatewayapiv1beta1.HTTPRouteRule) bo } } - // method - if s.Method != nil && !reflect.DeepEqual(s.Method, ruleMatch.Method) { - return false - } - return true }) From 9e38d57bdc32efe9dddf3c6402c52103568b2123 Mon Sep 17 00:00:00 2001 From: Guilherme Cassolato Date: Tue, 11 Jul 2023 11:09:22 +0200 Subject: [PATCH 12/15] refactor: ensure wasm rule conditions are generated out of selected HTTPRouteRules in the same order as the selectors select the rules --- api/v1beta2/route_selectors.go | 15 ++++++---- api/v1beta2/route_selectors_test.go | 2 +- go.mod | 1 + go.sum | 2 ++ pkg/common/common.go | 10 ------- pkg/common/common_test.go | 18 ------------ pkg/rlptools/wasm_utils.go | 3 ++ pkg/rlptools/wasm_utils_test.go | 43 ++--------------------------- 8 files changed, 20 insertions(+), 74 deletions(-) diff --git a/api/v1beta2/route_selectors.go b/api/v1beta2/route_selectors.go index 9e76e5d45..f896a2cdd 100644 --- a/api/v1beta2/route_selectors.go +++ b/api/v1beta2/route_selectors.go @@ -3,6 +3,8 @@ package v1beta2 import ( gatewayapiv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1" + orderedmap "github.com/elliotchance/orderedmap/v2" + "github.com/kuadrant/kuadrant-operator/pkg/common" ) @@ -25,10 +27,10 @@ type RouteSelector struct { // the matches of the selector. If the selector does not specify any matches, then all HTTPRouteRules are selected. // // Additionally, if the selector specifies a non-empty list of hostnames, a non-empty intersection between the literal -// hostnames of the selector and set of hostnames specified in the HTTPRoute must be exist. Otherwise, the function +// hostnames of the selector and set of hostnames specified in the HTTPRoute must exist. Otherwise, the function // returns nil. -func (s *RouteSelector) SelectRules(route *gatewayapiv1beta1.HTTPRoute) []gatewayapiv1beta1.HTTPRouteRule { - rulesIndices := make(map[int]gatewayapiv1beta1.HTTPRouteRule, 0) // using a map is an easy way to avoid repeated rules but it may not preserve the order +func (s *RouteSelector) SelectRules(route *gatewayapiv1beta1.HTTPRoute) (rules []gatewayapiv1beta1.HTTPRouteRule) { + rulesIndices := orderedmap.NewOrderedMap[int, gatewayapiv1beta1.HTTPRouteRule]() if len(s.Hostnames) > 0 && !common.Intersect(s.Hostnames, route.Spec.Hostnames) { return nil } @@ -39,9 +41,12 @@ func (s *RouteSelector) SelectRules(route *gatewayapiv1beta1.HTTPRoute) []gatewa for idx, rule := range route.Spec.Rules { rs := common.HTTPRouteRuleSelector{HTTPRouteMatch: &routeSelectorMatch} if rs.Selects(rule) { - rulesIndices[idx] = rule + rulesIndices.Set(idx, rule) } } } - return common.MapValues(rulesIndices) + for el := rulesIndices.Front(); el != nil; el = el.Next() { + rules = append(rules, el.Value) + } + return } diff --git a/api/v1beta2/route_selectors_test.go b/api/v1beta2/route_selectors_test.go index 0d2e3194e..347dbe20e 100644 --- a/api/v1beta2/route_selectors_test.go +++ b/api/v1beta2/route_selectors_test.go @@ -145,7 +145,7 @@ func TestRouteSelectors(t *testing.T) { }, }, route: route, - expected: []gatewayapiv1beta1.HTTPRouteRule{}, + expected: nil, }, { name: "route selector selects the HTTPRouteRules whose HTTPRoute's hostnames match the selector", diff --git a/go.mod b/go.mod index b1d06abf0..e7505d751 100644 --- a/go.mod +++ b/go.mod @@ -54,6 +54,7 @@ require ( github.com/decred/dcrd/dcrec/secp256k1/v4 v4.0.0-20210816181553-5444fa50b93d // indirect github.com/dgryski/go-rendezvous v0.0.0-20200823014737-9f7001d12a5f // indirect github.com/eko/gocache v1.2.0 // indirect + github.com/elliotchance/orderedmap/v2 v2.2.0 // indirect github.com/emicklei/go-restful/v3 v3.9.0 // indirect github.com/envoyproxy/go-control-plane v0.10.3 // indirect github.com/envoyproxy/protoc-gen-validate v0.9.1 // indirect diff --git a/go.sum b/go.sum index 2a70d3b6d..61ed90c1e 100644 --- a/go.sum +++ b/go.sum @@ -154,6 +154,8 @@ github.com/edsrzf/mmap-go v1.0.0/go.mod h1:YO35OhQPt3KJa3ryjFM5Bs14WD66h8eGKpfaB github.com/eko/gocache v1.2.0/go.mod h1:6u8/2bnr+nOf87mRXWS710rqNNZUECF4CGsPNnsoJ78= github.com/elazarl/goproxy v0.0.0-20170405201442-c4fc26588b6e/go.mod h1:/Zj4wYkgs4iZTTu3o/KG3Itv/qCCa8VVMlb3i9OVuzc= github.com/elazarl/goproxy v0.0.0-20180725130230-947c36da3153/go.mod h1:/Zj4wYkgs4iZTTu3o/KG3Itv/qCCa8VVMlb3i9OVuzc= +github.com/elliotchance/orderedmap/v2 v2.2.0 h1:7/2iwO98kYT4XkOjA9mBEIwvi4KpGB4cyHeOFOnj4Vk= +github.com/elliotchance/orderedmap/v2 v2.2.0/go.mod h1:85lZyVbpGaGvHvnKa7Qhx7zncAdBIBq6u56Hb1PRU5Q= github.com/emicklei/go-restful v0.0.0-20170410110728-ff4f55a20633/go.mod h1:otzb+WCGbkyDHkqmQmT5YD2WR4BBwUdeQoFo8l/7tVs= github.com/emicklei/go-restful v2.9.5+incompatible/go.mod h1:otzb+WCGbkyDHkqmQmT5YD2WR4BBwUdeQoFo8l/7tVs= github.com/emicklei/go-restful/v3 v3.9.0 h1:XwGDlfxEnQZzuopoqxwSEllNcCOM9DhhFyhFIIGKwxE= diff --git a/pkg/common/common.go b/pkg/common/common.go index 8ebe17fbf..e036a3ee3 100644 --- a/pkg/common/common.go +++ b/pkg/common/common.go @@ -188,16 +188,6 @@ func ReverseSlice[T any](input []T) []T { return output } -func MapValues[T comparable, U any](m map[T]U) []U { - values := make([]U, len(m)) - i := 0 - for k := range m { - values[i] = m[k] - i++ - } - return values -} - // MergeMapStringString Merge desired into existing. // Not Thread-Safe. Does it matter? func MergeMapStringString(existing *map[string]string, desired map[string]string) bool { diff --git a/pkg/common/common_test.go b/pkg/common/common_test.go index 50ba540d7..8b181a790 100644 --- a/pkg/common/common_test.go +++ b/pkg/common/common_test.go @@ -625,24 +625,6 @@ func TestReverseSlice(t *testing.T) { }) } -func TestMapValues(t *testing.T) { - t.Run("when given a map of strings to string then return a slice of the string values", func(t *testing.T) { - stos := map[string]string{"a": "foo", "b": "bar", "c": "baz"} - expected := []string{"foo", "bar", "baz"} - if r := MapValues(stos); len(r) != len(expected) || !SameElements(r, expected) { - t.Errorf("expected: %v; got: %v", expected, r) - } - }) - - t.Run("when given a map of strings to ints then return a slice of the int values", func(t *testing.T) { - stos := map[string]int{"a": 1, "b": 2, "c": 3} - expected := []int{1, 2, 3} - if r := MapValues(stos); len(r) != len(expected) || !SameElements(r, expected) { - t.Errorf("expected: %v; got: %v", expected, r) - } - }) -} - func TestMergeMapStringString(t *testing.T) { testCases := []struct { name string diff --git a/pkg/rlptools/wasm_utils.go b/pkg/rlptools/wasm_utils.go index 3dd71df19..5e8d5d3c8 100644 --- a/pkg/rlptools/wasm_utils.go +++ b/pkg/rlptools/wasm_utils.go @@ -84,6 +84,9 @@ func conditionsFromLimit(limit *kuadrantv1beta2.Limit, route *gatewayapiv1beta1. } if len(limit.When) == 0 { + if len(routeConditions) == 0 { + return nil, nil + } return routeConditions, nil } diff --git a/pkg/rlptools/wasm_utils_test.go b/pkg/rlptools/wasm_utils_test.go index f72064e92..fc6b02dd4 100644 --- a/pkg/rlptools/wasm_utils_test.go +++ b/pkg/rlptools/wasm_utils_test.go @@ -3,16 +3,14 @@ package rlptools import ( - "encoding/json" - "reflect" "testing" + "github.com/google/go-cmp/cmp" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" gatewayapiv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1" kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" - "github.com/kuadrant/kuadrant-operator/pkg/common" "github.com/kuadrant/kuadrant-operator/pkg/rlptools/wasm" ) @@ -332,43 +330,8 @@ func TestWasmRules(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { computedRules := WasmRules(tc.rlp, tc.route) - - if len(tc.expectedRules) != len(computedRules) { - t.Errorf("expected %d wasm rules, got (%d)", len(tc.expectedRules), len(computedRules)) - } - - for _, expectedRule := range tc.expectedRules { - if _, ruleFound := common.Find(computedRules, func(computedRule wasm.Rule) bool { - if len(expectedRule.Conditions) != len(computedRule.Conditions) { - return false - } - - // we cannot guarantee the order of the conditions, so we need to find each one - for _, expectedCondition := range expectedRule.Conditions { - if _, conditionFound := common.Find(computedRule.Conditions, func(computedCondition wasm.Condition) bool { - return reflect.DeepEqual(expectedCondition, computedCondition) - }); !conditionFound { - return false - } - } - - if len(expectedRule.Data) != len(computedRule.Data) { - return false - } - - // unlike the conditions, we can guarantee the order of the data items - for i := range expectedRule.Data { - if !reflect.DeepEqual(expectedRule.Data[i], computedRule.Data[i]) { - return false - } - } - - return true - }); !ruleFound { - expectedRuleJSON, _ := json.Marshal(expectedRule) - computedRulesJSON, _ := json.Marshal(computedRules) - t.Errorf("cannot find expected wasm rule: %s in %s", string(expectedRuleJSON), string(computedRulesJSON)) - } + if diff := cmp.Diff(tc.expectedRules, computedRules); diff != "" { + t.Errorf("unexpected wasm rules (-want +got):\n%s", diff) } }) } From 2bf7311846756f6a07dfc31969af0ab3e9eb6882 Mon Sep 17 00:00:00 2001 From: Guilherme Cassolato Date: Tue, 11 Jul 2023 21:30:06 +0200 Subject: [PATCH 13/15] Prevent the usage of routeSelectors in RLPs that target a Gateway --- api/v1beta2/ratelimitpolicy_types.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/api/v1beta2/ratelimitpolicy_types.go b/api/v1beta2/ratelimitpolicy_types.go index 6744bae27..dc7543c9e 100644 --- a/api/v1beta2/ratelimitpolicy_types.go +++ b/api/v1beta2/ratelimitpolicy_types.go @@ -183,6 +183,15 @@ func (r *RateLimitPolicy) Validate() error { return fmt.Errorf("invalid targetRef.Namespace %s. Currently only supporting references to the same namespace", *r.Spec.TargetRef.Namespace) } + // prevents usage of routeSelectors in a gateway RLP + if r.Spec.TargetRef.Kind == gatewayapiv1alpha2.Kind("Gateway") { + for _, limit := range r.Spec.Limits { + if len(limit.RouteSelectors) > 0 { + return fmt.Errorf("route selectors not supported when targetting a Gateway") + } + } + } + return nil } From 98cbe4a19750799ddabec5af0945b100bfe0a0b3 Mon Sep 17 00:00:00 2001 From: Guilherme Cassolato Date: Wed, 12 Jul 2023 12:25:26 +0200 Subject: [PATCH 14/15] 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 --- .../ratelimitpolicy_controller_test.go | 11 +-- controllers/ratelimitpolicy_wasm_plugins.go | 84 ++++++++++++------- pkg/reconcilers/targetref_reconciler.go | 4 +- 3 files changed, 57 insertions(+), 42 deletions(-) diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index bb5fe1ce0..d25d0025c 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -13,6 +13,7 @@ import ( istioclientgoextensionv1alpha1 "istio.io/client-go/pkg/apis/extensions/v1alpha1" istioclientnetworkingv1alpha3 "istio.io/client-go/pkg/apis/networking/v1alpha3" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -687,15 +688,9 @@ var _ = Describe("RateLimitPolicy controller", func() { wpName := fmt.Sprintf("kuadrant-%s", gwName) wasmPluginKey := client.ObjectKey{Name: wpName, Namespace: testNamespace} existingWasmPlugin := &istioclientgoextensionv1alpha1.WasmPlugin{} + // must not exist err = k8sClient.Get(context.Background(), wasmPluginKey, existingWasmPlugin) - // must exist - Expect(err).ToNot(HaveOccurred()) - existingWASMConfig, err := rlptools.WASMPluginFromStruct(existingWasmPlugin.Spec.PluginConfig) - Expect(err).ToNot(HaveOccurred()) - Expect(existingWASMConfig).To(Equal(&wasm.WASMPlugin{ - FailureMode: wasm.FailureModeDeny, - RateLimitPolicies: []wasm.RateLimitPolicy{}, - })) + Expect(apierrors.IsNotFound(err)).To(BeTrue()) // Check gateway back references err = k8sClient.Get(context.Background(), gwKey, existingGateway) diff --git a/controllers/ratelimitpolicy_wasm_plugins.go b/controllers/ratelimitpolicy_wasm_plugins.go index 32a05ad01..4b8bbc633 100644 --- a/controllers/ratelimitpolicy_wasm_plugins.go +++ b/controllers/ratelimitpolicy_wasm_plugins.go @@ -124,17 +124,16 @@ func (r *RateLimitPolicyReconciler) wasmPluginConfig(ctx context.Context, gw com logger, _ := logr.FromContext(ctx) logger = logger.WithName("wasmPluginConfig").WithValues("gateway", gw.Key()) - gwHostnames := gw.Hostnames() - if len(gwHostnames) == 0 { - gwHostnames = []gatewayapiv1beta1.Hostname{"*"} + type store struct { + rlp kuadrantv1beta2.RateLimitPolicy + route gatewayapiv1beta1.HTTPRoute + skip bool } + rlps := make(map[string]*store, len(rlpRefs)) + routeKeys := make(map[string]struct{}, 0) + var gwRLPKey string - type objects struct { - rlp kuadrantv1beta2.RateLimitPolicy - httpRoute gatewayapiv1beta1.HTTPRoute - } - rlps := make(map[string]*objects, 0) - + // store all rlps and find the one that targets the gateway (if there is one) for _, rlpKey := range rlpRefs { rlp := &kuadrantv1beta2.RateLimitPolicy{} err := r.Client().Get(ctx, rlpKey, rlp) @@ -145,11 +144,12 @@ func (r *RateLimitPolicyReconciler) wasmPluginConfig(ctx context.Context, gw com // target ref is a HTTPRoute if common.IsTargetRefHTTPRoute(rlp.Spec.TargetRef) { - httpRoute, err := r.FetchValidHTTPRoute(ctx, rlp.TargetKey()) + route, err := r.FetchValidHTTPRoute(ctx, rlp.TargetKey()) if err != nil { return nil, err } - rlps[rlpKey.String()] = &objects{rlp: *rlp, httpRoute: *httpRoute} + rlps[rlpKey.String()] = &store{rlp: *rlp, route: *route} + routeKeys[client.ObjectKeyFromObject(route).String()] = struct{}{} continue } @@ -157,27 +157,36 @@ func (r *RateLimitPolicyReconciler) wasmPluginConfig(ctx context.Context, gw com if rlps[rlpKey.String()] != nil { return nil, fmt.Errorf("wasmPluginConfig: multiple gateway RLP found and only one expected. rlp keys: %v", rlpRefs) } - // build a single httproute with all rules from all httproutes attached to the gateway and the hostnames of the gateway. - // the hostnames of the gateway are used because, otherwise, the rlp that targets the gateway would have to specify - // hostnames for route selection exactly as they are stated in the httproutes, not as they stated in the gateway, - // and this would be counterintuitive for the user, because, unlike other types of route selection (e.g. by path), - // the hostnames are part of the gateway spec. + gwRLPKey = rlpKey.String() + rlps[gwRLPKey] = &store{rlp: *rlp} + } + + gwHostnames := gw.Hostnames() + if len(gwHostnames) == 0 { + gwHostnames = []gatewayapiv1beta1.Hostname{"*"} + } + + // if there is a gateway rlp, fake a single httproute with all rules from all httproutes accepted by the gateway, + // that do not have a rlp of its own, so we can generate wasm rules for those cases + if gwRLPKey != "" { rules := make([]gatewayapiv1beta1.HTTPRouteRule, 0) - for _, route := range r.FetchAcceptedGatewayHTTPRoutes(ctx, rlp.TargetKey()) { + for _, route := range r.FetchAcceptedGatewayHTTPRoutes(ctx, rlps[gwRLPKey].rlp.TargetKey()) { + // skip routes that have a rlp of its own + if _, found := routeKeys[client.ObjectKeyFromObject(&route).String()]; found { + continue + } rules = append(rules, route.Spec.Rules...) } if len(rules) == 0 { - logger.V(1).Info("no httproutes attached to the targeted gateway, skipping wasm config for the rlp", "ratelimitpolicy", rlpKey) - continue - } - rlps[rlpKey.String()] = &objects{ - rlp: *rlp, - httpRoute: gatewayapiv1beta1.HTTPRoute{ + logger.V(1).Info("no httproutes attached to the targeted gateway, skipping wasm config for the gateway rlp", "ratelimitpolicy", gwRLPKey) + rlps[gwRLPKey].skip = true + } else { + rlps[gwRLPKey].route = gatewayapiv1beta1.HTTPRoute{ Spec: gatewayapiv1beta1.HTTPRouteSpec{ Hostnames: gwHostnames, Rules: rules, }, - }, + } } } @@ -186,24 +195,35 @@ func (r *RateLimitPolicyReconciler) wasmPluginConfig(ctx context.Context, gw com RateLimitPolicies: make([]wasm.RateLimitPolicy, 0), } - for rlpKey, objs := range rlps { - // modifies (in memory) the list of hostnames specified in the route so we don't generate rules that only apply to other gateways - // this is a no-op for the rlp that targets the gateway - httpRoute := objs.httpRoute - hostnames := common.FilterValidSubdomains(gwHostnames, httpRoute.Spec.Hostnames) + for _, rlpKey := range rlpRefs { + s := rlps[rlpKey.String()] + if s.skip { + continue + } + rlp := s.rlp + route := s.route + + // narrow the list of hostnames specified in the route so we don't generate wasm rules that only apply to other gateways + // this is a no-op for the gateway rlp + hostnames := common.FilterValidSubdomains(gwHostnames, route.Spec.Hostnames) if len(hostnames) == 0 { // it should only happen when the route specifies no hostnames hostnames = gwHostnames } - httpRoute.Spec.Hostnames = hostnames + route.Spec.Hostnames = hostnames wasmPlugin.RateLimitPolicies = append(wasmPlugin.RateLimitPolicies, wasm.RateLimitPolicy{ - Name: rlpKey, + Name: rlpKey.String(), Domain: common.MarshallNamespace(gw.Key(), string(hostnames[0])), // TODO(guicassolato): https://github.com/Kuadrant/kuadrant-operator/issues/201. Meanwhile, we are using the first hostname so it matches at least one set of limit definitions in the Limitador CR - Rules: rlptools.WasmRules(&objs.rlp, &httpRoute), + Rules: rlptools.WasmRules(&rlp, &route), Hostnames: common.HostnamesToStrings(hostnames), // we might be listing more hostnames than needed due to route selectors hostnames possibly being more restrictive Service: common.KuadrantRateLimitClusterName, }) } + // avoid building a wasm plugin config if there are no rules to apply + if len(wasmPlugin.RateLimitPolicies) == 0 { + return nil, nil + } + return wasmPlugin, nil } diff --git a/pkg/reconcilers/targetref_reconciler.go b/pkg/reconcilers/targetref_reconciler.go index d5d3b5613..a4bbc3e78 100644 --- a/pkg/reconcilers/targetref_reconciler.go +++ b/pkg/reconcilers/targetref_reconciler.go @@ -95,7 +95,7 @@ func (r *TargetRefReconciler) FetchValidTargetRef(ctx context.Context, targetRef } // FetchAcceptedGatewayHTTPRoutes returns the list of HTTPRoutes that have been accepted as children of a gateway. -func (r *TargetRefReconciler) FetchAcceptedGatewayHTTPRoutes(ctx context.Context, gwKey client.ObjectKey) (routeKeys []gatewayapiv1beta1.HTTPRoute) { +func (r *TargetRefReconciler) FetchAcceptedGatewayHTTPRoutes(ctx context.Context, gwKey client.ObjectKey) (routes []gatewayapiv1beta1.HTTPRoute) { logger, _ := logr.FromContext(ctx) logger = logger.WithName("FetchAcceptedGatewayHTTPRoutes").WithValues("gateway", gwKey) @@ -114,7 +114,7 @@ func (r *TargetRefReconciler) FetchAcceptedGatewayHTTPRoutes(ctx context.Context }) if found && meta.IsStatusConditionTrue(routeParentStatus.Conditions, "Accepted") { logger.V(1).Info("found route attached to gateway", "httproute", client.ObjectKeyFromObject(&route)) - routeKeys = append(routeKeys, route) + routes = append(routes, route) continue } logger.V(1).Info("skipping route, not attached to gateway", "httproute", client.ObjectKeyFromObject(&route)) From a7ed47865d4ac4f6ed1d08725db7a485cb3f41c1 Mon Sep 17 00:00:00 2001 From: Guilherme Cassolato Date: Tue, 18 Jul 2023 17:02:28 +0200 Subject: [PATCH 15/15] avoid adding rlps without any rules to the wasm config --- controllers/ratelimitpolicy_wasm_plugins.go | 7 ++++++- pkg/rlptools/wasm_utils.go | 4 ++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/controllers/ratelimitpolicy_wasm_plugins.go b/controllers/ratelimitpolicy_wasm_plugins.go index 4b8bbc633..22f191bbb 100644 --- a/controllers/ratelimitpolicy_wasm_plugins.go +++ b/controllers/ratelimitpolicy_wasm_plugins.go @@ -211,10 +211,15 @@ func (r *RateLimitPolicyReconciler) wasmPluginConfig(ctx context.Context, gw com } route.Spec.Hostnames = hostnames + rules := rlptools.WasmRules(&rlp, &route) + if len(rules) == 0 { + continue // no need to add the policy if there are no rules; a rlp can return no rules if all its limits fail to match any route rule + } + wasmPlugin.RateLimitPolicies = append(wasmPlugin.RateLimitPolicies, wasm.RateLimitPolicy{ Name: rlpKey.String(), Domain: common.MarshallNamespace(gw.Key(), string(hostnames[0])), // TODO(guicassolato): https://github.com/Kuadrant/kuadrant-operator/issues/201. Meanwhile, we are using the first hostname so it matches at least one set of limit definitions in the Limitador CR - Rules: rlptools.WasmRules(&rlp, &route), + Rules: rules, Hostnames: common.HostnamesToStrings(hostnames), // we might be listing more hostnames than needed due to route selectors hostnames possibly being more restrictive Service: common.KuadrantRateLimitClusterName, }) diff --git a/pkg/rlptools/wasm_utils.go b/pkg/rlptools/wasm_utils.go index 5e8d5d3c8..ca6b28d98 100644 --- a/pkg/rlptools/wasm_utils.go +++ b/pkg/rlptools/wasm_utils.go @@ -22,6 +22,8 @@ var ( ) // WasmRules computes WASM rules from the policy and the targeted route. +// It returns an empty list of wasm rules if the policy specifies no limits or if all limits specified in the policy +// fail to match any route rule according to the limits route selectors. func WasmRules(rlp *kuadrantv1beta2.RateLimitPolicy, route *gatewayapiv1beta1.HTTPRoute) []wasm.Rule { rules := make([]wasm.Rule, 0) if rlp == nil { @@ -34,8 +36,6 @@ func WasmRules(rlp *kuadrantv1beta2.RateLimitPolicy, route *gatewayapiv1beta1.HT rule, err := ruleFromLimit(limitFullName, &limit, route) if err == nil { rules = append(rules, rule) - } else { - // TODO(guicassolato): log the error } }