Improve Redis Cache key prefix for Deployment Isolation#2174
Improve Redis Cache key prefix for Deployment Isolation#2174KaveeshaPiumini merged 1 commit intoasgardeo:mainfrom
Conversation
KaveeshaPiumini
requested review from
ThaminduDilshan and
rajithacharith
as code owners
📝 WalkthroughWalkthroughA new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/internal/system/cache/manager.go`:
- Around line 209-221: The new behavior in buildRedisKeyPrefix makes
server.identifier influence Redis cache keys for per-deployment isolation; add
documentation entries under docs/content explaining this: update the
cache/config reference to describe the server.identifier configuration option,
show the effective Redis key shape (prefix[:deploymentID] — basePrefix when
identifier empty, deploymentID when basePrefix empty, otherwise
basePrefix:deploymentID), and add an operator guide note about migration impact
(changing server.identifier creates a new key namespace and existing cached
entries won’t be visible). Ensure both docs mention where to configure
server.identifier and include examples and recommended migration/rollout steps.
🪄 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: Pro
Run ID: d745a9da-13b1-4935-bf02-7f99580832f1
📒 Files selected for processing (2)
backend/internal/system/cache/manager.gobackend/internal/system/cache/manager_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2174 +/- ##
=======================================
Coverage 89.84% 89.85%
=======================================
Files 898 898
Lines 58951 58958 +7
=======================================
+ Hits 52967 52975 +8
+ Misses 4418 4417 -1
Partials 1566 1566
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| // buildRedisKeyPrefix composes the Redis key prefix with deployment ID for per-deployment isolation. | ||
| func buildRedisKeyPrefix(basePrefix string) string { |
There was a problem hiding this comment.
Why we need two properties here? Can't we only use the deploymentId as the key prefix?
There was a problem hiding this comment.
They serve two different purposes:
- basePrefix: Namespaces keys by application or logical group. This is useful when multiple applications share the same Redis instance (e.g., thunder:* vs other-app:*).
- deploymentID: Namespaces keys by deployment instance. This avoids collisions when multiple Thunder deployments share the same Redis (e.g., staging vs prod).
Regarding using only deploymentID: that would work only if Redis is exclusively used by Thunder deployments with properly configured IDs. However, basePrefix still matters when:
- Redis is shared with non-Thunder services
- Operators prefer a clear, human-readable namespace independent of deployment IDs
There was a problem hiding this comment.
And also, the ACLs can be set for namespaces as well. So following patterns can be used when using Redis
- Separate redis instance for Thunder -- No need to specify keyPrefix for thunder
- Separate Redis DB index for Thunder -- No need to specify keyPrefix for thunder as the Data logically separates in index.
- Use the same redis instance same db -- Need to use key prefix and namespace the keys
ACLs needs be set to the namespace for data isolation.
Pattern will be<keyPrefix>:<deploymentId>:<Key>
There was a problem hiding this comment.
@KaveeshaPiumini We need to add the above details to docs as well
3 participants
Purpose
This pull request enhances the cache key management for Redis by introducing deployment-specific key prefixes, ensuring better isolation between deployments. It also adds comprehensive unit tests to verify the new key prefix logic.
Redis key prefix isolation:
buildRedisKeyPrefixfunction inmanager.goto compose Redis key prefixes with the deployment ID, enabling per-deployment cache isolation.newCacheto use the newbuildRedisKeyPrefixfunction when setting the Redis key prefix.Testing improvements:
TestBuildRedisKeyPrefixunit test inmanager_test.goto verify the behavior of the new key prefix logic under different scenarios, ensuring correct key composition based on base prefix and deployment ID.Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
New Features
Tests