search

oconnor663 / duct.rs

a Rust library for running child processes
MIT License
819 stars 34 forks source link

Add unix::ExpressionExt containing a uid and gid function #56

Closed TethysSvensson closed 6 years ago

TethysSvensson commented 6 years ago

See #55 for context.

I went ahead and implemented a simple simple support for changing uid/gid.

This should not be considered done as it is -- at the very least it's missing documentation and possibly also some tests (I haven't looked at how duct is tested yet). I mostly made it to see how easy it would be to add the functionality and to give us something concrete to discuss.

The main open question is whether to do this as unix::ExpressionExt or just add conditional functions on Expression itself. I'm leaning towards keeping it as it is, but let me know what you think.

If you like it, I will go ahead an make this a more finished PR.

oconnor663 commented 6 years ago

Thanks for the PR! I probably won't get a chance to look at it until I'm on a plane tomorrow. Pardon me taking a few days to get back to you.

I went ahead and turned off those spammy Coveralls comments :-D

TethysSvensson commented 6 years ago

Don't worry about it. Take all the time you need. :D

TethysSvensson commented 6 years ago

I've added documentation now. If you agree that the #[cfg(unix)] is acceptable, this should be ready to merge as-is.

oconnor663 commented 6 years ago

Apart from the cfg_attr change above, could you also run cargo fmt on the final result?

One higher level question I want to make sure of: Are you sure you have a use case where you're using uid/gid together with, say, pipe? If you're not using pipe, my instinct is to suggest falling back to std::process::Command (possibly together with the os_pipe and shared_child libraries), instead of extending duct.

As a rule of thumb, duct isn't really aiming to support programs whose primary job is to monitor generic subprocesses. Instead, it's trying to be a convenience library for programs who need to shell out to something specific, like porting a Bash script. For example, if I was writing a Rust version of forever, I'd probably want such low level control over my children that eventually I'd be reaching for Command::before_exec. Giving duct enough features to handle that use case would be pretty complicated, and I'm not sure the convenience of duct is going to matter to that program anyway. The same thing might go for a Rust version of Systemd.

Your docker entrypoint sounds like it might fit into that category? In that its only job is to launch whatever command it was told to launch, in a specific and careful way? Let me know what you think.

oconnor663 commented 6 years ago

A second high level thought: It might make sense to add a constructor that lets you create an Expression from an arbitrary std::process::Command. If that would cover this use case (and maybe some of the crazy magical before_exec cases too for free), maybe we should go that direction?

Though there might be some trouble if the both the inner Command and the containing Expression both set stdout. The documentation would probably have to include some unfortunate caveats.

TethysSvensson commented 6 years ago

I think your reasoning makes sense. Supporting all of std::process::Command might make the most sense here.

I'm not sure whether our specific use-cases would benefit all that much from before_exec though. I'm sure some of them would, but given that we haven't actually implemented that part yet, I remain unsure.

Currently we are running as one specific user inside docker, but sometime soon we would want to run parts of the computation as more than one user. The root-part would mostly be responsible for starting the other jobs as the correct users.

I'll update the code with cargo fmt and the cfg_attr changes. Let me know if you decide to go forward with the other option.

oconnor663 commented 6 years ago

I'd like to play with some kind of from_command interface, but it's clearly taking me too long to get back to this! Would you like me to merge your changes in a kind-of-unstable way, where we might undo them in the future? Or do you mind waiting until I'm able to spend more time on it?

TethysSvensson commented 6 years ago

We don't mind waiting for a bit. We are not currently blocked by this, and might never be if we end of changing our design (or use Command directly).

oconnor663 commented 6 years ago

@Idolf what do you think of https://github.com/oconnor663/duct.rs/issues/60 and https://github.com/oconnor663/duct.rs/issues/61 as an alternative to this.

oconnor663 commented 6 years ago

I'm going to go ahead and release the pre_spawn function, but we can always change it before 1.0. No concrete plans on when that'll be yet anyway.

TethysSvensson commented 6 years ago

This sounds like an excellent solution!