mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 59552: Add support for explicitly setting bounding capabilities.
Date Fri, 09 Jun 2017 21:28:05 GMT

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




src/slave/containerizer/mesos/launch.cpp
Line 454 (original), 454 (patched)
<https://reviews.apache.org/r/59552/#comment249903>

    I suggest we use Option here. `Result` here is super wierd.



src/slave/containerizer/mesos/launch.cpp
Line 457 (original), 458 (patched)
<https://reviews.apache.org/r/59552/#comment249905>

    ```
    Try<Capabilities> _capabilitiesManager = Capabilities::create();
    if (_capabilitiesManager.isError()) {
      ...
    }
    
    capabilitiesManager = _capabilitiesManager.get();
    
    ```



src/slave/containerizer/mesos/isolators/linux/capabilities.cpp
Lines 99-103 (patched)
<https://reviews.apache.org/r/59552/#comment251128>

    In fact, i think the semantics should be:
    
    1) treat framework specificied capabilities as effective capabilities.
    2) in the future, allow frameworks to set bounding capabilities
    3) if effective capabilities is set and bounding capabilities is not set, set bounding
set to be the same as effective set (instead of "unbounded"). This matches the existing semantics.
    4) treat operator flags as the default values if the framework does not specify them (for
both bounding and effective capabilities).
    5) if both effective and bounding capabilities are set, make sure the effective is a subset
of bounding
    6) if effective capabilities is not set, but bounding capabilities is set, set effective
capabilities to be the same as bounding capabilities
    
    Given that, I'd adjust the logic of this function to the following:
    
    ```
    Option<CapabilityInfo> effective;
    Option<CapabilityInfo> bounding;
    
    if (containerConfig.has_container_info() &&
        containerConfig.container_info().has_linux_info() &&
        containerConfig.container_info().linux_info().has_capability_info()) {
      effective = containerConfig.container_info().linux_info().capability_info();
    }
    
    // Framework can override the effective capabilities.
    if (effective.isNone() && flags.effective_capabilities.isSome()) {
      effective = flags.effective_capabilities.get();
    }
    
    if (flags.bounding_capabilities.isSome()) {
      bounding = flags.bounding_capabilities.get();
    }
    
    if (effective.isSome() && bounding.isSome()) {
      // Validate that effective is a subset of bounding.
    }
    
    if (effective.isSome() && bounding.isNone()) {
      bounding = effective.get();
    }
    
    if (effective.isNone() && bounding.isSome()) {
      effective = bounding.get();
    }
    
    ...
    ```
    
    The above logic is a bit easier to follow, comparing putting some logic in `mergeCapabilities`.
I'll just kill `mergeCapabilities` function.



src/slave/containerizer/mesos/launch.cpp
Line 456 (original), 456-457 (patched)
<https://reviews.apache.org/r/59552/#comment251134>

    We probably should have a CHECK (or comment) here. both of them are either set, or either
not set. Maybe add a comment in the flags as well.



src/slave/containerizer/mesos/launch.cpp
Lines 627 (patched)
<https://reviews.apache.org/r/59552/#comment251140>

    Since two flags are either both set or both not set. I would simply check both are isSome()
here.


- Jie Yu


On June 5, 2017, 4:36 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59552/
> -----------------------------------------------------------
> 
> (Updated June 5, 2017, 4:36 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7476
>     https://issues.apache.org/jira/browse/MESOS-7476
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The linux/capabilities isolator implements the `--allowed_capabilities`
> option by granting all the allowed capabilities. This change explicitly
> populates the only the bounding capabilities in the case where
> `--bounding_capabilities` has been set but the task itself has not been
> granted any effective capabilities. This improves the security of tasks
> since it is now possible to configure the bounding set without actually
> granting privilege to the task.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/isolators/linux/capabilities.cpp 60d22aa877c1ab62a08222e5efe8800e337684da

>   src/slave/containerizer/mesos/launch.cpp f48d294a0a832dfe248c4a83849ee5a63cb76bce 
>   src/tests/containerizer/linux_capabilities_isolator_tests.cpp 40376a03fdb8f931f8d3f83b1c3fa6207e02c1d1

> 
> 
> Diff: https://reviews.apache.org/r/59552/diff/2/
> 
> 
> Testing
> -------
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>


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