From 3719b241aa474b852f785140bf286ad43bbd1bf8 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Fri, 27 Feb 2026 13:16:54 -0500 Subject: [PATCH 1/7] Require region in BSL validation when s3Url is set When a custom s3Url is configured, the BSL points to a non-AWS S3-compatible service (e.g. IBM Cloud Object Storage, MinIO) where region auto-discovery via AWS HeadBucket API is not valid. Previously, region was only enforced when s3ForcePathStyle was true or AWS discovery failed. This allowed validation to pass with a missing region if a same-named bucket happened to exist on AWS, causing Velero to fail at runtime. Fixes: https://github.com/openshift/oadp-operator/issues/2108 Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude Co-Authored-By: Happy Co-Authored-By: Claude Opus 4.6 --- internal/controller/bsl.go | 8 +- internal/controller/bsl_test.go | 125 ++++++++++++++++++++++++++++++++ 2 files changed, 131 insertions(+), 2 deletions(-) diff --git a/internal/controller/bsl.go b/internal/controller/bsl.go index fa2e95b406c..e496b69966c 100644 --- a/internal/controller/bsl.go +++ b/internal/controller/bsl.go @@ -527,12 +527,16 @@ 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") + (bslSpec.Config != nil && len(bslSpec.Config[S3URL]) > 0 || + bslSpec.Config != nil && bslSpec.Config[S3ForcePathStyle] == "true" || + !aws.BucketRegionIsDiscoverable(bslSpec.ObjectStorage.Bucket)) { + return fmt.Errorf("region for AWS backupstoragelocation is required. Please set the region in the backupstoragelocation config") } //TODO: Add minio, noobaa, local storage validations diff --git a/internal/controller/bsl_test.go b/internal/controller/bsl_test.go index f1b85d94214..027a3b5753d 100644 --- a/internal/controller/bsl_test.go +++ b/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{ From 571befee0cb2907ec9f497cefcd6d039cb93ddce Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 23 Apr 2026 15:24:15 -0400 Subject: [PATCH 2/7] Update error message for missing region in AWS BackupStorageLocation configuration Signed-off-by: Tiger Kaovilai --- tests/e2e/dpa_deployment_suite_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/e2e/dpa_deployment_suite_test.go b/tests/e2e/dpa_deployment_suite_test.go index e2503e86ff9..d411f9ee63a 100644 --- a/tests/e2e/dpa_deployment_suite_test.go +++ b/tests/e2e/dpa_deployment_suite_test.go @@ -377,7 +377,7 @@ var _ = ginkgo.Describe("Configuration testing for DPA Custom Resource", func() NoRegion: true, s3ForcePathStyle: true, }), - }, "region for AWS backupstoragelocation not automatically discoverable. Please set the region in the backupstoragelocation config"), + }, "region for AWS backupstoragelocation is required. Please set the region in the backupstoragelocation config"), ginkgo.Entry("DPA CR with restic config enabled", InstallCase{ DpaSpec: createTestDPASpec(TestDPASpec{ BSLSecretName: bslSecretName, From 18a4526d4618758acfa3691a2c5ad9a98a4f198e Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 23 Apr 2026 16:49:29 -0400 Subject: [PATCH 3/7] Use context-specific error messages for missing BSL region Split the generic "region is required" error into three distinct messages based on the trigger condition: s3Url configured, s3ForcePathStyle enabled, or region auto-discovery failure. This gives users actionable feedback about why region is needed. Update E2E test expectation to match the s3ForcePathStyle path. Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude Co-Authored-By: Happy Signed-off-by: Tiger Kaovilai --- internal/controller/bsl.go | 14 +++++++++----- tests/e2e/dpa_deployment_suite_test.go | 2 +- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/internal/controller/bsl.go b/internal/controller/bsl.go index e496b69966c..2ef62a96761 100644 --- a/internal/controller/bsl.go +++ b/internal/controller/bsl.go @@ -532,11 +532,15 @@ func (r *DataProtectionApplicationReconciler) validateAWSBackupStorageLocation(b // - 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 && len(bslSpec.Config[S3URL]) > 0 || - bslSpec.Config != nil && bslSpec.Config[S3ForcePathStyle] == "true" || - !aws.BucketRegionIsDiscoverable(bslSpec.ObjectStorage.Bucket)) { - return fmt.Errorf("region for AWS backupstoragelocation is required. 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 diff --git a/tests/e2e/dpa_deployment_suite_test.go b/tests/e2e/dpa_deployment_suite_test.go index d411f9ee63a..9fe2cbd16b4 100644 --- a/tests/e2e/dpa_deployment_suite_test.go +++ b/tests/e2e/dpa_deployment_suite_test.go @@ -377,7 +377,7 @@ var _ = ginkgo.Describe("Configuration testing for DPA Custom Resource", func() NoRegion: true, s3ForcePathStyle: true, }), - }, "region for AWS backupstoragelocation is required. Please set the region in the backupstoragelocation config"), + }, "region for AWS backupstoragelocation is required when s3ForcePathStyle is true. Please set the region in the backupstoragelocation config"), ginkgo.Entry("DPA CR with restic config enabled", InstallCase{ DpaSpec: createTestDPASpec(TestDPASpec{ BSLSecretName: bslSecretName, From 9868ffeda4e34347fa16dd85bcd6737ad78f617c Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Tue, 28 Apr 2026 11:15:14 -0400 Subject: [PATCH 4/7] Add logging support to UpdateBackupStorageLocation function Signed-off-by: Tiger Kaovilai --- internal/controller/bsl.go | 2 +- pkg/common/common.go | 13 +++++++------ pkg/common/common_test.go | 3 ++- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/internal/controller/bsl.go b/internal/controller/bsl.go index 2ef62a96761..2cbaff0759f 100644 --- a/internal/controller/bsl.go +++ b/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 } diff --git a/pkg/common/common.go b/pkg/common/common.go index 50a4352ce13..4abed57acf5 100644 --- a/pkg/common/common.go +++ b/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,7 +304,7 @@ 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) } @@ -350,13 +351,13 @@ func UpdateBackupStorageLocation(bsl *velerov1.BackupStorageLocation, bslSpec ve // 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 != "" { + 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. } } } diff --git a/pkg/common/common_test.go b/pkg/common/common_test.go index 1db843bdf6d..527020f55e6 100644 --- a/pkg/common/common_test.go +++ b/pkg/common/common_test.go @@ -5,6 +5,7 @@ import ( "reflect" "testing" + "github.com/go-logr/logr" velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -580,7 +581,7 @@ func TestUpdateBackupStorageLocation(t *testing.T) { bslCopy := tt.bsl.DeepCopy() bslSpecCopy := tt.bslSpec.DeepCopy() - UpdateBackupStorageLocation(bslCopy, *bslSpecCopy) + UpdateBackupStorageLocation(bslCopy, *bslSpecCopy, logr.Discard()) if !reflect.DeepEqual(bslCopy, tt.expectedBsl) { t.Errorf("UpdateBackupStorageLocation() = %v, want %v", bslCopy, tt.expectedBsl) From ea99f9bdd7d14d8c239ba49ee040edf68773eb81 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Tue, 28 Apr 2026 13:27:22 -0400 Subject: [PATCH 5/7] Fix nil config skipping AWS region auto-detection and checksumAlgorithm When DPA backupLocations has no config section, bslSpec.Config is nil, causing UpdateBackupStorageLocation to skip all AWS-specific logic including region auto-detection and checksumAlgorithm defaults. This is the root cause of OADP-5777 still reproducing after PR #1740. Initialize config map when nil so AWS logic runs regardless of whether user specifies a config section. Add test coverage for nil config with both discoverable and undiscoverable buckets. Fixes: https://issues.redhat.com/browse/OADP-5777 Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude Co-Authored-By: Happy Signed-off-by: Tiger Kaovilai --- pkg/common/common.go | 6 +++- pkg/common/common_test.go | 61 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/pkg/common/common.go b/pkg/common/common.go index 4abed57acf5..398b7eaaaad 100644 --- a/pkg/common/common.go +++ b/pkg/common/common.go @@ -311,7 +311,11 @@ func UpdateBackupStorageLocation(bsl *velerov1.BackupStorageLocation, bslSpec ve 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. diff --git a/pkg/common/common_test.go b/pkg/common/common_test.go index 527020f55e6..3dd475bf7b5 100644 --- a/pkg/common/common_test.go +++ b/pkg/common/common_test.go @@ -532,6 +532,67 @@ func TestUpdateBackupStorageLocation(t *testing.T) { }, }, }, + { + name: "AWS region auto-detection - nil config with discoverable bucket", + bsl: &velerov1.BackupStorageLocation{}, + bslSpec: velerov1.BackupStorageLocationSpec{ + Provider: "aws", + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Bucket: "openshift-velero-plugin-s3-auto-region-test-1", + }, + }, + }, + expectedBsl: &velerov1.BackupStorageLocation{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + RegistryDeploymentLabel: "True", + }, + }, + Spec: velerov1.BackupStorageLocationSpec{ + Provider: "aws", + Config: map[string]string{ + "region": "us-east-1", + "checksumAlgorithm": "", + }, + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Bucket: "openshift-velero-plugin-s3-auto-region-test-1", + }, + }, + }, + }, + }, + { + name: "AWS region auto-detection - nil config with undiscoverable bucket", + bsl: &velerov1.BackupStorageLocation{}, + bslSpec: velerov1.BackupStorageLocationSpec{ + Provider: "aws", + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Bucket: "some-nonexistent-bucket", + }, + }, + }, + expectedBsl: &velerov1.BackupStorageLocation{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + RegistryDeploymentLabel: "True", + }, + }, + Spec: velerov1.BackupStorageLocationSpec{ + Provider: "aws", + Config: map[string]string{ + "checksumAlgorithm": "", + }, + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Bucket: "some-nonexistent-bucket", + }, + }, + }, + }, + }, { name: "AWS region auto-detection - real bucket (openshift-velero-plugin-s3-auto-region-test-1)", bsl: &velerov1.BackupStorageLocation{}, From 43ae3b9a333544b3f46ebb6480cdcd9d5addec3b Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Tue, 28 Apr 2026 13:53:47 -0400 Subject: [PATCH 6/7] Fix test expecting nil config after nil-config initialization Update TestDPAReconciler_updateBSLFromSpec to expect checksumAlgorithm in config when bslSpec.Config starts nil, matching the new behavior. Add mock for GetBucketRegionFunc to prevent real AWS calls in test. Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude Co-Authored-By: Happy Signed-off-by: Tiger Kaovilai --- internal/controller/bsl_test.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/internal/controller/bsl_test.go b/internal/controller/bsl_test.go index 027a3b5753d..15dc685ad2e 100644 --- a/internal/controller/bsl_test.go +++ b/internal/controller/bsl_test.go @@ -2083,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 @@ -2174,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", @@ -2232,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", From 8788e9ca93c4595253dcbde9e41472264d09cd79 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Tue, 28 Apr 2026 17:22:30 -0400 Subject: [PATCH 7/7] Fix infinite reconciliation loop in BSL region auto-detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reuse previously auto-detected region from existing BSL instead of calling AWS GetBucketRegion on every reconcile. Without this, each reconcile reads DPA spec (no region) → auto-detects → updates BSL → triggers new reconcile in a loop. Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude Co-Authored-By: Happy Signed-off-by: Tiger Kaovilai --- pkg/common/common.go | 13 ++--- pkg/common/common_test.go | 110 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 6 deletions(-) diff --git a/pkg/common/common.go b/pkg/common/common.go index 398b7eaaaad..32eea51c159 100644 --- a/pkg/common/common.go +++ b/pkg/common/common.go @@ -347,14 +347,15 @@ 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 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) diff --git a/pkg/common/common_test.go b/pkg/common/common_test.go index 3dd475bf7b5..711c8eddf5c 100644 --- a/pkg/common/common_test.go +++ b/pkg/common/common_test.go @@ -3,9 +3,11 @@ package common import ( "errors" "reflect" + "strings" "testing" "github.com/go-logr/logr" + "github.com/go-logr/logr/funcr" velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -650,3 +652,111 @@ func TestUpdateBackupStorageLocation(t *testing.T) { }) } } + +func TestUpdateBackupStorageLocation_ExistingRegionSkipsAutoDetect(t *testing.T) { + originalGetBucketRegionFunc := aws.GetBucketRegionFunc + defer func() { aws.GetBucketRegionFunc = originalGetBucketRegionFunc }() + + callCount := 0 + aws.GetBucketRegionFunc = func(bucket string) (string, error) { + callCount++ + return "us-east-1", nil + } + + bsl := &velerov1.BackupStorageLocation{ + Spec: velerov1.BackupStorageLocationSpec{ + Provider: "aws", + Config: map[string]string{ + "region": "us-east-1", + "checksumAlgorithm": "", + }, + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Bucket: "my-bucket", + }, + }, + }, + } + + bslSpec := velerov1.BackupStorageLocationSpec{ + Provider: "aws", + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Bucket: "my-bucket", + }, + }, + } + + UpdateBackupStorageLocation(bsl, bslSpec, logr.Discard()) + + if callCount != 0 { + t.Errorf("GetBucketRegion called %d times, expected 0 (should reuse existing BSL region)", callCount) + } + if bsl.Spec.Config["region"] != "us-east-1" { + t.Errorf("region = %q, want %q", bsl.Spec.Config["region"], "us-east-1") + } +} + +func TestUpdateBackupStorageLocation_ReconcileLoopPrevention(t *testing.T) { + originalGetBucketRegionFunc := aws.GetBucketRegionFunc + defer func() { aws.GetBucketRegionFunc = originalGetBucketRegionFunc }() + + callCount := 0 + aws.GetBucketRegionFunc = func(bucket string) (string, error) { + callCount++ + return "us-west-2", nil + } + + var logMessages []string + logger := funcr.NewJSON(func(obj string) { + logMessages = append(logMessages, obj) + }, funcr.Options{}) + + bsl := &velerov1.BackupStorageLocation{} + + bslSpec := velerov1.BackupStorageLocationSpec{ + Provider: "aws", + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Bucket: "my-bucket", + }, + }, + } + + // First reconcile: auto-detect should fire + UpdateBackupStorageLocation(bsl, *bslSpec.DeepCopy(), logger) + + if callCount != 1 { + t.Fatalf("first reconcile: GetBucketRegion called %d times, expected 1", callCount) + } + if bsl.Spec.Config["region"] != "us-west-2" { + t.Fatalf("first reconcile: region = %q, want %q", bsl.Spec.Config["region"], "us-west-2") + } + + autoDetectLogs := 0 + for _, msg := range logMessages { + if strings.Contains(msg, "Auto-detected AWS bucket region") { + autoDetectLogs++ + } + } + if autoDetectLogs != 1 { + t.Fatalf("first reconcile: expected 1 auto-detect log, got %d", autoDetectLogs) + } + + // Second reconcile: BSL now has region, DPA spec still has none + logMessages = nil + UpdateBackupStorageLocation(bsl, *bslSpec.DeepCopy(), logger) + + if callCount != 1 { + t.Errorf("second reconcile: GetBucketRegion called %d total times, expected still 1", callCount) + } + if bsl.Spec.Config["region"] != "us-west-2" { + t.Errorf("second reconcile: region = %q, want %q", bsl.Spec.Config["region"], "us-west-2") + } + + for _, msg := range logMessages { + if strings.Contains(msg, "Auto-detected AWS bucket region") { + t.Errorf("second reconcile: unexpected auto-detect log emitted: %s", msg) + } + } +}