Re: Review Request 46798: Introduced linux capabilities support for mesos containerizer.

[Joris Van Remoortere]

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



Closing this review as Benjamin has follow up with separate reviews. Please see 
our 
[guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md)
 for reopening reviews.

- Joris Van Remoortere


On May 26, 2016, 3:03 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46798/
> ---
> 
> (Updated May 26, 2016, 3:03 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> 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/common/parse.hpp 19e56dbeb765f8bec92e0a3615f6f7c12466fa9e 
>   src/launcher/executor.cpp 7d111e668e0a139a98bdeb959997843180b40452 
>   src/slave/containerizer/mesos/containerizer.hpp 
> a1a00020668f6da8d611f26e5637afffc87d09ba 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 75e5a32a3e70ec60a6800e21a621673184ea0956 
>   src/slave/containerizer/mesos/launch.hpp 
> c716e0396736d1f2f60ec31540f12f4f7597d081 
>   src/slave/containerizer/mesos/launch.cpp 
> e22106b014c871e2184a15c2ab154a0674874e47 
>   src/slave/flags.hpp 80ba2887448e91c40ae68fc2d9f0c0bee1a49f48 
>   src/slave/flags.cpp b7df8f760d0f75459f1e80e3d8e18d49a3995df8 
>   src/tests/container_logger_tests.cpp 
> efadceafca5721bce4dbffadb35f54fd5365abb0 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> c524f42743bf08ee54f1cbb083d0d3c85a8b70c9 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> 4293416ac8434e9eb7e80724480a54936a2fe24a 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 09742ff21513dc2570684d384b257868dd57a9ce 
> 
> Diff: https://reviews.apache.org/r/46798/diff/
> 
> 
> Testing
> ---
> 
> make check; used mesos cli to test end to end functionality.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 46798: Introduced linux capabilities support for mesos containerizer.

[Benjamin Bannier]

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




src/launcher/executor.cpp (line 90)


I would much prefer this to not force each and every caller to deal with 
different function signatures.

We should probably try to find a way to use identical signatures on every 
platform, and then possible make the underlying implementation do what is 
actually supported.



src/launcher/executor.cpp (line 376)


It seems we should be able to do away with conditional compilation here if 
we'd push the check for platform support into the capabilities implementation.



src/launcher/executor.cpp (line 397)


Same here, we should try to remove the conditional compilation and instead 
push the checks for platform support into the capabilities infrastructure.



src/slave/containerizer/mesos/containerizer.cpp (line 270)


This could again be simplified by pushing the check for platform support 
into the capabilities infrastructure.



src/slave/containerizer/mesos/containerizer.cpp (line 271)


I think moving the capabilities check out of the loop would improve 
readibilty of this loop.



src/slave/containerizer/mesos/containerizer.cpp (line 1299)


Remove conditional compilation once member exists for all platforms.



src/slave/containerizer/mesos/launch.cpp (line 31)


Sort alphabetically.



src/slave/containerizer/mesos/launch.cpp (line 49)


As currently written this needs to be compiled conditionally as 
`Capabilities` is not defined on non-Linux platforms.



src/slave/containerizer/mesos/launch.cpp (line 280)


Why do we need to require a shell command here?



src/slave/containerizer/mesos/launch.cpp (line 281)


The nesting of these preprocessor checks (where one checks a negative and 
the other for a positive) on top of a already deeply control flow makes this 
really hard to read.

I believe this could again be simplified by pushing the check for platform 
support into the capabilities infrastructure.


- Benjamin Bannier


On May 26, 2016, 5:03 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46798/
> ---
> 
> (Updated May 26, 2016, 5:03 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> 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/common/parse.hpp 19e56dbeb765f8bec92e0a3615f6f7c12466fa9e 
>   src/launcher/executor.cpp 7d111e668e0a139a98bdeb959997843180b40452 
>   src/slave/containerizer/mesos/containerizer.hpp 
> a1a00020668f6da8d611f26e5637afffc87d09ba 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 75e5a32a3e70ec60a6800e21a621673184ea0956 
>   src/slave/containerizer/mesos/launch.hpp 
> c716e0396736d1f2f60ec31540f12f4f7597d081 
>   src/slave/containerizer/mesos/launch.cpp 
> e22106b014c871e2184a15c2ab154a0674874e47 
>   src/slave/flags.hpp 80ba2887448e91c40ae68fc2d9f0c0bee1a49f48 
>   src/slave/flags.cpp b7df8f760d0f75459f1e80e3d8e18d49a3995df8 
>   src/tests/container_logger_tests.cpp 
> efadceafca5721bce4dbffadb35f54fd5365abb0 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> c524f42743bf08ee54f1cbb083d0d3c85a8b70c9 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> 4293416ac8434e9eb7e80724480a54936a2fe24a 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 09742ff21513dc2570684d384b257868dd57a9ce 
> 
> Diff: https://reviews.apache.org/r/46798/diff/
> 
> 
> Testing
> ---
> 
> make check; used mesos cli to test end to end functionality.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 46798: Introduced linux capabilities support for mesos containerizer.

[Jojy Varghese]

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

(Updated May 26, 2016, 3:03 p.m.)


Review request for mesos and Jie Yu.


Changes
---

rebased.


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 (updated)
-

  src/common/parse.hpp 19e56dbeb765f8bec92e0a3615f6f7c12466fa9e 
  src/launcher/executor.cpp 7d111e668e0a139a98bdeb959997843180b40452 
  src/slave/containerizer/mesos/containerizer.hpp 
a1a00020668f6da8d611f26e5637afffc87d09ba 
  src/slave/containerizer/mesos/containerizer.cpp 
75e5a32a3e70ec60a6800e21a621673184ea0956 
  src/slave/containerizer/mesos/launch.hpp 
c716e0396736d1f2f60ec31540f12f4f7597d081 
  src/slave/containerizer/mesos/launch.cpp 
e22106b014c871e2184a15c2ab154a0674874e47 
  src/slave/flags.hpp 80ba2887448e91c40ae68fc2d9f0c0bee1a49f48 
  src/slave/flags.cpp b7df8f760d0f75459f1e80e3d8e18d49a3995df8 
  src/tests/container_logger_tests.cpp efadceafca5721bce4dbffadb35f54fd5365abb0 
  src/tests/containerizer/docker_volume_isolator_tests.cpp 
c524f42743bf08ee54f1cbb083d0d3c85a8b70c9 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
4293416ac8434e9eb7e80724480a54936a2fe24a 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
09742ff21513dc2570684d384b257868dd57a9ce 

Diff: https://reviews.apache.org/r/46798/diff/


Testing
---

make check; used mesos cli to test end to end functionality.


Thanks,

Jojy Varghese

Re: Review Request 46798: Introduced linux capabilities support for mesos containerizer.

[Jojy Varghese]

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

(Updated May 17, 2016, 2:59 p.m.)


Review request for mesos and Jie Yu.


Changes
---

rebased


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 (updated)
-

  src/launcher/executor.cpp 7d111e668e0a139a98bdeb959997843180b40452 
  src/slave/containerizer/mesos/containerizer.hpp 
a1a00020668f6da8d611f26e5637afffc87d09ba 
  src/slave/containerizer/mesos/containerizer.cpp 
75e5a32a3e70ec60a6800e21a621673184ea0956 
  src/slave/containerizer/mesos/launch.hpp 
c716e0396736d1f2f60ec31540f12f4f7597d081 
  src/slave/containerizer/mesos/launch.cpp 
e22106b014c871e2184a15c2ab154a0674874e47 
  src/slave/flags.hpp 80ba2887448e91c40ae68fc2d9f0c0bee1a49f48 
  src/slave/flags.cpp b7df8f760d0f75459f1e80e3d8e18d49a3995df8 
  src/tests/container_logger_tests.cpp efadceafca5721bce4dbffadb35f54fd5365abb0 
  src/tests/containerizer/docker_volume_isolator_tests.cpp 
c524f42743bf08ee54f1cbb083d0d3c85a8b70c9 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
4293416ac8434e9eb7e80724480a54936a2fe24a 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
09742ff21513dc2570684d384b257868dd57a9ce 

Diff: https://reviews.apache.org/r/46798/diff/


Testing
---

make check; used mesos cli to test end to end functionality.


Thanks,

Jojy Varghese

Re: Review Request 46798: Introduced linux capabilities support for mesos containerizer.

[Jojy Varghese]

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

(Updated May 13, 2016, 12:50 a.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description (updated)
---

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 (updated)
-

  src/launcher/executor.cpp fa4a89e97b0c427b4595a6adebc397aeb5bfaaa5 
  src/slave/containerizer/mesos/containerizer.hpp 
a1a00020668f6da8d611f26e5637afffc87d09ba 
  src/slave/containerizer/mesos/containerizer.cpp 
75e5a32a3e70ec60a6800e21a621673184ea0956 
  src/slave/containerizer/mesos/launch.hpp 
c716e0396736d1f2f60ec31540f12f4f7597d081 
  src/slave/containerizer/mesos/launch.cpp 
e22106b014c871e2184a15c2ab154a0674874e47 
  src/slave/flags.hpp 4fa3213545d4bd3525d85c3f71749f00f08dc998 
  src/slave/flags.cpp 6fde51fc61cfcad61d4085c208bd2eca2eae8f14 
  src/tests/container_logger_tests.cpp efadceafca5721bce4dbffadb35f54fd5365abb0 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
4293416ac8434e9eb7e80724480a54936a2fe24a 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
09742ff21513dc2570684d384b257868dd57a9ce 

Diff: https://reviews.apache.org/r/46798/diff/


Testing
---

make check; used mesos cli to test end to end functionality.


Thanks,

Jojy Varghese

Re: Review Request 46798: Introduced linux capabilities support for mesos containerizer.

[Jojy Varghese]

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

(Updated May 6, 2016, 5:05 p.m.)


Review request for mesos and Jie Yu.


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 (updated)
-

  src/launcher/executor.cpp 9f1d2168bc4ddbce1bcd25ff38dc1c34714eb28b 
  src/slave/containerizer/mesos/containerizer.hpp 
13399f014dcd85defbff79f3b5aa4e7e75d41fd1 
  src/slave/containerizer/mesos/containerizer.cpp 
8d538954d6e1f13e833d75c2eaa37e700278ee0c 
  src/slave/containerizer/mesos/launch.hpp 
c716e0396736d1f2f60ec31540f12f4f7597d081 
  src/slave/containerizer/mesos/launch.cpp 
e22106b014c871e2184a15c2ab154a0674874e47 
  src/slave/flags.hpp 4fa3213545d4bd3525d85c3f71749f00f08dc998 
  src/slave/flags.cpp 6fde51fc61cfcad61d4085c208bd2eca2eae8f14 
  src/tests/container_logger_tests.cpp efadceafca5721bce4dbffadb35f54fd5365abb0 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
29d313051865761306029f331eb36684c3252ffb 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
09742ff21513dc2570684d384b257868dd57a9ce 

Diff: https://reviews.apache.org/r/46798/diff/


Testing
---

make check; used mesos cli to test end to end functionality.


Thanks,

Jojy Varghese

Re: Review Request 46798: Introduced linux capabilities support for mesos containerizer.

[Jojy Varghese]

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

(Updated May 5, 2016, 9 p.m.)


Review request for mesos and Jie Yu.


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 (updated)
-

  src/launcher/executor.cpp 9f1d2168bc4ddbce1bcd25ff38dc1c34714eb28b 
  src/slave/containerizer/mesos/containerizer.hpp 
13399f014dcd85defbff79f3b5aa4e7e75d41fd1 
  src/slave/containerizer/mesos/containerizer.cpp 
8d538954d6e1f13e833d75c2eaa37e700278ee0c 
  src/slave/containerizer/mesos/launch.hpp 
c716e0396736d1f2f60ec31540f12f4f7597d081 
  src/slave/containerizer/mesos/launch.cpp 
e22106b014c871e2184a15c2ab154a0674874e47 
  src/slave/flags.hpp 4fa3213545d4bd3525d85c3f71749f00f08dc998 
  src/slave/flags.cpp 6fde51fc61cfcad61d4085c208bd2eca2eae8f14 
  src/tests/container_logger_tests.cpp efadceafca5721bce4dbffadb35f54fd5365abb0 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
29d313051865761306029f331eb36684c3252ffb 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
09742ff21513dc2570684d384b257868dd57a9ce 

Diff: https://reviews.apache.org/r/46798/diff/


Testing
---

make check; used mesos cli to test end to end functionality.


Thanks,

Jojy Varghese

Re: Review Request 46798: Introduced linux capabilities support for mesos containerizer.

[Jojy Varghese]

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

(Updated May 2, 2016, 5:12 p.m.)


Review request for mesos and Jie Yu.


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 (updated)
-

  src/launcher/executor.cpp 9f1d2168bc4ddbce1bcd25ff38dc1c34714eb28b 
  src/slave/containerizer/mesos/containerizer.hpp 
13399f014dcd85defbff79f3b5aa4e7e75d41fd1 
  src/slave/containerizer/mesos/containerizer.cpp 
8d538954d6e1f13e833d75c2eaa37e700278ee0c 
  src/slave/containerizer/mesos/launch.cpp 
e22106b014c871e2184a15c2ab154a0674874e47 
  src/slave/flags.hpp 4fa3213545d4bd3525d85c3f71749f00f08dc998 
  src/slave/flags.cpp 6fde51fc61cfcad61d4085c208bd2eca2eae8f14 
  src/tests/container_logger_tests.cpp efadceafca5721bce4dbffadb35f54fd5365abb0 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
29d313051865761306029f331eb36684c3252ffb 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
09742ff21513dc2570684d384b257868dd57a9ce 

Diff: https://reviews.apache.org/r/46798/diff/


Testing
---

make check; used mesos cli to test end to end functionality.


Thanks,

Jojy Varghese

Re: Review Request 46798: Introduced linux capabilities support for mesos containerizer.

[Jojy Varghese]

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

(Updated April 29, 2016, 6:35 p.m.)


Review request for mesos and Jie Yu.


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 (updated)
-

  src/launcher/executor.cpp 9f1d2168bc4ddbce1bcd25ff38dc1c34714eb28b 
  src/slave/containerizer/mesos/containerizer.hpp 
13399f014dcd85defbff79f3b5aa4e7e75d41fd1 
  src/slave/containerizer/mesos/containerizer.cpp 
8d538954d6e1f13e833d75c2eaa37e700278ee0c 
  src/slave/containerizer/mesos/launch.cpp 
e22106b014c871e2184a15c2ab154a0674874e47 
  src/slave/flags.hpp 4fa3213545d4bd3525d85c3f71749f00f08dc998 
  src/slave/flags.cpp 6fde51fc61cfcad61d4085c208bd2eca2eae8f14 
  src/tests/container_logger_tests.cpp efadceafca5721bce4dbffadb35f54fd5365abb0 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
29d313051865761306029f331eb36684c3252ffb 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
09742ff21513dc2570684d384b257868dd57a9ce 

Diff: https://reviews.apache.org/r/46798/diff/


Testing
---

make check; used mesos cli to test end to end functionality.


Thanks,

Jojy Varghese

Review Request 46798: Introduced linux capabilities support for mesos containerizer.

[Jojy Varghese]

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

Review request for mesos and Jie Yu.


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/launcher/executor.cpp 9f1d2168bc4ddbce1bcd25ff38dc1c34714eb28b 
  src/slave/containerizer/mesos/containerizer.hpp 
13399f014dcd85defbff79f3b5aa4e7e75d41fd1 
  src/slave/containerizer/mesos/containerizer.cpp 
8d538954d6e1f13e833d75c2eaa37e700278ee0c 
  src/slave/containerizer/mesos/launch.cpp 
e22106b014c871e2184a15c2ab154a0674874e47 
  src/slave/flags.hpp 4fa3213545d4bd3525d85c3f71749f00f08dc998 
  src/slave/flags.cpp 6fde51fc61cfcad61d4085c208bd2eca2eae8f14 
  src/tests/container_logger_tests.cpp efadceafca5721bce4dbffadb35f54fd5365abb0 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
29d313051865761306029f331eb36684c3252ffb 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
09742ff21513dc2570684d384b257868dd57a9ce 

Diff: https://reviews.apache.org/r/46798/diff/


Testing
---

make check; used mesos cli to test end to end functionality.


Thanks,

Jojy Varghese