Re: Review Request 49218: Add support for Mesos Fetcher

2016-06-24 Thread Renan DelValle 


> On June 24, 2016, 4:33 p.m., David McLaughlin wrote:
> > api/src/main/thrift/org/apache/aurora/gen/api.thrift, lines 266-267
> > 
> >
> > Please rename to something like mesosFetcherUris (or just fetcherUris) 
> > to document purpose of these uris in this context.

Sounds like a good idea, I'll get this done after all the technical stuff has 
been cleared up.


- Renan


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


On June 24, 2016, 4:01 p.m., Renan DelValle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49218/
> ---
> 
> (Updated June 24, 2016, 4:01 p.m.)
> 
> 
> Review request for Aurora.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding a URIs field to TaskConfig inside the ThriftAPI so that users are able 
> to specify resources they wish to download into the sandbox per job.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 3e6daf53dd563dd7a2d494cc95e9a0aba0b6 
>   src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
> 4089b79da8079243703eead884e80bcf736f8b29 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 3b01801d929dd61ee989495bf38af8f03e9f5ad4 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> c76164292cf62d2181374c09f8bf6d8d3358e982 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 571201094c1e576e496495a01cb83f6c57decfa8 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/migration/V007_CreateURIsTable.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java 
> a90cb00e240df25dce6d55728859768e22d741a6 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  2c8af8b88e41b3b381cac831fd43b1057e4df0aa 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> 5069bedc08bb7111d0e0f101c8a2c81495b97bc9 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> 58785bfa37ff214f26e9f94d836e6df40e411c3b 
>   src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java 
> a883b0e33bfec1d14e6fe4ee8ed2200d93acaeec 
> 
> Diff: https://reviews.apache.org/r/49218/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> ./build-support/jenkins/build.sh
> bash src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Renan DelValle
> 
>

Re: Review Request 49218: Add support for Mesos Fetcher

2016-06-24 Thread Renan DelValle 


> On June 24, 2016, 5:57 p.m., Joshua Cohen wrote:
> > I'm not super familiar with the Mesos resource fetcher, but I'm assuming 
> > Mesos does not apply any access control on the uris grabbed by the fetcher 
> > (based on the fact that we already use this to grab the thermos executor 
> > from whatever path is configured via the scheduler command line)?
> > 
> > Am I missing something, or is this potentially a privilege escalation to 
> > just blindly allow user tasks to grab arbitrary URIs into their sandbox? Is 
> > there any way to control this? I think at the very least we should wire 
> > this functionality off by default via a command line flag, rejecting any 
> > tasks that request uris in this fashion if it's not explicitly enabled.

Mesos does not apply any access control and it could lead a security issue. As 
far as I know, the only way of controlling this is by filtering the URIs on the 
serverside (i.e. whitelist/blacklist). Turning this off by default and having 
it be a configuration is a great option while a more refined solution is 
created. For now, the other way I'm combatting this is by never allowing the 
URI grabbed to be set as an executable.


- Renan


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


On June 24, 2016, 4:01 p.m., Renan DelValle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49218/
> ---
> 
> (Updated June 24, 2016, 4:01 p.m.)
> 
> 
> Review request for Aurora.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding a URIs field to TaskConfig inside the ThriftAPI so that users are able 
> to specify resources they wish to download into the sandbox per job.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 3e6daf53dd563dd7a2d494cc95e9a0aba0b6 
>   src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
> 4089b79da8079243703eead884e80bcf736f8b29 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 3b01801d929dd61ee989495bf38af8f03e9f5ad4 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> c76164292cf62d2181374c09f8bf6d8d3358e982 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 571201094c1e576e496495a01cb83f6c57decfa8 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/migration/V007_CreateURIsTable.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java 
> a90cb00e240df25dce6d55728859768e22d741a6 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  2c8af8b88e41b3b381cac831fd43b1057e4df0aa 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> 5069bedc08bb7111d0e0f101c8a2c81495b97bc9 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> 58785bfa37ff214f26e9f94d836e6df40e411c3b 
>   src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java 
> a883b0e33bfec1d14e6fe4ee8ed2200d93acaeec 
> 
> Diff: https://reviews.apache.org/r/49218/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> ./build-support/jenkins/build.sh
> bash src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Renan DelValle
> 
>

