Re: Review Request 45922: Added agent authorization flags.

2016-04-25 Thread Alexander Rojas

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


Ship it!




Ship It!

- Alexander Rojas


On April 22, 2016, 12:11 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 22, 2016, 12:11 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5 
>   src/local/local.cpp 7de8a2423185e49dfa849d069938d3243b4f4956 
>   src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315 
>   src/slave/main.cpp 38bd00584dd9c6a872398678b2288edeed1cd2a4 
>   src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5 
>   src/slave/slave.cpp a365e8f5f8d63bf80614fccf6adf35a4fee07526 
>   src/tests/cluster.hpp 96ec52af16776e91200aa755c7847f56e33d71f4 
>   src/tests/cluster.cpp 3e5fdf6b32a0d99d3ca83743386232d38471e34f 
>   src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
>   src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-25 Thread Adam B

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


Ship it!




Ship It!

- Adam B


On April 22, 2016, 3:11 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 22, 2016, 3:11 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5 
>   src/local/local.cpp 7de8a2423185e49dfa849d069938d3243b4f4956 
>   src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315 
>   src/slave/main.cpp 38bd00584dd9c6a872398678b2288edeed1cd2a4 
>   src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5 
>   src/slave/slave.cpp a365e8f5f8d63bf80614fccf6adf35a4fee07526 
>   src/tests/cluster.hpp 96ec52af16776e91200aa755c7847f56e33d71f4 
>   src/tests/cluster.cpp 3e5fdf6b32a0d99d3ca83743386232d38471e34f 
>   src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
>   src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-22 Thread Mesos ReviewBot

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



Patch looks great!

Reviews applied: [45922]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On April 22, 2016, 10:11 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 22, 2016, 10:11 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5 
>   src/local/local.cpp 7de8a2423185e49dfa849d069938d3243b4f4956 
>   src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315 
>   src/slave/main.cpp 38bd00584dd9c6a872398678b2288edeed1cd2a4 
>   src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5 
>   src/slave/slave.cpp a365e8f5f8d63bf80614fccf6adf35a4fee07526 
>   src/tests/cluster.hpp 96ec52af16776e91200aa755c7847f56e33d71f4 
>   src/tests/cluster.cpp 3e5fdf6b32a0d99d3ca83743386232d38471e34f 
>   src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
>   src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-22 Thread Jan Schlicht

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

(Updated April 22, 2016, 12:11 p.m.)


Review request for mesos, Adam B and Alexander Rojas.


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


Repository: mesos


Description (updated)
---

See summary.


Diffs
-

  docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5 
  src/local/local.cpp 7de8a2423185e49dfa849d069938d3243b4f4956 
  src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
  src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
  src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315 
  src/slave/main.cpp 38bd00584dd9c6a872398678b2288edeed1cd2a4 
  src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5 
  src/slave/slave.cpp a365e8f5f8d63bf80614fccf6adf35a4fee07526 
  src/tests/cluster.hpp 96ec52af16776e91200aa755c7847f56e33d71f4 
  src/tests/cluster.cpp 3e5fdf6b32a0d99d3ca83743386232d38471e34f 
  src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
  src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 

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


Testing
---

make check


Thanks,

Jan Schlicht



Re: Review Request 45922: Added agent authorization flags.

2016-04-22 Thread Jan Schlicht


> On April 22, 2016, 9:35 a.m., Adam B wrote:
> > src/tests/cluster.cpp, line 392
> > 
> >
> > Double underscores for variable names are discouraged (usually only 
> > acceptable for continuation function names).
> > Maybe the original parameter is `providedAuthorizer`, and 
> > `authorizer__` becomes `newAuthorizer`, then you could have:
> > `authorizer = providedAuthorizer;`
> > `if(...) newAuthorizer = Authorizer::create();`
> > `if(newAuthorizer.isSome()) authorizer = newAuthorizer;`
> > Or maybe you've got better names, but you get the idea.

Thanks for the explanation! I've changed them to `authorizer`, 
`providedAuthorizer` and `createdAuthorizer`.


- Jan


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


On April 22, 2016, 12:05 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 22, 2016, 12:05 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent authorization flags.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5 
>   src/local/local.cpp 7de8a2423185e49dfa849d069938d3243b4f4956 
>   src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315 
>   src/slave/main.cpp 38bd00584dd9c6a872398678b2288edeed1cd2a4 
>   src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5 
>   src/slave/slave.cpp a365e8f5f8d63bf80614fccf6adf35a4fee07526 
>   src/tests/cluster.hpp 96ec52af16776e91200aa755c7847f56e33d71f4 
>   src/tests/cluster.cpp 3e5fdf6b32a0d99d3ca83743386232d38471e34f 
>   src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
>   src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-22 Thread Jan Schlicht

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

(Updated April 22, 2016, 12:05 p.m.)


Review request for mesos, Adam B and Alexander Rojas.


Changes
---

Addressed issues and rebased.


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


Repository: mesos


Description (updated)
---

Added agent authorization flags.


Diffs (updated)
-

  docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5 
  src/local/local.cpp 7de8a2423185e49dfa849d069938d3243b4f4956 
  src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
  src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
  src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315 
  src/slave/main.cpp 38bd00584dd9c6a872398678b2288edeed1cd2a4 
  src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5 
  src/slave/slave.cpp a365e8f5f8d63bf80614fccf6adf35a4fee07526 
  src/tests/cluster.hpp 96ec52af16776e91200aa755c7847f56e33d71f4 
  src/tests/cluster.cpp 3e5fdf6b32a0d99d3ca83743386232d38471e34f 
  src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
  src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 

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


