RLP v2 CRD #180

eguzki · 2023-05-05T13:36:06Z

Fixes #179

Group	Version	Kind
kuadrant.io	v2beta1	RateLimitPolicy

Example with all fields:

apiVersion: kuadrant.io/v2beta1
kind: RateLimitPolicy
metadata: {}
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: <HTTPROUTE-NAME>
  limits:
    <limit-name>:
      routeSelectors:           
      - hostnames:
         - "*.example.com"
        matches:                # []HTTPRouteMatch
        - path:                      # HTTPPathMatch 
            type: PathPrefix
            value: "/toys"
          method: GET          # HTTPMethod 
          headers:                # []HTTPHeaderMatch 
          - type: Exact
            name: "X-Canary"
            value: "1"
          queryParams:      # []HTTPQueryParamMatch 
          - type: Exact
            name: "q"
            value: "1"
      rates:
      - limit: 50
        duration: 1
        unit: minute
      counters:
      - auth.identity.username
      - context.request.http.path
      when:
      - selector: auth.identity.group
        operator: neq
        value: admin

eguzki · 2023-05-05T13:39:21Z

config/manager/manager.yaml

-          requests:
-            cpu: 200m
-            memory: 200Mi
+        - command:


eguzki · 2023-05-05T13:40:35Z

this PR targets https://github.com/Kuadrant/kuadrant-operator/tree/rlp-v2-base

eguzki · 2023-05-05T13:42:20Z

api/v2beta1/ratelimitpolicy_types.go

