You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Marco Massenzio <ma...@mesosphere.io> on 2015/07/15 01:20:32 UTC
Re: Review Request 36389: Enable remote execution of arbitrary
command.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36389/
-----------------------------------------------------------
(Updated July 14, 2015, 11:20 p.m.)
Review request for mesos, Benjamin Hindman and Cody Maloney.
Summary (updated)
-----------------
Enable remote execution of arbitrary command.
Bugs: MESOS-2830
https://issues.apache.org/jira/browse/MESOS-2830
Repository: mesos
Description
-------
Jira: MESOS-2830
Under certain (maintenance) circumnstance, it may be necessary
(or desirable) to execute arbitrary operator's commands on the
slave (the entire fleet or a subset thereof) bypassing the Mesos
Task execution mechanism; this may typically be necessary for
maintenance and/or emergency actions.
This patch adds an HTTP endpoint (/execute) which accepts a
JSON-encoded CommandInfo structure and executes the given
command (with optional arguments).
The output of the command (along with potentially any stderr
messages) is returned JSON-encoded in the Response.
For more details, see the design doc at:
https://goo.gl/4npTMU
Diffs
-----
src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
src/slave/main.cpp 8008430e98773d8be9ba5ac6385cffb2e351932a
src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a
src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4
Diff: https://reviews.apache.org/r/36389/diff/
Testing
-------
make check
lots of manual testing (using Postman, REST client)
Thanks,
Marco Massenzio
Re: Review Request 36389: Enable remote execution of arbitrary
command.
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On July 14, 2015, 11:21 p.m., Jie Yu wrote:
> > Discussed with Cody on the design doc. Does it make sense to implement that as an anonymous module?
Can you please elaborate on the pros/cons of doing so?
thanks!
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36389/#review91678
-----------------------------------------------------------
On July 14, 2015, 11:20 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36389/
> -----------------------------------------------------------
>
> (Updated July 14, 2015, 11:20 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Cody Maloney.
>
>
> Bugs: MESOS-2830
> https://issues.apache.org/jira/browse/MESOS-2830
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2830
>
> Under certain (maintenance) circumnstance, it may be necessary
> (or desirable) to execute arbitrary operator's commands on the
> slave (the entire fleet or a subset thereof) bypassing the Mesos
> Task execution mechanism; this may typically be necessary for
> maintenance and/or emergency actions.
>
> This patch adds an HTTP endpoint (/execute) which accepts a
> JSON-encoded CommandInfo structure and executes the given
> command (with optional arguments).
>
> The output of the command (along with potentially any stderr
> messages) is returned JSON-encoded in the Response.
>
> For more details, see the design doc at:
> https://goo.gl/4npTMU
>
>
> Diffs
> -----
>
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/main.cpp 8008430e98773d8be9ba5ac6385cffb2e351932a
> src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a
> src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4
>
> Diff: https://reviews.apache.org/r/36389/diff/
>
>
> Testing
> -------
>
> make check
>
> lots of manual testing (using Postman, REST client)
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 36389: Enable remote execution of arbitrary
command.
Posted by Jie Yu <yu...@gmail.com>.
> On July 14, 2015, 11:21 p.m., Jie Yu wrote:
> > Discussed with Cody on the design doc. Does it make sense to implement that as an anonymous module?
>
> Marco Massenzio wrote:
> Can you please elaborate on the pros/cons of doing so?
> thanks!
Please refer to the design doc for the discussion. To summerize, I am not convinced that this functionality should belong to Mesos. It could be a standalone service (possibly launched by Mesos) that runs on each slave.
ALso, what if the slave is down? Do you still need some way to access the box?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36389/#review91678
-----------------------------------------------------------
On July 14, 2015, 11:20 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36389/
> -----------------------------------------------------------
>
> (Updated July 14, 2015, 11:20 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Cody Maloney.
>
>
> Bugs: MESOS-2830
> https://issues.apache.org/jira/browse/MESOS-2830
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2830
>
> Under certain (maintenance) circumnstance, it may be necessary
> (or desirable) to execute arbitrary operator's commands on the
> slave (the entire fleet or a subset thereof) bypassing the Mesos
> Task execution mechanism; this may typically be necessary for
> maintenance and/or emergency actions.
>
> This patch adds an HTTP endpoint (/execute) which accepts a
> JSON-encoded CommandInfo structure and executes the given
> command (with optional arguments).
>
> The output of the command (along with potentially any stderr
> messages) is returned JSON-encoded in the Response.
>
> For more details, see the design doc at:
> https://goo.gl/4npTMU
>
>
> Diffs
> -----
>
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/main.cpp 8008430e98773d8be9ba5ac6385cffb2e351932a
> src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a
> src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4
>
> Diff: https://reviews.apache.org/r/36389/diff/
>
>
> Testing
> -------
>
> make check
>
> lots of manual testing (using Postman, REST client)
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 36389: Enable remote execution of arbitrary
command.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36389/#review91678
-----------------------------------------------------------
Discussed with Cody on the design doc. Does it make sense to implement that as an anonymous module?
- Jie Yu
On July 14, 2015, 11:20 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36389/
> -----------------------------------------------------------
>
> (Updated July 14, 2015, 11:20 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Cody Maloney.
>
>
> Bugs: MESOS-2830
> https://issues.apache.org/jira/browse/MESOS-2830
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2830
>
> Under certain (maintenance) circumnstance, it may be necessary
> (or desirable) to execute arbitrary operator's commands on the
> slave (the entire fleet or a subset thereof) bypassing the Mesos
> Task execution mechanism; this may typically be necessary for
> maintenance and/or emergency actions.
>
> This patch adds an HTTP endpoint (/execute) which accepts a
> JSON-encoded CommandInfo structure and executes the given
> command (with optional arguments).
>
> The output of the command (along with potentially any stderr
> messages) is returned JSON-encoded in the Response.
>
> For more details, see the design doc at:
> https://goo.gl/4npTMU
>
>
> Diffs
> -----
>
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/main.cpp 8008430e98773d8be9ba5ac6385cffb2e351932a
> src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a
> src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4
>
> Diff: https://reviews.apache.org/r/36389/diff/
>
>
> Testing
> -------
>
> make check
>
> lots of manual testing (using Postman, REST client)
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 36389: Enable remote execution of arbitrary
command.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36389/#review91687
-----------------------------------------------------------
Patch looks great!
Reviews applied: [36389]
All tests passed.
- Mesos ReviewBot
On July 14, 2015, 11:20 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36389/
> -----------------------------------------------------------
>
> (Updated July 14, 2015, 11:20 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Cody Maloney.
>
>
> Bugs: MESOS-2830
> https://issues.apache.org/jira/browse/MESOS-2830
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2830
>
> Under certain (maintenance) circumnstance, it may be necessary
> (or desirable) to execute arbitrary operator's commands on the
> slave (the entire fleet or a subset thereof) bypassing the Mesos
> Task execution mechanism; this may typically be necessary for
> maintenance and/or emergency actions.
>
> This patch adds an HTTP endpoint (/execute) which accepts a
> JSON-encoded CommandInfo structure and executes the given
> command (with optional arguments).
>
> The output of the command (along with potentially any stderr
> messages) is returned JSON-encoded in the Response.
>
> For more details, see the design doc at:
> https://goo.gl/4npTMU
>
>
> Diffs
> -----
>
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/main.cpp 8008430e98773d8be9ba5ac6385cffb2e351932a
> src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a
> src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4
>
> Diff: https://reviews.apache.org/r/36389/diff/
>
>
> Testing
> -------
>
> make check
>
> lots of manual testing (using Postman, REST client)
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 36389: Enable remote execution of arbitrary
command.
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On July 15, 2015, 6:13 p.m., Artem Harutyunyan wrote:
> > To make sure that this is fool proof(ish), I would suggest that this should only ship only when the Authorizer framework (mentioned in the TODO comment) becomes available. Also, I would add a screaming comment to --usage, something along the lines of 'this is insecure, and this enables arbitrary command executrion with root privileges'.
> >
> > In general, I am of the firm opinion that this feature should come with a whitelisting mechanism that will allow operators to whitelist (benign) commands that they want to execute, and forbid anything else.
>To make sure that this is fool proof(ish), I would suggest that this should only ship only when the Authorizer framework (mentioned in the TODO comment) becomes available. Also, I would add a screaming comment to --usage, something along the lines of 'this is insecure, and this enables arbitrary command executrion with root privileges'.
This has to be deliberately enabled:
```
--allow_remote_execution
Allows the execution of arbitrary shell commands on the Slave, by
POSTing to the /execute endpoint with a JSON-serialized CommandInfo
protobuf, containing the command to execute and, optionally, the
arguments.
WARNING: this may cause a security vulnerability and is disabled by
default; enable only if you know exactly what you are doing.
```
The WARNING seems explicit enough to me, but if people agree, I'm happy TO MAKE IT SCREAM ;-)
> In general, I am of the firm opinion that this feature should come with a whitelisting mechanism that will allow operators to whitelist (benign) commands that they want to execute, and forbid anything else.
Perhaps; but please note that this is meant mostly for emergency/off-cycle maintenance reasons, so it's very difficult to know in advance *what* you will need (if you did, you probably wouldn't need the feauture in the first place).
This is also meant to be a "highly scriptable" endpoint, where you have either a generating function or a discovery methodology, that finds out (potentially 1,000s of) IPs for your Slaves and then does in very rapid succession a POST with the command to execute (I'm envisioning some catastrophic failure mode that needs to be prevented; or a security breach; or something like that).
In any event, in modern systems, the way to "protect" these endpoints is at the network security level, by isolating services within a protected, private subnet, with access granted only from "bastion" servers (hardened and secured) - re-implementing authentication/authorization on every single service is not only prone to leaving some vulnerable to security breaches, but it also becomes quickly unmanageable (as you know have disparate, and inconsistent, security policies and authentication mechanism and no centralized way to manage and revoke access).
(well, when I say "modern"... that was 5-6 years ago at Google... but I figure we were 4-5 years ahead of everyone else :) )
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36389/#review91784
-----------------------------------------------------------
On July 14, 2015, 11:20 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36389/
> -----------------------------------------------------------
>
> (Updated July 14, 2015, 11:20 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Cody Maloney.
>
>
> Bugs: MESOS-2830
> https://issues.apache.org/jira/browse/MESOS-2830
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2830
>
> Under certain (maintenance) circumnstance, it may be necessary
> (or desirable) to execute arbitrary operator's commands on the
> slave (the entire fleet or a subset thereof) bypassing the Mesos
> Task execution mechanism; this may typically be necessary for
> maintenance and/or emergency actions.
>
> This patch adds an HTTP endpoint (/execute) which accepts a
> JSON-encoded CommandInfo structure and executes the given
> command (with optional arguments).
>
> The output of the command (along with potentially any stderr
> messages) is returned JSON-encoded in the Response.
>
> For more details, see the design doc at:
> https://goo.gl/4npTMU
>
>
> Diffs
> -----
>
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/main.cpp 8008430e98773d8be9ba5ac6385cffb2e351932a
> src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a
> src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4
>
> Diff: https://reviews.apache.org/r/36389/diff/
>
>
> Testing
> -------
>
> make check
>
> lots of manual testing (using Postman, REST client)
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 36389: Enable remote execution of arbitrary
command.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36389/#review91784
-----------------------------------------------------------
To make sure that this is fool proof(ish), I would suggest that this should only ship only when the Authorizer framework (mentioned in the TODO comment) becomes available. Also, I would add a screaming comment to --usage, something along the lines of 'this is insecure, and this enables arbitrary command executrion with root privileges'.
In general, I am of the firm opinion that this feature should come with a whitelisting mechanism that will allow operators to whitelist (benign) commands that they want to execute, and forbid anything else.
- Artem Harutyunyan
On July 14, 2015, 4:20 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36389/
> -----------------------------------------------------------
>
> (Updated July 14, 2015, 4:20 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Cody Maloney.
>
>
> Bugs: MESOS-2830
> https://issues.apache.org/jira/browse/MESOS-2830
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2830
>
> Under certain (maintenance) circumnstance, it may be necessary
> (or desirable) to execute arbitrary operator's commands on the
> slave (the entire fleet or a subset thereof) bypassing the Mesos
> Task execution mechanism; this may typically be necessary for
> maintenance and/or emergency actions.
>
> This patch adds an HTTP endpoint (/execute) which accepts a
> JSON-encoded CommandInfo structure and executes the given
> command (with optional arguments).
>
> The output of the command (along with potentially any stderr
> messages) is returned JSON-encoded in the Response.
>
> For more details, see the design doc at:
> https://goo.gl/4npTMU
>
>
> Diffs
> -----
>
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/main.cpp 8008430e98773d8be9ba5ac6385cffb2e351932a
> src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a
> src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4
>
> Diff: https://reviews.apache.org/r/36389/diff/
>
>
> Testing
> -------
>
> make check
>
> lots of manual testing (using Postman, REST client)
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 36389: Enable remote execution of arbitrary
command.
Posted by Jie Yu <yu...@gmail.com>.
> On July 20, 2015, 4:20 p.m., Benjamin Hindman wrote:
> > Apologies for the delay on the review. Let's definitely get a test for this stuff too!
Ben, not sure if you see my comments. I am not convinced this should be part of Mesos. Should we reach a consensus first before moving this forward? Thanks.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36389/#review92221
-----------------------------------------------------------
On July 14, 2015, 11:20 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36389/
> -----------------------------------------------------------
>
> (Updated July 14, 2015, 11:20 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Cody Maloney.
>
>
> Bugs: MESOS-2830
> https://issues.apache.org/jira/browse/MESOS-2830
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2830
>
> Under certain (maintenance) circumnstance, it may be necessary
> (or desirable) to execute arbitrary operator's commands on the
> slave (the entire fleet or a subset thereof) bypassing the Mesos
> Task execution mechanism; this may typically be necessary for
> maintenance and/or emergency actions.
>
> This patch adds an HTTP endpoint (/execute) which accepts a
> JSON-encoded CommandInfo structure and executes the given
> command (with optional arguments).
>
> The output of the command (along with potentially any stderr
> messages) is returned JSON-encoded in the Response.
>
> For more details, see the design doc at:
> https://goo.gl/4npTMU
>
>
> Diffs
> -----
>
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/main.cpp 8008430e98773d8be9ba5ac6385cffb2e351932a
> src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a
> src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4
>
> Diff: https://reviews.apache.org/r/36389/diff/
>
>
> Testing
> -------
>
> make check
>
> lots of manual testing (using Postman, REST client)
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 36389: Enable remote execution of arbitrary
command.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36389/#review92221
-----------------------------------------------------------
Apologies for the delay on the review. Let's definitely get a test for this stuff too!
src/slave/flags.hpp (lines 118 - 119)
<https://reviews.apache.org/r/36389/#comment146243>
We name the flags to match how you'd see them on the command line and thus use snake case here (see above too). Thanks!
Also, I'd like to suggest s/remote_execution_allowed/allow_remote_execution/ in order to be more of a verb here, i.e, --allow_remote_execution=true versus --remote_execution_allowed=true.
src/slave/main.cpp (lines 261 - 265)
<https://reviews.apache.org/r/36389/#comment146245>
First, this shoudn't be needed, because the slave will get passed the flags and when it initializes can check to see if remote execution has been allowed.
Second, we try and avoid at all costs synchronously calling a libprocess process. There is only one place I know of that does this in some tests, but there is a big TODO and warning of why we don't want to do this. Happy to discuss in more detail. In this case since we shouldn't need this block of code at all you can just kill it.
src/slave/slave.hpp (lines 372 - 382)
<https://reviews.apache.org/r/36389/#comment146246>
These shouldn't be needed, the slave will have the flags and can just read the values from there.
src/slave/slave.hpp (lines 571 - 575)
<https://reviews.apache.org/r/36389/#comment146247>
Not needed, can just read values from 'flags', no need to have two copies, then on wonders which is the source of truth? This is one of the main reasons we pass 'flags' throughout everywhere as much as possible.
src/slave/slave.cpp (line 517)
<https://reviews.apache.org/r/36389/#comment146250>
Do you need to capture 'http'?
src/slave/slave.cpp (line 519)
<https://reviews.apache.org/r/36389/#comment146248>
FYI, the 'this->' isn't needed here and we haven't used it consistently.
src/slave/slave.cpp (line 4700)
<https://reviews.apache.org/r/36389/#comment146263>
Why `auto` here? Interestingly enough, you do `subprocess.get()` below and pass it to an `Option<Subprocess>`, which made me jump for a second thinking that we'd changed the return type of `process::subprocess()` to be `Try<Option<process::Subprocess>>` instead of just `Try<process::Subprocess>`. This is why I'm not convinced `auto` actually buys us anything here: the type provides documentation.
src/slave/slave.cpp (line 4701)
<https://reviews.apache.org/r/36389/#comment146258>
What about if 'shell' is true? Seems like we should support both cases, or explicitly return a `BadRequest` or `Unsupported...` if we don't support it for now.
src/slave/slave.cpp (lines 4703 - 4705)
<https://reviews.apache.org/r/36389/#comment146260>
One of these things is not like the others. ;-)
Any reason you didn't use the 'process::' prefix on the third `Subprocess::PIPE()`?
src/slave/slave.cpp (line 4712)
<https://reviews.apache.org/r/36389/#comment146262>
Why is this an `Option`? The return type of `process::subprocess(...)` above is `Try<process::Subprocess>`, so no need to put it into an option. In fact, why not just use `subprocess.get()` everywhere below instead of `cmd.get()`?
src/slave/slave.cpp (line 4713)
<https://reviews.apache.org/r/36389/#comment146261>
If we don't use stderr my recommendation would be to just send it to '/dev/null' by using `process::Subprocess::PATH("/dev/null")` above instead of a `process::Subprocess::PIPE()`.
src/slave/slave.cpp (line 4722)
<https://reviews.apache.org/r/36389/#comment146264>
Unused variable.
src/slave/slave.cpp (line 4731)
<https://reviews.apache.org/r/36389/#comment146265>
We don't actually know if this exited unless we do `WIFEXITED` first, it might have terminated due to a signal (which we determine via `WIFSIGNALED`).
src/slave/slave.cpp (line 4735)
<https://reviews.apache.org/r/36389/#comment146266>
Okay nevermind, it looks like you do use stderr?
Either way, calling 'get()' here now is blocking, because technically even though the process is completed we might not yet have read all the data from the pipe. I think what you want here is to wait for all of 'status()', 'out', and 'err', e.g., something like:
CHECK_SOME(subprocess.get().out());
CHECK_SOME(subprocess.get().err());
Future<string> out = subprocess.get().out().get();
Future<string> err = subprocess.get().err().get();
return await(subprocess.get().status(), out, err)
.then(...);
Note that 'await' will just mean the futures have completed, but they might have failed. You'd want 'collect' if you want to make sure they're successful. I think we'd need some overloads to make 'collect' work in this case too, but that's a quick fix, just let me know.
src/slave/slave.cpp (line 4735)
<https://reviews.apache.org/r/36389/#comment146332>
Okay nevermind, it looks like you do use stderr?
Either way, calling 'get()' here now is blocking, because technically even though the process is completed we might not yet have read all the data from the pipe. I think what you want here is to wait for all of 'status()', 'out', and 'err', e.g., something like:
CHECK_SOME(subprocess.get().out());
CHECK_SOME(subprocess.get().err());
Future<string> out = subprocess.get().out().get();
Future<string> err = subprocess.get().err().get();
return await(subprocess.get().status(), out, err)
.then(...);
Note that 'await' will just mean the futures have completed, but they might have failed. You'd want 'collect' if you want to make sure they're successful. I think we'd need some overloads to make 'collect' work in this case too, but that's a quick fix, just let me know.
src/slave/slave.cpp (line 4755)
<https://reviews.apache.org/r/36389/#comment146267>
Technically this can happen, because 'after' is going to get executed "after" the timeout but that doesn't mean that the lambda in the body of the 'then' is not still executing and couldn't complete before you check the status of 'response'. Note taht this is the caveat with 'after' that you have to look out for, but your code looks okay with respect to this (if, on the other hand, you were making some nonRegardless of the value of response, I'd just treat this as a timeout. It would probably be good, however, to 'os::kill(subprocess.get().pid())' so that we don't have a process that potentially runs forever.
- Benjamin Hindman
On July 14, 2015, 11:20 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36389/
> -----------------------------------------------------------
>
> (Updated July 14, 2015, 11:20 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Cody Maloney.
>
>
> Bugs: MESOS-2830
> https://issues.apache.org/jira/browse/MESOS-2830
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2830
>
> Under certain (maintenance) circumnstance, it may be necessary
> (or desirable) to execute arbitrary operator's commands on the
> slave (the entire fleet or a subset thereof) bypassing the Mesos
> Task execution mechanism; this may typically be necessary for
> maintenance and/or emergency actions.
>
> This patch adds an HTTP endpoint (/execute) which accepts a
> JSON-encoded CommandInfo structure and executes the given
> command (with optional arguments).
>
> The output of the command (along with potentially any stderr
> messages) is returned JSON-encoded in the Response.
>
> For more details, see the design doc at:
> https://goo.gl/4npTMU
>
>
> Diffs
> -----
>
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/main.cpp 8008430e98773d8be9ba5ac6385cffb2e351932a
> src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a
> src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4
>
> Diff: https://reviews.apache.org/r/36389/diff/
>
>
> Testing
> -------
>
> make check
>
> lots of manual testing (using Postman, REST client)
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 36389: Enable remote execution of arbitrary
command.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
> On July 15, 2015, 12:36 a.m., Artem Harutyunyan wrote:
> > src/slave/slave.cpp, line 4755
> > <https://reviews.apache.org/r/36389/diff/4/?file=1011886#file1011886line4755>
> >
> > Shouldn't there be an equivalent of an assert here if we never expect this to happen? Something like this:
> >
> > if (response.isReady()) {
> > ASSERT
> > }
> >
> > return http::BadRequest ...
> >
> > Unless, there is possibility of a race where the result becomes ready right at the 15th second (in a separate thread) and by the time this lambda is executed the response becomes actually (and legitimately) ready.
I sat down with Joris to verify whether or not there was a possible race condition resulting in a ready Future inside the `.after` lambda, and turned out that there is not. It's a great idea to have an explicit check for isReady() to be false, but the assert (i.e. CHECK) still does looks like a better option.
- Artem
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36389/#review91723
-----------------------------------------------------------
On July 14, 2015, 4:20 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36389/
> -----------------------------------------------------------
>
> (Updated July 14, 2015, 4:20 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Cody Maloney.
>
>
> Bugs: MESOS-2830
> https://issues.apache.org/jira/browse/MESOS-2830
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2830
>
> Under certain (maintenance) circumnstance, it may be necessary
> (or desirable) to execute arbitrary operator's commands on the
> slave (the entire fleet or a subset thereof) bypassing the Mesos
> Task execution mechanism; this may typically be necessary for
> maintenance and/or emergency actions.
>
> This patch adds an HTTP endpoint (/execute) which accepts a
> JSON-encoded CommandInfo structure and executes the given
> command (with optional arguments).
>
> The output of the command (along with potentially any stderr
> messages) is returned JSON-encoded in the Response.
>
> For more details, see the design doc at:
> https://goo.gl/4npTMU
>
>
> Diffs
> -----
>
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/main.cpp 8008430e98773d8be9ba5ac6385cffb2e351932a
> src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a
> src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4
>
> Diff: https://reviews.apache.org/r/36389/diff/
>
>
> Testing
> -------
>
> make check
>
> lots of manual testing (using Postman, REST client)
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 36389: Enable remote execution of arbitrary
command.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36389/#review91723
-----------------------------------------------------------
src/slave/flags.hpp (line 117)
<https://reviews.apache.org/r/36389/#comment145304>
Just for my own education purpose :-), Is there a reason for a newline here?
src/slave/slave.cpp (line 151)
<https://reviews.apache.org/r/36389/#comment145297>
Do you think it makes sense to capture the default value in a single location? Otherwise it's hardcoded in two places (here and in flags.cpp)
src/slave/slave.cpp (lines 4691 - 4694)
<https://reviews.apache.org/r/36389/#comment145310>
wouldn't const std::vector<string> args(command.arguments()); work here?
src/slave/slave.cpp (line 4698)
<https://reviews.apache.org/r/36389/#comment145298>
If you make this review dependent on /r/36424 then RB should be smart enough to apply the patch from there before apllying and building this one.
That should eliminate the need of having a temporary code in the review.
src/slave/slave.cpp (line 4702)
<https://reviews.apache.org/r/36389/#comment145311>
if we are using command.value() here we might as well use command.args() and ditch the args variable altogether.
src/slave/slave.cpp (line 4743)
<https://reviews.apache.org/r/36389/#comment145302>
Is it OK to return HTTP 200 if the command returned a non-zero error code?
In the statements above, for exaple, the error code is returned if the command execution failed (albeit, the reasons for a failure are different).
This approach makes the HTTP 200 insufficient for verifying successful execution of the command.
src/slave/slave.cpp (line 4755)
<https://reviews.apache.org/r/36389/#comment145299>
Shouldn't there be an equivalent of an assert here if we never expect this to happen? Something like this:
if (response.isReady()) {
ASSERT
}
return http::BadRequest ...
Unless, there is possibility of a race where the result becomes ready right at the 15th second (in a separate thread) and by the time this lambda is executed the response becomes actually (and legitimately) ready.
- Artem Harutyunyan
On July 14, 2015, 4:20 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36389/
> -----------------------------------------------------------
>
> (Updated July 14, 2015, 4:20 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Cody Maloney.
>
>
> Bugs: MESOS-2830
> https://issues.apache.org/jira/browse/MESOS-2830
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2830
>
> Under certain (maintenance) circumnstance, it may be necessary
> (or desirable) to execute arbitrary operator's commands on the
> slave (the entire fleet or a subset thereof) bypassing the Mesos
> Task execution mechanism; this may typically be necessary for
> maintenance and/or emergency actions.
>
> This patch adds an HTTP endpoint (/execute) which accepts a
> JSON-encoded CommandInfo structure and executes the given
> command (with optional arguments).
>
> The output of the command (along with potentially any stderr
> messages) is returned JSON-encoded in the Response.
>
> For more details, see the design doc at:
> https://goo.gl/4npTMU
>
>
> Diffs
> -----
>
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/main.cpp 8008430e98773d8be9ba5ac6385cffb2e351932a
> src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a
> src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4
>
> Diff: https://reviews.apache.org/r/36389/diff/
>
>
> Testing
> -------
>
> make check
>
> lots of manual testing (using Postman, REST client)
>
>
> Thanks,
>
> Marco Massenzio
>
>