Use singleton pattern in kms client #2158
Conversation
Summary by CodeRabbit
WalkthroughReplaces a module-level KMS client with a thread-safe singleton KmsClient, updates callers to use Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (2)
backend/tests/apps/nest/clients/kms_test.py (1)
41-48: Strengthen singleton test; keep boto3 patched and assert idempotent initAs written, init will run on each instantiation and may hit real boto3 because the patch from the setup fixture ends before the test body. Patch where KmsClient looks up boto3 and assert no new client creations on repeated calls.
@override_settings(AWS_REGION="us-west-2", AWS_KMS_KEY_ID="test_key_id") def test_singleton(self): """Test singleton behavior.""" - instance1 = KmsClient() - instance2 = KmsClient() - assert instance1 is instance2 - assert instance1.client is instance2.client + # Patch where used to avoid real boto3 and verify idempotent __init__ + with patch("apps.nest.clients.kms.boto3.client") as mock_client: + instance1 = KmsClient() + instance2 = KmsClient() + assert instance1 is instance2 + assert instance1.client is instance2.client + # No new boto3 client should be created on repeated instantiation + assert mock_client.call_count == 0backend/apps/common/model_fields.py (1)
26-27: Minor cleanup and note on perf coupling to KmsClient init
- Calling KmsClient() per use is fine so long as KmsClient.init is idempotent (see suggested fix in kms.py) to avoid recreating boto3 clients.
- The trailing conditional at Line 39 is redundant after the earlier emptiness checks.
- return KmsClient().decrypt(bytes(value)) if value else None + return KmsClient().decrypt(bytes(value))Also applies to: 39-39
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
backend/apps/common/model_fields.py(3 hunks)backend/apps/nest/clients/kms.py(1 hunks)backend/tests/apps/nest/clients/kms_test.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T15:34:29.661Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2089
File: backend/apps/common/model_fields.py:18-27
Timestamp: 2025-08-19T15:34:29.661Z
Learning: The KmsEncryptedField in backend/apps/common/model_fields.py is designed to be a KMS-only field that requires IS_AWS_KMS_ENABLED=True for any operation, including storing NULL values. This enforces a strict security model where the field cannot be used without proper KMS configuration.
Applied to files:
backend/apps/common/model_fields.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (1)
backend/apps/common/model_fields.py (1)
6-6: Import switch looks good and aligns with singleton usageMoving to KmsClient is consistent with the new design.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/apps/nest/clients/kms.py (2)
26-31: Prefer AWS default credential provider chain; avoid passing static keys.Rely on instance profiles/IRSA/assumed roles to reduce secret handling in app config and simplify tests.
- self.client = boto3.client( - "kms", - aws_access_key_id=settings.AWS_ACCESS_KEY_ID, - aws_secret_access_key=settings.AWS_SECRET_ACCESS_KEY, - region_name=settings.AWS_REGION, - ) + self.client = boto3.client("kms", region_name=settings.AWS_REGION)
14-21: Fix double-initialization race in init (singleton is created once but initialized twice under concurrency).Two concurrent calls can both pass the
client-presence check and each create a separate boto3 client, racing on the same instance. Guard initialization with a dedicated lock and an_initializedflag.Apply:
class KmsClient: """AWS KMS Client.""" - _lock = Lock() + _lock = Lock() + _init_lock = Lock() @@ - def __new__(cls): + def __new__(cls, *args, **kwargs): """Create a new instance of KmsClient.""" if not hasattr(cls, "instance"): with cls._lock: if not hasattr(cls, "instance"): cls.instance = super().__new__(cls) return cls.instance @@ - def __init__(self): + def __init__(self): """Initialize the KMS client.""" - if getattr(self, "client", None) is not None: - return - self.client = boto3.client( - "kms", - aws_access_key_id=settings.AWS_ACCESS_KEY_ID, - aws_secret_access_key=settings.AWS_SECRET_ACCESS_KEY, - region_name=settings.AWS_REGION, - ) + if getattr(self, "_initialized", False): + return + with self.__class__._init_lock: + if getattr(self, "_initialized", False): + return + self.client = boto3.client( + "kms", + aws_access_key_id=settings.AWS_ACCESS_KEY_ID, + aws_secret_access_key=settings.AWS_SECRET_ACCESS_KEY, + region_name=settings.AWS_REGION, + ) + self._initialized = TrueAlso applies to: 24-31
🧹 Nitpick comments (6)
backend/apps/nest/clients/kms.py (3)
14-20: Harden new signature and make the singleton attribute explicit.
- Accept
*args, **kwargsfor forward-compat.- Declare
instance = Noneat class-level to satisfy linters/static analysis and improve readability.class KmsClient: """AWS KMS Client.""" - _lock = Lock() + _lock = Lock() + instance = None # explicit singleton slot - def __new__(cls): + def __new__(cls, *args, **kwargs): """Create a new instance of KmsClient."""
26-31: Add client timeouts and bounded retries via botocore Config.Prevents hung requests and adds resilient backoff on transient AWS errors.
+from botocore.config import Config @@ - self.client = boto3.client("kms", region_name=settings.AWS_REGION) + self.client = boto3.client( + "kms", + region_name=settings.AWS_REGION, + config=Config( + retries={"max_attempts": 3, "mode": "standard"}, + read_timeout=5, + connect_timeout=2, + ), + )
22-31: Offer a safe, test-only reset hook instead of mutating class attrs directly in tests.Keeps tests deterministic and thread-safe.
Add:
@classmethod def _reset_for_tests(cls): with cls._lock: if hasattr(cls, "instance"): del cls.instanceWant me to push a commit updating tests to use
KmsClient._reset_for_tests()?backend/tests/apps/nest/clients/kms_test.py (3)
17-21: Patch where the symbol is used.Patch
apps.nest.clients.kms.boto3.clientinstead ofboto3.clientto avoid accidental leakage if boto3 was imported elsewhere before the patch.-with patch("boto3.client") as mock_boto3_client: +with patch("apps.nest.clients.kms.boto3.client") as mock_boto3_client:
21-23: Use the provided reset hook rather than deleting class attributes.Improves clarity and avoids attribute errors if the singleton slot changes.
- if hasattr(KmsClient, "instance"): - del KmsClient.instance + KmsClient._reset_for_tests()
45-50: Strengthen the singleton test by asserting a single boto3 client creation.Ensures
__init__guarding works as expected.instance1 = KmsClient() instance2 = KmsClient() assert instance1 is instance2 assert instance1.client is instance2.client + assert self.mock_boto3_client.call_count == 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
backend/apps/nest/clients/kms.py(1 hunks)backend/tests/apps/nest/clients/kms_test.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
ahmedxgouda
changed the title
arkid15r
merged commit 210e207
into
OWASP:feature/nestbot-calendar-events
* Use singleton * Add thread locking
* Use singleton * Add thread locking
* Use singleton * Add thread locking
* Use singleton * Add thread locking
* Use singleton * Add thread locking
* Use singleton * Add thread locking
* Use singleton * Add thread locking
* Use singleton * Add thread locking
2 participants
Proposed change
Add the PR description here.
Checklist
make check-testlocally; all checks and tests passed.