Add tls support for from_url #74
Conversation
WalkthroughThe changes in the pull request modify the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FalkorDB
participant ConnectionPool
participant Redis
Client->>FalkorDB: from_url(url, **kwargs)
FalkorDB->>ConnectionPool: retrieve connection kwargs
ConnectionPool-->>FalkorDB: return connection kwargs
FalkorDB->>Redis: check connection_class
alt If connection_class is SSLConnection
FalkorDB->>FalkorDB: set ssl=True
end
FalkorDB-->>Client: return FalkorDB instance
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #74 +/- ##
==========================================
- Coverage 92.29% 92.26% -0.03%
==========================================
Files 35 35
Lines 2610 2613 +3
==========================================
+ Hits 2409 2411 +2
- Misses 201 202 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
falkordb/falkordb.py (1)
174-178: Add URL scheme validationThe URL transformation logic should validate the input URL scheme. Currently, URLs with unsupported schemes would silently pass through to redis.from_url without transformation.
Consider adding scheme validation:
+ supported_schemes = ['falkor://', 'falkors://', 'redis://', 'rediss://', 'unix://'] + if not any(url.startswith(scheme) for scheme in supported_schemes): + raise ValueError(f"Unsupported URL scheme. Must start with one of: {', '.join(supported_schemes)}") + # switch from redis:// to falkordb:// if url.startswith("falkor://"): url = "redis://" + url[len("falkor://") :] elif url.startswith("falkors://"): url = "rediss://" + url[len("falkors://") :]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
falkordb/falkordb.py(1 hunks)
🔇 Additional comments (1)
falkordb/falkordb.py (1)
174-180: Verify SSL configuration inheritance
Let's verify that SSL configurations are correctly inherited from the URL and that there are no security regressions.
✅ Verification successful
SSL configuration inheritance is properly implemented
The code correctly handles SSL configuration in the following ways:
- Properly detects Redis SSL connections and sets
ssl=Truewhenconnection_classisredis.SSLConnection - Comprehensively supports all standard SSL parameters including certificates, keys, and validation options
- Correctly propagates SSL settings through different connection types (standard, sentinel, cluster)
- Default SSL configuration is secure with
ssl_cert_reqs="required"and proper certificate validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper SSL configuration handling
# Look for SSL-related configuration handling
rg -A 5 "SSLConnection|ssl=|ssl_" --type python
# Check for any security-related issues in SSL handling
ast-grep --pattern 'ssl = False'
# Look for existing SSL connection tests
fd -e py test_ --exec rg -l "ssl.*=.*True|SSLConnection"
Length of output: 174
Script:
#!/bin/bash
# Let's try different approaches to verify SSL configuration
# Search for SSL-related code without file type specification
rg -A 5 "SSLConnection|ssl=|ssl_"
# Search for SSL-related configuration in Python files using file extension
fd -e py -x rg -A 5 "SSLConnection|ssl=|ssl_" {}
# Look for Redis connection class usage
fd -e py -x rg -A 5 "connection_class|Connection" {}
# Check for SSL/TLS related test files
fd -e py -x grep -l "ssl" {}
Length of output: 17144
| if connection_class is redis.SSLConnection: | ||
| kwargs["ssl"] = True |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance SSL parameter propagation
While setting ssl=True for SSL connections is correct, other SSL-related parameters from the URL should also be propagated to maintain the SSL configuration.
Consider propagating all SSL parameters:
if connection_class is redis.SSLConnection:
kwargs["ssl"] = True
+ # Propagate SSL-specific parameters if present
+ ssl_params = [
+ "ssl_keyfile", "ssl_certfile", "ssl_cert_reqs",
+ "ssl_ca_certs", "ssl_ca_path", "ssl_ca_data",
+ "ssl_check_hostname", "ssl_password"
+ ]
+ for param in ssl_params:
+ if param in connection_kwargs:
+ kwargs[param] = connection_kwargs[param]Also, consider adding a warning if SSL parameters are provided in both the URL and kwargs to avoid potential conflicts.
📝 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.
| if connection_class is redis.SSLConnection: | |
| kwargs["ssl"] = True | |
| if connection_class is redis.SSLConnection: | |
| kwargs["ssl"] = True | |
| # Propagate SSL-specific parameters if present | |
| ssl_params = [ | |
| "ssl_keyfile", "ssl_certfile", "ssl_cert_reqs", | |
| "ssl_ca_certs", "ssl_ca_path", "ssl_ca_data", | |
| "ssl_check_hostname", "ssl_password" | |
| ] | |
| for param in ssl_params: | |
| if param in connection_kwargs: | |
| kwargs[param] = connection_kwargs[param] |
fix #73
Summary by CodeRabbit
New Features
FalkorDBinstance from a URL.Bug Fixes
from_urlmethod to ensure proper SSL configuration based on the connection class.