SSH tunnel support for S3Store #882

mjwen · 2023-11-07T17:45:16Z

Summary

This PR adds the support to use an ssh tunnel for S3Store, which can be useful for self-hosted MinIO. This follows the same pattern as the ssh tunnel support for MongoStore.

Created a new file ssh_tunnel.py and moved the SSHTunnel class and related stuff there. It's a bit weird to put it in the monogostore file.
Added a new optional parameter local_port for the SSHTunnel class. This is necessary for MinIO, by default which uses the 9000 port. The original implementation that uses an automatically chosen free port will not work in general.
Added ssh_tunnel support for the S3Store.
Added a tutorial doc to use SSHTunnel.

codecov · 2023-11-07T17:49:43Z

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (e31a8ce) 87.76% compared to head (6cae273) 88.16%.
Report is 10 commits behind head on main.

Files	Patch %	Lines
src/maggma/stores/ssh_tunnel.py	71.42%	12 Missing ⚠️
src/maggma/stores/aws.py	85.18%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #882      +/-   ##
==========================================
+ Coverage   87.76%   88.16%   +0.39%     
==========================================
  Files          44       45       +1     
  Lines        3614     3633      +19     
==========================================
+ Hits         3172     3203      +31     
+ Misses        442      430      -12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/maggma/stores/ssh_tunnel.py

+
+def _find_free_port(address="0.0.0.0"):
+    s = socket()
+    s.bind((address, 0))  # Bind to a free port provided by the host.


rkingsbury · 2023-11-07T18:09:02Z

Thanks @mjwen , this looks like a nice enhancement. After a quick review, can I ask for two changes?

