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