From af2a792a9a583b4ccc3f54c471819e9e304191d5 Mon Sep 17 00:00:00 2001 From: Justin Nauman Date: Sun, 27 Aug 2017 11:42:10 -0500 Subject: [PATCH 1/4] Allows explicit include/exclude of namespaces on restores - Introduces similar Include/Exclude declaration on the Restore resource and cli flags - Kept support for legacy Namespaces attribute until it could be deprecating. Defining both IncludeNamespaces and Namespaces results in a validation error and the Restore will not be processed (shouldn't be able to occur) Signed-off-by: Justin Nauman --- docs/cli-reference/ark_restore_create.md | 3 +- pkg/apis/ark/v1/restore.go | 10 ++++ pkg/cmd/cli/restore/create.go | 20 ++++--- pkg/controller/restore_controller.go | 17 +++++- pkg/controller/restore_controller_test.go | 59 +++++++++++-------- pkg/restore/restore.go | 9 +-- pkg/restore/restore_test.go | 25 +++++++- pkg/restore/restorers/namespace_restorer.go | 11 ++-- .../restorers/namespace_restorer_test.go | 18 +++++- pkg/util/test/test_restore.go | 13 +++- 10 files changed, 133 insertions(+), 52 deletions(-) diff --git a/docs/cli-reference/ark_restore_create.md b/docs/cli-reference/ark_restore_create.md index 7190ab667..b46a89e7b 100644 --- a/docs/cli-reference/ark_restore_create.md +++ b/docs/cli-reference/ark_restore_create.md @@ -14,10 +14,11 @@ ark restore create BACKUP ### Options ``` + --exclude-namespaces stringArray namespaces to exclude from the backup + --include-namespaces stringArray namespaces to include in the backup (use '*' for all namespaces) (default *) --label-columns stringArray a comma-separated list of labels to be displayed as columns --labels mapStringString labels to apply to the restore --namespace-mappings mapStringString namespace mappings from name in the backup to desired restored name in the form src1:dst1,src2:dst2,... - --namespaces stringArray comma-separated list of namespaces to restore -o, --output string Output display format. For create commands, display the object but do not send it to the server. Valid formats are 'table', 'json', and 'yaml'. --restore-volumes optionalBool[=true] whether to restore volumes from snapshots -l, --selector labelSelector only restore resources matching this label selector (default ) diff --git a/pkg/apis/ark/v1/restore.go b/pkg/apis/ark/v1/restore.go index 4e254f865..148ec8381 100644 --- a/pkg/apis/ark/v1/restore.go +++ b/pkg/apis/ark/v1/restore.go @@ -24,9 +24,19 @@ type RestoreSpec struct { // from. BackupName string `json:"backupName"` + // NOTE: This is deprecated. IncludedNamespaces and ExcludedNamespaces + // should be used instead // Namespaces is a slice of namespaces in the Ark backup to restore. Namespaces []string `json:"namespaces"` + // IncludedNamespaces is a slice of namespace names to include objects + // from. If empty, all namespaces are included. + IncludedNamespaces []string `json:"includedNamespaces"` + + // ExcludedNamespaces contains a list of namespaces that are not + // included in the backup. + ExcludedNamespaces []string `json:"excludedNamespaces"` + // NamespaceMapping is a map of source namespace names // to target namespace names to restore into. Any source // namespaces not included in the map will be restored into diff --git a/pkg/cmd/cli/restore/create.go b/pkg/cmd/cli/restore/create.go index 8dd7bbf79..266e61978 100644 --- a/pkg/cmd/cli/restore/create.go +++ b/pkg/cmd/cli/restore/create.go @@ -57,7 +57,8 @@ type CreateOptions struct { BackupName string RestoreVolumes flag.OptionalBool Labels flag.Map - Namespaces flag.StringArray + IncludeNamespaces flag.StringArray + ExcludeNamespaces flag.StringArray NamespaceMappings flag.Map Selector flag.LabelSelector } @@ -65,15 +66,17 @@ type CreateOptions struct { func NewCreateOptions() *CreateOptions { return &CreateOptions{ Labels: flag.NewMap(), + IncludeNamespaces: flag.NewStringArray("*"), NamespaceMappings: flag.NewMap().WithEntryDelimiter(",").WithKeyValueDelimiter(":"), RestoreVolumes: flag.NewOptionalBool(nil), } } func (o *CreateOptions) BindFlags(flags *pflag.FlagSet) { - flags.Var(&o.Labels, "labels", "labels to apply to the restore") - flags.Var(&o.Namespaces, "namespaces", "comma-separated list of namespaces to restore") + flags.Var(&o.IncludeNamespaces, "include-namespaces", "namespaces to include in the backup (use '*' for all namespaces)") + flags.Var(&o.ExcludeNamespaces, "exclude-namespaces", "namespaces to exclude from the backup") flags.Var(&o.NamespaceMappings, "namespace-mappings", "namespace mappings from name in the backup to desired restored name in the form src1:dst1,src2:dst2,...") + flags.Var(&o.Labels, "labels", "labels to apply to the restore") flags.VarP(&o.Selector, "selector", "l", "only restore resources matching this label selector") f := flags.VarPF(&o.RestoreVolumes, "restore-volumes", "", "whether to restore volumes from snapshots") // this allows the user to just specify "--restore-volumes" as shorthand for "--restore-volumes=true" @@ -111,11 +114,12 @@ func (o *CreateOptions) Run(c *cobra.Command, f client.Factory) error { Labels: o.Labels.Data(), }, Spec: api.RestoreSpec{ - BackupName: o.BackupName, - Namespaces: o.Namespaces, - NamespaceMapping: o.NamespaceMappings.Data(), - LabelSelector: o.Selector.LabelSelector, - RestorePVs: o.RestoreVolumes.Value, + BackupName: o.BackupName, + IncludedNamespaces: o.IncludeNamespaces, + ExcludedNamespaces: o.ExcludeNamespaces, + NamespaceMapping: o.NamespaceMappings.Data(), + LabelSelector: o.Selector.LabelSelector, + RestorePVs: o.RestoreVolumes.Value, }, } diff --git a/pkg/controller/restore_controller.go b/pkg/controller/restore_controller.go index 68350e6de..a380d944f 100644 --- a/pkg/controller/restore_controller.go +++ b/pkg/controller/restore_controller.go @@ -224,10 +224,17 @@ func (controller *restoreController) processRestore(key string) error { restore.Status.Phase = api.RestorePhaseFailedValidation } else { restore.Status.Phase = api.RestorePhaseInProgress - } - if len(restore.Spec.Namespaces) == 0 { - restore.Spec.Namespaces = []string{"*"} + if len(restore.Spec.Namespaces) != 0 { + glog.V(4).Info("the restore.Spec.Namespaces field has been deprecated. Please use the IncludedNamespaces and ExcludedNamespaces feature instead") + + restore.Spec.IncludedNamespaces = restore.Spec.Namespaces + restore.Spec.Namespaces = nil + } + + if len(restore.Spec.IncludedNamespaces) == 0 { + restore.Spec.IncludedNamespaces = []string{"*"} + } } // update status @@ -278,6 +285,10 @@ func (controller *restoreController) getValidationErrors(itm *api.Restore) []str validationErrors = append(validationErrors, "BackupName must be non-empty and correspond to the name of a backup in object storage.") } + if len(itm.Spec.Namespaces) > 0 && len(itm.Spec.IncludedNamespaces) > 0 { + validationErrors = append(validationErrors, "Namespace and ItemNamespaces can not both be defined on the backup spec.") + } + if !controller.pvProviderExists && itm.Spec.RestorePVs != nil && *itm.Spec.RestorePVs { validationErrors = append(validationErrors, "Server is not configured for PV snapshot restores") } diff --git a/pkg/controller/restore_controller_test.go b/pkg/controller/restore_controller_test.go index d8b2ab797..648d90d54 100644 --- a/pkg/controller/restore_controller_test.go +++ b/pkg/controller/restore_controller_test.go @@ -73,24 +73,37 @@ func TestProcessRestore(t *testing.T) { expectedErr: false, }, { - name: "new restore with empty backup name fails validation", - restore: NewTestRestore("foo", "bar", api.RestorePhaseNew).WithRestorableNamespace("ns-1").Restore, + name: "restore with both namespaces and includedNamespaces fails validation", + restore: NewTestRestore("foo", "bar", api.RestorePhaseNew).WithBackup("backup-1").WithNamespace("ns-1").WithIncludedNamespace("another-1").Restore, + backup: NewTestBackup().WithName("backup-1").Backup, expectedErr: false, expectedRestoreUpdates: []*api.Restore{ NewTestRestore("foo", "bar", api.RestorePhaseFailedValidation). - WithRestorableNamespace("ns-1"). + WithBackup("backup-1"). + WithNamespace("ns-1"). + WithIncludedNamespace("another-1"). + WithValidationError("Namespace and ItemNamespaces can not both be defined on the backup spec.").Restore, + }, + }, + { + name: "new restore with empty backup name fails validation", + restore: NewTestRestore("foo", "bar", api.RestorePhaseNew).WithIncludedNamespace("ns-1").Restore, + expectedErr: false, + expectedRestoreUpdates: []*api.Restore{ + NewTestRestore("foo", "bar", api.RestorePhaseFailedValidation). + WithIncludedNamespace("ns-1"). WithValidationError("BackupName must be non-empty and correspond to the name of a backup in object storage.").Restore, }, }, { name: "restore with non-existent backup name fails", - restore: NewTestRestore("foo", "bar", api.RestorePhaseNew).WithBackup("backup-1").WithRestorableNamespace("ns-1").Restore, + restore: NewTestRestore("foo", "bar", api.RestorePhaseNew).WithBackup("backup-1").WithIncludedNamespace("ns-1").Restore, expectedErr: false, expectedRestoreUpdates: []*api.Restore{ - NewTestRestore("foo", "bar", api.RestorePhaseInProgress).WithBackup("backup-1").WithRestorableNamespace("ns-1").Restore, + NewTestRestore("foo", "bar", api.RestorePhaseInProgress).WithBackup("backup-1").WithIncludedNamespace("ns-1").Restore, NewTestRestore("foo", "bar", api.RestorePhaseCompleted). WithBackup("backup-1"). - WithRestorableNamespace("ns-1"). + WithIncludedNamespace("ns-1"). WithErrors(api.RestoreResult{ Cluster: []string{"backup.ark.heptio.com \"backup-1\" not found"}, }). @@ -99,33 +112,33 @@ func TestProcessRestore(t *testing.T) { }, { name: "restorer throwing an error causes the restore to fail", - restore: NewTestRestore("foo", "bar", api.RestorePhaseNew).WithBackup("backup-1").WithRestorableNamespace("ns-1").Restore, + restore: NewTestRestore("foo", "bar", api.RestorePhaseNew).WithBackup("backup-1").WithIncludedNamespace("ns-1").Restore, backup: NewTestBackup().WithName("backup-1").Backup, restorerError: errors.New("blarg"), expectedErr: false, expectedRestoreUpdates: []*api.Restore{ - NewTestRestore("foo", "bar", api.RestorePhaseInProgress).WithBackup("backup-1").WithRestorableNamespace("ns-1").Restore, + NewTestRestore("foo", "bar", api.RestorePhaseInProgress).WithBackup("backup-1").WithIncludedNamespace("ns-1").Restore, NewTestRestore("foo", "bar", api.RestorePhaseCompleted). WithBackup("backup-1"). - WithRestorableNamespace("ns-1"). + WithIncludedNamespace("ns-1"). WithErrors(api.RestoreResult{ Namespaces: map[string][]string{ "ns-1": {"blarg"}, }, }).Restore, }, - expectedRestorerCall: NewTestRestore("foo", "bar", api.RestorePhaseInProgress).WithBackup("backup-1").WithRestorableNamespace("ns-1").Restore, + expectedRestorerCall: NewTestRestore("foo", "bar", api.RestorePhaseInProgress).WithBackup("backup-1").WithIncludedNamespace("ns-1").Restore, }, { name: "valid restore gets executed", - restore: NewTestRestore("foo", "bar", api.RestorePhaseNew).WithBackup("backup-1").WithRestorableNamespace("ns-1").Restore, + restore: NewTestRestore("foo", "bar", api.RestorePhaseNew).WithBackup("backup-1").WithIncludedNamespace("ns-1").Restore, backup: NewTestBackup().WithName("backup-1").Backup, expectedErr: false, expectedRestoreUpdates: []*api.Restore{ - NewTestRestore("foo", "bar", api.RestorePhaseInProgress).WithBackup("backup-1").WithRestorableNamespace("ns-1").Restore, - NewTestRestore("foo", "bar", api.RestorePhaseCompleted).WithBackup("backup-1").WithRestorableNamespace("ns-1").Restore, + NewTestRestore("foo", "bar", api.RestorePhaseInProgress).WithBackup("backup-1").WithIncludedNamespace("ns-1").Restore, + NewTestRestore("foo", "bar", api.RestorePhaseCompleted).WithBackup("backup-1").WithIncludedNamespace("ns-1").Restore, }, - expectedRestorerCall: NewTestRestore("foo", "bar", api.RestorePhaseInProgress).WithBackup("backup-1").WithRestorableNamespace("ns-1").Restore, + expectedRestorerCall: NewTestRestore("foo", "bar", api.RestorePhaseInProgress).WithBackup("backup-1").WithIncludedNamespace("ns-1").Restore, }, { name: "restore with no restorable namespaces gets defaulted to *", @@ -133,30 +146,30 @@ func TestProcessRestore(t *testing.T) { backup: NewTestBackup().WithName("backup-1").Backup, expectedErr: false, expectedRestoreUpdates: []*api.Restore{ - NewTestRestore("foo", "bar", api.RestorePhaseInProgress).WithBackup("backup-1").WithRestorableNamespace("*").Restore, - NewTestRestore("foo", "bar", api.RestorePhaseCompleted).WithBackup("backup-1").WithRestorableNamespace("*").Restore, + NewTestRestore("foo", "bar", api.RestorePhaseInProgress).WithBackup("backup-1").WithIncludedNamespace("*").Restore, + NewTestRestore("foo", "bar", api.RestorePhaseCompleted).WithBackup("backup-1").WithIncludedNamespace("*").Restore, }, - expectedRestorerCall: NewTestRestore("foo", "bar", api.RestorePhaseInProgress).WithBackup("backup-1").WithRestorableNamespace("*").Restore, + expectedRestorerCall: NewTestRestore("foo", "bar", api.RestorePhaseInProgress).WithBackup("backup-1").WithIncludedNamespace("*").Restore, }, { name: "valid restore with RestorePVs=true gets executed when allowRestoreSnapshots=true", - restore: NewTestRestore("foo", "bar", api.RestorePhaseNew).WithBackup("backup-1").WithRestorableNamespace("ns-1").WithRestorePVs(true).Restore, + restore: NewTestRestore("foo", "bar", api.RestorePhaseNew).WithBackup("backup-1").WithIncludedNamespace("ns-1").WithRestorePVs(true).Restore, backup: NewTestBackup().WithName("backup-1").Backup, allowRestoreSnapshots: true, expectedErr: false, expectedRestoreUpdates: []*api.Restore{ - NewTestRestore("foo", "bar", api.RestorePhaseInProgress).WithBackup("backup-1").WithRestorableNamespace("ns-1").WithRestorePVs(true).Restore, - NewTestRestore("foo", "bar", api.RestorePhaseCompleted).WithBackup("backup-1").WithRestorableNamespace("ns-1").WithRestorePVs(true).Restore, + NewTestRestore("foo", "bar", api.RestorePhaseInProgress).WithBackup("backup-1").WithIncludedNamespace("ns-1").WithRestorePVs(true).Restore, + NewTestRestore("foo", "bar", api.RestorePhaseCompleted).WithBackup("backup-1").WithIncludedNamespace("ns-1").WithRestorePVs(true).Restore, }, - expectedRestorerCall: NewTestRestore("foo", "bar", api.RestorePhaseInProgress).WithBackup("backup-1").WithRestorableNamespace("ns-1").WithRestorePVs(true).Restore, + expectedRestorerCall: NewTestRestore("foo", "bar", api.RestorePhaseInProgress).WithBackup("backup-1").WithIncludedNamespace("ns-1").WithRestorePVs(true).Restore, }, { name: "restore with RestorePVs=true fails validation when allowRestoreSnapshots=false", - restore: NewTestRestore("foo", "bar", api.RestorePhaseNew).WithBackup("backup-1").WithRestorableNamespace("ns-1").WithRestorePVs(true).Restore, + restore: NewTestRestore("foo", "bar", api.RestorePhaseNew).WithBackup("backup-1").WithIncludedNamespace("ns-1").WithRestorePVs(true).Restore, backup: NewTestBackup().WithName("backup-1").Backup, expectedErr: false, expectedRestoreUpdates: []*api.Restore{ - NewTestRestore("foo", "bar", api.RestorePhaseFailedValidation).WithBackup("backup-1").WithRestorableNamespace("ns-1").WithRestorePVs(true). + NewTestRestore("foo", "bar", api.RestorePhaseFailedValidation).WithBackup("backup-1").WithIncludedNamespace("ns-1").WithRestorePVs(true). WithValidationError("Server is not configured for PV snapshot restores").Restore, }, }, diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index a75a8b3dd..aec0c56ac 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -46,6 +46,7 @@ import ( arkv1client "github.com/heptio/ark/pkg/generated/clientset/typed/ark/v1" "github.com/heptio/ark/pkg/restore/restorers" "github.com/heptio/ark/pkg/util/kube" + "github.com/heptio/ark/pkg/util/collections" ) // Restorer knows how to restore a backup. @@ -225,18 +226,18 @@ func (kr *kubernetesRestorer) restoreFromDir( return warnings, errors } - namespacesToRestore := sets.NewString(restore.Spec.Namespaces...) + namespaceFilter := collections.NewIncludesExcludes().Includes(restore.Spec.IncludedNamespaces...).Excludes(restore.Spec.ExcludedNamespaces...) for _, ns := range nses { if !ns.IsDir() { continue } - nsPath := path.Join(namespacesPath, ns.Name()) - if !namespacesToRestore.Has("*") && !namespacesToRestore.Has(ns.Name()) { + if !namespaceFilter.ShouldInclude(ns.Name()) { glog.Infof("Skipping namespace %s", ns.Name()) continue } + w, e := kr.restoreNamespace(restore, ns.Name(), nsPath, prioritizedResources, selector, backup) merge(&warnings, &w) merge(&errors, &e) @@ -453,7 +454,7 @@ func (kr *kubernetesRestorer) restoreResourceForNamespace( // add an ark-restore label to each resource for easy ID addLabel(unstructuredObj, api.RestoreLabelKey, restore.Name) - glog.Infof("Restoring item %v", unstructuredObj.GetName()) + glog.Infof("Restoring %s: %v", obj.GroupVersionKind().Kind, unstructuredObj.GetName()) _, err = resourceClient.Create(unstructuredObj) if apierrors.IsAlreadyExists(err) { addToResult(&warnings, namespace, err) diff --git a/pkg/restore/restore_test.go b/pkg/restore/restore_test.go index a88b2d37d..879732145 100644 --- a/pkg/restore/restore_test.go +++ b/pkg/restore/restore_test.go @@ -102,16 +102,37 @@ func TestRestoreMethod(t *testing.T) { name: "namespacesToRestore having * restores all namespaces", fileSystem: newFakeFileSystem().WithDirectories("bak/cluster", "bak/namespaces/a", "bak/namespaces/b", "bak/namespaces/c"), baseDir: "bak", - restore: &api.Restore{Spec: api.RestoreSpec{Namespaces: []string{"*"}}}, + restore: &api.Restore{Spec: api.RestoreSpec{IncludedNamespaces: []string{"*"}}}, expectedReadDirs: []string{"bak/cluster", "bak/namespaces", "bak/namespaces/a", "bak/namespaces/b", "bak/namespaces/c"}, }, { name: "namespacesToRestore properly filters", fileSystem: newFakeFileSystem().WithDirectories("bak/cluster", "bak/namespaces/a", "bak/namespaces/b", "bak/namespaces/c"), baseDir: "bak", - restore: &api.Restore{Spec: api.RestoreSpec{Namespaces: []string{"b", "c"}}}, + restore: &api.Restore{Spec: api.RestoreSpec{IncludedNamespaces: []string{"b", "c"}}}, expectedReadDirs: []string{"bak/cluster", "bak/namespaces", "bak/namespaces/b", "bak/namespaces/c"}, }, + { + name: "namespacesToRestore properly filters with inclusion filter", + fileSystem: newFakeFileSystem().WithDirectories("bak/cluster", "bak/namespaces/a", "bak/namespaces/b", "bak/namespaces/c"), + baseDir: "bak", + restore: &api.Restore{Spec: api.RestoreSpec{IncludedNamespaces: []string{"b", "c"}}}, + expectedReadDirs: []string{"bak/cluster", "bak/namespaces", "bak/namespaces/b", "bak/namespaces/c"}, + }, + { + name: "namespacesToRestore properly filters with exclusion filter", + fileSystem: newFakeFileSystem().WithDirectories("bak/cluster", "bak/namespaces/a", "bak/namespaces/b", "bak/namespaces/c"), + baseDir: "bak", + restore: &api.Restore{Spec: api.RestoreSpec{IncludedNamespaces: []string{"*"}, ExcludedNamespaces: []string{"a"}}}, + expectedReadDirs: []string{"bak/cluster", "bak/namespaces", "bak/namespaces/b", "bak/namespaces/c"}, + }, + { + name: "namespacesToRestore properly filters with inclusion & exclusion filters", + fileSystem: newFakeFileSystem().WithDirectories("bak/cluster", "bak/namespaces/a", "bak/namespaces/b", "bak/namespaces/c"), + baseDir: "bak", + restore: &api.Restore{Spec: api.RestoreSpec{IncludedNamespaces: []string{"a", "b", "c"}, ExcludedNamespaces: []string{"b"}}}, + expectedReadDirs: []string{"bak/cluster", "bak/namespaces", "bak/namespaces/a", "bak/namespaces/c"}, + }, } for _, test := range tests { diff --git a/pkg/restore/restorers/namespace_restorer.go b/pkg/restore/restorers/namespace_restorer.go index a9b9960aa..f9bb7f8d5 100644 --- a/pkg/restore/restorers/namespace_restorer.go +++ b/pkg/restore/restorers/namespace_restorer.go @@ -37,13 +37,10 @@ func (nsr *namespaceRestorer) Handles(obj runtime.Unstructured, restore *api.Res return false } - for _, restorableNS := range restore.Spec.Namespaces { - if restorableNS == nsName { - return true - } - } - - return false + return collections.NewIncludesExcludes(). + Includes(restore.Spec.IncludedNamespaces...). + Excludes(restore.Spec.ExcludedNamespaces...). + ShouldInclude(nsName) } func (nsr *namespaceRestorer) Prepare(obj runtime.Unstructured, restore *api.Restore, backup *api.Backup) (runtime.Unstructured, error, error) { diff --git a/pkg/restore/restorers/namespace_restorer_test.go b/pkg/restore/restorers/namespace_restorer_test.go index 486106f5c..9b575698a 100644 --- a/pkg/restore/restorers/namespace_restorer_test.go +++ b/pkg/restore/restorers/namespace_restorer_test.go @@ -37,19 +37,31 @@ func TestHandles(t *testing.T) { { name: "restorable NS", obj: NewTestUnstructured().WithName("ns-1").Unstructured, - restore: testutil.NewDefaultTestRestore().WithRestorableNamespace("ns-1").Restore, + restore: testutil.NewDefaultTestRestore().WithIncludedNamespace("ns-1").Restore, + expect: true, + }, + { + name: "restorable NS via wildcard", + obj: NewTestUnstructured().WithName("ns-1").Unstructured, + restore: testutil.NewDefaultTestRestore().WithIncludedNamespace("*").Restore, expect: true, }, { name: "non-restorable NS", obj: NewTestUnstructured().WithName("ns-1").Unstructured, - restore: testutil.NewDefaultTestRestore().WithRestorableNamespace("ns-2").Restore, + restore: testutil.NewDefaultTestRestore().WithIncludedNamespace("ns-2").Restore, + expect: false, + }, + { + name: "namespace is explicitly excluded", + obj: NewTestUnstructured().WithName("ns-1").Unstructured, + restore: testutil.NewDefaultTestRestore().WithIncludedNamespace("*").WithExcludedNamespace("ns-1").Restore, expect: false, }, { name: "namespace obj doesn't have name", obj: NewTestUnstructured().WithMetadata().Unstructured, - restore: testutil.NewDefaultTestRestore().WithRestorableNamespace("ns-1").Restore, + restore: testutil.NewDefaultTestRestore().WithIncludedNamespace("ns-1").Restore, expect: false, }, } diff --git a/pkg/util/test/test_restore.go b/pkg/util/test/test_restore.go index 0b1ac3904..d560db15b 100644 --- a/pkg/util/test/test_restore.go +++ b/pkg/util/test/test_restore.go @@ -45,11 +45,22 @@ func NewDefaultTestRestore() *TestRestore { return NewTestRestore(api.DefaultNamespace, "", api.RestorePhase("")) } -func (r *TestRestore) WithRestorableNamespace(name string) *TestRestore { + +func (r *TestRestore) WithNamespace(name string) *TestRestore { r.Spec.Namespaces = append(r.Spec.Namespaces, name) return r } +func (r *TestRestore) WithIncludedNamespace(name string) *TestRestore { + r.Spec.IncludedNamespaces = append(r.Spec.IncludedNamespaces, name) + return r +} + +func (r *TestRestore) WithExcludedNamespace(name string) *TestRestore { + r.Spec.ExcludedNamespaces = append(r.Spec.ExcludedNamespaces, name) + return r +} + func (r *TestRestore) WithValidationError(err string) *TestRestore { r.Status.ValidationErrors = append(r.Status.ValidationErrors, err) return r From b50a046370e84c6f603e414cdb831cba1488a2c1 Mon Sep 17 00:00:00 2001 From: Justin Nauman Date: Tue, 29 Aug 2017 21:14:21 -0500 Subject: [PATCH 2/4] Additional Validation on Include/Exclude - Adding in additional test to ensure *Namespaces attributes don't directly conflict logically with one another - Additional PR changes around naming/typos Signed-off-by: Justin Nauman --- pkg/apis/ark/v1/restore.go | 2 +- pkg/cmd/cli/restore/create.go | 4 ++-- pkg/controller/restore_controller.go | 7 ++++++- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/pkg/apis/ark/v1/restore.go b/pkg/apis/ark/v1/restore.go index 148ec8381..2e4aa8fd1 100644 --- a/pkg/apis/ark/v1/restore.go +++ b/pkg/apis/ark/v1/restore.go @@ -34,7 +34,7 @@ type RestoreSpec struct { IncludedNamespaces []string `json:"includedNamespaces"` // ExcludedNamespaces contains a list of namespaces that are not - // included in the backup. + // included in the restore. ExcludedNamespaces []string `json:"excludedNamespaces"` // NamespaceMapping is a map of source namespace names diff --git a/pkg/cmd/cli/restore/create.go b/pkg/cmd/cli/restore/create.go index 266e61978..2a0df90d6 100644 --- a/pkg/cmd/cli/restore/create.go +++ b/pkg/cmd/cli/restore/create.go @@ -73,8 +73,8 @@ func NewCreateOptions() *CreateOptions { } func (o *CreateOptions) BindFlags(flags *pflag.FlagSet) { - flags.Var(&o.IncludeNamespaces, "include-namespaces", "namespaces to include in the backup (use '*' for all namespaces)") - flags.Var(&o.ExcludeNamespaces, "exclude-namespaces", "namespaces to exclude from the backup") + flags.Var(&o.IncludeNamespaces, "include-namespaces", "namespaces to include in the restore (use '*' for all namespaces)") + flags.Var(&o.ExcludeNamespaces, "exclude-namespaces", "namespaces to exclude from the restore") flags.Var(&o.NamespaceMappings, "namespace-mappings", "namespace mappings from name in the backup to desired restored name in the form src1:dst1,src2:dst2,...") flags.Var(&o.Labels, "labels", "labels to apply to the restore") flags.VarP(&o.Selector, "selector", "l", "only restore resources matching this label selector") diff --git a/pkg/controller/restore_controller.go b/pkg/controller/restore_controller.go index a380d944f..73f089db3 100644 --- a/pkg/controller/restore_controller.go +++ b/pkg/controller/restore_controller.go @@ -39,6 +39,7 @@ import ( informers "github.com/heptio/ark/pkg/generated/informers/externalversions/ark/v1" listers "github.com/heptio/ark/pkg/generated/listers/ark/v1" "github.com/heptio/ark/pkg/restore" + "github.com/heptio/ark/pkg/util/collections" ) type restoreController struct { @@ -286,7 +287,11 @@ func (controller *restoreController) getValidationErrors(itm *api.Restore) []str } if len(itm.Spec.Namespaces) > 0 && len(itm.Spec.IncludedNamespaces) > 0 { - validationErrors = append(validationErrors, "Namespace and ItemNamespaces can not both be defined on the backup spec.") + validationErrors = append(validationErrors, "Namespaces and IncludedNamespaces can not both be defined on the backup spec.") + } + + for err := range collections.ValidateIncludesExcludes(itm.Spec.IncludedNamespaces, itm.Spec.ExcludedNamespaces) { + validationErrors = append(validationErrors, fmt.Sprintf("Invalid included/excluded namespace lists: %v", err)) } if !controller.pvProviderExists && itm.Spec.RestorePVs != nil && *itm.Spec.RestorePVs { From 2457d8f11636634409663d0ceddeecd27a779925 Mon Sep 17 00:00:00 2001 From: Justin Nauman Date: Tue, 5 Sep 2017 17:06:15 -0500 Subject: [PATCH 3/4] Removing instead of deprecating Namespace attr - Per discussion, there is no reason to deal with the complexity of backwards compatibility with the Namespace attribute on the Restore domain. - Also noticed there was an error on the validation of the BackupController where the message would actually just be the index of the error and not the contents of the message itself. Signed-off-by: Justin Nauman --- pkg/apis/ark/v1/restore.go | 5 ----- pkg/controller/backup_controller.go | 4 ++-- pkg/controller/restore_controller.go | 21 +++++---------------- pkg/controller/restore_controller_test.go | 8 ++++---- pkg/util/test/test_restore.go | 6 ------ 5 files changed, 11 insertions(+), 33 deletions(-) diff --git a/pkg/apis/ark/v1/restore.go b/pkg/apis/ark/v1/restore.go index 2e4aa8fd1..43b649db9 100644 --- a/pkg/apis/ark/v1/restore.go +++ b/pkg/apis/ark/v1/restore.go @@ -24,11 +24,6 @@ type RestoreSpec struct { // from. BackupName string `json:"backupName"` - // NOTE: This is deprecated. IncludedNamespaces and ExcludedNamespaces - // should be used instead - // Namespaces is a slice of namespaces in the Ark backup to restore. - Namespaces []string `json:"namespaces"` - // IncludedNamespaces is a slice of namespace names to include objects // from. If empty, all namespaces are included. IncludedNamespaces []string `json:"includedNamespaces"` diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index 3d7303202..1cfa36355 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -292,11 +292,11 @@ func cloneBackup(in interface{}) (*api.Backup, error) { func (controller *backupController) getValidationErrors(itm *api.Backup) []string { var validationErrors []string - for err := range collections.ValidateIncludesExcludes(itm.Spec.IncludedResources, itm.Spec.ExcludedResources) { + for _, err := range collections.ValidateIncludesExcludes(itm.Spec.IncludedResources, itm.Spec.ExcludedResources) { validationErrors = append(validationErrors, fmt.Sprintf("Invalid included/excluded resource lists: %v", err)) } - for err := range collections.ValidateIncludesExcludes(itm.Spec.IncludedNamespaces, itm.Spec.ExcludedNamespaces) { + for _, err := range collections.ValidateIncludesExcludes(itm.Spec.IncludedNamespaces, itm.Spec.ExcludedNamespaces) { validationErrors = append(validationErrors, fmt.Sprintf("Invalid included/excluded namespace lists: %v", err)) } diff --git a/pkg/controller/restore_controller.go b/pkg/controller/restore_controller.go index 73f089db3..76a2d52df 100644 --- a/pkg/controller/restore_controller.go +++ b/pkg/controller/restore_controller.go @@ -220,22 +220,15 @@ func (controller *restoreController) processRestore(key string) error { return err } + if len(restore.Spec.IncludedNamespaces) == 0 { + restore.Spec.IncludedNamespaces = []string{"*"} + } + // validation if restore.Status.ValidationErrors = controller.getValidationErrors(restore); len(restore.Status.ValidationErrors) > 0 { restore.Status.Phase = api.RestorePhaseFailedValidation } else { restore.Status.Phase = api.RestorePhaseInProgress - - if len(restore.Spec.Namespaces) != 0 { - glog.V(4).Info("the restore.Spec.Namespaces field has been deprecated. Please use the IncludedNamespaces and ExcludedNamespaces feature instead") - - restore.Spec.IncludedNamespaces = restore.Spec.Namespaces - restore.Spec.Namespaces = nil - } - - if len(restore.Spec.IncludedNamespaces) == 0 { - restore.Spec.IncludedNamespaces = []string{"*"} - } } // update status @@ -286,11 +279,7 @@ func (controller *restoreController) getValidationErrors(itm *api.Restore) []str validationErrors = append(validationErrors, "BackupName must be non-empty and correspond to the name of a backup in object storage.") } - if len(itm.Spec.Namespaces) > 0 && len(itm.Spec.IncludedNamespaces) > 0 { - validationErrors = append(validationErrors, "Namespaces and IncludedNamespaces can not both be defined on the backup spec.") - } - - for err := range collections.ValidateIncludesExcludes(itm.Spec.IncludedNamespaces, itm.Spec.ExcludedNamespaces) { + for _, err := range collections.ValidateIncludesExcludes(itm.Spec.IncludedNamespaces, itm.Spec.ExcludedNamespaces) { validationErrors = append(validationErrors, fmt.Sprintf("Invalid included/excluded namespace lists: %v", err)) } diff --git a/pkg/controller/restore_controller_test.go b/pkg/controller/restore_controller_test.go index 648d90d54..73f12b0c9 100644 --- a/pkg/controller/restore_controller_test.go +++ b/pkg/controller/restore_controller_test.go @@ -73,16 +73,16 @@ func TestProcessRestore(t *testing.T) { expectedErr: false, }, { - name: "restore with both namespaces and includedNamespaces fails validation", - restore: NewTestRestore("foo", "bar", api.RestorePhaseNew).WithBackup("backup-1").WithNamespace("ns-1").WithIncludedNamespace("another-1").Restore, + name: "restore with both namespace in both includedNamespaces and excludedNamespaces fails validation", + restore: NewTestRestore("foo", "bar", api.RestorePhaseNew).WithBackup("backup-1").WithIncludedNamespace("another-1").WithExcludedNamespace("another-1").Restore, backup: NewTestBackup().WithName("backup-1").Backup, expectedErr: false, expectedRestoreUpdates: []*api.Restore{ NewTestRestore("foo", "bar", api.RestorePhaseFailedValidation). WithBackup("backup-1"). - WithNamespace("ns-1"). WithIncludedNamespace("another-1"). - WithValidationError("Namespace and ItemNamespaces can not both be defined on the backup spec.").Restore, + WithExcludedNamespace("another-1"). + WithValidationError("Invalid included/excluded namespace lists: excludes list cannot contain an item in the includes list: another-1").Restore, }, }, { diff --git a/pkg/util/test/test_restore.go b/pkg/util/test/test_restore.go index d560db15b..57ca9a094 100644 --- a/pkg/util/test/test_restore.go +++ b/pkg/util/test/test_restore.go @@ -45,12 +45,6 @@ func NewDefaultTestRestore() *TestRestore { return NewTestRestore(api.DefaultNamespace, "", api.RestorePhase("")) } - -func (r *TestRestore) WithNamespace(name string) *TestRestore { - r.Spec.Namespaces = append(r.Spec.Namespaces, name) - return r -} - func (r *TestRestore) WithIncludedNamespace(name string) *TestRestore { r.Spec.IncludedNamespaces = append(r.Spec.IncludedNamespaces, name) return r From 2d084c43486104b6f86bfd0e6800ff72cb537c6b Mon Sep 17 00:00:00 2001 From: Justin Nauman Date: Tue, 5 Sep 2017 17:06:15 -0500 Subject: [PATCH 4/4] Removing instead of deprecating Namespace attr - Per discussion, there is no reason to deal with the complexity of backwards compatibility with the Namespace attribute on the Restore domain. - Also noticed there was an error on the validation of the BackupController where the message would actually just be the index of the error and not the contents of the message itself. Signed-off-by: Justin Nauman --- docs/cli-reference/ark_restore_create.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/cli-reference/ark_restore_create.md b/docs/cli-reference/ark_restore_create.md index b46a89e7b..dfe86214f 100644 --- a/docs/cli-reference/ark_restore_create.md +++ b/docs/cli-reference/ark_restore_create.md @@ -14,8 +14,8 @@ ark restore create BACKUP ### Options ``` - --exclude-namespaces stringArray namespaces to exclude from the backup - --include-namespaces stringArray namespaces to include in the backup (use '*' for all namespaces) (default *) + --exclude-namespaces stringArray namespaces to exclude from the restore + --include-namespaces stringArray namespaces to include in the restore (use '*' for all namespaces) (default *) --label-columns stringArray a comma-separated list of labels to be displayed as columns --labels mapStringString labels to apply to the restore --namespace-mappings mapStringString namespace mappings from name in the backup to desired restored name in the form src1:dst1,src2:dst2,...