Re: [PR] Add srvkey option to stick-table

[Thayne McCombs]

Awesome! Thanks everyone.

Thayne McCombs
Senior Software Engineer
tha...@lucidchart.com



Lucid.co 


On Thu, Dec 31, 2020 at 2:10 AM Willy Tarreau  wrote:

> On Tue, Dec 29, 2020 at 10:52:56AM +0100, Frederic Lecaille wrote:
> > > +#REQUIRE_VERSION=2.0
> >
> > Ok for this test but I think we should use 2.4 which is the current
> > developemnt version.
>
> OK I applied this change. Tested here and it works.
>
> > Willy, I think we can import this patch after having merged the previous
> one
> > [1/2] patch about this feature implementation.
>
> Now done.
>
> Thanks!
> Willy
>

Re: [PR] Add srvkey option to stick-table

[Willy Tarreau]

On Tue, Dec 29, 2020 at 10:52:56AM +0100, Frederic Lecaille wrote:
> > +#REQUIRE_VERSION=2.0
> 
> Ok for this test but I think we should use 2.4 which is the current
> developemnt version.

OK I applied this change. Tested here and it works.

> Willy, I think we can import this patch after having merged the previous one
> [1/2] patch about this feature implementation.

Now done.

Thanks!
Willy

Re: [PR] Add srvkey option to stick-table

[Willy Tarreau]

Hi guys,

On Tue, Dec 29, 2020 at 10:45:17AM +0100, Frederic Lecaille wrote:
> You work sounds perfect to me.

FWIW I agree that th patch looks particularly clean. I've not studied it
though, I trust you :-)

> I am not sure we will import the second patch for the reg test: it makes
> usage of curl. We try to not use external programs for reg tests except if
> there is no choice. This is not the case here.
> 
> Willy, could you have a look at the first patch, especially sa2str()
> implementation. I have doubt about its portability:
> 
>const int max_length = sizeof(struct sockaddr_un) - offsetof(struct
> sockaddr_un, sun_path) - 1;
>return memprintf(, "abns@%.*s", max_length, path+1);
> 
> 
> As far as I see, everywhere that unix socket are used in haproxy code, we
> consider ->sun_path as NULL terminated.

I think it's OK since "%.*" fixes the maximum length anyway so it will
always work. But you're right, we normally enforce the trailing zero,
so I guess that "abns@%s" would equally work. But there's nothing wrong
here anyway, and we don't have an portability issues since abns only
exists on linux, which limits the scope.

I'm merging the patch then. Thayne, I'll extend a little bit your
commit message to mention that the peers support was also implemented
(the text only mentions stick-tables).

Thanks to you both!
Willy

Re: [PR] Add srvkey option to stick-table

[Thayne McCombs]

> I am not sure we will import the second patch for the reg test: it makes
usage of curl. We try to not use external programs for reg tests except
if there is no choice.

Fine by me. I actually meant to say that my second attempt at writing a reg
attempt should replace that patch. Apparently I forgot.

Thayne McCombs
Senior Software Engineer
tha...@lucidchart.com



Lucid.co 


On Tue, Dec 29, 2020 at 2:52 AM Frederic Lecaille 
wrote:

> On 12/19/20 9:07 AM, Thayne McCombs wrote:
> >  From 731d6a00f3cf0c70d7a8a916e1984c225f3a9dd6 Mon Sep 17 00:00:00 2001
> > From: Thayne McCombs 
> > Date: Sat, 19 Dec 2020 00:59:35 -0700
> > Subject: [PATCH] Add test for stickiness using the server address for the
> >   server key
> >
> > ---
> >   reg-tests/stickiness/srvkey-addr.vtc | 259 +++
> >   1 file changed, 259 insertions(+)
> >   create mode 100644 reg-tests/stickiness/srvkey-addr.vtc
> >
> > diff --git a/reg-tests/stickiness/srvkey-addr.vtc
> > b/reg-tests/stickiness/srvkey-addr.vtc
> > new file mode 100644
> > index 0..b5675089f
> > --- /dev/null
> > +++ b/reg-tests/stickiness/srvkey-addr.vtc
> > @@ -0,0 +1,259 @@
> > +vtest "A reg test for stickiness with srvkey addr"
> > +feature ignore_unknown_macro
> > +
> > +
> > +# The aim of this test is to check that "stick on" rules
> > +# do the job they are supposed to do.
> > +# If we remove one of the "stick on" rule, this script fails.
> > +
> > +#REQUIRE_VERSION=2.0
>
> Ok for this test but I think we should use 2.4 which is the current
> developemnt version.
>
> Willy, I think we can import this patch after having merged the previous
> one [1/2] patch about this feature implementation.
>

Re: [PR] Add srvkey option to stick-table

[Frederic Lecaille]

On 12/19/20 9:07 AM, Thayne McCombs wrote:

 From 731d6a00f3cf0c70d7a8a916e1984c225f3a9dd6 Mon Sep 17 00:00:00 2001
