BUG/MINOR: server/event_hdl: propagate map port info through inetaddr event

server addr:svc_port updates during runtime might set or clear the
SRV_F_MAPPORTS flag. Unfortunately, the flag update is still directly
performed by srv_update_addr_port() function while the addr:svc_port
update is being scheduled for atomic update. Given that existing readers
don't take server's lock to read addr:svc_port, they also check the
SRV_F_MAPPORTS flag right after without the lock.

So we could cause the readers to incorrectly interpret the svc_port from
the server struct because the mapport information is not published
atomically, resulting in inconsistencies between svc_port / mapport flag.
(MAPPORTS flag causes svc_port to be used differently by the reader)

To fix this, we publish the mapport information within the INETADDR server
event and we let the task responsible for updating server's addr and port
position or clear the flag depending on the mapport hint.

This patch depends on:
 - MINOR: server/event_hdl: add server_inetaddr struct to facilitate event data usage
 - MINOR: server/event_hdl: update _srv_event_hdl_prepare_inetaddr prototype

This should be backported in 2.9 with 683b2ae01 ("MINOR: server/event_hdl:
add SERVER_INETADDR event")

(cherry picked from commit 545e72546c0d729fd3de0f753adc905dd0b13908)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

diff --git a/include/haproxy/server-t.h b/include/haproxy/server-t.h
index 7d1cc70..9a9b48b 100644 (file)
--- a/include/haproxy/server-t.h
+++ b/include/haproxy/server-t.h
@@ -606,7 +606,10 @@ struct server_inetaddr {
                struct in_addr v4;
                struct in6_addr v6;
        } addr; /* may hold v4 or v6 addr */
-       unsigned int svc_port;
+       struct {
+               unsigned int svc;
+               uint8_t map; /* is a mapped port? (boolean) */
+       } port;
 };
 
 /* data provided to EVENT_HDL_SUB_SERVER_INETADDR handlers through

diff --git a/src/server.c b/src/server.c
index 9228b45..b62dd06 100644 (file)
--- a/src/server.c
+++ b/src/server.c
@@ -173,10 +173,17 @@ int srv_getinter(const struct check *check)
  * Must be called under thread isolation to ensure consistent readings accross
  * all threads (addr:svc_port might be read without srv lock being held).
  */
