Re: Review Request 54661: Enable authorization for the GET_FLAGS API Call.

tps://reviews.apache.org/r/54661/#comment230041> Looks like excessive `()` wrapping, but I can fix this before committing. - Adam B On Dec. 13, 2016, 6:49 a.m., Alexander Rojas wrote: > > --- > This is an automatically gener

Re: Review Request 54700: Fixed bug that prevented window size updates in the IOSwitchboard.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54700/#review159052 --- Ship it! Ship It! - Adam B On Dec. 13, 2016, 4:34 a.m

Re: Review Request 54538: Enabled fine grained authorization for the getContainers API Call.

n tomorrow. src/slave/http.cpp (line 1869) <https://reviews.apache.org/r/54538/#comment229880> Each parameter on a separate line, please src/slave/http.cpp (line 1929) <https://reviews.apache.org/r/54538/#comment229884> Already called `info` a few lines above. Can you reuse tha

Re: Review Request 54655: Renamed LocalNestedContainerObjectApprover for a more generic name.

.apache.org/r/54655/#comment229878> What's optional here? Should there be a `object.has_command_info()` in there somewhere? - Adam B On Dec. 12, 2016, 1:51 a.m., Alexander Rojas wrote: > > --- > This is an auto

Re: Review Request 54661: Enable authorization for the GET_FLAGS API Call.

too? src/slave/http.cpp (line 775) <https://reviews.apache.org/r/54661/#comment229872> Don't you need to check `approver->approved()` before you return OK or Forbidden? - Adam B On Dec. 12, 2016, 6:35 a.m., Alexander Rojas wrote: > >

Re: Review Request 54662: Enabled authorization in SET_LOG_LEVEL API call.

() src/slave/http.cpp (line 898) <https://reviews.apache.org/r/54662/#comment229873> Is the plan to move everything over to the ObjectApprover API? Why not use the simpler `slave->authorizer.get()->authorized(authRequest)` API for simpler authz checks? Because it's asynchr

Re: Review Request 54535: Added authorization actions VIEW_CONTAINERS and SET_LOG_LEVEL.

://reviews.apache.org/r/54535/#comment229865> s/authorizedto change/authorized to change/ src/tests/authorization_tests.cpp (line 1986) <https://reviews.apache.org/r/54535/#comment229867> s/a containers/containers/ here and elsewhere - Adam B On Dec. 12,

Re: Review Request 54538: WIP: Enabled fine grained authorization for the getContainers API Call.

g/r/54538/#comment229604> double-underscore? Do we just skip over `_containers()`? I suppose that's part of `/containers`? src/slave/http.cpp (lines 1901 - 1903) <https://reviews.apache.org/r/54538/#comment229605> No longer necessary - Adam B On Dec. 9, 2016, 7:33 a.m.

Re: Review Request 54535: WIP: Added authorization actions VIEW_CONTAINERS and SET_LOG_LEVEL.

r.cpp (line 559) <https://reviews.apache.org/r/54535/#comment229602> s/and/`AND`/ src/tests/authorization_tests.cpp (line 1929) <https://reviews.apache.org/r/54535/#comment229603> s/frameworks/containers/ here and elsewhere - Adam B On Dec. 9, 201

Re: Review Request 54538: WIP: Enabled fine grained authorization for the getContainers API Call.

ne-grained authz to the /containers endpoint as well (currently protected by coarse-grained GET_ENDPOINT authz)? - Adam B On Dec. 8, 2016, 9:33 a.m., Alexander Rojas wrote: > > --- > This is an automatically generated

Re: Review Request 54535: WIP: Added authorization actions VIEW_CONTAINERS and SET_LOG_LEVEL.

t;https://reviews.apache.org/r/54535/#comment229456> s/view_flags/set_log_level/ src/tests/authorization_tests.cpp (line 4154) <https://reviews.apache.org/r/54535/#comment229457> s/see the flags/set log level/g here and in other comments - Adam B On Dec. 8, 2016, 7:38 a.m., Alexander Rojas wro

Re: Review Request 54504: Added Mesos logo to the list of files installed for the web UI.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54504/#review158445 --- Ship it! Ship It! - Adam B On Dec. 7, 2016, 3:53 p.m., Greg

Re: Review Request 54013: Added user doc for nested container and task group.

