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

hughhhh · 2022-10-21T16:41:21Z

SUMMARY

Related to #21789

Creating table to store ssh tunnel credentials. This a starter ticket for allowing using to enable ssh tunneling when trying to connect to a database.

DAO (feat(ssh_tunnel): DAO Changes for SSH Tunnel #22120)
Commands + exceptions
- DELETE (feat(ssh_tunnel): Delete command & exceptions #22131)
- CREATE (feat(ssh_tunnel): Create command & exceptions #22148)
- UPDATE (feat(ssh_tunnel): Update command & exceptions #22132
Feature Flag to enable this feature (feat(ssh-tunnel): ssh manager config + feature flag #22201)

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-10-24T21:24:58Z

Codecov Report

Merging #21912 (9b09fc7) into master (a7a4561) will decrease coverage by 11.00%.
The diff coverage is 72.78%.

@@             Coverage Diff             @@
##           master   #21912       +/-   ##
===========================================
- Coverage   66.91%   55.91%   -11.01%     
===========================================
  Files        1851     1859        +8     
  Lines       70709    71016      +307     
  Branches     7766     7764        -2     
===========================================
- Hits        47316    39709     -7607     
- Misses      21371    29285     +7914     
  Partials     2022     2022

Flag	Coverage Δ
hive	`52.46% <51.77%> (+<0.01%)`	⬆️
mysql	`?`
postgres	`?`
presto	`52.36% <51.77%> (+<0.01%)`	⬆️
python	`58.15% <72.78%> (-23.12%)`	⬇️
sqlite	`?`
unit	`51.46% <71.89%> (+0.27%)`	⬆️

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

Impacted Files	Coverage Δ
superset/constants.py	`100.00% <ø> (ø)`
superset/databases/commands/update.py	`24.13% <20.00%> (-55.87%)`	⬇️
superset/utils/ssh_tunnel.py	`30.00% <30.00%> (ø)`
superset/databases/commands/create.py	`60.60% <35.71%> (-26.19%)`	⬇️
superset/models/core.py	`77.27% <51.06%> (-13.35%)`	⬇️
superset/extensions/ssh.py	`65.71% <65.71%> (ø)`
superset/config.py	`90.93% <66.66%> (-0.56%)`	⬇️
superset/databases/api.py	`53.63% <66.66%> (-39.91%)`	⬇️
superset/databases/commands/test_connection.py	`64.10% <75.00%> (-34.57%)`	⬇️
superset/databases/ssh_tunnel/commands/create.py	`87.75% <87.75%> (ø)`
... and 314 more

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

eschutho · 2022-10-25T21:22:02Z

superset/databases/models.py

+    database_id = sa.Column(sa.Integer, sa.ForeignKey("dbs.id"), nullable=False)
+    database: Database = relationship(
+        "Database",
+        backref=backref("ssh_tunnel_credentials", cascade="all, delete-orphan"),


this is a one-to-one, correct? If so, I think you'll need uselist=False here as well

eschutho · 2022-10-25T21:23:45Z

superset/databases/models.py

+app_config = app.config
+
+
+class SSHTunnelCredentials(


nit, but naming-wise, I feel like a model contains credentials, therefore the name of the model can be more generic, like just SSHTunnel. Open to other thoughts/ideas on that.

eschutho · 2022-10-25T21:31:14Z

superset/databases/models.py

+    database: Database = relationship(
+        "Database",
+        backref=backref("ssh_tunnel_credentials", cascade="all, delete-orphan"),
+        foreign_keys=[database_id],


I don't think you need this foreign_key here since you declared it in the database_id column

eschutho · 2022-10-25T23:10:06Z

superset/databases/models.py

+
+    # password protected pkey authentication
+    private_key = sa.Column(
+        EncryptedType(sa.String, app_config["SECRET_KEY"]), nullable=True


This is more of a question than a suggestion, but I see that we're using encrypted_field_factory for database passwords and encrypted_extra. Is there any benefit to using the EncryptedType here instead?

eschutho · 2022-10-25T23:18:34Z

This looks great @hughhhh. If you wanted to write a test, one scenario that would be useful to see would be the test connection where you would create instances of the db and ssh_tunnel without saving them and be able to pull information out of both of the models.

AAfghahi · 2022-10-26T15:24:14Z

superset/migrations/versions/2022-10-20_10-48_f3c2d8ec8595_create_ssh_tunnel_credentials_tbl.py

+app_config = app.config
+
+
+def upgrade():


I know that previously we had established the pattern that migration and logic should be in separate PRs. I thought that made sense, since it helps with reversing the migration in case something happens. Thoughts on doing that for this PR?

IMO, that pattern is hard to work with -- it's hard to make a single PR with migrations that are later depended upon by other code as the migrations tend to evolve as you work on the feature. Also, encapsulating related changes in a single PR makes more sense as the revert would remove the DB migrations along with the code that's depending on them :)

@craig-rueda I think we're always going to have this discussion... :)

Having migrations in separate PRs really helps when you need to cherry-pick a PR with a migration, since when that happens you also need to cherry-pick every PR that has a migration in between, regardless of what it is. If those intermediary PRs are harmless DB migrations the cherry-pick is much easier.

My recommendation has been: work on a single PR, since as you said the migrations tend to evolve during development. When the PR is ready, split it into 2. This of course assumes that the DB migration can live independently from the code (eg, it adds a new table or a new column).

@betodealmeida, I see your point, but still think it's easier to reason and easier to revert like @craig-rueda said. Also is happens you don't get things right the first time, then you end up with a stream of db mig PR's. I think it's easier to reason on 1 PR 1 feature (at least on the backend), follow up PR's are fixes or frontend code. Has always there can be exceptions on really big features.

Just adding to the discussion :)

craig-rueda · 2022-10-27T19:55:35Z

I think it might be better to just add this model into the PR you're planning down the road that adds the SSH handling. It's a little hard to grok what's going on out of context. Also, you almost ALWAYS end up making model tweaks as you implement a new feature, which will require another migration 😬

betodealmeida · 2022-10-27T20:06:29Z

I think it might be better to just add this model into the PR you're planning down the road that adds the SSH handling. It's a little hard to grok what's going on out of context. Also, you almost ALWAYS end up making model tweaks as you implement a new feature, which will require another migration 😬

IMHO the best approach is to write a single PR with logic + migration (to prevent the issues you raised), but then split it in two for review. Having self-contained migrations really helps with cherry-picking, because if you need to cherry-pick a PR that has a migration you have to also cherry-pick all previous PRs with migrations.

…te-sshtunnelconfig-tbl

craig-rueda · 2022-12-12T19:50:13Z

superset/models/core.py

+            database_id=self.id
+        ):
+            # if ssh_tunnel is available build engine with information
+            url = make_url_safe(self.sqlalchemy_uri_decrypted)


This block should be managed by a SSHManager that is configured via app_config. It's currently not possible for other folks to update the behavior of the tunneling mechanism

craig-rueda · 2022-12-15T19:27:57Z

superset/models/core.py

@@ -66,6 +66,7 @@
 from superset.utils.memoized import memoized

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


This should be exposed via extensions.

superset/migrations/versions/2022-10-20_10-48_f3c2d8ec8595_create_ssh_tunnel_credentials_tbl.py

…ate_ssh_tunnel_credentials_tbl.py Co-authored-by: Elizabeth Thompson <eschutho@gmail.com>

superset/migrations/versions/2022-10-20_10-48_f3c2d8ec8595_create_ssh_tunnel_credentials_tbl.py

…ate_ssh_tunnel_credentials_tbl.py Co-authored-by: Elizabeth Thompson <eschutho@gmail.com>

…te-sshtunnelconfig-tbl

He's out on vacation

…te-sshtunnelconfig-tbl

craig-rueda

LGTM!

Co-authored-by: hughhhh <hughmil3s@gmail.com>

github-actions · 2023-01-03T22:23:05Z

Ephemeral environment shutdown and build artifacts deleted.

hughhhh added 3 commits October 19, 2022 19:27

save

830a283

create migration

2c1e736

created schema and rename

f78df83

pull-request-size bot added the size/L label Oct 21, 2022

hughhhh added 3 commits October 21, 2022 11:36

linting

d482df4

fix encrpytions

9edb581

remove map tabl

da27d8f

hughhhh added 3 commits October 25, 2022 11:44

fix linting

773a6c8

add constraint

2f2dda2

add fk to migration

fd0d7f2

eschutho reviewed Oct 25, 2022

View reviewed changes

AAfghahi reviewed Oct 26, 2022

View reviewed changes

hughhhh added 4 commits October 26, 2022 14:03

init

158da8d

update all the examples

face73f

change remaining bits

95d079e

add id

d5926e3

hughhhh marked this pull request as ready for review October 27, 2022 19:41

hughhhh requested a review from a team as a code owner October 27, 2022 19:41

hughhhh added 5 commits October 27, 2022 21:49

use factory instead

f7a6a41

Merge branch 'master' of https://github.com/apache/superset into crea…

30e380a

…te-sshtunnelconfig-tbl

Merge branch 'master' into ref-get-sqla-engine-2

87c0d79

fix confict

11b240b

fix conflict

1bfdbda

craig-rueda reviewed Dec 12, 2022

View reviewed changes

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

c5c50ed

craig-rueda reviewed Dec 15, 2022

View reviewed changes

update kwarg function name

06e115b

hughhhh force-pushed the create-sshtunnelconfig-tbl branch from c346ae0 to 06e115b Compare December 15, 2022 19:39

hughhhh added 2 commits December 16, 2022 10:31

chore(ssh-tunnel): Move SSHManager to extensions pattern (#22433)

13ed50d

add flag to indicate ssh tunneling is enabled for this engine

54d51e2

eschutho reviewed Dec 19, 2022

View reviewed changes

superset/migrations/versions/2022-10-20_10-48_f3c2d8ec8595_create_ssh_tunnel_credentials_tbl.py Outdated Show resolved Hide resolved

Update superset/migrations/versions/2022-10-20_10-48_f3c2d8ec8595_cre…

53eaa63

…ate_ssh_tunnel_credentials_tbl.py Co-authored-by: Elizabeth Thompson <eschutho@gmail.com>

eschutho reviewed Dec 19, 2022

View reviewed changes

superset/migrations/versions/2022-10-20_10-48_f3c2d8ec8595_create_ssh_tunnel_credentials_tbl.py Outdated Show resolved Hide resolved

Update superset/migrations/versions/2022-10-20_10-48_f3c2d8ec8595_cre…

8f8faff

…ate_ssh_tunnel_credentials_tbl.py Co-authored-by: Elizabeth Thompson <eschutho@gmail.com>

eschutho approved these changes Dec 19, 2022

View reviewed changes

hughhhh added 3 commits December 19, 2022 20:41

fix linting

607c682

Merge branch 'master' of https://github.com/apache/superset into crea…

1ea0e8b

…te-sshtunnelconfig-tbl

fix requirements

7cc7bc8

eschutho requested a review from betodealmeida January 3, 2023 18:07

hughhhh requested a review from craig-rueda January 3, 2023 19:00

hughhhh added 2 commits January 3, 2023 15:03

Merge branch 'master' of https://github.com/apache/superset into crea…

7c539d2

…te-sshtunnelconfig-tbl

get df with get_raw_connection function

394afc1

craig-rueda approved these changes Jan 3, 2023

View reviewed changes

feat(ssh_tunnel): APIs for SSH Tunnels (#22199)

9b09fc7

Co-authored-by: hughhhh <hughmil3s@gmail.com>

hughhhh merged commit ebaad10 into master Jan 3, 2023

diegomedina248 mentioned this pull request Jan 5, 2023

chore: Migrate /superset/queries/<last_updated_ms> to API v1 #22611

Merged

9 tasks

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024

mistercrunch deleted the create-sshtunnelconfig-tbl branch March 26, 2024 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hughhhh commented Oct 21, 2022 •

edited

codecov bot commented Oct 24, 2022 •

edited

eschutho Oct 25, 2022

eschutho Oct 25, 2022 •

edited

eschutho Oct 25, 2022

eschutho Oct 25, 2022 •

edited

eschutho commented Oct 25, 2022

AAfghahi Oct 26, 2022

craig-rueda Nov 21, 2022

betodealmeida Nov 30, 2022

dpgaspar Dec 7, 2022

craig-rueda commented Oct 27, 2022

betodealmeida commented Oct 27, 2022

craig-rueda Dec 12, 2022

craig-rueda Dec 15, 2022

craig-rueda left a comment

github-actions bot commented Jan 3, 2023

		app_config = app.config


		class SSHTunnelCredentials(

		app_config = app.config


		def upgrade():

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

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

Conversation

hughhhh commented Oct 21, 2022 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Oct 24, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

eschutho Oct 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho Oct 25, 2022 • edited

Choose a reason for hiding this comment

eschutho commented Oct 25, 2022

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 commented Oct 27, 2022

betodealmeida commented Oct 27, 2022

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

github-actions bot commented Jan 3, 2023

hughhhh commented Oct 21, 2022 •

edited

codecov bot commented Oct 24, 2022 •

edited

eschutho Oct 25, 2022 •

edited

eschutho Oct 25, 2022 •

edited