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

Kong Rate Limiting Plugin not working as expected when limit by path #10672

Open
1 task done
cunkz opened this issue Apr 14, 2023 · 18 comments
Open
1 task done

Kong Rate Limiting Plugin not working as expected when limit by path #10672

cunkz opened this issue Apr 14, 2023 · 18 comments
Labels
pinned Issues that won't be marked as stale by stalebot plugins/rate-limiting task/bug

Comments

@cunkz
Copy link

cunkz commented Apr 14, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Kong version ($ kong version)

2.5

Current Behavior

When I add Rate Limiting plugin, I do this following step :

  1. set config.limit_by with path
  2. set config.path with /abc
  3. hit other path like /zxc

Kong also do rate limit in /zxc path

Expected Behavior

I assume Kong didn't rate limit it.
Then I check this line :

local req_path = kong.request.get_path()

And

return identifier or kong.client.get_forwarded_ip()

When request_path didn't same with config.path why Kong didnt bypass it ? But, he change identifier to forwarded ip ?

Steps To Reproduce

  1. set config.limit_by with path
  2. set config.path with /abc
  3. hit other path like /zxc

Anything else?

No response

@hanshuebner
Copy link
Contributor

Hello @cunkz,

sorry that it took so long to respond. I notice that you're using a very old version of Kong that we're not maintaining anymore. Please consider upgrading to the latest release. If the problem exists in Kong Gateway 3.2, please feel free to reach out again.

Kind regards,
Hans

@hanshuebner hanshuebner added the pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... label May 11, 2023
@stale
Copy link

stale bot commented May 31, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label May 31, 2023
@stale stale bot closed this as completed Jun 9, 2023
@Clint-Chester
Copy link

We are seeing this issue as well with the Advanced Rate Limiting Plugin. It doesn't seem like matching by path is actually matching any paths specified, and is applied to the lot.

@hanshuebner
Copy link
Contributor

@Clint-Chester The Rate Limiting Advanced plugin is an enterprise feature - Please reach out to your support contact for assistance. Thank you!

@Clint-Chester
Copy link

@Clint-Chester The Rate Limiting Advanced plugin is an enterprise feature - Please reach out to your support contact for assistance. Thank you!

Thanks @hanshuebner, tried the standard Rate Limiting Plugin and got the same behaviour unfortunately.

@hanshuebner
Copy link
Contributor

@Clint-Chester I'm going to re-open this ticket. You may get expedited responses through the enterprise support channel, though.

@hanshuebner hanshuebner reopened this Oct 28, 2023
@hanshuebner hanshuebner removed pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... stale labels Oct 28, 2023
@Clint-Chester
Copy link

Current configuration is the following.

apiVersion: configuration.konghq.com/v1
kind: KongPlugin
metadata:
  name: website-login-rate-limiting
plugin: rate-limiting
config:
  limit_by: path
  path: /api/login
  minute: 70
  sync_rate: -1
  hide_client_headers: true
  error_message: "Login Rate Limit Exceeded"

With the following as my Ingress Controller Configuration.

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: web-ingress
  annotations:
    konghq.com/plugins: website-login-rate-limiting
spec:
  ingressClassName: kong
  rules:
  - host: "website.com"
    http:
      paths:
      - pathType: Exact
        path: "/api/login"
        backend:
          service:
            name: web-api-backend
      - pathType: Prefix
        path: "/"
        backend:
          service:
            name: web-frontend

Is the behaviour that it's going to try and match all paths?

Thanks for re-opening @hanshuebner, appreciate it! We've got Enterprise Support Open as well but was very interested in seeing if others had experienced this in the past.

@StarlightIbuki
Copy link
Contributor

StarlightIbuki commented Oct 30, 2023

@Clint-Chester By the name ("login") I guess smaller limitations are configured than general requests?
limit-by uses a fallback mechanism to determine its limit scope, that is if the request path does not match the configured path, it will use IP to limit the request. So the behavior is kind of expected.
I also feel this behavior makes little sense. I'm looking for more information and opinions about this.

@mheap
Copy link
Member

mheap commented Oct 30, 2023

I can't comment on the rate limiting defaults, but to achieve what you're looking for with Ingress you can define two Ingress resources:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: login-ingress
  annotations:
    konghq.com/plugins: website-login-rate-limiting
