Require region in BSL validation when s3Url is set

[kaovilai]

When a custom s3Url is configured, the BSL points to a non-AWS
S3-compatible service (e.g. IBM Cloud Object Storage, MinIO) where
region auto-discovery via AWS HeadBucket API is not valid. Previously,
region was only enforced when s3ForcePathStyle was true or AWS
discovery failed. This allowed validation to pass with a missing
region if a same-named bucket happened to exist on AWS, causing
Velero to fail at runtime.

Fixes: #2108

Generated with Claude Code
via Happy

Co-Authored-By: Claude noreply@anthropic.com
Co-Authored-By: Happy yesreply@happy.engineering
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com

Why the changes were made

How to test the changes made

Summary by CodeRabbit

• Bug Fixes
  • AWS Backup Storage Location validation now requires an explicit region when S3URL is configured. Error messaging improved to clarify the configuration requirement for users setting up AWS storage locations.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between 87b9ef4 and 3719b24.

📒 Files selected for processing (2)

• internal/controller/bsl.go
• internal/controller/bsl_test.go

---

Walkthrough

The changes add validation to require explicit region configuration when a custom S3URL is set in AWS BackupStorageLocation specs. Three test cases are added to verify the validation behavior for S3URL configurations with and without regions.

Changes

Cohort / File(s)	Summary
Region validation for custom S3 endpoints internal/controller/bsl.go	Added check requiring explicit region when s3Url is configured, since region auto-discovery via AWS API is invalid for non-AWS S3-compatible endpoints. Updated error message to clarify region must be set in backupstoragelocation config.
Test coverage for S3URL validation internal/controller/bsl_test.go	Added three test cases covering S3URL validation scenarios: s3Url without region (failure), s3Url with region (success), and s3Url with s3ForcePathStyle but no region (failure).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 3

❌ Failed checks (2 warnings, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Test Structure And Quality	⚠️ Warning	New test cases lack meaningful failure messages in Expect assertions, reducing diagnostic value when tests fail.	Add context messages to all Expect assertions, e.g., Expect(err).To(HaveOccurred(), "expected reconciliation to fail when s3Url is set without region")
Description check	❓ Inconclusive	The PR description identifies the problem and context well but the required template sections 'Why the changes were made' and 'How to test the changes made' are incomplete with only placeholder comments.	Fill in the template sections with actual explanations of the problem being solved and clear testing instructions or examples.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: requiring region validation when s3Url is set in BSL configuration.
Linked Issues check	✅ Passed	The code changes fully address issue #2108 by requiring region when s3Url is set, preventing unreliable AWS HeadBucket discovery for non-AWS S3 endpoints with comprehensive test coverage.
Out of Scope Changes check	✅ Passed	All changes are scoped to validateAWSBackupStorageLocation validation logic and corresponding tests, directly addressing the linked issue without unrelated modifications.
Stable And Deterministic Test Names	✅ Passed	All three new test cases have stable and deterministic test names without dynamic information such as generated identifiers, timestamps, or UUIDs.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kaovilai

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [kaovilai]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Copilot]

Pull request overview

This PR enhances BSL (BackupStorageLocation) validation to require an explicit region parameter when a custom s3Url is configured for AWS-provider BSLs. This prevents scenarios where the operator incorrectly attempts AWS region auto-discovery for non-AWS S3-compatible services (e.g., IBM Cloud Object Storage, MinIO), which can lead to validation passing with incorrect configuration but Velero failing at runtime.

Changes:

• Added validation logic to require region when s3Url is set in AWS BSL configuration
• Updated error message to be more direct ("is required" vs "not automatically discoverable")
• Added comprehensive test cases for the new validation rule

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
internal/controller/bsl.go	Added check for s3Url in region validation logic and updated error message
internal/controller/bsl_test.go	Added three test cases covering s3Url with/without region and combination with s3ForcePathStyle

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[kaovilai]

/retest

[openshift-ci]

@kaovilai: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/5.0-e2e-test-aws	3719b24	link	false	/test 5.0-e2e-test-aws
ci/prow/4.21-e2e-test-kubevirt-aws	3719b24	link	true	/test 4.21-e2e-test-kubevirt-aws
ci/prow/4.21-e2e-test-aws	3719b24	link	true	/test 4.21-e2e-test-aws
ci/prow/4.21-e2e-test-hcp-aws	3719b24	link	true	/test 4.21-e2e-test-hcp-aws
ci/prow/4.22-e2e-test-aws	3719b24	link	false	/test 4.22-e2e-test-aws

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.