Re: [PATCH] MEDIUM: cli: Add multi-line mode support

2018-04-17 Thread Willy Tarreau
Hi Aurélien,

On Tue, Apr 17, 2018 at 09:06:35PM +0200, Aurélien Nephtali wrote:
> Example:
> If this command is entered:
> set server bk/fe <<
> state ready
> 
> Three arguments and a payload will be tagged. This command requires (in
> some cases) seven arguments (including the keywords) so a call to
> cli_resync_args(ci, 7) will extract what can be found from the payload.
> Once a parser has done everything it can to ensure it has access to its
> arguments, it can call cli_split_args() to do the proper tokenization
> and modify the input string to insert null bytes.

I'm confused, I thought we agreed *not* to mix arguments and payload, like
in the map example I provided, but now you seem to imply that extra args
are looked up in the payload part, while here you seem to have stuck to
this design, and your "add map" example seems to do exactly the case which
makes it useless (ie no way to enter spaces in the value because of the
undesired tokenizing).

> For now three commands was modified to take advantage of the multi-line
> mode: "set ssl ocsp-response" and "add map"/"add acl".
> 
> The number of arguments has been increased to 8192 (from 64) and the
> array to hold the pointers gets its memory from a pool. I chose 8192
> since it's the maximum number of arguments possible with an input buffer
> of 16384 (default tune.bufsize) (16384 / 2 if all arguments are one-char
> words). Would computing the max value at runtime be better ?

That's exactly what scares me in fact. For me there's no reason to increase
the argument count, the parser should parse a command's argument and decide
that what follows '<<' is a raw payload (we can debate whether it starts on
the next line or immediately after '<<' or if it's only '<<\n' which starts
a payload but for not it's a detail). The command should then get the pointer
to that payload as a payload pointer and take care of its payload itself.

Then we can indeed improve ocsp-response, map/acl to support taking entries
either as arguments or as payload or both. In fact I think that there is no
need for doing something very different from what we see in shell already :
the command's args are argv[0..N] and the payload is in stdin. Then many
commands decide where to pick their data from

Also, think about it, if you start to look up arguments in the payload during
the parsing, you will never be able to extend the system to support payloads
larger than one buffer. For example one day someone may want to be able to
preload the cache with known objects loaded from another instance (eg: on
reload). We could then have :

  > add cache object example.com/foo/bar <<
  + [base64 contents spanning over multiple buffers]
  +

Above for me the arguments are only on the first line. The rest is made
of bytes. It's up to the command's payload parser to know that the \n\n
sequence will mark the end of the body.

Similarly in shell you can do :

  $ grep foo /tmp/blah

  $ grep -f- /tmp/blah < foo
  > EOF

Please just let me know if I missed anything.

By the way, please better split the patch in several parts :
  - one for the infrastructure changes (CLI parser in general and its
repercussions all over the code)
  - one per modified subsystem to benefit from it (eg: adding payload
support to ocsp should be one patch, adding payload support to map/acl
should be another one)

This makes it much easier for all involved parties to review the changes
in their respective areas. And when a regression is reported and bisect
ends up on such a patch, it's much easier to spot the affected area (eg:
you prefer to know that add map was broken by the patch adding payload
support to add map than that it was broken by the one introducing the
multiline support, possibly requiring to revert it for all commands at
once to work around the issue instead of just for this one).

Thanks,
Willy



Re: 1.8.7 http-tunnel doesn't seem to work? (but default http-keep-alive does)

2018-04-17 Thread Willy Tarreau
On Tue, Apr 17, 2018 at 11:47:18PM +0200, PiBa-NL wrote:
> Op 17-4-2018 om 17:46 schreef Willy Tarreau:
> > On Tue, Apr 17, 2018 at 04:33:07PM +0200, Olivier Houchard wrote:
> > > After talking with Willy, here is an updated patch that does that.
> > > That way, the day we'll want to use EV_ONESHOT, we'll be ready, and won't
> > > miss any event.
> > Now merged, thanks guys!
> > Willy
> 
> Thanks!
> Works as expected with current master.
> 
> Do i dare ask a estimate when 1.8.8 might be released? (oh just did. ;) )

Probably tomorrow evening, with another fix we're working on.

> O well, i guess ill run production with nokqueue for a little while, that
> works alright for me sofar.

Or keep running on 1.8.7 with this patch ;-)

Willy



Re: 1.8.7 http-tunnel doesn't seem to work? (but default http-keep-alive does)

2018-04-17 Thread PiBa-NL

Op 17-4-2018 om 17:46 schreef Willy Tarreau:

On Tue, Apr 17, 2018 at 04:33:07PM +0200, Olivier Houchard wrote:

After talking with Willy, here is an updated patch that does that.
That way, the day we'll want to use EV_ONESHOT, we'll be ready, and won't
miss any event.

Now merged, thanks guys!
Willy


Thanks!
Works as expected with current master.

Do i dare ask a estimate when 1.8.8 might be released? (oh just did. ;) )
O well, i guess ill run production with nokqueue for a little while, 
that works alright for me sofar.


Regards,
PiBa-NL (Pieter)




Re: Version 1.5.12, getting 502 when server check fails, but server is still working

2018-04-17 Thread Shawn Heisey
On 4/17/2018 2:54 PM, Lukas Tribus wrote:
>> Originally, the "hollywood" entry on the be-cdn-9000 backend (which you can
>> see at the config I linked above) had the backup keyword.  But what I
>> noticed happening was that when planet went down, it took about ten
>> additional seconds (no precise timing was done) for hollywood to go active,
>> and during that time, the client got "no server available" messages.
> 
> You said this is about a 502 error. But "no server available" is not a
> error that haproxy emits and 503 would be "Service Unavailable".
> I don't see any issue replaying this here locally with haproxy 1.5.12.

