Add FTP support to jobstores #5142

stxue1 · 2024-10-25T19:35:23Z

Also turns all url_exists requests and size requests to HEAD requests instead, as mentioned here: #5114 (comment)

In my local testing, this significantly improves FTP performance. (Previously an existence check on a small text file on an insecure FTP server took around 5 seconds, though I'm unsure why it would take so long originally)

Changelog Entry

To be copied to the draft changelog by merger:

Added proper FTP support for jobstores
URL existence and size gets/checks are now done with HEAD requests

Reviewer Checklist

Make sure it is coming from issues/XXXX-fix-the-thing in the Toil repo, or from an external repo.
- If it is coming from an external repo, make sure to pull it in for CI with:
```
contrib/admin/test-pr otheruser theirbranchname issues/XXXX-fix-the-thing
```
- If there is no associated issue, create one.
Read through the code changes. Make sure that it doesn't have:
- Addition of trailing whitespace.
- New variable or member names in camelCase that want to be in snake_case.
- New functions without type hints.
- New functions or classes without informative docstrings.
- Changes to semantics not reflected in the relevant docstrings.
- New or changed command line options for Toil workflows that are not reflected in docs/running/{cliOptions,cwl,wdl}.rst
- New features without tests.
Comment on the lines of code where problems exist with a review comment. You can shift-click the line numbers in the diff to select multiple lines.
Finish the review with an overall description of your opinion.

Merger Checklist

Make sure the PR passes tests.
Make sure the PR has been reviewed since its last modification. If not, review it.
Merge with the Github "Squash and merge" feature.
- If there are multiple authors' commits, add Co-authored-by to give credit to all contributing authors.
Copy its recommended changelog entry to the Draft Changelog.
Append the issue number in parentheses to the changelog entry.

stxue1 · 2024-10-25T19:36:21Z

src/toil/jobStores/abstractJobStore.py

+    """
+    FTP access with upload.
+
+    Taken and modified from https://github.com/ohsu-comp-bio/cwl-tes/blob/03f0096f9fae8acd527687d3460a726e09190c3a/cwl_tes/ftp.py#L8


The license at cwl-tes is also Apache 2.0. Should I add a section to the Toil license that specifically states all code under this class is under cwl-tes's Apache 2.0 license?

You don't need to repeat the license.

If there are code quotes, then you could mention the copyright of the original authors / copyright holders:

Copyright 2018 EMBL-EBI <[email protected]> (Michael R. Crusoe's original implementation, written under contract) Copyright 2019 Adam Struck <[email protected]> Copyright 2020 Joris Vankerschaver <[email protected]> Copyright 2020 Dab Brill <[email protected]> Copyright 2020 Manabu ISHII <[email protected]>

Otherwise if this implementation is different enough, you don't need to mention the authors.

I suggest this metric: if we wanted to change the license of Toil, would you have to get permission from these copyright holders because the code is essentially the same? Or is the code so adapted or different for other reasons that it is a new work?

One approach is just add their Apache 2.0 license's copyright line to Toil's LICENSE file along with the others.

You could also drop it in a different file and import it to here, and in that file have Copyright 2017 Oregon Health and Science University and the copyright boilerplate we use for our files. I think.

Also the #8 at the end of this link is pointing to the wrong line number.

I think it's easier to do the latter by putting the code in a separate file, so I'll do that

docs/appendices/environment_vars.rst

adamnovak

I think we need some more documentation about how FTP-over-SSL (which I think is rather unusual) is being used here, and how we're handling non-SSL FTP servers vs. those without certificates (which themselves might actually be downgrade attacks).

We need to write to a user who doesn't necessarily know that FTP can happen over SSL.

adamnovak · 2024-10-30T21:50:00Z

src/toil/jobStores/abstractJobStore.py

+    """
+    FTP access with upload.
+
+    Taken and modified from https://github.com/ohsu-comp-bio/cwl-tes/blob/03f0096f9fae8acd527687d3460a726e09190c3a/cwl_tes/ftp.py#L8


One approach is just add their Apache 2.0 license's copyright line to Toil's LICENSE file along with the others.

You could also drop it in a different file and import it to here, and in that file have Copyright 2017 Oregon Health and Science University and the copyright boilerplate we use for our files. I think.

Also the #8 at the end of this link is pointing to the wrong line number.

adamnovak · 2024-10-30T21:51:56Z

src/toil/jobStores/abstractJobStore.py

+    def __init__(
+        self, cache: Optional[dict[Any, ftplib.FTP]] = None, insecure: bool = False
+    ):


We should have docstrings for this and all the other functions in the class, ideally. It's especially important with a parameter called insecure; what is not being secured, and what is it not being secured from? Does it mean we're vulnerable to password sniffing and someone tampering with the FTP data in transit, or are we now executing arbitrary code from every file we download, or what?

adamnovak · 2024-10-30T21:56:22Z

src/toil/jobStores/abstractJobStore.py

+                user = env_user
+            if env_passwd:
+                passwd = env_passwd
+            ftp.login(user or "", passwd or "", secure=not self.insecure)


Is this using an undocumented secure parameter? What does that do?

ftp in this case is an FTP_TLS object, which supports connecting over TLS. It's not on the official FTP_TLS python documentation, nor is there an associated docstring, but the code itself simply sets up an SSL connection to the FTP server

docs/appendices/environment_vars.rst

…pyright license in

adamnovak

I think we should cut the feature where we require user approval for an insecure FTP connection. (We might also want to then cut/flag off .netrc support?) If we do implement that, we also need to implement it for the actual requests, not just pass off to urllib.

I also think we should address the cached FTP connections that seem to live forever. Maybe we want them to time out somehow? Or maybe we can just bypass the cache or not store a persistent FtpFsAccess in a class-level variable.

adamnovak · 2024-11-01T18:59:38Z

src/toil/jobStores/abstractJobStore.py

+            cls._setup_ftp()
+            # mypy is unable to understand that ftp must exist by this point
+            assert cls.ftp is not None


If you structured this as a method that returns an FtpFsAccess and not one that has a side effect of setting a class-level field, you wouldn't need all these asserts and you also wouldn't need to remember two member names.

adamnovak · 2024-11-01T19:00:33Z

src/toil/jobStores/abstractJobStore.py

+    ftp = None
+
+    @classmethod
+    def _setup_ftp(cls) -> None:


It's weird that the ftp member is public, but the function that must be called before it is actually used has an underscore prefix.

adamnovak · 2024-11-01T19:01:06Z

src/toil/jobStores/ftp_utils.py

This might be better placed in src/toil/lib.

adamnovak · 2024-11-01T19:05:52Z

src/toil/jobStores/abstractJobStore.py

@@ -1872,15 +1874,28 @@ class JobStoreSupport(AbstractJobStore, metaclass=ABCMeta):
    stores.
    """

+    ftp = None


If we keep a class-level FtpFsAccess instance that is never destroyed, and the FtpFsAccess instance caches open FTP connections to servers, when do we close our FTP connections? It might be never, and that's probably not what we want.

adamnovak · 2024-11-01T19:07:12Z

src/toil/jobStores/ftp_utils.py

+        :param cache: cache of generated FTP objects
+        :param insecure: Whether to connect over FTP with TLS
+        """
+        self.cache = cache or {}


We probably want to be able to turn off using a connection cache at all, if we're going to make an FtpFsAccess that is never destroyed.

adamnovak · 2024-11-01T19:29:08Z

src/toil/jobStores/ftp_utils.py

+                user = env_user
+            if env_passwd:
+                passwd = env_passwd
+            ftp.login(user or "", passwd or "", secure=not self.insecure)


OK, so according to the obviously reliable https://tedboy.github.io/python_stdlib/_modules/ftplib.html#FTP_TLS.login what secure does here is call auth() if you haven't already set up SSL/TLS on the connection, before doing the login.

So it looks like unless the user sets TOIL_FTP_USE_SSL=False themselves, they're going to get an error here whenever they use an ftp:// URL, unless the server actually supports SSL/TLS.

One server people might want to use is ftp://ftp-trace.ncbi.nlm.nih.gov. I've just checked and it doesn't support FTP over SSL/TLS.

Another thing we don't do is call the prot_p() method on the connection to actually secure the data channel. And later we fall back to urlopen() and give it the full FTP URL with username and password, and I don't think that urlopen() can use or enforce SSL. We'd have to tinker with urlopen's handler as recommended in https://stackoverflow.com/a/73171311.

So I think that by default we should allow unencrypted FTP connections without user intervention to accept them. That's the current behavior.

If we want to change Toil to enforce secure FTP by default, then we also need to enforce security on the actual reads. And we probably would need a way to individually allow particular insecure servers?

…ssues/5134-ftp-get-size

adamnovak

The promised SSL enforcement doesn't actually work yet because of the fallback to urlopen().

adamnovak · 2024-11-20T16:40:30Z

src/toil/jobStores/abstractJobStore.py

@@ -1872,18 +1874,29 @@ class JobStoreSupport(AbstractJobStore, metaclass=ABCMeta):
    stores.
    """

+    @classmethod
+    def _setup_ftp(cls) -> FtpFsAccess:
+        return FtpFsAccess(insecure=strtobool(os.environ.get('TOIL_FTP_USE_SSL', 'False')) is False)


Now it looks like we don't reuse FTP connections at all. But I guess that's fine? There's nothing like a time-based @memoize; we'd need a thread to watch it and that would be a pain.

I can leave a comment that this doesn't reuse FTP connections, and that implementing it later may be a good idea

adamnovak · 2024-11-20T16:50:54Z

src/toil/lib/ftp_utils.py

+        """
+        if "r" in mode:
+            host, port, user, passwd, path = self._parse_url(fn)
+            handle = urlopen("ftp://{}:{}@{}:{}/{}".format(user, passwd, host, port, path))


This doesn't respect TOIL_FTP_USE_SSL.

If we implemented open() on top of https://docs.python.org/3/library/ftplib.html#ftplib.FTP.retrbinary and supporting files larger than memory, we'd need some kind of pipe file object and a thread to feed the data into it.

We have some pipe implementations over in

toil/src/toil/jobStores/utils.py

Line 153 in a5657cf

class ReadablePipe(ABC):

but they insist on driving the control flow to simplify cleanup; a real open() needs to produce a file object that Just Works and have all the cleanup happen automatically when it goes out of scope and is closed. We might need to use closing() to help with that?

Removed to put into another issue as it appears to be too large of an issue

adamnovak

Without the SSL enforcement feature I think this looks OK.

stxue1 added 3 commits October 25, 2024 11:43

Add ftp support

195fdc8

FTP support

176381c

strtobool for hardcoded strings for AWS env vars

af46642

stxue1 commented Oct 25, 2024

View reviewed changes

stxue1 changed the title ~~Issues/5134 ftp get size~~ Add FTP support to jobstores Oct 25, 2024

typo

1ea4e3d

stxue1 commented Oct 25, 2024

View reviewed changes

docs/appendices/environment_vars.rst Outdated Show resolved Hide resolved

github-actions bot added 2 commits October 28, 2024 20:30

Merge master into issues/5134-ftp-get-size

65e5564

Merge master into issues/5134-ftp-get-size

5ba00b2

adamnovak requested changes Oct 30, 2024

View reviewed changes

github-actions bot and others added 5 commits October 31, 2024 15:13

Merge master into issues/5134-ftp-get-size

35fbe08

Change envvar to USE_SSL and move to separate file to put original co…

754b43c

…pyright license in

Forgot to add file

5ed71cf

missing space in rst

17536ba

Merge master into issues/5134-ftp-get-size

13f4ac5

adamnovak requested changes Nov 1, 2024

View reviewed changes

github-actions bot and others added 14 commits November 5, 2024 07:11

Merge master into issues/5134-ftp-get-size

f234825

Don't keep FTP objects around forever and disable FTP SSL by default

0f38d58

Merge master into issues/5134-ftp-get-size

bc5da85

Merge master into issues/5134-ftp-get-size

665e3bd

Merge master into issues/5134-ftp-get-size

a7455a0

Merge master into issues/5134-ftp-get-size

7d31e54

Merge master into issues/5134-ftp-get-size

d2da46c

Fix HTTP errors + add prot_p in secure mode

a2105af

Merge remote-tracking branch 'origin/issues/5134-ftp-get-size' into i…

66d3587

…ssues/5134-ftp-get-size

Always try to use SSL before falling back, raising if user enforces SSL

9f86277

Change environment_vars.rst to reflect enforcement

91f1cbd

add proper port suport

5884417

Merge master into issues/5134-ftp-get-size

f490ed3

Merge master into issues/5134-ftp-get-size

bbd321a

Merge master into issues/5134-ftp-get-size

59e3fea

adamnovak requested changes Nov 20, 2024

View reviewed changes

Remove FTP SSL support and add comments

8fe81cc

stxue1 mentioned this pull request Nov 20, 2024

Support FTP over SSL #5167

Open

Remove envvar

f499caf

adamnovak self-requested a review November 22, 2024 18:08

adamnovak approved these changes Nov 22, 2024

View reviewed changes

adamnovak merged commit f3e98db into master Nov 22, 2024
3 checks passed

adamnovak deleted the issues/5134-ftp-get-size branch November 22, 2024 21:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FTP support to jobstores #5142

Add FTP support to jobstores #5142

stxue1 commented Oct 25, 2024 •

edited

Loading

stxue1 Oct 25, 2024

mr-c Oct 30, 2024 •

edited

Loading

adamnovak Oct 30, 2024

stxue1 Oct 31, 2024

adamnovak left a comment

adamnovak Oct 30, 2024

adamnovak Oct 30, 2024

adamnovak Oct 30, 2024

stxue1 Oct 31, 2024

adamnovak left a comment

adamnovak Nov 1, 2024

adamnovak Nov 1, 2024

adamnovak Nov 1, 2024

adamnovak Nov 1, 2024

adamnovak Nov 1, 2024

adamnovak Nov 1, 2024

adamnovak left a comment

adamnovak Nov 20, 2024

stxue1 Nov 20, 2024

adamnovak Nov 20, 2024

stxue1 Nov 20, 2024

adamnovak left a comment

Add FTP support to jobstores #5142

Add FTP support to jobstores #5142

Conversation

stxue1 commented Oct 25, 2024 • edited Loading

Changelog Entry

Reviewer Checklist

Merger Checklist

Choose a reason for hiding this comment

mr-c Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamnovak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamnovak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamnovak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamnovak left a comment

Choose a reason for hiding this comment

stxue1 commented Oct 25, 2024 •

edited

Loading

mr-c Oct 30, 2024 •

edited

Loading