Re: Review Request 68104: Implemented authorization for agent `GET_RESOURCE_PROVIDER` calls.

2018-08-01 Thread Mesos Reviewbot Windows 

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



PASS: Mesos patch 68104 was successfully built and tested.

Reviews applied: `['68103', '68104']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2019/mesos-review-68104

- Mesos Reviewbot Windows


On Aug. 1, 2018, 8:18 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68104/
> ---
> 
> (Updated Aug. 1, 2018, 8:18 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/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 68104: Implemented authorization for agent `GET_RESOURCE_PROVIDER` calls.

2018-08-01 Thread Benjamin Bannier 


> On July 31, 2018, 5:38 a.m., Chun-Hung Hsiao wrote:
> > src/tests/api_tests.cpp
> > Lines 7002-7004 (original), 7002-7021 (patched)
> > 
> >
> > 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> slave = StartSlave(, slaveFlags);
> > ```
> 
> Benjamin Bannier wrote:
> Great suggestion, applied.
> 
> I did not set `authenticate_http_readwrite` though since it is not 
> required.
> 
> Chun-Hung Hsiao wrote:
> Are you sure about this? The test won't pass if I don't turn this flag on:
> ```
> [ RUN  ] ContentType/AgentAPITest.GetResourceProviders/0
> ../src/tests/api_tests.cpp:7091: Failure
>   Expected: 0
> To be equal to: 
> v1Response->get_resource_providers().resource_providers_size()
>   Which is: 1
> [  FAILED  ] ContentType/AgentAPITest.GetResourceProviders/0, where 
> GetParam() = application/x-protobuf (192 ms)
> [ RUN  ] ContentType/AgentAPITest.GetResourceProviders/1
> ../src/tests/api_tests.cpp:7091: Failure
>   Expected: 0
> To be equal to: 
> v1Response->get_resource_providers().resource_providers_size()
>   Which is: 1
> [  FAILED  ] ContentType/AgentAPITest.GetResourceProviders/1, where 
> GetParam() = application/json (164 ms)
> ```
> 
> Also, we don't need to set up the ACL for `DEFAULT_CREDENTIAL` since the 
> permissive mode is turned on by default.

This only worked when I manually created the authorizer, but not anymore if it 
is constructed implicitly via `cluster::StartSlave`. Should be fixed now.


- Benjamin


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


