Merge pull request #874 from nats-io/fix_logtime_reload

[FIXED] Logtime reset to true on config reload

Author: kozlovic

server/opts.go

@@ -2249,6 +2249,9 @@ func ConfigureOptions(fs *flag.FlagSet, args []string, printVersion, printHelp,
 		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.")
@@ -2300,6 +2303,7 @@ 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
@@ -2344,6 +2348,20 @@ 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
+	fs.Visit(func(f *flag.Flag) {
+		if f.Name == "logtime" {
+			logtimeIsExplicitlySet = true
+		}
+	})
+	if !logtimeIsExplicitlySet {
+		FlagSnapshot.Logtime = false
+	}
+
 	// Process signal control.
 	if signal != "" {
 		if err := processSignal(signal); err != nil {

server/reload_test.go

@@ -17,6 +17,7 @@ import (
 	"crypto/tls"
 	"encoding/base64"
 	"encoding/json"
+	"flag"
 	"fmt"
 	"io/ioutil"
 	"net"
@@ -3215,3 +3216,37 @@ func TestConfigReloadNotPreventedByGateways(t *testing.T) {
 		t.Fatalf("Expected Reload to return a not supported error, got %v", err)
 	}
 }
+
+func TestConfigReloadLogtime(t *testing.T) {
+	logfile := "logtime.log"
+	defer os.Remove(logfile)
+	content := `
+		listen: "127.0.0.1:-1"
+		logfile: "%s"
+		logtime: false
+	`
+	conf := createConfFile(t, []byte(fmt.Sprintf(content, logfile)))
+	defer os.Remove(conf)
+
+	// For this test, we need to invoke ConfigureOptions which is what main.go does.
+	fs := flag.NewFlagSet("test", flag.ContinueOnError)
+	opts, err := ConfigureOptions(fs, []string{"-c", conf}, nil, nil, nil)
+	if err != nil {
+		t.Fatalf("Error processing config: %v", err)
+	}
+	opts.NoSigs = true
+	s := RunServer(opts)
+	defer s.Shutdown()
+
+	if s.getOpts().Logtime {
+		t.Fatal("Logtime should be set to false")
+	}
+
+	if err := s.Reload(); err != nil {
+		t.Fatalf("Error on reload: %v", err)
+	}
+
+	if s.getOpts().Logtime {
+		t.Fatal("Logtime should be set to false")
+	}
+}