Re: [1.3] Proxy fixes and FWD: Re: [apache-modules] Setting bytes_sent in Request Record while generating all headers by myself in Apache 1.3]

[Graham Leggett]

Martin Kraemer wrote:

> About the X-Forwarded-* stuff: It's non-standard anyway (you can add
> any X-whatever header and still be RFC2616 compliant) so I'd rather
> not see it in 1.3 now (maybe in the next release ;-)
> 
> I've seen proxies like squid hang because of nonstandard headers
> (proxy-connection: keep-alive) so I am not very fond of polluting the
> header namespace with more non-standard cruft.

These headers have been present for a long time in the httpd-2.0
mod_proxy, and are also added as standard by Squid, so they do
definitely work. As there is not likely to be another release unless new
bugs are found, I would rather add it now and see it included, rather
than add it later and have it never released.

I'll commit it now (as I am in an internet cafe and cannot commit it
later), however if there are any -1's please revert the commit before
the release.

Regards,
Graham
-- 
-
[EMAIL PROTECTED]"There's a moon
over Bourbon Street
tonight..."


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [1.3] Proxy fixes and FWD: Re: [apache-modules] Setting bytes_sent in Request Record while generating all headers by myself in Apache 1.3]

[Thomas Eibner]

On Wed, May 29, 2002 at 10:44:16PM +0200, Martin Kraemer wrote:
> Yes, the patch was correct (IMHO) and yes, I committed this one.
> 
> About the X-Forwarded-* stuff: It's non-standard anyway (you can add
> any X-whatever header and still be RFC2616 compliant) so I'd rather
> not see it in 1.3 now (maybe in the next release ;-)

My only reasoning for wanting it in was that it's the way it works in
mod_proxy in 2.0.

> I've seen proxies like squid hang because of nonstandard headers
> (proxy-connection: keep-alive) so I am not very fond of polluting the
> header namespace with more non-standard cruft.

Not good.

-- 
  Thomas Eibner  DnsZone 
  mod_pointer  
  !(C)  
  Putting the HEST in .COM 

Re: [1.3] Proxy fixes and FWD: Re: [apache-modules] Setting bytes_sent in Request Record while generating all headers by myself in Apache 1.3]

[Martin Kraemer]

On Wed, May 29, 2002 at 06:28:24PM +0200, Thomas Eibner wrote:
> From: Anthony Howe <[EMAIL PROTECTED]>
> Subject: Re: [apache-modules] Setting bytes_sent in Request Record while generating
>  all headers by myself in Apache 1.3
> 
>  >Number: 6841
>ap_kill_timeout(r);
> +
> + r->bytes_sent += total_bytes_rcvd;
> +

Yes, the patch was correct (IMHO) and yes, I committed this one.

About the X-Forwarded-* stuff: It's non-standard anyway (you can add
any X-whatever header and still be RFC2616 compliant) so I'd rather
not see it in 1.3 now (maybe in the next release ;-)

I've seen proxies like squid hang because of nonstandard headers
(proxy-connection: keep-alive) so I am not very fond of polluting the
header namespace with more non-standard cruft.

   Martin
-- 
<[EMAIL PROTECTED]> | Fujitsu Siemens
Fon: +49-89-636-46021, FAX: +49-89-636-47655 | 81730  Munich,  Germany

Re: [1.3] Proxy fixes and FWD: Re: [apache-modules] Setting bytes_sent in Request Record while generating all headers by myself in Apache 1.3]

[Thomas Eibner]

