From 560c44f71eba87c6bb08bb119e7572abdc9bfc69 Mon Sep 17 00:00:00 2001 From: Myasnikov Daniil Date: Wed, 4 Nov 2026 13:18:39 +0000 Subject: [PATCH] fix(backups): read a MariaDB dump from where its operator Backup wrote it The size probe took the bucket, endpoint, prefix, credentials and TLS settings from the strategy as it renders on the reconcile that creates the Backup, which is not necessarily what it rendered when the operator Backup was created: an existing operator Backup is reused untouched, and a backup can run for up to the driver's 30-minute deadline. An admin who edits the strategy in between sends the probe to the new location, where it measures the wrong prefix or nothing at all. The operator Backup's own spec.storage.s3 is where the dump went, so the probe reads that. It is also the only source a restore can use, since it targets that operator Backup long after the strategy may have changed. An operator Backup that does not store to S3 is refused before anything is listed. Assisted-by: LLM Signed-off-by: Myasnikov Daniil --- .../backupcontroller/mariadb_artifact_size.go | 43 +++--- .../mariadb_artifact_size_test.go | 139 ++++++++++++++---- .../mariadbstrategy_controller.go | 2 +- 3 files changed, 131 insertions(+), 53 deletions(-) diff --git a/internal/backupcontroller/mariadb_artifact_size.go b/internal/backupcontroller/mariadb_artifact_size.go index bf9dedcd9..d10f1c4a5 100644 --- a/internal/backupcontroller/mariadb_artifact_size.go +++ b/internal/backupcontroller/mariadb_artifact_size.go @@ -8,7 +8,6 @@ import ( "strings" "time" - strategyv1alpha1 "github.com/cozystack/cozystack/api/backups/strategy/v1alpha1" "github.com/cozystack/cozystack/internal/backupcontroller/mariadbtypes" ) @@ -40,16 +39,10 @@ type mariadbDump struct { // mdbBackup. Returns 0 - leaving Artifact.SizeBytes absent - when the dump // cannot be found or told apart: an unknown size must not be reported as a // measured one. -func (r *BackupJobReconciler) mariadbArtifactSizeBytes( - ctx context.Context, - namespace string, - s3 *strategyv1alpha1.MariaDBS3Template, - mdbBackup *mariadbtypes.Backup, - completedAt time.Time, -) int64 { +func (r *BackupJobReconciler) mariadbArtifactSizeBytes(ctx context.Context, mdbBackup *mariadbtypes.Backup, completedAt time.Time) int64 { ctx, cancel := context.WithTimeout(ctx, artifactSizeProbeTimeout) defer cancel() - dump, err := findMariaDBDump(ctx, r.objectStore(), namespace, s3, mdbBackup, completedAt) + dump, err := findMariaDBDump(ctx, r.objectStore(), mdbBackup, completedAt) if err != nil { getLogger(ctx).Info("MariaDB backup size not measured; leaving it unset", "backup", mdbBackup.Name, "error", err) return 0 @@ -57,17 +50,17 @@ func (r *BackupJobReconciler) mariadbArtifactSizeBytes( return dump.Size } -// findMariaDBDump lists the bucket the operator Backup wrote to and returns -// the one dump that belongs to it, as matchMariaDBDump decides. -func findMariaDBDump( - ctx context.Context, - store objectStore, - namespace string, - s3 *strategyv1alpha1.MariaDBS3Template, - mdbBackup *mariadbtypes.Backup, - completedAt time.Time, -) (mariadbDump, error) { - conn, err := mariadbS3Connection(ctx, store, namespace, s3) +// findMariaDBDump lists the bucket mdbBackup wrote to and returns the one +// dump that belongs to it, as matchMariaDBDump decides. The location and +// credentials come from mdbBackup's own spec: the strategy it was rendered +// from can have been edited since, and pointing at another bucket now says +// nothing about where this dump went. +func findMariaDBDump(ctx context.Context, store objectStore, mdbBackup *mariadbtypes.Backup, completedAt time.Time) (mariadbDump, error) { + s3 := mdbBackup.Spec.Storage.S3 + if s3 == nil { + return mariadbDump{}, errors.New("the operator Backup does not store to S3") + } + conn, err := mariadbS3Connection(ctx, store, mdbBackup.Namespace, s3) if err != nil { return mariadbDump{}, fmt.Errorf("resolve S3 credentials: %w", err) } @@ -114,17 +107,17 @@ func matchMariaDBDump(objects []s3ObjectInfo, prefix string, created, completed } } -// mariadbS3Connection resolves the Secrets and TLS settings the operator -// Backup was rendered with, so the bucket is read with the tenant's -// credentials rather than the controller's. -func mariadbS3Connection(ctx context.Context, store objectStore, namespace string, s3 *strategyv1alpha1.MariaDBS3Template) (s3Connection, error) { +// mariadbS3Connection resolves the Secrets and TLS settings of the operator +// Backup's storage, so the bucket is read with the tenant's credentials +// rather than the controller's. +func mariadbS3Connection(ctx context.Context, store objectStore, namespace string, s3 *mariadbtypes.S3Storage) (s3Connection, error) { conn := s3Connection{ Endpoint: s3.Endpoint, Region: s3.Region, Secure: s3.TLS != nil && s3.TLS.Enabled, } var err error - if conn.AccessKey, err = store.secretValue(ctx, namespace, s3.AccessKeyIDSecretKeyRef.Name, s3.AccessKeyIDSecretKeyRef.Key); err != nil { + if conn.AccessKey, err = store.secretValue(ctx, namespace, s3.AccessKeyIdSecretKeyRef.Name, s3.AccessKeyIdSecretKeyRef.Key); err != nil { return conn, err } if conn.SecretKey, err = store.secretValue(ctx, namespace, s3.SecretAccessKeySecretKeyRef.Name, s3.SecretAccessKeySecretKeyRef.Key); err != nil { diff --git a/internal/backupcontroller/mariadb_artifact_size_test.go b/internal/backupcontroller/mariadb_artifact_size_test.go index c28d0ad4e..719678892 100644 --- a/internal/backupcontroller/mariadb_artifact_size_test.go +++ b/internal/backupcontroller/mariadb_artifact_size_test.go @@ -77,14 +77,26 @@ func TestMariaDBOperatorS3Prefix(t *testing.T) { } } +// operatorBackupStoringTo returns the operator Backup a strategy with s3 +// storage renders, created at created in namespace tenant. +func operatorBackupStoringTo(t *testing.T, s3 *strategyv1alpha1.MariaDBS3Template, created time.Time) *mariadbtypes.Backup { + t.Helper() + storage, err := buildMariaDBBackupStorage(strategyv1alpha1.MariaDBStorageTemplate{S3: s3}) + if err != nil { + t.Fatal(err) + } + return &mariadbtypes.Backup{ + ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: "op-bk", CreationTimestamp: metav1.NewTime(created)}, + Spec: mariadbtypes.BackupSpec{Storage: storage}, + } +} + func TestMariaDBArtifactSizeBytes(t *testing.T) { created := time.Date(2026, 10, 21, 10, 0, 0, 0, time.UTC) completed := created.Add(3 * time.Minute) - mdbBackup := &mariadbtypes.Backup{ObjectMeta: metav1.ObjectMeta{ - Namespace: "tenant", Name: "op-bk", CreationTimestamp: metav1.NewTime(created), - }} s3 := newRenderedMariaDBTemplate().Storage.S3 s3.Prefix = "tenant/db" + mdbBackup := operatorBackupStoringTo(t, s3, created) const prefix = "tenant/db/" t.Run("single dump inside the backup window", func(t *testing.T) { @@ -93,7 +105,7 @@ func TestMariaDBArtifactSizeBytes(t *testing.T) { {Key: dumpKey(prefix, created.Add(time.Minute), "gzip"), Size: 4242, LastModified: created.Add(time.Minute)}, {Key: prefix + "0-backup-target.txt", Size: 7}, }} - got := reconcilerWithLister(t, l, s3CredsSecret("tenant", "creds")).mariadbArtifactSizeBytes(context.Background(), "tenant", s3, mdbBackup, completed) + got := reconcilerWithLister(t, l, s3CredsSecret("tenant", "creds")).mariadbArtifactSizeBytes(context.Background(), mdbBackup, completed) if got != 4242 { t.Errorf("size = %d, want 4242", got) } @@ -118,7 +130,7 @@ func TestMariaDBArtifactSizeBytes(t *testing.T) { } l := &fakeS3Lister{objects: []s3ObjectInfo{{Key: dumpKey(prefix, created.Add(time.Minute), "gzip"), Size: 3, LastModified: created.Add(time.Minute)}}} r := reconcilerWithLister(t, l, s3CredsSecret("tenant", "creds"), ca) - if got := r.mariadbArtifactSizeBytes(context.Background(), "tenant", tlsS3, mdbBackup, completed); got != 3 { + if got := r.mariadbArtifactSizeBytes(context.Background(), operatorBackupStoringTo(t, tlsS3, created), completed); got != 3 { t.Errorf("size = %d, want 3", got) } if !l.conn.Secure || string(l.conn.CAPEM) != "PEM" { @@ -136,7 +148,7 @@ func TestMariaDBArtifactSizeBytes(t *testing.T) { } l := &fakeS3Lister{objects: []s3ObjectInfo{{Key: dumpKey(prefix, created.Add(time.Minute), "gzip"), Size: 4, LastModified: created.Add(time.Minute)}}} r := reconcilerWithLister(t, l, s3CredsSecret("tenant", "creds"), sts) - if got := r.mariadbArtifactSizeBytes(context.Background(), "tenant", stsS3, mdbBackup, completed); got != 4 { + if got := r.mariadbArtifactSizeBytes(context.Background(), operatorBackupStoringTo(t, stsS3, created), completed); got != 4 { t.Errorf("size = %d, want 4", got) } if l.conn.SessionToken != "TOKEN" { @@ -148,7 +160,7 @@ func TestMariaDBArtifactSizeBytes(t *testing.T) { l := &fakeS3Lister{objects: []s3ObjectInfo{ {Key: dumpKey(prefix, created.Add(-30*time.Second), ""), Size: 9, LastModified: created.Add(time.Minute)}, }} - if got := reconcilerWithLister(t, l, s3CredsSecret("tenant", "creds")).mariadbArtifactSizeBytes(context.Background(), "tenant", s3, mdbBackup, completed); got != 9 { + if got := reconcilerWithLister(t, l, s3CredsSecret("tenant", "creds")).mariadbArtifactSizeBytes(context.Background(), mdbBackup, completed); got != 9 { t.Errorf("size = %d, want 9", got) } }) @@ -161,7 +173,7 @@ func TestMariaDBArtifactSizeBytes(t *testing.T) { {Key: dumpKey(prefix, created.Add(-4*time.Minute), "gzip"), Size: 7, LastModified: created.Add(time.Minute)}, {Key: dumpKey(prefix, created.Add(30*time.Second), "gzip"), Size: 666, LastModified: created.Add(-4 * time.Minute)}, }} - if got := reconcilerWithLister(t, l, s3CredsSecret("tenant", "creds")).mariadbArtifactSizeBytes(context.Background(), "tenant", s3, mdbBackup, completed); got != 0 { + if got := reconcilerWithLister(t, l, s3CredsSecret("tenant", "creds")).mariadbArtifactSizeBytes(context.Background(), mdbBackup, completed); got != 0 { t.Errorf("size = %d, want 0: no dump is inside the window on both clocks", got) } }) @@ -170,7 +182,7 @@ func TestMariaDBArtifactSizeBytes(t *testing.T) { l := &fakeS3Lister{objects: []s3ObjectInfo{ {Key: dumpKey(prefix, completed.Add(30*time.Second), "gzip"), Size: 5, LastModified: completed.Add(30 * time.Second)}, }} - if got := reconcilerWithLister(t, l, s3CredsSecret("tenant", "creds")).mariadbArtifactSizeBytes(context.Background(), "tenant", s3, mdbBackup, completed); got != 5 { + if got := reconcilerWithLister(t, l, s3CredsSecret("tenant", "creds")).mariadbArtifactSizeBytes(context.Background(), mdbBackup, completed); got != 5 { t.Errorf("size = %d, want 5", got) } }) @@ -179,7 +191,7 @@ func TestMariaDBArtifactSizeBytes(t *testing.T) { r := reconcilerWithLister(t, &fakeS3Lister{}, s3CredsSecret("tenant", "creds")) r.newS3ObjectLister = func(s3Connection) (s3ObjectLister, error) { return blockingLister{}, nil } withShortProbeTimeout(t, func() { - if got := r.mariadbArtifactSizeBytes(context.Background(), "tenant", s3, mdbBackup, completed); got != 0 { + if got := r.mariadbArtifactSizeBytes(context.Background(), mdbBackup, completed); got != 0 { t.Errorf("size = %d, want 0", got) } }) @@ -190,7 +202,7 @@ func TestMariaDBArtifactSizeBytes(t *testing.T) { {Key: dumpKey(prefix, created.Add(time.Minute), "gzip"), Size: 1, LastModified: created.Add(time.Minute)}, {Key: dumpKey(prefix, created.Add(2*time.Minute), "gzip"), Size: 2, LastModified: created.Add(2 * time.Minute)}, }} - if got := reconcilerWithLister(t, l, s3CredsSecret("tenant", "creds")).mariadbArtifactSizeBytes(context.Background(), "tenant", s3, mdbBackup, completed); got != 0 { + if got := reconcilerWithLister(t, l, s3CredsSecret("tenant", "creds")).mariadbArtifactSizeBytes(context.Background(), mdbBackup, completed); got != 0 { t.Errorf("size = %d, want 0 (ambiguous)", got) } }) @@ -199,14 +211,14 @@ func TestMariaDBArtifactSizeBytes(t *testing.T) { l := &fakeS3Lister{objects: []s3ObjectInfo{ {Key: dumpKey(prefix, completed.Add(time.Hour), "gzip"), Size: 1, LastModified: completed.Add(time.Hour)}, }} - if got := reconcilerWithLister(t, l, s3CredsSecret("tenant", "creds")).mariadbArtifactSizeBytes(context.Background(), "tenant", s3, mdbBackup, completed); got != 0 { + if got := reconcilerWithLister(t, l, s3CredsSecret("tenant", "creds")).mariadbArtifactSizeBytes(context.Background(), mdbBackup, completed); got != 0 { t.Errorf("size = %d, want 0", got) } }) t.Run("list error leaves size unset", func(t *testing.T) { l := &fakeS3Lister{err: errors.New("AccessDenied")} - if got := reconcilerWithLister(t, l, s3CredsSecret("tenant", "creds")).mariadbArtifactSizeBytes(context.Background(), "tenant", s3, mdbBackup, completed); got != 0 { + if got := reconcilerWithLister(t, l, s3CredsSecret("tenant", "creds")).mariadbArtifactSizeBytes(context.Background(), mdbBackup, completed); got != 0 { t.Errorf("size = %d, want 0", got) } }) @@ -214,7 +226,7 @@ func TestMariaDBArtifactSizeBytes(t *testing.T) { t.Run("missing credentials Secret leaves size unset", func(t *testing.T) { c := newMariaDBStrategyTestClient(t) r := &BackupJobReconciler{Client: c, Scheme: c.Scheme()} - if got := r.mariadbArtifactSizeBytes(context.Background(), "tenant", s3, mdbBackup, completed); got != 0 { + if got := r.mariadbArtifactSizeBytes(context.Background(), mdbBackup, completed); got != 0 { t.Errorf("size = %d, want 0", got) } }) @@ -252,6 +264,45 @@ func TestMatchMariaDBDump(t *testing.T) { } } +// TestFindMariaDBDump pins that a dump is read with nothing but a client, +// as a reconciler other than BackupJobReconciler has, and that an operator +// Backup that does not store to S3 is refused without a listing. +func TestFindMariaDBDump(t *testing.T) { + created := time.Date(2026, 10, 21, 10, 0, 0, 0, time.UTC) + completed := created.Add(3 * time.Minute) + s3 := newRenderedMariaDBTemplate().Storage.S3 + s3.Prefix = "tenant/db" + l := &fakeS3Lister{objects: []s3ObjectInfo{ + {Key: dumpKey("tenant/db/", created.Add(time.Minute), "gzip"), Size: 4242, LastModified: created.Add(time.Minute)}, + }} + store := objectStore{ + reader: newMariaDBStrategyTestClient(t, s3CredsSecret("tenant", "creds")), + newLister: func(conn s3Connection) (s3ObjectLister, error) { + l.conn = conn + return l, nil + }, + } + + got, err := findMariaDBDump(context.Background(), store, operatorBackupStoringTo(t, s3, created), completed) + if err != nil || got.Size != 4242 { + t.Fatalf("findMariaDBDump = %+v, %v; want the 4242-byte dump", got, err) + } + if l.conn.AccessKey != "AKID" { + t.Errorf("connection = %+v, want the Secret's key", l.conn) + } + + pvc := &mariadbtypes.Backup{ + ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: "op-pvc", CreationTimestamp: metav1.NewTime(created)}, + Spec: mariadbtypes.BackupSpec{Storage: mariadbtypes.BackupStorage{PersistentVolumeClaim: &corev1.PersistentVolumeClaimSpec{}}}, + } + if _, err := findMariaDBDump(context.Background(), store, pvc, completed); err == nil { + t.Error("findMariaDBDump on a PVC-backed operator Backup returned no error") + } + if l.calls != 1 { + t.Errorf("S3 listed %d times, want once: a PVC-backed Backup has nothing to list", l.calls) + } +} + // TestMariaDBArtifactSizeBytes_MinioClient drives the real minio client // against a stub ListObjectsV2 endpoint, so the Secret wiring, prefix and // response parsing are exercised together. @@ -280,11 +331,8 @@ func TestMariaDBArtifactSizeBytes_MinioClient(t *testing.T) { s3.Endpoint = strings.TrimPrefix(srv.URL, "http://") s3.Prefix = "tenant/db/" s3.Region = "us-east-1" - mdbBackup := &mariadbtypes.Backup{ObjectMeta: metav1.ObjectMeta{ - Namespace: "tenant", Name: "op-bk", CreationTimestamp: metav1.NewTime(created), - }} - if got := r.mariadbArtifactSizeBytes(context.Background(), "tenant", s3, mdbBackup, created.Add(3*time.Minute)); got != 31337 { + if got := r.mariadbArtifactSizeBytes(context.Background(), operatorBackupStoringTo(t, s3, created), created.Add(3*time.Minute)); got != 31337 { t.Fatalf("size = %d, want 31337", got) } if gotPath != "/bkt/" || gotPrefix != "tenant/db/" || gotDelimiter != "/" { @@ -298,13 +346,11 @@ func TestMariaDBArtifactSizeBytes_MinioClient(t *testing.T) { func TestCreateMariaDBBackupArtifact_SizeBytes(t *testing.T) { apps := mariadbapp.GroupName completed := metav1.NewTime(time.Date(2026, 10, 21, 10, 3, 0, 0, time.UTC)) - mdbBackup := &mariadbtypes.Backup{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "tenant", Name: "src-op-bk", - CreationTimestamp: metav1.NewTime(completed.Add(-3 * time.Minute)), - }, - Status: mariadbtypes.BackupStatus{Conditions: []metav1.Condition{mariadbBackupSucceeded(completed)}}, - } + rendered := newRenderedMariaDBTemplate() + rendered.Storage.S3.Prefix = "src/" + mdbBackup := operatorBackupStoringTo(t, rendered.Storage.S3, completed.Add(-3*time.Minute)) + mdbBackup.Name = "src-op-bk" + mdbBackup.Status.Conditions = []metav1.Condition{mariadbBackupSucceeded(completed)} resolved := &ResolvedBackupConfig{ StrategyRef: corev1.TypedLocalObjectReference{Kind: "MariaDB", Name: "mariadb-strategy-default"}, Parameters: map[string]string{}, @@ -318,8 +364,6 @@ func TestCreateMariaDBBackupArtifact_SizeBytes(t *testing.T) { r := reconcilerWithLister(t, &fakeS3Lister{objects: []s3ObjectInfo{ {Key: dumpKey("src/", completed.Add(-time.Minute), "gzip"), Size: 2048, LastModified: completed.Add(-time.Minute)}, }}, job, s3CredsSecret("tenant", "creds")) - rendered := newRenderedMariaDBTemplate() - rendered.Storage.S3.Prefix = "src/" artefact, err := r.createMariaDBBackupArtifact(context.Background(), job, resolved, mdbBackup, rendered) if err != nil { @@ -330,6 +374,47 @@ func TestCreateMariaDBBackupArtifact_SizeBytes(t *testing.T) { } } +// TestCreateMariaDBBackupArtifact_StrategyEditedMidBackup pins that the +// probe reads where the operator Backup wrote its dump, not where the +// strategy, edited while the backup ran, now points. +func TestCreateMariaDBBackupArtifact_StrategyEditedMidBackup(t *testing.T) { + apps := mariadbapp.GroupName + completed := metav1.NewTime(time.Date(2026, 10, 21, 10, 3, 0, 0, time.UTC)) + original := newRenderedMariaDBTemplate().Storage.S3 + original.Prefix = "original/" + mdbBackup := operatorBackupStoringTo(t, original, completed.Add(-3*time.Minute)) + mdbBackup.Status.Conditions = []metav1.Condition{mariadbBackupSucceeded(completed)} + edited := newRenderedMariaDBTemplate() + edited.Storage.S3.Bucket = "replacement-bkt" + edited.Storage.S3.Endpoint = "replacement.example:9000" + edited.Storage.S3.Prefix = "replacement/" + job := &backupsv1alpha1.BackupJob{ + ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: "bj-edited"}, + Spec: backupsv1alpha1.BackupJobSpec{ + ApplicationRef: corev1.TypedLocalObjectReference{Kind: "MariaDB", Name: "src", APIGroup: &apps}, + }, + } + l := &fakeS3Lister{objects: []s3ObjectInfo{ + {Key: dumpKey("original/", completed.Add(-time.Minute), "gzip"), Size: 2048, LastModified: completed.Add(-time.Minute)}, + }} + r := reconcilerWithLister(t, l, job, s3CredsSecret("tenant", "creds")) + resolved := &ResolvedBackupConfig{ + StrategyRef: corev1.TypedLocalObjectReference{Kind: "MariaDB", Name: "mariadb-strategy-default"}, + Parameters: map[string]string{}, + } + + artefact, err := r.createMariaDBBackupArtifact(context.Background(), job, resolved, mdbBackup, edited) + if err != nil { + t.Fatalf("createMariaDBBackupArtifact: %v", err) + } + if l.bucket != "bkt" || l.prefix != "original/" || l.conn.Endpoint != "s3.example:9000" { + t.Errorf("listed %s/%s at %s, want bkt/original/ at s3.example:9000, where the operator Backup wrote", l.bucket, l.prefix, l.conn.Endpoint) + } + if artefact.Status.Artifact == nil || artefact.Status.Artifact.SizeBytes != 2048 { + t.Errorf("Artifact = %#v, want SizeBytes 2048", artefact.Status.Artifact) + } +} + func TestCreateMariaDBBackupArtifact_ExistingBackupSkipsProbe(t *testing.T) { apps := mariadbapp.GroupName job := &backupsv1alpha1.BackupJob{ diff --git a/internal/backupcontroller/mariadbstrategy_controller.go b/internal/backupcontroller/mariadbstrategy_controller.go index 749d63c9c..4d88ab8a1 100644 --- a/internal/backupcontroller/mariadbstrategy_controller.go +++ b/internal/backupcontroller/mariadbstrategy_controller.go @@ -398,7 +398,7 @@ func (r *BackupJobReconciler) createMariaDBBackupArtifact( if uri := driverMD[mariadbStorageURIKey]; uri != "" { status.Artifact = &backupsv1alpha1.BackupArtifact{URI: uri} if probe { - status.Artifact.SizeBytes = r.mariadbArtifactSizeBytes(ctx, j.Namespace, rendered.Storage.S3, mdbBackup, takenAt.Time) + status.Artifact.SizeBytes = r.mariadbArtifactSizeBytes(ctx, mdbBackup, takenAt.Time) } } backup := &backupsv1alpha1.Backup{