951 finalize ratelimitpolicy v1beta3

[eguzki]

What

Finalize ratelimitpolicy v1beta3. Basically, exposing CEL at RateLimitPolicy level to express predicates at top level and limit levels.

Updates applied

• Top level CEL predicate's
• Limit level CEL predicate's
• Rates window in GEP-2257: Gateway API Duration Format
• Counters CEL expression

TODO:

• unittests
• e2e tests
• doc
• Test user guides with new RLP API

apiVersion: kuadrant.io/v1beta3
kind: RateLimitPolicy
metadata:
  name: toy
  namespace: gateway-system
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: Gateway
    name: kuadrant-ingressgateway
  when:
    - predicate: "auth.identity.group == 'admin'"   # a list of "top-level" `Predicate`s
  limits:
    "expensive-operation":
      rates:
        - limit: 2
          window: 30s   # a duration as we now express them elsewhere and in GwAPI
      when:
        - predicate: "request.method == 'POST'"  # a list of per-limit `Predicate`s as were our `Condition`s, but CEL

    "limit-per-ip":
      rates:
        - limit: 5
          window: 30s
      counters:
        - expression: source.remote_address  # this maps more or less to what we do in authorino wrt `properties` et al 

Verification

make local-setup

kubectl -n kuadrant-system apply -f - <<EOF
apiVersion: kuadrant.io/v1beta1
kind: Kuadrant
metadata:
  name: kuadrant
spec: {}
EOF

kubectl apply -f examples/toystore/toystore.yaml

kubectl apply -f - <<EOF
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: toystore
spec:
  parentRefs:
  - name: kuadrant-ingressgateway
    namespace: gateway-system
  hostnames:
  - api.toystore.com
  rules:
  - matches:
    - method: GET
      path:
        type: PathPrefix
        value: "/cars"
    - method: GET
      path:
        type: PathPrefix
        value: "/dolls"
    backendRefs:
    - name: toystore
      port: 80
  - matches:
    - path:
        type: PathPrefix
        value: "/admin"
    backendRefs:
    - name: toystore
      port: 80
EOF

Export the gateway hostname and port:

export INGRESS_HOST=$(kubectl get gtw kuadrant-ingressgateway -n gateway-system -o jsonpath='{.status.addresses[0].value}')
export INGRESS_PORT=$(kubectl get gtw kuadrant-ingressgateway -n gateway-system -o jsonpath='{.spec.listeners[?(@.name=="http")].port}')
export GATEWAY_URL=$INGRESS_HOST:$INGRESS_PORT

Verify the route works:

curl -H 'Host: api.toystore.com' http://$GATEWAY_URL/cars -i
# HTTP/1.1 200 OK

Enforce rate limiting on requests to the Toy Store API

kubectl apply -f - <<EOF
apiVersion: kuadrant.io/v1beta3
kind: RateLimitPolicy
metadata:
  name: toystore
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: toystore
  limits:
    "create-toy":
      rates:
      - limit: 5
        window: 10s
      when:
      - predicate: "request.url_path != '/admin'"
EOF

Note: It may take a couple of minutes for the RateLimitPolicy to be applied depending on your cluster.

Verify the rate limiting works by sending requests in a loop.

Up to 5 successful (200 OK) requests every 10 seconds to GET /cars, then 429 Too Many Requests:

while :; do curl --write-out '%{http_code}\n' --silent --output /dev/null -H 'Host: api.toystore.com' http://$GATEWAY_URL/cars  | grep -E --color "\b(429)\b|$"; sleep 1; done

Unlimited successful (200 OK) to GET /admin:

while :; do curl --write-out '%{http_code}\n' --silent --output /dev/null -H 'Host: api.toystore.com' http://$GATEWAY_URL/admin | grep -E --color "\b(429)\b|$"; sleep 1; done

Check wasm plugin

kubectl get -n gateway-system wasmplugin/kuadrant-kuadrant-ingressgateway -o yaml

apiVersion: extensions.istio.io/v1alpha1
kind: WasmPlugin
metadata:
  creationTimestamp: "2024-11-06T14:13:41Z"
  generation: 2
  labels:
    kuadrant.io/managed: "true"
  name: kuadrant-kuadrant-ingressgateway
  namespace: gateway-system
  ownerReferences:
  - apiVersion: gateway.networking.k8s.io/v1
    blockOwnerDeletion: true
    controller: true
    kind: Gateway
    name: kuadrant-ingressgateway
    uid: d0f27e1d-2cb8-4dc3-b1f6-94204d70aaef
  resourceVersion: "6002"
  uid: 3e506fd8-0517-4ab0-9307-c822ce3b0032
