Skip to content

Conversation

@sammy200-ui
Copy link
Contributor

@sammy200-ui sammy200-ui commented Nov 20, 2025

Description
Implements HTTP server-side rate limiting middleware using token bucket algorithm for #2551 (HTTP portion).

Changes

  • Add RateLimiter middleware using golang.org/x/time/rate
  • Support per-IP and global rate limiting modes
  • Add ErrorTooManyRequests (HTTP 429) error type
  • Include 9 comprehensive test cases (all passing)
  • Add example application with full documentation
  • Auto-skip rate limiting for health check endpoints
  • Automatic cleanup of stale per-IP limiters every 5 minutes

Features

  • Token Bucket Algorithm: Smooth rate limiting with configurable burst
  • Per-IP Limiting: Each client IP gets independent rate limit (configurable)
  • Health Check Exemption: /.well-known/alive and /.well-known/health always accessible
  • Prometheus Metrics: Track rate limit violations via app_http_rate_limit_exceeded_total
  • IP Extraction: Supports X-Forwarded-For, X-Real-IP, and RemoteAddr

Testing
All tests pass:

go test ./pkg/gofr/http/middleware/... -v -run TestRateLimiter
# PASS: 9/9 tests

Additional Information:

Uses golang.org/x/time/rate (already in dependencies)
Metrics exposed: app_http_rate_limit_exceeded_total counter
Example application available at using-rate-limiter
Complete documentation in example README with testing instructions
This PR implements the HTTP portion of #2551.

Checklist:

  • I have formatted my code using goimport and golangci-lint.
  • All new code is covered by unit tests.
  • This PR does not decrease the overall code coverage.
  • I have reviewed the code comments and documentation for clarity.

Thank you for your contribution!

- Add token bucket rate limiting using golang.org/x/time/rate
- Support per-IP and global rate limiting modes
- Add ErrorTooManyRequests (HTTP 429) error type
- Include comprehensive tests (9 test cases, all passing)
- Add example application with documentation
- Skip rate limiting for health check endpoints
- Automatic cleanup of stale per-IP limiters

Implements gofr-dev#2551 (HTTP portion)
@sammy200-ui
Copy link
Contributor Author

@coolwednesday i have raised the pr , you can review it

@sammy200-ui
Copy link
Contributor Author

@Umang01-hash just a follow up on the pr, any changes or fixes for the feat

@Umang01-hash
Copy link
Member

@sammy200-ui We will review your PR soon. Once reviewed we will let you know about any changes required for it.

Thanks for contributing to GoFr.

@coolwednesday
Copy link
Member

coolwednesday commented Nov 24, 2025

@sammy200-ui , is this feature only for single pod servers. How do you think we should handle rate limiting in case of distributed systems ?

@sammy200-ui
Copy link
Contributor Author

sammy200-ui commented Nov 24, 2025

@coolwednesday Yes, currently this works for single-pod deployments. For distributed systems, each pod would enforce limits independently, so a client could exceed the global limit by hitting different pods, to handle this we would need ,redis-backed limiter or api gateway rate limiting or have document the limitation and keep it simple for v1

@coolwednesday
Copy link
Member

coolwednesday commented Nov 24, 2025

@sammy200-ui, Let's keep it simple and create another issue for that linking it to this. Also document this behaviour. However, I would suggest at least to make the factory function configurable enough to be extensive to Redis in future.

@sammy200-ui
Copy link
Contributor Author

@coolwednesday ok so i will add limiterBackend interface that will makes it easy to add Redis/other backends in future , add notes in code comments and example README explaining single-pod behavior , would that be good for this?? ,also do you want me to create a follow up issue for redis backed limiter or would you like to assign to someone else?

@Umang01-hash
Copy link
Member

@coolwednesday ok so i will add limiterBackend interface that will makes it easy to add Redis/other backends in future , add notes in code comments and example README explaining single-pod behavior , would that be good for this?? ,also do you want me to create a follow up issue for redis backed limiter or would you like to assign to someone else?

@sammy200-ui I already defined one RateLimiterStore interface while implementing it for http-service option. Maybe you can have a look and use it :

