aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kunal Thakar <kun...@gmail.com>
Subject Re: Review Request 45042: Add ACL support for announcer
Date Mon, 28 Mar 2016 22:51:58 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message