> On Dec. 4, 2016, 11:44 a.m., Vinod Kone wrote: > > docs/nested-container-and-task-group.md, line 209 > > > > > > mention that only 2 levels of nesting is supported as of 1.1? > > Gilbert Song wrote: > We alrea

Re: Review Request 54381: Adds authorization support when attaching containers input/output.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54381/#review158317 --- Ship it! Ship It! - Adam B On Dec. 7, 2016, 1:50 a.m

Re: Review Request 54381: Adds authorization support when attaching containers input/output.

> that `Containerizer::attach()` was getting called from the API handler. > > Alexander Rojas wrote: > Because one needs to find the containers to perform authorization, and > with these tests there was no container to be found, so no authz to be > performed and therefor

Re: Review Request 54381: Adds authorization support when attaching containers input/output.

> On Dec. 6, 2016, 12:50 a.m., Adam B wrote: > > src/slave/http.cpp, lines 2465-2468 > > <https://reviews.apache.org/r/54381/diff/1/?file=1576455#file1576455line2465> > > > > Can you please comment on the need for std::move here? > > Why is decode

Re: Review Request 54381: Adds authorization support when attaching containers input/output.

just `if (executor.isSome() && executor == nullptr)` src/slave/http.cpp (line 2712) <https://reviews.apache.org/r/54381/#comment229040> Nit: container_id is `required` in `AttachContainerOutput` so it's not really necessary to CHECK it here. -

Re: Review Request 54381: Adds authorization support when attaching containers input/output.

--- On Dec. 5, 2016, 8:19 a.m., Alexander Rojas wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54381/ > --- > > (Updated Dec. 5, 2016,

Re: Review Request 54381: Adds authorization support when attaching containers input/output.

he.org/r/54381/#comment228877> Can you please comment on the need for std::move here? Why is decoder a `&&` in the first place? @vinodkone may be able to comment - Adam B On Dec. 5, 2016, 8:19 a.m., Alexander Rojas wrote: > > -

Re: Review Request 53541: Added authorization actions for Nested Container and Debug API.

991> s/no user in command/no command/ src/tests/authorization_tests.cpp (line 3376) <https://reviews.apache.org/r/53541/#comment227992> s/cannot/can/ - Adam B On Nov. 29, 2016, 7:18 a.m., Alexander Rojas wrote: > > ---

Re: Review Request 53541: Added authorization actions for Nested Container and Debug API.

lt;https://reviews.apache.org/r/53541/#comment227732> s/killing for/killing/ src/tests/authorization_tests.cpp (line 3298) <https://reviews.apache.org/r/53541/#comment227733> s/wait for/kill/ src/tests/authorization_tests.cpp (line 3311) <https://reviews.apache.org/r/53541/#

Re: Review Request 54038: Added new hook for covering executor and task environment.

es 1224 - 1225) <https://reviews.apache.org/r/54038/#comment227122> So, if executorEnvironment and taskEnvironment both have the same keys, then the taskEnvironment will override? - Adam B On Nov. 23, 2016, 11:26 a.m., Till Toenshoff wrote: > > -

Re: Review Request 53541: Added authorization actions for debug API.

org/r/53541/#comment226976> s/an/and/ (here and in LNC_SESSION) - Adam B On Nov. 22, 2016, 6:48 a.m., Alexander Rojas wrote: > > --- > This is an automatically gene

Re: Review Request 54021: Updated a Docker containerizer test.

/tests/containerizer/docker_containerizer_tests.cpp (line 552) <https://reviews.apache.org/r/54021/#comment226957> Not quite a sentence. Perhaps "Verify the ContainerStatus fields in the TaskStatus." - Adam B On Nov. 22, 2016, 10:06 p.

Re: Review Request 54001: Addes status method to DockerContainerizer.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54001/#review156682 --- Ship it! Ship It! - Adam B On Nov. 22, 2016, 1:51 p.m., Jie

Re: Review Request 53308: Added new hook for modifying the executor environment.

--- > > (Updated Oct. 31, 2016, 8:49 a.m.) > > > Review request for mesos, Adam B, Jie Yu, Kapil Arya, and Joseph Wu. > > > Bugs: MESOS-6396 > https://issues.apache.org/jira/browse/MESOS-6396 > > > Repository: mesos

Re: Review Request 53998: Fixed hook to allow for executor environment modifications.

