Skip to content

impr(WKBCH-23): Add rate limit supprt#78

Merged
tmacro merged 1 commit into
mainfrom
improvement/WKBCH-23/rate_limit_support
Apr 30, 2026
Merged

impr(WKBCH-23): Add rate limit supprt#78
tmacro merged 1 commit into
mainfrom
improvement/WKBCH-23/rate_limit_support

Conversation

@tmacro
Copy link
Copy Markdown
Contributor

@tmacro tmacro commented Apr 29, 2026

No description provided.

Comment thread templates/cloudserver/config-v9.json Outdated
Comment thread templates/cloudserver/config-v7.json Outdated
Comment thread cmd/config.go
Lifecycle: LifecycleFeatureConfig{
Enabled: false,
},
RateLimiting: RateLimitingFeatureConfig{
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Rate limiting requires Redis (via localCache), but the Redis service in docker-compose.yaml does not include a feature-rate-limiting profile. If a user enables only rate limiting (without utapi, CRR, etc.), Redis won't start and cloudserver will fail to connect. A feature-rate-limiting profile needs to be added to the Redis service, and getComposeProfiles in cmd/util.go needs a corresponding entry for RateLimiting.Enabled.

— Claude Code

@claude
Copy link
Copy Markdown

claude Bot commented Apr 29, 2026

  • cmd/config.go:302 — Missing Redis profile for rate limiting. Rate limiting depends on Redis (via localCache), but neither docker-compose.yaml nor getComposeProfiles in cmd/util.go handle a feature-rate-limiting profile. Enabling only rate limiting will leave Redis unstarted.
  • templates/cloudserver/config-v9.json:213,224 — configCacheTTL hardcoded to 30000 instead of using the ConfigCacheTTL config field from RateLimitingDefaultLimits. Users can't override it.
  • templates/cloudserver/config-v7.json:131,142 — Same hardcoded configCacheTTL issue as v9.

Review by Claude Code

"limit": {{ .Features.RateLimiting.Account.RequestsPerSecond.Limit }},
"burstCapacity": {{ .Features.RateLimiting.Account.RequestsPerSecond.BurstCapacity }}
}
},
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

configCacheTTL is hardcoded to 30000 here instead of using the template variable. The bucket section correctly uses {{ .Features.RateLimiting.Bucket.ConfigCacheTTL }}, but the account section ignores the config value.

Suggested change
},
"configCacheTTL": {{ .Features.RateLimiting.Account.ConfigCacheTTL }}

— Claude Code

