From 8766a4dbd48076ca742e6c0582f2dab319b239dd Mon Sep 17 00:00:00 2001 From: Xun Jiang Date: Mon, 29 May 2023 17:32:28 +0800 Subject: [PATCH] Make namespace resource doesn't honor label selector filters anymore. For some use cases, namespaced-scope resources are inluded into backup, but the namespaces are not included due to filters setting. To do this, removing label selector filter from namespace resource. Namespace resource only honor namespace exclude/include filters. Signed-off-by: Xun Jiang --- changelogs/unreleased/6320-blackpiglet | 1 + pkg/backup/backup_test.go | 144 +++++++++++++++++++++++++ pkg/backup/item_collector.go | 111 ++++++++++--------- 3 files changed, 204 insertions(+), 52 deletions(-) create mode 100644 changelogs/unreleased/6320-blackpiglet diff --git a/changelogs/unreleased/6320-blackpiglet b/changelogs/unreleased/6320-blackpiglet new file mode 100644 index 000000000..fee63052c --- /dev/null +++ b/changelogs/unreleased/6320-blackpiglet @@ -0,0 +1 @@ +Include namespaces needed by namespaced-scope resources in backup. \ No newline at end of file diff --git a/pkg/backup/backup_test.go b/pkg/backup/backup_test.go index a90b3e0b1..30231b6fa 100644 --- a/pkg/backup/backup_test.go +++ b/pkg/backup/backup_test.go @@ -4101,3 +4101,147 @@ func TestBackupNewResourceFiltering(t *testing.T) { }) } } + +func TestBackupNamespaces(t *testing.T) { + tests := []struct { + name string + backup *velerov1.Backup + apiResources []*test.APIResource + want []string + }{ + { + name: "LabelSelector test", + backup: defaultBackup().LabelSelector(&metav1.LabelSelector{MatchLabels: map[string]string{"a": "b"}}). + Result(), + apiResources: []*test.APIResource{ + test.Namespaces( + builder.ForNamespace("ns-1").Result(), + builder.ForNamespace("ns-2").Result(), + builder.ForNamespace("ns-3").Result(), + ), + test.Deployments( + builder.ForDeployment("ns-1", "deploy-1").ObjectMeta(builder.WithLabels("a", "b")).Result(), + ), + }, + want: []string{ + "resources/namespaces/cluster/ns-1.json", + "resources/namespaces/v1-preferredversion/cluster/ns-1.json", + "resources/deployments.apps/namespaces/ns-1/deploy-1.json", + "resources/deployments.apps/v1-preferredversion/namespaces/ns-1/deploy-1.json", + "resources/namespaces/cluster/ns-2.json", + "resources/namespaces/v1-preferredversion/cluster/ns-2.json", + "resources/namespaces/cluster/ns-3.json", + "resources/namespaces/v1-preferredversion/cluster/ns-3.json", + }, + }, + { + name: "OrLabelSelector test", + backup: defaultBackup().OrLabelSelector([]*metav1.LabelSelector{ + {MatchLabels: map[string]string{"a": "b"}}, + {MatchLabels: map[string]string{"c": "d"}}, + }). + Result(), + apiResources: []*test.APIResource{ + test.Namespaces( + builder.ForNamespace("ns-1").Result(), + builder.ForNamespace("ns-2").Result(), + builder.ForNamespace("ns-3").Result(), + ), + test.Deployments( + builder.ForDeployment("ns-1", "deploy-1").ObjectMeta(builder.WithLabels("a", "b")).Result(), + builder.ForDeployment("ns-2", "deploy-2").ObjectMeta(builder.WithLabels("c", "d")).Result(), + ), + }, + want: []string{ + "resources/namespaces/cluster/ns-1.json", + "resources/namespaces/v1-preferredversion/cluster/ns-1.json", + "resources/namespaces/cluster/ns-2.json", + "resources/namespaces/v1-preferredversion/cluster/ns-2.json", + "resources/namespaces/cluster/ns-3.json", + "resources/namespaces/v1-preferredversion/cluster/ns-3.json", + "resources/deployments.apps/namespaces/ns-1/deploy-1.json", + "resources/deployments.apps/v1-preferredversion/namespaces/ns-1/deploy-1.json", + "resources/deployments.apps/namespaces/ns-2/deploy-2.json", + "resources/deployments.apps/v1-preferredversion/namespaces/ns-2/deploy-2.json", + }, + }, + { + name: "LabelSelector and Namespace filtering test", + backup: defaultBackup().IncludedNamespaces("ns-1").LabelSelector(&metav1.LabelSelector{MatchLabels: map[string]string{"a": "b"}}). + Result(), + apiResources: []*test.APIResource{ + test.Namespaces( + builder.ForNamespace("ns-1").Result(), + builder.ForNamespace("ns-2").Result(), + builder.ForNamespace("ns-3").Result(), + ), + test.Deployments( + builder.ForDeployment("ns-1", "deploy-1").ObjectMeta(builder.WithLabels("a", "b")).Result(), + ), + }, + want: []string{ + "resources/namespaces/cluster/ns-1.json", + "resources/namespaces/v1-preferredversion/cluster/ns-1.json", + "resources/deployments.apps/namespaces/ns-1/deploy-1.json", + "resources/deployments.apps/v1-preferredversion/namespaces/ns-1/deploy-1.json", + }, + }, + { + name: "Empty namespace test", + backup: defaultBackup().IncludedNamespaces("invalid*").Result(), + apiResources: []*test.APIResource{ + test.Namespaces( + builder.ForNamespace("ns-1").Result(), + builder.ForNamespace("ns-2").Result(), + builder.ForNamespace("ns-3").Result(), + ), + test.Deployments( + builder.ForDeployment("ns-1", "deploy-1").ObjectMeta(builder.WithLabels("a", "b")).Result(), + ), + }, + want: []string{}, + }, + { + name: "Default namespace filter test", + backup: defaultBackup().Result(), + apiResources: []*test.APIResource{ + test.Namespaces( + builder.ForNamespace("ns-1").Result(), + builder.ForNamespace("ns-2").Result(), + builder.ForNamespace("ns-3").Result(), + ), + test.Deployments( + builder.ForDeployment("ns-1", "deploy-1").ObjectMeta(builder.WithLabels("a", "b")).Result(), + ), + }, + want: []string{ + "resources/namespaces/cluster/ns-1.json", + "resources/namespaces/v1-preferredversion/cluster/ns-1.json", + "resources/namespaces/cluster/ns-2.json", + "resources/namespaces/v1-preferredversion/cluster/ns-2.json", + "resources/namespaces/cluster/ns-3.json", + "resources/namespaces/v1-preferredversion/cluster/ns-3.json", + "resources/deployments.apps/namespaces/ns-1/deploy-1.json", + "resources/deployments.apps/v1-preferredversion/namespaces/ns-1/deploy-1.json", + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + var ( + h = newHarness(t) + req = &Request{Backup: tc.backup} + backupFile = bytes.NewBuffer([]byte{}) + ) + + for _, resource := range tc.apiResources { + h.addItems(t, resource) + } + + h.backupper.Backup(h.log, req, backupFile, nil, nil) + + assertTarballContents(t, backupFile, append(tc.want, "metadata/version")...) + }) + } +} diff --git a/pkg/backup/item_collector.go b/pkg/backup/item_collector.go index 373cbaa55..5dc6ceacb 100644 --- a/pkg/backup/item_collector.go +++ b/pkg/backup/item_collector.go @@ -29,7 +29,6 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/tools/pager" @@ -275,56 +274,24 @@ func (r *itemCollector) getResourceItems(log logrus.FieldLogger, gv schema.Group namespacesToList := getNamespacesToList(r.backupRequest.NamespaceIncludesExcludes) - // Check if we're backing up namespaces for a less-than-full backup. - // We enter this block if resource is Namespaces and the namespace list is either empty or contains - // an explicit namespace list. (We skip this block if the list contains "" since that indicates - // a full-cluster backup - if gr == kuberesource.Namespaces && (len(namespacesToList) == 0 || namespacesToList[0] != "") { + // Handle namespace resource here. + // Namespace are only filtered by namespace include/exclude filters. + // Label selectors are not checked. + if gr == kuberesource.Namespaces { resourceClient, err := r.dynamicFactory.ClientForGroupVersionResource(gv, resource, "") if err != nil { log.WithError(err).Error("Error getting dynamic client") - } else { - var labelSelector labels.Selector - if r.backupRequest.Spec.LabelSelector != nil { - labelSelector, err = metav1.LabelSelectorAsSelector(r.backupRequest.Spec.LabelSelector) - if err != nil { - // This should never happen... - return nil, errors.Wrap(err, "invalid label selector") - } - } - - var items []*kubernetesResource - for _, ns := range namespacesToList { - log = log.WithField("namespace", ns) - log.Info("Getting namespace") - unstructured, err := resourceClient.Get(ns, metav1.GetOptions{}) - if err != nil { - log.WithError(errors.WithStack(err)).Error("Error getting namespace") - continue - } - - labels := labels.Set(unstructured.GetLabels()) - if labelSelector != nil && !labelSelector.Matches(labels) { - log.Info("Skipping namespace because it does not match the backup's label selector") - continue - } - - path, err := r.writeToFile(unstructured) - if err != nil { - log.WithError(err).Error("Error writing item to file") - continue - } - - items = append(items, &kubernetesResource{ - groupResource: gr, - preferredGVR: preferredGVR, - name: ns, - path: path, - }) - } - - return items, nil + return nil, errors.WithStack(err) } + unstructuredList, err := resourceClient.List(metav1.ListOptions{}) + if err != nil { + log.WithError(errors.WithStack(err)).Error("error list namespaces") + return nil, errors.WithStack(err) + } + + items := r.backupNamespaces(unstructuredList, namespacesToList, gr, preferredGVR, log) + + return items, nil } // If we get here, we're backing up something other than namespaces @@ -390,11 +357,6 @@ func (r *itemCollector) getResourceItems(log logrus.FieldLogger, gv schema.Group for i := range unstructuredItems { item := &unstructuredItems[i] - if gr == kuberesource.Namespaces && !r.backupRequest.NamespaceIncludesExcludes.ShouldInclude(item.GetName()) { - log.WithField("name", item.GetName()).Info("Skipping namespace because it's excluded") - continue - } - path, err := r.writeToFile(item) if err != nil { log.WithError(err).Error("Error writing item to file") @@ -568,3 +530,48 @@ func (r *itemCollector) listItemsForLabel(unstructuredItems []unstructured.Unstr } return unstructuredItems, nil } + +// backupNamespaces process namespace resource according to namespace filters. +func (r *itemCollector) backupNamespaces(unstructuredList *unstructured.UnstructuredList, + namespacesToList []string, gr schema.GroupResource, preferredGVR schema.GroupVersionResource, + log logrus.FieldLogger) []*kubernetesResource { + var items []*kubernetesResource + for index, unstructured := range unstructuredList.Items { + found := false + if len(namespacesToList) == 0 { + // No namespace found. By far, this condition cannot be triggered. Either way, + // namespacesToList is not empty. + log.Debug("Skip namespace resource, because no item found by namespace filters.") + break + } else if len(namespacesToList) == 1 && namespacesToList[0] == "" { + // All namespaces are included. + log.Debugf("Backup namespace %s due to full cluster backup.", unstructured.GetName()) + found = true + } else { + for _, ns := range namespacesToList { + if unstructured.GetName() == ns { + log.Debugf("Backup namespace %s due to namespace filters setting.", unstructured.GetName()) + found = true + break + } + } + } + + if found { + path, err := r.writeToFile(&unstructuredList.Items[index]) + if err != nil { + log.WithError(err).Error("Error writing item to file") + continue + } + + items = append(items, &kubernetesResource{ + groupResource: gr, + preferredGVR: preferredGVR, + name: unstructured.GetName(), + path: path, + }) + } + } + + return items +}