Fix OADP-6459: Adding a backupLocation to DPA with an existing backupLocation name is not rejected (#1867)

* Fix OADP-6459: Adding a backupLocation to DPA with an existing backupLcation name is not rejected

* add func for bslName

Author: shubham-pampattiwar

internal/controller/bsl.go

@@ -19,11 +19,40 @@ import (
 	"github.com/openshift/oadp-operator/pkg/storage/aws"
 )
 
+// getBSLName generates the BackupStorageLocation name for a given backup location spec and index.
+// It returns the user-provided name if specified, otherwise generates a name using the DPA name and index.
+func (r *DataProtectionApplicationReconciler) getBSLName(bslSpec *oadpv1alpha1.BackupLocation, index int) string {
+	if bslSpec.Name != "" {
+		return bslSpec.Name
+	}
+	return fmt.Sprintf("%s-%d", r.NamespacedName.Name, index+1)
+}
+
 func (r *DataProtectionApplicationReconciler) ValidateBackupStorageLocations() (bool, error) {
 	// Ensure BSL is a valid configuration
 	// First, check for provider and then call functions based on the cloud provider for each backupstoragelocation configured
 	dpa := r.dpa
 	numDefaultLocations := 0
+	namesSeen := make(map[string]bool)
+
+	// Check for duplicate backup location names and validate name format
+	for i, bslSpec := range dpa.Spec.BackupLocations {
+		// Validate that user-provided names are not empty strings
+		if bslSpec.Name == "" {
+			// Empty names are allowed and will be auto-generated, skip validation
+		} else if strings.TrimSpace(bslSpec.Name) == "" {
+			return false, fmt.Errorf("backup location name cannot be empty or whitespace only")
+		}
+
+		// Determine the BSL name using the helper function
+		bslName := r.getBSLName(&bslSpec, i)
+
+		if namesSeen[bslName] {
+			return false, fmt.Errorf("backup location name '%s' is duplicated. Backup location names must be unique", bslName)
+		}
+		namesSeen[bslName] = true
+	}
+
 	for _, bslSpec := range dpa.Spec.BackupLocations {
 		if err := r.ensureBackupLocationHasVeleroOrCloudStorage(&bslSpec); err != nil {
 			return false, err
@@ -96,11 +125,8 @@ func (r *DataProtectionApplicationReconciler) ReconcileBackupStorageLocations(lo
 		// Create BSL as is, we can safely assume they are valid from
 		// ValidateBackupStorageLocations
 
-		// check if BSL name is specified in DPA spec
-		bslName := fmt.Sprintf("%s-%d", r.NamespacedName.Name, i+1)
-		if bslSpec.Name != "" {
-			bslName = bslSpec.Name
-		}
+		// Get BSL name using the helper function
+		bslName := r.getBSLName(&bslSpec, i)
 		dpaBSLNames = append(dpaBSLNames, bslName)
 
 		bsl := velerov1.BackupStorageLocation{

internal/controller/bsl_test.go

@@ -1494,6 +1494,147 @@ func TestDPAReconciler_ValidateBackupStorageLocations(t *testing.T) {
 			want:    false,
 			wantErr: true,
 		},
+		{
+			name: "test duplicate backup location names should fail validation",
+			dpa: &oadpv1alpha1.DataProtectionApplication{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "foo",
+					Namespace: "test-ns",
+				},
+				Spec: oadpv1alpha1.DataProtectionApplicationSpec{
+					Configuration: &oadpv1alpha1.ApplicationConfig{
+						Velero: &oadpv1alpha1.VeleroConfig{
+							DefaultPlugins: []oadpv1alpha1.DefaultPlugin{
+								oadpv1alpha1.DefaultPluginAWS,
+								oadpv1alpha1.DefaultPluginMicrosoftAzure,
+							},
+						},
+					},
+					BackupLocations: []oadpv1alpha1.BackupLocation{
+						{
+							Name: "duplicate-name",
+							Velero: &velerov1.BackupStorageLocationSpec{
+								Provider: "aws",
+								Default:  true,
+								StorageType: velerov1.StorageType{
+									ObjectStorage: &velerov1.ObjectStorageLocation{
+										Bucket: "test-aws-bucket",
+										Prefix: "velero/backups",
+									},
+								},
+								Config: map[string]string{
+									"region": "us-east-1",
+								},
+								Credential: &corev1.SecretKeySelector{
+									LocalObjectReference: corev1.LocalObjectReference{
+										Name: "cloud-credentials",
+									},
+									Key: "cloud",
+								},
+							},
+						},
+						{
+							Name: "duplicate-name", // Same name as above
+							Velero: &velerov1.BackupStorageLocationSpec{
+								Provider: "azure",
+								Default:  false,
+								StorageType: velerov1.StorageType{
+									ObjectStorage: &velerov1.ObjectStorageLocation{
+										Bucket: "test-azure-bucket",
+										Prefix: "velero/backups",
+									},
+								},
+								Config: map[string]string{
+									"resourceGroup":  "test-rg",
+									"storageAccount": "test-sa",
+								},
+								Credential: &corev1.SecretKeySelector{
+									LocalObjectReference: corev1.LocalObjectReference{
+										Name: "azure-credentials",
+									},
+									Key: "cloud",
+								},
+							},
+						},
+					},
+				},
+			},
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "cloud-credentials",
+					Namespace: "test-ns",
+				},
+				Data: map[string][]byte{
+					"cloud": []byte("[default]\naws_access_key_id=test\naws_secret_access_key=test"),
+				},
+			},
+			objects: []client.Object{
+				&corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "azure-credentials",
+						Namespace: "test-ns",
+					},
+					Data: map[string][]byte{
+						"cloud": []byte("AZURE_SUBSCRIPTION_ID=test\nAZURE_TENANT_ID=test\nAZURE_CLIENT_ID=test\nAZURE_CLIENT_SECRET=test\nAZURE_RESOURCE_GROUP=test-rg\nAZURE_CLOUD_NAME=AzurePublicCloud"),
+					},
+				},
+			},
+			want:    false,
+			wantErr: true,
+		},
+		{
+			name: "test backup location with whitespace-only name should fail validation",
+			dpa: &oadpv1alpha1.DataProtectionApplication{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "foo",
+					Namespace: "test-ns",
+				},
+				Spec: oadpv1alpha1.DataProtectionApplicationSpec{
+					Configuration: &oadpv1alpha1.ApplicationConfig{
+						Velero: &oadpv1alpha1.VeleroConfig{
+							DefaultPlugins: []oadpv1alpha1.DefaultPlugin{
+								oadpv1alpha1.DefaultPluginAWS,
+							},
+						},
+					},
+					BackupLocations: []oadpv1alpha1.BackupLocation{
+						{
+							Name: "   ", // Whitespace-only name should fail
+							Velero: &velerov1.BackupStorageLocationSpec{
+								Provider: "aws",
+								Default:  true,
+								StorageType: velerov1.StorageType{
+									ObjectStorage: &velerov1.ObjectStorageLocation{
+										Bucket: "test-aws-bucket",
+										Prefix: "velero/backups",
+									},
+								},
+								Config: map[string]string{
+									"region": "us-east-1",
+								},
+								Credential: &corev1.SecretKeySelector{
+									LocalObjectReference: corev1.LocalObjectReference{
+										Name: "cloud-credentials",
+									},
+									Key: "cloud",
+								},
+							},
+						},
+					},
+				},
+			},
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "cloud-credentials",
+					Namespace: "test-ns",
+				},
+				Data: map[string][]byte{
+					"cloud": []byte("[default]\naws_access_key_id=test\naws_secret_access_key=test"),
+				},
+			},
+			want:    false,
+			wantErr: true,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {