Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-12-02 Thread Alexander Rukletsov

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



src/master/master.cpp (lines 3029 - 3030)


Blank line? Here and below



src/master/master.cpp (lines 3032 - 3034)


We validate later on that in `principal` is `None`, reserve is aborted. 
IIUC, if `FrameworkInfo` does not specify principal, it cannot reserve 
resources. So the question is: do we need to check this first and only then 
proceed with authz?



src/master/master.cpp (line 3180)


s/Testing/Test for consistency, here and below.


- Alexander Rukletsov


On Dec. 2, 2015, 9:41 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Dec. 2, 2015, 9:41 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
>   src/tests/reservation_tests.cpp 15d180f92ec0aea99e6f3a7d0b505c62bd207b61 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-12-02 Thread Jie Yu


> On Dec. 2, 2015, 4:05 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, lines 3041-3043
> > 
> >
> > We validate later on that in `principal` is `None`, reserve is aborted. 
> > IIUC, if `FrameworkInfo` does not specify principal, it cannot reserve 
> > resources. So the question is: do we need to check this first and only then 
> > proceed with authz?

Alex, from the impl. perspective, we thought about that in the earlier 
iteration, and found that it's quite difficult to do it cleanly given the 
current structure of the code. We want validation to be done at the same place 
(`Master::_accept`).

