> On July 22, 2016, 2:38 a.m., Adam B wrote: > > src/master/master.cpp, line 386 > > <https://reviews.apache.org/r/50223/diff/4/?file=1448832#file1448832line386> > > > > This now applies to `http_authenticators` and > > `http_framework_authenticators`
In the workgroup today, we decided to drop `authenticate_http_framework` and `http_framework_authenticators` flags as well as `DEFAULT_HTTP_FRAMEWORK_AUTHENTICATION_REALM` realm, and roll them into the the `READWRITE` realm. cc @vinodkone. > On July 22, 2016, 2:38 a.m., Adam B wrote: > > src/slave/flags.cpp, line 790 > > <https://reviews.apache.org/r/50223/diff/4/?file=1448835#file1448835line790> > > > > No deprecated flag here, because we never shipped an agent > > `--authenticate_http` flag, right? Yes. > On July 22, 2016, 2:38 a.m., Adam B wrote: > > src/slave/slave.cpp, lines 373-376 > > <https://reviews.apache.org/r/50223/diff/4/?file=1448838#file1448838line373> > > > > 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? The first reason is to align the behavior with master side. Also, consider the following scenario: 1. operator put an invalid credentials file, and passed its path to `--crendentials`, but keep any authentication flags off initially; 2. after a while, operator turned authetnication flags on. IMO, the issue should surface at the first step because that's where issue is actually introduced. The old behavior would only surface at the second step instead, which is slightly confusing to me. > On July 22, 2016, 2:38 a.m., Adam B wrote: > > src/slave/slave.cpp, lines 410-415 > > <https://reviews.apache.org/r/50223/diff/4/?file=1448838#file1448838line410> > > > > This error is gone? Yes, because I refactored the code to be aligned with master side. This exit message was inconsistent between master and agent before my change. I actually think that a valid but unused credential file might be useful: operator can gradually roll it out across the cluster and later turn on authentication. Maybe we can warn instead of exit here? > On July 22, 2016, 2:38 a.m., Adam B wrote: > > src/slave/slave.cpp, line 430 > > <https://reviews.apache.org/r/50223/diff/4/?file=1448838#file1448838line430> > > > > Seems redundant to split the string at every callsite. Why not pass the > > full, unsplit string into `registerHttpAuthenticator` and split it in there? I'm also refactoring this helper function shared by master and agent. I personally perfer a vector<string> than comma separated string because the former is more type safe. > On July 22, 2016, 2:38 a.m., Adam B wrote: > > src/slave/slave.cpp, lines 433-437 > > <https://reviews.apache.org/r/50223/diff/4/?file=1448838#file1448838line433> > > > > This error is gone now? I don't see a possible code path to actually trigger the error here. I added a `CHECK(httpAuthenticator.isSome())` in my future helper, but in all above code path error is always checked and returned. > On July 22, 2016, 2:38 a.m., Adam B wrote: > > src/slave/slave.cpp, lines 443-451 > > <https://reviews.apache.org/r/50223/diff/4/?file=1448838#file1448838line443> > > > > This logic seems to be missing from your new patch. This is because I aligned this logic with master side, which is not as strict on flag checking as agent (see my other comment above). Do you think we should stricten both, possibly at the risk of surprising crashes during upgrade? - Zhitao ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50223/#review143153 ----------------------------------------------------------- On July 22, 2016, 12:25 a.m., Zhitao Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50223/ > ----------------------------------------------------------- > > (Updated July 22, 2016, 12:25 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 > >
