Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (nike.apache.org: domain of ylavic.dev@gmail.com designates
 74.125.82.50 as permitted sender)
Message-ID: <52850C32.7010508@gmail.com>
Date: Thu, 14 Nov 2013 18:45:22 +0100
From: Yann Ylavic <ylavic.dev@gmail.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64;
 rv:24.0) Gecko/20100101 Thunderbird/24.1.0
MIME-Version: 1.0
To: dev@httpd.apache.org
Subject: mod_proxy ping and 100-continue (was Re: NOTE: Intent to T&R 2.2.6
 tomorrow)
References: <52850415.2070906@gmail.com>
In-Reply-To: <52850415.2070906@gmail.com>
Content-Type: multipart/mixed;
 boundary="------------030104010303070707030700"

This is a multi-part message in MIME format.
--------------030104010303070707030700
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit


I peek this message from another thread and create a new one, since
details may not be relevant in the T&R note.


On 11/12/2013 06:56 PM, William A. Rowe Jr. wrote:
> On Tue, 12 Nov 2013 11:48:16 -0500
> Jim Jagielski <jim@jaguNET.com> wrote:
>
>> I intend to T&R 2.2.26 tomorrow... post now if that's
>> an issue or problem...
> As I mentioned earlier, two additional patches should possibly be
> considered for protocol correctness.  The first you shepherded into
> trunk, so I'm particularly interested in your thoughts on backporting
> this, Jim...
>
> http://svn.apache.org/viewvc?view=revision&revision=1524192
> http://svn.apache.org/viewvc?view=revision&revision=1524770
> (Note that the commit log message is missing patch attribution)
>
> A backport is attached, as best as I've figured from the trunk-modulo-
> 2.2 code path.
>
> The second is the 100-continue behavior, when proxy-interim-response is
> set to RFC.  As Yann noted in a very long and winding message thread,
> the core http filter is pushing a 100 continue interim status, and then
> mod_proxy_http is pushing back yet another interim status response.  The
> core status response must be suppressed on proxy-interim-response RFC
> requests.
>
> It's not clear where that discussion thread has ended up, or whether
> there is a usable patch to enforce this behavior.  As you had the most
> to contribute to that thread, can you give us your thoughts on its
> current status, Jim?

Actually the issue I described does not depend on the
proxy-interim-response value: when a (positive) ping is configured for
the worker, mod_proxy_http (via ap_proxy_create_hdrbrgd) will set
r->expecting_100 which will cause the http input filter to push a "100
Continue" to the client, before anything is forwarded (when the request
body is prefetched), whether or not the client expects one (eg. "Expect:
100-continue" or not).

But I agree that the proxy-interim-response == "RFC" is an issue too,
per se.

In fact I don't really see why any "100 Continue" response from the
backend should be forwarded to the client, since the client has already
received one during prefetch (from the http input filter, if expected).
When the (interim) response is about to be forwarded, the request has
already been read (fully), hence the client has nothing more to continue...
Forcing another "100 Continue" here (when proxy-interim-response ==
"RFC") is at least useless (it will be ignored), at worst not rfc
compliant (can a receiver send multiple "100 Continue"? when it has
everything already?).

IMHO, the root issue is that mod_proxy_http cannot be an expectation
proxy with its current implementation (forward all the request, then
forward all the response). It acts more as a server for the client and a
client for the backend, and hence should follow the related RFC parts
(not the ones for an expectation proxy, which I guess is what is
intended by the proxy-interim-response == "RFC").

So the 100-continue with the client should be a client<->proxy only
stuff (controlled by r->expecting_100), and the one with the backend
another proxy<->backend only stuff (with the interim responses being
eaten by the proxy), both should be handled separately.

Below is the patch for ap_proxy_create_hdrbrgd (attached too), but I
think a separate client<->proxy and proxy<->backend handling would be
better, I can try to propose it if you agree.

Regards,
Yann.

Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c    (revision 1541907)
+++ modules/proxy/proxy_util.c    (working copy)
@@ -3126,7 +3126,7 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
       apr_bucket *e;
       int do_100_continue;
       conn_rec *origin = p_conn->connection;
-    const char *fpr1;
+    const char *fpr1, *expect;
       proxy_dir_conf *dconf = ap_get_module_config(r->per_dir_config,
&proxy_module);

       /*
@@ -3227,13 +3227,37 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
                            );
       }

-    /* Use HTTP/1.1 100-Continue as quick "HTTP ping" test
-     * to backend
+    /* Use HTTP/1.1 100-Continue as quick "HTTP ping" test to backend.
+     * If a ping test was already tried with the same request, restore
+     * the original Expect header to avoid using the modified one with
+     * subsequent requests (the new backend may not be ping-able).
        */
+    expect = apr_table_get(r->notes, "proxy-ping-expect");
+    if (expect == NULL) {
+        expect = apr_table_get(r->headers_in, "Expect");
+        if (expect != NULL) {
+            apr_table_setn(r->notes, "proxy-ping-expect", expect);
+        }
+        else {
+            apr_table_setn(r->notes, "proxy-ping-expect", "\n");
+        }
+    }
+    else if (strcmp(expect, "\n") != 0) {
+        apr_table_setn(r->headers_in, "Expect", expect);
+    }
+    else {
+        apr_table_unset(r->headers_in, "Expect");
+        expect = NULL;
+    }
       if (do_100_continue) {
-        apr_table_mergen(r->headers_in, "Expect", "100-Continue");
-        r->expecting_100 = 1;
+        if (!ap_find_token(r->pool, expect, "100-Continue")) {
+            apr_table_mergen(r->headers_in, "Expect", "100-Continue");
+        }
+        apr_table_setn(r->notes, "proxy-ping-100-continue", "1");
       }
+    else {
+        apr_table_unset(r->notes, "proxy-ping-100-continue");
+    }

       /* X-Forwarded-*: handling
        *
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c    (revision 1541907)
+++ modules/proxy/mod_proxy_http.c    (working copy)
@@ -1537,10 +1537,12 @@ int ap_proxy_http_process_response(apr_pool_t * p,
                * We need to set "r->expecting_100 = 1" otherwise origin
                * server behaviour will apply.
                */
+            ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
+                          "HTTP: received interim %d response", r->status);
+            if ((r->status != HTTP_CONTINUE) ||
+                    !apr_table_get(r->notes, "proxy-ping-100-continue")) {
               const char *policy = apr_table_get(r->subprocess_env,
"proxy-interim-response");
-            ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
-                          "HTTP: received interim %d response", r->status);
               if (!policy
                       || (!strcasecmp(policy, "RFC") &&
((r->expecting_100 = 1)))) {
                   ap_send_interim_response(r, 1);
@@ -1552,6 +1554,7 @@ int ap_proxy_http_process_response(apr_pool_t * p,
                   ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r,
APLOGNO(01108)
                                 "undefined proxy interim response policy");
               }
