> 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.
> 
> Adam B wrote:
>     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.
> 
> Jim Klucar wrote:
>     I wasn't aware of that. I'll give it a shot. Is there a standard way 
> other tests have done similar things? For instance, create a new user, 
> destroy the user, assume some other non-root user exists, etc.

Looks like UserCgroupIsolatorTest creates/destroys user 
"mesos.test.unprivileged.user" on test SetUp/TearDown, assuming that it doesn't 
exist.
On the other hand, ROOT_RunTaskWithCommandInfoWithUser tried assuming that 
'nobody' was present (and could access mesos-executor), but that failed on some 
systems. See MESOS-2199


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

Reply via email to