diff --git a/internal/controller/postgrescluster/controller.go b/internal/controller/postgrescluster/controller.go index 36fad289df..7b5ed67422 100644 --- a/internal/controller/postgrescluster/controller.go +++ b/internal/controller/postgrescluster/controller.go @@ -459,6 +459,11 @@ func (r *Reconciler) Reconcile( if err == nil { err = r.reconcilePostgresDatabases(ctx, cluster, instances, patchClusterStatus) } + // K8SPG-911: the two reconcilers around this one need a writable instance. + // A standby has none, so its pg_tde status comes from what it reports. + if err == nil { + r.reconcilePGTDEStandby(ctx, cluster, instances) + } if err == nil { err = r.reconcilePGTDEProviders(ctx, cluster, instances, patchClusterStatus) } diff --git a/internal/controller/postgrescluster/instance.go b/internal/controller/postgrescluster/instance.go index ea67429f6d..ff6b874f77 100644 --- a/internal/controller/postgrescluster/instance.go +++ b/internal/controller/postgrescluster/instance.go @@ -293,6 +293,30 @@ func (observed *observedInstances) writablePod(container string) (*corev1.Pod, * return nil, nil } +// standbyLeaderPod finds the instance Patroni reports as the standby leader: +// the one replaying from a source outside this cluster. Unlike writablePod it +// deliberately accepts an instance in recovery, so callers must send it only +// statements that read. +func (observed *observedInstances) standbyLeaderPod(container string) (*corev1.Pod, *Instance) { + if observed == nil { + return nil, nil + } + + for _, instance := range observed.forCluster { + if terminating, known := instance.IsTerminating(); terminating || !known { + continue + } + if len(instance.Pods) != 1 || !patroni.PodIsStandbyLeader(instance.Pods[0]) { + continue + } + if running, known := instance.IsRunning(container); running && known { + return instance.Pods[0], instance + } + } + + return nil, nil +} + // runningPods returns the Pod of every non-terminating instance whose named // container is running, and whether that accounts for every instance in the // cluster. Callers that must reach the whole cluster, rather than any one diff --git a/internal/controller/postgrescluster/instance_test.go b/internal/controller/postgrescluster/instance_test.go index f47b8be7c9..38f4886996 100644 --- a/internal/controller/postgrescluster/instance_test.go +++ b/internal/controller/postgrescluster/instance_test.go @@ -523,6 +523,88 @@ func TestWritablePod(t *testing.T) { }) } +// K8SPG-911 +func TestStandbyLeaderPod(t *testing.T) { + container := "container" + + instance := func(role string, terminating, running bool) *Instance { + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "namespace", + Name: "pod", + }, + Status: corev1.PodStatus{ + ContainerStatuses: []corev1.ContainerStatus{{Name: container}}, + }, + } + if role != "" { + pod.Annotations = map[string]string{"status": `{"role":"` + role + `"}`} + } + if terminating { + pod.DeletionTimestamp = &metav1.Time{} + } + if running { + pod.Status.ContainerStatuses[0].State.Running = new(corev1.ContainerStateRunning) + } else { + pod.Status.ContainerStatuses[0].State.Waiting = new(corev1.ContainerStateWaiting) + } + + return &Instance{Name: "instance", Pods: []*corev1.Pod{pod}, Runner: &appsv1.StatefulSet{}} + } + + t.Run("empty observed", func(t *testing.T) { + pod, instance := (&observedInstances{}).standbyLeaderPod(container) + assert.Assert(t, pod == nil) + assert.Assert(t, instance == nil) + }) + + t.Run("nil observed", func(t *testing.T) { + var observed *observedInstances + pod, instance := observed.standbyLeaderPod(container) + assert.Assert(t, pod == nil) + assert.Assert(t, instance == nil) + }) + + for _, tc := range []struct { + name string + role string + terminating bool + running bool + expected bool + }{ + {name: "StandbyLeader", role: "standby_leader", running: true, expected: true}, + {name: "Terminating", role: "standby_leader", terminating: true, running: true}, + {name: "NotRunning", role: "standby_leader"}, + {name: "Replica", role: "replica", running: true}, + {name: "NoStatusAnnotation", running: true}, + { + // The role label Patroni puts on a standby leader is the same one it + // puts on a real primary, which is why standbyLeaderPod reads the + // member status instead. A writable instance belongs to + // writablePod, and the two must never both match. + name: "Primary", role: "primary", running: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + inst := instance(tc.role, tc.terminating, tc.running) + observed := &observedInstances{forCluster: []*Instance{inst}} + + pod, matched := observed.standbyLeaderPod(container) + if !tc.expected { + assert.Assert(t, pod == nil) + assert.Assert(t, matched == nil) + return + } + + assert.Assert(t, pod != nil) + assert.Equal(t, matched, inst) + + writable, _ := observed.writablePod(container) + assert.Assert(t, writable == nil, "a standby leader is not writable") + }) + } +} + func TestAddPGBackRestToInstancePodSpec(t *testing.T) { t.Parallel() diff --git a/internal/controller/postgrescluster/pgtde_test.go b/internal/controller/postgrescluster/pgtde_test.go index 94d6f4b08b..312ece802a 100644 --- a/internal/controller/postgrescluster/pgtde_test.go +++ b/internal/controller/postgrescluster/pgtde_test.go @@ -133,6 +133,48 @@ func tdeInstance(annotations map[string]string) *Instance { } } +// tdeStandbyInstance builds an observed instance whose Patroni member status is +// the one a standby cluster's leader publishes: running and labeled primary, but +// in recovery and therefore not writable. K8SPG-911 +func tdeStandbyInstance(annotations map[string]string) *Instance { + instance := tdeInstance(annotations) + instance.Pods[0].Annotations["status"] = `{"role":"standby_leader"}` + return instance +} + +// execResponder returns a PodExec function that appends every call to calls and +// lets respond write the psql output the caller will parse. +func execResponder(calls *[]execCall, respond func(call execCall, stdout io.Writer) error) func( + ctx context.Context, namespace, pod, container string, + stdin io.Reader, stdout, stderr io.Writer, command ...string, +) error { + return func( + ctx context.Context, namespace, pod, container string, + stdin io.Reader, stdout, stderr io.Writer, command ...string, + ) error { + call := execCall{ + namespace: namespace, + pod: pod, + container: container, + command: command, + } + if stdin != nil { + b, err := io.ReadAll(stdin) + if err != nil { + return err + } + call.stdin = string(b) + } + + *calls = append(*calls, call) + + if respond != nil { + return respond(call, stdout) + } + return nil + } +} + func TestPGTDEVaultRevision(t *testing.T) { t.Parallel() @@ -912,6 +954,58 @@ func TestReconcilePGTDEProviders(t *testing.T) { assert.Equal(t, len(calls), 0) }) + // K8SPG-911: reconcilePGTDEStandby reports the key provider of a cluster in + // recovery, and this function must not undo what it said. + t.Run("Standby", func(t *testing.T) { + var calls []execCall + cluster := newCluster() + cluster.Spec.Standby = &v1beta1.PostgresStandbySpec{Enabled: true, RepoName: "repo1"} + pgtde.ReportStandby(cluster, true, nil) + + r := &Reconciler{ + Recorder: events.NewRecorder(t, runtime.Scheme), + PodExec: execRecorder(&calls, nil), + } + observed := &observedInstances{forCluster: []*Instance{ + tdeStandbyInstance(map[string]string{naming.TDEInstalledAnnotation: "true"}), + }} + + assert.NilError(t, r.reconcilePGTDEProviders(ctx, cluster, observed, failPatch(t))) + assert.Equal(t, len(psqlCalls(calls)), 0, + "a cluster in recovery rejects every statement this function runs") + assert.Equal(t, cluster.Status.PGTDERevision, "", + "a revision would make promotion skip repointing the inherited provider") + assertTDEProviderCondition(t, cluster, metav1.ConditionTrue, "ReplicatedFromSource") + }) + + // A standby whose spec turns pg_tde off still needs its revision cleared and + // its provider condition removed, which is what ReportStandby steps aside for. + t.Run("StandbyDisabled", func(t *testing.T) { + var calls []execCall + cluster := newCluster() + cluster.Spec.Standby = &v1beta1.PostgresStandbySpec{Enabled: true, RepoName: "repo1"} + cluster.Spec.Extensions.PGTDE.Enabled = false + cluster.Status.PGTDERevision = standardRevision + meta.SetStatusCondition(&cluster.Status.Conditions, metav1.Condition{ + Type: v1beta1.PGTDEVaultProviderReady, + Status: metav1.ConditionTrue, + Reason: "ReplicatedFromSource", + }) + + r := &Reconciler{ + Recorder: events.NewRecorder(t, runtime.Scheme), + PodExec: execRecorder(&calls, nil), + } + observed := &observedInstances{forCluster: []*Instance{ + tdeStandbyInstance(map[string]string{naming.TDEInstalledAnnotation: "true"}), + }} + + assert.NilError(t, r.reconcilePGTDEProviders(ctx, cluster, observed, failPatch(t))) + assert.Equal(t, cluster.Status.PGTDERevision, "") + assert.Assert(t, meta.FindStatusCondition(cluster.Status.Conditions, + v1beta1.PGTDEVaultProviderReady) == nil) + }) + t.Run("WaitsForExtension", func(t *testing.T) { var calls []execCall cluster := newCluster() @@ -1114,7 +1208,6 @@ func TestReconcilePGTDEProviders(t *testing.T) { "the condition should name the Secret that could not be read") assert.Equal(t, patched, 1, "the failure condition is useless unless it is written to the API") - assertEvent(t, r.Recorder, "PGTDEVaultProviderChangeFailed") }) t.Run("PhaseTwo", func(t *testing.T) { @@ -1380,6 +1473,20 @@ func TestReconcilePostgresDatabasesPGTDEReporting(t *testing.T) { "no SQL ran, so there is nothing to report") }) + // K8SPG-911: a standby leader is labeled primary but stays in recovery, so + // none of the SQL here can run. reconcilePGTDEStandby owns that case. + t.Run("NothingIsReportedOnAStandbyLeader", func(t *testing.T) { + cluster := newCluster(true) + cluster.Spec.Standby = &v1beta1.PostgresStandbySpec{Enabled: true, RepoName: "repo1"} + + r := &Reconciler{Recorder: events.NewRecorder(t, runtime.Scheme)} + + assert.NilError(t, r.reconcilePostgresDatabases(ctx, cluster, + &observedInstances{forCluster: []*Instance{tdeStandbyInstance(nil)}}, failPatch(t))) + assert.Assert(t, pgTDECondition(cluster) == nil) + assert.Equal(t, cluster.Status.DatabaseRevision, "") + }) + t.Run("DisableIsReportedOnlyAfterItRuns", func(t *testing.T) { cluster := newCluster(false) @@ -1409,6 +1516,243 @@ func TestReconcilePostgresDatabasesPGTDEReporting(t *testing.T) { }) } +// K8SPG-911 +func TestReconcilePGTDEStandby(t *testing.T) { + t.Parallel() + ctx := context.Background() + + newCluster := func() *v1beta1.PostgresCluster { + cluster := &v1beta1.PostgresCluster{ + ObjectMeta: metav1.ObjectMeta{Namespace: "ns1", Name: "pgc1", UID: "the-uid"}, + } + cluster.Spec.Standby = &v1beta1.PostgresStandbySpec{Enabled: true, RepoName: "repo1"} + cluster.Spec.Extensions.PGTDE = v1beta1.PGTDESpec{ + Enabled: true, + Vault: tdeVaultSpec(), + } + return cluster + } + + // answer replies to the extension query with installed, and to the key + // verification with keyErr. + answer := func(installed bool, keyErr error) func(execCall, io.Writer) error { + return func(call execCall, stdout io.Writer) error { + switch { + case strings.Contains(call.stdin, "pg_extension"): + if installed { + _, _ = io.WriteString(stdout, "t\n") + } else { + _, _ = io.WriteString(stdout, "f\n") + } + return nil + case strings.Contains(call.stdin, "pg_tde_verify_default_key"): + return keyErr + } + t.Errorf("unexpected statement %q", call.stdin) + return nil + } + } + + extensionCondition := func(cluster *v1beta1.PostgresCluster) *metav1.Condition { + return meta.FindStatusCondition(cluster.Status.Conditions, v1beta1.PGTDEEnabled) + } + + standbyObserved := func() *observedInstances { + return &observedInstances{forCluster: []*Instance{tdeStandbyInstance(nil)}} + } + + t.Run("NotStandby", func(t *testing.T) { + var calls []execCall + cluster := newCluster() + cluster.Spec.Standby = nil + + r := &Reconciler{ + PodExec: execResponder(&calls, answer(true, nil)), + } + + r.reconcilePGTDEStandby(ctx, cluster, standbyObserved()) + assert.Equal(t, len(calls), 0) + assert.Equal(t, len(cluster.Status.Conditions), 0) + }) + + t.Run("NoStandbyLeader", func(t *testing.T) { + var calls []execCall + cluster := newCluster() + + // Patroni has not elected a standby leader yet, or has promoted the one + // it had, in which case the writable path takes over. + instance := tdeInstance(nil) + + r := &Reconciler{ + PodExec: execResponder(&calls, answer(true, nil)), + } + + r.reconcilePGTDEStandby(ctx, cluster, &observedInstances{forCluster: []*Instance{instance}}) + assert.Equal(t, len(calls), 0) + assert.Equal(t, len(cluster.Status.Conditions), 0) + }) + + t.Run("NoExecWhenPGTDEWasNeverHere", func(t *testing.T) { + var calls []execCall + cluster := newCluster() + cluster.Spec.Extensions.PGTDE = v1beta1.PGTDESpec{} + + r := &Reconciler{ + Recorder: events.NewRecorder(t, runtime.Scheme), + PodExec: execResponder(&calls, answer(false, nil)), + } + + r.reconcilePGTDEStandby(ctx, cluster, standbyObserved()) + assert.Equal(t, len(calls), 0, + "a plain standby must not be exec'd into on every reconcile") + }) + + t.Run("ObservesExtensionAndKey", func(t *testing.T) { + var calls []execCall + cluster := newCluster() + + r := &Reconciler{ + Recorder: events.NewRecorder(t, runtime.Scheme), + PodExec: execResponder(&calls, answer(true, nil)), + } + + r.reconcilePGTDEStandby(ctx, cluster, standbyObserved()) + + assert.Equal(t, len(calls), 2) + for _, call := range calls { + assert.Equal(t, call.namespace, "ns1") + assert.Equal(t, call.pod, "pgc1-instance1-abcd-0") + assert.Equal(t, call.container, naming.ContainerDatabase) + assert.Equal(t, call.command[0], "psql") + assert.Assert(t, argsContain(call.command, "--tuples-only")) + } + assert.Assert(t, strings.Contains(calls[0].stdin, "pg_extension")) + assert.Assert(t, strings.Contains(calls[1].stdin, "pg_tde_verify_default_key")) + + condition := extensionCondition(cluster) + assert.Assert(t, condition != nil) + assert.Equal(t, condition.Status, metav1.ConditionTrue) + assert.Equal(t, condition.Reason, "ReplicatedFromSource") + assertTDEProviderCondition(t, cluster, metav1.ConditionTrue, "ReplicatedFromSource") + + assert.Equal(t, cluster.Status.PGTDERevision, "") + }) + + t.Run("ExtensionMissing", func(t *testing.T) { + var calls []execCall + cluster := newCluster() + + r := &Reconciler{ + Recorder: events.NewRecorder(t, runtime.Scheme), + PodExec: execResponder(&calls, answer(false, nil)), + } + + r.reconcilePGTDEStandby(ctx, cluster, standbyObserved()) + + assert.Equal(t, len(calls), 1, "there is no key to verify without the extension") + + condition := extensionCondition(cluster) + assert.Assert(t, condition != nil) + assert.Equal(t, condition.Status, metav1.ConditionFalse) + assert.Equal(t, condition.Reason, "RecoveryCannotInstall") + assertTDEProviderCondition(t, cluster, metav1.ConditionFalse, "ExtensionNotInstalled") + }) + + t.Run("KeyUnavailable", func(t *testing.T) { + var calls []execCall + cluster := newCluster() + + r := &Reconciler{ + Recorder: events.NewRecorder(t, runtime.Scheme), + PodExec: execResponder(&calls, + answer(true, errors.New("could not fetch principal key"))), + } + + r.reconcilePGTDEStandby(ctx, cluster, standbyObserved()) + + assert.Equal(t, len(calls), 2) + assert.Assert(t, meta.IsStatusConditionTrue(cluster.Status.Conditions, v1beta1.PGTDEEnabled), + "the extension is installed even though its key is out of reach") + assertTDEProviderCondition(t, cluster, metav1.ConditionFalse, "KeyUnavailable") + }) + + t.Run("ObservationFailureLeavesConditionsAlone", func(t *testing.T) { + var calls []execCall + cluster := newCluster() + + r := &Reconciler{ + Recorder: events.NewRecorder(t, runtime.Scheme), + PodExec: execResponder(&calls, func(call execCall, stdout io.Writer) error { + return errors.New("connection to server failed") + }), + } + + r.reconcilePGTDEStandby(ctx, cluster, standbyObserved()) + + // A false condition here would strip pg_tde from + // shared_preload_libraries and take the vault credentials off the Pods. + // A Postgres still replaying WAL and refusing connections is normal. + assert.Equal(t, len(calls), 1) + assert.Equal(t, len(cluster.Status.Conditions), 0) + rec, ok := r.Recorder.(*events.Recorder) + assert.Assert(t, ok) + assert.Equal(t, len(rec.Events), 0) + }) + + t.Run("SpecDisabledKeepsTheExtensionReported", func(t *testing.T) { + var calls []execCall + cluster := newCluster() + cluster.Spec.Extensions.PGTDE.Enabled = false + meta.SetStatusCondition(&cluster.Status.Conditions, metav1.Condition{ + Type: v1beta1.PGTDEEnabled, + Status: metav1.ConditionTrue, + Reason: "ReplicatedFromSource", + }) + + r := &Reconciler{ + Recorder: events.NewRecorder(t, runtime.Scheme), + PodExec: execResponder(&calls, answer(true, nil)), + } + + r.reconcilePGTDEStandby(ctx, cluster, standbyObserved()) + + condition := extensionCondition(cluster) + assert.Assert(t, condition != nil) + assert.Equal(t, condition.Status, metav1.ConditionTrue, + "a cluster in recovery cannot drop the extension it is reading") + assert.Equal(t, condition.Reason, "RecoveryCannotDrop") + + // reconcilePGTDEProviders removes the provider condition when the spec + // disables pg_tde, so this must not put one back. + assert.Assert(t, meta.FindStatusCondition(cluster.Status.Conditions, + v1beta1.PGTDEVaultProviderReady) == nil) + }) + + t.Run("PendingProviderChangeIsNotOverwritten", func(t *testing.T) { + var calls []execCall + cluster := newCluster() + cluster.Status.PGTDERevision = "some-revision" + meta.SetStatusCondition(&cluster.Status.Conditions, metav1.Condition{ + Type: v1beta1.PGTDEVaultProviderReady, + Status: metav1.ConditionFalse, + Reason: "ChangeInProgress", + }) + + r := &Reconciler{ + Recorder: events.NewRecorder(t, runtime.Scheme), + PodExec: execResponder(&calls, answer(true, nil)), + } + + r.reconcilePGTDEStandby(ctx, cluster, standbyObserved()) + + // Only promotion can finish a change reconcilePGTDEProviders started, so + // the stall stays visible. + assertTDEProviderCondition(t, cluster, metav1.ConditionFalse, "ChangeInProgress") + assert.Equal(t, cluster.Status.PGTDERevision, "some-revision") + assert.Assert(t, meta.IsStatusConditionTrue(cluster.Status.Conditions, v1beta1.PGTDEEnabled)) + }) +} + func TestReconcilePostgresDatabasesPGTDEStatusIsIndependent(t *testing.T) { t.Parallel() ctx := context.Background() diff --git a/internal/controller/postgrescluster/postgres.go b/internal/controller/postgrescluster/postgres.go index 2458095293..885b26b437 100644 --- a/internal/controller/postgrescluster/postgres.go +++ b/internal/controller/postgrescluster/postgres.go @@ -216,7 +216,8 @@ func (r *Reconciler) reconcilePostgresDatabases( } // Find the PostgreSQL instance that can execute SQL that writes system - // catalogs. When there is none, return early. + // catalogs. When there is none, return early. A standby cluster never has + // one; reconcilePGTDEStandby reports its pg_tde state instead. K8SPG-911 pod, _ := instances.writablePod(container) if pod == nil { return nil @@ -471,6 +472,54 @@ func (r *Reconciler) reconcilePostgresDatabases( return err } +// reconcilePGTDEStandby reports the pg_tde state of a cluster in recovery. +func (r *Reconciler) reconcilePGTDEStandby( + ctx context.Context, + cluster *v1beta1.PostgresCluster, + instances *observedInstances, +) { + const container = naming.ContainerDatabase + + if !cluster.IsStandby() { + return + } + + if !cluster.Spec.Extensions.PGTDE.Enabled && + !meta.IsStatusConditionTrue(cluster.Status.Conditions, v1beta1.PGTDEEnabled) { + return + } + + log := logging.FromContext(ctx).WithName("PGTDE") + + pod, _ := instances.standbyLeaderPod(container) + if pod == nil { + log.V(1).Info("Waiting for a standby leader") + return + } + + log = log.WithValues("pod", pod.Name) + ctx = logging.NewContext(ctx, log) + + pgExecutor := postgres.Executor(func( + ctx context.Context, stdin io.Reader, stdout, stderr io.Writer, command ...string, + ) error { + return r.PodExec(ctx, pod.Namespace, pod.Name, container, stdin, stdout, stderr, command...) + }) + + installed, err := pgtde.ObserveExtension(ctx, pgExecutor) + if err != nil { + log.V(1).Info("could not observe pg_tde", "error", err.Error()) + return + } + + var keyErr error + if installed { + keyErr = pgtde.VerifyPrincipalKey(ctx, pgExecutor) + } + + pgtde.ReportStandby(cluster, installed, keyErr) +} + // reconcilePGTDEProviders configures pg_tde providers using a two-phase // approach for vault credential changes: // @@ -501,6 +550,13 @@ func (r *Reconciler) reconcilePGTDEProviders( return nil } + // K8SPG-911: everything below writes, and a cluster in recovery cannot run + // any of it. reconcilePGTDEStandby reports the key provider from what pg_tde + // says instead. + if cluster.IsStandby() { + return nil + } + // Wait for all instances to match their pod templates before configuring // the vault provider. This prevents running SQL on pods that are mid-rollout. for _, inst := range instances.forCluster { diff --git a/internal/pgtde/postgres.go b/internal/pgtde/postgres.go index 29e54155a7..d0439cc8b8 100644 --- a/internal/pgtde/postgres.go +++ b/internal/pgtde/postgres.go @@ -114,12 +114,143 @@ func ReportExtension(cluster *crunchyv1beta1.PostgresCluster, record record.Even meta.SetStatusCondition(&cluster.Status.Conditions, condition) } +// readOnlyPSQLArgs make psql print a single value and nothing else, so the +// result of a one-row, one-column query is exactly "t" or "f". +var readOnlyPSQLArgs = []string{"--no-align", "--tuples-only"} + +// ObserveExtension reports whether pg_tde is installed. +func ObserveExtension(ctx context.Context, exec postgres.Executor) (bool, error) { + log := logging.FromContext(ctx) + + stdout, stderr, err := exec.Exec(ctx, + strings.NewReader(`SELECT EXISTS (SELECT 1 FROM pg_catalog.pg_extension WHERE extname = 'pg_tde')`), + map[string]string{ + "ON_ERROR_STOP": "on", // Abort when any one statement fails. + "QUIET": "on", // Do not print successful statements to stdout. + }, readOnlyPSQLArgs) + + log.V(1).Info("observed pg_tde extension", "stdout", stdout, "stderr", stderr) + + if err != nil { + return false, errors.Wrap(err, psqlStderr(stderr)) + } + + switch strings.TrimSpace(stdout) { + case "t": + return true, nil + case "f": + return false, nil + } + + return false, errors.Errorf("unexpected result from pg_extension: %q", stdout) +} + +// VerifyPrincipalKey asks pg_tde to fetch the principal key its key provider names. +func VerifyPrincipalKey(ctx context.Context, exec postgres.Executor) error { + log := logging.FromContext(ctx) + + stdout, stderr, err := exec.Exec(ctx, + strings.NewReader(`SELECT pg_tde_verify_default_key()`), + map[string]string{ + "ON_ERROR_STOP": "on", // Abort when any one statement fails. + "QUIET": "on", // Do not print successful statements to stdout. + }, readOnlyPSQLArgs) + + log.V(1).Info("verified pg_tde principal key", "stdout", stdout, "stderr", stderr) + + if err != nil { + return errors.Wrap(err, psqlStderr(stderr)) + } + + return nil +} + +func psqlStderr(stderr string) string { + line, _, _ := strings.Cut(strings.TrimSpace(stderr), "\n") + if line == "" { + return "psql failed" + } + return line +} + +// ReportStandby records the pg_tde state of a cluster in recovery from what its +// standby leader reported. +func ReportStandby( + cluster *crunchyv1beta1.PostgresCluster, installed bool, keyErr error) { + pgTDE := cluster.Spec.Extensions.PGTDE + + extension := metav1.Condition{ + Type: crunchyv1beta1.PGTDEEnabled, + Status: metav1.ConditionTrue, + Reason: "ReplicatedFromSource", + Message: "pg_tde arrived with the data replicated from the source cluster", + ObservedGeneration: cluster.GetGeneration(), + } + + switch { + case installed && !pgTDE.Enabled: + // Stay true. This condition is what holds pg_tde in + // shared_preload_libraries and the vault credentials on the Pods, and a + // cluster replaying encrypted data cannot read it without both. + extension.Reason = "RecoveryCannotDrop" + extension.Message = "pg_tde is disabled in PerconaPGCluster but the extension is still" + + " installed; a cluster in recovery cannot drop it. Disable pg_tde on the source" + + " cluster, or promote this one first." + + case !installed && pgTDE.Enabled: + extension.Status = metav1.ConditionFalse + extension.Reason = "RecoveryCannotInstall" + extension.Message = "pg_tde is enabled in PerconaPGCluster but the extension is not" + + " installed; a cluster in recovery cannot install it. Enable pg_tde on the source" + + " cluster." + + case !installed && !pgTDE.Enabled: + extension.Status = metav1.ConditionFalse + extension.Reason = "Disabled" + extension.Message = "pg_tde is disabled in PerconaPGCluster" + } + + meta.SetStatusCondition(&cluster.Status.Conditions, extension) + + if !pgTDE.Enabled || pgTDE.Vault == nil || cluster.Status.PGTDERevision != "" { + return + } + + provider := metav1.Condition{ + Type: crunchyv1beta1.PGTDEVaultProviderReady, + Status: metav1.ConditionTrue, + Reason: "ReplicatedFromSource", + Message: "pg_tde fetched its principal key using the replicated key provider", + ObservedGeneration: cluster.GetGeneration(), + } + + switch { + case !installed: + provider.Status = metav1.ConditionFalse + provider.Reason = "ExtensionNotInstalled" + provider.Message = "pg_tde is not installed, so it has no key provider" + + case keyErr != nil: + provider.Status = metav1.ConditionFalse + provider.Reason = "KeyUnavailable" + provider.Message = "pg_tde could not fetch its principal key on the standby leader: " + + keyErr.Error() + } + + meta.SetStatusCondition(&cluster.Status.Conditions, provider) +} + func PostgreSQLParameters(cluster *crunchyv1beta1.PostgresCluster, outParameters *postgres.Parameters) { outParameters.Mandatory.AppendToList("shared_preload_libraries", "pg_tde") canEnableWALEncryption := meta.IsStatusConditionTrue(cluster.Status.Conditions, crunchyv1beta1.PGTDEEnabled) && meta.IsStatusConditionTrue(cluster.Status.Conditions, crunchyv1beta1.PGTDEVaultProviderReady) + if cluster.IsStandby() { + canEnableWALEncryption = !meta.IsStatusConditionFalse( + cluster.Status.Conditions, crunchyv1beta1.PGTDEEnabled) + } + if cluster.Spec.Extensions.PGTDE.WALEncryption && canEnableWALEncryption { outParameters.Mandatory.Add("pg_tde.wal_encrypt", "on") } else { diff --git a/internal/pgtde/postgres_test.go b/internal/pgtde/postgres_test.go index a67b6a1a4d..71d9a0e2ec 100644 --- a/internal/pgtde/postgres_test.go +++ b/internal/pgtde/postgres_test.go @@ -68,8 +68,7 @@ func TestDisableInPostgreSQL(t *testing.T) { return expected } - ctx := context.Background() - assert.Equal(t, expected, disableInPostgreSQL(ctx, exec)) + assert.Equal(t, expected, disableInPostgreSQL(t.Context(), exec)) } func TestPostgreSQLParameters(t *testing.T) { @@ -163,6 +162,323 @@ func TestPostgreSQLParameters(t *testing.T) { "shared_preload_libraries": "pg_tde", "pg_tde.wal_encrypt": "off", }) + + // K8SPG-911: a standby cluster cannot install the extension nor configure a + // key provider, so the interlock above can never clear. Its pg_tde state + // arrives replicated and the WAL it replays is encrypted from its first + // start, so the parameter follows the spec until something says otherwise. + t.Run("Standby", func(t *testing.T) { + standby := new(crunchyv1beta1.PostgresCluster) + standby.Spec.Standby = &crunchyv1beta1.PostgresStandbySpec{ + Enabled: true, RepoName: "repo1", + } + standby.Spec.Extensions.PGTDE.Enabled = true + standby.Spec.Extensions.PGTDE.WALEncryption = true + + for _, tc := range []struct { + name string + conditions []metav1.Condition + expected string + }{ + { + // The case that matters: the parameter is rendered into the + // cluster ConfigMap before any Pod exists to be observed, so a + // standby with no conditions has to start with WAL encryption on. + "NoConditions", nil, "on", + }, + { + // The provider condition is informational on a standby. The + // operator cannot repoint the replicated provider, so refusing + // WAL encryption over it would only leave the cluster wrong in a + // second way. + "ProviderNotReady", []metav1.Condition{ + condition(crunchyv1beta1.PGTDEEnabled, metav1.ConditionTrue), + condition(crunchyv1beta1.PGTDEVaultProviderReady, metav1.ConditionFalse), + }, "on", + }, + { + "Both", []metav1.Condition{ + condition(crunchyv1beta1.PGTDEEnabled, metav1.ConditionTrue), + condition(crunchyv1beta1.PGTDEVaultProviderReady, metav1.ConditionTrue), + }, "on", + }, + { + // Observed to be absent: the source cluster does not have pg_tde, + // so there is nothing to encrypt WAL with. + "ExtensionAbsent", []metav1.Condition{ + condition(crunchyv1beta1.PGTDEEnabled, metav1.ConditionFalse), + }, "off", + }, + } { + t.Run(tc.name, func(t *testing.T) { + standby.Status.Conditions = tc.conditions + parameters := postgres.Parameters{Mandatory: postgres.NewParameterSet()} + PostgreSQLParameters(standby, ¶meters) + + assert.DeepEqual(t, parameters.Mandatory.AsMap(), map[string]string{ + "shared_preload_libraries": "pg_tde", + "pg_tde.wal_encrypt": tc.expected, + }) + }) + } + + t.Run("WALEncryptionDisabled", func(t *testing.T) { + standby.Spec.Extensions.PGTDE.WALEncryption = false + standby.Status.Conditions = nil + parameters := postgres.Parameters{Mandatory: postgres.NewParameterSet()} + PostgreSQLParameters(standby, ¶meters) + + assert.Equal(t, parameters.Mandatory.AsMap()["pg_tde.wal_encrypt"], "off") + }) + }) +} + +// execReturning builds an Executor that writes the given stdout and stderr and +// then returns err, so a caller that parses psql output can be tested. +func execReturning(stdout, stderr string, err error, assertions func(sql string, command []string)) postgres.Executor { + return func( + _ context.Context, stdin io.Reader, outWriter, errWriter io.Writer, command ...string, + ) error { + if assertions != nil { + sql, readErr := io.ReadAll(stdin) + if readErr != nil { + return readErr + } + assertions(string(sql), command) + } + _, _ = outWriter.Write([]byte(stdout)) + _, _ = errWriter.Write([]byte(stderr)) + return err + } +} + +func TestObserveExtension(t *testing.T) { + t.Run("statement", func(t *testing.T) { + var seenSQL string + var seenCommand []string + exec := execReturning("t\n", "", nil, func(sql string, command []string) { + seenSQL, seenCommand = sql, command + }) + + installed, err := ObserveExtension(t.Context(), exec) + assert.NilError(t, err) + assert.Assert(t, installed) + + assert.Equal(t, seenSQL, `SELECT EXISTS (SELECT 1 FROM pg_catalog.pg_extension WHERE extname = 'pg_tde')`) + + joined := strings.Join(seenCommand, " ") + assert.Assert(t, strings.Contains(joined, "--no-align")) + assert.Assert(t, strings.Contains(joined, "--tuples-only")) + assert.Assert(t, strings.Contains(joined, "--set=ON_ERROR_STOP=on")) + }) + + for _, tc := range []struct { + name string + stdout string + stderr string + execErr error + installed bool + wantErr string + }{ + {name: "Installed", stdout: "t\n", installed: true}, + {name: "NotInstalled", stdout: "f\n"}, + {name: "PaddedOutput", stdout: " t ", installed: true}, + { + // A server that is still starting up refuses the connection. Saying + // "not installed" here would strip pg_tde from a cluster that has it. + name: "ExecFails", + stderr: "psql: error: connection to server failed\nsomething else", + execErr: errors.New("exit status 2"), + wantErr: "connection to server failed", + }, + {name: "EmptyOutput", wantErr: `unexpected result from pg_extension: ""`}, + {name: "UnexpectedOutput", stdout: "x", wantErr: `unexpected result from pg_extension: "x"`}, + } { + t.Run(tc.name, func(t *testing.T) { + installed, err := ObserveExtension(t.Context(), + execReturning(tc.stdout, tc.stderr, tc.execErr, nil)) + + if tc.wantErr == "" { + assert.NilError(t, err) + assert.Equal(t, installed, tc.installed) + return + } + + assert.ErrorContains(t, err, tc.wantErr) + assert.Assert(t, !installed, "an unreadable answer must not report pg_tde as absent") + assert.Assert(t, !strings.Contains(err.Error(), "something else"), + "expected only the first line of the psql diagnostic") + }) + } +} + +func TestVerifyPrincipalKey(t *testing.T) { + t.Run("statement", func(t *testing.T) { + var seenSQL string + var seenCommand []string + exec := execReturning("", "", nil, func(sql string, command []string) { + seenSQL, seenCommand = sql, command + }) + + assert.NilError(t, VerifyPrincipalKey(t.Context(), exec)) + assert.Equal(t, seenSQL, `SELECT pg_tde_verify_default_key()`) + assert.Assert(t, strings.Contains(strings.Join(seenCommand, " "), "--set=ON_ERROR_STOP=on")) + }) + + t.Run("reports the first line of the diagnostic", func(t *testing.T) { + stderr := strings.Join([]string{ + `ERROR: failed to retrieve principal key from keyring "vault-provider"`, + `CONTEXT: SQL statement "SELECT pg_tde_verify_default_key();"`, + `STATEMENT: SELECT pg_tde_verify_default_key();`, + }, "\n") + + err := VerifyPrincipalKey(t.Context(), + execReturning("", stderr, errors.New("exit status 3"), nil)) + + assert.ErrorContains(t, err, `failed to retrieve principal key from keyring "vault-provider"`) + assert.Assert(t, !strings.Contains(err.Error(), "CONTEXT:")) + assert.Assert(t, !strings.Contains(err.Error(), "STATEMENT:")) + }) +} + +func TestReportStandby(t *testing.T) { + standbyCluster := func(enabled bool, vault *crunchyv1beta1.PGTDEVaultSpec) *crunchyv1beta1.PostgresCluster { + cluster := &crunchyv1beta1.PostgresCluster{} + cluster.Generation = 3 + cluster.Spec.Standby = &crunchyv1beta1.PostgresStandbySpec{Enabled: true, RepoName: "repo1"} + cluster.Spec.Extensions.PGTDE.Enabled = enabled + cluster.Spec.Extensions.PGTDE.Vault = vault + return cluster + } + + vault := &crunchyv1beta1.PGTDEVaultSpec{ + Host: "https://vault.example.com", + MountPath: "secret/data", + TokenSecret: crunchyv1beta1.PGTDESecretObjectReference{ + Name: "token-secret", Key: "token-key", + }, + } + + t.Run("installed and enabled", func(t *testing.T) { + cluster := standbyCluster(true, vault) + + ReportStandby(cluster, true, nil) + + extension := meta.FindStatusCondition(cluster.Status.Conditions, crunchyv1beta1.PGTDEEnabled) + assert.Assert(t, extension != nil) + assert.Equal(t, extension.Status, metav1.ConditionTrue) + assert.Equal(t, extension.Reason, "ReplicatedFromSource") + assert.Equal(t, extension.Message, + "pg_tde arrived with the data replicated from the source cluster") + assert.Equal(t, extension.ObservedGeneration, int64(3)) + + provider := meta.FindStatusCondition(cluster.Status.Conditions, crunchyv1beta1.PGTDEVaultProviderReady) + assert.Assert(t, provider != nil) + assert.Equal(t, provider.Status, metav1.ConditionTrue) + assert.Equal(t, provider.Reason, "ReplicatedFromSource") + assert.Equal(t, provider.Message, + "pg_tde fetched its principal key using the replicated key provider") + + assert.Equal(t, cluster.Status.PGTDERevision, "", + "the operator has run no provider SQL on a standby") + }) + + t.Run("installed but disabled in the spec", func(t *testing.T) { + cluster := standbyCluster(false, vault) + + ReportStandby(cluster, true, nil) + + // A false condition here would strip pg_tde from + // shared_preload_libraries and take the vault credentials off the Pods, + // leaving a cluster that cannot read the encrypted data it is replaying. + extension := meta.FindStatusCondition(cluster.Status.Conditions, crunchyv1beta1.PGTDEEnabled) + assert.Assert(t, extension != nil) + assert.Equal(t, extension.Status, metav1.ConditionTrue) + assert.Equal(t, extension.Reason, "RecoveryCannotDrop") + assert.Assert(t, strings.Contains(extension.Message, "promote this one first")) + + // reconcilePGTDEProviders removes this condition when the spec disables + // pg_tde, so reporting one here would only fight with it. + assert.Assert(t, meta.FindStatusCondition( + cluster.Status.Conditions, crunchyv1beta1.PGTDEVaultProviderReady) == nil) + }) + + t.Run("enabled but not installed", func(t *testing.T) { + cluster := standbyCluster(true, vault) + + ReportStandby(cluster, false, nil) + + extension := meta.FindStatusCondition(cluster.Status.Conditions, crunchyv1beta1.PGTDEEnabled) + assert.Assert(t, extension != nil) + assert.Equal(t, extension.Status, metav1.ConditionFalse) + assert.Equal(t, extension.Reason, "RecoveryCannotInstall") + + provider := meta.FindStatusCondition(cluster.Status.Conditions, crunchyv1beta1.PGTDEVaultProviderReady) + assert.Assert(t, provider != nil) + assert.Equal(t, provider.Status, metav1.ConditionFalse) + assert.Equal(t, provider.Reason, "ExtensionNotInstalled") + }) + + t.Run("neither installed nor enabled", func(t *testing.T) { + cluster := standbyCluster(false, nil) + + ReportStandby(cluster, false, nil) + + extension := meta.FindStatusCondition(cluster.Status.Conditions, crunchyv1beta1.PGTDEEnabled) + assert.Assert(t, extension != nil) + assert.Equal(t, extension.Status, metav1.ConditionFalse) + assert.Equal(t, extension.Reason, "Disabled", + "expected the same reason the writable path reports") + }) + + t.Run("key unavailable", func(t *testing.T) { + cluster := standbyCluster(true, vault) + + ReportStandby(cluster, true, errors.New("ERROR: principal key not found")) + + // The extension really is there; only the key is not reachable. + extension := meta.FindStatusCondition(cluster.Status.Conditions, crunchyv1beta1.PGTDEEnabled) + assert.Assert(t, extension != nil) + assert.Equal(t, extension.Status, metav1.ConditionTrue) + + provider := meta.FindStatusCondition(cluster.Status.Conditions, crunchyv1beta1.PGTDEVaultProviderReady) + assert.Assert(t, provider != nil) + assert.Equal(t, provider.Status, metav1.ConditionFalse) + assert.Equal(t, provider.Reason, "KeyUnavailable") + assert.Assert(t, strings.Contains(provider.Message, "principal key not found")) + }) + + t.Run("without a vault in the spec", func(t *testing.T) { + cluster := standbyCluster(true, nil) + + ReportStandby(cluster, true, nil) + + assert.Assert(t, meta.IsStatusConditionTrue( + cluster.Status.Conditions, crunchyv1beta1.PGTDEEnabled)) + assert.Assert(t, meta.FindStatusCondition( + cluster.Status.Conditions, crunchyv1beta1.PGTDEVaultProviderReady) == nil) + }) + + t.Run("leaves a pending provider change alone", func(t *testing.T) { + cluster := standbyCluster(true, vault) + cluster.Status.PGTDERevision = "abc123" + meta.SetStatusCondition(&cluster.Status.Conditions, metav1.Condition{ + Type: crunchyv1beta1.PGTDEVaultProviderReady, + Status: metav1.ConditionFalse, + Reason: "ChangeInProgress", + Message: "waiting for Pods to restart with the new vault credentials", + }) + + ReportStandby(cluster, true, nil) + + // A change reconcilePGTDEProviders started is one a cluster in recovery + // cannot finish. Papering over it with a cheerful condition would hide a + // stall that only promotion can clear. + provider := meta.FindStatusCondition(cluster.Status.Conditions, crunchyv1beta1.PGTDEVaultProviderReady) + assert.Assert(t, provider != nil) + assert.Equal(t, provider.Reason, "ChangeInProgress") + assert.Equal(t, cluster.Status.PGTDERevision, "abc123") + }) } func TestAddVaultProvider(t *testing.T) { @@ -190,7 +506,6 @@ func TestAddVaultProvider(t *testing.T) { return expected } - ctx := context.Background() vault := &crunchyv1beta1.PGTDEVaultSpec{ Host: "https://vault.example.com", MountPath: "secret/data", @@ -204,7 +519,7 @@ func TestAddVaultProvider(t *testing.T) { }, } tokenPath, caPath := VaultCredentialPaths(vault) - assert.Equal(t, expected, addVaultProvider(ctx, exec, vault, tokenPath, caPath)) + assert.Equal(t, expected, addVaultProvider(t.Context(), exec, vault, tokenPath, caPath)) }) t.Run("does not interpret stderr", func(t *testing.T) { @@ -217,7 +532,6 @@ func TestAddVaultProvider(t *testing.T) { return nil } - ctx := context.Background() vault := &crunchyv1beta1.PGTDEVaultSpec{ Host: "https://vault.example.com", MountPath: "secret/data", @@ -227,7 +541,7 @@ func TestAddVaultProvider(t *testing.T) { }, } tokenPath, caPath := VaultCredentialPaths(vault) - assert.NilError(t, addVaultProvider(ctx, exec, vault, tokenPath, caPath)) + assert.NilError(t, addVaultProvider(t.Context(), exec, vault, tokenPath, caPath)) }) t.Run("without CA secret", func(t *testing.T) { @@ -323,8 +637,7 @@ func TestSetDefaultKey(t *testing.T) { return expected } - ctx := context.Background() - assert.Equal(t, expected, setDefaultKey(ctx, exec, clusterID)) + assert.Equal(t, expected, setDefaultKey(t.Context(), exec, clusterID)) }) } @@ -353,7 +666,6 @@ func TestChangeVaultProvider(t *testing.T) { return expected } - ctx := context.Background() vault := &crunchyv1beta1.PGTDEVaultSpec{ Host: "https://vault.example.com", MountPath: "secret/data", @@ -367,7 +679,7 @@ func TestChangeVaultProvider(t *testing.T) { }, } tokenPath, caPath := VaultCredentialPaths(vault) - assert.Equal(t, expected, changeVaultProvider(ctx, exec, vault, tokenPath, caPath)) + assert.Equal(t, expected, changeVaultProvider(t.Context(), exec, vault, tokenPath, caPath)) }) t.Run("without CA secret", func(t *testing.T) { @@ -381,7 +693,6 @@ func TestChangeVaultProvider(t *testing.T) { return nil } - ctx := context.Background() vault := &crunchyv1beta1.PGTDEVaultSpec{ Host: "https://vault.example.com", MountPath: "secret/data", @@ -391,7 +702,7 @@ func TestChangeVaultProvider(t *testing.T) { }, } tokenPath, caPath := VaultCredentialPaths(vault) - assert.NilError(t, changeVaultProvider(ctx, exec, vault, tokenPath, caPath)) + assert.NilError(t, changeVaultProvider(t.Context(), exec, vault, tokenPath, caPath)) }) } diff --git a/pkg/apis/upstream.pgv2.percona.com/v1beta1/postgrescluster_test.go b/pkg/apis/upstream.pgv2.percona.com/v1beta1/postgrescluster_test.go index becd075b8b..c1a5a23a5a 100644 --- a/pkg/apis/upstream.pgv2.percona.com/v1beta1/postgrescluster_test.go +++ b/pkg/apis/upstream.pgv2.percona.com/v1beta1/postgrescluster_test.go @@ -247,3 +247,14 @@ func TestMetadataGetAnnotations(t *testing.T) { }) } } + +func TestIsStandby(t *testing.T) { + var cluster PostgresCluster + assert.Assert(t, !cluster.IsStandby(), "expected no standby spec to mean no standby") + + cluster.Spec.Standby = &PostgresStandbySpec{RepoName: "repo1"} + assert.Assert(t, !cluster.IsStandby(), "expected a disabled standby spec to mean no standby") + + cluster.Spec.Standby.Enabled = true + assert.Assert(t, cluster.IsStandby()) +} diff --git a/pkg/apis/upstream.pgv2.percona.com/v1beta1/postgrescluster_types.go b/pkg/apis/upstream.pgv2.percona.com/v1beta1/postgrescluster_types.go index 6c78c6d587..5213af1d50 100644 --- a/pkg/apis/upstream.pgv2.percona.com/v1beta1/postgrescluster_types.go +++ b/pkg/apis/upstream.pgv2.percona.com/v1beta1/postgrescluster_types.go @@ -1062,6 +1062,11 @@ func (cr *PostgresCluster) BackupSpecFound() bool { return !reflect.DeepEqual(cr.Spec.Backups, Backups{PGBackRest: PGBackRestArchive{}}) } +// IsStandby returns whether this cluster replays WAL from a source outside of itself. +func (cr *PostgresCluster) IsStandby() bool { + return cr.Spec.Standby != nil && cr.Spec.Standby.Enabled +} + // K8SPG-864 type SidecarPVC struct { Name string `json:"name"`