diff --git a/pkg/datamover/dataupload_delete_action.go b/pkg/datamover/dataupload_delete_action.go index 9a550a1f9..6b36e1068 100644 --- a/pkg/datamover/dataupload_delete_action.go +++ b/pkg/datamover/dataupload_delete_action.go @@ -36,17 +36,35 @@ func (d *DataUploadDeleteAction) Execute(input *velero.DeleteItemActionExecuteIn if err := runtime.DefaultUnstructuredConverter.FromUnstructured(input.Item.UnstructuredContent(), &du); err != nil { return errors.WithStack(errors.Wrapf(err, "failed to convert input.Item from unstructured")) } - // Detect DataUploads that do not belong to the backup being deleted. - // Velero does not support self-protection: the velero namespace should - // never be captured in a backup tarball. When it is (e.g. an operator - // schedule covers the velero namespace), the tarball can contain - // DataUpload CRs belonging to *other* backups. Creating a snapshot-info - // ConfigMap labeled with the executing backup's name in that case - // mislabels the snapshot and causes the real owning backup's - // deleteMovedSnapshots query to miss it, leaking the Kopia snapshot in - // the object store. Log a warning so misconfigured installs are visible, - // and skip the snapshot-info ConfigMap creation to avoid mislabeling. - if owner := du.Labels[velerov1.BackupNameLabel]; owner != "" && owner != label.GetValidName(input.Backup.Name) { + // Only create a snapshot-info ConfigMap when the DataUpload's owning + // backup (its velero.io/backup-name label) matches the backup currently + // being deleted. Two other cases reach this code path and must be + // skipped, because the resulting CM would be unmatchable and only adds + // etcd churn: + // + // 1. The label is missing. We have no verifiable owner, so a CM created + // with the executing backup's label is a guess that deleteMovedSnapshots + // cannot rely on. + // 2. The label names a different backup. Velero does not support + // self-protection, so this almost always means the velero namespace + // was captured in a backup tarball and the DataUpload CR belongs to + // an unrelated backup. Creating a CM labeled with the executing + // backup mislabels the snapshot and causes the real owning backup's + // deleteMovedSnapshots query to miss it, leaking the Kopia snapshot + // in the object store. + // + // Both cases warn so misconfigured installs surface in logs. + owner := du.Labels[velerov1.BackupNameLabel] + switch { + case owner == "": + d.logger.Warnf( + "DataUpload %q has no %q label, so its owning backup cannot be verified; "+ + "skipping snapshot-info ConfigMap creation because a CM without a verifiable owner "+ + "cannot be matched back to its snapshot at backup deletion time.", + du.Name, velerov1.BackupNameLabel, + ) + return nil + case owner != label.GetValidName(input.Backup.Name): d.logger.Warnf( "DataUpload %q belongs to backup %q but is being deleted under backup %q; "+ "this almost always means the velero namespace was included in a backup tarball. "+ diff --git a/pkg/datamover/dataupload_delete_action_test.go b/pkg/datamover/dataupload_delete_action_test.go index 35af628bb..cdbf4b006 100644 --- a/pkg/datamover/dataupload_delete_action_test.go +++ b/pkg/datamover/dataupload_delete_action_test.go @@ -83,36 +83,36 @@ func TestDataUploadDeleteActionAppliesTo(t *testing.T) { func TestDataUploadDeleteActionExecute(t *testing.T) { tests := []struct { - name string - duName string - duOwnerBackup string // value placed in velero.io/backup-name label on the DataUpload - executingBackup string // name of the Backup being deleted (input.Backup.Name) - wantConfigMap bool - wantWarn bool // whether a warn-level log about a foreign DataUpload is expected + name string + duName string + duOwnerBackup string // value placed in velero.io/backup-name label on the DataUpload + executingBackup string // name of the Backup being deleted (input.Backup.Name) + wantConfigMap bool + wantWarnContains string // substring expected in a warn-level log entry; empty means no warn expected }{ { - name: "DataUpload owned by the executing backup creates a snapshot-info ConfigMap", - duName: "daily-backup-abcde", - duOwnerBackup: "daily-backup", - executingBackup: "daily-backup", - wantConfigMap: true, - wantWarn: false, + name: "DataUpload owned by the executing backup creates a snapshot-info ConfigMap", + duName: "daily-backup-abcde", + duOwnerBackup: "daily-backup", + executingBackup: "daily-backup", + wantConfigMap: true, + wantWarnContains: "", }, { - name: "DataUpload owned by a different backup is skipped and a warning is logged", - duName: "daily-backup-abcde", - duOwnerBackup: "daily-backup", - executingBackup: "hourly-backup", - wantConfigMap: false, - wantWarn: true, + name: "DataUpload owned by a different backup is skipped and a warning is logged", + duName: "daily-backup-abcde", + duOwnerBackup: "daily-backup", + executingBackup: "hourly-backup", + wantConfigMap: false, + wantWarnContains: "velero namespace", }, { - name: "DataUpload with no backup-name label falls through (legacy behavior preserved)", - duName: "legacy-du", - duOwnerBackup: "", - executingBackup: "some-backup", - wantConfigMap: true, - wantWarn: false, + name: "DataUpload with no backup-name label is skipped and a warning is logged", + duName: "unlabeled-du", + duOwnerBackup: "", + executingBackup: "some-backup", + wantConfigMap: false, + wantWarnContains: "cannot be verified", }, } @@ -148,22 +148,24 @@ func TestDataUploadDeleteActionExecute(t *testing.T) { "expected no ConfigMap to be created for foreign DataUpload, but got: %v", getErr) } - // The action must surface foreign-backup DataUploads as warnings so - // operators who accidentally included the velero namespace in a - // backup can detect the misconfiguration from logs, instead of - // having the case silently swallowed. - var sawForeignWarn bool + // The action must surface DataUploads it cannot generate a useful + // snapshot-info ConfigMap for as warnings, so operators who + // accidentally included the velero namespace in a backup (or + // otherwise produced DataUploads without a verifiable owner) can + // detect the misconfiguration from logs instead of having the + // case silently swallowed. + var sawWarn bool for _, entry := range hook.AllEntries() { if entry.Level == logrus.WarnLevel && - strings.Contains(entry.Message, "velero namespace") && - strings.Contains(entry.Message, tc.duName) { - sawForeignWarn = true + strings.Contains(entry.Message, tc.duName) && + (tc.wantWarnContains == "" || strings.Contains(entry.Message, tc.wantWarnContains)) { + sawWarn = true break } } - assert.Equal(t, tc.wantWarn, sawForeignWarn, - "unexpected foreign-backup warn log presence (want=%v, got=%v); entries=%v", - tc.wantWarn, sawForeignWarn, hook.AllEntries()) + assert.Equal(t, tc.wantWarnContains != "", sawWarn, + "unexpected warn log presence (wantContains=%q, got=%v); entries=%v", + tc.wantWarnContains, sawWarn, hook.AllEntries()) }) } }