On Wed, May 29, 2002 at 08:18:53PM +0200, Thomas Eibner wrote:
> On Wed, May 29, 2002 at 02:02:53PM -0400, Greg Marr wrote:
> > At 01:30 PM 05/29/2002, Thomas Eibner wrote:
> > >Index: proxy_http.c
> > >===
> > >RCS file: /home/cvspublic/apache-1.3/src/modules/proxy/proxy_http.c,v
> > >retrieving revision 1.98
> > >diff -u -r1.98 proxy_http.c
> > >--- proxy_http.c21 Apr 2002 21:16:39 -  1.98
> > >+++ proxy_http.c29 May 2002 17:04:38 -
> > >@@ -350,6 +350,20 @@
> > >   * where the original request came from.
> > >   */
> > >  ap_table_mergen(req_hdrs, "X-Forwarded-For", 
> > > r->connection->remote_ip);
> > >+if (r->proxyreq == PROXY_PASS) {
> > >+const char *buf;
> > >+/* Add X-Forwarded-Host: so that upstream knows what the
> > >+ * original request hostname was.
> > >+ */
> > >+if ((buf - ap_table_get(r->headers_in, "Host"))) {
> > 
> > I think this should be "buf =" instead of "buf -".
> 
> Yup, and:
> 
> +ap_table_mergen(req_hdrs, "X-Forwarded-Server", r->server_hostname
> 
> Should be r->server->server_hostname too.
> 
> But it seems that getting the Host at this point is already to late.
> buf contains the remote hostname here already.

Which was fixed by the s/-/=/, thanks Greg.

-- 
  Thomas Eibner  DnsZone 
  mod_pointer  
  !(C)  
  Putting the HEST in .COM 

Re: [1.3] Proxy fixes and FWD: Re: [apache-modules] Setting bytes_sent in Request Record while generating all headers by myself in Apache 1.3]

[Thomas Eibner]

On Wed, May 29, 2002 at 02:02:53PM -0400, Greg Marr wrote:
> At 01:30 PM 05/29/2002, Thomas Eibner wrote:
> >Index: proxy_http.c
> >===
> >RCS file: /home/cvspublic/apache-1.3/src/modules/proxy/proxy_http.c,v
> >retrieving revision 1.98
> >diff -u -r1.98 proxy_http.c
> >--- proxy_http.c21 Apr 2002 21:16:39 -  1.98
> >+++ proxy_http.c29 May 2002 17:04:38 -
> >@@ -350,6 +350,20 @@
> >   * where the original request came from.
> >   */
> >  ap_table_mergen(req_hdrs, "X-Forwarded-For", 
> > r->connection->remote_ip);
> >+if (r->proxyreq == PROXY_PASS) {
> >+const char *buf;
> >+/* Add X-Forwarded-Host: so that upstream knows what the
> >+ * original request hostname was.
> >+ */
> >+if ((buf - ap_table_get(r->headers_in, "Host"))) {
> 
> I think this should be "buf =" instead of "buf -".

Yup, and:

+ap_table_mergen(req_hdrs, "X-Forwarded-Server", r->server_hostname

Should be r->server->server_hostname too.

But it seems that getting the Host at this point is already to late.
buf contains the remote hostname here already.

-- 
  Thomas Eibner  DnsZone 
  mod_pointer  
  !(C)  
  Putting the HEST in .COM 

Re: [1.3] Proxy fixes and FWD: Re: [apache-modules] Setting bytes_sent in Request Record while generating all headers by myself in Apache 1.3]

[Greg Marr]

At 01:30 PM 05/29/2002, Thomas Eibner wrote:
>Index: proxy_http.c
>===
>RCS file: /home/cvspublic/apache-1.3/src/modules/proxy/proxy_http.c,v
>retrieving revision 1.98
>diff -u -r1.98 proxy_http.c
>--- proxy_http.c21 Apr 2002 21:16:39 -  1.98
>+++ proxy_http.c29 May 2002 17:04:38 -
>@@ -350,6 +350,20 @@
>   * where the original request came from.
>   */
>  ap_table_mergen(req_hdrs, "X-Forwarded-For", 
> r->connection->remote_ip);
>+if (r->proxyreq == PROXY_PASS) {
>+const char *buf;
>+/* Add X-Forwarded-Host: so that upstream knows what the
>+ * original request hostname was.
>+ */
>+if ((buf - ap_table_get(r->headers_in, "Host"))) {

I think this should be "buf =" instead of "buf -".

-- 
Greg Marr
[EMAIL PROTECTED]
"We thought you were dead."
"I was, but I'm better now." - Sheridan, "The Summoning"

Re: [1.3] Proxy fixes and FWD: Re: [apache-modules] Setting bytes_sent in Request Record while generating all headers by myself in Apache 1.3]

[Thomas Eibner]

On Wed, May 29, 2002 at 07:44:16PM +0200, Graham Leggett wrote:
> Thomas Eibner wrote:
> 
> > Inline patch here, but I'm wondering if you want the X-Forwarded-For
> > header to be stuck inside the conditional too?
> 
> I think it should be... will sort this out later tonight or first thing
> tomorrow, have to leave the internet cafe now to fetch someone.

Good, there was a small mistake in the patch anyway. I'll setup a
test for it later so I can see if it actually works (doh).

-- 
  Thomas Eibner  DnsZone 
  mod_pointer  
  !(C)  
  Putting the HEST in .COM 

Re: [1.3] Proxy fixes and FWD: Re: [apache-modules] Setting bytes_sent in Request Record while generating all headers by myself in Apache 1.3]

[Graham Leggett]

Thomas Eibner wrote:

> Inline patch here, but I'm wondering if you want the X-Forwarded-For
> header to be stuck inside the conditional too?

I think it should be... will sort this out later tonight or first thing
tomorrow, have to leave the internet cafe now to fetch someone.

Regards,
Graham
-- 
-
[EMAIL PROTECTED]"There's a moon
over Bourbon Street
tonight..."


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [1.3] Proxy fixes and FWD: Re: [apache-modules] Setting bytes_sent in Request Record while generating all headers by myself in Apache 1.3]

[Thomas Eibner]

On Wed, May 29, 2002 at 07:20:17PM +0200, Graham Leggett wrote:
> Thomas Eibner wrote:
> 
> > Ah yes, X-Forwarded-For is there, but not the two others there is in
> > 2.0 (X-Forwarded-Server and X-Forwared-Host) I read in the source that
> > someone thinks it needs to go into the Via header instead. And as I can
> > read from the source, X-Forwarded-For is only sent when it's a reverse
> > proxy request in 2.0, and always sent in 1.3.
> 
> Oh yes... I remember now (memory rusty). If you can get a patch in
> before tomorrow, should be cool :)

Inline patch here, but I'm wondering if you want the X-Forwarded-For
header to be stuck inside the conditional too? 

Index: proxy_http.c
===
RCS file: /home/cvspublic/apache-1.3/src/modules/proxy/proxy_http.c,v
retrieving revision 1.98
diff -u -r1.98 proxy_http.c
--- proxy_http.c21 Apr 2002 21:16:39 -  1.98
+++ proxy_http.c29 May 2002 17:04:38 -
@@ -350,6 +350,20 @@
  * where the original request came from.
  */
 ap_table_mergen(req_hdrs, "X-Forwarded-For", r->connection->remote_ip);
+if (r->proxyreq == PROXY_PASS) {
+const char *buf;
+/* Add X-Forwarded-Host: so that upstream knows what the
+ * original request hostname was.
+ */
+if ((buf - ap_table_get(r->headers_in, "Host"))) {
+ap_table_mergen(req_hdrs, "X-Forwarded-Host", buf);
+}
+/* Add X-Forwarded-Server: so that upstream knows what the
+ * name of this proxy server is (if there are more than one)
+ * XXX: This duplicates Via: - do we strictly need it?
+ */
+ap_table_mergen(req_hdrs, "X-Forwarded-Server", r->server_hostname);
+} 
 
 /* we don't yet support keepalives - but we will soon, I promise! */
 ap_table_set(req_hdrs, "Connection", "close");


-- 
  Thomas Eibner  DnsZone 
  mod_pointer  
  !(C)  
  Putting the HEST in .COM 

Re: [1.3] Proxy fixes and FWD: Re: [apache-modules] Setting bytes_sent in Request Record while generating all headers by myself in Apache 1.3]

[Graham Leggett]

Thomas Eibner wrote:

> Ah yes, X-Forwarded-For is there, but not the two others there is in
> 2.0 (X-Forwarded-Server and X-Forwared-Host) I read in the source that
> someone thinks it needs to go into the Via header instead. And as I can
> read from the source, X-Forwarded-For is only sent when it's a reverse
> proxy request in 2.0, and always sent in 1.3.

Oh yes... I remember now (memory rusty). If you can get a patch in
before tomorrow, should be cool :)

