Skip to content

Commit fcdf59a

Browse files
Andrey Kolkovclaude
authored and
androndo
committed
fix(backups): clear useSystemBucket on Postgres in-place restore (#3327)
A Postgres app created with the platform-recommended backup.useSystemBucket=true cannot be restored in place. The CNPG restore driver writes explicit backup coordinates (destinationPath / endpointURL / s3CredentialsSecret) and flips bootstrap.enabled=true, but it never cleared the server-side useSystemBucket=true. The chart then fails its `bootstrap.enabled + useSystemBucket` render guard, the HelmRelease never re-renders bootstrap.recovery, and the RestoreJob can never converge (helm remediation retries forever). Clear backup.useSystemBucket in the restore patch so the chart receives a self-consistent value set: explicit coordinates, no system-bucket flag, recovery on. postgresapp.Backup.UseSystemBucket intentionally omits `omitempty` so the false zero-value survives into the JSON merge patch and actually overwrites the server value; with omitempty it would be dropped and the bug would persist. Signed-off-by: Andrey Kolkov <[email protected]> Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
1 parent 61a236c commit fcdf59a

3 files changed

Lines changed: 50 additions & 2 deletions

File tree

‎internal/backupcontroller/cnpgstrategy_controller.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1102,6 +1102,12 @@ func buildPostgresAppRestorePatch(
11021102

11031103
patched.Spec.Backup.DestinationPath = sourceDestinationPath
11041104
patched.Spec.Backup.EndpointURL = sourceEndpointURL
1105+
// Clear useSystemBucket: an app created with backup.useSystemBucket=true
1106+
// keeps that flag server-side, which combined with the explicit
1107+
// coordinates written above trips the chart's `bootstrap.enabled +
1108+
// useSystemBucket` render guard, so bootstrap.recovery never renders and
1109+
// the restore can't converge (#3327).
1110+
patched.Spec.Backup.UseSystemBucket = false
11051111
// Switching to s3CredentialsSecret means inline keys must not survive
11061112
// on the CR .spec; otherwise tenants who switch credential modes leave
11071113
// cleartext keys behind in etcd and audit logs.

‎internal/backupcontroller/cnpgstrategy_controller_test.go‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -548,6 +548,44 @@ func TestBuildPostgresAppRestorePatch_NoSecretRefIsSkipped(t *testing.T) {
548548
}
549549
}
550550

551+
// TestBuildPostgresAppRestorePatch_ClearsUseSystemBucket locks in the fix for
552+
// issue #3327. A target app created with the platform-recommended
553+
// backup.useSystemBucket=true cannot be restored in place: the driver writes
554+
// explicit backup coordinates and flips bootstrap.enabled=true, but if the
555+
// server-side useSystemBucket=true survives, the chart fails the render on its
556+
// `bootstrap.enabled + useSystemBucket` guard and the HelmRelease never
557+
// re-renders bootstrap.recovery, so the restore can never converge.
558+
//
559+
// The patch must (1) set the field to false on the struct AND (2) actually
560+
// carry that false into the JSON merge patch. Point (2) is the subtle half:
561+
// with `omitempty` the false zero-value is dropped from the marshalled patch,
562+
// the server's true is left untouched, and the bug reappears. Asserting on the
563+
// raw merge-patch bytes guards that specific regression.
564+
func TestBuildPostgresAppRestorePatch_ClearsUseSystemBucket(t *testing.T) {
565+
app := newPostgresApp("pg", "tenant")
566+
app.Spec.Backup.UseSystemBucket = true
567+
568+
creds := &strategyv1alpha1.S3CredentialsTemplate{
569+
SecretRef: corev1.LocalObjectReference{Name: "creds"},
570+
}
571+
patched := buildPostgresAppRestorePatch(app, "src", "s3://b/", "https://s3", "", creds, nil, nil, nil)
572+
573+
if patched.Spec.Backup.UseSystemBucket {
574+
t.Errorf("spec.backup.useSystemBucket must be cleared on restore; got true")
575+
}
576+
577+
// The merge patch computed against the original (useSystemBucket=true) app
578+
// must serialise the false so it overwrites the server value. If it does
579+
// not appear here, the server keeps true and the chart guard wedges.
580+
data, err := client.MergeFrom(app).Data(patched)
581+
if err != nil {
582+
t.Fatalf("computing merge patch: %v", err)
583+
}
584+
if !strings.Contains(string(data), `"useSystemBucket":false`) {
585+
t.Errorf("merge patch must overwrite useSystemBucket with false; patch was: %s", data)
586+
}
587+
}
588+
551589
// TestCNPGPurgeNeeded locks in the dual-guard against re-purging the
552590
// freshly-recovered Cluster on a status-update failure. The controller used
553591
// to rely solely on a Status condition: if the post-purge Status().Update

‎internal/backupcontroller/postgresapp/types.go‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,8 +78,12 @@ type Bootstrap struct {
7878
}
7979

8080
type Backup struct {
81-
DestinationPath string `json:"destinationPath,omitempty"`
82-
EndpointURL string `json:"endpointURL,omitempty"`
81+
DestinationPath string `json:"destinationPath,omitempty"`
82+
EndpointURL string `json:"endpointURL,omitempty"`
83+
// Omits `omitempty` on purpose: restore must overwrite a server-side
84+
// `true` with false, but a JSON merge patch drops omitempty zero-values,
85+
// so the false has to be serialised to win (#3327).
86+
UseSystemBucket bool `json:"useSystemBucket"`
8387
S3AccessKey string `json:"s3AccessKey,omitempty"`
8488
S3SecretKey string `json:"s3SecretKey,omitempty"`
8589
S3CredentialsSecret S3CredentialsSecret `json:"s3CredentialsSecret,omitempty"`

0 commit comments

Comments
 (0)