Re: Review Request 49218: Add support for Mesos Fetcher

2016-06-24 Thread Joshua Cohen 

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



I'm not super familiar with the Mesos resource fetcher, but I'm assuming Mesos 
does not apply any access control on the uris grabbed by the fetcher (based on 
the fact that we already use this to grab the thermos executor from whatever 
path is configured via the scheduler command line)?

Am I missing something, or is this potentially a privilege escalation to just 
blindly allow user tasks to grab arbitrary URIs into their sandbox? Is there 
any way to control this? I think at the very least we should wire this 
functionality off by default via a command line flag, rejecting any tasks that 
request uris in this fashion if it's not explicitly enabled.


src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
(line 150)


Fix this copy/paste.



src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml 
(line 196)


This should be `u_id` not `m_id`.



src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml 
(lines 408 - 411)


Indent 2


- Joshua Cohen


On June 24, 2016, 11:01 p.m., Renan DelValle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49218/
> ---
> 
> (Updated June 24, 2016, 11:01 p.m.)
> 
> 
> Review request for Aurora.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding a URIs field to TaskConfig inside the ThriftAPI so that users are able 
> to specify resources they wish to download into the sandbox per job.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 3e6daf53dd563dd7a2d494cc95e9a0aba0b6 
>   src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
> 4089b79da8079243703eead884e80bcf736f8b29 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 3b01801d929dd61ee989495bf38af8f03e9f5ad4 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> c76164292cf62d2181374c09f8bf6d8d3358e982 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 571201094c1e576e496495a01cb83f6c57decfa8 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/migration/V007_CreateURIsTable.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java 
> a90cb00e240df25dce6d55728859768e22d741a6 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  2c8af8b88e41b3b381cac831fd43b1057e4df0aa 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> 5069bedc08bb7111d0e0f101c8a2c81495b97bc9 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> 58785bfa37ff214f26e9f94d836e6df40e411c3b 
>   src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java 
> a883b0e33bfec1d14e6fe4ee8ed2200d93acaeec 
> 
> Diff: https://reviews.apache.org/r/49218/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> ./build-support/jenkins/build.sh
> bash src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Renan DelValle
> 
>

Re: Review Request 49218: Add support for Mesos Fetcher

2016-06-24 Thread David McLaughlin 

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




api/src/main/thrift/org/apache/aurora/gen/api.thrift (lines 266 - 267)


Please rename to something like mesosFetcherUris (or just fetcherUris) to 
document purpose of these uris in this context.


- David McLaughlin


On June 24, 2016, 11:01 p.m., Renan DelValle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49218/
> ---
> 
> (Updated June 24, 2016, 11:01 p.m.)
> 
> 
> Review request for Aurora.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding a URIs field to TaskConfig inside the ThriftAPI so that users are able 
> to specify resources they wish to download into the sandbox per job.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 3e6daf53dd563dd7a2d494cc95e9a0aba0b6 
>   src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
> 4089b79da8079243703eead884e80bcf736f8b29 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 3b01801d929dd61ee989495bf38af8f03e9f5ad4 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> c76164292cf62d2181374c09f8bf6d8d3358e982 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 571201094c1e576e496495a01cb83f6c57decfa8 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/migration/V007_CreateURIsTable.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java 
> a90cb00e240df25dce6d55728859768e22d741a6 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  2c8af8b88e41b3b381cac831fd43b1057e4df0aa 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> 5069bedc08bb7111d0e0f101c8a2c81495b97bc9 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> 58785bfa37ff214f26e9f94d836e6df40e411c3b 
>   src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java 
> a883b0e33bfec1d14e6fe4ee8ed2200d93acaeec 
> 
> Diff: https://reviews.apache.org/r/49218/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> ./build-support/jenkins/build.sh
> bash src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Renan DelValle
> 
>

Review Request 49218: Add support for Mesos Fetcher

2016-06-24 Thread Renan DelValle 

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

Review request for Aurora.


Repository: aurora


Description
---

Adding a URIs field to TaskConfig inside the ThriftAPI so that users are able 
to specify resources they wish to download into the sandbox per job.


Diffs
-

  api/src/main/thrift/org/apache/aurora/gen/api.thrift 
3e6daf53dd563dd7a2d494cc95e9a0aba0b6 
  src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
4089b79da8079243703eead884e80bcf736f8b29 
  src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
