Limitador cluster EnvoyFilter controller #243

eguzki · 2023-08-30T22:53:19Z

What

Specific controller to manage rate limit's EnvoyFilter used to add Limitador's service cluster to the Gateway config.

RateLimitPolicy CRD controller is responsible of adding/deleting RLP refs to the gateway.

EnvoyFilter controller watches gateways. When one of them has kuadrant.io/ratelimitpolicies annotation, it means rate limiting should happen in that gateway, thus a EnvoyFilter resource is created for that gateway.

When there is no annotation or the list of RLP refs is empty, the EnvoyFilter resource is deleted (if exists).

The EnvoyFilter resource has owner ref to the gateway associated. The controller also watches for changes in the EnvoyFilter resource and reverts them back (if spec is changed).

Verification steps

Setup the environment:

make local-setup

Request an instance of Kuadrant:

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

Create a Kuadrant RateLimitPolicy to configure rate limiting:

kubectl apply -n istio-system -f - <<EOF
apiVersion: kuadrant.io/v1beta2
kind: RateLimitPolicy
metadata:
  name: gw-rlp
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: Gateway
    name: istio-ingressgateway
    namespace: istio-system
  limits:
    "global":
      rates:
      - limit: 5
        duration: 10
        unit: second
EOF

Wait for RLP to be available

kubectl wait --for=condition=available ratelimitpolicy/gw-rlp -n istio-system --timeout=10s

Check for the Gateway's references to RLPs

kubectl get gateways istio-ingressgateway -n istio-system -o jsonpath='{.metadata.annotations.kuadrant\.io/ratelimitpolicies}' | yq e -P

It should output something like

- Namespace: istio-system
  Name: gw-rlp

Check for the Limitador's Cluster EnvoyFilter resource

kubectl get envoyfilter kuadrant-ratelimiting-cluster-istio-ingressgateway -n istio-system -o yaml

It should output something like

apiVersion: networking.istio.io/v1alpha3
kind: EnvoyFilter
metadata:
  creationTimestamp: "2023-08-31T17:36:09Z"
  generation: 1
  name: kuadrant-ratelimiting-cluster-istio-ingressgateway
  namespace: istio-system
  ownerReferences:
  - apiVersion: gateway.networking.k8s.io/v1beta1
    blockOwnerDeletion: true
    controller: true
    kind: Gateway
    name: istio-ingressgateway
    uid: 043732fa-e588-4c3b-980f-7d9497e3342a
  resourceVersion: "2000"
  uid: 91b89285-eca3-4c0b-a7e4-84f6a7c4aeae
spec:
  configPatches:
  - applyTo: CLUSTER
    match:
      cluster:
        service: limitador-limitador.kuadrant-system.svc.cluster.local
    patch:
      operation: ADD
      value:
        connect_timeout: 1s
        http2_protocol_options: {}
        lb_policy: ROUND_ROBIN
        load_assignment:
          cluster_name: kuadrant-rate-limiting-service
          endpoints:
          - lb_endpoints:
            - endpoint:
                address:
                  socket_address:
                    address: limitador-limitador.kuadrant-system.svc.cluster.local
                    port_value: 8081
        name: kuadrant-rate-limiting-service
        type: STRICT_DNS
  workloadSelector:
    labels:
      app: istio-ingressgateway
      istio: ingressgateway

Deleting the RLP should also delete the EnvoyFilter object as there should not be RLP refs in the gateway

kubectl delete ratelimitpolicy/gw-rlp -n istio-system

Check for the Gateway's references to RLPs

kubectl get gateways istio-ingressgateway -n istio-system -o jsonpath='{.metadata.annotations.kuadrant\.io/ratelimitpolicies}' | yq e -P

It should output empty array

[]

Check for the Limitador's Cluster EnvoyFilter resource

kubectl get envoyfilter kuadrant-ratelimiting-cluster-istio-ingressgateway -n istio-system -o yaml

It should NotFound error

