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

Improve policy constraint error message #145

Merged
merged 6 commits into from
Feb 3, 2023

Conversation

didierofrivia
Copy link
Member

@didierofrivia didierofrivia commented Jan 27, 2023

This PR besides aiming to improve the policy constrain error message, introduces a minor refactor. TargetHostnames and ValidateHierarchicalRules were controller methods that didn't need to make any use of the K8s client, thus they were extracted, simplified and unit tested .

The error message now looks like this:

rule host (keystep.arturia.com) does not follow any hierarchical constraints,
for the *v1beta1.RateLimitPolicy to be validated, it must match with at least one 
of the target network hostnames ["*.moog.com", "*.korg.com"]

This new version, besides showing the wrong policy target hostname, it shows the Type of the policy being reconciled and the target network object (HTTPRoute or Gateway) hostnames.

closes #113

Verification Steps

  1. Setup your local Kind env with
make local-setup
  1. Create a route with the following spec:
 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

Note the hostnames value being "*toystore.com"

  1. Apply the following RLP:
kubectl apply -f - <<EOF
---
apiVersion: kuadrant.io/v1beta1
kind: RateLimitPolicy
metadata:
  name: toystore
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: toystore
  rateLimits:
    - rules:
        - hosts: ["*.petstore.com"]
      configurations:
        - actions:
            - generic_key:
                descriptor_key: "limited"
                descriptor_value: "1"
      limits:
        - conditions:
            - "limited == 1"
          maxValue: 5
          seconds: 10
          variables: []
EOF

Note here that the spec.rateLimits[0].hosts value is "*.petstore.com"

  1. Once applied, the status of the RLP should show a failed condition with the following message:
kubectl get ratelimitpolicies/toystore -o yaml | yq ".status.conditions[0].message"
rule host (*.petstore.com) does not follow any hierarchical constrains,
for the *v1beta1.RateLimitPolicy to be validated, it must match with 
at least one of the target network hostnames ["*.toystore.com"]

@didierofrivia didierofrivia changed the title Improve policy constrain message Improve policy constrain error message Jan 30, 2023
@didierofrivia didierofrivia marked this pull request as ready for review January 30, 2023 15:10
@didierofrivia didierofrivia requested a review from a team as a code owner January 30, 2023 15:10
@alexsnaps
Copy link
Member

alexsnaps commented Jan 30, 2023

it must match with at least one of the target network hostnames ["*.moog.com", "*.korg.com"]

I think you meant *.moogmusic.com... 😉

@didierofrivia didierofrivia force-pushed the improve-policy-constrain-message branch from c1cbbf9 to e1c310d Compare January 31, 2023 11:42
@didierofrivia didierofrivia force-pushed the improve-policy-constrain-message branch from e1c310d to e668493 Compare January 31, 2023 13:36
@didierofrivia didierofrivia changed the title Improve policy constrain error message Improve policy constraint error message Feb 2, 2023
@eguzki
Copy link
Contributor

eguzki commented Feb 2, 2023

LGTM

There are conflicts to be merged as main has changed since the PR was created

@@ -20,7 +22,7 @@ func (r *AuthPolicyReconciler) reconcileAuthConfigs(ctx context.Context, ap *api
return err
}

authConfig, err := r.desiredAuthConfig(ctx, ap, targetNetworkObject)
authConfig, err := r.desiredAuthConfig(ap, targetNetworkObject)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch!

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.

👍

@didierofrivia didierofrivia force-pushed the improve-policy-constrain-message branch from e668493 to 683c497 Compare February 3, 2023 15:51
Copy link
Contributor

@pehala pehala left a comment

Choose a reason for hiding this comment

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

The message is certainly better

@didierofrivia didierofrivia merged commit 5cfc533 into main Feb 3, 2023
@didierofrivia didierofrivia deleted the improve-policy-constrain-message branch February 3, 2023 16:33
mikenairn pushed a commit to mikenairn/kuadrant-operator that referenced this pull request Mar 23, 2023
* rlp: validate httproute target is accepted

* fix lint issues
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.

Improve AuthPolicy hierarchical constraints message
5 participants