I have figured out how to resolve the difficulty I was encountering on
this thread, and I can now do service restarts with zero loss.  The
disable-on-404 config, combined with a change to the way the init script
does the restart, has fixed it.

I switched gears and that part about the 'backup' keyword is for a
different problem.  One I already emailed the list separately on and
haven't gotten a response.  Feel free to drop this thread and move to
the other one.  Here is the message on the list archive:

https://www.mail-archive.com/haproxy@formilux.org/msg29615.html

I apologize for any confusion.  The "no server available" is something I
saw in the output from curl when the primary server went down and the
backup had not yet been changed to Active.

I don't have a transcript of that ssh session, and right now I can't
take the steps to reproduce the error -- the servers are being used
heavily for something that can't be postponed, and reproducing the error
would cause some of those requests to fail.

Thanks,
Shawn



Re: Version 1.5.12, getting 502 when server check fails, but server is still working

2018-04-17 Thread Lukas Tribus
Hello Shawn,



On 17 April 2018 at 15:24, Shawn Heisey  wrote:
>>> I described that issue in a separate message to the
>>> list.  I do have a workaround to that issue -- I'm no longer using
>>> "backup" on any server entries for this service.
>>
>> Then I don't see how it can work for you. It's a bit confusing I'm afraid.
>
>
> Originally, the "hollywood" entry on the be-cdn-9000 backend (which you can
> see at the config I linked above) had the backup keyword.  But what I
> noticed happening was that when planet went down, it took about ten
> additional seconds (no precise timing was done) for hollywood to go active,
> and during that time, the client got "no server available" messages.

You said this is about a 502 error. But "no server available" is not a
error that haproxy emits and 503 would be "Service Unavailable".
I don't see any issue replaying this here locally with haproxy 1.5.12.

Can you clarify what error you are seeing exactly and  also the
relevant haproxy logs (including server down message).



Lukas



Re: [PATCH] MEDIUM: cli: Add multi-line mode support

2018-04-17 Thread Aurélien Nephtali

Hello,

Here is another attempt to add multi-lines support to the CLI.

To enter a multi-lines command, a special pattern (<<) has to be used at
the end of the first line. Once in this mode, everything else is
gathered as it and the command is considered terminated on the reception
of an empty line. The command is then parsed and passed to a parser if a
valid keyword is found. This mode is not persistent across commands.

Technical part:
---

Parsing consists of "tagging" the position of each word BEFORE the
multi-lines pattern or until the end of the buffer if there is only one
line. This way, the input buffer is untouched and it is the parser
responsabilty to decide if it needs tokenization of its arguments. To do
this, there are two new functions: cli_resync_args() and
cli_split_args().

The main difficulty is that the parser does not know how many arguments
takes a command and if it accepts a payload (what is found after the
multi-lines pattern), that is why it only tags the input. If a parser
needs N arguments (mandatory or optional) it can call
cli_resync_args(ci, N) to try to get them from the payload. If there is
no payload (or not enough data to reach N args) the missing slots are
filled with the empty string). The list of arguments always ends with a
pointer on an empty string.

Example:
If this command is entered:
set server bk/fe <<
state ready

Three arguments and a payload will be tagged. This command requires (in
some cases) seven arguments (including the keywords) so a call to
cli_resync_args(ci, 7) will extract what can be found from the payload.
Once a parser has done everything it can to ensure it has access to its
arguments, it can call cli_split_args() to do the proper tokenization
and modify the input string to insert null bytes.

For now three commands was modified to take advantage of the multi-line
mode: "set ssl ocsp-response" and "add map"/"add acl".

The number of arguments has been increased to 8192 (from 64) and the
array to hold the pointers gets its memory from a pool. I chose 8192
since it's the maximum number of arguments possible with an input buffer
of 16384 (default tune.bufsize) (16384 / 2 if all arguments are one-char
words). Would computing the max value at runtime be better ?

A field, 'arg_count', is left unused by the parsers (but filled) in
'struct cli_input'. As a next step, instead of checking if the n-th
argument is not an empty string, it could be a good idea to convert all
parsers to check this field so they can know how many arguments they can
access to.

--
Aurélien.
>From 37b3c2f86bc77f6fd9a9eb416b43c1183cd81351 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= 
Date: Sun, 15 Apr 2018 18:38:51 +0200
Subject: [PATCH] MEDIUM: cli: Add multi-line mode support
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

It is now possible to write commands spanning on multiple lines. To do
so, a special pattern (<<) must be entered at the end of the first line.

Example:

# echo -e "set ssl ocsp-response <<\n$(base64 ocsp.der)\n" | \
   socat /tmp/sock1 -

All commands support this mode and some of them ("set ssl ocsp-response"
and "add map"/"add acl") have seen their syntax enhanced to take advantage
of it (i.e.: accept base64 on multiple lines or accept multiple key/value
pairs).

Signed-off-by: Aurélien Nephtali 
---
 doc/management.txt|  24 
 include/common/defaults.h |   4 +-
 include/proto/applet.h|   4 +-
 include/proto/cli.h   |   3 +-
 include/types/applet.h|   6 +-
 include/types/cli.h   |  22 ++-
 src/cache.c   |   2 +-
 src/cli.c | 344 +++---
 src/dns.c |   9 +-
 src/hlua.c|   8 +-
 src/map.c | 188 +
 src/proto_http.c  |  15 +-
 src/proxy.c   |  66 ++---
 src/server.c  | 131 +++---
 src/ssl_sock.c|  42 --
 src/stats.c   |  39 --
 src/stick_table.c |  19 +--
 src/stream.c  |  27 ++--
 18 files changed, 647 insertions(+), 306 deletions(-)

diff --git a/doc/management.txt b/doc/management.txt
index 4b6901851..f01c87e95 100644
--- a/doc/management.txt
+++ b/doc/management.txt
@@ -1298,6 +1298,30 @@ delimiter to mark an end of output for each command, and takes care of ensuring
 that no command can emit an empty line on output. A script can thus easily
 parse the output even when multiple commands were pipelined on a single line.
 
