mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 50270: Introduced linux capabilities support for mesos containerizer.
Date Thu, 08 Sep 2016 05:55:53 GMT


> On Sept. 6, 2016, 8:35 nachm., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp, line 378
> > <https://reviews.apache.org/r/50270/diff/6/?file=1491898#file1491898line378>
> >
> >     I am wondering if we should rename this to:
> >     ```
> >     capabilities->setKeepCaps()
> >     capabilities->clearKeepCaps()
> >     ```

Done, also submitted https://reviews.apache.org/r/51697/. Could you commit that as part of
this chain? I did not add a `Capabilities::clearKeepCaps` as we currently have no use for
it. I am also not sure how useful clearing keep caps again might be midterm as we'd likely
never execute `setKeepCaps` in anything not already executing in a separate process we could
just let die (like we do here).


> On Sept. 6, 2016, 8:35 nachm., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp, lines 382-383
> > <https://reviews.apache.org/r/50270/diff/6/?file=1491898#file1491898line382>
> >
> >     `uid` might be None, right?

I reworded the log output slightly to not require some `uid` anymore.


> On Sept. 6, 2016, 8:35 nachm., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp, line 417
> > <https://reviews.apache.org/r/50270/diff/6/?file=1491898#file1491898line417>
> >
> >     Why this is not guarded by ifdef linux?

I was trying to minimize code behind `ifdef` guards. While it should not be possible to set
the capabilities flag on non-Linux systems the `Flag` member is defined for all platforms
as it has no dependencies on Linux-specific features.


> On Sept. 6, 2016, 8:35 nachm., Jie Yu wrote:
> > src/slave/flags.cpp, line 466
> > <https://reviews.apache.org/r/50270/diff/6/?file=1491900#file1491900line466>
> >
> >     What's this? I thought we use PR_SET_KEEPCAPS and agent is running under root.

I think there's actually no need to make a hard requirement on the agent to be run as root
(at least to make use of capabilities) -- we do only require `SETPCAP` and that the capabilities
requested for a container are contained in the agent's capabilities. I believe it makes sense
to anticipate that users of this capabilities feature are aware of fine-grained permissions.

I added an additional note to the help text to make clear that `SETPCAP` is required for the
agent.


> On Sept. 6, 2016, 8:35 nachm., Jie Yu wrote:
> > src/slave/flags.cpp, line 94
> > <https://reviews.apache.org/r/50270/diff/6/?file=1491900#file1491900line94>
> >
> >     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.

I filed https://issues.apache.org/jira/browse/MESOS-6133.


- Benjamin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50270/#review147872
-----------------------------------------------------------


On Sept. 7, 2016, 6:43 nachm., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50270/
> -----------------------------------------------------------
> 
> (Updated Sept. 7, 2016, 6:43 nachm.)
> 
> 
> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message