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

GEP: Gateway API should support basic rate limiting (aka throttling) per (sub)path. #326

Closed
nielsbasjes opened this issue Sep 11, 2020 · 27 comments
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. kind/gep PRs related to Gateway Enhancement Proposal(GEP) priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.

Comments

@nielsbasjes
Copy link

See also: kubernetes/ingress-nginx#6011
What would you like to be added:

I would like to be able to have rate limiting (throttling) as part of the standard way of defining an Ingress.
By having this a part of the api the various backends can start implementing support for it.

Why is this needed:

In general there is a need for limiting the rate at which requests are allowed to avoid overloading the servers.
Having this at this level would make this very easy to use in many places.

Also I found that in many situations the rate limit depends on the exact path.
Some are easy for the server to handle (send static content) and some are hard (like build a complex report).
So in general I want to specifiy the rat limits per sub-path on a specific ingress.

What I have in mind

Right now you can defined per host which (Prefix) paths should go to which service.
My first idea is that having the limits per prefix path of that service is probably enough for most usecases.

My first rough sketch looks like this:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: my-ingress
  annotations:
    kubernetes.io/ingress.class: nginx
spec:
  rules:
    - host: something.example.nl
      http:
        paths:
          - path: /
            pathType: Prefix
            backend:
              service:
                name: something-service
                port:
                  number: 80
            rateLimits:
              - path: /      # I would do this relative to the 'parent path', implicitly this is always "pathType: Prefix"
                byCookie: jsessionid
                #byHeader: customerid
                #byIP
                # If nothing is specified then "global limit"
                maxPerSecond: 5
                allowBurst: 10
                delayBurst: false
              - path: /reporting
                #byCookie: jsessionid
                byHeader: customerid
                #byIP
                # If nothing is specified then "global limit"
                maxPerSecond: 1
                allowBurst: 2
                delayBurst: true
@nielsbasjes nielsbasjes added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 11, 2020
@howardjohn
Copy link
Contributor

cc @mandarjog @gargnupur

@gargnupur
Copy link

This would be good to have.. In the ratelimits section, it would be good to add support for generic keys too, so that custom attributes in ingress can be used for ratelimiting..

@hbagdi
Copy link
Contributor

hbagdi commented Sep 11, 2020

Thanks for the issue, @nielsbasjes .

Rate-limiting is indeed a very common use-case that is tackled at the Ingress layer and most implementations have support for it. The catch here is that rate-limiting is an extremely nuanced area. It is very uncommon for different rate-limit implementations to have the exact same behavior.

The purpose of the service-apis is to design an API that can be implemented by a major subset of proxies out there, and then have extensions points for implementation-specific features. In this spirit, we added Filters to HTTPRoute (equivalent of Ingress v1 resource) which is exactly meant for use-cases like the one proposed here.
Can you take a look at Filters and see if it satisfies your use-case? If not, I'd love to hear your feedback and see how we can support your problem better.

@nielsbasjes
Copy link
Author

I had a look at this Filters feature and (although I do not yet fully understand how it should work) it seems like a generic solution to handle variation in the available backends.

The downside I see is that it may be too generic: will it create a multitude of configurations each specific for the actual implementation used?
So where we now have a single "Ingress" which works with all backends; it seems that for ratelimiting it is going to converge towards a separate Filter (+ possibly very different config) for each and every distinct implementation.

So I was wondering if there is a way to have a basic rate limiting (like only max requests per second per global/IP/cookie) which is generic in the base implementation; and then to have the option for specific implementations (similar to creating a subclass) to add more advanced features (like the bursts in nginx) ?

