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 <songzihao1...@gmail.com> Authored: Fri Jul 28 12:27:55 2017 -0700 Committer: Gilbert Song <songzihao1...@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
