Add fsGroupChangePolicy to pod (#3296)

* Add fsGroupChangePolicy to pod

Issue [sc-14235]

* Bump test behavior around fsGroupChangePolicy

* bump test k8s 1.19=>1.20 for github k3d test action
* specify 1.19 for kubernetes-api test action, alter tests to
check for k8s version and pass with 1.19 (no fsGroupChangePolicy in check)
and >=1.20 (fsGroupChangePolicy in check)

.github/workflows/test.yaml

@@ -25,7 +25,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        kubernetes: [default]
+        kubernetes: ["1.19.2"] # TODO(benjaminjb)(issue sc-11672): bump to 1.20.2 or higher after we update controller-runtime
     steps:
       - uses: actions/checkout@v3
       - uses: actions/setup-go@v3
@@ -51,7 +51,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        kubernetes: [latest, v1.19]
+        kubernetes: [latest, v1.20]
     steps:
       - uses: actions/checkout@v3
       - uses: actions/setup-go@v3

internal/controller/postgrescluster/pgadmin_test.go

@@ -21,6 +21,7 @@ package postgrescluster
 import (
 	"context"
 	"io"
+	"os"
 	"testing"
 
 	"github.com/pkg/errors"
@@ -503,7 +504,11 @@ labels:
   postgres-operator.crunchydata.com/data: pgadmin
   postgres-operator.crunchydata.com/role: pgadmin
 		`))
-		assert.Assert(t, cmp.MarshalMatches(template.Spec, `
+
+		// TODO(benjaminjb)(issue sc-11672): after we update controller-runtime and
+		// are no longer testing in Github actions with K8s 1.19.2, reduce the following comparison
+		// to simply testing against a given, fixed string.
+		compare := `
 automountServiceAccountToken: false
 containers: null
 dnsPolicy: ClusterFirst
@@ -512,9 +517,25 @@ restartPolicy: Always
 schedulerName: default-scheduler
 securityContext:
   fsGroup: 26
+  fsGroupChangePolicy: OnRootMismatch
   runAsNonRoot: true
 terminationGracePeriodSeconds: 30
-		`))
+		`
+		if os.Getenv("ENVTEST_K8S_VERSION") == "1.19.2" {
+			compare = `
+automountServiceAccountToken: false
+containers: null
+dnsPolicy: ClusterFirst
+enableServiceLinks: false
+restartPolicy: Always
+schedulerName: default-scheduler
+securityContext:
+  fsGroup: 26
+  runAsNonRoot: true
+terminationGracePeriodSeconds: 30
+		`
+		}
+		assert.Assert(t, cmp.MarshalMatches(template.Spec, compare))
 	})
 
 	t.Run("verify customized deployment", func(t *testing.T) {
@@ -614,7 +635,11 @@ labels:
   postgres-operator.crunchydata.com/data: pgadmin
   postgres-operator.crunchydata.com/role: pgadmin
 		`))
