Add rate limiting, production server hardening, and CD workflow#20
Add rate limiting, production server hardening, and CD workflow#20pascaljoly wants to merge 1 commit into
Conversation
Closes Ribbit-Network#17: per-API-key token-bucket rate limiter (60 req/s, burst 30) in new internal/ratelimit package, wired into /data after auth. Closes Ribbit-Network#15: http.Server with read/write/idle timeouts, graceful SIGTERM shutdown, /healthz endpoint, CORS middleware, and a fly.io CD workflow (.github/workflows/deploy.yml) that deploys on push to main. Issue Ribbit-Network#14 (API keys) was already implemented in the codebase. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Hey @pascaljoly! Thanks for the new changeset! I see the CI is failing due to a license key issue. I thought I had sorted that out for the secret monitoring, but let me fix it again. |
keenanjohnson
left a comment
keenanjohnson
left a comment
There was a problem hiding this comment.
Ok this is a good start, but there are a few changes I think we need to make before we merge this. Let me know if you have bandwidth to do so, but if not I can knock them out @pascaljoly
| } | ||
|
|
||
| requireKey := auth.Require(store) | ||
| // 60 requests/minute per key with a burst of 30. |
There was a problem hiding this comment.
The comment here and the burst limit in the code below disagree. The comment says burst 30, but the code below is burst 60.
| }) | ||
| } | ||
|
|
||
| func (l *Limiter) get(key string) *rate.Limiter { |
There was a problem hiding this comment.
The actual rate limited never removes a client when added, which will be an infinite memory leak. We should eventually remove clients after some time threshold or use a library like https://github.com/didip/tollbooth which handles the removal.
| return lim | ||
| } | ||
|
|
||
| func extractKey(r *http.Request) string { |
There was a problem hiding this comment.
This function appears to be a duplicate of the one in middleware.go
There was a problem hiding this comment.
We should probably use the authentication layer. The auth should then stash the verified key in r.Context() and let ratelimit read it from there.
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: superfly/flyctl-actions/setup-flyctl@master | ||
| - run: flyctl deploy --remote-only |
There was a problem hiding this comment.
This deploys no matter what, even if the tests fail etc. Either needs: the CI job or run go test ./... before flyctl deploy
|
@pascaljoly I've also investigated the gitleaks license key issue and it seems that a limitation of the gitleaks secret scanning is that it doesn't work on community fork contributions like yours per this github issue: gitleaks/gitleaks#2063 You can see new PR's I opened from inside the repo run fine, like this one, which means the API key is working: #21 I'll try to investigate a workaround but for now, let's just ignore that failing check. |
2 participants
Closes #14, #15, #17
Changes
Issue #17 — Rate limiting
internal/ratelimitpackage: per-API-key token-bucket limiter (60 req/s, burst 30)/dataafter the auth middlewareIssue #15 — Deploy to Hosting
http.Serverwith 30s read/write + 120s idle timeouts/healthzendpoint for fly.io health checks.github/workflows/deploy.yml— auto-deploys to fly.io on push to main (requiresFLY_API_TOKENGitHub secret)Issue #14 — API Keys