Hi all,

Jude (and FrancisL) reported an issue when using DNS SRV resolution coupled
with server-state: the state of the servers were not properly re-applied to
the new process.
This patch improve the server-state file to fix this issue: the srv record
used to manage this server is now saved by the previous process and changes
can be re-applied by the new one (unless the SRV record has changed, of
course)

Thx to Jude who provided time and resources to be able to reproduce a last
bug.

There are 2 patches, one for -dev and one for 1.8 (since server.c content
has changed a bit between those 2 versions).

Baptiste
From 42dc52b1a992212e31b67a31441036b494a3d935 Mon Sep 17 00:00:00 2001
From: Baptiste Assmann <bed...@gmail.com>
Date: Tue, 4 Sep 2018 09:57:17 +0200
Subject: [PATCH] BUG/MEDIUM: dns/server: fix incomatibility between SRV
 resolution and server state file

Server state file has no indication that a server is currently managed
by a DNS SRV resolution.
And thus, both feature (DNS SRV resolution and server state), when used
together, does not provide the expected behavior: a smooth experience...

This patch introduce the "SRV record name" in the server state file and
loads and applies it if found and wherever required.

This patch applies to HAProxy 1.8 branch only.
---
 doc/management.txt     |  1 +
 include/types/server.h |  7 ++++---
 src/proxy.c            | 10 ++++++++--
 src/server.c           | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 66 insertions(+), 5 deletions(-)

diff --git a/doc/management.txt b/doc/management.txt
index 46e7fd0..35bc3a9 100644
--- a/doc/management.txt
+++ b/doc/management.txt
@@ -2094,6 +2094,7 @@ show servers state [<backend>]
                                   configuration.
      srv_fqdn:                    Server FQDN.
      srv_port:                    Server port.
+     srvrecord:                   DNS SRV record associated to this SRV.
 
 show sess
   Dump all known sessions. Avoid doing this on slow connections as this can
diff --git a/include/types/server.h b/include/types/server.h
index fd1dad5..fd3c8ba 100644
--- a/include/types/server.h
+++ b/include/types/server.h
@@ -126,10 +126,11 @@ enum srv_initaddr {
     "bk_f_forced_id "             \
     "srv_f_forced_id "            \
     "srv_fqdn "                   \
-    "srv_port"
+    "srv_port "                   \
+    "srvrecord"
 
-#define SRV_STATE_FILE_MAX_FIELDS 19
-#define SRV_STATE_FILE_NB_FIELDS_VERSION_1 18
+#define SRV_STATE_FILE_MAX_FIELDS 20
+#define SRV_STATE_FILE_NB_FIELDS_VERSION_1 19
 #define SRV_STATE_LINE_MAXLEN 512
 
 /* server flags -- 32 bits */
diff --git a/src/proxy.c b/src/proxy.c
index 8926ba8..cff6c0a 100644
--- a/src/proxy.c
+++ b/src/proxy.c
@@ -1429,6 +1429,7 @@ static int dump_servers_state(struct stream_interface *si, struct chunk *buf)
 	char srv_addr[INET6_ADDRSTRLEN + 1];
 	time_t srv_time_since_last_change;
 	int bk_f_forced_id, srv_f_forced_id;
+	char *srvrecord;
 
 	/* we don't want to report any state if the backend is not enabled on this process */
 	if (px->bind_proc && !(px->bind_proc & pid_bit))
@@ -1458,18 +1459,23 @@ static int dump_servers_state(struct stream_interface *si, struct chunk *buf)
 		bk_f_forced_id = px->options & PR_O_FORCED_ID ? 1 : 0;
 		srv_f_forced_id = srv->flags & SRV_F_FORCED_ID ? 1 : 0;
 
+		srvrecord = NULL;
+		if (srv->srvrq && srv->srvrq->name)
+			srvrecord = srv->srvrq->name;
+
 		chunk_appendf(buf,
 				"%d %s "
 				"%d %s %s "
 				"%d %d %d %d %ld "
 				"%d %d %d %d %d "
-				"%d %d %s %u"
+				"%d %d %s %u %s"
 				"\n",
 				px->uuid, px->id,
 				srv->puid, srv->id, srv_addr,
 				srv->cur_state, srv->cur_admin, srv->uweight, srv->iweight, (long int)srv_time_since_last_change,
 				srv->check.status, srv->check.result, srv->check.health, srv->check.state, srv->agent.state,
-				bk_f_forced_id, srv_f_forced_id, srv->hostname ? srv->hostname : "-", srv->svc_port);
+				bk_f_forced_id, srv_f_forced_id, srv->hostname ? srv->hostname : "-", srv->svc_port,
+				srvrecord ? srvrecord : "-");
 		if (ci_putchk(si_ic(si), &trash) == -1) {
 			si_applet_cant_put(si);
 			return 0;
diff --git a/src/server.c b/src/server.c
index 98dae53..3962b0a 100644
--- a/src/server.c
+++ b/src/server.c
@@ -2667,6 +2667,7 @@ static void srv_update_state(struct server *srv, int version, char **params)
 	const char *fqdn;
 	const char *port_str;
 	unsigned int port;
+	char *srvrecord;
 
 	fqdn = NULL;
 	port = 0;
@@ -2690,6 +2691,7 @@ static void srv_update_state(struct server *srv, int version, char **params)
 			 * srv_f_forced_id:      params[12]
 			 * srv_fqdn:             params[13]
 			 * srv_port:             params[14]
+			 * srvrecord:            params[15]
 			 */
 
 			/* validating srv_op_state */
@@ -2822,6 +2824,13 @@ static void srv_update_state(struct server *srv, int version, char **params)
 				}
 			}
 
