OADP-5777: Require region in BSL validation when s3Url is set, fix nil config skipping region auto-detection

internal/controller/bsl.go

@@ -413,7 +413,7 @@ func (r *DataProtectionApplicationReconciler) updateBSLFromSpec(bsl *velerov1.Ba
 	}
 
 	// Update BSL spec and registry-deployment label
-	if err := common.UpdateBackupStorageLocation(bsl, bslSpec); err != nil {
+	if err := common.UpdateBackupStorageLocation(bsl, bslSpec, r.Log); err != nil {
 		return err
 	}
 
@@ -527,12 +527,20 @@ func (r *DataProtectionApplicationReconciler) validateAWSBackupStorageLocation(b
 	}
 
 	// BSL region is required when
+	// - s3Url is set, because the user is pointing to a non-AWS S3-compatible service
+	//   where region auto-discovery via AWS API is not valid
 	// - s3ForcePathStyle is true, because some velero processes requires region to be set and is not auto-discoverable when s3ForcePathStyle is true
 	//   imagestream backup in openshift-velero-plugin now uses the same method to discover region as the rest of the velero codebase
 	// - even when s3ForcePathStyle is false, some aws bucket regions may not be discoverable and the user has to set it manually
-	if (bslSpec.Config == nil || len(bslSpec.Config[Region]) == 0) &&
-		(bslSpec.Config != nil && bslSpec.Config[S3ForcePathStyle] == "true" || !aws.BucketRegionIsDiscoverable(bslSpec.ObjectStorage.Bucket)) {
-		return fmt.Errorf("region for AWS backupstoragelocation not automatically discoverable. Please set the region in the backupstoragelocation config")
+	if bslSpec.Config == nil || len(bslSpec.Config[Region]) == 0 {
+		switch {
+		case bslSpec.Config != nil && len(bslSpec.Config[S3URL]) > 0:
+			return fmt.Errorf("region for AWS backupstoragelocation is required when s3Url is configured. Please set the region in the backupstoragelocation config")
+		case bslSpec.Config != nil && bslSpec.Config[S3ForcePathStyle] == "true":
+			return fmt.Errorf("region for AWS backupstoragelocation is required when s3ForcePathStyle is true. Please set the region in the backupstoragelocation config")
+		case !aws.BucketRegionIsDiscoverable(bslSpec.ObjectStorage.Bucket):
+			return fmt.Errorf("region for AWS backupstoragelocation not automatically discoverable. Please set the region in the backupstoragelocation config")
+		}
 	}
 
 	//TODO: Add minio, noobaa, local storage validations

internal/controller/bsl_test.go

@@ -1459,6 +1459,131 @@ func TestDPAReconciler_ValidateBackupStorageLocations(t *testing.T) {
 			want:    true,
 			wantErr: false,
 		},
+		{
+			name: "BSL with s3Url and no region expect to fail",
+			dpa: &oadpv1alpha1.DataProtectionApplication{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "foo",
+					Namespace: "test-ns",
+				},
+				Spec: oadpv1alpha1.DataProtectionApplicationSpec{
+					Configuration: &oadpv1alpha1.ApplicationConfig{
+						Velero: &oadpv1alpha1.VeleroConfig{},
+					},
+					BackupImages: ptr.To(false),
+					BackupLocations: []oadpv1alpha1.BackupLocation{
+						{
+							Velero: &velerov1.BackupStorageLocationSpec{
+								Provider: "aws",
+								Config: map[string]string{
+									S3URL: "https://s3.us-south.cloud-object-storage.appdomain.cloud",
+								},
+								StorageType: velerov1.StorageType{
+									ObjectStorage: &velerov1.ObjectStorageLocation{
+										Bucket: DiscoverableBucket,
+										Prefix: "prefix",
+									},
+								},
+								Default: true,
+							},
+						},
+					},
+				},
+			},
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "cloud-credentials",
+					Namespace: "test-ns",
+				},
+				Data: map[string][]byte{"cloud": []byte("[default]\naws_access_key_id=AKIAIOSFODNN7EXAMPLE\naws_secret_access_key=wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY")},
+			},
+			want:    false,
+			wantErr: true,
+		},
+		{
+			name: "BSL with s3Url and region set expect to succeed",
+			dpa: &oadpv1alpha1.DataProtectionApplication{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "foo",
+					Namespace: "test-ns",
+				},
+				Spec: oadpv1alpha1.DataProtectionApplicationSpec{
+					Configuration: &oadpv1alpha1.ApplicationConfig{
+						Velero: &oadpv1alpha1.VeleroConfig{},
+					},
+					BackupImages: ptr.To(false),
+					BackupLocations: []oadpv1alpha1.BackupLocation{
+						{
+							Velero: &velerov1.BackupStorageLocationSpec{
+								Provider: "aws",
+								Config: map[string]string{
+									S3URL:  "https://s3.us-south.cloud-object-storage.appdomain.cloud",
+									Region: "us-south",
+								},
+								StorageType: velerov1.StorageType{
+									ObjectStorage: &velerov1.ObjectStorageLocation{
+										Bucket: "ibm-cos-bucket",
+										Prefix: "prefix",
+									},
+								},
+								Default: true,
+							},
+						},
+					},
+				},
+			},
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "cloud-credentials",
+					Namespace: "test-ns",
+				},
+				Data: map[string][]byte{"cloud": []byte("[default]\naws_access_key_id=AKIAIOSFODNN7EXAMPLE\naws_secret_access_key=wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY")},
+			},
+			want:    true,
+			wantErr: false,
+		},
+		{
+			name: "BSL with s3Url and s3ForcePathStyle but no region expect to fail",
+			dpa: &oadpv1alpha1.DataProtectionApplication{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "foo",
+					Namespace: "test-ns",
+				},
+				Spec: oadpv1alpha1.DataProtectionApplicationSpec{
+					Configuration: &oadpv1alpha1.ApplicationConfig{
+						Velero: &oadpv1alpha1.VeleroConfig{},
+					},
+					BackupImages: ptr.To(false),
+					BackupLocations: []oadpv1alpha1.BackupLocation{
+						{
+							Velero: &velerov1.BackupStorageLocationSpec{
+								Provider: "aws",
+								Config: map[string]string{
+									S3URL:            "https://s3.us-south.cloud-object-storage.appdomain.cloud",
+									S3ForcePathStyle: "true",
+								},
+								StorageType: velerov1.StorageType{
+									ObjectStorage: &velerov1.ObjectStorageLocation{
+										Bucket: "ibm-cos-bucket",
+										Prefix: "prefix",
+									},
+								},
+								Default: true,
+							},
+						},
+					},
+				},
+			},
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "cloud-credentials",
+					Namespace: "test-ns",
+				},
+				Data: map[string][]byte{"cloud": []byte("[default]\naws_access_key_id=AKIAIOSFODNN7EXAMPLE\naws_secret_access_key=wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY")},
+			},
+			want:    false,
+			wantErr: true,
+		},
 		{
 			name: "CloudStorage with different providers - AWS",
 			dpa: &oadpv1alpha1.DataProtectionApplication{
@@ -1958,6 +2083,12 @@ func newContextForTest() context.Context {
 }
 
 func TestDPAReconciler_updateBSLFromSpec(t *testing.T) {
+	originalGetBucketRegionFunc := aws.GetBucketRegionFunc
+	aws.GetBucketRegionFunc = func(bucket string) (string, error) {
+		return "", fmt.Errorf("bucket region not discoverable")
+	}
+	defer func() { aws.GetBucketRegionFunc = originalGetBucketRegionFunc }()
+
 	tests := []struct {
 		name    string
 		bsl     *velerov1.BackupStorageLocation
@@ -2049,7 +2180,7 @@ func TestDPAReconciler_updateBSLFromSpec(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "BSL spec config is nil, no BSL spec update",
+			name: "BSL spec config is nil, config initialized with checksumAlgorithm",
 			bsl: &velerov1.BackupStorageLocation{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "foo-1",
@@ -2107,6 +2238,9 @@ func TestDPAReconciler_updateBSLFromSpec(t *testing.T) {
 				},
 				Spec: velerov1.BackupStorageLocationSpec{
 					Provider: "aws",
+					Config: map[string]string{
+						"checksumAlgorithm": "",
+					},
 					StorageType: velerov1.StorageType{
 						ObjectStorage: &velerov1.ObjectStorageLocation{
 							Bucket: "test-aws-bucket",

pkg/common/common.go

@@ -6,6 +6,7 @@ import (
 	"sort"
 	"strings"
 
+	"github.com/go-logr/logr"
 	velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	"github.com/vmware-tanzu/velero/pkg/types"
 	corev1 "k8s.io/api/core/v1"
@@ -303,14 +304,18 @@ func ApplyUnsupportedServerArgsOverride(container *corev1.Container, unsupported
 // UpdateBackupStorageLocation updates the BackupStorageLocation spec and config.
 //
 //nolint:unparam // Keeping error return type for making the public function flexible for future usage
-func UpdateBackupStorageLocation(bsl *velerov1.BackupStorageLocation, bslSpec velerov1.BackupStorageLocationSpec) error {
+func UpdateBackupStorageLocation(bsl *velerov1.BackupStorageLocation, bslSpec velerov1.BackupStorageLocationSpec, logger logr.Logger) error {
 	if bsl.ObjectMeta.Labels == nil {
 		bsl.ObjectMeta.Labels = make(map[string]string)
 	}
 
 	bsl.ObjectMeta.Labels[RegistryDeploymentLabel] = "True"
 
-	if bslSpec.Config != nil {
+	if bslSpec.Config == nil {
+		bslSpec.Config = make(map[string]string)
+	}
+
+	{
 		// While using Service Principal as Azure credentials, `storageAccountKeyEnvVar` value is not required to be set.
 		// However, the registry deployment fails without a valid storage account key.
 		// This logic prevents the registry pods from being deployed if Azure SP is used as an auth mechanism.
@@ -342,21 +347,22 @@ func UpdateBackupStorageLocation(bsl *velerov1.BackupStorageLocation, bslSpec ve
 			// Auto-detect region for actual AWS S3 buckets (not S3-compatible storage)
 			// This only applies when:
 			// 1. No custom s3Url is configured (meaning it's real AWS S3)
-			// 2. No region is already specified in the config
-			// 3. A bucket name is provided in ObjectStorage
+			// 2. No region is already specified in the DPA spec config
+			// 3. No region was previously auto-detected (stored in existing BSL)
+			// 4. A bucket name is provided in ObjectStorage
+			// If the bucket is recreated in a different region, delete the BSL to trigger re-detection.
 			if _, hasS3Url := bslSpec.Config["s3Url"]; !hasS3Url {
 				if _, hasRegion := bslSpec.Config["region"]; !hasRegion {
-					if bslSpec.ObjectStorage != nil && bslSpec.ObjectStorage.Bucket != "" {
-						// Attempt to auto-detect the bucket's region
-						// AWS Security confirmed that GetBucketRegion works with anonymous credentials
-						// for both public and private buckets (Engagement ID: CACenGS4Mha_KeJ=e3jBSLD6rPZ2iNtfuJUv9QJViaCOt7GVNDg)
-						if detectedRegion, err := aws.GetBucketRegion(bslSpec.ObjectStorage.Bucket); err == nil && detectedRegion != "" {
+					if existingRegion := bsl.Spec.Config["region"]; existingRegion != "" {
+						bslSpec.Config["region"] = existingRegion
+					} else if bslSpec.ObjectStorage != nil && bslSpec.ObjectStorage.Bucket != "" {
+						detectedRegion, err := aws.GetBucketRegion(bslSpec.ObjectStorage.Bucket)
+						if err != nil {
+							logger.Error(err, "Failed to auto-detect AWS bucket region", "bucket", bslSpec.ObjectStorage.Bucket)
+						} else if detectedRegion != "" {
+							logger.Info("Auto-detected AWS bucket region", "bucket", bslSpec.ObjectStorage.Bucket, "region", detectedRegion)
 							bslSpec.Config["region"] = detectedRegion
-							// Note: We successfully auto-detected the region. This is logged at a higher level
-							// to avoid importing logging dependencies here.
 						}
-						// If auto-detection fails, we continue without setting the region.
-						// The user can still manually specify it if needed.
 					}
 				}
 			}