You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "betodealmeida (via GitHub)" <gi...@apache.org> on 2025/07/29 21:43:05 UTC
[PR] chore: cleanup ssh tunnel [superset]
betodealmeida opened a new pull request, #34388:
URL: https://github.com/apache/superset/pull/34388
<!---
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 -->
SQLAlchemy relationships are great, let's use them.
This PR cleans up the relationship between databases and their optional SSH tunnels, renaming it to `ssh_tunnel` (since it's 1:1) and using the relationship instead of the DAO.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] 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
- [ ] 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
Re: [PR] chore: cleanup ssh tunnel [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on PR #34388:
URL: https://github.com/apache/superset/pull/34388#issuecomment-3222189039
Performed these manual tests:
1. Created a PSQL connection with SSH on `master`.
2. Checked out to this branch.
3. Confirmed the connection is still working.
4. Deleted the bastion host on AWS.
5. Confirmed the connection is no longer working.
6. Updated the connection with the new EC2 info.
7. Connection is back working.
8. Tested creating a new connection -- works.
@betodealmeida it's def not related with this PR, but wondering if you want to also tackle this. In the dynamic form, we have **Private Key Password** marked with `*` (required) even tho it's not (and it shouldn't be, as the private key might not have a password):
<img width="507" height="425" alt="image" src="https://github.com/user-attachments/assets/9b4841ea-825a-4bd8-856d-aad2e16e4b66" />
I also noticed some issues in which I would have all requires fields filled (except this one) and the Connect button would still be disabled, but then clicking on some fields would unblock the button. I think the validation is a bit flaky.
--
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
Re: [PR] chore: cleanup ssh tunnel [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #34388:
URL: https://github.com/apache/superset/pull/34388#discussion_r2304897509
##########
superset/commands/database/create.py:
##########
@@ -59,6 +57,12 @@ def __init__(self, data: dict[str, Any]):
def run(self) -> Model:
self.validate()
+ if "sqlalchemy_uri" not in self._properties:
+ raise DatabaseRequiredFieldValidationError("sqlalchemy_uri")
Review Comment:
Ah, good catch! Will remove it!
--
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
Re: [PR] chore: cleanup ssh tunnel [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #34388:
URL: https://github.com/apache/superset/pull/34388#discussion_r2299424809
##########
superset/commands/database/create.py:
##########
@@ -118,25 +111,23 @@ def run(self) -> Model:
) as ex:
event_logger.log_with_context(
action=f"db_creation_failed.{ex.__class__.__name__}",
- engine=database.db_engine_spec.__name__,
+ engine=engine,
)
raise DatabaseCreateFailedError() from ex
- if ssh_tunnel:
- stats_logger.incr("db_creation_success.ssh_tunnel")
-
return database
def validate(self) -> None:
Review Comment:
Does it make sense to validate in here if 1) the DB has an SSH config and 2) the FF is disabled? This way we short-circuit early.
--
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
Re: [PR] chore: cleanup ssh tunnel [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #34388:
URL: https://github.com/apache/superset/pull/34388#discussion_r2304910943
##########
superset/commands/database/create.py:
##########
@@ -118,25 +111,23 @@ def run(self) -> Model:
) as ex:
event_logger.log_with_context(
action=f"db_creation_failed.{ex.__class__.__name__}",
- engine=database.db_engine_spec.__name__,
+ engine=engine,
)
raise DatabaseCreateFailedError() from ex
- if ssh_tunnel:
- stats_logger.incr("db_creation_success.ssh_tunnel")
-
return database
def validate(self) -> None:
Review Comment:
Yeah, let me do it!
--
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
Re: [PR] chore: cleanup ssh tunnel [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #34388:
URL: https://github.com/apache/superset/pull/34388#discussion_r2252540071
##########
superset/databases/ssh_tunnel/models.py:
##########
@@ -44,11 +44,19 @@ class SSHTunnel(AuditMixinNullable, ExtraJSONMixin, ImportExportMixin, Model):
id = sa.Column(sa.Integer, primary_key=True)
database_id = sa.Column(
- sa.Integer, sa.ForeignKey("dbs.id"), nullable=False, unique=True
+ sa.Integer,
+ sa.ForeignKey("dbs.id"),
+ nullable=False,
+ unique=True,
)
database: Database = relationship(
"Database",
- backref=backref("ssh_tunnels", uselist=False, cascade="all, delete-orphan"),
+ backref=backref(
+ "ssh_tunnel",
+ uselist=False,
+ cascade="all, delete-orphan",
+ lazy="joined",
Review Comment:
Allows us to simply `database.ssh_tunnel`.
--
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
Re: [PR] chore: cleanup ssh tunnel [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #34388:
URL: https://github.com/apache/superset/pull/34388#discussion_r2252540573
##########
superset/databases/api.py:
##########
@@ -1984,73 +1983,6 @@ def validate_parameters(self) -> FlaskResponse:
command.run()
return self.response(200, message="OK")
- @expose("/<int:pk>/ssh_tunnel/", methods=("DELETE",))
- @protect()
- @statsd_metrics
- @deprecated(deprecated_in="4.0")
- @event_logger.log_this_with_context(
- action=lambda self, *args, **kwargs: f"{self.__class__.__name__}"
- f".delete_ssh_tunnel",
- log_to_statsd=False,
- )
- def delete_ssh_tunnel(self, pk: int) -> Response:
Review Comment:
This was not used anywhere.
--
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
Re: [PR] chore: cleanup ssh tunnel [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida merged PR #34388:
URL: https://github.com/apache/superset/pull/34388
--
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
Re: [PR] chore: cleanup ssh tunnel [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #34388:
URL: https://github.com/apache/superset/pull/34388#discussion_r2252537110
##########
superset/commands/database/create.py:
##########
@@ -118,25 +111,23 @@ def run(self) -> Model:
) as ex:
event_logger.log_with_context(
action=f"db_creation_failed.{ex.__class__.__name__}",
- engine=database.db_engine_spec.__name__,
Review Comment:
Here `database` can be undefined, raising an exception inside the catch block. Instead I compute the engine from the URI, and pass it to all instances where we log.
--
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
Re: [PR] chore: cleanup ssh tunnel [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #34388:
URL: https://github.com/apache/superset/pull/34388#discussion_r2304901931
##########
superset/commands/database/test_connection.py:
##########
@@ -108,33 +109,37 @@ def run( # noqa: C901
)
)
+ # collect SSH tunnel info
+ ssh_tunnel_properties = self._properties.get("ssh_tunnel")
+ if ssh_tunnel_properties and self._model and self._model.ssh_tunnel:
+ # unmask password while allowing for updated values
+ ssh_tunnel_properties = unmask_password_info(
+ ssh_tunnel_properties,
+ self._model.ssh_tunnel,
+ )
+
+ database: Database | None = None
Review Comment:
`validate()` will have checked the FF by this point
--
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
Re: [PR] chore: cleanup ssh tunnel [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on PR #34388:
URL: https://github.com/apache/superset/pull/34388#issuecomment-3134162399
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment `/korbit-review`.
> Your admin can change your review schedule in the [Korbit Console](https://app.korbit.ai/settings)
[](2b99e881-ba0e-4d43-99f8-ed26cb1eb4fd)
--
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
Re: [PR] chore: cleanup ssh tunnel [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #34388:
URL: https://github.com/apache/superset/pull/34388#issuecomment-3607739867
CodeAnt AI is reviewing your PR.
--
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
Re: [PR] chore: cleanup ssh tunnel [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #34388:
URL: https://github.com/apache/superset/pull/34388#discussion_r2252539640
##########
superset/databases/ssh_tunnel/models.py:
##########
@@ -44,11 +44,19 @@ class SSHTunnel(AuditMixinNullable, ExtraJSONMixin, ImportExportMixin, Model):
id = sa.Column(sa.Integer, primary_key=True)
database_id = sa.Column(
- sa.Integer, sa.ForeignKey("dbs.id"), nullable=False, unique=True
+ sa.Integer,
+ sa.ForeignKey("dbs.id"),
+ nullable=False,
+ unique=True,
)
database: Database = relationship(
"Database",
- backref=backref("ssh_tunnels", uselist=False, cascade="all, delete-orphan"),
+ backref=backref(
+ "ssh_tunnel",
Review Comment:
Singular, since it's 1:1.
--
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
Re: [PR] chore: cleanup ssh tunnel [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #34388:
URL: https://github.com/apache/superset/pull/34388#discussion_r2299426739
##########
superset/commands/database/test_connection.py:
##########
@@ -108,33 +109,37 @@ def run( # noqa: C901
)
)
+ # collect SSH tunnel info
+ ssh_tunnel_properties = self._properties.get("ssh_tunnel")
+ if ssh_tunnel_properties and self._model and self._model.ssh_tunnel:
+ # unmask password while allowing for updated values
+ ssh_tunnel_properties = unmask_password_info(
+ ssh_tunnel_properties,
+ self._model.ssh_tunnel,
+ )
+
+ database: Database | None = None
Review Comment:
Here as well -- do we want to have a server-side validation that SSH FF is enabled?
--
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
Re: [PR] chore: cleanup ssh tunnel [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #34388:
URL: https://github.com/apache/superset/pull/34388#discussion_r2299421097
##########
superset/commands/database/create.py:
##########
@@ -59,6 +57,12 @@ def __init__(self, data: dict[str, Any]):
def run(self) -> Model:
self.validate()
+ if "sqlalchemy_uri" not in self._properties:
+ raise DatabaseRequiredFieldValidationError("sqlalchemy_uri")
Review Comment:
I think this is already covered in `validate`? https://github.com/apache/superset/blob/5ff2b8ca07b8971637c43cee6d8ea4a86c6186cf/superset/commands/database/create.py#L120-L125
--
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
Re: [PR] chore: cleanup ssh tunnel [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #34388:
URL: https://github.com/apache/superset/pull/34388#discussion_r2299427376
##########
superset/commands/database/update.py:
##########
@@ -95,7 +88,7 @@ def run(self) -> Model:
# build new DB
Review Comment:
We prob want to validate that in `update` too.
--
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