Re: Review Request 62145: Implemented Standalone Container API.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/http.cpp
Line 2421 (original), 2453 (patched)


`switch_user` is posix only flag. You need to wrap with `ifndef 
__WINDOWS__`?



src/slave/http.cpp
Line 2422 (original), 2454-2456 (patched)


For nested container, we by default use executor's user? Can you add that 
logic back?


- Jie Yu


On Nov. 14, 2017, 1:32 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62145/
> ---
> 
> (Updated Nov. 14, 2017, 1:32 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7305
> https://issues.apache.org/jira/browse/MESOS-7305
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The Standalone and Nested Container APIs are very similar.
> This commit combines the two API implementations by adding a
> translation function (i.e. `launchNestedContainer` and 
> `launchContainer`) which unpacks the V1 protobuf into fields
> which can be passed into a common function (i.e. `_launchContainer`).
> 
> The common functions authorize based on the type of container being
> launched and it is possible to use both Standalone and Nested
> Container APIs interchangably for nested containers.
> 
> This approach is somewhat messy for for the `WAIT_(NESTED_)CONTAINER`
> calls, as these methods require different return protobufs based on
> the original call.
> 
> 
> Diffs
> -
> 
>   src/slave/http.hpp 44a95dec4c9b8bb65d712c5538bbd7afffe2cf7b 
>   src/slave/http.cpp 22cdac97e1a8a28ca149043ffa1d0646073d7fdb 
> 
> 
> Diff: https://reviews.apache.org/r/62145/diff/5/
> 
> 
> Testing
> ---
> 
> See later in chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 62145: Implemented Standalone Container API.

[Joseph Wu]

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

(Updated Nov. 13, 2017, 5:32 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

* Changed AuthZ to use the AuthorizationAcceptor pattern.
* Removed the `user` fields from AuthZ objects due to decision to reduce 
granularity of AuthZ.
* Combined `__launchContainer` into `_launchContainer`.
* Made failure to chown standalone sandbox into a 500 error.


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


Repository: mesos


Description
---

The Standalone and Nested Container APIs are very similar.
This commit combines the two API implementations by adding a
translation function (i.e. `launchNestedContainer` and 
`launchContainer`) which unpacks the V1 protobuf into fields
which can be passed into a common function (i.e. `_launchContainer`).

The common functions authorize based on the type of container being
launched and it is possible to use both Standalone and Nested
Container APIs interchangably for nested containers.

This approach is somewhat messy for for the `WAIT_(NESTED_)CONTAINER`
calls, as these methods require different return protobufs based on
the original call.


Diffs (updated)
-

  src/slave/http.hpp 44a95dec4c9b8bb65d712c5538bbd7afffe2cf7b 
  src/slave/http.cpp 22cdac97e1a8a28ca149043ffa1d0646073d7fdb 


Diff: https://reviews.apache.org/r/62145/diff/5/

Changes: https://reviews.apache.org/r/62145/diff/4-5/


Testing
---

See later in chain.


Thanks,

Joseph Wu

Re: Review Request 62145: Implemented Standalone Container API.

[Joseph Wu]

> On Nov. 8, 2017, 10:28 a.m., Jie Yu wrote:
> > src/slave/http.cpp
> > Line 2359 (original), 2420-2423 (patched)
> > 
> >
> > I think we should only try to set 'user' if agent `switch_user` flag is 
> > set.
> > 
> > ```
> > if (slave->flags.switch_user) {
> >   if (commandInfo.has_user()) {
> > containerUser = commandInfo.user();
> >   }
> >   
> >   // If 'user' is not specified for standalone container,
> >   // it'll be the same as not switching user in containerizer.
> >   // No need to call os::user() here.
> > }
> > ```

As discussed offline, since we're making the AuthZ into ANY or NONE, some of 
this can go away.


> On Nov. 8, 2017, 10:28 a.m., Jie Yu wrote:
> > src/slave/http.cpp
> > Lines 2510 (patched)
> > 
> >
> > maybe try to remove the sandbox created above and return a 
> > InternalServerError()?

This is mirroring the logic of sandbox creation for normal top-level containers:
https://github.com/apache/mesos/blob/a595bcbf7afd9783e15f3e32cd9e70fa979531df/src/slave/paths.cpp#L613-L617

We don't fail the launch if the sandbox chown fails.

However, since this API is more user facing, it makes sense to fail here.


- Joseph


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


On Nov. 2, 2017, 8:57 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62145/
> ---
> 
> (Updated Nov. 2, 2017, 8:57 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7305
> https://issues.apache.org/jira/browse/MESOS-7305
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The Standalone and Nested Container APIs are very similar.
> This commit combines the two API implementations by adding a
> translation function (i.e. `launchNestedContainer` and 
> `launchContainer`) which unpacks the V1 protobuf into fields
> which can be passed into a common function (i.e. `_launchContainer`).
> 
> The common functions authorize based on the type of container being
> launched and it is possible to use both Standalone and Nested
> Container APIs interchangably for nested containers.
> 
> This approach is somewhat messy for for the `WAIT_(NESTED_)CONTAINER`
> calls, as these methods require different return protobufs based on
> the original call.
> 
> 
> Diffs
> -
> 
>   src/slave/http.hpp 44a95dec4c9b8bb65d712c5538bbd7afffe2cf7b 
>   src/slave/http.cpp f2e06aff95e0628624b6ed25de222fd3f3577a0b 
> 
> 
> Diff: https://reviews.apache.org/r/62145/diff/4/
> 
> 
> Testing
> ---
> 
> See later in chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 62145: Implemented Standalone Container API.

[Jie Yu]

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




src/slave/http.cpp
Line 2359 (original), 2420-2423 (patched)


I think we should only try to set 'user' if agent `switch_user` flag is set.

```
if (slave->flags.switch_user) {
  if (commandInfo.has_user()) {
containerUser = commandInfo.user();
  }
  
  // If 'user' is not specified for standalone container,
  // it'll be the same as not switching user in containerizer.
  // No need to call os::user() here.
}
```



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


```
containerUser = executor->user;

if (slave->flags.switch_user && commandInfo.has_user()) {
  containerUser = commandInfo.user();
}
```



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


Instead of passing a default user, i'd calculate that in the caller 
(`_launchContainer`) so that the contaienr user calculation is done in one 
place.



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


maybe try to remove the sandbox created above and return a 
InternalServerError()?



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


I am not sure if this is going to work. Looks like the way we do 
authorization for wait_nested_contianer is very special. It basically check if 
the top level container id matches or not.

For standalone container, that means somehow we need to use the same JWT 
token based authentication and set `cid` claim properly? is that what we plan 
to do?

I am not super familiar with the code around that. You may want to check 
with Greg on this?


- Jie Yu


On Nov. 2, 2017, 3:57 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62145/
> ---
> 
> (Updated Nov. 2, 2017, 3:57 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7305
> https://issues.apache.org/jira/browse/MESOS-7305
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The Standalone and Nested Container APIs are very similar.
> This commit combines the two API implementations by adding a
> translation function (i.e. `launchNestedContainer` and 
> `launchContainer`) which unpacks the V1 protobuf into fields
> which can be passed into a common function (i.e. `_launchContainer`).
> 
> The common functions authorize based on the type of container being
> launched and it is possible to use both Standalone and Nested
> Container APIs interchangably for nested containers.
> 
> This approach is somewhat messy for for the `WAIT_(NESTED_)CONTAINER`
> calls, as these methods require different return protobufs based on
> the original call.
> 
> 
> Diffs
> -
> 
>   src/slave/http.hpp 44a95dec4c9b8bb65d712c5538bbd7afffe2cf7b 
>   src/slave/http.cpp f2e06aff95e0628624b6ed25de222fd3f3577a0b 
> 
> 
> Diff: https://reviews.apache.org/r/62145/diff/4/
> 
> 
> Testing
> ---
> 
> See later in chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 62145: Implemented Standalone Container API.

[Joseph Wu]

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

(Updated Nov. 2, 2017, 8:57 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Updated implementation of `WaitContainer` to match `WaitNestedContainer`.


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


Repository: mesos


Description
---

The Standalone and Nested Container APIs are very similar.
This commit combines the two API implementations by adding a
translation function (i.e. `launchNestedContainer` and 
`launchContainer`) which unpacks the V1 protobuf into fields
which can be passed into a common function (i.e. `_launchContainer`).

The common functions authorize based on the type of container being
launched and it is possible to use both Standalone and Nested
Container APIs interchangably for nested containers.

This approach is somewhat messy for for the `WAIT_(NESTED_)CONTAINER`
calls, as these methods require different return protobufs based on
the original call.


Diffs (updated)
-

  src/slave/http.hpp 44a95dec4c9b8bb65d712c5538bbd7afffe2cf7b 
  src/slave/http.cpp f2e06aff95e0628624b6ed25de222fd3f3577a0b 


Diff: https://reviews.apache.org/r/62145/diff/4/

Changes: https://reviews.apache.org/r/62145/diff/3-4/


Testing
---

See later in chain.


Thanks,

Joseph Wu

Re: Review Request 62145: Implemented Standalone Container API.

[Joseph Wu]

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

(Updated Oct. 16, 2017, 5:02 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Fairly major change (the diff of diffs is not informative).  This approach has 
slightly less code duplication and makes it possible to use the Nested 
Container API and Standalone Container API interchangably for nested containers.


Summary (updated)
-

Implemented Standalone Container API.


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


Repository: mesos


Description (updated)
---

The Standalone and Nested Container APIs are very similar.
This commit combines the two API implementations by adding a
translation function (i.e. `launchNestedContainer` and 
`launchContainer`) which unpacks the V1 protobuf into fields
which can be passed into a common function (i.e. `_launchContainer`).

The common functions authorize based on the type of container being
launched and it is possible to use both Standalone and Nested
Container APIs interchangably for nested containers.

This approach is somewhat messy for for the `WAIT_(NESTED_)CONTAINER`
calls, as these methods require different return protobufs based on
the original call.


Diffs (updated)
-

  src/slave/http.hpp 44a95dec4c9b8bb65d712c5538bbd7afffe2cf7b 
  src/slave/http.cpp f2e06aff95e0628624b6ed25de222fd3f3577a0b 


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

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


Testing
---

TODO


Thanks,

Joseph Wu