Re: Review Request 41472: Disallowed dynamic reservations for roles not on the role whitelist.

[Adam B]

> On Dec. 21, 2015, 7:46 a.m., Michael Park wrote:
> > src/master/http.cpp, line 1001
> > 
> >
> > We actually don't need this `if` statement. Although the `role` field 
> > is marked `optional`, we "fill it in" with the default value of `"*"` if 
> > unspecified by simply calling `role()`. `roleWhitelist` always has `"*"`.
> > 
> > 
> > https://github.com/apache/mesos/blob/master/src/master/master.cpp#L564-L565

But MPark, isn't it invalid to try to reserve resources for `*`? I thought each 
resource in the reserve operation was supposed to have a non-default role set. 
If so, we should either error here for a missing role, or ignore it here and 
error in operation::validate(), where we'd anyway need to validate for an 
explicit `*` (since it would pass the `has_role && isWhitelistedRole` test). 
Doesn't look like the current operation::validate checks that. Seems like a 
separate issue/patch though.


- Adam


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


On Dec. 16, 2015, 2:48 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41472/
> ---
> 
> (Updated Dec. 16, 2015, 2:48 p.m.)
> 
> 
> Review request for mesos, Adam B and Michael Park.
> 
> 
> Bugs: MESOS-4143
> https://issues.apache.org/jira/browse/MESOS-4143
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also added a test that dynamic reservations via the "/reserve" endpoint are
> allowed when using implicit roles.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd 
>   src/tests/reservation_endpoints_tests.cpp 
> b8edd6fafedd4c2221a8d19c1ebc71254071a8c7 
> 
> Diff: https://reviews.apache.org/r/41472/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 41472: Disallowed dynamic reservations for roles not on the role whitelist.

[Michael Park]

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



src/master/http.cpp (line 1001)


We actually don't need this `if` statement. Although the `role` field is 
marked `optional`, we "fill it in" with the default value of `"*"` if 
unspecified by simply calling `role()`. `roleWhitelist` always has `"*"`.

https://github.com/apache/mesos/blob/master/src/master/master.cpp#L564-L565


- Michael Park


On Dec. 16, 2015, 10:48 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41472/
> ---
> 
> (Updated Dec. 16, 2015, 10:48 p.m.)
> 
> 
> Review request for mesos, Adam B and Michael Park.
> 
> 
> Bugs: MESOS-4143
> https://issues.apache.org/jira/browse/MESOS-4143
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also added a test that dynamic reservations via the "/reserve" endpoint are
> allowed when using implicit roles.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd 
>   src/tests/reservation_endpoints_tests.cpp 
> b8edd6fafedd4c2221a8d19c1ebc71254071a8c7 
> 
> Diff: https://reviews.apache.org/r/41472/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 41472: Disallowed dynamic reservations for roles not on the role whitelist.

[Michael Park]

> On Dec. 17, 2015, 1:17 a.m., Michael Park wrote:
> > src/master/http.cpp, lines 1000-1006
> > 
> >
> > Hm, should this be added to `validation::operation::validate` instead? 
> > That way if/when we have frameworks reserving for specific roles, it'll 
> > just work?
> 
> Neil Conway wrote:
> I thought about that, but it would require passing either the role 
> whitelist or a pointer to the master itself into the 
> `validation::operation::validate()` -- both of which seemed like they would 
> be regrettable changes to make. What do you think?
> 
> Adam B wrote:
> I'm fine with it as is, but I'll wait for @mcypark to make the call.
> 
> Michael Park wrote:
> To me, it seems natural to have to pass whatever state is necessary to 
> perform validation properly.
> The `offer` validation for example takes `Master*` since it needs some 
> information from the master.
> 
> ```cpp
> // Validates the given offers.
> Option validate(
> const google::protobuf::RepeatedPtrField& offerIds,
> Master* master,
> Framework* framework);
> ```
> 
> Ideally, we would pass along something more constrained than "all of 
> master", similar to `CreateVolume` validation.
> 
> ```cpp
> // Validates the CREATE operation. We need slave's checkpointed
> // resources so that we can validate persistence ID uniqueness.
> Option validate(
> const Offer::Operation::Create& create,
> const Resources& checkpointedResources);
> ```
> 
> Here, `checkpointedResources` is a part of the master state. Passing 
> along the role whitelist would be similar to this case.
> 
> What do you think?
> 
> Neil Conway wrote:
> I started implementing this: 
> https://gist.github.com/neilconway/b6dc7a3b6eb84f187923
> 
> However, it also requires changes to the tests in 
> `ReserveOperationValidationTest`, which will be non-trivial. Personally, 
> given that (a) making the check in `http.cpp` is functionally fine (b) we're 
> going to remove the role whitelist in ~six months anyway, I'd vote for 
> keeping the current approach and not moving the checking of the role 
> whitelist into `::validate`. I'll add a TODO, although I suspect we'll remove 
> all this code before anyone gets around to implementing it :)

