Create directories with group-write permissions

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)