You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by yl...@apache.org on 2020/12/10 16:04:35 UTC
svn commit: r1884280 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy_ftp.c modules/proxy/mod_proxy_http.c
modules/proxy/proxy_util.c
Author: ylavic
Date: Thu Dec 10 16:04:34 2020
New Revision: 1884280
URL: http://svn.apache.org/viewvc?rev=1884280&view=rev
Log:
Revert r1480058, -1'ed on dev@ and STATUS.
Never backported (and never will supposedly), while often creating
merge conflicts.
See https://lists.apache.org/thread.html/be0e7bdc3510fddd2dd80accece44917eba361ef4fcc713dd0f7f7fa%401367999236%40%3Cdev.httpd.apache.org%3E
and https://lists.apache.org/thread.html/6e63271b308a2723285d288857318e7bb51b6756690514d9bc75a71b%401371148914%40%3Ccvs.httpd.apache.org%3E
Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c
httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
httpd/httpd/trunk/modules/proxy/proxy_util.c
Modified: httpd/httpd/trunk/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1884280&r1=1884279&r2=1884280&view=diff
==============================================================================
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Thu Dec 10 16:04:34 2020
@@ -981,10 +981,6 @@ Changes with Apache 2.5.0-alpha
clean mapping from APR codes to HTTP status codes, and use it where
needed. [Graham Leggett]
- *) mod_proxy: Ensure network errors detected by the proxy are returned as
- 504 Gateway Timeout as opposed to 502 Bad Gateway, in order to be
- compliant with RFC2616 14.9.4 Cache Revalidation and Reload Controls.
-
*) mod_dav: mod_dav overrides dav_fs response on PUT failure. PR 35981
[Basant Kumar Kukreja <basant.kukreja sun.com>, Alejandro Alvarez
<alejandro.alvarez.ayllon cern.ch>]
Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c?rev=1884280&r1=1884279&r2=1884280&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c (original)
+++ httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c Thu Dec 10 16:04:34 2020
@@ -869,11 +869,7 @@ static int ftp_set_TYPE(char xfer_type,
/* 501 Syntax error in parameters or arguments. */
/* 504 Command not implemented for that parameter. */
/* 530 Not logged in. */
- if (rc == -1) {
- ret = ap_proxyerror(r, HTTP_GATEWAY_TIME_OUT,
- "Error reading from remote server");
- }
- else if (rc == 421) {
+ if (rc == -1 || rc == 421) {
ret = ap_proxyerror(r, HTTP_BAD_GATEWAY,
"Error reading from remote server");
}
@@ -905,10 +901,6 @@ static char *ftp_get_PWD(request_rec *r,
/* 550 Requested action not taken. */
switch (proxy_ftp_command("PWD" CRLF, r, ftp_ctrl, bb, &ftpmessage)) {
case -1:
- ap_proxyerror(r, HTTP_GATEWAY_TIME_OUT,
- "Failed to read PWD on ftp server");
- break;
-
case 421:
case 550:
ap_proxyerror(r, HTTP_BAD_GATEWAY,
@@ -1164,7 +1156,7 @@ static int proxy_ftp_handler(request_rec
* them until we get a successful connection
*/
if (APR_SUCCESS != err) {
- return ap_proxyerror(r, HTTP_GATEWAY_TIME_OUT, apr_pstrcat(p,
+ return ap_proxyerror(r, HTTP_BAD_GATEWAY, apr_pstrcat(p,
"DNS lookup failure for: ",
connectname, NULL));
}
@@ -1235,15 +1227,10 @@ static int proxy_ftp_handler(request_rec
/* 220 Service ready for new user. */
/* 421 Service not available, closing control connection. */
rc = proxy_ftp_command(NULL, r, origin, bb, &ftpmessage);
- if (rc == -1) {
- return ftp_proxyerror(r, backend, HTTP_GATEWAY_TIME_OUT,
- "Error reading from remote server");
- }
- else if (rc == 421) {
- return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY,
- "Error reading from remote server");
+ if (rc == -1 || rc == 421) {
+ return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY, "Error reading from remote server");
}
- else if (rc == 120) {
+ if (rc == 120) {
/*
* RFC2616 states: 14.37 Retry-After
*
@@ -1269,7 +1256,7 @@ static int proxy_ftp_handler(request_rec
}
return ftp_proxyerror(r, backend, HTTP_SERVICE_UNAVAILABLE, ftpmessage);
}
- else if (rc != 220) {
+ if (rc != 220) {
return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY, ftpmessage);
}
@@ -1285,20 +1272,15 @@ static int proxy_ftp_handler(request_rec
/* (This may include errors such as command line too long.) */
/* 501 Syntax error in parameters or arguments. */
/* 530 Not logged in. */
- if (rc == -1) {
- return ftp_proxyerror(r, backend, HTTP_GATEWAY_TIME_OUT,
- "Error reading from remote server");
- }
- else if (rc == 421) {
- return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY,
- "Error reading from remote server");
+ if (rc == -1 || rc == 421) {
+ return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY, "Error reading from remote server");
}
- else if (rc == 530) {
+ if (rc == 530) {
proxy_ftp_cleanup(r, backend);
return ftp_unauthorized(r, 1); /* log it: user name guessing
* attempt? */
}
- else if (rc != 230 && rc != 331) {
+ if (rc != 230 && rc != 331) {
return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY, ftpmessage);
}
@@ -1318,25 +1300,21 @@ static int proxy_ftp_handler(request_rec
/* 501 Syntax error in parameters or arguments. */
/* 503 Bad sequence of commands. */
/* 530 Not logged in. */
- if (rc == -1) {
- return ftp_proxyerror(r, backend, HTTP_GATEWAY_TIME_OUT,
- "Error reading from remote server");
- }
- else if (rc == 421) {
+ if (rc == -1 || rc == 421) {
return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY,
"Error reading from remote server");
}
- else if (rc == 332) {
+ if (rc == 332) {
return ftp_proxyerror(r, backend, HTTP_UNAUTHORIZED,
apr_pstrcat(p, "Need account for login: ", ftpmessage, NULL));
}
/* @@@ questionable -- we might as well return a 403 Forbidden here */
- else if (rc == 530) {
+ if (rc == 530) {
proxy_ftp_cleanup(r, backend);
return ftp_unauthorized(r, 1); /* log it: passwd guessing
* attempt? */
}
- else if (rc != 230 && rc != 202) {
+ if (rc != 230 && rc != 202) {
return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY, ftpmessage);
}
}
@@ -1352,14 +1330,9 @@ static int proxy_ftp_handler(request_rec
++path;
rc = proxy_ftp_command("CWD /" CRLF, r, origin, bb, &ftpmessage);
- if (rc == -1) {
- return ftp_proxyerror(r, backend, HTTP_GATEWAY_TIME_OUT,
- "Error reading from remote server");
- }
- else if (rc == 421) {
+ if (rc == -1 || rc == 421)
return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY,
- "Error reading from remote server");
- }
+ "Error reading from remote server");
}
/*
@@ -1396,18 +1369,14 @@ static int proxy_ftp_handler(request_rec
/* 502 Command not implemented. */
/* 530 Not logged in. */
/* 550 Requested action not taken. */
- if (rc == -1) {
- return ftp_proxyerror(r, backend, HTTP_GATEWAY_TIME_OUT,
- "Error reading from remote server");
- }
- else if (rc == 421) {
+ if (rc == -1 || rc == 421) {
return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY,
- "Error reading from remote server");
+ "Error reading from remote server");
}
- else if (rc == 550) {
+ if (rc == 550) {
return ftp_proxyerror(r, backend, HTTP_NOT_FOUND, ftpmessage);
}
- else if (rc != 250) {
+ if (rc != 250) {
return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY, ftpmessage);
}
@@ -1441,15 +1410,11 @@ static int proxy_ftp_handler(request_rec
/* 501 Syntax error in parameters or arguments. */
/* 502 Command not implemented. */
/* 530 Not logged in. */
- if (rc == -1) {
- return ftp_proxyerror(r, backend, HTTP_GATEWAY_TIME_OUT,
- "Error reading from remote server");
- }
- else if (rc == 421) {
+ if (rc == -1 || rc == 421) {
return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY,
- "Error reading from remote server");
+ "Error reading from remote server");
}
- else if (rc != 229 && rc != 500 && rc != 501 && rc != 502) {
+ if (rc != 229 && rc != 500 && rc != 501 && rc != 502) {
return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY, ftpmessage);
}
else if (rc == 229) {
@@ -1511,10 +1476,8 @@ static int proxy_ftp_handler(request_rec
ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01043)
"EPSV attempt to connect to %pI failed - "
"Firewall/NAT?", &epsv_addr);
- return ftp_proxyerror(r, backend, HTTP_GATEWAY_TIME_OUT,
- apr_psprintf(r->pool,
- "EPSV attempt to connect to %pI failed - firewall/NAT?",
- &epsv_addr));
+ return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY, apr_psprintf(r->pool,
+ "EPSV attempt to connect to %pI failed - firewall/NAT?", &epsv_addr));
}
else {
ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r,
@@ -1536,15 +1499,11 @@ static int proxy_ftp_handler(request_rec
/* 501 Syntax error in parameters or arguments. */
/* 502 Command not implemented. */
/* 530 Not logged in. */
- if (rc == -1) {
- return ftp_proxyerror(r, backend, HTTP_GATEWAY_TIME_OUT,
- "Error reading from remote server");
- }
- else if (rc == 421) {
+ if (rc == -1 || rc == 421) {
return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY,
- "Error reading from remote server");
+ "Error reading from remote server");
}
- else if (rc != 227 && rc != 502) {
+ if (rc != 227 && rc != 502) {
return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY, ftpmessage);
}
else if (rc == 227) {
@@ -1606,10 +1565,8 @@ static int proxy_ftp_handler(request_rec
if (rv != APR_SUCCESS) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01048)
"PASV attempt to connect to %pI failed - Firewall/NAT?", pasv_addr);
- return ftp_proxyerror(r, backend, HTTP_GATEWAY_TIME_OUT,
- apr_psprintf(r->pool,
- "PASV attempt to connect to %pI failed - firewall/NAT?",
- pasv_addr));
+ return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY, apr_psprintf(r->pool,
+ "PASV attempt to connect to %pI failed - firewall/NAT?", pasv_addr));
}
else {
connect = 1;
@@ -1679,15 +1636,11 @@ static int proxy_ftp_handler(request_rec
/* 501 Syntax error in parameters or arguments. */
/* 502 Command not implemented. */
/* 530 Not logged in. */
- if (rc == -1) {
- return ftp_proxyerror(r, backend, HTTP_GATEWAY_TIME_OUT,
- "Error reading from remote server");
- }
- else if (rc == 421) {
+ if (rc == -1 || rc == 421) {
return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY,
- "Error reading from remote server");
+ "Error reading from remote server");
}
- else if (rc != 200) {
+ if (rc != 200) {
return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY, ftpmessage);
}
@@ -1748,13 +1701,9 @@ static int proxy_ftp_handler(request_rec
rc = proxy_ftp_command(apr_pstrcat(p, "SIZE ",
ftp_escape_globbingchars(p, path, fdconf), CRLF, NULL),
r, origin, bb, &ftpmessage);
- if (rc == -1) {
- return ftp_proxyerror(r, backend, HTTP_GATEWAY_TIME_OUT,
- "Error reading from remote server");
- }
- else if (rc == 421) {
+ if (rc == -1 || rc == 421) {
return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY,
- "Error reading from remote server");
+ "Error reading from remote server");
}
else if (rc == 213) {/* Size command ok */
int j;
@@ -1779,18 +1728,14 @@ static int proxy_ftp_handler(request_rec
/* 502 Command not implemented. */
/* 530 Not logged in. */
/* 550 Requested action not taken. */
- if (rc == -1) {
- return ftp_proxyerror(r, backend, HTTP_GATEWAY_TIME_OUT,
- "Error reading from remote server");
- }
- else if (rc == 421) {
+ if (rc == -1 || rc == 421) {
return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY,
- "Error reading from remote server");
+ "Error reading from remote server");
}
- else if (rc == 550) {
+ if (rc == 550) {
return ftp_proxyerror(r, backend, HTTP_NOT_FOUND, ftpmessage);
}
- else if (rc != 250) {
+ if (rc != 250) {
return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY, ftpmessage);
}
path = "";
@@ -1897,15 +1842,11 @@ static int proxy_ftp_handler(request_rec
/* 501 Syntax error in parameters or arguments. */
/* 530 Not logged in. */
/* 550 Requested action not taken. */
- if (rc == -1) {
- return ftp_proxyerror(r, backend, HTTP_GATEWAY_TIME_OUT,
- "Error reading from remote server");
- }
- else if (rc == 421) {
+ if (rc == -1 || rc == 421) {
return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY,
"Error reading from remote server");
}
- else if (rc == 550) {
+ if (rc == 550) {
ap_log_rerror(APLOG_MARK, APLOG_TRACE4, 0, r,
"RETR failed, trying LIST instead");
@@ -1924,18 +1865,14 @@ static int proxy_ftp_handler(request_rec
/* 502 Command not implemented. */
/* 530 Not logged in. */
/* 550 Requested action not taken. */
- if (rc == -1) {
- return ftp_proxyerror(r, backend, HTTP_GATEWAY_TIME_OUT,
- "Error reading from remote server");
- }
- else if (rc == 421) {
+ if (rc == -1 || rc == 421) {
return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY,
- "Error reading from remote server");
+ "Error reading from remote server");
}
- else if (rc == 550) {
+ if (rc == 550) {
return ftp_proxyerror(r, backend, HTTP_NOT_FOUND, ftpmessage);
}
- else if (rc != 250) {
+ if (rc != 250) {
return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY, ftpmessage);
}
@@ -1951,14 +1888,9 @@ static int proxy_ftp_handler(request_rec
r, origin, bb, &ftpmessage);
/* rc is an intermediate response for the LIST command (125 transfer starting, 150 opening data connection) */
- if (rc == -1) {
- return ftp_proxyerror(r, backend, HTTP_GATEWAY_TIME_OUT,
- "Error reading from remote server");
- }
- else if (rc == 421) {
+ if (rc == -1 || rc == 421)
return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY,
- "Error reading from remote server");
- }
+ "Error reading from remote server");
}
if (rc != 125 && rc != 150 && rc != 226 && rc != 250) {
return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY, ftpmessage);
@@ -2030,7 +1962,7 @@ static int proxy_ftp_handler(request_rec
ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01053)
"failed to accept data connection");
proxy_ftp_cleanup(r, backend);
- return HTTP_GATEWAY_TIME_OUT;
+ return HTTP_BAD_GATEWAY;
}
}
}
Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_http.c?rev=1884280&r1=1884279&r2=1884280&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/proxy/mod_proxy_http.c (original)
+++ httpd/httpd/trunk/modules/proxy/mod_proxy_http.c Thu Dec 10 16:04:34 2020
@@ -1136,7 +1136,7 @@ int ap_proxy_http_process_response(proxy
" Number of keepalives %i", backend->hostname,
backend->port, c->keepalives);
- e = ap_bucket_error_create(HTTP_GATEWAY_TIME_OUT, NULL,
+ e = ap_bucket_error_create(HTTP_BAD_GATEWAY, NULL,
r->pool, c->bucket_alloc);
APR_BRIGADE_INSERT_TAIL(bb, e);
e = ap_bucket_eoc_create(c->bucket_alloc);
@@ -1154,7 +1154,7 @@ int ap_proxy_http_process_response(proxy
" failed.",
backend->hostname, backend->port);
}
- return ap_proxyerror(r, HTTP_GATEWAY_TIME_OUT,
+ return ap_proxyerror(r, HTTP_BAD_GATEWAY,
"Error reading from remote server");
}
/* XXX: Is this a real headers length send from remote? */
@@ -1686,7 +1686,7 @@ int ap_proxy_http_process_response(proxy
* disconnect the client too.
*/
apr_brigade_cleanup(bb);
- e = ap_bucket_error_create(HTTP_GATEWAY_TIME_OUT, NULL,
+ e = ap_bucket_error_create(HTTP_BAD_GATEWAY, NULL,
r->pool, c->bucket_alloc);
APR_BRIGADE_INSERT_TAIL(bb, e);
e = ap_bucket_eoc_create(c->bucket_alloc);
Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1884280&r1=1884279&r2=1884280&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
+++ httpd/httpd/trunk/modules/proxy/proxy_util.c Thu Dec 10 16:04:34 2020
@@ -2639,7 +2639,7 @@ ap_proxy_determine_connection(apr_pool_t
}
if (err != APR_SUCCESS) {
- return ap_proxyerror(r, HTTP_GATEWAY_TIME_OUT,
+ return ap_proxyerror(r, HTTP_BAD_GATEWAY,
apr_pstrcat(p, "DNS lookup failure for: ",
conn->hostname, NULL));
}
@@ -3461,7 +3461,7 @@ PROXY_DECLARE(void) ap_proxy_backend_bro
*/
if (r->main)
r->main->no_cache = 1;
- e = ap_bucket_error_create(HTTP_GATEWAY_TIME_OUT, NULL, c->pool,
+ e = ap_bucket_error_create(HTTP_BAD_GATEWAY, NULL, c->pool,
c->bucket_alloc);
APR_BRIGADE_INSERT_TAIL(brigade, e);
e = apr_bucket_eos_create(c->bucket_alloc);
@@ -4294,7 +4294,8 @@ PROXY_DECLARE(int) ap_proxy_pass_brigade
"Error during SSL Handshake with"
" remote server");
}
- return HTTP_GATEWAY_TIME_OUT;
+ return APR_STATUS_IS_TIMEUP(status) ? HTTP_GATEWAY_TIME_OUT
+ : HTTP_BAD_GATEWAY;
}
else {
return HTTP_BAD_REQUEST;
Re: svn commit: r1884280 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy_ftp.c modules/proxy/mod_proxy_http.c
modules/proxy/proxy_util.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 12/12/20 12:59 AM, Roy T. Fielding wrote:
>> On Dec 11, 2020, at 6:28 AM, Ruediger Pluem <rpluem@apache.org <ma...@apache.org>> wrote:
>> On 12/11/20 1:13 PM, Yann Ylavic wrote:
>>> On Fri, Dec 11, 2020 at 12:49 PM Graham Leggett <minfrin@sharp.fm <ma...@sharp.fm>> wrote:
>>>>
>>>> On 10 Dec 2020, at 18:04, ylavic@apache.org <ma...@apache.org> wrote:
>>
>> Old RFC2616 SectionNew RFC and sectionLink
>> 14.9.4RFC7234, 5.2.2.1https://tools.ietf.org/html/rfc7234#section-5.2.2
>> 10.5.3RFC7231, 6.6.3https://tools.ietf.org/html/rfc7231#section-6.6.3
>> 10.5.5RFC7231, 6.6.5https://tools.ietf.org/html/rfc7231#section-6.6.5
>
> Heh, sorry, the current version is
>
> https://httpwg.org/http-core/draft-ietf-httpbis-cache-latest.html#cache-response-directive.must-revalidate
> https://httpwg.org/http-core/draft-ietf-httpbis-cache-latest.html#rfc.section.4.3.3
>
> https://httpwg.org/http-core/draft-ietf-httpbis-semantics-latest.html#status.502
>
> and we have until Monday to fix it, if needed.
>
>> After rereading the sections on the new RFC's my opinion is still the same as in
>>
>> https://lists.apache.org/thread.html/be0e7bdc3510fddd2dd80accece44917eba361ef4fcc713dd0f7f7fa%401367999236%40%3Cdev.httpd.apache.org%3E
>> <https://lists.apache.org/thread.html/be0e7bdc3510fddd2dd80accece44917eba361ef4fcc713dd0f7f7fa@1367999236@<dev.httpd.apache.org>>
>>
>> IMHO RFC7231, 6.6.3 and RFC7231, 6.6.5 apply for the proxy/gateway. In the revalidate case (RFC7234, 5.2.2.1) the issue for the
>> cache may be even wider: What if the reply on the revalidation request from the backend is not cachable for whatever reason (like
>> the 502 here without appropriate headers that allow caching)?
>> Does the cache just pass the reply on and does not update its cache? Does it remove the stale entry from the cache?
>>
>> Apart from this the proxy could add a note to r->notes if there was a network issue with the backend such that the cache can reply
>> with 504 in this case. But of course this only helps in the case that the next hop was not reachable. If the 502 is created by a
>> further gateway between us and the origin server the issue remains.
>
> That is too many questions. The purpose of the cache requirement is so that the cache
> does not deliver a non-validated entry after receiving a failed validation. It doesn't really
> matter what code is returned so long as it is 5xx, so the specs are over-constraining here.
>
> The CoAdvisor test suite is overly pedantic.
>
> And, as stated, this only applies when the request contains max-revalidate and the
> action being done is a cache revalidation. No other code should behave this way.
>
>>> You should do that, it's not my veto. Failing to resolve the
>>> discussion, the commit should be reverted right?
>>>
>>>>
>>>> The last on the thread is that Roy was asked for advice, but he was busy. In the light of RFC7230 it would be useful to get a
>>>> new answer on this question.
>>>
>>> Sure.
>>
>> Can you please help us resolving this Roy?
>
> Reverting the change is the correct call, regardless, but it is also the right choice.
> I have filed a bug on the Cache spec to change that MUST send 504 to a MUST send 5xx.
>
> https://github.com/httpwg/http-core/issues/608
>
Thanks for the clarification and for creating the issue to the spec.
Regards
Rüdiger
Re: svn commit: r1884280 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy_ftp.c modules/proxy/mod_proxy_http.c
modules/proxy/proxy_util.c
Posted by "Roy T. Fielding" <fi...@gbiv.com>.
> On Dec 16, 2020, at 2:52 AM, Graham Leggett <mi...@sharp.fm> wrote:
>
> On 12 Dec 2020, at 01:59, Roy T. Fielding <fielding@gbiv.com <ma...@gbiv.com>> wrote:
>
>> That is too many questions. The purpose of the cache requirement is so that the cache
>> does not deliver a non-validated entry after receiving a failed validation. It doesn't really
>> matter what code is returned so long as it is 5xx, so the specs are over-constraining here.
>>
>> The CoAdvisor test suite is overly pedantic.
>>
>> And, as stated, this only applies when the request contains max-revalidate and the
>> action being done is a cache revalidation. No other code should behave this way.
>
> To clarify, the change under discussion covers behaviour when a proxy of any kind (including an ftp proxy) suffers a low level network error of any kind, from DNS errors to low level network errors.
Yes, and as I said, that is wrong.
> Whether ultimately correct or not, CoAdvisor was very specific as to the codes to be returned in these cases.
I am very happy for their financial success. However, they are frequently
wrong, even though they are supposed to be based on the standard.
>> Reverting the change is the correct call, regardless, but it is also the right choice.
>> I have filed a bug on the Cache spec to change that MUST send 504 to a MUST send 5xx.
>>
>> https://github.com/httpwg/http-core/issues/608 <https://github.com/httpwg/http-core/issues/608>
>>
>> If you think we need to change other things in the specs, please file bugs now.
>
> The above issue relates to must-revalidate only, while the wider issue is for which 5xx error to be returned in which situation. Can you clarify?
>
> The patch in this thread covers each case, and basically boils down to a choice between “Bad Gateway” or “Timed Out”.
The patch changed BAD_GATEWAY to Timed Out for no reason whatsoever.
The spec text that you pointed to is about cache and cache control when
the cache is disconnected from the network. The spec says that the cache
must generate a 504 when it is disconnected, meaning it has NO response
other than the entry in its cache. I fully understand why CoAdvisor is
confused and unable to properly test for that condition, given that it isn't
an internal cache testing suite, but this has nothing to do with a gateway
receiving or generating a 5xx code upon gateway fail. And, in any case,
the requirement in the spec is overly constraining when it is supposed to
be minimally preventing the cache from reusing its cached response.
IOW, gateway != cache.
> Given the long list here http://coad.measurement-factory.com/clients.html <http://coad.measurement-factory.com/clients.html> I would be keen to make sure httpd worked the same way as all those other servers.
That is not an interop concern.
....Roy
Re: svn commit: r1884280 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy_ftp.c modules/proxy/mod_proxy_http.c
modules/proxy/proxy_util.c
Posted by Graham Leggett <mi...@sharp.fm>.
On 12 Dec 2020, at 01:59, Roy T. Fielding <fi...@gbiv.com> wrote:
> That is too many questions. The purpose of the cache requirement is so that the cache
> does not deliver a non-validated entry after receiving a failed validation. It doesn't really
> matter what code is returned so long as it is 5xx, so the specs are over-constraining here.
>
> The CoAdvisor test suite is overly pedantic.
>
> And, as stated, this only applies when the request contains max-revalidate and the
> action being done is a cache revalidation. No other code should behave this way.
To clarify, the change under discussion covers behaviour when a proxy of any kind (including an ftp proxy) suffers a low level network error of any kind, from DNS errors to low level network errors.
Whether ultimately correct or not, CoAdvisor was very specific as to the codes to be returned in these cases.
> Reverting the change is the correct call, regardless, but it is also the right choice.
> I have filed a bug on the Cache spec to change that MUST send 504 to a MUST send 5xx.
>
> https://github.com/httpwg/http-core/issues/608 <https://github.com/httpwg/http-core/issues/608>
>
> If you think we need to change other things in the specs, please file bugs now.
The above issue relates to must-revalidate only, while the wider issue is for which 5xx error to be returned in which situation. Can you clarify?
The patch in this thread covers each case, and basically boils down to a choice between “Bad Gateway” or “Timed Out”.
Given the long list here http://coad.measurement-factory.com/clients.html I would be keen to make sure httpd worked the same way as all those other servers.
Regards,
Graham
—
Re: svn commit: r1884280 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy_ftp.c modules/proxy/mod_proxy_http.c
modules/proxy/proxy_util.c
Posted by "Roy T. Fielding" <fi...@gbiv.com>.
> On Dec 11, 2020, at 6:28 AM, Ruediger Pluem <rp...@apache.org> wrote:
> On 12/11/20 1:13 PM, Yann Ylavic wrote:
>> On Fri, Dec 11, 2020 at 12:49 PM Graham Leggett <mi...@sharp.fm> wrote:
>>>
>>> On 10 Dec 2020, at 18:04, ylavic@apache.org wrote:
>>>
>>> Author: ylavic
>>> Date: Thu Dec 10 16:04:34 2020
>>> New Revision: 1884280
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1884280&view=rev
>>> Log:
>>> Revert r1480058, -1'ed on dev@ and STATUS.
>>>
>>> Never backported (and never will supposedly), while often creating
>>> merge conflicts.
>>>
>>> You’ve just reverted a fix to an RFC violation that was picked up by the CoAdvisor test suite.
>>
>> Where is this test suite?
>> Which RFC violation, a proxy socket connection error should return 504
>> Gateway Timeout??
>> I see that RFC2616 14.9.4 is about cache, why don't you fix this in mod_cache?
>>
>>>
>>> “Creating merge conflicts” is not a sufficient technical justification for a veto that results in the re-introduction of an RFC violation.
>>>
>>> Please back this out.
>>>
>>> See https://lists.apache.org/thread.html/be0e7bdc3510fddd2dd80accece44917eba361ef4fcc713dd0f7f7fa%401367999236%40%3Cdev.httpd.apache.org%3E
>>> and https://lists.apache.org/thread.html/6e63271b308a2723285d288857318e7bb51b6756690514d9bc75a71b%401371148914%40%3Ccvs.httpd.apache.org%3E
>>>
>>> Please resolve the discussion above.
>
> Just as an update for restarting the discussion:
>
> Old RFC2616 Section New RFC and section Link
> 14.9.4 RFC7234, 5.2.2.1 https://tools.ietf.org/html/rfc7234#section-5.2.2 <https://tools.ietf.org/html/rfc7234#section-5.2.2>
> 10.5.3 RFC7231, 6.6.3 https://tools.ietf.org/html/rfc7231#section-6.6.3 <https://tools.ietf.org/html/rfc7231#section-6.6.3>
> 10.5.5 RFC7231, 6.6.5 https://tools.ietf.org/html/rfc7231#section-6.6.5 <https://tools.ietf.org/html/rfc7231#section-6.6.5>
Heh, sorry, the current version is
https://httpwg.org/http-core/draft-ietf-httpbis-cache-latest.html#cache-response-directive.must-revalidate <https://httpwg.org/http-core/draft-ietf-httpbis-cache-latest.html#cache-response-directive.must-revalidate>
https://httpwg.org/http-core/draft-ietf-httpbis-cache-latest.html#rfc.section.4.3.3 <https://httpwg.org/http-core/draft-ietf-httpbis-cache-latest.html#rfc.section.4.3.3>
https://httpwg.org/http-core/draft-ietf-httpbis-semantics-latest.html#status.502 <https://httpwg.org/http-core/draft-ietf-httpbis-semantics-latest.html#status.502>
and we have until Monday to fix it, if needed.
> After rereading the sections on the new RFC's my opinion is still the same as in
>
> https://lists.apache.org/thread.html/be0e7bdc3510fddd2dd80accece44917eba361ef4fcc713dd0f7f7fa%401367999236%40%3Cdev.httpd.apache.org%3E <https://lists.apache.org/thread.html/be0e7bdc3510fddd2dd80accece44917eba361ef4fcc713dd0f7f7fa@1367999236@%3Cdev.httpd.apache.org%3E>
>
> IMHO RFC7231, 6.6.3 and RFC7231, 6.6.5 apply for the proxy/gateway. In the revalidate case (RFC7234, 5.2.2.1) the issue for the
> cache may be even wider: What if the reply on the revalidation request from the backend is not cachable for whatever reason (like
> the 502 here without appropriate headers that allow caching)?
> Does the cache just pass the reply on and does not update its cache? Does it remove the stale entry from the cache?
>
> Apart from this the proxy could add a note to r->notes if there was a network issue with the backend such that the cache can reply
> with 504 in this case. But of course this only helps in the case that the next hop was not reachable. If the 502 is created by a
> further gateway between us and the origin server the issue remains.
That is too many questions. The purpose of the cache requirement is so that the cache
does not deliver a non-validated entry after receiving a failed validation. It doesn't really
matter what code is returned so long as it is 5xx, so the specs are over-constraining here.
The CoAdvisor test suite is overly pedantic.
And, as stated, this only applies when the request contains max-revalidate and the
action being done is a cache revalidation. No other code should behave this way.
>> You should do that, it's not my veto. Failing to resolve the
>> discussion, the commit should be reverted right?
>>
>>>
>>> The last on the thread is that Roy was asked for advice, but he was busy. In the light of RFC7230 it would be useful to get a new answer on this question.
>>
>> Sure.
>
> Can you please help us resolving this Roy?
Reverting the change is the correct call, regardless, but it is also the right choice.
I have filed a bug on the Cache spec to change that MUST send 504 to a MUST send 5xx.
https://github.com/httpwg/http-core/issues/608 <https://github.com/httpwg/http-core/issues/608>
If you think we need to change other things in the specs, please file bugs now.
....Roy
Re: svn commit: r1884280 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy_ftp.c modules/proxy/mod_proxy_http.c
modules/proxy/proxy_util.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 12/11/20 1:13 PM, Yann Ylavic wrote:
> On Fri, Dec 11, 2020 at 12:49 PM Graham Leggett <mi...@sharp.fm> wrote:
>>
>> On 10 Dec 2020, at 18:04, ylavic@apache.org wrote:
>>
>> Author: ylavic
>> Date: Thu Dec 10 16:04:34 2020
>> New Revision: 1884280
>>
>> URL: http://svn.apache.org/viewvc?rev=1884280&view=rev
>> Log:
>> Revert r1480058, -1'ed on dev@ and STATUS.
>>
>> Never backported (and never will supposedly), while often creating
>> merge conflicts.
>>
>> You’ve just reverted a fix to an RFC violation that was picked up by the CoAdvisor test suite.
>
> Where is this test suite?
> Which RFC violation, a proxy socket connection error should return 504
> Gateway Timeout??
> I see that RFC2616 14.9.4 is about cache, why don't you fix this in mod_cache?
>
>>
>> “Creating merge conflicts” is not a sufficient technical justification for a veto that results in the re-introduction of an RFC violation.
>>
>> Please back this out.
>>
>> See https://lists.apache.org/thread.html/be0e7bdc3510fddd2dd80accece44917eba361ef4fcc713dd0f7f7fa%401367999236%40%3Cdev.httpd.apache.org%3E
>> and https://lists.apache.org/thread.html/6e63271b308a2723285d288857318e7bb51b6756690514d9bc75a71b%401371148914%40%3Ccvs.httpd.apache.org%3E
>>
>> Please resolve the discussion above.
Just as an update for restarting the discussion:
Old RFC2616 Section New RFC and section Link
14.9.4 RFC7234, 5.2.2.1 https://tools.ietf.org/html/rfc7234#section-5.2.2
10.5.3 RFC7231, 6.6.3 https://tools.ietf.org/html/rfc7231#section-6.6.3
10.5.5 RFC7231, 6.6.5 https://tools.ietf.org/html/rfc7231#section-6.6.5
After rereading the sections on the new RFC's my opinion is still the same as in
https://lists.apache.org/thread.html/be0e7bdc3510fddd2dd80accece44917eba361ef4fcc713dd0f7f7fa%401367999236%40%3Cdev.httpd.apache.org%3E
IMHO RFC7231, 6.6.3 and RFC7231, 6.6.5 apply for the proxy/gateway. In the revalidate case (RFC7234, 5.2.2.1) the issue for the
cache may be even wider: What if the reply on the revalidation request from the backend is not cachable for whatever reason (like
the 502 here without appropriate headers that allow caching)?
Does the cache just pass the reply on and does not update its cache? Does it remove the stale entry from the cache?
Apart from this the proxy could add a note to r->notes if there was a network issue with the backend such that the cache can reply
with 504 in this case. But of course this only helps in the case that the next hop was not reachable. If the 502 is created by a
further gateway between us and the origin server the issue remains.
>
> You should do that, it's not my veto. Failing to resolve the
> discussion, the commit should be reverted right?
>
>>
>> The last on the thread is that Roy was asked for advice, but he was busy. In the light of RFC7230 it would be useful to get a new answer on this question.
>
> Sure.
Can you please help us resolving this Roy?
Regards
Rüdiger
Re: svn commit: r1884280 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy_ftp.c modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Dec 11, 2020 at 1:13 PM Yann Ylavic <yl...@gmail.com> wrote:
>
> I see that RFC2616 14.9.4 is about cache, why don't you fix this in mod_cache?
If you need a hint from mod_proxy (when it failed to forward the
request), you could (for instance) add a r->notes for mod_cache to do
the right thing.
But I don't see why a connection aborted by the backend qualifies as
Gateway Timeout.
Regards;
Yann.
Re: svn commit: r1884280 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy_ftp.c modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Dec 11, 2020 at 12:49 PM Graham Leggett <mi...@sharp.fm> wrote:
>
> On 10 Dec 2020, at 18:04, ylavic@apache.org wrote:
>
> Author: ylavic
> Date: Thu Dec 10 16:04:34 2020
> New Revision: 1884280
>
> URL: http://svn.apache.org/viewvc?rev=1884280&view=rev
> Log:
> Revert r1480058, -1'ed on dev@ and STATUS.
>
> Never backported (and never will supposedly), while often creating
> merge conflicts.
>
> You’ve just reverted a fix to an RFC violation that was picked up by the CoAdvisor test suite.
Where is this test suite?
Which RFC violation, a proxy socket connection error should return 504
Gateway Timeout??
I see that RFC2616 14.9.4 is about cache, why don't you fix this in mod_cache?
>
> “Creating merge conflicts” is not a sufficient technical justification for a veto that results in the re-introduction of an RFC violation.
>
> Please back this out.
>
> See https://lists.apache.org/thread.html/be0e7bdc3510fddd2dd80accece44917eba361ef4fcc713dd0f7f7fa%401367999236%40%3Cdev.httpd.apache.org%3E
> and https://lists.apache.org/thread.html/6e63271b308a2723285d288857318e7bb51b6756690514d9bc75a71b%401371148914%40%3Ccvs.httpd.apache.org%3E
>
> Please resolve the discussion above.
You should do that, it's not my veto. Failing to resolve the
discussion, the commit should be reverted right?
>
> The last on the thread is that Roy was asked for advice, but he was busy. In the light of RFC7230 it would be useful to get a new answer on this question.
Sure.
Regards;
Yann.
Re: svn commit: r1884280 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy_ftp.c modules/proxy/mod_proxy_http.c
modules/proxy/proxy_util.c
Posted by Graham Leggett <mi...@sharp.fm>.
On 10 Dec 2020, at 18:04, ylavic@apache.org wrote:
> Author: ylavic
> Date: Thu Dec 10 16:04:34 2020
> New Revision: 1884280
>
> URL: http://svn.apache.org/viewvc?rev=1884280&view=rev
> Log:
> Revert r1480058, -1'ed on dev@ and STATUS.
>
> Never backported (and never will supposedly), while often creating
> merge conflicts.
You’ve just reverted a fix to an RFC violation that was picked up by the CoAdvisor test suite.
“Creating merge conflicts” is not a sufficient technical justification for a veto that results in the re-introduction of an RFC violation.
Please back this out.
> See https://lists.apache.org/thread.html/be0e7bdc3510fddd2dd80accece44917eba361ef4fcc713dd0f7f7fa%401367999236%40%3Cdev.httpd.apache.org%3E
> and https://lists.apache.org/thread.html/6e63271b308a2723285d288857318e7bb51b6756690514d9bc75a71b%401371148914%40%3Ccvs.httpd.apache.org%3E <https://lists.apache.org/thread.html/6e63271b308a2723285d288857318e7bb51b6756690514d9bc75a71b@1371148914@%3Ccvs.httpd.apache.org%3E>
Please resolve the discussion above.
The last on the thread is that Roy was asked for advice, but he was busy. In the light of RFC7230 it would be useful to get a new answer on this question.
Regards,
Graham
—