Just so I understand your points correctly, do you mean that the changes to the 
tests in `ReserveOperationValidationTest` will be non-trivial because we would 
have to start up a master, and potentially other components like allocator? If 
that's the case, it seems to me like that would only be true if we take 
`Master*` (as you have in the implementation you started). If we were to pass 
the role whitelist instead, we could simply construct an instance of 
`hashmap` within the test and pass that to `validate`, right? That 
actually seems quite clean to me.


- Michael


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


On Dec. 16, 2015, 10:48 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41472/
> ---
> 
> (Updated Dec. 16, 2015, 10:48 p.m.)
> 
> 
> Review request for mesos, Adam B and Michael Park.
> 
> 
> Bugs: MESOS-4143
> https://issues.apache.org/jira/browse/MESOS-4143
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also added a test that dynamic reservations via the "/reserve" endpoint are
> allowed when using implicit roles.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd 
>   src/tests/reservation_endpoints_tests.cpp 
> b8edd6fafedd4c2221a8d19c1ebc71254071a8c7 
> 
> Diff: https://reviews.apache.org/r/41472/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 41472: Disallowed dynamic reservations for roles not on the role whitelist.

[Neil Conway]

> On Dec. 17, 2015, 1:17 a.m., Michael Park wrote:
> > src/master/http.cpp, lines 1000-1006
> > 
> >
> > Hm, should this be added to `validation::operation::validate` instead? 
> > That way if/when we have frameworks reserving for specific roles, it'll 
> > just work?
> 
> Neil Conway wrote:
> I thought about that, but it would require passing either the role 
> whitelist or a pointer to the master itself into the 
> `validation::operation::validate()` -- both of which seemed like they would 
> be regrettable changes to make. What do you think?
> 
> Adam B wrote:
> I'm fine with it as is, but I'll wait for @mcypark to make the call.
> 
> Michael Park wrote:
> To me, it seems natural to have to pass whatever state is necessary to 
> perform validation properly.
> The `offer` validation for example takes `Master*` since it needs some 
> information from the master.
> 
> ```cpp
> // Validates the given offers.
> Option validate(
> const google::protobuf::RepeatedPtrField& offerIds,
> Master* master,
> Framework* framework);
> ```
> 
> Ideally, we would pass along something more constrained than "all of 
> master", similar to `CreateVolume` validation.
> 
> ```cpp
> // Validates the CREATE operation. We need slave's checkpointed
> // resources so that we can validate persistence ID uniqueness.
> Option validate(
> const Offer::Operation::Create& create,
> const Resources& checkpointedResources);
> ```
> 
> Here, `checkpointedResources` is a part of the master state. Passing 
> along the role whitelist would be similar to this case.
> 
> What do you think?

I started implementing this: 
https://gist.github.com/neilconway/b6dc7a3b6eb84f187923

However, it also requires changes to the tests in 
`ReserveOperationValidationTest`, which will be non-trivial. Personally, given 
that (a) making the check in `http.cpp` is functionally fine (b) we're going to 
remove the role whitelist in ~six months anyway, I'd vote for keeping the 
current approach and not moving the checking of the role whitelist into 
`::validate`. I'll add a TODO, although I suspect we'll remove all this code 
before anyone gets around to implementing it :)


- Neil


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


