Re: Review Request 44570: Added an owner field to TaskInfo and ExecutorInfo.

[Jan Schlicht]

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

(Updated March 15, 2016, 10:59 a.m.)


Review request for mesos, Adam B and Joerg Schad.


Changes
---

Addressed Jörg's issues.


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


Repository: mesos


Description
---

To be able to authenticate HTTP requests for tasks, the authorizer has to
determine who owns the tasks. This is achieved by adding an owner field to
TaskInfo and ExecutorInfo.


Diffs (updated)
-

  include/mesos/mesos.proto deb9c0910a27afd67276f54b3f666a878212727b 
  include/mesos/v1/mesos.proto a981e750c24cfc48177bbc9ca56f0c3ecfae1a1b 
  src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
  src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
  src/tests/master_validation_tests.cpp 
c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 44570: Added an owner field to TaskInfo and ExecutorInfo.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44570]

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 March 14, 2016, 11:06 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44570/
> ---
> 
> (Updated March 14, 2016, 11:06 a.m.)
> 
> 
> Review request for mesos, Adam B and Joerg Schad.
> 
> 
> Bugs: MESOS-4772
> https://issues.apache.org/jira/browse/MESOS-4772
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To be able to authenticate HTTP requests for tasks, the authorizer has to
> determine who owns the tasks. This is achieved by adding an owner field to
> TaskInfo and ExecutorInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 56d456acfd35fa59f394b27d62f62772eec03f6a 
>   include/mesos/v1/mesos.proto 4fba77464bb052d27c424f7721397142850b1144 
>   src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
>   src/tests/master_validation_tests.cpp 
> c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 
> 
> Diff: https://reviews.apache.org/r/44570/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 44570: Added an owner field to TaskInfo and ExecutorInfo.

[Joerg Schad]

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


Fix it, then Ship it!





include/mesos/mesos.proto (line 440)


Maybe we could add some more information about the potential values could 
be set for the owner field here (and below).
I just imagine myself as a framework writer seeing this field...



include/mesos/mesos.proto (line 441)


task/executor related information?



include/mesos/mesos.proto (line 1221)


see comment above.



include/mesos/v1/mesos.proto (line 440)


dito.



include/mesos/v1/mesos.proto (line 441)


dito.



include/mesos/v1/mesos.proto (line 1218)


dito.



src/master/validation.cpp (line 381)


s/Owner/owner

I addition I personally would prefer a more descriptive Error message 
(something like 'as the owner will be inherited from the executor'), but this 
is ok for me right now.



src/tests/master_validation_tests.cpp (line 1166)


This test verifies legal combination of set owner field for tasks and 
executors.



src/tests/master_validation_tests.cpp (line 1196)


A task without owner and an executor


- Joerg Schad


On March 14, 2016, 11:06 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44570/
> ---
> 
> (Updated March 14, 2016, 11:06 a.m.)
> 
> 
> Review request for mesos, Adam B and Joerg Schad.
> 
> 
> Bugs: MESOS-4772
> https://issues.apache.org/jira/browse/MESOS-4772
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To be able to authenticate HTTP requests for tasks, the authorizer has to
> determine who owns the tasks. This is achieved by adding an owner field to
> TaskInfo and ExecutorInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 56d456acfd35fa59f394b27d62f62772eec03f6a 
>   include/mesos/v1/mesos.proto 4fba77464bb052d27c424f7721397142850b1144 
>   src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
>   src/tests/master_validation_tests.cpp 
> c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 
> 
> Diff: https://reviews.apache.org/r/44570/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 44570: Added an owner field to TaskInfo and ExecutorInfo.

[Jan Schlicht]

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

(Updated March 14, 2016, 12:06 p.m.)


Review request for mesos, Adam B and Joerg Schad.


Changes
---

Updated V1 comments.


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


Repository: mesos


Description
---

