> On April 22, 2016, 10:22 a.m., Adam B wrote: > > src/slave/http.cpp, lines 620-622 > > <https://reviews.apache.org/r/46319/diff/2/?file=1348481#file1348481line620> > > > > Not sure why you reversed the boolean/order here, but ok.
You are right, putting the expected case first makes this more readable, fixed. > On April 22, 2016, 10:22 a.m., Adam B wrote: > > src/slave/http.cpp, line 613 > > <https://reviews.apache.org/r/46319/diff/2/?file=1348481#file1348481line613> > > > > s/context/pid/? What is the 'context' referring to? `context` is the execution context for `_statistics`, but you are right, we usually call this just some `pid`, adjusted. > On April 22, 2016, 10:22 a.m., Adam B wrote: > > src/slave/http.cpp, line 626 > > <https://reviews.apache.org/r/46319/diff/2/?file=1348481#file1348481line626> > > > > s/discard/discarded/ Fixed. > On April 22, 2016, 10:22 a.m., Adam B wrote: > > src/slave/http.cpp, lines 642-643 > > <https://reviews.apache.org/r/46319/diff/2/?file=1348481#file1348481line642> > > > > Why change the wrapping/tabbing? If you were going to drop the first > > parameter down to the next line, then each parameter must be on its own > > line. > > I would just leave the wrapping as is, personally. Revert this auto-indention fat-fingering. > On April 22, 2016, 10:22 a.m., Adam B wrote: > > src/slave/slave.hpp, lines 471-472 > > <https://reviews.apache.org/r/46319/diff/2/?file=1348482#file1348482line471> > > > > How did this come up? The original `_statistics()` is not static, and > > it had no issues. The original `statistics` did not make any use of member data in any of the `defer`ed actions (all the used data came from some `Future<ResourceUsage>` directly passed into the callback; the call of `slave->self()` was not executed in the `defer`ed action). Now here we need to access a rate limiter which is a member of `Http` which in turn might go away before `_statistics` is executed. > On April 22, 2016, 10:22 a.m., Adam B wrote: > > src/tests/slave_tests.cpp, line 1935 > > <https://reviews.apache.org/r/46319/diff/2/?file=1348483#file1348483line1935> > > > > These two parameters need to be on different lines if the whole command > > doesn't fit on one line. I'd be ok though if you moved the first param up > > to the `EXPECT_EQ(` line Fixed. > On April 22, 2016, 10:22 a.m., Adam B wrote: > > src/tests/slave_tests.cpp, line 1956 > > <https://reviews.apache.org/r/46319/diff/2/?file=1348483#file1348483line1956> > > > > Shadows the `agent` variable on line 1905 I did this on purpose, but it is indeed confusing. Changed to now create specific agents in each block below. > On April 22, 2016, 10:22 a.m., Adam B wrote: > > src/tests/slave_tests.cpp, lines 1959-1965 > > <https://reviews.apache.org/r/46319/diff/2/?file=1348483#file1348483line1959> > > > > Can't you leave these out as default? Indeed I can, changed. > On April 22, 2016, 10:22 a.m., Adam B wrote: > > src/tests/slave_tests.cpp, lines 1926-1927 > > <https://reviews.apache.org/r/46319/diff/2/?file=1348483#file1348483line1926> > > > > I would expect you to await/validate the response after validating the > > request. Wouldn't that be mixing two concerns in the same test (and e.g., lead to bloat hiding what is the crux of the test)? I think since here I check only whether an endpoint is authorized looking at the status code should be enough. > On April 22, 2016, 10:22 a.m., Adam B wrote: > > src/tests/slave_tests.cpp, line 1895 > > <https://reviews.apache.org/r/46319/diff/2/?file=1348483#file1348483line1895> > > > > Should we create a TYPED_TEST that tests this ACL in the local > > authorizer (direct and as a module), or do we only need these tests for one > > (of the many) ACCESS_ENDPOINT_WITH_PATH endpoints? I would prefer if we'd separate whether some authorizer works and whether an endpoint correctly queries the authorizer. You already suggested testing the former to Jan [here](https://reviews.apache.org/r/46203/#comment193252); how about I migrate the current test to a *parameterized test* (on the endpoint) so we have a single infrastructure to check authorization for all `GET` requests against agent endpoints? Once we start tackling e.g., `POST` requests we could extend this harness. I made this test parameterized on the agent endpoint in https://reviews.apache.org/r/46569/; if we'd want to go that direction we could squash it into this patch. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46319/#review130057 ----------------------------------------------------------- On April 22, 2016, 4:04 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46319/ > ----------------------------------------------------------- > > (Updated April 22, 2016, 4:04 p.m.) > > > Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht. > > > Bugs: MESOS-5164 > https://issues.apache.org/jira/browse/MESOS-5164 > > > Repository: mesos > > > Description > ------- > > Added authorization to agents' `/monitor/statistics` endpoints. > > > Diffs > ----- > > src/slave/http.cpp f887a71684304a82ff0d81dbd240e29aa7e2ac67 > src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5 > src/tests/slave_authorization_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/46319/diff/ > > > Testing > ------- > > make check (OS X, clang w/o optimization) > > > Thanks, > > Benjamin Bannier > >
