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

Fix bug in response validation rules #287

Merged

Conversation

adam-cattermole
Copy link
Member

@adam-cattermole adam-cattermole commented Nov 2, 2023

There is a bug where if the response field is present, but no response.success field in a submitted AuthPolicy it cannot be reconciled by the controller. The Success field is not a pointer so the validation evaluates to false when trying to add the finalizer.

The exists check for the routeselectors in dynamicMetadata and headers is also inverted.

The (second) AuthPolicy in the user guide can be used as validation - it fails to reconcile from main but works in this PR:

kubectl -n istio-system apply -f - <<EOF
apiVersion: kuadrant.io/v1beta2
kind: AuthPolicy
metadata:
  name: gw-auth
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: Gateway
    name: istio-ingressgateway
  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

@adam-cattermole adam-cattermole requested a review from a team as a code owner November 2, 2023 12:19
Copy link

codecov bot commented Nov 2, 2023

Codecov Report

Merging #287 (5f23e9b) into main (fbe0c44) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #287   +/-   ##
=======================================
  Coverage   65.37%   65.37%           
=======================================
  Files          35       35           
  Lines        3806     3806           
=======================================
  Hits         2488     2488           
  Misses       1133     1133           
  Partials      185      185           
Flag Coverage Δ
unit 58.20% <ø> (ø)

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% <ø> (ø)
pkg/istio (u) 30.24% <ø> (ø)
pkg/log (u) 31.81% <ø> (ø)
pkg/reconcilers (u) 33.68% <ø> (ø)
pkg/rlptools (u) 56.41% <ø> (ø)
controllers (i) 71.61% <ø> (ø)
Files Coverage Δ
api/v1beta2/authpolicy_types.go 77.57% <ø> (ø)

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.

This is my bad and I'm sorry about it. We're gonna need to patch the latest release of the Kuadrant Operator with this fix.

Thanks, @adam-cattermole. Nice catch!

@guicassolato guicassolato added kind/bug Something isn't working target/current size/small area/api Changes user facing APIs labels Nov 2, 2023
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.

🐛 🔫

@adam-cattermole adam-cattermole merged commit bb6476e into Kuadrant:main Nov 2, 2023
13 checks passed
@adam-cattermole adam-cattermole deleted the fix-validation-rules branch November 2, 2023 15:11
@guicassolato
Copy link
Contributor

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/bug Something isn't working size/small
Projects
No open projects
Status: To test
Development

Successfully merging this pull request may close these issues.

3 participants