Re: Review Request 45042: Add ACL support for announcer

2016-03-30 Thread Bill Farner

---
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": "",
>   "credential": ""
> }
>   ],
>   "acls": [
> {
>   "scheme": "",
>   "credential": "",
>   "permissions": {
> "read": ,
> "write": ,
> "create": ,
> "delete": ,
> "admin": ,
> "all": 
>   }
> }
>   ]
> }
> ```
> 
> 
> 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

2016-03-30 Thread Aurora ReviewBot

---
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": "",
>   "credential": ""
> }
>   ],
>   "acls": [
> {
>   "scheme": "",
>   "credential": "",
>   "permissions": {
> "read": ,
> "write": ,
> "create": ,
> "delete": ,
> "admin": ,
> "all": 
>   }
> }
>   ]
> }
> ```
> 
> 
> 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

2016-03-30 Thread Kunal Thakar

---
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": "",
  "credential": ""
}
  ],
  "acls": [
{
  "scheme": "",
  "credential": "",
  "permissions": {
"read": ,
"write": ,
"create": ,
"delete": ,
"admin": ,
"all": 
  }
}
  ]
}
```


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

2016-03-30 Thread Kunal Thakar


> On March 28, 2016, 11:46 p.m., Bill Farner wrote:
> > examples/vagrant/announcer-auth.json, line 11
> > 
> >
> > 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": "",
>   "credential": ""
> }
>   ],
>   "acls": [
> {
>   "scheme": "",
>   "credential": "",
>   "permissions": {
> "read": ,
> "write": ,
> "create": ,
> "delete": ,
> "admin": ,
> "all": 
>   }
> }
>   ]
> }
> ```
> 
> 
> 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

2016-03-30 Thread Kunal Thakar


> On March 30, 2016, 2:20 a.m., Bill Farner wrote:
> > RELEASE-NOTES.md, line 14
> > 
> >
> > 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
> > 
> >
> > s/Zookeeper/ZooKeeper/ (capital K)

Done


> On March 30, 2016, 2:20 a.m., Bill Farner wrote:
> > docs/operations/security.md, line 34
> > 
> >
> > "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
> > 
> >
> > "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
> > 
> >
> > s/ACL/an ACL/

Done


> On March 30, 2016, 2:20 a.m., Bill Farner wrote:
> > examples/vagrant/announcer-auth.json, line 1
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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": "",
>   "credential": ""
> }
>   ],
>   "acls": [
> {
>   "scheme": "",
>   "credential": "",
>   "permissions": {
> "read": ,
> "write": ,
> "create": ,
>

Re: Review Request 45042: Add ACL support for announcer

2016-03-29 Thread Bill Farner

---
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)


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



docs/operations/security.md (line 26)


s/Zookeeper/ZooKeeper/ (capital K)



docs/operations/security.md (line 34)


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



docs/operations/security.md (line 292)


"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)


s/ACL/an ACL/



examples/vagrant/announcer-auth.json (line 1)


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)


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



src/main/python/apache/aurora/executor/common/announcer.py (lines 65 - 70)


Nice!



src/main/python/apache/aurora/executor/common/announcer.py (lines 154 - 164)


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)


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)


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": "",
>   "credential": ""
> }
>   ],
>   "acls": [
> {
>   "scheme": "",
>   "credential": "",
>   "permissions": {
> "read": ,
> "write": ,
> "create": ,
> "delete": ,
> "admin": ,
> "all": 
>   }
> }
>   ]
> }
> ```
> 
> 
> 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 
> 

Re: Review Request 45042: Add ACL support for announcer

2016-03-29 Thread Bill Farner


> On March 28, 2016, 4:46 p.m., Bill Farner wrote:
> > examples/vagrant/announcer-auth.json, line 11
> > 
> >
> > 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": "",
>   "credential": ""
> }
>   ],
>   "acls": [
> {
>   "scheme": "",
>   "credential": "",
>   "permissions": {
> "read": ,
> "write": ,
> "create": ,
> "delete": ,
> "admin": ,
> "all": 
>   }
> }
>   ]
> }
> ```
> 
> 
> 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

2016-03-29 Thread Aurora ReviewBot

