Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kap remove hosts from authscheme #99

Merged
merged 1 commit into from
Nov 11, 2022
Merged

Conversation

eguzki
Copy link
Contributor

@eguzki eguzki commented Nov 8, 2022

what

  • Implementation of "hosts field not exposed in the AuthPolicy"

Fixes #97

  • Adding e2e tests
  • Fix documentation authpolicy examples

verification steps

Setup env

make local-setup

Create HTTPRoute for *.toystore.com

kubectl apply -f - <<EOF
---
apiVersion: gateway.networking.k8s.io/v1alpha2
kind: HTTPRoute
metadata:
  name: toystore
  labels:
    app: toystore
spec:
  parentRefs:
    - name: istio-ingressgateway
      namespace: istio-system
  hostnames: ["*.toystore.com"]
  rules:
    - matches:
        - path:
            type: PathPrefix
            value: "/toy"
          method: GET
      backendRefs:
        - name: toystore
          port: 80
EOF

Create a kuadrant AuthPolicy in which one of the rules the hosts field is missing

kubectl apply -f - <<EOF
---
apiVersion: kuadrant.io/v1beta1
kind: AuthPolicy
metadata:
  name: toystore
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: toystore
  rules:
  - hosts: ["*.admin.toystore.com"]
    methods: ["DELETE", "POST"]
    paths: ["/admin*"]
  - methods: ["DELETE", "POST"]
    paths: ["/private*"]
  authScheme:
    identity:
    - name: friends
      apiKey:
        selector:
          matchLabels:        
            group: friends
      credentials:
        in: authorization_header
        keySelector: APIKEY
EOF

Check that the authconfig object's hosts is set to the route's hostnames

k get authconfig ap-default-toystore -n kuadrant-system  -o jsonpath='{.spec.hosts}'
["*.toystore.com"]

Check that the Istio's authorizationpolicy's rules' hosts all fall under route's hostnames

k get authorizationpolicy on-istio-ingressgateway-using-toystore -n istio-system -o yaml | yq e -P
apiVersion: security.istio.io/v1beta1
kind: AuthorizationPolicy
metadata:
  creationTimestamp: "2022-09-27T15:26:18Z"
  generation: 1
  name: on-istio-ingressgateway-using-toystore
  namespace: istio-system
  resourceVersion: "8945"
  uid: 353aa299-97a9-4459-a293-20d03d1185d3
spec:
  action: CUSTOM
  provider:
    name: kuadrant-authorization
  rules:
    - to:
        - operation:
            hosts:
              - '*.admin.toystore.com'
            methods:
              - DELETE
              - POST
            paths:
              - /admin*
        - operation:
            hosts:
              - '*.toystore.com'
            methods:
              - DELETE
              - POST
            paths:
              - /private*
  selector: {}

@eguzki eguzki force-pushed the kap-remove-hosts-from-authscheme branch from d7ab64b to 4aa83aa Compare November 8, 2022 15:54
@eguzki eguzki marked this pull request as ready for review November 8, 2022 16:26
@eguzki eguzki requested a review from a team November 8, 2022 16:27
@eguzki eguzki force-pushed the kap-remove-hosts-from-authscheme branch from 4aa83aa to 8ab9d5e Compare November 9, 2022 16:47
@eguzki eguzki force-pushed the kap-remove-hosts-from-authscheme branch from 8ab9d5e to 8fe7e38 Compare November 9, 2022 16:50
@eguzki eguzki removed the request for review from alexsnaps November 9, 2022 16:50
Comment on lines +344 to +345
// When one of the rules does not have hosts, just return target hostnames
return r.TargetHostnames(ctx, ap.Spec.TargetRef, ap.Namespace)
Copy link
Member

@didierofrivia didierofrivia Nov 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what happens when the next idx of the AuthRules has Hosts ? those would be ignored right? and used the TargetHostnames only

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Eguz for the explanation, so if there's at least one rule without the hostname, the authConfig will include the parent targetHostnames since it's the more global one.

Copy link
Member

@didierofrivia didierofrivia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only detail that in the case that makes sense, could be include in a future PR

@eguzki eguzki merged commit c1a7244 into main Nov 11, 2022
@eguzki eguzki deleted the kap-remove-hosts-from-authscheme branch November 11, 2022 07:58
mikenairn pushed a commit to mikenairn/kuadrant-operator that referenced this pull request Mar 23, 2023
* ratelimitpolicy: new action types

* add action types to example

* ratelimitpolicy example fix metadata action stage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hosts field not exposed in the AuthPolicy
2 participants