feat(ssh_tunnel): APIs for SSH Tunnels #22199
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Antonio-RiveroMartnez
changed the title
Codecov Report
@@ Coverage Diff @@
## create-sshtunnelconfig-tbl #22199 +/- ##
===============================================================
- Coverage 66.97% 55.78% -11.20%
===============================================================
Files 1858 1859 +1
Lines 70928 71006 +78
Branches 7764 7764
===============================================================
- Hits 47506 39610 -7896
- Misses 21400 29374 +7974
Partials 2022 2022
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Antonio-RiveroMartnez
changed the title
hughhhh
reviewed
Nov 28, 2022
hughhhh
reviewed
Nov 29, 2022
Antonio-RiveroMartnez
force-pushed
the
api_ssh_tunnel
branch
from
e02d44b
to
cead0e6
Compare
hughhhh
force-pushed
the
create-sshtunnelconfig-tbl
branch
2 times, most recently
from
225ea11
to
466703a
Compare
eschutho
reviewed
Nov 30, 2022
eschutho
reviewed
Nov 30, 2022
| .filter(SSHTunnel.database_id == response.get("id")) | ||
| .one_or_none() | ||
| ) | ||
| assert model_ssh_tunnel is None |
eschutho
reviewed
Nov 30, 2022
eschutho
reviewed
Nov 30, 2022
|
/testenv up |
|
@hughhhh Ephemeral environment spinning up at http://35.90.30.149:8080. Credentials are |
Antonio-RiveroMartnez
force-pushed
the
api_ssh_tunnel
branch
3 times, most recently
from
8889387
to
1f3499e
Compare
|
Ephemeral environment shutdown and build artifacts deleted. |
Antonio-RiveroMartnez
force-pushed
the
api_ssh_tunnel
branch
from
99f8cc6
to
4edd673
Compare
15 tasks
Antonio-RiveroMartnez
force-pushed
the
api_ssh_tunnel
branch
from
58a52e2
to
ace19a2
Compare
- Add not found test when deleting the tunnel
- FIx linting errors
- Fix pre-commit errors
- Remove bind properties from schema used in TestConnection - Add test for SSH Tunnel failure and no DB creation
- Make server_address, server_port and username required fields for our SSHTunnel schema - Update the tests so we can consider new message with required fields
- Changes from Code review feedback - Solve conflict with base branch
- Remove safe decorator so SIP-41 errors can be returned
- Fix update tests
- Mask passwords for SSH when creating a new one - Fix arg name in test connection
- Use SSHTunnel commands instead of DAOs - Update tests - Fix linting and pre-commit
- Flush only if creating a ssh tunnel
- Do not return passwords for SSH Tunnel in any API call
- Using nested transactions so we can rollback if anything fails
Antonio-RiveroMartnez
force-pushed
the
api_ssh_tunnel
branch
from
36d428f
to
a5cf0e4
Compare
- Extend GET endpoint on Database APi so it includes SSH Tunnel if any
9 tasks
hughhhh
reviewed
Jan 3, 2023
superset/databases/api.py
Outdated
| --- | ||
| get: | ||
| description: >- | ||
| Get a database with its SSH Tunnel if any |
hughhhh
reviewed
Jan 3, 2023
superset/databases/api.py
Outdated
| @@ -280,6 +327,12 @@ def post(self) -> FlaskResponse: | |||
| if new_model.driver: | |||
| item["driver"] = new_model.driver | |||
|
|
|||
| # Return SSH Tunnel and hide passwords if any | |||
| if item.get("ssh_tunnel"): | |||
| item["ssh_tunnel"] = new_model.ssh_tunnel # pylint: disable=no-member | |||
There was a problem hiding this comment.
create a util function that does the popping/deletion and returns the object without those values
There was a problem hiding this comment.
Sure! I'll indeed use our mask as I did here: https://github.com/apache/superset/pull/22513/files#diff-f9deb9b63f5f2db38b811ff092cf90b0d4d558223f743bbbb5b203c203d2d590R336 but sure, let me add that util. Thanks!
hughhhh
reviewed
Jan 3, 2023
superset/databases/api.py
Outdated
| @@ -361,6 +414,11 @@ def put(self, pk: int) -> Response: | |||
| item["sqlalchemy_uri"] = changed_model.sqlalchemy_uri | |||
| if changed_model.parameters: | |||
| item["parameters"] = changed_model.parameters | |||
| # Return SSH Tunnel and hide passwords if any | |||
| if item.get("ssh_tunnel"): | |||
| item["ssh_tunnel"] = changed_model.ssh_tunnel | |||
There was a problem hiding this comment.
use the util function here as well
hughhhh
requested changes
Jan 3, 2023
| try: | ||
| # So database.id is not None | ||
| db.session.flush() | ||
| CreateSSHTunnelCommand(database.id, ssh_tunnel_properties).run() |
There was a problem hiding this comment.
we need to have a reference for ssh_tunnel after it's created so it can pull down all the schema
There was a problem hiding this comment.
Good catch, let me add it there! Thanks
- Add util method to mask password - Adjust tests to consider password masking
- Remove the extra description on GET endpoint for databases
- Fix pre-commit error
hughhhh
approved these changes
Jan 3, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
Add our CREATE, UPDATE and DELETE APIs for SSH Tunnels.
POST /api/v1/databaseUPDATE /api/v1/database/DELETE /api/v1/database/{db_id}/ssh_tunnelBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION