refactor: target status -> discoverability reconciler #958

KevFan · 2024-10-25T08:20:33Z

Description

Refactors target status controller to seperate gateway and http route discoverability reconcilers
Adds xPolicyAffected condition to Listeners status, listing accepted policies targeting the gateway and the listener (if any)
Refactors xPolicyAffected condtions to list accepted policies in path that may affect the targetablle
- Eg. Gateway condition will list only gateway policies, Listener status condition will list both gateway and listener policies, Route condition will list Gateway, Listener and route policies

Verification

Create cluster

make local-setup

Create gateway RLP

kubectl apply -n gateway-system -f - <<EOF                                                                                                                     
apiVersion: kuadrant.io/v1beta3
kind: RateLimitPolicy
metadata:      
  name: gw-rlp           
spec:        
  targetRef:       
    group: gateway.networking.k8s.io
    kind: Gateway    
    name: kuadrant-ingressgateway
  overrides:      
    limits:         
      "global":   
        rates:   
        - limit: 5
          duration: 10
          unit: second        
        when:      
        - selector: source.address
          operator: neq  
          value: 127.0.0.1
    strategy: merge
EOF

Verify Gateway kuadrant.io/RateLimitPolicyAffected condition is Object affected by RateLimitPolicy [gateway-system/gw-rlp] for Gateway.Status.Condtions and Gateway.Status.Listener[].Condition

kubectl get gateway kuadrant-ingressgateway -n gateway-system -o yaml | yq '.status'

Create HTTP Route

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

Verify HTTP Route kuadrant.io/RateLimitPolicyAffected condition is Object affected by RateLimitPolicy [gateway-system/gw-rlp] for Route.Status.Condtions

 kubectl get httproute toystore -o yaml | yq '.status'

Creation section name rlp

kubectl apply -n gateway-system -f - <<EOF              
apiVersion: kuadrant.io/v1beta3
kind: RateLimitPolicy
metadata:
  name: section-rlp
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: Gateway
    name: kuadrant-ingressgateway
    sectionName: http
  limits:
    "specific":
      rates:
      - limit: 3
        duration: 5
        unit: second
      - limit: 20
        duration: 1
        unit: minute
EOF

Verify the gw-rlp is listed along with section-rlp for the targeted listener. All other listener status and the parent gatway condition should only list gw-rlp

kubectl get gateway kuadrant-ingressgateway -n gateway-system -o yaml | yq '.status'

Verify http route condition also lists gw-rlp and section-rlp in the affection condition

 kubectl get httproute toystore -o yaml | yq '.status'

Create Route RLP

kubectl apply -f - <<EOF                 
apiVersion: kuadrant.io/v1beta3
kind: RateLimitPolicy
metadata:
  name: route-rlp
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: toystore
  limits:
    "specific":
      rates:
      - limit: 3
        duration: 5
        unit: second
      - limit: 20
        duration: 1
        unit: minute
EOF

Verify http route condition also lists route-rlp, section-rlp and gw-rlp and in the affection condition
Verify route-rlp is not listed in the gateway or gateway listener status

guicassolato · 2024-10-28T16:32:37Z

controllers/gateway_policy_discoverability_reconciler.go

+		conditions := gw.Status.DeepCopy().Conditions
+
+		// TODO: What happens for multiple policies of the same kind -
+		// TODO: Should it be conditions per listener?


I would say so. TLSPolicy and DNSPolicy still don't support targeting individual listeners (#891), but the AuthPolicy and RateLimitPolicy do.

Yeah, I've something working where the Gateway.Status.Conditions[XPolicyAffected] lists xPolicies that just specifically targets the gateway. The listener status in Gateway.Status.ListenersStatus will list inherited gateway policies and policies targetting specifically the listener. I think this is nicer and more informative to at least know which policies affect which sections of the gateway 🤔

Example:

conditions: - lastTransitionTime: "2024-10-29T09:30:44Z" message: Resource accepted observedGeneration: 1 reason: Accepted status: "True" type: Accepted - lastTransitionTime: "2024-10-29T09:30:49Z" message: Resource programmed, assigned to service(s) kuadrant-ingressgateway-istio.gateway-system.svc.cluster.local:80 observedGeneration: 1 reason: Programmed status: "True" type: Programmed - lastTransitionTime: "2024-10-29T09:37:57Z" message: Object affected by RateLimitPolicy [gateway-system/gw-rlp] observedGeneration: 1 reason: Accepted status: "True" type: kuadrant.io/RateLimitPolicyAffected listeners: - attachedRoutes: 0 conditions: - lastTransitionTime: "2024-10-29T09:30:44Z" message: No errors found observedGeneration: 1 reason: Accepted status: "True" type: Accepted - lastTransitionTime: "2024-10-29T09:30:44Z" message: No errors found observedGeneration: 1 reason: NoConflicts status: "False" type: Conflicted - lastTransitionTime: "2024-10-29T09:30:44Z" message: No errors found observedGeneration: 1 reason: Programmed status: "True" type: Programmed - lastTransitionTime: "2024-10-29T09:30:44Z" message: No errors found observedGeneration: 1 reason: ResolvedRefs status: "True" type: ResolvedRefs - lastTransitionTime: "2024-10-29T09:37:39Z" message: Object affected by RateLimitPolicy [gateway-system/gw-rlp gateway-system/section-rlp] observedGeneration: 1 reason: Accepted status: "True" type: kuadrant.io/RateLimitPolicyAffected name: http supportedKinds: - group: gateway.networking.k8s.io kind: HTTPRoute - group: gateway.networking.k8s.io kind: GRPCRoute

