Skip to content

Commit 2abf698

Browse files
kaovilaiclaude
kaovilai
and
claude
authored
OADP-5777: Add automatic S3 bucket region detection and document AWS HeadBucket API behavior (#1740)
* Add tests for auto bucket region on priv/pubic bucket Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com> * OADP-5777: Add automatic S3 bucket region detection for AWS BSLs 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 <noreply@anthropic.com> * OADP-5777: Fix E2E test to accept auto-detected region in BSL 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 <noreply@anthropic.com> --------- Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com> Co-authored-by: Claude <noreply@anthropic.com>
1 parent 24e3758 commit 2abf698

File tree

6 files changed

+176
-6
lines changed

6 files changed

+176
-6
lines changed

‎go.mod‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,8 @@ require (
3131
github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.10.1
3232
github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/storage/armstorage v1.8.1
3333
github.com/Azure/azure-sdk-for-go/sdk/storage/azblob v1.6.1
34-
github.com/aws/aws-sdk-go-v2 v1.30.3
3534
github.com/aws/aws-sdk-go-v2/config v1.26.3
35+
github.com/aws/aws-sdk-go-v2/credentials v1.17.26
3636
github.com/aws/aws-sdk-go-v2/feature/s3/manager v1.15.11
3737
github.com/aws/aws-sdk-go-v2/service/s3 v1.48.0
3838
github.com/deckarep/golang-set/v2 v2.3.0
@@ -60,8 +60,8 @@ require (
6060
github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp v1.27.0 // indirect
6161
github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/metric v0.51.0 // indirect
6262
github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.51.0 // indirect
63+
github.com/aws/aws-sdk-go-v2 v1.30.3 // indirect
6364
github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.5.4 // indirect
64-
github.com/aws/aws-sdk-go-v2/credentials v1.17.26 // indirect
6565
github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.16.11 // indirect
6666
github.com/aws/aws-sdk-go-v2/internal/configsources v1.3.15 // indirect
6767
github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.15 // indirect

‎pkg/common/common.go‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -336,6 +336,28 @@ func UpdateBackupStorageLocation(bsl *velerov1.BackupStorageLocation, bslSpec ve
336336
if _, ok := bslSpec.Config["checksumAlgorithm"]; !ok {
337337
bslSpec.Config["checksumAlgorithm"] = ""
338338
}
339+
340+
// Auto-detect region for actual AWS S3 buckets (not S3-compatible storage)
341+
// This only applies when:
342+
// 1. No custom s3Url is configured (meaning it's real AWS S3)
343+
// 2. No region is already specified in the config
344+
// 3. A bucket name is provided in ObjectStorage
345+
if _, hasS3Url := bslSpec.Config["s3Url"]; !hasS3Url {
346+
if _, hasRegion := bslSpec.Config["region"]; !hasRegion {
347+
if bslSpec.ObjectStorage != nil && bslSpec.ObjectStorage.Bucket != "" {
348+
// Attempt to auto-detect the bucket's region
349+
// AWS Security confirmed that GetBucketRegion works with anonymous credentials
350+
// for both public and private buckets (Engagement ID: CACenGS4Mha_KeJ=e3jBSLD6rPZ2iNtfuJUv9QJViaCOt7GVNDg)
351+
if detectedRegion, err := aws.GetBucketRegion(bslSpec.ObjectStorage.Bucket); err == nil && detectedRegion != "" {
352+
bslSpec.Config["region"] = detectedRegion
353+
// Note: We successfully auto-detected the region. This is logged at a higher level
354+
// to avoid importing logging dependencies here.
355+
}
356+
// If auto-detection fails, we continue without setting the region.
357+
// The user can still manually specify it if needed.
358+
}
359+
}
360+
}
339361
}
340362
}
341363

‎pkg/common/common_test.go‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -460,6 +460,106 @@ func TestUpdateBackupStorageLocation(t *testing.T) {
460460
},
461461
},
462462
},
463+
{
464+
name: "AWS region auto-detection - region already specified",
465+
bsl: &velerov1.BackupStorageLocation{},
466+
bslSpec: velerov1.BackupStorageLocationSpec{
467+
Provider: "aws",
468+
Config: map[string]string{
469+
"region": "us-west-2",
470+
},
471+
StorageType: velerov1.StorageType{
472+
ObjectStorage: &velerov1.ObjectStorageLocation{
473+
Bucket: "test-bucket",
474+
},
475+
},
476+
},
477+
expectedBsl: &velerov1.BackupStorageLocation{
478+
ObjectMeta: metav1.ObjectMeta{
479+
Labels: map[string]string{
480+
RegistryDeploymentLabel: "True",
481+
},
482+
},
483+
Spec: velerov1.BackupStorageLocationSpec{
484+
Provider: "aws",
485+
Config: map[string]string{
486+
"region": "us-west-2",
487+
"checksumAlgorithm": "",
488+
},
489+
StorageType: velerov1.StorageType{
490+
ObjectStorage: &velerov1.ObjectStorageLocation{
491+
Bucket: "test-bucket",
492+
},
493+
},
494+
},
495+
},
496+
},
497+
{
498+
name: "AWS region auto-detection - skipped for S3-compatible storage",
499+
bsl: &velerov1.BackupStorageLocation{},
500+
bslSpec: velerov1.BackupStorageLocationSpec{
501+
Provider: "aws",
502+
Config: map[string]string{
503+
"s3Url": "https://minio.example.com",
504+
},
505+
StorageType: velerov1.StorageType{
506+
ObjectStorage: &velerov1.ObjectStorageLocation{
507+
Bucket: "test-bucket",
508+
},
509+
},
510+
},
511+
expectedBsl: &velerov1.BackupStorageLocation{
512+
ObjectMeta: metav1.ObjectMeta{
513+
Labels: map[string]string{
514+
RegistryDeploymentLabel: "True",
515+
},
516+
},
517+
Spec: velerov1.BackupStorageLocationSpec{
518+
Provider: "aws",
519+
Config: map[string]string{
520+
"s3Url": "https://minio.example.com",
521+
"checksumAlgorithm": "",
522+
},
523+
StorageType: velerov1.StorageType{
524+
ObjectStorage: &velerov1.ObjectStorageLocation{
525+
Bucket: "test-bucket",
526+
},
527+
},
528+
},
529+
},
530+
},
531+
{
532+
name: "AWS region auto-detection - real bucket (openshift-velero-plugin-s3-auto-region-test-1)",
533+
bsl: &velerov1.BackupStorageLocation{},
534+
bslSpec: velerov1.BackupStorageLocationSpec{
535+
Provider: "aws",
536+
Config: map[string]string{},
537+
StorageType: velerov1.StorageType{
538+
ObjectStorage: &velerov1.ObjectStorageLocation{
539+
Bucket: "openshift-velero-plugin-s3-auto-region-test-1",
540+
},
541+
},
542+
},
543+
expectedBsl: &velerov1.BackupStorageLocation{
544+
ObjectMeta: metav1.ObjectMeta{
545+
Labels: map[string]string{
546+
RegistryDeploymentLabel: "True",
547+
},
548+
},
549+
Spec: velerov1.BackupStorageLocationSpec{
550+
Provider: "aws",
551+
Config: map[string]string{
552+
"region": "us-east-1",
553+
"checksumAlgorithm": "",
554+
},
555+
StorageType: velerov1.StorageType{
556+
ObjectStorage: &velerov1.ObjectStorageLocation{
557+
Bucket: "openshift-velero-plugin-s3-auto-region-test-1",
558+
},
559+
},
560+
},
561+
},
562+
},
463563
}
464564