From: Thayne McCombs 
Date: Sat, 19 Dec 2020 00:59:35 -0700
Subject: [PATCH] Add test for stickiness using the server address for the
  server key

---
  reg-tests/stickiness/srvkey-addr.vtc | 259 +++
  1 file changed, 259 insertions(+)
  create mode 100644 reg-tests/stickiness/srvkey-addr.vtc

diff --git a/reg-tests/stickiness/srvkey-addr.vtc 
b/reg-tests/stickiness/srvkey-addr.vtc

new file mode 100644
index 0..b5675089f
--- /dev/null
+++ b/reg-tests/stickiness/srvkey-addr.vtc
@@ -0,0 +1,259 @@
+vtest "A reg test for stickiness with srvkey addr"
+feature ignore_unknown_macro
+
+
+# The aim of this test is to check that "stick on" rules
+# do the job they are supposed to do.
+# If we remove one of the "stick on" rule, this script fails.
+
+#REQUIRE_VERSION=2.0


Ok for this test but I think we should use 2.4 which is the current 
developemnt version.


Willy, I think we can import this patch after having merged the previous 
one [1/2] patch about this feature implementation.

Re: [PR] Add srvkey option to stick-table

[Frederic Lecaille]

On 12/16/20 8:39 AM, Thayne McCombs wrote:

On 12/10/20 1:31 AM, Frederic Lecaille wrote:

It would be preferable to send all your patches, so that others than me
may review your work (no diff between different versions of patches) and
continue to split your work in several patches.


Ok, here is what I have so far as two patches (I combined feedback into the 
original commit):



Hello Thayne,

You work sounds perfect to me.

I am not sure we will import the second patch for the reg test: it makes 
usage of curl. We try to not use external programs for reg tests except 
if there is no choice. This is not the case here.


Willy, could you have a look at the first patch, especially sa2str() 
implementation. I have doubt about its portability:


   const int max_length = sizeof(struct sockaddr_un) - offsetof(struct 
sockaddr_un, sun_path) - 1;

   return memprintf(, "abns@%.*s", max_length, path+1);


As far as I see, everywhere that unix socket are used in haproxy code, 
we consider ->sun_path as NULL terminated.

Re: [PR] Add srvkey option to stick-table

[Thayne McCombs]

On 12/10/20 1:36 AM, Frederic Lecaille wrote:
About a possible reg test, I remember there exists already one to test 
the stickiness. Have a look to reg-tests/stickiness/lb-services.vtc.
I think it should be preferable to make a new reg test which would be a 
copy of this latter but with the "server key" feature you have implemented.




Here it is:

From 731d6a00f3cf0c70d7a8a916e1984c225f3a9dd6 Mon Sep 17 00:00:00 2001
From: Thayne McCombs 
Date: Sat, 19 Dec 2020 00:59:35 -0700
Subject: [PATCH] Add test for stickiness using the server address for the
 server key

---
 reg-tests/stickiness/srvkey-addr.vtc | 259 +++
 1 file changed, 259 insertions(+)
 create mode 100644 reg-tests/stickiness/srvkey-addr.vtc

diff --git a/reg-tests/stickiness/srvkey-addr.vtc 
b/reg-tests/stickiness/srvkey-addr.vtc

