-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PROD-2654 - MySQL on RDS as a detection/discovery source #5275
Changes from 11 commits
70ef665
209fe1e
791df83
020364a
b4735ec
9cf5dcc
146f939
a3a5b5a
8dcb4b5
4eea417
3cc3b09
1edaf55
353dbad
bdad19f
d588db2
a7f9b57
83653ba
c7ba64e
afec854
f43775a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,102 @@ | ||
"""add_rds_mysql_to_connector_type | ||
|
||
Revision ID: 25fe48d56eaa | ||
Revises: 9de4bb76307a | ||
Create Date: 2024-09-20 17:06:31.944225 | ||
|
||
""" | ||
|
||
import sqlalchemy as sa | ||
from alembic import op | ||
|
||
# revision identifiers, used by Alembic. | ||
revision = "25fe48d56eaa" | ||
down_revision = "9de4bb76307a" | ||
branch_labels = None | ||
depends_on = None | ||
|
||
|
||
def upgrade(): | ||
# Add 'rds_mysql' to ConnectionType enum | ||
op.execute("ALTER TYPE connectiontype RENAME TO connectiontype_old") | ||
op.execute( | ||
""" | ||
CREATE TYPE connectiontype AS ENUM ( | ||
'mongodb', | ||
'mysql', | ||
'https', | ||
'snowflake', | ||
'redshift', | ||
'mssql', | ||
'mariadb', | ||
'bigquery', | ||
'saas', | ||
'manual', | ||
'manual_webhook', | ||
'timescale', | ||
'fides', | ||
'sovrn', | ||
'attentive', | ||
'dynamodb', | ||
'postgres', | ||
'generic_consent_email', | ||
'generic_erasure_email', | ||
'scylla', | ||
's3', | ||
'google_cloud_sql_mysql', | ||
'google_cloud_sql_postgres', | ||
'dynamic_erasure_email', | ||
'rds_mysql' | ||
) | ||
""" | ||
) | ||
op.execute( | ||
""" | ||
ALTER TABLE connectionconfig ALTER COLUMN connection_type TYPE connectiontype USING | ||
connection_type::text::connectiontype | ||
""" | ||
) | ||
op.execute("DROP TYPE connectiontype_old") | ||
|
||
|
||
def downgrade(): | ||
# Remove 'rds_mysql' from ConnectionType enum | ||
op.execute("DELETE FROM connectionconfig WHERE connection_type IN ('rds_mysql')") | ||
op.execute("ALTER TYPE connectiontype RENAME TO connectiontype_old") | ||
op.execute( | ||
""" | ||
CREATE TYPE connectiontype AS ENUM ( | ||
'mongodb', | ||
'mysql', | ||
'https', | ||
'snowflake', | ||
'redshift', | ||
'mssql', | ||
'mariadb', | ||
'bigquery', | ||
'saas', | ||
'manual', | ||
'manual_webhook', | ||
'timescale', | ||
'fides', | ||
'sovrn', | ||
'attentive', | ||
'dynamodb', | ||
'postgres', | ||
'generic_consent_email', | ||
'generic_erasure_email', | ||
'scylla', | ||
's3', | ||
'google_cloud_sql_mysql', | ||
'google_cloud_sql_postgres', | ||
'dynamic_erasure_email' | ||
) | ||
""" | ||
) | ||
op.execute( | ||
""" | ||
ALTER TABLE connectionconfig ALTER COLUMN connection_type TYPE connectiontype USING | ||
connection_type::text::connectiontype | ||
""" | ||
) | ||
op.execute("DROP TYPE connectiontype_old") |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
from pydantic import Field | ||
|
||
from fides.api.schemas.base_class import NoValidationSchema | ||
from fides.api.schemas.connection_configuration.connection_secrets_base_aws import ( | ||
BaseAWSSchema, | ||
) | ||
|
||
|
||
class RDSMySQLSchema(BaseAWSSchema): | ||
""" | ||
Schema to validate the secrets needed to connect to a RDS MySQL Database | ||
""" | ||
|
||
username: str = Field( | ||
default="fides_explorer", | ||
andres-torres-marroquin marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: |
||
title="Username", | ||
description="The user account used to authenticate and access the databases.", | ||
) | ||
region: str = Field( | ||
title="Region", | ||
description="The AWS region where the RDS instances are located.", | ||
) | ||
ca_cert_url: str = Field( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. knowing what i know about how this field is used downstream in the monitor, and the fact that it's user-input data - i think we need some more validation on this field. at a minimum, we should be using the additionally, we should probably perform some more checks on the URL before we fetch it and save it to the filesystem downstream. i'm not exactly sure the best way to check that, but it feels like it'd be wise... if you feel like it makes more sense to just hardcode the default value here rather than accept user config for it, that may be OK. i know i'd vouched for a configurable field earlier, since i thought the flexibility would be a good fallback in case a customer isn't using this global AWS cert that we expect, but i hadn't really considered the security implications. sorry to go back and forth on this! i still think the flexibility would be nice, but we need to be able to do it in a reasonably secure way, so let's weigh that accordingly. if you do decide to remove the flexibility, i think it would be good for us to confirm with our prospective customers of this feature whether the global AWS cert we're defaulting to is one that would work for their use case 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I confirmed with Michael that they use the standard certificate bundle. |
||
default="https://truststore.pki.rds.amazonaws.com/global/global-bundle.pem", | ||
title="CA Certificate URL", | ||
description="The URL to the CA certificate used to authenticate the RDS instances.", | ||
) | ||
|
||
|
||
class RDSMySQLDocsSchema(RDSMySQLSchema, NoValidationSchema): | ||
"""RDS MySQL Secrets Schema for API Docs""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I might rename this
db_username
to clarify it's not an AWS IAM username