To be able to authenticate HTTP requests for tasks, the authorizer has to
determine who owns the tasks. This is achieved by adding an owner field to
TaskInfo and ExecutorInfo.


Diffs (updated)
-

  include/mesos/mesos.proto 56d456acfd35fa59f394b27d62f62772eec03f6a 
  include/mesos/v1/mesos.proto 4fba77464bb052d27c424f7721397142850b1144 
  src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
  src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
  src/tests/master_validation_tests.cpp 
c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 44570: Added an owner field to TaskInfo and ExecutorInfo.

[Adam B]

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


Fix it, then Ship it!




Just fix the v1 comments, and then I think we're good to go.
Anybody else want to take a look before I commit?


include/mesos/v1/mesos.proto (lines 440 - 442)


Update these comments to match the others.


- Adam B


On March 14, 2016, 3:46 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44570/
> ---
> 
> (Updated March 14, 2016, 3:46 a.m.)
> 
> 
> Review request for mesos, Adam B and Joerg Schad.
> 
> 
> Bugs: MESOS-4772
> https://issues.apache.org/jira/browse/MESOS-4772
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To be able to authenticate HTTP requests for tasks, the authorizer has to
> determine who owns the tasks. This is achieved by adding an owner field to
> TaskInfo and ExecutorInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 56d456acfd35fa59f394b27d62f62772eec03f6a 
>   include/mesos/v1/mesos.proto 4fba77464bb052d27c424f7721397142850b1144 
>   src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
>   src/tests/master_validation_tests.cpp 
> c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 
> 
> Diff: https://reviews.apache.org/r/44570/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 44570: Added an owner field to TaskInfo and ExecutorInfo.

[Jan Schlicht]

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

(Updated March 14, 2016, 11:46 a.m.)


Review request for mesos, Adam B and Joerg Schad.


Changes
---

Updated comments.


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


Repository: mesos


Description
---

To be able to authenticate HTTP requests for tasks, the authorizer has to
determine who owns the tasks. This is achieved by adding an owner field to
TaskInfo and ExecutorInfo.


Diffs (updated)
-

  include/mesos/mesos.proto 56d456acfd35fa59f394b27d62f62772eec03f6a 
  include/mesos/v1/mesos.proto 4fba77464bb052d27c424f7721397142850b1144 
  src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
  src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
  src/tests/master_validation_tests.cpp 
c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 44570: Added an owner field to TaskInfo and ExecutorInfo.

[Adam B]

> On March 9, 2016, 7:33 a.m., James DeFelice wrote:
> > src/master/validation.cpp, line 379
> > 
> >
> > this is exactly the type of thing that I'd expect to be documented in 
> > the protobuf

I believe this is represented in the protobuf now, in the latest patch 
revisions. Can we mark this as "Fixed"?


- Adam


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


On March 14, 2016, 2:37 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44570/
> ---
> 
> (Updated March 14, 2016, 2:37 a.m.)
> 
> 
> Review request for mesos, Adam B and Joerg Schad.
> 
> 
> Bugs: MESOS-4772
> https://issues.apache.org/jira/browse/MESOS-4772
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To be able to authenticate HTTP requests for tasks, the authorizer has to
> determine who owns the tasks. This is achieved by adding an owner field to
> TaskInfo and ExecutorInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 56d456acfd35fa59f394b27d62f62772eec03f6a 
>   include/mesos/v1/mesos.proto 4fba77464bb052d27c424f7721397142850b1144 
>   src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
>   src/tests/master_validation_tests.cpp 
> c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 
> 
> Diff: https://reviews.apache.org/r/44570/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 44570: Added an owner field to TaskInfo and ExecutorInfo.

[Adam B]

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



Looks good. Just update the comments and error string.


src/master/validation.cpp (line 382)


Update the error string. Perhaps "TaskInfo cannot have an Owner if 
ExecutorInfo is set"



include/mesos/mesos.proto (lines 440 - 441)


"Owner of the executor. This field may be used by authorizers to determine 
access to executor-related information and actions."



