From b8cb669c2e3c69125cd76c3212bde701741c153a Mon Sep 17 00:00:00 2001 From: Simone Gotti Date: Thu, 24 Aug 2017 10:14:15 +0200 Subject: [PATCH] keeper: disable 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. ALTER SYSTEM writes to postgresql.auto.conf, so we make it a symlink to /dev/null. An ALTER SYSTEM command will return an error. --- doc/postgres_parameters.md | 8 ++- pkg/postgresql/postgresql.go | 22 ++++++++- tests/integration/config_test.go | 85 ++++++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+), 3 deletions(-) diff --git a/doc/postgres_parameters.md b/doc/postgres_parameters.md index 9c0b122a8..ddbd194fb 100644 --- a/doc/postgres_parameters.md +++ b/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. diff --git a/pkg/postgresql/postgresql.go b/pkg/postgresql/postgresql.go index b20926ec5..733a04ef5 100644 --- a/pkg/postgresql/postgresql.go +++ b/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") diff --git a/tests/integration/config_test.go b/tests/integration/config_test.go index 7995c6f2d..62fb0ccf7 100644 --- a/tests/integration/config_test.go +++ b/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) + } +}