RE: [RFC] BUG/MEDIUM: Checks: support for HTTP health checks with POST and data corrupted by extra connection close

[Kiran Gavali]

Hi Christopher And Willy,

>I've finally pushed this patch ! Better late that never. I will backport it to
>2.1 and 2.0 for now. But I guess it could be backported as far as 1.7.

Thank you very much for your support and accepting the patch.

Thanks,
Kiran Gavali

-Original Message-
From: Christopher Faulet [mailto:cfau...@haproxy.com]
Sent: Tuesday, April 28, 2020 10:52 AM
To: Kiran Gavali ; Willy Tarreau 
Cc: haproxy@formilux.org; Shivharsh Singh ; 
Priya Ranjan ; Ramanpreet Singh Bakshi 

Subject: Re: [RFC] BUG/MEDIUM: Checks: support for HTTP health checks with POST 
and data corrupted by extra connection close

Le 16/04/2020 à 07:55, Kiran Gavali a écrit :
> Hi Christopher,
>
> Thank you or updating the patch. I have seen the "http-check send" directive 
> you have added in the patch and agree with your point that the syntax must be 
> compatible with the rework in HTTP checks for H1/H2.
> As for the reg-test, sorry for my misinterpretation, I thought you wanted me 
> to run the test suite to ensure that the patch doesn’t break any existing 
> functionality.
> Nevertheless, I can now see that you have already created http-check-send.vtc 
> and included it in the patch file.
>
> So, I hope it is right for me to now presume that the patch is complete and 
> there’s nothing actionable at my end.
> Please do let me know if any further action is required from my end in this 
> regard.
>

Hi,

I've finally pushed this patch ! Better late that never. I will backport it to
2.1 and 2.0 for now. But I guess it could be backported as far as 1.7.

Thanks,
--
Christopher Faulet

 The contents of this e-mail and any attachment(s) are confidential and 
intended for the named recipient(s) only. It shall not attach any liability on 
the originator or NECTI or its affiliates. Any views or opinions presented in 
this email are solely those of the author and may not necessarily reflect the 
opinions of NECTI or its affiliates. Any form of reproduction, dissemination, 
copying, disclosure, modification, distribution and / or publication of this 
message without the prior written consent of the author of this e-mail is 
strictly prohibited. If you have received this email in error please delete it 
and notify the sender immediately.

Re: [RFC] BUG/MEDIUM: Checks: support for HTTP health checks with POST and data corrupted by extra connection close

[Christopher Faulet]

Le 16/04/2020 à 07:55, Kiran Gavali a écrit :

Hi Christopher,

Thank you or updating the patch. I have seen the "http-check send" directive 
you have added in the patch and agree with your point that the syntax must be compatible 
with the rework in HTTP checks for H1/H2.
As for the reg-test, sorry for my misinterpretation, I thought you wanted me to 
run the test suite to ensure that the patch doesn’t break any existing 
functionality.
Nevertheless, I can now see that you have already created http-check-send.vtc 
and included it in the patch file.

So, I hope it is right for me to now presume that the patch is complete and 
there’s nothing actionable at my end.
Please do let me know if any further action is required from my end in this 
regard.



Hi,

I've finally pushed this patch ! Better late that never. I will backport it to 
2.1 and 2.0 for now. But I guess it could be backported as far as 1.7.


Thanks,
--
Christopher Faulet

RE: [RFC] BUG/MEDIUM: Checks: support for HTTP health checks with POST and data corrupted by extra connection close

[Kiran Gavali]

Hi Christopher,

Thank you or updating the patch. I have seen the "http-check send" directive 
you have added in the patch and agree with your point that the syntax must be 
compatible with the rework in HTTP checks for H1/H2.
As for the reg-test, sorry for my misinterpretation, I thought you wanted me to 
run the test suite to ensure that the patch doesn’t break any existing 
functionality.
Nevertheless, I can now see that you have already created http-check-send.vtc 
and included it in the patch file.

So, I hope it is right for me to now presume that the patch is complete and 
there’s nothing actionable at my end.
Please do let me know if any further action is required from my end in this 
regard.

Thank You,
Kiran Gavali

-Original Message-
From: Christopher Faulet [mailto:cfau...@haproxy.com]
Sent: Thursday, April 9, 2020 1:17 PM
To: Kiran Gavali ; Willy Tarreau 
Cc: haproxy@formilux.org; Shivharsh Singh ; 
Priya Ranjan ; Ramanpreet Singh Bakshi 

Subject: Re: [RFC] BUG/MEDIUM: Checks: support for HTTP health checks with POST 
and data corrupted by extra connection close

Le 08/04/2020 à 13:08, Kiran Gavali a écrit :
> Hi Christopher And Willy,
>
> Please review the RT test results.
>

Hi,

Sorry for the delay, I was fully busy on the tcp-check refactoring and had no 
brain time to work on this :)