/slave/containerizer/docker.cpp (lines 1148 - 1154) <https://reviews.apache.org/r/53998/#comment226874> Do we need to worry about this part for https://issues.apache.org/jira/browse/MESOS-6566 ? - Adam B On Nov. 22, 2016, 12:26 p.m., Till Toenshoff

Re: Review Request 53877: Added temporary file environment passing towards docker.

(lines 574 - 582) <https://reviews.apache.org/r/53877/#comment226853> Looks like you're doing the close in both the `if(write.isError())` above and here below. You could pull it out of the if and just do it immediately after the write, regardless of error. - Adam B On Nov. 22, 201

Re: Review Request 53541: WIP: Added authorization actions for debug API.

> Remind me, are we guaranteed to have an executor_info->command.user at this point? Or do we need to check FrameworkInfo.user as a backup? src/authorizer/local/authorizer.cpp (line 999) <https://reviews.apache.org/r/53541/#comment226353> "specialized" - Adam B

Re: Review Request 53541: Added authorization actions for debug API.

) <https://reviews.apache.org/r/53541/#comment226127> What are these exceptional cases? What is an authorizer supposed to do with no object metadata at all? - Adam B On Nov. 8, 2016, 4:43 p.m., Alexander Rojas wrote: > > ---

Re: Review Request 52600: Enable multiple field based authorization in the authorizer interface.

> On Nov. 15, 2016, 6:53 p.m., Adam B wrote: > > Looks great! Just a couple of nits. Fixing the nits myself and committing. > On Nov. 15, 2016, 6:53 p.m., Adam B wrote: > > include/mesos/authorizer/authorizer.proto, lines 68-69 > > <https://reviews.apache.org/r

Re: Review Request 53058: Added tests for whole protobuf message based authorization.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53058/#review156018 --- Ship it! Ship It! - Adam B On Nov. 7, 2016, 7:34 a.m

Re: Review Request 52600: Enable multiple field based authorization in the authorizer interface.

> On Nov. 15, 2016, 6:53 p.m., Adam B wrote: > > src/authorizer/local/authorizer.cpp, lines 231-233 > > <https://reviews.apache.org/r/52600/diff/7/?file=1555485#file1555485line231> > > > > Ooh, only 2 left using 'value'. I wonder if we can ev

Re: Review Request 52600: Enable multiple field based authorization in the authorizer interface.

breaks, for consistency src/authorizer/local/authorizer.cpp (line 332) <https://reviews.apache.org/r/52600/#comment226107> nit: newline before the breaks, for consistency - Adam B On Nov. 7, 2016, 7:29 a.m., Alexander Rojas wrote: > > ---

Re: Review Request 53057: Updates calls to the authorizer to use whole protobuf messages.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53057/#review156016 --- Ship it! Flawless. - Adam B On Nov. 7, 2016, 7:30 a.m

Re: Review Request 52600: Enable multiple field based authorization in the authorizer interface.

