PolicyAffected condition for AuthPolicy and RateLimitPolicy #536

guicassolato · 2024-04-11T14:48:55Z

PolicyAffected status condition added to the Gateway objects for AuthPolicy and RateLimitPolicy
PolicyAffected status condition added to HTTPRoute objects for AuthPolicy and RateLimitPolicy

Closes #467 and #468

Verification steps

Setup (1-off step)

Create the cluster:

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

Deploy the sample app:

kubectl create namespace toystore

kubectl apply -n toystore -f examples/toystore/toystore.yaml

kubectl apply -n toystore -f - <<EOF
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: api
spec:
  parentRefs:
  - name: istio-ingressgateway
    namespace: istio-system
  hostnames:
  - api.toys.io
  rules:
  - backendRefs:
    - name: toystore
      port: 80
---
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: admin
spec:
  parentRefs:
  - name: istio-ingressgateway
    namespace: istio-system
  hostnames:
  - admin.toys.io
  rules:
  - backendRefs:
    - name: toystore
      port: 80
EOF

Check the state (repeat before and after every step of creating/deleting a policy)

Check the state conditions of the Gateway and HTTPRoutes:

kubectl get -n istio-system gateway/istio-ingressgateway -o jsonpath='{.status.conditions}' | jq
kubectl get -n toystore httproute/api -o jsonpath='{.status.parents[?(@.controllerName=="kuadrant.io/policy-controller")].conditions }' | jq
kubectl get -n toystore httproute/admin -o jsonpath='{.status.parents[?(@.controllerName=="kuadrant.io/policy-controller")].conditions }' | jq

Check the enforcement of the policies by sending requests to the app:

kubectl port-forward -n istio-system service/istio-ingressgateway-istio 8443:443 2>&1 >/dev/null &

To the api route without authentication:

host=api.toys.io; while :; do curl --write-out '%{http_code}\n' --silent --output /dev/null -k --resolve $host:8443:127.0.0.1 -H "Host: $host" https://$host:8443 | grep -E --color "\b(4\d\d)\b|$"; sleep 1; done

To the api route with authentication:

host=api.toys.io; while :; do curl --write-out '%{http_code}\n' --silent --output /dev/null -k --resolve $host:8443:127.0.0.1 -H "Host: $host" -H 'Authorization: APIKEY IAMALICE' https://$host:8443 | grep -E --color "\b(4\d\d)\b|$"; sleep 1; done

To the admin route without authentication:

host=admin.toys.io; while :; do curl --write-out '%{http_code}\n' --silent --output /dev/null -k --resolve $host:8443:127.0.0.1 -H "Host: $host" https://$host:8443 | grep -E --color "\b(4\d\d)\b|$"; sleep 1; done

Create/delete policies

❶ Create TLSPolicy

kubectl apply -n istio-system -f - <<EOF
apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
  name: istio-ingressgateway
spec:
  gatewayClassName: istio
  listeners:
  - name: http
    hostname: '*.toys.io'
    port: 443
    protocol: HTTPS
    tls:
      mode: Terminate
      certificateRefs:
      - name: toys
        kind: Secret
    allowedRoutes:
      namespaces:
        from: All    
---
apiVersion: cert-manager.io/v1
kind: Issuer
metadata:
  name: selfsigned-issuer
spec:
  selfSigned: {}
---
apiVersion: kuadrant.io/v1alpha1
kind: TLSPolicy
metadata:
  name: gw-tls
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: Gateway
    name: istio-ingressgateway
  issuerRef:
    group: cert-manager.io
    kind: Issuer
    name: selfsigned-issuer
EOF

❷ Create Gateway default AuthPolicy

kubectl apply -f  - <<EOF
apiVersion: kuadrant.io/v1beta2
kind: AuthPolicy
metadata:
  name: default-deny
  namespace: istio-system
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: Gateway
    name: istio-ingressgateway
  defaults:
    rules:
      authorization:
        "deny":
          opa:
            rego: "allow = false"
