Re: [PATCH] BUG: spoe: Fix parsing of SPOE actions in ACK frames

[Willy Tarreau]

On Thu, Nov 24, 2016 at 09:00:51PM +0100, Christopher Faulet wrote:
> New patch attached. thanks.

Applied now, thanks!

Willy

Re: Backend: Multiple A records

[Willy Tarreau]

Hi Tim,

On Fri, Nov 25, 2016 at 02:34:49AM +0100, Tim Düsterhus wrote:
> Hi
> 
> On 28.08.2016 19:57, Baptiste wrote:
> > This should happen soon, for 1.7.
> 
> I noticed Willy's email ???1.7 => almost there??? and wanted to test out the
> feature. Has this feature been implemented for 1.7?

No, unfortunately none of us had the time to complete this. It's sad
but true. And I definitely refuse to reproduce the 1.5 model where
we wait for a certain feature to release and where it takes 4.5
years to produce the expected 6-months release. So this will have to
wait for 1.8 or later, until someone has time to complete this feature.

At least right now you can update the IP addresses from the CLI, so
you could very well run a script iterating over the output of a
"host" command to feed it. It's not as magical but will work.

Regards,
Willy

Re: [PATCH] allow higer averages then 16448ms

[Willy Tarreau]

Hi Reinhard,

On Thu, Nov 24, 2016 at 10:04:31PM +0100, Reinhard Vicinus wrote:
> Hi,
> 
> we use haproxy (1.6.9) to balance very long running POST requests
> (around 50 seconds) to backend servers. It generally works like a charm,
> but the average queue time and average total session time statistic
> values are totally screwed up.
> 
> The problem is that the average is calculated like this for every request:
> 
> sum = sum * 511 / 512 + value
> 
> for a fixed value and enough iterations:
> 
> sum = value * 511
> 
> the problem is that at every iteration sum will first be multiplied by
> 511 and therefore the maximum value during the calculation is:
> 
> value * 511 * 511
>
> A unsigned int can store a maximum value of 4294967296. Divided by
> 511*511 results in 16448. That means any backend with average times
> above 16448ms will be affected by integer overflow and have wrong values.

Yes we do know this limitation.

> The attached patch tries to solve this by storing and calculating sum as
> unsigned long long instead of a unsigned int. I don't know if the
> attached patch will work in every case, but during my limited testing it
> worked.

It will definitely work, but I didn't want to do it because of the
very expensive cost of the 64x64 multiply and divide on 32 bit
platforms which causes a measurable performance impact. However I'll
do some tests because is often OK, and doing 32x32/32 with a 64-bit
intermediary result is OK as well. If I can do it this way, I'll do
it. Otherwise I'd prefer that we just switch do long so that 64-bit
platforms can benefit from the large timers and 32-bit ones are
limited to lower values.

Thanks,
Willy

Re: Backend: Multiple A records

[Tim Düsterhus]

Hi

On 28.08.2016 19:57, Baptiste wrote:
> This should happen soon, for 1.7.

I noticed Willy's email “1.7 => almost there” and wanted to test out the
feature. Has this feature been implemented for 1.7? I noticed the
init-addr feature mentioned in the dev6 release announcement and
modified the configuration in my initial mail to the following:

server nginx1 nginx.containers.example.com:80 check resolvers
containers resolve-prefer ipv4 init-addr last,libc,none
server nginx2 nginx.containers.example.com:80 check resolvers
containers resolve-prefer ipv4 init-addr last,libc,none
server nginx3 nginx.containers.example.com:80 check resolvers
containers resolve-prefer ipv4 init-addr last,libc,none
server nginx4 nginx.containers.example.com:80 check resolvers
containers resolve-prefer ipv4 init-addr last,libc,none

haproxy started up without any A records at
nginx.containers.example.com, great! It also brought the backends down
once the record disappeared, also great!

Unfortunately it still sent the requests to a single nginx backend only,
instead of using all the available IP addresses.

Did I configure something wrong?

Best regards
Tim Düsterhus

Re: [PATCH] MINOR: session: close HTTP keepalived connections when haproxy is stopping

[Willy Tarreau]

Hi Cyril,