3b01801d929dd61ee989495bf38af8f03e9f5ad4 
  src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
c76164292cf62d2181374c09f8bf6d8d3358e982 
  src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
571201094c1e576e496495a01cb83f6c57decfa8 
  
src/main/java/org/apache/aurora/scheduler/storage/db/migration/V007_CreateURIsTable.java
 PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java 
a90cb00e240df25dce6d55728859768e22d741a6 
  
src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml 
2c8af8b88e41b3b381cac831fd43b1057e4df0aa 
  src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
5069bedc08bb7111d0e0f101c8a2c81495b97bc9 
  src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
58785bfa37ff214f26e9f94d836e6df40e411c3b 
  src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java 
a883b0e33bfec1d14e6fe4ee8ed2200d93acaeec 

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


Testing
---

./gradlew build -Pq
./build-support/jenkins/build.sh
bash src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh


Thanks,

Renan DelValle

Re: Review Request 49048: AURORA-1710 Make 'tier' required and remove support for 'production' flag in Job configuration - CLI changes

2016-06-24 Thread Joshua Cohen 

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



Just some style stuff.


src/main/python/apache/aurora/client/cli/context.py (lines 136 - 137)


continuation indent here should be...

next(
(t.name for t in ...),
tier_configurations.defaultTierName)



src/main/python/apache/aurora/client/cli/context.py (lines 141 - 143)


same here



src/test/python/apache/aurora/client/cli/test_context.py (lines 90 - 95)


fix indent here as well.



src/test/python/apache/aurora/client/cli/test_context.py (line 98)


The names of these tests are a little unclear, it took some time to parse 
what each one was testing.

Do you think it would make sense to rename to make their asserts more 
explict. E.g. `test_get_config_production_and_tier_unset_is_preemptible` and 
`test_get_config_production_set_is_preferred`, etc.?


- Joshua Cohen


On June 23, 2016, 10:35 p.m., Mehrdad Nurolahzade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49048/
> ---
> 
> (Updated June 23, 2016, 10:35 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> AURORA-1710 Make 'tier' required and remove support for 'production' flag in 
> Job configuration - CLI changes
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 68baf8fdb90cd26100159401c46c9963c24332b3 
>   src/main/python/apache/aurora/client/cli/context.py 
> 9b1511801d031ff48b81c25688a55cb586b8ac66 
>   src/main/python/apache/aurora/client/config.py 
> 2fc12559016d406c347adb416a5166cca31c961e 
>   src/main/python/apache/aurora/config/__init__.py 
> 65923be1cb8b88139b8eab0ac5b75428972d3cb1 
>   src/test/python/apache/aurora/client/cli/test_command_hooks.py 
> 2130f1fa71be02a004cdf8e476a270c81a7105d3 
>   src/test/python/apache/aurora/client/cli/test_context.py 
> 204ca092adad8bf43c5032a02f61bf303fb0b2fc 
>   src/test/python/apache/aurora/client/cli/test_create.py 
> 8c27e2b340bb0a5fb5bcb44ef94d433e7f92c76c 
>   src/test/python/apache/aurora/client/cli/test_cron.py 
> f3c522ed94a2d774865811ceb546bf9df083c14f 
>   src/test/python/apache/aurora/client/cli/test_plugins.py 
> a545fece5e2b3e0017a61e1be9ac478372b1f34d 
>   src/test/python/apache/aurora/client/cli/test_restart.py 
> 967d560e5c7eb0ed85b215fb11d9751b8666acb5 
>   src/test/python/apache/aurora/client/cli/util.py 
> 7b4558ec7f0fb0fd2902591bc6a90dc15051dd6e 
>   src/test/python/apache/aurora/client/test_config.py 
> b1a3c1865819899ef19173be0f861783a2631d0a 
> 
> Diff: https://reviews.apache.org/r/49048/diff/
> 
> 
> Testing
> ---
> 
> ```
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> *** OK (All tests passed) ***
> 
> mesos-master start/running, process 26868
> + RETCODE=0
> + restore_netrc
> + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc
> + true
> Connection to 127.0.0.1 closed.
> 
> real  19m46.324s
> user  0m1.496s
> sys   0m0.774s
> ```
> 
> 
> Thanks,
> 
> Mehrdad Nurolahzade
> 
>