You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by gi...@apache.org on 2017/07/28 19:28:16 UTC
[1/4] mesos git commit: Fixed the sandbox_path volume source path
ownership.
Repository: mesos
Updated Branches:
refs/heads/master 155285d3d -> 8edfbaaf4
Fixed the sandbox_path volume source path ownership.
This bugfix addresses the issue from MESOS-7830. Basically, the
sandbox path volume ownership was not set correctly. This issue
can be exposed if a framework user is non-root while the agent
process runs as root. Then, the non-root user does not have
permissions to write to this volume.
The correct solution should be giving permissions to corresponding
users by leveraging supplementary groups. But we can still
introduce a workaround in this patch by changing the ownership
of the sandbox path volume to its sandbox's ownership.
Review: https://reviews.apache.org/r/61120/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/63fd94ca
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/63fd94ca
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/63fd94ca
Branch: refs/heads/master
Commit: 63fd94ca8836be32ba7ec6e770df4b1b411ab726
Parents: 155285d
Author: Gilbert Song <so...@gmail.com>
Authored: Fri Jul 28 12:27:49 2017 -0700
Committer: Gilbert Song <so...@gmail.com>
Committed: Fri Jul 28 12:27:49 2017 -0700
----------------------------------------------------------------------
.../mesos/isolators/volume/sandbox_path.cpp | 34 +++++++++++++++++---
1 file changed, 29 insertions(+), 5 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/63fd94ca/src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp b/src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp
index 6f7304d..b321b86 100644
--- a/src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp
+++ b/src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp
@@ -166,11 +166,35 @@ Future<Option<ContainerLaunchInfo>> VolumeSandboxPathIsolatorProcess::prepare(
sandboxes[containerId.parent()],
sandboxPath.path());
- Try<Nothing> mkdir = os::mkdir(source);
- if (mkdir.isError()) {
- return Failure(
- "Failed to create the directory in the parent sandbox: " +
- mkdir.error());
+ // NOTE: Chown should be avoided if the source directory already
+ // exists because it may be owned by some other user and should
+ // not be mutated.
+ if (!os::exists(source)) {
+ Try<Nothing> mkdir = os::mkdir(source);
+ if (mkdir.isError()) {
+ return Failure(
+ "Failed to create the directory in the parent sandbox: " +
+ mkdir.error());
+ }
+
+ // Get the parent sandbox user and group info for the source path.
+ struct stat s;
+ if (::stat(sandboxes[containerId.parent()].c_str(), &s) < 0) {
+ return Failure(ErrnoError(
+ "Failed to stat '" + sandboxes[containerId.parent()] + "'"));
+ }
+
+ LOG(INFO) << "Changing the ownership of the sandbox_path volume at '"
+ << source << "' with UID " << s.st_uid << " and GID "
+ << s.st_gid;
+
+ Try<Nothing> chown = os::chown(s.st_uid, s.st_gid, source, false);
+ if (chown.isError()) {
+ return Failure(
+ "Failed to change the ownership of the sandbox_path volume at '" +
+ source + "' with UID " + stringify(s.st_uid) + " and GID " +
+ stringify(s.st_gid) + ": " + chown.error());
+ }
}
// Prepare the target.
[4/4] mesos git commit: Added regression test for sandbox volume
ownership issue.
Posted by gi...@apache.org.
Added regression test for sandbox volume ownership issue.
Added regression test for sandbox volume ownership issue.
Review: https://reviews.apache.org/r/61123/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/8edfbaaf
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/8edfbaaf
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/8edfbaaf
Branch: refs/heads/master
Commit: 8edfbaaf49c0b09ed02bd07334fb65b29d088a40
Parents: b5efb91
Author: Gilbert Song <so...@gmail.com>
Authored: Fri Jul 28 12:27:58 2017 -0700
Committer: Gilbert Song <so...@gmail.com>
Committed: Fri Jul 28 12:27:58 2017 -0700
----------------------------------------------------------------------
.../linux_filesystem_isolator_tests.cpp | 61 ++++++++++++++++++++
1 file changed, 61 insertions(+)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/8edfbaaf/src/tests/containerizer/linux_filesystem_isolator_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/containerizer/linux_filesystem_isolator_tests.cpp b/src/tests/containerizer/linux_filesystem_isolator_tests.cpp
index 457311e..f9cab2f 100644
--- a/src/tests/containerizer/linux_filesystem_isolator_tests.cpp
+++ b/src/tests/containerizer/linux_filesystem_isolator_tests.cpp
@@ -239,6 +239,67 @@ TEST_F(LinuxFilesystemIsolatorTest, ROOT_VolumeFromSandbox)
}
+// This is a regression test for MESOS-5187. It is a ROOT test to
+// simulate the scenario that the framework user is non-root while
+// the agent process is root, to make sure that non-root user can
+// still have the permission to write to the volume as expected.
+TEST_F(LinuxFilesystemIsolatorTest, ROOT_SandboxVolumeOwnership)
+{
+ string registry = path::join(sandbox.get(), "registry");
+ AWAIT_READY(DockerArchive::create(registry, "test_image"));
+
+ slave::Flags flags = CreateSlaveFlags();
+ flags.isolation = "filesystem/linux,docker/runtime";
+ flags.docker_registry = registry;
+ flags.docker_store_dir = path::join(sandbox.get(), "store");
+ flags.image_providers = "docker";
+
+ Fetcher fetcher(flags);
+
+ Try<MesosContainerizer*> create =
+ MesosContainerizer::create(flags, true, &fetcher);
+
+ ASSERT_SOME(create);
+
+ Owned<Containerizer> containerizer(create.get());
+
+ ContainerID containerId;
+ containerId.set_value(UUID::random().toString());
+
+ ExecutorInfo executor = createExecutorInfo(
+ "test_executor",
+ "echo abc > /tmp/file");
+
+ executor.mutable_container()->CopyFrom(createContainerInfo(
+ "test_image",
+ {createVolumeFromHostPath("/tmp", "tmp", Volume::RW)}));
+
+ string directory = path::join(flags.work_dir, "sandbox");
+ ASSERT_SOME(os::mkdir(directory));
+
+ // Simulate the executor sandbox ownership as the user
+ // from FrameworkInfo.
+ ASSERT_SOME(os::chown("nobody", directory));
+
+ Future<bool> launch = containerizer->launch(
+ containerId,
+ createContainerConfig(None(), executor, directory, "nobody"),
+ map<string, string>(),
+ None());
+
+ AWAIT_READY(launch);
+
+ Future<Option<ContainerTermination>> wait = containerizer->wait(containerId);
+
+ AWAIT_READY(wait);
+ ASSERT_SOME(wait.get());
+ ASSERT_TRUE(wait->get().has_status());
+ EXPECT_WEXITSTATUS_EQ(0, wait->get().status());
+
+ EXPECT_SOME_EQ("abc\n", os::read(path::join(directory, "tmp", "file")));
+}
+
+
// This test verifies that a volume with an absolute host path as
// well as an absolute container path is properly mounted in the
// container's mount namespace.
[2/4] mesos git commit: Added regression test for sandbox_path volume
ownership issue.
Posted by gi...@apache.org.
Added regression test for sandbox_path volume ownership issue.
Added regression test for sandbox_path volume ownership issue.
Review: https://reviews.apache.org/r/61121/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/f99a7170
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/f99a7170
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/f99a7170
Branch: refs/heads/master
Commit: f99a7170716bba52b05732833fb26df1d01e2b42
Parents: 63fd94c
Author: Gilbert Song <so...@gmail.com>
Authored: Fri Jul 28 12:27:52 2017 -0700
Committer: Gilbert Song <so...@gmail.com>
Committed: Fri Jul 28 12:27:52 2017 -0700
----------------------------------------------------------------------
.../volume_sandbox_path_isolator_tests.cpp | 94 ++++++++++++++++++++
1 file changed, 94 insertions(+)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/f99a7170/src/tests/containerizer/volume_sandbox_path_isolator_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/containerizer/volume_sandbox_path_isolator_tests.cpp b/src/tests/containerizer/volume_sandbox_path_isolator_tests.cpp
index 3228b9a..0da01c1 100644
--- a/src/tests/containerizer/volume_sandbox_path_isolator_tests.cpp
+++ b/src/tests/containerizer/volume_sandbox_path_isolator_tests.cpp
@@ -147,6 +147,100 @@ TEST_F(VolumeSandboxPathIsolatorTest, SharedVolume)
EXPECT_WTERMSIG_EQ(SIGKILL, wait.get()->status());
}
+
+// This is a regression test for MESOS-7830. It is a ROOT test to
+// simulate the scenario that the framework user is non-root while
+// the agent process is root, to make sure that non-root user can
+// still have the permission to write to the volume as expected.
+TEST_F(VolumeSandboxPathIsolatorTest, ROOT_SandboxPathVolumeOwnership)
+{
+ slave::Flags flags = CreateSlaveFlags();
+ flags.isolation = "volume/sandbox_path";
+
+ Fetcher fetcher(flags);
+
+ Try<MesosContainerizer*> create = MesosContainerizer::create(
+ flags,
+ true,
+ &fetcher);
+
+ ASSERT_SOME(create);
+
+ Owned<MesosContainerizer> containerizer(create.get());
+
+ SlaveState state;
+ state.id = SlaveID();
+
+ AWAIT_READY(containerizer->recover(state));
+
+ ContainerID containerId;
+ containerId.set_value(UUID::random().toString());
+
+ ExecutorInfo executor = createExecutorInfo("executor", "sleep 99", "cpus:1");
+
+ Try<string> directory = environment->mkdtemp();
+ ASSERT_SOME(directory);
+
+ // Simulate the executor sandbox ownership as the user
+ // from FrameworkInfo.
+ ASSERT_SOME(os::chown("nobody", directory.get()));
+
+ Future<bool> launch = containerizer->launch(
+ containerId,
+ createContainerConfig(None(), executor, directory.get(), "nobody"),
+ map<string, string>(),
+ None());
+
+ AWAIT_ASSERT_TRUE(launch);
+
+ ContainerID nestedContainerId;
+ nestedContainerId.mutable_parent()->CopyFrom(containerId);
+ nestedContainerId.set_value(UUID::random().toString());
+
+ ContainerInfo containerInfo;
+ containerInfo.set_type(ContainerInfo::MESOS);
+
+ Volume* volume = containerInfo.add_volumes();
+ volume->set_mode(Volume::RW);
+ volume->set_container_path("parent");
+
+ Volume::Source* source = volume->mutable_source();
+ source->set_type(Volume::Source::SANDBOX_PATH);
+
+ Volume::Source::SandboxPath* sandboxPath = source->mutable_sandbox_path();
+ sandboxPath->set_type(Volume::Source::SandboxPath::PARENT);
+ sandboxPath->set_path("shared");
+
+ launch = containerizer->launch(
+ nestedContainerId,
+ createContainerConfig(
+ createCommandInfo("echo 'hello' > parent/file"),
+ containerInfo,
+ None(),
+ "nobody"),
+ map<string, string>(),
+ None());
+
+ AWAIT_ASSERT_TRUE(launch);
+
+ Future<Option<ContainerTermination>> wait =
+ containerizer->wait(nestedContainerId);
+
+ AWAIT_READY(wait);
+ ASSERT_SOME(wait.get());
+ ASSERT_TRUE(wait.get()->has_status());
+ EXPECT_WEXITSTATUS_EQ(0, wait.get()->status());
+
+ wait = containerizer->wait(containerId);
+
+ containerizer->destroy(containerId);
+
+ AWAIT_READY(wait);
+ ASSERT_SOME(wait.get());
+ ASSERT_TRUE(wait.get()->has_status());
+ EXPECT_WTERMSIG_EQ(SIGKILL, wait.get()->status());
+}
+
} // namespace tests {
} // namespace internal {
} // namespace mesos {
[3/4] mesos git commit: Fixed the sandbox volume relative host path
ownership.
Posted by gi...@apache.org.
Fixed the sandbox volume relative host path ownership.
This bugfix addresses the issue from MESOS-5178. Basically, the
sandbox volume ownership was not set correctly. This issue can be
exposed if a framework user is non-root while the agent
process runs as root. Then, the non-root user does not have
permissions to write to this volume.
The correct solution should be giving permissions to corresponding
users by leveraging supplementary groups. But we can still
introduce a workaround in this patch by changing the ownership
of this sandbox volume to its sandbox's ownership.
Review: https://reviews.apache.org/r/61122/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/b5efb912
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/b5efb912
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/b5efb912
Branch: refs/heads/master
Commit: b5efb9121e523edf344534ba6c5332d6f7190643
Parents: f99a717
Author: Gilbert Song <so...@gmail.com>
Authored: Fri Jul 28 12:27:55 2017 -0700
Committer: Gilbert Song <so...@gmail.com>
Committed: Fri Jul 28 12:27:55 2017 -0700
----------------------------------------------------------------------
.../mesos/isolators/filesystem/linux.cpp | 37 ++++++++++++++++----
1 file changed, 30 insertions(+), 7 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/b5efb912/src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/mesos/isolators/filesystem/linux.cpp b/src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
index bf35b7f..d7fe9a8 100644
--- a/src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
+++ b/src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
@@ -432,6 +432,15 @@ Try<vector<CommandInfo>> LinuxFilesystemIsolatorProcess::getPreExecCommands(
commands.push_back(command);
}
+ // Get the parent sandbox user and group info for the source path.
+ struct stat s;
+ if (::stat(containerConfig.directory().c_str(), &s) < 0) {
+ return ErrnoError("Failed to stat '" + containerConfig.directory() + "'");
+ }
+
+ const uid_t uid = s.st_uid;
+ const gid_t gid = s.st_gid;
+
foreach (const Volume& volume, containerConfig.container_info().volumes()) {
// NOTE: Volumes with source will be handled by the corresponding
// isolators (e.g., docker/volume).
@@ -477,14 +486,28 @@ Try<vector<CommandInfo>> LinuxFilesystemIsolatorProcess::getPreExecCommands(
// work directory because a user can potentially use a container
// path like '../../abc'.
- Try<Nothing> mkdir = os::mkdir(source);
- if (mkdir.isError()) {
- return Error(
- "Failed to create the source of the mount at '" +
- source + "': " + mkdir.error());
- }
+ // NOTE: Chown should be avoided if the source directory already
+ // exists because it may be owned by some other user and should
+ // not be mutated.
+ if (!os::exists(source)) {
+ Try<Nothing> mkdir = os::mkdir(source);
+ if (mkdir.isError()) {
+ return Error(
+ "Failed to create the source of the mount at '" +
+ source + "': " + mkdir.error());
+ }
- // TODO(idownes): Consider setting ownership and mode.
+ LOG(INFO) << "Changing the ownership of the sandbox volume at '"
+ << source << "' with UID " << uid << " and GID " << gid;
+
+ Try<Nothing> chown = os::chown(uid, gid, source, false);
+ if (chown.isError()) {
+ return Error(
+ "Failed to change the ownership of the sandbox volume at '" +
+ source + "' with UID " + stringify(uid) + " and GID " +
+ stringify(gid) + ": " + chown.error());
+ }
+ }
}
// Determine the target of the mount. The mount target