Re: Review Request 36927: Pulled apart authorization and authentication validation for frameworks in the master.

2015-07-30 Thread Vinod Kone

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36927/#review93657
---

Ship it!


Ship It!

- Vinod Kone


On July 30, 2015, 10:16 p.m., Ben Mahler wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/36927/
 ---
 
 (Updated July 30, 2015, 10:16 p.m.)
 
 
 Review request for mesos and Vinod Kone.
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Framework authorization is now done through `authorizeFramework` which is 
 consistent with `authorizeTask`.
 Also, `validateFrameworkAuthentication` captures the validation for 
 authentication.
 
 I also made registration and re-registration consistent:
 
 * Both perform the check for root submission, rather than just registration.
 * Authentication checks in `_registerFramework` and `_reregisterFramework` 
 are now comprehensive (thanks to `validateFrameworkAuthentication`).
 
 
 Diffs
 -
 
   src/master/master.hpp 1135049d34f8ed4d56c795ad28c9ca1d07237b63 
   src/master/master.cpp 8162b2b796c9712a7b2d544118fe8de2646a8d32 
 
 Diff: https://reviews.apache.org/r/36927/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Ben Mahler
 




Re: Review Request 36927: Pulled apart authorization and authentication validation for frameworks in the master.

2015-07-30 Thread Ben Mahler


 On July 30, 2015, 12:49 a.m., Vinod Kone wrote:
 

Kept the validation error composition per our offline discussion, returning for 
each case individually led to really verbose code, and we looked at using a 
lambda to leverage 'return', but this seemed to be the simplest route for now.


 On July 30, 2015, 12:49 a.m., Vinod Kone wrote:
  src/master/master.cpp, lines 1574-1576
  https://reviews.apache.org/r/36927/diff/1/?file=1024921#file1024921line1574
 
  so this line will be presented twice if principal is not set (which 
  will be for most frameworks) because this function is called twice. that is 
  kinda unfortunate and likely confusing to users.
  
  not sure what we could do here yet.

Hm.. this is unfortunate, I've pulled it out into the caller code for now to 
avoid the double logging.


- Ben


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36927/#review93532
---


On July 29, 2015, 11:52 p.m., Ben Mahler wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/36927/
 ---
 
 (Updated July 29, 2015, 11:52 p.m.)
 
 
 Review request for mesos and Vinod Kone.
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Framework authorization is now done through `authorizeFramework` which is 
 consistent with `authorizeTask`.
 Also, `validateFrameworkAuthentication` captures the validation for 
 authentication.
 
 I also made registration and re-registration consistent:
 
 * Both perform the check for root submission, rather than just registration.
 * Authentication checks in `_registerFramework` and `_reregisterFramework` 
 are now comprehensive (thanks to `validateFrameworkAuthentication`).
 
 
 Diffs
 -
 
   src/master/master.hpp 1135049d34f8ed4d56c795ad28c9ca1d07237b63 
   src/master/master.cpp 2bfec2f69375444925252480142ee409b8474761 
 
 Diff: https://reviews.apache.org/r/36927/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Ben Mahler
 




Re: Review Request 36927: Pulled apart authorization and authentication validation for frameworks in the master.

2015-07-30 Thread Ben Mahler

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36927/
---