On Thu, Nov 24, 2016 at 11:00:42PM +0100, Cyril Bonté wrote:
> Hi Willy,
> 
> This is an old patch I've never send, so I do it right now to open the
> discussion, it' not necessarily to merge it immediately in the 1.7.0 release
> ;-)

Ah OK thanks for the precision :-)

Just out of curiosity, have you checked if it was not sufficient to list
all idle sessions ? I *think* all keep-alive sessions are now in the
idle queue, I should recheck. And you can even decide which ones to
keep or kill, as if they're in the private queue they must not be shared
because of 401/407. But again I could be missing something big.

Cheers,
Willy

Re: [PATCH] MINOR: session: close HTTP keepalived connections when haproxy is stopping

[Cyril Bonté]

Hi Willy,

This is an old patch I've never send, so I do it right now to open the 
discussion, it' not necessarily to merge it immediately in the 1.7.0 
release ;-)



Le 24/11/2016 à 22:54, Cyril Bonté a écrit :

During a reload, the old process is still alive until all connections are
closed. There is a chance that the connections are HTTP keepalived ones, which
may not be reused. Hence, those connections will remain alive until a timeout
expires.
It is possible to detect such connections and immediately close them. An
exception is made when the previous HTTP status code is 401 or 407 in order
to leave NTLM sessions as is.
---
 src/proxy.c | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/src/proxy.c b/src/proxy.c
index dc6d3e1..0207642 100644
--- a/src/proxy.c
+++ b/src/proxy.c
@@ -983,6 +983,28 @@ int pause_proxy(struct proxy *p)
 }


+void sessions_cleanup(struct proxy *p)
+{
+   struct stream *s;
+
+   if (!(p->cap & PR_CAP_FE))
+   return;
+
+   list_for_each_entry(s, , list) {
+   struct session *sess = s->sess;
+
+   if (sess && sess->fe == p) {
+   struct http_txn *txn = s->txn;
+
+   if (txn) {
+   int prev_status = txn->status;
+   if (txn->flags & TX_WAIT_NEXT_RQ && 
!(prev_status == 401 || prev_status == 407))
+   stream_shutdown(s, SF_ERR_KILLED);
+   }
+   }
+   }
+}
+
 /*
  * This function completely stops a proxy and releases its listeners. It has
  * to be called when going down in order to release the ports so that another
@@ -1002,6 +1024,7 @@ void stop_proxy(struct proxy *p)
jobs--;
}
}
+   sessions_cleanup(p);
p->state = PR_STSTOPPED;
 }





--
Cyril Bonté

[PATCH] MINOR: session: close HTTP keepalived connections when haproxy is stopping

[Cyril Bonté]

During a reload, the old process is still alive until all connections are
closed. There is a chance that the connections are HTTP keepalived ones, which
may not be reused. Hence, those connections will remain alive until a timeout
expires.
It is possible to detect such connections and immediately close them. An
exception is made when the previous HTTP status code is 401 or 407 in order
to leave NTLM sessions as is.
---
 src/proxy.c | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/src/proxy.c b/src/proxy.c
index dc6d3e1..0207642 100644
--- a/src/proxy.c
+++ b/src/proxy.c
@@ -983,6 +983,28 @@ int pause_proxy(struct proxy *p)
 }
 
 
+void sessions_cleanup(struct proxy *p)
+{
+   struct stream *s;
+
+   if (!(p->cap & PR_CAP_FE))
+   return;
+
+   list_for_each_entry(s, , list) {
+   struct session *sess = s->sess;
+
+   if (sess && sess->fe == p) {
+   struct http_txn *txn = s->txn;
+
+   if (txn) {
+   int prev_status = txn->status;
+   if (txn->flags & TX_WAIT_NEXT_RQ && 
!(prev_status == 401 || prev_status == 407))
+   stream_shutdown(s, SF_ERR_KILLED);
+   }
+   }
+   }
+}
+
 /*
  * This function completely stops a proxy and releases its listeners. It has
  * to be called when going down in order to release the ports so that another
@@ -1002,6 +1024,7 @@ void stop_proxy(struct proxy *p)
jobs--;
}
}
+   sessions_cleanup(p);
p->state = PR_STSTOPPED;
 }
 
-- 
2.10.2

[PATCH] allow higer averages then 16448ms

[Reinhard Vicinus]

Hi,

we use haproxy (1.6.9) to balance very long running POST requests
(around 50 seconds) to backend servers. It generally works like a charm,
but the average queue time and average total session time statistic
values are totally screwed up.

The problem is that the average is calculated like this for every request:

sum = sum * 511 / 512 + value

for a fixed value and enough iterations:

sum = value * 511

the problem is that at every iteration sum will first be multiplied by
511 and therefore the maximum value during the calculation is:

value * 511 * 511

A unsigned int can store a maximum value of 4294967296. Divided by
511*511 results in 16448. That means any backend with average times
above 16448ms will be affected by integer overflow and have wrong values.

The attached patch tries to solve this by storing and calculating sum as
unsigned long long instead of a unsigned int. I don't know if the
attached patch will work in every case, but during my limited testing it
worked. I'm also not sure if the implicit conversion from unsigned long
long to unsigned int in swrate_avg is a good idea. It should never be a
problem, because value is an unsigned int and the average can never be
higher then the maximum value. But I can understand if there are coding
conventions against implicit conversions. Generally I don't have a lot
of c experience, so feel free to improve the patch or solve the problem
another way.

Kind regards
Reinhard
From b298b230b541bb33d1cbea92f5472c926e10b7f0 Mon Sep 17 00:00:00 2001
From: "Vicinus, Reinhard" 
Date: Thu, 24 Nov 2016 21:41:17 +0100
Subject: [PATCH] allow higer averages then 16448ms

---
 include/proto/freq_ctr.h | 4 ++--
 include/types/counters.h | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/proto/freq_ctr.h b/include/proto/freq_ctr.h
index 65388b1..099a58d 100644
--- a/include/proto/freq_ctr.h
+++ b/include/proto/freq_ctr.h
@@ -218,7 +218,7 @@ unsigned int freq_ctr_remain_period(struct freq_ctr_period *ctr, unsigned int pe
 /* Adds sample value  to sliding window sum  configured for  samples.
  * The sample is returned. Better if  is a power of two.
  */
