Skip to content
This repository has been archived by the owner on Dec 16, 2024. It is now read-only.

Control plane RateLimitPolicy #163

Merged

Conversation

mikenairn
Copy link
Member

@mikenairn mikenairn commented Apr 14, 2023

closes #116

Adds a RateLimitPolicy reconciler that will reconcile kuadrant RateLimitPolicies to apply mctc sync and patch annotations based on the RLPs target owner Gateway. If the target Gateway is using the wildcard all sync annotation, a list of available clusters is queried, and individual sync annotations added for each. A cluster specific patch annotation is added for each cluster the RLP is being synced to which adds an RLP configuration action using the cluster name as a key.
This will automatically be made available for use in the limits section of the RLP allowing gateway admins to add cluster specific limits.

  • Add kuadrant RateLimitPolicy CRD to control plane cluster.
  • Add RateLimitPolicy controller and kuadrant-operator APIs as a dependency.
  • Update sigs.k8s.io/gateway-api to v0.6.2.
  • Add ratelimitpolicies.v1beta1.kuadrant.io to default syncer synced resources.
  • Use gateway-api-v0.6.2 version of kuadrant
  • Set gateway as owner of RLP.
  • Add sync annotations based on target gateway sync annotations.

Includes a commit from #124 (f9ffbe1), we should try and merge that first, or pull these out into their own PR.

Requires Kuadrant/kuadrant-operator#153. We can merge this using the gateway-api-0.6.2 kuadrant operator branch, but we should probably try and get that kuadrant operator change merged first and revert to main.

Verification

Follow this doc https://github.com/Kuadrant/multicluster-gateway-controller/blob/e1b74f4040760fa5893eb15495b1b38f90a489fb/docs/ratelimitpolicy/walkthrough/rlp_walkthrough.md

  • tests
  • docs
    - [ ] gateway status Deferring to after ACM POC work so we are clear how we are doing this
    - [ ] aggregated RLP status Deferring to after ACM POC work so we are clear how we are doing this

@maleck13
Copy link
Contributor

maleck13 commented Apr 17, 2023

@mikenairn i didn't see anything around the control plane level gateway status in the PR?
the final item on #116 is to add multi-cluster gateway status. However I also see there is not a lot of details there. I will add some more detail. I don't think it blocks this PR but would be good to cover it in a follow up. Added some more details to that issue.

@mikenairn Ignore me. I see you called it out at the end ! I need to read crossed out stuff :) holding off for now is fine

@mikenairn
Copy link
Member Author

@mikenairn i didn't see anything around the control plane level gateway status in the PR? the final item on #116 is to add multi-cluster gateway status. However I also see there is not a lot of details there. I will add some more detail. I don't think it blocks this PR but would be good to cover it in a follow up. Added some more details to that issue.

@mikenairn Ignore me. I see you called it out at the end ! I need to read crossed out stuff :) holding off for now is fine

Yeah, i did look into this for a while, but since we deferred the gateway aggregated status, and the RLP in the control plane isn't going to have a status until we do that, i decided to leave it out for now. Will add a follow up task to add it.

@mikenairn mikenairn force-pushed the control_plane_rate_limit_policy branch from f9ffbe1 to 3d8507c Compare April 18, 2023 15:18
@mikenairn mikenairn force-pushed the control_plane_rate_limit_policy branch 2 times, most recently from b1fd1b1 to e1b74f4 Compare April 20, 2023 09:06
@maleck13
Copy link
Contributor

maleck13 commented Apr 21, 2023

working through the walkthough

One minor change as mentioned in chat:

currently we set the descriptor key for the cluster to be

configurations:
      - actions:
        - generic_key:
            descriptor_key: mctcworkload1
            descriptor_value: "1"

I think it would make more sense and be easier to communicate via docs if it was under a well known key example:

- configurations:
      - actions:
        - generic_key:
            descriptor_key: kudarant_gateway_cluster
            descriptor_value: "mctcworkload1"

