added defaultazurecredential for azure redis#272
Open
rootflo-hardik wants to merge 4 commits intodevelopfrom
Open
added defaultazurecredential for azure redis#272rootflo-hardik wants to merge 4 commits intodevelopfrom
rootflo-hardik wants to merge 4 commits intodevelopfrom
Conversation
📝 WalkthroughWalkthroughAdded an Azure-managed Redis credential provider and refactored Redis connection pool construction to select SSL and authentication (password or Azure token) from environment variables; added the Changes
Sequence DiagramsequenceDiagram
actor App as Application
participant CM as CacheManager
participant ARP as AzureManagedRedisProvider
participant DAC as DefaultAzureCredential
participant AAD as AzureAD
participant Redis as RedisServer
App->>CM: _create_connection_pool()
rect rgba(200, 100, 100, 0.5)
CM->>CM: Read env vars, decide ssl/auth
CM->>ARP: Instantiate provider (if CLOUD_PROVIDER=azure && no REDIS_PASSWORD)
ARP->>DAC: Initialize / request token
end
CM->>Redis: ConnectionPool(**pool_kwargs)
rect rgba(100, 150, 200, 0.5)
Redis->>ARP: Request credentials (get_credentials)
ARP->>DAC: get_token(scope=https://redis.azure.com/.default)
DAC->>AAD: Request access token
AAD-->>DAC: Return token
DAC-->>ARP: Return token
ARP-->>Redis: Return ('default', token)
end
Redis-->>CM: Connection established
CM-->>App: Pool ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
🧹 Nitpick comments (3)
wavefront/server/apps/call_processing/call_processing/cache/cache_manager.py (1)
61-62: Missing connection verification compared todb_repo_module.The
db_repo_moduleversion performs aping()test immediately after creating the Redis connection to fail fast if Redis is unreachable. This version only logs success without verification, which could delay failure detection until the first actual cache operation.♻️ Proposed fix to add connection verification
self.redis = self._create_redis_connection() - logger.info('Connected to Redis with connection pooling enabled') + + # Test the connection immediately - fail fast if Redis is unreachable + try: + self.redis.ping() + logger.info('Connected to Redis with connection pooling enabled') + except (ConnectionError, TimeoutError, RedisError) as e: + logger.error(f'Failed to connect to Redis during initialization: {e}') + logger.error('Server will not start without Redis connectivity') + raise RuntimeError(f'Redis connection test failed: {e}') from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wavefront/server/apps/call_processing/call_processing/cache/cache_manager.py` around lines 61 - 62, The Redis connection is being assumed healthy after self._create_redis_connection() but not verified; update CacheManager (use the same place where self.redis is assigned — look for self._create_redis_connection and the logger.info call) to perform an immediate health check by calling self.redis.ping() (or await self.redis.ping() if async), catch connection exceptions, log the error including exception details via logger.error, and fail fast (raise the error or exit) instead of only logging success; after a successful ping, then log the existing "Connected to Redis..." message.wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py (2)
20-33: Consider adding error handling for token acquisition failures.The
get_credentialsmethod doesn't handle exceptions fromDefaultAzureCredential.get_token(), which can raiseCredentialUnavailableErrororClientAuthenticationErrorwhen Azure authentication fails (e.g., missing workload identity configuration, network issues). This could result in cryptic errors during Redis connection attempts.♻️ Proposed improvement with error handling
+from azure.core.exceptions import ClientAuthenticationError + class AzureManagedRedisProvider(CredentialProvider): """ Adapter to bridge Azure Identity with Redis CredentialProvider. Azure Managed Redis requires 'default' as the username and the Entra ID access token as the password. """ def __init__(self): self.credential = DefaultAzureCredential() self.scope = 'https://redis.azure.com/.default' def get_credentials(self): - token = self.credential.get_token(self.scope) - return ('default', token.token) + try: + token = self.credential.get_token(self.scope) + return ('default', token.token) + except ClientAuthenticationError as e: + logger.error(f'Azure authentication failed for Redis: {e}') + raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py` around lines 20 - 33, The AzureManagedRedisProvider.get_credentials currently calls DefaultAzureCredential.get_token without handling authentication errors; wrap the call to self.credential.get_token(self.scope) in a try/except that catches DefaultAzureCredential-related exceptions (e.g., CredentialUnavailableError, ClientAuthenticationError, and a general Exception fallback), log or re-raise a clear, contextual error (including the underlying exception message) and ensure the method returns or raises a predictable error for callers to handle; update the AzureManagedRedisProvider class to perform this guarded token acquisition and surface meaningful diagnostics when token retrieval fails.
99-101: Consider documenting theport == 10000heuristic.The SSL enablement based on
port == 10000appears to be a heuristic for Azure Managed Redis (which uses port 10000 for SSL by default). While this works, it could cause unexpected behavior if another Redis deployment happens to use port 10000 without SSL. Consider either documenting this assumption or making SSL configuration more explicit via theREDIS_PROTOCOLvariable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py` around lines 99 - 101, The current SSL toggle uses a heuristic (port == 10000) that should be documented or made explicit; update cache_manager.py to either (a) add a clear comment above the block explaining that port 10000 is an Azure Managed Redis SSL convention or (b) prefer an explicit environment/config flag (REDIS_PROTOCOL or a new REDIS_USE_SSL) to control SSL enablement and fall back to the protocol variable, and then set pool_kwargs['ssl'] accordingly; reference the existing symbols protocol, port, pool_kwargs, and the new/existing REDIS_PROTOCOL to implement and document the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@wavefront/server/apps/call_processing/call_processing/cache/cache_manager.py`:
- Around line 21-34: The AzureManagedRedisProvider class and the
_create_connection_pool function are duplicated across
call_processing/cache/cache_manager.py and
db_repo_module/cache/cache_manager.py; extract these into a shared common_module
(e.g., common_module/cache/redis.py), move the AzureManagedRedisProvider class
(including DefaultAzureCredential instantiation and scope string) and the
_create_connection_pool implementation into that module, export them, and update
both original files to import AzureManagedRedisProvider and
_create_connection_pool from common_module; ensure any module-level constants
(like the scope string) and error handling remain intact and update
imports/usages to the new symbols in common_module.
---
Nitpick comments:
In
`@wavefront/server/apps/call_processing/call_processing/cache/cache_manager.py`:
- Around line 61-62: The Redis connection is being assumed healthy after
self._create_redis_connection() but not verified; update CacheManager (use the
same place where self.redis is assigned — look for self._create_redis_connection
and the logger.info call) to perform an immediate health check by calling
self.redis.ping() (or await self.redis.ping() if async), catch connection
exceptions, log the error including exception details via logger.error, and fail
fast (raise the error or exit) instead of only logging success; after a
successful ping, then log the existing "Connected to Redis..." message.
In
`@wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py`:
- Around line 20-33: The AzureManagedRedisProvider.get_credentials currently
calls DefaultAzureCredential.get_token without handling authentication errors;
wrap the call to self.credential.get_token(self.scope) in a try/except that
catches DefaultAzureCredential-related exceptions (e.g.,
CredentialUnavailableError, ClientAuthenticationError, and a general Exception
fallback), log or re-raise a clear, contextual error (including the underlying
exception message) and ensure the method returns or raises a predictable error
for callers to handle; update the AzureManagedRedisProvider class to perform
this guarded token acquisition and surface meaningful diagnostics when token
retrieval fails.
- Around line 99-101: The current SSL toggle uses a heuristic (port == 10000)
that should be documented or made explicit; update cache_manager.py to either
(a) add a clear comment above the block explaining that port 10000 is an Azure
Managed Redis SSL convention or (b) prefer an explicit environment/config flag
(REDIS_PROTOCOL or a new REDIS_USE_SSL) to control SSL enablement and fall back
to the protocol variable, and then set pool_kwargs['ssl'] accordingly; reference
the existing symbols protocol, port, pool_kwargs, and the new/existing
REDIS_PROTOCOL to implement and document the change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64b3aefb-f403-4911-bc79-4ba0f745164f
⛔ Files ignored due to path filters (1)
wavefront/server/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
wavefront/server/apps/call_processing/call_processing/cache/cache_manager.pywavefront/server/apps/call_processing/pyproject.tomlwavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.pywavefront/server/modules/db_repo_module/pyproject.toml
Comment on lines
+21
to
+34
| class AzureManagedRedisProvider(CredentialProvider): | ||
| """ | ||
| Adapter to bridge Azure Identity with Redis CredentialProvider. | ||
| Azure Managed Redis requires 'default' as the username and the | ||
| Entra ID access token as the password. | ||
| """ | ||
|
|
||
| def __init__(self): | ||
| self.credential = DefaultAzureCredential() | ||
| self.scope = 'https://redis.azure.com/.default' | ||
|
|
||
| def get_credentials(self): | ||
| token = self.credential.get_token(self.scope) | ||
| return ('default', token.token) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the code duplication between the two cache_manager.py files
echo "=== AzureManagedRedisProvider in db_repo_module ==="
ast-grep --pattern $'class AzureManagedRedisProvider($_) {
$$$
}'
echo ""
echo "=== Diff between the two _create_connection_pool implementations ==="
# Extract and compare the connection pool methods
diff <(sed -n '/def _create_connection_pool/,/return ConnectionPool/p' wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py) \
<(sed -n '/def _create_connection_pool/,/return ConnectionPool/p' wavefront/server/apps/call_processing/call_processing/cache/cache_manager.py) || trueRepository: rootflo/wavefront
Length of output: 287
🏁 Script executed:
#!/bin/bash
# Check if both cache_manager.py files exist
echo "=== Checking file paths ==="
ls -la wavefront/server/apps/call_processing/call_processing/cache/cache_manager.py 2>&1 | head -20
echo ""
ls -la wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py 2>&1 | head -20Repository: rootflo/wavefront
Length of output: 343
🏁 Script executed:
#!/bin/bash
# Search for AzureManagedRedisProvider class in both files
echo "=== AzureManagedRedisProvider in call_processing ==="
rg -A 12 "class AzureManagedRedisProvider" wavefront/server/apps/call_processing/call_processing/cache/cache_manager.py
echo ""
echo "=== AzureManagedRedisProvider in db_repo_module ==="
rg -A 12 "class AzureManagedRedisProvider" wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.pyRepository: rootflo/wavefront
Length of output: 1096
🏁 Script executed:
#!/bin/bash
# Extract _create_connection_pool methods for comparison
echo "=== _create_connection_pool in call_processing ==="
rg -A 30 "def _create_connection_pool" wavefront/server/apps/call_processing/call_processing/cache/cache_manager.py
echo ""
echo "=== _create_connection_pool in db_repo_module ==="
rg -A 30 "def _create_connection_pool" wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.pyRepository: rootflo/wavefront
Length of output: 2572
🏁 Script executed:
#!/bin/bash
# Check if common_module exists and what cache utilities it has
echo "=== Checking common_module structure ==="
find wavefront -name "common_module" -type d 2>/dev/null | head -10
echo ""
echo "=== Files in common_module/cache if it exists ==="
find wavefront -path "*/common_module/cache*" -type f 2>/dev/null
echo ""
echo "=== Searching for CommonCache class ==="
rg -l "class CommonCache" wavefront/Repository: rootflo/wavefront
Length of output: 354
🏁 Script executed:
#!/bin/bash
# Get full _create_connection_pool methods to see complete duplication
echo "=== Full _create_connection_pool in call_processing ==="
rg -A 50 "def _create_connection_pool" wavefront/server/apps/call_processing/call_processing/cache/cache_manager.py
echo ""
echo "=== Full _create_connection_pool in db_repo_module ==="
rg -A 50 "def _create_connection_pool" wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.pyRepository: rootflo/wavefront
Length of output: 4048
🏁 Script executed:
#!/bin/bash
# Check if there are other duplicated methods/classes between the two files
echo "=== Line counts and structure of both cache_manager.py files ==="
wc -l wavefront/server/apps/call_processing/call_processing/cache/cache_manager.py
wc -l wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py
echo ""
echo "=== Classes/methods in call_processing cache_manager.py ==="
rg "^class |^ def " wavefront/server/apps/call_processing/call_processing/cache/cache_manager.py
echo ""
echo "=== Classes/methods in db_repo_module cache_manager.py ==="
rg "^class |^ def " wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.pyRepository: rootflo/wavefront
Length of output: 1828
🏁 Script executed:
#!/bin/bash
# Check common_module structure and what's already there
echo "=== common_module structure ==="
find wavefront/server/modules/common_module -type f -name "*.py" | head -20
echo ""
echo "=== CommonCache class location and size ==="
wc -l wavefront/server/modules/common_module/common_module/common_cache.py
</parameter>Repository: rootflo/wavefront
Length of output: 1516
Extract duplicated Redis configuration code to common_module.
The AzureManagedRedisProvider class and the _create_connection_pool method are duplicated verbatim between this file and db_repo_module/cache/cache_manager.py. Both should be moved to a shared location within common_module to avoid maintaining identical code in two places. Any future updates (scope changes, error handling improvements, environment variable additions) would otherwise need to be applied separately in both files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wavefront/server/apps/call_processing/call_processing/cache/cache_manager.py`
around lines 21 - 34, The AzureManagedRedisProvider class and the
_create_connection_pool function are duplicated across
call_processing/cache/cache_manager.py and
db_repo_module/cache/cache_manager.py; extract these into a shared common_module
(e.g., common_module/cache/redis.py), move the AzureManagedRedisProvider class
(including DefaultAzureCredential instantiation and scope string) and the
_create_connection_pool implementation into that module, export them, and update
both original files to import AzureManagedRedisProvider and
_create_connection_pool from common_module; ensure any module-level constants
(like the scope string) and error handling remain intact and update
imports/usages to the new symbols in common_module.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
wavefront/server/apps/call_processing/call_processing/cache/cache_manager.py (2)
101-107: Clarify authentication precedence in a comment.The logic correctly prioritizes password-based auth when
REDIS_PASSWORDis set (even on Azure), allowing fallback for debugging or hybrid scenarios. A brief comment would help future maintainers understand this is intentional.💡 Suggested comment
+ # Password auth takes precedence if set; Azure token auth used only when + # CLOUD_PROVIDER=azure and no password is configured if cloud_provider == 'azure' and not password:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wavefront/server/apps/call_processing/call_processing/cache/cache_manager.py` around lines 101 - 107, Add a brief clarifying comment above the conditional that checks cloud_provider == 'azure' and password to state that password-based authentication (REDIS_PASSWORD) takes precedence even on Azure, and that AzureManagedRedisProvider is used only as a fallback for workload-identity scenarios; update the block around the cloud_provider/password check (variables: cloud_provider, password, pool_kwargs and AzureManagedRedisProvider) so future maintainers understand this intentional precedence.
97-99: Document the magic port number or use configuration.The
port == 10000check is Azure Managed Redis-specific but undocumented in the code. This could be confusing for maintainers and fragile if Azure changes ports.💡 Suggested improvement
Consider using an environment variable or adding a clarifying comment:
- if protocol == 'rediss' or port == 10000: + # Azure Managed Redis uses port 10000 with SSL by default + if protocol == 'rediss' or port == 10000: logger.info(f'Enabling SSL for Redis connection (Port: {port})') pool_kwargs['ssl'] = TrueAlternatively, use a dedicated
REDIS_SSLenvironment variable for explicit control:use_ssl = os.getenv('REDIS_SSL', '').lower() == 'true' or protocol == 'rediss' if use_ssl: pool_kwargs['ssl'] = True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wavefront/server/apps/call_processing/call_processing/cache/cache_manager.py` around lines 97 - 99, The conditional in cache_manager.py that enables SSL uses a magic number (port == 10000) — update the check around protocol, port and pool_kwargs to be explicit: introduce a configuration/env var (e.g. REDIS_SSL or REDIS_AZURE_MANAGED) and use it alongside protocol == 'rediss' when deciding to set pool_kwargs['ssl'] and logging via logger.info; add a short clarifying comment explaining the Azure-managed-Redis case (or define a named constant like AZURE_MANAGED_REDIS_PORT) and prefer reading os.getenv('REDIS_SSL', '').lower() == 'true' so maintainers can opt-in instead of relying on the undocumented port check.wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py (1)
104-106: Same magic port number issue as noted in the other file.The
port == 10000check should be documented or replaced with explicit configuration. See the suggestion incall_processing/cache/cache_manager.pyfor options.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py` around lines 104 - 106, The conditional that enables SSL based on protocol == 'rediss' or port == 10000 is using a magic port (10000); update the logic in cache_manager where protocol and port are evaluated so SSL is controlled by an explicit configuration flag (e.g., redis_ssl or use_ssl) or by a named constant, not by literal 10000. Replace the port == 10000 check with a config-driven check (read from existing settings/ENV) and update the logger message in the same block that sets pool_kwargs['ssl'] = True to reflect the config key; keep protocol == 'rediss' behavior but remove/replace the magic-port branch and document the new config variable where connection options are loaded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@wavefront/server/apps/call_processing/call_processing/cache/cache_manager.py`:
- Around line 101-107: Add a brief clarifying comment above the conditional that
checks cloud_provider == 'azure' and password to state that password-based
authentication (REDIS_PASSWORD) takes precedence even on Azure, and that
AzureManagedRedisProvider is used only as a fallback for workload-identity
scenarios; update the block around the cloud_provider/password check (variables:
cloud_provider, password, pool_kwargs and AzureManagedRedisProvider) so future
maintainers understand this intentional precedence.
- Around line 97-99: The conditional in cache_manager.py that enables SSL uses a
magic number (port == 10000) — update the check around protocol, port and
pool_kwargs to be explicit: introduce a configuration/env var (e.g. REDIS_SSL or
REDIS_AZURE_MANAGED) and use it alongside protocol == 'rediss' when deciding to
set pool_kwargs['ssl'] and logging via logger.info; add a short clarifying
comment explaining the Azure-managed-Redis case (or define a named constant like
AZURE_MANAGED_REDIS_PORT) and prefer reading os.getenv('REDIS_SSL', '').lower()
== 'true' so maintainers can opt-in instead of relying on the undocumented port
check.
In
`@wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py`:
- Around line 104-106: The conditional that enables SSL based on protocol ==
'rediss' or port == 10000 is using a magic port (10000); update the logic in
cache_manager where protocol and port are evaluated so SSL is controlled by an
explicit configuration flag (e.g., redis_ssl or use_ssl) or by a named constant,
not by literal 10000. Replace the port == 10000 check with a config-driven check
(read from existing settings/ENV) and update the logger message in the same
block that sets pool_kwargs['ssl'] = True to reflect the config key; keep
protocol == 'rediss' behavior but remove/replace the magic-port branch and
document the new config variable where connection options are loaded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ed894d0-4b0e-4dfe-9006-ed944a41a7de
📒 Files selected for processing (2)
wavefront/server/apps/call_processing/call_processing/cache/cache_manager.pywavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py (1)
92-95: Document the magic port number10000.Same issue as the
call_processingmodule - the Azure Cache for Redis SSL port should be documented or extracted to a named constant for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py` around lines 92 - 95, The code uses a magic port value 10000 when selecting SSLConnection (in the conditional that sets connection_class based on protocol and port); extract that numeric literal into a clearly named constant (e.g., AZURE_REDIS_SSL_PORT or REDIS_AZURE_SSL_PORT) and replace the literal 10000 in the condition, and add a short comment explaining it's the Azure Cache for Redis SSL port; update references in cache_manager.py where connection_class, SSLConnection and protocol/port are checked so the intent is clear.wavefront/server/apps/call_processing/call_processing/cache/cache_manager.py (1)
86-88: Document the magic port number10000.The condition
port == 10000is Azure Cache for Redis's SSL port, but this isn't obvious to readers. Consider extracting to a named constant or adding a brief comment.📝 Suggested improvement
+ # Azure Cache for Redis uses port 10000 for SSL connections connection_class = Connection - if protocol == 'rediss' or port == 10000: + AZURE_REDIS_SSL_PORT = 10000 + if protocol == 'rediss' or port == AZURE_REDIS_SSL_PORT: logger.info(f'Using SSLConnection for Redis (Port: {port})') connection_class = SSLConnection🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wavefront/server/apps/call_processing/call_processing/cache/cache_manager.py` around lines 86 - 88, The check that treats port == 10000 as SSL is a magic number; extract it into a named constant (e.g., AZURE_CACHE_REDIS_SSL_PORT = 10000) or add an inline comment explaining it is Azure Cache for Redis's SSL port, then replace the literal in the condition (the if that sets connection_class = SSLConnection) with that constant and update the log message to reference the constant for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@wavefront/server/apps/call_processing/call_processing/cache/cache_manager.py`:
- Around line 105-112: The current branch silently creates an unauthenticated
Redis pool when cloud_provider != 'azure' and password is falsy; update the
logic around the pool_kwargs assignment (the block referencing cloud_provider,
password, pool_kwargs and AzureManagedRedisProvider) to either (a) log a clear
warning via logger.warning when running in non-local/non-debug modes that Redis
will be created without authentication, or (b) require an explicit opt-in
environment flag (e.g., ALLOW_UNAUTHENTICATED_REDIS or a check against
settings.DEBUG/ENVIRONMENT) before allowing omission of pool_kwargs['password'];
implement the chosen approach by adding a conditional guard and warning/error
log so production misconfiguration is visible and only permit unauthenticated
connections when the opt-in flag is present.
In
`@wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py`:
- Around line 112-118: The code silently falls back to unauthenticated Redis
when cloud_provider != 'azure' and password is falsy; update cache_manager.py to
detect this condition and emit a warning log so misconfigured REDIS_PASSWORD is
visible: inside the block handling pool_kwargs (referencing cloud_provider,
password, pool_kwargs and AzureManagedRedisProvider) add a logger.warn/info
message when password is not provided for non-azure providers, and ensure that
when cloud_provider == 'azure' we still use AzureManagedRedisProvider as before;
keep existing behavior but surface the missing credentials via a clear log
message.
---
Nitpick comments:
In
`@wavefront/server/apps/call_processing/call_processing/cache/cache_manager.py`:
- Around line 86-88: The check that treats port == 10000 as SSL is a magic
number; extract it into a named constant (e.g., AZURE_CACHE_REDIS_SSL_PORT =
10000) or add an inline comment explaining it is Azure Cache for Redis's SSL
port, then replace the literal in the condition (the if that sets
connection_class = SSLConnection) with that constant and update the log message
to reference the constant for clarity.
In
`@wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py`:
- Around line 92-95: The code uses a magic port value 10000 when selecting
SSLConnection (in the conditional that sets connection_class based on protocol
and port); extract that numeric literal into a clearly named constant (e.g.,
AZURE_REDIS_SSL_PORT or REDIS_AZURE_SSL_PORT) and replace the literal 10000 in
the condition, and add a short comment explaining it's the Azure Cache for Redis
SSL port; update references in cache_manager.py where connection_class,
SSLConnection and protocol/port are checked so the intent is clear.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a51dce9-7f9a-4c41-a4b8-ffffb53ce04f
📒 Files selected for processing (2)
wavefront/server/apps/call_processing/call_processing/cache/cache_manager.pywavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py
Comment on lines
+105
to
+112
| if cloud_provider == 'azure' and not password: | ||
| logger.info( | ||
| 'Configuring Azure Entra ID (Workload Identity) authentication' | ||
| ) | ||
| pool_kwargs['credential_provider'] = AzureManagedRedisProvider() | ||
| elif password: | ||
| pool_kwargs['password'] = password | ||
|
|
There was a problem hiding this comment.
Silent fallback to unauthenticated Redis when credentials are misconfigured.
When CLOUD_PROVIDER is not azure and REDIS_PASSWORD is unset, the pool is created without any authentication. This may be intentional for local development, but in production it risks connecting to Redis without auth if environment variables are misconfigured.
Consider adding a warning log or, for non-local environments, requiring explicit opt-in for unauthenticated connections:
🛡️ Suggested improvement
if cloud_provider == 'azure' and not password:
logger.info(
'Configuring Azure Entra ID (Workload Identity) authentication'
)
pool_kwargs['credential_provider'] = AzureManagedRedisProvider()
elif password:
pool_kwargs['password'] = password
+ else:
+ logger.warning(
+ 'No Redis authentication configured. '
+ 'Set REDIS_PASSWORD or use CLOUD_PROVIDER=azure for managed identity.'
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wavefront/server/apps/call_processing/call_processing/cache/cache_manager.py`
around lines 105 - 112, The current branch silently creates an unauthenticated
Redis pool when cloud_provider != 'azure' and password is falsy; update the
logic around the pool_kwargs assignment (the block referencing cloud_provider,
password, pool_kwargs and AzureManagedRedisProvider) to either (a) log a clear
warning via logger.warning when running in non-local/non-debug modes that Redis
will be created without authentication, or (b) require an explicit opt-in
environment flag (e.g., ALLOW_UNAUTHENTICATED_REDIS or a check against
settings.DEBUG/ENVIRONMENT) before allowing omission of pool_kwargs['password'];
implement the chosen approach by adding a conditional guard and warning/error
log so production misconfiguration is visible and only permit unauthenticated
connections when the opt-in flag is present.
Comment on lines
+112
to
+118
| if cloud_provider == 'azure' and not password: | ||
| logger.info( | ||
| 'Configuring Azure Entra ID (Workload Identity) authentication' | ||
| ) | ||
| pool_kwargs['credential_provider'] = AzureManagedRedisProvider() | ||
| elif password: | ||
| pool_kwargs['password'] = password |
There was a problem hiding this comment.
Silent fallback to unauthenticated Redis when credentials are misconfigured.
Same concern as the call_processing module - when CLOUD_PROVIDER is not azure and REDIS_PASSWORD is unset, no authentication is configured. A warning log would help surface misconfigurations.
🛡️ Suggested improvement
if cloud_provider == 'azure' and not password:
logger.info(
'Configuring Azure Entra ID (Workload Identity) authentication'
)
pool_kwargs['credential_provider'] = AzureManagedRedisProvider()
elif password:
pool_kwargs['password'] = password
+ else:
+ logger.warning(
+ 'No Redis authentication configured. '
+ 'Set REDIS_PASSWORD or use CLOUD_PROVIDER=azure for managed identity.'
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if cloud_provider == 'azure' and not password: | |
| logger.info( | |
| 'Configuring Azure Entra ID (Workload Identity) authentication' | |
| ) | |
| pool_kwargs['credential_provider'] = AzureManagedRedisProvider() | |
| elif password: | |
| pool_kwargs['password'] = password | |
| if cloud_provider == 'azure' and not password: | |
| logger.info( | |
| 'Configuring Azure Entra ID (Workload Identity) authentication' | |
| ) | |
| pool_kwargs['credential_provider'] = AzureManagedRedisProvider() | |
| elif password: | |
| pool_kwargs['password'] = password | |
| else: | |
| logger.warning( | |
| 'No Redis authentication configured. ' | |
| 'Set REDIS_PASSWORD or use CLOUD_PROVIDER=azure for managed identity.' | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py`
around lines 112 - 118, The code silently falls back to unauthenticated Redis
when cloud_provider != 'azure' and password is falsy; update cache_manager.py to
detect this condition and emit a warning log so misconfigured REDIS_PASSWORD is
visible: inside the block handling pool_kwargs (referencing cloud_provider,
password, pool_kwargs and AzureManagedRedisProvider) add a logger.warn/info
message when password is not provided for non-azure providers, and ensure that
when cloud_provider == 'azure' we still use AzureManagedRedisProvider as before;
keep existing behavior but surface the missing credentials via a clear log
message.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py (1)
93-96: Add a comment explaining the port 10000 heuristic.Same as the other file—the magic number
10000for Azure Managed Redis's default TLS port should be documented.📝 Suggested documentation
connection_class = Connection + # Azure Managed Redis (AMR) uses port 10000 by default and requires TLS if protocol == 'rediss' or port == 10000: logger.info(f'Using SSLConnection for Redis (Port: {port})') connection_class = SSLConnection🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py` around lines 93 - 96, The code sets connection_class to SSLConnection when protocol == 'rediss' or port == 10000 but lacks explanation; add an inline comment next to the conditional (the block using variables protocol, port and symbols Connection, SSLConnection, connection_class) that documents the port 10000 heuristic as Azure Managed Redis's default TLS port so future readers understand why that magic number implies SSL/TLS.wavefront/server/apps/call_processing/call_processing/cache/cache_manager.py (1)
86-89: Add a comment explaining the port 10000 heuristic.The magic number
10000refers to Azure Managed Redis's default TLS port, but this isn't obvious to readers unfamiliar with AMR.📝 Suggested documentation
connection_class = Connection + # Azure Managed Redis (AMR) uses port 10000 by default and requires TLS if protocol == 'rediss' or port == 10000: logger.info(f'Using SSLConnection for Redis (Port: {port})') connection_class = SSLConnection🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wavefront/server/apps/call_processing/call_processing/cache/cache_manager.py` around lines 86 - 89, The conditional that switches connection_class to SSLConnection when protocol == 'rediss' or port == 10000 uses a magic number; add an inline comment above or beside that conditional explaining that port 10000 is the default TLS port used by Azure Managed Redis (AMR), so the check covers AMR instances that present TLS on port 10000; reference the variables/properties used (protocol, port) and the symbols Connection and SSLConnection so future readers understand why the heuristic exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@wavefront/server/apps/call_processing/call_processing/cache/cache_manager.py`:
- Around line 86-89: The conditional that switches connection_class to
SSLConnection when protocol == 'rediss' or port == 10000 uses a magic number;
add an inline comment above or beside that conditional explaining that port
10000 is the default TLS port used by Azure Managed Redis (AMR), so the check
covers AMR instances that present TLS on port 10000; reference the
variables/properties used (protocol, port) and the symbols Connection and
SSLConnection so future readers understand why the heuristic exists.
In
`@wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py`:
- Around line 93-96: The code sets connection_class to SSLConnection when
protocol == 'rediss' or port == 10000 but lacks explanation; add an inline
comment next to the conditional (the block using variables protocol, port and
symbols Connection, SSLConnection, connection_class) that documents the port
10000 heuristic as Azure Managed Redis's default TLS port so future readers
understand why that magic number implies SSL/TLS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8cc659a9-7546-48c3-b8dd-655dc2e8d81d
📒 Files selected for processing (2)
wavefront/server/apps/call_processing/call_processing/cache/cache_manager.pywavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Dependencies