Skip to content

Commit bf152ba

Browse files
IvanHuntersIvanHunters
committed
fix(backupstrategy-controller): resolve the default BSL through the RESTMapper
The BSL existence check used a hardcoded GVR and the dynamic client, whose Get returns a plain 404 (IsNotFound) for an unserved API group. The IsNoMatchError branch meant to skip an absent Velero API was therefore dead code, and if Velero were uninstalled after bootstrap while bslEnabled=true the BSL would be counted missing and force a Helm upgrade forever. Route the lookup through the RESTMapper like the strategy loop, so an absent velero.io API is a NoMatch we skip. A regression test fails without the change. Signed-off-by: IvanHunters <[email protected]>
1 parent aa3accd commit bf152ba

2 files changed

Lines changed: 67 additions & 5 deletions

File tree

‎internal/backupcontroller/default_objects_gate.go‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -364,15 +364,28 @@ func (g *DefaultObjectsGate) missingObjects(ctx context.Context, backupClass *ba
364364
}
365365

366366
if g.VeleroNamespace != "" {
367-
_, err := g.Resource(backupStorageLocationGVR).Namespace(g.VeleroNamespace).Get(ctx, defaultBackupStorageLocationName, metav1.GetOptions{})
367+
// Resolve through the RESTMapper, like the strategy loop above, so a
368+
// genuinely absent Velero API is a NoMatch we skip rather than a
369+
// NotFound we count. The dynamic client's Get bypasses the mapper and
370+
// returns a plain 404 for an unserved group, which IsNotFound would
371+
// catch first, making the "CRDs absent" branch dead code and looping
372+
// the gate forever if Velero were removed while bslEnabled=true.
373+
mapping, err := g.RESTMapping(schema.GroupKind{Group: backupStorageLocationGVR.Group, Kind: "BackupStorageLocation"})
368374
switch {
369375
case err == nil:
370-
case apierrors.IsNotFound(err):
371-
missing = append(missing, fmt.Sprintf("BackupStorageLocation/%s", defaultBackupStorageLocationName))
376+
_, getErr := g.Resource(mapping.Resource).Namespace(g.VeleroNamespace).Get(ctx, defaultBackupStorageLocationName, metav1.GetOptions{})
377+
switch {
378+
case getErr == nil:
379+
case apierrors.IsNotFound(getErr):
380+
missing = append(missing, fmt.Sprintf("BackupStorageLocation/%s", defaultBackupStorageLocationName))
381+
default:
382+
return nil, fmt.Errorf("get BackupStorageLocation %s: %w", defaultBackupStorageLocationName, getErr)
383+
}
372384
case meta.IsNoMatchError(err):
373-
// Velero CRDs absent: nothing to materialise.
385+
// Velero API not served (e.g. Velero uninstalled after bootstrap):
386+
// no Helm re-render can create the BSL, so it is not "missing".
374387
default:
375-
return nil, fmt.Errorf("get BackupStorageLocation %s: %w", defaultBackupStorageLocationName, err)
388+
return nil, fmt.Errorf("map BackupStorageLocation: %w", err)
376389
}
377390
}
378391

‎internal/backupcontroller/default_objects_gate_test.go‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -410,6 +410,55 @@ func TestMissingObjectsSkipsVeleroWhenNamespaceEmpty(t *testing.T) {
410410
}
411411
}
412412

413+
// TestMissingObjectsSkipsBSLWhenVeleroAPIAbsent covers Velero being removed
414+
// after bootstrap while bslEnabled=true: the velero.io API is no longer
415+
// served, so the BSL cannot be re-rendered and must not be counted as
416+
// missing. Resolving through the RESTMapper turns that into a NoMatch skip;
417+
// the previous hardcoded-GVR Get returned a NotFound that would have looped
418+
// the gate forever.
419+
func TestMissingObjectsSkipsBSLWhenVeleroAPIAbsent(t *testing.T) {
420+
bc := cozyDefaultBackupClass()
421+
g, _ := newGate(t, []client.Object{sourceSecret("b"), bc},
422+
helmReleaseObject(),
423+
strategyObject("CNPG", "cozy-default-cnpg"),
424+
strategyObject("Etcd", "cozy-default-etcd"),
425+
strategyObject("Velero", "cozy-default-velero-vminstance"),
426+
strategyObject("Velero", "cozy-default-velero-vmdisk"),
427+
)
428+
// Rebuild the RESTMapper without BackupStorageLocation: the velero.io API
429+
// is not served on this cluster.
430+
gvset := map[schema.GroupVersion]struct{}{}
431+
var kept []schema.GroupVersionKind
432+
for _, gvk := range gateGVKs {
433+
if gvk.Kind == "BackupStorageLocation" {
434+
continue
435+
}
436+
gvset[gvk.GroupVersion()] = struct{}{}
437+
kept = append(kept, gvk)
438+
}
439+
var gvs []schema.GroupVersion
440+
for gv := range gvset {
441+
gvs = append(gvs, gv)
442+
}
443+
m := meta.NewDefaultRESTMapper(gvs)
444+
for _, gvk := range kept {
445+
scope := meta.RESTScopeRoot
446+
if gvk.Kind == "HelmRelease" {
447+
scope = meta.RESTScopeNamespace
448+
}
449+
m.Add(gvk, scope)
450+
}
451+
g.RESTMapper = m
452+
453+
missing, err := g.missingObjects(context.Background(), bc)
454+
if err != nil {
455+
t.Fatalf("missingObjects: %v", err)
456+
}
457+
if len(missing) != 0 {
458+
t.Fatalf("missing = %v, want none when the Velero API is absent", missing)
459+
}
460+
}
461+
413462
// TestCheckSurfacesPatchFailure pins that a failed force is reported rather
414463
// than silently recorded as done — and that lastForce is NOT advanced, so
415464
// the next tick retries instead of waiting out MinForceInterval.

0 commit comments

Comments
 (0)