resource->disk->persistence->principal`? - Adam B On Oct. 24, 2016, 1:31 a.m., Alexander Rojas wrote: > > --- > This is an automatically generated e-mai

Re: Review Request 51505: Replace http::get with http::request in mesos project.

tps://reviews.apache.org/r/51505/#comment222310> Please, if there are multiple layers of function nesting, wrap after the `=` so we can see them all lined up properly. - Adam B On Aug. 29, 2016, 10:36 p.m., Yongqiao Wang

Re: Review Request 51495: Replace http::get with http::request in libprocess project.

/http_tests.cpp (lines 204 - 205) <https://reviews.apache.org/r/51495/#comment222305> We generally prefer to wrap (2-space indent) after the `=` whenever reasonable. Please fix here and everywhere in these patches. - Adam B On Aug. 29, 2016, 10:27 p.m., Yongqiao Wang

Re: Review Request 51493: Add the query parameter in createRequest.

and bubble up appropriately as a failed Future. - Adam B On Aug. 29, 2016, 3:47 p.m., Yongqiao Wang wrote: > > --- > This is an automatically generated e-mail. To reply, vis

Re: Review Request 52034: Disabled error dialog in WebUI when user is unauthorized to see metrics.

aster/static/js/controllers.js (lines 404 - 406) <https://reviews.apache.org/r/52034/#comment219759> Anything else we'd want to do instead for unauthorized users? I guess we'll just show empty/zero cluster metrics? That's better than the continuous error dialog at least

Re: Review Request 52042: Changed error message in pailer if user is unauthorized.

haven't tried it personally yet. src/webui/master/static/js/jquery.pailer.js (line 139) <https://reviews.apache.org/r/52042/#comment219758> s/SSS/SS/ - Adam B On Sept. 20, 2016, 12:41 a.m., Alexander Rojas wrote: > > ---

Re: Review Request 50723: Fixed the master to recover resources/update state for orphan tasks.

> On Aug. 3, 2016, 12:58 p.m., Adam B wrote: > > src/master/master.cpp, lines 5164-5166 > > <https://reviews.apache.org/r/50723/diff/1/?file=1460860#file1460860line5164> > > > > Maybe increment `metrics->invalid_status_updates` in the else case h

Re: Review Request 50723: Fixed the master to recover resources/update state for orphan tasks.

tps://reviews.apache.org/r/50723/#comment210692> Maybe increment `metrics->invalid_status_updates` in the else case here? And log here, as before? - Adam B On Aug. 2, 2016, 2:45 p.m., Anand Mazumdar wrote: > > --- > This i

Re: Review Request 50329: Added readonly/readwrite authentication support to endpoint help.

tps://reviews.apache.org/r/50329/#comment208999> Let's make this Realm, and then we can include "mesos-master-scheduler" for `/v1/scheduler` - Adam B On July 22, 2016, 12:50 a.m., Greg Mann wrote: > > ---

Re: Review Request 50332: Updated CHANGELOG for new HTTP authentication flags.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50332/#review143206 --- Ship it! Ship It! - Adam B On July 22, 2016, 12:55 a.m

Re: Review Request 50333: Updated upgrades.md for new HTTP authentication flags.

tps://reviews.apache.org/r/50333/#comment208998> Sounds great! I might add a couple of examples of read-only endpoints, "like /state or /flags." - Adam B On July 22, 2016, 1:10 a.m., Greg Mann wrote: > > --- >

Re: Review Request 50322: Added readonly/readwrite auth flags to the docs.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50322/#review143203 --- Ship it! Ship It! - Adam B On July 21, 2016, 11:29 p.m

Re: Review Request 50320: Refactor common HTTP authenticator initialize into helper function.

t; This is called 'credentials' in the doxygen and the cpp - Adam B On July 22, 2016, 12:55 a.m., Zhitao Li wrote: > > --- > This is an automatically generated e-mail. To reply, vi

Re: Review Request 50223: Separate AuthN for readonly and readwrite endpoints in Mesos.

> On July 21, 2016, 7:38 p.m., Adam B wrote: > > src/slave/slave.cpp, lines 443-451 > > <https://reviews.apache.org/r/50223/diff/4/?file=1448838#file1448838line443> > > > > This logic seems to be missing from your new patch. > > Zhitao Li wrote: >

Re: Review Request 50320: Refactor common HTTP authenticator initialize into helper function.

91> Just move the constant to common/http.hpp and use it here. No need for an extra Option check. src/common/http.cpp (lines 897 - 899) <https://reviews.apache.org/r/50320/#comment208987> return Error() - Adam B On July 21, 2016, 9:45 p.m., Zhitao Li wrote: > > ---

Re: Review Request 50223: Separate AuthN for readonly and readwrite endpoints in Mesos.

8983> "Test unauthenticated GET requests on various endpoints when authentication is disabled for read-only endpoints." - Adam B On July 21, 2016, 11:21 p.m., Zhitao Li wrote: > > --- > This is an automat

Re: Review Request 50223: Separate AuthN for readonly and readwrite endpoints in Mesos.

> On July 21, 2016, 7:38 p.m., Adam B wrote: > > src/slave/slave.cpp, lines 410-415 > > <https://reviews.apache.org/r/50223/diff/4/?file=1448838#file1448838line410> > > > > This error is gone? > > Zhitao Li wrote: > Yes, because I refactored the

Re: Review Request 50223: Separate AuthN for readonly and readwrite endpoints in Mesos.

registerHttpAuthenticator` and split it in there? src/slave/slave.cpp (lines 370 - 374) <https://reviews.apache.org/r/50223/#comment208954> This error is gone now? src/slave/slave.cpp <https://reviews.apache.org/r/50223/#comment208953> This logic seems

