From e1bef5b6c271ab89bb6ff004124a87a482820d6b Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Mon, 29 Apr 2024 16:06:49 -0400 Subject: [PATCH 1/2] Add existingResourcePolicy restore CR validation to controller. Signed-off-by: Tiger Kaovilai --- changelogs/unreleased/7757-kaovilai | 1 + pkg/cmd/cli/restore/create.go | 10 ++-------- pkg/cmd/cli/restore/create_test.go | 6 ------ pkg/controller/restore_controller.go | 6 ++++++ pkg/restore/util/util.go | 12 ++++++++++++ pkg/restore/util/util_test.go | 15 +++++++++++++++ 6 files changed, 36 insertions(+), 14 deletions(-) create mode 100644 changelogs/unreleased/7757-kaovilai create mode 100644 pkg/restore/util/util.go create mode 100644 pkg/restore/util/util_test.go diff --git a/changelogs/unreleased/7757-kaovilai b/changelogs/unreleased/7757-kaovilai new file mode 100644 index 0000000000..92317ed6b7 --- /dev/null +++ b/changelogs/unreleased/7757-kaovilai @@ -0,0 +1 @@ +Add existingResourcePolicy restore CR validation to controller diff --git a/pkg/cmd/cli/restore/create.go b/pkg/cmd/cli/restore/create.go index e855ed99ca..9b779c2171 100644 --- a/pkg/cmd/cli/restore/create.go +++ b/pkg/cmd/cli/restore/create.go @@ -37,6 +37,7 @@ import ( "github.com/vmware-tanzu/velero/pkg/cmd" "github.com/vmware-tanzu/velero/pkg/cmd/util/flag" "github.com/vmware-tanzu/velero/pkg/cmd/util/output" + "github.com/vmware-tanzu/velero/pkg/restore/util" "github.com/vmware-tanzu/velero/pkg/util/boolptr" "github.com/vmware-tanzu/velero/pkg/util/kube" ) @@ -199,7 +200,7 @@ func (o *CreateOptions) Validate(c *cobra.Command, args []string, f client.Facto return errors.New("either a 'selector' or an 'or-selector' can be specified, but not both") } - if len(o.ExistingResourcePolicy) > 0 && !isResourcePolicyValid(o.ExistingResourcePolicy) { + if len(o.ExistingResourcePolicy) > 0 && !util.IsResourcePolicyValid(o.ExistingResourcePolicy) { return errors.New("existing-resource-policy has invalid value, it accepts only none, update as value") } @@ -428,10 +429,3 @@ func (o *CreateOptions) Run(c *cobra.Command, f client.Factory) error { return nil } - -func isResourcePolicyValid(resourcePolicy string) bool { - if resourcePolicy == string(api.PolicyTypeNone) || resourcePolicy == string(api.PolicyTypeUpdate) { - return true - } - return false -} diff --git a/pkg/cmd/cli/restore/create_test.go b/pkg/cmd/cli/restore/create_test.go index 4fbe7f3723..bf771bfe3b 100644 --- a/pkg/cmd/cli/restore/create_test.go +++ b/pkg/cmd/cli/restore/create_test.go @@ -34,12 +34,6 @@ import ( velerotest "github.com/vmware-tanzu/velero/pkg/test" ) -func TestIsResourcePolicyValid(t *testing.T) { - require.True(t, isResourcePolicyValid(string(velerov1api.PolicyTypeNone))) - require.True(t, isResourcePolicyValid(string(velerov1api.PolicyTypeUpdate))) - require.False(t, isResourcePolicyValid("")) -} - func TestMostRecentBackup(t *testing.T) { backups := []velerov1api.Backup{ *builder.ForBackup(cmdtest.VeleroNameSpace, "backup0").StartTimestamp(time.Now().Add(3 * time.Second)).Phase(velerov1api.BackupPhaseDeleting).Result(), diff --git a/pkg/controller/restore_controller.go b/pkg/controller/restore_controller.go index 86372b453a..d748a90568 100644 --- a/pkg/controller/restore_controller.go +++ b/pkg/controller/restore_controller.go @@ -53,6 +53,7 @@ import ( "github.com/vmware-tanzu/velero/pkg/plugin/clientmgmt" "github.com/vmware-tanzu/velero/pkg/plugin/framework" pkgrestore "github.com/vmware-tanzu/velero/pkg/restore" + "github.com/vmware-tanzu/velero/pkg/restore/util" "github.com/vmware-tanzu/velero/pkg/util/collections" kubeutil "github.com/vmware-tanzu/velero/pkg/util/kube" "github.com/vmware-tanzu/velero/pkg/util/logging" @@ -346,6 +347,11 @@ func (r *restoreReconciler) validateAndComplete(restore *api.Restore) (backupInf } } + // validate ExistingResourcePolicy + if restore.Spec.ExistingResourcePolicy != "" && !util.IsResourcePolicyValid(string(restore.Spec.ExistingResourcePolicy)) { + restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, fmt.Sprintf("Invalid ExistingResourcePolicy: %s", restore.Spec.ExistingResourcePolicy)) + } + // if ScheduleName is specified, fill in BackupName with the most recent successful backup from // the schedule if restore.Spec.ScheduleName != "" { diff --git a/pkg/restore/util/util.go b/pkg/restore/util/util.go new file mode 100644 index 0000000000..d25ad8819a --- /dev/null +++ b/pkg/restore/util/util.go @@ -0,0 +1,12 @@ +package util + +import ( + api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" +) + +func IsResourcePolicyValid(resourcePolicy string) bool { + if resourcePolicy == string(api.PolicyTypeNone) || resourcePolicy == string(api.PolicyTypeUpdate) { + return true + } + return false +} diff --git a/pkg/restore/util/util_test.go b/pkg/restore/util/util_test.go new file mode 100644 index 0000000000..71c516f9ca --- /dev/null +++ b/pkg/restore/util/util_test.go @@ -0,0 +1,15 @@ +package util + +import ( + "testing" + + "github.com/stretchr/testify/require" + + velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" +) + +func TestIsResourcePolicyValid(t *testing.T) { + require.True(t, IsResourcePolicyValid(string(velerov1api.PolicyTypeNone))) + require.True(t, IsResourcePolicyValid(string(velerov1api.PolicyTypeUpdate))) + require.False(t, IsResourcePolicyValid("")) +} From 3c937d42dd7343558620ae35b61d6746557b1e78 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Wed, 1 May 2024 11:39:24 -0400 Subject: [PATCH 2/2] ignore .git dir when formatting Signed-off-by: Tiger Kaovilai --- hack/update-1fmt.sh | 2 +- pkg/cmd/cli/restore/create.go | 4 +-- pkg/controller/restore_controller.go | 4 +-- pkg/controller/restore_controller_test.go | 33 +++++++++++++++++++ .../util => util/velero/restore}/util.go | 2 +- .../util => util/velero/restore}/util_test.go | 2 +- 6 files changed, 40 insertions(+), 7 deletions(-) rename pkg/{restore/util => util/velero/restore}/util.go (94%) rename pkg/{restore/util => util/velero/restore}/util_test.go (95%) diff --git a/hack/update-1fmt.sh b/hack/update-1fmt.sh index 204801bdf8..a23261cb2a 100755 --- a/hack/update-1fmt.sh +++ b/hack/update-1fmt.sh @@ -33,7 +33,7 @@ if ! command -v goimports > /dev/null; then exit 1 fi -files="$(find . -type f -name '*.go' -not -path './.go/*' -not -path './vendor/*' -not -path './site/*' -not -path '*/generated/*' -not -name 'zz_generated*' -not -path '*/mocks/*')" +files="$(find . -type f -name '*.go' -not -path './.go/*' -not -path './vendor/*' -not -path './site/*' -not -path './.git/*' -not -path '*/generated/*' -not -name 'zz_generated*' -not -path '*/mocks/*')" echo "${ACTION} gofmt" output=$(gofmt "${MODE}" -s ${files}) if [[ -n "${output}" ]]; then diff --git a/pkg/cmd/cli/restore/create.go b/pkg/cmd/cli/restore/create.go index 9b779c2171..6d4b25f0ab 100644 --- a/pkg/cmd/cli/restore/create.go +++ b/pkg/cmd/cli/restore/create.go @@ -37,9 +37,9 @@ import ( "github.com/vmware-tanzu/velero/pkg/cmd" "github.com/vmware-tanzu/velero/pkg/cmd/util/flag" "github.com/vmware-tanzu/velero/pkg/cmd/util/output" - "github.com/vmware-tanzu/velero/pkg/restore/util" "github.com/vmware-tanzu/velero/pkg/util/boolptr" "github.com/vmware-tanzu/velero/pkg/util/kube" + "github.com/vmware-tanzu/velero/pkg/util/velero/restore" ) func NewCreateCommand(f client.Factory, use string) *cobra.Command { @@ -200,7 +200,7 @@ func (o *CreateOptions) Validate(c *cobra.Command, args []string, f client.Facto return errors.New("either a 'selector' or an 'or-selector' can be specified, but not both") } - if len(o.ExistingResourcePolicy) > 0 && !util.IsResourcePolicyValid(o.ExistingResourcePolicy) { + if len(o.ExistingResourcePolicy) > 0 && !restore.IsResourcePolicyValid(o.ExistingResourcePolicy) { return errors.New("existing-resource-policy has invalid value, it accepts only none, update as value") } diff --git a/pkg/controller/restore_controller.go b/pkg/controller/restore_controller.go index d748a90568..31b060e123 100644 --- a/pkg/controller/restore_controller.go +++ b/pkg/controller/restore_controller.go @@ -53,11 +53,11 @@ import ( "github.com/vmware-tanzu/velero/pkg/plugin/clientmgmt" "github.com/vmware-tanzu/velero/pkg/plugin/framework" pkgrestore "github.com/vmware-tanzu/velero/pkg/restore" - "github.com/vmware-tanzu/velero/pkg/restore/util" "github.com/vmware-tanzu/velero/pkg/util/collections" kubeutil "github.com/vmware-tanzu/velero/pkg/util/kube" "github.com/vmware-tanzu/velero/pkg/util/logging" "github.com/vmware-tanzu/velero/pkg/util/results" + pkgrestoreUtil "github.com/vmware-tanzu/velero/pkg/util/velero/restore" ) // nonRestorableResources is an exclusion list for the restoration process. Any resources @@ -348,7 +348,7 @@ func (r *restoreReconciler) validateAndComplete(restore *api.Restore) (backupInf } // validate ExistingResourcePolicy - if restore.Spec.ExistingResourcePolicy != "" && !util.IsResourcePolicyValid(string(restore.Spec.ExistingResourcePolicy)) { + if restore.Spec.ExistingResourcePolicy != "" && !pkgrestoreUtil.IsResourcePolicyValid(string(restore.Spec.ExistingResourcePolicy)) { restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, fmt.Sprintf("Invalid ExistingResourcePolicy: %s", restore.Spec.ExistingResourcePolicy)) } diff --git a/pkg/controller/restore_controller_test.go b/pkg/controller/restore_controller_test.go index 1b60f95326..e124a98e91 100644 --- a/pkg/controller/restore_controller_test.go +++ b/pkg/controller/restore_controller_test.go @@ -312,6 +312,39 @@ func TestRestoreReconcile(t *testing.T) { expectedRestoreErrors: 1, expectedRestorerCall: NewRestore("foo", "bar", "backup-1", "ns-1", "", velerov1api.RestorePhaseInProgress).Result(), }, + { + name: "valid restore with none existingresourcepolicy gets executed", + location: defaultStorageLocation, + restore: NewRestore("foo", "bar", "backup-1", "ns-1", "", velerov1api.RestorePhaseNew).ExistingResourcePolicy("none").Result(), + backup: defaultBackup().StorageLocation("default").Result(), + expectedErr: false, + expectedPhase: string(velerov1api.RestorePhaseInProgress), + expectedStartTime: ×tamp, + expectedCompletedTime: ×tamp, + expectedRestorerCall: NewRestore("foo", "bar", "backup-1", "ns-1", "", velerov1api.RestorePhaseInProgress).ExistingResourcePolicy("none").Result(), + }, + { + name: "valid restore with update existingresourcepolicy gets executed", + location: defaultStorageLocation, + restore: NewRestore("foo", "bar", "backup-1", "ns-1", "", velerov1api.RestorePhaseNew).ExistingResourcePolicy("update").Result(), + backup: defaultBackup().StorageLocation("default").Result(), + expectedErr: false, + expectedPhase: string(velerov1api.RestorePhaseInProgress), + expectedStartTime: ×tamp, + expectedCompletedTime: ×tamp, + expectedRestorerCall: NewRestore("foo", "bar", "backup-1", "ns-1", "", velerov1api.RestorePhaseInProgress).ExistingResourcePolicy("update").Result(), + }, + { + name: "invalid restore with invalid existingresourcepolicy errors", + location: defaultStorageLocation, + restore: NewRestore("foo", "invalidexistingresourcepolicy", "backup-1", "ns-1", "", velerov1api.RestorePhaseNew).ExistingResourcePolicy("invalid").Result(), + backup: defaultBackup().StorageLocation("default").Result(), + expectedErr: false, + expectedPhase: string(velerov1api.RestorePhaseFailedValidation), + expectedStartTime: ×tamp, + expectedCompletedTime: ×tamp, + expectedRestorerCall: nil, // this restore should fail validation and not be passed to the restorer + }, { name: "valid restore gets executed", location: defaultStorageLocation, diff --git a/pkg/restore/util/util.go b/pkg/util/velero/restore/util.go similarity index 94% rename from pkg/restore/util/util.go rename to pkg/util/velero/restore/util.go index d25ad8819a..e0812884b4 100644 --- a/pkg/restore/util/util.go +++ b/pkg/util/velero/restore/util.go @@ -1,4 +1,4 @@ -package util +package restore import ( api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" diff --git a/pkg/restore/util/util_test.go b/pkg/util/velero/restore/util_test.go similarity index 95% rename from pkg/restore/util/util_test.go rename to pkg/util/velero/restore/util_test.go index 71c516f9ca..be72ff8bac 100644 --- a/pkg/restore/util/util_test.go +++ b/pkg/util/velero/restore/util_test.go @@ -1,4 +1,4 @@ -package util +package restore import ( "testing"