NS included via labelSelector act as IncludedNamespaces

every item under NS is included.

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

Author: kaovilai

changelogs/unreleased/8342-kaovilai

@@ -0,0 +1 @@
+breaking: Namespaces included by labelSelector act as IncludedNamespaces, all items including unlabeled under a namespace with labelSelector selected by backup selector will be included in the backup.

config/crd/v1/bases/velero.io_backups.yaml

@@ -337,7 +337,9 @@ spec:
                 description: |-
                   LabelSelector is a metav1.LabelSelector to filter with
                   when adding individual objects to the backup. If empty
-                  or nil, all objects are included. Optional.
+                  or nil, all objects are included.
+                  If matched object is Namespace, the namespace will act as IncludedNamespaces,
+                  and all resources in the namespace are included in the backup. Optional.
                 nullable: true
                 properties:
                   matchExpressions:

config/crd/v1/bases/velero.io_schedules.yaml

@@ -378,7 +378,9 @@ spec:
                     description: |-
                       LabelSelector is a metav1.LabelSelector to filter with
                       when adding individual objects to the backup. If empty
-                      or nil, all objects are included. Optional.
+                      or nil, all objects are included.
+                      If matched object is Namespace, the namespace will act as IncludedNamespaces,
+                      and all resources in the namespace are included in the backup. Optional.
                     nullable: true
                     properties:
                       matchExpressions:

config/crd/v1/crds/crds.go

@@ -87,7 +87,9 @@ type BackupSpec struct {
 
 	// LabelSelector is a metav1.LabelSelector to filter with
 	// when adding individual objects to the backup. If empty
-	// or nil, all objects are included. Optional.
+	// or nil, all objects are included.
+	// If matched object is Namespace, the namespace will act as IncludedNamespaces,
+	// and all resources in the namespace are included in the backup. Optional.
 	// +optional
 	// +nullable
 	LabelSelector *metav1.LabelSelector `json:"labelSelector,omitempty"`

pkg/apis/velero/v1/backup_types.go

@@ -269,6 +269,41 @@ func TestBackupOldResourceFiltering(t *testing.T) {
 				"resources/deployments.apps/v1-preferredversion/namespaces/foo/bar.json",
 			},
 		},