{{- end }}
"configCacheTTL": 30000
},
"error": {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same hardcoded configCacheTTL issue as in config-v9.json — should use {{ .Features.RateLimiting.Account.ConfigCacheTTL }} to respect user config.

Suggested change
"error": {
"configCacheTTL": {{ .Features.RateLimiting.Account.ConfigCacheTTL }}

— Claude Code

{{ if .Features.Utapi.Enabled }}
{{ if or .Features.Utapi.Enabled .Features.RateLimiting.Enabled }}
"localCache": {
"host": "localhost",
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The templates now require Redis (localCache) when rate limiting is enabled, but the Redis service in docker-compose.yaml does not include a rate-limiting profile, and getComposeProfiles() in cmd/util.go has no entry for features.rate_limiting. If a user enables only rate limiting (no utapi, crr, notifications, migration, or lifecycle), Redis will not start and cloudserver will fail to connect.

You need to: (1) add a profile like feature-rate-limiting to the Redis service in docker-compose.yaml, and (2) add a corresponding block in getComposeProfiles() to activate it when cfg.Features.RateLimiting.Enabled is true.

— Claude Code

@claude
Copy link
Copy Markdown

claude Bot commented Apr 29, 2026

  • Bug: Redis won't start with rate limiting alonetemplates/cloudserver/config-v{7,9}.json now emit localCache when rate limiting is enabled, requiring Redis. But the Redis service in docker-compose.yaml has no feature-rate-limiting profile, and cmd/util.go:getComposeProfiles() has no entry for it. Enabling only rate limiting means cloudserver will try to connect to Redis but Redis won't be running.
  • Bug: Hardcoded configCacheTTL in account sectiontemplates/cloudserver/config-v7.json:144 and config-v9.json:222 hardcode "configCacheTTL": 30000 in the account rate limiting block instead of using {{ .Features.RateLimiting.Account.ConfigCacheTTL }} like the bucket section does.
  • Typo in PR title — "supprt" → "support"

Review by Claude Code

@tmacro tmacro force-pushed the improvement/WKBCH-23/rate_limit_support branch from 551d6a2 to a3d5a32 Compare April 29, 2026 20:07
Comment thread cmd/config.go
@@ -275,6 +299,19 @@ func DefaultEnvironmentConfig() EnvironmentConfig {
Lifecycle: LifecycleFeatureConfig{
Enabled: false,
},
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Every other feature in DefaultEnvironmentConfig() explicitly sets Enabled: false (e.g., Utapi, Migration, Lifecycle). While Go's zero value handles this correctly, adding it here keeps the defaults self-documenting and consistent.

Suggested change
},
RateLimiting: RateLimitingFeatureConfig{
Enabled: false,
Bucket: RateLimitingDefaultLimits{

— Claude Code

@claude
Copy link
Copy Markdown

claude Bot commented Apr 29, 2026

  • cmd/config.go:301 — RateLimiting default missing explicit Enabled: false unlike all other features in DefaultEnvironmentConfig()

    Everything else looks solid: JSON templates produce valid output in all feature-flag combinations (utapi on/off × rate-limiting on/off), localCache is correctly gated behind or .Features.Utapi.Enabled .Features.RateLimiting.Enabled, the feature-rate-limiting profile is properly added to the redis service, and the config structs/defaults are well-structured.

    Review by Claude Code

@tmacro tmacro force-pushed the improvement/WKBCH-23/rate_limit_support branch from a3d5a32 to 7e1c043 Compare April 29, 2026 20:53
Comment thread cmd/util.go Outdated
Comment thread templates/global/docker-compose.yaml Outdated
volumes:
- ./config/vault/:/secrets
profiles:
- base
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

setup-cloudserver is in the base profile, so it runs for ALL environments — even those not using rate limiting. This forces every user to pull SCUBA_IMAGE and run the rate-limit service user creation script. It should use feature-rate-limiting profile, matching the pattern of setup-scuba in feature-scuba.

— Claude Code

Comment thread templates/global/docker-compose.yaml Outdated
@claude
Copy link
Copy Markdown

claude Bot commented Apr 29, 2026

  • cmd/util.go:29 — template.New("base") creates a misnamed root template. ParseFS names templates after file basenames, so Execute will fail with "incomplete or empty template". Use template.New(filepath.Base(path)) instead.
  • templates/global/docker-compose.yaml:91 — setup-cloudserver is in base profile but only needed for rate limiting. This forces all environments to pull SCUBA_IMAGE and run the service-user creation script. Should use feature-rate-limiting profile.
  • templates/global/docker-compose.yaml:55-58 — cloudserver unconditionally depends_on setup-cloudserver. If setup-cloudserver moves to feature-rate-limiting profile, this dependency will cause Compose errors when rate limiting is disabled.

Review by Claude Code

@tmacro tmacro force-pushed the improvement/WKBCH-23/rate_limit_support branch from 7e1c043 to 54e45a3 Compare April 30, 2026 16:45
volumes:
- ./config/vault/:/secrets
profiles:
- feature-rate-limiting
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

cloudserver (in the base profile) has no depends_on for setup-rate-limiting-svc-user. When rate limiting is enabled, cloudserver could start and receive requests before the rate-limit service user is created in vault. If cloudserver needs the service user ARN to be resolvable at request time, rate-limiting would silently fail during that startup window.

Docker Compose profiles make conditional depends_on tricky — adding the dependency directly would break when the feature-rate-limiting profile is inactive. Worth documenting this startup ordering assumption, or having cloudserver retry/degrade gracefully if the service user doesn't exist yet.

— Claude Code


echo "[setup] rate-limit service user credentials:"
echo "SERVICE_ACCESS_KEY=$SERVICE_ACCESS_KEY"
echo "SERVICE_SECRET_KEY=$SERVICE_SECRET_KEY"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Service credentials (access key and secret key) are echoed to stdout on lines 19-20, which will appear in container logs. This matches the existing pattern in templates/scuba/create-service-user.sh, but it's worth noting that anyone with access to container logs can read these internal service credentials.

— Claude Code

@claude
Copy link
Copy Markdown

claude Bot commented Apr 30, 2026

  • docker-compose.yaml:73-88 — setup-rate-limiting-svc-user runs in the feature-rate-limiting profile but cloudserver (in base) has no depends_on for it. Potential race condition: cloudserver could start with rate limiting enabled before the service user exists in vault, causing rate-limit operations to fail until the setup container completes.
    - create-service-user.sh:18-20 — Service credentials (access key/secret key) are echoed to stdout and will appear in container logs. Matches the existing scuba pattern but worth noting.

    Everything else looks good: JSON templates produce valid output in all feature flag combinations, sprig integration is correct, embed directives cover the new files, Redis is properly included in the feature-rate-limiting profile, and the config structs/defaults are well-structured.

    Review by Claude Code

@tmacro tmacro merged commit 3733d90 into main Apr 30, 2026
3 checks passed
@tmacro tmacro deleted the improvement/WKBCH-23/rate_limit_support branch April 30, 2026 16:59
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