Merge pull request #343 from sgotti/disable_alter_system

keeper: disable ALTER SYSTEM commands.

doc/postgres_parameters.md

@@ -35,4 +35,10 @@ When [initializing the cluster](initialization.md), by default, stolon will merg
 * When initMode is new, it'll merge the initdb generated parameters.
 * When initMode is existing it'll merge the parameters of the existing instance.
 
-To disable this behavior just set `mergePgParameters` to false in the cluster spec and manually set all the pgParameters in the cluster specification. 
+To disable this behavior just set `mergePgParameters` to false in the cluster spec and manually set all the pgParameters in the cluster specification.
+
+## postgresql.auto.conf and ALTER SYSTEM commands
+
+Since postgresql.auto.conf overrides postgresql.conf parameters, changing some of them with ALTER SYSTEM could break the cluster (parameters managed by stolon could be overridden) and make pg parameters different between the instances.
+
+To avoid this stolon disables the execution of ALTER SYSTEM commands making postgresql.auto.conf a symlink to /dev/null. When an ALTER SYSTEM command is executed it'll return an error.

pkg/postgresql/postgresql.go

@@ -37,8 +37,9 @@ import (
 )
 
 const (
-	postgresConf    = "postgresql.conf"
-	tmpPostgresConf = "stolon-temp-postgresql.conf"
+	postgresConf     = "postgresql.conf"
+	postgresAutoConf = "postgresql.auto.conf"
+	tmpPostgresConf  = "stolon-temp-postgresql.conf"
 
 	startTimeout = 60 * time.Second
 )
@@ -266,6 +267,10 @@ func (p *Manager) start(args ...string) error {
 	// the instance parent is the keeper instead of the defined system reaper
 	// (since pg_ctl forks and then exits leaving the postmaster orphaned).
 
+	if err := p.createPostgresqlAutoConf(); err != nil {
+		return err
+	}
+
 	log.Infow("starting database")
 	name := filepath.Join(p.pgBinPath, "postgres")
 	args = append([]string{"-D", p.dataDir, "-c", "unix_socket_directories=" + common.PgUnixSocketDirectories}, args...)
@@ -741,6 +746,19 @@ func (p *Manager) writePgHba() error {
 	return nil
 }
 
+// createPostgresqlAutoConf creates postgresql.auto.conf as a symlink to
+// /dev/null to block alter systems commands (they'll return an error)
+func (p *Manager) createPostgresqlAutoConf() error {
+	pgAutoConfPath := filepath.Join(p.dataDir, postgresAutoConf)
+	if err := os.Remove(pgAutoConfPath); err != nil && !os.IsNotExist(err) {
+		return fmt.Errorf("error removing postgresql.auto.conf file: %v", err)
+	}
+	if err := os.Symlink("/dev/null", pgAutoConfPath); err != nil {
+		return fmt.Errorf("error symlinking postgresql.auto.conf file to /dev/null: %v", err)
+	}
+	return nil
+}
+
 func (p *Manager) SyncFromFollowedPGRewind(followedConnParams ConnParams, password string) error {
 	// ioutil.Tempfile already creates files with 0600 permissions
 	pgpass, err := ioutil.TempFile("", "pgpass")

tests/integration/config_test.go

@@ -125,3 +125,88 @@ func TestServerParameters(t *testing.T) {
 		t.Fatalf("unexpected err: %v", err)
 	}
 }
+
+func TestAlterSystem(t *testing.T) {
+	t.Parallel()
+
+	dir, err := ioutil.TempDir("", "")
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	defer os.RemoveAll(dir)
+
+	tstore, err := NewTestStore(t, dir)
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	if err := tstore.Start(); err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	if err := tstore.WaitUp(10 * time.Second); err != nil {
+		t.Fatalf("error waiting on store up: %v", err)
+	}
+	storeEndpoints := fmt.Sprintf("%s:%s", tstore.listenAddress, tstore.port)
+	defer tstore.Stop()
+
+	clusterName := uuid.NewV4().String()
+
+	storePath := filepath.Join(common.StoreBasePath, clusterName)
+
+	sm := store.NewStore(tstore.store, storePath)
+
+	initialClusterSpec := &cluster.ClusterSpec{
+		InitMode:           cluster.ClusterInitModeP(cluster.ClusterInitModeNew),
+		SleepInterval:      &cluster.Duration{Duration: 2 * time.Second},
+		FailInterval:       &cluster.Duration{Duration: 5 * time.Second},
+		ConvergenceTimeout: &cluster.Duration{Duration: 30 * time.Second},
+	}
+	initialClusterSpecFile, err := writeClusterSpec(dir, initialClusterSpec)
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	ts, err := NewTestSentinel(t, dir, clusterName, tstore.storeBackend, storeEndpoints, fmt.Sprintf("--initial-cluster-spec=%s", initialClusterSpecFile))
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	if err := ts.Start(); err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	tk, err := NewTestKeeper(t, dir, clusterName, pgSUUsername, pgSUPassword, pgReplUsername, pgReplPassword, tstore.storeBackend, storeEndpoints)
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	if err := tk.Start(); err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+
+	if err := WaitClusterPhase(sm, cluster.ClusterPhaseNormal, 60*time.Second); err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	if err := tk.WaitDBUp(60 * time.Second); err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+
+	expectedErr := `pq: could not fsync file "postgresql.auto.conf": Invalid argument`
+	if _, err := tk.Exec("alter system set archive_mode to on"); err != nil {
+		if err.Error() != expectedErr {
+			t.Fatalf("expected err: %q, got: %q", expectedErr, err)
+		}
+	} else {
+		t.Fatalf("expected err: %q, got no error", expectedErr)
+	}
+
+	tk.Stop()
+	if err := tk.Start(); err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	if err := tk.WaitDBUp(60 * time.Second); err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	pgParameters, err := tk.GetPGParameters()
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	if v, ok := pgParameters["archive_mode"]; ok {
+		t.Fatalf("expected archive_mode not defined, got value: %q", v)
+	}
+}