EOF

❸ Create Route AuthPolicy

kubectl apply -f  - <<EOF
apiVersion: kuadrant.io/v1beta2
kind: AuthPolicy
metadata:
  name: api-authn
  namespace: toystore
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: api
  rules:
    authentication:
      "api-key":
        apiKey:
          selector:
            matchLabels:
              app: toys
        credentials:
          authorizationHeader:
            prefix: APIKEY
---
apiVersion: v1
kind: Secret
metadata:
  name: alice-api-key
  namespace: toystore
  labels:
    authorino.kuadrant.io/managed-by: authorino
    app: toys
stringData:
  api_key: IAMALICE
EOF

❹ Create Route RateLimitPolicy

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

❺ Create Gateway override RateLimitPolicy

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

❻ Delete Gateway default AuthPolicy

kubectl delete -n istio-system authpolicy/default-deny

❼ Delete Route AuthPolicy

kubectl delete -n toystore authpolicy/api-authn

❽ Delete Route RateLimitPolicy

kubectl delete -n toystore ratelimitpolicy/high-limit-api

❾ Delete Gateway override RateLimitPolicy

kubectl delete -n istio-system ratelimitpolicy/low-limit

Expected states

Step	Gateway	Route
Step	istio-system/istio-ingressgateway	toystore/api	toystore/admin
Initial	∅	∅	∅
❶ Create TLSPolicy istio-system/gw-tls	`kuadrant.io/TLSPolicyAffected` ^{Object affected by TLSPolicy istio-system/gw-tls}	∅	∅
❷ Create AuthPolicy istio-system/default-deny	`kuadrant.io/TLSPolicyAffected` ^{Object affected by TLSPolicy istio-system/gw-tls} `kuadrant.io/AuthPolicyAffected` ^{Object affected by AuthPolicy istio-system/default-deny}	`kuadrant.io/AuthPolicyAffected` ^{Object affected by AuthPolicy istio-system/default-deny}	`kuadrant.io/AuthPolicyAffected` ^{Object affected by AuthPolicy istio-system/default-deny}
❸ Create AuthPolicy toystore/api-authn	`kuadrant.io/TLSPolicyAffected` ^{Object affected by TLSPolicy istio-system/gw-tls} `kuadrant.io/AuthPolicyAffected` ^{Object affected by AuthPolicy istio-system/default-deny}	`kuadrant.io/AuthPolicyAffected` ^{Object affected by AuthPolicy toystore/api-authn}	`kuadrant.io/AuthPolicyAffected` ^{Object affected by AuthPolicy istio-system/default-deny}
❹ Create RateLimitPolicy toystore/high-limit-api	`kuadrant.io/TLSPolicyAffected` ^{Object affected by TLSPolicy istio-system/gw-tls} `kuadrant.io/AuthPolicyAffected` ^{Object affected by AuthPolicy istio-system/default-deny}	`kuadrant.io/AuthPolicyAffected` ^{Object affected by AuthPolicy toystore/api-authn} `kuadrant.io/RateLimitPolicyAffected` ^{Object affected by RateLimitPolicy toystore/high-limit-api}	`kuadrant.io/AuthPolicyAffected` ^{Object affected by AuthPolicy istio-system/default-deny}
❺ Create RateLimitPolicy istio-system/low-limit	`kuadrant.io/TLSPolicyAffected` ^{Object affected by TLSPolicy istio-system/gw-tls} `kuadrant.io/AuthPolicyAffected` ^{Object affected by AuthPolicy istio-system/default-deny} `kuadrant.io/RateLimitPolicyAffected` ^{Object affected by RateLimitPolicy istio-system/low-limit}	`kuadrant.io/AuthPolicyAffected` ^{Object affected by AuthPolicy toystore/api-authn} `kuadrant.io/RateLimitPolicyAffected` ^{Object affected by RateLimitPolicy toystore/high-limit-api ⚠️}	`kuadrant.io/AuthPolicyAffected` ^{Object affected by AuthPolicy istio-system/default-deny} `kuadrant.io/RateLimitPolicyAffected` ^{Object affected by RateLimitPolicy istio-system/low-limit}
❻ Delete AuthPolicy istio-system/default-deny	`kuadrant.io/TLSPolicyAffected` ^{Object affected by TLSPolicy istio-system/gw-tls} `kuadrant.io/RateLimitPolicyAffected` ^{Object affected by RateLimitPolicy istio-system/low-limit}	`kuadrant.io/AuthPolicyAffected` ^{Object affected by AuthPolicy toystore/api-authn} `kuadrant.io/RateLimitPolicyAffected` ^{Object affected by RateLimitPolicy toystore/high-limit-api ⚠️}	`kuadrant.io/RateLimitPolicyAffected` ^{Object affected by RateLimitPolicy istio-system/low-limit ⚠️}
❼ Delete AuthPolicy toystore/api-authn	`kuadrant.io/TLSPolicyAffected` ^{Object affected by TLSPolicy istio-system/gw-tls} `kuadrant.io/RateLimitPolicyAffected` ^{Object affected by RateLimitPolicy istio-system/low-limit}	`kuadrant.io/RateLimitPolicyAffected` ^{Object affected by RateLimitPolicy toystore/high-limit-api}	`kuadrant.io/RateLimitPolicyAffected` ^{Object affected by RateLimitPolicy istio-system/low-limit}
❽ Delete RateLimitPolicy toystore/high-limit-api	`kuadrant.io/TLSPolicyAffected` ^{Object affected by TLSPolicy istio-system/gw-tls} `kuadrant.io/RateLimitPolicyAffected` ^{Object affected by RateLimitPolicy istio-system/low-limit}	`kuadrant.io/RateLimitPolicyAffected` ^{Object affected by RateLimitPolicy istio-system/low-limit}	`kuadrant.io/RateLimitPolicyAffected` ^{Object affected by RateLimitPolicy istio-system/low-limit}
❾ Delete RateLimitPolicy istio-system/low-limit	`kuadrant.io/TLSPolicyAffected` ^{Object affected by TLSPolicy istio-system/gw-tls}	∅	∅

