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.

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

Optional SSH Support for DSR Processing #3374

Merged
merged 21 commits into from
Jun 14, 2023

Conversation

SteveDMurphy
Copy link
Contributor

@SteveDMurphy SteveDMurphy commented May 24, 2023

Closes #3329

Code Changes

  • Create the config variables for ssh config
  • Create a boolean to require ssh on specific connectors
  • Functionalize the creation of an SSH tunnel as required
  • Update the query config to use the SSH tunnel as required
  • Look at the prospect of having the generate and classify commands use the SSH tunnel in this first iteration
  • Add SSH URI for both Postgres and RedShift

Steps to Confirm

  • Set the environment variables as defined in this 1Password secure note - (these will all be exchanged with some proper test credentials)
  • If on an M1, add these commands to the Dockerfile to handle a known issue
  • nox -s dev -- shell
  • Create a fake dataset: fides user login && fides push
  • Connect to the db behind the vpc via the bastion host with these credentials, connecting to your pushed dataset
  • Validate the connection test succeeds

Pre-Merge Checklist

Description Of Changes

This is currently set up to only support a single bastion host in order to get moving more quickly. In a follow on change, we will likely create an SSH connection config type to support multiple ssh requirements.

@cypress
Copy link

cypress bot commented May 26, 2023

Passing run #2686 ↗︎

0 4 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge 5764332 into 225f211...
Project: fides Commit: 3241b01425 ℹ️
Status: Passed Duration: 00:50 💡
Started: Jun 14, 2023 3:48 PM Ended: Jun 14, 2023 3:49 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@codecov
Copy link

codecov bot commented May 30, 2023

Codecov Report

Patch coverage: 40.32% and project coverage change: -0.16 ⚠️

Comparison is base (225f211) 87.17% compared to head (5764332) 87.01%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3374      +/-   ##
==========================================
- Coverage   87.17%   87.01%   -0.16%     
==========================================
  Files         311      311              
  Lines       18802    18863      +61     
  Branches     2395     2402       +7     
==========================================
+ Hits        16390    16414      +24     
- Misses       1990     2025      +35     
- Partials      422      424       +2     
Impacted Files Coverage Δ
src/fides/api/service/connectors/sql_connector.py 67.15% <31.48%> (-8.99%) ⬇️
src/fides/api/common_exceptions.py 91.11% <100.00%> (+0.09%) ⬆️
...ction_configuration/connection_secrets_postgres.py 100.00% <100.00%> (ø)
...ction_configuration/connection_secrets_redshift.py 100.00% <100.00%> (ø)
src/fides/core/config/security_settings.py 98.94% <100.00%> (+0.05%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

I was originally intending on implementing this and using more of the `SQLConnector` Class but it turned out to be more trouble than it was worth without implementing more of the ssh set variable across other db connectors. As this will likely change again in the near future it made sense to keep this more isolated and leave the duplicate code found in the overridden `create_client` method
Copy link
Contributor Author

@SteveDMurphy SteveDMurphy left a comment

Choose a reason for hiding this comment

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

Still need to add test infrastructure, have it added to secrets, then write tests and docs

@SteveDMurphy SteveDMurphy self-assigned this Jun 2, 2023
Dockerfile Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
@seanpreston seanpreston marked this pull request as ready for review June 13, 2023 21:32
Copy link
Contributor

@seanpreston seanpreston left a comment

Choose a reason for hiding this comment

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

LGTM @SteveDMurphy — would be good to get a follow-up ticket in for some more thorough testing

@SteveDMurphy
Copy link
Contributor Author

LGTM @SteveDMurphy — would be good to get a follow-up ticket in for some more thorough testing

Manual testing looks good and added #3562 for the testing follow on - thanks @seanpreston and @RobertKeyser !!

Will merge once checks complete

@SteveDMurphy SteveDMurphy merged commit 84f7c02 into main Jun 14, 2023
@SteveDMurphy SteveDMurphy deleted the SteveDMurphy-3329-dsr-ssh-option branch June 14, 2023 16:39
SteveDMurphy added a commit that referenced this pull request Jun 14, 2023
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.

Provide Optional SSH Tunneling for Database Connections
2 participants