So, as said, the problem was not to support header and body addition to HTTP 
health checks but to use the good syntax. I'll rework the HTTP checks to support
H1/H2 checks. And the syntax must be compatible.

I attached a patch. It adds the "http-check send" directive. It is very close 
to your patch but the syntax should be compatible with the work I'll do on the 
http checks. at least I hope so.

The "send" keyword may seem a bit strange but one idea of the refactoring is to 
support several request/response exchanges within the same HTTP check.

Just a note about the reg-tests. My comment about it was not to run the 
testsuite but to add a specific reg-test to validate the feature.

--
Christopher Faulet

 The contents of this e-mail and any attachment(s) are confidential and 
intended for the named recipient(s) only. It shall not attach any liability on 
the originator or NECTI or its affiliates. Any views or opinions presented in 
this email are solely those of the author and may not necessarily reflect the 
opinions of NECTI or its affiliates. Any form of reproduction, dissemination, 
copying, disclosure, modification, distribution and / or publication of this 
message without the prior written consent of the author of this e-mail is 
strictly prohibited. If you have received this email in error please delete it 
and notify the sender immediately.

Re: [RFC] BUG/MEDIUM: Checks: support for HTTP health checks with POST and data corrupted by extra connection close

[Christopher Faulet]

Le 08/04/2020 à 13:08, Kiran Gavali a écrit :

Hi Christopher And Willy,

Please review the RT test results.



Hi,

Sorry for the delay, I was fully busy on the tcp-check refactoring and had no 
brain time to work on this :)


So, as said, the problem was not to support header and body addition to HTTP 
health checks but to use the good syntax. I'll rework the HTTP checks to support 
H1/H2 checks. And the syntax must be compatible.


I attached a patch. It adds the "http-check send" directive. It is very close to 
your patch but the syntax should be compatible with the work I'll do on the http 
checks. at least I hope so.


The "send" keyword may seem a bit strange but one idea of the refactoring is to 
support several request/response exchanges within the same HTTP check.


Just a note about the reg-tests. My comment about it was not to run the 
testsuite but to add a specific reg-test to validate the feature.


--
Christopher Faulet
>From 6fb6f1147f641c870adc4173644c533c2e5275c7 Mon Sep 17 00:00:00 2001
From: Christopher Faulet 
Date: Thu, 9 Apr 2020 08:44:06 +0200
Subject: [PATCH] MINOR: checks: Add a way to send custom headers and payload
 during http chekcs

