aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bill Farner <wfar...@apache.org>
Subject Re: Review Request 45042: Add ACL support for announcer
Date Mon, 28 Mar 2016 23:46:51 GMT

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



Great to see that you got it working in end-to-end tests!  A few things left to clean up;
we're very close.


examples/vagrant/announcer-auth.json (line 8)
<https://reviews.apache.org/r/45042/#comment188639>

    Tying in with my comment towards the end - let's add 'world read' access here.  I believe
that would be represented as:
    
    ```
    {
      "scheme": "world",
      "credential": "anyone",
      "permissions": {
        "read": true
      }
    }
    ```



examples/vagrant/announcer-auth.json (line 11)
<https://reviews.apache.org/r/45042/#comment188632>

    I now have to backpedal on my advice to store the encrypted credentials here.  Since our
hand is forced to store plaintext for the auth section, we might as well make this part plaintext
too.  That leaves us with the burden of handling the digest step, but that shouldn't be too
bad.



src/main/python/apache/aurora/executor/common/announcer.py (line 125)
<https://reviews.apache.org/r/45042/#comment188634>

    A dict isn't a great substitute for a class.  I'd rather see something like this, which
contains all data/logic associated with the config parsing:
    
    ```python
    class ZkAuthConfig(object):
      def __init__(self, auths, acls):
        self.auths = auths
        self.acls = acls
    
      @staticmethod
      def from_file(zk_auth_config):
        # make_zk_auth body
    ```



src/main/python/apache/aurora/executor/common/announcer.py (line 145)
<https://reviews.apache.org/r/45042/#comment188635>

    Echoing past comment on truthiness - you almost certainly want `if zk_auth_config is None`



src/main/python/apache/aurora/executor/common/announcer.py (line 230)
<https://reviews.apache.org/r/45042/#comment188636>

    Avoid mutable kwarg defaults, they will leave you in a fun multi-hour debug session one
day!
    
    Use `zk_auth=None` instead
    
    Background here:
    http://stackoverflow.com/questions/1132941/least-astonishment-in-python-the-mutable-default-argument



src/test/sh/org/apache/aurora/e2e/test_announcer_auth_end_to_end.sh (line 1)
<https://reviews.apache.org/r/45042/#comment188637>

    I apologize if i gave the impression that another end-to-end test routine was needed,
that was not the intent!  I merely intended for the ZK announcer auth to be 'enabled' in the
vagrant environment by default, all the time.  I assume that is just a matter of a few bits
of configuration, and little if anything else.
    
    Feel free to nuke this file; i hope you at least learned something along the way!



src/test/sh/org/apache/aurora/e2e/validate_serverset.py (lines 31 - 60)
<https://reviews.apache.org/r/45042/#comment188638>

    How about we simplify this and grant world-read access in `announcer-auth.json`?  In addition
to making the test easier, i think it represents a common real-world configuration.


- Bill Farner


On March 28, 2016, 4:10 p.m., Kunal Thakar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45042/
> -----------------------------------------------------------
> 
> (Updated March 28, 2016, 4:10 p.m.)
> 
> 
> Review request for Aurora, Bill Farner and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Add ACL support for announcer
> https://issues.apache.org/jira/browse/AURORA-1643
> 
> Adding support for service discovery ZK authentication. ZK authentication secrets should
be stored in a file as json (as follows):
> (Updated JSON format for config file)
> ```json
> {
>   "auth": [
>     {
>       "scheme": "<scheme>",
>       "credential": "<plain_credential>"
>     }
>   ],
>   "acls": [
>     {
>       "scheme": "<scheme>",
>       "credential": "<encrypted_credential>",
>       "permissions": {
>         "read": <bool>,
>         "write": <bool>,
>         "create": <bool>,
>         "delete": <bool>,
>         "admin": <bool>,
>         "all": <bool>
>       }
>     }
>   ]
> }
> ```
> 
> 
> Diffs
> -----
> 
>   RELEASE-NOTES.md 34f28a165aae4ae24fa95ef19b4972e088fd63a0 
>   docs/operations/security.md 1a3d9b7e7ba4ec1952dc886d5fbeb6b85d994fb9 
>   examples/vagrant/announcer-auth.json PRE-CREATION 
>   examples/vagrant/upstart/aurora-scheduler-announcer-auth.conf PRE-CREATION 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 6634506108c346f8c23b2da7cc8d20d09d07d590

>   src/main/python/apache/aurora/executor/common/announcer.py 79a9cfb6ac3a8444f09fb3658e6e859e06941ba4

>   src/test/python/apache/aurora/executor/bin/test_thermos_executor_entry_point.py e9f7851292aef3a36da5da9b0fc333a7e7750cf3

>   src/test/python/apache/aurora/executor/common/test_announcer.py 142b58d5e577c9f4b8e2ae8473cffdea94eba21f

>   src/test/sh/org/apache/aurora/e2e/test_announcer_auth_end_to_end.sh PRE-CREATION 
>   src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh e1c12bbd4382c31e576439f6693d82d5661029b9

>   src/test/sh/org/apache/aurora/e2e/validate_serverset.py fca1137bd2e7b1306a03dc2a54d2ef15b59af6a8

> 
> Diff: https://reviews.apache.org/r/45042/diff/
> 
> 
> Testing
> -------
> 
> /vagrant/src/test/sh/org/apache/aurora/e2e/test_announcer_auth_end_to_end.sh
> /vagrant/src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Kunal Thakar
> 
>


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