You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Isabel Jimenez <co...@isabeljimenez.com> on 2014/06/20 21:45:26 UTC

Review Request 22832: HTTP Authenticated '/shutdown' endpoint

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

Review request for mesos, Adam B, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.


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


Repository: mesos-git


Description
-------

HTTP Authenticated '/shutdown' endpoint for shutting down a running framework


Diffs
-----

  src/master/http.cpp 5d86976 
  src/master/master.hpp b56e9f4 
  src/master/master.cpp dcf28ad 

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


Testing
-------

make check


Thanks,

Isabel Jimenez


Re: Review Request 22832: HTTP Authenticated '/shutdown' endpoint

Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22832/#review47177
-----------------------------------------------------------


Patch looks great!

Reviews applied: [22832]

All tests passed.

- Mesos ReviewBot


On July 1, 2014, 10:46 p.m., Isabel Jimenez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22832/
> -----------------------------------------------------------
> 
> (Updated July 1, 2014, 10:46 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.
> 
> 
> Bugs: MESOS-1390
>     https://issues.apache.org/jira/browse/MESOS-1390
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> HTTP Authenticated '/shutdown' endpoint for shutting down a running framework
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 12d84bf 
>   src/master/http.cpp 5d86976 
>   src/master/master.hpp 5fef354 
>   src/master/master.cpp 474014b 
>   src/slave/http.cpp cd7f692 
>   src/slave/slave.hpp 605ee4a 
>   src/slave/slave.cpp f42ab60 
>   src/tests/shutdown_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22832/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>


Re: Review Request 22832: HTTP Authenticated '/shutdown' endpoint

Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22832/#review47415
-----------------------------------------------------------

Ship it!


Great! Thanks Isabel!

- Benjamin Hindman


On July 7, 2014, 8:59 p.m., Isabel Jimenez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22832/
> -----------------------------------------------------------
> 
> (Updated July 7, 2014, 8:59 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.
> 
> 
> Bugs: MESOS-1390
>     https://issues.apache.org/jira/browse/MESOS-1390
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> HTTP Authenticated '/shutdown' endpoint for shutting down a running framework
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am e3ff6d7 
>   src/master/http.cpp 5d86976 
>   src/master/master.hpp 5fef354 
>   src/master/master.cpp 251b699 
>   src/slave/http.cpp cd7f692 
>   src/slave/slave.hpp 605ee4a 
>   src/slave/slave.cpp f42ab60 
>   src/tests/shutdown_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22832/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>


Re: Review Request 22832: HTTP Authenticated '/shutdown' endpoint

Posted by Adam B <ad...@mesosphere.io>.

> On July 7, 2014, 3:03 p.m., Adam B wrote:
> > src/master/http.cpp, line 637
> > <https://reviews.apache.org/r/22832/diff/3/?file=625040#file625040line637>
> >
> >     Can we get some more descriptive error strings, please? ("Mesos master" everywhere may not be descriptive enough when debugging)
> 
> Benjamin Hindman wrote:
>     This is actually the "realm", not an error message. Do you have a suggestion for a better realm? Thinking about Kerberos, what constraints will we have on the realm (e.g., no spaces)?

Kerberos realms are usually an ALL.UPPERCASE DNS name (no spaces), like APACHE.ORG
See http://www.cmf.nrl.navy.mil/krb/kerberos-faq.html#realms


> On July 7, 2014, 3:03 p.m., Adam B wrote:
> > src/slave/slave.hpp, line 341
> > <https://reviews.apache.org/r/22832/diff/3/?file=625044#file625044line341>
> >
> >     Why did the const Slave& have to become a Slave* too?
> 
> Benjamin Hindman wrote:
>     Done for consistency, since we've also used * for nested classes in other places (the Master::Http and Slave::Http classes were the only remaining ones that I was aware of that didn't pass a *).

Understood & accepted.


> On July 7, 2014, 3:03 p.m., Adam B wrote:
> > src/master/http.cpp, lines 622-623
> > <https://reviews.apache.org/r/22832/diff/3/?file=625040#file625040line622>
> >
> >     Does this mean that the /shutdown API only works when authentication is enabled (or at least credentials are provided)? What if I just want to /shutdown on my local test cluster? Shouldn't there be a way to allow /shutdown without authentication?
> 
> Benjamin Hindman wrote:
>     This is a good point, let's circle back to this in a follow up review Isabel.

Thanks. Should be easy to fix.


- Adam


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


On July 7, 2014, 1:59 p.m., Isabel Jimenez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22832/
> -----------------------------------------------------------
> 
> (Updated July 7, 2014, 1:59 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.
> 
> 
> Bugs: MESOS-1390
>     https://issues.apache.org/jira/browse/MESOS-1390
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> HTTP Authenticated '/shutdown' endpoint for shutting down a running framework
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am e3ff6d7 
>   src/master/http.cpp 5d86976 
>   src/master/master.hpp 5fef354 
>   src/master/master.cpp 251b699 
>   src/slave/http.cpp cd7f692 
>   src/slave/slave.hpp 605ee4a 
>   src/slave/slave.cpp f42ab60 
>   src/tests/shutdown_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22832/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>


Re: Review Request 22832: HTTP Authenticated '/shutdown' endpoint

Posted by Benjamin Hindman <be...@berkeley.edu>.

> On July 7, 2014, 10:03 p.m., Adam B wrote:
> > src/master/http.cpp, lines 622-623
> > <https://reviews.apache.org/r/22832/diff/3/?file=625040#file625040line622>
> >
> >     Does this mean that the /shutdown API only works when authentication is enabled (or at least credentials are provided)? What if I just want to /shutdown on my local test cluster? Shouldn't there be a way to allow /shutdown without authentication?

This is a good point, let's circle back to this in a follow up review Isabel.


> On July 7, 2014, 10:03 p.m., Adam B wrote:
> > src/master/http.cpp, line 637
> > <https://reviews.apache.org/r/22832/diff/3/?file=625040#file625040line637>
> >
> >     Can we get some more descriptive error strings, please? ("Mesos master" everywhere may not be descriptive enough when debugging)

This is actually the "realm", not an error message. Do you have a suggestion for a better realm? Thinking about Kerberos, what constraints will we have on the realm (e.g., no spaces)?


> On July 7, 2014, 10:03 p.m., Adam B wrote:
> > src/slave/slave.hpp, line 341
> > <https://reviews.apache.org/r/22832/diff/3/?file=625044#file625044line341>
> >
> >     Why did the const Slave& have to become a Slave* too?

Done for consistency, since we've also used * for nested classes in other places (the Master::Http and Slave::Http classes were the only remaining ones that I was aware of that didn't pass a *).


- Benjamin


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


On July 7, 2014, 8:59 p.m., Isabel Jimenez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22832/
> -----------------------------------------------------------
> 
> (Updated July 7, 2014, 8:59 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.
> 
> 
> Bugs: MESOS-1390
>     https://issues.apache.org/jira/browse/MESOS-1390
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> HTTP Authenticated '/shutdown' endpoint for shutting down a running framework
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am e3ff6d7 
>   src/master/http.cpp 5d86976 
>   src/master/master.hpp 5fef354 
>   src/master/master.cpp 251b699 
>   src/slave/http.cpp cd7f692 
>   src/slave/slave.hpp 605ee4a 
>   src/slave/slave.cpp f42ab60 
>   src/tests/shutdown_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22832/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>


Re: Review Request 22832: HTTP Authenticated '/shutdown' endpoint

Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22832/#review47417
-----------------------------------------------------------


Looks great! I just wonder if we should also allow /shutdown to be called on an unauthenticated cluster (no --credentials).


src/master/http.cpp
<https://reviews.apache.org/r/22832/#comment83159>

    Does this mean that the /shutdown API only works when authentication is enabled (or at least credentials are provided)? What if I just want to /shutdown on my local test cluster? Shouldn't there be a way to allow /shutdown without authentication?



src/master/http.cpp
<https://reviews.apache.org/r/22832/#comment83160>

    Can we get some more descriptive error strings, please? ("Mesos master" everywhere may not be descriptive enough when debugging)



src/master/http.cpp
<https://reviews.apache.org/r/22832/#comment83162>

    Please end your comment with punctuation.



src/slave/slave.hpp
<https://reviews.apache.org/r/22832/#comment83156>

    Why did the const Slave& have to become a Slave* too?


- Adam B


On July 7, 2014, 1:59 p.m., Isabel Jimenez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22832/
> -----------------------------------------------------------
> 
> (Updated July 7, 2014, 1:59 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.
> 
> 
> Bugs: MESOS-1390
>     https://issues.apache.org/jira/browse/MESOS-1390
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> HTTP Authenticated '/shutdown' endpoint for shutting down a running framework
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am e3ff6d7 
>   src/master/http.cpp 5d86976 
>   src/master/master.hpp 5fef354 
>   src/master/master.cpp 251b699 
>   src/slave/http.cpp cd7f692 
>   src/slave/slave.hpp 605ee4a 
>   src/slave/slave.cpp f42ab60 
>   src/tests/shutdown_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22832/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>


Re: Review Request 22832: HTTP Authenticated '/shutdown' endpoint

Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22832/
-----------------------------------------------------------

(Updated July 7, 2014, 8:59 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.


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


Repository: mesos-git


Description
-------

HTTP Authenticated '/shutdown' endpoint for shutting down a running framework


Diffs (updated)
-----

  src/Makefile.am e3ff6d7 
  src/master/http.cpp 5d86976 
  src/master/master.hpp 5fef354 
  src/master/master.cpp 251b699 
  src/slave/http.cpp cd7f692 
  src/slave/slave.hpp 605ee4a 
  src/slave/slave.cpp f42ab60 
  src/tests/shutdown_tests.cpp PRE-CREATION 

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


Testing
-------

make check


Thanks,

Isabel Jimenez


Re: Review Request 22832: HTTP Authenticated '/shutdown' endpoint

Posted by Isabel Jimenez <co...@isabeljimenez.com>.

> On July 1, 2014, 11:04 p.m., Dominic Hamon wrote:
> > src/slave/slave.hpp, line 324
> > <https://reviews.apache.org/r/22832/diff/2/?file=622206#file622206line324>
> >
> >     is this change just for symmetry with Master::Http?

Yes, this change is just for consistency


> On July 1, 2014, 11:04 p.m., Dominic Hamon wrote:
> > src/master/http.cpp, line 649
> > <https://reviews.apache.org/r/22832/diff/2/?file=622202#file622202line649>
> >
> >     how long do you expect this call to take? ie, will the client timeout waiting for a response? should this dispatch the request to the master instead and return Accepted()?

I put on comment for this to be changed in a future patch 


- Isabel


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


On July 7, 2014, 8:59 p.m., Isabel Jimenez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22832/
> -----------------------------------------------------------
> 
> (Updated July 7, 2014, 8:59 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.
> 
> 
> Bugs: MESOS-1390
>     https://issues.apache.org/jira/browse/MESOS-1390
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> HTTP Authenticated '/shutdown' endpoint for shutting down a running framework
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am e3ff6d7 
>   src/master/http.cpp 5d86976 
>   src/master/master.hpp 5fef354 
>   src/master/master.cpp 251b699 
>   src/slave/http.cpp cd7f692 
>   src/slave/slave.hpp 605ee4a 
>   src/slave/slave.cpp f42ab60 
>   src/tests/shutdown_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22832/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>


Re: Review Request 22832: HTTP Authenticated '/shutdown' endpoint

Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22832/#review47156
-----------------------------------------------------------



src/Makefile.am
<https://reviews.apache.org/r/22832/#comment82743>

    nit: watch the tabs/spaces here :)



src/master/http.cpp
<https://reviews.apache.org/r/22832/#comment82745>

    would you mind changing the indent here to match the lines above?



src/master/http.cpp
<https://reviews.apache.org/r/22832/#comment82746>

    how long do you expect this call to take? ie, will the client timeout waiting for a response? should this dispatch the request to the master instead and return Accepted()?



src/slave/slave.hpp
<https://reviews.apache.org/r/22832/#comment82747>

    is this change just for symmetry with Master::Http?



src/tests/shutdown_tests.cpp
<https://reviews.apache.org/r/22832/#comment82748>

    our test methods usually have capital initials:
    
    TEST_F(ShutdownTest, ShutdownEndpoint)



src/tests/shutdown_tests.cpp
<https://reviews.apache.org/r/22832/#comment82751>

    i think each of these test cases should be a different test.



src/tests/shutdown_tests.cpp
<https://reviews.apache.org/r/22832/#comment82749>

    you can also use OK().status (which is more descriptive)



src/tests/shutdown_tests.cpp
<https://reviews.apache.org/r/22832/#comment82750>

    also test with authorization header and bad credentials?


- Dominic Hamon


On July 1, 2014, 3:46 p.m., Isabel Jimenez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22832/
> -----------------------------------------------------------
> 
> (Updated July 1, 2014, 3:46 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.
> 
> 
> Bugs: MESOS-1390
>     https://issues.apache.org/jira/browse/MESOS-1390
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> HTTP Authenticated '/shutdown' endpoint for shutting down a running framework
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 12d84bf 
>   src/master/http.cpp 5d86976 
>   src/master/master.hpp 5fef354 
>   src/master/master.cpp 474014b 
>   src/slave/http.cpp cd7f692 
>   src/slave/slave.hpp 605ee4a 
>   src/slave/slave.cpp f42ab60 
>   src/tests/shutdown_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22832/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>


Re: Review Request 22832: HTTP Authenticated '/shutdown' endpoint

Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22832/
-----------------------------------------------------------

(Updated July 1, 2014, 10:46 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.


Changes
-------

Changes after Benh's and Adam's comments


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


Repository: mesos-git


Description
-------

HTTP Authenticated '/shutdown' endpoint for shutting down a running framework


Diffs (updated)
-----

  src/Makefile.am 12d84bf 
  src/master/http.cpp 5d86976 
  src/master/master.hpp 5fef354 
  src/master/master.cpp 474014b 
  src/slave/http.cpp cd7f692 
  src/slave/slave.hpp 605ee4a 
  src/slave/slave.cpp f42ab60 
  src/tests/shutdown_tests.cpp PRE-CREATION 

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


Testing
-------

make check


Thanks,

Isabel Jimenez


Re: Review Request 22832: HTTP Authenticated '/shutdown' endpoint

Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22832/#review46410
-----------------------------------------------------------


Can we make 'master' be a pointer in the nested Http class instead of a reference? This has been the pattern we've followed for nested classes.


src/master/http.cpp
<https://reviews.apache.org/r/22832/#comment81791>

    if (credential.principal() == username &&
        credential.secret() == password) {
    
    }


- Benjamin Hindman


On June 20, 2014, 7:45 p.m., Isabel Jimenez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22832/
> -----------------------------------------------------------
> 
> (Updated June 20, 2014, 7:45 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.
> 
> 
> Bugs: MESOS-1390
>     https://issues.apache.org/jira/browse/MESOS-1390
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> HTTP Authenticated '/shutdown' endpoint for shutting down a running framework
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp 5d86976 
>   src/master/master.hpp b56e9f4 
>   src/master/master.cpp dcf28ad 
> 
> Diff: https://reviews.apache.org/r/22832/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>


Re: Review Request 22832: HTTP Authenticated '/shutdown' endpoint

Posted by Isabel Jimenez <co...@isabeljimenez.com>.

> On June 24, 2014, 10:03 a.m., Adam B wrote:
> > src/master/master.hpp, line 422
> > <https://reviews.apache.org/r/22832/diff/1/?file=614321#file614321line422>
> >
> >     No more const. :(
> >     Can you give us a quick explanation why? (Master::removeFramework() is not const?)

No removeFramework is not const :(


- Isabel


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


On July 1, 2014, 10:46 p.m., Isabel Jimenez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22832/
> -----------------------------------------------------------
> 
> (Updated July 1, 2014, 10:46 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.
> 
> 
> Bugs: MESOS-1390
>     https://issues.apache.org/jira/browse/MESOS-1390
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> HTTP Authenticated '/shutdown' endpoint for shutting down a running framework
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 12d84bf 
>   src/master/http.cpp 5d86976 
>   src/master/master.hpp 5fef354 
>   src/master/master.cpp 474014b 
>   src/slave/http.cpp cd7f692 
>   src/slave/slave.hpp 605ee4a 
>   src/slave/slave.cpp f42ab60 
>   src/tests/shutdown_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22832/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>


Re: Review Request 22832: HTTP Authenticated '/shutdown' endpoint

Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22832/#review46506
-----------------------------------------------------------


Any ideas how this could be extended to handle OAuth or other http authentication mechanisms?


src/master/http.cpp
<https://reviews.apache.org/r/22832/#comment81912>

    Why check frameworkId.isNone() again here? You already returned BadRequest() above, and I don't see any way for frameworkId to have changed since then.



src/master/http.cpp
<https://reviews.apache.org/r/22832/#comment81913>

    Just use find() to find only the first instance of ':', and then you can use substr() to get your username and password (can include ':'s).



src/master/http.cpp
<https://reviews.apache.org/r/22832/#comment81926>

    Check for credentials.isSome() before calling get()?



src/master/http.cpp
<https://reviews.apache.org/r/22832/#comment81936>

    Check credential.has_secret()?



src/master/master.hpp
<https://reviews.apache.org/r/22832/#comment81937>

    No more const. :(
    Can you give us a quick explanation why? (Master::removeFramework() is not const?)


- Adam B


On June 20, 2014, 12:45 p.m., Isabel Jimenez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22832/
> -----------------------------------------------------------
> 
> (Updated June 20, 2014, 12:45 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.
> 
> 
> Bugs: MESOS-1390
>     https://issues.apache.org/jira/browse/MESOS-1390
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> HTTP Authenticated '/shutdown' endpoint for shutting down a running framework
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp 5d86976 
>   src/master/master.hpp b56e9f4 
>   src/master/master.cpp dcf28ad 
> 
> Diff: https://reviews.apache.org/r/22832/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>


Re: Review Request 22832: HTTP Authenticated '/shutdown' endpoint

Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22832/#review46605
-----------------------------------------------------------


Bad patch!

Reviews applied: [22222, 22825, 22832]

Failed command: ./support/mesos-style.py

Error:
 Checking 481 files using filter --filter=-,+build/class,+build/deprecated,+build/endif_comment,+runtime/vlog,+whitespace/blank_line,+whitespace/comma,+whitespace/ending_newline,+whitespace/forcolon,+whitespace/indent,+whitespace/line_length,+whitespace/tab,+whitespace/todo
3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp:268:  Missing space after ,  [whitespace/comma] [3]
3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp:284:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 2


- Mesos ReviewBot


On June 20, 2014, 7:45 p.m., Isabel Jimenez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22832/
> -----------------------------------------------------------
> 
> (Updated June 20, 2014, 7:45 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.
> 
> 
> Bugs: MESOS-1390
>     https://issues.apache.org/jira/browse/MESOS-1390
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> HTTP Authenticated '/shutdown' endpoint for shutting down a running framework
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp 5d86976 
>   src/master/master.hpp b56e9f4 
>   src/master/master.cpp dcf28ad 
> 
> Diff: https://reviews.apache.org/r/22832/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>