You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Greg Mann <gr...@mesosphere.io> on 2015/11/12 23:54:19 UTC
Review Request 40167: [2/6] Added ACL protobuf messages
'CreateVolume' and 'DestroyVolume'.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40167/
-----------------------------------------------------------
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
-------
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'.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
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'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
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'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
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'.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Dec. 16, 2015, 2:35 p.m., Alexander Rukletsov wrote:
> > include/mesos/authorizer/authorizer.proto, line 96
> > <https://reviews.apache.org/r/40167/diff/4/?file=1156029#file1156029line96>
> >
> > 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'.
Posted by Greg Mann <gr...@mesosphere.io>.
> 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
> > <https://reviews.apache.org/r/40167/diff/4/?file=1156029#file1156029line106>
> >
> > 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
> > <https://reviews.apache.org/r/40167/diff/4/?file=1156029#file1156029line100>
> >
> > 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'.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40167/#review110641
-----------------------------------------------------------
include/mesos/authorizer/authorizer.proto (line 90)
<https://reviews.apache.org/r/40167/#comment170654>
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)
<https://reviews.apache.org/r/40167/#comment170657>
Again, should not be singular, I suppose
include/mesos/authorizer/authorizer.proto (line 96)
<https://reviews.apache.org/r/40167/#comment170706>
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)
<https://reviews.apache.org/r/40167/#comment170655>
Why did you wrap the comment this way?
include/mesos/authorizer/authorizer.proto (line 106)
<https://reviews.apache.org/r/40167/#comment170656>
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'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
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'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
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'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
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'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
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'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
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
Re: Review Request 40167: [2/7] Added ACL protobuf messages
'CreateVolume' and 'DestroyVolume'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
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/6] Added ACL protobuf messages
'CreateVolume' and 'DestroyVolume'.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40167/#review106342
-----------------------------------------------------------
Patch looks great!
Reviews applied: [39985, 39986, 39987, 39988, 39989, 40118, 40167]
All tests passed.
- Mesos ReviewBot
On Nov. 12, 2015, 10:54 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40167/
> -----------------------------------------------------------
>
> (Updated Nov. 12, 2015, 10:54 p.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
> -------
>
> 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
>
>