465565
for _, tt := range tests {

‎pkg/storage/aws/s3.go‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,8 @@ import (
66
"net/http"
77
"net/url"
88

9-
"github.com/aws/aws-sdk-go-v2/aws"
109
"github.com/aws/aws-sdk-go-v2/config"
10+
"github.com/aws/aws-sdk-go-v2/credentials"
1111
"github.com/aws/aws-sdk-go-v2/feature/s3/manager"
1212
"github.com/aws/aws-sdk-go-v2/service/s3"
1313
"github.com/aws/aws-sdk-go/aws/request"
@@ -29,15 +29,23 @@ func GetBucketRegion(bucket string) (string, error) {
2929
// Client therefore needs to be configured with region.
3030
// In local dev environments, you might have ~/.aws/config that could be loaded and set with default region.
3131
// In cluster/CI environment, ~/.aws/config may not be configured, so set hinting region server explicitly.
32-
// Also set to use anonymous credentials. If the bucket is private, this function would not work unless we modify it to take credentials.
32+
// Also set to use anonymous credentials. This works for both public and private buckets as AWS Security
33+
// confirmed that HeadBucket API (used by GetBucketRegion) doesn't enforce s3:ListBucket permissions
34+
// for region retrieval - this is expected AWS behavior.
3335
cfg, err := config.LoadDefaultConfig(context.Background(),
3436
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.
35-
config.WithCredentialsProvider(aws.AnonymousCredentials{}),
3637
)
3738
if err != nil {
3839
return "", err
3940
}
40-
region, err = manager.GetBucketRegion(context.Background(), s3.NewFromConfig(cfg), bucket)
41+
region, err = manager.GetBucketRegion(context.Background(), s3.NewFromConfig(cfg), bucket, func(o *s3.Options) {
42+
// AWS Security confirmed that anonymous credentials can be used here for GetBucketRegion.
43+
// The HeadBucket API endpoint used internally by GetBucketRegion does not enforce
44+
// s3:ListBucket permissions for retrieving bucket region information.
45+
// Reference: AWS Security response (Engagement ID: CACenGS4Mha_KeJ=e3jBSLD6rPZ2iNtfuJUv9QJViaCOt7GVNDg)
46+
// This is expected AWS behavior, not a security vulnerability.
47+
o.Credentials = credentials.NewStaticCredentialsProvider("anon-credentials", "anon-secret", "")
48+
})
4149
if region != "" {
4250
return region, nil
4351
}

‎pkg/storage/aws/s3_test.go‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,38 @@ func TestGetBucketRegion(t *testing.T) {
1414
wantErr bool
1515
}{
1616
{
17+
// Public bucket with s3:ListBucket permission works anonymously
18+
// Note: While the bucket policy includes s3:ListBucket permission (shown below),
19+
// AWS Security confirmed that HeadBucket API doesn't actually enforce this
20+
// permission for region retrieval. The GetBucketRegion operation works with
21+
// anonymous credentials on both public and private buckets.
22+
// {
23+
// "Version": "2012-10-17",
24+
// "Statement": [
25+
// {
26+
// "Sid": "publicList",
27+
// "Effect": "Allow",
28+
// "Principal": "*",
29+
// "Action": "s3:ListBucket",
30+
// "Resource": "arn:aws:s3:::openshift-velero-plugin-s3-auto-region-test-1"
31+
// }
32+
// ]
33+
// }
34+
// ❯ aws s3api head-bucket --bucket openshift-velero-plugin-s3-auto-region-test-1 --no-sign-request
35+
// {
36+
// "BucketRegion": "us-east-1",
37+
// "AccessPointAlias": false
38+
// }
1739
name: "openshift-velero-plugin-s3-auto-region-test-1",
1840
bucket: "openshift-velero-plugin-s3-auto-region-test-1",
1941
region: "us-east-1",
2042
wantErr: false,
2143
},
2244
{
45+
// Private bucket - AWS Security confirmed that HeadBucket API (used by GetBucketRegion)
46+
// does NOT require credentials with s3:ListBucket permission for region retrieval.
47+
// This is expected AWS behavior, not a security vulnerability.
48+
// Reference: AWS Security Engagement ID: CACenGS4Mha_KeJ=e3jBSLD6rPZ2iNtfuJUv9QJViaCOt7GVNDg
2349
name: "openshift-velero-plugin-s3-auto-region-test-2",
2450
bucket: "openshift-velero-plugin-s3-auto-region-test-2",
2551
region: "us-west-1",

‎tests/e2e/lib/dpa_helpers.go‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -324,6 +324,20 @@ func (v *DpaCustomResource) DoesBSLSpecMatchesDpa(dpaBSLSpec velero.BackupStorag
324324
configWithChecksumAlgorithm["checksumAlgorithm"] = ""
325325
dpaBSLSpec.Config = configWithChecksumAlgorithm
326326
}
327+
328+
// Handle auto-detected region for AWS (real AWS S3, not S3-compatible storage)
329+
_, hasS3Url := dpaBSLSpec.Config["s3Url"]
330+
_, dpaHasRegion := dpaBSLSpec.Config["region"]
331+
bslRegion, bslHasRegion := bslReal.Spec.Config["region"]
332+
333+
// If this is real AWS (no s3Url), DPA has no region, but BSL has one (auto-detected)
334+
if !hasS3Url && !dpaHasRegion && bslHasRegion {
335+
// Accept the auto-detected region by adding it to the expected spec
336+
if dpaBSLSpec.Config == nil {
337+
dpaBSLSpec.Config = make(map[string]string)
338+
}
339+
dpaBSLSpec.Config["region"] = bslRegion
340+
}
327341
}
328342
if !reflect.DeepEqual(dpaBSLSpec, bslReal.Spec) {
329343
log.Println(cmp.Diff(dpaBSLSpec, bslReal.Spec))

0 commit comments

Comments
 (0)