Extending read-only DB configuration #7139
Conversation
|
Deployment failed with the following error: View Documentation: https://vercel.com/docs/two-factor-authentication |
![github-advanced-security[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Greptile OverviewGreptile SummaryThis PR extends read-only database configuration support by adding new fields for readonly database credentials and creating both synchronous and asynchronous readonly connection strings. Major changes:
Critical issues found:
Confidence Score: 2/5
Important Files ChangedFile Analysis
|
![greptile-apps[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
tests/ctl/core/config/test_config.py
Outdated
| assert "replica-db.example.com" in db_settings.sqlalchemy_readonly_database_uri | ||
| assert "readonly_user" in db_settings.sqlalchemy_readonly_database_uri | ||
| assert "5433" in db_settings.sqlalchemy_readonly_database_uri | ||
| assert "fides_replica" in db_settings.sqlalchemy_readonly_database_uri | ||
| assert "postgresql+psycopg2" in db_settings.sqlalchemy_readonly_database_uri |
There was a problem hiding this comment.
syntax: Vulnerable URL validation using substring matching
| assert "replica-db.example.com" in db_settings.sqlalchemy_readonly_database_uri | |
| assert "readonly_user" in db_settings.sqlalchemy_readonly_database_uri | |
| assert "5433" in db_settings.sqlalchemy_readonly_database_uri | |
| assert "fides_replica" in db_settings.sqlalchemy_readonly_database_uri | |
| assert "postgresql+psycopg2" in db_settings.sqlalchemy_readonly_database_uri | |
| from urllib.parse import urlparse | |
| parsed = urlparse(db_settings.sqlalchemy_readonly_database_uri) | |
| assert parsed.hostname == "replica-db.example.com" | |
| assert parsed.username == "readonly_user" | |
| assert parsed.port == 5433 | |
| assert parsed.path == "/fides_replica" | |
| assert parsed.scheme == "postgresql+psycopg2" |
Context Used: Rule from dashboard - When writing tests that validate URL properties, always parse the URL into components and assert on ... (source)
tests/ctl/core/config/test_config.py
Outdated
| assert "replica-db.example.com" in db_settings.async_readonly_database_uri | ||
| assert "readonly_user" in db_settings.async_readonly_database_uri | ||
| assert "5433" in db_settings.async_readonly_database_uri | ||
| assert "fides_replica" in db_settings.async_readonly_database_uri | ||
| assert "postgresql+asyncpg" in db_settings.async_readonly_database_uri |
There was a problem hiding this comment.
syntax: Vulnerable URL validation using substring matching
| assert "replica-db.example.com" in db_settings.async_readonly_database_uri | |
| assert "readonly_user" in db_settings.async_readonly_database_uri | |
| assert "5433" in db_settings.async_readonly_database_uri | |
| assert "fides_replica" in db_settings.async_readonly_database_uri | |
| assert "postgresql+asyncpg" in db_settings.async_readonly_database_uri | |
| parsed_async = urlparse(db_settings.async_readonly_database_uri) | |
| assert parsed_async.hostname == "replica-db.example.com" | |
| assert parsed_async.username == "readonly_user" | |
| assert parsed_async.port == 5433 | |
| assert parsed_async.path == "/fides_replica" | |
| assert parsed_async.scheme == "postgresql+asyncpg" |
Context Used: Rule from dashboard - When writing tests that validate URL properties, always parse the URL into components and assert on ... (source)
tests/ctl/core/config/test_config.py
Outdated
| assert "replica-db.example.com" in db_settings.sqlalchemy_readonly_database_uri | ||
| assert "app_user" in db_settings.sqlalchemy_readonly_database_uri | ||
| assert "5432" in db_settings.sqlalchemy_readonly_database_uri | ||
| assert "fides" in db_settings.sqlalchemy_readonly_database_uri |
There was a problem hiding this comment.
syntax: Vulnerable URL validation using substring matching
| assert "replica-db.example.com" in db_settings.sqlalchemy_readonly_database_uri | |
| assert "app_user" in db_settings.sqlalchemy_readonly_database_uri | |
| assert "5432" in db_settings.sqlalchemy_readonly_database_uri | |
| assert "fides" in db_settings.sqlalchemy_readonly_database_uri | |
| parsed = urlparse(db_settings.sqlalchemy_readonly_database_uri) | |
| assert parsed.hostname == "replica-db.example.com" | |
| assert parsed.username == "app_user" | |
| assert parsed.port == 5432 | |
| assert parsed.path == "/fides" |
Context Used: Rule from dashboard - When writing tests that validate URL properties, always parse the URL into components and assert on ... (source)
tests/ctl/core/config/test_config.py
Outdated
| # sslmode should be converted to ssl in query params | ||
| assert "ssl=" in db_settings.async_readonly_database_uri | ||
| # sslrootcert should be removed from query params | ||
| assert "sslrootcert" not in db_settings.async_readonly_database_uri |
There was a problem hiding this comment.
syntax: Vulnerable URL validation using substring matching
| # sslmode should be converted to ssl in query params | |
| assert "ssl=" in db_settings.async_readonly_database_uri | |
| # sslrootcert should be removed from query params | |
| assert "sslrootcert" not in db_settings.async_readonly_database_uri | |
| from urllib.parse import parse_qs | |
| parsed = urlparse(db_settings.async_readonly_database_uri) | |
| query_params = parse_qs(parsed.query) | |
| assert query_params.get("ssl") == ["require"] | |
| assert "sslrootcert" not in query_params |
Context Used: Rule from dashboard - When writing tests that validate URL properties, always parse the URL into components and assert on ... (source)
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (61.76%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #7139 +/- ##
==========================================
- Coverage 87.30% 87.26% -0.05%
==========================================
Files 533 533
Lines 35095 35161 +66
Branches 4067 4083 +16
==========================================
+ Hits 30640 30683 +43
- Misses 3570 3591 +21
- Partials 885 887 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
erosselli
left a comment
erosselli
left a comment
There was a problem hiding this comment.
Approved with a question! Also let's add a changelog
| scheme="postgresql+psycopg2", | ||
| username=info.data.get("readonly_user"), | ||
| password=info.data.get("readonly_password"), |
There was a problem hiding this comment.
just checking, the field validators defined above will run before this one, so if I don't provide readonly_user then it will already be populated with user by the time we get here?
Ticket ENG-1811
Description Of Changes
Adds support for configuring a read-only database replica with optional credentials. When configured, applications can route read operations to a replica to improve performance and separate read/write traffic.
Code Changes
Configuration (
fides/src/fides/config/database_settings.py):readonly_user,readonly_password,readonly_port,readonly_db,readonly_paramsasync_readonly_database_uricomputed fieldassemble_readonly_db_connectionto use new fields and proper scheme (postgresql+psycopg2)assemble_async_readonly_db_connectionwith asyncpg SSL handlingSession Factories:
readonly_async_sessioninfides/src/fides/api/db/ctl_session.py(module-level, async)get_readonly_autoclose_db_session()infides/src/fides/api/api/deps.py(context manager, sync)Tests (
fides/tests/ctl/core/config/test_config.py):TestReadOnlyDatabaseConfigclass testing URI construction, credential fallback, and SSL parameter handlingConfiguration Example:
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works