Re: Review Request 32975: MESOS-1790 Adds chown option to CommandInfo.URI

[Adam B <ad...@mesosphere.io>]

> On April 8, 2015, 2:57 p.m., Vinod Kone wrote:
> > src/tests/fetcher_tests.cpp, lines 112-116
> > <https://reviews.apache.org/r/32975/diff/1/?file=920880#file920880line112>
> >
> >     Add a note/todo here mentioning why you are setting these fields but not doing any asserts/expects on them.
> 
> Jim Klucar wrote:
>     With the type_utils.cpp change, the EXPECT_EQ on lines 140/141 should be checking the chown field. Should I still add text regarding how we can't check this capability without assuming the user running the test has permission to chown to some other user?
> 
> Vinod Kone wrote:
>     Yes. Because the EXPECT on #140 just checks that the chown field is properly propagated to fetcher environment but not that fetcher actually skips chown if chown is false.
>     
>     If you write a FetcherTest (see examples later in the file) that actually runs a fetcher process, would you able to test the semantics? I'm thinking that you can create a non-existent user (random string) and set chown to false. Once fetcher process finishes, you can check that the owner of the fetched file hasn't changed to the user. Does that make sense?
> 
> Jim Klucar wrote:
>     This makes sense. I'll create the test, back out the change, see the test fail, then add the change back in.
> 
> Jim Klucar wrote:
>     I ended up creating two new tests. Both extract a framework, one sets chown true and expects a failure due to the unknown user name. The other sets chown to false and expects the fetcher to finish without error.

FYI, if you prefix the test name with `ROOT_` then it will only be run when `make check` is run as root. That way you can provide tests that exercise the chown permissions.


- Adam


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


On April 21, 2015, 10 a.m., Jim Klucar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32975/
> -----------------------------------------------------------
> 
> (Updated April 21, 2015, 10 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-1790
>     https://issues.apache.org/jira/browse/MESOS-1790
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added chown to CommandInfo.URI protocol buffer as an optional
> boolean that defaults to true, the current chown behavior.
> 
> The fetcher was updated to skip the os::chown operation if the chown
> boolean is set to false.
> 
> No documentation was updated.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 3c592d5ab3092ecbeddfaff95e0c1addc3ac58f8 
>   src/common/type_utils.cpp e92f6f36de0955784619029a016667b46bbe221b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
> 
> Diff: https://reviews.apache.org/r/32975/diff/
> 
> 
> Testing
> -------
> 
> Unit testing this functionality is difficult because it would require that the user running the test to have permission to chown a file to someone other than themselves. I didn't want to add that as a requirement to build. I added the new field to the existing test cases just to see that they populate.
> 
> 
> Thanks,
> 
> Jim Klucar
> 
>