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

Improve HTTP rate limiting classes and keys #6714

Merged
merged 3 commits into from
Nov 21, 2023

Conversation

adriansmares
Copy link
Contributor

@adriansmares adriansmares commented Nov 20, 2023

Summary

This PR allows more fine grained control of HTTP endpoint rate limiting, by adding a class which includes the path template. This allows rate limiting individual HTTP endpoints (like http:account:/oauth/api/auth/login) next to the full class (like http:account).

Paths which contain a template (containing a gateway ID for example) will maintain their template form (like http:gcs:/api/v3/gcs/gateways/{gateway_id}/semtechudp/global_conf.json).

Changes

  • Ensure that rate limiting middlewares are called after the authorization header has been converted to gRPC metadata.
  • Use the API token ID as an identifier of the caller, instead of the IP, when available. The IP is still used when there is no authorization available.
  • Add path template classes to HTTP rate limiting.

Testing

Add the following rate limiting configuration to your stack configuration file:

rate-limiting:
  profiles:
    - name: HTTP Account login
      max-per-min: 5
      associations:
        - http:account:/oauth/api/auth/login

Then try to login with a wrong password 5 times - you should get a rate limiting error.

Regressions

There is a behavioral change regarding the caller identity - using different API keys with the same caller IP will now have individual rate limits, which can allow more calls for a single source IP. This should allow better cloud-to-cloud connectivity via HTTP APIs (of which there aren't many, but still), and I consider it to be a reasonable change.

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md for the chosen target branch.
  • Documentation: Relevant documentation is added or updated.
  • The steps/process to test this feature are clearly explained including testing for regressions.
  • Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
  • Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

@adriansmares adriansmares added this to the v3.28.2 milestone Nov 20, 2023
@adriansmares adriansmares self-assigned this Nov 20, 2023
@github-actions github-actions bot added c/console This is related to the Console c/gateway conf server This is related to the Gateway Configuration Server labels Nov 20, 2023
@adriansmares adriansmares marked this pull request as ready for review November 20, 2023 16:18
Copy link
Member

@KrishnaIyer KrishnaIyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beat me to it. Good stuff!

pkg/ratelimit/resource.go Show resolved Hide resolved
pkg/ratelimit/resource.go Outdated Show resolved Hide resolved
@KrishnaIyer
Copy link
Member

@adriansmares: Shall we try to get this into v3.28.1?

@johanstokking
Copy link
Member

Regressions

There is a behavioral change regarding the caller identity - using different API keys with the same caller IP will now have individual rate limits, which can allow more calls for a single source IP. This should allow better cloud-to-cloud connectivity via HTTP APIs (of which there aren't many, but still), and I consider it to be a reasonable change.

Yep that's definitely reasonable.

Copy link
Contributor

@kschiffer kschiffer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codeowned LGTM

@adriansmares adriansmares force-pushed the feature/rate-limit-http branch from 58989ac to e1da170 Compare November 21, 2023 10:55
@adriansmares
Copy link
Contributor Author

@adriansmares: Shall we try to get this into v3.28.1?

I don't consider that we need these fine grained controls in the release. We'll add however some generic limits to our deployment templates.

@adriansmares adriansmares merged commit 69259b5 into v3.28 Nov 21, 2023
12 of 13 checks passed
@adriansmares adriansmares deleted the feature/rate-limit-http branch November 21, 2023 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/console This is related to the Console c/gateway conf server This is related to the Gateway Configuration Server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants