feat: secrets caching support #32
Conversation
WalkthroughThis pull request updates the Infisical SDK and its documentation. The README now details the optional parameters for the InfisicalSDKClient and documents a new “kms” category. The client has been refactored to include a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant InfSDK as InfisicalSDKClient
participant V3Secrets as V3RawSecrets
participant Cache as SecretsCache
participant API
Client->>InfSDK: list_secrets()
InfSDK->>V3Secrets: list_secrets()
V3Secrets->>Cache: get(cache_key)
alt Cache Hit
Cache-->>V3Secrets: cached response
else Cache Miss
V3Secrets->>API: API request for secrets
API-->>V3Secrets: secrets response
V3Secrets->>Cache: set(cache_key, data)
end
V3Secrets-->>InfSDK: return secrets list
InfSDK-->>Client: secrets list delivered
sequenceDiagram
participant Client
participant Auth as Auth
participant UniAuth as UniversalAuth
participant API
Client->>Auth: login(client_id, client_secret)
Auth->>UniAuth: login(client_id, client_secret)
UniAuth->>API: POST /api/v1/auth/universal-auth/login
API-->>UniAuth: access_token response
UniAuth->>Auth: return access_token
Auth->>Client: execute setToken(access_token)
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (27)
infisical_sdk/resources/auth_methods/__init__.py (1)
1-2: Consider addressing unused imports warning.Both imports are flagged by the static analyzer as unused. Since this is an
__init__.pyfile, these imports are likely intended to expose these classes at the package level. Consider adding these to__all__to make the exports explicit.from .aws_auth import AWSAuth from .universal_auth import UniversalAuth + +__all__ = ["AWSAuth", "UniversalAuth"]🧰 Tools
🪛 Ruff (0.8.2)
1-1:
.aws_auth.AWSAuthimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
2-2:
.universal_auth.UniversalAuthimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
infisical_sdk/resources/__init__.py (1)
1-3: Consider addressing unused imports warning.All three imports are flagged by the static analyzer as unused. Since this is an
__init__.pyfile, these imports are likely intended to expose these classes at the package level. Consider adding these to__all__to make the exports explicit.from .secrets import V3RawSecrets from .kms import KMS from .auth import Auth + +__all__ = ["V3RawSecrets", "KMS", "Auth"]🧰 Tools
🪛 Ruff (0.8.2)
1-1:
.secrets.V3RawSecretsimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
2-2:
.kms.KMSimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
3-3:
.auth.Authimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
infisical_sdk/resources/auth.py (1)
6-10: Consider adding class and constructor docstrings.
Adding docstrings (e.g., explaining whatrequestsandsetTokendo) will improve clarity and maintainability for future contributors.infisical_sdk/resources/auth_methods/universal_auth.py (1)
10-20: Align docstring return type with actual return type.
The docstring mentions returning a generic dictionary, yet the function actually returns aMachineIdentityLoginResponse. Update the docstring to avoid confusion.infisical_sdk/resources/auth_methods/aws_auth.py (2)
25-34: Clarify return type in docstring.
The docstring says it returns a dictionary, but the method signature returns aMachineIdentityLoginResponse. Consider updating the docstring to match the declared return type.
81-82: Preserve original traceback when re-raising.
Use theraise ... from esyntax so that the original traceback context is retained, in accordance with static analysis recommendations.Apply this diff to line 82:
- raise RuntimeError(f"AWS IAM Auth Login failed: {str(e)}") + raise RuntimeError(f"AWS IAM Auth Login failed: {str(e)}") from e🧰 Tools
🪛 Ruff (0.8.2)
82-82: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
infisical_sdk/resources/kms.py (1)
128-131: Correct the docstring forencrypt_datato avoid confusion.Currently, the docstring references "decrypt the ciphertext" instead of "encrypt the plaintext" and uses
:type plaintext:instead of the actual parameter namebase64EncodedPlaintext. Consider updating it as follows to maintain clarity:- :param key_id: The ID of the key to decrypt the ciphertext with + :param key_id: The ID of the key to encrypt the plaintext with - :param base64EncodedPlaintext: The base64 encoded plaintext to encrypt - :type plaintext: str + :param base64EncodedPlaintext: The base64 encoded plaintext to encrypt + :type base64EncodedPlaintext: strinfisical_sdk/resources/secrets.py (1)
98-129: Consider invalidating or refreshing the cache after secret creation.When new secrets are created, previously cached listings or lookups may become stale. You might want to unset or refresh your cached entries in
create_secret_by_name(and similarly inupdate_secret_by_nameanddelete_secret_by_name) to ensure the cache remains consistent.infisical_sdk/util/__init__.py (1)
1-1: Export or remove the unused import.
SecretsCacheis imported but does not appear to be used in this file. You can either remove it or export it explicitly using__all__to clarify the intended usage:-from .secrets_cache import SecretsCache +from .secrets_cache import SecretsCache +__all__ = ["SecretsCache"]🧰 Tools
🪛 Ruff (0.8.2)
1-1:
.secrets_cache.SecretsCacheimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
README.md (4)
48-51: Typo in parameter description.The text has a grammatical error in the description of parameters.
-The `InfisicalSDKClient` takes the following parameters, which are used a global configuration. +The `InfisicalSDKClient` takes the following parameters, which are used as a global configuration.🧰 Tools
🪛 LanguageTool
[uncategorized] ~50-~50: Possible missing preposition found.
Context: ...he following parameters, which are used a global configuration. - host (`str...(AI_HYDRA_LEO_MISSING_IN)
53-53: Grammatical error: "a authentication" should be "an authentication".The article "a" should be "an" when preceding words beginning with vowel sounds.
-Specify a authentication token to use for all requests. +Specify an authentication token to use for all requests.🧰 Tools
🪛 LanguageTool
[misspelling] ~53-~53: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ... token (str, Optional): Specify a authentication token to use for all req...(EN_A_VS_AN)
54-54: Missing comma after "By default".Add a comma after the introductory phrase for better readability.
-By default secrets are cached for 5 minutes. +By default, secrets are cached for 5 minutes.🧰 Tools
🪛 LanguageTool
[uncategorized] ~54-~54: Did you mean: “By default,”?
Context: ... improves speed for the secret methods. By default secrets are cached for 5 minutes. You c...(BY_DEFAULT_COMMA)
361-361: Typo in "Decrypte Data" heading.There's a typo in the heading for the decrypt data section.
-#### Decrypte Data with KMS Key +#### Decrypt Data with KMS Keysink/cache_expire_test.py (2)
28-28: Unused loop variable.The variable
iis not used within the loop body. Consider renaming it to_to indicate intentional non-usage.-for i in range(10): +for _ in range(10):🧰 Tools
🪛 Ruff (0.8.2)
28-28: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
1-55: Missing docstring and error handling.This test file lacks a docstring explaining its purpose and any error handling for API requests. Consider adding a docstring and try-except blocks around API calls.
Add a docstring at the beginning of the file:
""" Test script to verify caching expiration functionality in the Infisical SDK. This script tests the behavior of the cache by timing API calls before and after cache expiration. """And implement error handling around API calls:
try: single_secret_cached = cache_enabled_client.secrets.get_secret_by_name( secret_name="TEST", project_id=SECRETS_PROJECT_ID, environment_slug=SECRETS_ENVIRONMENT_SLUG, secret_path="/", expand_secret_references=True, include_imports=True) except Exception as e: print(f"Error fetching secret: {e}") sys.exit(1)With the import:
import sys🧰 Tools
🪛 Ruff (0.8.2)
28-28: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
sink/cache_test.py (3)
21-21: Unused loop variable.The variable
iis not used within the loop body. Consider renaming it to_to indicate intentional non-usage.-for i in range(100): +for _ in range(100):🧰 Tools
🪛 Ruff (0.8.2)
21-21: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
37-37: Unused loop variable.The variable
iis not used within the loop body. Consider renaming it to_to indicate intentional non-usage.-for i in range(100): +for _ in range(100):🧰 Tools
🪛 Ruff (0.8.2)
37-37: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
1-75: Missing docstring and test structure.The test file lacks a docstring explaining its purpose and a structured testing approach. Consider adding a docstring and improving the test organization.
Add a docstring at the beginning of the file:
""" Benchmark script comparing performance between cache-enabled and cache-disabled Infisical SDK clients. This script measures the time for both list_secrets and get_secret_by_name operations. """Consider reorganizing with functions to make the test more structured:
def benchmark_list_secrets(client, iterations=100, label=""): """Benchmark the list_secrets method""" time_start = time.time() for _ in range(iterations): client.secrets.list_secrets( project_id=SECRETS_PROJECT_ID, environment_slug=SECRETS_ENVIRONMENT_SLUG, secret_path="/", expand_secret_references=True, include_imports=True ) time_end = time.time() elapsed = time_end - time_start print(f"[{label}] Time taken: {elapsed} seconds") return elapsed # Then call the function instead of duplicated code benchmark_list_secrets(cache_disabled_client, label="CACHE DISABLED") benchmark_list_secrets(cache_enabled_client, label="CACHE ENABLED")🧰 Tools
🪛 Ruff (0.8.2)
21-21: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
37-37: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
infisical_sdk/util/secrets_cache.py (4)
3-3: Unused import.The
BaseSecretimport is not used in this file.-from infisical_sdk.api_types import BaseSecret🧰 Tools
🪛 Ruff (0.8.2)
3-3:
infisical_sdk.api_types.BaseSecretimported but unusedRemove unused import:
infisical_sdk.api_types.BaseSecret(F401)
32-32: Debug print statement.Debug print statements should be removed or replaced with proper logging.
- print(f"Cache key: {operation_name}:{json_str}") + # Use logging instead of print for better control + import logging + logging.debug(f"Cache key: {operation_name}:{json_str}")
44-51: Debug print statements should use logging.Replace print statements with proper logging for better control in production environments.
- print(f"Cache hit: {cache_key}") + logging.debug(f"Cache hit: {cache_key}") return pickle.loads(serialized_value) else: - print(f"Cache miss (expired): {cache_key}") + logging.debug(f"Cache miss (expired): {cache_key}") del self.cache[cache_key] return None else: - print(f"Cache miss (not in cache): {cache_key}") + logging.debug(f"Cache miss (not in cache): {cache_key}")
1-93: Missing proper docstrings for class and methods.The class and methods lack proper docstrings with parameters and return value descriptions.
Add a proper class docstring:
class SecretsCache: """ A thread-safe cache for secrets with automatic expiration. This class provides a caching mechanism with configurable TTL (time-to-live) for secrets, helping to reduce API calls and improve performance. Attributes: enabled (bool): Whether caching is enabled. ttl (int): Time to live for cache entries in seconds. cleanup_interval (int): Interval in seconds between cache cleanup operations. """And improve method docstrings, for example:
def compute_cache_key(self, operation_name: str, **kwargs) -> str: """ Compute a unique cache key based on operation name and parameters. Args: operation_name (str): The name of the operation being cached. **kwargs: Additional parameters that determine the uniqueness of the cache key. Returns: str: A hexadecimal digest representing the unique cache key. """🧰 Tools
🪛 Ruff (0.8.2)
3-3:
infisical_sdk.api_types.BaseSecretimported but unusedRemove unused import:
infisical_sdk.api_types.BaseSecret(F401)
sink/example.py (4)
7-7: Consider using a secure protocol.
Currently, the SDK client is initialized withhost="http://localhost:8080". If this is intended for production or sensitive data, consider using HTTPS to protect against data interception or tampering.
78-80: Avoid hard-coded secret count check.
The check for exactly one secret may fail if the environment already has multiple secrets. Consider making the test more robust (e.g., searching for the newly created secret by name or verifying the difference before and after).
107-107: Typographical variable name.
It appears there’s a typo in the variableplantext. Rename it toplaintextfor clarity.- plantext = "Hello, world!" + plaintext = "Hello, world!"
114-121: Validate before printing sensitive content.
Even though this is a demo, printing encrypted and decrypted values can risk exposing sensitive data in logs. In real scenarios, either skip printing or sanitize the output to avoid security leaks.infisical_sdk/client.py (1)
37-38: Docstring mismatch for get_token.
The docstring says “Set the access token...” but the function name isget_token. This might confuse users. Reword it to reflect that it “Retrieves the current access token.”- Set the access token for future requests. + Retrieve the access token for future requests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
README.md(1 hunks)example.py(0 hunks)infisical_sdk/__init__.py(1 hunks)infisical_sdk/client.py(1 hunks)infisical_sdk/resources/__init__.py(1 hunks)infisical_sdk/resources/auth.py(1 hunks)infisical_sdk/resources/auth_methods/__init__.py(1 hunks)infisical_sdk/resources/auth_methods/aws_auth.py(1 hunks)infisical_sdk/resources/auth_methods/universal_auth.py(1 hunks)infisical_sdk/resources/kms.py(1 hunks)infisical_sdk/resources/secrets.py(1 hunks)infisical_sdk/util/__init__.py(1 hunks)infisical_sdk/util/secrets_cache.py(1 hunks)sink/README.md(1 hunks)sink/cache_expire_test.py(1 hunks)sink/cache_test.py(1 hunks)sink/example.py(1 hunks)
💤 Files with no reviewable changes (1)
- example.py
🧰 Additional context used
🧬 Code Definitions (11)
infisical_sdk/util/__init__.py (1)
infisical_sdk/util/secrets_cache.py (1)
SecretsCache(10-92)
infisical_sdk/resources/__init__.py (3)
infisical_sdk/resources/secrets.py (1)
V3RawSecrets(10-185)infisical_sdk/resources/kms.py (1)
KMS(8-177)infisical_sdk/resources/auth.py (1)
Auth(6-10)
infisical_sdk/__init__.py (1)
infisical_sdk/api_types.py (4)
SingleSecretResponse(111-119)ListSecretsResponse(96-107)BaseSecret(63-83)SymmetricEncryption(131-133)
infisical_sdk/resources/auth_methods/__init__.py (2)
infisical_sdk/resources/auth_methods/aws_auth.py (1)
AWSAuth(20-134)infisical_sdk/resources/auth_methods/universal_auth.py (1)
UniversalAuth(5-35)
sink/cache_test.py (3)
infisical_sdk/client.py (1)
InfisicalSDKClient(9-39)infisical_sdk/resources/auth_methods/universal_auth.py (1)
login(10-35)infisical_sdk/resources/secrets.py (2)
list_secrets(15-56)get_secret_by_name(58-96)
infisical_sdk/resources/auth.py (2)
infisical_sdk/resources/auth_methods/aws_auth.py (1)
AWSAuth(20-134)infisical_sdk/resources/auth_methods/universal_auth.py (1)
UniversalAuth(5-35)
sink/example.py (4)
infisical_sdk/client.py (1)
InfisicalSDKClient(9-39)infisical_sdk/resources/auth_methods/universal_auth.py (1)
login(10-35)infisical_sdk/resources/secrets.py (3)
create_secret_by_name(98-128)update_secret_by_name(130-163)delete_secret_by_name(165-185)infisical_sdk/resources/kms.py (7)
create_key(66-86)encrypt_data(121-148)decrypt_data(150-177)get_key_by_id(38-47)get_key_by_name(49-64)list_keys(12-36)delete_key(109-119)
infisical_sdk/resources/auth_methods/universal_auth.py (1)
infisical_sdk/resources/auth_methods/aws_auth.py (1)
login(25-73)
infisical_sdk/client.py (5)
infisical_sdk/resources/auth.py (1)
Auth(6-10)infisical_sdk/resources/secrets.py (1)
V3RawSecrets(10-185)infisical_sdk/resources/kms.py (1)
KMS(8-177)infisical_sdk/util/secrets_cache.py (1)
SecretsCache(10-92)infisical_sdk/infisical_requests.py (2)
InfisicalRequests(53-192)set_token(71-73)
infisical_sdk/resources/kms.py (1)
infisical_sdk/util/secrets_cache.py (1)
get(36-52)
sink/cache_expire_test.py (3)
infisical_sdk/client.py (1)
InfisicalSDKClient(9-39)infisical_sdk/resources/auth_methods/universal_auth.py (1)
login(10-35)infisical_sdk/resources/secrets.py (1)
get_secret_by_name(58-96)
🪛 Ruff (0.8.2)
infisical_sdk/util/__init__.py
1-1: .secrets_cache.SecretsCache imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
infisical_sdk/resources/__init__.py
1-1: .secrets.V3RawSecrets imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
2-2: .kms.KMS imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
3-3: .auth.Auth imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
infisical_sdk/resources/auth_methods/__init__.py
1-1: .aws_auth.AWSAuth imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
2-2: .universal_auth.UniversalAuth imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
sink/cache_test.py
21-21: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
37-37: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
infisical_sdk/util/secrets_cache.py
3-3: infisical_sdk.api_types.BaseSecret imported but unused
Remove unused import: infisical_sdk.api_types.BaseSecret
(F401)
sink/cache_expire_test.py
28-28: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
infisical_sdk/resources/secrets.py
24-24: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
infisical_sdk/resources/auth_methods/aws_auth.py
82-82: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🪛 LanguageTool
README.md
[uncategorized] ~50-~50: Possible missing preposition found.
Context: ...he following parameters, which are used a global configuration. - host (`str...
(AI_HYDRA_LEO_MISSING_IN)
[misspelling] ~53-~53: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ... token (str, Optional): Specify a authentication token to use for all req...
(EN_A_VS_AN)
[uncategorized] ~54-~54: Did you mean: “By default,”?
Context: ... improves speed for the secret methods. By default secrets are cached for 5 minutes. You c...
(BY_DEFAULT_COMMA)
[uncategorized] ~68-~68: Loose punctuation mark.
Context: ...lowing high-level categories: 1. auth: Handles authentication methods. 2. `sec...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~69-~69: Loose punctuation mark.
Context: ...les authentication methods. 2. secrets: Manages CRUD operations for secrets. 3....
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~70-~70: Loose punctuation mark.
Context: ...es CRUD operations for secrets. 3. kms: Perform cryptographic operations with I...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (6)
sink/README.md (1)
1-3: Clear and helpful developer instruction.This developer note provides clear instructions on how to properly set up the environment to run the sink files. Using
pip install -e .for development mode is the correct approach as it allows changes to the SDK to be immediately reflected without reinstallation.infisical_sdk/__init__.py (1)
3-3: Good addition of SymmetricEncryption to public API.Adding
SymmetricEncryptionto the imports makes it accessible at the package level, which is appropriate given the new KMS functionality introduced in this PR. This will allow users to easily specify encryption algorithms when using the KMS features.infisical_sdk/resources/auth.py (1)
1-3: Imports look good!
All dependencies appear accurately referenced, and there are no obvious issues with missing or extraneous imports.infisical_sdk/client.py (3)
10-17: Constructor docstring alignment.
The docstring properly documents the newcache_ttlparameter, which is beneficial for controlling how long secrets remain cached. The explanation is concise and clear.
23-24: Good approach to caching.
UsingSecretsCache(cache_ttl)provides a manageable TTL-based cache. Ensure that the background cleanup thread inSecretsCacheis properly terminated if the application shuts down.
25-26: Centralized architecture for resources.
InstantiatingAuth,V3RawSecrets, andKMSwith shared API and cache references is a clean design, promoting clarity and reusability across the SDK’s modules.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
infisical_sdk/util/secrets_cache.py (4)
3-3: Remove unused import.The
BaseSecretimport frominfisical_sdk.api_typesis not used anywhere in this file.-from infisical_sdk.api_types import BaseSecret🧰 Tools
🪛 Ruff (0.8.2)
3-3:
infisical_sdk.api_types.BaseSecretimported but unusedRemove unused import:
infisical_sdk.api_types.BaseSecret(F401)
77-82: Consider implementing a more precise cleanup interval.The current implementation might lead to drift in cleanup timing if the cleanup operation takes a significant amount of time.
def _cleanup_worker(self) -> None: """Background worker that periodically cleans up expired items.""" + next_cleanup = time.time() + self.cleanup_interval while not self.stop_cleanup_thread: - time.sleep(self.cleanup_interval) + now = time.time() + if now < next_cleanup: + time.sleep(min(next_cleanup - now, 1.0)) # Sleep at most 1 second to check stop flag more frequently + continue + next_cleanup = time.time() + self.cleanup_interval self._cleanup_expired_items()
83-88: Consider adding an explicit close() method.The current implementation relies on
__del__for cleanup, but Python doesn't guarantee when or if__del__will be called.Add an explicit
close()method that users can call to ensure resources are properly cleaned up:+ def close(self) -> None: + """Explicitly close the cache and clean up resources.""" + self.stop_cleanup_thread = True + if self.enabled and self.cleanup_thread.is_alive(): + self.cleanup_thread.join(timeout=1.0) + def __del__(self) -> None: """Ensure thread is properly stopped when the object is garbage collected.""" - self.stop_cleanup_thread = True - if self.enabled and self.cleanup_thread.is_alive(): - self.cleanup_thread.join(timeout=1.0) + self.close()
1-88: Consider implementing a maximum cache size.The current implementation has no limit on how many items can be stored in the cache, which could potentially lead to memory issues.
Implement a maximum size limit for the cache and an eviction policy (e.g., LRU) when the limit is reached. Here's a simplified example:
def __init__(self, ttl_seconds: int = 300, max_size: int = 1000) -> None: # ... existing code ... self.max_size = max_size if max_size and max_size > 0 else None # ... existing code ... def set(self, cache_key: str, value: Any) -> None: if not self.enabled: return with self.lock: # Evict items if we're at capacity if self.max_size and len(self.cache) >= self.max_size: # Simple LRU implementation - remove oldest item oldest_key = min(self.cache.items(), key=lambda x: x[1][1])[0] del self.cache[oldest_key] serialized_value = pickle.dumps(value) self.cache[cache_key] = (serialized_value, time.time())🧰 Tools
🪛 Ruff (0.8.2)
3-3:
infisical_sdk.api_types.BaseSecretimported but unusedRemove unused import:
infisical_sdk.api_types.BaseSecret(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infisical_sdk/util/secrets_cache.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
infisical_sdk/util/secrets_cache.py
3-3: infisical_sdk.api_types.BaseSecret imported but unused
Remove unused import: infisical_sdk.api_types.BaseSecret
(F401)
🔇 Additional comments (6)
infisical_sdk/util/secrets_cache.py (6)
11-27: LGTM: Well-structured cache initialization with proper thread safety.The constructor correctly handles the TTL parameter, initializes threading components, and sets up a daemon thread for cleanup. The use of RLock is appropriate for protecting the cache in a multi-threaded environment.
28-33: LGTM: Robust cache key generation.The cache key computation uses a deterministic approach by sorting keyword arguments and applying SHA-256 hashing, which effectively prevents collisions.
34-48: LGTM: Thread-safe cache retrieval with expiration handling.The
getmethod correctly handles cache misses and expired entries, with proper thread synchronization via the lock.
50-57: Consider security implications of using pickle for serialization.While pickle is convenient for serializing Python objects, it can present security risks if the cache ever contains data derived from untrusted sources.
Consider using a more secure serialization method if the cached data might include user input or data from external APIs. Alternatively, document that this cache should only be used with trusted data.
58-63: Missing key error handling in unset method.The
unsetmethod doesn't handle the case where the key doesn't exist in the cache, which could lead to a KeyError.def unset(self, cache_key: str) -> None: if not self.enabled: return with self.lock: - del self.cache[cache_key] + self.cache.pop(cache_key, None) # Use pop with default to avoid KeyError
66-76: LGTM: Efficient expired item cleanup.The cleanup method properly identifies and removes expired cache entries while holding the lock for thread safety.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
infisical_sdk/util/secrets_cache.py (6)
3-3: Remove unused import.The
BaseSecretimport frominfisical_sdk.api_typesis not used in this file.-from infisical_sdk.api_types import BaseSecret🧰 Tools
🪛 Ruff (0.8.2)
3-3:
infisical_sdk.api_types.BaseSecretimported but unusedRemove unused import:
infisical_sdk.api_types.BaseSecret(F401)
8-8: Consider security implications of using pickle for serialization.Using
picklefor serialization can be a security risk if the cache could ever contain untrusted data. Consider using a safer serialization method like JSON with custom encoders/decoders for complex types, or at least document that this cache should only be used with trusted data.
10-27: Consider adding a cache size limit.The cache currently has no limit on the number of entries it can store, which could potentially lead to memory issues if a large number of items are cached. Consider adding a maximum size parameter and implementing a strategy (like LRU) to remove entries when the limit is reached.
77-82: Improve cleanup worker implementation.The current implementation doesn't account for the time spent in
_cleanup_expired_items()when calculating the next sleep interval. This could lead to the cleanup running less frequently than intended if the cleanup operation takes significant time.def _cleanup_worker(self) -> None: """Background worker that periodically cleans up expired items.""" while not self.stop_cleanup_thread: - time.sleep(self.cleanup_interval) - self._cleanup_expired_items() + start_time = time.time() + self._cleanup_expired_items() + elapsed = time.time() - start_time + sleep_time = max(0.1, self.cleanup_interval - elapsed) + time.sleep(sleep_time)
83-87: Improve cleanup thread termination.The
__del__method isn't guaranteed to be called in all Python implementations, which could lead to the cleanup thread not being properly terminated. Consider implementing a more reliable cleanup mechanism, such as a context manager pattern with__enter__and__exit__methods.+ def __enter__(self): + return self + + def __exit__(self, exc_type, exc_val, exc_tb): + self.stop_cleanup_thread = True + if self.enabled and self.cleanup_thread.is_alive(): + self.cleanup_thread.join(timeout=1.0)This would allow users to use the cache with a
withstatement, ensuring proper cleanup.
10-89: Add a method to clear the entire cache.The class doesn't provide a method to clear the entire cache, which might be useful in certain scenarios.
+ def clear(self) -> None: + """Clear all items from the cache.""" + if not self.enabled: + return + + with self.lock: + self.cache.clear()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infisical_sdk/util/secrets_cache.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
infisical_sdk/util/secrets_cache.py
3-3: infisical_sdk.api_types.BaseSecret imported but unused
Remove unused import: infisical_sdk.api_types.BaseSecret
(F401)
🔇 Additional comments (1)
infisical_sdk/util/secrets_cache.py (1)
62-63: LGTM! Fixed key error handling.I notice that you've already addressed the previous review comment about key error handling in the
unsetmethod by usingpopwith a default value.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (2)
48-55: Enhance SDK Client Parameters DocumentationThe new "InfisicalSDKClient Parameters" section is clear and provides useful details on the optional parameters. However, please consider the following minor adjustments for clarity and grammar:
- On line 53, change “Specify a authentication token…” to “Specify an authentication token…”.
- On line 54, add a comma after “By default” so it reads “By default, secrets are cached for 5 minutes.”
These adjustments will improve readability and grammatical correctness.🧰 Tools
🪛 LanguageTool
[misspelling] ~53-~53: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ... token (str, Optional): Specify a authentication token to use for all req...(EN_A_VS_AN)
[uncategorized] ~54-~54: Did you mean: “By default,”?
Context: ... improves speed for the secret methods. By default secrets are cached for 5 minutes. You c...(BY_DEFAULT_COMMA)
[uncategorized] ~54-~54: A determiner appears to be missing. Consider inserting it.
Context: ...300equates to 5 minutes caching TTL. Default is300seconds. ```python client = I...(AI_EN_LECTOR_MISSING_DETERMINER)
68-70: Introduce KMS Category ConsistentlyThe addition of the
kmscategory in the Core Methods section effectively highlights the new cryptographic capabilities. For consistency with the other categories, double-check that its description matches the tone and style used elsewhere in the documentation.🧰 Tools
🪛 LanguageTool
[uncategorized] ~68-~68: Loose punctuation mark.
Context: ...lowing high-level categories: 1.auth: Handles authentication methods. 2. `sec...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~69-~69: Loose punctuation mark.
Context: ...les authentication methods. 2.secrets: Manages CRUD operations for secrets. 3....(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~70-~70: Loose punctuation mark.
Context: ...es CRUD operations for secrets. 3.kms: Perform cryptographic operations with I...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[misspelling] ~53-~53: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ... token (str, Optional): Specify a authentication token to use for all req...
(EN_A_VS_AN)
[uncategorized] ~54-~54: Did you mean: “By default,”?
Context: ... improves speed for the secret methods. By default secrets are cached for 5 minutes. You c...
(BY_DEFAULT_COMMA)
[uncategorized] ~54-~54: A determiner appears to be missing. Consider inserting it.
Context: ...300 equates to 5 minutes caching TTL. Default is 300 seconds. ```python client = I...
(AI_EN_LECTOR_MISSING_DETERMINER)
[uncategorized] ~68-~68: Loose punctuation mark.
Context: ...lowing high-level categories: 1. auth: Handles authentication methods. 2. `sec...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~69-~69: Loose punctuation mark.
Context: ...les authentication methods. 2. secrets: Manages CRUD operations for secrets. 3....
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~70-~70: Loose punctuation mark.
Context: ...es CRUD operations for secrets. 3. kms: Perform cryptographic operations with I...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (1)
README.md (1)
56-62: Validate SDK Client Initialization ExampleThe Python code snippet for initializing the
InfisicalSDKClientis well formatted and clear. One suggestion from previous reviews was to ensure that the parameter list includes the necessary commas. In this snippet, the commas appear correctly.
varonix0
requested review from
scott-ray-wilson
and removed request for
sheensantoscapadngan
scott-ray-wilson
left a comment
scott-ray-wilson
left a comment
There was a problem hiding this comment.
Few typos/suggestions/questions
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (15)
infisical_sdk/client.py (1)
10-17: Clarifycache_ttlusage in docstring.Your current docstring states that setting
cache_ttltoNonedisables caching, which is consistent withSecretsCache. Consider also mentioning that negative or zero values will effectively disable caching to reflect howSecretsCacheimplements its behavior.setup.cfg (1)
3-3: Revisit ignoring thesinkdirectory in linting.Excluding
sinkfromflake8can speed development but risks missing linter issues in tests. Consider selectively re-including it or using separate lint rules to maintain consistent code quality across test files.README.md (1)
48-55: Update documentation for better clarity.The documentation on parameters is well-structured, but there are a few clarity improvements:
- Line 51 has a grammatical error: "used as a global configuration" should be "used as global configuration" or "used as a global configuration setting"
- Line 54 mentions a default cache time of 1 minute (60 seconds), but line 60 shows an example using 300 seconds. Consider clarifying that 300 is a custom value, not the default.
-The `InfisicalSDKClient` takes the following parameters, which are used as a global configuration for the lifetime of the SDK instance. +The `InfisicalSDKClient` takes the following parameters, which are used as global configuration settings for the lifetime of the SDK instance. -The SDK has built-in client-side caching for secrets, greatly improving response times. By default, secrets are cached for 1 minute (60 seconds). You can disable caching by setting `cache_ttl` to `None`, or adjust the duration in seconds as needed. +The SDK has built-in client-side caching for secrets, greatly improving response times. By default, secrets are cached for 1 minute (60 seconds). You can disable caching by setting `cache_ttl` to `None`, or adjust the duration in seconds as needed (example below uses 5 minutes).sink/cache_deletion_test.py (4)
8-18: Consider refactoring the duplicated environment loading code.This function
loadEnvVarsFromFileIntoEnv()appears in multiple test files with identical implementation. Consider moving it to a shared utility module to reduce duplication.+# In a new file: sink/utils.py +import os + +def loadEnvVarsFromFileIntoEnv(): + d = dict() + with open("./.env", "r") as fp: + for line in fp: + line = line.strip() + if line and not line.startswith("#"): + line = line.split("=", 1) + d[line[0]] = line[1] + + for key, value in d.items(): + os.environ[key] = value + +# Then in this and other files: +from sink.utils import loadEnvVarsFromFileIntoEnv
33-33: Remove unused variable.The variable
time_start_cache_disabledis defined but never used in this test file.-time_start_cache_disabled = time.time()
68-78: Improve error handling specificity.The error handling correctly tests that retrieving a deleted secret throws an exception, but it would be more robust to assert on the specific exception type expected rather than catching all exceptions.
try: single_secret_cached = cache_enabled_client.secrets.get_secret_by_name( secret_name=created_sec.secretKey, project_id=SECRETS_PROJECT_ID, environment_slug=SECRETS_ENVIRONMENT_SLUG, secret_path="/", expand_secret_references=True, include_imports=True) -except Exception as e: +except KeyError as e: # Replace with the specific exception type that should be raised print(e) print("Good, we errored as expected!") +else: + raise AssertionError("Expected an error when retrieving a deleted secret, but no error was raised")
1-79: Add docstring or comments explaining the test purpose.The file tests cache invalidation when deleting secrets, but lacks a docstring or comments that clearly explain the purpose of the test.
from infisical_sdk import InfisicalSDKClient import time import os import random import string +""" +Test to verify that the SDK's caching system correctly handles deleted secrets. + +This test: +1. Creates a new random secret +2. Retrieves it to verify it's cached +3. Deletes the secret +4. Attempts to retrieve it again, expecting an error + +This ensures that deleted secrets don't remain accessible via the cache. +""" def loadEnvVarsFromFileIntoEnv():sink/cache_test.py (4)
37-44: Fix unused loop variable.The loop variable
iis defined but not used in the loop. Consider renaming it to_or_ito indicate it's intentionally unused.-for i in range(100): +for _ in range(100): all_secrets = cache_disabled_client.secrets.list_secrets( project_id=SECRETS_PROJECT_ID, environment_slug=SECRETS_ENVIRONMENT_SLUG, secret_path="/", expand_secret_references=True, include_imports=True )🧰 Tools
🪛 Ruff (0.8.2)
37-37: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
53-60: Fix unused loop variable.Same issue with unused loop variable
i.-for i in range(100): +for _ in range(100): all_secrets = cache_enabled_client.secrets.list_secrets( project_id=SECRETS_PROJECT_ID, environment_slug=SECRETS_ENVIRONMENT_SLUG, secret_path="/", expand_secret_references=True, include_imports=True )🧰 Tools
🪛 Ruff (0.8.2)
53-53: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
68-91: Consider adding verification of cached results.This code tests retrieving the same secret with different parameter combinations, but doesn't verify that the responses are correct or that caching behaves correctly with different parameters.
Consider adding assertions to verify:
- That secrets retrieved with different parameters are cached separately
- That the secret values match expected values
- That subsequent retrievals are faster than the first retrieval
# Example verification code to add import time # First retrieval (uncached) start_time = time.time() secret1 = cache_enabled_client.secrets.get_secret_by_name( secret_name="TEST", project_id=SECRETS_PROJECT_ID, environment_slug=SECRETS_ENVIRONMENT_SLUG, secret_path="/", expand_secret_references=True, include_imports=True) first_retrieval_time = time.time() - start_time # Second retrieval (should be cached) start_time = time.time() secret2 = cache_enabled_client.secrets.get_secret_by_name( secret_name="TEST", project_id=SECRETS_PROJECT_ID, environment_slug=SECRETS_ENVIRONMENT_SLUG, secret_path="/", expand_secret_references=True, include_imports=True) second_retrieval_time = time.time() - start_time # Verify values match assert secret1.secretValue == secret2.secretValue, "Cached secret value doesn't match original" # Verify second retrieval is faster assert second_retrieval_time < first_retrieval_time, "Cached retrieval should be faster" print(f"First retrieval: {first_retrieval_time:.6f}s, Cached retrieval: {second_retrieval_time:.6f}s")
1-91: Add docstring to explain the test purpose.The file appears to be benchmarking the performance difference between cached and non-cached operations, but lacks a clear docstring explaining its purpose.
from infisical_sdk import InfisicalSDKClient import time import os +""" +Performance benchmark test for secret caching functionality. + +This test compares the performance of retrieving secrets with and without caching: +1. Lists secrets 100 times with caching disabled and measures the time +2. Lists secrets 100 times with caching enabled and measures the time +3. Retrieves a single secret multiple times with different parameter combinations + to verify the caching behavior with varying parameters + +Expected outcome: The cached operations should be significantly faster than non-cached ones. +""" def loadEnvVarsFromFileIntoEnv():🧰 Tools
🪛 Ruff (0.8.2)
37-37: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
53-53: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
sink/cache_expire_test.py (3)
43-51: Fix unused loop variable.The loop variable
iis defined but not used in the loop. Consider renaming it to_or_ito indicate it's intentionally unused.-for i in range(10): +for _ in range(10): single_secret_cached = cache_enabled_client.secrets.get_secret_by_name( secret_name="TEST", project_id=SECRETS_PROJECT_ID, environment_slug=SECRETS_ENVIRONMENT_SLUG, secret_path="/", expand_secret_references=True, include_imports=True)🧰 Tools
🪛 Ruff (0.8.2)
43-43: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
55-69: Test looks good but could use assertions.This part of the test correctly demonstrates cache expiration by sleeping for a period equal to the cache TTL and then fetching again. However, it only prints times without asserting that the time after expiration is indeed slower than when the cache is fresh.
Consider adding assertions to verify the cache expiration behavior:
# Store the cached retrieval time cached_retrieval_time = time_end_cache_enabled - time_start_cache_enabled print("Sleeping for 10 seconds") time.sleep(10) print("Getting secret again") time_start_expired = time.time() expired_secret = cache_enabled_client.secrets.get_secret_by_name( secret_name="TEST", project_id=SECRETS_PROJECT_ID, environment_slug=SECRETS_ENVIRONMENT_SLUG, secret_path="/", expand_secret_references=True, include_imports=True) time_end_expired = time.time() expired_retrieval_time = time_end_expired - time_start_expired print(f"[CACHE EXPIRED] Time taken: {expired_retrieval_time} seconds") # Assert that retrieving with expired cache is slower assert expired_retrieval_time > cached_retrieval_time, "Expired cache retrieval should be slower than cached retrieval" ratio = expired_retrieval_time / cached_retrieval_time print(f"Expired retrieval is {ratio:.2f}x slower than cached retrieval")
1-73: Add docstring explaining the test purpose.The file tests the cache expiration behavior but lacks a clear docstring explaining its purpose.
from infisical_sdk import InfisicalSDKClient import time import os +""" +Test to verify cache expiration behavior. + +This test: +1. Retrieves a secret once to prime the cache +2. Retrieves the same secret multiple times in rapid succession and measures the time +3. Waits for the cache TTL to expire (10 seconds) +4. Retrieves the secret again and measures the time + +Expected outcome: The retrieval after expiration should be slower than when using +the active cache, demonstrating that the cache does indeed expire after the TTL. +""" + def loadEnvVarsFromFileIntoEnv():🧰 Tools
🪛 Ruff (0.8.2)
43-43: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
sink/example.py (1)
127-127: Minor naming inconsistency.To maintain clarity, rename the variable "plantext" to "plaintext":
- plantext = "Hello, world!" + plaintext = "Hello, world!"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.gitignore(1 hunks)README.md(1 hunks)infisical_sdk/client.py(1 hunks)infisical_sdk/resources/secrets.py(1 hunks)setup.cfg(1 hunks)sink/.env.example(1 hunks)sink/cache_deletion_test.py(1 hunks)sink/cache_expire_test.py(1 hunks)sink/cache_test.py(1 hunks)sink/example.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- sink/.env.example
🧰 Additional context used
🧠 Learnings (2)
sink/cache_test.py (1)
Learnt from: DanielHougaard
PR: Infisical/python-sdk-official#32
File: sink/cache_test.py:7-8
Timestamp: 2025-04-02T17:04:48.405Z
Learning: Credentials in files within the `sink` directory are intentionally using placeholder values as these are demo files for testing and showcasing SDK functionality, not production code.
infisical_sdk/resources/secrets.py (1)
Learnt from: DanielHougaard
PR: Infisical/python-sdk-official#32
File: infisical_sdk/resources/secrets.py:23-24
Timestamp: 2025-04-02T17:04:48.405Z
Learning: The mutable default arguments issue in the `list_secrets` method with `tag_filters: List[str] = []` is handled by the API layer, so this doesn't need to be fixed in the codebase.
🧬 Code Definitions (6)
sink/cache_expire_test.py (6)
infisical_sdk/client.py (1)
InfisicalSDKClient(9-39)sink/cache_test.py (1)
loadEnvVarsFromFileIntoEnv(6-16)sink/example.py (1)
loadEnvVarsFromFileIntoEnv(8-18)sink/cache_deletion_test.py (1)
loadEnvVarsFromFileIntoEnv(8-18)infisical_sdk/resources/auth_methods/universal_auth.py (1)
login(10-35)infisical_sdk/resources/secrets.py (1)
get_secret_by_name(58-103)
sink/example.py (6)
sink/cache_test.py (1)
loadEnvVarsFromFileIntoEnv(6-16)sink/cache_expire_test.py (1)
loadEnvVarsFromFileIntoEnv(6-16)sink/cache_deletion_test.py (1)
loadEnvVarsFromFileIntoEnv(8-18)infisical_sdk/resources/auth_methods/universal_auth.py (1)
login(10-35)infisical_sdk/resources/secrets.py (3)
create_secret_by_name(105-147)update_secret_by_name(149-194)delete_secret_by_name(196-227)infisical_sdk/resources/kms.py (7)
create_key(66-86)encrypt_data(121-148)decrypt_data(150-177)get_key_by_id(38-47)get_key_by_name(49-64)list_keys(12-36)delete_key(109-119)
infisical_sdk/client.py (5)
infisical_sdk/resources/auth.py (1)
Auth(6-10)infisical_sdk/resources/secrets.py (1)
V3RawSecrets(10-227)infisical_sdk/resources/kms.py (1)
KMS(8-177)infisical_sdk/util/secrets_cache.py (1)
SecretsCache(10-87)infisical_sdk/infisical_requests.py (2)
InfisicalRequests(53-192)set_token(71-73)
sink/cache_test.py (6)
infisical_sdk/client.py (1)
InfisicalSDKClient(9-39)sink/cache_expire_test.py (1)
loadEnvVarsFromFileIntoEnv(6-16)sink/example.py (1)
loadEnvVarsFromFileIntoEnv(8-18)sink/cache_deletion_test.py (1)
loadEnvVarsFromFileIntoEnv(8-18)infisical_sdk/resources/auth_methods/universal_auth.py (1)
login(10-35)infisical_sdk/resources/secrets.py (2)
list_secrets(15-56)get_secret_by_name(58-103)
sink/cache_deletion_test.py (3)
sink/cache_test.py (1)
loadEnvVarsFromFileIntoEnv(6-16)sink/cache_expire_test.py (1)
loadEnvVarsFromFileIntoEnv(6-16)sink/example.py (1)
loadEnvVarsFromFileIntoEnv(8-18)
infisical_sdk/resources/secrets.py (1)
infisical_sdk/util/secrets_cache.py (4)
SecretsCache(10-87)compute_cache_key(28-32)get(34-47)unset(58-63)
🪛 Ruff (0.8.2)
sink/cache_expire_test.py
43-43: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
sink/cache_test.py
37-37: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
53-53: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
infisical_sdk/resources/secrets.py
24-24: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🪛 LanguageTool
README.md
[uncategorized] ~68-~68: Loose punctuation mark.
Context: ...lowing high-level categories: 1. auth: Handles authentication methods. 2. `sec...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~69-~69: Loose punctuation mark.
Context: ...les authentication methods. 2. secrets: Manages CRUD operations for secrets. 3....
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~70-~70: Loose punctuation mark.
Context: ...es CRUD operations for secrets. 3. kms: Perform cryptographic operations with I...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (7)
infisical_sdk/client.py (1)
23-26: Looks good!Instantiating
SecretsCache,Auth,V3RawSecrets, andKMShere provides a clean modular approach. The initialization flow appears straightforward and maintainable.infisical_sdk/resources/secrets.py (1)
24-24: Previous mutable default argument comment still applies.Using
tag_filters: List[str] = []as a default argument can lead to shared state between calls. However, based on your past feedback, you’ve confirmed the API layer handles this scenario, so no changes are requested. Marking as duplicate of a past comment.🧰 Tools
🪛 Ruff (0.8.2)
24-24: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
README.md (2)
57-61: Example looks good.The initialization example clearly demonstrates all the parameters with helpful comments. The comma after the token parameter is correctly included based on past review comments.
68-70: Documentation for the new kms category.The new kms category is now properly documented in the Core Methods section, which matches the implementation changes.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~68-~68: Loose punctuation mark.
Context: ...lowing high-level categories: 1.auth: Handles authentication methods. 2. `sec...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~69-~69: Loose punctuation mark.
Context: ...les authentication methods. 2.secrets: Manages CRUD operations for secrets. 3....(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~70-~70: Loose punctuation mark.
Context: ...es CRUD operations for secrets. 3.kms: Perform cryptographic operations with I...(UNLIKELY_OPENING_PUNCTUATION)
sink/example.py (3)
37-40: Use of non-cryptographic random generator has been previously flagged.In previous reviews, we suggested using the
secretsmodule for better randomness guarantees. However, we understand from past conversations that this is a demo file in thesinkfolder, and security-grade randomness is not strictly necessary here. Marking this as a duplicate comment.
98-99: Potential issue with filtering secrets before counting.After filtering out secrets whose key is
"TEST", the code expects exactly one secret to remain. This may fail if there are multiple secrets with different keys. Ensure that the environment is controlled or adjust the logic if more secrets could exist.
1-167: Overall code structure and clarity.The file demonstrates the InfisicalSDKClient usage well, including environment variable loading and sample KMS/secret operations. The code is cohesive, reads environment variables securely, and shows how to interact with the client’s methods.
scott-ray-wilson
left a comment
scott-ray-wilson
left a comment
There was a problem hiding this comment.
Two concerns and a question
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
infisical_sdk/resources/secrets.py (1)
24-24: 🛠️ Refactor suggestionAvoid using a mutable default argument.
Using a mutable list (
[]) as a default argument can lead to unexpected shared state across function calls. A safer pattern is to useNoneand create a new list within the method:- tag_filters: List[str] = [] + tag_filters: Optional[List[str]] = None ... if tag_filters is None: tag_filters = []🧰 Tools
🪛 Ruff (0.8.2)
24-24: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
🧹 Nitpick comments (3)
infisical_sdk/util/secrets_cache.py (2)
3-3: Remove unused import.
BaseSecretis imported here but never actually used in this file. Removing it will help keep the code clean and avoid lint warnings.-from infisical_sdk.api_types import BaseSecret🧰 Tools
🪛 Ruff (0.8.2)
3-3:
infisical_sdk.api_types.BaseSecretimported but unusedRemove unused import:
infisical_sdk.api_types.BaseSecret(F401)
100-105: Use a more reliable shutdown method.Relying on
__del__to stop the cleanup thread can be fragile since__del__is not guaranteed to run in all scenarios. A safer approach is to expose an explicitclose()orshutdown()method that callers can invoke before object deallocation, ensuring the cleanup thread is properly terminated.infisical_sdk/resources/secrets.py (1)
149-197: Consider partial invalidation for performance.After updating a secret, this code invalidates all list-secrets cache entries. While consistent, it may degrade performance if you have many cached lists. A more targeted cache-invalidation approach could improve efficiency for large-scale usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
infisical_sdk/resources/secrets.py(1 hunks)infisical_sdk/util/secrets_cache.py(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
infisical_sdk/util/secrets_cache.py (1)
infisical_sdk/api_types.py (1)
BaseSecret(63-83)
🪛 Ruff (0.8.2)
infisical_sdk/util/secrets_cache.py
3-3: infisical_sdk.api_types.BaseSecret imported but unused
Remove unused import: infisical_sdk.api_types.BaseSecret
(F401)
infisical_sdk/resources/secrets.py
24-24: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🔇 Additional comments (1)
infisical_sdk/util/secrets_cache.py (1)
30-33:Details
❓ Verification inconclusive
Ensure JSON-serializable keyword arguments.
The
json.dumps()call might raise aTypeErrorif any of the values inkwargsare not JSON-serializable. Consider documenting or enforcing supported argument types, or extending serialization to handle custom objects.
🏁 Script executed:
#!/bin/bash # Description: Check if there are any calls passing non-JSON-serializable arguments to `compute_cache_key`. rg -A 5 "compute_cache_key"Length of output: 2813
Ensure JSON-serializable kwargs for compute_cache_key
I reviewed the call sites (e.g., in
infisical_sdk/resources/secrets.py) and found thatcompute_cache_keyis consistently invoked with typical dictionary parameters, which are likely JSON serializable. However, to prevent potential runtime errors when non-JSON-serializable objects might be passed in the future, please consider either:
Explicitly documenting that only basic types (e.g., strings, numbers, booleans) are supported.
Optionally adding input validation or error handling to provide a clear message when non-serializable values are encountered.
Location:
infisical_sdk/util/secrets_cache.py, lines 30–33Context: Call sites in
infisical_sdk/resources/secrets.pypass kwargs via**paramsand**cache_params.
scott-ray-wilson
left a comment
scott-ray-wilson
left a comment
There was a problem hiding this comment.
last comment, rest looks good
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
infisical_sdk/util/secrets_cache.py (8)
3-3: Remove unused import BaseSecret.The import
BaseSecretfrominfisical_sdk.api_typesis not used anywhere in this file.-from infisical_sdk.api_types import BaseSecret🧰 Tools
🪛 Ruff (0.8.2)
3-3:
infisical_sdk.api_types.BaseSecretimported but unusedRemove unused import:
infisical_sdk.api_types.BaseSecret(F401)
10-10: Consider making MAX_CACHE_SIZE configurable.The cache size limit is currently hardcoded to 1000 entries. Making this configurable would provide more flexibility for different usage scenarios with varying memory constraints.
-MAX_CACHE_SIZE = 1000 +DEFAULT_MAX_CACHE_SIZE = 1000 class SecretsCache: - def __init__(self, ttl_seconds: int = 60) -> None: + def __init__(self, ttl_seconds: int = 60, max_cache_size: int = DEFAULT_MAX_CACHE_SIZE) -> None: if ttl_seconds is None or ttl_seconds <= 0: self.enabled = False return self.enabled = True self.ttl = ttl_seconds self.cleanup_interval = 60 + self.max_cache_size = max_cache_size
13-29: Add class and method docstrings.The class and its methods lack documentation. Adding docstrings would help other developers understand the purpose and usage of this cache implementation.
class SecretsCache: + """A thread-safe cache for secrets with TTL-based expiration and size limits. + + This class provides caching functionality for secrets with automatic + expiration after a configurable time-to-live (TTL) period. It also + maintains a maximum cache size by evicting the oldest entries when needed. + """ def __init__(self, ttl_seconds: int = 60) -> None: + """Initialize the secrets cache. + + Args: + ttl_seconds: Time-to-live in seconds for cached items. If None or <= 0, + caching is disabled. + """ if ttl_seconds is None or ttl_seconds <= 0: self.enabled = False return
20-20: Make cleanup_interval configurable.The cleanup interval is hardcoded to 60 seconds. Consider making this configurable through the constructor to allow for customization based on workload characteristics.
class SecretsCache: - def __init__(self, ttl_seconds: int = 60) -> None: + def __init__(self, ttl_seconds: int = 60, cleanup_interval: int = 60) -> None: if ttl_seconds is None or ttl_seconds <= 0: self.enabled = False return self.enabled = True self.ttl = ttl_seconds - self.cleanup_interval = 60 + self.cleanup_interval = cleanup_interval
30-35: Document JSON serialization limitations in compute_cache_key.The method uses
json.dumps()which may fail with non-serializable types like datetime objects or custom classes. This limitation should be documented.def compute_cache_key(self, operation_name: str, **kwargs) -> str: + """Compute a unique cache key based on operation name and kwargs. + + Args: + operation_name: The name of the operation + **kwargs: Additional parameters used for the cache key + + Returns: + A unique string key derived from operation name and parameters + + Note: + The kwargs must contain only JSON-serializable types + (str, int, float, bool, list, dict, None). + """ sorted_kwargs = sorted(kwargs.items()) json_str = json.dumps(sorted_kwargs) return f"{operation_name}-{sha256(json_str.encode()).hexdigest()}"
52-63: Add memory usage safeguards for large values.While the implementation limits the number of entries to
MAX_CACHE_SIZE, there's no limit on the size of individual entries. Large secret values could lead to excessive memory usage.Consider adding a size check before storing values to prevent memory issues:
def set(self, cache_key: str, value: Any) -> None: + """Store a value in the cache with the current timestamp. + + Args: + cache_key: The key under which to store the value + value: The value to cache + """ if not self.enabled: return with self.lock: serialized_value = pickle.dumps(value) + # Optional size limit check + if len(serialized_value) > 1024 * 1024: # 1MB size limit example + return # Skip caching for very large values + self.cache[cache_key] = (serialized_value, time.time()) - if len(self.cache) > MAX_CACHE_SIZE: + if len(self.cache) > self.max_cache_size: oldest_key = min(self.cache.keys(), key=lambda k: self.cache[k][1]) # oldest key based on timestamp self.cache.pop(oldest_key)
100-105: Improve thread cleanup in the destructor.The current
__del__implementation has a race condition if the object is being deleted during a cleanup operation. Also, the join timeout could be too short for heavy cleanup.def __del__(self) -> None: """Ensure thread is properly stopped when the object is garbage collected.""" self.stop_cleanup_thread = True if self.enabled and hasattr(self, 'cleanup_thread') and self.cleanup_thread.is_alive(): - self.cleanup_thread.join(timeout=1.0) + # Use a longer timeout for potentially heavy cleanup operations + try: + self.cleanup_thread.join(timeout=5.0) + except Exception: + pass # Best effort cleanup
12-105: Add statistics and monitoring capabilities.For a caching system, it's valuable to track hit/miss rates and other performance metrics. Consider adding simple statistics collection.
This could be implemented by adding counters for hits, misses, and evictions, along with methods to retrieve these statistics:
def __init__(self, ttl_seconds: int = 60): # Existing initialization code... # Add statistics counters self.stats = { 'hits': 0, 'misses': 0, 'evictions': 0, 'expirations': 0 } def get(self, cache_key: str) -> Any: # Existing get logic with added statistics if not self.enabled: return None with self.lock: if cache_key in self.cache: serialized_value, timestamp = self.cache[cache_key] if time.time() - timestamp <= self.ttl: self.stats['hits'] += 1 return pickle.loads(serialized_value) else: self.stats['expirations'] += 1 self.cache.pop(cache_key, None) self.stats['misses'] += 1 return None else: self.stats['misses'] += 1 return None def get_statistics(self) -> Dict[str, int]: """Return cache statistics.""" with self.lock: return self.stats.copy()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infisical_sdk/util/secrets_cache.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
infisical_sdk/util/secrets_cache.py (1)
infisical_sdk/api_types.py (1)
BaseSecret(63-83)
🪛 Ruff (0.8.2)
infisical_sdk/util/secrets_cache.py
3-3: infisical_sdk.api_types.BaseSecret imported but unused
Remove unused import: infisical_sdk.api_types.BaseSecret
(F401)
🔇 Additional comments (1)
infisical_sdk/util/secrets_cache.py (1)
57-58: Caution with pickle serialization.Using pickle for serialization can be a security risk if the cache might store untrusted data. Consider alternatives like JSON with custom serializers for complex types if security is a concern.
Do you expect to store potentially untrusted data in this cache? If so, consider using a more secure serialization method.
scott-ray-wilson
left a comment
scott-ray-wilson
left a comment
There was a problem hiding this comment.
Actually looks good to me this time
This PR adds caching support for listing secrets and fetching a single secret. Additionally I took the freedom to do the much needed structural refactoring of the Python SDK, as this seemed like the right time to do it as we've adding utility folders as well.
I've also added a new
sinkfolder which holds all the testing files. I've added two new files for testing the cache (one to test expiration, and one for general caching speeds).Remember to run
pip install -e .before trying to use the files inside thesinkfolder.Summary by CodeRabbit