From 6f3d26c4914bb024deaeada3fd96633fa25b6f4c Mon Sep 17 00:00:00 2001 From: Andrey Kolkov Date: Tue, 10 Nov 2026 11:21:46 +0700 Subject: [PATCH] feat(backups): tell on the BackupJob why a Redis artifact was not recorded A Backup without status.artifact looked the same whether a custom strategy wrote elsewhere, the credentials Secret was unusable or the endpoint CA was gone; only the controller log told them apart. The probe now returns why it gave up, and the controller records it as a Normal ArtifactNotMeasured Event on the BackupJob. The Event lives in the application namespace, so it carries a fixed reason only; the raw error, which may name the platform's endpoint and bucket, stays in the log. Assisted-by: LLM Signed-off-by: Andrey Kolkov --- .../backupcontroller/redis_artifact_size.go | 36 +++++++------ .../redis_artifact_size_test.go | 51 ++++++++++++++++--- .../redisstrategy_controller.go | 11 +++- 3 files changed, 74 insertions(+), 24 deletions(-) diff --git a/internal/backupcontroller/redis_artifact_size.go b/internal/backupcontroller/redis_artifact_size.go index c49ee3960..49a73d292 100644 --- a/internal/backupcontroller/redis_artifact_size.go +++ b/internal/backupcontroller/redis_artifact_size.go @@ -17,43 +17,49 @@ import ( // 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, with the TLS trust the Job's pod applied -// (see redisS3Trust). 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, podSpec *corev1.PodSpec) *backupsv1alpha1.BackupArtifact { - logger := getLogger(ctx) +// (see redisS3Trust). It returns an artifactProbeError, and no Artifact, +// 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, podSpec *corev1.PodSpec) (*backupsv1alpha1.BackupArtifact, error) { 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 + return nil, &artifactProbeError{reason: "the backup credentials Secret is unusable", err: err} } if conn.CAPEM, conn.InsecureSkipVerify, err = r.redisS3Trust(ctx, namespace, podSpec); err != nil { - logger.Info("cannot read the S3 endpoint CA to locate the Redis snapshot", "objectKey", objectKey, "error", err) - return nil + return nil, &artifactProbeError{reason: "the S3 endpoint CA the Job mounted cannot be read", err: err} } // The script passes the CA as curl --cacert, which replaces the default // trust store rather than adding to it. conn.CAOnly = true 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 + return nil, &artifactProbeError{reason: "the S3 endpoint is not usable", err: err} } 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 nil, &artifactProbeError{reason: "the snapshot could not be found or read in the bucket", err: err} } return &backupsv1alpha1.BackupArtifact{ URI: fmt.Sprintf("s3://%s/%s", bucket, objectKey), SizeBytes: size, - } + }, nil } +// artifactProbeError carries a fixed reason fit for an Event in the +// application namespace; err may name the platform's endpoint and bucket, +// so it goes to the controller log only. +type artifactProbeError struct { + reason string + err error +} + +func (e *artifactProbeError) Error() string { return e.reason + ": " + e.err.Error() } +func (e *artifactProbeError) Unwrap() error { return e.err } + // projectedS3Connection reads the backup credentials Secret the controller // projects into the application namespace. The projector stores the endpoint // without a scheme, and the strategy scripts reach it over https. It can diff --git a/internal/backupcontroller/redis_artifact_size_test.go b/internal/backupcontroller/redis_artifact_size_test.go index 24050baf1..f733086e6 100644 --- a/internal/backupcontroller/redis_artifact_size_test.go +++ b/internal/backupcontroller/redis_artifact_size_test.go @@ -14,6 +14,7 @@ import ( batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" clientfake "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -56,7 +57,7 @@ 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, nil) + got, _ := r.redisArtifact(context.Background(), "tenant", redisTestKey, nil) if got == nil || got.URI != "s3://cozy-backups/"+redisTestKey || got.SizeBytes != 2048 { t.Fatalf("artifact = %#v", got) } @@ -87,7 +88,7 @@ func TestRedisArtifact(t *testing.T) { if tc.noTarget { r.CredentialsConfig.TargetSecretName = "" } - if got := r.redisArtifact(context.Background(), "tenant", redisTestKey, nil); got != nil { + if got, _ := r.redisArtifact(context.Background(), "tenant", redisTestKey, nil); got != nil { t.Errorf("artifact = %#v, want nil", got) } }) @@ -124,11 +125,11 @@ func TestRedisArtifact_MinioClient(t *testing.T) { r := &BackupJobReconciler{Client: c, Scheme: c.Scheme()} r.CredentialsConfig.TargetSecretName = "cozy-backups-creds" - if got := r.redisArtifact(context.Background(), "tenant", redisTestKey, &corev1.PodSpec{}); got != nil { + if got, _ := r.redisArtifact(context.Background(), "tenant", redisTestKey, &corev1.PodSpec{}); got != nil { t.Errorf("without the Job's trust settings: artifact = %#v, want nil (untrusted certificate)", got) } - got := r.redisArtifact(context.Background(), "tenant", redisTestKey, redisPodSpec("s3-endpoint-ca", "")) + got, _ := r.redisArtifact(context.Background(), "tenant", redisTestKey, redisPodSpec("s3-endpoint-ca", "")) if got == nil || got.SizeBytes != 4096 { t.Fatalf("with the s3-ca volume: artifact = %#v, want SizeBytes 4096", got) } @@ -139,7 +140,7 @@ func TestRedisArtifact_MinioClient(t *testing.T) { t.Errorf("request not signed with the Secret's access key: %q", gotAuth) } - if got := r.redisArtifact(context.Background(), "tenant", redisTestKey, redisPodSpec("", "true")); got == nil || got.SizeBytes != 4096 { + if got, _ := r.redisArtifact(context.Background(), "tenant", redisTestKey, redisPodSpec("", "true")); got == nil || got.SizeBytes != 4096 { t.Errorf("with S3_INSECURE_SKIP_VERIFY=true: artifact = %#v, want SizeBytes 4096", got) } @@ -147,7 +148,7 @@ func TestRedisArtifact_MinioClient(t *testing.T) { if err := c.Update(context.Background(), ca); err != nil { t.Fatalf("update CA Secret: %v", err) } - if got := r.redisArtifact(context.Background(), "tenant", redisTestKey, redisPodSpec("s3-endpoint-ca", "true")); got != nil { + if got, _ := r.redisArtifact(context.Background(), "tenant", redisTestKey, redisPodSpec("s3-endpoint-ca", "true")); got != nil { t.Errorf("with a CA that did not sign the endpoint and S3_INSECURE_SKIP_VERIFY=true: artifact = %#v, want nil", got) } } @@ -314,6 +315,40 @@ func TestCreateRedisBackupArtifact_Artifact(t *testing.T) { } }) + t.Run("an unmeasured artifact is reported on the BackupJob without the raw error", func(t *testing.T) { + l := &fakeS3Lister{statErr: errors.New("Get https://s3.internal.example/cozy-backups: NoSuchKey")} + r := redisReconciler(t, l, j, projectedCredsSecret(nil)) + rec := record.NewFakeRecorder(4) + r.Recorder = rec + got, err := r.createRedisBackupArtifact(context.Background(), j, &ResolvedBackupConfig{}, redisTestKey, nil) + if err != nil || got == nil || got.Status.Artifact != nil { + t.Fatalf("createRedisBackupArtifact = %#v, %v; want a Backup without an artifact", got, err) + } + select { + case ev := <-rec.Events: + if !strings.HasPrefix(ev, "Normal ArtifactNotMeasured ") || !strings.Contains(ev, "could not be found or read") { + t.Errorf("event = %q, want Normal ArtifactNotMeasured with the reason", ev) + } + if strings.Contains(ev, "s3.internal.example") { + t.Errorf("event = %q leaks the raw S3 error into the application namespace", ev) + } + default: + t.Error("no event on the BackupJob for an artifact left unset") + } + }) + + t.Run("a measured artifact raises no event", func(t *testing.T) { + r := redisReconciler(t, &fakeS3Lister{statSize: 1}, j, projectedCredsSecret(nil)) + rec := record.NewFakeRecorder(4) + r.Recorder = rec + if _, err := r.createRedisBackupArtifact(context.Background(), j, &ResolvedBackupConfig{}, redisTestKey, nil); err != nil { + t.Fatalf("createRedisBackupArtifact: %v", err) + } + if len(rec.Events) != 0 { + t.Errorf("event %q for an artifact that was recorded", <-rec.Events) + } + }) + 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} @@ -445,7 +480,7 @@ func TestRedisArtifact_HangingEndpointIsGivenUp(t *testing.T) { r := redisReconciler(t, &fakeS3Lister{}, projectedCredsSecret(nil)) r.newS3ObjectLister = func(s3Connection) (s3ObjectLister, error) { return blockingLister{}, nil } withShortProbeTimeout(t, func() { - if got := r.redisArtifact(context.Background(), "tenant", redisTestKey, nil); got != nil { + if got, _ := r.redisArtifact(context.Background(), "tenant", redisTestKey, nil); got != nil { t.Errorf("artifact = %#v, want nil", got) } }) @@ -458,7 +493,7 @@ func TestRedisArtifact_TrustsOnlyTheCA(t *testing.T) { Data: map[string][]byte{"ca.crt": []byte("PEM")}, } r := redisReconciler(t, l, projectedCredsSecret(nil), ca) - if got := r.redisArtifact(context.Background(), "tenant", redisTestKey, redisPodSpec("s3-endpoint-ca", "")); got == nil { + if got, _ := r.redisArtifact(context.Background(), "tenant", redisTestKey, redisPodSpec("s3-endpoint-ca", "")); got == nil { t.Fatal("artifact = nil") } if !l.conn.CAOnly { diff --git a/internal/backupcontroller/redisstrategy_controller.go b/internal/backupcontroller/redisstrategy_controller.go index 76030cb10..8d57d1144 100644 --- a/internal/backupcontroller/redisstrategy_controller.go +++ b/internal/backupcontroller/redisstrategy_controller.go @@ -3,6 +3,7 @@ package backupcontroller import ( "context" + "errors" "fmt" "maps" "strings" @@ -498,7 +499,15 @@ func (r *BackupJobReconciler) createRedisBackupArtifact( }, } if probe { - backup.Status.Artifact = r.redisArtifact(ctx, j.Namespace, objectKey, podSpec) + artifact, err := r.redisArtifact(ctx, j.Namespace, objectKey, podSpec) + if err != nil { + getLogger(ctx).Info("leaving the Redis backup artifact unset", "backupjob", j.Name, "objectKey", objectKey, "error", err) + var probeErr *artifactProbeError + if r.Recorder != nil && errors.As(err, &probeErr) { + r.Recorder.Eventf(j, corev1.EventTypeNormal, "ArtifactNotMeasured", "The backup succeeded, but its location and size were not recorded: %s", probeErr.reason) + } + } + backup.Status.Artifact = artifact } if j.Spec.PlanRef != nil { backup.Spec.PlanRef = j.Spec.PlanRef