Can you please expand the test coverage of SSHTunnel? E.g. add tests that attributes get set correctly, etc.? I'd really like to see test coverage for the patch close to 100%; and in any case I don't want the project test coverage to decrease.
In S3Store.connect, please make sure to implement force_reset in a way that is consistent with other stores (see Store.connect: fix force_reset kwarg implementations #879 ; comments welcome on how that works!)

Finally (and this is a bigger ask), if you'd be able to add a page to the docs describing how to connect via SSH, I'm sure many users would appreciate it. But I understand if that's not possible at this time.

mjwen · 2023-11-07T18:23:51Z

@rkingsbury thanks for the quick feedback!

Great that the functionality looks alright to you. I will add the enhancements (tests and docs) you mentioned above and ping you once ready.

tests/stores/test_aws.py

rkingsbury · 2023-11-21T16:21:43Z

tests/stores/test_ssh_tunnel.py

+def local_port_available(local_port):  # noqa: PT004
+    """Fixture to determine if a local port is available to test the SSH tunnel."""
+    client = paramiko.SSHClient()
+    client.set_missing_host_key_policy(paramiko.AutoAddPolicy())


Would the tests work if you were to set client.set_missing_host_key_policy(RejectPolicy) ? If so, please do that. If not, it's OK b/c this is just a test file so I'm not so concerned about security issues like this.

See here for more details: https://github.com/materialsproject/maggma/security/code-scanning/138

mjwen · 2023-11-19T22:02:59Z

@rkingsbury I've addressed your comments. Sorry for the delay.

A couple of stuff to consider before we think about merging it in?

Seems the test GH action is not set up to enable database server, e.g. localhost:22 cannot be access. As a results, many of the tests (old and the new ones I've added) involving database are skipped. Then the coverage tests cannot cover them as well. We should somehow enable this? I've no idea how to do it in GH action. @jmmshn might have some experience?
I did not find anything on automatic docs build and publish in the docs directory. Is it located somewhere else? The current docs seem old -- some of the md pages are now included, e.g. the docs on FileStore you've added.

jmmshn · 2023-11-21T00:18:58Z

Is there some way to spoof the ssh_tunnel connection?
I think paramiko should not actually be called during any of the CI just like how we are using moto to mock all the bots connections.

So I think if the basic tests are designed only in a way to check the arguments to paramiko then we are OK.

For more complex tests, you can consider mocking the entry functions to the ssh_tunnel and have that turn some kind of a flag, then you just have to check that connecting to a DB properly hits that flag?
Basically, I think our CI should only check that settings are properly passed to SSH tunnel API but we don't need to actually check the SSH tunnels.

rkingsbury · 2023-11-21T01:21:32Z

Thanks @mjwen !

1. Seems the test GH action is not set up to enable database server, e.g. localhost:22 cannot be access. As a results, many of the tests (old and the new ones I've added) involving database are skipped. Then the coverage tests cannot cover them as well. We should somehow enable this? I've no idea how to do it in GH action. @jmmshn might have some experience?

I'll defer to Jimmy on this one, and I think the approach he suggested sounds ok (just make sure we are passing the right arguments, not testing the tunnel itself)

2. I did not find anything on automatic docs build and publish in the [docs](https://github.com/materialsproject/maggma/tree/main/docs) directory. Is it located somewhere else? The [current docs](https://materialsproject.github.io/maggma/) seem old -- some of the md pages are now included, e.g. the docs on [FileStore](https://github.com/materialsproject/maggma/blob/main/docs/getting_started/using_file_store.md) you've added.

I'm not sure I follow - as far as I can tell, the current docs you linked are up to date. I think you just need to add modify a .md file in docs/reference (or wherever you think).

I guess the docs build process isn't document, but we use mkdocs. So if you want to preview your changes, you can run mkdocs serve and then it should serve your documentation in the browser, and refresh in real time as you edit. Does that help?

mjwen · 2023-11-21T01:40:01Z

@rkingsbury My bad regarding the docs! I was looking for some configure file in the docs directory, but now I see mkdocs.yml is in the project root directory. Updated.

mjwen · 2023-11-21T02:19:52Z

@jmmshn Thanks for the suggestions! Sounds like a good compromise for the CI tests. I've added some fake tunnel to test the functionality s3store.

mjwen · 2023-11-21T02:28:08Z

@rkingsbury Ready for final review.

Note, the CI won't test some of the attributes assignments of the SSHTunnel because of the way we are setting it up.

Also, the CI tests seem fail. I've taken a quick look at it -- seems related to pyproject.toml and setup.py. A rerun might work?

rkingsbury

Thanks @mjwen , looks good. I just have a few minor typo edits to suggest.

Apologies for the CI issues; apparently there was a setuptools update or something that broke the tests. They should be working now if you pull the latest.

tests/stores/test_aws.py

docs/getting_started/using_ssh_tunnel.md

rkingsbury · 2023-11-21T16:21:43Z

tests/stores/test_ssh_tunnel.py

+def local_port_available(local_port):  # noqa: PT004
+    """Fixture to determine if a local port is available to test the SSH tunnel."""
+    client = paramiko.SSHClient()
+    client.set_missing_host_key_policy(paramiko.AutoAddPolicy())


Would the tests work if you were to set client.set_missing_host_key_policy(RejectPolicy) ? If so, please do that. If not, it's OK b/c this is just a test file so I'm not so concerned about security issues like this.

See here for more details: https://github.com/materialsproject/maggma/security/code-scanning/138

…sshtunnel

mjwen · 2023-11-21T20:25:28Z

I've checked client.set_missing_host_key_policy a bit, and it seems client.set_missing_host_key_policy(RejectPolicy()) is the default and it won't work with the tests, because we need to load the host key info for the tests.

I believe we need to leave it like that.

rkingsbury · 2023-11-21T21:03:14Z

Thanks @mjwen !

mjwen added 4 commits November 7, 2023 09:59

Move ssh tunnel to a new file

58fd7fb

Add new local_port arg, to allow S3 9000 port

c6c98ab

Add sshtunnel support for S3Store

ea77d0f

Run pre-commit

e8fcac6

github-advanced-security bot found potential problems Nov 7, 2023

View reviewed changes

src/maggma/stores/ssh_tunnel.py

def _find_free_port(address="0.0.0.0"):

s = socket()

s.bind((address, 0)) # Bind to a free port provided by the host.

Check warning

Code scanning / CodeQL

Binding a socket to all network interfaces Medium

'0.0.0.0' binds a socket to all interfaces.

mjwen mentioned this pull request Nov 7, 2023

Allow SSHTunnel in job stores materialsproject/jobflow#477

Merged

mjwen added 6 commits November 10, 2023 17:11

Enable force_reset for connect

e51d006

Fix some doc string

6db3d02

Fix to pass original S3 tests

92f807b

Add test force_reset

b730130

Add test for local_port for ssh tunnel

814ad40

Add test S3 store wit h ssh tunnel

0b0d2da

github-advanced-security bot found potential problems Nov 19, 2023

View reviewed changes

mjwen added 3 commits November 19, 2023 15:47

Add brief doc on using SSHTunnel

bf56d2a

Remove unnecessary assignment

afa1050

explicit catch error

aa38775

Add sshtunnel to mkdocs.yml

07d3df9

Add a fake ssh tunnel to test S3Store

cc048d1

Merge branch 'main' into feat_s3_sshtunnel

d31a054

rkingsbury requested changes Nov 21, 2023

View reviewed changes

rkingsbury added the release:minor label Nov 21, 2023

rkingsbury added enhancement docs documentation labels Nov 21, 2023

rkingsbury self-assigned this Nov 21, 2023

rkingsbury and others added 3 commits November 21, 2023 11:54

Merge branch 'main' into feat_s3_sshtunnel

8308ad7

Fix typo

31c6ce7

Merge remote-tracking branch 'origin/feat_s3_sshtunnel' into feat_s3_…

6cae273

…sshtunnel

rkingsbury merged commit f792dd8 into materialsproject:main Nov 21, 2023
8 of 10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SSH tunnel support for S3Store #882

SSH tunnel support for S3Store #882

mjwen commented Nov 7, 2023 •

edited

codecov bot commented Nov 7, 2023 •

edited

rkingsbury commented Nov 7, 2023

mjwen commented Nov 7, 2023

rkingsbury Nov 21, 2023

mjwen commented Nov 19, 2023 •

edited

jmmshn commented Nov 21, 2023

rkingsbury commented Nov 21, 2023

mjwen commented Nov 21, 2023

mjwen commented Nov 21, 2023

mjwen commented Nov 21, 2023

rkingsbury left a comment

rkingsbury Nov 21, 2023

mjwen commented Nov 21, 2023

rkingsbury commented Nov 21, 2023

SSH tunnel support for S3Store #882

SSH tunnel support for S3Store #882

Conversation

mjwen commented Nov 7, 2023 • edited

Summary

codecov bot commented Nov 7, 2023 • edited

Codecov Report

rkingsbury commented Nov 7, 2023

mjwen commented Nov 7, 2023

rkingsbury Nov 21, 2023

Choose a reason for hiding this comment

mjwen commented Nov 19, 2023 • edited

jmmshn commented Nov 21, 2023

rkingsbury commented Nov 21, 2023

mjwen commented Nov 21, 2023

mjwen commented Nov 21, 2023

mjwen commented Nov 21, 2023

rkingsbury left a comment

Choose a reason for hiding this comment

rkingsbury Nov 21, 2023

Choose a reason for hiding this comment

mjwen commented Nov 21, 2023

rkingsbury commented Nov 21, 2023

mjwen commented Nov 7, 2023 •

edited

codecov bot commented Nov 7, 2023 •

edited

mjwen commented Nov 19, 2023 •

edited