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 <androndo@gmail.com>

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

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 {

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