-----------------------------------------------------------
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
> 
>

Reply via email to