From 399ccd3f4aaf2c1f7d64fccc76173021104f0717 Mon Sep 17 00:00:00 2001 From: Alexey Artamonov Date: Thu, 5 Nov 2026 21:02:16 +0300 Subject: [PATCH] fix(backups): refresh stale credentials even when the routed objects are unreadable The credentials endpoint comparison needs only the credentials Secret and the COSI Secret it is rendered from, yet its forced render was issued only after the BackupClass and every routed object had been read. A missing BackupClass or one unreadable Strategy therefore kept the old endpoint in the credentials Secret, which the projector hands to every driver, not just the one whose object failed. Assisted-by: LLM Signed-off-by: Alexey Artamonov --- .../backupcontroller/default_objects_gate.go | 25 ++++++---- .../default_objects_gate_test.go | 50 +++++++++++++++++++ 2 files changed, 64 insertions(+), 11 deletions(-) diff --git a/internal/backupcontroller/default_objects_gate.go b/internal/backupcontroller/default_objects_gate.go index 8fb58c65e..b99f67b41 100644 --- a/internal/backupcontroller/default_objects_gate.go +++ b/internal/backupcontroller/default_objects_gate.go @@ -378,17 +378,29 @@ func (g *DefaultObjectsGate) Check(ctx context.Context) ([]string, bool, error) return g.reconcileCredentials(ctx) } + // The credentials comparison needs only the two Secrets, so its force goes + // out before the object reads: an unreadable BackupClass or Strategy must + // not keep the stale endpoint in the Secret every driver reads. + var needed []string + var forced bool staleCreds, credsErr := g.staleCredentials(ctx, src) + errs := []error{credsErr} if credsErr != nil { defaultObjectsCheckErrors.WithLabelValues(g.BackupClassName).Inc() } + if staleCreds != "" { + _, credsForced, err := g.force(ctx, g.CredentialsHelmRelease, &g.lastCredentialsForce, []string{staleCreds}) + needed = append(needed, staleCreds) + forced = credsForced + errs = append(errs, err) + } backupClass := &backupsv1alpha1.BackupClass{} if err := g.Client.Get(ctx, client.ObjectKey{Name: g.BackupClassName}, backupClass); err != nil { if credsErr == nil { defaultObjectsCheckErrors.WithLabelValues(g.BackupClassName).Inc() } - return nil, false, errors.Join(credsErr, fmt.Errorf("get BackupClass %s: %w", g.BackupClassName, err)) + return needed, forced, errors.Join(append(errs, fmt.Errorf("get BackupClass %s: %w", g.BackupClassName, err))...) } // With provisionBucket=false (no bucket release) the chart renders @@ -403,19 +415,10 @@ func (g *DefaultObjectsGate) Check(ctx context.Context) ([]string, bool, error) if credsErr == nil { defaultObjectsCheckErrors.WithLabelValues(g.BackupClassName).Inc() } - return nil, false, errors.Join(credsErr, err) + return needed, forced, errors.Join(append(errs, err)...) } defaultObjectsMissing.WithLabelValues(g.BackupClassName).Set(float64(len(state.missing))) - var needed []string - var forced bool - errs := []error{credsErr} - if staleCreds != "" { - _, credsForced, err := g.force(ctx, g.CredentialsHelmRelease, &g.lastCredentialsForce, []string{staleCreds}) - needed = append(needed, staleCreds) - forced = credsForced - errs = append(errs, err) - } if objects := append(state.missing, state.stale...); len(objects) > 0 { sort.Strings(objects) _, objectsForced, err := g.force(ctx, g.HelmRelease, &g.lastForce, objects) diff --git a/internal/backupcontroller/default_objects_gate_test.go b/internal/backupcontroller/default_objects_gate_test.go index fac76e132..befaedabe 100644 --- a/internal/backupcontroller/default_objects_gate_test.go +++ b/internal/backupcontroller/default_objects_gate_test.go @@ -1283,3 +1283,53 @@ func TestCheckSurfacesCredentialsPatchFailure(t *testing.T) { t.Error("lastCredentialsForce advanced despite the patch failing") } } + +// TestCheckForcesStaleCredentialsWhenObjectsUnreadable pins that the +// credentials comparison needs only the two Secrets: a BackupClass or a routed +// object the gate cannot read must not keep the stale endpoint in the +// credentials Secret, which every other driver reads through the projector. +func TestCheckForcesStaleCredentialsWhenObjectsUnreadable(t *testing.T) { + for _, tc := range []struct { + name string + backupClass bool + forbidCNPG bool + }{ + {"BackupClass absent", false, false}, + {"routed Strategy unreadable", true, true}, + } { + t.Run(tc.name, func(t *testing.T) { + defaultObjectsCheckErrors.Reset() + ctrlObjs := []client.Object{sourceSecret("bucket-1a2b"), cosiSecret("https://s3.new.example.com")} + if tc.backupClass { + ctrlObjs = append(ctrlObjs, cozyDefaultBackupClass()) + } + objs := append([]runtime.Object{helmReleaseObject(), credentialsHelmReleaseObject()}, allDefaultObjects()...) + g, dyn := newGate(t, ctrlObjs, objs...) + if tc.forbidCNPG { + dyn.PrependReactor("get", "cnpgs", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, apierrors.NewForbidden(schema.GroupResource{Group: strategyAPIGroup, Resource: "cnpgs"}, "cozy-default-cnpg", nil) + }) + } + + missing, forced, err := g.Check(context.Background()) + if err == nil { + t.Fatal("Check succeeded although the routed objects could not be evaluated") + } + if !forced { + t.Error("forced = false, want the bucket release forced for its stale credentials Secret") + } + if ann := credentialsForceAnnotations(t, dyn); ann["reconcile.fluxcd.io/forceAt"] == "" { + t.Errorf("bucket release not forced: %v", ann) + } + if ann := forceAnnotations(t, dyn); len(ann) != 0 { + t.Errorf("own HelmRelease forced without a conclusive object check: %v", ann) + } + if len(missing) != 1 || !strings.HasPrefix(missing[0], "Secret/bucket-cozy-backups-system-credentials") { + t.Errorf("missing = %v, want the credentials Secret", missing) + } + if got := testutil.ToFloat64(defaultObjectsCheckErrors.WithLabelValues("cozy-default")); got != 1 { + t.Errorf("check errors = %v, want 1", got) + } + }) + } +}