Re: Review Request 40167: [2/7] Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Dec. 17, 2015, 6:58 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40167/
> ---
> 
> (Updated Dec. 17, 2015, 6:58 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-4178
> https://issues.apache.org/jira/browse/MESOS-4178
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 74fcc86d3c92cb3aa27e45b647b1653705b3201c 
> 
> Diff: https://reviews.apache.org/r/40167/diff/
> 
> 
> Testing
> ---
> 
> This is the second in a chain of 7 patches. `make check` was used to test 
> after all patches were applied.
> 
> Note that this chain of patches touches many of the same files as another 
> chain beginning with Review #39985 and ending with Review #39989, which is 
> currently in review as well. To avoid conflicts, the beginning of this chain 
> begins on top of Review #39989.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 40167: [2/7] Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.

[Greg Mann]

> On Dec. 16, 2015, 2:35 p.m., Alexander Rukletsov wrote:
> >

Thanks for the reviews, AlexR!!


> On Dec. 16, 2015, 2:35 p.m., Alexander Rukletsov wrote:
> > include/mesos/authorizer/authorizer.proto, line 106
> > 
> >
> > created the volume?

:facepalm:

Thanks AlexR :-)


> On Dec. 16, 2015, 2:35 p.m., Alexander Rukletsov wrote:
> > include/mesos/authorizer/authorizer.proto, lines 100-101
> > 
> >
> > Why did you wrap the comment this way?

This comment is just over 80 characters long, and by wrapping this way I reduce 
the overall "jaggedness". I find this a bit easier to read than a two-line 
comment that has just one or two words on the second line.


- Greg


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


