Skipping validation for volumesnapshotlocation for backup if snapshotvolume set to false (#2450)

* Disabling validation for volumesnapshotlocation if the backup has snapshotvolume set to false

Signed-off-by: mayank <mayank.patel@mayadata.io>

* adding a changelog

Signed-off-by: mayank <mayank.patel@mayadata.io>

* addressing review comment

Signed-off-by: mayank <mayank.patel@mayadata.io>

changelogs/unreleased/2450-mynktl

@@ -0,0 +1 @@
+Support to skip VSL validation for the backup having SnapshotVolumes set to false or created with `--snapshot-volumes=false`

pkg/backup/item_backupper.go

@@ -41,6 +41,7 @@ import (
 	"github.com/vmware-tanzu/velero/pkg/plugin/velero"
 	"github.com/vmware-tanzu/velero/pkg/podexec"
 	"github.com/vmware-tanzu/velero/pkg/restic"
+	"github.com/vmware-tanzu/velero/pkg/util/boolptr"
 	"github.com/vmware-tanzu/velero/pkg/volume"
 )
 
@@ -399,7 +400,7 @@ const (
 func (ib *defaultItemBackupper) takePVSnapshot(obj runtime.Unstructured, log logrus.FieldLogger) error {
 	log.Info("Executing takePVSnapshot")
 
-	if ib.backupRequest.Spec.SnapshotVolumes != nil && !*ib.backupRequest.Spec.SnapshotVolumes {
+	if boolptr.IsSetToFalse(ib.backupRequest.Spec.SnapshotVolumes) {
 		log.Info("Backup has volume snapshots disabled; skipping volume snapshot action.")
 		return nil
 	}

pkg/controller/backup_controller.go

@@ -51,6 +51,7 @@ import (
 	"github.com/vmware-tanzu/velero/pkg/metrics"
 	"github.com/vmware-tanzu/velero/pkg/persistence"
 	"github.com/vmware-tanzu/velero/pkg/plugin/clientmgmt"
+	"github.com/vmware-tanzu/velero/pkg/util/boolptr"
 	"github.com/vmware-tanzu/velero/pkg/util/collections"
 	"github.com/vmware-tanzu/velero/pkg/util/encode"
 	kubeutil "github.com/vmware-tanzu/velero/pkg/util/kube"
@@ -397,10 +398,16 @@ func (c *backupController) prepareBackupRequest(backup *velerov1api.Backup) *pkg
 // - a given provider's default location name is added to .spec.volumeSnapshotLocations if one
 //   is not explicitly specified for the provider (if there's only one location for the provider,
 //   it will automatically be used)
+// if backup has snapshotVolume disabled then it returns empty VSL
 func (c *backupController) validateAndGetSnapshotLocations(backup *velerov1api.Backup) (map[string]*velerov1api.VolumeSnapshotLocation, []string) {
 	errors := []string{}
 	providerLocations := make(map[string]*velerov1api.VolumeSnapshotLocation)
 
+	// if snapshotVolume is set to false then we don't need to validate volumesnapshotlocation
+	if boolptr.IsSetToFalse(backup.Spec.SnapshotVolumes) {
+		return nil, nil
+	}
+
 	for _, locationName := range backup.Spec.VolumeSnapshotLocations {
 		// validate each locationName exists as a VolumeSnapshotLocation
 		location, err := c.snapshotLocationLister.VolumeSnapshotLocations(backup.Namespace).Get(locationName)

pkg/controller/backup_controller_test.go

@@ -767,6 +767,46 @@ func TestValidateAndGetSnapshotLocations(t *testing.T) {
 			expectedVolumeSnapshotLocationNames: []string{"aws-us-west-1", "some-name"},
 			expectedSuccess:                     true,
 		},
+		{
+			name:   "location name does not correspond to any existing location and snapshotvolume disabled; should return empty VSL and no error",
+			backup: defaultBackup().Phase(velerov1api.BackupPhaseNew).VolumeSnapshotLocations("random-name").SnapshotVolumes(false).Result(),
+			locations: []*velerov1api.VolumeSnapshotLocation{
+				builder.ForVolumeSnapshotLocation(velerov1api.DefaultNamespace, "aws-us-east-1").Provider("aws").Result(),
+				builder.ForVolumeSnapshotLocation(velerov1api.DefaultNamespace, "aws-us-west-1").Provider("aws").Result(),
+				builder.ForVolumeSnapshotLocation(velerov1api.DefaultNamespace, "some-name").Provider("fake-provider").Result(),
+			},
+			expectedVolumeSnapshotLocationNames: nil,
+			expectedSuccess:                     true,
+		},
+		{
+			name:   "duplicate locationName per provider and snapshotvolume disabled; should return empty VSL and no error",
+			backup: defaultBackup().Phase(velerov1api.BackupPhaseNew).VolumeSnapshotLocations("aws-us-west-1", "aws-us-west-1").SnapshotVolumes(false).Result(),
+			locations: []*velerov1api.VolumeSnapshotLocation{
+				builder.ForVolumeSnapshotLocation(velerov1api.DefaultNamespace, "aws-us-east-1").Provider("aws").Result(),
+				builder.ForVolumeSnapshotLocation(velerov1api.DefaultNamespace, "aws-us-west-1").Provider("aws").Result(),
+			},
+			expectedVolumeSnapshotLocationNames: nil,
+			expectedSuccess:                     true,
+		},
+		{
+			name:   "no location name for the provider exists, only one VSL created and snapshotvolume disabled; should return empty VSL and no error",
+			backup: defaultBackup().Phase(velerov1api.BackupPhaseNew).SnapshotVolumes(false).Result(),
+			locations: []*velerov1api.VolumeSnapshotLocation{
+				builder.ForVolumeSnapshotLocation(velerov1api.DefaultNamespace, "aws-us-east-1").Provider("aws").Result(),
+			},
+			expectedVolumeSnapshotLocationNames: nil,
+			expectedSuccess:                     true,
+		},
+		{
+			name:   "multiple location names for a provider, no default location and backup has no location defined, but snapshotvolume disabled, should return empty VSL and no error",
+			backup: defaultBackup().Phase(velerov1api.BackupPhaseNew).SnapshotVolumes(false).Result(),
+			locations: []*velerov1api.VolumeSnapshotLocation{
+				builder.ForVolumeSnapshotLocation(velerov1api.DefaultNamespace, "aws-us-west-1").Provider("aws").Result(),
+				builder.ForVolumeSnapshotLocation(velerov1api.DefaultNamespace, "aws-us-east-1").Provider("aws").Result(),
+			},
+			expectedVolumeSnapshotLocationNames: nil,
+			expectedSuccess:                     true,
+		},
 	}
 
 	for _, test := range tests {