You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Kunal Thakar <ku...@gmail.com> on 2016/03/18 19:37:32 UTC
Review Request 45042: Add ACL support for announcer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/
-----------------------------------------------------------
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
-----
src/main/python/apache/aurora/executor/bin/thermos_executor_main.py f82858c528808d2a9e77bb56f16e897cfb5bbe73
src/main/python/apache/aurora/executor/common/announcer.py 34e36e0a59093468a8934f58bacb68512949347c
src/test/python/apache/aurora/executor/common/test_announcer.py f4032c7302f4733ab5670322b1905102c200f1c9
Diff: https://reviews.apache.org/r/45042/diff/
Testing
-------
Thanks,
Kunal Thakar
Re: Review Request 45042: Add ACL support for announcer
Posted by Zameer Manji <zm...@apache.org>.
> On March 19, 2016, 6:46 p.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/common/announcer.py, line 128
> > <https://reviews.apache.org/r/45042/diff/2/?file=1306943#file1306943line128>
> >
> > please refactor to make the constructor accept the ACL list, making the caller read/parse. this improves testability and reduces the surface area of the class. it also moves errors like file access and parsing issues closer to the source
+1
The caller should load and parse this file.
- Zameer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/#review124431
-----------------------------------------------------------
On March 18, 2016, 2:57 p.m., Kunal Thakar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45042/
> -----------------------------------------------------------
>
> (Updated March 18, 2016, 2:57 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):
> ```json
> {
> "scheme": "<scheme>",
> "credential": "<credential>",
> "permissions": {
> "read": <bool>,
> "write": <bool>,
> "create": <bool>,
> "delete": <bool>,
> "admin": <bool>,
> "all": <bool>
> }
> }
> ```
>
>
> Diffs
> -----
>
> 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/common/test_announcer.py 142b58d5e577c9f4b8e2ae8473cffdea94eba21f
>
> Diff: https://reviews.apache.org/r/45042/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kunal Thakar
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Zameer Manji <zm...@apache.org>.
> On March 19, 2016, 6:46 p.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/common/announcer.py, line 153
> > <https://reviews.apache.org/r/45042/diff/2/?file=1306943#file1306943line153>
> >
> > -1 on the approach of logging and moving forward without auth when errors are encountered. these should all be fatal
+1
It's a common pattern in the rest of the code too. It's best that these errors are fatal so the task can be rescheduled elsewhere and operators get a crude signal for a possible misconfiguration. This current setup enables for silent failures which makes operating Mesos/Aurora difficult.
- Zameer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/#review124431
-----------------------------------------------------------
On March 18, 2016, 2:57 p.m., Kunal Thakar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45042/
> -----------------------------------------------------------
>
> (Updated March 18, 2016, 2:57 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):
> ```json
> {
> "scheme": "<scheme>",
> "credential": "<credential>",
> "permissions": {
> "read": <bool>,
> "write": <bool>,
> "create": <bool>,
> "delete": <bool>,
> "admin": <bool>,
> "all": <bool>
> }
> }
> ```
>
>
> Diffs
> -----
>
> 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/common/test_announcer.py 142b58d5e577c9f4b8e2ae8473cffdea94eba21f
>
> Diff: https://reviews.apache.org/r/45042/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kunal Thakar
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/#review124431
-----------------------------------------------------------
Overall looks in decent shape, made some suggestions.
Before this is ready to land, please:
- add documentation
- cross-link with the jira ticket
src/main/python/apache/aurora/executor/common/announcer.py (line 128)
<https://reviews.apache.org/r/45042/#comment186991>
please refactor to make the constructor accept the ACL list, making the caller read/parse. this improves testability and reduces the surface area of the class. it also moves errors like file access and parsing issues closer to the source
src/main/python/apache/aurora/executor/common/announcer.py (lines 149 - 151)
<https://reviews.apache.org/r/45042/#comment186994>
please read the json as a list, to support multiple ACLs (mentioned this in the ticket)
src/main/python/apache/aurora/executor/common/announcer.py (line 153)
<https://reviews.apache.org/r/45042/#comment186993>
-1 on the approach of logging and moving forward without auth when errors are encountered. these should all be fatal
src/main/python/apache/aurora/executor/common/announcer.py (line 162)
<https://reviews.apache.org/r/45042/#comment186992>
General tip, avoid relying on truthiness when you really mean to compare against `None`
From https://www.python.org/dev/peps/pep-0008/#programming-recommendations
> beware of writing if x when you really mean if x is not None -- e.g. when testing whether a variable or argument that defaults to None was set to some other value. The other value might have a type (such as a container) that could be false in a boolean context
- Bill Farner
On March 18, 2016, 2:57 p.m., Kunal Thakar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45042/
> -----------------------------------------------------------
>
> (Updated March 18, 2016, 2:57 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):
> ```json
> {
> "scheme": "<scheme>",
> "credential": "<credential>",
> "permissions": {
> "read": <bool>,
> "write": <bool>,
> "create": <bool>,
> "delete": <bool>,
> "admin": <bool>,
> "all": <bool>
> }
> }
> ```
>
>
> Diffs
> -----
>
> 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/common/test_announcer.py 142b58d5e577c9f4b8e2ae8473cffdea94eba21f
>
> Diff: https://reviews.apache.org/r/45042/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kunal Thakar
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Zameer Manji <zm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/#review124839
-----------------------------------------------------------
Ship it!
This change LGTM. I was a bit confused if `default_acl` was supposed to be a list or a single instance of `ACL`, but I confirmed that it was a list via checking the source of Kazoo 1.3.1.
Thanks for your contribution, this should help operators who want to maintain a secure ZK cluster for service discovery.
- Zameer Manji
On March 22, 2016, 10:02 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, 10:02 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
> -----
>
> 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/common/test_announcer.py 142b58d5e577c9f4b8e2ae8473cffdea94eba21f
>
> Diff: https://reviews.apache.org/r/45042/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kunal Thakar
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/#review124871
-----------------------------------------------------------
Ship it!
Master (d5d7ec0) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On March 22, 2016, 6:51 p.m., Kunal Thakar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45042/
> -----------------------------------------------------------
>
> (Updated March 22, 2016, 6:51 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):
> ```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
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/#review125778
-----------------------------------------------------------
RELEASE-NOTES.md (line 15)
<https://reviews.apache.org/r/45042/#comment188640>
Link is now dead - should be `docs/operations/security.md#announcer-authentication`
- Bill Farner
On March 28, 2016, 4:10 p.m., Kunal Thakar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45042/
> -----------------------------------------------------------
>
> (Updated March 28, 2016, 4:10 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-announcer-auth.conf PRE-CREATION
> 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
> src/test/sh/org/apache/aurora/e2e/test_announcer_auth_end_to_end.sh PRE-CREATION
> src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh e1c12bbd4382c31e576439f6693d82d5661029b9
> 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
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/#review125775
-----------------------------------------------------------
Ship it!
Master (f28f41a) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On March 28, 2016, 11:10 p.m., Kunal Thakar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45042/
> -----------------------------------------------------------
>
> (Updated March 28, 2016, 11:10 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-announcer-auth.conf PRE-CREATION
> 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
> src/test/sh/org/apache/aurora/e2e/test_announcer_auth_end_to_end.sh PRE-CREATION
> src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh e1c12bbd4382c31e576439f6693d82d5661029b9
> 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
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Kunal Thakar <ku...@gmail.com>.
> 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
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/#review125991
-----------------------------------------------------------
This patch does not apply cleanly against master (ec29ac1), do you need to rebase?
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
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
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/#review126191
-----------------------------------------------------------
Ship it!
Thanks for sticking it out through the review, nice patch!
- Bill Farner
On March 30, 2016, 12:50 p.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:50 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 3a83961f5133fc0021b7c61f1117488703a9849d
> 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
>
> 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
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/#review126190
-----------------------------------------------------------
Ship it!
Master (bc4649e) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On March 30, 2016, 7:50 p.m., Kunal Thakar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45042/
> -----------------------------------------------------------
>
> (Updated March 30, 2016, 7:50 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 3a83961f5133fc0021b7c61f1117488703a9849d
> 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
>
> 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
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Kunal Thakar <ku...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/
-----------------------------------------------------------
(Updated March 30, 2016, 7:50 p.m.)
Review request for Aurora, Bill Farner and Zameer Manji.
Changes
-------
Updated documenation and announcer-auth.json
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 (updated)
-----
RELEASE-NOTES.md 3a83961f5133fc0021b7c61f1117488703a9849d
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
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
Re: Review Request 45042: Add ACL support for announcer
Posted by Kunal Thakar <ku...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
Pystachio'ed the config for easier validation/conversion, removed new e2e test and added auth to main e2e test
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 (updated)
-----
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
Re: Review Request 45042: Add ACL support for announcer
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/#review125781
-----------------------------------------------------------
examples/vagrant/announcer-auth.json (line 8)
<https://reviews.apache.org/r/45042/#comment188649>
nit: this really should be `acl` (it's a single Access Control List).
- Bill Farner
On March 28, 2016, 4:10 p.m., Kunal Thakar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45042/
> -----------------------------------------------------------
>
> (Updated March 28, 2016, 4:10 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-announcer-auth.conf PRE-CREATION
> 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
> src/test/sh/org/apache/aurora/e2e/test_announcer_auth_end_to_end.sh PRE-CREATION
> src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh e1c12bbd4382c31e576439f6693d82d5661029b9
> 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
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Bill Farner <wf...@apache.org>.
> On March 28, 2016, 4:46 p.m., Bill Farner wrote:
> > examples/vagrant/announcer-auth.json, line 11
> > <https://reviews.apache.org/r/45042/diff/5/?file=1317202#file1317202line11>
> >
> > I now have to backpedal on my advice to store the encrypted credentials here. Since our hand is forced to store plaintext for the auth section, we might as well make this part plaintext too. That leaves us with the burden of handling the digest step, but that shouldn't be too bad.
>
> Kunal Thakar wrote:
> I'd prefer to keep the burden on the configuration provider to keep it simple.
I'm still a -1 to that, but willing to be out-voted by Zameer.
In my opinion, requiring the user to configure the same data (passwords) in 2 different ways (encrypted and plaintext) introduces unnecessary burden and a class of misconfiguration that mere mortals should not be subjected to :-)
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/#review125773
-----------------------------------------------------------
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
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Kunal Thakar <ku...@gmail.com>.
> On March 28, 2016, 11:46 p.m., Bill Farner wrote:
> > examples/vagrant/announcer-auth.json, line 11
> > <https://reviews.apache.org/r/45042/diff/5/?file=1317202#file1317202line11>
> >
> > I now have to backpedal on my advice to store the encrypted credentials here. Since our hand is forced to store plaintext for the auth section, we might as well make this part plaintext too. That leaves us with the burden of handling the digest step, but that shouldn't be too bad.
>
> Kunal Thakar wrote:
> I'd prefer to keep the burden on the configuration provider to keep it simple.
>
> Bill Farner wrote:
> I'm still a -1 to that, but willing to be out-voted by Zameer.
>
> In my opinion, requiring the user to configure the same data (passwords) in 2 different ways (encrypted and plaintext) introduces unnecessary burden and a class of misconfiguration that mere mortals should not be subjected to :-)
Okay. I have special cased 'digest' scheme to generate the credential.
- Kunal
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/#review125773
-----------------------------------------------------------
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
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Kunal Thakar <ku...@gmail.com>.
> On March 28, 2016, 11:46 p.m., Bill Farner wrote:
> > examples/vagrant/announcer-auth.json, line 11
> > <https://reviews.apache.org/r/45042/diff/5/?file=1317202#file1317202line11>
> >
> > I now have to backpedal on my advice to store the encrypted credentials here. Since our hand is forced to store plaintext for the auth section, we might as well make this part plaintext too. That leaves us with the burden of handling the digest step, but that shouldn't be too bad.
I'd prefer to keep the burden on the configuration provider to keep it simple.
- Kunal
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/#review125773
-----------------------------------------------------------
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
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/#review125773
-----------------------------------------------------------
Great to see that you got it working in end-to-end tests! A few things left to clean up; we're very close.
examples/vagrant/announcer-auth.json (line 8)
<https://reviews.apache.org/r/45042/#comment188639>
Tying in with my comment towards the end - let's add 'world read' access here. I believe that would be represented as:
```
{
"scheme": "world",
"credential": "anyone",
"permissions": {
"read": true
}
}
```
examples/vagrant/announcer-auth.json (line 11)
<https://reviews.apache.org/r/45042/#comment188632>
I now have to backpedal on my advice to store the encrypted credentials here. Since our hand is forced to store plaintext for the auth section, we might as well make this part plaintext too. That leaves us with the burden of handling the digest step, but that shouldn't be too bad.
src/main/python/apache/aurora/executor/common/announcer.py (line 125)
<https://reviews.apache.org/r/45042/#comment188634>
A dict isn't a great substitute for a class. I'd rather see something like this, which contains all data/logic associated with the config parsing:
```python
class ZkAuthConfig(object):
def __init__(self, auths, acls):
self.auths = auths
self.acls = acls
@staticmethod
def from_file(zk_auth_config):
# make_zk_auth body
```
src/main/python/apache/aurora/executor/common/announcer.py (line 145)
<https://reviews.apache.org/r/45042/#comment188635>
Echoing past comment on truthiness - you almost certainly want `if zk_auth_config is None`
src/main/python/apache/aurora/executor/common/announcer.py (line 230)
<https://reviews.apache.org/r/45042/#comment188636>
Avoid mutable kwarg defaults, they will leave you in a fun multi-hour debug session one day!
Use `zk_auth=None` instead
Background here:
http://stackoverflow.com/questions/1132941/least-astonishment-in-python-the-mutable-default-argument
src/test/sh/org/apache/aurora/e2e/test_announcer_auth_end_to_end.sh (line 1)
<https://reviews.apache.org/r/45042/#comment188637>
I apologize if i gave the impression that another end-to-end test routine was needed, that was not the intent! I merely intended for the ZK announcer auth to be 'enabled' in the vagrant environment by default, all the time. I assume that is just a matter of a few bits of configuration, and little if anything else.
Feel free to nuke this file; i hope you at least learned something along the way!
src/test/sh/org/apache/aurora/e2e/validate_serverset.py (lines 31 - 60)
<https://reviews.apache.org/r/45042/#comment188638>
How about we simplify this and grant world-read access in `announcer-auth.json`? In addition to making the test easier, i think it represents a common real-world configuration.
- Bill Farner
On March 28, 2016, 4:10 p.m., Kunal Thakar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45042/
> -----------------------------------------------------------
>
> (Updated March 28, 2016, 4:10 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-announcer-auth.conf PRE-CREATION
> 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
> src/test/sh/org/apache/aurora/e2e/test_announcer_auth_end_to_end.sh PRE-CREATION
> src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh e1c12bbd4382c31e576439f6693d82d5661029b9
> 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
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Kunal Thakar <ku...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/
-----------------------------------------------------------
(Updated March 28, 2016, 11:10 p.m.)
Review request for Aurora, Bill Farner and Zameer Manji.
Changes
-------
rebase
Repository: aurora
Description (updated)
-------
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 (updated)
-----
RELEASE-NOTES.md 34f28a165aae4ae24fa95ef19b4972e088fd63a0
docs/operations/security.md 1a3d9b7e7ba4ec1952dc886d5fbeb6b85d994fb9
examples/vagrant/announcer-auth.json PRE-CREATION
examples/vagrant/upstart/aurora-scheduler-announcer-auth.conf PRE-CREATION
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
src/test/sh/org/apache/aurora/e2e/test_announcer_auth_end_to_end.sh PRE-CREATION
src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh e1c12bbd4382c31e576439f6693d82d5661029b9
src/test/sh/org/apache/aurora/e2e/validate_serverset.py fca1137bd2e7b1306a03dc2a54d2ef15b59af6a8
Diff: https://reviews.apache.org/r/45042/diff/
Testing (updated)
-------
/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
Re: Review Request 45042: Add ACL support for announcer
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/#review125769
-----------------------------------------------------------
This patch does not apply cleanly against master (f28f41a), do you need to rebase?
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On March 28, 2016, 10:51 p.m., Kunal Thakar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45042/
> -----------------------------------------------------------
>
> (Updated March 28, 2016, 10:51 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):
> ```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
> examples/vagrant/announcer-auth.json PRE-CREATION
> examples/vagrant/upstart/aurora-scheduler-announcer-auth.conf PRE-CREATION
> 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
> src/test/sh/org/apache/aurora/e2e/test_announcer_auth_end_to_end.sh PRE-CREATION
> src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh b469f9bbbdfbf98df947832411bd0cdce97affdc
> src/test/sh/org/apache/aurora/e2e/validate_serverset.py fca1137bd2e7b1306a03dc2a54d2ef15b59af6a8
>
> Diff: https://reviews.apache.org/r/45042/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kunal Thakar
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Kunal Thakar <ku...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/
-----------------------------------------------------------
(Updated March 28, 2016, 10:51 p.m.)
Review request for Aurora, Bill Farner and Zameer Manji.
Changes
-------
Thanks for the feedback, auth_data is indeed a required piece of information that is needed before setting ACLs. I have updated the config file structure as follows to accomodate authentication credentials in addition to the ACL objects:
```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>
}
}
]
}
```
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 (updated)
-----
RELEASE-NOTES.md 6e9364e34db6dbb270468db3ff333b956c6bf9f3
docs/security.md 32bea426fbceec83187e851a5db11e82df55e962
examples/vagrant/announcer-auth.json PRE-CREATION
examples/vagrant/upstart/aurora-scheduler-announcer-auth.conf PRE-CREATION
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
src/test/sh/org/apache/aurora/e2e/test_announcer_auth_end_to_end.sh PRE-CREATION
src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh b469f9bbbdfbf98df947832411bd0cdce97affdc
src/test/sh/org/apache/aurora/e2e/validate_serverset.py fca1137bd2e7b1306a03dc2a54d2ef15b59af6a8
Diff: https://reviews.apache.org/r/45042/diff/
Testing
-------
Thanks,
Kunal Thakar
Re: Review Request 45042: Add ACL support for announcer
Posted by Kunal Thakar <ku...@gmail.com>.
> On March 23, 2016, 2:23 a.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/bin/thermos_executor_main.py, line 107
> > <https://reviews.apache.org/r/45042/diff/4/?file=1310998#file1310998line107>
> >
> > Sorry for not catching this earlier - how about s/auth/acl/? I think this is more specific to what the file defines.
Kept it the same since we have more than ACLs in the file now.
> On March 23, 2016, 2:23 a.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/bin/thermos_executor_main.py, line 111
> > <https://reviews.apache.org/r/45042/diff/4/?file=1310998#file1310998line111>
> >
> > Is this more accurate? `Path to ZooKeeper ACL to use for announcer nodes.`
Done
> On March 23, 2016, 2:23 a.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/common/announcer.py, line 84
> > <https://reviews.apache.org/r/45042/diff/4/?file=1310999#file1310999line84>
> >
> > I think we should also require that at least one field is specified in `permissions`. Seems like the ACL entry would be meaningliess otherwise.
Done. Will it be an overkill to use something like jsonschema to validate this instead?
> On March 23, 2016, 2:23 a.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/common/announcer.py, lines 186-187
> > <https://reviews.apache.org/r/45042/diff/4/?file=1310999#file1310999line186>
> >
> > Feeling some deja vu here - the double underscore is unconventional - please change to single underscore.
There are plenty of double underscores throughout the file that predate my changes. I am following the single underscore for new code. Should I refactor the entire file?
> On March 23, 2016, 2:23 a.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/common/announcer.py, lines 95-96
> > <https://reviews.apache.org/r/45042/diff/4/?file=1310999#file1310999line95>
> >
> > 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
> > ```
Done
> On March 23, 2016, 2:23 a.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/common/announcer.py, line 192
> > <https://reviews.apache.org/r/45042/diff/4/?file=1310999#file1310999line192>
> >
> > 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 itself?
> >
> > 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`.
Great catch.
> On March 23, 2016, 2:23 a.m., Bill Farner wrote:
> > src/test/python/apache/aurora/executor/common/test_announcer.py, line 352
> > <https://reviews.apache.org/r/45042/diff/4/?file=1311001#file1311001line352>
> >
> > 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'`)?
Done
> On March 23, 2016, 2:23 a.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/common/announcer.py, line 193
> > <https://reviews.apache.org/r/45042/diff/4/?file=1310999#file1310999line193>
> >
> > 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(
> > self._ensemble,
> > connection_retry=self.DEFAULT_RETRY_POLICY,
> > default_acl=self._zk_acl)
> > ```
Done
> On March 23, 2016, 2:23 a.m., Bill Farner wrote:
> > docs/security.md, line 289
> > <https://reviews.apache.org/r/45042/diff/4/?file=1310997#file1310997line289>
> >
> > 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
> > [ACLs](https://zookeeper.apache.org/doc/current/zookeeperProgrammers.html#sc_ZooKeeperAccessControl),
> > 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:
> >
> > ```json
> > [
> > {
> > "scheme": "<scheme>",
> > "credential": "<credential>",
> > "permissions": {
> > "read": <bool>,
> > "write": <bool>,
> > "create": <bool>,
> > "delete": <bool>,
> > "admin": <bool>,
> > "all": <bool>
> > }
> > }
> > ]
> > ```
> >
> > The [scheme](http://zookeeper.apache.org/doc/r3.1.2/zookeeperProgrammers.html#sc_BuiltinACLSchemes)
> > 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 provided.
> > ```
>
> Bill Farner wrote:
> Formatting was broken above due to nested preformatted text, but it should be relatively close to being copy/paste-able.
Thanks for the rewrite. I have updated the documentation with your writeup.
- Kunal
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/#review124935
-----------------------------------------------------------
On March 28, 2016, 10:51 p.m., Kunal Thakar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45042/
> -----------------------------------------------------------
>
> (Updated March 28, 2016, 10:51 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):
> ```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
> examples/vagrant/announcer-auth.json PRE-CREATION
> examples/vagrant/upstart/aurora-scheduler-announcer-auth.conf PRE-CREATION
> 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
> src/test/sh/org/apache/aurora/e2e/test_announcer_auth_end_to_end.sh PRE-CREATION
> src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh b469f9bbbdfbf98df947832411bd0cdce97affdc
> src/test/sh/org/apache/aurora/e2e/validate_serverset.py fca1137bd2e7b1306a03dc2a54d2ef15b59af6a8
>
> Diff: https://reviews.apache.org/r/45042/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kunal Thakar
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Bill Farner <wf...@apache.org>.
> On March 22, 2016, 7:23 p.m., Bill Farner wrote:
> > docs/security.md, line 289
> > <https://reviews.apache.org/r/45042/diff/4/?file=1310997#file1310997line289>
> >
> > 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
> > [ACLs](https://zookeeper.apache.org/doc/current/zookeeperProgrammers.html#sc_ZooKeeperAccessControl),
> > 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:
> >
> > ```json
> > [
> > {
> > "scheme": "<scheme>",
> > "credential": "<credential>",
> > "permissions": {
> > "read": <bool>,
> > "write": <bool>,
> > "create": <bool>,
> > "delete": <bool>,
> > "admin": <bool>,
> > "all": <bool>
> > }
> > }
> > ]
> > ```
> >
> > The [scheme](http://zookeeper.apache.org/doc/r3.1.2/zookeeperProgrammers.html#sc_BuiltinACLSchemes)
> > 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 provided.
> > ```
Formatting was broken above due to nested preformatted text, but it should be relatively close to being copy/paste-able.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/#review124935
-----------------------------------------------------------
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
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Bill Farner <wf...@apache.org>.
> On March 22, 2016, 7:23 p.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/common/announcer.py, line 84
> > <https://reviews.apache.org/r/45042/diff/4/?file=1310999#file1310999line84>
> >
> > I think we should also require that at least one field is specified in `permissions`. Seems like the ACL entry would be meaningliess otherwise.
>
> Kunal Thakar wrote:
> Done. Will it be an overkill to use something like jsonschema to validate this instead?
>
> Bill Farner wrote:
> I'll let you assess that for the time being. I would not be opposed. You should also consider defining a schema in pystachio, however.
Here's the schema fleshed out in pystachio. I think this is how you should proceed.
```python
from apache.thermos.config.schema import (
Boolean,
Default,
List,
Required,
String,
Struct
)
class Auth(Struct):
scheme = Required(String)
credential = Required(String)
class Permissions(Struct):
read = Default(Boolean, False)
write = Default(Boolean, False)
create = Default(Boolean, False)
delete = Default(Boolean, False)
class Access(Struct):
scheme = Required(String)
credential = Required(String)
permissions = Required(Permissions)
class ZkAuth(Struct):
auth = Default(List(Auth), [])
acl = Default(List(Access), [])
```
If you're not familiar with pystachio, i suggest you drop into a repl to get a feel for the API:
`./pants -q repl src/main/python/apache/aurora/client`
>From there, you can paste the above classes and try your example schema:
```
>>> example = '''{
... "auth": [
... {
... "scheme": "digest",
... "credential": "user:pass"
... }
... ],
... "acl": [
... {
... "scheme": "digest",
... "credential": "user:smGaoVKd/cQkjm7b88GyorAUz20=",
... "permissions": {
... "read": true,
... "write": true,
... "create": true,
... "delete": true
... }
... }
... ]
... }'''
>>> a = ZkAuth.json_loads(example, strict=True)
>>> print a
ZkAuth(auth=AuthList(Auth(credential=user:pass,
scheme=digest)),
acl=AccessList(Access(credential=user:smGaoVKd/cQkjm7b88GyorAUz20=,
scheme=digest,
permissions=Permissions(read=True,
write=True,
create=True,
delete=True))))
>>> for auth in a.auth():
... print auth.credential()
...
user:pass
```
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/#review124935
-----------------------------------------------------------
On March 28, 2016, 4:10 p.m., Kunal Thakar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45042/
> -----------------------------------------------------------
>
> (Updated March 28, 2016, 4:10 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-announcer-auth.conf PRE-CREATION
> 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
> src/test/sh/org/apache/aurora/e2e/test_announcer_auth_end_to_end.sh PRE-CREATION
> src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh e1c12bbd4382c31e576439f6693d82d5661029b9
> 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
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Bill Farner <wf...@apache.org>.
> On March 22, 2016, 7:23 p.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/common/announcer.py, line 84
> > <https://reviews.apache.org/r/45042/diff/4/?file=1310999#file1310999line84>
> >
> > I think we should also require that at least one field is specified in `permissions`. Seems like the ACL entry would be meaningliess otherwise.
>
> Kunal Thakar wrote:
> Done. Will it be an overkill to use something like jsonschema to validate this instead?
I'll let you assess that for the time being. I would not be opposed. You should also consider defining a schema in pystachio, however.
> On March 22, 2016, 7:23 p.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/common/announcer.py, lines 186-187
> > <https://reviews.apache.org/r/45042/diff/4/?file=1310999#file1310999line186>
> >
> > Feeling some deja vu here - the double underscore is unconventional - please change to single underscore.
>
> Kunal Thakar wrote:
> There are plenty of double underscores throughout the file that predate my changes. I am following the single underscore for new code. Should I refactor the entire file?
Yes please do, it's nice when source files are at least internally consistent.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/#review124935
-----------------------------------------------------------
On March 28, 2016, 3:51 p.m., Kunal Thakar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45042/
> -----------------------------------------------------------
>
> (Updated March 28, 2016, 3:51 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):
> ```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
> examples/vagrant/announcer-auth.json PRE-CREATION
> examples/vagrant/upstart/aurora-scheduler-announcer-auth.conf PRE-CREATION
> 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
> src/test/sh/org/apache/aurora/e2e/test_announcer_auth_end_to_end.sh PRE-CREATION
> src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh b469f9bbbdfbf98df947832411bd0cdce97affdc
> src/test/sh/org/apache/aurora/e2e/validate_serverset.py fca1137bd2e7b1306a03dc2a54d2ef15b59af6a8
>
> Diff: https://reviews.apache.org/r/45042/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kunal Thakar
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/#review124935
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/45042/#comment187649>
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
[ACLs](https://zookeeper.apache.org/doc/current/zookeeperProgrammers.html#sc_ZooKeeperAccessControl),
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:
```json
[
{
"scheme": "<scheme>",
"credential": "<credential>",
"permissions": {
"read": <bool>,
"write": <bool>,
"create": <bool>,
"delete": <bool>,
"admin": <bool>,
"all": <bool>
}
}
]
```
The [scheme](http://zookeeper.apache.org/doc/r3.1.2/zookeeperProgrammers.html#sc_BuiltinACLSchemes)
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 provided.
```
src/main/python/apache/aurora/executor/bin/thermos_executor_main.py (line 107)
<https://reviews.apache.org/r/45042/#comment187650>
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)
<https://reviews.apache.org/r/45042/#comment187651>
Is this more accurate? `Path to ZooKeeper ACL to use for announcer nodes.`
src/main/python/apache/aurora/executor/common/announcer.py (line 84)
<https://reviews.apache.org/r/45042/#comment187654>
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)
<https://reviews.apache.org/r/45042/#comment187653>
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)
<https://reviews.apache.org/r/45042/#comment187658>
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)
<https://reviews.apache.org/r/45042/#comment187656>
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 itself?
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)
<https://reviews.apache.org/r/45042/#comment187657>
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(
self._ensemble,
connection_retry=self.DEFAULT_RETRY_POLICY,
default_acl=self._zk_acl)
```
src/test/python/apache/aurora/executor/common/test_announcer.py (line 352)
<https://reviews.apache.org/r/45042/#comment187655>
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
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Kunal Thakar <ku...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/
-----------------------------------------------------------
(Updated March 22, 2016, 6:51 p.m.)
Review request for Aurora, Bill Farner and Zameer Manji.
Changes
-------
Add release note and fix test. @ReviewBot retry
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 (updated)
-----
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
Re: Review Request 45042: Add ACL support for announcer
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/#review124841
-----------------------------------------------------------
Master (335cf88) is red with this patch.
./build-support/jenkins/build.sh
[1m mock_dump_runner_pex.return_value = Mock()[0m
[1m mock_options = Mock()[0m
[1m mock_options.execute_as_user = False[0m
[1m mock_options.nosetuid = False[0m
[1m with patch([0m
[1m 'apache.aurora.executor.bin.thermos_executor_main.dump_runner_pex',[0m
[1m return_value=mock_dump_runner_pex):[0m
[1m with patch([0m
[1m 'apache.aurora.executor.bin.thermos_executor_main.DefaultThermosTaskRunnerProvider',[0m
[1m return_value=mock_runner_provider) as mock_provider:[0m
[1m [0m
[1m expected_path = os.path.join(os.path.abspath('.'), MesosPathDetector.DEFAULT_SANDBOX_PATH)[0m
[1m> thermos_executor = initialize(mock_options)[0m
src/test/python/apache/aurora/executor/bin/test_thermos_executor_entry_point.py:44:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
.pants.d/python-setup/chroots/3acd03dbb81b37f2a14a0799a3e0bd882c60c91a/apache/aurora/executor/bin/thermos_executor_main.py:204: in initialize
[1m default_acl = make_default_acl(options.announcer_zookeeper_auth_config)[0m
.pants.d/python-setup/chroots/3acd03dbb81b37f2a14a0799a3e0bd882c60c91a/apache/aurora/executor/common/announcer.py:107: in make_default_acl
[1m auth_config = parse_config(zk_auth_config)[0m
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
config_file = <Mock name='mock.announcer_zookeeper_auth_config' id='140044375200848'>
[1m def parse_config(config_file):[0m
[1m try:[0m
[1m> with open(config_file) as fp:[0m
[1m[31mE TypeError: coercing to Unicode: need string or buffer, Mock found[0m
.pants.d/python-setup/chroots/3acd03dbb81b37f2a14a0799a3e0bd882c60c91a/apache/aurora/executor/common/announcer.py:92: TypeError
-------------- Captured stderr call --------------
WARNING:root:Please remove the deprecated and no-op --announcer-enable flag in scheduler config!
generated xml file: /home/jenkins/jenkins-slave/workspace/AuroraBot/dist/test-results/415337499eb72578eab327a6487c1f5c9452b3d6.xml
[1m[31m 1 failed, 667 passed, 5 skipped, 1 warnings in 438.62 seconds [0m
FAILURE
17:31:23 08:55 [complete][31m
FAILURE[0m
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On March 22, 2016, 5:02 p.m., Kunal Thakar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45042/
> -----------------------------------------------------------
>
> (Updated March 22, 2016, 5:02 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):
> ```json
> {
> "scheme": "<scheme>",
> "credential": "<credential>",
> "permissions": {
> "read": <bool>,
> "write": <bool>,
> "create": <bool>,
> "delete": <bool>,
> "admin": <bool>,
> "all": <bool>
> }
> }
> ```
>
>
> Diffs
> -----
>
> 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/common/test_announcer.py 142b58d5e577c9f4b8e2ae8473cffdea94eba21f
>
> Diff: https://reviews.apache.org/r/45042/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kunal Thakar
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Stephan Erb <se...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/#review124833
-----------------------------------------------------------
Drive-by review: Please also add an entry to the RELEASE-NOTES.md. Thanks! :-)
- Stephan Erb
On March 22, 2016, 6:02 p.m., Kunal Thakar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45042/
> -----------------------------------------------------------
>
> (Updated March 22, 2016, 6:02 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):
> ```json
> {
> "scheme": "<scheme>",
> "credential": "<credential>",
> "permissions": {
> "read": <bool>,
> "write": <bool>,
> "create": <bool>,
> "delete": <bool>,
> "admin": <bool>,
> "all": <bool>
> }
> }
> ```
>
>
> Diffs
> -----
>
> 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/common/test_announcer.py 142b58d5e577c9f4b8e2ae8473cffdea94eba21f
>
> Diff: https://reviews.apache.org/r/45042/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kunal Thakar
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Kunal Thakar <ku...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/
-----------------------------------------------------------
(Updated March 22, 2016, 5:02 p.m.)
Review request for Aurora, Bill Farner and Zameer Manji.
Changes
-------
Added documentation, switched to ACL lists and moved config parsing out of DefaultAnnouncerProviderChecker
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 (updated)
-----
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/common/test_announcer.py 142b58d5e577c9f4b8e2ae8473cffdea94eba21f
Diff: https://reviews.apache.org/r/45042/diff/
Testing
-------
Thanks,
Kunal Thakar
Re: Review Request 45042: Add ACL support for announcer
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/#review124334
-----------------------------------------------------------
Master (b24619b) is red with this patch.
./build-support/jenkins/build.sh
[1m proxy_driver = ProxyDriver()[0m
[1m with temporary_dir() as checkpoint_root:[0m
[1m te = AuroraExecutor([0m
[1m> runner_provider=make_provider(checkpoint_root),[0m
[1m sandbox_provider=DefaultTestSandboxProvider())[0m
src/test/python/apache/aurora/executor/test_thermos_executor.py:580:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
src/test/python/apache/aurora/executor/test_thermos_executor.py:193: in make_provider
[1m pex_location=thermos_runner_path(),[0m
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
build = True
[1m def thermos_runner_path(build=True):[0m
[1m if not build:[0m
[1m return getattr(thermos_runner_path, 'value', None)[0m
[1m [0m
[1m if not hasattr(thermos_runner_path, 'value'):[0m
[1m pex_dir = safe_mkdtemp()[0m
[1m> assert subprocess.call(["./pants", "--pants-distdir=%s" % pex_dir, "binary",[0m
[1m "src/main/python/apache/thermos/runner:thermos_runner"]) == 0[0m
[1m[31mE assert 1 == 0[0m
[1m[31mE + where 1 = <function call at 0x7f528a0c5b18>(['./pants', '--pants-distdir=/tmp/user/10021/tmpXl2PW0', 'binary', 'src/main/python/apache/thermos/runner:thermos_runner'])[0m
[1m[31mE + where <function call at 0x7f528a0c5b18> = subprocess.call[0m
src/test/python/apache/aurora/executor/test_thermos_executor.py:185: AssertionError
-------------- Captured stderr call --------------
Traceback (most recent call last):
File "/home/jenkins/.cache/pants/setup/bootstrap-Linux-x86_64/0.0.75/bin/pants", line 7, in <module>
from pants.bin.pants_exe import main
ImportError: No module named pants.bin.pants_exe
generated xml file: /home/jenkins/jenkins-slave/workspace/AuroraBot/dist/test-results/415337499eb72578eab327a6487c1f5c9452b3d6.xml
[1m[31m 16 failed, 643 passed, 5 skipped, 1 warnings, 8 error in 228.64 seconds [0m
FAILURE
00:17:28 04:44 [complete][31m
FAILURE[0m
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On March 18, 2016, 9:57 p.m., Kunal Thakar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45042/
> -----------------------------------------------------------
>
> (Updated March 18, 2016, 9:57 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):
> ```json
> {
> "scheme": "<scheme>",
> "credential": "<credential>",
> "permissions": {
> "read": <bool>,
> "write": <bool>,
> "create": <bool>,
> "delete": <bool>,
> "admin": <bool>,
> "all": <bool>
> }
> }
> ```
>
>
> Diffs
> -----
>
> 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/common/test_announcer.py 142b58d5e577c9f4b8e2ae8473cffdea94eba21f
>
> Diff: https://reviews.apache.org/r/45042/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kunal Thakar
>
>
Re: Review Request 45042: Add ACL support for announcer
Posted by Kunal Thakar <ku...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/
-----------------------------------------------------------
(Updated March 18, 2016, 9:57 p.m.)
Review request for Aurora, Bill Farner and Zameer Manji.
Changes
-------
Rebased
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 (updated)
-----
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/common/test_announcer.py 142b58d5e577c9f4b8e2ae8473cffdea94eba21f
Diff: https://reviews.apache.org/r/45042/diff/
Testing
-------
Thanks,
Kunal Thakar
Re: Review Request 45042: Add ACL support for announcer
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45042/#review124255
-----------------------------------------------------------
Ship it!
Master (12be6fb) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On March 18, 2016, 6:37 p.m., Kunal Thakar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45042/
> -----------------------------------------------------------
>
> (Updated March 18, 2016, 6:37 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):
> ```json
> {
> "scheme": "<scheme>",
> "credential": "<credential>",
> "permissions": {
> "read": <bool>,
> "write": <bool>,
> "create": <bool>,
> "delete": <bool>,
> "admin": <bool>,
> "all": <bool>
> }
> }
> ```
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/bin/thermos_executor_main.py f82858c528808d2a9e77bb56f16e897cfb5bbe73
> src/main/python/apache/aurora/executor/common/announcer.py 34e36e0a59093468a8934f58bacb68512949347c
> src/test/python/apache/aurora/executor/common/test_announcer.py f4032c7302f4733ab5670322b1905102c200f1c9
>
> Diff: https://reviews.apache.org/r/45042/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kunal Thakar
>
>