+			/* SRV record
+			 * NOTE: in HAProxy, SRV records must start with an underscore '_'
+			 */
+			srvrecord = params[15];
+			if (srvrecord && *srvrecord != '_')
+				srvrecord = NULL;
+
 			/* don't apply anything if one error has been detected */
 			if (msg->len)
 				goto out;
@@ -2954,6 +2963,49 @@ static void srv_update_state(struct server *srv, int version, char **params)
 					}
 				}
 			}
+			/* If all the conditions below are validated, this means
+			 * we're evaluating a server managed by SRV resolution
+			 */
+			else if (fqdn && !srv->hostname && srvrecord) {
+				int res;
+
+				/* we can't apply previous state if SRV record has changed */
+				if (srv->srvrq && strcmp(srv->srvrq->name, srvrecord) != 0) {
+					chunk_appendf(msg, ", SRV record mismatch between configuration ('%s') and state file ('%s) for server '%s'. Previous state not applied", srv->srvrq->name, srvrecord, srv->id);
+					HA_SPIN_UNLOCK(SERVER_LOCK, &srv->lock);
+					goto out;
+				}
+
+				/* create or find a SRV resolution for this srv record */
+				if (srv->srvrq == NULL && (srv->srvrq = find_srvrq_by_name(srvrecord, srv->proxy)) == NULL)
+					srv->srvrq = new_dns_srvrq(srv, srvrecord);
+				if (srv->srvrq == NULL) {
+					chunk_appendf(msg, ", can't create or find SRV resolution '%s' for server '%s'", srvrecord, srv->id);
+					HA_SPIN_UNLOCK(SERVER_LOCK, &srv->lock);
+					goto out;
+				}
+
+				/* prepare DNS resolution for this server */
+				res = srv_prepare_for_resolution(srv, fqdn);
+				if (res == -1) {
+					chunk_appendf(msg, ", can't allocate memory for DNS resolution for server '%s'", srv->id);
+					HA_SPIN_UNLOCK(SERVER_LOCK, &srv->lock);
+					goto out;
+				}
+
+				/* configure check.port accordingly */
+				if ((srv->check.state & CHK_ST_CONFIGURED) &&
+				    !(srv->flags & SRV_F_CHECKPORT))
+					srv->check.port = port;
+
+				/* Unset SRV_F_MAPPORTS for SRV records.
+				 * SRV_F_MAPPORTS is unfortunately set by parse_server()
+				 * because no ports are provided in the configuration file.
+				 * This is because HAProxy will use the port found into the SRV record.
+				 */
+				srv->flags &= ~SRV_F_MAPPORTS;
+			}
+
 
 			if (port_str)
 				srv->svc_port = port;
@@ -3208,6 +3260,7 @@ void apply_server_state(void)
 							 * srv_f_forced_id:      params[16] => srv_params[12]
 							 * srv_fqdn:             params[17] => srv_params[13]
 							 * srv_port:             params[18] => srv_params[14]
