From aa145569e63404f231eb577ec8c7bce473298111 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Mon, 4 Aug 2025 18:47:24 -0400 Subject: [PATCH 1/3] Add tests for auto bucket region on priv/pubic bucket Signed-off-by: Tiger Kaovilai --- go.mod | 4 ++-- pkg/storage/aws/s3.go | 16 ++++++++++++---- pkg/storage/aws/s3_test.go | 26 ++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/go.mod b/go.mod index 696cb0ce7c4..4f6eada5c38 100644 --- a/go.mod +++ b/go.mod @@ -31,8 +31,8 @@ require ( github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.10.1 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/storage/armstorage v1.8.1 github.com/Azure/azure-sdk-for-go/sdk/storage/azblob v1.6.1 - github.com/aws/aws-sdk-go-v2 v1.30.3 github.com/aws/aws-sdk-go-v2/config v1.26.3 + github.com/aws/aws-sdk-go-v2/credentials v1.17.26 github.com/aws/aws-sdk-go-v2/feature/s3/manager v1.15.11 github.com/aws/aws-sdk-go-v2/service/s3 v1.48.0 github.com/deckarep/golang-set/v2 v2.3.0 @@ -60,8 +60,8 @@ require ( github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp v1.27.0 // indirect github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/metric v0.51.0 // indirect github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.51.0 // indirect + github.com/aws/aws-sdk-go-v2 v1.30.3 // indirect github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.5.4 // indirect - github.com/aws/aws-sdk-go-v2/credentials v1.17.26 // indirect github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.16.11 // indirect github.com/aws/aws-sdk-go-v2/internal/configsources v1.3.15 // indirect github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.15 // indirect diff --git a/pkg/storage/aws/s3.go b/pkg/storage/aws/s3.go index 3a7d25b4170..166619b71eb 100644 --- a/pkg/storage/aws/s3.go +++ b/pkg/storage/aws/s3.go @@ -6,8 +6,8 @@ import ( "net/http" "net/url" - "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/config" + "github.com/aws/aws-sdk-go-v2/credentials" "github.com/aws/aws-sdk-go-v2/feature/s3/manager" "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/aws/aws-sdk-go/aws/request" @@ -29,15 +29,23 @@ func GetBucketRegion(bucket string) (string, error) { // Client therefore needs to be configured with region. // In local dev environments, you might have ~/.aws/config that could be loaded and set with default region. // In cluster/CI environment, ~/.aws/config may not be configured, so set hinting region server explicitly. - // Also set to use anonymous credentials. If the bucket is private, this function would not work unless we modify it to take credentials. + // Also set to use anonymous credentials. This works for both public and private buckets as AWS Security + // confirmed that HeadBucket API (used by GetBucketRegion) doesn't enforce s3:ListBucket permissions + // for region retrieval - this is expected AWS behavior. cfg, err := config.LoadDefaultConfig(context.Background(), config.WithRegion("us-east-1"), // This is not default region being used, this is to specify a region hinting server that we will use to get region from. - config.WithCredentialsProvider(aws.AnonymousCredentials{}), ) if err != nil { return "", err } - region, err = manager.GetBucketRegion(context.Background(), s3.NewFromConfig(cfg), bucket) + region, err = manager.GetBucketRegion(context.Background(), s3.NewFromConfig(cfg), bucket, func(o *s3.Options) { + // AWS Security confirmed that anonymous credentials can be used here for GetBucketRegion. + // The HeadBucket API endpoint used internally by GetBucketRegion does not enforce + // s3:ListBucket permissions for retrieving bucket region information. + // Reference: AWS Security response (Engagement ID: CACenGS4Mha_KeJ=e3jBSLD6rPZ2iNtfuJUv9QJViaCOt7GVNDg) + // This is expected AWS behavior, not a security vulnerability. + o.Credentials = credentials.NewStaticCredentialsProvider("anon-credentials", "anon-secret", "") + }) if region != "" { return region, nil } diff --git a/pkg/storage/aws/s3_test.go b/pkg/storage/aws/s3_test.go index 4843808034c..3df9527a0a9 100644 --- a/pkg/storage/aws/s3_test.go +++ b/pkg/storage/aws/s3_test.go @@ -14,12 +14,38 @@ func TestGetBucketRegion(t *testing.T) { wantErr bool }{ { + // Public bucket with s3:ListBucket permission works anonymously + // Note: While the bucket policy includes s3:ListBucket permission (shown below), + // AWS Security confirmed that HeadBucket API doesn't actually enforce this + // permission for region retrieval. The GetBucketRegion operation works with + // anonymous credentials on both public and private buckets. + // { + // "Version": "2012-10-17", + // "Statement": [ + // { + // "Sid": "publicList", + // "Effect": "Allow", + // "Principal": "*", + // "Action": "s3:ListBucket", + // "Resource": "arn:aws:s3:::openshift-velero-plugin-s3-auto-region-test-1" + // } + // ] + // } + // ❯ aws s3api head-bucket --bucket openshift-velero-plugin-s3-auto-region-test-1 --no-sign-request + // { + // "BucketRegion": "us-east-1", + // "AccessPointAlias": false + // } name: "openshift-velero-plugin-s3-auto-region-test-1", bucket: "openshift-velero-plugin-s3-auto-region-test-1", region: "us-east-1", wantErr: false, }, { + // Private bucket - AWS Security confirmed that HeadBucket API (used by GetBucketRegion) + // does NOT require credentials with s3:ListBucket permission for region retrieval. + // This is expected AWS behavior, not a security vulnerability. + // Reference: AWS Security Engagement ID: CACenGS4Mha_KeJ=e3jBSLD6rPZ2iNtfuJUv9QJViaCOt7GVNDg name: "openshift-velero-plugin-s3-auto-region-test-2", bucket: "openshift-velero-plugin-s3-auto-region-test-2", region: "us-west-1", From 36d2c177ef0f17854cd9f1c48ce5753d6aed5f29 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 16 Oct 2025 13:44:33 -0400 Subject: [PATCH 2/3] OADP-5777: Add automatic S3 bucket region detection for AWS BSLs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit adds automatic region detection for AWS S3 buckets in BackupStorageLocation configurations when using actual AWS S3 (not S3-compatible storage). Changes: - Modified UpdateBackupStorageLocation in pkg/common/common.go to auto-detect and set the region when: * Provider is "aws" * No custom s3Url is configured (meaning it's real AWS S3) * No region is already specified in the config * A bucket name is provided in ObjectStorage - The implementation uses aws.GetBucketRegion() which AWS Security confirmed works with anonymous credentials for both public and private buckets (Engagement ID: CACenGS4Mha_KeJ=e3jBSLD6rPZ2iNtfuJUv9QJViaCOt7GVNDg) - Added comprehensive test cases to verify: * Region auto-detection is skipped when region is already specified * Region auto-detection is skipped for S3-compatible storage (with s3Url) * Region auto-detection works with real AWS bucket (tested with openshift-velero-plugin-s3-auto-region-test-1) Benefits: - Prevents configuration errors from incorrect region specifications - Reduces manual configuration requirements for AWS BSLs - Works seamlessly with existing anonymous credential approach 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- pkg/common/common.go | 22 +++++++++ pkg/common/common_test.go | 100 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+) diff --git a/pkg/common/common.go b/pkg/common/common.go index c42734fd3f5..a2f1523b979 100644 --- a/pkg/common/common.go +++ b/pkg/common/common.go @@ -336,6 +336,28 @@ func UpdateBackupStorageLocation(bsl *velerov1.BackupStorageLocation, bslSpec ve if _, ok := bslSpec.Config["checksumAlgorithm"]; !ok { bslSpec.Config["checksumAlgorithm"] = "" } + + // 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 + 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 != "" { + 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 f1f0053fb5f..ed533d501da 100644 --- a/pkg/common/common_test.go +++ b/pkg/common/common_test.go @@ -460,6 +460,106 @@ func TestUpdateBackupStorageLocation(t *testing.T) { }, }, }, + { + name: "AWS region auto-detection - region already specified", + bsl: &velerov1.BackupStorageLocation{}, + bslSpec: velerov1.BackupStorageLocationSpec{ + Provider: "aws", + Config: map[string]string{ + "region": "us-west-2", + }, + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Bucket: "test-bucket", + }, + }, + }, + expectedBsl: &velerov1.BackupStorageLocation{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + RegistryDeploymentLabel: "True", + }, + }, + Spec: velerov1.BackupStorageLocationSpec{ + Provider: "aws", + Config: map[string]string{ + "region": "us-west-2", + "checksumAlgorithm": "", + }, + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Bucket: "test-bucket", + }, + }, + }, + }, + }, + { + name: "AWS region auto-detection - skipped for S3-compatible storage", + bsl: &velerov1.BackupStorageLocation{}, + bslSpec: velerov1.BackupStorageLocationSpec{ + Provider: "aws", + Config: map[string]string{ + "s3Url": "https://minio.example.com", + }, + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Bucket: "test-bucket", + }, + }, + }, + expectedBsl: &velerov1.BackupStorageLocation{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + RegistryDeploymentLabel: "True", + }, + }, + Spec: velerov1.BackupStorageLocationSpec{ + Provider: "aws", + Config: map[string]string{ + "s3Url": "https://minio.example.com", + "checksumAlgorithm": "", + }, + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Bucket: "test-bucket", + }, + }, + }, + }, + }, + { + name: "AWS region auto-detection - real bucket (openshift-velero-plugin-s3-auto-region-test-1)", + bsl: &velerov1.BackupStorageLocation{}, + bslSpec: velerov1.BackupStorageLocationSpec{ + Provider: "aws", + Config: map[string]string{}, + 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", + }, + }, + }, + }, + }, } for _, tt := range tests { From b0cdbf6149578c40b65678388fa3ce6c46283e1f Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 16 Oct 2025 18:00:06 -0400 Subject: [PATCH 3/3] OADP-5777: Fix E2E test to accept auto-detected region in BSL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update DoesBSLSpecMatchesDpa function to accept that DPA spec can have an empty region while the deployed BSL has an auto-detected region. The test now properly handles the scenario where: - DPA spec doesn't specify a region - No custom s3Url is configured (real AWS S3) - The deployed BSL has an auto-detected region This ensures the E2E test "DPA CR without Region, without S3ForcePathStyle and with BackupImages false" passes with the new auto-detection feature. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- tests/e2e/lib/dpa_helpers.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/e2e/lib/dpa_helpers.go b/tests/e2e/lib/dpa_helpers.go index 1ddee4f7916..1ce4b45f023 100644 --- a/tests/e2e/lib/dpa_helpers.go +++ b/tests/e2e/lib/dpa_helpers.go @@ -324,6 +324,20 @@ func (v *DpaCustomResource) DoesBSLSpecMatchesDpa(dpaBSLSpec velero.BackupStorag configWithChecksumAlgorithm["checksumAlgorithm"] = "" dpaBSLSpec.Config = configWithChecksumAlgorithm } + + // Handle auto-detected region for AWS (real AWS S3, not S3-compatible storage) + _, hasS3Url := dpaBSLSpec.Config["s3Url"] + _, dpaHasRegion := dpaBSLSpec.Config["region"] + bslRegion, bslHasRegion := bslReal.Spec.Config["region"] + + // If this is real AWS (no s3Url), DPA has no region, but BSL has one (auto-detected) + if !hasS3Url && !dpaHasRegion && bslHasRegion { + // Accept the auto-detected region by adding it to the expected spec + if dpaBSLSpec.Config == nil { + dpaBSLSpec.Config = make(map[string]string) + } + dpaBSLSpec.Config["region"] = bslRegion + } } if !reflect.DeepEqual(dpaBSLSpec, bslReal.Spec) { log.Println(cmp.Diff(dpaBSLSpec, bslReal.Spec))