On Dec. 16, 2015, 10:48 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41472/
> ---
> 
> (Updated Dec. 16, 2015, 10:48 p.m.)
> 
> 
> Review request for mesos, Adam B and Michael Park.
> 
> 
> Bugs: MESOS-4143
> https://issues.apache.org/jira/browse/MESOS-4143
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also added a test that dynamic reservations via the "/reserve" endpoint are
> allowed when using implicit roles.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd 
>   src/tests/reservation_endpoints_tests.cpp 
> b8edd6fafedd4c2221a8d19c1ebc71254071a8c7 
> 
> Diff: https://reviews.apache.org/r/41472/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 41472: Disallowed dynamic reservations for roles not on the role whitelist.

[Michael Park]

> On Dec. 17, 2015, 1:17 a.m., Michael Park wrote:
> > src/master/http.cpp, lines 1000-1006
> > 
> >
> > Hm, should this be added to `validation::operation::validate` instead? 
> > That way if/when we have frameworks reserving for specific roles, it'll 
> > just work?
> 
> Neil Conway wrote:
> I thought about that, but it would require passing either the role 
> whitelist or a pointer to the master itself into the 
> `validation::operation::validate()` -- both of which seemed like they would 
> be regrettable changes to make. What do you think?
> 
> Adam B wrote:
> I'm fine with it as is, but I'll wait for @mcypark to make the call.

To me, it seems natural to have to pass whatever state is necessary to perform 
validation properly.
The `offer` validation for example takes `Master*` since it needs some 
information from the master.

```cpp
// Validates the given offers.
Option validate(
const google::protobuf::RepeatedPtrField& offerIds,
Master* master,
Framework* framework);
```

Ideally, we would pass along something more constrained than "all of master", 
similar to `CreateVolume` validation.

```cpp
// Validates the CREATE operation. We need slave's checkpointed
// resources so that we can validate persistence ID uniqueness.
Option validate(
const Offer::Operation::Create& create,
const Resources& checkpointedResources);
```

Here, `checkpointedResources` is a part of the master state. Passing along the 
role whitelist would be similar to this case.

What do you think?


- Michael


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


On Dec. 16, 2015, 10:48 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41472/
> ---
> 
> (Updated Dec. 16, 2015, 10:48 p.m.)
> 
> 
> Review request for mesos, Adam B and Michael Park.
> 
> 
> Bugs: MESOS-4143
> https://issues.apache.org/jira/browse/MESOS-4143
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also added a test that dynamic reservations via the "/reserve" endpoint are
> allowed when using implicit roles.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd 
>   src/tests/reservation_endpoints_tests.cpp 
> b8edd6fafedd4c2221a8d19c1ebc71254071a8c7 
> 
> Diff: https://reviews.apache.org/r/41472/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 41472: Disallowed dynamic reservations for roles not on the role whitelist.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40995, 41075, 41225, 41408, 41472]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 16, 2015, 10:48 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41472/
> ---
> 
> (Updated Dec. 16, 2015, 10:48 p.m.)
> 
> 
> Review request for mesos, Adam B and Michael Park.
> 
> 
> Bugs: MESOS-4143
> https://issues.apache.org/jira/browse/MESOS-4143
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also added a test that dynamic reservations via the "/reserve" endpoint are
> allowed when using implicit roles.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd 
>   src/tests/reservation_endpoints_tests.cpp 
> b8edd6fafedd4c2221a8d19c1ebc71254071a8c7 
> 
> Diff: https://reviews.apache.org/r/41472/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 41472: Disallowed dynamic reservations for roles not on the role whitelist.

[Adam B]

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

Ship it!


Looks good to me. Address MPark's concern and let's commit it!

- Adam B


On Dec. 16, 2015, 2:48 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41472/
> ---
> 
> (Updated Dec. 16, 2015, 2:48 p.m.)
> 
> 
> Review request for mesos, Adam B and Michael Park.
> 
> 
> Bugs: MESOS-4143
> https://issues.apache.org/jira/browse/MESOS-4143
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also added a test that dynamic reservations via the "/reserve" endpoint are
> allowed when using implicit roles.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd 
>   src/tests/reservation_endpoints_tests.cpp 
> b8edd6fafedd4c2221a8d19c1ebc71254071a8c7 
> 
> Diff: https://reviews.apache.org/r/41472/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 41472: Disallowed dynamic reservations for roles not on the role whitelist.