+It is also possible to send commands spanning on multiple lines. This is done by
+ending the first line with the "<<" pattern. The next lines will be treated as
+being part of the command and an empty line will validate the command.
+This multi-line mode is also usable with the interactive mode: in this case the
+prompt 

Re: Question regarding haproxy backend behaviour

2018-04-17 Thread Moemen MHEDHBI
Hi


On 16/04/2018 12:04, Igor Cicimov wrote:
>
>
> On Mon, 16 Apr 2018 6:09 pm Ayush Goyal  > wrote:
>
> Hi Moemen,
>
> Thanks for your response. But I think I need to clarify a few
> things here. 
>
> On Mon, Apr 16, 2018 at 4:33 AM Moemen MHEDHBI
> > wrote:
>
> Hi
>
>
> On 12/04/2018 19:16, Ayush Goyal wrote:
>> Hi,
>>
>> I have a question regarding haproxy backend connection
>> behaviour. We have following setup:
>>
>>   +-+     +---+
>>   | haproxy |>| nginx |
>>   +-+     +---+
>>
>> We use a haproxy cluster for ssl off-loading and then load
>> balance request to
>> nginx cluster. We are currently benchmarking this setup with
>> 3 nodes for haproxy
>> cluster and 1 nginx node. Each haproxy node has two
>> frontend/backend pair. First
>> frontend is a router for ssl connection which redistributes
>> request to the second 
>> frontend in the haproxy cluster. The second frontend is for
>> ssl handshake and 
>> routing requests to nginx servers. Our configuration is as
>> follows:
>>
>> ```
>> global
>>     maxconn 10
>>     user haproxy
>>     group haproxy
>>     nbproc 2
>>     cpu-map 1 1
>>     cpu-map 2 2
>>
>> defaults
>>     mode http
>>     option forwardfor
>>     timeout connect 5s
>>     timeout client 30s
>>     timeout server 30s
>>     timeout tunnel 30m
>>     timeout client-fin 5s
>>
>> frontend ssl_sess_id_router
>>         bind *:443
>>         bind-process 1
>>         mode tcp
>>         maxconn 10
>>         log global
>>         option tcp-smart-accept
>>         option splice-request
>>         option splice-response
>>         default_backend ssl_sess_id_router_backend
>>
>> backend ssl_sess_id_router_backend
>>         bind-process 1
>>         mode tcp
>>         fullconn 5
>>         balance roundrobin
>>         ..
>>         option tcp-smart-connect
>>         server lbtest01 :8443 weight 1 check send-proxy
>>         server lbtest02 :8443 weight 1 check send-proxy
>>         server lbtest03 :8443 weight 1 check send-proxy
>>
>> frontend nginx_ssl_fe
>>         bind *:8443 ssl 
>>         maxconn 10
>>         bind-process 2
>>         option tcp-smart-accept
>>         option splice-request
>>         option splice-response
>>         option forwardfor
>>         reqadd X-Forwarded-Proto:\ https
>>         timeout client-fin 5s
>>         timeout http-request 8s
>>         timeout http-keep-alive 30s
>>         default_backend nginx_backend
>>
>> backend nginx_backend
>>         bind-process 2
>>         balance roundrobin
>>         http-reuse safe
>>         option tcp-smart-connect
>>         option splice-request
>>         option splice-response
>>         timeout tunnel 30m
>>         timeout http-request 8s
>>         timeout http-keep-alive 30s
>>         server testnginx :80  weight 1 check
>> ```
>>
>> The nginx node has nginx with 4 workers and 8192 max clients,
>> therefore the max
>> number of connection it can accept is 32768.
>>
>> For benchmark, we are generating ~3k new connections per
>> second where each
>> connection makes 1 http request and then holds the connection
>> for next 30
>> seconds. This results in a high established connection on the
>> first frontend,
>> ssl_sess_id_router, ~25k per haproxy node (Total ~77k
>> connections on 3 haproxy
>> nodes). The second frontend (nginx_ssl_fe) receives the same
>> number of
>> connection on the frontend. On nginx node, we see that active
>> connections
>> increase to ~32k.
>>
>> Our understanding is that haproxy should keep a 1:1
>> connection mapping for each
>> new connection in frontend/backend. But there is a connection
>> count mismatch
>> between haproxy and nginx (Total 77k connections in all 3
>> haproxy for both
>> frontends vs 32k connections in nginx made by nginx_backend),
>> We are still not
>> facing any major 5xx or connection errors. We are assuming
>> that this is
>> happening because haproxy is terminating old idle ssl
>> connections to serve the
>> new 

Re: 1.8.7 http-tunnel doesn't seem to work? (but default http-keep-alive does)

2018-04-17 Thread Willy Tarreau
On Tue, Apr 17, 2018 at 04:33:07PM +0200, Olivier Houchard wrote:
> After talking with Willy, here is an updated patch that does that.
> That way, the day we'll want to use EV_ONESHOT, we'll be ready, and won't
> miss any event.

Now merged, thanks guys!
Willy



Re: 1.8.7 http-tunnel doesn't seem to work? (but default http-keep-alive does)

2018-04-17 Thread Olivier Houchard
Hi again,

On Tue, Apr 17, 2018 at 01:07:49PM +0200, Olivier Houchard wrote:
[...]
> We only need one to prevent kevent() from trying to scanning the kqueue, so
> only setting kev[0] should be enough. It's inside an #ifdef because
> EV_RECEIPT was only implemented recently in OpenBSD, so all users may not have
> it.
> My hope is, if EV_RECEIPT is not set, we will indeed read even needlessly,
> but we will read them again at the next kevent() call. If it does not, and
> events are lost, then we'll have to add an extra entry that generates an 
> error.

