You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Silas Snider <sw...@apple.com> on 2017/04/05 19:53:18 UTC
Re: Review Request 57884: Ensure that host /etc/* files are mounted
RDONLY by the CNI Isolator.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57884/
-----------------------------------------------------------
(Updated April 5, 2017, 7:53 p.m.)
Review request for mesos and Jie Yu.
Bugs: MESOS-7268
https://issues.apache.org/jira/browse/MESOS-7268
Repository: mesos
Description
-------
Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 6e95315b70a5d9d3b4b21c4cf235b0a483760190
src/tests/containerizer/linux_filesystem_isolator_tests.cpp 5e489ef6a522000c55b0fb9a27bce2567f82bb73
Diff: https://reviews.apache.org/r/57884/diff/2/
Changes: https://reviews.apache.org/r/57884/diff/1-2/
Testing
-------
Thanks,
Silas Snider
Re: Review Request 57884: Ensure that host /etc/* files are mounted
RDONLY by the CNI Isolator.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57884/#review177922
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On June 14, 2017, 1:06 a.m., Silas Snider wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57884/
> -----------------------------------------------------------
>
> (Updated June 14, 2017, 1:06 a.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-7268
> https://issues.apache.org/jira/browse/MESOS-7268
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 36217d2e5
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 3aecd8c5a
>
>
> Diff: https://reviews.apache.org/r/57884/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Silas Snider
>
>
Re: Review Request 57884: Ensure that host /etc/* files are mounted
RDONLY by the CNI Isolator.
Posted by Silas Snider <sw...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57884/
-----------------------------------------------------------
(Updated June 14, 2017, 1:06 a.m.)
Review request for mesos and Jie Yu.
Bugs: MESOS-7268
https://issues.apache.org/jira/browse/MESOS-7268
Repository: mesos
Description
-------
Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 36217d2e5
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 3aecd8c5a
Diff: https://reviews.apache.org/r/57884/diff/4/
Changes: https://reviews.apache.org/r/57884/diff/3-4/
Testing
-------
Thanks,
Silas Snider
Re: Review Request 57884: Ensure that host /etc/* files are mounted
RDONLY by the CNI Isolator.
Posted by Silas Snider <sw...@apple.com>.
> On June 13, 2017, 11:57 p.m., Jie Yu wrote:
> >
>
> Jie Yu wrote:
> Also, for nested container, we don't need to do another read only bind mount.
fixed
- Silas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57884/#review177833
-----------------------------------------------------------
On June 14, 2017, 1:06 a.m., Silas Snider wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57884/
> -----------------------------------------------------------
>
> (Updated June 14, 2017, 1:06 a.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-7268
> https://issues.apache.org/jira/browse/MESOS-7268
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 36217d2e5
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 3aecd8c5a
>
>
> Diff: https://reviews.apache.org/r/57884/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Silas Snider
>
>
Re: Review Request 57884: Ensure that host /etc/* files are mounted
RDONLY by the CNI Isolator.
Posted by Jie Yu <yu...@gmail.com>.
> On June 13, 2017, 11:57 p.m., Jie Yu wrote:
> >
Also, for nested container, we don't need to do another read only bind mount.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57884/#review177833
-----------------------------------------------------------
On June 13, 2017, 10:02 p.m., Silas Snider wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57884/
> -----------------------------------------------------------
>
> (Updated June 13, 2017, 10:02 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-7268
> https://issues.apache.org/jira/browse/MESOS-7268
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 6e95315b70a5d9d3b4b21c4cf235b0a483760190
>
>
> Diff: https://reviews.apache.org/r/57884/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Silas Snider
>
>
Re: Review Request 57884: Ensure that host /etc/* files are mounted
RDONLY by the CNI Isolator.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57884/#review177833
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 1918-1928 (patched)
<https://reviews.apache.org/r/57884/#comment251555>
Thought about it more. I don't think we should do read-only bind mount if the 'source' is not from host `/etc/*`. We should allow container to modify it because the file is private to each container.
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 1980-1990 (patched)
<https://reviews.apache.org/r/57884/#comment251556>
Ditto here.
- Jie Yu
On June 13, 2017, 10:02 p.m., Silas Snider wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57884/
> -----------------------------------------------------------
>
> (Updated June 13, 2017, 10:02 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-7268
> https://issues.apache.org/jira/browse/MESOS-7268
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 6e95315b70a5d9d3b4b21c4cf235b0a483760190
>
>
> Diff: https://reviews.apache.org/r/57884/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Silas Snider
>
>
Re: Review Request 57884: Ensure that host /etc/* files are mounted
RDONLY by the CNI Isolator.
Posted by Silas Snider <sw...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57884/
-----------------------------------------------------------
(Updated June 13, 2017, 10:02 p.m.)
Review request for mesos and Jie Yu.
Bugs: MESOS-7268
https://issues.apache.org/jira/browse/MESOS-7268
Repository: mesos
Description
-------
Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 6e95315b70a5d9d3b4b21c4cf235b0a483760190
Diff: https://reviews.apache.org/r/57884/diff/3/
Changes: https://reviews.apache.org/r/57884/diff/2-3/
Testing
-------
Thanks,
Silas Snider
Re: Review Request 57884: Ensure that host /etc/* files are mounted
RDONLY by the CNI Isolator.
Posted by Jie Yu <yu...@gmail.com>.
> On April 6, 2017, 12:22 a.m., Jie Yu wrote:
> > src/tests/containerizer/linux_filesystem_isolator_tests.cpp
> > Lines 1523 (patched)
> > <https://reviews.apache.org/r/57884/diff/2/?file=1685497#file1685497line1523>
> >
> > Instead of putting this test under LinuxFilesystemIsolatorTest, I would put it under CniIsolatorTest. The code change is in CNI isolator, so putting it in CniIsolatorTest is more appropriate?
> >
> > I would write a similar test like this test:
> > TEST_F(CniIsolatorTest, ROOT_OverrideHostname)
>
> Silas Snider wrote:
> I disagree (though not super strongly) -- I think that the functionality being guarded/tested is that the linux fs isolator correctly mounts the /etc/ networking files readonly. That it does so *today* by stealth-including the CNI isolator is an implementation detail, and a surprising one at that. To protect against the general case, I put it here.
>
> If you still think it should move, I'm totally willing to move it though.
the mount is done by the CNI isolator, not linux filesystem isolator. Even in the future, we allow isolator dependency, and let linux filesystem isolator handles all the bind mount, this is still a logic (read-only bind mount for /etc) introduced by CNI isolator. Put that in other words, if we disable cni isolator and enable linux filesystem isolator, your test will fail.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57884/#review171183
-----------------------------------------------------------
On April 5, 2017, 7:53 p.m., Silas Snider wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57884/
> -----------------------------------------------------------
>
> (Updated April 5, 2017, 7:53 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-7268
> https://issues.apache.org/jira/browse/MESOS-7268
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 6e95315b70a5d9d3b4b21c4cf235b0a483760190
> src/tests/containerizer/linux_filesystem_isolator_tests.cpp 5e489ef6a522000c55b0fb9a27bce2567f82bb73
>
>
> Diff: https://reviews.apache.org/r/57884/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Silas Snider
>
>
Re: Review Request 57884: Ensure that host /etc/* files are mounted
RDONLY by the CNI Isolator.
Posted by Silas Snider <sw...@apple.com>.
> On April 6, 2017, 12:22 a.m., Jie Yu wrote:
> > I would actually split this patch into two patches (one for code one for test) so that we can easily backport the code if we want to.
Done.
> On April 6, 2017, 12:22 a.m., Jie Yu wrote:
> > src/tests/containerizer/linux_filesystem_isolator_tests.cpp
> > Lines 1523 (patched)
> > <https://reviews.apache.org/r/57884/diff/2/?file=1685497#file1685497line1523>
> >
> > Instead of putting this test under LinuxFilesystemIsolatorTest, I would put it under CniIsolatorTest. The code change is in CNI isolator, so putting it in CniIsolatorTest is more appropriate?
> >
> > I would write a similar test like this test:
> > TEST_F(CniIsolatorTest, ROOT_OverrideHostname)
I disagree (though not super strongly) -- I think that the functionality being guarded/tested is that the linux fs isolator correctly mounts the /etc/ networking files readonly. That it does so *today* by stealth-including the CNI isolator is an implementation detail, and a surprising one at that. To protect against the general case, I put it here.
If you still think it should move, I'm totally willing to move it though.
- Silas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57884/#review171183
-----------------------------------------------------------
On April 5, 2017, 7:53 p.m., Silas Snider wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57884/
> -----------------------------------------------------------
>
> (Updated April 5, 2017, 7:53 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-7268
> https://issues.apache.org/jira/browse/MESOS-7268
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 6e95315b70a5d9d3b4b21c4cf235b0a483760190
> src/tests/containerizer/linux_filesystem_isolator_tests.cpp 5e489ef6a522000c55b0fb9a27bce2567f82bb73
>
>
> Diff: https://reviews.apache.org/r/57884/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Silas Snider
>
>
Re: Review Request 57884: Ensure that host /etc/* files are mounted
RDONLY by the CNI Isolator.
Posted by Silas Snider <sw...@apple.com>.
> On April 6, 2017, 12:22 a.m., Jie Yu wrote:
> > I would actually split this patch into two patches (one for code one for test) so that we can easily backport the code if we want to.
>
> Silas Snider wrote:
> Done.
(see https://reviews.apache.org/r/58250/)
- Silas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57884/#review171183
-----------------------------------------------------------
On April 5, 2017, 7:53 p.m., Silas Snider wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57884/
> -----------------------------------------------------------
>
> (Updated April 5, 2017, 7:53 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-7268
> https://issues.apache.org/jira/browse/MESOS-7268
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 6e95315b70a5d9d3b4b21c4cf235b0a483760190
> src/tests/containerizer/linux_filesystem_isolator_tests.cpp 5e489ef6a522000c55b0fb9a27bce2567f82bb73
>
>
> Diff: https://reviews.apache.org/r/57884/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Silas Snider
>
>
Re: Review Request 57884: Ensure that host /etc/* files are mounted
RDONLY by the CNI Isolator.
Posted by Silas Snider <sw...@apple.com>.
> On April 6, 2017, 12:22 a.m., Jie Yu wrote:
> > src/tests/containerizer/linux_filesystem_isolator_tests.cpp
> > Lines 1523 (patched)
> > <https://reviews.apache.org/r/57884/diff/2/?file=1685497#file1685497line1523>
> >
> > Instead of putting this test under LinuxFilesystemIsolatorTest, I would put it under CniIsolatorTest. The code change is in CNI isolator, so putting it in CniIsolatorTest is more appropriate?
> >
> > I would write a similar test like this test:
> > TEST_F(CniIsolatorTest, ROOT_OverrideHostname)
>
> Silas Snider wrote:
> I disagree (though not super strongly) -- I think that the functionality being guarded/tested is that the linux fs isolator correctly mounts the /etc/ networking files readonly. That it does so *today* by stealth-including the CNI isolator is an implementation detail, and a surprising one at that. To protect against the general case, I put it here.
>
> If you still think it should move, I'm totally willing to move it though.
>
> Jie Yu wrote:
> the mount is done by the CNI isolator, not linux filesystem isolator. Even in the future, we allow isolator dependency, and let linux filesystem isolator handles all the bind mount, this is still a logic (read-only bind mount for /etc) introduced by CNI isolator. Put that in other words, if we disable cni isolator and enable linux filesystem isolator, your test will fail.
I finally got the CNI tests working locally per our slack discussion yesterday, so I now have a working test that fails before this change and succeeds afterwards in the CNI isolator tests in review request https://reviews.apache.org/r/58250/
- Silas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57884/#review171183
-----------------------------------------------------------
On April 5, 2017, 7:53 p.m., Silas Snider wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57884/
> -----------------------------------------------------------
>
> (Updated April 5, 2017, 7:53 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-7268
> https://issues.apache.org/jira/browse/MESOS-7268
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 6e95315b70a5d9d3b4b21c4cf235b0a483760190
> src/tests/containerizer/linux_filesystem_isolator_tests.cpp 5e489ef6a522000c55b0fb9a27bce2567f82bb73
>
>
> Diff: https://reviews.apache.org/r/57884/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Silas Snider
>
>
Re: Review Request 57884: Ensure that host /etc/* files are mounted
RDONLY by the CNI Isolator.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57884/#review171183
-----------------------------------------------------------
Fix it, then Ship it!
I would actually split this patch into two patches (one for code one for test) so that we can easily backport the code if we want to.
src/tests/containerizer/linux_filesystem_isolator_tests.cpp
Line 1520 (original), 1520 (patched)
<https://reviews.apache.org/r/57884/#comment244062>
2 lines apart.
src/tests/containerizer/linux_filesystem_isolator_tests.cpp
Lines 1523 (patched)
<https://reviews.apache.org/r/57884/#comment244063>
Instead of putting this test under LinuxFilesystemIsolatorTest, I would put it under CniIsolatorTest. The code change is in CNI isolator, so putting it in CniIsolatorTest is more appropriate?
I would write a similar test like this test:
TEST_F(CniIsolatorTest, ROOT_OverrideHostname)
- Jie Yu
On April 5, 2017, 7:53 p.m., Silas Snider wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57884/
> -----------------------------------------------------------
>
> (Updated April 5, 2017, 7:53 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-7268
> https://issues.apache.org/jira/browse/MESOS-7268
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 6e95315b70a5d9d3b4b21c4cf235b0a483760190
> src/tests/containerizer/linux_filesystem_isolator_tests.cpp 5e489ef6a522000c55b0fb9a27bce2567f82bb73
>
>
> Diff: https://reviews.apache.org/r/57884/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Silas Snider
>
>
Re: Review Request 57884: Ensure that host /etc/* files are mounted
RDONLY by the CNI Isolator.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57884/#review171168
-----------------------------------------------------------
Patch looks great!
Reviews applied: [57884]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On April 5, 2017, 7:53 p.m., Silas Snider wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57884/
> -----------------------------------------------------------
>
> (Updated April 5, 2017, 7:53 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-7268
> https://issues.apache.org/jira/browse/MESOS-7268
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 6e95315b70a5d9d3b4b21c4cf235b0a483760190
> src/tests/containerizer/linux_filesystem_isolator_tests.cpp 5e489ef6a522000c55b0fb9a27bce2567f82bb73
>
>
> Diff: https://reviews.apache.org/r/57884/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Silas Snider
>
>
Re: Review Request 57884: Ensure that host /etc/* files are mounted
RDONLY by the CNI Isolator.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57884/#review177807
-----------------------------------------------------------
Fix it, then Ship it!
src/tests/containerizer/linux_filesystem_isolator_tests.cpp
Lines 1546-1547 (patched)
<https://reviews.apache.org/r/57884/#comment251533>
The file won't even exist in the rootfs? What's the purpose of this test? I'll just just remove this test.
- Jie Yu
On April 5, 2017, 7:53 p.m., Silas Snider wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57884/
> -----------------------------------------------------------
>
> (Updated April 5, 2017, 7:53 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-7268
> https://issues.apache.org/jira/browse/MESOS-7268
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 6e95315b70a5d9d3b4b21c4cf235b0a483760190
> src/tests/containerizer/linux_filesystem_isolator_tests.cpp 5e489ef6a522000c55b0fb9a27bce2567f82bb73
>
>
> Diff: https://reviews.apache.org/r/57884/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Silas Snider
>
>