new file mode 100644
index 0..b5675089f
--- /dev/null
+++ b/reg-tests/stickiness/srvkey-addr.vtc
@@ -0,0 +1,259 @@
+vtest "A reg test for stickiness with srvkey addr"
+feature ignore_unknown_macro
+
+
+# The aim of this test is to check that "stick on" rules
+# do the job they are supposed to do.
+# If we remove one of the "stick on" rule, this script fails.
+
+#REQUIRE_VERSION=2.0
+
+server s_not_used_1 {}
+server s_not_used_2 {}
+server s_not_used_3 {}
+server s_not_used_4 {}
+server s_not_used_5 {}
+server s_not_used_6 {}
+server s_not_used_7 {}
+server s_not_used_8 {}
+server s_not_used_9 {}
+server s_not_used_10 {}
+server s_not_used_11 {}
+server s_not_used_12 {}
+
+# h1/be1 servers
+server s1 {
+rxreq
+txresp -hdr "Server: s1"
+} -repeat 8 -start
+
+server s2 {
+rxreq
+txresp -hdr "Server: s2"
+} -repeat 8 -start
+
+haproxy h1 -arg "-L A" -conf {
+defaults
+mode http
+${no-htx} option http-use-htx
+timeout server 1s
+timeout connect 1s
+timeout client 1s
+log stdout format raw local0  debug
+
+peers mypeers
+bind "fd@${A}"
+server A
+server B ${h2_B_addr}:${h2_B_port}
+table mytable type string size 10m srvkey addr
+
+backend be1
+balance roundrobin
+stick on urlp(client) table mypeers/mytable
+server srv1 ${s1_addr}:${s1_port}
+server srv2 ${s2_addr}:${s2_port}
+
+backend be2
+balance roundrobin
+stick on urlp(client) table mypeers/mytable
+server s_not_used_1 ${s_not_used_1_addr}:${s_not_used_1_port}
+server s_not_used_2 ${s_not_used_2_addr}:${s_not_used_2_port}
+server s_not_used_3 ${s_not_used_3_addr}:${s_not_used_3_port}
+server srv2_2 ${s2_addr}:${s2_port}
+server s_not_used_4 ${s_not_used_4_addr}:${s_not_used_4_port}
+server s_not_used_5 ${s_not_used_5_addr}:${s_not_used_5_port}
+server s_not_used_6 ${s_not_used_6_addr}:${s_not_used_6_port}
+server srv1_2 ${s1_addr}:${s1_port}
+
+frontend fe
+acl acl_be1 path_beg /be1
+acl acl_be2 path_beg /be2
+use_backend be1 if acl_be1
+use_backend be2 if acl_be2
+bind "fd@${fe}"
+} -start
+
+haproxy h2 -arg "-L B" -conf {
+defaults
+mode http
+${no-htx} option http-use-htx
+timeout server 1s
+timeout connect 1s
+timeout client 1s
+
+peers mypeers
+bind "fd@${B}"
+server A ${h1_A_addr}:${h1_A_port}
+server B
+table mytable type string size 10m srvkey addr
+
+backend be1
+balance roundrobin
+stick on urlp(client) table mypeers/mytable
+server s_not_used_7 ${s_not_used_7_addr}:${s_not_used_7_port}
+server s_not_used_8 ${s_not_used_8_addr}:${s_not_used_8_port}
+server s_not_used_9 ${s_not_used_9_addr}:${s_not_used_9_port}
+server srv1_h2_1 ${s1_addr}:${s1_port}
+server s_not_used_10 ${s_not_used_10_addr}:${s_not_used_10_port}
+server s_not_used_11 ${s_not_used_11_addr}:${s_not_used_11_port}
+server s_not_used_12 ${s_not_used_12_addr}:${s_not_used_12_port}
+server srv2_h2_1 ${s2_addr}:${s2_port}
+
+backend be2
+balance roundrobin
+stick on urlp(client) table mypeers/mytable
+server s_not_used_1 ${s_not_used_1_addr}:${s_not_used_1_port}
+server s_not_used_2 ${s_not_used_2_addr}:${s_not_used_2_port}
+server s_not_used_3 ${s_not_used_3_addr}:${s_not_used_3_port}
+server s_not_used_4 ${s_not_used_4_addr}:${s_not_used_4_port}
+server s_not_used_5 ${s_not_used_5_addr}:${s_not_used_5_port}
+server s_not_used_6 ${s_not_used_6_addr}:${s_not_used_6_port}
+server srv1_h2_2 ${s1_addr}:${s1_port}
+server srv2_h2_2 ${s2_addr}:${s2_port}
+
+frontend fe
+acl acl_be1 path_beg /be1
+acl acl_be2 path_beg /be2
+use_backend be1 if acl_be1
+use_backend be2 if acl_be2
+bind "fd@${fe}"
+} -start
+
+delay 0.2
+
+client cx -connect ${h1_fe_sock} {
+txreq -url "/be1?client=c1"
+rxresp
+expect 

Re: [PR] Add srvkey option to stick-table

[Thayne McCombs]

On 12/10/20 1:31 AM, Frederic Lecaille wrote:
>
> It would be preferable to send all your patches, so that others than me 
> may review your work (no diff between different versions of patches) and 
> continue to split your work in several patches.
> 

Ok, here is what I have so far as two patches (I combined feedback into the 
original commit):


>From cf965f47e04776ca20d2ee6ed22028741493824c Mon Sep 17 00:00:00 2001
From: Thayne McCombs 
Date: Fri, 20 Nov 2020 01:28:26 -0700
Subject: [PATCH 1/2] Add srvkey option to stick-table

This allows using the address of the server rather than the name of the
server for keeping track of servers in a backend for stickiness.

Fixes #814
---
 doc/configuration.txt   | 12 -
 include/haproxy/dict.h  |  1 +
 include/haproxy/proxy-t.h   |  1 +
 include/haproxy/server-t.h  |  1 +
 include/haproxy/server.h|  2 +-
 include/haproxy/stick_table-t.h | 11 ++--
 include/haproxy/tools.h | 13 +
 src/cfgparse-listen.c   |  1 +
 src/cfgparse.c  |  4 +--
 src/dict.c  | 24 -
 src/peers.c |  9 +--
 src/server.c| 40 ++--
 src/stick_table.c   | 31 +-
 src/stream.c| 47 +++--
 src/tools.c | 45 +++
 15 files changed, 216 insertions(+), 26 deletions(-)

diff --git a/doc/configuration.txt b/doc/configuration.txt
index e60e3428d..e17061518 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -10649,7 +10649,7 @@ stick store-request  [table ] [{if | 
unless} ]
 
 
 stick-table type {ip | integer | string [len ] | binary [len ]}
-size  [expire ] [nopurge] [peers ]
+size  [expire ] [nopurge] [peers ] [srvkey 
]
 [store ]*
   Configure the stickiness table for the current section
   May be used in sections :   defaults | frontend | listen | backend
@@ -10726,6 +10726,16 @@ stick-table type {ip | integer | string [len ] 
| binary [len ]}
be removed once full. Be sure not to use the "nopurge" parameter
if not expiration delay is specified.
 
+   specifies how each server is identified for the purposes of the
+   stick table. The valid values are "name" and "addr". If "name" 
is
+   given, then  argument for the server (may be generated by
+   a template). If "addr" is given, then the server is identified
+   by its current network address, including the port. "addr" is
+   especially useful if you are using service discovery to generate
+   the addresses for servers with peered stick-tables and want
+   to consistently use the same host across peers for a stickiness
+   token.
+
 is used to store additional information in the stick-table. This
may be used by ACLs in order to control various criteria related
to the activity of the client matching the stick-table. For each
diff --git a/include/haproxy/dict.h b/include/haproxy/dict.h
index 59e81352c..c55834ca5 100644
--- a/include/haproxy/dict.h
+++ b/include/haproxy/dict.h
@@ -31,5 +31,6 @@
 
 struct dict *new_dict(const char *name);
 struct dict_entry *dict_insert(struct dict *d, char *str);
+void dict_entry_unref(struct dict *d, struct dict_entry *de);
 
 #endif  /* _HAPROXY_DICT_H */
diff --git a/include/haproxy/proxy-t.h b/include/haproxy/proxy-t.h
index 998e210f6..e62b79765 100644
--- a/include/haproxy/proxy-t.h
+++ b/include/haproxy/proxy-t.h
@@ -424,6 +424,7 @@ struct proxy {
char *lfsd_file;/* file name where the 
structured-data logformat string for RFC5424 appears (strdup) */
int  lfsd_line; /* file name where the 
structured-data logformat string for RFC5424 appears */
} conf; /* config information */
+   struct eb_root used_server_addr;/* list of server addresses in 
use */
void *parent;   /* parent of the proxy when 
applicable */
struct comp *comp;  /* http compression */
 
diff --git a/include/haproxy/server-t.h b/include/haproxy/server-t.h
index 0e66be693..13f5a5dab 100644
--- a/include/haproxy/server-t.h
+++ b/include/haproxy/server-t.h
@@ -337,6 +337,7 @@ struct server {
struct ebpt_node name;  /* place in the tree of used 
names */
int line;   /* line where the section 
appears */
} conf; /* config information */
+   struct ebpt_node addr_node; /* Node for string 
representation of address for the server (including port number) */
/* Template information used only for server objects which
 * serve as 

Re: [PR] Add srvkey option to stick-table

[Frederic Lecaille]

On 12/10/20 8:20 AM, Thayne McCombs wrote:

Here are my updates from the feedback.  It took me quite a while to figure out 
how to send this diff properly formatted with gmail.

If you would like a single patch with all my changes, I can send that as well.


About a possible reg test, I remember there exists already one to test 
the stickiness. Have a look to reg-tests/stickiness/lb-services.vtc.
I think it should be preferable to make a new reg test which would be a 
copy of this latter but with the "server key" feature you have implemented.

Re: [PR] Add srvkey option to stick-table

[Frederic Lecaille]

On 12/10/20 8:20 AM, Thayne McCombs wrote:

Here are my updates from the feedback.  It took me quite a while to figure out 
how to send this diff properly formatted with gmail.


Thank you for your effort. The result is perfect.


If you would like a single patch with all my changes, I can send that as well.


It would be preferable to send all your patches, so that others than me 
may review your work (no diff between different versions of patches) and 
continue to split your work in several patches.

Re: [PR] Add srvkey option to stick-table

[Thayne McCombs]

Here are my updates from the feedback.  It took me quite a while to figure out 
how to send this diff properly formatted with gmail.

If you would like a single patch with all my changes, I can send that as well.

>From 9c71b643e993dcafca36feae71cdfacc24ffaaa2 Mon Sep 17 00:00:00 2001
From: Thayne McCombs 
Date: Sat, 5 Dec 2020 23:09:24 -0700
Subject: [PATCH 1/2] Feedback from initial review

---
 doc/configuration.txt  |  4 ++--
 include/haproxy/dict.h |  1 +
 include/haproxy/server-t.h |  1 -
 src/cfgparse-listen.c  |  1 +
 src/dict.c | 24 +-
 src/peers.c|  7 ++-
 src/server.c   | 41 --
 src/stick_table.c  |  8 
 src/stream.c   |  2 +-
 src/tools.c|  3 ++-
 10 files changed, 66 insertions(+), 26 deletions(-)

diff --git a/doc/configuration.txt b/doc/configuration.txt
index 95c9abd8c..d1f7374ea 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -10711,8 +10711,8 @@ stick-table type {ip | integer | string [len ] 
| binary [len ]}
specifies how each server is identified for the purposes of the
stick table. The valid values are "name" and "addr". If "name" 
is
given, then  argument for the server (may be generated by
-   a template). If "addr" is given, then the server is idntified
-   by it's current network address, including the port. "addr" is
+   a template). If "addr" is given, then the server is identified
+   by its current network address, including the port. "addr" is
especially useful if you are using service discovery to generate
the addresses for servers with peered stick-tables and want
to consistently use the same host across peers for a stickiness
diff --git a/include/haproxy/dict.h b/include/haproxy/dict.h
index 59e81352c..c55834ca5 100644
--- a/include/haproxy/dict.h
+++ b/include/haproxy/dict.h
@@ -31,5 +31,6 @@
 
 struct dict *new_dict(const char *name);
 struct dict_entry *dict_insert(struct dict *d, char *str);
+void dict_entry_unref(struct dict *d, struct dict_entry *de);
 
 #endif  /* _HAPROXY_DICT_H */
diff --git a/include/haproxy/server-t.h b/include/haproxy/server-t.h
index 5a4dadfbd..13f5a5dab 100644
--- a/include/haproxy/server-t.h
+++ b/include/haproxy/server-t.h
@@ -274,7 +274,6 @@ struct server {
const struct netns_entry *netns;/* contains network namespace 
name or NULL. Network namespace comes from configuration */
/* warning, these structs are huge, keep them at the bottom */
struct sockaddr_storage addr;   /* the address to connect to, 
doesn't include the port */
-   char *addr_desc;/* string description of the 
address and port for the server */
struct xprt_ops *xprt;  /* transport-layer operations */
unsigned int svc_port;  /* the port to connect to (for 
relevant families) */
unsigned down_time; /* total time the server was 
down */
diff --git a/src/cfgparse-listen.c b/src/cfgparse-listen.c
index 97a97e746..a493e741c 100644
--- a/src/cfgparse-listen.c
+++ b/src/cfgparse-listen.c
@@ -457,6 +457,7 @@ int cfg_parse_listen(const char *file, int linenum, char 
**args, int kwm)
curproxy->grace  = defproxy.grace;
curproxy->conf.used_listener_id = EB_ROOT;
curproxy->conf.used_server_id = EB_ROOT;
+   curproxy->used_server_addr = EB_ROOT_UNIQUE;
 
if (defproxy.check_path)
curproxy->check_path = strdup(defproxy.check_path);
diff --git a/src/dict.c b/src/dict.c
index 903f07323..9b3536d96 100644
--- a/src/dict.c
+++ b/src/dict.c
@@ -87,8 +87,10 @@ struct dict_entry *dict_insert(struct dict *d, char *s)
HA_RWLOCK_RDLOCK(DICT_LOCK, >rwlock);
de = __dict_lookup(d, s);
HA_RWLOCK_RDUNLOCK(DICT_LOCK, >rwlock);
-   if (de)
+   if (de) {
+   HA_ATOMIC_ADD(>refcount, 1);
return de;
+   }
 
de = new_dict_entry(s);
if (!de)
@@ -105,3 +107,23 @@ struct dict_entry *dict_insert(struct dict *d, char *s)
return de;
 }
 
+
+/*
+ * Unreference a dict entry previously acquired with .
+ * If this is the last live reference to the entry, it is
+ * removed from the dictionary.
+ */
+void dict_entry_unref(struct dict *d, struct dict_entry *de)
+{
+   if (!de)
+   return;
+
+   if (HA_ATOMIC_SUB(>refcount, 1) != 0)
+   return;
+
+   HA_RWLOCK_WRLOCK(DICT_LOCK, >rwlock);
+   ebpt_delete(>value);
+   HA_RWLOCK_WRUNLOCK(DICT_LOCK, >rwlock);
+
+   free_dict_entry(de);
+}
diff --git a/src/peers.c b/src/peers.c
index 3f4e2c575..3fa1a28b4 100644
--- a/src/peers.c
+++ b/src/peers.c
@@ -1634,12 +1634,15 @@ static int 

Re: [PR] Add srvkey option to stick-table

[Frederic Lecaille]

On 12/2/20 8:48 AM, Thayne McCombs wrote:

Thanks for your feedback. I have some followup questions.


I think you should initialized this ebtree with EB_ROOT_UNIQUE as

value.

I'm not sure I understand what the distinction between EB_ROOT and
EB_ROOT_UNIQUE is. I'm happy to add this, but I'd like to understand
why it should be EB_ROOT_UNIQUE here (since it looks like it is
initialized as EB_ROOT for used_server_id and used_listener_id, and I
can't find where it is initialized for used_server_name, which I
believe would be equivalent since EB_ROOT is basically just zero
initialized).


EB_NORMAL ebtree can store entries with the same key contrary to 
EB_UNIQUE ebtre (see include/import/ebtree.h for the details.).


You are true about EB_ROOT initialization which is a zero initialization 
which is not necessary if the proxies are calloc()'ed. EB_ROOT_UNIQUE is 
not a zero initialization.



I think this is dangerous to initialize two objects with the same value

as a string. I would strdup()  to initialize s->addr_node.key.

ok. I was following the pattern used with conf.name in struct server.
Would it be better to use strdup, or to not have a separate field for
addr_desc, and just use addr_node.key (which is a void*)? If the
latter maybe rename addr_node as addr_desc?


Yes, this is the question I forgot to ask: if addr_desc field is not 
necessary, remove it. addr_node is ok for the name.



Additionally:

In srv_set_addr_desc, would it be better to hold the lock over the
entire operation, or just while we are actually manipulating the tree?
Meaning should I release the lock between deleting the old node and
inserting the new node? And should this use the existing proxy lock,
or should there be a separate dedicated lock for used_server_addr?


You must add a comment for this function about the lock for the server. 
The lock for the server must be hold. But at parsing time this is not 
necessary, so you do not have to lock the server.


Then you must lock the proxy to manipulate the ebtree attached to the 
proxy when you enter srv_set_addr_desc() , and release before leaving it.



I'm also a little bit concerned about leaking memory in the
server_key_dict. I couldn't find anything that cleans up these keys,
although there is a refcount on them. In an environment that uses
service discovery and the servers change frequently, this could lead
to a memory leak as this dict only grows in size.



The server names were not supposed to change. If the server addresses 
changes, we must remove the entries from the dictionary if they are no 
more used. I guess there may be several servers with the same address.

Re: [PR] Add srvkey option to stick-table

[Thayne McCombs]

Thanks for your feedback. I have some followup questions.

> I think you should initialized this ebtree with EB_ROOT_UNIQUE as
value.

I'm not sure I understand what the distinction between EB_ROOT and
EB_ROOT_UNIQUE is. I'm happy to add this, but I'd like to understand
why it should be EB_ROOT_UNIQUE here (since it looks like it is
initialized as EB_ROOT for used_server_id and used_listener_id, and I
can't find where it is initialized for used_server_name, which I
believe would be equivalent since EB_ROOT is basically just zero
initialized).

> I think this is dangerous to initialize two objects with the same value
as a string. I would strdup()  to initialize s->addr_node.key.

ok. I was following the pattern used with conf.name in struct server.
Would it be better to use strdup, or to not have a separate field for
addr_desc, and just use addr_node.key (which is a void*)? If the
latter maybe rename addr_node as addr_desc?

Additionally:

In srv_set_addr_desc, would it be better to hold the lock over the
entire operation, or just while we are actually manipulating the tree?
Meaning should I release the lock between deleting the old node and
inserting the new node? And should this use the existing proxy lock,
or should there be a separate dedicated lock for used_server_addr?

I'm also a little bit concerned about leaking memory in the
server_key_dict. I couldn't find anything that cleans up these keys,
although there is a refcount on them. In an environment that uses
service discovery and the servers change frequently, this could lead
to a memory leak as this dict only grows in size.

Thank you,

Thayne

Thayne McCombs
Senior Software Engineer
tha...@lucidchart.com


Lucid.co



On Tue, Dec 1, 2020 at 10:24 AM Frederic Lecaille  wrote:
>
> On 12/1/20 11:50 AM, Emeric Brun wrote:
> > Hi,
>
> Hi Thayne,
>
> See my answers below.
>
> > On 11/30/20 10:23 AM, PR Bot wrote:
> >> Dear list!
> >>
> >> Author: Thayne McCombs 
> >> Number of patches: 2
> >>
> >> This is an automated relay of the Github pull request:
> >> Add srvkey option to stick-table
> >>
> >> Patch title(s):
> >> Add srvkey option to stick-table
> >> Harden sa2str agains 107-byte-long abstract unix domain path
> >>
> >> Link:
> >> https://github.com/haproxy/haproxy/pull/979
> >>
> >> Edit locally:
> >> wget https://github.com/haproxy/haproxy/pull/979.patch && vi 979.patch
> >>
> >> Apply locally:
> >> curl https://github.com/haproxy/haproxy/pull/979.patch | git am -
> >>
> >> Description:
> >> This allows using the address of the server rather than the name of
> >> the
> >> server for keeping track of servers in a backend for
> >> stickiness.
> >>
> >> Fixes #814
> >>
> >> I haven't tested this at all
> >> yet, and it still needs some polish, but here is a draft of how to fix
> >> #814.
> >>
> >> This is my first significant contribution to haproxy,
> >> so I would not be surprised if I'm doing something terribly wrong, and
> >> I'm sure there are at least some small mistakes in it. Initial
> >> feedback would be very welcome.
>
> Thank you for this first contribution which looks almost good!!! We all
> do wrong things ;) Even if it not easy to comment your code because it
> is not included in this mail, let's giving a try.
>
>
> I have noticed two places where initializations are missing:
>
>
> diff --git a/include/haproxy/proxy-t.h b/include/haproxy/proxy-t.h
> index 998e210f6..e62b79765 100644
> --- a/include/haproxy/proxy-t.h
> +++ b/include/haproxy/proxy-t.h
> @@ -424,6 +424,7 @@ struct proxy {
> char *lfsd_file;/* file name where the 
> structured-data logformat
> string for RFC5424 appears (strdup) */
> int  lfsd_line; /* file name where the 
> structured-data logformat
> string for RFC5424 appears */
> } conf; /* config information */
> +   struct eb_root used_server_addr;/* list of server addresses in
> use */
>
>
> < used_server_addr> ebtree is not initialized. This would make haproxy
> crash. I think you should initialized this ebtree with EB_ROOT_UNIQUE as
> value.
>
>
> You should comment all the new functions (see srv_set_addr_desc() and
> perhaps others).
>
> The following function should be commente as this done for
> srv_set_dyncookie() espcially for the locks we need even if this
> function is only used at parsing time, as far as I see. What if we want
> to use it at runing time? This function should be called with a lock on
>  server (not necessary at parsing time).
>
>
> +static void srv_set_addr_desc(struct server *s)
> +{
> +   struct proxy *p = s->proxy;
> +   char *key;
> +
> +   key = sa2str(>addr, s->svc_port, s->flags & SRV_F_MAPPORTS);
> +
> +   if (strcmp(key, s->addr_desc) == 0) {
>
> ->addr_desc is initialized only by srv_set_addr_desc(). So the first
> time we enter this function ->addr_desc has NULL as value.
>
>

Re: [PR] Add srvkey option to stick-table

[Frederic Lecaille]

On 12/1/20 11:50 AM, Emeric Brun wrote:

Hi,


Hi Thayne,

See my answers below.


On 11/30/20 10:23 AM, PR Bot wrote:

Dear list!

Author: Thayne McCombs 
Number of patches: 2

This is an automated relay of the Github pull request:
Add srvkey option to stick-table

Patch title(s):
Add srvkey option to stick-table
Harden sa2str agains 107-byte-long abstract unix domain path

Link:
https://github.com/haproxy/haproxy/pull/979

Edit locally:
wget https://github.com/haproxy/haproxy/pull/979.patch && vi 979.patch

Apply locally:
curl https://github.com/haproxy/haproxy/pull/979.patch | git am -

Description:
This allows using the address of the server rather than the name of
the
server for keeping track of servers in a backend for
stickiness.

Fixes #814

I haven't tested this at all

yet, and it still needs some polish, but here is a draft of how to fix
#814.

This is my first significant contribution to haproxy,

so I would not be surprised if I'm doing something terribly wrong, and
I'm sure there are at least some small mistakes in it. Initial
feedback would be very welcome.


Thank you for this first contribution which looks almost good!!! We all 
do wrong things ;) Even if it not easy to comment your code because it 
is not included in this mail, let's giving a try.



I have noticed two places where initializations are missing:


diff --git a/include/haproxy/proxy-t.h b/include/haproxy/proxy-t.h
index 998e210f6..e62b79765 100644
--- a/include/haproxy/proxy-t.h
+++ b/include/haproxy/proxy-t.h
@@ -424,6 +424,7 @@ struct proxy {
 		char *lfsd_file;		/* file name where the structured-data logformat 
string for RFC5424 appears (strdup) */
 		int  lfsd_line;			/* file name where the structured-data logformat 
string for RFC5424 appears */

} conf; /* config information */
+	struct eb_root used_server_addr;/* list of server addresses in 
use */



< used_server_addr> ebtree is not initialized. This would make haproxy 
crash. I think you should initialized this ebtree with EB_ROOT_UNIQUE as 
value.



You should comment all the new functions (see srv_set_addr_desc() and 
perhaps others).


The following function should be commente as this done for 
srv_set_dyncookie() espcially for the locks we need even if this 
function is only used at parsing time, as far as I see. What if we want 
to use it at runing time? This function should be called with a lock on 
 server (not necessary at parsing time).



+static void srv_set_addr_desc(struct server *s)
+{
+   struct proxy *p = s->proxy;
+   char *key;
+
+   key = sa2str(>addr, s->svc_port, s->flags & SRV_F_MAPPORTS);
+
+   if (strcmp(key, s->addr_desc) == 0) {

->addr_desc is initialized only by srv_set_addr_desc(). So the first 
time we enter this function ->addr_desc has NULL as value.



+   free(key);
+   return;
+   }
+
+   HA_RWLOCK_WRLOCK(PROXY_LOCK, >lock);
+   ebpt_delete(>addr_node);
+   HA_RWLOCK_WRUNLOCK(PROXY_LOCK, >lock);
+
+   free(s->addr_desc);
+   s->addr_desc = key;
+   s->addr_node.key = key;

I think this is dangerous to initialize two objects with the same value 
as a string. I would strdup()  to initialize s->addr_node.key.


+   // TODO: should this use a dedicated lock?
+   HA_RWLOCK_WRLOCK(PROXY_LOCK, >lock);
+   ebis_insert(>used_server_addr, >addr_node);
+   HA_RWLOCK_WRUNLOCK(PROXY_LOCK, >lock);
+}
+



There are two typos in the documentation:

+  a template). If "addr" is given, then the server is idntified
^
+  by it's current network address
^