include/mesos/mesos.proto (lines 1222 - 1224)


"Owner of the task. This field may be used by authorizers to determine 
access to task/executor-related information and actions. If ExecutorInfo is 
present, use the owner field there instead."



src/master/validation.cpp (lines 377 - 378)


Update the comment: "Validates that ownership is not set on TaskInfo if 
ExecutorInfo is present."



src/tests/master_validation_tests.cpp (lines 1166 - 1167)


Update comment please.
"This test verifies that owner can only be set in TaskInfo if ExecutorInfo 
is not present."


- Adam B


On March 14, 2016, 2:37 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44570/
> ---
> 
> (Updated March 14, 2016, 2:37 a.m.)
> 
> 
> Review request for mesos, Adam B and Joerg Schad.
> 
> 
> Bugs: MESOS-4772
> https://issues.apache.org/jira/browse/MESOS-4772
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To be able to authenticate HTTP requests for tasks, the authorizer has to
> determine who owns the tasks. This is achieved by adding an owner field to
> TaskInfo and ExecutorInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 56d456acfd35fa59f394b27d62f62772eec03f6a 
>   include/mesos/v1/mesos.proto 4fba77464bb052d27c424f7721397142850b1144 
>   src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
>   src/tests/master_validation_tests.cpp 
> c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 
> 
> Diff: https://reviews.apache.org/r/44570/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 44570: Added an owner field to TaskInfo and ExecutorInfo.

[Jan Schlicht]

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

(Updated March 14, 2016, 10:37 a.m.)


Review request for mesos, Adam B and Joerg Schad.


Changes
---

Addressed issues.


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


Repository: mesos


Description
---

To be able to authenticate HTTP requests for tasks, the authorizer has to
determine who owns the tasks. This is achieved by adding an owner field to
TaskInfo and ExecutorInfo.


Diffs (updated)
-

  include/mesos/mesos.proto 56d456acfd35fa59f394b27d62f62772eec03f6a 
  include/mesos/v1/mesos.proto 4fba77464bb052d27c424f7721397142850b1144 
  src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
  src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
  src/tests/master_validation_tests.cpp 
c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 44570: Added an owner field to TaskInfo and ExecutorInfo.

[Adam B]

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



Sorry for the misunderstanding. As far as (sandbox) access-control is 
concerned, we can only isolate containers from each other, so each container 
can only have a single owner. With the default executor, the TaskInfo is 1:1 
with the container. But with a custom executor, the ExecutorInfo is 1:1 with 
the container.


include/mesos/mesos.proto (lines 440 - 442)


Owner of the executor...



src/tests/master_validation_tests.cpp (lines 1184 - 1185)


Not true. If an executorInfo is specified, then TaskInfo.owner is 
irrelevant, since the executor is the unit of containerization, ownership, and 
access control.
The only reason we need a TaskInfo.owner is because there is no 
ExecutorInfo for the default command executor. Same holds true for CommandInfo.


- Adam B


On March 10, 2016, 4:49 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44570/
> ---
> 
> (Updated March 10, 2016, 4:49 a.m.)
> 
> 
> Review request for mesos, Adam B and Joerg Schad.
> 
> 
> Bugs: MESOS-4772
> https://issues.apache.org/jira/browse/MESOS-4772
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To be able to authenticate HTTP requests for tasks, the authorizer has to
> determine who owns the tasks. This is achieved by adding an owner field to
> TaskInfo and ExecutorInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 3d22ec32655dca741169e1f0e382303e061c38b7 
>   include/mesos/v1/mesos.proto 31960a52061f70d80528fb8326522ae1d6f75b2c 
>   src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
>   src/tests/master_validation_tests.cpp 
> c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 
> 
> Diff: https://reviews.apache.org/r/44570/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 44570: Added an owner field to TaskInfo and ExecutorInfo.

[Adam B]