After talking with Willy, here is an updated patch that does that.
That way, the day we'll want to use EV_ONESHOT, we'll be ready, and won't
miss any event.

Regards,

Olivier

>From 56701f85b3311657ea6915df94ec75ac7a83eb54 Mon Sep 17 00:00:00 2001
From: Olivier Houchard 
Date: Mon, 16 Apr 2018 13:24:48 +0200
Subject: [PATCH] BUG/MEDIUM: kqueue: When adding new events, provide an output
 to get errors.

When adding new events using kevent(), if there's an error, because we're
trying to delete an event that wasn't there, or because the fd has already
been closed, kevent() will either add an event in the eventlist array if
there's enough room for it, and keep on handling other events, or stop and
return -1.
We want it to process all the events, so give it a large-enough array to
store any error.

This should be backported to 1.8.
---
 src/ev_kqueue.c | 43 +--
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/src/ev_kqueue.c b/src/ev_kqueue.c
index a103ece9d..ebfd5d210 100644
--- a/src/ev_kqueue.c
+++ b/src/ev_kqueue.c
@@ -31,6 +31,7 @@
 /* private data */
 static int kqueue_fd[MAX_THREADS]; // per-thread kqueue_fd
 static THREAD_LOCAL struct kevent *kev = NULL;
+static struct kevent *kev_out = NULL; // Trash buffer for kevent() to write 
the eventlist in
 
 /*
  * kqueue() poller
@@ -43,6 +44,8 @@ REGPRM2 static void _do_poll(struct poller *p, int exp)
int updt_idx, en;
int changes = 0;
 
+   timeout.tv_sec  = 0;
+   timeout.tv_nsec = 0;
/* first, scan the update list to find changes */
for (updt_idx = 0; updt_idx < fd_nbupdt; updt_idx++) {
fd = fd_updt[updt_idx];
@@ -81,13 +84,21 @@ REGPRM2 static void _do_poll(struct poller *p, int exp)
HA_ATOMIC_OR([fd].polled_mask, tid_bit);
}
}
-   if (changes)
-   kevent(kqueue_fd[tid], kev, changes, NULL, 0, NULL);
+   if (changes) {
+#ifdef EV_RECEIPT
+   kev[0].flags |= EV_RECEIPT;
+#else
+   /* If EV_RECEIPT isn't defined, just add an invalid entry,
+* so that we get an error and kevent() stops before scanning
+* the kqueue.
+*/
+   EV_SET([changes++], -1, EVFILT_WRITE, EV_DELETE, 0, 0, 
NULL);
+#endif
+   kevent(kqueue_fd[tid], kev, changes, kev_out, changes, 
);
+   }
fd_nbupdt = 0;
 
delta_ms= 0;
-   timeout.tv_sec  = 0;
-   timeout.tv_nsec = 0;
 