⚠️ The actual RateLimitPolicy being enforced is istio-system/low-limit. Check the status of toystore/high-limit-api to verify it's being overriden by the gateway policy.

codecov · 2024-04-11T14:51:41Z

Codecov Report

Merging #536 (30cd6be) into main (ece13e8) will decrease coverage by 48.26%.
Report is 48 commits behind head on main.
The diff coverage is 7.21%.

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #536       +/-   ##
===========================================
- Coverage   80.20%   31.95%   -48.26%     
===========================================
  Files          64       65        +1     
  Lines        4492     4913      +421     
===========================================
- Hits         3603     1570     -2033     
- Misses        600     3250     +2650     
+ Partials      289       93      -196

Flag	Coverage Δ
integration	`?`
unit	`31.95% <7.21%> (+1.92%)`	⬆️

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

Components	Coverage Δ
api/v1beta1 (u)	`∅ <ø> (∅)`
api/v1beta2 (u)	`47.43% <53.33%> (-44.00%)`	⬇️
pkg/common (u)	`77.47% <ø> (-11.35%)`	⬇️
pkg/istio (u)	`44.37% <ø> (-29.54%)`	⬇️
pkg/log (u)	`36.84% <ø> (-57.90%)`	⬇️
pkg/reconcilers (u)	`∅ <ø> (∅)`
pkg/rlptools (u)	`57.25% <ø> (-22.21%)`	⬇️
controllers (i)	`7.23% <0.00%> (-69.58%)`	⬇️