Testing
---

make check


Thanks,

Jan Schlicht



Re: Review Request 45922: Added agent authorization flags.

2016-04-22 Thread Adam B

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


Fix it, then Ship it!




Looks great! Just one issue with variable naming (one underscore doesn't bother 
me much, but two does)


docs/configuration.md (lines 937 - 939)


Unnecessary here; only necessary for the --acls flag.



src/slave/main.cpp (line 292)


You can leave out the quotes. I just wanted the hyphenation



src/tests/cluster.cpp (line 392)


Double underscores for variable names are discouraged (usually only 
acceptable for continuation function names).
Maybe the original parameter is `providedAuthorizer`, and `authorizer__` 
becomes `newAuthorizer`, then you could have:
`authorizer = providedAuthorizer;`
`if(...) newAuthorizer = Authorizer::create();`
`if(newAuthorizer.isSome()) authorizer = newAuthorizer;`
Or maybe you've got better names, but you get the idea.



src/slave/main.cpp (lines 291 - 293)


Let's move this comment into the `if (authorizerName != 
slave::DEFAULT_AUTHORIZER)` block, since that's the condition it applies to. 
Then you can even simplify it down to "NOTE: The contents of --acls are ignored 
for custom authorizers."



src/tests/cluster.cpp (lines 389 - 391)


Simplify and move to before `Authorizer::create(authorizerName);`


- Adam B


On April 21, 2016, 7:48 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 21, 2016, 7:48 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5 
>   src/local/local.cpp 7de8a2423185e49dfa849d069938d3243b4f4956 
>   src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315 
>   src/slave/main.cpp 38bd00584dd9c6a872398678b2288edeed1cd2a4 
>   src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
>   src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553 
>   src/tests/cluster.hpp 96ec52af16776e91200aa755c7847f56e33d71f4 
>   src/tests/cluster.cpp 3e5fdf6b32a0d99d3ca83743386232d38471e34f 
>   src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
>   src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-22 Thread Adam B


> On April 20, 2016, 12:26 a.m., Adam B wrote:
> > src/tests/cluster.hpp, line 151
> > 
> >
> > Why do you even need the overload for the authorizer here? Seems like 
> > most tests will either provide --acls and use the default, or set the 
> > modules/authorizer flags. We can leave this part out until a test needs it.
> 
> Jan Schlicht wrote:
> We will need the overload to be able to use mocked authorizers. This will 
> be important for later authz patches. Of course it is not needed right now, 
> and I could remove it until a test needs it, on the other hand I think it 
> should be part of this patch to provide a "ready-to-use" implementation of an 
> agent authorizer.

Ok, I'm generally against adding things in case we need them later, but I agree 
that mock authorizers will be useful for testing later patches. Dropping the 
issue.


- Adam


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


On April 21, 2016, 7:48 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 21, 2016, 7:48 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5 
>   src/local/local.cpp 7de8a2423185e49dfa849d069938d3243b4f4956 
>   src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315 
>   src/slave/main.cpp 38bd00584dd9c6a872398678b2288edeed1cd2a4 
>   src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
>   src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553 
>   src/tests/cluster.hpp 96ec52af16776e91200aa755c7847f56e33d71f4 
>   src/tests/cluster.cpp 3e5fdf6b32a0d99d3ca83743386232d38471e34f 
>   src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
>   src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-21 Thread Jan Schlicht


> On April 20, 2016, 9:26 a.m., Adam B wrote:
> > src/tests/cluster.cpp, lines 406-412
> > 
> >
> > Why is it an error to start an agent with no authorizer and no ACLs? 
> > What if I don't want to do any authorization?

This was due to a bug that will set `slave::Slave::authorizer` to `nullptr` 
instead of `None()` if no ACLs were provided -- which is how to disable 
authorization. While the above code fixed that, it also removed the possibility 
to disable agent authorization in tests. I've changed the approach, to support 
tests with disabled agent authorization.


- Jan


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


On April 21, 2016, 3:45 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 21, 2016, 3:45 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent authorization flags.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5 
>   src/local/local.cpp 7de8a2423185e49dfa849d069938d3243b4f4956 
>   src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315 
>   src/slave/main.cpp 38bd00584dd9c6a872398678b2288edeed1cd2a4 
>   src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
>   src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553 
>   src/tests/cluster.hpp 96ec52af16776e91200aa755c7847f56e33d71f4 
>   src/tests/cluster.cpp 3e5fdf6b32a0d99d3ca83743386232d38471e34f 
>   src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
>   src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-21 Thread Jan Schlicht

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

(Updated April 21, 2016, 3:45 p.m.)


Review request for mesos, Adam B and Alexander Rojas.


Changes
---

Addressed Adam's issues.


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


Repository: mesos


Description (updated)
---

Added agent authorization flags.


Diffs (updated)
-

  docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5 
  src/local/local.cpp 7de8a2423185e49dfa849d069938d3243b4f4956 
  src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
  src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
  src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315 
  src/slave/main.cpp 38bd00584dd9c6a872398678b2288edeed1cd2a4 
  src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
  src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553 
  src/tests/cluster.hpp 96ec52af16776e91200aa755c7847f56e33d71f4 
  src/tests/cluster.cpp 3e5fdf6b32a0d99d3ca83743386232d38471e34f 
  src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
  src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 

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


Testing
---

make check


Thanks,

Jan Schlicht



Re: Review Request 45922: Added agent authorization flags.

2016-04-21 Thread Jan Schlicht


> On April 20, 2016, 9:26 a.m., Adam B wrote:
> > src/tests/cluster.hpp, line 151
> > 
> >
> > Why do you even need the overload for the authorizer here? Seems like 
> > most tests will either provide --acls and use the default, or set the 
> > modules/authorizer flags. We can leave this part out until a test needs it.

We will need the overload to be able to use mocked authorizers. This will be 
important for later authz patches. Of course it is not needed right now, and I 
could remove it until a test needs it, on the other hand I think it should be 
part of this patch to provide a "ready-to-use" implementation of an agent 
authorizer.


- Jan


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


On April 18, 2016, 11:49 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 18, 2016, 11:49 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md cd9733002a0940f44edd4e56593a0ca3fe9f77f5 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315 
>   src/slave/main.cpp 38bd00584dd9c6a872398678b2288edeed1cd2a4 
>   src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
>   src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553 
>   src/tests/cluster.hpp 887342acc72b33b4c904d610da47394f9a7d7188 
>   src/tests/cluster.cpp 31d2556a078429dd45315aa74cd21ad436372d31 
>   src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
>   src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-20 Thread Adam B

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



Looks great. So very close! Just an unnecessary slave::start() parameter in 
cluster.hpp, and dubious error when authorization is completely disabled in a 
test.
I'll review the /flags authz now, so I can see how these flags/APIs will be 
used.


docs/configuration.md (line 889)


`s/flag --authorizer/--authorizer flag/`



docs/configuration.md (line 890)


s/different/other/



docs/configuration.md (line 932)


`s/flag --authorizer/--authorizer flag/`



docs/configuration.md (line 933)


s/different/other/



docs/configuration.md (line 935)


"See the ACLs protobuf in authorizer.proto for the expected format." (like 
in the flags)
Or is this really "acls.proto" now?



src/slave/flags.cpp (line 453)


`s/flag --authorizer/--authorizer flag/`



src/slave/flags.cpp (line 454)


s/different/other/



src/slave/flags.cpp (line 457)


Isn't this acls.proto now?



src/slave/flags.cpp (line 733)


`--authorizer flag`



src/slave/flags.cpp (line 734)


s/different/other/



src/slave/main.cpp (line 291)


`s/flag --foo/--foo flag/g`



src/slave/main.cpp (line 292)


"non-default"
s/it will be used and //



src/tests/cluster.hpp (line 151)


Why do you even need the overload for the authorizer here? Seems like most 
tests will either provide --acls and use the default, or set the 
modules/authorizer flags. We can leave this part out until a test needs it.



src/tests/cluster.cpp (lines 406 - 412)


Why is it an error to start an agent with no authorizer and no ACLs? What 
if I don't want to do any authorization?



src/tests/mesos.cpp (line 179)


"Set default (permissive) ACLs."



src/tests/mesos.cpp (line 180)


This assumes we separate master and agent ACLs in the local authorizer, 
which your next patch seems to ignore


- Adam B


On April 18, 2016, 2:49 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 18, 2016, 2:49 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md cd9733002a0940f44edd4e56593a0ca3fe9f77f5 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315 
>   src/slave/main.cpp 38bd00584dd9c6a872398678b2288edeed1cd2a4 
>   src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
>   src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553 
>   src/tests/cluster.hpp 887342acc72b33b4c904d610da47394f9a7d7188 
>   src/tests/cluster.cpp 31d2556a078429dd45315aa74cd21ad436372d31 
>   src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
>   src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-19 Thread Jan Schlicht


> On April 12, 2016, 12:30 p.m., Joerg Schad wrote:
> > src/slave/flags.hpp, line 102
> > 
> >
> > Do we have a particular order here?

I don't know exactly. I kept it consistent to the one in `master/flags.hpp`. In 
that context it does make sense to place `acls` next to `credentials`. Of 
course other orders could be possible as well.


- Jan


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


On April 18, 2016, 11:49 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 18, 2016, 11:49 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md cd9733002a0940f44edd4e56593a0ca3fe9f77f5 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315 
>   src/slave/main.cpp 38bd00584dd9c6a872398678b2288edeed1cd2a4 
>   src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
>   src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553 
>   src/tests/cluster.hpp 887342acc72b33b4c904d610da47394f9a7d7188 
>   src/tests/cluster.cpp 31d2556a078429dd45315aa74cd21ad436372d31 
>   src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
>   src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-18 Thread Jan Schlicht


> On April 15, 2016, 1:33 p.m., Benjamin Bannier wrote:
> > src/tests/mesos.cpp, lines 373-377
> > 
> >
> > Could you add a similar helper to inject e.g., a mock authorizer?

It looks like these helper functions are created on demand, so I'd rather not 
add one here now.


- Jan


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


On April 18, 2016, 11:49 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 18, 2016, 11:49 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md cd9733002a0940f44edd4e56593a0ca3fe9f77f5 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315 
>   src/slave/main.cpp 38bd00584dd9c6a872398678b2288edeed1cd2a4 
>   src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
>   src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553 
>   src/tests/cluster.hpp 887342acc72b33b4c904d610da47394f9a7d7188 
>   src/tests/cluster.cpp 31d2556a078429dd45315aa74cd21ad436372d31 
>   src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
>   src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-18 Thread Jan Schlicht


> On April 15, 2016, 12:16 p.m., Alexander Rojas wrote:
> > src/slave/slave.hpp, line 108
> > 
> >
> > I think is a better practice to move optional parameters at the end and 
> > default initialize them to `None()`. 
> > 
> > In fact if you check `master.hpp` it is how it is done there.

It's moved to the end now. Nevertheless, they default initialization to 
`None()` is done in `cluster.hpp`, because how it's done for all of the other 
parameters.


- Jan


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


On April 18, 2016, 11:49 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 18, 2016, 11:49 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md cd9733002a0940f44edd4e56593a0ca3fe9f77f5 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315 
>   src/slave/main.cpp 38bd00584dd9c6a872398678b2288edeed1cd2a4 
>   src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
>   src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553 
>   src/tests/cluster.hpp 887342acc72b33b4c904d610da47394f9a7d7188 
>   src/tests/cluster.cpp 31d2556a078429dd45315aa74cd21ad436372d31 
>   src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
>   src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-18 Thread Jan Schlicht

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

(Updated April 18, 2016, 11:49 a.m.)


Review request for mesos, Adam B and Alexander Rojas.


Changes
---

Addressed issues and rebased.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  docs/configuration.md cd9733002a0940f44edd4e56593a0ca3fe9f77f5 
  src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
  src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
  src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
  src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315 
  src/slave/main.cpp 38bd00584dd9c6a872398678b2288edeed1cd2a4 
  src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
  src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553 
  src/tests/cluster.hpp 887342acc72b33b4c904d610da47394f9a7d7188 
  src/tests/cluster.cpp 31d2556a078429dd45315aa74cd21ad436372d31 
  src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
  src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 

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


Testing
---

make check


Thanks,

Jan Schlicht



Re: Review Request 45922: Added agent authorization flags.

2016-04-15 Thread Benjamin Bannier

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




src/tests/mesos.cpp (lines 373 - 377)


Could you add a similar helper to inject e.g., a mock authorizer?


- Benjamin Bannier


On April 14, 2016, 6:51 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 14, 2016, 6:51 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md ce51f26f0584eeecb5c96e39a5bcc21f5906a29c 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 316feec26e52a8cb1f0fd0739176b3f502e1ed86 
>   src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
>   src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
>   src/slave/slave.cpp de99e9eb5cc812b2e07deb749b98b4f4db363728 
>   src/tests/cluster.hpp 887342acc72b33b4c904d610da47394f9a7d7188 
>   src/tests/cluster.cpp b4d69106388892b88f7de20b248cac8b950b861c 
>   src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-15 Thread Alexander Rojas

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




src/local/local.cpp (line 393)


Move this comment to line 398



src/local/local.cpp (line 398)


```
authorizer_, // Same authorizer as master.
```



src/slave/slave.hpp (line 108)


I think is a better practice to move optional parameters at the end and 
default initialize them to `None()`. 

In fact if you check `master.hpp` it is how it is done there.



src/tests/mesos.cpp (line 468)


This is what I meant. If this were at the end and default initialize, you 
wouldn't have to define it.


- Alexander Rojas


On April 14, 2016, 6:51 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 14, 2016, 6:51 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md ce51f26f0584eeecb5c96e39a5bcc21f5906a29c 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 316feec26e52a8cb1f0fd0739176b3f502e1ed86 
>   src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
>   src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
>   src/slave/slave.cpp de99e9eb5cc812b2e07deb749b98b4f4db363728 
>   src/tests/cluster.hpp 887342acc72b33b4c904d610da47394f9a7d7188 
>   src/tests/cluster.cpp b4d69106388892b88f7de20b248cac8b950b861c 
>   src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-14 Thread Jan Schlicht

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

(Updated April 14, 2016, 6:51 p.m.)


Review request for mesos, Adam B and Alexander Rojas.


Changes
---

Rebased.


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


Repository: mesos


Description (updated)
---

See summary.


Diffs (updated)
-

  docs/configuration.md ce51f26f0584eeecb5c96e39a5bcc21f5906a29c 
  src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
  src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
  src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
  src/slave/flags.cpp 316feec26e52a8cb1f0fd0739176b3f502e1ed86 
  src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
  src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
  src/slave/slave.cpp de99e9eb5cc812b2e07deb749b98b4f4db363728 
  src/tests/cluster.hpp 887342acc72b33b4c904d610da47394f9a7d7188 
  src/tests/cluster.cpp b4d69106388892b88f7de20b248cac8b950b861c 
  src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 

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


Testing
---

make check


Thanks,

Jan Schlicht



Re: Review Request 45922: Added agent authorization flags.

2016-04-14 Thread Jan Schlicht


> On April 12, 2016, 11:31 a.m., Adam B wrote:
> > docs/configuration.md, line 886
> > 
> >
> > None of these examples apply to an agent. Maybe we need to implement an 
> > ACL (e.g. GET_ENDPOINT_WITH_PATH "/flags") before we can provide any 
> > example ACLs for the agent.

Will be implemented in https://reviews.apache.org/r/46203


- Jan


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


On April 14, 2016, 6:16 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 14, 2016, 6:16 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent authorization flags.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md ce51f26f0584eeecb5c96e39a5bcc21f5906a29c 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 316feec26e52a8cb1f0fd0739176b3f502e1ed86 
>   src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
>   src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
>   src/slave/slave.cpp 49fa4a06e26d0d4475ed50db254a320a7030f896 
>   src/tests/cluster.hpp 887342acc72b33b4c904d610da47394f9a7d7188 
>   src/tests/cluster.cpp b4d69106388892b88f7de20b248cac8b950b861c 
>   src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-14 Thread Jan Schlicht

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

(Updated April 14, 2016, 6:16 p.m.)


Review request for mesos, Adam B and Alexander Rojas.


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


Repository: mesos


Description (updated)
---

Added agent authorization flags.


Diffs (updated)
-

  docs/configuration.md ce51f26f0584eeecb5c96e39a5bcc21f5906a29c 
  src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
  src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
  src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
  src/slave/flags.cpp 316feec26e52a8cb1f0fd0739176b3f502e1ed86 
  src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
  src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
  src/slave/slave.cpp 49fa4a06e26d0d4475ed50db254a320a7030f896 
  src/tests/cluster.hpp 887342acc72b33b4c904d610da47394f9a7d7188 
  src/tests/cluster.cpp b4d69106388892b88f7de20b248cac8b950b861c 
  src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 

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


Testing
---

make check


Thanks,

Jan Schlicht



Re: Review Request 45922: Added agent authorization flags.

2016-04-14 Thread Jan Schlicht


> On April 13, 2016, 3:58 p.m., Benjamin Bannier wrote:
> > src/tests/cluster.cpp, lines 398-410
> > 
> >
> > You are shadowing the outer `Option` here with a 
> > `Result`. All of the following assignments are to that and not 
> > the outer one. I believe this leads us to assigning not what was intended 
> > in l.462 below. Having at least a different name would help readability 
> > some here.
> > 
> > As an aside, for my taste the control flow in this block has too much 
> > nesting making this less transparent than it should be.

The shadowing has been resolved. If no authorizer and no ACLs were provided, a 
segfault could occur. This has been fixed as well by making it an error.


- Jan


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


On April 14, 2016, 1 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 14, 2016, 1 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md ce51f26f0584eeecb5c96e39a5bcc21f5906a29c 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 316feec26e52a8cb1f0fd0739176b3f502e1ed86 
>   src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
>   src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
>   src/slave/slave.cpp 49fa4a06e26d0d4475ed50db254a320a7030f896 
>   src/tests/cluster.hpp 887342acc72b33b4c904d610da47394f9a7d7188 
>   src/tests/cluster.cpp b4d69106388892b88f7de20b248cac8b950b861c 
>   src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-14 Thread Mesos ReviewBot

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



Patch looks great!

Reviews applied: [45922]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On April 14, 2016, 11 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 14, 2016, 11 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md ce51f26f0584eeecb5c96e39a5bcc21f5906a29c 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 316feec26e52a8cb1f0fd0739176b3f502e1ed86 
>   src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
>   src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
>   src/slave/slave.cpp 49fa4a06e26d0d4475ed50db254a320a7030f896 
>   src/tests/cluster.hpp 887342acc72b33b4c904d610da47394f9a7d7188 
>   src/tests/cluster.cpp b4d69106388892b88f7de20b248cac8b950b861c 
>   src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-14 Thread Jan Schlicht

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

(Updated April 14, 2016, 1 p.m.)


Review request for mesos, Adam B and Alexander Rojas.


Changes
---

Interim patch. It's fixing the open issues but doesn't add a test case. This 
will be done in a later rev.


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


Repository: mesos


Description (updated)
---

See summary.


Diffs (updated)
-

  docs/configuration.md ce51f26f0584eeecb5c96e39a5bcc21f5906a29c 
  src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
  src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
  src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
  src/slave/flags.cpp 316feec26e52a8cb1f0fd0739176b3f502e1ed86 
  src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
  src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
  src/slave/slave.cpp 49fa4a06e26d0d4475ed50db254a320a7030f896 
  src/tests/cluster.hpp 887342acc72b33b4c904d610da47394f9a7d7188 
  src/tests/cluster.cpp b4d69106388892b88f7de20b248cac8b950b861c 
  src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 

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


Testing
---

make check


Thanks,

Jan Schlicht



Re: Review Request 45922: Added agent authorization flags.

2016-04-13 Thread Jan Schlicht


> On April 12, 2016, 11:31 a.m., Adam B wrote:
> > docs/configuration.md, line 94
> > 
> >
> > Why would we need to support multiple authorizers? Would a particular 
> > request check two authorizers then and/or the results?
> > Or are we somehow using one authorizer for some things and another for 
> > others? This could be explainable for the master's framework authz vs. http 
> > authz, but makes no sense for the agent.
> 
> Alexander Rojas wrote:
> That was an original requirement on the master, since the original 
> crammd5 authorizer was supposed to come also in the multiple version. I think 
> we can, not only remove it here, but also in the master. I don't think we 
> will move onto the multiple authorizers anytime soon.
> 
> Adam B wrote:
> +1, although I dread deprecating the plural flag in master. I could live 
> with it being singular on agent and plural on master, at least for this 
> release; but I guess we should start deprecations ASAP if we have to wait 6mo 
> before we can remove them.

Got it! I'll remove support for multiple authorizers on the agent.


> On April 12, 2016, 11:31 a.m., Adam B wrote:
> > src/slave/flags.cpp, lines 446-449
> > 
> >
> > I'd rather have no example than a confusing/invalid example. We can add 
> > an example after we have at least one ACL
> 
> Joerg Schad wrote:
> +1

Will remove the example.


- Jan


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


On April 8, 2016, 4:25 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 8, 2016, 4:25 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent authorization flags.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 309a5a05eab386c8943ba6bdee8d5efeb448aa0c 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
>   src/slave/constants.hpp 449c8cd9f43f71b4612023eb463969e9db0bc960 
>   src/slave/flags.hpp 4236b7fe40b9aa26d57b651be06a0e36037f65c6 
>   src/slave/flags.cpp 7164afef9c82a9bba0f137d7387c3569912b6599 
>   src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
>   src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
>   src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
>   src/tests/cluster.hpp 39ca15e3cc2f4f2ac9b8a73482c2bd945b1824e3 
>   src/tests/cluster.cpp 7e488d28b7a0d3bef9cd76bf7df2de4822256ef6 
>   src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-13 Thread Benjamin Bannier

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




src/tests/cluster.cpp (lines 398 - 410)


You are shadowing the outer `Option` here with a 
`Result`. All of the following assignments are to that and not the 
outer one. I believe this leads us to assigning not what was intended in l.462 
below. Having at least a different name would help readability some here.

As an aside, for my taste the control flow in this block has too much 
nesting making this less transparent than it should be.


- Benjamin Bannier


On April 8, 2016, 4:25 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 8, 2016, 4:25 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent authorization flags.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 309a5a05eab386c8943ba6bdee8d5efeb448aa0c 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
>   src/slave/constants.hpp 449c8cd9f43f71b4612023eb463969e9db0bc960 
>   src/slave/flags.hpp 4236b7fe40b9aa26d57b651be06a0e36037f65c6 
>   src/slave/flags.cpp 7164afef9c82a9bba0f137d7387c3569912b6599 
>   src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
>   src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
>   src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
>   src/tests/cluster.hpp 39ca15e3cc2f4f2ac9b8a73482c2bd945b1824e3 
>   src/tests/cluster.cpp 7e488d28b7a0d3bef9cd76bf7df2de4822256ef6 
>   src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-13 Thread Adam B


> On April 12, 2016, 2:31 a.m., Adam B wrote:
> > docs/configuration.md, line 94
> > 
> >
> > Why would we need to support multiple authorizers? Would a particular 
> > request check two authorizers then and/or the results?
> > Or are we somehow using one authorizer for some things and another for 
> > others? This could be explainable for the master's framework authz vs. http 
> > authz, but makes no sense for the agent.
> 
> Alexander Rojas wrote:
> That was an original requirement on the master, since the original 
> crammd5 authorizer was supposed to come also in the multiple version. I think 
> we can, not only remove it here, but also in the master. I don't think we 
> will move onto the multiple authorizers anytime soon.

+1, although I dread deprecating the plural flag in master. I could live with 
it being singular on agent and plural on master, at least for this release; but 
I guess we should start deprecations ASAP if we have to wait 6mo before we can 
remove them.


- Adam


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


On April 8, 2016, 7:25 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 8, 2016, 7:25 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent authorization flags.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 309a5a05eab386c8943ba6bdee8d5efeb448aa0c 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
>   src/slave/constants.hpp 449c8cd9f43f71b4612023eb463969e9db0bc960 
>   src/slave/flags.hpp 4236b7fe40b9aa26d57b651be06a0e36037f65c6 
>   src/slave/flags.cpp 7164afef9c82a9bba0f137d7387c3569912b6599 
>   src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
>   src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
>   src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
>   src/tests/cluster.hpp 39ca15e3cc2f4f2ac9b8a73482c2bd945b1824e3 
>   src/tests/cluster.cpp 7e488d28b7a0d3bef9cd76bf7df2de4822256ef6 
>   src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-12 Thread Alexander Rojas


> On April 12, 2016, 11:31 a.m., Adam B wrote:
> > docs/configuration.md, line 94
> > 
> >
> > Why would we need to support multiple authorizers? Would a particular 
> > request check two authorizers then and/or the results?
> > Or are we somehow using one authorizer for some things and another for 
> > others? This could be explainable for the master's framework authz vs. http 
> > authz, but makes no sense for the agent.

That was an original requirement on the master, since the original crammd5 
authorizer was supposed to come also in the multiple version. I think we can, 
not only remove it here, but also in the master. I don't think we will move 
onto the multiple authorizers anytime soon.


- Alexander


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


On April 8, 2016, 4:25 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 8, 2016, 4:25 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent authorization flags.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 309a5a05eab386c8943ba6bdee8d5efeb448aa0c 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
>   src/slave/constants.hpp 449c8cd9f43f71b4612023eb463969e9db0bc960 
>   src/slave/flags.hpp 4236b7fe40b9aa26d57b651be06a0e36037f65c6 
>   src/slave/flags.cpp 7164afef9c82a9bba0f137d7387c3569912b6599 
>   src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
>   src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
>   src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
>   src/tests/cluster.hpp 39ca15e3cc2f4f2ac9b8a73482c2bd945b1824e3 
>   src/tests/cluster.cpp 7e488d28b7a0d3bef9cd76bf7df2de4822256ef6 
>   src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-12 Thread Joerg Schad


> On April 12, 2016, 9:31 a.m., Adam B wrote:
> > src/slave/flags.cpp, lines 446-449
> > 
> >
> > I'd rather have no example than a confusing/invalid example. We can add 
> > an example after we have at least one ACL

+1


> On April 12, 2016, 9:31 a.m., Adam B wrote:
> > src/slave/main.cpp, line 301
> > 
> >
> > It seems that by naming the flag `authorizers`, you've already decided 
> > that we want to add support for multiple authorizers

Isn't that also implied by the parsing logic above/in the master/main.cpp?


> On April 12, 2016, 9:31 a.m., Adam B wrote:
> > src/tests/cluster.hpp, line 146
> > 
> >
> > Why add this here instead of at the end? Do you expect 'authorizer' to 
> > be a more populer parameter than the ones after it?

This also causes that you have to touch a lot of tests


- Joerg


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


On April 8, 2016, 2:25 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 8, 2016, 2:25 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent authorization flags.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 309a5a05eab386c8943ba6bdee8d5efeb448aa0c 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
>   src/slave/constants.hpp 449c8cd9f43f71b4612023eb463969e9db0bc960 
>   src/slave/flags.hpp 4236b7fe40b9aa26d57b651be06a0e36037f65c6 
>   src/slave/flags.cpp 7164afef9c82a9bba0f137d7387c3569912b6599 
>   src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
>   src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
>   src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
>   src/tests/cluster.hpp 39ca15e3cc2f4f2ac9b8a73482c2bd945b1824e3 
>   src/tests/cluster.cpp 7e488d28b7a0d3bef9cd76bf7df2de4822256ef6 
>   src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-12 Thread Joerg Schad

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




docs/configuration.md (line 867)


This seems not relevant at the agent (register framework). If it is just 
used a placeholder please remove.



src/local/local.cpp (line 241)


Add a comment that this (master flag) is currently also used for the agent 
authorizer.



src/local/local.cpp (line 397)


Could we add a comment here that this is currently the same authorizer as 
the master i.e. is already checked byt the Master code?



src/slave/flags.hpp (line 102)


Do we have a particular order here?



src/slave/main.cpp (line 26)


This should go above mesos/master/detector or?



src/slave/main.cpp (line 293)


The blank lines between calls are inconsistent with 
src/tests/cluster.cpp below.



src/tests/cluster.cpp (line 395)


blank lines inconsistent with main.cpp above.



src/tests/cluster.cpp (line 406)


We only create an authorizer if the acls flags are set, correct?
In the case of no acl flags it means the local authorizer permits 
everything and hence we don't have to specify it (i.e. authorizer((None())). 
Ist that correct?

In that case maybe "Creating default '" << authorizerName << "' authorizer 
as acls flags are are set"?


- Joerg Schad


On April 8, 2016, 2:25 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 8, 2016, 2:25 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent authorization flags.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 309a5a05eab386c8943ba6bdee8d5efeb448aa0c 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
>   src/slave/constants.hpp 449c8cd9f43f71b4612023eb463969e9db0bc960 
>   src/slave/flags.hpp 4236b7fe40b9aa26d57b651be06a0e36037f65c6 
>   src/slave/flags.cpp 7164afef9c82a9bba0f137d7387c3569912b6599 
>   src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
>   src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
>   src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
>   src/tests/cluster.hpp 39ca15e3cc2f4f2ac9b8a73482c2bd945b1824e3 
>   src/tests/cluster.cpp 7e488d28b7a0d3bef9cd76bf7df2de4822256ef6 
>   src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-12 Thread Adam B

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



Looks nice and clean. Three main concerns:
1. Why support multiple authorizers?
2. Don't show confusing/invalid examples
3. How will we test this? Do we need to add authz to at least one endpoint?


docs/configuration.md (line 94)


Why would we need to support multiple authorizers? Would a particular 
request check two authorizers then and/or the results?
Or are we somehow using one authorizer for some things and another for 
others? This could be explainable for the master's framework authz vs. http 
authz, but makes no sense for the agent.



docs/configuration.md (line 869)


None of these examples apply to an agent. Maybe we need to implement an ACL 
(e.g. GET_ENDPOINT_WITH_PATH "/flags") before we can provide any example ACLs 
for the agent.



src/slave/flags.cpp (lines 446 - 449)


I'd rather have no example than a confusing/invalid example. We can add an 
example after we have at least one ACL



src/slave/flags.cpp (line 777)


I still can't imagine an agent with multiple authorizers.



src/slave/main.cpp (lines 22 - 26)


Pretty sure `mesos/authorizer/*` goes before `mesos/master/*` alphabetically



src/slave/main.cpp (line 301)


It seems that by naming the flag `authorizers`, you've already decided that 
we want to add support for multiple authorizers



src/tests/cluster.hpp (line 146)


Why add this here instead of at the end? Do you expect 'authorizer' to be a 
more populer parameter than the ones after it?


- Adam B


On April 8, 2016, 7:25 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 8, 2016, 7:25 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent authorization flags.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 309a5a05eab386c8943ba6bdee8d5efeb448aa0c 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
>   src/slave/constants.hpp 449c8cd9f43f71b4612023eb463969e9db0bc960 
>   src/slave/flags.hpp 4236b7fe40b9aa26d57b651be06a0e36037f65c6 
>   src/slave/flags.cpp 7164afef9c82a9bba0f137d7387c3569912b6599 
>   src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
>   src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
>   src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
>   src/tests/cluster.hpp 39ca15e3cc2f4f2ac9b8a73482c2bd945b1824e3 
>   src/tests/cluster.cpp 7e488d28b7a0d3bef9cd76bf7df2de4822256ef6 
>   src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-08 Thread Mesos ReviewBot

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



Patch looks great!

Reviews applied: [45922]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On April 8, 2016, 2:25 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 8, 2016, 2:25 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent authorization flags.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 309a5a05eab386c8943ba6bdee8d5efeb448aa0c 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
>   src/slave/constants.hpp 449c8cd9f43f71b4612023eb463969e9db0bc960 
>   src/slave/flags.hpp 4236b7fe40b9aa26d57b651be06a0e36037f65c6 
>   src/slave/flags.cpp 7164afef9c82a9bba0f137d7387c3569912b6599 
>   src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
>   src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
>   src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
>   src/tests/cluster.hpp 39ca15e3cc2f4f2ac9b8a73482c2bd945b1824e3 
>   src/tests/cluster.cpp 7e488d28b7a0d3bef9cd76bf7df2de4822256ef6 
>   src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-08 Thread Jan Schlicht

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

(Updated April 8, 2016, 4:25 p.m.)


Review request for mesos, Adam B and Alexander Rojas.


Changes
---

Add a comment explaining the current ACLs example.


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


Repository: mesos


Description (updated)
---

Added agent authorization flags.


Diffs (updated)
-

  docs/configuration.md 309a5a05eab386c8943ba6bdee8d5efeb448aa0c 
  src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
  src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
  src/slave/constants.hpp 449c8cd9f43f71b4612023eb463969e9db0bc960 
  src/slave/flags.hpp 4236b7fe40b9aa26d57b651be06a0e36037f65c6 
  src/slave/flags.cpp 7164afef9c82a9bba0f137d7387c3569912b6599 
  src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
  src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
  src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
  src/tests/cluster.hpp 39ca15e3cc2f4f2ac9b8a73482c2bd945b1824e3 
  src/tests/cluster.cpp 7e488d28b7a0d3bef9cd76bf7df2de4822256ef6 
  src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 

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


Testing
---

make check


Thanks,

Jan Schlicht



Re: Review Request 45922: Added agent authorization flags.

2016-04-08 Thread Alexander Rojas

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




src/slave/flags.cpp (lines 459 - 491)


Can you at least add a note mentioned that the example is not relevant to 
the agent but show how the format may look like.


- Alexander Rojas


On April 8, 2016, 11:26 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 8, 2016, 11:26 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 309a5a05eab386c8943ba6bdee8d5efeb448aa0c 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
>   src/slave/constants.hpp 449c8cd9f43f71b4612023eb463969e9db0bc960 
>   src/slave/flags.hpp 4236b7fe40b9aa26d57b651be06a0e36037f65c6 
>   src/slave/flags.cpp 7164afef9c82a9bba0f137d7387c3569912b6599 
>   src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
>   src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
>   src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
>   src/tests/cluster.hpp 39ca15e3cc2f4f2ac9b8a73482c2bd945b1824e3 
>   src/tests/cluster.cpp 7e488d28b7a0d3bef9cd76bf7df2de4822256ef6 
>   src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-08 Thread Mesos ReviewBot

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



Patch looks great!

Reviews applied: [45922]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On April 8, 2016, 9:26 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 8, 2016, 9:26 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 309a5a05eab386c8943ba6bdee8d5efeb448aa0c 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
>   src/slave/constants.hpp 449c8cd9f43f71b4612023eb463969e9db0bc960 
>   src/slave/flags.hpp 4236b7fe40b9aa26d57b651be06a0e36037f65c6 
>   src/slave/flags.cpp 7164afef9c82a9bba0f137d7387c3569912b6599 
>   src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
>   src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
>   src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
>   src/tests/cluster.hpp 39ca15e3cc2f4f2ac9b8a73482c2bd945b1824e3 
>   src/tests/cluster.cpp 7e488d28b7a0d3bef9cd76bf7df2de4822256ef6 
>   src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-08 Thread Jan Schlicht

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

(Updated April 8, 2016, 11:26 a.m.)


Review request for mesos, Adam B and Alexander Rojas.


Changes
---

Added flags documentation in `docs/configuration.md`.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  docs/configuration.md 309a5a05eab386c8943ba6bdee8d5efeb448aa0c 
  src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
  src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
  src/slave/constants.hpp 449c8cd9f43f71b4612023eb463969e9db0bc960 
  src/slave/flags.hpp 4236b7fe40b9aa26d57b651be06a0e36037f65c6 
  src/slave/flags.cpp 7164afef9c82a9bba0f137d7387c3569912b6599 
  src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
  src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
  src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
  src/tests/cluster.hpp 39ca15e3cc2f4f2ac9b8a73482c2bd945b1824e3 
  src/tests/cluster.cpp 7e488d28b7a0d3bef9cd76bf7df2de4822256ef6 
  src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 

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


Testing
---

make check


Thanks,

Jan Schlicht



Re: Review Request 45922: Added agent authorization flags.

2016-04-08 Thread Joerg Schad

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




src/slave/flags.cpp (line 446)


Please also add this to 
https://github.com/apache/mesos/blob/master/docs/configuration.md


- Joerg Schad


On April 8, 2016, 8:52 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 8, 2016, 8:52 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
>   src/slave/constants.hpp 449c8cd9f43f71b4612023eb463969e9db0bc960 
>   src/slave/flags.hpp 4236b7fe40b9aa26d57b651be06a0e36037f65c6 
>   src/slave/flags.cpp 7164afef9c82a9bba0f137d7387c3569912b6599 
>   src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
>   src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
>   src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
>   src/tests/cluster.hpp 39ca15e3cc2f4f2ac9b8a73482c2bd945b1824e3 
>   src/tests/cluster.cpp 7e488d28b7a0d3bef9cd76bf7df2de4822256ef6 
>   src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 45922: Added agent authorization flags.

2016-04-08 Thread Jan Schlicht

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




src/slave/flags.cpp (lines 459 - 491)


The agent ACLs will probably change, but because they aren't defined/used 
yet, I've kept the example the same as in `master/flags.cpp`, but could also 
remove it completely.


- Jan Schlicht


On April 8, 2016, 10:52 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 8, 2016, 10:52 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
>   src/slave/constants.hpp 449c8cd9f43f71b4612023eb463969e9db0bc960 
>   src/slave/flags.hpp 4236b7fe40b9aa26d57b651be06a0e36037f65c6 
>   src/slave/flags.cpp 7164afef9c82a9bba0f137d7387c3569912b6599 
>   src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
>   src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
>   src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
>   src/tests/cluster.hpp 39ca15e3cc2f4f2ac9b8a73482c2bd945b1824e3 
>   src/tests/cluster.cpp 7e488d28b7a0d3bef9cd76bf7df2de4822256ef6 
>   src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>