Skip to content

ENG-3566: Wire all DB connections through DBCredentialProvider#8176

Merged
erosselli merged 18 commits into
mainfrom
erosselli/ENG-3566-wire-credential-refresh
May 15, 2026
Merged

ENG-3566: Wire all DB connections through DBCredentialProvider#8176
erosselli merged 18 commits into
mainfrom
erosselli/ENG-3566-wire-credential-refresh

Conversation

@erosselli
Copy link
Copy Markdown
Contributor

Ticket ENG-3566

Stacked on: #8175 (must be merged first)

Description Of Changes

Wires the DBCredentialProvider (from #8175) into all database connection paths so credentials always come from the provider — no hardcoded user/password in config required for AWS Secrets Manager deployments.

Code Changes

  • engine_creators.py — module-level _db_cred_provider replaces get_db_credentials()/get_readonly_db_credentials(); creator signatures unchanged (no call site changes to ctl_session, session_management, tasks)
  • db_credential_provider.py — added get_database_url() for callers that need a connection URI
  • session.pyget_db_session() defaults to make_sync_creator() when no engine provided (fixes 6 callers that used URI-based path)
  • env.py — alembic online migrations use make_sync_creator(), offline uses get_database_url()
  • app_setup.py — startup configure_db uses provider URL
  • admin.py — migrate/reset/configure endpoints use provider URL
  • health.py — health check uses provider URL
  • test_engine_creators.py — removed tests for deleted credential helpers, added SSL context integration test

Steps to Confirm

  1. Start Fides normally (static provider) — GET /health/database returns healthy
  2. Configure AWS Secrets Manager with DB credentials — Fides starts and connects without user/password in config
  3. Run nox -s "pytest(lib)" and nox -s "pytest(misc-unit)" — all tests pass

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Updates unreleased work already in Changelog, no new entry necessary
  • UX feedback:
    • No UX review needed
  • Followup issues:
    • Followup issues created
  • Database migrations:
    • No migrations
  • Documentation:
    • No documentation updates required

erosselli and others added 7 commits May 13, 2026 15:17
Add DBCredentialProvider class that sits between SQLAlchemy engine
creators and the SecretProvider abstraction. Resolves credentials
from static config or AWS Secrets Manager, retries once on auth
failure during credential rotation, and sanitizes all connection
exceptions to prevent credential leakage.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The build_field_documentation function assumed every anyOf entry
has a "type" key, but Pydantic uses "$ref" for nested models.
Skip $ref entries to avoid KeyError.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- aws_secrets_manager is None when provider is static
- region is required (no default) when AWS is configured
- Before-validator constructs from env vars when needed
- Fix config doc generator for Optional nested model $ref

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace asserts with explicit raises for cases where upstream
validators should prevent None but callers may bypass validation.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Route all database engines, migrations, admin endpoints, and
health checks through the credential provider so credentials
always come from the SecretProvider.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@vercel
Copy link
Copy Markdown
Contributor

vercel Bot commented May 13, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Actions Updated (UTC)
fides-plus-nightly Ignored Ignored Preview May 15, 2026 1:48pm
fides-privacy-center Ignored Ignored May 15, 2026 1:48pm

Request Review

@codecov
Copy link
Copy Markdown

codecov Bot commented May 13, 2026

Codecov Report

❌ Patch coverage is 94.44444% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.61%. Comparing base (d64e6f1) to head (2afa345).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/fides/common/engine_creators.py 87.50% 1 Missing and 1 partial ⚠️
src/fides/api/app_setup.py 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8176      +/-   ##
==========================================
+ Coverage   85.59%   85.61%   +0.01%     
==========================================
  Files         658      658              
  Lines       42847    42858      +11     
  Branches     5018     5016       -2     
==========================================
+ Hits        36675    36691      +16     
+ Misses       5064     5063       -1     
+ Partials     1108     1104       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Base automatically changed from erosselli/ENG-3566-db-cred-provider to main May 14, 2026 14:02
erosselli and others added 2 commits May 14, 2026 11:03
…URL constants

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
# Conflicts:
#	src/fides/common/db_credential_provider.py
#	src/fides/config/secrets/__init__.py
#	src/fides/config/secrets/aws_secrets_manager_provider.py
#	src/fides/config/secrets_settings.py
#	tests/common/test_db_credential_provider.py
#	tests/config/secrets/test_factory.py
erosselli and others added 2 commits May 14, 2026 11:22
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

def get_db_session(
config: FidesConfig,
config: FidesConfig, # TODO: remove — no longer used, all callers pass CONFIG
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will address this in a follow-up PR -- I suspect I can also remove the sync_database_uri and async_database_uri builders in the db settings , but I want to make sure that wouldn't break anything before removing it.

erosselli and others added 4 commits May 14, 2026 12:18
- Resolve database URL once per request in admin.py
- Include CONFIG.database.params in get_database_url() for SSL support
- Rename _db_cred_provider to db_cred_provider (public API)
- Add tests for get_database_url() with special characters
- Extract shared self_signed_cert fixture for SSL tests

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
RDS Proxy may return non-standard error messages on auth failure
that don't match SQLSTATE 28P01 or "password authentication failed".
Retry on any OperationalError when using dynamic credentials.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@erosselli erosselli marked this pull request as ready for review May 14, 2026 20:13
@erosselli erosselli requested a review from a team as a code owner May 14, 2026 20:13
@erosselli erosselli requested review from JadeCara and vcruces and removed request for a team and vcruces May 14, 2026 20:13
Copy link
Copy Markdown
Contributor

@JadeCara JadeCara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only thing I saw that we might want to document or something in case it ever becomes an issue is the catch-all isinstance(exc, Psycopg2OperationalError). The docstring explicitly acknowledges the tradeoff.

The concern for dynamic-credential deployments with this is: during a DB outage, every pool connection attempt across all workers will hammer Secrets Manager simultaneously (thundering herd). With n workers × m pool_size = many concurrent invalidation calls, this could hit AWS rate limits.

@erosselli
Copy link
Copy Markdown
Contributor Author

@JadeCara re the thundering-herd issue:

The concern for dynamic-credential deployments with this is: during a DB outage, every pool connection attempt across all workers will hammer Secrets Manager simultaneously (thundering herd). With n workers × m pool_size = many concurrent invalidation calls, this could hit AWS rate limits.

The thundering-herd protection in AWSSecretsManagerProvider handles concurrent calls for the same secret within a single process (per-secret lock), though each pod will have its own lock.

The circuit breaker pattern is built to mitigate this: after the first failed refresh (credentials are fine, DB is just down), subsequent invalidate() calls within the 30s cooldown are no-ops. So the burst is bounded to one SM call per pod per 30 seconds, not per connection attempt. The cooldown period is also configurable via FIDES__SECRETS__AWS_SECRETS_MANAGER__CIRCUIT_BREAKER_COOLDOWN_SECONDS

erosselli and others added 2 commits May 15, 2026 10:43
…l_secret_name

- Region defaults to None, letting boto3 discover it from the
  environment (AWS_DEFAULT_REGION, ~/.aws/config, instance metadata)
- Rename credential_secret_id/readonly_credential_secret_id to
  credential_secret_name/readonly_credential_secret_name to match
  AWS Secrets Manager terminology

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@erosselli erosselli added this pull request to the merge queue May 15, 2026
Merged via the queue into main with commit dc05748 May 15, 2026
69 of 71 checks passed
@erosselli erosselli deleted the erosselli/ENG-3566-wire-credential-refresh branch May 15, 2026 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants