mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Adam B <a...@mesosphere.io>
Subject Re: Review Request 53541: Added authorization actions for Nested Container and Debug API.
Date Tue, 29 Nov 2016 09:47:23 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message