You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Gilbert Song <so...@gmail.com> on 2016/02/02 18:18:42 UTC
Re: Review Request 43083: Supported working dir in docker runtime
isolator.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43083/
-----------------------------------------------------------
(Updated Feb. 2, 2016, 9:18 a.m.)
Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
Bugs: MESOS-4005
https://issues.apache.org/jira/browse/MESOS-4005
Repository: mesos
Description
-------
Supported working dir in docker runtime isolator.
Diffs
-----
src/slave/containerizer/mesos/containerizer.cpp 4b504dbb58823ce7675f1d2048dcc7a27c05663d
src/slave/containerizer/mesos/isolators/docker/runtime.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/docker/runtime.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/43083/diff/
Testing
-------
make check (ubuntu14.04 + clang-3.6)
Thanks,
Gilbert Song
Re: Review Request 43083: Supported working dir in docker runtime
isolator.
Posted by Gilbert Song <so...@gmail.com>.
> On Feb. 2, 2016, 4:16 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 1029-1034
> > <https://reviews.apache.org/r/43083/diff/1/?file=1228986#file1228986line1029>
> >
> > We shouldn't allow executor to cd into an arbitrary directory if filesystem isolation is not used (because that'll create security issue).
> >
> > I would do the following:
> > ```
> > if (rootfs.isSome()) {
> > launchFlags.directory = workingDir.isSome()
> > ? workingDir.get()
> > : flags.sandbox_directory;
> > } else {
> > // NOTE: If the executor shares the host filesystem, we
> > // should not allow them to 'cd' into an arbitrary directory
> > // because that'll create security issues.
> > if (workingDir.isSome()) {
> > LOG(WARNING) << "Ignore working directory '" << workingDir.get()
> > << "' specified in container launch info for container "
> > << containerId << " since the executor is using the "
> > << "host filesystem";
> > }
> > launchFlags.directory = directory;
> > }
> > ```
Note that this issue is fixed in /mesos/launch.cpp
- Gilbert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43083/#review117522
-----------------------------------------------------------
On Feb. 3, 2016, 4:49 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43083/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 4:49 p.m.)
>
>
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-4005
> https://issues.apache.org/jira/browse/MESOS-4005
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Supported working dir in docker runtime isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 4b504dbb58823ce7675f1d2048dcc7a27c05663d
> src/slave/containerizer/mesos/isolators/docker/runtime.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/runtime.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/43083/diff/
>
>
> Testing
> -------
>
> make check (ubuntu14.04 + clang-3.6)
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 43083: Supported working dir in docker runtime
isolator.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43083/#review117522
-----------------------------------------------------------
src/slave/containerizer/mesos/containerizer.cpp (lines 1029 - 1034)
<https://reviews.apache.org/r/43083/#comment178716>
We shouldn't allow executor to cd into an arbitrary directory if filesystem isolation is not used (because that'll create security issue).
I would do the following:
```
if (rootfs.isSome()) {
launchFlags.directory = workingDir.isSome()
? workingDir.get()
: flags.sandbox_directory;
} else {
// NOTE: If the executor shares the host filesystem, we
// should not allow them to 'cd' into an arbitrary directory
// because that'll create security issues.
if (workingDir.isSome()) {
LOG(WARNING) << "Ignore working directory '" << workingDir.get()
<< "' specified in container launch info for container "
<< containerId << " since the executor is using the "
<< "host filesystem";
}
launchFlags.directory = directory;
}
```
src/slave/containerizer/mesos/isolators/docker/runtime.cpp (lines 112 - 113)
<https://reviews.apache.org/r/43083/#comment178719>
I think we should distinguish between sandbox_direcotry and working_directory. For instance, for the marathon docker image, sandbox_directory is still /mnt/mesos/sandbox while working_directory is /marathon.
So please add another flag to command executor.
src/slave/containerizer/mesos/isolators/docker/runtime.cpp (lines 114 - 121)
<https://reviews.apache.org/r/43083/#comment178722>
if (!containerConfig.has_task_info()) {
// Custom executor case.
Option<string> dir = getWorkingDirectory(containerConfig);
if (dir.isSome()) {
launchInfo.set_working_directory(dir.get());
}
}
src/slave/containerizer/mesos/isolators/docker/runtime.cpp (lines 357 - 359)
<https://reviews.apache.org/r/43083/#comment178721>
I would make this function very simple: get the working directory specified in the docker image.
I'll move the logics of setting --working_directory to getExecutorLaunchCommand.
src/slave/containerizer/mesos/isolators/docker/runtime.cpp (lines 361 - 363)
<https://reviews.apache.org/r/43083/#comment178718>
Ditto on container_config?
- Jie Yu
On Feb. 2, 2016, 5:18 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43083/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2016, 5:18 p.m.)
>
>
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-4005
> https://issues.apache.org/jira/browse/MESOS-4005
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Supported working dir in docker runtime isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 4b504dbb58823ce7675f1d2048dcc7a27c05663d
> src/slave/containerizer/mesos/isolators/docker/runtime.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/runtime.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/43083/diff/
>
>
> Testing
> -------
>
> make check (ubuntu14.04 + clang-3.6)
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 43083: Supported working dir in docker runtime
isolator.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43083/#review117906
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/docker/runtime.cpp (lines 97 - 118)
<https://reviews.apache.org/r/43083/#comment179165>
How about restructuring the code like the following:
```
Option<Environment> environment = getEnvironment(
containerId,
containerConfig);
Option<string> workingDirectory = getWorkingDirectory(
containerId,
containerConfig);
Try<CommandInfo> command = getLaunchCommand(
containerId,
containerConfig);
if (command.isError()) {
...
}
// Set 'launchInfo'.
ContainerLaunchInfo launchInfo;
if (environment.isSome()) {
launchInfo.mutable_environment()->CopyFrom(environment.get());
}
if (!containerConfig.has_task_info()) {
// Custom executor case.
launchInfo.set_working_directory(workingDirectory.get());
launchInfo.mutable_command()->CopyFrom(command.get());
} else {
// Command task case.
CommandInfo executorCommand = containerConfig.executor_info().command();
if (environment.isSome()) {
launchInfo.mutable_environment()->CopyFrom(environment.get());
}
// Pass working directory to command executor as a flag.
if (workingDirectory.isSome()) {
executorCommand.add_arguments(
"--working_directory=" + workingDirectory.get());
}
// Pass task command as a flag, which will be loaded by
// command executor.
executorCommand.add_arguments(
"--task_command=" +
stringify(JSON::protobuf(command.get())));
launchInfo.mutable_command()->CopyFrom(executorCommand);
}
```
src/slave/containerizer/mesos/isolators/docker/runtime.cpp (line 323)
<https://reviews.apache.org/r/43083/#comment179164>
s/getWorkingDir/getWorkingDirectory/
- Jie Yu
On Feb. 4, 2016, 12:49 a.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43083/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2016, 12:49 a.m.)
>
>
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-4005
> https://issues.apache.org/jira/browse/MESOS-4005
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Supported working dir in docker runtime isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 4b504dbb58823ce7675f1d2048dcc7a27c05663d
> src/slave/containerizer/mesos/isolators/docker/runtime.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/runtime.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/43083/diff/
>
>
> Testing
> -------
>
> make check (ubuntu14.04 + clang-3.6)
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 43083: Supported working dir in docker runtime
isolator.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43083/#review118003
-----------------------------------------------------------
Patch looks great!
Reviews applied: [43019, 43020, 43021, 43022, 43036, 43037, 43081, 43082, 43166, 43167, 43168, 43083]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On Feb. 5, 2016, 8:13 a.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43083/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2016, 8:13 a.m.)
>
>
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-4005
> https://issues.apache.org/jira/browse/MESOS-4005
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Supported working dir in docker runtime isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp dc0868e24486644371b7c285fdf16b9aeac4ca5b
> src/slave/containerizer/mesos/isolators/docker/runtime.hpp 6e6ec86d04b157321141cdf5214033e83ce74548
> src/slave/containerizer/mesos/isolators/docker/runtime.cpp c8a93725fa04476c935a8ba18ad2a145c36fb5fa
>
> Diff: https://reviews.apache.org/r/43083/diff/
>
>
> Testing
> -------
>
> make check (ubuntu14.04 + clang-3.6)
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 43083: Supported working dir in docker runtime
isolator.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43083/
-----------------------------------------------------------
(Updated Feb. 5, 2016, 12:13 a.m.)
Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
Bugs: MESOS-4005
https://issues.apache.org/jira/browse/MESOS-4005
Repository: mesos
Description
-------
Supported working dir in docker runtime isolator.
Diffs (updated)
-----
src/slave/containerizer/mesos/containerizer.cpp dc0868e24486644371b7c285fdf16b9aeac4ca5b
src/slave/containerizer/mesos/isolators/docker/runtime.hpp 6e6ec86d04b157321141cdf5214033e83ce74548
src/slave/containerizer/mesos/isolators/docker/runtime.cpp c8a93725fa04476c935a8ba18ad2a145c36fb5fa
Diff: https://reviews.apache.org/r/43083/diff/
Testing
-------
make check (ubuntu14.04 + clang-3.6)
Thanks,
Gilbert Song
Re: Review Request 43083: Supported working dir in docker runtime
isolator.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43083/#review117758
-----------------------------------------------------------
Patch looks great!
Reviews applied: [43019, 43020, 43021, 43022, 43036, 43037, 43081, 43082, 43166, 43167, 43168, 43083]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On Feb. 4, 2016, 12:49 a.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43083/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2016, 12:49 a.m.)
>
>
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-4005
> https://issues.apache.org/jira/browse/MESOS-4005
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Supported working dir in docker runtime isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 4b504dbb58823ce7675f1d2048dcc7a27c05663d
> src/slave/containerizer/mesos/isolators/docker/runtime.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/runtime.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/43083/diff/
>
>
> Testing
> -------
>
> make check (ubuntu14.04 + clang-3.6)
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 43083: Supported working dir in docker runtime
isolator.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43083/
-----------------------------------------------------------
(Updated Feb. 3, 2016, 4:49 p.m.)
Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
Bugs: MESOS-4005
https://issues.apache.org/jira/browse/MESOS-4005
Repository: mesos
Description
-------
Supported working dir in docker runtime isolator.
Diffs (updated)
-----
src/slave/containerizer/mesos/containerizer.cpp 4b504dbb58823ce7675f1d2048dcc7a27c05663d
src/slave/containerizer/mesos/isolators/docker/runtime.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/docker/runtime.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/43083/diff/
Testing
-------
make check (ubuntu14.04 + clang-3.6)
Thanks,
Gilbert Song
Re: Review Request 43083: Supported working dir in docker runtime
isolator.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43083/#review117448
-----------------------------------------------------------
Patch looks great!
Reviews applied: [43019, 43020, 43021, 43022, 43036, 43037, 43081, 43082, 43083]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On Feb. 2, 2016, 5:18 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43083/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2016, 5:18 p.m.)
>
>
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-4005
> https://issues.apache.org/jira/browse/MESOS-4005
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Supported working dir in docker runtime isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 4b504dbb58823ce7675f1d2048dcc7a27c05663d
> src/slave/containerizer/mesos/isolators/docker/runtime.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/runtime.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/43083/diff/
>
>
> Testing
> -------
>
> make check (ubuntu14.04 + clang-3.6)
>
>
> Thanks,
>
> Gilbert Song
>
>