> On July 31, 2018, 5:38 a.m., Chun-Hung Hsiao wrote: > > src/slave/http.cpp > > Lines 1818 (patched) > > <https://reviews.apache.org/r/68104/diff/1/?file=2065137#file2065137line1818> > > > > Conventionally we only indent this by 4 extra spaces aligning with > > "return".
Good catch, I filed https://issues.apache.org/jira/browse/MESOS-9121. > On July 31, 2018, 5:38 a.m., Chun-Hung Hsiao wrote: > > src/slave/http.cpp > > Lines 1821-1822 (patched) > > <https://reviews.apache.org/r/68104/diff/1/?file=2065137#file2065137line1821> > > > > The following can be fit into 80 characters: > > ``` > > [this, acceptType](const Owned<ObjectApprover>& approvers) -> > > Response { > > ``` > > Or for here it seems harmless to just use `[=]`. I'm fine with either > > though. That line seems to be exactly 81 characters long, so it looks like we do need to split somewhere. I now manually adjusted this to not split the capture list (the previous version was formated by our `clang-format`). I don't think that implicitly capturing pointers to not immediately invoked lambdas is safe style, much less when capturing `this` like here. I'll keep the explicit capture. > On July 31, 2018, 5:38 a.m., Chun-Hung Hsiao wrote: > > src/slave/http.cpp > > Lines 1824 (patched) > > <https://reviews.apache.org/r/68104/diff/1/?file=2065137#file2065137line1824> > > > > Should we return an empty list of resource provider infos or return a > > 403 Forbidden? Good point, changed to return an empty set when not authorized. This maps well on e.g., specifying sets of RPs somebody can view in the local authorizer. > On July 31, 2018, 5:38 a.m., Chun-Hung Hsiao wrote: > > src/tests/api_tests.cpp > > Lines 7002-7004 (original), 7002-7021 (patched) > > <https://reviews.apache.org/r/68104/diff/1/?file=2065138#file2065138line7002> > > > > How about the following: > > ``` > > slave::Flags slaveFlags = CreateSlaveFlags(); > > slaveFlags.authenticate_http_readwrite = true; > > > > { > > // `DEFAULT_CREDENTIAL_2` is not allowed to view any resource > > provider. > > mesos::ACL::ViewResourceProvider* acl = > > slaveFlags.acls->add_view_resource_providers(); > > > > acl->mutable_principals()->add_values(DEFAULT_CREDENTIAL_2.principal()); > > acl->mutable_resource_providers()->set_type(mesos::ACL::Entity::NONE); > > { > > > > Try<Owned<cluster::Slave>> slave = StartSlave(&detector, slaveFlags); > > ``` Great suggestion, applied. I did not set `authenticate_http_readwrite` though since it is not required. > On July 31, 2018, 5:38 a.m., Chun-Hung Hsiao wrote: > > src/tests/api_tests.cpp > > Lines 7046-7049 (patched) > > <https://reviews.apache.org/r/68104/diff/1/?file=2065138#file2065138line7046> > > > > I'm a bit against checking content of the failure string since it's > > slightly hard to maintain. Can we avoid this? I removed this check for now as knowning that the request failed already tell us _something_. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68104/#review206637 ----------------------------------------------------------- On July 31, 2018, 12:19 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68104/ > ----------------------------------------------------------- > > (Updated July 31, 2018, 12:19 p.m.) > > > Review request for mesos, Chun-Hung Hsiao and Jan Schlicht. > > > Bugs: MESOS-8314 > https://issues.apache.org/jira/browse/MESOS-8314 > > > Repository: mesos > > > Description > ------- > > Implemented authorization for agent `GET_RESOURCE_PROVIDER` calls. > > > Diffs > ----- > > src/slave/http.cpp ab5864d9fd2fde478ed7da2ca7ed8abedc72c7c5 > src/tests/api_tests.cpp 182622a62d350ebefc891a385de3f2d35a7c0243 > > > Diff: https://reviews.apache.org/r/68104/diff/2/ > > > Testing > ------- > > `make check` > > > Thanks, > > Benjamin Bannier > >