---
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": "",
>   "credential": ""
> }
>   ],
>   "acls": [
> {
>   "scheme": "",
>   "credential": "",
>   "permissions": {
> "read": ,
> "write": ,
> "create": ,
> "delete": ,
> "admin": ,
> "all": 
>   }
> }
>   ]
> }
> ```
> 
> 
> 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

2016-03-29 Thread Kunal Thakar


> On March 28, 2016, 11:46 p.m., Bill Farner wrote:
> > examples/vagrant/announcer-auth.json, line 11
> > 
> >
> > 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": "",
>   "credential": ""
> }
>   ],
>   "acls": [
> {
>   "scheme": "",
>   "credential": "",
>   "permissions": {
> "read": ,
> "write": ,
> "create": ,
> "delete": ,
> "admin": ,
> "all": 
>   }
> }
>   ]
> }
> ```
> 
> 
> 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

2016-03-29 Thread Kunal Thakar

---
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": "",
  "credential": ""
}
  ],
  "acls": [
{
  "scheme": "",
  "credential": "",
  "permissions": {
"read": ,
"write": ,
"create": ,
"delete": ,
"admin": ,
"all": 
  }
}
  ]
}
```


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

2016-03-28 Thread Bill Farner


> On March 22, 2016, 7:23 p.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/common/announcer.py, line 84
> > 
> >
> > I think we should also require that at least one field is specified in 
> > `permissions`.  Seems like the ACL entry would be meaningliess otherwise.
> 
> 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": "",
>   "credential": ""
> }
>   ],
>   "acls": [
> {
>   "scheme": "",
>   "credential": "",
>   "permissions": {
> "read": ,
> "write": ,
> "create": ,
> "delete": ,
> "admin": ,
> "all": 
>   }
> }
>   ]
> }
> ```
> 
> 
> 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
> 

Re: Review Request 45042: Add ACL support for announcer

2016-03-28 Thread Bill Farner

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




examples/vagrant/announcer-auth.json (line 8)


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": "",
>   "credential": ""
> }
>   ],
>   "acls": [
> {
>   "scheme": "",
>   "credential": "",
>   "permissions": {
> "read": ,
> "write": ,
> "create": ,
> "delete": ,
> "admin": ,
> "all": 
>   }
> }
>   ]
> }
> ```
> 
> 
> 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

2016-03-28 Thread Bill Farner

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




RELEASE-NOTES.md (line 15)


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": "",
>   "credential": ""
> }
>   ],
>   "acls": [
> {
>   "scheme": "",
>   "credential": "",
>   "permissions": {
> "read": ,
> "write": ,
> "create": ,
> "delete": ,
> "admin": ,
> "all": 
>   }
> }
>   ]
> }
> ```
> 
> 
> 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

2016-03-28 Thread Bill Farner

---
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)


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)


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)


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)


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)


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)


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)


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": "",
>   "credential": ""
> }
>   ],
>   "acls": [
> {
>   "scheme": "",
>   "credential": "",
>   "permissions": {
> "read": ,
> "write": ,
> "create": ,
> "delete": ,
> "admin": ,
> "all": 
>   }
> }
>   ]
> }
> ```
> 
> 
> 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 
>   

Re: Review Request 45042: Add ACL support for announcer

2016-03-28 Thread Aurora ReviewBot

---
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": "",
>   "credential": ""
> }
>   ],
>   "acls": [
> {
>   "scheme": "",
>   "credential": "",
>   "permissions": {
> "read": ,
> "write": ,
> "create": ,
> "delete": ,
> "admin": ,
> "all": 
>   }
> }
>   ]
> }
> ```
> 
> 
> 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

2016-03-28 Thread Kunal Thakar

---
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": "",
  "credential": ""
}
  ],
  "acls": [
{
  "scheme": "",
  "credential": "",
  "permissions": {
"read": ,
"write": ,
"create": ,
"delete": ,
"admin": ,
"all": 
  }
}
  ]
}
```


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

2016-03-28 Thread Bill Farner


> On March 22, 2016, 7:23 p.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/common/announcer.py, line 84
> > 
> >
> > I think we should also require that at least one field is specified in 
> > `permissions`.  Seems like the ACL entry would be meaningliess otherwise.
> 
> 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
> > 
> >
> > 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": "",
>   "credential": "",
>   "permissions": {
> "read": ,
> "write": ,
> "create": ,
> "delete": ,
> "admin": ,
> "all": 
>   }
> }
> ```
> 
> 
> 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

2016-03-28 Thread Aurora ReviewBot

