fix direct reference annotation reconciliation

[eguzki]

What

Found issue with the policies direct references in network resources used to prevent multiple policies (of the same kind) to target the same network resource.

The issue can be reproduced by:

• Crate RLP A -> Route A
• Change RLP A target to Route B: RLP A -> Route B
• Route A annotation kuadrant.io/ratelimitpolicy is never deleted and therefore, new policies cannot target Route A.

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 HTTPRoute A

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

Create RLP targeting route A

kubectl apply -f - <<EOF
apiVersion: kuadrant.io/v1beta2
kind: RateLimitPolicy
metadata:
  name: toystore
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: route-a
  limits:
    "create-toy":
      rates:
      - limit: 5
        duration: 10
        unit: second
EOF

Checking the direct backreference annotation of the route-a gateway

kubectl get httproute route-a -o jsonpath='{.metadata.annotations}' | yq e '."kuadrant.io/ratelimitpolicy"' -P

should give rate limit policy toystore

default/toystore

Create a new Route B

kubectl apply -f - <<EOF
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: route-b
spec:
  parentRefs:
  - name: istio-ingressgateway
    namespace: istio-system
  hostnames:
  - a.toystore.com
  rules:
  - matches:
    - method: GET
      path:
        type: PathPrefix
        value: "/toys"
    backendRefs:
    - name: toystore
      port: 80
EOF

RLP toystore switches target to Route B

kubectl patch ratelimitpolicy toystore --type=merge --patch '{"spec": {"targetRef": {"name": "route-b"}}}'

Checking the direct backreference annotation of the route-a gateway

kubectl get httproute route-a -o jsonpath='{.metadata.annotations}' | yq e '."kuadrant.io/ratelimitpolicy"' -P

no, it should be empty

null

Checking the direct backreference annotation of the route-b gateway

kubectl get httproute route-b -o jsonpath='{.metadata.annotations}' | yq e '."kuadrant.io/ratelimitpolicy"' -P

Is should give rate limit policy toystore

default/toystore

[codecov]

Codecov Report

Merging #537 (e68f22a) into main (ece13e8) will increase coverage by 0.80%.
Report is 33 commits behind head on main.
The diff coverage is 76.92%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #537      +/-   ##
==========================================
+ Coverage   80.20%   81.01%   +0.80%     
==========================================
  Files          64       71       +7     
  Lines        4492     4988     +496     
==========================================
+ Hits         3603     4041     +438     
- Misses        600      639      +39     
- Partials      289      308      +19     

Flag	Coverage Δ
integration	71.99% <76.92%> (+0.71%)	⬆️
unit	33.53% <23.07%> (+3.50%)	⬆️

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

Components	Coverage Δ
api/v1beta1 (u)	71.42% <ø> (ø)
api/v1beta2 (u)	88.81% <100.00%> (-2.62%)	⬇️
pkg/common (u)	83.01% <ø> (-5.81%)	⬇️
pkg/istio (u)	75.14% <100.00%> (+1.23%)	⬆️
pkg/log (u)	94.73% <ø> (ø)
pkg/reconcilers (u)	∅ <ø> (∅)
pkg/rlptools (u)	79.84% <ø> (+0.39%)	⬆️
controllers (i)	77.53% <82.66%> (+0.73%)	⬆️

Files	Coverage Δ
pkg/library/reconcilers/target_ref_reconciler.go	77.34% <76.92%> (-0.11%)	⬇️

... and 15 files with indirect coverage changes

[guicassolato]

I haven't tried the verification steps, but this looks good... and needed.

Only risk I can think of is that there's no rollback in case of failure mid-step 2 or during step 3. However, and to be fair, this is pretty much the case of most of our reconciliation functions ATM.

🚀

[eguzki]

I have been looking at the integration tests failing. The test failed because the tear down (AfterEach) expired. It has nothing to do with the test itself (one regarding validation of targets happen in the same namespace).

The process happened as follow:

• The tear down method deleted the namespace. That triggered deletion of all the resources without any guarantee on the order.
• First, the controller responsible of the kuadrant.io/namespace annotation, got Kuadrant CR deletion event. Thus, the controller removed that annotation from the gateway.
• Then, the rate limit policy controller got event for the deletion of the policy. The method handling removal of the policy tried to read the kuadrant namespace to get limitador's namespace and reconcile limitador's limits. An error was thrown and the method handling the removal of the policy could not finish, never removing the finalizer. Hence, the tear down method timed out.

I think I have fixed the issue in #527. I will make sure the controllers, on handling removal, they do not rely on some order when objects are deleted like when a namespace is deleted.