Re: Warnings when using dynamic cookies and server-template

[Willy Tarreau]

Hi Olivier,

On Tue, Jan 23, 2018 at 01:39:24PM +0100, Olivier Houchard wrote:
> Willy, can you please push it ?

OK now pushed in 1.9, will backport soon.

thanks guys,
Willy

Re: Warnings when using dynamic cookies and server-template

[Olivier Houchard]

Hi William,

On Mon, Jan 22, 2018 at 08:03:55PM +0100, William Dauchy wrote:
> Hello Olivier,
> 
> On Wed, Jan 17, 2018 at 05:43:02PM +0100, Olivier Houchard wrote:
> > Ok you got me convinced, the attached patch don't check for duplicate
> > cookies for disabled server, until we enable them.
> 
> I took the time to test it on top of 1.8.x and it works as expected,
> removing the warnings.
> 
> Thanks,
> 

Thanks for testing !

Willy, can you please push it ?

Regards,

Olivier
>From cfc333d2b04686a3c488fdcb495cba64dbfec14b Mon Sep 17 00:00:00 2001
From: Olivier Houchard 
Date: Wed, 17 Jan 2018 17:39:34 +0100
Subject: [PATCH] MINOR: servers: Don't report duplicate dyncookies for
 disabled servers.

Especially with server-templates, it can happen servers starts with a
placeholder IP, in the disabled state. In this case, we don't want to report
that the same cookie was generated for multiple servers. So defer the test
until the server is enabled.

This should be backported to 1.8.
---
 src/server.c | 50 +++---
 1 file changed, 35 insertions(+), 15 deletions(-)

diff --git a/src/server.c b/src/server.c
index a37e91968..3901e7d8b 100644
--- a/src/server.c
+++ b/src/server.c
@@ -86,10 +86,34 @@ int srv_getinter(const struct check *check)
return (check->fastinter)?(check->fastinter):(check->inter);
 }
 
