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

Kept it the same since we have more than ACLs in the file now.


> On March 23, 2016, 2:23 a.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/bin/thermos_executor_main.py, line 
> > 111
> > <https://reviews.apache.org/r/45042/diff/4/?file=1310998#file1310998line111>
> >
> >     Is this more accurate? `Path to ZooKeeper ACL to use for announcer 
> > nodes.`

Done


> On March 23, 2016, 2:23 a.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/common/announcer.py, line 84
> > <https://reviews.apache.org/r/45042/diff/4/?file=1310999#file1310999line84>
> >
> >     I think we should also require that at least one field is specified in 
> > `permissions`.  Seems like the ACL entry would be meaningliess otherwise.

Done. Will it be an overkill to use something like jsonschema to validate this 
instead?


> On March 23, 2016, 2:23 a.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/common/announcer.py, lines 186-187
> > <https://reviews.apache.org/r/45042/diff/4/?file=1310999#file1310999line186>
> >
> >     Feeling some deja vu here - the double underscore is unconventional - 
> > please change to single underscore.

There are plenty of double underscores throughout the file that predate my 
changes. I am following the single underscore for new code. Should I refactor 
the entire file?


> On March 23, 2016, 2:23 a.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/common/announcer.py, lines 95-96
> > <https://reviews.apache.org/r/45042/diff/4/?file=1310999#file1310999line95>
> >
> >     I suggest you just have `validate` raise for invalid and catch here, 
> > rather than returning a `bool`.  The sequence here creates two separate log 
> > entries that could just as easily be 1, e.g.
> >     
> >     ```
> >     Expecting a list of ACL objects
> >     ZK authentication config is invalid
> >     ```

Done


> On March 23, 2016, 2:23 a.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/common/announcer.py, line 192
> > <https://reviews.apache.org/r/45042/diff/4/?file=1310999#file1310999line192>
> >
> >     I just realized something - does it make much sense to specify 
> > `default_acl` without also specifying `auth_data`?
> >     
> >     ```
> >     :param auth_data:
> >       A list of authentication credentials to use for the
> >       connection. Should be a list of (scheme, credential)
> >       tuples as :meth:`add_auth` takes.
> >     ```
> >     
> >     Worse yet - can the announcer lock itself out if it _doesn't_ 
> > authenticate itself?
> >     
> >     Enabling this feature in the default vagrant environment will be 
> > informative, and may guide our approach.  But i do think that either way it 
> > makes sense to follow up with plumbing for `auth_data`.

Great catch.


> On March 23, 2016, 2:23 a.m., Bill Farner wrote:
> > src/test/python/apache/aurora/executor/common/test_announcer.py, line 352
> > <https://reviews.apache.org/r/45042/diff/4/?file=1311001#file1311001line352>
> >
> >     I assume this string is effectively a blob for this test.  If so, can 
> > you make it explicit by using a fake value (e.g. `'fake'`)?

Done


> On March 23, 2016, 2:23 a.m., Bill Farner wrote:
> > src/main/python/apache/aurora/executor/common/announcer.py, line 193
> > <https://reviews.apache.org/r/45042/diff/4/?file=1310999#file1310999line193>
> >
> >     Formatting nit - when a method signature or method call needs to wrap, 
> > please wrap to put each argument on its own line.  In this case:
> >     
> >     ```
> >     return KazooClient(
> >         self._ensemble,
> >         connection_retry=self.DEFAULT_RETRY_POLICY,
> >         default_acl=self._zk_acl)
> >     ```

Done