if (!exp) {
delta_ms= MAX_DELAY_MS;
@@ -150,8 +161,12 @@ static int init_kqueue_per_thread()
 {
int fd;
 
-   /* we can have up to two events per fd (*/
-   kev = calloc(1, sizeof(struct kevent) * 2 * global.maxsock);
+   /* we can have up to two events per fd, so allocate enough to store
+* 2*fd event, and an extra one, in case EV_RECEIPT isn't defined,
+* so that we can add an invalid entry and get an error, to avoid
+* scanning the kqueue uselessly.
+*/
+   kev = calloc(1, sizeof(struct kevent) * (2 * global.maxsock + 1));
if (kev == NULL)
goto fail_alloc;
 
@@ -194,6 +209,15 @@ REGPRM1 static int _do_init(struct poller *p)
 {
p->private = NULL;
 
+   /* we can have up to two events per fd, so allocate enough to store
+* 2*fd event, and an extra one, in case EV_RECEIPT isn't defined,
+* so that we can add an invalid entry and get an error, to avoid
+* scanning the kqueue uselessly.
+*/
+   kev_out = calloc(1, sizeof(struct kevent) * (2 * global.maxsock + 1));
+   if (!kev_out)
+   goto fail_alloc;
+
kqueue_fd[tid] = kqueue();
if (kqueue_fd[tid] < 0)
goto fail_fd;
@@ -203,6 +227,9 @@ REGPRM1 static int _do_init(struct poller *p)
return 1;
 
  fail_fd:
+   free(kev_out);
+   kev_out = NULL;
+fail_alloc:
p->pref = 0;
return 0;
 }
@@ -220,6 +247,10 @@ REGPRM1 static void _do_term(struct poller *p)
 
p->private = NULL;
p->pref = 0;
+   if (kev_out) {
+   free(kev_out);
+   kev_out = NULL;
+   }
 }
 
 /*
-- 
2.14.3



Re: Haproxy 1.8 with OpenSSL 1.1.1-pre4 stops working after 1 hour

2018-04-17 Thread Lukas Tribus
Hello Sander,


On 16 April 2018 at 10:55, Sander Hoentjen  wrote:
> Reading my email again it looks like somehow I messed up part of it,
> retrying:
>
> Hi all,
>
> I built Haproxy (1.8.7) against openssl 1.1.1-pre4, and now after 1 hour
> running haproxy stops accepting new SSL connections.

I have seen something like that a few weeks ago as well, but I've
reverted to openssl stable, as I did not have time to get involved
with troubleshooting openssl master at this point.


That having said you may want to retry with latest 1.1.1-pre5, it may
have been already fixed (Prevent a possible recursion in ERR_get_state
...).



Lukas



Re: resolvers - resolv.conf fallback

2018-04-17 Thread Ben Draut
Yep, will do.

On Tue, Apr 17, 2018 at 8:04 AM, Baptiste  wrote:

>
> On Sat, Apr 14, 2018 at 5:39 AM, Jonathan Matthews <
> cont...@jpluscplusm.com> wrote:
>
>> On 14 April 2018 at 05:13, Willy Tarreau  wrote:
>> > On Fri, Apr 13, 2018 at 03:48:19PM -0600, Ben Draut wrote:
>> >> How about 'parse-resolv-conf' for the current feature, and we reserve
>> >> 'use-system-resolvers' for the feature that Jonathan described?
>> >
>> > Perfect! "parse" is quite explicit at least!
>>
>> Works for me :-)
>>
>>
> Great, amazing!!!
> Ben, could you provide a patch using native code? (no third party
> libraries)
>
> Baptiste
>


Re: resolvers - resolv.conf fallback

2018-04-17 Thread Baptiste
On Sat, Apr 14, 2018 at 5:39 AM, Jonathan Matthews 
wrote:

> On 14 April 2018 at 05:13, Willy Tarreau  wrote:
> > On Fri, Apr 13, 2018 at 03:48:19PM -0600, Ben Draut wrote:
> >> How about 'parse-resolv-conf' for the current feature, and we reserve
> >> 'use-system-resolvers' for the feature that Jonathan described?
> >
> > Perfect! "parse" is quite explicit at least!
>
> Works for me :-)
>
>
Great, amazing!!!
Ben, could you provide a patch using native code? (no third party libraries)

Baptiste


Re: DNS resolver and mixed case responses

2018-04-17 Thread Baptiste
Hi all,

Thanks a lot for your various investigations!
As a conclusion, HAProxy's behavior is "as expected".

Baptiste


Re: SSL/TLS support for peers

2018-04-17 Thread Frederic Lecaille

On 04/11/2018 03:10 PM, Frederic Lecaille wrote:

Hello ML,

This is a first patch attempt to add the SSL/TLS support to peers.



So, after having discussed about this feature in private here is a more 
complete patch to support SSL/TLS over peer protocol.


Regards,
Fred.

>From ecf7ddcc431bfc5ebaf151af2027a17bea123f97 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= 
Date: Tue, 17 Apr 2018 08:42:57 +0200
Subject: [PATCH] MINOR: peers: Add SSL/TLS support.

As all peers belonging to a "peers" section share the same SSL/TLS settings,
these latter are provided on lines which may potentially be the last ones of
a "peers" section.

So, this patch extracts the current code used to setup the connection binding
of each frontend for each "peers" section so that it can be used after
having completely parsed "peers" sections. A "bind_conf" structure field
has been added to "peers" structures to do so. On the counterpart, a "server"
structure field has been also added to "peers" structure to store at parsing
time the SSL/TLS settings used to connect to remote peers.

Both these two new "peers" fields are used after having parsed the configuration
files to setup the SSL/TLS settings of all peers in the section. This is the reason
why these two new structure fields have been also added also to "peer" structure.

The line to setup the peer frontend SSL/TLS binding of a "peers" section are
identified by "ssl-accept" keyword. The remote peers SSL/TLS settings are setup
on "ssl-connect" lines. "ssl-accept" lines support all "bind" SSL/TLS settings
even when irrelevant (but without any impact). "ssl-connect" lines also support
all "server" SSL/TLS settings even if some of them are irrelevant.

Ex:
  # Enable SSL/TLS support for "my_peers" section peers
  peers my_peers
ssl-accept crt my_certs/pem
ssl-connect verify none
peer peerA...
peer peerB...

or
  global
ssl-server-verify none

  peers my_peers
ssl-accept crt my_certs/pem
peer peerA...
peer peerB...
---
 doc/configuration.txt  |  40 +++
 include/proto/peers.h  |  64 +
 include/proto/server.h |   1 +
 include/types/peers.h  |  11 +++
 src/cfgparse.c | 192 +
 src/peers.c|   3 +-
 src/server.c   |   2 +-
 7 files changed, 283 insertions(+), 30 deletions(-)

diff --git a/doc/configuration.txt b/doc/configuration.txt
index c687722..755a823 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -1851,6 +1851,28 @@ peer  :
   You may want to reference some environment variables in the address
   parameter, see section 2.3 about environment variables.
 
+ssl-connect [params*]
+  This setting is only available when support for OpenSSL was built in. It
+  enables SSL on connections instantiated from the listener or local peer of
+  this "peers" section to connect to remote peers. All "server" SSL/TLS
+  parameters are supported on this line even if some of them are really not
+  relevant for peer protocol.
+
+  It is not mandatory to set "ssl" as parameter because it is enabled by default.
+
+ssl-accept [params*]
+  This setting is only available when support for OpenSSL was built in. It
+  enables SSL on connections instantiated from the listener or local peer of
+  this "peers" section. A certificate is necessary (see "crt" "bind" option).
+  All "bind" SSL/TLS parameters are supported on this line even if some of
+  them are really not relevant for peer protocol.
+
+  It is not mandatory to set "ssl" as parameter because it is enabled by default.
+
+  When "ssl-accept" is set, it is also not mandatory to set "ssl-connect" to
+  support the basic following SSL/TLS configuration without CA file to verify
+  client certificates (see SSL/TLS example below).
+
   Example:
 peers mypeers
 peer haproxy1 192.168.0.1:1024
@@ -1866,6 +1888,24 @@ peer  :
 server srv1 192.168.0.30:80
 server srv2 192.168.0.31:80
 
+   SSL/TLS example:
+ peers mypeers
+ssl-accept crt mycerts/pem
+ssl-connect verify none
+peer haproxy1 192.168.0.1:1024
+peer haproxy2 192.168.0.2:1024
+peer haproxy3 10.2.0.1:1024
+
+   is equivalent to:
+
+ global
+ssl-server-verify none
+
+ peers mypeers
+ssl-accept crt mycerts/pem
+peer haproxy1 192.168.0.1:1024
+peer haproxy2 192.168.0.2:1024
+peer haproxy3 10.2.0.1:1024
 
 3.6. Mailers
 
diff --git a/include/proto/peers.h b/include/proto/peers.h
index 782b66e..32ce3c2 100644
--- a/include/proto/peers.h
+++ b/include/proto/peers.h
@@ -28,6 +28,70 @@
 #include 
 #include 
 
+#if defined(USE_OPENSSL)
+#include 
+
+static inline enum obj_type *peer_session_target(struct peer *p, struct stream *s)
+{
+	if (p->srv.use_ssl)
+		return >srv.obj_type;
+	else
+		return >be->obj_type;
+}
+
+static inline struct xprt_ops *peers_fe_xprt(struct peers *peers)

Re: Version 1.5.12, getting 502 when server check fails, but server is still working

2018-04-17 Thread Shawn Heisey

On 4/17/2018 3:41 AM, Willy Tarreau wrote:

Here I'm afraid we're all wasting a lot of time trying to guess what
you have in your config that causes the problem. It's OK if you cannot
post your config here, but please at least post a smaller one reproducing
the issue so that we can help you.


I did send a message with the config, but it turns out that it was only 
in an accidental unicast to Lukas.  Sorry about that! Here is the 
redacted version of my config (one month expiration):


https://apaste.info/xVwg


As of a little while ago, I have solved all the problems I encountered
on the road to graceful application restarts except the one where a
backup server is not promoted to active as soon as the primary servers
are all down.

This is normally done with the "backup" keyword on server lines.


I described that issue in a separate message to the
list.  I do have a workaround to that issue -- I'm no longer using
"backup" on any server entries for this service.

Then I don't see how it can work for you. It's a bit confusing I'm afraid.


Originally, the "hollywood" entry on the be-cdn-9000 backend (which you 
can see at the config I linked above) had the backup keyword.  But what 
I noticed happening was that when planet went down, it took about ten 
additional seconds (no precise timing was done) for hollywood to go 
active, and during that time, the client got "no server available" 
messages.  Removing the backup keyword caused hollywood to be active 
full time and requests to be load balanced to both servers, so when one 
server goes down, the other is already active and everything works.


Removing the backup keyword from this particular backend is not a 
problem, but I do have other backends where I really do want only one 
server to get requests unless that server goes down.  I would like the 
highest weight backup server to take over immediately when the primary 
is marked down, not wait for another timeout to pass.


Thanks,
Shawn




cfgparse: multiline parsing

2018-04-17 Thread Frederic Lecaille

Hello ML,

Here is a simple patch to support the multiline parsing.

Regards,

Fred.
>From 6ab05b5c9bb8f0a644652ba58376b21ccf800f4b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= 
Date: Tue, 17 Apr 2018 10:54:08 +0200
Subject: [PATCH] MINOR: cfgparse: Add support for multiline parsing.

When an escaped trailing new line character is read, we make the parser
believe we have just read the number of bytes we read minus 2.

May be easily backported to versions 1.6 and newer.
---
 doc/configuration.txt | 4 +++-
 src/cfgparse.c| 6 ++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/doc/configuration.txt b/doc/configuration.txt
index c687722..9e0408f 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -417,7 +417,9 @@ escaped by doubling or strong quoting them.
 
 Escaping is achieved by preceding a special character by a backslash ('\'):
 
-  \to mark a space and differentiate it from a delimiter
+  \to mark a space and differentiate it from a delimiter;
+   when followed by a trailing new line character, split the current
+   line into two parts (multiline parsing).
   \#   to mark a hash and differentiate it from a comment
   \\   to use a backslash
   \'   to use a single quote and differentiate it from strong quoting
diff --git a/src/cfgparse.c b/src/cfgparse.c
index 621af6c..85c67a3 100644
--- a/src/cfgparse.c
+++ b/src/cfgparse.c
@@ -6980,6 +6980,12 @@ next_line:
 			continue;
 		}
 
+		if (end - line >= 2 && end[-2] == '\\') {
+			/* Make the parser erase the escaped trailing new line character. */
+			readbytes = end - line - 2;
+			continue;
+		}
+
 		readbytes = 0;
 
 		/* skip leading spaces */