On Aug. 1, 2018, 10:18 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68104/
> ---
> 
> (Updated Aug. 1, 2018, 10:18 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/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 68104: Implemented authorization for agent `GET_RESOURCE_PROVIDER` calls.

2018-08-01 Thread Benjamin Bannier 

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

(Updated Aug. 1, 2018, 10:18 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 (updated)
-

  src/slave/http.cpp ab5864d9fd2fde478ed7da2ca7ed8abedc72c7c5 
  src/tests/api_tests.cpp 182622a62d350ebefc891a385de3f2d35a7c0243 


Diff: https://reviews.apache.org/r/68104/diff/3/

Changes: https://reviews.apache.org/r/68104/diff/2-3/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 68104: Implemented authorization for agent `GET_RESOURCE_PROVIDER` calls.

2018-07-31 Thread Chun-Hung Hsiao 

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


Ship it!





src/slave/http.cpp
Lines 1831-1833 (patched)


It seems we can hoist this outside the loop. However we might want to 
consider having finer-grained ACLs in the future so I'm fine with keeping it as 
is.


- Chun-Hung Hsiao


On July 31, 2018, 10:19 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68104/
> ---
> 
> (Updated July 31, 2018, 10:19 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/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 68104: Implemented authorization for agent `GET_RESOURCE_PROVIDER` calls.

2018-07-31 Thread Mesos Reviewbot Windows 

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['68103', '68104']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2010/mesos-review-68104

Relevant logs:

- 
[mesos-tests-stdout.log](http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2010/mesos-review-68104/logs/mesos-tests-stdout.log):

```
[--] 9 tests from Endpoint/SlaveEndpointTest (929 ms total)

[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0 (32 
ms)
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1 (37 
ms)
[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest (72 ms 
total)

[--] 1 test from IsolationFlag/CpuIsolatorTest
[ RUN  ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0
[   OK ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0 (625 ms)
[--] 1 test from IsolationFlag/CpuIsolatorTest (648 ms total)

[--] 1 test from IsolationFlag/MemoryIsolatorTest
[ RUN  ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0
[   OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (680 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (706 ms total)

[--] Global test environment tear-down
[==] 1013 tests from 98 test cases ran. (511266 ms total)
[  PASSED  ] 1011 tests.
[  FAILED  ] 2 tests, listed below:
[  FAILED  ] ContentType/AgentAPITest.GetResourceProviders/0, where GetParam() 
= application/x-protobuf
[  FAILED  ] ContentType/AgentAPITest.GetResourceProviders/1, where GetParam() 
= application/json

 2 FAILED TESTS
  YOU HAVE 222 DISABLED TESTS

```

- 
[mesos-tests-stderr.log](http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2010/mesos-review-68104/logs/mesos-tests-stderr.log):

```
I0731 11:21:49.415030 50408 master.cpp:10917] Updating the state of task 
007f0173-d0d3-4f8a-a6ae-5c6b17b727a8 of framework 
1b290b3b-4469-448d-8fca-f9840778b748- (latest state: TASK_KILLED, status 
update state: TASK_KILLED)
I0731 11:21:49.415030 44552 slave.cpp:3939] Shutting down framework 
1b290b3b-4469-448d-8fca-f9840778b748-
I0731 11:21:49.415693 44552 slave.cpp:6658] Shutting down executor 
'007f0173-d0d3-4f8a-a6ae-5c6b17b727a8' of framework 
1b290b3b-4469-448d-8fca-f9840778b748- at executor(1)@192.10.1.6:49239
I0731 11:21:49.417690 44552 slave.cpp:931] Agent terminating
W0731 11:21:49.417690 44552 slave.cpp:3935] Ignoring shutdown framework 
1b290b3b-4469-448d-8fca-f9840778b748- because it is terminating
I0731 11:21:49.417690 50408 master.cpp:11016] Removing task 
007f0173-d0d3-4f8a-a6ae-5c6b17b727a8 with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework 1b290b3b-4469-448d-8fca-f9840778b748- on 
agent 1I0731 11:21:49.247696 48276 exec.cpp:162] Version: 1.7.0
I0731 11:21:49.273716 50344 exec.cpp:236] Executor registered on agent 
1b290b3b-4469-448d-8fca-f9840778b748-S0
I0731 11:21:49.276705 50884 executor.cpp:182] Received SUBSCRIBED event
I0731 11:21:49.281693 50884 executor.cpp:186] Subscribed executor on 
windows-02.enofukwu14ruplxn0gs3yzmsgf.xx.internal.cloudapp.net
I0731 11:21:49.281693 50884 executor.cpp:182] Received LAUNCH event
I0731 11:21:49.286708 50884 executor.cpp:679] Starting task 
007f0173-d0d3-4f8a-a6ae-5c6b17b727a8
I0731 11:21:49.368700 50884 executor.cpp:499] Running 
'D:\DCOS\mesos\src\mesos-containerizer.exe launch '
I0731 11:21:49.376683 50884 executor.cpp:693] Forked command at 46460
I0731 11:21:49.417690 50276 exec.cpp:445] Executor asked to shutdown
I0731 11:21:49.418694 50884 executor.cpp:182] Received SHUTDOWN event
I0731 11:21:49.418694 50884 executor.cpp:796] Shutting down
I0731 11:21:49.418694 50884 executor.cpp:909] Sending SIGTERM to process tree 
at pid 46b290b3b-4469-448d-8fca-f9840778b748-S0 at slave(462)@192.10.1.6:63931 
(windows-02.enofukwu14ruplxn0gs3yzmsgf.xx.internal.cloudapp.net)
I0731 11:21:49.421685 50408 master.cpp:1330] Agent 
1b290b3b-4469-448d-8fca-f9840778b748-S0 at slave(462)@192.10.1.6:63931 
(windows-02.enofukwu14ruplxn0gs3yzmsgf.xx.internal.cloudapp.net) disconnected
I0731 11:21:49.421685 50408 master.cpp:3340] Disconnecting agent 
1b290b3b-4469-448d-8fca-f9840778b748-S0 at slave(462)@192.10.1.6:63931 
(windows-02.enofukwu14ruplxn0gs3yzmsgf.xx.internal.cloudapp.net)
I0731 11:21:49.421685 49432 hierarchical.cpp:345] Removed

Re: Review Request 68104: Implemented authorization for agent `GET_RESOURCE_PROVIDER` calls.

2018-07-31 Thread Benjamin Bannier 

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

(Updated July 31, 2018, 12:19 p.m.)


Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.


Changes
---

Addressed comments from Chun.


Bugs: MESOS-8314
https://issues.apache.org/jira/browse/MESOS-8314


Repository: mesos


Description
---

Implemented authorization for agent `GET_RESOURCE_PROVIDER` calls.


Diffs (updated)
-

  src/slave/http.cpp ab5864d9fd2fde478ed7da2ca7ed8abedc72c7c5 
  src/tests/api_tests.cpp 182622a62d350ebefc891a385de3f2d35a7c0243 


Diff: https://reviews.apache.org/r/68104/diff/2/

Changes: https://reviews.apache.org/r/68104/diff/1-2/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 68104: Implemented authorization for agent `GET_RESOURCE_PROVIDER` calls.

2018-07-31 Thread Benjamin Bannier 


> On July 31, 2018, 5:38 a.m., Chun-Hung Hsiao wrote:
> > src/slave/http.cpp
> > Lines 1818 (patched)
> > 
> >
> > Conventionally we only indent this by 4 extra spaces aligning with 
> > "return".

Good catch, I filed https://issues.apache.org/jira/browse/MESOS-9121.


> On July 31, 2018, 5:38 a.m., Chun-Hung Hsiao wrote:
> > src/slave/http.cpp
> > Lines 1821-1822 (patched)
> > 
> >
> > The following can be fit into 80 characters:
> > ```
> > [this, acceptType](const Owned& approvers) -> 
> > Response {
> > ```
> > Or for here it seems harmless to just use `[=]`. I'm fine with either 
> > though.

That line seems to be exactly 81 characters long, so it looks like we do need 
to split somewhere. I now manually adjusted this to not split the capture list 
(the previous version was formated by our `clang-format`).

I don't think that implicitly capturing pointers to not immediately invoked 
lambdas is safe style, much less when capturing `this` like here. I'll keep the 
explicit capture.


> On July 31, 2018, 5:38 a.m., Chun-Hung Hsiao wrote:
> > src/slave/http.cpp
> > Lines 1824 (patched)
> > 
> >
> > Should we return an empty list of resource provider infos or return a 
> > 403 Forbidden?

Good point, changed to return an empty set when not authorized. This maps well 
on e.g., specifying sets of RPs somebody can view in the local authorizer.


> On July 31, 2018, 5:38 a.m., Chun-Hung Hsiao wrote:
> > src/tests/api_tests.cpp
> > Lines 7002-7004 (original), 7002-7021 (patched)
> > 
> >
> > 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> slave = StartSlave(, slaveFlags);
> > ```

Great suggestion, applied.

I did not set `authenticate_http_readwrite` though since it is not required.


> On July 31, 2018, 5:38 a.m., Chun-Hung Hsiao wrote:
> > src/tests/api_tests.cpp
> > Lines 7046-7049 (patched)
> > 
> >
> > I'm a bit against checking content of the failure string since it's 
> > slightly hard to maintain. Can we avoid this?

I removed this check for now as knowning that the request failed already tell 
us _something_.


- Benjamin


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


On July 31, 2018, 12:19 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68104/
> ---
> 
> (Updated July 31, 2018, 12:19 p.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/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 68104: Implemented authorization for agent `GET_RESOURCE_PROVIDER` calls.

2018-07-30 Thread Chun-Hung Hsiao 

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




src/slave/http.cpp
Lines 1818 (patched)


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



src/slave/http.cpp
Lines 1821-1822 (patched)


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



src/slave/http.cpp
Lines 1824 (patched)


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)


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> slave = StartSlave(, slaveFlags);
```



src/tests/api_tests.cpp
Lines 7046-7049 (patched)


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