What do you think? 🤔

guicassolato · 2024-10-28T16:34:20Z

controllers/gateway_policy_discoverability_reconciler.go

+		for _, policyKind := range policyKinds {
+			// Policies targeting gateway and listeners
+			path := []machinery.Targetable{gw}
+			path = append(path, topology.Targetables().Children(gw)...)


I believe this will not work on Gateways with 2+ listeners. It would yield a path Gateway → Listener 1 → Listener 2 → … → Listener N

Boomatang

Functionally it seems better than before. I compared the status of the gateways and httproutes on the this branch and off main. The setup I used has bad tls configurations which cause errors. In this work those error were reported better. This work also show what DNS policies affected a httproute in the httproute status. I am assuming this was an improvement and not an error in the implementation.

The only comments I have is on the structure of the code its self. It is stuff that I feel we should address but at the same time I am okay if we address it after GA.

controllers/gateway_policy_discoverability_reconciler.go

controllers/httproute_policy_discoverability_reconciler.go

KevFan · 2024-11-04T18:40:44Z

@Boomatang Thanks for the review 👍

On:

This work also show what DNS policies affected a httproute in the httproute status. I am assuming this was an improvement and not an error in the implementation.

This was an unintentional improvement from the refactor from simply including the DNSPolicy and TLSPolicy in the http route discoverability reconciler loop:

kuadrant-operator/controllers/common.go

Lines 76 to 82 in 8098ca7

    
           func policyGroupKinds() []*schema.GroupKind { 
        
           	return []*schema.GroupKind{ 
        
           		&kuadrantv1beta3.AuthPolicyGroupKind, 
        
           		&kuadrantv1beta3.RateLimitPolicyGroupKind, 
        
           		&kuadrantv1alpha1.TLSPolicyGroupKind, 
        
           		&kuadrantv1alpha1.DNSPolicyGroupKind, 
        
           	}

I think it's a nice addition, but not sure do we want this in general to have the xPoilcyAffection for DNSPolicy and TLSPolicy for Routes like RateLimitPolicy and AuthPolicy previous to this refactor 🤔

Maybe @mikenairn @guicassolato or @maleck13 can comment if we want this for DNSPolicy and TLSPolicy

maleck13 · 2024-11-05T07:30:06Z

Well for me it comes down to is this useful for the end user. I think it is useful to know your HTTPRoute is covered by TLSPolicy or DNSPolicy. Do you have an example of the status message? I only would have a concern if we were leaking additional info but I doubt we are so +1 from me

Boomatang

Changes look good to me.

mikenairn · 2024-11-05T09:31:10Z

I think it's a nice addition, but not sure do we want this in general to have the xPoilcyAffection for DNSPolicy and TLSPolicy for Routes like RateLimitPolicy and AuthPolicy previous to this refactor 🤔

I see no issue with it, makes sense to show that it is affected by these policies also.

KevFan · 2024-11-05T09:57:17Z

Do you have an example of the status message?

@maleck13 This would be an example status condition for TLSPolicy on a HTTPRoute (DNSPolicy will look similar):

    status:
      parents:
        - conditions:
            - lastTransitionTime: "2024-11-05T09:47:22Z"
              message: Object affected by TLSPolicy [istio-system/gw-tls]
              observedGeneration: 1
              reason: Accepted
              status: "True"
              type: kuadrant.io/TLSPolicyAffected
          controllerName: kuadrant.io/policy-controller
          parentRef:
            group: gateway.networking.k8s.io
            kind: Gateway
            name: istio-ingressgateway
            namespace: istio-system

A more complicated one for say RateLimitPolicy with multiple policies on different targets along the path would look like this for a HTTPRoute:

  - conditions:
      - lastTransitionTime: "2024-11-05T09:51:35Z"
        message: Object affected by RateLimitPolicy [default/route-rlp gateway-system/section-rlp gateway-system/gw-rlp]
        observedGeneration: 1
        reason: Accepted
        status: "True"
        type: kuadrant.io/RateLimitPolicyAffected
    controllerName: kuadrant.io/policy-controller
    parentRef:
      group: gateway.networking.k8s.io
      kind: Gateway
      name: kuadrant-ingressgateway
      namespace: gateway-system

maleck13 · 2024-11-05T09:59:47Z

Thanks @KevFan looks good. I say merge it

…onciler Signed-off-by: KevFan <[email protected]>

…reconciler Signed-off-by: KevFan <[email protected]>

Signed-off-by: KevFan <[email protected]>

* refactor: initial target status -> gateway policy discoverability reconciler Signed-off-by: KevFan <[email protected]> * refactor: initial target status -> http route policy discoverability reconciler Signed-off-by: KevFan <[email protected]> * feat: policyAffectedBy condition is ListenerStatus Signed-off-by: KevFan <[email protected]> * refactor: list all policies in httproute affected by condition Signed-off-by: KevFan <[email protected]> * refactor: smaller function for gateway discoverability Signed-off-by: KevFan <[email protected]> * refactor: breakdown to methods/functions + integration tests Signed-off-by: KevFan <[email protected]> * refactor: methods/functions Signed-off-by: KevFan <[email protected]> * tests: additional tests for auth, dns and tls Signed-off-by: KevFan <[email protected]> * gomod: tidy Signed-off-by: KevFan <[email protected]> --------- Signed-off-by: KevFan <[email protected]>

KevFan force-pushed the discoverability branch from 0e94177 to d9829f5 Compare October 25, 2024 11:43

guicassolato reviewed Oct 28, 2024

View reviewed changes

guicassolato mentioned this pull request Oct 29, 2024

[state-of-the-world reconciler] DNSPolicy discoverability #827

Closed

KevFan force-pushed the discoverability branch 3 times, most recently from b6680c5 to ade9318 Compare November 1, 2024 20:59

KevFan marked this pull request as ready for review November 4, 2024 09:30

KevFan requested review from Boomatang and guicassolato November 4, 2024 09:30

KevFan self-assigned this Nov 4, 2024

KevFan force-pushed the discoverability branch from ade9318 to 5af7788 Compare November 4, 2024 09:57

Boomatang reviewed Nov 4, 2024

View reviewed changes

Boomatang approved these changes Nov 5, 2024

View reviewed changes

KevFan force-pushed the discoverability branch from 8098ca7 to 70ddfdd Compare November 5, 2024 11:18

KevFan added 8 commits November 5, 2024 11:27

refactor: initial target status -> gateway policy discoverability rec…

f89c47b

…onciler Signed-off-by: KevFan <[email protected]>

refactor: initial target status -> http route policy discoverability …

69d03bb

…reconciler Signed-off-by: KevFan <[email protected]>

feat: policyAffectedBy condition is ListenerStatus

6620832

Signed-off-by: KevFan <[email protected]>

refactor: list all policies in httproute affected by condition

a58c0bf

Signed-off-by: KevFan <[email protected]>

refactor: smaller function for gateway discoverability

7429fe2

Signed-off-by: KevFan <[email protected]>

refactor: breakdown to methods/functions + integration tests

32da6f6

Signed-off-by: KevFan <[email protected]>

refactor: methods/functions

5015e8d

Signed-off-by: KevFan <[email protected]>

tests: additional tests for auth, dns and tls

b8a499a

Signed-off-by: KevFan <[email protected]>

KevFan force-pushed the discoverability branch from 70ddfdd to b8a499a Compare November 5, 2024 11:28

gomod: tidy

1960679

Signed-off-by: KevFan <[email protected]>

KevFan requested a review from Boomatang November 5, 2024 12:02

KevFan merged commit 87a5613 into Kuadrant:main Nov 5, 2024
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: target status -> discoverability reconciler #958

refactor: target status -> discoverability reconciler #958

KevFan commented Oct 25, 2024 •

edited

Loading

guicassolato Oct 28, 2024

KevFan Oct 29, 2024 •

edited

Loading

guicassolato Oct 28, 2024

Boomatang left a comment

KevFan commented Nov 4, 2024

maleck13 commented Nov 5, 2024

Boomatang left a comment

mikenairn commented Nov 5, 2024

KevFan commented Nov 5, 2024

maleck13 commented Nov 5, 2024

refactor: target status -> discoverability reconciler #958

refactor: target status -> discoverability reconciler #958

Conversation

KevFan commented Oct 25, 2024 • edited Loading

Description

Verification

guicassolato Oct 28, 2024

Choose a reason for hiding this comment

KevFan Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

guicassolato Oct 28, 2024

Choose a reason for hiding this comment

Boomatang left a comment

Choose a reason for hiding this comment

KevFan commented Nov 4, 2024

maleck13 commented Nov 5, 2024

Boomatang left a comment

Choose a reason for hiding this comment

mikenairn commented Nov 5, 2024

KevFan commented Nov 5, 2024

maleck13 commented Nov 5, 2024

KevFan commented Oct 25, 2024 •

edited

Loading

KevFan Oct 29, 2024 •

edited

Loading