(Updated July 30, 2015, 10:16 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Avoid double logging.


Repository: mesos


Description
---

Framework authorization is now done through `authorizeFramework` which is 
consistent with `authorizeTask`.
Also, `validateFrameworkAuthentication` captures the validation for 
authentication.

I also made registration and re-registration consistent:

* Both perform the check for root submission, rather than just registration.
* Authentication checks in `_registerFramework` and `_reregisterFramework` are 
now comprehensive (thanks to `validateFrameworkAuthentication`).


Diffs (updated)
-

  src/master/master.hpp 1135049d34f8ed4d56c795ad28c9ca1d07237b63 
  src/master/master.cpp 8162b2b796c9712a7b2d544118fe8de2646a8d32 

Diff: https://reviews.apache.org/r/36927/diff/


Testing
---

make check


Thanks,

Ben Mahler



Re: Review Request 36927: Pulled apart authorization and authentication validation for frameworks in the master.

2015-07-29 Thread Vinod Kone

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36927/#review93532
---



src/master/master.cpp (lines 1574 - 1576)
https://reviews.apache.org/r/36927/#comment147918

so this line will be presented twice if principal is not set (which will be 
for most frameworks) because this function is called twice. that is kinda 
unfortunate and likely confusing to users.

not sure what we could do here yet.



src/master/master.cpp (lines 1720 - 1742)
https://reviews.apache.org/r/36927/#comment147917

checking validationError.isNone() in each if statement looks a bit weird. 
how about doing these in an else if instead?

if (frameworkInfo.has_id()  !(frameworkInfo.id() == )) {

} else if (!roles.contains(frameworkInfo.role()) {

} else if (frameworkInfo.user() == root  !flags.root_submissions) {

} else {

}

i think the above is easier to read?



src/master/master.cpp (lines 1731 - 1732)
https://reviews.apache.org/r/36927/#comment147916

why did you use 2 if loops here instead of ?

anyway, see above comment to make it simpler.



src/master/master.cpp (lines 1866 - 1882)
https://reviews.apache.org/r/36927/#comment147919

ditto. please use else if.


- Vinod Kone


On July 29, 2015, 11:52 p.m., Ben Mahler wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/36927/
 ---
 
 (Updated July 29, 2015, 11:52 p.m.)
 
 
 Review request for mesos and Vinod Kone.
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Framework authorization is now done through `authorizeFramework` which is 
 consistent with `authorizeTask`.
 Also, `validateFrameworkAuthentication` captures the validation for 
 authentication.
 
 I also made registration and re-registration consistent:
 
 * Both perform the check for root submission, rather than just registration.
 * Authentication checks in `_registerFramework` and `_reregisterFramework` 
 are now comprehensive (thanks to `validateFrameworkAuthentication`).
 
 
 Diffs
 -
 
   src/master/master.hpp 1135049d34f8ed4d56c795ad28c9ca1d07237b63 
   src/master/master.cpp 2bfec2f69375444925252480142ee409b8474761 
 
 Diff: https://reviews.apache.org/r/36927/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Ben Mahler
 




Re: Review Request 36927: Pulled apart authorization and authentication validation for frameworks in the master.

2015-07-29 Thread Mesos ReviewBot

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36927/#review93549
---


Patch looks great!

Reviews applied: [36927]

All tests passed.

- Mesos ReviewBot


On July 29, 2015, 11:52 p.m., Ben Mahler wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/36927/
 ---
 
 (Updated July 29, 2015, 11:52 p.m.)
 
 
 Review request for mesos and Vinod Kone.
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Framework authorization is now done through `authorizeFramework` which is 
 consistent with `authorizeTask`.
 Also, `validateFrameworkAuthentication` captures the validation for 
 authentication.
 
 I also made registration and re-registration consistent:
 
 * Both perform the check for root submission, rather than just registration.
 * Authentication checks in `_registerFramework` and `_reregisterFramework` 
 are now comprehensive (thanks to `validateFrameworkAuthentication`).
 
 
 Diffs
 -
 
   src/master/master.hpp 1135049d34f8ed4d56c795ad28c9ca1d07237b63 
   src/master/master.cpp 2bfec2f69375444925252480142ee409b8474761 
 
 Diff: https://reviews.apache.org/r/36927/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Ben Mahler
 




Re: Review Request 36927: Pulled apart authorization and authentication validation for frameworks in the master.

2015-07-29 Thread Ben Mahler


 On July 30, 2015, 12:49 a.m., Vinod Kone wrote:
  src/master/master.cpp, lines 1757-1779
  https://reviews.apache.org/r/36927/diff/1/?file=1024921#file1024921line1757
 
  checking validationError.isNone() in each if statement looks a bit 
  weird. how about doing these in an else if instead?
  
  if (frameworkInfo.has_id()  !(frameworkInfo.id() == )) {
  
  } else if (!roles.contains(frameworkInfo.role()) {
  
  } else if (frameworkInfo.user() == root  !flags.root_submissions) {
  
  } else {
  
  }
  
  
  i think the above is easier to read?

That's originally what I did, but it's harder to read that way. It looks saner 
in your example because you used empty blocks. Here is what it would actually 
look like:

```
  // TODO(vinod): Add != operator for FrameworkID.
  if (frameworkInfo.has_id()  !(frameworkInfo.id() == )) {
validationError = Error(Registering with 'id' already set);
  } else if (!roles.contains(frameworkInfo.role())) {
// TODO(vinod): Deprecate this in favor of ACLs.
validationError = Error(Role ' + frameworkInfo.role() + ' is not +
 present in the master's --roles);
  } else if (frameworkInfo.user() == root  !flags.root_submissions) {
// TODO(vinod): Deprecate this in favor of authorization.
validationError = Error(User 'root' is not allowed to run frameworks
 without --root_submissions set);
  }
```

Also the it doesn't work so nicely for reregister because one of the checks 
requires looping through framework ids.


- Ben


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36927/#review93532
---


On July 29, 2015, 11:52 p.m., Ben Mahler wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/36927/
 ---
 
 (Updated July 29, 2015, 11:52 p.m.)
 
 
 Review request for mesos and Vinod Kone.
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Framework authorization is now done through `authorizeFramework` which is 
 consistent with `authorizeTask`.
 Also, `validateFrameworkAuthentication` captures the validation for 
 authentication.
 
 I also made registration and re-registration consistent:
 
 * Both perform the check for root submission, rather than just registration.
 * Authentication checks in `_registerFramework` and `_reregisterFramework` 
 are now comprehensive (thanks to `validateFrameworkAuthentication`).
 
 
 Diffs
 -
 
   src/master/master.hpp 1135049d34f8ed4d56c795ad28c9ca1d07237b63 
   src/master/master.cpp 2bfec2f69375444925252480142ee409b8474761 
 
 Diff: https://reviews.apache.org/r/36927/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Ben Mahler