The 'http-check send' directive have been added to add headers and optionnaly a
payload to the request sent during HTTP healthchecks. The request line may be
customized by the "option httpchk" directive but there was not official way to
add extra headers. An old trick consisted to hide these headers at the end of
the version string, on the "option httpchk" line. And it was impossible to add
an extra payload with an "http-check expect" directive because of the
"Connection: close" header appended to the request (See issue #16 for details).

So to make things official and fully support payload additions, the "http-check
send" directive have been added :

option httpchk POST /status HTTP/1.1

http-check send hdr Content-Type "application/json;charset=UTF-8" \
hdr X-test-1 value1 hdr X-test-2 value2 \
body "{id: 1, field: \"value\"}"

When a payload is defined, the Content-Length header is automatically added. So
chunk-encoded requests are not supported yet. For now, there is no special
validity checks on the extra headers.

This patch is inspired by Kiran Gavali's work. It should fix the issue #16 and
as far as possible, it may be backported, at least as far as 1.8.
---
 doc/configuration.txt|  38 ++-
 include/types/proxy.h|   4 +
 reg-tests/checks/http-check-send.vtc | 151 +++
 src/cfgparse-listen.c| 121 +++--
 src/checks.c |  23 +++-
 src/haproxy.c|   2 +
 6 files changed, 325 insertions(+), 14 deletions(-)
 create mode 100644 reg-tests/checks/http-check-send.vtc

diff --git a/doc/configuration.txt b/doc/configuration.txt
index 8347e8a4d..f94a82543 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -4463,6 +4463,33 @@ http-check expect [!]  
   See also : "option httpchk", "http-check disable-on-404"
 
 
+http-check send [hdr  ]* [body ]
+  Add a possible list of headers and/or a body to the request sent during HTTP
+  health checks.
+  May be used in sections :   defaults | frontend | listen | backend
+ yes   |no|   yes  |   yes
+  Arguments :
+hdr adds the HTTP header field whose name is specified in
+  and whose value is defined by  to the
+ request sent during HTTP health checks.
+
+body add the body defined by  to the request sent
+ sent during HTTP health checks. If defined, the
+ "Content-Length" header is thus automatically added
+ to the request.
+
+  In addition to the request line defined by the "option httpchk" directive,
+  this one is the valid way to add some headers and optionally a body to the
+  request sent during HTTP health checks. If a body is defined, the associate
+  "Content-Length" header is automatically added. The old trick consisting to
+  add headers after the version string on the "option httpchk" line is now
+  deprecated. Note also the "Connection: close" header is still added if a
+  "http-check expect" direcive is defined independently of this directive, just
+  like the state header if the directive "http-check send-state" is defined.
+
+  See also : "option httpchk", "http-check send-state" and "http-check expect"
+
+
 http-check send-state
   Enable emission of a state header with HTTP health checks
   May be used in sections :   defaults | frontend | listen | backend
@@ -7108,8 +7135,7 @@ option httpchk   
  is the optional HTTP version string. It defaults to "HTTP/1.0"
   but some servers might behave incorrectly in HTTP 1.0, so turning
   it to HTTP/1.1 may sometimes help. Note that the Host field is
-  mandatory in

RE: [RFC] BUG/MEDIUM: Checks: support for HTTP health checks with POST and data corrupted by extra connection close

[Kiran Gavali]

Hi Christopher And Willy,

Please review the RT test results.

Thanks,
Kiran Gavali

-Original Message-
From: Kiran Gavali
Sent: Monday, March 30, 2020 6:59 PM
To: Christopher Faulet ; Willy Tarreau 
Cc: haproxy@formilux.org; Shivharsh Singh ; 
Priya Ranjan ; Ramanpreet Singh Bakshi 
; Kiran Gavali 
Subject: RE: [RFC] BUG/MEDIUM: Checks: support for HTTP health checks with POST 
and data corrupted by extra connection close

Thank you Christopher for providing HAProxy reg-test suite !

>About the regression tests, we use varnishtest 
>(https://github.com/vtest/VTest).
>All our tests are placed in the "reg-tests" subdirectory. Here is a 
>documentation to write VTC tests:
>https://varnish-cache.org/docs/trunk/reference/vtc.html.

>To run tests, you may use the script "scripts/run-regtests.sh" or the "make 
>reg-tests" command.

We have executed the regression tests using varnishtest 
(https://github.com/vtest/VTest).
Please find the attached HAProxy regression test result, there is no any test 
case failed , All tests are passed.

## Starting vtest ## Testing 
with haproxy version: 2.2-dev4-0576db-97
0 tests failed, 0 tests skipped, 62 tests passed

Please confirm the RT execution and suggest us further any work is need to 
resolve this bug.

Thanks,
Kiran Gavali

-Original Message-
From: Christopher Faulet [mailto:cfau...@haproxy.com]
Sent: Thursday, March 26, 2020 2:15 PM
To: Kiran Gavali ; Willy Tarreau 
Cc: haproxy@formilux.org; Shivharsh Singh ; 
Priya Ranjan ; Ramanpreet Singh Bakshi 

Subject: Re: [RFC] BUG/MEDIUM: Checks: support for HTTP health checks with POST 
and data corrupted by extra connection close

Le 26/03/2020 à 09:24, Kiran Gavali a écrit :
> Thank you Christopher and Willy for your responses !
>
> We have discussed the resolution for the issue on GitHub at following
> link: https://github.com/haproxy/haproxy/issues/16
> However to further explain the patch fix, we have introduced new options, 
> "header" and "body" in http-check directive. Based on the content for these 
> options configured in haproxy.cfg and if expect option is also configured for 
> http-check, the header is added to a buffer followed by the "Connection: 
> close" string which is further followed by the body.
> For cases, when either header or body or both is not configured in 
> haproxy.cfg, we have used default values to create the data packet in the 
> buffer.
> We would definitely update the documentation once the patch is finalized and 
> therefore shared it with RFC tag.
Ah, ok. I understand now. I missed the RFC tag in the email subject, sorry :)

>
> To answer your  query on reg-test, We have performed regression testing of 
> the patch using the RT suite available at our end. We can share with you the 
> test report, if required. However, if there is any community RT suite that 
> you would like us to follow, please do let me know.
>

About the regression tests, we use varnishtest (https://github.com/vtest/VTest).
All our tests are placed in the "reg-tests" subdirectory. Here is a 
documentation to write VTC tests:
https://varnish-cache.org/docs/trunk/reference/vtc.html.

To run tests, you may use the script "scripts/run-regtests.sh" or the "make 
reg-tests" command.

> As far as the relevance of this patch is concerned, considering the
> planned http-check refactoring at your end, we were already aware that
> the patch might not be merged due to the  fact that the check system
> is currently being reworked to support muxes for HTTP/1 and HTTP/2 so
> that there are better checks in 2.2
>

As Willy said, have a solution for current versions is also important. But the 
syntax must be compatible with the next one. This part must be discussed first.

--
Christopher Faulet

 The contents of this e-mail and any attachment(s) are confidential and 
intended for the named recipient(s) only. It shall not attach any liability on 
the originator or NECTI or its affiliates. Any views or opinions presented in 
this email are solely those of the author and may not necessarily reflect the 
opinions of NECTI or its affiliates. Any form of reproduction, dissemination, 
copying, disclosure, modification, distribution and / or publication of this 
message without the prior written consent of the author of this e-mail is 
strictly prohibited. If you have received this email in error please delete it 
and notify the sender immediately.
[root@haproxy haproxy]# sh scripts/run-regtests.sh

## Preparing to run tests ##
Testing with haproxy version: 2.2-dev4-0576db-97
Target : linux-glibc
Options : +EPOLL -KQUEUE +NETFILTER +PCRE +PCRE_JIT -PCRE2 -PCRE2_JIT +POLL 
-PRIVATE_CACHE +THREAD -PTHREAD_PSHARED +BA

RE: [RFC] BUG/MEDIUM: Checks: support for HTTP health checks with POST and data corrupted by extra connection close

[Kiran Gavali]

Thank you Christopher for providing HAProxy reg-test suite !

>About the regression tests, we use varnishtest 
>(https://github.com/vtest/VTest).
>All our tests are placed in the "reg-tests" subdirectory. Here is a 
>documentation to write VTC tests:
>https://varnish-cache.org/docs/trunk/reference/vtc.html.

>To run tests, you may use the script "scripts/run-regtests.sh" or the "make 
>reg-tests" command.

We have executed the regression tests using varnishtest 
(https://github.com/vtest/VTest).
Please find the attached HAProxy regression test result, there is no any test 
case failed , All tests are passed.

## Starting vtest ## Testing 
with haproxy version: 2.2-dev4-0576db-97
0 tests failed, 0 tests skipped, 62 tests passed

Please confirm the RT execution and suggest us further any work is need to 
resolve this bug.

Thanks,
Kiran Gavali

-Original Message-
From: Christopher Faulet [mailto:cfau...@haproxy.com]
Sent: Thursday, March 26, 2020 2:15 PM
To: Kiran Gavali ; Willy Tarreau 
Cc: haproxy@formilux.org; Shivharsh Singh ; 
Priya Ranjan ; Ramanpreet Singh Bakshi 

Subject: Re: [RFC] BUG/MEDIUM: Checks: support for HTTP health checks with POST 
and data corrupted by extra connection close

Le 26/03/2020 à 09:24, Kiran Gavali a écrit :
> Thank you Christopher and Willy for your responses !
>
> We have discussed the resolution for the issue on GitHub at following
> link: https://github.com/haproxy/haproxy/issues/16
> However to further explain the patch fix, we have introduced new options, 
> "header" and "body" in http-check directive. Based on the content for these 
> options configured in haproxy.cfg and if expect option is also configured for 
> http-check, the header is added to a buffer followed by the "Connection: 
> close" string which is further followed by the body.
> For cases, when either header or body or both is not configured in 
> haproxy.cfg, we have used default values to create the data packet in the 
> buffer.
> We would definitely update the documentation once the patch is finalized and 
> therefore shared it with RFC tag.
Ah, ok. I understand now. I missed the RFC tag in the email subject, sorry :)

>
> To answer your  query on reg-test, We have performed regression testing of 
> the patch using the RT suite available at our end. We can share with you the 
> test report, if required. However, if there is any community RT suite that 
> you would like us to follow, please do let me know.
>

About the regression tests, we use varnishtest (https://github.com/vtest/VTest).
All our tests are placed in the "reg-tests" subdirectory. Here is a 
documentation to write VTC tests:
https://varnish-cache.org/docs/trunk/reference/vtc.html.

To run tests, you may use the script "scripts/run-regtests.sh" or the "make 
reg-tests" command.

> As far as the relevance of this patch is concerned, considering the
> planned http-check refactoring at your end, we were already aware that
> the patch might not be merged due to the  fact that the check system
> is currently being reworked to support muxes for HTTP/1 and HTTP/2 so
> that there are better checks in 2.2
>

As Willy said, have a solution for current versions is also important. But the 
syntax must be compatible with the next one. This part must be discussed first.

--
Christopher Faulet

 The contents of this e-mail and any attachment(s) are confidential and 
intended for the named recipient(s) only. It shall not attach any liability on 
the originator or NECTI or its affiliates. Any views or opinions presented in 
this email are solely those of the author and may not necessarily reflect the 
opinions of NECTI or its affiliates. Any form of reproduction, dissemination, 
copying, disclosure, modification, distribution and / or publication of this 
message without the prior written consent of the author of this e-mail is 
strictly prohibited. If you have received this email in error please delete it 
and notify the sender immediately.
[root@haproxy haproxy]# sh scripts/run-regtests.sh

## Preparing to run tests ##
Testing with haproxy version: 2.2-dev4-0576db-97
Target : linux-glibc
Options : +EPOLL -KQUEUE +NETFILTER +PCRE +PCRE_JIT -PCRE2 -PCRE2_JIT +POLL 
-PRIVATE_CACHE +THREAD -PTHREAD_PSHARED +BACKTRACE -STATIC_PCRE -STATIC_PCRE2 
+TPROXY +LINUX_TPROXY +LINUX_SPLICE +LIBCRYPT +CRYPT_H +GETADDRINFO +OPENSSL 
+LUA +FUTEX +ACCEPT4 +ZLIB -SLZ +CPU_AFFINITY +TFO +NS +DL +RT -DEVICEATLAS 
-51DEGREES -WURFL -SYSTEMD -OBSOLETE_LINKER +PRCTL +THREAD_DUMP -EVPORTS
## Gathering tests to run ##
  Add test: reg-tests/cache/basic.vtc
  Add test: reg-t

Re: [RFC] BUG/MEDIUM: Checks: support for HTTP health checks with POST and data corrupted by extra connection close

[Aleksandar Lazic]

On 26.03.20 09:42, Willy Tarreau wrote:

On Thu, Mar 26, 2020 at 09:25:31AM +0100, Christopher Faulet wrote:

It is a good idea. For now, I have only few idea though. For the header
part, it must not be a raw block. Because it will be really hard to keep the
same syntax with the HTX. I propose to keep same syntax than http-request
rules :

http-check add-header  

And later :

http-check set-header  
http-check del-header 
http-check replace-header   
http-check replace-value   


I don't think we'll need to perform such operations in health checks because
they're normally only useful for real traffic. Here everything is built from
scratch so deleting a header or replacing it will be of very limited use, it
just means they would have been added by a previous rule. Also thinking about
this when combined with multiple requests gives me a headache :-)


Well I think that could be required when the backend have different vhosts
and ssl-vhosts.

Can we add request|respone to make it a little bit clear when which header will
be set.

# set session cookie for example
http-check request set-header  |
http-check request set-sni  |

http-check response expect 
# I'm not sure if there is a use case for that
http-check response del-header 


What I like is to be able to define a check backend and use the track keyword
for checks.

http://cbonte.github.io/haproxy-dconv/2.2/configuration.html#5.2-track

Will this be also available in the first implementation?


Another idea could be to have a syntax similar to the HTTP return action :

http-check hdr   hdr   string 

or


why not both => AND but only one is usable in one check sequence?


http-check hdr   hdr   file 


These ones do seem like good ideas. They continue to allow to fully
define a check in a simple statement. And maybe later when we start
to think about sending check sequences they will be very convenient
because each line could define an entire request (possibly reusing
parts from the previous response if needed).


The request URI is passed on the "option httpchk" line. But, maybe the
method, the path and the version may also be defined on such line.


Agreed.


There are many other problems to deal with. But it is a first step. The
content-length must be automatically deduced when a body is defined.


Fully agreed!


For
current versions, the connection header must also be added the same way it
is done today.


Sure. I'd say it's a "detail" in that it's not visible from the users' end.


During my refactoring, I can try to first work on a way to support such
syntax on current versions. But we must be sure to have the right syntax
first. It is the harder part :)


Yes, that's exactly my point as well. If we figure the long-term picture,
we can narrow it down for a first implementation.

Willy

Re: [RFC] BUG/MEDIUM: Checks: support for HTTP health checks with POST and data corrupted by extra connection close

[Christopher Faulet]

Le 26/03/2020 à 09:24, Kiran Gavali a écrit :

Thank you Christopher and Willy for your responses !

We have discussed the resolution for the issue on GitHub at following link: 
https://github.com/haproxy/haproxy/issues/16
However to further explain the patch fix, we have introduced new options, "header" and 
"body" in http-check directive. Based on the content for these options configured in haproxy.cfg 
and if expect option is also configured for http-check, the header is added to a buffer followed by the 
"Connection: close" string which is further followed by the body.
For cases, when either header or body or both is not configured in haproxy.cfg, 
we have used default values to create the data packet in the buffer.
We would definitely update the documentation once the patch is finalized and 
therefore shared it with RFC tag.

Ah, ok. I understand now. I missed the RFC tag in the email subject, sorry :)



To answer your  query on reg-test, We have performed regression testing of the 
patch using the RT suite available at our end. We can share with you the test 
report, if required. However, if there is any community RT suite that you would 
like us to follow, please do let me know.



About the regression tests, we use varnishtest (https://github.com/vtest/VTest). 
All our tests are placed in the "reg-tests" subdirectory. Here is a 
documentation to write VTC tests: 
https://varnish-cache.org/docs/trunk/reference/vtc.html.


To run tests, you may use the script "scripts/run-regtests.sh" or the "make 
reg-tests" command.



As far as the relevance of this patch is concerned, considering the planned 
http-check refactoring at your end, we were already aware that the patch might 
not be merged due to the  fact that the check system is currently being 
reworked to support muxes for HTTP/1 and HTTP/2 so that there are better checks 
in 2.2



As Willy said, have a solution for current versions is also important. But the 
syntax must be compatible with the next one. This part must be discussed first.


--
Christopher Faulet

Re: [RFC] BUG/MEDIUM: Checks: support for HTTP health checks with POST and data corrupted by extra connection close

[Willy Tarreau]

On Thu, Mar 26, 2020 at 09:25:31AM +0100, Christopher Faulet wrote:
> It is a good idea. For now, I have only few idea though. For the header
> part, it must not be a raw block. Because it will be really hard to keep the
> same syntax with the HTX. I propose to keep same syntax than http-request
> rules :
> 
> http-check add-header  
> 
> And later :
> 
> http-check set-header  
> http-check del-header 
> http-check replace-header   
> http-check replace-value   

I don't think we'll need to perform such operations in health checks because
they're normally only useful for real traffic. Here everything is built from
scratch so deleting a header or replacing it will be of very limited use, it
just means they would have been added by a previous rule. Also thinking about
this when combined with multiple requests gives me a headache :-)

> Another idea could be to have a syntax similar to the HTTP return action :
> 
> http-check hdr   hdr   string 
> 
> or
> 
> http-check hdr   hdr   file 

These ones do seem like good ideas. They continue to allow to fully
define a check in a simple statement. And maybe later when we start
to think about sending check sequences they will be very convenient
because each line could define an entire request (possibly reusing
parts from the previous response if needed).

> The request URI is passed on the "option httpchk" line. But, maybe the
> method, the path and the version may also be defined on such line.

Agreed.

> There are many other problems to deal with. But it is a first step. The
> content-length must be automatically deduced when a body is defined.

Fully agreed!

> For
> current versions, the connection header must also be added the same way it
> is done today.

Sure. I'd say it's a "detail" in that it's not visible from the users' end.

> During my refactoring, I can try to first work on a way to support such
> syntax on current versions. But we must be sure to have the right syntax
> first. It is the harder part :)

Yes, that's exactly my point as well. If we figure the long-term picture,
we can narrow it down for a first implementation.

Willy

RE: [RFC] BUG/MEDIUM: Checks: support for HTTP health checks with POST and data corrupted by extra connection close

[Kiran Gavali]

Thank you Christopher and Willy for your responses !

We have discussed the resolution for the issue on GitHub at following link: 
https://github.com/haproxy/haproxy/issues/16
However to further explain the patch fix, we have introduced new options, 
"header" and "body" in http-check directive. Based on the content for these 
options configured in haproxy.cfg and if expect option is also configured for 
http-check, the header is added to a buffer followed by the "Connection: close" 
string which is further followed by the body.
For cases, when either header or body or both is not configured in haproxy.cfg, 
we have used default values to create the data packet in the buffer.
We would definitely update the documentation once the patch is finalized and 
therefore shared it with RFC tag.

To answer your  query on reg-test, We have performed regression testing of the 
patch using the RT suite available at our end. We can share with you the test 
report, if required. However, if there is any community RT suite that you would 
like us to follow, please do let me know.

As far as the relevance of this patch is concerned, considering the planned 
http-check refactoring at your end, we were already aware that the patch might 
not be merged due to the  fact that the check system is currently being 
reworked to support muxes for HTTP/1 and HTTP/2 so that there are better checks 
in 2.2

Thanks,
Kiran Gavali

-Original Message-
From: Willy Tarreau [mailto:w...@1wt.eu]
Sent: Thursday, March 26, 2020 1:07 PM
To: Christopher Faulet 
Cc: Kiran Gavali ; haproxy@formilux.org; Shivharsh 
Singh ; Priya Ranjan 
; Ramanpreet Singh Bakshi 

Subject: Re: [RFC] BUG/MEDIUM: Checks: support for HTTP health checks with POST 
and data corrupted by extra connection close

Hi Christopher,

On Thu, Mar 26, 2020 at 07:40:06AM +0100, Christopher Faulet wrote:
> Thanks ! However there are many problems with your patch. First there
> is no commit message. You must explain what is exactly the problem and
> how you fix it. Then, the documentation must be updated. You
> introduced 2 new options without any explanation. Without these info,
> it is really hard to figure out what the patch do.

I noticed these as well, thanks for pointing out before me :-)

