From cfa143956671bf0b7a4822fde84fa23141f51777 Mon Sep 17 00:00:00 2001 From: Chris Bandy Date: Thu, 31 Jul 2025 13:39:06 -0500 Subject: [PATCH 1/4] Resolve symbolic links when finding CRDs in tests Without this, the relative path to the "external-snapshotter" module would be incorrect depending on the symlinks involved. --- internal/testing/require/kubernetes.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/internal/testing/require/kubernetes.go b/internal/testing/require/kubernetes.go index f2640a715..3953d7c38 100644 --- a/internal/testing/require/kubernetes.go +++ b/internal/testing/require/kubernetes.go @@ -139,14 +139,14 @@ func kubernetes3(t TestingT) (*envtest.Environment, client.Client) { // Calculate the project directory as reported by [goruntime.CallersFrames]. frame, ok := frames.Next() self := frame.File - root := strings.TrimSuffix(self, - filepath.Join("internal", "testing", "require", "kubernetes.go")) + root := Value(filepath.EvalSymlinks(strings.TrimSuffix(self, + filepath.Join("internal", "testing", "require", "kubernetes.go")))) // Find the first caller that is not in this file. for ok && frame.File == self { frame, ok = frames.Next() } - caller := frame.File + caller := Value(filepath.EvalSymlinks(frame.File)) // Calculate the project directory path relative to the caller. base := Value(filepath.Rel(filepath.Dir(caller), root)) @@ -159,8 +159,7 @@ func kubernetes3(t TestingT) (*envtest.Environment, client.Client) { ); assert.Check(t, err == nil && len(pkgs) > 0 && pkgs[0].Module != nil, "got %v\n%#v", err, pkgs, ) { - snapshotter, err = filepath.Rel(root, pkgs[0].Module.Dir) - assert.NilError(t, err) + snapshotter = Value(filepath.Rel(root, pkgs[0].Module.Dir)) } env := EnvTest(t, envtest.CRDInstallOptions{ From d233c6915f530f2e915f2e8764ae38a23d043765 Mon Sep 17 00:00:00 2001 From: Chris Bandy Date: Thu, 31 Jul 2025 16:01:39 -0500 Subject: [PATCH 2/4] Point the default KUTTL command to the correct package Follow-up to 777699608b14e455e6043d818a1ff22ab5cafc32. Did this ever work? --- Makefile | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 92ee2e618..2e553d101 100644 --- a/Makefile +++ b/Makefile @@ -13,7 +13,7 @@ CONTROLLER ?= $(GO) tool sigs.k8s.io/controller-tools/cmd/controller-gen # Run tests using the latest tools. ENVTEST ?= $(GO) run sigs.k8s.io/controller-runtime/tools/setup-envtest@latest -KUTTL ?= $(GO) run github.com/kudobuilder/kuttl/pkg/kuttlctl/cmd/kubectl-kuttl@latest +KUTTL ?= $(GO) run github.com/kudobuilder/kuttl/cmd/kubectl-kuttl@latest KUTTL_TEST ?= $(KUTTL) test ##@ General @@ -171,6 +171,10 @@ check-envtest-existing: createnamespaces kubectl delete -k ./config/dev # Expects operator to be running +# +# KUTTL runs with a single kubectl context named "cluster". +# If you experience `cluster "minikube" does not exist`, try `MINIKUBE_PROFILE=cluster`. +# .PHONY: check-kuttl check-kuttl: ## Run kuttl end-to-end tests check-kuttl: ## example command: make check-kuttl KUTTL_TEST=' From 9b74d39772673b3fe5b979290e661f4086a77604 Mon Sep 17 00:00:00 2001 From: Chris Bandy Date: Thu, 31 Jul 2025 13:45:46 -0500 Subject: [PATCH 3/4] Consistently use our scheme for serializing API objects --- internal/controller/runtime/pod_client.go | 4 ++-- internal/registration/runner_test.go | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/controller/runtime/pod_client.go b/internal/controller/runtime/pod_client.go index a20f92b18..c47673757 100644 --- a/internal/controller/runtime/pod_client.go +++ b/internal/controller/runtime/pod_client.go @@ -24,8 +24,8 @@ type podExecutor func( ) error func newPodClient(config *rest.Config) (rest.Interface, error) { - codecs := serializer.NewCodecFactory(scheme.Scheme) - gvk, _ := apiutil.GVKForObject(&corev1.Pod{}, scheme.Scheme) + codecs := serializer.NewCodecFactory(Scheme) + gvk, _ := apiutil.GVKForObject(&corev1.Pod{}, Scheme) httpClient, err := rest.HTTPClientFor(config) if err != nil { return nil, err diff --git a/internal/registration/runner_test.go b/internal/registration/runner_test.go index c70c07c6b..32bea6a48 100644 --- a/internal/registration/runner_test.go +++ b/internal/registration/runner_test.go @@ -20,9 +20,9 @@ import ( "gotest.tools/v3/assert" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/manager" + "github.com/crunchydata/postgres-operator/internal/controller/runtime" "github.com/crunchydata/postgres-operator/internal/testing/events" ) @@ -381,7 +381,7 @@ func TestRunnerRequiredEvents(t *testing.T) { conditions := append([]metav1.Condition{}, tt.before...) object := &corev1.ConfigMap{} - recorder := events.NewRecorder(t, scheme.Scheme) + recorder := events.NewRecorder(t, runtime.Scheme) result := r.Required(recorder, object, &conditions) @@ -413,7 +413,7 @@ func TestRunnerRequiredEvents(t *testing.T) { } { conditions := append([]metav1.Condition{}, tt.before...) object := &corev1.ConfigMap{} - recorder := events.NewRecorder(t, scheme.Scheme) + recorder := events.NewRecorder(t, runtime.Scheme) result := r.Required(recorder, object, &conditions) @@ -441,7 +441,7 @@ func TestRunnerRequiredEvents(t *testing.T) { } { conditions := append([]metav1.Condition{}, tt.before...) object := &corev1.ConfigMap{} - recorder := events.NewRecorder(t, scheme.Scheme) + recorder := events.NewRecorder(t, runtime.Scheme) result := r.Required(recorder, object, &conditions) @@ -475,7 +475,7 @@ func TestRunnerRequiredEvents(t *testing.T) { conditions := append([]metav1.Condition{}, tt.before...) object := &corev1.ConfigMap{} - recorder := events.NewRecorder(t, scheme.Scheme) + recorder := events.NewRecorder(t, runtime.Scheme) result := r.Required(recorder, object, &conditions) @@ -508,7 +508,7 @@ func TestRunnerRequiredEvents(t *testing.T) { } { conditions := append([]metav1.Condition{}, tt.before...) object := &corev1.ConfigMap{} - recorder := events.NewRecorder(t, scheme.Scheme) + recorder := events.NewRecorder(t, runtime.Scheme) result := r.Required(recorder, object, &conditions) From c777bbccc7963f655c32545a477c7eb8a7714266 Mon Sep 17 00:00:00 2001 From: Chris Bandy Date: Thu, 31 Jul 2025 12:26:10 -0500 Subject: [PATCH 4/4] 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 --- internal/controller/pgupgrade/jobs.go | 2 +- internal/controller/pgupgrade/jobs_test.go | 2 +- internal/patroni/config.go | 17 +----------- internal/patroni/config_test.go | 32 +++------------------- internal/pgbackrest/config.go | 2 +- internal/postgres/config.go | 13 +++++---- internal/postgres/reconcile_test.go | 4 +-- 7 files changed, 17 insertions(+), 55 deletions(-) diff --git a/internal/controller/pgupgrade/jobs.go b/internal/controller/pgupgrade/jobs.go index c7b6e4e01..4715c8da9 100644 --- a/internal/controller/pgupgrade/jobs.go +++ b/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`, diff --git a/internal/controller/pgupgrade/jobs_test.go b/internal/controller/pgupgrade/jobs_test.go index c3f3608e4..a94641d4c 100644 --- a/internal/controller/pgupgrade/jobs_test.go +++ b/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 diff --git a/internal/patroni/config.go b/internal/patroni/config.go index 61d3721ec..7815fc8c8 100644 --- a/internal/patroni/config.go +++ b/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. diff --git a/internal/patroni/config_test.go b/internal/patroni/config_test.go index f1d2a4c5d..d5aef835e 100644 --- a/internal/patroni/config_test.go +++ b/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) { diff --git a/internal/pgbackrest/config.go b/internal/pgbackrest/config.go index 3899c3333..f023aa77d 100644 --- a/internal/pgbackrest/config.go +++ b/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" diff --git a/internal/postgres/config.go b/internal/postgres/config.go index 942803975..174aee34b 100644 --- a/internal/postgres/config.go +++ b/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 ||`, diff --git a/internal/postgres/reconcile_test.go b/internal/postgres/reconcile_test.go index af656327d..61a85d5cd 100644 --- a/internal/postgres/reconcile_test.go +++ b/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' || :; }) ||