[authpolicy-v2] route selectors

[guicassolato]

Closes:

• Add routeSelectors fields to the AuthPolicy #248

TODOs

• Merge the hostnames of HTTPRoute (direct or inherited from the Gateway) into all Istio AuthorizationPolicy rules that do not include hostnames already built from the route selectors
• Fix Authorino conditions built out of multiple HTTPRouteMatches → matches should be OR'ed together instead of AND'ed → requires Logical disjunction (aka "OR" operator) in conditions authorino#424
• Trigger reconciliation of Gateway-targeting policies after finishing reconciling HTTPRoute ones - to workaround AuthorizationPolicies and AuthConfigs generated under race condition
• Implement or skip rule matches based on regexp types when building Authorino conditions and Istio AuthorizationPolicies rules
  • Authorino conditions for query string params from GW-API HTTPRouteMatches with QueryParamMatchRegularExpression type
  • Istio AuthorizationPolicies rules for path and headers from GW-API HTTPRouteMatches with PathMatchRegularExpression and HeaderMatchRegularExpression types respectively
• Report (in the logs, status, etc) about route selectors that match no HTTPRouteRule (separate PR)

Verification steps

Setup

① Create the cluster, install & deploy Kuadrant:

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 a sample API:

kubectl apply -f examples/toystore/toystore.yaml
kubectl apply -f - <<EOF
apiVersion: gateway.networking.k8s.io/v1beta1
kind: HTTPRoute
metadata:
  name: toystore
spec:
  parentRefs:
  - name: istio-ingressgateway
    namespace: istio-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

(Optional) Test the API unprotected:

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

curl -H 'Host: api.toystore.com' http://localhost:9080/dolls -i
# HTTP/1.1 200 OK

curl -H 'Host: api.toystore.com' http://localhost:9080/admin -i
# HTTP/1.1 200 OK

Create AuthPolicies

④ Create a trivial AuthPolicy:

kubectl apply -f - <<EOF
apiVersion: kuadrant.io/v1beta2
kind: AuthPolicy
metadata:
  name: toystore
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: toystore
EOF

⑤ Create an AuthPolicy with auth rules:

kubectl apply -f - <<EOF
apiVersion: kuadrant.io/v1beta2
kind: AuthPolicy
metadata:
  name: toystore
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: toystore
  rules:
    authentication:
      "api-key-users":
        apiKey:
          selector:
            matchLabels:
              app: toystore
          allNamespaces: true
        credentials:
          authorizationHeader:
            prefix: APIKEY
EOF

⑥ Create an AuthPolicy with top-level route selectors:

kubectl apply -f - <<EOF
apiVersion: kuadrant.io/v1beta2
kind: AuthPolicy
metadata:
  name: toystore
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: toystore
  routeSelectors:
  - matches:
    - path:
        type: PathPrefix
        value: "/admin"
  rules:
    authentication:
      "api-key-users":
        apiKey:
          selector:
            matchLabels:
              app: toystore
          allNamespaces: true
        credentials:
          authorizationHeader:
            prefix: APIKEY
EOF

⑦ Create an AuthPolicy with config route selectors:

kubectl apply -f - <<EOF
apiVersion: kuadrant.io/v1beta2
kind: AuthPolicy
metadata:
  name: toystore
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: toystore
  rules:
    authentication:
      "api-key-users":
        apiKey:
          selector:
            matchLabels:
              app: toystore
          allNamespaces: true
        credentials:
          authorizationHeader:
            prefix: APIKEY
        routeSelectors:
        - matches:
          - path:
              type: PathPrefix
              value: "/admin"
EOF

After each step ④ → ⑦ above,

❶ Check the state of the Istio AuthorizationPolicy:

kubectl get authorizationpolicy/on-istio-ingressgateway-using-toystore -n istio-system -o yaml

❷ Check the state of the Authorino AuthConfig:

kubectl get authconfig/ap-default-toystore -o yaml

❸ Send requests to the 3 endpoints of the API:

curl -H 'Host: api.toystore.com' http://localhost:9080/cars -i

curl -H 'Host: api.toystore.com' http://localhost:9080/dolls -i

curl -H 'Host: api.toystore.com' http://localhost:9080/admin -i

[codecov]

Codecov Report

Merging #256 (a3ea6b1) into authpolicy-v2 (702e821) will increase coverage by 1.00%.
The diff coverage is 68.61%.

@@                Coverage Diff                @@
##           authpolicy-v2     #256      +/-   ##
=================================================
+ Coverage          62.76%   63.77%   +1.00%     
=================================================
  Files                 34       35       +1     
  Lines               3242     3776     +534     
=================================================
+ Hits                2035     2408     +373     
- Misses              1036     1172     +136     
- Partials             171      196      +25     