spec:
  ingressClassName: kong
  rules:
  - host: "website.com"
    http:
      paths:
      - pathType: Exact
        path: "/api/login"
        backend:
          service:
            name: web-api-backend
---
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: web-ingress
spec:
  ingressClassName: kong
  rules:
  - host: "website.com"
    http:
      paths:
      - pathType: Prefix
        path: "/"
        backend:
          service:
            name: web-frontend

The konghq.com/plugins annotation applies to all routes in an Ingress definition, which means they should be split if you only want the plugin to apply to specific routes

@Clint-Chester
Copy link

@Clint-Chester By the name ("login") I guess smaller limitations are configured than general requests? limit-by uses a fallback mechanism to determine its limit scope, that is if the request path does not match the configured path, it will use IP to limit the request. So the behavior is kind of expected. I also feel this behavior makes little sense. I'm looking for more information and opinions about this.

Yes that's correct, it would be looking to have a smaller limit on a particular path.

So the behaviour in this example is that you'd want to be mitigating a DDoS attack or a credential stuffing attack used a bot network. Rate limiting by IP address doesn't work here because the attack is cycling through different IP addresses. By doing rate limiting via the path, it acts as a kill switch on the API to slow down the damage.

Is there something missing in my path matching? As I've tried a lot of different combinations to no avail.

@Clint-Chester
Copy link

I can't comment on the rate limiting defaults, but to achieve what you're looking for with Ingress you can define two Ingress resources:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: login-ingress
  annotations:
    konghq.com/plugins: website-login-rate-limiting
spec:
  ingressClassName: kong
  rules:
  - host: "website.com"
    http:
      paths:
      - pathType: Exact
        path: "/api/login"
        backend:
          service:
            name: web-api-backend
---
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: web-ingress
spec:
  ingressClassName: kong
  rules:
  - host: "website.com"
    http:
      paths:
      - pathType: Prefix
        path: "/"
        backend:
          service:
            name: web-frontend

The konghq.com/plugins annotation applies to all routes in an Ingress definition, which means they should be split if you only want the plugin to apply to specific routes

I did actually try this but it still didn't seem to work. It makes me think that what @StarlightIbuki was talking about that the path match must not have been detecting at all, and it was defaulting to IP rate limiting.

@StarlightIbuki
Copy link
Contributor

@Clint-Chester The plugin falls back to limit by IP when the request path does not match the configured one. I do not understand the design's intention so I'm trying to confirm.

@StarlightIbuki
Copy link
Contributor

The fallback mechanism seems to be introduced when we have only consumer and credential for limit-by, and it can't take later introduced type into consideration.

Please refer to the doc:

If the underlying service or route has no authentication layer, the Client IP address is used. Otherwise, the consumer is used if an authentication plugin has been configured.

I think it makes no sense to fall back when we limit by path.

@tzssangglass
Copy link
Member

If there is no fallback, when limit by path fails, it is equivalent to no limit, which is similar to bypass.

@StarlightIbuki
Copy link
Contributor

StarlightIbuki commented Nov 2, 2023

We had a discussion and this seems a legacy of old design, and the fallback here is not useful at all. We are going to redesign the plugin. For now, a possible workaround is to have the path limited split to a separate route and config the plugin on it. @cunkz @Clint-Chester

@chronolaw chronolaw added the pinned Issues that won't be marked as stale by stalebot label Jan 17, 2024
@dilanka-cacib
Copy link

Hello @StarlightIbuki,
Can I know possible fix date of this bug?
Thanks

@nvervelle
Copy link

Hello !

I just found this issue after opening a discussion on this subject.

I think the fix is quite simple, and I can open a PR for it if it helps. Just tell me

@StarlightIbuki
Copy link
Contributor

Hello !

I just found this issue after opening a discussion on this subject.

I think the fix is quite simple, and I can open a PR for it if it helps. Just tell me

Hi. Thanks for your kind offering. This is more of a design issue than an implementation bug, and unfortunately, it did not draw enough attention for a production decision. I'm creating an internal ticket and asking for comments.

Tracked by KAG-5426

Personally, I agree with the solution you proposed (that's also the same as I thought of). PRs are welcome. Please refer to https://github.com/Kong/kong/blob/master/CONTRIBUTING.md

lhanjian pushed a commit that referenced this issue Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pinned Issues that won't be marked as stale by stalebot plugins/rate-limiting task/bug
Projects
None yet
Development

No branches or pull requests

10 participants