You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/08/24 13:26:17 UTC

[GitHub] [superset] xiayanzheng opened a new pull request, #21180: add snowflake private key support

xiayanzheng opened a new pull request, #21180:
URL: https://github.com/apache/superset/pull/21180

   <!---
   Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
   Example:
   fix(dashboard): load charts correctly
   -->
   
   ### SUMMARY
   <!--- Describe the change below, including rationale and design decisions -->
   We want add snowflake in superset, but our snowflake is using SSO authentication so we can't add it with built-in snowflake connector, we found we can connect it with snowflake keypair authentication. so I modified connector file to support keypair authentication. 
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   BEFORE:
      I can't add snowflake with keypair  authentication
   AFTER：
      We can add two parameters in SQLALCHEMY URI.
       
           privatekey={privatekey path on server}
           privatekeypw={privatekey password}
      Like this:
           snowflake://{user}:{password}@{account}/{database}?role={role}&warehouse={warehouse}&schema={schema}&privatekey={privatekey path on server}&privatekeypw={privatekey password}
   ![after](https://user-images.githubusercontent.com/14834951/186428260-da7397c4-2db1-48a6-b7bd-8dda6b8dc174.png)
   
   ### TESTING INSTRUCTIONS
   <!--- Required! What steps can be taken to manually verify the changes? -->
   1. Follow this [document](https://docs.snowflake.com/en/user-guide/key-pair-auth.html) to create and bind keypair to snowflake
   2. Upload private key file(.p8 file) to server(ie. /app/rsa_key.p8)
   3. In Superset web page, add database, select "Other" option in SUPPORTED DATABASES dropdown.
   4. Add SQLALCHEMY URI with following format.
   snowflake://{user}:{password}@{account}/{database}?role={role}&warehouse={warehouse}&schema={schema}&privatekey={privatekey path on server}&privatekeypw={privatekey password}
   5. Click connect
   
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [x] Has associated issue: https://github.com/apache/superset/discussions/21107
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [x] Introduces new feature or API
   - [ ] Removes existing feature or API
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] xiayanzheng commented on pull request #21180: add snowflake private key support

Posted by GitBox <gi...@apache.org>.
xiayanzheng commented on PR #21180:
URL: https://github.com/apache/superset/pull/21180#issuecomment-1229175498

   added snowflake dependencies, please rerun pipeline.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] mebegu commented on pull request #21180: feat: add snowflake private key support

