diff --git a/internal/controller/bsl.go b/internal/controller/bsl.go index fa2e95b406c..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 } @@ -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 diff --git a/internal/controller/bsl_test.go b/internal/controller/bsl_test.go index f1b85d94214..15dc685ad2e 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{ @@ -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", diff --git a/pkg/common/common.go b/pkg/common/common.go index 50a4352ce13..32eea51c159 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,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. } } } diff --git a/pkg/common/common_test.go b/pkg/common/common_test.go index 1db843bdf6d..711c8eddf5c 100644 --- a/pkg/common/common_test.go +++ b/pkg/common/common_test.go @@ -3,8 +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" @@ -531,6 +534,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{}, @@ -580,7 +644,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) @@ -588,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) + } + } +} diff --git a/tests/e2e/dpa_deployment_suite_test.go b/tests/e2e/dpa_deployment_suite_test.go index e2503e86ff9..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 not automatically discoverable. 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,