Ensure Postgres data directories have group-read permission

Postgres has allowed group-read on its data directories since v11.
This permission enables more avenues for data recovery when storage
misbehaves.

Issue: PGO-300

Author: cbandy

internal/controller/pgupgrade/jobs.go

@@ -98,7 +98,7 @@ func upgradeCommand(spec *v1beta1.PGUpgradeSettings, fetchKeyCommand string) []s
 		// proper permissions have to be set on the old pgdata directory and the
 		// preload library settings must be copied over.
 		`echo -e "\nStep 3: Setting the expected permissions on the old pgdata directory...\n"`,
-		`chmod 700 /pgdata/pg"${old_version}"`,
+		`chmod 750 /pgdata/pg"${old_version}"`,
 		`echo -e "Step 4: Copying shared_preload_libraries setting to new postgresql.conf file...\n"`,
 		`echo "shared_preload_libraries = '$(/usr/pgsql-"""${old_version}"""/bin/postgres -D \`,
 		`/pgdata/pg"""${old_version}""" -C shared_preload_libraries)'" >> /pgdata/pg"${new_version}"/postgresql.conf`,

internal/controller/pgupgrade/jobs_test.go

@@ -214,7 +214,7 @@ spec:
           echo -e "Step 2: Initializing new pgdata directory...\n"
           /usr/pgsql-"${new_version}"/bin/initdb -k -D /pgdata/pg"${new_version}"
           echo -e "\nStep 3: Setting the expected permissions on the old pgdata directory...\n"