// RateLimiterStore abstracts the storage and cleanup for rate limiter buckets.
type RateLimiterStore interface {
	Allow(ctx context.Context, key string, config RateLimiterConfig) (allowed bool, retryAfter time.Duration, err error)
	StartCleanup(ctx context.Context)
	StopCleanup()
}

The goal then will be we will use a single implementation for rate limiter of http service also and http or grpc also. This helps us to resuing the same code and having a consistent functionality throughout. You can make it a part of another ticket for time.

@coolwednesday thoughts?

@sammy200-ui
Copy link
Contributor Author

@Umang01-hash @coolwednesday The RateLimiterStore interface is for outbound HTTP client rate limiting, while my implementation is for incoming HTTP middleware.
They serve different purposes:
Your interface: Limits outbound requests to other services
My implementation: Limits incoming requests to the server

However, I see the value in consistency. Two options:

1 : Keep this PR simple (in-memory only) and create a follow-up ticket to align both implementations with a shared interface + add Redis support for both.

2: Refactor now to use RateLimiterStore, but it may delay this PR.

I prefer 1 to keep this focused. Thoughts?

@coolwednesday
Copy link
Member

@Umang01-hash @coolwednesday The RateLimiterStore interface is for outbound HTTP client rate limiting, while my implementation is for incoming HTTP middleware. They serve different purposes: Your interface: Limits outbound requests to other services My implementation: Limits incoming requests to the server

However, I see the value in consistency. Two options:

1 : Keep this PR simple (in-memory only) and create a follow-up ticket to align both implementations with a shared interface + add Redis support for both.

2: Refactor now to use RateLimiterStore, but it may delay this PR.

I prefer 1 to keep this focused. Thoughts?

You can implement the interface and return errors as not supported when redis configs are passed. You can then pick this up in a subsequent PR.

@sammy200-ui
Copy link
Contributor Author

@Umang01-hash @coolwednesday I've refactored the rate limiter to use the RateLimiterStore interface pattern.

Changes made:

Created rate_limiter_store.go with the RateLimiterStore interface and memoryRateLimiterStore implementation
Refactored RateLimiter middleware to use the store pattern via an optional Store field in config
In-memory implementation works now with automatic cleanup
All tests passing with backward compatibility maintained
Added documentation about single-pod limitation

Redis support: As you suggested, I've left Redis implementation for a future PR. The store interface makes it easy to add later without breaking changes.

Let me know if you'd like any adjustments!

@sammy200-ui
Copy link
Contributor Author

@coolwednesday @Umang01-hash any new changes to be expected ?

@sammy200-ui
Copy link
Contributor Author

@Umang01-hash @coolwednesday just a follow up on the pr

Copilot AI review requested due to automatic review settings December 8, 2025 05:18
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements HTTP server-side rate limiting middleware using the token bucket algorithm to protect APIs from abuse and ensure fair resource distribution. The implementation supports both per-IP and global rate limiting modes with automatic cleanup of stale limiters.

Key Changes:

  • Added RateLimiter middleware with configurable request rate and burst capacity
  • Implemented in-memory rate limiter store with automatic cleanup of stale entries
  • Added ErrorTooManyRequests HTTP error type for 429 responses
  • Included comprehensive test suite with 9 test cases covering various scenarios
  • Provided example application with detailed documentation

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
pkg/gofr/http/middleware/rate_limiter.go Core middleware implementation with IP extraction and rate limit enforcement
pkg/gofr/http/middleware/rate_limiter_store.go In-memory storage backend with periodic cleanup of stale limiters
pkg/gofr/http/middleware/rate_limiter_test.go Comprehensive test suite covering global/per-IP limits, health endpoints, concurrency, and token refill
pkg/gofr/http/errors.go Added ErrorTooManyRequests error type with 429 status code and WARN log level
examples/using-rate-limiter/main.go Example application demonstrating rate limiter configuration and usage
examples/using-rate-limiter/README.md Detailed documentation with configuration options, testing instructions, and deployment considerations

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@sammy200-ui
Copy link
Contributor Author

