OADP-5777: Add automatic S3 bucket region detection and document AWS HeadBucket API behavior#1740
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@kaovilai: This pull request references OADP-5777 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.20.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
approved
|
/test unit-test |
|
/retest |
openshift-merge-robot
added
the
needs-rebase
kaovilai
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
openshift-merge-robot
removed
the
needs-rebase
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
kaovilai
changed the title
|
@kaovilai: This pull request references OADP-5777 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
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>
kaovilai
changed the title
| // 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", "") |
There was a problem hiding this comment.
The reason we are using here is
- simplifies unit test.
- why not. it works for prod too.
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>
|
/test 4.19-e2e-test-aws |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai, shubham-pampattiwar, weshayutin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughThis PR implements AWS region auto-detection for S3 buckets by refactoring AWS SDK v2 dependencies (shifting credentials from indirect to direct), adding auto-detection logic to the backup storage location update flow, replacing anonymous credential handling with inline setup, and updating end-to-end test helpers to account for auto-detected regions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/storage/aws/s3.go (1)
10-10: Clarify static “anon” credentials usage and consider configuring them at config-load timeThe pattern of using
credentials.NewStaticCredentialsProvider("anon-credentials", "anon-secret", "")in theGetBucketRegioncall is a reasonable way to avoid requiring real AWS credentials while still exercising the region lookup. Two small suggestions:
- The comment at Lines 32–35 talks about “anonymous credentials”, but the behavior is now implemented in the per-call options (Lines 41–48) with static (signed) credentials; tightening the wording there would make the intent clearer to future readers.
- To avoid any reliance on the default credential chain and keep behavior fully explicit, you could set the static provider directly in
config.LoadDefaultConfigviaconfig.WithCredentialsProvider(credentials.NewStaticCredentialsProvider(...))instead of only via thes3.Optionscallback.Please also ensure
TestGetBucketRegionis run in an environment without real AWS credentials to confirm region detection still behaves as expected with these static “anon” credentials.Also applies to: 32-35, 41-48
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (6)
go.mod(2 hunks)pkg/common/common.go(1 hunks)pkg/common/common_test.go(1 hunks)pkg/storage/aws/s3.go(2 hunks)pkg/storage/aws/s3_test.go(1 hunks)tests/e2e/lib/dpa_helpers.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/common/common.gopkg/common/common_test.gotests/e2e/lib/dpa_helpers.gopkg/storage/aws/s3.gogo.modpkg/storage/aws/s3_test.go
🔇 Additional comments (6)
go.mod (1)
35-35: AWS SDK v2 dependency reshuffle is consistent with usagePromoting
github.com/aws/aws-sdk-go-v2/credentials v1.17.26to a direct dependency and marking the core module as indirect matches the new import pattern inpkg/storage/aws/s3.goand keeps the module graph tidy. No issues from a correctness or maintainability standpoint.Also applies to: 63-63
pkg/storage/aws/s3_test.go (1)
17-38: Expanded TestGetBucketRegion documentation is helpfulThe additional comments around the public and private bucket cases clearly document the expected AWS HeadBucket behavior and the rationale for using anonymous-style access in tests. Since there are no code changes here, there’s no impact on test behavior.
Also applies to: 45-48
pkg/common/common.go (1)
340-360: AWS region auto-detection logic is well-scoped and safeThe new branch only calls
aws.GetBucketRegionwhen there’s nos3Url, no existingregion, and a non-empty bucket, which correctly targets real AWS S3 while leaving S3-compatible endpoints and explicitly configured regions untouched. On failure it simply skips setting the region, so this is a safe, backwards-compatible enhancement.tests/e2e/lib/dpa_helpers.go (1)
328-340: E2E BSL spec normalization correctly accounts for auto-detected AWS regionThe added normalization (copying the BSL’s
config["region"]into the expected spec when there’s nos3Urland no region set on the DPA side) cleanly aligns the comparison helper with the new auto-detection behavior. It’s scoped to AWS only and safely handles a nil config map before writing, so it shouldn’t introduce regressions for other providers.pkg/common/common_test.go (2)
463-496: LGTM: Region preservation test is correct.The test properly verifies that pre-configured regions are not overridden by the auto-detection logic.
497-530: LGTM: S3-compatible storage skip test is correct.The test properly verifies that region auto-detection is bypassed for S3-compatible storage backends.
| { | ||
| 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", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
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.
|
/retest ai-retester: The e2e tests for the |
|
/retest ai-retester: The |
|
/retest ai-retester: The |
|
from prior fail |
|
/retest ai-retester: The |
|
@kaovilai: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest ai-retester: The e2e tests for "Mongo application DATAMOVER" failed because the |
|
testing that node now reply to correct PR :) should work now. |
…HeadBucket API behavior (openshift#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>
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 openshift#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 <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering> Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
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 <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering> Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
5 participants
Why the changes were made
This PR implements automatic S3 bucket region detection for AWS BackupStorageLocations (BSLs) and documents the expected AWS behavior for the
GetBucketRegionfunction, based on AWS Security's official confirmation.Key Improvements
Background
We discovered that the AWS SDK's
GetBucketRegionfunction works with anonymous credentials on both public and private S3 buckets. AWS Security has confirmed this is expected behavior, not a security vulnerability:GetBucketRegion) does not enforces3:ListBucketpermissions for region retrievalChanges Made
1. S3 Region Detection (
pkg/storage/aws/s3.go)GetBucketRegionto use anonymous credentials viacredentials.NewStaticCredentialsProvider2. Auto-Region Detection (
pkg/common/common.go)UpdateBackupStorageLocationfor AWS BSLss3Urlis configured (indicating real AWS S3, not S3-compatible storage)3. Test Coverage (
pkg/common/common_test.go&pkg/storage/aws/s3_test.go)openshift-velero-plugin-s3-auto-region-test-1Benefits
How to test the changes made
Run the S3 region tests:
go test -v ./pkg/storage/aws/... -run TestGetBucketRegionRun the BSL update tests:
go test -v ./pkg/common/... -run TestUpdateBackupStorageLocationThe tests demonstrate that:
GetBucketRegionsuccessfully retrieves region information for both public and private buckets using anonymous credentialsManual Testing
s3Url) doesn't trigger auto-detectionFixes: https://issues.redhat.com/browse/OADP-5777
Note
Responses generated with Claude