Files	Coverage Δ
controllers/kuadrant_controller.go	`0.00% <ø> (-53.58%)`	⬇️
controllers/kuadrant_status.go	`0.00% <ø> (-71.30%)`	⬇️
pkg/library/gatewayapi/types.go	`100.00% <ø> (ø)`
pkg/library/gatewayapi/utils.go	`63.76% <100.00%> (ø)`
pkg/library/kuadrant/kuadrant.go	`72.94% <ø> (-27.06%)`	⬇️
pkg/library/utils/slice_utils.go	`82.97% <100.00%> (-17.03%)`	⬇️
controllers/authpolicy_authconfig.go	`33.04% <0.00%> (-52.02%)`	⬇️
...ontrollers/authpolicy_istio_authorizationpolicy.go	`27.18% <0.00%> (-60.14%)`	⬇️
pkg/library/mappers/policy_to_gateway.go	`0.00% <0.00%> (ø)`
controllers/dnspolicy_controller.go	`0.00% <0.00%> (-83.48%)`	⬇️
... and 8 more

... and 35 files with indirect coverage changes

guicassolato · 2024-04-11T15:15:39Z

controllers/kuadrant_status.go

@@ -196,7 +195,7 @@ func BuildPolicyAffectedCondition(conditionType string, policyObject runtime.Obj
 		return condition
 	}

-	condition.Message = fmt.Sprintf("policy success. Object affected by policy %s in namespace %s with name %s ", policyObject.GetObjectKind().GroupVersionKind().String(), objectMeta.GetNamespace(), objectMeta.GetName())
+	condition.Message = fmt.Sprintf("policy success. Object affected by policy %s in namespace %s with name %s", policyObject.GetObjectKind().GroupVersionKind().String(), objectMeta.GetNamespace(), objectMeta.GetName())


@mikenairn @maleck13, how important would you say having the name of the policy in the message here is?

I guess for gateway policies – which is always the case of DNSPolicy and TLSPolicy – it's probably good?

This doesn't work well though when the gateway can be affected by more than one policy, such as with AuthPolicies and RateLimitPolicies, that can affect gateways by targeting the routes as well, or, in general, when more than one policy of a kind can target the Gateway (not allowed today).

Currently, the message contains the name of the last policy which caused the PolicyAffected condition to be set (to either true or false.) We also have the full list of policies that affect a gateway stored in the back ref annotation of the gateway object.

I guess my actual question is: are we OK with only the last policy mentioned in the message for each and all kinds of policies or I may change the message to make it more generic without mentioning the name of the policy?

Interesting question. I don't yet have a strong opinion. If the policy is truly affecting the gateway and not being overridden, does it not make sense to have multiple conditions perhaps at the listener and top level? I guess it comes down to what we want to achieve with the condition. Is it that we want to indicate this gateway is affected by at least one policy or do we want a pointer to say these specific policies are affecting this gateway. Sorry for the late reply

Ended applying the following:

If only a policy targeting the object indirectly exists (i.e. Gateway policy with no route policy) → this policy will be referred in the PolicyAffected message

If a policy targeting the object directly exists → this policy will be referred in the PolicyAffected message even if the policy is being overridden by another. To confirm if a policy referred in PolicyAffected message is being enforced, the user must check the status of the policy object itself.

guicassolato · 2024-04-11T15:28:43Z

pkg/library/reconcilers/target_ref_reconciler.go

+			if err := r.Client.Status().Update(ctx, gw.Gateway); err != nil {
+				return err
+			}
+			// TODO(guicassolato): for inherited policies only – add the policyaffetced status condition on all routes accepted by the gateway


This is assuming that, no matter at level the policy targets an object (gateway or route), all objects (gateways and routes) will be marked as affected by the policy.

Is that really what we want or we want to set this condition only on the object targeted by the policy plus its children (but never upwards)?

cc @maleck13 @R-Lawton @david-martin

