Re: Review Request 44319: Removed initialize method from the authorizer interface.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On March 11, 2016, 1:45 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44319/
> ---
> 
> (Updated March 11, 2016, 1:45 p.m.)
> 
> 
> Review request for mesos, Adam B, Joerg Schad, and Vinod Kone.
> 
> 
> Bugs: MESOS-2950
> https://issues.apache.org/jira/browse/MESOS-2950
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed initialize method from the authorizer interface.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> 705482656788ac12e9d21e355b71fd2ede2be558 
>   src/authorizer/authorizer.cpp 54278b022118c40d3b976794fd472ce8d8b6a5e2 
>   src/authorizer/local/authorizer.hpp 
> c87a9915bae6bae7744bd57abd12e8d857181051 
>   src/authorizer/local/authorizer.cpp 
> 15c857de79cd7dd2c29b6b8cfb81204b919f1b28 
>   src/examples/test_authorizer_module.cpp 
> 95d77fbff0cdfdb360a8597fbba28404b59d0042 
>   src/local/local.cpp 359fc54d7c4081f536a8de8b1dfcde413d75c9a9 
>   src/master/main.cpp 4263110c9b889984ef74eb94fed629958f2abd79 
>   src/tests/authorization_tests.cpp 2b2297036550412a955ff479f6ec9d7dad8cb0e3 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/44319/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 44319: Removed initialize method from the authorizer interface.

[Alexander Rojas]

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

(Updated March 11, 2016, 2:45 p.m.)


Review request for mesos, Adam B, Joerg Schad, and Vinod Kone.


Changes
---

Addresses Vinod's issues.


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


Repository: mesos


Description
---

Removed initialize method from the authorizer interface.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.hpp 
705482656788ac12e9d21e355b71fd2ede2be558 
  src/authorizer/authorizer.cpp 54278b022118c40d3b976794fd472ce8d8b6a5e2 
  src/authorizer/local/authorizer.hpp c87a9915bae6bae7744bd57abd12e8d857181051 
  src/authorizer/local/authorizer.cpp 15c857de79cd7dd2c29b6b8cfb81204b919f1b28 
  src/examples/test_authorizer_module.cpp 
95d77fbff0cdfdb360a8597fbba28404b59d0042 
  src/local/local.cpp 359fc54d7c4081f536a8de8b1dfcde413d75c9a9 
  src/master/main.cpp 4263110c9b889984ef74eb94fed629958f2abd79 
  src/tests/authorization_tests.cpp 2b2297036550412a955ff479f6ec9d7dad8cb0e3 
  src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 44319: Removed initialize method from the authorizer interface.

[Alexander Rojas]