You should split your patch to provide such cleanup code in seperated patch:

@@ -1453,7 +1497,7 @@ int url2sa(const char *url, int ulen, struct 
sockaddr_storage *addr, struct spli

/* Secondly, if :// pattern is found, verify parsed stuff
 * before pattern is matching our http pattern.
 * If so parse ip address and port in uri.
-*
+*



I am not sure at all about this modification, but it semms 
implementation dependent to me:


diff --git a/src/tools.c b/src/tools.c
index c51f542c6..21e3f06ef 100644
--- a/src/tools.c
+++ b/src/tools.c
@@ -1228,7 +1228,7 @@ char * sa2str(const struct sockaddr_storage *addr, 
int port, int map_ports)

case AF_UNIX:
path = ((struct sockaddr_un *)addr)->sun_path;
if (path[0] == '\0') {
-   return memprintf(, "abns@%s", path+1);
+   return memprintf(, "abns@%107s", path+1);
} else {
return strdup(path);
}


I would not implement this function:

+static void srv_addr_updated(struct server *srv)
+{
+   srv_set_dyncookie(srv);
+   srv_set_addr_desc(srv);
+}


Note something important: there are 

Re: [PR] Add srvkey option to stick-table

[Emeric Brun]

Hi,
On 11/30/20 10:23 AM, PR Bot wrote:
> Dear list!
> 
> Author: Thayne McCombs 
> Number of patches: 2
> 
> This is an automated relay of the Github pull request:
>Add srvkey option to stick-table
> 
> Patch title(s): 
>Add srvkey option to stick-table
>Harden sa2str agains 107-byte-long abstract unix domain path
> 
> Link:
>https://github.com/haproxy/haproxy/pull/979
> 
> Edit locally:
>wget https://github.com/haproxy/haproxy/pull/979.patch && vi 979.patch
> 
> Apply locally:
>curl https://github.com/haproxy/haproxy/pull/979.patch | git am -
> 
> Description:
>This allows using the address of the server rather than the name of
>the
>server for keeping track of servers in a backend for
>stickiness.
>
>Fixes #814
>
>I haven't tested this at all
>yet, and it still needs some polish, but here is a draft of how to fix
>#814. 
>
>This is my first significant contribution to haproxy,
>so I would not be surprised if I'm doing something terribly wrong, and
>I'm sure there are at least some small mistakes in it. Initial
>feedback would be very welcome.
> 
> Instructions:
>This github pull request will be closed automatically; patch should be
>reviewed on the haproxy mailing list (haproxy@formilux.org). Everyone is
>invited to comment, even the patch's author. Please keep the author and
>list CCed in replies. Please note that in absence of any response this
>pull request will be lost.
> 


I'm CCing Fred

This pull request seems the extension for stick on server adresses we discussed 
a long time Ago when Fred implements
the stick on server name.

Willy exposed what to do here (using the dictionnary etc..)
https://github.com/haproxy/haproxy/issues/814

I think Fred is more accurate on the subject and implemntation to perform the 
pull request revue.

R,
Emeric

[PR] Add srvkey option to stick-table

[PR Bot]

Dear list!

Author: Thayne McCombs 
Number of patches: 2

This is an automated relay of the Github pull request:
   Add srvkey option to stick-table

Patch title(s): 
   Add srvkey option to stick-table
   Harden sa2str agains 107-byte-long abstract unix domain path

Link:
   https://github.com/haproxy/haproxy/pull/979

Edit locally:
   wget https://github.com/haproxy/haproxy/pull/979.patch && vi 979.patch

Apply locally:
   curl https://github.com/haproxy/haproxy/pull/979.patch | git am -

Description:
   This allows using the address of the server rather than the name of
   the
   server for keeping track of servers in a backend for
   stickiness.
   
   Fixes #814
   
   I haven't tested this at all
   yet, and it still needs some polish, but here is a draft of how to fix
   #814. 
   
   This is my first significant contribution to haproxy,
   so I would not be surprised if I'm doing something terribly wrong, and
   I'm sure there are at least some small mistakes in it. Initial
   feedback would be very welcome.

Instructions:
   This github pull request will be closed automatically; patch should be
   reviewed on the haproxy mailing list (haproxy@formilux.org). Everyone is
   invited to comment, even the patch's author. Please keep the author and
   list CCed in replies. Please note that in absence of any response this
   pull request will be lost.