Regards,
Graham
-- 
-
[EMAIL PROTECTED]"There's a moon
over Bourbon Street
tonight..."


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [1.3] Proxy fixes and FWD: Re: [apache-modules] Setting bytes_sent in Request Record while generating all headers by myself in Apache 1.3]

[Thomas Eibner]

On Wed, May 29, 2002 at 07:10:25PM +0200, Graham Leggett wrote:
> Thomas Eibner wrote:
> 
> > Looking at apache-1.3 in cvs VS httpd-2.0 there seems to be a few
> > changes, and the X-Forwarded-* headers are some of them. I have a
> > patch ready if needed (with what I believe are your comments in
> > it).
> 
> Just checked - the X-Forwarded-For is definitely there in v1.3. Dunno
> about the send_bytes fix. Can someone check for me?

Ah yes, X-Forwarded-For is there, but not the two others there is in
2.0 (X-Forwarded-Server and X-Forwared-Host) I read in the source that
someone thinks it needs to go into the Via header instead. And as I can
read from the source, X-Forwarded-For is only sent when it's a reverse
proxy request in 2.0, and always sent in 1.3.

-- 
  Thomas Eibner  DnsZone 
  mod_pointer  
  !(C)  
  Putting the HEST in .COM 

Re: [1.3] Proxy fixes and FWD: Re: [apache-modules] Setting bytes_sent in Request Record while generating all headers by myself in Apache 1.3]

