Skip to content

Commit 51235a4

Browse files
cbandycbandy
committed
Add a Bash function to create Postgres tablespace and data directories
The commands for these were intentionally similar, and now they are the same and documented together. Issue: PGO-2558
1 parent 64a2e07 commit 51235a4

File tree

2 files changed

+66
-86
lines changed

2 files changed

+66
-86
lines changed

‎internal/postgres/config.go‎

Lines changed: 58 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,38 @@ import (
2121
)
2222

2323
const (
24+
// bashDataDirectory is a Bash function that ensures a directory has the correct permissions for PostgreSQL data.
25+
//
26+
// Postgres requires its data directories be writable by only itself.
27+
// Pod "securityContext.fsGroup" sets g+w on directories for *some* storage providers.
28+
// Ensure the current user owns the directory, and remove group-write permission.
29+
//
30+
// - https://www.postgresql.org/docs/current/creating-cluster.html
31+
// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/backend/postmaster/postmaster.c;hb=REL_10_0#l1522
32+
// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/backend/utils/init/miscinit.c;hb=REL_11_0#l142
33+
// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/backend/utils/init/miscinit.c;hb=REL_17_0#l386
34+
// - https://issue.k8s.io/93802#issuecomment-717646167
35+
//
36+
// During CREATE TABLESPACE, Postgres sets the permissions of a tablespace directory to match the data directory.
37+
//
38+
// - https://www.postgresql.org/docs/current/manage-ag-tablespaces.html
39+
// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/backend/commands/tablespace.c;hb=REL_14_0#l600
40+
// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/common/file_perm.c;hb=REL_14_0#l27
41+
//
42+
bashDataDirectory = `dataDirectory() {` +
43+
// When the directory does not exist, create it with the correct permissions.
44+
// When the directory has the correct owner, set the correct permissions.
45+
` if [[ ! -e "$1" || -O "$1" ]]; then install --directory --mode=0750 "$1";` +
46+
//
47+
// The directory exists but its owner is wrong.
48+
// When it is writable, the set-group-ID bit indicates that "fsGroup" probably ran on its contents making them safe to use.
49+
// In this case, we can make a new directory (owned by this user) and refill it.
50+
` elif [[ -w "$1" && -g "$1" ]]; then recreate "$1" '0750';` +
51+
//
52+
// The directory exists, its owner is wrong, and it is not writable.
53+
// This is probably fatal, but indicate failure to let the caller decide.
54+
` else false; fi; }`
55+
2456
// bashHalt is a Bash function that prints its arguments to stderr then
2557
// exits with a non-zero status. It uses the exit status of the prior
2658
// command if that was not zero.
@@ -327,47 +359,31 @@ func startupCommand(
327359
version := fmt.Sprint(cluster.Spec.PostgresVersion)
328360
walDir := WALDirectory(cluster, instance)
329361

330-
// If the user requests tablespaces, we want to make sure the directories exist with the
331-
// correct owner and permissions.
332-
tablespaceCmd := ""
333-
if feature.Enabled(ctx, feature.TablespaceVolumes) {
334-
// This command checks if a dir exists and if not, creates it;
335-
// if the dir does exist, then we `recreate` it to make sure the owner is correct;
336-
// if the dir exists with the wrong owner and is not writeable, we error.
337-
// This is the same behavior we use for the main PGDATA directory.
338-
// Note: Postgres requires the tablespace directory to be "an existing, empty directory
339-
// that is owned by the PostgreSQL operating system user."
340-
// - https://www.postgresql.org/docs/current/manage-ag-tablespaces.html
341-
// However, unlike the PGDATA directory, Postgres will _not_ error out
342-
// if the permissions are wrong on the tablespace directory.
343-
// Instead, when a tablespace is created in Postgres, Postgres will `chmod` the
344-
// tablespace directory to match permissions on the PGDATA directory (either 700 or 750).
345-
// Postgres setting the tablespace directory permissions:
346-
// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/backend/commands/tablespace.c;hb=REL_14_0#l600
347-
// Postgres choosing directory permissions:
348-
// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/common/file_perm.c;hb=REL_14_0#l27
349-
// Note: This permission change seems to happen only when the tablespace is created in Postgres.
350-
// If the user manually `chmod`'ed the directory after the creation of the tablespace, Postgres
351-
// would not attempt to change the directory permissions.
352-
// Note: as noted below, we mount the tablespace directory to the mountpoint `/tablespaces/NAME`,
353-
// and so we add the subdirectory `data` in order to set the permissions.
354-
checkInstallRecreateCmd := strings.Join([]string{
355-
`if [[ ! -e "${tablespace_dir}" || -O "${tablespace_dir}" ]]; then`,
356-
`install --directory --mode=0750 "${tablespace_dir}"`,
357-
`elif [[ -w "${tablespace_dir}" && -g "${tablespace_dir}" ]]; then`,
358-
`recreate "${tablespace_dir}" '0750'`,
359-
`else (halt Permissions!); fi ||`,
360-
`halt "$(permissions "${tablespace_dir}" ||:)"`,
361-
}, "\n")
362+
mkdirs := make([]string, 0, 7+len(instance.TablespaceVolumes))
363+
mkdirs = append(mkdirs, `dataDirectory "${postgres_data_directory}" || halt "$(permissions "${postgres_data_directory}" ||:)"`)
362364

365+
// If the user requests tablespaces, we want to make sure the directories exist with the correct owner and permissions.
366+
//
367+
// The path for tablespaces volumes is /tablespaces/NAME/data -- the `data` directory is so we can arrange the permissions.
368+
if feature.Enabled(ctx, feature.TablespaceVolumes) {
363369
for _, tablespace := range instance.TablespaceVolumes {
364-
// The path for tablespaces volumes is /tablespaces/NAME/data
365-
// -- the `data` path is added so that we can arrange the permissions.
366-
tablespaceCmd = tablespaceCmd + "\ntablespace_dir=/tablespaces/" + tablespace.Name + "/data" + "\n" +
367-
checkInstallRecreateCmd
370+
dir := shell.QuoteWord("/tablespaces/" + tablespace.Name + "/data")
371+
mkdirs = append(mkdirs, `dataDirectory `+dir+` || halt "$(permissions `+dir+` ||:)"`)
368372
}
369373
}
370374

375+
// These directories are outside "data_directory" and can be created.
376+
mkdirs = append(mkdirs,
377+
`(`+shell.MakeDirectories(dataMountPath, LogDirectory())+`) ||`,
378+
`halt "$(permissions `+shell.QuoteWord(LogDirectory())+` ||:)"`,
379+
380+
`(`+shell.MakeDirectories(dataMountPath, naming.PatroniPGDataLogPath)+`) ||`,
381+
`halt "$(permissions `+shell.QuoteWord(naming.PatroniPGDataLogPath)+` ||:)"`,
382+
383+
`(`+shell.MakeDirectories(dataMountPath, naming.PGBackRestPGDataLogPath)+`) ||`,
384+
`halt "$(permissions `+shell.QuoteWord(naming.PGBackRestPGDataLogPath)+` ||:)"`,
385+
)
386+
371387
pg_rewind_override := ""
372388
if config.FetchKeyCommand(&cluster.Spec) != "" {
373389
// Quoting "EOF" disables parameter substitution during write.
@@ -384,6 +400,9 @@ chmod +x /tmp/pg_rewind_tde.sh
384400
script := strings.Join([]string{
385401
`declare -r expected_major_version="$1" pgwal_directory="$2"`,
386402

403+
// Function to create a Postgres data directory.
404+
bashDataDirectory,
405+
387406
// Function to print the permissions of a file or directory and its parents.
388407
bashPermissions,
389408

@@ -425,42 +444,10 @@ chmod +x /tmp/pg_rewind_tde.sh
425444

426445
// Determine if the data directory has been prepared for bootstrapping the cluster
427446
`bootstrap_dir="${postgres_data_directory}_bootstrap"`,
428-
`[[ -d "${bootstrap_dir}" ]] && results 'bootstrap directory' "${bootstrap_dir}"`,
429-
`[[ -d "${bootstrap_dir}" ]] && postgres_data_directory="${bootstrap_dir}"`,
430-
431-
// PostgreSQL requires its directory to be writable by only itself.
432-
// Pod "securityContext.fsGroup" sets g+w on directories for *some*
433-
// storage providers. Ensure the current user owns the directory, and
434-
// remove group-write permission.
435-
// - https://www.postgresql.org/docs/current/creating-cluster.html
436-
// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/backend/postmaster/postmaster.c;hb=REL_10_0#l1522
437-
// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/backend/utils/init/miscinit.c;hb=REL_11_0#l142
438-
// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/backend/utils/init/miscinit.c;hb=REL_17_0#l386
439-
// - https://issue.k8s.io/93802#issuecomment-717646167
440-
//
441-
// When the directory does not exist, create it with the correct permissions.
442-
// When the directory has the correct owner, set the correct permissions.
443-
`if [[ ! -e "${postgres_data_directory}" || -O "${postgres_data_directory}" ]]; then`,
444-
`install --directory --mode=0750 "${postgres_data_directory}"`,
445-
//
446-
// The directory exists but its owner is wrong. When it is writable,
447-
// the set-group-ID bit indicates that "fsGroup" probably ran on its
448-
// contents making them safe to use. In this case, we can make a new
449-
// directory (owned by this user) and refill it.
450-
`elif [[ -w "${postgres_data_directory}" && -g "${postgres_data_directory}" ]]; then`,
451-
`recreate "${postgres_data_directory}" '0750'`,
452-
//
453-
// The directory exists, its owner is wrong, and it is not writable.
454-
`else (halt Permissions!); fi ||`,
455-
`halt "$(permissions "${postgres_data_directory}" ||:)"`,
447+
`[[ -d "${bootstrap_dir}" ]] && postgres_data_directory="${bootstrap_dir}" && results 'bootstrap directory' "${bootstrap_dir}"`,
456448

457-
// Create log directories.
458-
`(` + shell.MakeDirectories(dataMountPath, naming.PGBackRestPGDataLogPath) + `) ||`,
459-
`halt "$(permissions ` + naming.PGBackRestPGDataLogPath + ` ||:)"`,
460-
`(` + shell.MakeDirectories(dataMountPath, naming.PatroniPGDataLogPath) + `) ||`,
461-
`halt "$(permissions ` + naming.PatroniPGDataLogPath + ` ||:)"`,
462-
`(` + shell.MakeDirectories(dataMountPath, LogDirectory()) + `) ||`,
463-
`halt "$(permissions ` + LogDirectory() + ` ||:)"`,
449+
// Create directories for and related to the data directory.
450+
strings.Join(mkdirs, "\n"),
464451

465452
// Copy replication client certificate files
466453
// from the /pgconf/tls/replication directory to the /tmp/replication directory in order
@@ -478,7 +465,6 @@ chmod +x /tmp/pg_rewind_tde.sh
478465
// Add the pg_rewind wrapper script, if TDE is enabled.
479466
pg_rewind_override,
480467

481-
tablespaceCmd,
482468
// When the data directory is empty, there's nothing more to do.
483469
`[[ -f "${postgres_data_directory}/PG_VERSION" ]] || exit 0`,
484470

‎internal/postgres/reconcile_test.go‎

Lines changed: 8 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -239,6 +239,7 @@ initContainers:
239239
- --
240240
- |-
241241
declare -r expected_major_version="$1" pgwal_directory="$2"
242+
dataDirectory() { if [[ ! -e "$1" || -O "$1" ]]; then install --directory --mode=0750 "$1"; elif [[ -w "$1" && -g "$1" ]]; then recreate "$1" '0750'; else false; fi; }
242243
permissions() { while [[ -n "$1" ]]; do set "${1%/*}" "$@"; done; shift; stat -Lc '%A %4u %4g %n' "$@"; }
243244
halt() { local rc=$?; >&2 echo "$@"; exit "${rc/#0/1}"; }
244245
results() { printf '::postgres-operator: %s::%s\n' "$@"; }
@@ -267,23 +268,16 @@ initContainers:
267268
[[ "${postgres_data_directory}" == "${PGDATA}" ]] ||
268269
halt Expected matching config and data directories
269270
bootstrap_dir="${postgres_data_directory}_bootstrap"
270-
[[ -d "${bootstrap_dir}" ]] && results 'bootstrap directory' "${bootstrap_dir}"
271-
[[ -d "${bootstrap_dir}" ]] && postgres_data_directory="${bootstrap_dir}"
272-
if [[ ! -e "${postgres_data_directory}" || -O "${postgres_data_directory}" ]]; then
273-
install --directory --mode=0750 "${postgres_data_directory}"
274-
elif [[ -w "${postgres_data_directory}" && -g "${postgres_data_directory}" ]]; then
275-
recreate "${postgres_data_directory}" '0750'
276-
else (halt Permissions!); fi ||
277-
halt "$(permissions "${postgres_data_directory}" ||:)"
278-
(mkdir -p '/pgdata/pgbackrest/log' && { chmod 0775 '/pgdata/pgbackrest/log' '/pgdata/pgbackrest' || :; }) ||
279-
halt "$(permissions /pgdata/pgbackrest/log ||:)"
280-
(mkdir -p '/pgdata/patroni/log' && { chmod 0775 '/pgdata/patroni/log' '/pgdata/patroni' || :; }) ||
281-
halt "$(permissions /pgdata/patroni/log ||:)"
271+
[[ -d "${bootstrap_dir}" ]] && postgres_data_directory="${bootstrap_dir}" && results 'bootstrap directory' "${bootstrap_dir}"
272+
dataDirectory "${postgres_data_directory}" || halt "$(permissions "${postgres_data_directory}" ||:)"
282273
(mkdir -p '/pgdata/logs/postgres' && { chmod 0775 '/pgdata/logs/postgres' '/pgdata/logs' || :; }) ||
283-
halt "$(permissions /pgdata/logs/postgres ||:)"
274+
halt "$(permissions '/pgdata/logs/postgres' ||:)"
275+
(mkdir -p '/pgdata/patroni/log' && { chmod 0775 '/pgdata/patroni/log' '/pgdata/patroni' || :; }) ||
276+
halt "$(permissions '/pgdata/patroni/log' ||:)"
277+
(mkdir -p '/pgdata/pgbackrest/log' && { chmod 0775 '/pgdata/pgbackrest/log' '/pgdata/pgbackrest' || :; }) ||
278+
halt "$(permissions '/pgdata/pgbackrest/log' ||:)"
284279
install -D --mode=0600 -t "/tmp/replication" "/pgconf/tls/replication"/{tls.crt,tls.key,ca.crt}
285280
286-
287281
[[ -f "${postgres_data_directory}/PG_VERSION" ]] || exit 0
288282
results 'data version' "${postgres_data_version:=$(< "${postgres_data_directory}/PG_VERSION")}"
289283
[[ "${postgres_data_version}" == "${expected_major_version}" ]] ||

0 commit comments

Comments
 (0)