+            }
           }
           /* Moved the fixups of Date headers and those affected by
            * ProxyPassReverse/etc from here to ap_proxy_read_headers
[EOS]


--------------030104010303070707030700
Content-Type: text/x-patch;
 name="httpd-trunk-mod_proxy_ping_expectin_100.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="httpd-trunk-mod_proxy_ping_expectin_100.patch"

Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c	(revision 1541907)
+++ modules/proxy/proxy_util.c	(working copy)
@@ -3126,7 +3126,7 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
     apr_bucket *e;
     int do_100_continue;
     conn_rec *origin = p_conn->connection;
-    const char *fpr1;
+    const char *fpr1, *expect;
     proxy_dir_conf *dconf = ap_get_module_config(r->per_dir_config, &proxy_module);
 
     /*
@@ -3227,13 +3227,37 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
                          );
     }
 
-    /* Use HTTP/1.1 100-Continue as quick "HTTP ping" test
-     * to backend
+    /* Use HTTP/1.1 100-Continue as quick "HTTP ping" test to backend.
+     * If a ping test was already tried with the same request, restore
+     * the original Expect header to avoid using the modified one with
+     * subsequent requests (the new backend may not be ping-able).
      */
+    expect = apr_table_get(r->notes, "proxy-ping-expect");
+    if (expect == NULL) {
+        expect = apr_table_get(r->headers_in, "Expect");
+        if (expect != NULL) {
+            apr_table_setn(r->notes, "proxy-ping-expect", expect);
+        }
+        else {
+            apr_table_setn(r->notes, "proxy-ping-expect", "\n");
+        }
+    }
+    else if (strcmp(expect, "\n") != 0) {
+        apr_table_setn(r->headers_in, "Expect", expect);
+    }
+    else {
+        apr_table_unset(r->headers_in, "Expect");
+        expect = NULL;
+    }
     if (do_100_continue) {
-        apr_table_mergen(r->headers_in, "Expect", "100-Continue");
-        r->expecting_100 = 1;
+        if (!ap_find_token(r->pool, expect, "100-Continue")) {
+            apr_table_mergen(r->headers_in, "Expect", "100-Continue");
+        }
+        apr_table_setn(r->notes, "proxy-ping-100-continue", "1");
     }
+    else {
+        apr_table_unset(r->notes, "proxy-ping-100-continue");
+    }
 
     /* X-Forwarded-*: handling
      *
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c	(revision 1541907)
+++ modules/proxy/mod_proxy_http.c	(working copy)
@@ -1537,10 +1537,12 @@ int ap_proxy_http_process_response(apr_pool_t * p,
              * We need to set "r->expecting_100 = 1" otherwise origin
              * server behaviour will apply.
              */
+            ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
+                          "HTTP: received interim %d response", r->status);
+            if ((r->status != HTTP_CONTINUE) ||
+                    !apr_table_get(r->notes, "proxy-ping-100-continue")) {
             const char *policy = apr_table_get(r->subprocess_env,
                                                "proxy-interim-response");
-            ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
-                          "HTTP: received interim %d response", r->status);
             if (!policy
                     || (!strcasecmp(policy, "RFC") && ((r->expecting_100 = 1)))) {
                 ap_send_interim_response(r, 1);
@@ -1552,6 +1554,7 @@ int ap_proxy_http_process_response(apr_pool_t * p,
                 ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01108)
                               "undefined proxy interim response policy");
             }
+            }
         }
         /* Moved the fixups of Date headers and those affected by
          * ProxyPassReverse/etc from here to ap_proxy_read_headers


--------------030104010303070707030700--