[Adam B]

> On Dec. 16, 2015, 5:17 p.m., Michael Park wrote:
> > src/master/http.cpp, lines 1000-1006
> > 
> >
> > Hm, should this be added to `validation::operation::validate` instead? 
> > That way if/when we have frameworks reserving for specific roles, it'll 
> > just work?
> 
> Neil Conway wrote:
> I thought about that, but it would require passing either the role 
> whitelist or a pointer to the master itself into the 
> `validation::operation::validate()` -- both of which seemed like they would 
> be regrettable changes to make. What do you think?

I'm fine with it as is, but I'll wait for @mcypark to make the call.


- Adam


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


On Dec. 16, 2015, 2:48 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41472/
> ---
> 
> (Updated Dec. 16, 2015, 2:48 p.m.)
> 
> 
> Review request for mesos, Adam B and Michael Park.
> 
> 
> Bugs: MESOS-4143
> https://issues.apache.org/jira/browse/MESOS-4143
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also added a test that dynamic reservations via the "/reserve" endpoint are
> allowed when using implicit roles.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd 
>   src/tests/reservation_endpoints_tests.cpp 
> b8edd6fafedd4c2221a8d19c1ebc71254071a8c7 
> 
> Diff: https://reviews.apache.org/r/41472/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 41472: Disallowed dynamic reservations for roles not on the role whitelist.

[Neil Conway]

> On Dec. 17, 2015, 1:17 a.m., Michael Park wrote:
> > src/master/http.cpp, lines 1000-1006
> > 
> >
> > Hm, should this be added to `validation::operation::validate` instead? 
> > That way if/when we have frameworks reserving for specific roles, it'll 
> > just work?

I thought about that, but it would require passing either the role whitelist or 
a pointer to the master itself into the `validation::operation::validate()` -- 
both of which seemed like they would be regrettable changes to make. What do 
you think?


- Neil


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


On Dec. 16, 2015, 10:48 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41472/
> ---
> 
> (Updated Dec. 16, 2015, 10:48 p.m.)
> 
> 
> Review request for mesos, Adam B and Michael Park.
> 
> 
> Bugs: MESOS-4143
> https://issues.apache.org/jira/browse/MESOS-4143
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also added a test that dynamic reservations via the "/reserve" endpoint are
> allowed when using implicit roles.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd 
>   src/tests/reservation_endpoints_tests.cpp 
> b8edd6fafedd4c2221a8d19c1ebc71254071a8c7 
> 
> Diff: https://reviews.apache.org/r/41472/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 41472: Disallowed dynamic reservations for roles not on the role whitelist.

[Michael Park]

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



src/master/http.cpp (lines 1000 - 1006)


Hm, should this be added to `validation::operation::validate` instead? That 
way if/when we have frameworks reserving for specific roles, it'll just work?


- Michael Park


On Dec. 16, 2015, 10:48 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41472/
> ---
> 
> (Updated Dec. 16, 2015, 10:48 p.m.)
> 
> 
> Review request for mesos, Adam B and Michael Park.
> 
> 
> Bugs: MESOS-4143
> https://issues.apache.org/jira/browse/MESOS-4143
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also added a test that dynamic reservations via the "/reserve" endpoint are
> allowed when using implicit roles.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd 
>   src/tests/reservation_endpoints_tests.cpp 
> b8edd6fafedd4c2221a8d19c1ebc71254071a8c7 
> 
> Diff: https://reviews.apache.org/r/41472/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 41472: Disallowed dynamic reservations for roles not on the role whitelist.

[Neil Conway]

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

Review request for mesos, Adam B and Michael Park.


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


Repository: mesos


Description
---

Also added a test that dynamic reservations via the "/reserve" endpoint are
allowed when using implicit roles.


Diffs
-

  src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd 
  src/tests/reservation_endpoints_tests.cpp 
b8edd6fafedd4c2221a8d19c1ebc71254071a8c7 

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


Testing
---

make check


Thanks,

Neil Conway