Repository navigation
Support web addresses in preload - #3755
Conversation
This commit does two things:
1. We support passing web addresses as preloads,
allowing for for functionality like the following:
dask-scheduler --preload http://my-web-address/myfile.py
2. We refactor preloads into a class structure.
I'm usually against this, but I think that in this case it cleans
things up.
I think that it will also make it easier to rewrite how we handle
preload and preload_argv in configuration later
| async with Scheduler(preload=["http://localhost:12345/preload"]) as s: | ||
| assert s.foo == 1 | ||
| finally: | ||
| server.stop() |
There was a problem hiding this comment.
This was the primary motivation for this PR
There was a problem hiding this comment.
I actually don't see the motivation for this, can you expand? exec'ing code that is loaded from an http response seems like a risky thing to enable users to do - without proper security steps to prevent MITM attacks, this could load and exec a malicious script.
jcrist
left a comment
jcrist
left a comment
There was a problem hiding this comment.
If I understand correctly, the only user-facing change here is support for getting preload scripts from a remote endpoint? The class refactor is internal only?
| self.argv = argv | ||
| self.file_dir = file_dir | ||
|
|
||
| self.module = _import_module(name, file_dir) |
There was a problem hiding this comment.
I would move this to under start, no reason to do it here afaict.
There was a problem hiding this comment.
In between now and start lots of decisions are made. For example, it's useful to import the module before we decide on protocol (this is an old request from the ucx folks, who want to import ucx to depend on the side effects of loading "ucx://" into the registry of protocols before we try to use that protocol.
There was a problem hiding this comment.
Then maybe set this to None in the case of a remote address? Creating and stashing coroutine that is later run seems like a bit convoluted logic that may be cleaner if the logic wasn't split between __init__ and start. idk.
There was a problem hiding this comment.
Yeah, I agree that it's a little convoluted. Worse is that behavior for web-downloaded preloads is different than those for file-loaded ones (we don't get on-import behavior for web-downloaded preloads). It's a choice between this and poor testing though, and at least for now I'd prefer to optimize for testability.
In terms of how to handle the magic coroutine I don't really have a good answer here, other than to store state about if it's a web address, and to explicitly call this out in def start. I'll do that now. I guess we're being more explicit, which is good.
There was a problem hiding this comment.
OK, we're now more explicit. The behavior is the same, but things are hopefully a bit cleaner.
| async with Scheduler(preload=["http://localhost:12345/preload"]) as s: | ||
| assert s.foo == 1 | ||
| finally: | ||
| server.stop() |
There was a problem hiding this comment.
I actually don't see the motivation for this, can you expand? exec'ing code that is loaded from an http response seems like a risky thing to enable users to do - without proper security steps to prevent MITM attacks, this could load and exec a malicious script.
At this point they're running preload scripts which run arbitrary Python code anyway. If they want to avoid MITM attacks then they could use https? |
|
Right, but I'd argue many users probably won't bother, which exposes them. I think we don't want to enable poor security practices without good reason. Can you provide a motivating example for this feature? |
Dask gateway could run user-provided docker images, but append |
Correct. I started doing this in order to switch out the preload/preload_argv to a single preloads config value, but decided that was a bit too much for this PR. I still think that it's a good move though. |
jrbourbeau
left a comment
jrbourbeau
left a comment
There was a problem hiding this comment.
This section, specifically m["dask_setup"], is raising an error based on the changes in c5769dc. That line is still expecting _import_module to return a dictionary instead of the actual module
|
@jcrist any thoughts on recent changes? Are your concerns resolved (or resolved enough)? |
|
Merging tomorrow if there are no further comments. |
jcrist
left a comment
jcrist
left a comment
There was a problem hiding this comment.
Left a few brief comments, but overall this looks fine to me.
|
LGTM, merging. |
|
Thanks @jcrist ! |
3 participants
This commit does two things:
We support passing web addresses as preloads,
allowing for for functionality like the following:
dask-scheduler --preload http://my-web-address/myfile.py
We refactor preloads into a class structure.
I'm usually against this, but I think that in this case it cleans
things up.
I think that it will also make it easier to rewrite how we handle
preload and preload_argv in configuration later
cc @jcrist I think that this might interest you for Dask-Gateway work