Merge pull request #980 from rqlite/config-validate

Move config validation to Config type

CHANGELOG.md

@@ -3,6 +3,7 @@
 ### Implementation changes and bug fixes
 - [PR #976](https://github.com/rqlite/rqlite/pull/976): Improve `readyz/` response.
 - [PR #978](https://github.com/rqlite/rqlite/pull/978): Return error on join request if node ID is the same as receiving node.
+- [PR #980](https://github.com/rqlite/rqlite/pull/980): Move config validation to Config type.
 
 ## 7.1.0 (January 28th 2022)
 This release introduces a new automatic clustering approach, known as _Bootstrapping_, which allows rqlite clusters to form without assistance from an external system such as Consul. This can be very useful for certain deployment scenarios. See the [documentation](https://github.com/rqlite/rqlite/blob/master/DOC/AUTO_CLUSTERING.md) for full details on using the new Bootstrapping mode. Special thanks to [Nathan Ferch](https://github.com/nferch) for his advice regarding the design and development of this feature.

cmd/rqlited/flags.go

@@ -1,9 +1,11 @@
 package main
 
 import (
+	"errors"
 	"flag"
 	"fmt"
 	"net"
+	"net/url"
 	"os"
 	"runtime"
 	"strings"
@@ -163,9 +165,6 @@ type Config struct {
 	// CompressionBatch sets request batch threshold for compression attempt.
 	CompressionBatch int
 
-	// ShowVersion instructs the node to show version information and exit.
-	ShowVersion bool
-
 	// CPUProfile enables CPU profiling.
 	CPUProfile string
 
@@ -173,6 +172,90 @@ type Config struct {
 	MemProfile string
 }
 
+// Validate checks the configuration for internal consistency, and activates
+// important rqlite policies. It must be called at least once on a Config
+// object before the Config object is used. It is OK to call more than
+// once.
+func (c *Config) Validate() error {
+	if c.OnDiskPath != "" && !c.OnDisk {
+		return errors.New("-on-disk-path is set, but -on-disk is not")
+	}
+
+	// Enforce policies regarding addresses
+	if c.RaftAdv == "" {
+		c.RaftAdv = c.RaftAddr
+	}
+	if c.HTTPAdv == "" {
+		c.HTTPAdv = c.HTTPAddr
+	}
+
+	// Node ID policy
+	if c.NodeID == "" {
+		c.NodeID = c.RaftAdv
+	}
+
+	// Perfom some address validity checks.
+	if strings.HasPrefix(strings.ToLower(c.HTTPAddr), "http") ||
+		strings.HasPrefix(strings.ToLower(c.HTTPAdv), "http") {
+		return errors.New("HTTP options should not include protocol (http:// or https://)")
+	}
+	if _, _, err := net.SplitHostPort(c.HTTPAddr); err != nil {
+		return errors.New("HTTP bind address not valid")
+	}
+	if _, _, err := net.SplitHostPort(c.HTTPAdv); err != nil {
+		return errors.New("HTTP advertised address not valid")
+	}
+	if _, _, err := net.SplitHostPort(c.RaftAddr); err != nil {
+		return errors.New("raft bind address not valid")
+	}
+	if _, _, err := net.SplitHostPort(c.RaftAdv); err != nil {
+		return errors.New("raft advertised address not valid")
+	}
+
+	// Enforce bootstrapping policies
+	if c.BootstrapExpect > 0 && c.RaftNonVoter {
+		return errors.New("bootstrapping only applicable to voting nodes")
+	}
+
+	// Join addresses OK?
+	if c.JoinAddr != "" {
+		addrs := strings.Split(c.JoinAddr, ",")
+		for i := range addrs {
+			u, err := url.Parse(addrs[i])
+			if err != nil {
+				return fmt.Errorf("%s is an invalid join adddress", addrs[i])
+			}
+			if c.BootstrapExpect == 0 {
+				if u.Host == c.HTTPAdv || addrs[i] == c.HTTPAddr {
+					return errors.New("node cannot join with itself unless bootstrapping")
+				}
+			}
+		}
+	}
+
+	// Valid disco mode?
+	switch c.DiscoMode {
+	case "":
+	case DiscoModeEtcdKV, DiscoModeConsulKV:
+		if c.BootstrapExpect > 0 {
+			return fmt.Errorf("bootstrapping not applicable when using %s", c.DiscoMode)
+		}
+	default:
+		return fmt.Errorf("disco mode must be %s or %s", DiscoModeConsulKV, DiscoModeEtcdKV)
+	}
+
+	return nil
+}
+
+// JoinAddresses returns the join addresses set at the command line. Returns nil
+// if no join addresses were set.
+func (c *Config) JoinAddresses() []string {
+	if c.JoinAddr == "" {
+		return nil
+	}
+	return strings.Split(c.JoinAddr, ",")
+}
+
 // HTTPURL returns the fully-formed, advertised HTTP API address for this config, including
 // protocol, host and port.
 func (c *Config) HTTPURL() string {
@@ -195,7 +278,8 @@ func ParseFlags(name, desc string, build *BuildInfo) (*Config, error) {
 	if flag.Parsed() {
 		return nil, fmt.Errorf("command-line flags already parsed")
 	}
-	config := Config{}
+	config := &Config{}
+	showVersion := false
 
 	flag.StringVar(&config.NodeID, "node-id", "", "Unique name for node. If not set, set to advertised Raft address")
 	flag.StringVar(&config.HTTPAddr, "http-addr", "localhost:4001", "HTTP server bind address. To enable HTTPS, set X.509 cert and key")
@@ -229,7 +313,7 @@ func ParseFlags(name, desc string, build *BuildInfo) (*Config, error) {
 	flag.StringVar(&config.OnDiskPath, "on-disk-path", "", "Path for SQLite on-disk database file. If not set, use file in data directory")
 	flag.BoolVar(&config.OnDiskStartup, "on-disk-startup", false, "Do not initialize on-disk database in memory first at startup")
 	flag.BoolVar(&config.FKConstraints, "fk", false, "Enable SQLite foreign key constraints")
-	flag.BoolVar(&config.ShowVersion, "version", false, "Show version information and exit")
+	flag.BoolVar(&showVersion, "version", false, "Show version information and exit")
 	flag.BoolVar(&config.RaftNonVoter, "raft-non-voter", false, "Configure as non-voting node")
 	flag.DurationVar(&config.RaftHeartbeatTimeout, "raft-timeout", time.Second, "Raft heartbeat timeout")
 	flag.DurationVar(&config.RaftElectionTimeout, "raft-election-timeout", time.Second, "Raft election timeout")
@@ -251,8 +335,8 @@ func ParseFlags(name, desc string, build *BuildInfo) (*Config, error) {
 	}
 	flag.Parse()
 
-	if config.ShowVersion {
-		msg := fmt.Sprintf("%s %s %s %s %s (commit %s, branch %s, compiler %s)\n",
+	if showVersion {
+		msg := fmt.Sprintf("%s %s %s %s %s (commit %s, branch %s, compiler %s)",
 			name, build.Version, runtime.GOOS, runtime.GOARCH, runtime.Version(), build.Commit, build.Branch, runtime.Compiler)
 		errorExit(0, msg)
 	}
@@ -268,60 +352,10 @@ func ParseFlags(name, desc string, build *BuildInfo) (*Config, error) {
 		errorExit(1, "arguments after data directory are not accepted")
 	}
 
-	if config.OnDiskPath != "" && !config.OnDisk {
-		errorExit(1, "on-disk-path is set, but on-disk is not")
+	if err := config.Validate(); err != nil {
+		errorExit(1, err.Error())
 	}
-
-	// Enforce policies regarding addresses
-	if config.RaftAdv == "" {
-		config.RaftAdv = config.RaftAddr
-	}
-	if config.HTTPAdv == "" {
-		config.HTTPAdv = config.HTTPAddr
-	}
-
-	// Perfom some address validity checks.
-	if strings.HasPrefix(strings.ToLower(config.HTTPAddr), "http") ||
-		strings.HasPrefix(strings.ToLower(config.HTTPAdv), "http") {
-		errorExit(1, "HTTP options should not include protocol (http:// or https://)")
-	}
-	if _, _, err := net.SplitHostPort(config.HTTPAddr); err != nil {
-		errorExit(1, "HTTP bind address not valid")
-	}
-	if _, _, err := net.SplitHostPort(config.HTTPAdv); err != nil {
-		errorExit(1, "HTTP advertised address not valid")
-	}
-	if _, _, err := net.SplitHostPort(config.RaftAddr); err != nil {
-		errorExit(1, "raft bind address not valid")
-	}
-	if _, _, err := net.SplitHostPort(config.RaftAdv); err != nil {
-		errorExit(1, "raft advertised address not valid")
-	}
-
-	// Enforce bootstrapping policies
-	if config.BootstrapExpect > 0 && config.RaftNonVoter {
-		errorExit(1, "bootstrapping only applicable to voting nodes")
-	}
-
-	// Valid disco mode?
-	switch config.DiscoMode {
-	case "":
-	case DiscoModeEtcdKV, DiscoModeConsulKV:
-		if config.BootstrapExpect > 0 {
-			errorExit(1, fmt.Sprintf("bootstrapping not applicable when using %s",
-				config.DiscoMode))
-		}
-	default:
-		errorExit(1, fmt.Sprintf("invalid disco mode, choose %s or %s",
-			DiscoModeConsulKV, DiscoModeEtcdKV))
-	}
-
-	// Node ID policy
-	if config.NodeID == "" {
-		config.NodeID = config.RaftAdv
-	}
-
-	return &config, nil
+	return config, nil
 }
 
 func errorExit(code int, msg string) {

cmd/rqlited/main.go

@@ -88,13 +88,6 @@ func main() {
 		log.Fatalf("failed to create store: %s", err.Error())
 	}
 
-	// Determine join addresses
-	var joins []string
-	joins, err = determineJoinAddresses(cfg)
-	if err != nil {
-		log.Fatalf("unable to determine join addresses: %s", err.Error())
-	}
-
 	// Now, open store.
 	if err := str.Open(); err != nil {
 		log.Fatalf("failed to open store: %s", err.Error())
@@ -145,7 +138,7 @@ func main() {
 	if err != nil {
 		log.Fatalf("failed to get nodes %s", err.Error())
 	}
-	if err := createCluster(cfg, joins, &tlsConfig, len(nodes) > 0, str, httpServ, credStr); err != nil {
+	if err := createCluster(cfg, &tlsConfig, len(nodes) > 0, str, httpServ, credStr); err != nil {
 		log.Fatalf("clustering failure: %s", err.Error())
 	}
 
@@ -167,27 +160,6 @@ func main() {
 	log.Println("rqlite server stopped")
 }
 
-// determineJoinAddresses returns the join addresses supplied at the command-line.
-// removing any occurence of this nodes HTTP address.
-func determineJoinAddresses(cfg *Config) ([]string, error) {
-	var addrs []string
-	if cfg.JoinAddr != "" {
-		addrs = strings.Split(cfg.JoinAddr, ",")
-	}
-
-	// It won't work to attempt an explicit self-join, so remove any such address.
-	var validAddrs []string
-	for i := range addrs {
-		if addrs[i] == cfg.HTTPAdv || addrs[i] == cfg.HTTPAddr {
-			log.Printf("ignoring join address %s equal to this node's address", addrs[i])
-			continue
-		}
-		validAddrs = append(validAddrs, addrs[i])
-	}
-
-	return validAddrs, nil
-}
-
 func createStore(cfg *Config, ln *tcp.Layer) (*store.Store, error) {
 	dataPath, err := filepath.Abs(cfg.DataPath)
 	if err != nil {
@@ -355,9 +327,10 @@ func clusterService(cfg *Config, tn cluster.Transport, db cluster.Database) (*cl
 	return c, nil
 }
 
-func createCluster(cfg *Config, joins []string, tlsConfig *tls.Config, hasPeers bool, str *store.Store,
+func createCluster(cfg *Config, tlsConfig *tls.Config, hasPeers bool, str *store.Store,
 	httpServ *httpd.Service, credStr *auth.CredentialsStore) error {
-	if len(joins) == 0 && cfg.DiscoMode == "" && !hasPeers {
+	joins := cfg.JoinAddresses()
+	if joins == nil && cfg.DiscoMode == "" && !hasPeers {
 		// Brand new node, told to bootstrap itself. So do it.
 		log.Println("bootstraping single new node")
 		if err := str.Bootstrap(store.NewServer(str.ID(), str.Addr(), true)); err != nil {
@@ -366,11 +339,9 @@ func createCluster(cfg *Config, joins []string, tlsConfig *tls.Config, hasPeers
 		return nil
 	}
 
-	if len(joins) > 0 {
+	if joins != nil {
 		if cfg.BootstrapExpect == 0 {
 			// Explicit join operation requested, so do it.
-			log.Println("explicit join addresses are:", joins)
-
 			if err := addJoinCreds(joins, cfg.JoinAs, credStr); err != nil {
 				return fmt.Errorf("failed to add BasicAuth creds: %s", err.Error())
 			}
@@ -388,13 +359,10 @@ func createCluster(cfg *Config, joins []string, tlsConfig *tls.Config, hasPeers
 			return nil
 		}
 
-		// Must self-notify when bootstrapping
-		targets := append(joins, cfg.HTTPAdv)
-		log.Println("bootstrap addresses are:", targets)
-		if err := addJoinCreds(targets, cfg.JoinAs, credStr); err != nil {
+		if err := addJoinCreds(joins, cfg.JoinAs, credStr); err != nil {
 			return fmt.Errorf("failed to add BasicAuth creds: %s", err.Error())
 		}
-		bs := cluster.NewBootstrapper(cluster.NewAddressProviderString(targets),
+		bs := cluster.NewBootstrapper(cluster.NewAddressProviderString(joins),
 			cfg.BootstrapExpect, tlsConfig)
 
 		done := func() bool {