Re: Review Request 50277: Separate readonly and readwrite realms in libprocess.

) <https://reviews.apache.org/r/50277/#comment208907> Please alphabetize these: - Future - READWRITE - UPID - Adam B On July 21, 2016, 11:13 a.m., Zhitao Li wrote: > > --- > This is an automatically gener

Re: Review Request 50277: Separate readonly and readwrite realms in libprocess.

2016, 11:13 a.m., Zhitao Li wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50277/ > --- > > (Updated July 21, 2016, 11:13 a.m.) > > > Review r

Re: Review Request 50277: Separate readonly and readwrite realms in libprocess.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50277/#review143144 --- Ship it! Ship It! - Adam B On July 21, 2016, 11:13 a.m

Re: Review Request 50257: Used LIBPROCESS_SSL_ instead of SSL_ as the prefix for ssl support.

these environment variables?!? - Adam B On July 20, 2016, 1:34 p.m., Jie Yu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.a

Re: Review Request 50257: Used LIBPROCESS_SSL_ instead of SSL_ as the prefix for ssl support.

> On July 20, 2016, 2:39 p.m., Adam B wrote: > > Won't this break upgrades for any user already using these environment > > variables?!? Nevermind.. premature panic. Now I see ``` // To be backward compatible, for each environment variable prefixed // by SSL_, we genera

Re: Review Request 49794: Added texts for authorization in endpoint docs for '/files/debug'.

endpoints fresh just before cutting rc2: commit 35852ce16a2375079d2cfe9292641ba4da16a16a Author: Vinod Kone Date: Thu Jul 7 09:47:08 2016 -0500 Generated endpoints. - Adam B On July 8, 2016, 1:54 a.m., Abhishek Dasgupta wrote

Re: Review Request 32700: Removed FrameworkID from FrameworkState.

ee to reopen if you rebase and update. Or just open a new RR. - Adam B On Aug. 1, 2015, 12:13 p.m., Kapil Arya wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.a

Re: Review Request 49308: Added agent and scheduler authentication backoff.

> On July 6, 2016, 11:21 a.m., Adam B wrote: > > src/sched/sched.cpp, line 483 > > <https://reviews.apache.org/r/49308/diff/3/?file=1437634#file1437634line483> > > > > Seems `failedAuthentications` is never 0 here (since you increment it > > just befor

Re: Review Request 49308: Added agent and scheduler authentication backoff.

241) <https://reviews.apache.org/r/49308/#comment206417> s/authentication/authenticate/ src/slave/flags.cpp (line 242) <https://reviews.apache.org/r/49308/#comment206418> s/1str/1st/ - Adam B On July 6, 2016, 6:58 a.m., Benjamin Bannier wrote: > >

Re: Review Request 49600: Added authz to /files/debug endpoint.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49600/#review141035 --- Ship it! Ship It! - Adam B On July 6, 2016, 8:13 a.m

Re: Review Request 49600: Added authz to /files/debug endpoint.

is up to you and your shepherd to decide. `this` should go anyway though. > > Abhishek Dasgupta wrote: > And I myself was asked to use defer > [here](https://reviews.apache.org/r/49446/diff/1?file=1434166#file1434166line2793).Adam > , any comment?? > > Adam B wrote: >

Re: Review Request 49600: Added authz to /files/debug endpoint.

> On July 5, 2016, 3:04 a.m., Adam B wrote: > > src/files/files.cpp, lines 89-90 > > <https://reviews.apache.org/r/49600/diff/1/?file=1436468#file1436468line89> > > > > Why did these have to move? > > Abhishek Dasgupta wrote: > Or else, it was

Re: Review Request 49600: Added authz to /files/debug endpoint.

> On July 5, 2016, 3:04 a.m., Adam B wrote: > > src/files/files.hpp, line 65 > > <https://reviews.apache.org/r/49600/diff/1/?file=1436467#file1436467line65> > > > > Shame we have to perpetuate these naked pointers. Any chance we could > > use a `sh

Re: Review Request 49600: Added authz to /files/debug endpoint.

July 5, 2016, 1:08 p.m., Abhishek Dasgupta wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49600/ > --- &

Re: Review Request 49308: Added agent and scheduler authentication backoff.

ilover? - Adam B On June 29, 2016, 12:08 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49308/ >

Re: Review Request 49359: Fixed minor style issues.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49359/#review140744 --- Ship it! Ship It! - Adam B On June 29, 2016, 1:39 a.m

Re: Review Request 49600: Added authz to /files/debug endpoint.

src/tests/files_tests.cpp (lines 537 - 539) <https://reviews.apache.org/r/49600/#comment206101> You're neither waiting on nor validating this `request`. Either do so, or remove it and the FutureArg that sets it. - Adam

Re: Review Request 49257: Added documentation on coarse grain authorization for endpoints.

g/r/49257/#comment205449> s/togge/toggle/ - Adam B On June 28, 2016, 8:12 a.m., Alexander Rojas wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.

Re: Review Request 49369: Introduced authorization based filtering for /roles.

t) src/master/http.cpp (line 2531) <https://reviews.apache.org/r/49369/#comment205422> Let's not squish this line together. The `);});` is pretty confusing. Please wrap after `{` and before `}`. src/master/http.cpp (line 2546) <https://reviews.apache.org/r/49369/#comment205423>

