mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chun-Hung Hsiao <chhs...@apache.org>
Subject Re: Review Request 68104: Implemented authorization for agent `GET_RESOURCE_PROVIDER` calls.
Date Tue, 31 Jul 2018 03:38:45 GMT

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




src/slave/http.cpp
Lines 1818 (patched)
<https://reviews.apache.org/r/68104/#comment289695>

    Conventionally we only indent this by 4 extra spaces aligning with "return".



src/slave/http.cpp
Lines 1821-1822 (patched)
<https://reviews.apache.org/r/68104/#comment289696>

    The following can be fit into 80 characters:
    ```
            [this, acceptType](const Owned<ObjectApprover>& approvers) -> Response
{
    ```
    Or for here it seems harmless to just use `[=]`. I'm fine with either though.



src/slave/http.cpp
Lines 1824 (patched)
<https://reviews.apache.org/r/68104/#comment289698>

    Should we return an empty list of resource provider infos or return a 403 Forbidden?



src/tests/api_tests.cpp
Lines 7002-7004 (original), 7002-7021 (patched)
<https://reviews.apache.org/r/68104/#comment289697>

    How about the following:
    ```
    slave::Flags slaveFlags = CreateSlaveFlags();
    slaveFlags.authenticate_http_readwrite = true;
    
    {
      // `DEFAULT_CREDENTIAL_2` is not allowed to view any resource provider.
      mesos::ACL::ViewResourceProvider* acl =
        slaveFlags.acls->add_view_resource_providers();
      acl->mutable_principals()->add_values(DEFAULT_CREDENTIAL_2.principal());
      acl->mutable_resource_providers()->set_type(mesos::ACL::Entity::NONE);
    {
    
    Try<Owned<cluster::Slave>> slave = StartSlave(&detector, slaveFlags);
    ```



src/tests/api_tests.cpp
Lines 7046-7049 (patched)
<https://reviews.apache.org/r/68104/#comment289699>

    I'm a bit against checking content of the failure string since it's slightly hard to maintain.
Can we avoid this?


- Chun-Hung Hsiao


On July 30, 2018, 8:57 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68104/
> -----------------------------------------------------------
> 
> (Updated July 30, 2018, 8:57 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Bugs: MESOS-8314
>     https://issues.apache.org/jira/browse/MESOS-8314
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented authorization for agent `GET_RESOURCE_PROVIDER` calls.
> 
> 
> Diffs
> -----
> 
>   src/slave/http.cpp ab5864d9fd2fde478ed7da2ca7ed8abedc72c7c5 
>   src/tests/api_tests.cpp 182622a62d350ebefc891a385de3f2d35a7c0243 
> 
> 
> Diff: https://reviews.apache.org/r/68104/diff/1/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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