I would go for the latter. It makes more sense to me this way

Ok. I need to change a few things then. Thanks!

Changed so direct policies (TLS/DNS) only add to the statuses of the objects they target (Gateways only) and inherited (auth/RL) to the objects they target plus children beneath.

guicassolato · 2024-04-12T17:00:48Z

pkg/library/reconcilers/target_ref_reconciler.go

+			ControllerName: controllerName,
+			ParentRef: gatewayapiv1.ParentReference{
+				Kind:      ptr.To(gatewayapiv1.Kind(gateway.Kind)),
+				Name:      gatewayapiv1.ObjectName(gateway.GetName()),
+				Namespace: ptr.To(gatewayapiv1.Namespace(gateway.GetNamespace())),
+			},


This will create in the route a parent status block with controllerName: kuadrant.io/policy-controller and targetRef pointing to the affected gateway.

One can only assume what the actual controllerName of the gateway is, but this information is omitted here.

Should we instead reuse the same parent status block of the gateway controller and not have one specifically for Kuadrant?

guicassolato · 2024-04-21T08:26:17Z

controllers/target_status_controller.go

+}
+
+func (r *TargetStatusReconciler) Reconcile(eventCtx context.Context, req ctrl.Request) (ctrl.Result, error) {
+	logger := r.Logger().WithValues("gateway", req.NamespacedName, "request id", uuid.NewString())


Thinking about adding request id at all reconcilers. WDYT?

I was also thinking about this but wondering should it follow the guidelines set in opentelemetry, but I am not sure if our current approach fits into that style if parent ids.

Actually, I may have seen reconcileID in some other log trail. No longer sure if this request id will be useful in the end.

Boomatang · 2024-04-22T15:42:03Z

Everything looks good to me. That said I could only run the verification steps for the AuthPolicies. I have a local issue with RateLimiting, unrelated to the changes in this PR. You will need to find someone to do the full verification.

guicassolato · 2024-04-22T18:35:11Z

Verification steps updated in the description of the PR to:

simplify the topology (2 routes instead of 3);
add a step for the TLSPolicy (DNSPolicy should also work);
include curls to the app (to check as well the enforcement of the policy – optional.)

…s for AuthPolicy and RateLimitPolicy

… the gateway controller name Removes the need for authorizing reading gateway classes

…tus changed only

…policy status changed only based on simple predicate function instead of custom handler

KevFan

Verified, works perfectly 💯
Have a minor comment / question on integration test that I'm happy for it to be addressed at a later PR if it's something that was missed since it's very minor 👍

controllers/target_status_controller_test.go

…by the routes

guicassolato · 2024-04-23T12:36:42Z

I’m merging this in spite of the integration test failing. It’s unrelated to the PR. In fact, similar ones are currently failing in main as well.

* PolicyAffected status condition added to Gateway and HTTPRoute objects for AuthPolicy and RateLimitPolicy * refactor: fetch routes accepted by a given gateway without specifying the gateway controller name Removes the need for authorizing reading gateway classes * Trigger target status reconciliation for policies when the policy status changed only * refactor: trigger target status reconciliation for policies when the policy status changed only based on simple predicate function instead of custom handler * tests: fixed integration test for policyaffected condition inherited by the routes

guicassolato self-assigned this Apr 11, 2024

guicassolato commented Apr 11, 2024

View reviewed changes

guicassolato force-pushed the policy-affected branch 5 times, most recently from 67aa611 to faf82b0 Compare April 12, 2024 16:53

guicassolato commented Apr 12, 2024

View reviewed changes

guicassolato force-pushed the policy-affected branch 14 times, most recently from 78e0e42 to 850b412 Compare April 20, 2024 10:42

guicassolato marked this pull request as ready for review April 20, 2024 13:20

guicassolato requested a review from a team as a code owner April 20, 2024 13:20

guicassolato commented Apr 21, 2024

View reviewed changes

guicassolato force-pushed the policy-affected branch 3 times, most recently from 26fd172 to adb681a Compare April 21, 2024 08:37

guicassolato force-pushed the policy-affected branch 2 times, most recently from 8c139f5 to 92bc448 Compare April 22, 2024 14:05

guicassolato force-pushed the policy-affected branch from 92bc448 to 3dbe4d4 Compare April 22, 2024 18:18

guicassolato added 4 commits April 23, 2024 10:02

PolicyAffected status condition added to Gateway and HTTPRoute object…

e921274

…s for AuthPolicy and RateLimitPolicy

refactor: fetch routes accepted by a given gateway without specifying…

5609855

… the gateway controller name Removes the need for authorizing reading gateway classes

Trigger target status reconciliation for policies when the policy sta…

6aac8a3

…tus changed only

refactor: trigger target status reconciliation for policies when the …

e7e8b38

…policy status changed only based on simple predicate function instead of custom handler

guicassolato force-pushed the policy-affected branch from 3dbe4d4 to e7e8b38 Compare April 23, 2024 08:02

KevFan approved these changes Apr 23, 2024

View reviewed changes

controllers/target_status_controller_test.go Outdated Show resolved Hide resolved

tests: fixed integration test for policyaffected condition inherited …

30cd6be

…by the routes

guicassolato merged commit bed695f into main Apr 23, 2024
18 of 21 checks passed

guicassolato deleted the policy-affected branch April 23, 2024 12:36

guicassolato mentioned this pull request Apr 23, 2024

AuthPolicy discoverability #468

Closed

martinhesko mentioned this pull request Apr 23, 2024

Atomic defaults tests Kuadrant/testsuite#375

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PolicyAffected condition for AuthPolicy and RateLimitPolicy #536

PolicyAffected condition for AuthPolicy and RateLimitPolicy #536

guicassolato commented Apr 11, 2024 •

edited

Loading

codecov bot commented Apr 11, 2024 •

edited

Loading

guicassolato Apr 11, 2024

maleck13 Apr 15, 2024

guicassolato Apr 22, 2024

guicassolato Apr 11, 2024

maleck13 Apr 16, 2024

guicassolato Apr 16, 2024

guicassolato Apr 22, 2024

guicassolato Apr 12, 2024

guicassolato Apr 21, 2024

Boomatang Apr 22, 2024

guicassolato Apr 22, 2024

Boomatang commented Apr 22, 2024

guicassolato commented Apr 22, 2024

KevFan left a comment •

edited

Loading

guicassolato commented Apr 23, 2024

PolicyAffected condition for AuthPolicy and RateLimitPolicy #536

PolicyAffected condition for AuthPolicy and RateLimitPolicy #536

Conversation

guicassolato commented Apr 11, 2024 • edited Loading

Verification steps

Setup (1-off step)

Check the state (repeat before and after every step of creating/deleting a policy)

Check the state conditions of the Gateway and HTTPRoutes:

Check the enforcement of the policies by sending requests to the app:

Create/delete policies

❶ Create TLSPolicy

❷ Create Gateway default AuthPolicy

❸ Create Route AuthPolicy

❹ Create Route RateLimitPolicy

❺ Create Gateway override RateLimitPolicy

❻ Delete Gateway default AuthPolicy

❼ Delete Route AuthPolicy

❽ Delete Route RateLimitPolicy

❾ Delete Gateway override RateLimitPolicy

Expected states

codecov bot commented Apr 11, 2024 • edited Loading

Codecov Report

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

Boomatang commented Apr 22, 2024

guicassolato commented Apr 22, 2024

KevFan left a comment • edited Loading

Choose a reason for hiding this comment

guicassolato commented Apr 23, 2024

guicassolato commented Apr 11, 2024 •

edited

Loading

codecov bot commented Apr 11, 2024 •

edited

Loading

KevFan left a comment •

edited

Loading