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:13:26 UTC
Re: Review Request 62636: Generated authentication tokens for local
resource providers.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62636/
-----------------------------------------------------------
(Updated Oct. 16, 2017, 11:13 p.m.)
Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
Summary (updated)
-----------------
Generated authentication tokens for local resource providers.
Bugs: MESOS-8100
https://issues.apache.org/jira/browse/MESOS-8100
Repository: mesos
Description
-------
`LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
generate an authentication token for each local resource provider. The
authentication token can then be used to call the V1 agent API. In order
to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
asynchronous function.
Diffs (updated)
-----
src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263
src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed
src/slave/slave.hpp aea1e948209c7c8945665915bc2f6d8eb47814ef
src/slave/slave.cpp c35cf7d72c66bf7cdfd7efa322bd7d73c923fac9
src/tests/mock_slave.cpp db24f9e5b71f558d2f811f0da8a9cc9c7c2dd341
Diff: https://reviews.apache.org/r/62636/diff/3/
Changes: https://reviews.apache.org/r/62636/diff/2-3/
Testing
-------
make
Thanks,
Chun-Hung Hsiao
Re: Review Request 62636: Generated authentication tokens for local
resource providers.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
> On Nov. 7, 2017, 2:30 a.m., Joseph Wu wrote:
> > src/resource_provider/daemon.cpp
> > Lines 96-99 (original), 114-118 (patched)
> > <https://reviews.apache.org/r/62636/diff/5/?file=1880935#file1880935line114>
> >
> > I'd consider an error at this step to be a fatal error (basically a misconfiguration), so we should fail-fast here and exit the agent.
Fixed in r63376.
> On Nov. 7, 2017, 2:30 a.m., Joseph Wu wrote:
> > src/slave/slave.cpp
> > Line 1234 (original), 1234-1236 (patched)
> > <https://reviews.apache.org/r/62636/diff/5/?file=1880936#file1880936line1234>
> >
> > Consider moving this comment above `start` in `daemon.hpp`. (With the side benefit of not needing to duplicate the comment in two places)
Done. This makes sense to me, as the caller would go to the header file to check why we need this argument.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62636/#review190257
-----------------------------------------------------------
On Nov. 17, 2017, 12:19 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62636/
> -----------------------------------------------------------
>
> (Updated Nov. 17, 2017, 12:19 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
> -------
>
> `LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
> generate an authentication token for each local resource provider. The
> authentication token can then be used to call the V1 agent API. In order
> to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
> asynchronous function.
>
>
> Diffs
> -----
>
> src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263
> src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed
> src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0
>
>
> Diff: https://reviews.apache.org/r/62636/diff/6/
>
>
> Testing
> -------
>
> make
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 62636: Generated authentication tokens for local
resource providers.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62636/#review190257
-----------------------------------------------------------
You could probably combine this review with https://reviews.apache.org/r/62633/ , as that review is basically just plumbing the auth token from Daemon -> Local -> SLRP. And this review adds plumbing of the token (via the secret generator) from Agent -> Daemon.
src/resource_provider/daemon.cpp
Lines 96-99 (original), 114-118 (patched)
<https://reviews.apache.org/r/62636/#comment267562>
I'd consider an error at this step to be a fatal error (basically a misconfiguration), so we should fail-fast here and exit the agent.
src/resource_provider/daemon.cpp
Lines 207 (patched)
<https://reviews.apache.org/r/62636/#comment267564>
s/insteaf/instead/
src/slave/slave.cpp
Line 1234 (original), 1234-1236 (patched)
<https://reviews.apache.org/r/62636/#comment267557>
Consider moving this comment above `start` in `daemon.hpp`. (With the side benefit of not needing to duplicate the comment in two places)
- Joseph Wu
On Nov. 3, 2017, 5:51 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62636/
> -----------------------------------------------------------
>
> (Updated Nov. 3, 2017, 5:51 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
> -------
>
> `LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
> generate an authentication token for each local resource provider. The
> authentication token can then be used to call the V1 agent API. In order
> to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
> asynchronous function.
>
>
> Diffs
> -----
>
> src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263
> src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed
> src/slave/slave.cpp 337083dbe60bba2d3773b785bdceeaf0b8fcd070
>
>
> Diff: https://reviews.apache.org/r/62636/diff/5/
>
>
> Testing
> -------
>
> make
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 62636: Generated authentication tokens for local
resource providers.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62636/#review191816
-----------------------------------------------------------
Fix it, then Ship it!
Let's follow up with tests later this chain. I understand that it's pretty hard to just test this patch.
src/resource_provider/daemon.cpp
Lines 78 (patched)
<https://reviews.apache.org/r/62636/#comment269745>
I'd prefer we pass in `secretGenerator` during daemon initialization, instead of passing it in during start.
- Jie Yu
On Nov. 23, 2017, 10:10 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62636/
> -----------------------------------------------------------
>
> (Updated Nov. 23, 2017, 10: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
> -------
>
> `LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
> generate an authentication token for each local resource provider. The
> authentication token can then be used to call the V1 agent API. In order
> to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
> asynchronous function.
>
>
> Diffs
> -----
>
> src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263
> src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed
> 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
> src/slave/slave.cpp 6e9adc60f593faf1b0e56caeea04882f67af7080
>
>
> Diff: https://reviews.apache.org/r/62636/diff/9/
>
>
> Testing
> -------
>
> make
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 62636: Generated authentication tokens for local
resource providers.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62636/#review192191
-----------------------------------------------------------
Two tests has been added:
https://reviews.apache.org/r/64176
https://reviews.apache.org/r/64177
- Jie Yu
On Nov. 29, 2017, 7:35 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62636/
> -----------------------------------------------------------
>
> (Updated Nov. 29, 2017, 7:35 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
> -------
>
> `LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
> generate an authentication token for each local resource provider. The
> authentication token can then be used to call the V1 agent API. In order
> to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
> asynchronous function.
>
>
> Diffs
> -----
>
> src/resource_provider/daemon.hpp 8668cd1c4c0be79e8e18371744c9f043129f30c5
> src/resource_provider/daemon.cpp 80b5c71e266bda2f15d5dfb9b1c15f01c6aa8e93
> 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 49c042cdb1837860aaedde2e48f318ed5ac8b1d1
> src/slave/slave.cpp e1566832f90cca372ad2f1cc13d1e7f76fa53285
>
>
> Diff: https://reviews.apache.org/r/62636/diff/12/
>
>
> Testing
> -------
>
> make
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 62636: Generated authentication tokens for local
resource providers.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62636/
-----------------------------------------------------------
(Updated Nov. 29, 2017, 7:35 p.m.)
Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
Changes
-------
Started the LRP daemon once the agent is running.
Bugs: MESOS-8100
https://issues.apache.org/jira/browse/MESOS-8100
Repository: mesos
Description
-------
`LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
generate an authentication token for each local resource provider. The
authentication token can then be used to call the V1 agent API. In order
to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
asynchronous function.
Diffs (updated)
-----
src/resource_provider/daemon.hpp 8668cd1c4c0be79e8e18371744c9f043129f30c5
src/resource_provider/daemon.cpp 80b5c71e266bda2f15d5dfb9b1c15f01c6aa8e93
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 49c042cdb1837860aaedde2e48f318ed5ac8b1d1
src/slave/slave.cpp e1566832f90cca372ad2f1cc13d1e7f76fa53285
Diff: https://reviews.apache.org/r/62636/diff/12/
Changes: https://reviews.apache.org/r/62636/diff/11-12/
Testing
-------
make
Thanks,
Chun-Hung Hsiao
Re: Review Request 62636: Generated authentication tokens for local
resource providers.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62636/
-----------------------------------------------------------
(Updated Nov. 29, 2017, 8:15 a.m.)
Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
Changes
-------
Passed `secretGenerator` during daemon creation.
Bugs: MESOS-8100
https://issues.apache.org/jira/browse/MESOS-8100
Repository: mesos
Description
-------
`LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
generate an authentication token for each local resource provider. The
authentication token can then be used to call the V1 agent API. In order
to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
asynchronous function.
Diffs (updated)
-----
src/resource_provider/daemon.hpp 8668cd1c4c0be79e8e18371744c9f043129f30c5
src/resource_provider/daemon.cpp 80b5c71e266bda2f15d5dfb9b1c15f01c6aa8e93
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 49c042cdb1837860aaedde2e48f318ed5ac8b1d1
src/slave/slave.cpp e1566832f90cca372ad2f1cc13d1e7f76fa53285
Diff: https://reviews.apache.org/r/62636/diff/11/
Changes: https://reviews.apache.org/r/62636/diff/10-11/
Testing
-------
make
Thanks,
Chun-Hung Hsiao
Re: Review Request 62636: Generated authentication tokens for local
resource providers.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62636/
-----------------------------------------------------------
(Updated Nov. 27, 2017, 9:26 p.m.)
Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
Changes
-------
Rebased.
Bugs: MESOS-8100
https://issues.apache.org/jira/browse/MESOS-8100
Repository: mesos
Description
-------
`LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
generate an authentication token for each local resource provider. The
authentication token can then be used to call the V1 agent API. In order
to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
asynchronous function.
Diffs (updated)
-----
src/resource_provider/daemon.hpp 8668cd1c4c0be79e8e18371744c9f043129f30c5
src/resource_provider/daemon.cpp 80b5c71e266bda2f15d5dfb9b1c15f01c6aa8e93
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 49c042cdb1837860aaedde2e48f318ed5ac8b1d1
src/slave/slave.cpp 6ed5c7887cf998b92cf3181b29cb6cf09cc73e61
Diff: https://reviews.apache.org/r/62636/diff/10/
Changes: https://reviews.apache.org/r/62636/diff/9-10/
Testing
-------
make
Thanks,
Chun-Hung Hsiao
Re: Review Request 62636: Generated authentication tokens for local
resource providers.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62636/
-----------------------------------------------------------
(Updated Nov. 23, 2017, 10:10 a.m.)
Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
Changes
-------
Rebased and addressed some of arojas' comments.
Bugs: MESOS-8100
https://issues.apache.org/jira/browse/MESOS-8100
Repository: mesos
Description
-------
`LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
generate an authentication token for each local resource provider. The
authentication token can then be used to call the V1 agent API. In order
to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
asynchronous function.
Diffs (updated)
-----
src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263
src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed
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
src/slave/slave.cpp 6e9adc60f593faf1b0e56caeea04882f67af7080
Diff: https://reviews.apache.org/r/62636/diff/9/
Changes: https://reviews.apache.org/r/62636/diff/8-9/
Testing
-------
make
Thanks,
Chun-Hung Hsiao
Re: Review Request 62636: Generated authentication tokens for local
resource providers.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
> On Nov. 22, 2017, 8:38 a.m., Alexander Rojas wrote:
> > src/resource_provider/daemon.cpp
> > Lines 78 (patched)
> > <https://reviews.apache.org/r/62636/diff/8/?file=1897918#file1897918line78>
> >
> > This line is not needed. Mostly for consistence reason, you don't see this pattern of `nullptr` initializing anything, in this case default constructing a pointer sets it to `nullptr` but also not adding it to the initializer list will default construct it to `nullptr`
I thought that for POD fields, including pointers, the default initialization does nothing: http://en.cppreference.com/w/cpp/language/default_initialization. Am I mistaken?
> On Nov. 22, 2017, 8:38 a.m., Alexander Rojas wrote:
> > src/resource_provider/daemon.cpp
> > Lines 112 (patched)
> > <https://reviews.apache.org/r/62636/diff/8/?file=1897918#file1897918line112>
> >
> > From the code here is not clear who owns the instance of `secretGenerator`. The fact that it is a private attribute tells me it is owned by this process, but it doesn't delete it on destruction.
> >
> > If you go for it, there should be a comment on why is it ok not to destroy this instance and why is not a memory leak, but really what you want is to use a managed pointer even if it is a const reference to an existing one.
> >
> > check `process::Owned`. I'm not so sure if we can now use `std::unique_ptr` or `std::shared_ptr`.
This one is passed from the `Slave::initialize()`, and is shared with the slave. The slave is the one responsible to manage its lifecycle. We probably need some refactor on this.
> On Nov. 22, 2017, 8:38 a.m., Alexander Rojas wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 104 (patched)
> > <https://reviews.apache.org/r/62636/diff/8/?file=1897922#file1897922line104>
> >
> > is there any situatio when the `authToken` is `None`?
Yes. When the authentication is not turned on.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62636/#review191707
-----------------------------------------------------------
On Nov. 21, 2017, 12:10 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62636/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2017, 12: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
> -------
>
> `LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
> generate an authentication token for each local resource provider. The
> authentication token can then be used to call the V1 agent API. In order
> to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
> asynchronous function.
>
>
> Diffs
> -----
>
> src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263
> src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed
> 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
> src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0
>
>
> Diff: https://reviews.apache.org/r/62636/diff/8/
>
>
> Testing
> -------
>
> make
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 62636: Generated authentication tokens for local
resource providers.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
> On Nov. 22, 2017, 8:38 a.m., Alexander Rojas wrote:
> > src/resource_provider/daemon.cpp
> > Lines 253 (patched)
> > <https://reviews.apache.org/r/62636/diff/8/?file=1897918#file1897918line253>
> >
> > This is a personal preference, but I don't see the reason to add more strings to a string which most likely already says it failed to validate a secret. You could just do `return error.get()`.
This is copied from `slave.cpp`.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62636/#review191707
-----------------------------------------------------------
On Nov. 21, 2017, 12:10 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62636/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2017, 12: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
> -------
>
> `LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
> generate an authentication token for each local resource provider. The
> authentication token can then be used to call the V1 agent API. In order
> to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
> asynchronous function.
>
>
> Diffs
> -----
>
> src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263
> src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed
> 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
> src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0
>
>
> Diff: https://reviews.apache.org/r/62636/diff/8/
>
>
> Testing
> -------
>
> make
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 62636: Generated authentication tokens for local
resource providers.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62636/#review191707
-----------------------------------------------------------
src/resource_provider/daemon.cpp
Lines 78 (patched)
<https://reviews.apache.org/r/62636/#comment269590>
This line is not needed. Mostly for consistence reason, you don't see this pattern of `nullptr` initializing anything, in this case default constructing a pointer sets it to `nullptr` but also not adding it to the initializer list will default construct it to `nullptr`
src/resource_provider/daemon.cpp
Lines 112 (patched)
<https://reviews.apache.org/r/62636/#comment269591>
From the code here is not clear who owns the instance of `secretGenerator`. The fact that it is a private attribute tells me it is owned by this process, but it doesn't delete it on destruction.
If you go for it, there should be a comment on why is it ok not to destroy this instance and why is not a memory leak, but really what you want is to use a managed pointer even if it is a const reference to an existing one.
check `process::Owned`. I'm not so sure if we can now use `std::unique_ptr` or `std::shared_ptr`.
src/resource_provider/daemon.cpp
Lines 237 (patched)
<https://reviews.apache.org/r/62636/#comment269593>
We don't make use of the booleaness of pointers. Replace with `secretGenerator == nullptr`.
Also Tthe code is more readable if you do:
```c++
if (secretGenerator == nullptr) {
return None();
}
...
```
Since you don't have to go to the end of the code to see what happens in the `else` case (which is only one line), and you save yourself one indentation, which with 80 characters is a great compromise.
src/resource_provider/daemon.cpp
Lines 253 (patched)
<https://reviews.apache.org/r/62636/#comment269594>
This is a personal preference, but I don't see the reason to add more strings to a string which most likely already says it failed to validate a secret. You could just do `return error.get()`.
src/resource_provider/storage/provider.cpp
Lines 104 (patched)
<https://reviews.apache.org/r/62636/#comment269595>
is there any situatio when the `authToken` is `None`?
- Alexander Rojas
On Nov. 21, 2017, 1:10 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62636/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2017, 1: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
> -------
>
> `LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
> generate an authentication token for each local resource provider. The
> authentication token can then be used to call the V1 agent API. In order
> to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
> asynchronous function.
>
>
> Diffs
> -----
>
> src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263
> src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed
> 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
> src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0
>
>
> Diff: https://reviews.apache.org/r/62636/diff/8/
>
>
> Testing
> -------
>
> make
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 62636: Generated authentication tokens for local
resource providers.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
> On Nov. 22, 2017, 8:40 a.m., Alexander Rojas wrote:
> > There are no tests anywhere in the chain. I disapprove of code written without unit tests. Please address that in the whole chain.
Will do. Need more time to add more tests :(
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62636/#review191708
-----------------------------------------------------------
On Nov. 21, 2017, 12:10 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62636/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2017, 12: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
> -------
>
> `LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
> generate an authentication token for each local resource provider. The
> authentication token can then be used to call the V1 agent API. In order
> to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
> asynchronous function.
>
>
> Diffs
> -----
>
> src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263
> src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed
> 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
> src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0
>
>
> Diff: https://reviews.apache.org/r/62636/diff/8/
>
>
> Testing
> -------
>
> make
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 62636: Generated authentication tokens for local
resource providers.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62636/#review191708
-----------------------------------------------------------
There are no tests anywhere in the chain. I disapprove of code written without unit tests. Please address that in the whole chain.
- Alexander Rojas
On Nov. 21, 2017, 1:10 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62636/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2017, 1: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
> -------
>
> `LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
> generate an authentication token for each local resource provider. The
> authentication token can then be used to call the V1 agent API. In order
> to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
> asynchronous function.
>
>
> Diffs
> -----
>
> src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263
> src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed
> 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
> src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0
>
>
> Diff: https://reviews.apache.org/r/62636/diff/8/
>
>
> Testing
> -------
>
> make
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 62636: Generated authentication tokens for local
resource providers.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62636/
-----------------------------------------------------------
(Updated Nov. 21, 2017, 12:10 a.m.)
Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
Changes
-------
Rebased on r63376.
Bugs: MESOS-8100
https://issues.apache.org/jira/browse/MESOS-8100
Repository: mesos
Description
-------
`LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
generate an authentication token for each local resource provider. The
authentication token can then be used to call the V1 agent API. In order
to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
asynchronous function.
Diffs (updated)
-----
src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263
src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed
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
src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0
Diff: https://reviews.apache.org/r/62636/diff/8/
Changes: https://reviews.apache.org/r/62636/diff/7-8/
Testing
-------
make
Thanks,
Chun-Hung Hsiao
Re: Review Request 62636: Generated authentication tokens for local
resource providers.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62636/
-----------------------------------------------------------
(Updated Nov. 17, 2017, 12:57 a.m.)
Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
Changes
-------
Combined with r62633.
Bugs: MESOS-8100
https://issues.apache.org/jira/browse/MESOS-8100
Repository: mesos
Description
-------
`LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
generate an authentication token for each local resource provider. The
authentication token can then be used to call the V1 agent API. In order
to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
asynchronous function.
Diffs (updated)
-----
src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263
src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed
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
src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0
Diff: https://reviews.apache.org/r/62636/diff/7/
Changes: https://reviews.apache.org/r/62636/diff/6-7/
Testing
-------
make
Thanks,
Chun-Hung Hsiao
Re: Review Request 62636: Generated authentication tokens for local
resource providers.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62636/#review191276
-----------------------------------------------------------
OK, i think we probably need to do the following given you're reusing the JWT authenticator built for executor authn:
1) probably rename `executor_secret_key` to `jwt_secret_key` (using flag alias) because this is used
2) if `jwt_secret_key` is not set, we should not load the JWT authenticator. It's already an error if someone set `authencate_http_executor` but not `jwt_secret_key`. So this sounds reasonable to me
```
string httpAuthenticators;
if (flags.http_authenticators.isSome()) {
httpAuthenticators = flags.http_authenticators.get();
#ifdef USE_SSL_SOCKET
} else if (flags.jwt_secret_key.isSome()) {
httpAuthenticators =
string(DEFAULT_BASIC_HTTP_AUTHENTICATOR) + "," +
string(DEFAULT_JWT_HTTP_AUTHENTICATOR);
#endif // USE_SSL_SOCKET
} else {
httpAuthenticators = DEFAULT_BASIC_HTTP_AUTHENTICATOR;
}
```
- Jie Yu
On Nov. 17, 2017, 12:19 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62636/
> -----------------------------------------------------------
>
> (Updated Nov. 17, 2017, 12:19 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
> -------
>
> `LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
> generate an authentication token for each local resource provider. The
> authentication token can then be used to call the V1 agent API. In order
> to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
> asynchronous function.
>
>
> Diffs
> -----
>
> src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263
> src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed
> src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0
>
>
> Diff: https://reviews.apache.org/r/62636/diff/6/
>
>
> Testing
> -------
>
> make
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 62636: Generated authentication tokens for local
resource providers.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62636/
-----------------------------------------------------------
(Updated Nov. 17, 2017, 12:19 a.m.)
Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
Changes
-------
Addressed Joseph's comments.
Bugs: MESOS-8100
https://issues.apache.org/jira/browse/MESOS-8100
Repository: mesos
Description
-------
`LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
generate an authentication token for each local resource provider. The
authentication token can then be used to call the V1 agent API. In order
to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
asynchronous function.
Diffs (updated)
-----
src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263
src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed
src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0
Diff: https://reviews.apache.org/r/62636/diff/6/
Changes: https://reviews.apache.org/r/62636/diff/5-6/
Testing
-------
make
Thanks,
Chun-Hung Hsiao
Re: Review Request 62636: Generated authentication tokens for local
resource providers.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62636/
-----------------------------------------------------------
(Updated Nov. 4, 2017, 12:51 a.m.)
Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
Changes
-------
Fixed a bug that generated undefined `stringify(Secret::Type)` in different translation units.
Bugs: MESOS-8100
https://issues.apache.org/jira/browse/MESOS-8100
Repository: mesos
Description
-------
`LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
generate an authentication token for each local resource provider. The
authentication token can then be used to call the V1 agent API. In order
to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
asynchronous function.
Diffs (updated)
-----
src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263
src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed
src/slave/slave.cpp 337083dbe60bba2d3773b785bdceeaf0b8fcd070
Diff: https://reviews.apache.org/r/62636/diff/5/
Changes: https://reviews.apache.org/r/62636/diff/4-5/
Testing
-------
make
Thanks,
Chun-Hung Hsiao
Re: Review Request 62636: Generated authentication tokens for local
resource providers.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62636/
-----------------------------------------------------------
(Updated Oct. 27, 2017, 9:58 p.m.)
Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
Changes
-------
Rebased.
Bugs: MESOS-8100
https://issues.apache.org/jira/browse/MESOS-8100
Repository: mesos
Description
-------
`LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
generate an authentication token for each local resource provider. The
authentication token can then be used to call the V1 agent API. In order
to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
asynchronous function.
Diffs (updated)
-----
src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263
src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed
src/slave/slave.cpp d8477b4e364270f5ad61311aa8fe48f823bc7aac
Diff: https://reviews.apache.org/r/62636/diff/4/
Changes: https://reviews.apache.org/r/62636/diff/3-4/
Testing
-------
make
Thanks,
Chun-Hung Hsiao