-void _srv_set_inetaddr(struct server *srv, const struct sockaddr_storage *addr, unsigned int svc_port)
+static void _srv_set_inetaddr_port(struct server *srv,
+                                   const struct sockaddr_storage *addr,
+                                   unsigned int svc_port, uint8_t mapped_port)
 {
        ipcpy(addr, &srv->addr);
        srv->svc_port = svc_port;
+       if (mapped_port)
+               srv->flags |= SRV_F_MAPPORTS;
+       else
+               srv->flags &= ~SRV_F_MAPPORTS;
+
        if (srv->log_target && srv->log_target->type == LOG_TARGET_DGRAM) {
                /* server is used as a log target, manually update log target addr for DGRAM */
                ipcpy(addr, srv->log_target->addr);
@@ -184,6 +191,14 @@ void _srv_set_inetaddr(struct server *srv, const struct sockaddr_storage *addr,
        }
 }
 
+/* same as _srv_set_inetaddr_port() but only updates the addr part
+ */
+static void _srv_set_inetaddr(struct server *srv,
+                              const struct sockaddr_storage *addr)
+{
+       _srv_set_inetaddr_port(srv, addr, srv->svc_port, !!(srv->flags & SRV_F_MAPPORTS));
+}
+
 /*
  * Function executed by server_atomic_sync_task to perform atomic updates on
  * compatible server struct members that are not guarded by any lock since
@@ -288,7 +303,8 @@ static struct task *server_atomic_sync(struct task *task, void *context, unsigne
                                thread_isolate_full();
 
                        /* apply new addr:port combination */
-                       _srv_set_inetaddr(srv, &new_addr, data->safe.next.svc_port);
+                       _srv_set_inetaddr_port(srv, &new_addr,
+                                              data->safe.next.port.svc, data->safe.next.port.map);
 
                        /* propagate the changes */
                        if (data->safe.purge_conn) /* force connection cleanup on the given server? */
@@ -422,11 +438,12 @@ void _srv_event_hdl_prepare_state(struct event_hdl_cb_data_server_state *cb_data
 static void _srv_event_hdl_prepare_inetaddr(struct event_hdl_cb_data_server_inetaddr *cb_data,
                                             struct server *srv,
                                             const struct sockaddr_storage *next_addr,
-                                            unsigned int next_port,
+                                            unsigned int next_port, uint8_t next_mapports,
                                             uint8_t purge_conn)
 {
        struct sockaddr_storage *prev_addr = &srv->addr;
        unsigned int prev_port = srv->svc_port;
+       uint8_t prev_mapports = !!(srv->flags & SRV_F_MAPPORTS);
 
        /* only INET families are supported */
        BUG_ON((prev_addr->ss_family != AF_UNSPEC &&
@@ -444,7 +461,8 @@ static void _srv_event_hdl_prepare_inetaddr(struct event_hdl_cb_data_server_inet
                memcpy(&cb_data->safe.prev.addr.v6,
                       &((struct sockaddr_in6 *)prev_addr)->sin6_addr,
                       sizeof(struct in6_addr));
-       cb_data->safe.prev.svc_port = prev_port;
+       cb_data->safe.prev.port.svc = prev_port;
+       cb_data->safe.prev.port.map = prev_mapports;
 
        /* next */
        cb_data->safe.next.family = next_addr->ss_family;
@@ -456,7 +474,8 @@ static void _srv_event_hdl_prepare_inetaddr(struct event_hdl_cb_data_server_inet
                memcpy(&cb_data->safe.next.addr.v6,
                       &((struct sockaddr_in6 *)next_addr)->sin6_addr,
                       sizeof(struct in6_addr));
-       cb_data->safe.next.svc_port = next_port;
+       cb_data->safe.next.port.svc = next_port;
+       cb_data->safe.next.port.map = next_mapports;
 
        cb_data->safe.purge_conn = purge_conn;
 }
@@ -3771,7 +3790,9 @@ int srv_update_addr(struct server *s, void *ip, int ip_sin_family, const char *u
        };
 
        _srv_event_hdl_prepare(&cb_data.common, s, 0);
-       _srv_event_hdl_prepare_inetaddr(&cb_data.addr, s, &new_addr, s->svc_port, 0);
+       _srv_event_hdl_prepare_inetaddr(&cb_data.addr, s,
+                                        &new_addr, s->svc_port, !!(s->flags & SRV_F_MAPPORTS),
+                                        0);
 
        /* server_atomic_sync_task will apply the changes for us */
        _srv_event_hdl_publish(EVENT_HDL_SUB_SERVER_INETADDR, cb_data, s);
@@ -3910,6 +3931,7 @@ const char *srv_update_addr_port(struct server *s, const char *addr, const char
        struct buffer *msg;
        int ip_change = 0;
        int port_change = 0;
+       uint8_t mapports = !!(s->flags & SRV_F_MAPPORTS);
 
        msg = get_trash_chunk();
        chunk_reset(msg);
@@ -4005,17 +4027,17 @@ const char *srv_update_addr_port(struct server *s, const char *addr, const char
                        chunk_appendf(msg, "%d' to '", current_port);
 
                        if (sign == '-') {
-                               s->flags |= SRV_F_MAPPORTS;
+                               mapports = 1;
                                chunk_appendf(msg, "%c", sign);
                                /* just use for result output */
                                new_port_print = -new_port_print;
                        }
                        else if (sign == '+') {
-                               s->flags |= SRV_F_MAPPORTS;
+                               mapports = 1;
                                chunk_appendf(msg, "%c", sign);
                        }
                        else {
-                               s->flags &= ~SRV_F_MAPPORTS;
+                               mapports = 0;
                        }
 
                        chunk_appendf(msg, "%d'", new_port_print);
@@ -4030,7 +4052,7 @@ out:
                _srv_event_hdl_prepare(&cb_data.common, s, 0);
                _srv_event_hdl_prepare_inetaddr(&cb_data.addr, s,
                                                ((ip_change) ? &sa : &s->addr),
-                                               ((port_change) ? new_port : s->svc_port),
+                                               ((port_change) ? new_port : s->svc_port), mapports,
                                                1);
 
                /* server_atomic_sync_task will apply the changes for us */
@@ -4447,7 +4469,7 @@ int srv_set_addr_via_libc(struct server *srv, int *err_code)
                        *err_code |= ERR_WARN;
                return 1;
        }
-       _srv_set_inetaddr(srv, &new_addr, srv->svc_port);
+       _srv_set_inetaddr(srv, &new_addr);
        return 0;
 }
 
@@ -4534,7 +4556,7 @@ static int srv_apply_lastaddr(struct server *srv, int *err_code)
                        *err_code |= ERR_WARN;
                return 1;
        }
-       _srv_set_inetaddr(srv, &new_addr, srv->svc_port);
+       _srv_set_inetaddr(srv, &new_addr);
        return 0;
 }
 
@@ -4596,7 +4618,7 @@ static int srv_iterate_initaddr(struct server *srv)
                        return return_code;
 
                case SRV_IADDR_IP:
-                       _srv_set_inetaddr(srv, &srv->init_addr, srv->svc_port);
+                       _srv_set_inetaddr(srv, &srv->init_addr);
                        if (return_code) {
                                ha_warning("could not resolve address '%s', falling back to configured address.\n",
                                           name);