This is an automatically generated e-mail. To reply, visit:

docs/authorization.md (line 12)

    s/JSON based/JSON-based/
    It might be helpful at the beginning to give a broad picture of how these 
ACLs work, something like: "When authorizing an action, Mesos proceeds through 
the list of relevant ACLs until it finds one that can either grant or deny 
permission to the  subject making the request."

docs/authorization.md (line 14)

    I would prefer to avoid mentioning 'acls.proto' here; I think that we 
should try to create documentation that provides adequate guidance for users 
without referring them to the code itself.
    I think that we could provide a schema here, or we might be able to get rid 
of this passage (except for the explanation of the `permissive` field). The two 
passages following this one do a good job of explaining the structure of the 
    What do you think?

docs/authorization.md (lines 16 - 18)

    s/consist on an array/consists of an array/
    s/Each of this objects/Each of these objects/
    I would recommend splitting the second sentence into three: "Each of these 
objects has two entries. The first, `principals`, is common to all actions and 
describes the subjects which wish to perform the given action. The second entry 
varies among actions and describes the object on which the action will be 
    I would prefer to avoid mentioning the `Entity` protobuf message directly. 
I think it would be easier to understand if we simply describe the 
characteristics of the subject and object fields, as you do in line 18, without 
mentioning `Entity`. What do you think?

docs/authorization.md (line 21)

    s/which is default/which is the default/
    s/In case permissive/If permissive/
    s/set to false all/set to false, all/
    s/non-matching request/non-matching requests/

docs/authorization.md (line 24)

    The ACL doesn't imply that `foo` can register in no other role besides 
`analytics`. If `foo` tries to register in another role, it won't match any 
ACL, and since `permissive` is true by default, the request will be authorized.

docs/authorization.md (line 49)

    s/set to `false` and hence principals/set to `false`; hence, principals/
    /but not as any other user/and not as any other user/

docs/authorization.md (lines 51 - 61)

    I think the doc would benefit from a couple more examples here. An example 
showcasing the order of ACLs might be helpful (i.e., a case where the order of 
the ACLs really matters). Also, perhaps an example with multiple 
principals/objects in those JSON lists?

docs/authorization.md (line 65)

    s/Currently _local authorizer_/Currently the _local authorizer_/
    s/for following/for the following/

docs/authorization.md (line 92)

    I would change the actions to be consistent in tense. The preceding items 
use the progressive tense - i.e., "Destroying quotas" - while this one doesn't 
- "Reserve resources".

docs/authorization.md (line 94)

    Could also be an operator

docs/authorization.md (line 99)

    Can also be an operator

docs/authorization.md (line 104)

    Can also be an operator

docs/authorization.md (line 109)

    Can also be an operator

docs/authorization.md (line 117)

    s/Implementing Authorizer/Implementing an Authorizer/

docs/authorization.md (line 119)

    Is this section targeted at people who will write their own authorizer 
modules? If so, we could make this more explicit like so: "In case you plan to 
implement your own authorizer module, the authorization interface consists of 
two parts:"

docs/authorization.md (line 120)

    s/First the/First, the/
    s/interface defining the/interface defines the/
    s/and _local authorizer_/and the _local authorizer_/
    s/In case the request ... otherwise./This function should return `false` if 
the request is rejected and `true` otherwise./

docs/authorization.md (line 122)

    s/Secondly the/Second, the/
    s/message representing/message represents/
    Actually, since `authorization::Request` occurs in the definition of 
`authorized()`, perhaps it's better for this part of the interface to be 
explained first, what do you think?

docs/authorization.md (line 141)

    s/is kept optional ... quantification./is optional; if it isn't set, then 
the `Subject` or `Object` will only match an ACL with ANY or NONE in the 
corresponding location./
    s/This allows users can construct/This allows users to construct/

docs/authorization.md (line 143)

    s/valid action is necessary/valid action necessary/

docs/authorization.md (line 147)

    Do we really need this here?

- Greg Mann

On April 22, 2016, 12:13 p.m., Alexander Rojas wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46501/
> -----------------------------------------------------------
> (Updated April 22, 2016, 12:13 p.m.)
> Review request for mesos, Adam B, Greg Mann, and Neil Conway.
> Bugs: MESOS-4785
>     https://issues.apache.org/jira/browse/MESOS-4785
> Repository: mesos
> Description
> -------
> The API of the authorization has been changing constantly over the
> last few versions. This patch attempts to update the documentation to
> the those changes into account.
> Diffs
> -----
>   docs/authorization.md 0db5c345b3239814b3b9d2e8a87601ff69d0f869 
> Diff: https://reviews.apache.org/r/46501/diff/
> Testing
> -------
> Thanks,
> Alexander Rojas

Reply via email to