Re: Review Request 46211: Added flags for authenticating HTTP frameworks to master.

2016-04-15 Thread Vinod Kone 

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


Ship it!




Ship It!

- Vinod Kone


On April 15, 2016, 6:55 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46211/
> ---
> 
> (Updated April 15, 2016, 6:55 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3923
> https://issues.apache.org/jira/browse/MESOS-3923
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces two new flags `authenticate_http_frameworks`
> and `http_framework_authenticators` to the master. This allows us
> to selectively turn on/off framework authentication and decouple
> them from authentication for operator endpoints.
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp 7c7cc25fcc897dedb28001fbb944d2e50eca4713 
>   src/master/flags.hpp 83bb9088e595b393d610cc65479cb6a35fb31842 
>   src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
>   src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 
> 
> Diff: https://reviews.apache.org/r/46211/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 46211: Added flags for authenticating HTTP frameworks to master.

2016-04-15 Thread Anand Mazumdar 

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

(Updated April 15, 2016, 6:55 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Review comments from Vinod


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


Repository: mesos


Description
---

This change introduces two new flags `authenticate_http_frameworks`
and `http_framework_authenticators` to the master. This allows us
to selectively turn on/off framework authentication and decouple
them from authentication for operator endpoints.


Diffs (updated)
-

  src/master/constants.hpp 7c7cc25fcc897dedb28001fbb944d2e50eca4713 
  src/master/flags.hpp 83bb9088e595b393d610cc65479cb6a35fb31842 
  src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
  src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 46211: Added flags for authenticating HTTP frameworks to master.

2016-04-15 Thread Vinod Kone 


> On April 14, 2016, 5:33 p.m., Greg Mann wrote:
> > src/master/flags.cpp, lines 225-226
> > 
> >
> > This seems a tiny bit misleading: if the value is `false`, no 
> > authentication will be performed at all. Perhaps something like "If 
> > `false`, HTTP frameworks are not authenticated."

+1


> On April 14, 2016, 5:33 p.m., Greg Mann wrote:
> > src/master/master.cpp, lines 665-673
> > 
> >
> > This can be moved into the `if (flags.authenticate_http_frameworks)` 
> > scope, as well as the above declaration of `httpFrameworkAuthenticator`.

+1


- Vinod


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


On April 15, 2016, 4:39 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46211/
> ---
> 
> (Updated April 15, 2016, 4:39 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3923
> https://issues.apache.org/jira/browse/MESOS-3923
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces two new flags `authenticate_http_frameworks`
> and `http_framework_authenticators` to the master. This allows us
> to selectively turn on/off framework authentication and decouple
> them from authentication for operator endpoints.
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp 7c7cc25fcc897dedb28001fbb944d2e50eca4713 
>   src/master/flags.hpp 83bb9088e595b393d610cc65479cb6a35fb31842 
>   src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
>   src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 
> 
> Diff: https://reviews.apache.org/r/46211/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 46211: Added flags for authenticating HTTP frameworks to master.

2016-04-15 Thread Anand Mazumdar 

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

(Updated April 15, 2016, 4:39 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Review comments from Vinod & Greg


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


Repository: mesos


Description
---

This change introduces two new flags `authenticate_http_frameworks`
and `http_framework_authenticators` to the master. This allows us
to selectively turn on/off framework authentication and decouple
them from authentication for operator endpoints.


Diffs (updated)
-

  src/master/constants.hpp 7c7cc25fcc897dedb28001fbb944d2e50eca4713 
  src/master/flags.hpp 83bb9088e595b393d610cc65479cb6a35fb31842 
  src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
  src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 46211: Added flags for authenticating HTTP frameworks to master.

2016-04-15 Thread Anand Mazumdar 


- Anand


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


On April 15, 2016, 4:39 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46211/
> ---
> 
> (Updated April 15, 2016, 4:39 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3923
> https://issues.apache.org/jira/browse/MESOS-3923
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces two new flags `authenticate_http_frameworks`
> and `http_framework_authenticators` to the master. This allows us
> to selectively turn on/off framework authentication and decouple
> them from authentication for operator endpoints.
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp 7c7cc25fcc897dedb28001fbb944d2e50eca4713 
>   src/master/flags.hpp 83bb9088e595b393d610cc65479cb6a35fb31842 
>   src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
>   src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 
> 
> Diff: https://reviews.apache.org/r/46211/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 46211: Added flags for authenticating HTTP frameworks to master.

2016-04-15 Thread Anand Mazumdar 


> On April 15, 2016, 12:42 a.m., Vinod Kone wrote:
> > src/master/flags.cpp, line 482
> > 
> >
> > do we need a default here? we needed a default for 
> > `--http_authenticators` for backwards compatibility. since there is no 
> > backwards compatibility concern here, i think we should be ok with no 
> > default? having a default and not loading is a bit weird IMO.
> > 
> > remove the default and mention in the description  that this flag is 
> > required iff `--authenticate_http_frameworks` is set.

FWIW, I do like the idea of having the default authenticator be `basic` i.e. 
have a default value. It becomes easier to get started with using AuthN. 
Otherwise, they have to first search around for the module JSON string 
documentation, populate the fields etc. to set up the module correctly. Even, 
we need to do it to wire up our test driver. I wonder if it’s worth the hassle 
for operators/framework developers to go through this extra step.

We can explicitly include in the documentation that the module (including 
default) is only loaded when `--authenticate_http_frameworks` is set. 

I updated the review diff based on the above proposal. Let me know what do you 
think?


> On April 15, 2016, 12:42 a.m., Vinod Kone wrote:
> > src/master/constants.hpp, line 132
> > 
> >
> > If and when we add AuthN support for agent <-> executor, what is that 
> > realm going to be? 'mesos-http-framework' or 'mesos-http-executor'? I guess 
> > it has to the latter because we bring up both master and agent in the same 
> > OS process in tests?
> > 
> > so should this be called mesos-http-scheduler instead? it's kinda 
> > unfortunate that we sometimes equate framework with scheduler and sometimes 
> > with framework and executor.

Sounds good. Also, I don't like the idea of having the protocol name embedded 
in the realm. How about just: "mesos-scheduler"?


- Anand


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


On April 15, 2016, 4:39 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46211/
> ---
> 
> (Updated April 15, 2016, 4:39 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3923
> https://issues.apache.org/jira/browse/MESOS-3923
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces two new flags `authenticate_http_frameworks`
> and `http_framework_authenticators` to the master. This allows us
> to selectively turn on/off framework authentication and decouple
> them from authentication for operator endpoints.
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp 7c7cc25fcc897dedb28001fbb944d2e50eca4713 
>   src/master/flags.hpp 83bb9088e595b393d610cc65479cb6a35fb31842 
>   src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
>   src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 
> 
> Diff: https://reviews.apache.org/r/46211/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 46211: Added flags for authenticating HTTP frameworks to master.

2016-04-14 Thread Vinod Kone 

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




src/master/constants.hpp (line 132)


If and when we add AuthN support for agent <-> executor, what is that realm 
going to be? 'mesos-http-framework' or 'mesos-http-executor'? I guess it has to 
the latter because we bring up both master and agent in the same OS process in 
tests?

so should this be called mesos-http-scheduler instead? it's kinda 
unfortunate that we sometimes equate framework with scheduler and sometimes 
with framework and executor.



src/master/flags.cpp (line 482)


do we need a default here? we needed a default for `--http_authenticators` 
for backwards compatibility. since there is no backwards compatibility concern 
here, i think we should be ok with no default? having a default and not loading 
is a bit weird IMO.

remove the default and mention in the description  that this flag is 
required iff `--authenticate_http_frameworks` is set.



src/master/master.cpp (line 448)


Master allowing HTTP frameworks to register without authentication



src/master/master.cpp (line 674)


this whole logic can be cleaned up a bit if we make the flag required as 
suggested.


- Vinod Kone


On April 14, 2016, 3:34 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46211/
> ---
> 
> (Updated April 14, 2016, 3:34 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3923
> https://issues.apache.org/jira/browse/MESOS-3923
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces two new flags `authenticate_http_frameworks`
> and `http_framework_authenticators` to the master. This allows us
> to selectively turn on/off framework authentication and decouple
> them from authentication for operator endpoints.
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp 7c7cc25fcc897dedb28001fbb944d2e50eca4713 
>   src/master/flags.hpp 83bb9088e595b393d610cc65479cb6a35fb31842 
>   src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
>   src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 
> 
> Diff: https://reviews.apache.org/r/46211/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 46211: Added flags for authenticating HTTP frameworks to master.

2016-04-14 Thread Greg Mann 

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




src/master/flags.cpp (lines 225 - 226)


This seems a tiny bit misleading: if the value is `false`, no 
authentication will be performed at all. Perhaps something like "If `false`, 
HTTP frameworks are not authenticated."



src/master/master.cpp (lines 665 - 673)


This can be moved into the `if (flags.authenticate_http_frameworks)` scope, 
as well as the above declaration of `httpFrameworkAuthenticator`.


- Greg Mann


On April 14, 2016, 3:34 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46211/
> ---
> 
> (Updated April 14, 2016, 3:34 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3923
> https://issues.apache.org/jira/browse/MESOS-3923
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces two new flags `authenticate_http_frameworks`
> and `http_framework_authenticators` to the master. This allows us
> to selectively turn on/off framework authentication and decouple
> them from authentication for operator endpoints.
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp 7c7cc25fcc897dedb28001fbb944d2e50eca4713 
>   src/master/flags.hpp 83bb9088e595b393d610cc65479cb6a35fb31842 
>   src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
>   src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 
> 
> Diff: https://reviews.apache.org/r/46211/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Review Request 46211: Added flags for authenticating HTTP frameworks to master.

2016-04-14 Thread Anand Mazumdar 

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

This change introduces two new flags `authenticate_http_frameworks`
and `http_framework_authenticators` to the master. This allows us
to selectively turn on/off framework authentication and decouple
them from authentication for operator endpoints.


Diffs
-

  src/master/constants.hpp 7c7cc25fcc897dedb28001fbb944d2e50eca4713 
  src/master/flags.hpp 83bb9088e595b393d610cc65479cb6a35fb31842 
  src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
  src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 

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


Testing
---

make check


Thanks,

Anand Mazumdar