-- 
2.1.4



Re: 1.8.7 http-tunnel doesn't seem to work? (but default http-keep-alive does)

2018-04-17 Thread Olivier Houchard
Hi Pieter,

On Mon, Apr 16, 2018 at 10:41:48PM +0200, PiBa-NL wrote:
> Hi Olivier,
> 
> Op 16-4-2018 om 17:09 schreef Olivier Houchard:
> > After some discussion with Willy, we came with a solution that may fix your
> > problem with kqueue.
> > Can you test the attached patch and let me know if it fixes it for you ?
> > 
> > Minor variation of the patch, that uses EV_RECEIPT if available, to avoid
> > scanning needlessly the kqueue.
> > 
> > Regards,
> > 
> > Olivier
> 
> Thanks the patch solves the issue i experienced at least for the testcase
> that i had. (And doesn't seem to cause obvious new issues that i could
> quickly spot..) Both with and without EV_RECEIPT on kev[0] it seems to work
> the same for my testcase..
> 
> Just a few thoughts though:
> Now only the first kev[0] gets the EV_RECEIPT flag, shouldn't it be added to
> all items in the array? Now sometimes 3 changes are send and only 2
> 'results' are reported back. If i read right the EV_RECEIPT should 'force' a
> result for each change send. Also is there a reason you put it inside a
> '#ifdef' ? It seems to me a hard requirement to not read any possible
> pending events when sending the list of updated filters at that moment.?. Or
> perhaps its possible to call kevent only once? Both sending changes, and
> receiving new events in 1 big go and without the RECEIPT flag?
> 

