mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexander Rukletsov" <ruklet...@gmail.com>
Subject Re: Review Request 39043: Added support for HTTP Authentication in Mesos.
Date Thu, 07 Jan 2016 15:28:51 GMT

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


Great stuff, happy to see it landing! Some general suggestions, not necessarily for immediate
execution, but for posterity (and maybe follow-up JIRAs ; ):

1. Smaller, self-contained reviews. For example, you can update `Unauthorized` -> `Forbidden`
in a separate review for clarity and to keep this one smaller.
2. When updating endpoints code, always check and update appropriate documentation. For example
`Unauthorized` -> `Forbidden` must be reflected in reservation and persistent volumes docs.
3. Spooky places like blobs that are hard to read should ideally be prepended with a TODO
saying you plan to refactor it. If it does not look nice and slick to you, chances are the
next guy will feel the same. Let's show we care about that and are not leaving broken windows.
4. Changes touching libprocess are vulnerable to races, especially in tests, where the libprocess
context may be shared among multiple masters and agents. Please extend the testing (and mention
this in the "Testing Done" section) with different platforms, and goodies like `--gtest_shuffle`
and `--gtest-repeat`.


src/authentication/http/basic_authenticator_factory.cpp (line 40)
<https://reviews.apache.org/r/39043/#comment173751>

    #include <stout/foreach.hpp>?



src/master/flags.cpp (lines 447 - 449)
<https://reviews.apache.org/r/39043/#comment173769>

    The variable name implies there can be many authenticators, while the comment says it
may just be one. What's right? I assume the former, because you do a split on comma later
in the patch. Mind mentioning that?



src/master/http.cpp (lines 1671 - 1672)
<https://reviews.apache.org/r/39043/#comment173770>

    I like how the scope is narrowed now and we do not need to carry around the unused secret!



src/master/master.hpp (lines 1179 - 1182)
<https://reviews.apache.org/r/39043/#comment173764>

    Let's remove this in a patch chained to this review (and fix MESOS-4149 as a consequence)
in order not to forget.



src/master/master.cpp (lines 500 - 508)
<https://reviews.apache.org/r/39043/#comment173778>

    Let's add a TODO to refactor lines 500-556 into a factory so people see we consider such
blobs a tech debt.



src/tests/mesos.cpp (lines 435 - 438)
<https://reviews.apache.org/r/39043/#comment173768>

    Since `setAuthenticator` is called in `Master::initialize()` won't it be a problem if
we have multiple masters in a test case? Is `unsetAuthenticator()` idempotent? In tests multiple
masters (and agents) share the same libprocess context (which is not true in production scenarios),
I wonder whether this can be a problem.



src/tests/mesos.cpp (line 438)
<https://reviews.apache.org/r/39043/#comment173765>

    4 spaces, please.



src/tests/persistent_volume_endpoints_tests.cpp (lines 900 - 902)
<https://reviews.apache.org/r/39043/#comment173743>

    I suggest to make this change in a separate patch, but this fits one line now.


- Alexander Rukletsov


On Jan. 7, 2016, 3:25 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39043/
> -----------------------------------------------------------
> 
> (Updated Jan. 7, 2016, 3:25 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Isabel Jimenez, Jan Schlicht, and Till
Toenshoff.
> 
> 
> Bugs: MESOS-3756
>     https://issues.apache.org/jira/browse/MESOS-3756
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> 1. Adds a flag to load an HTTP Authenticator module from the flags.
> 2. If provided, uses the credentials file to initialize the default HTTP Authenticator.
> 3. Updates the existing endpoints which implement their own basic HTTP authenticator
with the libprocess one.
> 4. Updates one test which expected the wrong results since now credentials are checked
before the body of the request.
> 
> 
> Diffs
> -----
> 
>   include/mesos/authentication/http/basic_authenticator_factory.hpp PRE-CREATION 
>   src/authentication/http/basic_authenticator_factory.cpp PRE-CREATION 
>   src/master/constants.hpp f1624e1ed5091f08f57d4382b344cab8b05ba840 
>   src/master/constants.cpp 589f2ee55a24d7e8b9352a4c8f94a3785fd1bef4 
>   src/master/flags.hpp 9af6c68eef6bcf39d5776809fab6c66dc95da6b2 
>   src/master/flags.cpp f864419a6276356c97a0a4fe29e5cfce6c4660c4 
>   src/master/http.cpp d7afa2af989eebfc9039b3681f087ce570f601d5 
>   src/master/master.hpp f764915ff8bbf74b99a617e3c4735d38fc13e5f0 
>   src/master/master.cpp 40ce3e17fca88da689128bcf5d35fdddc396c011 
>   src/master/quota_handler.cpp 93960f3728f9e68c085e6009fe975542d29d2551 
>   src/tests/master_quota_tests.cpp bc8a117982b994279e0df7639b21abaeb308b76d 
>   src/tests/mesos.cpp 3867ed2fcb197f98f0a19ca37ed81392db27e8a0 
> 
> Diff: https://reviews.apache.org/r/39043/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


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