-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
feat(ssh-tunnelling): Setup SSH Tunneling Commands for Database Connections #21912
Changes from 9 commits
830a283
2c1e736
f78df83
d482df4
9edb581
da27d8f
773a6c8
2f2dda2
fd0d7f2
158da8d
face73f
95d079e
d5926e3
f7a6a41
30e380a
87c0d79
11b240b
1bfdbda
4146d5a
f8b877d
54fc147
fdc6ca3
66c0801
1f9ec5e
c698cf4
41bd19b
8811a99
82d7532
1f829ac
d53d116
752161d
8c4b081
1a19a97
58b9cce
0ac6fb1
31f3c1d
a0b30e6
e089a8d
81b2f88
45686b7
7ce5836
d9c8d0d
ec27b80
65e3e29
9fa9db5
1a11ff4
3f0dae1
2777807
8ed02cd
6a68147
6bd32e8
8a3ee35
bc89194
adb9451
16d960b
d2ab4a6
fb2acd0
4d807c9
dc0c848
21fcdf0
68cb75f
44ca56b
7e1461e
92e41f1
6c59663
466703a
554de53
4448739
bb78055
f507385
45aa022
3d3b71b
86436b6
54a8d7f
3f6afec
7625566
0578a8e
ec20429
1f57d4a
ed19a3e
852c8bb
be5c005
948f748
c636ce7
908896f
e3ef835
c5c50ed
06e115b
13ed50d
54d51e2
53eaa63
8f8faff
607c682
1ea0e8b
7cc7bc8
7c539d2
394afc1
9b09fc7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
# Licensed to the Apache Software Foundation (ASF) under one | ||
# or more contributor license agreements. See the NOTICE file | ||
# distributed with this work for additional information | ||
# regarding copyright ownership. The ASF licenses this file | ||
# to you under the Apache License, Version 2.0 (the | ||
# "License"); you may not use this file except in compliance | ||
# with the License. You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, | ||
# software distributed under the License is distributed on an | ||
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
# KIND, either express or implied. See the License for the | ||
# specific language governing permissions and limitations | ||
# under the License. | ||
|
||
import sqlalchemy as sa | ||
from flask_appbuilder import Model | ||
from sqlalchemy.orm import backref, relationship | ||
from sqlalchemy_utils import EncryptedType | ||
|
||
from superset import app | ||
from superset.models.core import Database | ||
from superset.models.helpers import ( | ||
AuditMixinNullable, | ||
ExtraJSONMixin, | ||
ImportExportMixin, | ||
) | ||
|
||
app_config = app.config | ||
|
||
|
||
class SSHTunnelCredentials( | ||
Model, AuditMixinNullable, ExtraJSONMixin, ImportExportMixin | ||
): | ||
""" | ||
A ssh tunnel configuration in a database. | ||
""" | ||
|
||
__tablename__ = "ssh_tunnel_credentials" | ||
|
||
id = sa.Column(sa.Integer, primary_key=True) | ||
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"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a one-to-one, correct? If so, I think you'll need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to rename this to be |
||
foreign_keys=[database_id], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think you need this foreign_key here since you declared it in the database_id column |
||
) | ||
|
||
server_address = sa.Column(EncryptedType(sa.String, app_config["SECRET_KEY"])) | ||
server_port = sa.Column(EncryptedType(sa.String, app_config["SECRET_KEY"])) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be |
||
username = sa.Column(EncryptedType(sa.String, app_config["SECRET_KEY"])) | ||
|
||
# basic authentication | ||
password = sa.Column( | ||
EncryptedType(sa.String, app_config["SECRET_KEY"]), nullable=True | ||
) | ||
|
||
# password protected pkey authentication | ||
private_key = sa.Column( | ||
EncryptedType(sa.String, app_config["SECRET_KEY"]), nullable=True | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is more of a question than a suggestion, but I see that we're using |
||
) | ||
private_key_password = sa.Column( | ||
EncryptedType(sa.String, app_config["SECRET_KEY"]), nullable=True | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
# Licensed to the Apache Software Foundation (ASF) under one | ||
# or more contributor license agreements. See the NOTICE file | ||
# distributed with this work for additional information | ||
# regarding copyright ownership. The ASF licenses this file | ||
# to you under the Apache License, Version 2.0 (the | ||
# "License"); you may not use this file except in compliance | ||
# with the License. You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, | ||
# software distributed under the License is distributed on an | ||
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
# KIND, either express or implied. See the License for the | ||
# specific language governing permissions and limitations | ||
# under the License. | ||
"""create_ssh_tunnel_credentials_tbl | ||
|
||
Revision ID: f3c2d8ec8595 | ||
Revises: deb4c9d4a4ef | ||
Create Date: 2022-10-20 10:48:08.722861 | ||
|
||
""" | ||
|
||
# revision identifiers, used by Alembic. | ||
revision = "f3c2d8ec8595" | ||
down_revision = "deb4c9d4a4ef" | ||
|
||
from uuid import uuid4 | ||
|
||
import sqlalchemy as sa | ||
from alembic import op | ||
from sqlalchemy_utils import EncryptedType, UUIDType | ||
|
||
from superset import app | ||
|
||
app_config = app.config | ||
|
||
|
||
def upgrade(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 :) |
||
op.create_table( | ||
"ssh_tunnel_credentials", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this means user will still be creating a |
||
# AuditMixinNullable | ||
sa.Column("created_on", sa.DateTime(), nullable=True), | ||
eschutho marked this conversation as resolved.
Show resolved
Hide resolved
|
||
sa.Column("changed_on", sa.DateTime(), nullable=True), | ||
sa.Column("created_by_fk", sa.Integer(), nullable=True), | ||
sa.Column("changed_by_fk", sa.Integer(), nullable=True), | ||
# ExtraJSONMixin | ||
sa.Column("extra_json", sa.Text(), nullable=True), | ||
# ImportExportMixin | ||
sa.Column("uuid", UUIDType(binary=True), primary_key=False, default=uuid4), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we put a uniqueness constraint on this column? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @betodealmeida do we need to put a constaint on these, in the ImportExportMixin columns i didn't a constraint being made so didn't feel like it was necessary There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you ever look up by uuid? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no right now, but this is mostly for export/import if we wanted to have ssh tunnels exported with the db credentials
hughhhh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# SSHTunnelCredentials | ||
sa.Column("database_id", sa.INTEGER(), sa.ForeignKey("dbs.id"), nullable=True), | ||
sa.Column("server_address", EncryptedType(sa.String, app_config["SECRET_KEY"])), | ||
sa.Column("server_port", EncryptedType(sa.String, app_config["SECRET_KEY"])), | ||
sa.Column( | ||
"username", | ||
EncryptedType(sa.String, app_config["SECRET_KEY"]), | ||
), | ||
sa.Column( | ||
"password", | ||
EncryptedType(sa.String, app_config["SECRET_KEY"]), | ||
nullable=True, | ||
), | ||
sa.Column( | ||
"private_key", | ||
EncryptedType(sa.String, app_config["SECRET_KEY"]), | ||
nullable=True, | ||
), | ||
sa.Column( | ||
"private_key_password", | ||
EncryptedType(sa.String, app_config["SECRET_KEY"]), | ||
nullable=True, | ||
), | ||
sa.PrimaryKeyConstraint("id"), | ||
) | ||
|
||
|
||
def downgrade(): | ||
op.drop_table("ssh_tunnel_credentials") |
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.
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.