PROD-2654 - MySQL on RDS as a detection/discovery source #5275
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
fides
|
||||||||||||||||||||||||||||||
| Project |
fides
|
| Branch Review |
refs/pull/5275/merge
|
| Run status |
|
| Run duration | 00m 37s |
| Commit |
|
| Committer | Andres Torres |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
4
|
Upgrade your plan to view test results. | |
| View all changes introduced in this branch ↗︎ | |
src/fides/api/alembic/migrations/versions/25fe48d56eaa_add_rds_mysql_to_connector_type.py
Outdated
Show resolved
Hide resolved
src/fides/api/alembic/migrations/versions/25fe48d56eaa_add_rds_mysql_to_connector_type.py
Outdated
Show resolved
Hide resolved
| """ | ||
|
|
||
| username: str = Field( | ||
| default="fides_explorer", |
There was a problem hiding this comment.
Looking at connection secrets for other integrations, we don't set defaults for username fields... in what way is this connection different?
There was a problem hiding this comment.
In this case fides_explorer is the user that the customers should set up on their databases to give us access.
There was a problem hiding this comment.
so I'm still a bit confused (perhaps just from the lack of context I have on the feature 😅 ) -- if the username is not named fides_explorer then this won't work? if so why is it configurable?
There was a problem hiding this comment.
if they want to set a different username instead of fides_explorer, they can customize it here. but they have to set the exact username here and on their dbs, does that make sense?
src/fides/api/schemas/connection_configuration/connection_secrets_rds_mysql.py
Outdated
Show resolved
Hide resolved
src/fides/api/schemas/connection_configuration/connection_secrets_rds_mysql.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@andres-torres-marroquin i think we at least need a bit of a stronger type on the cert URL property here, and may need to think a bit more about the security implications - see my comment below
| title="Region", | ||
| description="The AWS region where the RDS instances are located.", | ||
| ) | ||
| ca_cert_url: str = Field( |
There was a problem hiding this comment.
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 AnyHttpUrl pydantic class (or even better, using @pattisdr 's AnyHttpUrlString in `fideslang) to put some bounds on this field.
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 👍
Closes #PROD-2654
Description Of Changes
Added model for MySQL on RDS.
Code Changes
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdmaindowngrade()migration is correct and works