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

feat: auth policy enforced condition #411

Merged
merged 4 commits into from
Feb 20, 2024
Merged

Conversation

KevFan
Copy link
Contributor

@KevFan KevFan commented Jan 31, 2024

Description

Part of #290 Kuadrant/architecture#38
Closes: #349

Add Enforced condition type to AuthPolicy.

Verification

Functionality is generally already tested with the integration tests added.

To verify manually instead:

Setup

  • Checkout this branch
  • Deploy using make local-setup
  • Create HTTPRoute
kubectl apply -f - <<EOF
apiVersion: gateway.networking.k8s.io/v1
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: "/toys"
    backendRefs:
    - name: toystore
      port: 80
EOF
  • Create AuthPolicy
kubectl apply -f - <<EOF
---
apiVersion: kuadrant.io/v1beta2
kind: AuthPolicy
metadata:
  name: auth0
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: toystore
  rules:
    authorization:
      deny-all:
        opa:
          rego: "allow = false"
    response:
      unauthorized:
        headers:
          "content-type":
            value: application/json
        body:
          value: |
            {
              "error": "Forbidden",
              "message": "Access denied by default by the gateway operator. If you are the administrator of the service, create a specific auth policy for the route."
            }
EOF
  • Verify status:
    • Accepted - True
    • Enforced - False
kubectl get authpolicy -A -o wide
kubectl get authpolicy auth0 -o yaml | yq '.status'

image

  • Deploy Kuadrant CR
kubectl apply -f - <<EOF
---                      
apiVersion: kuadrant.io/v1beta1
kind: Kuadrant
metadata:
  name: kuadrant-sample
spec: {}
EOF
  • Verify status:
    • Accepted - True
    • Enforced - True
kubectl get authpolicy -A -o wide
kubectl get authpolicy auth0 -o yaml | yq '.status'

image

@KevFan KevFan added kind/enhancement New feature or request area/api Changes user facing APIs labels Jan 31, 2024
@KevFan KevFan added this to the v0.7.0 milestone Jan 31, 2024
@KevFan KevFan self-assigned this Jan 31, 2024
Copy link

codecov bot commented Jan 31, 2024

Codecov Report

Merging #411 (3b71908) into main (c831a70) will decrease coverage by 0.60%.
The diff coverage is 74.59%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #411      +/-   ##
==========================================
- Coverage   66.18%   65.59%   -0.60%     
==========================================
  Files          38       38              
  Lines        3901     3991      +90     
==========================================
+ Hits         2582     2618      +36     
- Misses       1131     1179      +48     
- Partials      188      194       +6     
Flag Coverage Δ
integration 70.06% <81.25%> (-1.18%) ⬇️
unit 60.79% <67.24%> (+0.09%) ⬆️

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

Components Coverage Δ
api/v1beta1 (u) ∅ <ø> (∅)
pkg/common (u) 77.72% <67.24%> (-0.88%) ⬇️
pkg/istio (u) 37.11% <ø> (ø)
pkg/log (u) 31.81% <ø> (ø)
pkg/reconcilers (u) 33.44% <ø> (ø)
pkg/rlptools (u) 56.46% <ø> (ø)
controllers (i) 70.06% <81.25%> (-1.18%) ⬇️
Files Coverage Δ
api/v1beta2/authpolicy_types.go 70.83% <ø> (ø)
api/v1beta2/ratelimitpolicy_types.go 17.24% <ø> (ø)
controllers/authpolicy_authconfig.go 66.66% <100.00%> (+0.35%) ⬆️
controllers/authpolicy_controller.go 78.66% <100.00%> (ø)
pkg/common/errors.go 100.00% <100.00%> (ø)
controllers/authpolicy_status.go 86.17% <79.66%> (-7.28%) ⬇️
pkg/common/apimachinery_status_conditions.go 68.85% <52.50%> (-31.15%) ⬇️

... and 6 files with indirect coverage changes

