Skip to content
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

SSH tunnels not closed after creating them to test DW connections. We should use with-open in SSH tunnel code. #24445

Open
camsaul opened this issue Jul 29, 2022 · 2 comments
Labels
Administration/Databases .Backend Priority:P2 Average run of the mill bug Type:Bug Product defects

Comments

@camsaul
Copy link
Member

camsaul commented Jul 29, 2022

https://github.com/metabase/metabase/blob/master/src/metabase/driver/sql_jdbc/connection.clj#L252-L259

details->connection-spec-for-testing-connection calls driver/incorporate-ssh-tunnel-details which creates a new SSH tunnel:

https://github.com/metabase/metabase/blob/master/src/metabase/util/ssh.clj#L105-L115

However it never calls metabase.util.ssh/close-tunnel!.

I'm guessing this is not causing huge problems IRL since we only test connections when adding new DWs in the admin panel, but it's still bad form to have leaky code like this. So we should fix it.

The SSH session returned by the sshd lib already implements AutoCloseable, so we can use with-open with it -- I think we need to rework our code so we're using with-open when we have an SSH tunnel to make sure we're not accidentally leaking them. That would be easy enough to do in details->connection-spec-for-testing-connection... for the connection pool we'd have to hold it open in some other manner, maybe by having it live in the atom like the c3p0 connection pool. Maybe what we need is a SSHTunnelConnectionPool class that wraps both an SSH tunnel and a c3p0 connection pool so that can live in the same atom as the other things

@camsaul
Copy link
Member Author

camsaul commented Jul 29, 2022

It's too easy to unknowingly create a tunnel and then leak it... driver/incorporate-ssh-tunnel-details (for SQL JDBC at least) creates a new tunnel but doesn't really hint at this anywhere. There's nothing to suggest that people calling that method may need to be doing cleanup when they use it.

@flamber
Copy link
Contributor

flamber commented Jul 30, 2022

Very related to #13456.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Administration/Databases .Backend Priority:P2 Average run of the mill bug Type:Bug Product defects
Projects
None yet
Development

No branches or pull requests

2 participants