----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50270/#review147872 -----------------------------------------------------------
src/slave/containerizer/mesos/launch.cpp (line 366) <https://reviews.apache.org/r/50270/#comment215170> to avoid confusion, I'd s/capabilities/capabilitiesManager/ Also, Error("Not initialized") src/slave/containerizer/mesos/launch.cpp (line 370) <https://reviews.apache.org/r/50270/#comment215160> kill this line src/slave/containerizer/mesos/launch.cpp (line 372) <https://reviews.apache.org/r/50270/#comment215171> s/instantiate/initialize/ TO avoid jaggedness, i'd: ``` cerr << "Failed to initialize capabilities: " << capabilitiesManager.error() << endl; ``` src/slave/containerizer/mesos/launch.cpp (line 373) <https://reviews.apache.org/r/50270/#comment215166> return EXIT_FAILURE? src/slave/containerizer/mesos/launch.cpp (line 378) <https://reviews.apache.org/r/50270/#comment215159> I am wondering if we should rename this to: ``` capabilities->setKeepCaps() capabilities->clearKeepCaps() ``` src/slave/containerizer/mesos/launch.cpp (line 379) <https://reviews.apache.org/r/50270/#comment215161> kill this line src/slave/containerizer/mesos/launch.cpp (lines 382 - 383) <https://reviews.apache.org/r/50270/#comment215172> `uid` might be None, right? src/slave/containerizer/mesos/launch.cpp (line 383) <https://reviews.apache.org/r/50270/#comment215167> return EXIT_FAILURE? src/slave/containerizer/mesos/launch.cpp (line 417) <https://reviews.apache.org/r/50270/#comment215175> Why this is not guarded by ifdef linux? src/slave/containerizer/mesos/launch.cpp (lines 422 - 423) <https://reviews.apache.org/r/50270/#comment215173> Ditto on formatting. src/slave/containerizer/mesos/launch.cpp (line 428) <https://reviews.apache.org/r/50270/#comment215174> s/processCapabilities/capabilities/ src/slave/flags.cpp (line 94) <https://reviews.apache.org/r/50270/#comment215147> This list is not complete. I'd suggest we don't make this change in this patch. It should point to a document online with a complete list. Let's create a ticket to track. src/slave/flags.cpp (line 458) <https://reviews.apache.org/r/50270/#comment215148> s/agent/operator/ src/slave/flags.cpp (line 460) <https://reviews.apache.org/r/50270/#comment215149> s/unifed// We might want to support that for Docker containerizer in the future. You can add `(Currently only supported in MesosContainerizer)` src/slave/flags.cpp (line 466) <https://reviews.apache.org/r/50270/#comment215154> What's this? I thought we use PR_SET_KEEPCAPS and agent is running under root. - Jie Yu On Sept. 6, 2016, 3:04 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50270/ > ----------------------------------------------------------- > > (Updated Sept. 6, 2016, 3:04 p.m.) > > > Review request for mesos, Jay Guo and Jie Yu. > > > Bugs: MESOS-5303 > https://issues.apache.org/jira/browse/MESOS-5303 > > > Repository: mesos > > > Description > ------- > > This change introduces linux capability based security for unified > containerizer. A new agent flag `allowed_capabilities` has been > introduced to override the default capabilities of the user or the > capabilities requested by the user. > > This feature is only available on Linux. > > > Diffs > ----- > > src/slave/containerizer/mesos/launch.hpp > 0e86da9c7bd9c7fbedd7102d66b902d1c10e5e0b > src/slave/containerizer/mesos/launch.cpp > 13b65d82e029650e150eb2bc3647d95af167bd72 > src/slave/flags.hpp 1a006663e7cc58ee548b3dda686cfbac0c240baa > src/slave/flags.cpp 0f2be1700f41b74da4ea1ce699a81ec33cf92a9a > > Diff: https://reviews.apache.org/r/50270/diff/ > > > Testing > ------- > > `make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o > optimizations) > > > Thanks, > > Benjamin Bannier > >