Re: Review Request 49313: Added the VIEW_FLAGS authorization action.

n June 28, 2016, 3:51 p.m., Alexander Rojas wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49313/ > --- > > (Updated Jun

Re: Review Request 49313: Added the VIEW_FLAGS authorization action.

/reviews.apache.org/r/49313/#comment205282> double-blank line src/slave/http.cpp (line 552) <https://reviews.apache.org/r/49313/#comment205283> s/query this endpoint/view all flags/ src/slave/http.cpp (lines 573 - 574) <https://reviews.apache.org/r/49313/#comment205284> doubl

Re: Review Request 49196: Disabled authorization from the `/flags` endpoints.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49196/#review139755 --- Ship it! Ship It! - Adam B On June 27, 2016, 3 a.m

Re: Review Request 49201: Added validation for the `get_endpoints` ACL.

ir("/metrics/snapshot", getEndpoint)); - Adam B On June 24, 2016, 7:26 a.m., Alexander Rojas wrote: > > --- > This is an automatically generated e-mail. To reply, vis

Re: Review Request 49131: Fixed failed authorization of '/files/*' endpoints.

tps://reviews.apache.org/r/49131/#comment204450> You don't even need the if clause anymore, since strings::remove() is a noop when "/" isn't the suffix. - Adam B On June 23, 2016, 11:06 a.m., Greg Mann wrote: > > --- > T

Re: Review Request 49082: Enabled fine grained authorization in the Agent.

committing. Anybody else have any last-minute concerns? src/slave/http.cpp (lines 127 - 129) <https://reviews.apache.org/r/49082/#comment204440> "Filtered representation of Executor. Tasks within this executor are filtered based on whether the user is authorized to view them."

Re: Review Request 49132: Fixed an unsafe callback in the Master.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49132/#review139244 --- Ship it! Ship It! - Adam B On June 22, 2016, 11:08 p.m

Re: Review Request 49082: Enabled fine grained authorization in the Agent.

> On June 23, 2016, 5:14 a.m., Adam B wrote: > > src/slave/http.cpp, line 150 > > <https://reviews.apache.org/r/49082/diff/3/?file=1428527#file1428527line150> > > > > Is it safe to pass `this` through here? Could you instead just pass > > immutable c

Re: Review Request 49131: Fixed failed authorization of '/files/*' endpoints.

