Support getting configuration from both stdin and file at the same time#7893
Support getting configuration from both stdin and file at the same time#7893yossigo merged 1 commit intoredis:unstablefrom
Conversation
src/server.c
Outdated
There was a problem hiding this comment.
i suppose there's a reason for that.
probably that sentinel must be able to rewrite it's config and restart.
There was a problem hiding this comment.
Yes, It still won't start without a configfile -- I just simplified the check a bit since I now have an explicit flag for stdin and don't have to use the hack of saving '-' as the config file name.
There was a problem hiding this comment.
i think it's not just that it shouldn't start without a config file.
i think the intention was that it should explicitly fail when config_from_stdin is true.
if the main reason for providing config via stdin is that we don't store passwords on disk, sentinel isn't designed to do that.
There was a problem hiding this comment.
Don't think so -- It's perfectly fine to have the config come from stdin initially AS LONG as you also have a file to write to later on.
(it's less useful obviously, but not something that will fail later on).
There was a problem hiding this comment.
ok. sounds reasonable to me, but i'd like @yossigo to confirm that.
ushachar
force-pushed
the
handle_stdin_config
branch
from
7c97805 to
16fcf5f
Compare
This allows supplying secret configuration (for example - masterauth) via a secure channel instead of having it in a plaintext file / command line param, while still allowing for most of the configuration to reside there. Also, remove 'special' case handling for --check-rdb which hasn't been relevant since 4.0.0.
ushachar
force-pushed
the
handle_stdin_config
branch
from
16fcf5f to
1892c7e
Compare
| } | ||
| /* Append content from stdin */ | ||
| if (config_from_stdin) { | ||
| serverLog(LL_WARNING,"Reading config from stdin"); |
There was a problem hiding this comment.
| serverLog(LL_WARNING,"Reading config from stdin"); | |
| serverLog(LL_VERBOSE,"Reading config from stdin"); |
There was a problem hiding this comment.
I've intentionally made this a warning -- if someone uses this flag but doesn't provide any content via stdin the server will hang waiting for it (this is the behavior without this change as well). Emitting the warning might help avoid the follow-up SO question / GitHub issue.
|
|
||
| void usage(void) { | ||
| fprintf(stderr,"Usage: ./redis-server [/path/to/redis.conf] [options]\n"); | ||
| fprintf(stderr,"Usage: ./redis-server [/path/to/redis.conf] [options] [-]\n"); |
There was a problem hiding this comment.
I have some concerns about the user experience here. If only the filename is specified, it is used for input+rewrite. If stdin+filename are specified, the filename is used for output only which can be a bit confusing.
Maybe we should consider being more explicit about it, i.e. add a 'config-file-name' configuration directive which is used for rewriting and is by default the name of the input file (and none if we read stdin / command line args only). That directive could be used on the command line or inside the stdin stream of course.
@ushachar Does that make sense?
There was a problem hiding this comment.
The config file is always used for input+rewrite --- if stdin is provided as well then the content of stdin is appended to the string handed off to the parsing function.
…me (redis#7893) This allows supplying secret configuration (for example - masterauth) via a secure channel instead of having it in a plaintext file / command line param, while still allowing for most of the configuration to reside there. Also, remove 'special' case handling for --check-rdb which hasn't been relevant since 4.0.0.
3 participants
This allows supplying secret configuration (for example - masterauth) via a secure channel
instead of having it in a plaintext file / command line param, while still allowing for most
of the configuration to reside there.
Also, remove 'special' case handling for --check-rdb which hasn't been relevant
since 4.0.0.