You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jan Schlicht <ja...@mesosphere.io> on 2016/04/18 11:53:00 UTC
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/
-----------------------------------------------------------
(Updated April 18, 2016, 11:52 a.m.)
Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
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
include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
src/Makefile.am ec855263d620e4723c8ba9cd056c40a3a2e9ca99
src/authorizer/local/authorizer.cpp c744a16041c2466d3839a37fbee2bf86887bf4e1
src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
src/slave/http.cpp 3908e33ed5b233387790276f6f5d884452087d2c
src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1
src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb
src/tests/slave_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/46203/diff/
Testing
-------
make check (SlaveAuthorizationTest.AuthorizeFlagsEndpointWithoutPrincipal fails though it shouldn't, tests sometimes segfault)
Thanks,
Jan Schlicht
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review129507
-----------------------------------------------------------
src/slave/http.cpp (lines 358 - 359)
<https://reviews.apache.org/r/46203/#comment192944>
for readability in this case you could indent to the parenthesis on the `.then(`.
It is allowed by the styleguide if you ask yourself
src/slave/http.cpp (lines 360 - 362)
<https://reviews.apache.org/r/46203/#comment192942>
Formatting is wrong
src/slave/http.cpp (lines 658 - 659)
<https://reviews.apache.org/r/46203/#comment192943>
Whitespace between these two lines (see c++ styleguide)
- Alexander Rojas
On April 18, 2016, 2:53 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 18, 2016, 2:53 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md cd9733002a0940f44edd4e56593a0ca3fe9f77f5
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am ec855263d620e4723c8ba9cd056c40a3a2e9ca99
> src/authorizer/local/authorizer.cpp c744a16041c2466d3839a37fbee2bf86887bf4e1
> src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
> src/slave/http.cpp 3908e33ed5b233387790276f6f5d884452087d2c
> src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1
> src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 26, 2016, 4:01 p.m., Benjamin Bannier wrote:
> > src/slave/http.cpp, line 362
> > <https://reviews.apache.org/r/46203/diff/15/?file=1361588#file1361588line362>
> >
> > You assume that `slave(XYZ)/flags` will only receive `GET` requests, but I think it would make more sense to pass the method of the `Request` down into `authorizeEndpoint` so we do not need make this decision here (in some places it is directly tied to the handler, but the way the ACLs are designed you mix in concerns of the `Authorizer`).
That's a good point! The current routing in libprocess doesn't dispatch based on the request method. The functions that are routed to have to take care of that and unfortunately most of them don't check with what method they've been called. E.g. if '/flags' is requested with a POST, it will return the same as if requested with a GET -- I'd expect it to drop the POST request as unsupported.
So my assumption is here that while `/flags` may receive other requests than `GET`, it will always act like it's a `GET` request and therefore the authz should behave like it's a `GET`. That's a "not my department" approach to this issue. Your suggestion is "if it's a `POST` authorize it like a `POST`". I think we can go with that.
> On April 26, 2016, 4:01 p.m., Benjamin Bannier wrote:
> > src/slave/http.cpp, line 787
> > <https://reviews.apache.org/r/46203/diff/15/?file=1361588#file1361588line787>
> >
> > I really like that we use a dedicated `enum` in the switch below, but dislike how users of this function need to manually translate from a `std::string` to a `Method`.
> >
> > What about passing some `std::string` like stored inside the `Request` here (feel free to add a `TODO` to `Request` to strongly type `method`), and then doing the conversion in here? An unknown method would result in e.g., a `Failure`.
+1 for making `Request::method` strongly typed. To implement it right now I would remove the `Method` enum and compare the `Request::method` string directly with string of supported methods. I'd rather `switch` over the values of an enum, but the code will stay simple and readable by the string-string comparison.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130610
-----------------------------------------------------------
On April 26, 2016, 2:51 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 26, 2016, 2:51 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On April 26, 2016, 2:01 p.m., Benjamin Bannier wrote:
> > src/slave/http.cpp, line 787
> > <https://reviews.apache.org/r/46203/diff/15/?file=1361588#file1361588line787>
> >
> > I really like that we use a dedicated `enum` in the switch below, but dislike how users of this function need to manually translate from a `std::string` to a `Method`.
> >
> > What about passing some `std::string` like stored inside the `Request` here (feel free to add a `TODO` to `Request` to strongly type `method`), and then doing the conversion in here? An unknown method would result in e.g., a `Failure`.
>
> Jan Schlicht wrote:
> +1 for making `Request::method` strongly typed. To implement it right now I would remove the `Method` enum and compare the `Request::method` string directly with string of supported methods. I'd rather `switch` over the values of an enum, but the code will stay simple and readable by the string-string comparison.
Yep, let's do it in generally.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130610
-----------------------------------------------------------
On April 26, 2016, 3:27 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 26, 2016, 3:27 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130610
-----------------------------------------------------------
src/slave/http.cpp (line 362)
<https://reviews.apache.org/r/46203/#comment194427>
You assume that `slave(XYZ)/flags` will only receive `GET` requests, but I think it would make more sense to pass the method of the `Request` down into `authorizeEndpoint` so we do not need make this decision here (in some places it is directly tied to the handler, but the way the ACLs are designed you mix in concerns of the `Authorizer`).
src/slave/http.cpp (line 787)
<https://reviews.apache.org/r/46203/#comment194428>
I really like that we use a dedicated `enum` in the switch below, but dislike how users of this function need to manually translate from a `std::string` to a `Method`.
What about passing some `std::string` like stored inside the `Request` here (feel free to add a `TODO` to `Request` to strongly type `method`), and then doing the conversion in here? An unknown method would result in e.g., a `Failure`.
- Benjamin Bannier
On April 26, 2016, 2:51 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 26, 2016, 2:51 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Adam B <ad...@mesosphere.io>.
> On April 27, 2016, 1:24 a.m., Alexander Rojas wrote:
> > src/slave/http.cpp, line 798
> > <https://reviews.apache.org/r/46203/diff/18/?file=1362275#file1362275line798>
> >
> > This looks like a bad pattern here, you could use instead `slave->self().id == pathComponents[0]`.
> >
> > Remember that `!(p && q) == !p || !q`. It might improve readability in this expression.
>
> Jan Schlicht wrote:
> `!(p && q)` is the more readable option here (compare rev 18 to rev 17), because I'd have to add an additional check if I'd want to do `!p || !q`.
>
> Adam B wrote:
> Not true. Due to boolean short-circuiting, `if (pathComponents.size() != 2u)` (e.g. `0`), then we skip the second half of the `||` and go right to `return Failure`. Otherwise, `pathComponents.size() == 2u`, so it's ok to call `strings::startsWith(pathComponents[0], "slave(")`.
> The `pathComponents.size() > 0` is unnecessary.
FWIW, since it's the same number of lines, I prefer the `!p || !q` version, because it means I see the `!` on each line and don't have to guess about where the `!` ends.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130731
-----------------------------------------------------------
On April 27, 2016, 1:54 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 27, 2016, 1:54 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 2796a812b72f2089999b1ae2d65a4ba843b50d70
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 27, 2016, 10:24 a.m., Alexander Rojas wrote:
> > src/slave/http.cpp, line 798
> > <https://reviews.apache.org/r/46203/diff/18/?file=1362275#file1362275line798>
> >
> > This looks like a bad pattern here, you could use instead `slave->self().id == pathComponents[0]`.
> >
> > Remember that `!(p && q) == !p || !q`. It might improve readability in this expression.
>
> Jan Schlicht wrote:
> `!(p && q)` is the more readable option here (compare rev 18 to rev 17), because I'd have to add an additional check if I'd want to do `!p || !q`.
>
> Adam B wrote:
> Not true. Due to boolean short-circuiting, `if (pathComponents.size() != 2u)` (e.g. `0`), then we skip the second half of the `||` and go right to `return Failure`. Otherwise, `pathComponents.size() == 2u`, so it's ok to call `strings::startsWith(pathComponents[0], "slave(")`.
> The `pathComponents.size() > 0` is unnecessary.
>
> Adam B wrote:
> FWIW, since it's the same number of lines, I prefer the `!p || !q` version, because it means I see the `!` on each line and don't have to guess about where the `!` ends.
My bad, forgot about the short-circuiting of `||` here. Yes, the `!p || !q` is much better, will change it.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130731
-----------------------------------------------------------
On April 27, 2016, 10:54 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 27, 2016, 10:54 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 2796a812b72f2089999b1ae2d65a4ba843b50d70
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 27, 2016, 10:24 a.m., Alexander Rojas wrote:
> > src/slave/http.cpp, line 798
> > <https://reviews.apache.org/r/46203/diff/18/?file=1362275#file1362275line798>
> >
> > This looks like a bad pattern here, you could use instead `slave->self().id == pathComponents[0]`.
> >
> > Remember that `!(p && q) == !p || !q`. It might improve readability in this expression.
`!(p && q)` is the more readable option here (compare rev 18 to rev 17), because I'd have to add an additional check if I'd want to do `!p || !q`.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130731
-----------------------------------------------------------
On April 26, 2016, 8:29 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 26, 2016, 8:29 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 2796a812b72f2089999b1ae2d65a4ba843b50d70
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Adam B <ad...@mesosphere.io>.
> On April 27, 2016, 1:24 a.m., Alexander Rojas wrote:
> > src/slave/http.cpp, line 798
> > <https://reviews.apache.org/r/46203/diff/18/?file=1362275#file1362275line798>
> >
> > This looks like a bad pattern here, you could use instead `slave->self().id == pathComponents[0]`.
> >
> > Remember that `!(p && q) == !p || !q`. It might improve readability in this expression.
>
> Jan Schlicht wrote:
> `!(p && q)` is the more readable option here (compare rev 18 to rev 17), because I'd have to add an additional check if I'd want to do `!p || !q`.
Not true. Due to boolean short-circuiting, `if (pathComponents.size() != 2u)` (e.g. `0`), then we skip the second half of the `||` and go right to `return Failure`. Otherwise, `pathComponents.size() == 2u`, so it's ok to call `strings::startsWith(pathComponents[0], "slave(")`.
The `pathComponents.size() > 0` is unnecessary.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130731
-----------------------------------------------------------
On April 27, 2016, 1:54 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 27, 2016, 1:54 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 2796a812b72f2089999b1ae2d65a4ba843b50d70
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130731
-----------------------------------------------------------
src/slave/http.cpp (line 798)
<https://reviews.apache.org/r/46203/#comment194601>
This looks like a bad pattern here, you could use instead `slave->self().id == pathComponents[0]`.
Remember that `!(p && q) == !p || !q`. It might improve readability in this expression.
- Alexander Rojas
On April 26, 2016, 8:29 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 26, 2016, 8:29 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 2796a812b72f2089999b1ae2d65a4ba843b50d70
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 27, 2016, 12:09 p.m., Adam B wrote:
> > src/tests/slave_authorization_tests.cpp, line 93
> > <https://reviews.apache.org/r/46203/diff/20/?file=1363047#file1363047line93>
> >
> > Should we add a test for "/flags/"?
See answer above. Requesting `/flags/` would result in a 404 because it isn't routed to `Slave::Http:flags`.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130749
-----------------------------------------------------------
On April 27, 2016, 11:20 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 27, 2016, 11:20 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 2796a812b72f2089999b1ae2d65a4ba843b50d70
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130749
-----------------------------------------------------------
And a couple more comments on the tests
src/tests/slave_authorization_tests.cpp (line 93)
<https://reviews.apache.org/r/46203/#comment194628>
Should we add a test for "/flags/"?
src/tests/slave_authorization_tests.cpp (lines 133 - 136)
<https://reviews.apache.org/r/46203/#comment194631>
Could you please run the tests with --gtest_shuffle so we know that we're ok no matter what order these tests execute in?
src/tests/slave_authorization_tests.cpp (line 153)
<https://reviews.apache.org/r/46203/#comment194632>
s/slaveFlags.acls.get()/acls/? And then maybe this fits on one line?
- Adam B
On April 27, 2016, 2:20 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 27, 2016, 2:20 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 2796a812b72f2089999b1ae2d65a4ba843b50d70
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 27, 2016, 11:29 a.m., Adam B wrote:
> > src/authorizer/local/authorizer.cpp, line 213
> > <https://reviews.apache.org/r/46203/diff/18/?file=1362273#file1362273line213>
> >
> > Does this only match exact strings, or endpoints nested under this path as well?
> > For example, could I set an ACL that allows Dan to access "/monitor", and then he's implicitly allowed to access "/monitor/statistics"?
> > Maybe not necessary for LocalAuthorizer MVP, but seems valuable.
>
> Jan Schlicht wrote:
> It only matches exact strings. Doing matching of "layers" like you're suggesting above would require more effort and IMO shouldn't be part of this patch.
>
> Jan Schlicht wrote:
> Had a discussion with @arojas about this. It seems not that hard to implement, but I'm still confident that we shouldn't do it in this patch but in a separate one. I'll add a TODO in the file.
>
> Alexander Rukletsov wrote:
> Please file a JIRA as well.
JIRA: [MESOS-5299](https://issues.apache.org/jira/browse/MESOS-5299)
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130736
-----------------------------------------------------------
On April 27, 2016, 4:32 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 27, 2016, 4:32 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 2796a812b72f2089999b1ae2d65a4ba843b50d70
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/authorization_tests.cpp d4ef0f94c86b1287c98671c5d478ce6ac3d90636
> src/tests/mesos.hpp 78edab89e6c13ff0892b2f3b5cb6886f08b02c82
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> ./bin/mesos-tests.sh --gtest_shuffle
> ./bin/mesos-tests.sh --gtest_shuffle --gtest_repeat=100 --gtest_filter=*SlaveAuthorization*
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On April 27, 2016, 9:29 a.m., Adam B wrote:
> > src/authorizer/local/authorizer.cpp, line 213
> > <https://reviews.apache.org/r/46203/diff/18/?file=1362273#file1362273line213>
> >
> > Does this only match exact strings, or endpoints nested under this path as well?
> > For example, could I set an ACL that allows Dan to access "/monitor", and then he's implicitly allowed to access "/monitor/statistics"?
> > Maybe not necessary for LocalAuthorizer MVP, but seems valuable.
>
> Jan Schlicht wrote:
> It only matches exact strings. Doing matching of "layers" like you're suggesting above would require more effort and IMO shouldn't be part of this patch.
>
> Jan Schlicht wrote:
> Had a discussion with @arojas about this. It seems not that hard to implement, but I'm still confident that we shouldn't do it in this patch but in a separate one. I'll add a TODO in the file.
Please file a JIRA as well.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130736
-----------------------------------------------------------
On April 27, 2016, 2:32 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 27, 2016, 2:32 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 2796a812b72f2089999b1ae2d65a4ba843b50d70
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/authorization_tests.cpp d4ef0f94c86b1287c98671c5d478ce6ac3d90636
> src/tests/mesos.hpp 78edab89e6c13ff0892b2f3b5cb6886f08b02c82
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> ./bin/mesos-tests.sh --gtest_shuffle
> ./bin/mesos-tests.sh --gtest_shuffle --gtest_repeat=100 --gtest_filter=*SlaveAuthorization*
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 27, 2016, 11:29 a.m., Adam B wrote:
> > src/authorizer/local/authorizer.cpp, line 213
> > <https://reviews.apache.org/r/46203/diff/18/?file=1362273#file1362273line213>
> >
> > Does this only match exact strings, or endpoints nested under this path as well?
> > For example, could I set an ACL that allows Dan to access "/monitor", and then he's implicitly allowed to access "/monitor/statistics"?
> > Maybe not necessary for LocalAuthorizer MVP, but seems valuable.
It only matches exact strings. Doing matching of "layers" like you're suggesting above would require more effort and IMO shouldn't be part of this patch.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130736
-----------------------------------------------------------
On April 27, 2016, 11:20 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 27, 2016, 11:20 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 2796a812b72f2089999b1ae2d65a4ba843b50d70
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 27, 2016, 11:29 a.m., Adam B wrote:
> > src/tests/slave_authorization_tests.cpp, lines 58-66
> > <https://reviews.apache.org/r/46203/diff/18/?file=1362277#file1362277line58>
> >
> > Isn't this literally the same code in authorization_tests.cpp? Can we factor this into a common header?
> > (And fix the `Parameter *` wherever it lands.)
I've moved it into `tests/mesos.hpp` as that seems to be the most suitable place.
> On April 27, 2016, 11:29 a.m., Adam B wrote:
> > src/slave/http.cpp, lines 374-377
> > <https://reviews.apache.org/r/46203/diff/18/?file=1362275#file1362275line374>
> >
> > Yikes! A 20 char indent is intense, and this wrapping seems extreme. Can we do `using flags::Flag` or `s/slaveFlags/flags/` or even split it out into a named function rather than an anonymous inline lambda? Something to make this fit on one line
>
> Jan Schlicht wrote:
> I'll use a continuation.
Because we can't capture `this`, the continuation needs to be static. @bbannier opened [MESOS-5293](https://issues.apache.org/jira/browse/MESOS-5293) to eventually make things consistent.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130736
-----------------------------------------------------------
On April 27, 2016, 3:21 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 27, 2016, 3:21 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 2796a812b72f2089999b1ae2d65a4ba843b50d70
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/authorization_tests.cpp d4ef0f94c86b1287c98671c5d478ce6ac3d90636
> src/tests/mesos.hpp 78edab89e6c13ff0892b2f3b5cb6886f08b02c82
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> ./bin/mesos-tests.sh --gtest_shuffle
> ./bin/mesos-tests.sh --gtest_shuffle --gtest_repeat=100 --gtest_filter=*SlaveAuthorization*
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 27, 2016, 11:29 a.m., Adam B wrote:
> > src/authorizer/local/authorizer.cpp, line 213
> > <https://reviews.apache.org/r/46203/diff/18/?file=1362273#file1362273line213>
> >
> > Does this only match exact strings, or endpoints nested under this path as well?
> > For example, could I set an ACL that allows Dan to access "/monitor", and then he's implicitly allowed to access "/monitor/statistics"?
> > Maybe not necessary for LocalAuthorizer MVP, but seems valuable.
>
> Jan Schlicht wrote:
> It only matches exact strings. Doing matching of "layers" like you're suggesting above would require more effort and IMO shouldn't be part of this patch.
Had a discussion with @arojas about this. It seems not that hard to implement, but I'm still confident that we shouldn't do it in this patch but in a separate one. I'll add a TODO in the file.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130736
-----------------------------------------------------------
On April 27, 2016, 3:21 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 27, 2016, 3:21 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 2796a812b72f2089999b1ae2d65a4ba843b50d70
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/authorization_tests.cpp d4ef0f94c86b1287c98671c5d478ce6ac3d90636
> src/tests/mesos.hpp 78edab89e6c13ff0892b2f3b5cb6886f08b02c82
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> ./bin/mesos-tests.sh --gtest_shuffle
> ./bin/mesos-tests.sh --gtest_shuffle --gtest_repeat=100 --gtest_filter=*SlaveAuthorization*
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 27, 2016, 11:29 a.m., Adam B wrote:
> > src/slave/http.cpp, line 795
> > <https://reviews.apache.org/r/46203/diff/18/?file=1362275#file1362275line795>
> >
> > What happens if the request is for `/slave(0)/flags/`? Does the trailing slash get removed before comparing against the ACL, or will it fail to match?
> > This should be clearly documented.
Request for `/slave(0)/flags/` wouldn't get routed through to the endpoint and therefore this function won't be called. So I don't think we need to cover that case here.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130736
-----------------------------------------------------------
On April 27, 2016, 11:20 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 27, 2016, 11:20 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 2796a812b72f2089999b1ae2d65a4ba843b50d70
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 27, 2016, 11:29 a.m., Adam B wrote:
> > src/slave/http.cpp, lines 374-377
> > <https://reviews.apache.org/r/46203/diff/18/?file=1362275#file1362275line374>
> >
> > Yikes! A 20 char indent is intense, and this wrapping seems extreme. Can we do `using flags::Flag` or `s/slaveFlags/flags/` or even split it out into a named function rather than an anonymous inline lambda? Something to make this fit on one line
I'll use a continuation.
> On April 27, 2016, 11:29 a.m., Adam B wrote:
> > src/tests/slave_authorization_tests.cpp, line 11
> > <https://reviews.apache.org/r/46203/diff/18/?file=1362277#file1362277line11>
> >
> > s/writiDng/writing/
:|, how did that sneak in?
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130736
-----------------------------------------------------------
On April 27, 2016, 11:20 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 27, 2016, 11:20 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 2796a812b72f2089999b1ae2d65a4ba843b50d70
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130736
-----------------------------------------------------------
Mostly minor nits and a couple of questions about url->acl matching.
src/slave/http.cpp (line 785)
<https://reviews.apache.org/r/46203/#comment194604>
`s/request_/httpRequest/`, for readability? Those are two very different kinds of requests.
src/slave/http.cpp (lines 813 - 816)
<https://reviews.apache.org/r/46203/#comment194607>
Can you put this further down, just above the call to `authorized(request)`? I'd prefer to keep the request construction logic together, where we set subject/action/object
src/slave/slave.hpp (line 473)
<https://reviews.apache.org/r/46203/#comment194608>
s/method/`method`/ so it's obvious that you're talking about an actual variable/protobuf/field named 'method'
src/authorizer/local/authorizer.cpp (line 213)
<https://reviews.apache.org/r/46203/#comment194609>
Does this only match exact strings, or endpoints nested under this path as well?
For example, could I set an ACL that allows Dan to access "/monitor", and then he's implicitly allowed to access "/monitor/statistics"?
Maybe not necessary for LocalAuthorizer MVP, but seems valuable.
src/slave/flags.cpp (line 464)
<https://reviews.apache.org/r/46203/#comment194610>
Update these to match configuration.md
src/slave/http.cpp (lines 374 - 377)
<https://reviews.apache.org/r/46203/#comment194616>
Yikes! A 20 char indent is intense, and this wrapping seems extreme. Can we do `using flags::Flag` or `s/slaveFlags/flags/` or even split it out into a named function rather than an anonymous inline lambda? Something to make this fit on one line
src/slave/http.cpp (line 795)
<https://reviews.apache.org/r/46203/#comment194618>
What happens if the request is for `/slave(0)/flags/`? Does the trailing slash get removed before comparing against the ACL, or will it fail to match?
This should be clearly documented.
src/tests/slave_authorization_tests.cpp (line 11)
<https://reviews.apache.org/r/46203/#comment194619>
s/writiDng/writing/
src/tests/slave_authorization_tests.cpp (lines 58 - 66)
<https://reviews.apache.org/r/46203/#comment194623>
Isn't this literally the same code in authorization_tests.cpp? Can we factor this into a common header?
(And fix the `Parameter *` wherever it lands.)
- Adam B
On April 27, 2016, 2:20 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 27, 2016, 2:20 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 2796a812b72f2089999b1ae2d65a4ba843b50d70
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130788
-----------------------------------------------------------
Ship it!
I'll fix the remamining issues and commit it for you shortly.
src/slave/http.cpp (line 364)
<https://reviews.apache.org/r/46203/#comment194681>
Please indent by 4 spaces instead of 6.
src/slave/http.cpp (line 807)
<https://reviews.apache.org/r/46203/#comment194682>
s/request/authorizationRequest
src/slave/slave.hpp (lines 473 - 474)
<https://reviews.apache.org/r/46203/#comment194683>
Indent by 4 spaces.
src/slave/slave.hpp (lines 476 - 477)
<https://reviews.apache.org/r/46203/#comment194684>
This looks like a trade-off between a doxygen-styled comment and a common comment. Let's make it look like a normal comment.
src/tests/authorization_tests.cpp
<https://reviews.apache.org/r/46203/#comment194686>
It would have been great to have done this refactoring in a separate patch.
src/tests/mesos.hpp (line 739)
<https://reviews.apache.org/r/46203/#comment194687>
It seems unfortunate that we inline such helpers.
src/tests/slave_authorization_tests.cpp (lines 71 - 72)
<https://reviews.apache.org/r/46203/#comment194688>
Reformat to reduce jaggeddness.
src/tests/slave_authorization_tests.cpp (lines 75 - 76)
<https://reviews.apache.org/r/46203/#comment194689>
Ditto.
src/tests/slave_authorization_tests.cpp (line 82)
<https://reviews.apache.org/r/46203/#comment194690>
Extract endpoint as a constant.
src/tests/slave_authorization_tests.cpp (line 90)
<https://reviews.apache.org/r/46203/#comment194694>
Let's abolish slavery in variable names.
src/tests/slave_authorization_tests.cpp (line 133)
<https://reviews.apache.org/r/46203/#comment194693>
Ditto.
src/tests/slave_authorization_tests.cpp (line 146)
<https://reviews.apache.org/r/46203/#comment194695>
Ditto.
src/tests/slave_authorization_tests.cpp (lines 147 - 149)
<https://reviews.apache.org/r/46203/#comment194696>
Any reason you formatted this differently from the previous test?
- Alexander Rukletsov
On April 27, 2016, 2:32 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 27, 2016, 2:32 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 2796a812b72f2089999b1ae2d65a4ba843b50d70
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/authorization_tests.cpp d4ef0f94c86b1287c98671c5d478ce6ac3d90636
> src/tests/mesos.hpp 78edab89e6c13ff0892b2f3b5cb6886f08b02c82
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> ./bin/mesos-tests.sh --gtest_shuffle
> ./bin/mesos-tests.sh --gtest_shuffle --gtest_repeat=100 --gtest_filter=*SlaveAuthorization*
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/
-----------------------------------------------------------
(Updated April 27, 2016, 4:32 p.m.)
Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
Changes
-------
Added a TODO regarding ACL "subpath" handling.
Bugs: MESOS-5142
https://issues.apache.org/jira/browse/MESOS-5142
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
docs/configuration.md 2796a812b72f2089999b1ae2d65a4ba843b50d70
include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
src/tests/authorization_tests.cpp d4ef0f94c86b1287c98671c5d478ce6ac3d90636
src/tests/mesos.hpp 78edab89e6c13ff0892b2f3b5cb6886f08b02c82
src/tests/slave_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/46203/diff/
Testing
-------
./bin/mesos-tests.sh --gtest_shuffle
./bin/mesos-tests.sh --gtest_shuffle --gtest_repeat=100 --gtest_filter=*SlaveAuthorization*
Thanks,
Jan Schlicht
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/
-----------------------------------------------------------
(Updated April 27, 2016, 3:21 p.m.)
Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
Changes
-------
Addressed Adam's issues.
Bugs: MESOS-5142
https://issues.apache.org/jira/browse/MESOS-5142
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
docs/configuration.md 2796a812b72f2089999b1ae2d65a4ba843b50d70
include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
src/tests/authorization_tests.cpp d4ef0f94c86b1287c98671c5d478ce6ac3d90636
src/tests/mesos.hpp 78edab89e6c13ff0892b2f3b5cb6886f08b02c82
src/tests/slave_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/46203/diff/
Testing (updated)
-------
./bin/mesos-tests.sh --gtest_shuffle
./bin/mesos-tests.sh --gtest_shuffle --gtest_repeat=100 --gtest_filter=*SlaveAuthorization*
Thanks,
Jan Schlicht
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/
-----------------------------------------------------------
(Updated April 27, 2016, 11:20 a.m.)
Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
Changes
-------
Simplified the validation if-clause.
Bugs: MESOS-5142
https://issues.apache.org/jira/browse/MESOS-5142
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
docs/configuration.md 2796a812b72f2089999b1ae2d65a4ba843b50d70
include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
src/tests/slave_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/46203/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/
-----------------------------------------------------------
(Updated April 27, 2016, 10:54 a.m.)
Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
Changes
-------
Addressed Alexander's issues.
Bugs: MESOS-5142
https://issues.apache.org/jira/browse/MESOS-5142
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
docs/configuration.md 2796a812b72f2089999b1ae2d65a4ba843b50d70
include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
src/tests/slave_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/46203/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/
-----------------------------------------------------------
(Updated April 26, 2016, 8:29 p.m.)
Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
Changes
-------
Uses an equivalent but simpler validation if-clause.
Bugs: MESOS-5142
https://issues.apache.org/jira/browse/MESOS-5142
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
docs/configuration.md 2796a812b72f2089999b1ae2d65a4ba843b50d70
include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
src/tests/slave_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/46203/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/
-----------------------------------------------------------
(Updated April 26, 2016, 8:04 p.m.)
Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
Changes
-------
Uses `strings::tokenize` instead of `strings::split` and fixes a validation bug.
Bugs: MESOS-5142
https://issues.apache.org/jira/browse/MESOS-5142
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab
include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
src/tests/slave_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/46203/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On April 26, 2016, 3:40 p.m., Alexander Rukletsov wrote:
> > docs/configuration.md, line 899
> > <https://reviews.apache.org/r/46203/diff/15/?file=1361582#file1361582line899>
> >
> > Let's entertain our reader a bit : ). Instead of "a", we can use something like "padavan", and if you plan to introduce say `post_endpoints`, you can reuse "yoda".
>
> Jan Schlicht wrote:
> All the other examples for principals use "a" and "b", though. If we want to introduce better principal names, we should do it everywhere.
https://github.com/apache/mesos/blob/dc8b9a4abff74d1a2e33039836682bdfa7188f87/src/slave/flags.cpp#L764
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130609
-----------------------------------------------------------
On April 26, 2016, 6:29 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 26, 2016, 6:29 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 2796a812b72f2089999b1ae2d65a4ba843b50d70
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 26, 2016, 5:40 p.m., Alexander Rukletsov wrote:
> > src/slave/slave.hpp, lines 471-473
> > <https://reviews.apache.org/r/46203/diff/15/?file=1361589#file1361589line471>
> >
> > It feels like this class should be somewhere in a more general place. Moreover, libprocess' `Request` struct has the `method` field, which is a string. Any reason why you introduce an enum here?
Take a look at revision 16 please :D
> On April 26, 2016, 5:40 p.m., Alexander Rukletsov wrote:
> > docs/configuration.md, line 899
> > <https://reviews.apache.org/r/46203/diff/15/?file=1361582#file1361582line899>
> >
> > Let's entertain our reader a bit : ). Instead of "a", we can use something like "padavan", and if you plan to introduce say `post_endpoints`, you can reuse "yoda".
All the other examples for principals use "a" and "b", though. If we want to introduce better principal names, we should do it everywhere.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130609
-----------------------------------------------------------
On April 26, 2016, 5:27 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 26, 2016, 5:27 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130609
-----------------------------------------------------------
docs/configuration.md (line 899)
<https://reviews.apache.org/r/46203/#comment194426>
Let's entertain our reader a bit : ). Instead of "a", we can use something like "padavan", and if you plan to introduce say `post_endpoints`, you can reuse "yoda".
src/slave/http.cpp (lines 796 - 799)
<https://reviews.apache.org/r/46203/#comment194442>
Looks like `strings::tokenize()` suits better here.
src/slave/slave.hpp (lines 471 - 473)
<https://reviews.apache.org/r/46203/#comment194434>
It feels like this class should be somewhere in a more general place. Moreover, libprocess' `Request` struct has the `method` field, which is a string. Any reason why you introduce an enum here?
- Alexander Rukletsov
On April 26, 2016, 3:27 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 26, 2016, 3:27 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130655
-----------------------------------------------------------
src/slave/http.cpp (lines 797 - 799)
<https://reviews.apache.org/r/46203/#comment194479>
Nobody caught that one yet.
- Jan Schlicht
On April 26, 2016, 5:27 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 26, 2016, 5:27 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/
-----------------------------------------------------------
(Updated April 26, 2016, 5:27 p.m.)
Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
Changes
-------
Addressed Benjamin's issues (made `authorizeEndpoint` use a full `Request` as parameter and extracting `url` and `method` from that)
Bugs: MESOS-5142
https://issues.apache.org/jira/browse/MESOS-5142
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab
include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
src/tests/slave_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/46203/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/
-----------------------------------------------------------
(Updated April 26, 2016, 2:51 p.m.)
Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
Changes
-------
Add documentation for the `authorizeEndpoint` method, explaining endpoint extraction from URLs.
Bugs: MESOS-5142
https://issues.apache.org/jira/browse/MESOS-5142
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab
include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
src/tests/slave_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/46203/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/
-----------------------------------------------------------
(Updated April 26, 2016, 1:10 p.m.)
Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
Changes
-------
Addressed issues and added an enum to distinguish between authorization types.
Bugs: MESOS-5142
https://issues.apache.org/jira/browse/MESOS-5142
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab
include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
src/tests/slave_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/46203/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 26, 2016, 8:27 a.m., Adam B wrote:
> > src/slave/http.cpp, line 804
> > <https://reviews.apache.org/r/46203/diff/13/?file=1359473#file1359473line804>
> >
> > s/access/GET/ and shouldn't you be checking the Verb here, for when we have to authorize things other than GETs?
I've put the whole LOG(INFO) inside the switch-case for `GET`. That saves us from checking the verb, but introduces some slight code dublication.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130538
-----------------------------------------------------------
On April 26, 2016, 1:10 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 26, 2016, 1:10 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 26, 2016, 8:27 a.m., Adam B wrote:
> > src/slave/http.cpp, line 360
> > <https://reviews.apache.org/r/46203/diff/13/?file=1359473#file1359473line360>
> >
> > Should this perhaps be a `Shared<>`?
>
> Jan Schlicht wrote:
> I don't think so. `Shared<>` is about shared ownership, but the closure shouldn't own `Slave*`, but only use it. If we'd want to make sure that `slave_ != NULL` during the lifetime of the closure, we would have to use something like `std::weak_ptr`, managed by a shared pointer.
> I'd argue that because the `Slave` instance routes its HTTP requests to an `Http` instance, `slave_ != NULL` during the lifetime of the `Http` instance. Hence it's safe to use the pointer to `Slave` in the closure.
>
> Anyways, I'll change the closure to use `Flags` instead of `Slave*` as a parameter. The will be copied by value, object lifetime isn't a problem in that case.
Can we drop this?
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130538
-----------------------------------------------------------
On April 26, 2016, 1:10 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 26, 2016, 1:10 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 26, 2016, 8:27 a.m., Adam B wrote:
> > Looks great! I think we just need to pass the GET/POST verb into `authorizeEndpoint()` and fix the other minor nits, then we'll be ready to ship. Or maybe you can convince me that we don't need to add the verb until we actually have to authorize a non-GET verb (e.g. for maintenance primitives).
No, it makes sense to lay the foundation here by adding the verb.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130538
-----------------------------------------------------------
On April 25, 2016, 2:50 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 25, 2016, 2:50 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 26, 2016, 8:27 a.m., Adam B wrote:
> > src/slave/http.cpp, line 360
> > <https://reviews.apache.org/r/46203/diff/13/?file=1359473#file1359473line360>
> >
> > Should this perhaps be a `Shared<>`?
I don't think so. `Shared<>` is about shared ownership, but the closure shouldn't own `Slave*`, but only use it. If we'd want to make sure that `slave_ != NULL` during the lifetime of the closure, we would have to use something like `std::weak_ptr`, managed by a shared pointer.
I'd argue that because the `Slave` instance routes its HTTP requests to an `Http` instance, `slave_ != NULL` during the lifetime of the `Http` instance. Hence it's safe to use the pointer to `Slave` in the closure.
Anyways, I'll change the closure to use `Flags` instead of `Slave*` as a parameter. The will be copied by value, object lifetime isn't a problem in that case.
> On April 26, 2016, 8:27 a.m., Adam B wrote:
> > src/slave/http.cpp, line 365
> > <https://reviews.apache.org/r/46203/diff/13/?file=1359473#file1359473line365>
> >
> > Why pass the entire Slave down when you only use the flags?
Yes, much better approach.
> On April 26, 2016, 8:27 a.m., Adam B wrote:
> > src/tests/slave_authorization_tests.cpp, line 144
> > <https://reviews.apache.org/r/46203/diff/13/?file=1359475#file1359475line144>
> >
> > After reading the description of the test, I expected to see ACLs that set permissive=false, but adds a rule for GetEndpoint(ANY, "/flags")
> > What you're testing is fully permissive ACLs, which is a bit different, and probably tested throughout the rest of the existing tests.
Will change it. That way it'll be much clearer.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130538
-----------------------------------------------------------
On April 25, 2016, 2:50 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 25, 2016, 2:50 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 26, 2016, 8:27 a.m., Adam B wrote:
> > src/tests/slave_authorization_tests.cpp, lines 73-75
> > <https://reviews.apache.org/r/46203/diff/13/?file=1359475#file1359475line73>
> >
> > I'd rather you wrap the first line at `<` so LocalAuthorizer and tests::Module start at the same indentation as AuthorizerTypes.
> > I know the other AuthorizerTypes and AllocatorTypes follow the same pattern you have here, but they look ugly/jagged too.
> > I prefer the look of HttpAuthenticatorTypes in http_authentication_tests.cpp
Thanks for pointing me to `http_authentication_tests.cpp`, looks less ugly that way.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130538
-----------------------------------------------------------
On April 26, 2016, 1:10 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 26, 2016, 1:10 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 26, 2016, 8:27 a.m., Adam B wrote:
> > src/slave/http.cpp, line 362
> > <https://reviews.apache.org/r/46203/diff/13/?file=1359473#file1359473line362>
> >
> > This function still assumes GET. Please pass a something like a Verb enum as a parameter, or else you'll need an `authorizeGetEndpoint()`, `authorizePostEndpoint()`, etc.
Okay, I'll add an enum that holds the verb (or method, as that's what it's called in `process::http::Request`. In this patch the enum will only have a `GET` value. Only when other patches need authorization of `POST` requests, will they have to add the `POST` value and add the respective authorization code in `authorizeEndpoint`.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130538
-----------------------------------------------------------
On April 25, 2016, 2:50 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 25, 2016, 2:50 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review130538
-----------------------------------------------------------
Looks great! I think we just need to pass the GET/POST verb into `authorizeEndpoint()` and fix the other minor nits, then we'll be ready to ship. Or maybe you can convince me that we don't need to add the verb until we actually have to authorize a non-GET verb (e.g. for maintenance primitives).
docs/configuration.md (line 900)
<https://reviews.apache.org/r/46203/#comment194312>
How about you give some real example endpoints, like "/flags" and "/monitor/statistics" (the latter shows that longer URLs are allowed)?
Same in flags.cpp
include/mesos/authorizer/acls.proto (line 150)
<https://reviews.apache.org/r/46203/#comment194313>
s/access/GET HTTP/
include/mesos/authorizer/acls.proto (line 152)
<https://reviews.apache.org/r/46203/#comment194314>
Not necessarily an operator.
s/Operator/HTTP/?
src/slave/http.cpp (line 360)
<https://reviews.apache.org/r/46203/#comment194319>
Should this perhaps be a `Shared<>`?
src/slave/http.cpp (line 362)
<https://reviews.apache.org/r/46203/#comment194316>
This function still assumes GET. Please pass a something like a Verb enum as a parameter, or else you'll need an `authorizeGetEndpoint()`, `authorizePostEndpoint()`, etc.
src/slave/http.cpp (line 365)
<https://reviews.apache.org/r/46203/#comment194320>
Why pass the entire Slave down when you only use the flags?
src/slave/http.cpp (lines 797 - 799)
<https://reviews.apache.org/r/46203/#comment194317>
For my comfort, can you also validate that `pathComponents[0] == ""` and `pathComponents[1].startsWith("slave(")` so that it's clearer how this string is being split?
Then we'll fail fast if the format changes, rather than passing incorrect substrings to the authorizer.
Then we can drop the other issue about the magic number '3', since it's more clearly documented/explained.
src/slave/http.cpp (line 804)
<https://reviews.apache.org/r/46203/#comment194318>
s/access/GET/ and shouldn't you be checking the Verb here, for when we have to authorize things other than GETs?
src/tests/slave_authorization_tests.cpp (line 61)
<https://reviews.apache.org/r/46203/#comment194359>
`s/Parameter *parameter/Parameter* parameter/`
src/tests/slave_authorization_tests.cpp (lines 73 - 75)
<https://reviews.apache.org/r/46203/#comment194360>
I'd rather you wrap the first line at `<` so LocalAuthorizer and tests::Module start at the same indentation as AuthorizerTypes.
I know the other AuthorizerTypes and AllocatorTypes follow the same pattern you have here, but they look ugly/jagged too.
I prefer the look of HttpAuthenticatorTypes in http_authentication_tests.cpp
src/tests/slave_authorization_tests.cpp (lines 90 - 92)
<https://reviews.apache.org/r/46203/#comment194361>
s/acl1/acl/g
src/tests/slave_authorization_tests.cpp (lines 100 - 102)
<https://reviews.apache.org/r/46203/#comment194363>
If you wrap after the `=`, you can fit the entire rhs on one line.
src/tests/slave_authorization_tests.cpp (line 144)
<https://reviews.apache.org/r/46203/#comment194366>
After reading the description of the test, I expected to see ACLs that set permissive=false, but adds a rule for GetEndpoint(ANY, "/flags")
What you're testing is fully permissive ACLs, which is a bit different, and probably tested throughout the rest of the existing tests.
- Adam B
On April 25, 2016, 5:50 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 25, 2016, 5:50 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/
-----------------------------------------------------------
(Updated April 25, 2016, 2:50 p.m.)
Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
Changes
-------
Added comments to explain the intent of certain actions.
Bugs: MESOS-5142
https://issues.apache.org/jira/browse/MESOS-5142
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5
include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5
src/tests/slave_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/46203/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/
-----------------------------------------------------------
(Updated April 25, 2016, 10:30 a.m.)
Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
Changes
-------
Rebased.
Bugs: MESOS-5142
https://issues.apache.org/jira/browse/MESOS-5142
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5
include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5
src/tests/slave_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/46203/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/
-----------------------------------------------------------
(Updated April 25, 2016, 10:19 a.m.)
Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
Changes
-------
Changed dependency chain.
Bugs: MESOS-5142
https://issues.apache.org/jira/browse/MESOS-5142
Repository: mesos
Description
-------
See summary.
Diffs
-----
docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5
include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
src/authorizer/local/authorizer.cpp c744a16041c2466d3839a37fbee2bf86887bf4e1
src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
src/slave/http.cpp f887a71684304a82ff0d81dbd240e29aa7e2ac67
src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5
src/tests/slave_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/46203/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/
-----------------------------------------------------------
(Updated April 22, 2016, 7:46 p.m.)
Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
Changes
-------
To remove dependency circle, "depends on" was changed back to 45922. But the actual dependency chain is now: 45922, 46318, 46203, 46319. Need to sync that with bbannier.
Bugs: MESOS-5142
https://issues.apache.org/jira/browse/MESOS-5142
Repository: mesos
Description
-------
See summary.
Diffs
-----
docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5
include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
src/authorizer/local/authorizer.cpp c744a16041c2466d3839a37fbee2bf86887bf4e1
src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
src/slave/http.cpp f887a71684304a82ff0d81dbd240e29aa7e2ac67
src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5
src/tests/slave_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/46203/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/
-----------------------------------------------------------
(Updated April 22, 2016, 7:35 p.m.)
Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
Changes
-------
Addresses issues (renamed ACL action, changed tests to TYPED_TEST_CASE as in `authorization_tests.cpp`)
Bugs: MESOS-5142
https://issues.apache.org/jira/browse/MESOS-5142
Repository: mesos
Description (updated)
-------
See summary.
Diffs (updated)
-----
docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5
include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
src/authorizer/local/authorizer.cpp c744a16041c2466d3839a37fbee2bf86887bf4e1
src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
src/slave/http.cpp f887a71684304a82ff0d81dbd240e29aa7e2ac67
src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5
src/tests/slave_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/46203/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/
-----------------------------------------------------------
(Updated April 22, 2016, 12:12 p.m.)
Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
Changes
-------
Rebased and addressed some issues.
Bugs: MESOS-5142
https://issues.apache.org/jira/browse/MESOS-5142
Repository: mesos
Description (updated)
-------
Added authorization of the '/flags' endpoint.
Diffs (updated)
-----
docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5
include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
src/authorizer/local/authorizer.cpp c744a16041c2466d3839a37fbee2bf86887bf4e1
src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
src/slave/http.cpp f887a71684304a82ff0d81dbd240e29aa7e2ac67
src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5
src/tests/slave_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/46203/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 20, 2016, 10:35 a.m., Adam B wrote:
> > src/slave/http.cpp, lines 658-660
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350392#file1350392line658>
> >
> > Where did you come up with the magic number 3? What if we reorganize the operator endpoints in the (1.0) future? How will we know what the new value should be here?
> > What if the user setup a reverse proxy (like in dcos) and these requests are actually coming from a different base url than expected?
>
> Benjamin Bannier wrote:
> @adam: The three here is needed so that this just strips the agent part of the path, not everything up to the last `/`. An example endpoint would be `/slave(1)/monitor/statistics`.
>
> Jan Schlicht wrote:
> Seems like a hard problem to fully support both requirements. Maybe reverting back to using `std::string` instead of `http::URL` as the function parameter for `endpoint` could resolve this.
>
> Benjamin Bannier wrote:
> Please use some typed entity that the usual endpoint handlers are aware of. They currently have a `Request`, but e.g., have no idea how they are being routed.
>
> Jan Schlicht wrote:
> I'll go back to using the "magic number 3". At this point `URL::path` will look like this: "/slave(n)/name/of/endpoint". By splitting into 3 components we get rid of the "/slave(n)/". The path is not the full URL that has been requested, hence reverse proxies shouldn't be an issue here. I'll add a comment, explaining this.
>
> Adam B wrote:
> I see. And will this value be the same for the master's endpoints?
> Good to hear that reverse proxies won't be affected since it's not a full URL.
This values won't work for the master's endpoint. In that case `URL::path` will be "/name/of/endpoint" and we wouldn't need to split. Because we're in `Slave::Http` we can expect that this code is only called for agents.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review129696
-----------------------------------------------------------
On April 25, 2016, 10:30 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 25, 2016, 10:30 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 20, 2016, 10:35 a.m., Adam B wrote:
> > docs/configuration.md, line 897
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350386#file1350386line897>
> >
> > We're going to have to start documenting which endpoints can/must be authorized this way, similar to how Joerg added authentication to endpoint help. Can you create a new JIRA for this?
I've added [MESOS-5273](https://issues.apache.org/jira/browse/MESOS-5142) for that task.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review129696
-----------------------------------------------------------
On April 25, 2016, 10:30 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 25, 2016, 10:30 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 20, 2016, 10:35 a.m., Adam B wrote:
> > src/slave/http.cpp, line 354
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350392#file1350392line354>
> >
> > Forgive my lambda-ignorance here, but are you creating this locally scoped pointer just so that you can expose it to the lambda? Can you not reference `slave` directly?
>
> Jan Schlicht wrote:
> I can't use `slave` directly because it's a member of `this`. We can't capture `this` in the lambda because it may be no longer known when the lambda is called, resulting in a segfault.
>
> Adam B wrote:
> Thanks for the education!
But I'll add a comment, describing the intent because it is not immediately clear.
> On April 20, 2016, 10:35 a.m., Adam B wrote:
> > src/tests/slave_authorization_tests.cpp, lines 110-114
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350395#file1350395line110>
> >
> > This seems wrong. You don't even bother to reset the authenticator after you're done?
> > Why do you even need to unset the authenticator when you already set `slaveFlags.authenticate_http = false`?
>
> Jan Schlicht wrote:
> Unfortunately the libprocess environment is shared between tests. An agent sets the libprocess-wide authenticator when initialized but doesn't unset it -- which is good, because otherwise tests instanciating multiple agents may not work correctly. As a consequence, the authenticator may still be set when this test case is started, forcing us to have to explicitly unset it here.
>
> Adam B wrote:
> And then you don't need to reset it at the end of this test because the next agent that starts will set it anew?
Yes, exactly so.
But resetting it would be the right way, i.e. all tests should do it like that. Then we wouldn't need to unset it here. That's what's done for the master in `cluster.cpp` (see `Master::~Master`) but it would restrict us to using a single agent in the tests. Better would be something like a reference pointed pointer to some object that unsets the authenticator after all agents used in the test have been destroyed.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review129696
-----------------------------------------------------------
On April 19, 2016, 2:08 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 19, 2016, 2:08 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md cd9733002a0940f44edd4e56593a0ca3fe9f77f5
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am 5a2b4efa781752863d6751f98614fb78bece73ac
> src/authorizer/local/authorizer.cpp c744a16041c2466d3839a37fbee2bf86887bf4e1
> src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
> src/slave/http.cpp 3908e33ed5b233387790276f6f5d884452087d2c
> src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1
> src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Adam B <ad...@mesosphere.io>.
> On April 20, 2016, 1:35 a.m., Adam B wrote:
> > include/mesos/authorizer/acls.proto, line 151
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350387#file1350387line151>
> >
> > Let's consider calling this `GetEndpoint`, to match the HTTP verb? There may be some users that are allowed to GET the endpoint at a particular path, but cannot POST to it.
>
> Jan Schlicht wrote:
> You're right, follow up tickets will want to authorize `POST` actions and operators pretty sure will want them to be different from their `GET` acl.
> Will rename it to `GetEndpoint`. As a consequence `ACLs.acces_endpoints` will be renamed to `ACLs.get_endpoints` and `ACCESS_ENDPOINT_WITH_PATH` to `GET_ENDPOINTS_WITH_PATH`.
+1
> On April 20, 2016, 1:35 a.m., Adam B wrote:
> > src/slave/http.cpp, lines 658-660
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350392#file1350392line658>
> >
> > Where did you come up with the magic number 3? What if we reorganize the operator endpoints in the (1.0) future? How will we know what the new value should be here?
> > What if the user setup a reverse proxy (like in dcos) and these requests are actually coming from a different base url than expected?
>
> Benjamin Bannier wrote:
> @adam: The three here is needed so that this just strips the agent part of the path, not everything up to the last `/`. An example endpoint would be `/slave(1)/monitor/statistics`.
>
> Jan Schlicht wrote:
> Seems like a hard problem to fully support both requirements. Maybe reverting back to using `std::string` instead of `http::URL` as the function parameter for `endpoint` could resolve this.
>
> Benjamin Bannier wrote:
> Please use some typed entity that the usual endpoint handlers are aware of. They currently have a `Request`, but e.g., have no idea how they are being routed.
>
> Jan Schlicht wrote:
> I'll go back to using the "magic number 3". At this point `URL::path` will look like this: "/slave(n)/name/of/endpoint". By splitting into 3 components we get rid of the "/slave(n)/". The path is not the full URL that has been requested, hence reverse proxies shouldn't be an issue here. I'll add a comment, explaining this.
I see. And will this value be the same for the master's endpoints?
Good to hear that reverse proxies won't be affected since it's not a full URL.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review129696
-----------------------------------------------------------
On April 25, 2016, 1:30 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 25, 2016, 1:30 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Alexander Rojas <al...@mesosphere.io>.
> On April 20, 2016, 10:35 a.m., Adam B wrote:
> > src/slave/http.cpp, lines 658-660
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350392#file1350392line658>
> >
> > Where did you come up with the magic number 3? What if we reorganize the operator endpoints in the (1.0) future? How will we know what the new value should be here?
> > What if the user setup a reverse proxy (like in dcos) and these requests are actually coming from a different base url than expected?
>
> Benjamin Bannier wrote:
> @adam: The three here is needed so that this just strips the agent part of the path, not everything up to the last `/`. An example endpoint would be `/slave(1)/monitor/statistics`.
>
> Jan Schlicht wrote:
> Seems like a hard problem to fully support both requirements. Maybe reverting back to using `std::string` instead of `http::URL` as the function parameter for `endpoint` could resolve this.
>
> Benjamin Bannier wrote:
> Please use some typed entity that the usual endpoint handlers are aware of. They currently have a `Request`, but e.g., have no idea how they are being routed.
>
> Jan Schlicht wrote:
> I'll go back to using the "magic number 3". At this point `URL::path` will look like this: "/slave(n)/name/of/endpoint". By splitting into 3 components we get rid of the "/slave(n)/". The path is not the full URL that has been requested, hence reverse proxies shouldn't be an issue here. I'll add a comment, explaining this.
>
> Adam B wrote:
> I see. And will this value be the same for the master's endpoints?
> Good to hear that reverse proxies won't be affected since it's not a full URL.
>
> Jan Schlicht wrote:
> This values won't work for the master's endpoint. In that case `URL::path` will be "/name/of/endpoint" and we wouldn't need to split. Because we're in `Slave::Http` we can expect that this code is only called for agents.
>
> Alexander Rojas wrote:
> So here is my issue wit this, you break it into three, and pass only the second one to the authorizer, but that just sets a bad precedent. There are endpoint that added with more components, e.g. `/api/v1/scheduler`. The right way to solve this is to do something like:
>
> ```c++
> // … code to handle when `url.path` is empty.
>
> std::string path = url.path;
> std::size_t position = path.find('/', 1);
> if (position != std::string::npos) {
> path = path.substr(position);
> }
>
> // Call the authorizer.
> ```
>
> And we can add code to the authorizer module instead on how to handle objects which encode paths (just like we dispatch to the endpoint handlers).
Forget what I said, I now understand what split with parameter does.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review129696
-----------------------------------------------------------
On April 25, 2016, 2:50 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 25, 2016, 2:50 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 20, 2016, 10:35 a.m., Adam B wrote:
> > include/mesos/authorizer/acls.proto, line 151
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350387#file1350387line151>
> >
> > Let's consider calling this `GetEndpoint`, to match the HTTP verb? There may be some users that are allowed to GET the endpoint at a particular path, but cannot POST to it.
You're right, follow up tickets will want to authorize `POST` actions and operators pretty sure will want them to be different from their `GET` acl.
Will rename it to `GetEndpoint`. As a consequence `ACLs.acces_endpoints` will be renamed to `ACLs.get_endpoints` and `ACCESS_ENDPOINT_WITH_PATH` to `GET_ENDPOINTS_WITH_PATH`.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review129696
-----------------------------------------------------------
On April 22, 2016, 12:12 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 22, 2016, 12:12 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added authorization of the '/flags' endpoint.
>
>
> Diffs
> -----
>
> docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp c744a16041c2466d3839a37fbee2bf86887bf4e1
> src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
> src/slave/http.cpp f887a71684304a82ff0d81dbd240e29aa7e2ac67
> src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 20, 2016, 10:35 a.m., Adam B wrote:
> > src/tests/slave_authorization_tests.cpp, lines 110-114
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350395#file1350395line110>
> >
> > This seems wrong. You don't even bother to reset the authenticator after you're done?
> > Why do you even need to unset the authenticator when you already set `slaveFlags.authenticate_http = false`?
>
> Jan Schlicht wrote:
> Unfortunately the libprocess environment is shared between tests. An agent sets the libprocess-wide authenticator when initialized but doesn't unset it -- which is good, because otherwise tests instanciating multiple agents may not work correctly. As a consequence, the authenticator may still be set when this test case is started, forcing us to have to explicitly unset it here.
>
> Adam B wrote:
> And then you don't need to reset it at the end of this test because the next agent that starts will set it anew?
>
> Jan Schlicht wrote:
> Yes, exactly so.
> But resetting it would be the right way, i.e. all tests should do it like that. Then we wouldn't need to unset it here. That's what's done for the master in `cluster.cpp` (see `Master::~Master`) but it would restrict us to using a single agent in the tests. Better would be something like a reference pointed pointer to some object that unsets the authenticator after all agents used in the test have been destroyed.
s/reference pointed pointer/reference counted pointer/ ... = `std::shared_ptr`.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review129696
-----------------------------------------------------------
On April 22, 2016, 12:12 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 22, 2016, 12:12 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added authorization of the '/flags' endpoint.
>
>
> Diffs
> -----
>
> docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp c744a16041c2466d3839a37fbee2bf86887bf4e1
> src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
> src/slave/http.cpp f887a71684304a82ff0d81dbd240e29aa7e2ac67
> src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On April 20, 2016, 10:35 a.m., Adam B wrote:
> > src/slave/http.cpp, lines 658-660
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350392#file1350392line658>
> >
> > Where did you come up with the magic number 3? What if we reorganize the operator endpoints in the (1.0) future? How will we know what the new value should be here?
> > What if the user setup a reverse proxy (like in dcos) and these requests are actually coming from a different base url than expected?
>
> Benjamin Bannier wrote:
> @adam: The three here is needed so that this just strips the agent part of the path, not everything up to the last `/`. An example endpoint would be `/slave(1)/monitor/statistics`.
>
> Jan Schlicht wrote:
> Seems like a hard problem to fully support both requirements. Maybe reverting back to using `std::string` instead of `http::URL` as the function parameter for `endpoint` could resolve this.
Please use some typed entity that the usual endpoint handlers are aware of. They currently have a `Request`, but e.g., have no idea how they are being routed.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review129696
-----------------------------------------------------------
On April 25, 2016, 10:30 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 25, 2016, 10:30 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On April 20, 2016, 10:35 a.m., Adam B wrote:
> > src/slave/http.cpp, lines 658-660
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350392#file1350392line658>
> >
> > Where did you come up with the magic number 3? What if we reorganize the operator endpoints in the (1.0) future? How will we know what the new value should be here?
> > What if the user setup a reverse proxy (like in dcos) and these requests are actually coming from a different base url than expected?
@adam: The three here is needed so that this just strips the agent part of the path, not everything up to the last `/`. An example endpoint would be `/slave(1)/monitor/statistics`.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review129696
-----------------------------------------------------------
On April 25, 2016, 10:30 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 25, 2016, 10:30 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Adam B <ad...@mesosphere.io>.
> On April 20, 2016, 1:35 a.m., Adam B wrote:
> > src/slave/http.cpp, line 354
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350392#file1350392line354>
> >
> > Forgive my lambda-ignorance here, but are you creating this locally scoped pointer just so that you can expose it to the lambda? Can you not reference `slave` directly?
>
> Jan Schlicht wrote:
> I can't use `slave` directly because it's a member of `this`. We can't capture `this` in the lambda because it may be no longer known when the lambda is called, resulting in a segfault.
Thanks for the education!
> On April 20, 2016, 1:35 a.m., Adam B wrote:
> > src/tests/slave_authorization_tests.cpp, line 49
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350395#file1350395line49>
> >
> > Any reason why these shouldn't just go in authorization_tests.cpp?
>
> Jan Schlicht wrote:
> I'm following the pattern established with `authorization_tests.cpp` vs `master_authorization_tests.cpp`: I'd consider the tests in `authorization_tests.cpp` the unit tests of the various actions and `master_authorization_tests.cpp` the integration tests, putting those actions into use. Because there wasn't an equivalent for agents yet, I created it.
sgtm
> On April 20, 2016, 1:35 a.m., Adam B wrote:
> > src/tests/slave_authorization_tests.cpp, lines 110-114
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350395#file1350395line110>
> >
> > This seems wrong. You don't even bother to reset the authenticator after you're done?
> > Why do you even need to unset the authenticator when you already set `slaveFlags.authenticate_http = false`?
>
> Jan Schlicht wrote:
> Unfortunately the libprocess environment is shared between tests. An agent sets the libprocess-wide authenticator when initialized but doesn't unset it -- which is good, because otherwise tests instanciating multiple agents may not work correctly. As a consequence, the authenticator may still be set when this test case is started, forcing us to have to explicitly unset it here.
And then you don't need to reset it at the end of this test because the next agent that starts will set it anew?
> On April 20, 2016, 1:35 a.m., Adam B wrote:
> > src/tests/slave_authorization_tests.cpp, line 119
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350395#file1350395line119>
> >
> > You shouldn't need to change these
>
> Jan Schlicht wrote:
> I need to, because the agent would fail to start otherwise, see https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L439
True, true. I forgot that http authn cannot act in authn-allowed-but-not-required mode, like framework/agent authn can. Dropping.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review129696
-----------------------------------------------------------
On April 19, 2016, 5:08 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 19, 2016, 5:08 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md cd9733002a0940f44edd4e56593a0ca3fe9f77f5
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am 5a2b4efa781752863d6751f98614fb78bece73ac
> src/authorizer/local/authorizer.cpp c744a16041c2466d3839a37fbee2bf86887bf4e1
> src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
> src/slave/http.cpp 3908e33ed5b233387790276f6f5d884452087d2c
> src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1
> src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 20, 2016, 10:35 a.m., Adam B wrote:
> > src/slave/http.cpp, lines 658-660
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350392#file1350392line658>
> >
> > Where did you come up with the magic number 3? What if we reorganize the operator endpoints in the (1.0) future? How will we know what the new value should be here?
> > What if the user setup a reverse proxy (like in dcos) and these requests are actually coming from a different base url than expected?
>
> Benjamin Bannier wrote:
> @adam: The three here is needed so that this just strips the agent part of the path, not everything up to the last `/`. An example endpoint would be `/slave(1)/monitor/statistics`.
Seems like a hard problem to fully support both requirements. Maybe reverting back to using `std::string` instead of `http::URL` as the function parameter for `endpoint` could resolve this.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review129696
-----------------------------------------------------------
On April 25, 2016, 10:30 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 25, 2016, 10:30 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On April 20, 2016, 8:35 a.m., Adam B wrote:
> > src/slave/http.cpp, lines 658-660
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350392#file1350392line658>
> >
> > Where did you come up with the magic number 3? What if we reorganize the operator endpoints in the (1.0) future? How will we know what the new value should be here?
> > What if the user setup a reverse proxy (like in dcos) and these requests are actually coming from a different base url than expected?
>
> Benjamin Bannier wrote:
> @adam: The three here is needed so that this just strips the agent part of the path, not everything up to the last `/`. An example endpoint would be `/slave(1)/monitor/statistics`.
>
> Jan Schlicht wrote:
> Seems like a hard problem to fully support both requirements. Maybe reverting back to using `std::string` instead of `http::URL` as the function parameter for `endpoint` could resolve this.
>
> Benjamin Bannier wrote:
> Please use some typed entity that the usual endpoint handlers are aware of. They currently have a `Request`, but e.g., have no idea how they are being routed.
>
> Jan Schlicht wrote:
> I'll go back to using the "magic number 3". At this point `URL::path` will look like this: "/slave(n)/name/of/endpoint". By splitting into 3 components we get rid of the "/slave(n)/". The path is not the full URL that has been requested, hence reverse proxies shouldn't be an issue here. I'll add a comment, explaining this.
>
> Adam B wrote:
> I see. And will this value be the same for the master's endpoints?
> Good to hear that reverse proxies won't be affected since it's not a full URL.
>
> Jan Schlicht wrote:
> This values won't work for the master's endpoint. In that case `URL::path` will be "/name/of/endpoint" and we wouldn't need to split. Because we're in `Slave::Http` we can expect that this code is only called for agents.
>
> Alexander Rojas wrote:
> So here is my issue wit this, you break it into three, and pass only the second one to the authorizer, but that just sets a bad precedent. There are endpoint that added with more components, e.g. `/api/v1/scheduler`. The right way to solve this is to do something like:
>
> ```c++
> // … code to handle when `url.path` is empty.
>
> std::string path = url.path;
> std::size_t position = path.find('/', 1);
> if (position != std::string::npos) {
> path = path.substr(position);
> }
>
> // Call the authorizer.
> ```
>
> And we can add code to the authorizer module instead on how to handle objects which encode paths (just like we dispatch to the endpoint handlers).
>
> Alexander Rojas wrote:
> Forget what I said, I now understand what split with parameter does.
Once we `tokenize` instead of `split`, 3 will become 2, which will make it easier to understand where the magic number is coming from.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review129696
-----------------------------------------------------------
On April 26, 2016, 3:27 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 26, 2016, 3:27 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 20, 2016, 10:35 a.m., Adam B wrote:
> > src/tests/slave_authorization_tests.cpp, line 49
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350395#file1350395line49>
> >
> > Any reason why these shouldn't just go in authorization_tests.cpp?
I'm following the pattern established with `authorization_tests.cpp` vs `master_authorization_tests.cpp`: I'd consider the tests in `authorization_tests.cpp` the unit tests of the various actions and `master_authorization_tests.cpp` the integration tests, putting those actions into use. Because there wasn't an equivalent for agents yet, I created it.
> On April 20, 2016, 10:35 a.m., Adam B wrote:
> > src/tests/slave_authorization_tests.cpp, lines 110-114
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350395#file1350395line110>
> >
> > This seems wrong. You don't even bother to reset the authenticator after you're done?
> > Why do you even need to unset the authenticator when you already set `slaveFlags.authenticate_http = false`?
Unfortunately the libprocess environment is shared between tests. An agent sets the libprocess-wide authenticator when initialized but doesn't unset it -- which is good, because otherwise tests instanciating multiple agents may not work correctly. As a consequence, the authenticator may still be set when this test case is started, forcing us to have to explicitly unset it here.
> On April 20, 2016, 10:35 a.m., Adam B wrote:
> > src/slave/http.cpp, line 354
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350392#file1350392line354>
> >
> > Forgive my lambda-ignorance here, but are you creating this locally scoped pointer just so that you can expose it to the lambda? Can you not reference `slave` directly?
I can't use `slave` directly because it's a member of `this`. We can't capture `this` in the lambda because it may be no longer known when the lambda is called, resulting in a segfault.
> On April 20, 2016, 10:35 a.m., Adam B wrote:
> > src/tests/slave_authorization_tests.cpp, line 119
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350395#file1350395line119>
> >
> > You shouldn't need to change these
I need to, because the agent would fail to start otherwise, see https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L439
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review129696
-----------------------------------------------------------
On April 19, 2016, 2:08 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 19, 2016, 2:08 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md cd9733002a0940f44edd4e56593a0ca3fe9f77f5
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am 5a2b4efa781752863d6751f98614fb78bece73ac
> src/authorizer/local/authorizer.cpp c744a16041c2466d3839a37fbee2bf86887bf4e1
> src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
> src/slave/http.cpp 3908e33ed5b233387790276f6f5d884452087d2c
> src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1
> src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Alexander Rojas <al...@mesosphere.io>.
> On April 20, 2016, 10:35 a.m., Adam B wrote:
> > src/slave/http.cpp, lines 658-660
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350392#file1350392line658>
> >
> > Where did you come up with the magic number 3? What if we reorganize the operator endpoints in the (1.0) future? How will we know what the new value should be here?
> > What if the user setup a reverse proxy (like in dcos) and these requests are actually coming from a different base url than expected?
>
> Benjamin Bannier wrote:
> @adam: The three here is needed so that this just strips the agent part of the path, not everything up to the last `/`. An example endpoint would be `/slave(1)/monitor/statistics`.
>
> Jan Schlicht wrote:
> Seems like a hard problem to fully support both requirements. Maybe reverting back to using `std::string` instead of `http::URL` as the function parameter for `endpoint` could resolve this.
>
> Benjamin Bannier wrote:
> Please use some typed entity that the usual endpoint handlers are aware of. They currently have a `Request`, but e.g., have no idea how they are being routed.
>
> Jan Schlicht wrote:
> I'll go back to using the "magic number 3". At this point `URL::path` will look like this: "/slave(n)/name/of/endpoint". By splitting into 3 components we get rid of the "/slave(n)/". The path is not the full URL that has been requested, hence reverse proxies shouldn't be an issue here. I'll add a comment, explaining this.
>
> Adam B wrote:
> I see. And will this value be the same for the master's endpoints?
> Good to hear that reverse proxies won't be affected since it's not a full URL.
>
> Jan Schlicht wrote:
> This values won't work for the master's endpoint. In that case `URL::path` will be "/name/of/endpoint" and we wouldn't need to split. Because we're in `Slave::Http` we can expect that this code is only called for agents.
So here is my issue wit this, you break it into three, and pass only the second one to the authorizer, but that just sets a bad precedent. There are endpoint that added with more components, e.g. `/api/v1/scheduler`. The right way to solve this is to do something like:
```c++
// … code to handle when `url.path` is empty.
std::string path = url.path;
std::size_t position = path.find('/', 1);
if (position != std::string::npos) {
path = path.substr(position);
}
// Call the authorizer.
```
And we can add code to the authorizer module instead on how to handle objects which encode paths (just like we dispatch to the endpoint handlers).
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review129696
-----------------------------------------------------------
On April 25, 2016, 2:50 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 25, 2016, 2:50 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 20, 2016, 10:35 a.m., Adam B wrote:
> > src/slave/http.cpp, lines 658-660
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350392#file1350392line658>
> >
> > Where did you come up with the magic number 3? What if we reorganize the operator endpoints in the (1.0) future? How will we know what the new value should be here?
> > What if the user setup a reverse proxy (like in dcos) and these requests are actually coming from a different base url than expected?
>
> Benjamin Bannier wrote:
> @adam: The three here is needed so that this just strips the agent part of the path, not everything up to the last `/`. An example endpoint would be `/slave(1)/monitor/statistics`.
>
> Jan Schlicht wrote:
> Seems like a hard problem to fully support both requirements. Maybe reverting back to using `std::string` instead of `http::URL` as the function parameter for `endpoint` could resolve this.
>
> Benjamin Bannier wrote:
> Please use some typed entity that the usual endpoint handlers are aware of. They currently have a `Request`, but e.g., have no idea how they are being routed.
I'll go back to using the "magic number 3". At this point `URL::path` will look like this: "/slave(n)/name/of/endpoint". By splitting into 3 components we get rid of the "/slave(n)/". The path is not the full URL that has been requested, hence reverse proxies shouldn't be an issue here. I'll add a comment, explaining this.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review129696
-----------------------------------------------------------
On April 25, 2016, 10:30 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 25, 2016, 10:30 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Adam B <ad...@mesosphere.io>.
> On April 20, 2016, 1:35 a.m., Adam B wrote:
> > src/tests/slave_authorization_tests.cpp, lines 110-114
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350395#file1350395line110>
> >
> > This seems wrong. You don't even bother to reset the authenticator after you're done?
> > Why do you even need to unset the authenticator when you already set `slaveFlags.authenticate_http = false`?
>
> Jan Schlicht wrote:
> Unfortunately the libprocess environment is shared between tests. An agent sets the libprocess-wide authenticator when initialized but doesn't unset it -- which is good, because otherwise tests instanciating multiple agents may not work correctly. As a consequence, the authenticator may still be set when this test case is started, forcing us to have to explicitly unset it here.
>
> Adam B wrote:
> And then you don't need to reset it at the end of this test because the next agent that starts will set it anew?
>
> Jan Schlicht wrote:
> Yes, exactly so.
> But resetting it would be the right way, i.e. all tests should do it like that. Then we wouldn't need to unset it here. That's what's done for the master in `cluster.cpp` (see `Master::~Master`) but it would restrict us to using a single agent in the tests. Better would be something like a reference pointed pointer to some object that unsets the authenticator after all agents used in the test have been destroyed.
>
> Jan Schlicht wrote:
> s/reference pointed pointer/reference counted pointer/ ... = `std::shared_ptr`.
Ok, please add a TODO to fix this weird unset behavior somehow. Then we can drop this review issue.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review129696
-----------------------------------------------------------
On April 25, 2016, 1:30 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 25, 2016, 1:30 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
> src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b
> src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
> src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e
> src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/#review129696
-----------------------------------------------------------
Great start! Some concerns:
- We need a JIRA to make sure we document which endpoints require authorization for what.
- Handling multiple HTTP verbs that authorize differently
- Handling custom base urls (e.g. reverse proxies)
- Should use TYPED_TEST_CASE to test the module API as well
docs/configuration.md (line 893)
<https://reviews.apache.org/r/46203/#comment193230>
acls.proto
docs/configuration.md (line 897)
<https://reviews.apache.org/r/46203/#comment193231>
We're going to have to start documenting which endpoints can/must be authorized this way, similar to how Joerg added authentication to endpoint help. Can you create a new JIRA for this?
include/mesos/authorizer/acls.proto (line 150)
<https://reviews.apache.org/r/46203/#comment193233>
s/for given paths/at the given paths/?
include/mesos/authorizer/acls.proto (line 151)
<https://reviews.apache.org/r/46203/#comment193232>
Let's consider calling this `GetEndpoint`, to match the HTTP verb? There may be some users that are allowed to GET the endpoint at a particular path, but cannot POST to it.
include/mesos/authorizer/acls.proto (line 195)
<https://reviews.apache.org/r/46203/#comment193234>
Why the blank line?
include/mesos/authorizer/authorizer.proto (lines 68 - 70)
<https://reviews.apache.org/r/46203/#comment193235>
This won't be agent-only for long. Or do you imagine a separate enum for ACCESS_MASTER_ENDPOINT_WITH_PATH? Maybe "agent-only" is better as an //end of line comment, so it doesn't impact enum ordering
If we're going to keep adding master and agent ACLs in the same enum, I'd rather keep them in enum id order, so we don't accidentally duplicate enum ids.
If we do want to have master only, agent only, and both sections, we should start them at 0, 100, and 200; or 0, 1000, and 2000 to leave room for expansion.
src/slave/flags.cpp (line 457)
<https://reviews.apache.org/r/46203/#comment193236>
acls.proto
src/slave/flags.cpp (line 464)
<https://reviews.apache.org/r/46203/#comment193237>
Shouldn't some of these be forward-slashes, e.g. /b and /c?
Did you try running the --help after building your changes, to see what this looks like in a console?
src/slave/http.cpp (line 354)
<https://reviews.apache.org/r/46203/#comment193241>
Forgive my lambda-ignorance here, but are you creating this locally scoped pointer just so that you can expose it to the lambda? Can you not reference `slave` directly?
src/slave/http.cpp (lines 658 - 660)
<https://reviews.apache.org/r/46203/#comment193247>
Where did you come up with the magic number 3? What if we reorganize the operator endpoints in the (1.0) future? How will we know what the new value should be here?
What if the user setup a reverse proxy (like in dcos) and these requests are actually coming from a different base url than expected?
src/slave/http.cpp (line 667)
<https://reviews.apache.org/r/46203/#comment193249>
How did you know it was a GET? That's not part of the function signature.
src/tests/slave_authorization_tests.cpp (line 49)
<https://reviews.apache.org/r/46203/#comment193251>
Any reason why these shouldn't just go in authorization_tests.cpp?
src/tests/slave_authorization_tests.cpp (line 54)
<https://reviews.apache.org/r/46203/#comment193252>
Could you please use TYPED_TEST_CASE and TYPED_TEST so that we can test this with both the LocalAuthorizer directly, and the TestLocalAuthorizer module.
src/tests/slave_authorization_tests.cpp (lines 64 - 66)
<https://reviews.apache.org/r/46203/#comment193253>
You should be setting acls.permissive = false instead
src/tests/slave_authorization_tests.cpp (line 71)
<https://reviews.apache.org/r/46203/#comment193254>
Why do you need to start a master to access a slave's /flags?
src/tests/slave_authorization_tests.cpp (line 98)
<https://reviews.apache.org/r/46203/#comment193261>
I'm not really convinced that this test gives us much, since it's using default ACLs. Can you write a comment explaining what this tests that we don't get from other existing tests?
src/tests/slave_authorization_tests.cpp (lines 103 - 106)
<https://reviews.apache.org/r/46203/#comment193255>
Unnecessary, since acls.permissive=true by default.
src/tests/slave_authorization_tests.cpp (lines 110 - 114)
<https://reviews.apache.org/r/46203/#comment193258>
This seems wrong. You don't even bother to reset the authenticator after you're done?
Why do you even need to unset the authenticator when you already set `slaveFlags.authenticate_http = false`?
src/tests/slave_authorization_tests.cpp (line 119)
<https://reviews.apache.org/r/46203/#comment193259>
You shouldn't need to change these
src/tests/slave_authorization_tests.cpp (lines 125 - 127)
<https://reviews.apache.org/r/46203/#comment193260>
Won't this all fit on one line?
- Adam B
On April 19, 2016, 5:08 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
>
> (Updated April 19, 2016, 5:08 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
>
>
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/configuration.md cd9733002a0940f44edd4e56593a0ca3fe9f77f5
> include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
> include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
> src/Makefile.am 5a2b4efa781752863d6751f98614fb78bece73ac
> src/authorizer/local/authorizer.cpp c744a16041c2466d3839a37fbee2bf86887bf4e1
> src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
> src/slave/http.cpp 3908e33ed5b233387790276f6f5d884452087d2c
> src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1
> src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb
> src/tests/slave_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46203/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/
-----------------------------------------------------------
(Updated April 19, 2016, 2:08 p.m.)
Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
Bugs: MESOS-5142
https://issues.apache.org/jira/browse/MESOS-5142
Repository: mesos
Description (updated)
-------
See summary.
Diffs
-----
docs/configuration.md cd9733002a0940f44edd4e56593a0ca3fe9f77f5
include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
src/Makefile.am 5a2b4efa781752863d6751f98614fb78bece73ac
src/authorizer/local/authorizer.cpp c744a16041c2466d3839a37fbee2bf86887bf4e1
src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
src/slave/http.cpp 3908e33ed5b233387790276f6f5d884452087d2c
src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1
src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb
src/tests/slave_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/46203/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/
-----------------------------------------------------------
(Updated April 19, 2016, 2:05 p.m.)
Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
Changes
-------
Addressed Alexanders' issues.
Bugs: MESOS-5142
https://issues.apache.org/jira/browse/MESOS-5142
Repository: mesos
Description (updated)
-------
Added authorization of the '/flags' endpoint.
Diffs (updated)
-----
docs/configuration.md cd9733002a0940f44edd4e56593a0ca3fe9f77f5
include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
src/Makefile.am 5a2b4efa781752863d6751f98614fb78bece73ac
src/authorizer/local/authorizer.cpp c744a16041c2466d3839a37fbee2bf86887bf4e1
src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
src/slave/http.cpp 3908e33ed5b233387790276f6f5d884452087d2c
src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1
src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb
src/tests/slave_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/46203/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/
-----------------------------------------------------------
(Updated April 18, 2016, 2:53 p.m.)
Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
Changes
-------
Fixed segfaults and `SlaveAuthorizationTest.AuthorizeFlagsEndpointWithoutPrincipal`.
Bugs: MESOS-5142
https://issues.apache.org/jira/browse/MESOS-5142
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
docs/configuration.md cd9733002a0940f44edd4e56593a0ca3fe9f77f5
include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
src/Makefile.am ec855263d620e4723c8ba9cd056c40a3a2e9ca99
src/authorizer/local/authorizer.cpp c744a16041c2466d3839a37fbee2bf86887bf4e1
src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
src/slave/http.cpp 3908e33ed5b233387790276f6f5d884452087d2c
src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1
src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb
src/tests/slave_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/46203/diff/
Testing (updated)
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 46203: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46203/
-----------------------------------------------------------
(Updated April 18, 2016, 12:07 p.m.)
Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
Changes
-------
s/AccessEndpoints/AccessEndpoint
Bugs: MESOS-5142
https://issues.apache.org/jira/browse/MESOS-5142
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
docs/configuration.md cd9733002a0940f44edd4e56593a0ca3fe9f77f5
include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579
include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe
src/Makefile.am ec855263d620e4723c8ba9cd056c40a3a2e9ca99
src/authorizer/local/authorizer.cpp c744a16041c2466d3839a37fbee2bf86887bf4e1
src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315
src/slave/http.cpp 3908e33ed5b233387790276f6f5d884452087d2c
src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1
src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb
src/tests/slave_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/46203/diff/
Testing
-------
make check (SlaveAuthorizationTest.AuthorizeFlagsEndpointWithoutPrincipal fails though it shouldn't, tests sometimes segfault)
Thanks,
Jan Schlicht