We only need one to prevent kevent() from trying to scanning the kqueue, so
only setting kev[0] should be enough. It's inside an #ifdef because
EV_RECEIPT was only implemented recently in OpenBSD, so all users may not have
it.
My hope is, if EV_RECEIPT is not set, we will indeed read even needlessly,
but we will read them again at the next kevent() call. If it does not, and
events are lost, then we'll have to add an extra entry that generates an error.
We considered using only one kevent() call for both adding events, and reading
them, but it makes it difficult to respect maxpollevents, because we'd need
enough room for all errors and reading the new events. 

> There are now more 'changes' send than required that need to be disregarded
> with a 'error flag' by kqueue
> Doesn't that (slightly) affect performance? Or would checking a bitmask
> beforehand not be cheaper than what kevent itself needs to do to ignore an
> item and 'error report' some of the changes.?. I've not tried to measure
> this, but technically i think there will be a few more cpu operations needed
> overall this way.?.

It should be fine. What kevent() does is :
foreach (change) {
ret = process_change();
if (ret != 0 || flags & EV_RECEIPT)
nerrors++;
}
if (nerrors > 0)
return nerrors;
return scan_kqueue();

So it should virtually have no impact on performances.

Regards,

Olivier





Re: Version 1.5.12, getting 502 when server check fails, but server is still working

2018-04-17 Thread Willy Tarreau
On Mon, Apr 16, 2018 at 04:13:28PM -0600, Shawn Heisey wrote:
> [ALERT] 105/095234 (7186) : config : backend 'be-cdn-9000', server
> 'planet': unable to use chk-cdn-9000/planet fortracking: disable-on-404
> option inconsistency.
> [ALERT] 105/095234 (7186) : config : backend 'be-cdn-9000', server
> 'hollywood': unable to use chk-cdn-9000/hollywood fortracking:
> disable-on-404 option inconsistency.
> [ALERT] 105/095234 (7186) : Fatal errors found in configuration.
> 
> I also tried it with that option in both backend configurations.  That
> didn't work either.  I don't recall the error, but it was probably the
> same as one of the other errors I had gotten before.

Well, the doc about "track" says this :

track [/]
  This option enables ability to set the current state of the server by tracking
  another one. It is possible to track a server which itself tracks another
  server, provided that at the end of the chain, a server has health checks
  enabled. If  is omitted the current one is used. If disable-on-404 is
  used, it has to be enabled on both proxies.

So it might have been a different error that you got, possibly caused by an
incompatibility with something else.

> This is on 1.5.12, and I can't really blame you if the "standard" mental
> map you keep of the project doesn't include that version.

Not at all! Versions which are considered supported are still watched,
and eventhough they are updated less often, they still receive fixes if
needed. The latest 1.5 is 1.5.19, so your version is at least affected
by all the issues referenced here :

http://www.haproxy.org/bugs/bugs-1.5.12.html

But none of them seem to involve tracking nor disable-on-404 at first
glance.

> It's got to
> be hard enough keeping that straight for just 1.8 and 1.9-dev!  Maybe
> the error I encountered would be solved by upgrading.  Upgrading is on
> the (really long) todo list.

There are two distinct things :
  - getting issues fixed
  - upgrading to get new features or support

As long as an issue exists in a supported version, it has to be fixed.
Some issues may be the result of an architectural limitation which will
require an upgrade. But most often it is not the case.

Here I'm afraid we're all wasting a lot of time trying to guess what
you have in your config that causes the problem. It's OK if you cannot
post your config here, but please at least post a smaller one reproducing
the issue so that we can help you.

> I do have a config that works.  I'm no longer tracking another backend,
> but doing the health checks in the load-balancing backend.  The whole
> reason I had migrated server checks to dedicated back ends was because I
> wanted to reduce the number of check requests being sent, and I'm
> sharing the check backends with multiple balancing backends in some
> cases.

That's fine, it's exactly what "track" is made for.

> For the one I've been describing, I don't need to share the
> check backend.

OK but it possibly uncovers another issue in your config.

> I ran into other problems on the application side with how process
> shutdowns work, but resolved those by adding an endpoint into my app
> with the URL path of "/lbdisable" and handling the disable/pause in the
> init script instead of the application.  I can now restart my custom
> application at will without any loss, and without a client even noticing
> there was a problem.

OK.

> As of a little while ago, I have solved all the problems I encountered
> on the road to graceful application restarts except the one where a
> backup server is not promoted to active as soon as the primary servers
> are all down.

This is normally done with the "backup" keyword on server lines.

> I described that issue in a separate message to the
> list.  I do have a workaround to that issue -- I'm no longer using
> "backup" on any server entries for this service.

Then I don't see how it can work for you. It's a bit confusing I'm afraid.

Willy



Re: [External] Re: [PATCH] MEDIUM: sample: Extend functionality for field/word converters

2018-04-17 Thread Marcin Deranek
On 04/17/2018 11:29 AM, Willy Tarreau wrote:

> Pretty cool, thank you. I've already missed this for domain names (using
> the dot as the delimitor). Now merged :-)

Cool. We missed this functionality too, so eventually I invested some
time to get it done :-))

Marcin Deranek




Re: [PATCH] MEDIUM: sample: Extend functionality for field/word converters

2018-04-17 Thread Willy Tarreau
Hi Marcin,

On Tue, Apr 17, 2018 at 11:12:54AM +0200, Marcin Deranek wrote:
> Hi,
> 
> This patch extends functionality of field/word converters, so it's
> possible to count fields/words from the beginning/end of the string and
> extract multiple fields/words if needed. Change is backward compatible
> and should cleanly apply to both 1.8 & 1.9 branches.

Pretty cool, thank you. I've already missed this for domain names (using
the dot as the delimitor). Now merged :-)

Willy



[PATCH] MEDIUM: sample: Extend functionality for field/word converters

2018-04-17 Thread Marcin Deranek
Hi,

