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

Thanks for the changes, this is looking great!  My most pressing concern is 
related to not also passing `auth_data`.  Based on the outcome of that topic, 
we may want to change the JSON schema to allow specification of that as well.

Thinking on this more, it would be great for the project and for the 
longetivity of your feature to enable it in end-to-end tests.  (Really, all i 
mean is to configure the stock vagrant environment to use it.)  Happy to help 
on the dev list and/or IRC if it's not clear how to do this.

Meta-review request: please make use of reviewboard's comment feature to 
ack/nack/discuss review comments.  Especially when there's a bunch of comments, 
it can be difficult as a reviewer to keep track of what action was taken on 
each item, if any.

docs/security.md (line 289)

    I'd like to propose several changes to this section, which i've made in the 
rewritten block below.
    - Use consistent naming and proper nouns for projects (Thermos, ZooKeeper)
    - Link to latest version of ZooKeeper docs
    - Immediately link to relevant ZooKeeper ACL section
    - Describe how to enable the feature before describing the format of the 
ACL file
    - Use more accurate requirements level terminology, e.g. must/may/should 
(context reading http://tools.ietf.org/html/rfc2119)
    # Announcer Authentication
    Nodes created by the Thermos executor may include ZooKeeper
    which will specify the priviliges of clients to perform different actions 
on these nodes.  This
    feature is enabled by specifying an ACL configuration file to the executor 
with the
    `--announcer-zookeeper-auth-config` command line argument.
    When this feature is _not_ enabled, nodes created by the executor will have 
'world/all' permission
    (`ZOO_OPEN_ACL_UNSAFE`).  In most production environments, operators should 
specify ACLs and
    limit access.
    ## ACL configuration format
    The configuration file must be formatted as JSON with the following schema:
        "scheme": "<scheme>",
        "credential": "<credential>",
        "permissions": {
          "read": <bool>,
          "write": <bool>,
          "create": <bool>,
          "delete": <bool>,
          "admin": <bool>,
          "all": <bool>
    defines the encoding of the `credential` field.  Note that these fields are 
passed directly to
    ZoooKeeper.  If a scheme is used that requires credential hashing, the 
value of the `credential`
    field must be hashed (i.e. the executor will not hash this value).
    All properties of the `permissions` object will default to `False` if not 

src/main/python/apache/aurora/executor/bin/thermos_executor_main.py (line 107)

    Sorry for not catching this earlier - how about s/auth/acl/?  I think this 
is more specific to what the file defines.

src/main/python/apache/aurora/executor/bin/thermos_executor_main.py (line 111)

    Is this more accurate? `Path to ZooKeeper ACL to use for announcer nodes.`

src/main/python/apache/aurora/executor/common/announcer.py (line 84)

    I think we should also require that at least one field is specified in 
`permissions`.  Seems like the ACL entry would be meaningliess otherwise.

src/main/python/apache/aurora/executor/common/announcer.py (lines 95 - 96)

    I suggest you just have `validate` raise for invalid and catch here, rather 
than returning a `bool`.  The sequence here creates two separate log entries 
that could just as easily be 1, e.g.
    Expecting a list of ACL objects
    ZK authentication config is invalid

src/main/python/apache/aurora/executor/common/announcer.py (lines 186 - 187)

    Feeling some deja vu here - the double underscore is unconventional - 
please change to single underscore.

src/main/python/apache/aurora/executor/common/announcer.py (line 192)

    I just realized something - does it make much sense to specify 
`default_acl` without also specifying `auth_data`?
    :param auth_data:
      A list of authentication credentials to use for the
      connection. Should be a list of (scheme, credential)
      tuples as :meth:`add_auth` takes.
    Worse yet - can the announcer lock itself out if it _doesn't_ authenticate 
    Enabling this feature in the default vagrant environment will be 
informative, and may guide our approach.  But i do think that either way it 
makes sense to follow up with plumbing for `auth_data`.

src/main/python/apache/aurora/executor/common/announcer.py (line 193)

    Formatting nit - when a method signature or method call needs to wrap, 
please wrap to put each argument on its own line.  In this case:
    return KazooClient(

src/test/python/apache/aurora/executor/common/test_announcer.py (line 352)

    I assume this string is effectively a blob for this test.  If so, can you 
make it explicit by using a fake value (e.g. `'fake'`)?

- Bill Farner

On March 22, 2016, 11:51 a.m., Kunal Thakar wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45042/
> -----------------------------------------------------------
> (Updated March 22, 2016, 11:51 a.m.)
> Review request for Aurora, Bill Farner and Zameer Manji.
> Repository: aurora
> Description
> -------
> Add ACL support for announcer
> https://issues.apache.org/jira/browse/AURORA-1643
> Adding support for service discovery ZK authentication. ZK authentication 
> secrets should be stored in a file as json (as follows):
> ```json
> {
>   "scheme": "<scheme>",
>   "credential": "<credential>",
>       "permissions": {
>         "read": <bool>,
>         "write": <bool>,
>         "create": <bool>,
>         "delete": <bool>,
>         "admin": <bool>,
>         "all": <bool>
>       }
> }
> ```
> Diffs
> -----
>   RELEASE-NOTES.md 6e9364e34db6dbb270468db3ff333b956c6bf9f3 
>   docs/security.md 32bea426fbceec83187e851a5db11e82df55e962 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 
> 6634506108c346f8c23b2da7cc8d20d09d07d590 
>   src/main/python/apache/aurora/executor/common/announcer.py 
> 79a9cfb6ac3a8444f09fb3658e6e859e06941ba4 
> src/test/python/apache/aurora/executor/bin/test_thermos_executor_entry_point.py
>  e9f7851292aef3a36da5da9b0fc333a7e7750cf3 
>   src/test/python/apache/aurora/executor/common/test_announcer.py 
> 142b58d5e577c9f4b8e2ae8473cffdea94eba21f 
> Diff: https://reviews.apache.org/r/45042/diff/
> Testing
> -------
> Thanks,
> Kunal Thakar

Reply via email to