You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rojas <al...@mesosphere.io> on 2015/06/26 12:14:44 UTC
Review Request 35919: Firewall rule's apply method returns an HTTP
response instead of an error message.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35919/
-----------------------------------------------------------
Review request for mesos, Adam B and Till Toenshoff.
Bugs: MESOS-2877
https://issues.apache.org/jira/browse/MESOS-2877
Repository: mesos
Description
-------
see summary.
Diffs
-----
3rdparty/libprocess/include/process/firewall.hpp f71d6541e48e2481c69f401b388977b153503962
3rdparty/libprocess/src/process.cpp 52649fb90cdbefb495b68d0beb8c7f7e5ef6888e
Diff: https://reviews.apache.org/r/35919/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 35919: Firewall rule's apply method returns an HTTP
response instead of an error message.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35919/#review89483
-----------------------------------------------------------
Patch looks great!
Reviews applied: [35919]
All tests passed.
- Mesos ReviewBot
On June 26, 2015, 10:18 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35919/
> -----------------------------------------------------------
>
> (Updated June 26, 2015, 10:18 a.m.)
>
>
> Review request for mesos, Adam B and Till Toenshoff.
>
>
> Bugs: MESOS-2877
> https://issues.apache.org/jira/browse/MESOS-2877
>
>
> Repository: mesos
>
>
> Description
> -------
>
> see summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/firewall.hpp f71d6541e48e2481c69f401b388977b153503962
> 3rdparty/libprocess/src/process.cpp 52649fb90cdbefb495b68d0beb8c7f7e5ef6888e
>
> Diff: https://reviews.apache.org/r/35919/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 35919: Firewall rule's apply method returns an HTTP
response instead of an error message.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35919/#review89715
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/src/process.cpp (line 2017)
<https://reviews.apache.org/r/35919/#comment142395>
Might return all kinds of HTTP return codes, no?
3rdparty/libprocess/src/process.cpp (line 2019)
<https://reviews.apache.org/r/35919/#comment142396>
Use that HTTP status code here instead of `body` to make sure we always (also in the future) get a nice log output (mind there may even be HTML within the body).
3rdparty/libprocess/src/process.cpp (line 2029)
<https://reviews.apache.org/r/35919/#comment142400>
How about adding a `CHECK(rejection.get() != NULL)` here somewhere?
- Till Toenshoff
On June 26, 2015, 10:18 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35919/
> -----------------------------------------------------------
>
> (Updated June 26, 2015, 10:18 a.m.)
>
>
> Review request for mesos, Adam B and Till Toenshoff.
>
>
> Bugs: MESOS-2877
> https://issues.apache.org/jira/browse/MESOS-2877
>
>
> Repository: mesos
>
>
> Description
> -------
>
> see summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/firewall.hpp f71d6541e48e2481c69f401b388977b153503962
> 3rdparty/libprocess/src/process.cpp 52649fb90cdbefb495b68d0beb8c7f7e5ef6888e
>
> Diff: https://reviews.apache.org/r/35919/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 35919: Firewall rule's apply method returns an HTTP
response instead of an error message.
Posted by Alexander Rojas <al...@mesosphere.io>.
> On June 30, 2015, 6:08 a.m., Adam B wrote:
> > Minor points, but otherwise shippable.
> > I guess I'm still a little confused by the motivation. When would the firewall filter rule have more information about how to respond than the caller would? Can you give an example?
The idea is to implement SPNEGO in the least intrusive way. That can be achieve with an `FirewallRule`. The problem happens if the request doesn't include the header `Authenticate: Negotiate`, in such case, we should return an `HTTP 401 Unauthorize` response with a header `WWW-Authenticate: Negotiate U29tZSB0ZXh0Cg==`. But this cannot be done if we cannot generate the response object directly from the `FirewallRule`.
> On June 30, 2015, 6:08 a.m., Adam B wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 2019-2020
> > <https://reviews.apache.org/r/35919/diff/3/?file=994356#file994356line2019>
> >
> > You're not logging the Response body anymore?
Previous reviewer asked to remove it. What should be done in this case?
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35919/#review89854
-----------------------------------------------------------
On June 29, 2015, 12:21 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35919/
> -----------------------------------------------------------
>
> (Updated June 29, 2015, 12:21 p.m.)
>
>
> Review request for mesos, Adam B and Till Toenshoff.
>
>
> Bugs: MESOS-2877
> https://issues.apache.org/jira/browse/MESOS-2877
>
>
> Repository: mesos
>
>
> Description
> -------
>
> see summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/firewall.hpp f71d6541e48e2481c69f401b388977b153503962
> 3rdparty/libprocess/src/process.cpp 52649fb90cdbefb495b68d0beb8c7f7e5ef6888e
>
> Diff: https://reviews.apache.org/r/35919/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 35919: Firewall rule's apply method returns an HTTP
response instead of an error message.
Posted by Adam B <ad...@mesosphere.io>.
> On June 29, 2015, 9:08 p.m., Adam B wrote:
> > Minor points, but otherwise shippable.
> > I guess I'm still a little confused by the motivation. When would the firewall filter rule have more information about how to respond than the caller would? Can you give an example?
>
> Alexander Rojas wrote:
> The idea is to implement SPNEGO in the least intrusive way. That can be achieve with an `FirewallRule`. The problem happens if the request doesn't include the header `Authenticate: Negotiate`, in such case, we should return an `HTTP 401 Unauthorize` response with a header `WWW-Authenticate: Negotiate U29tZSB0ZXh0Cg==`. But this cannot be done if we cannot generate the response object directly from the `FirewallRule`.
SGTM. Thanks for the explanation.
> On June 29, 2015, 9:08 p.m., Adam B wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 2019-2020
> > <https://reviews.apache.org/r/35919/diff/3/?file=994356#file994356line2019>
> >
> > You're not logging the Response body anymore?
>
> Alexander Rojas wrote:
> Previous reviewer asked to remove it. What should be done in this case?
Fair enough. I'll defer to Till's judgement.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35919/#review89854
-----------------------------------------------------------
On June 30, 2015, 1:34 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35919/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 1:34 a.m.)
>
>
> Review request for mesos, Adam B and Till Toenshoff.
>
>
> Bugs: MESOS-2877
> https://issues.apache.org/jira/browse/MESOS-2877
>
>
> Repository: mesos
>
>
> Description
> -------
>
> see summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/firewall.hpp f71d6541e48e2481c69f401b388977b153503962
> 3rdparty/libprocess/src/process.cpp 52649fb90cdbefb495b68d0beb8c7f7e5ef6888e
>
> Diff: https://reviews.apache.org/r/35919/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 35919: Firewall rule's apply method returns an HTTP
response instead of an error message.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35919/#review89854
-----------------------------------------------------------
Ship it!
Minor points, but otherwise shippable.
I guess I'm still a little confused by the motivation. When would the firewall filter rule have more information about how to respond than the caller would? Can you give an example?
3rdparty/libprocess/include/process/firewall.hpp (line 57)
<https://reviews.apache.org/r/35919/#comment142703>
s/and/an/
s/option/Option/
Maybe worth clarifying that you do not return "200 OK" for rules that apply cleanly.
3rdparty/libprocess/include/process/firewall.hpp (line 59)
<https://reviews.apache.org/r/35919/#comment142707>
So if the caller wants to override the Response, they should check for a specific response type, extract the error message, and then handle it appropriately?
3rdparty/libprocess/include/process/firewall.hpp (line 86)
<https://reviews.apache.org/r/35919/#comment142711>
How about `Forbidden("Endpoint '" + request.path + "' is disabled")` so you don't start the message with a path.
3rdparty/libprocess/src/process.cpp (lines 2019 - 2020)
<https://reviews.apache.org/r/35919/#comment142712>
You're not logging the Response body anymore?
- Adam B
On June 29, 2015, 3:21 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35919/
> -----------------------------------------------------------
>
> (Updated June 29, 2015, 3:21 a.m.)
>
>
> Review request for mesos, Adam B and Till Toenshoff.
>
>
> Bugs: MESOS-2877
> https://issues.apache.org/jira/browse/MESOS-2877
>
>
> Repository: mesos
>
>
> Description
> -------
>
> see summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/firewall.hpp f71d6541e48e2481c69f401b388977b153503962
> 3rdparty/libprocess/src/process.cpp 52649fb90cdbefb495b68d0beb8c7f7e5ef6888e
>
> Diff: https://reviews.apache.org/r/35919/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 35919: Firewall rule's apply method returns an HTTP
response instead of an error message.
Posted by Ben Mahler <be...@gmail.com>.
> On July 2, 2015, 1:35 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/firewall.hpp, line 59
> > <https://reviews.apache.org/r/35919/diff/4/?file=995819#file995819line59>
> >
> > Hm.. why is this an Owned<http::Response> as opposed to just an http::Response? Is there something subtle going on here, or can we just have Option<http::Response>?
>
> Alexander Rojas wrote:
> In order to avoid object slicing in the future. While it is true that all the `http::Response` is a struct and therefore object slicing is not an issue, nothing prevents this from changing in the future which can lead to weird errors. Personally, I prefer to discourage situations in which object slicing is a posibility.
Got it, we already have a lot of code relying on `Future<http::Response>`, so I'd suggest keeping it consistent with that and assuming that slicing is not an issue. Otherwise, people browsing the code have a hard time figuring out why things were done inconsistently :(
If we introduced some changes that made slicing a problem, we could then do one consistent sweep to capture it.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35919/#review90183
-----------------------------------------------------------
On June 30, 2015, 8:34 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35919/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 8:34 a.m.)
>
>
> Review request for mesos, Adam B and Till Toenshoff.
>
>
> Bugs: MESOS-2877
> https://issues.apache.org/jira/browse/MESOS-2877
>
>
> Repository: mesos
>
>
> Description
> -------
>
> see summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/firewall.hpp f71d6541e48e2481c69f401b388977b153503962
> 3rdparty/libprocess/src/process.cpp 52649fb90cdbefb495b68d0beb8c7f7e5ef6888e
>
> Diff: https://reviews.apache.org/r/35919/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 35919: Firewall rule's apply method returns an HTTP
response instead of an error message.
Posted by Alexander Rojas <al...@mesosphere.io>.
> On July 2, 2015, 3:35 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/firewall.hpp, line 59
> > <https://reviews.apache.org/r/35919/diff/4/?file=995819#file995819line59>
> >
> > Hm.. why is this an Owned<http::Response> as opposed to just an http::Response? Is there something subtle going on here, or can we just have Option<http::Response>?
In order to avoid object slicing in the future. While it is true that all the `http::Response` is a struct and therefore object slicing is not an issue, nothing prevents this from changing in the future which can lead to weird errors. Personally, I prefer to discourage situations in which object slicing is a posibility.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35919/#review90183
-----------------------------------------------------------
On June 30, 2015, 10:34 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35919/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 10:34 a.m.)
>
>
> Review request for mesos, Adam B and Till Toenshoff.
>
>
> Bugs: MESOS-2877
> https://issues.apache.org/jira/browse/MESOS-2877
>
>
> Repository: mesos
>
>
> Description
> -------
>
> see summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/firewall.hpp f71d6541e48e2481c69f401b388977b153503962
> 3rdparty/libprocess/src/process.cpp 52649fb90cdbefb495b68d0beb8c7f7e5ef6888e
>
> Diff: https://reviews.apache.org/r/35919/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 35919: Firewall rule's apply method returns an HTTP
response instead of an error message.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35919/#review90183
-----------------------------------------------------------
3rdparty/libprocess/include/process/firewall.hpp (line 59)
<https://reviews.apache.org/r/35919/#comment143181>
Hm.. why is this an Owned<http::Response> as opposed to just an http::Response? Is there something subtle going on here, or can we just have Option<http::Response>?
- Ben Mahler
On June 30, 2015, 8:34 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35919/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 8:34 a.m.)
>
>
> Review request for mesos, Adam B and Till Toenshoff.
>
>
> Bugs: MESOS-2877
> https://issues.apache.org/jira/browse/MESOS-2877
>
>
> Repository: mesos
>
>
> Description
> -------
>
> see summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/firewall.hpp f71d6541e48e2481c69f401b388977b153503962
> 3rdparty/libprocess/src/process.cpp 52649fb90cdbefb495b68d0beb8c7f7e5ef6888e
>
> Diff: https://reviews.apache.org/r/35919/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 35919: Firewall rule's apply method returns an HTTP
response instead of an error message.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35919/#review89880
-----------------------------------------------------------
Patch looks great!
Reviews applied: [35919]
All tests passed.
- Mesos ReviewBot
On June 30, 2015, 8:34 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35919/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 8:34 a.m.)
>
>
> Review request for mesos, Adam B and Till Toenshoff.
>
>
> Bugs: MESOS-2877
> https://issues.apache.org/jira/browse/MESOS-2877
>
>
> Repository: mesos
>
>
> Description
> -------
>
> see summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/firewall.hpp f71d6541e48e2481c69f401b388977b153503962
> 3rdparty/libprocess/src/process.cpp 52649fb90cdbefb495b68d0beb8c7f7e5ef6888e
>
> Diff: https://reviews.apache.org/r/35919/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 35919: Firewall rule's apply method returns an HTTP
response instead of an error message.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35919/
-----------------------------------------------------------
(Updated June 30, 2015, 10:34 a.m.)
Review request for mesos, Adam B and Till Toenshoff.
Changes
-------
Adam's review.
Bugs: MESOS-2877
https://issues.apache.org/jira/browse/MESOS-2877
Repository: mesos
Description
-------
see summary.
Diffs (updated)
-----
3rdparty/libprocess/include/process/firewall.hpp f71d6541e48e2481c69f401b388977b153503962
3rdparty/libprocess/src/process.cpp 52649fb90cdbefb495b68d0beb8c7f7e5ef6888e
Diff: https://reviews.apache.org/r/35919/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 35919: Firewall rule's apply method returns an HTTP
response instead of an error message.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35919/#review89728
-----------------------------------------------------------
Patch looks great!
Reviews applied: [35919]
All tests passed.
- Mesos ReviewBot
On June 29, 2015, 10:21 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35919/
> -----------------------------------------------------------
>
> (Updated June 29, 2015, 10:21 a.m.)
>
>
> Review request for mesos, Adam B and Till Toenshoff.
>
>
> Bugs: MESOS-2877
> https://issues.apache.org/jira/browse/MESOS-2877
>
>
> Repository: mesos
>
>
> Description
> -------
>
> see summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/firewall.hpp f71d6541e48e2481c69f401b388977b153503962
> 3rdparty/libprocess/src/process.cpp 52649fb90cdbefb495b68d0beb8c7f7e5ef6888e
>
> Diff: https://reviews.apache.org/r/35919/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 35919: Firewall rule's apply method returns an HTTP
response instead of an error message.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35919/
-----------------------------------------------------------
(Updated June 29, 2015, 12:21 p.m.)
Review request for mesos, Adam B and Till Toenshoff.
Changes
-------
Adresses till's comments.
Bugs: MESOS-2877
https://issues.apache.org/jira/browse/MESOS-2877
Repository: mesos
Description
-------
see summary.
Diffs (updated)
-----
3rdparty/libprocess/include/process/firewall.hpp f71d6541e48e2481c69f401b388977b153503962
3rdparty/libprocess/src/process.cpp 52649fb90cdbefb495b68d0beb8c7f7e5ef6888e
Diff: https://reviews.apache.org/r/35919/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 35919: Firewall rule's apply method returns an HTTP
response instead of an error message.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35919/
-----------------------------------------------------------
(Updated June 26, 2015, 12:18 p.m.)
Review request for mesos, Adam B and Till Toenshoff.
Changes
-------
Added include for `Owned`
Bugs: MESOS-2877
https://issues.apache.org/jira/browse/MESOS-2877
Repository: mesos
Description
-------
see summary.
Diffs (updated)
-----
3rdparty/libprocess/include/process/firewall.hpp f71d6541e48e2481c69f401b388977b153503962
3rdparty/libprocess/src/process.cpp 52649fb90cdbefb495b68d0beb8c7f7e5ef6888e
Diff: https://reviews.apache.org/r/35919/diff/
Testing
-------
make check
Thanks,
Alexander Rojas