+		{
+			name: "included namespaces via label filter backs up all resources in those namespaces and other includedNamespaces resources containing label",
+			backup: defaultBackup().
+				IncludedNamespaces("moo").
+				LabelSelector(&metav1.LabelSelector{MatchLabels: map[string]string{"backup-ns": "true"}}).
+				Result(),
+			apiResources: []*test.APIResource{
+				test.Namespaces(
+					builder.ForNamespace("foo").ObjectMeta(builder.WithLabels("backup-ns", "true")).Result(),
+					builder.ForNamespace("moo").Result(),
+				),
+				test.Pods(
+					builder.ForPod("foo", "bar").Result(),
+					builder.ForPod("moo", "mar").ObjectMeta(builder.WithLabels("backup-ns", "true")).Result(),
+					builder.ForPod("moo", "unlabeled-not-included").Result(),
+					builder.ForPod("zoo", "raz").Result(),
+				),
+				test.Deployments(
+					builder.ForDeployment("foo", "bar").Result(),
+					builder.ForDeployment("zoo", "raz").Result(),
+				),
+			},
+			want: []string{
+				"resources/deployments.apps/namespaces/foo/bar.json",
+				"resources/deployments.apps/v1-preferredversion/namespaces/foo/bar.json",
+				"resources/pods/v1-preferredversion/namespaces/foo/bar.json",
+				"resources/pods/v1-preferredversion/namespaces/moo/mar.json",
+				"resources/pods/namespaces/foo/bar.json",
+				"resources/pods/namespaces/moo/mar.json",
+				"resources/namespaces/cluster/foo.json",
+				"resources/namespaces/cluster/moo.json",
+				"resources/namespaces/v1-preferredversion/cluster/foo.json",
+				"resources/namespaces/v1-preferredversion/cluster/moo.json",
+			},
+		},
 		{
 			name: "excluded namespaces filter only backs up resources not in those namespaces",
 			backup: defaultBackup().

pkg/backup/backup_test.go

@@ -32,6 +32,7 @@ import (
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/tools/pager"
 
 	"github.com/vmware-tanzu/velero/pkg/client"
@@ -44,14 +45,16 @@ import (
 // itemCollector collects items from the Kubernetes API according to
 // the backup spec and writes them to files inside dir.
 type itemCollector struct {
-	log                   logrus.FieldLogger
-	backupRequest         *Request
-	discoveryHelper       discovery.Helper
-	dynamicFactory        client.DynamicFactory
-	cohabitatingResources map[string]*cohabitatingResource
-	dir                   string
-	pageSize              int
-	nsTracker             nsTracker
+	log           logrus.FieldLogger
+	backupRequest *Request
+	// Namespaces that are included by the backup's labelSelector will backup all resources in that namespace even if resources are not labeled.
+	namespacesIncludedByLabelSelector sets.Set[string]
+	discoveryHelper                   discovery.Helper
+	dynamicFactory                    client.DynamicFactory
+	cohabitatingResources             map[string]*cohabitatingResource
+	dir                               string
+	pageSize                          int
+	nsTracker                         nsTracker
 }
 
 // nsTracker is used to integrate several namespace filters together.
@@ -72,42 +75,47 @@ type nsTracker struct {
 	orLabelSelector     []labels.Selector
 	namespaceFilter     *collections.IncludesExcludes
 	logger              logrus.FieldLogger
-
-	namespaceMap map[string]bool
+	namespaceMap        sets.Set[string] // namespaceMap is a set of namespaces tracked to include in backup
+	labeledNamespaces   sets.Set[string] // labeledNamespaces is a subset of namespaceMap added via label selectors.
 }
 
 // track add the namespace into the namespaceMap.
 func (nt *nsTracker) track(ns string) {
 	if nt.namespaceMap == nil {
-		nt.namespaceMap = make(map[string]bool)
+		nt.namespaceMap = make(sets.Set[string])
 	}
+	nt.namespaceMap.Insert(ns)
+}
 
-	if _, ok := nt.namespaceMap[ns]; !ok {
-		nt.namespaceMap[ns] = true
+// trackViaLabel add the namespace into the labeledNamespaces and namespacesMap.
+func (nt *nsTracker) trackViaLabel(ns string) {
+	if nt.labeledNamespaces == nil {
+		nt.labeledNamespaces = make(sets.Set[string])
 	}
+	nt.labeledNamespaces.Insert(ns)
+	nt.track(ns)
 }
 
 // isTracked check whether the namespace's name exists in
 // namespaceMap.
 func (nt *nsTracker) isTracked(ns string) bool {
-	if nt.namespaceMap != nil {
-		return nt.namespaceMap[ns]
-	}
-	return false
+	return nt.namespaceMap.Has(ns)
+}
+
+func (nt *nsTracker) isTrackedViaLabel(ns string) bool {
+	return nt.labeledNamespaces.Has(ns)
 }
 
-// init initialize the namespaceMap, and add elements according to
+// init initialize the namespaceMap, and add (track) elements according to
 // namespace include/exclude filters and the backup label selectors.
+// and return only namespaces that are added to tracker.
 func (nt *nsTracker) init(
 	unstructuredNSs []unstructured.Unstructured,
 	singleLabelSelector labels.Selector,
 	orLabelSelector []labels.Selector,
 	namespaceFilter *collections.IncludesExcludes,
 	logger logrus.FieldLogger,
 ) {
-	if nt.namespaceMap == nil {
-		nt.namespaceMap = make(map[string]bool)
-	}
 	nt.singleLabelSelector = singleLabelSelector
 	nt.orLabelSelector = orLabelSelector
 	nt.namespaceFilter = namespaceFilter
@@ -120,7 +128,7 @@ func (nt *nsTracker) init(
 				namespace.GetName(),
 			)
 
-			nt.track(namespace.GetName())
+			nt.trackViaLabel(namespace.GetName())
 			continue
 		}
 
@@ -130,7 +138,7 @@ func (nt *nsTracker) init(
 					nt.logger.Debugf("Track namespace %s, because its labels match the backup OrLabelSelector.",
 						namespace.GetName(),
 					)
-					nt.track(namespace.GetName())
+					nt.trackViaLabel(namespace.GetName())
 					continue
 				}
 			}
@@ -196,6 +204,7 @@ func (r *itemCollector) getItemsFromResourceIdentifiers(
 
 // getAllItems gets all backup-relevant items from all API groups.
 func (r *itemCollector) getAllItems() []*kubernetesResource {
+	// getItems should return all namespaces and will be filtered by nsTracker.filterNamespaces
 	resources := r.getItems(nil)
 
 	return r.nsTracker.filterNamespaces(resources)
@@ -438,13 +447,33 @@ func (r *itemCollector) getResourceItems(
 	// Namespace are filtered by namespace include/exclude filters,
 	// backup LabelSelectors and OrLabelSelectors are checked too.
 	if gr == kuberesource.Namespaces {
-		return r.collectNamespaces(
+		// collectNamespaces initializes the nsTracker which should only contain namespaces to backup but returns all namespaces.
+		namespaces, err := r.collectNamespaces(
 			resource,
 			gv,
 			gr,
 			preferredGVR,
 			log,
 		)
+		if err != nil {
+			return nil, err
+		}
+		// The namespaces collected contains namespaces selected by namespace filters like include/exclude and label selector.
+		// Per https://github.com/vmware-tanzu/velero/issues/7492#issuecomment-1986146411 we want labelSelector included namespace to act as IncludedNamespaces so add to NamespaceIncludesExcludes string set.
+		r.namespacesIncludedByLabelSelector = make(sets.Set[string])
+		for i := range namespaces {
+			if namespaces[i] != nil {
+				if r.nsTracker.isTrackedViaLabel(namespaces[i].name) {
+					// this namespace is included by the backup's label selector, not the namespace include/exclude filter
+					// this is a special case where we want to include this namespace in the backup and all resources in it regardless of the resource's label selector
+					// in other cases, if label selector is set, we only want to include resources that match the label selector
+					r.namespacesIncludedByLabelSelector.Insert(namespaces[i].name)
+					r.backupRequest.NamespaceIncludesExcludes.Includes(namespaces[i].name)
+					r.log.Infof("including namespace %s by labelselector\n", namespaces[i].name)
+				}
+			}
+		}
+		return namespaces, err
 	}
 
 	clusterScoped := !resource.Namespaced
@@ -511,9 +540,13 @@ func (r *itemCollector) listResourceByLabelsPerNamespace(
 		logger.WithError(err).Error("Error getting dynamic client")
 		return nil, err
 	}
-
+	labeledNsSoBackupUnlabeled := false
+	if r.namespacesIncludedByLabelSelector.Has(namespace) {
+		logger.Infof("Listing all items including unlabeled in namespace %s because ns was selected by the backup's label selector and acts as IncludedNamespaces", namespace)
+		labeledNsSoBackupUnlabeled = true
+	}
 	var orLabelSelectors []string
-	if r.backupRequest.Spec.OrLabelSelectors != nil {
+	if r.backupRequest.Spec.OrLabelSelectors != nil && !labeledNsSoBackupUnlabeled {
 		for _, s := range r.backupRequest.Spec.OrLabelSelectors {
 			orLabelSelectors = append(orLabelSelectors, metav1.FormatLabelSelector(s))
 		}
@@ -539,7 +572,7 @@ func (r *itemCollector) listResourceByLabelsPerNamespace(
 	}
 
 	var labelSelector string
-	if selector := r.backupRequest.Spec.LabelSelector; selector != nil {
+	if selector := r.backupRequest.Spec.LabelSelector; selector != nil && !labeledNsSoBackupUnlabeled {
 		labelSelector = metav1.FormatLabelSelector(selector)
 	}
 
@@ -592,11 +625,13 @@ func sortCoreGroup(group *metav1.APIResourceList) {
 }
 
 // These constants represent the relative priorities for resources in the core API group. We want to
-// ensure that we process pods, then pvcs, then pvs, then anything else. This ensures that when a
-// pod is backed up, we can perform a pre hook, then process pvcs and pvs (including taking a
+// ensure that we process namespaces, pods, then pvcs, then pvs, then anything else. This ensures that when a
+// namespaces is labeled as included, that it is processed first to act as includedNamespaces for the following resources.
+// if pod is backed up, we can perform a pre hook, then process pvcs and pvs (including taking a
 // snapshot), then perform a post hook on the pod.
 const (
-	pod = iota
+	namespaces = iota
+	pod
 	pvc
 	pv
 	other
@@ -606,6 +641,8 @@ const (
 // pods, pvcs, pvs, everything else.
 func coreGroupResourcePriority(resource string) int {
 	switch strings.ToLower(resource) {
+	case "namespaces":
+		return namespaces
 	case "pods":
 		return pod
 	case "persistentvolumeclaims":
@@ -724,6 +761,7 @@ func (r *itemCollector) listItemsForLabel(
 }
 
 // collectNamespaces process namespace resource according to namespace filters.
+// returns a list of ALL namespaces. Filtering will happen outside of this function.
 func (r *itemCollector) collectNamespaces(
 	resource metav1.APIResource,
 	gv schema.GroupVersion,
@@ -782,7 +820,8 @@ func (r *itemCollector) collectNamespaces(
 			orSelectors = append(orSelectors, orSelector)
 		}
 	}
-
+	// init initialize the namespaceMap, and add elements according to
+	// namespace include/exclude filters and the backup label selectors.
 	r.nsTracker.init(
 		unstructuredList.Items,
 		singleSelector,
@@ -808,6 +847,5 @@ func (r *itemCollector) collectNamespaces(
 			path:          path,
 		})
 	}
-
 	return items, nil
 }

pkg/backup/item_collector.go

@@ -93,7 +93,9 @@ spec:
   # all namespace-scoped resources are included. Optional.
   # Cannot work with include-resources, exclude-resources and include-cluster-resources.
   includedNamespaceScopedResources: {}
-  # Individual objects must match this label selector to be included in the backup. Optional.
+  # Individual objects must match this label selector to be included in the backup.
+  # If matched object is Namespace, the namespace will act as IncludedNamespaces, and all resources in the namespace are included in the backup.
+  # Optional.
   labelSelector:
     matchLabels:
       app: velero