feat(ssh-tunnel): ssh manager config + feature flag #22201

hughhhh · 2022-11-22T22:30:39Z

SUMMARY

Created a config object class to manage how ssh tunnels can be configured. The main use case for this is to allow mutation and validation on ssh tunnels being created for a given superset instance. For the SSHManager the

class SSHManager:  # pylint: disable=too-few-public-methods
    local_bind_address = "127.0.0.1"

    def mutator(  # pylint: disable=no-self-use
        self, sqlalchemy_url: str, server: sshtunnel.SSHTunnelForwarder
    ) -> str:
        # override any ssh tunnel configuration object
        from superset.databases.utils import make_url_safe

        url = make_url_safe(sqlalchemy_url)
        return url.set(
            host=self.local_bind_address,
            port=server.local_bind_port,
        )

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2022-11-22T22:32:49Z

Codecov Report

Merging #22201 (c6a3dee) into create-sshtunnelconfig-tbl (e3ef835) will decrease coverage by 11.16%.
The diff coverage is 69.23%.

❗ Current head c6a3dee differs from pull request most recent head 146c476. Consider uploading reports for the commit 146c476 to get more accurate results

@@                       Coverage Diff                       @@
##           create-sshtunnelconfig-tbl   #22201       +/-   ##
===============================================================
- Coverage                       66.95%   55.79%   -11.17%     
===============================================================
  Files                            1852     1852               
  Lines                           70504    70511        +7     
  Branches                         7689     7689               
===============================================================
- Hits                            47206    39341     -7865     
- Misses                          21304    29176     +7872     
  Partials                         1994     1994

