Re: Review Request 57535: Applied RegisterAgent ACL to the master.

2017-04-27 Thread Anindya Sinha 

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


Ship it!




Ship It!

- Anindya Sinha


On April 25, 2017, 5:30 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57535/
> ---
> 
> (Updated April 25, 2017, 5:30 p.m.)
> 
> 
> Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, Benjamin 
> Mahler, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7097
> https://issues.apache.org/jira/browse/MESOS-7097
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Applied RegisterAgent ACL to the master.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
>   src/master/master.cpp d1cdc35a066e190ef8e0bd788e07e63b92f7fce7 
>   src/tests/master_authorization_tests.cpp 
> 1a0285a3f345ef21a8256d4123d8bb684f538da4 
> 
> 
> Diff: https://reviews.apache.org/r/57535/diff/8/
> 
> 
> Testing
> ---
> 
> make check.
> 
> The tests added here cover some basic scenarios, I have more tests but will 
> add them when MESOS-7244 is fixed.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 57535: Applied RegisterAgent ACL to the master.

2017-04-25 Thread Jiang Yan Xu 

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

(Updated April 25, 2017, 10:30 a.m.)


Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, Benjamin 
Mahler, Greg Mann, and Vinod Kone.


Changes
---

Minor fix.


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


Repository: mesos


Description
---

Applied RegisterAgent ACL to the master.


Diffs (updated)
-

  src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
  src/master/master.cpp d1cdc35a066e190ef8e0bd788e07e63b92f7fce7 
  src/tests/master_authorization_tests.cpp 
1a0285a3f345ef21a8256d4123d8bb684f538da4 


Diff: https://reviews.apache.org/r/57535/diff/8/

Changes: https://reviews.apache.org/r/57535/diff/7-8/


Testing
---

make check.

The tests added here cover some basic scenarios, I have more tests but will add 
them when MESOS-7244 is fixed.


Thanks,

Jiang Yan Xu

Re: Review Request 57535: Applied RegisterAgent ACL to the master.

2017-04-20 Thread Jiang Yan Xu 

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

(Updated April 20, 2017, 9:53 p.m.)


Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, Benjamin 
Mahler, Greg Mann, and Vinod Kone.


Changes
---

Rebase. Please review.


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


Repository: mesos


Description
---

Applied RegisterAgent ACL to the master.


Diffs (updated)
-

  src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
  src/master/master.cpp d1cdc35a066e190ef8e0bd788e07e63b92f7fce7 
  src/tests/master_authorization_tests.cpp 
1a0285a3f345ef21a8256d4123d8bb684f538da4 


Diff: https://reviews.apache.org/r/57535/diff/7/

Changes: https://reviews.apache.org/r/57535/diff/6-7/


Testing
---

make check.

The tests added here cover some basic scenarios, I have more tests but will add 
them when MESOS-7244 is fixed.


Thanks,

Jiang Yan Xu

Re: Review Request 57535: Applied RegisterAgent ACL to the master.

2017-04-04 Thread Greg Mann 


> On March 17, 2017, 9:32 p.m., Greg Mann wrote:
> > src/master/master.hpp
> > Lines 684-686 (patched)
> > 
> >
> > Could you leave a TODO here to update this function to use `Principal` 
> > when MESOS-7202 is resolved?
> 
> Jiang Yan Xu wrote:
> Will MESOS-7202 cover the master <-> agent protocol and the 
> `authenticated`?

Hmm perhaps not, since the master <-> agent communication will still be 
authenticated using the PID-based authenticator interface.


- Greg


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


On March 28, 2017, 8:40 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57535/
> ---
> 
> (Updated March 28, 2017, 8:40 a.m.)
> 
> 
> Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, Benjamin 
> Mahler, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7097
> https://issues.apache.org/jira/browse/MESOS-7097
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Applied RegisterAgent ACL to the master.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 
>   src/master/master.cpp 43e9d26167c1f405329ea05224c22e7b8c65315f 
>   src/tests/master_authorization_tests.cpp 
> 1a0285a3f345ef21a8256d4123d8bb684f538da4 
> 
> 
> Diff: https://reviews.apache.org/r/57535/diff/6/
> 
> 
> Testing
> ---
> 
> make check.
> 
> The tests added here cover some basic scenarios, I have more tests but will 
> add them when MESOS-7244 is fixed.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 57535: Applied RegisterAgent ACL to the master.

