[BUG] CONFIG REWRITE ignores already set ACL in included config - preventing service start

[marekhanzlik]

Describe the bug

Simplified config:

redis.conf

include /etc/redis/redis-inc.conf

redis-inc.conf

user default on nopass sanitize-payload allcommands allkeys allchannels

or (doesnt matter which format, both behave the same)

user default on nopass sanitize-payload ~* &* +@all

when we call CONFIG REWRITE the user directive is again resaved into redis.conf

This prevent redis from starting after service restart or system reboot with error

Error in user declaration 'default': Duplicate user found. A user can only be defined once in config files

[enjoy-binbin]

thanks for the report. i see the problem, @madolson Do you have any suggestions?

from redis.conf:

# Include one or more other config files here.  This is useful if you
# have a standard template that goes to all Redis servers but also need
# to customize a few per-server settings.  Include files can include
# other files, so use this wisely.
#
# Note that option "include" won't be rewritten by command "CONFIG REWRITE"
# from admin or Redis Sentinel. Since Redis always uses the last processed
# line as value of a configuration directive, you'd better put includes
# at the beginning of this file to avoid overwriting config change at runtime.
#
# If instead you are interested in using includes to override configuration
# options, it is better to use include as the last line.

[oranagra]

CONFIG REWRITE doesn't really play well with include files.
for most configs, it'll just add duplicate lines, and the last one simply overrides the earlier ones (as noted by the above quote),
but for ACL we recently traded that with an error, see #9330.

since the rewrite code doesn't have any visibility of the include files (and will certainly not modify it), i think we should just argue that ACL should be managed by an aclfile, and if not then at least the plain config file, and mixing it in include files and config rewrite is unsupported.

[marekhanzlik]

Thanks for the aclfile tip, i will use this as a workaround.

Regarding the REWRITE and include files: Then this documentation line does not make much sense

The CONFIG REWRITE command rewrites the redis.conf file the server was started with, 
applying the minimal changes needed to make it reflect the configuration currently used by the server

It implies that rewrite will write out only changes that are not already in redis.conf (and in this case even included files, cause it's not clear how is include handled)

[oranagra]

i'm not sure what you mean. feel free to make a PR to suggest a better text.

[kbcmdba]

I see this was closed above, but I'm looking for a version this is fixed in...

[oranagra]

@kbcmdba we didn't fix anything, config rewrite doesn't work well with include, for ACL you have a better option which is to use the aclfile config.

if your problem wasn't with ACL, maybe add some more details..

the conclusions was that maybe the docs can be improve to warn users, feel free to make a PR where you think that can be effective.

[kbcmdba]

IMNSHO, rewrite shouldn't cause configuration problems that would prevent Redis from running in the future. To me, that's a bug. As such, I believe this should be reopened.

[oranagra]

i agree that in a perfect world it shouldn't (cause configuration problems), but the fact is that it's complicated and not a priority.
maybe we should remove the include feature instead, then we won't have that problem anymore 😏

the way i see it, the include feature is meant to be used in certain types of use cases, and the rewrite is for others, and they should not be mixed.

that said, if someone submits a PR to solves the problem, we'll certainly consider merging it.

[enjoy-binbin]

maybe we can consider this? #12010 (comment)