+							 * srvrecord:            params[19] => srv_params[15]
 							 */
 							if (arg >= 4) {
 								srv_params[srv_arg] = cur;
-- 
2.7.4

From 3bdac16b6cd6986393c9a332c92abbcb59f8ed64 Mon Sep 17 00:00:00 2001
From: Baptiste Assmann <bed...@gmail.com>
Date: Mon, 2 Jul 2018 17:00:54 +0200
Subject: [PATCH 1/3] BUG/MEDIUM: dns/server: fix incomatibility between SRV
 resolution and server state file

Server state file has no indication that a server is currently managed
by a DNS SRV resolution.
And thus, both feature (DNS SRV resolution and server state), when used
together, does not provide the expected behavior: a smooth experience...

This patch introduce the "SRV record name" in the server state file and
loads and applies it if found and wherever required.

This patch applies to haproxy-dev branch only. For backport, a specific patch
is provided for 1.8.

---
 doc/management.txt     |  1 +
 include/types/server.h |  7 ++++---
 src/proxy.c            | 10 ++++++++--
 src/server.c           | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 65 insertions(+), 5 deletions(-)

diff --git a/doc/management.txt b/doc/management.txt
index c78dc74..17f1505 100644
--- a/doc/management.txt
+++ b/doc/management.txt
@@ -2130,6 +2130,7 @@ show servers state [<backend>]
                                   configuration.
      srv_fqdn:                    Server FQDN.
      srv_port:                    Server port.
+     srvrecord:                   DNS SRV record associated to this SRV.
 
 show sess
   Dump all known sessions. Avoid doing this on slow connections as this can
diff --git a/include/types/server.h b/include/types/server.h
index 8825928..8adc29b 100644
--- a/include/types/server.h
+++ b/include/types/server.h
@@ -126,10 +126,11 @@ enum srv_initaddr {
     "bk_f_forced_id "             \
     "srv_f_forced_id "            \
     "srv_fqdn "                   \
-    "srv_port"
+    "srv_port "                   \
+    "srvrecord"
 
-#define SRV_STATE_FILE_MAX_FIELDS 19
-#define SRV_STATE_FILE_NB_FIELDS_VERSION_1 18
+#define SRV_STATE_FILE_MAX_FIELDS 20
+#define SRV_STATE_FILE_NB_FIELDS_VERSION_1 19
 #define SRV_STATE_LINE_MAXLEN 512
 
 /* server flags -- 32 bits */
diff --git a/src/proxy.c b/src/proxy.c
index 73aa118..c939e82 100644
--- a/src/proxy.c
+++ b/src/proxy.c
@@ -1429,6 +1429,7 @@ static int dump_servers_state(struct stream_interface *si, struct buffer *buf)
 	char srv_addr[INET6_ADDRSTRLEN + 1];
 	time_t srv_time_since_last_change;
 	int bk_f_forced_id, srv_f_forced_id;
+	char *srvrecord;
 
 	/* we don't want to report any state if the backend is not enabled on this process */
 	if (px->bind_proc && !(px->bind_proc & pid_bit))
@@ -1458,18 +1459,23 @@ static int dump_servers_state(struct stream_interface *si, struct buffer *buf)
 		bk_f_forced_id = px->options & PR_O_FORCED_ID ? 1 : 0;
 		srv_f_forced_id = srv->flags & SRV_F_FORCED_ID ? 1 : 0;
 
+		srvrecord = NULL;
+		if (srv->srvrq && srv->srvrq->name)
+			srvrecord = srv->srvrq->name;
+
 		chunk_appendf(buf,
 				"%d %s "
 				"%d %s %s "
 				"%d %d %d %d %ld "
 				"%d %d %d %d %d "
-				"%d %d %s %u"
+				"%d %d %s %u %s"
 				"\n",
 				px->uuid, px->id,
 				srv->puid, srv->id, srv_addr,
 				srv->cur_state, srv->cur_admin, srv->uweight, srv->iweight, (long int)srv_time_since_last_change,
 				srv->check.status, srv->check.result, srv->check.health, srv->check.state, srv->agent.state,
-				bk_f_forced_id, srv_f_forced_id, srv->hostname ? srv->hostname : "-", srv->svc_port);
+				bk_f_forced_id, srv_f_forced_id, srv->hostname ? srv->hostname : "-", srv->svc_port,
+				srvrecord ? srvrecord : "-");
 		if (ci_putchk(si_ic(si), &trash) == -1) {
 			si_applet_cant_put(si);
 			return 0;
diff --git a/src/server.c b/src/server.c
index 78d5a0f..766f5de 100644
--- a/src/server.c
+++ b/src/server.c
@@ -2758,6 +2758,7 @@ static void srv_update_state(struct server *srv, int version, char **params)
 	const char *fqdn;
 	const char *port_str;
 	unsigned int port;
+	char *srvrecord;
 
 	fqdn = NULL;
 	port = 0;
@@ -2781,6 +2782,7 @@ static void srv_update_state(struct server *srv, int version, char **params)
 			 * srv_f_forced_id:      params[12]
 			 * srv_fqdn:             params[13]
 			 * srv_port:             params[14]
+			 * srvrecord:            params[15]
 			 */
 
 			/* validating srv_op_state */
@@ -2913,6 +2915,13 @@ static void srv_update_state(struct server *srv, int version, char **params)
 				}
 			}
 
