From 2f3d1bd9f8c1c4cbf61d26261378a9510599ca38 Mon Sep 17 00:00:00 2001 From: KevFan Date: Wed, 10 Apr 2024 15:52:50 +0100 Subject: [PATCH 01/14] feat: apply RLP gateway overrides --- api/v1beta2/ratelimitpolicy_types.go | 5 + api/v1beta2/zz_generated.deepcopy.go | 5 + .../kuadrant.io_ratelimitpolicies.yaml | 394 ++++++++++++++++++ .../bases/kuadrant.io_ratelimitpolicies.yaml | 394 ++++++++++++++++++ controllers/ratelimitpolicy_controller.go | 4 +- .../ratelimitpolicy_controller_test.go | 232 +++++++++++ controllers/ratelimitpolicy_limits.go | 46 +- 7 files changed, 1071 insertions(+), 9 deletions(-) diff --git a/api/v1beta2/ratelimitpolicy_types.go b/api/v1beta2/ratelimitpolicy_types.go index 5788c4b1a..a13a9e8f4 100644 --- a/api/v1beta2/ratelimitpolicy_types.go +++ b/api/v1beta2/ratelimitpolicy_types.go @@ -133,6 +133,11 @@ type RateLimitPolicySpec struct { // +optional Defaults *RateLimitPolicyCommonSpec `json:"defaults,omitempty"` + // Overrides define override values for this policy and for policies inheriting this policy. + // Defaults are mutually exclusive with implicit defaults defined by RateLimitPolicyCommonSpec. + // +optional + Overrides *RateLimitPolicyCommonSpec `json:"overrides,omitempty"` + // RateLimitPolicyCommonSpec defines implicit default values for this policy and for policies inheriting this policy. // RateLimitPolicyCommonSpec is mutually exclusive with explicit defaults defined by Defaults. RateLimitPolicyCommonSpec `json:""` diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go index 95828e596..747f6278c 100644 --- a/api/v1beta2/zz_generated.deepcopy.go +++ b/api/v1beta2/zz_generated.deepcopy.go @@ -481,6 +481,11 @@ func (in *RateLimitPolicySpec) DeepCopyInto(out *RateLimitPolicySpec) { *out = new(RateLimitPolicyCommonSpec) (*in).DeepCopyInto(*out) } + if in.Overrides != nil { + in, out := &in.Overrides, &out.Overrides + *out = new(RateLimitPolicyCommonSpec) + (*in).DeepCopyInto(*out) + } in.RateLimitPolicyCommonSpec.DeepCopyInto(&out.RateLimitPolicyCommonSpec) } diff --git a/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml b/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml index 6fb2a0500..f869d3640 100644 --- a/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml +++ b/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml @@ -847,6 +847,400 @@ spec: name maxProperties: 14 type: object + overrides: + description: |- + Overrides define override values for this policy and for policies inheriting this policy. + Defaults are mutually exclusive with implicit defaults defined by RateLimitPolicyCommonSpec. + properties: + limits: + additionalProperties: + description: Limit represents a complete rate limit configuration + properties: + counters: + description: |- + Counters defines additional rate limit counters based on context qualifiers and well known selectors + TODO Document properly "Well-known selector" https://github.com/Kuadrant/architecture/blob/main/rfcs/0001-rlp-v2.md#well-known-selectors + items: + description: |- + ContextSelector defines one item from the well known attributes + Attributes: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/advanced/attributes + Well-known selectors: https://github.com/Kuadrant/architecture/blob/main/rfcs/0001-rlp-v2.md#well-known-selectors + They are named by a dot-separated path (e.g. request.path) + Example: "request.path" -> The path portion of the URL + maxLength: 253 + minLength: 1 + type: string + type: array + rates: + description: Rates holds the list of limit rates + items: + description: Rate defines the actual rate limit that will + be used when there is a match + properties: + duration: + description: Duration defines the time period for + which the Limit specified above applies. + type: integer + limit: + description: Limit defines the max value allowed for + a given period of time + type: integer + unit: + description: |- + Duration defines the time uni + Possible values are: "second", "minute", "hour", "day" + enum: + - second + - minute + - hour + - day + type: string + required: + - duration + - limit + - unit + type: object + type: array + routeSelectors: + description: RouteSelectors defines semantics for matching + an HTTP request based on conditions + items: + description: |- + RouteSelector defines semantics for matching an HTTP request based on conditions + https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io/v1.HTTPRouteSpec + properties: + hostnames: + description: |- + 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/reference/spec/#gateway.networking.k8s.io/v1.HTTPRouteSpec + items: + description: |- + Hostname is the fully qualified domain name of a network host. This matches + the RFC 1123 definition of a hostname with 2 notable exceptions: + + + 1. IPs are not allowed. + 2. A hostname may be prefixed with a wildcard label (`*.`). The wildcard + label must appear by itself as the first label. + + + Hostname can be "precise" which is a domain name without the terminating + dot of a network host (e.g. "foo.example.com") or "wildcard", which is a + domain name prefixed with a single wildcard label (e.g. `*.example.com`). + + + Note that as per RFC1035 and RFC1123, a *label* must consist of lower case + alphanumeric characters or '-', and must start and end with an alphanumeric + character. No other punctuation is allowed. + maxLength: 253 + minLength: 1 + pattern: ^(\*\.)?[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ + type: string + type: array + matches: + description: |- + Matches define conditions used for matching the rule against incoming HTTP requests. + https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io/v1.HTTPRouteSpec + items: + description: "HTTPRouteMatch defines the predicate + used to match requests to a given\naction. Multiple + match types are ANDed together, i.e. the match + will\nevaluate to true only if all conditions + are satisfied.\n\n\nFor example, the match below + will match a HTTP request only if its path\nstarts + with `/foo` AND it contains the `version: v1` + header:\n\n\n```\nmatch:\n\n\n\tpath:\n\t value: + \"/foo\"\n\theaders:\n\t- name: \"version\"\n\t + \ value \"v1\"\n\n\n```" + properties: + headers: + description: |- + Headers specifies HTTP request header matchers. Multiple match values are + ANDed together, meaning, a request must match all the specified headers + to select the route. + items: + description: |- + HTTPHeaderMatch describes how to select a HTTP route by matching HTTP request + headers. + properties: + name: + description: |- + Name is the name of the HTTP Header to be matched. Name matching MUST be + case insensitive. (See https://tools.ietf.org/html/rfc7230#section-3.2). + + + If multiple entries specify equivalent header names, only the first + entry with an equivalent name MUST be considered for a match. Subsequent + entries with an equivalent header name MUST be ignored. Due to the + case-insensitivity of header names, "foo" and "Foo" are considered + equivalent. + + + When a header is repeated in an HTTP request, it is + implementation-specific behavior as to how this is represented. + Generally, proxies should follow the guidance from the RFC: + https://www.rfc-editor.org/rfc/rfc7230.html#section-3.2.2 regarding + processing a repeated header, with special handling for "Set-Cookie". + maxLength: 256 + minLength: 1 + pattern: ^[A-Za-z0-9!#$%&'*+\-.^_\x60|~]+$ + type: string + type: + default: Exact + description: |- + Type specifies how to match against the value of the header. + + + Support: Core (Exact) + + + Support: Implementation-specific (RegularExpression) + + + Since RegularExpression HeaderMatchType has implementation-specific + conformance, implementations can support POSIX, PCRE or any other dialects + of regular expressions. Please read the implementation's documentation to + determine the supported dialect. + enum: + - Exact + - RegularExpression + type: string + value: + description: Value is the value of HTTP + Header to be matched. + maxLength: 4096 + minLength: 1 + type: string + required: + - name + - value + type: object + maxItems: 16 + type: array + x-kubernetes-list-map-keys: + - name + x-kubernetes-list-type: map + method: + description: |- + Method specifies HTTP method matcher. + When specified, this route will be matched only if the request has the + specified method. + + + Support: Extended + enum: + - GET + - HEAD + - POST + - PUT + - DELETE + - CONNECT + - OPTIONS + - TRACE + - PATCH + type: string + path: + default: + type: PathPrefix + value: / + description: |- + Path specifies a HTTP request path matcher. If this field is not + specified, a default prefix match on the "/" path is provided. + properties: + type: + default: PathPrefix + description: |- + Type specifies how to match against the path Value. + + + Support: Core (Exact, PathPrefix) + + + Support: Implementation-specific (RegularExpression) + enum: + - Exact + - PathPrefix + - RegularExpression + type: string + value: + default: / + description: Value of the HTTP path to match + against. + maxLength: 1024 + type: string + type: object + x-kubernetes-validations: + - message: value must be an absolute path and + start with '/' when type one of ['Exact', + 'PathPrefix'] + rule: '(self.type in [''Exact'',''PathPrefix'']) + ? self.value.startsWith(''/'') : true' + - message: must not contain '//' when type one + of ['Exact', 'PathPrefix'] + rule: '(self.type in [''Exact'',''PathPrefix'']) + ? !self.value.contains(''//'') : true' + - message: must not contain '/./' when type + one of ['Exact', 'PathPrefix'] + rule: '(self.type in [''Exact'',''PathPrefix'']) + ? !self.value.contains(''/./'') : true' + - message: must not contain '/../' when type + one of ['Exact', 'PathPrefix'] + rule: '(self.type in [''Exact'',''PathPrefix'']) + ? !self.value.contains(''/../'') : true' + - message: must not contain '%2f' when type + one of ['Exact', 'PathPrefix'] + rule: '(self.type in [''Exact'',''PathPrefix'']) + ? !self.value.contains(''%2f'') : true' + - message: must not contain '%2F' when type + one of ['Exact', 'PathPrefix'] + rule: '(self.type in [''Exact'',''PathPrefix'']) + ? !self.value.contains(''%2F'') : true' + - message: must not contain '#' when type one + of ['Exact', 'PathPrefix'] + rule: '(self.type in [''Exact'',''PathPrefix'']) + ? !self.value.contains(''#'') : true' + - message: must not end with '/..' when type + one of ['Exact', 'PathPrefix'] + rule: '(self.type in [''Exact'',''PathPrefix'']) + ? !self.value.endsWith(''/..'') : true' + - message: must not end with '/.' when type + one of ['Exact', 'PathPrefix'] + rule: '(self.type in [''Exact'',''PathPrefix'']) + ? !self.value.endsWith(''/.'') : true' + - message: type must be one of ['Exact', 'PathPrefix', + 'RegularExpression'] + rule: self.type in ['Exact','PathPrefix'] + || self.type == 'RegularExpression' + - message: must only contain valid characters + (matching ^(?:[-A-Za-z0-9/._~!$&'()*+,;=:@]|[%][0-9a-fA-F]{2})+$) + for types ['Exact', 'PathPrefix'] + rule: '(self.type in [''Exact'',''PathPrefix'']) + ? self.value.matches(r"""^(?:[-A-Za-z0-9/._~!$&''()*+,;=:@]|[%][0-9a-fA-F]{2})+$""") + : true' + queryParams: + description: |- + QueryParams specifies HTTP query parameter matchers. Multiple match + values are ANDed together, meaning, a request must match all the + specified query parameters to select the route. + + + Support: Extended + items: + description: |- + HTTPQueryParamMatch describes how to select a HTTP route by matching HTTP + query parameters. + properties: + name: + description: |- + Name is the name of the HTTP query param to be matched. This must be an + exact string match. (See + https://tools.ietf.org/html/rfc7230#section-2.7.3). + + + If multiple entries specify equivalent query param names, only the first + entry with an equivalent name MUST be considered for a match. Subsequent + entries with an equivalent query param name MUST be ignored. + + + If a query param is repeated in an HTTP request, the behavior is + purposely left undefined, since different data planes have different + capabilities. However, it is *recommended* that implementations should + match against the first value of the param if the data plane supports it, + as this behavior is expected in other load balancing contexts outside of + the Gateway API. + + + Users SHOULD NOT route traffic based on repeated query params to guard + themselves against potential differences in the implementations. + maxLength: 256 + minLength: 1 + pattern: ^[A-Za-z0-9!#$%&'*+\-.^_\x60|~]+$ + type: string + type: + default: Exact + description: |- + Type specifies how to match against the value of the query parameter. + + + Support: Extended (Exact) + + + Support: Implementation-specific (RegularExpression) + + + Since RegularExpression QueryParamMatchType has Implementation-specific + conformance, implementations can support POSIX, PCRE or any other + dialects of regular expressions. Please read the implementation's + documentation to determine the supported dialect. + enum: + - Exact + - RegularExpression + type: string + value: + description: Value is the value of HTTP + query param to be matched. + maxLength: 1024 + minLength: 1 + type: string + required: + - name + - value + type: object + maxItems: 16 + type: array + x-kubernetes-list-map-keys: + - name + x-kubernetes-list-type: map + type: object + maxItems: 8 + type: array + type: object + maxItems: 15 + type: array + when: + description: |- + When holds the list of conditions for the policy to be enforced. + Called also "soft" conditions as route selectors must also match + items: + description: |- + RouteSelector defines semantics for matching an HTTP request based on conditions + https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io/v1.HTTPRouteSpec + properties: + operator: + description: |- + The binary operator to be applied to the content fetched from the selector + Possible values are: "eq" (equal to), "neq" (not equal to) + enum: + - eq + - neq + - startswith + - endswith + - incl + - excl + - matches + type: string + selector: + description: |- + Selector defines one item from the well known selectors + TODO Document properly "Well-known selector" https://github.com/Kuadrant/architecture/blob/main/rfcs/0001-rlp-v2.md#well-known-selectors + maxLength: 253 + minLength: 1 + type: string + value: + description: The value of reference for the comparison. + type: string + required: + - operator + - selector + - value + type: object + type: array + type: object + description: Limits holds the struct of limits indexed by a unique + name + maxProperties: 14 + type: object + type: object targetRef: description: TargetRef identifies an API object to apply policy to. properties: diff --git a/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml b/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml index 4b6f276ca..603f8bb4b 100644 --- a/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml +++ b/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml @@ -846,6 +846,400 @@ spec: name maxProperties: 14 type: object + overrides: + description: |- + Overrides define override values for this policy and for policies inheriting this policy. + Defaults are mutually exclusive with implicit defaults defined by RateLimitPolicyCommonSpec. + properties: + limits: + additionalProperties: + description: Limit represents a complete rate limit configuration + properties: + counters: + description: |- + Counters defines additional rate limit counters based on context qualifiers and well known selectors + TODO Document properly "Well-known selector" https://github.com/Kuadrant/architecture/blob/main/rfcs/0001-rlp-v2.md#well-known-selectors + items: + description: |- + ContextSelector defines one item from the well known attributes + Attributes: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/advanced/attributes + Well-known selectors: https://github.com/Kuadrant/architecture/blob/main/rfcs/0001-rlp-v2.md#well-known-selectors + They are named by a dot-separated path (e.g. request.path) + Example: "request.path" -> The path portion of the URL + maxLength: 253 + minLength: 1 + type: string + type: array + rates: + description: Rates holds the list of limit rates + items: + description: Rate defines the actual rate limit that will + be used when there is a match + properties: + duration: + description: Duration defines the time period for + which the Limit specified above applies. + type: integer + limit: + description: Limit defines the max value allowed for + a given period of time + type: integer + unit: + description: |- + Duration defines the time uni + Possible values are: "second", "minute", "hour", "day" + enum: + - second + - minute + - hour + - day + type: string + required: + - duration + - limit + - unit + type: object + type: array + routeSelectors: + description: RouteSelectors defines semantics for matching + an HTTP request based on conditions + items: + description: |- + RouteSelector defines semantics for matching an HTTP request based on conditions + https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io/v1.HTTPRouteSpec + properties: + hostnames: + description: |- + 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/reference/spec/#gateway.networking.k8s.io/v1.HTTPRouteSpec + items: + description: |- + Hostname is the fully qualified domain name of a network host. This matches + the RFC 1123 definition of a hostname with 2 notable exceptions: + + + 1. IPs are not allowed. + 2. A hostname may be prefixed with a wildcard label (`*.`). The wildcard + label must appear by itself as the first label. + + + Hostname can be "precise" which is a domain name without the terminating + dot of a network host (e.g. "foo.example.com") or "wildcard", which is a + domain name prefixed with a single wildcard label (e.g. `*.example.com`). + + + Note that as per RFC1035 and RFC1123, a *label* must consist of lower case + alphanumeric characters or '-', and must start and end with an alphanumeric + character. No other punctuation is allowed. + maxLength: 253 + minLength: 1 + pattern: ^(\*\.)?[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ + type: string + type: array + matches: + description: |- + Matches define conditions used for matching the rule against incoming HTTP requests. + https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io/v1.HTTPRouteSpec + items: + description: "HTTPRouteMatch defines the predicate + used to match requests to a given\naction. Multiple + match types are ANDed together, i.e. the match + will\nevaluate to true only if all conditions + are satisfied.\n\n\nFor example, the match below + will match a HTTP request only if its path\nstarts + with `/foo` AND it contains the `version: v1` + header:\n\n\n```\nmatch:\n\n\n\tpath:\n\t value: + \"/foo\"\n\theaders:\n\t- name: \"version\"\n\t + \ value \"v1\"\n\n\n```" + properties: + headers: + description: |- + Headers specifies HTTP request header matchers. Multiple match values are + ANDed together, meaning, a request must match all the specified headers + to select the route. + items: + description: |- + HTTPHeaderMatch describes how to select a HTTP route by matching HTTP request + headers. + properties: + name: + description: |- + Name is the name of the HTTP Header to be matched. Name matching MUST be + case insensitive. (See https://tools.ietf.org/html/rfc7230#section-3.2). + + + If multiple entries specify equivalent header names, only the first + entry with an equivalent name MUST be considered for a match. Subsequent + entries with an equivalent header name MUST be ignored. Due to the + case-insensitivity of header names, "foo" and "Foo" are considered + equivalent. + + + When a header is repeated in an HTTP request, it is + implementation-specific behavior as to how this is represented. + Generally, proxies should follow the guidance from the RFC: + https://www.rfc-editor.org/rfc/rfc7230.html#section-3.2.2 regarding + processing a repeated header, with special handling for "Set-Cookie". + maxLength: 256 + minLength: 1 + pattern: ^[A-Za-z0-9!#$%&'*+\-.^_\x60|~]+$ + type: string + type: + default: Exact + description: |- + Type specifies how to match against the value of the header. + + + Support: Core (Exact) + + + Support: Implementation-specific (RegularExpression) + + + Since RegularExpression HeaderMatchType has implementation-specific + conformance, implementations can support POSIX, PCRE or any other dialects + of regular expressions. Please read the implementation's documentation to + determine the supported dialect. + enum: + - Exact + - RegularExpression + type: string + value: + description: Value is the value of HTTP + Header to be matched. + maxLength: 4096 + minLength: 1 + type: string + required: + - name + - value + type: object + maxItems: 16 + type: array + x-kubernetes-list-map-keys: + - name + x-kubernetes-list-type: map + method: + description: |- + Method specifies HTTP method matcher. + When specified, this route will be matched only if the request has the + specified method. + + + Support: Extended + enum: + - GET + - HEAD + - POST + - PUT + - DELETE + - CONNECT + - OPTIONS + - TRACE + - PATCH + type: string + path: + default: + type: PathPrefix + value: / + description: |- + Path specifies a HTTP request path matcher. If this field is not + specified, a default prefix match on the "/" path is provided. + properties: + type: + default: PathPrefix + description: |- + Type specifies how to match against the path Value. + + + Support: Core (Exact, PathPrefix) + + + Support: Implementation-specific (RegularExpression) + enum: + - Exact + - PathPrefix + - RegularExpression + type: string + value: + default: / + description: Value of the HTTP path to match + against. + maxLength: 1024 + type: string + type: object + x-kubernetes-validations: + - message: value must be an absolute path and + start with '/' when type one of ['Exact', + 'PathPrefix'] + rule: '(self.type in [''Exact'',''PathPrefix'']) + ? self.value.startsWith(''/'') : true' + - message: must not contain '//' when type one + of ['Exact', 'PathPrefix'] + rule: '(self.type in [''Exact'',''PathPrefix'']) + ? !self.value.contains(''//'') : true' + - message: must not contain '/./' when type + one of ['Exact', 'PathPrefix'] + rule: '(self.type in [''Exact'',''PathPrefix'']) + ? !self.value.contains(''/./'') : true' + - message: must not contain '/../' when type + one of ['Exact', 'PathPrefix'] + rule: '(self.type in [''Exact'',''PathPrefix'']) + ? !self.value.contains(''/../'') : true' + - message: must not contain '%2f' when type + one of ['Exact', 'PathPrefix'] + rule: '(self.type in [''Exact'',''PathPrefix'']) + ? !self.value.contains(''%2f'') : true' + - message: must not contain '%2F' when type + one of ['Exact', 'PathPrefix'] + rule: '(self.type in [''Exact'',''PathPrefix'']) + ? !self.value.contains(''%2F'') : true' + - message: must not contain '#' when type one + of ['Exact', 'PathPrefix'] + rule: '(self.type in [''Exact'',''PathPrefix'']) + ? !self.value.contains(''#'') : true' + - message: must not end with '/..' when type + one of ['Exact', 'PathPrefix'] + rule: '(self.type in [''Exact'',''PathPrefix'']) + ? !self.value.endsWith(''/..'') : true' + - message: must not end with '/.' when type + one of ['Exact', 'PathPrefix'] + rule: '(self.type in [''Exact'',''PathPrefix'']) + ? !self.value.endsWith(''/.'') : true' + - message: type must be one of ['Exact', 'PathPrefix', + 'RegularExpression'] + rule: self.type in ['Exact','PathPrefix'] + || self.type == 'RegularExpression' + - message: must only contain valid characters + (matching ^(?:[-A-Za-z0-9/._~!$&'()*+,;=:@]|[%][0-9a-fA-F]{2})+$) + for types ['Exact', 'PathPrefix'] + rule: '(self.type in [''Exact'',''PathPrefix'']) + ? self.value.matches(r"""^(?:[-A-Za-z0-9/._~!$&''()*+,;=:@]|[%][0-9a-fA-F]{2})+$""") + : true' + queryParams: + description: |- + QueryParams specifies HTTP query parameter matchers. Multiple match + values are ANDed together, meaning, a request must match all the + specified query parameters to select the route. + + + Support: Extended + items: + description: |- + HTTPQueryParamMatch describes how to select a HTTP route by matching HTTP + query parameters. + properties: + name: + description: |- + Name is the name of the HTTP query param to be matched. This must be an + exact string match. (See + https://tools.ietf.org/html/rfc7230#section-2.7.3). + + + If multiple entries specify equivalent query param names, only the first + entry with an equivalent name MUST be considered for a match. Subsequent + entries with an equivalent query param name MUST be ignored. + + + If a query param is repeated in an HTTP request, the behavior is + purposely left undefined, since different data planes have different + capabilities. However, it is *recommended* that implementations should + match against the first value of the param if the data plane supports it, + as this behavior is expected in other load balancing contexts outside of + the Gateway API. + + + Users SHOULD NOT route traffic based on repeated query params to guard + themselves against potential differences in the implementations. + maxLength: 256 + minLength: 1 + pattern: ^[A-Za-z0-9!#$%&'*+\-.^_\x60|~]+$ + type: string + type: + default: Exact + description: |- + Type specifies how to match against the value of the query parameter. + + + Support: Extended (Exact) + + + Support: Implementation-specific (RegularExpression) + + + Since RegularExpression QueryParamMatchType has Implementation-specific + conformance, implementations can support POSIX, PCRE or any other + dialects of regular expressions. Please read the implementation's + documentation to determine the supported dialect. + enum: + - Exact + - RegularExpression + type: string + value: + description: Value is the value of HTTP + query param to be matched. + maxLength: 1024 + minLength: 1 + type: string + required: + - name + - value + type: object + maxItems: 16 + type: array + x-kubernetes-list-map-keys: + - name + x-kubernetes-list-type: map + type: object + maxItems: 8 + type: array + type: object + maxItems: 15 + type: array + when: + description: |- + When holds the list of conditions for the policy to be enforced. + Called also "soft" conditions as route selectors must also match + items: + description: |- + RouteSelector defines semantics for matching an HTTP request based on conditions + https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io/v1.HTTPRouteSpec + properties: + operator: + description: |- + The binary operator to be applied to the content fetched from the selector + Possible values are: "eq" (equal to), "neq" (not equal to) + enum: + - eq + - neq + - startswith + - endswith + - incl + - excl + - matches + type: string + selector: + description: |- + Selector defines one item from the well known selectors + TODO Document properly "Well-known selector" https://github.com/Kuadrant/architecture/blob/main/rfcs/0001-rlp-v2.md#well-known-selectors + maxLength: 253 + minLength: 1 + type: string + value: + description: The value of reference for the comparison. + type: string + required: + - operator + - selector + - value + type: object + type: array + type: object + description: Limits holds the struct of limits indexed by a unique + name + maxProperties: 14 + type: object + type: object targetRef: description: TargetRef identifies an API object to apply policy to. properties: diff --git a/controllers/ratelimitpolicy_controller.go b/controllers/ratelimitpolicy_controller.go index b5e8fe3ff..0a44f25b0 100644 --- a/controllers/ratelimitpolicy_controller.go +++ b/controllers/ratelimitpolicy_controller.go @@ -175,7 +175,7 @@ func (r *RateLimitPolicyReconciler) reconcileResources(ctx context.Context, rlp return err } - if err := r.reconcileLimits(ctx, rlp); err != nil { + if err := r.reconcileLimits(ctx, rlp, targetNetworkObject); err != nil { return err } @@ -195,7 +195,7 @@ func (r *RateLimitPolicyReconciler) deleteResources(ctx context.Context, rlp *ku return err } - if err := r.deleteLimits(ctx, rlp); err != nil && !apierrors.IsNotFound(err) { + if err := r.deleteLimits(ctx, rlp, targetNetworkObject); err != nil && !apierrors.IsNotFound(err) { return err } diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index 5ee202a80..ec13a4aba 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -326,6 +326,238 @@ var _ = Describe("RateLimitPolicy controller", func() { }, SpecTimeout(time.Minute)) }) + Context("RLP Overrides", func() { + It("Gateway atomic override - gateway overrides exist and then route policy created", func(ctx SpecContext) { + // create httproute + httpRoute := testBuildBasicHttpRoute(routeName, gwName, testNamespace, []string{"*.example.com"}) + Expect(k8sClient.Create(ctx, httpRoute)).To(Succeed()) + Eventually(testRouteIsAccepted(client.ObjectKeyFromObject(httpRoute))).WithContext(ctx).Should(BeTrue()) + + // create GW RLP + gwRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Spec.TargetRef.Kind = "Gateway" + policy.Spec.TargetRef.Name = gatewayapiv1.ObjectName(gwName) + policy.Spec.Defaults = nil + policy.Spec.Overrides = &kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, + }, + }, + }, + } + }) + Expect(k8sClient.Create(ctx, gwRLP)).To(Succeed()) + rlpKey := client.ObjectKey{Name: gwRLP.Name, Namespace: testNamespace} + Eventually(testRLPIsAccepted(rlpKey)).WithContext(ctx).Should(BeTrue()) + + // Create HTTPRoute RLP with new default limits + routeRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Name = "httproute-rlp" + policy.Spec.CommonSpec().Limits = map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 10, Duration: 5, Unit: kuadrantv1beta2.TimeUnit("second"), + }, + }, + }, + } + }) + Expect(k8sClient.Create(ctx, routeRLP)).To(Succeed()) + rlpKey = client.ObjectKey{Name: routeRLP.Name, Namespace: testNamespace} + Eventually(testRLPIsAccepted(rlpKey)).WithContext(ctx).Should(BeTrue()) + + // Check Gateway direct back reference + gwKey := client.ObjectKeyFromObject(gateway) + existingGateway := &gatewayapiv1.Gateway{} + Expect(k8sClient.Get(ctx, gwKey, existingGateway)).To(Succeed()) + Expect(existingGateway.GetAnnotations()).To(HaveKeyWithValue( + gwRLP.DirectReferenceAnnotationName(), client.ObjectKeyFromObject(gwRLP).String())) + + // check limits - should contain override values + Eventually(func(g Gomega) { + limitadorKey := client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace} + existingLimitador := &limitadorv1alpha1.Limitador{} + g.Expect(k8sClient.Get(ctx, limitadorKey, existingLimitador)).To(Succeed()) + g.Expect(existingLimitador.Spec.Limits).To(ContainElements(limitadorv1alpha1.RateLimit{ + MaxValue: 1, + Seconds: 180, + Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP), + Conditions: []string{`limit.l1__2804bad6 == "1"`}, + Variables: []string{}, + Name: rlptools.LimitsNameFromRLP(routeRLP), + })) + }).WithContext(ctx).Should(Succeed()) + + // Gateway should contain HTTPRoute RLP in backreference + Expect(k8sClient.Get(ctx, gwKey, existingGateway)).To(Succeed()) + serialized, err := json.Marshal(rlpKey) + Expect(err).ToNot(HaveOccurred()) + Expect(existingGateway.GetAnnotations()).To(HaveKey(routeRLP.BackReferenceAnnotationName())) + Expect(existingGateway.GetAnnotations()[routeRLP.BackReferenceAnnotationName()]).To(ContainSubstring(string(serialized))) + + }, SpecTimeout(time.Minute)) + + It("Gateway atomic override - route policy exits and then gateway policy created", func(ctx SpecContext) { + // create httproute + httpRoute := testBuildBasicHttpRoute(routeName, gwName, testNamespace, []string{"*.example.com"}) + Expect(k8sClient.Create(ctx, httpRoute)).To(Succeed()) + Eventually(testRouteIsAccepted(client.ObjectKeyFromObject(httpRoute))).WithContext(ctx).Should(BeTrue()) + + // Create HTTPRoute RLP + routeRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Name = "httproute-rlp" + policy.Spec.CommonSpec().Limits = map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 10, Duration: 5, Unit: kuadrantv1beta2.TimeUnit("second"), + }, + }, + }, + } + }) + Expect(k8sClient.Create(ctx, routeRLP)).To(Succeed()) + rlpKey := client.ObjectKey{Name: routeRLP.Name, Namespace: testNamespace} + Eventually(testRLPIsAccepted(rlpKey)).WithContext(ctx).Should(BeTrue()) + + // create GW RLP + gwRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Spec.TargetRef.Kind = "Gateway" + policy.Spec.TargetRef.Name = gatewayapiv1.ObjectName(gwName) + policy.Spec.Defaults = nil + policy.Spec.Overrides = &kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, + }, + }, + }, + } + }) + Expect(k8sClient.Create(ctx, gwRLP)).To(Succeed()) + rlpKey = client.ObjectKey{Name: gwRLP.Name, Namespace: testNamespace} + Eventually(testRLPIsAccepted(rlpKey)).WithContext(ctx).Should(BeTrue()) + + // Check Gateway direct back reference + gwKey := client.ObjectKeyFromObject(gateway) + existingGateway := &gatewayapiv1.Gateway{} + Expect(k8sClient.Get(ctx, gwKey, existingGateway)).To(Succeed()) + Expect(existingGateway.GetAnnotations()).To(HaveKeyWithValue( + gwRLP.DirectReferenceAnnotationName(), client.ObjectKeyFromObject(gwRLP).String())) + + Eventually(func(g Gomega) { + // check limits - should contain override values + limitadorKey := client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace} + existingLimitador := &limitadorv1alpha1.Limitador{} + Expect(k8sClient.Get(ctx, limitadorKey, existingLimitador)).To(Succeed()) + Expect(existingLimitador.Spec.Limits).To(ContainElements(limitadorv1alpha1.RateLimit{ + MaxValue: 1, + Seconds: 180, + Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP), + Conditions: []string{`limit.l1__2804bad6 == "1"`}, + Variables: []string{}, + Name: rlptools.LimitsNameFromRLP(routeRLP), + })) + }) + + // Gateway should contain HTTPRoute RLP in backreference + Expect(k8sClient.Get(ctx, gwKey, existingGateway)).To(Succeed()) + serialized, err := json.Marshal(rlpKey) + Expect(err).ToNot(HaveOccurred()) + Expect(existingGateway.GetAnnotations()).To(HaveKey(routeRLP.BackReferenceAnnotationName())) + Expect(existingGateway.GetAnnotations()[routeRLP.BackReferenceAnnotationName()]).To(ContainSubstring(string(serialized))) + + }, SpecTimeout(time.Minute)) + + It("Gateway atomic override - gateway override added later on", func(ctx SpecContext) { + // create httproute + httpRoute := testBuildBasicHttpRoute(routeName, gwName, testNamespace, []string{"*.example.com"}) + Expect(k8sClient.Create(ctx, httpRoute)).To(Succeed()) + Eventually(testRouteIsAccepted(client.ObjectKeyFromObject(httpRoute))).WithContext(ctx).Should(BeTrue()) + + // Create HTTPRoute RLP + routeRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Name = "httproute-rlp" + policy.Spec.CommonSpec().Limits = map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 10, Duration: 5, Unit: kuadrantv1beta2.TimeUnit("second"), + }, + }, + }, + } + }) + Expect(k8sClient.Create(ctx, routeRLP)).To(Succeed()) + rlpKey := client.ObjectKey{Name: routeRLP.Name, Namespace: testNamespace} + Eventually(testRLPIsAccepted(rlpKey)).WithContext(ctx).Should(BeTrue()) + + // create GW RLP + gwRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Spec.TargetRef.Kind = "Gateway" + policy.Spec.TargetRef.Name = gatewayapiv1.ObjectName(gwName) + policy.Spec.Defaults = &kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, + }, + }, + }, + } + }) + Expect(k8sClient.Create(ctx, gwRLP)).To(Succeed()) + rlpKey = client.ObjectKey{Name: gwRLP.Name, Namespace: testNamespace} + Eventually(testRLPIsAccepted(rlpKey)).WithContext(ctx).Should(BeTrue()) + + // check limits - should contain override values + limitadorKey := client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace} + existingLimitador := &limitadorv1alpha1.Limitador{} + Expect(k8sClient.Get(ctx, limitadorKey, existingLimitador)).To(Succeed()) + Expect(existingLimitador.Spec.Limits).To(ContainElements(limitadorv1alpha1.RateLimit{ + MaxValue: 10, + Seconds: 5, + Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP), + Conditions: []string{`limit.l1__2804bad6 == "1"`}, + Variables: []string{}, + Name: rlptools.LimitsNameFromRLP(routeRLP), + })) + + updatedGRLP := &kuadrantv1beta2.RateLimitPolicy{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: gwRLP.Name, Namespace: testNamespace}, updatedGRLP)).To(Succeed()) + Eventually(func(g Gomega) { + updatedGRLP.Spec.Overrides = updatedGRLP.Spec.Defaults.DeepCopy() + updatedGRLP.Spec.Defaults = nil + g.Expect(k8sClient.Update(ctx, updatedGRLP)).To(Succeed()) + }).WithContext(ctx).Should(Succeed()) + + Eventually(func(g Gomega) { + // check limits - should contain override values + limitadorKey = client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace} + existingLimitador = &limitadorv1alpha1.Limitador{} + g.Expect(k8sClient.Get(ctx, limitadorKey, existingLimitador)).To(Succeed()) + g.Expect(existingLimitador.Spec.Limits).To(ContainElements(limitadorv1alpha1.RateLimit{ + MaxValue: 1, + Seconds: 180, + Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP), + Conditions: []string{`limit.l1__2804bad6 == "1"`}, + Variables: []string{}, + Name: rlptools.LimitsNameFromRLP(routeRLP), + })) + }) + }, SpecTimeout(time.Minute)) + }) + Context("RLP accepted condition reasons", func() { assertAcceptedConditionFalse := func(rlp *kuadrantv1beta2.RateLimitPolicy, reason, message string) func() bool { return func() bool { diff --git a/controllers/ratelimitpolicy_limits.go b/controllers/ratelimitpolicy_limits.go index f8b421962..99840673e 100644 --- a/controllers/ratelimitpolicy_limits.go +++ b/controllers/ratelimitpolicy_limits.go @@ -6,23 +6,25 @@ import ( "github.com/go-logr/logr" limitadorv1alpha1 "github.com/kuadrant/limitador-operator/api/v1alpha1" "sigs.k8s.io/controller-runtime/pkg/client" + gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" "github.com/kuadrant/kuadrant-operator/pkg/common" + "github.com/kuadrant/kuadrant-operator/pkg/library/gatewayapi" "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" "github.com/kuadrant/kuadrant-operator/pkg/library/utils" "github.com/kuadrant/kuadrant-operator/pkg/rlptools" ) -func (r *RateLimitPolicyReconciler) reconcileLimits(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy) error { +func (r *RateLimitPolicyReconciler) reconcileLimits(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy, targetNetworkObject client.Object) error { rlpRefs, err := r.TargetRefReconciler.GetAllGatewayPolicyRefs(ctx, rlp) if err != nil { return err } - return r.reconcileLimitador(ctx, rlp, append(rlpRefs, client.ObjectKeyFromObject(rlp))) + return r.reconcileLimitador(ctx, rlp, append(rlpRefs, client.ObjectKeyFromObject(rlp)), targetNetworkObject) } -func (r *RateLimitPolicyReconciler) deleteLimits(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy) error { +func (r *RateLimitPolicyReconciler) deleteLimits(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy, targetNetworkObject client.Object) error { rlpRefs, err := r.TargetRefReconciler.GetAllGatewayPolicyRefs(ctx, rlp) if err != nil { return err @@ -32,14 +34,14 @@ func (r *RateLimitPolicyReconciler) deleteLimits(ctx context.Context, rlp *kuadr return rlpRef.Name != rlp.Name || rlpRef.Namespace != rlp.Namespace }) - return r.reconcileLimitador(ctx, rlp, rlpRefsWithoutRLP) + return r.reconcileLimitador(ctx, rlp, rlpRefsWithoutRLP, targetNetworkObject) } -func (r *RateLimitPolicyReconciler) reconcileLimitador(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy, rlpRefs []client.ObjectKey) error { +func (r *RateLimitPolicyReconciler) reconcileLimitador(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy, rlpRefs []client.ObjectKey, targetNetworkObject client.Object) error { logger, _ := logr.FromContext(ctx) logger = logger.WithName("reconcileLimitador").WithValues("rlp refs", utils.Map(rlpRefs, func(ref client.ObjectKey) string { return ref.String() })) - rateLimitIndex, err := r.buildRateLimitIndex(ctx, rlpRefs) + rateLimitIndex, err := r.buildRateLimitIndex(ctx, rlpRefs, targetNetworkObject) if err != nil { return err } @@ -87,7 +89,7 @@ func (r *RateLimitPolicyReconciler) reconcileLimitador(ctx context.Context, rlp return nil } -func (r *RateLimitPolicyReconciler) buildRateLimitIndex(ctx context.Context, rlpRefs []client.ObjectKey) (*rlptools.RateLimitIndex, error) { +func (r *RateLimitPolicyReconciler) buildRateLimitIndex(ctx context.Context, rlpRefs []client.ObjectKey, targetNetworkObject client.Object) (*rlptools.RateLimitIndex, error) { logger, _ := logr.FromContext(ctx) logger = logger.WithName("buildRateLimitIndex").WithValues("ratelimitpolicies", rlpRefs) @@ -105,8 +107,38 @@ func (r *RateLimitPolicyReconciler) buildRateLimitIndex(ctx context.Context, rlp return nil, err } + if err := r.applyOverrides(ctx, rlp, targetNetworkObject); err != nil { + return nil, err + } + rateLimitIndex.Set(rlpKey, rlptools.LimitadorRateLimitsFromRLP(rlp)) } return rateLimitIndex, nil } + +// applyOverrides checks for any overrides set for the RateLimitPolicy. +// It iterates through the RateLimitPolicyList to find overrides for the provided target HTTPRoute. +// If an override is found, it updates the limits in the RateLimitPolicySpec in accordingly. +func (r *RateLimitPolicyReconciler) applyOverrides(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy, targetNetworkObject client.Object) error { + if route, ok := targetNetworkObject.(*gatewayapiv1.HTTPRoute); ok { + rlpList := &kuadrantv1beta2.RateLimitPolicyList{} + if err := r.Client().List(ctx, rlpList); err != nil { + return err + } + + for _, p := range rlpList.Items { + clientKeys := gatewayapi.GetRouteAcceptedGatewayParentKeys(route) + for _, clientKey := range clientKeys { + if gatewayapi.IsTargetRefGateway(p.GetTargetRef()) && + clientKey.Name == string(p.Spec.TargetRef.Name) && clientKey.Namespace == p.Namespace { + if p.Spec.Overrides != nil { + rlp.Spec.CommonSpec().Limits = p.Spec.Overrides.Limits + } + } + } + } + } + + return nil +} From d6ac7a05e0f4aff2f08e3d2a3ffab0c0121f4b1a Mon Sep 17 00:00:00 2001 From: KevFan Date: Thu, 11 Apr 2024 13:02:44 +0100 Subject: [PATCH 02/14] feat: RLP CEL implicit default and override mutual exclusivity --- api/v1beta2/ratelimitpolicy_types.go | 1 + .../kuadrant.io_ratelimitpolicies.yaml | 3 + .../bases/kuadrant.io_ratelimitpolicies.yaml | 3 + controllers/helper_test.go | 20 +++ .../ratelimitpolicy_controller_test.go | 151 +++++++++++------- go.mod | 2 +- 6 files changed, 119 insertions(+), 61 deletions(-) diff --git a/api/v1beta2/ratelimitpolicy_types.go b/api/v1beta2/ratelimitpolicy_types.go index a13a9e8f4..eea1396ef 100644 --- a/api/v1beta2/ratelimitpolicy_types.go +++ b/api/v1beta2/ratelimitpolicy_types.go @@ -122,6 +122,7 @@ func (l Limit) CountersAsStringList() []string { // RateLimitPolicySpec defines the desired state of RateLimitPolicy // +kubebuilder:validation:XValidation:rule="self.targetRef.kind != 'Gateway' || !has(self.limits) || !self.limits.exists(x, has(self.limits[x].routeSelectors))",message="route selectors not supported when targeting a Gateway" // +kubebuilder:validation:XValidation:rule="!(has(self.defaults) && has(self.limits))",message="Implicit and explicit defaults are mutually exclusive" +// +kubebuilder:validation:XValidation:rule="!(has(self.overrides) && has(self.limits))",message="Overrides and implicit defaults are mutually exclusive. Use defaults block instead to set defaults explicitly" type RateLimitPolicySpec struct { // TargetRef identifies an API object to apply policy to. // +kubebuilder:validation:XValidation:rule="self.group == 'gateway.networking.k8s.io'",message="Invalid targetRef.group. The only supported value is 'gateway.networking.k8s.io'" diff --git a/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml b/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml index f869d3640..d6e75746b 100644 --- a/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml +++ b/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml @@ -1290,6 +1290,9 @@ spec: has(self.limits[x].routeSelectors)) - message: Implicit and explicit defaults are mutually exclusive rule: '!(has(self.defaults) && has(self.limits))' + - message: Overrides and implicit defaults are mutually exclusive. Use + defaults block instead to set defaults explicitly + rule: '!(has(self.overrides) && has(self.limits))' status: description: RateLimitPolicyStatus defines the observed state of RateLimitPolicy properties: diff --git a/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml b/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml index 603f8bb4b..711a93dcd 100644 --- a/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml +++ b/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml @@ -1289,6 +1289,9 @@ spec: has(self.limits[x].routeSelectors)) - message: Implicit and explicit defaults are mutually exclusive rule: '!(has(self.defaults) && has(self.limits))' + - message: Overrides and implicit defaults are mutually exclusive. Use + defaults block instead to set defaults explicitly + rule: '!(has(self.overrides) && has(self.limits))' status: description: RateLimitPolicyStatus defines the observed state of RateLimitPolicy properties: diff --git a/controllers/helper_test.go b/controllers/helper_test.go index 54f6d3db1..5e1ccc089 100644 --- a/controllers/helper_test.go +++ b/controllers/helper_test.go @@ -88,6 +88,16 @@ func ApplyKuadrantCRWithName(namespace, name string) { }, time.Minute, 5*time.Second).Should(BeTrue()) } +func CreateNamespaceWithContext(ctx context.Context, namespace *string) { + nsObject := &v1.Namespace{ + TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "Namespace"}, + ObjectMeta: metav1.ObjectMeta{GenerateName: "test-namespace-"}, + } + Expect(testClient().Create(context.Background(), nsObject)).To(Succeed()) + + *namespace = nsObject.Name +} + func CreateNamespace(namespace *string) { var generatedTestNamespace = "test-namespace-" + uuid.New().String() @@ -108,6 +118,16 @@ func CreateNamespace(namespace *string) { *namespace = existingNamespace.Name } +func DeleteNamespaceCallbackWithContext(ctx context.Context, namespace *string) { + desiredTestNamespace := &v1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: *namespace}} + Eventually(func(g Gomega) { + err := k8sClient.Delete(ctx, desiredTestNamespace, client.PropagationPolicy(metav1.DeletePropagationForeground)) + g.Expect(err).ToNot(BeNil()) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) + }).WithContext(ctx).Should(Succeed()) + +} + func DeleteNamespaceCallback(namespace *string) func() { return func() { desiredTestNamespace := &v1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: *namespace}} diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index ec13a4aba..fd56f2fbe 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -54,7 +54,7 @@ var _ = Describe("RateLimitPolicy controller", func() { "l1": { Rates: []kuadrantv1beta2.Rate{ { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + Limit: 1, Duration: 3, Unit: "minute", }, }, }, @@ -69,17 +69,19 @@ var _ = Describe("RateLimitPolicy controller", func() { return policy } - beforeEachCallback := func() { - CreateNamespace(&testNamespace) + beforeEachCallback := func(ctx SpecContext) { + CreateNamespaceWithContext(ctx, &testNamespace) gateway = testBuildBasicGateway(gwName, testNamespace) - err := k8sClient.Create(context.Background(), gateway) - Expect(err).ToNot(HaveOccurred()) - Eventually(testGatewayIsReady(gateway), 30*time.Second, 5*time.Second).Should(BeTrue()) + + Expect(k8sClient.Create(ctx, gateway)).To(Succeed()) + Eventually(testGatewayIsReady(gateway)).WithContext(ctx).Should(BeTrue()) ApplyKuadrantCR(testNamespace) } - BeforeEach(beforeEachCallback) - AfterEach(DeleteNamespaceCallback(&testNamespace)) + BeforeEach(beforeEachCallback, NodeTimeout(time.Minute)) + AfterEach(func(ctx SpecContext) { + DeleteNamespaceCallbackWithContext(ctx, &testNamespace) + }, NodeTimeout(time.Minute)) Context("RLP targeting HTTPRoute", func() { It("Creates all the resources for a basic HTTPRoute and RateLimitPolicy", func() { @@ -165,7 +167,7 @@ var _ = Describe("RateLimitPolicy controller", func() { "l1": { Rates: []kuadrantv1beta2.Rate{ { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + Limit: 1, Duration: 3, Unit: "minute", }, }, }, @@ -286,7 +288,7 @@ var _ = Describe("RateLimitPolicy controller", func() { "l1": { Rates: []kuadrantv1beta2.Rate{ { - Limit: 10, Duration: 5, Unit: kuadrantv1beta2.TimeUnit("second"), + Limit: 10, Duration: 5, Unit: "second", }, }, }, @@ -343,7 +345,7 @@ var _ = Describe("RateLimitPolicy controller", func() { "l1": { Rates: []kuadrantv1beta2.Rate{ { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + Limit: 1, Duration: 3, Unit: "minute", }, }, }, @@ -361,7 +363,7 @@ var _ = Describe("RateLimitPolicy controller", func() { "l1": { Rates: []kuadrantv1beta2.Rate{ { - Limit: 10, Duration: 5, Unit: kuadrantv1beta2.TimeUnit("second"), + Limit: 10, Duration: 5, Unit: "second", }, }, }, @@ -415,7 +417,7 @@ var _ = Describe("RateLimitPolicy controller", func() { "l1": { Rates: []kuadrantv1beta2.Rate{ { - Limit: 10, Duration: 5, Unit: kuadrantv1beta2.TimeUnit("second"), + Limit: 10, Duration: 5, Unit: "second", }, }, }, @@ -435,7 +437,7 @@ var _ = Describe("RateLimitPolicy controller", func() { "l1": { Rates: []kuadrantv1beta2.Rate{ { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + Limit: 1, Duration: 3, Unit: "minute", }, }, }, @@ -477,7 +479,7 @@ var _ = Describe("RateLimitPolicy controller", func() { }, SpecTimeout(time.Minute)) - It("Gateway atomic override - gateway override added later on", func(ctx SpecContext) { + It("Gateway atomic override - gateway defaults turned into overrides later on", func(ctx SpecContext) { // create httproute httpRoute := testBuildBasicHttpRoute(routeName, gwName, testNamespace, []string{"*.example.com"}) Expect(k8sClient.Create(ctx, httpRoute)).To(Succeed()) @@ -490,7 +492,7 @@ var _ = Describe("RateLimitPolicy controller", func() { "l1": { Rates: []kuadrantv1beta2.Rate{ { - Limit: 10, Duration: 5, Unit: kuadrantv1beta2.TimeUnit("second"), + Limit: 10, Duration: 5, Unit: "second", }, }, }, @@ -509,7 +511,7 @@ var _ = Describe("RateLimitPolicy controller", func() { "l1": { Rates: []kuadrantv1beta2.Rate{ { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + Limit: 1, Duration: 3, Unit: "minute", }, }, }, @@ -520,7 +522,7 @@ var _ = Describe("RateLimitPolicy controller", func() { rlpKey = client.ObjectKey{Name: gwRLP.Name, Namespace: testNamespace} Eventually(testRLPIsAccepted(rlpKey)).WithContext(ctx).Should(BeTrue()) - // check limits - should contain override values + // check limits - should contain HTTPRoute values limitadorKey := client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace} existingLimitador := &limitadorv1alpha1.Limitador{} Expect(k8sClient.Get(ctx, limitadorKey, existingLimitador)).To(Succeed()) @@ -559,58 +561,49 @@ var _ = Describe("RateLimitPolicy controller", func() { }) Context("RLP accepted condition reasons", func() { - assertAcceptedConditionFalse := func(rlp *kuadrantv1beta2.RateLimitPolicy, reason, message string) func() bool { - return func() bool { + assertAcceptedConditionFalse := func(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy, reason, message string) func(g Gomega) { + return func(g Gomega) { rlpKey := client.ObjectKeyFromObject(rlp) existingRLP := &kuadrantv1beta2.RateLimitPolicy{} - err := k8sClient.Get(context.Background(), rlpKey, existingRLP) - if err != nil { - return false - } + g.Expect(k8sClient.Get(ctx, rlpKey, existingRLP)).To(Succeed()) cond := meta.FindStatusCondition(existingRLP.Status.Conditions, string(gatewayapiv1alpha2.PolicyConditionAccepted)) - if cond == nil { - return false - } - - return cond.Status == metav1.ConditionFalse && cond.Reason == reason && cond.Message == message + g.Expect(cond).ToNot(BeNil()) + g.Expect(cond.Status == metav1.ConditionFalse && cond.Reason == reason && cond.Message == message).To(BeTrue()) } } // Accepted reason is already tested generally by the existing tests - It("Target not found reason", func() { + It("Target not found reason", func(ctx SpecContext) { rlp := policyFactory() - err := k8sClient.Create(context.Background(), rlp) - Expect(err).ToNot(HaveOccurred()) + Expect(k8sClient.Create(ctx, rlp)).To(Succeed()) - Eventually(assertAcceptedConditionFalse(rlp, string(gatewayapiv1alpha2.PolicyReasonTargetNotFound), + Eventually(assertAcceptedConditionFalse(ctx, rlp, string(gatewayapiv1alpha2.PolicyReasonTargetNotFound), fmt.Sprintf("RateLimitPolicy target %s was not found", routeName)), - time.Minute, 5*time.Second).Should(BeTrue()) - }) + ).WithContext(ctx).Should(Succeed()) + }, SpecTimeout(time.Minute)) - It("Conflict reason", func() { + It("Conflict reason", func(ctx SpecContext) { httpRoute := testBuildBasicHttpRoute(routeName, gwName, testNamespace, []string{"*.example.com"}) - err := k8sClient.Create(context.Background(), httpRoute) - Expect(err).ToNot(HaveOccurred()) - Eventually(testRouteIsAccepted(client.ObjectKeyFromObject(httpRoute)), time.Minute, 5*time.Second).Should(BeTrue()) + Expect(k8sClient.Create(ctx, httpRoute)).To(Succeed()) + Eventually(testRouteIsAccepted(client.ObjectKeyFromObject(httpRoute))).WithContext(ctx).Should(BeTrue()) rlp := policyFactory() - err = k8sClient.Create(context.Background(), rlp) - Expect(err).ToNot(HaveOccurred()) + Expect(k8sClient.Create(ctx, rlp)).To(Succeed()) + Eventually(testRLPIsAccepted(client.ObjectKeyFromObject(rlp))).WithContext(ctx).Should(BeTrue()) rlp2 := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { policy.Name = "conflicting-rlp" }) - err = k8sClient.Create(context.Background(), rlp2) - Expect(err).ToNot(HaveOccurred()) + Expect(k8sClient.Create(ctx, rlp2)).To(Succeed()) - Eventually(assertAcceptedConditionFalse(rlp2, string(gatewayapiv1alpha2.PolicyReasonConflicted), + Eventually(assertAcceptedConditionFalse(ctx, rlp2, string(gatewayapiv1alpha2.PolicyReasonConflicted), fmt.Sprintf("RateLimitPolicy is conflicted by %[1]v/toystore-rlp: the gateway.networking.k8s.io/v1, Kind=HTTPRoute target %[1]v/toystore-route is already referenced by policy %[1]v/toystore-rlp", testNamespace)), - time.Minute, 5*time.Second).Should(BeTrue()) - }) + ).WithContext(ctx).Should(Succeed()) + }, SpecTimeout(2*time.Minute)) - It("Invalid reason", func() { + It("Invalid reason", func(ctx SpecContext) { var otherNamespace string CreateNamespace(&otherNamespace) defer DeleteNamespaceCallback(&otherNamespace) @@ -621,10 +614,10 @@ var _ = Describe("RateLimitPolicy controller", func() { policy.Spec.TargetRef.Name = gatewayapiv1.ObjectName(gateway.Name) policy.Spec.TargetRef.Namespace = ptr.To(gatewayapiv1.Namespace(testNamespace)) }) - Expect(k8sClient.Create(context.Background(), policy)).To(Succeed()) + Expect(k8sClient.Create(ctx, policy)).To(Succeed()) - Eventually(assertAcceptedConditionFalse(policy, string(gatewayapiv1alpha2.PolicyReasonInvalid), fmt.Sprintf("RateLimitPolicy target is invalid: invalid targetRef.Namespace %s. Currently only supporting references to the same namespace", testNamespace)), 30*time.Second, 5*time.Second).Should(BeTrue()) - }) + Eventually(assertAcceptedConditionFalse(ctx, policy, string(gatewayapiv1alpha2.PolicyReasonInvalid), fmt.Sprintf("RateLimitPolicy target is invalid: invalid targetRef.Namespace %s. Currently only supporting references to the same namespace", testNamespace)), 30*time.Second, 5*time.Second).Should(BeTrue()) + }, SpecTimeout(time.Minute)) }) Context("When RLP switches target from one HTTPRoute to another HTTPRoute", func() { @@ -1111,8 +1104,8 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() { }) }) - Context("Defaults validation", func() { - It("Valid only implicit defaults", func(ctx SpecContext) { + Context("Defaults / Override validation", func() { + It("Valid - only implicit defaults defined", func(ctx SpecContext) { policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { policy.Spec.Limits = map[string]kuadrantv1beta2.Limit{ "implicit": { @@ -1120,11 +1113,10 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() { }, } }) - err := k8sClient.Create(ctx, policy) - Expect(err).To(BeNil()) + Expect(k8sClient.Create(ctx, policy)).To(Succeed()) }) - It("Valid only explicit defaults", func(ctx SpecContext) { + It("Valid - only explicit defaults defined", func(ctx SpecContext) { policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { policy.Spec.Defaults = &kuadrantv1beta2.RateLimitPolicyCommonSpec{ Limits: map[string]kuadrantv1beta2.Limit{ @@ -1134,11 +1126,10 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() { }, } }) - err := k8sClient.Create(ctx, policy) - Expect(err).To(BeNil()) + Expect(k8sClient.Create(ctx, policy)).To(Succeed()) }) - It("Invalid implicit and explicit defaults are mutually exclusive", func(ctx SpecContext) { + It("Invalid - implicit and explicit defaults are mutually exclusive", func(ctx SpecContext) { policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { policy.Spec.Defaults = &kuadrantv1beta2.RateLimitPolicyCommonSpec{ Limits: map[string]kuadrantv1beta2.Limit{ @@ -1154,9 +1145,49 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() { } }) err := k8sClient.Create(ctx, policy) - Expect(err).To(Not(BeNil())) + Expect(err).ToNot(BeNil()) Expect(strings.Contains(err.Error(), "Implicit and explicit defaults are mutually exclusive")).To(BeTrue()) }) + + It("Valid - explicit default and override defined", func(ctx SpecContext) { + policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Spec.Defaults = &kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "implicit": { + Rates: []kuadrantv1beta2.Rate{{Limit: 2, Duration: 20, Unit: "second"}}, + }, + }, + } + policy.Spec.Overrides = &kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "explicit": { + Rates: []kuadrantv1beta2.Rate{{Limit: 1, Duration: 10, Unit: "second"}}, + }, + }, + } + }) + Expect(k8sClient.Create(ctx, policy)).To(Succeed()) + }) + + It("Invalid - implicit default and explicit override are mutually exclusive", func(ctx SpecContext) { + policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Spec.Limits = map[string]kuadrantv1beta2.Limit{ + "implicit": { + Rates: []kuadrantv1beta2.Rate{{Limit: 2, Duration: 20, Unit: "second"}}, + }, + } + policy.Spec.Overrides = &kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "explicit": { + Rates: []kuadrantv1beta2.Rate{{Limit: 1, Duration: 10, Unit: "second"}}, + }, + }, + } + }) + err := k8sClient.Create(ctx, policy) + Expect(err).ToNot(BeNil()) + Expect(strings.Contains(err.Error(), "Overrides and implicit defaults are mutually exclusive. Use defaults block instead to set defaults explicitly")).To(BeTrue()) + }) }) Context("Route Selector Validation", func() { @@ -1173,7 +1204,7 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() { "l1": { Rates: []kuadrantv1beta2.Rate{ { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + Limit: 1, Duration: 3, Unit: "minute", }, }, RouteSelectors: []kuadrantv1beta2.RouteSelector{ diff --git a/go.mod b/go.mod index 3ec8bfd4f..243696b47 100644 --- a/go.mod +++ b/go.mod @@ -17,7 +17,6 @@ require ( github.com/onsi/gomega v1.30.0 go.uber.org/zap v1.26.0 golang.org/x/net v0.19.0 - golang.org/x/sync v0.5.0 google.golang.org/protobuf v1.33.0 gotest.tools v2.2.0+incompatible istio.io/api v1.20.0 @@ -148,6 +147,7 @@ require ( golang.org/x/crypto v0.17.0 // indirect golang.org/x/exp v0.0.0-20231127185646-65229373498e // indirect golang.org/x/oauth2 v0.15.0 // indirect + golang.org/x/sync v0.5.0 // indirect golang.org/x/sys v0.15.0 // indirect golang.org/x/term v0.15.0 // indirect golang.org/x/text v0.14.0 // indirect From d5f37c7befdaee59f0fa0f35d9d400bfb98e7edd Mon Sep 17 00:00:00 2001 From: KevFan Date: Fri, 12 Apr 2024 13:37:01 +0100 Subject: [PATCH 03/14] refactor: use DAG for applying RLP overrides --- controllers/ratelimitpolicy_controller.go | 4 +- controllers/ratelimitpolicy_limits.go | 119 ++++++++++++++++------ pkg/library/gatewayapi/types.go | 26 ++++- pkg/library/gatewayapi/types_test.go | 72 ++++++++++++- 4 files changed, 186 insertions(+), 35 deletions(-) diff --git a/controllers/ratelimitpolicy_controller.go b/controllers/ratelimitpolicy_controller.go index 0a44f25b0..b5e8fe3ff 100644 --- a/controllers/ratelimitpolicy_controller.go +++ b/controllers/ratelimitpolicy_controller.go @@ -175,7 +175,7 @@ func (r *RateLimitPolicyReconciler) reconcileResources(ctx context.Context, rlp return err } - if err := r.reconcileLimits(ctx, rlp, targetNetworkObject); err != nil { + if err := r.reconcileLimits(ctx, rlp); err != nil { return err } @@ -195,7 +195,7 @@ func (r *RateLimitPolicyReconciler) deleteResources(ctx context.Context, rlp *ku return err } - if err := r.deleteLimits(ctx, rlp, targetNetworkObject); err != nil && !apierrors.IsNotFound(err) { + if err := r.deleteLimits(ctx, rlp); err != nil && !apierrors.IsNotFound(err) { return err } diff --git a/controllers/ratelimitpolicy_limits.go b/controllers/ratelimitpolicy_limits.go index 99840673e..d63540b05 100644 --- a/controllers/ratelimitpolicy_limits.go +++ b/controllers/ratelimitpolicy_limits.go @@ -2,29 +2,32 @@ package controllers import ( "context" + "slices" + "sort" "github.com/go-logr/logr" limitadorv1alpha1 "github.com/kuadrant/limitador-operator/api/v1alpha1" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" "github.com/kuadrant/kuadrant-operator/pkg/common" - "github.com/kuadrant/kuadrant-operator/pkg/library/gatewayapi" + kuadrantgatewayapi "github.com/kuadrant/kuadrant-operator/pkg/library/gatewayapi" "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" "github.com/kuadrant/kuadrant-operator/pkg/library/utils" "github.com/kuadrant/kuadrant-operator/pkg/rlptools" ) -func (r *RateLimitPolicyReconciler) reconcileLimits(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy, targetNetworkObject client.Object) error { +func (r *RateLimitPolicyReconciler) reconcileLimits(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy) error { rlpRefs, err := r.TargetRefReconciler.GetAllGatewayPolicyRefs(ctx, rlp) if err != nil { return err } - return r.reconcileLimitador(ctx, rlp, append(rlpRefs, client.ObjectKeyFromObject(rlp)), targetNetworkObject) + return r.reconcileLimitador(ctx, rlp, append(rlpRefs, client.ObjectKeyFromObject(rlp))) } -func (r *RateLimitPolicyReconciler) deleteLimits(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy, targetNetworkObject client.Object) error { +func (r *RateLimitPolicyReconciler) deleteLimits(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy) error { rlpRefs, err := r.TargetRefReconciler.GetAllGatewayPolicyRefs(ctx, rlp) if err != nil { return err @@ -34,14 +37,14 @@ func (r *RateLimitPolicyReconciler) deleteLimits(ctx context.Context, rlp *kuadr return rlpRef.Name != rlp.Name || rlpRef.Namespace != rlp.Namespace }) - return r.reconcileLimitador(ctx, rlp, rlpRefsWithoutRLP, targetNetworkObject) + return r.reconcileLimitador(ctx, rlp, rlpRefsWithoutRLP) } -func (r *RateLimitPolicyReconciler) reconcileLimitador(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy, rlpRefs []client.ObjectKey, targetNetworkObject client.Object) error { +func (r *RateLimitPolicyReconciler) reconcileLimitador(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy, rlpRefs []client.ObjectKey) error { logger, _ := logr.FromContext(ctx) logger = logger.WithName("reconcileLimitador").WithValues("rlp refs", utils.Map(rlpRefs, func(ref client.ObjectKey) string { return ref.String() })) - rateLimitIndex, err := r.buildRateLimitIndex(ctx, rlpRefs, targetNetworkObject) + rateLimitIndex, err := r.buildRateLimitIndex(ctx, rlpRefs) if err != nil { return err } @@ -89,10 +92,15 @@ func (r *RateLimitPolicyReconciler) reconcileLimitador(ctx context.Context, rlp return nil } -func (r *RateLimitPolicyReconciler) buildRateLimitIndex(ctx context.Context, rlpRefs []client.ObjectKey, targetNetworkObject client.Object) (*rlptools.RateLimitIndex, error) { +func (r *RateLimitPolicyReconciler) buildRateLimitIndex(ctx context.Context, rlpRefs []client.ObjectKey) (*rlptools.RateLimitIndex, error) { logger, _ := logr.FromContext(ctx) logger = logger.WithName("buildRateLimitIndex").WithValues("ratelimitpolicies", rlpRefs) + t, err := r.generateTopology(ctx) + if err != nil { + return nil, err + } + rateLimitIndex := rlptools.NewRateLimitIndex() for _, rlpKey := range rlpRefs { @@ -107,9 +115,7 @@ func (r *RateLimitPolicyReconciler) buildRateLimitIndex(ctx context.Context, rlp return nil, err } - if err := r.applyOverrides(ctx, rlp, targetNetworkObject); err != nil { - return nil, err - } + r.applyOverrides(ctx, rlp, t) rateLimitIndex.Set(rlpKey, rlptools.LimitadorRateLimitsFromRLP(rlp)) } @@ -118,27 +124,82 @@ func (r *RateLimitPolicyReconciler) buildRateLimitIndex(ctx context.Context, rlp } // applyOverrides checks for any overrides set for the RateLimitPolicy. -// It iterates through the RateLimitPolicyList to find overrides for the provided target HTTPRoute. -// If an override is found, it updates the limits in the RateLimitPolicySpec in accordingly. -func (r *RateLimitPolicyReconciler) applyOverrides(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy, targetNetworkObject client.Object) error { - if route, ok := targetNetworkObject.(*gatewayapiv1.HTTPRoute); ok { - rlpList := &kuadrantv1beta2.RateLimitPolicyList{} - if err := r.Client().List(ctx, rlpList); err != nil { - return err +// It iterates through the slice of policies to find overrides for the provided target HTTPRoute. +// If an override is found, it updates the limits in the RateLimitPolicySpec accordingly. +func (r *RateLimitPolicyReconciler) applyOverrides(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy, t *kuadrantgatewayapi.Topology) { + logger, _ := logr.FromContext(ctx) + logger = logger.WithName("applyOverrides") + + topologyIndexes := kuadrantgatewayapi.NewTopologyIndexes(t) + + var policies []kuadrantgatewayapi.Policy + // For each gw, get all the policies if the current rlp is within the topology for this gateway + for _, gw := range t.Gateways() { + policyList := topologyIndexes.PoliciesFromGateway(gw.Gateway) + policyKeys := utils.Map(policyList, func(p kuadrantgatewayapi.Policy) client.ObjectKey { + return client.ObjectKeyFromObject(p) + }) + + // this policy is potentially affected by other policies from this gateway + if slices.Contains(policyKeys, client.ObjectKeyFromObject(rlp)) { + policies = append(policies, policyList...) } + } + + filteredPolicies := utils.Filter(policies, func(policy kuadrantgatewayapi.Policy) bool { + // HTTPRoute RLPs should only care about overrides from gateways + if kuadrantgatewayapi.IsTargetRefHTTPRoute(rlp.GetTargetRef()) { + return kuadrantgatewayapi.IsTargetRefGateway(policy.GetTargetRef()) + } + // Gateway RLPs are not affected by other Gateway RLPs + return false + }) - for _, p := range rlpList.Items { - clientKeys := gatewayapi.GetRouteAcceptedGatewayParentKeys(route) - for _, clientKey := range clientKeys { - if gatewayapi.IsTargetRefGateway(p.GetTargetRef()) && - clientKey.Name == string(p.Spec.TargetRef.Name) && clientKey.Namespace == p.Namespace { - if p.Spec.Overrides != nil { - rlp.Spec.CommonSpec().Limits = p.Spec.Overrides.Limits - } - } - } + // Sort by TargetRefKind and creation timestamp + // Gateways RLPs are listed first in the order of oldest policy + sort.Sort(kuadrantgatewayapi.PolicyByTargetRefKindAndCreationTimeStamp(filteredPolicies)) + + // Iterate in order of precedence until finding a block of overrides + for _, policy := range filteredPolicies { + p := policy.(*kuadrantv1beta2.RateLimitPolicy) + if p.Spec.Overrides != nil { + rlp.Spec.CommonSpec().Limits = p.Spec.Overrides.Limits + logger.V(1).Info("applying overrides from parent policy", "parentPolicy", client.ObjectKeyFromObject(p)) + break } } +} - return nil +func (r *RateLimitPolicyReconciler) generateTopology(ctx context.Context) (*kuadrantgatewayapi.Topology, error) { + logger, _ := logr.FromContext(ctx) + + gwList := &gatewayapiv1.GatewayList{} + err := r.Client().List(ctx, gwList) + logger.V(1).Info("topology: list gateways", "#Gateways", len(gwList.Items), "err", err) + if err != nil { + return nil, err + } + + routeList := &gatewayapiv1.HTTPRouteList{} + err = r.Client().List(ctx, routeList) + logger.V(1).Info("topology: list httproutes", "#HTTPRoutes", len(routeList.Items), "err", err) + if err != nil { + return nil, err + } + + rlpList := &kuadrantv1beta2.RateLimitPolicyList{} + err = r.Client().List(ctx, rlpList) + logger.V(1).Info("topology: list rate limit policies", "#RLPS", len(rlpList.Items), "err", err) + if err != nil { + return nil, err + } + + policies := utils.Map(rlpList.Items, func(p kuadrantv1beta2.RateLimitPolicy) kuadrantgatewayapi.Policy { return &p }) + + return kuadrantgatewayapi.NewTopology( + kuadrantgatewayapi.WithGateways(utils.Map(gwList.Items, ptr.To[gatewayapiv1.Gateway])), + kuadrantgatewayapi.WithRoutes(utils.Map(routeList.Items, ptr.To[gatewayapiv1.HTTPRoute])), + kuadrantgatewayapi.WithPolicies(policies), + kuadrantgatewayapi.WithLogger(logger), + ) } diff --git a/pkg/library/gatewayapi/types.go b/pkg/library/gatewayapi/types.go index b2a223baa..cb222aa6e 100644 --- a/pkg/library/gatewayapi/types.go +++ b/pkg/library/gatewayapi/types.go @@ -19,7 +19,31 @@ func (a PolicyByCreationTimestamp) Less(i, j int) bool { p1Time := ptr.To(a[i].GetCreationTimestamp()) p2Time := ptr.To(a[j].GetCreationTimestamp()) if !p1Time.Equal(p2Time) { - return ptr.To(a[i].GetCreationTimestamp()).Before(ptr.To(a[j].GetCreationTimestamp())) + return p1Time.Before(p2Time) + } + + // The policy appearing first in alphabetical order by "{namespace}/{name}". + return client.ObjectKeyFromObject(a[i]).String() < client.ObjectKeyFromObject(a[j]).String() +} + +type PolicyByTargetRefKindAndCreationTimeStamp []Policy + +func (a PolicyByTargetRefKindAndCreationTimeStamp) Len() int { return len(a) } +func (a PolicyByTargetRefKindAndCreationTimeStamp) Swap(i, j int) { a[i], a[j] = a[j], a[i] } +func (a PolicyByTargetRefKindAndCreationTimeStamp) Less(i, j int) bool { + targetRef1 := a[i].GetTargetRef() + targetRef2 := a[j].GetTargetRef() + + // Compare kind first + if targetRef1.Kind != targetRef2.Kind { + return targetRef1.Kind < targetRef2.Kind + } + + // Then compare timestamp + p1Time := ptr.To(a[i].GetCreationTimestamp()) + p2Time := ptr.To(a[j].GetCreationTimestamp()) + if !p1Time.Equal(p2Time) { + return p1Time.Before(p2Time) } // The policy appearing first in alphabetical order by "{namespace}/{name}". diff --git a/pkg/library/gatewayapi/types_test.go b/pkg/library/gatewayapi/types_test.go index 00f02d17e..1233345db 100644 --- a/pkg/library/gatewayapi/types_test.go +++ b/pkg/library/gatewayapi/types_test.go @@ -10,6 +10,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" ) @@ -105,12 +106,77 @@ func TestPolicyByCreationTimestamp(t *testing.T) { } } -func createTestPolicy(name string, creationTime time.Time) *TestPolicy { - return &TestPolicy{ +func TestPolicyByTargetRef(t *testing.T) { + testCases := []struct { + name string + policies []Policy + sortedPolicies []Policy + }{ + { + name: "nil input", + policies: nil, + sortedPolicies: nil, + }, + { + name: "empty slices", + policies: make([]Policy, 0), + sortedPolicies: make([]Policy, 0), + }, + { + name: "by kind, and creation date", + policies: []Policy{ + createTestPolicy("ccc", time.Date(2020, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("Gateway")), + createTestPolicy("bbb", time.Date(2000, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("HTTPRoute")), + createTestPolicy("aaa", time.Date(2010, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("Gateway")), + }, + sortedPolicies: []Policy{ + createTestPolicy("aaa", time.Date(2010, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("Gateway")), + createTestPolicy("ccc", time.Date(2020, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("Gateway")), + createTestPolicy("bbb", time.Date(2000, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("HTTPRoute")), + }, + }, + { + name: "by kind, and then name when creation date equal", + policies: []Policy{ + createTestPolicy("ccc", time.Date(2020, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("Gateway")), + createTestPolicy("bbb", time.Date(2020, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("HTTPRoute")), + createTestPolicy("aaa", time.Date(2020, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("Gateway")), + }, + sortedPolicies: []Policy{ + createTestPolicy("aaa", time.Date(2020, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("Gateway")), + createTestPolicy("ccc", time.Date(2020, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("Gateway")), + createTestPolicy("bbb", time.Date(2020, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("HTTPRoute")), + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(subT *testing.T) { + sort.Sort(PolicyByTargetRefKindAndCreationTimeStamp(tc.policies)) + if !reflect.DeepEqual(tc.policies, tc.sortedPolicies) { + subT.Errorf("expected=%v; got=%v", tc.sortedPolicies, tc.policies) + } + }) + } +} + +func createTestPolicy(name string, creationTime time.Time, mutateFn ...func(p *TestPolicy)) *TestPolicy { + p := &TestPolicy{ ObjectMeta: metav1.ObjectMeta{ Namespace: "testnamespace", Name: name, - CreationTimestamp: metav1.Time{creationTime}, + CreationTimestamp: metav1.Time{Time: creationTime}, }, } + for _, fn := range mutateFn { + fn(p) + } + + return p +} + +func withTargetRefKind(targetRefKind string) func(p *TestPolicy) { + return func(p *TestPolicy) { + p.TargetRef = gatewayapiv1alpha2.PolicyTargetReference{Kind: gatewayapiv1.Kind(targetRefKind)} + } } From 3fc0f8e5b9ac7d3a3dacf1fe7f7c6ef03d8674d5 Mon Sep 17 00:00:00 2001 From: KevFan Date: Fri, 12 Apr 2024 14:07:30 +0100 Subject: [PATCH 04/14] refactor: RLP CEL for override - only allowed for gateways and mutually exclusive with defaults --- api/v1beta2/ratelimitpolicy_types.go | 4 +- .../kuadrant.io_ratelimitpolicies.yaml | 8 +- .../bases/kuadrant.io_ratelimitpolicies.yaml | 8 +- controllers/helper_test.go | 2 +- .../ratelimitpolicy_controller_test.go | 135 +++++++++++++++--- 5 files changed, 133 insertions(+), 24 deletions(-) diff --git a/api/v1beta2/ratelimitpolicy_types.go b/api/v1beta2/ratelimitpolicy_types.go index eea1396ef..b74d5b5a2 100644 --- a/api/v1beta2/ratelimitpolicy_types.go +++ b/api/v1beta2/ratelimitpolicy_types.go @@ -122,7 +122,9 @@ func (l Limit) CountersAsStringList() []string { // RateLimitPolicySpec defines the desired state of RateLimitPolicy // +kubebuilder:validation:XValidation:rule="self.targetRef.kind != 'Gateway' || !has(self.limits) || !self.limits.exists(x, has(self.limits[x].routeSelectors))",message="route selectors not supported when targeting a Gateway" // +kubebuilder:validation:XValidation:rule="!(has(self.defaults) && has(self.limits))",message="Implicit and explicit defaults are mutually exclusive" -// +kubebuilder:validation:XValidation:rule="!(has(self.overrides) && has(self.limits))",message="Overrides and implicit defaults are mutually exclusive. Use defaults block instead to set defaults explicitly" +// +kubebuilder:validation:XValidation:rule="!(has(self.defaults) && has(self.overrides))",message="Overrides and explicit defaults are mutually exclusive" +// +kubebuilder:validation:XValidation:rule="!(has(self.overrides) && has(self.limits))",message="Overrides and implicit defaults are mutually exclusive" +// +kubebuilder:validation:XValidation:rule="!(has(self.overrides) && self.targetRef.kind != 'Gateway')",message="Overrides are only allowed for policies targeting a Gateway resource" type RateLimitPolicySpec struct { // TargetRef identifies an API object to apply policy to. // +kubebuilder:validation:XValidation:rule="self.group == 'gateway.networking.k8s.io'",message="Invalid targetRef.group. The only supported value is 'gateway.networking.k8s.io'" diff --git a/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml b/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml index d6e75746b..e06ca01b7 100644 --- a/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml +++ b/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml @@ -1290,9 +1290,13 @@ spec: has(self.limits[x].routeSelectors)) - message: Implicit and explicit defaults are mutually exclusive rule: '!(has(self.defaults) && has(self.limits))' - - message: Overrides and implicit defaults are mutually exclusive. Use - defaults block instead to set defaults explicitly + - message: Overrides and explicit defaults are mutually exclusive + rule: '!(has(self.defaults) && has(self.overrides))' + - message: Overrides and implicit defaults are mutually exclusive rule: '!(has(self.overrides) && has(self.limits))' + - message: Overrides are only allowed for policies targeting a Gateway + resource + rule: '!(has(self.overrides) && self.targetRef.kind != ''Gateway'')' status: description: RateLimitPolicyStatus defines the observed state of RateLimitPolicy properties: diff --git a/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml b/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml index 711a93dcd..96899903b 100644 --- a/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml +++ b/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml @@ -1289,9 +1289,13 @@ spec: has(self.limits[x].routeSelectors)) - message: Implicit and explicit defaults are mutually exclusive rule: '!(has(self.defaults) && has(self.limits))' - - message: Overrides and implicit defaults are mutually exclusive. Use - defaults block instead to set defaults explicitly + - message: Overrides and explicit defaults are mutually exclusive + rule: '!(has(self.defaults) && has(self.overrides))' + - message: Overrides and implicit defaults are mutually exclusive rule: '!(has(self.overrides) && has(self.limits))' + - message: Overrides are only allowed for policies targeting a Gateway + resource + rule: '!(has(self.overrides) && self.targetRef.kind != ''Gateway'')' status: description: RateLimitPolicyStatus defines the observed state of RateLimitPolicy properties: diff --git a/controllers/helper_test.go b/controllers/helper_test.go index 5e1ccc089..a2b78ee5f 100644 --- a/controllers/helper_test.go +++ b/controllers/helper_test.go @@ -93,7 +93,7 @@ func CreateNamespaceWithContext(ctx context.Context, namespace *string) { TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "Namespace"}, ObjectMeta: metav1.ObjectMeta{GenerateName: "test-namespace-"}, } - Expect(testClient().Create(context.Background(), nsObject)).To(Succeed()) + Expect(testClient().Create(ctx, nsObject)).To(Succeed()) *namespace = nsObject.Name } diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index fd56f2fbe..a912e9fef 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -81,7 +81,7 @@ var _ = Describe("RateLimitPolicy controller", func() { BeforeEach(beforeEachCallback, NodeTimeout(time.Minute)) AfterEach(func(ctx SpecContext) { DeleteNamespaceCallbackWithContext(ctx, &testNamespace) - }, NodeTimeout(time.Minute)) + }, NodeTimeout(3*time.Minute)) Context("RLP targeting HTTPRoute", func() { It("Creates all the resources for a basic HTTPRoute and RateLimitPolicy", func() { @@ -329,12 +329,14 @@ var _ = Describe("RateLimitPolicy controller", func() { }) Context("RLP Overrides", func() { - It("Gateway atomic override - gateway overrides exist and then route policy created", func(ctx SpecContext) { + BeforeEach(func(ctx SpecContext) { // create httproute httpRoute := testBuildBasicHttpRoute(routeName, gwName, testNamespace, []string{"*.example.com"}) Expect(k8sClient.Create(ctx, httpRoute)).To(Succeed()) Eventually(testRouteIsAccepted(client.ObjectKeyFromObject(httpRoute))).WithContext(ctx).Should(BeTrue()) + }, NodeTimeout(time.Minute)) + It("Gateway atomic override - gateway overrides exist and then route policy created", func(ctx SpecContext) { // create GW RLP gwRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { policy.Spec.TargetRef.Kind = "Gateway" @@ -405,11 +407,6 @@ var _ = Describe("RateLimitPolicy controller", func() { }, SpecTimeout(time.Minute)) It("Gateway atomic override - route policy exits and then gateway policy created", func(ctx SpecContext) { - // create httproute - httpRoute := testBuildBasicHttpRoute(routeName, gwName, testNamespace, []string{"*.example.com"}) - Expect(k8sClient.Create(ctx, httpRoute)).To(Succeed()) - Eventually(testRouteIsAccepted(client.ObjectKeyFromObject(httpRoute))).WithContext(ctx).Should(BeTrue()) - // Create HTTPRoute RLP routeRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { policy.Name = "httproute-rlp" @@ -480,11 +477,6 @@ var _ = Describe("RateLimitPolicy controller", func() { }, SpecTimeout(time.Minute)) It("Gateway atomic override - gateway defaults turned into overrides later on", func(ctx SpecContext) { - // create httproute - httpRoute := testBuildBasicHttpRoute(routeName, gwName, testNamespace, []string{"*.example.com"}) - Expect(k8sClient.Create(ctx, httpRoute)).To(Succeed()) - Eventually(testRouteIsAccepted(client.ObjectKeyFromObject(httpRoute))).WithContext(ctx).Should(BeTrue()) - // Create HTTPRoute RLP routeRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { policy.Name = "httproute-rlp" @@ -558,6 +550,82 @@ var _ = Describe("RateLimitPolicy controller", func() { })) }) }, SpecTimeout(time.Minute)) + + It("Gateway atomic override - gateway overrides turned into defaults later on", func(ctx SpecContext) { + // Create HTTPRoute RLP + routeRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Name = "httproute-rlp" + policy.Spec.CommonSpec().Limits = map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 10, Duration: 5, Unit: "second", + }, + }, + }, + } + }) + Expect(k8sClient.Create(ctx, routeRLP)).To(Succeed()) + rlpKey := client.ObjectKey{Name: routeRLP.Name, Namespace: testNamespace} + Eventually(testRLPIsAccepted(rlpKey)).WithContext(ctx).Should(BeTrue()) + + // create GW RLP + gwRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Spec.Defaults = nil + policy.Spec.TargetRef.Kind = "Gateway" + policy.Spec.TargetRef.Name = gatewayapiv1.ObjectName(gwName) + policy.Spec.Overrides = &kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: "minute", + }, + }, + }, + }, + } + }) + Expect(k8sClient.Create(ctx, gwRLP)).To(Succeed()) + rlpKey = client.ObjectKey{Name: gwRLP.Name, Namespace: testNamespace} + Eventually(testRLPIsAccepted(rlpKey)).WithContext(ctx).Should(BeTrue()) + + // check limits - overridden - should contain GW RLP values + limitadorKey := client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace} + existingLimitador := &limitadorv1alpha1.Limitador{} + Expect(k8sClient.Get(ctx, limitadorKey, existingLimitador)).To(Succeed()) + Expect(existingLimitador.Spec.Limits).To(ContainElements(limitadorv1alpha1.RateLimit{ + MaxValue: 1, + Seconds: 180, + Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP), + Conditions: []string{`limit.l1__2804bad6 == "1"`}, + Variables: []string{}, + Name: rlptools.LimitsNameFromRLP(routeRLP), + })) + + updatedGRLP := &kuadrantv1beta2.RateLimitPolicy{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: gwRLP.Name, Namespace: testNamespace}, updatedGRLP)).To(Succeed()) + Eventually(func(g Gomega) { + updatedGRLP.Spec.Defaults = updatedGRLP.Spec.Overrides.DeepCopy() + updatedGRLP.Spec.Overrides = nil + g.Expect(k8sClient.Update(ctx, updatedGRLP)).To(Succeed()) + }).WithContext(ctx).Should(Succeed()) + + Eventually(func(g Gomega) { + // check limits - should contain HTTPRoute RLP values + limitadorKey = client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace} + existingLimitador = &limitadorv1alpha1.Limitador{} + g.Expect(k8sClient.Get(ctx, limitadorKey, existingLimitador)).To(Succeed()) + g.Expect(existingLimitador.Spec.Limits).To(ContainElements(limitadorv1alpha1.RateLimit{ + MaxValue: 10, + Seconds: 5, + Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP), + Conditions: []string{`limit.l1__2804bad6 == "1"`}, + Variables: []string{}, + Name: rlptools.LimitsNameFromRLP(routeRLP), + })) + }) + }, SpecTimeout(2*time.Minute)) }) Context("RLP accepted condition reasons", func() { @@ -1146,10 +1214,10 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() { }) err := k8sClient.Create(ctx, policy) Expect(err).ToNot(BeNil()) - Expect(strings.Contains(err.Error(), "Implicit and explicit defaults are mutually exclusive")).To(BeTrue()) + Expect(err.Error()).To(ContainSubstring("Implicit and explicit defaults are mutually exclusive")) }) - It("Valid - explicit default and override defined", func(ctx SpecContext) { + It("Invalid - explicit default and override defined", func(ctx SpecContext) { policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { policy.Spec.Defaults = &kuadrantv1beta2.RateLimitPolicyCommonSpec{ Limits: map[string]kuadrantv1beta2.Limit{ @@ -1166,10 +1234,12 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() { }, } }) - Expect(k8sClient.Create(ctx, policy)).To(Succeed()) + err := k8sClient.Create(ctx, policy) + Expect(err).ToNot(BeNil()) + Expect(err.Error()).To(ContainSubstring("Overrides and explicit defaults are mutually exclusive")) }) - It("Invalid - implicit default and explicit override are mutually exclusive", func(ctx SpecContext) { + It("Invalid - implicit default and override defined", func(ctx SpecContext) { policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { policy.Spec.Limits = map[string]kuadrantv1beta2.Limit{ "implicit": { @@ -1178,7 +1248,7 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() { } policy.Spec.Overrides = &kuadrantv1beta2.RateLimitPolicyCommonSpec{ Limits: map[string]kuadrantv1beta2.Limit{ - "explicit": { + "overrides": { Rates: []kuadrantv1beta2.Rate{{Limit: 1, Duration: 10, Unit: "second"}}, }, }, @@ -1186,7 +1256,36 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() { }) err := k8sClient.Create(ctx, policy) Expect(err).ToNot(BeNil()) - Expect(strings.Contains(err.Error(), "Overrides and implicit defaults are mutually exclusive. Use defaults block instead to set defaults explicitly")).To(BeTrue()) + Expect(err.Error()).To(ContainSubstring("Overrides and implicit defaults are mutually exclusive")) + }) + + It("Invalid - policy override targeting resource other than Gateway", func(ctx SpecContext) { + policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Spec.Overrides = &kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "implicit": { + Rates: []kuadrantv1beta2.Rate{{Limit: 1, Duration: 10, Unit: "second"}}, + }, + }, + } + }) + err := k8sClient.Create(ctx, policy) + Expect(err).ToNot(BeNil()) + Expect(err.Error()).To(ContainSubstring("Overrides are only allowed for policies targeting a Gateway resource")) + }) + + It("Valid - policy override targeting Gateway", func(ctx SpecContext) { + policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Spec.TargetRef.Kind = "Gateway" + policy.Spec.Overrides = &kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "override": { + Rates: []kuadrantv1beta2.Rate{{Limit: 1, Duration: 10, Unit: "second"}}, + }, + }, + } + }) + Expect(k8sClient.Create(ctx, policy)).To(Succeed()) }) }) From 8bafd7ff9760a684cef5c1abe0e482684674a1d6 Mon Sep 17 00:00:00 2001 From: KevFan Date: Mon, 15 Apr 2024 09:47:48 +0100 Subject: [PATCH 05/14] docs: RLP overrides --- api/v1beta2/ratelimitpolicy_types.go | 2 +- bundle/manifests/kuadrant.io_ratelimitpolicies.yaml | 2 +- config/crd/bases/kuadrant.io_ratelimitpolicies.yaml | 2 +- doc/reference/ratelimitpolicy.md | 11 ++++++----- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/api/v1beta2/ratelimitpolicy_types.go b/api/v1beta2/ratelimitpolicy_types.go index b74d5b5a2..fa704ee53 100644 --- a/api/v1beta2/ratelimitpolicy_types.go +++ b/api/v1beta2/ratelimitpolicy_types.go @@ -137,7 +137,7 @@ type RateLimitPolicySpec struct { Defaults *RateLimitPolicyCommonSpec `json:"defaults,omitempty"` // Overrides define override values for this policy and for policies inheriting this policy. - // Defaults are mutually exclusive with implicit defaults defined by RateLimitPolicyCommonSpec. + // Overrides are mutually exclusive with implicit defaults and explicit Defaults defined by RateLimitPolicyCommonSpec. // +optional Overrides *RateLimitPolicyCommonSpec `json:"overrides,omitempty"` diff --git a/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml b/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml index e06ca01b7..76884d89f 100644 --- a/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml +++ b/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml @@ -850,7 +850,7 @@ spec: overrides: description: |- Overrides define override values for this policy and for policies inheriting this policy. - Defaults are mutually exclusive with implicit defaults defined by RateLimitPolicyCommonSpec. + Overrides are mutually exclusive with implicit defaults and explicit Defaults defined by RateLimitPolicyCommonSpec. properties: limits: additionalProperties: diff --git a/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml b/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml index 96899903b..95cebf811 100644 --- a/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml +++ b/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml @@ -849,7 +849,7 @@ spec: overrides: description: |- Overrides define override values for this policy and for policies inheriting this policy. - Defaults are mutually exclusive with implicit defaults defined by RateLimitPolicyCommonSpec. + Overrides are mutually exclusive with implicit defaults and explicit Defaults defined by RateLimitPolicyCommonSpec. properties: limits: additionalProperties: diff --git a/doc/reference/ratelimitpolicy.md b/doc/reference/ratelimitpolicy.md index aac4fb735..72403cb1c 100644 --- a/doc/reference/ratelimitpolicy.md +++ b/doc/reference/ratelimitpolicy.md @@ -18,11 +18,12 @@ ## RateLimitPolicySpec -| **Field** | **Type** | **Required** | **Description** | -|-------------|---------------------------------------------------------------------------------------------------------------------------------------------|--------------|-------------------------------------------------------------------------------------------------------------| -| `targetRef` | [PolicyTargetReference](https://gateway-api.sigs.k8s.io/v1alpha2/references/spec/#gateway.networking.k8s.io/v1alpha2.PolicyTargetReference) | Yes | Reference to a Kubernetes resource that the policy attaches to | -| `defaults` | [RateLimitPolicyCommonSpec](#rateLimitPolicyCommonSpec) | No | Default limit definitions. This field is mutually exclusive with the `limits` field | -| `limits` | Map | No | Limit definitions. This field is mutually exclusive with the [`defaults`](#rateLimitPolicyCommonSpec) field | +| **Field** | **Type** | **Required** | **Description** | +|-------------|---------------------------------------------------------------------------------------------------------------------------------------------|--------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `targetRef` | [PolicyTargetReference](https://gateway-api.sigs.k8s.io/v1alpha2/references/spec/#gateway.networking.k8s.io/v1alpha2.PolicyTargetReference) | Yes | Reference to a Kubernetes resource that the policy attaches to | +| `defaults` | [RateLimitPolicyCommonSpec](#rateLimitPolicyCommonSpec) | No | Default limit definitions. This field is mutually exclusive with the `limits` field | +| `overrides` | [RateLimitPolicyCommonSpec](#rateLimitPolicyCommonSpec) | No | Overrides limit definitions. This field is mutually exclusive with the `limits` field and `defaults` field. This field is only allowed for policies targeting `Gateway` in `targetRef.kind` | +| `limits` | Map | No | Limit definitions. This field is mutually exclusive with the [`defaults`](#rateLimitPolicyCommonSpec) field | ### RateLimitPolicyCommonSpec From 26ec9df155f6f5ac4abfd96fe5909960a03f916d Mon Sep 17 00:00:00 2001 From: KevFan Date: Wed, 17 Apr 2024 14:49:46 +0100 Subject: [PATCH 06/14] feat: rlp enforced condition --- controllers/ratelimitpolicy_controller.go | 5 ++ .../ratelimitpolicy_controller_test.go | 64 ++++++++++++++ controllers/ratelimitpolicy_limits.go | 49 ++++++----- controllers/ratelimitpolicy_status.go | 32 ++++++- pkg/library/mappers/limitador_to_rlps.go | 47 ++++++++++ pkg/library/mappers/limitador_to_rlps_test.go | 88 +++++++++++++++++++ 6 files changed, 264 insertions(+), 21 deletions(-) create mode 100644 pkg/library/mappers/limitador_to_rlps.go create mode 100644 pkg/library/mappers/limitador_to_rlps_test.go diff --git a/controllers/ratelimitpolicy_controller.go b/controllers/ratelimitpolicy_controller.go index b5e8fe3ff..9793aacfe 100644 --- a/controllers/ratelimitpolicy_controller.go +++ b/controllers/ratelimitpolicy_controller.go @@ -21,6 +21,7 @@ import ( "encoding/json" "github.com/go-logr/logr" + limitadorv1alpha1 "github.com/kuadrant/limitador-operator/api/v1alpha1" apierrors "k8s.io/apimachinery/pkg/api/errors" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -223,9 +224,13 @@ func (r *RateLimitPolicyReconciler) deleteNetworkResourceDirectBackReference(ctx func (r *RateLimitPolicyReconciler) SetupWithManager(mgr ctrl.Manager) error { httpRouteEventMapper := mappers.NewHTTPRouteEventMapper(mappers.WithLogger(r.Logger().WithName("httpRouteEventMapper"))) gatewayEventMapper := mappers.NewGatewayEventMapper(mappers.WithLogger(r.Logger().WithName("gatewayEventMapper"))) + limitadorToRLPsEventMapper := mappers.NewLimitadorToRateLimitPoliciesEventMapper( + mappers.WithLogger(r.Logger().WithName("limitadorToRLPsEventMapper")), + mappers.WithClient(r.Client())) return ctrl.NewControllerManagedBy(mgr). For(&kuadrantv1beta2.RateLimitPolicy{}). + Watches(&limitadorv1alpha1.Limitador{}, handler.EnqueueRequestsFromMapFunc(limitadorToRLPsEventMapper.Map)). Watches( &gatewayapiv1.HTTPRoute{}, handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, object client.Object) []reconcile.Request { diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index a912e9fef..d15fc95f0 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -12,6 +12,8 @@ import ( limitadorv1alpha1 "github.com/kuadrant/limitador-operator/api/v1alpha1" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + appsv1 "k8s.io/api/apps/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/ptr" @@ -21,6 +23,7 @@ import ( kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" "github.com/kuadrant/kuadrant-operator/pkg/common" + "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" "github.com/kuadrant/kuadrant-operator/pkg/rlptools" ) @@ -688,6 +691,67 @@ var _ = Describe("RateLimitPolicy controller", func() { }, SpecTimeout(time.Minute)) }) + Context("RLP Enforced Reasons", func() { + assertAcceptedCondTrueAndEnforcedCond := func(policy *kuadrantv1beta2.RateLimitPolicy, conditionStatus metav1.ConditionStatus, reason, message string) func(g Gomega) { + return func(g Gomega) { + existingPolicy := &kuadrantv1beta2.RateLimitPolicy{} + g.Expect(k8sClient.Get(context.Background(), client.ObjectKeyFromObject(policy), existingPolicy)).To(Succeed()) + acceptedCond := meta.FindStatusCondition(existingPolicy.Status.Conditions, string(gatewayapiv1alpha2.PolicyConditionAccepted)) + g.Expect(acceptedCond).ToNot(BeNil()) + + acceptedCondMatch := acceptedCond.Status == metav1.ConditionTrue && acceptedCond.Reason == string(gatewayapiv1alpha2.PolicyReasonAccepted) + + enforcedCond := meta.FindStatusCondition(existingPolicy.Status.Conditions, string(kuadrant.PolicyReasonEnforced)) + g.Expect(enforcedCond).ToNot(BeNil()) + enforcedCondMatch := enforcedCond.Status == conditionStatus && enforcedCond.Reason == reason && enforcedCond.Message == message + + g.Expect(acceptedCondMatch && enforcedCondMatch).To(BeTrue()) + } + } + + BeforeEach(func(ctx SpecContext) { + route := testBuildBasicHttpRoute(testHTTPRouteName, testGatewayName, testNamespace, []string{"*.toystore.com"}) + Expect(k8sClient.Create(ctx, route)).To(Succeed()) + Eventually(testRouteIsAccepted(client.ObjectKeyFromObject(route))).WithContext(ctx).Should(BeTrue()) + }, NodeTimeout(time.Minute)) + + It("Enforced Reason", func(ctx SpecContext) { + policy := policyFactory() + Expect(k8sClient.Create(ctx, policy)).To(Succeed()) + + Eventually(assertAcceptedCondTrueAndEnforcedCond(policy, metav1.ConditionTrue, string(kuadrant.PolicyReasonEnforced), + "RateLimitPolicy has been successfully enforced")).WithContext(ctx).Should(Succeed()) + + // Remove limitador deployment to simulate enforcement error + // RLP should transition to enforcement false in this case + Expect(k8sClient.Delete(ctx, &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Name: "limitador-limitador", Namespace: testNamespace}})).To(Succeed()) + Eventually(func() bool { + return apierrors.IsNotFound(k8sClient.Get(ctx, client.ObjectKey{Name: "limitador-limitador", Namespace: testNamespace}, &appsv1.Deployment{})) + }).WithContext(ctx).Should(BeTrue()) + + Eventually(assertAcceptedCondTrueAndEnforcedCond(policy, metav1.ConditionFalse, string(kuadrant.PolicyReasonUnknown), + "RateLimitPolicy has encountered some issues: limitador is not ready")).WithContext(ctx).Should(Succeed()) + }, SpecTimeout(time.Minute)) + + It("Unknown Reason", func(ctx SpecContext) { + // Remove limitador deployment to simulate enforcement error + Expect(k8sClient.Delete(ctx, &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Name: "limitador-limitador", Namespace: testNamespace}})).To(Succeed()) + Eventually(func() bool { + return apierrors.IsNotFound(k8sClient.Get(ctx, client.ObjectKey{Name: "limitador-limitador", Namespace: testNamespace}, &appsv1.Deployment{})) + }).WithContext(ctx).Should(BeTrue()) + + // Enforced false as limitador is not ready + policy := policyFactory() + Expect(k8sClient.Create(ctx, policy)).To(Succeed()) + Eventually(assertAcceptedCondTrueAndEnforcedCond(policy, metav1.ConditionFalse, string(kuadrant.PolicyReasonUnknown), + "RateLimitPolicy has encountered some issues: limitador is not ready")).WithContext(ctx).Should(Succeed()) + + // Enforced true once limitador is ready + Eventually(assertAcceptedCondTrueAndEnforcedCond(policy, metav1.ConditionTrue, string(kuadrant.PolicyReasonEnforced), + "RateLimitPolicy has been successfully enforced")).WithContext(ctx).Should(Succeed()) + }, SpecTimeout(time.Minute)) + }) + Context("When RLP switches target from one HTTPRoute to another HTTPRoute", func() { var ( routeAName = "route-a" diff --git a/controllers/ratelimitpolicy_limits.go b/controllers/ratelimitpolicy_limits.go index d63540b05..254d9f85e 100644 --- a/controllers/ratelimitpolicy_limits.go +++ b/controllers/ratelimitpolicy_limits.go @@ -48,8 +48,31 @@ func (r *RateLimitPolicyReconciler) reconcileLimitador(ctx context.Context, rlp if err != nil { return err } - // get the current limitador cr for the kuadrant instance so we can compare if it needs to be updated + limitador, err := r.getLimitador(ctx, rlp) + if err != nil { + return err + } + // return if limitador is up-to-date + if rlptools.Equal(rateLimitIndex.ToRateLimits(), limitador.Spec.Limits) { + logger.V(1).Info("limitador is up to date, skipping update") + return nil + } + + // update limitador + limitador.Spec.Limits = rateLimitIndex.ToRateLimits() + err = r.UpdateResource(ctx, limitador) + logger.V(1).Info("update limitador", "limitador", client.ObjectKeyFromObject(limitador), "err", err) + if err != nil { + return err + } + + return nil +} + +func (r *RateLimitPolicyReconciler) getLimitador(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy) (*limitadorv1alpha1.Limitador, error) { + logger, _ := logr.FromContext(ctx) + logger.V(1).Info("get kuadrant namespace") var kuadrantNamespace string kuadrantNamespace, isSet := kuadrant.GetKuadrantNamespaceFromPolicy(rlp) @@ -58,38 +81,24 @@ func (r *RateLimitPolicyReconciler) reconcileLimitador(ctx context.Context, rlp kuadrantNamespace, err = kuadrant.GetKuadrantNamespaceFromPolicyTargetRef(ctx, r.Client(), rlp) if err != nil { logger.Error(err, "failed to get kuadrant namespace") - return err + return nil, err } kuadrant.AnnotateObject(rlp, kuadrantNamespace) err = r.UpdateResource(ctx, rlp) // @guicassolato: not sure if this belongs to here if err != nil { logger.Error(err, "failed to update policy, re-queuing") - return err + return nil, err } } limitadorKey := client.ObjectKey{Name: common.LimitadorName, Namespace: kuadrantNamespace} limitador := &limitadorv1alpha1.Limitador{} - err = r.Client().Get(ctx, limitadorKey, limitador) + err := r.Client().Get(ctx, limitadorKey, limitador) logger.V(1).Info("get limitador", "limitador", limitadorKey, "err", err) if err != nil { - return err - } - - // return if limitador is up to date - if rlptools.Equal(rateLimitIndex.ToRateLimits(), limitador.Spec.Limits) { - logger.V(1).Info("limitador is up to date, skipping update") - return nil - } - - // update limitador - limitador.Spec.Limits = rateLimitIndex.ToRateLimits() - err = r.UpdateResource(ctx, limitador) - logger.V(1).Info("update limitador", "limitador", limitadorKey, "err", err) - if err != nil { - return err + return nil, err } - return nil + return limitador, nil } func (r *RateLimitPolicyReconciler) buildRateLimitIndex(ctx context.Context, rlpRefs []client.ObjectKey) (*rlptools.RateLimitIndex, error) { diff --git a/controllers/ratelimitpolicy_status.go b/controllers/ratelimitpolicy_status.go index 0edffd78e..d27757c19 100644 --- a/controllers/ratelimitpolicy_status.go +++ b/controllers/ratelimitpolicy_status.go @@ -2,14 +2,17 @@ package controllers import ( "context" + "errors" "fmt" "slices" "github.com/go-logr/logr" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/reconcile" + gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" @@ -51,7 +54,7 @@ func (r *RateLimitPolicyReconciler) reconcileStatus(ctx context.Context, rlp *ku return ctrl.Result{}, nil } -func (r *RateLimitPolicyReconciler) calculateStatus(_ context.Context, rlp *kuadrantv1beta2.RateLimitPolicy, specErr error) *kuadrantv1beta2.RateLimitPolicyStatus { +func (r *RateLimitPolicyReconciler) calculateStatus(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy, specErr error) *kuadrantv1beta2.RateLimitPolicyStatus { newStatus := &kuadrantv1beta2.RateLimitPolicyStatus{ // Copy initial conditions. Otherwise, status will always be updated Conditions: slices.Clone(rlp.Status.Conditions), @@ -62,5 +65,32 @@ func (r *RateLimitPolicyReconciler) calculateStatus(_ context.Context, rlp *kuad meta.SetStatusCondition(&newStatus.Conditions, *acceptedCond) + // Do not set enforced condition if Accepted condition is false + if meta.IsStatusConditionFalse(newStatus.Conditions, string(gatewayapiv1alpha2.PolicyReasonAccepted)) { + return newStatus + } + + enforcedCond := r.enforcedCondition(ctx, rlp) + meta.SetStatusCondition(&newStatus.Conditions, *enforcedCond) + return newStatus } + +func (r *RateLimitPolicyReconciler) enforcedCondition(ctx context.Context, policy *kuadrantv1beta2.RateLimitPolicy) *metav1.Condition { + logger, _ := logr.FromContext(ctx) + + limitador, err := r.getLimitador(ctx, policy) + if err != nil { + logger.V(1).Error(err, "failed to get limitador") + return kuadrant.EnforcedCondition(policy, kuadrant.NewErrUnknown(policy.Kind(), err), false) + } + if meta.IsStatusConditionFalse(limitador.Status.Conditions, "Ready") { + logger.V(1).Info("Limitador is not ready") + return kuadrant.EnforcedCondition(policy, kuadrant.NewErrUnknown(policy.Kind(), errors.New("limitador is not ready")), false) + } + + // TODO: Overridden + + logger.V(1).Info("RateLimitPolicy is enforced") + return kuadrant.EnforcedCondition(policy, nil, true) +} diff --git a/pkg/library/mappers/limitador_to_rlps.go b/pkg/library/mappers/limitador_to_rlps.go new file mode 100644 index 000000000..33efd9d2e --- /dev/null +++ b/pkg/library/mappers/limitador_to_rlps.go @@ -0,0 +1,47 @@ +package mappers + +import ( + "context" + + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + kuadrantv1beta1 "github.com/kuadrant/kuadrant-operator/api/v1beta1" + kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" + "github.com/kuadrant/kuadrant-operator/pkg/library/utils" +) + +type LimitadorToRateLimitPoliciesEventMapper struct { + opts MapperOptions +} + +func NewLimitadorToRateLimitPoliciesEventMapper(o ...MapperOption) *LimitadorToRateLimitPoliciesEventMapper { + return &LimitadorToRateLimitPoliciesEventMapper{opts: Apply(o...)} +} + +// Map limitador to RLP requests +func (m *LimitadorToRateLimitPoliciesEventMapper) Map(ctx context.Context, obj client.Object) []reconcile.Request { + kuadrantList := &kuadrantv1beta1.KuadrantList{} + if err := m.opts.Client.List(ctx, kuadrantList, &client.ListOptions{Namespace: obj.GetNamespace()}); err != nil { + m.opts.Logger.V(1).Error(err, "failed to list kuadrant in namespace", "namespace", obj.GetNamespace()) + return []reconcile.Request{} + } + + // No kuadrant in limitador namespace - skipping as it's not managed by kuadrant + if len(kuadrantList.Items) == 0 { + m.opts.Logger.V(1).Info("no kuadrant resources found in limitador namespace, skipping") + return []reconcile.Request{} + } + + // List all RLPs as there's been an event from Limitador which may affect RLP status + rlpList := &kuadrantv1beta2.RateLimitPolicyList{} + if err := m.opts.Client.List(ctx, rlpList); err != nil { + m.opts.Logger.V(1).Error(err, "failed to list RLPs") + return []reconcile.Request{} + } + + return utils.Map(rlpList.Items, func(policy kuadrantv1beta2.RateLimitPolicy) reconcile.Request { + return reconcile.Request{NamespacedName: types.NamespacedName{Namespace: policy.GetNamespace(), Name: policy.GetName()}} + }) +} diff --git a/pkg/library/mappers/limitador_to_rlps_test.go b/pkg/library/mappers/limitador_to_rlps_test.go new file mode 100644 index 000000000..77e273436 --- /dev/null +++ b/pkg/library/mappers/limitador_to_rlps_test.go @@ -0,0 +1,88 @@ +package mappers + +import ( + "context" + "reflect" + "testing" + + limitadorv1alpha1 "github.com/kuadrant/limitador-operator/api/v1alpha1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + kuadrantv1beta1 "github.com/kuadrant/kuadrant-operator/api/v1beta1" + kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" + "github.com/kuadrant/kuadrant-operator/pkg/log" +) + +func TestLimitadorToRateLimitPoliciesEventMapper_Map(t *testing.T) { + scheme := runtime.NewScheme() + if err := kuadrantv1beta1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + if err := kuadrantv1beta2.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + + type fields struct { + opts MapperOptions + } + type args struct { + ctx context.Context + obj client.Object + } + tests := []struct { + name string + fields fields + args args + want []reconcile.Request + }{ + { + name: "no kuadrants in object ns", + fields: fields{ + opts: MapperOptions{Logger: log.NewLogger(), + Client: fake.NewClientBuilder().WithObjects( + &kuadrantv1beta1.Kuadrant{ObjectMeta: metav1.ObjectMeta{Namespace: "kuadrant"}}, + ).WithScheme(scheme).Build()}, + }, + args: args{ + ctx: context.Background(), + obj: &limitadorv1alpha1.Limitador{ObjectMeta: metav1.ObjectMeta{Namespace: "limitador"}}, + }, + want: []reconcile.Request{}, + }, + { + name: "kuadrant in object ns - map RLP to requests", + fields: fields{ + opts: MapperOptions{ + Logger: log.NewLogger(), + Client: fake.NewClientBuilder().WithObjects( + &kuadrantv1beta1.Kuadrant{ObjectMeta: metav1.ObjectMeta{Namespace: "kuadrant"}}, + &kuadrantv1beta2.RateLimitPolicy{ObjectMeta: metav1.ObjectMeta{Name: "rlp1", Namespace: "ns1"}}, + &kuadrantv1beta2.RateLimitPolicy{ObjectMeta: metav1.ObjectMeta{Name: "rlp2", Namespace: "ns2"}}, + ).WithScheme(scheme).Build()}, + }, + args: args{ + ctx: context.Background(), + obj: &limitadorv1alpha1.Limitador{ObjectMeta: metav1.ObjectMeta{Namespace: "kuadrant"}}, + }, + want: []reconcile.Request{ + {NamespacedName: types.NamespacedName{Name: "rlp1", Namespace: "ns1"}}, + {NamespacedName: types.NamespacedName{Name: "rlp2", Namespace: "ns2"}}, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + m := &LimitadorToRateLimitPoliciesEventMapper{ + opts: tt.fields.opts, + } + if got := m.Map(tt.args.ctx, tt.args.obj); !reflect.DeepEqual(got, tt.want) { + t.Errorf("Map() = %v, want %v", got, tt.want) + } + }) + } +} From d7c61f7804e53c07679438fba65bde556825f4fa Mon Sep 17 00:00:00 2001 From: KevFan Date: Thu, 18 Apr 2024 11:17:56 +0100 Subject: [PATCH 07/14] refactor: event handler for limitador status changes only --- api/v1beta2/ratelimitpolicy_types.go | 4 + controllers/ratelimitpolicy_controller.go | 7 +- ...itpolicy_limitador_status_event_handler.go | 84 +++++++++++++++++ controllers/ratelimitpolicy_limits.go | 94 ++++++++++++++----- controllers/ratelimitpolicy_status.go | 4 +- controllers/suite_test.go | 1 + main.go | 1 + pkg/library/mappers/limitador_to_rlps.go | 47 ---------- pkg/library/mappers/limitador_to_rlps_test.go | 88 ----------------- 9 files changed, 164 insertions(+), 166 deletions(-) create mode 100644 controllers/ratelimitpolicy_limitador_status_event_handler.go delete mode 100644 pkg/library/mappers/limitador_to_rlps.go delete mode 100644 pkg/library/mappers/limitador_to_rlps_test.go diff --git a/api/v1beta2/ratelimitpolicy_types.go b/api/v1beta2/ratelimitpolicy_types.go index fa704ee53..7b509c027 100644 --- a/api/v1beta2/ratelimitpolicy_types.go +++ b/api/v1beta2/ratelimitpolicy_types.go @@ -294,6 +294,10 @@ func (r *RateLimitPolicySpec) CommonSpec() *RateLimitPolicyCommonSpec { return r.Defaults } + if r.Overrides != nil { + return r.Overrides + } + return &r.RateLimitPolicyCommonSpec } diff --git a/controllers/ratelimitpolicy_controller.go b/controllers/ratelimitpolicy_controller.go index 9793aacfe..92cb572b1 100644 --- a/controllers/ratelimitpolicy_controller.go +++ b/controllers/ratelimitpolicy_controller.go @@ -42,6 +42,8 @@ const rateLimitPolicyFinalizer = "ratelimitpolicy.kuadrant.io/finalizer" type RateLimitPolicyReconciler struct { *reconcilers.BaseReconciler TargetRefReconciler reconcilers.TargetRefReconciler + // OverriddenPolicyMap tracks the overridden policies to report their status. + OverriddenPolicyMap *kuadrant.OverriddenPolicyMap } //+kubebuilder:rbac:groups=kuadrant.io,resources=ratelimitpolicies,verbs=get;list;watch;create;update;patch;delete @@ -224,13 +226,10 @@ func (r *RateLimitPolicyReconciler) deleteNetworkResourceDirectBackReference(ctx func (r *RateLimitPolicyReconciler) SetupWithManager(mgr ctrl.Manager) error { httpRouteEventMapper := mappers.NewHTTPRouteEventMapper(mappers.WithLogger(r.Logger().WithName("httpRouteEventMapper"))) gatewayEventMapper := mappers.NewGatewayEventMapper(mappers.WithLogger(r.Logger().WithName("gatewayEventMapper"))) - limitadorToRLPsEventMapper := mappers.NewLimitadorToRateLimitPoliciesEventMapper( - mappers.WithLogger(r.Logger().WithName("limitadorToRLPsEventMapper")), - mappers.WithClient(r.Client())) return ctrl.NewControllerManagedBy(mgr). For(&kuadrantv1beta2.RateLimitPolicy{}). - Watches(&limitadorv1alpha1.Limitador{}, handler.EnqueueRequestsFromMapFunc(limitadorToRLPsEventMapper.Map)). + Watches(&limitadorv1alpha1.Limitador{}, limitadorStatusEventHandler{Client: r.Client(), Logger: r.Logger().WithName("limitadorStatusToRLPsEventHandler")}). Watches( &gatewayapiv1.HTTPRoute{}, handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, object client.Object) []reconcile.Request { diff --git a/controllers/ratelimitpolicy_limitador_status_event_handler.go b/controllers/ratelimitpolicy_limitador_status_event_handler.go new file mode 100644 index 000000000..69ba179aa --- /dev/null +++ b/controllers/ratelimitpolicy_limitador_status_event_handler.go @@ -0,0 +1,84 @@ +package controllers + +import ( + "context" + + "github.com/go-logr/logr" + limitadorv1alpha1 "github.com/kuadrant/limitador-operator/api/v1alpha1" + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/client-go/util/workqueue" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/handler" + + kuadrantv1beta1 "github.com/kuadrant/kuadrant-operator/api/v1beta1" + kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" + "github.com/kuadrant/kuadrant-operator/pkg/common" +) + +var _ handler.EventHandler = &limitadorStatusEventHandler{} + +type limitadorStatusEventHandler struct { + Client client.Client + Logger logr.Logger +} + +func (eh limitadorStatusEventHandler) Create(_ context.Context, _ event.CreateEvent, _ workqueue.RateLimitingInterface) { +} + +func (eh limitadorStatusEventHandler) Update(ctx context.Context, e event.UpdateEvent, limitingInterface workqueue.RateLimitingInterface) { + oldL := e.ObjectOld.(*limitadorv1alpha1.Limitador) + newL := e.ObjectNew.(*limitadorv1alpha1.Limitador) + + if !eh.IsKuadrantInstalled(ctx, oldL) { + return + } + + oldCond := meta.FindStatusCondition(oldL.Status.Conditions, "Ready") + newCond := meta.FindStatusCondition(newL.Status.Conditions, "Ready") + + if oldCond != nil && newCond != nil && oldCond.Status != newCond.Status && oldL.Name == common.LimitadorName { + eh.Logger.V(1).Info("Limitador status Ready condition change event detected") + eh.enqueue(ctx, limitingInterface) + } +} + +func (eh limitadorStatusEventHandler) Delete(ctx context.Context, e event.DeleteEvent, limitingInterface workqueue.RateLimitingInterface) { + eh.Logger.V(1).Info("Limitador delete event detected") + if !eh.IsKuadrantInstalled(ctx, e.Object) || e.Object.GetName() == common.LimitadorName { + eh.enqueue(ctx, limitingInterface) + } +} + +func (eh limitadorStatusEventHandler) Generic(_ context.Context, _ event.GenericEvent, _ workqueue.RateLimitingInterface) { +} + +func (eh limitadorStatusEventHandler) IsKuadrantInstalled(ctx context.Context, obj client.Object) bool { + kuadrantList := &kuadrantv1beta1.KuadrantList{} + if err := eh.Client.List(ctx, kuadrantList, &client.ListOptions{Namespace: obj.GetNamespace()}); err != nil { + eh.Logger.V(1).Error(err, "failed to list kuadrant in namespace", "namespace", obj.GetNamespace()) + return false + } + + // No kuadrant in limitador namespace - skipping as it's not managed by kuadrant + if len(kuadrantList.Items) == 0 { + eh.Logger.V(1).Info("no kuadrant resources found in limitador namespace, skipping") + return false + } + + return true +} +func (eh limitadorStatusEventHandler) enqueue(ctx context.Context, limitingInterface workqueue.RateLimitingInterface) { + // List all RLPs as there's been an event from Limitador which may affect RLP status + rlpList := &kuadrantv1beta2.RateLimitPolicyList{} + if err := eh.Client.List(ctx, rlpList); err != nil { + eh.Logger.V(1).Error(err, "failed to list RLPs") + } + for idx := range rlpList.Items { + eh.Logger.V(1).Info("queueing rate limiting policy", "policy", rlpList.Items[idx].Name) + limitingInterface.Add(ctrl.Request{ + NamespacedName: client.ObjectKeyFromObject(&rlpList.Items[idx]), + }) + } +} diff --git a/controllers/ratelimitpolicy_limits.go b/controllers/ratelimitpolicy_limits.go index 254d9f85e..c37aabec8 100644 --- a/controllers/ratelimitpolicy_limits.go +++ b/controllers/ratelimitpolicy_limits.go @@ -124,7 +124,9 @@ func (r *RateLimitPolicyReconciler) buildRateLimitIndex(ctx context.Context, rlp return nil, err } - r.applyOverrides(ctx, rlp, t) + if err := r.applyOverrides(ctx, rlp, t); err != nil { + return nil, err + } rateLimitIndex.Set(rlpKey, rlptools.LimitadorRateLimitsFromRLP(rlp)) } @@ -135,13 +137,72 @@ func (r *RateLimitPolicyReconciler) buildRateLimitIndex(ctx context.Context, rlp // applyOverrides checks for any overrides set for the RateLimitPolicy. // It iterates through the slice of policies to find overrides for the provided target HTTPRoute. // If an override is found, it updates the limits in the RateLimitPolicySpec accordingly. -func (r *RateLimitPolicyReconciler) applyOverrides(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy, t *kuadrantgatewayapi.Topology) { +func (r *RateLimitPolicyReconciler) applyOverrides(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy, t *kuadrantgatewayapi.Topology) error { logger, _ := logr.FromContext(ctx) logger = logger.WithName("applyOverrides") + affectedPolicies, numUnTargetedRoutes := r.getAffectedPolicies(rlp, t) + + filteredPolicies := utils.Filter(affectedPolicies, func(policy kuadrantgatewayapi.Policy) bool { + // HTTPRoute RLPs should only care about overrides from gateways + if kuadrantgatewayapi.IsTargetRefHTTPRoute(rlp.GetTargetRef()) { + return kuadrantgatewayapi.IsTargetRefGateway(policy.GetTargetRef()) + } + // Gateway RLPs are not affected by other Gateway RLPs + return false + }) + + // Is a GW policy + if len(filteredPolicies) == 0 { + // Specifies defaults and no free routes => not enforced + if rlp.Spec.Overrides == nil && numUnTargetedRoutes == 0 { + r.OverriddenPolicyMap.SetOverriddenPolicy(rlp) + logger.V(1).Info("policy has no free routes to enforce policy") + } else { + r.OverriddenPolicyMap.RemoveOverriddenPolicy(rlp) + } + } else { + // Is a Route policy + // Sort by TargetRefKind and creation timestamp + // Gateways RLPs are listed first in the order of oldest policy + sort.Sort(kuadrantgatewayapi.PolicyByTargetRefKindAndCreationTimeStamp(filteredPolicies)) + + // Iterate in order of precedence until finding a block of overrides + for _, policy := range filteredPolicies { + p := policy.(*kuadrantv1beta2.RateLimitPolicy) + if p.Spec.Overrides != nil { + rlp.Spec.CommonSpec().Limits = p.Spec.Overrides.Limits + logger.V(1).Info("applying overrides from parent policy", "parentPolicy", client.ObjectKeyFromObject(p)) + // Overridden by another policy + r.OverriddenPolicyMap.SetOverriddenPolicy(rlp) + break + } + r.OverriddenPolicyMap.RemoveOverriddenPolicy(rlp) + } + } + + // Reconcile status for all policies that are affected by this RLP + for _, policy := range affectedPolicies { + if policy.GetUID() != rlp.GetUID() { + p := policy.(*kuadrantv1beta2.RateLimitPolicy) + if kuadrantgatewayapi.IsTargetRefGateway(rlp.GetTargetRef()) && rlp.Spec.Overrides != nil { + r.OverriddenPolicyMap.SetOverriddenPolicy(p) + } + _, err := r.reconcileStatus(ctx, p, nil) + if err != nil { + return err + } + } + } + + return nil +} + +func (r *RateLimitPolicyReconciler) getAffectedPolicies(rlp *kuadrantv1beta2.RateLimitPolicy, t *kuadrantgatewayapi.Topology) ([]kuadrantgatewayapi.Policy, int) { topologyIndexes := kuadrantgatewayapi.NewTopologyIndexes(t) - var policies []kuadrantgatewayapi.Policy + var affectedPolicies []kuadrantgatewayapi.Policy + var numUnTargetedRoutes int // For each gw, get all the policies if the current rlp is within the topology for this gateway for _, gw := range t.Gateways() { policyList := topologyIndexes.PoliciesFromGateway(gw.Gateway) @@ -149,34 +210,15 @@ func (r *RateLimitPolicyReconciler) applyOverrides(ctx context.Context, rlp *kua return client.ObjectKeyFromObject(p) }) + numUnTargetedRoutes = numUnTargetedRoutes + len(topologyIndexes.GetUntargetedRoutes(gw.Gateway)) + // this policy is potentially affected by other policies from this gateway if slices.Contains(policyKeys, client.ObjectKeyFromObject(rlp)) { - policies = append(policies, policyList...) + affectedPolicies = append(affectedPolicies, policyList...) } } - filteredPolicies := utils.Filter(policies, func(policy kuadrantgatewayapi.Policy) bool { - // HTTPRoute RLPs should only care about overrides from gateways - if kuadrantgatewayapi.IsTargetRefHTTPRoute(rlp.GetTargetRef()) { - return kuadrantgatewayapi.IsTargetRefGateway(policy.GetTargetRef()) - } - // Gateway RLPs are not affected by other Gateway RLPs - return false - }) - - // Sort by TargetRefKind and creation timestamp - // Gateways RLPs are listed first in the order of oldest policy - sort.Sort(kuadrantgatewayapi.PolicyByTargetRefKindAndCreationTimeStamp(filteredPolicies)) - - // Iterate in order of precedence until finding a block of overrides - for _, policy := range filteredPolicies { - p := policy.(*kuadrantv1beta2.RateLimitPolicy) - if p.Spec.Overrides != nil { - rlp.Spec.CommonSpec().Limits = p.Spec.Overrides.Limits - logger.V(1).Info("applying overrides from parent policy", "parentPolicy", client.ObjectKeyFromObject(p)) - break - } - } + return affectedPolicies, numUnTargetedRoutes } func (r *RateLimitPolicyReconciler) generateTopology(ctx context.Context) (*kuadrantgatewayapi.Topology, error) { diff --git a/controllers/ratelimitpolicy_status.go b/controllers/ratelimitpolicy_status.go index d27757c19..b3b9b074a 100644 --- a/controllers/ratelimitpolicy_status.go +++ b/controllers/ratelimitpolicy_status.go @@ -89,7 +89,9 @@ func (r *RateLimitPolicyReconciler) enforcedCondition(ctx context.Context, polic return kuadrant.EnforcedCondition(policy, kuadrant.NewErrUnknown(policy.Kind(), errors.New("limitador is not ready")), false) } - // TODO: Overridden + if r.OverriddenPolicyMap.IsPolicyOverridden(policy) { + return kuadrant.EnforcedCondition(policy, kuadrant.NewErrOverridden(policy.Kind(), "overridden"), false) + } logger.V(1).Info("RateLimitPolicy is enforced") return kuadrant.EnforcedCondition(policy, nil, true) diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 61edd2e7c..3279acc3c 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -160,6 +160,7 @@ var _ = BeforeSuite(func() { err = (&RateLimitPolicyReconciler{ BaseReconciler: rateLimitPolicyBaseReconciler, TargetRefReconciler: reconcilers.TargetRefReconciler{Client: mgr.GetClient()}, + OverriddenPolicyMap: kuadrant.NewOverriddenPolicyMap(), }).SetupWithManager(mgr) Expect(err).NotTo(HaveOccurred()) diff --git a/main.go b/main.go index 75f44f596..0eecc7dac 100644 --- a/main.go +++ b/main.go @@ -154,6 +154,7 @@ func main() { if err = (&controllers.RateLimitPolicyReconciler{ TargetRefReconciler: reconcilers.TargetRefReconciler{Client: mgr.GetClient()}, BaseReconciler: rateLimitPolicyBaseReconciler, + OverriddenPolicyMap: kuadrant.NewOverriddenPolicyMap(), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "RateLimitPolicy") os.Exit(1) diff --git a/pkg/library/mappers/limitador_to_rlps.go b/pkg/library/mappers/limitador_to_rlps.go deleted file mode 100644 index 33efd9d2e..000000000 --- a/pkg/library/mappers/limitador_to_rlps.go +++ /dev/null @@ -1,47 +0,0 @@ -package mappers - -import ( - "context" - - "k8s.io/apimachinery/pkg/types" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/reconcile" - - kuadrantv1beta1 "github.com/kuadrant/kuadrant-operator/api/v1beta1" - kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" - "github.com/kuadrant/kuadrant-operator/pkg/library/utils" -) - -type LimitadorToRateLimitPoliciesEventMapper struct { - opts MapperOptions -} - -func NewLimitadorToRateLimitPoliciesEventMapper(o ...MapperOption) *LimitadorToRateLimitPoliciesEventMapper { - return &LimitadorToRateLimitPoliciesEventMapper{opts: Apply(o...)} -} - -// Map limitador to RLP requests -func (m *LimitadorToRateLimitPoliciesEventMapper) Map(ctx context.Context, obj client.Object) []reconcile.Request { - kuadrantList := &kuadrantv1beta1.KuadrantList{} - if err := m.opts.Client.List(ctx, kuadrantList, &client.ListOptions{Namespace: obj.GetNamespace()}); err != nil { - m.opts.Logger.V(1).Error(err, "failed to list kuadrant in namespace", "namespace", obj.GetNamespace()) - return []reconcile.Request{} - } - - // No kuadrant in limitador namespace - skipping as it's not managed by kuadrant - if len(kuadrantList.Items) == 0 { - m.opts.Logger.V(1).Info("no kuadrant resources found in limitador namespace, skipping") - return []reconcile.Request{} - } - - // List all RLPs as there's been an event from Limitador which may affect RLP status - rlpList := &kuadrantv1beta2.RateLimitPolicyList{} - if err := m.opts.Client.List(ctx, rlpList); err != nil { - m.opts.Logger.V(1).Error(err, "failed to list RLPs") - return []reconcile.Request{} - } - - return utils.Map(rlpList.Items, func(policy kuadrantv1beta2.RateLimitPolicy) reconcile.Request { - return reconcile.Request{NamespacedName: types.NamespacedName{Namespace: policy.GetNamespace(), Name: policy.GetName()}} - }) -} diff --git a/pkg/library/mappers/limitador_to_rlps_test.go b/pkg/library/mappers/limitador_to_rlps_test.go deleted file mode 100644 index 77e273436..000000000 --- a/pkg/library/mappers/limitador_to_rlps_test.go +++ /dev/null @@ -1,88 +0,0 @@ -package mappers - -import ( - "context" - "reflect" - "testing" - - limitadorv1alpha1 "github.com/kuadrant/limitador-operator/api/v1alpha1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/client/fake" - "sigs.k8s.io/controller-runtime/pkg/reconcile" - - kuadrantv1beta1 "github.com/kuadrant/kuadrant-operator/api/v1beta1" - kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" - "github.com/kuadrant/kuadrant-operator/pkg/log" -) - -func TestLimitadorToRateLimitPoliciesEventMapper_Map(t *testing.T) { - scheme := runtime.NewScheme() - if err := kuadrantv1beta1.AddToScheme(scheme); err != nil { - t.Fatal(err) - } - if err := kuadrantv1beta2.AddToScheme(scheme); err != nil { - t.Fatal(err) - } - - type fields struct { - opts MapperOptions - } - type args struct { - ctx context.Context - obj client.Object - } - tests := []struct { - name string - fields fields - args args - want []reconcile.Request - }{ - { - name: "no kuadrants in object ns", - fields: fields{ - opts: MapperOptions{Logger: log.NewLogger(), - Client: fake.NewClientBuilder().WithObjects( - &kuadrantv1beta1.Kuadrant{ObjectMeta: metav1.ObjectMeta{Namespace: "kuadrant"}}, - ).WithScheme(scheme).Build()}, - }, - args: args{ - ctx: context.Background(), - obj: &limitadorv1alpha1.Limitador{ObjectMeta: metav1.ObjectMeta{Namespace: "limitador"}}, - }, - want: []reconcile.Request{}, - }, - { - name: "kuadrant in object ns - map RLP to requests", - fields: fields{ - opts: MapperOptions{ - Logger: log.NewLogger(), - Client: fake.NewClientBuilder().WithObjects( - &kuadrantv1beta1.Kuadrant{ObjectMeta: metav1.ObjectMeta{Namespace: "kuadrant"}}, - &kuadrantv1beta2.RateLimitPolicy{ObjectMeta: metav1.ObjectMeta{Name: "rlp1", Namespace: "ns1"}}, - &kuadrantv1beta2.RateLimitPolicy{ObjectMeta: metav1.ObjectMeta{Name: "rlp2", Namespace: "ns2"}}, - ).WithScheme(scheme).Build()}, - }, - args: args{ - ctx: context.Background(), - obj: &limitadorv1alpha1.Limitador{ObjectMeta: metav1.ObjectMeta{Namespace: "kuadrant"}}, - }, - want: []reconcile.Request{ - {NamespacedName: types.NamespacedName{Name: "rlp1", Namespace: "ns1"}}, - {NamespacedName: types.NamespacedName{Name: "rlp2", Namespace: "ns2"}}, - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - m := &LimitadorToRateLimitPoliciesEventMapper{ - opts: tt.fields.opts, - } - if got := m.Map(tt.args.ctx, tt.args.obj); !reflect.DeepEqual(got, tt.want) { - t.Errorf("Map() = %v, want %v", got, tt.want) - } - }) - } -} From db61290fb995ef89c69e278ffc5049ba0ab4cbdc Mon Sep 17 00:00:00 2001 From: KevFan Date: Thu, 18 Apr 2024 17:42:49 +0100 Subject: [PATCH 08/14] refactor: overridden policy map --- controllers/authpolicy_authconfig.go | 2 +- controllers/authpolicy_controller_test.go | 2 +- controllers/authpolicy_status.go | 9 +- controllers/helper_test.go | 19 +++- .../ratelimitpolicy_controller_test.go | 101 ++++++++++-------- controllers/ratelimitpolicy_limits.go | 21 +++- controllers/ratelimitpolicy_status.go | 2 +- .../apimachinery_status_conditions.go | 16 ++- .../apimachinery_status_conditions_test.go | 5 +- pkg/library/kuadrant/errors.go | 5 +- 10 files changed, 107 insertions(+), 75 deletions(-) diff --git a/controllers/authpolicy_authconfig.go b/controllers/authpolicy_authconfig.go index 925fc5307..0e40e2ae0 100644 --- a/controllers/authpolicy_authconfig.go +++ b/controllers/authpolicy_authconfig.go @@ -105,7 +105,7 @@ func (r *AuthPolicyReconciler) desiredAuthConfig(ctx context.Context, ap *api.Au if len(rules) == 0 { logger.V(1).Info("no httproutes attached to the targeted gateway, skipping authorino authconfig for the gateway authpolicy") utils.TagObjectToDelete(authConfig) - r.OverriddenPolicyMap.SetOverriddenPolicy(ap) + r.OverriddenPolicyMap.SetOverriddenPolicy(ap, []client.ObjectKey{}) return authConfig, nil } route = &gatewayapiv1.HTTPRoute{ diff --git a/controllers/authpolicy_controller_test.go b/controllers/authpolicy_controller_test.go index 15707cbc2..ea9402a8a 100644 --- a/controllers/authpolicy_controller_test.go +++ b/controllers/authpolicy_controller_test.go @@ -1243,7 +1243,7 @@ var _ = Describe("AuthPolicy controller", func() { Eventually(isAuthPolicyAccepted(gwPolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) Eventually( assertAcceptedCondTrueAndEnforcedCond(gwPolicy, metav1.ConditionFalse, string(kuadrant.PolicyReasonOverridden), - fmt.Sprintf("AuthPolicy is overridden by [{\"Namespace\":\"%s\",\"Name\":\"%s\"}]", testNamespace, routePolicy.Name)), + fmt.Sprintf("AuthPolicy is overridden by [%s/%s]", testNamespace, routePolicy.Name)), 30*time.Second, 5*time.Second).Should(BeTrue()) // check istio authorizationpolicy diff --git a/controllers/authpolicy_status.go b/controllers/authpolicy_status.go index 6823816ad..2e1d1c184 100644 --- a/controllers/authpolicy_status.go +++ b/controllers/authpolicy_status.go @@ -2,7 +2,6 @@ package controllers import ( "context" - "encoding/json" "errors" "fmt" "slices" @@ -159,12 +158,8 @@ func (r *AuthPolicyReconciler) handleGatewayPolicyOverride(logger logr.Logger, p filteredRef := utils.Filter(refs, func(key client.ObjectKey) bool { return key != client.ObjectKeyFromObject(policy) }) - jsonData, err := json.Marshal(filteredRef) - if err != nil { - logger.Error(err, "Failed to marshal filtered references") - return kuadrant.EnforcedCondition(policy, kuadrant.NewErrUnknown(policy.Kind(), err), false) - } - return kuadrant.EnforcedCondition(policy, kuadrant.NewErrOverridden(policy.Kind(), string(jsonData)), false) + + return kuadrant.EnforcedCondition(policy, kuadrant.NewErrOverridden(policy.Kind(), filteredRef), false) } // handleHTTPRoutePolicyOverride handles the case where the HTTPRoute Policy is overridden by filtering policy references diff --git a/controllers/helper_test.go b/controllers/helper_test.go index a2b78ee5f..3e3084563 100644 --- a/controllers/helper_test.go +++ b/controllers/helper_test.go @@ -37,6 +37,7 @@ import ( kuadrantv1beta1 "github.com/kuadrant/kuadrant-operator/api/v1beta1" kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" kuadrantgatewayapi "github.com/kuadrant/kuadrant-operator/pkg/library/gatewayapi" + "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" "github.com/kuadrant/kuadrant-operator/pkg/library/utils" ) @@ -395,6 +396,14 @@ func testWasmPluginIsAvailable(key client.ObjectKey) func() bool { } func testRLPIsAccepted(rlpKey client.ObjectKey) func() bool { + return testRLPIsConditionTrue(rlpKey, string(gatewayapiv1alpha2.PolicyConditionAccepted)) +} + +func testRLPIsEnforced(rlpKey client.ObjectKey) func() bool { + return testRLPIsConditionTrue(rlpKey, string(kuadrant.PolicyConditionEnforced)) +} + +func testRLPIsNotAccepted(rlpKey client.ObjectKey) func() bool { return func() bool { existingRLP := &kuadrantv1beta2.RateLimitPolicy{} err := k8sClient.Get(context.Background(), rlpKey, existingRLP) @@ -402,8 +411,8 @@ func testRLPIsAccepted(rlpKey client.ObjectKey) func() bool { logf.Log.V(1).Info("ratelimitpolicy not read", "rlp", rlpKey, "error", err) return false } - if !meta.IsStatusConditionTrue(existingRLP.Status.Conditions, string(gatewayapiv1alpha2.PolicyConditionAccepted)) { - logf.Log.V(1).Info("ratelimitpolicy not available", "rlp", rlpKey) + if meta.IsStatusConditionTrue(existingRLP.Status.Conditions, string(gatewayapiv1alpha2.PolicyConditionAccepted)) { + logf.Log.V(1).Info("ratelimitpolicy still accepted", "rlp", rlpKey) return false } @@ -411,7 +420,7 @@ func testRLPIsAccepted(rlpKey client.ObjectKey) func() bool { } } -func testRLPIsNotAccepted(rlpKey client.ObjectKey) func() bool { +func testRLPIsConditionTrue(rlpKey client.ObjectKey, condition string) func() bool { return func() bool { existingRLP := &kuadrantv1beta2.RateLimitPolicy{} err := k8sClient.Get(context.Background(), rlpKey, existingRLP) @@ -419,8 +428,8 @@ func testRLPIsNotAccepted(rlpKey client.ObjectKey) func() bool { logf.Log.V(1).Info("ratelimitpolicy not read", "rlp", rlpKey, "error", err) return false } - if meta.IsStatusConditionTrue(existingRLP.Status.Conditions, string(gatewayapiv1alpha2.PolicyConditionAccepted)) { - logf.Log.V(1).Info("ratelimitpolicy still accepted", "rlp", rlpKey) + if !meta.IsStatusConditionTrue(existingRLP.Status.Conditions, condition) { + logf.Log.V(1).Info("ratelimitpolicy not available", "rlp", rlpKey) return false } diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index d15fc95f0..a9aaf4c0d 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -309,25 +309,28 @@ var _ = Describe("RateLimitPolicy controller", func() { gwRLP.DirectReferenceAnnotationName(), client.ObjectKeyFromObject(gwRLP).String())) // check limits - limitadorKey := client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace} - existingLimitador := &limitadorv1alpha1.Limitador{} - Expect(k8sClient.Get(ctx, limitadorKey, existingLimitador)).To(Succeed()) - Expect(existingLimitador.Spec.Limits).To(ContainElements(limitadorv1alpha1.RateLimit{ - MaxValue: 10, - Seconds: 5, - Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP), - Conditions: []string{`limit.l1__2804bad6 == "1"`}, - Variables: []string{}, - Name: rlptools.LimitsNameFromRLP(routeRLP), - })) + Eventually(func(g Gomega) { + limitadorKey := client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace} + existingLimitador := &limitadorv1alpha1.Limitador{} + g.Expect(k8sClient.Get(ctx, limitadorKey, existingLimitador)).To(Succeed()) + g.Expect(existingLimitador.Spec.Limits).To(ContainElements(limitadorv1alpha1.RateLimit{ + MaxValue: 10, + Seconds: 5, + Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP), + Conditions: []string{`limit.l1__2804bad6 == "1"`}, + Variables: []string{}, + Name: rlptools.LimitsNameFromRLP(routeRLP), + })) + }).WithContext(ctx).Should(Succeed()) // Gateway should contain HTTPRoute RLP in backreference - Expect(k8sClient.Get(ctx, gwKey, existingGateway)).To(Succeed()) - serialized, err := json.Marshal(rlpKey) - Expect(err).ToNot(HaveOccurred()) - Expect(existingGateway.GetAnnotations()).To(HaveKey(routeRLP.BackReferenceAnnotationName())) - Expect(existingGateway.GetAnnotations()[routeRLP.BackReferenceAnnotationName()]).To(ContainSubstring(string(serialized))) - + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, gwKey, existingGateway)).To(Succeed()) + serialized, err := json.Marshal(rlpKey) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(existingGateway.GetAnnotations()).To(HaveKey(routeRLP.BackReferenceAnnotationName())) + g.Expect(existingGateway.GetAnnotations()[routeRLP.BackReferenceAnnotationName()]).To(ContainSubstring(string(serialized))) + }).WithContext(ctx).Should(Succeed()) }, SpecTimeout(time.Minute)) }) @@ -401,12 +404,13 @@ var _ = Describe("RateLimitPolicy controller", func() { }).WithContext(ctx).Should(Succeed()) // Gateway should contain HTTPRoute RLP in backreference - Expect(k8sClient.Get(ctx, gwKey, existingGateway)).To(Succeed()) - serialized, err := json.Marshal(rlpKey) - Expect(err).ToNot(HaveOccurred()) - Expect(existingGateway.GetAnnotations()).To(HaveKey(routeRLP.BackReferenceAnnotationName())) - Expect(existingGateway.GetAnnotations()[routeRLP.BackReferenceAnnotationName()]).To(ContainSubstring(string(serialized))) - + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, gwKey, existingGateway)).To(Succeed()) + serialized, err := json.Marshal(rlpKey) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(existingGateway.GetAnnotations()).To(HaveKey(routeRLP.BackReferenceAnnotationName())) + g.Expect(existingGateway.GetAnnotations()[routeRLP.BackReferenceAnnotationName()]).To(ContainSubstring(string(serialized))) + }).WithContext(ctx).Should(Succeed()) }, SpecTimeout(time.Minute)) It("Gateway atomic override - route policy exits and then gateway policy created", func(ctx SpecContext) { @@ -451,9 +455,11 @@ var _ = Describe("RateLimitPolicy controller", func() { // Check Gateway direct back reference gwKey := client.ObjectKeyFromObject(gateway) existingGateway := &gatewayapiv1.Gateway{} - Expect(k8sClient.Get(ctx, gwKey, existingGateway)).To(Succeed()) - Expect(existingGateway.GetAnnotations()).To(HaveKeyWithValue( - gwRLP.DirectReferenceAnnotationName(), client.ObjectKeyFromObject(gwRLP).String())) + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, gwKey, existingGateway)).To(Succeed()) + g.Expect(existingGateway.GetAnnotations()).To(HaveKeyWithValue( + gwRLP.DirectReferenceAnnotationName(), client.ObjectKeyFromObject(gwRLP).String())) + }).WithContext(ctx).Should(Succeed()) Eventually(func(g Gomega) { // check limits - should contain override values @@ -471,12 +477,13 @@ var _ = Describe("RateLimitPolicy controller", func() { }) // Gateway should contain HTTPRoute RLP in backreference - Expect(k8sClient.Get(ctx, gwKey, existingGateway)).To(Succeed()) - serialized, err := json.Marshal(rlpKey) - Expect(err).ToNot(HaveOccurred()) - Expect(existingGateway.GetAnnotations()).To(HaveKey(routeRLP.BackReferenceAnnotationName())) - Expect(existingGateway.GetAnnotations()[routeRLP.BackReferenceAnnotationName()]).To(ContainSubstring(string(serialized))) - + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, gwKey, existingGateway)).To(Succeed()) + serialized, err := json.Marshal(rlpKey) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(existingGateway.GetAnnotations()).To(HaveKey(routeRLP.BackReferenceAnnotationName())) + g.Expect(existingGateway.GetAnnotations()[routeRLP.BackReferenceAnnotationName()]).To(ContainSubstring(string(serialized))) + }).WithContext(ctx).Should(Succeed()) }, SpecTimeout(time.Minute)) It("Gateway atomic override - gateway defaults turned into overrides later on", func(ctx SpecContext) { @@ -594,17 +601,19 @@ var _ = Describe("RateLimitPolicy controller", func() { Eventually(testRLPIsAccepted(rlpKey)).WithContext(ctx).Should(BeTrue()) // check limits - overridden - should contain GW RLP values - limitadorKey := client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace} - existingLimitador := &limitadorv1alpha1.Limitador{} - Expect(k8sClient.Get(ctx, limitadorKey, existingLimitador)).To(Succeed()) - Expect(existingLimitador.Spec.Limits).To(ContainElements(limitadorv1alpha1.RateLimit{ - MaxValue: 1, - Seconds: 180, - Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP), - Conditions: []string{`limit.l1__2804bad6 == "1"`}, - Variables: []string{}, - Name: rlptools.LimitsNameFromRLP(routeRLP), - })) + Eventually(func(g Gomega) { + limitadorKey := client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace} + existingLimitador := &limitadorv1alpha1.Limitador{} + g.Expect(k8sClient.Get(ctx, limitadorKey, existingLimitador)).To(Succeed()) + g.Expect(existingLimitador.Spec.Limits).To(ContainElements(limitadorv1alpha1.RateLimit{ + MaxValue: 1, + Seconds: 180, + Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP), + Conditions: []string{`limit.l1__2804bad6 == "1"`}, + Variables: []string{}, + Name: rlptools.LimitsNameFromRLP(routeRLP), + })) + }).WithContext(ctx).Should(Succeed()) updatedGRLP := &kuadrantv1beta2.RateLimitPolicy{} Expect(k8sClient.Get(ctx, client.ObjectKey{Name: gwRLP.Name, Namespace: testNamespace}, updatedGRLP)).To(Succeed()) @@ -616,8 +625,8 @@ var _ = Describe("RateLimitPolicy controller", func() { Eventually(func(g Gomega) { // check limits - should contain HTTPRoute RLP values - limitadorKey = client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace} - existingLimitador = &limitadorv1alpha1.Limitador{} + limitadorKey := client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace} + existingLimitador := &limitadorv1alpha1.Limitador{} g.Expect(k8sClient.Get(ctx, limitadorKey, existingLimitador)).To(Succeed()) g.Expect(existingLimitador.Spec.Limits).To(ContainElements(limitadorv1alpha1.RateLimit{ MaxValue: 10, @@ -627,7 +636,7 @@ var _ = Describe("RateLimitPolicy controller", func() { Variables: []string{}, Name: rlptools.LimitsNameFromRLP(routeRLP), })) - }) + }).WithContext(ctx).Should(Succeed()) }, SpecTimeout(2*time.Minute)) }) diff --git a/controllers/ratelimitpolicy_limits.go b/controllers/ratelimitpolicy_limits.go index c37aabec8..f7e3ca82e 100644 --- a/controllers/ratelimitpolicy_limits.go +++ b/controllers/ratelimitpolicy_limits.go @@ -142,8 +142,19 @@ func (r *RateLimitPolicyReconciler) applyOverrides(ctx context.Context, rlp *kua logger = logger.WithName("applyOverrides") affectedPolicies, numUnTargetedRoutes := r.getAffectedPolicies(rlp, t) + affectedPolicies = utils.Filter(affectedPolicies, func(policy kuadrantgatewayapi.Policy) bool { + // Filter out current policy from affected policies + return policy.GetUID() != rlp.GetUID() + }) + affectedPoliciesKeys := utils.Map(affectedPolicies, func(p kuadrantgatewayapi.Policy) client.ObjectKey { + return client.ObjectKeyFromObject(p) + }) filteredPolicies := utils.Filter(affectedPolicies, func(policy kuadrantgatewayapi.Policy) bool { + if policy.GetUID() == rlp.GetUID() { + return false + } + // HTTPRoute RLPs should only care about overrides from gateways if kuadrantgatewayapi.IsTargetRefHTTPRoute(rlp.GetTargetRef()) { return kuadrantgatewayapi.IsTargetRefGateway(policy.GetTargetRef()) @@ -153,10 +164,10 @@ func (r *RateLimitPolicyReconciler) applyOverrides(ctx context.Context, rlp *kua }) // Is a GW policy - if len(filteredPolicies) == 0 { - // Specifies defaults and no free routes => not enforced + if kuadrantgatewayapi.IsTargetRefGateway(rlp.GetTargetRef()) { + // Specifies defaults and has free routes => not enforced if rlp.Spec.Overrides == nil && numUnTargetedRoutes == 0 { - r.OverriddenPolicyMap.SetOverriddenPolicy(rlp) + r.OverriddenPolicyMap.SetOverriddenPolicy(rlp, affectedPoliciesKeys) logger.V(1).Info("policy has no free routes to enforce policy") } else { r.OverriddenPolicyMap.RemoveOverriddenPolicy(rlp) @@ -174,7 +185,7 @@ func (r *RateLimitPolicyReconciler) applyOverrides(ctx context.Context, rlp *kua rlp.Spec.CommonSpec().Limits = p.Spec.Overrides.Limits logger.V(1).Info("applying overrides from parent policy", "parentPolicy", client.ObjectKeyFromObject(p)) // Overridden by another policy - r.OverriddenPolicyMap.SetOverriddenPolicy(rlp) + r.OverriddenPolicyMap.SetOverriddenPolicy(rlp, []client.ObjectKey{client.ObjectKeyFromObject(p)}) break } r.OverriddenPolicyMap.RemoveOverriddenPolicy(rlp) @@ -186,7 +197,7 @@ func (r *RateLimitPolicyReconciler) applyOverrides(ctx context.Context, rlp *kua if policy.GetUID() != rlp.GetUID() { p := policy.(*kuadrantv1beta2.RateLimitPolicy) if kuadrantgatewayapi.IsTargetRefGateway(rlp.GetTargetRef()) && rlp.Spec.Overrides != nil { - r.OverriddenPolicyMap.SetOverriddenPolicy(p) + r.OverriddenPolicyMap.SetOverriddenPolicy(p, []client.ObjectKey{client.ObjectKeyFromObject(rlp)}) } _, err := r.reconcileStatus(ctx, p, nil) if err != nil { diff --git a/controllers/ratelimitpolicy_status.go b/controllers/ratelimitpolicy_status.go index b3b9b074a..5530c89db 100644 --- a/controllers/ratelimitpolicy_status.go +++ b/controllers/ratelimitpolicy_status.go @@ -90,7 +90,7 @@ func (r *RateLimitPolicyReconciler) enforcedCondition(ctx context.Context, polic } if r.OverriddenPolicyMap.IsPolicyOverridden(policy) { - return kuadrant.EnforcedCondition(policy, kuadrant.NewErrOverridden(policy.Kind(), "overridden"), false) + return kuadrant.EnforcedCondition(policy, kuadrant.NewErrOverridden(policy.Kind(), r.OverriddenPolicyMap.PolicyOverriddenBy(policy)), false) } logger.V(1).Info("RateLimitPolicy is enforced") diff --git a/pkg/library/kuadrant/apimachinery_status_conditions.go b/pkg/library/kuadrant/apimachinery_status_conditions.go index a87123b8a..99ae86384 100644 --- a/pkg/library/kuadrant/apimachinery_status_conditions.go +++ b/pkg/library/kuadrant/apimachinery_status_conditions.go @@ -10,6 +10,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" ) @@ -23,24 +24,24 @@ const ( func NewOverriddenPolicyMap() *OverriddenPolicyMap { return &OverriddenPolicyMap{ - policies: make(map[types.UID]bool), + policies: make(map[types.UID][]client.ObjectKey), } } type OverriddenPolicyMap struct { - policies map[types.UID]bool + policies map[types.UID][]client.ObjectKey mu sync.RWMutex } // SetOverriddenPolicy sets the provided Policy as overridden in the tracking map. -func (o *OverriddenPolicyMap) SetOverriddenPolicy(p Policy) { +func (o *OverriddenPolicyMap) SetOverriddenPolicy(p Policy, affectedBy []client.ObjectKey) { o.mu.Lock() defer o.mu.Unlock() if o.policies == nil { - o.policies = make(map[types.UID]bool) + o.policies = make(map[types.UID][]client.ObjectKey) } - o.policies[p.GetUID()] = true + o.policies[p.GetUID()] = affectedBy } // RemoveOverriddenPolicy removes the provided Policy from the tracking map of overridden policies. @@ -53,6 +54,11 @@ func (o *OverriddenPolicyMap) RemoveOverriddenPolicy(p Policy) { // IsPolicyOverridden checks if the provided Policy is overridden based on the tracking map maintained. func (o *OverriddenPolicyMap) IsPolicyOverridden(p Policy) bool { + return o.policies[p.GetUID()] != nil +} + +// PolicyOverriddenBy returns the clients keys that a policy is overridden by +func (o *OverriddenPolicyMap) PolicyOverriddenBy(p Policy) []client.ObjectKey { return o.policies[p.GetUID()] } diff --git a/pkg/library/kuadrant/apimachinery_status_conditions_test.go b/pkg/library/kuadrant/apimachinery_status_conditions_test.go index 3c33968cc..5d5b27362 100644 --- a/pkg/library/kuadrant/apimachinery_status_conditions_test.go +++ b/pkg/library/kuadrant/apimachinery_status_conditions_test.go @@ -12,6 +12,7 @@ import ( apiErrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/controller-runtime/pkg/client" gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" ) @@ -265,13 +266,13 @@ func TestEnforcedCondition(t *testing.T) { name: "enforced false - overridden", args: args{ policy: &FakePolicy{}, - err: NewErrOverridden(policy.Kind(), "ns1/policy1"), + err: NewErrOverridden(policy.Kind(), []client.ObjectKey{{Namespace: "ns1", Name: "policy1"}, {Namespace: "ns2", Name: "policy2"}}), }, want: &metav1.Condition{ Type: string(PolicyConditionEnforced), Status: metav1.ConditionFalse, Reason: string(PolicyReasonOverridden), - Message: "FakePolicy is overridden by ns1/policy1", + Message: "FakePolicy is overridden by [ns1/policy1 ns2/policy2]", }, }, } diff --git a/pkg/library/kuadrant/errors.go b/pkg/library/kuadrant/errors.go index 42847d823..48bde13ab 100644 --- a/pkg/library/kuadrant/errors.go +++ b/pkg/library/kuadrant/errors.go @@ -5,6 +5,7 @@ import ( "fmt" apierrors "k8s.io/apimachinery/pkg/api/errors" + "sigs.k8s.io/controller-runtime/pkg/client" gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" ) @@ -113,7 +114,7 @@ var _ PolicyError = ErrOverridden{} type ErrOverridden struct { Kind string - OverridingPolicies string + OverridingPolicies []client.ObjectKey } func (e ErrOverridden) Error() string { @@ -124,7 +125,7 @@ func (e ErrOverridden) Reason() gatewayapiv1alpha2.PolicyConditionReason { return PolicyReasonOverridden } -func NewErrOverridden(kind, overridingPolicies string) ErrOverridden { +func NewErrOverridden(kind string, overridingPolicies []client.ObjectKey) ErrOverridden { return ErrOverridden{ Kind: kind, OverridingPolicies: overridingPolicies, From cf114630bebdc7527999a5110dc4b6f8f1184b63 Mon Sep 17 00:00:00 2001 From: KevFan Date: Fri, 19 Apr 2024 11:15:49 +0100 Subject: [PATCH 09/14] refactor: override logic and integration tests --- controllers/authpolicy_authconfig.go | 2 +- controllers/authpolicy_status.go | 23 +- controllers/helper_test.go | 25 +- .../ratelimitpolicy_controller_test.go | 382 ++++++++---------- controllers/ratelimitpolicy_limits.go | 109 +++-- controllers/ratelimitpolicy_status.go | 3 + pkg/library/gatewayapi/types.go | 5 + pkg/library/gatewayapi/types_test.go | 4 + pkg/library/kuadrant/httproute_wrapper.go | 26 +- 9 files changed, 281 insertions(+), 298 deletions(-) diff --git a/controllers/authpolicy_authconfig.go b/controllers/authpolicy_authconfig.go index 0e40e2ae0..576bf4740 100644 --- a/controllers/authpolicy_authconfig.go +++ b/controllers/authpolicy_authconfig.go @@ -74,7 +74,7 @@ func (r *AuthPolicyReconciler) desiredAuthConfig(ctx context.Context, ap *api.Au if ok { logger.V(1).Info("targeted gateway has authpolicy with atomic overrides, skipping authorino authconfig for the HTTPRoute authpolicy") utils.TagObjectToDelete(authConfig) - r.OverriddenPolicyMap.SetOverriddenPolicy(ap) + r.OverriddenPolicyMap.SetOverriddenPolicy(ap, []client.ObjectKey{}) return authConfig, nil } route = obj diff --git a/controllers/authpolicy_status.go b/controllers/authpolicy_status.go index 2e1d1c184..d6d0b13a2 100644 --- a/controllers/authpolicy_status.go +++ b/controllers/authpolicy_status.go @@ -6,20 +6,19 @@ import ( "fmt" "slices" - kuadrantgatewayapi "github.com/kuadrant/kuadrant-operator/pkg/library/gatewayapi" - "k8s.io/utils/ptr" - "github.com/go-logr/logr" authorinoapi "github.com/kuadrant/authorino/api/v1beta2" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" api "github.com/kuadrant/kuadrant-operator/api/v1beta2" + kuadrantgatewayapi "github.com/kuadrant/kuadrant-operator/pkg/library/gatewayapi" "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" "github.com/kuadrant/kuadrant-operator/pkg/library/utils" ) @@ -140,9 +139,9 @@ func (r *AuthPolicyReconciler) isAuthConfigReady(ctx context.Context, policy *ap func (r *AuthPolicyReconciler) handlePolicyOverride(logger logr.Logger, policy *api.AuthPolicy, targetNetworkObject client.Object, t *kuadrantgatewayapi.Topology) *metav1.Condition { switch targetNetworkObject.(type) { case *gatewayapiv1.Gateway: - return r.handleGatewayPolicyOverride(logger, policy, targetNetworkObject) + return r.handleGatewayPolicyOverride(policy, targetNetworkObject) case *gatewayapiv1.HTTPRoute: - return r.handleHTTPRoutePolicyOverride(logger, policy, targetNetworkObject, t) + return r.handleHTTPRoutePolicyOverride(policy, targetNetworkObject, t) default: logger.Error(errors.New("this point should never be reached"), "failed to match target network object", targetNetworkObject) return nil @@ -151,7 +150,7 @@ func (r *AuthPolicyReconciler) handlePolicyOverride(logger logr.Logger, policy * // handleGatewayPolicyOverride handles the case where the Gateway Policy is overridden by filtering policy references // and creating a corresponding error condition. -func (r *AuthPolicyReconciler) handleGatewayPolicyOverride(logger logr.Logger, policy *api.AuthPolicy, targetNetworkObject client.Object) *metav1.Condition { +func (r *AuthPolicyReconciler) handleGatewayPolicyOverride(policy *api.AuthPolicy, targetNetworkObject client.Object) *metav1.Condition { obj := targetNetworkObject.(*gatewayapiv1.Gateway) gatewayWrapper := kuadrant.GatewayWrapper{Gateway: obj, Referrer: policy} refs := gatewayWrapper.PolicyRefs() @@ -164,16 +163,12 @@ func (r *AuthPolicyReconciler) handleGatewayPolicyOverride(logger logr.Logger, p // handleHTTPRoutePolicyOverride handles the case where the HTTPRoute Policy is overridden by filtering policy references // and creating a corresponding error condition. -func (r *AuthPolicyReconciler) handleHTTPRoutePolicyOverride(logger logr.Logger, policy *api.AuthPolicy, targetNetworkObject client.Object, t *kuadrantgatewayapi.Topology) *metav1.Condition { +func (r *AuthPolicyReconciler) handleHTTPRoutePolicyOverride(policy *api.AuthPolicy, targetNetworkObject client.Object, t *kuadrantgatewayapi.Topology) *metav1.Condition { obj := targetNetworkObject.(*gatewayapiv1.HTTPRoute) - httpRouteWrapper := kuadrant.HTTPRouteWrapper{HTTPRoute: obj, Referrer: policy} + httpRouteWrapper := kuadrant.HTTPRouteWrapper{HTTPRoute: obj, Policy: policy} refs := httpRouteWrapper.PolicyRefs(t) - jsonData, err := json.Marshal(refs) - if err != nil { - logger.Error(err, "Failed to marshal filtered references") - return kuadrant.EnforcedCondition(policy, kuadrant.NewErrUnknown(policy.Kind(), err), false) - } - return kuadrant.EnforcedCondition(policy, kuadrant.NewErrOverridden(policy.Kind(), string(jsonData)), false) + + return kuadrant.EnforcedCondition(policy, kuadrant.NewErrOverridden(policy.Kind(), refs), false) } func (r *AuthPolicyReconciler) generateTopology(ctx context.Context) (*kuadrantgatewayapi.Topology, error) { diff --git a/controllers/helper_test.go b/controllers/helper_test.go index 3e3084563..cc1ca51fd 100644 --- a/controllers/helper_test.go +++ b/controllers/helper_test.go @@ -403,6 +403,20 @@ func testRLPIsEnforced(rlpKey client.ObjectKey) func() bool { return testRLPIsConditionTrue(rlpKey, string(kuadrant.PolicyConditionEnforced)) } +func testRLPEnforcedCondition(rlpKey client.ObjectKey, reason gatewayapiv1alpha2.PolicyConditionReason, message string) bool { + p := &kuadrantv1beta2.RateLimitPolicy{} + if err := k8sClient.Get(context.Background(), rlpKey, p); err != nil { + return false + } + + cond := meta.FindStatusCondition(p.Status.Conditions, string(kuadrant.PolicyConditionEnforced)) + if cond == nil { + return false + } + + return cond.Reason == string(reason) && cond.Message == message +} + func testRLPIsNotAccepted(rlpKey client.ObjectKey) func() bool { return func() bool { existingRLP := &kuadrantv1beta2.RateLimitPolicy{} @@ -424,16 +438,7 @@ func testRLPIsConditionTrue(rlpKey client.ObjectKey, condition string) func() bo return func() bool { existingRLP := &kuadrantv1beta2.RateLimitPolicy{} err := k8sClient.Get(context.Background(), rlpKey, existingRLP) - if err != nil { - logf.Log.V(1).Info("ratelimitpolicy not read", "rlp", rlpKey, "error", err) - return false - } - if !meta.IsStatusConditionTrue(existingRLP.Status.Conditions, condition) { - logf.Log.V(1).Info("ratelimitpolicy not available", "rlp", rlpKey) - return false - } - - return true + return err == nil && meta.IsStatusConditionTrue(existingRLP.Status.Conditions, condition) } } diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index a9aaf4c0d..b4bc83c42 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -332,40 +332,64 @@ var _ = Describe("RateLimitPolicy controller", func() { g.Expect(existingGateway.GetAnnotations()[routeRLP.BackReferenceAnnotationName()]).To(ContainSubstring(string(serialized))) }).WithContext(ctx).Should(Succeed()) }, SpecTimeout(time.Minute)) + + It("Explicit defaults - no underlying routes to enforce policy", func(ctx SpecContext) { + gwRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Spec.TargetRef.Kind = "Gateway" + policy.Spec.TargetRef.Name = gatewayapiv1.ObjectName(gwName) + }) + + Expect(k8sClient.Create(ctx, gwRLP)).To(Succeed()) + gwRLPKey := client.ObjectKey{Name: gwRLP.Name, Namespace: testNamespace} + Eventually(testRLPIsAccepted(gwRLPKey)).WithContext(ctx).Should(BeTrue()) + Eventually(testRLPIsEnforced(gwRLPKey)).WithContext(ctx).Should(BeFalse()) + Expect(testRLPEnforcedCondition(gwRLPKey, kuadrant.PolicyReasonUnknown, "RateLimitPolicy has encountered some issues: no free routes to enforce policy")) + }, SpecTimeout(time.Minute)) + + It("Implicit defaults - no underlying routes to enforce policy", func(ctx SpecContext) { + gwRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Spec.TargetRef.Kind = "Gateway" + policy.Spec.TargetRef.Name = gatewayapiv1.ObjectName(gwName) + policy.Spec.RateLimitPolicyCommonSpec = *policy.Spec.Defaults.DeepCopy() + policy.Spec.Defaults = nil + }) + + Expect(k8sClient.Create(ctx, gwRLP)).To(Succeed()) + gwRLPKey := client.ObjectKey{Name: gwRLP.Name, Namespace: testNamespace} + Eventually(testRLPIsAccepted(gwRLPKey)).WithContext(ctx).Should(BeTrue()) + Eventually(testRLPIsEnforced(gwRLPKey)).WithContext(ctx).Should(BeFalse()) + Expect(testRLPEnforcedCondition(gwRLPKey, kuadrant.PolicyReasonUnknown, "RateLimitPolicy has encountered some issues: no free routes to enforce policy")) + }, SpecTimeout(time.Minute)) }) Context("RLP Overrides", func() { + var gwRLP *kuadrantv1beta2.RateLimitPolicy + var routeRLP *kuadrantv1beta2.RateLimitPolicy + + limitadorContainsLimit := func(ctx context.Context, limit limitadorv1alpha1.RateLimit) func(g Gomega) { + return func(g Gomega) { + // check limits - should contain HTTPRoute RLP values + limitadorKey := client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace} + existingLimitador := &limitadorv1alpha1.Limitador{} + g.Expect(k8sClient.Get(ctx, limitadorKey, existingLimitador)).To(Succeed()) + g.Expect(existingLimitador.Spec.Limits).To(ContainElements(limit)) + } + } + BeforeEach(func(ctx SpecContext) { // create httproute httpRoute := testBuildBasicHttpRoute(routeName, gwName, testNamespace, []string{"*.example.com"}) Expect(k8sClient.Create(ctx, httpRoute)).To(Succeed()) Eventually(testRouteIsAccepted(client.ObjectKeyFromObject(httpRoute))).WithContext(ctx).Should(BeTrue()) - }, NodeTimeout(time.Minute)) - It("Gateway atomic override - gateway overrides exist and then route policy created", func(ctx SpecContext) { - // create GW RLP - gwRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + gwRLP = policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { policy.Spec.TargetRef.Kind = "Gateway" policy.Spec.TargetRef.Name = gatewayapiv1.ObjectName(gwName) + policy.Spec.Overrides = policy.Spec.Defaults.DeepCopy() policy.Spec.Defaults = nil - policy.Spec.Overrides = &kuadrantv1beta2.RateLimitPolicyCommonSpec{ - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: "minute", - }, - }, - }, - }, - } }) - Expect(k8sClient.Create(ctx, gwRLP)).To(Succeed()) - rlpKey := client.ObjectKey{Name: gwRLP.Name, Namespace: testNamespace} - Eventually(testRLPIsAccepted(rlpKey)).WithContext(ctx).Should(BeTrue()) - // Create HTTPRoute RLP with new default limits - routeRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + routeRLP = policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { policy.Name = "httproute-rlp" policy.Spec.CommonSpec().Limits = map[string]kuadrantv1beta2.Limit{ "l1": { @@ -377,9 +401,22 @@ var _ = Describe("RateLimitPolicy controller", func() { }, } }) + + }, NodeTimeout(time.Minute)) + + It("Gateway atomic override - gateway overrides exist and then route policy created", func(ctx SpecContext) { + // create GW RLP with overrides + Expect(k8sClient.Create(ctx, gwRLP)).To(Succeed()) + gwRLPKey := client.ObjectKeyFromObject(gwRLP) + Eventually(testRLPIsAccepted(gwRLPKey)).WithContext(ctx).Should(BeTrue()) + Eventually(testRLPIsEnforced(gwRLPKey)).WithContext(ctx).Should(BeTrue()) + + // Create HTTPRoute RLP Expect(k8sClient.Create(ctx, routeRLP)).To(Succeed()) - rlpKey = client.ObjectKey{Name: routeRLP.Name, Namespace: testNamespace} - Eventually(testRLPIsAccepted(rlpKey)).WithContext(ctx).Should(BeTrue()) + routeRLPKey := client.ObjectKeyFromObject(routeRLP) + Eventually(testRLPIsAccepted(routeRLPKey)).WithContext(ctx).Should(BeTrue()) + Eventually(testRLPIsEnforced(routeRLPKey)).WithContext(ctx).Should(BeFalse()) + Expect(testRLPEnforcedCondition(routeRLPKey, kuadrant.PolicyReasonOverridden, fmt.Sprintf("RateLimitPolicy is overridden by [%s]", gwRLPKey))) // Check Gateway direct back reference gwKey := client.ObjectKeyFromObject(gateway) @@ -389,24 +426,19 @@ var _ = Describe("RateLimitPolicy controller", func() { gwRLP.DirectReferenceAnnotationName(), client.ObjectKeyFromObject(gwRLP).String())) // check limits - should contain override values - Eventually(func(g Gomega) { - limitadorKey := client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace} - existingLimitador := &limitadorv1alpha1.Limitador{} - g.Expect(k8sClient.Get(ctx, limitadorKey, existingLimitador)).To(Succeed()) - g.Expect(existingLimitador.Spec.Limits).To(ContainElements(limitadorv1alpha1.RateLimit{ - MaxValue: 1, - Seconds: 180, - Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP), - Conditions: []string{`limit.l1__2804bad6 == "1"`}, - Variables: []string{}, - Name: rlptools.LimitsNameFromRLP(routeRLP), - })) - }).WithContext(ctx).Should(Succeed()) + Eventually(limitadorContainsLimit(ctx, limitadorv1alpha1.RateLimit{ + MaxValue: 1, + Seconds: 180, + Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP), + Conditions: []string{`limit.l1__2804bad6 == "1"`}, + Variables: []string{}, + Name: rlptools.LimitsNameFromRLP(routeRLP), + })).WithContext(ctx).Should(Succeed()) // Gateway should contain HTTPRoute RLP in backreference Eventually(func(g Gomega) { g.Expect(k8sClient.Get(ctx, gwKey, existingGateway)).To(Succeed()) - serialized, err := json.Marshal(rlpKey) + serialized, err := json.Marshal(gwRLPKey) g.Expect(err).ToNot(HaveOccurred()) g.Expect(existingGateway.GetAnnotations()).To(HaveKey(routeRLP.BackReferenceAnnotationName())) g.Expect(existingGateway.GetAnnotations()[routeRLP.BackReferenceAnnotationName()]).To(ContainSubstring(string(serialized))) @@ -414,43 +446,21 @@ var _ = Describe("RateLimitPolicy controller", func() { }, SpecTimeout(time.Minute)) It("Gateway atomic override - route policy exits and then gateway policy created", func(ctx SpecContext) { - // Create HTTPRoute RLP - routeRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { - policy.Name = "httproute-rlp" - policy.Spec.CommonSpec().Limits = map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 10, Duration: 5, Unit: "second", - }, - }, - }, - } - }) + // Create Route RLP Expect(k8sClient.Create(ctx, routeRLP)).To(Succeed()) - rlpKey := client.ObjectKey{Name: routeRLP.Name, Namespace: testNamespace} - Eventually(testRLPIsAccepted(rlpKey)).WithContext(ctx).Should(BeTrue()) + routeRLPKey := client.ObjectKeyFromObject(routeRLP) + Eventually(testRLPIsAccepted(routeRLPKey)).WithContext(ctx).Should(BeTrue()) + Eventually(testRLPIsEnforced(routeRLPKey)).WithContext(ctx).Should(BeTrue()) - // create GW RLP - gwRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { - policy.Spec.TargetRef.Kind = "Gateway" - policy.Spec.TargetRef.Name = gatewayapiv1.ObjectName(gwName) - policy.Spec.Defaults = nil - policy.Spec.Overrides = &kuadrantv1beta2.RateLimitPolicyCommonSpec{ - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: "minute", - }, - }, - }, - }, - } - }) + // create GW RLP with override Expect(k8sClient.Create(ctx, gwRLP)).To(Succeed()) - rlpKey = client.ObjectKey{Name: gwRLP.Name, Namespace: testNamespace} - Eventually(testRLPIsAccepted(rlpKey)).WithContext(ctx).Should(BeTrue()) + gwRLPKey := client.ObjectKeyFromObject(gwRLP) + Eventually(testRLPIsAccepted(gwRLPKey)).WithContext(ctx).Should(BeTrue()) + Eventually(testRLPIsEnforced(gwRLPKey)).WithContext(ctx).Should(BeTrue()) + + // Route RLP should no longer be enforced + Eventually(testRLPIsEnforced(routeRLPKey)).WithContext(ctx).Should(BeFalse()) + Expect(testRLPEnforcedCondition(routeRLPKey, kuadrant.PolicyReasonOverridden, fmt.Sprintf("RateLimitPolicy is overridden by [%s]", gwRLPKey))) // Check Gateway direct back reference gwKey := client.ObjectKeyFromObject(gateway) @@ -461,25 +471,20 @@ var _ = Describe("RateLimitPolicy controller", func() { gwRLP.DirectReferenceAnnotationName(), client.ObjectKeyFromObject(gwRLP).String())) }).WithContext(ctx).Should(Succeed()) - Eventually(func(g Gomega) { - // check limits - should contain override values - limitadorKey := client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace} - existingLimitador := &limitadorv1alpha1.Limitador{} - Expect(k8sClient.Get(ctx, limitadorKey, existingLimitador)).To(Succeed()) - Expect(existingLimitador.Spec.Limits).To(ContainElements(limitadorv1alpha1.RateLimit{ - MaxValue: 1, - Seconds: 180, - Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP), - Conditions: []string{`limit.l1__2804bad6 == "1"`}, - Variables: []string{}, - Name: rlptools.LimitsNameFromRLP(routeRLP), - })) - }) + // Should contain override values + Eventually(limitadorContainsLimit(ctx, limitadorv1alpha1.RateLimit{ + MaxValue: 1, + Seconds: 180, + Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP), + Conditions: []string{`limit.l1__2804bad6 == "1"`}, + Variables: []string{}, + Name: rlptools.LimitsNameFromRLP(routeRLP), + })).WithContext(ctx).Should(Succeed()) // Gateway should contain HTTPRoute RLP in backreference Eventually(func(g Gomega) { g.Expect(k8sClient.Get(ctx, gwKey, existingGateway)).To(Succeed()) - serialized, err := json.Marshal(rlpKey) + serialized, err := json.Marshal(routeRLPKey) g.Expect(err).ToNot(HaveOccurred()) g.Expect(existingGateway.GetAnnotations()).To(HaveKey(routeRLP.BackReferenceAnnotationName())) g.Expect(existingGateway.GetAnnotations()[routeRLP.BackReferenceAnnotationName()]).To(ContainSubstring(string(serialized))) @@ -487,157 +492,122 @@ var _ = Describe("RateLimitPolicy controller", func() { }, SpecTimeout(time.Minute)) It("Gateway atomic override - gateway defaults turned into overrides later on", func(ctx SpecContext) { - // Create HTTPRoute RLP - routeRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { - policy.Name = "httproute-rlp" - policy.Spec.CommonSpec().Limits = map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 10, Duration: 5, Unit: "second", - }, - }, - }, - } - }) + // Create Route RLP Expect(k8sClient.Create(ctx, routeRLP)).To(Succeed()) - rlpKey := client.ObjectKey{Name: routeRLP.Name, Namespace: testNamespace} - Eventually(testRLPIsAccepted(rlpKey)).WithContext(ctx).Should(BeTrue()) + routeRLPKey := client.ObjectKeyFromObject(routeRLP) + Eventually(testRLPIsAccepted(routeRLPKey)).WithContext(ctx).Should(BeTrue()) + Eventually(testRLPIsEnforced(routeRLPKey)).WithContext(ctx).Should(BeTrue()) - // create GW RLP - gwRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + // Create GW RLP with defaults + gwRLP = policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { policy.Spec.TargetRef.Kind = "Gateway" policy.Spec.TargetRef.Name = gatewayapiv1.ObjectName(gwName) - policy.Spec.Defaults = &kuadrantv1beta2.RateLimitPolicyCommonSpec{ - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: "minute", - }, - }, - }, - }, - } }) Expect(k8sClient.Create(ctx, gwRLP)).To(Succeed()) - rlpKey = client.ObjectKey{Name: gwRLP.Name, Namespace: testNamespace} - Eventually(testRLPIsAccepted(rlpKey)).WithContext(ctx).Should(BeTrue()) + gwRLPKey := client.ObjectKeyFromObject(gwRLP) + Eventually(testRLPIsAccepted(gwRLPKey)).WithContext(ctx).Should(BeTrue()) + Eventually(testRLPIsEnforced(gwRLPKey)).WithContext(ctx).Should(BeFalse()) + Expect(testRLPEnforcedCondition(gwRLPKey, kuadrant.PolicyReasonOverridden, fmt.Sprintf("RateLimitPolicy is overridden by [%s]", routeRLPKey))) - // check limits - should contain HTTPRoute values - limitadorKey := client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace} - existingLimitador := &limitadorv1alpha1.Limitador{} - Expect(k8sClient.Get(ctx, limitadorKey, existingLimitador)).To(Succeed()) - Expect(existingLimitador.Spec.Limits).To(ContainElements(limitadorv1alpha1.RateLimit{ + // Route RLP should still be enforced + Eventually(testRLPIsEnforced(routeRLPKey)).WithContext(ctx).Should(BeTrue()) + + // Should contain Route RLP values + Eventually(limitadorContainsLimit(ctx, limitadorv1alpha1.RateLimit{ MaxValue: 10, Seconds: 5, Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP), Conditions: []string{`limit.l1__2804bad6 == "1"`}, Variables: []string{}, Name: rlptools.LimitsNameFromRLP(routeRLP), - })) + })).WithContext(ctx).Should(Succeed()) - updatedGRLP := &kuadrantv1beta2.RateLimitPolicy{} - Expect(k8sClient.Get(ctx, client.ObjectKey{Name: gwRLP.Name, Namespace: testNamespace}, updatedGRLP)).To(Succeed()) + // Update GW RLP defaults to overrides Eventually(func(g Gomega) { - updatedGRLP.Spec.Overrides = updatedGRLP.Spec.Defaults.DeepCopy() - updatedGRLP.Spec.Defaults = nil - g.Expect(k8sClient.Update(ctx, updatedGRLP)).To(Succeed()) + g.Expect(k8sClient.Get(ctx, gwRLPKey, gwRLP)).To(Succeed()) + gwRLP.Spec.Overrides = gwRLP.Spec.Defaults.DeepCopy() + gwRLP.Spec.Defaults = nil + g.Expect(k8sClient.Update(ctx, gwRLP)).To(Succeed()) }).WithContext(ctx).Should(Succeed()) - Eventually(func(g Gomega) { - // check limits - should contain override values - limitadorKey = client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace} - existingLimitador = &limitadorv1alpha1.Limitador{} - g.Expect(k8sClient.Get(ctx, limitadorKey, existingLimitador)).To(Succeed()) - g.Expect(existingLimitador.Spec.Limits).To(ContainElements(limitadorv1alpha1.RateLimit{ - MaxValue: 1, - Seconds: 180, - Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP), - Conditions: []string{`limit.l1__2804bad6 == "1"`}, - Variables: []string{}, - Name: rlptools.LimitsNameFromRLP(routeRLP), - })) - }) + // GW RLP should now be enforced + Eventually(testRLPIsEnforced(routeRLPKey)).WithContext(ctx).Should(BeFalse()) + Expect(testRLPEnforcedCondition(routeRLPKey, kuadrant.PolicyReasonOverridden, fmt.Sprintf("RateLimitPolicy is overridden by [%s]", gwRLPKey))) + Eventually(testRLPIsEnforced(gwRLPKey)).WithContext(ctx).Should(BeTrue()) + + // Should contain override values + Eventually(limitadorContainsLimit(ctx, limitadorv1alpha1.RateLimit{ + MaxValue: 1, + Seconds: 180, + Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP), + Conditions: []string{`limit.l1__2804bad6 == "1"`}, + Variables: []string{}, + Name: rlptools.LimitsNameFromRLP(routeRLP), + })).WithContext(ctx).Should(Succeed()) }, SpecTimeout(time.Minute)) It("Gateway atomic override - gateway overrides turned into defaults later on", func(ctx SpecContext) { // Create HTTPRoute RLP - routeRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { - policy.Name = "httproute-rlp" - policy.Spec.CommonSpec().Limits = map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 10, Duration: 5, Unit: "second", - }, - }, - }, - } - }) Expect(k8sClient.Create(ctx, routeRLP)).To(Succeed()) - rlpKey := client.ObjectKey{Name: routeRLP.Name, Namespace: testNamespace} - Eventually(testRLPIsAccepted(rlpKey)).WithContext(ctx).Should(BeTrue()) + routeRLPKey := client.ObjectKeyFromObject(routeRLP) + Eventually(testRLPIsAccepted(routeRLPKey)).WithContext(ctx).Should(BeTrue()) + Eventually(testRLPIsEnforced(routeRLPKey)).WithContext(ctx).Should(BeTrue()) - // create GW RLP - gwRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { - policy.Spec.Defaults = nil - policy.Spec.TargetRef.Kind = "Gateway" - policy.Spec.TargetRef.Name = gatewayapiv1.ObjectName(gwName) - policy.Spec.Overrides = &kuadrantv1beta2.RateLimitPolicyCommonSpec{ - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: "minute", - }, - }, - }, - }, - } - }) + // create GW RLP with overrides Expect(k8sClient.Create(ctx, gwRLP)).To(Succeed()) - rlpKey = client.ObjectKey{Name: gwRLP.Name, Namespace: testNamespace} - Eventually(testRLPIsAccepted(rlpKey)).WithContext(ctx).Should(BeTrue()) + gwRLPKey := client.ObjectKey{Name: gwRLP.Name, Namespace: testNamespace} + Eventually(testRLPIsAccepted(gwRLPKey)).WithContext(ctx).Should(BeTrue()) + Eventually(testRLPIsEnforced(gwRLPKey)).WithContext(ctx).Should(BeTrue()) - // check limits - overridden - should contain GW RLP values - Eventually(func(g Gomega) { - limitadorKey := client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace} - existingLimitador := &limitadorv1alpha1.Limitador{} - g.Expect(k8sClient.Get(ctx, limitadorKey, existingLimitador)).To(Succeed()) - g.Expect(existingLimitador.Spec.Limits).To(ContainElements(limitadorv1alpha1.RateLimit{ - MaxValue: 1, - Seconds: 180, - Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP), - Conditions: []string{`limit.l1__2804bad6 == "1"`}, - Variables: []string{}, - Name: rlptools.LimitsNameFromRLP(routeRLP), - })) - }).WithContext(ctx).Should(Succeed()) + // Route RLP should not be enforced + Eventually(testRLPIsEnforced(routeRLPKey)).WithContext(ctx).Should(BeFalse()) + Expect(testRLPEnforcedCondition(routeRLPKey, kuadrant.PolicyReasonOverridden, fmt.Sprintf("RateLimitPolicy is overridden by [%s]", gwRLPKey))) - updatedGRLP := &kuadrantv1beta2.RateLimitPolicy{} - Expect(k8sClient.Get(ctx, client.ObjectKey{Name: gwRLP.Name, Namespace: testNamespace}, updatedGRLP)).To(Succeed()) - Eventually(func(g Gomega) { - updatedGRLP.Spec.Defaults = updatedGRLP.Spec.Overrides.DeepCopy() - updatedGRLP.Spec.Overrides = nil - g.Expect(k8sClient.Update(ctx, updatedGRLP)).To(Succeed()) - }).WithContext(ctx).Should(Succeed()) + // Should contain override values + Eventually(limitadorContainsLimit(ctx, limitadorv1alpha1.RateLimit{ + MaxValue: 1, + Seconds: 180, + Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP), + Conditions: []string{`limit.l1__2804bad6 == "1"`}, + Variables: []string{}, + Name: rlptools.LimitsNameFromRLP(routeRLP), + })).WithContext(ctx).Should(Succeed()) + // Update GW RLP overrides to defaults Eventually(func(g Gomega) { - // check limits - should contain HTTPRoute RLP values - limitadorKey := client.ObjectKey{Name: common.LimitadorName, Namespace: testNamespace} - existingLimitador := &limitadorv1alpha1.Limitador{} - g.Expect(k8sClient.Get(ctx, limitadorKey, existingLimitador)).To(Succeed()) - g.Expect(existingLimitador.Spec.Limits).To(ContainElements(limitadorv1alpha1.RateLimit{ - MaxValue: 10, - Seconds: 5, - Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP), - Conditions: []string{`limit.l1__2804bad6 == "1"`}, - Variables: []string{}, - Name: rlptools.LimitsNameFromRLP(routeRLP), - })) + g.Expect(k8sClient.Get(ctx, gwRLPKey, gwRLP)).To(Succeed()) + gwRLP.Spec.Defaults = gwRLP.Spec.Overrides.DeepCopy() + gwRLP.Spec.Overrides = nil + g.Expect(k8sClient.Update(ctx, gwRLP)).To(Succeed()) }).WithContext(ctx).Should(Succeed()) + + // Route RLP now takes precedence + Eventually(testRLPIsEnforced(gwRLPKey)).WithContext(ctx).Should(BeFalse()) + Expect(testRLPEnforcedCondition(gwRLPKey, kuadrant.PolicyReasonOverridden, fmt.Sprintf("RateLimitPolicy is overridden by [%s]", routeRLPKey))) + Eventually(testRLPIsEnforced(routeRLPKey)).WithContext(ctx).Should(BeTrue()) + + // Should contain Route RLP values + Eventually(limitadorContainsLimit(ctx, limitadorv1alpha1.RateLimit{ + MaxValue: 10, + Seconds: 5, + Namespace: rlptools.LimitsNamespaceFromRLP(routeRLP), + Conditions: []string{`limit.l1__2804bad6 == "1"`}, + Variables: []string{}, + Name: rlptools.LimitsNameFromRLP(routeRLP), + })).WithContext(ctx).Should(Succeed()) }, SpecTimeout(2*time.Minute)) + + It("Gateway atomic override - no underlying routes to enforce policy", func(ctx SpecContext) { + // Delete HTTPRoute + Expect(k8sClient.Delete(ctx, &gatewayapiv1.HTTPRoute{ObjectMeta: metav1.ObjectMeta{Name: routeName, Namespace: testNamespace}})).To(Succeed()) + + // create GW RLP with overrides + Expect(k8sClient.Create(ctx, gwRLP)).To(Succeed()) + gwRLPKey := client.ObjectKey{Name: gwRLP.Name, Namespace: testNamespace} + Eventually(testRLPIsAccepted(gwRLPKey)).WithContext(ctx).Should(BeTrue()) + Eventually(testRLPIsEnforced(gwRLPKey)).WithContext(ctx).Should(BeFalse()) + Expect(testRLPEnforcedCondition(gwRLPKey, kuadrant.PolicyReasonUnknown, "RateLimitPolicy has encountered some issues: no free routes to enforce policy")) + }, SpecTimeout(time.Minute)) }) Context("RLP accepted condition reasons", func() { diff --git a/controllers/ratelimitpolicy_limits.go b/controllers/ratelimitpolicy_limits.go index f7e3ca82e..2726c6b58 100644 --- a/controllers/ratelimitpolicy_limits.go +++ b/controllers/ratelimitpolicy_limits.go @@ -134,68 +134,30 @@ func (r *RateLimitPolicyReconciler) buildRateLimitIndex(ctx context.Context, rlp return rateLimitIndex, nil } -// applyOverrides checks for any overrides set for the RateLimitPolicy. -// It iterates through the slice of policies to find overrides for the provided target HTTPRoute. -// If an override is found, it updates the limits in the RateLimitPolicySpec accordingly. func (r *RateLimitPolicyReconciler) applyOverrides(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy, t *kuadrantgatewayapi.Topology) error { logger, _ := logr.FromContext(ctx) logger = logger.WithName("applyOverrides") - affectedPolicies, numUnTargetedRoutes := r.getAffectedPolicies(rlp, t) + // Retrieve affected policies and the number of untargeted routes + affectedPolicies, numUnTargetedRoutes := r.getAffectedPoliciesInfo(rlp, t) + + // Filter out current policy from affected policies affectedPolicies = utils.Filter(affectedPolicies, func(policy kuadrantgatewayapi.Policy) bool { - // Filter out current policy from affected policies return policy.GetUID() != rlp.GetUID() }) - affectedPoliciesKeys := utils.Map(affectedPolicies, func(p kuadrantgatewayapi.Policy) client.ObjectKey { - return client.ObjectKeyFromObject(p) - }) - - filteredPolicies := utils.Filter(affectedPolicies, func(policy kuadrantgatewayapi.Policy) bool { - if policy.GetUID() == rlp.GetUID() { - return false - } - - // HTTPRoute RLPs should only care about overrides from gateways - if kuadrantgatewayapi.IsTargetRefHTTPRoute(rlp.GetTargetRef()) { - return kuadrantgatewayapi.IsTargetRefGateway(policy.GetTargetRef()) - } - // Gateway RLPs are not affected by other Gateway RLPs - return false - }) - // Is a GW policy + // Apply overrides based on the type of policy (Gateway or Route) if kuadrantgatewayapi.IsTargetRefGateway(rlp.GetTargetRef()) { - // Specifies defaults and has free routes => not enforced - if rlp.Spec.Overrides == nil && numUnTargetedRoutes == 0 { - r.OverriddenPolicyMap.SetOverriddenPolicy(rlp, affectedPoliciesKeys) - logger.V(1).Info("policy has no free routes to enforce policy") - } else { - r.OverriddenPolicyMap.RemoveOverriddenPolicy(rlp) - } + r.applyGatewayOverrides(logger, rlp, numUnTargetedRoutes, affectedPolicies) } else { - // Is a Route policy - // Sort by TargetRefKind and creation timestamp - // Gateways RLPs are listed first in the order of oldest policy - sort.Sort(kuadrantgatewayapi.PolicyByTargetRefKindAndCreationTimeStamp(filteredPolicies)) - - // Iterate in order of precedence until finding a block of overrides - for _, policy := range filteredPolicies { - p := policy.(*kuadrantv1beta2.RateLimitPolicy) - if p.Spec.Overrides != nil { - rlp.Spec.CommonSpec().Limits = p.Spec.Overrides.Limits - logger.V(1).Info("applying overrides from parent policy", "parentPolicy", client.ObjectKeyFromObject(p)) - // Overridden by another policy - r.OverriddenPolicyMap.SetOverriddenPolicy(rlp, []client.ObjectKey{client.ObjectKeyFromObject(p)}) - break - } - r.OverriddenPolicyMap.RemoveOverriddenPolicy(rlp) - } + r.applyRouteOverrides(logger, rlp, affectedPolicies) } - // Reconcile status for all policies that are affected by this RLP + // Reconcile status for affected policies for _, policy := range affectedPolicies { if policy.GetUID() != rlp.GetUID() { p := policy.(*kuadrantv1beta2.RateLimitPolicy) + // Override is set -> affected policy is overridden by rlp if kuadrantgatewayapi.IsTargetRefGateway(rlp.GetTargetRef()) && rlp.Spec.Overrides != nil { r.OverriddenPolicyMap.SetOverriddenPolicy(p, []client.ObjectKey{client.ObjectKeyFromObject(rlp)}) } @@ -209,22 +171,18 @@ func (r *RateLimitPolicyReconciler) applyOverrides(ctx context.Context, rlp *kua return nil } -func (r *RateLimitPolicyReconciler) getAffectedPolicies(rlp *kuadrantv1beta2.RateLimitPolicy, t *kuadrantgatewayapi.Topology) ([]kuadrantgatewayapi.Policy, int) { +func (r *RateLimitPolicyReconciler) getAffectedPoliciesInfo(rlp *kuadrantv1beta2.RateLimitPolicy, t *kuadrantgatewayapi.Topology) ([]kuadrantgatewayapi.Policy, int) { topologyIndexes := kuadrantgatewayapi.NewTopologyIndexes(t) - var affectedPolicies []kuadrantgatewayapi.Policy var numUnTargetedRoutes int - // For each gw, get all the policies if the current rlp is within the topology for this gateway + for _, gw := range t.Gateways() { policyList := topologyIndexes.PoliciesFromGateway(gw.Gateway) - policyKeys := utils.Map(policyList, func(p kuadrantgatewayapi.Policy) client.ObjectKey { - return client.ObjectKeyFromObject(p) - }) + numUnTargetedRoutes += len(topologyIndexes.GetUntargetedRoutes(gw.Gateway)) - numUnTargetedRoutes = numUnTargetedRoutes + len(topologyIndexes.GetUntargetedRoutes(gw.Gateway)) - - // this policy is potentially affected by other policies from this gateway - if slices.Contains(policyKeys, client.ObjectKeyFromObject(rlp)) { + if slices.Contains(utils.Map(policyList, func(p kuadrantgatewayapi.Policy) client.ObjectKey { + return client.ObjectKeyFromObject(p) + }), client.ObjectKeyFromObject(rlp)) { affectedPolicies = append(affectedPolicies, policyList...) } } @@ -232,6 +190,43 @@ func (r *RateLimitPolicyReconciler) getAffectedPolicies(rlp *kuadrantv1beta2.Rat return affectedPolicies, numUnTargetedRoutes } +// applyGatewayOverrides a Gateway RLP can be "overridden" is sense where every underlying route has their own policy +// and the Gateway RLP does not specify an overrides section +func (r *RateLimitPolicyReconciler) applyGatewayOverrides(logger logr.Logger, rlp *kuadrantv1beta2.RateLimitPolicy, numUnTargetedRoutes int, affectedPolicies []kuadrantgatewayapi.Policy) { + if rlp.Spec.Overrides == nil && numUnTargetedRoutes == 0 { + r.OverriddenPolicyMap.SetOverriddenPolicy(rlp, utils.Map(affectedPolicies, func(p kuadrantgatewayapi.Policy) client.ObjectKey { + return client.ObjectKeyFromObject(p) + })) + logger.V(1).Info("policy has no free routes to enforce default policy") + } else if rlp.Spec.Overrides != nil && len(affectedPolicies) == 0 && numUnTargetedRoutes == 0 { + r.OverriddenPolicyMap.SetOverriddenPolicy(rlp, []client.ObjectKey{}) + logger.V(1).Info("policy has no free routes to enforce override policy") + } else { + r.OverriddenPolicyMap.RemoveOverriddenPolicy(rlp) + } +} + +func (r *RateLimitPolicyReconciler) applyRouteOverrides(logger logr.Logger, rlp *kuadrantv1beta2.RateLimitPolicy, affectedPolicies []kuadrantgatewayapi.Policy) { + filteredPolicies := utils.Filter(affectedPolicies, func(policy kuadrantgatewayapi.Policy) bool { + if policy.GetUID() == rlp.GetUID() { + return false + } + return kuadrantgatewayapi.IsTargetRefGateway(policy.GetTargetRef()) + }) + + sort.Sort(kuadrantgatewayapi.PolicyByTargetRefKindAndCreationTimeStamp(filteredPolicies)) + + for _, policy := range filteredPolicies { + p := policy.(*kuadrantv1beta2.RateLimitPolicy) + if p.Spec.Overrides != nil { + rlp.Spec.CommonSpec().Limits = p.Spec.Overrides.Limits + logger.V(1).Info("applying overrides from parent policy", "parentPolicy", client.ObjectKeyFromObject(p)) + r.OverriddenPolicyMap.SetOverriddenPolicy(rlp, []client.ObjectKey{client.ObjectKeyFromObject(p)}) + } + r.OverriddenPolicyMap.RemoveOverriddenPolicy(rlp) + } +} + func (r *RateLimitPolicyReconciler) generateTopology(ctx context.Context) (*kuadrantgatewayapi.Topology, error) { logger, _ := logr.FromContext(ctx) diff --git a/controllers/ratelimitpolicy_status.go b/controllers/ratelimitpolicy_status.go index 5530c89db..55c97112b 100644 --- a/controllers/ratelimitpolicy_status.go +++ b/controllers/ratelimitpolicy_status.go @@ -90,6 +90,9 @@ func (r *RateLimitPolicyReconciler) enforcedCondition(ctx context.Context, polic } if r.OverriddenPolicyMap.IsPolicyOverridden(policy) { + if len(r.OverriddenPolicyMap.PolicyOverriddenBy(policy)) == 0 { + return kuadrant.EnforcedCondition(policy, kuadrant.NewErrUnknown(policy.Kind(), errors.New("no free routes to enforce policy")), false) // Maybe this should be a standard condition rather than an unknown condition + } return kuadrant.EnforcedCondition(policy, kuadrant.NewErrOverridden(policy.Kind(), r.OverriddenPolicyMap.PolicyOverriddenBy(policy)), false) } diff --git a/pkg/library/gatewayapi/types.go b/pkg/library/gatewayapi/types.go index cb222aa6e..751f37bfd 100644 --- a/pkg/library/gatewayapi/types.go +++ b/pkg/library/gatewayapi/types.go @@ -36,6 +36,11 @@ func (a PolicyByTargetRefKindAndCreationTimeStamp) Less(i, j int) bool { // Compare kind first if targetRef1.Kind != targetRef2.Kind { + if targetRef1.Kind == "Gateway" { + return true + } else if targetRef2.Kind == "HTTPRoute" { + return false + } return targetRef1.Kind < targetRef2.Kind } diff --git a/pkg/library/gatewayapi/types_test.go b/pkg/library/gatewayapi/types_test.go index 1233345db..5c500993f 100644 --- a/pkg/library/gatewayapi/types_test.go +++ b/pkg/library/gatewayapi/types_test.go @@ -128,11 +128,13 @@ func TestPolicyByTargetRef(t *testing.T) { createTestPolicy("ccc", time.Date(2020, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("Gateway")), createTestPolicy("bbb", time.Date(2000, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("HTTPRoute")), createTestPolicy("aaa", time.Date(2010, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("Gateway")), + createTestPolicy("ddd", time.Date(2005, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("Service")), }, sortedPolicies: []Policy{ createTestPolicy("aaa", time.Date(2010, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("Gateway")), createTestPolicy("ccc", time.Date(2020, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("Gateway")), createTestPolicy("bbb", time.Date(2000, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("HTTPRoute")), + createTestPolicy("ddd", time.Date(2005, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("Service")), }, }, { @@ -141,11 +143,13 @@ func TestPolicyByTargetRef(t *testing.T) { createTestPolicy("ccc", time.Date(2020, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("Gateway")), createTestPolicy("bbb", time.Date(2020, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("HTTPRoute")), createTestPolicy("aaa", time.Date(2020, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("Gateway")), + createTestPolicy("ddd", time.Date(2020, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("Service")), }, sortedPolicies: []Policy{ createTestPolicy("aaa", time.Date(2020, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("Gateway")), createTestPolicy("ccc", time.Date(2020, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("Gateway")), createTestPolicy("bbb", time.Date(2020, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("HTTPRoute")), + createTestPolicy("ddd", time.Date(2020, time.November, 10, 23, 0, 0, 0, time.UTC), withTargetRefKind("Service")), }, }, } diff --git a/pkg/library/kuadrant/httproute_wrapper.go b/pkg/library/kuadrant/httproute_wrapper.go index a839a5bc6..c785339d7 100644 --- a/pkg/library/kuadrant/httproute_wrapper.go +++ b/pkg/library/kuadrant/httproute_wrapper.go @@ -1,27 +1,33 @@ package kuadrant import ( - "github.com/kuadrant/kuadrant-operator/pkg/common" kuadrantgatewayapi "github.com/kuadrant/kuadrant-operator/pkg/library/gatewayapi" + "github.com/kuadrant/kuadrant-operator/pkg/library/utils" + + "sigs.k8s.io/controller-runtime/pkg/client" gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" ) type HTTPRouteWrapper struct { *gatewayapiv1.HTTPRoute - Referrer + kuadrantgatewayapi.Policy } -func (r HTTPRouteWrapper) PolicyRefs(t *kuadrantgatewayapi.Topology) []string { +func (r HTTPRouteWrapper) PolicyRefs(t *kuadrantgatewayapi.Topology) []client.ObjectKey { if r.HTTPRoute == nil { - return make([]string, 0) + return make([]client.ObjectKey, 0) } - refs := make([]string, 0) + refs := make([]client.ObjectKey, 0) for _, gw := range t.Gateways() { - authPolicyRefs, ok := gw.GetAnnotations()[common.AuthPolicyBackRefAnnotation] - if !ok { - continue - } - refs = append(refs, authPolicyRefs) + affectedPolicies := utils.Filter(gw.AttachedPolicies(), func(policy kuadrantgatewayapi.Policy) bool { + return kuadrantgatewayapi.IsTargetRefGateway(policy.GetTargetRef()) && r.Policy.GetUID() != policy.GetUID() + }) + + policyKeys := utils.Map(affectedPolicies, func(policy kuadrantgatewayapi.Policy) client.ObjectKey { + return client.ObjectKeyFromObject(policy) + }) + + refs = append(refs, policyKeys...) } return refs } From bf18ae17886d179b80973ca3faef7f7f22fe4f04 Mon Sep 17 00:00:00 2001 From: KevFan Date: Fri, 19 Apr 2024 15:27:52 +0100 Subject: [PATCH 10/14] refactor: overridden to affected policy map --- controllers/authpolicy_authconfig.go | 65 ++++++++++--------- controllers/authpolicy_controller.go | 8 +-- controllers/authpolicy_status.go | 58 ++++------------- controllers/ratelimitpolicy_controller.go | 4 +- controllers/ratelimitpolicy_limits.go | 42 ++++++------ controllers/ratelimitpolicy_status.go | 6 +- controllers/suite_test.go | 4 +- main.go | 4 +- .../apimachinery_status_conditions.go | 28 ++++---- pkg/library/kuadrant/httproute_wrapper.go | 47 -------------- 10 files changed, 97 insertions(+), 169 deletions(-) delete mode 100644 pkg/library/kuadrant/httproute_wrapper.go diff --git a/controllers/authpolicy_authconfig.go b/controllers/authpolicy_authconfig.go index 576bf4740..611d36718 100644 --- a/controllers/authpolicy_authconfig.go +++ b/controllers/authpolicy_authconfig.go @@ -7,8 +7,6 @@ import ( "reflect" "strings" - "k8s.io/utils/ptr" - "github.com/go-logr/logr" authorinoapi "github.com/kuadrant/authorino/api/v1beta2" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -18,6 +16,7 @@ import ( api "github.com/kuadrant/kuadrant-operator/api/v1beta2" "github.com/kuadrant/kuadrant-operator/pkg/common" + kuadrantgatewayapi "github.com/kuadrant/kuadrant-operator/pkg/library/gatewayapi" "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" "github.com/kuadrant/kuadrant-operator/pkg/library/utils" ) @@ -67,14 +66,17 @@ func (r *AuthPolicyReconciler) desiredAuthConfig(ctx context.Context, ap *api.Au switch obj := targetNetworkObject.(type) { case *gatewayapiv1.HTTPRoute: - ok, err := routeGatewayHasAuthOverrides(ctx, obj, r.Client()) + t, err := r.generateTopology(ctx) if err != nil { + logger.V(1).Info("Failed to generate topology", "error", err) return nil, err } - if ok { + + refs := routeAffectedPolicies(t, ap) + if len(refs) != 0 { logger.V(1).Info("targeted gateway has authpolicy with atomic overrides, skipping authorino authconfig for the HTTPRoute authpolicy") utils.TagObjectToDelete(authConfig) - r.OverriddenPolicyMap.SetOverriddenPolicy(ap, []client.ObjectKey{}) + r.AffectedPolicyMap.SetAffectedPolicy(ap, refs) return authConfig, nil } route = obj @@ -105,7 +107,14 @@ func (r *AuthPolicyReconciler) desiredAuthConfig(ctx context.Context, ap *api.Au if len(rules) == 0 { logger.V(1).Info("no httproutes attached to the targeted gateway, skipping authorino authconfig for the gateway authpolicy") utils.TagObjectToDelete(authConfig) - r.OverriddenPolicyMap.SetOverriddenPolicy(ap, []client.ObjectKey{}) + obj := targetNetworkObject.(*gatewayapiv1.Gateway) + gatewayWrapper := kuadrant.GatewayWrapper{Gateway: obj, Referrer: ap} + refs := gatewayWrapper.PolicyRefs() + filteredRef := utils.Filter(refs, func(key client.ObjectKey) bool { + return key != client.ObjectKeyFromObject(ap) + }) + + r.AffectedPolicyMap.SetAffectedPolicy(ap, filteredRef) return authConfig, nil } route = &gatewayapiv1.HTTPRoute{ @@ -116,8 +125,8 @@ func (r *AuthPolicyReconciler) desiredAuthConfig(ctx context.Context, ap *api.Au } } - // AuthPolicy is not overridden if we still need to create an AuthConfig for it - r.OverriddenPolicyMap.RemoveOverriddenPolicy(ap) + // AuthPolicy is not Affected if we still need to create an AuthConfig for it + r.AffectedPolicyMap.RemoveAffectedPolicy(ap) // hosts authConfig.Spec.Hosts = hosts @@ -185,32 +194,26 @@ func (r *AuthPolicyReconciler) desiredAuthConfig(ctx context.Context, ap *api.Au return mergeConditionsFromRouteSelectorsIntoConfigs(ap, route, authConfig) } -// routeGatewayHasAuthOverrides return true when the gateway which a route is attached to has an attached authPolicy that defines atomic overrides -func routeGatewayHasAuthOverrides(ctx context.Context, route *gatewayapiv1.HTTPRoute, c client.Client) (bool, error) { - for idx := range route.Spec.ParentRefs { - parentRef := route.Spec.ParentRefs[idx] - gw := &gatewayapiv1.Gateway{} - namespace := ptr.Deref(parentRef.Namespace, gatewayapiv1.Namespace(route.GetNamespace())) - err := c.Get(ctx, client.ObjectKey{Name: string(parentRef.Name), Namespace: string(namespace)}, gw) - if err != nil { - return false, err - } +func routeAffectedPolicies(t *kuadrantgatewayapi.Topology, ap *api.AuthPolicy) []client.ObjectKey { + refs := make([]client.ObjectKey, 0) + for _, gw := range t.Gateways() { + affectedPolicies := utils.Filter(gw.AttachedPolicies(), func(policy kuadrantgatewayapi.Policy) bool { + p, ok := policy.(*api.AuthPolicy) + if !ok { + return false + } + return kuadrantgatewayapi.IsTargetRefGateway(policy.GetTargetRef()) && + ap.GetUID() != policy.GetUID() && p.IsAtomicOverride() + }) - annotation, ok := gw.GetAnnotations()[common.AuthPolicyBackRefAnnotation] - if !ok { - continue - } - otherAP := &api.AuthPolicy{} - err = c.Get(ctx, utils.NamespacedNameToObjectKey(annotation, gw.Namespace), otherAP) - if err != nil { - return false, err - } + policyKeys := utils.Map(affectedPolicies, func(policy kuadrantgatewayapi.Policy) client.ObjectKey { + return client.ObjectKeyFromObject(policy) + }) - if otherAP.IsAtomicOverride() { - return true, nil - } + refs = append(refs, policyKeys...) } - return false, nil + + return refs } // authConfigName returns the name of Authorino AuthConfig CR. diff --git a/controllers/authpolicy_controller.go b/controllers/authpolicy_controller.go index 57856b074..8fb053676 100644 --- a/controllers/authpolicy_controller.go +++ b/controllers/authpolicy_controller.go @@ -26,8 +26,8 @@ const authPolicyFinalizer = "authpolicy.kuadrant.io/finalizer" type AuthPolicyReconciler struct { *reconcilers.BaseReconciler TargetRefReconciler reconcilers.TargetRefReconciler - // OverriddenPolicyMap tracks the overridden policies to report their status. - OverriddenPolicyMap *kuadrant.OverriddenPolicyMap + // AffectedPolicyMap tracks the affected policies to report their status. + AffectedPolicyMap *kuadrant.AffectedPolicyMap } //+kubebuilder:rbac:groups=kuadrant.io,resources=authpolicies,verbs=get;list;watch;create;update;patch;delete @@ -72,7 +72,7 @@ func (r *AuthPolicyReconciler) Reconcile(eventCtx context.Context, req ctrl.Requ if delResErr == nil { delResErr = err } - return r.reconcileStatus(ctx, ap, targetNetworkObject, kuadrant.NewErrTargetNotFound(ap.Kind(), ap.GetTargetRef(), delResErr)) + return r.reconcileStatus(ctx, ap, kuadrant.NewErrTargetNotFound(ap.Kind(), ap.GetTargetRef(), delResErr)) } return ctrl.Result{}, err } @@ -108,7 +108,7 @@ func (r *AuthPolicyReconciler) Reconcile(eventCtx context.Context, req ctrl.Requ specErr := r.reconcileResources(ctx, ap, targetNetworkObject) // reconcile authpolicy status - statusResult, statusErr := r.reconcileStatus(ctx, ap, targetNetworkObject, specErr) + statusResult, statusErr := r.reconcileStatus(ctx, ap, specErr) if specErr != nil { return ctrl.Result{}, specErr diff --git a/controllers/authpolicy_status.go b/controllers/authpolicy_status.go index d6d0b13a2..b2e9409be 100644 --- a/controllers/authpolicy_status.go +++ b/controllers/authpolicy_status.go @@ -24,11 +24,11 @@ import ( ) // reconcileStatus makes sure status block of AuthPolicy is up-to-date. -func (r *AuthPolicyReconciler) reconcileStatus(ctx context.Context, ap *api.AuthPolicy, targetNetworkObject client.Object, specErr error) (ctrl.Result, error) { +func (r *AuthPolicyReconciler) reconcileStatus(ctx context.Context, ap *api.AuthPolicy, specErr error) (ctrl.Result, error) { logger, _ := logr.FromContext(ctx) logger.V(1).Info("Reconciling AuthPolicy status", "spec error", specErr) - newStatus := r.calculateStatus(ctx, ap, targetNetworkObject, specErr) + newStatus := r.calculateStatus(ctx, ap, specErr) equalStatus := ap.Status.Equals(newStatus, logger) logger.V(1).Info("Status", "status is different", !equalStatus) @@ -60,7 +60,7 @@ func (r *AuthPolicyReconciler) reconcileStatus(ctx context.Context, ap *api.Auth return ctrl.Result{}, nil } -func (r *AuthPolicyReconciler) calculateStatus(ctx context.Context, ap *api.AuthPolicy, targetNetworkObject client.Object, specErr error) *api.AuthPolicyStatus { +func (r *AuthPolicyReconciler) calculateStatus(ctx context.Context, ap *api.AuthPolicy, specErr error) *api.AuthPolicyStatus { newStatus := &api.AuthPolicyStatus{ Conditions: slices.Clone(ap.Status.Conditions), ObservedGeneration: ap.Status.ObservedGeneration, @@ -74,7 +74,7 @@ func (r *AuthPolicyReconciler) calculateStatus(ctx context.Context, ap *api.Auth return newStatus } - enforcedCond := r.enforcedCondition(ctx, ap, targetNetworkObject) + enforcedCond := r.enforcedCondition(ctx, ap) meta.SetStatusCondition(&newStatus.Conditions, *enforcedCond) return newStatus @@ -86,21 +86,16 @@ func (r *AuthPolicyReconciler) acceptedCondition(policy kuadrant.Policy, specErr // enforcedCondition checks if the provided AuthPolicy is enforced, ensuring it is properly configured and applied based // on the status of the associated AuthConfig and Gateway. -func (r *AuthPolicyReconciler) enforcedCondition(ctx context.Context, policy *api.AuthPolicy, targetNetworkObject client.Object) *metav1.Condition { +func (r *AuthPolicyReconciler) enforcedCondition(ctx context.Context, policy *api.AuthPolicy) *metav1.Condition { logger, _ := logr.FromContext(ctx) - t, err := r.generateTopology(ctx) - if err != nil { - logger.V(1).Info("Failed to generate topology", "error", err) - return nil - } - // Check if the policy is overridden + // Check if the policy is Affected // Note: This logic assumes synchronous processing, where computing the desired AuthConfig, marking the AuthPolicy - // as overridden, and calculating the Enforced condition happen sequentially. + // as Affected, and calculating the Enforced condition happen sequentially. // Introducing a goroutine in this flow could break this assumption and lead to unexpected behavior. - if r.OverriddenPolicyMap.IsPolicyOverridden(policy) { + if r.AffectedPolicyMap.IsPolicyAffected(policy) { logger.V(1).Info("Gateway Policy is overridden") - return r.handlePolicyOverride(logger, policy, targetNetworkObject, t) + return r.handlePolicyOverride(policy) } // Check if the AuthConfig is ready @@ -136,39 +131,12 @@ func (r *AuthPolicyReconciler) isAuthConfigReady(ctx context.Context, policy *ap return authConfig.Status.Ready(), nil } -func (r *AuthPolicyReconciler) handlePolicyOverride(logger logr.Logger, policy *api.AuthPolicy, targetNetworkObject client.Object, t *kuadrantgatewayapi.Topology) *metav1.Condition { - switch targetNetworkObject.(type) { - case *gatewayapiv1.Gateway: - return r.handleGatewayPolicyOverride(policy, targetNetworkObject) - case *gatewayapiv1.HTTPRoute: - return r.handleHTTPRoutePolicyOverride(policy, targetNetworkObject, t) - default: - logger.Error(errors.New("this point should never be reached"), "failed to match target network object", targetNetworkObject) - return nil +func (r *AuthPolicyReconciler) handlePolicyOverride(policy *api.AuthPolicy) *metav1.Condition { + if !r.AffectedPolicyMap.IsPolicyOverridden(policy) { + return kuadrant.EnforcedCondition(policy, kuadrant.NewErrUnknown(policy.Kind(), errors.New("no free routes to enforce policy")), false) // Maybe this should be a standard condition rather than an unknown condition } -} - -// handleGatewayPolicyOverride handles the case where the Gateway Policy is overridden by filtering policy references -// and creating a corresponding error condition. -func (r *AuthPolicyReconciler) handleGatewayPolicyOverride(policy *api.AuthPolicy, targetNetworkObject client.Object) *metav1.Condition { - obj := targetNetworkObject.(*gatewayapiv1.Gateway) - gatewayWrapper := kuadrant.GatewayWrapper{Gateway: obj, Referrer: policy} - refs := gatewayWrapper.PolicyRefs() - filteredRef := utils.Filter(refs, func(key client.ObjectKey) bool { - return key != client.ObjectKeyFromObject(policy) - }) - - return kuadrant.EnforcedCondition(policy, kuadrant.NewErrOverridden(policy.Kind(), filteredRef), false) -} - -// handleHTTPRoutePolicyOverride handles the case where the HTTPRoute Policy is overridden by filtering policy references -// and creating a corresponding error condition. -func (r *AuthPolicyReconciler) handleHTTPRoutePolicyOverride(policy *api.AuthPolicy, targetNetworkObject client.Object, t *kuadrantgatewayapi.Topology) *metav1.Condition { - obj := targetNetworkObject.(*gatewayapiv1.HTTPRoute) - httpRouteWrapper := kuadrant.HTTPRouteWrapper{HTTPRoute: obj, Policy: policy} - refs := httpRouteWrapper.PolicyRefs(t) - return kuadrant.EnforcedCondition(policy, kuadrant.NewErrOverridden(policy.Kind(), refs), false) + return kuadrant.EnforcedCondition(policy, kuadrant.NewErrOverridden(policy.Kind(), r.AffectedPolicyMap.PolicyAffectedBy(policy)), false) } func (r *AuthPolicyReconciler) generateTopology(ctx context.Context) (*kuadrantgatewayapi.Topology, error) { diff --git a/controllers/ratelimitpolicy_controller.go b/controllers/ratelimitpolicy_controller.go index 92cb572b1..b5a973c3c 100644 --- a/controllers/ratelimitpolicy_controller.go +++ b/controllers/ratelimitpolicy_controller.go @@ -42,8 +42,8 @@ const rateLimitPolicyFinalizer = "ratelimitpolicy.kuadrant.io/finalizer" type RateLimitPolicyReconciler struct { *reconcilers.BaseReconciler TargetRefReconciler reconcilers.TargetRefReconciler - // OverriddenPolicyMap tracks the overridden policies to report their status. - OverriddenPolicyMap *kuadrant.OverriddenPolicyMap + // AffectedPolicyMap tracks the affected policies to report their status. + AffectedPolicyMap *kuadrant.AffectedPolicyMap } //+kubebuilder:rbac:groups=kuadrant.io,resources=ratelimitpolicies,verbs=get;list;watch;create;update;patch;delete diff --git a/controllers/ratelimitpolicy_limits.go b/controllers/ratelimitpolicy_limits.go index 2726c6b58..e248ee10f 100644 --- a/controllers/ratelimitpolicy_limits.go +++ b/controllers/ratelimitpolicy_limits.go @@ -150,21 +150,19 @@ func (r *RateLimitPolicyReconciler) applyOverrides(ctx context.Context, rlp *kua if kuadrantgatewayapi.IsTargetRefGateway(rlp.GetTargetRef()) { r.applyGatewayOverrides(logger, rlp, numUnTargetedRoutes, affectedPolicies) } else { - r.applyRouteOverrides(logger, rlp, affectedPolicies) + affectedPolicies = r.applyRouteOverrides(logger, rlp, affectedPolicies) } // Reconcile status for affected policies for _, policy := range affectedPolicies { - if policy.GetUID() != rlp.GetUID() { - p := policy.(*kuadrantv1beta2.RateLimitPolicy) - // Override is set -> affected policy is overridden by rlp - if kuadrantgatewayapi.IsTargetRefGateway(rlp.GetTargetRef()) && rlp.Spec.Overrides != nil { - r.OverriddenPolicyMap.SetOverriddenPolicy(p, []client.ObjectKey{client.ObjectKeyFromObject(rlp)}) - } - _, err := r.reconcileStatus(ctx, p, nil) - if err != nil { - return err - } + p := policy.(*kuadrantv1beta2.RateLimitPolicy) + // Override is set -> affected policy is Affected by rlp + if kuadrantgatewayapi.IsTargetRefGateway(rlp.GetTargetRef()) && rlp.Spec.Overrides != nil { + r.AffectedPolicyMap.SetAffectedPolicy(p, []client.ObjectKey{client.ObjectKeyFromObject(rlp)}) + } + _, err := r.reconcileStatus(ctx, p, nil) + if err != nil { + return err } } @@ -190,27 +188,24 @@ func (r *RateLimitPolicyReconciler) getAffectedPoliciesInfo(rlp *kuadrantv1beta2 return affectedPolicies, numUnTargetedRoutes } -// applyGatewayOverrides a Gateway RLP can be "overridden" is sense where every underlying route has their own policy -// and the Gateway RLP does not specify an overrides section +// applyGatewayOverrides a Gateway RLP is not affected if there is untargetted routes or affects other policies +// Otherwise, it func (r *RateLimitPolicyReconciler) applyGatewayOverrides(logger logr.Logger, rlp *kuadrantv1beta2.RateLimitPolicy, numUnTargetedRoutes int, affectedPolicies []kuadrantgatewayapi.Policy) { if rlp.Spec.Overrides == nil && numUnTargetedRoutes == 0 { - r.OverriddenPolicyMap.SetOverriddenPolicy(rlp, utils.Map(affectedPolicies, func(p kuadrantgatewayapi.Policy) client.ObjectKey { + r.AffectedPolicyMap.SetAffectedPolicy(rlp, utils.Map(affectedPolicies, func(p kuadrantgatewayapi.Policy) client.ObjectKey { return client.ObjectKeyFromObject(p) })) logger.V(1).Info("policy has no free routes to enforce default policy") } else if rlp.Spec.Overrides != nil && len(affectedPolicies) == 0 && numUnTargetedRoutes == 0 { - r.OverriddenPolicyMap.SetOverriddenPolicy(rlp, []client.ObjectKey{}) + r.AffectedPolicyMap.SetAffectedPolicy(rlp, []client.ObjectKey{}) logger.V(1).Info("policy has no free routes to enforce override policy") } else { - r.OverriddenPolicyMap.RemoveOverriddenPolicy(rlp) + r.AffectedPolicyMap.RemoveAffectedPolicy(rlp) } } -func (r *RateLimitPolicyReconciler) applyRouteOverrides(logger logr.Logger, rlp *kuadrantv1beta2.RateLimitPolicy, affectedPolicies []kuadrantgatewayapi.Policy) { +func (r *RateLimitPolicyReconciler) applyRouteOverrides(logger logr.Logger, rlp *kuadrantv1beta2.RateLimitPolicy, affectedPolicies []kuadrantgatewayapi.Policy) []kuadrantgatewayapi.Policy { filteredPolicies := utils.Filter(affectedPolicies, func(policy kuadrantgatewayapi.Policy) bool { - if policy.GetUID() == rlp.GetUID() { - return false - } return kuadrantgatewayapi.IsTargetRefGateway(policy.GetTargetRef()) }) @@ -221,10 +216,13 @@ func (r *RateLimitPolicyReconciler) applyRouteOverrides(logger logr.Logger, rlp if p.Spec.Overrides != nil { rlp.Spec.CommonSpec().Limits = p.Spec.Overrides.Limits logger.V(1).Info("applying overrides from parent policy", "parentPolicy", client.ObjectKeyFromObject(p)) - r.OverriddenPolicyMap.SetOverriddenPolicy(rlp, []client.ObjectKey{client.ObjectKeyFromObject(p)}) + r.AffectedPolicyMap.SetAffectedPolicy(rlp, []client.ObjectKey{client.ObjectKeyFromObject(p)}) + break } - r.OverriddenPolicyMap.RemoveOverriddenPolicy(rlp) + r.AffectedPolicyMap.RemoveAffectedPolicy(rlp) } + + return filteredPolicies } func (r *RateLimitPolicyReconciler) generateTopology(ctx context.Context) (*kuadrantgatewayapi.Topology, error) { diff --git a/controllers/ratelimitpolicy_status.go b/controllers/ratelimitpolicy_status.go index 55c97112b..08abb28dc 100644 --- a/controllers/ratelimitpolicy_status.go +++ b/controllers/ratelimitpolicy_status.go @@ -89,11 +89,11 @@ func (r *RateLimitPolicyReconciler) enforcedCondition(ctx context.Context, polic return kuadrant.EnforcedCondition(policy, kuadrant.NewErrUnknown(policy.Kind(), errors.New("limitador is not ready")), false) } - if r.OverriddenPolicyMap.IsPolicyOverridden(policy) { - if len(r.OverriddenPolicyMap.PolicyOverriddenBy(policy)) == 0 { + if r.AffectedPolicyMap.IsPolicyAffected(policy) { + if !r.AffectedPolicyMap.IsPolicyOverridden(policy) { return kuadrant.EnforcedCondition(policy, kuadrant.NewErrUnknown(policy.Kind(), errors.New("no free routes to enforce policy")), false) // Maybe this should be a standard condition rather than an unknown condition } - return kuadrant.EnforcedCondition(policy, kuadrant.NewErrOverridden(policy.Kind(), r.OverriddenPolicyMap.PolicyOverriddenBy(policy)), false) + return kuadrant.EnforcedCondition(policy, kuadrant.NewErrOverridden(policy.Kind(), r.AffectedPolicyMap.PolicyAffectedBy(policy)), false) } logger.V(1).Info("RateLimitPolicy is enforced") diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 3279acc3c..6a3033b2a 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -147,7 +147,7 @@ var _ = BeforeSuite(func() { err = (&AuthPolicyReconciler{ BaseReconciler: authPolicyBaseReconciler, TargetRefReconciler: reconcilers.TargetRefReconciler{Client: mgr.GetClient()}, - OverriddenPolicyMap: kuadrant.NewOverriddenPolicyMap(), + AffectedPolicyMap: kuadrant.NewAffectedPolicyMap(), }).SetupWithManager(mgr) Expect(err).NotTo(HaveOccurred()) @@ -160,7 +160,7 @@ var _ = BeforeSuite(func() { err = (&RateLimitPolicyReconciler{ BaseReconciler: rateLimitPolicyBaseReconciler, TargetRefReconciler: reconcilers.TargetRefReconciler{Client: mgr.GetClient()}, - OverriddenPolicyMap: kuadrant.NewOverriddenPolicyMap(), + AffectedPolicyMap: kuadrant.NewAffectedPolicyMap(), }).SetupWithManager(mgr) Expect(err).NotTo(HaveOccurred()) diff --git a/main.go b/main.go index 0eecc7dac..9cdba3237 100644 --- a/main.go +++ b/main.go @@ -154,7 +154,7 @@ func main() { if err = (&controllers.RateLimitPolicyReconciler{ TargetRefReconciler: reconcilers.TargetRefReconciler{Client: mgr.GetClient()}, BaseReconciler: rateLimitPolicyBaseReconciler, - OverriddenPolicyMap: kuadrant.NewOverriddenPolicyMap(), + AffectedPolicyMap: kuadrant.NewAffectedPolicyMap(), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "RateLimitPolicy") os.Exit(1) @@ -169,7 +169,7 @@ func main() { if err = (&controllers.AuthPolicyReconciler{ TargetRefReconciler: reconcilers.TargetRefReconciler{Client: mgr.GetClient()}, BaseReconciler: authPolicyBaseReconciler, - OverriddenPolicyMap: kuadrant.NewOverriddenPolicyMap(), + AffectedPolicyMap: kuadrant.NewAffectedPolicyMap(), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "AuthPolicy") os.Exit(1) diff --git a/pkg/library/kuadrant/apimachinery_status_conditions.go b/pkg/library/kuadrant/apimachinery_status_conditions.go index 99ae86384..fb0822ac5 100644 --- a/pkg/library/kuadrant/apimachinery_status_conditions.go +++ b/pkg/library/kuadrant/apimachinery_status_conditions.go @@ -22,19 +22,19 @@ const ( PolicyReasonUnknown gatewayapiv1alpha2.PolicyConditionReason = "Unknown" ) -func NewOverriddenPolicyMap() *OverriddenPolicyMap { - return &OverriddenPolicyMap{ +func NewAffectedPolicyMap() *AffectedPolicyMap { + return &AffectedPolicyMap{ policies: make(map[types.UID][]client.ObjectKey), } } -type OverriddenPolicyMap struct { +type AffectedPolicyMap struct { policies map[types.UID][]client.ObjectKey mu sync.RWMutex } -// SetOverriddenPolicy sets the provided Policy as overridden in the tracking map. -func (o *OverriddenPolicyMap) SetOverriddenPolicy(p Policy, affectedBy []client.ObjectKey) { +// SetAffectedPolicy sets the provided Policy as Affected in the tracking map. +func (o *AffectedPolicyMap) SetAffectedPolicy(p Policy, affectedBy []client.ObjectKey) { o.mu.Lock() defer o.mu.Unlock() @@ -44,21 +44,27 @@ func (o *OverriddenPolicyMap) SetOverriddenPolicy(p Policy, affectedBy []client. o.policies[p.GetUID()] = affectedBy } -// RemoveOverriddenPolicy removes the provided Policy from the tracking map of overridden policies. -func (o *OverriddenPolicyMap) RemoveOverriddenPolicy(p Policy) { +// RemoveAffectedPolicy removes the provided Policy from the tracking map of Affected policies. +func (o *AffectedPolicyMap) RemoveAffectedPolicy(p Policy) { o.mu.Lock() defer o.mu.Unlock() delete(o.policies, p.GetUID()) } -// IsPolicyOverridden checks if the provided Policy is overridden based on the tracking map maintained. -func (o *OverriddenPolicyMap) IsPolicyOverridden(p Policy) bool { +// IsPolicyAffected checks if the provided Policy is affected based on the tracking map maintained. +func (o *AffectedPolicyMap) IsPolicyAffected(p Policy) bool { return o.policies[p.GetUID()] != nil } -// PolicyOverriddenBy returns the clients keys that a policy is overridden by -func (o *OverriddenPolicyMap) PolicyOverriddenBy(p Policy) []client.ObjectKey { +// IsPolicyOverridden checks if the provided Policy is affected based on the tracking map maintained. +// It is overridden if there is policies affecting it +func (o *AffectedPolicyMap) IsPolicyOverridden(p Policy) bool { + return o.IsPolicyAffected(p) && len(o.policies[p.GetUID()]) > 0 +} + +// PolicyAffectedBy returns the clients keys that a policy is Affected by +func (o *AffectedPolicyMap) PolicyAffectedBy(p Policy) []client.ObjectKey { return o.policies[p.GetUID()] } diff --git a/pkg/library/kuadrant/httproute_wrapper.go b/pkg/library/kuadrant/httproute_wrapper.go deleted file mode 100644 index c785339d7..000000000 --- a/pkg/library/kuadrant/httproute_wrapper.go +++ /dev/null @@ -1,47 +0,0 @@ -package kuadrant - -import ( - kuadrantgatewayapi "github.com/kuadrant/kuadrant-operator/pkg/library/gatewayapi" - "github.com/kuadrant/kuadrant-operator/pkg/library/utils" - - "sigs.k8s.io/controller-runtime/pkg/client" - gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" -) - -type HTTPRouteWrapper struct { - *gatewayapiv1.HTTPRoute - kuadrantgatewayapi.Policy -} - -func (r HTTPRouteWrapper) PolicyRefs(t *kuadrantgatewayapi.Topology) []client.ObjectKey { - if r.HTTPRoute == nil { - return make([]client.ObjectKey, 0) - } - refs := make([]client.ObjectKey, 0) - for _, gw := range t.Gateways() { - affectedPolicies := utils.Filter(gw.AttachedPolicies(), func(policy kuadrantgatewayapi.Policy) bool { - return kuadrantgatewayapi.IsTargetRefGateway(policy.GetTargetRef()) && r.Policy.GetUID() != policy.GetUID() - }) - - policyKeys := utils.Map(affectedPolicies, func(policy kuadrantgatewayapi.Policy) client.ObjectKey { - return client.ObjectKeyFromObject(policy) - }) - - refs = append(refs, policyKeys...) - } - return refs -} - -// HTTPRouteWrapperList is a list of HTTPRouteWrapper that implements sort.interface -// impl: sort.interface -type HTTPRouteWrapperList []HTTPRouteWrapper - -func (l HTTPRouteWrapperList) Len() int { return len(l) } - -func (l HTTPRouteWrapperList) Less(i, j int) bool { - return l[i].CreationTimestamp.Before(&l[j].CreationTimestamp) -} - -func (l HTTPRouteWrapperList) Swap(i, j int) { - l[i], l[j] = l[j], l[i] -} From 707e779da9b7cdaccee9cc2b03da1ac37fa13790 Mon Sep 17 00:00:00 2001 From: KevFan Date: Mon, 22 Apr 2024 10:50:39 +0100 Subject: [PATCH 11/14] tests: add testing enforced condition to other integration tests --- ...dor_cluster_envoyfilter_controller_test.go | 1 + ...ate_limiting_wasmplugin_controller_test.go | 19 +++++++++++++++++++ .../ratelimitpolicy_controller_test.go | 19 +++++++++++++++++++ 3 files changed, 39 insertions(+) diff --git a/controllers/limitador_cluster_envoyfilter_controller_test.go b/controllers/limitador_cluster_envoyfilter_controller_test.go index 29ce4e12f..eee1e4e66 100644 --- a/controllers/limitador_cluster_envoyfilter_controller_test.go +++ b/controllers/limitador_cluster_envoyfilter_controller_test.go @@ -108,6 +108,7 @@ var _ = Describe("Limitador Cluster EnvoyFilter controller", func() { // Check RLP status is available rlpKey := client.ObjectKey{Name: rlpName, Namespace: testNamespace} Eventually(testRLPIsAccepted(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) // Check envoy filter Eventually(func() bool { diff --git a/controllers/rate_limiting_wasmplugin_controller_test.go b/controllers/rate_limiting_wasmplugin_controller_test.go index 3b6a83fef..07e873160 100644 --- a/controllers/rate_limiting_wasmplugin_controller_test.go +++ b/controllers/rate_limiting_wasmplugin_controller_test.go @@ -21,6 +21,7 @@ import ( kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" "github.com/kuadrant/kuadrant-operator/pkg/common" + "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" "github.com/kuadrant/kuadrant-operator/pkg/rlptools" "github.com/kuadrant/kuadrant-operator/pkg/rlptools/wasm" ) @@ -93,6 +94,7 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { // Check RLP status is available rlpKey := client.ObjectKeyFromObject(rlp) Eventually(testRLPIsAccepted(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) // Check wasm plugin wasmPluginKey := client.ObjectKey{Name: rlptools.WASMPluginName(gateway), Namespace: testNamespace} @@ -253,6 +255,7 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { // Check RLP status is available rlpKey := client.ObjectKeyFromObject(rlp) Eventually(testRLPIsAccepted(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) // Check wasm plugin wasmPluginKey := client.ObjectKey{Name: rlptools.WASMPluginName(gateway), Namespace: testNamespace} @@ -396,6 +399,7 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { // Check RLP status is available rlpKey := client.ObjectKey{Name: rlpName, Namespace: testNamespace} Eventually(testRLPIsAccepted(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) // Check wasm plugin wasmPluginKey := client.ObjectKey{Name: rlptools.WASMPluginName(gateway), Namespace: testNamespace} @@ -496,6 +500,8 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { // Check RLP status is available rlpKey := client.ObjectKey{Name: rlpName, Namespace: testNamespace} Eventually(testRLPIsAccepted(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpKey), time.Minute, 5*time.Second).Should(BeFalse()) + Expect(testRLPEnforcedCondition(rlpKey, kuadrant.PolicyReasonUnknown, "RateLimitPolicy has encountered some issues: no free routes to enforce policy")) // Check wasm plugin wasmPluginKey := client.ObjectKey{Name: rlptools.WASMPluginName(gateway), Namespace: testNamespace} @@ -575,6 +581,7 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { // Check RLP status is available rlpKey := client.ObjectKeyFromObject(rlp) Eventually(testRLPIsAccepted(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) // Check wasm plugin wasmPluginKey := client.ObjectKey{Name: rlptools.WASMPluginName(gateway), Namespace: testNamespace} @@ -637,6 +644,7 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { // Check RLP status is available rlpAKey := client.ObjectKey{Name: rlpAName, Namespace: testNamespace} Eventually(testRLPIsAccepted(rlpAKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpAKey), time.Minute, 5*time.Second).Should(BeTrue()) // create httproute C httpRouteC := testBuildBasicHttpRoute(routeCName, gwName, testNamespace, []string{"*.c.example.com"}) @@ -701,6 +709,7 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { // Check RLP status is available rlpBKey := client.ObjectKey{Name: rlpBName, Namespace: testNamespace} Eventually(testRLPIsAccepted(rlpBKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpBKey), time.Minute, 5*time.Second).Should(BeTrue()) // Check wasm plugin only has configuration ONLY from the RLP targeting the gateway // it may take some reconciliation loops to get to that, so checking it with eventually @@ -834,12 +843,15 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { // Check RLP status is available rlpKey := client.ObjectKey{Name: rlpName, Namespace: testNamespace} Eventually(testRLPIsAccepted(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpKey), time.Minute, 5*time.Second).Should(BeFalse()) + Expect(testRLPEnforcedCondition(rlpKey, kuadrant.PolicyReasonUnknown, "RateLimitPolicy has encountered some issues: no free routes to enforce policy")) // create Route A -> Gw A httpRoute := testBuildBasicHttpRoute(routeName, gwName, testNamespace, []string{"*.example.com"}) err = k8sClient.Create(context.Background(), httpRoute) Expect(err).ToNot(HaveOccurred()) Eventually(testRouteIsAccepted(client.ObjectKeyFromObject(httpRoute)), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) // create Gateway B gwB := testBuildBasicGateway(gwBName, testNamespace) @@ -1039,6 +1051,7 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { // Check RLP status is available rlpKey := client.ObjectKey{Name: rlpName, Namespace: testNamespace} Eventually(testRLPIsAccepted(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) // Initial state set. // Check wasm plugin for gateway A has configuration from the route @@ -1334,6 +1347,7 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { // Check RLP status is available rlpKey := client.ObjectKey{Name: rlpName, Namespace: testNamespace} Eventually(testRLPIsAccepted(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) // Initial state set. // Check wasm plugin has configuration from the route A @@ -1567,6 +1581,7 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { // Check RLP status is available rlp1Key := client.ObjectKey{Name: rlp1Name, Namespace: testNamespace} Eventually(testRLPIsAccepted(rlp1Key), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlp1Key), time.Minute, 5*time.Second).Should(BeTrue()) // Initial state set. // Check wasm plugin for gateway A has configuration from the route 1 @@ -1671,6 +1686,7 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { // Check RLP status is available rlp2Key := client.ObjectKey{Name: rlp2Name, Namespace: testNamespace} Eventually(testRLPIsAccepted(rlp2Key), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlp2Key), time.Minute, 5*time.Second).Should(BeTrue()) // Check wasm plugin has configuration from the route A and RLP 2. // RLP 1 should not add any config to the wasm plugin @@ -1830,6 +1846,7 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { // Check RLP status is available rlp1Key := client.ObjectKey{Name: rlp1Name, Namespace: testNamespace} Eventually(testRLPIsAccepted(rlp1Key), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlp1Key), time.Minute, 5*time.Second).Should(BeTrue()) // create RLP 2 -> Route A rlp2 := &kuadrantv1beta2.RateLimitPolicy{ @@ -1861,6 +1878,7 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { // Check RLP status is available rlp2Key := client.ObjectKey{Name: rlp2Name, Namespace: testNamespace} Eventually(testRLPIsAccepted(rlp2Key), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlp2Key), time.Minute, 5*time.Second).Should(BeTrue()) // Initial state set. // Check wasm plugin for gateway A has configuration from the route A only affected by RLP 2 @@ -2127,6 +2145,7 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { // Check RLP status is available rlpKey := client.ObjectKeyFromObject(rlp) Eventually(testRLPIsAccepted(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) // Check wasm plugin wasmPluginKey := client.ObjectKey{Name: rlptools.WASMPluginName(gateway), Namespace: testNamespace} diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index b4bc83c42..218779b77 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -102,6 +102,7 @@ var _ = Describe("RateLimitPolicy controller", func() { // Check RLP status is available rlpKey := client.ObjectKeyFromObject(rlp) Eventually(testRLPIsAccepted(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) // Check HTTPRoute direct back reference routeKey := client.ObjectKey{Name: routeName, Namespace: testNamespace} @@ -184,6 +185,7 @@ var _ = Describe("RateLimitPolicy controller", func() { // Check RLP status is available rlpKey := client.ObjectKey{Name: rlpName, Namespace: testNamespace} Eventually(testRLPIsAccepted(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) // Check Gateway direct back reference gwKey := client.ObjectKeyFromObject(gateway) @@ -231,6 +233,8 @@ var _ = Describe("RateLimitPolicy controller", func() { // Check RLP status is available rlpKey := client.ObjectKey{Name: rlpName, Namespace: testNamespace} Eventually(testRLPIsAccepted(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpKey), time.Minute, 5*time.Second).Should(BeFalse()) + Expect(testRLPEnforcedCondition(rlpKey, kuadrant.PolicyReasonUnknown, "RateLimitPolicy has encountered some issues: no free routes to enforce policy")) // Check Gateway direct back reference gwKey := client.ObjectKeyFromObject(gateway) @@ -283,6 +287,7 @@ var _ = Describe("RateLimitPolicy controller", func() { Expect(k8sClient.Create(ctx, gwRLP)).To(Succeed()) rlpKey := client.ObjectKey{Name: gwRLP.Name, Namespace: testNamespace} Eventually(testRLPIsAccepted(rlpKey)).WithContext(ctx).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpKey)).WithContext(ctx).Should(BeTrue()) // Create HTTPRoute RLP with new default limits routeRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { @@ -300,6 +305,7 @@ var _ = Describe("RateLimitPolicy controller", func() { Expect(k8sClient.Create(ctx, routeRLP)).To(Succeed()) rlpKey = client.ObjectKey{Name: routeRLP.Name, Namespace: testNamespace} Eventually(testRLPIsAccepted(rlpKey)).WithContext(ctx).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpKey)).WithContext(ctx).Should(BeTrue()) // Check Gateway direct back reference gwKey := client.ObjectKeyFromObject(gateway) @@ -764,6 +770,7 @@ var _ = Describe("RateLimitPolicy controller", func() { // Check RLP status is available rlpKey := client.ObjectKeyFromObject(rlp) Eventually(testRLPIsAccepted(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) // Check HTTPRoute A direct back reference routeAKey := client.ObjectKey{Name: routeAName, Namespace: testNamespace} @@ -792,6 +799,7 @@ var _ = Describe("RateLimitPolicy controller", func() { Expect(err).ToNot(HaveOccurred()) // Check RLP status is available Eventually(testRLPIsAccepted(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) // Check HTTPRoute A direct back reference is gone Eventually( @@ -842,6 +850,8 @@ var _ = Describe("RateLimitPolicy controller", func() { // Check RLP status is available rlpKey := client.ObjectKeyFromObject(rlp) Eventually(testRLPIsAccepted(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpKey), time.Minute, 5*time.Second).Should(BeFalse()) + Expect(testRLPEnforcedCondition(rlpKey, kuadrant.PolicyReasonUnknown, "RateLimitPolicy has encountered some issues: no free routes to enforce policy")) // Check Gateway direct back reference gwAKey := client.ObjectKey{Name: gwAName, Namespace: testNamespace} @@ -870,6 +880,8 @@ var _ = Describe("RateLimitPolicy controller", func() { Expect(err).ToNot(HaveOccurred()) // Check RLP status is available Eventually(testRLPIsAccepted(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpKey), time.Minute, 5*time.Second).Should(BeFalse()) + Expect(testRLPEnforcedCondition(rlpKey, kuadrant.PolicyReasonUnknown, "RateLimitPolicy has encountered some issues: no free routes to enforce policy")) // Check Gw A direct back reference is gone Eventually( @@ -931,6 +943,7 @@ var _ = Describe("RateLimitPolicy controller", func() { rlpAKey := client.ObjectKeyFromObject(rlpA) Eventually(testRLPIsAccepted(rlpAKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpAKey), time.Minute, 5*time.Second).Should(BeTrue()) // create rlpB rlpB := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { @@ -944,6 +957,7 @@ var _ = Describe("RateLimitPolicy controller", func() { // Check RLP status is available rlpBKey := client.ObjectKeyFromObject(rlpB) Eventually(testRLPIsAccepted(rlpBKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpBKey), time.Minute, 5*time.Second).Should(BeTrue()) // Check HTTPRoute A direct back reference routeAKey := client.ObjectKey{Name: routeAName, Namespace: testNamespace} @@ -1020,6 +1034,7 @@ var _ = Describe("RateLimitPolicy controller", func() { rlpKey := client.ObjectKeyFromObject(rlp) Eventually(testRLPIsAccepted(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) // Check HTTPRoute A direct back reference routeKey := client.ObjectKey{Name: routeName, Namespace: testNamespace} @@ -1082,6 +1097,7 @@ var _ = Describe("RateLimitPolicy controller", func() { rlpAKey := client.ObjectKeyFromObject(rlpA) Eventually(testRLPIsAccepted(rlpAKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpAKey), time.Minute, 5*time.Second).Should(BeTrue()) // Proceed with the update: // new RLP B -> Route A (already taken) @@ -1098,6 +1114,7 @@ var _ = Describe("RateLimitPolicy controller", func() { // Check RLP status is not available rlpBKey := client.ObjectKeyFromObject(rlpB) Eventually(testRLPIsNotAccepted(rlpBKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpBKey), time.Minute, 5*time.Second).Should(BeFalse()) // Check HTTPRoute A direct back reference to RLP A routeAKey := client.ObjectKey{Name: routeAName, Namespace: testNamespace} @@ -1136,6 +1153,7 @@ var _ = Describe("RateLimitPolicy controller", func() { time.Minute, 5*time.Second).Should(BeTrue()) Eventually(testRLPIsAccepted(rlpBKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpBKey), time.Minute, 5*time.Second).Should(BeTrue()) routeBKey := client.ObjectKey{Name: routeBName, Namespace: testNamespace} // Check HTTPRoute B direct back reference to RLP A @@ -1147,6 +1165,7 @@ var _ = Describe("RateLimitPolicy controller", func() { time.Minute, 5*time.Second).Should(BeTrue()) Eventually(testRLPIsAccepted(rlpAKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpAKey), time.Minute, 5*time.Second).Should(BeTrue()) }) }) }) From 8f371e777b9f3933b6641bf52c7ac335652a5c59 Mon Sep 17 00:00:00 2001 From: KevFan Date: Mon, 22 Apr 2024 12:21:33 +0100 Subject: [PATCH 12/14] fix: wasm plugin config not accounting for limit overrides --- .../rate_limiting_wasmplugin_controller.go | 24 ++- ...ate_limiting_wasmplugin_controller_test.go | 162 ++++++++++++++++++ 2 files changed, 183 insertions(+), 3 deletions(-) diff --git a/controllers/rate_limiting_wasmplugin_controller.go b/controllers/rate_limiting_wasmplugin_controller.go index bbb2e37ec..393e50911 100644 --- a/controllers/rate_limiting_wasmplugin_controller.go +++ b/controllers/rate_limiting_wasmplugin_controller.go @@ -170,11 +170,11 @@ func (r *RateLimitingWASMPluginReconciler) wasmPluginConfig(ctx context.Context, logger.V(1).Info("wasmPluginConfig", "#RLPS", len(rateLimitPolicies)) // Sort RLPs for consistent comparison with existing objects - sort.Sort(kuadrantgatewayapi.PolicyByCreationTimestamp(rateLimitPolicies)) + sort.Sort(kuadrantgatewayapi.PolicyByTargetRefKindAndCreationTimeStamp(rateLimitPolicies)) for _, policy := range rateLimitPolicies { rlp := policy.(*kuadrantv1beta2.RateLimitPolicy) - wasmRLP, err := r.wasmRateLimitPolicy(ctx, t, rlp, gw) + wasmRLP, err := r.wasmRateLimitPolicy(ctx, t, rlp, gw, rateLimitPolicies) if err != nil { return nil, err } @@ -232,7 +232,9 @@ func (r *RateLimitingWASMPluginReconciler) topologyIndexesFromGateway(ctx contex return kuadrantgatewayapi.NewTopologyIndexes(t), nil } -func (r *RateLimitingWASMPluginReconciler) wasmRateLimitPolicy(ctx context.Context, t *kuadrantgatewayapi.TopologyIndexes, rlp *kuadrantv1beta2.RateLimitPolicy, gw *gatewayapiv1.Gateway) (*wasm.RateLimitPolicy, error) { +func (r *RateLimitingWASMPluginReconciler) wasmRateLimitPolicy(ctx context.Context, t *kuadrantgatewayapi.TopologyIndexes, rlp *kuadrantv1beta2.RateLimitPolicy, gw *gatewayapiv1.Gateway, affectedPolices []kuadrantgatewayapi.Policy) (*wasm.RateLimitPolicy, error) { + logger, _ := logr.FromContext(ctx) + route, err := r.routeFromRLP(ctx, t, rlp, gw) if err != nil { return nil, err @@ -265,6 +267,22 @@ func (r *RateLimitingWASMPluginReconciler) wasmRateLimitPolicy(ctx context.Conte routeWithEffectiveHostnames := route.DeepCopy() routeWithEffectiveHostnames.Spec.Hostnames = hostnames + // Policy limits may be overridden by a gateway policy for route policies + if kuadrantgatewayapi.IsTargetRefHTTPRoute(rlp.GetTargetRef()) { + filteredPolicies := utils.Filter(affectedPolices, func(p kuadrantgatewayapi.Policy) bool { + return kuadrantgatewayapi.IsTargetRefGateway(p.GetTargetRef()) && p.GetUID() != rlp.GetUID() + }) + + for _, policy := range filteredPolicies { + p := policy.(*kuadrantv1beta2.RateLimitPolicy) + if p.Spec.Overrides != nil { + rlp.Spec.CommonSpec().Limits = p.Spec.Overrides.Limits + logger.V(1).Info("applying overrides from parent policy", "parentPolicy", client.ObjectKeyFromObject(p)) + break + } + } + } + rules := rlptools.WasmRules(rlp, routeWithEffectiveHostnames) if len(rules) == 0 { // 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 diff --git a/controllers/rate_limiting_wasmplugin_controller_test.go b/controllers/rate_limiting_wasmplugin_controller_test.go index 07e873160..f8ba3eed7 100644 --- a/controllers/rate_limiting_wasmplugin_controller_test.go +++ b/controllers/rate_limiting_wasmplugin_controller_test.go @@ -2197,4 +2197,166 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { })) }) }) + + Context("Gateway defaults & overrides", func() { + var ( + routeName = "toystore-route" + gwRLPName = "gw-rlp" + routeRLPName = "route-rlp" + gwName = "toystore-gw" + gateway *gatewayapiv1.Gateway + ) + + beforeEachCallback := func() { + gateway = testBuildBasicGateway(gwName, testNamespace) + err := k8sClient.Create(context.Background(), gateway) + Expect(err).ToNot(HaveOccurred()) + Eventually(testGatewayIsReady(gateway), 30*time.Second, 5*time.Second).Should(BeTrue()) + } + + expectedWasmPluginConfig := func(rlpKey client.ObjectKey, rlp *kuadrantv1beta2.RateLimitPolicy, key, hostname string) *wasm.Plugin { + return &wasm.Plugin{ + FailureMode: wasm.FailureModeDeny, + RateLimitPolicies: []wasm.RateLimitPolicy{ + { + Name: rlpKey.String(), + Domain: rlptools.LimitsNamespaceFromRLP(rlp), + Rules: []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: key, + Value: "1", + }, + }, + }, + }, + }, + Hostnames: []string{hostname}, + Service: common.KuadrantRateLimitClusterName, + }, + }, + } + } + + BeforeEach(beforeEachCallback) + + It("Limit key shifts correctly from Gateway RLP default -> Route RLP -> Gateway RLP overrides", func(ctx SpecContext) { + // create httproute + httpRoute := testBuildBasicHttpRoute(routeName, gwName, testNamespace, []string{"*.example.com"}) + Expect(k8sClient.Create(ctx, httpRoute)).To(Succeed()) + Eventually(testRouteIsAccepted(client.ObjectKeyFromObject(httpRoute))).WithContext(ctx).Should(BeTrue()) + + // create GW ratelimitpolicy with defaults + gwRLP := &kuadrantv1beta2.RateLimitPolicy{ + TypeMeta: metav1.TypeMeta{ + Kind: "RateLimitPolicy", APIVersion: kuadrantv1beta2.GroupVersion.String(), + }, + ObjectMeta: metav1.ObjectMeta{Name: gwRLPName, Namespace: testNamespace}, + Spec: kuadrantv1beta2.RateLimitPolicySpec{ + TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ + Group: gatewayapiv1.GroupName, + Kind: "Gateway", + Name: gatewayapiv1.ObjectName(gwName), + }, + Defaults: &kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "gateway": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, + }, + }, + }, + }, + }, + } + Expect(k8sClient.Create(ctx, gwRLP)).To(Succeed()) + + // Check RLP status is available + gwRLPKey := client.ObjectKeyFromObject(gwRLP) + Eventually(testRLPIsAccepted(gwRLPKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(gwRLPKey), time.Minute, 5*time.Second).Should(BeTrue()) + + // Check wasm plugin + wasmPluginKey := client.ObjectKey{Name: rlptools.WASMPluginName(gateway), Namespace: testNamespace} + Eventually(testWasmPluginIsAvailable(wasmPluginKey), time.Minute, 5*time.Second).Should(BeTrue()) + existingWasmPlugin := &istioclientgoextensionv1alpha1.WasmPlugin{} + // must exist + Expect(k8sClient.Get(ctx, wasmPluginKey, existingWasmPlugin)).To(Succeed()) + existingWASMConfig, err := rlptools.WASMPluginFromStruct(existingWasmPlugin.Spec.PluginConfig) + Expect(err).ToNot(HaveOccurred()) + Expect(existingWASMConfig).To(Equal(expectedWasmPluginConfig(gwRLPKey, gwRLP, "limit.gateway__4ea5ee68", "*"))) + + // Create Route RLP + routeRLP := &kuadrantv1beta2.RateLimitPolicy{ + TypeMeta: metav1.TypeMeta{ + Kind: "RateLimitPolicy", APIVersion: kuadrantv1beta2.GroupVersion.String(), + }, + ObjectMeta: metav1.ObjectMeta{Name: routeRLPName, Namespace: testNamespace}, + Spec: kuadrantv1beta2.RateLimitPolicySpec{ + TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ + Group: gatewayapiv1.GroupName, + Kind: "HTTPRoute", + Name: gatewayapiv1.ObjectName(routeName), + }, + RateLimitPolicyCommonSpec: kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "route": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 10, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, + }, + }, + }, + }, + }, + } + Expect(k8sClient.Create(ctx, routeRLP)).To(Succeed()) + routeRLPKey := client.ObjectKeyFromObject(routeRLP) + Eventually(testRLPIsAccepted(routeRLPKey)).WithContext(ctx).Should(BeTrue()) + Eventually(testRLPIsEnforced(routeRLPKey)).WithContext(ctx).Should(BeTrue()) + Eventually(testRLPIsEnforced(gwRLPKey)).WithContext(ctx).Should(BeFalse()) + // Wasm plugin config should now use route RLP limit key + Expect(k8sClient.Get(ctx, wasmPluginKey, existingWasmPlugin)).To(Succeed()) + existingWASMConfig, err = rlptools.WASMPluginFromStruct(existingWasmPlugin.Spec.PluginConfig) + Expect(err).ToNot(HaveOccurred()) + Expect(existingWASMConfig).To(Equal(expectedWasmPluginConfig(routeRLPKey, routeRLP, "limit.route__8a84e406", "*.example.com"))) + + // Update GW RLP to overrides + Eventually(func(g Gomega) { + Expect(k8sClient.Get(ctx, gwRLPKey, gwRLP)).To(Succeed()) + gwRLP.Spec.Overrides = gwRLP.Spec.Defaults.DeepCopy() + gwRLP.Spec.Defaults = nil + Expect(k8sClient.Update(ctx, gwRLP)).To(Succeed()) + }).WithContext(ctx).Should(Succeed()) + Eventually(testRLPIsEnforced(gwRLPKey)).WithContext(ctx).Should(BeTrue()) + Eventually(testRLPIsEnforced(routeRLPKey)).WithContext(ctx).Should(BeFalse()) + // Wasm plugin config should now use GW RLP limit key for route + Expect(k8sClient.Get(ctx, wasmPluginKey, existingWasmPlugin)).To(Succeed()) + existingWASMConfig, err = rlptools.WASMPluginFromStruct(existingWasmPlugin.Spec.PluginConfig) + Expect(err).ToNot(HaveOccurred()) + Expect(existingWASMConfig).To(Equal(expectedWasmPluginConfig(routeRLPKey, routeRLP, "limit.gateway__4ea5ee68", "*.example.com"))) + + }, SpecTimeout(2*time.Minute)) + }) }) From cb4c7783b7aaf3bbeb4b4f85815d3f65de5d5a52 Mon Sep 17 00:00:00 2001 From: KevFan Date: Mon, 22 Apr 2024 17:18:05 +0100 Subject: [PATCH 13/14] tests: AuthPolicy enforced condition message --- controllers/authpolicy_authconfig.go | 56 ++++--- controllers/authpolicy_controller_test.go | 137 +++++++++++------- ...dor_cluster_envoyfilter_controller_test.go | 4 +- 3 files changed, 121 insertions(+), 76 deletions(-) diff --git a/controllers/authpolicy_authconfig.go b/controllers/authpolicy_authconfig.go index 611d36718..15d0c95aa 100644 --- a/controllers/authpolicy_authconfig.go +++ b/controllers/authpolicy_authconfig.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "reflect" + "slices" "strings" "github.com/go-logr/logr" @@ -72,11 +73,11 @@ func (r *AuthPolicyReconciler) desiredAuthConfig(ctx context.Context, ap *api.Au return nil, err } - refs := routeAffectedPolicies(t, ap) - if len(refs) != 0 { + overrides := routeGatewayAuthOverrides(t, ap) + if len(overrides) != 0 { logger.V(1).Info("targeted gateway has authpolicy with atomic overrides, skipping authorino authconfig for the HTTPRoute authpolicy") utils.TagObjectToDelete(authConfig) - r.AffectedPolicyMap.SetAffectedPolicy(ap, refs) + r.AffectedPolicyMap.SetAffectedPolicy(ap, overrides) return authConfig, nil } route = obj @@ -194,26 +195,43 @@ func (r *AuthPolicyReconciler) desiredAuthConfig(ctx context.Context, ap *api.Au return mergeConditionsFromRouteSelectorsIntoConfigs(ap, route, authConfig) } -func routeAffectedPolicies(t *kuadrantgatewayapi.Topology, ap *api.AuthPolicy) []client.ObjectKey { - refs := make([]client.ObjectKey, 0) - for _, gw := range t.Gateways() { - affectedPolicies := utils.Filter(gw.AttachedPolicies(), func(policy kuadrantgatewayapi.Policy) bool { - p, ok := policy.(*api.AuthPolicy) - if !ok { - return false - } - return kuadrantgatewayapi.IsTargetRefGateway(policy.GetTargetRef()) && - ap.GetUID() != policy.GetUID() && p.IsAtomicOverride() - }) +// routeGatewayAuthOverrides returns the GW auth policies that has a +func routeGatewayAuthOverrides(t *kuadrantgatewayapi.Topology, ap *api.AuthPolicy) []client.ObjectKey { + affectedPolicies := getAffectedPolicies(t, ap) + + // Filter the policies where: + // 1. targets a gateway + // 2. is not the current AP that is being assessed + // 3. is an overriding policy + affectedPolicies = utils.Filter(affectedPolicies, func(policy kuadrantgatewayapi.Policy) bool { + p, ok := policy.(*api.AuthPolicy) + if !ok { + return false + } + return kuadrantgatewayapi.IsTargetRefGateway(policy.GetTargetRef()) && + ap.GetUID() != policy.GetUID() && p.IsAtomicOverride() + }) - policyKeys := utils.Map(affectedPolicies, func(policy kuadrantgatewayapi.Policy) client.ObjectKey { - return client.ObjectKeyFromObject(policy) - }) + return utils.Map(affectedPolicies, func(policy kuadrantgatewayapi.Policy) client.ObjectKey { + return client.ObjectKeyFromObject(policy) + }) +} - refs = append(refs, policyKeys...) +func getAffectedPolicies(t *kuadrantgatewayapi.Topology, ap *api.AuthPolicy) []kuadrantgatewayapi.Policy { + topologyIndexes := kuadrantgatewayapi.NewTopologyIndexes(t) + var affectedPolicies []kuadrantgatewayapi.Policy + + // If AP is listed within the policies from gateway, it potentially can be overridden by it + for _, gw := range t.Gateways() { + policyList := topologyIndexes.PoliciesFromGateway(gw.Gateway) + if slices.Contains(utils.Map(policyList, func(p kuadrantgatewayapi.Policy) client.ObjectKey { + return client.ObjectKeyFromObject(p) + }), client.ObjectKeyFromObject(ap)) { + affectedPolicies = append(affectedPolicies, policyList...) + } } - return refs + return affectedPolicies } // authConfigName returns the name of Authorino AuthConfig CR. diff --git a/controllers/authpolicy_controller_test.go b/controllers/authpolicy_controller_test.go index ea9402a8a..68a7c43a8 100644 --- a/controllers/authpolicy_controller_test.go +++ b/controllers/authpolicy_controller_test.go @@ -39,8 +39,8 @@ const ( var _ = Describe("AuthPolicy controller", func() { var testNamespace string - BeforeEach(func() { - CreateNamespace(&testNamespace) + BeforeEach(func(ctx SpecContext) { + CreateNamespaceWithContext(ctx, &testNamespace) gateway := testBuildBasicGateway(testGatewayName, testNamespace) err := k8sClient.Create(context.Background(), gateway) @@ -49,9 +49,11 @@ var _ = Describe("AuthPolicy controller", func() { Eventually(testGatewayIsReady(gateway), 15*time.Second, 5*time.Second).Should(BeTrue()) ApplyKuadrantCR(testNamespace) - }) + }, NodeTimeout(3*time.Minute)) - AfterEach(DeleteNamespaceCallback(&testNamespace)) + AfterEach(func(ctx SpecContext) { + DeleteNamespaceCallbackWithContext(ctx, &testNamespace) + }, NodeTimeout(3*time.Minute)) policyFactory := func(mutateFns ...func(policy *api.AuthPolicy)) *api.AuthPolicy { policy := &api.AuthPolicy{ @@ -1283,7 +1285,7 @@ var _ = Describe("AuthPolicy controller", func() { Eventually(testRouteIsAccepted(client.ObjectKeyFromObject(route)), time.Minute, 5*time.Second).Should(BeTrue()) }) - It("Gateway AuthPolicy has overrides and Route AuthPolicy is added.", func() { + It("Gateway AuthPolicy has overrides and Route AuthPolicy is added.", func(ctx SpecContext) { gatewayPolicy := policyFactory(func(policy *api.AuthPolicy) { policy.Name = "gw-auth" policy.Spec.TargetRef.Group = gatewayapiv1.GroupName @@ -1295,13 +1297,13 @@ var _ = Describe("AuthPolicy controller", func() { policy.Spec.Overrides.AuthScheme.Authentication["apiKey"].ApiKey.Selector.MatchLabels["admin"] = "yes" }) - err := k8sClient.Create(context.Background(), gatewayPolicy) + err := k8sClient.Create(ctx, gatewayPolicy) logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(gatewayPolicy).String(), "error", err) Expect(err).ToNot(HaveOccurred()) // check policy status - Eventually(isAuthPolicyAccepted(gatewayPolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) - Eventually(isAuthPolicyEnforced(gatewayPolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(isAuthPolicyAccepted(gatewayPolicy)).WithContext(ctx).Should(BeTrue()) + Eventually(isAuthPolicyEnforced(gatewayPolicy)).WithContext(ctx).Should(BeTrue()) routePolicy := policyFactory() err = k8sClient.Create(context.Background(), routePolicy) @@ -1309,17 +1311,19 @@ var _ = Describe("AuthPolicy controller", func() { Expect(err).ToNot(HaveOccurred()) // check policy status - Eventually(isAuthPolicyAccepted(routePolicy), 30*time.Second, 5*time.Second).Should(BeFalse()) - }) + Eventually(isAuthPolicyAccepted(routePolicy)).WithContext(ctx).Should(BeTrue()) + Eventually(isAuthPolicyEnforced(routePolicy)).WithContext(ctx).Should(BeFalse()) + Eventually(isAuthPolicyEnforcedCondition(client.ObjectKeyFromObject(routePolicy), kuadrant.PolicyReasonOverridden, fmt.Sprintf("AuthPolicy is overridden by [%s]", client.ObjectKeyFromObject(gatewayPolicy)))).WithContext(ctx).Should(BeTrue()) + }, SpecTimeout(2*time.Minute)) - It("Route AuthPolicy exists and Gateway AuthPolicy with overrides is added.", func() { + It("Route AuthPolicy exists and Gateway AuthPolicy with overrides is added.", func(ctx SpecContext) { routePolicy := policyFactory() - err := k8sClient.Create(context.Background(), routePolicy) + err := k8sClient.Create(ctx, routePolicy) logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(routePolicy).String(), "error", err) Expect(err).ToNot(HaveOccurred()) // check policy status - Eventually(isAuthPolicyAccepted(routePolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(isAuthPolicyAccepted(routePolicy)).WithContext(ctx).Should(BeTrue()) gatewayPolicy := policyFactory(func(policy *api.AuthPolicy) { policy.Name = "gw-auth" @@ -1332,24 +1336,26 @@ var _ = Describe("AuthPolicy controller", func() { policy.Spec.Overrides.AuthScheme.Authentication["apiKey"].ApiKey.Selector.MatchLabels["admin"] = "yes" }) - err = k8sClient.Create(context.Background(), gatewayPolicy) + err = k8sClient.Create(ctx, gatewayPolicy) logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(gatewayPolicy).String(), "error", err) Expect(err).ToNot(HaveOccurred()) // check policy status - Eventually(isAuthPolicyAccepted(gatewayPolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) - Eventually(isAuthPolicyEnforced(gatewayPolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) - Eventually(isAuthPolicyEnforced(routePolicy), 30*time.Second, 5*time.Second).Should(BeFalse()) - }) + Eventually(isAuthPolicyAccepted(gatewayPolicy)).WithContext(ctx).Should(BeTrue()) + Eventually(isAuthPolicyEnforced(gatewayPolicy)).WithContext(ctx).Should(BeTrue()) + Eventually(isAuthPolicyEnforced(routePolicy)).WithContext(ctx).Should(BeFalse()) + Eventually(isAuthPolicyEnforcedCondition(client.ObjectKeyFromObject(routePolicy), kuadrant.PolicyReasonOverridden, fmt.Sprintf("AuthPolicy is overridden by [%s]", client.ObjectKeyFromObject(gatewayPolicy)))).WithContext(ctx).Should(BeTrue()) + }, SpecTimeout(2*time.Minute)) - It("Route AuthPolicy exists and Gateway AuthPolicy with overrides is removed.", func() { + It("Route AuthPolicy exists and Gateway AuthPolicy with overrides is removed.", func(ctx SpecContext) { routePolicy := policyFactory() - err := k8sClient.Create(context.Background(), routePolicy) + err := k8sClient.Create(ctx, routePolicy) logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(routePolicy).String(), "error", err) Expect(err).ToNot(HaveOccurred()) // check policy status - Eventually(isAuthPolicyAccepted(routePolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(isAuthPolicyAccepted(routePolicy)).WithContext(ctx).Should(BeTrue()) + Eventually(isAuthPolicyEnforced(routePolicy)).WithContext(ctx).Should(BeTrue()) gatewayPolicy := policyFactory(func(policy *api.AuthPolicy) { policy.Name = "gw-auth" @@ -1362,31 +1368,32 @@ var _ = Describe("AuthPolicy controller", func() { policy.Spec.Overrides.AuthScheme.Authentication["apiKey"].ApiKey.Selector.MatchLabels["admin"] = "yes" }) - err = k8sClient.Create(context.Background(), gatewayPolicy) + err = k8sClient.Create(ctx, gatewayPolicy) logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(gatewayPolicy).String(), "error", err) Expect(err).ToNot(HaveOccurred()) // check policy status - Eventually(isAuthPolicyAccepted(gatewayPolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) - Eventually(isAuthPolicyEnforced(gatewayPolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) - Eventually(isAuthPolicyEnforced(routePolicy), 30*time.Second, 5*time.Second).Should(BeFalse()) + Eventually(isAuthPolicyAccepted(gatewayPolicy)).WithContext(ctx).Should(BeTrue()) + Eventually(isAuthPolicyEnforced(gatewayPolicy)).WithContext(ctx).Should(BeTrue()) + Eventually(isAuthPolicyEnforced(routePolicy)).WithContext(ctx).Should(BeFalse()) + Eventually(isAuthPolicyEnforcedCondition(client.ObjectKeyFromObject(routePolicy), kuadrant.PolicyReasonOverridden, fmt.Sprintf("AuthPolicy is overridden by [%s]", client.ObjectKeyFromObject(gatewayPolicy)))).WithContext(ctx).Should(BeTrue()) err = k8sClient.Delete(context.Background(), gatewayPolicy) logf.Log.V(1).Info("Deleting AuthPolicy", "key", client.ObjectKeyFromObject(gatewayPolicy).String(), "error", err) Expect(err).ToNot(HaveOccurred()) // check policy status - Eventually(isAuthPolicyEnforced(routePolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) - }) + Eventually(isAuthPolicyEnforced(routePolicy)).WithContext(ctx).Should(BeTrue()) + }, SpecTimeout(2*time.Minute)) - It("Route and Gateway AuthPolicies exist. Gateway AuthPolicy updated to include overrides.", func() { + It("Route and Gateway AuthPolicies exist. Gateway AuthPolicy updated to include overrides.", func(ctx SpecContext) { routePolicy := policyFactory() - err := k8sClient.Create(context.Background(), routePolicy) + err := k8sClient.Create(ctx, routePolicy) logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(routePolicy).String(), "error", err) Expect(err).ToNot(HaveOccurred()) // check policy status - Eventually(isAuthPolicyAccepted(routePolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(isAuthPolicyAccepted(routePolicy)).WithContext(ctx).Should(BeTrue()) gatewayPolicy := policyFactory(func(policy *api.AuthPolicy) { policy.Name = "gw-auth" @@ -1396,14 +1403,15 @@ var _ = Describe("AuthPolicy controller", func() { policy.Spec.CommonSpec().AuthScheme.Authentication["apiKey"].ApiKey.Selector.MatchLabels["admin"] = "yes" }) - err = k8sClient.Create(context.Background(), gatewayPolicy) + err = k8sClient.Create(ctx, gatewayPolicy) logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(gatewayPolicy).String(), "error", err) Expect(err).ToNot(HaveOccurred()) // check policy status - Eventually(isAuthPolicyAccepted(gatewayPolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) - Eventually(isAuthPolicyEnforced(gatewayPolicy), 30*time.Second, 5*time.Second).Should(BeFalse()) - Eventually(isAuthPolicyEnforced(routePolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(isAuthPolicyAccepted(gatewayPolicy)).WithContext(ctx).Should(BeTrue()) + Eventually(isAuthPolicyEnforced(gatewayPolicy)).WithContext(ctx).Should(BeFalse()) + Eventually(isAuthPolicyEnforcedCondition(client.ObjectKeyFromObject(gatewayPolicy), kuadrant.PolicyReasonOverridden, fmt.Sprintf("AuthPolicy is overridden by [%s]", client.ObjectKeyFromObject(routePolicy)))).WithContext(ctx).Should(BeTrue()) + Eventually(isAuthPolicyEnforced(routePolicy)).WithContext(ctx).Should(BeTrue()) Eventually(func() bool { err = k8sClient.Get(context.Background(), client.ObjectKeyFromObject(gatewayPolicy), gatewayPolicy) @@ -1417,23 +1425,24 @@ var _ = Describe("AuthPolicy controller", func() { err = k8sClient.Update(context.Background(), gatewayPolicy) logf.Log.V(1).Info("Updating AuthPolicy", "key", client.ObjectKeyFromObject(gatewayPolicy).String(), "error", err) return err == nil - }, 30*time.Second, 5*time.Second).Should(BeTrue()) + }).WithContext(ctx).Should(BeTrue()) // check policy status - Eventually(isAuthPolicyAccepted(gatewayPolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) - Eventually(isAuthPolicyEnforced(gatewayPolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) - Eventually(isAuthPolicyEnforced(routePolicy), 30*time.Second, 5*time.Second).Should(BeFalse()) + Eventually(isAuthPolicyAccepted(gatewayPolicy)).WithContext(ctx).Should(BeTrue()) + Eventually(isAuthPolicyEnforced(gatewayPolicy)).WithContext(ctx).Should(BeTrue()) + Eventually(isAuthPolicyEnforced(routePolicy)).WithContext(ctx).Should(BeFalse()) + Eventually(isAuthPolicyEnforcedCondition(client.ObjectKeyFromObject(routePolicy), kuadrant.PolicyReasonOverridden, fmt.Sprintf("AuthPolicy is overridden by [%s]", client.ObjectKeyFromObject(gatewayPolicy)))).WithContext(ctx).Should(BeTrue()) + }, SpecTimeout(2*time.Minute)) - }) - - It("Route and Gateway AuthPolicies exist. Gateway AuthPolicy updated to remove overrides.", func() { + It("Route and Gateway AuthPolicies exist. Gateway AuthPolicy updated to remove overrides.", func(ctx SpecContext) { routePolicy := policyFactory() - err := k8sClient.Create(context.Background(), routePolicy) + err := k8sClient.Create(ctx, routePolicy) logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(routePolicy).String(), "error", err) Expect(err).ToNot(HaveOccurred()) // check policy status - Eventually(isAuthPolicyAccepted(routePolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(isAuthPolicyAccepted(routePolicy)).WithContext(ctx).Should(BeTrue()) + Eventually(isAuthPolicyEnforced(routePolicy)).WithContext(ctx).Should(BeFalse()) gatewayPolicy := policyFactory(func(policy *api.AuthPolicy) { policy.Name = "gw-auth" @@ -1446,45 +1455,47 @@ var _ = Describe("AuthPolicy controller", func() { policy.Spec.Overrides.AuthScheme.Authentication["apiKey"].ApiKey.Selector.MatchLabels["admin"] = "yes" }) - err = k8sClient.Create(context.Background(), gatewayPolicy) + err = k8sClient.Create(ctx, gatewayPolicy) logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(gatewayPolicy).String(), "error", err) Expect(err).ToNot(HaveOccurred()) // check policy status - Eventually(isAuthPolicyAccepted(gatewayPolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) - Eventually(isAuthPolicyEnforced(gatewayPolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) - Eventually(isAuthPolicyEnforced(routePolicy), 30*time.Second, 5*time.Second).Should(BeFalse()) + Eventually(isAuthPolicyAccepted(gatewayPolicy)).WithContext(ctx).Should(BeTrue()) + Eventually(isAuthPolicyEnforced(gatewayPolicy)).WithContext(ctx).Should(BeTrue()) + Eventually(isAuthPolicyEnforced(routePolicy)).WithContext(ctx).Should(BeFalse()) + Eventually(isAuthPolicyEnforcedCondition(client.ObjectKeyFromObject(routePolicy), kuadrant.PolicyReasonOverridden, fmt.Sprintf("AuthPolicy is overridden by [%s]", client.ObjectKeyFromObject(gatewayPolicy)))).WithContext(ctx).Should(BeTrue()) Eventually(func() bool { - err = k8sClient.Get(context.Background(), client.ObjectKeyFromObject(gatewayPolicy), gatewayPolicy) + err = k8sClient.Get(ctx, client.ObjectKeyFromObject(gatewayPolicy), gatewayPolicy) if err != nil { return false } gatewayPolicy.Spec.Overrides = nil gatewayPolicy.Spec.CommonSpec().AuthScheme = testBasicAuthScheme() gatewayPolicy.Spec.CommonSpec().AuthScheme.Authentication["apiKey"].ApiKey.Selector.MatchLabels["admin"] = "yes" - err = k8sClient.Update(context.Background(), gatewayPolicy) + err = k8sClient.Update(ctx, gatewayPolicy) logf.Log.V(1).Info("Updating AuthPolicy", "key", client.ObjectKeyFromObject(gatewayPolicy).String(), "error", err) return err == nil - }, 30*time.Second, 5*time.Second).Should(BeTrue()) + }).WithContext(ctx).Should(BeTrue()) // check policy status - Eventually(isAuthPolicyAccepted(gatewayPolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) - Eventually(isAuthPolicyEnforced(gatewayPolicy), 30*time.Second, 5*time.Second).Should(BeFalse()) - Eventually(isAuthPolicyEnforced(routePolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(isAuthPolicyAccepted(gatewayPolicy)).WithContext(ctx).Should(BeTrue()) + Eventually(isAuthPolicyEnforced(gatewayPolicy)).WithContext(ctx).Should(BeFalse()) + Eventually(isAuthPolicyEnforcedCondition(client.ObjectKeyFromObject(gatewayPolicy), kuadrant.PolicyReasonOverridden, fmt.Sprintf("AuthPolicy is overridden by [%s]", client.ObjectKeyFromObject(routePolicy)))).WithContext(ctx).Should(BeTrue()) + Eventually(isAuthPolicyEnforced(routePolicy)).WithContext(ctx).Should(BeTrue()) }) - It("Blocks creation of AuthPolicies with overrides targeting HTTPRoutes", func() { + It("Blocks creation of AuthPolicies with overrides targeting HTTPRoutes", func(ctx SpecContext) { routePolicy := policyFactory(func(policy *api.AuthPolicy) { policy.Spec.Overrides = &api.AuthPolicyCommonSpec{} policy.Spec.Defaults = nil policy.Spec.Overrides.AuthScheme = testBasicAuthScheme() }) - err := k8sClient.Create(context.Background(), routePolicy) + err := k8sClient.Create(ctx, routePolicy) logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(routePolicy).String(), "error", err) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("Overrides are not allowed for policies targeting a HTTPRoute resource")) - }) + }, SpecTimeout(2*time.Minute)) }) }) @@ -2043,6 +2054,20 @@ func isAuthPolicyEnforced(policy *api.AuthPolicy) func() bool { return isAuthPolicyConditionTrue(policy, string(kuadrant.PolicyConditionEnforced)) } +func isAuthPolicyEnforcedCondition(key client.ObjectKey, reason gatewayapiv1alpha2.PolicyConditionReason, message string) bool { + p := &api.AuthPolicy{} + if err := k8sClient.Get(context.Background(), key, p); err != nil { + return false + } + + cond := meta.FindStatusCondition(p.Status.Conditions, string(kuadrant.PolicyConditionEnforced)) + if cond == nil { + return false + } + + return cond.Reason == string(reason) && cond.Message == message +} + func isAuthPolicyConditionTrue(policy *api.AuthPolicy, condition string) func() bool { return func() bool { existingPolicy := &api.AuthPolicy{} diff --git a/controllers/limitador_cluster_envoyfilter_controller_test.go b/controllers/limitador_cluster_envoyfilter_controller_test.go index eee1e4e66..5e06dfe09 100644 --- a/controllers/limitador_cluster_envoyfilter_controller_test.go +++ b/controllers/limitador_cluster_envoyfilter_controller_test.go @@ -21,6 +21,7 @@ import ( kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" "github.com/kuadrant/kuadrant-operator/pkg/common" + "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" ) var _ = Describe("Limitador Cluster EnvoyFilter controller", func() { @@ -108,7 +109,8 @@ var _ = Describe("Limitador Cluster EnvoyFilter controller", func() { // Check RLP status is available rlpKey := client.ObjectKey{Name: rlpName, Namespace: testNamespace} Eventually(testRLPIsAccepted(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) - Eventually(testRLPIsEnforced(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsEnforced(rlpKey), time.Minute, 5*time.Second).Should(BeFalse()) + Expect(testRLPEnforcedCondition(rlpKey, kuadrant.PolicyReasonUnknown, "RateLimitPolicy has encountered some issues: no free routes to enforce policy")) // Check envoy filter Eventually(func() bool { From 76cc36fce2b86a66a3afe4b066eb0aa4be2d3a05 Mon Sep 17 00:00:00 2001 From: KevFan Date: Mon, 22 Apr 2024 18:35:23 +0100 Subject: [PATCH 14/14] fix: invalid reason not deleting second ns after test & fix missing comments --- controllers/authpolicy_authconfig.go | 2 +- controllers/authpolicy_controller_test.go | 10 +++++----- controllers/ratelimitpolicy_controller_test.go | 4 ++-- controllers/ratelimitpolicy_limits.go | 2 +- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/controllers/authpolicy_authconfig.go b/controllers/authpolicy_authconfig.go index 15d0c95aa..dfd673cb4 100644 --- a/controllers/authpolicy_authconfig.go +++ b/controllers/authpolicy_authconfig.go @@ -195,7 +195,7 @@ func (r *AuthPolicyReconciler) desiredAuthConfig(ctx context.Context, ap *api.Au return mergeConditionsFromRouteSelectorsIntoConfigs(ap, route, authConfig) } -// routeGatewayAuthOverrides returns the GW auth policies that has a +// routeGatewayAuthOverrides returns the GW auth policies that has an override field set func routeGatewayAuthOverrides(t *kuadrantgatewayapi.Topology, ap *api.AuthPolicy) []client.ObjectKey { affectedPolicies := getAffectedPolicies(t, ap) diff --git a/controllers/authpolicy_controller_test.go b/controllers/authpolicy_controller_test.go index 68a7c43a8..72e718298 100644 --- a/controllers/authpolicy_controller_test.go +++ b/controllers/authpolicy_controller_test.go @@ -1136,10 +1136,10 @@ var _ = Describe("AuthPolicy controller", func() { ), 30*time.Second, 5*time.Second).Should(BeTrue()) }) - It("Invalid reason", func() { + It("Invalid reason", func(ctx SpecContext) { var otherNamespace string CreateNamespace(&otherNamespace) - defer DeleteNamespaceCallback(&otherNamespace) + defer DeleteNamespaceCallback(&otherNamespace)() policy := policyFactory(func(policy *api.AuthPolicy) { policy.Namespace = otherNamespace // create the policy in a different namespace than the target @@ -1147,10 +1147,10 @@ var _ = Describe("AuthPolicy controller", func() { policy.Spec.TargetRef.Name = gatewayapiv1.ObjectName(testGatewayName) policy.Spec.TargetRef.Namespace = ptr.To(gatewayapiv1.Namespace(testNamespace)) }) - Expect(k8sClient.Create(context.Background(), policy)).To(Succeed()) + Expect(k8sClient.Create(ctx, policy)).To(Succeed()) - Eventually(assertAcceptedCondFalseAndEnforcedCondNil(policy, string(gatewayapiv1alpha2.PolicyReasonInvalid), fmt.Sprintf("AuthPolicy target is invalid: invalid targetRef.Namespace %s. Currently only supporting references to the same namespace", testNamespace)), 30*time.Second, 5*time.Second).Should(BeTrue()) - }) + Eventually(assertAcceptedCondFalseAndEnforcedCondNil(policy, string(gatewayapiv1alpha2.PolicyReasonInvalid), fmt.Sprintf("AuthPolicy target is invalid: invalid targetRef.Namespace %s. Currently only supporting references to the same namespace", testNamespace))).WithContext(ctx).Should(BeTrue()) + }, SpecTimeout(time.Minute)) }) Context("AuthPolicy enforced condition reasons", func() { diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index 218779b77..014f08f26 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -662,7 +662,7 @@ var _ = Describe("RateLimitPolicy controller", func() { It("Invalid reason", func(ctx SpecContext) { var otherNamespace string CreateNamespace(&otherNamespace) - defer DeleteNamespaceCallback(&otherNamespace) + defer DeleteNamespaceCallback(&otherNamespace)() policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { policy.Namespace = otherNamespace // create the policy in a different namespace than the target @@ -672,7 +672,7 @@ var _ = Describe("RateLimitPolicy controller", func() { }) Expect(k8sClient.Create(ctx, policy)).To(Succeed()) - Eventually(assertAcceptedConditionFalse(ctx, policy, string(gatewayapiv1alpha2.PolicyReasonInvalid), fmt.Sprintf("RateLimitPolicy target is invalid: invalid targetRef.Namespace %s. Currently only supporting references to the same namespace", testNamespace)), 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(assertAcceptedConditionFalse(ctx, policy, string(gatewayapiv1alpha2.PolicyReasonInvalid), fmt.Sprintf("RateLimitPolicy target is invalid: invalid targetRef.Namespace %s. Currently only supporting references to the same namespace", testNamespace))).WithContext(ctx).Should(Succeed()) }, SpecTimeout(time.Minute)) }) diff --git a/controllers/ratelimitpolicy_limits.go b/controllers/ratelimitpolicy_limits.go index e248ee10f..a90817570 100644 --- a/controllers/ratelimitpolicy_limits.go +++ b/controllers/ratelimitpolicy_limits.go @@ -189,7 +189,7 @@ func (r *RateLimitPolicyReconciler) getAffectedPoliciesInfo(rlp *kuadrantv1beta2 } // applyGatewayOverrides a Gateway RLP is not affected if there is untargetted routes or affects other policies -// Otherwise, it +// Otherwise, it has no free routes, and therefore "overridden" by the route policies func (r *RateLimitPolicyReconciler) applyGatewayOverrides(logger logr.Logger, rlp *kuadrantv1beta2.RateLimitPolicy, numUnTargetedRoutes int, affectedPolicies []kuadrantgatewayapi.Policy) { if rlp.Spec.Overrides == nil && numUnTargetedRoutes == 0 { r.AffectedPolicyMap.SetAffectedPolicy(rlp, utils.Map(affectedPolicies, func(p kuadrantgatewayapi.Policy) client.ObjectKey {