mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jan Schlicht" <...@mesosphere.io>
Subject Re: Review Request 39520: Quota: Added authentication, authorization tests.
Date Fri, 18 Dec 2015 09:20:06 GMT


> On Dec. 14, 2015, 4:52 p.m., Till Toenshoff wrote:
> > src/tests/authorization_tests.cpp, lines 591-592
> > <https://reviews.apache.org/r/39520/diff/7/?file=1158400#file1158400line591>
> >
> >     Why would we check a request with multiple principals in this integration test?
> 
> Greg Mann wrote:
>     I would say this is closer to a unit test, since we're not testing realistic attempts
at authorization, but are rather just exercising the authorization of ACLs without the extra
bits for communication with a framework or operator. While it's true that we don't have any
current use cases for multiple principals in a request, the ACLs do allow it, so perhaps it's
reasonable to test this? I don't have strong feelings either way; maybe best to just split
them into two requests to make the test closer to real-world usage patterns?
> 
> Till Toenshoff wrote:
>     Greg, you are right - it is meant to be a unit test. 
>     
>     Right now my assumption is that our API allows multiple principals for requests as
a result of the implementation details -- streamlined re-use of that `Entity` concept making
that matrix check rather slick. I do not think that this feature was/is a goal for that API.
So I agree that testing this option can be done in a dedicated unit-test even if it was only
for documenting this option. I also think repeating that test over and over for all kinds
of endpoints / ACLs should be avoided as it seems of little value and even confusing to the
reader that tries to see how this would ever happen in reality.

The tests now only use a single principal in the request.


> On Dec. 14, 2015, 4:52 p.m., Till Toenshoff wrote:
> > src/tests/master_quota_tests.cpp, line 1014
> > <https://reviews.apache.org/r/39520/diff/7/?file=1158401#file1158401line1014>
> >
> >     Can you please also add a test that handles a missing principle in the request?

The new test fixture `AuthorizedQuotaSetRequestWithoutPrincipal` now does that.


- Jan


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


On Dec. 18, 2015, 10:12 a.m., Jan Schlicht wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39520/
> -----------------------------------------------------------
> 
> (Updated Dec. 18, 2015, 10:12 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
> 
> 
> Bugs: MESOS-4082
>     https://issues.apache.org/jira/browse/MESOS-4082
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Quota: Added authentication, authorization tests.
> 
> 
> Diffs
> -----
> 
>   src/tests/authorization_tests.cpp dcf348acc75b143e3a854bde17e6042dc90599d5 
>   src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74 
> 
> Diff: https://reviews.apache.org/r/39520/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>


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