> On March 9, 2016, 10:31 p.m., Adam B wrote:
> > src/master/validation.cpp, line 381
> > 
> >
> > It may be sufficient to only check `if (task.has_owner() && 
> > task.has_executor())` since a custom executor should set the owner on the 
> > ExecutorInfo not the TaskInfo.
> 
> Jan Schlicht wrote:
> That is if the `ExecutorInfo` is reused in a `TaskInfo` it's sufficient 
> to check for that? That makes sense because the `ExecutorInfo` validation 
> compares the `ExecutorId` against the ones of existing `ExecutorInfo`s. But 
> how'd we make sure that the owner is set in `ExecutorInfo` if that executor 
> isn't existing yet?

Well, we can't enforce that the owner field has to be set, since that would 
break backwards compatibility. It's an optional field, so even if a custom 
ExecutorInfo is provided, the owner field may not be set. Here's what's allowed:
a) !task.has_executor() && (task.has_owner() || !task.has_owner()) // Default 
executor, may or may not have owner set.
b) task.has_executor() && !task.has_owner() && (task.executor().has_owner() || 
!task.executor().has_owner()) // Custom executor, may or may not have owner 
set; taskInfo does not have an owner set.
Here's what's not allowed:
x) task.has_executor() && task.has_owner() && task.executor().has_owner() // 
How can the task have a different owner than the executor?
y) task.has_executor() && task.has_owner() && !task.executor().has_owner() // 
Why would the task have an owner but the executor doesn't?


- Adam


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


On March 10, 2016, 4:49 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44570/
> ---
> 
> (Updated March 10, 2016, 4:49 a.m.)
> 
> 
> Review request for mesos, Adam B and Joerg Schad.
> 
> 
> Bugs: MESOS-4772
> https://issues.apache.org/jira/browse/MESOS-4772
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To be able to authenticate HTTP requests for tasks, the authorizer has to
> determine who owns the tasks. This is achieved by adding an owner field to
> TaskInfo and ExecutorInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 3d22ec32655dca741169e1f0e382303e061c38b7 
>   include/mesos/v1/mesos.proto 31960a52061f70d80528fb8326522ae1d6f75b2c 
>   src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
>   src/tests/master_validation_tests.cpp 
> c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 
> 
> Diff: https://reviews.apache.org/r/44570/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 44570: Added an owner field to TaskInfo and ExecutorInfo.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44570]

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 March 10, 2016, 12:49 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44570/
> ---
> 
> (Updated March 10, 2016, 12:49 p.m.)
> 
> 
> Review request for mesos, Adam B and Joerg Schad.
> 
> 
> Bugs: MESOS-4772
> https://issues.apache.org/jira/browse/MESOS-4772
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To be able to authenticate HTTP requests for tasks, the authorizer has to
> determine who owns the tasks. This is achieved by adding an owner field to
> TaskInfo and ExecutorInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 3d22ec32655dca741169e1f0e382303e061c38b7 
>   include/mesos/v1/mesos.proto 31960a52061f70d80528fb8326522ae1d6f75b2c 
>   src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
>   src/tests/master_validation_tests.cpp 
> c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 
> 
> Diff: https://reviews.apache.org/r/44570/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 44570: Added an owner field to TaskInfo and ExecutorInfo.

[Jan Schlicht]

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

(Updated March 10, 2016, 1:49 p.m.)


Review request for mesos, Adam B and Joerg Schad.


Changes
---

Addressed some issues.


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


Repository: mesos


Description
---

To be able to authenticate HTTP requests for tasks, the authorizer has to
determine who owns the tasks. This is achieved by adding an owner field to
TaskInfo and ExecutorInfo.


Diffs (updated)
-

  include/mesos/mesos.proto 3d22ec32655dca741169e1f0e382303e061c38b7 
  include/mesos/v1/mesos.proto 31960a52061f70d80528fb8326522ae1d6f75b2c 
  src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
  src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
  src/tests/master_validation_tests.cpp 
