Skip to content

Commit 6f3d26c

Browse files
androndoandrondo
committed
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 <[email protected]>
1 parent d61a505 commit 6f3d26c

3 files changed

Lines changed: 74 additions & 24 deletions

File tree

‎internal/backupcontroller/redis_artifact_size.go‎

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -17,43 +17,49 @@ import (
1717
// an Artifact carrying its s3:// URI and size. The default Redis strategy
1818
// takes its S3 coordinates from the projected backup credentials Secret, so
1919
// the object is looked up there, with the TLS trust the Job's pod applied
20-
// (see redisS3Trust). Returns nil - leaving Status.Artifact absent - when
21-
// the Secret cannot be read or the object is not found: a custom strategy
22-
// that writes elsewhere must not get a URI pointing at an object that does
23-
// not exist.
24-
func (r *BackupJobReconciler) redisArtifact(ctx context.Context, namespace, objectKey string, podSpec *corev1.PodSpec) *backupsv1alpha1.BackupArtifact {
25-
logger := getLogger(ctx)
20+
// (see redisS3Trust). It returns an artifactProbeError, and no Artifact,
21+
// when the Secret cannot be read or the object is not found: a custom
22+
// strategy that writes elsewhere must not get a URI pointing at an object
23+
// that does not exist.
24+
func (r *BackupJobReconciler) redisArtifact(ctx context.Context, namespace, objectKey string, podSpec *corev1.PodSpec) (*backupsv1alpha1.BackupArtifact, error) {
2625
ctx, cancel := context.WithTimeout(ctx, artifactSizeProbeTimeout)
2726
defer cancel()
2827

2928
bucket, conn, err := r.projectedS3Connection(ctx, namespace)
3029
if err != nil {
31-
logger.Info("cannot resolve backup credentials to locate the Redis snapshot", "objectKey", objectKey, "error", err)
32-
return nil
30+
return nil, &artifactProbeError{reason: "the backup credentials Secret is unusable", err: err}
3331
}
3432
if conn.CAPEM, conn.InsecureSkipVerify, err = r.redisS3Trust(ctx, namespace, podSpec); err != nil {
35-
logger.Info("cannot read the S3 endpoint CA to locate the Redis snapshot", "objectKey", objectKey, "error", err)
36-
return nil
33+
return nil, &artifactProbeError{reason: "the S3 endpoint CA the Job mounted cannot be read", err: err}
3734
}
3835
// The script passes the CA as curl --cacert, which replaces the default
3936
// trust store rather than adding to it.
4037
conn.CAOnly = true
4138
lister, err := r.s3Lister(conn)
4239
if err != nil {
43-
logger.Info("cannot build S3 client to locate the Redis snapshot", "objectKey", objectKey, "error", err)
44-
return nil
40+
return nil, &artifactProbeError{reason: "the S3 endpoint is not usable", err: err}
4541
}
4642
size, err := lister.StatObject(ctx, bucket, objectKey)
4743
if err != nil {
48-
logger.Info("cannot stat the Redis snapshot; leaving the artifact unset", "bucket", bucket, "objectKey", objectKey, "error", err)
49-
return nil
44+
return nil, &artifactProbeError{reason: "the snapshot could not be found or read in the bucket", err: err}
5045
}
5146
return &backupsv1alpha1.BackupArtifact{
5247
URI: fmt.Sprintf("s3://%s/%s", bucket, objectKey),
5348
SizeBytes: size,
54-
}
49+
}, nil
50+
}
51+
52+
// artifactProbeError carries a fixed reason fit for an Event in the
53+
// application namespace; err may name the platform's endpoint and bucket,
54+
// so it goes to the controller log only.
55+
type artifactProbeError struct {
56+
reason string
57+
err error
5558
}
5659

60+
func (e *artifactProbeError) Error() string { return e.reason + ": " + e.err.Error() }
61+
func (e *artifactProbeError) Unwrap() error { return e.err }
62+
5763
// projectedS3Connection reads the backup credentials Secret the controller
5864
// projects into the application namespace. The projector stores the endpoint
5965
// without a scheme, and the strategy scripts reach it over https. It can

‎internal/backupcontroller/redis_artifact_size_test.go‎

Lines changed: 43 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
batchv1 "k8s.io/api/batch/v1"
1515
corev1 "k8s.io/api/core/v1"
1616
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
17+
"k8s.io/client-go/tools/record"
1718
"sigs.k8s.io/controller-runtime/pkg/client"
1819
clientfake "sigs.k8s.io/controller-runtime/pkg/client/fake"
1920

@@ -56,7 +57,7 @@ func TestRedisArtifact(t *testing.T) {
5657
t.Run("stats the snapshot with the projected credentials", func(t *testing.T) {
5758
l := &fakeS3Lister{statSize: 2048}
5859
r := redisReconciler(t, l, projectedCredsSecret(nil))
59-
got := r.redisArtifact(context.Background(), "tenant", redisTestKey, nil)
60+
got, _ := r.redisArtifact(context.Background(), "tenant", redisTestKey, nil)
6061
if got == nil || got.URI != "s3://cozy-backups/"+redisTestKey || got.SizeBytes != 2048 {
6162
t.Fatalf("artifact = %#v", got)
6263
}
@@ -87,7 +88,7 @@ func TestRedisArtifact(t *testing.T) {
8788
if tc.noTarget {
8889
r.CredentialsConfig.TargetSecretName = ""
8990
}
90-
if got := r.redisArtifact(context.Background(), "tenant", redisTestKey, nil); got != nil {
91+
if got, _ := r.redisArtifact(context.Background(), "tenant", redisTestKey, nil); got != nil {
9192
t.Errorf("artifact = %#v, want nil", got)
9293
}
9394
})
@@ -124,11 +125,11 @@ func TestRedisArtifact_MinioClient(t *testing.T) {
124125
r := &BackupJobReconciler{Client: c, Scheme: c.Scheme()}
125126
r.CredentialsConfig.TargetSecretName = "cozy-backups-creds"
126127

127-
if got := r.redisArtifact(context.Background(), "tenant", redisTestKey, &corev1.PodSpec{}); got != nil {
128+
if got, _ := r.redisArtifact(context.Background(), "tenant", redisTestKey, &corev1.PodSpec{}); got != nil {
128129
t.Errorf("without the Job's trust settings: artifact = %#v, want nil (untrusted certificate)", got)
129130
}
130131

131-
got := r.redisArtifact(context.Background(), "tenant", redisTestKey, redisPodSpec("s3-endpoint-ca", ""))
132+
got, _ := r.redisArtifact(context.Background(), "tenant", redisTestKey, redisPodSpec("s3-endpoint-ca", ""))
132133
if got == nil || got.SizeBytes != 4096 {
133134
t.Fatalf("with the s3-ca volume: artifact = %#v, want SizeBytes 4096", got)
134135
}
@@ -139,15 +140,15 @@ func TestRedisArtifact_MinioClient(t *testing.T) {
139140
t.Errorf("request not signed with the Secret's access key: %q", gotAuth)
140141
}
141142

142-
if got := r.redisArtifact(context.Background(), "tenant", redisTestKey, redisPodSpec("", "true")); got == nil || got.SizeBytes != 4096 {
143+
if got, _ := r.redisArtifact(context.Background(), "tenant", redisTestKey, redisPodSpec("", "true")); got == nil || got.SizeBytes != 4096 {
143144
t.Errorf("with S3_INSECURE_SKIP_VERIFY=true: artifact = %#v, want SizeBytes 4096", got)
144145
}
145146

146147
ca.Data["ca.crt"] = unrelatedCAPEM(t)
147148
if err := c.Update(context.Background(), ca); err != nil {
148149
t.Fatalf("update CA Secret: %v", err)
149150
}
150-
if got := r.redisArtifact(context.Background(), "tenant", redisTestKey, redisPodSpec("s3-endpoint-ca", "true")); got != nil {
151+
if got, _ := r.redisArtifact(context.Background(), "tenant", redisTestKey, redisPodSpec("s3-endpoint-ca", "true")); got != nil {
151152
t.Errorf("with a CA that did not sign the endpoint and S3_INSECURE_SKIP_VERIFY=true: artifact = %#v, want nil", got)
152153
}
153154
}
@@ -314,6 +315,40 @@ func TestCreateRedisBackupArtifact_Artifact(t *testing.T) {
314315
}
315316
})
316317

318+
t.Run("an unmeasured artifact is reported on the BackupJob without the raw error", func(t *testing.T) {
319+
l := &fakeS3Lister{statErr: errors.New("Get https://s3.internal.example/cozy-backups: NoSuchKey")}
320+
r := redisReconciler(t, l, j, projectedCredsSecret(nil))
321+
rec := record.NewFakeRecorder(4)
322+
r.Recorder = rec
323+
got, err := r.createRedisBackupArtifact(context.Background(), j, &ResolvedBackupConfig{}, redisTestKey, nil)
324+
if err != nil || got == nil || got.Status.Artifact != nil {
325+
t.Fatalf("createRedisBackupArtifact = %#v, %v; want a Backup without an artifact", got, err)
326+
}
327+
select {
328+
case ev := <-rec.Events:
329+
if !strings.HasPrefix(ev, "Normal ArtifactNotMeasured ") || !strings.Contains(ev, "could not be found or read") {
330+
t.Errorf("event = %q, want Normal ArtifactNotMeasured with the reason", ev)
331+
}
332+
if strings.Contains(ev, "s3.internal.example") {
333+
t.Errorf("event = %q leaks the raw S3 error into the application namespace", ev)
334+
}
335+
default:
336+
t.Error("no event on the BackupJob for an artifact left unset")
337+
}
338+
})
339+
340+
t.Run("a measured artifact raises no event", func(t *testing.T) {
341+
r := redisReconciler(t, &fakeS3Lister{statSize: 1}, j, projectedCredsSecret(nil))
342+
rec := record.NewFakeRecorder(4)
343+
r.Recorder = rec
344+
if _, err := r.createRedisBackupArtifact(context.Background(), j, &ResolvedBackupConfig{}, redisTestKey, nil); err != nil {
345+
t.Fatalf("createRedisBackupArtifact: %v", err)
346+
}
347+
if len(rec.Events) != 0 {
348+
t.Errorf("event %q for an artifact that was recorded", <-rec.Events)
349+
}
350+
})
351+
317352
t.Run("an existing Backup is returned without probing", func(t *testing.T) {
318353
existing := &backupsv1alpha1.Backup{ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: "bj-1"}}
319354
l := &fakeS3Lister{statSize: 777}
@@ -445,7 +480,7 @@ func TestRedisArtifact_HangingEndpointIsGivenUp(t *testing.T) {
445480
r := redisReconciler(t, &fakeS3Lister{}, projectedCredsSecret(nil))
446481
r.newS3ObjectLister = func(s3Connection) (s3ObjectLister, error) { return blockingLister{}, nil }
447482
withShortProbeTimeout(t, func() {
448-
if got := r.redisArtifact(context.Background(), "tenant", redisTestKey, nil); got != nil {
483+
if got, _ := r.redisArtifact(context.Background(), "tenant", redisTestKey, nil); got != nil {
449484
t.Errorf("artifact = %#v, want nil", got)
450485
}
451486
})
@@ -458,7 +493,7 @@ func TestRedisArtifact_TrustsOnlyTheCA(t *testing.T) {
458493
Data: map[string][]byte{"ca.crt": []byte("PEM")},
459494
}
460495
r := redisReconciler(t, l, projectedCredsSecret(nil), ca)
461-
if got := r.redisArtifact(context.Background(), "tenant", redisTestKey, redisPodSpec("s3-endpoint-ca", "")); got == nil {
496+
if got, _ := r.redisArtifact(context.Background(), "tenant", redisTestKey, redisPodSpec("s3-endpoint-ca", "")); got == nil {
462497
t.Fatal("artifact = nil")
463498
}
464499
if !l.conn.CAOnly {

‎internal/backupcontroller/redisstrategy_controller.go‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package backupcontroller
33

44
import (
55
"context"
6+
"errors"
67
"fmt"
78
"maps"
89
"strings"
@@ -498,7 +499,15 @@ func (r *BackupJobReconciler) createRedisBackupArtifact(
498499
},
499500
}
500501
if probe {
501-
backup.Status.Artifact = r.redisArtifact(ctx, j.Namespace, objectKey, podSpec)
502+
artifact, err := r.redisArtifact(ctx, j.Namespace, objectKey, podSpec)
503+
if err != nil {
504+
getLogger(ctx).Info("leaving the Redis backup artifact unset", "backupjob", j.Name, "objectKey", objectKey, "error", err)
505+
var probeErr *artifactProbeError
506+
if r.Recorder != nil && errors.As(err, &probeErr) {
507+
r.Recorder.Eventf(j, corev1.EventTypeNormal, "ArtifactNotMeasured", "The backup succeeded, but its location and size were not recorded: %s", probeErr.reason)
508+
}
509+
}
510+
backup.Status.Artifact = artifact
502511
}
503512
if j.Spec.PlanRef != nil {
504513
backup.Spec.PlanRef = j.Spec.PlanRef

0 commit comments

Comments
 (0)