Re: Review Request 37999: Introduced an Authenticator interface and an AuthenticationRouter in libprocess.

2015-11-20 Thread Ben Mahler 


> On Nov. 20, 2015, 2 p.m., Ben Mahler wrote:
> > For transparency we pulled out the libprocess integration because we 
> > realized that requests sent to the authentication router need to have 
> > authentication results satisfied in the same order in which the requests 
> > were sent. We're still thinking through how to solve this within 
> > libprocess, so for now we're just going to commit these interfaces (we 
> > don't expect these interfaces to change further for the MVP).
> > 
> > I will update the description in the commit to reflect that this no longer 
> > includes the ProcessManager integration.

We're also pulling out the remaining changes to event.hpp and process.cpp in 
this patch, as they will make more sense alongside the subsequent integration 
patches.


- Ben


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


On Nov. 20, 2015, 12:53 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> ---
> 
> (Updated Nov. 20, 2015, 12:53 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3231
> https://issues.apache.org/jira/browse/MESOS-3231
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The Authenticator interface allows us to implement different
> authenticators based on the scheme (e.g. Basic, Digest, SPNEGO).
> The AuthenticationRouter manages the authentication realms and
> the mapping from endpoints to realms. It is then used by the
> ProcessManager to route requests to the authenticator for the
> realm, if applicable.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am cdefa37528ea69422978a9772f955042e882dde4 
>   3rdparty/libprocess/include/Makefile.am 
> e6be2c4db121585bbe7f3c0627de048adc7cfb2c 
>   3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/event.hpp 
> 60c615281f3810230bf6c17866f46eaa6855ca29 
>   3rdparty/libprocess/include/process/http.hpp 
> 90c9be122ee0c402b806d70fc818e3c03b15101a 
>   3rdparty/libprocess/src/CMakeLists.txt 
> fb9bd04832779ac43151f2feb3dfbf58cb996434 
>   3rdparty/libprocess/src/authentication_router.hpp PRE-CREATION 
>   3rdparty/libprocess/src/authentication_router.cpp PRE-CREATION 
>   3rdparty/libprocess/src/process.cpp 
> 7abdf21a5784920251c3627f9820c12fdc356c6e 
> 
> Diff: https://reviews.apache.org/r/37999/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 37999: Introduced an Authenticator interface and an AuthenticationRouter in libprocess.

2015-11-20 Thread Ben Mahler 

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

Ship it!


For transparency we pulled out the libprocess integration because we realized 
that requests sent to the authentication router need to have authentication 
results satisfied in the same order in which the requests were sent. We're 
still thinking through how to solve this within libprocess, so for now we're 
just going to commit these interfaces (we don't expect these interfaces to 
change further for the MVP).

I will update the description in the commit to reflect that this no longer 
includes the ProcessManager integration.

- Ben Mahler


On Nov. 20, 2015, 12:53 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> ---
> 
> (Updated Nov. 20, 2015, 12:53 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3231
> https://issues.apache.org/jira/browse/MESOS-3231
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The Authenticator interface allows us to implement different
> authenticators based on the scheme (e.g. Basic, Digest, SPNEGO).
> The AuthenticationRouter manages the authentication realms and
> the mapping from endpoints to realms. It is then used by the
> ProcessManager to route requests to the authenticator for the
> realm, if applicable.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am cdefa37528ea69422978a9772f955042e882dde4 
>   3rdparty/libprocess/include/Makefile.am 
> e6be2c4db121585bbe7f3c0627de048adc7cfb2c 
>   3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/event.hpp 
> 60c615281f3810230bf6c17866f46eaa6855ca29 
>   3rdparty/libprocess/include/process/http.hpp 
> 90c9be122ee0c402b806d70fc818e3c03b15101a 
>   3rdparty/libprocess/src/CMakeLists.txt 
> fb9bd04832779ac43151f2feb3dfbf58cb996434 
>   3rdparty/libprocess/src/authentication_router.hpp PRE-CREATION 
>   3rdparty/libprocess/src/authentication_router.cpp PRE-CREATION 
>   3rdparty/libprocess/src/process.cpp 
> 7abdf21a5784920251c3627f9820c12fdc356c6e 
> 
> Diff: https://reviews.apache.org/r/37999/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 37999: Introduced an Authenticator interface and an AuthenticationRouter in libprocess.

2015-11-20 Thread Alexander Rojas 

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

(Updated Nov. 20, 2015, 1:53 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
Toenshoff.


Changes
---

Removes actual authentication logic from `ProcessManager::handle()` until 
delivering pipelining to processes is cleared.


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


Repository: mesos


Description
---

The Authenticator interface allows us to implement different
authenticators based on the scheme (e.g. Basic, Digest, SPNEGO).
The AuthenticationRouter manages the authentication realms and
the mapping from endpoints to realms. It is then used by the
ProcessManager to route requests to the authenticator for the
realm, if applicable.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am cdefa37528ea69422978a9772f955042e882dde4 
  3rdparty/libprocess/include/Makefile.am 
e6be2c4db121585bbe7f3c0627de048adc7cfb2c 
  3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION 
  3rdparty/libprocess/include/process/event.hpp 
60c615281f3810230bf6c17866f46eaa6855ca29 
  3rdparty/libprocess/include/process/http.hpp 
90c9be122ee0c402b806d70fc818e3c03b15101a 
  3rdparty/libprocess/src/CMakeLists.txt 
fb9bd04832779ac43151f2feb3dfbf58cb996434 
  3rdparty/libprocess/src/authentication_router.hpp PRE-CREATION 
  3rdparty/libprocess/src/authentication_router.cpp PRE-CREATION 
  3rdparty/libprocess/src/process.cpp 7abdf21a5784920251c3627f9820c12fdc356c6e 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 37999: Introduced an Authenticator interface and an AuthenticationRouter in libprocess.

2015-11-18 Thread Alexander Rojas 

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

(Updated Nov. 18, 2015, 11:43 a.m.)


Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
Toenshoff.


Changes
---

Updated summary and description.


Summary (updated)
-

Introduced an Authenticator interface and an AuthenticationRouter in libprocess.


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


Repository: mesos


Description (updated)
---

The Authenticator interface allows us to implement different
authenticators based on the scheme (e.g. Basic, Digest, SPNEGO).
The AuthenticationRouter manages the authentication realms and
the mapping from endpoints to realms. It is then used by the
ProcessManager to route requests to the authenticator for the
realm, if applicable.


Diffs
-

  3rdparty/libprocess/Makefile.am cdefa37528ea69422978a9772f955042e882dde4 
  3rdparty/libprocess/include/Makefile.am 
e6be2c4db121585bbe7f3c0627de048adc7cfb2c 
  3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION 
  3rdparty/libprocess/include/process/event.hpp 
28ce1928877084f0e1a73fdad789224c86e53f46 
  3rdparty/libprocess/include/process/http.hpp 
90c9be122ee0c402b806d70fc818e3c03b15101a 
  3rdparty/libprocess/src/CMakeLists.txt 
fb9bd04832779ac43151f2feb3dfbf58cb996434 
  3rdparty/libprocess/src/authentication_router.hpp PRE-CREATION 
  3rdparty/libprocess/src/authentication_router.cpp PRE-CREATION 
  3rdparty/libprocess/src/process.cpp 7abdf21a5784920251c3627f9820c12fdc356c6e 
  3rdparty/libprocess/src/process_reference.hpp 
e6110bba2a54948be68e58ab9de988565b7d95a8 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 37999: Introduced an Authenticator interface and an AuthenticationRouter in libprocess.

2015-11-18 Thread Ben Mahler 

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

Ship it!


Was great sitting down and going over all of this stuff with you over the past 
week, thanks for the patience! Just some final adjustments that we discussed 
based on pulling this down and getting ready to commit.


3rdparty/libprocess/include/process/authenticator.hpp (line 52)


Let's maybe say here that this allows us to implement different 
authenticators based on the scheme and include the examples below (Basic, 
Digest, SPNEGO, etc).



3rdparty/libprocess/include/process/authenticator.hpp (line 65)


Whoops, stale now that you have the namespace.



3rdparty/libprocess/include/process/http.hpp (lines 545 - 552)


As we discussed, should we have a TODO to remove these?



3rdparty/libprocess/src/authentication_router.hpp (lines 35 - 37)


A little bit stale now that we called this router instead of realm manager, 
right?



3rdparty/libprocess/src/authentication_router.hpp (line 48)


How about unset for symettry? I see why you called this 'set' and 'unset' 
and that does describe the single authenticator per-realm constraint. Thinking 
over this again though, let's also add a small comment to describe this 
constraint.



3rdparty/libprocess/src/authentication_router.hpp (line 57)


Whoops, "either" here is orphaned.



3rdparty/libprocess/src/authentication_router.hpp (line 69)


Whoops, stale comment.



3rdparty/libprocess/src/authentication_router.cpp (line 96)


Hm.. perhaps we also need something here to mention that we're assuming 
absolute paths, which is ensured by libprocess. Should consider maybe returning 
a failure here when a relative path is detected.



3rdparty/libprocess/src/authentication_router.cpp (line 107)


Whoops, should have a space before the brace here.



3rdparty/libprocess/src/authentication_router.cpp (line 111)


Perhaps we should handle the "error" logging case first, as that tends to 
be how we structure the code and we can avoid an else more clearly



3rdparty/libprocess/src/authentication_router.cpp (lines 114 - 116)


How about we validate that the Result is valid before we send it up to 
libprocess? We can CHECK validity in libprocess since we control the code here, 
and avoid having to do validity checking (which we weren't really doing 
currently).



3rdparty/libprocess/src/authentication_router.cpp (line 136)


Why the 'if' here? 'process' should not be null here, unless there is a 
bug. We could add a check but for now I'll just remove the 'if' guard to be 
consistent with the rest of the process wrapper code FWICT.



3rdparty/libprocess/src/process.cpp (line 60)


What was this for? The result? Hm.. should be ok in this case to just 
include the router since we are only using signatures from the router.



3rdparty/libprocess/src/process.cpp (line 109)


Alphabetical?



3rdparty/libprocess/src/process.cpp (line 128)


We should consider just making this authentication::Router, but I'll leave 
it for now.



3rdparty/libprocess/src/process.cpp (line 953)


Even though there whould probably be just one comment above all these 
saying we're creating the globals, let's add another comment here to be 
consistent with the current code:

```
// Create the global HTTP authentication router.
authentication_router = new AuthenticationRouter();
```



3rdparty/libprocess/src/process_reference.hpp (line 52)


This should be in a separate patch, much like we did for the promise 
setting bug we noticed.


- Ben Mahler


On Nov. 18, 2015, 10:43 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> ---
> 
> (Updated Nov. 18, 2015, 10:43 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin