mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Adam B <a...@mesosphere.io>
Subject Re: Review Request 47805: Add authorization to GET /weights.
Date Fri, 27 May 2016 09:38:13 GMT


> On May 26, 2016, 3:30 a.m., Adam B wrote:
> > src/master/weights_handler.cpp, line 85
> > <https://reviews.apache.org/r/47805/diff/1/?file=1392984#file1392984line85>
> >
> >     s/=/request, weightInfos/
> >     s/list<bool>/list<bool>&/
> 
> zhou xing wrote:
>     Adam, this function also requires to capture 'this', which will be used in the invocation
of '_get()', can we keep using '=' here?

Yeah, ok. I'm usually in favor of being explicit when we can, but others favor readability.
Dropping.


> On May 26, 2016, 3:30 a.m., Adam B wrote:
> > src/tests/dynamic_weights_tests.cpp, lines 492-494
> > <https://reviews.apache.org/r/47805/diff/1/?file=1392985#file1392985line492>
> >
> >     Seems like `checkWithGetRequest` should take the credential as a parameter,
rather than hiding that credential-selection logic inside the check method.
> 
> zhou xing wrote:
>     as in checkWithGetRequest, we are using the constant strings to identify the result
of get weights, so for 'get weights' specific test cases, I suggest keep these two new constant
strings. I will add a new credential parameter to checkWithGetRequest method and remove the
credential-selection logic in it.

Thanks, but let's remove the default value too, so the credential must always be specified.


> On May 26, 2016, 3:30 a.m., Adam B wrote:
> > src/tests/dynamic_weights_tests.cpp, line 512
> > <https://reviews.apache.org/r/47805/diff/1/?file=1392985#file1392985line512>
> >
> >     Since this test already uses a principal, why not only allow that principal
to get the weights?
> >     Then you can also test that a get request without a principal (or with CREDENTIAL_2)
will see no roles.
> 
> zhou xing wrote:
>     Adam, yes, I will only allow DEFAULT_CREDENTIAL in these test cases, but for the
get check, as I already added a new test case to test "get weights with role" only, I suggest
not to add a new get request test in this test case. Or we can merge the get weights test
and update weights test together into one, please let me know your comments, thanks.

If it's already covered by another test, we can leave it at that. Dropping.


- Adam


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47805/#review134954
-----------------------------------------------------------


On May 26, 2016, 10:45 p.m., zhou xing wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47805/
> -----------------------------------------------------------
> 
> (Updated May 26, 2016, 10:45 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: mesos-5335
>     https://issues.apache.org/jira/browse/mesos-5335
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add 'GET_WEIGHTS_WITH_ROLE' for the authorization of GET /weights.
> 
> 
> Diffs
> -----
> 
>   docs/endpoints/master/weights.md 1e540795c5fb90c8ea957dd444b9e564e0b1ac23 
>   include/mesos/authorizer/acls.proto ace9b698f46e1437911115c82324a87a0d7827fb 
>   include/mesos/authorizer/authorizer.proto 02d1a01d57cf34b38524f4368187878b03343537

>   src/authorizer/local/authorizer.cpp 3c7c791bde65cfcbcc4e319c9ccc487ab37d8029 
>   src/master/http.cpp b36b439a1fa07c52146deff2b90728f92676ade3 
>   src/master/master.hpp 1a875c32eddfb6d884e3d0dda7f5716ee53966c3 
>   src/master/weights_handler.cpp 4bc060fdb015df6658194eef92fe11b14aa15c79 
>   src/tests/dynamic_weights_tests.cpp 362c59aae7b305710d5985bfec28f881be3b64b8 
> 
> Diff: https://reviews.apache.org/r/47805/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> 
> Thanks,
> 
> zhou xing
> 
>


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