> On March 10, 2016, 8:32 p.m., Vinod Kone wrote:
> > src/master/main.cpp, line 372
> > 
> >
> > So, we create non-default authorizer if --authorizers specifies a 
> > non-default name but create the default authorizer only if --acls is 
> > present (irrespective of whether --authorizers says "local)". 
> > 
> > I think those semantics are a bit weird to grok. IIRC, we did it that 
> > way for backwards compatibility with old releases that didn't have 
> > "--authorizers" flag? We should atleast mention this behavior in the flags 
> > help.
> 
> Alexander Rojas wrote:
> That is not how the block reads, that block is equivalent to:
> 
> ```c++
> if (authorizerName != master::DEFAULT_AUTHORIZER) {
> LOG(INFO) << "Creating '" << authorizerName << "' authorizer";
> 
> create = Authorizer::create(authorizerName);
>   } else { // authorizerName is the default authorizer at this point!
> if (flags.acls.isSome()) {
>   LOG(INFO) << "Creating default '" << authorizerName << "' 
> authorizer";
> 
>   create = Authorizer::create(flags.acls.get());
> } 
>   }
> ```
> 
> So `--authorizer` has to say _local_ **and** `--acls` have to be present.

I changed the code to the snippet above, so there are no confusions in the 
future.


- Alexander


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


On March 9, 2016, 3:06 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44319/
> ---
> 
> (Updated March 9, 2016, 3:06 p.m.)
> 
> 
> Review request for mesos, Adam B, Joerg Schad, and Vinod Kone.
> 
> 
> Bugs: MESOS-2950
> https://issues.apache.org/jira/browse/MESOS-2950
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed initialize method from the authorizer interface.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> 705482656788ac12e9d21e355b71fd2ede2be558 
>   src/authorizer/authorizer.cpp 54278b022118c40d3b976794fd472ce8d8b6a5e2 
>   src/authorizer/local/authorizer.hpp 
> c87a9915bae6bae7744bd57abd12e8d857181051 
>   src/authorizer/local/authorizer.cpp 
> 15c857de79cd7dd2c29b6b8cfb81204b919f1b28 
>   src/examples/test_authorizer_module.cpp 
> 95d77fbff0cdfdb360a8597fbba28404b59d0042 
>   src/local/local.cpp 359fc54d7c4081f536a8de8b1dfcde413d75c9a9 
>   src/master/main.cpp 4263110c9b889984ef74eb94fed629958f2abd79 
>   src/tests/authorization_tests.cpp 2b2297036550412a955ff479f6ec9d7dad8cb0e3 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/44319/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 44319: Removed initialize method from the authorizer interface.

[Vinod Kone]

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


Fix it, then Ship it!




Looks good. Some minor comments.


include/mesos/authorizer/authorizer.hpp (line 61)


s/returned value/the authorizer/



src/authorizer/local/authorizer.hpp (line 50)


Add a comment for this?



src/authorizer/local/authorizer.cpp (line 353)


s/stringAcls/acls/



src/authorizer/local/authorizer.cpp (line 364)


s/acls/acls_/



src/local/local.cpp (line 228)


s/create/authorizer/



src/local/local.cpp (line 243)


s/authorizer/authorizer_/



src/master/main.cpp (line 372)


So, we create non-default authorizer if --authorizers specifies a 
non-default name but create the default authorizer only if --acls is present 
(irrespective of whether --authorizers says "local)". 

I think those semantics are a bit weird to grok. IIRC, we did it that way 
for backwards compatibility with old releases that didn't have "--authorizers" 
flag? We should atleast mention this behavior in the flags help.



src/tests/cluster.cpp (line 175)


s/create/authorizer/


- Vinod Kone


On March 9, 2016, 2:06 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44319/
> ---
> 
> (Updated March 9, 2016, 2:06 p.m.)
> 
> 
> Review request for mesos, Adam B, Joerg Schad, and Vinod Kone.
> 
> 
> Bugs: MESOS-2950
> https://issues.apache.org/jira/browse/MESOS-2950
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed initialize method from the authorizer interface.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> 705482656788ac12e9d21e355b71fd2ede2be558 
>   src/authorizer/authorizer.cpp 54278b022118c40d3b976794fd472ce8d8b6a5e2 
>   src/authorizer/local/authorizer.hpp 
> c87a9915bae6bae7744bd57abd12e8d857181051 
>   src/authorizer/local/authorizer.cpp 
> 15c857de79cd7dd2c29b6b8cfb81204b919f1b28 
>   src/examples/test_authorizer_module.cpp 
> 95d77fbff0cdfdb360a8597fbba28404b59d0042 
>   src/local/local.cpp 359fc54d7c4081f536a8de8b1dfcde413d75c9a9 
>   src/master/main.cpp 4263110c9b889984ef74eb94fed629958f2abd79 
>   src/tests/authorization_tests.cpp 2b2297036550412a955ff479f6ec9d7dad8cb0e3 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/44319/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 44319: Removed initialize method from the authorizer interface.

[Alexander Rojas]

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

(Updated March 9, 2016, 5:54 a.m.)


Review request for mesos, Adam B, Joerg Schad, and Vinod Kone.


Changes
---

Adresses issues reported by Vinod.


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


Repository: mesos


Description
---

Removed initialize method from the authorizer interface.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.hpp 
ec6c9928c55c3096c7de634f900419abbdd00886 
  src/authorizer/authorizer.cpp 54278b022118c40d3b976794fd472ce8d8b6a5e2 
  src/authorizer/local/authorizer.hpp 96baf77709cf721caf46b6c2c096a843c1b5d9c0 
  src/authorizer/local/authorizer.cpp 4e5c2c2869823ec957735cebfc80dc850d40f9eb 
  src/examples/test_authorizer_module.cpp 
95d77fbff0cdfdb360a8597fbba28404b59d0042 
  src/local/local.cpp 359fc54d7c4081f536a8de8b1dfcde413d75c9a9 
  src/master/main.cpp 4263110c9b889984ef74eb94fed629958f2abd79 
  src/tests/authorization_tests.cpp 2b2297036550412a955ff479f6ec9d7dad8cb0e3 
  src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 44319: Removed initialize method from the authorizer interface.

[Alexander Rojas]

> On March 8, 2016, 7:16 p.m., Vinod Kone wrote:
> > src/examples/test_authorizer_module.cpp, lines 41-61
> > 
> >
> > why can't you use Authorizer::create() here instead of repeating this 
> > flags extracting/parsing logic?

Refactor the code to `local::Authorizer::create(ACLs)` so I guess this is not 
relevant anymore.


> On March 8, 2016, 7:16 p.m., Vinod Kone wrote:
> > src/local/local.cpp, lines 225-233
> > 
> >
> > I'm a little confused. 
> > 
> > Does this mean we allow specifying non-local authorizer as 
> > "flags.authorizers" but only allow "acls" parameter to it? What if the 
> > module writer want to send extra parameters to their module?

I refactored this code based on offline discussion.


> On March 8, 2016, 7:16 p.m., Vinod Kone wrote:
> > src/master/main.cpp, lines 363-369
> > 
> >
> > I see the repetition now.
> > 
> > How about we create an overload  Authorizer::create(const ACLs& acls) 
> > that calls Authorizer::create(const string& name, const Parameters& 
> > parameters). Add a TODO saying that this overload should be killed once 
> > ACLs can be input as a module param instead of top level "--acls" flag.

I refactored this code based on offline discussion.


> On March 8, 2016, 7:16 p.m., Vinod Kone wrote:
> > src/tests/authorization_tests.cpp, lines 56-75
> > 
> >
> > why not use Authorizer::create() instead of the repetition here?

I refactored this code based on offline discussion.


- Alexander


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


On March 8, 2016, 5:43 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44319/
> ---
> 
> (Updated March 8, 2016, 5:43 p.m.)
> 
> 
> Review request for mesos, Adam B, Joerg Schad, and Vinod Kone.
> 
> 
> Bugs: MESOS-2950
> https://issues.apache.org/jira/browse/MESOS-2950
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed initialize method from the authorizer interface.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> ec6c9928c55c3096c7de634f900419abbdd00886 
>   src/authorizer/authorizer.cpp 54278b022118c40d3b976794fd472ce8d8b6a5e2 
>   src/authorizer/local/authorizer.hpp 
> 96baf77709cf721caf46b6c2c096a843c1b5d9c0 
>   src/authorizer/local/authorizer.cpp 
> 4e5c2c2869823ec957735cebfc80dc850d40f9eb 
>   src/examples/test_authorizer_module.cpp 
> 95d77fbff0cdfdb360a8597fbba28404b59d0042 
>   src/local/local.cpp 359fc54d7c4081f536a8de8b1dfcde413d75c9a9 
>   src/master/main.cpp 4263110c9b889984ef74eb94fed629958f2abd79 
>   src/tests/authorization_tests.cpp 2b2297036550412a955ff479f6ec9d7dad8cb0e3 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/44319/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 44319: Removed initialize method from the authorizer interface.

[Vinod Kone]

> On March 8, 2016, 1:08 a.m., Vinod Kone wrote:
> > include/mesos/authorizer/authorizer.hpp, line 58
> > 
> >
> > seems weird that this interface takes ACLs as a param. can we make it 
> > take Parameters instead? I think it gets rid of a bunch of boilerplate code 
> > below.
> 
> Alexander Rojas wrote:
> It won't get rid of any boiler plate code, and I will actually have to 
> add some extra code to translate flags to acls, since the place where this 
> constructor is used is done as `Try create = 
> Authorizer::create(authorizerName, flags.acls);`, Not that this works mostly 
> as a dispatcher which will either call `LocalAuthorizer::create(const ACLs& 
> acls)` or `modules::ModuleManager::create(name);`.
> 
> I considered using a constructor which looks as `Authorizer::create(const 
> Flags&)` but the flags are `internal` which means they are not part of the 
> exported headers.
> 
> However, I will change it to parameters mostly for flexibility! since 
> this header doesn't belong neither to master nor to the agent but can be used 
> by both.

I see. Lets add a separate overload for Authorizer::create() that takes ACLs 
then. See my comments in the next review.


> On March 8, 2016, 1:08 a.m., Vinod Kone wrote:
> > src/examples/test_authorizer_module.cpp, line 41
> > 
> >
> > s/rawAcls/stringAcls/

actually, lets call this "acls_".


> On March 8, 2016, 1:08 a.m., Vinod Kone wrote:
> > src/tests/authorization_tests.cpp, line 51
> > 
> >
> > have we used this pattern elsewhere?
> 
> Alexander Rojas wrote:
> We have a similar patter with the 
> [`http::Authenticator`](https://github.com/apache/mesos/blob/95faeac356d7918803beeaa77098569383539a16/include/mesos/authentication/http/basic_authenticator_factory.hpp),
>  althrough there the factory is part of Mesos (we had to do such a thing 
> because the interface to be a module was part of libprocess, so we couldn't 
> create a mesos friendly `create()` method there, so we extracted to its own 
> factory class).
> 
> The reason I went for this pattern was so I didn't have to create an 
> extra factory function in the `LocalAuthorizer` which takes parameters only 
> for  
> 
> The only alternative I saw to this was to add a constructor which takes 
> `Parameters` to the `LocaAuthorizer` which will be used only for testing 
> purposes, which I didn't want to have. But if you think that path is the 
> right one, please let me know.

LocalAuthorizer::create(const Parameters& parameters) sounds fine to me.


- Vinod


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


On March 8, 2016, 4:43 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44319/
> ---
> 
> (Updated March 8, 2016, 4:43 p.m.)
> 
> 
> Review request for mesos, Adam B, Joerg Schad, and Vinod Kone.
> 
> 
> Bugs: MESOS-2950
> https://issues.apache.org/jira/browse/MESOS-2950
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed initialize method from the authorizer interface.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> ec6c9928c55c3096c7de634f900419abbdd00886 
>   src/authorizer/authorizer.cpp 54278b022118c40d3b976794fd472ce8d8b6a5e2 
>   src/authorizer/local/authorizer.hpp 
> 96baf77709cf721caf46b6c2c096a843c1b5d9c0 
>   src/authorizer/local/authorizer.cpp 
> 4e5c2c2869823ec957735cebfc80dc850d40f9eb 
>   src/examples/test_authorizer_module.cpp 
> 95d77fbff0cdfdb360a8597fbba28404b59d0042 
>   src/local/local.cpp 359fc54d7c4081f536a8de8b1dfcde413d75c9a9 
>   src/master/main.cpp 4263110c9b889984ef74eb94fed629958f2abd79 
>   src/tests/authorization_tests.cpp 2b2297036550412a955ff479f6ec9d7dad8cb0e3 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/44319/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 44319: Removed initialize method from the authorizer interface.

[Alexander Rojas]

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

(Updated March 8, 2016, 5:43 p.m.)


Review request for mesos, Adam B, Joerg Schad, and Vinod Kone.


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


Repository: mesos


Description
---

Removed initialize method from the authorizer interface.


Diffs
-

  include/mesos/authorizer/authorizer.hpp 
ec6c9928c55c3096c7de634f900419abbdd00886 
  src/authorizer/authorizer.cpp 54278b022118c40d3b976794fd472ce8d8b6a5e2 
  src/authorizer/local/authorizer.hpp 96baf77709cf721caf46b6c2c096a843c1b5d9c0 
  src/authorizer/local/authorizer.cpp 4e5c2c2869823ec957735cebfc80dc850d40f9eb 
  src/examples/test_authorizer_module.cpp 
95d77fbff0cdfdb360a8597fbba28404b59d0042 
  src/local/local.cpp 359fc54d7c4081f536a8de8b1dfcde413d75c9a9 
  src/master/main.cpp 4263110c9b889984ef74eb94fed629958f2abd79 
  src/tests/authorization_tests.cpp 2b2297036550412a955ff479f6ec9d7dad8cb0e3 
  src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 

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


Testing
---

make check


Thanks,

Alexander Rojas