> But reading how headers and body buffers are inserted in the request,
> I suspect some bugs. A reg-test is probably necessary to validate such
> feature.

Yes that would indeed be useful.

> But, don't bother for now submitting a new patch. I'm currently
> refactoring the tcp-checks. But on my todo list, the next big step is
> the http-check refactoring. It is painful but I hope to finish the
> refactoring of the checks for the 2.2.0.

My main concern is the backward compatibility. Kiran's patch is small enough to 
be backported, so that might constitute a possible solution for existing 
setups. However we cannot afford to do that if it works differently than what 
we'll have in the final release. As such I think it's important to quickly 
figure how we want such a separate body part to be *configured* and make sure 
that it's done the same in your new version and in Kiran's patch. If so, then 
we could imagine merging it early to have it backported so that it's ultimately 
replaced in 2.2 by your work once ready.

What do you think ?

Thanks,
Willy

 The contents of this e-mail and any attachment(s) are confidential and 
intended for the named recipient(s) only. It shall not attach any liability on 
the originator or NECTI or its affiliates. Any views or opinions presented in 
this email are solely those of the author and may not necessarily reflect the 
opinions of NECTI or its affiliates. Any form of reproduction, dissemination, 
copying, disclosure, modification, distribution and / or publication of this 
message without the prior written consent of the author of this e-mail is 
strictly prohibited. If you have received this email in error please delete it 
and notify the sender immediately.