Flag	Coverage Δ
hive	`52.58% <69.23%> (+<0.01%)`	⬆️
mysql	`?`
postgres	`?`
presto	`52.47% <69.23%> (+<0.01%)`	⬆️
python	`57.97% <69.23%> (-23.38%)`	⬇️
sqlite	`?`
unit	`51.11% <69.23%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/constants.py	`100.00% <ø> (ø)`
superset/config.py	`90.82% <66.66%> (-1.03%)`	⬇️
superset/models/core.py	`76.73% <66.66%> (-13.05%)`	⬇️
superset/databases/ssh_tunnel/models.py	`75.86% <100.00%> (ø)`
superset/utils/dashboard_import_export.py	`0.00% <0.00%> (-100.00%)`	⬇️
superset/tags/core.py	`4.54% <0.00%> (-95.46%)`	⬇️
superset/key_value/commands/update.py	`0.00% <0.00%> (-90.91%)`	⬇️
superset/key_value/commands/delete.py	`0.00% <0.00%> (-87.88%)`	⬇️
superset/key_value/commands/delete_expired.py	`0.00% <0.00%> (-84.00%)`	⬇️
superset/dashboards/commands/importers/v0.py	`15.62% <0.00%> (-76.25%)`	⬇️
... and 288 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

craig-rueda · 2022-11-29T18:58:54Z

superset/config.py

+#         # `server` return value
+#         return "127.0.0.1"
+
+SSH_TUNNEL_MANAGER = None


Should we provide a default impl here?

craig-rueda · 2022-11-29T18:59:30Z

superset/databases/ssh_tunnel/commands/create.py

@@ -20,16 +20,24 @@
 from flask_appbuilder.models.sqla import Model
 from marshmallow import ValidationError

+
+from superset import app, is_feature_enabled


Don't directly import app here. Instead, use flask.current_app

craig-rueda · 2022-11-29T18:59:59Z

superset/databases/ssh_tunnel/commands/create.py

 from superset.commands.base import BaseCommand
 from superset.dao.exceptions import DAOCreateFailedError
 from superset.databases.dao import DatabaseDAO
 from superset.databases.ssh_tunnel.commands.exceptions import SSHTunnelCreateFailedError
 from superset.databases.ssh_tunnel.dao import SSHTunnelDAO

+config = app.config
+ssh_tunnel_manager = config["SSH_TUNNEL_MANAGER"]


Put this inside the command's init() and leverage current_app.config

craig-rueda · 2022-11-29T19:01:38Z

superset/config.py

+#                          FIREWALL (only port 22 is open)
+
+# ----------------------------------------------------------------------
+# class SSHManager:


The default manager should leverage the app_init pattern. So, inside the AppInitializer, the actual manager would be instantiated and then would have init_app(app: Flask) invoked on it

craig-rueda · 2022-11-29T19:02:20Z

superset/databases/ssh_tunnel/commands/update.py

@@ -33,6 +34,9 @@

 logger = logging.getLogger(__name__)

+config = app.config
+ssh_tunnel_manager = config["SSH_TUNNEL_MANAGER"]


Same comment as before. We should be getting the SSHManager from the current_app

craig-rueda

Let's use the app factory pattern for the SSHManager: https://flask.palletsprojects.com/en/2.2.x/extensiondev/#the-extension-class-and-initialization

eschutho · 2022-11-30T00:40:45Z

superset/config.py

+
+#      def mutator(self, ssh_tunnel_params: Dict[str, Any]) -> Dict[str, Any]:
+#          # override any ssh tunnel configuration object
+#          raise NotImplemented()


do we need to make these two methods required? They seems more like nice to haves.

eschutho · 2022-11-30T00:41:23Z

superset/databases/ssh_tunnel/commands/create.py

 logger = logging.getLogger(__name__)


 class CreateSSHTunnelCommand(BaseCommand):
+    def __str__(self) -> str:
+        return super().__str__()


isn't this redundant?

eschutho · 2022-11-30T00:41:53Z

superset/databases/ssh_tunnel/commands/create.py

-        # using the config.SSH_TUNNEL_MANAGER
-        return
+        if is_feature_enabled("SSH_TUNNELING") and ssh_tunnel_manager:
+            ssh_tunnel_manager.validate(self._properties)


is this the only validation that we want to do here?

eschutho · 2022-11-30T00:46:25Z

superset/models/core.py

-        else:
-            # do look up in table for using database_id
-            try:
+            with sshtunnel.open_tunnel(**ssh_params) as server:


are there any known errors from the library that we need to catch here?

…e/superset into ssh-manager-config

hughhhh added 3 commits November 22, 2022 17:02

write class in config

0f6038f

add mutate function

2502f63

refactor exceptions and add mutator

2522660

pull-request-size bot added the size/M label Nov 22, 2022

add feature flag to enable validation checks

73b58e5

hughhhh changed the title ~~Ssh manager config~~ feat(ssh-tunnel): ssh manager config + feature flag Nov 24, 2022

update class object nots

58e37f1

pull-request-size bot added size/L and removed size/M labels Nov 25, 2022

add test for ssh manager

d720e75

hughhhh force-pushed the ssh-manager-config branch from 32e0564 to d720e75 Compare November 25, 2022 15:35

add test for update and engine instantiation

097179c

hughhhh marked this pull request as ready for review November 29, 2022 16:31

fix pre-commit

8c6d6e4

craig-rueda reviewed Nov 29, 2022

View reviewed changes

craig-rueda requested changes Nov 29, 2022

View reviewed changes

hughhhh force-pushed the create-sshtunnelconfig-tbl branch 2 times, most recently from 225ea11 to 466703a Compare November 29, 2022 23:56

eschutho reviewed Nov 30, 2022

View reviewed changes

hughhhh added 2 commits November 30, 2022 12:15

Merge branch 'create-sshtunnelconfig-tbl' of https://github.com/apach…

7168f00

…e/superset into ssh-manager-config

pull config from current_app

96cdbf5

hughhhh force-pushed the create-sshtunnelconfig-tbl branch from 1a1141a to b25ff51 Compare December 2, 2022 00:47

hughhhh force-pushed the create-sshtunnelconfig-tbl branch 5 times, most recently from 2ba07f5 to 1f57d4a Compare December 7, 2022 21:33

hughhhh added 3 commits December 14, 2022 12:22

fix merge conflicts

9f39c83

fix pylint

87f3065

remove validation tests

1f99503

pull-request-size bot added size/M and removed size/L labels Dec 14, 2022

okkkk

874c8e3

hughhhh force-pushed the ssh-manager-config branch 5 times, most recently from dde7f64 to 19f9368 Compare December 15, 2022 04:19

ignore nonsense

71ff153

hughhhh force-pushed the ssh-manager-config branch from 19f9368 to 71ff153 Compare December 15, 2022 05:46

hughhhh added 2 commits December 15, 2022 01:12

ok i need to test this

27c5023

ok i need to test this

146c476

hughhhh force-pushed the ssh-manager-config branch from 6b3c0c4 to 146c476 Compare December 15, 2022 17:57

verified working update

329e3a5

hughhhh merged commit c5c50ed into create-sshtunnelconfig-tbl Dec 15, 2022

hughhhh mentioned this pull request Dec 15, 2022

feat(ssh-tunnelling): Setup SSH Tunneling Commands for Database Connections #21912

Merged

15 tasks

mistercrunch deleted the ssh-manager-config branch March 26, 2024 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ssh-tunnel): ssh manager config + feature flag #22201

feat(ssh-tunnel): ssh manager config + feature flag #22201

hughhhh commented Nov 22, 2022 •

edited

Loading

codecov bot commented Nov 22, 2022 •

edited

Loading

craig-rueda Nov 29, 2022

craig-rueda Nov 29, 2022

craig-rueda Nov 29, 2022

craig-rueda Nov 29, 2022

craig-rueda Nov 29, 2022

craig-rueda left a comment

eschutho Nov 30, 2022

eschutho Nov 30, 2022

eschutho Nov 30, 2022

eschutho Nov 30, 2022

feat(ssh-tunnel): ssh manager config + feature flag #22201

feat(ssh-tunnel): ssh manager config + feature flag #22201

Conversation

hughhhh commented Nov 22, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Nov 22, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

craig-rueda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughhhh commented Nov 22, 2022 •

edited

Loading

codecov bot commented Nov 22, 2022 •

edited

Loading