Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/ --- (Updated Oct. 31, 2014, 10:18 a.m.) Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. Changes --- Addressed comments. Bugs: MESOS-1889 https://issues.apache.org/jira/browse/MESOS-1889 Repository: mesos-git Description --- Enables selecting a module based authenticator via the new --authenticators flag for mesos master. Additionally, all have been fixed towards in master.hpp and master.cpp. Diffs (updated) - src/master/constants.hpp a8298bc src/master/constants.cpp 3ebd246 src/master/flags.hpp c931fd9 src/master/master.hpp b1a2cd0 src/master/master.cpp 762d2ff src/tests/cram_md5_authentication_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/26859/diff/ Testing --- make check NOTE all four CRAM-MD5 authenticator module related RRs need to get applied before running make check. Thanks, Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/#review59353 --- Ship it! Ship It! - Adam B On Oct. 31, 2014, 3:18 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/ --- (Updated Oct. 31, 2014, 3:18 a.m.) Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. Bugs: MESOS-1889 https://issues.apache.org/jira/browse/MESOS-1889 Repository: mesos-git Description --- Enables selecting a module based authenticator via the new --authenticators flag for mesos master. Additionally, all have been fixed towards in master.hpp and master.cpp. Diffs - src/master/constants.hpp a8298bc src/master/constants.cpp 3ebd246 src/master/flags.hpp c931fd9 src/master/master.hpp b1a2cd0 src/master/master.cpp 762d2ff src/tests/cram_md5_authentication_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/26859/diff/ Testing --- make check NOTE all four CRAM-MD5 authenticator module related RRs need to get applied before running make check. Thanks, Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/ --- (Updated Oct. 30, 2014, 10:18 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. Changes --- Addressed comments. Bugs: MESOS-1889 https://issues.apache.org/jira/browse/MESOS-1889 Repository: mesos-git Description --- Enables selecting a module based authenticator via the new --authenticators flag for mesos master. Additionally, all have been fixed towards in master.hpp and master.cpp. Diffs (updated) - src/master/constants.hpp a8298bc src/master/constants.cpp 3ebd246 src/master/flags.hpp c931fd9 src/master/master.hpp b1a2cd0 src/master/master.cpp 762d2ff src/tests/cram_md5_authentication_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/26859/diff/ Testing --- make check NOTE all four CRAM-MD5 authenticator module related RRs need to get applied before running make check. Thanks, Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/ --- (Updated Oct. 30, 2014, 11:58 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. Changes --- tiny text correction. Bugs: MESOS-1889 https://issues.apache.org/jira/browse/MESOS-1889 Repository: mesos-git Description --- Enables selecting a module based authenticator via the new --authenticators flag for mesos master. Additionally, all have been fixed towards in master.hpp and master.cpp. Diffs (updated) - src/master/constants.hpp a8298bc src/master/constants.cpp 3ebd246 src/master/flags.hpp c931fd9 src/master/master.hpp b1a2cd0 src/master/master.cpp 762d2ff src/tests/cram_md5_authentication_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/26859/diff/ Testing --- make check NOTE all four CRAM-MD5 authenticator module related RRs need to get applied before running make check. Thanks, Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/#review59108 --- src/master/master.cpp https://reviews.apache.org/r/26859/#comment100425 i wish you did this cleanup as a dependent patch. oh well. src/master/master.cpp https://reviews.apache.org/r/26859/#comment100426 Why not just EXIT if we don't support multiple modules? src/master/master.cpp https://reviews.apache.org/r/26859/#comment100427 Do you also want to specify how to specify the modules, e.g., via --modules flag? - Vinod Kone On Oct. 30, 2014, 11:58 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/ --- (Updated Oct. 30, 2014, 11:58 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. Bugs: MESOS-1889 https://issues.apache.org/jira/browse/MESOS-1889 Repository: mesos-git Description --- Enables selecting a module based authenticator via the new --authenticators flag for mesos master. Additionally, all have been fixed towards in master.hpp and master.cpp. Diffs - src/master/constants.hpp a8298bc src/master/constants.cpp 3ebd246 src/master/flags.hpp c931fd9 src/master/master.hpp b1a2cd0 src/master/master.cpp 762d2ff src/tests/cram_md5_authentication_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/26859/diff/ Testing --- make check NOTE all four CRAM-MD5 authenticator module related RRs need to get applied before running make check. Thanks, Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/ --- (Updated Oct. 31, 2014, 1:37 a.m.) Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. Changes --- Addressed comments. Bugs: MESOS-1889 https://issues.apache.org/jira/browse/MESOS-1889 Repository: mesos-git Description --- Enables selecting a module based authenticator via the new --authenticators flag for mesos master. Additionally, all have been fixed towards in master.hpp and master.cpp. Diffs (updated) - src/master/constants.hpp a8298bc src/master/constants.cpp 3ebd246 src/master/flags.hpp c931fd9 src/master/master.hpp b1a2cd0 src/master/master.cpp 762d2ff src/tests/cram_md5_authentication_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/26859/diff/ Testing --- make check NOTE all four CRAM-MD5 authenticator module related RRs need to get applied before running make check. Thanks, Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
On Oct. 31, 2014, 12:43 a.m., Vinod Kone wrote: src/master/master.cpp, line 124 https://reviews.apache.org/r/26859/diff/7/?file=741143#file741143line124 i wish you did this cleanup as a dependent patch. oh well. That indeed would have been better - sry for the inconvenience caused. On Oct. 31, 2014, 12:43 a.m., Vinod Kone wrote: src/master/master.cpp, line 386 https://reviews.apache.org/r/26859/diff/7/?file=741143#file741143line386 Do you also want to specify how to specify the modules, e.g., via --modules flag? Seems you were looking at an older RR - the latest one used the following: EXIT(1) Authenticator ' authenticatorNames[0] ' not found. Check the spelling (compare to BUILTIN_AUTHENTICATOR_NAME ) or verify that the authenticator was loaded from a module (see --modules); - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/#review59108 --- On Oct. 30, 2014, 11:58 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/ --- (Updated Oct. 30, 2014, 11:58 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. Bugs: MESOS-1889 https://issues.apache.org/jira/browse/MESOS-1889 Repository: mesos-git Description --- Enables selecting a module based authenticator via the new --authenticators flag for mesos master. Additionally, all have been fixed towards in master.hpp and master.cpp. Diffs - src/master/constants.hpp a8298bc src/master/constants.cpp 3ebd246 src/master/flags.hpp c931fd9 src/master/master.hpp b1a2cd0 src/master/master.cpp 762d2ff src/tests/cram_md5_authentication_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/26859/diff/ Testing --- make check NOTE all four CRAM-MD5 authenticator module related RRs need to get applied before running make check. Thanks, Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/#review59270 --- src/master/flags.hpp https://reviews.apache.org/r/26859/#comment100563 s/module //? s/loaded and //? src/master/constants.hpp https://reviews.apache.org/r/26859/#comment100536 s/BUILTIN/DEFAULT/? - Adam B On Oct. 30, 2014, 6:37 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/ --- (Updated Oct. 30, 2014, 6:37 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. Bugs: MESOS-1889 https://issues.apache.org/jira/browse/MESOS-1889 Repository: mesos-git Description --- Enables selecting a module based authenticator via the new --authenticators flag for mesos master. Additionally, all have been fixed towards in master.hpp and master.cpp. Diffs - src/master/constants.hpp a8298bc src/master/constants.cpp 3ebd246 src/master/flags.hpp c931fd9 src/master/master.hpp b1a2cd0 src/master/master.cpp 762d2ff src/tests/cram_md5_authentication_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/26859/diff/ Testing --- make check NOTE all four CRAM-MD5 authenticator module related RRs need to get applied before running make check. Thanks, Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26859/#review59308
---
Ship it!
A few minor re-wordings, then we can commit it!
src/master/flags.hpp
https://reviews.apache.org/r/26859/#comment100577
Update this comment to match the actually accepted inputs. No longer can
there be multiple authenticators or even a comma.
Suggested new text:
Authenticator implementation to use when authenticating frameworks and/or
slaves. Use the default ' + DEFAULT_AUTHENTICATOR + ', or load an alternate
authenticator module using --modules.
src/master/master.cpp
https://reviews.apache.org/r/26859/#comment100579
These TODOs are probably obvious enough you could remove them. If/when we
support multiple authenticators, this is the first place we'll have to change
the code, and the EXIT error makes that obvious. :)
src/master/master.cpp
https://reviews.apache.org/r/26859/#comment100580
I'd add single quotes (') around the default authenticator name, so it
reads (compare to 'crammd5')
src/master/master.cpp
https://reviews.apache.org/r/26859/#comment100581
... verify that the authenticator module was loaded successfully (see
--modules)
- Adam B
On Oct. 30, 2014, 6:37 p.m., Till Toenshoff wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26859/
---
(Updated Oct. 30, 2014, 6:37 p.m.)
Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone.
Bugs: MESOS-1889
https://issues.apache.org/jira/browse/MESOS-1889
Repository: mesos-git
Description
---
Enables selecting a module based authenticator via the new --authenticators
flag for mesos master.
Additionally, all have been fixed towards in master.hpp and
master.cpp.
Diffs
-
src/master/constants.hpp a8298bc
src/master/constants.cpp 3ebd246
src/master/flags.hpp c931fd9
src/master/master.hpp b1a2cd0
src/master/master.cpp 762d2ff
src/tests/cram_md5_authentication_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/26859/diff/
Testing
---
make check
NOTE all four CRAM-MD5 authenticator module related RRs need to get applied
before running make check.
Thanks,
Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/ --- (Updated Oct. 29, 2014, 5:19 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. Changes --- Addressed comments. Bugs: MESOS-1889 https://issues.apache.org/jira/browse/MESOS-1889 Repository: mesos-git Description --- Enables selecting a module based authenticator via the new --authenticators flag for mesos master. Additionally, all have been fixed towards in master.hpp and master.cpp. Diffs (updated) - src/examples/test_authenticator_module.cpp PRE-CREATION src/master/flags.hpp c931fd9 src/master/master.hpp b1a2cd0 src/master/master.cpp 762d2ff src/tests/cram_md5_authentication_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/26859/diff/ Testing (updated) --- make check NOTE all four CRAM-MD5 authenticator module related RRs need to get applied before running make check. Thanks, Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
On Oct. 28, 2014, 11:58 p.m., Kapil Arya wrote:
src/master/master.cpp, lines 3879-3888
https://reviews.apache.org/r/26859/diff/6/?file=735286#file735286line3879
Here if the user specifies an invalid modulename but doesn't specify
the module with --modules, shouldn't we give a simple error that the
authenticator 'name' was not found?
Sth along the lines of :
} else if
(modules::ModuleManager::containsAuthenticator(authenticatorModules[0])) {
... ModuleManager::create ...
} else {
ERROR(invalid module)
}
Yes, always better to have deterministic error messages where possible. Thank
you! I did however move that check further up, into initialize as we can check
at that point already.
- Till
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26859/#review58924
---
On Oct. 29, 2014, 5:19 p.m., Till Toenshoff wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26859/
---
(Updated Oct. 29, 2014, 5:19 p.m.)
Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone.
Bugs: MESOS-1889
https://issues.apache.org/jira/browse/MESOS-1889
Repository: mesos-git
Description
---
Enables selecting a module based authenticator via the new --authenticators
flag for mesos master.
Additionally, all have been fixed towards in master.hpp and
master.cpp.
Diffs
-
src/examples/test_authenticator_module.cpp PRE-CREATION
src/master/flags.hpp c931fd9
src/master/master.hpp b1a2cd0
src/master/master.cpp 762d2ff
src/tests/cram_md5_authentication_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/26859/diff/
Testing
---
make check
NOTE all four CRAM-MD5 authenticator module related RRs need to get applied
before running make check.
Thanks,
Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
On Oct. 29, 2014, 12:18 a.m., Cody Maloney wrote: src/master/master.cpp, line 3912 https://reviews.apache.org/r/26859/diff/6/?file=735286#file735286line3912 All the right angle bracket fixes are nice, but the ones in chunks of code which aren't otherwise touched make the diff harder to read. Yes, I feared this would come up. I think that we won't touch all areas of this implementation for a lng time, hence I simply took the freedom to get it done now. On Oct. 29, 2014, 12:18 a.m., Cody Maloney wrote: src/module/authenticator.hpp, line 1 https://reviews.apache.org/r/26859/diff/6/?file=735287#file735287line1 This needs to be added to a Makefile.am so that distcheck will pass. Ow, great point - thanks a bunch! As this file should have been added in 26857, I will fix it there. On Oct. 29, 2014, 12:18 a.m., Cody Maloney wrote: src/master/master.cpp, line 3873 https://reviews.apache.org/r/26859/diff/6/?file=735286#file735286line3873 Since all this code only can load one authenticator module it would be good to check the authetnicatorModules argument only produces a list of length 1 max and warn / exit early. Currently extra arguments are silently ignored. Yes indeed. On Oct. 29, 2014, 12:18 a.m., Cody Maloney wrote: src/master/master.cpp, line 3876 https://reviews.apache.org/r/26859/diff/6/?file=735286#file735286line3876 This assuems crammd5 can only appear first in the list of authenticator modules. MESOS-1939 will fix this, as noted. On Oct. 29, 2014, 12:18 a.m., Cody Maloney wrote: src/master/master.cpp, line 3875 https://reviews.apache.org/r/26859/diff/6/?file=735286#file735286line3875 The strings::split() API makes this a lot more complicated than it should be. split of an empty string returning an empty vector I think would make more sense... But alas... I think it would make sense though to default the argument to a list of one element ['crammd5'] by default. Then you should just error if not --authenticator= is passed without listing any authenticators, rather than try to fall back. Ideally the error would list available authentication modules. Using a default makes much more sense, even though it complicates the flags description a little. Changed that accordingly - thank you!. - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/#review58921 --- On Oct. 29, 2014, 5:19 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/ --- (Updated Oct. 29, 2014, 5:19 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. Bugs: MESOS-1889 https://issues.apache.org/jira/browse/MESOS-1889 Repository: mesos-git Description --- Enables selecting a module based authenticator via the new --authenticators flag for mesos master. Additionally, all have been fixed towards in master.hpp and master.cpp. Diffs - src/examples/test_authenticator_module.cpp PRE-CREATION src/master/flags.hpp c931fd9 src/master/master.hpp b1a2cd0 src/master/master.cpp 762d2ff src/tests/cram_md5_authentication_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/26859/diff/ Testing --- make check NOTE all four CRAM-MD5 authenticator module related RRs need to get applied before running make check. Thanks, Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/ --- (Updated Oct. 29, 2014, 6:51 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. Changes --- Fixed dependency list. Bugs: MESOS-1889 https://issues.apache.org/jira/browse/MESOS-1889 Repository: mesos-git Description --- Enables selecting a module based authenticator via the new --authenticators flag for mesos master. Additionally, all have been fixed towards in master.hpp and master.cpp. Diffs - src/examples/test_authenticator_module.cpp PRE-CREATION src/master/flags.hpp c931fd9 src/master/master.hpp b1a2cd0 src/master/master.cpp 762d2ff src/tests/cram_md5_authentication_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/26859/diff/ Testing --- make check NOTE all four CRAM-MD5 authenticator module related RRs need to get applied before running make check. Thanks, Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/#review59088 --- Looks really good, but I think we need one more round to clear out the references to 'modules' when using generic 'authenticators'. src/examples/test_authenticator_module.cpp https://reviews.apache.org/r/26859/#comment100396 Strange, I didn't see these merge annotations in your previous patch that introduced this file. Not sure why it shows up in this diff, but as long as the lhs never gets checked in, we're fine. src/master/flags.hpp https://reviews.apache.org/r/26859/#comment100400 Why does this even talk about modules? It should talk about authenticators, and the --modules flag can talk about modules. And if there's a module with a new authenticator, you can tell them in your module's howto doc to use --modules to load it and --authenticators to enable it. Suggested new text: Comma separated list of authenticator(s) to use. Only uses the first authenticator in the list for now. src/master/master.hpp https://reviews.apache.org/r/26859/#comment100401 These are no longer necessarily 'modules', so maybe rename the variable to 'authenticatorNames' src/master/master.cpp https://reviews.apache.org/r/26859/#comment100403 s/module // src/master/master.cpp https://reviews.apache.org/r/26859/#comment100404 Shouldn't you also check that !authenticatorNames.empty() in case somebody overrides the default to set --authenticators=? Error and exit src/master/master.cpp https://reviews.apache.org/r/26859/#comment100402 It would be nice to see the name of the authenticator being used: Multiple authenticators are not supported. Only using: + authenticatorNames[0] src/master/master.cpp https://reviews.apache.org/r/26859/#comment100406 Let's get rid of the magic string and make crammd5 a string constant. src/master/master.cpp https://reviews.apache.org/r/26859/#comment100405 Can we reword this to not discuss 'modules'? Maybe just Authenticator ' authenticatorNames[0] ' not found. If you want to suggest how to fix it, you could suggest they check the spelling (compare to crammd5) or verify that they have the authenticator loaded in a module from --modules src/master/master.cpp https://reviews.apache.org/r/26859/#comment100407 Is this an error to 'load' or an error to 'create'? s/load/create/? src/master/master.cpp https://reviews.apache.org/r/26859/#comment100409 'auth' is to short/ambiguous. Use authenticator and authenticator_. You can pick which one is the Owned ptr vs. the normal pointer. src/master/master.cpp https://reviews.apache.org/r/26859/#comment100412 Blank like between these; pretty much always want a blank line before comments. src/tests/cram_md5_authentication_tests.cpp https://reviews.apache.org/r/26859/#comment100450 Two blank lines between tests - Adam B On Oct. 29, 2014, 11:51 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/ --- (Updated Oct. 29, 2014, 11:51 a.m.) Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. Bugs: MESOS-1889 https://issues.apache.org/jira/browse/MESOS-1889 Repository: mesos-git Description --- Enables selecting a module based authenticator via the new --authenticators flag for mesos master. Additionally, all have been fixed towards in master.hpp and master.cpp. Diffs - src/examples/test_authenticator_module.cpp PRE-CREATION src/master/flags.hpp c931fd9 src/master/master.hpp b1a2cd0 src/master/master.cpp 762d2ff src/tests/cram_md5_authentication_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/26859/diff/ Testing --- make check NOTE all four CRAM-MD5 authenticator module related RRs need to get applied before running make check. Thanks, Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/ --- (Updated Oct. 28, 2014, 12:44 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. Changes --- Fixed style issue. Bugs: MESOS-1889 https://issues.apache.org/jira/browse/MESOS-1889 Repository: mesos-git Description --- Enables selecting a module based authenticator via the new --authenticators flag for mesos master. Additionally, all have been fixed towards in master.hpp and master.cpp. Diffs (updated) - src/master/flags.hpp c931fd9 src/master/master.hpp b1a2cd0 src/master/master.cpp 762d2ff src/module/authenticator.hpp PRE-CREATION src/tests/crammd5_authentication_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/26859/diff/ Testing --- make check NOTE all three CRAM-MD5 authenticator module related RRs need to get applied before running make check. Thanks, Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26859/#review58924
---
src/master/master.cpp
https://reviews.apache.org/r/26859/#comment100107
Here if the user specifies an invalid modulename but doesn't specify the
module with --modules, shouldn't we give a simple error that the authenticator
'name' was not found?
Sth along the lines of :
} else if
(modules::ModuleManager::containsAuthenticator(authenticatorModules[0])) {
... ModuleManager::create ...
} else {
ERROR(invalid module)
}
- Kapil Arya
On Oct. 28, 2014, 8:44 a.m., Till Toenshoff wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26859/
---
(Updated Oct. 28, 2014, 8:44 a.m.)
Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone.
Bugs: MESOS-1889
https://issues.apache.org/jira/browse/MESOS-1889
Repository: mesos-git
Description
---
Enables selecting a module based authenticator via the new --authenticators
flag for mesos master.
Additionally, all have been fixed towards in master.hpp and
master.cpp.
Diffs
-
src/master/flags.hpp c931fd9
src/master/master.hpp b1a2cd0
src/master/master.cpp 762d2ff
src/module/authenticator.hpp PRE-CREATION
src/tests/crammd5_authentication_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/26859/diff/
Testing
---
make check
NOTE all three CRAM-MD5 authenticator module related RRs need to get applied
before running make check.
Thanks,
Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/#review58921 --- src/master/master.hpp https://reviews.apache.org/r/26859/#comment100101 There shouldn't be a newline added here src/master/master.cpp https://reviews.apache.org/r/26859/#comment100105 Since all this code only can load one authenticator module it would be good to check the authetnicatorModules argument only produces a list of length 1 max and warn / exit early. Currently extra arguments are silently ignored. src/master/master.cpp https://reviews.apache.org/r/26859/#comment100103 The strings::split() API makes this a lot more complicated than it should be. split of an empty string returning an empty vector I think would make more sense... But alas... I think it would make sense though to default the argument to a list of one element ['crammd5'] by default. Then you should just error if not --authenticator= is passed without listing any authenticators, rather than try to fall back. Ideally the error would list available authentication modules. src/master/master.cpp https://reviews.apache.org/r/26859/#comment100104 This assuems crammd5 can only appear first in the list of authenticator modules. src/master/master.cpp https://reviews.apache.org/r/26859/#comment100113 If authenticator should really be Owned here it would be a lot better to make it owned much earlier so that it is never unowned / a resource which could potentially be lost if / when the unexpected happens. src/master/master.cpp https://reviews.apache.org/r/26859/#comment100114 All the right angle bracket fixes are nice, but the ones in chunks of code which aren't otherwise touched make the diff harder to read. src/module/authenticator.hpp https://reviews.apache.org/r/26859/#comment100115 This needs to be added to a Makefile.am so that distcheck will pass. - Cody Maloney On Oct. 28, 2014, 12:44 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/ --- (Updated Oct. 28, 2014, 12:44 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. Bugs: MESOS-1889 https://issues.apache.org/jira/browse/MESOS-1889 Repository: mesos-git Description --- Enables selecting a module based authenticator via the new --authenticators flag for mesos master. Additionally, all have been fixed towards in master.hpp and master.cpp. Diffs - src/master/flags.hpp c931fd9 src/master/master.hpp b1a2cd0 src/master/master.cpp 762d2ff src/module/authenticator.hpp PRE-CREATION src/tests/crammd5_authentication_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/26859/diff/ Testing --- make check NOTE all three CRAM-MD5 authenticator module related RRs need to get applied before running make check. Thanks, Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/ --- (Updated Oct. 25, 2014, 8:22 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. Changes --- rebased and fixed all towards in master.cpp/.hpp Bugs: MESOS-1889 https://issues.apache.org/jira/browse/MESOS-1889 Repository: mesos-git Description (updated) --- Enables selecting a module based authenticator via the new --authenticators flag for mesos master. Additionally, all have been fixed towards in master.hpp and master.cpp. Diffs (updated) - src/master/flags.hpp 9d06856 src/master/master.hpp b1a2cd0 src/master/master.cpp 95589b8 src/module/authenticator.hpp PRE-CREATION src/tests/crammd5_authentication_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/26859/diff/ Testing --- make check NOTE all three CRAM-MD5 authenticator module related RRs need to get applied before running make check. Thanks, Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/ --- (Updated Oct. 25, 2014, 8:28 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. Changes --- namespace glitch fixed. Bugs: MESOS-1889 https://issues.apache.org/jira/browse/MESOS-1889 Repository: mesos-git Description --- Enables selecting a module based authenticator via the new --authenticators flag for mesos master. Additionally, all have been fixed towards in master.hpp and master.cpp. Diffs (updated) - src/master/flags.hpp 9d06856 src/master/master.hpp b1a2cd0 src/master/master.cpp 95589b8 src/module/authenticator.hpp PRE-CREATION src/tests/crammd5_authentication_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/26859/diff/ Testing --- make check NOTE all three CRAM-MD5 authenticator module related RRs need to get applied before running make check. Thanks, Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
On Oct. 22, 2014, 7:01 a.m., Adam B wrote:
src/master/master.cpp, line 3803
https://reviews.apache.org/r/26859/diff/2/?file=727115#file727115line3803
Would there be any problem creating thousands of instances of the same
module, one for each authentication? This doesn't actually load the library
each time, right?
Correct, it does not. A module create is simply invoking the factory method for
the specific module class -- e.g. src/examples/test_authenticator_module.cpp:
static Authenticator* createCRAMMD5Authenticator(const Parameters parameters)
{
return new mesos::internal::sasl::CRAMMD5Authenticator();
}
The actual dlopen is triggered within src/master/main.cpp, via
ModuleManager::load.
- Till
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26859/#review57763
---
On Oct. 21, 2014, 1:01 p.m., Till Toenshoff wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26859/
---
(Updated Oct. 21, 2014, 1:01 p.m.)
Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone.
Bugs: MESOS-1889
https://issues.apache.org/jira/browse/MESOS-1889
Repository: mesos-git
Description
---
see summary.
Diffs
-
src/master/flags.hpp 9d06856
src/master/master.hpp 14f1d0f
src/master/master.cpp 0a5c9a3
Diff: https://reviews.apache.org/r/26859/diff/
Testing
---
make check
NOTE all three CRAM-MD5 authenticator module related RRs need to get applied
before running make check.
Thanks,
Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
On Oct. 22, 2014, 6:24 p.m., Vinod Kone wrote: src/master/flags.hpp, line 339 https://reviews.apache.org/r/26859/diff/2/?file=727113#file727113line339 I thought the default is going to be cram-md5 to make it backwards compatible? Indeed, the logic has entirely changed in the now updated review-request proposal, following MESOS-1966. - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/#review57845 --- On Oct. 21, 2014, 1:01 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/ --- (Updated Oct. 21, 2014, 1:01 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. Bugs: MESOS-1889 https://issues.apache.org/jira/browse/MESOS-1889 Repository: mesos-git Description --- see summary. Diffs - src/master/flags.hpp 9d06856 src/master/master.hpp 14f1d0f src/master/master.cpp 0a5c9a3 Diff: https://reviews.apache.org/r/26859/diff/ Testing --- make check NOTE all three CRAM-MD5 authenticator module related RRs need to get applied before running make check. Thanks, Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/ --- (Updated Oct. 24, 2014, 2:26 a.m.) Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. Bugs: MESOS-1889 https://issues.apache.org/jira/browse/MESOS-1889 Repository: mesos-git Description --- see summary. Diffs (updated) - src/master/flags.hpp c931fd9 src/master/master.hpp b1a2cd0 src/master/master.cpp 95589b8 src/module/authenticator.hpp PRE-CREATION src/tests/crammd5_authentication_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/26859/diff/ Testing --- make check NOTE all three CRAM-MD5 authenticator module related RRs need to get applied before running make check. Thanks, Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/ --- (Updated Oct. 24, 2014, 2:28 a.m.) Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. Bugs: MESOS-1889 https://issues.apache.org/jira/browse/MESOS-1889 Repository: mesos-git Description --- see summary. Diffs - src/master/flags.hpp c931fd9 src/master/master.hpp b1a2cd0 src/master/master.cpp 95589b8 src/module/authenticator.hpp PRE-CREATION src/tests/crammd5_authentication_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/26859/diff/ Testing --- make check NOTE all three CRAM-MD5 authenticator module related RRs need to get applied before running make check. Thanks, Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/#review57763 --- Great job! src/master/master.cpp https://reviews.apache.org/r/26859/#comment98637 Should this really be a CHECK? What happens if an authenticatee (e.g. slave) tries to authenticate with a master not configured for authentication? src/master/master.cpp https://reviews.apache.org/r/26859/#comment98638 Would there be any problem creating thousands of instances of the same module, one for each authentication? This doesn't actually load the library each time, right? - Adam B On Oct. 21, 2014, 6:01 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/ --- (Updated Oct. 21, 2014, 6:01 a.m.) Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. Bugs: MESOS-1889 https://issues.apache.org/jira/browse/MESOS-1889 Repository: mesos-git Description --- see summary. Diffs - src/master/flags.hpp 9d06856 src/master/master.hpp 14f1d0f src/master/master.cpp 0a5c9a3 Diff: https://reviews.apache.org/r/26859/diff/ Testing --- make check NOTE all three CRAM-MD5 authenticator module related RRs need to get applied before running make check. Thanks, Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/#review57845 --- src/master/flags.hpp https://reviews.apache.org/r/26859/#comment98718 I thought the default is going to be cram-md5 to make it backwards compatible? src/master/master.cpp https://reviews.apache.org/r/26859/#comment98720 +1 This should not be a CHECK. Also, the module name should be extracted out in initialize() and not here. - Vinod Kone On Oct. 21, 2014, 1:01 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/ --- (Updated Oct. 21, 2014, 1:01 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. Bugs: MESOS-1889 https://issues.apache.org/jira/browse/MESOS-1889 Repository: mesos-git Description --- see summary. Diffs - src/master/flags.hpp 9d06856 src/master/master.hpp 14f1d0f src/master/master.cpp 0a5c9a3 Diff: https://reviews.apache.org/r/26859/diff/ Testing --- make check NOTE all three CRAM-MD5 authenticator module related RRs need to get applied before running make check. Thanks, Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/ --- (Updated Oct. 21, 2014, 1:01 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. Bugs: MESOS-1889 https://issues.apache.org/jira/browse/MESOS-1889 Repository: mesos-git Description --- see summary. Diffs (updated) - src/master/flags.hpp 9d06856 src/master/master.hpp 14f1d0f src/master/master.cpp 0a5c9a3 Diff: https://reviews.apache.org/r/26859/diff/ Testing --- make check NOTE all three CRAM-MD5 authenticator module related RRs need to get applied before running make check. Thanks, Till Toenshoff
Review Request 26859: Integrated CRAM-MD5 Authenticator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/ --- Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. Bugs: MESOS-1889 https://issues.apache.org/jira/browse/MESOS-1889 Repository: mesos-git Description --- see summary. Diffs - src/local/local.cpp 66de798 src/master/flags.hpp 9d06856 src/master/master.hpp 14f1d0f src/master/master.cpp 0a5c9a3 Diff: https://reviews.apache.org/r/26859/diff/ Testing --- make check NOTE all three CRAM-MD5 authenticator module related RRs need to get applied before running make check. Thanks, Till Toenshoff
Re: Review Request 26859: Integrated CRAM-MD5 Authenticator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/ --- (Updated Oct. 17, 2014, 4:08 a.m.) Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. Bugs: MESOS-1889 https://issues.apache.org/jira/browse/MESOS-1889 Repository: mesos-git Description --- see summary. Diffs - src/local/local.cpp 66de798 src/master/flags.hpp 9d06856 src/master/master.hpp 14f1d0f src/master/master.cpp 0a5c9a3 Diff: https://reviews.apache.org/r/26859/diff/ Testing --- make check NOTE all three CRAM-MD5 authenticator module related RRs need to get applied before running make check. Thanks, Till Toenshoff