[ Note that I'm unfamiliar with the code you linked to (seems like a config structure definition to me) so I have not been able to figure out what kind of config this will lead to. ]

@jmprusi
Copy link

jmprusi commented Nov 11, 2020

This seems to be related to #114

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 9, 2021
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 11, 2021
@robscott
Copy link
Member

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Mar 11, 2021
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 9, 2021
@jpeach
Copy link
Contributor

jpeach commented Jun 11, 2021

/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jun 11, 2021
@shaneutt shaneutt changed the title Ingress api should support basic rate limiting (aka throttling) per (sub)path. Gateway API should support basic rate limiting (aka throttling) per (sub)path. Jul 21, 2022
@shaneutt
Copy link
Member

Despite this issue being quite old, we the maintainers are still pretty convinced that we want to have this functionality in a future release. We are marking this help wanted as we're looking for contributors with strong use cases to help champion and drive this forward.

One important note: the original issue referred to the Ingress API which may be confusing, but we do indeed intend this for Gateway API.

@shaneutt shaneutt added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Jul 21, 2022
@keithmattix
Copy link
Contributor

For the mesh/GAMMA use-case, we'll probably be looking at Policy Attachment as a way of providing this since each implementation's rate-limiting feature is rarely the same.

@kahirokunn
Copy link

I need this.

@hzxuzhonghu
Copy link
Member

IMO, every users may need ratelimit, @keithmattix I admit different implementors can not be same, but if gateway API can not provide some typical API, why would end-user pay for it? They already have all their own API.

@arkodg
Copy link
Contributor

arkodg commented Feb 14, 2023

Envoy Gateway recently implemented Rate Limiting using ExtensionFilter https://gateway.envoyproxy.io/v0.3.0/user/rate-limit.html

@robscott @shaneutt @youngnick would be great if you can highlight how many implementations are required to support a feature (rate limiting in this case) for the feature to start being considered part of the Gateway API

@youngnick
Copy link
Contributor

I think that for rate limiting to be considered part of Gateway API, and not just a (really great) Implementation Specific feature of Envoy Gateway, we'll need:

  • some form of object in the gateway.networking.k8s.io API Group that configures it (it could well be something like the ExtensionFilter if we can get everyone on board).
  • Conformance tests for that object that put it in Extended support

Once we have that and at least a few implementations, then we can mark Ratelimiting as a Gateway API feature.

@arkodg
Copy link
Contributor

arkodg commented Feb 20, 2023

thanks for your response @youngnick , I was trying to understand when can a community member kickstart a GEP for this feature ?

@youngnick
Copy link
Contributor

Any time you like! I'd recommend starting with a Google doc that has the same headings as the GEP template, and fill it out, including prior art (which I think applies in this case). I think the other thing that's particularly important is talking about the use cases you're aiming for, what ones are out of scope, and so on.

@shaneutt shaneutt added the priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. label Feb 22, 2023
@shaneutt shaneutt moved this from Backlog to Triage in Gateway API: The Road to GA Feb 22, 2023
@shaneutt shaneutt removed help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. labels Mar 16, 2023
@shaneutt
Copy link
Member

Accepted from a triage perspective given the feedback and the fact that we are still getting comments for it after multiple years.

/priority important-longterm

I'm going to mark this as wanted for GA (milestone v1.0.0) as it seems important enough that we should try and get it in before then. It also seems like we'll need a GEP for this first as consensus needs to be built around the how (though it seems the what and why are fairly non-controversial).

/kind gep

That said, we're still lacking someone to champion this issue and move it forward. Without a champion this may continue to linger and go stale and may not make it into v1.0.0. If you are reading this and feel strongly that you want to see this feature in GA we would encourage you to help us push it forward by taking this one on.

/help

@k8s-ci-robot
Copy link
Contributor

@shaneutt:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

  • Why are we solving this issue?
  • To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
  • Does this issue have zero to low barrier of entry?
  • How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

Accepted from a triage perspective given the feedback and the fact that we are still getting comments for it after multiple years.

/priority important-longterm

I'm going to mark this as wanted for GA (milestone v1.0.0) as it seems important enough that we should try and get it in before then. It also seems like we'll need a GEP for this first as consensus needs to be built around the how (though it seems the what and why are fairly non-controversial).

/kind gep

That said, we're still lacking someone to champion this issue and move it forward. Without a champion this may continue to linger and go stale and may not make it into v1.0.0. If you are reading this and feel strongly that you want to see this feature in GA we would encourage you to help us push it forward by taking this one on.

/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/gep PRs related to Gateway Enhancement Proposal(GEP) labels Mar 16, 2023
@shaneutt shaneutt moved this from Triage to Backlog in Gateway API: The Road to GA Mar 16, 2023
@shaneutt shaneutt added this to the v1.0.0 milestone Mar 16, 2023
@shaneutt shaneutt removed the priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. label Mar 16, 2023
@alexsnaps
Copy link

I know this talks quite a bit about using Filters to enable rate limiting. As part of Kuadrant we are currently looking at leveraging Policy Attachment to address it, with our new RateLimitPolicy proposal. We'd love to take a stab at extending this proposal as a GEP and try to come up with something more widely implementable. As we do not plan on providing an actual Gateway implementation, we're leveraging the Gateway API's Policy Attachment as a mechanism to extend the API using our own CRDs, operator and eventually wire the Gateway (currently only Envoy, thru Istio) to provide Rate Limiting and AuthN/Z capabilities.

So other than someone thinking PA is definitively not the way to go for adding Rate Limiting to the Gateway API, I'll start a Google Doc and start adding some thoughts based of what we have done & learned so far. Does that sound good?

@shaneutt
Copy link
Member

@alexsnaps we really appreciate you being willing to put some effort into moving this one forward! As for the specifics, generally a GEP is the place to start, but given that you're saying you aren't really trying to make a specific proposal right now but instead share an alternative perspective, a doc seems fine. I would recommend you share it in our #sig-network-gateway-api channel on Kubernetes Slack, and add an agenda item for an upcoming Monday meeting to discuss it. Let us know how we can help and support you!

@shaneutt
Copy link
Member

shaneutt commented Jul 3, 2023

Reviewing what's in our v1.0.0 milestone for GA, it doesn't seems like this one needs to HOLD that release up (though we'd still be happy to have you move this one forward if you're still interested @alexsnaps) so just removing it from the milestone

/cc @robscott @youngnick

@shaneutt shaneutt removed this from the v1.0.0 milestone Jul 3, 2023
@youngnick youngnick changed the title Gateway API should support basic rate limiting (aka throttling) per (sub)path. GEP: Gateway API should support basic rate limiting (aka throttling) per (sub)path. Mar 11, 2024
@shaneutt
Copy link
Member

While grooming we saw that this one was open for a long period of time without anyone with a strong use case to champion it. We're going to close this as we don't expect anyone's ready to drive it forward, but if you still want this feature and have a strong use case we will be happy to reconsider this and re-open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. kind/gep PRs related to Gateway Enhancement Proposal(GEP) priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Projects
Development

No branches or pull requests