spec:
  phase: STATS
  pluginConfig:
    actionSets:
    - actions:
      - data:
        - expression:
            key: limit.create_toy__fb82c51a
            value: "1"
        predicates:
        - request.url_path != '/admin'
        scope: default/toystore
        service: ratelimit-service
      name: 3333a094e3493a905c35f12ffe3fa372e02a38c9700389a532f722e00d28d898
      routeRuleConditions:
        hostnames:
        - api.toystore.com
        predicates:
        - request.url_path.startsWith('/admin')
    - actions:
      - data:
        - expression:
            key: limit.create_toy__fb82c51a
            value: "1"
        predicates:
        - request.url_path != '/admin'
        scope: default/toystore
        service: ratelimit-service
      name: 5d3152cd9b75ba0b71f4c631dac258c905748ba6cce17a9b887b8bebe43a77f0
      routeRuleConditions:
        hostnames:
        - api.toystore.com
        predicates:
        - request.method == 'GET'
        - request.url_path.startsWith('/dolls')
    - actions:
      - data:
        - expression:
            key: limit.create_toy__fb82c51a
            value: "1"
        predicates:
        - request.url_path != '/admin'
        scope: default/toystore
        service: ratelimit-service
      name: 221b3f3322ef9c2a2a833343073f3d5997ad81354f67d8b4c652cbbd054d692c
      routeRuleConditions:
        hostnames:
        - api.toystore.com
        predicates:
        - request.method == 'GET'
        - request.url_path.startsWith('/cars')
    services:
      auth-service:
        endpoint: kuadrant-auth-service
        failureMode: deny
        type: auth
      ratelimit-service:
        endpoint: kuadrant-ratelimit-service
        failureMode: allow
        type: ratelimit
  targetRefs:
  - group: gateway.networking.k8s.io
    kind: Gateway
    name: kuadrant-ingressgateway
  url: oci://quay.io/kuadrant/wasm-shim:latest

[codecov]

Codecov Report

Attention: Patch coverage is 92.07921% with 8 lines in your changes missing coverage. Please review.

Project coverage is 76.24%. Comparing base (cc1b41f) to head (7377568).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/wasm/utils.go	71.42%	4 Missing ⚠️
controllers/ratelimit_workflow_helpers.go	92.00%	1 Missing and 1 partial ⚠️
pkg/wasm/types.go	83.33%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #976      +/-   ##
==========================================
+ Coverage   76.15%   76.24%   +0.09%     
==========================================
  Files         111      111              
  Lines        8986     9050      +64     
==========================================
+ Hits         6843     6900      +57     
- Misses       1852     1855       +3     
- Partials      291      295       +4     

Flag	Coverage Δ
bare-k8s-integration	10.79% <0.00%> (-0.08%)	⬇️
controllers-integration	58.59% <52.47%> (-0.27%)	⬇️
envoygateway-integration	32.50% <51.48%> (+<0.01%)	⬆️
gatewayapi-integration	13.19% <0.00%> (-0.25%)	⬇️
istio-integration	34.53% <88.11%> (+0.20%)	⬆️
unit	25.24% <26.73%> (-0.13%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
api/v1beta1 (u)	92.18% <100.00%> (ø)
api/v1beta2 (u)	∅ <ø> (∅)
pkg/common (u)	87.67% <ø> (ø)
pkg/istio (u)	47.03% <ø> (ø)
pkg/log (u)	93.18% <ø> (ø)
pkg/reconcilers (u)	∅ <ø> (∅)
pkg/rlptools (u)	∅ <ø> (∅)
controllers (i)	84.30% <89.04%> (-0.13%)	⬇️

Files with missing lines	Coverage Δ
api/v1beta3/authpolicy_types.go	79.44% <ø> (ø)
api/v1beta3/ratelimitpolicy_types.go	83.33% <100.00%> (+5.03%)	⬆️
controllers/limitador_limits_reconciler.go	85.10% <100.00%> (+1.19%)	⬆️
controllers/ratelimit_workflow_helpers.go	96.81% <92.00%> (+0.86%)	⬆️
pkg/wasm/types.go	67.82% <83.33%> (-1.41%)	⬇️
pkg/wasm/utils.go	82.64% <71.42%> (+0.10%)	⬆️

... and 7 files with indirect coverage changes

[eguzki]

⚠️ RateLimitPolicy API changing here @azgabur @jsmolar @trepel

[maleck13]

Nice work @eguzki love how the API is looking in those examples.

[guicassolato]

We can ID the rules however we want, as long as each rule has a unique ID. That was the case of limits as the only kind of rule before this PR. It made sense IDing the rule after the unique limit name rather than coming up with something different and possibly more opaque for the rule ID.

Now, we want to differentiate these two kinds of rules (conditions and limits) within a RateLimitPolicy spec. Similarly to how we handle this in the AuthPolicy (where there are 10 different kinds of rules), we can define different prefixes for each kind. E.g. conditions could be conditions#<condition-index> and limits limits#<limit-name>.

Sure, the new rule ID limits prefix would ultimately show up in the Limitador CR and wasm config as limit.limits_<limit-name>__<hash:rlp_ns/rlp_name>. To avoida that, a trick that I think would work is setting the prefix in Rules()/SetRules() to limit. and changing this line to identifier := "".

[guicassolato]

Using a single key for all conditions as if the conditions in a policy were a single rule will effectively cause the merging of policies to select one entire set or another (atomically). I.e., the two sets of conditions will not be merged into one.

Maybe that's what we want. If so, then I'd would recommend applying the same behavior to the AuthPolicy, which is not the case today.

[guicassolato]

I've chatted with @eguzki offline about #976 (comment).

We made a call together that the approach proposed by this PR is preferable than the behaviour implemented in the AuthPolicy. Merging conditions based on index position feels like dark magic and better avoiding it.

I will unblock this PR with the promise of the work on the AuthPolicy to copy the one here (i.e. the entire set of top-level when conditions always treated as one single rule) will follow this one ASAP.