Re: ProxyErrorOverride and redirects (PR 39245)
On 4/9/07, Nick Kew [EMAIL PROTECTED] wrote:
On Mon, 9 Apr 2007 11:08:55 -0400
Jeff Trawick [EMAIL PROTECTED] wrote:
On 4/5/07, Nick Kew [EMAIL PROTECTED] wrote:
On Thu, 5 Apr 2007 10:04:19 +0100
Joe Orton [EMAIL PROTECTED] wrote:
I agree that the intended behaviour of the original code was
intuitively correct, only = 400 errors should be overriden,
A redirection page is likely to include a redirected URL.
In a reverse proxy situation, that may need to be rewritten.
Use of ProxyErrorOverride could be a valid alternative to
mod_proxy_html in that situation, couldn't it?
Looks to me like a valid usage case for ProxyErrorOverride 3xx.
We have a couple of people trying to make their actual problem go away
permanently by keeping up with this thread and trying to match
developer comments with patches.
Yep.
IMO we should go ahead and fix what is broken now, and let
requirements for the new feature (ProxyErrorOverride nxx) present
themselves in the fullness of time. If somebody is relying on the
current feature-by-defect, then we'll find out soon enough. If there
is never a compelling requirement, then we're left with the simpler
code (which is already hard enough to keep working ;) ).
Fine by me. I was ready to commit a give-the-users-the-choice patch,
then someone objected to it. If you want to commit the no-choice
patch instead, that's OK.
Either way, it'll want an accompanying documentation patch before it
goes into a release.
I wonder why Error in ProxyErrorOverride doesn't match the meaning
of ap_is_HTTP_ERROR(), as in the attached patch (with doc).
1xx isn't something the user should see/react to either.
Index: modules/proxy/mod_proxy_http.c
===
--- modules/proxy/mod_proxy_http.c (revision 527940)
+++ modules/proxy/mod_proxy_http.c (working copy)
@@ -1448,7 +1448,7 @@
* if we are overriding the errors, we can't put the content
* of the page into the brigade
*/
-if (!conf-error_override || ap_is_HTTP_SUCCESS(r-status)) {
+if (!conf-error_override || !ap_is_HTTP_ERROR(r-status)) {
/* read the body, pass it to the output filters */
apr_read_type_e mode = APR_NONBLOCK_READ;
int finish = FALSE;
@@ -1557,7 +1557,7 @@
if (conf-error_override) {
/* the code above this checks for 'OK' which is what the hook expects
*/
-if (ap_is_HTTP_SUCCESS(r-status))
+if (!ap_is_HTTP_ERROR(r-status))
return OK;
else {
/* clear r-status for override error, otherwise ErrorDocument
Index: docs/manual/mod/mod_proxy.xml
===
--- docs/manual/mod/mod_proxy.xml (revision 527940)
+++ docs/manual/mod/mod_proxy.xml (working copy)
@@ -1196,6 +1196,9 @@
the error code and act accordingly (default behavior would display
the error page of the proxied server, turning this on shows the SSI
Error message)./p
+
+pThis directive does not affect the processing of informational (1xx),
+normal success (2xx), or redirect (3xx) responses./p
/usage
/directivesynopsis
Re: ProxyErrorOverride and redirects (PR 39245)
On Thu, Apr 12, 2007 at 10:05:06AM -0400, Jeff Trawick wrote: I wonder why Error in ProxyErrorOverride doesn't match the meaning of ap_is_HTTP_ERROR(), as in the attached patch (with doc). Great, +1 1xx isn't something the user should see/react to either. Forwarding 1xx responses is actually a 2616 MUST, but they don't have bodies so are handled separately in this code anyway (though are not in fact forwarded IIRC). joe
Re: ProxyErrorOverride and redirects (PR 39245)
On Apr 12, 2007, at 10:16 AM, Joe Orton wrote: On Thu, Apr 12, 2007 at 10:05:06AM -0400, Jeff Trawick wrote: I wonder why Error in ProxyErrorOverride doesn't match the meaning of ap_is_HTTP_ERROR(), as in the attached patch (with doc). Great, +1 1xx isn't something the user should see/react to either. Forwarding 1xx responses is actually a 2616 MUST, but they don't have bodies so are handled separately in this code anyway (though are not in fact forwarded IIRC). Agreed.
Re: ProxyErrorOverride and redirects (PR 39245)
On 4/5/07, Nick Kew [EMAIL PROTECTED] wrote: On Thu, 5 Apr 2007 10:04:19 +0100 Joe Orton [EMAIL PROTECTED] wrote: I agree that the intended behaviour of the original code was intuitively correct, only = 400 errors should be overriden, A redirection page is likely to include a redirected URL. In a reverse proxy situation, that may need to be rewritten. Use of ProxyErrorOverride could be a valid alternative to mod_proxy_html in that situation, couldn't it? Looks to me like a valid usage case for ProxyErrorOverride 3xx. We have a couple of people trying to make their actual problem go away permanently by keeping up with this thread and trying to match developer comments with patches. IMO we should go ahead and fix what is broken now, and let requirements for the new feature (ProxyErrorOverride nxx) present themselves in the fullness of time. If somebody is relying on the current feature-by-defect, then we'll find out soon enough. If there is never a compelling requirement, then we're left with the simpler code (which is already hard enough to keep working ;) ).
Re: ProxyErrorOverride and redirects (PR 39245)
On Mon, 9 Apr 2007 11:08:55 -0400 Jeff Trawick [EMAIL PROTECTED] wrote: On 4/5/07, Nick Kew [EMAIL PROTECTED] wrote: On Thu, 5 Apr 2007 10:04:19 +0100 Joe Orton [EMAIL PROTECTED] wrote: I agree that the intended behaviour of the original code was intuitively correct, only = 400 errors should be overriden, A redirection page is likely to include a redirected URL. In a reverse proxy situation, that may need to be rewritten. Use of ProxyErrorOverride could be a valid alternative to mod_proxy_html in that situation, couldn't it? Looks to me like a valid usage case for ProxyErrorOverride 3xx. We have a couple of people trying to make their actual problem go away permanently by keeping up with this thread and trying to match developer comments with patches. Yep. IMO we should go ahead and fix what is broken now, and let requirements for the new feature (ProxyErrorOverride nxx) present themselves in the fullness of time. If somebody is relying on the current feature-by-defect, then we'll find out soon enough. If there is never a compelling requirement, then we're left with the simpler code (which is already hard enough to keep working ;) ). Fine by me. I was ready to commit a give-the-users-the-choice patch, then someone objected to it. If you want to commit the no-choice patch instead, that's OK. Either way, it'll want an accompanying documentation patch before it goes into a release. -- Nick Kew Application Development with Apache - the Apache Modules Book http://www.apachetutor.org/
Re: ProxyErrorOverride and redirects (PR 39245)
On Wed, Apr 04, 2007 at 10:34:31PM +0100, Stuart Children wrote: Behaviour *has already been broken* from 2.0.x to 2.2.x - I've given evidence of this. Our work systems heavily rely on the 2.0 behaviour. Maybe someone else would like to repeat my tests - it's possible it's not as simple as I think. Checking the 1.3.x behaviour would be interesting too. The original code did only override = 400 responses, but was broken as described in PR 20183. That was changed to override all = 300 responses as what looks like a mis-guided attempt to fix PR 22951, which was probably really just PR 20183 in disguise: http://svn.apache.org/viewvc?view=revrevision=102069 the original bug, PR 20183, was later fixed: http://svn.apache.org/viewvc?view=revrevision=102935 but I confess to missing the preceding 400-300 change at the time. That leads to what is in trunk/2.2. I agree that the intended behaviour of the original code was intuitively correct, only = 400 errors should be overriden, and adding configuration foo to try to maintain compatibility is adding complexity to cover up a screw-up. Nobody actually *wants* 3xx errors to be overrided here, it makes no sense. joe
Re: ProxyErrorOverride and redirects (PR 39245)
Nick Kew wrote: A redirection page is likely to include a redirected URL. In a reverse proxy situation, that may need to be rewritten. Conceivably, yes. Though I would maintain that *by default* people with reverse-proxies would want/expect all their headers retained intact. Use of ProxyErrorOverride could be a valid alternative to mod_proxy_html in that situation, couldn't it? Not sure what you mean there - ProxyErrorOverride is a directive for mod_proxy... In any case, it does not allow one to rewrite the redirect URL. The 2.2.4 behaviour is that the Location header gets retained as-is, but all other headers are stripped out - and I can't see why anyone would want that. Looks to me like a valid usage case for ProxyErrorOverride 3xx. I don't see how it would achieve what you stated above. If you do want to rewrite headers (Location or any others), the code that deals with altering the response for what are considered error pages is surely not the place to do it. -- Stuart Children http://terminus.co.uk/ -- Visit Guardian Unlimited - the UK's most popular newspaper website http://guardian.co.uk http://observer.co.uk -- The Newspaper Marketing Agency Opening Up Newspapers http://www.nmauk.co.uk -- This e-mail and all attachments are confidential and may also be privileged. If you are not the named recipient, please notify the sender and delete the e-mail and all attachments immediately. Do not disclose the contents to another person. You may not use the information for any purpose, or store, or copy, it in any way. Guardian News Media Limited is not liable for any computer viruses or other material transmitted with or as part of this e-mail. You should employ virus checking software. Guardian News Media Limited A member of Guardian Media Group PLC Registered Office Number 1 Scott Place, Manchester M3 3GG Registered in England Number 908396
Re: ProxyErrorOverride and redirects (PR 39245)
-Ursprüngliche Nachricht- Von: Stuart Children Gesendet: Donnerstag, 5. April 2007 14:25 An: dev@httpd.apache.org Betreff: Re: ProxyErrorOverride and redirects (PR 39245) Looks to me like a valid usage case for ProxyErrorOverride 3xx. I don't see how it would achieve what you stated above. If you do want to rewrite headers (Location or any others), the code that deals with altering the response for what are considered error pages is surely not the place to do it. I guess what Nick is talking about is the body of a redirect response. A redirect response is often accompanied by a small HTML page that also contains the redirect target as a hyperlink in the case that your client does not understand the Location header. While ProxyPassReverse takes care of adjusting the Location header nobody takes care of adjusting the HTML body of the response with the wrong link. This could be fixed by an ErrorDocument for 30x response codes. IMHO only in a static matter. Regards Rüdiger
Re: ProxyErrorOverride and redirects (PR 39245)
Hi Plüm wrote: I guess what Nick is talking about is the body of a redirect response. A redirect response is often accompanied by a small HTML page that also contains the redirect target as a hyperlink in the case that your client does not understand the Location header. While ProxyPassReverse takes care of adjusting the Location header nobody takes care of adjusting the HTML body of the response with the wrong link. This could be fixed by an ErrorDocument for 30x response codes. IMHO only in a static matter. OK, that makes sense - I can see a usage for that. Though if that was what I was trying to achieve, I still wouldn't want it removing all my other headers (which is what happens as a result of going through the ErrorDocument processing) - so ProxyErrorOverride/ErrorDocument still seems like the wrong tool for the job. A module with an output filter that modifies the URLs within the body for redirect responses would be a better option (maybe as part of mod_prox_http). Cheers -- Stuart Children http://terminus.co.uk/ -- Visit Guardian Unlimited - the UK's most popular newspaper website http://guardian.co.uk http://observer.co.uk -- The Newspaper Marketing Agency Opening Up Newspapers http://www.nmauk.co.uk -- This e-mail and all attachments are confidential and may also be privileged. If you are not the named recipient, please notify the sender and delete the e-mail and all attachments immediately. Do not disclose the contents to another person. You may not use the information for any purpose, or store, or copy, it in any way. Guardian News Media Limited is not liable for any computer viruses or other material transmitted with or as part of this e-mail. You should employ virus checking software. Guardian News Media Limited A member of Guardian Media Group PLC Registered Office Number 1 Scott Place, Manchester M3 3GG Registered in England Number 908396
Re: ProxyErrorOverride and redirects (PR 39245)
On Wed, Apr 04, 2007 at 10:48:58AM -0400, Eric Covener wrote: There's a simple patch attached to the report which tells httpd to keep its hands off ap_is_HTTPD_REDIRECT e.g. 3xx. Given the description of ProxyErrorOverride I wouldn't think expectations would be that 3xx responses would be overridden. That's my expectation too. Whatever people argue, I've shown in this comment: http://issues.apache.org/bugzilla/show_bug.cgi?id=39245#c11 that it's a change in behaviour since 2.0.x. The actual patch on the report might be out of date for trunk. Try the ones on http://issues.apache.org/bugzilla/show_bug.cgi?id=41601 (which I filed but closed as a dupe of the above). Though I've not checked them recently. They're making the same change. I had been meaning to chase this (and the two other bugs I found in this area of code whilst investigating - see later comments in the report). Unfortunately I'm about to go on holiday for three weeks and shall be staying far away from computers! Happy to help when I'm back though. Regards -- Stuart Children http://terminus.co.uk/
Re: ProxyErrorOverride and redirects (PR 39245)
On Wed, 4 Apr 2007 10:48:58 -0400
Eric Covener [EMAIL PROTECTED] wrote:
Can any proxy gurus reconsider this bug status?
Hmmm, I have some recollection of debating this before,
presumably without reaching any consensus for change.
-if (conf-error_override == 0 ||
ap_is_HTTP_SUCCESS(r-status)) {
+if (conf-error_override == 0 ||
ap_is_HTTP_SUCCESS(r-status) || ap_is_HTTP_REDIRECT(r-status)) {
I don't think we could accept that, because it breaks existing
behaviour someone might be relying on.
What I'd suggest is giving conf-error_override a numeric value
rather than an On/Off flag, and checking
r-status conf-error_override
That way we'll get existing behaviour with it set to 300 (so we
can alias On to that), and get the behaviour you want with 400
(which could perhaps be given another alias).
Any objections to that?
--
Nick Kew
Application Development with Apache - the Apache Modules Book
http://www.apachetutor.org/
Re: ProxyErrorOverride and redirects (PR 39245)
On 4/4/07, Nick Kew [EMAIL PROTECTED] wrote:
On Wed, 4 Apr 2007 10:48:58 -0400
Eric Covener [EMAIL PROTECTED] wrote:
Can any proxy gurus reconsider this bug status?
Hmmm, I have some recollection of debating this before,
presumably without reaching any consensus for change.
-if (conf-error_override == 0 ||
ap_is_HTTP_SUCCESS(r-status)) {
+if (conf-error_override == 0 ||
ap_is_HTTP_SUCCESS(r-status) || ap_is_HTTP_REDIRECT(r-status)) {
I don't think we could accept that, because it breaks existing
behaviour someone might be relying on.
What I'd suggest is giving conf-error_override a numeric value
rather than an On/Off flag, and checking
r-status conf-error_override
That way we'll get existing behaviour with it set to 300 (so we
can alias On to that), and get the behaviour you want with 400
(which could perhaps be given another alias).
Any objections to that?
Thanks Nick -- Looks reasonable, but flipping the comparison might
make more sense based on the structure of context:
# 2.2.4 behavior
# Alias for ProxyErrorOverride 300
ProxyErrorOverride on
# Only override 4xx/5xx
# Alias as ERRORS ?
ProxyErrorOverride 400
if (conf-error_override == 0 ||
ap_is_HTTP_SUCCESS(r-status) ||
r-status = conf-error_override) {
// no override
--
Eric Covener
[EMAIL PROTECTED]
Re: ProxyErrorOverride and redirects (PR 39245)
On 4/4/07, Eric Covener [EMAIL PROTECTED] wrote: but flipping the comparison might make more sense based on the structure of context: Scratch that of course, -- Eric Covener [EMAIL PROTECTED]
Re: ProxyErrorOverride and redirects (PR 39245)
On Wed, Apr 04, 2007 at 04:30:31PM +0100, Nick Kew wrote:
Hmmm, I have some recollection of debating this before,
Yes, in the comments of the bug the OP linked.
presumably without reaching any consensus for change.
Well no-one's refuted (or replied to in any fashion) my last posts in
bugzilla.
-if (conf-error_override == 0 ||
ap_is_HTTP_SUCCESS(r-status)) {
+if (conf-error_override == 0 ||
ap_is_HTTP_SUCCESS(r-status) || ap_is_HTTP_REDIRECT(r-status)) {
I don't think we could accept that, because it breaks existing
behaviour someone might be relying on.
Behaviour *has already been broken* from 2.0.x to 2.2.x - I've given
evidence of this. Our work systems heavily rely on the 2.0 behaviour.
Maybe someone else would like to repeat my tests - it's possible
it's not as simple as I think. Checking the 1.3.x behaviour would be
interesting too.
Without any evidence to the contrary though, I'd really appeal for the
patch to be applied.
Whatever happens, the documentation should be clarified. But to my mind
there is no doubt that I would not expect a redirect (or Not Modified
for that matter) to be considered an error.
Cheers
--
Stuart
Re: ProxyErrorOverride and redirects (PR 39245)
On Wed, 4 Apr 2007 22:34:31 +0100 Stuart Children [EMAIL PROTECTED] wrote: Whatever happens, the documentation should be clarified. But to my mind there is no doubt that I would not expect a redirect (or Not Modified for that matter) to be considered an error. The semantics of Error here are the same as in ErrorDocument (which is, after all, what you're invoking with ProxyErrorOverride). -- Nick Kew Application Development with Apache - the Apache Modules Book http://www.apachetutor.org/
Re: ProxyErrorOverride and redirects (PR 39245)
On 4/4/07, Nick Kew [EMAIL PROTECTED] wrote: What I'd suggest is giving conf-error_override a numeric value rather than an On/Off flag, and checking r-status conf-error_override That way we'll get existing behaviour with it set to 300 (so we can alias On to that), and get the behaviour you want with 400 (which could perhaps be given another alias). new patch available that preserves the old behavior wrt 3xx responses using the strategy quoted above -- no thought given to change in behavior of 1xx responses http://issues.apache.org/bugzilla/show_bug.cgi?id=39245 -- Eric Covener [EMAIL PROTECTED]
Re: ProxyErrorOverride and redirects (PR 39245)
On Wed, Apr 04, 2007 at 10:40:27PM +0100, Nick Kew wrote: Whatever happens, the documentation should be clarified. But to my mind there is no doubt that I would not expect a redirect (or Not Modified for that matter) to be considered an error. The semantics of Error here are the same as in ErrorDocument (which is, after all, what you're invoking with ProxyErrorOverride). Absolutely. Only if I RTFM: http://httpd.apache.org/docs/2.2/mod/core.html#errordocument it says In the event of a problem or error. So what's wrong with what I've said? I take problem/error to not include 3xx responses. If you disagree then the documentation would appear to not be clear enough to one or other of us. Given it does not explicitly indicate behaviour in this particular matter, we should look at what the code does; which _was_ not to treat them as errors... until someone made a change at which point it broke peoples' assumptions/environments and caused them to file bugs. -- Stuart
Re: ProxyErrorOverride and redirects (PR 39245)
On Wed, 4 Apr 2007 18:08:14 -0400 Eric Covener [EMAIL PROTECTED] wrote: new patch available that preserves the old behavior wrt 3xx responses using the strategy quoted above -- no thought given to change in behavior of 1xx responses http://issues.apache.org/bugzilla/show_bug.cgi?id=39245 Thanks. Bug me if I don't get around to reviewing/applying it. -- Nick Kew Application Development with Apache - the Apache Modules Book http://www.apachetutor.org/
