From 2b5fa1f0ebfe154090f92052c788b9e00fd4e072 Mon Sep 17 00:00:00 2001 From: Andrey Kolkov Date: Wed, 21 Oct 2026 19:11:09 +0400 Subject: [PATCH] feat(backups): record the RDB snapshot location and size for Redis backups Backups made by the Redis strategy, which also serves Valkey, carried no Backup.status.artifact at all: neither a URI nor a size. The snapshot is one object per backup, //.rdb, whose key the driver already records, and the default strategy uploads it with the S3 coordinates of the backup credentials Secret the controller projects into the application namespace. On completion the driver now stats that object with the same coordinates and records its s3:// URI and size. The artifact is set only when the object is found, so a custom strategy that writes elsewhere gets no URI pointing at a missing object. A Backup that already exists is returned without probing, as for the MariaDB and CNPG drivers. Assisted-by: LLM Signed-off-by: Andrey Kolkov --- internal/backupcontroller/artifact_size.go | 9 + .../backupcontroller/artifact_size_test.go | 15 ++ .../backupcontroller/redis_artifact_size.go | 80 +++++++++ .../redis_artifact_size_test.go | 166 ++++++++++++++++++ .../redisstrategy_controller.go | 13 +- 5 files changed, 280 insertions(+), 3 deletions(-) create mode 100644 internal/backupcontroller/redis_artifact_size.go create mode 100644 internal/backupcontroller/redis_artifact_size_test.go diff --git a/internal/backupcontroller/artifact_size.go b/internal/backupcontroller/artifact_size.go index 941b00323..dc23542b9 100644 --- a/internal/backupcontroller/artifact_size.go +++ b/internal/backupcontroller/artifact_size.go @@ -36,6 +36,7 @@ type s3ObjectInfo struct { type s3ObjectLister interface { ListObjects(ctx context.Context, bucket, prefix string, recursive bool) ([]s3ObjectInfo, error) + StatObject(ctx context.Context, bucket, key string) (int64, error) } // s3Connection is what a driver resolves from its rendered strategy to @@ -148,6 +149,14 @@ func (l *minioObjectLister) ListObjects(ctx context.Context, bucket, prefix stri return collectObjects(ctx, l.client.ListObjects(ctx, bucket, minio.ListObjectsOptions{Prefix: prefix, Recursive: recursive})) } +func (l *minioObjectLister) StatObject(ctx context.Context, bucket, key string) (int64, error) { + info, err := l.client.StatObject(ctx, bucket, key, minio.StatObjectOptions{}) + if err != nil { + return 0, err + } + return info.Size, nil +} + // collectObjects drains a minio listing. minio-go ends a listing whose // context is cancelled between pages by closing the channel without an // error item, so a cancelled context is checked once the channel closes: diff --git a/internal/backupcontroller/artifact_size_test.go b/internal/backupcontroller/artifact_size_test.go index 3d9c35b23..55073e742 100644 --- a/internal/backupcontroller/artifact_size_test.go +++ b/internal/backupcontroller/artifact_size_test.go @@ -37,6 +37,16 @@ type fakeS3Lister struct { bucket, prefix string recursive bool calls int + + statSize int64 + statErr error + statKey string +} + +func (f *fakeS3Lister) StatObject(_ context.Context, bucket, key string) (int64, error) { + f.calls++ + f.bucket, f.statKey = bucket, key + return f.statSize, f.statErr } func (f *fakeS3Lister) ListObjects(_ context.Context, bucket, prefix string, recursive bool) ([]s3ObjectInfo, error) { @@ -167,6 +177,11 @@ func (blockingLister) ListObjects(ctx context.Context, _, _ string, _ bool) ([]s return nil, ctx.Err() } +func (blockingLister) StatObject(ctx context.Context, _, _ string) (int64, error) { + <-ctx.Done() + return 0, ctx.Err() +} + // withShortProbeTimeout shortens the probe timeout and fails the test if fn // is not back well before the default timeout would have expired. func withShortProbeTimeout(t *testing.T, fn func()) { diff --git a/internal/backupcontroller/redis_artifact_size.go b/internal/backupcontroller/redis_artifact_size.go new file mode 100644 index 000000000..0e76d0b9c --- /dev/null +++ b/internal/backupcontroller/redis_artifact_size.go @@ -0,0 +1,80 @@ +// SPDX-License-Identifier: Apache-2.0 +package backupcontroller + +import ( + "context" + "errors" + "fmt" + "net/url" + "strings" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" + + backupsv1alpha1 "github.com/cozystack/cozystack/api/backups/v1alpha1" +) + +// redisArtifact locates the RDB snapshot the backup Job uploaded and returns +// an Artifact carrying its s3:// URI and size. The default Redis strategy +// takes its S3 coordinates from the projected backup credentials Secret, so +// the object is looked up there. Returns nil - leaving Status.Artifact +// absent - when the Secret cannot be read or the object is not found: a +// custom strategy that writes elsewhere must not get a URI pointing at an +// object that does not exist. +func (r *BackupJobReconciler) redisArtifact(ctx context.Context, namespace, objectKey string) *backupsv1alpha1.BackupArtifact { + logger := getLogger(ctx) + ctx, cancel := context.WithTimeout(ctx, artifactSizeProbeTimeout) + defer cancel() + + bucket, conn, err := r.projectedS3Connection(ctx, namespace) + if err != nil { + logger.Info("cannot resolve backup credentials to locate the Redis snapshot", "objectKey", objectKey, "error", err) + return nil + } + lister, err := r.s3Lister(conn) + if err != nil { + logger.Info("cannot build S3 client to locate the Redis snapshot", "objectKey", objectKey, "error", err) + return nil + } + size, err := lister.StatObject(ctx, bucket, objectKey) + if err != nil { + logger.Info("cannot stat the Redis snapshot; leaving the artifact unset", "bucket", bucket, "objectKey", objectKey, "error", err) + return nil + } + return &backupsv1alpha1.BackupArtifact{ + URI: fmt.Sprintf("s3://%s/%s", bucket, objectKey), + SizeBytes: size, + } +} + +// projectedS3Connection reads the backup credentials Secret the controller +// projects into the application namespace. Its endpoint is a bare host, +// which the strategy scripts reach over https. +func (r *BackupJobReconciler) projectedS3Connection(ctx context.Context, namespace string) (string, s3Connection, error) { + var conn s3Connection + name := r.CredentialsConfig.TargetSecretName + if name == "" { + return "", conn, errors.New("no projected credentials Secret is configured") + } + secret := &corev1.Secret{} + if err := r.Get(ctx, types.NamespacedName{Namespace: namespace, Name: name}, secret); err != nil { + return "", conn, err + } + value := func(key string) string { return string(secret.Data[key]) } + + bucket := value("bucketName") + conn.AccessKey, conn.SecretKey = value("AWS_ACCESS_KEY_ID"), value("AWS_SECRET_ACCESS_KEY") + if bucket == "" || value("endpoint") == "" || conn.AccessKey == "" || conn.SecretKey == "" { + return "", conn, fmt.Errorf("secret %s/%s lacks bucketName, endpoint or access keys", namespace, name) + } + conn.Endpoint, conn.Secure = value("endpoint"), true + if strings.Contains(conn.Endpoint, "://") { + u, err := url.Parse(conn.Endpoint) + if err != nil || u.Host == "" { + return "", conn, fmt.Errorf("invalid endpoint %q in secret %s/%s", conn.Endpoint, namespace, name) + } + conn.Endpoint, conn.Secure = u.Host, u.Scheme == "https" + } + conn.Region = value("region") + return bucket, conn, nil +} diff --git a/internal/backupcontroller/redis_artifact_size_test.go b/internal/backupcontroller/redis_artifact_size_test.go new file mode 100644 index 000000000..458726b67 --- /dev/null +++ b/internal/backupcontroller/redis_artifact_size_test.go @@ -0,0 +1,166 @@ +// SPDX-License-Identifier: Apache-2.0 +package backupcontroller + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + backupsv1alpha1 "github.com/cozystack/cozystack/api/backups/v1alpha1" +) + +const redisTestKey = "tenant/cache/bj-1.rdb" + +func projectedCredsSecret(data map[string]string) *corev1.Secret { + s := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: "cozy-backups-creds"}, + Data: map[string][]byte{}, + } + for k, v := range map[string]string{ + "endpoint": "s3.example.org", + "bucketName": "cozy-backups", + "region": "us-east-1", + "AWS_ACCESS_KEY_ID": "AKID", + "AWS_SECRET_ACCESS_KEY": "secret", + } { + s.Data[k] = []byte(v) + } + for k, v := range data { + if v == "" { + delete(s.Data, k) + continue + } + s.Data[k] = []byte(v) + } + return s +} + +func redisReconciler(t *testing.T, l *fakeS3Lister, objs ...client.Object) *BackupJobReconciler { + r := reconcilerWithLister(t, l, objs...) + r.CredentialsConfig.TargetSecretName = "cozy-backups-creds" + return r +} + +func TestRedisArtifact(t *testing.T) { + t.Run("stats the snapshot with the projected credentials", func(t *testing.T) { + l := &fakeS3Lister{statSize: 2048} + r := redisReconciler(t, l, projectedCredsSecret(nil)) + got := r.redisArtifact(context.Background(), "tenant", redisTestKey) + if got == nil || got.URI != "s3://cozy-backups/"+redisTestKey || got.SizeBytes != 2048 { + t.Fatalf("artifact = %#v", got) + } + if l.bucket != "cozy-backups" || l.statKey != redisTestKey { + t.Errorf("stat %q/%q, want cozy-backups/%q", l.bucket, l.statKey, redisTestKey) + } + if l.conn.Endpoint != "s3.example.org" || !l.conn.Secure || l.conn.AccessKey != "AKID" || l.conn.Region != "us-east-1" { + t.Errorf("connection = %+v", l.conn) + } + }) + + t.Run("an endpoint with a scheme sets the transport", func(t *testing.T) { + l := &fakeS3Lister{statSize: 1} + r := redisReconciler(t, l, projectedCredsSecret(map[string]string{"endpoint": "http://seaweedfs:8333"})) + if got := r.redisArtifact(context.Background(), "tenant", redisTestKey); got == nil { + t.Fatal("artifact = nil") + } + if l.conn.Endpoint != "seaweedfs:8333" || l.conn.Secure { + t.Errorf("connection = %+v", l.conn) + } + }) + + unset := map[string]struct { + lister *fakeS3Lister + objs []client.Object + noTarget bool + }{ + "missing object": {lister: &fakeS3Lister{statErr: errors.New("NoSuchKey")}, objs: []client.Object{projectedCredsSecret(nil)}}, + "missing Secret": {lister: &fakeS3Lister{statSize: 1}}, + "missing bucket": {lister: &fakeS3Lister{statSize: 1}, objs: []client.Object{projectedCredsSecret(map[string]string{"bucketName": ""})}}, + "missing keys": {lister: &fakeS3Lister{statSize: 1}, objs: []client.Object{projectedCredsSecret(map[string]string{"AWS_SECRET_ACCESS_KEY": ""})}}, + "no target name": {lister: &fakeS3Lister{statSize: 1}, objs: []client.Object{projectedCredsSecret(nil)}, noTarget: true}, + "broken endpoint": {lister: &fakeS3Lister{statSize: 1}, objs: []client.Object{projectedCredsSecret(map[string]string{"endpoint": "https://"})}}, + } + for name, tc := range unset { + t.Run(name+" leaves the artifact unset", func(t *testing.T) { + r := redisReconciler(t, tc.lister, tc.objs...) + if tc.noTarget { + r.CredentialsConfig.TargetSecretName = "" + } + if got := r.redisArtifact(context.Background(), "tenant", redisTestKey); got != nil { + t.Errorf("artifact = %#v, want nil", got) + } + }) + } +} + +// TestRedisArtifact_MinioClient drives the real minio client against a stub +// that answers the HEAD request, so addressing, signing and the +// Content-Length parsing are exercised together. +func TestRedisArtifact_MinioClient(t *testing.T) { + var gotMethod, gotPath, gotAuth string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + gotMethod, gotPath, gotAuth = req.Method, req.URL.Path, req.Header.Get("Authorization") + w.Header().Set("Content-Length", "4096") + w.Header().Set("ETag", `"abc"`) + w.Header().Set("Last-Modified", "Wed, 21 Oct 2026 10:00:00 GMT") + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + creds := projectedCredsSecret(map[string]string{"endpoint": srv.URL}) + c := newMariaDBStrategyTestClient(t, creds) + r := &BackupJobReconciler{Client: c, Scheme: c.Scheme()} + r.CredentialsConfig.TargetSecretName = "cozy-backups-creds" + + got := r.redisArtifact(context.Background(), "tenant", redisTestKey) + if got == nil || got.SizeBytes != 4096 { + t.Fatalf("artifact = %#v, want SizeBytes 4096", got) + } + if gotMethod != http.MethodHead || gotPath != "/cozy-backups/"+redisTestKey { + t.Errorf("request %s %s, want HEAD /cozy-backups/%s", gotMethod, gotPath, redisTestKey) + } + if !strings.Contains(gotAuth, "Credential=AKID/") { + t.Errorf("request not signed with the Secret's access key: %q", gotAuth) + } +} + +func TestCreateRedisBackupArtifact_Artifact(t *testing.T) { + apiGroup := backupsv1alpha1.DefaultApplicationAPIGroup + j := &backupsv1alpha1.BackupJob{ + ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: "bj-1"}, + Spec: backupsv1alpha1.BackupJobSpec{ + ApplicationRef: corev1.TypedLocalObjectReference{APIGroup: &apiGroup, Kind: "Redis", Name: "cache"}, + }, + } + + t.Run("records the snapshot location and size", func(t *testing.T) { + l := &fakeS3Lister{statSize: 777} + r := redisReconciler(t, l, j, projectedCredsSecret(nil)) + got, err := r.createRedisBackupArtifact(context.Background(), j, &ResolvedBackupConfig{}, redisTestKey) + if err != nil { + t.Fatalf("createRedisBackupArtifact: %v", err) + } + if got.Status.Artifact == nil || got.Status.Artifact.SizeBytes != 777 || got.Status.Artifact.URI != "s3://cozy-backups/"+redisTestKey { + t.Fatalf("Artifact = %#v", got.Status.Artifact) + } + }) + + t.Run("an existing Backup is returned without probing", func(t *testing.T) { + existing := &backupsv1alpha1.Backup{ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: "bj-1"}} + l := &fakeS3Lister{statSize: 777} + r := redisReconciler(t, l, j, existing, projectedCredsSecret(nil)) + if _, err := r.createRedisBackupArtifact(context.Background(), j, &ResolvedBackupConfig{}, redisTestKey); err != nil { + t.Fatalf("createRedisBackupArtifact: %v", err) + } + if l.calls != 0 { + t.Errorf("S3 probed %d times for a Backup that already exists", l.calls) + } + }) +} diff --git a/internal/backupcontroller/redisstrategy_controller.go b/internal/backupcontroller/redisstrategy_controller.go index 7d287d385..40f3a99aa 100644 --- a/internal/backupcontroller/redisstrategy_controller.go +++ b/internal/backupcontroller/redisstrategy_controller.go @@ -368,7 +368,7 @@ func (r *BackupJobReconciler) reconcileRedis(ctx context.Context, j *backupsv1al } artifact, err := r.createRedisBackupArtifact(ctx, j, resolved, objectKey) if err != nil { - return r.markBackupJobFailed(ctx, j, fmt.Sprintf("failed to create Backup artifact: %v", err)) + return r.failBackupArtifact(ctx, j, err) } now := metav1.Now() j.Status.BackupRef = &corev1.LocalObjectReference{Name: artifact.Name} @@ -472,6 +472,10 @@ func (r *BackupJobReconciler) createRedisBackupArtifact( resolved *ResolvedBackupConfig, objectKey string, ) (*backupsv1alpha1.Backup, error) { + existing, probe := r.existingBackup(ctx, j.Namespace, j.Name) + if existing != nil { + return existing, nil + } driverMD := map[string]string{redisObjectMetaKey: objectKey} for k, v := range resolved.Parameters { driverMD[redisParamPrefix+k] = v @@ -492,6 +496,9 @@ func (r *BackupJobReconciler) createRedisBackupArtifact( Phase: backupsv1alpha1.BackupPhaseReady, }, } + if probe { + backup.Status.Artifact = r.redisArtifact(ctx, j.Namespace, objectKey) + } if j.Spec.PlanRef != nil { backup.Spec.PlanRef = j.Spec.PlanRef } @@ -500,8 +507,8 @@ func (r *BackupJobReconciler) createRedisBackupArtifact( return nil, err } existing := &backupsv1alpha1.Backup{} - if getErr := r.Get(ctx, types.NamespacedName{Namespace: backup.Namespace, Name: backup.Name}, existing); getErr != nil { - return nil, getErr + if getErr := r.apiReader().Get(ctx, types.NamespacedName{Namespace: backup.Namespace, Name: backup.Name}, existing); getErr != nil { + return nil, fmt.Errorf("%w: %v", errBackupNotReadBack, getErr) } return existing, nil }