Re: [RFC] BUG/MEDIUM: Checks: support for HTTP health checks with POST and data corrupted by extra connection close

[Christopher Faulet]

Le 26/03/2020 à 08:36, Willy Tarreau a écrit :

Hi Christopher,

On Thu, Mar 26, 2020 at 07:40:06AM +0100, Christopher Faulet wrote:

Thanks ! However there are many problems with your patch. First there is no
commit message. You must explain what is exactly the problem and how you fix
it. Then, the documentation must be updated. You introduced 2 new options
without any explanation. Without these info, it is really hard to figure out
what the patch do.


I noticed these as well, thanks for pointing out before me :-)


But reading how headers and body buffers are inserted in
the request, I suspect some bugs. A reg-test is probably necessary to
validate such feature.


Yes that would indeed be useful.


But, don't bother for now submitting a new patch. I'm currently refactoring
the tcp-checks. But on my todo list, the next big step is the http-check
refactoring. It is painful but I hope to finish the refactoring of the
checks for the 2.2.0.


My main concern is the backward compatibility. Kiran's patch is small
enough to be backported, so that might constitute a possible solution
for existing setups. However we cannot afford to do that if it works
differently than what we'll have in the final release. As such I think
it's important to quickly figure how we want such a separate body part
to be *configured* and make sure that it's done the same in your new
version and in Kiran's patch. If so, then we could imagine merging it
early to have it backported so that it's ultimately replaced in 2.2 by
your work once ready.