+			/* SRV record
+			 * NOTE: in HAProxy, SRV records must start with an underscore '_'
+			 */
+			srvrecord = params[15];
+			if (srvrecord && *srvrecord != '_')
+				srvrecord = NULL;
+
 			/* don't apply anything if one error has been detected */
 			if (msg->data)
 				goto out;
@@ -3045,6 +3054,48 @@ static void srv_update_state(struct server *srv, int version, char **params)
 					}
 				}
 			}
+			/* If all the conditions below are validated, this means
+			 * we're evaluating a server managed by SRV resolution
+			 */
+			else if (fqdn && !srv->hostname && srvrecord) {
+				int res;
+
+				/* we can't apply previous state if SRV record has changed */
+				if (srv->srvrq && strcmp(srv->srvrq->name, srvrecord) != 0) {
+					chunk_appendf(msg, ", SRV record mismatch between configuration ('%s') and state file ('%s) for server '%s'. Previous state not applied", srv->srvrq->name, srvrecord, srv->id);
+					HA_SPIN_UNLOCK(SERVER_LOCK, &srv->lock);
+					goto out;
+				}
+
+				/* create or find a SRV resolution for this srv record */
+				if (srv->srvrq == NULL && (srv->srvrq = find_srvrq_by_name(srvrecord, srv->proxy)) == NULL)
+					srv->srvrq = new_dns_srvrq(srv, srvrecord);
+				if (srv->srvrq == NULL) {
+					chunk_appendf(msg, ", can't create or find SRV resolution '%s' for server '%s'", srvrecord, srv->id);
+					HA_SPIN_UNLOCK(SERVER_LOCK, &srv->lock);
+					goto out;
+				}
+
+				/* prepare DNS resolution for this server */
+				res = srv_prepare_for_resolution(srv, fqdn);
+				if (res == -1) {
+					chunk_appendf(msg, ", can't allocate memory for DNS resolution for server '%s'", srv->id);
+					HA_SPIN_UNLOCK(SERVER_LOCK, &srv->lock);
+					goto out;
+				}
+
+				/* configure check.port accordingly */
+				if ((srv->check.state & CHK_ST_CONFIGURED) &&
+				    !(srv->flags & SRV_F_CHECKPORT))
+					srv->check.port = port;
+
+				/* Unset SRV_F_MAPPORTS for SRV records.
+				 * SRV_F_MAPPORTS is unfortunately set by parse_server()
+				 * because no ports are provided in the configuration file.
+				 * This is because HAProxy will use the port found into the SRV record.
+				 */
+				srv->flags &= ~SRV_F_MAPPORTS;
+			}
 
 			if (port_str)
 				srv->svc_port = port;
@@ -3301,6 +3352,7 @@ void apply_server_state(void)
 							 * srv_f_forced_id:      params[16] => srv_params[12]
 							 * srv_fqdn:             params[17] => srv_params[13]
 							 * srv_port:             params[18] => srv_params[14]
+							 * srvrecord:            params[19] => srv_params[15]
 							 */
 							if (arg >= 4) {
 								srv_params[srv_arg] = cur;
-- 
2.7.4

Reply via email to