Also skip snapshot-info CM when DataUpload has no owner label

Per review feedback on #9791, the previous revision still let a
DataUpload with an empty velero.io/backup-name label fall through to
genConfigmap, creating a ConfigMap that deleteMovedSnapshots can never
match back to a snapshot. The CM is useless and only adds etcd churn.

Treat the missing-label case the same way as the foreign-owner case:
warn and skip the ConfigMap creation. Use a distinct warn message so
operators can tell the two misconfiguration classes apart in logs
(missing-label vs. owner mismatch from a captured velero namespace).

Test for the missing-label case is updated to assert no ConfigMap is
created and a warn is emitted. The warn assertion is generalized to
match the per-case message substring instead of a fixed string.

Signed-off-by: Christian Schlichtherle <christian@schlichtherle.de>

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. "+

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())
 		})
 	}
 }