---
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": "",
>   "credential": "",
>   "permissions": {
> "read": ,
> "write": ,
> "create": ,
> "delete": ,
> "admin": ,
> "all": 
>   }
> }
> ```
> 
> 
> 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

2016-03-28 Thread Kunal Thakar


> On March 23, 2016, 2:23 a.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/bin/thermos_executor_main.py, line 
> > 107
> > 
> >
> > Sorry for not catching this earlier - how about s/auth/acl/?  I think 
> > this is more specific to what the file defines.

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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> > 

Re: Review Request 45042: Add ACL support for announcer

2016-03-28 Thread Kunal Thakar

---
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": "",
  "credential": ""
}
  ],
  "acls": [
{
  "scheme": "",
  "credential": "",
  "permissions": {
"read": ,
"write": ,
"create": ,
"delete": ,
"admin": ,
"all": 
  }
}
  ]
}
```


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": "",
  "credential": "",
"permissions": {
  "read": ,
  "write": ,
  "create": ,
  "delete": ,
  "admin": ,
  "all": 
}
}
```


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

2016-03-22 Thread Bill Farner


> On March 22, 2016, 7:23 p.m., Bill Farner wrote:
> > docs/security.md, line 289
> > 
> >
> > I'd like to propose several changes to this section, which i've made in 
> > the rewritten block below.
> > 
> > - Use consistent naming and proper nouns for projects (Thermos, 
> > ZooKeeper)
> > - Link to latest version of ZooKeeper docs
> > - Immediately link to relevant ZooKeeper ACL section
> > - Describe how to enable the feature before describing the format of 
> > the ACL file
> > - Use more accurate requirements level terminology, e.g. 
> > must/may/should (context reading http://tools.ietf.org/html/rfc2119)
> > 
> > ```
> > # Announcer Authentication
> > Nodes created by the Thermos executor may include ZooKeeper
> > 
> > [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": "",
> > "credential": "",
> > "permissions": {
> >   "read": ,
> >   "write": ,
> >   "create": ,
> >   "delete": ,
> >   "admin": ,
> >   "all": 
> > }
> >   }
> > ]
> > ```
> > 
> > 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": "",
>   "credential": "",
>   "permissions": {
> "read": ,
> "write": ,
> "create": ,
> "delete": ,
> "admin": ,
> "all": 
>   }
> }
> ```
> 
> 
> 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

2016-03-22 Thread Bill Farner

---
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)


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": "",
"credential": "",
"permissions": {
  "read": ,
  "write": ,
  "create": ,
  "delete": ,
  "admin": ,
  "all": 
}
  }
]
```

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)


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



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


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



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


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



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


I suggest you just have `validate` raise for invalid and catch here, rather 
than returning a `bool`.  The sequence here creates two separate log entries 
that could just as easily be 1, e.g.

```
Expecting a list of ACL objects
ZK authentication config is invalid
```



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


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



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


I just realized something - does it make much sense to specify 
`default_acl` without also specifying `auth_data`?

```
:param auth_data:
  A list of authentication credentials to use for the
  connection. Should be a list of (scheme, credential)
  tuples as 

Re: Review Request 45042: Add ACL support for announcer

2016-03-22 Thread Aurora ReviewBot

---
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": "",
>   "credential": "",
>   "permissions": {
> "read": ,
> "write": ,
> "create": ,
> "delete": ,
> "admin": ,
> "all": 
>   }
> }
> ```
> 
> 
> 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

2016-03-22 Thread Kunal Thakar

---
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": "",
  "credential": "",
"permissions": {
  "read": ,
  "write": ,
  "create": ,
  "delete": ,
  "admin": ,
  "all": 
}
}
```


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

2016-03-22 Thread Aurora ReviewBot

---
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

   mock_dump_runner_pex.return_value = Mock()
   mock_options = Mock()
   mock_options.execute_as_user = False
   mock_options.nosetuid = False
   with patch(
   
'apache.aurora.executor.bin.thermos_executor_main.dump_runner_pex',
   return_value=mock_dump_runner_pex):
 with patch(
 
'apache.aurora.executor.bin.thermos_executor_main.DefaultThermosTaskRunnerProvider',
 return_value=mock_runner_provider) as 
mock_provider:
 
   expected_path = 
os.path.join(os.path.abspath('.'), MesosPathDetector.DEFAULT_SANDBOX_PATH)
 > thermos_executor = 
initialize(mock_options)
 
 
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
 default_acl = 
make_default_acl(options.announcer_zookeeper_auth_config)
 
