Skip to content

Commit de399d3

Browse files
committed
fix(backups): adopt-guard Kafka Backup artifact and correct URI-template doc
Non-blocking review follow-ups on #4000: - createKafkaBackupArtifact now rejects adopting a same-named Backup that describes a different application, so a retained Backup reused for a different app of the same name is not reported as this run's success. Mirrors the RabbitMQ driver's guard; pinned by a Go test. - ArtifactURITemplate godoc said empty leaves status.artifact unset, but the driver hard-fails an empty template. Reword to state it is required, and regenerate the CRD description to match. Signed-off-by: Andrey Kolkov <[email protected]> Assisted-by: LLM
1 parent a9e8548 commit de399d3

4 files changed

Lines changed: 57 additions & 8 deletions

File tree

‎api/backups/strategy/v1alpha1/kafka_types.go‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -80,10 +80,13 @@ type KafkaSpec struct {
8080
// +kubebuilder:pruning:PreserveUnknownFields
8181
Template corev1.PodTemplateSpec `json:"template"`
8282

83-
// ArtifactURITemplate is rendered against the same context on a successful
84-
// backup and recorded on the produced Backup's status.artifact.uri, so the
85-
// stored object's location is introspectable. Ignored on restore; empty
86-
// leaves status.artifact unset.
83+
// ArtifactURITemplate is rendered against the same context, injected into the
84+
// Job as ARTIFACT_URI (the single source of truth for the object location),
85+
// and recorded on the produced Backup's status.artifact.uri so restore and
86+
// cleanup act on the exact object the backup wrote. The driver requires it:
87+
// reconcileKafka fails the BackupJob when it is empty, since nothing would
88+
// record where the export is stored. The shipped cozy-default-kafka strategy
89+
// always sets it; a custom Kafka strategy that stores backups must too.
8790
// +optional
8891
ArtifactURITemplate string `json:"artifactURITemplate,omitempty"`
8992
}

‎internal/backupcontroller/kafkastrategy_controller.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -409,6 +409,18 @@ func (r *BackupJobReconciler) createKafkaBackupArtifact(
409409
if getErr := r.Get(ctx, types.NamespacedName{Namespace: backup.Namespace, Name: backup.Name}, existing); getErr != nil {
410410
return nil, getErr
411411
}
412+
// Adopt an existing Backup only when it describes the same application.
413+
// A retained Backup whose BackupJob was reaped, then reused for a
414+
// different application of the same name, would otherwise be reported as
415+
// this run's success while describing something else (mirrors the
416+
// RabbitMQ driver's guard).
417+
if existing.Spec.ApplicationRef.Kind != backup.Spec.ApplicationRef.Kind ||
418+
existing.Spec.ApplicationRef.Name != backup.Spec.ApplicationRef.Name {
419+
return nil, fmt.Errorf("Backup %s/%s already exists for a different application (%s/%s, not %s/%s)",
420+
backup.Namespace, backup.Name,
421+
existing.Spec.ApplicationRef.Kind, existing.Spec.ApplicationRef.Name,
422+
backup.Spec.ApplicationRef.Kind, backup.Spec.ApplicationRef.Name)
423+
}
412424
return existing, nil
413425
}
414426
return backup, nil

‎internal/backupcontroller/kafkastrategy_controller_test.go‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -307,6 +307,37 @@ func TestCreateKafkaBackupArtifact_StampsScope(t *testing.T) {
307307
}
308308
}
309309

310+
// TestCreateKafkaBackupArtifact_RejectsDifferentApp pins the adoption guard: a
311+
// same-named Backup left over for a different application is not reported as this
312+
// run's success (mirrors the RabbitMQ driver).
313+
func TestCreateKafkaBackupArtifact_RejectsDifferentApp(t *testing.T) {
314+
testScheme := runtime.NewScheme()
315+
_ = scheme.AddToScheme(testScheme)
316+
_ = backupsv1alpha1.AddToScheme(testScheme)
317+
318+
// A retained Backup named "bj" that describes application "other".
319+
stale := &backupsv1alpha1.Backup{
320+
ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: "bj"},
321+
Spec: backupsv1alpha1.BackupSpec{ApplicationRef: kafkaAppRef("other")},
322+
}
323+
c := clientfake.NewClientBuilder().WithScheme(testScheme).
324+
WithStatusSubresource(&backupsv1alpha1.Backup{}).WithObjects(stale).Build()
325+
r := &BackupJobReconciler{Client: c, Scheme: testScheme, Recorder: record.NewFakeRecorder(10)}
326+
327+
bj := &backupsv1alpha1.BackupJob{
328+
ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: "bj"},
329+
Spec: backupsv1alpha1.BackupJobSpec{ApplicationRef: kafkaAppRef("src")},
330+
}
331+
resolved := &ResolvedBackupConfig{
332+
StrategyRef: corev1.TypedLocalObjectReference{Kind: strategyv1alpha1.KafkaStrategyKind, Name: "cozy-default-kafka"},
333+
}
334+
if _, err := r.createKafkaBackupArtifact(context.Background(), bj, resolved, "s3://bkt/tenant/src/bj/kafka-metadata.txt"); err == nil {
335+
t.Fatal("expected an error adopting a same-named Backup for a different application, got nil")
336+
} else if !strings.Contains(err.Error(), "different application") {
337+
t.Fatalf("error = %v, want it to mention a different application", err)
338+
}
339+
}
340+
310341
// kafkaBackup builds a Backup fixture that has already recorded its metadata
311342
// object, for the cleanup path (whose delete is keyed on status.artifact.uri).
312343
func kafkaBackup(name, namespace string) *backupsv1alpha1.Backup {

‎packages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_kafkas.yaml‎

Lines changed: 7 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)