From cf3323dba473f3849200d294bb0349cd442f2006 Mon Sep 17 00:00:00 2001
From: zhugezy <44550833+zhugezy@users.noreply.github.com>
Date: Thu, 2 Jun 2022 13:36:55 +0800
Subject: [PATCH] Fix bugs in CONFIG REWRITE, omitting rename-command and
 include lines, and inserting comments around module and acl configs (#10761)

A regression from #10285 (redis 7.0).
CONFIG REWRITE would put lines with: `include`, `rename-command`,
`user`,  `loadmodule`, and any module specific config in a comment.

For ACL `user`, `loadmodule` and module specific configs would be
re-inserted at the end (instead of updating existing lines), so the only
implication is a messy config file full of comments.

But for `rename-command` and `include`, the implication would be that
they're now missing, so a server restart would lose them.

Co-authored-by: Oran Agra <oran@redislabs.com>
---
 src/config.c                 | 19 +++++++++++++++----
 tests/unit/acl.tcl           | 10 ++++++++++
 tests/unit/introspection.tcl | 12 ++++++++++++
 3 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/src/config.c b/src/config.c
index 24760b030b..9856af3c08 100644
--- a/src/config.c
+++ b/src/config.c
@@ -1142,10 +1142,21 @@ struct rewriteConfigState *rewriteConfigReadOldFile(char *path) {
 
         /* Not a comment, split into arguments. */
         argv = sdssplitargs(line,&argc);
-        if (argv == NULL || (!server.sentinel_mode && !lookupConfig(argv[0]))) {
-            /* Apparently the line is unparsable for some reason, for
-             * instance it may have unbalanced quotes, or may contain a
-             * config that doesn't exist anymore. Load it as a comment. */
+
+        if (argv == NULL ||
+            (!lookupConfig(argv[0]) &&
+             /* The following is a list of config features that are only supported in
+              * config file parsing and are not recognized by lookupConfig */
+             strcasecmp(argv[0],"include") &&
+             strcasecmp(argv[0],"rename-command") &&
+             strcasecmp(argv[0],"user") &&
+             strcasecmp(argv[0],"loadmodule") &&
+             strcasecmp(argv[0],"sentinel")))
+        {
+            /* The line is either unparsable for some reason, for
+             * instance it may have unbalanced quotes, may contain a
+             * config that doesn't exist anymore, for instance a module that got
+             * unloaded. Load it as a comment. */
             sds aux = sdsnew("# ??? ");
             aux = sdscatsds(aux,line);
             if (argv) sdsfreesplitres(argv, argc);
diff --git a/tests/unit/acl.tcl b/tests/unit/acl.tcl
index 11c871bb61..5f4be18889 100644
--- a/tests/unit/acl.tcl
+++ b/tests/unit/acl.tcl
@@ -933,3 +933,13 @@ start_server [list overrides [list "dir" $server_path "aclfile" "user.acl"] tags
         assert_match {*Duplicate user*} $err
     } {} {external:skip}
 }
+
+start_server {overrides {user "default on nopass ~* +@all -flushdb"} tags {acl external:skip}} {
+    test {ACL from config file and config rewrite} {
+        assert_error {NOPERM *} {r flushdb}
+        r config rewrite
+        restart_server 0 true false
+        assert_error {NOPERM *} {r flushdb}
+    }
+}
+
diff --git a/tests/unit/introspection.tcl b/tests/unit/introspection.tcl
index 3a11a7027e..2bee01e7c9 100644
--- a/tests/unit/introspection.tcl
+++ b/tests/unit/introspection.tcl
@@ -582,3 +582,15 @@ test {config during loading} {
         exec kill [srv 0 pid]
     }
 } {} {external:skip}
+
+test {CONFIG REWRITE handles rename-command properly} {
+    start_server {tags {"introspection"} overrides {rename-command {flushdb badger}}} {
+        assert_error {ERR unknown command*} {r flushdb}
+
+        r config rewrite
+        restart_server 0 true false
+
+        assert_error {ERR unknown command*} {r flushdb}
+    }
+} {} {external:skip}
+