----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53541/#review157211 -----------------------------------------------------------
Looks great! Nothing too major. I think we can ship this once you've addressed these comments. - In a follow-up patch, please update authorization.md with the new actions/ACLs. include/mesos/authorizer/acls.proto (line 327) <https://reviews.apache.org/r/53541/#comment227705> s/containers/container/ include/mesos/authorizer/acls.proto (line 334) <https://reviews.apache.org/r/53541/#comment227707> s/containers/nested containers/ ... whose nested containers can be killed. include/mesos/authorizer/acls.proto (line 338) <https://reviews.apache.org/r/53541/#comment227706> s/containers/container/ include/mesos/authorizer/authorizer.proto (line 162) <https://reviews.apache.org/r/53541/#comment227708> s/an object/objects/ src/authorizer/local/authorizer.cpp (line 471) <https://reviews.apache.org/r/53541/#comment227709> Why check the command_info? It's optional either way, so leave it out. Your current logic would allow an executor_info without a framework_info (or vice versa) as long as there's a command_info. I don't think that's what we want. I assumed you were trying to check that if there's an object present, then it should have both executor_info and framework_info. include/mesos/authorizer/acls.proto (line 282) <https://reviews.apache.org/r/53541/#comment227711> s/containers/container/ include/mesos/authorizer/acls.proto (line 293) <https://reviews.apache.org/r/53541/#comment227712> s/containers/container/ src/authorizer/local/authorizer.cpp (lines 468 - 471) <https://reviews.apache.org/r/53541/#comment227714> When will object ever be None? src/authorizer/local/authorizer.cpp (line 491) <https://reviews.apache.org/r/53541/#comment227715> Don't you mean `framework-info->user()`? src/authorizer/local/authorizer.cpp (line 535) <https://reviews.apache.org/r/53541/#comment227716> s/user/child/ or s/user/command/? src/authorizer/local/authorizer.cpp (lines 565 - 568) <https://reviews.apache.org/r/53541/#comment227710> Maybe let's call it the `commandApprover` or `childApprover`, since we could technically be approving anything about the child command, not just its user. We're more generally concerned about approving the subject to perform the action a) for a child container with the given command_info, and b) under the given parent executor. src/tests/authorization_tests.cpp (line 2517) <https://reviews.apache.org/r/53541/#comment227739> No test for `LaunchNestedContainer`? Should be identical except for comments, but probably worth including. src/tests/authorization_tests.cpp (line 2523) <https://reviews.apache.org/r/53541/#comment227717> s/nested/parent/ src/tests/authorization_tests.cpp (line 2539) <https://reviews.apache.org/r/53541/#comment227718> "bar" can launch sessions nested under a container running as linux user "bar" src/tests/authorization_tests.cpp (line 2546) <https://reviews.apache.org/r/53541/#comment227719> What's the comment here? I can't tell if this is equivalent to a permissive=false for this ACL. src/tests/authorization_tests.cpp (line 2554) <https://reviews.apache.org/r/53541/#comment227720> `"foo" principal cannot launch nested container sessions as any user.`? Can foo still launch container_info's instead of command_info's? src/tests/authorization_tests.cpp (line 2562) <https://reviews.apache.org/r/53541/#comment227721> s/in containers // src/tests/authorization_tests.cpp (line 2570) <https://reviews.apache.org/r/53541/#comment227722> s/in all containers/as any linux user/ src/tests/authorization_tests.cpp (line 2578) <https://reviews.apache.org/r/53541/#comment227723> s/in nested containers/as any user/ src/tests/authorization_tests.cpp (lines 2613 - 2614) <https://reviews.apache.org/r/53541/#comment227724> And an ExecutorInfo with a ContainerInfo instead of a CommandInfo? We ought to test `command_info.isNone() src/tests/authorization_tests.cpp (line 2618) <https://reviews.apache.org/r/53541/#comment227725> Want to give it a unique executorId, maybe `n`? src/tests/authorization_tests.cpp (line 2662) <https://reviews.apache.org/r/53541/#comment227726> ... running with the default frameworkInfo.user "user" src/tests/authorization_tests.cpp (lines 2709 - 2711) <https://reviews.apache.org/r/53541/#comment227727> `commandInfoNoUser.set_value("echo hello");` No `executorInfoNoUser` needed. src/tests/authorization_tests.cpp (lines 2744 - 2745) <https://reviews.apache.org/r/53541/#comment227728> Principal "bar" can launch a session as user "bar" under parent containers running as user "bar" src/tests/authorization_tests.cpp (line 2759) <https://reviews.apache.org/r/53541/#comment227729> s/cannot/can/ src/tests/authorization_tests.cpp (lines 2772 - 2775) <https://reviews.apache.org/r/53541/#comment227731> Could you please also test: - executorInfoNoUser (defaults to frameworkInfo's "user") + commandInfo[Bar] - executorInfoNoUser + commandInfoNoUser src/tests/authorization_tests.cpp (line 2934) <https://reviews.apache.org/r/53541/#comment227738> s/input/output/ throughout this test src/tests/authorization_tests.cpp (line 3206) <https://reviews.apache.org/r/53541/#comment227737> s/bar/ops/ here and in other tests src/tests/authorization_tests.cpp (line 3222) <https://reviews.apache.org/r/53541/#comment227732> s/killing for/killing/ src/tests/authorization_tests.cpp (line 3298) <https://reviews.apache.org/r/53541/#comment227733> s/wait for/kill/ src/tests/authorization_tests.cpp (line 3311) <https://reviews.apache.org/r/53541/#comment227734> s/wait for/kill/ src/tests/authorization_tests.cpp (line 3324) <https://reviews.apache.org/r/53541/#comment227735> s/wait for/kill/ in the rest of these comments src/tests/authorization_tests.cpp (line 3351) <https://reviews.apache.org/r/53541/#comment227736> s/bar/ops/ - Adam B On Nov. 28, 2016, 9:32 a.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53541/ > ----------------------------------------------------------- > > (Updated Nov. 28, 2016, 9:32 a.m.) > > > Review request for mesos, Adam B, Kapil Arya, Kevin Klues, and Till Toenshoff. > > > Bugs: MESOS-6474 > https://issues.apache.org/jira/browse/MESOS-6474 > > > Repository: mesos > > > Description > ------- > > Creates new authorization action for all the API's related to > nested containers. This patch does not add the code necesary to > call use those actions, this is done in a latter patch. > > > Diffs > ----- > > include/mesos/authorizer/acls.proto > e3fd6a4a1b617a75714ebd6e08ab10cffa1a7d1b > include/mesos/authorizer/authorizer.hpp > 3cd0f84e25c0ea91a6f20eba26341a1b830d8cf2 > include/mesos/authorizer/authorizer.proto > 0696a629ac2d2b9950e20708f0c3666b58ff7ca0 > src/authorizer/local/authorizer.cpp > 77e05dd2475d6e7511e7c7eeea578ec31ff3d198 > src/tests/authorization_tests.cpp d23f551c2caa454da0c0f6cb7d77a8c2bd75a474 > > Diff: https://reviews.apache.org/r/53541/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Alexander Rojas > >