> On March 23, 2016, 2:23 a.m., Bill Farner wrote:
> > docs/security.md, line 289
> > <https://reviews.apache.org/r/45042/diff/4/?file=1310997#file1310997line289>
> >
> >     I'd like to propose several changes to this section, which i've made in 
> > the rewritten block below.
> >     
> >     - Use consistent naming and proper nouns for projects (Thermos, 
> > ZooKeeper)
> >     - Link to latest version of ZooKeeper docs
> >     - Immediately link to relevant ZooKeeper ACL section
> >     - Describe how to enable the feature before describing the format of 
> > the ACL file
> >     - Use more accurate requirements level terminology, e.g. 
> > must/may/should (context reading http://tools.ietf.org/html/rfc2119)
> >     
> >     ```
> >     # Announcer Authentication
> >     Nodes created by the Thermos executor may include ZooKeeper
> >     
> > [ACLs](https://zookeeper.apache.org/doc/current/zookeeperProgrammers.html#sc_ZooKeeperAccessControl),
> >     which will specify the priviliges of clients to perform different 
> > actions on these nodes.  This
> >     feature is enabled by specifying an ACL configuration file to the 
> > executor with the
> >     `--announcer-zookeeper-auth-config` command line argument.
> >     
> >     When this feature is _not_ enabled, nodes created by the executor will 
> > have 'world/all' permission
> >     (`ZOO_OPEN_ACL_UNSAFE`).  In most production environments, operators 
> > should specify ACLs and
> >     limit access.
> >     
> >     ## ACL configuration format
> >     The configuration file must be formatted as JSON with the following 
> > schema:
> >     
> >     ```json
> >     [
> >       {
> >         "scheme": "<scheme>",
> >         "credential": "<credential>",
> >         "permissions": {
> >           "read": <bool>,
> >           "write": <bool>,
> >           "create": <bool>,
> >           "delete": <bool>,
> >           "admin": <bool>,
> >           "all": <bool>
> >         }
> >       }
> >     ]
> >     ```
> >     
> >     The 
> > [scheme](http://zookeeper.apache.org/doc/r3.1.2/zookeeperProgrammers.html#sc_BuiltinACLSchemes)
> >     defines the encoding of the `credential` field.  Note that these fields 
> > are passed directly to
> >     ZoooKeeper.  If a scheme is used that requires credential hashing, the 
> > value of the `credential`
> >     field must be hashed (i.e. the executor will not hash this value).
> >     
> >     All properties of the `permissions` object will default to `False` if 
> > not provided.
> >     ```
> 
> Bill Farner wrote:
>     Formatting was broken above due to nested preformatted text, but it 
> should be relatively close to being copy/paste-able.

Thanks for the rewrite. I have updated the documentation with your writeup.


- Kunal


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


On March 28, 2016, 10:51 p.m., Kunal Thakar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45042/
> -----------------------------------------------------------
> 
> (Updated March 28, 2016, 10:51 p.m.)
> 
> 
> Review request for Aurora, Bill Farner and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Add ACL support for announcer
> https://issues.apache.org/jira/browse/AURORA-1643
> 
> Adding support for service discovery ZK authentication. ZK authentication 
> secrets should be stored in a file as json (as follows):
> ```json
> {
>   "scheme": "<scheme>",
>   "credential": "<credential>",
>       "permissions": {
>         "read": <bool>,
>         "write": <bool>,
>         "create": <bool>,
>         "delete": <bool>,
>         "admin": <bool>,
>         "all": <bool>
>       }
> }
> ```
> 
> 
> Diffs
> -----
> 
>   RELEASE-NOTES.md 6e9364e34db6dbb270468db3ff333b956c6bf9f3 
>   docs/security.md 32bea426fbceec83187e851a5db11e82df55e962 
>   examples/vagrant/announcer-auth.json PRE-CREATION 
>   examples/vagrant/upstart/aurora-scheduler-announcer-auth.conf PRE-CREATION 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 
> 6634506108c346f8c23b2da7cc8d20d09d07d590 
>   src/main/python/apache/aurora/executor/common/announcer.py 
> 79a9cfb6ac3a8444f09fb3658e6e859e06941ba4 
>   
> src/test/python/apache/aurora/executor/bin/test_thermos_executor_entry_point.py
>  e9f7851292aef3a36da5da9b0fc333a7e7750cf3 
>   src/test/python/apache/aurora/executor/common/test_announcer.py 
> 142b58d5e577c9f4b8e2ae8473cffdea94eba21f 
>   src/test/sh/org/apache/aurora/e2e/test_announcer_auth_end_to_end.sh 
> PRE-CREATION 
>   src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 
> b469f9bbbdfbf98df947832411bd0cdce97affdc 
>   src/test/sh/org/apache/aurora/e2e/validate_serverset.py 
> fca1137bd2e7b1306a03dc2a54d2ef15b59af6a8 
> 
> Diff: https://reviews.apache.org/r/45042/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Kunal Thakar
> 
>

Reply via email to