c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 44570: Added an owner field to TaskInfo and ExecutorInfo.

[Jan Schlicht]

> On March 10, 2016, 7:31 a.m., Adam B wrote:
> > src/master/validation.cpp, line 381
> > 
> >
> > It may be sufficient to only check `if (task.has_owner() && 
> > task.has_executor())` since a custom executor should set the owner on the 
> > ExecutorInfo not the TaskInfo.

That is if the `ExecutorInfo` is reused in a `TaskInfo` it's sufficient to 
check for that? That makes sense because the `ExecutorInfo` validation compares 
the `ExecutorId` against the ones of existing `ExecutorInfo`s. But how'd we 
make sure that the owner is set in `ExecutorInfo` if that executor isn't 
existing yet?


- Jan


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


On March 9, 2016, 4:48 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44570/
> ---
> 
> (Updated March 9, 2016, 4:48 p.m.)
> 
> 
> Review request for mesos, Adam B and Joerg Schad.
> 
> 
> Bugs: MESOS-4772
> https://issues.apache.org/jira/browse/MESOS-4772
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To be able to authenticate HTTP requests for tasks, the authorizer has to
> determine who owns the tasks. This is achieved by adding an owner field to
> TaskInfo and ExecutorInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 3d22ec32655dca741169e1f0e382303e061c38b7 
>   src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
>   src/tests/master_validation_tests.cpp 
> c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 
> 
> Diff: https://reviews.apache.org/r/44570/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 44570: Added an owner field to TaskInfo and ExecutorInfo.

[Jan Schlicht]

> On March 10, 2016, 9:22 a.m., Klaus Ma wrote:
> > include/mesos/mesos.proto, line 443
> > 
> >
> > Also added in v1 APIs?

Oh sure, good catch!


- Jan


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


On March 9, 2016, 4:48 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44570/
> ---
> 
> (Updated March 9, 2016, 4:48 p.m.)
> 
> 
> Review request for mesos, Adam B and Joerg Schad.
> 
> 
> Bugs: MESOS-4772
> https://issues.apache.org/jira/browse/MESOS-4772
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To be able to authenticate HTTP requests for tasks, the authorizer has to
> determine who owns the tasks. This is achieved by adding an owner field to
> TaskInfo and ExecutorInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 3d22ec32655dca741169e1f0e382303e061c38b7 
>   src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
>   src/tests/master_validation_tests.cpp 
> c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 
> 
> Diff: https://reviews.apache.org/r/44570/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 44570: Added an owner field to TaskInfo and ExecutorInfo.

[Klaus Ma]

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




include/mesos/mesos.proto (line 443)


Also added in v1 APIs?



src/master/validation.cpp (lines 381 - 382)


move `{` into one line.


- Klaus Ma


On March 9, 2016, 11:48 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44570/
> ---
> 
> (Updated March 9, 2016, 11:48 p.m.)
> 
> 
> Review request for mesos, Adam B and Joerg Schad.
> 
> 
> Bugs: MESOS-4772
> https://issues.apache.org/jira/browse/MESOS-4772
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To be able to authenticate HTTP requests for tasks, the authorizer has to
> determine who owns the tasks. This is achieved by adding an owner field to
> TaskInfo and ExecutorInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 3d22ec32655dca741169e1f0e382303e061c38b7 
>   src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
>   src/tests/master_validation_tests.cpp 
> c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 
> 
> Diff: https://reviews.apache.org/r/44570/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 44570: Added an owner field to TaskInfo and ExecutorInfo.

[Adam B]

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



We may need to do some discovery on whether an executor could have a different 
owner on each of its tasks.


src/master/validation.cpp (line 381)


It may be sufficient to only check `if (task.has_owner() && 
task.has_executor())` since a custom executor should set the owner on the 
ExecutorInfo not the TaskInfo.



src/tests/master_validation_tests.cpp (line 1185)


Also, a task with a set owner and an executor without a set owner is 
invalid?


