Add per-account rate limiting for Platform API#680
Conversation
ElastiCache Valkey 9.1 replication group for shared GCRA rate limit counters, with ECS bootstrap threading to pass the Valkey endpoint through to ArgoCD cluster secret annotations and Helm values. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Per-account GCRA rate limiting via Helm values, configmap, deployment env vars, and ServiceMonitor. PrometheusRule alerts for Valkey downtime and high denial ratio, with promtool CI tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update CLAUDE.md with rate limiting feature overview, key files, and data flow. Add encryption-at-rest/in-transit security rationale to the rate limiting ADR. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThe PR provisions optional ElastiCache Valkey infrastructure, propagates its endpoint through regional bootstrap and Argo CD, configures Platform API rate limiting and metrics scraping, and adds Prometheus alerts with promtool coverage. ChangesPlatform API rate limiting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Terraform
participant ECSBootstrap
participant ClusterSecret
participant ArgoCD
participant PlatformAPI
participant Valkey
Terraform->>Valkey: provision and expose endpoint
Terraform->>ECSBootstrap: pass REDIS_ENDPOINT
ECSBootstrap->>ClusterSecret: write redis_endpoint annotation
ArgoCD->>ClusterSecret: read redis_endpoint
ArgoCD->>PlatformAPI: configure rateLimit.redisEndpoint
PlatformAPI->>Valkey: store and read GCRA counters
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/hold Awaiting review and merge for PR openshift-online/rosa-hyperfleet-api#121 before this is reviewed and merged |
openshift-ci
Bot
added
the
do-not-merge/hold
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
config/defaults.yaml (1)
70-72: 🚀 Performance & Scalability | 🔵 TrivialDefault-enabling ElastiCache Valkey for all regions.
enable_rate_limit_redisnow defaults totrue, meaning every new region (including ephemeral/dev) provisions ElastiCache Valkey unless explicitly opted out. Given the related Terraform guideline elsewhere in this repo favors defaults that provide "the lowest barrier to starting a new region," it's worth confirming this cost/complexity tradeoff is intentional for a rate-limiting feature versus defaulting new regions to off and opting in per-environment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/defaults.yaml` around lines 70 - 72, Change regional_cluster.enable_rate_limit_redis to default to false so new regions do not provision ElastiCache Valkey automatically, and update any related documentation or environment-specific configuration to explicitly opt in where rate limiting is required.terraform/modules/hyperfleet-infrastructure/elasticache.tf (1)
23-29: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueUnrestricted egress on the Valkey security group.
Egress is open to
0.0.0.0/0. This is flagged by Trivy (AWS-0104) and, per path instructions, security groups should "follow least-privilege networking." Since this SG only needs to serve inbound Valkey connections and doesn't need broad outbound internet access, egress could be scoped down (e.g., restricted to VPC CIDR or removed entirely if not needed for connection tracking).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@terraform/modules/hyperfleet-infrastructure/elasticache.tf` around lines 23 - 29, Restrict the outbound rules in the Valkey security group’s egress block to the required VPC CIDR, or remove the egress block if outbound access is unnecessary; do not allow unrestricted 0.0.0.0/0 traffic.Sources: Path instructions, Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@argocd/config/regional-cluster/alerting-rules/templates/ratelimit.yaml`:
- Around line 23-29: Update the RateLimitHighDenialRatio Prometheus expression
to aggregate over-limit request rates with sum by(method, path) before dividing
by the route-wide total, then remove result: over_limit from the corresponding
test expectation.
In `@argocd/config/regional-cluster/platform-api/values.yaml`:
- Around line 20-21: The image configuration currently uses the mutable
“ratelimit” tag with imagePullPolicy: Always; replace the tag under the platform
API image settings with the required immutable image digest SHA, preserving the
existing repository and pull policy.
In `@ci/promtool-test/ratelimit-rules_test.yaml`:
- Around line 35-78: The RateLimitHighDenialRatio alert currently evaluates
denial ratios per pod, so it can miss breaches across multiple instances. Update
the alert expression to sum
rate(ratelimit_requests_total{result="over_limit"}[5m]) by method and path
before dividing, and add a multi-pod test case in the RateLimitHighDenialRatio
alert_rule_test scenarios where the combined ratio exceeds 50%.
In `@CLAUDE.md`:
- Line 182: Update the Valkey description in CLAUDE.md to qualify the 20% cost
savings as applying specifically to node-based ElastiCache deployments, and add
an AWS ElastiCache FAQ or pricing reference link supporting the claim.
In `@docs/design/rate-limiting-architecture.md`:
- Line 675: Update the “Encryption at rest / in transit” section to explicitly
address Valkey encryption and authentication: either document and implement
TLS/authentication in the rate-limiter client configuration, or record a named
architect/compliance exception approving plaintext operation despite account IDs
in keys and relying only on network controls.
---
Nitpick comments:
In `@config/defaults.yaml`:
- Around line 70-72: Change regional_cluster.enable_rate_limit_redis to default
to false so new regions do not provision ElastiCache Valkey automatically, and
update any related documentation or environment-specific configuration to
explicitly opt in where rate limiting is required.
In `@terraform/modules/hyperfleet-infrastructure/elasticache.tf`:
- Around line 23-29: Restrict the outbound rules in the Valkey security group’s
egress block to the required VPC CIDR, or remove the egress block if outbound
access is unnecessary; do not allow unrestricted 0.0.0.0/0 traffic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8b62951f-eea6-426a-a29a-f3c365ea359b
📒 Files selected for processing (28)
CLAUDE.mdargocd/config/regional-cluster/alerting-rules/templates/ratelimit.yamlargocd/config/regional-cluster/platform-api/templates/deployment.yamlargocd/config/regional-cluster/platform-api/templates/ratelimit-configmap.yamlargocd/config/regional-cluster/platform-api/templates/servicemonitor.yamlargocd/config/regional-cluster/platform-api/values.yamlci/promtool-test/ratelimit-rules_test.yamlconfig/defaults.yamlconfig/templates/argocd-bootstrap/applicationset.yaml.j2config/templates/pipeline-regional-cluster-inputs/terraform.json.j2deploy/ephemeral/us-east-1/_merged_config.yamldeploy/ephemeral/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamldeploy/ephemeral/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yamldeploy/ephemeral/us-east-1/pipeline-regional-cluster-inputs/terraform.jsondeploy/integration/us-east-1/_merged_config.yamldeploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamldeploy/integration/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yamldeploy/integration/us-east-1/pipeline-regional-cluster-inputs/terraform.jsondocs/design/rate-limiting-architecture.mdscripts/bootstrap-argocd.shterraform/config/regional-cluster/main.tfterraform/config/regional-cluster/outputs.tfterraform/config/regional-cluster/variables.tfterraform/modules/ecs-bootstrap/main.tfterraform/modules/ecs-bootstrap/variables.tfterraform/modules/hyperfleet-infrastructure/elasticache.tfterraform/modules/hyperfleet-infrastructure/outputs.tfterraform/modules/hyperfleet-infrastructure/variables.tf
| repository: quay.io/travi/rosa-hyperfleet-api | ||
| tag: "ratelimit" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Replace the mutable ratelimit image tag with an immutable SHA.
A mutable tag plus imagePullPolicy: Always allows different code to be deployed without a GitOps change. Use the required SHA-pinned image tag before merge. The PR objective explicitly calls for this replacement.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@argocd/config/regional-cluster/platform-api/values.yaml` around lines 20 -
21, The image configuration currently uses the mutable “ratelimit” tag with
imagePullPolicy: Always; replace the tag under the platform API image settings
with the required immutable image digest SHA, preserving the existing repository
and pull policy.
There was a problem hiding this comment.
This will be changed after merging the API changes - correct?
There was a problem hiding this comment.
@psav Correct. Per the PR description, this is currently pinned to quay.io/travi/rosa-hyperfleet-api:ratelimit as a placeholder while the dependent rosa-hyperfleet-api PR #121 is under review. Once that PR is merged, the tag here will be replaced with a SHA-pinned image reference before this PR is merged. No action needed on this right now — I'll leave this open as a reminder until that swap happens.
| # RateLimitHighDenialRatio fires when > 50% of requests are over_limit for > 10m | ||
| - interval: 1m | ||
| input_series: | ||
| - series: 'ratelimit_requests_total{result="over_limit", method="GET", path="/api/v0/clusters"}' | ||
| values: "0+6x20" | ||
| - series: 'ratelimit_requests_total{result="ok", method="GET", path="/api/v0/clusters"}' | ||
| values: "0+4x20" | ||
| alert_rule_test: | ||
| - eval_time: 11m | ||
| alertname: RateLimitHighDenialRatio | ||
| exp_alerts: | ||
| - exp_labels: | ||
| severity: warning | ||
| method: GET | ||
| path: /api/v0/clusters | ||
| result: over_limit | ||
| exp_annotations: | ||
| summary: "Over 50% of requests are being rate limited" | ||
| description: "More than 50% of rate-limited requests on method=GET, path=/api/v0/clusters have been denied for over 10 minutes. This may indicate a misbehaving client or rate limits that need tuning." | ||
|
|
||
| # RateLimitHighDenialRatio does NOT fire when denial ratio is below 50% | ||
| # over_limit is 10% of total traffic (1 denied per 9 ok) | ||
| - interval: 1m | ||
| input_series: | ||
| - series: 'ratelimit_requests_total{result="over_limit", method="GET", path="/api/v0/clusters"}' | ||
| values: "0+1x20" | ||
| - series: 'ratelimit_requests_total{result="ok", method="GET", path="/api/v0/clusters"}' | ||
| values: "0+9x20" | ||
| alert_rule_test: | ||
| - eval_time: 15m | ||
| alertname: RateLimitHighDenialRatio | ||
| exp_alerts: [] | ||
|
|
||
| # RateLimitHighDenialRatio does NOT fire before 10m even with high denial ratio | ||
| - interval: 1m | ||
| input_series: | ||
| - series: 'ratelimit_requests_total{result="over_limit", method="GET", path="/api/v0/clusters"}' | ||
| values: "0+9x8" | ||
| - series: 'ratelimit_requests_total{result="ok", method="GET", path="/api/v0/clusters"}' | ||
| values: "0+1x8" | ||
| alert_rule_test: | ||
| - eval_time: 8m | ||
| alertname: RateLimitHighDenialRatio | ||
| exp_alerts: [] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Aggregate over_limit before dividing. Traffic split across pods/instances can keep each series under 50% even when the combined denial ratio exceeds the threshold. Add a multi-pod case here, and sum rate(ratelimit_requests_total{result="over_limit"}[5m]) by method, path in the alert.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ci/promtool-test/ratelimit-rules_test.yaml` around lines 35 - 78, The
RateLimitHighDenialRatio alert currently evaluates denial ratios per pod, so it
can miss breaches across multiple instances. Update the alert expression to sum
rate(ratelimit_requests_total{result="over_limit"}[5m]) by method and path
before dividing, and add a multi-pod test case in the RateLimitHighDenialRatio
alert_rule_test scenarios where the combined ratio exceeds 50%.
|
|
||
| ### Rate Limiting | ||
|
|
||
| The Platform API implements per-account rate limiting using the GCRA (Generic Cell Rate Algorithm) via `go-redis/redis_rate`. Rate limit counters are stored in ElastiCache Valkey 9.1, chosen over Redis OSS for 20% lower cost, open-source licensing (BSD 3-Clause), and performance improvements. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
AWS ElastiCache Valkey 20% lower cost node-based pricing FAQ
💡 Result:
Amazon ElastiCache for Valkey offers node-based pricing that is 20% lower than comparable ElastiCache for Redis OSS configurations [1][2][3]. This pricing structure is a permanent feature of the service [4], not a temporary promotion, and is available for all node-based ElastiCache deployments [4]. Key details regarding this pricing and engine choice include: Service Comparison: The 20% cost reduction applies specifically to self-designed (node-based) clusters [2][5]. For users utilizing ElastiCache Serverless, Valkey offers a 33% lower price compared to Redis OSS [2][4][6]. Technical Compatibility: Valkey is an open-source, high-performance, key-value datastore stewarded by the Linux Foundation [1][4]. Because it maintains API compatibility with Redis OSS 7.2, it is designed as a drop-in replacement that typically does not require application code changes [7][8]. Reserved Nodes: If you are currently using ElastiCache reserved nodes for Redis OSS, you can transition to ElastiCache for Valkey and retain your existing discounted reserved node rates across all node sizes within the same family [4]. Strategic Recommendation: AWS recommends Valkey for new deployments [6][8] due to its lower cost, ongoing performance enhancements (such as improved multithreading and memory efficiency), and active open-source development [7][4][9]. For the most accurate and up-to-date pricing for specific regions and node types, please consult the official Amazon ElastiCache pricing page [2].
Citations:
- 1: https://aws.amazon.com/elasticache/faqs/
- 2: https://aws.amazon.com/elasticache/pricing/
- 3: https://www.amazonaws.cn/en/new/2024/announcing-amazon-elasticache-for-valkey/
- 4: https://aws.amazon.com/about-aws/whats-new/2024/10/amazon-elasticache-valkey/
- 5: https://aws.amazon.com/blogs/database/reduce-your-amazon-elasticache-costs-by-up-to-60-with-valkey-and-cudos/
- 6: https://www.usage.ai/blogs/aws/database-savings-plans/elasticache/serverless/
- 7: https://aws.amazon.com/blogs/database/migrating-to-amazon-elasticache-for-valkey-best-practices-and-a-customer-success-story/
- 8: https://www.wring.co/blog/aws-elasticache-pricing-guide
- 9: https://aws.amazon.com/blogs/database/how-alight-solutions-achieved-60-cost-savings-with-amazon-elasticache-for-valkey/
Qualify the Valkey cost claim. The 20% savings applies to node-based ElastiCache deployments; make that scope explicit and add an AWS ElastiCache FAQ/pricing reference.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CLAUDE.md` at line 182, Update the Valkey description in CLAUDE.md to qualify
the 20% cost savings as applying specifically to node-based ElastiCache
deployments, and add an AWS ElastiCache FAQ or pricing reference link supporting
the claim.
Source: MCP tools
| - SigV4 identity headers (`X-Caller-Account`, `X-Caller-Arn`) are injected by API Gateway after authentication and cannot be forged by customers | ||
| - Fail-open behavior on Redis failure is a deliberate choice — matches ROSA HCP v1's Limitador fail-open pattern | ||
| - API GW throttle fail-closes by default (returns 429) | ||
| - **Encryption at rest / in transit**: Not enabled on ElastiCache Valkey. The data stored is purely ephemeral GCRA counters (e.g., `rl:account:method:path → count`) — no PII, credentials, or customer data. All traffic flows within the same VPC between private subnets, secured by security groups with no cross-network-boundary exposure. Enabling TLS would add latency to every rate limit check without meaningful security benefit for ephemeral counters. If FedRAMP compliance (HPSTRAT-11) requires encryption regardless of data sensitivity, this should be revisited as part of a broader infrastructure encryption effort across all managed services. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the referenced section and related terms in the design doc.
git ls-files 'docs/design/rate-limiting-architecture.md' 'docs/design/**' | sed -n '1,120p'
echo
rg -n -i 'encryption at rest|in transit|Valkey|GCRA|rl:|account identifier|account id|Platform API client|TLS|security group|private subnet|FedRAMP|HPSTRAT-11|architect' docs/design/rate-limiting-architecture.md
echo
# Show the surrounding lines for the cited snippet.
sed -n '640,705p' docs/design/rate-limiting-architecture.mdRepository: openshift-online/rosa-hyperfleet
Length of output: 8710
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for the key format and any client config references elsewhere in the repo.
rg -n -i 'rl:account:method:path|account identifier|platform api client|valkey|redis|tls|in transit encryption|encryption at rest|fedramp|hpstrat-11' .Repository: openshift-online/rosa-hyperfleet
Length of output: 49975
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Valkey Terraform resource and surrounding docs for encryption/auth settings.
sed -n '1,220p' terraform/modules/hyperfleet-infrastructure/elasticache.tf
echo
sed -n '1,120p' terraform/modules/hyperfleet-infrastructure/README.md
echo
sed -n '670,680p' docs/design/rate-limiting-architecture.mdRepository: openshift-online/rosa-hyperfleet
Length of output: 11066
Treat Valkey encryption/auth as an explicit exception decision. The key format includes the caller account ID (rl:{account}:...), and private subnets/security groups do not provide encryption or server authentication. Either enable Valkey TLS/auth and update the client config, or record an architect/compliance exception for plaintext operation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/rate-limiting-architecture.md` at line 675, Update the
“Encryption at rest / in transit” section to explicitly address Valkey
encryption and authentication: either document and implement TLS/authentication
in the rate-limiter client configuration, or record a named architect/compliance
exception approving plaintext operation despite account IDs in keys and relying
only on network controls.
Sources: Coding guidelines, Path instructions, Learnings, MCP tools
psav
left a comment
psav
left a comment
There was a problem hiding this comment.
A couple comments but minor
| repository: quay.io/travi/rosa-hyperfleet-api | ||
| tag: "ratelimit" |
There was a problem hiding this comment.
This will be changed after merging the API changes - correct?
| summary: "Over 50% of requests are being rate limited" | ||
| description: "More than 50% of rate-limited requests on method=GET, path=/api/v0/clusters have been denied for over 10 minutes. This may indicate a misbehaving client or rate limits that need tuning." | ||
|
|
||
| # RateLimitHighDenialRatio fires when combined ratio across pods exceeds 50% |
There was a problem hiding this comment.
Are we concerned about a single pod with high individual denial rate, potentially signifying an error? It would probably lead to a reduced HA scenario?
2 participants
Add per-account rate limiting for Platform API
Summary
CLAUDE.mdand the ADRDetails
Implements per-account rate limiting at the Platform API layer using the GCRA (Generic Cell Rate Algorithm) via
go-redis/redis_rate, with ElastiCache Valkey as the shared counter store. Seedocs/design/rate-limiting-architecture.mdfor the full ADR.Architecture: API Gateway (global throttle) → Platform API middleware (per-account GCRA) → ElastiCache Valkey (shared counters)
Key design decisions:
X-Amz-Account-Id— trusted, not spoofable (unlike v1's source IP)Rate limits are configured per-route with 2x burst headroom — balanced starting values for iterative tuning based on metrics/alerts/logs:
POST /clustersGET /clusters(list)GET /clusters/*PATCH /clusters/*DELETE /clusters/*POST /nodepoolsAlerting:
RateLimitValkeyDown(critical) — fires when rate limiter fails open for >2mRateLimitHighDenialRatio(warning) — fires when >50% of requests are denied for >10mData flow for Valkey endpoint: Terraform output →
bootstrap-argocd.sh→ ECS task env var → cluster secret annotation → ApplicationSetvaluesObject→ Helm values →REDIS_ENDPOINTenv var on platform-api podsWarning
Note: The
platform-apiimage is currently overridden to fork (quay.io/travi/rosa-hyperfleet-api:ratelimit) and will need to be updated with SHA ID once PR openshift-online/rosa-hyperfleet-api#121 is mergedTest plan
make pre-pushpasses (terraform-validate,helm-lint,check-docs,promtool-test)terraform planplatform-apipod env varsmake ephemeral-e2e E2E_LABEL_FILTER='ratelimit'to validate rate limiting behaviorServiceMonitoris discovered andratelimit_requests_totalmetrics appear in Thanos🤖 Assisted by Claude Code
Summary by CodeRabbit