-		assert.Assert(t, cmp.MarshalMatches(template.Spec, `
+
+		// TODO(benjaminjb)(issue sc-11672): after we update controller-runtime and
+		// are no longer testing in Github actions with K8s 1.19.2, reduce the following comparison
+		// to simply testing against a given, fixed string.
+		compare := `
 affinity:
   nodeAffinity:
     requiredDuringSchedulingIgnoredDuringExecution:
@@ -632,6 +657,7 @@ restartPolicy: Always
 schedulerName: default-scheduler
 securityContext:
   fsGroup: 26
+  fsGroupChangePolicy: OnRootMismatch
   runAsNonRoot: true
 terminationGracePeriodSeconds: 30
 tolerations:
@@ -648,7 +674,45 @@ topologySpreadConstraints:
   maxSkew: 1
   topologyKey: fakekey
   whenUnsatisfiable: ScheduleAnyway
-		`))
+`
+		if os.Getenv("ENVTEST_K8S_VERSION") == "1.19.2" {
+			compare = `
+affinity:
+  nodeAffinity:
+    requiredDuringSchedulingIgnoredDuringExecution:
+      nodeSelectorTerms:
+      - matchExpressions:
+        - key: key
+          operator: Exists
+automountServiceAccountToken: false
+containers: null
+dnsPolicy: ClusterFirst
+enableServiceLinks: false
+imagePullSecrets:
+- name: myImagePullSecret
+restartPolicy: Always
+schedulerName: default-scheduler
+securityContext:
+  fsGroup: 26
+  runAsNonRoot: true
+terminationGracePeriodSeconds: 30
+tolerations:
+- key: sometoleration
+topologySpreadConstraints:
+- labelSelector:
+    matchExpressions:
+    - key: postgres-operator.crunchydata.com/cluster
+      operator: In
+      values:
+      - somename
+    - key: postgres-operator.crunchydata.com/data
+      operator: Exists
+  maxSkew: 1
+  topologyKey: fakekey
+  whenUnsatisfiable: ScheduleAnyway
+		`
+		}
+		assert.Assert(t, cmp.MarshalMatches(template.Spec, compare))
 	})
 }
 

internal/controller/postgrescluster/pgbackrest_test.go

@@ -2518,6 +2518,7 @@ containers:
 enableServiceLinks: false
 restartPolicy: Never
 securityContext:
+  fsGroupChangePolicy: OnRootMismatch
   runAsNonRoot: true
 volumes:
 - name: pgbackrest-config

internal/controller/postgrescluster/pgbouncer_test.go

@@ -469,6 +469,7 @@ containers: null
 enableServiceLinks: false
 restartPolicy: Always
 securityContext:
+  fsGroupChangePolicy: OnRootMismatch
   runAsNonRoot: true
 shareProcessNamespace: true
 topologySpreadConstraints:

internal/controller/postgrescluster/volumes_test.go