Flag	Coverage Δ
integration	69.45% <68.07%> (-0.22%)	⬇️
unit	57.14% <72.50%> (+0.79%)	⬆️

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

Components	Coverage Δ
api/v1beta1 (u)	∅ <ø> (∅)
pkg/common (u)	73.92% <0.00%> (-1.98%)	⬇️
pkg/istio (u)	30.24% <ø> (ø)
pkg/log (u)	31.81% <ø> (ø)
pkg/reconcilers (u)	33.68% <ø> (ø)
pkg/rlptools (u)	56.41% <100.00%> (-1.23%)	⬇️
controllers (i)	69.45% <68.07%> (-0.22%)	⬇️

Files	Coverage Δ
api/v1beta2/authpolicy_types.go	61.03% <100.00%> (+55.77%)	⬆️
api/v1beta2/ratelimitpolicy_types.go	26.86% <100.00%> (+4.64%)	⬆️
api/v1beta2/route_selectors.go	100.00% <100.00%> (ø)
pkg/common/common.go	88.39% <ø> (ø)
pkg/rlptools/wasm_utils.go	61.05% <100.00%> (-1.77%)	⬇️
controllers/authpolicy_status.go	94.44% <0.00%> (+8.33%)	⬆️
controllers/authpolicy_controller.go	70.27% <70.00%> (+5.42%)	⬆️
controllers/httprouteparentrefs_eventmapper.go	53.19% <53.19%> (ø)
pkg/common/gatewayapi_utils.go	62.82% <0.00%> (-3.11%)	⬇️
...ntrollers/authpolicy_istio_authorization_policy.go	69.84% <75.13%> (+9.32%)	⬆️
... and 1 more

... and 3 files with indirect coverage changes

[Boomatang]

Verification steps work and nothing in the code is standing.

[Boomatang]

I have seen this in a few places and I don't have a problem with it but my question is more of a why. The ptr.To returns a pointer to what ever objects but putting & before the object would be the same and is what ptr.To does. Why bring in the extra dependency? It feels very like the js isOdd saga.

We may have found a bug that needs clearing up

[adam-cattermole]

Initially I was not 100% on the intended behaviour of the AuthPolicy in step 7 of the verification as it didn't seem entirely intuitive but after some more investigation and discussion with @Boomatang and @alexsnaps I'm happy that it's expected. Code all looks good as far as I can tell also.

[maleck13]

This seems odd. Are we not dropping errors and reconcile requests being returned from this this way? Should it be done as part of a Watches in the NewControllerManagedBy ?

We have something "like this "

Watches(&source.Kind{Type: &workv1.ManifestWork{}}, handler.EnqueueRequestsFromMapFunc(func(o client.Object) []reconcile.Request {
			log.V(3).Info("enqueuing gateways based on manifest work change ", "work namespace", o.GetNamespace())
			requests := []reconcile.Request{}
			annotations := o.GetAnnotations()
			if annotations == nil {
				log.V(3).Info("no parent or anotations on manifest work ", "work ns", o.GetNamespace(), "name", o.GetName())
				return requests
			}
			key := annotations["kuadrant.io/parent"]
			ns, name, err := cache.SplitMetaNamespaceKey(key)
			if err != nil {
				log.Error(err, "failed to parse namespace and name from maifiest work")
				return requests
			}
			log.Info("requeuing gateway ", "namespace", ns, "name", name)
			requests = append(requests, reconcile.Request{
				NamespacedName: types.NamespacedName{Namespace: ns, Name: name},
			})
			return requests
		}), builder.OnlyMetadata).

[guicassolato]

What kind of resource should we watch and map to AuthPolicies, @maleck13?

The intent here is, after reconciling an AuthPolicy attached to a HTTPRoute, to trigger the reconciliation of all AuthPolicies attached to Gateways that are parents of the HTTPRoute.

A few constraints are:

1. Neither HTTPRoute nor the Gateways are touched during any of those reconciliation loops. Only the internal custom resources might (i.e. AuthorizationPolicy, AuthConfig.)
2. We do not want to rely on back-ref annotations for this, as order matters here and therefore we want the latest state possible.

[maleck13]

Right. Prob wrong place to start figuring this out here. It would seem we need to trigger an event on the HTTPRoute (status update / annotation) or we need to track the parent policies. Do we have an issue for looking into a better option? I know it was discussed. IE have an in mem structure that we maintain of the policy hierarchy. Trigger a reconcile on parents on child change (or something along those lines) so I guess if we have an issue for that we could settle with this for now?

[guicassolato]

I'll make sure to open one and add it here for ref.

[guicassolato]

Kuadrant/architecture#29

[maleck13]

I have given this a review as best I can without knowing the code base well. I have one question about how we are triggering a reconcile other than that it looks reasonable