search

kubernetes-sigs / gateway-api

Repository for the next iteration of composite service (e.g. Ingress) and load balancing APIs.
https://gateway-api.sigs.k8s.io
Apache License 2.0
1.76k stars 457 forks source link

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

Closed nielsbasjes closed 6 months ago

nielsbasjes commented 4 years ago

See also: https://github.com/kubernetes/ingress-nginx/issues/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
howardjohn commented 4 years ago

cc @mandarjog @gargnupur

gargnupur commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

This seems to be related to https://github.com/kubernetes-sigs/service-apis/issues/114

fejta-bot commented 3 years ago

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

fejta-bot commented 3 years ago

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

robscott commented 3 years ago

/remove-lifecycle rotten

fejta-bot commented 3 years ago

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

jpeach commented 3 years ago

/lifecycle frozen

shaneutt commented 2 years ago

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.

keithmattix commented 2 years ago

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 commented 2 years ago

I need this.

hzxuzhonghu commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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:

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

arkodg commented 1 year ago

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

youngnick commented 1 year ago

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.

maleck13 commented 1 year ago

I think it would be valuable if 3rd parties were able to fulfil this API so that a Gateway provider or user could leverage an external provider of this capability. An example of this would be Envoys existing external auth and rate limiting that allows a separate implementation. @youngnick When you said ExtensionFilter is that a new concept or similar to the existing ExtensionRef

youngnick commented 1 year ago

I agree that having someone else handle the actual config is reasonable, particularly for ext_auth style use cases, as long as there's some way to coordinate with the Gateway implementation to ensure that all the pieces line up.

Oh, and thanks, yes, I meant the ExtensionRef inside the Filters section.

maleck13 commented 1 year ago

Right this coordination is the tricky part. There needs to be a contract between the gateway provider and the rate limiting provider / extension provider. Perhaps an agreed set of conditions as part of the status? It would also require providers to understand that an extension from GVK they don't own is acceptable. We would also need to think about when the route can be accepted into the gateway (IE does it require all the filters including 3rd party extensions to be ready).

shaneutt commented 1 year ago

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 commented 1 year ago

@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:

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](https://github.com/kubernetes-sigs/gateway-api/issues/326): >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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
alexsnaps commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 6 months ago

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.