> On June 9, 2017, 9:28 p.m., Jie Yu wrote: > > src/slave/containerizer/mesos/isolators/linux/capabilities.cpp > > Lines 99-103 (patched) > > <https://reviews.apache.org/r/59552/diff/2/?file=1741704#file1741704line101> > > > > 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.
Thinking some more about this one :) > On June 9, 2017, 9:28 p.m., Jie Yu wrote: > > src/slave/containerizer/mesos/launch.cpp > > Line 456 (original), 456-457 (patched) > > <https://reviews.apache.org/r/59552/diff/2/?file=1741705#file1741705line456> > > > > 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. The effective and bounding capabilities are currently allowed to vary (see below). > On June 9, 2017, 9:28 p.m., Jie Yu wrote: > > src/slave/containerizer/mesos/launch.cpp > > Lines 627 (patched) > > <https://reviews.apache.org/r/59552/diff/2/?file=1741705#file1741705line627> > > > > Since two flags are either both set or both not set. I would simply > > check both are isSome() here. They are actually allowed to vary. If the neither the framework nor the operator specified effective capabilities, then `launchInfo.has_effective_capabilities()` would be false but `launchInfo.has_bounding_capabilities()` could still be true. - James ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59552/#review176530 ----------------------------------------------------------- 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 > >
