diff --git a/internal/controller/postgrescluster/pgbackrest.go b/internal/controller/postgrescluster/pgbackrest.go index e4e1a06d3..52065093a 100644 --- a/internal/controller/postgrescluster/pgbackrest.go +++ b/internal/controller/postgrescluster/pgbackrest.go @@ -11,6 +11,7 @@ import ( "path/filepath" "reflect" "regexp" + "slices" "sort" "strings" "time" @@ -1190,35 +1191,36 @@ func (r *Reconciler) reconcileRestoreJob(ctx context.Context, pgdataVolume, pgwalVolume *corev1.PersistentVolumeClaim, pgtablespaceVolumes []*corev1.PersistentVolumeClaim, dataSource *v1beta1.PostgresClusterDataSource, - instanceName, instanceSetName, configHash, stanzaName string) error { - + instanceName, instanceSetName, configHash, stanzaName string, +) error { + hasFlag := make(map[string]bool) + matchFlag := regexp.MustCompile(`--[^ =]+`) repoName := dataSource.RepoName - options := dataSource.Options + + for _, input := range dataSource.Options { + for _, match := range matchFlag.FindAllString(input, -1) { + hasFlag[match] = true + } + } // ensure options are properly set // TODO (andrewlecuyer): move validation logic to a webhook - for _, opt := range options { + { var msg string switch { - // Since '--repo' can be set with or without an equals ('=') sign, we check for both - // usage patterns. - case strings.Contains(opt, "--repo=") || strings.Contains(opt, "--repo "): + case hasFlag["--repo"]: msg = "Option '--repo' is not allowed: please use the 'repoName' field instead." - case strings.Contains(opt, "--stanza"): - msg = "Option '--stanza' is not allowed: the operator will automatically set this " + - "option" - case strings.Contains(opt, "--pg1-path"): - msg = "Option '--pg1-path' is not allowed: the operator will automatically set this " + - "option" - case strings.Contains(opt, "--target-action"): - msg = "Option '--target-action' is not allowed: the operator will automatically set this " + - "option " - case strings.Contains(opt, "--link-map"): - msg = "Option '--link-map' is not allowed: the operator will automatically set this " + - "option " + case hasFlag["--stanza"]: + msg = "Option '--stanza' is not allowed: the operator will automatically set this option" + case hasFlag["--pg1-path"]: + msg = "Option '--pg1-path' is not allowed: the operator will automatically set this option" + case hasFlag["--target-action"]: + msg = "Option '--target-action' is not allowed: the operator will automatically set this option" + case hasFlag["--link-map"]: + msg = "Option '--link-map' is not allowed: the operator will automatically set this option" } if msg != "" { - r.Recorder.Eventf(cluster, corev1.EventTypeWarning, "InvalidDataSource", msg, repoName) + r.Recorder.Event(cluster, corev1.EventTypeWarning, "InvalidDataSource", msg) return nil } } @@ -1226,27 +1228,12 @@ func (r *Reconciler) reconcileRestoreJob(ctx context.Context, pgdata := postgres.DataDirectory(cluster) // combine options provided by user in the spec with those populated by the operator for a // successful restore - opts := append(options, []string{ - "--stanza=" + stanzaName, - "--pg1-path=" + pgdata, - "--repo=" + regexRepoIndex.FindString(repoName)}...) - - // Look specifically for the "--target" flag, NOT flags that contain - // "--target" (e.g. "--target-timeline") - targetRegex, err := regexp.Compile("--target[ =]") - if err != nil { - return err - } - var deltaOptFound, foundTarget bool - for _, opt := range opts { - switch { - case targetRegex.MatchString(opt): - foundTarget = true - case strings.Contains(opt, "--delta"): - deltaOptFound = true - } - } - if !deltaOptFound { + opts := append(slices.Clone(dataSource.Options), shell.QuoteWords( + "--stanza="+stanzaName, + "--pg1-path="+pgdata, + "--repo="+regexRepoIndex.FindString(repoName), + )...) + if !hasFlag["--delta"] { opts = append(opts, "--delta") } @@ -1262,28 +1249,26 @@ func (r *Reconciler) reconcileRestoreJob(ctx context.Context, // - https://github.com/pgbackrest/pgbackrest/blob/bb03b3f41942d0b781931092a76877ad309001ef/src/command/restore/restore.c#L1623 // - https://github.com/pgbackrest/pgbackrest/issues/1314 // - https://github.com/pgbackrest/pgbackrest/issues/987 - if foundTarget { + if hasFlag["--target"] { opts = append(opts, "--target-action=promote") } for i, instanceSpec := range cluster.Spec.InstanceSets { if instanceSpec.Name == instanceSetName { - opts = append(opts, "--link-map=pg_wal="+postgres.WALDirectory(cluster, - &cluster.Spec.InstanceSets[i])) + opts = append(opts, "--link-map=pg_wal="+ + postgres.WALDirectory(cluster, &cluster.Spec.InstanceSets[i])) } } - // Check to see if huge pages have been requested in the spec. If they have, include 'huge_pages = try' - // in the restore command. If they haven't, include 'huge_pages = off'. - hugePagesSetting := "off" - if postgres.HugePagesRequested(cluster) { - hugePagesSetting = "try" + params := postgres.NewParameterSet() + postgres.SetHugePages(cluster, params) + if fetchKeyCommand := config.FetchKeyCommand(&cluster.Spec); fetchKeyCommand != "" { + params.Add("encryption_key_command", fetchKeyCommand) } // NOTE (andrewlecuyer): Forcing users to put each argument separately might prevent the need // to do any escaping or use eval. - cmd := pgbackrest.RestoreCommand(pgdata, hugePagesSetting, config.FetchKeyCommand(&cluster.Spec), - pgtablespaceVolumes, strings.Join(opts, " ")) + cmd := pgbackrest.RestoreCommand(cluster.Spec.PostgresVersion, pgdata, params, strings.Join(opts, " ")) // create the volume resources required for the postgres data directory dataVolumeMount := postgres.DataVolumeMount() diff --git a/internal/controller/postgrescluster/pgbackrest_test.go b/internal/controller/postgrescluster/pgbackrest_test.go index b0f4d0eb8..a976ff9ff 100644 --- a/internal/controller/postgrescluster/pgbackrest_test.go +++ b/internal/controller/postgrescluster/pgbackrest_test.go @@ -34,6 +34,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/reconcile" + "sigs.k8s.io/yaml" "github.com/crunchydata/postgres-operator/internal/controller/runtime" "github.com/crunchydata/postgres-operator/internal/initialize" @@ -2355,7 +2356,8 @@ func TestReconcileCloudBasedDataSource(t *testing.T) { LabelSelector: naming.PGBackRestRestoreJobSelector(clusterName), Namespace: cluster.Namespace, })) - assert.Assert(t, tc.result.jobCount == len(restoreJobs.Items)) + assert.Equal(t, tc.result.jobCount, len(restoreJobs.Items), + "got:\n%s", require.Value(yaml.Marshal(restoreJobs.Items))) if len(restoreJobs.Items) == 1 { assert.Assert(t, restoreJobs.Items[0].Labels[naming.LabelStartupInstance] != "") assert.Assert(t, restoreJobs.Items[0].Annotations[naming.PGBackRestConfigHash] != "") diff --git a/internal/controller/postgrescluster/postgres.go b/internal/controller/postgrescluster/postgres.go index beaec3cdf..33907043f 100644 --- a/internal/controller/postgrescluster/postgres.go +++ b/internal/controller/postgrescluster/postgres.go @@ -134,7 +134,7 @@ func (r *Reconciler) generatePostgresParameters( pgaudit.PostgreSQLParameters(&builtin) pgbackrest.PostgreSQLParameters(cluster, &builtin, backupsSpecFound) pgmonitor.PostgreSQLParameters(ctx, cluster, &builtin) - postgres.SetHugePages(cluster, &builtin) + postgres.SetHugePages(cluster, builtin.Default) // Last write wins, so start with the recommended defaults. result := cmp.Or(builtin.Default.DeepCopy(), postgres.NewParameterSet()) diff --git a/internal/pgbackrest/config.go b/internal/pgbackrest/config.go index 808354007..f411491fc 100644 --- a/internal/pgbackrest/config.go +++ b/internal/pgbackrest/config.go @@ -212,10 +212,9 @@ func MakePGBackrestLogDir(template *corev1.PodTemplateSpec, // - Renames the data directory as needed to bootstrap the cluster using the restored database. // This ensures compatibility with the "existing" bootstrap method that is included in the // Patroni config when bootstrapping a cluster using an existing data directory. -func RestoreCommand(pgdata, hugePagesSetting, fetchKeyCommand string, _ []*corev1.PersistentVolumeClaim, args ...string) []string { - ps := postgres.NewParameterSet() +func RestoreCommand(postgresVersion int32, pgdata string, params *postgres.ParameterSet, args ...string) []string { + ps := params.DeepCopy() ps.Add("data_directory", pgdata) - ps.Add("huge_pages", hugePagesSetting) // Keep history and WAL files until the cluster starts with its normal // archiving enabled. @@ -226,10 +225,6 @@ func RestoreCommand(pgdata, hugePagesSetting, fetchKeyCommand string, _ []*corev // progress during recovery. ps.Add("hot_standby", "on") - if fetchKeyCommand != "" { - ps.Add("encryption_key_command", fetchKeyCommand) - } - configure := strings.Join([]string{ // With "hot_standby" on, some parameters cannot be smaller than they were // when Postgres was backed up. Configure these to match values reported by @@ -271,6 +266,7 @@ func RestoreCommand(pgdata, hugePagesSetting, fetchKeyCommand string, _ []*corev script := strings.Join([]string{ `declare -r PGDATA="$1" opts="$2"; export PGDATA PGHOST`, + postgres.ShellPath(postgresVersion), // Remove any "postmaster.pid" file leftover from a prior failure. `rm -f "${PGDATA}/postmaster.pid"`, diff --git a/internal/pgbackrest/config_test.go b/internal/pgbackrest/config_test.go index 91ce833c0..e6ca0b2a7 100644 --- a/internal/pgbackrest/config_test.go +++ b/internal/pgbackrest/config_test.go @@ -18,6 +18,7 @@ import ( "github.com/crunchydata/postgres-operator/internal/initialize" "github.com/crunchydata/postgres-operator/internal/naming" + "github.com/crunchydata/postgres-operator/internal/postgres" "github.com/crunchydata/postgres-operator/internal/testing/cmp" "github.com/crunchydata/postgres-operator/internal/testing/require" "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1" @@ -789,15 +790,14 @@ func TestReloadCommandPrettyYAML(t *testing.T) { func TestRestoreCommand(t *testing.T) { shellcheck := require.ShellCheck(t) - pgdata := "/pgdata/pg13" - opts := []string{ - "--stanza=" + DefaultStanzaName, "--pg1-path=" + pgdata, - "--repo=1"} - command := RestoreCommand(pgdata, "try", "", nil, strings.Join(opts, " ")) + command := RestoreCommand(19, "/pgdata/pg13", postgres.NewParameterSet(), "--repo=1") assert.DeepEqual(t, command[:3], []string{"bash", "-ceu", "--"}) assert.Assert(t, len(command) > 3) + assert.Assert(t, cmp.Contains(command[3], "/usr/pgsql-19/bin"), + "expected path to PostgreSQL binaries") + dir := t.TempDir() file := filepath.Join(dir, "script.bash") assert.NilError(t, os.WriteFile(file, []byte(command[3]), 0o600)) @@ -810,17 +810,20 @@ func TestRestoreCommand(t *testing.T) { func TestRestoreCommandPrettyYAML(t *testing.T) { assert.Assert(t, cmp.MarshalContains( - RestoreCommand("/dir", "try", "", nil, "--options"), + RestoreCommand(9, "/dir", postgres.NewParameterSet(), "--options"), "\n- |", ), "expected literal block scalar") } func TestRestoreCommandTDE(t *testing.T) { + params := postgres.NewParameterSet() + params.Add("encryption_key_command", "whatever") + assert.Assert(t, cmp.MarshalContains( - RestoreCommand("/dir", "try", "echo testValue", nil, "--options"), - "encryption_key_command = 'echo testValue'", + RestoreCommand(20, "/dir", params, "--options"), + "encryption_key_command = 'whatever'", ), "expected encryption_key_command setting") } diff --git a/internal/postgres/huge_pages.go b/internal/postgres/huge_pages.go index b38120baf..9dd408ba3 100644 --- a/internal/postgres/huge_pages.go +++ b/internal/postgres/huge_pages.go @@ -16,11 +16,11 @@ import ( // This function looks for a valid huge_pages resource request. If it finds one, // it sets the PostgreSQL parameter "huge_pages" to "try". If it doesn't find // one, it sets "huge_pages" to "off". -func SetHugePages(cluster *v1beta1.PostgresCluster, pgParameters *Parameters) { +func SetHugePages(cluster *v1beta1.PostgresCluster, params *ParameterSet) { if HugePagesRequested(cluster) { - pgParameters.Default.Add("huge_pages", "try") + params.Add("huge_pages", "try") } else { - pgParameters.Default.Add("huge_pages", "off") + params.Add("huge_pages", "off") } } diff --git a/internal/postgres/huge_pages_test.go b/internal/postgres/huge_pages_test.go index 9b9f12172..69528d568 100644 --- a/internal/postgres/huge_pages_test.go +++ b/internal/postgres/huge_pages_test.go @@ -27,11 +27,11 @@ func TestSetHugePages(t *testing.T) { }, }} - pgParameters := NewParameters() - SetHugePages(cluster, &pgParameters) + params := NewParameterSet() + SetHugePages(cluster, params) - assert.Equal(t, pgParameters.Default.Has("huge_pages"), true) - assert.Equal(t, pgParameters.Default.Value("huge_pages"), "off") + assert.Equal(t, params.Has("huge_pages"), true) + assert.Equal(t, params.Value("huge_pages"), "off") }) t.Run("hugepages quantity not set", func(t *testing.T) { @@ -48,11 +48,11 @@ func TestSetHugePages(t *testing.T) { }, }} - pgParameters := NewParameters() - SetHugePages(cluster, &pgParameters) + params := NewParameterSet() + SetHugePages(cluster, params) - assert.Equal(t, pgParameters.Default.Has("huge_pages"), true) - assert.Equal(t, pgParameters.Default.Value("huge_pages"), "off") + assert.Equal(t, params.Has("huge_pages"), true) + assert.Equal(t, params.Value("huge_pages"), "off") }) t.Run("hugepages set to zero", func(t *testing.T) { @@ -68,11 +68,11 @@ func TestSetHugePages(t *testing.T) { }, }} - pgParameters := NewParameters() - SetHugePages(cluster, &pgParameters) + params := NewParameterSet() + SetHugePages(cluster, params) - assert.Equal(t, pgParameters.Default.Has("huge_pages"), true) - assert.Equal(t, pgParameters.Default.Value("huge_pages"), "off") + assert.Equal(t, params.Has("huge_pages"), true) + assert.Equal(t, params.Value("huge_pages"), "off") }) t.Run("hugepages set correctly", func(t *testing.T) { @@ -88,11 +88,11 @@ func TestSetHugePages(t *testing.T) { }, }} - pgParameters := NewParameters() - SetHugePages(cluster, &pgParameters) + params := NewParameterSet() + SetHugePages(cluster, params) - assert.Equal(t, pgParameters.Default.Has("huge_pages"), true) - assert.Equal(t, pgParameters.Default.Value("huge_pages"), "try") + assert.Equal(t, params.Has("huge_pages"), true) + assert.Equal(t, params.Value("huge_pages"), "try") }) }