Error from server (NotFound): envoyfilters.networking.istio.io "kuadrant-ratelimiting-cluster-istio-ingressgateway" not found

codecov · 2023-08-30T22:57:17Z

Codecov Report

Merging #243 (500887c) into main (7dcbc3d) will decrease coverage by 0.97%.
Report is 4 commits behind head on main.
The diff coverage is 79.79%.

@@            Coverage Diff             @@
##             main     #243      +/-   ##
==========================================
- Coverage   63.83%   62.87%   -0.97%     
==========================================
  Files          32       32              
  Lines        3127     3135       +8     
==========================================
- Hits         1996     1971      -25     
- Misses        969      994      +25     
- Partials      162      170       +8

Flag	Coverage Δ
integration	`68.74% <79.79%> (-2.06%)`	⬇️
unit	`57.52% <ø> (ø)`

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

Components	Coverage Δ
api/v1beta1 (u)	`∅ <ø> (∅)`
pkg/common (u)	`75.90% <ø> (ø)`
pkg/istio (u)	`30.24% <ø> (ø)`
pkg/log (u)	`31.81% <ø> (ø)`
pkg/reconcilers (u)	`33.68% <ø> (ø)`
pkg/rlptools (u)	`57.63% <ø> (ø)`
controllers (i)	`68.74% <79.79%> (-2.06%)`	⬇️

Files Changed	Coverage Δ
controllers/ratelimitpolicy_controller.go	`73.88% <ø> (-1.12%)`	⬇️
...ollers/limitador_cluster_envoyfilter_controller.go	`79.79% <79.79%> (ø)`

... and 4 files with indirect coverage changes

eguzki · 2023-08-30T22:58:05Z

Looking for early review

guicassolato

Looking good, @eguzki! I know you'll probably add some tests still, but leaving my 👍 here since now 'cause I like the direction this is going. Thanks!

…roller

eguzki · 2023-08-31T19:40:35Z

Test script for macos failing 🤷

Run make operator-sdk
./utils/install-operator-sdk.sh /Users/runner/work/kuadrant-operator/kuadrant-operator/bin/operator-sdk v1.28.1
Downloading https://github.com/operator-framework/operator-sdk/releases/download/v1.28.1/operator-sdk_darwin_amd64
gpg: directory '/Users/runner/.gnupg' created
gpg: keyserver receive failed: No route to host
make: *** [/Users/runner/work/kuadrant-operator/kuadrant-operator/bin/operator-sdk] Error 2
Error: Process completed with exit code 2.

https://github.com/operator-framework/operator-sdk/releases/download/v1.28.1/operator-sdk_darwin_amd64 can be downloaded correctly

guicassolato · 2023-09-01T08:25:41Z

Test script for macos failing

It's been for a while, including in main.

I think the GHA fails to resolve keyserver.ubuntu.com from the gpg command. We could try keys.openpgp.org instead, but I read somewhere that it could also be a problem of the Docker environment that runs the action, on macos.

Update: IPv6-related issue reported in the past: rvm/rvm#4215

controllers/limitador_cluster_envoyfilter_controller.go

guicassolato · 2023-09-13T16:04:40Z

controllers/limitador_cluster_envoyfilter_controller.go

+	}
+
+	if logger.V(1).Enabled() {
+		jsonData, err := json.MarshalIndent(gw, "", "  ")


Why MarshalIndent? I wonder whether it's suitable for when using structured logging, aka production log mode.

AFAIK, from the go-logr interface I can only check logger level, nothing about the format (defined by development or production mode). I see that this MarshalIndent call is not entirely correct as logger could be configured with structured format and debug level. By default (no LOG_* env vars), the logger is configured with info level and production level. When running with make run. it is debug and development. Thus, no issue with this so far.

Any better approach you are considering? Not for this PR, but considering that (almost) all controllers have this code in the beginning of the Reconcile method, it would be nice to have all of them fixed.

I didn't mean checking here what the log mode setting is. I just assumed that log level and log mode were two separate things, like in Authorino. But it looks like log mode and log level are somehow coupled together.

IMO, it does not have to be like that.

In Authorino, all 4 combinations are allowed, i.e.:

level: info, mode: production

level: debug, mode: production

level: info, mode: development

level: debug, mode: production

(Maybe the naming is a bit unfortunate, because production and development mean actually "structured" and "human-readable", respectively.)

Because it felt weird printing indented JSON when log mode is set to production, I'd probably have used json.Marshal instead. Not a big deal though.

This project's log system is exactly the same, with those 4 combinations allowed.

Because it felt weird printing indented JSON when log mode is set to production, I'd probably have used json.Marshal instead. Not a big deal though.

Check my previous comment out again. You are right. However, if I change to Mashal, when I run in debug + development mode when I run with make run, it is not human readable. I need to indent it :)

