Skip to content
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.

Sign up for GitHub

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

Jump to bottom

PROD-2654 - MySQL on RDS as a detection/discovery source #5275

Merged
merged 20 commits into from
Sep 25, 2024
Merged

PROD-2654 - MySQL on RDS as a detection/discovery source #5275

merged 20 commits into from
Sep 25, 2024

Conversation

andres-torres-marroquin
Copy link
Contributor

@andres-torres-marroquin andres-torres-marroquin commented Sep 10, 2024 •
edited
Loading

Closes #PROD-2654

Description Of Changes

Added model for MySQL on RDS.

Code Changes

  • Added connection_secrets_rds_mysql.py
  • Added migration for the new type on connection

Steps to Confirm

  • list any manual steps for reviewers to confirm the changes

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Documentation:
    • documentation complete, PR opened in fidesdocs
    • documentation issue created in fidesdocs
    • if there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md
  • For API changes, the Postman collection has been updated
  • If there are any database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!

@vercel Vercel
Copy link

vercel bot commented Sep 10, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
fides-plus-nightly ⬜️ Ignored (Inspect) Visit Preview Sep 25, 2024 1:13pm

@cypress Cypress
Copy link

cypress bot commented Sep 10, 2024 •
edited
Loading

fides    Run #10134

Run Properties:  status check passed Passed #10134  •  git commit 4675601428 ℹ️: Merge f43775a1355a049a8a2c86d5b661789bdd3c0556 into 0e2db7c28dbae0b21812a789edf6...
Project fides
Branch Review refs/pull/5275/merge
Run status status check passed Passed #10134
Run duration 00m 38s
Commit git commit 4675601428 ℹ️: Merge f43775a1355a049a8a2c86d5b661789bdd3c0556 into 0e2db7c28dbae0b21812a789edf6...
Committer Andres Torres
View all properties for this run ↗︎
Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 4
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.
View all changes introduced in this branch ↗︎

@andres-torres-marroquin andres-torres-marroquin marked this pull request as ready for review September 20, 2024 17:10
erosselli
erosselli reviewed Sep 20, 2024
View reviewed changes
Copy link
Contributor

@erosselli erosselli left a comment

Choose a reason for hiding this comment

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

Also maybe we can add a test on tests/ops/api/v1/endpoints/test_connection_template_endpoints.py::TestGetConnectionSecretSchema to test for the added schema

erosselli
erosselli approved these changes Sep 20, 2024
View reviewed changes
Copy link
Contributor

@erosselli erosselli left a comment

Choose a reason for hiding this comment

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

LGTM!

adamsachs
adamsachs requested changes Sep 20, 2024
View reviewed changes
Copy link
Contributor

@adamsachs adamsachs left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@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

adamsachs
adamsachs reviewed Sep 20, 2024
View reviewed changes
src/fides/api/schemas/connection_configuration/connection_secrets_rds_mysql.py Outdated
title="Region",
description="The AWS region where the RDS instances are located.",
)
ca_cert_url: str = Field(
Copy link
Contributor

Choose a reason for hiding this comment

The 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 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 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I confirmed with Michael that they use the standard certificate bundle.

adamsachs
adamsachs approved these changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

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

👍

src/fides/api/schemas/connection_configuration/connection_secrets_rds_mysql.py Outdated
"""

username: str = Field(
default="fides_explorer",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: fides_explorer feels a bit arbitrary/random to me as a default, we don't use "explorer" anywhere else in the product and it's not really a standard. I'd probably just stick with fides or fides_service_user or fides_db_user or something like that

src/fides/api/schemas/connection_configuration/connection_secrets_rds_mysql.py Outdated
Schema to validate the secrets needed to connect to a RDS MySQL Database
"""

username: str = Field(
Copy link
Contributor

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

@andres-torres-marroquin andres-torres-marroquin merged commit 019dad1 into main Sep 25, 2024
20 checks passed
adamsachs
adamsachs reviewed Sep 25, 2024
View reviewed changes
src/fides/api/schemas/connection_configuration/connection_secrets_rds_mysql.py
Schema to validate the secrets needed to connect to a RDS MySQL Database
"""

db_sername: str = Field(
Copy link
Contributor

Choose a reason for hiding this comment

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

@andres-torres-marroquin looks like a typo here. should read db_username

@cypress Cypress
Copy link

cypress bot commented Sep 25, 2024

fides    Run #10135

Run Properties:  status check passed Passed #10135  •  git commit 019dad1612: PROD-2654 - MySQL on RDS as a detection/discovery source (#5275)
Project fides
Branch Review main
Run status status check passed Passed #10135
Run duration 00m 40s
Commit git commit 019dad1612: PROD-2654 - MySQL on RDS as a detection/discovery source (#5275)
Committer Andres Torres
View all properties for this run ↗︎
Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 4
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.
View all changes introduced in this branch ↗︎

Kelsey-Ethyca pushed a commit that referenced this pull request Oct 7, 2024
@andres-torres-marroquin @Kelsey-Ethyca
PROD-2654 - MySQL on RDS as a detection/discovery source (#5275)
b622b17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamsachs adamsachs adamsachs approved these changes

@erosselli erosselli erosselli approved these changes

@galvana galvana Awaiting requested review from galvana

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andres-torres-marroquin @adamsachs @erosselli