aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kunal Thakar <kun...@gmail.com>
Subject Re: Review Request 45042: Add ACL support for announcer
Date Wed, 30 Mar 2016 19:36:26 GMT


> On March 30, 2016, 2:20 a.m., Bill Farner wrote:
> > RELEASE-NOTES.md, line 14
> > <https://reviews.apache.org/r/45042/diff/7/?file=1318568#file1318568line14>
> >
> >     s/Support/Added support/
> >     s/ZK/ZooKeeper/

Done


> On March 30, 2016, 2:20 a.m., Bill Farner wrote:
> > docs/operations/security.md, line 26
> > <https://reviews.apache.org/r/45042/diff/7/?file=1318569#file1318569line26>
> >
> >     s/Zookeeper/ZooKeeper/ (capital K)

Done


> On March 30, 2016, 2:20 a.m., Bill Farner wrote:
> > docs/operations/security.md, line 34
> > <https://reviews.apache.org/r/45042/diff/7/?file=1318569#file1318569line34>
> >
> >     "To enable authentication for the announcer, see..."

Done


> On March 30, 2016, 2:20 a.m., Bill Farner wrote:
> > docs/operations/security.md, line 292
> > <https://reviews.apache.org/r/45042/diff/7/?file=1318569#file1318569line292>
> >
> >     "The Thermos executor can be configured to authenticate with ZooKeeper and include
an [ACL] on the nodes it creates, which will specify..."

Done


> On March 30, 2016, 2:20 a.m., Bill Farner wrote:
> > docs/operations/security.md, line 299
> > <https://reviews.apache.org/r/45042/diff/7/?file=1318569#file1318569line299>
> >
> >     s/ACL/an ACL/

Done


> On March 30, 2016, 2:20 a.m., Bill Farner wrote:
> > examples/vagrant/announcer-auth.json, line 1
> > <https://reviews.apache.org/r/45042/diff/7/?file=1318570#file1318570line1>
> >
> >     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.

Done. I have a added read and delete permissions for world:anyone as validate_serverset.py
also needs delete perms.


> On March 30, 2016, 2:20 a.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/bin/thermos_executor_main.py, line 201
> > <https://reviews.apache.org/r/45042/diff/7/?file=1318572#file1318572line201>
> >
> >     feel free to inline this below to avoid the extra var declaration

Done


> On March 30, 2016, 2:20 a.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/common/announcer.py, lines 154-164
> > <https://reviews.apache.org/r/45042/diff/7/?file=1318573#file1318573line154>
> >
> >     Use a list comprehension instead:
> >     
> >     ```
> >     def to_acl(access):
> >       return make_acl(...)
> >     
> >     default_acl = [to_acl(a) for a in self._zk_auth.acl()]
> >     ```

Done


> On March 30, 2016, 2:20 a.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/common/announcer.py, lines 168-169
> > <https://reviews.apache.org/r/45042/diff/7/?file=1318573#file1318573line168>
> >
> >     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)
> >     ```

Done


> On March 30, 2016, 2:20 a.m., Bill Farner wrote:
> > src/test/sh/org/apache/aurora/e2e/validate_serverset.py, line 50
> > <https://reviews.apache.org/r/45042/diff/7/?file=1318577#file1318577line50>
> >
> >     Is this needed?  If so, please include a comment explaining why the timeout
needs to be at 30.

This was an artifact of my separate e2e test. I have reverted the timeout to 10.


- Kunal


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


On March 30, 2016, 12:17 a.m., Kunal Thakar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45042/
> -----------------------------------------------------------
> 
> (Updated March 30, 2016, 12:17 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):
> (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