What do you think ?



It is a good idea. For now, I have only few idea though. For the header part, it 
must not be a raw block. Because it will be really hard to keep the same syntax 
with the HTX. I propose to keep same syntax than http-request rules :


http-check add-header  

And later :

http-check set-header  
http-check del-header 
http-check replace-header   
http-check replace-value   

Another idea could be to have a syntax similar to the HTTP return action :

http-check hdr   hdr   string 

or

http-check hdr   hdr   file 

The request URI is passed on the "option httpchk" line. But, maybe the method, 
the path and the version may also be defined on such line.


There are many other problems to deal with. But it is a first step. The 
content-length must be automatically deduced when a body is defined. For current 
versions, the connection header must also be added the same way it is done today.


During my refactoring, I can try to first work on a way to support such syntax 
on current versions. But we must be sure to have the right syntax first. It is 
the harder part :)


--
Christopher Faulet

Re: [RFC] BUG/MEDIUM: Checks: support for HTTP health checks with POST and data corrupted by extra connection close

[Willy Tarreau]

Hi Christopher,

On Thu, Mar 26, 2020 at 07:40:06AM +0100, Christopher Faulet wrote:
> Thanks ! However there are many problems with your patch. First there is no
> commit message. You must explain what is exactly the problem and how you fix
> it. Then, the documentation must be updated. You introduced 2 new options
> without any explanation. Without these info, it is really hard to figure out
> what the patch do.

