From cf5dfdf42dd32d9956421f87e0d592f2bcfd3c85 Mon Sep 17 00:00:00 2001 From: Xun Jiang Date: Wed, 3 Jul 2024 18:11:26 +0800 Subject: [PATCH] Check whether the namespaces specified in namespace filter exist. Check whether the namespaces specified in the backup.Spec.IncludeNamespaces exist during backup resource collcetion If not, log error to mark the backup as PartiallyFailed. Signed-off-by: Xun Jiang --- changelogs/unreleased/7965-blackpiglet | 1 + pkg/backup/item_collector.go | 17 ++++++++ pkg/backup/item_collector_test.go | 11 ++++++ pkg/controller/backup_controller.go | 20 +--------- pkg/controller/backup_controller_test.go | 50 ++---------------------- 5 files changed, 33 insertions(+), 66 deletions(-) create mode 100644 changelogs/unreleased/7965-blackpiglet diff --git a/changelogs/unreleased/7965-blackpiglet b/changelogs/unreleased/7965-blackpiglet new file mode 100644 index 000000000..72c8a4fdd --- /dev/null +++ b/changelogs/unreleased/7965-blackpiglet @@ -0,0 +1 @@ +Check whether the namespaces specified in namespace filter exist. \ No newline at end of file diff --git a/pkg/backup/item_collector.go b/pkg/backup/item_collector.go index c40f152a9..af7ffe9ae 100644 --- a/pkg/backup/item_collector.go +++ b/pkg/backup/item_collector.go @@ -741,6 +741,23 @@ func (r *itemCollector) collectNamespaces( return nil, errors.WithStack(err) } + for _, includedNSName := range r.backupRequest.Backup.Spec.IncludedNamespaces { + nsExists := false + // Skip checking the namespace existing when it's "*". + if includedNSName == "*" { + continue + } + for _, unstructuredNS := range unstructuredList.Items { + if unstructuredNS.GetName() == includedNSName { + nsExists = true + } + } + + if !nsExists { + log.Errorf("fail to get the namespace %s specified in backup.Spec.IncludedNamespaces", includedNSName) + } + } + var singleSelector labels.Selector var orSelectors []labels.Selector diff --git a/pkg/backup/item_collector_test.go b/pkg/backup/item_collector_test.go index 01c8dd216..562e6a2c3 100644 --- a/pkg/backup/item_collector_test.go +++ b/pkg/backup/item_collector_test.go @@ -224,6 +224,17 @@ func TestItemCollectorBackupNamespaces(t *testing.T) { }, expectedTrackedNS: []string{"ns1", "ns2"}, }, + { + name: "ns specified by the IncludeNamespaces cannot be found", + backup: builder.ForBackup("velero", "backup").IncludedNamespaces("ns1", "invalid", "*").Result(), + ie: collections.NewIncludesExcludes().Includes("ns1", "invalid", "*"), + namespaces: []*corev1.Namespace{ + builder.ForNamespace("ns1").ObjectMeta(builder.WithLabels("name", "ns1")).Result(), + builder.ForNamespace("ns2").Result(), + builder.ForNamespace("ns3").Result(), + }, + expectedTrackedNS: []string{"ns1"}, + }, } for _, tc := range tests { diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index 8ccd8db18..34d3d43ee 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -464,7 +464,7 @@ func (b *backupReconciler) prepareBackupRequest(backup *velerov1api.Backup, logg } // validate the included/excluded namespaces - for _, err := range b.validateNamespaceIncludesExcludes(request.Spec.IncludedNamespaces, request.Spec.ExcludedNamespaces) { + for _, err := range collections.ValidateNamespaceIncludesExcludes(request.Spec.IncludedNamespaces, request.Spec.ExcludedNamespaces) { request.Status.ValidationErrors = append(request.Status.ValidationErrors, fmt.Sprintf("Invalid included/excluded namespace lists: %v", err)) } @@ -596,24 +596,6 @@ func (b *backupReconciler) validateAndGetSnapshotLocations(backup *velerov1api.B return providerLocations, nil } -func (b *backupReconciler) validateNamespaceIncludesExcludes(includedNamespaces, excludedNamespaces []string) []error { - var errs []error - if errs = collections.ValidateNamespaceIncludesExcludes(includedNamespaces, excludedNamespaces); len(errs) > 0 { - return errs - } - - namespace := &corev1api.Namespace{} - for _, name := range collections.NewIncludesExcludes().Includes(includedNamespaces...).GetIncludes() { - if name == "" || name == "*" { - continue - } - if err := b.kbClient.Get(context.Background(), kbclient.ObjectKey{Name: name}, namespace); err != nil { - errs = append(errs, err) - } - } - return errs -} - // runBackup runs and uploads a validated backup. Any error returned from this function // causes the backup to be Failed; if no error is returned, the backup's status's Errors // field is checked to see if the backup was a partial failure. diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index 2f613af2c..46f123f1a 100644 --- a/pkg/controller/backup_controller_test.go +++ b/pkg/controller/backup_controller_test.go @@ -190,16 +190,10 @@ func TestProcessBackupValidationFailures(t *testing.T) { }, { name: "use old filter parameters and new filter parameters together", - backup: defaultBackup().IncludeClusterResources(true).IncludedNamespaceScopedResources("Deployment").IncludedNamespaces("foo").Result(), + backup: defaultBackup().IncludeClusterResources(true).IncludedNamespaceScopedResources("Deployment").IncludedNamespaces("default").Result(), backupLocation: defaultBackupLocation, expectedErrs: []string{"include-resources, exclude-resources and include-cluster-resources are old filter parameters.\ninclude-cluster-scoped-resources, exclude-cluster-scoped-resources, include-namespace-scoped-resources and exclude-namespace-scoped-resources are new filter parameters.\nThey cannot be used together"}, }, - { - name: "nonexisting namespace", - backup: defaultBackup().IncludedNamespaces("non-existing").Result(), - backupLocation: defaultBackupLocation, - expectedErrs: []string{"Invalid included/excluded namespace lists: namespaces \"non-existing\" not found"}, - }, } for _, test := range tests { @@ -214,11 +208,10 @@ func TestProcessBackupValidationFailures(t *testing.T) { require.NoError(t, err) var fakeClient kbclient.Client - namespace := builder.ForNamespace("foo").Result() if test.backupLocation != nil { - fakeClient = velerotest.NewFakeControllerRuntimeClient(t, test.backupLocation, namespace) + fakeClient = velerotest.NewFakeControllerRuntimeClient(t, test.backupLocation) } else { - fakeClient = velerotest.NewFakeControllerRuntimeClient(t, namespace) + fakeClient = velerotest.NewFakeControllerRuntimeClient(t) } c := &backupReconciler{ @@ -1574,43 +1567,6 @@ func TestValidateAndGetSnapshotLocations(t *testing.T) { } } -func TestValidateNamespaceIncludesExcludes(t *testing.T) { - namespace := builder.ForNamespace("default").Result() - reconciler := &backupReconciler{ - kbClient: velerotest.NewFakeControllerRuntimeClient(t, namespace), - } - - // empty string as includedNamespaces - includedNamespaces := []string{""} - excludedNamespaces := []string{"test"} - errs := reconciler.validateNamespaceIncludesExcludes(includedNamespaces, excludedNamespaces) - assert.Empty(t, errs) - - // "*" as includedNamespaces - includedNamespaces = []string{"*"} - excludedNamespaces = []string{"test"} - errs = reconciler.validateNamespaceIncludesExcludes(includedNamespaces, excludedNamespaces) - assert.Empty(t, errs) - - // invalid namespaces - includedNamespaces = []string{"1@#"} - excludedNamespaces = []string{"2@#"} - errs = reconciler.validateNamespaceIncludesExcludes(includedNamespaces, excludedNamespaces) - assert.Len(t, errs, 2) - - // not exist namespaces - includedNamespaces = []string{"non-existing-namespace"} - excludedNamespaces = []string{} - errs = reconciler.validateNamespaceIncludesExcludes(includedNamespaces, excludedNamespaces) - assert.Len(t, errs, 1) - - // valid namespaces - includedNamespaces = []string{"default"} - excludedNamespaces = []string{} - errs = reconciler.validateNamespaceIncludesExcludes(includedNamespaces, excludedNamespaces) - assert.Empty(t, errs) -} - // Test_getLastSuccessBySchedule verifies that the getLastSuccessBySchedule helper function correctly returns // the completion timestamp of the most recent completed backup for each schedule, including an entry for ad-hoc // or non-scheduled backups.