----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54062/#review157651 -----------------------------------------------------------
Thanks Jay, looks pretty good! Could you split this patch into the following? (1) Introduce `validate(const FrameworkInfo&)`. (2) Add the tests for (1). (3) Integrate the validation into the master code. (4) Add the tests for (3). src/master/master.cpp (lines 2336 - 2349) <https://reviews.apache.org/r/54062/#comment228287> It seems problematic here that `frameworkInfo.role()` is accesssed when the framework is MULTI_ROLE capable, since it returns `"*"` and will be checked against the whitelist (and this would fail if `"*"` is not in the whitelist!). You could do something like this: ``` if (validationError.isNone()) { // Check the framework's role(s) against the whitelist. set<string> invalidRoles; if (frameworkHasCapability(frameworkInfo, MULTI_ROLE)) { foreach (const string& role, frameworkInfo.roles()) { if (!isWhitelisted(role)) { invalidRoles.insert(role); } } } else { if (!isWhitelisted(frameworkInfo.role())) { invalidRoles.insert(frameworkInfo.role()); } } validationError = Error("Roles " + stringify(invalidRoles) + " are not present in master's --roles"); } ``` src/master/master.cpp (lines 2344 - 2345) <https://reviews.apache.org/r/54062/#comment228284> "the master's" to be consistent with the existing message. src/master/master.cpp (lines 2573 - 2589) <https://reviews.apache.org/r/54062/#comment228288> Ditto here w.r.t. accessing frameworkInfo.role when the framework is MULTI_ROLE capable. src/master/master.cpp (lines 2584 - 2585) <https://reviews.apache.org/r/54062/#comment228285> Ditto here, this one is missing "the" from the existing message. src/master/validation.hpp (lines 59 - 78) <https://reviews.apache.org/r/54062/#comment228291> Let's also mention here that duplicate role entries are not allowed? Also, how about using + characters for the intersecting lines? I send to see this more commonly, e.g.: https://ozh.github.io/ascii-tables/ src/master/validation.hpp (lines 79 - 81) <https://reviews.apache.org/r/54062/#comment228290> Doesn't look like this file is using this comment format, I'd suggest just removing this to avoid making it inconsistent (also it's not really adding a lot of value here?). src/master/validation.cpp (line 18) <https://reviews.apache.org/r/54062/#comment228292> Can you just use hashset instead of set since you do not need ordering? src/master/validation.cpp (lines 240 - 241) <https://reviews.apache.org/r/54062/#comment228293> I don't think we allow putting using statements anywhere except near the top of the .cpp file. Can you just use these in a fully qualified way? Note that you're in mesos::internal already so these could be called as: ``` roles::validate(r); protobuf::frameworkHasCapability(f, c); ``` src/master/validation.cpp (lines 245 - 290) <https://reviews.apache.org/r/54062/#comment228294> We could pull out a `multiRole` boolean here and perform validation based on it, this might make the MULTI_ROLE vs non-MULTI_ROLE distinction a bit clearer. I also noticed that the first check (for both being set) isn't necessary? Here's a suggestion for this logic: ``` bool multiRole = frameworkHasCapability( frameworkInfo, FrameworkInfo::Capability::MULTI_ROLE; // Ensure that the right fields are used. if (multiRole) { if (frameworkInfo.has_role()) { return Error("'FrameworkInfo.role' must not be set when the framework is MULTI_ROLE capable"); } } else { if (frameworkInfo.roles_size() > 0) { return Error("'FrameworkInfo.roles' must not be set when the framework is not MULTI_ROLE capable"); } } // Check for duplicate entries. // // TODO(bmahler): Use a generic duplicate check function. if (multiRole) { const hashset<string> duplicateRoles = [&]() { hashset<string> roles; hashset<string> duplicates; foreach (const string& role, frameworkInfo.roles()) { if (roles.contains(role)) { duplicates.insert(role); } roles.insert(role); } return duplicateRoles; }(); if (!duplicateRoles.empty()) { return Error("'FrameworkInfo.roles' contains duplicate items: " + stringify(duplicateRoles)); } } // Validate the role(s). if (multiRole) { foreach (const string& role, frameworkInfo.roles()) { Option<Error> error = validate(role); if (error) { return Error("'FrameworkInfo.roles' contains invalid role(s)" ": " + error->message); } } } else { Option<Error> error = validate(frameworkInfo.role()); if (error) { return Error("'FrameworkInfo.role' is not a valid role" ": " + error->message); } } return None(); ``` src/master/validation.cpp (line 279) <https://reviews.apache.org/r/54062/#comment228297> We should use single quotes on 'FrameworkInfo.roles' and don't forget to avoid the trailing period :) src/master/validation.cpp (lines 295 - 296) <https://reviews.apache.org/r/54062/#comment228298> s/TODO(jay_guo)/TODO(jay_guo): / Also, you could just say: ``` // TODO(jay_guo): Validate more fields of FrameworkInfo. ``` src/tests/master_validation_tests.cpp (line 2330) <https://reviews.apache.org/r/54062/#comment228299> How about FrameworkInfoValidationTest? - Benjamin Mahler On Nov. 30, 2016, 9:57 a.m., Jay Guo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54062/ > ----------------------------------------------------------- > > (Updated Nov. 30, 2016, 9:57 a.m.) > > > Review request for mesos, Benjamin Mahler, Guangya Liu, and Qiang Zhang. > > > Bugs: MESOS-6629 > https://issues.apache.org/jira/browse/MESOS-6629 > > > Repository: mesos > > > Description > ------- > > We need to do necessary validation for the conflicts of role, roles > and MULTI_ROLE capability. It complies with following matrix: > > -- MULTI_ROLE is NOT set - > |-------|---------| > |Roles |No Roles | > |-------|-------|---------| > |Role | Error | None | > |-------|-------|---------| > |No Role| Error | None | > |-------|-------|---------| > > --- MULTI_ROLE is set ---- > |-------|---------| > |Roles |No Roles | > |-------|-------|---------| > |Role | Error | Error | > |-------|-------|---------| > |No Role| None | None | > |-------|-------|---------| > > One test case is added to ensure validateRoles() catches all invalid > combinations of these attributes. Another three test cases are added > to ensure the master accepts/rejects subscription given valid/invalid > multiple roles. > > > Diffs > ----- > > src/master/master.cpp e03a2e8025943825a2902102c43dc0eb66bacb6a > src/master/validation.hpp b8389460f34b3531f2b6ff93f18f496c01e1a079 > src/master/validation.cpp 42d9b4a8784c2a161b74d7b46619cc22272e14e3 > src/tests/master_validation_tests.cpp > f893067859425967654401f3226149268b51cf57 > > Diff: https://reviews.apache.org/r/54062/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jay Guo > >
