> On Aug. 8, 2017, 9:36 a.m., Alexander Rojas wrote: > > src/master/master.cpp > > Lines 9547-9569 (patched) > > <https://reviews.apache.org/r/61189/diff/2/?file=1785310#file1785310line9547> > > > > I don't think this part should be done as it is. Consider the case when > > you have an `Acceptor` which uses a security module which connects to LDAP > > for ACLs. > > > > There is a delay for each request you make. It would be wasteful to > > create an Acceptor which you will later not use.
After discussion in the #ship-mesos-api-filter channel, we decide to keep the logic. Thus the authorizer is called for every events emitted by the stream. > On Aug. 8, 2017, 9:36 a.m., Alexander Rojas wrote: > > src/master/master.cpp > > Lines 9547-9569 (patched) > > <https://reviews.apache.org/r/61189/diff/2/?file=1785310#file1785310line9547> > > > > In fact, why not having one acceptor for each event, something like: > > > > ```c++ > > // This code doesn't necesarily compile. > > > > class EventTaskAddedAcceptor > > { > > public: > > static Owned<EventTaskAddedAcceptor> create(Principal, Authorizer); > > > > bool accept(Task, FrameworkInfo); > > > > private: > > EventTaskAddedAcceptor( > > Owned<AuthorizationAcceptor> authorizeTask, > > Owned<AuthorizationAcceptor> authorizeFramework); > > > > Owned<AuthorizationAcceptor> authorizeTask_; > > Owned<AuthorizationAcceptor> authorizeFramework_; > > }; > > > > Owned<EventTaskAddedAcceptor> EventTaskAddedAcceptor::create( > > Principal principal, > > Authorizer authorizer) > > { > > Future<Owned<AuthorizationAcceptor>> authorizeRole = > > AuthorizationAcceptor::create( > > subscriber->principal, > > subscriber->master->authorizer, > > authorization::VIEW_ROLE); > > > > Future<Owned<AuthorizationAcceptor>> authorizeFramework = > > AuthorizationAcceptor::create( > > subscriber->principal, > > subscriber->authorizer, > > authorization::VIEW_FRAMEWORK); > > > > return collect(authorizeRole, authorizeFramework) > > .then([](tuple<Owned<AuthorizationAcceptor>, > > Owned<AuthorizationAcceptor>> acceptors) { > > return new EventTaskAddedAcceptor(acceptors[0], acceptors[1]); > > }); > > } > > > > EventTaskAddedAcceptor::EventTaskAddedAcceptor( > > Owned<AuthorizationAcceptor> authorizeTask, > > Owned<AuthorizationAcceptor> authorizeFramework) > > : authorizeTask_(authorizeTask), > > authorizeFramework_(authorizeFramework) > > { > > } > > > > bool EventTaskAddedAcceptor::accept(Task task, FrameworkInfo info) > > { > > return authorizeTask->accept(task, info) && > > authorizeFramework->accept(info); > > } > > ``` > > > > This way you only need one acceptor per event and you hid the details > > on how the authorization is made inside the acceptor API and remove that > > code from the caller. Given our decision to call authorizer for every event, this combination doesn't bring in much benefit. - Quinn ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61189/#review182374 ----------------------------------------------------------- On Aug. 17, 2017, 10:59 p.m., Quinn Leng wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61189/ > ----------------------------------------------------------- > > (Updated Aug. 17, 2017, 10:59 p.m.) > > > Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann. > > > Bugs: MESOS-7785 > https://issues.apache.org/jira/browse/MESOS-7785 > > > Repository: mesos > > > Description > ------- > > Added authorization filtering for the V1 operator events on the > master, the subscriber should only receive events that are > authorized based on their principal and ACLs. Since the authorizer > is called for every event emitted on the stream, the change of > authorization rule will affect events that the subscribers can > receive. > > > Diffs > ----- > > src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54 > src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995 > src/master/master.cpp 7f38a5e21884546d4b4c866ca5918db779af8f99 > src/tests/api_tests.cpp 3ab4740bcac29ecb89585da6adb1f563d6fc1f5f > > > Diff: https://reviews.apache.org/r/61189/diff/3/ > > > Testing > ------- > > make check > GLOG_v=2 ./bin/mesos-tests.sh > --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" > --verbose --gtest_repeat=100 --gtest_break_on_failure > > > Thanks, > > Quinn Leng > >
