Re: strange cppcheck finding

[Willy Tarreau]

On Mon, Mar 19, 2018 at 06:55:46PM +0500,  ??? wrote:
> (it's master)
> 
> is it in purpose ?
> 
> [src/ssl_sock.c:1553]: (warning) Invalid test for overflow
> 'msg+rec_len overflow is UB.

The code is :

rec_len = (msg[0] << 8) + msg[1];
msg += 2;
if (msg + rec_len > end || msg + rec_len < msg)
return;

It's indeed an overflow check which was placed on purpose. What does
your tool propose as a better way to check for an overflow ? rec_len
being a size_t, it's unsigned so the overflow check is fine and
necessary in my opinion.

Regards,
Willy

Re: add header into http-request redirect

[Willy Tarreau]

On Mon, Mar 19, 2018 at 10:23:47PM +0100, Tim Düsterhus wrote:
> Willy,
> 
> Am 19.03.2018 um 22:15 schrieb Willy Tarreau:
> > Looks like it indeed. By then there was no "http-request" ruleset
> > either. Maybe we could move it to a place where it's generated
> > earlier, or maybe we could ensure that it's computed on the fly
> > when the associated sample fetch function is called for %ID (I
> > didn't remember it was available like this).
> 
> Is there some specific place I should file this "bug" report or is my
> email sufficient for you to keep track of?

We hope soon to reuse the github issues for such things. For now it's
already in my todo list.

Willy

Re: add header into http-request redirect

[Tim Düsterhus]

Willy,

Am 19.03.2018 um 22:15 schrieb Willy Tarreau:
> Looks like it indeed. By then there was no "http-request" ruleset
> either. Maybe we could move it to a place where it's generated
> earlier, or maybe we could ensure that it's computed on the fly
> when the associated sample fetch function is called for %ID (I
> didn't remember it was available like this).

Is there some specific place I should file this "bug" report or is my
email sufficient for you to keep track of?

>>
>> Here's two more that came into my mind:
>>
>> - Expect-CT
>> - Public-Key-Pins (a.k.a. HPKP)
>>
>> Both are deeply related to HSTS due do being TLS security headers. The
>> latter is being deprecated by the browsers, because of footgun issues,
>> though. The former is fairly new.
> 
> Yes it's still a draft (unless I missed the announce).
> 

Expect-CT technically still is a draft [1], but it is implemented since
Google Chrome 61 [2]. Personally I know that Cloudflare already is
setting that header on their responses.

HPKP is deprecated in Google Chrome and header processing will be
removed for Chrome 67 (which is due in May).

Best regards
Tim Düsterhus

[1] https://tools.ietf.org/html/draft-ietf-httpbis-expect-ct-02
[2] https://www.chromestatus.com/feature/5677171733430272
[3]
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/he9tr7p3rZ8/eNMwKPmUBAAJ

Re: add header into http-request redirect

[Willy Tarreau]

On Mon, Mar 19, 2018 at 10:04:25PM +0100, Tim Düsterhus wrote:
> Willy,
> 
> Am 19.03.2018 um 21:47 schrieb Willy Tarreau:
> > Simply because unique-id was created many years before the extensible
> > log-format ou know today existed, and that apparently nobody felt the
> > need to port it. It may be as simple as creating a few sample fetches,
> > I don't know.
> 
> This was more of a rhetorical question. It looks like that the unique ID
> is handled somewhat differently in the processing (just like the
> redirects are). I mentioned it because it possibly is related. Here's an
> example configuration:
> 
> > global
> > stats timeout 30s
> > 
> > defaults
> > log global
> > timeout connect 5s
> > timeout client  50s
> > timeout server  50s
> > unique-id-format %{+X}o\ REQ-%Ts%rt
> > 
> > frontend fe_http
> > mode http
> > 
> > bind :::8080 v4v6
> > 
> > unique-id-header X-Req-ID1
> > http-request set-header X-Req-ID2 %ID
> > http-response set-header X-Req-ID %ID
> > 
> > use_backend bk_example
> > 
> > backend bk_example
> > mode http
> > 
> > http-request set-header Host postb.in
> > server example postb.in:80
> 
> I feel like X-Req-ID1 and X-Req-ID2 should have the same value for the
> upstream service, yet X-Req-ID2 is *empty* for `http-request set-header`
> and works fine for `http-response set-header`. This does not look like
> missing fetches, but rather like the ID being generated *after*
> http-request set-header already has been processed.

Looks like it indeed. By then there was no "http-request" ruleset
either. Maybe we could move it to a place where it's generated
earlier, or maybe we could ensure that it's computed on the fly
when the associated sample fetch function is called for %ID (I
didn't remember it was available like this).

> > I really think it's where we need to invest more thoughts. At least you
> > provided two use cases and that shows that a single header directive
> > might not be enough, and that HSTS definitely isn't a special case at
> > all.
> > 
> 
> Here's two more that came into my mind:
> 
> - Expect-CT
> - Public-Key-Pins (a.k.a. HPKP)
> 
> Both are deeply related to HSTS due do being TLS security headers. The
> latter is being deprecated by the browsers, because of footgun issues,
> though. The former is fairly new.

Yes it's still a draft (unless I missed the announce).

Thanks for your inputs.

Willy

Re: add header into http-request redirect

[Tim Düsterhus]

Willy,

Am 19.03.2018 um 21:47 schrieb Willy Tarreau:
> Simply because unique-id was created many years before the extensible
> log-format ou know today existed, and that apparently nobody felt the
> need to port it. It may be as simple as creating a few sample fetches,
> I don't know.

This was more of a rhetorical question. It looks like that the unique ID
is handled somewhat differently in the processing (just like the
redirects are). I mentioned it because it possibly is related. Here's an
example configuration:

> global
>   stats timeout 30s
> 
> defaults
>   log global
>   timeout connect 5s
>   timeout client  50s
>   timeout server  50s
>   unique-id-format %{+X}o\ REQ-%Ts%rt
> 
> frontend fe_http
>   mode http
> 
>   bind :::8080 v4v6
> 
>   unique-id-header X-Req-ID1
>   http-request set-header X-Req-ID2 %ID
>   http-response set-header X-Req-ID %ID
> 
>   use_backend bk_example
> 
> backend bk_example
>   mode http
>   
>   http-request set-header Host postb.in
>   server example postb.in:80

I feel like X-Req-ID1 and X-Req-ID2 should have the same value for the
upstream service, yet X-Req-ID2 is *empty* for `http-request set-header`
and works fine for `http-response set-header`. This does not look like
missing fetches, but rather like the ID being generated *after*
http-request set-header already has been processed.

> I really think it's where we need to invest more thoughts. At least you
> provided two use cases and that shows that a single header directive
> might not be enough, and that HSTS definitely isn't a special case at
> all.
> 

Here's two more that came into my mind:

- Expect-CT
- Public-Key-Pins (a.k.a. HPKP)

Both are deeply related to HSTS due do being TLS security headers. The
latter is being deprecated by the browsers, because of footgun issues,
though. The former is fairly new.

Best regards
Tim Düsterhus

Re: add header into http-request redirect

[Willy Tarreau]

On Mon, Mar 19, 2018 at 09:40:01PM +0100, Tim Düsterhus wrote:
> As a side question: Why do I have to do unique-id-header, instead of
> http-request set-header for the unique request ID? And why can't I
> capture it with capture (request|response) header but instead have to
> plug into manually into the log format? This feels inconsistent.

Simply because unique-id was created many years before the extensible
log-format ou know today existed, and that apparently nobody felt the
need to port it. It may be as simple as creating a few sample fetches,
I don't know.

> I don't really like the duplication of configuration, though. This would
> be introducing a special case where really no special case should be
> needed and would require me to update headers in two places. But I'm
> also not deep enough in haproxy's internals to know how hard it would be
> treating the `redirect` like a regular backend response and applying the
> regular http-response logic there.

I really think it's where we need to invest more thoughts. At least you
provided two use cases and that shows that a single header directive
might not be enough, and that HSTS definitely isn't a special case at
all.

Cheers,
Willy

Re: add header into http-request redirect

[Tim Düsterhus]

Willy,

Am 19.03.2018 um 11:54 schrieb Willy Tarreau:
>> This issue prevents me from submitting one domain to the HSTS preload
>> list, as I need to perform a redirect on the zone's apex and that
>> redirect does not include the HSTS header.
> 
> I *suspect* that in the end we could simply add a series of "header"
> statements to the redirect rules. These ones would be followed by a
> log-format expression making it possible to send various elements in
> these response. But if it's mainly needed for HSTS, probably that in
> the end we could be fine with (at least initially) adding a single
> "header" directive. We'd then have :
> 
>   http-request redirect location "/foo" header x-my-header "my-expression"
> 

HSTS probably is the most important one. Personally I also add the
unique-id-header to the responses to be able to correlate them to my logs:

>   unique-id-header X-Req-ID
>   http-response set-header X-Req-ID %[unique-id]

It would be good to have them in the redirect responses (but not really
critical).
As a side question: Why do I have to do unique-id-header, instead of
http-request set-header for the unique request ID? And why can't I
capture it with capture (request|response) header but instead have to
plug into manually into the log format? This feels inconsistent.

-

I don't really like the duplication of configuration, though. This would
be introducing a special case where really no special case should be
needed and would require me to update headers in two places. But I'm
also not deep enough in haproxy's internals to know how hard it would be
treating the `redirect` like a regular backend response and applying the
regular http-response logic there.

Best regards
Tim Düsterhus

Re: segfault in haproxy=1.8.4

[William Dauchy]

On Mon, Mar 19, 2018 at 08:41:16PM +0100, Willy Tarreau wrote:
> For me, "experimental" simply means "we did our best to ensure it works
> but we're realist and know that bug-free doesn't exist, so a risk remains
> that a bug will be hard enough to fix so as to force you to disable the
> feature for the time it takes to fix it". This issue between threads and
> queue is one such example. Some of the bugs faced on H2 requiring some
> heavy changes were other examples. But overall we know these features
> are highly demanded and are committed to make them work fine :-)

you are right, we probably magnified in our head the different issues we
had related to this.

> I'm still interested in knowing if you find crazy last percentile values.
> We've had that a very long time ago (version 1.3 or so) when some pending
> conns were accidently skipped, so I know how queues can amplify small
> issues. The only real risk here in my opinion is that the sync point was
> only used for health checks till now so it was running at low loads and
> if it had any issue, it would likely have remained unnoticed. But the code
> is small enough to be audited, and after re-reading it this afternoon I
> found it fine.

will do, migrating some low latency applications is more mid/longterm but
will see how the first results during the preparation tests.

> If you want to run a quick test with epoll, just apply this dirty hack :
>
> diff --git a/src/ev_epoll.c b/src/ev_epoll.c
> index b98ca8c..7bafd16 100644
> --- a/src/ev_epoll.c
> +++ b/src/ev_epoll.c
> @@ -116,7 +116,9 @@ REGPRM2 static void _do_poll(struct poller *p, int exp)
> fd_nbupdt = 0;
>
> /* compute the epoll_wait() timeout */
> -   if (!exp)
> +   if (1)
> +   wait_time = 0;
> +   else if (!exp)
> wait_time = MAX_DELAY_MS;
> else if (tick_is_expired(exp, now_ms)) {
> activity[tid].poll_exp++;
>
> Please note that as this, it's suboptimal because it will increase the
> contention on other places, causing the perfomance to be a bit lower in
> certain situations. I do have some experimental code to loop on epoll
> instead but it's not completely stable yet. We an exchange on this later
> if you want. But feel free to apply this to your latency tests.

thanks a lot, will give a try!

-- 
William

Re: segfault in haproxy=1.8.4

[Willy Tarreau]

On Mon, Mar 19, 2018 at 08:28:14PM +0100, William Dauchy wrote:
> On Mon, Mar 19, 2018 at 07:28:16PM +0100, Willy Tarreau wrote:
> > Threading was clearly released with an experimental status, just like
> > H2, because we knew we'd be facing some post-release issues in these
> > two areas that are hard to get 100% right at once. However I consider
> > that the situation has got much better, and to confirm this, both of
> > these are now enabled by default in HapTech's products. With this said,
> > I expect that over time we'll continue to see a few bugs, but not more
> > than what we're seeing in various areas. For example, we didn't get a
> > single issue on haproxy.org since it was updated to the 1.8.1 or so,
> > 3 months ago. So this is getting quite good.
> 
> ok it was not clear to me as being experimental since it was quite
> widely advertised in several blog posts, but I probably missed
> something.

For me, "experimental" simply means "we did our best to ensure it works
but we're realist and know that bug-free doesn't exist, so a risk remains
that a bug will be hard enough to fix so as to force you to disable the
feature for the time it takes to fix it". This issue between threads and
queue is one such example. Some of the bugs faced on H2 requiring some
heavy changes were other examples. But overall we know these features
are highly demanded and are committed to make them work fine :-)

> > Also if you're running with nbproc > 1 instead, the maxconn setting is
> > not really respected since it becomes per-process. When you run with
> > 8 processes it doesn't mean much anymore, or you need to have small
> > maxconn settings, implying that sometimes a process might queue some
> > requests while there are available slots in other processes. Thus I'd
> > argue that the threads here significantly improve the situation by
> > allowing all connection slots to be used by all CPUs, which is a real
> > improvement which should theorically show you lower latencies.
> 
> thanks for these details. We will run some tests on our side as well;
> the commit message made me worried about the last percentile of
> requests which might have crazy numbers sometimes.
> I now better understand we are speaking about 1.75 extra microseconds.

I'm still interested in knowing if you find crazy last percentile values.
We've had that a very long time ago (version 1.3 or so) when some pending
conns were accidently skipped, so I know how queues can amplify small
issues. The only real risk here in my opinion is that the sync point was
only used for health checks till now so it was running at low loads and
if it had any issue, it would likely have remained unnoticed. But the code
is small enough to be audited, and after re-reading it this afternoon I
found it fine.

> > Note that if this is of interest to you, it's trivial to make haproxy
> > run in busy polling mode, and in this case the performance increases to
> > 30900 conn/s, at the expense of eating all your CPU (which possibly you
> > don't care about if the latency is your worst ennemy). We can possibly
> > even improve this to ensure that it's done only when there are existing
> > sessions on a given thread. Let me know if this is something that could
> > be of interest to you, as I think we could make this configurable and
> > bypass the sync point in this case.
> 
> It is definitely something interesting for us to make it configurable.
> I will try to have a look as well.

If you want to run a quick test with epoll, just apply this dirty hack :

diff --git a/src/ev_epoll.c b/src/ev_epoll.c
index b98ca8c..7bafd16 100644
--- a/src/ev_epoll.c
+++ b/src/ev_epoll.c
@@ -116,7 +116,9 @@ REGPRM2 static void _do_poll(struct poller *p, int exp)
fd_nbupdt = 0;
 
/* compute the epoll_wait() timeout */
-   if (!exp)
+   if (1)
+   wait_time = 0;
+   else if (!exp)
wait_time = MAX_DELAY_MS;
else if (tick_is_expired(exp, now_ms)) {
activity[tid].poll_exp++;

Please note that as this, it's suboptimal because it will increase the
contention on other places, causing the perfomance to be a bit lower in
certain situations. I do have some experimental code to loop on epoll
instead but it's not completely stable yet. We an exchange on this later
if you want. But feel free to apply this to your latency tests.

> > We noticed a nice performance boost on the last one with many cores
> > (24 threads, something like +40% on connection rate), but we'll probably
> > see even better once the rest is addressed.
> 
> indeed, I remember we spoke about those improvments at the last meetup.
> nice work, 1.9 looks already interesting from this point of view!

In fact 1.8's target was to get threads working and in a good enough
shape to improve on them. 1.9's target will be to have them even faster :-)

Willy

Re: segfault in haproxy=1.8.4

[William Dauchy]

Hi Willy,

Thank your for your detailed answer.

On Mon, Mar 19, 2018 at 07:28:16PM +0100, Willy Tarreau wrote:
> Threading was clearly released with an experimental status, just like
> H2, because we knew we'd be facing some post-release issues in these
> two areas that are hard to get 100% right at once. However I consider
> that the situation has got much better, and to confirm this, both of
> these are now enabled by default in HapTech's products. With this said,
> I expect that over time we'll continue to see a few bugs, but not more
> than what we're seeing in various areas. For example, we didn't get a
> single issue on haproxy.org since it was updated to the 1.8.1 or so,
> 3 months ago. So this is getting quite good.

ok it was not clear to me as being experimental since it was quite
widely advertised in several blog posts, but I probably missed
something. Thanks for the clarification though.
Since 1.8.1 the only issues we had with nbthread was indeed related
to using it along with seamless reload but I will give a second try with
the latest patches you released in 1.8 tree today.

> I ran a stress test on this patch, with a single server running with
> "maxconn 1", with a frontend bound to two threads. I measure exactly
> 3 conn/s with a single thread (keep in mind that there's a single
> connection at once), and 28500 with two threads. Thus the sync point
> takes on average an extra 1.75 microsecond, compared to the 35
> microseconds it takes on average to finish processing the request
> (connect, server processing, response, close).
> Also if you're running with nbproc > 1 instead, the maxconn setting is
> not really respected since it becomes per-process. When you run with
> 8 processes it doesn't mean much anymore, or you need to have small
> maxconn settings, implying that sometimes a process might queue some
> requests while there are available slots in other processes. Thus I'd
> argue that the threads here significantly improve the situation by
> allowing all connection slots to be used by all CPUs, which is a real
> improvement which should theorically show you lower latencies.

thanks for these details. We will run some tests on our side as well;
the commit message made me worried about the last percentile of
requests which might have crazy numbers sometimes.
I now better understand we are speaking about 1.75 extra microseconds.

> Note that if this is of interest to you, it's trivial to make haproxy
> run in busy polling mode, and in this case the performance increases to
> 30900 conn/s, at the expense of eating all your CPU (which possibly you
> don't care about if the latency is your worst ennemy). We can possibly
> even improve this to ensure that it's done only when there are existing
> sessions on a given thread. Let me know if this is something that could
> be of interest to you, as I think we could make this configurable and
> bypass the sync point in this case.

It is definitely something interesting for us to make it configurable.
I will try to have a look as well.

> No they're definitely not for 1.8 and still really touchy. We're
> progressively attacking locks wherever we can. Some further patches
> will refine the scheduler to make it more parallel, and even the code
> above will continue to change, see it as a first step in the right
> direction.

understood.

> We noticed a nice performance boost on the last one with many cores
> (24 threads, something like +40% on connection rate), but we'll probably
> see even better once the rest is addressed.

indeed, I remember we spoke about those improvments at the last meetup.
nice work, 1.9 looks already interesting from this point of view!


Cheers,

-- 
William

Re: segfault in haproxy=1.8.4

[Willy Tarreau]

Hi William,

On Mon, Mar 19, 2018 at 06:57:50PM +0100, William Dauchy wrote:
> > However, be careful. This new implementation should be thread-safe
> > (hopefully...). But it is not optimal and in some situations, it could be 
> > really
> > slower in multi-threaded mode than in single-threaded one. The problem is 
> > that,
> > when we try to dequeue pending connections, we process it from the older 
> > one to
> > the newer one independently to the thread's affinity. So we need to wait the
> > other threads' wakeup to really process them. If threads are blocked in the
> > poller, this will add a significant latency. This problem happens when 
> > maxconn
> > values are very low.
> 
> Regarding this last section, we are a bit worried about the usability
> of the new `nbthread` feature in 1.8. It raised a few question on our
> side:
> - Is it considered as an experimental feature?

Threading was clearly released with an experimental status, just like
H2, because we knew we'd be facing some post-release issues in these
two areas that are hard to get 100% right at once. However I consider
that the situation has got much better, and to confirm this, both of
these are now enabled by default in HapTech's products. With this said,
I expect that over time we'll continue to see a few bugs, but not more
than what we're seeing in various areas. For example, we didn't get a
single issue on haproxy.org since it was updated to the 1.8.1 or so,
3 months ago. So this is getting quite good.

> - Should we expect potential latencies side effects in some situations
> as described in your commit message? (and so avoid to use it for low
> latency usage)

I ran a stress test on this patch, with a single server running with
"maxconn 1", with a frontend bound to two threads. I measure exactly
3 conn/s with a single thread (keep in mind that there's a single
connection at once), and 28500 with two threads. Thus the sync point
takes on average an extra 1.75 microsecond, compared to the 35
microseconds it takes on average to finish processing the request
(connect, server processing, response, close).

Also if you're running with nbproc > 1 instead, the maxconn setting is
not really respected since it becomes per-process. When you run with
8 processes it doesn't mean much anymore, or you need to have small
maxconn settings, implying that sometimes a process might queue some
requests while there are available slots in other processes. Thus I'd
argue that the threads here significantly improve the situation by
allowing all connection slots to be used by all CPUs, which is a real
improvement which should theorically show you lower latencies.

Note that if this is of interest to you, it's trivial to make haproxy
run in busy polling mode, and in this case the performance increases to
30900 conn/s, at the expense of eating all your CPU (which possibly you
don't care about if the latency is your worst ennemy). We can possibly
even improve this to ensure that it's done only when there are existing
sessions on a given thread. Let me know if this is something that could
be of interest to you, as I think we could make this configurable and
bypass the sync point in this case.

> - I saw some commits for 1.9 release which probably improves the
> situation about lockess threading
> http://git.haproxy.org/?p=haproxy.git;a=commit;h=cf975d46bca2515056a4f55e55fedbbc7b4eda59
> http://git.haproxy.org/?p=haproxy.git;a=commit;h=4815c8cbfe7817939bcac7adc18fd9f86993e4fc
> But I guess they will not be backported for 1.8, right?

No they're definitely not for 1.8 and still really touchy. We're
progressively attacking locks wherever we can. Some further patches
will refine the scheduler to make it more parallel, and even the code
above will continue to change, see it as a first step in the right
direction.

We noticed a nice performance boost on the last one with many cores
(24 threads, something like +40% on connection rate), but we'll probably
see even better once the rest is addressed.

Cheers,
Willy

Re: segfault in haproxy=1.8.4

[William Dauchy]

Hi Christopher,

On Thu, Mar 15, 2018 at 04:05:04PM +0100, Christopher Faulet wrote:
> From 91b1349b6a1a64d43cc41e8546ff1d1ce17a8e14 Mon Sep 17 00:00:00 2001
> From: Christopher Faulet 
> Date: Wed, 14 Mar 2018 16:18:06 +0100
> Subject: [PATCH] BUG/MAJOR: threads/queue: Fix thread-safety issues on the
>  queues management

[...]

> However, be careful. This new implementation should be thread-safe
> (hopefully...). But it is not optimal and in some situations, it could be 
> really
> slower in multi-threaded mode than in single-threaded one. The problem is 
> that,
> when we try to dequeue pending connections, we process it from the older one 
> to
> the newer one independently to the thread's affinity. So we need to wait the
> other threads' wakeup to really process them. If threads are blocked in the
> poller, this will add a significant latency. This problem happens when maxconn
> values are very low.

Regarding this last section, we are a bit worried about the usability
of the new `nbthread` feature in 1.8. It raised a few question on our
side:
- Is it considered as an experimental feature?
- Should we expect potential latencies side effects in some situations
as described in your commit message? (and so avoid to use it for low
latency usage)
- I saw some commits for 1.9 release which probably improves the
situation about lockess threading
http://git.haproxy.org/?p=haproxy.git;a=commit;h=cf975d46bca2515056a4f55e55fedbbc7b4eda59
http://git.haproxy.org/?p=haproxy.git;a=commit;h=4815c8cbfe7817939bcac7adc18fd9f86993e4fc
But I guess they will not be backported for 1.8, right?

Best,
-- 
William

strange cppcheck finding

[Илья Шипицин]

(it's master)

is it in purpose ?

[src/ssl_sock.c:1553]: (warning) Invalid test for overflow
'msg+rec_len

Re: [Patch] minor bugfix

[Willy Tarreau]

On Sun, Feb 25, 2018 at 09:48:21PM +0100, Thierry Fournier wrote:
> Hi,
> 
> This is a lot of minor patches.
> 
>  * The 0001 should be backported in 1.7. The backport is done in the
>patch v17_0001.
> 
>  * The 0002 should be backported in 1.7 and 1.6
> 
>  * The 0003 could be backported in 1.6 and 1.7 but it is very minor and
>I think that is useless to backport it.

Thanks Thierry, now applied.

Willy

Re: [PATCH] Feature for substituting header matches

[Willy Tarreau]

Hi Darren,

On Fri, Feb 23, 2018 at 05:29:47PM +, Darren Demicoli wrote:
> Hi all
> 
> Recently I needed to substitute http headers in requests such as the 
> following:
> 
> From
> Referer: https://a-b-c.example.com/x-caliber
> To
> Referer: https://a.b.c.example.com/x-caliber
> 
> Basically any hyphen in the subdomain part of example.com needed to be
> replaced by dots, but any other hyphens should not be replaced. I tried doing
> it with regsub() but the limitation on the accepted characters (no
> parenthesis or square brackets) limits the possible regular expressions that
> can be used. Also "http-request replace-header" is limited because it does
> not handle multiple matches - if there is more than one match there is no way
> of back-referencing to all matches (so varying number of matches cannot be
> handled). What I needed was a cross between replace-header and regsub. So I
> am suggesting the substitute-header and substitute value actions for http
> requests and responses.

Well, given that it's very similar to the replace-header with different
semantics and should be able to be addressed properly using regsub(), I'd
rather work on finding a way to express all characters in regsub(). This
problem remains present with every other use cases and we must not start
to multiply the http actions just to workaround some parser limitations.

I'd like to see the config parser rewritten to be recursive instead of
left-right, but a lot of parts will have to be modified at the same time
and it will not be done quickly. But maybe we can find an acceptable
short-term workaround consisting in finding how to escape certain such
characters (basically, the comma, the quotes, parenthesis, braces and
brackets). I don't have much to propose for now but I'd rather see the
efforts focused on doing this you see.

Thanks!
Willy

Re: Fix building without NPN support

[Willy Tarreau]

Hi guys,

On Sun, Feb 18, 2018 at 11:58:41AM +0300, Dmitry Sivachenko wrote:
(...)
> > Agree. Updated patch attached.
> > 
> > Bernard.
> 
> 
> Is this patch good, Lukas?
> Any plans to integrate it?

Just noticed this one now and took it. I also backported it to 1.8.
Thanks!

Willy

Re: [PATCH] support CRC32c for proxy protocol v2 (send, accept)

[Willy Tarreau]

Hi Manu,

On Mon, Feb 05, 2018 at 05:10:05PM +0100, Emmanuel Hocdet wrote:
> Hi,
> 
> Series of patches to support CRC32c checksum to proxy protocol v2 header
> (as describe in "doc/proxy-protocol.txt »)
> . add hash_crc32c function
> . add « crc32c » option to proxy-v2-options
> . check crc32c checksum when CRC32C tlv is received.

Thanks. I'm having a few comments here :

> From a62ca59e175631a0adbb9b8adf1d58492f6bce5a Mon Sep 17 00:00:00 2001
> From: Emmanuel Hocdet 
> Date: Mon, 5 Feb 2018 15:26:43 +0100
> Subject: [PATCH 2/3] MINOR: proxy-v2-options: add crc32c
> 
> This patch add option crc32c (PP2_TYPE_CRC32C) to proxy protocol v2.
> It compute the checksum of proxy protocol v2 header as describe in
> "doc/proxy-protocol.txt".
> (...)
> diff --git a/include/types/server.h b/include/types/server.h
> index 6d0566be2..e68b2036b 100644
> --- a/include/types/server.h
> +++ b/include/types/server.h
> @@ -195,7 +196,7 @@ struct server {
>   enum obj_type obj_type; /* object type == 
> OBJ_TYPE_SERVER */
>   enum srv_state next_state, cur_state;   /* server state among SRV_ST_* 
> */
>   enum srv_admin next_admin, cur_admin;   /* server maintenance status : 
> SRV_ADMF_* */
> - unsigned char pp_opts;  /* proxy protocol options 
> (SRV_PP_*) */
> + unsigned int pp_opts;   /* proxy protocol options 
> (SRV_PP_*) */
>   struct server *next;

This is going to punch a 8-bit hole before the struct member. While not
dramatic for such a struct, it's a good practice to place a comment before
indicating that 8 bits are available there.

> diff --git a/src/connection.c b/src/connection.c
> index 412fe3f41..833763e4c 100644
> --- a/src/connection.c
> +++ b/src/connection.c
> @@ -990,6 +991,7 @@ static int make_tlv(char *dest, int dest_len, char type, 
> uint16_t length, const
>  int make_proxy_line_v2(char *buf, int buf_len, struct server *srv, struct 
> connection *remote)
>  {
>   const char pp2_signature[] = PP2_SIGNATURE;
> +uint32_t *tlv_crc32c_p = NULL;
>   int ret = 0;

Spaces instead of tab above, lazy copy-paste ? :-)

> @@ -1037,6 +1039,14 @@ int make_proxy_line_v2(char *buf, int buf_len, struct 
> server *srv, struct connec
>   ret = PP2_HDR_LEN_UNSPEC;
>   }
>  
> + if (srv->pp_opts & SRV_PP_V2_CRC32C) {
> + uint32_t zero_crc32c = 0;
> + if ((buf_len - ret) < sizeof(struct tlv))
> + return 0;
> + tlv_crc32c_p = (uint32_t *)((struct tlv *)[ret])->value;
>
> + ret += make_tlv([ret], (buf_len - ret), PP2_TYPE_CRC32C, 
> sizeof(zero_crc32c), (const char *)_crc32c);
> + }
> +
>   if (conn_get_alpn(remote, , _len)) {
>   if ((buf_len - ret) < sizeof(struct tlv))
>   return 0;
> @@ -1115,6 +1125,10 @@ int make_proxy_line_v2(char *buf, int buf_len, struct 
> server *srv, struct connec
>  
>   hdr->len = htons((uint16_t)(ret - PP2_HEADER_LEN));
>  
> + if (tlv_crc32c_p) {
> + *tlv_crc32c_p = htonl(hash_crc32c(buf, ret));

How can you be sure the pointer is 32-bit aligned there ? I don't think
we have anything guaranteeing it. Please take a look at write_u32() in
net_helper.h, it's provided exactly for this, and does exactly the same
thing on architectures supporting unaligned accesses (x86, armv7/8). And
in order to avoid any accidental misuse you can then declare tlv_crc32c_p
as a void*.

> From 4a22d7ca1d4f711b0d777095e5ce793e04a93c78 Mon Sep 17 00:00:00 2001
> From: Emmanuel Hocdet 
> Date: Mon, 5 Feb 2018 16:23:23 +0100
> Subject: [PATCH 3/3] MINOR: accept-proxy: support proxy protocol v2 CRC32c
>  checksum
> 
> When proxy protocol v2 CRC32c tlv is received, check it before accept
> connection (as describe in "doc/proxy-protocol.txt").
> ---
>  src/connection.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/connection.c b/src/connection.c
> index 833763e4c..8fd3209d5 100644
> --- a/src/connection.c
> +++ b/src/connection.c
> @@ -612,6 +612,13 @@ int conn_recv_proxy(struct connection *conn, int flag)
>   tlv_offset += tlv_len + TLV_HEADER_SIZE;
>  
>   switch (tlv_packet->type) {
> + case PP2_TYPE_CRC32C: {
> + uint32_t n_crc32c = ntohl(*(uint32_t 
> *)tlv_packet->value);
> + *(uint32_t *)tlv_packet->value = 0;

Same here, please see read_u32() and write_u32().

Otherwise looks good.

Thanks!
Willy

Re: [PATCH] BUG/MINOR: cli: Fix a crash when sending a command with too many arguments

[Willy Tarreau]

On Fri, Mar 16, 2018 at 11:02:33AM +0100, Aurélien Nephtali wrote:
> Hello,
> 
> This patch fixes a crash when the CLI is fed with too many arguments:
> 
> $ seq -s ' ' 0 64 | socat /tmp/sock1 -

Wow, nice one, thanks!

Willy

Re: [PATCH] BUG/MINOR: cli: Ensure all command outputs end with a LF

[Willy Tarreau]

Hi Aurélien,

On Thu, Mar 15, 2018 at 10:27:31PM +0100, Aurélien Nephtali wrote:
> Hello,
> 
> This patch adds some missing LF to the outputs of some commands.
> It may break some scripts that rely on these broken outputs but it also
> breaks the parsing of pipelined commands.

Good catch! In fact I don't think it will break any script, since these
scripts are supposed to either read an empty line to find the end of the
output, or to simply ignore the output. So I suspect it should even be
safe for backporting. But for now I've merged it into 1.9 only. Let's
not take it yet for 1.8 until someone requests it.

Thanks,
Willy

Re: [MINOR][PATCH] Fix segfault when trying to use seemless reload with at least an interface bound

[Willy Tarreau]

On Thu, Mar 15, 2018 at 05:54:09PM +0100, Olivier Houchard wrote:
> 
> Hi,
> 
> Trying to do a seemless reload while at least one socket has been bound to
> a specifig interface will lead to a segfault, because the guy who wrote that
> code did it by copying/pasting, and forgot to change an instance of
> "namespace" to "iface".
> The attached patch should fix it.

Applied, thanks!

willy

Re: small cleanup of src/dns.c

[Willy Tarreau]

Hi,

On Thu, Mar 15, 2018 at 04:43:29PM +0500,  ??? wrote:
> Hello,
> 
> small issue (no real impact) identified by cppcheck

> From 733d99f42d93898232bb8c3c953b662ee889c034 Mon Sep 17 00:00:00 2001
> From: Ilya Shipitsin 
> Date: Thu, 15 Mar 2018 16:38:38 +0500
> Subject: [PATCH] CLEANUP: remove duplicate code in src/dns.c
> 
> issue was identified by cppcheck
> 
> [src/dns.c:2037] -> [src/dns.c:2041]: (warning) Variable 'appctx->st2' is 
> reassigned a value before the old one has been used. 'break;' missing?
> ---
>  src/dns.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/src/dns.c b/src/dns.c
> index 280bc155..c49a4c6d 100644
> --- a/src/dns.c
> +++ b/src/dns.c
> @@ -2034,9 +2034,6 @@ static int 
> cli_io_handler_dump_resolvers_to_buffer(struct appctx *appctx)
>   return 0;
>   }
>  
> - appctx->st2 = STAT_ST_FIN;
> - /* fall through */
> -

Thanks, however I'll keep the comment, because the most recent gcc versions
now have implemented a shitty feature consisting in reading your comments
and emitting warnings if they don't find such a comment in a "case"
statement missing a break! Given that these people don't use C anymore,
they probably feel like it's safe to break it for everyone else. So I'll
take your patch and adapt it a bit.

Thanks,
Willy

>   default:
>   appctx->st2 = STAT_ST_FIN;
>   return 1;
> -- 
> 2.14.3
> 

Re: cppcheck finding

[Willy Tarreau]

Hi,

On Thu, Mar 15, 2018 at 04:27:43PM +0500,  ??? wrote:
> [src/51d.c:373]: (error) Invalid number of character '{' when no macros are
> defined.

Just a small hint, please always mention which version (or ideally commit)
you report issues like this.

>From what I'm seeing, the program is complaining that the code will
not build if neither FIFTYONEDEGREES_H_PATTERN_INCLUDED nor
FIFTYONEDEGREES_H_TRIE_INCLUDED is set. I think we necessarily have one
or the other so it might be a non-issue. I tend to think that this code
could be slightly refactored to limit the impact of these #ifdef, but
to be honest, I'm not sure we'd get any benefit from this.

Thanks,
Willy

Re: New 1.8 release ?

[Willy Tarreau]

Hi Olivier,

On Wed, Mar 14, 2018 at 05:55:33PM +0100, Olivier Doucet wrote:
> Hello all,
> 
> I can see several fixes in 1.8 trunk about 100% CPU usage and some other
> bugs.
> Last release was more than a month ago. Is there a new release expected
> soon ? I'm about to start using 1.8 on some production boxes and wanted to
> start on a fresh new release with all known bugs fixed :)

You're totally right. We've been thinking about issuing 1.8.5 for a while
now, and the pain caused by working on the latest bugs has been growing,
diverting us from such a release.

I'm focusing on 1.8.5 for this week now. I still don't understand some
of the reported crashes (h2, cache). There's one issue that I have to
fix on h2 regarding missing send timeouts (the test patch has been
running for a while now). And I still need to check some of the flow
control bugs reported by klzgrad some time ago now. These ones might
be addressed after 1.8.5 if needed.

Cheers,
Willy

Re: add header into http-request redirect

[Willy Tarreau]

Hi Tim,

On Tue, Mar 13, 2018 at 12:37:44AM +0100, Tim Düsterhus wrote:
> Willy,
> 
> I'd like to bring this issue to your attention again, possibly you are
> able to find a solution for haproxy 1.9?

I hope so, but we'll need to be sure that someone is assigned to this,
otherwise I'll keep being busy with other stuff.

> This issue prevents me from submitting one domain to the HSTS preload
> list, as I need to perform a redirect on the zone's apex and that
> redirect does not include the HSTS header.

I *suspect* that in the end we could simply add a series of "header"
statements to the redirect rules. These ones would be followed by a
log-format expression making it possible to send various elements in
these response. But if it's mainly needed for HSTS, probably that in
the end we could be fine with (at least initially) adding a single
"header" directive. We'd then have :

  http-request redirect location "/foo" header x-my-header "my-expression"

Just my two cents,
Willy

Re: Need help?

[Willy Tarreau]

Hi Nikhil,

On Sat, Mar 17, 2018 at 05:39:29PM +, Nikhil Kapoor wrote:
> Actually, I just wanted to deeply understand the code of haproxy. So just
> wanted to know which tool should i use in order to understand the code. Is it
> only gdb that you all use or any other?

Well, gdb is not suited to read code. You'd rather need an editor or
an IDE to help you follow the code. Gdb can serve to place break points
at certain locations however.

What I could suggest you is to take a look at these places particularly :
  - run_poll_loop() : this one is the main loop, iterating between polling,
processing I/O and processing tasks ;

  - listener_accept() : it's where an incoming connection is accepted

  - session_new() : it's called some time after listener_accept(), and
creates a new session ;

  - stream_new() : it creates a fresh new stream on a session ;

  - process_stream() : it calls all analysers subscribed to a stream, and
deals with timeouts, events etc... It's where the main tcp/http stuff
happens ; 

  - cfg_parse_listen() : it's where most of the config keywords are still
parsed (many of them have moved to other locations now). This will
help you figure how config elements are allocated and initialized,
how certain callbacks or analysers are enabled and why/when ;

  - check_config_validity() : some validity checks are run on the config
there late in the boot process, some config elements are resolved,
and some default values are assigned. I think it's also where we
assign an ID to the backends and servers if they don't have one yet.
It will definitely help you understand the relations between various
elements.

Before this, take a look inside doc/internals, and particularly the file
"entities.pdf" which shows how a stream is attached to objects surrounding
it.

Hoping this helps,
Willy

Re: Can HA-Proxy set an header when he "breaks" stick routing

[Willy Tarreau]

Hi,

On Fri, Mar 16, 2018 at 12:31:47PM +, Gisle Grimen wrote:
> Hi,
> 
> We are using HA-Proxy with sticky routing in front of our cluster. Is there a
> way to get HA-Proxy to add or set an header on a forwarded request when
> HA-Proxy "breaks" sticky routing i.e. when forwarding the request to another
> server then the one indicated in the sticky table?

No, there is no such thing. You have this information in the logs however.
The difficulty lies with adding some information late in the LB+connection
process, as they happen after headers are processed. There is one exception
to this, "option http-send-name-header", which is able to rewind the stream
and insert a server name after the LB is performed, and it has been causing
tons of bugs alone for more than two years because it's very tricky.

I think that it would not be very difficult to implement something adding
a header containing the ID of the initial server that the stickiness was
expecting however. This way it could allow your servers to see that the
initial name is not the one they expected and deduce the stickiness is
broken. I don't know if that could suit your needs, nor if anyone would
be willing to work on this (maybe you would ?).

Regards,
Willy

Re: New HTTP action: DNS resolution at run time

[Willy Tarreau]

Hi Jonathan,

On Sat, Mar 17, 2018 at 02:46:20PM +, Jonathan Matthews wrote:
> On 30 January 2018 at 09:04, Baptiste  wrote:
> > Hi all,
> >
> > Please find enclosed a few patches which adds a new HTTP action into
> > HAProxy: do-resolve.
> > This action can be used to perform DNS resolution based on information found
> > in the client request and the result is stored in an HAProxy variable (to
> > discover the IP address of the server on the fly or logging purpose,
> > etc...).
> 
> Hello folks,
> 
> Did this feature ever go anywhere?
> 
> I'm trying to write some ACLs matching X-Forwarded-For headers against
> a DNS record, and I *think* this set of patches is my only way to
> achieve this, without using an external lookup process to modify ACLs
> via the admin socket ...

Not lost, but not merged yet by lack of time to review them, spending
time trying to figure some painful bugs :-(

However as with any feature patch, your feedback on these ones would
quite welcome!

Thanks!
Willy