[FIXED] Override of config file options with command line parameters

The idea is to call flag.Parse() twice. The first time - as it was
the case before - is setting the empty Options structure with all
the specified flag defaults and with values provided from the command
line.
If a config file is specified, we then apply to this options structure
the content of the config file. This is done with a new receiver
function for the Options structure. After that, we call flag.Parse() again
which will override the latest changes with any command line parameter.
We don't need MergeOptions() anymore since the above process takes
care of the proper override.

Resolves #574

main.go

@@ -3,11 +3,13 @@
 package main
 
 import (
+	"errors"
 	"flag"
 	"fmt"
 	"net"
 	"net/url"
 	"os"
+	"strconv"
 	"strings"
 
 	"github.com/nats-io/gnatsd/server"
@@ -70,11 +72,10 @@ func main() {
 	opts := &server.Options{}
 
 	var (
-		showVersion   bool
-		debugAndTrace bool
-		configFile    string
-		signal        string
-		showTLSHelp   bool
+		showVersion bool
+		configFile  string
+		signal      string
+		showTLSHelp bool
 	)
 
 	// Parse flags
@@ -87,7 +88,7 @@ func main() {
 	flag.BoolVar(&opts.Debug, "debug", false, "Enable Debug logging.")
 	flag.BoolVar(&opts.Trace, "V", false, "Enable Trace logging.")
 	flag.BoolVar(&opts.Trace, "trace", false, "Enable Trace logging.")
-	flag.BoolVar(&debugAndTrace, "DV", false, "Enable Debug and Trace logging.")
+	flag.Bool("DV", false, "Enable Debug and Trace logging.")
 	flag.BoolVar(&opts.Logtime, "T", true, "Timestamp log entries.")
 	flag.BoolVar(&opts.Logtime, "logtime", true, "Timestamp log entries.")
 	flag.StringVar(&opts.Username, "user", "", "Username required for connection.")
@@ -128,6 +129,14 @@ func main() {
 		fmt.Printf("%s\n", usageStr)
 	}
 
+	// The flags definition above set "default" values to some of the options.
+	// Calling Parse() here will override the default options with any value
+	// specified from the command line. This is ok. We will then update the
+	// options with the content of the configuration file (if present), and then,
+	// call Parse() again to override the default+config with command line values.
+	// Calling Parse() before processing config file is necessary since configFile
+	// itself is a command line argument, and also Parse() is required in order
+	// to know if user wants simply to show "help" or "version", etc...
 	flag.Parse()
 
 	// Show version and exit
@@ -139,11 +148,6 @@ func main() {
 		server.PrintTLSHelpAndDie()
 	}
 
-	// One flag can set multiple options.
-	if debugAndTrace {
-		opts.Trace, opts.Debug = true, true
-	}
-
 	// Process args looking for non-flag options,
 	// 'version' and 'help' only for now
 	showVersion, showHelp, err := server.ProcessCommandLineArgs(flag.CommandLine)
@@ -165,13 +169,34 @@ func main() {
 
 	// Parse config if given
 	if configFile != "" {
-		fileOpts, err := server.ProcessConfigFile(configFile)
-		if err != nil {
+		// This will update the options with values from the config file.
+		if err := opts.ProcessConfigFile(configFile); err != nil {
 			server.PrintAndDie(err.Error())
 		}
-		opts = server.MergeOptions(fileOpts, opts)
+		// Call this again to override config file options with options from command line.
+		flag.Parse()
 	}
 
+	// Special handling of some flags
+	flag.Visit(func(f *flag.Flag) {
+		switch f.Name {
+		case "DV":
+			// Check value to support -DV=false
+			boolValue, _ := strconv.ParseBool(f.Value.String())
+			opts.Trace, opts.Debug = boolValue, boolValue
+		case "routes":
+			// Keep in mind that the flag has updated opts.RoutesStr at this point.
+			if opts.RoutesStr == "" {
+				// Set routes array to nil since routes string is empty
+				opts.Routes = nil
+				return
+			}
+			routeUrls := server.RoutesFromStr(f.Value.String())
+			opts.Routes = routeUrls
+			opts.RoutesStr = f.Value.String()
+		}
+	})
+
 	// Remove any host/ip that points to itself in Route
 	newroutes, err := server.RemoveSelfReference(opts.Cluster.Port, opts.Routes)
 	if err != nil {
@@ -257,7 +282,7 @@ func configureClusterOpts(opts *server.Options) error {
 		if clusterURL.User != nil {
 			pass, hasPassword := clusterURL.User.Password()
 			if !hasPassword {
-				return fmt.Errorf("Expected cluster password to be set.")
+				return errors.New("expected cluster password to be set")
 			}
 			opts.Cluster.Password = pass
 

server/opts.go

@@ -18,7 +18,7 @@ import (
 	"github.com/nats-io/gnatsd/util"
 )
 
-// Options for clusters.
+// ClusterOpts are options for clusters.
 type ClusterOpts struct {
 	Host           string      `json:"addr"`
 	Port           int         `json:"cluster_port"`
@@ -156,129 +156,150 @@ Available cipher suites include:
 // ProcessConfigFile processes a configuration file.
 // FIXME(dlc): Hacky
 func ProcessConfigFile(configFile string) (*Options, error) {
-	opts := &Options{ConfigFile: configFile}
+	opts := &Options{}
+	if err := opts.ProcessConfigFile(configFile); err != nil {
+		return nil, err
+	}
+	return opts, nil
+}
 
+// ProcessConfigFile updates the Options structure with options
+// present in the given configuration file.
+// This version is convenient if one wants to set some default
+// options and then override them with what is in the config file.
+// For instance, this version allows you to do something such as:
+//
+// opts := &Options{Debug: true}
+// opts.ProcessConfigFile(myConfigFile)
+//
+// If the config file contains "debug: false", after this call,
+// opts.Debug would really be false. It would be impossible to
+// achieve that with the non receiver ProcessConfigFile() version,
+// since one would not know after the call if "debug" was not present
+// or was present but set to false.
+func (o *Options) ProcessConfigFile(configFile string) error {
+	o.ConfigFile = configFile
 	if configFile == "" {
-		return opts, nil
+		return nil
 	}
 
 	m, err := conf.ParseFile(configFile)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	for k, v := range m {
 		switch strings.ToLower(k) {
 		case "listen":
 			hp, err := parseListen(v)
 			if err != nil {
-				return nil, err
+				return err
 			}
-			opts.Host = hp.host
-			opts.Port = hp.port
+			o.Host = hp.host
+			o.Port = hp.port
 		case "port":
-			opts.Port = int(v.(int64))
+			o.Port = int(v.(int64))
 		case "host", "net":
-			opts.Host = v.(string)
+			o.Host = v.(string)
 		case "debug":
-			opts.Debug = v.(bool)
+			o.Debug = v.(bool)
 		case "trace":
-			opts.Trace = v.(bool)
+			o.Trace = v.(bool)
 		case "logtime":
-			opts.Logtime = v.(bool)
+			o.Logtime = v.(bool)
 		case "authorization":
 			am := v.(map[string]interface{})
 			auth, err := parseAuthorization(am)
 			if err != nil {
-				return nil, err
+				return err
 			}
-			opts.Username = auth.user
-			opts.Password = auth.pass
-			opts.Authorization = auth.token
+			o.Username = auth.user
+			o.Password = auth.pass
+			o.Authorization = auth.token
 			if (auth.user != "" || auth.pass != "") && auth.token != "" {
-				return nil, fmt.Errorf("Cannot have a user/pass and token")
+				return fmt.Errorf("Cannot have a user/pass and token")
 			}
-			opts.AuthTimeout = auth.timeout
+			o.AuthTimeout = auth.timeout
 			// Check for multiple users defined
 			if auth.users != nil {
 				if auth.user != "" {
-					return nil, fmt.Errorf("Can not have a single user/pass and a users array")
+					return fmt.Errorf("Can not have a single user/pass and a users array")
 				}
 				if auth.token != "" {
-					return nil, fmt.Errorf("Can not have a token and a users array")
+					return fmt.Errorf("Can not have a token and a users array")
 				}
-				opts.Users = auth.users
+				o.Users = auth.users
 			}
 		case "http":
 			hp, err := parseListen(v)
 			if err != nil {
-				return nil, err
+				return err
 			}
-			opts.HTTPHost = hp.host
-			opts.HTTPPort = hp.port
+			o.HTTPHost = hp.host
+			o.HTTPPort = hp.port
 		case "https":
 			hp, err := parseListen(v)
 			if err != nil {
-				return nil, err
+				return err
 			}
-			opts.HTTPHost = hp.host
-			opts.HTTPSPort = hp.port
+			o.HTTPHost = hp.host
+			o.HTTPSPort = hp.port
 		case "http_port", "monitor_port":
-			opts.HTTPPort = int(v.(int64))
+			o.HTTPPort = int(v.(int64))
 		case "https_port":
-			opts.HTTPSPort = int(v.(int64))
+			o.HTTPSPort = int(v.(int64))
 		case "cluster":
 			cm := v.(map[string]interface{})
-			if err := parseCluster(cm, opts); err != nil {
-				return nil, err
+			if err := parseCluster(cm, o); err != nil {
+				return err
 			}
 		case "logfile", "log_file":
-			opts.LogFile = v.(string)
+			o.LogFile = v.(string)
 		case "syslog":
-			opts.Syslog = v.(bool)
+			o.Syslog = v.(bool)
 		case "remote_syslog":
-			opts.RemoteSyslog = v.(string)
+			o.RemoteSyslog = v.(string)
 		case "pidfile", "pid_file":
-			opts.PidFile = v.(string)
+			o.PidFile = v.(string)
 		case "prof_port":
-			opts.ProfPort = int(v.(int64))
+			o.ProfPort = int(v.(int64))
 		case "max_control_line":
-			opts.MaxControlLine = int(v.(int64))
+			o.MaxControlLine = int(v.(int64))
 		case "max_payload":
-			opts.MaxPayload = int(v.(int64))
+			o.MaxPayload = int(v.(int64))
 		case "max_connections", "max_conn":
-			opts.MaxConn = int(v.(int64))
+			o.MaxConn = int(v.(int64))
 		case "ping_interval":
-			opts.PingInterval = time.Duration(int(v.(int64))) * time.Second
+			o.PingInterval = time.Duration(int(v.(int64))) * time.Second
 		case "ping_max":
-			opts.MaxPingsOut = int(v.(int64))
+			o.MaxPingsOut = int(v.(int64))
 		case "tls":
 			tlsm := v.(map[string]interface{})
 			tc, err := parseTLS(tlsm)
 			if err != nil {
-				return nil, err
+				return err
 			}
-			if opts.TLSConfig, err = GenTLSConfig(tc); err != nil {
-				return nil, err
+			if o.TLSConfig, err = GenTLSConfig(tc); err != nil {
+				return err
 			}
-			opts.TLSTimeout = tc.Timeout
+			o.TLSTimeout = tc.Timeout
 		case "write_deadline":
 			wd, ok := v.(string)
 			if ok {
 				dur, err := time.ParseDuration(wd)
 				if err != nil {
-					return nil, fmt.Errorf("error parsing write_deadline: %v", err)
+					return fmt.Errorf("error parsing write_deadline: %v", err)
 				}
-				opts.WriteDeadline = dur
+				o.WriteDeadline = dur
 			} else {
 				// Backward compatible with old type, assume this is the
 				// number of seconds.
-				opts.WriteDeadline = time.Duration(v.(int64)) * time.Second
+				o.WriteDeadline = time.Duration(v.(int64)) * time.Second
 				fmt.Printf("WARNING: write_deadline should be converted to a duration\n")
 			}
 		}
 	}
-	return opts, nil
+	return nil
 }
 
 // hostPort is simple struct to hold parsed listen/addr strings.
@@ -686,7 +707,7 @@ func MergeOptions(fileOpts, flagOpts *Options) *Options {
 		return fileOpts
 	}
 	// Merge the two, flagOpts override
-	opts := *fileOpts
+	opts := fileOpts.Clone()
 
 	if flagOpts.Port != 0 {
 		opts.Port = flagOpts.Port
@@ -734,9 +755,9 @@ func MergeOptions(fileOpts, flagOpts *Options) *Options {
 		opts.Cluster.ConnectRetries = flagOpts.Cluster.ConnectRetries
 	}
 	if flagOpts.RoutesStr != "" {
-		mergeRoutes(&opts, flagOpts)
+		mergeRoutes(opts, flagOpts)
 	}
-	return &opts
+	return opts
 }
 
 // RoutesFromStr parses route URLs from a string

server/opts_test.go

@@ -789,3 +789,32 @@ func TestMalformedClusterAddress(t *testing.T) {
 		t.Fatalf("Expected an error reading config file: got %+v\n", opts)
 	}
 }
+
+func TestOptionsProcessConfigFile(t *testing.T) {
+	// Create options with default values of Debug and Trace
+	// that are the opposite of what is in the config file.
+	// Set another option that is not present in the config file.
+	logFileName := "test.log"
+	opts := &Options{
+		Debug:   true,
+		Trace:   false,
+		LogFile: logFileName,
+	}
+	configFileName := "./configs/test.conf"
+	if err := opts.ProcessConfigFile(configFileName); err != nil {
+		t.Fatalf("Error processing config file: %v", err)
+	}
+	// Verify that values are as expected
+	if opts.ConfigFile != configFileName {
+		t.Fatalf("Expected ConfigFile to be set to %q, got %v", configFileName, opts.ConfigFile)
+	}
+	if opts.Debug {
+		t.Fatal("Debug option should have been set to false from config file")
+	}
+	if !opts.Trace {
+		t.Fatal("Trace option should have been set to true from config file")
+	}
+	if opts.LogFile != logFileName {
+		t.Fatalf("Expected LogFile to be %q, got %q", logFileName, opts.LogFile)
+	}
+}