@KevFan KevFan force-pushed the ap-enforced branch 4 times, most recently from 38a9336 to f19dcf4 Compare February 2, 2024 14:18
@KevFan KevFan force-pushed the ap-enforced branch 5 times, most recently from 5b15865 to 336c81d Compare February 6, 2024 16:30
@KevFan KevFan changed the title [wip] feat: auth policy enforced condition feat: auth policy enforced condition Feb 7, 2024
@KevFan KevFan marked this pull request as ready for review February 7, 2024 10:37
@KevFan KevFan requested a review from a team as a code owner February 7, 2024 10:37
@KevFan KevFan force-pushed the ap-enforced branch 3 times, most recently from 87c3578 to 233405a Compare February 12, 2024 14:24
@guicassolato
Copy link
Contributor

Verification steps work as expected 🎉

After the last step, I've added:

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

Then:

kubectl get authpolicy -A -o wide
NAMESPACE      NAME      ACCEPTED   ENFORCED   TARGETREFKIND   TARGETREFNAME          AGE
default        auth0     True       True       HTTPRoute       toystore               19m
istio-system   gw-auth   True       False      Gateway         istio-ingressgateway   7s

kubectl get authpolicy gw-auth -n istio-system -o yaml | yq '.status'
conditions:
  - lastTransitionTime: "2024-02-13T13:23:05Z"
    message: AuthPolicy has been accepted
    reason: Accepted
    status: "True"
    type: Accepted
  - lastTransitionTime: "2024-02-13T13:23:05Z"
    message: AuthPolicy is overridden by [{"Namespace":"default","Name":"auth0"}]
    reason: Overridden
    status: "False"
    type: Enforced
observedGeneration: 1

kubectl delete authpolicy auth0

kubectl get authpolicy -A -o wide
NAMESPACE      NAME      ACCEPTED   ENFORCED   TARGETREFKIND   TARGETREFNAME          AGE
istio-system   gw-auth   True       True       Gateway         istio-ingressgateway   3m37s

kubectl get authpolicy gw-auth -n istio-system -o yaml | yq '.status'
conditions:
  - lastTransitionTime: "2024-02-13T13:23:05Z"
    message: AuthPolicy has been accepted
    reason: Accepted
    status: "True"
    type: Accepted
  - lastTransitionTime: "2024-02-13T13:26:34Z"
    message: AuthPolicy has been successfully enforced
    reason: Enforced
    status: "True"
    type: Enforced
observedGeneration: 1

Comment on lines +171 to +172
// +kubebuilder:printcolumn:name="Accepted",type=string,JSONPath=`.status.conditions[?(@.type=="Accepted")].status`,description="RateLimitPolicy Accepted",priority=2
// +kubebuilder:printcolumn:name="Enforced",type=string,JSONPath=`.status.conditions[?(@.type=="Enforced")].status`,description="RateLimitPolicy Enforced",priority=2
Copy link
Contributor

Choose a reason for hiding this comment

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

Though the title says "auth policy", it's good to have this here as well, for completeness 👍

@@ -22,6 +22,7 @@ import (
"os"
"runtime"

"github.com/kuadrant/kuadrant-operator/pkg/common"
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: I fail to see the logic behind how we group these dependencies. Maybe in another PR we can work on this and give them some order people can reason about. E.g. Kuadrant-controlled packages always grouped together, or layered based on proximity to the Golang std lib.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, grouping is inconsistent in a few places. I think the main reason, at least for me, for this inconsistency is that these are frequently auto added by an IDE but ignores the groups that have been already been logically broken up previously. Goland IDE seems to always auto add a dependency to the first group list after the Golang std libs 🤔

Copy link
Member

Choose a reason for hiding this comment

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

fyi, i had opened a PR to add the openshift goimports to keep module grouping consistent #415. Backed it out as we seem to already use goimports in CI tests, but maybe we could look into it more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #426 to track this

Copy link
Contributor

@guicassolato guicassolato left a comment

Choose a reason for hiding this comment

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

Nice job!

I've tested it again after last refactoring and still working perfectly fine.

This was referenced Feb 15, 2024
@KevFan KevFan merged commit f982a02 into Kuadrant:main Feb 20, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Changes user facing APIs kind/enhancement New feature or request size/small
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Authpolicy status should not report error when authconfig resource is not required.
4 participants