So looking for alternatives that makes a good dev experience as well.

guicassolato

Left a couple comments, but nothing that would block the PR.
In general, excellent work, @eguzki!

Co-authored-by: Guilherme Cassolato <[email protected]>

guicassolato reviewed Aug 31, 2023

View reviewed changes

eguzki added 2 commits August 31, 2023 16:26

envoyfilter controller

6d7384b

fix linting issues

10cc4fc

eguzki force-pushed the envoyfilter-controller branch from f182cc9 to 10cc4fc Compare August 31, 2023 14:28

eguzki added 2 commits August 31, 2023 19:23

renaming envoyfilter_controller to limitador_cluster_envoyfilter_cont…

2ed853d

…roller

limitador_cluster_envoyfilter_controller e2e tests

54d9907

eguzki marked this pull request as ready for review August 31, 2023 17:46

eguzki requested a review from a team as a code owner August 31, 2023 17:46

eguzki requested a review from guicassolato September 6, 2023 13:07

guicassolato reviewed Sep 13, 2023

View reviewed changes

controllers/limitador_cluster_envoyfilter_controller.go Show resolved Hide resolved

guicassolato reviewed Sep 13, 2023

View reviewed changes

guicassolato approved these changes Sep 13, 2023

View reviewed changes

Update controllers/limitador_cluster_envoyfilter_controller.go

500887c

Co-authored-by: Guilherme Cassolato <[email protected]>

eguzki merged commit 6f257ce into main Sep 18, 2023
20 of 21 checks passed

eguzki deleted the envoyfilter-controller branch September 18, 2023 09:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limitador cluster EnvoyFilter controller #243

Limitador cluster EnvoyFilter controller #243

eguzki commented Aug 30, 2023 •

edited

Loading

codecov bot commented Aug 30, 2023 •

edited

Loading

eguzki commented Aug 30, 2023

guicassolato left a comment

eguzki commented Aug 31, 2023

guicassolato commented Sep 1, 2023 •

edited

Loading

guicassolato Sep 13, 2023

eguzki Sep 18, 2023

guicassolato Sep 18, 2023

eguzki Sep 18, 2023

guicassolato left a comment

Limitador cluster EnvoyFilter controller #243

Limitador cluster EnvoyFilter controller #243

Conversation

eguzki commented Aug 30, 2023 • edited Loading

What

Verification steps

codecov bot commented Aug 30, 2023 • edited Loading

Codecov Report

eguzki commented Aug 30, 2023

guicassolato left a comment

Choose a reason for hiding this comment

eguzki commented Aug 31, 2023

guicassolato commented Sep 1, 2023 • edited Loading

guicassolato Sep 13, 2023

Choose a reason for hiding this comment

eguzki Sep 18, 2023

Choose a reason for hiding this comment

guicassolato Sep 18, 2023

Choose a reason for hiding this comment

eguzki Sep 18, 2023

Choose a reason for hiding this comment

guicassolato left a comment

Choose a reason for hiding this comment

eguzki commented Aug 30, 2023 •

edited

Loading

codecov bot commented Aug 30, 2023 •

edited

Loading

guicassolato commented Sep 1, 2023 •

edited

Loading