Posted by GitBox <gi...@apache.org>.
mebegu commented on PR #21180:
URL: https://github.com/apache/superset/pull/21180#issuecomment-1231480031

   Thanks for quick response. Honestly, I also think it can be improved. I was spending time on how can it be added, and I have some pseudo implementation. My idea was to take the key path and the passphrase in the db engine extra parameters.
   
   But I have not contributed superset before, so, there is an overhead time for me to understand how to setup the local development environment, and testing etc. 
   
   ```
   <new function in superset/db_engine_specs/snowflake.py>
   ....
       @staticmethod
       def get_extra_params(database: "Database") -> Dict[str, Any]:
           """
           Loads private key indicated in the `connect_args`.
   
           :param database: database instance from which to extract extras
           :raises SupersetException: If database extra json payload is unparseable
           """
           
           try:
               extra = json.loads(database.extra or "{}")
           except json.JSONDecodeError as ex:
               raise SupersetException("Unable to parse database extras") from ex
   
           engine_params = extra.get("engine_params", {})
           connect_args = engine_params.get("connect_args", {})
   
           if not connect_args.get("private_key"):
               return
               
           with open(connect_args.get("private_key"), "rb") as key:
                   p_key= serialization.load_pem_private_key(
                       key.read(),
                       password=connect_args.get("private_key_passphrase").encode(),
                       backend=default_backend()
                   )
   
           pkb = p_key.private_bytes(
                   encoding=serialization.Encoding.DER,
                   format=serialization.PrivateFormat.PKCS8,
                   encryption_algorithm=serialization.NoEncryption())
   
           connect_args["private_key"] = pkb
           if connect_args.get("private_key_passphrase"):
               del connect_args["private_key_passphrase"]
   
           engine_params["connect_args"] = connect_args
           extra["engine_params"] = engine_params
   
           return extra
   ....
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] xiayanzheng commented on pull request #21180: feat: add snowflake private key support

Posted by GitBox <gi...@apache.org>.
xiayanzheng commented on PR #21180:
URL: https://github.com/apache/superset/pull/21180#issuecomment-1234479444

   > > > Thanks for implementing this feature @xiayanzheng. We need key-pair authentication in our project too. I was searching for how can I include it in superset, and I encountered your pull request. Hope it would be merged soon grimacing
   > > > @dpgaspar @villebro I've seen you reviewed another pull request for this project recently. How does the review process go? Can we prioritize this small feature? upside_down_face Because, there is no way-around to use keypair auth for snowflake in superset, and we are kinda blocked in our project at the moment.
   > > > About the implementation, in [the official snowflake docs](https://docs.snowflake.com/en/user-guide/python-connector-example.html#using-key-pair-authentication-key-pair-rotation), the decryption of the private key has been explained exactly the same method with this pull request.
   > > > Similarly, in [the snowflake-sqlalchemy repository docs](https://github.com/snowflakedb/snowflake-sqlalchemy#key-pair-authentication-support), the private key has been passed into `connect_args` as bytes during the engine creation, which is same as in the pull request.
   > > > So, technical wise, it looks good to me. slightly_smiling_face
   > > 
   > > 
   > > Thanks for bringing maintainers to this PRsmile, I also hope superset can officially support snowflake keypair authenticating in future release, let‘s make it happenlaughing
   > 
   > @xiayanzheng Do you have time to make the changes mentioned in the code review above, until around end of next week? :)
   
   Yeah, if everything goes fine, I'll finish these changes. I'm planning to set up a dev env and start making changes this weekend.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] villebro commented on a diff in pull request #21180: feat: add snowflake private key support

Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #21180:
URL: https://github.com/apache/superset/pull/21180#discussion_r959140949


##########
superset/databases/commands/test_connection.py:
##########
@@ -73,8 +74,12 @@ def run(self) -> None:
 
             database.set_sqlalchemy_uri(uri)
             database.db_engine_spec.mutate_db_for_connection_test(database)
-
-            engine = database.get_sqla_engine()
+            
+            engine = None
+            if "snowflake" in str(url) and "privatekey" in str(url):

Review Comment:
   @xiayanzheng yes - please see how the Trino connector is currently doing it: 
   In short here's what I think should work:
   
   1. The user should add the private key and password in `encryption_extra` similar to Trino: https://superset.apache.org/docs/databases/trino#authentications . It's important you also add documentation for the precise format in which users should add this information to the JSON object in this file: https://github.com/apache/superset/blob/master/docs/docs/databases/snowflake.mdx
   2. You move the cryptography logic to `superset.db_engine_specs.snowflake.SnowflakeEngineSpec.update_encrypted_extra_params` like Trino does it here: https://github.com/apache/superset/blob/034ee1c3c1fda20b238ba1cc9093a4156d9fa01e/superset/db_engine_specs/trino.py#L183-L221



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] mebegu commented on pull request #21180: feat: add snowflake private key support

Posted by GitBox <gi...@apache.org>.
mebegu commented on PR #21180:
URL: https://github.com/apache/superset/pull/21180#issuecomment-1231396961

   Thanks for implementing this feature @xiayanzheng. We need key-pair authentication in our project too. I was searching for how can I include it in superset, and I encountered your pull request. Hope it would be merged soon :grimacing: 
   
   @dpgaspar @villebro 
   I've seen you reviewed another pull request for this project recently. How does the review process go? Can we prioritize this small feature? :upside_down_face:  Because, there is no way-around to use keypair auth for snowflake in superset, and we are kinda blocked in our project at the moment.
   
   About the implementation, in [the official snowflake docs](https://docs.snowflake.com/en/user-guide/python-connector-example.html#using-key-pair-authentication-key-pair-rotation), the decryption of the private key has been explained exactly the same method with this pull request. 
   
   Similarly, in [the snowflake-sqlalchemy repository docs](https://github.com/snowflakedb/snowflake-sqlalchemy#key-pair-authentication-support), the private key has been passed into `connect_args` as bytes during the engine creation, which is same as in the pull request.
   
   So, technical wise, it looks good to me. :slightly_smiling_face: 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] villebro commented on a diff in pull request #21180: feat: add snowflake private key support

Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #21180:
URL: https://github.com/apache/superset/pull/21180#discussion_r959140949


##########
superset/databases/commands/test_connection.py:
##########
@@ -73,8 +74,12 @@ def run(self) -> None:
 
             database.set_sqlalchemy_uri(uri)
             database.db_engine_spec.mutate_db_for_connection_test(database)
-
-            engine = database.get_sqla_engine()
+            
+            engine = None
+            if "snowflake" in str(url) and "privatekey" in str(url):

Review Comment:
   @xiayanzheng yes - please see how the Trino connector is currently doing it: 
   In short here's what I think should work:
   
   1. The user should add the private key and password in `encryption_extra` similar to Trino: https://superset.apache.org/docs/databases/trino#authentications . You are free to choose the structure of the JSON object, but I think the format chosen by the Trino spec is good, as it makes it easy to add additional authentication types as they become supported later. It's important you also add documentation for the precise format in which users should add this information to the JSON object in this file: https://github.com/apache/superset/blob/master/docs/docs/databases/snowflake.mdx
   2. You move the cryptography logic to `superset.db_engine_specs.snowflake.SnowflakeEngineSpec.update_encrypted_extra_params` like Trino does it here: https://github.com/apache/superset/blob/034ee1c3c1fda20b238ba1cc9093a4156d9fa01e/superset/db_engine_specs/trino.py#L183-L221



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] villebro commented on pull request #21180: feat: add snowflake private key support

Posted by GitBox <gi...@apache.org>.
villebro commented on PR #21180:
URL: https://github.com/apache/superset/pull/21180#issuecomment-1231459998

   @mebegu thanks for bringing this to my attention; I will review this today or tomorrow


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] villebro commented on pull request #21180: feat: add snowflake private key support

Posted by GitBox <gi...@apache.org>.
villebro commented on PR #21180:
URL: https://github.com/apache/superset/pull/21180#issuecomment-1231463235

   I quickly looked at the changes, and this PR will need extensive modification (it should be possible to simplify/generify a great deal). But I will need to set aside some more time for this, so I may not be able to do it very quickly.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] mebegu commented on a diff in pull request #21180: feat: add snowflake private key support

Posted by GitBox <gi...@apache.org>.
mebegu commented on code in PR #21180:
URL: https://github.com/apache/superset/pull/21180#discussion_r960773037


##########
superset/databases/commands/test_connection.py:
##########
@@ -73,8 +74,12 @@ def run(self) -> None:
 
             database.set_sqlalchemy_uri(uri)
             database.db_engine_spec.mutate_db_for_connection_test(database)
-
-            engine = database.get_sqla_engine()
+            
+            engine = None
+            if "snowflake" in str(url) and "privatekey" in str(url):

Review Comment:
   If we run the superset from docker container, are we gonna be able to read the private_key file from the host with given file path? 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov[bot] commented on pull request #21180: feat: add snowflake private key support

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #21180:
URL: https://github.com/apache/superset/pull/21180#issuecomment-1231498122

   # [Codecov](https://codecov.io/gh/apache/superset/pull/21180?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#21180](https://codecov.io/gh/apache/superset/pull/21180?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a2640d9) into [master](https://codecov.io/gh/apache/superset/commit/ed6212a1f9e4d0bdf2b256d83c797bda38784f8e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ed6212a) will **decrease** coverage by `11.64%`.
   > The diff coverage is `41.30%`.
   
   ```diff
   @@             Coverage Diff             @@
   ##           master   #21180       +/-   ##
   ===========================================
   - Coverage   66.39%   54.75%   -11.65%     
   ===========================================
     Files        1781     1783        +2     
     Lines       67874    68038      +164     
     Branches     7239     7239               
   ===========================================
   - Hits        45067    37254     -7813     
   - Misses      20949    28926     +7977     
     Partials     1858     1858               
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | hive | `?` | |
   | mysql | `?` | |
   | postgres | `?` | |
   | presto | `52.99% <34.78%> (-0.08%)` | :arrow_down: |
   | python | `57.36% <41.30%> (-24.16%)` | :arrow_down: |
   | sqlite | `?` | |
   | unit | `50.66% <41.30%> (-0.08%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/21180?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [superset/db\_engine\_specs/snowflake\_privatekey.py](https://codecov.io/gh/apache/superset/pull/21180/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3Nub3dmbGFrZV9wcml2YXRla2V5LnB5) | `28.57% <28.57%> (ø)` | |
   | [superset/databases/commands/test\_connection.py](https://codecov.io/gh/apache/superset/pull/21180/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL3Rlc3RfY29ubmVjdGlvbi5weQ==) | `64.86% <80.00%> (-33.71%)` | :arrow_down: |
   | [superset/models/core.py](https://codecov.io/gh/apache/superset/pull/21180/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `78.72% <83.33%> (-10.04%)` | :arrow_down: |
   | [superset/utils/dashboard\_import\_export.py](https://codecov.io/gh/apache/superset/pull/21180/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvZGFzaGJvYXJkX2ltcG9ydF9leHBvcnQucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset/key\_value/commands/update.py](https://codecov.io/gh/apache/superset/pull/21180/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `0.00% <0.00%> (-88.89%)` | :arrow_down: |
   | [superset/key\_value/commands/delete.py](https://codecov.io/gh/apache/superset/pull/21180/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZS5weQ==) | `0.00% <0.00%> (-85.30%)` | :arrow_down: |
   | [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/21180/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.19%)` | :arrow_down: |
   | [superset/key\_value/commands/delete\_expired.py](https://codecov.io/gh/apache/superset/pull/21180/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZV9leHBpcmVkLnB5) | `0.00% <0.00%> (-80.77%)` | :arrow_down: |
   | [superset/dashboards/commands/importers/v0.py](https://codecov.io/gh/apache/superset/pull/21180/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9jb21tYW5kcy9pbXBvcnRlcnMvdjAucHk=) | `15.62% <0.00%> (-76.25%)` | :arrow_down: |
   | [superset/datasets/commands/update.py](https://codecov.io/gh/apache/superset/pull/21180/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvdXBkYXRlLnB5) | `25.00% <0.00%> (-69.05%)` | :arrow_down: |
   | ... and [285 more](https://codecov.io/gh/apache/superset/pull/21180/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] dpgaspar commented on a diff in pull request #21180: feat: add snowflake private key support

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on code in PR #21180:
URL: https://github.com/apache/superset/pull/21180#discussion_r958305090


##########
superset/databases/commands/test_connection.py:
##########
@@ -73,8 +74,12 @@ def run(self) -> None:
 
             database.set_sqlalchemy_uri(uri)
             database.db_engine_spec.mutate_db_for_connection_test(database)
-
-            engine = database.get_sqla_engine()
+            
+            engine = None
+            if "snowflake" in str(url) and "privatekey" in str(url):

Review Comment:
   possibly the best path would be to use `encryption_extra`, using the database URI with sensitive information like a private key leads to the following security issues:
   - sqlalchemy_uri is not encrypted on the db (but password is, this is the only secret expected on the URI)
   - disclosure of sensitive info through the API
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] villebro commented on a diff in pull request #21180: feat: add snowflake private key support

Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #21180:
URL: https://github.com/apache/superset/pull/21180#discussion_r960778237


##########
superset/databases/commands/test_connection.py:
##########
@@ -73,8 +74,12 @@ def run(self) -> None:
 
             database.set_sqlalchemy_uri(uri)
             database.db_engine_spec.mutate_db_for_connection_test(database)
-
-            engine = database.get_sqla_engine()
+            
+            engine = None
+            if "snowflake" in str(url) and "privatekey" in str(url):

Review Comment:
   > If we run the superset from docker container, are we gonna be able to read the private_key file from the host with given file path? 
   
   I meant to address this in my review, but I forgot. I would recommend placing the contents of the file in the object rather than a path to the file. We do something similar for BigQuery. If help is needed I can elsborate, but the docs+code should be pretty straight forward.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] villebro commented on a diff in pull request #21180: feat: add snowflake private key support

Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #21180:
URL: https://github.com/apache/superset/pull/21180#discussion_r960890192


##########
superset/databases/commands/test_connection.py:
##########
@@ -73,8 +74,12 @@ def run(self) -> None:
 
             database.set_sqlalchemy_uri(uri)
             database.db_engine_spec.mutate_db_for_connection_test(database)
-
-            engine = database.get_sqla_engine()
+            
+            engine = None
+            if "snowflake" in str(url) and "privatekey" in str(url):

Review Comment:
   @mebegu the root certificate is for a different use case. I would recommend using the encrypted extra params, as that's precisely for this use case.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] mebegu commented on a diff in pull request #21180: feat: add snowflake private key support

Posted by GitBox <gi...@apache.org>.
mebegu commented on code in PR #21180:
URL: https://github.com/apache/superset/pull/21180#discussion_r960789591


##########
superset/databases/commands/test_connection.py:
##########
@@ -73,8 +74,12 @@ def run(self) -> None:
 
             database.set_sqlalchemy_uri(uri)
             database.db_engine_spec.mutate_db_for_connection_test(database)
-
-            engine = database.get_sqla_engine()
+            
+            engine = None
+            if "snowflake" in str(url) and "privatekey" in str(url):

Review Comment:
   There is also a text field called `Root certificate` under security tab, that can be used. But I am not sure if it would conflict or be consistent with other components.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] villebro commented on a diff in pull request #21180: feat: add snowflake private key support

Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #21180:
URL: https://github.com/apache/superset/pull/21180#discussion_r959140949


##########
superset/databases/commands/test_connection.py:
##########
@@ -73,8 +74,12 @@ def run(self) -> None:
 
             database.set_sqlalchemy_uri(uri)
             database.db_engine_spec.mutate_db_for_connection_test(database)
-
-            engine = database.get_sqla_engine()
+            
+            engine = None
+            if "snowflake" in str(url) and "privatekey" in str(url):

Review Comment:
   @xiayanzheng yes - please see how the Trino connector is currently doing it: 
   In short here's what I think should work:
   
   1. You assume the user should add the private key and password in `encryption_extra` similar to Trino: https://superset.apache.org/docs/databases/trino#authentications . It's important you also add documentation for the precise format in which users should add this information to the JSON object in this file: https://github.com/apache/superset/blob/master/docs/docs/databases/snowflake.mdx
   2. You move the cryptography logic to `superset.db_engine_specs.snowflake.SnowflakeEngineSpec.update_encrypted_extra_params` like Trino does it here: https://github.com/apache/superset/blob/034ee1c3c1fda20b238ba1cc9093a4156d9fa01e/superset/db_engine_specs/trino.py#L183-L221



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] dpgaspar commented on a diff in pull request #21180: feat: add snowflake private key support

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on code in PR #21180:
URL: https://github.com/apache/superset/pull/21180#discussion_r958299751


