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 <ohouch...@haproxy.com>
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
