You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Neil Conway <ne...@gmail.com> on 2017/02/28 20:23:07 UTC
Review Request 57166: Updated role validation for hierarchical roles.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57166/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier and Michael Park.
Repository: mesos
Description
-------
Role names can now contain forward slashes. Each component in a role
name must now be validated separately.
Diffs
-----
src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a
src/tests/role_tests.cpp 77f3d46a544a51ba71476e2f0735bb32758dd9e1
Diff: https://reviews.apache.org/r/57166/diff/
Testing
-------
`make check`
Thanks,
Neil Conway
Re: Review Request 57166: Updated role validation for hierarchical
roles.
Posted by Neil Conway <ne...@gmail.com>.
> On March 1, 2017, 10:43 a.m., Benjamin Bannier wrote:
> > I wonder if it would make sense to introduce some more knowledge about role hierarchies here to give the user clearer diagnostics; a role `A/B` could be though of as semantically a role `B` with a parent role `A` and as a user it might make sense to think of both `A` and `B` as roles (this picture works as long we are in the hierarchy below `A`, and do not have roles like `C/B` in the picture).
> >
> > With that we wouldn't talk of _path components_ when validating, but instead directly call each element a role. I believe this might possibly also simplify the implementation, e.g., (pseudocode)
> >
> > def validate(role):
> > # Split `role` keeping empty elements
> > for element in strings.split(role, '/'):
> > # validate `element` with existing non-hierarchical role validation
> >
> > With that validation e.g., a role `A/../B` would yield an `Error("Role name '..' is invalid")`, or a role `A//B` an `Error("Empty role name is invalid")`.
Hmmm, I wonder if this is an improvement: reporting `Error("Empty role name is invalid")` for `A//B` seems like it would be confusing. If we wanted to be more verbose / detailed in the errors we report, I'd vote for something like: `Error("Component '" + element + "' in role '" + role + "' is invalid: ...")`.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57166/#review167254
-----------------------------------------------------------
On March 2, 2017, 6:12 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57166/
> -----------------------------------------------------------
>
> (Updated March 2, 2017, 6:12 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Role names can now contain forward slashes. Each component in a role
> name must now be validated separately.
>
>
> Diffs
> -----
>
> src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a
> src/tests/role_tests.cpp 77f3d46a544a51ba71476e2f0735bb32758dd9e1
>
>
> Diff: https://reviews.apache.org/r/57166/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57166: Updated role validation for hierarchical
roles.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57166/#review167254
-----------------------------------------------------------
I wonder if it would make sense to introduce some more knowledge about role hierarchies here to give the user clearer diagnostics; a role `A/B` could be though of as semantically a role `B` with a parent role `A` and as a user it might make sense to think of both `A` and `B` as roles (this picture works as long we are in the hierarchy below `A`, and do not have roles like `C/B` in the picture).
With that we wouldn't talk of _path components_ when validating, but instead directly call each element a role. I believe this might possibly also simplify the implementation, e.g., (pseudocode)
def validate(role):
# Split `role` keeping empty elements
for element in strings.split(role, '/'):
# validate `element` with existing non-hierarchical role validation
With that validation e.g., a role `A/../B` would yield an `Error("Role name '..' is invalid")`, or a role `A//B` an `Error("Empty role name is invalid")`.
- Benjamin Bannier
On Feb. 28, 2017, 9:23 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57166/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 9:23 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Role names can now contain forward slashes. Each component in a role
> name must now be validated separately.
>
>
> Diffs
> -----
>
> src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a
> src/tests/role_tests.cpp 77f3d46a544a51ba71476e2f0735bb32758dd9e1
>
>
> Diff: https://reviews.apache.org/r/57166/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57166: Updated role validation for hierarchical
roles.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57166/#review171391
-----------------------------------------------------------
Ship it!
Ship It!
- Adam B
On March 7, 2017, 7:38 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57166/
> -----------------------------------------------------------
>
> (Updated March 7, 2017, 7:38 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Role names can now contain forward slashes. Each component in a role
> name must now be validated separately.
>
>
> Diffs
> -----
>
> src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a
> src/tests/role_tests.cpp 77f3d46a544a51ba71476e2f0735bb32758dd9e1
>
>
> Diff: https://reviews.apache.org/r/57166/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57166: Updated role validation for hierarchical
roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57166/
-----------------------------------------------------------
(Updated March 7, 2017, 3:38 p.m.)
Review request for mesos, Benjamin Bannier and Michael Park.
Changes
-------
Address review comments.
Repository: mesos
Description
-------
Role names can now contain forward slashes. Each component in a role
name must now be validated separately.
Diffs (updated)
-----
src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a
src/tests/role_tests.cpp 77f3d46a544a51ba71476e2f0735bb32758dd9e1
Diff: https://reviews.apache.org/r/57166/diff/3/
Changes: https://reviews.apache.org/r/57166/diff/2-3/
Testing
-------
`make check`
Thanks,
Neil Conway
Re: Review Request 57166: Updated role validation for hierarchical
roles.
Posted by Neil Conway <ne...@gmail.com>.
> On March 7, 2017, 5:26 a.m., Michael Park wrote:
> > src/common/roles.cpp
> > Lines 72 (patched)
> > <https://reviews.apache.org/r/57166/diff/2/?file=1653834#file1653834line73>
> >
> > For these error messages, how about we say something like this?
> >
> > ```cpp
> > "A role cannot start with a slash, given: '" + role + "'"
> > ```
> >
> > Here and below.
The "given" phrasing here seems confusing to me (it isn't clear who is being "given" what...). Personally I think the current phrasing is okay...
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57166/#review168096
-----------------------------------------------------------
On March 2, 2017, 6:12 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57166/
> -----------------------------------------------------------
>
> (Updated March 2, 2017, 6:12 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Role names can now contain forward slashes. Each component in a role
> name must now be validated separately.
>
>
> Diffs
> -----
>
> src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a
> src/tests/role_tests.cpp 77f3d46a544a51ba71476e2f0735bb32758dd9e1
>
>
> Diff: https://reviews.apache.org/r/57166/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57166: Updated role validation for hierarchical
roles.
Posted by Michael Park <mp...@apache.org>.
> On March 6, 2017, 9:26 p.m., Michael Park wrote:
> > src/common/roles.cpp
> > Lines 72 (patched)
> > <https://reviews.apache.org/r/57166/diff/2/?file=1653834#file1653834line73>
> >
> > For these error messages, how about we say something like this?
> >
> > ```cpp
> > "A role cannot start with a slash, given: '" + role + "'"
> > ```
> >
> > Here and below.
>
> Neil Conway wrote:
> The "given" phrasing here seems confusing to me (it isn't clear who is being "given" what...). Personally I think the current phrasing is okay...
Okay, I think I was just trying to suggest that the statement should make a general statement about all roles, rather than say the __particular role__ cannot <whatever>.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57166/#review168096
-----------------------------------------------------------
On March 7, 2017, 7:38 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57166/
> -----------------------------------------------------------
>
> (Updated March 7, 2017, 7:38 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Role names can now contain forward slashes. Each component in a role
> name must now be validated separately.
>
>
> Diffs
> -----
>
> src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a
> src/tests/role_tests.cpp 77f3d46a544a51ba71476e2f0735bb32758dd9e1
>
>
> Diff: https://reviews.apache.org/r/57166/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57166: Updated role validation for hierarchical
roles.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57166/#review168096
-----------------------------------------------------------
Fix it, then Ship it!
src/common/roles.cpp
Line 71 (original), 71-79 (patched)
<https://reviews.apache.org/r/57166/#comment240222>
Newlines in between.
src/common/roles.cpp
Lines 72 (patched)
<https://reviews.apache.org/r/57166/#comment240223>
For these error messages, how about we say something like this?
```cpp
"A role cannot start with a slash, given: '" + role + "'"
```
Here and below.
src/common/roles.cpp
Lines 82 (patched)
<https://reviews.apache.org/r/57166/#comment240220>
`s/pathElements/components/`
src/common/roles.cpp
Line 78 (original), 90 (patched)
<https://reviews.apache.org/r/57166/#comment240221>
`s/element/component/`
- Michael Park
On March 2, 2017, 10:12 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57166/
> -----------------------------------------------------------
>
> (Updated March 2, 2017, 10:12 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Role names can now contain forward slashes. Each component in a role
> name must now be validated separately.
>
>
> Diffs
> -----
>
> src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a
> src/tests/role_tests.cpp 77f3d46a544a51ba71476e2f0735bb32758dd9e1
>
>
> Diff: https://reviews.apache.org/r/57166/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57166: Updated role validation for hierarchical
roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57166/
-----------------------------------------------------------
(Updated March 2, 2017, 6:12 p.m.)
Review request for mesos, Benjamin Bannier and Michael Park.
Changes
-------
Address review comments.
Repository: mesos
Description
-------
Role names can now contain forward slashes. Each component in a role
name must now be validated separately.
Diffs (updated)
-----
src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a
src/tests/role_tests.cpp 77f3d46a544a51ba71476e2f0735bb32758dd9e1
Diff: https://reviews.apache.org/r/57166/diff/2/
Changes: https://reviews.apache.org/r/57166/diff/1-2/
Testing
-------
`make check`
Thanks,
Neil Conway
Re: Review Request 57166: Updated role validation for hierarchical
roles.
Posted by Qian Zhang <zh...@gmail.com>.
> On March 2, 2017, 9:21 a.m., Qian Zhang wrote:
> > src/common/roles.cpp
> > Line 71 (original), 71 (patched)
> > <https://reviews.apache.org/r/57166/diff/1/?file=1652003#file1652003line72>
> >
> > Why can't role start with a slash? According to the design doc, it seems the role like `/eng/frontend` is valid.
>
> Neil Conway wrote:
> Qian, this aspect of the hierarchical role design has changed since the design doc was initially proposed. The design doc has been partially updated -- I'll finish updating it shortly. The basic idea is that the syntax for hierarchical roles will just be an extension to the current syntax -- i.e., hierarchical role names will look like `eng/dev` and `eng/prod`, not `/eng/dev`.
Thanks Neil for the clarification. But with this new design, the role name is more like a relative path rather than an absolute path, but I guess what we need is an absolute one, so when framework tries to register a role like `eng/dev`, we will always assume it is an absolute one (i.e., `eng` is root role), right?
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57166/#review167623
-----------------------------------------------------------
On March 3, 2017, 2:12 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57166/
> -----------------------------------------------------------
>
> (Updated March 3, 2017, 2:12 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Role names can now contain forward slashes. Each component in a role
> name must now be validated separately.
>
>
> Diffs
> -----
>
> src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a
> src/tests/role_tests.cpp 77f3d46a544a51ba71476e2f0735bb32758dd9e1
>
>
> Diff: https://reviews.apache.org/r/57166/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57166: Updated role validation for hierarchical
roles.
Posted by Neil Conway <ne...@gmail.com>.
> On March 2, 2017, 1:21 a.m., Qian Zhang wrote:
> > src/common/roles.cpp
> > Line 71 (original), 71 (patched)
> > <https://reviews.apache.org/r/57166/diff/1/?file=1652003#file1652003line72>
> >
> > Why can't role start with a slash? According to the design doc, it seems the role like `/eng/frontend` is valid.
Qian, this aspect of the hierarchical role design has changed since the design doc was initially proposed. The design doc has been partially updated -- I'll finish updating it shortly. The basic idea is that the syntax for hierarchical roles will just be an extension to the current syntax -- i.e., hierarchical role names will look like `eng/dev` and `eng/prod`, not `/eng/dev`.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57166/#review167623
-----------------------------------------------------------
On Feb. 28, 2017, 8:23 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57166/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 8:23 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Role names can now contain forward slashes. Each component in a role
> name must now be validated separately.
>
>
> Diffs
> -----
>
> src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a
> src/tests/role_tests.cpp 77f3d46a544a51ba71476e2f0735bb32758dd9e1
>
>
> Diff: https://reviews.apache.org/r/57166/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57166: Updated role validation for hierarchical
roles.
Posted by Neil Conway <ne...@gmail.com>.
> On March 2, 2017, 1:21 a.m., Qian Zhang wrote:
> > src/common/roles.cpp
> > Line 71 (original), 71 (patched)
> > <https://reviews.apache.org/r/57166/diff/1/?file=1652003#file1652003line72>
> >
> > Why can't role start with a slash? According to the design doc, it seems the role like `/eng/frontend` is valid.
>
> Neil Conway wrote:
> Qian, this aspect of the hierarchical role design has changed since the design doc was initially proposed. The design doc has been partially updated -- I'll finish updating it shortly. The basic idea is that the syntax for hierarchical roles will just be an extension to the current syntax -- i.e., hierarchical role names will look like `eng/dev` and `eng/prod`, not `/eng/dev`.
>
> Qian Zhang wrote:
> Thanks Neil for the clarification. But with this new design, the role name is more like a relative path rather than an absolute path, but I guess what we need is an absolute one, so when framework tries to register a role like `eng/dev`, we will always assume it is an absolute one (i.e., `eng` is root role), right?
Role names do not begin with `/`, but they are effectively relative paths that are interpreted by starting from the root of the role tree.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57166/#review167623
-----------------------------------------------------------
On March 2, 2017, 6:12 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57166/
> -----------------------------------------------------------
>
> (Updated March 2, 2017, 6:12 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Role names can now contain forward slashes. Each component in a role
> name must now be validated separately.
>
>
> Diffs
> -----
>
> src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a
> src/tests/role_tests.cpp 77f3d46a544a51ba71476e2f0735bb32758dd9e1
>
>
> Diff: https://reviews.apache.org/r/57166/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57166: Updated role validation for hierarchical
roles.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57166/#review167623
-----------------------------------------------------------
src/common/roles.cpp
Line 71 (original), 71 (patched)
<https://reviews.apache.org/r/57166/#comment239529>
Why can't role start with a slash? According to the design doc, it seems the role like `/eng/frontend` is valid.
src/common/roles.cpp
Lines 83 (patched)
<https://reviews.apache.org/r/57166/#comment239533>
Kill this blank line.
src/common/roles.cpp
Line 82 (original), 95 (patched)
<https://reviews.apache.org/r/57166/#comment239549>
s/Roles/Role/
- Qian Zhang
On March 1, 2017, 4:23 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57166/
> -----------------------------------------------------------
>
> (Updated March 1, 2017, 4:23 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Role names can now contain forward slashes. Each component in a role
> name must now be validated separately.
>
>
> Diffs
> -----
>
> src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a
> src/tests/role_tests.cpp 77f3d46a544a51ba71476e2f0735bb32758dd9e1
>
>
> Diff: https://reviews.apache.org/r/57166/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>