@@ -21,6 +21,7 @@ package postgrescluster
 import (
 	"context"
 	"errors"
+	"os"
 	"testing"
 	"time"
 
@@ -755,7 +756,61 @@ func TestReconcileMoveDirectories(t *testing.T) {
 
 		for i := range moveJobs.Items {
 			if moveJobs.Items[i].Name == "testcluster-move-pgdata-dir" {
-				assert.Assert(t, marshalMatches(moveJobs.Items[i].Spec.Template.Spec, `
+				// TODO(benjaminjb)(issue sc-11672): after we update controller-runtime and
+				// are no longer testing in Github actions with K8s 1.19.2, reduce the following comparison
+				// to simply testing against a given, fixed string.
+
+				compare := `
+automountServiceAccountToken: false
+containers:
+- command:
+  - bash
+  - -ceu
+  - "echo \"Preparing cluster testcluster volumes for PGO v5.x\"\n    echo \"pgdata_pvc=testpgdata\"\n
+    \   echo \"Current PG data directory volume contents:\" \n    ls -lh \"/pgdata\"\n
+    \   echo \"Now updating PG data directory...\"\n    [ -d \"/pgdata/testpgdatadir\"
+    ] && mv \"/pgdata/testpgdatadir\" \"/pgdata/pg13_bootstrap\"\n    rm -f \"/pgdata/pg13/patroni.dynamic.json\"\n
+    \   echo \"Updated PG data directory contents:\" \n    ls -lh \"/pgdata\"\n    echo
+    \"PG Data directory preparation complete\"\n    "
+  image: example.com/crunchy-postgres-ha:test
+  imagePullPolicy: Always
+  name: pgdata-move-job
+  resources:
+    requests:
+      cpu: 1m
+  securityContext:
+    allowPrivilegeEscalation: false
+    capabilities:
+      drop:
+      - ALL
+    privileged: false
+    readOnlyRootFilesystem: true
+    runAsNonRoot: true
+  terminationMessagePath: /dev/termination-log
+  terminationMessagePolicy: File
+  volumeMounts:
+  - mountPath: /pgdata
+    name: postgres-data
+dnsPolicy: ClusterFirst
+enableServiceLinks: false
+imagePullSecrets:
+- name: test-secret
+priorityClassName: some-priority-class
+restartPolicy: Never
+schedulerName: default-scheduler
+securityContext:
+  fsGroup: 26
+  fsGroupChangePolicy: OnRootMismatch
+  runAsNonRoot: true
+terminationGracePeriodSeconds: 30
+volumes:
+- name: postgres-data
+  persistentVolumeClaim:
+    claimName: testpgdata
+	`
+
+				if os.Getenv("ENVTEST_K8S_VERSION") == "1.19.2" {
+					compare = `
 automountServiceAccountToken: false
 containers:
 - command:
@@ -801,7 +856,10 @@ volumes:
 - name: postgres-data
   persistentVolumeClaim:
     claimName: testpgdata
-	`+"\n"))
+	`
+				}
+
+				assert.Assert(t, marshalMatches(moveJobs.Items[i].Spec.Template.Spec, compare+"\n"))
 			}
 		}
 
@@ -811,7 +869,60 @@ volumes:
 
 		for i := range moveJobs.Items {
 			if moveJobs.Items[i].Name == "testcluster-move-pgwal-dir" {
-				assert.Assert(t, marshalMatches(moveJobs.Items[i].Spec.Template.Spec, `
+				// TODO(benjaminjb)(issue sc-11672): after we update controller-runtime and
+				// are no longer testing in Github actions with K8s 1.19.2, reduce the following comparison
+				// to simply testing against a given, fixed string.
+
+				compare := `
+automountServiceAccountToken: false
+containers:
+- command:
+  - bash
+  - -ceu
+  - "echo \"Preparing cluster testcluster volumes for PGO v5.x\"\n    echo \"pg_wal_pvc=testwal\"\n
+    \   echo \"Current PG WAL directory volume contents:\"\n    ls -lh \"/pgwal\"\n
+    \   echo \"Now updating PG WAL directory...\"\n    [ -d \"/pgwal/testwaldir\"
+    ] && mv \"/pgwal/testwaldir\" \"/pgwal/testcluster-wal\"\n    echo \"Updated PG
+    WAL directory contents:\"\n    ls -lh \"/pgwal\"\n    echo \"PG WAL directory
+    preparation complete\"\n    "
+  image: example.com/crunchy-postgres-ha:test
+  imagePullPolicy: Always
+  name: pgwal-move-job
+  resources:
+    requests:
+      cpu: 1m
+  securityContext:
+    allowPrivilegeEscalation: false
+    capabilities:
+      drop:
+      - ALL
+    privileged: false
+    readOnlyRootFilesystem: true
+    runAsNonRoot: true
+  terminationMessagePath: /dev/termination-log
+  terminationMessagePolicy: File
+  volumeMounts:
+  - mountPath: /pgwal
+    name: postgres-wal
+dnsPolicy: ClusterFirst
+enableServiceLinks: false
+imagePullSecrets:
+- name: test-secret
+priorityClassName: some-priority-class
+restartPolicy: Never
+schedulerName: default-scheduler
+securityContext:
+  fsGroup: 26
+  fsGroupChangePolicy: OnRootMismatch
+  runAsNonRoot: true
+terminationGracePeriodSeconds: 30
+volumes:
+- name: postgres-wal
+  persistentVolumeClaim:
+    claimName: testwal
+	`
+				if os.Getenv("ENVTEST_K8S_VERSION") == "1.19.2" {
+					compare = `
 automountServiceAccountToken: false
 containers:
 - command:
@@ -857,7 +968,9 @@ volumes:
 - name: postgres-wal
   persistentVolumeClaim:
     claimName: testwal
-	`+"\n"))
+	`
+				}
+				assert.Assert(t, marshalMatches(moveJobs.Items[i].Spec.Template.Spec, compare+"\n"))
 			}
 		}
 
@@ -867,7 +980,64 @@ volumes:
 
 		for i := range moveJobs.Items {
 			if moveJobs.Items[i].Name == "testcluster-move-pgbackrest-repo-dir" {
-				assert.Assert(t, marshalMatches(moveJobs.Items[i].Spec.Template.Spec, `
+
+				// TODO(benjaminjb)(issue sc-11672): after we update controller-runtime and
+				// are no longer testing in Github actions with K8s 1.19.2, reduce the following comparison
+				// to simply testing against a given, fixed string.
+
+				compare := `
+automountServiceAccountToken: false
+containers:
+- command:
+  - bash
+  - -ceu
+  - "echo \"Preparing cluster testcluster pgBackRest repo volume for PGO v5.x\"\n
+    \   echo \"repo_pvc=testrepo\"\n    echo \"pgbackrest directory:\"\n    ls -lh
+    /pgbackrest\n    echo \"Current pgBackRest repo directory volume contents:\" \n
+    \   ls -lh \"/pgbackrest/testrepodir\"\n    echo \"Now updating repo directory...\"\n
+    \   [ -d \"/pgbackrest/testrepodir\" ] && mv -t \"/pgbackrest/\" \"/pgbackrest/testrepodir/archive\"\n
+    \   [ -d \"/pgbackrest/testrepodir\" ] && mv -t \"/pgbackrest/\" \"/pgbackrest/testrepodir/backup\"\n
+    \   echo \"Updated /pgbackrest directory contents:\"\n    ls -lh \"/pgbackrest\"\n
+    \   echo \"Repo directory preparation complete\"\n    "
+  image: example.com/crunchy-pgbackrest:test
+  imagePullPolicy: Always
+  name: repo-move-job
+  resources:
+    requests:
+      cpu: 1m
+  securityContext:
+    allowPrivilegeEscalation: false
+    capabilities:
+      drop:
+      - ALL
+    privileged: false
+    readOnlyRootFilesystem: true
+    runAsNonRoot: true
+  terminationMessagePath: /dev/termination-log
+  terminationMessagePolicy: File
+  volumeMounts:
+  - mountPath: /pgbackrest
+    name: pgbackrest-repo
+dnsPolicy: ClusterFirst
+enableServiceLinks: false
+imagePullSecrets:
+- name: test-secret
+priorityClassName: some-priority-class
+restartPolicy: Never
+schedulerName: default-scheduler
+securityContext:
+  fsGroup: 26
+  fsGroupChangePolicy: OnRootMismatch
+  runAsNonRoot: true
+terminationGracePeriodSeconds: 30
+volumes:
+- name: pgbackrest-repo
+  persistentVolumeClaim:
+    claimName: testrepo
+	`
+
+				if os.Getenv("ENVTEST_K8S_VERSION") == "1.19.2" {
+					compare = `
 automountServiceAccountToken: false
 containers:
 - command:
@@ -915,7 +1085,9 @@ volumes:
 - name: pgbackrest-repo
   persistentVolumeClaim:
     claimName: testrepo
-	`+"\n"))
+	`
+				}
+				assert.Assert(t, marshalMatches(moveJobs.Items[i].Spec.Template.Spec, compare+"\n"))
 			}
 		}
 

internal/initialize/security.go

@@ -22,9 +22,14 @@ import (
 // RestrictedPodSecurityContext returns a v1.PodSecurityContext with safe defaults.
 // See https://docs.k8s.io/concepts/security/pod-security-standards/
 func RestrictedPodSecurityContext() *corev1.PodSecurityContext {
+	onRootMismatch := corev1.FSGroupChangeOnRootMismatch
 	return &corev1.PodSecurityContext{
 		// Fail to start a container if its image runs as UID 0 (root).
 		RunAsNonRoot: Bool(true),
+
+		// If set to "OnRootMismatch", if the root of the volume already has
+		// the correct permissions, the recursive permission change can be skipped
+		FSGroupChangePolicy: &onRootMismatch,
 	}
 }