mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.
Date Fri, 22 Apr 2016 14:05:39 GMT


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


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