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 50223: Separate AuthN for readonly and readwrite endpoints in Mesos.
Date Fri, 22 Jul 2016 02:38:55 GMT

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



Looking good. Thanks for the patch! Mostly just nits and some suggested API changes (and missing
errors) for the helpers.
If you can fix up most of these tonight, I can fix the remainng nits before committing. Thanks
again!


src/local/local.cpp (lines 228 - 229)
<https://reviews.apache.org/r/50223/#comment208905>

    We prefer to wrap after `=` if possible. Looks like `new Registrar(flags, state, master::READONLY_HTTP_AUTHENTICATION_REALM);`
would fit on one 80char line, even after indenting.
    FYI, we indent 2 spaces after wrapping on an `=`, but 4 spaces if wrapping parameters.



src/master/constants.hpp (line 127)
<https://reviews.apache.org/r/50223/#comment208926>

    I'd prefer to keep these terms hyphenated in comments, log messages, and other English
sentences.
    Variable names and string values can be compound words like "mesos-master-readonly".



src/master/flags.cpp (lines 225 - 227)
<https://reviews.apache.org/r/50223/#comment208930>

    Technically, we're already deprecating `authenticate_http` by setting `flags::DeprecatedName("authenticate_http")`,
so let's update this comment to something like "Remove deprecated `--authenticate_http` flag
name after the deprecation cycle which started with Mesos 1.0."



src/master/flags.cpp (lines 231 - 232)
<https://reviews.apache.org/r/50223/#comment208942>

    Let's put back in the clause about "endpoints supporting authentication" (or call them
"authenticatable endpoints"), since not all endpoints support authentication (e.g. /help,
/redirect, /health)
    Ditto for http_readonly



src/master/flags.cpp (lines 231 - 233)
<https://reviews.apache.org/r/50223/#comment208952>

    Please wrap these in a less jagged way.



src/master/master.cpp (line 380)
<https://reviews.apache.org/r/50223/#comment208945>

    Rather than EXIT() inside this function, how about having it return a `Try<Nothing>`
and have those EXIT lines return `Error("<exit error message>")` instead, so that the
callers can handle the error during registration with an EXIT or whatever they find appropriate.



src/master/master.cpp (line 386)
<https://reviews.apache.org/r/50223/#comment208947>

    This now applies to `http_authenticators` and `http_framework_authenticators`



src/master/master.cpp (line 393)
<https://reviews.apache.org/r/50223/#comment208948>

    ... for realm 'foo'



src/master/master.cpp (lines 399 - 400)
<https://reviews.apache.org/r/50223/#comment208946>

    Not yours, but let's wrap the `<< "' not"` onto the next line so it can be merged
into the next string.
    This may become a non-issue once you change this to an Error("message").



src/slave/constants.hpp (line 130)
<https://reviews.apache.org/r/50223/#comment208949>

    hyphenate in comment sentences



src/slave/flags.cpp (line 790)
<https://reviews.apache.org/r/50223/#comment208950>

    No deprecated flag here, because we never shipped an agent `--authenticate_http` flag,
right?



src/slave/flags.cpp (lines 791 - 793)
<https://reviews.apache.org/r/50223/#comment208951>

    Wrapping is a bit off on these; please re-align so it's less "jagged"



src/slave/slave.cpp (line 349)
<https://reviews.apache.org/r/50223/#comment208956>

    So, now you parse the credentials file even if http_authentication isn't enabled anywhere?
Or even if they're using a custom authenticator that doesn't use `--credentials`?
    Seems like a behavior change. Can you justify it?



src/slave/slave.cpp 
<https://reviews.apache.org/r/50223/#comment208955>

    This error is gone?



src/slave/slave.cpp (line 367)
<https://reviews.apache.org/r/50223/#comment208943>

    Seems redundant to split the string at every callsite. Why not pass the full, unsplit
string into `registerHttpAuthenticator` and split it in there?



src/slave/slave.cpp (lines 370 - 374)
<https://reviews.apache.org/r/50223/#comment208954>

    This error is gone now?



src/slave/slave.cpp 
<https://reviews.apache.org/r/50223/#comment208953>

    This logic seems to be missing from your new patch.


- Adam B


On July 21, 2016, 5:25 p.m., Zhitao Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50223/
> -----------------------------------------------------------
> 
> (Updated July 21, 2016, 5:25 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, Greg Mann, and Till Toenshoff.
> 
> 
> Bugs: MESOS-5851
>     https://issues.apache.org/jira/browse/MESOS-5851
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Changes included:
> - separate flags for readonly and readwrite endpoints;
> - helper function for registering http authenticator;
> - fixing existing tests.
> 
> Next step:
> - docs fix.
> - refactor common helper.
> 
> 
> Diffs
> -----
> 
>   src/local/local.cpp a543aef117fea62660d55435be4d66d30f8ee860 
>   src/master/constants.hpp 410c388c8f8ad98777c6587fc0b06807639e782a 
>   src/master/flags.hpp a5dd11b624d19a9ea3508031ded4c0199098afd1 
>   src/master/flags.cpp ca3e80bf9467328892be89718e5e0a1a05264ab8 
>   src/master/main.cpp 9775b8a1e5fe51670789805557339bd0737a02b7 
>   src/master/master.hpp 6decff6f4b9c3434de030fd5c06df4c683a7abad 
>   src/master/master.cpp 370fd8712062dc75bb81824cb99ccc7920acbf78 
>   src/slave/constants.hpp 10319396a6694e17137876b95ac6866c3d2ebcbd 
>   src/slave/flags.hpp e798dbf2554a85310d71697d873bca4445a6161a 
>   src/slave/flags.cpp 166a6516362a23bc5012aaa2dd45edfc6446de48 
>   src/slave/main.cpp 4624392d30cf391015dcd63f447fe2414a47a16a 
>   src/slave/slave.hpp ffe4220c9289419ab1b1a2c1f499b6eac3c01e4b 
>   src/slave/slave.cpp 3e7131170e1f9bf682fb0c603d2ca39f514d87d9 
>   src/tests/cluster.hpp 55dbaaef6c703676859e39e50bb1c31b942d0929 
>   src/tests/cluster.cpp e1be275fccf130181ed18fd1c5a1b7b18979d7aa 
>   src/tests/dynamic_weights_tests.cpp 6aa0102d3821c1b9d09672c706d6d8850f3729c3 
>   src/tests/logging_tests.cpp 8712d332de50ee70584e6cb8c730cc243f4ba504 
>   src/tests/main.cpp 1425a04c6f6ba9e512b44f083bdd66e3140925cf 
>   src/tests/master_quota_tests.cpp 639f4c4146c61c0713e2945fea4fd6ffe1f3e726 
>   src/tests/master_tests.cpp 252b8f9f2740256aaf54f24efe961d49fce53932 
>   src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
>   src/tests/mesos.cpp d073d79c5797ecb021f0294ab6586a000f3ca600 
>   src/tests/metrics_tests.cpp e470e75e2457b01d24b50fd4ddefffb7553bd485 
>   src/tests/persistent_volume_endpoints_tests.cpp fdd10a76dbfaf8bcae021b9d8b976f43748117fe

>   src/tests/reservation_endpoints_tests.cpp 48c002d1dc371c285b9421ef5a2c57250d270fa8

>   src/tests/slave_tests.cpp 60f9e1644efaeba893f4ff38b6d5a07087d1a355 
> 
> Diff: https://reviews.apache.org/r/50223/diff/
> 
> 
> Testing
> -------
> 
> `make check` on Mac OS.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>


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