Re: Review Request 36049: Added support for modularized Authorizer

[Till Toenshoff]

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

Ship it!



src/local/local.cpp (line 219)


s/authenticator/authorizer/  - will fix while committing.



src/local/local.cpp (line 222)


See above.



src/master/main.cpp (line 349)


See above.



src/master/main.cpp (line 352)


See above.


- Till Toenshoff


On Aug. 12, 2015, 1:52 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36049/
> ---
> 
> (Updated Aug. 12, 2015, 1:52 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Jan Schlicht, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-2947
> https://issues.apache.org/jira/browse/MESOS-2947
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds and integrates helper classes needed to support an `Authorizer` module. 
> Also adds a flag to the master, allowing the selection of an `Authorizer` 
> module.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 506bab15a452f8b1b22328d0c5e9c46af6dfebff 
>   include/mesos/authorizer/authorizer.hpp PRE-CREATION 
>   include/mesos/module/authorizer.hpp PRE-CREATION 
>   src/Makefile.am 942003149b071a322933e2c085d9122903f69713 
>   src/authorizer/authorizer.cpp PRE-CREATION 
>   src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
>   src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
>   src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
>   src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
>   src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
>   src/master/main.cpp e05a472b86170eb26df26aaa4b65437fcdd413ce 
>   src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 
>   src/tests/cluster.hpp 422ebd17da045002294e67027e9ebe1377bc53cd 
> 
> Diff: https://reviews.apache.org/r/36049/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 36049: Added support for modularized Authorizer

[Bernd Mathiske]

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

Ship it!


Ship It!

- Bernd Mathiske


On Aug. 12, 2015, 6:52 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36049/
> ---
> 
> (Updated Aug. 12, 2015, 6:52 a.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Jan Schlicht, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-2947
> https://issues.apache.org/jira/browse/MESOS-2947
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds and integrates helper classes needed to support an `Authorizer` module. 
> Also adds a flag to the master, allowing the selection of an `Authorizer` 
> module.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 506bab15a452f8b1b22328d0c5e9c46af6dfebff 
>   include/mesos/authorizer/authorizer.hpp PRE-CREATION 
>   include/mesos/module/authorizer.hpp PRE-CREATION 
>   src/Makefile.am 942003149b071a322933e2c085d9122903f69713 
>   src/authorizer/authorizer.cpp PRE-CREATION 
>   src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
>   src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
>   src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
>   src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
>   src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
>   src/master/main.cpp e05a472b86170eb26df26aaa4b65437fcdd413ce 
>   src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 
>   src/tests/cluster.hpp 422ebd17da045002294e67027e9ebe1377bc53cd 
> 
> Diff: https://reviews.apache.org/r/36049/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 36049: Added support for modularized Authorizer

[Alexander Rojas]

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

(Updated Aug. 12, 2015, 3:52 p.m.)


Review request for mesos, Adam B, Bernd Mathiske, Jan Schlicht, and Till 
Toenshoff.


Bugs: MESOS-2947
https://issues.apache.org/jira/browse/MESOS-2947


Repository: mesos


Description
---

Adds and integrates helper classes needed to support an `Authorizer` module. 
Also adds a flag to the master, allowing the selection of an `Authorizer` 
module.


Diffs
-

  docs/configuration.md 506bab15a452f8b1b22328d0c5e9c46af6dfebff 
  include/mesos/authorizer/authorizer.hpp PRE-CREATION 
  include/mesos/module/authorizer.hpp PRE-CREATION 
  src/Makefile.am 942003149b071a322933e2c085d9122903f69713 
  src/authorizer/authorizer.cpp PRE-CREATION 
  src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
  src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
  src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
  src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
  src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
  src/master/main.cpp e05a472b86170eb26df26aaa4b65437fcdd413ce 
  src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 
  src/tests/cluster.hpp 422ebd17da045002294e67027e9ebe1377bc53cd 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 36049: Added support for modularized Authorizer

[Alexander Rojas]

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

(Updated Aug. 10, 2015, 5:29 p.m.)


Review request for mesos, Adam B, Bernd Mathiske, Jan Schlicht, and Till 
Toenshoff.


Changes
---

mcpark review.


Bugs: MESOS-2947
https://issues.apache.org/jira/browse/MESOS-2947


Repository: mesos


Description
---

Adds and integrates helper classes needed to support an `Authorizer` module. 
Also adds a flag to the master, allowing the selection of an `Authorizer` 
module.


Diffs (updated)
-

  docs/configuration.md 506bab15a452f8b1b22328d0c5e9c46af6dfebff 
  include/mesos/authorizer/authorizer.hpp PRE-CREATION 
  include/mesos/module/authorizer.hpp PRE-CREATION 
  src/Makefile.am 942003149b071a322933e2c085d9122903f69713 
  src/authorizer/authorizer.cpp PRE-CREATION 
  src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
  src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
  src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
  src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
  src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
  src/master/main.cpp e05a472b86170eb26df26aaa4b65437fcdd413ce 
  src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 
  src/tests/cluster.hpp 422ebd17da045002294e67027e9ebe1377bc53cd 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 36049: Added support for modularized Authorizer

[Joerg Schad]

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



src/master/flags.cpp (line 415)


Could we please add this flag to the documentation i.e. configuration.md


- Joerg Schad


On Aug. 5, 2015, 9:15 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36049/
> ---
> 
> (Updated Aug. 5, 2015, 9:15 a.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Jan Schlicht, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-2947
> https://issues.apache.org/jira/browse/MESOS-2947
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds and integrates helper classes needed to support an `Authorizer` module. 
> Also adds a flag to the master, allowing the selection of an `Authorizer` 
> module.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp PRE-CREATION 
>   include/mesos/module/authorizer.hpp PRE-CREATION 
>   src/Makefile.am 54eaf205eecb6bf1a9a5c4b5ddad55f46ad635ec 
>   src/authorizer/authorizer.cpp PRE-CREATION 
>   src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
>   src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
>   src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
>   src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
>   src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
>   src/master/main.cpp e05a472b86170eb26df26aaa4b65437fcdd413ce 
>   src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 
>   src/tests/cluster.hpp ba17c0c74a9dc36c595c4ad77fe68be94c5c7c0b 
> 
> Diff: https://reviews.apache.org/r/36049/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 36049: Added support for modularized Authorizer

[Michael Park]

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



src/local/local.cpp (lines 217 - 219)


Does it make sense to log a warning for this?

```
if (flags.authorizers != master::DEFAULT_AUTHORIZER && flags.acls.isSome()) 
{
  LOG(WARNNING) << /* acls are being ignored! */;
}
```



src/local/local.cpp (lines 238 - 248)


This behavior is noticeably different, since we used to simply 
`EXIT(EXIT_FAILURE)` on this error. What's the rationale here?

My thoughts: if an authorizer fails to initialize given some ACLs, it's 
seems likely that the ACLs are ill-formed. In which case, the intent from the 
user is clear that they __want__ authorization, but they made a mistake in 
their ACL construction. Therefore, it seems more reasonable for us to exit with 
an error rather than ignoring their intent and skipping authorization 
altogether.



src/master/flags.cpp (line 417)


Can we expand a little upon `Authorizer implementation to use.`?

Maybe something like: `Authorizer implementation to use when authorizating 
actions that required it.`?



src/master/flags.cpp (line 422)


`s/than default/than the default`
`s/contents//`



src/master/flags.cpp (line 425)


I think we can follow what we do for `authenticators` in 
`src/master/master.cpp` to indicate the lack of support for multiple 
authorizers.

(1) split `flags.authorizers` by `,`
(2) if empty -> no authorizers specified
(3) if size > 1, multiple authorizers not supported
(4) proceed with the first name.



src/master/main.cpp (lines 346 - 379)


Same comments as `src/local/local.cpp`


- Michael Park


On Aug. 5, 2015, 9:15 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36049/
> ---
> 
> (Updated Aug. 5, 2015, 9:15 a.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Jan Schlicht, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-2947
> https://issues.apache.org/jira/browse/MESOS-2947
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds and integrates helper classes needed to support an `Authorizer` module. 
> Also adds a flag to the master, allowing the selection of an `Authorizer` 
> module.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp PRE-CREATION 
>   include/mesos/module/authorizer.hpp PRE-CREATION 
>   src/Makefile.am 54eaf205eecb6bf1a9a5c4b5ddad55f46ad635ec 
>   src/authorizer/authorizer.cpp PRE-CREATION 
>   src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
>   src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
>   src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
>   src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
>   src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
>   src/master/main.cpp e05a472b86170eb26df26aaa4b65437fcdd413ce 
>   src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 
>   src/tests/cluster.hpp ba17c0c74a9dc36c595c4ad77fe68be94c5c7c0b 
> 
> Diff: https://reviews.apache.org/r/36049/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 36049: Added support for modularized Authorizer

[Alexander Rojas]

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

(Updated Aug. 5, 2015, 11:15 a.m.)


Review request for mesos, Adam B, Bernd Mathiske, Jan Schlicht, and Till 
Toenshoff.


Changes
---

Fixing spacing issues.


Bugs: MESOS-2947
https://issues.apache.org/jira/browse/MESOS-2947


Repository: mesos


Description
---

Adds and integrates helper classes needed to support an `Authorizer` module. 
Also adds a flag to the master, allowing the selection of an `Authorizer` 
module.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.hpp PRE-CREATION 
  include/mesos/module/authorizer.hpp PRE-CREATION 
  src/Makefile.am 54eaf205eecb6bf1a9a5c4b5ddad55f46ad635ec 
  src/authorizer/authorizer.cpp PRE-CREATION 
  src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
  src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
  src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
  src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
  src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
  src/master/main.cpp e05a472b86170eb26df26aaa4b65437fcdd413ce 
  src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 
  src/tests/cluster.hpp ba17c0c74a9dc36c595c4ad77fe68be94c5c7c0b 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 36049: Added support for modularized Authorizer

[Jan Schlicht]

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

Ship it!



src/local/local.cpp (line 226)


Wrong spacing.


- Jan Schlicht


On Aug. 4, 2015, 6:26 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36049/
> ---
> 
> (Updated Aug. 4, 2015, 6:26 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Jan Schlicht, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-2947
> https://issues.apache.org/jira/browse/MESOS-2947
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds and integrates helper classes needed to support an `Authorizer` module. 
> Also adds a flag to the master, allowing the selection of an `Authorizer` 
> module.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp PRE-CREATION 
>   include/mesos/module/authorizer.hpp PRE-CREATION 
>   src/Makefile.am 54eaf205eecb6bf1a9a5c4b5ddad55f46ad635ec 
>   src/authorizer/authorizer.cpp PRE-CREATION 
>   src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
>   src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
>   src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
>   src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
>   src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
>   src/master/main.cpp e05a472b86170eb26df26aaa4b65437fcdd413ce 
>   src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 
>   src/tests/cluster.hpp ba17c0c74a9dc36c595c4ad77fe68be94c5c7c0b 
> 
> Diff: https://reviews.apache.org/r/36049/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 36049: Added support for modularized Authorizer

[Alexander Rojas]

> On Aug. 3, 2015, 4:34 p.m., Bernd Mathiske wrote:
> > src/local/local.cpp, line 221
> > 
> >
> > That's a bit too subtle for me. Proposals, either:
> > - pass the acls to the custom authorizer and let it decide what to do 
> > with them
> > - or disallow the presence of both flags, terminate the slave with an 
> > error.
> > 
> > The latter would also greatly simplify the control flow here.

The ideal would be to remove the whole `Authorizer::initialize()`, however we 
cannot disallow the presence of both flags since `--authorizers` is always set 
(with a default value). True, the code ain't pretty, but instead of giving it a 
make up, I would rather push for a long term solution (removing the initialize 
method at least with that signature), and then entirely remove this code.


- Alexander


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


On Aug. 4, 2015, 6:26 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36049/
> ---
> 
> (Updated Aug. 4, 2015, 6:26 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Jan Schlicht, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-2947
> https://issues.apache.org/jira/browse/MESOS-2947
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds and integrates helper classes needed to support an `Authorizer` module. 
> Also adds a flag to the master, allowing the selection of an `Authorizer` 
> module.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp PRE-CREATION 
>   include/mesos/module/authorizer.hpp PRE-CREATION 
>   src/Makefile.am 54eaf205eecb6bf1a9a5c4b5ddad55f46ad635ec 
>   src/authorizer/authorizer.cpp PRE-CREATION 
>   src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
>   src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
>   src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
>   src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
>   src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
>   src/master/main.cpp e05a472b86170eb26df26aaa4b65437fcdd413ce 
>   src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 
>   src/tests/cluster.hpp ba17c0c74a9dc36c595c4ad77fe68be94c5c7c0b 
> 
> Diff: https://reviews.apache.org/r/36049/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 36049: Added support for modularized Authorizer

[Alexander Rojas]

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

(Updated Aug. 4, 2015, 6:26 p.m.)


Review request for mesos, Adam B, Bernd Mathiske, Jan Schlicht, and Till 
Toenshoff.


Changes
---

Rebase, changes from reviewers.


Bugs: MESOS-2947
https://issues.apache.org/jira/browse/MESOS-2947


Repository: mesos


Description
---

Adds and integrates helper classes needed to support an `Authorizer` module. 
Also adds a flag to the master, allowing the selection of an `Authorizer` 
module.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.hpp PRE-CREATION 
  include/mesos/module/authorizer.hpp PRE-CREATION 
  src/Makefile.am 54eaf205eecb6bf1a9a5c4b5ddad55f46ad635ec 
  src/authorizer/authorizer.cpp PRE-CREATION 
  src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
  src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
  src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
  src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
  src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
  src/master/main.cpp e05a472b86170eb26df26aaa4b65437fcdd413ce 
  src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 
  src/tests/cluster.hpp ba17c0c74a9dc36c595c4ad77fe68be94c5c7c0b 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 36049: Added support for modularized Authorizer

[Jan Schlicht]

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

Ship it!


Ship It!

- Jan Schlicht


On Aug. 3, 2015, 11:47 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36049/
> ---
> 
> (Updated Aug. 3, 2015, 11:47 a.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Jan Schlicht, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-2947
> https://issues.apache.org/jira/browse/MESOS-2947
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds and integrates helper classes needed to support an `Authorizer` module. 
> Also adds a flag to the master, allowing the selection of an `Authorizer` 
> module.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp PRE-CREATION 
>   include/mesos/module/authorizer.hpp PRE-CREATION 
>   src/Makefile.am e2cbd1524f25b3e3a9419af6bdf8cc6e5022a784 
>   src/authorizer/authorizer.cpp PRE-CREATION 
>   src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
>   src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
>   src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
>   src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
>   src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
>   src/master/main.cpp fd4de4d0d9c3e9617408022d10b5e161bdc911e1 
>   src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 
> 
> Diff: https://reviews.apache.org/r/36049/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 36049: Added support for modularized Authorizer

[Bernd Mathiske]

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



src/local/local.cpp (line 221)


That's a bit too subtle for me. Proposals, either:
- pass the acls to the custom authorizer and let it decide what to do with 
them
- or disallow the presence of both flags, terminate the slave with an error.

The latter would also greatly simplify the control flow here.



src/master/flags.cpp (line 230)


See above. IMHO better to disallow having both flags simultaneously. If the 
user gives contradictory instructions, the slave should not start.



src/master/flags.cpp (line 421)


See above.



src/master/main.cpp (line 305)


See above.


- Bernd Mathiske


On Aug. 3, 2015, 2:47 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36049/
> ---
> 
> (Updated Aug. 3, 2015, 2:47 a.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Jan Schlicht, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-2947
> https://issues.apache.org/jira/browse/MESOS-2947
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds and integrates helper classes needed to support an `Authorizer` module. 
> Also adds a flag to the master, allowing the selection of an `Authorizer` 
> module.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp PRE-CREATION 
>   include/mesos/module/authorizer.hpp PRE-CREATION 
>   src/Makefile.am e2cbd1524f25b3e3a9419af6bdf8cc6e5022a784 
>   src/authorizer/authorizer.cpp PRE-CREATION 
>   src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
>   src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
>   src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
>   src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
>   src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
>   src/master/main.cpp fd4de4d0d9c3e9617408022d10b5e161bdc911e1 
>   src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 
> 
> Diff: https://reviews.apache.org/r/36049/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 36049: Added support for modularized Authorizer

[Till Toenshoff]

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

Ship it!


Ship It!

- Till Toenshoff


On Aug. 3, 2015, 9:47 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36049/
> ---
> 
> (Updated Aug. 3, 2015, 9:47 a.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Jan Schlicht, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-2947
> https://issues.apache.org/jira/browse/MESOS-2947
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds and integrates helper classes needed to support an `Authorizer` module. 
> Also adds a flag to the master, allowing the selection of an `Authorizer` 
> module.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp PRE-CREATION 
>   include/mesos/module/authorizer.hpp PRE-CREATION 
>   src/Makefile.am e2cbd1524f25b3e3a9419af6bdf8cc6e5022a784 
>   src/authorizer/authorizer.cpp PRE-CREATION 
>   src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
>   src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
>   src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
>   src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
>   src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
>   src/master/main.cpp fd4de4d0d9c3e9617408022d10b5e161bdc911e1 
>   src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 
> 
> Diff: https://reviews.apache.org/r/36049/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 36049: Added support for modularized Authorizer

[Alexander Rojas]

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

(Updated July 20, 2015, 11:01 p.m.)


Review request for mesos, Adam B and Till Toenshoff.


Changes
---

Till's review.


Bugs: MESOS-2947
https://issues.apache.org/jira/browse/MESOS-2947


Repository: mesos


Description
---

Adds and integrates helper classes needed to support an `Authorizer` module. 
Also adds a flag to the master, allowing the selection of an `Authorizer` 
module.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.hpp PRE-CREATION 
  include/mesos/module/authorizer.hpp PRE-CREATION 
  src/Makefile.am e2cbd1524f25b3e3a9419af6bdf8cc6e5022a784 
  src/authorizer/authorizer.cpp PRE-CREATION 
  src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
  src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
  src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
  src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
  src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
  src/master/main.cpp fd4de4d0d9c3e9617408022d10b5e161bdc911e1 
  src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 36049: Added support for modularized Authorizer

[Alexander Rojas]

> On July 8, 2015, 12:12 a.m., Till Toenshoff wrote:
> > src/local/local.cpp, line 241
> > 
> >
> > I am assuming that the `LocalAuthorizer` should be considered unusable 
> > should its initialize function ever fail.
> > 
> > My most favored solution here would be to log the failure and make sure 
> > that `authorizer` remains unset so that we can operate without any 
> > authorization. That would be following the approach of the authenticator 
> > `initialize` failure handling.
> > 
> > ```
> >  Try initialize = 
> > authorizer.get()->initialize(flags.acls.get());
> >  
> >  if (initialize.isError()) {
> >   // A failure to initialize the authorizer does lead to unusable 
> > authorization
> >   // but allows actions to skip authorization.
> >   LOG(WARNING) << "Authorization is disabled: Failed to initialize '"
> ><< flags.authorizers << "' authorizer: " << 
> > initialize.error();
> >   delete authorizer.get();
> >   authorizer = None();
> > }
> > ```
> > 
> > Inherited from  
> > https://github.com/apache/mesos/blob/master/src/master/master.cpp#L484

As a note, please don't use links to the master branch, use a specific review 
since any update on the master invalidates the line you want to show. e.g. 
https://github.com/apache/mesos/blob/bfe6c07b79550bb3d1f2ab6f5344d740e6eb6f60/src/master/master.cpp#L484


- Alexander


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


On July 7, 2015, 9:34 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36049/
> ---
> 
> (Updated July 7, 2015, 9:34 a.m.)
> 
> 
> Review request for mesos, Adam B and Till Toenshoff.
> 
> 
> Bugs: MESOS-2947
> https://issues.apache.org/jira/browse/MESOS-2947
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds and integrates helper classes needed to support an `Authorizer` module. 
> Also adds a flag to the master, allowing the selection of an `Authorizer` 
> module.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp PRE-CREATION 
>   include/mesos/module/authorizer.hpp PRE-CREATION 
>   src/Makefile.am addb63f615f16ae6b25f745b2e79fd9fc0e27851 
>   src/authorizer/authorizer.cpp PRE-CREATION 
>   src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
>   src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
>   src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
>   src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
>   src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
>   src/master/main.cpp 2624b7ea4920a534c98f5dfbf9286c54c50f11a9 
>   src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 
>   src/tests/cluster.hpp cfe7ef0c7a6dc62cddc3e5f5b5b28c8bcb2bed26 
> 
> Diff: https://reviews.apache.org/r/36049/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 36049: Added support for modularized Authorizer

[Till Toenshoff]

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


There are some nits and slight inconsistencies but overall I think we are in 
good shape here.


src/local/local.cpp (line 217)


Capital "The" please.



src/local/local.cpp (line 220)


Please start with a capital "Add" after that colon.



src/local/local.cpp (lines 227 - 228)


I think we should rephrase the message here;

```
"Could not create '" << flags.authorizers << "' authorizer: " << 
create.error()
```



src/local/local.cpp (line 232)


For validating the configuration, I always found it very helpful that we 
were showing the activated authenticator name/s in the master log -- hence I 
would like to suggest to do the same here as well;

```
LOG(INFO) << "Using '" << flags.authorizers << "' authorizer";
```



src/local/local.cpp (line 234)


I am assuming that the `LocalAuthorizer` should be considered unusable 
should its initialize function ever fail.

My most favored solution here would be to log the failure and make sure 
that `authorizer` remains unset so that we can operate without any 
authorization. That would be following the approach of the authenticator 
`initialize` failure handling.

```
 Try initialize = authorizer.get()->initialize(flags.acls.get());
 
 if (initialize.isError()) {
  // A failure to initialize the authorizer does lead to unusable 
authorization
  // but allows actions to skip authorization.
  LOG(WARNING) << "Authorization is disabled: Failed to initialize '"
   << flags.authorizers << "' authorizer: " << 
initialize.error();
  delete authorizer.get();
  authorizer = None();
}
```

Inherited from  
https://github.com/apache/mesos/blob/master/src/master/master.cpp#L484



src/master/flags.cpp (line 230)


s/authorizer/authorizers/



src/master/flags.cpp (line 231)


Lets make sure we match the flag name and also replace that "default" by 
the actual  implementation name.

```
  "Note that if the flag --authorizers is provided with a value different\n"
  "than '" + DEFAULT_AUTHORIZER + "', the ACLs contents will be ignored.\n"
  "\n"
```



src/master/flags.cpp (line 421)


s/authorizer/authorizers/

Please sure you check if you properly renamed that flag in all references. 
Thanks Alexander :)



src/master/flags.cpp (lines 423 - 424)


That looks like weird wrapping to me.



src/master/main.cpp (lines 301 - 317)


See my comments on local.cpp starting at line 217 ff. regarding this entire 
block.


- Till Toenshoff


On July 7, 2015, 7:34 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36049/
> ---
> 
> (Updated July 7, 2015, 7:34 a.m.)
> 
> 
> Review request for mesos, Adam B and Till Toenshoff.
> 
> 
> Bugs: MESOS-2947
> https://issues.apache.org/jira/browse/MESOS-2947
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds and integrates helper classes needed to support an `Authorizer` module. 
> Also adds a flag to the master, allowing the selection of an `Authorizer` 
> module.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp PRE-CREATION 
>   include/mesos/module/authorizer.hpp PRE-CREATION 
>   src/Makefile.am addb63f615f16ae6b25f745b2e79fd9fc0e27851 
>   src/authorizer/authorizer.cpp PRE-CREATION 
>   src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
>   src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
>   src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
>   src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
>   src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
>   src/master/main.cpp 2624b7ea4920a534c98f5dfbf9286c54c50f11a9 
>   src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 
>   src/tests/cluster.hpp cfe7ef0c7a6dc62cddc3e5f5b5b28c8bcb2bed26 
> 
> Diff: https://reviews.apache.org/r/36049/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 36049: Added support for modularized Authorizer

[Alexander Rojas]

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

(Updated July 7, 2015, 9:34 a.m.)


Review request for mesos, Adam B and Till Toenshoff.


Changes
---

Update JIRA entry.


Bugs: MESOS-2947
https://issues.apache.org/jira/browse/MESOS-2947


Repository: mesos


Description
---

Adds and integrates helper classes needed to support an `Authorizer` module. 
Also adds a flag to the master, allowing the selection of an `Authorizer` 
module.


Diffs
-

  include/mesos/authorizer/authorizer.hpp PRE-CREATION 
  include/mesos/module/authorizer.hpp PRE-CREATION 
  src/Makefile.am addb63f615f16ae6b25f745b2e79fd9fc0e27851 
  src/authorizer/authorizer.cpp PRE-CREATION 
  src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
  src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
  src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
  src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
  src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
  src/master/main.cpp 2624b7ea4920a534c98f5dfbf9286c54c50f11a9 
  src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 
  src/tests/cluster.hpp cfe7ef0c7a6dc62cddc3e5f5b5b28c8bcb2bed26 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 36049: Added support for modularized Authorizer

[Alexander Rojas]

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

(Updated July 6, 2015, 5:42 p.m.)


Review request for mesos, Adam B and Till Toenshoff.


Changes
---

Proper use of factory. 
Includes cleanup.


Bugs: MESOS-2945
https://issues.apache.org/jira/browse/MESOS-2945


Repository: mesos


Description
---

Adds and integrates helper classes needed to support an `Authorizer` module. 
Also adds a flag to the master, allowing the selection of an `Authorizer` 
module.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.hpp PRE-CREATION 
  include/mesos/module/authorizer.hpp PRE-CREATION 
  src/Makefile.am addb63f615f16ae6b25f745b2e79fd9fc0e27851 
  src/authorizer/authorizer.cpp PRE-CREATION 
  src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
  src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
  src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
  src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
  src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
  src/master/main.cpp 2624b7ea4920a534c98f5dfbf9286c54c50f11a9 
  src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 
  src/tests/cluster.hpp cfe7ef0c7a6dc62cddc3e5f5b5b28c8bcb2bed26 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 36049: Added support for modularized Authorizer

[Alexander Rojas]

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

(Updated July 3, 2015, 3:40 p.m.)


Review request for mesos and Till Toenshoff.


Bugs: MESOS-2945
https://issues.apache.org/jira/browse/MESOS-2945


Repository: mesos


Description (updated)
---

Adds and integrates helper classes needed to support an `Authorizer` module. 
Also adds a flag to the master, allowing the selection of an `Authorizer` 
module.


Diffs
-

  include/mesos/authorizer/authorizer.hpp PRE-CREATION 
  include/mesos/module/authorizer.hpp PRE-CREATION 
  src/Makefile.am addb63f615f16ae6b25f745b2e79fd9fc0e27851 
  src/authorizer/authorizer.cpp PRE-CREATION 
  src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
  src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
  src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
  src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
  src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
  src/master/main.cpp 2624b7ea4920a534c98f5dfbf9286c54c50f11a9 
  src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 36049: Added support for modularized Authorizer

[Alexander Rojas]

> On July 3, 2015, 10:29 a.m., Till Toenshoff wrote:
> > src/authorizer/authorizer.cpp, lines 17-28
> > 
> >
> > Great update! But it seems the comment is not applying, no?
> > 
> > The built-in authorizer factory `Try 
> > LocalAuthorizer::create()` does not seem to test for `nullptr`. You seem to 
> > do those checks in master.cpp and local.cpp instead. Or am I missing the 
> > point here?

`LocalAuthorizer::create()` now returns an error if new fails, i.e. returns a 
null pointer. Still not sure if I should remove the check in main.


> On July 3, 2015, 10:29 a.m., Till Toenshoff wrote:
> > src/local/local.cpp, lines 227-228
> > 
> >
> > You may reach this without an error-message, no?
> > 
> > How about:
> > ```
> > if (create.isError() || create.get() == nullptr) {
> >   EXIT(EXIT_FAILURE) << "Could not create authorizer module '"
> >  << flags.authorizer << "'"
> >  << (create.isError() ? ": " + create.error() : "");
> >   ;
> > }
> > 
> > ```
> > 
> > Or actually, that factory can not return a `nullptr` anymore, can it?

Now the comment from the previos issue is true, so create never returns a 
nullptr but instead sets the `Try` to an `Error`.


> On July 3, 2015, 10:29 a.m., Till Toenshoff wrote:
> > src/master/flags.cpp, lines 415-424
> > 
> >
> > We may want to follow the pattern used for the authenticator - as in, 
> > call the flag "authorizers" and prepare the CLI interface for allowing the 
> > activation of multiple, concurrently active authorizers.
> > 
> > What do you think?

as bespoken.


- Alexander


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


On July 3, 2015, 3:17 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36049/
> ---
> 
> (Updated July 3, 2015, 3:17 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-2945
> https://issues.apache.org/jira/browse/MESOS-2945
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds and integrates helper classes needed to support an `Authorizer` module. 
> Also adds a flag to the master, allowing the selection of an `Authorizer` 
> module.
> 
> It also adds a flag which allows to select the module name.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp PRE-CREATION 
>   include/mesos/module/authorizer.hpp PRE-CREATION 
>   src/Makefile.am addb63f615f16ae6b25f745b2e79fd9fc0e27851 
>   src/authorizer/authorizer.cpp PRE-CREATION 
>   src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
>   src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
>   src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
>   src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
>   src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
>   src/master/main.cpp 2624b7ea4920a534c98f5dfbf9286c54c50f11a9 
>   src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 
> 
> Diff: https://reviews.apache.org/r/36049/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 36049: Added support for modularized Authorizer

[Alexander Rojas]

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

(Updated July 3, 2015, 3:17 p.m.)


Review request for mesos and Till Toenshoff.


Changes
---

No longer assumes that `Authorizer::create()` can return a null pointer.

Adds documentation on the usage of the `--authorizers` flag.

Adds a **TODO** for support of multiple authorizers.


Bugs: MESOS-2945
https://issues.apache.org/jira/browse/MESOS-2945


Repository: mesos


Description (updated)
---

Adds and integrates helper classes needed to support an `Authorizer` module. 
Also adds a flag to the master, allowing the selection of an `Authorizer` 
module.

It also adds a flag which allows to select the module name.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.hpp PRE-CREATION 
  include/mesos/module/authorizer.hpp PRE-CREATION 
  src/Makefile.am addb63f615f16ae6b25f745b2e79fd9fc0e27851 
  src/authorizer/authorizer.cpp PRE-CREATION 
  src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
  src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
  src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
  src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
  src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
  src/master/main.cpp 2624b7ea4920a534c98f5dfbf9286c54c50f11a9 
  src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 36049: Added support for modularized Authorizer

[Till Toenshoff]

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



src/master/flags.cpp (line 422)


s/default/" + DEFAULT_AUTHORIZER + "/


- Till Toenshoff


On July 2, 2015, 11:34 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36049/
> ---
> 
> (Updated July 2, 2015, 11:34 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-2945
> https://issues.apache.org/jira/browse/MESOS-2945
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds all the helper classes needed to support an Authorizer module.
> 
> It also adds a flag which allows to select the module name.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp PRE-CREATION 
>   include/mesos/module/authorizer.hpp PRE-CREATION 
>   src/Makefile.am addb63f615f16ae6b25f745b2e79fd9fc0e27851 
>   src/authorizer/authorizer.cpp PRE-CREATION 
>   src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
>   src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
>   src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
>   src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
>   src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
>   src/master/main.cpp 2624b7ea4920a534c98f5dfbf9286c54c50f11a9 
>   src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 
> 
> Diff: https://reviews.apache.org/r/36049/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 36049: Added support for modularized Authorizer

[Till Toenshoff]

> On July 3, 2015, 8:29 a.m., Till Toenshoff wrote:
> >

Let's also update the description of this RR;
```
Adds and integrates helper classes needed to support an Authorizer
module. Also adds a flag to the master, allowing the selection of
an Authorizer module.
```

or maybe better `.. Authorizer modules.`


- Till


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


On July 2, 2015, 11:34 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36049/
> ---
> 
> (Updated July 2, 2015, 11:34 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-2945
> https://issues.apache.org/jira/browse/MESOS-2945
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds all the helper classes needed to support an Authorizer module.
> 
> It also adds a flag which allows to select the module name.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp PRE-CREATION 
>   include/mesos/module/authorizer.hpp PRE-CREATION 
>   src/Makefile.am addb63f615f16ae6b25f745b2e79fd9fc0e27851 
>   src/authorizer/authorizer.cpp PRE-CREATION 
>   src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
>   src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
>   src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
>   src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
>   src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
>   src/master/main.cpp 2624b7ea4920a534c98f5dfbf9286c54c50f11a9 
>   src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 
> 
> Diff: https://reviews.apache.org/r/36049/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 36049: Added support for modularized Authorizer

[Till Toenshoff]

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



src/authorizer/authorizer.cpp (lines 17 - 28)


Great update! But it seems the comment is not applying, no?

The built-in authorizer factory `Try 
LocalAuthorizer::create()` does not seem to test for `nullptr`. You seem to do 
those checks in master.cpp and local.cpp instead. Or am I missing the point 
here?



src/local/local.cpp (lines 227 - 228)


You may reach this without an error-message, no?

How about:
```
if (create.isError() || create.get() == nullptr) {
  EXIT(EXIT_FAILURE) << "Could not create authorizer module '"
 << flags.authorizer << "'"
 << (create.isError() ? ": " + create.error() : "");
  ;
}

```

Or actually, that factory can not return a `nullptr` anymore, can it?



src/master/flags.cpp (lines 415 - 424)


We may want to follow the pattern used for the authenticator - as in, call 
the flag "authorizers" and prepare the CLI interface for allowing the 
activation of multiple, concurrently active authorizers.

What do you think?



src/master/main.cpp (line 311)


See local.cpp


- Till Toenshoff


On July 2, 2015, 11:34 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36049/
> ---
> 
> (Updated July 2, 2015, 11:34 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-2945
> https://issues.apache.org/jira/browse/MESOS-2945
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds all the helper classes needed to support an Authorizer module.
> 
> It also adds a flag which allows to select the module name.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp PRE-CREATION 
>   include/mesos/module/authorizer.hpp PRE-CREATION 
>   src/Makefile.am addb63f615f16ae6b25f745b2e79fd9fc0e27851 
>   src/authorizer/authorizer.cpp PRE-CREATION 
>   src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
>   src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
>   src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
>   src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
>   src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
>   src/master/main.cpp 2624b7ea4920a534c98f5dfbf9286c54c50f11a9 
>   src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 
> 
> Diff: https://reviews.apache.org/r/36049/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 36049: Added support for modularized Authorizer

[Alexander Rojas]

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

(Updated July 3, 2015, 1:34 a.m.)


Review request for mesos and Till Toenshoff.


Bugs: MESOS-2945
https://issues.apache.org/jira/browse/MESOS-2945


Repository: mesos


Description
---

Adds all the helper classes needed to support an Authorizer module.

It also adds a flag which allows to select the module name.


Diffs
-

  include/mesos/authorizer/authorizer.hpp PRE-CREATION 
  include/mesos/module/authorizer.hpp PRE-CREATION 
  src/Makefile.am addb63f615f16ae6b25f745b2e79fd9fc0e27851 
  src/authorizer/authorizer.cpp PRE-CREATION 
  src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
  src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
  src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
  src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
  src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
  src/master/main.cpp 2624b7ea4920a534c98f5dfbf9286c54c50f11a9 
  src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 36049: Added support for modularized Authorizer

[Alexander Rojas]

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

(Updated July 2, 2015, 12:25 p.m.)


Review request for mesos and Till Toenshoff.


Changes
---

Fixes include path.


Bugs: MESOS-2945
https://issues.apache.org/jira/browse/MESOS-2945


Repository: mesos


Description
---

Adds all the helper classes needed to support an Authorizer module.

It also adds a flag which allows to select the module name.


Diffs (updated)
-

  include/mesos/module/authorizer.hpp PRE-CREATION 
  src/Makefile.am addb63f615f16ae6b25f745b2e79fd9fc0e27851 
  src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
  src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
  src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
  src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
  src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
  src/master/main.cpp 2624b7ea4920a534c98f5dfbf9286c54c50f11a9 
  src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 

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


Testing
---

make check


Thanks,

Alexander Rojas