From 9ee6aafa60b39b9fca884f39316469c865e2cb04 Mon Sep 17 00:00:00 2001 From: KevFan Date: Mon, 11 Mar 2024 10:30:51 +0000 Subject: [PATCH 01/12] feat: rlp default field --- api/v1beta2/ratelimitpolicy_types.go | 10 + api/v1beta2/zz_generated.deepcopy.go | 47 ++- .../kuadrant.io_ratelimitpolicies.yaml | 368 ++++++++++++++++++ .../bases/kuadrant.io_ratelimitpolicies.yaml | 368 ++++++++++++++++++ ...dor_cluster_envoyfilter_controller_test.go | 12 +- .../ratelimitpolicy_controller_test.go | 40 +- pkg/rlptools/utils_test.go | 97 ++--- pkg/rlptools/wasm_utils_test.go | 4 +- 8 files changed, 871 insertions(+), 75 deletions(-) diff --git a/api/v1beta2/ratelimitpolicy_types.go b/api/v1beta2/ratelimitpolicy_types.go index c7a2910c0..78cb05985 100644 --- a/api/v1beta2/ratelimitpolicy_types.go +++ b/api/v1beta2/ratelimitpolicy_types.go @@ -127,6 +127,16 @@ type RateLimitPolicySpec struct { // +kubebuilder:validation:XValidation:rule="self.kind == 'HTTPRoute' || self.kind == 'Gateway'",message="Invalid targetRef.kind. The only supported values are 'HTTPRoute' and 'Gateway'" TargetRef gatewayapiv1alpha2.PolicyTargetReference `json:"targetRef"` + // Defaults define explicit default values for this policy and for policies inheriting this policy + // +optional + Defaults CommonSpec `json:"defaults"` + + // Implicit default + CommonSpec `json:""` +} + +// CommonSpec contains fields +type CommonSpec struct { // Limits holds the struct of limits indexed by a unique name // +optional // +kubebuilder:validation:MaxProperties=14 diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go index cfb368da2..f0614a2c6 100644 --- a/api/v1beta2/zz_generated.deepcopy.go +++ b/api/v1beta2/zz_generated.deepcopy.go @@ -301,6 +301,44 @@ func (in *CommonAuthRuleSpec) DeepCopy() *CommonAuthRuleSpec { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *CommonSpec) DeepCopyInto(out *CommonSpec) { + *out = *in + if in.Limits != nil { + in, out := &in.Limits, &out.Limits + *out = make(map[string]Limit, len(*in)) + for key, val := range *in { + (*out)[key] = *val.DeepCopy() + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CommonSpec. +func (in *CommonSpec) DeepCopy() *CommonSpec { + if in == nil { + return nil + } + out := new(CommonSpec) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *Defaults) DeepCopyInto(out *Defaults) { + *out = *in + in.CommonSpec.DeepCopyInto(&out.CommonSpec) +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Defaults. +func (in *Defaults) DeepCopy() *Defaults { + if in == nil { + return nil + } + out := new(Defaults) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *HeaderSuccessResponseSpec) DeepCopyInto(out *HeaderSuccessResponseSpec) { *out = *in @@ -449,13 +487,8 @@ func (in *RateLimitPolicyList) DeepCopyObject() runtime.Object { func (in *RateLimitPolicySpec) DeepCopyInto(out *RateLimitPolicySpec) { *out = *in in.TargetRef.DeepCopyInto(&out.TargetRef) - if in.Limits != nil { - in, out := &in.Limits, &out.Limits - *out = make(map[string]Limit, len(*in)) - for key, val := range *in { - (*out)[key] = *val.DeepCopy() - } - } + in.Defaults.DeepCopyInto(&out.Defaults) + in.CommonSpec.DeepCopyInto(&out.CommonSpec) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RateLimitPolicySpec. diff --git a/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml b/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml index 5adb3a30b..84fc07ac2 100644 --- a/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml +++ b/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml @@ -67,6 +67,374 @@ spec: spec: description: RateLimitPolicySpec defines the desired state of RateLimitPolicy properties: + defaults: + description: Defaults define explicit default values for this policy + and for policies inheriting this policy + 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: + \n 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. + \n 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`). \n 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 action. Multiple + match types are ANDed together, i.e. the match + will evaluate to true only if all conditions are + satisfied. \n For example, the match below will + match a HTTP request only if its path starts with + `/foo` AND it contains the `version: v1` header: + \n ``` match: \n path: value: \"/foo\" headers: + - name: \"version\" value \"v1\" \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). + \n 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. \n 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. \n + Support: Core (Exact) \n Support: Implementation-specific + (RegularExpression) \n 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. + \n 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. \n Support: Core + (Exact, PathPrefix) \n 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. \n 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). + \n 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. \n 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. \n 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. + \n Support: Extended (Exact) \n Support: + Implementation-specific (RegularExpression) + \n 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 limits: additionalProperties: description: Limit represents a complete rate limit configuration diff --git a/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml b/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml index 1c722597c..e58c4dd2b 100644 --- a/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml +++ b/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml @@ -66,6 +66,374 @@ spec: spec: description: RateLimitPolicySpec defines the desired state of RateLimitPolicy properties: + defaults: + description: Defaults define explicit default values for this policy + and for policies inheriting this policy + 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: + \n 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. + \n 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`). \n 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 action. Multiple + match types are ANDed together, i.e. the match + will evaluate to true only if all conditions are + satisfied. \n For example, the match below will + match a HTTP request only if its path starts with + `/foo` AND it contains the `version: v1` header: + \n ``` match: \n path: value: \"/foo\" headers: + - name: \"version\" value \"v1\" \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). + \n 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. \n 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. \n + Support: Core (Exact) \n Support: Implementation-specific + (RegularExpression) \n 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. + \n 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. \n Support: Core + (Exact, PathPrefix) \n 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. \n 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). + \n 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. \n 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. \n 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. + \n Support: Extended (Exact) \n Support: + Implementation-specific (RegularExpression) + \n 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 limits: additionalProperties: description: Limit represents a complete rate limit configuration diff --git a/controllers/limitador_cluster_envoyfilter_controller_test.go b/controllers/limitador_cluster_envoyfilter_controller_test.go index a932aad82..8661a1393 100644 --- a/controllers/limitador_cluster_envoyfilter_controller_test.go +++ b/controllers/limitador_cluster_envoyfilter_controller_test.go @@ -90,11 +90,13 @@ var _ = Describe("Limitador Cluster EnvoyFilter controller", func() { Kind: "Gateway", Name: gatewayapiv1.ObjectName(gwName), }, - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + CommonSpec: kuadrantv1beta2.CommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, }, }, }, diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index 6d0512173..905a102d1 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -47,11 +47,13 @@ var _ = Describe("RateLimitPolicy controller", func() { Kind: "HTTPRoute", Name: gatewayapiv1.ObjectName(routeName), }, - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + CommonSpec: kuadrantv1beta2.CommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, }, }, }, @@ -338,20 +340,22 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() { Kind: "Gateway", Name: "my-gw", }, - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + CommonSpec: kuadrantv1beta2.CommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, }, - }, - RouteSelectors: []kuadrantv1beta2.RouteSelector{ - { - Hostnames: []gatewayapiv1.Hostname{"*.foo.io"}, - Matches: []gatewayapiv1.HTTPRouteMatch{ - { - Path: &gatewayapiv1.HTTPPathMatch{ - Value: ptr.To("/foo"), + RouteSelectors: []kuadrantv1beta2.RouteSelector{ + { + Hostnames: []gatewayapiv1.Hostname{"*.foo.io"}, + Matches: []gatewayapiv1.HTTPRouteMatch{ + { + Path: &gatewayapiv1.HTTPPathMatch{ + Value: ptr.To("/foo"), + }, }, }, }, diff --git a/pkg/rlptools/utils_test.go b/pkg/rlptools/utils_test.go index 5db768bbd..e3077b789 100644 --- a/pkg/rlptools/utils_test.go +++ b/pkg/rlptools/utils_test.go @@ -7,10 +7,11 @@ import ( "regexp" "testing" - "github.com/kuadrant/kuadrant-operator/pkg/library/utils" limitadorv1alpha1 "github.com/kuadrant/limitador-operator/api/v1alpha1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/kuadrant/kuadrant-operator/pkg/library/utils" + kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" ) @@ -25,13 +26,15 @@ func testRLP_1Limit_1Rate(ns, name string) *kuadrantv1beta2.RateLimitPolicy { Namespace: ns, }, Spec: kuadrantv1beta2.RateLimitPolicySpec{ - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 5, - Duration: 10, - Unit: "second", + CommonSpec: kuadrantv1beta2.CommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 5, + Duration: 10, + Unit: "second", + }, }, }, }, @@ -51,22 +54,24 @@ func testRLP_2Limits_1Rate(ns, name string) *kuadrantv1beta2.RateLimitPolicy { Namespace: ns, }, Spec: kuadrantv1beta2.RateLimitPolicySpec{ - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 5, - Duration: 10, - Unit: "second", + CommonSpec: kuadrantv1beta2.CommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 5, + Duration: 10, + Unit: "second", + }, }, }, - }, - "l2": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 3, - Duration: 1, - Unit: "hour", + "l2": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 3, + Duration: 1, + Unit: "hour", + }, }, }, }, @@ -86,18 +91,20 @@ func testRLP_1Limit_2Rates(ns, name string) *kuadrantv1beta2.RateLimitPolicy { Namespace: ns, }, Spec: kuadrantv1beta2.RateLimitPolicySpec{ - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 5, - Duration: 10, - Unit: "second", - }, - { - Limit: 3, - Duration: 1, - Unit: "minute", + CommonSpec: kuadrantv1beta2.CommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 5, + Duration: 10, + Unit: "second", + }, + { + Limit: 3, + Duration: 1, + Unit: "minute", + }, }, }, }, @@ -117,16 +124,18 @@ func testRLP_1Limit_1Rate_1Counter(ns, name string) *kuadrantv1beta2.RateLimitPo Namespace: ns, }, Spec: kuadrantv1beta2.RateLimitPolicySpec{ - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Counters: []kuadrantv1beta2.ContextSelector{ - "request.path", - }, - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 5, - Duration: 10, - Unit: "second", + CommonSpec: kuadrantv1beta2.CommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + Counters: []kuadrantv1beta2.ContextSelector{ + "request.path", + }, + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 5, + Duration: 10, + Unit: "second", + }, }, }, }, diff --git a/pkg/rlptools/wasm_utils_test.go b/pkg/rlptools/wasm_utils_test.go index ad728fa90..7412a6ac3 100644 --- a/pkg/rlptools/wasm_utils_test.go +++ b/pkg/rlptools/wasm_utils_test.go @@ -51,7 +51,9 @@ func TestWasmRules(t *testing.T) { Namespace: "my-app", }, Spec: kuadrantv1beta2.RateLimitPolicySpec{ - Limits: limits, + CommonSpec: kuadrantv1beta2.CommonSpec{ + Limits: limits, + }, }, } } From 12ad279748dc0604a7922122aa4f85dfc422f77a Mon Sep 17 00:00:00 2001 From: KevFan Date: Mon, 11 Mar 2024 12:08:43 +0000 Subject: [PATCH 02/12] refactor: rlp GetLimits based on implicit or default rules --- api/v1beta2/ratelimitpolicy_types.go | 11 ++- api/v1beta2/ratelimitpolicy_types_test.go | 82 +++++++++++++++++++++++ api/v1beta2/zz_generated.deepcopy.go | 16 ----- pkg/rlptools/utils.go | 5 +- pkg/rlptools/wasm_utils.go | 4 +- 5 files changed, 97 insertions(+), 21 deletions(-) diff --git a/api/v1beta2/ratelimitpolicy_types.go b/api/v1beta2/ratelimitpolicy_types.go index 78cb05985..4c2a4df1a 100644 --- a/api/v1beta2/ratelimitpolicy_types.go +++ b/api/v1beta2/ratelimitpolicy_types.go @@ -247,7 +247,7 @@ func (r *RateLimitPolicy) GetWrappedNamespace() gatewayapiv1.Namespace { func (r *RateLimitPolicy) GetRulesHostnames() (ruleHosts []string) { ruleHosts = make([]string, 0) - for _, limit := range r.Spec.Limits { + for _, limit := range r.GetLimits() { for _, routeSelector := range limit.RouteSelectors { convertHostnamesToString := func(gwHostnames []gatewayapiv1.Hostname) []string { hostnames := make([]string, 0, len(gwHostnames)) @@ -274,6 +274,15 @@ func (r *RateLimitPolicy) DirectReferenceAnnotationName() string { return RateLimitPolicyDirectReferenceAnnotationName } +// GetLimits returns the limits based on whether default or implicit rules are defined. +// Default rules takes precedence +func (r *RateLimitPolicy) GetLimits() map[string]Limit { + if r.Spec.Defaults.Limits != nil { + return r.Spec.Defaults.Limits + } + return r.Spec.Limits +} + func init() { SchemeBuilder.Register(&RateLimitPolicy{}, &RateLimitPolicyList{}) } diff --git a/api/v1beta2/ratelimitpolicy_types_test.go b/api/v1beta2/ratelimitpolicy_types_test.go index 5cd7b28a9..d598d07ec 100644 --- a/api/v1beta2/ratelimitpolicy_types_test.go +++ b/api/v1beta2/ratelimitpolicy_types_test.go @@ -3,6 +3,7 @@ package v1beta2 import ( + "reflect" "strings" "testing" @@ -75,3 +76,84 @@ func TestRateLimitPolicyListGetItems(t *testing.T) { t.Errorf("Expected item to be a Policy") } } + +func TestRateLimitPolicy_GetLimits(t *testing.T) { + var ( + defaultLimits = map[string]Limit{ + "default": { + Rates: []Rate{ + { + Limit: 10, + Duration: 1, + Unit: "seconds", + }, + }, + }, + } + + implicitLimits = map[string]Limit{ + "implicit": { + Rates: []Rate{ + { + Limit: 20, + Duration: 2, + Unit: "seconds", + }, + }, + }, + } + ) + + type fields struct { + Spec RateLimitPolicySpec + } + tests := []struct { + name string + fields fields + want map[string]Limit + }{ + { + name: "No limits defined", + fields: fields{Spec: RateLimitPolicySpec{}}, + want: nil, + }, + { + name: "Defaults defined", + fields: fields{ + Spec: RateLimitPolicySpec{ + Defaults: CommonSpec{Limits: defaultLimits}, + }, + }, + want: defaultLimits, + }, + { + name: "implicit rules defined", + fields: fields{ + Spec: RateLimitPolicySpec{ + CommonSpec: CommonSpec{Limits: implicitLimits}, + }, + }, + want: implicitLimits, + }, + { + name: "default rules takes precedence over implicit rules", + fields: fields{ + Spec: RateLimitPolicySpec{ + CommonSpec: CommonSpec{Limits: implicitLimits}, + Defaults: CommonSpec{Limits: defaultLimits}, + }, + }, + want: defaultLimits, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := &RateLimitPolicy{ + Spec: tt.fields.Spec, + } + if got := r.GetLimits(); !reflect.DeepEqual(got, tt.want) { + t.Errorf("GetLimits() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go index f0614a2c6..bde5f2446 100644 --- a/api/v1beta2/zz_generated.deepcopy.go +++ b/api/v1beta2/zz_generated.deepcopy.go @@ -323,22 +323,6 @@ func (in *CommonSpec) DeepCopy() *CommonSpec { return out } -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *Defaults) DeepCopyInto(out *Defaults) { - *out = *in - in.CommonSpec.DeepCopyInto(&out.CommonSpec) -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Defaults. -func (in *Defaults) DeepCopy() *Defaults { - if in == nil { - return nil - } - out := new(Defaults) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *HeaderSuccessResponseSpec) DeepCopyInto(out *HeaderSuccessResponseSpec) { *out = *in diff --git a/pkg/rlptools/utils.go b/pkg/rlptools/utils.go index 30348b240..56f9cb83e 100644 --- a/pkg/rlptools/utils.go +++ b/pkg/rlptools/utils.go @@ -6,9 +6,10 @@ import ( "fmt" "unicode" - "github.com/kuadrant/kuadrant-operator/pkg/library/utils" limitadorv1alpha1 "github.com/kuadrant/limitador-operator/api/v1alpha1" + "github.com/kuadrant/kuadrant-operator/pkg/library/utils" + kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" ) @@ -41,7 +42,7 @@ func LimitadorRateLimitsFromRLP(rlp *kuadrantv1beta2.RateLimitPolicy) []limitado limitsNamespace := LimitsNamespaceFromRLP(rlp) rateLimits := make([]limitadorv1alpha1.RateLimit, 0) - for limitKey, limit := range rlp.Spec.Limits { + for limitKey, limit := range rlp.GetLimits() { limitIdentifier := LimitNameToLimitadorIdentifier(limitKey) for _, rate := range limit.Rates { maxValue, seconds := rateToSeconds(rate) diff --git a/pkg/rlptools/wasm_utils.go b/pkg/rlptools/wasm_utils.go index 0421a02fb..b997af767 100644 --- a/pkg/rlptools/wasm_utils.go +++ b/pkg/rlptools/wasm_utils.go @@ -33,7 +33,7 @@ func WasmRules(rlp *kuadrantv1beta2.RateLimitPolicy, route *gatewayapiv1.HTTPRou // Sort RLP limits for consistent comparison with existing wasmplugin objects limitNames := make([]string, 0, len(rlp.Spec.Limits)) - for name := range rlp.Spec.Limits { + for name := range rlp.GetLimits() { limitNames = append(limitNames, name) } @@ -42,7 +42,7 @@ func WasmRules(rlp *kuadrantv1beta2.RateLimitPolicy, route *gatewayapiv1.HTTPRou for _, limitName := range limitNames { // 1 RLP limit <---> 1 WASM rule - limit := rlp.Spec.Limits[limitName] + limit := rlp.GetLimits()[limitName] limitIdentifier := LimitNameToLimitadorIdentifier(limitName) rule, err := ruleFromLimit(limitIdentifier, &limit, route) if err == nil { From 6bdcd5d391bdcca54269106473a520e021a149ed Mon Sep 17 00:00:00 2001 From: KevFan Date: Mon, 11 Mar 2024 15:50:39 +0000 Subject: [PATCH 03/12] feat: rlp validation to prevent both implicit and explicit default rules --- api/v1beta2/ratelimitpolicy_types.go | 9 ++ api/v1beta2/ratelimitpolicy_types_test.go | 180 ++++++++++++---------- 2 files changed, 104 insertions(+), 85 deletions(-) diff --git a/api/v1beta2/ratelimitpolicy_types.go b/api/v1beta2/ratelimitpolicy_types.go index 4c2a4df1a..bfcacfcf9 100644 --- a/api/v1beta2/ratelimitpolicy_types.go +++ b/api/v1beta2/ratelimitpolicy_types.go @@ -143,6 +143,11 @@ type CommonSpec struct { Limits map[string]Limit `json:"limits,omitempty"` } +// IsUsed determines if any fields within CommonSpec is used +func (c CommonSpec) IsUsed() bool { + return c.Limits != nil +} + // RateLimitPolicyStatus defines the observed state of RateLimitPolicy type RateLimitPolicyStatus struct { // ObservedGeneration reflects the generation of the most recently observed spec. @@ -207,6 +212,10 @@ func (r *RateLimitPolicy) Validate() error { return fmt.Errorf("invalid targetRef.Namespace %s. Currently only supporting references to the same namespace", *r.Spec.TargetRef.Namespace) } + if r.Spec.Defaults.IsUsed() && r.Spec.CommonSpec.IsUsed() { + return fmt.Errorf("cannot use implicit defaults if explicit defaults are defined") + } + return nil } diff --git a/api/v1beta2/ratelimitpolicy_types_test.go b/api/v1beta2/ratelimitpolicy_types_test.go index d598d07ec..95c37f501 100644 --- a/api/v1beta2/ratelimitpolicy_types_test.go +++ b/api/v1beta2/ratelimitpolicy_types_test.go @@ -3,10 +3,10 @@ package v1beta2 import ( - "reflect" "strings" "testing" + "gotest.tools/assert" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" @@ -14,8 +14,8 @@ import ( "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" ) -func testBuildBasicRLP(name string, kind gatewayapiv1.Kind) *RateLimitPolicy { - return &RateLimitPolicy{ +func testBuildBasicRLP(name string, kind gatewayapiv1.Kind, mutateFn func(*RateLimitPolicy)) *RateLimitPolicy { + p := &RateLimitPolicy{ TypeMeta: metav1.TypeMeta{ Kind: "RateLimitPolicy", APIVersion: GroupVersion.String(), @@ -32,14 +32,16 @@ func testBuildBasicRLP(name string, kind gatewayapiv1.Kind) *RateLimitPolicy { }, }, } -} -func testBuildBasicGatewayRLP(name string) *RateLimitPolicy { - return testBuildBasicRLP(name, gatewayapiv1.Kind("Gateway")) + if mutateFn != nil { + mutateFn(p) + } + + return p } -func testBuildBasicHTTPRouteRLP(name string) *RateLimitPolicy { - return testBuildBasicRLP(name, gatewayapiv1.Kind("HTTPRoute")) +func testBuildBasicHTTPRouteRLP(name string, mutateFn func(*RateLimitPolicy)) *RateLimitPolicy { + return testBuildBasicRLP(name, "HTTPRoute", mutateFn) } // TestRateLimitPolicyValidation calls rlp.Validate() @@ -47,17 +49,66 @@ func testBuildBasicHTTPRouteRLP(name string) *RateLimitPolicy { func TestRateLimitPolicyValidation(t *testing.T) { name := "httproute-a" - // Different namespace - rlp := testBuildBasicHTTPRouteRLP(name) - otherNS := gatewayapiv1.Namespace(rlp.GetNamespace() + "other") - rlp.Spec.TargetRef.Namespace = &otherNS - err := rlp.Validate() - if err == nil { - t.Fatal(`rlp.Validate() did not return error and should`) - } - if !strings.Contains(err.Error(), "invalid targetRef.Namespace") { - t.Fatalf(`rlp.Validate() did not return expected error. Instead: %v`, err) - } + t.Run("Invalid - Different namespace", func(subT *testing.T) { + rlp := testBuildBasicHTTPRouteRLP(name, func(policy *RateLimitPolicy) { + otherNS := gatewayapiv1.Namespace(policy.GetNamespace() + "other") + policy.Spec.TargetRef.Namespace = &otherNS + }) + err := rlp.Validate() + if err == nil { + subT.Fatal(`rlp.Validate() did not return error and should`) + } + if !strings.Contains(err.Error(), "invalid targetRef.Namespace") { + subT.Fatalf(`rlp.Validate() did not return expected error. Instead: %v`, err) + } + }) + + t.Run("Valid - no implicit or explicit defaults", func(subT *testing.T) { + rlp := testBuildBasicHTTPRouteRLP(name, nil) + if err := rlp.Validate(); err != nil { + subT.Fatalf(`rlp.Validate() did return error and should not: %v`, err) + } + }) + + t.Run("Valid - Implicit defaults only", func(subT *testing.T) { + rlp := testBuildBasicHTTPRouteRLP(name, func(policy *RateLimitPolicy) { + policy.Spec.Limits = map[string]Limit{ + "implicit": {Rates: []Rate{{Limit: 0}}}, + } + }) + if err := rlp.Validate(); err != nil { + subT.Fatalf(`rlp.Validate() did return error and should not: %v`, err) + } + }) + + t.Run("Valid - Explicit defaults only", func(subT *testing.T) { + rlp := testBuildBasicHTTPRouteRLP(name, func(policy *RateLimitPolicy) { + policy.Spec.Defaults.Limits = map[string]Limit{ + "explicit": {Rates: []Rate{{Limit: 1}}}, + } + }) + if err := rlp.Validate(); err != nil { + subT.Fatalf(`rlp.Validate() did return error and should not: %v`, err) + } + }) + + t.Run("Invalid - Implicit and explicit defaults ", func(subT *testing.T) { + rlp := testBuildBasicHTTPRouteRLP(name, func(policy *RateLimitPolicy) { + policy.Spec.Limits = map[string]Limit{ + "implicit": {Rates: []Rate{{Limit: 0}}}, + } + policy.Spec.Defaults.Limits = map[string]Limit{ + "explicit": {Rates: []Rate{{Limit: 1}}}, + } + }) + err := rlp.Validate() + if err == nil { + subT.Fatal(`rlp.Validate() did not return error and should`) + } + if !strings.Contains(err.Error(), "cannot use implicit defaults if explicit defaults are defined") { + subT.Fatalf(`rlp.Validate() did not return expected error. Instead: %v`, err) + } + }) } func TestRateLimitPolicyListGetItems(t *testing.T) { @@ -78,82 +129,41 @@ func TestRateLimitPolicyListGetItems(t *testing.T) { } func TestRateLimitPolicy_GetLimits(t *testing.T) { + const name = "policy" var ( defaultLimits = map[string]Limit{ "default": { - Rates: []Rate{ - { - Limit: 10, - Duration: 1, - Unit: "seconds", - }, - }, + Rates: []Rate{{Limit: 10, Duration: 1, Unit: "seconds"}}, }, } - implicitLimits = map[string]Limit{ "implicit": { - Rates: []Rate{ - { - Limit: 20, - Duration: 2, - Unit: "seconds", - }, - }, + Rates: []Rate{{Limit: 20, Duration: 2, Unit: "minutes"}}, }, } ) - type fields struct { - Spec RateLimitPolicySpec - } - tests := []struct { - name string - fields fields - want map[string]Limit - }{ - { - name: "No limits defined", - fields: fields{Spec: RateLimitPolicySpec{}}, - want: nil, - }, - { - name: "Defaults defined", - fields: fields{ - Spec: RateLimitPolicySpec{ - Defaults: CommonSpec{Limits: defaultLimits}, - }, - }, - want: defaultLimits, - }, - { - name: "implicit rules defined", - fields: fields{ - Spec: RateLimitPolicySpec{ - CommonSpec: CommonSpec{Limits: implicitLimits}, - }, - }, - want: implicitLimits, - }, - { - name: "default rules takes precedence over implicit rules", - fields: fields{ - Spec: RateLimitPolicySpec{ - CommonSpec: CommonSpec{Limits: implicitLimits}, - Defaults: CommonSpec{Limits: defaultLimits}, - }, - }, - want: defaultLimits, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - r := &RateLimitPolicy{ - Spec: tt.fields.Spec, - } - if got := r.GetLimits(); !reflect.DeepEqual(got, tt.want) { - t.Errorf("GetLimits() = %v, want %v", got, tt.want) - } + t.Run("No limits defined", func(subT *testing.T) { + r := testBuildBasicHTTPRouteRLP(name, nil) + assert.DeepEqual(subT, r.GetLimits(), map[string]Limit(nil)) + }) + t.Run("Defaults defined", func(subT *testing.T) { + r := testBuildBasicHTTPRouteRLP(name, func(policy *RateLimitPolicy) { + policy.Spec.Defaults.Limits = defaultLimits }) - } + assert.DeepEqual(subT, r.GetLimits(), defaultLimits) + }) + t.Run("Implicit rules defined", func(subT *testing.T) { + r := testBuildBasicHTTPRouteRLP(name, func(policy *RateLimitPolicy) { + policy.Spec.Limits = implicitLimits + }) + assert.DeepEqual(subT, r.GetLimits(), implicitLimits) + }) + t.Run("Default rules takes precedence over implicit rules if validation is somehow bypassed", func(subT *testing.T) { + r := testBuildBasicHTTPRouteRLP(name, func(policy *RateLimitPolicy) { + policy.Spec.Defaults.Limits = defaultLimits + policy.Spec.Limits = implicitLimits + }) + assert.DeepEqual(subT, r.GetLimits(), defaultLimits) + }) } From 4a3195d8afca2eb7246b3979f5d46cce937cf417 Mon Sep 17 00:00:00 2001 From: KevFan Date: Wed, 13 Mar 2024 11:49:15 +0000 Subject: [PATCH 04/12] refactor: CEL for validation of RLP implicit and explicit default mutual exclusivity --- api/v1beta2/ratelimitpolicy_types.go | 12 +---- api/v1beta2/ratelimitpolicy_types_test.go | 47 ------------------- .../kuadrant.io_ratelimitpolicies.yaml | 2 + .../bases/kuadrant.io_ratelimitpolicies.yaml | 2 + .../ratelimitpolicy_controller_test.go | 42 +++++++++++++++++ pkg/rlptools/utils.go | 3 +- pkg/rlptools/utils_test.go | 3 +- 7 files changed, 50 insertions(+), 61 deletions(-) diff --git a/api/v1beta2/ratelimitpolicy_types.go b/api/v1beta2/ratelimitpolicy_types.go index bfcacfcf9..be55c634a 100644 --- a/api/v1beta2/ratelimitpolicy_types.go +++ b/api/v1beta2/ratelimitpolicy_types.go @@ -121,6 +121,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.limits) && has(self.limits))",message="Implicit and explicit defaults are mutually exclusive" 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'" @@ -129,7 +130,7 @@ type RateLimitPolicySpec struct { // Defaults define explicit default values for this policy and for policies inheriting this policy // +optional - Defaults CommonSpec `json:"defaults"` + Defaults CommonSpec `json:"defaults,omitempty"` // Implicit default CommonSpec `json:""` @@ -143,11 +144,6 @@ type CommonSpec struct { Limits map[string]Limit `json:"limits,omitempty"` } -// IsUsed determines if any fields within CommonSpec is used -func (c CommonSpec) IsUsed() bool { - return c.Limits != nil -} - // RateLimitPolicyStatus defines the observed state of RateLimitPolicy type RateLimitPolicyStatus struct { // ObservedGeneration reflects the generation of the most recently observed spec. @@ -212,10 +208,6 @@ func (r *RateLimitPolicy) Validate() error { return fmt.Errorf("invalid targetRef.Namespace %s. Currently only supporting references to the same namespace", *r.Spec.TargetRef.Namespace) } - if r.Spec.Defaults.IsUsed() && r.Spec.CommonSpec.IsUsed() { - return fmt.Errorf("cannot use implicit defaults if explicit defaults are defined") - } - return nil } diff --git a/api/v1beta2/ratelimitpolicy_types_test.go b/api/v1beta2/ratelimitpolicy_types_test.go index 95c37f501..5bd71ce57 100644 --- a/api/v1beta2/ratelimitpolicy_types_test.go +++ b/api/v1beta2/ratelimitpolicy_types_test.go @@ -62,53 +62,6 @@ func TestRateLimitPolicyValidation(t *testing.T) { subT.Fatalf(`rlp.Validate() did not return expected error. Instead: %v`, err) } }) - - t.Run("Valid - no implicit or explicit defaults", func(subT *testing.T) { - rlp := testBuildBasicHTTPRouteRLP(name, nil) - if err := rlp.Validate(); err != nil { - subT.Fatalf(`rlp.Validate() did return error and should not: %v`, err) - } - }) - - t.Run("Valid - Implicit defaults only", func(subT *testing.T) { - rlp := testBuildBasicHTTPRouteRLP(name, func(policy *RateLimitPolicy) { - policy.Spec.Limits = map[string]Limit{ - "implicit": {Rates: []Rate{{Limit: 0}}}, - } - }) - if err := rlp.Validate(); err != nil { - subT.Fatalf(`rlp.Validate() did return error and should not: %v`, err) - } - }) - - t.Run("Valid - Explicit defaults only", func(subT *testing.T) { - rlp := testBuildBasicHTTPRouteRLP(name, func(policy *RateLimitPolicy) { - policy.Spec.Defaults.Limits = map[string]Limit{ - "explicit": {Rates: []Rate{{Limit: 1}}}, - } - }) - if err := rlp.Validate(); err != nil { - subT.Fatalf(`rlp.Validate() did return error and should not: %v`, err) - } - }) - - t.Run("Invalid - Implicit and explicit defaults ", func(subT *testing.T) { - rlp := testBuildBasicHTTPRouteRLP(name, func(policy *RateLimitPolicy) { - policy.Spec.Limits = map[string]Limit{ - "implicit": {Rates: []Rate{{Limit: 0}}}, - } - policy.Spec.Defaults.Limits = map[string]Limit{ - "explicit": {Rates: []Rate{{Limit: 1}}}, - } - }) - err := rlp.Validate() - if err == nil { - subT.Fatal(`rlp.Validate() did not return error and should`) - } - if !strings.Contains(err.Error(), "cannot use implicit defaults if explicit defaults are defined") { - subT.Fatalf(`rlp.Validate() did not return expected error. Instead: %v`, err) - } - }) } func TestRateLimitPolicyListGetItems(t *testing.T) { diff --git a/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml b/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml index 84fc07ac2..fa1345dfd 100644 --- a/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml +++ b/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml @@ -868,6 +868,8 @@ spec: - message: route selectors not supported when targeting a Gateway rule: self.targetRef.kind != 'Gateway' || !has(self.limits) || !self.limits.exists(x, has(self.limits[x].routeSelectors)) + - message: Implicit and explicit defaults are mutually exclusive + rule: '!(has(self.defaults.limits) && 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 e58c4dd2b..89507ac0f 100644 --- a/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml +++ b/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml @@ -867,6 +867,8 @@ spec: - message: route selectors not supported when targeting a Gateway rule: self.targetRef.kind != 'Gateway' || !has(self.limits) || !self.limits.exists(x, has(self.limits[x].routeSelectors)) + - message: Implicit and explicit defaults are mutually exclusive + rule: '!(has(self.defaults.limits) && has(self.limits))' status: description: RateLimitPolicyStatus defines the observed state of RateLimitPolicy properties: diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index 905a102d1..82570cea1 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -321,6 +321,48 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() { Expect(err).To(Not(BeNil())) Expect(strings.Contains(err.Error(), "Invalid targetRef.kind. The only supported values are 'HTTPRoute' and 'Gateway'")).To(BeTrue()) }) + + It("Valid only implicit defaults", func() { + policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Spec.Limits = map[string]kuadrantv1beta2.Limit{ + "implicit": { + Rates: []kuadrantv1beta2.Rate{{Limit: 2, Duration: 20, Unit: "second"}}, + }, + } + }) + err := k8sClient.Create(context.Background(), policy) + Expect(err).To(BeNil()) + }) + + It("Valid only explicit defaults", func() { + policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Spec.Defaults.Limits = map[string]kuadrantv1beta2.Limit{ + "explicit": { + Rates: []kuadrantv1beta2.Rate{{Limit: 1, Duration: 10, Unit: "second"}}, + }, + } + }) + err := k8sClient.Create(context.Background(), policy) + Expect(err).To(BeNil()) + }) + + It("Invalid implicit and explicit defaults are mutually exclusive", func() { + policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Spec.Defaults.Limits = map[string]kuadrantv1beta2.Limit{ + "explicit": { + Rates: []kuadrantv1beta2.Rate{{Limit: 1, Duration: 10, Unit: "second"}}, + }, + } + policy.Spec.Limits = map[string]kuadrantv1beta2.Limit{ + "implicit": { + Rates: []kuadrantv1beta2.Rate{{Limit: 2, Duration: 20, Unit: "second"}}, + }, + } + }) + err := k8sClient.Create(context.Background(), policy) + Expect(err).To(Not(BeNil())) + Expect(strings.Contains(err.Error(), "Implicit and explicit defaults are mutually exclusive")).To(BeTrue()) + }) }) Context("Route Selector Validation", func() { diff --git a/pkg/rlptools/utils.go b/pkg/rlptools/utils.go index 56f9cb83e..9e1433f20 100644 --- a/pkg/rlptools/utils.go +++ b/pkg/rlptools/utils.go @@ -8,9 +8,8 @@ import ( limitadorv1alpha1 "github.com/kuadrant/limitador-operator/api/v1alpha1" - "github.com/kuadrant/kuadrant-operator/pkg/library/utils" - kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" + "github.com/kuadrant/kuadrant-operator/pkg/library/utils" ) const ( diff --git a/pkg/rlptools/utils_test.go b/pkg/rlptools/utils_test.go index e3077b789..6199649ec 100644 --- a/pkg/rlptools/utils_test.go +++ b/pkg/rlptools/utils_test.go @@ -10,9 +10,8 @@ import ( limitadorv1alpha1 "github.com/kuadrant/limitador-operator/api/v1alpha1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "github.com/kuadrant/kuadrant-operator/pkg/library/utils" - kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" + "github.com/kuadrant/kuadrant-operator/pkg/library/utils" ) func testRLP_1Limit_1Rate(ns, name string) *kuadrantv1beta2.RateLimitPolicy { From 2e9faca1621385b1d9e801943faebc46e872ea97 Mon Sep 17 00:00:00 2001 From: KevFan Date: Thu, 14 Mar 2024 13:08:07 +0000 Subject: [PATCH 05/12] refactor: use default limits field for rlp controller tests --- api/v1beta2/ratelimitpolicy_types.go | 8 +- .../kuadrant.io_ratelimitpolicies.yaml | 292 ++++++++++-------- .../bases/kuadrant.io_ratelimitpolicies.yaml | 292 ++++++++++-------- .../ratelimitpolicy_controller_test.go | 14 +- 4 files changed, 331 insertions(+), 275 deletions(-) diff --git a/api/v1beta2/ratelimitpolicy_types.go b/api/v1beta2/ratelimitpolicy_types.go index be55c634a..ec674ae9f 100644 --- a/api/v1beta2/ratelimitpolicy_types.go +++ b/api/v1beta2/ratelimitpolicy_types.go @@ -128,15 +128,17 @@ type RateLimitPolicySpec struct { // +kubebuilder:validation:XValidation:rule="self.kind == 'HTTPRoute' || self.kind == 'Gateway'",message="Invalid targetRef.kind. The only supported values are 'HTTPRoute' and 'Gateway'" TargetRef gatewayapiv1alpha2.PolicyTargetReference `json:"targetRef"` - // Defaults define explicit default values for this policy and for policies inheriting this policy + // Defaults define explicit default values for this policy and for policies inheriting this policy. + // Defaults are mutually exclusive with implicit defaults defined by CommonSpec. // +optional Defaults CommonSpec `json:"defaults,omitempty"` - // Implicit default + // CommonSpec defines implicit default values for this policy and for policies inheriting this policy. + // CommonSpec is mutually exclusive with explicit defaults defined by Defaults. CommonSpec `json:""` } -// CommonSpec contains fields +// CommonSpec contains common shared fields. type CommonSpec struct { // Limits holds the struct of limits indexed by a unique name // +optional diff --git a/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml b/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml index fa1345dfd..d4cf856fd 100644 --- a/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml +++ b/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml @@ -68,23 +68,25 @@ spec: description: RateLimitPolicySpec defines the desired state of RateLimitPolicy properties: defaults: - description: Defaults define explicit default values for this policy - and for policies inheriting this policy + description: |- + Defaults define explicit default values for this policy and for policies inheriting this policy. + Defaults are mutually exclusive with implicit defaults defined by CommonSpec. 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 + 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 + 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' + Example: "request.path" -> The path portion of the URL maxLength: 253 minLength: 1 type: string @@ -104,8 +106,9 @@ spec: a given period of time type: integer unit: - description: 'Duration defines the time uni Possible - values are: "second", "minute", "hour", "day"' + description: |- + Duration defines the time uni + Possible values are: "second", "minute", "hour", "day" enum: - second - minute @@ -122,92 +125,102 @@ spec: 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 + 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 + 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: - \n 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. - \n 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`). \n 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." + 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 + 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 action. Multiple + used to match requests to a given\naction. Multiple match types are ANDed together, i.e. the match - will evaluate to true only if all conditions are - satisfied. \n For example, the match below will - match a HTTP request only if its path starts with - `/foo` AND it contains the `version: v1` header: - \n ``` match: \n path: value: \"/foo\" headers: - - name: \"version\" value \"v1\" \n ```" + 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. + 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. + 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). - \n 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. \n 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\"." + 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. \n - Support: Core (Exact) \n Support: Implementation-specific - (RegularExpression) \n 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." + 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 @@ -228,10 +241,13 @@ spec: - 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. - \n Support: Extended" + 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 @@ -247,16 +263,20 @@ spec: 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. + 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. \n Support: Core - (Exact, PathPrefix) \n Support: Implementation-specific - (RegularExpression)" + description: |- + Type specifies how to match against the path Value. + + + Support: Core (Exact, PathPrefix) + + + Support: Implementation-specific (RegularExpression) enum: - Exact - PathPrefix @@ -318,55 +338,60 @@ spec: ? 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. \n Support: Extended" + 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 + 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 + 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). - \n 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. \n 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. \n Users SHOULD - NOT route traffic based on repeated - query params to guard themselves against - potential differences in the implementations." + + + 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. - \n Support: Extended (Exact) \n Support: - Implementation-specific (RegularExpression) - \n 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." + 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 @@ -393,17 +418,18 @@ spec: 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 + 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 + 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)' + 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 @@ -414,9 +440,9 @@ spec: - 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 + 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 diff --git a/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml b/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml index 89507ac0f..1b47636d0 100644 --- a/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml +++ b/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml @@ -67,23 +67,25 @@ spec: description: RateLimitPolicySpec defines the desired state of RateLimitPolicy properties: defaults: - description: Defaults define explicit default values for this policy - and for policies inheriting this policy + description: |- + Defaults define explicit default values for this policy and for policies inheriting this policy. + Defaults are mutually exclusive with implicit defaults defined by CommonSpec. 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 + 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 + 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' + Example: "request.path" -> The path portion of the URL maxLength: 253 minLength: 1 type: string @@ -103,8 +105,9 @@ spec: a given period of time type: integer unit: - description: 'Duration defines the time uni Possible - values are: "second", "minute", "hour", "day"' + description: |- + Duration defines the time uni + Possible values are: "second", "minute", "hour", "day" enum: - second - minute @@ -121,92 +124,102 @@ spec: 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 + 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 + 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: - \n 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. - \n 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`). \n 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." + 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 + 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 action. Multiple + used to match requests to a given\naction. Multiple match types are ANDed together, i.e. the match - will evaluate to true only if all conditions are - satisfied. \n For example, the match below will - match a HTTP request only if its path starts with - `/foo` AND it contains the `version: v1` header: - \n ``` match: \n path: value: \"/foo\" headers: - - name: \"version\" value \"v1\" \n ```" + 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. + 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. + 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). - \n 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. \n 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\"." + 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. \n - Support: Core (Exact) \n Support: Implementation-specific - (RegularExpression) \n 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." + 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 @@ -227,10 +240,13 @@ spec: - 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. - \n Support: Extended" + 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 @@ -246,16 +262,20 @@ spec: 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. + 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. \n Support: Core - (Exact, PathPrefix) \n Support: Implementation-specific - (RegularExpression)" + description: |- + Type specifies how to match against the path Value. + + + Support: Core (Exact, PathPrefix) + + + Support: Implementation-specific (RegularExpression) enum: - Exact - PathPrefix @@ -317,55 +337,60 @@ spec: ? 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. \n Support: Extended" + 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 + 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 + 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). - \n 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. \n 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. \n Users SHOULD - NOT route traffic based on repeated - query params to guard themselves against - potential differences in the implementations." + + + 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. - \n Support: Extended (Exact) \n Support: - Implementation-specific (RegularExpression) - \n 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." + 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 @@ -392,17 +417,18 @@ spec: 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 + 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 + 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)' + 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 @@ -413,9 +439,9 @@ spec: - 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 + 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 diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index 82570cea1..6cf395ac7 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -47,7 +47,7 @@ var _ = Describe("RateLimitPolicy controller", func() { Kind: "HTTPRoute", Name: gatewayapiv1.ObjectName(routeName), }, - CommonSpec: kuadrantv1beta2.CommonSpec{ + Defaults: kuadrantv1beta2.CommonSpec{ Limits: map[string]kuadrantv1beta2.Limit{ "l1": { Rates: []kuadrantv1beta2.Rate{ @@ -158,11 +158,13 @@ var _ = Describe("RateLimitPolicy controller", func() { Kind: "Gateway", Name: gatewayapiv1.ObjectName(gwName), }, - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + Defaults: kuadrantv1beta2.CommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, }, }, }, From 3bd517b2fa7f87211e7688cd5ff1a470f4869c01 Mon Sep 17 00:00:00 2001 From: KevFan Date: Fri, 15 Mar 2024 10:42:55 +0000 Subject: [PATCH 06/12] docs: update rlp reference --- doc/reference/ratelimitpolicy.md | 58 ++++++++++++++++++-------------- 1 file changed, 33 insertions(+), 25 deletions(-) diff --git a/doc/reference/ratelimitpolicy.md b/doc/reference/ratelimitpolicy.md index 711b08569..5a346f788 100644 --- a/doc/reference/ratelimitpolicy.md +++ b/doc/reference/ratelimitpolicy.md @@ -2,50 +2,58 @@ - [RateLimitPolicy](#ratelimitpolicy) - [RateLimitPolicySpec](#ratelimitpolicyspec) - - [Limit](#limit) - - [RateLimit](#ratelimit) - - [WhenCondition](#whencondition) + - [Defaults](#defaults) + - [Limit](#limit) + - [RateLimit](#ratelimit) + - [WhenCondition](#whencondition) - [RateLimitPolicyStatus](#ratelimitpolicystatus) - - [ConditionSpec](#conditionspec) + - [ConditionSpec](#conditionspec) ## RateLimitPolicy -| **Field** | **Type** | **Required** | **Description** | -|-----------|-------------------------------------------------|:------------:|------------------------------------------------------| -| `spec` | [RateLimitPolicySpec](#ratelimitpolicyspec) | Yes | The specfication for RateLimitPolicy custom resource | -| `status` | [RateLimitPolicyStatus](#ratelimitpolicystatus) | No | The status for the custom resource | +| **Field** | **Type** | **Required** | **Description** | +|-----------|-------------------------------------------------|:------------:|-------------------------------------------------------| +| `spec` | [RateLimitPolicySpec](#ratelimitpolicyspec) | Yes | The specification for RateLimitPolicy custom resource | +| `status` | [RateLimitPolicyStatus](#ratelimitpolicystatus) | No | The status for the custom resource | ## 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 Kuberentes resource that the policy attaches to | -| `limits` | Map | No | Limit definitions | +| **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` | Map | No | Explicit Limit definitions. This field is mutually exclusive with [Defaults](#defaults) `limits` field | +| `limits` | [Defaults](#defaults) | No | Implicit default definitions | + +### Defaults + +| **Field** | **Type** | **Required** | **Description** | +|-----------|------------------------------|--------------|------------------------------------------------------------------------------------------------------------------------------| +| `limits` | Map | No | Explicit Limit definitions. This field is mutually exclusive with [RateLimitPolicySpec](#ratelimitpolicyspec) `limits` field | ### Limit | **Field** | **Type** | **Required** | **Description** | |------------------|-----------------------------------------------------|:------------:|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| `rates` | [][RateLimit](#ratelimit) | No | List of rate limits associated with the limit definition | -| `counters` | []String | No | List of rate limit counter qualifiers. Items must be a valid [Well-known attribute](https://github.com/Kuadrant/architecture/blob/main/rfcs/0002-well-known-attributes.md). Each distinct value resolved in the data plane starts a separate counter for each rate limit. | -| `routeSelectors` | [][RouteSelector](route-selectors.md#routeselector) | No | List of selectors of HTTPRouteRules whose matching rules activate the limit. At least one HTTPRouteRule must be selected to activate the limit. If omitted, all HTTPRouteRules of the targeted HTTPRoute activate the limit. Do not use it in policies targeting a Gateway. | -| `when` | [][WhenCondition](#whencondition) | No | List of additional dynamic conditions (expressions) to activate the limit. All expression must evaluate to true for the limit to be applied. Use it for filtering attributes that cannot be expressed in the targeted HTTPRoute's `spec.hostnames` and `spec.rules.matches` fields, or when targeting a Gateway. | +| `rates` | [][RateLimit](#ratelimit) | No | List of rate limits associated with the limit definition | +| `counters` | []String | No | List of rate limit counter qualifiers. Items must be a valid [Well-known attribute](https://github.com/Kuadrant/architecture/blob/main/rfcs/0002-well-known-attributes.md). Each distinct value resolved in the data plane starts a separate counter for each rate limit. | +| `routeSelectors` | [][RouteSelector](route-selectors.md#routeselector) | No | List of selectors of HTTPRouteRules whose matching rules activate the limit. At least one HTTPRouteRule must be selected to activate the limit. If omitted, all HTTPRouteRules of the targeted HTTPRoute activate the limit. Do not use it in policies targeting a Gateway. | +| `when` | [][WhenCondition](#whencondition) | No | List of additional dynamic conditions (expressions) to activate the limit. All expression must evaluate to true for the limit to be applied. Use it for filtering attributes that cannot be expressed in the targeted HTTPRoute's `spec.hostnames` and `spec.rules.matches` fields, or when targeting a Gateway. | #### RateLimit -| **Field** | **Type** | **Required** | **Description** | -|------------------|----------|:------------:|----------------------------------------------------------------------------------------| -| `limit` | Number | Yes | Maximum value allowed within the given period of time (duration) | -| `duration` | Number | Yes | The period of time in the specified unit that the limit applies | -| `unit` | String | Yes | Unit of time for the duration of the limit. One-of: "second", "minute", "hour", "day". | +| **Field** | **Type** | **Required** | **Description** | +|------------|----------|:------------:|----------------------------------------------------------------------------------------| +| `limit` | Number | Yes | Maximum value allowed within the given period of time (duration) | +| `duration` | Number | Yes | The period of time in the specified unit that the limit applies | +| `unit` | String | Yes | Unit of time for the duration of the limit. One-of: "second", "minute", "hour", "day". | #### WhenCondition | **Field** | **Type** | **Required** | **Description** | |------------|----------|:------------:|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| `selector` | String | Yes | A valid [Well-known attribute](https://github.com/Kuadrant/architecture/blob/main/rfcs/0002-well-known-attributes.md) whose resolved value in the data plane will be compared to `value`, using the `operator`. | -| `operator` | String | Yes | The binary operator to be applied to the resolved value specified by the selector. One-of: "eq" (equal to), "neq" (not equal to) | -| `value` | String | Yes | The static value to be compared to the one resolved from the selector. | +| `selector` | String | Yes | A valid [Well-known attribute](https://github.com/Kuadrant/architecture/blob/main/rfcs/0002-well-known-attributes.md) whose resolved value in the data plane will be compared to `value`, using the `operator`. | +| `operator` | String | Yes | The binary operator to be applied to the resolved value specified by the selector. One-of: "eq" (equal to), "neq" (not equal to) | +| `value` | String | Yes | The static value to be compared to the one resolved from the selector. | ## RateLimitPolicyStatus @@ -61,7 +69,7 @@ * The *reason* field is a unique, one-word, CamelCase reason for the condition’s last transition. * The *status* field is a string, with possible values **True**, **False**, and **Unknown**. * The *type* field is a string with the following possible values: - * Available: the resource has successfully configured; + * Available: the resource has successfully configured; | **Field** | **Type** | **Description** | |----------------------|-----------|------------------------------| From b19e1dbc8bbc5bee21d3dd1e5b9761a9efab08cf Mon Sep 17 00:00:00 2001 From: KevFan Date: Mon, 25 Mar 2024 12:17:18 +0000 Subject: [PATCH 07/12] refactor: common spec validation --- api/v1beta2/ratelimitpolicy_types.go | 29 ++-- api/v1beta2/ratelimitpolicy_types_test.go | 8 +- api/v1beta2/zz_generated.deepcopy.go | 52 ++++---- .../kuadrant.io_ratelimitpolicies.yaml | 4 +- .../bases/kuadrant.io_ratelimitpolicies.yaml | 4 +- ...dor_cluster_envoyfilter_controller_test.go | 2 +- .../rate_limiting_wasmplugin_controller.go | 2 +- .../ratelimitpolicy_controller_test.go | 124 +++++++++--------- pkg/rlptools/utils_test.go | 8 +- pkg/rlptools/wasm_utils_test.go | 2 +- 10 files changed, 123 insertions(+), 112 deletions(-) diff --git a/api/v1beta2/ratelimitpolicy_types.go b/api/v1beta2/ratelimitpolicy_types.go index ec674ae9f..f752ddcf2 100644 --- a/api/v1beta2/ratelimitpolicy_types.go +++ b/api/v1beta2/ratelimitpolicy_types.go @@ -121,7 +121,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.limits) && has(self.limits))",message="Implicit and explicit defaults are mutually exclusive" +// +kubebuilder:validation:XValidation:rule="!(has(self.defaults) && has(self.limits))",message="Implicit and explicit defaults are mutually exclusive" 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'" @@ -129,17 +129,17 @@ type RateLimitPolicySpec struct { TargetRef gatewayapiv1alpha2.PolicyTargetReference `json:"targetRef"` // Defaults define explicit default values for this policy and for policies inheriting this policy. - // Defaults are mutually exclusive with implicit defaults defined by CommonSpec. + // Defaults are mutually exclusive with implicit defaults defined by RateLimitPolicyCommonSpec. // +optional - Defaults CommonSpec `json:"defaults,omitempty"` + Defaults *RateLimitPolicyCommonSpec `json:"defaults,omitempty"` - // CommonSpec defines implicit default values for this policy and for policies inheriting this policy. - // CommonSpec is mutually exclusive with explicit defaults defined by Defaults. - CommonSpec `json:""` + // 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:""` } -// CommonSpec contains common shared fields. -type CommonSpec struct { +// RateLimitPolicyCommonSpec contains common shared fields. +type RateLimitPolicyCommonSpec struct { // Limits holds the struct of limits indexed by a unique name // +optional // +kubebuilder:validation:MaxProperties=14 @@ -277,13 +277,18 @@ func (r *RateLimitPolicy) DirectReferenceAnnotationName() string { return RateLimitPolicyDirectReferenceAnnotationName } +func (r *RateLimitPolicy) GetRateLimitPolicyCommonSpec() *RateLimitPolicyCommonSpec { + if r.Spec.Defaults != nil { + return r.Spec.Defaults + } + + return &r.Spec.RateLimitPolicyCommonSpec +} + // GetLimits returns the limits based on whether default or implicit rules are defined. // Default rules takes precedence func (r *RateLimitPolicy) GetLimits() map[string]Limit { - if r.Spec.Defaults.Limits != nil { - return r.Spec.Defaults.Limits - } - return r.Spec.Limits + return r.GetRateLimitPolicyCommonSpec().Limits } func init() { diff --git a/api/v1beta2/ratelimitpolicy_types_test.go b/api/v1beta2/ratelimitpolicy_types_test.go index 5bd71ce57..73f101988 100644 --- a/api/v1beta2/ratelimitpolicy_types_test.go +++ b/api/v1beta2/ratelimitpolicy_types_test.go @@ -102,7 +102,9 @@ func TestRateLimitPolicy_GetLimits(t *testing.T) { }) t.Run("Defaults defined", func(subT *testing.T) { r := testBuildBasicHTTPRouteRLP(name, func(policy *RateLimitPolicy) { - policy.Spec.Defaults.Limits = defaultLimits + policy.Spec.Defaults = &RateLimitPolicyCommonSpec{ + Limits: defaultLimits, + } }) assert.DeepEqual(subT, r.GetLimits(), defaultLimits) }) @@ -114,7 +116,9 @@ func TestRateLimitPolicy_GetLimits(t *testing.T) { }) t.Run("Default rules takes precedence over implicit rules if validation is somehow bypassed", func(subT *testing.T) { r := testBuildBasicHTTPRouteRLP(name, func(policy *RateLimitPolicy) { - policy.Spec.Defaults.Limits = defaultLimits + policy.Spec.Defaults = &RateLimitPolicyCommonSpec{ + Limits: defaultLimits, + } policy.Spec.Limits = implicitLimits }) assert.DeepEqual(subT, r.GetLimits(), defaultLimits) diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go index bde5f2446..005e27d77 100644 --- a/api/v1beta2/zz_generated.deepcopy.go +++ b/api/v1beta2/zz_generated.deepcopy.go @@ -301,28 +301,6 @@ func (in *CommonAuthRuleSpec) DeepCopy() *CommonAuthRuleSpec { return out } -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *CommonSpec) DeepCopyInto(out *CommonSpec) { - *out = *in - if in.Limits != nil { - in, out := &in.Limits, &out.Limits - *out = make(map[string]Limit, len(*in)) - for key, val := range *in { - (*out)[key] = *val.DeepCopy() - } - } -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CommonSpec. -func (in *CommonSpec) DeepCopy() *CommonSpec { - if in == nil { - return nil - } - out := new(CommonSpec) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *HeaderSuccessResponseSpec) DeepCopyInto(out *HeaderSuccessResponseSpec) { *out = *in @@ -435,6 +413,28 @@ func (in *RateLimitPolicy) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *RateLimitPolicyCommonSpec) DeepCopyInto(out *RateLimitPolicyCommonSpec) { + *out = *in + if in.Limits != nil { + in, out := &in.Limits, &out.Limits + *out = make(map[string]Limit, len(*in)) + for key, val := range *in { + (*out)[key] = *val.DeepCopy() + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RateLimitPolicyCommonSpec. +func (in *RateLimitPolicyCommonSpec) DeepCopy() *RateLimitPolicyCommonSpec { + if in == nil { + return nil + } + out := new(RateLimitPolicyCommonSpec) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *RateLimitPolicyList) DeepCopyInto(out *RateLimitPolicyList) { *out = *in @@ -471,8 +471,12 @@ func (in *RateLimitPolicyList) DeepCopyObject() runtime.Object { func (in *RateLimitPolicySpec) DeepCopyInto(out *RateLimitPolicySpec) { *out = *in in.TargetRef.DeepCopyInto(&out.TargetRef) - in.Defaults.DeepCopyInto(&out.Defaults) - in.CommonSpec.DeepCopyInto(&out.CommonSpec) + if in.Defaults != nil { + in, out := &in.Defaults, &out.Defaults + *out = new(RateLimitPolicyCommonSpec) + (*in).DeepCopyInto(*out) + } + in.RateLimitPolicyCommonSpec.DeepCopyInto(&out.RateLimitPolicyCommonSpec) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RateLimitPolicySpec. diff --git a/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml b/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml index d4cf856fd..eef7b092a 100644 --- a/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml +++ b/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml @@ -70,7 +70,7 @@ spec: defaults: description: |- Defaults define explicit default values for this policy and for policies inheriting this policy. - Defaults are mutually exclusive with implicit defaults defined by CommonSpec. + Defaults are mutually exclusive with implicit defaults defined by RateLimitPolicyCommonSpec. properties: limits: additionalProperties: @@ -895,7 +895,7 @@ spec: rule: self.targetRef.kind != 'Gateway' || !has(self.limits) || !self.limits.exists(x, has(self.limits[x].routeSelectors)) - message: Implicit and explicit defaults are mutually exclusive - rule: '!(has(self.defaults.limits) && has(self.limits))' + rule: '!(has(self.defaults) && 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 1b47636d0..1a804d40b 100644 --- a/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml +++ b/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml @@ -69,7 +69,7 @@ spec: defaults: description: |- Defaults define explicit default values for this policy and for policies inheriting this policy. - Defaults are mutually exclusive with implicit defaults defined by CommonSpec. + Defaults are mutually exclusive with implicit defaults defined by RateLimitPolicyCommonSpec. properties: limits: additionalProperties: @@ -894,7 +894,7 @@ spec: rule: self.targetRef.kind != 'Gateway' || !has(self.limits) || !self.limits.exists(x, has(self.limits[x].routeSelectors)) - message: Implicit and explicit defaults are mutually exclusive - rule: '!(has(self.defaults.limits) && has(self.limits))' + rule: '!(has(self.defaults) && has(self.limits))' status: description: RateLimitPolicyStatus defines the observed state of RateLimitPolicy properties: diff --git a/controllers/limitador_cluster_envoyfilter_controller_test.go b/controllers/limitador_cluster_envoyfilter_controller_test.go index 8661a1393..29ce4e12f 100644 --- a/controllers/limitador_cluster_envoyfilter_controller_test.go +++ b/controllers/limitador_cluster_envoyfilter_controller_test.go @@ -90,7 +90,7 @@ var _ = Describe("Limitador Cluster EnvoyFilter controller", func() { Kind: "Gateway", Name: gatewayapiv1.ObjectName(gwName), }, - CommonSpec: kuadrantv1beta2.CommonSpec{ + RateLimitPolicyCommonSpec: kuadrantv1beta2.RateLimitPolicyCommonSpec{ Limits: map[string]kuadrantv1beta2.Limit{ "l1": { Rates: []kuadrantv1beta2.Rate{ diff --git a/controllers/rate_limiting_wasmplugin_controller.go b/controllers/rate_limiting_wasmplugin_controller.go index 2e6b2c7be..bbb2e37ec 100644 --- a/controllers/rate_limiting_wasmplugin_controller.go +++ b/controllers/rate_limiting_wasmplugin_controller.go @@ -221,7 +221,7 @@ func (r *RateLimitingWASMPluginReconciler) topologyIndexesFromGateway(ctx contex t, err := kuadrantgatewayapi.NewTopology( kuadrantgatewayapi.WithGateways([]*gatewayapiv1.Gateway{gw}), - kuadrantgatewayapi.WithRoutes(utils.Map(routeList.Items, ptr.To)), + kuadrantgatewayapi.WithRoutes(utils.Map(routeList.Items, ptr.To[gatewayapiv1.HTTPRoute])), kuadrantgatewayapi.WithPolicies(policies), kuadrantgatewayapi.WithLogger(logger), ) diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index 6cf395ac7..853318857 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -47,7 +47,7 @@ var _ = Describe("RateLimitPolicy controller", func() { Kind: "HTTPRoute", Name: gatewayapiv1.ObjectName(routeName), }, - Defaults: kuadrantv1beta2.CommonSpec{ + Defaults: &kuadrantv1beta2.RateLimitPolicyCommonSpec{ Limits: map[string]kuadrantv1beta2.Limit{ "l1": { Rates: []kuadrantv1beta2.Rate{ @@ -158,7 +158,7 @@ var _ = Describe("RateLimitPolicy controller", func() { Kind: "Gateway", Name: gatewayapiv1.ObjectName(gwName), }, - Defaults: kuadrantv1beta2.CommonSpec{ + Defaults: &kuadrantv1beta2.RateLimitPolicyCommonSpec{ Limits: map[string]kuadrantv1beta2.Limit{ "l1": { Rates: []kuadrantv1beta2.Rate{ @@ -271,27 +271,28 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() { AfterEach(DeleteNamespaceCallback(&testNamespace)) - Context("Spec TargetRef Validations", func() { - policyFactory := func(mutateFns ...func(policy *kuadrantv1beta2.RateLimitPolicy)) *kuadrantv1beta2.RateLimitPolicy { - policy := &kuadrantv1beta2.RateLimitPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "my-policy", - Namespace: testNamespace, - }, - Spec: kuadrantv1beta2.RateLimitPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: gatewayapiv1.GroupName, - Kind: "HTTPRoute", - Name: "my-target", - }, + policyFactory := func(mutateFns ...func(policy *kuadrantv1beta2.RateLimitPolicy)) *kuadrantv1beta2.RateLimitPolicy { + policy := &kuadrantv1beta2.RateLimitPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-policy", + Namespace: testNamespace, + }, + Spec: kuadrantv1beta2.RateLimitPolicySpec{ + TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ + Group: gatewayapiv1.GroupName, + Kind: "HTTPRoute", + Name: "my-target", }, - } - for _, mutateFn := range mutateFns { - mutateFn(policy) - } - - return policy + }, } + for _, mutateFn := range mutateFns { + mutateFn(policy) + } + + return policy + } + + Context("Spec TargetRef Validations", func() { It("Valid policy targeting HTTPRoute", func() { policy := policyFactory() err := k8sClient.Create(context.Background(), policy) @@ -323,8 +324,10 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() { Expect(err).To(Not(BeNil())) Expect(strings.Contains(err.Error(), "Invalid targetRef.kind. The only supported values are 'HTTPRoute' and 'Gateway'")).To(BeTrue()) }) + }) - It("Valid only implicit defaults", func() { + Context("Defaults validation", func() { + It("Valid only implicit defaults", func(ctx SpecContext) { policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { policy.Spec.Limits = map[string]kuadrantv1beta2.Limit{ "implicit": { @@ -332,27 +335,31 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() { }, } }) - err := k8sClient.Create(context.Background(), policy) + err := k8sClient.Create(ctx, policy) Expect(err).To(BeNil()) }) - It("Valid only explicit defaults", func() { + It("Valid only explicit defaults", func(ctx SpecContext) { policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { - policy.Spec.Defaults.Limits = map[string]kuadrantv1beta2.Limit{ - "explicit": { - Rates: []kuadrantv1beta2.Rate{{Limit: 1, Duration: 10, Unit: "second"}}, + policy.Spec.Defaults = &kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "explicit": { + Rates: []kuadrantv1beta2.Rate{{Limit: 1, Duration: 10, Unit: "second"}}, + }, }, } }) - err := k8sClient.Create(context.Background(), policy) + err := k8sClient.Create(ctx, policy) Expect(err).To(BeNil()) }) - It("Invalid implicit and explicit defaults are mutually exclusive", func() { + It("Invalid implicit and explicit defaults are mutually exclusive", func(ctx SpecContext) { policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { - policy.Spec.Defaults.Limits = map[string]kuadrantv1beta2.Limit{ - "explicit": { - Rates: []kuadrantv1beta2.Rate{{Limit: 1, Duration: 10, Unit: "second"}}, + policy.Spec.Defaults = &kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "explicit": { + Rates: []kuadrantv1beta2.Rate{{Limit: 1, Duration: 10, Unit: "second"}}, + }, }, } policy.Spec.Limits = map[string]kuadrantv1beta2.Limit{ @@ -361,7 +368,7 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() { }, } }) - err := k8sClient.Create(context.Background(), policy) + err := k8sClient.Create(ctx, policy) Expect(err).To(Not(BeNil())) Expect(strings.Contains(err.Error(), "Implicit and explicit defaults are mutually exclusive")).To(BeTrue()) }) @@ -372,34 +379,25 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() { gateWayRouteSelectorErrorMessage = "route selectors not supported when targeting a Gateway" ) - It("invalid usage of limit route selectors with a gateway targetRef", func() { - policy := &kuadrantv1beta2.RateLimitPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "my-policy", - Namespace: testNamespace, - }, - Spec: kuadrantv1beta2.RateLimitPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: gatewayapiv1.GroupName, - Kind: "Gateway", - Name: "my-gw", - }, - CommonSpec: kuadrantv1beta2.CommonSpec{ - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), - }, + It("invalid usage of limit route selectors with a gateway targetRef", func(ctx SpecContext) { + policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Spec.TargetRef.Kind = "Gateway" + policy.Spec.TargetRef.Name = "my-gw" + policy.Spec.RateLimitPolicyCommonSpec = kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), }, - RouteSelectors: []kuadrantv1beta2.RouteSelector{ - { - Hostnames: []gatewayapiv1.Hostname{"*.foo.io"}, - Matches: []gatewayapiv1.HTTPRouteMatch{ - { - Path: &gatewayapiv1.HTTPPathMatch{ - Value: ptr.To("/foo"), - }, + }, + RouteSelectors: []kuadrantv1beta2.RouteSelector{ + { + Hostnames: []gatewayapiv1.Hostname{"*.foo.io"}, + Matches: []gatewayapiv1.HTTPRouteMatch{ + { + Path: &gatewayapiv1.HTTPPathMatch{ + Value: ptr.To("/foo"), }, }, }, @@ -407,10 +405,10 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() { }, }, }, - }, - } + } + }) - err := k8sClient.Create(context.Background(), policy) + err := k8sClient.Create(ctx, policy) Expect(err).To(Not(BeNil())) Expect(strings.Contains(err.Error(), gateWayRouteSelectorErrorMessage)).To(BeTrue()) }) diff --git a/pkg/rlptools/utils_test.go b/pkg/rlptools/utils_test.go index 6199649ec..8b93e1e49 100644 --- a/pkg/rlptools/utils_test.go +++ b/pkg/rlptools/utils_test.go @@ -25,7 +25,7 @@ func testRLP_1Limit_1Rate(ns, name string) *kuadrantv1beta2.RateLimitPolicy { Namespace: ns, }, Spec: kuadrantv1beta2.RateLimitPolicySpec{ - CommonSpec: kuadrantv1beta2.CommonSpec{ + RateLimitPolicyCommonSpec: kuadrantv1beta2.RateLimitPolicyCommonSpec{ Limits: map[string]kuadrantv1beta2.Limit{ "l1": { Rates: []kuadrantv1beta2.Rate{ @@ -53,7 +53,7 @@ func testRLP_2Limits_1Rate(ns, name string) *kuadrantv1beta2.RateLimitPolicy { Namespace: ns, }, Spec: kuadrantv1beta2.RateLimitPolicySpec{ - CommonSpec: kuadrantv1beta2.CommonSpec{ + RateLimitPolicyCommonSpec: kuadrantv1beta2.RateLimitPolicyCommonSpec{ Limits: map[string]kuadrantv1beta2.Limit{ "l1": { Rates: []kuadrantv1beta2.Rate{ @@ -90,7 +90,7 @@ func testRLP_1Limit_2Rates(ns, name string) *kuadrantv1beta2.RateLimitPolicy { Namespace: ns, }, Spec: kuadrantv1beta2.RateLimitPolicySpec{ - CommonSpec: kuadrantv1beta2.CommonSpec{ + RateLimitPolicyCommonSpec: kuadrantv1beta2.RateLimitPolicyCommonSpec{ Limits: map[string]kuadrantv1beta2.Limit{ "l1": { Rates: []kuadrantv1beta2.Rate{ @@ -123,7 +123,7 @@ func testRLP_1Limit_1Rate_1Counter(ns, name string) *kuadrantv1beta2.RateLimitPo Namespace: ns, }, Spec: kuadrantv1beta2.RateLimitPolicySpec{ - CommonSpec: kuadrantv1beta2.CommonSpec{ + RateLimitPolicyCommonSpec: kuadrantv1beta2.RateLimitPolicyCommonSpec{ Limits: map[string]kuadrantv1beta2.Limit{ "l1": { Counters: []kuadrantv1beta2.ContextSelector{ diff --git a/pkg/rlptools/wasm_utils_test.go b/pkg/rlptools/wasm_utils_test.go index 7412a6ac3..31eb841b0 100644 --- a/pkg/rlptools/wasm_utils_test.go +++ b/pkg/rlptools/wasm_utils_test.go @@ -51,7 +51,7 @@ func TestWasmRules(t *testing.T) { Namespace: "my-app", }, Spec: kuadrantv1beta2.RateLimitPolicySpec{ - CommonSpec: kuadrantv1beta2.CommonSpec{ + RateLimitPolicyCommonSpec: kuadrantv1beta2.RateLimitPolicyCommonSpec{ Limits: limits, }, }, From d61e68efcf1075c9df1dde23f4bfe3aca11d890d Mon Sep 17 00:00:00 2001 From: KevFan Date: Tue, 26 Mar 2024 15:39:46 +0000 Subject: [PATCH 08/12] test: RLP atomic default integration test --- .github/workflows/test.yaml | 2 +- Makefile | 2 +- .../ratelimitpolicy_controller_test.go | 135 +++++++++++++++++- doc/development.md | 2 +- utils/kind-cluster.yaml | 2 +- 5 files changed, 138 insertions(+), 5 deletions(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index e26907066..d2739a08e 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -78,7 +78,7 @@ jobs: - name: Create k8s Kind Cluster uses: helm/kind-action@v1.2.0 with: - version: v0.20.0 + version: v0.22.0 config: utils/kind-cluster.yaml cluster_name: ${{ env.KIND_CLUSTER_NAME }} wait: 120s diff --git a/Makefile b/Makefile index 8b0f3c7d4..24704dab1 100644 --- a/Makefile +++ b/Makefile @@ -229,7 +229,7 @@ $(OPM): opm: $(OPM) ## Download opm locally if necessary. KIND = $(PROJECT_PATH)/bin/kind -KIND_VERSION = v0.20.0 +KIND_VERSION = v0.22.0 $(KIND): $(call go-install-tool,$(KIND),sigs.k8s.io/kind@$(KIND_VERSION)) diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index 853318857..a88522aeb 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -5,12 +5,14 @@ package controllers import ( "context" "encoding/json" + "fmt" "strings" "time" limitadorv1alpha1 "github.com/kuadrant/limitador-operator/api/v1alpha1" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" @@ -257,7 +259,138 @@ var _ = Describe("RateLimitPolicy controller", func() { serialized, err := json.Marshal(refs) Expect(err).ToNot(HaveOccurred()) Expect(existingGateway.GetAnnotations()).To(HaveKeyWithValue( - rlp.BackReferenceAnnotationName(), string(serialized))) + rlp.BackReferenceAnnotationName(), string(serialized))) }) + }) + + Context("RLP Defaults", func() { + It("HTTPRoute atomic default taking precedence over Gateway defaults", 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) + }) + 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.Defaults.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 + 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), + })) + + // 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(1*time.Minute)) + }) + + Context("RLP accepted condition reasons", func() { + assertAcceptedConditionFalse := func(rlp *kuadrantv1beta2.RateLimitPolicy, reason, message string) func() bool { + return func() bool { + rlpKey := client.ObjectKeyFromObject(rlp) + existingRLP := &kuadrantv1beta2.RateLimitPolicy{} + err := k8sClient.Get(context.Background(), rlpKey, existingRLP) + if err != nil { + return false + } + + 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 + } + } + + // Accepted reason is already tested generally by the existing tests + + It("Target not found reason", func() { + rlp := policyFactory() + err := k8sClient.Create(context.Background(), rlp) + Expect(err).ToNot(HaveOccurred()) + + Eventually(assertAcceptedConditionFalse(rlp, string(gatewayapiv1alpha2.PolicyReasonTargetNotFound), + fmt.Sprintf("RateLimitPolicy target %s was not found", routeName)), + time.Minute, 5*time.Second).Should(BeTrue()) + }) + + It("Conflict reason", func() { + 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()) + + rlp := policyFactory() + err = k8sClient.Create(context.Background(), rlp) + Expect(err).ToNot(HaveOccurred()) + + rlp2 := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Name = "conflicting-rlp" + }) + err = k8sClient.Create(context.Background(), rlp2) + Expect(err).ToNot(HaveOccurred()) + + Eventually(assertAcceptedConditionFalse(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()) + }) + + It("Validation reason", func() { + const targetRefName, targetRefNamespace = "istio-ingressgateway", "istio-system" + + rlp := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Spec.TargetRef.Kind = "Gateway" + policy.Spec.TargetRef.Name = targetRefName + policy.Spec.TargetRef.Namespace = ptr.To(gatewayapiv1.Namespace(targetRefNamespace)) + }) + err := k8sClient.Create(context.Background(), rlp) + Expect(err).ToNot(HaveOccurred()) + + Eventually(assertAcceptedConditionFalse(rlp, string(gatewayapiv1alpha2.PolicyReasonInvalid), + fmt.Sprintf("RateLimitPolicy target is invalid: invalid targetRef.Namespace %s. Currently only supporting references to the same namespace", targetRefNamespace)), + time.Minute, 5*time.Second).Should(BeTrue()) }) }) }) diff --git a/doc/development.md b/doc/development.md index 1e2f1dc35..6aa6f4a62 100644 --- a/doc/development.md +++ b/doc/development.md @@ -3,7 +3,7 @@ ## Technology stack required for development * [operator-sdk] version v1.28.1 -* [kind] version v0.20.0 +* [kind] version v0.22.0 * [git][git_tool] * [go] version 1.21+ * [kubernetes] version v1.19+ diff --git a/utils/kind-cluster.yaml b/utils/kind-cluster.yaml index 4697238ef..ba3d385d3 100644 --- a/utils/kind-cluster.yaml +++ b/utils/kind-cluster.yaml @@ -3,4 +3,4 @@ kind: Cluster apiVersion: kind.x-k8s.io/v1alpha4 nodes: - role: control-plane - image: kindest/node:v1.27.3 + image: kindest/node:v1.29.2 From 9b559f4a2217f776b7f569ee2a69f50e36941e36 Mon Sep 17 00:00:00 2001 From: KevFan Date: Tue, 2 Apr 2024 15:35:09 +0100 Subject: [PATCH 09/12] refactor: remove unneccessary getters --- api/v1beta2/ratelimitpolicy_types.go | 20 +- api/v1beta2/ratelimitpolicy_types_test.go | 8 +- ...ate_limiting_wasmplugin_controller_test.go | 280 ++++++++++-------- .../ratelimitpolicy_controller_test.go | 7 +- pkg/rlptools/utils.go | 2 +- pkg/rlptools/wasm_utils.go | 5 +- 6 files changed, 176 insertions(+), 146 deletions(-) diff --git a/api/v1beta2/ratelimitpolicy_types.go b/api/v1beta2/ratelimitpolicy_types.go index f752ddcf2..123fbfc24 100644 --- a/api/v1beta2/ratelimitpolicy_types.go +++ b/api/v1beta2/ratelimitpolicy_types.go @@ -250,7 +250,7 @@ func (r *RateLimitPolicy) GetWrappedNamespace() gatewayapiv1.Namespace { func (r *RateLimitPolicy) GetRulesHostnames() (ruleHosts []string) { ruleHosts = make([]string, 0) - for _, limit := range r.GetLimits() { + for _, limit := range r.Spec.CommonSpec().Limits { for _, routeSelector := range limit.RouteSelectors { convertHostnamesToString := func(gwHostnames []gatewayapiv1.Hostname) []string { hostnames := make([]string, 0, len(gwHostnames)) @@ -277,18 +277,16 @@ func (r *RateLimitPolicy) DirectReferenceAnnotationName() string { return RateLimitPolicyDirectReferenceAnnotationName } -func (r *RateLimitPolicy) GetRateLimitPolicyCommonSpec() *RateLimitPolicyCommonSpec { - if r.Spec.Defaults != nil { - return r.Spec.Defaults +// CommonSpec returns the Default RateLimitPolicyCommonSpec if it is defined. +// Otherwise, it returns the RateLimitPolicyCommonSpec from the spec. +// This function should be used instead of accessing the fields directly, so that either the explicit or implicit default +// is returned. +func (r *RateLimitPolicySpec) CommonSpec() *RateLimitPolicyCommonSpec { + if r.Defaults != nil { + return r.Defaults } - return &r.Spec.RateLimitPolicyCommonSpec -} - -// GetLimits returns the limits based on whether default or implicit rules are defined. -// Default rules takes precedence -func (r *RateLimitPolicy) GetLimits() map[string]Limit { - return r.GetRateLimitPolicyCommonSpec().Limits + return &r.RateLimitPolicyCommonSpec } func init() { diff --git a/api/v1beta2/ratelimitpolicy_types_test.go b/api/v1beta2/ratelimitpolicy_types_test.go index 73f101988..2dd512d7d 100644 --- a/api/v1beta2/ratelimitpolicy_types_test.go +++ b/api/v1beta2/ratelimitpolicy_types_test.go @@ -98,7 +98,7 @@ func TestRateLimitPolicy_GetLimits(t *testing.T) { t.Run("No limits defined", func(subT *testing.T) { r := testBuildBasicHTTPRouteRLP(name, nil) - assert.DeepEqual(subT, r.GetLimits(), map[string]Limit(nil)) + assert.DeepEqual(subT, r.Spec.CommonSpec().Limits, map[string]Limit(nil)) }) t.Run("Defaults defined", func(subT *testing.T) { r := testBuildBasicHTTPRouteRLP(name, func(policy *RateLimitPolicy) { @@ -106,13 +106,13 @@ func TestRateLimitPolicy_GetLimits(t *testing.T) { Limits: defaultLimits, } }) - assert.DeepEqual(subT, r.GetLimits(), defaultLimits) + assert.DeepEqual(subT, r.Spec.CommonSpec().Limits, defaultLimits) }) t.Run("Implicit rules defined", func(subT *testing.T) { r := testBuildBasicHTTPRouteRLP(name, func(policy *RateLimitPolicy) { policy.Spec.Limits = implicitLimits }) - assert.DeepEqual(subT, r.GetLimits(), implicitLimits) + assert.DeepEqual(subT, r.Spec.CommonSpec().Limits, implicitLimits) }) t.Run("Default rules takes precedence over implicit rules if validation is somehow bypassed", func(subT *testing.T) { r := testBuildBasicHTTPRouteRLP(name, func(policy *RateLimitPolicy) { @@ -121,6 +121,6 @@ func TestRateLimitPolicy_GetLimits(t *testing.T) { } policy.Spec.Limits = implicitLimits }) - assert.DeepEqual(subT, r.GetLimits(), defaultLimits) + assert.DeepEqual(subT, r.Spec.CommonSpec().Limits, defaultLimits) }) } diff --git a/controllers/rate_limiting_wasmplugin_controller_test.go b/controllers/rate_limiting_wasmplugin_controller_test.go index e0500d9a5..3b6a83fef 100644 --- a/controllers/rate_limiting_wasmplugin_controller_test.go +++ b/controllers/rate_limiting_wasmplugin_controller_test.go @@ -74,11 +74,13 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { Kind: "HTTPRoute", Name: gatewayapiv1.ObjectName(routeName), }, - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + RateLimitPolicyCommonSpec: kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, }, }, }, @@ -195,45 +197,47 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { Kind: "HTTPRoute", Name: gatewayapiv1.ObjectName(routeName), }, - Limits: map[string]kuadrantv1beta2.Limit{ - "toys": { - Rates: []kuadrantv1beta2.Rate{ - {Limit: 50, Duration: 1, Unit: kuadrantv1beta2.TimeUnit("minute")}, - }, - Counters: []kuadrantv1beta2.ContextSelector{"auth.identity.username"}, - RouteSelectors: []kuadrantv1beta2.RouteSelector{ - { // selects the 1st HTTPRouteRule (i.e. get|post /toys*) for one of the hostnames - Matches: []gatewayapiv1.HTTPRouteMatch{ - { - Path: &gatewayapiv1.HTTPPathMatch{ - Type: ptr.To(gatewayapiv1.PathMatchPathPrefix), - Value: ptr.To("/toys"), + RateLimitPolicyCommonSpec: kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "toys": { + Rates: []kuadrantv1beta2.Rate{ + {Limit: 50, Duration: 1, Unit: kuadrantv1beta2.TimeUnit("minute")}, + }, + Counters: []kuadrantv1beta2.ContextSelector{"auth.identity.username"}, + RouteSelectors: []kuadrantv1beta2.RouteSelector{ + { // selects the 1st HTTPRouteRule (i.e. get|post /toys*) for one of the hostnames + Matches: []gatewayapiv1.HTTPRouteMatch{ + { + Path: &gatewayapiv1.HTTPPathMatch{ + Type: ptr.To(gatewayapiv1.PathMatchPathPrefix), + Value: ptr.To("/toys"), + }, }, }, + Hostnames: []gatewayapiv1.Hostname{"*.toystore.acme.com"}, }, - Hostnames: []gatewayapiv1.Hostname{"*.toystore.acme.com"}, }, - }, - When: []kuadrantv1beta2.WhenCondition{ - { - Selector: "auth.identity.group", - Operator: kuadrantv1beta2.WhenConditionOperator("neq"), - Value: "admin", + When: []kuadrantv1beta2.WhenCondition{ + { + Selector: "auth.identity.group", + Operator: kuadrantv1beta2.WhenConditionOperator("neq"), + Value: "admin", + }, }, }, - }, - "assets": { - Rates: []kuadrantv1beta2.Rate{ - {Limit: 5, Duration: 1, Unit: kuadrantv1beta2.TimeUnit("minute")}, - {Limit: 100, Duration: 12, Unit: kuadrantv1beta2.TimeUnit("hour")}, - }, - RouteSelectors: []kuadrantv1beta2.RouteSelector{ - { // selects the 2nd HTTPRouteRule (i.e. /assets*) for all hostnames - Matches: []gatewayapiv1.HTTPRouteMatch{ - { - Path: &gatewayapiv1.HTTPPathMatch{ - Type: ptr.To(gatewayapiv1.PathMatchPathPrefix), - Value: ptr.To("/assets"), + "assets": { + Rates: []kuadrantv1beta2.Rate{ + {Limit: 5, Duration: 1, Unit: kuadrantv1beta2.TimeUnit("minute")}, + {Limit: 100, Duration: 12, Unit: kuadrantv1beta2.TimeUnit("hour")}, + }, + RouteSelectors: []kuadrantv1beta2.RouteSelector{ + { // selects the 2nd HTTPRouteRule (i.e. /assets*) for all hostnames + Matches: []gatewayapiv1.HTTPRouteMatch{ + { + Path: &gatewayapiv1.HTTPPathMatch{ + Type: ptr.To(gatewayapiv1.PathMatchPathPrefix), + Value: ptr.To("/assets"), + }, }, }, }, @@ -373,11 +377,13 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { Kind: "Gateway", Name: gatewayapiv1.ObjectName(gwName), }, - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + RateLimitPolicyCommonSpec: kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, }, }, }, @@ -471,11 +477,13 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { Kind: "Gateway", Name: gatewayapiv1.ObjectName(gwName), }, - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + RateLimitPolicyCommonSpec: kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, }, }, }, @@ -536,23 +544,25 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { Kind: "HTTPRoute", Name: gatewayapiv1.ObjectName(routeName), }, - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - RouteSelectors: []kuadrantv1beta2.RouteSelector{ - { // does no select any HTTPRouteRule (i.e. GET /toys*) - Matches: []gatewayapiv1.HTTPRouteMatch{ - { - Path: &gatewayapiv1.HTTPPathMatch{ - Type: ptr.To(gatewayapiv1.PathMatchPathPrefix), - Value: ptr.To("/other"), + RateLimitPolicyCommonSpec: kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + RouteSelectors: []kuadrantv1beta2.RouteSelector{ + { // does no select any HTTPRouteRule (i.e. GET /toys*) + Matches: []gatewayapiv1.HTTPRouteMatch{ + { + Path: &gatewayapiv1.HTTPPathMatch{ + Type: ptr.To(gatewayapiv1.PathMatchPathPrefix), + Value: ptr.To("/other"), + }, }, }, }, }, - }, - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, }, }, }, @@ -608,11 +618,13 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { Kind: "Gateway", Name: gatewayapiv1.ObjectName(gwName), }, - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + RateLimitPolicyCommonSpec: kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, }, }, }, @@ -658,23 +670,25 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { Kind: "HTTPRoute", Name: gatewayapiv1.ObjectName(routeCName), }, - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - RouteSelectors: []kuadrantv1beta2.RouteSelector{ - { // does no select any HTTPRouteRule (i.e. GET /otherPathRouteC*) - Matches: []gatewayapiv1.HTTPRouteMatch{ - { - Path: &gatewayapiv1.HTTPPathMatch{ - Type: ptr.To(gatewayapiv1.PathMatchPathPrefix), - Value: ptr.To("/notmatchingpath"), + RateLimitPolicyCommonSpec: kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + RouteSelectors: []kuadrantv1beta2.RouteSelector{ + { // does no select any HTTPRouteRule (i.e. GET /otherPathRouteC*) + Matches: []gatewayapiv1.HTTPRouteMatch{ + { + Path: &gatewayapiv1.HTTPPathMatch{ + Type: ptr.To(gatewayapiv1.PathMatchPathPrefix), + Value: ptr.To("/notmatchingpath"), + }, }, }, }, }, - }, - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, }, }, }, @@ -802,11 +816,13 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { Kind: "Gateway", Name: gatewayapiv1.ObjectName(gwName), }, - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + RateLimitPolicyCommonSpec: kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, }, }, }, @@ -1005,11 +1021,13 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { Kind: "HTTPRoute", Name: gatewayapiv1.ObjectName(routeName), }, - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + RateLimitPolicyCommonSpec: kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, }, }, }, @@ -1298,11 +1316,13 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { Kind: "HTTPRoute", Name: gatewayapiv1.ObjectName(routeAName), }, - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + RateLimitPolicyCommonSpec: kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, }, }, }, @@ -1529,11 +1549,13 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { Kind: "Gateway", Name: gatewayapiv1.ObjectName(gwName), }, - Limits: map[string]kuadrantv1beta2.Limit{ - "gatewaylimit": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + RateLimitPolicyCommonSpec: kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "gatewaylimit": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, }, }, }, @@ -1631,11 +1653,13 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { Kind: "HTTPRoute", Name: gatewayapiv1.ObjectName(routeAName), }, - Limits: map[string]kuadrantv1beta2.Limit{ - "routelimit": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 4, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + RateLimitPolicyCommonSpec: kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "routelimit": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 4, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, }, }, }, @@ -1788,11 +1812,13 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { Kind: "Gateway", Name: gatewayapiv1.ObjectName(gwName), }, - Limits: map[string]kuadrantv1beta2.Limit{ - "gatewaylimit": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + RateLimitPolicyCommonSpec: kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "gatewaylimit": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, }, }, }, @@ -1817,11 +1843,13 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { Kind: "HTTPRoute", Name: gatewayapiv1.ObjectName(routeAName), }, - Limits: map[string]kuadrantv1beta2.Limit{ - "routelimit": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 4, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + RateLimitPolicyCommonSpec: kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "routelimit": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 4, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, }, }, }, @@ -2072,19 +2100,21 @@ var _ = Describe("Rate Limiting WasmPlugin controller", func() { Kind: "HTTPRoute", Name: gatewayapiv1.ObjectName(routeName), }, - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - RouteSelectors: []kuadrantv1beta2.RouteSelector{ - { - // Route does not specify any hostname - // gateway's listener specifies *.gw.example.com - Hostnames: []gatewayapiv1.Hostname{"*.gw.example.com"}, + RateLimitPolicyCommonSpec: kuadrantv1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + RouteSelectors: []kuadrantv1beta2.RouteSelector{ + { + // Route does not specify any hostname + // gateway's listener specifies *.gw.example.com + Hostnames: []gatewayapiv1.Hostname{"*.gw.example.com"}, + }, }, - }, - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, }, }, }, diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index a88522aeb..d83d48811 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -259,7 +259,8 @@ var _ = Describe("RateLimitPolicy controller", func() { serialized, err := json.Marshal(refs) Expect(err).ToNot(HaveOccurred()) Expect(existingGateway.GetAnnotations()).To(HaveKeyWithValue( - rlp.BackReferenceAnnotationName(), string(serialized))) }) + rlp.BackReferenceAnnotationName(), string(serialized))) + }) }) Context("RLP Defaults", func() { @@ -281,7 +282,7 @@ var _ = Describe("RateLimitPolicy controller", func() { // Create HTTPRoute RLP with new default limits routeRLP := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { policy.Name = "httproute-rlp" - policy.Spec.Defaults.Limits = map[string]kuadrantv1beta2.Limit{ + policy.Spec.CommonSpec().Limits = map[string]kuadrantv1beta2.Limit{ "l1": { Rates: []kuadrantv1beta2.Rate{ { @@ -322,7 +323,7 @@ var _ = Describe("RateLimitPolicy controller", func() { Expect(existingGateway.GetAnnotations()).To(HaveKey(routeRLP.BackReferenceAnnotationName())) Expect(existingGateway.GetAnnotations()[routeRLP.BackReferenceAnnotationName()]).To(ContainSubstring(string(serialized))) - }, SpecTimeout(1*time.Minute)) + }, SpecTimeout(time.Minute)) }) Context("RLP accepted condition reasons", func() { diff --git a/pkg/rlptools/utils.go b/pkg/rlptools/utils.go index 9e1433f20..d6033c0db 100644 --- a/pkg/rlptools/utils.go +++ b/pkg/rlptools/utils.go @@ -41,7 +41,7 @@ func LimitadorRateLimitsFromRLP(rlp *kuadrantv1beta2.RateLimitPolicy) []limitado limitsNamespace := LimitsNamespaceFromRLP(rlp) rateLimits := make([]limitadorv1alpha1.RateLimit, 0) - for limitKey, limit := range rlp.GetLimits() { + for limitKey, limit := range rlp.Spec.CommonSpec().Limits { limitIdentifier := LimitNameToLimitadorIdentifier(limitKey) for _, rate := range limit.Rates { maxValue, seconds := rateToSeconds(rate) diff --git a/pkg/rlptools/wasm_utils.go b/pkg/rlptools/wasm_utils.go index b997af767..dcc368b1b 100644 --- a/pkg/rlptools/wasm_utils.go +++ b/pkg/rlptools/wasm_utils.go @@ -33,7 +33,8 @@ func WasmRules(rlp *kuadrantv1beta2.RateLimitPolicy, route *gatewayapiv1.HTTPRou // Sort RLP limits for consistent comparison with existing wasmplugin objects limitNames := make([]string, 0, len(rlp.Spec.Limits)) - for name := range rlp.GetLimits() { + limits := rlp.Spec.CommonSpec().Limits + for name := range limits { limitNames = append(limitNames, name) } @@ -42,7 +43,7 @@ func WasmRules(rlp *kuadrantv1beta2.RateLimitPolicy, route *gatewayapiv1.HTTPRou for _, limitName := range limitNames { // 1 RLP limit <---> 1 WASM rule - limit := rlp.GetLimits()[limitName] + limit := limits[limitName] limitIdentifier := LimitNameToLimitadorIdentifier(limitName) rule, err := ruleFromLimit(limitIdentifier, &limit, route) if err == nil { From 9ea64ce97d256fc6b8f4c0d7dbd4a1fe535543e3 Mon Sep 17 00:00:00 2001 From: KevFan Date: Tue, 9 Apr 2024 16:21:24 +0100 Subject: [PATCH 10/12] fix: use common spec limits for wasm rules --- pkg/rlptools/wasm_utils.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/rlptools/wasm_utils.go b/pkg/rlptools/wasm_utils.go index dcc368b1b..f502095e2 100644 --- a/pkg/rlptools/wasm_utils.go +++ b/pkg/rlptools/wasm_utils.go @@ -32,8 +32,8 @@ func WasmRules(rlp *kuadrantv1beta2.RateLimitPolicy, route *gatewayapiv1.HTTPRou } // Sort RLP limits for consistent comparison with existing wasmplugin objects - limitNames := make([]string, 0, len(rlp.Spec.Limits)) limits := rlp.Spec.CommonSpec().Limits + limitNames := make([]string, 0, len(limits)) for name := range limits { limitNames = append(limitNames, name) } From 554edbc3f5a89b6a93d8b0364090b7e058c2b990 Mon Sep 17 00:00:00 2001 From: KevFan Date: Wed, 10 Apr 2024 13:22:13 +0100 Subject: [PATCH 11/12] docs: address comments with new defaults field --- doc/reference/ratelimitpolicy.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/doc/reference/ratelimitpolicy.md b/doc/reference/ratelimitpolicy.md index 5a346f788..aac4fb735 100644 --- a/doc/reference/ratelimitpolicy.md +++ b/doc/reference/ratelimitpolicy.md @@ -2,7 +2,7 @@ - [RateLimitPolicy](#ratelimitpolicy) - [RateLimitPolicySpec](#ratelimitpolicyspec) - - [Defaults](#defaults) + - [RateLimitPolicyCommonSpec](#rateLimitPolicyCommonSpec) - [Limit](#limit) - [RateLimit](#ratelimit) - [WhenCondition](#whencondition) @@ -18,13 +18,13 @@ ## 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` | Map | No | Explicit Limit definitions. This field is mutually exclusive with [Defaults](#defaults) `limits` field | -| `limits` | [Defaults](#defaults) | No | Implicit default definitions | +| **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 | -### Defaults +### RateLimitPolicyCommonSpec | **Field** | **Type** | **Required** | **Description** | |-----------|------------------------------|--------------|------------------------------------------------------------------------------------------------------------------------------| From 493736d36b2184e15579dbf66965d9846567be03 Mon Sep 17 00:00:00 2001 From: KevFan Date: Wed, 10 Apr 2024 13:27:44 +0100 Subject: [PATCH 12/12] fix: load image for docker use explicitly --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 24704dab1..5bbe2ba52 100644 --- a/Makefile +++ b/Makefile @@ -393,7 +393,7 @@ run: generate fmt vet ## Run a controller from your host. go run ./main.go docker-build: ## Build docker image with the manager. - docker build -t $(IMG) . + docker build -t $(IMG) . --load docker-push: ## Push docker image with the manager. docker push $(IMG)