-          chmod 700 /pgdata/pg"${old_version}"
+          chmod 750 /pgdata/pg"${old_version}"
           echo -e "Step 4: Copying shared_preload_libraries setting to new postgresql.conf file...\n"
           echo "shared_preload_libraries = '$(/usr/pgsql-"""${old_version}"""/bin/postgres -D \
           /pgdata/pg"""${old_version}""" -C shared_preload_libraries)'" >> /pgdata/pg"${new_version}"/postgresql.conf

internal/patroni/config.go

@@ -518,22 +518,7 @@ func instanceYAML(
 	if command := pgbackrestReplicaCreateCommand; len(command) > 0 {
 
 		// Regardless of the "keep_data" setting below, Patroni deletes the
-		// data directory when all methods fail. pgBackRest will not restore
-		// when the data directory is missing, so create it before running the
-		// command. PostgreSQL requires that the directory is writable by only
-		// itself.
-		// - https://github.com/zalando/patroni/blob/v2.0.2/patroni/ha.py#L249
-		// - https://github.com/pgbackrest/pgbackrest/issues/1445
-		// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/backend/utils/init/miscinit.c;hb=REL_13_0#l319
-		//
-		// NOTE(cbandy): The "PATRONI_POSTGRESQL_DATA_DIR" environment variable
-		// is defined in this package, but it is removed by Patroni at runtime.
-		command = append([]string{
-			"bash", "-ceu", "--",
-			`install --directory --mode=0700 "${PGDATA?}" && exec "$@"`,
-			"-",
-		}, command...)
-
+		// data directory when all methods fail.
 		postgresql[pgBackRestCreateReplicaMethod] = map[string]any{
 			"command":   strings.Join(shell.QuoteWords(command...), " "),
 			"keep_data": true, // Use the data directory from a prior method.

internal/patroni/config_test.go

@@ -720,8 +720,7 @@ postgresql:
   - pgbackrest
   - basebackup
   pgbackrest:
-    command: '''bash'' ''-ceu'' ''--'' ''install --directory --mode=0700 "${PGDATA?}"
-      && exec "$@"'' ''-'' ''some'' ''backrest'' ''cmd'''
+    command: '''some'' ''backrest'' ''cmd'''
     keep_data: true
     no_leader: true
     no_params: true
@@ -786,40 +785,17 @@ func TestPGBackRestCreateReplicaCommand(t *testing.T) {
 	}
 	assert.NilError(t, yaml.Unmarshal([]byte(data), &parsed))
 
-	dir := t.TempDir()
+	assert.Equal(t, parsed.PostgreSQL.PGBackRest.Command, `'some' 'backrest' 'cmd'`)
 
 	// The command should be compatible with any shell.
 	{
-		command := parsed.PostgreSQL.PGBackRest.Command
-		file := filepath.Join(dir, "command.sh")
-		assert.NilError(t, os.WriteFile(file, []byte(command), 0o600))
+		file := filepath.Join(t.TempDir(), "command.sh")
+		assert.NilError(t, os.WriteFile(file, []byte(parsed.PostgreSQL.PGBackRest.Command), 0o600))
 
 		cmd := exec.CommandContext(t.Context(), shellcheck, "--enable=all", "--shell=sh", file)
 		output, err := cmd.CombinedOutput()
 		assert.NilError(t, err, "%q\n%s", cmd.Args, output)
 	}
-
-	// Naive parsing of shell words...
-	command := strings.Split(strings.Trim(parsed.PostgreSQL.PGBackRest.Command, "'"), "' '")
-
-	// Expect a bash command with an inline script.
-	assert.DeepEqual(t, command[:3], []string{"bash", "-ceu", "--"})
-	assert.Assert(t, len(command) > 3)
-	script := command[3]
-
-	// It should call the pgBackRest command.
-	assert.Assert(t, strings.HasSuffix(script, ` exec "$@"`))
-	assert.DeepEqual(t, command[len(command)-3:], []string{"some", "backrest", "cmd"})
-
-	// It should pass shellcheck.
-	{
-		file := filepath.Join(dir, "script.bash")
-		assert.NilError(t, os.WriteFile(file, []byte(script), 0o600))
-
-		cmd := exec.CommandContext(t.Context(), shellcheck, "--enable=all", file)
-		output, err := cmd.CombinedOutput()
-		assert.NilError(t, err, "%q\n%s", cmd.Args, output)
-	}
 }
 
 func TestProbeTiming(t *testing.T) {

internal/pgbackrest/config.go

@@ -356,7 +356,7 @@ func DedicatedSnapshotVolumeRestoreCommand(pgdata string, args ...string) []stri
 BACKUP_LABEL=$([[ ! -e "${pgdata}/backup_label" ]] || md5sum "${pgdata}/backup_label")
 echo "Starting pgBackRest delta restore"
 
-install --directory --mode=0700 "${pgdata}"
+install --directory --mode=0750 "${pgdata}"
 rm -f "${pgdata}/postmaster.pid"
 bash -xc "pgbackrest restore ${opts}"
 rm -f "${pgdata}/patroni.dynamic.json"

internal/postgres/config.go

@@ -341,9 +341,9 @@ func startupCommand(
 		// and so we add the subdirectory `data` in order to set the permissions.
 		checkInstallRecreateCmd := strings.Join([]string{
 			`if [[ ! -e "${tablespace_dir}" || -O "${tablespace_dir}" ]]; then`,
-			`install --directory --mode=0700 "${tablespace_dir}"`,
+			`install --directory --mode=0750 "${tablespace_dir}"`,
 			`elif [[ -w "${tablespace_dir}" && -g "${tablespace_dir}" ]]; then`,
-			`recreate "${tablespace_dir}" '0700'`,
+			`recreate "${tablespace_dir}" '0750'`,
 			`else (halt Permissions!); fi ||`,
 			`halt "$(permissions "${tablespace_dir}" ||:)"`,
 		}, "\n")
@@ -419,23 +419,24 @@ chmod +x /tmp/pg_rewind_tde.sh
 		// PostgreSQL requires its directory to be writable by only itself.
 		// Pod "securityContext.fsGroup" sets g+w on directories for *some*
 		// storage providers. Ensure the current user owns the directory, and
-		// remove group permissions.
+		// remove group-write permission.
 		// - https://www.postgresql.org/docs/current/creating-cluster.html
 		// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/backend/postmaster/postmaster.c;hb=REL_10_0#l1522
-		// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/backend/utils/init/miscinit.c;hb=REL_14_0#l349
+		// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/backend/utils/init/miscinit.c;hb=REL_11_0#l142
+		// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/backend/utils/init/miscinit.c;hb=REL_17_0#l386
 		// - https://issue.k8s.io/93802#issuecomment-717646167
 		//
 		// When the directory does not exist, create it with the correct permissions.
 		// When the directory has the correct owner, set the correct permissions.
 		`if [[ ! -e "${postgres_data_directory}" || -O "${postgres_data_directory}" ]]; then`,
-		`install --directory --mode=0700 "${postgres_data_directory}"`,
+		`install --directory --mode=0750 "${postgres_data_directory}"`,
 		//
 		// The directory exists but its owner is wrong. When it is writable,
 		// the set-group-ID bit indicates that "fsGroup" probably ran on its
 		// contents making them safe to use. In this case, we can make a new
 		// directory (owned by this user) and refill it.
 		`elif [[ -w "${postgres_data_directory}" && -g "${postgres_data_directory}" ]]; then`,
-		`recreate "${postgres_data_directory}" '0700'`,
+		`recreate "${postgres_data_directory}" '0750'`,
 		//
 		// The directory exists, its owner is wrong, and it is not writable.
 		`else (halt Permissions!); fi ||`,

internal/postgres/reconcile_test.go

@@ -263,9 +263,9 @@ initContainers:
     [[ -d "${bootstrap_dir}" ]] && results 'bootstrap directory' "${bootstrap_dir}"
     [[ -d "${bootstrap_dir}" ]] && postgres_data_directory="${bootstrap_dir}"
     if [[ ! -e "${postgres_data_directory}" || -O "${postgres_data_directory}" ]]; then
-    install --directory --mode=0700 "${postgres_data_directory}"
+    install --directory --mode=0750 "${postgres_data_directory}"
     elif [[ -w "${postgres_data_directory}" && -g "${postgres_data_directory}" ]]; then
-    recreate "${postgres_data_directory}" '0700'
+    recreate "${postgres_data_directory}" '0750'
     else (halt Permissions!); fi ||
     halt "$(permissions "${postgres_data_directory}" ||:)"
     (mkdir -p '/pgdata/pgbackrest/log' && { chmod 0775 '/pgdata/pgbackrest/log' '/pgdata/pgbackrest' || :; }) ||