----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50223/#review143110 -----------------------------------------------------------
src/master/main.cpp (lines 278 - 280) <https://reviews.apache.org/r/50223/#comment208829> I think this case is slightly ambiguous, but in general we prefer the style: ``` process::initialize( "master", READWRITE_HTTP_AUTHENTICATION_REALM, READONLY_HTTP_AUTHENTICATION_REALM); ``` See "Indentation" in http://mesos.apache.org/documentation/latest/c++-style-guide/ src/master/master.cpp (line 394) <https://reviews.apache.org/r/50223/#comment208833> newline after this? src/slave/main.cpp (lines 259 - 261) <https://reviews.apache.org/r/50223/#comment208860> See other comment about indentation. src/slave/slave.cpp (line 764) <https://reviews.apache.org/r/50223/#comment208834> From the description in this RR it sound like you may have plans along these lines already: perhaps we could factor the master & agent's `registerHttpAuthenticator` out into a common function that they both use? src/tests/main.cpp (lines 96 - 98) <https://reviews.apache.org/r/50223/#comment208835> See related comment above regarding indentation. src/tests/master_quota_tests.cpp (lines 1052 - 1053) <https://reviews.apache.org/r/50223/#comment208837> Perhaps we can get away with just setting `masterFlags.authenticate_http_readwrite = false`, since this test is only hitting the `/quota` endpoint? Here and below. src/tests/master_tests.cpp (line 4580) <https://reviews.apache.org/r/50223/#comment208838> s/are/is/ src/tests/master_tests.cpp (line 4581) <https://reviews.apache.org/r/50223/#comment208865> We have other tests at the libprocess level that test successful/failed requests when authentication is enabled/disabled. What I think would be useful in this test is to check that when `authenticate_http_readonly == true`, one of the master's readonly endpoints is authenticated as expected, while one of its readwrite endpoints is unauthenticated as expected. Likewise with `authenticate_http_readwrite`, and likewise with the `SlaveTest` below. What do you think? src/tests/master_tests.cpp (line 4590) <https://reviews.apache.org/r/50223/#comment208839> s/frameworks/`/frameworks`/ and below for other endpoints src/tests/master_tests.cpp (line 4592) <https://reviews.apache.org/r/50223/#comment208864> Since you state in your comment above that requests are made without authentication, I think you can remove this comment and the identical ones below. src/tests/master_tests.cpp (line 4642) <https://reviews.apache.org/r/50223/#comment208863> s/following/The following/ - Greg Mann On July 21, 2016, 4:33 a.m., Zhitao Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50223/ > ----------------------------------------------------------- > > (Updated July 21, 2016, 4:33 a.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 > >
