Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review78222 --- Ship it! Ship It! - Vinod Kone On March 28, 2015, 2:25 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 28, 2015, 2:25 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/master/master.hpp 3c957ab src/master/master.cpp dccd7c6 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review78123 --- Fix looks good to me. Buildbot, please take another look. ;) - Adam B On March 25, 2015, 4:35 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 25, 2015, 4:35 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/master/master.hpp 3c957ab src/master/master.cpp dccd7c6 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On March 27, 2015, 10:46 p.m., Adam B wrote: Fix looks good to me. Buildbot, please take another look. ;) Can you or @till let me know what the race and temporal issue here was? And how this fixes it? - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review78123 --- On March 25, 2015, 11:35 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 25, 2015, 11:35 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/master/master.hpp 3c957ab src/master/master.cpp dccd7c6 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On March 27, 2015, 10:46 p.m., Adam B wrote: Fix looks good to me. Buildbot, please take another look. ;) Vinod Kone wrote: Can you or @till let me know what the race and temporal issue here was? And how this fixes it? There was one major problem in these lines: ``` // Start authentication. const FutureOptionstring future = authenticator.get()-authenticate(from) .onAny(defer(self(), Self::_authenticate, pid, lambda::_1)); ``` The returned future was destructed before the local reference (`future`) would even receive it. On the left side, we have a const ref. On the right side, we have a temporal (returned by `authenticate`). This in itself is fine as the lifetime of the temporary will be extended by the lifetime of the reference. Trouble is that the expression `onAny` then gets called on a reference to a temporary and generates a reference to that on return. The returned reference will not extend the lifetime of the future. Now havoc breaks lose as we have a dangling reference. The problem is not showing on all systems as they OS may not immediately reuse the destroyed objects' memory. For the possible race - that actually was not really a race - my update-comment was misleading. It was the fact that I realized that an onAny/... assignment could immediately trigger a callback invocation. Our defer certainly delays the callback but I felt that it would look much cleaner if I made sure that the initializing of authenticating happened before assigning the callback. - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review78123 --- On March 25, 2015, 11:35 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 25, 2015, 11:35 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/master/master.hpp 3c957ab src/master/master.cpp dccd7c6 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review78149 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On March 28, 2015, 2:25 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 28, 2015, 2:25 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/master/master.hpp 3c957ab src/master/master.cpp dccd7c6 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 28, 2015, 2:25 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Changes --- No changes, hoping to trigger review-bot. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs (updated) - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/master/master.hpp 3c957ab src/master/master.cpp dccd7c6 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On March 27, 2015, 10:46 p.m., Adam B wrote: Fix looks good to me. Buildbot, please take another look. ;) Vinod Kone wrote: Can you or @till let me know what the race and temporal issue here was? And how this fixes it? Till Toenshoff wrote: There was one major problem in these lines: ``` // Start authentication. const FutureOptionstring future = authenticator.get()-authenticate(from) .onAny(defer(self(), Self::_authenticate, pid, lambda::_1)); ``` The returned future was destructed before the local reference (`future`) would even receive it. On the left side, we have a const ref. On the right side, we have a temporal (returned by `authenticate`). This in itself is fine as the lifetime of the temporary will be extended by the lifetime of the reference. Trouble is that the expression `onAny` then gets called on a reference to a temporary and generates a reference to that on return. The returned reference will not extend the lifetime of the future. Now havoc breaks lose as we have a dangling reference. The problem is not showing on all systems as they OS may not immediately reuse the destroyed objects' memory. For the possible race - that actually was not really a race - my update-comment was misleading. It was the fact that I realized that an onAny/... assignment could immediately trigger a callback invocation. Our defer certainly delays the callback but I felt that it would look much cleaner if I made sure that the initializing of authenticating happened before assigning the callback. Let me correct myself here; onAny gets called on a temporary, generates a reference and that is then referenced by `future`. - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review78123 --- On March 25, 2015, 11:35 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 25, 2015, 11:35 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/master/master.hpp 3c957ab src/master/master.cpp dccd7c6 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 25, 2015, 11:35 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Changes --- Fixed possible race and reference to temporal issue (the latter triggered an ASF builtbot make check failure on AuthenticationTest.SchedulerFailover). Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs (updated) - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/master/master.hpp 3c957ab src/master/master.cpp dccd7c6 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On March 11, 2015, 9:42 p.m., Vinod Kone wrote: src/master/master.cpp, lines 3858-3861 https://reviews.apache.org/r/27760/diff/23/?file=890575#file890575line3858 Send a FrameworkError message (instead of AuthenticationError) here to avoid retries? Till Toenshoff wrote: The `AuthenticationErrorMessage` is sent to the AuthenticateeProcess, not the process to be authenticated (slave / framework). The behaviour on this specific event (failed to init the authenticator) has not changed. I believe we commented on this before and agreed to come up with a cleanup on authentication result handling later, in a follow-up-patch. Do I remember correctly here? Adam B wrote: Let's create a separate JIRA so that authenticatees (or the owning framework/slave) can know from the authentication response (error/failed) whether to quit or retry. https://issues.apache.org/jira/browse/MESOS-2482. Marking this as fixed for now. - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review76090 --- On March 20, 2015, 10:21 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 20, 2015, 10:21 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/master/master.hpp 3c957ab src/master/master.cpp dccd7c6 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On March 16, 2015, 9:41 p.m., Vinod Kone wrote: src/authentication/cram_md5/authenticator.cpp, lines 552-554 https://reviews.apache.org/r/27760/diff/24/?file=891848#file891848line552 Why is this an initialization error? The previous semantics are that if credentials were not provided, authentication was simply refused. Now because of this change, we are sending an AuthenticationErrorMessage in Master::authenticate(). Yes, thanks for pointing this out. On March 16, 2015, 9:41 p.m., Vinod Kone wrote: src/master/master.cpp, lines 3844-3845 https://reviews.apache.org/r/27760/diff/24/?file=891851#file891851line3844 see my comments in CramMD5Authenticator::initialize(). No credentials didn't lead to AuthenticationErrorMessage before but rather refusal of authentication. I am now adding a warning log into the authenticator to make sure the user gets a hint on that; ``` LOG(WARNING) No credentials provided, authentication requests will be refused.; ``` - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review76642 --- On March 12, 2015, 12:32 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 12, 2015, 12:32 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/master/master.hpp 3c957ab src/master/master.cpp dccd7c6 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 20, 2015, 10:21 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Changes --- Adressed Vinods comments. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs (updated) - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/master/master.hpp 3c957ab src/master/master.cpp dccd7c6 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review76642 --- src/authentication/cram_md5/authenticator.cpp https://reviews.apache.org/r/27760/#comment124231 log the pid. src/authentication/cram_md5/authenticator.cpp https://reviews.apache.org/r/27760/#comment124232 No need to include the 'pid' in the failure because the caller already knows about the pid. src/authentication/cram_md5/authenticator.cpp https://reviews.apache.org/r/27760/#comment124230 log the pid. src/authentication/cram_md5/authenticator.cpp https://reviews.apache.org/r/27760/#comment124227 indentation? src/authentication/cram_md5/authenticator.cpp https://reviews.apache.org/r/27760/#comment124238 Why is this an initialization error? The previous semantics are that if credentials were not provided, authentication was simply refused. Now because of this change, we are sending an AuthenticationErrorMessage in Master::authenticate(). src/master/master.cpp https://reviews.apache.org/r/27760/#comment124240 see my comments in CramMD5Authenticator::initialize(). No credentials didn't lead to AuthenticationErrorMessage before but rather refusal of authentication. - Vinod Kone On March 12, 2015, 12:32 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 12, 2015, 12:32 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/master/master.hpp 3c957ab src/master/master.cpp dccd7c6 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review76090 --- src/authentication/cram_md5/authenticator.hpp https://reviews.apache.org/r/27760/#comment123587 i wish this move was done in its own review (w/o functional changes), so that we can commit it right away. src/authentication/cram_md5/authenticator.cpp https://reviews.apache.org/r/27760/#comment123494 looks like AuthenticatorSessionProcess already has an onDiscard handler that transitions the innermost future to FAILED. Do we still need the onDiscard handler here? src/authentication/cram_md5/authenticator.cpp https://reviews.apache.org/r/27760/#comment123495 // The 'error' is set atmost once per os process. src/authentication/cram_md5/auxprop.hpp https://reviews.apache.org/r/27760/#comment123589 lets do this lock protection in its own dependent review. src/master/master.hpp https://reviews.apache.org/r/27760/#comment123497 s/authenticator/authenticator./ src/master/master.hpp https://reviews.apache.org/r/27760/#comment123498 Can you comment on what the outer and inner future signifies? src/master/master.cpp https://reviews.apache.org/r/27760/#comment123499 s/Could not/Failed to/ src/master/master.cpp https://reviews.apache.org/r/27760/#comment123500 s/Cannot/Failed to/ src/master/master.cpp https://reviews.apache.org/r/27760/#comment123501 Send a FrameworkError message (instead of AuthenticationError) here to avoid retries? src/master/master.cpp https://reviews.apache.org/r/27760/#comment123505 s/authenticator/authenticator session/ src/master/master.cpp https://reviews.apache.org/r/27760/#comment123502 authenticated successfully is confusing. do you mean completed authentication process? src/master/master.cpp https://reviews.apache.org/r/27760/#comment123585 Per our offline discussion, I think we can get rid of this promise altogether now. please send a dependent review for that. src/master/master.cpp https://reviews.apache.org/r/27760/#comment123503 Not yours. but can you s/happen/complete/ - Vinod Kone On March 10, 2015, 7:30 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 10, 2015, 7:30 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am 3059818 src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/authentication/cram_md5/auxprop.cpp cf503a2 src/master/master.hpp 3c957ab src/master/master.cpp dccd7c6 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 12, 2015, 12:32 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Changes --- Split review into dependency chain and addressed a couple of comments. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs (updated) - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/master/master.hpp 3c957ab src/master/master.cpp dccd7c6 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On March 11, 2015, 9:42 p.m., Vinod Kone wrote: src/authentication/cram_md5/authenticator.hpp, lines 79-82 https://reviews.apache.org/r/27760/diff/23/?file=890570#file890570line79 i wish this move was done in its own review (w/o functional changes), so that we can commit it right away. Fixed that, now we got https://reviews.apache.org/r/31961/ On March 11, 2015, 9:42 p.m., Vinod Kone wrote: src/authentication/cram_md5/auxprop.hpp, line 54 https://reviews.apache.org/r/27760/diff/23/?file=890572#file890572line54 lets do this lock protection in its own dependent review. Fixed that, now we got https://reviews.apache.org/r/31960/ On March 11, 2015, 9:42 p.m., Vinod Kone wrote: src/master/master.hpp, lines 668-670 https://reviews.apache.org/r/27760/diff/23/?file=890574#file890574line668 Can you comment on what the outer and inner future signifies? We got rid of them :) On March 11, 2015, 9:42 p.m., Vinod Kone wrote: src/master/master.cpp, lines 3888-3889 https://reviews.apache.org/r/27760/diff/23/?file=890575#file890575line3888 Per our offline discussion, I think we can get rid of this promise altogether now. please send a dependent review for that. Fixed that, we now got https://reviews.apache.org/r/31957/ On March 11, 2015, 9:42 p.m., Vinod Kone wrote: src/master/master.cpp, line 3887 https://reviews.apache.org/r/27760/diff/23/?file=890575#file890575line3887 authenticated successfully is confusing. do you mean completed authentication process? Fixed by removing out promise/future alltogether. On March 11, 2015, 9:42 p.m., Vinod Kone wrote: src/authentication/cram_md5/authenticator.cpp, lines 419-421 https://reviews.apache.org/r/27760/diff/23/?file=890571#file890571line419 looks like AuthenticatorSessionProcess already has an onDiscard handler that transitions the innermost future to FAILED. Do we still need the onDiscard handler here? Yeah, I noticed that as well, now that I was re-re-refactoring things :) -- we got confused here, manifested in the previous update to this RR. - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review76090 --- On March 12, 2015, 12:32 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 12, 2015, 12:32 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/master/master.hpp 3c957ab src/master/master.cpp dccd7c6 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On March 11, 2015, 9:42 p.m., Vinod Kone wrote: src/master/master.cpp, lines 3858-3861 https://reviews.apache.org/r/27760/diff/23/?file=890575#file890575line3858 Send a FrameworkError message (instead of AuthenticationError) here to avoid retries? The `AuthenticationErrorMessage` is sent to the AuthenticateeProcess, not the process to be authenticated (slave / framework). The behaviour on this specific event (failed to init the authenticator) has not changed. I believe we commented on this before and agreed to come up with a cleanup on authentication result handling later, in a follow-up-patch. Do I remember correctly here? - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review76090 --- On March 12, 2015, 12:32 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 12, 2015, 12:32 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/master/master.hpp 3c957ab src/master/master.cpp dccd7c6 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On March 11, 2015, 2:42 p.m., Vinod Kone wrote: src/master/master.cpp, lines 3858-3861 https://reviews.apache.org/r/27760/diff/23/?file=890575#file890575line3858 Send a FrameworkError message (instead of AuthenticationError) here to avoid retries? Till Toenshoff wrote: The `AuthenticationErrorMessage` is sent to the AuthenticateeProcess, not the process to be authenticated (slave / framework). The behaviour on this specific event (failed to init the authenticator) has not changed. I believe we commented on this before and agreed to come up with a cleanup on authentication result handling later, in a follow-up-patch. Do I remember correctly here? Let's create a separate JIRA so that authenticatees (or the owning framework/slave) can know from the authentication response (error/failed) whether to quit or retry. - Adam --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review76090 --- On March 11, 2015, 5:32 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 11, 2015, 5:32 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/master/master.hpp 3c957ab src/master/master.cpp dccd7c6 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 10, 2015, 7:30 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Changes --- Added onDiscard handler into the AuthenticatorSession as onAny wont get triggered by a discard on a future. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs (updated) - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am 3059818 src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/authentication/cram_md5/auxprop.cpp cf503a2 src/master/master.hpp 3c957ab src/master/master.cpp dccd7c6 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review75995 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On March 10, 2015, 7:30 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 10, 2015, 7:30 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am 3059818 src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/authentication/cram_md5/auxprop.cpp cf503a2 src/master/master.hpp 3c957ab src/master/master.cpp dccd7c6 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On Feb. 20, 2015, 2:19 p.m., Vinod Kone wrote:
src/master/master.cpp, lines 3897-3898
https://reviews.apache.org/r/27760/diff/17/?file=869282#file869282line3897
Why do we need this 'onAny'? Why can't 'authenticated' be set in
__authenticate()?
Till Toenshoff wrote:
`__authenticated` is needed to capture (at least) a discard on the outer
future. I have rearranged the code of both continuations to be more explicit
about that.
Vinod Kone wrote:
A Future::discard() *does not* result in a future being DISCARDED (and
onAny callbacks being called). You want an onDiscard() handler (instead of
onAny) to capture that case.
A Future::discard() does not result in a future being DISCARDED
That sounds strange. What does it do then? Is an onDiscard() handler going to
be sufficient here?
On Feb. 20, 2015, 2:19 p.m., Vinod Kone wrote:
src/master/master.cpp, lines 3952-3954
https://reviews.apache.org/r/27760/diff/17/?file=869282#file869282line3952
Can you rephrase these log messages to something meaninguful when one
looks at master log?
Till Toenshoff wrote:
How about this?
```
if (authenticate.isDiscarded()) {
LOG(WARNING) Authentication for pid
has pending cancel request;
} else if (authenticate.discard()) {
LOG(WARNING) Requested to cancel authentication for pid;
}
```
Adam B wrote:
How about: Tried to cancel authentication session for [pid], but it was
already cancelled and Cancelling authentication session for PID?
Vinod Kone wrote:
How are you cancelling the authentication here?
The intention was that discarding the authenticate future would trigger the
(now onDiscard) handler, which would end the authentication session.
- Adam
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27760/#review73310
---
On March 5, 2015, 11:22 a.m., Till Toenshoff wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27760/
---
(Updated March 5, 2015, 11:22 a.m.)
Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone.
Bugs: MESOS-2050
https://issues.apache.org/jira/browse/MESOS-2050
Repository: mesos
Description
---
The initial design and implementation of the authenticator module interface
caused issues and was not optimal for heavy lifting setup of external
dependencies. By introducing a two fold design, this has been decoupled from
the authentication message processing. The new design also gets us back on
track to the goal of makeing SASL a soft dependency of mesos.
Diffs
-
include/mesos/authentication/authenticator.hpp f66217a
src/Makefile.am d299f07
src/authentication/cram_md5/authenticator.hpp c6f465f
src/authentication/cram_md5/authenticator.cpp PRE-CREATION
src/authentication/cram_md5/auxprop.hpp b894386
src/authentication/cram_md5/auxprop.cpp cf503a2
src/master/master.hpp 3c957ab
src/master/master.cpp 68ca19a
src/tests/cram_md5_authentication_tests.cpp 92a89c5
Diff: https://reviews.apache.org/r/27760/diff/
Testing
---
make check
Thanks,
Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote: src/authentication/cram_md5/authenticator.cpp, line 449 https://reviews.apache.org/r/27760/diff/17/?file=869278#file869278line449 Shouldn't this be protected by once() to avoid 2 different threads loading secrets at the same time? Till Toenshoff wrote: That would render our tests broken (those that reset the credentials). The concurrency is covered by a Lock within that SASL aux plugin code. Till Toenshoff wrote: Let me know if this is ok, please. Vinod Kone wrote: Sorry, I don't think I follow. Can you elaborate on what specific tests fail and why? Till Toenshoff wrote: Our tests all run within the same OS-process. A ONCE-gatekeeper on credential loading within this process would indeed allow this only a single time. The CRAM-MD5 tests do attempt to re/load the credentials; see cram_md5_authentication.cpp #95, #140, ... These tests make sense given that we want to test the behavior of the authenticator/authenticatee when using non matching credentials. Vinod Kone wrote: I don't see them reloading out of band. In other words, before your patch we used to call secrets::load() directly from tests, which I agree has thread safety issues if secrets::load() itself is not protected. But now, with your patch, all the loading happens via Authenticator-initialize(). So I'm trying to understand why the protection in Authenticator initialize is not enough. Sorry, if I'm mising something. I added some comment that should help explaining this rather complicated issue, as we had discussed on IRC. Assuming that this covers the issue, I will now mark it as fixed. - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73310 --- On March 5, 2015, 7:22 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 5, 2015, 7:22 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d299f07 src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/authentication/cram_md5/auxprop.cpp cf503a2 src/master/master.hpp 3c957ab src/master/master.cpp 68ca19a src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 5, 2015, 7:22 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs (updated) - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d299f07 src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/authentication/cram_md5/auxprop.cpp cf503a2 src/master/master.hpp 3c957ab src/master/master.cpp 68ca19a src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review75436 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On March 5, 2015, 7:22 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 5, 2015, 7:22 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d299f07 src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/authentication/cram_md5/auxprop.cpp cf503a2 src/master/master.hpp 3c957ab src/master/master.cpp 68ca19a src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
src/authentication/cram_md5/authenticator.cpp, line 449
https://reviews.apache.org/r/27760/diff/17/?file=869278#file869278line449
Shouldn't this be protected by once() to avoid 2 different threads
loading secrets at the same time?
Till Toenshoff wrote:
That would render our tests broken (those that reset the credentials).
The concurrency is covered by a Lock within that SASL aux plugin code.
Till Toenshoff wrote:
Let me know if this is ok, please.
Vinod Kone wrote:
Sorry, I don't think I follow. Can you elaborate on what specific tests
fail and why?
Till Toenshoff wrote:
Our tests all run within the same OS-process. A ONCE-gatekeeper on
credential loading within this process would indeed allow this only a single
time. The CRAM-MD5 tests do attempt to re/load the credentials; see
cram_md5_authentication.cpp #95, #140, ...
These tests make sense given that we want to test the behavior of the
authenticator/authenticatee when using non matching credentials.
I don't see them reloading out of band. In other words, before your patch we
used to call secrets::load() directly from tests, which I agree has thread
safety issues if secrets::load() itself is not protected. But now, with your
patch, all the loading happens via Authenticator-initialize(). So I'm trying
to understand why the protection in Authenticator initialize is not enough.
Sorry, if I'm mising something.
On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
src/master/master.cpp, line 3842
https://reviews.apache.org/r/27760/diff/17/?file=869282#file869282line3842
I think seeing this log line in master log would not reveal much
information on what's happening.
How about
LOG(ERROR) Received authentication request from pid
but authenticator is not loaded;
AuthenticationErrorMessage is only used by authenticator. How about sending
FrameworkError message instead? That way the framework will atleast abort and
know that it is doing something wrong. With AuthenticationErrorMessage it will
just blindly retry.
On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
src/master/master.cpp, lines 3897-3898
https://reviews.apache.org/r/27760/diff/17/?file=869282#file869282line3897
Why do we need this 'onAny'? Why can't 'authenticated' be set in
__authenticate()?
Till Toenshoff wrote:
`__authenticated` is needed to capture (at least) a discard on the outer
future. I have rearranged the code of both continuations to be more explicit
about that.
A Future::discard() *does not* result in a future being DISCARDED (and onAny
callbacks being called). You want an onDiscard() handler (instead of onAny) to
capture that case.
On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
src/master/master.cpp, lines 3952-3954
https://reviews.apache.org/r/27760/diff/17/?file=869282#file869282line3952
Can you rephrase these log messages to something meaninguful when one
looks at master log?
Till Toenshoff wrote:
How about this?
```
if (authenticate.isDiscarded()) {
LOG(WARNING) Authentication for pid
has pending cancel request;
} else if (authenticate.discard()) {
LOG(WARNING) Requested to cancel authentication for pid;
}
```
Adam B wrote:
How about: Tried to cancel authentication session for [pid], but it was
already cancelled and Cancelling authentication session for PID?
How are you cancelling the authentication here?
- Vinod
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27760/#review73310
---
On March 3, 2015, 2:58 p.m., Till Toenshoff wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27760/
---
(Updated March 3, 2015, 2:58 p.m.)
Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone.
Bugs: MESOS-2050
https://issues.apache.org/jira/browse/MESOS-2050
Repository: mesos
Description
---
The initial design and implementation of the authenticator module interface
caused issues and was not optimal for heavy lifting setup of external
dependencies. By introducing a two fold design, this has been decoupled from
the authentication message processing. The new design also gets us back on
track to the goal of makeing SASL a soft dependency of mesos.
Diffs
-
include/mesos/authentication/authenticator.hpp f66217a
src/Makefile.am d299f07
src/authentication/cram_md5/authenticator.hpp c6f465f
src/authentication/cram_md5/authenticator.cpp PRE-CREATION
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 3, 2015, 2:58 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Changes --- Addressed Vinod's comment on the conditional logging of startup failures. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs (updated) - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d299f07 src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/authentication/cram_md5/auxprop.cpp cf503a2 src/master/master.hpp ce0e0b3 src/master/master.cpp 53c8696 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review75119 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On March 3, 2015, 2:58 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 3, 2015, 2:58 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d299f07 src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/authentication/cram_md5/auxprop.cpp cf503a2 src/master/master.hpp ce0e0b3 src/master/master.cpp 53c8696 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
src/authentication/cram_md5/authenticator.cpp, line 449
https://reviews.apache.org/r/27760/diff/17/?file=869278#file869278line449
Shouldn't this be protected by once() to avoid 2 different threads
loading secrets at the same time?
Till Toenshoff wrote:
That would render our tests broken (those that reset the credentials).
The concurrency is covered by a Lock within that SASL aux plugin code.
Till Toenshoff wrote:
Let me know if this is ok, please.
Vinod Kone wrote:
Sorry, I don't think I follow. Can you elaborate on what specific tests
fail and why?
Our tests all run within the same OS-process. A ONCE-gatekeeper on credential
loading within this process would indeed allow this only a single time. The
CRAM-MD5 tests do attempt to re/load the credentials; see
cram_md5_authentication.cpp #95, #140, ...
These tests make sense given that we want to test the behavior of the
authenticator/authenticatee when using non matching credentials.
On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
src/master/master.cpp, lines 466-467
https://reviews.apache.org/r/27760/diff/17/?file=869282#file869282line466
why this if condition for logging?
Till Toenshoff wrote:
Not sure I understand. Did you see the comment directly above those lines?
```
// A failure to initialize the authenticator does lead to
// unusable authentication but still allows non authenticating
// frameworks and slaves to connect.
```
Adam B wrote:
I think (a few revisions back), the thought was that the default
parameters for a master are authenticate_frameworks/slaves=false
credentials=none authenticator=default(cram), at which point it seems
unnecessary to warn somebody that we didn't load the authenticator. Upon
returning to this decision, I see no reason not to log this message anytime
authentication is disabled, even if it's the default setting. Maybe it would
seem less harsh as an INFO in the default case, but a single WARN message on
master startup could be a gentle nudge to try out authentication.
Till Toenshoff wrote:
Let's try to get this sorted out as well. Any actions needed?
Vinod Kone wrote:
I just meant, that we should probably always log that warning
irrespective of the 'if(credentials.isSome() || (authenticatorNames[0] !=
DEFAULT_AUTHENTICATOR)) ' condition. IIUC, the fact that we are initializing
an authenticator, irrespective of whether 'credentials' is provided is
because there could be authenticators which don't depend on 'credentials'.
So, if there are initialization errors it is better to log them sooner than
later when they do enable 'authenticate_frameworks' or 'authenticate_slaves'.
Alternatively, I would just simplify this as follows:
```
if (intialize.isError()) {
EXIT(1) Failed to initialize authenticator '
authenticatorNames[0]
: initialize.error();
}
```
The default authenticator (CRAM MD5) does rely upon receiving credentials,
hence its initializing fails without credentials. By default, users are not
required to provide credentials, hence mesos would exit by default even though
the user did not even activate authentication -- not a nice user experience.
I think the only possible simplification here is indeed removing the limitation
on `if(credentials.isSome() || (authenticatorNames[0] !=
DEFAULT_AUTHENTICATOR))` and that is what I am going to propose in an update
now :)
- Till
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27760/#review73310
---
On Feb. 27, 2015, 2:28 a.m., Till Toenshoff wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27760/
---
(Updated Feb. 27, 2015, 2:28 a.m.)
Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone.
Bugs: MESOS-2050
https://issues.apache.org/jira/browse/MESOS-2050
Repository: mesos
Description
---
The initial design and implementation of the authenticator module interface
caused issues and was not optimal for heavy lifting setup of external
dependencies. By introducing a two fold design, this has been decoupled from
the authentication message processing. The new design also gets us back on
track to the goal of makeing SASL a soft dependency of mesos.
Diffs
-
include/mesos/authentication/authenticator.hpp f66217a
src/Makefile.am 17d0d7a
src/authentication/cram_md5/authenticator.hpp c6f465f
src/authentication/cram_md5/authenticator.cpp PRE-CREATION
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
src/authentication/cram_md5/authenticator.cpp, line 449
https://reviews.apache.org/r/27760/diff/17/?file=869278#file869278line449
Shouldn't this be protected by once() to avoid 2 different threads
loading secrets at the same time?
Till Toenshoff wrote:
That would render our tests broken (those that reset the credentials).
The concurrency is covered by a Lock within that SASL aux plugin code.
Till Toenshoff wrote:
Let me know if this is ok, please.
Sorry, I don't think I follow. Can you elaborate on what specific tests fail
and why?
On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
src/master/master.cpp, lines 466-467
https://reviews.apache.org/r/27760/diff/17/?file=869282#file869282line466
why this if condition for logging?
Till Toenshoff wrote:
Not sure I understand. Did you see the comment directly above those lines?
```
// A failure to initialize the authenticator does lead to
// unusable authentication but still allows non authenticating
// frameworks and slaves to connect.
```
Adam B wrote:
I think (a few revisions back), the thought was that the default
parameters for a master are authenticate_frameworks/slaves=false
credentials=none authenticator=default(cram), at which point it seems
unnecessary to warn somebody that we didn't load the authenticator. Upon
returning to this decision, I see no reason not to log this message anytime
authentication is disabled, even if it's the default setting. Maybe it would
seem less harsh as an INFO in the default case, but a single WARN message on
master startup could be a gentle nudge to try out authentication.
Till Toenshoff wrote:
Let's try to get this sorted out as well. Any actions needed?
I just meant, that we should probably always log that warning irrespective of
the 'if(credentials.isSome() || (authenticatorNames[0] !=
DEFAULT_AUTHENTICATOR)) ' condition. IIUC, the fact that we are initializing an
authenticator, irrespective of whether 'credentials' is provided is because
there could be authenticators which don't depend on 'credentials'. So, if there
are initialization errors it is better to log them sooner than later when they
do enable 'authenticate_frameworks' or 'authenticate_slaves'.
Alternatively, I would just simplify this as follows:
```
if (intialize.isError()) {
EXIT(1) Failed to initialize authenticator ' authenticatorNames[0]
: initialize.error();
}
```
- Vinod
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27760/#review73310
---
On Feb. 27, 2015, 2:28 a.m., Till Toenshoff wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27760/
---
(Updated Feb. 27, 2015, 2:28 a.m.)
Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone.
Bugs: MESOS-2050
https://issues.apache.org/jira/browse/MESOS-2050
Repository: mesos
Description
---
The initial design and implementation of the authenticator module interface
caused issues and was not optimal for heavy lifting setup of external
dependencies. By introducing a two fold design, this has been decoupled from
the authentication message processing. The new design also gets us back on
track to the goal of makeing SASL a soft dependency of mesos.
Diffs
-
include/mesos/authentication/authenticator.hpp f66217a
src/Makefile.am 17d0d7a
src/authentication/cram_md5/authenticator.hpp c6f465f
src/authentication/cram_md5/authenticator.cpp PRE-CREATION
src/authentication/cram_md5/auxprop.hpp b894386
src/authentication/cram_md5/auxprop.cpp cf503a2
src/master/master.hpp e288cdb
src/master/master.cpp 76e217d
src/tests/cram_md5_authentication_tests.cpp 92a89c5
Diff: https://reviews.apache.org/r/27760/diff/
Testing
---
make check
Thanks,
Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On Feb. 25, 2015, 4:01 p.m., Niklas Nielsen wrote: Hey Till, Can you rebase this? :) Niklas Nielsen wrote: Vinod, think that this is only awaiting responses from you on a few questions/answers above :) Would love to get this in. @vinodkone Ready for you to take another look regarding your open issues. - Adam --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review74158 --- On Feb. 26, 2015, 6:28 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 26, 2015, 6:28 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am 17d0d7a src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/authentication/cram_md5/auxprop.cpp cf503a2 src/master/master.hpp e288cdb src/master/master.cpp 76e217d src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On Feb. 25, 2015, 4:01 p.m., Niklas Nielsen wrote: Hey Till, Can you rebase this? :) Vinod, think that this is only awaiting responses from you on a few questions/answers above :) Would love to get this in. - Niklas --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review74158 --- On Feb. 22, 2015, 2:03 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 22, 2015, 2:03 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp a466f92 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 27, 2015, 2:28 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Changes --- Rebased, added one missing include. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs (updated) - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am 17d0d7a src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/authentication/cram_md5/auxprop.cpp cf503a2 src/master/master.hpp e288cdb src/master/master.cpp 76e217d src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review74439 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 27, 2015, 2:28 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 27, 2015, 2:28 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am 17d0d7a src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/authentication/cram_md5/auxprop.cpp cf503a2 src/master/master.hpp e288cdb src/master/master.cpp 76e217d src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review74158 --- Hey Till, Can you rebase this? :) - Niklas Nielsen On Feb. 22, 2015, 2:03 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 22, 2015, 2:03 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp a466f92 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73465 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 22, 2015, 10:03 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 22, 2015, 10:03 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp a466f92 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 22, 2015, 10:03 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Changes --- Adressed Adams comments. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs (updated) - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp a466f92 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On Feb. 22, 2015, 7:11 a.m., Adam B wrote: src/master/master.cpp, line 453 https://reviews.apache.org/r/27760/diff/17-18/?file=869282#file869282line453 Maybe do a `CHECK_SOME(authenticator)`? The current code-path to this use of authenticator does not permit anything but set value. So I guess you would like to see this for future robustnes / defensive style reasons. - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73446 --- On Feb. 22, 2015, 10:03 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 22, 2015, 10:03 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp a466f92 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote: src/authentication/cram_md5/authenticator.cpp, line 449 https://reviews.apache.org/r/27760/diff/17/?file=869278#file869278line449 Shouldn't this be protected by once() to avoid 2 different threads loading secrets at the same time? Till Toenshoff wrote: That would render our tests broken (those that reset the credentials). The concurrency is covered by a Lock within that SASL aux plugin code. Let me know if this is ok, please. On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote: src/master/master.cpp, lines 466-467 https://reviews.apache.org/r/27760/diff/17/?file=869282#file869282line466 why this if condition for logging? Till Toenshoff wrote: Not sure I understand. Did you see the comment directly above those lines? ``` // A failure to initialize the authenticator does lead to // unusable authentication but still allows non authenticating // frameworks and slaves to connect. ``` Adam B wrote: I think (a few revisions back), the thought was that the default parameters for a master are authenticate_frameworks/slaves=false credentials=none authenticator=default(cram), at which point it seems unnecessary to warn somebody that we didn't load the authenticator. Upon returning to this decision, I see no reason not to log this message anytime authentication is disabled, even if it's the default setting. Maybe it would seem less harsh as an INFO in the default case, but a single WARN message on master startup could be a gentle nudge to try out authentication. Let's try to get this sorted out as well. Any actions needed? - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73310 --- On Feb. 22, 2015, 10:03 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 22, 2015, 10:03 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp a466f92 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73422 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 21, 2015, 10:16 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 21, 2015, 10:16 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp a466f92 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote: src/master/master.cpp, lines 3870-3873 https://reviews.apache.org/r/27760/diff/17/?file=869282#file869282line3870 Did you mix up inner and outer here? Till Toenshoff wrote: I rephrased it, maybe this is better? ``` // The inner future may get satisfied by an empty option, hinting // that the authentication has gotten refused. // The outer future will be ready only if we have entirely // authenticated successfully. ``` Adam B wrote: FTFY: The inner future will be satisfied when the authentication process has completed and returned a result; however, it could return an Option None, indicating that authentication was denied. The outer future will be ready only if/when authentication has succeeded. Why did we go for an Optionstring rather than a Trystring? Seems like Try would better reflect the difference between success/failure than some/none does. That is a good question, Adam. I don't know - that is logic I inherited from the original implementation - it appears to be rooted within changes introduced tbrough MESOS-1383. - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73310 --- On Feb. 19, 2015, 2:02 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 19, 2015, 2:02 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 21, 2015, 10:16 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Changes --- Addressed all comments that were left without open questions. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs (updated) - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp a466f92 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On Jan. 28, 2015, 8:33 p.m., Vinod Kone wrote: src/master/master.cpp, lines 4193-4203 https://reviews.apache.org/r/27760/diff/12/?file=834181#file834181line4193 IIRC, sending an AuthenticationErrorMessage here causes the driver to simply retry the authentication and thus fall into a retry loop? Adam B wrote: Truth. There is a TODO about at least adding a backoff, but maybe we should also attempt registering without authentication? Maybe authentication shouldn't even cause retries under certain scenarios where it will never eventually succeed (bad credentials, no authenticator). Till Toenshoff wrote: Can we make this a TODO at this spot as well and create a JIRA? I'ld also be happy to work on a follow up patch, covering more finegrained authentication result-triggers. Adam B wrote: We should create a JIRA for the authentication backoff TODO, and perhaps another JIRA for retrying without authentication. I would like to point out that the behavior after this patch matches the AuthenticationErrorMessage behavior all the way back to 0.19 or earlier, so we can certainly leave the TODOs alone for this release and just create JIRAs for proper tracking in the future. Vinod Kone wrote: sgtm. please resolve this once you add the TODO. Created MESOS-2379 and referenced this in a TODO. - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review70051 --- On Feb. 19, 2015, 2:02 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 19, 2015, 2:02 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73446 --- LGTM, pending the other open issues. src/master/master.cpp https://reviews.apache.org/r/27760/#comment119782 Maybe do a `CHECK_SOME(authenticator)`? src/master/master.cpp https://reviews.apache.org/r/27760/#comment119783 s/hinting/indicating/, since it's stronger than a hint. s/gotten refused/been refused/ - Adam B On Feb. 21, 2015, 2:16 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 21, 2015, 2:16 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp a466f92 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On Feb. 20, 2015, 2:19 p.m., Vinod Kone wrote:
src/master/master.cpp, lines 466-467
https://reviews.apache.org/r/27760/diff/17/?file=869282#file869282line466
why this if condition for logging?
Till Toenshoff wrote:
Not sure I understand. Did you see the comment directly above those lines?
```
// A failure to initialize the authenticator does lead to
// unusable authentication but still allows non authenticating
// frameworks and slaves to connect.
```
I think (a few revisions back), the thought was that the default parameters for
a master are authenticate_frameworks/slaves=false credentials=none
authenticator=default(cram), at which point it seems unnecessary to warn
somebody that we didn't load the authenticator. Upon returning to this
decision, I see no reason not to log this message anytime authentication is
disabled, even if it's the default setting. Maybe it would seem less harsh as
an INFO in the default case, but a single WARN message on master startup could
be a gentle nudge to try out authentication.
On Feb. 20, 2015, 2:19 p.m., Vinod Kone wrote:
src/master/master.cpp, lines 3870-3873
https://reviews.apache.org/r/27760/diff/17/?file=869282#file869282line3870
Did you mix up inner and outer here?
Till Toenshoff wrote:
I rephrased it, maybe this is better?
```
// The inner future may get satisfied by an empty option, hinting
// that the authentication has gotten refused.
// The outer future will be ready only if we have entirely
// authenticated successfully.
```
FTFY: The inner future will be satisfied when the authentication process has
completed and returned a result; however, it could return an Option None,
indicating that authentication was denied. The outer future will be ready only
if/when authentication has succeeded.
Why did we go for an Optionstring rather than a Trystring? Seems like Try
would better reflect the difference between success/failure than some/none does.
On Feb. 20, 2015, 2:19 p.m., Vinod Kone wrote:
src/master/master.cpp, lines 3952-3954
https://reviews.apache.org/r/27760/diff/17/?file=869282#file869282line3952
Can you rephrase these log messages to something meaninguful when one
looks at master log?
Till Toenshoff wrote:
How about this?
```
if (authenticate.isDiscarded()) {
LOG(WARNING) Authentication for pid
has pending cancel request;
} else if (authenticate.discard()) {
LOG(WARNING) Requested to cancel authentication for pid;
}
```
How about: Tried to cancel authentication session for [pid], but it was
already cancelled and Cancelling authentication session for PID?
- Adam
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27760/#review73310
---
On Feb. 19, 2015, 6:02 a.m., Till Toenshoff wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27760/
---
(Updated Feb. 19, 2015, 6:02 a.m.)
Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone.
Bugs: MESOS-2050
https://issues.apache.org/jira/browse/MESOS-2050
Repository: mesos
Description
---
The initial design and implementation of the authenticator module interface
caused issues and was not optimal for heavy lifting setup of external
dependencies. By introducing a two fold design, this has been decoupled from
the authentication message processing. The new design also gets us back on
track to the goal of makeing SASL a soft dependency of mesos.
Diffs
-
include/mesos/authentication/authenticator.hpp f66217a
src/Makefile.am d372404
src/authentication/cram_md5/authenticator.hpp 7578ea1
src/authentication/cram_md5/authenticator.cpp PRE-CREATION
src/authentication/cram_md5/auxprop.hpp d036b11
src/authentication/cram_md5/auxprop.cpp 5ff9755
src/master/master.hpp 6a39df0
src/master/master.cpp f10a3cf
src/tests/cram_md5_authentication_tests.cpp dd102dc
Diff: https://reviews.apache.org/r/27760/diff/
Testing
---
make check
Thanks,
Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73310 --- src/authentication/cram_md5/authenticator.hpp https://reviews.apache.org/r/27760/#comment119590 Do we need this in the header? Can it be moved to cpp? src/authentication/cram_md5/authenticator.cpp https://reviews.apache.org/r/27760/#comment119591 How about: ``` sessions.put(pid, session); return session-authenticate() .onAny(defer(..)); ``` src/authentication/cram_md5/authenticator.cpp https://reviews.apache.org/r/27760/#comment119592 Can you add a comment on when this is possible? src/authentication/cram_md5/authenticator.cpp https://reviews.apache.org/r/27760/#comment119593 What do you mean by within our Once covered case? Also, just put subsequent on the next line. src/authentication/cram_md5/authenticator.cpp https://reviews.apache.org/r/27760/#comment119595 Do this at the very beginning. src/authentication/cram_md5/authenticator.cpp https://reviews.apache.org/r/27760/#comment119594 Shouldn't this be protected by once() to avoid 2 different threads loading secrets at the same time? src/authentication/cram_md5/auxprop.hpp https://reviews.apache.org/r/27760/#comment119596 s/persistence/properties/ src/authentication/cram_md5/auxprop.hpp https://reviews.apache.org/r/27760/#comment119597 if you protect secrets::load() by Once, as suggested above, do we need this? src/master/master.hpp https://reviews.apache.org/r/27760/#comment119598 can you name this something more descriptive than inner? Also comment this method. src/master/master.hpp https://reviews.apache.org/r/27760/#comment119605 make this OptionAuthenticator* src/master/master.cpp https://reviews.apache.org/r/27760/#comment119604 why this if condition for logging? src/master/master.cpp https://reviews.apache.org/r/27760/#comment119607 I think seeing this log line in master log would not reveal much information on what's happening. How about LOG(ERROR) Received authentication request from pid but authenticator is not loaded; src/master/master.cpp https://reviews.apache.org/r/27760/#comment119620 Did you mix up inner and outer here? src/master/master.cpp https://reviews.apache.org/r/27760/#comment119621 Why do we need this 'onAny'? Why can't 'authenticated' be set in __authenticate()? src/master/master.cpp https://reviews.apache.org/r/27760/#comment119614 Can you rephrase these log messages to something meaninguful when one looks at master log? - Vinod Kone On Feb. 19, 2015, 2:02 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 19, 2015, 2:02 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
Thanks a bunch for your thorrough review Vinod - much appreciated.
On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
src/authentication/cram_md5/authenticator.cpp, line 449
https://reviews.apache.org/r/27760/diff/17/?file=869278#file869278line449
Shouldn't this be protected by once() to avoid 2 different threads
loading secrets at the same time?
That would render our tests broken (those that reset the credentials). The
concurrency is covered by a Lock within that SASL aux plugin code.
On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
src/authentication/cram_md5/auxprop.hpp, line 100
https://reviews.apache.org/r/27760/diff/17/?file=869279#file869279line100
if you protect secrets::load() by Once, as suggested above, do we need
this?
See above
On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
src/authentication/cram_md5/authenticator.hpp, line 254
https://reviews.apache.org/r/27760/diff/17/?file=869277#file869277line254
Do we need this in the header? Can it be moved to cpp?
Good catch, artefact from recent refactoring. Thank you!
On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
src/authentication/cram_md5/authenticator.cpp, lines 54-59
https://reviews.apache.org/r/27760/diff/17/?file=869278#file869278line54
How about:
```
sessions.put(pid, session);
return session-authenticate()
.onAny(defer(..));
```
Nice, yes!
On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
src/authentication/cram_md5/authenticator.cpp, line 436
https://reviews.apache.org/r/27760/diff/17/?file=869278#file869278line436
What do you mean by within our Once covered case?
Also, just put subsequent on the next line.
Rephreased that one to:
```
// The 'error' is set, if at all, only once per (os-) process.
// To allow subsequent calls to return the possibly set Error
// object, we make this a static pointer.
```
On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
src/master/master.hpp, line 655
https://reviews.apache.org/r/27760/diff/17/?file=869281#file869281line655
make this OptionAuthenticator*
Neat, much nicer.
On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
src/authentication/cram_md5/authenticator.cpp, line 70
https://reviews.apache.org/r/27760/diff/17/?file=869278#file869278line70
Can you add a comment on when this is possible?
It shouldn't :) replaced by
```
CHECK(sessions.contains(pid));
```
On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
src/master/master.cpp, lines 3952-3954
https://reviews.apache.org/r/27760/diff/17/?file=869282#file869282line3952
Can you rephrase these log messages to something meaninguful when one
looks at master log?
How about this?
```
if (authenticate.isDiscarded()) {
LOG(WARNING) Authentication for pid
has pending cancel request;
} else if (authenticate.discard()) {
LOG(WARNING) Requested to cancel authentication for pid;
}
```
On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
src/master/master.cpp, lines 466-467
https://reviews.apache.org/r/27760/diff/17/?file=869282#file869282line466
why this if condition for logging?
Not sure I understand. Did you see the comment directly above those lines?
```
// A failure to initialize the authenticator does lead to
// unusable authentication but still allows non authenticating
// frameworks and slaves to connect.
```
On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
src/master/master.cpp, lines 3870-3873
https://reviews.apache.org/r/27760/diff/17/?file=869282#file869282line3870
Did you mix up inner and outer here?
I rephrased it, maybe this is better?
```
// The inner future may get satisfied by an empty option, hinting
// that the authentication has gotten refused.
// The outer future will be ready only if we have entirely
// authenticated successfully.
```
On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
src/master/master.cpp, lines 3897-3898
https://reviews.apache.org/r/27760/diff/17/?file=869282#file869282line3897
Why do we need this 'onAny'? Why can't 'authenticated' be set in
__authenticate()?
`__authenticated` is needed to capture (at least) a discard on the outer
future. I have rearranged the code of both continuations to be more explicit
about that.
- Till
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27760/#review73310
---
On Feb. 19, 2015, 2:02 p.m., Till Toenshoff wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27760/
---
(Updated Feb. 19, 2015, 2:02
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73285 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 19, 2015, 2:02 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 19, 2015, 2:02 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73292 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 19, 2015, 2:02 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 19, 2015, 2:02 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73298 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 19, 2015, 2:02 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 19, 2015, 2:02 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73102 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 18, 2015, 3:57 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 18, 2015, 3:57 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73106 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 18, 2015, 3:57 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 18, 2015, 3:57 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73189 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 19, 2015, 2:02 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 19, 2015, 2:02 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73177 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 19, 2015, 2:02 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 19, 2015, 2:02 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 19, 2015, 11:42 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Changes --- Addressed Adams comments. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs (updated) - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73123 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 18, 2015, 3:57 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 18, 2015, 3:57 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73135 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 19, 2015, 11:42 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 19, 2015, 11:42 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73138 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 19, 2015, 2:02 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 19, 2015, 2:02 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On Feb. 18, 2015, 10:50 a.m., Adam B wrote: src/authentication/cram_md5/authenticator.cpp, line 44 https://reviews.apache.org/r/27760/diff/15/?file=867261#file867261line44 I'm still confused about what this log line is supposed to mean.. that we're inside [CRAMMD5Authenticator]Process and the authentication session is about to start? If that's the case, perhaps CRAMMD5AuthenticatorProcess starting authentication session? (and then CRAM...Process cleaning up after authentication session) Otherwise, I'm very unclear what the Process refers to, and what authentication modifies. Or is Process intended as a verb? Much too overloaded here. hehe, you are right - I meant to say ``` VLOG(1) Starting authentication session; ``` - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review72942 --- On Feb. 19, 2015, 11:42 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 19, 2015, 11:42 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73145 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 19, 2015, 2:02 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 19, 2015, 2:02 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73216 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 19, 2015, 2:02 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 19, 2015, 2:02 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73251 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 19, 2015, 2:02 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 19, 2015, 2:02 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73258 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 19, 2015, 2:02 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 19, 2015, 2:02 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 19, 2015, 2:02 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs (updated) - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73157 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 19, 2015, 2:02 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 19, 2015, 2:02 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73167 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 19, 2015, 2:02 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 19, 2015, 2:02 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73236 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 19, 2015, 2:02 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 19, 2015, 2:02 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review72921 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 18, 2015, 3:57 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 18, 2015, 3:57 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On Feb. 16, 2015, 11:37 p.m., Adam B wrote: src/authentication/cram_md5/authenticator.hpp, line 533 https://reviews.apache.org/r/27760/diff/14/?file=863170#file863170line533 Should this still `process::terminate(process, false)` if the short term fix is now in `~CRAMMD5AuthenticatorSession`? The 'false' injects the 'terminate' at the end of the process' queue, so other in-flight events get handled first (semi-graceful shutdown). Till Toenshoff wrote: Correct, I think it should be a regular terminate at this point now (getting pushed into the front of the queue). From the description of MESOS-1866, I understand the race to apply to destruction of the session, not the entire master/authenticator. The double-future approach might even fix this somehow. I'd have to dig deeper. Unfortunately, MESOS-1866 Race 1 never had a unit test written for it, so there's no way to test that we aren't regressing. Maybe @vinodkone can comment on the previous race and help us determine if we're maintaining/improving the proper behavior. On Feb. 16, 2015, 11:37 p.m., Adam B wrote: src/authentication/cram_md5/authenticator.hpp, line 545 https://reviews.apache.org/r/27760/diff/14/?file=863170#file863170line545 What's the reasoning for this being a `static*`? Why wouldn't a plain old `OptionError` work? Till Toenshoff wrote: That way, if an error occured in the once-covered case, then any additional calls to initialize (e.g. by another instance) would still have access to that former, once-covered error and be able to return it. Okay, makes sense. Worth a comment? - Adam --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review72680 --- On Feb. 17, 2015, 7:57 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 17, 2015, 7:57 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review72932 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 18, 2015, 3:57 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 18, 2015, 3:57 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On Jan. 28, 2015, 12:33 p.m., Vinod Kone wrote: src/master/master.cpp, lines 4193-4203 https://reviews.apache.org/r/27760/diff/12/?file=834181#file834181line4193 IIRC, sending an AuthenticationErrorMessage here causes the driver to simply retry the authentication and thus fall into a retry loop? Adam B wrote: Truth. There is a TODO about at least adding a backoff, but maybe we should also attempt registering without authentication? Maybe authentication shouldn't even cause retries under certain scenarios where it will never eventually succeed (bad credentials, no authenticator). Till Toenshoff wrote: Can we make this a TODO at this spot as well and create a JIRA? I'ld also be happy to work on a follow up patch, covering more finegrained authentication result-triggers. We should create a JIRA for the authentication backoff TODO, and perhaps another JIRA for retrying without authentication. I would like to point out that the behavior after this patch matches the AuthenticationErrorMessage behavior all the way back to 0.19 or earlier, so we can certainly leave the TODOs alone for this release and just create JIRAs for proper tracking in the future. - Adam --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review70051 --- On Feb. 17, 2015, 7:57 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 17, 2015, 7:57 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review72942 --- src/master/master.cpp https://reviews.apache.org/r/27760/#comment119071 Remove trailing ',' src/master/master.cpp https://reviews.apache.org/r/27760/#comment119072 Merge these lines src/authentication/cram_md5/authenticator.cpp https://reviews.apache.org/r/27760/#comment119069 I'm still confused about what this log line is supposed to mean.. that we're inside [CRAMMD5Authenticator]Process and the authentication session is about to start? If that's the case, perhaps CRAMMD5AuthenticatorProcess starting authentication session? (and then CRAM...Process cleaning up after authentication session) Otherwise, I'm very unclear what the Process refers to, and what authentication modifies. Or is Process intended as a verb? Much too overloaded here. src/authentication/cram_md5/authenticator.cpp https://reviews.apache.org/r/27760/#comment119073 Authentication session already active...? - Adam B On Feb. 17, 2015, 7:57 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 17, 2015, 7:57 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review72970 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 18, 2015, 3:57 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 18, 2015, 3:57 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On Jan. 28, 2015, 8:33 p.m., Vinod Kone wrote: src/master/master.cpp, lines 4193-4203 https://reviews.apache.org/r/27760/diff/12/?file=834181#file834181line4193 IIRC, sending an AuthenticationErrorMessage here causes the driver to simply retry the authentication and thus fall into a retry loop? Adam B wrote: Truth. There is a TODO about at least adding a backoff, but maybe we should also attempt registering without authentication? Maybe authentication shouldn't even cause retries under certain scenarios where it will never eventually succeed (bad credentials, no authenticator). Till Toenshoff wrote: Can we make this a TODO at this spot as well and create a JIRA? I'ld also be happy to work on a follow up patch, covering more finegrained authentication result-triggers. Adam B wrote: We should create a JIRA for the authentication backoff TODO, and perhaps another JIRA for retrying without authentication. I would like to point out that the behavior after this patch matches the AuthenticationErrorMessage behavior all the way back to 0.19 or earlier, so we can certainly leave the TODOs alone for this release and just create JIRAs for proper tracking in the future. sgtm. please resolve this once you add the TODO. On Jan. 28, 2015, 8:33 p.m., Vinod Kone wrote: src/master/master.cpp, lines 4173-4190 https://reviews.apache.org/r/27760/diff/12/?file=834181#file834181line4173 Hmm. I think this is a serious enough error that the master should fail during initilization rather than sending an error message here. Do you have use cases in mind that warranted this? Adam B wrote: The default authenticator is CRAM-MD5 rather than none, and that authenticator fails to initialize if no credentials are provided (or if auxprops cannot be loaded). Since the default parameters specify CRAM-MD5 authenticator, no required authentication, and no credentials, we must support this starting successfully, although we do log a warning (Only non-authenticating frameworks and slaves are allowed to connect. Authentication is disabled: error_message). In this case, we must allow non-authenticating frameworks/slaves to register without authentication, but we will return an AuthenticationError if they actually try to authenticate. This is the same behavior as before. For scenarios where the authenticator fails to load, but authentication is explicitly enabled for frameworks/slaves, we do exit the master during initialization Cannot start master with authentication enabled: error_message) Till Toenshoff wrote: Thanks Adam for your explanations. Vinod, please let me know if this part was matching your expectations. yea. that makes sense. can you add comments in the code mentioning the same, for future readers? you can resolve this then. - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review70051 --- On Feb. 18, 2015, 3:57 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 18, 2015, 3:57 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review72995 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 18, 2015, 3:57 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 18, 2015, 3:57 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review72974 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 18, 2015, 3:57 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 18, 2015, 3:57 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73011 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 18, 2015, 3:57 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 18, 2015, 3:57 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73021 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 18, 2015, 3:57 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 18, 2015, 3:57 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73089 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 18, 2015, 3:57 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 18, 2015, 3:57 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73093 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 18, 2015, 3:57 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 18, 2015, 3:57 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73092 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 18, 2015, 3:57 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 18, 2015, 3:57 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73083 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 18, 2015, 3:57 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 18, 2015, 3:57 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review72967 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 18, 2015, 3:57 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 18, 2015, 3:57 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review72963 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 18, 2015, 3:57 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 18, 2015, 3:57 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73047 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 18, 2015, 3:57 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 18, 2015, 3:57 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 18, 2015, 3:57 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Changes --- Addressed all of Adam's recent comments. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs (updated) - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review72912 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 18, 2015, 3:57 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 18, 2015, 3:57 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On Jan. 28, 2015, 8:33 p.m., Vinod Kone wrote: src/master/master.cpp, lines 4173-4190 https://reviews.apache.org/r/27760/diff/12/?file=834181#file834181line4173 Hmm. I think this is a serious enough error that the master should fail during initilization rather than sending an error message here. Do you have use cases in mind that warranted this? Adam B wrote: The default authenticator is CRAM-MD5 rather than none, and that authenticator fails to initialize if no credentials are provided (or if auxprops cannot be loaded). Since the default parameters specify CRAM-MD5 authenticator, no required authentication, and no credentials, we must support this starting successfully, although we do log a warning (Only non-authenticating frameworks and slaves are allowed to connect. Authentication is disabled: error_message). In this case, we must allow non-authenticating frameworks/slaves to register without authentication, but we will return an AuthenticationError if they actually try to authenticate. This is the same behavior as before. For scenarios where the authenticator fails to load, but authentication is explicitly enabled for frameworks/slaves, we do exit the master during initialization Cannot start master with authentication enabled: error_message) Thanks Adam for your explanations. Vinod, please let me know if this part was matching your expectations. On Jan. 28, 2015, 8:33 p.m., Vinod Kone wrote: src/master/master.cpp, lines 4193-4203 https://reviews.apache.org/r/27760/diff/12/?file=834181#file834181line4193 IIRC, sending an AuthenticationErrorMessage here causes the driver to simply retry the authentication and thus fall into a retry loop? Adam B wrote: Truth. There is a TODO about at least adding a backoff, but maybe we should also attempt registering without authentication? Maybe authentication shouldn't even cause retries under certain scenarios where it will never eventually succeed (bad credentials, no authenticator). Can we make this a TODO at this spot as well and create a JIRA? I'ld also be happy to work on a follow up patch, covering more finegrained authentication result-triggers. - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review70051 --- On Feb. 18, 2015, 3:57 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 18, 2015, 3:57 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/Makefile.am d372404 src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On Feb. 17, 2015, 7:37 a.m., Adam B wrote:
Looks much cleaner on first pass, but I'm still trying to figure out if you
really need the whole
Authenticator-AuthenticatorProcess-AuthenticatorSession-AuthenticatorSessionProcess
hierarchy, or if just 2-3 of those would suffice. What's your reasoning?
We want the future to be usable for canceling the authentication progress. So
the onAny callback of the inner future is supposed to terminate the
AuthenticatorSessionProcess - termination here should be entirely
deterministically, so it should not just signal a termination but also wait for
it to be accomplished. Waiting for a process within the same process certainly
would introduce a deadlock. So I need another process to cover this. We want
the entire session management to be covered by the authenticator itself, hence
we can not do this within the Master process.
Hope I am making sense here. Please keep asking as I would love to strip this
down, I just do not see any option right now -- maybe I had just worked on this
stuff for too long :)
On Feb. 17, 2015, 7:37 a.m., Adam B wrote:
src/authentication/cram_md5/authenticator.hpp, lines 112-113
https://reviews.apache.org/r/27760/diff/14/?file=863170#file863170line112
Why aren't these method implementations in an `authenticator.cpp`?
Good point. I was sticking to the old structure far too long. Looks much nicer
when splitting.
On Feb. 17, 2015, 7:37 a.m., Adam B wrote:
src/authentication/cram_md5/authenticator.hpp, line 533
https://reviews.apache.org/r/27760/diff/14/?file=863170#file863170line533
Should this still `process::terminate(process, false)` if the short
term fix is now in `~CRAMMD5AuthenticatorSession`? The 'false' injects the
'terminate' at the end of the process' queue, so other in-flight events get
handled first (semi-graceful shutdown).
Correct, I think it should be a regular terminate at this point now (getting
pushed into the front of the queue).
On Feb. 17, 2015, 7:37 a.m., Adam B wrote:
src/authentication/cram_md5/authenticator.hpp, line 545
https://reviews.apache.org/r/27760/diff/14/?file=863170#file863170line545
What's the reasoning for this being a `static*`? Why wouldn't a plain
old `OptionError` work?
That way, if an error occured in the once-covered case, then any additional
calls to initialize (e.g. by another instance) would still have access to that
former, once-covered error and be able to return it.
On Feb. 17, 2015, 7:37 a.m., Adam B wrote:
src/master/master.cpp, lines 3932-3935
https://reviews.apache.org/r/27760/diff/14/?file=863174#file863174line3932
Not sure why this couldn't be merged into `_authenticate()`. What
really needs to be done asynchronously?
... Oh, is this in case something like `Master::finalize()` discards
the outer future, then the inner (authenticate) future will also be
discarded? Worth a comment?
That is exactly what it is supposed to do, correct. Will add a comment.
On Feb. 17, 2015, 7:37 a.m., Adam B wrote:
src/master/master.cpp, line 3937
https://reviews.apache.org/r/27760/diff/14/?file=863174#file863174line3937
` authenticate.get().isSome()`?
How about this:
```
if (future.isReady()) {
// We should arrive at this point only if the inner future is both,
// ready and has a container value set.
CHECK(authenticate.isReady());
CHECK(authenticate.get().isSome());
[...]
```
On Feb. 17, 2015, 7:37 a.m., Adam B wrote:
src/master/master.cpp, lines 3943-3944
https://reviews.apache.org/r/27760/diff/14/?file=863174#file863174line3943
`else if`? Do you really want to call discard again if already
discarded?
It should be a noop but you are certainly right, should be explicit by using
such ```else if```
- Till
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27760/#review72680
---
On Feb. 13, 2015, 3:20 p.m., Till Toenshoff wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27760/
---
(Updated Feb. 13, 2015, 3:20 p.m.)
Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone.
Bugs: MESOS-2050
https://issues.apache.org/jira/browse/MESOS-2050
Repository: mesos
Description
---
The initial design and implementation of the authenticator module interface
caused issues and was not optimal for heavy lifting setup of external
dependencies. By introducing a two fold design, this has been decoupled from
the authentication message processing. The new design also gets us back on
track to the goal of makeing SASL a soft dependency of mesos.
Diffs
-
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On Feb. 3, 2015, 8:56 p.m., Vinod Kone wrote: src/authentication/cram_md5/authenticator.hpp, lines 517-526 https://reviews.apache.org/r/27760/diff/12-13/?file=834177#file834177line517 Instead of terminate() (or making it public) why not have an onDiscard handler on the future returned at #508 that erases the pid? We were not holding on onto the inner future (authenticate) and hence I had no way to do that. Now I have solved this by wrapping the authenticate future with another one that becomes ready only if we had a completely succefull authentication (that is both, future is ready __AND__ Optionstring contains a principal). On Feb. 3, 2015, 8:56 p.m., Vinod Kone wrote: src/authentication/authenticator.hpp, lines 76-77 https://reviews.apache.org/r/27760/diff/12-13/?file=834176#file834176line76 Not looking at the implementation, this seems to be a weird interface method. if authenticate() returns a future why do the caller need an explicit terminate() to terminate the authentication? would be easy if he can just 'discard' the future which will cause termination. You are completely right. It makes much more sense that way. - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review70822 --- On Feb. 13, 2015, 3:20 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 13, 2015, 3:20 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 13, 2015, 3:20 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs (updated) - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On Jan. 28, 2015, 8:33 p.m., Vinod Kone wrote: Can you expand description on why you designed the authenticator inteface this way? Particularly, expand on The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies and The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. An issue with the new design is that now users have to implement an Authenticator and an AuthenticatorSession to implement a new type of authenticator. Also, we lost the symmetry between authenticatee and authenticator. An Authenticatee now communicates with AuthenticatorSession instead of Authenticator, which is confusing. Is the plan to refactor the Authenticatee similarly? Adam B wrote: We need to load/initialize the auxprop plugin at most once per linux process, so we need the authenticator to live for the lifetime of the master, but we need the sasl connection to be created/disposed with the lifetime of a per-authenticatee auth session. We could probably get away with hiding the Session variable underneath the Authenticator interface, and just call Authenticator::authenticate(pid) which will spin up a new session/connection and Authenticator::cancel/complete(pid) which erases/disposes the session/connection. The sasl-based authenticators will then manage the pid-session map internally, and other non-sasl authenticators don't have to know about that implementation detail. (I think I mentioned this before, but Till will have to remind me why we decided against it.) *The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies* The credentials loading was moved away from `Master::initialize` into the `Master::authenticate` phase. That however caused havoc due to the fact that concurrent read- and write access to the credentials storage (`InMemoryAuxiliaryPropertyPlugin`) now became possible. So we want the credential loading to happen as early as possible and not per authentication request. When looking at possible authenticator implementations, I also realized that some may actually have rather costly initializing to do - e.g. setup a secure connection to a database. *The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos.* In the end, CyrysSASL2 does not have to be a hard dependency of Mesos given that we can wrap its access under a single module library (which then has that dependency). The use of CRAM-MD5 within mesos has a hard dependency against CyrusSASL2 due to fact that the master.cpp is depending on having direct access to `InMemoryAuxiliaryPropertyPlugin`. My goal was to wrap all of that with the modules. - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review70051 --- On Feb. 13, 2015, 3:20 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 13, 2015, 3:20 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On Feb. 4, 2015, 2:29 a.m., Adam B wrote: src/tests/cram_md5_authentication_tests.cpp, line 277 https://reviews.apache.org/r/27760/diff/12-13/?file=834182#file834182line277 Did you mean cancel the authentication process? How about discard...? No I actually meant process authentication as that is exactly what the authenticator does, it authenticates a libprocess process via the Authenticatee interface. But discard sounds much better indeed. On Feb. 4, 2015, 2:29 a.m., Adam B wrote: src/master/master.cpp, line 4319 https://reviews.apache.org/r/27760/diff/12-13/?file=834181#file834181line4319 If we change `authenticator-terminate(pid)` to `future.discard()`, and the future has actually become Ready, will a call to future.discard() still call the onDiscard handler? Because what we really want is just to erase the session from the map so that the Owned pointer drops to 0 refs and the session self-destructs. Once the future becomes ready or discarded or failed, the session gets killed (onAny). On Feb. 4, 2015, 2:29 a.m., Adam B wrote: src/master/master.cpp, lines 4319-4320 https://reviews.apache.org/r/27760/diff/12-13/?file=834181#file834181line4319 Can we tie the authenticator's future to the master's `authenticating` future? Does that even make sense? Do they always have the same lifetime/effects? Yeah, that is what I did - I wrapped the authenticator (authenticate) future by another authenticating future to allow for the status merge (!authenticate.isReady || authenticate.get.isSome() = authenticating.fail()). - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review70863 --- On Feb. 13, 2015, 3:20 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 13, 2015, 3:20 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp 7578ea1 src/authentication/cram_md5/auxprop.hpp d036b11 src/authentication/cram_md5/auxprop.cpp 5ff9755 src/master/master.hpp 6a39df0 src/master/master.cpp f10a3cf src/tests/cram_md5_authentication_tests.cpp dd102dc Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review70863 --- src/authentication/cram_md5/authenticator.hpp https://reviews.apache.org/r/27760/#comment116311 Shouldn't you still check if the authenticator has been initialized? An ignorant developer might try to call authenticate without calling initialize src/master/master.cpp https://reviews.apache.org/r/27760/#comment116375 If we change `authenticator-terminate(pid)` to `future.discard()`, and the future has actually become Ready, will a call to future.discard() still call the onDiscard handler? Because what we really want is just to erase the session from the map so that the Owned pointer drops to 0 refs and the session self-destructs. src/master/master.cpp https://reviews.apache.org/r/27760/#comment116378 Can we tie the authenticator's future to the master's `authenticating` future? Does that even make sense? Do they always have the same lifetime/effects? src/tests/cram_md5_authentication_tests.cpp https://reviews.apache.org/r/27760/#comment116380 Did you mean cancel the authentication process? How about discard...? - Adam B On Feb. 3, 2015, 3:05 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 3, 2015, 3:05 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - src/authentication/authenticator.hpp 460494a src/authentication/cram_md5/authenticator.hpp d739a02 src/authentication/cram_md5/auxprop.hpp b894386 src/authentication/cram_md5/auxprop.cpp cf503a2 src/master/master.hpp 337e00a src/master/master.cpp 1005686 src/tests/cram_md5_authentication_tests.cpp a356aa1 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review70733 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Feb. 3, 2015, 11:05 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 3, 2015, 11:05 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - src/authentication/authenticator.hpp 460494a src/authentication/cram_md5/authenticator.hpp d739a02 src/authentication/cram_md5/auxprop.hpp b894386 src/authentication/cram_md5/auxprop.cpp cf503a2 src/master/master.hpp 337e00a src/master/master.cpp 1005686 src/tests/cram_md5_authentication_tests.cpp a356aa1 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review70822 --- src/authentication/authenticator.hpp https://reviews.apache.org/r/27760/#comment116260 Not looking at the implementation, this seems to be a weird interface method. if authenticate() returns a future why do the caller need an explicit terminate() to terminate the authentication? would be easy if he can just 'discard' the future which will cause termination. src/authentication/cram_md5/authenticator.hpp https://reviews.apache.org/r/27760/#comment116268 Instead of terminate() (or making it public) why not have an onDiscard handler on the future returned at #508 that erases the pid? src/master/master.cpp https://reviews.apache.org/r/27760/#comment116269 Can this be authenticating[pid].discard() ? src/master/master.cpp https://reviews.apache.org/r/27760/#comment116270 ditto. - Vinod Kone On Feb. 3, 2015, 11:05 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 3, 2015, 11:05 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - src/authentication/authenticator.hpp 460494a src/authentication/cram_md5/authenticator.hpp d739a02 src/authentication/cram_md5/auxprop.hpp b894386 src/authentication/cram_md5/auxprop.cpp cf503a2 src/master/master.hpp 337e00a src/master/master.cpp 1005686 src/tests/cram_md5_authentication_tests.cpp a356aa1 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Feb. 3, 2015, 11:05 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Changes --- Revised API again to hide session management within the CRAM MD5 authenticator itself. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs (updated) - src/authentication/authenticator.hpp 460494a src/authentication/cram_md5/authenticator.hpp d739a02 src/authentication/cram_md5/auxprop.hpp b894386 src/authentication/cram_md5/auxprop.cpp cf503a2 src/master/master.hpp 337e00a src/master/master.cpp 1005686 src/tests/cram_md5_authentication_tests.cpp a356aa1 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review69997 --- Ship it! I think this is ready to commit now. @vinodkone, @bmahler, @karya, @nnielsen: Make any objecions ASAP, or I'll go ahead and commit this so it gets into 0.22. - Adam B On Jan. 26, 2015, 12:49 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Jan. 26, 2015, 12:49 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos-git Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - src/authentication/authenticator.hpp 460494a src/authentication/cram_md5/authenticator.hpp d739a02 src/authentication/cram_md5/auxprop.hpp b894386 src/authentication/cram_md5/auxprop.cpp cf503a2 src/master/master.hpp 1d342e5 src/master/master.cpp bda8fda src/tests/cram_md5_authentication_tests.cpp a356aa1 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review70051 --- Can you expand description on why you designed the authenticator inteface this way? Particularly, expand on The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies and The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. An issue with the new design is that now users have to implement an Authenticator and an AuthenticatorSession to implement a new type of authenticator. Also, we lost the symmetry between authenticatee and authenticator. An Authenticatee now communicates with AuthenticatorSession instead of Authenticator, which is confusing. Is the plan to refactor the Authenticatee similarly? src/authentication/cram_md5/authenticator.hpp https://reviews.apache.org/r/27760/#comment114999 lets keep the definitions for all methods of this class all in one place (.cpp). src/authentication/cram_md5/auxprop.hpp https://reviews.apache.org/r/27760/#comment115003 s/persistence/properties/ ? src/master/master.cpp https://reviews.apache.org/r/27760/#comment115034 Hmm. I think this is a serious enough error that the master should fail during initilization rather than sending an error message here. Do you have use cases in mind that warranted this? src/master/master.cpp https://reviews.apache.org/r/27760/#comment115036 IIRC, sending an AuthenticationErrorMessage here causes the driver to simply retry the authentication and thus fall into a retry loop? - Vinod Kone On Jan. 26, 2015, 8:49 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Jan. 26, 2015, 8:49 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos-git Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - src/authentication/authenticator.hpp 460494a src/authentication/cram_md5/authenticator.hpp d739a02 src/authentication/cram_md5/auxprop.hpp b894386 src/authentication/cram_md5/auxprop.cpp cf503a2 src/master/master.hpp 1d342e5 src/master/master.cpp bda8fda src/tests/cram_md5_authentication_tests.cpp a356aa1 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On Jan. 28, 2015, 12:33 p.m., Vinod Kone wrote: Can you expand description on why you designed the authenticator inteface this way? Particularly, expand on The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies and The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. An issue with the new design is that now users have to implement an Authenticator and an AuthenticatorSession to implement a new type of authenticator. Also, we lost the symmetry between authenticatee and authenticator. An Authenticatee now communicates with AuthenticatorSession instead of Authenticator, which is confusing. Is the plan to refactor the Authenticatee similarly? We need to load/initialize the auxprop plugin at most once per linux process, so we need the authenticator to live for the lifetime of the master, but we need the sasl connection to be created/disposed with the lifetime of a per-authenticatee auth session. We could probably get away with hiding the Session variable underneath the Authenticator interface, and just call Authenticator::authenticate(pid) which will spin up a new session/connection and Authenticator::cancel/complete(pid) which erases/disposes the session/connection. The sasl-based authenticators will then manage the pid-session map internally, and other non-sasl authenticators don't have to know about that implementation detail. (I think I mentioned this before, but Till will have to remind me why we decided against it.) On Jan. 28, 2015, 12:33 p.m., Vinod Kone wrote: src/master/master.cpp, lines 4173-4190 https://reviews.apache.org/r/27760/diff/12/?file=834181#file834181line4173 Hmm. I think this is a serious enough error that the master should fail during initilization rather than sending an error message here. Do you have use cases in mind that warranted this? The default authenticator is CRAM-MD5 rather than none, and that authenticator fails to initialize if no credentials are provided (or if auxprops cannot be loaded). Since the default parameters specify CRAM-MD5 authenticator, no required authentication, and no credentials, we must support this starting successfully, although we do log a warning (Only non-authenticating frameworks and slaves are allowed to connect. Authentication is disabled: error_message). In this case, we must allow non-authenticating frameworks/slaves to register without authentication, but we will return an AuthenticationError if they actually try to authenticate. This is the same behavior as before. For scenarios where the authenticator fails to load, but authentication is explicitly enabled for frameworks/slaves, we do exit the master during initialization Cannot start master with authentication enabled: error_message) On Jan. 28, 2015, 12:33 p.m., Vinod Kone wrote: src/master/master.cpp, lines 4193-4203 https://reviews.apache.org/r/27760/diff/12/?file=834181#file834181line4193 IIRC, sending an AuthenticationErrorMessage here causes the driver to simply retry the authentication and thus fall into a retry loop? Truth. There is a TODO about at least adding a backoff, but maybe we should also attempt registering without authentication? Maybe authentication shouldn't even cause retries under certain scenarios where it will never eventually succeed (bad credentials, no authenticator). - Adam --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review70051 --- On Jan. 26, 2015, 12:49 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Jan. 26, 2015, 12:49 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos-git Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - src/authentication/authenticator.hpp 460494a src/authentication/cram_md5/authenticator.hpp d739a02 src/authentication/cram_md5/auxprop.hpp b894386 src/authentication/cram_md5/auxprop.cpp cf503a2 src/master/master.hpp 1d342e5 src/master/master.cpp bda8fda
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On Jan. 28, 2015, noon, Adam B wrote: I think this is ready to commit now. @vinodkone, @bmahler, @karya, @nnielsen: Make any objecions ASAP, or I'll go ahead and commit this so it gets into 0.22. i'll take a look today. thanks for pinging. - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review69997 --- On Jan. 26, 2015, 8:49 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Jan. 26, 2015, 8:49 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos-git Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - src/authentication/authenticator.hpp 460494a src/authentication/cram_md5/authenticator.hpp d739a02 src/authentication/cram_md5/auxprop.hpp b894386 src/authentication/cram_md5/auxprop.cpp cf503a2 src/master/master.hpp 1d342e5 src/master/master.cpp bda8fda src/tests/cram_md5_authentication_tests.cpp a356aa1 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review69593 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On Jan. 26, 2015, 8:49 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated Jan. 26, 2015, 8:49 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos-git Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - src/authentication/authenticator.hpp 460494a src/authentication/cram_md5/authenticator.hpp d739a02 src/authentication/cram_md5/auxprop.hpp b894386 src/authentication/cram_md5/auxprop.cpp cf503a2 src/master/master.hpp 1d342e5 src/master/master.cpp bda8fda src/tests/cram_md5_authentication_tests.cpp a356aa1 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
