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

RateLimitPolicy conditions don't indicate that the limit is applied #140

Closed
pehala opened this issue Jan 16, 2023 · 9 comments
Closed

RateLimitPolicy conditions don't indicate that the limit is applied #140

pehala opened this issue Jan 16, 2023 · 9 comments
Labels
kind/bug Something isn't working
Milestone

Comments

@pehala
Copy link
Contributor

pehala commented Jan 16, 2023

RateLimitPolicy has a condition that says that HTTPRoute is protected; however, that condition doesn't mean that the RLP is being applied and you can start making requests. You need to wait an additional minute or so for the limit to be applied.

@pehala pehala added the kind/bug Something isn't working label Jan 16, 2023
@thomasmaas
Copy link
Contributor

Do you mean "state" when you say "condition"? And if so, is there a way to make that state reflect reality?

@pehala
Copy link
Contributor Author

pehala commented Jan 16, 2023

The RLP has this condition and even when the status is "True" and the messages says that HTTPRoute is ratelimited, the limit is not applied yet and you need to wait a minute or so for it to work.

status:
  conditions:
    - lastTransitionTime: '2023-01-16T11:50:19Z'
      message: HTTPRoute is ratelimited
      reason: HTTPRouteProtected
      status: 'True'
      type: Available

@thomasmaas
Copy link
Contributor

And is it theoretically possible to only report true once it is applied (callback?)?

@eguzki
Copy link
Contributor

eguzki commented Jan 16, 2023

This usually happens when the limits CR is updated (not created). The associated config map changes and k8s takes a bit to update Limitador's internal config.

Currently, the controller sets the condition to "protected" when all the required changes in the cluster have been done. But it is also true that the change sometimes takes a bit to be effective.

I do not think this is a critical issue. But the user experience can indeed be improved. The operator could be doing few more checks to make sure the changes are effective. It is open for discussion what can be done.

@eguzki
Copy link
Contributor

eguzki commented Jan 16, 2023

A possible mitigation would be force re-sync of mounted configmap content. As the 3scale SaaS operator does, updating pod's annotations will make kubelet to re-sync mounted configmap volumes. https://github.com/3scale-ops/saas-operator/blob/e4b75b0e2e9b44adb594eb15504f745b117bbbc7/controllers/twemproxyconfig_controller.go#L210

Article explaining this: https://ahmet.im/blog/kubernetes-secret-volumes-delay/

That way there is no need to wait for the automatic update which can take up to few minutes (kubelet sync periodic time+ used cache TTL)

@maleck13
Copy link
Collaborator

maleck13 commented Feb 3, 2023

Possibly a different issue. But another status condition that might be useful would be to identify if the RLP is not matching any path method combination so essentially not being applied. IE on HTTPRoute update or RLP update validate that the RLP is valid and going to be applied to "something" within the defined target? I can open a different issue if better there

mikenairn pushed a commit to mikenairn/kuadrant-operator that referenced this issue Mar 23, 2023
* ratelimitpolicy controller: targetref

* ratelimitpolicy controller: status conditions

* ratelimitpolicy controller: validate targetref namespace and kind

* README: remove authenticated rate limit from user guide

* ratelimitpolicy_controller: report when target is already referenced

* ratelimitpolicy_controller: reconcile HTTPRoute changes and RLP targetRef changes

* fix lint issues

* resolve rebase conflicts
@alexsnaps
Copy link
Member

See this on-going effort

@alexsnaps alexsnaps added this to the v0.5.0 milestone Sep 12, 2023
@didierofrivia didierofrivia removed their assignment Nov 2, 2023
@alexsnaps alexsnaps removed this from the v0.5.0 milestone Nov 14, 2023
@github-project-automation github-project-automation bot moved this from To do to To test in Kuadrant Service Protection Dec 8, 2023
@pehala
Copy link
Contributor Author

pehala commented Dec 11, 2023

Reopening as it is still an issue, but it will be fixed as part of Kuadrant/architecture#38

@pehala pehala reopened this Dec 11, 2023
@alexsnaps alexsnaps moved this to Todo in Kuadrant Dec 11, 2023
@alexsnaps alexsnaps added this to the v0.7.0 milestone Dec 18, 2023
@KevFan
Copy link
Contributor

KevFan commented Apr 23, 2024

Resolved as part of #523

@KevFan KevFan closed this as completed Apr 23, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in Kuadrant Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
Status: Done
Status: To test
Development

No branches or pull requests

7 participants