Config mgmt updates + tests (#119)

* Config updates + tests

* Remove state changes

* Revert utils change

* Fix deepsource complaints

* Don't write to internal config file when syncing user config

* Don't overwrite internal config

* Config fix

* Expand tests a bit more

* Ditch commented out code

* cleanup

Author: davissp14

internal/api/handle_admin.go

@@ -138,37 +138,35 @@ func handleUpdatePostgresSettings(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	user, err := flypg.ReadFromFile(node.PGConfig.UserConfigFile())
+	cfg, err := flypg.ReadFromFile(node.PGConfig.UserConfigFile())
 	if err != nil {
 		renderErr(w, err)
 		return
 	}
 
-	var in map[string]interface{}
-
-	if err := json.NewDecoder(r.Body).Decode(&in); err != nil {
+	var requestedChanges map[string]interface{}
+	if err := json.NewDecoder(r.Body).Decode(&requestedChanges); err != nil {
 		renderErr(w, err)
 		return
 	}
 
-	for k, v := range in {
+	for k, v := range requestedChanges {
 		exists, err := admin.SettingExists(r.Context(), conn, k)
 		if err != nil {
 			renderErr(w, err)
-			return
 		}
 		if !exists {
 			renderErr(w, fmt.Errorf("invalid config option: %s", k))
 			return
 		}
-		user[k] = v
+		cfg[k] = v
 	}
 
-	node.PGConfig.SetUserConfig(user)
+	node.PGConfig.SetUserConfig(cfg)
 
 	var requiresRestart []string
 
-	for k := range user {
+	for k := range cfg {
 		restart, err := admin.SettingRequiresRestart(r.Context(), conn, k)
 		if err != nil {
 			renderErr(w, err)

internal/flypg/config.go

@@ -61,8 +61,8 @@ func SyncUserConfig(c Config, consul *state.Store) error {
 	}
 	c.SetUserConfig(cfg)
 
-	if err := WriteConfigFiles(c); err != nil {
-		return fmt.Errorf("failed to write to pg config file: %s", err)
+	if err := writeUserConfigFile(c); err != nil {
+		return fmt.Errorf("failed to write user config: %s", err)
 	}
 
 	return nil
@@ -167,11 +167,8 @@ func writeUserConfigFile(c Config) error {
 	}
 	defer func() { _ = file.Close() }()
 
-	internal := c.InternalConfig()
-
 	for key, value := range c.UserConfig() {
 		entry := fmt.Sprintf("%s = %v\n", key, value)
-		delete(internal, key)
 		file.Write([]byte(entry))
 	}
 

internal/flypg/pg.go

@@ -118,14 +118,14 @@ func (c *PGConfig) Print(w io.Writer) error {
 	return e.Encode(cfg)
 }
 
-// SetDefaults WriteDefaults will resolve the default configuration settings and write them to the
+// SetDefaults will resolve the default configuration settings and write them to the
 // internal config file.
 func (c *PGConfig) SetDefaults() error {
 	// The default wal_segment_size in mb
 	const walSegmentSize = 16
 
 	// Calculate total allocated disk in bytes
-	diskSizeBytes, err := diskSizeInBytes()
+	diskSizeBytes, err := diskSizeInBytes(c.dataDir)
 	if err != nil {
 		return fmt.Errorf("failed to fetch disk size: %s", err)
 	}
@@ -166,7 +166,7 @@ func (c *PGConfig) SetDefaults() error {
 		sharedPreloadLibraries = append(sharedPreloadLibraries, "timescaledb")
 	}
 
-	conf := ConfigMap{
+	c.internalConfig = ConfigMap{
 		"random_page_cost":         "1.1",
 		"port":                     c.port,
 		"shared_buffers":           fmt.Sprintf("%dMB", sharedBuffersMb),
@@ -183,11 +183,37 @@ func (c *PGConfig) SetDefaults() error {
 		"shared_preload_libraries": fmt.Sprintf("'%s'", strings.Join(sharedPreloadLibraries, ",")),
 	}
 
-	c.internalConfig = conf
+	if err := writeInternalConfigFile(c); err != nil {
+		return fmt.Errorf("failed to write internal config file: %s", err)
+	}
 
 	return nil
 }
 
+func memTotalInBytes() (int64, error) {
+	memoryStr := os.Getenv("FLY_VM_MEMORY_MB")
+	if memoryStr == "" {
+		return 0, fmt.Errorf("FLY_VM_MEMORY_MB envvar has not been set")
+	}
+
+	parsed, err := strconv.ParseInt(memoryStr, 10, 64)
+	if err != nil {
+		return 0, err
+	}
+
+	memoryBytes := parsed * (1024 * 1024)
+
+	return memoryBytes, nil
+}
+
+func diskSizeInBytes(dir string) (uint64, error) {
+	var stat syscall.Statfs_t
+	if err := syscall.Statfs(dir, &stat); err != nil {
+		return 0, err
+	}
+	return stat.Blocks * uint64(stat.Bsize), nil
+}
+
 func (c *PGConfig) RuntimeApply(ctx context.Context, conn *pgx.Conn) error {
 	for key, value := range c.userConfig {
 		if err := admin.SetConfigurationSetting(ctx, conn, key, value); err != nil {
@@ -201,24 +227,20 @@ func (c *PGConfig) RuntimeApply(ctx context.Context, conn *pgx.Conn) error {
 // initialize will ensure the required configuration files are stubbed and the parent
 // postgresql.conf file includes them.
 func (c *PGConfig) initialize() error {
-	if _, err := os.Stat(c.internalConfigFilePath); err != nil {
-		if os.IsNotExist(err) {
-			if _, err := utils.RunCommand(fmt.Sprintf("touch %s", c.internalConfigFilePath), "postgres"); err != nil {
-				return err
-			}
-		} else {
-			return err
-		}
+	if err := stubConfigurationFile(c.internalConfigFilePath); err != nil {
+		return err
 	}
 
-	if _, err := os.Stat(c.userConfigFilePath); err != nil {
-		if os.IsNotExist(err) {
-			if _, err := utils.RunCommand(fmt.Sprintf("touch %s", c.userConfigFilePath), "postgres"); err != nil {
-				return err
-			}
-		} else {
-			return err
-		}
+	if err := utils.SetFileOwnership(c.internalConfigFilePath, "postgres"); err != nil {
+		return err
+	}
+
+	if err := stubConfigurationFile(c.userConfigFilePath); err != nil {
+		return err
+	}
+
+	if err := utils.SetFileOwnership(c.userConfigFilePath, "postgres"); err != nil {
+		return err
 	}
 
 	b, err := os.ReadFile(c.configFilePath)
@@ -264,26 +286,19 @@ func (c *PGConfig) writePGConfigEntries(entries []string) error {
 	return file.Sync()
 }
 
-func memTotalInBytes() (int64, error) {
-	memoryStr := os.Getenv("FLY_VM_MEMORY_MB")
-	if memoryStr == "" {
-		return 0, fmt.Errorf("FLY_VM_MEMORY_MB envvar has not been set")
-	}
+func stubConfigurationFile(path string) error {
+	_, err := os.Stat(path)
+	if os.IsNotExist(err) {
+		file, err := os.Create(path)
+		if err != nil {
+			return err
+		}
+		defer func() { _ = file.Close() }()
 
-	parsed, err := strconv.ParseInt(memoryStr, 10, 64)
-	if err != nil {
-		return 0, err
+		return file.Sync()
+	} else if err != nil {
+		return nil
 	}
 
-	memoryBytes := parsed * (1024 * 1024)
-
-	return memoryBytes, nil
-}
-
-func diskSizeInBytes() (uint64, error) {
-	var stat syscall.Statfs_t
-	if err := syscall.Statfs("/data", &stat); err != nil {
-		return 0, err
-	}
-	return stat.Blocks * uint64(stat.Bsize), nil
+	return nil
 }

internal/flypg/pg_test.go

@@ -0,0 +1,126 @@
+package flypg
+
+import (
+	"fmt"
+	"os"
+	"testing"
+)
+
+const (
+	pgTestDirectory          = "./test_results"
+	pgConfigFilePath         = "./test_results/postgresql.conf"
+	pgInternalConfigFilePath = "./test_results/postgresql.internal.conf"
+	pgUserConfigFilePath     = "./test_results/postgresql.user.conf"
+)
+
+func TestPGConfigDefaults(t *testing.T) {
+	if err := setup(t); err != nil {
+		t.Fatal(err)
+	}
+	defer cleanup()
+
+	pgConf := &PGConfig{
+		dataDir:                pgTestDirectory,
+		port:                   5433,
+		configFilePath:         pgConfigFilePath,
+		internalConfigFilePath: pgInternalConfigFilePath,
+		userConfigFilePath:     pgUserConfigFilePath,
+		userConfig:             ConfigMap{},
+		internalConfig:         ConfigMap{},
+	}
+
+	if err := pgConf.initialize(); err != nil {
+		t.Error(err)
+	}
+
+	cfg, err := ReadFromFile(pgInternalConfigFilePath)
+	if err != nil {
+		t.Error(err)
+	}
+
+	if cfg["port"] != "5433" {
+		t.Fatalf("expected port to be 5433, got %v", cfg["port"])
+	}
+
+	if cfg["hot_standby"] != "true" {
+		t.Fatalf("expected hot_standby to be true, got %v", cfg["hot_standby"])
+	}
+}
+
+func TestPGSettingOverride(t *testing.T) {
+	if err := setup(t); err != nil {
+		t.Fatal(err)
+	}
+	defer cleanup()
+
+	pgConf := &PGConfig{
+		dataDir:                pgTestDirectory,
+		port:                   5433,
+		configFilePath:         pgConfigFilePath,
+		internalConfigFilePath: pgInternalConfigFilePath,
+		userConfigFilePath:     pgUserConfigFilePath,
+	}
+
+	if err := pgConf.initialize(); err != nil {
+		t.Error(err)
+	}
+
+	pgConf.SetUserConfig(ConfigMap{
+		"log_statement": "ddl",
+		"port":          "10000",
+	})
+
+	if err := WriteConfigFiles(pgConf); err != nil {
+		t.Error(err)
+	}
+
+	cfg, err := pgConf.CurrentConfig()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if cfg["port"] != "10000" {
+		t.Fatalf("expected port to be 10000, got %v", cfg["port"])
+	}
+
+	if cfg["log_statement"] != "ddl" {
+		t.Fatalf("expected log_statement to be ddl, got %v", cfg["log_statement"])
+	}
+
+	// Ensure defaults were not touched
+	if cfg["max_connections"] != "300" {
+		t.Fatalf("expected max_connections to be 300, got %v", cfg["max_connections"])
+	}
+
+	if cfg["hot_standby"] != "true" {
+		t.Fatalf("expected hot_standby to be true, got %v", cfg["hot_standby"])
+	}
+}
+
+func setup(t *testing.T) error {
+	t.Setenv("FLY_VM_MEMORY_MB", fmt.Sprint(256*(1024*1024)))
+	t.Setenv("UNIT_TESTING", "true")
+
+	if _, err := os.Stat(pgTestDirectory); err != nil {
+		if os.IsNotExist(err) {
+			if err := os.Mkdir(pgTestDirectory, 0750); err != nil {
+				return err
+			}
+		} else {
+			return err
+		}
+	}
+
+	file, err := os.Create(pgConfigFilePath)
+	if err != nil {
+		return err
+	}
+	defer func() { _ = file.Close() }()
+
+	return file.Sync()
+
+}
+
+func cleanup() {
+	os.RemoveAll(pgTestDirectory)
+}

internal/utils/shell.go

@@ -9,19 +9,24 @@ import (
 	"syscall"
 )
 
-func RunCommand(cmdStr, user string) ([]byte, error) {
-	pgUID, pgGID, err := SystemUserIDs(user)
+func RunCommand(cmdStr, usr string) ([]byte, error) {
+	uid, gid, err := SystemUserIDs(usr)
 	if err != nil {
 		return nil, err
 	}
 
 	cmd := exec.Command("sh", "-c", cmdStr)
 	cmd.SysProcAttr = &syscall.SysProcAttr{}
-	cmd.SysProcAttr.Credential = &syscall.Credential{Uid: uint32(pgUID), Gid: uint32(pgGID)}
+	cmd.SysProcAttr.Credential = &syscall.Credential{Uid: uint32(uid), Gid: uint32(gid)}
+
 	return cmd.Output()
 }
 
 func SetFileOwnership(pathToFile, owner string) error {
+	if os.Getenv("UNIT_TESTING") != "" {
+		return nil
+	}
+
 	uid, gid, err := SystemUserIDs(owner)
 	if err != nil {
 		return fmt.Errorf("failed to resolve system user ids: %s", err)