Skip to content

OADP-5777: Add automatic S3 bucket region detection and document AWS HeadBucket API behavior #1740

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 3 commits into from
Nov 19, 2025
Merged

OADP-5777: Add automatic S3 bucket region detection and document AWS HeadBucket API behavior #1740

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aa14556
Add tests for auto bucket region on priv/pubic bucket
kaovilai Aug 4, 2025
36d2c17
OADP-5777: Add automatic S3 bucket region detection for AWS BSLs
kaovilai Oct 16, 2025
b0cdbf6
OADP-5777: Fix E2E test to accept auto-detected region in BSL
kaovilai Oct 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
22 changes: 22 additions & 0 deletions pkg/common/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
}
}
}
}
}

Expand Down
100 changes: 100 additions & 0 deletions pkg/common/common_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
},
},
},
},
},
Comment on lines +531 to +562
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Consider mocking AWS API calls or relocating to integration tests.

This test makes actual AWS API calls to detect the region of a real bucket, introducing several concerns:

  • Network dependency: Requires AWS connectivity, making tests slower and potentially flaky.
  • External resource dependency: Bucket must exist and remain accessible; deletion or renaming breaks the test.
  • CI/CD reliability: May fail in restricted network environments or during AWS outages.
  • Unit test principles: Unit tests should be fast, isolated, and deterministic.

Consider either mocking the GetBucketRegion call for unit tests or moving this test to an integration test suite (e.g., tests/e2e/) where external dependencies are acceptable.

🤖 Prompt for AI Agents 
In pkg/common/common_test.go around lines 531 to 562 the unit test performs a
real AWS API call to detect the S3 bucket region which introduces network and
external-resource flakiness; either (A) convert the test to use a mock for the
AWS region lookup by injecting a small interface around the GetBucketRegion call
and in the test replace the real client with a fake that returns the desired
region (and adjust the expectedBsl.Region value accordingly), or (B) move this
test out of the unit tests into an integration/e2e suite (e.g., tests/e2e/) and
mark it to run only in environments with AWS access; pick one approach and
remove any hard dependency on the real bucket name from the unit test.

}

for _, tt := range tests {
Expand Down
16 changes: 12 additions & 4 deletions pkg/storage/aws/s3.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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", "")
Copy link
Copy Markdown
Member Author

@kaovilai kaovilai Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason we are using here is

  1. simplifies unit test.
  2. why not. it works for prod too.

})
if region != "" {
return region, nil
}
Expand Down
26 changes: 26 additions & 0 deletions pkg/storage/aws/s3_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
14 changes: 14 additions & 0 deletions tests/e2e/lib/dpa_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down