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