mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 46260: Added a MetricsTest with authentication to libprocess.
Date Fri, 15 Apr 2016 17:46:37 GMT


> On April 15, 2016, 11:22 a.m., Alexander Rojas wrote:
> > 3rdparty/libprocess/src/tests/metrics_tests.cpp, line 551
> > <https://reviews.apache.org/r/46260/diff/1/?file=1346281#file1346281line551>
> >
> >     Can you please add a test where authentication actually works? (for saftey)

I wonder if we really need this? I've written some previous tests along those lines (which
test full endpoint functionality both with and without authentication), but after some conversations
with bmahler recently I'm not sure this is a good idea. Since we don't currently use the authenticated
`principal` at all in the endpoint handler, we really just need to test here that unauthenticated
requests receive an `Unauthorized` response. The `HttpAuthenticationTest.Authenticated` test
in libprocess already tests that a properly authenticated request will succeed in reaching
the HTTP endpoint and passing the correct principal to the handler, so we don't need to test
that again. Once authorization is enabled on this endpoint, it will make sense to have successfully
authenticated test cases in order to test the authorization code, but at this point I think
it would introduce unnecessary tests and increase the code maintenance burden. What do you
think?


- Greg


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


On April 15, 2016, 6:58 a.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46260/
> -----------------------------------------------------------
> 
> (Updated April 15, 2016, 6:58 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-4902
>     https://issues.apache.org/jira/browse/MESOS-4902
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The test `MetricsTest.SnapshotAuthenticationEnabled`
> is added to the libprocess tests in this patch.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/tests/metrics_tests.cpp b84dc8d858f58bc9f52b218b7153510417cf34c2

> 
> Diff: https://reviews.apache.org/r/46260/diff/
> 
> 
> Testing
> -------
> 
> `sudo make check` on OSX.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


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