+// https://gateway-api.sigs.k8s.io/v1alpha2/references/spec/#gateway.networking.k8s.io/v1beta1.HTTPRouteSpec
+type WhenCondition struct {
+	// 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


Those well known selectors need to be properly documented

guicassolato · 2023-05-08T08:58:06Z

api/v2beta1/ratelimitpolicy_types.go

+// +kubebuilder:validation:MaxLength=253
+type ContextSelector string
+
+// +kubebuilder:validation:Enum:=eq;neq;incl;excl;matches


For RLP, I thought we were going with only eq and neq for now, so nothing needs to change in Limitador.

Suggested change

// +kubebuilder:validation:Enum:=eq;neq;incl;excl;matches

// +kubebuilder:validation:Enum:=eq;neq

Well, not necessarily when conditions are going to be applied in Limitador. The wasm-shim can enforce those rules as well to save Limitador's traffic.

But It's ok to start with only eq and neq for now. We can extend those later.

Probably a debate for another forum, when discussing the attributions of the wasm-whim rather...

You are right about the wasm-shim technically being capable of enforcing all kinds of user-defined conditions (route related or non-route related) if we want it to. After all, it's all data the wasm-shim will need to request anyway to build the descriptors for the RL request.

I personally have always thought on route-related conditions evaluated by the wasm-shim and translated into one single descriptor to be matched by one single Limitador condition, which is the one with the identifier of the limit; and then conditions not related to route attributes, transparently passed along by the wasm-shim in the RL request and evaluated only by Limitador. But it doesn't have to be this way.

What I think you're suggesting as alternative is a bit bolder and could/would make the wasm-shim the solely evaluator of user-defined conditions effectively. Perhaps saving requests to Limitador is enough reason to do that.

Of course, requests to Limitador can only be saved when none of the limits have matching conditions, after all it takes only one single limit definition to have a matching condition for the wasm-shim to have to call Limitador. But perhaps the wasm-shim could be more selective and inject less descriptors when building requests to Limitador – i.e., only the ones for matching limit identifiers, because other conditions were all evaluated upfront by the wasm-shim itself and the limits are known to have to be triggered?

So less requests to Limitador and less conditions evaluated by Limitador in the cases where a RL request is unavoidable. 🤔

didierofrivia · 2023-05-09T09:12:20Z

api/v2beta1/ratelimitpolicy_types.go

+// TODO Document properly "Well-known selector" https://github.com/Kuadrant/architecture/blob/main/rfcs/0001-rlp-v2.md#well-known-selectors
+// +kubebuilder:validation:MinLength=1
+// +kubebuilder:validation:MaxLength=253
+type ContextSelector string


If we already have some identified, maybe we could enum them ?

the list is too big and we also do not want to limit to a set of options, do we? It is all about a context with attributes that can be added dynamically.

Right, @guicassolato ? BTW, do we know if this is documented? do we have a link?

It is documented, but it isn't. Perhaps the closest thing we've got is https://github.com/Kuadrant/authorino/blob/main/docs/architecture.md#the-authorization-json, combined with https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/auth/v3/external_auth.proto#envoy-v3-api-msg-service-auth-v3-checkrequest. What's not explicitly there is because it's really an open struct. But we do need a proper doc explaining it, with examples and all.

In the case of Authorino, the selectors include this as well: https://github.com/Kuadrant/authorino/blob/main/docs/features.md#common-feature-json-paths-valuefromauthjson

Adding as well https://github.com/Kuadrant/architecture/blob/main/rfcs/0001-rlp-v2.md#well-known-selectors, which is already in a comment below and may be the best doc we had on the topic so far.

The Envoy's Ext-Authz CheckRequest.AttributeContext type context may not be the same a wasm filter (or any filter) may access to.

This is promising doc: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/advanced/attributes

guicassolato · 2023-05-11T10:00:24Z

api/v2beta1/groupversion_info.go

+
+var (
+	// GroupVersion is group version used to register these objects
+	GroupVersion = schema.GroupVersion{Group: "kuadrant.io", Version: "v2beta1"}


I wonder if it shouldn't be v1beta2, provided v1beta1 may never become v1. WDYT?

#179 (comment)

guicassolato · 2023-05-11T10:02:46Z

api/v2beta1/ratelimitpolicy_types.go

+	Selector ContextSelector `json:"selector"`
+
+	// The binary operator to be applied to the content fetched from the selector
+	// Possible values are: "eq" (equal to), "neq" (not equal to), "incl" (includes; for arrays), "excl" (excludes; for arrays), "matches" (regex)


I think some of these were removed.

guicassolato · 2023-05-11T10:06:44Z

api/v2beta1/ratelimitpolicy_types.go

+
+	// The value of reference for the comparison.
+	// If used with the "matches" operator, the value must compile to a valid Golang regex.
+	Value string `json:"value"`


Authorino conditions support only string comparisons too. I've been wondering lately if we shouldn't try to support any JSON/YAML value type. In the future, this may be helpful for numeric operators (e.g. greater than, lower than, etc), array lookups, etc. WDYT?

Good point. For now, as the condition operator is either eq or neq, I would stick to literal (string).

guicassolato · 2023-05-11T10:12:28Z

api/v2beta1/ratelimitpolicy_types.go

+	// Hostnames defines a set of hostname that should match against the HTTP Host header to select a HTTPRoute to process the request
+	// https://gateway-api.sigs.k8s.io/v1alpha2/references/spec/#gateway.networking.k8s.io/v1beta1.HTTPRouteSpec
+	// +optional
+	Hostnames []gatewayapiv1alpha2.Hostname `json:"hostnames,omitempty"`


This means a validation (which I like!) against the following pattern:

^(\*\.)?[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$

Only to highlight that the example provided in the description of the PR (*example.com) is not valid.

good catch! I will change it. (I think it was a typo anyway)

eguzki · 2023-05-17T22:09:08Z

ready for a new review @guicassolato

didierofrivia · 2023-05-22T13:46:39Z

api/v1beta2/ratelimitpolicy_types.go

+	if r.Spec.TargetRef.Kind != gatewayapiv1alpha2.Kind("HTTPRoute") && r.Spec.TargetRef.Kind != gatewayapiv1alpha2.Kind("Gateway") {
+		return fmt.Errorf("invalid targetRef.Kind %s. The only supported kind types are HTTPRoute and Gateway", r.Spec.TargetRef.Kind)


Oh, so, we will still only cover the usage of HTTPRoute and Gateway. I thought for this v2 we were thinking on adding GatewayClass.. not sure about the VirtualService

Currently, it is only HTTPRoute. #156

didierofrivia · 2023-05-22T13:47:49Z

api/v1beta2/ratelimitpolicy_types.go

+	}
+
+	if r.Spec.TargetRef.Namespace != nil && string(*r.Spec.TargetRef.Namespace) != r.Namespace {
+		return fmt.Errorf("invalid targetRef.Namespace %s. Currently only supporting references to the same namespace", *r.Spec.TargetRef.Namespace)


Would be informative if we show in the error the valid namespace

It's the current one. The rate limit policy's namespace.

Anyway, this is only about the CRD we can improve that in coming PR's with actual implementation

guicassolato · 2023-05-23T11:12:38Z

api/v1beta2/ratelimitpolicy_types.go

+		diff := cmp.Diff(string(currentMarshaledJSON), string(otherMarshaledJSON))
+		logger.V(1).Info("Conditions not equal", "difference", diff)


Worth saving the cost of cmp.Diff if !logger.V(1).Enabled()?

guicassolato · 2023-05-23T11:14:25Z

api/v1beta2/ratelimitpolicy_types.go

+	}
+
+	// TODO(eastizle): reflect.DeepEqual does not work well with lists without order
+	if !reflect.DeepEqual(s.GatewaysRateLimits, other.GatewaysRateLimits) {


I wonder if order matters. If it does matter, do we want to enforce same order? I'm guessing "yes, we do", right?

I have removed the GatewaysRateLimits struct from the status section of the CRD. I guess this is still a valid feature, but needs to be refactored entirely. We can add items to the status later when we implement the feature. Does it sound good to you?

eguzki requested a review from a team as a code owner May 5, 2023 13:36

eguzki commented May 5, 2023

View reviewed changes

config/manager/manager.yaml

requests:

cpu: 200m

memory: 200Mi

- command:

Copy link

Contributor Author

eguzki May 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷

eguzki commented May 5, 2023

View reviewed changes

guicassolato reviewed May 8, 2023

View reviewed changes

didierofrivia reviewed May 9, 2023

View reviewed changes

eguzki requested a review from a team May 9, 2023 15:09

guicassolato reviewed May 11, 2023

View reviewed changes

eguzki added 2 commits May 17, 2023 23:56

rpl v2 CRD

fe9ce18

RLP CRD v2: WhenConditionOperator only eq and neq

23948f5

eguzki force-pushed the crd branch from 04d79ec to 23948f5 Compare May 17, 2023 21:56

controller-gen paths for v1beta2 as well

3166bce

eguzki requested a review from a team May 18, 2023 15:04

didierofrivia reviewed May 22, 2023

View reviewed changes

guicassolato approved these changes May 23, 2023

View reviewed changes

minor enhancement about logging only in debug mode

929a802

eguzki merged commit b314383 into rlp-v2-base May 24, 2023

eguzki deleted the crd branch May 24, 2023 14:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RLP v2 CRD #180

RLP v2 CRD #180

eguzki commented May 5, 2023 •

edited

Loading

eguzki May 5, 2023

eguzki commented May 5, 2023

eguzki May 5, 2023

guicassolato May 8, 2023

eguzki May 8, 2023

guicassolato May 8, 2023

didierofrivia May 9, 2023

eguzki May 9, 2023

guicassolato May 11, 2023

guicassolato May 11, 2023

eguzki May 17, 2023

guicassolato May 11, 2023

guicassolato May 11, 2023

guicassolato May 11, 2023

eguzki May 17, 2023

guicassolato May 11, 2023

eguzki May 17, 2023 •

edited

Loading

eguzki commented May 17, 2023

didierofrivia May 22, 2023

eguzki May 23, 2023

didierofrivia May 22, 2023

eguzki May 23, 2023

guicassolato May 23, 2023

guicassolato May 23, 2023

eguzki May 23, 2023

	// +kubebuilder:validation:Enum:=eq;neq;incl;excl;matches
	// +kubebuilder:validation:Enum:=eq;neq

		if r.Spec.TargetRef.Kind != gatewayapiv1alpha2.Kind("HTTPRoute") && r.Spec.TargetRef.Kind != gatewayapiv1alpha2.Kind("Gateway") {
		return fmt.Errorf("invalid targetRef.Kind %s. The only supported kind types are HTTPRoute and Gateway", r.Spec.TargetRef.Kind)

		diff := cmp.Diff(string(currentMarshaledJSON), string(otherMarshaledJSON))
		logger.V(1).Info("Conditions not equal", "difference", diff)

RLP v2 CRD #180

RLP v2 CRD #180

Conversation

eguzki commented May 5, 2023 • edited Loading

Choose a reason for hiding this comment

eguzki commented May 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eguzki May 17, 2023 • edited Loading

Choose a reason for hiding this comment

eguzki commented May 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eguzki commented May 5, 2023 •

edited

Loading

eguzki May 17, 2023 •

edited

Loading