diff --git a/changelogs/unreleased/7998-blackpiglet b/changelogs/unreleased/7998-blackpiglet new file mode 100644 index 0000000000..72c8a4fdd9 --- /dev/null +++ b/changelogs/unreleased/7998-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 c40f152a96..af7ffe9ae6 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 01c8dd2163..562e6a2c33 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 24f8e64b2a..989617722c 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -463,7 +463,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)) } @@ -586,24 +586,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 c89bdf3894..ad7677923d 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.