Create directories with group-write permissions

The group-write permission is important for persistent file systems in
environments where different containers are assigned different UIDs
over time. Some network file systems, however, reject attempts to set
POSIX directory permissions. CIFS and NFS are notable in this regard.

Issue: PGO-2417

Author: cbandy

internal/collector/instance.go

@@ -180,8 +180,7 @@ func startCommand(logDirectories []string, includeLogrotate bool) []string {
 	if len(logDirectories) != 0 {
 		for _, logDir := range logDirectories {
 			mkdirScript = mkdirScript + `
-` + shell.MakeDirectories(0o775, logDir,
-				path.Join(logDir, "receiver"))
+` + shell.MakeDirectories(logDir, path.Join(logDir, "receiver"))
 		}
 	}
 

internal/controller/standalone_pgadmin/pod.go

@@ -442,10 +442,10 @@ with open('` + configMountPath + `/` + gunicornConfigFilePath + `') as _f:
 	script := strings.Join([]string{
 		// Create the config directory so Kubernetes can mount it later.
 		// - https://issue.k8s.io/121294
-		shell.MakeDirectories(0o775, scriptMountPath, configMountPath),
+		shell.MakeDirectories(scriptMountPath, configMountPath),
 
-		// Create the logs directory with g+rwx to ensure pgAdmin can write to it as well.
-		shell.MakeDirectories(0o775, dataMountPath, LogDirectoryAbsolutePath),
+		// Create the logs directory and ensure pgAdmin can write to it as well.
+		shell.MakeDirectories(dataMountPath, LogDirectoryAbsolutePath),
 
 		// Write the system and server configurations.
 		`echo "$1" > ` + scriptMountPath + `/config_system.py`,

internal/controller/standalone_pgadmin/pod_test.go

@@ -137,8 +137,8 @@ initContainers:
   - -ceu
   - --
   - |-
-    mkdir -p '/etc/pgadmin/conf.d' && chmod 0775 '/etc/pgadmin/conf.d'
-    mkdir -p '/var/lib/pgadmin/logs' && chmod 0775 '/var/lib/pgadmin/logs'
+    mkdir -p '/etc/pgadmin/conf.d' && { chmod 0775 '/etc/pgadmin/conf.d' || :; }
+    mkdir -p '/var/lib/pgadmin/logs' && { chmod 0775 '/var/lib/pgadmin/logs' || :; }
     echo "$1" > /etc/pgadmin/config_system.py
     echo "$2" > /etc/pgadmin/gunicorn_config.py
   - startup
@@ -342,8 +342,8 @@ initContainers:
   - -ceu
   - --
   - |-
-    mkdir -p '/etc/pgadmin/conf.d' && chmod 0775 '/etc/pgadmin/conf.d'
-    mkdir -p '/var/lib/pgadmin/logs' && chmod 0775 '/var/lib/pgadmin/logs'
+    mkdir -p '/etc/pgadmin/conf.d' && { chmod 0775 '/etc/pgadmin/conf.d' || :; }
+    mkdir -p '/var/lib/pgadmin/logs' && { chmod 0775 '/var/lib/pgadmin/logs' || :; }
     echo "$1" > /etc/pgadmin/config_system.py
     echo "$2" > /etc/pgadmin/gunicorn_config.py
   - startup

internal/pgbackrest/config.go

@@ -177,7 +177,7 @@ func MakePGBackrestLogDir(template *corev1.PodTemplateSpec,
 	container := corev1.Container{
 		// TODO(log-rotation): The second argument here should be the path
 		// of the volume mount. Find a way to calculate that consistently.
-		Command:         []string{"bash", "-c", shell.MakeDirectories(0o775, path.Dir(pgBackRestLogPath), pgBackRestLogPath)},
+		Command:         []string{"bash", "-c", shell.MakeDirectories(path.Dir(pgBackRestLogPath), pgBackRestLogPath)},
 		Image:           config.PGBackRestContainerImage(cluster),
 		ImagePullPolicy: cluster.Spec.ImagePullPolicy,
 		Name:            naming.ContainerPGBackRestLogDirInit,

internal/pgbackrest/config_test.go

@@ -292,7 +292,7 @@ func TestMakePGBackrestLogDir(t *testing.T) {
 	for _, c := range podTemplate.Spec.InitContainers {
 		if c.Name == naming.ContainerPGBackRestLogDirInit {
 			// ignore "bash -c", should skip repo with no volume
-			assert.Equal(t, `mkdir -p '/pgbackrest/repo2/log' && chmod 0775 '/pgbackrest/repo2/log'`, c.Command[2])
+			assert.Equal(t, `mkdir -p '/pgbackrest/repo2/log' && { chmod 0775 '/pgbackrest/repo2/log' || :; }`, c.Command[2])
 			assert.Equal(t, c.Image, "test-image")
 			assert.Equal(t, c.ImagePullPolicy, corev1.PullAlways)
 			assert.Assert(t, !cmp.DeepEqual(c.SecurityContext,

internal/postgres/config.go

@@ -375,11 +375,11 @@ chmod +x /tmp/pg_rewind_tde.sh
 		`halt "$(permissions "${postgres_data_directory}" ||:)"`,
 
 		// Create log directories.
-		`(` + shell.MakeDirectories(0o775, dataMountPath, naming.PGBackRestPGDataLogPath) + `) ||`,
+		`(` + shell.MakeDirectories(dataMountPath, naming.PGBackRestPGDataLogPath) + `) ||`,
 		`halt "$(permissions ` + naming.PGBackRestPGDataLogPath + ` ||:)"`,
-		`(` + shell.MakeDirectories(0o775, dataMountPath, naming.PatroniPGDataLogPath) + `) ||`,
+		`(` + shell.MakeDirectories(dataMountPath, naming.PatroniPGDataLogPath) + `) ||`,
 		`halt "$(permissions ` + naming.PatroniPGDataLogPath + ` ||:)"`,
-		`(` + shell.MakeDirectories(0o775, dataMountPath, LogDirectory()) + `) ||`,
+		`(` + shell.MakeDirectories(dataMountPath, LogDirectory()) + `) ||`,
 		`halt "$(permissions ` + LogDirectory() + ` ||:)"`,
 
 		// Copy replication client certificate files

internal/postgres/reconcile_test.go

@@ -268,11 +268,11 @@ initContainers:
     recreate "${postgres_data_directory}" '0700'
     else (halt Permissions!); fi ||
     halt "$(permissions "${postgres_data_directory}" ||:)"
-    (mkdir -p '/pgdata/pgbackrest/log' && chmod 0775 '/pgdata/pgbackrest/log' '/pgdata/pgbackrest') ||
+    (mkdir -p '/pgdata/pgbackrest/log' && { chmod 0775 '/pgdata/pgbackrest/log' '/pgdata/pgbackrest' || :; }) ||
     halt "$(permissions /pgdata/pgbackrest/log ||:)"
-    (mkdir -p '/pgdata/patroni/log' && chmod 0775 '/pgdata/patroni/log' '/pgdata/patroni') ||
+    (mkdir -p '/pgdata/patroni/log' && { chmod 0775 '/pgdata/patroni/log' '/pgdata/patroni' || :; }) ||
     halt "$(permissions /pgdata/patroni/log ||:)"
-    (mkdir -p '/pgdata/logs/postgres' && chmod 0775 '/pgdata/logs/postgres' '/pgdata/logs') ||
+    (mkdir -p '/pgdata/logs/postgres' && { chmod 0775 '/pgdata/logs/postgres' '/pgdata/logs' || :; }) ||
     halt "$(permissions /pgdata/logs/postgres ||:)"
     install -D --mode=0600 -t "/tmp/replication" "/pgconf/tls/replication"/{tls.crt,tls.key,ca.crt}
 

internal/shell/paths.go

@@ -33,14 +33,14 @@ func CleanFileName(path string) string {
 
 // MakeDirectories returns a list of POSIX shell commands that ensure each path
 // exists. It creates every directory leading to path from (but not including)
-// base and sets their permissions to exactly perms, regardless of umask.
+// base and sets their permissions for Kubernetes, regardless of umask.
 //
 // See:
 //   - https://pubs.opengroup.org/onlinepubs/9799919799/utilities/chmod.html
 //   - https://pubs.opengroup.org/onlinepubs/9799919799/utilities/mkdir.html
 //   - https://pubs.opengroup.org/onlinepubs/9799919799/utilities/test.html
 //   - https://pubs.opengroup.org/onlinepubs/9799919799/utilities/umask.html
-func MakeDirectories(perms fs.FileMode, base string, paths ...string) string {
+func MakeDirectories(base string, paths ...string) string {
 	// Without any paths, return a command that succeeds when the base path
 	// exists.
 	if len(paths) == 0 {
@@ -61,14 +61,22 @@ func MakeDirectories(perms fs.FileMode, base string, paths ...string) string {
 		}
 	}
 
+	const perms fs.FileMode = 0 |
+		// S_IRWXU: enable owner read, write, and execute permissions.
+		0o0700 |
+		// S_IRWXG: enable group read, write, and execute permissions.
+		0o0070 |
+		// S_IXOTH, S_IROTH: enable other read and execute permissions.
+		0o0001 | 0o0004
+
 	return `` +
 		// Create all the paths and any missing parents.
 		`mkdir -p ` + strings.Join(QuoteWords(paths...), " ") +
 
-		// Set the permissions of every path and each parent.
-		// NOTE: FileMode bits other than file permissions are ignored.
-		fmt.Sprintf(` && chmod %#o %s`,
-			perms&fs.ModePerm,
-			strings.Join(QuoteWords(allPaths...), " "),
+		// Try to set the permissions of every path and each parent.
+		// This swallows the exit status of `chmod` because not all filesystems
+		// tolerate the operation; CIFS and NFS are notable examples.
+		fmt.Sprintf(` && { chmod %#o %s || :; }`,
+			perms, strings.Join(QuoteWords(allPaths...), " "),
 		)
 }

internal/shell/paths_test.go

@@ -52,20 +52,20 @@ func TestMakeDirectories(t *testing.T) {
 
 	t.Run("NoPaths", func(t *testing.T) {
 		assert.Equal(t,
-			MakeDirectories(0o755, "/asdf/jklm"),
+			MakeDirectories("/asdf/jklm"),
 			`test -d '/asdf/jklm'`)
 	})
 
 	t.Run("Children", func(t *testing.T) {
 		assert.DeepEqual(t,
-			MakeDirectories(0o775, "/asdf", "/asdf/jklm", "/asdf/qwerty"),
-			`mkdir -p '/asdf/jklm' '/asdf/qwerty' && chmod 0775 '/asdf/jklm' '/asdf/qwerty'`)
+			MakeDirectories("/asdf", "/asdf/jklm", "/asdf/qwerty"),
+			`mkdir -p '/asdf/jklm' '/asdf/qwerty' && { chmod 0775 '/asdf/jklm' '/asdf/qwerty' || :; }`)
 	})
 
 	t.Run("Grandchild", func(t *testing.T) {
-		script := MakeDirectories(0o775, "/asdf", "/asdf/qwerty/boots")
+		script := MakeDirectories("/asdf", "/asdf/qwerty/boots")
 		assert.DeepEqual(t, script,
-			`mkdir -p '/asdf/qwerty/boots' && chmod 0775 '/asdf/qwerty/boots' '/asdf/qwerty'`)
+			`mkdir -p '/asdf/qwerty/boots' && { chmod 0775 '/asdf/qwerty/boots' '/asdf/qwerty' || :; }`)
 
 		t.Run("ShellCheckPOSIX", func(t *testing.T) {
 			shellcheck := require.ShellCheck(t)
@@ -83,7 +83,7 @@ func TestMakeDirectories(t *testing.T) {
 	})
 
 	t.Run("Long", func(t *testing.T) {
-		script := MakeDirectories(0o700, "/", strings.Repeat("/asdf", 20))
+		script := MakeDirectories("/", strings.Repeat("/asdf", 20))
 
 		t.Run("PrettyYAML", func(t *testing.T) {
 			b, err := yaml.Marshal(script)