I noticed these as well, thanks for pointing out before me :-)

> But reading how headers and body buffers are inserted in
> the request, I suspect some bugs. A reg-test is probably necessary to
> validate such feature.

Yes that would indeed be useful.

> But, don't bother for now submitting a new patch. I'm currently refactoring
> the tcp-checks. But on my todo list, the next big step is the http-check
> refactoring. It is painful but I hope to finish the refactoring of the
> checks for the 2.2.0.

My main concern is the backward compatibility. Kiran's patch is small
enough to be backported, so that might constitute a possible solution
for existing setups. However we cannot afford to do that if it works
differently than what we'll have in the final release. As such I think
it's important to quickly figure how we want such a separate body part
to be *configured* and make sure that it's done the same in your new
version and in Kiran's patch. If so, then we could imagine merging it
early to have it backported so that it's ultimately replaced in 2.2 by
your work once ready.

What do you think ?

Thanks,
Willy

Re: [RFC] BUG/MEDIUM: Checks: support for HTTP health checks with POST and data corrupted by extra connection close

[Christopher Faulet]

Le 23/03/2020 à 13:24, Kiran Gavali a écrit :

Hello Mr. Willy Tarreau ,

I have created a patch to resolve following issue 
"https://github.com/haproxy/haproxy/issues/16"; .
And I have generated the patch files by following the steps defined in 
"https://github.com/haproxy/haproxy/blob/master/CONTRIBUTING"; document.
Request you to please review the patch and provide your feedback.