ALso, we had some discussion on whether 'principal' in ReservationInfo needs to 
be required or not (MESOS-3064). In the future, we might want to make it 
'optional' so that a framework without principal can also reserve resources 
(it's reserved resources can be unreserved by anyone).

So I would suggest we keep the current structure and add a comment here saying 
that: currently, if framework's principal is not specified, the operation 
validation will fail in `_accept` even thought authorization might succeed.


- Jie


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


On Dec. 2, 2015, 9:41 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Dec. 2, 2015, 9:41 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
>   src/tests/reservation_tests.cpp 15d180f92ec0aea99e6f3a7d0b505c62bd207b61 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-12-02 Thread Greg Mann

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

(Updated Dec. 2, 2015, 7:55 p.m.)


Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

Added framework authorization for dynamic reservation.
Note: this review is continued from https://reviews.apache.org/r/37127/


Diffs (updated)
-

  src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
  src/tests/reservation_tests.cpp 15d180f92ec0aea99e6f3a7d0b505c62bd207b61 

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


Testing
---

This is the fifth in a chain of 5 patches. New reservation tests were added to 
`reservation_tests.cpp` to validate the authentication of framework reserve and 
unreserve operations using ACLs. `make check` was run to test after all patches 
were applied.


Thanks,

Greg Mann



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-12-02 Thread Greg Mann


> On Dec. 2, 2015, 4:05 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, lines 3041-3043
> > 
> >
> > We validate later on that in `principal` is `None`, reserve is aborted. 
> > IIUC, if `FrameworkInfo` does not specify principal, it cannot reserve 
> > resources. So the question is: do we need to check this first and only then 
> > proceed with authz?
> 
> Jie Yu wrote:
> Alex, from the impl. perspective, we thought about that in the earlier 
> iteration, and found that it's quite difficult to do it cleanly given the 
> current structure of the code. We want validation to be done at the same 
> place (`Master::_accept`).
> 
> ALso, we had some discussion on whether 'principal' in ReservationInfo 
> needs to be required or not (MESOS-3064). In the future, we might want to 
> make it 'optional' so that a framework without principal can also reserve 
> resources (it's reserved resources can be unreserved by anyone).
> 
> So I would suggest we keep the current structure and add a comment here 
> saying that: currently, if framework's principal is not specified, the 
> operation validation will fail in `_accept` even thought authorization might 
> succeed.

I added a comment along the lines of Jie's suggestion.


- Greg


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


On Dec. 2, 2015, 9:41 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Dec. 2, 2015, 9:41 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
>   src/tests/reservation_tests.cpp 15d180f92ec0aea99e6f3a7d0b505c62bd207b61 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-12-02 Thread Alexander Rukletsov


> On Dec. 2, 2015, 4:05 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, lines 3041-3043
> > 
> >
> > We validate later on that in `principal` is `None`, reserve is aborted. 
> > IIUC, if `FrameworkInfo` does not specify principal, it cannot reserve 
> > resources. So the question is: do we need to check this first and only then 
> > proceed with authz?
> 
> Jie Yu wrote:
> Alex, from the impl. perspective, we thought about that in the earlier 
> iteration, and found that it's quite difficult to do it cleanly given the 
> current structure of the code. We want validation to be done at the same 
> place (`Master::_accept`).
> 
> ALso, we had some discussion on whether 'principal' in ReservationInfo 
> needs to be required or not (MESOS-3064). In the future, we might want to 
> make it 'optional' so that a framework without principal can also reserve 
> resources (it's reserved resources can be unreserved by anyone).
> 
> So I would suggest we keep the current structure and add a comment here 
> saying that: currently, if framework's principal is not specified, the 
> operation validation will fail in `_accept` even thought authorization might 
> succeed.
> 
> Greg Mann wrote:
> I added a comment along the lines of Jie's suggestion.

Thanks a lot for the explanation and pointing me to the discussions in the 
ticket. It is very valuable for me as we design authz for quota now.


- Alexander


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


On Dec. 2, 2015, 7:55 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Dec. 2, 2015, 7:55 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
>   src/tests/reservation_tests.cpp 15d180f92ec0aea99e6f3a7d0b505c62bd207b61 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-12-02 Thread Greg Mann

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

(Updated Dec. 2, 2015, 9:41 a.m.)


Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

Added framework authorization for dynamic reservation.
Note: this review is continued from https://reviews.apache.org/r/37127/


Diffs (updated)
-

  src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
  src/tests/reservation_tests.cpp 15d180f92ec0aea99e6f3a7d0b505c62bd207b61 

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


Testing
---

This is the fifth in a chain of 5 patches. New reservation tests were added to 
`reservation_tests.cpp` to validate the authentication of framework reserve and 
unreserve operations using ACLs. `make check` was run to test after all patches 
were applied.


Thanks,

Greg Mann



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-12-01 Thread Michael Park

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



src/master/master.cpp (lines 3190 - 3191)


As I asked and pointed out in https://reviews.apache.org/r/39987/, this is 
not true for launch task operations.

We can see below that launch task validation is performed after 
authorization. Couldn't we do the same thing?

```cpp
Future authorization = authorizations.front();
authorizations.pop_front();

...

CHECK(!authorization.isDiscarded());

if (authorization.isFailed() || !authorization.get()) {
  // authorization failed; drop or send status update.
}

Option error = <...>::validate(...);
if (error.isSome()) {
  // validation failed: drop or send status update.
}

...
```



src/master/master.cpp (lines 3201 - 3206)


This case is actually a validation error, yet our error message is 
"authorization failure" which is a bit inaccurate. Seems to me like a result of 
bundling validation within authorization.


- Michael Park


On Nov. 30, 2015, 9:20 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Nov. 30, 2015, 9:20 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
>   src/tests/reservation_tests.cpp 15d180f92ec0aea99e6f3a7d0b505c62bd207b61 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-11-30 Thread Jie Yu

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

Ship it!



src/master/master.cpp (line 3191)


Do you also need to add the comment about the ordering in `authorizations` 
is important?


- Jie Yu


On Nov. 30, 2015, 9:20 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Nov. 30, 2015, 9:20 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
>   src/tests/reservation_tests.cpp 15d180f92ec0aea99e6f3a7d0b505c62bd207b61 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-11-30 Thread Greg Mann

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

(Updated Nov. 30, 2015, 9:20 p.m.)


Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.


Changes
---

Addressed comments, refactored validation into master authorization helpers.


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


Repository: mesos


Description
---

Added framework authorization for dynamic reservation.
Note: this review is continued from https://reviews.apache.org/r/37127/


Diffs (updated)
-

  src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
  src/tests/reservation_tests.cpp 15d180f92ec0aea99e6f3a7d0b505c62bd207b61 

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


Testing
---

This is the fifth in a chain of 5 patches. New reservation tests were added to 
`reservation_tests.cpp` to validate the authentication of framework reserve and 
unreserve operations using ACLs. `make check` was run to test after all patches 
were applied.


Thanks,

Greg Mann



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-11-30 Thread Greg Mann

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

(Updated Nov. 30, 2015, 8:48 p.m.)


Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.


Changes
---

Addressed comments, refactored validation into master's authorization helpers.


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


Repository: mesos


Description
---

Added framework authorization for dynamic reservation.
Note: this review is continued from https://reviews.apache.org/r/37127/


Diffs (updated)
-

  src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
  src/tests/reservation_tests.cpp 15d180f92ec0aea99e6f3a7d0b505c62bd207b61 

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


Testing
---

This is the fifth in a chain of 5 patches. New reservation tests were added to 
`reservation_tests.cpp` to validate the authentication of framework reserve and 
unreserve operations using ACLs. `make check` was run to test after all patches 
were applied.


Thanks,

Greg Mann



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-11-19 Thread Jie Yu

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



src/master/master.cpp (lines 3016 - 3028)


Let's move the validation to aurhtorizeReserveResources.

You also need to add a NOTE in `_accept` saying that no need to validate 
the operation again since it's already validated during authorization.



src/master/master.cpp (lines 3053 - 3054)


I don't think the comments here is necessary. We need to avoid 
over-commenting. If the code itself is quite clear about what it's about to do, 
no need for the extra comments here.



src/master/master.cpp (line 3160)


No need for this comment.



src/master/master.cpp (line 3195)


I don't think this comment add more value. The code itself is quite clear 
what it is about to do.



src/master/master.cpp (line 3202)


Ditto.



src/master/master.cpp (line 3206)


Remove this comment.



src/master/master.cpp (line 3237)


Ditto.



src/master/master.cpp (line 3310)


Ditto.



src/tests/reservation_tests.cpp (lines 1657 - 1668)


This might be flaky because master might send out an offer before it 
processes the terminal status update.


- Jie Yu


On Nov. 20, 2015, 12:20 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Nov. 20, 2015, 12:20 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28 
>   src/tests/reservation_tests.cpp ac664ebb49e74aa28551f427ea8f39ac9ce0cfb3 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-11-19 Thread Greg Mann

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



src/master/master.cpp (lines 3278 - 3288)


Whoops, these changes from later on in the chain crept in here. I'll remove 
them.


- Greg Mann


On Nov. 17, 2015, 5:07 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Nov. 17, 2015, 5:07 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28 
>   src/tests/reservation_tests.cpp ac664ebb49e74aa28551f427ea8f39ac9ce0cfb3 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-11-19 Thread Greg Mann

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

(Updated Nov. 20, 2015, 12:20 a.m.)


Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.


Changes
---

Removed unwanted changes.


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


Repository: mesos


Description
---

Added framework authorization for dynamic reservation.
Note: this review is continued from https://reviews.apache.org/r/37127/


Diffs (updated)
-

  src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28 
  src/tests/reservation_tests.cpp ac664ebb49e74aa28551f427ea8f39ac9ce0cfb3 

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


Testing
---

This is the fifth in a chain of 5 patches. New reservation tests were added to 
`reservation_tests.cpp` to validate the authentication of framework reserve and 
unreserve operations using ACLs. `make check` was run to test after all patches 
were applied.


Thanks,

Greg Mann



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-11-19 Thread Greg Mann


> On Nov. 14, 2015, 12:07 a.m., Jie Yu wrote:
> > src/master/master.cpp, lines 3037-3049
> > 
> >
> > Hum, this looks problematic to me. The authorization results are stored 
> > in 'futures'. The ordering in 'futures' is important because we'll read the 
> > authorization results in `_accept` based on the ordering.
> > 
> > However, you call `drop` here and skip the authorization. That means 
> > the ordering invariant is no longer hold.
> > 
> > I would suggest that you perform operation validation in 
> > authorizeReserveResources and returns a Failure if validation fails. In 
> > that way, you can drop the operation in `_accept` if authorization returns 
> > a failure.
> > 
> > Please also add a comment about the fact that ordering in 'futures' is 
> > very important.
> > 
> > Also, you may want to add a test to test the cases where RESERVE and 
> > LAUNCH are in one single `accept` call.
> 
> Greg Mann wrote:
> Thanks Jie, this is indeed a problem. I've implemented a solution which 
> simply pushes a failed `Future` onto `futures` so that `_accept()` can handle 
> the failure. The current diff includes this, as well as a new test that 
> explicitly probes the previous bug.
> 
> I like your idea of putting the validation into 
> `authorizeReserveResources()`, especially since similar validation logic is 
> also found in the HTTP endpoint authorization code. That requires a bit of 
> refactoring, which I'm working on currently. Will post a new patch soon.
> 
> Greg Mann wrote:
> FYI, I'm having trouble with the HTTP endpoint callbacks 
> `Master::Http::reserve()` and `Master::Http::unreserve()`, where I'm trying 
> things like the following to get `Master::authorizeReserveResources()` to 
> pass the full `Future` return value to `_operation()` so that the 
> validation error can be caught there:
> 
> ```cpp
> return master->authorizeUnreserveResources(operation, principal, NULL)
>   .onAny(defer(master->self(),
>   _operation,
>   slaveId,
>   resources,
>   operation,
>   lambda::_1));
> ```
> 
> 
> or
> 
> 
> ```cpp
> lambda::function _reserve =
>   lambda::bind(
>   ::Http::_operation,
>   *this,
>   slaveId,
>   resources.flatten(),
>   operation,
>   lambda::_1);
> 
> return master->authorizeUnreserveResources(operation, principal, NULL)
>   .onAny(defer(master->self(), _reserve))
> ```
> 
> 
> where `_operation` is now accepting the entire offer operation as well as 
> a third parameter of type `Framework*` so that the operation can be dropped 
> if necessary.

But getting errors in the instantiation of `defer()` in the former case, or 
`lambda::bind()` in the latter...


- Greg


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


On Nov. 17, 2015, 5:07 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Nov. 17, 2015, 5:07 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28 
>   src/tests/reservation_tests.cpp ac664ebb49e74aa28551f427ea8f39ac9ce0cfb3 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-11-19 Thread Greg Mann


> On Nov. 14, 2015, 12:07 a.m., Jie Yu wrote:
> > src/master/master.cpp, lines 3037-3049
> > 
> >
> > Hum, this looks problematic to me. The authorization results are stored 
> > in 'futures'. The ordering in 'futures' is important because we'll read the 
> > authorization results in `_accept` based on the ordering.
> > 
> > However, you call `drop` here and skip the authorization. That means 
> > the ordering invariant is no longer hold.
> > 
> > I would suggest that you perform operation validation in 
> > authorizeReserveResources and returns a Failure if validation fails. In 
> > that way, you can drop the operation in `_accept` if authorization returns 
> > a failure.
> > 
> > Please also add a comment about the fact that ordering in 'futures' is 
> > very important.
> > 
> > Also, you may want to add a test to test the cases where RESERVE and 
> > LAUNCH are in one single `accept` call.
> 
> Greg Mann wrote:
> Thanks Jie, this is indeed a problem. I've implemented a solution which 
> simply pushes a failed `Future` onto `futures` so that `_accept()` can handle 
> the failure. The current diff includes this, as well as a new test that 
> explicitly probes the previous bug.
> 
> I like your idea of putting the validation into 
> `authorizeReserveResources()`, especially since similar validation logic is 
> also found in the HTTP endpoint authorization code. That requires a bit of 
> refactoring, which I'm working on currently. Will post a new patch soon.

FYI, I'm having trouble with the HTTP endpoint callbacks 
`Master::Http::reserve()` and `Master::Http::unreserve()`, where I'm trying 
things like the following to get `Master::authorizeReserveResources()` to pass 
the full `Future` return value to `_operation()` so that the validation 
error can be caught there:

```cpp
return master->authorizeUnreserveResources(operation, principal, NULL)
  .onAny(defer(master->self(),
  _operation,
  slaveId,
  resources,
  operation,
  lambda::_1));
```


or


```cpp
lambda::function _reserve =
  lambda::bind(
  ::Http::_operation,
  *this,
  slaveId,
  resources.flatten(),
  operation,
  lambda::_1);

return master->authorizeUnreserveResources(operation, principal, NULL)
  .onAny(defer(master->self(), _reserve))
```


where `_operation` is now accepting the entire offer operation as well as a 
third parameter of type `Framework*` so that the operation can be dropped if 
necessary.


- Greg


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


On Nov. 17, 2015, 5:07 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Nov. 17, 2015, 5:07 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28 
>   src/tests/reservation_tests.cpp ac664ebb49e74aa28551f427ea8f39ac9ce0cfb3 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-11-19 Thread Greg Mann


> On Nov. 14, 2015, 12:07 a.m., Jie Yu wrote:
> > src/master/master.cpp, lines 3037-3049
> > 
> >
> > Hum, this looks problematic to me. The authorization results are stored 
> > in 'futures'. The ordering in 'futures' is important because we'll read the 
> > authorization results in `_accept` based on the ordering.
> > 
> > However, you call `drop` here and skip the authorization. That means 
> > the ordering invariant is no longer hold.
> > 
> > I would suggest that you perform operation validation in 
> > authorizeReserveResources and returns a Failure if validation fails. In 
> > that way, you can drop the operation in `_accept` if authorization returns 
> > a failure.
> > 
> > Please also add a comment about the fact that ordering in 'futures' is 
> > very important.
> > 
> > Also, you may want to add a test to test the cases where RESERVE and 
> > LAUNCH are in one single `accept` call.
> 
> Greg Mann wrote:
> Thanks Jie, this is indeed a problem. I've implemented a solution which 
> simply pushes a failed `Future` onto `futures` so that `_accept()` can handle 
> the failure. The current diff includes this, as well as a new test that 
> explicitly probes the previous bug.
> 
> I like your idea of putting the validation into 
> `authorizeReserveResources()`, especially since similar validation logic is 
> also found in the HTTP endpoint authorization code. That requires a bit of 
> refactoring, which I'm working on currently. Will post a new patch soon.
> 
> Greg Mann wrote:
> FYI, I'm having trouble with the HTTP endpoint callbacks 
> `Master::Http::reserve()` and `Master::Http::unreserve()`, where I'm trying 
> things like the following to get `Master::authorizeReserveResources()` to 
> pass the full `Future` return value to `_operation()` so that the 
> validation error can be caught there:
> 
> ```cpp
> return master->authorizeUnreserveResources(operation, principal, NULL)
>   .onAny(defer(master->self(),
>   _operation,
>   slaveId,
>   resources,
>   operation,
>   lambda::_1));
> ```
> 
> 
> or
> 
> 
> ```cpp
> lambda::function _reserve =
>   lambda::bind(
>   ::Http::_operation,
>   *this,
>   slaveId,
>   resources.flatten(),
>   operation,
>   lambda::_1);
> 
> return master->authorizeUnreserveResources(operation, principal, NULL)
>   .onAny(defer(master->self(), _reserve))
> ```
> 
> 
> where `_operation` is now accepting the entire offer operation as well as 
> a third parameter of type `Framework*` so that the operation can be dropped 
> if necessary.
> 
> Greg Mann wrote:
> But getting errors in the instantiation of `defer()` in the former case, 
> or `lambda::bind()` in the latter...

Thanks Jie! It looks like `.onAny()` only accepts functions with a `void` 
return type:

```
typedef lambda::function AnyCallback;

...

const Future& onAny(AnyCallback&& callback) const;
```


- Greg


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


On Nov. 17, 2015, 5:07 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Nov. 17, 2015, 5:07 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28 
>   src/tests/reservation_tests.cpp ac664ebb49e74aa28551f427ea8f39ac9ce0cfb3 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-11-17 Thread Greg Mann

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

(Updated Nov. 17, 2015, 5:07 p.m.)


Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.


Changes
---

Fixed authorization ordering, added new test.


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


Repository: mesos


Description
---

Added framework authorization for dynamic reservation.
Note: this review is continued from https://reviews.apache.org/r/37127/


Diffs (updated)
-

  src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28 
  src/tests/reservation_tests.cpp ac664ebb49e74aa28551f427ea8f39ac9ce0cfb3 

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


Testing
---

This is the fifth in a chain of 5 patches. New reservation tests were added to 
`reservation_tests.cpp` to validate the authentication of framework reserve and 
unreserve operations using ACLs. `make check` was run to test after all patches 
were applied.


Thanks,

Greg Mann



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-11-17 Thread Greg Mann


> On Nov. 14, 2015, 12:07 a.m., Jie Yu wrote:
> > src/master/master.cpp, lines 3037-3049
> > 
> >
> > Hum, this looks problematic to me. The authorization results are stored 
> > in 'futures'. The ordering in 'futures' is important because we'll read the 
> > authorization results in `_accept` based on the ordering.
> > 
> > However, you call `drop` here and skip the authorization. That means 
> > the ordering invariant is no longer hold.
> > 
> > I would suggest that you perform operation validation in 
> > authorizeReserveResources and returns a Failure if validation fails. In 
> > that way, you can drop the operation in `_accept` if authorization returns 
> > a failure.
> > 
> > Please also add a comment about the fact that ordering in 'futures' is 
> > very important.
> > 
> > Also, you may want to add a test to test the cases where RESERVE and 
> > LAUNCH are in one single `accept` call.

Thanks Jie, this is indeed a problem. I've implemented a solution which simply 
pushes a failed `Future` onto `futures` so that `_accept()` can handle the 
failure. The current diff includes this, as well as a new test that explicitly 
probes the previous bug.

I like your idea of putting the validation into `authorizeReserveResources()`, 
especially since similar validation logic is also found in the HTTP endpoint 
authorization code. That requires a bit of refactoring, which I'm working on 
currently. Will post a new patch soon.


- Greg


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


On Nov. 17, 2015, 5:07 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Nov. 17, 2015, 5:07 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28 
>   src/tests/reservation_tests.cpp ac664ebb49e74aa28551f427ea8f39ac9ce0cfb3 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-11-13 Thread Greg Mann

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

(Updated Nov. 13, 2015, 11:17 p.m.)


Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.


Changes
---

Updated ACL names.


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


Repository: mesos


Description
---

Added framework authorization for dynamic reservation.
Note: this review is continued from https://reviews.apache.org/r/37127/


Diffs (updated)
-

  src/master/master.cpp 7bac0fea4bcd040307fdfdcd002387d5baee46d1 
  src/tests/reservation_tests.cpp ac664ebb49e74aa28551f427ea8f39ac9ce0cfb3 

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


Testing
---

This is the fifth in a chain of 5 patches. New reservation tests were added to 
`reservation_tests.cpp` to validate the authentication of framework reserve and 
unreserve operations using ACLs. `make check` was run to test after all patches 
were applied.


Thanks,

Greg Mann



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-11-13 Thread Greg Mann

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

(Updated Nov. 13, 2015, 9:15 p.m.)


Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.


Changes
---

Updated 'ReservationTest.BadACLDropUnreserve' to test multiple offer operations 
in one message.


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


Repository: mesos


Description
---

Added framework authorization for dynamic reservation.
Note: this review is continued from https://reviews.apache.org/r/37127/


Diffs (updated)
-

  src/master/master.cpp 7bac0fea4bcd040307fdfdcd002387d5baee46d1 
  src/tests/reservation_tests.cpp ac664ebb49e74aa28551f427ea8f39ac9ce0cfb3 

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


Testing
---

This is the fifth in a chain of 5 patches. New reservation tests were added to 
`reservation_tests.cpp` to validate the authentication of framework reserve and 
unreserve operations using ACLs. `make check` was run to test after all patches 
were applied.


Thanks,

Greg Mann



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-11-13 Thread Jie Yu

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



src/master/master.cpp (lines 3028 - 3040)


Hum, this looks problematic to me. The authorization results are stored in 
'futures'. The ordering in 'futures' is important because we'll read the 
authorization results in `_accept` based on the ordering.

However, you call `drop` here and skip the authorization. That means the 
ordering invariant is no longer hold.

I would suggest that you perform operation validation in 
authorizeReserveResources and returns a Failure if validation fails. In that 
way, you can drop the operation in `_accept` if authorization returns a failure.

Please also add a comment about the fact that ordering in 'futures' is very 
important.

Also, you may want to add a test to test the cases where RESERVE and LAUNCH 
are in one single `accept` call.


- Jie Yu


On Nov. 13, 2015, 11:17 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Nov. 13, 2015, 11:17 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 7bac0fea4bcd040307fdfdcd002387d5baee46d1 
>   src/tests/reservation_tests.cpp ac664ebb49e74aa28551f427ea8f39ac9ce0cfb3 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-11-12 Thread Greg Mann

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

(Updated Nov. 13, 2015, 4:32 a.m.)


Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.


Changes
---

Added comments.


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


Repository: mesos


Description
---

Added framework authorization for dynamic reservation.
Note: this review is continued from https://reviews.apache.org/r/37127/


Diffs (updated)
-

  src/master/master.cpp 7bac0fea4bcd040307fdfdcd002387d5baee46d1 
  src/tests/reservation_tests.cpp ac664ebb49e74aa28551f427ea8f39ac9ce0cfb3 

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


Testing
---

This is the fifth in a chain of 5 patches. New reservation tests were added to 
`reservation_tests.cpp` to validate the authentication of framework reserve and 
unreserve operations using ACLs. `make check` was run to test after all patches 
were applied.


Thanks,

Greg Mann



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-11-09 Thread Mesos ReviewBot

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


Patch looks great!

Reviews applied: [39985, 39986, 39987, 39988, 39989]

All tests passed.

- Mesos ReviewBot


On Nov. 9, 2015, 5:26 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Nov. 9, 2015, 5:26 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 7bac0fea4bcd040307fdfdcd002387d5baee46d1 
>   src/tests/reservation_tests.cpp 4bc2b313572a8cd76fa798ac745c319a7c11df0e 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-11-09 Thread Greg Mann

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

(Updated Nov. 9, 2015, 5:26 p.m.)


Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added framework authorization for dynamic reservation.
Note: this review is continued from https://reviews.apache.org/r/37127/


Diffs (updated)
-

  src/master/master.cpp 7bac0fea4bcd040307fdfdcd002387d5baee46d1 
  src/tests/reservation_tests.cpp 4bc2b313572a8cd76fa798ac745c319a7c11df0e 

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


Testing
---

This is the fifth in a chain of 5 patches. New reservation tests were added to 
`reservation_tests.cpp` to validate the authentication of framework reserve and 
unreserve operations using ACLs. `make check` was run to test after all patches 
were applied.


Thanks,

Greg Mann



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-11-08 Thread Greg Mann

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

(Updated Nov. 9, 2015, 6:13 a.m.)


Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

Added framework authorization for dynamic reservation.
Note: this review is continued from https://reviews.apache.org/r/37127/


Diffs (updated)
-

  src/master/master.cpp 25b94c8b41f65599327e0a0459ba86c6078895a8 
  src/tests/reservation_tests.cpp 6b7c43c8b5c64618249dbee926383242320c111e 

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


Testing
---

This is the fifth in a chain of 5 patches. New reservation tests were added to 
`reservation_tests.cpp` to validate the authentication of framework reserve and 
unreserve operations using ACLs. `make check` was run to test after all patches 
were applied.


Thanks,

Greg Mann



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-11-06 Thread Greg Mann


> On Nov. 6, 2015, 9:08 a.m., Guangya Liu wrote:
> > src/master/master.cpp, line 2986
> > 
> >
> > Why not "const TaskInfo& task"?

Good catch!


- Greg


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


On Nov. 5, 2015, 9:53 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Nov. 5, 2015, 9:53 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp cbae27e7a4059a72bc69e152ec8adaf4ef725965 
>   src/tests/reservation_tests.cpp 6b7c43c8b5c64618249dbee926383242320c111e 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-11-06 Thread Greg Mann

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

(Updated Nov. 6, 2015, 11:58 p.m.)


Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.


Changes
---

Addressed style comment.


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


Repository: mesos


Description
---

Added framework authorization for dynamic reservation.
Note: this review is continued from https://reviews.apache.org/r/37127/


Diffs (updated)
-

  src/master/master.cpp cbae27e7a4059a72bc69e152ec8adaf4ef725965 
  src/tests/reservation_tests.cpp 6b7c43c8b5c64618249dbee926383242320c111e 

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


Testing
---

This is the fifth in a chain of 5 patches. New reservation tests were added to 
`reservation_tests.cpp` to validate the authentication of framework reserve and 
unreserve operations using ACLs. `make check` was run to test after all patches 
were applied.


Thanks,

Greg Mann



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-11-06 Thread Neil Conway

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


Also need to update the docs.


src/tests/reservation_tests.cpp (line 1323)


Grammar.



src/tests/reservation_tests.cpp (line 1406)


Grammar.


- Neil Conway


On Nov. 6, 2015, 11:58 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Nov. 6, 2015, 11:58 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp cbae27e7a4059a72bc69e152ec8adaf4ef725965 
>   src/tests/reservation_tests.cpp 6b7c43c8b5c64618249dbee926383242320c111e 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-11-06 Thread Mesos ReviewBot

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


Patch looks great!

Reviews applied: [39985, 39986, 39987, 39988, 39989]

All tests passed.

- Mesos ReviewBot


On Nov. 6, 2015, 11:58 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Nov. 6, 2015, 11:58 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp cbae27e7a4059a72bc69e152ec8adaf4ef725965 
>   src/tests/reservation_tests.cpp 6b7c43c8b5c64618249dbee926383242320c111e 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-11-06 Thread Guangya Liu

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



src/master/master.cpp (line 2977)


Why not "const TaskInfo& task"?


- Guangya Liu


On 十一月 5, 2015, 9:53 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated 十一月 5, 2015, 9:53 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp cbae27e7a4059a72bc69e152ec8adaf4ef725965 
>   src/tests/reservation_tests.cpp 6b7c43c8b5c64618249dbee926383242320c111e 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-11-05 Thread Greg Mann

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

(Updated Nov. 5, 2015, 9:53 p.m.)


Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.


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


Repository: mesos


Description (updated)
---

Added framework authorization for dynamic reservation.
Note: this review is continued from https://reviews.apache.org/r/37127/


Diffs
-

  src/master/master.cpp cbae27e7a4059a72bc69e152ec8adaf4ef725965 
  src/tests/reservation_tests.cpp 6b7c43c8b5c64618249dbee926383242320c111e 

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


Testing
---

This is the fifth in a chain of 5 patches. New reservation tests were added to 
`reservation_tests.cpp` to validate the authentication of framework reserve and 
unreserve operations using ACLs. `make check` was run to test after all patches 
were applied.


Thanks,

Greg Mann



Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

2015-11-05 Thread Mesos ReviewBot

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


Patch looks great!

Reviews applied: [39985, 39986, 39987, 39988, 39989]

All tests passed.

- Mesos ReviewBot


On Nov. 5, 2015, 9:53 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Nov. 5, 2015, 9:53 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp cbae27e7a4059a72bc69e152ec8adaf4ef725965 
>   src/tests/reservation_tests.cpp 6b7c43c8b5c64618249dbee926383242320c111e 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>