-
Notifications
You must be signed in to change notification settings - Fork 73
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
Add support for "Key-pair" authentication to Snowflake integration #5079
Add support for "Key-pair" authentication to Snowflake integration #5079
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
src/fides/api/schemas/connection_configuration/connection_secrets_snowflake.py
Outdated
Show resolved
Hide resolved
Passing run #9002 ↗︎
Details:
Review all test suite changes for PR #5079 ↗︎ |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5079 +/- ##
==========================================
- Coverage 86.51% 86.43% -0.08%
==========================================
Files 357 357
Lines 22310 22344 +34
Branches 2947 2952 +5
==========================================
+ Hits 19301 19313 +12
- Misses 2495 2514 +19
- Partials 514 517 +3 ☔ View full report in Codecov by Sentry. |
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.
thanks for getting a solid starting approach in place! here's a few initial comments, haven't had a chance to fully review yet.
src/fides/api/schemas/connection_configuration/connection_secrets_snowflake.py
Outdated
Show resolved
Hide resolved
src/fides/api/schemas/connection_configuration/connection_secrets_snowflake.py
Show resolved
Hide resolved
…ey-pair-authentication-to-snowflake-integration
…ey-pair-authentication-to-snowflake-integration
…ey-pair-authentication-to-snowflake-integration
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.
@andres-torres-marroquin generally looking good to me, I'd just like to get the key parsing bit sorted and removed ideally - it feels like it shouldn't be necessary, but maybe there's a nuance there I'm not aware of.
also - have you done any manual testing? we'd like to be extra sure here that this is working as we expect end to end. if you have, pasting some steps and/or screenshots would be helpful - if the screenshots are at all sensitive, feel free to paste it to the jira ticket.
src/fides/api/schemas/connection_configuration/connection_secrets_snowflake.py
Show resolved
Hide resolved
src/fides/api/schemas/connection_configuration/connection_secrets_snowflake.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Adam Sachs <adam@ethyca.com>
…ey-pair-authentication-to-snowflake-integration
…ey-pair-authentication-to-snowflake-integration
@pytest.fixture( | ||
params=[ | ||
"snowflake_connection_config", | ||
"snowflake_connection_config_with_keypair", | ||
] | ||
) |
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.
Clever!
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.
@andres-torres-marroquin and I verified both authentication methods from the Admin UI. I just called out the need for error handling around missing passphrases for encrypted private keys but we're following up with that in a separate ticket.
…5079) Co-authored-by: Adam Sachs <adam@ethyca.com>
Passing run #9003 ↗︎
Details:
Review all test suite changes for PR #5079 ↗︎ |
Closes #PROD-2140
Description Of Changes
Add support for Key-Pair authentication to the SnowflakeConnector, so it now allows a couple fields to the
SnowflakeSchema
:private_key
andprivate_key_passphrase
.Code Changes
SnowflakeSchema
:private_key
andprivate_key_passphrase
.SQLConnector
so nowconnect_args
can be passed down toSQLConnector.create_client
through the newSQLConnector.get_connect_args()
method.SnowflakeSchema.get_connect_args()
so it now takes theprivate_key
and decrypts it ifprivate_key_passphrase
was defined as well.password
mode andprivate_key
mode.Steps to Confirm
private_key
.Pre-Merge Checklist
CHANGELOG.md
main
downgrade()
migration is correct and works