Address duplicate node-id issue with math/rand (#127)

* Addresses duplicate node-id issue with math/rand

* Remove extra newline

* Add tests

* Throw error if we are unable to resolve the node id from the config file

* Cleanup

* Cleaning up test

Author: davissp14

internal/flypg/node.go

@@ -2,10 +2,8 @@ package flypg
 
 import (
 	"context"
-	"encoding/binary"
 	"errors"
 	"fmt"
-	"math/rand"
 	"net"
 	"os"
 	"os/exec"
@@ -86,13 +84,7 @@ func NewNode() (*Node, error) {
 		Password: os.Getenv("REPL_PASSWORD"),
 	}
 
-	// Generate a random, reconstructable signed int32
-	machineID := os.Getenv("FLY_ALLOC_ID")
-	seed := binary.LittleEndian.Uint64([]byte(machineID))
-	rand.Seed(int64(seed))
-
 	node.RepMgr = RepMgr{
-		ID:                 rand.Int31(), // skipcq: GSC-G404
 		AppName:            node.AppName,
 		PrimaryRegion:      node.PrimaryRegion,
 		Region:             os.Getenv("FLY_REGION"),

internal/flypg/repmgr.go

@@ -2,7 +2,10 @@ package flypg
 
 import (
 	"context"
+	"crypto/rand"
 	"fmt"
+	"math"
+	"math/big"
 	"net"
 	"os"
 	"strconv"
@@ -95,7 +98,9 @@ func (r *RepMgr) NewRemoteConnection(ctx context.Context, hostname string) (*pgx
 }
 
 func (r *RepMgr) initialize() error {
-	r.setDefaults()
+	if err := r.setDefaults(); err != nil {
+		return fmt.Errorf("failed to set repmgr defaults: %s", err)
+	}
 
 	file, err := os.Create(r.ConfigPath)
 	if err != nil {
@@ -139,9 +144,14 @@ func (r *RepMgr) setup(ctx context.Context, conn *pgx.Conn) error {
 	return nil
 }
 
-func (r *RepMgr) setDefaults() {
+func (r *RepMgr) setDefaults() error {
+	nodeID, err := r.resolveNodeID()
+	if err != nil {
+		return err
+	}
+
 	conf := ConfigMap{
-		"node_id":                      fmt.Sprint(r.ID),
+		"node_id":                      nodeID,
 		"node_name":                    fmt.Sprintf("'%s'", r.PrivateIP),
 		"conninfo":                     fmt.Sprintf("'host=%s port=%d user=%s dbname=%s connect_timeout=10'", r.PrivateIP, r.Port, r.Credentials.Username, r.DatabaseName),
 		"data_directory":               fmt.Sprintf("'%s'", r.DataDir),
@@ -163,6 +173,36 @@ func (r *RepMgr) setDefaults() {
 	}
 
 	r.internalConfig = conf
+
+	return nil
+}
+
+func (r *RepMgr) resolveNodeID() (string, error) {
+	var nodeID string
+	if utils.FileExists(r.InternalConfigFile()) {
+		// Pull existing id from configuraiton file
+		config, err := r.CurrentConfig()
+		if err != nil {
+			return "", fmt.Errorf("failed to resolve current repmgr config: %s", err)
+		}
+
+		if val, ok := config["node_id"]; ok {
+			nodeID = fmt.Sprint(val)
+		}
+
+		if nodeID == "" {
+			return "", fmt.Errorf("failed to resolve existing node_id: %s", err)
+		}
+	} else {
+		// Generate a new random id
+		id, err := rand.Int(rand.Reader, big.NewInt(math.MaxInt32))
+		if err != nil {
+			return "", fmt.Errorf("failed to generate node id: %s", err)
+		}
+		nodeID = id.String()
+	}
+
+	return nodeID, nil
 }
 
 func (r *RepMgr) registerPrimary() error {

internal/flypg/repmgr_test.go

@@ -0,0 +1,84 @@
+package flypg
+
+import (
+	"testing"
+)
+
+const (
+	repmgrTestDirectory          = "./test_results"
+	repmgrConfigFilePath         = "./test_results/repmgr.conf"
+	repgmrInternalConfigFilePath = "./test_results/repmgr.internal.conf"
+	repgmrUserConfigFilePath     = "./test_results/repmgr.internal.conf"
+)
+
+func TestRepmgrConfigDefaults(t *testing.T) {
+	if err := setup(t); err != nil {
+		t.Fatal(err)
+	}
+	defer cleanup()
+
+	conf := &RepMgr{
+		AppName:            "test-app",
+		PrimaryRegion:      "dev",
+		Region:             "dev",
+		ConfigPath:         repmgrConfigFilePath,
+		InternalConfigPath: repgmrInternalConfigFilePath,
+		UserConfigPath:     repgmrUserConfigFilePath,
+		DataDir:            repmgrTestDirectory,
+		PrivateIP:          "127.0.0.1",
+		Port:               5433,
+		DatabaseName:       "repmgr",
+	}
+
+	if err := conf.setDefaults(); err != nil {
+		t.Error(err)
+	}
+
+	if conf.internalConfig["node_name"] != "'127.0.0.1'" {
+		t.Fatalf("expected node_name to be '127.0.0.1', got %v", conf.internalConfig["node_name"])
+	}
+
+	if conf.internalConfig["node_id"] == "" {
+		t.Fatalf("expected node_id to not be empty, got %q", conf.internalConfig["node_id"])
+	}
+
+}
+
+func TestRepmgrNodeIDGeneration(t *testing.T) {
+	if err := setup(t); err != nil {
+		t.Fatal(err)
+	}
+	defer cleanup()
+
+	conf := &RepMgr{
+		AppName:            "test-app",
+		PrimaryRegion:      "dev",
+		Region:             "dev",
+		ConfigPath:         repmgrConfigFilePath,
+		InternalConfigPath: repgmrInternalConfigFilePath,
+		UserConfigPath:     repgmrUserConfigFilePath,
+		DataDir:            repmgrTestDirectory,
+		PrivateIP:          "127.0.0.1",
+		Port:               5433,
+		DatabaseName:       "repmgr",
+	}
+
+	if err := conf.setDefaults(); err != nil {
+		t.Error(err)
+	}
+
+	if err := writeInternalConfigFile(conf); err != nil {
+		t.Error(err)
+	}
+
+	nodeID := conf.internalConfig["node_id"]
+
+	resolvedNodeID, err := conf.resolveNodeID()
+	if err != nil {
+		t.Error(err)
+	}
+
+	if nodeID != resolvedNodeID {
+		t.Fatalf("expected node_id to be %s, got %q", nodeID, resolvedNodeID)
+	}
+}

internal/utils/shell.go

@@ -39,6 +39,13 @@ func SetFileOwnership(pathToFile, owner string) error {
 	return nil
 }
 
+func FileExists(path string) bool {
+	if _, err := os.Stat(path); err != nil {
+		return false
+	}
+	return true
+}
+
 func SystemUserIDs(usr string) (int, int, error) {
 	pgUser, err := user.Lookup(usr)
 	if err != nil {