On Dec. 17, 2015, 12:03 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40167/
> ---
> 
> (Updated Dec. 17, 2015, 12:03 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-4178
> https://issues.apache.org/jira/browse/MESOS-4178
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 74fcc86d3c92cb3aa27e45b647b1653705b3201c 
> 
> Diff: https://reviews.apache.org/r/40167/diff/
> 
> 
> Testing
> ---
> 
> This is the second in a chain of 7 patches. `make check` was used to test 
> after all patches were applied.
> 
> Note that this chain of patches touches many of the same files as another 
> chain beginning with Review #39985 and ending with Review #39989, which is 
> currently in review as well. To avoid conflicts, the beginning of this chain 
> begins on top of Review #39989.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 40167: [2/7] Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.

[Greg Mann]

> On Dec. 16, 2015, 2:35 p.m., Alexander Rukletsov wrote:
> > include/mesos/authorizer/authorizer.proto, line 96
> > 
> >
> > I see that you ensure this when you create requests in 
> > `authorizeCreateVolume`, but I'm not sure it's validated for ACLs. Do you 
> > think it makes sense to add validation in the 
> > `LocalAuthorizer::initialize()`?
> > 
> > After a second thought, this note relates to the default 
> > implementation, because the master does not really validate it, right? 
> > Which means a 3rdparty authorizer may react to particular types.

You're correct that a 3rd party authorizer could handle values other than ANY 
and NONE, but the current default implementation enforces this constraint by 
always setting `volume_types` equal to ANY in the request ACL. I altered this 
comment to clarify that point a bit; feel free to re-open the issue if you 
think we should do more.


- Greg


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


On Dec. 17, 2015, 6:58 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40167/
> ---
> 
> (Updated Dec. 17, 2015, 6:58 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-4178
> https://issues.apache.org/jira/browse/MESOS-4178
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 74fcc86d3c92cb3aa27e45b647b1653705b3201c 
> 
> Diff: https://reviews.apache.org/r/40167/diff/
> 
> 
> Testing
> ---
> 
> This is the second in a chain of 7 patches. `make check` was used to test 
> after all patches were applied.
> 
> Note that this chain of patches touches many of the same files as another 
> chain beginning with Review #39985 and ending with Review #39989, which is 
> currently in review as well. To avoid conflicts, the beginning of this chain 
> begins on top of Review #39989.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 40167: [2/7] Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.

[Greg Mann]

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

(Updated Dec. 17, 2015, 6:58 a.m.)


Review request for mesos, Jie Yu, Michael Park, and Neil Conway.


Changes
---

Addressed comment.


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


Repository: mesos


Description
---

Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.proto 
74fcc86d3c92cb3aa27e45b647b1653705b3201c 

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


Testing
---

This is the second in a chain of 7 patches. `make check` was used to test after 
all patches were applied.

Note that this chain of patches touches many of the same files as another chain 
beginning with Review #39985 and ending with Review #39989, which is currently 
in review as well. To avoid conflicts, the beginning of this chain begins on 
top of Review #39989.


Thanks,

Greg Mann

Re: Review Request 40167: [2/7] Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.

[Greg Mann]

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

(Updated Dec. 17, 2015, 12:03 a.m.)


Review request for mesos, Jie Yu, Michael Park, and Neil Conway.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.proto 
74fcc86d3c92cb3aa27e45b647b1653705b3201c 

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


Testing
---

This is the second in a chain of 7 patches. `make check` was used to test after 
all patches were applied.

Note that this chain of patches touches many of the same files as another chain 
beginning with Review #39985 and ending with Review #39989, which is currently 
in review as well. To avoid conflicts, the beginning of this chain begins on 
top of Review #39989.


Thanks,

Greg Mann

Re: Review Request 40167: [2/7] Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.

[Alexander Rukletsov]

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



include/mesos/authorizer/authorizer.proto (line 90)


I think "this principal" is a bit misleading, because we may specify 
multiple principals. I would say "a principal" or "listed principals" are a 
better fit. Not a native speaker though.



include/mesos/authorizer/authorizer.proto (line 95)


Again, should not be singular, I suppose



include/mesos/authorizer/authorizer.proto (line 96)


I see that you ensure this when you create requests in 
`authorizeCreateVolume`, but I'm not sure it's validated for ACLs. Do you think 
it makes sense to add validation in the `LocalAuthorizer::initialize()`?

After a second thought, this note relates to the default implementation, 
because the master does not really validate it, right? Which means a 3rdparty 
authorizer may react to particular types.



include/mesos/authorizer/authorizer.proto (lines 100 - 101)


Why did you wrap the comment this way?



include/mesos/authorizer/authorizer.proto (line 106)


created the volume?


- Alexander Rukletsov


On Dec. 15, 2015, 9:18 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40167/
> ---
> 
> (Updated Dec. 15, 2015, 9:18 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-4178
> https://issues.apache.org/jira/browse/MESOS-4178
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 74fcc86d3c92cb3aa27e45b647b1653705b3201c 
> 
> Diff: https://reviews.apache.org/r/40167/diff/
> 
> 
> Testing
> ---
> 
> This is the second in a chain of 7 patches. `make check` was used to test 
> after all patches were applied.
> 
> Note that this chain of patches touches many of the same files as another 
> chain beginning with Review #39985 and ending with Review #39989, which is 
> currently in review as well. To avoid conflicts, the beginning of this chain 
> begins on top of Review #39989.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 40167: [2/7] Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.

[Greg Mann]

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

(Updated Dec. 15, 2015, 9:18 p.m.)


Review request for mesos, Jie Yu, Michael Park, and Neil Conway.


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


Repository: mesos


Description
---

Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.


Diffs
-

  include/mesos/authorizer/authorizer.proto 
74fcc86d3c92cb3aa27e45b647b1653705b3201c 

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


Testing
---

This is the second in a chain of 7 patches. `make check` was used to test after 
all patches were applied.

Note that this chain of patches touches many of the same files as another chain 
beginning with Review #39985 and ending with Review #39989, which is currently 
in review as well. To avoid conflicts, the beginning of this chain begins on 
top of Review #39989.


Thanks,

Greg Mann

Re: Review Request 40167: [2/7] Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.

[Greg Mann]

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

(Updated Dec. 8, 2015, 5:51 p.m.)


Review request for mesos, Jie Yu, Michael Park, and Neil Conway.


Changes
---

Updated comment.


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


Repository: mesos


Description
---

Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.proto 
74fcc86d3c92cb3aa27e45b647b1653705b3201c 

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


Testing
---

This is the second in a chain of 7 patches. `make check` was used to test after 
all patches were applied.

Note that this chain of patches touches many of the same files as another chain 
beginning with Review #39985 and ending with Review #39989, which is currently 
in review as well. To avoid conflicts, the beginning of this chain begins on 
top of Review #39989.


Thanks,

Greg Mann

Re: Review Request 40167: [2/7] Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.

[Greg Mann]

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

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


Review request for mesos, Jie Yu, Michael Park, and Neil Conway.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.proto 
8e72003f405770f00c5d87f318a9e1a8ed7430ee 

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


Testing
---

This is the second in a chain of 7 patches. `make check` was used to test after 
all patches were applied.

Note that this chain of patches touches many of the same files as another chain 
beginning with Review #39985 and ending with Review #39989, which is currently 
in review as well. To avoid conflicts, the beginning of this chain begins on 
top of Review #39989.


Thanks,

Greg Mann

Re: Review Request 40167: [2/7] Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.

[Greg Mann]

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

(Updated Nov. 14, 2015, 12:22 a.m.)


Review request for mesos, Jie Yu, Michael Park, and Neil Conway.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.proto 
86bbb45f9d91b4098a262e3e50a793f3bb39497e 

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


Testing
---

This is the second in a chain of 7 patches. `make check` was used to test after 
all patches were applied.

Note that this chain of patches touches many of the same files as another chain 
beginning with Review #39985 and ending with Review #39989, which is currently 
in review as well. To avoid conflicts, the beginning of this chain begins on 
top of Review #39989.


Thanks,

Greg Mann

Re: Review Request 40167: [2/7] Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.

[Greg Mann]

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

(Updated Nov. 13, 2015, 1:20 a.m.)


Review request for mesos, Jie Yu, Michael Park, and Neil Conway.


Summary (updated)
-

[2/7] Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.


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


Repository: mesos


Description
---

Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.


Diffs
-

  include/mesos/authorizer/authorizer.proto 
86bbb45f9d91b4098a262e3e50a793f3bb39497e 

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


Testing
---

This is the second in a chain of 6 patches. `make check` was used to test after 
all patches were applied.

Note that this chain of patches touches many of the same files as another chain 
beginning with Review #39985 and ending with Review #39989, which is currently 
in review as well. To avoid conflicts, the beginning of this chain begins on 
top of Review #39989.


Thanks,

Greg Mann

Re: Review Request 40167: [2/7] Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.

[Greg Mann]

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

(Updated Nov. 13, 2015, 1:22 a.m.)


Review request for mesos, Jie Yu, Michael Park, and Neil Conway.


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


Repository: mesos


Description
---

Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.


Diffs
-

  include/mesos/authorizer/authorizer.proto 
86bbb45f9d91b4098a262e3e50a793f3bb39497e 

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


Testing (updated)
---

This is the second in a chain of 7 patches. `make check` was used to test after 
all patches were applied.

Note that this chain of patches touches many of the same files as another chain 
beginning with Review #39985 and ending with Review #39989, which is currently 
in review as well. To avoid conflicts, the beginning of this chain begins on 
top of Review #39989.


Thanks,

Greg Mann