Re: Review Request 57884: Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.

[Jie Yu]

---
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.

[Silas Snider]

---
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.

[Silas Snider]

> 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.

[Jie Yu]

> 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.

[Jie Yu]

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


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)


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.

[Silas Snider]

---
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.

[Jie Yu]

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


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
> 
>

Re: Review Request 57884: Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.

[Silas Snider]

> On April 6, 2017, 12:22 a.m., Jie Yu wrote:
> > src/tests/containerizer/linux_filesystem_isolator_tests.cpp
> > Lines 1523 (patched)
> > 
> >
> > 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.

[Jie Yu]

> On April 6, 2017, 12:22 a.m., Jie Yu wrote:
> > src/tests/containerizer/linux_filesystem_isolator_tests.cpp
> > Lines 1523 (patched)
> > 
> >
> > 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.

[Silas Snider]

> 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.

[Silas Snider]

> 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)
> > 
> >
> > 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.

[Jie Yu]

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


2 lines apart.



src/tests/containerizer/linux_filesystem_isolator_tests.cpp
Lines 1523 (patched)


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.

[Mesos Reviewbot]

---
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.

[Silas Snider]

---
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.

[Silas Snider]

- Silas


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


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.

[Silas Snider]

> On March 27, 2017, 5:19 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
> > Line 1910 (original), 1910 (patched)
> > 
> >
> > Are you sure this works? I remembered that for read only bind mount, 
> > you need to do a bind mount and a remount with read only flag.
> > https://lwn.net/Articles/281157/
> > 
> > That probably means we should add a unit test for this. Take a look at 
> > CniIsolatorTest.ROOT_OverrideHostname which will give you some idea how to 
> > adding a unit test for this.

Sorry it took so long -- I was pulled onto another project briefly. I've 
updated to match the recommendation, confirmed that it works by adding a test 
which I confirmed passes now, but does not without my change.


- Silas


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


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.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Line 1910 (original), 1910 (patched)


Are you sure this works? I remembered that for read only bind mount, you 
need to do a bind mount and a remount with read only flag.
https://lwn.net/Articles/281157/

That probably means we should add a unit test for this. Take a look at 
CniIsolatorTest.ROOT_OverrideHostname which will give you some idea how to 
adding a unit test for this.


- Jie Yu


On March 23, 2017, 5:11 p.m., Silas Snider wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57884/
> ---
> 
> (Updated March 23, 2017, 5:11 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/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Silas Snider
> 
>

Re: Review Request 57884: Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.

[Mesos Reviewbot]

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



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 March 23, 2017, 5:11 p.m., Silas Snider wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57884/
> ---
> 
> (Updated March 23, 2017, 5:11 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/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Silas Snider
> 
>

Review Request 57884: Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.

[Silas Snider]

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

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/1/


Testing
---


Thanks,

Silas Snider