This patch extends functionality of field/word converters, so it's
possible to count fields/words from the beginning/end of the string and
extract multiple fields/words if needed. Change is backward compatible
and should cleanly apply to both 1.8 & 1.9 branches.
Regards,

Marcin Deranek

>From 3393f952788e26e6e8add5b6ca472d3e765b57ca Mon Sep 17 00:00:00 2001
From: Marcin Deranek 
Date: Mon, 16 Apr 2018 14:30:46 +0200
Subject: [PATCH] Extend functionality for field/word converters

Extend functionality of field/word converters, so it's possible
to extract field(s)/word(s) counting from the beginning/end and/or
extract multiple fields/words (including separators) eg.

str(f1_f2_f3__f5),field(2,_,2)  # f2_f3
str(f1_f2_f3__f5),field(2,_,0)  # f2_f3__f5
str(f1_f2_f3__f5),field(-2,_,3) # f2_f3_
str(f1_f2_f3__f5),field(-3,_,0) # f1_f2_f3

str(w1_w2_w3___w4),word(3,_,2)  # w3___w4
str(w1_w2_w3___w4),word(2,_,0)  # w2_w3___w4
str(w1_w2_w3___w4),word(-2,_,3) # w1_w2_w3
str(w1_w2_w3___w4),word(-3,_,0) # w1_w2

Change is backward compatible.
---
 doc/configuration.txt |  34 ++---
 src/sample.c  | 132 +-
 2 files changed, 125 insertions(+), 41 deletions(-)

diff --git a/doc/configuration.txt b/doc/configuration.txt
index 1e0b26f8..a9f75579 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -12907,10 +12907,20 @@ even
   Returns a boolean TRUE if the input value of type signed integer is even
   otherwise returns FALSE. It is functionally equivalent to "not,and(1),bool".
 
-field(,)
-  Extracts the substring at the given index considering given delimiters from
-  an input string. Indexes start at 1 and delimiters are a string formatted
-  list of chars.
+field(,[,])
+  Extracts the substring at the given index counting from the beginning
+  (positive index) or from the end (negative index) considering given delimiters
+  from an input string. Indexes start at 1 or -1 and delimiters are a string
+  formatted list of chars. Optionally you can specify  of fields to
+  extract (default: 1). Value of 0 indicates extraction of all remaining
+  fields.
+
+  Example :
+  str(f1_f2_f3__f5),field(5,_)# f5
+  str(f1_f2_f3__f5),field(2,_,0)  # f2_f3__f5
+  str(f1_f2_f3__f5),field(2,_,2)  # f2_f3
+  str(f1_f2_f3__f5),field(-2,_,3) # f2_f3_
+  str(f1_f2_f3__f5),field(-3,_,0) # f1_f2_f3
 
 hex
   Converts a binary input sample to a hex string containing two hex digits per
@@ -13440,9 +13450,19 @@ utime([,])
   # e.g.  20140710162350 127.0.0.1:57325
   log-format %[date,utime(%Y%m%d%H%M%S)]\ %ci:%cp
 
-word(,)
-  Extracts the nth word considering given delimiters from an input string.
-  Indexes start at 1 and delimiters are a string formatted list of chars.
+word(,[,])
+  Extracts the nth word counting from the beginning (positive index) or from
+  the end (negative index) considering given delimiters from an input string.
+  Indexes start at 1 or -1 and delimiters are a string formatted list of chars.
+  Optionally you can specify  of words to extract (default: 1).
+  Value of 0 indicates extraction of all remaining words.
+
+  Example :
+  str(f1_f2_f3__f5),word(4,_)# f5
+  str(f1_f2_f3__f5),word(2,_,0)  # f2_f3__f5
+  str(f1_f2_f3__f5),word(3,_,2)  # f3__f5
+  str(f1_f2_f3__f5),word(-2,_,3) # f1_f2_f3
+  str(f1_f2_f3__f5),word(-3,_,0) # f1_f2
 
 wt6([])
   Hashes a binary input sample into an unsigned 32-bit quantity using the WT6
diff --git a/src/sample.c b/src/sample.c
index 71ee59f0..154beb5c 100644
--- a/src/sample.c
+++ b/src/sample.c
@@ -1997,27 +1997,54 @@ static int sample_conv_field_check(struct arg *args, struct sample_conv *conv,
  */
 static int sample_conv_field(const struct arg *arg_p, struct sample *smp, void *private)
 {
-	unsigned int field;
+	int field;
 	char *start, *end;
 	int i;
+	int count = (arg_p[2].type == ARGT_SINT) ? arg_p[2].data.sint : 1;
 
 	if (!arg_p[0].data.sint)
 		return 0;
 
-	field = 1;
-	end = start = smp->data.u.str.str;
-	while (end - smp->data.u.str.str < smp->data.u.str.len) {
-
-		for (i = 0 ; i < arg_p[1].data.str.len ; i++) {
-			if (*end == arg_p[1].data.str.str[i]) {
-if (field == arg_p[0].data.sint)
-	goto found;
-start = end+1;
-field++;
-break;
+	if (arg_p[0].data.sint < 0) {
+		field = -1;
+		end = start = smp->data.u.str.str + smp->data.u.str.len;
+		while (start > smp->data.u.str.str) {
+			for (i = 0 ; i < arg_p[1].data.str.len ; i++) {
+if (*(start-1) == arg_p[1].data.str.str[i]) {
+	if (field == arg_p[0].data.sint) {
+		if (count == 1)
+			goto found;
+		else if (count > 1)
+			count--;
+	} else {
+		end = start-1;
+		field--;
+	}
+	break;
+}
 			}
+			start--;
+		}
+	} else {
+		field = 1;
+		end = start = smp->data.u.str.str;
+		while (end - smp->data.u.str.str < smp->data.u.str.len) {
+			for (i = 0 ; i < arg_p[1].data.str.len ; i++) {
+