[Graham Leggett]

Thomas Eibner wrote:

> Looking at apache-1.3 in cvs VS httpd-2.0 there seems to be a few
> changes, and the X-Forwarded-* headers are some of them. I have a
> patch ready if needed (with what I believe are your comments in
> it).

Just checked - the X-Forwarded-For is definitely there in v1.3. Dunno
about the send_bytes fix. Can someone check for me?

Regards,
Graham
-- 
-
[EMAIL PROTECTED]"There's a moon
over Bourbon Street
tonight..."


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [1.3] Proxy fixes and FWD: Re: [apache-modules] Setting bytes_sent in Request Record while generating all headers by myself in Apache 1.3]

[Thomas Eibner]

On Wed, May 29, 2002 at 06:47:20PM +0200, Graham Leggett wrote:
> Thomas Eibner wrote:
> 
> > Anyone looked at the remaining open bugs in 1.3 and might want to include
> > this patch (and bug)?
> 
> Only if someone can verify that this patch actually does anything. The
> proxy has been largely rewritten since then, so this bug might not still
> be outstanding.
> 
> There is a release coming out in another day or two, I am stuck offline
> - can someone take a look at this fix and see if it works...?
>
> > Would it also be possible to have mod_proxy for 1.3 set the same
> > X-Forwarded-* headers as the 2.0 proxy does? Need patches for this?
> 
> I thought v1.3.24 already did this - don't remember if I added it to
> v1.3 as well, I am sure I did.

Looking at apache-1.3 in cvs VS httpd-2.0 there seems to be a few
changes, and the X-Forwarded-* headers are some of them. I have a
patch ready if needed (with what I believe are your comments in
it).

-- 
  Thomas Eibner  DnsZone 
  mod_pointer  
  !(C)  
  Putting the HEST in .COM 

Re: [1.3] Proxy fixes and FWD: Re: [apache-modules] Setting bytes_sent in Request Record while generating all headers by myself in Apache 1.3]

[Graham Leggett]

Thomas Eibner wrote:

> Anyone looked at the remaining open bugs in 1.3 and might want to include
> this patch (and bug)?

Only if someone can verify that this patch actually does anything. The
proxy has been largely rewritten since then, so this bug might not still
be outstanding.

There is a release coming out in another day or two, I am stuck offline
- can someone take a look at this fix and see if it works...?

> Would it also be possible to have mod_proxy for 1.3 set the same
> X-Forwarded-* headers as the 2.0 proxy does? Need patches for this?

I thought v1.3.24 already did this - don't remember if I added it to
v1.3 as well, I am sure I did.

Regards,
Graham
-- 
-
[EMAIL PROTECTED]"There's a moon
over Bourbon Street
tonight..."


smime.p7s
Description: S/MIME Cryptographic Signature

[1.3] Proxy fixes and FWD: Re: [apache-modules] Setting bytes_sent in Request Record while generating all headers by myself in Apache 1.3]

[Thomas Eibner]

Anyone looked at the remaining open bugs in 1.3 and might want to include
this patch (and bug)?

Would it also be possible to have mod_proxy for 1.3 set the same
X-Forwarded-* headers as the 2.0 proxy does? Need patches for this?

- Forwarded message from Anthony Howe <[EMAIL PROTECTED]> -

Date: Wed, 29 May 2002 18:23:00 +0200
From: Anthony Howe <[EMAIL PROTECTED]>
To: [EMAIL PROTECTED]
Reply-To: [EMAIL PROTECTED]
Subject: Re: [apache-modules] Setting bytes_sent in Request Record while generating
 all headers by myself in Apache 1.3
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.0rc3) Gecko/20020523
MIME-Version: 1.0

