Conversation
Document shared and per-user token bucket rate limits on MCPServer resources, including per-tool overrides and combined configurations. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Adds Kubernetes Operator documentation for configuring MCPServer rate limiting and links it into the Operator docs sidebar.
Changes:
- Add a new Kubernetes guide page describing MCPServer rate limiting configuration and behavior.
- Add the new guide to the
Kubernetes Operatorsection of the Docusaurus sidebar.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
sidebars.ts |
Adds the new rate limiting guide to the Kubernetes Operator navigation. |
docs/toolhive/guides-k8s/rate-limiting.mdx |
New guide explaining rate limiting concepts and providing YAML configuration examples. |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
danbarr
left a comment
There was a problem hiding this comment.
Summary
The guide is well-structured with a clear logical flow, useful code examples, and good cross-linking coverage. Most findings are secondary. One primary issue needs attention before merge: the "Next steps" section points backward to a prerequisite.
Primary Issues
1. "Next steps" links backward to a prerequisite
The first "Next steps" entry links to Authentication and authorization. Auth is a prerequisite for this guide, not a next step — pointing backward breaks the forward-momentum principle and is confusing to a reader who just completed the guide.
Ask: Replace the auth link with a forward-pointing link. The logical next step in the sidebar is redis-session-storage (which is also a dependency for rate limiting), or token-exchange-k8s.
2. Redis address examples aren't clearly placeholders
All three YAML examples that include sessionStorage use redis.default.svc.cluster.local:6379. This looks like a concrete, copy-pasteable value, but:
- It places Redis in the
defaultnamespace, while the Redis Sentinel session storage guide deploys Redis in a dedicatedredisnamespace (redis.redis.svc.cluster.local:6379). - It uses a direct Redis address, while the session storage guide uses the Sentinel-based config — it's not clear whether rate limiting's
sessionStoragefield supports Sentinel or only direct connections.
Other values that the reader must supply use the <ALL_CAPS> convention (e.g., <YOUR_UPSTREAM_CLIENT_SECRET>). Using a concrete-looking DNS name here is inconsistent with that convention and will confuse readers who followed the Redis guide first.
Ask: Replace with a <YOUR_REDIS_ADDRESS> placeholder (consistent with the project's placeholder convention) and add a brief note pointing to Redis Sentinel session storage for deployment instructions. Alternatively, clarify whether rate limiting supports the Sentinel config or only a direct-address connection.
Secondary Issues
| Issue | Recommendation |
|---|---|
| Description opener ("How to configure...") | Per the style guide, lead with the action directly. Replace with: "Configure per-user and shared rate limits on MCPServer resources to prevent noisy neighbors and protect downstream services." Same meaning, cleaner DocCard truncation. |
| Prerequisite conflates Redis purposes | "Redis deployed for session storage (rate limiting uses Redis...)" conflates two separate uses of Redis. Rephrase: "Redis deployed in your cluster — rate limiting stores token bucket counters in Redis" to avoid implying it reuses the session storage connection. |
| Per-tool override YAML is a partial fragment | The mcpserver-pertool-ratelimit.yaml example starts at spec:, unlike the three preceding examples which are complete manifests with apiVersion/kind/metadata. Either make it a complete manifest for consistency, or add a note like "Add to your existing MCPServer spec:". |
| Sidebar order: rate-limiting precedes its Redis dependency | redis-session-storage is listed after rate-limiting in the sidebar, but Redis is required before rate limiting can be configured. A reader going in order hits the rate limiting guide before setting up Redis. Consider swapping the two entries. |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Thanks for the thorough review, Dan! Addressed all your feedback in 489df8f: Primary issues:
Secondary issues:
|
Summary
Document the rate limiting capabilities added to MCPServer in toolhive v0.17.0 (stacklok/toolhive#4550, #4704, #4718).
Video to demonstrate rendering:
Screen.Recording.2026-04-10.at.9.01.11.AM.mov
Related
Generated with Claude Code