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

2020-12-16 Thread Roy T. Fielding 
> On Dec 16, 2020, at 2:52 AM, Graham Leggett  wrote:
> 
> On 12 Dec 2020, at 01:59, Roy T. Fielding  > 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 
>> 
>> 
>> 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 
>  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

2020-12-16 Thread Graham Leggett 
On 12 Dec 2020, at 01:59, Roy T. Fielding  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 
> 
> 
> 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

2020-12-15 Thread Ruediger Pluem 



On 12/15/20 1:23 PM, Graham Leggett wrote:
> On 11 Dec 2020, at 14:13, Yann Ylavic  wrote:
> 
>> Where is this test suite?
> 
> To fill you in, the Co-Advisor test suite is a commercial HTTP suite 
> available here: http://coad.measurement-factory.com
> 
> A number of years ago they donated to our project one year access to their 
> suite for free, a service worth many thousands of dollars, and I used their 
> test suite within the time limit they gave us to take httpd from many 
> hundreds of protocol violations down to zero.
> 
> All violations were backported to v2.4 but this one, and as a result Apache 
> is not listed here: http://coad.measurement-factory.com/clients.html
> 
>> Which RFC violation, a proxy socket connection error should return 504
>> Gateway Timeout??
> 
> The RFC violation that was flagged by the test suite as described above.
> 
>> I see that RFC2616 14.9.4 is about cache, why don't you fix this in mod_cac=
>> he?
> 
> The fix applied consisted of the required changes to make the Co-Advisor 
> suite resolve the violation.
> 
>>> 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?
> 
> It should not be reverted, no.
> 
> The commit was not vetoed, the backport to 2.4 was, and for a good reason - a 
> change to the response code in a point release would have destabilised some 
> people. Fixing this issue on trunk for a future release is entirely fine.
> 
> The problem you’re really trying to solve is the inconvenience of having 
> trunk and v2.4 being different.
> 
> The fix to this is to replace HTTP_BAD_GATEWAY with a neural macro like 
> PROXY_TIMEOUT, and then #define PROXY_TIMEOUT to be HTTP_GATEWAY_TIME_OUT on 
> trunk, and HTTP_BAD_GATEWAY on v2.4.
> 
> Please don’t back out protocol behaviour without checking the origin of the 
> change first. All of what I describe above is in our commit history and 
> mailing lists.

Given the latest feedback from Roy and 
https://github.com/httpwg/http-core/issues/608 I think the way forward for this 
case here
is to leave it backed out as done in r1884280. Once 
https://github.com/httpwg/http-core/issues/608 is applied (I assume it gets
applied) Co-Advisor would need to adopt and we are fine.

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

2020-12-15 Thread Graham Leggett 
On 11 Dec 2020, at 14:13, Yann Ylavic  wrote:

> Where is this test suite?

To fill you in, the Co-Advisor test suite is a commercial HTTP suite available 
here: http://coad.measurement-factory.com

A number of years ago they donated to our project one year access to their 
suite for free, a service worth many thousands of dollars, and I used their 
test suite within the time limit they gave us to take httpd from many hundreds 
of protocol violations down to zero.

All violations were backported to v2.4 but this one, and as a result Apache is 
not listed here: http://coad.measurement-factory.com/clients.html

> Which RFC violation, a proxy socket connection error should return 504
> Gateway Timeout??

The RFC violation that was flagged by the test suite as described above.

> I see that RFC2616 14.9.4 is about cache, why don't you fix this in mod_cac=
> he?

The fix applied consisted of the required changes to make the Co-Advisor suite 
resolve the violation.

>> 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?

It should not be reverted, no.

The commit was not vetoed, the backport to 2.4 was, and for a good reason - a 
change to the response code in a point release would have destabilised some 
people. Fixing this issue on trunk for a future release is entirely fine.

The problem you’re really trying to solve is the inconvenience of having trunk 
and v2.4 being different.

The fix to this is to replace HTTP_BAD_GATEWAY with a neural macro like 
PROXY_TIMEOUT, and then #define PROXY_TIMEOUT to be HTTP_GATEWAY_TIME_OUT on 
trunk, and HTTP_BAD_GATEWAY on v2.4.

Please don’t back out protocol behaviour without checking the origin of the 
change first. All of what I describe above is in our commit history and mailing 
lists.

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

2020-12-14 Thread Ruediger Pluem 



On 12/12/20 12:59 AM, Roy T. Fielding wrote:
>> On Dec 11, 2020, at 6:28 AM, Ruediger Pluem > > wrote:
>> On 12/11/20 1:13 PM, Yann Ylavic wrote:
>>> On Fri, Dec 11, 2020 at 12:49 PM Graham Leggett >> > wrote:

 On 10 Dec 2020, at 18:04, yla...@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
>> 
>>
>> 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

2020-12-11 Thread Roy T. Fielding 
> On Dec 11, 2020, at 6:28 AM, Ruediger Pluem  wrote:
> On 12/11/20 1:13 PM, Yann Ylavic wrote:
>> On Fri, Dec 11, 2020 at 12:49 PM Graham Leggett  wrote:
>>> 
>>> On 10 Dec 2020, at 18:04, yla...@apache.org wrote:
>>> 
>>> Author: ylavic
>>> Date: Thu Dec 10 16:04:34 2020
>>> New Revision: 1884280
>>> 
>>> URL: http://svn.apache.org/viewvc?rev=1884280=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.4RFC7234, 5.2.2.1
> https://tools.ietf.org/html/rfc7234#section-5.2.2 
> 
> 10.5.3RFC7231, 6.6.3  
> https://tools.ietf.org/html/rfc7231#section-6.6.3 
> 
> 10.5.5RFC7231, 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#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
>  
> 
> 
> 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 


If you think we need to change other things in the specs, please file bugs now.

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

2020-12-11 Thread Ruediger Pluem 



On 12/11/20 1:13 PM, Yann Ylavic wrote:
> On Fri, Dec 11, 2020 at 12:49 PM Graham Leggett  wrote:
>>
>> On 10 Dec 2020, at 18:04, yla...@apache.org wrote:
>>
>> Author: ylavic
>> Date: Thu Dec 10 16:04:34 2020
>> New Revision: 1884280
>>
>> URL: http://svn.apache.org/viewvc?rev=1884280=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

2020-12-11 Thread Yann Ylavic 
On Fri, Dec 11, 2020 at 1:13 PM Yann Ylavic  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

2020-12-11 Thread Yann Ylavic 
On Fri, Dec 11, 2020 at 12:49 PM Graham Leggett  wrote:
>
> On 10 Dec 2020, at 18:04, yla...@apache.org wrote:
>
> Author: ylavic
> Date: Thu Dec 10 16:04:34 2020
> New Revision: 1884280
>
> URL: http://svn.apache.org/viewvc?rev=1884280=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

2020-12-11 Thread Graham Leggett 
On 10 Dec 2020, at 18:04, yla...@apache.org wrote:

> Author: ylavic
> Date: Thu Dec 10 16:04:34 2020
> New Revision: 1884280
> 
> URL: http://svn.apache.org/viewvc?rev=1884280=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
>  
> 

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
—