2017-03-28 Thread Jiang Yan Xu 


> On March 24, 2017, 2:58 p.m., Benjamin Mahler wrote:
> > Nice tests! Overall approach looks good to me. A few comments below.
> > 
> > Unrelated to your changes I noticed a few issues:
> > 
> > There are some inconsistencies between the framework and agent paths. For 
> > example, we don't log when we receive an agent's (re-)registration message 
> > but we log the framework's subscription, not sure why we did that. Also, 
> > since we don't track a framework's pending subscription, if the 
> > authorization futures are re-ordered we could process subscre 2 before 
> > subscre 1, but this is unrelated to your change here.
> > 
> > The "queueing up" logic (example 
> > [here](https://github.com/apache/mesos/blob/1bb7ed28977d9b03c6a9162e8d8d10c7420a47f9/src/master/master.cpp#L5344-L5345))
> >  also allows re-ordering.

Thanks!

- The logging is easy to fix. We can add such logging for the agent 
registration as well.
- Pending framework registration re-ordering: It looks to me we are OK for now 
because we only have one hop in the async flow (subscribe -> authorizer -> 
_subscribe) and do we 
[this](https://github.com/apache/mesos/blob/1bb7ed28977d9b03c6a9162e8d8d10c7420a47f9/src/master/master.cpp#L2902-L2911)
 in the 2nd step. If we later add frameworks to the registrar we would need 
more hops and need to do something similar to what this patch does.
- Can authorization futures be re-ordered? At least with the local authorizer 
they are executed by the same authorizer actor that satifies the futures. I was 
thinking more about other events like ExitedEvent being enqueued between 
duplicate subscribes and registrations.
- The "queueing up" logic: I don't see how `registerSlave` retries can 
themsevles get re-ordered. Perhaps two `Master::registerSlave` dispatches 
enqueued by the authenticator can have an UnregisterSlaveMessage go between 
then due to the async step but `registering` and `reregistering` seem to be 
able to handle it.


> On March 24, 2017, 2:58 p.m., Benjamin Mahler wrote:
> > src/master/master.hpp
> > Line 1640 (original), 1665-1668 (patched)
> > 
> >
> > It looks like the comment about not answering questions about these 
> > transitioning agents was removed, can we restore it?

Given the recent changes in /r/52083/, Perhaps it's best to put the comment 
above `recovered`?

```
// Slaves that have been recovered from the registrar after master
// failover. Slaves are removed from this collection when they
// either re-register with the master or are marked unreachable
// because they do not re-register before `recoveredTimer` fires.
// We must not answer questions related to these slaves (e.g.,
// during task reconciliation) until we determine their fate.
hashmap recovered;
```


- Jiang Yan


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


On March 28, 2017, 1:40 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57535/
> ---
> 
> (Updated March 28, 2017, 1:40 a.m.)
> 
> 
> Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, Benjamin 
> Mahler, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7097
> https://issues.apache.org/jira/browse/MESOS-7097
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Applied RegisterAgent ACL to the master.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 
>   src/master/master.cpp 43e9d26167c1f405329ea05224c22e7b8c65315f 
>   src/tests/master_authorization_tests.cpp 
> 1a0285a3f345ef21a8256d4123d8bb684f538da4 
> 
> 
> Diff: https://reviews.apache.org/r/57535/diff/6/
> 
> 
> Testing
> ---
> 
> make check.
> 
> The tests added here cover some basic scenarios, I have more tests but will 
> add them when MESOS-7244 is fixed.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 57535: Applied RegisterAgent ACL to the master.

2017-03-28 Thread Jiang Yan Xu 

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

(Updated March 28, 2017, 1:40 a.m.)


Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, Benjamin 
Mahler, Greg Mann, and Vinod Kone.


Changes
---

Comments.


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


Repository: mesos


Description
---

Applied RegisterAgent ACL to the master.


Diffs (updated)
-

  src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 
  src/master/master.cpp 43e9d26167c1f405329ea05224c22e7b8c65315f 
  src/tests/master_authorization_tests.cpp 
1a0285a3f345ef21a8256d4123d8bb684f538da4 


Diff: https://reviews.apache.org/r/57535/diff/6/

Changes: https://reviews.apache.org/r/57535/diff/5-6/


Testing
---

make check.

The tests added here cover some basic scenarios, I have more tests but will add 
them when MESOS-7244 is fixed.


Thanks,

Jiang Yan Xu

Re: Review Request 57535: Applied RegisterAgent ACL to the master.

2017-03-24 Thread Benjamin Mahler 

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



Nice tests! Overall approach looks good to me. A few comments below.

Unrelated to your changes I noticed a few issues:

There are some inconsistencies between the framework and agent paths. For 
example, we don't log when we receive an agent's (re-)registration message but 
we log the framework's subscription, not sure why we did that. Also, since we 
don't track a framework's pending subscription, if the authorization futures 
are re-ordered we could process subscre 2 before subscre 1, but this is 
unrelated to your change here.

The "queueing up" logic (example 
[here](https://github.com/apache/mesos/blob/1bb7ed28977d9b03c6a9162e8d8d10c7420a47f9/src/master/master.cpp#L5344-L5345))
 also allows re-ordering.


src/master/master.hpp
Line 1640 (original), 1665-1668 (patched)


It looks like the comment about not answering questions about these 
transitioning agents was removed, can we restore it?



src/master/master.cpp
Lines 3642-3656 (patched)


Any reason there's no logging here? It would be nice to log consistently 
with the framework authorization path:


https://github.com/apache/mesos/blob/1bb7ed28977d9b03c6a9162e8d8d10c7420a47f9/src/master/master.cpp#L2201-L2203



src/master/master.cpp
Lines 3654 (patched)


Maybe a little comment about why we don't have a request object here?


- Benjamin Mahler


On March 22, 2017, 12:48 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57535/
> ---
> 
> (Updated March 22, 2017, 12:48 a.m.)
> 
> 
> Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, Benjamin 
> Mahler, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7097
> https://issues.apache.org/jira/browse/MESOS-7097
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Applied RegisterAgent ACL to the master.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 
>   src/master/master.cpp 43e9d26167c1f405329ea05224c22e7b8c65315f 
>   src/tests/master_authorization_tests.cpp 
> 1a0285a3f345ef21a8256d4123d8bb684f538da4 
> 
> 
> Diff: https://reviews.apache.org/r/57535/diff/5/
> 
> 
> Testing
> ---
> 
> make check.
> 
> The tests added here cover some basic scenarios, I have more tests but will 
> add them when MESOS-7244 is fixed.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 57535: Applied RegisterAgent ACL to the master.

2017-03-20 Thread Jiang Yan Xu 


> On March 17, 2017, 2:32 p.m., Greg Mann wrote:
> > src/master/master.hpp
> > Lines 684-686 (patched)
> > 
> >
> > Could you leave a TODO here to update this function to use `Principal` 
> > when MESOS-7202 is resolved?

Will MESOS-7202 cover the master <-> agent protocol and the `authenticated`?


> On March 17, 2017, 2:32 p.m., Greg Mann wrote:
> > src/master/master.hpp
> > Lines 686 (patched)
> > 
> >
> > Should we do `authorizeAgent` since this is an entirely new function? 
> > I'm not sure if we're trying to transition as we add new code; I only see 
> > one example in the agent code, `getAgent()` in 'slave/http.cpp'.

`getAgents` directly corresponds to the new API `GET_AGENTS` so it makese more 
sense to use the new terminology. AFAIK in other cases we maintain file level 
consistency and defer to a later sweep.


> On March 17, 2017, 2:32 p.m., Greg Mann wrote:
> > src/master/master.cpp
> > Lines 5437-5442 (patched)
> > 
> >
> > I'm not confident in verifying this myself without some more effort; 
> > perhaps Jie or Neil or somebody else familiar with the agent registration 
> > code path in the master could verify that this is the case?
> > 
> > I could have another look next week to dig in here a bit.

To add to the comment: `authenticated` supports an optional step (i.e., 
authentication) in the (re-)registration path. If no authentication is done, 
the pid wouldn't be in `authenticated` but the whole process still works by 
design. Therefore `authenticated` is checked in the beginning and never checked 
again after this step. So the robustness of agent (re-)registration doesn't 
care if `pid` is removed from `authenticated` (or not in it in the first 
place). In other words, `pid` being removed from `authenticated` would suggest 
disconnection but this is not the place to handle it.


> On March 17, 2017, 2:32 p.m., Greg Mann wrote:
> > src/tests/master_authorization_tests.cpp
> > Lines 2357-2362 (patched)
> > 
> >
> > Are we sure that this represents a reregistration, rather than a 
> > registration of a new agent? Perhaps you could do `EXPECT_CALL` on 
> > something in the agent's reregistration code path to verify?

Good catch. I created a `slaveFlags` above but forgot to use it. I should've 
used `FUTURE_MESSAGE(Eq(SlaveReregisteredMessage().GetTypeName()), _, _);` and 
that should confirm it.


- Jiang Yan


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


On March 14, 2017, 6:09 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57535/
> ---
> 
> (Updated March 14, 2017, 6:09 p.m.)
> 
> 
> Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, Greg Mann, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-7097
> https://issues.apache.org/jira/browse/MESOS-7097
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Applied RegisterAgent ACL to the master.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 
>   src/master/master.cpp d43350d08ddd14fb7ba2a79c899abda6a864038c 
>   src/tests/master_authorization_tests.cpp 
> 1a0285a3f345ef21a8256d4123d8bb684f538da4 
> 
> 
> Diff: https://reviews.apache.org/r/57535/diff/3/
> 
> 
> Testing
> ---
> 
> make check.
> 
> The tests added here cover some basic scenarios, I have more tests but will 
> add them when MESOS-7244 is fixed.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 57535: Applied RegisterAgent ACL to the master.

2017-03-20 Thread Jiang Yan Xu 

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

(Updated March 19, 2017, 11:18 p.m.)


Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, Greg Mann, 
and Vinod Kone.


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


Repository: mesos


Description
---

Applied RegisterAgent ACL to the master.


Diffs (updated)
-

  src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 
  src/master/master.cpp 43e9d26167c1f405329ea05224c22e7b8c65315f 
  src/tests/master_authorization_tests.cpp 
1a0285a3f345ef21a8256d4123d8bb684f538da4 


Diff: https://reviews.apache.org/r/57535/diff/4/

Changes: https://reviews.apache.org/r/57535/diff/3-4/


Testing
---

make check.

The tests added here cover some basic scenarios, I have more tests but will add 
them when MESOS-7244 is fixed.


Thanks,

Jiang Yan Xu

Re: Review Request 57535: Applied RegisterAgent ACL to the master.

2017-03-17 Thread Greg Mann 

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




src/master/master.hpp
Lines 684-686 (patched)


Could you leave a TODO here to update this function to use `Principal` when 
MESOS-7202 is resolved?



src/master/master.hpp
Lines 684-685 (patched)


Could you tweak this comment a bit? "Returns whether the principal is 
authorized to (re-)register an agent." seems a bit more accurate to me?



src/master/master.hpp
Lines 686 (patched)


Should we do `authorizeAgent` since this is an entirely new function? I'm 
not sure if we're trying to transition as we add new code; I only see one 
example in the agent code, `getAgent()` in 'slave/http.cpp'.



src/master/master.cpp
Lines 3642 (patched)


Ditto



src/master/master.cpp
Lines 5419-5420 (patched)


Fits on one line.



src/master/master.cpp
Lines 5423-5424 (patched)


s/to to/to/

Also, could you make this logging a bit more explicit for the NONE 
principal case? Just to be clear that it's not a principal called 'none', might 
be better if it printed "Not authorized to register as agent without a 
principal" or something similar.



src/master/master.cpp
Lines 5437-5442 (patched)


I'm not confident in verifying this myself without some more effort; 
perhaps Jie or Neil or somebody else familiar with the agent registration code 
path in the master could verify that this is the case?

I could have another look next week to dig in here a bit.



src/master/master.cpp
Lines 5674-5675 (patched)


Fits on one line.



src/master/master.cpp
Lines 5678-5679 (patched)


s/to to/to/

Also, same comment regarding the NONE case.



src/tests/master_authorization_tests.cpp
Lines 2354 (patched)


s/it is/verify that it is/



src/tests/master_authorization_tests.cpp
Lines 2357-2362 (patched)


Are we sure that this represents a reregistration, rather than a 
registration of a new agent? Perhaps you could do `EXPECT_CALL` on something in 
the agent's reregistration code path to verify?


- Greg Mann


On March 15, 2017, 1:09 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57535/
> ---
> 
> (Updated March 15, 2017, 1:09 a.m.)
> 
> 
> Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, Greg Mann, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-7097
> https://issues.apache.org/jira/browse/MESOS-7097
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Applied RegisterAgent ACL to the master.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 
>   src/master/master.cpp d43350d08ddd14fb7ba2a79c899abda6a864038c 
>   src/tests/master_authorization_tests.cpp 
> 1a0285a3f345ef21a8256d4123d8bb684f538da4 
> 
> 
> Diff: https://reviews.apache.org/r/57535/diff/3/
> 
> 
> Testing
> ---
> 
> make check.
> 
> The tests added here cover some basic scenarios, I have more tests but will 
> add them when MESOS-7244 is fixed.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 57535: Applied RegisterAgent ACL to the master.

2017-03-15 Thread Anindya Sinha 

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


Ship it!




Ship It!

- Anindya Sinha


On March 15, 2017, 1:09 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57535/
> ---
> 
> (Updated March 15, 2017, 1:09 a.m.)
> 
> 
> Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, Greg Mann, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-7097
> https://issues.apache.org/jira/browse/MESOS-7097
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Applied RegisterAgent ACL to the master.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 
>   src/master/master.cpp d43350d08ddd14fb7ba2a79c899abda6a864038c 
>   src/tests/master_authorization_tests.cpp 
> 1a0285a3f345ef21a8256d4123d8bb684f538da4 
> 
> 
> Diff: https://reviews.apache.org/r/57535/diff/3/
> 
> 
> Testing
> ---
> 
> make check.
> 
> The tests added here cover some basic scenarios, I have more tests but will 
> add them when MESOS-7244 is fixed.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 57535: Applied RegisterAgent ACL to the master.

2017-03-14 Thread Jiang Yan Xu 

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

(Updated March 14, 2017, 6:09 p.m.)


Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, Greg Mann, 
and Vinod Kone.


Changes
---

Comments.


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


Repository: mesos


Description
---

Applied RegisterAgent ACL to the master.


Diffs (updated)
-

  src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 
  src/master/master.cpp d43350d08ddd14fb7ba2a79c899abda6a864038c 
  src/tests/master_authorization_tests.cpp 
1a0285a3f345ef21a8256d4123d8bb684f538da4 


Diff: https://reviews.apache.org/r/57535/diff/3/

Changes: https://reviews.apache.org/r/57535/diff/2-3/


Testing
---

make check.

The tests added here cover some basic scenarios, I have more tests but will add 
them when MESOS-7244 is fixed.


Thanks,

Jiang Yan Xu

Re: Review Request 57535: Applied RegisterAgent ACL to the master.

2017-03-14 Thread Jiang Yan Xu 


> On March 14, 2017, 5:08 p.m., Anindya Sinha wrote:
> > src/master/master.hpp
> > Lines 686 (patched)
> > 
> >
> > nit: s/authorizeSlave/authorizeAgent?

This is for the sake of consistency.

The master already/still has:

```
Master::addSlave
Master::removeSlave
...
```

I think we are maintaining consistency within old files and will later do a 
sweeping change. This is of course sometimes less straightfoward: e.g., you 
have an internal method that directly corresponds to a new API which already 
uses the new terminology, etc.


> On March 14, 2017, 5:08 p.m., Anindya Sinha wrote:
> > src/master/master.cpp
> > Line 5372 (original), 5446 (patched)
> > 
> >
> > nit: Is there a specific reason for the change here (and in 
> > reregisterSlave) from `from` to `pid`?

I am maintaining the pattern in the current `_registerSlave` and 
`_reregisterSlave`: 
https://github.com/apache/mesos/blob/5abaeec1ca53642bad329fce89fb3a03fbebc079/src/master/master.cpp#L5454

The way to look at it is:

`registerSlave` is a message handler: the `from` here is injected by libprocess.

In internal methods like `_registerSlave`, the meaning is changed: with

```
  void _registerSlave(
  const SlaveInfo& slaveInfo,
  const process::UPID& pid,
  const Option& principal,
  const std::vector& checkpointedResources,
  const std::string& version,
  const std::vector& agentCapabilities,
  const process::Future& authorized);
```

I am saying this method takes a `slaveInfo`, its associated pid is `pid`, its 
principal is `principal`, ...

So `from` is not as needed / as clear anymore.


- Jiang Yan


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


On March 14, 2017, 11:27 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57535/
> ---
> 
> (Updated March 14, 2017, 11:27 a.m.)
> 
> 
> Review request for mesos, Adam B, Anindya Sinha, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7097
> https://issues.apache.org/jira/browse/MESOS-7097
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Applied RegisterAgent ACL to the master.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> e75e1879435f1c2bce47a86e9feebf9d051e969b 
>   src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 
>   src/master/master.cpp d43350d08ddd14fb7ba2a79c899abda6a864038c 
>   src/tests/master_authorization_tests.cpp 
> 1a0285a3f345ef21a8256d4123d8bb684f538da4 
> 
> 
> Diff: https://reviews.apache.org/r/57535/diff/2/
> 
> 
> Testing
> ---
> 
> make check.
> 
> The tests added here cover some basic scenarios, I have more tests but will 
> add them when MESOS-7244 is fixed.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 57535: Applied RegisterAgent ACL to the master.

2017-03-14 Thread Anindya Sinha 

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




include/mesos/authorizer/acls.proto
Line 459 (original), 459 (patched)


Oh, I see this is fixed here. I think it is better to move this change to 
https://reviews.apache.org/r/57534



src/master/master.hpp
Lines 684 (patched)


s/wehther/whether.



src/master/master.hpp
Lines 686 (patched)


nit: s/authorizeSlave/authorizeAgent?



src/master/master.cpp
Lines 3642 (patched)


Similar comment as in header file. s/authorizeSlave/authorizeAgent?



src/master/master.cpp
Line 5372 (original), 5446 (patched)


nit: Is there a specific reason for the change here (and in 
reregisterSlave) from `from` to `pid`?


- Anindya Sinha


On March 14, 2017, 6:27 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57535/
> ---
> 
> (Updated March 14, 2017, 6:27 p.m.)
> 
> 
> Review request for mesos, Adam B, Anindya Sinha, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7097
> https://issues.apache.org/jira/browse/MESOS-7097
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Applied RegisterAgent ACL to the master.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> e75e1879435f1c2bce47a86e9feebf9d051e969b 
>   src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 
>   src/master/master.cpp d43350d08ddd14fb7ba2a79c899abda6a864038c 
>   src/tests/master_authorization_tests.cpp 
> 1a0285a3f345ef21a8256d4123d8bb684f538da4 
> 
> 
> Diff: https://reviews.apache.org/r/57535/diff/2/
> 
> 
> Testing
> ---
> 
> make check.
> 
> The tests added here cover some basic scenarios, I have more tests but will 
> add them when MESOS-7244 is fixed.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>