----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54535/#review158660 -----------------------------------------------------------
I'm a bit confused why we don't just have a simple single `ViewContainer` ACL based on the (Executor) container's user. Why bother with the role here if we don't in `ViewTask`/`ViewExecutor`? include/mesos/authorizer/acls.proto (line 351) <https://reviews.apache.org/r/54535/#comment229451> Let's think for a minute how this will change when we have a multi-role framework. The framework may be subscribed for multiple roles foo and bar, but only uses one (foo) to launch this container. Should users with access to ViewContainerOfFrameworkWithRole=bar be able to access a container launched using foo resources? I would think not. In which case, the "role" more specifically describes the container, not the framework. In which case, this becomes `ViewContainerWithRole` include/mesos/authorizer/acls.proto (lines 355 - 356) <https://reviews.apache.org/r/54535/#comment229452> "The list of roles whose container metadata the principal can see." include/mesos/authorizer/acls.proto (line 367) <https://reviews.apache.org/r/54535/#comment229453> Is this necessarily a "nested container"? Then perhaps `ViewNestedContainer[RunningAsUser]` would be more appropriate. include/mesos/authorizer/acls.proto (line 371) <https://reviews.apache.org/r/54535/#comment229454> s/authorized change/authorized to change/ include/mesos/authorizer/acls.proto (lines 443 - 445) <https://reviews.apache.org/r/54535/#comment229449> It seems we didn't include the `AsUser` or `WithRole` in the ACL names for `RunTask` or `RegisterFramework`, so maybe we don't need to here either (or in the nested container ACLs), since we already have the Object Entities named `roles` or `users`. `repeated ACL.ViewContainerOfFramework view_containers_of_framework = 31;` `repeated ACL.ViewContainer view_containers = 32;` Why do we even need both of these ACLs? Why not just `ViewContainer` with Object: `users`, just like ViewTask/ViewExecutor? src/authorizer/local/authorizer.cpp (line 1164) <https://reviews.apache.org/r/54535/#comment229456> s/view_flags/set_log_level/ src/tests/authorization_tests.cpp (line 4154) <https://reviews.apache.org/r/54535/#comment229457> s/see the flags/set log level/g here and in other comments - Adam B On Dec. 8, 2016, 7:38 a.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54535/ > ----------------------------------------------------------- > > (Updated Dec. 8, 2016, 7:38 a.m.) > > > Review request for mesos and Adam B. > > > Bugs: MESOS-6670 > https://issues.apache.org/jira/browse/MESOS-6670 > > > Repository: mesos > > > Description > ------- > > Adds the authorization action `VIEW_CONTAINERS` which takes an object > of type `FrameworkInfo` and `ExecutorInfo` and optionally a > `CommandInfo`. > > It also adds the authorization action `SET_LOG_LEVEL` which takes no > object. > > Includes testing for the ACLs and interface. > > > Diffs > ----- > > include/mesos/authorizer/acls.proto > 3499cac43d77c371448a9c9e1ac95d42b24a54dd > include/mesos/authorizer/authorizer.proto > b7371cd580bbe64eb1566876d0f253eedbd0789d > src/authorizer/local/authorizer.cpp > 3b983d0c0dea3ad761e7c684a9f943809dc541e9 > src/tests/authorization_tests.cpp f70d60d73a64e7eb4be20a17b62a8726b659e1b8 > > Diff: https://reviews.apache.org/r/54535/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Alexander Rojas > >
