Make port, tls-port and bind configurations modifiable (#8510)

Add ability to modify port, tls-port and bind configurations by CONFIG SET command.

To simplify the code and make it cleaner, a new structure
added, socketFds, which contains the file descriptors array and its counter,
and used for TCP, TLS and Cluster sockets file descriptors.

src/cluster.c

@@ -499,7 +499,7 @@ void clusterInit(void) {
     if (saveconf) clusterSaveConfigOrDie(1);
 
     /* We need a listening TCP port for our cluster messaging needs. */
-    server.cfd_count = 0;
+    server.cfd.count = 0;
 
     /* Port sanity check II
      * The other handshake port check is triggered too late to stop
@@ -512,19 +512,11 @@ void clusterInit(void) {
                    "Your Redis port number must be 55535 or less.");
         exit(1);
     }
-    if (listenToPort(port+CLUSTER_PORT_INCR,
+    if (listenToPort(port+CLUSTER_PORT_INCR, &server.cfd) == C_ERR) {
-        server.cfd,&server.cfd_count) == C_ERR)
-    {
         exit(1);
-    } else {
+    }
-        int j;
+    if (createSocketAcceptHandler(&server.cfd, clusterAcceptHandler) != C_OK) {
-
+        serverPanic("Unrecoverable error creating Redis Cluster socket accept handler.");
-        for (j = 0; j < server.cfd_count; j++) {
-            if (aeCreateFileEvent(server.el, server.cfd[j], AE_READABLE,
-                clusterAcceptHandler, NULL) == AE_ERR)
-                    serverPanic("Unrecoverable error creating Redis Cluster "
-                                "file event.");
-        }
     }
 
     /* The slots -> keys map is a radix tree. Initialize it here. */

src/config.c

@@ -728,7 +728,23 @@ void configSetCommand(client *c) {
     if (0) { /* this starts the config_set macros else-if chain. */
 
     /* Special fields that can't be handled with general macros. */
-    config_set_special_field("save") {
+    config_set_special_field("bind") {
+        int vlen;
+        sds *v = sdssplitlen(o->ptr,sdslen(o->ptr)," ",1,&vlen);
+
+        if (vlen < 1 || vlen > CONFIG_BINDADDR_MAX) {
+            addReplyError(c, "Too many bind addresses specified.");
+            sdsfreesplitres(v, vlen);
+            return;
+        }
+
+        if (changeBindAddr(v, vlen) == C_ERR) {
+            addReplyError(c, "Failed to bind to specified addresses.");
+            sdsfreesplitres(v, vlen);
+            return;
+        }
+        sdsfreesplitres(v, vlen);
+    } config_set_special_field("save") {
         int vlen, j;
         sds *v = sdssplitlen(o->ptr,sdslen(o->ptr)," ",1,&vlen);
 
@@ -2191,6 +2207,20 @@ static int updateHZ(long long val, long long prev, const char **err) {
     return 1;
 }
 
+static int updatePort(long long val, long long prev, const char **err) {
+    /* Do nothing if port is unchanged */
+    if (val == prev) {
+        return 1;
+    }
+
+    if (changeListenPort(val, &server.ipfd, acceptTcpHandler) == C_ERR) {
+        *err = "Unable to listen on this port. Check server logs.";
+        return 0;
+    }
+
+    return 1;
+}
+
 static int updateJemallocBgThread(int val, int prev, const char **err) {
     UNUSED(prev);
     UNUSED(err);
@@ -2329,6 +2359,27 @@ static int updateTlsCfgInt(long long val, long long prev, const char **err) {
     UNUSED(prev);
     return updateTlsCfg(NULL, NULL, err);
 }
+
+static int updateTLSPort(long long val, long long prev, const char **err) {
+    /* Do nothing if port is unchanged */
+    if (val == prev) {
+        return 1;
+    }
+
+    /* Configure TLS if tls is enabled */
+    if (prev == 0 && tlsConfigure(&server.tls_ctx_config) == C_ERR) {
+        *err = "Unable to update TLS configuration. Check server logs.";
+        return 0;
+    }
+
+    if (changeListenPort(val, &server.tlsfd, acceptTLSHandler) == C_ERR) {
+        *err = "Unable to listen on this port. Check server logs.";
+        return 0;
+    }
+
+    return 1;
+}
+
 #endif  /* USE_OPENSSL */
 
 standardConfig configs[] = {
@@ -2409,7 +2460,7 @@ standardConfig configs[] = {
 
     /* Integer configs */
     createIntConfig("databases", NULL, IMMUTABLE_CONFIG, 1, INT_MAX, server.dbnum, 16, INTEGER_CONFIG, NULL, NULL),
-    createIntConfig("port", NULL, IMMUTABLE_CONFIG, 0, 65535, server.port, 6379, INTEGER_CONFIG, NULL, NULL), /* TCP port. */
+    createIntConfig("port", NULL, MODIFIABLE_CONFIG, 0, 65535, server.port, 6379, INTEGER_CONFIG, NULL, updatePort), /* TCP port. */
     createIntConfig("io-threads", NULL, IMMUTABLE_CONFIG, 1, 128, server.io_threads_num, 1, INTEGER_CONFIG, NULL, NULL), /* Single threaded by default */
     createIntConfig("auto-aof-rewrite-percentage", NULL, MODIFIABLE_CONFIG, 0, INT_MAX, server.aof_rewrite_perc, 100, INTEGER_CONFIG, NULL, NULL),
     createIntConfig("cluster-replica-validity-factor", "cluster-slave-validity-factor", MODIFIABLE_CONFIG, 0, INT_MAX, server.cluster_slave_validity_factor, 10, INTEGER_CONFIG, NULL, NULL), /* Slave max data age factor. */
@@ -2478,7 +2529,7 @@ standardConfig configs[] = {
     createOffTConfig("auto-aof-rewrite-min-size", NULL, MODIFIABLE_CONFIG, 0, LLONG_MAX, server.aof_rewrite_min_size, 64*1024*1024, MEMORY_CONFIG, NULL, NULL),
 
 #ifdef USE_OPENSSL
-    createIntConfig("tls-port", NULL, IMMUTABLE_CONFIG, 0, 65535, server.tls_port, 0, INTEGER_CONFIG, NULL, updateTlsCfgInt), /* TCP port. */
+    createIntConfig("tls-port", NULL, MODIFIABLE_CONFIG, 0, 65535, server.tls_port, 0, INTEGER_CONFIG, NULL, updateTLSPort), /* TCP port. */
     createIntConfig("tls-session-cache-size", NULL, MODIFIABLE_CONFIG, 0, INT_MAX, server.tls_ctx_config.session_cache_size, 20*1024, INTEGER_CONFIG, NULL, updateTlsCfgInt),
     createIntConfig("tls-session-cache-timeout", NULL, MODIFIABLE_CONFIG, 0, INT_MAX, server.tls_ctx_config.session_cache_timeout, 300, INTEGER_CONFIG, NULL, updateTlsCfgInt),
     createBoolConfig("tls-cluster", NULL, MODIFIABLE_CONFIG, server.tls_cluster, 0, NULL, updateTlsCfgBool),

src/server.c

@@ -2648,8 +2648,8 @@ void initServerConfig(void) {
     server.arch_bits = (sizeof(long) == 8) ? 64 : 32;
     server.bindaddr_count = 0;
     server.unixsocketperm = CONFIG_DEFAULT_UNIX_SOCKET_PERM;
-    server.ipfd_count = 0;
+    server.ipfd.count = 0;
-    server.tlsfd_count = 0;
+    server.tlsfd.count = 0;
     server.sofd = -1;
     server.active_expire_enabled = 1;
     server.skip_checksum_validation = 0;
@@ -2988,6 +2988,34 @@ void checkTcpBacklogSettings(void) {
 #endif
 }
 
+void closeSocketListeners(socketFds *sfd) {
+    int j;
+
+    for (j = 0; j < sfd->count; j++) {
+        if (sfd->fd[j] == -1) continue;
+
+        aeDeleteFileEvent(server.el, sfd->fd[j], AE_READABLE);
+        close(sfd->fd[j]);
+    }
+
+    sfd->count = 0;
+}
+
+/* Create an event handler for accepting new connections in TCP or TLS domain sockets.
+ * This works atomically for all socket fds */
+int createSocketAcceptHandler(socketFds *sfd, aeFileProc *accept_handler) {
+    int j;
+
+    for (j = 0; j < sfd->count; j++) {
+        if (aeCreateFileEvent(server.el, sfd->fd[j], AE_READABLE, accept_handler,NULL) == AE_ERR) {
+            /* Rollback */
+            for (j = j-1; j >= 0; j--) aeDeleteFileEvent(server.el, sfd->fd[j], AE_READABLE);
+            return C_ERR;
+        }
+    }
+    return C_OK;
+}
+
 /* Initialize a set of file descriptors to listen to the specified 'port'
  * binding the addresses specified in the Redis server configuration.
  *
@@ -3006,7 +3034,7 @@ void checkTcpBacklogSettings(void) {
  * impossible to bind, or no bind addresses were specified in the server
  * configuration but the function is not able to bind * for at least
  * one of the IPv4 or IPv6 protocols. */
-int listenToPort(int port, int *fds, int *count) {
+int listenToPort(int port, socketFds *sfd) {
     int j;
     char **bindaddr = server.bindaddr;
     int bindaddr_count = server.bindaddr_count;
@@ -3024,12 +3052,12 @@ int listenToPort(int port, int *fds, int *count) {
         if (optional) addr++;
         if (strchr(addr,':')) {
             /* Bind IPv6 address. */
-            fds[*count] = anetTcp6Server(server.neterr,port,addr,server.tcp_backlog);
+            sfd->fd[sfd->count] = anetTcp6Server(server.neterr,port,addr,server.tcp_backlog);
         } else {
             /* Bind IPv4 address. */
-            fds[*count] = anetTcpServer(server.neterr,port,addr,server.tcp_backlog);
+            sfd->fd[sfd->count] = anetTcpServer(server.neterr,port,addr,server.tcp_backlog);
         }
-        if (fds[*count] == ANET_ERR) {
+        if (sfd->fd[sfd->count] == ANET_ERR) {
             serverLog(LL_WARNING,
                 "Could not create server TCP listening socket %s:%d: %s",
                 addr, port, server.neterr);
@@ -3039,11 +3067,14 @@ int listenToPort(int port, int *fds, int *count) {
                 errno == ESOCKTNOSUPPORT || errno == EPFNOSUPPORT ||
                 errno == EAFNOSUPPORT)
                 continue;
+
+            /* Rollback successful listens before exiting */
+            closeSocketListeners(sfd);
             return C_ERR;
         }
-        anetNonBlock(NULL,fds[*count]);
+        anetNonBlock(NULL,sfd->fd[sfd->count]);
-        anetCloexec(fds[*count]);
+        anetCloexec(sfd->fd[sfd->count]);
-        (*count)++;
+        sfd->count++;
     }
     return C_OK;
 }
@@ -3165,10 +3196,10 @@ void initServer(void) {
 
     /* Open the TCP listening socket for the user commands. */
     if (server.port != 0 &&
-        listenToPort(server.port,server.ipfd,&server.ipfd_count) == C_ERR)
+        listenToPort(server.port,&server.ipfd) == C_ERR)
         exit(1);
     if (server.tls_port != 0 &&
-        listenToPort(server.tls_port,server.tlsfd,&server.tlsfd_count) == C_ERR)
+        listenToPort(server.tls_port,&server.tlsfd) == C_ERR)
         exit(1);
 
     /* Open the listening Unix domain socket. */
@@ -3185,7 +3216,7 @@ void initServer(void) {
     }
 
     /* Abort if there are no listening sockets at all. */
-    if (server.ipfd_count == 0 && server.tlsfd_count == 0 && server.sofd < 0) {
+    if (server.ipfd.count == 0 && server.tlsfd.count == 0 && server.sofd < 0) {
         serverLog(LL_WARNING, "Configured to not listen anywhere, exiting.");
         exit(1);
     }
@@ -3263,21 +3294,11 @@ void initServer(void) {
 
     /* Create an event handler for accepting new connections in TCP and Unix
      * domain sockets. */
-    for (j = 0; j < server.ipfd_count; j++) {
+    if (createSocketAcceptHandler(&server.ipfd, acceptTcpHandler) != C_OK) {
-        if (aeCreateFileEvent(server.el, server.ipfd[j], AE_READABLE,
+        serverPanic("Unrecoverable error creating TCP socket accept handler.");
-            acceptTcpHandler,NULL) == AE_ERR)
-            {
-                serverPanic(
-                    "Unrecoverable error creating server.ipfd file event.");
-            }
     }
-    for (j = 0; j < server.tlsfd_count; j++) {
+    if (createSocketAcceptHandler(&server.tlsfd, acceptTLSHandler) != C_OK) {
-        if (aeCreateFileEvent(server.el, server.tlsfd[j], AE_READABLE,
+        serverPanic("Unrecoverable error creating TLS socket accept handler.");
-            acceptTLSHandler,NULL) == AE_ERR)
-            {
-                serverPanic(
-                    "Unrecoverable error creating server.tlsfd file event.");
-            }
     }
     if (server.sofd > 0 && aeCreateFileEvent(server.el,server.sofd,AE_READABLE,
         acceptUnixHandler,NULL) == AE_ERR) serverPanic("Unrecoverable error creating server.sofd file event.");
@@ -4182,11 +4203,11 @@ void incrementErrorCount(const char *fullerr, size_t namelen) {
 void closeListeningSockets(int unlink_unix_socket) {
     int j;
 
-    for (j = 0; j < server.ipfd_count; j++) close(server.ipfd[j]);
+    for (j = 0; j < server.ipfd.count; j++) close(server.ipfd.fd[j]);
-    for (j = 0; j < server.tlsfd_count; j++) close(server.tlsfd[j]);
+    for (j = 0; j < server.tlsfd.count; j++) close(server.tlsfd.fd[j]);
     if (server.sofd != -1) close(server.sofd);
     if (server.cluster_enabled)
-        for (j = 0; j < server.cfd_count; j++) close(server.cfd[j]);
+        for (j = 0; j < server.cfd.count; j++) close(server.cfd.fd[j]);
     if (unlink_unix_socket && server.unixsocket) {
         serverLog(LL_NOTICE,"Removing the unix socket file.");
         unlink(server.unixsocket); /* don't care if this fails */
@@ -5536,6 +5557,105 @@ void redisAsciiArt(void) {
     zfree(buf);
 }
 
+int changeBindAddr(sds *addrlist, int addrlist_len) {
+    int i;
+    int result = C_OK;
+
+    char *prev_bindaddr[CONFIG_BINDADDR_MAX];
+    int prev_bindaddr_count;
+
+    /* Close old TCP and TLS servers */
+    closeSocketListeners(&server.ipfd);
+    closeSocketListeners(&server.tlsfd);
+
+    /* Keep previous settings */
+    prev_bindaddr_count = server.bindaddr_count;
+    memcpy(prev_bindaddr, server.bindaddr, sizeof(server.bindaddr));
+
+    /* Copy new settings */
+    memset(server.bindaddr, 0, sizeof(server.bindaddr));
+    for (i = 0; i < addrlist_len; i++) {
+        server.bindaddr[i] = zstrdup(addrlist[i]);
+    }
+    server.bindaddr_count = addrlist_len;
+
+    /* Bind to the new port */
+    if ((server.port != 0 && listenToPort(server.port, &server.ipfd) != C_OK) ||
+        (server.tls_port != 0 && listenToPort(server.tls_port, &server.tlsfd) != C_OK)) {
+        serverLog(LL_WARNING, "Failed to bind, trying to restore old listening sockets.");
+
+        /* Restore old bind addresses */
+        for (i = 0; i < addrlist_len; i++) {
+            zfree(server.bindaddr[i]);
+        }
+        memcpy(server.bindaddr, prev_bindaddr, sizeof(server.bindaddr));
+        server.bindaddr_count = prev_bindaddr_count;
+
+        /* Re-Listen TCP and TLS */
+        server.ipfd.count = 0;
+        if (server.port != 0 && listenToPort(server.port, &server.ipfd) != C_OK) {
+            serverPanic("Failed to restore old listening sockets.");
+        }
+
+        server.tlsfd.count = 0;
+        if (server.tls_port != 0 && listenToPort(server.tls_port, &server.tlsfd) != C_OK) {
+            serverPanic("Failed to restore old listening sockets.");
+        }
+
+        result = C_ERR;
+    } else {
+        /* Free old bind addresses */
+        for (i = 0; i < prev_bindaddr_count; i++) {
+            zfree(prev_bindaddr[i]);
+        }
+    }
+
+    /* Create TCP and TLS event handlers */
+    if (createSocketAcceptHandler(&server.ipfd, acceptTcpHandler) != C_OK) {
+        serverPanic("Unrecoverable error creating TCP socket accept handler.");
+    }
+    if (createSocketAcceptHandler(&server.tlsfd, acceptTLSHandler) != C_OK) {
+        serverPanic("Unrecoverable error creating TLS socket accept handler.");
+    }
+
+    if (server.set_proc_title) redisSetProcTitle(NULL);
+
+    return result;
+}
+
+int changeListenPort(int port, socketFds *sfd, aeFileProc *accept_handler) {
+    socketFds new_sfd = {{0}};
+
+    /* Just close the server if port disabled */
+    if (port == 0) {
+        closeSocketListeners(sfd);
+        if (server.set_proc_title) redisSetProcTitle(NULL);
+        return C_OK;
+    }
+
+    /* Bind to the new port */
+    if (listenToPort(port, &new_sfd) != C_OK) {
+        return C_ERR;
+    }
+
+    /* Create event handlers */
+    if (createSocketAcceptHandler(&new_sfd, accept_handler) != C_OK) {
+        closeSocketListeners(&new_sfd);
+        return C_ERR;
+    }
+
+    /* Close old servers */
+    closeSocketListeners(sfd);
+
+    /* Copy new descriptors */
+    sfd->count = new_sfd.count;
+    memcpy(sfd->fd, new_sfd.fd, sizeof(new_sfd.fd));
+
+    if (server.set_proc_title) redisSetProcTitle(NULL);
+
+    return C_OK;
+}
+
 static void sigShutdownHandler(int sig) {
     char *msg;
 
@@ -6124,7 +6244,7 @@ int main(int argc, char **argv) {
                 exit(1);
             }
         }
-        if (server.ipfd_count > 0 || server.tlsfd_count > 0)
+        if (server.ipfd.count > 0 || server.tlsfd.count > 0)
             serverLog(LL_NOTICE,"Ready to accept connections");
         if (server.sofd > 0)
             serverLog(LL_NOTICE,"The server is now ready to accept connections at %s", server.unixsocket);

src/server.h

@@ -1104,6 +1104,11 @@ struct malloc_stats {
     size_t allocator_resident;
 };
 
+typedef struct socketFds {
+    int fd[CONFIG_BINDADDR_MAX];
+    int count;
+} socketFds;
+
 /*-----------------------------------------------------------------------------
  * TLS Context Configuration
  *----------------------------------------------------------------------------*/
@@ -1204,13 +1209,10 @@ struct redisServer {
     int bindaddr_count;         /* Number of addresses in server.bindaddr[] */
     char *unixsocket;           /* UNIX socket path */
     mode_t unixsocketperm;      /* UNIX socket permission */
-    int ipfd[CONFIG_BINDADDR_MAX]; /* TCP socket file descriptors */
+    socketFds ipfd;             /* TCP socket file descriptors */
-    int ipfd_count;             /* Used slots in ipfd[] */
+    socketFds tlsfd;            /* TLS socket file descriptors */
-    int tlsfd[CONFIG_BINDADDR_MAX]; /* TLS socket file descriptors */
-    int tlsfd_count;            /* Used slots in tlsfd[] */
     int sofd;                   /* Unix socket file descriptor */
-    int cfd[CONFIG_BINDADDR_MAX];/* Cluster bus listening socket */
+    socketFds cfd;              /* Cluster bus listening socket */
-    int cfd_count;              /* Used slots in cfd[] */
     list *clients;              /* List of active clients */
     list *clients_to_close;     /* Clients to close asynchronously */
     list *clients_pending_write; /* There is to write or install handler. */
@@ -1855,7 +1857,7 @@ int getClientTypeByName(char *name);
 char *getClientTypeName(int class);
 void flushSlavesOutputBuffers(void);
 void disconnectSlaves(void);
-int listenToPort(int port, int *fds, int *count);
+int listenToPort(int port, socketFds *fds);
 void pauseClients(mstime_t duration, pause_type type);
 void unpauseClients(void);
 int areClientsPaused(void);
@@ -2184,6 +2186,9 @@ int processCommand(client *c);
 int processPendingCommandsAndResetClient(client *c);
 void setupSignalHandlers(void);
 void removeSignalHandlers(void);
+int createSocketAcceptHandler(socketFds *sfd, aeFileProc *accept_handler);
+int changeListenPort(int port, socketFds *sfd, aeFileProc *accept_handler);
+int changeBindAddr(sds *addrlist, int addrlist_len);
 struct redisCommand *lookupCommand(sds name);
 struct redisCommand *lookupCommandByCString(const char *s);
 struct redisCommand *lookupCommandOrOriginal(sds name);

tests/test_helper.tcl

@@ -75,6 +75,7 @@ set ::all_tests {
     unit/tracking
     unit/oom-score-adj
     unit/shutdown
+    unit/networking
 }
 # Index to the next test to run in the ::all_tests list.
 set ::next_test 0

tests/unit/introspection.tcl

@@ -100,13 +100,10 @@ start_server {tags {"introspection"}} {
             supervised
             syslog-facility
             databases
-            port
-            tls-port
             io-threads
             logfile
             unixsocketperm
             slaveof
-            bind
             requirepass
             server_cpulist
             bio_cpulist
@@ -131,6 +128,7 @@ start_server {tags {"introspection"}} {
                 tls-protocols
                 tls-ciphers
                 tls-ciphersuites
+                tls-port
             }
         }
 

tests/unit/networking.tcl

@@ -0,0 +1,34 @@
+test {CONFIG SET port number} {
+    start_server {} {
+        # available port
+        set avail_port [find_available_port $::baseport $::portcount]
+        set rd [redis [srv 0 host] [srv 0 port] 0 0]
+        $rd CONFIG SET port $avail_port
+        $rd close
+        set rd [redis [srv 0 host] $avail_port 0 0]
+        $rd PING
+
+        # already inuse port
+        catch {$rd CONFIG SET port $::test_server_port} e
+        assert_match {*Unable to listen on this port*} $e
+        $rd close
+
+        # make sure server still listening on the previous port
+        set rd [redis [srv 0 host] $avail_port 0 0]
+        $rd PING
+        $rd close
+    }
+}
+
+test {CONFIG SET bind address} {
+    start_server {} {
+        # non-valid address
+        catch {r CONFIG SET bind "some.wrong.bind.address"} e
+        assert_match {*Failed to bind to specified addresses*} $e
+
+        # make sure server still bound to the previous address
+        set rd [redis [srv 0 host] [srv 0 port] 0 0]
+        $rd PING
+        $rd close
+    }
+}

tests/unit/tls.tcl

@@ -114,5 +114,30 @@ start_server {tags {"tls"}} {
                 }
             }
         }
+
+        test {TLS: switch between tcp and tls ports} {
+            set srv_port [srv 0 port]
+
+            # TLS
+            set rd [redis [srv 0 host] $srv_port 0 1]
+            $rd PING
+
+            # TCP
+            $rd CONFIG SET tls-port 0
+            $rd CONFIG SET port $srv_port
+            $rd close
+
+            set rd [redis [srv 0 host] $srv_port 0 0]
+            $rd PING
+
+            # TLS
+            $rd CONFIG SET port 0
+            $rd CONFIG SET tls-port $srv_port
+            $rd close
+
+            set rd [redis [srv 0 host] $srv_port 0 1]
+            $rd PING
+            $rd close
+        }
     }
 }