Hey @Umang01-hash do i need to resolve the co-pilot requests?

@Umang01-hash
Copy link
Member

Hey @Umang01-hash do i need to resolve the co-pilot requests?

Yes @sammy200-ui if the comments given by co-pilot seems meaningful you should definitely resolve it. If not please comment why you think the given review comment is not useful/meaninful.

- Add sync.Once to prevent race conditions and double-close panics
- Add config validation with static errors
- Add RFC 6585 compliant Retry-After header
- Add TrustedProxies security feature to prevent IP spoofing
- Store config in memoryRateLimiterStore for consistency
- Add comprehensive tests for new functionality
- Document cleanup goroutine lifetime

Resolves all 8 Copilot review comments
@sammy200-ui
Copy link
Contributor Author

sammy200-ui commented Dec 8, 2025

Hello @Umang01-hash I have addressed all 8 Copilot review comments , let me know if there are any more changes

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@sammy200-ui
Copy link
Contributor Author

@Umang01-hash Sounds good. Let me know if you need any changes from my end.

Copy link
Member

@Umang01-hash Umang01-hash left a comment

Choose a reason for hiding this comment

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

@sammy200-ui Creating a separate example for this functionality may not be a very good option. Either we can try to fit it in an already existing example or only update the documentation. Also we always try and keep our documentation simple to the point and clear. I think we can remove Running the example, Testing, monitoring, etc from the docs you created.

You should create a new documentation inside the docs directory and delete this new using-rate-limiter directory from examples.

@Umang01-hash
Copy link
Member

Screenshot 2025-12-16 at 1 17 25 PM

@sammy200-ui Please also resolve these code quality issues in the PR. Thanks for the patience.

- Fix wsl linting issues (add whitespace before variable declarations)
- Move documentation to docs/advanced-guide/middlewares/page.md
- Remove examples/using-rate-limiter directory as requested
- Unexport error variables (ErrInvalidRequestsPerSecond -> errInvalidRequestsPerSecond)
- Reuse existing metrics interface instead of custom rateLimiterMetrics
- Fix IP extraction to validate trimmed values are non-empty
- Fix retry-after calculation to use actual rate (not fixed 1 second)
- Refactor getIP() into helper functions to reduce complexity
@sammy200-ui
Copy link
Contributor Author

Hello @Umang01-hash All review feedback has been addressed!
Let me know if you need any changes from my end.

@sammy200-ui
Copy link
Contributor Author

@Umang01-hash some code quality errors that are fixed , you can run the test again

Umang01-hash
Umang01-hash previously approved these changes Dec 22, 2025
@sammy200-ui
Copy link
Contributor Author

@Umang01-hash Hi! Just checking in on this PR since it’s already approved.
Let me know if there’s anything else needed from my side before merge. Thanks!

@Umang01-hash
Copy link
Member

@sammy200-ui Hey! Your PR has some review comments on it. Can you please resolve it so we can re-review and proceed towards merging if all things have been addressed. Please let us know if you need any assistance for the same.

@sammy200-ui
Copy link
Contributor Author

@Umang01-hash @coolwednesday oks sure i will make these changes

Umang01-hash and others added 2 commits January 12, 2026 10:44
…miter

- Fix memory pattern on concurrent LoadOrStore by checking loaded flag
- Add fallback to 'unknown' key when getIP returns empty string
- Replace Allow()+Reserve() with single Reserve() to avoid race condition
- Add calculateSafeDelay() with bounds checking (min 1ms, max 1min)
- Add MaxKeys config field (default 100k) to prevent memory exhaustion
- Add 6 new tests covering all fixes
@sammy200-ui
Copy link
Contributor Author

Hello @Umang01-hash @coolwednesday done with the changes , you can review the pr again. let me know if any more changes to be made

@coolwednesday coolwednesday force-pushed the feature/http-rate-limiter branch from 52553df to 1933b70 Compare January 16, 2026 05:19
@coolwednesday coolwednesday merged commit 995a595 into gofr-dev:development Jan 16, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants