aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bill Farner <wfar...@apache.org>
Subject Re: Review Request 45042: Add ACL support for announcer
Date Wed, 30 Mar 2016 02:20:34 GMT

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



Went through docs with a fine-toothed comb.  Only ship blocker for me right now is that the
latest draft doesn't exercise ZK auth in the vagrant example config.


RELEASE-NOTES.md (line 14)
<https://reviews.apache.org/r/45042/#comment188917>

    s/Support/Added support/
    s/ZK/ZooKeeper/



docs/operations/security.md (line 26)
<https://reviews.apache.org/r/45042/#comment188918>

    s/Zookeeper/ZooKeeper/ (capital K)



docs/operations/security.md (line 34)
<https://reviews.apache.org/r/45042/#comment188919>

    "To enable authentication for the announcer, see..."



docs/operations/security.md (line 292)
<https://reviews.apache.org/r/45042/#comment188920>

    "The Thermos executor can be configured to authenticate with ZooKeeper and include an
[ACL] on the nodes it creates, which will specify..."



docs/operations/security.md (line 299)
<https://reviews.apache.org/r/45042/#comment188921>

    s/ACL/an ACL/



examples/vagrant/announcer-auth.json (line 1)
<https://reviews.apache.org/r/45042/#comment188922>

    Whoops, in the last round i intended for you to only _add_ world read access.  We should
not remove the auth and restricted write/create access - these are valuable to exercise in
the vagrant environment.



src/main/python/apache/aurora/executor/bin/thermos_executor_main.py (line 201)
<https://reviews.apache.org/r/45042/#comment188923>

    feel free to inline this below to avoid the extra var declaration



src/main/python/apache/aurora/executor/common/announcer.py (lines 65 - 70)
<https://reviews.apache.org/r/45042/#comment188924>

    Nice!



src/main/python/apache/aurora/executor/common/announcer.py (lines 154 - 164)
<https://reviews.apache.org/r/45042/#comment188926>

    Use a list comprehension instead:
    
    ```
    def to_acl(access):
      return make_acl(...)
    
    default_acl = [to_acl(a) for a in self._zk_auth.acl()]
    ```



src/main/python/apache/aurora/executor/common/announcer.py (lines 168 - 169)
<https://reviews.apache.org/r/45042/#comment188925>

    I assume you do the `[]` -> `None` conversion because the behavior is different for
these args?
    
    If so, you can make the code more concise and avoid these variable reassignments:
    ```
    auth_data = auth_data if auth_data else None
    ...
    default_acl = default_acl if default_acl else None
    ```
    
    By instead doing this:
    ```python
    return KazooClient(self._ensemble,
                       connection_retry=self.DEFAULT_RETRY_POLICY,
                       default_acl=default_acl or None,
                       auth_data=auth_data or None)
    ```



src/test/sh/org/apache/aurora/e2e/validate_serverset.py (line 48)
<https://reviews.apache.org/r/45042/#comment188927>

    Is this needed?  If so, please include a comment explaining why the timeout needs to be
at 30.


- Bill Farner


On March 29, 2016, 5:17 p.m., Kunal Thakar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45042/
> -----------------------------------------------------------
> 
> (Updated March 29, 2016, 5:17 p.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):
> (Updated JSON format for config file)
> ```json
> {
>   "auth": [
>     {
>       "scheme": "<scheme>",
>       "credential": "<plain_credential>"
>     }
>   ],
>   "acls": [
>     {
>       "scheme": "<scheme>",
>       "credential": "<encrypted_credential>",
>       "permissions": {
>         "read": <bool>,
>         "write": <bool>,
>         "create": <bool>,
>         "delete": <bool>,
>         "admin": <bool>,
>         "all": <bool>
>       }
>     }
>   ]
> }
> ```
> 
> 
> Diffs
> -----
> 
>   RELEASE-NOTES.md 34f28a165aae4ae24fa95ef19b4972e088fd63a0 
>   docs/operations/security.md 1a3d9b7e7ba4ec1952dc886d5fbeb6b85d994fb9 
>   examples/vagrant/announcer-auth.json PRE-CREATION 
>   examples/vagrant/upstart/aurora-scheduler.conf 120b89a1dc10a259940cb9527eb2517f19d04471

>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 6634506108c346f8c23b2da7cc8d20d09d07d590

>   src/main/python/apache/aurora/executor/common/announcer.py 79a9cfb6ac3a8444f09fb3658e6e859e06941ba4

>   src/main/python/apache/aurora/executor/common/announcer_zkauth_schema.py PRE-CREATION

>   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

>   src/test/sh/org/apache/aurora/e2e/validate_serverset.py fca1137bd2e7b1306a03dc2a54d2ef15b59af6a8

> 
> Diff: https://reviews.apache.org/r/45042/diff/
> 
> 
> Testing
> -------
> 
> /vagrant/src/test/sh/org/apache/aurora/e2e/test_announcer_auth_end_to_end.sh
> /vagrant/src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Kunal Thakar
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message