@mikenairn mikenairn force-pushed the control_plane_rate_limit_policy branch 2 times, most recently from 137f8d9 to bafcc3a Compare April 21, 2023 13:34
@mikenairn
Copy link
Member Author

working through the walkthough

One minor change as mentioned in chat:

currently we set the descriptor key for the cluster to be

configurations:
      - actions:
        - generic_key:
            descriptor_key: mctcworkload1
            descriptor_value: "1"

I think it would make more sense and be easier to communicate via docs if it was under a well known key example:

- configurations:
      - actions:
        - generic_key:
            descriptor_key: kudarant_gateway_cluster
            descriptor_value: "mctcworkload1"

Updated

@mikenairn
Copy link
Member Author

mikenairn commented Apr 21, 2023

/hold

I need to update this to pull the cluster name from the secret data rather than using the secret name

@mikenairn mikenairn force-pushed the control_plane_rate_limit_policy branch from bafcc3a to f8b8ea3 Compare April 24, 2023 07:37
@mikenairn
Copy link
Member Author

mikenairn commented Apr 24, 2023

/unhold

Updated to use the name from the contents of the cluster secret instead of the secret name e644c42

Also moves some of the cluster secret logic to make it reusable by other controllers @sergioifg94 fyi

@mikenairn mikenairn force-pushed the control_plane_rate_limit_policy branch from e644c42 to 249ff37 Compare April 24, 2023 10:12
@maleck13
Copy link
Contributor

so now if a cluster changes (IE an attribute annotation is added) that triggers a gateway reconcile that enques any RLP targeting that gateway? Just trying to understand the flow.

@mikenairn
Copy link
Member Author

so now if a cluster changes (IE an attribute annotation is added) that triggers a gateway reconcile that enques any RLP targeting that gateway? Just trying to understand the flow.

Not exactly, it just triggers the reconciliation of a Gateway and an RLP. The cluster event handler was already there for Gateways to get updates when an associated secret changes, this just adds the RLPs to that. If a secret changes, it checks alls the RLPs and triggers a reconcile of any that are being synced to that cluster.

@mikenairn mikenairn force-pushed the control_plane_rate_limit_policy branch 5 times, most recently from 5520e1d to 5903e29 Compare April 25, 2023 08:14
@maleck13
Copy link
Contributor

I haven't re-tested but generally looking fine.
@mikenairn ok to put a demo together for this functionality?

Adds a RateLimitPolicy reconciler that will reconcile kuadrant
RateLimitPolicies to apply mctc sync and patch annotations based on the
RLPs target owner Gateway. If the target Gateway is using the wildcard
`all` sync annotation, a list of available clusters is queried, and
individual sync annotations added for each. A cluster specific
patch annotation is added for each cluster the RLP is being synced to
which adds an RLP configuration action using the cluster name as a key.
This will automatically be made available for use in the limits section
of the RLP allowing gateway admins to add cluster specific limits.

* Add kuadrant RateLimitPolicy CRD to control plane cluster.
* Add RateLimitPolicy controller and kuadrant-operator APIs as a dependency.
* Update sigs.k8s.io/gateway-api to v0.6.2.
* Add ratelimitpolicies.v1beta1.kuadrant.io to default syncer synced resources.
* Use gateway-api-v0.6.2 version of kuadrant
* Set gateway as owner of RLP.
* Add sync annotations based on target gateway sync annotations.
* Allow adding cluster attribues to RLP via annotations
`kuadrant.io/attribute-cloud=aws = (key: cloud, value: aws)`
@mikenairn mikenairn force-pushed the control_plane_rate_limit_policy branch from 5903e29 to a1a16a6 Compare April 26, 2023 10:40
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 27, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maleck13, mikenairn

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@maleck13
Copy link
Contributor

went through initial verification. several code reviews

/lgtm

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multi-Cluster rate limit
3 participants