-static inline unsigned int swrate_add(unsigned int *sum, unsigned int n, unsigned int v)
+static inline unsigned long long swrate_add(unsigned long long *sum, unsigned int n, unsigned int v)
 {
 	return *sum = *sum * (n - 1) / n + v;
 }
@@ -227,7 +227,7 @@ static inline unsigned int swrate_add(unsigned int *sum, unsigned int n, unsigne
  *  samples. Better if  is a power of two. It must be the same  as the
  * one used above in all additions.
  */
-static inline unsigned int swrate_avg(unsigned int sum, unsigned int n)
+static inline unsigned int swrate_avg(unsigned long long sum, unsigned int n)
 {
 	return (sum + n - 1) / n;
 }
diff --git a/include/types/counters.h b/include/types/counters.h
index 3e62763..08bcee7 100644
--- a/include/types/counters.h
+++ b/include/types/counters.h
@@ -56,7 +56,7 @@ struct pxcounters {
 	long long redispatches; /* retried and redispatched connections (BE only) */
 	long long intercepted_req;  /* number of monitoring or stats requests intercepted by the frontend */
 
-	unsigned int q_time, c_time, d_time, t_time; /* sums of conn_time, queue_time, data_time, total_time */
+	unsigned long long q_time, c_time, d_time, t_time; /* sums of conn_time, queue_time, data_time, total_time */
 
 	union {
 		struct {
@@ -100,7 +100,7 @@ struct srvcounters {
 	long long retries, redispatches;	/* retried and redispatched connections */
 	long long failed_secu;			/* blocked responses because of security concerns */
 
-	unsigned int q_time, c_time, d_time, t_time; /* sums of conn_time, queue_time, data_time, total_time */
+	unsigned long long q_time, c_time, d_time, t_time; /* sums of conn_time, queue_time, data_time, total_time */
 
 	union {
 		struct {
-- 
2.10.2

Re: [PATCH] BUG: spoe: Fix parsing of SPOE actions in ACK frames

[Christopher Faulet]

Le 24/11/2016 à 19:50, Willy Tarreau a écrit :

Hi Christopher,

On Thu, Nov 24, 2016 at 03:06:13PM +0100, Christopher Faulet wrote:

>From 7ed3c2942d57ea2ddfc8973cce9cc1c94bca01da Mon Sep 17 00:00:00 2001
From: Christopher Faulet 
Date: Thu, 24 Nov 2016 14:53:22 +0100
Subject: [PATCH] BUG: spoe: Fix parsing of SPOE actions in ACK frames
X-Bogosity: Ham, tests=bogofilter, spamicity=0.00, version=1.2.4

For "SET-VAR" actions, data was not correctly parsed. 'idx' variable was not
correctly updated when the 3rd argument was parsed.


Are you sure your patch is correct ? I think it's bogus :


diff --git a/src/flt_spoe.c b/src/flt_spoe.c
index 0b722b6..8227140 100644
--- a/src/flt_spoe.c
+++ b/src/flt_spoe.c
@@ -2017,7 +2017,7 @@ process_spoe_actions(struct stream *s, struct 
spoe_context *ctx,
goto skip;
memset(, 0, sizeof(smp));
smp_set_owner(, s->be, s->sess, s, 
dir|SMP_OPT_FINAL);
-   if (decode_spoe_data(p+idx, p+size, ) == -1)
+   if ((idx += decode_spoe_data(p+idx, p+size, 
)) == -1)
goto skip;


The only case it will work is when idx = 0 before decoding, which doesn't
really look like the only case you're interested in. I guess you wanted to
do this instead :

-   if (decode_spoe_data(p+idx, p+size, ) == -1)
+   ret = decode_spoe_data(p+idx, p+size, );
+   if (ret == -1)
goto skip;
+   idx += ret;

Am I wrong ? That's the reason why I hate assignments in "if" conditions,
half of the time they are bogus, the other half they make the reader
scratch his head wondering if it's bogus or intended :-)



Ah of course you're right ! too tired...
New patch attached. thanks.

--
Christopher
>From 1b46491bf93c76602122ea5814c42fdcfbf2d816 Mon Sep 17 00:00:00 2001
From: Christopher Faulet 
Date: Thu, 24 Nov 2016 14:53:22 +0100
Subject: [PATCH] BUG: spoe: Fix parsing of SPOE actions in ACK frames
X-Bogosity: Ham, tests=bogofilter, spamicity=0.00, version=1.2.4

For "SET-VAR" actions, data was not correctly parsed. 'idx' variable was not
correctly updated when the 3rd argument was parsed.
---
 src/flt_spoe.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/flt_spoe.c b/src/flt_spoe.c
index 0b722b6..776848e 100644
--- a/src/flt_spoe.c
+++ b/src/flt_spoe.c
@@ -2017,8 +2017,10 @@ process_spoe_actions(struct stream *s, struct spoe_context *ctx,
 	goto skip;
 memset(, 0, sizeof(smp));
 smp_set_owner(, s->be, s->sess, s, dir|SMP_OPT_FINAL);
-if (decode_spoe_data(p+idx, p+size, ) == -1)
+
+if ((i = decode_spoe_data(p+idx, p+size, )) == -1)
 	goto skip;
+idx += i;
 
 SPOE_PRINTF(stderr, "%d.%06d [SPOE/%-15s] %s: stream=%p"
 	" - set-var '%s.%s.%.*s'\n",
-- 
2.7.4

1.7 => almost there

[Willy Tarreau]

Hi all,

I think we're OK now. I'll check tomorrow morning with Christopher regarding
this latest patch, and maybe another one from Thierry, but the most painful
cleanup was done since dev6 : mainly splitting dumpstats.c into cli+stats and
moving the other parts away (thanks William), and finally checking errors in
log-formats (thanks Thierry)). The easy things were still left not done given
that nobody stepped up to do them but that's not dramatic. We fixed quite a
bunch of remaining bugs, and openssl 0.9.8 works again.

For this reason I intend to release tomorrow if everything's OK.

Thus, if you're silently working on some harmless cleanups that you think
are better done before the release than after in order to simplify later
backports, please send them NOW even if they're not perfect, I can help
finish them.

I'm glad to see that 1.7 is much cleaner than 1.6 and that almost all the
bugs we found there were also in 1.6. In my opinion 1.7 is what 1.6 should
have been, and I suspect lots of people will upgrade (and we'll get new
bug reports ;-))

Cheers,
Willy

Re: [PATCH] BUG: spoe: Fix parsing of SPOE actions in ACK frames

[Willy Tarreau]

Hi Christopher,

On Thu, Nov 24, 2016 at 03:06:13PM +0100, Christopher Faulet wrote:
> >From 7ed3c2942d57ea2ddfc8973cce9cc1c94bca01da Mon Sep 17 00:00:00 2001
> From: Christopher Faulet 
> Date: Thu, 24 Nov 2016 14:53:22 +0100
> Subject: [PATCH] BUG: spoe: Fix parsing of SPOE actions in ACK frames
> X-Bogosity: Ham, tests=bogofilter, spamicity=0.00, version=1.2.4
> 
> For "SET-VAR" actions, data was not correctly parsed. 'idx' variable was not
> correctly updated when the 3rd argument was parsed.

Are you sure your patch is correct ? I think it's bogus :

> diff --git a/src/flt_spoe.c b/src/flt_spoe.c
> index 0b722b6..8227140 100644
> --- a/src/flt_spoe.c
> +++ b/src/flt_spoe.c
> @@ -2017,7 +2017,7 @@ process_spoe_actions(struct stream *s, struct 
> spoe_context *ctx,
>   goto skip;
>   memset(, 0, sizeof(smp));
>   smp_set_owner(, s->be, s->sess, s, 
> dir|SMP_OPT_FINAL);
> - if (decode_spoe_data(p+idx, p+size, ) == -1)
> + if ((idx += decode_spoe_data(p+idx, p+size, 
> )) == -1)
>   goto skip;

The only case it will work is when idx = 0 before decoding, which doesn't
really look like the only case you're interested in. I guess you wanted to
do this instead :

-   if (decode_spoe_data(p+idx, p+size, ) == -1)
+   ret = decode_spoe_data(p+idx, p+size, );
+   if (ret == -1)
goto skip;
+   idx += ret;

Am I wrong ? That's the reason why I hate assignments in "if" conditions,
half of the time they are bogus, the other half they make the reader
scratch his head wondering if it's bogus or intended :-)

Willy

[PATCH] BUG: spoe: Fix parsing of SPOE actions in ACK frames

[Christopher Faulet]

Hi,

Here is a small bug fix on SPOE filter.


--
Christopher
>From 7ed3c2942d57ea2ddfc8973cce9cc1c94bca01da Mon Sep 17 00:00:00 2001
From: Christopher Faulet 
Date: Thu, 24 Nov 2016 14:53:22 +0100
Subject: [PATCH] BUG: spoe: Fix parsing of SPOE actions in ACK frames
X-Bogosity: Ham, tests=bogofilter, spamicity=0.00, version=1.2.4

For "SET-VAR" actions, data was not correctly parsed. 'idx' variable was not
correctly updated when the 3rd argument was parsed.
---
 src/flt_spoe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/flt_spoe.c b/src/flt_spoe.c
index 0b722b6..8227140 100644
--- a/src/flt_spoe.c
+++ b/src/flt_spoe.c
@@ -2017,7 +2017,7 @@ process_spoe_actions(struct stream *s, struct spoe_context *ctx,
 	goto skip;
 memset(, 0, sizeof(smp));
 smp_set_owner(, s->be, s->sess, s, dir|SMP_OPT_FINAL);
-if (decode_spoe_data(p+idx, p+size, ) == -1)
+if ((idx += decode_spoe_data(p+idx, p+size, )) == -1)
 	goto skip;
 
 SPOE_PRINTF(stderr, "%d.%06d [SPOE/%-15s] %s: stream=%p"
-- 
2.7.4

assistance on haproxy

[Raj]

Hi Team,

I installed HAproxy on pfsense as a package and it works well. I have exchange 
behind it and phones sync no issues. I created a new vlan and installed 
Iredmail.
I was able to hit the web interface for roundcube and sogo with no issues but 
can`t get phones to sync from external the network. If I forward port 443 
direct from the firewall to the Iredmail server, the phone setup completes and 
the phone syncs well. I am lost on this one. Could anyone please assist.

Raj