##########
superset/databases/commands/test_connection.py:
##########
@@ -73,8 +74,12 @@ def run(self) -> None:
 
             database.set_sqlalchemy_uri(uri)
             database.db_engine_spec.mutate_db_for_connection_test(database)
-
-            engine = database.get_sqla_engine()
+            
+            engine = None
+            if "snowflake" in str(url) and "privatekey" in str(url):

Review Comment:
   Following @villebro comment, we should avoid having specific engine assertion at this layer, it's also not the most robust solution to search for "snowflake" inside the url str. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] xiayanzheng commented on a diff in pull request #21180: feat: add snowflake private key support

Posted by GitBox <gi...@apache.org>.
xiayanzheng commented on code in PR #21180:
URL: https://github.com/apache/superset/pull/21180#discussion_r958739898


##########
superset/databases/commands/test_connection.py:
##########
@@ -73,8 +74,12 @@ def run(self) -> None:
 
             database.set_sqlalchemy_uri(uri)
             database.db_engine_spec.mutate_db_for_connection_test(database)
-
-            engine = database.get_sqla_engine()
+            
+            engine = None
+            if "snowflake" in str(url) and "privatekey" in str(url):

Review Comment:
   Thanks for the review, you mentioned we should put “private key path” and “private key pw” into encryption_extra, is this the same thing with blow?
   ![image](https://user-images.githubusercontent.com/14834951/187499981-96513d62-7cdf-41b7-b02f-ee544e44b31f.png)
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] xiayanzheng commented on pull request #21180: feat: add snowflake private key support