Hi,

Thanks ! However there are many problems with your patch. First there is no 
commit message. You must explain what is exactly the problem and how you fix it. 
Then, the documentation must be updated. You introduced 2 new options without 
any explanation. Without these info, it is really hard to figure out what the 
patch do. But reading how headers and body buffers are inserted in the request, 
I suspect some bugs. A reg-test is probably necessary to validate such feature.


But, don't bother for now submitting a new patch. I'm currently refactoring the 
tcp-checks. But on my todo list, the next big step is the http-check 
refactoring. It is painful but I hope to finish the refactoring of the checks 
for the 2.2.0.


Regards,
--
Christopher Faulet

[RFC] BUG/MEDIUM: Checks: support for HTTP health checks with POST and data corrupted by extra connection close

[Kiran Gavali]

Hello Mr. Willy Tarreau ,

I have created a patch to resolve following issue 
"https://github.com/haproxy/haproxy/issues/16"; .
And I have generated the patch files by following the steps defined in 
"https://github.com/haproxy/haproxy/blob/master/CONTRIBUTING"; document.
Request you to please review the patch and provide your feedback.

Regards
Kiran Gavali

-Original Message-
From: Willy Tarreau [mailto:w...@1wt.eu]
Sent: Monday, February 24, 2020 1:26 PM
To: Kiran Gavali 
Subject: Re: HAProxy v2.1.0 health checks with POST and data corrupted by extra 
"connection: close"

Hello Kiran,

On Mon, Feb 24, 2020 at 07:00:08AM +, Kiran Gavali wrote:
> Hello Mr. Willy Tarreau ,
>
> Issue Link: https://github.com/haproxy/haproxy/issues/16

Oh I didn't notice that you've updated the report there a month ago, sorry 
about that, I get so many github notifications that I do miss quite some of 
them :-(

> I have reproduced and analyzed the above bug on HAProxy v2.1.0 (using 
> Wireshark).
> As per my findings, the "Connection: close" string is appended after
> the data block instead of being appended after the header. [Refer the
> attached Wireshark screenshot at the end]

Yes absolutely, that's the problem (and limitation) described there.

> As stated by @wtarreau in [
> #https://www.mail-archive.com/haproxy@formilux.org/msg28200.html], it
> would be best to add another option "body" to http-check directive so
> as to correctly differentiate body from header, as shown below:-
>
> option httpchk POST / HTTP/1.1\r\n
> http-check header Host: 10.10.26.236\r\nContent-Type:
> application/json\r\nContent-Length: 38\r\n http-check body
> {"command":"getNodeInfo"} http-check expect rstatus (2|3)[0-9][0-9]
>
> To incorporate the change, we need to do changes in below code:-
> File: cfgparse-listen.c
> Function: int cfg_parse_listen(const char *file, int linenum, char
> **args, int kwm) Code Snippet: curproxy->check_len =
> snprintf(curproxy->check_req, reqlen, "%s %s %s\r\n", args[2],
> args[3], *args[4]?args[4]:"HTTP/1.0");
>
> Please let me know if my understanding of the above issue is correct,
> so that I can proceed further accordingly. Looking forward for your guidance.

Your analysis is correct. However this issue is more than two years old now and 
the check system is currently being reworked in order to support muxes for 
HTTP/1 and HTTP/2 so that we have better checks in 2.2. However I don't know if 
for this specific issue we can come up with something trivial enough to be 
backported to older releases with no risk at all.
I guess that it might be worth trying, but let me be clear about the fact that 
if the code becomes tricky, we may finally decide not to merge it and/or not to 
backport it.

Do not hesitate to share your findings on the mailing list so that others can 
follow the progress and possibly suggest adjustments.

Thanks!
Willy

 The contents of this e-mail and any attachment(s) are confidential and 
intended for the named recipient(s) only. It shall not attach any liability on 
the originator or NECTI or its affiliates. Any views or opinions presented in 
this email are solely those of the author and may not necessarily reflect the 
opinions of NECTI or its affiliates. Any form of reproduction, dissemination, 
copying, disclosure, modification, distribution and / or publication of this 
message without the prior written consent of the author of this e-mail is 
strictly prohibited. If you have received this email in error please delete it 
and notify the sender immediately.


0001-resolving-bug-of-health-checks-with-POST-and-data-co.patch
Description:  0001-resolving-bug-of-health-checks-with-POST-and-data-co.patch