feat: Implement API Key Management and Rotation #69
Conversation
WalkthroughAdds a new APIKeyManager utility that loads provider-scoped API keys from environment variables, groups them per provider, creates a rotation cycle for each provider, and exposes get_key(provider_name) to return the next key. A module-level singleton api_key_manager is initialized at import. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant APIKeyManager
participant Env as "Environment (os.environ)"
Caller->>APIKeyManager: request get_key(provider_name)
alt provider cycle exists
APIKeyManager->>APIKeyManager: acquire provider lock
APIKeyManager->>APIKeyManager: next(key_cycle)
APIKeyManager->>APIKeyManager: release provider lock
APIKeyManager-->>Caller: return key
else no keys loaded for provider
APIKeyManager-->>Caller: return null / None
end
note over APIKeyManager,Env: On init, _load_keys() scans Env,\ngroups keys by provider and builds cycles
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 2
🧹 Nitpick comments (2)
backend/app/security/api_keys.py (2)
6-8: Type annotation mismatch during construction.
self._keystemporarily holdslistvalues during_load_keys()before being converted tocycleobjects. Consider usingUnion[List[str], cycle]or storing raw lists separately.- def __init__(self): - self._keys: Dict[str, cycle] = {} + def __init__(self): + self._raw_keys: Dict[str, list] = {} + self._keys: Dict[str, cycle] = {}Then use
_raw_keysduring loading and convert to_keysat the end.
36-37: Consider adding a method to refresh keys at runtime.The singleton is initialized at import time, so environment variable changes require an application restart. For a key rotation feature, you may want to support runtime refresh.
+ def refresh_keys(self): + """Reloads API keys from environment variables.""" + with self._lock: + self._keys.clear() + self._load_keys() + # Initialize the APIKeyManager api_key_manager = APIKeyManager()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/security/api_keys.py(1 hunks)
🔇 Additional comments (1)
backend/app/security/api_keys.py (1)
1-3: LGTM!Standard library imports are appropriate for the module's functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/app/security/api_keys.py (3)
12-31: Env var parsing now handles earlier edge cases; only minor nits leftThe
_load_keyslogic correctly ignores empty values, handles multiple_API_KEYoccurrences viarsplit, and avoids empty/underscore-only provider names, which resolves the earlier parsing issues. One minor nit: the docstring implies onlyPROVIDER_NAME_API_KEY_1‑style vars, but the code will accept any name containing_API_KEY; if you expect stricter naming, you could tighten the condition or document the broader behavior, and optionally log when entries are skipped to aid debugging misconfigurations.
7-10: Clarify_keystyping by separating raw key collection from rotationFunctionally this works and the per‑provider locks around
next(self._keys[provider_name])give you thread‑safe rotation. The only rough edge is that_keysis annotated asDict[str, cycle]but temporarily holds lists before being converted, which will trip static analyzers.You can keep behavior but make typing cleaner by using a local
raw_keysdict in_load_keys:@@ - def _load_keys(self): + def _load_keys(self): @@ - for key, value in os.environ.items(): + raw_keys: Dict[str, list[str]] = {} + for key, value in os.environ.items(): @@ - if provider_name not in self._keys: - self._keys[provider_name] = [] - self._keys[provider_name].append(value) + if provider_name not in raw_keys: + raw_keys[provider_name] = [] + raw_keys[provider_name].append(value) @@ - # Convert lists to cycles for rotation - for provider_name, key_list in self._keys.items(): - self._keys[provider_name] = cycle(key_list) + # Convert lists to cycles for rotation + for provider_name, key_list in raw_keys.items(): + self._keys[provider_name] = cycle(key_list) self._key_locks[provider_name] = threading.Lock()This keeps
_keysalways “cycle‑typed” while leaving the runtime behavior unchanged.Also applies to: 28-35, 37-45
47-48: Consider lazy or explicit initialization instead of import‑time singletonCreating
api_key_managerat import time tightly couples env evaluation to when this module is first imported, which can make tests or dynamic configuration (e.g., setting env vars in a test before import vs. after) a bit brittle. If you anticipate that need, consider a small factory likeget_api_key_manager()with lazy initialization, or exposing areload()method to re‑scan env vars explicitly.
|
nice, this’ll really simplify key management... merging it now! 🚀 |
Overview: This PR introduces a new module for robust management and rotation of external API keys.
Changes
app/security/api_keys.pyto centralize API key handling..envfiles.get_key(provider_name)for efficient key retrieval.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.