- Adam B


On March 9, 2016, 7:48 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44570/
> ---
> 
> (Updated March 9, 2016, 7:48 a.m.)
> 
> 
> Review request for mesos, Adam B and Joerg Schad.
> 
> 
> Bugs: MESOS-4772
> https://issues.apache.org/jira/browse/MESOS-4772
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To be able to authenticate HTTP requests for tasks, the authorizer has to
> determine who owns the tasks. This is achieved by adding an owner field to
> TaskInfo and ExecutorInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 3d22ec32655dca741169e1f0e382303e061c38b7 
>   src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
>   src/tests/master_validation_tests.cpp 
> c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 
> 
> Diff: https://reviews.apache.org/r/44570/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 44570: Added an owner field to TaskInfo and ExecutorInfo.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44570]

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 March 9, 2016, 3:48 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44570/
> ---
> 
> (Updated March 9, 2016, 3:48 p.m.)
> 
> 
> Review request for mesos, Adam B and Joerg Schad.
> 
> 
> Bugs: MESOS-4772
> https://issues.apache.org/jira/browse/MESOS-4772
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To be able to authenticate HTTP requests for tasks, the authorizer has to
> determine who owns the tasks. This is achieved by adding an owner field to
> TaskInfo and ExecutorInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 3d22ec32655dca741169e1f0e382303e061c38b7 
>   src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
>   src/tests/master_validation_tests.cpp 
> c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 
> 
> Diff: https://reviews.apache.org/r/44570/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 44570: Added an owner field to TaskInfo and ExecutorInfo.

[Jan Schlicht]

> On March 9, 2016, 4:33 p.m., James DeFelice wrote:
> > include/mesos/mesos.proto, line 442
> > 
> >
> > what if it's specified in both places, two different ways? I think 
> > there's room for a little more description in these comments.

Yes, it makes sense to document the resulting error here. Though I'd argue that 
saying "Setting both will result in an error." includes the case that owner is 
specified in both places, two different ways.


- Jan


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


On March 9, 2016, 4 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44570/
> ---
> 
> (Updated March 9, 2016, 4 p.m.)
> 
> 
> Review request for mesos, Adam B and Joerg Schad.
> 
> 
> Bugs: MESOS-4772
> https://issues.apache.org/jira/browse/MESOS-4772
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To be able to authenticate HTTP requests for tasks, the authorizer has to
> determine who owns the tasks. This is achieved by adding an owner field to
> TaskInfo and ExecutorInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 3d22ec32655dca741169e1f0e382303e061c38b7 
>   src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
>   src/tests/master_validation_tests.cpp 
> c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 
> 
> Diff: https://reviews.apache.org/r/44570/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 44570: Added an owner field to TaskInfo and ExecutorInfo.

[Jan Schlicht]

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

(Updated March 9, 2016, 4:48 p.m.)


Review request for mesos, Adam B and Joerg Schad.


Changes
---

Document behavior if both, `TaskInfo` and `ExecutorInfo` have a set owner.


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


Repository: mesos


Description
---

To be able to authenticate HTTP requests for tasks, the authorizer has to
determine who owns the tasks. This is achieved by adding an owner field to
TaskInfo and ExecutorInfo.


Diffs (updated)
-

  include/mesos/mesos.proto 3d22ec32655dca741169e1f0e382303e061c38b7 
  src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
  src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
  src/tests/master_validation_tests.cpp 
c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 44570: Added an owner field to TaskInfo and ExecutorInfo.

[James DeFelice]

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




include/mesos/mesos.proto (line 442)


what if it's specified in both places, two different ways? I think there's 
room for a little more description in these comments.



src/master/validation.cpp (line 379)


this is exactly the type of thing that I'd expect to be documented in the 
protobuf


- James DeFelice