-void srv_set_dyncookie(struct server *s)
+/*
+ * Check that we did not get a hash collision.
+ * Unlikely, but it can happen.
+ */
+static inline void srv_check_for_dup_dyncookie(struct server *s)
 {
struct proxy *p = s->proxy;
struct server *tmpserv;
+
+   for (tmpserv = p->srv; tmpserv != NULL;
+   tmpserv = tmpserv->next) {
+   if (tmpserv == s)
+   continue;
+   if (tmpserv->next_admin & SRV_ADMF_FMAINT)
+   continue;
+   if (tmpserv->cookie &&
+   strcmp(tmpserv->cookie, s->cookie) == 0) {
+   ha_warning("We generated two equal cookies for two 
different servers.\n"
+  "Please change the secret key for '%s'.\n",
+  s->proxy->id);
+   }
+   }
+
+}
+
+void srv_set_dyncookie(struct server *s)
+{
+   struct proxy *p = s->proxy;
char *tmpbuf;
unsigned long long hash_value;
size_t key_len;
@@ -136,21 +160,13 @@ void srv_set_dyncookie(struct server *s)
if (!s->cookie)
return;
s->cklen = 16;
-   /*
-* Check that we did not get a hash collision.
-* Unlikely, but it can happen.
+
+   /* Don't bother checking if the dyncookie is duplicated if
+* the server is marked as "disabled", maybe it doesn't have
+* its real IP yet, but just a place holder.
 */
-   for (tmpserv = p->srv; tmpserv != NULL;
-   tmpserv = tmpserv->next) {
-   if (tmpserv == s)
-   continue;
-   if (tmpserv->cookie &&
-   strcmp(tmpserv->cookie, s->cookie) == 0) {
-   ha_warning("We generated two equal cookies for two 
different servers.\n"
-  "Please change the secret key for '%s'.\n",
-  s->proxy->id);
-   }
-   }
+   if (!(s->next_admin & SRV_ADMF_FMAINT))
+   srv_check_for_dup_dyncookie(s);
 }
 
 /*
@@ -4398,6 +4414,10 @@ static int cli_parse_enable_server(char **args, struct 
appctx *appctx, void *pri
return 1;
 
srv_adm_set_ready(sv);
+   if (!(sv->flags & SRV_F_COOKIESET)
+   && (sv->proxy->ck_opts & PR_CK_DYNAMIC) &&
+   sv->cookie)
+   srv_check_for_dup_dyncookie(sv);
return 1;
 }
 
-- 
2.14.3

Re: Warnings when using dynamic cookies and server-template

[William Dauchy]

Hello Olivier,

On Wed, Jan 17, 2018 at 05:43:02PM +0100, Olivier Houchard wrote:
> Ok you got me convinced, the attached patch don't check for duplicate
> cookies for disabled server, until we enable them.

I took the time to test it on top of 1.8.x and it works as expected,
removing the warnings.

Thanks,

> From cfc333d2b04686a3c488fdcb495cba64dbfec14b Mon Sep 17 00:00:00 2001
> From: Olivier Houchard 
> Date: Wed, 17 Jan 2018 17:39:34 +0100
> Subject: [PATCH] MINOR: servers: Don't report duplicate dyncookies for
>  disabled servers.
>
> Especially with server-templates, it can happen servers starts with a
> placeholder IP, in the disabled state. In this case, we don't want to report
> that the same cookie was generated for multiple servers. So defer the test
> until the server is enabled.
>
> This should be backported to 1.8.

Reported-by: Pierre Cheynier 
Tested-by: William Dauchy 

> ---
>  src/server.c | 50 +++---
>  1 file changed, 35 insertions(+), 15 deletions(-)
>
> diff --git a/src/server.c b/src/server.c
> index a37e91968..3901e7d8b 100644
> --- a/src/server.c
> +++ b/src/server.c
> @@ -86,10 +86,34 @@ int srv_getinter(const struct check *check)
>   return (check->fastinter)?(check->fastinter):(check->inter);
>  }
>
> -void srv_set_dyncookie(struct server *s)
> +/*
> + * Check that we did not get a hash collision.
> + * Unlikely, but it can happen.
> + */
> +static inline void srv_check_for_dup_dyncookie(struct server *s)
>  {
>   struct proxy *p = s->proxy;
>   struct server *tmpserv;
> +
> + for (tmpserv = p->srv; tmpserv != NULL;
> +tmpserv = tmpserv->next) {
> + if (tmpserv == s)
> + continue;
> + if (tmpserv->next_admin & SRV_ADMF_FMAINT)
> + continue;
> + if (tmpserv->cookie &&
> +strcmp(tmpserv->cookie, s->cookie) == 0) {
> + ha_warning("We generated two equal cookies for two different servers.\n"
> +   "Please change the secret key for '%s'.\n",
> +   s->proxy->id);
> + }
> + }
> +
> +}
> +
> +void srv_set_dyncookie(struct server *s)
> +{
> + struct proxy *p = s->proxy;
>   char *tmpbuf;
>   unsigned long long hash_value;
>   size_t key_len;
> @@ -136,21 +160,13 @@ void srv_set_dyncookie(struct server *s)
>   if (!s->cookie)
>   return;
>   s->cklen = 16;
> - /*
> - * Check that we did not get a hash collision.
> - * Unlikely, but it can happen.
> +
> + /* Don't bother checking if the dyncookie is duplicated if
> + * the server is marked as "disabled", maybe it doesn't have
> + * its real IP yet, but just a place holder.
>   */
> - for (tmpserv = p->srv; tmpserv != NULL;
> -tmpserv = tmpserv->next) {
> - if (tmpserv == s)
> - continue;
> - if (tmpserv->cookie &&
> -strcmp(tmpserv->cookie, s->cookie) == 0) {
> - ha_warning("We generated two equal cookies for two different servers.\n"
> -   "Please change the secret key for '%s'.\n",
> -   s->proxy->id);
> - }
> - }
> + if (!(s->next_admin & SRV_ADMF_FMAINT))
> + srv_check_for_dup_dyncookie(s);
>  }
>
>  /*
> @@ -4398,6 +4414,10 @@ static int cli_parse_enable_server(char **args, struct 
> appctx *appctx, void *pri
>   return 1;
>
>   srv_adm_set_ready(sv);
> + if (!(sv->flags & SRV_F_COOKIESET)
> +&& (sv->proxy->ck_opts & PR_CK_DYNAMIC) &&
> +sv->cookie)
> + srv_check_for_dup_dyncookie(sv);
>   return 1;
>  }
>
> --
> 2.14.3

Re: Warnings when using dynamic cookies and server-template

[William Dauchy]

On Wed, Jan 17, 2018 at 05:43:02PM +0100, Olivier Houchard wrote:
> Ok you got me convinced, the attached patch don't check for duplicate
> cookies for disabled server, until we enable them.

I also prefer this approach.

> From cfc333d2b04686a3c488fdcb495cba64dbfec14b Mon Sep 17 00:00:00 2001
> From: Olivier Houchard 
> Date: Wed, 17 Jan 2018 17:39:34 +0100
> Subject: [PATCH] MINOR: servers: Don't report duplicate dyncookies for
>  disabled servers.
> 
> Especially with server-templates, it can happen servers starts with a
> placeholder IP, in the disabled state. In this case, we don't want to report
> that the same cookie was generated for multiple servers. So defer the test
> until the server is enabled.
> 
> This should be backported to 1.8.
> ---
>  src/server.c | 50 +++---
>  1 file changed, 35 insertions(+), 15 deletions(-)
> 
> diff --git a/src/server.c b/src/server.c
> index a37e91968..3901e7d8b 100644
> --- a/src/server.c
> +++ b/src/server.c
> @@ -86,10 +86,34 @@ int srv_getinter(const struct check *check)
>   return (check->fastinter)?(check->fastinter):(check->inter);
>  }
>  
> -void srv_set_dyncookie(struct server *s)
> +/*
> + * Check that we did not get a hash collision.
> + * Unlikely, but it can happen.
> + */
> +static inline void srv_check_for_dup_dyncookie(struct server *s)
>  {
>   struct proxy *p = s->proxy;
>   struct server *tmpserv;
> +
> + for (tmpserv = p->srv; tmpserv != NULL;
> + tmpserv = tmpserv->next) {
> + if (tmpserv == s)
> + continue;
> + if (tmpserv->next_admin & SRV_ADMF_FMAINT)
> + continue;
> + if (tmpserv->cookie &&
> + strcmp(tmpserv->cookie, s->cookie) == 0) {
> + ha_warning("We generated two equal cookies for two 
> different servers.\n"
> +"Please change the secret key for '%s'.\n",
> +s->proxy->id);
> + }
> + }
> +
> +}
> +
> +void srv_set_dyncookie(struct server *s)
> +{
> + struct proxy *p = s->proxy;
>   char *tmpbuf;
>   unsigned long long hash_value;
>   size_t key_len;
> @@ -136,21 +160,13 @@ void srv_set_dyncookie(struct server *s)
>   if (!s->cookie)
>   return;
>   s->cklen = 16;
> - /*
> -  * Check that we did not get a hash collision.
> -  * Unlikely, but it can happen.
> +
> + /* Don't bother checking if the dyncookie is duplicated if
> +  * the server is marked as "disabled", maybe it doesn't have
> +  * its real IP yet, but just a place holder.
>*/
> - for (tmpserv = p->srv; tmpserv != NULL;
> - tmpserv = tmpserv->next) {
> - if (tmpserv == s)
> - continue;
> - if (tmpserv->cookie &&
> - strcmp(tmpserv->cookie, s->cookie) == 0) {
> - ha_warning("We generated two equal cookies for two 
> different servers.\n"
> -"Please change the secret key for '%s'.\n",
> -s->proxy->id);
> - }
> - }
> + if (!(s->next_admin & SRV_ADMF_FMAINT))
> + srv_check_for_dup_dyncookie(s);
>  }
>  
>  /*
> @@ -4398,6 +4414,10 @@ static int cli_parse_enable_server(char **args, struct 
> appctx *appctx, void *pri
>   return 1;
>  
>   srv_adm_set_ready(sv);
> + if (!(sv->flags & SRV_F_COOKIESET)
> + && (sv->proxy->ck_opts & PR_CK_DYNAMIC) &&
> + sv->cookie)
> + srv_check_for_dup_dyncookie(sv);
>   return 1;
>  }
>  
> -- 
> 2.14.3
> 

Re: Warnings when using dynamic cookies and server-template

[Olivier Houchard]

On Wed, Jan 17, 2018 at 04:42:01PM +0100, Pierre Cheynier wrote:
> On 17/01/2018 15:56, Olivier Houchard wrote:
> >
> >> So, as a conclusion, I'm just not sure that producing this warning is
> >> relevant in case the IP is duplicated for several servers *if they are
> >> disabled*...
> > Or maybe we should just advocate using 0.0.0.0 when we mean "no IP" :)
> Not sure about that, 0.0.0.0 is not valid in the config in this case but
> it is in some others (thinking about "bind" for ex.)
> 
> Advertising it can be a bit confusing.
> 
> > It would be a bit painful, though doable, to don't check if the server is
> > disabled, but to add the check when server is enabled. I don't know if
> > it's worth it.
> >
> 
> To me this would be the best (functionally speaking, don't know about
> the performances aspects :) ), since in the context of cookies, you
> probably doing it wrong if you enable 2 servers with the same IP.
> 
> But server-templates are a good use-case in which you want to be able to
> do what you want with disabled servers.
> 
> To be discussed with people on your side ? we can clearly live with both
> options.
> 

Ok you got me convinced, the attached patch don't check for duplicate
cookies for disabled server, until we enable them.

Regards,

Olivier
>From cfc333d2b04686a3c488fdcb495cba64dbfec14b Mon Sep 17 00:00:00 2001
From: Olivier Houchard 
Date: Wed, 17 Jan 2018 17:39:34 +0100
Subject: [PATCH] MINOR: servers: Don't report duplicate dyncookies for
 disabled servers.

Especially with server-templates, it can happen servers starts with a
placeholder IP, in the disabled state. In this case, we don't want to report
that the same cookie was generated for multiple servers. So defer the test
until the server is enabled.

This should be backported to 1.8.
---
 src/server.c | 50 +++---
 1 file changed, 35 insertions(+), 15 deletions(-)

diff --git a/src/server.c b/src/server.c
index a37e91968..3901e7d8b 100644
--- a/src/server.c
+++ b/src/server.c
@@ -86,10 +86,34 @@ int srv_getinter(const struct check *check)
return (check->fastinter)?(check->fastinter):(check->inter);
 }
 
-void srv_set_dyncookie(struct server *s)
+/*
+ * Check that we did not get a hash collision.
+ * Unlikely, but it can happen.
+ */
+static inline void srv_check_for_dup_dyncookie(struct server *s)
 {
struct proxy *p = s->proxy;
struct server *tmpserv;
+
+   for (tmpserv = p->srv; tmpserv != NULL;
+   tmpserv = tmpserv->next) {
+   if (tmpserv == s)
+   continue;
+   if (tmpserv->next_admin & SRV_ADMF_FMAINT)
+   continue;
+   if (tmpserv->cookie &&
+   strcmp(tmpserv->cookie, s->cookie) == 0) {
+   ha_warning("We generated two equal cookies for two 
different servers.\n"
+  "Please change the secret key for '%s'.\n",
+  s->proxy->id);
+   }
+   }
+
+}
+
+void srv_set_dyncookie(struct server *s)
+{
+   struct proxy *p = s->proxy;
char *tmpbuf;
unsigned long long hash_value;
size_t key_len;
@@ -136,21 +160,13 @@ void srv_set_dyncookie(struct server *s)
if (!s->cookie)
return;
s->cklen = 16;
-   /*
-* Check that we did not get a hash collision.
-* Unlikely, but it can happen.
+
+   /* Don't bother checking if the dyncookie is duplicated if
+* the server is marked as "disabled", maybe it doesn't have
+* its real IP yet, but just a place holder.
 */
-   for (tmpserv = p->srv; tmpserv != NULL;
-   tmpserv = tmpserv->next) {
-   if (tmpserv == s)
-   continue;
-   if (tmpserv->cookie &&
-   strcmp(tmpserv->cookie, s->cookie) == 0) {
-   ha_warning("We generated two equal cookies for two 
different servers.\n"
-  "Please change the secret key for '%s'.\n",
-  s->proxy->id);
-   }
-   }
+   if (!(s->next_admin & SRV_ADMF_FMAINT))
+   srv_check_for_dup_dyncookie(s);
 }
 
 /*
@@ -4398,6 +4414,10 @@ static int cli_parse_enable_server(char **args, struct 
appctx *appctx, void *pri
return 1;
 
srv_adm_set_ready(sv);
+   if (!(sv->flags & SRV_F_COOKIESET)
+   && (sv->proxy->ck_opts & PR_CK_DYNAMIC) &&
+   sv->cookie)
+   srv_check_for_dup_dyncookie(sv);
return 1;
 }
 
-- 
2.14.3

Re: Warnings when using dynamic cookies and server-template

[Pierre Cheynier]

On 17/01/2018 15:56, Olivier Houchard wrote:
>
>> So, as a conclusion, I'm just not sure that producing this warning is
>> relevant in case the IP is duplicated for several servers *if they are
>> disabled*...
> Or maybe we should just advocate using 0.0.0.0 when we mean "no IP" :)
Not sure about that, 0.0.0.0 is not valid in the config in this case but
it is in some others (thinking about "bind" for ex.)

Advertising it can be a bit confusing.

> It would be a bit painful, though doable, to don't check if the server is
> disabled, but to add the check when server is enabled. I don't know if
> it's worth it.
>

To me this would be the best (functionally speaking, don't know about
the performances aspects :) ), since in the context of cookies, you
probably doing it wrong if you enable 2 servers with the same IP.

But server-templates are a good use-case in which you want to be able to
do what you want with disabled servers.

To be discussed with people on your side ? we can clearly live with both
options.

Pierre

Re: Warnings when using dynamic cookies and server-template

[Olivier Houchard]

On Wed, Jan 17, 2018 at 02:25:59PM +0100, Pierre Cheynier wrote:
> Hi,
> 
> On 16/01/2018 18:48, Olivier Houchard wrote:
> >
> > Not really :) That's not a case I thought of.
> > The attached patch disables the generation of the dynamic cookie if the IP
> > is 0.0.0.0 or ::, so that it only gets generated when the server gets a real
> > IP. Is it OK with you ?
> I'm not sure this will fix the issue.
> In this blogpost
> https://www.haproxy.com/fr/blog/dynamic-scaling-for-microservices-with-runtime-api/,
> the example given is
> 
> server-template websrv 1-100192.168.122.1:8080check disabled
> 
> Which in fact will do exactly the same as 1 hundred times
> 
> server websrvN 192.168.122.1:8080 check disabled
> 
> right ?
> (I just confirmed that BTW, by switching my 'high-level' templating to
> HAProxy's one, and I reproduce the issue).
> 
> So, as a conclusion, I'm just not sure that producing this warning is
> relevant in case the IP is duplicated for several servers *if they are
> disabled*...
> 

Or maybe we should just advocate using 0.0.0.0 when we mean "no IP" :)
It would be a bit painful, though doable, to don't check if the server is
disabled, but to add the check when server is enabled. I don't know if
it's worth it.

Regards,

Olivier

Re: Warnings when using dynamic cookies and server-template

[Pierre Cheynier]

Hi,

On 16/01/2018 18:48, Olivier Houchard wrote:
>
> Not really :) That's not a case I thought of.
> The attached patch disables the generation of the dynamic cookie if the IP
> is 0.0.0.0 or ::, so that it only gets generated when the server gets a real
> IP. Is it OK with you ?
I'm not sure this will fix the issue.
In this blogpost
https://www.haproxy.com/fr/blog/dynamic-scaling-for-microservices-with-runtime-api/,
the example given is

server-template websrv 1-100192.168.122.1:8080check disabled

Which in fact will do exactly the same as 1 hundred times

server websrvN 192.168.122.1:8080 check disabled

right ?
(I just confirmed that BTW, by switching my 'high-level' templating to
HAProxy's one, and I reproduce the issue).

So, as a conclusion, I'm just not sure that producing this warning is
relevant in case the IP is duplicated for several servers *if they are
disabled*...

Pierre

Re: Warnings when using dynamic cookies and server-template

[Olivier Houchard]

Hi Pierre,

On Tue, Jan 16, 2018 at 06:08:40PM +0100, Pierre Cheynier wrote:
> Hi Olivier,
> 
> 
> On 16/01/2018 15:43, Olivier Houchard wrote:
> > I'm not so sure about this.
> > It won't be checked again when server are enabled, so you won't get the
> > warning if it's still the case.
> > You shouldn't get those warnings unless multiple servers have the same IP,
> > though. What does your server-template look like ?
> In fact, it's a confusion between 3 level of templating... /
> At the end it's not a server template in HAProxy, but raw servers, in a
> disabled state:
> 
> backend be_service
>     server srv0 10.0.0.2:1234
>     server srv1 0.0.0.0:0 check disabled
>     server srv2 0.0.0.0:0 check disabled
>     server srv3 0.0.0.0:0 check disabled
>     server srv4 0.0.0.0:0 check disabled
> 
> So I guess this warning was intended to cover this case, right ?
> 

Not really :) That's not a case I thought of.
The attached patch disables the generation of the dynamic cookie if the IP
is 0.0.0.0 or ::, so that it only gets generated when the server gets a real
IP. Is it OK with you ?

Regards,

Olivier
>From ede677c8625fed5984d98ccaddcba2eee12d6c43 Mon Sep 17 00:00:00 2001
From: Olivier Houchard 
Date: Tue, 16 Jan 2018 18:42:07 +0100
Subject: [PATCH] MINOR: server: Don't generate dynamic cookies if addr is
 INADDR_ANY.

Don't attempt to generate a dynamic cookie if the server address is set
to INADDR_ANY or IN6ADDR_ANY, that makes little sense, and produce
useless warning if there are multiple such servers.

This should be backported to 1.8.
---
 src/server.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/server.c b/src/server.c
index a37e91968..03caf3772 100644
--- a/src/server.c
+++ b/src/server.c
@@ -103,8 +103,8 @@ void srv_set_dyncookie(struct server *s)
return;
key_len = strlen(p->dyncookie_key);
 
-   if (s->addr.ss_family != AF_INET &&
-   s->addr.ss_family != AF_INET6)
+   if (s->addr.ss_family == AF_UNIX ||
+   !is_addr(>addr))
return;
/*
 * Buffer to calculate the cookie value.
-- 
2.14.3

Re: Warnings when using dynamic cookies and server-template

[Pierre Cheynier]

Hi Olivier,


On 16/01/2018 15:43, Olivier Houchard wrote:
> I'm not so sure about this.
> It won't be checked again when server are enabled, so you won't get the
> warning if it's still the case.
> You shouldn't get those warnings unless multiple servers have the same IP,
> though. What does your server-template look like ?
In fact, it's a confusion between 3 level of templating... /
At the end it's not a server template in HAProxy, but raw servers, in a
disabled state:

backend be_service
    server srv0 10.0.0.2:1234
    server srv1 0.0.0.0:0 check disabled
    server srv2 0.0.0.0:0 check disabled
    server srv3 0.0.0.0:0 check disabled
    server srv4 0.0.0.0:0 check disabled

So I guess this warning was intended to cover this case, right ?

Pierre

Re: Warnings when using dynamic cookies and server-template

[Olivier Houchard]

Hi Pierre,

On Mon, Jan 15, 2018 at 06:45:52PM +0100, Pierre Cheynier wrote:
> Hello,
> 
> We started to use the server-template approach in which you basically
> provision servers in backends using a "check disabled" state, then
> re-enabling them using the Runtime API.
> 
> I recently noticed that when used with dynamic cookies, we end up
> getting these warnings:
> 
> haproxy.c:149
> 
>     ha_warning("We generated two equal cookies for
> two different servers.\n"
>    "Please change the secret key for
> '%s'.\n",
>    s->proxy->id);
> 
> The check seems consistent, but not in the case of disabled servers,
> what do you think ?
> 

I'm not so sure about this.
It won't be checked again when server are enabled, so you won't get the
warning if it's still the case.
You shouldn't get those warnings unless multiple servers have the same IP,
though. What does your server-template look like ?

Thanks !

Olivier