Re: Review Request 61723: Bumped version to 1.5.0.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On Aug. 17, 2017, 4:41 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61723/
> ---
> 
> (Updated Aug. 17, 2017, 4:41 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Bumped version to 1.5.0.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 75468abd3641171470a0b8759179dc86496820b5 
>   configure.ac ee3818d404013e172bc51f11e8c5792cb335a22c 
> 
> 
> Diff: https://reviews.apache.org/r/61723/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 60426: Marked 1.2.2 as WIP in CHANGELOG.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On June 26, 2017, 3:42 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60426/
> ---
> 
> (Updated June 26, 2017, 3:42 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Marked 1.2.2 as WIP in CHANGELOG.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 7536717b85bb188d8a463b36ce75e4e1cecc445d 
> 
> 
> Diff: https://reviews.apache.org/r/60426/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 60405: Documented resource format in agent <-> master protocol.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On June 24, 2017, 10:30 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60405/
> ---
> 
> (Updated June 24, 2017, 10:30 a.m.)
> 
> 
> Review request for mesos, Adam B and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Documented resource format in agent <-> master protocol.
> 
> 
> Diffs
> -
> 
>   src/messages/messages.proto 2c086263fdcee4d54a76a61379c2d4dba5271d23 
>   src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c 
> 
> 
> Diff: https://reviews.apache.org/r/60405/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 60405: Documented resource format in agent <-> master protocol.

[Adam B]

> On June 23, 2017, 7:21 p.m., Adam B wrote:
> > src/slave/slave.cpp
> > Lines 1405-1407 (original), 1405-1408 (patched)
> > <https://reviews.apache.org/r/60405/diff/2/?file=1761692#file1761692line1405>
> >
> > If this agent has refinements, and we send post format to an old 
> > master, will the old master safely reject the registration, crash and burn, 
> > or something in between?
> 
> Neil Conway wrote:
> The master will basically consider the resources to be unreserved; 
> because the master and agent will have inconsistent views of the resource 
> state at the agent, this will cause problems.
> 
> Since you need a new master to create reservation refinements in the 
> first place, you can only arrive in this situation by:
> 
> Upgrading master
> Upgrading agent
> Creating res refinement
> Downgrading master
> 
> Which arguably falls under the "don't downgrade if you are using new 
> featues" bucket. But yes, this is certainly unfortunate. Hard to prevent 
> without introducing something similar to master capabilities, which we 
> definitely need (MESOS-5675). I'll drop this issue for now, since AFAIK 
> there's not much we can do to improve this in the short term.

Sounds good. Thanks for the explanation. At least we don't crash..


> On June 23, 2017, 7:21 p.m., Adam B wrote:
> > src/slave/slave.cpp
> > Line 1408 (original), 1412-1414 (patched)
> > <https://reviews.apache.org/r/60405/diff/2/?file=1761692#file1761692line1412>
> >
> > We could at least log an INFO/WARN if we aren't able to downgrade, and 
> > still send it anyway.
> 
> Neil Conway wrote:
> Hmm, not sure a warning/log is warranted here. In the common case 
> (refined reservation, >= 1.4 master, >= 1.4 agent), we will fail to downgrade 
> the resources, but that is fine and expected. Should we really be cluttering 
> the logs with this information?

Good point. Don't want to over-log in the common case. Would be nice if we had 
master capabilities so we only had to log failure-to-downgrade when we're 
(re)registering with an old master.


- Adam


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


On June 24, 2017, 10:30 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60405/
> ---
> 
> (Updated June 24, 2017, 10:30 a.m.)
> 
> 
> Review request for mesos, Adam B and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Documented resource format in agent <-> master protocol.
> 
> 
> Diffs
> -
> 
>   src/messages/messages.proto 2c086263fdcee4d54a76a61379c2d4dba5271d23 
>   src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c 
> 
> 
> Diff: https://reviews.apache.org/r/60405/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 60407: Avoided master crash on agent re-registration.

[Adam B]

> On June 23, 2017, 7:35 p.m., Adam B wrote:
> >

What's the JIRA for this bug?


- Adam


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


On June 23, 2017, 6:48 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60407/
> ---
> 
> (Updated June 23, 2017, 6:48 p.m.)
> 
> 
> Review request for mesos, Adam B and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When validating the agent's ReregisterSlaveMessage, the master's
> validation code neglected to account for the fact that the task
> resources might not be in post-refinement format (e.g., if the agent
> does not support reservation refinement). This lead to a `CHECK` failure
> during validation.
> 
> Fix this by relaxing the validation of ReregisterSlaveMessage so that we
> do not depend on the task resources being in post-refinement
> format. This means validation of ReregisterSlaveMessage will be less
> effective, but since it is best-effort anyway, this seems tolerable.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 33e9ff7db9e2789cbb2d6dfd015288dfa1faa7c5 
> 
> 
> Diff: https://reviews.apache.org/r/60407/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 60407: Avoided master crash on agent re-registration.

[Adam B]

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




src/master/validation.cpp
Line 370 (original), 370-372 (patched)
<https://reviews.apache.org/r/60407/#comment253088>

Even if we can't `validateDynamicReservationInfo` (or  
`validateDiskInfo`?), would it be worthwhile to `validateGpus`? Maybe 
clone/parameterize `resource::validate` to validate what we can?


- Adam B


On June 23, 2017, 6:48 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60407/
> ---
> 
> (Updated June 23, 2017, 6:48 p.m.)
> 
> 
> Review request for mesos, Adam B and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When validating the agent's ReregisterSlaveMessage, the master's
> validation code neglected to account for the fact that the task
> resources might not be in post-refinement format (e.g., if the agent
> does not support reservation refinement). This lead to a `CHECK` failure
> during validation.
> 
> Fix this by relaxing the validation of ReregisterSlaveMessage so that we
> do not depend on the task resources being in post-refinement
> format. This means validation of ReregisterSlaveMessage will be less
> effective, but since it is best-effort anyway, this seems tolerable.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 33e9ff7db9e2789cbb2d6dfd015288dfa1faa7c5 
> 
> 
> Diff: https://reviews.apache.org/r/60407/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 60406: Added sanity check to resource downgrade on agent registration.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On June 23, 2017, 6:48 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60406/
> ---
> 
> (Updated June 23, 2017, 6:48 p.m.)
> 
> 
> Review request for mesos, Adam B and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `SlaveInfo.resources` should always be representable in the
> "pre-reservation-refinement" format, so `downgradeResources` should
> always succeed.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c 
> 
> 
> Diff: https://reviews.apache.org/r/60406/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 60405: Documented resource format in agent <-> master protocol.

[Adam B]

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



Just a couple of clarifying questions before we commit these comments.


src/messages/messages.proto
Line 436 (original), 439-442 (patched)
<https://reviews.apache.org/r/60405/#comment253085>

Since this is a `repeated Resource` list, is it possible that some 
resources in the list are pre-format and some are post? Or will they all be 
post if any resource has a refinement?



src/slave/slave.cpp
Lines 1405-1407 (original), 1405-1408 (patched)
<https://reviews.apache.org/r/60405/#comment253086>

If this agent has refinements, and we send post format to an old master, 
will the old master safely reject the registration, crash and burn, or 
something in between?



src/slave/slave.cpp
Line 1408 (original), 1412-1414 (patched)
<https://reviews.apache.org/r/60405/#comment253087>

We could at least log an INFO/WARN if we aren't able to downgrade, and 
still send it anyway.


- Adam B


On June 23, 2017, 6:48 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60405/
> ---
> 
> (Updated June 23, 2017, 6:48 p.m.)
> 
> 
> Review request for mesos, Adam B and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Documented resource format in agent <-> master protocol.
> 
> 
> Diffs
> -
> 
>   src/messages/messages.proto 2c086263fdcee4d54a76a61379c2d4dba5271d23 
>   src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c 
> 
> 
> Diff: https://reviews.apache.org/r/60405/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 60404: Documented the content of the `SlaveInfo.resources` field.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On June 23, 2017, 6:49 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60404/
> ---
> 
> (Updated June 23, 2017, 6:49 p.m.)
> 
> 
> Review request for mesos, Adam B and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Documented the content of the `SlaveInfo.resources` field.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 38449da44a5288afc7e2ef17ef5e1380ecabd5d2 
>   include/mesos/v1/mesos.proto 81eca47ce1074f5e522a0410f01ab2b2cfc76aa2 
> 
> 
> Diff: https://reviews.apache.org/r/60404/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 58964: Added authorization support for operator endpoints.

[Adam B]

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



Didn't get to the tests, but here's something to start with.


include/mesos/authorizer/acls.proto
Line 354 (original), 354 (patched)
<https://reviews.apache.org/r/58964/#comment248672>

Should this be `agents` plural?



include/mesos/authorizer/acls.proto
Lines 364 (patched)
<https://reviews.apache.org/r/58964/#comment248660>

Why do we think `machines` is the entity we want to authorize on? What if 
we decide we want to authorize on `schedules` in the future? This required 
field isn't very flexible.
Also, why not `agents` like in `RegisterAgent` above. Is there a 
distinction between agents and machines?



include/mesos/authorizer/acls.proto
Lines 377 (patched)
<https://reviews.apache.org/r/58964/#comment248661>

s/in/on/



include/mesos/authorizer/acls.proto
Lines 387 (patched)
<https://reviews.apache.org/r/58964/#comment248662>

s/in/on/



include/mesos/authorizer/acls.proto
Lines 391 (patched)
<https://reviews.apache.org/r/58964/#comment248663>

Missed the comment on this object entity



include/mesos/authorizer/acls.proto
Lines 394-395 (patched)
<https://reviews.apache.org/r/58964/#comment248664>

s/status of  maintenance in/maintenance status of/



include/mesos/authorizer/authorizer.proto
Lines 58 (patched)
<https://reviews.apache.org/r/58964/#comment248673>

Unused?!?



include/mesos/authorizer/authorizer.proto
Lines 204 (patched)
<https://reviews.apache.org/r/58964/#comment248665>

s/he's/is/g in all of these comments



include/mesos/authorizer/authorizer.proto
Lines 213 (patched)
<https://reviews.apache.org/r/58964/#comment248666>

"on all nodes or none" in each of these comments



include/mesos/authorizer/authorizer.proto
Lines 221 (patched)
<https://reviews.apache.org/r/58964/#comment248667>

"of all nodes or none"



src/authorizer/local/authorizer.cpp
Lines 391-398 (original), 391-406 (patched)
<https://reviews.apache.org/r/58964/#comment248668>

Why not merge these all into a single case body?



src/authorizer/local/authorizer.cpp
Lines 866-867 (original), 879-885 (patched)
<https://reviews.apache.org/r/58964/#comment248669>

These should probably be alpha-sorted as well



src/authorizer/local/authorizer.cpp
Lines 1024-1025 (original), 1042-1048 (patched)
<https://reviews.apache.org/r/58964/#comment248670>

These should probably be alpha-sorted into the list above.


- Adam B


On May 12, 2017, 5:51 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58964/
> ---
> 
> (Updated May 12, 2017, 5:51 a.m.)
> 
> 
> Review request for mesos, Adam B and Greg Mann.
> 
> 
> Bugs: MESOS-7415
> https://issues.apache.org/jira/browse/MESOS-7415
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the actions `UPDATE_MAINTENANCE_SCHEDULE`,
> `GET_MAINTENANCE_SCHEDULE`, `START_MAINTENANCE`, `STOP_MAINTENANCE`
> and `GET_MAINTENANCE_STATUS` to the authorizer API as well as the
> necesary code to handle these new actions.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> ae0b1ea2e6417d186b1606542d75f3a20e0811db 
>   include/mesos/authorizer/authorizer.proto 
> c9184d151befa4cea9bdebb36a315c760e6424b2 
>   src/authorizer/local/authorizer.cpp 
> 89aaf4b712d337d519445c922606789c334e5101 
>   src/tests/authorization_tests.cpp 32aa6ac4db7854507127ea2fb88b3e92daa277c0 
> 
> 
> Diff: https://reviews.apache.org/r/58964/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 58955: Adds authorization for the master's v1 API call SET_LOGGING_LEVEL.

[Adam B]

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



Looks pretty good. Just some questions about the v0 /logging/toggle endpoint


src/master/http.cpp
Lines 2056 (patched)
<https://reviews.apache.org/r/58955/#comment248656>

Should we modify the v0 `/logging/toggle` endpoint to use this 
authorization::action instead of authorization::GET_ENDPOINT_WITH_PATH ? What's 
the compatibility/deprecation process there?



src/master/http.cpp
Lines 2062-2064 (patched)
<https://reviews.apache.org/r/58955/#comment248654>

I don't care what clang-format says, this is much uglier than my preferred 
wrapping:
```
  [level, duration](const Owned& approver)
  -> Future {
Try approved = approver->approved((ObjectApprover::Object()));
```



src/tests/api_tests.cpp
Lines 772 (patched)
<https://reviews.apache.org/r/58955/#comment248657>

Is there a v0 equivalent of this test for `/logging/toggle`?


- Adam B


On May 12, 2017, 2:28 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58955/
> ---
> 
> (Updated May 12, 2017, 2:28 a.m.)
> 
> 
> Review request for mesos, Adam B and Greg Mann.
> 
> 
> Bugs: MESOS-7414
> https://issues.apache.org/jira/browse/MESOS-7414
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enables authorization in the v1 API call `SET_LOGGING_LEVEL` following
> the existing implementation for the agent call with the same name.
> Likewise it reuses the authorizer action `SET_LOG_LEVEL`.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp e2590a17044ac019b24a24629428d4ec8adc0c31 
>   src/tests/api_tests.cpp faf3242f9c86d866c4bb5e457fcfe47c1063cc09 
> 
> 
> Diff: https://reviews.apache.org/r/58955/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 58099: Added authorization for frameworks in `GetRoles` v1 API.

[Adam B]

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



Seems like we're adding even more duplicate code into this v1 clone of 
`roles()`. Can you find a way to reduce the redundance?

- Adam B


On May 10, 2017, 6:52 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58099/
> ---
> 
> (Updated May 10, 2017, 6:52 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7260
> https://issues.apache.org/jira/browse/MESOS-7260
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization for frameworks in `GetRoles` v1 API.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp e2590a17044ac019b24a24629428d4ec8adc0c31 
> 
> 
> Diff: https://reviews.apache.org/r/58099/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 58097: Added a test to check framework filtering in /roles endpoint.

[Adam B]

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



Also need a test for the v1 API authz changes in 
https://reviews.apache.org/r/58099


src/tests/master_authorization_tests.cpp
Lines 2148 (patched)
<https://reviews.apache.org/r/58097/#comment248646>

s/frameworks under a roles is/frameworks under a role are/



src/tests/master_authorization_tests.cpp
Lines 2226 (patched)
<https://reviews.apache.org/r/58097/#comment248648>

Please validate the single role name is as expected.


- Adam B


On May 10, 2017, 6:47 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58097/
> ---
> 
> (Updated May 10, 2017, 6:47 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7260
> https://issues.apache.org/jira/browse/MESOS-7260
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test to check framework filtering in /roles endpoint.
> 
> 
> Diffs
> -
> 
>   src/tests/master_authorization_tests.cpp 
> e4233c19b1d9e3e2734259503d0daec4ce243667 
> 
> 
> Diff: https://reviews.apache.org/r/58097/diff/6/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 58096: Added authorization for frameworks in /roles endpoint.

[Adam B]

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



Looks pretty good to me. Just a few minor comments


src/master/http.cpp
Lines 3504 (patched)
<https://reviews.apache.org/r/58096/#comment248641>

What makes a role "active"? Having `active` frameworks registered for that 
role? This function seems to return a list of all roles that have one or more 
weights, quota, reservations, or registered frameworks associated with them. 
More accurate would be to call it `knownRoles`.



src/master/http.cpp
Lines 3507 (patched)
<https://reviews.apache.org/r/58096/#comment248639>

Nit: `ObjectApprover` singular, since it's a single approver that can 
approve/deny multiple frameworks, not an approver per framework.



src/master/http.cpp
Lines 3524 (patched)
<https://reviews.apache.org/r/58096/#comment248644>

`futures` is an over-vague variable name, especially since neither are 
Futures by this point. Can we do better?


- Adam B


On May 10, 2017, 9:33 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58096/
> ---
> 
> (Updated May 10, 2017, 9:33 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7260
> https://issues.apache.org/jira/browse/MESOS-7260
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While /roles displays a list of frameworksIds that register with
> a role, it did NOT filter them based on VIEW_FRAMEWORK ACL, which
> impose a security risk. This patch fixed this issue by taking a
> frameworksApprover in `Master::Http::roles()` which is used to
> filter framework IDs.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp e2590a17044ac019b24a24629428d4ec8adc0c31 
> 
> 
> Diff: https://reviews.apache.org/r/58096/diff/7/
> 
> 
> Testing
> ---
> 
> see next patch in the chain.
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 58095: Refactored `Master::Http::roles` to use `jsonify`.

[Adam B]

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



Re: "Testing Done": Can you point to the unit test that proves we haven't 
change the output of /roles? I'd love to see the before/after json from a 
manual test on a master with quota and weights set for a couple of roles.


src/common/http.cpp
Lines 510-511 (patched)
<https://reviews.apache.org/r/58095/#comment248615>

Why is this the only `json()` that nees to be in its type's namespace. Does 
QuotaInfo really need to be in the `quota` namespace? Sounds like stuttering to 
me. Not your problem, I guess, but I'd suggest a follow-up JIRA to remove the 
redundant namespacing there. `quota::Info` or `::QuotaInfo` is descriptive 
enough.



src/common/http.cpp
Lines 518-520 (patched)
<https://reviews.apache.org/r/58095/#comment248616>

Were we printing the principal before? Seems like unnecessary info that'll 
just clog up the output.



src/master/http.cpp
Lines 410 (patched)
<https://reviews.apache.org/r/58095/#comment248636>

Not for backwards compatibility, I think, but for roles that only exist in 
weight/quota definitions, without resources reserved or frameworks registered. 
Do we have a test case that exercises this path?



src/master/http.cpp
Lines 412 (patched)
<https://reviews.apache.org/r/58095/#comment248638>

Why a `vector`? Wouldn't it be a `vector` since it's a vector 
of `FrameworkID`s?



src/master/http.cpp
Lines 3492-3493 (original), 3497-3498 (patched)
<https://reviews.apache.org/r/58095/#comment248630>

Fits on one line. No need to wrap.

Also, why rename `filteredRoles` to `activeRoles`? That variable is the 
result of the `_roles(principal)` authorization filtering. Maybe 
`authorizedRoles` if you're going to rename. Or call them `roleNames` since 
it's just the string representation and not the full `Role` object.


- Adam B


On May 10, 2017, 9:32 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58095/
> ---
> 
> (Updated May 10, 2017, 9:32 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Benjamin Mahler, and 
> Michael Park.
> 
> 
> Bugs: MESOS-4732 and MESOS-7260
> https://issues.apache.org/jira/browse/MESOS-4732
> https://issues.apache.org/jira/browse/MESOS-7260
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored `Master::Http::roles` to use `jsonify`.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 93d6088e97c2384f9f6d26e010a501abf2deb43e 
>   src/common/http.cpp 167dce2b9a2d3b68a1df5b4079f701482d34db28 
>   src/master/http.cpp e2590a17044ac019b24a24629428d4ec8adc0c31 
> 
> 
> Diff: https://reviews.apache.org/r/58095/diff/7/
> 
> 
> Testing
> ---
> 
> no functional changes
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 58095: Refactored `Master::Http::roles` to use `jsonify`.

[Adam B]

> On April 27, 2017, 7:58 a.m., Alexander Rojas wrote:
> > src/master/http.cpp
> > Lines 3481-3482 (original), 3456-3457 (patched)
> > <https://reviews.apache.org/r/58095/diff/4/?file=1693998#file1693998line3495>
> >
> > Not yours, but I feel the formatting of this lambda is really off. The 
> > body of the lambda starts a couple of columns earlier than declaration.
> 
> Jay Guo wrote:
> Other indents in this file are off too, e.g. `state()`, `frameworks()`
> 
> Alexander Rojas wrote:
> I see, we should open a JIRA ticket requesting to fix those wrong 
> formattings.

I didn't see a link to a new JIRA for this yet. If it's easier, you can just do 
a follow-up patch, but let's not let bad style spread/linger.


- Adam


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


On May 10, 2017, 9:32 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58095/
> ---
> 
> (Updated May 10, 2017, 9:32 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Benjamin Mahler, and 
> Michael Park.
> 
> 
> Bugs: MESOS-4732 and MESOS-7260
> https://issues.apache.org/jira/browse/MESOS-4732
> https://issues.apache.org/jira/browse/MESOS-7260
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored `Master::Http::roles` to use `jsonify`.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 93d6088e97c2384f9f6d26e010a501abf2deb43e 
>   src/common/http.cpp 167dce2b9a2d3b68a1df5b4079f701482d34db28 
>   src/master/http.cpp e2590a17044ac019b24a24629428d4ec8adc0c31 
> 
> 
> Diff: https://reviews.apache.org/r/58095/diff/7/
> 
> 
> Testing
> ---
> 
> no functional changes
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 59198: Documented that Mesos 1.3.0, 1.2.1 don't allow old agents to register.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On May 11, 2017, 2:10 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59198/
> ---
> 
> (Updated May 11, 2017, 2:10 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Documented that Mesos 1.3.0, 1.2.1 don't allow old agents to register.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 31c21e9a69a6e11429b58a7eddecf7746a284530 
>   docs/upgrades.md 74e74642b411eefb737d575b89a1f206cae62b46 
> 
> 
> Diff: https://reviews.apache.org/r/59198/diff/4/
> 
> 
> Testing
> ---
> 
> Visual inspection, no functional change.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 59198: Documented that Mesos 1.3.0, 1.2.1 don't allow old agents to register.

[Adam B]

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



Couple more nits, then we can ship-it


CHANGELOG
Line 52 (original), 52 (patched)
<https://reviews.apache.org/r/59198/#comment247939>

Still mixing `> 1.0` and `pre-1.0`, so either `s/pre-1.0/0.x/g` or 
`s/pre-1.0/< 1.0/` or `s/> 1.0/1.1+/`



docs/upgrades.md
Line 278 (original), 278 (patched)
<https://reviews.apache.org/r/59198/#comment247940>

Ditto on `> 1.0` and `pre-1.0`.



docs/upgrades.md
Lines 50 (patched)
<https://reviews.apache.org/r/59198/#comment247941>

Should we add the same thing to 1.2.x (with a special note that it only 
applies to `1.2.1+`)?


- Adam B


On May 11, 2017, 12:47 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59198/
> ---
> 
> (Updated May 11, 2017, 12:47 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Documented that Mesos 1.3.0, 1.2.1 don't allow old agents to register.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 31c21e9a69a6e11429b58a7eddecf7746a284530 
>   docs/upgrades.md 74e74642b411eefb737d575b89a1f206cae62b46 
> 
> 
> Diff: https://reviews.apache.org/r/59198/diff/3/
> 
> 
> Testing
> ---
> 
> Visual inspection, no functional change.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 59198: Documented that Mesos 1.3.0, 1.2.1 don't allow old agents to register.

[Adam B]

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



A few thoughts


CHANGELOG
Lines 51-52 (patched)
<https://reviews.apache.org/r/59198/#comment247929>

Can we refer to "pre-1.0" as `0.x`?



CHANGELOG
Lines 52 (patched)
<https://reviews.apache.org/r/59198/#comment247930>

Interoperability between 1.0 masters and 0.28 agents was supported, during 
live upgrade.



CHANGELOG
Lines 300 (patched)
<https://reviews.apache.org/r/59198/#comment247928>

    s/1.3.0/1.2.1/ ?


- Adam B


On May 11, 2017, 11:58 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59198/
> ---
> 
> (Updated May 11, 2017, 11:58 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Documented that Mesos 1.3.0, 1.2.1 don't allow old agents to register.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 31c21e9a69a6e11429b58a7eddecf7746a284530 
>   docs/upgrades.md 74e74642b411eefb737d575b89a1f206cae62b46 
> 
> 
> Diff: https://reviews.apache.org/r/59198/diff/2/
> 
> 
> Testing
> ---
> 
> Visual inspection, no functional change.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 58942: CHANGELOG for 1.3.0 release.

[Adam B]

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




CHANGELOG
Lines 95 (patched)
<https://reviews.apache.org/r/58942/#comment246939>

I bet we can graduate a few more of these. Maybe not the ones that still 
have "Unresolved Critical Issues", but some of the others.


- Adam B


On May 2, 2017, 4:48 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58942/
> ---
> 
> (Updated May 2, 2017, 4:48 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CHANGELOG for 1.3.0 release.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 998f30d576d87e72b1d52e8bd1e43ce6ba67a54b 
> 
> 
> Diff: https://reviews.apache.org/r/58942/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 58942: CHANGELOG for 1.3.0 release.

[Adam B]

> On May 3, 2017, 4:48 a.m., Stephan Erb wrote:
> > CHANGELOG
> > Lines 117 (patched)
> > 
> >
> > I was once confused by this "All issues" title in presence of the 
> > "Unresolved Critical Issues". 
> > 
> > Maybe change it to "Newly resolved Issues" or something similar to make 
> > it clearer?

Great feedback. We only recently added the "Unresolved Critical Issues" and 
other sections above.
I like the sound of "All newly resolved issues:" or "All issues resolved in 
this release:".
We have "All" in there because we don't bother to go through the JIRA-generated 
list to remove the issues already mentioned above; otherwise it could be "Other 
newly resolved issues:"


- Adam


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


On May 2, 2017, 4:48 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58942/
> ---
> 
> (Updated May 2, 2017, 4:48 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CHANGELOG for 1.3.0 release.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 998f30d576d87e72b1d52e8bd1e43ce6ba67a54b 
> 
> 
> Diff: https://reviews.apache.org/r/58942/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 58923: Added new ContainerLaunchInfo task_environment.

[Adam B]

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



The terminology in this area of the code confuses me. Do we have a glossary for 
the different executors, command tasks, etc. somewhere?


include/mesos/slave/containerizer.proto
Lines 166 (patched)
<https://reviews.apache.org/r/58923/#comment246905>

+1, or just drop "already".
And tell me which non-custom executors do decide to pass their environment 
on to the tasks.

Also, the "Additional environment" confuses me. What is it in addition to? 
The new `task_environment`? Comment should explain the relationship.



include/mesos/slave/containerizer.proto
Lines 180-184 (original), 184-191 (patched)
<https://reviews.apache.org/r/58923/#comment246909>

What's the difference between a `task command` and a `command task`?



include/mesos/slave/containerizer.proto
Lines 192 (patched)
<https://reviews.apache.org/r/58923/#comment246907>

What order did you insert this in? I'd say either go numeric and put it at 
the end, or put it next to `environment` so you can explain the difference.


- Adam B


On May 2, 2017, 10:28 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58923/
> ---
> 
> (Updated May 2, 2017, 10:28 a.m.)
> 
> 
> Review request for mesos, Adam B, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-7429
> https://issues.apache.org/jira/browse/MESOS-7429
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> c30b1fc659ee9b3cd343899638ced6408d8b51a2 
> 
> 
> Diff: https://reviews.apache.org/r/58923/diff/1/
> 
> 
> Testing
> ---
> 
> make check and functional testing on chain end.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 57474: Added test for authorization of hierarchical roles.

[Adam B]

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


Fix it, then Ship it!




LGTM. I will fix the tiny typo when I commit.


src/tests/authorization_tests.cpp
Line 1493 (original), 1493 (patched)
<https://reviews.apache.org/r/57474/#comment246237>

s/crete/create/


- Adam B


On April 18, 2017, 6:28 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57474/
> ---
> 
> (Updated April 18, 2017, 6:28 a.m.)
> 
> 
> Review request for mesos, Adam B and Benjamin Bannier.
> 
> 
> Bugs: MESOS-7026
> https://issues.apache.org/jira/browse/MESOS-7026
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds tests for each of the actions which support hierarchical roles.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 3e18c70738b6b7098f37fadebb799a596e76452d 
> 
> 
> Diff: https://reviews.apache.org/r/57474/diff/5/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 57473: Added support for authorization of Hierachical roles.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On April 11, 2017, 3:58 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57473/
> ---
> 
> (Updated April 11, 2017, 3:58 a.m.)
> 
> 
> Review request for mesos, Adam B and Benjamin Bannier.
> 
> 
> Bugs: MESOS-7026
> https://issues.apache.org/jira/browse/MESOS-7026
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds mechanisms to support authorization of hierarchical roles,
> that is, it allows operators to write ACLs of the form `role/%`
> which will enforce the rule for any nested role, e.g. `role/a`,
> `role/b` and such.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> 1c1f912794cfe61112a0e513b217ba3a755f35f1 
> 
> 
> Diff: https://reviews.apache.org/r/57473/diff/7/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 58690: Fixed leak of sensitive data in agent log of docker containerizer.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On April 24, 2017, 5:41 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58690/
> ---
> 
> (Updated April 24, 2017, 5:41 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp c6f7d789345af63bb1f5aadec52b4514d03badd0 
> 
> 
> Diff: https://reviews.apache.org/r/58690/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 57474: Added test for authorization of hierarchical roles.

[Adam B]

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



Test logic looks good, although I question "archbishop" as a principal.
And it doesn't look like you thoroughly reviewed the text in your comments.


src/tests/authorization_tests.cpp
Lines 888 (patched)
<https://reviews.apache.org/r/57474/#comment244705>

s/Cheks/Checks/



src/tests/authorization_tests.cpp
Lines 912 (patched)
<https://reviews.apache.org/r/57474/#comment244706>

When has an archbishop ever also been a king? I was expecting another 
person's name. As a title, "archbishop" makes more sense as a Mesos role than a 
principal.

Archbishop William King was technically both.
https://en.wikipedia.org/wiki/William_King_(bishop)



src/tests/authorization_tests.cpp
Lines 1048 (patched)
<https://reviews.apache.org/r/57474/#comment244708>

s/Not not/Not/
s/irself/itself/



src/tests/authorization_tests.cpp
Lines 1184 (patched)
<https://reviews.apache.org/r/57474/#comment244709>

s/register frameworks/reserve resources/



src/tests/authorization_tests.cpp
Lines 1478 (patched)
<https://reviews.apache.org/r/57474/#comment244710>

"Not not" and "irself" again. Please fix throughout.



src/tests/authorization_tests.cpp
Lines 1597 (patched)
<https://reviews.apache.org/r/57474/#comment244711>

s/register frameworks/create volumes/



src/tests/authorization_tests.cpp
Lines 1866 (patched)
<https://reviews.apache.org/r/57474/#comment244712>

These could uses comments like the others



src/tests/authorization_tests.cpp
Line 1578 (original), 1943 (patched)
<https://reviews.apache.org/r/57474/#comment244714>

"register frameworks" again. Please fix throughout.



src/tests/authorization_tests.cpp
Lines 4522-4524 (patched)
<https://reviews.apache.org/r/57474/#comment244715>

This comment belongs above the previous block of archbishop->king



src/tests/authorization_tests.cpp
Lines 4588 (patched)
<https://reviews.apache.org/r/57474/#comment244719>

s/view any role/update the weight of any role/



src/tests/authorization_tests.cpp
Lines 4737 (patched)
<https://reviews.apache.org/r/57474/#comment244716>

s/get weights/get quotas/g and s/update weights/get quotas/g throughout 
this test.



src/tests/authorization_tests.cpp
Lines 4753 (patched)
<https://reviews.apache.org/r/57474/#comment244718>

s/update the weight/view the quota/



src/tests/authorization_tests.cpp
Lines 4760 (patched)
<https://reviews.apache.org/r/57474/#comment244717>

s/view any role/view quota for any role/


- Adam B


On April 11, 2017, 1:47 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57474/
> ---
> 
> (Updated April 11, 2017, 1:47 a.m.)
> 
> 
> Review request for mesos, Adam B and Benjamin Bannier.
> 
> 
> Bugs: MESOS-7026
> https://issues.apache.org/jira/browse/MESOS-7026
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds tests for each of the actions which support hierarchical roles.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 3e18c70738b6b7098f37fadebb799a596e76452d 
> 
> 
> Diff: https://reviews.apache.org/r/57474/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 57474: Added test for authorization of hierarchical roles.

[Adam B]

> On March 20, 2017, 10:28 a.m., Benjamin Bannier wrote:
> > LGTM but for one non-functional issue.
> > 
> > I believe it might make sense to combine this into one change set with the 
> > previous patch since we e.g., likely wouldn't be interested in backporting. 
> > Maybe @adam-mesos has some idea.
> 
> Alexander Rojas wrote:
> I'm inclined to agree with you. Other reviewers in the past have 
> requested to break patches betweent he functional changes and tests. Let's 
> @adam-mesos give us a directive here.

Either way's fine with me. It's nice having them in separate reviews so (as a 
reviewer) I can review all but the tests in my first pass, but I also do that 
based on filenames in a combined impl+tests patch. As committer, though I may 
stamp a ShipIt on the impl patch while still polishing up the test patch, I can 
still choose to wait to commit both in the same push.


- Adam


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


On April 11, 2017, 1:47 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57474/
> ---
> 
> (Updated April 11, 2017, 1:47 a.m.)
> 
> 
> Review request for mesos, Adam B and Benjamin Bannier.
> 
> 
> Bugs: MESOS-7026
> https://issues.apache.org/jira/browse/MESOS-7026
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds tests for each of the actions which support hierarchical roles.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 3e18c70738b6b7098f37fadebb799a596e76452d 
> 
> 
> Diff: https://reviews.apache.org/r/57474/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 58253: Added a ContainerID to 'ObjectApprover::Object'.

[Adam B]

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




include/mesos/authorizer/authorizer.proto
Lines 57 (patched)
<https://reviews.apache.org/r/58253/#comment244485>

Good question. I wonder if there's a more generic data structure (not 
`ContainerInfo`, I guess) that we could use that still includes ContainerID. 
`ContainerState` looks promising, but I'm not sure if we have all of that when 
we're trying to authorize.
Can you elaborate on what (new) action will use this object type, and how, 
and from where?


- Adam B


On April 6, 2017, 8:33 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58253/
> ---
> 
> (Updated April 6, 2017, 8:33 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Till Toenshoff, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7014
> https://issues.apache.org/jira/browse/MESOS-7014
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a new member, `container_id` to the
> `ObjectApprover::Object` to facilitate implicit executor
> authorization.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> 75801ccc753a60ce5e5979b6723fd2294ce7ffe5 
>   include/mesos/authorizer/authorizer.proto 
> 736f76d552956f2351ffd40fc51d088dff83f8c8 
>   src/authorizer/local/authorizer.cpp 
> e241edf4afa48d35d94d72e8e8690f5bedfc 
> 
> 
> Diff: https://reviews.apache.org/r/58253/diff/1/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 57788: Changed allocator to skip allocation on weight and quota changes.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On March 22, 2017, 11:36 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57788/
> ---
> 
> (Updated March 22, 2017, 11:36 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Bannier, 
> Benjamin Mahler, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changing weight or quota will no longer trigger a batch allocation.
> Since the allocator does not rebalance currently offered resources to
> reflect changes to weight or quota, doing a batch allocation is not
> useful; instead, the updated quota/weight values will be reflected on
> the next allocation.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 8d54a8cca1bb478f4437f68c5e14f66a9f9bb9e9 
>   src/tests/hierarchical_allocator_tests.cpp 
> e343dc37bd7136f0f6dd5dbc22a25cabe715038d 
> 
> 
> Diff: https://reviews.apache.org/r/57788/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 57730: Fixed example tests which broke due to the new `register_agents` ACL.

[Adam B]

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




src/tests/script.cpp
Lines 161 (patched)
<https://reviews.apache.org/r/57730/#comment244481>

We don't even enable agent authentication in most(/any?) tests, so I'd 
expect the agents to try to register without authentication. Should we enable 
agent authentication (and authorization) by default in the tests, and only 
disable it for the rare few tests that don't need it?


- Adam B


On March 17, 2017, 8:53 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57730/
> ---
> 
> (Updated March 17, 2017, 8:53 a.m.)
> 
> 
> Review request for mesos and Anindya Sinha.
> 
> 
> Bugs: MESOS-7097
> https://issues.apache.org/jira/browse/MESOS-7097
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - With the new `register_agents` ACL, `permissive == false` would lead
>   to the agent unable to register unless explicitly allowed.
> 
> 
> Diffs
> -
> 
>   src/tests/script.cpp 3b68b845b06fe19acb8b08e1ff3dd0bec9117f05 
> 
> 
> Diff: https://reviews.apache.org/r/57730/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 57710: Added `register_agents` to authorization.md.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On March 16, 2017, 4:07 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57710/
> ---
> 
> (Updated March 16, 2017, 4:07 p.m.)
> 
> 
> Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7097
> https://issues.apache.org/jira/browse/MESOS-7097
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `register_agents` to authorization.md.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md 0abb05cce1d1453cdefc42e3510cbe100e64 
> 
> 
> Diff: https://reviews.apache.org/r/57710/diff/1/
> 
> 
> Testing
> ---
> 
> via a markdown viewer.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 57534: Added and implemented RegisterAgent ACL.

[Adam B]

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


Fix it, then Ship it!




Nit: unnecessary `break`


src/authorizer/local/authorizer.cpp
Lines 1062 (patched)
<https://reviews.apache.org/r/57534/#comment244480>

Unnecessary `break` after `return` (recently removed elsewhere)


- Adam B


On March 28, 2017, 1:24 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57534/
> ---
> 
> (Updated March 28, 2017, 1:24 a.m.)
> 
> 
> Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, and Greg 
> Mann.
> 
> 
> Bugs: MESOS-7097
> https://issues.apache.org/jira/browse/MESOS-7097
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added and implemented RegisterAgent ACL.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> 46e66e8ed42240e5258890cdaf5aebe8800efcf0 
>   include/mesos/authorizer/authorizer.proto 
> 736f76d552956f2351ffd40fc51d088dff83f8c8 
>   src/authorizer/local/authorizer.cpp 
> be8037299601427e5d5e79f58f77eea3f89579d0 
>   src/tests/authorization_tests.cpp cd15add7d7b01c2b316ac946e017a4d0b502237f 
> 
> 
> Diff: https://reviews.apache.org/r/57534/diff/6/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 58304: Remove unnecessary hashmap lookups.

[Adam B]

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


Fix it, then Ship it!




LGTM, minor tweak to a log message


src/master/master.cpp
Lines 6429-6431 (original), 6433-6435 (patched)
<https://reviews.apache.org/r/58304/#comment244479>

s/shutdown.executor_id()/executorId/
s/shutdown.slave_id()/slaveId/
(now that you have them already)


- Adam B


On April 10, 2017, 8:50 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58304/
> ---
> 
> (Updated April 10, 2017, 8:50 a.m.)
> 
> 
> Review request for mesos and Mesos Reviewbot.
> 
> 
> Bugs: MESOS-7372
> https://issues.apache.org/jira/browse/MESOS-7372
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In a number of places, we tested whether the slaves hashmap contains
> the desired element before indexing it. It is safe to just index it and
> check for a NULL result, which saves us some hashing and lookups.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp c1f3c4f3bbcbfb10ae8fc974bd4a16ec070a79fd 
> 
> 
> Diff: https://reviews.apache.org/r/58304/diff/1/
> 
> 
> Testing
> ---
> 
> Make check (Fedora 25). Internal fuzzing.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58303: Pass the slave pointer directly to Master::removeTask.

[Adam B]

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


Fix it, then Ship it!




LGTM, minor nit.


src/master/master.cpp
Lines 8435-8439 (original), 8435-8436 (patched)
<https://reviews.apache.org/r/58303/#comment244478>

Nit: I'd probably swap these CHECKs to match the parameter order (or swap 
the parameters).


- Adam B


On April 10, 2017, 8:48 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58303/
> ---
> 
> (Updated April 10, 2017, 8:48 a.m.)
> 
> 
> Review request for mesos and Mesos Reviewbot.
> 
> 
> Bugs: MESOS-7372
> https://issues.apache.org/jira/browse/MESOS-7372
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In all cases where we call Master::removeTask, we have the correct slave
> pointer in hand. We can just pass it down, avoiding the need to look it
> up again with the SlaveID.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
>   src/master/master.cpp c1f3c4f3bbcbfb10ae8fc974bd4a16ec070a79fd 
> 
> 
> Diff: https://reviews.apache.org/r/58303/diff/1/
> 
> 
> Testing
> ---
> 
> Make check (Fedora 25). Internal fuzzing.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 57652: Allow authenticators to return any http Response.

[Adam B]

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




3rdparty/libprocess/include/process/authenticator.hpp
Lines 94-95 (original), 94-95 (patched)
<https://reviews.apache.org/r/57652/#comment244477>

I don't think either of these fields should be used to return a redirect. 
If we want to allow an authenticator module to return a (temporary) redirect, 
then we should a) add a new redirect field to the struct, and b) teach any 
users of the http authenticator (i.e. master, agent, etc.) to handle/log such a 
result and return the appropriate response.


- Adam B


On April 5, 2017, 12:13 p.m., Silas Snider wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57652/
> ---
> 
> (Updated April 5, 2017, 12:13 p.m.)
> 
> 
> Review request for mesos, Adam B, Anand Mazumdar, Alexander Rojas, and Greg 
> Mann.
> 
> 
> Bugs: MESOS-7247
> https://issues.apache.org/jira/browse/MESOS-7247
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously only allowed Unauthorized/Forbidden.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> 272d92117547512328c09dfc04c6ca4bf7b6b937 
> 
> 
> Diff: https://reviews.apache.org/r/57652/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Silas Snider
> 
>

Re: Review Request 57989: Removing deprecated ACL `ShutdownFramework`.

[Adam B]

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


Fix it, then Ship it!




Minor doc rewording, and you probably need to rebase, but otherwise great.


docs/upgrades.md
Lines 265 (patched)
<https://reviews.apache.org/r/57989/#comment244476>

s/its replacemente/the new/
s/After consolidation of the ACLs, the binaries could be safely 
replaced./After updating the ACLs, the binaries can be safely replaced.


- Adam B


On March 28, 2017, 2:35 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57989/
> ---
> 
> (Updated March 28, 2017, 2:35 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-7259
> https://issues.apache.org/jira/browse/MESOS-7259
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `ShutdownFramework` message from `acls.proto` was deprecated more
> than six months ago in favor of `TeardownFramework`. Since its
> deprecation cycle is already over, this patch removes the message.
> 
> Note that this only removes the message support from the ACLs, since
> the associated action was removed long ago.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 875ae02008cb22515396cf17c1d2e7da0046d068 
>   docs/upgrades.md e7f95aadeee330b3a9dbafc0f716c13df1bc8252 
>   include/mesos/authorizer/acls.proto 
> 46e66e8ed42240e5258890cdaf5aebe8800efcf0 
>   src/authorizer/local/authorizer.cpp 
> be8037299601427e5d5e79f58f77eea3f89579d0 
>   src/tests/teardown_tests.cpp 239a87ee0be8605a3f13196933038ef752ad9d4b 
> 
> 
> Diff: https://reviews.apache.org/r/57989/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 58252: Allowed the local authorizer to accept subjects with no value.

[Adam B]

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



LGTM. Just get rid of the unnecessary parentheses.


src/authorizer/local/authorizer.cpp
Line 1091 (original), 1091 (patched)
<https://reviews.apache.org/r/58252/#comment244475>

No reason for the extra parentheses around this pair anymore either. Why 
not:
```
  CHECK(!request.has_subject() ||
request.subject().has_value() ||
request.subject().has_claims());
```


- Adam B


On April 7, 2017, 4:15 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58252/
> ---
> 
> (Updated April 7, 2017, 4:15 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Till Toenshoff, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7014
> https://issues.apache.org/jira/browse/MESOS-7014
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates checks in the local authorizer to allow subjects
> which specify `claims` instead of a `value`.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> e241edf4afa48d35d94d72e8e8690f5bedfc 
> 
> 
> Diff: https://reviews.apache.org/r/58252/diff/2/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 57473: Added support for authorization of Hierachical roles.

[Adam B]

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



Minor comments, but it looks pretty close to shippable to me.


src/authorizer/local/authorizer.cpp
Line 202 (original), 202 (patched)
<https://reviews.apache.org/r/57473/#comment244300>

If GET_ENDPOINT_WITH_PATH is the only one now, then you can update the 
comment here to be less generic.



src/authorizer/local/authorizer.cpp
Line 458 (original), 405 (patched)
<https://reviews.apache.org/r/57473/#comment244299>

Why isn't this a `return Error();` too?



src/authorizer/local/authorizer.cpp
Lines 517-519 (patched)
<https://reviews.apache.org/r/57473/#comment244301>

Why are these the only checks with the `!= nullptr`? These checks weren't 
written that way before, and now we're inconsistent. I'd leave it out unless 
there's some reason to include it everywhere.



src/authorizer/local/authorizer.cpp
Lines 601 (patched)
<https://reviews.apache.org/r/57473/#comment244473>

Unnecessary `break` after a `return`



src/authorizer/local/authorizer.cpp
Lines 677 (patched)
<https://reviews.apache.org/r/57473/#comment244474>

Sounds like reason for an assert, not a mere comment.


- Adam B


On April 10, 2017, 3:11 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57473/
> ---
> 
> (Updated April 10, 2017, 3:11 a.m.)
> 
> 
> Review request for mesos, Adam B and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds mechanisms to support authorization of hierarchical roles,
> that is, it allows operators to write ACLs of the form `role/%`
> which will enforce the rule for any nested role, e.g. `role/a`,
> `role/b` and such.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> e241edf4afa48d35d94d72e8e8690f5bedfc 
> 
> 
> Diff: https://reviews.apache.org/r/57473/diff/6/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 58292: Removed unnecesary break statements in local approver.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On April 10, 2017, 3:09 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58292/
> ---
> 
> (Updated April 10, 2017, 3:09 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removes `break` statements located in lines following a `return`
> statement since they are effectively unreachable code, don't improve
> readability nor make the code cleaner.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> e241edf4afa48d35d94d72e8e8690f5bedfc 
> 
> 
> Diff: https://reviews.apache.org/r/58292/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 57166: Updated role validation for hierarchical roles.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On March 7, 2017, 7:38 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57166/
> ---
> 
> (Updated March 7, 2017, 7:38 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Role names can now contain forward slashes. Each component in a role
> name must now be validated separately.
> 
> 
> Diffs
> -
> 
>   src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a 
>   src/tests/role_tests.cpp 77f3d46a544a51ba71476e2f0735bb32758dd9e1 
> 
> 
> Diff: https://reviews.apache.org/r/57166/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 57472: Removing deprecated ACLs `SetQuota` and `RemoveQuota`.

[Adam B]

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


Fix it, then Ship it!




Minor clarification, otherwise great.


CHANGELOG
Lines 7-8 (patched)
<https://reviews.apache.org/r/57472/#comment243071>

This change is only applicable to the local authorizer, right? Other 
authorizers probably don't use `--acls`.


- Adam B


On March 21, 2017, 6:28 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57472/
> ---
> 
> (Updated March 21, 2017, 6:28 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Benjamin Bannier.
> 
> 
> Bugs: MESOS-7259
> https://issues.apache.org/jira/browse/MESOS-7259
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The ACLs `SetQuota` and `RemoveQuota` where marked for deprecation
> more than six months ago, where they were replaced for the more
> convenient `UpdateQuota` ACL. The deprecation cycle for these actions
> is finally due while at the same time removing will make easier to
> implement the hierarchical role feature in a generic way.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 09fbf03b4274098e8051460bf9d7e9dac4acc987 
>   docs/upgrades.md e7f95aadeee330b3a9dbafc0f716c13df1bc8252 
>   include/mesos/authorizer/acls.proto 
> e75e1879435f1c2bce47a86e9feebf9d051e969b 
>   src/authorizer/local/authorizer.cpp 
> be8037299601427e5d5e79f58f77eea3f89579d0 
>   src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 
>   src/master/quota_handler.cpp 36ea1acca47014b2fb7a3b597b857c8ec9e2ab67 
>   src/tests/authorization_tests.cpp cd15add7d7b01c2b316ac946e017a4d0b502237f 
>   src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837 
> 
> 
> Diff: https://reviews.apache.org/r/57472/diff/5/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 57846: Docker environment gets passed on docker run command.

[Adam B]

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


Ship it!




LGTM. You might want to quote the path, as @tillt suggests.

- Adam B


On March 22, 2017, 12:57 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57846/
> ---
> 
> (Updated March 22, 2017, 12:57 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Gilbert Song, James 
> DeFelice, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-6951
> https://issues.apache.org/jira/browse/MESOS-6951
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removes the use of `--env_file` as that does not support newlines in
> environment variable values.
> Removes leaking of possibly sensitive environment variables to the log.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 44fbde8e8a2c6c957af7339b4fb9479af7f21ff7 
> 
> 
> Diff: https://reviews.apache.org/r/57846/diff/2/
> 
> 
> Testing
> ---
> 
> make check & sudo ./bin/mesos-tests.sh
> 
> + functional testing
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 57762: Fixed environment duplication in command executor.

[Adam B]

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


Fix it, then Ship it!




I've got some nits, but nothing major.


src/launcher/executor.cpp
Line 473 (original), 483 (patched)
<https://reviews.apache.org/r/57762/#comment242674>

Technically the default, so you don't have to set it.



src/launcher/executor.cpp
Lines 491 (patched)
<https://reviews.apache.org/r/57762/#comment242675>

Not my favorite wrapping, but I'd allow it. I'd probably prefer:
```
  foreach (const Environment::Variable& variable,
   taskEnvironment->variables()) {
```



src/launcher/executor.cpp
Lines 494 (patched)
<https://reviews.apache.org/r/57762/#comment242677>

"Overwriting environment variable 'foo' with value from task environment."



src/launcher/executor.cpp
Lines 497 (patched)
<https://reviews.apache.org/r/57762/#comment242676>

No need to overwrite if the value is the same. Not a big deal if you do 
though.



src/launcher/executor.cpp
Lines 507 (patched)
<https://reviews.apache.org/r/57762/#comment242678>

"Overwriting environment variable 'foo' with value from command 
environment."


- Adam B


On March 23, 2017, 7:24 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57762/
> ---
> 
> (Updated March 23, 2017, 7:24 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, Greg Mann, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-7263
> https://issues.apache.org/jira/browse/MESOS-7263
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp db703f054decbca62f7fbe98f5d28f6e4c6c9b0f 
> 
> 
> Diff: https://reviews.apache.org/r/57762/diff/7/
> 
> 
> Testing
> ---
> 
> make check + functional testing..
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 57889: Fixed flags logging in Docker executor.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On March 23, 2017, 12:43 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57889/
> ---
> 
> (Updated March 23, 2017, 12:43 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp b652a99340f6bbbef2dbdd3a6e10333f32762c0f 
> 
> 
> Diff: https://reviews.apache.org/r/57889/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 57472: Removing deprecated ACLs `SetQuota` and `RemoveQuota`.

[Adam B]

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



Thanks for taking this on. Besides a couple of doc updates, there's at least 
one more place to clean up:
https://github.com/apache/mesos/blob/1.2.0/src/master/master.hpp#L1036
```
// TODO(mpark): The following functions `authorizeSetQuota` and
// `authorizeRemoveQuota` should be replaced with `authorizeUpdateQuota` at
// the end of deprecation cycle which started with 1.0.`
```


CHANGELOG
Lines 1 (patched)
<https://reviews.apache.org/r/57472/#comment240847>

`Release Notes - Mesos - Version 1.3.0 (WIP)`



CHANGELOG
Lines 6 (patched)
<https://reviews.apache.org/r/57472/#comment240848>

Please mention this in the docs/upgrades.md table as well.


- Adam B


On March 10, 2017, 12:43 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57472/
> ---
> 
> (Updated March 10, 2017, 12:43 a.m.)
> 
> 
> Review request for mesos, Adam B and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The ACLs `SetQuota` and `RemoveQuota` where marked for deprecation
> more than six months ago, where they were replaced for the more
> convenient `UpdateQuota` ACL. The deprecation cycle for these actions
> is finally due while at the same time removing will make easier to
> implement the hierarchical role feature in a generic way.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 0a4e1dd52f9197151c5ef4318579ffa4e9bcf0ee 
>   include/mesos/authorizer/acls.proto 
> 8389917d12f9a9a3c9b4281f48e23ade14c20528 
>   src/authorizer/local/authorizer.cpp 
> 2227b241eab1606815fa6464e3d6b1345624fd22 
>   src/tests/authorization_tests.cpp 42edecc794b71a00ca32d26ae9b74e9f3ef97510 
>   src/tests/master_quota_tests.cpp e418f22ea1773f4356ced44b8d57a80e826c8837 
> 
> 
> Diff: https://reviews.apache.org/r/57472/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Review Request 57442: Update website for Mesos 1.2.0 release.

[Adam B]

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

Review request for mesos, Gilbert Song, Till Toenshoff, and Vinod Kone.


Repository: mesos


Description
---

Update website for Mesos 1.2.0 release.


Diffs
-

  docs/getting-started.md bce9649229852488e2873f38694adb5b16268edf 
  site/data/release_plan.yaml fc57d591d9b034eb06691d756f5c94acf0e8a3e3 
  site/data/releases.yml eecc0b110ce0c74a18e7a909aa1ec62f6ebbcc9a 
  site/source/blog/2017-03-08-mesos-1-2-0-released.md PRE-CREATION 
  site/source/layouts/post.erb fdb566b01892d87fe6e121c958fe07b29cedf072 


Diff: https://reviews.apache.org/r/57442/diff/1/


Testing
---


Thanks,

Adam B

Re: Review Request 56805: Simplified interface for setting weights in allocator.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On Feb. 28, 2017, 12:24 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56805/
> ---
> 
> (Updated Feb. 28, 2017, 12:24 p.m.)
> 
> 
> Review request for mesos, Adam B and Yongqiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Now that weights can change dynamically, passing a list of weights to
> the allocator at initialization-time is unnecessary and confusing (e.g.,
> the initial weights might be wrong, if a different set of weight values
> are recovered from the registry).
> 
> Instead, require that weights are communicated to the allocator via the
> existing `updateWeights` method.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> 25ed5f36b186f2bd257dd0bdb366d0b21a795622 
>   src/master/allocator/mesos/allocator.hpp 
> 1defb59b686c9cd8d403a0ed6825219f62a7801d 
>   src/master/allocator/mesos/hierarchical.hpp 
> 0bb24be2761bde6d1baad69f5654029f3ceed553 
>   src/master/allocator/mesos/hierarchical.cpp 
> 696815795dc391b4ec7538892b1224b812482d34 
>   src/master/master.cpp ae36bf477851bf2fe11eb7913c580e3e0b9cbbe5 
>   src/tests/allocator.hpp b6b0022d581bd688900aaf5beb0af7ce6e0129a1 
>   src/tests/api_tests.cpp 607392ff6b714a9e812c2802f4d1465e8f71ad09 
>   src/tests/hierarchical_allocator_tests.cpp 
> cdf1f15b7802439b28405ca8f6634ce83e886630 
>   src/tests/master_allocator_tests.cpp 
> 7b0b786f1c6c53616fd7ae1f7f765752d94a4f83 
>   src/tests/master_quota_tests.cpp 91219d6693fdd119ed3b0bf734eaa55da9c58b0a 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 1cc6c9d01a3a473f5a44210ea725310ea5931ff6 
>   src/tests/reservation_endpoints_tests.cpp 
> 345f0457ec1fc00b7033d71227ff178c14e015bb 
>   src/tests/reservation_tests.cpp 5c0d01483efb4561b8c0016c3a2fa6ea5574196e 
>   src/tests/resource_offers_tests.cpp 
> 74dacf140e49e402a4ad02ce7751e7c7b2f78ee1 
>   src/tests/slave_recovery_tests.cpp b5b805868bed61bf482d71322fb1918a0d020d48 
> 
> 
> Diff: https://reviews.apache.org/r/56805/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 56623: Implemented the 'Principal' type in libprocess.

[Adam B]

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



LGTM


3rdparty/libprocess/include/process/http.hpp
Line 106 (original), 108 (patched)
<https://reviews.apache.org/r/56623/#comment239791>

"authentication principal"?


- Adam B


On Feb. 28, 2017, 10:28 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56623/
> ---
> 
> (Updated Feb. 28, 2017, 10:28 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till 
> Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-7003
> https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a new struct, `Principal`, to libprocess
> to represent an authenticated entity in the system.
> The new type contains a string `value` and a map containing
> arbitrary key-value pairs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> e5489c6cb4adc8a822e7dd4515542618c36136f9 
>   3rdparty/libprocess/include/process/http.hpp 
> eb2c87dee2a911085592e0e14a7499d526ad0bfc 
>   3rdparty/libprocess/include/process/process.hpp 
> ceedbe86ccba5c24bf1e9e7f032d7dbcf6acb0f3 
>   3rdparty/libprocess/src/authenticator.cpp 
> cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
>   3rdparty/libprocess/src/authenticator_manager.cpp 
> a22acd026a001788dc39b8005a56577e33c6800b 
>   3rdparty/libprocess/src/process.cpp 
> 3ad485f9136cd9edf0a6db76ff1f475039236391 
> 
> 
> Diff: https://reviews.apache.org/r/56623/diff/9/
> 
> 
> Testing
> ---
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 57054: Fixed a bug in master and agent handler authorization logic.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On Feb. 27, 2017, 9:31 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57054/
> ---
> 
> (Updated Feb. 27, 2017, 9:31 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7003
> https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch fixes a bug where endpoint handlers would not
> correctly handle the case in which authorization is enabled
> when authentication is disabled. In this case, the handlers
> would send a default-constructed `authorization::Subject` to
> the authorizer, leading to an empty-string principal being
> evaluated as the subject.
> 
> This patch updates the handlers to correctly send `NONE` as
> the subject in this case.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 6e5178eeea6cc6b90ae253840da22be13444b088 
>   src/slave/http.cpp 94731ec883c309cefb811694dc4e39de12d1ac59 
>   src/slave/slave.cpp fc480ae23ffa5cdeeb79b3621a08e1f8703bc01a 
> 
> 
> Diff: https://reviews.apache.org/r/57054/diff/1/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this patch chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56742: Updated CHANGELOG for Mesos 1.2.0 release.

[Adam B]

> On Feb. 23, 2017, 4:39 p.m., Benjamin Bannier wrote:
> > We simultanously use _unified containerizer_ and _Mesos containerizer_ here 
> > to describe 1.2.0 changes. I think picking a single name, at least in 
> > high-level descriptions, might be less confusing.
> 
> Adam B wrote:
> $ grep -i "unified containerizer" CHANGELOG |wc -l
> 13
> $ grep -i "mesos containerizer" CHANGELOG |wc -l
> 36
> 
> So I guess we've used both quite a bit. I'll try to rename a few to 
> "Mesos containerizer", although I think we specifically use "unified 
> containerizer" to discuss

... to discuss Mesos containerizer using docker images/registries.


- Adam


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


On Feb. 23, 2017, 5:01 p.m., Adam B wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56742/
> ---
> 
> (Updated Feb. 23, 2017, 5:01 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated CHANGELOG for Mesos 1.2.0 release.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 1478ad4dd3b20fc768fd679f81d803acf0995ad2 
> 
> Diff: https://reviews.apache.org/r/56742/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Adam B
> 
>

Re: Review Request 56742: Updated CHANGELOG for Mesos 1.2.0 release.

[Adam B]

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

(Updated Feb. 23, 2017, 5:01 p.m.)


Review request for mesos, Jie Yu and Vinod Kone.


Changes
---

Mesos containerizer, rlimit.


Repository: mesos


Description
---

Updated CHANGELOG for Mesos 1.2.0 release.


Diffs (updated)
-

  CHANGELOG 1478ad4dd3b20fc768fd679f81d803acf0995ad2 

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


Testing
---


Thanks,

Adam B

Re: Review Request 56742: Updated CHANGELOG for Mesos 1.2.0 release.

[Adam B]

> On Feb. 23, 2017, 4:39 p.m., Benjamin Bannier wrote:
> > We simultanously use _unified containerizer_ and _Mesos containerizer_ here 
> > to describe 1.2.0 changes. I think picking a single name, at least in 
> > high-level descriptions, might be less confusing.

$ grep -i "unified containerizer" CHANGELOG |wc -l
13
$ grep -i "mesos containerizer" CHANGELOG |wc -l
36

So I guess we've used both quite a bit. I'll try to rename a few to "Mesos 
containerizer", although I think we specifically use "unified containerizer" to 
discuss


> On Feb. 23, 2017, 4:39 p.m., Benjamin Bannier wrote:
> > CHANGELOG, line 126
> > <https://reviews.apache.org/r/56742/diff/2/?file=1640488#file1640488line126>
> >
> > s/rLimit/rlimit/

Fixing.


- Adam


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


On Feb. 23, 2017, 4:04 p.m., Adam B wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56742/
> ---
> 
> (Updated Feb. 23, 2017, 4:04 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated CHANGELOG for Mesos 1.2.0 release.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 1478ad4dd3b20fc768fd679f81d803acf0995ad2 
> 
> Diff: https://reviews.apache.org/r/56742/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Adam B
> 
>

Re: Review Request 56867: Added upgrade guide for 1.2.x.

[Adam B]

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

(Updated Feb. 23, 2017, 4:47 p.m.)


Review request for mesos.


Changes
---

Use proper authorization action names/cases


Repository: mesos


Description
---

Added upgrade guide for 1.2.x.


Diffs (updated)
-

  docs/upgrades.md 74f4367020d7025bdcfbbf50a166a644ee5050bc 

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


Testing
---


Thanks,

Adam B

Re: Review Request 56867: Added upgrade guide for 1.2.x.

[Adam B]

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

(Updated Feb. 23, 2017, 4:35 p.m.)


Review request for mesos.


Changes
---

New/Added is 'A'


Repository: mesos


Description
---

Added upgrade guide for 1.2.x.


Diffs (updated)
-

  docs/upgrades.md 74f4367020d7025bdcfbbf50a166a644ee5050bc 

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


Testing
---


Thanks,

Adam B

Re: Review Request 56867: Added upgrade guide for 1.2.x.

[Adam B]

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

(Updated Feb. 23, 2017, 4:31 p.m.)


Review request for mesos.


Summary (updated)
-

Added upgrade guide for 1.2.x.


Repository: mesos


Description
---

Added upgrade guide for 1.2.x.


Diffs (updated)
-

  docs/upgrades.md 74f4367020d7025bdcfbbf50a166a644ee5050bc 

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


Testing
---


Thanks,

Adam B

Re: Review Request 56742: Updated CHANGELOG for Mesos 1.2.0 release.

[Adam B]

> On Feb. 21, 2017, 11:11 a.m., Vinod Kone wrote:
> > CHANGELOG, line 111
> > <https://reviews.apache.org/r/56742/diff/2/?file=1640488#file1640488line111>
> >
> > I think we can graduate this? cc @alexR
> 
> Alexander Rukletsov wrote:
> We haven't tested it on windows yet. Also we may be changing the protos 
> in the nearest future per recent discussions. Does it make more sense to wait 
> with graduation until then?

You're the maintainer. If you want to wait, we can wait. It stays experimental 
then.


- Adam


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


On Feb. 23, 2017, 4:04 p.m., Adam B wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56742/
> ---
> 
> (Updated Feb. 23, 2017, 4:04 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated CHANGELOG for Mesos 1.2.0 release.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 1478ad4dd3b20fc768fd679f81d803acf0995ad2 
> 
> Diff: https://reviews.apache.org/r/56742/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Adam B
> 
>

Re: Review Request 56742: Updated CHANGELOG for Mesos 1.2.0 release.

[Adam B]

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

(Updated Feb. 23, 2017, 4:04 p.m.)


Review request for mesos, Jie Yu and Vinod Kone.


Repository: mesos


Description
---

Updated CHANGELOG for Mesos 1.2.0 release.


Diffs (updated)
-

  CHANGELOG 1478ad4dd3b20fc768fd679f81d803acf0995ad2 

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


Testing
---


Thanks,

Adam B

Review Request 57005: Updated configuration.md for --http_heartbeat_interval.

[Adam B]

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

Review request for mesos and Kevin Klues.


Repository: mesos


Description
---

Updated configuration.md for --http_heartbeat_interval.


Diffs
-

  docs/configuration.md b3e1f8ec20c663a49128a858f636580983891c8b 

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


Testing
---


Thanks,

Adam B

Re: Review Request 56711: Fixed fetcher to not pick up environment variables it should not see.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On Feb. 22, 2017, 4:41 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56711/
> ---
> 
> (Updated Feb. 22, 2017, 4:41 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, and Greg Mann.
> 
> 
> Bugs: MESOS-6751 and MESOS-7133
> https://issues.apache.org/jira/browse/MESOS-6751
> https://issues.apache.org/jira/browse/MESOS-7133
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed fetcher to not pick up environment variables it should not see.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 9ec38dc95dddfcd990369d0146986e20b15da1a0 
> 
> Diff: https://reviews.apache.org/r/56711/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 56711: Fixed fetcher to not pick up environment variables it should not see.

[Adam B]

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


Fix it, then Ship it!




Looks good; just a couple of minor comments


src/slave/containerizer/fetcher.cpp (lines 833 - 838)
<https://reviews.apache.org/r/56711/#comment238207>

Alphabetize these?



src/slave/containerizer/fetcher.cpp (lines 843 - 844)
<https://reviews.apache.org/r/56711/#comment238208>

If you do `using std::startsWith;` then you could fit these two one one 
line.


- Adam B


On Feb. 21, 2017, 3:46 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56711/
> ---
> 
> (Updated Feb. 21, 2017, 3:46 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, and Greg Mann.
> 
> 
> Bugs: MESOS-6751 and MESOS-7133
> https://issues.apache.org/jira/browse/MESOS-6751
> https://issues.apache.org/jira/browse/MESOS-7133
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed fetcher to not pick up environment variables it should not see.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 9ec38dc95dddfcd990369d0146986e20b15da1a0 
> 
> Diff: https://reviews.apache.org/r/56711/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 56771: Added regression test against fetcher SSL spillover.

[Adam B]

> On Feb. 21, 2017, 5:16 p.m., Adam B wrote:
> > Looks good to me. Just a cuminor comments.

s/cuminor/few minor/


- Adam


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


On Feb. 21, 2017, 7:56 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56771/
> ---
> 
> (Updated Feb. 21, 2017, 7:56 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added regression test against fetcher SSL spillover.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_tests.cpp 9c7e8b94071501c435e26850d66a8f3e8950c6cd 
> 
> Diff: https://reviews.apache.org/r/56771/diff/
> 
> 
> Testing
> ---
> 
> Before applying RR56711:
> ```
> [ RUN  ] FetcherTest.EnvironmentSpillover
> W0216 23:31:25.794740 12402 fetcher.cpp:899] Begin fetcher log (stderr in 
> sandbox) for container b0fd3f0f-ae72-4d88-b8c2-b4c1af1edee9 from running 
> command: /Users/till/Development/mesos-private/build/src/mesos-fetcher
> I0216 23:31:25.746598 3022980032 fetcher.cpp:531] Fetcher Info: 
> {"cache_directory":"\/var\/folders\/_t\/rdp354gx7j5fjww270kbk6_rgn\/T\/mesos\/fetch\/slaves\/","items":[{"action":"BYPASS_CACHE","uri":{"extract":true,"value":"\/private\/var\/folders\/_t\/rdp354gx7j5fjww270kbk6_rgn\/T\/KBvUlM\/Fo2V66\/zpbEP1.gz"}}],"sandbox_directory":"\/private\/var\/folders\/_t\/rdp354gx7j5fjww270kbk6_rgn\/T\/KBvUlM"}
> I0216 23:31:25.752025 3022980032 fetcher.cpp:442] Fetching URI 
> '/private/var/folders/_t/rdp354gx7j5fjww270kbk6_rgn/T/KBvUlM/Fo2V66/zpbEP1.gz'
> I0216 23:31:25.752058 3022980032 fetcher.cpp:283] Fetching directly into the 
> sandbox directory
> I0216 23:31:25.752096 3022980032 fetcher.cpp:220] Fetching URI 
> '/private/var/folders/_t/rdp354gx7j5fjww270kbk6_rgn/T/KBvUlM/Fo2V66/zpbEP1.gz'
> I0216 23:31:25.756001 3022980032 fetcher.cpp:205] Copied resource 
> '/private/var/folders/_t/rdp354gx7j5fjww270kbk6_rgn/T/KBvUlM/Fo2V66/zpbEP1.gz'
>  to 
> '/private/var/folders/_t/rdp354gx7j5fjww270kbk6_rgn/T/KBvUlM/zpbEP1.gz'
> SSL requires key! NOTE: Set path with LIBPROCESS_SSL_KEY_FILE
> 
> End fetcher log for container b0fd3f0f-ae72-4d88-b8c2-b4c1af1edee9
> E0216 23:31:25.795078 12402 fetcher.cpp:555] Failed to run mesos-fetcher: 
> Failed to fetch all URIs for container 'b0fd3f0f-ae72-4d88-b8c2-b4c1af1edee9' 
> with exit status: 256
> ../../src/tests/fetcher_tests.cpp:1134: Failure
> (fetch).failure(): Failed to fetch all URIs for container 
> 'b0fd3f0f-ae72-4d88-b8c2-b4c1af1edee9' with exit status: 256
> [  FAILED  ] FetcherTest.EnvironmentSpillover (115 ms)
> ```
> 
> After applying RR56711:
> ```
> [ RUN  ] FetcherTest.EnvironmentSpillover
> [   OK ] FetcherTest.EnvironmentSpillover (300 ms)
> ```
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 56771: Added regression test against fetcher SSL spillover.

[Adam B]

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


Fix it, then Ship it!




Looks good to me. Just a cuminor comments.


src/tests/fetcher_tests.cpp (line 1112)
<https://reviews.apache.org/r/56771/#comment238063>

Maybe at least choose different strings for dirname and path?



src/tests/fetcher_tests.cpp (line 1135)
<https://reviews.apache.org/r/56771/#comment238203>

Seems like this AWAIT is the final check. Please add a comment explaining 
how a successful fetch means a successful test run.


- Adam B


On Feb. 21, 2017, 7:56 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56771/
> ---
> 
> (Updated Feb. 21, 2017, 7:56 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added regression test against fetcher SSL spillover.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_tests.cpp 9c7e8b94071501c435e26850d66a8f3e8950c6cd 
> 
> Diff: https://reviews.apache.org/r/56771/diff/
> 
> 
> Testing
> ---
> 
> Before applying RR56711:
> ```
> [ RUN  ] FetcherTest.EnvironmentSpillover
> W0216 23:31:25.794740 12402 fetcher.cpp:899] Begin fetcher log (stderr in 
> sandbox) for container b0fd3f0f-ae72-4d88-b8c2-b4c1af1edee9 from running 
> command: /Users/till/Development/mesos-private/build/src/mesos-fetcher
> I0216 23:31:25.746598 3022980032 fetcher.cpp:531] Fetcher Info: 
> {"cache_directory":"\/var\/folders\/_t\/rdp354gx7j5fjww270kbk6_rgn\/T\/mesos\/fetch\/slaves\/","items":[{"action":"BYPASS_CACHE","uri":{"extract":true,"value":"\/private\/var\/folders\/_t\/rdp354gx7j5fjww270kbk6_rgn\/T\/KBvUlM\/Fo2V66\/zpbEP1.gz"}}],"sandbox_directory":"\/private\/var\/folders\/_t\/rdp354gx7j5fjww270kbk6_rgn\/T\/KBvUlM"}
> I0216 23:31:25.752025 3022980032 fetcher.cpp:442] Fetching URI 
> '/private/var/folders/_t/rdp354gx7j5fjww270kbk6_rgn/T/KBvUlM/Fo2V66/zpbEP1.gz'
> I0216 23:31:25.752058 3022980032 fetcher.cpp:283] Fetching directly into the 
> sandbox directory
> I0216 23:31:25.752096 3022980032 fetcher.cpp:220] Fetching URI 
> '/private/var/folders/_t/rdp354gx7j5fjww270kbk6_rgn/T/KBvUlM/Fo2V66/zpbEP1.gz'
> I0216 23:31:25.756001 3022980032 fetcher.cpp:205] Copied resource 
> '/private/var/folders/_t/rdp354gx7j5fjww270kbk6_rgn/T/KBvUlM/Fo2V66/zpbEP1.gz'
>  to 
> '/private/var/folders/_t/rdp354gx7j5fjww270kbk6_rgn/T/KBvUlM/zpbEP1.gz'
> SSL requires key! NOTE: Set path with LIBPROCESS_SSL_KEY_FILE
> 
> End fetcher log for container b0fd3f0f-ae72-4d88-b8c2-b4c1af1edee9
> E0216 23:31:25.795078 12402 fetcher.cpp:555] Failed to run mesos-fetcher: 
> Failed to fetch all URIs for container 'b0fd3f0f-ae72-4d88-b8c2-b4c1af1edee9' 
> with exit status: 256
> ../../src/tests/fetcher_tests.cpp:1134: Failure
> (fetch).failure(): Failed to fetch all URIs for container 
> 'b0fd3f0f-ae72-4d88-b8c2-b4c1af1edee9' with exit status: 256
> [  FAILED  ] FetcherTest.EnvironmentSpillover (115 ms)
> ```
> 
> After applying RR56711:
> ```
> [ RUN  ] FetcherTest.EnvironmentSpillover
> [   OK ] FetcherTest.EnvironmentSpillover (300 ms)
> ```
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 56867: WIP: Added upgrade guide for 1.2.x.

[Adam B]

> On Feb. 21, 2017, 3:20 p.m., Zhitao Li wrote:
> > docs/upgrades.md, lines 252-260
> > <https://reviews.apache.org/r/56867/diff/1/?file=1640650#file1640650line252>
> >
> > 1. Is the order of these actions matter? I recall we specifically say 
> > order of upgrading each component does not mater, so I'm interested in why 
> > this is being explicitly called out;
> > 2. Is this specific to 1.1.x -> 1.2.x upgrade, or serves just a general 
> > guideline regardless of versions? (if former, maybe call out the reason; if 
> > latter, maybe move to a different location).

1. We generally recommend upgrading masters before agents (and rebuilding 
modules before either).
2. This is nothing new to 1.2.x, and is actually included in each release's 
section (except 1.1.x). The order/recommendation doesn't usually change much, 
although it has evolved as we have added modules and clarified the need for 
scheduler/executor upgrades.


- Adam


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


On Feb. 20, 2017, 11:57 p.m., Adam B wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56867/
> ---
> 
> (Updated Feb. 20, 2017, 11:57 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added upgrade guide for 1.2.x.
> 
> 
> Diffs
> -
> 
>   docs/upgrades.md 74f4367020d7025bdcfbbf50a166a644ee5050bc 
> 
> Diff: https://reviews.apache.org/r/56867/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Adam B
> 
>

Re: Review Request 56711: Fixed fetcher to not pick up environment variables it should not see.

[Adam B]

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




src/slave/containerizer/fetcher.cpp (line 846)
<https://reviews.apache.org/r/56711/#comment238062>

Why not `whitelist.contains(toUpper(key))`?


- Adam B


On Feb. 21, 2017, 8:04 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56711/
> ---
> 
> (Updated Feb. 21, 2017, 8:04 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, and Greg Mann.
> 
> 
> Bugs: MESOS-6751 and MESOS-7133
> https://issues.apache.org/jira/browse/MESOS-6751
> https://issues.apache.org/jira/browse/MESOS-7133
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed fetcher to not pick up environment variables it should not see.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 9ec38dc95dddfcd990369d0146986e20b15da1a0 
> 
> Diff: https://reviews.apache.org/r/56711/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Review Request 56867: WIP: Added upgrade guide for 1.2.x.

[Adam B]

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

Review request for mesos.


Repository: mesos


Description
---

Added upgrade guide for 1.2.x.


Diffs
-

  docs/upgrades.md 74f4367020d7025bdcfbbf50a166a644ee5050bc 

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


Testing
---


Thanks,

Adam B

Re: Review Request 56812: Updated agent handlers to use 'AuthenticationContext'.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On Feb. 17, 2017, 7:02 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56812/
> ---
> 
> (Updated Feb. 17, 2017, 7:02 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till 
> Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-7003
> https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the HTTP endpoint handlers in the
> agent process to accept an `AuthenticationContext`
> instead of an `Option& principal`.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp af70b6f294a04f23b04cd1d8c36c1c3e86d7d5e6 
>   src/slave/slave.hpp 3b0aea4e3e9a17501077beccbccaab4abbe11af2 
>   src/slave/slave.cpp 7564e8d39530794131dbbc928fcbc59fb65ef471 
> 
> Diff: https://reviews.apache.org/r/56812/diff/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56804: Cleaned up weights handling code.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On Feb. 17, 2017, 4:14 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56804/
> ---
> 
> (Updated Feb. 17, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos, Adam B and Yongqiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed a redundant statement, fixed log message style, cleaned up
> comments.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 5534674fa2740b0a6e37fc1df34b582f1e691133 
>   src/master/weights_handler.cpp da0b995e1866e7eb57b8ef885864c6c6d66405d6 
>   src/tests/dynamic_weights_tests.cpp 
> ce577ce6c71bbfffba7db7c72523a9fd2d68141b 
> 
> Diff: https://reviews.apache.org/r/56804/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 54784: Added authorization tests when trying to attach to a container input.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On Feb. 15, 2017, 6:31 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54784/
> ---
> 
> (Updated Feb. 15, 2017, 6:31 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-6886
> https://issues.apache.org/jira/browse/MESOS-6886
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduces a test where a principal is allowed to launch a container
> session, but not to attach to its input, therefore its attempts should
> fail.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp aa7d89a710400d1460bc9139ea563cfdd45bd58f 
> 
> Diff: https://reviews.apache.org/r/54784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 54783: Adds authorization tests of launching container sessions API.

[Adam B]

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


Ship it!




Looks like a fine test of unauthorized access. Authorized access is tested by 
`AgentAPITest.LaunchNestedContainerSession`

- Adam B


On Feb. 15, 2017, 6:30 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54783/
> ---
> 
> (Updated Feb. 15, 2017, 6:30 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-6886
> https://issues.apache.org/jira/browse/MESOS-6886
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduces a test which ensures that unauthorized users are unable to
> launch nested container sessions.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp aa7d89a710400d1460bc9139ea563cfdd45bd58f 
> 
> Diff: https://reviews.apache.org/r/54783/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 56619: Updated 'Files' handlers to use 'AuthenticationContext'.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On Feb. 17, 2017, 7 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56619/
> ---
> 
> (Updated Feb. 17, 2017, 7 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till 
> Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-7003
> https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the HTTP endpoint handlers in the
> Mesos `Files` process to accept an `AuthenticationContext`
> instead of an `Option& principal`.
> 
> 
> Diffs
> -
> 
>   src/files/files.hpp 8cffc26fc7d674187e55663f23f1e10bed40229e 
>   src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a 
> 
> Diff: https://reviews.apache.org/r/56619/diff/
> 
> 
> Testing
> ---
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56618: Updated common Mesos code to use 'AuthenticationContext'.

[Adam B]

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



Lookin good


include/mesos/authorizer/authorizer.proto (lines 29 - 30)
<https://reviews.apache.org/r/56618/#comment238013>

"currently only a value" is no longer accurate


- Adam B


On Feb. 17, 2017, 2:34 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56618/
> ---
> 
> (Updated Feb. 17, 2017, 2:34 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till 
> Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-7003
> https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates common Mesos HTTP-related helpers,
> as well as the `authorization::Subject` protobuf
> message, to make use of the `AuthenticationContext`
> type instead of an `Option principal`.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 9cc75b0db17b2d0bab3f449f795cbf505c5b0f15 
>   src/common/http.hpp 3d5ab59ddc4dce4d791c1b439f5e1459d1a724a4 
>   src/common/http.cpp abfbf7248beb2d4068be06b7f5f829d7617f943e 
> 
> Diff: https://reviews.apache.org/r/56618/diff/
> 
> 
> Testing
> ---
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56617: Updated libprocess handlers to use 'AuthenticationContext'.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On Feb. 13, 2017, 7:41 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56617/
> ---
> 
> (Updated Feb. 13, 2017, 7:41 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till 
> Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-7003
> https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the HTTP endpoint handlers in libprocess
> to make use of the `AuthenticationContext` instead of an
> `Option& principal`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/logging.hpp 
> b5c2515605d5a9fab304f1425d29082f6167cea4 
>   3rdparty/libprocess/include/process/metrics/metrics.hpp 
> d9c64b7a4660182a5a23beed50a8e42b89d8b2aa 
>   3rdparty/libprocess/include/process/profiler.hpp 
> d19d647aa0d1172f09940fae1d1dfa83fd07b924 
>   3rdparty/libprocess/src/logging.cpp 
> cf7b43d2339df57ce899677419968d7e00ad8c38 
>   3rdparty/libprocess/src/metrics/metrics.cpp 
> 29c8c1da4e1424f029ccdf9fab890e8c1ec0ccb8 
>   3rdparty/libprocess/src/profiler.cpp 
> 2b3b6d5220aed9568f9fef51045cf322aa2934d7 
> 
> Diff: https://reviews.apache.org/r/56617/diff/
> 
> 
> Testing
> ---
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56623: Implemented 'AuthenticationContext' in libprocess.

[Adam B]

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



Looks good for a foundation, but I haven't looked at the rest of the patch 
chain yet


3rdparty/libprocess/include/process/authenticator.hpp (line 36)
<https://reviews.apache.org/r/56623/#comment238012>

"claims" or "labels"? I kinda like "claims"


- Adam B


On Feb. 17, 2017, 6:54 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56623/
> ---
> 
> (Updated Feb. 17, 2017, 6:54 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till 
> Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-7003
> https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a new struct, `AuthenticationContext`, to
> libprocess to represent an authenticated entity in the system.
> The new type contains a `principal` and a map containing
> arbitrary key-value pairs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> e5489c6cb4adc8a822e7dd4515542618c36136f9 
>   3rdparty/libprocess/include/process/http.hpp 
> 3a406963c1defc037d58971f2400e40855503fbb 
>   3rdparty/libprocess/include/process/process.hpp 
> ceedbe86ccba5c24bf1e9e7f032d7dbcf6acb0f3 
>   3rdparty/libprocess/src/authenticator.cpp 
> cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
>   3rdparty/libprocess/src/authenticator_manager.cpp 
> a22acd026a001788dc39b8005a56577e33c6800b 
>   3rdparty/libprocess/src/http.cpp 0e37936eec5b2910e54d40788c26f98665cbe1f1 
>   3rdparty/libprocess/src/process.cpp 
> 3ad485f9136cd9edf0a6db76ff1f475039236391 
> 
> Diff: https://reviews.apache.org/r/56623/diff/
> 
> 
> Testing
> ---
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56742: Updated CHANGELOG for Mesos 1.2.0 release.

[Adam B]

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

(Updated Feb. 20, 2017, 1:22 a.m.)


Review request for mesos, Jie Yu and Vinod Kone.


Changes
---

Updates!


Repository: mesos


Description
---

Updated CHANGELOG for Mesos 1.2.0 release.


Diffs (updated)
-

  CHANGELOG 1478ad4dd3b20fc768fd679f81d803acf0995ad2 

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


Testing
---


Thanks,

Adam B

Re: Review Request 56711: Fixed fetcher to not pick up environment variables it should not see.

[Adam B]

> On Feb. 16, 2017, 1:39 a.m., Adam B wrote:
> > src/slave/containerizer/fetcher.cpp, lines 827-828
> > <https://reviews.apache.org/r/56711/diff/1/?file=1634532#file1634532line827>
> >
> > How confident are you that the fetcher doesn't actually need any of 
> > these env vars? How can we test/prove it?
> 
> Till Toenshoff wrote:
> So far I simply tried to identify such possible conflicts by looking at 
> the code.
> 
> The `mesos-fetcher` itself is offering support for 
> `mesos::internal::logging::Flags` which add the following options:
> ```
>   add(::quiet,
>   add(::logging_level,
>   add(::log_dir,
>   add(::logbufsecs,
>   add(::initialize_driver_logging,
>   add(::external_log_file,
> ```
> All of the above are certainly supported by use if `MESOS_` prefixed 
> environment variable names. Other than that, the `mesos-fetcher` has support 
> for `MESOS_FETCHER_INFO` which we explicitly pass on even after that patch we 
> are discussing here. 
> So the things to look at should be limited towards the candidates covered 
> by those `Flags`. To be entirely clean we might want to consider passing 
> those candidates through explicitly.

Well, we certainly want to support MESOS_FETCHER_INFO and the others, so let's 
consider blacklist/whitelist.


- Adam


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


On Feb. 15, 2017, 5:31 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56711/
> -------
> 
> (Updated Feb. 15, 2017, 5:31 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, and Greg Mann.
> 
> 
> Bugs: MESOS-6751 and MESOS-7133
> https://issues.apache.org/jira/browse/MESOS-6751
> https://issues.apache.org/jira/browse/MESOS-7133
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 9ec38dc95dddfcd990369d0146986e20b15da1a0 
> 
> Diff: https://reviews.apache.org/r/56711/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Review Request 56742: Updated CHANGELOG for Mesos 1.2.0 release.

[Adam B]

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

Review request for mesos.


Repository: mesos


Description
---

Updated CHANGELOG for Mesos 1.2.0 release.


Diffs
-

  CHANGELOG 1478ad4dd3b20fc768fd679f81d803acf0995ad2 

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


Testing
---


Thanks,

Adam B

Re: Review Request 56711: Fixed fetcher to not pick up environment variables it should not see.

[Adam B]

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




src/slave/containerizer/fetcher.cpp (lines 827 - 828)
<https://reviews.apache.org/r/56711/#comment237661>

How confident are you that the fetcher doesn't actually need any of these 
env vars? How can we test/prove it?


- Adam B


On Feb. 15, 2017, 5:31 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56711/
> ---
> 
> (Updated Feb. 15, 2017, 5:31 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, and Greg Mann.
> 
> 
> Bugs: MESOS-6751 and MESOS-7133
> https://issues.apache.org/jira/browse/MESOS-6751
> https://issues.apache.org/jira/browse/MESOS-7133
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 9ec38dc95dddfcd990369d0146986e20b15da1a0 
> 
> Diff: https://reviews.apache.org/r/56711/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 56178: Enabled the authorizer to work with MULTI_ROLE frameworks.

[Adam B]

> On Feb. 9, 2017, 1:59 p.m., Benjamin Mahler wrote:
> > src/authorizer/local/authorizer.cpp, line 248
> > <https://reviews.apache.org/r/56178/diff/8/?file=1628302#file1628302line248>
> >
> > Not yours, but I find it rather confusing as to what the object value 
> > is, looking at the other code, is it the role? It would be nice to clarify 
> > how one figures out what `value` represents.

That's part of the reason why we're moving away from 'value' to more explicit 
FrameworkInfo/FooInfos, from which the authorizer can authorize based on 
any/many fields.

Until then, the best documentation is in authorizer.proto:
```
  // `REGISTER_FRAMEWORK` will have an object with `FrameworkInfo` set.
  // The `_WITH_ROLE` alias is deprecated and will be removed after
  // Mesos 1.2's deprecation cycle ends. The `value` field will continue
  // to be set until that time.
  REGISTER_FRAMEWORK = 1;
  REGISTER_FRAMEWORK_WITH_ROLE = 1;
```


- Adam


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


On Feb. 9, 2017, 1:26 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56178/
> ---
> 
> (Updated Feb. 9, 2017, 1:26 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7022
> https://issues.apache.org/jira/browse/MESOS-7022
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the local authorizer so that MULTI_ROLE frameworks can be
> authorized.
> 
> For non-MULTI_ROLE frameworks we continue to support use of the
> deprecated 'value' field in the authorization request's 'Object';
> however for MULTI_ROLE frameworks the 'value' field will not be set,
> and authorizers still relying on it should be updated to instead use
> the object's 'framework_info' field to extract roles to authorize
> against from.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> b98e1fcdf2ee5ec1f6ac0be6f8accdefaa390a09 
>   src/master/master.cpp 620919ecfe85367b5c1281afc5216cc20e5e2e3c 
> 
> Diff: https://reviews.apache.org/r/56178/diff/
> 
> 
> Testing
> ---
> 
> Tested on various configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 56053: Added a 'SECRET' type to the 'Environment' protobuf message.

[Adam B]

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



LGTM, but you're mostly just updating the tests to uses VALUE, and there's only 
a single test for the REFERENCE type. Is there nothing else we should test 
there?

- Adam B


On Feb. 8, 2017, 10:34 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56053/
> ---
> 
> (Updated Feb. 8, 2017, 10:34 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Kapil Arya, Jan Schlicht, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7009
> https://issues.apache.org/jira/browse/MESOS-7009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a field of type `Secret` to the
> `Environment` protobuf message, enabling the passing
> of secrets into the environments of executors and
> tasks. Additional validation and test code is added
> as well.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 34a288b6f5c2fca090a7aa7a61798e3255d6663a 
>   include/mesos/v1/mesos.proto 6638111d10f4a36cdf91dfce1019871e9839c306 
>   src/common/validation.cpp 0f1a02286d8431acfee6136e8ada49b0ac746897 
>   src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c 
>   src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 
>   src/tests/master_validation_tests.cpp 
> 0c2649089d7fd29eb021ac75c71e6a74368577dc 
>   src/tests/slave_validation_tests.cpp 
> 3d17799ed04951fb56524db0f5d89347192300b2 
> 
> Diff: https://reviews.apache.org/r/56053/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56052: Added the 'Secret' protobuf message.

[Adam B]

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


Ship it!




LGTM (no new tests though)


include/mesos/mesos.proto (line 1986)
<https://reviews.apache.org/r/56052/#comment236730>

And if key is left out, then the entire secret contents are returned/used?



include/mesos/mesos.proto (line 1994)
<https://reviews.apache.org/r/56052/#comment236731>

Why is this optional? Forward-looking for proto3?
I can't imagine any use for a Value secret without a value. Optional could 
still be useful if we eventually decide to replace `string text` with a 
different type


- Adam B


On Feb. 8, 2017, 10:33 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56052/
> ---
> 
> (Updated Feb. 8, 2017, 10:33 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Kapil Arya, Jan Schlicht, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-6996
> https://issues.apache.org/jira/browse/MESOS-6996
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a new `Secret` protobuf message which
> is designed to serve as a generic mechanism for passing
> priviledged information within Mesos.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 34a288b6f5c2fca090a7aa7a61798e3255d6663a 
>   include/mesos/v1/mesos.proto 6638111d10f4a36cdf91dfce1019871e9839c306 
> 
> Diff: https://reviews.apache.org/r/56052/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56178: Enabled the authorizer to work with MULTI_ROLE frameworks.

[Adam B]

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


Fix it, then Ship it!




Thanks!


src/authorizer/local/authorizer.cpp (line 247)
<https://reviews.apache.org/r/56178/#comment236726>

MESOS-7091?


- Adam B


On Feb. 9, 2017, 12:59 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56178/
> ---
> 
> (Updated Feb. 9, 2017, 12:59 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7022
> https://issues.apache.org/jira/browse/MESOS-7022
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the local authorizer so that MULTI_ROLE frameworks can be
> authorized.
> 
> For non-MULTI_ROLE frameworks we continue to support use of the
> deprecated 'value' field in the authorization request's 'Object';
> however for MULTI_ROLE frameworks the 'value' field will not be set,
> and authorizers still relying on it should be updated to instead use
> the object's 'framework_info' field to extract roles to authorize
> against from.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> b98e1fcdf2ee5ec1f6ac0be6f8accdefaa390a09 
>   src/master/master.cpp 620919ecfe85367b5c1281afc5216cc20e5e2e3c 
> 
> Diff: https://reviews.apache.org/r/56178/diff/
> 
> 
> Testing
> ---
> 
> Tested on various configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 56178: Enabled the authorizer to work with MULTI_ROLE frameworks.

[Adam B]

> On Feb. 3, 2017, 2:26 a.m., Adam B wrote:
> > src/authorizer/local/authorizer.cpp, line 244
> > <https://reviews.apache.org/r/56178/diff/1/?file=1621348#file1621348line244>
> >
> > Seems like framework_info is always set, so how/why would we ever fall 
> > through to the other cases?
> 
> Benjamin Bannier wrote:
> Yes, this is currently dead code. I was also wondering whether it should 
> be removed, but decided against it since it provides some level of redundancy 
> as long as `value` still exists and code in the master and in authorizers 
> might not evolve consistently.
> 
> Do you believe it should be removed?
> 
> Alexander Rojas wrote:
> The main reason the `object->value` is still there, is that the local 
> authorizer is a reference implementation for module writers who want to build 
> their own modules, as such, it does provide a reference. I myself will vote 
> to remove the `value` field if possible. However, we makred as deprecated in 
> November 2016 which means we need it there until June (at the same time it 
> had said it is supposed to be removed on 1.2).
> 
> Adam B wrote:
> Alright. Please add a comment explaining why we're keeping dead code 
> around, so I am not tempted to delete it next time I see it.
> 
> Benjamin Bannier wrote:
> Yes, a comment is in order here, updated the patch. I also slipped in a 
> `TODO` to remove this branch when `value` is purged.
> 
> Adam B wrote:
> This code is in the LocalAuthorizer, which doesn't support other custom 
> authorizers. While Mesos master/agent should continue to set both 
> FrameworkInfo and `value`, to support custom authorizers, an authorizer 
> implementation like LocalAuthorizer doesn't need to look at `value` if it's 
> already looking at FrameworkInfo. Even as a reference implementation, we 
> should not show an example of using the deprecated field. I think we can 
> safely remove this entire block.
> I am, however, happy that we now have MESOS-7073 to track the eventual 
> removal of the value field altogether.
> 
> Benjamin Bannier wrote:
> This requires updating a number of tests which still set value. I would 
> like to avoid pulling this work into this change and created MESOS-7091 to 
> track this work.

Ok, dropping this since we have a JIRA for it now.


- Adam


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


On Feb. 9, 2017, 12:59 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56178/
> ---
> 
> (Updated Feb. 9, 2017, 12:59 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7022
> https://issues.apache.org/jira/browse/MESOS-7022
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the local authorizer so that MULTI_ROLE frameworks can be
> authorized.
> 
> For non-MULTI_ROLE frameworks we continue to support use of the
> deprecated 'value' field in the authorization request's 'Object';
> however for MULTI_ROLE frameworks the 'value' field will not be set,
> and authorizers still relying on it should be updated to instead use
> the object's 'framework_info' field to extract roles to authorize
> against from.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> b98e1fcdf2ee5ec1f6ac0be6f8accdefaa390a09 
>   src/master/master.cpp 620919ecfe85367b5c1281afc5216cc20e5e2e3c 
> 
> Diff: https://reviews.apache.org/r/56178/diff/
> 
> 
> Testing
> ---
> 
> Tested on various configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 56178: Enabled the authorizer to work with MULTI_ROLE frameworks.

[Adam B]

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



Thanks for your patience. Just a few more minor concerns.


src/master/master.cpp (lines 2175 - 2176)
<https://reviews.apache.org/r/56178/#comment236707>

s/cannot be used to authorize `MULTI_ROLE` frameworks/will get an empty 
string value for `MULTI_ROLE` frameworks/



src/master/master.cpp (line 2177)
<https://reviews.apache.org/r/56178/#comment236705>

This could use a reference to MESOS-7073 (especially if we remove the other 
reference in LocalAuthorizer)



src/master/master.cpp (lines 2178 - 2179)
<https://reviews.apache.org/r/56178/#comment236708>

Why not check `if(protobuf::framework::getRoles(frameworkInfo).size() <= 
1)` instead of checking the capability? A legacy authorizer could still 
authorize a multi-role-capable framework if it's only trying to register with a 
single role.


- Adam B


On Feb. 7, 2017, 2:26 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56178/
> ---
> 
> (Updated Feb. 7, 2017, 2:26 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7022
> https://issues.apache.org/jira/browse/MESOS-7022
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the local authorizer so that MULTI_ROLE frameworks can be
> authorized.
> 
> For non-MULTI_ROLE frameworks we continue to support use of the
> deprecated 'value' field in the authorization request's 'Object';
> however for MULTI_ROLE frameworks the 'value' field will not be set,
> and authorizers still relying on it should be updated to instead use
> the object's 'framework_info' field to extract roles to authorize
> against from.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> b98e1fcdf2ee5ec1f6ac0be6f8accdefaa390a09 
>   src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 
> 
> Diff: https://reviews.apache.org/r/56178/diff/
> 
> 
> Testing
> ---
> 
> Tested on various configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 56178: Enabled the authorizer to work with MULTI_ROLE frameworks.

[Adam B]

> On Feb. 3, 2017, 2:26 a.m., Adam B wrote:
> > src/authorizer/local/authorizer.cpp, line 244
> > <https://reviews.apache.org/r/56178/diff/1/?file=1621348#file1621348line244>
> >
> > Seems like framework_info is always set, so how/why would we ever fall 
> > through to the other cases?
> 
> Benjamin Bannier wrote:
> Yes, this is currently dead code. I was also wondering whether it should 
> be removed, but decided against it since it provides some level of redundancy 
> as long as `value` still exists and code in the master and in authorizers 
> might not evolve consistently.
> 
> Do you believe it should be removed?
> 
> Alexander Rojas wrote:
> The main reason the `object->value` is still there, is that the local 
> authorizer is a reference implementation for module writers who want to build 
> their own modules, as such, it does provide a reference. I myself will vote 
> to remove the `value` field if possible. However, we makred as deprecated in 
> November 2016 which means we need it there until June (at the same time it 
> had said it is supposed to be removed on 1.2).
> 
> Adam B wrote:
> Alright. Please add a comment explaining why we're keeping dead code 
> around, so I am not tempted to delete it next time I see it.
> 
> Benjamin Bannier wrote:
> Yes, a comment is in order here, updated the patch. I also slipped in a 
> `TODO` to remove this branch when `value` is purged.

This code is in the LocalAuthorizer, which doesn't support other custom 
authorizers. While Mesos master/agent should continue to set both FrameworkInfo 
and `value`, to support custom authorizers, an authorizer implementation like 
LocalAuthorizer doesn't need to look at `value` if it's already looking at 
FrameworkInfo. Even as a reference implementation, we should not show an 
example of using the deprecated field. I think we can safely remove this entire 
block.
I am, however, happy that we now have MESOS-7073 to track the eventual removal 
of the value field altogether.


- Adam


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


On Feb. 7, 2017, 2:26 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56178/
> ---
> 
> (Updated Feb. 7, 2017, 2:26 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7022
> https://issues.apache.org/jira/browse/MESOS-7022
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the local authorizer so that MULTI_ROLE frameworks can be
> authorized.
> 
> For non-MULTI_ROLE frameworks we continue to support use of the
> deprecated 'value' field in the authorization request's 'Object';
> however for MULTI_ROLE frameworks the 'value' field will not be set,
> and authorizers still relying on it should be updated to instead use
> the object's 'framework_info' field to extract roles to authorize
> against from.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> b98e1fcdf2ee5ec1f6ac0be6f8accdefaa390a09 
>   src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 
> 
> Diff: https://reviews.apache.org/r/56178/diff/
> 
> 
> Testing
> ---
> 
> Tested on various configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 56178: Enabled the authorizer to work with MULTI_ROLE frameworks.

[Adam B]

> On Feb. 7, 2017, 1:16 a.m., Adam B wrote:
> > src/master/master.cpp, line 2178
> > <https://reviews.apache.org/r/56178/diff/5/?file=1626265#file1626265line2178>
> >
> > What does the framework's capability have to do with whether a custom 
> > authorizer can support multi-role authorization?
> > If I have an old authorizer module, and multi-role (phase 1) capable 
> > frameworks, I will only be able to authorize frameworks with a single role 
> > (regardless of capability). For a multi-role framework, I could authorize a 
> > single role from the list, but that essentially provides a back-door for 
> > those frameworks to use other roles. Or I could call the authorizer 
> > multiple times, one for each role. But we wouldn't want to do that for 
> > multi-role capable authorizers. Maybe we need the concept of "module 
> > capabilites" now too?
> > Am I overthinking this?
> 
> Benjamin Bannier wrote:
> There is only a weak relation between this framework capability and the 
> capabilities of authorizers. My thinking here was roughly:
> 
> * The way authorization works ("send at most a single authorization 
> request per authorizable action") all information required for authorization 
> needs to be packed into a single authorization request.
> * There are authorizers relying only on the `value` field to determine 
> the role of a framework; this approach is deprecated.
> * In the current authorization approach there is no meaningful value for 
> `value` for multirole frameworks.
> * Multirole frameworks are a new feature so not updating deprecated 
> features to work with them does not break backwards compatibility.
> 
> With this change we
> 
> * maintain the current, single-request authorization approach,
> * avoid breaking authorizers assuming frameworks can only have a single 
> role,
> * avoid breaking authorizers using `value` and assuming a single-role 
> world, and
> * enable authorizers using `framework_info` to authorize multirole 
> frameworks.
> 
> This is very much in line with how e.g., `FrameworkInfo` has both a 
> (deprecated) `role` and a `roles` field, and the correct action is taken 
> depending on framework capabilities.
> 
> Would we instead e.g., send an authorization request for each role, we 
> wouldn't just introduce new code to use a deprecated field (`value`), but we 
> would also introduce a lot of potential overhead, all while ignoring the 
> existence of `framework_info` which already now bundles all this information 
> and can be sent as part of an authorization request.
> 
> Adam B wrote:
> Thanks for the explanation.
> I agree that sending an authz request for each role is overkill just to 
> support a deprecated field. Forget about that.
> My only concern is that authz decisions generally prefer to deny access 
> when they don't know what to do, and an old authorizer that only looks at the 
> `value` field will try to authorize a Framework registering with no role. Is 
> that the same as registering with `*`? If not, they probably already handle 
> that as an error. If so, they would probably authorize based on `*` (allow), 
> when they should be authorizing based on `foo` and `bar` (deny). If that's a 
> serious concern, I'd rather remove the `value` field or otherwise break their 
> compilation so they are forced to review the API changes.
> 
> Benjamin Bannier wrote:
> > My only concern is that authz decisions generally prefer to deny access 
> when they don't know what to do, and an old authorizer that only looks at the 
> value field will try to authorize a Framework registering with no role. Is 
> that the same as registering with *?
> 
> Yes, for single role frameworks, if no role is set, we currently and in 
> the future will explicitly propagate the default `*` -- that's 
> (unfortunately) part of our `FrameworkInfo` proto API.
> 
> For multirole frameworks we leave `value` unset, and since there is no 
> default value for `value` in the `Object` proto, consumers of this value 
> would get the default value for the string type (empty string). So 
> authorizers relying on `value` and not explicitly checking if `value` has 
> been set, would see different behavior for new multirole-capable frameworks 
> (empty string vs. `*`) which I believe is good and should give them some 
> pause. I'd argue that since we explicitly propagate even an unset single-role 
> framework role as `*` to authorizers, having rules to match the empty string 
> in an authorizer implementation seems unlikely as the empty string is not a 
> valid role value.
>

Re: Review Request 56178: Enabled the authorizer to work with MULTI_ROLE frameworks.

[Adam B]

> On Feb. 7, 2017, 1:16 a.m., Adam B wrote:
> > src/master/master.cpp, line 2178
> > <https://reviews.apache.org/r/56178/diff/5/?file=1626265#file1626265line2178>
> >
> > What does the framework's capability have to do with whether a custom 
> > authorizer can support multi-role authorization?
> > If I have an old authorizer module, and multi-role (phase 1) capable 
> > frameworks, I will only be able to authorize frameworks with a single role 
> > (regardless of capability). For a multi-role framework, I could authorize a 
> > single role from the list, but that essentially provides a back-door for 
> > those frameworks to use other roles. Or I could call the authorizer 
> > multiple times, one for each role. But we wouldn't want to do that for 
> > multi-role capable authorizers. Maybe we need the concept of "module 
> > capabilites" now too?
> > Am I overthinking this?
> 
> Benjamin Bannier wrote:
> There is only a weak relation between this framework capability and the 
> capabilities of authorizers. My thinking here was roughly:
> 
> * The way authorization works ("send at most a single authorization 
> request per authorizable action") all information required for authorization 
> needs to be packed into a single authorization request.
> * There are authorizers relying only on the `value` field to determine 
> the role of a framework; this approach is deprecated.
> * In the current authorization approach there is no meaningful value for 
> `value` for multirole frameworks.
> * Multirole frameworks are a new feature so not updating deprecated 
> features to work with them does not break backwards compatibility.
> 
> With this change we
> 
> * maintain the current, single-request authorization approach,
> * avoid breaking authorizers assuming frameworks can only have a single 
> role,
> * avoid breaking authorizers using `value` and assuming a single-role 
> world, and
> * enable authorizers using `framework_info` to authorize multirole 
> frameworks.
> 
> This is very much in line with how e.g., `FrameworkInfo` has both a 
> (deprecated) `role` and a `roles` field, and the correct action is taken 
> depending on framework capabilities.
> 
> Would we instead e.g., send an authorization request for each role, we 
> wouldn't just introduce new code to use a deprecated field (`value`), but we 
> would also introduce a lot of potential overhead, all while ignoring the 
> existence of `framework_info` which already now bundles all this information 
> and can be sent as part of an authorization request.

Thanks for the explanation.
I agree that sending an authz request for each role is overkill just to support 
a deprecated field. Forget about that.
My only concern is that authz decisions generally prefer to deny access when 
they don't know what to do, and an old authorizer that only looks at the 
`value` field will try to authorize a Framework registering with no role. Is 
that the same as registering with `*`? If not, they probably already handle 
that as an error. If so, they would probably authorize based on `*` (allow), 
when they should be authorizing based on `foo` and `bar` (deny). If that's a 
serious concern, I'd rather remove the `value` field or otherwise break their 
compilation so they are forced to review the API changes.


- Adam


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


On Feb. 7, 2017, 2:26 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56178/
> ---
> 
> (Updated Feb. 7, 2017, 2:26 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7022
> https://issues.apache.org/jira/browse/MESOS-7022
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the local authorizer so that MULTI_ROLE frameworks can be
> authorized.
> 
> For non-MULTI_ROLE frameworks we continue to support use of the
> deprecated 'value' field in the authorization request's 'Object';
> however for MULTI_ROLE frameworks the 'value' field will not be set,
> and authorizers still relying on it should be updated to instead use
> the object's 'framework_info' field to extract roles to authorize
> against from.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> b98e1fcdf2ee5ec1f6ac0be6f8accdefaa390a09 
>   src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 
> 
> Diff: https://reviews.apache.org/r/56178/diff/
> 
> 
> Testing
> ---
> 
> Tested on various configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 56359: Add framework principal to the slave state endpoint

[Adam B]

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


Ship it!




Ship It!

- Adam B


On Feb. 6, 2017, 6:14 p.m., Jeff Malnick wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56359/
> ---
> 
> (Updated Feb. 6, 2017, 6:14 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-7071
> https://issues.apache.org/jira/browse/MESOS-7071
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit adds the framework principal to the slave state endpoint. The 
> documentation for the agent state API needs to be updated to reflect this 
> change.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp d68c9adc1db43c2e853c8b2290705fdc7739d45c 
> 
> Diff: https://reviews.apache.org/r/56359/diff/
> 
> 
> Testing
> ---
> 
> Tests are in progress.
> 
> 
> Thanks,
> 
> Jeff Malnick
> 
>

Re: Review Request 56178: Enabled the authorizer to work with MULTI_ROLE frameworks.

[Adam B]

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



There's a difference between backwards compatibility with frameworks that are 
not capable of multi-role and backwards compatibility with authorizer modules 
that are not multi-role capable. Let's not confuse the two.


src/master/master.cpp (line 2177)
<https://reviews.apache.org/r/56178/#comment236234>

I find it helpful to mention when the deprecation cycle started (release 
date or version), so we can compute when it ends, based on our current policy.



src/master/master.cpp (line 2178)
<https://reviews.apache.org/r/56178/#comment236235>

What does the framework's capability have to do with whether a custom 
authorizer can support multi-role authorization?
If I have an old authorizer module, and multi-role (phase 1) capable 
frameworks, I will only be able to authorize frameworks with a single role 
(regardless of capability). For a multi-role framework, I could authorize a 
single role from the list, but that essentially provides a back-door for those 
frameworks to use other roles. Or I could call the authorizer multiple times, 
one for each role. But we wouldn't want to do that for multi-role capable 
authorizers. Maybe we need the concept of "module capabilites" now too?
Am I overthinking this?


- Adam B


On Feb. 7, 2017, 12:32 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56178/
> ---
> 
> (Updated Feb. 7, 2017, 12:32 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7022
> https://issues.apache.org/jira/browse/MESOS-7022
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the local authorizer so that MULTI_ROLE frameworks can be
> authorized.
> 
> For non-MULTI_ROLE frameworks we continue to support use of the
> deprecated 'value' field in the authorization request's 'Object';
> however for MULTI_ROLE frameworks the 'value' field will not be set,
> and authorizers still relying on it should be updated to instead use
> the object's 'framework_info' field to extract roles to authorize
> against from.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> b98e1fcdf2ee5ec1f6ac0be6f8accdefaa390a09 
>   src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 
> 
> Diff: https://reviews.apache.org/r/56178/diff/
> 
> 
> Testing
> ---
> 
> Tested on various configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 56178: Enabled the authorizer to work with MULTI_ROLE frameworks.

[Adam B]

> On Feb. 3, 2017, 2:26 a.m., Adam B wrote:
> > src/authorizer/local/authorizer.cpp, line 244
> > <https://reviews.apache.org/r/56178/diff/1/?file=1621348#file1621348line244>
> >
> > Seems like framework_info is always set, so how/why would we ever fall 
> > through to the other cases?
> 
> Benjamin Bannier wrote:
> Yes, this is currently dead code. I was also wondering whether it should 
> be removed, but decided against it since it provides some level of redundancy 
> as long as `value` still exists and code in the master and in authorizers 
> might not evolve consistently.
> 
> Do you believe it should be removed?
> 
> Alexander Rojas wrote:
> The main reason the `object->value` is still there, is that the local 
> authorizer is a reference implementation for module writers who want to build 
> their own modules, as such, it does provide a reference. I myself will vote 
> to remove the `value` field if possible. However, we makred as deprecated in 
> November 2016 which means we need it there until June (at the same time it 
> had said it is supposed to be removed on 1.2).

Alright. Please add a comment explaining why we're keeping dead code around, so 
I am not tempted to delete it next time I see it.


- Adam


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


On Feb. 6, 2017, 7:44 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56178/
> ---
> 
> (Updated Feb. 6, 2017, 7:44 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7022
> https://issues.apache.org/jira/browse/MESOS-7022
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the local authorizer so that MULTI_ROLE frameworks can be
> authorized.
> 
> For non-MULTI_ROLE frameworks we continue to support use of the
> deprecated 'value' field in the authorization request's 'Object';
> however for MULTI_ROLE frameworks the 'value' field will not be set,
> and authorizers still relying on it should be updated to instead use
> the object's 'framework_info' field to extract roles to authorize
> against from.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> b98e1fcdf2ee5ec1f6ac0be6f8accdefaa390a09 
>   src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 
> 
> Diff: https://reviews.apache.org/r/56178/diff/
> 
> 
> Testing
> ---
> 
> Tested on various configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 56178: Enabled the authorizer to work with MULTI_ROLE frameworks.

[Adam B]

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




src/authorizer/local/authorizer.cpp (line 240)
<https://reviews.apache.org/r/56178/#comment235689>

Does this return a list with a single role, if the framework fills in 
`framework_info.role` instead of `framework_info.roles`?

Is there a reason you can't just use `framework_info.roles()` here instead 
of using the protobuf util?



src/authorizer/local/authorizer.cpp (line 243)
<https://reviews.apache.org/r/56178/#comment235690>

Seems like framework_info is always set, so how/why would we ever fall 
through to the other cases?



src/master/master.cpp (line 2175)
<https://reviews.apache.org/r/56178/#comment235687>

    s/use/used/


- Adam B


On Feb. 1, 2017, 9:06 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56178/
> ---
> 
> (Updated Feb. 1, 2017, 9:06 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7022
> https://issues.apache.org/jira/browse/MESOS-7022
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the local authorizer so that MULTI_ROLE frameworks can be
> authorized.
> 
> For non-MULTI_ROLE frameworks we continue to support use of the
> deprecated 'value' field in the authorization request's 'Object';
> however for MULTI_ROLE frameworks the 'value' field will not be set,
> and authorizers still relying on it should be updated to instead use
> the object's 'framework_info' field to extract roles to authorize
> against from.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> b98e1fcdf2ee5ec1f6ac0be6f8accdefaa390a09 
>   src/master/master.cpp 284566ca72bd5c6bd581db9b65d404f86aa7bf61 
> 
> Diff: https://reviews.apache.org/r/56178/diff/
> 
> 
> Testing
> ---
> 
> Tested on various configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 56052: Added the 'Secret' protobuf message.

[Adam B]

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




include/mesos/mesos.proto (lines 1967 - 1968)
<https://reviews.apache.org/r/56052/#comment235683>

What are these two fields used for? Can you give an example to demonstrate 
the difference between name and key?


- Adam B


On Feb. 2, 2017, 11:39 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56052/
> ---
> 
> (Updated Feb. 2, 2017, 11:39 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Kapil Arya, Jan Schlicht, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-6996
> https://issues.apache.org/jira/browse/MESOS-6996
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a new `Secret` protobuf message which
> is designed to serve as a generic mechanism for passing
> priviledged information within Mesos.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto a08b8fb2edf26ae730467557ae0e33e3d4252593 
>   include/mesos/v1/mesos.proto 4a4609e7db659252ef7f1ca61f8ba079b901b7d0 
> 
> Diff: https://reviews.apache.org/r/56052/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

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

[Adam B]

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



Ping. Do we still need this?

- Adam B


On Nov. 22, 2016, 12:26 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53998/
> ---
> 
> (Updated Nov. 22, 2016, 12:26 p.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
> 
> 
> Description
> ---
> 
> When being used on a task run by a command executor, the hook
> `slavePreLaunchDockerEnvironmentDecorator` did not allow for extending
> the executor's own environment but only the environment of the task.
> By using the hook returned environment for the executor in any case
> and supplying a task specific environment for the command executor,
> that limitation is fixed.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp c2ed5240aab9ea9d1a386c44c94e5ae7e98d313c 
> 
> Diff: https://reviews.apache.org/r/53998/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> * WIP - functional testing pending - WIP *
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 55152: Fixed nested container docs and added a link to home.md.

[Adam B]

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



minor comments


docs/nested-container-and-task-group.md (line 2)
<https://reviews.apache.org/r/55152/#comment231601>

Maybe put "pods" in the page title, for SEO?



docs/nested-container-and-task-group.md (line 125)
<https://reviews.apache.org/r/55152/#comment231604>

Have you verified this anchor link using `site/Dockerfile` to generate the 
website, rather than just following github markdown?


- Adam B


On Jan. 3, 2017, 1:44 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55152/
> ---
> 
> (Updated Jan. 3, 2017, 1:44 p.m.)
> 
> 
> Review request for mesos, Adam B, Anand Mazumdar, Avinash sridharan, Jie Yu, 
> Neil Conway, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed nested container docs and added a link to home.md.
> 
> 
> Diffs
> -
> 
>   docs/home.md 091dae20b0a26aac03186edaefab93c54b3063b4 
>   docs/nested-container-and-task-group.md 
> debf9f82622557aff915dd04ef06a50793f496b6 
> 
> Diff: https://reviews.apache.org/r/55152/diff/
> 
> 
> Testing
> ---
> 
> https://github.com/Gilbert88/mesos/blob/doc_pod/docs/nested-container-and-task-group.md
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 54803: Fixed `SlaveTests` to pass when `HAS_AUTHENTICATION` is undefined.

[Adam B]

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



Looks pretty reasonable to me, but I've got a few questions about your 
pausing/advancing.


src/tests/slave_tests.cpp (line 228)
<https://reviews.apache.org/r/54803/#comment231001>

Do you really need to advance by both values, or would advancing by the 
larger of the two be sufficient?



src/tests/slave_tests.cpp (line 3648)
<https://reviews.apache.org/r/54803/#comment231000>

Why did you need to move the pause all the way up here, instead of just 
before the first `advance`? Please add a comment about why we're pausing.


- Adam B


On Dec. 20, 2016, 2:32 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54803/
> ---
> 
> (Updated Dec. 20, 2016, 2:32 p.m.)
> 
> 
> Review request for mesos, Adam B, Andrew Schwartzmeyer, Daniel Pravat, Greg 
> Mann, John Kordich, Joseph Wu, and Vinod Kone.
> 
> 
> Bugs: MESOS-6803
> https://issues.apache.org/jira/browse/MESOS-6803
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, when `HAS_AUTHENTICATION` is undefined, the Agent will
> use `delay` to schedule a random time in the future to register with the
> Master, to avoid the thundering herd problem after a Master failover.
> The authentication codepath, in contrast, schedules the registration
> immediately.
> 
> In tests where we have `Clock::pause`'d when we are supposed to be
> registering the slave, the authention codepath will succeeed, while
> no-authentication codepath will hang forever.
> 
> A much more detailed analysis of this situation exists in MESOS-6803.
> 
> This commit will resolve this issue for `slave_tests.cpp` by changing
> the tests to not use `Clock::pause` when we are waiting for Agent
> registration.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp d956a326ef29bf29837e0587a14bae457147cbca 
> 
> Diff: https://reviews.apache.org/r/54803/diff/
> 
> 
> Testing
> ---
> 
> Added `delay` to the call to `authenticate` in `Slave::detected`, ran tests 
> to find failing tests in `SlaveTest.*`, then fixed, then ran again.
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 54731: Fixed a check bug in LAUNCH_NESTED_CONTAINER_SESSION_CALL.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On Dec. 13, 2016, 8:15 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54731/
> ---
> 
> (Updated Dec. 13, 2016, 8:15 p.m.)
> 
> 
> Review request for mesos, Adam B and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `attachContainerOutput` could return a non OK response (e.g., authz
> error). We should return the response in this case instead of failing
> the CHECK.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 56c2879c41a84f959107e28410a46aeb11457975 
> 
> Diff: https://reviews.apache.org/r/54731/diff/
> 
> 
> Testing
> ---
> 
> I will add a test in the next review.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

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

[Adam B]

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


Fix it, then Ship it!





src/slave/http.cpp (line 904)
<https://reviews.apache.org/r/54662/#comment230044>

No need to `defer(slave->self)` here since the lambda doesn't use anything 
from the agent (level, duration, approver are passed in directly), and the 
result is a `dispatch()`? Let me double-check with somebody here so I can 
commit this today.



src/slave/http.cpp (lines 905 - 906)
<https://reviews.apache.org/r/54662/#comment230042>

Looks like weird wrapping to me. I'd prefer to see it wrapped after `](` 
like other lambdas


- Adam B


On Dec. 12, 2016, 6:37 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54662/
> ---
> 
> (Updated Dec. 12, 2016, 6:37 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-6670
> https://issues.apache.org/jira/browse/MESOS-6670
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the stub which allows only authorized users to change the log
> level of Mesos using the HTTP API v1.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 8a71eadd8f26df147ddea800221b6f243280bf3b 
> 
> Diff: https://reviews.apache.org/r/54662/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

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

[Adam B]

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


Ship it!




Ship It!

- Adam B


On Dec. 13, 2016, 6:47 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54538/
> ---
> 
> (Updated Dec. 13, 2016, 6:47 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-6670
> https://issues.apache.org/jira/browse/MESOS-6670
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enables fine grained authorization for the v1 API call `GET_CONTAINERS`.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 8a71eadd8f26df147ddea800221b6f243280bf3b 
>   src/slave/slave.hpp 059eb770051f54d5d1fc116bd491f460ee177b0a 
> 
> Diff: https://reviews.apache.org/r/54538/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

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

[Adam B]

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


Fix it, then Ship it!





src/slave/http.cpp (line 775)
<https://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 generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54661/
> ---
> 
> (Updated Dec. 13, 2016, 6:49 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-6670
> https://issues.apache.org/jira/browse/MESOS-6670
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the stub which allows for restriction of users when attempting
> to access the `GET_FLAGS` API v1 call.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 8a71eadd8f26df147ddea800221b6f243280bf3b 
> 
> Diff: https://reviews.apache.org/r/54661/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

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

[Adam B]

---
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., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54700/
> ---
> 
> (Updated Dec. 13, 2016, 4:34 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed bug that prevented window size updates in the IOSwitchboard.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> f900924dd55c42966deb14c65fca380bebc86e2f 
> 
> Diff: https://reviews.apache.org/r/54700/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Manual test with a TTY client to make sure the window resized properly.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

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

[Adam B]

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



Looks good, but I'm tired. I'll merge all these in 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 that?


- Adam B


On Dec. 12, 2016, 2:46 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54538/
> ---
> 
> (Updated Dec. 12, 2016, 2:46 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-6670
> https://issues.apache.org/jira/browse/MESOS-6670
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enables fine grained authorization for the v1 API call `GET_CONTAINERS`.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 8a71eadd8f26df147ddea800221b6f243280bf3b 
>   src/slave/slave.hpp 059eb770051f54d5d1fc116bd491f460ee177b0a 
> 
> Diff: https://reviews.apache.org/r/54538/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>