Re: Review Request 47530: Added authorization to agent's '/containers' endpoint.

2016-05-19 Thread Alexander Rukletsov 

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


Fix it, then Ship it!




I'll take care of the outstanding issues and commit this for you in a jiffy.


src/slave/http.cpp (line 769)


This looks fishy. I understand why you do this, but we should resolve 
https://issues.apache.org/jira/browse/MESOS-5293 ASAP



src/slave/http.cpp (lines 781 - 785)


Why do you need to move `.repair` here?



src/slave/slave.hpp (line 23)


We don't need it here any more. Move it back to cpp.


- Alexander Rukletsov


On May 18, 2016, 8 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47530/
> ---
> 
> (Updated May 18, 2016, 8 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, Jan 
> Schlicht, and Till Toenshoff.
> 
> 
> Bugs: MESOS-5317
> https://issues.apache.org/jira/browse/MESOS-5317
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used GET_ENDPOINT_WITH_PATH coarse-grained authz on agent's 
> '/containers' endpoint to enable authorization on this endpoint.
> Updated docs and testcases as well.
> 
> 
> Diffs
> -
> 
>   docs/endpoints/slave/containers.md 959f40b9db4de4b6cea456ecf7bcb402f7a94f05 
>   src/slave/http.cpp fb48ec61e2fe0c83f80d3b8aa4c2ef5a96b748ae 
>   src/slave/slave.hpp 209f071448e3c52d16d3366d564003ee36b1d2e0 
>   src/tests/slave_authorization_tests.cpp 
> 843cf1c631e0a25125ca1c0c0028ad1a920c2c2f 
> 
> Diff: https://reviews.apache.org/r/47530/diff/
> 
> 
> Testing
> ---
> 
> ## Unit tests.
> 
> On ubuntu 16.04:
> `sudo GTEST_FILTER="*SlaveEndpointTest*.*" make -j2 check`
> 
> ## Manual testing.
> 
> 1. Ran master with:
> ```
> sudo  ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
> ```
> 
> 2. ACL File: 
> ```
>   {
> "get_endpoints": [
>   {
> "principals": { "type": "NONE" },
> "paths": { "values": ["/flags", "/monitor/statistics", "/containers"] 
> }
>   }
> ]
>   } 
> ```
> 
> 3. Ran slave with: 
> ```
> sudo ./bin/mesos-slave.sh --master=127.0.0.1:5050 --ip=0.0.0.0 
> --acls=file:///home/abhishek/testAcl
> ```
> 
> 4. Ran toy-framework with: 
> ```
> sudo ./no-executor-framework --master=master@127.0.0.1:5050 --command="echo 
> hello"
> ```
> 
> 5. Output while hitting "http://127.0.0.1:5051/slave(1)/containers" - HTTP 
> error 403: Forbidden
> 
> 6. Changed ACL to: 
> ```
>   {
> "get_endpoints": [
>   {
> "principals": { "type": "ANY" },
> "paths": { "values": ["/flags", "/monitor/statistics", "/containers"] 
> }
>   }
> ]
>   }
> ```
> 
> 7. Ran slave and framework again.
> 
> 8. Output:
> ```
> 
> [{"container_id":"9b8a6a51-68be-4763-9c7d-b67e85fccb4a","executor_id":"42","executor_name":"Command
>  Executor (Task: 42) (Command: sh -c 'echo hello')","framework_id":"52...
> ```
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 47530: Added authorization to agent's '/containers' endpoint.

2016-05-19 Thread Jan Schlicht 

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


Fix it, then Ship it!





src/slave/http.cpp 


The `#include ` will need to be re-added when you remove the one in 
`slave/slave.hpp` (see issue below)



src/slave/http.cpp (lines 781 - 786)


This `repair` is probaly in a better place if it's moved to the end of 
`_containers`, similar to how it was before, so it'll stay
```
return process::http::OK(result, request.url.query.get("jsonp"));
  })
  .repair([](const Future& future) {
LOG(WARNING) << "Could not collect container status and statistics: 
"
 << (future.isFailed() ? future.failure() : 
"discarded");

return process::http::InternalServerError();
  });
}
```



src/slave/slave.hpp (line 23)


I think this can be removed.


- Jan Schlicht


On May 18, 2016, 10 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47530/
> ---
> 
> (Updated May 18, 2016, 10 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, Jan 
> Schlicht, and Till Toenshoff.
> 
> 
> Bugs: MESOS-5317
> https://issues.apache.org/jira/browse/MESOS-5317
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used GET_ENDPOINT_WITH_PATH coarse-grained authz on agent's 
> '/containers' endpoint to enable authorization on this endpoint.
> Updated docs and testcases as well.
> 
> 
> Diffs
> -
> 
>   docs/endpoints/slave/containers.md 959f40b9db4de4b6cea456ecf7bcb402f7a94f05 
>   src/slave/http.cpp fb48ec61e2fe0c83f80d3b8aa4c2ef5a96b748ae 
>   src/slave/slave.hpp 209f071448e3c52d16d3366d564003ee36b1d2e0 
>   src/tests/slave_authorization_tests.cpp 
> 843cf1c631e0a25125ca1c0c0028ad1a920c2c2f 
> 
> Diff: https://reviews.apache.org/r/47530/diff/
> 
> 
> Testing
> ---
> 
> ## Unit tests.
> 
> On ubuntu 16.04:
> `sudo GTEST_FILTER="*SlaveEndpointTest*.*" make -j2 check`
> 
> ## Manual testing.
> 
> 1. Ran master with:
> ```
> sudo  ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
> ```
> 
> 2. ACL File: 
> ```
>   {
> "get_endpoints": [
>   {
> "principals": { "type": "NONE" },
> "paths": { "values": ["/flags", "/monitor/statistics", "/containers"] 
> }
>   }
> ]
>   } 
> ```
> 
> 3. Ran slave with: 
> ```
> sudo ./bin/mesos-slave.sh --master=127.0.0.1:5050 --ip=0.0.0.0 
> --acls=file:///home/abhishek/testAcl
> ```
> 
> 4. Ran toy-framework with: 
> ```
> sudo ./no-executor-framework --master=master@127.0.0.1:5050 --command="echo 
> hello"
> ```
> 
> 5. Output while hitting "http://127.0.0.1:5051/slave(1)/containers" - HTTP 
> error 403: Forbidden
> 
> 6. Changed ACL to: 
> ```
>   {
> "get_endpoints": [
>   {
> "principals": { "type": "ANY" },
> "paths": { "values": ["/flags", "/monitor/statistics", "/containers"] 
> }
>   }
> ]
>   }
> ```
> 
> 7. Ran slave and framework again.
> 
> 8. Output:
> ```
> 
> [{"container_id":"9b8a6a51-68be-4763-9c7d-b67e85fccb4a","executor_id":"42","executor_name":"Command
>  Executor (Task: 42) (Command: sh -c 'echo hello')","framework_id":"52...
> ```
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 47530: Added authorization to agent's '/containers' endpoint.

2016-05-18 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [47530]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On May 18, 2016, 8 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47530/
> ---
> 
> (Updated May 18, 2016, 8 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, Jan 
> Schlicht, and Till Toenshoff.
> 
> 
> Bugs: MESOS-5317
> https://issues.apache.org/jira/browse/MESOS-5317
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used GET_ENDPOINT_WITH_PATH coarse-grained authz on agent's 
> '/containers' endpoint to enable authorization on this endpoint.
> Updated docs and testcases as well.
> 
> 
> Diffs
> -
> 
>   docs/endpoints/slave/containers.md 959f40b9db4de4b6cea456ecf7bcb402f7a94f05 
>   src/slave/http.cpp fb48ec61e2fe0c83f80d3b8aa4c2ef5a96b748ae 
>   src/slave/slave.hpp 209f071448e3c52d16d3366d564003ee36b1d2e0 
>   src/tests/slave_authorization_tests.cpp 
> 843cf1c631e0a25125ca1c0c0028ad1a920c2c2f 
> 
> Diff: https://reviews.apache.org/r/47530/diff/
> 
> 
> Testing
> ---
> 
> ## Unit tests.
> 
> On ubuntu 16.04:
> `sudo GTEST_FILTER="*SlaveEndpointTest*.*" make -j2 check`
> 
> ## Manual testing.
> 
> 1. Ran master with:
> ```
> sudo  ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
> ```
> 
> 2. ACL File: 
> ```
>   {
> "get_endpoints": [
>   {
> "principals": { "type": "NONE" },
> "paths": { "values": ["/flags", "/monitor/statistics", "/containers"] 
> }
>   }
> ]
>   } 
> ```
> 
> 3. Ran slave with: 
> ```
> sudo ./bin/mesos-slave.sh --master=127.0.0.1:5050 --ip=0.0.0.0 
> --acls=file:///home/abhishek/testAcl
> ```
> 
> 4. Ran toy-framework with: 
> ```
> sudo ./no-executor-framework --master=master@127.0.0.1:5050 --command="echo 
> hello"
> ```
> 
> 5. Output while hitting "http://127.0.0.1:5051/slave(1)/containers" - HTTP 
> error 403: Forbidden
> 
> 6. Changed ACL to: 
> ```
>   {
> "get_endpoints": [
>   {
> "principals": { "type": "ANY" },
> "paths": { "values": ["/flags", "/monitor/statistics", "/containers"] 
> }
>   }
> ]
>   }
> ```
> 
> 7. Ran slave and framework again.
> 
> 8. Output:
> ```
> 
> [{"container_id":"9b8a6a51-68be-4763-9c7d-b67e85fccb4a","executor_id":"42","executor_name":"Command
>  Executor (Task: 42) (Command: sh -c 'echo hello')","framework_id":"52...
> ```
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 47530: Added authorization to agent's '/containers' endpoint.

2016-05-18 Thread Abhishek Dasgupta 

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

(Updated May 18, 2016, 8 p.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, Jan Schlicht, 
and Till Toenshoff.


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


Repository: mesos


Description (updated)
---

Used GET_ENDPOINT_WITH_PATH coarse-grained authz on agent's 
'/containers' endpoint to enable authorization on this endpoint.
Updated docs and testcases as well.


Diffs
-

  docs/endpoints/slave/containers.md 959f40b9db4de4b6cea456ecf7bcb402f7a94f05 
  src/slave/http.cpp fb48ec61e2fe0c83f80d3b8aa4c2ef5a96b748ae 
  src/slave/slave.hpp 209f071448e3c52d16d3366d564003ee36b1d2e0 
  src/tests/slave_authorization_tests.cpp 
843cf1c631e0a25125ca1c0c0028ad1a920c2c2f 

Diff: https://reviews.apache.org/r/47530/diff/


Testing
---

## Unit tests.

On ubuntu 16.04:
`sudo GTEST_FILTER="*SlaveEndpointTest*.*" make -j2 check`

## Manual testing.

1. Ran master with:
```
sudo  ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
```

2. ACL File: 
```
  {
"get_endpoints": [
  {
"principals": { "type": "NONE" },
"paths": { "values": ["/flags", "/monitor/statistics", "/containers"] }
  }
]
  } 
```

3. Ran slave with: 
```
sudo ./bin/mesos-slave.sh --master=127.0.0.1:5050 --ip=0.0.0.0 
--acls=file:///home/abhishek/testAcl
```

4. Ran toy-framework with: 
```
sudo ./no-executor-framework --master=master@127.0.0.1:5050 --command="echo 
hello"
```

5. Output while hitting "http://127.0.0.1:5051/slave(1)/containers" - HTTP 
error 403: Forbidden

6. Changed ACL to: 
```
  {
"get_endpoints": [
  {
"principals": { "type": "ANY" },
"paths": { "values": ["/flags", "/monitor/statistics", "/containers"] }
  }
]
  }
```

7. Ran slave and framework again.

8. Output:
```

[{"container_id":"9b8a6a51-68be-4763-9c7d-b67e85fccb4a","executor_id":"42","executor_name":"Command
 Executor (Task: 42) (Command: sh -c 'echo hello')","framework_id":"52...
```


Thanks,

Abhishek Dasgupta

Re: Review Request 47530: Added authorization to agent's '/containers' endpoint.

2016-05-18 Thread Abhishek Dasgupta 

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

(Updated May 18, 2016, 7:59 p.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, Jan Schlicht, 
and Till Toenshoff.


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


Repository: mesos


Description (updated)
---

Added authorization to agent's '/containers' endpoint.


Diffs (updated)
-

  docs/endpoints/slave/containers.md 959f40b9db4de4b6cea456ecf7bcb402f7a94f05 
  src/slave/http.cpp fb48ec61e2fe0c83f80d3b8aa4c2ef5a96b748ae 
  src/slave/slave.hpp 209f071448e3c52d16d3366d564003ee36b1d2e0 
  src/tests/slave_authorization_tests.cpp 
843cf1c631e0a25125ca1c0c0028ad1a920c2c2f 

Diff: https://reviews.apache.org/r/47530/diff/


Testing
---

## Unit tests.

On ubuntu 16.04:
`sudo GTEST_FILTER="*SlaveEndpointTest*.*" make -j2 check`

## Manual testing.

1. Ran master with:
```
sudo  ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
```

2. ACL File: 
```
  {
"get_endpoints": [
  {
"principals": { "type": "NONE" },
"paths": { "values": ["/flags", "/monitor/statistics", "/containers"] }
  }
]
  } 
```

3. Ran slave with: 
```
sudo ./bin/mesos-slave.sh --master=127.0.0.1:5050 --ip=0.0.0.0 
--acls=file:///home/abhishek/testAcl
```

4. Ran toy-framework with: 
```
sudo ./no-executor-framework --master=master@127.0.0.1:5050 --command="echo 
hello"
```

5. Output while hitting "http://127.0.0.1:5051/slave(1)/containers" - HTTP 
error 403: Forbidden

6. Changed ACL to: 
```
  {
"get_endpoints": [
  {
"principals": { "type": "ANY" },
"paths": { "values": ["/flags", "/monitor/statistics", "/containers"] }
  }
]
  }
```

7. Ran slave and framework again.

8. Output:
```

[{"container_id":"9b8a6a51-68be-4763-9c7d-b67e85fccb4a","executor_id":"42","executor_name":"Command
 Executor (Task: 42) (Command: sh -c 'echo hello')","framework_id":"52...
```


Thanks,

Abhishek Dasgupta

Re: Review Request 47530: Added authorization to agent's '/containers' endpoint.

2016-05-18 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [47530]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On May 18, 2016, 12:59 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47530/
> ---
> 
> (Updated May 18, 2016, 12:59 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, Jan 
> Schlicht, and Till Toenshoff.
> 
> 
> Bugs: MESOS-5317
> https://issues.apache.org/jira/browse/MESOS-5317
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used GET_ENDPOINT_WITH_PATH coarse-grained authz on agent's 
>  '/containers' endpoint to enable authorization on this endpoint.
>  Updated docs and testcases as well.
> 
> 
> Diffs
> -
> 
>   docs/endpoints/slave/containers.md 959f40b9db4de4b6cea456ecf7bcb402f7a94f05 
>   src/slave/http.cpp fb48ec61e2fe0c83f80d3b8aa4c2ef5a96b748ae 
>   src/slave/slave.hpp 209f071448e3c52d16d3366d564003ee36b1d2e0 
>   src/tests/slave_authorization_tests.cpp 
> 843cf1c631e0a25125ca1c0c0028ad1a920c2c2f 
> 
> Diff: https://reviews.apache.org/r/47530/diff/
> 
> 
> Testing
> ---
> 
> ## Unit tests.
> 
> On ubuntu 16.04:
> `sudo GTEST_FILTER="*SlaveEndpointTest*.*" make -j2 check`
> 
> ## Manual testing.
> 
> 1. Ran master with:
> ```
> sudo  ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
> ```
> 
> 2. ACL File: 
> ```
>   {
> "get_endpoints": [
>   {
> "principals": { "type": "NONE" },
> "paths": { "values": ["/flags", "/monitor/statistics", "/containers"] 
> }
>   }
> ]
>   } 
> ```
> 
> 3. Ran slave with: 
> ```
> sudo ./bin/mesos-slave.sh --master=127.0.0.1:5050 --ip=0.0.0.0 
> --acls=file:///home/abhishek/testAcl
> ```
> 
> 4. Ran toy-framework with: 
> ```
> sudo ./no-executor-framework --master=master@127.0.0.1:5050 --command="echo 
> hello"
> ```
> 
> 5. Output while hitting "http://127.0.0.1:5051/slave(1)/containers" - HTTP 
> error 403: Forbidden
> 
> 6. Changed ACL to: 
> ```
>   {
> "get_endpoints": [
>   {
> "principals": { "type": "ANY" },
> "paths": { "values": ["/flags", "/monitor/statistics", "/containers"] 
> }
>   }
> ]
>   }
> ```
> 
> 7. Ran slave and framework again.
> 
> 8. Output:
> ```
> 
> [{"container_id":"9b8a6a51-68be-4763-9c7d-b67e85fccb4a","executor_id":"42","executor_name":"Command
>  Executor (Task: 42) (Command: sh -c 'echo hello')","framework_id":"52...
> ```
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 47530: Added authorization to agent's '/containers' endpoint.

2016-05-18 Thread Alexander Rukletsov 

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




src/slave/http.cpp (line 788)


4 spaces indent, please


- Alexander Rukletsov


On May 18, 2016, 12:59 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47530/
> ---
> 
> (Updated May 18, 2016, 12:59 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, Jan 
> Schlicht, and Till Toenshoff.
> 
> 
> Bugs: MESOS-5317
> https://issues.apache.org/jira/browse/MESOS-5317
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used GET_ENDPOINT_WITH_PATH coarse-grained authz on agent's 
>  '/containers' endpoint to enable authorization on this endpoint.
>  Updated docs and testcases as well.
> 
> 
> Diffs
> -
> 
>   docs/endpoints/slave/containers.md 959f40b9db4de4b6cea456ecf7bcb402f7a94f05 
>   src/slave/http.cpp fb48ec61e2fe0c83f80d3b8aa4c2ef5a96b748ae 
>   src/slave/slave.hpp 209f071448e3c52d16d3366d564003ee36b1d2e0 
>   src/tests/slave_authorization_tests.cpp 
> 843cf1c631e0a25125ca1c0c0028ad1a920c2c2f 
> 
> Diff: https://reviews.apache.org/r/47530/diff/
> 
> 
> Testing
> ---
> 
> ## Unit tests.
> 
> On ubuntu 16.04:
> `sudo GTEST_FILTER="*SlaveEndpointTest*.*" make -j2 check`
> 
> ## Manual testing.
> 
> 1. Ran master with:
> ```
> sudo  ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
> ```
> 
> 2. ACL File: 
> ```
>   {
> "get_endpoints": [
>   {
> "principals": { "type": "NONE" },
> "paths": { "values": ["/flags", "/monitor/statistics", "/containers"] 
> }
>   }
> ]
>   } 
> ```
> 
> 3. Ran slave with: 
> ```
> sudo ./bin/mesos-slave.sh --master=127.0.0.1:5050 --ip=0.0.0.0 
> --acls=file:///home/abhishek/testAcl
> ```
> 
> 4. Ran toy-framework with: 
> ```
> sudo ./no-executor-framework --master=master@127.0.0.1:5050 --command="echo 
> hello"
> ```
> 
> 5. Output while hitting "http://127.0.0.1:5051/slave(1)/containers" - HTTP 
> error 403: Forbidden
> 
> 6. Changed ACL to: 
> ```
>   {
> "get_endpoints": [
>   {
> "principals": { "type": "ANY" },
> "paths": { "values": ["/flags", "/monitor/statistics", "/containers"] 
> }
>   }
> ]
>   }
> ```
> 
> 7. Ran slave and framework again.
> 
> 8. Output:
> ```
> 
> [{"container_id":"9b8a6a51-68be-4763-9c7d-b67e85fccb4a","executor_id":"42","executor_name":"Command
>  Executor (Task: 42) (Command: sh -c 'echo hello')","framework_id":"52...
> ```
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 47530: Added authorization to agent's '/containers' endpoint.

2016-05-18 Thread Alexander Rukletsov 


> On May 18, 2016, 12:55 p.m., Jan Schlicht wrote:
> > src/slave/http.cpp, line 787
> > 
> >
> > Please call `authorizeEndpoint` as soon as possible, i.e. after the 
> > endpoint has been extracted from the URL.
> > 
> > While I like the idea of doing work in parallel, by requesting the 
> > containerizer statuses prior to the authorization, this work should only be 
> > done after the authorization was successful. Hence this part should be in 
> > the `_containers` continuation.

This will also allow us to avoid the tuple-induced mess in the header.


- Alexander


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


On May 18, 2016, 12:59 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47530/
> ---
> 
> (Updated May 18, 2016, 12:59 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, Jan 
> Schlicht, and Till Toenshoff.
> 
> 
> Bugs: MESOS-5317
> https://issues.apache.org/jira/browse/MESOS-5317
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used GET_ENDPOINT_WITH_PATH coarse-grained authz on agent's 
>  '/containers' endpoint to enable authorization on this endpoint.
>  Updated docs and testcases as well.
> 
> 
> Diffs
> -
> 
>   docs/endpoints/slave/containers.md 959f40b9db4de4b6cea456ecf7bcb402f7a94f05 
>   src/slave/http.cpp fb48ec61e2fe0c83f80d3b8aa4c2ef5a96b748ae 
>   src/slave/slave.hpp 209f071448e3c52d16d3366d564003ee36b1d2e0 
>   src/tests/slave_authorization_tests.cpp 
> 843cf1c631e0a25125ca1c0c0028ad1a920c2c2f 
> 
> Diff: https://reviews.apache.org/r/47530/diff/
> 
> 
> Testing
> ---
> 
> ## Unit tests.
> 
> On ubuntu 16.04:
> `sudo GTEST_FILTER="*SlaveEndpointTest*.*" make -j2 check`
> 
> ## Manual testing.
> 
> 1. Ran master with:
> ```
> sudo  ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
> ```
> 
> 2. ACL File: 
> ```
>   {
> "get_endpoints": [
>   {
> "principals": { "type": "NONE" },
> "paths": { "values": ["/flags", "/monitor/statistics", "/containers"] 
> }
>   }
> ]
>   } 
> ```
> 
> 3. Ran slave with: 
> ```
> sudo ./bin/mesos-slave.sh --master=127.0.0.1:5050 --ip=0.0.0.0 
> --acls=file:///home/abhishek/testAcl
> ```
> 
> 4. Ran toy-framework with: 
> ```
> sudo ./no-executor-framework --master=master@127.0.0.1:5050 --command="echo 
> hello"
> ```
> 
> 5. Output while hitting "http://127.0.0.1:5051/slave(1)/containers" - HTTP 
> error 403: Forbidden
> 
> 6. Changed ACL to: 
> ```
>   {
> "get_endpoints": [
>   {
> "principals": { "type": "ANY" },
> "paths": { "values": ["/flags", "/monitor/statistics", "/containers"] 
> }
>   }
> ]
>   }
> ```
> 
> 7. Ran slave and framework again.
> 
> 8. Output:
> ```
> 
> [{"container_id":"9b8a6a51-68be-4763-9c7d-b67e85fccb4a","executor_id":"42","executor_name":"Command
>  Executor (Task: 42) (Command: sh -c 'echo hello')","framework_id":"52...
> ```
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 47530: Added authorization to agent's '/containers' endpoint.

2016-05-18 Thread Abhishek Dasgupta 

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

(Updated May 18, 2016, 12:59 p.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, Jan Schlicht, 
and Till Toenshoff.


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


Repository: mesos


Description
---

Used GET_ENDPOINT_WITH_PATH coarse-grained authz on agent's 
 '/containers' endpoint to enable authorization on this endpoint.
 Updated docs and testcases as well.


Diffs
-

  docs/endpoints/slave/containers.md 959f40b9db4de4b6cea456ecf7bcb402f7a94f05 
  src/slave/http.cpp fb48ec61e2fe0c83f80d3b8aa4c2ef5a96b748ae 
  src/slave/slave.hpp 209f071448e3c52d16d3366d564003ee36b1d2e0 
  src/tests/slave_authorization_tests.cpp 
843cf1c631e0a25125ca1c0c0028ad1a920c2c2f 

Diff: https://reviews.apache.org/r/47530/diff/


Testing (updated)
---

## Unit tests.

On ubuntu 16.04:
`sudo GTEST_FILTER="*SlaveEndpointTest*.*" make -j2 check`

## Manual testing.

1. Ran master with:
```
sudo  ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
```

2. ACL File: 
```
  {
"get_endpoints": [
  {
"principals": { "type": "NONE" },
"paths": { "values": ["/flags", "/monitor/statistics", "/containers"] }
  }
]
  } 
```

3. Ran slave with: 
```
sudo ./bin/mesos-slave.sh --master=127.0.0.1:5050 --ip=0.0.0.0 
--acls=file:///home/abhishek/testAcl
```

4. Ran toy-framework with: 
```
sudo ./no-executor-framework --master=master@127.0.0.1:5050 --command="echo 
hello"
```

5. Output while hitting "http://127.0.0.1:5051/slave(1)/containers" - HTTP 
error 403: Forbidden

6. Changed ACL to: 
```
  {
"get_endpoints": [
  {
"principals": { "type": "ANY" },
"paths": { "values": ["/flags", "/monitor/statistics", "/containers"] }
  }
]
  }
```

7. Ran slave and framework again.

8. Output:
```

[{"container_id":"9b8a6a51-68be-4763-9c7d-b67e85fccb4a","executor_id":"42","executor_name":"Command
 Executor (Task: 42) (Command: sh -c 'echo hello')","framework_id":"52...
```


Thanks,

Abhishek Dasgupta

Re: Review Request 47530: Added authorization to agent's '/containers' endpoint.

2016-05-18 Thread Jan Schlicht 

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




src/slave/http.cpp (line 756)


Please make sure that `request.method` is "GET" when this function is 
called with enabled authorization. See `Slave::Http::flags` for an example.

Currently, for many existing endpoints, the request method isn't checked 
which can lead to problems with authorization. We plan to change that later, 
see [MESOS-5346](https://issues.apache.org/jira/browse/MESOS-5346).



src/slave/http.cpp (line 786)


Please call `authorizeEndpoint` as soon as possible, i.e. after the 
endpoint has been extracted from the URL.

While I like the idea of doing work in parallel, by requesting the 
containerizer statuses prior to the authorization, this work should only be 
done after the authorization was successful. Hence this part should be in the 
`_containers` continuation.



src/slave/slave.hpp (lines 96 - 97)


Please don't do this in a header. The `using namespace process;` above is a 
bad example and probably shouldn't even be there.


- Jan Schlicht


On May 18, 2016, 12:06 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47530/
> ---
> 
> (Updated May 18, 2016, 12:06 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, Jan 
> Schlicht, and Till Toenshoff.
> 
> 
> Bugs: MESOS-5317
> https://issues.apache.org/jira/browse/MESOS-5317
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used GET_ENDPOINT_WITH_PATH coarse-grained authz on agent's 
>  '/containers' endpoint to enable authorization on this endpoint.
>  Updated docs and testcases as well.
> 
> 
> Diffs
> -
> 
>   docs/endpoints/slave/containers.md 959f40b9db4de4b6cea456ecf7bcb402f7a94f05 
>   src/slave/http.cpp fb48ec61e2fe0c83f80d3b8aa4c2ef5a96b748ae 
>   src/slave/slave.hpp 209f071448e3c52d16d3366d564003ee36b1d2e0 
>   src/tests/slave_authorization_tests.cpp 
> 843cf1c631e0a25125ca1c0c0028ad1a920c2c2f 
> 
> Diff: https://reviews.apache.org/r/47530/diff/
> 
> 
> Testing
> ---
> 
> sudo GTEST_FILTER="*SlaveEndpointTest*.*" make -j2 check
> 
> On ubuntu 16.04.
> 
> Ran manual testing as well.
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>