Connect: Respect sslmode URI parameter
#408
Conversation
|
Caution Review failedThe pull request is closed. """ WalkthroughThis change updates the CrateDB toolkit to properly handle the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DatabaseAddress
participant Client
User->>DatabaseAddress: Provide SQLAlchemy connection URL
DatabaseAddress->>DatabaseAddress: Parse URL, extract ssl/sslmode
DatabaseAddress->>DatabaseAddress: Determine scheme (http/https) and verify_ssl
User->>Client: Call connect with httpuri, username, password, schema, verify_ssl_cert
Client->>Database: Establish connection with appropriate SSL settings
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🪧 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 (
|
amotl
force-pushed
the
url-respect-sslmode
branch
from
58d012e to
835185e
Compare
![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
🔭 Outside diff range comments (1)
cratedb_toolkit/model.py (1)
10-72: 💡 Verification agent❓ Verification inconclusive
Improve test coverage for new SSL functionality.
According to the static analysis, the changes to the SSL handling logic lack sufficient test coverage. Consider adding tests for different
sslmodevalues and their effects on both the URI scheme and theverify_sslproperty.
🏁 Script executed:
#!/bin/bash # Check test coverage of the verify_ssl property and various sslmode values find tests/ -name "test_*.py" -type f -exec grep -l "verify_ssl" {} \; find tests/ -name "test_*.py" -type f -exec grep -l "sslmode" {} \;Length of output: 144
Action Required: Enhance test coverage for SSL functionality.
It appears there are no tests verifying the updated SSL logic. Please add tests to cover:
- Different
sslmodevalues (e.g.,"disable","allow","prefer","require","verify-ca","verify-full") and verify how they affect the transformation of the URI scheme in thehttpuriproperty.- The behavior of the
verify_sslproperty to ensure it returns the expected boolean under various SSL configurations.Adding these tests will help prevent regressions in SSL handling.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 10-10: cratedb_toolkit/model.py#L10
Added line #L10 was not covered by tests
[warning] 54-55: cratedb_toolkit/model.py#L54-L55
Added lines #L54 - L55 were not covered by tests
[warning] 57-58: cratedb_toolkit/model.py#L57-L58
Added lines #L57 - L58 were not covered by tests
[warning] 65-66: cratedb_toolkit/model.py#L65-L66
Added lines #L65 - L66 were not covered by tests
[warning] 69-71: cratedb_toolkit/model.py#L69-L71
Added lines #L69 - L71 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGES.md(1 hunks)cratedb_toolkit/cfr/jobstats.py(1 hunks)cratedb_toolkit/model.py(2 hunks)tests/test_model.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CHANGES.md
🧰 Additional context used
🧬 Code Graph Analysis (2)
cratedb_toolkit/cfr/jobstats.py (1)
cratedb_toolkit/model.py (5)
httpuri(48-67)username(96-100)password(103-107)schema(110-114)verify_ssl(70-71)
cratedb_toolkit/model.py (1)
cratedb_toolkit/util/data.py (1)
asbool(25-36)
🪛 GitHub Check: codecov/patch
cratedb_toolkit/cfr/jobstats.py
[warning] 57-57: cratedb_toolkit/cfr/jobstats.py#L57
Added line #L57 was not covered by tests
cratedb_toolkit/model.py
[warning] 10-10: cratedb_toolkit/model.py#L10
Added line #L10 was not covered by tests
[warning] 54-55: cratedb_toolkit/model.py#L54-L55
Added lines #L54 - L55 were not covered by tests
[warning] 57-58: cratedb_toolkit/model.py#L57-L58
Added lines #L57 - L58 were not covered by tests
[warning] 65-66: cratedb_toolkit/model.py#L65-L66
Added lines #L65 - L66 were not covered by tests
[warning] 69-71: cratedb_toolkit/model.py#L69-L71
Added lines #L69 - L71 were not covered by tests
🔇 Additional comments (9)
cratedb_toolkit/model.py (4)
10-10: Import added for SSL parameter handling.The addition of
asboolis necessary for properly parsing string-based SSL parameter values into booleans, supporting the enhanced SSL handling logic in thehttpuriproperty.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 10-10: cratedb_toolkit/model.py#L10
Added line #L10 was not covered by tests
54-56: Default port explicitly set for HTTP URIs.Setting a default port of 4200 when none is specified ensures consistency in the generated HTTP URIs and aligns with CrateDB's default HTTP port. This improves reliability when connecting to CrateDB instances.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 54-55: cratedb_toolkit/model.py#L54-L55
Added lines #L54 - L55 were not covered by tests
57-67: Enhanced SSL handling based on bothsslandsslmodeparameters.This implementation correctly respects both
sslandsslmodeparameters from the connection URL, determining when to use HTTPS based on standard PostgreSQL SSL modes. The code properly handles all PostgreSQL SSL modes including "allow", "prefer", "require", "verify-ca", and "verify-full".🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 57-58: cratedb_toolkit/model.py#L57-L58
Added lines #L57 - L58 were not covered by tests
[warning] 65-66: cratedb_toolkit/model.py#L65-L66
Added lines #L65 - L66 were not covered by tests
69-72: New property added to control SSL certificate verification.The
verify_sslproperty provides a clean interface for determining whether SSL certificate validation should be performed, based on thesslmodeparameter value. This correctly follows PostgreSQL's SSL mode behavior, where "disable" and "require" modes do not verify certificates.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 69-71: cratedb_toolkit/model.py#L69-L71
Added lines #L69 - L71 were not covered by testscratedb_toolkit/cfr/jobstats.py (1)
57-63: Connection setup now respects SSL certificate verification settings.The
client.connect()call has been updated to pass theverify_ssl_certparameter based on theaddress.verify_sslproperty. This ensures the connection respects the SSL verification settings derived from thesslmodeparameter in the connection URL.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 57-57: cratedb_toolkit/cfr/jobstats.py#L57
Added line #L57 was not covered by teststests/test_model.py (4)
4-6: Test renamed to clarify it verifies default port behavior.Test renamed to explicitly indicate it's checking the default port behavior with a standard (non-SSL) connection. The assertion correctly verifies that port 4200 is added to the URI when no port is specified.
9-12: New test added for explicit port with standard connection.This test ensures that when an explicit port is provided in the connection string, it's preserved in the resulting HTTP URI rather than being replaced with the default port.
14-17: Test renamed and updated to verify SSL behavior with default port.The test now explicitly verifies that both the scheme is changed to HTTPS and the default port (4200) is added when an SSL connection is requested without specifying a port.
19-22: New test added for explicit port with SSL connection.This test ensures that when an explicit port is provided with an SSL connection, both the HTTPS scheme is used and the specified port is preserved in the resulting URI.
... when converting SQLAlchemy connection URLs to `http(s)://`.
amotl
force-pushed
the
url-respect-sslmode
branch
from
835185e to
7e8abf4
Compare
Problem
The new
?sslmode=requireparameter with theCRATEDB_SQLALCHEMY_URLenvironment variable wasn't respected by the CFR job statistics subsystem.Solution
Respect
sslmodeURI parameter when converting SQLAlchemy connection URLs tohttp(s)://.