You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Chun-Hung Hsiao <ch...@mesosphere.io> on 2017/10/16 23:09:19 UTC
Re: Review Request 62762: Added `LocalResourceProvider::principal()`
for authentication.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62762/
-----------------------------------------------------------
(Updated Oct. 16, 2017, 11:09 p.m.)
Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
Changes
-------
Refactored prefix generation.
Bugs: MESOS-8100
https://issues.apache.org/jira/browse/MESOS-8100
Repository: mesos
Description
-------
The `LocalResourceProvider::principal()` function takes a
`ResourceProviderInfo` and generates a principal with the following
claim:
{"cid_prefix", name prefix for the resource provider}
where the name prefix is a unique prefix for all containers that will be
launched by the resource provider. For example, for resource provider
with type 'org.apache.rp.local.storage' and name 'foo-bar', the claim
would be:
{"cid_prefix", "mesos-rp-local-storage-foo%2Dbar-"}
In the future, we could add more claims for authorizing other
operations, such as authorization for Resource Provider API.
Diffs (updated)
-----
src/resource_provider/local.hpp ebaa07d03ad77d516066ee2d4b60864be0611b5f
src/resource_provider/local.cpp ad98f333c5668ca81de6e7ed3fc8f59323b151da
src/resource_provider/storage/provider.hpp 6de88c2329b358fcf48bc39ddda0132170991c3c
src/resource_provider/storage/provider.cpp 46224997430ac0c568904d80014166a6f059907f
Diff: https://reviews.apache.org/r/62762/diff/3/
Changes: https://reviews.apache.org/r/62762/diff/2-3/
Testing
-------
make
Thanks,
Chun-Hung Hsiao
Re: Review Request 62762: Added `LocalResourceProvider::principal()`
for authentication.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62762/#review190262
-----------------------------------------------------------
configure.ac
Lines 2020-2021 (patched)
<https://reviews.apache.org/r/62762/#comment267563>
This definition probably belongs in a separate review. Along with the other build changes related to this flag, in this review.
- Joseph Wu
On Nov. 3, 2017, 5:49 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62762/
> -----------------------------------------------------------
>
> (Updated Nov. 3, 2017, 5:49 p.m.)
>
>
> Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
>
>
> Bugs: MESOS-8100
> https://issues.apache.org/jira/browse/MESOS-8100
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `LocalResourceProvider::principal()` function takes a
> `ResourceProviderInfo` and generates a principal with the following
> claim:
>
> {"cid_prefix", name prefix for the resource provider}
>
> where the name prefix is a unique prefix for all containers that will be
> launched by the resource provider. For example, for resource provider
> with type 'org.apache.rp.local.storage' and name 'foo-bar', the claim
> would be:
>
> {"cid_prefix", "mesos-slrp-foo%2Dbar-"}
>
> In the future, we could add more claims for authorizing other
> operations, such as authorization for Resource Provider API.
>
>
> Diffs
> -----
>
> configure.ac 067005e1be3a09e8320be120c8784f195e30ac5d
> src/Makefile.am 1c97b1fd8151f87c4e9e6d62884b0ef7d582c312
> src/resource_provider/local.hpp ebaa07d03ad77d516066ee2d4b60864be0611b5f
> src/resource_provider/local.cpp ad98f333c5668ca81de6e7ed3fc8f59323b151da
> src/resource_provider/storage/provider.hpp 6de88c2329b358fcf48bc39ddda0132170991c3c
> src/resource_provider/storage/provider.cpp 46224997430ac0c568904d80014166a6f059907f
>
>
> Diff: https://reviews.apache.org/r/62762/diff/5/
>
>
> Testing
> -------
>
> make
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 62762: Added `LocalResourceProvider::principal()`
for authentication.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62762/#review191689
-----------------------------------------------------------
Fix it, then Ship it!
src/resource_provider/local.cpp
Lines 46-47 (patched)
<https://reviews.apache.org/r/62762/#comment269547>
`#if defined(ENABLE_GRPC) && defined(__linux__)`
src/resource_provider/storage/provider.cpp
Lines 57 (patched)
<https://reviews.apache.org/r/62762/#comment269543>
Let's use `org.apache.mesos.rp.local.storage` here (rp.type)
src/resource_provider/storage/provider.cpp
Lines 62 (patched)
<https://reviews.apache.org/r/62762/#comment269545>
Let's verify that `-` is not allowed in either `type` or `name` because we say in the API that this should be in java package naming convension.
- Jie Yu
On Nov. 21, 2017, 12:07 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62762/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2017, 12:07 a.m.)
>
>
> Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
>
>
> Bugs: MESOS-8100
> https://issues.apache.org/jira/browse/MESOS-8100
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `LocalResourceProvider::principal()` function takes a
> `ResourceProviderInfo` and generates a principal with the following
> claim:
>
> {"cid_prefix", name prefix for the resource provider}
>
> where the name prefix is a unique prefix for all containers that will be
> launched by the resource provider. For example, for resource provider
> with type 'org.apache.rp.local.storage' and name 'foo', the claim
> would be:
>
> {"cid_prefix", "mesos-slrp-foo-"}
>
> In the future, we could add more claims for authorizing other
> operations, such as authorization for Resource Provider API.
>
>
> Diffs
> -----
>
> src/resource_provider/local.hpp ebaa07d03ad77d516066ee2d4b60864be0611b5f
> src/resource_provider/local.cpp ad98f333c5668ca81de6e7ed3fc8f59323b151da
> src/resource_provider/storage/provider.hpp 6de88c2329b358fcf48bc39ddda0132170991c3c
> src/resource_provider/storage/provider.cpp 46224997430ac0c568904d80014166a6f059907f
>
>
> Diff: https://reviews.apache.org/r/62762/diff/6/
>
>
> Testing
> -------
>
> make
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 62762: Added `LocalResourceProvider::principal()`
for authentication.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62762/#review191690
-----------------------------------------------------------
src/resource_provider/storage/provider.cpp
Lines 65-66 (patched)
<https://reviews.apache.org/r/62762/#comment269549>
Adjust the commnent here.
src/resource_provider/storage/provider.cpp
Lines 67 (patched)
<https://reviews.apache.org/r/62762/#comment269550>
s/getPrefix/getContainerIdPrefix/
- Jie Yu
On Nov. 21, 2017, 12:07 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62762/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2017, 12:07 a.m.)
>
>
> Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
>
>
> Bugs: MESOS-8100
> https://issues.apache.org/jira/browse/MESOS-8100
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `LocalResourceProvider::principal()` function takes a
> `ResourceProviderInfo` and generates a principal with the following
> claim:
>
> {"cid_prefix", name prefix for the resource provider}
>
> where the name prefix is a unique prefix for all containers that will be
> launched by the resource provider. For example, for resource provider
> with type 'org.apache.rp.local.storage' and name 'foo', the claim
> would be:
>
> {"cid_prefix", "mesos-slrp-foo-"}
>
> In the future, we could add more claims for authorizing other
> operations, such as authorization for Resource Provider API.
>
>
> Diffs
> -----
>
> src/resource_provider/local.hpp ebaa07d03ad77d516066ee2d4b60864be0611b5f
> src/resource_provider/local.cpp ad98f333c5668ca81de6e7ed3fc8f59323b151da
> src/resource_provider/storage/provider.hpp 6de88c2329b358fcf48bc39ddda0132170991c3c
> src/resource_provider/storage/provider.cpp 46224997430ac0c568904d80014166a6f059907f
>
>
> Diff: https://reviews.apache.org/r/62762/diff/6/
>
>
> Testing
> -------
>
> make
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 62762: Added `LocalResourceProvider::principal()`
for authentication.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62762/
-----------------------------------------------------------
(Updated Nov. 23, 2017, 9:41 a.m.)
Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
Changes
-------
Addressed some of jieyu's comments.
Bugs: MESOS-8100
https://issues.apache.org/jira/browse/MESOS-8100
Repository: mesos
Description
-------
The `LocalResourceProvider::principal()` function takes a
`ResourceProviderInfo` and generates a principal with the following
claim:
{"cid_prefix", <type-with-dots-replaced-by-dashes>-<name>--}
For example, for resource provider with type
'org.apache.mesos.rp.local.storage' and name 'foo', the claim would be:
{"cid_prefix", "org-apache-mesos-rp-local-storage-foo--"}
In the future, we could add more claims for authorizing other
operations, such as authorization for Resource Provider API.
Diffs (updated)
-----
src/resource_provider/local.hpp ebaa07d03ad77d516066ee2d4b60864be0611b5f
src/resource_provider/local.cpp ad98f333c5668ca81de6e7ed3fc8f59323b151da
src/resource_provider/storage/provider.hpp 6de88c2329b358fcf48bc39ddda0132170991c3c
src/resource_provider/storage/provider.cpp 46224997430ac0c568904d80014166a6f059907f
Diff: https://reviews.apache.org/r/62762/diff/9/
Changes: https://reviews.apache.org/r/62762/diff/8-9/
Testing
-------
make
Thanks,
Chun-Hung Hsiao
Re: Review Request 62762: Added `LocalResourceProvider::principal()`
for authentication.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
> On Nov. 23, 2017, 5:27 a.m., Jie Yu wrote:
> > src/resource_provider/local.cpp
> > Lines 45 (patched)
> > <https://reviews.apache.org/r/62762/diff/8/?file=1899402#file1899402line45>
> >
> > We try to avoid non-POD (plain old data) global variables because it'll cause issues during exit (see google style guide).
> >
> > So I'll make this a helper function.
I'm thinking about following this style: https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/containerizer.cpp#L360
Let me update the patch and please see if that is better.
> On Nov. 23, 2017, 5:27 a.m., Jie Yu wrote:
> > src/resource_provider/local.cpp
> > Lines 49 (patched)
> > <https://reviews.apache.org/r/62762/diff/8/?file=1899402#file1899402line49>
> >
> > Do we still need this? It looks to me that the current naming scheme makes it so that you don't have to customize it for each LRP?
This is for future proof. What if a new LRP needs to use some other API?
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62762/#review191779
-----------------------------------------------------------
On Nov. 22, 2017, 3:10 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62762/
> -----------------------------------------------------------
>
> (Updated Nov. 22, 2017, 3:10 a.m.)
>
>
> Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
>
>
> Bugs: MESOS-8100
> https://issues.apache.org/jira/browse/MESOS-8100
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `LocalResourceProvider::principal()` function takes a
> `ResourceProviderInfo` and generates a principal with the following
> claim:
>
> {"cid_prefix", <type-with-dots-replaced-by-dashes>-<name>--}
>
> For example, for resource provider with type
> 'org.apache.mesos.rp.local.storage' and name 'foo', the claim would be:
>
> {"cid_prefix", "org-apache-mesos-rp-local-storage-foo--"}
>
> In the future, we could add more claims for authorizing other
> operations, such as authorization for Resource Provider API.
>
>
> Diffs
> -----
>
> src/resource_provider/local.hpp ebaa07d03ad77d516066ee2d4b60864be0611b5f
> src/resource_provider/local.cpp ad98f333c5668ca81de6e7ed3fc8f59323b151da
> src/resource_provider/storage/provider.hpp 6de88c2329b358fcf48bc39ddda0132170991c3c
> src/resource_provider/storage/provider.cpp 46224997430ac0c568904d80014166a6f059907f
>
>
> Diff: https://reviews.apache.org/r/62762/diff/8/
>
>
> Testing
> -------
>
> make
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 62762: Added `LocalResourceProvider::principal()`
for authentication.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62762/#review191779
-----------------------------------------------------------
src/resource_provider/local.cpp
Lines 35-40 (patched)
<https://reviews.apache.org/r/62762/#comment269692>
see my comments below, you probably do not need this struct anymore.
src/resource_provider/local.cpp
Lines 45 (patched)
<https://reviews.apache.org/r/62762/#comment269690>
We try to avoid non-POD (plain old data) global variables because it'll cause issues during exit (see google style guide).
So I'll make this a helper function.
src/resource_provider/local.cpp
Lines 49 (patched)
<https://reviews.apache.org/r/62762/#comment269691>
Do we still need this? It looks to me that the current naming scheme makes it so that you don't have to customize it for each LRP?
- Jie Yu
On Nov. 22, 2017, 3:10 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62762/
> -----------------------------------------------------------
>
> (Updated Nov. 22, 2017, 3:10 a.m.)
>
>
> Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
>
>
> Bugs: MESOS-8100
> https://issues.apache.org/jira/browse/MESOS-8100
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `LocalResourceProvider::principal()` function takes a
> `ResourceProviderInfo` and generates a principal with the following
> claim:
>
> {"cid_prefix", <type-with-dots-replaced-by-dashes>-<name>--}
>
> For example, for resource provider with type
> 'org.apache.mesos.rp.local.storage' and name 'foo', the claim would be:
>
> {"cid_prefix", "org-apache-mesos-rp-local-storage-foo--"}
>
> In the future, we could add more claims for authorizing other
> operations, such as authorization for Resource Provider API.
>
>
> Diffs
> -----
>
> src/resource_provider/local.hpp ebaa07d03ad77d516066ee2d4b60864be0611b5f
> src/resource_provider/local.cpp ad98f333c5668ca81de6e7ed3fc8f59323b151da
> src/resource_provider/storage/provider.hpp 6de88c2329b358fcf48bc39ddda0132170991c3c
> src/resource_provider/storage/provider.cpp 46224997430ac0c568904d80014166a6f059907f
>
>
> Diff: https://reviews.apache.org/r/62762/diff/8/
>
>
> Testing
> -------
>
> make
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 62762: Added `LocalResourceProvider::principal()`
for authentication.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62762/
-----------------------------------------------------------
(Updated Nov. 22, 2017, 3:10 a.m.)
Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
Changes
-------
Addde more comments to explain the naming convention for container IDs.
Bugs: MESOS-8100
https://issues.apache.org/jira/browse/MESOS-8100
Repository: mesos
Description
-------
The `LocalResourceProvider::principal()` function takes a
`ResourceProviderInfo` and generates a principal with the following
claim:
{"cid_prefix", <type-with-dots-replaced-by-dashes>-<name>--}
For example, for resource provider with type
'org.apache.mesos.rp.local.storage' and name 'foo', the claim would be:
{"cid_prefix", "org-apache-mesos-rp-local-storage-foo--"}
In the future, we could add more claims for authorizing other
operations, such as authorization for Resource Provider API.
Diffs (updated)
-----
src/resource_provider/local.hpp ebaa07d03ad77d516066ee2d4b60864be0611b5f
src/resource_provider/local.cpp ad98f333c5668ca81de6e7ed3fc8f59323b151da
src/resource_provider/storage/provider.hpp 6de88c2329b358fcf48bc39ddda0132170991c3c
src/resource_provider/storage/provider.cpp 46224997430ac0c568904d80014166a6f059907f
Diff: https://reviews.apache.org/r/62762/diff/8/
Changes: https://reviews.apache.org/r/62762/diff/7-8/
Testing
-------
make
Thanks,
Chun-Hung Hsiao
Re: Review Request 62762: Added `LocalResourceProvider::principal()`
for authentication.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62762/
-----------------------------------------------------------
(Updated Nov. 22, 2017, 2:46 a.m.)
Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
Changes
-------
Addressed Jie's comments.
Bugs: MESOS-8100
https://issues.apache.org/jira/browse/MESOS-8100
Repository: mesos
Description (updated)
-------
The `LocalResourceProvider::principal()` function takes a
`ResourceProviderInfo` and generates a principal with the following
claim:
{"cid_prefix", <type-with-dots-replaced-by-dashes>-<name>--}
For example, for resource provider with type
'org.apache.mesos.rp.local.storage' and name 'foo', the claim would be:
{"cid_prefix", "org-apache-mesos-rp-local-storage-foo--"}
In the future, we could add more claims for authorizing other
operations, such as authorization for Resource Provider API.
Diffs (updated)
-----
src/resource_provider/local.hpp ebaa07d03ad77d516066ee2d4b60864be0611b5f
src/resource_provider/local.cpp ad98f333c5668ca81de6e7ed3fc8f59323b151da
src/resource_provider/storage/provider.hpp 6de88c2329b358fcf48bc39ddda0132170991c3c
src/resource_provider/storage/provider.cpp 46224997430ac0c568904d80014166a6f059907f
Diff: https://reviews.apache.org/r/62762/diff/7/
Changes: https://reviews.apache.org/r/62762/diff/6-7/
Testing
-------
make
Thanks,
Chun-Hung Hsiao
Re: Review Request 62762: Added `LocalResourceProvider::principal()`
for authentication.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62762/
-----------------------------------------------------------
(Updated Nov. 21, 2017, 12:07 a.m.)
Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
Changes
-------
Addressed kaysoky's comments.
Bugs: MESOS-8100
https://issues.apache.org/jira/browse/MESOS-8100
Repository: mesos
Description (updated)
-------
The `LocalResourceProvider::principal()` function takes a
`ResourceProviderInfo` and generates a principal with the following
claim:
{"cid_prefix", name prefix for the resource provider}
where the name prefix is a unique prefix for all containers that will be
launched by the resource provider. For example, for resource provider
with type 'org.apache.rp.local.storage' and name 'foo', the claim
would be:
{"cid_prefix", "mesos-slrp-foo-"}
In the future, we could add more claims for authorizing other
operations, such as authorization for Resource Provider API.
Diffs (updated)
-----
src/resource_provider/local.hpp ebaa07d03ad77d516066ee2d4b60864be0611b5f
src/resource_provider/local.cpp ad98f333c5668ca81de6e7ed3fc8f59323b151da
src/resource_provider/storage/provider.hpp 6de88c2329b358fcf48bc39ddda0132170991c3c
src/resource_provider/storage/provider.cpp 46224997430ac0c568904d80014166a6f059907f
Diff: https://reviews.apache.org/r/62762/diff/6/
Changes: https://reviews.apache.org/r/62762/diff/5-6/
Testing
-------
make
Thanks,
Chun-Hung Hsiao
Re: Review Request 62762: Added `LocalResourceProvider::principal()`
for authentication.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62762/
-----------------------------------------------------------
(Updated Nov. 4, 2017, 12:49 a.m.)
Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
Changes
-------
Made storage local resource provider only compiled on Linux with GRPC enabled.
Bugs: MESOS-8100
https://issues.apache.org/jira/browse/MESOS-8100
Repository: mesos
Description (updated)
-------
The `LocalResourceProvider::principal()` function takes a
`ResourceProviderInfo` and generates a principal with the following
claim:
{"cid_prefix", name prefix for the resource provider}
where the name prefix is a unique prefix for all containers that will be
launched by the resource provider. For example, for resource provider
with type 'org.apache.rp.local.storage' and name 'foo-bar', the claim
would be:
{"cid_prefix", "mesos-slrp-foo%2Dbar-"}
In the future, we could add more claims for authorizing other
operations, such as authorization for Resource Provider API.
Diffs (updated)
-----
configure.ac 067005e1be3a09e8320be120c8784f195e30ac5d
src/Makefile.am 1c97b1fd8151f87c4e9e6d62884b0ef7d582c312
src/resource_provider/local.hpp ebaa07d03ad77d516066ee2d4b60864be0611b5f
src/resource_provider/local.cpp ad98f333c5668ca81de6e7ed3fc8f59323b151da
src/resource_provider/storage/provider.hpp 6de88c2329b358fcf48bc39ddda0132170991c3c
src/resource_provider/storage/provider.cpp 46224997430ac0c568904d80014166a6f059907f
Diff: https://reviews.apache.org/r/62762/diff/5/
Changes: https://reviews.apache.org/r/62762/diff/4-5/
Testing
-------
make
Thanks,
Chun-Hung Hsiao