Posted by GitBox <gi...@apache.org>.
xiayanzheng commented on PR #21180:
URL: https://github.com/apache/superset/pull/21180#issuecomment-1232346129

   > Thanks for implementing this feature @xiayanzheng. We need key-pair authentication in our project too. I was searching for how can I include it in superset, and I encountered your pull request. Hope it would be merged soon 😬
   > 
   > @dpgaspar @villebro I've seen you reviewed another pull request for this project recently. How does the review process go? Can we prioritize this small feature? 🙃 Because, there is no way-around to use keypair auth for snowflake in superset, and we are kinda blocked in our project at the moment.
   > 
   > About the implementation, in [the official snowflake docs](https://docs.snowflake.com/en/user-guide/python-connector-example.html#using-key-pair-authentication-key-pair-rotation), the decryption of the private key has been explained exactly the same method with this pull request.
   > 
   > Similarly, in [the snowflake-sqlalchemy repository docs](https://github.com/snowflakedb/snowflake-sqlalchemy#key-pair-authentication-support), the private key has been passed into `connect_args` as bytes during the engine creation, which is same as in the pull request.
   > 
   > So, technical wise, it looks good to me. 🙂
   
   Thanks for bringing maintainers to this PR😄, I also hope superset can officially support snowflake keypair authenticating in future release, let‘s make it happen😆 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] mebegu commented on pull request #21180: feat: add snowflake private key support

Posted by GitBox <gi...@apache.org>.
mebegu commented on PR #21180:
URL: https://github.com/apache/superset/pull/21180#issuecomment-1234407411

   > > Thanks for implementing this feature @xiayanzheng. We need key-pair authentication in our project too. I was searching for how can I include it in superset, and I encountered your pull request. Hope it would be merged soon grimacing
   > > @dpgaspar @villebro I've seen you reviewed another pull request for this project recently. How does the review process go? Can we prioritize this small feature? upside_down_face Because, there is no way-around to use keypair auth for snowflake in superset, and we are kinda blocked in our project at the moment.
   > > About the implementation, in [the official snowflake docs](https://docs.snowflake.com/en/user-guide/python-connector-example.html#using-key-pair-authentication-key-pair-rotation), the decryption of the private key has been explained exactly the same method with this pull request.
   > > Similarly, in [the snowflake-sqlalchemy repository docs](https://github.com/snowflakedb/snowflake-sqlalchemy#key-pair-authentication-support), the private key has been passed into `connect_args` as bytes during the engine creation, which is same as in the pull request.
   > > So, technical wise, it looks good to me. slightly_smiling_face
   > 
   > Thanks for bringing maintainers to this PRsmile, I also hope superset can officially support snowflake keypair authenticating in future release, let‘s make it happenlaughing
   
   @xiayanzheng  Do you have time to make the changes mentioned in the code review above, until around end of next week? :)  


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] xiayanzheng closed pull request #21180: feat: add snowflake private key support

Posted by GitBox <gi...@apache.org>.
xiayanzheng closed pull request #21180: feat: add snowflake private key support
URL: https://github.com/apache/superset/pull/21180


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org