Skip to content

Fixed reload of boolean flags #879

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 15, 2019
Merged

Fixed reload of boolean flags #879

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 45 additions & 17 deletions server/opts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,11 @@ type Options struct {
// CheckConfig configuration file syntax test was successful and exit.
CheckConfig bool `json:"-"`

// private fields, used to know if bool options are explicitly
// defined in config and/or command line params.
inConfig map[string]bool
inCmdLine map[string]bool

// private fields, used for testing
gatewaysSolicitDelay time.Duration
}
Expand Down Expand Up @@ -340,10 +345,13 @@ func (o *Options) ProcessConfigFile(configFile string) error {
o.Host = v.(string)
case "debug":
o.Debug = v.(bool)
trackExplicitVal(o, &o.inConfig, "Debug", o.Debug)
case "trace":
o.Trace = v.(bool)
trackExplicitVal(o, &o.inConfig, "Trace", o.Trace)
case "logtime":
o.Logtime = v.(bool)
trackExplicitVal(o, &o.inConfig, "Logtime", o.Logtime)
case "accounts":
err := parseAccounts(tk, o, &errors, &warnings)
if err != nil {
Expand Down Expand Up @@ -424,6 +432,7 @@ func (o *Options) ProcessConfigFile(configFile string) error {
o.LogFile = v.(string)
case "syslog":
o.Syslog = v.(bool)
trackExplicitVal(o, &o.inConfig, "Syslog", o.Syslog)
case "remote_syslog":
o.RemoteSyslog = v.(string)
case "pidfile", "pid_file":
Expand Down Expand Up @@ -630,6 +639,15 @@ func (o *Options) ProcessConfigFile(configFile string) error {
return nil
}

func trackExplicitVal(opts *Options, pm *map[string]bool, name string, val bool) {
m := *pm
if m == nil {
m = make(map[string]bool)
*pm = m
}
m[name] = val
}

// hostPort is simple struct to hold parsed listen/addr strings.
type hostPort struct {
host string
Expand Down Expand Up @@ -732,6 +750,7 @@ func parseCluster(v interface{}, opts *Options, errors *[]error, warnings *[]err
opts.Cluster.Advertise = mv.(string)
case "no_advertise":
opts.Cluster.NoAdvertise = mv.(bool)
trackExplicitVal(opts, &opts.inConfig, "Cluster.NoAdvertise", opts.Cluster.NoAdvertise)
case "connect_retries":
opts.Cluster.ConnectRetries = int(mv.(int64))
case "permissions":
Expand Down Expand Up @@ -2190,12 +2209,10 @@ func ConfigureOptions(fs *flag.FlagSet, args []string, printVersion, printHelp,
showTLSHelp bool
signal string
configFile string
dbgAndTrace bool
err error
)

// IMPORTANT: If you add boolean flags and use a 'true' as the default,
// check what needs to be done for FlagSnapshot down below.
// See logtimeIsExplicitlySet for reference.
fs.BoolVar(&showHelp, "h", false, "Show this message.")
fs.BoolVar(&showHelp, "help", false, "Show this message.")
fs.IntVar(&opts.Port, "port", 0, "Port to listen on.")
Expand All @@ -2208,7 +2225,7 @@ func ConfigureOptions(fs *flag.FlagSet, args []string, printVersion, printHelp,
fs.BoolVar(&opts.Debug, "debug", false, "Enable Debug logging.")
fs.BoolVar(&opts.Trace, "V", false, "Enable Trace logging.")
fs.BoolVar(&opts.Trace, "trace", false, "Enable Trace logging.")
fs.Bool("DV", false, "Enable Debug and Trace logging.")
fs.BoolVar(&dbgAndTrace, "DV", false, "Enable Debug and Trace logging.")
fs.BoolVar(&opts.Logtime, "T", true, "Timestamp log entries.")
fs.BoolVar(&opts.Logtime, "logtime", true, "Timestamp log entries.")
fs.StringVar(&opts.Username, "user", "", "Username required for connection.")
Expand Down Expand Up @@ -2247,7 +2264,6 @@ func ConfigureOptions(fs *flag.FlagSet, args []string, printVersion, printHelp,
fs.StringVar(&opts.TLSCert, "tlscert", "", "Server certificate file.")
fs.StringVar(&opts.TLSKey, "tlskey", "", "Private key for server certificate.")
fs.StringVar(&opts.TLSCaCert, "tlscacert", "", "Client certificate CA for verification.")
// --- See comment on top of flags definition before adding a flag ---

// The flags definition above set "default" values to some of the options.
// Calling Parse() here will override the default options with any value
Expand Down Expand Up @@ -2292,19 +2308,32 @@ func ConfigureOptions(fs *flag.FlagSet, args []string, printVersion, printHelp,
// Snapshot flag options.
FlagSnapshot = opts.Clone()

// Before parsing the config file, we need to know if boolean
// flags are explicitly set or not, and for those which default
// to `true`, we need to reset them otherwise config reload would
// incorrectly override what is in the config file.
logtimeIsExplicitlySet := false
// Keep track of the boolean flags that were explicitly set with their value.
fs.Visit(func(f *flag.Flag) {
if f.Name == "logtime" {
logtimeIsExplicitlySet = true
switch f.Name {
case "DV":
trackExplicitVal(FlagSnapshot, &FlagSnapshot.inCmdLine, "Debug", dbgAndTrace)
trackExplicitVal(FlagSnapshot, &FlagSnapshot.inCmdLine, "Trace", dbgAndTrace)
case "D":
fallthrough
case "debug":
trackExplicitVal(FlagSnapshot, &FlagSnapshot.inCmdLine, "Debug", FlagSnapshot.Debug)
case "V":
fallthrough
case "trace":
trackExplicitVal(FlagSnapshot, &FlagSnapshot.inCmdLine, "Trace", FlagSnapshot.Trace)
case "T":
fallthrough
case "logtime":
trackExplicitVal(FlagSnapshot, &FlagSnapshot.inCmdLine, "Logtime", FlagSnapshot.Logtime)
case "s":
fallthrough
case "syslog":
trackExplicitVal(FlagSnapshot, &FlagSnapshot.inCmdLine, "Syslog", FlagSnapshot.Syslog)
case "no_advertise":
trackExplicitVal(FlagSnapshot, &FlagSnapshot.inCmdLine, "Cluster.NoAdvertise", FlagSnapshot.Cluster.NoAdvertise)
}
})
if !logtimeIsExplicitlySet {
FlagSnapshot.Logtime = false
}

// Process signal control.
if signal != "" {
Expand Down Expand Up @@ -2371,8 +2400,7 @@ func ConfigureOptions(fs *flag.FlagSet, args []string, printVersion, printHelp,
switch f.Name {
case "DV":
// Check value to support -DV=false
boolValue, _ := strconv.ParseBool(f.Value.String())
opts.Trace, opts.Debug = boolValue, boolValue
opts.Trace, opts.Debug = dbgAndTrace, dbgAndTrace
case "cluster", "cluster_listen":
// Override cluster config if explicitly set via flags.
flagErr = overrideCluster(opts)
Expand Down
49 changes: 21 additions & 28 deletions server/opts_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,19 @@ import (
"time"
)

func checkOptionsEqual(t *testing.T, golden, opts *Options) {
t.Helper()
// Clone them so we can remove private fields that we don't
// want to be compared.
goldenClone := golden.Clone()
goldenClone.inConfig, goldenClone.inCmdLine = nil, nil
optsClone := opts.Clone()
optsClone.inConfig, optsClone.inCmdLine = nil, nil
if !reflect.DeepEqual(goldenClone, optsClone) {
t.Fatalf("Options are incorrect.\nexpected: %+v\ngot: %+v", goldenClone, optsClone)
}
}

func TestDefaultOptions(t *testing.T) {
golden := &Options{
Host: DEFAULT_HOST,
Expand All @@ -49,10 +62,7 @@ func TestDefaultOptions(t *testing.T) {
opts := &Options{}
setBaselineOptions(opts)

if !reflect.DeepEqual(golden, opts) {
t.Fatalf("Default Options are incorrect.\nexpected: %+v\ngot: %+v",
golden, opts)
}
checkOptionsEqual(t, golden, opts)
}

func TestOptions_RandomPort(t *testing.T) {
Expand Down Expand Up @@ -97,10 +107,7 @@ func TestConfigFile(t *testing.T) {
t.Fatalf("Received an error reading config file: %v\n", err)
}

if !reflect.DeepEqual(golden, opts) {
t.Fatalf("Options are incorrect.\nexpected: %+v\ngot: %+v",
golden, opts)
}
checkOptionsEqual(t, golden, opts)
}

func TestTLSConfigFile(t *testing.T) {
Expand All @@ -122,10 +129,8 @@ func TestTLSConfigFile(t *testing.T) {
t.Fatal("Expected opts.TLSConfig to be non-nil")
}
opts.TLSConfig = nil
if !reflect.DeepEqual(golden, opts) {
t.Fatalf("Options are incorrect.\nexpected: %+v\ngot: %+v",
golden, opts)
}
checkOptionsEqual(t, golden, opts)

// Now check TLSConfig a bit more closely
// CipherSuites
ciphers := defaultCipherSuites()
Expand Down Expand Up @@ -272,10 +277,7 @@ func TestMergeOverrides(t *testing.T) {
}
merged := MergeOptions(fopts, opts)

if !reflect.DeepEqual(golden, merged) {
t.Fatalf("Options are incorrect.\nexpected: %+v\ngot: %+v",
golden, merged)
}
checkOptionsEqual(t, golden, merged)
}

func TestRemoveSelfReference(t *testing.T) {
Expand Down Expand Up @@ -343,10 +345,7 @@ func TestRouteFlagOverride(t *testing.T) {
}
merged := MergeOptions(fopts, opts)

if !reflect.DeepEqual(golden, merged) {
t.Fatalf("Options are incorrect.\nexpected: %+v\ngot: %+v",
golden, merged)
}
checkOptionsEqual(t, golden, merged)
}

func TestClusterFlagsOverride(t *testing.T) {
Expand Down Expand Up @@ -387,10 +386,7 @@ func TestClusterFlagsOverride(t *testing.T) {
}
merged := MergeOptions(fopts, opts)

if !reflect.DeepEqual(golden, merged) {
t.Fatalf("Options are incorrect.\nexpected: %+v\ngot: %+v",
golden, merged)
}
checkOptionsEqual(t, golden, merged)
}

func TestRouteFlagOverrideWithMultiple(t *testing.T) {
Expand Down Expand Up @@ -423,10 +419,7 @@ func TestRouteFlagOverrideWithMultiple(t *testing.T) {
}
merged := MergeOptions(fopts, opts)

if !reflect.DeepEqual(golden, merged) {
t.Fatalf("Options are incorrect.\nexpected: %+v\ngot: %+v",
golden, merged)
}
checkOptionsEqual(t, golden, merged)
}

func TestDynamicPortOnListen(t *testing.T) {
Expand Down
31 changes: 31 additions & 0 deletions server/reload.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,6 +518,12 @@ func (s *Server) Reload() error {

// Apply flags over config file settings.
newOpts = MergeOptions(newOpts, FlagSnapshot)

// Need more processing for boolean flags...
if FlagSnapshot != nil {
applyBoolFlags(newOpts, FlagSnapshot)
}

setBaselineOptions(newOpts)

// processOptions sets Port to 0 if set to -1 (RANDOM port)
Expand All @@ -543,6 +549,31 @@ func (s *Server) Reload() error {
return nil
}

func applyBoolFlags(newOpts, flagOpts *Options) {
// Reset fields that may have been set to `true` in
// MergeOptions() when some of the flags default to `true`
// but have not been explicitly set and therefore value
// from config file should take precedence.
for name, val := range newOpts.inConfig {
f := reflect.ValueOf(newOpts).Elem()
names := strings.Split(name, ".")
for _, name := range names {
f = f.FieldByName(name)
}
f.SetBool(val)
}
// Now apply value (true or false) from flags that have
// been explicitly set in command line
for name, val := range flagOpts.inCmdLine {
f := reflect.ValueOf(newOpts).Elem()
names := strings.Split(name, ".")
for _, name := range names {
f = f.FieldByName(name)
}
f.SetBool(val)
}
}

// reloadOptions reloads the server config with the provided options. If an
// option that doesn't support hot-swapping is changed, this returns an error.
func (s *Server) reloadOptions(newOpts *Options) error {
Expand Down
Loading