Thomas Eibner wrote:
> On Tue, May 28, 2002 at 01:21:59PM +0200, Anthony Howe wrote:
> 
>>Are you using Apache 1.3? Are you using mod_proxy? I submitted a long 
>>time ago a patch to Apache to fix a bug in mod_proxy, which fails to 
>>update the bytes_sent field. Don't know if they ever included it, but I 
>>have it still if required.
> 
> 
> Why not check up on wheter it actually was included/fixed and then submit
> a patch if it is still broken? Otherwise it will never make it into the
> 1.3 branch (which btw is having it's last release very soon now.)

Well from what I can tell from just looking at 1.3.24, they STILL 
haven't included the patch.

I'm sure I already submitted the patch sometime ago, 14 Nov 2000, old 
bug database number 6841, and its STILL open.

I figure if they can't address a one line patch when it was submitted 
then, why should I keep pushing for something only to be ignored. 
Obviously someone doesn't think my input is worth anything or that my 
patch breaks something else. I figured I was doing my part by submitting 
the bug in the first place.

Oh hum.

->8--

Full text of PR number 6841:

Received: (qmail 3213 invoked by uid 501); 14 Nov 2000 14:17:09 -
Message-Id: <[EMAIL PROTECTED]>
Date: 14 Nov 2000 14:17:09 -
From: Anthony Howe <[EMAIL PROTECTED]>
Reply-To: [EMAIL PROTECTED]
To: [EMAIL PROTECTED]
Subject: mod_proxy does not maintain the request_rec->bytes_sent field.
X-Send-Pr-Version: 3.110

 >Number: 6841
 >Category:   mod_proxy
 >Synopsis:   mod_proxy does not maintain the 
request_rec->bytes_sent field.
 >Confidential:   no
 >Severity:   non-critical
 >Priority:   medium
 >Responsible:apache
 >State:  open
 >Quarter:
 >Keywords:
 >Date-Required:
 >Class:  sw-bug
 >Submitter-Id:   apache
 >Arrival-Date:   Tue Nov 14 06:20:01 PST 2000
 >Closed-Date:
 >Last-Modified:
 >Originator: [EMAIL PROTECTED]
 >Release:1.3.14
 >Organization:
apache
 >Environment:
Linux mail.snert.net 2.0.34C52_SK #1 Tue Nov 30 18:14:40 PST 1999 mips 
unknown
 >Description:
A user reported a bug against mod_throttle claiming that mod_throttle 
failed to
record the number of bytes sent when the request passed through mod_proxy.
Apon debugging and examination of the mod_proxy source, I found that
ap_proxy_send_fb() tracked and returned the number of bytes received/sent,
but that NO ONE made use of the return value to update the request_rec's
bytes_sent field.

Find enclosed a one line change to src/modules/proxy/proxy_util.c that 
updates
the request_rec.

By making the change in ap_proxy_send_fb(), http and ftp response from the
remote server or from the cache will all correctly update the request_rec
so that other modules can make use of this information in the logging phase.

 >How-To-Repeat:

 >Fix:
*** proxy_util.c.orig   Tue Nov 14 14:34:42 2000
--- proxy_util.cTue Nov 14 14:49:25 2000
***
*** 618,623 
--- 618,626 
ap_bflush(con->client);

   ap_kill_timeout(r);
+
+   r->bytes_sent += total_bytes_rcvd;
+
   return total_bytes_rcvd;
   }

 >Release-Note:
 >Audit-Trail:
 >Unformatted:
  [In order for any reply to be added to the PR database, you need]
  [to include <[EMAIL PROTECTED]> in the Cc line and make sure the]
  [subject line starts with the report component and number, with ]
  [or without any 'Re:' prefixes (such as "general/1098:" or  ]
  ["Re: general/1098:").  If the subject doesn't match this   ]
  [pattern, your message will be misfiled and ignored.  The   ]
  ["apbugs" address is not added to the Cc line of messages from  ]
  [the database automatically because of the potential for mail   ]
  [loops.  If you do not include this Cc, your reply may be ig-   ]
  [nored unless you are responding to an explicit request from a  ]
  [developer.  Reply only with text; DO NOT SEND ATTACHMENTS! ]



->8--



-- 
Anthony C Howe+33 6 11 89 73 78
http://www.snert.com/ ICQ: 7116561  AIM: Sir Wumpus
"Microsoft (cough, sputter, spit, !@#$%) ..."


-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

- End forwarded message -

-- 
  Thomas Eibner