On March 9, 2016, 3 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44570/
> ---
> 
> (Updated March 9, 2016, 3 p.m.)
> 
> 
> Review request for mesos, Adam B and Joerg Schad.
> 
> 
> Bugs: MESOS-4772
> https://issues.apache.org/jira/browse/MESOS-4772
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To be able to authenticate HTTP requests for tasks, the authorizer has to
> determine who owns the tasks. This is achieved by adding an owner field to
> TaskInfo and ExecutorInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 3d22ec32655dca741169e1f0e382303e061c38b7 
>   src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
>   src/tests/master_validation_tests.cpp 
> c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 
> 
> Diff: https://reviews.apache.org/r/44570/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 44570: Added an owner field to TaskInfo and ExecutorInfo.

[Jan Schlicht]

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

(Updated March 9, 2016, 4 p.m.)


Review request for mesos, Adam B and Joerg Schad.


Changes
---

Addressed issues.


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


Repository: mesos


Description
---

To be able to authenticate HTTP requests for tasks, the authorizer has to
determine who owns the tasks. This is achieved by adding an owner field to
TaskInfo and ExecutorInfo.


Diffs (updated)
-

  include/mesos/mesos.proto 3d22ec32655dca741169e1f0e382303e061c38b7 
  src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
  src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
  src/tests/master_validation_tests.cpp 
c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 44570: Added an owner field to TaskInfo and ExecutorInfo.

[Jan Schlicht]

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

(Updated March 9, 2016, 3:51 p.m.)


Review request for mesos, Adam B and Joerg Schad.


Changes
---

Typo in description.


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


Repository: mesos


Description (updated)
---

To be able to authenticate HTTP requests for tasks, the authorizer has to
determine who owns the tasks. This is achieved by adding an owner field to
TaskInfo and ExecutorInfo.


Diffs
-

  include/mesos/mesos.proto 3d22ec32655dca741169e1f0e382303e061c38b7 
  src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
  src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
  src/tests/master_validation_tests.cpp 
c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 44570: Added an owner field to TaskInfo and ExecutorInfo.

[Guangya Liu]

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



Does it a typo in Description? s/archived/achieved


src/tests/master_validation_tests.cpp (lines 1180 - 1192)


What about update logic as following, this can make sure no need to call 
`set_owner` twice.

  ExecutorInfo executor;
  executor.set_owner("foo");

  task.mutable_executor()->CopyFrom(executor);
  
  // A task with a set owner and an executor with a set owner is invalid.
  EXPECT_SOME(task::internal::validateOwner(task));

  task.clear_owner();
  
  // A task with an executor that has a set owner is valid.
  EXPECT_NONE(task::internal::validateOwner(task));


- Guangya Liu


On 三月 9, 2016, 2:21 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44570/
> ---
> 
> (Updated 三月 9, 2016, 2:21 p.m.)
> 
> 
> Review request for mesos, Adam B and Joerg Schad.
> 
> 
> Bugs: MESOS-4772
> https://issues.apache.org/jira/browse/MESOS-4772
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To be able to authenticate HTTP requests for tasks, the authorizer has to
> determine who owns the tasks. This is archived by adding an owner field to
> TaskInfo and ExecutorInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 3d22ec32655dca741169e1f0e382303e061c38b7 
>   src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
>   src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
>   src/tests/master_validation_tests.cpp 
> c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 
> 
> Diff: https://reviews.apache.org/r/44570/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Review Request 44570: Added an owner field to TaskInfo and ExecutorInfo.

[Jan Schlicht]

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

Review request for mesos, Adam B and Joerg Schad.


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


Repository: mesos


Description
---

To be able to authenticate HTTP requests for tasks, the authorizer has to
determine who owns the tasks. This is archived by adding an owner field to
TaskInfo and ExecutorInfo.


Diffs
-

  include/mesos/mesos.proto 3d22ec32655dca741169e1f0e382303e061c38b7 
  src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
  src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 
  src/tests/master_validation_tests.cpp 
c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 

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


Testing
---

make check


Thanks,

Jan Schlicht