.pants.d/python-setup/chroots/3acd03dbb81b37f2a14a0799a3e0bd882c60c91a/apache/aurora/executor/common/announcer.py:107:
 in make_default_acl
 auth_config = parse_config(zk_auth_config)
 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
 
 config_file = 
 
 def parse_config(config_file):
   try:
 >   with open(config_file) as fp:
 E   TypeError: coercing to Unicode: need 
string or buffer, Mock found
 
 
.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
 
  1 failed, 667 passed, 5 skipped, 1 warnings in 
438.62 seconds 
 
FAILURE


17:31:23 08:55   [complete]
   FAILURE


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": "",
>   "credential": "",
>   "permissions": {
> "read": ,
> "write": ,
> "create": ,
> "delete": ,
> "admin": ,
> "all": 
>   }
> }
> ```
> 
> 
> 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

2016-03-22 Thread Zameer Manji

---
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": "",
>   "credential": "",
>   "permissions": {
> "read": ,
> "write": ,
> "create": ,
> "delete": ,
> "admin": ,
> "all": 
>   }
> }
> ```
> 
> 
> 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

2016-03-22 Thread Stephan Erb

---
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": "",
>   "credential": "",
>   "permissions": {
> "read": ,
> "write": ,
> "create": ,
> "delete": ,
> "admin": ,
> "all": 
>   }
> }
> ```
> 
> 
> 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

2016-03-21 Thread Zameer Manji


> On March 19, 2016, 6:46 p.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/common/announcer.py, line 153
> > 
> >
> > -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": "",
>   "credential": "",
>   "permissions": {
> "read": ,
> "write": ,
> "create": ,
> "delete": ,
> "admin": ,
> "all": 
>   }
> }
> ```
> 
> 
> 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

2016-03-21 Thread Zameer Manji


> On March 19, 2016, 6:46 p.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/common/announcer.py, line 128
> > 
> >
> > 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": "",
>   "credential": "",
>   "permissions": {
> "read": ,
> "write": ,
> "create": ,
> "delete": ,
> "admin": ,
> "all": 
>   }
> }
> ```
> 
> 
> 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

2016-03-20 Thread Aurora ReviewBot

---
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": "",
>   "credential": "",
>   "permissions": {
> "read": ,
> "write": ,
> "create": ,
> "delete": ,
> "admin": ,
> "all": 
>   }
> }
> ```
> 
> 
> 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

2016-03-19 Thread Bill Farner

---
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)


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)


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)


-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)


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": "",
>   "credential": "",
>   "permissions": {
> "read": ,
> "write": ,
> "create": ,
> "delete": ,
> "admin": ,
> "all": 
>   }
> }
> ```
> 
> 
> 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

2016-03-19 Thread Aurora ReviewBot

---
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

   proxy_driver = ProxyDriver()
   with temporary_dir() as checkpoint_root:
 te = AuroraExecutor(
 >   
runner_provider=make_provider(checkpoint_root),
 
sandbox_provider=DefaultTestSandboxProvider())
 
 
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
 pex_location=thermos_runner_path(),
 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
 
 build = True
 
 def thermos_runner_path(build=True):
   if not build:
 return getattr(thermos_runner_path, 'value', 
None)
 
   if not hasattr(thermos_runner_path, 'value'):
 pex_dir = safe_mkdtemp()
 >   assert subprocess.call(["./pants", 
"--pants-distdir=%s" % pex_dir, "binary",
   
"src/main/python/apache/thermos/runner:thermos_runner"]) == 0
 E   assert 1 == 0
 E+  where 1 = (['./pants', '--pants-distdir=/tmp/user/10021/tmpXl2PW0', 
'binary', 'src/main/python/apache/thermos/runner:thermos_runner'])
 E+where  = subprocess.call
 
 
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 
 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
 
  16 failed, 643 passed, 5 skipped, 1 warnings, 8 
error in 228.64 seconds 
 
FAILURE


00:17:28 04:44   [complete]
   FAILURE


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": "",
>   "credential": "",
>   "permissions": {
> "read": ,
> "write": ,
> "create": ,
> "delete": ,
> "admin": ,
> "all": 
>   }
> }
> ```
> 
> 
> 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

2016-03-18 Thread Kunal Thakar

---
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": "",
  "credential": "",
"permissions": {
  "read": ,
  "write": ,
  "create": ,
  "delete": ,
  "admin": ,
  "all": 
}
}
```


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