::remove` if you like, or I'm also comfortable committing it like this. src/files/files.cpp (lines 321 - 323) <https://reviews.apache.org/r/49131/#comment204392> one-liner: `string trimmedPath = strings::remove(requestedPath, "/", strings::SUFFIX);` - Adam B On June 22,

Re: Review Request 49082: Enabled fine grained authorization in the Agent.

082/#comment204386> s/this->// src/tests/slave_authorization_tests.cpp (line 315) <https://reviews.apache.org/r/49082/#comment204388> This seems redundant with the `EXPECT_EQ(1u, frameworks.values.size());` check below. Remove this one. src/tests/slave_authorization_tests.c

Re: Review Request 49082: Enabled fine grained authorization in the Agent.

g/r/49082/#comment204343> Looks like this and all the approveViewXInfos are copied exactly from src/master/http.cpp. Any chance you can move them somewhere common to reduce code duplication? - Adam B On June 22, 2016, 7:49 a.m., Alexander Rojas

Re: Review Request 49049: Restored the continuation logic fix.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49049/#review138969 --- Ship it! Ship It! - Adam B On June 21, 2016, 4 p.m

Re: Review Request 46872: Updated quota.md and weights.md for set quota and update weight.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46872/#review138318 --- Ship it! Ship It! - Adam B On May 2, 2016, 3:35 p.m

Re: Review Request 46872: Updated quota.md and weights.md for set quota and update weight.

ay 2, 2016, 3:35 p.m., Guangya Liu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46872/ > --- > > (Updated May

Re: Review Request 46875: Some cleanup in weights_handler.cpp.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46875/#review138309 --- Ship it! Ship It! - Adam B On April 30, 2016, 11:52 p.m

Re: Review Request 48744: Changed agent and scheduler authentication timeouts to ensure progress.

onential backoff would work better than a hardcoded sched/slave timeout. - Adam B On June 15, 2016, 2:20 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated

Re: Review Request 48781: Marked some optional fields in acls.proto as required.

pread like wildfire. Glad we've cleared up the API since then. I don't see why any objects in acls.proto should be optional. - Adam B On June 16, 2016, 11:05 p.m., Alexander Rojas wrote: > > --- > This is an automatical

Re: Review Request 48781: Marked some optional fields in state.json as required.

tps://reviews.apache.org/r/48781/#comment203306> required? include/mesos/authorizer/acls.proto (line 178) <https://reviews.apache.org/r/48781/#comment203307> required? - Adam B On June 16, 2016, 3:06 a.m., Alexander

Re: Review Request 48764: Refactored sandbox authorization logic to use ObjectAuthorizer.

// Construct authorization object. ``` - Adam B On June 16, 2016, 8:14 a.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To repl

Re: Review Request 48497: Added documentation on starting to use acls.

g/r/48497/#comment203233> Will this link work on the website too? I think not. Need to point directly to source on github? - Adam B On June 16, 2016, 6:44 a.m., Joerg Schad wrote: > > --- > This is an automatically gener

Re: Review Request 48495: Added missing 'get_weights' actions to authorization.md.

tion.md (line 134) <https://reviews.apache.org/r/48495/#comment203229> s/executors/tasks/ - Adam B On June 15, 2016, 12:53 a.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit

Re: Review Request 48497: Added documentation on starting to use acls.

enerated e-mail. To reply, visit: > https://reviews.apache.org/r/48497/ > --- > > (Updated June 14, 2016, 6:20 a.m.) > > > Review request for mesos, Adam B and Neil Conway. > > > Bugs: MESOS-5583 > https://issues.apache.org/jir

Re: Review Request 48497: Added documentation on starting to use acls.

} ``` (but you can move permissive back to the top if you like) support/acls_template.json (lines 26 - 35) <https://reviews.apache.org/r/48497/#comment202861> Remove duplicate (this may have once been shutdown_frameworks) - Adam B On June 14, 2016, 6:20 a.m., Joer

Re: Review Request 48496: Fixed misleading acls example in authorization.md.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48496/#review137671 --- Ship it! Ship It! - Adam B On June 14, 2016, 6:21 a.m

Re: Review Request 48495: Added missing 'get_weights' actions to authorization.md.

2) are wrong. The subject in this ACL is not the "Unix user...", but the HTTP user. And we're talking about the LocalAuthorizer here, so the Object (column 3) should have nothing to do with a WhateverInfo, but instead should be the "Operating system user whose whatever can

Re: Review Request 48566: Fixed continuation logic for sandbox authorization.

this, path](bool authorized) -> Future { if (authorized) { ``` - Adam B On June 10, 2016, 9:19 p.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visi

Re: Review Request 48563: Fixed continuation logic in endpoint authorization.

Owned>& approvers) -> Response { // Get approver from tuple. Owned frameworksApprover; ``` - Adam B On June 10, 2016, 9:28 p.m., Joerg Schad wrote: > > --- > Th

Re: Review Request 47374: Separated mesos test helpers into a separate library.

> On May 23, 2016, 5:09 p.m., Adam B wrote: > > src/Makefile.am, lines 1937-1938 > > <https://reviews.apache.org/r/47374/diff/3/?file=1384789#file1384789line1937> > > > > Why does libmesos_tests_la_SOURCES need to include qos_controllers code? > > Jose

<    1   2   3   4   5   6   7   8   >