Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On May 9, 2016, 11:14 a.m., Alexander Rukletsov wrote: > > src/slave/http.cpp, line 814 > > > > > > I think we should have used `MethodNotAllowed` here. See my comment in https://reviews.apache.org/r/46784/. I do

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On May 5, 2016, 7:20 p.m., Till Toenshoff wrote: > > src/slave/http.cpp, line 373 > > > > > > Two blanks please. Good catch! I've created https://reviews.apache.org/r/47116/ to fix that. - Jan ---

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/#review132231 --- src/slave/http.cpp (line 814)

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/#review131866 --- src/slave/http.cpp (line 373)

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 27, 2016, 11:29 a.m., Adam B wrote: > > src/authorizer/local/authorizer.cpp, line 213 > > > > > > Does this only match exact strings, or endpoints nested under this path > > as well? > > For example,

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 27, 2016, 9:29 a.m., Adam B wrote: > > src/authorizer/local/authorizer.cpp, line 213 > > > > > > Does this only match exact strings, or endpoints nested under this path > > as well? > > For example,

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/#review130788 --- Ship it! I'll fix the remamining issues and commit it for you s

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 27, 2016, 4:32 p.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 27, 2016, 11:29 a.m., Adam B wrote: > > src/authorizer/local/authorizer.cpp, line 213 > > > > > > Does this only match exact strings, or endpoints nested under this path > > as well? > > For example,

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 27, 2016, 11:29 a.m., Adam B wrote: > > src/tests/slave_authorization_tests.cpp, lines 58-66 > > > > > > Isn't this literally the same code in authorization_tests.cpp? Can we > > factor this into a common

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 27, 2016, 3:21 p.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 27, 2016, 11:29 a.m., Adam B wrote: > > src/authorizer/local/authorizer.cpp, line 213 > > > > > > Does this only match exact strings, or endpoints nested under this path > > as well? > > For example,

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 27, 2016, 12:09 p.m., Adam B wrote: > > src/tests/slave_authorization_tests.cpp, line 93 > > > > > > Should we add a test for "/flags/"? See answer above. Requesting `/flags/` would result in a 404 becaus

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 27, 2016, 11:29 a.m., Adam B wrote: > > src/slave/http.cpp, line 795 > > > > > > What happens if the request is for `/slave(0)/flags/`? Does the > > trailing slash get removed before comparing against th

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/#review130749 --- And a couple more comments on the tests src/tests/slave_authoriz

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 27, 2016, 11:29 a.m., Adam B wrote: > > src/slave/http.cpp, lines 374-377 > > > > > > Yikes! A 20 char indent is intense, and this wrapping seems extreme. > > Can we do `using flags::Flag` or `s/slaveFla

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/#review130736 --- Mostly minor nits and a couple of questions about url->acl matchin

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 27, 2016, 11:20 a.m.) Review request for mesos, Adam B, Alexande

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 27, 2016, 10:24 a.m., Alexander Rojas wrote: > > src/slave/http.cpp, line 798 > > > > > > This looks like a bad pattern here, you could use instead > > `slave->self().id == pathComponents[0]`. > > >

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 27, 2016, 1:24 a.m., Alexander Rojas wrote: > > src/slave/http.cpp, line 798 > > > > > > This looks like a bad pattern here, you could use instead > > `slave->self().id == pathComponents[0]`. > > >

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 27, 2016, 1:24 a.m., Alexander Rojas wrote: > > src/slave/http.cpp, line 798 > > > > > > This looks like a bad pattern here, you could use instead > > `slave->self().id == pathComponents[0]`. > > >

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 27, 2016, 10:54 a.m.) Review request for mesos, Adam B, Alexande

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 27, 2016, 10:24 a.m., Alexander Rojas wrote: > > src/slave/http.cpp, line 798 > > > > > > This looks like a bad pattern here, you could use instead > > `slave->self().id == pathComponents[0]`. > > >

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/#review130731 --- src/slave/http.cpp (line 798)

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 26, 2016, 3:40 p.m., Alexander Rukletsov wrote: > > docs/configuration.md, line 899 > > > > > > Let's entertain our reader a bit : ). Instead of "a", we can use > > something like "padavan", and if you p

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 26, 2016, 8:29 p.m.) Review request for mesos, Adam B, Alexander

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 26, 2016, 8:04 p.m.) Review request for mesos, Adam B, Alexander

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/#review130655 --- src/slave/http.cpp (lines 797 - 799)

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 26, 2016, 5:40 p.m., Alexander Rukletsov wrote: > > src/slave/slave.hpp, lines 471-473 > > > > > > It feels like this class should be somewhere in a more general place. > > Moreover, libprocess' `Request

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 20, 2016, 8:35 a.m., Adam B wrote: > > src/slave/http.cpp, lines 658-660 > > > > > > Where did you come up with the magic number 3? What if we reorganize > > the operator endpoints in the (1.0) future? Ho

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 26, 2016, 2:01 p.m., Benjamin Bannier wrote: > > src/slave/http.cpp, line 787 > > > > > > I really like that we use a dedicated `enum` in the switch below, but > > dislike how users of this function need

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/#review130609 --- docs/configuration.md (line 899)

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 26, 2016, 5:27 p.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 26, 2016, 4:01 p.m., Benjamin Bannier wrote: > > src/slave/http.cpp, line 362 > > > > > > You assume that `slave(XYZ)/flags` will only receive `GET` requests, > > but I think it would make more sense to

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/#review130610 --- src/slave/http.cpp (line 362)

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 26, 2016, 2:51 p.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 26, 2016, 8:27 a.m., Adam B wrote: > > src/slave/http.cpp, line 804 > > > > > > s/access/GET/ and shouldn't you be checking the Verb here, for when we > > have to authorize things other than GETs? I've

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 26, 2016, 8:27 a.m., Adam B wrote: > > src/slave/http.cpp, line 360 > > > > > > Should this perhaps be a `Shared<>`? > > Jan Schlicht wrote: > I don't think so. `Shared<>` is about shared ownership,

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 26, 2016, 8:27 a.m., Adam B wrote: > > src/tests/slave_authorization_tests.cpp, lines 73-75 > > > > > > I'd rather you wrap the first line at `<` so LocalAuthorizer and > > tests::Module start at the same

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 26, 2016, 1:10 p.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> 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

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 26, 2016, 8:27 a.m., Adam B wrote: > > src/slave/http.cpp, line 362 > > > > > > This function still assumes GET. Please pass a something like a Verb > > enum as a parameter, or else you'll need an `autho

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 26, 2016, 8:27 a.m., Adam B wrote: > > src/slave/http.cpp, line 360 > > > > > > Should this perhaps be a `Shared<>`? I don't think so. `Shared<>` is about shared ownership, but the closure shouldn't own

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/#review130538 --- Looks great! I think we just need to pass the GET/POST verb into

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 20, 2016, 10:35 a.m., Adam B wrote: > > src/slave/http.cpp, lines 658-660 > > > > > > Where did you come up with the magic number 3? What if we reorganize > > the operator endpoints in the (1.0) future? H

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 20, 2016, 10:35 a.m., Adam B wrote: > > src/slave/http.cpp, lines 658-660 > > > > > > Where did you come up with the magic number 3? What if we reorganize > > the operator endpoints in the (1.0) future? H

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 25, 2016, 2:50 p.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 20, 2016, 10:35 a.m., Adam B wrote: > > src/slave/http.cpp, lines 658-660 > > > > > > Where did you come up with the magic number 3? What if we reorganize > > the operator endpoints in the (1.0) future? H

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 20, 2016, 1:35 a.m., Adam B wrote: > > include/mesos/authorizer/acls.proto, line 151 > > > > > > Let's consider calling this `GetEndpoint`, to match the HTTP verb? > > There may be some users that are all

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 20, 2016, 1:35 a.m., Adam B wrote: > > src/tests/slave_authorization_tests.cpp, lines 110-114 > > > > > > This seems wrong. You don't even bother to reset the authenticator > > after you're done? > >

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 20, 2016, 10:35 a.m., Adam B wrote: > > src/slave/http.cpp, lines 658-660 > > > > > > Where did you come up with the magic number 3? What if we reorganize > > the operator endpoints in the (1.0) future? H

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 20, 2016, 10:35 a.m., Adam B wrote: > > src/slave/http.cpp, lines 658-660 > > > > > > Where did you come up with the magic number 3? What if we reorganize > > the operator endpoints in the (1.0) future? H

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 20, 2016, 10:35 a.m., Adam B wrote: > > src/slave/http.cpp, lines 658-660 > > > > > > Where did you come up with the magic number 3? What if we reorganize > > the operator endpoints in the (1.0) future? H

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 20, 2016, 10:35 a.m., Adam B wrote: > > src/slave/http.cpp, lines 658-660 > > > > > > Where did you come up with the magic number 3? What if we reorganize > > the operator endpoints in the (1.0) future? H

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 20, 2016, 10:35 a.m., Adam B wrote: > > docs/configuration.md, line 897 > > > > > > We're going to have to start documenting which endpoints can/must be > > authorized this way, similar to how Joerg added

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 25, 2016, 10:30 a.m.) Review request for mesos, Adam B, Alexande

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 25, 2016, 10:19 a.m.) Review request for mesos, Adam B, Alexande

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 22, 2016, 7:46 p.m.) Review request for mesos, Adam B, Alexander

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 22, 2016, 7:35 p.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 20, 2016, 10:35 a.m., Adam B wrote: > > src/tests/slave_authorization_tests.cpp, lines 110-114 > > > > > > This seems wrong. You don't even bother to reset the authenticator > > after you're done? > >

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 20, 2016, 10:35 a.m., Adam B wrote: > > include/mesos/authorizer/acls.proto, line 151 > > > > > > Let's consider calling this `GetEndpoint`, to match the HTTP verb? > > There may be some users that are al

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 22, 2016, 12:12 p.m.) Review request for mesos, Adam B, Alexande

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 20, 2016, 10:35 a.m., Adam B wrote: > > src/slave/http.cpp, line 354 > > > > > > Forgive my lambda-ignorance here, but are you creating this locally > > scoped pointer just so that you can expose it to th

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 20, 2016, 1:35 a.m., Adam B wrote: > > src/slave/http.cpp, line 354 > > > > > > Forgive my lambda-ignorance here, but are you creating this locally > > scoped pointer just so that you can expose it to the

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 20, 2016, 10:35 a.m., Adam B wrote: > > src/tests/slave_authorization_tests.cpp, line 49 > > > > > > Any reason why these shouldn't just go in authorization_tests.cpp? I'm following the pattern established

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/#review129696 --- Great start! Some concerns: - We need a JIRA to make sure we docum

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 19, 2016, 2:08 p.m.) Review request for mesos, Adam B, Alexander

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 19, 2016, 2:05 p.m.) Review request for mesos, Adam B, Alexander

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/#review129507 --- src/slave/http.cpp (lines 358 - 359)

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, 2:53 p.m.) Review request for mesos, Adam B, Alexander

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

> On April 15, 2016, 11:34 a.m., Alexander Rojas wrote: > > docs/configuration.md, line 871 > > > > > > s/get_endpoints/access_endpoint/ I'd keep it plural here. - Jan ---

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, 12:07 p.m.) Review request for mesos, Adam B, Alexande

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