Re: Review Request 35919: Firewall rule's apply method returns an HTTP response instead of an error message.
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 Ownedhttp::Response as opposed to just an http::Response? Is there something subtle going on here, or can we just have Optionhttp::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.
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 Ownedhttp::Response as opposed to just an http::Response? Is there something subtle going on here, or can we just have Optionhttp::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 `Futurehttp::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.
--- 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 Ownedhttp::Response as opposed to just an http::Response? Is there something subtle going on here, or can we just have Optionhttp::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.
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.
--- 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.
--- 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.
--- 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.
--- 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.
--- 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.
--- 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
Re: 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/#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
