From d8ed145e18b3105d39083ce2b538b5528c6eedab Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Tue, 6 Jan 2026 17:43:12 -0800 Subject: [PATCH] Fix S3 bucket region unit tests by adding mocking support (#2052) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The unit tests for S3 bucket region auto-discovery were making real network calls to AWS, causing failures in CI environments without network access to the test buckets. Changes: - Add GetBucketRegionFunc variable in pkg/storage/aws/s3.go to enable mocking in tests - Refactor s3_test.go to use mocks instead of real AWS calls - Add mock setup in bsl_test.go for tests using DiscoverableBucket - Add mock setup in common_test.go for TestUpdateBackupStorageLocation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.5 --- internal/controller/bsl_test.go | 11 ++++ pkg/common/common_test.go | 13 +++++ pkg/storage/aws/s3.go | 15 ++++- pkg/storage/aws/s3_test.go | 100 +++++++++++++++++++++----------- 4 files changed, 102 insertions(+), 37 deletions(-) diff --git a/internal/controller/bsl_test.go b/internal/controller/bsl_test.go index aa70d4954b2..3f020ca62a5 100644 --- a/internal/controller/bsl_test.go +++ b/internal/controller/bsl_test.go @@ -25,6 +25,7 @@ import ( oadpv1alpha1 "github.com/openshift/oadp-operator/api/v1alpha1" "github.com/openshift/oadp-operator/pkg/common" "github.com/openshift/oadp-operator/pkg/credentials/stsflow" + "github.com/openshift/oadp-operator/pkg/storage/aws" ) // A bucket that region can be automatically discovered @@ -1911,6 +1912,16 @@ func TestDPAReconciler_ValidateBackupStorageLocations(t *testing.T) { wantErr: true, }, } + // Mock GetBucketRegionFunc to return a region for DiscoverableBucket + originalGetBucketRegionFunc := aws.GetBucketRegionFunc + aws.GetBucketRegionFunc = func(bucket string) (string, error) { + if bucket == DiscoverableBucket { + return "us-east-1", nil + } + return "", fmt.Errorf("bucket region not discoverable") + } + defer func() { aws.GetBucketRegionFunc = originalGetBucketRegionFunc }() + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { tt.objects = append(tt.objects, tt.dpa, tt.secret) diff --git a/pkg/common/common_test.go b/pkg/common/common_test.go index f1f0053fb5f..5c3e3cc9383 100644 --- a/pkg/common/common_test.go +++ b/pkg/common/common_test.go @@ -1,6 +1,7 @@ package common import ( + "errors" "reflect" "testing" @@ -8,6 +9,8 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/ptr" + + "github.com/openshift/oadp-operator/pkg/storage/aws" ) func TestAppendUniqueKeyTOfTMaps(t *testing.T) { @@ -462,6 +465,16 @@ func TestUpdateBackupStorageLocation(t *testing.T) { }, } + // Mock GetBucketRegionFunc to return a region for the test bucket + originalGetBucketRegionFunc := aws.GetBucketRegionFunc + aws.GetBucketRegionFunc = func(bucket string) (string, error) { + if bucket == "openshift-velero-plugin-s3-auto-region-test-1" { + return "us-east-1", nil + } + return "", errors.New("bucket region not discoverable") + } + defer func() { aws.GetBucketRegionFunc = originalGetBucketRegionFunc }() + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { bslCopy := tt.bsl.DeepCopy() diff --git a/pkg/storage/aws/s3.go b/pkg/storage/aws/s3.go index 3a7d25b4170..267108387f8 100644 --- a/pkg/storage/aws/s3.go +++ b/pkg/storage/aws/s3.go @@ -13,16 +13,25 @@ import ( "github.com/aws/aws-sdk-go/aws/request" ) +// GetBucketRegionFunc is the function used to get bucket region. +// It can be replaced in tests for mocking. +var GetBucketRegionFunc = getBucketRegionImpl + func BucketRegionIsDiscoverable(bucket string) bool { - _, err := GetBucketRegion(bucket) + _, err := GetBucketRegionFunc(bucket) return err == nil } // GetBucketRegion returns the AWS region that a bucket is in, or an error -// if the region cannot be determined. +// if the region cannot be determined. This is a wrapper that calls GetBucketRegionFunc. +func GetBucketRegion(bucket string) (string, error) { + return GetBucketRegionFunc(bucket) +} + +// getBucketRegionImpl is the actual implementation that calls AWS. // copied from https://github.com/openshift/openshift-velero-plugin/pull/223/files#diff-da482ef606b3938b09ae46990a60eb0ad49ebfb4885eb1af327d90f215bf58b1 // modified to aws-sdk-go-v2 -func GetBucketRegion(bucket string) (string, error) { +func getBucketRegionImpl(bucket string) (string, error) { var region string // GetBucketRegion will attempt to get the region for a bucket using the client's configured region to determine // which AWS partition to perform the query on. diff --git a/pkg/storage/aws/s3_test.go b/pkg/storage/aws/s3_test.go index 4843808034c..7db15b98c7c 100644 --- a/pkg/storage/aws/s3_test.go +++ b/pkg/storage/aws/s3_test.go @@ -1,58 +1,67 @@ package aws import ( + "errors" "reflect" "testing" ) -// from https://github.com/openshift/openshift-velero-plugin/pull/223/files#diff-4f17f1708744bd4d8cb7a4232212efa0e3bfde2b9c7b12e3a23dcc913b9fc2ec func TestGetBucketRegion(t *testing.T) { tests := []struct { - name string - bucket string - region string - wantErr bool + name string + bucket string + mockRegion string + mockErr error + wantRegion string + wantErr bool }{ { - name: "openshift-velero-plugin-s3-auto-region-test-1", - bucket: "openshift-velero-plugin-s3-auto-region-test-1", - region: "us-east-1", - wantErr: false, + name: "successful region discovery", + bucket: "test-bucket", + mockRegion: "us-east-1", + mockErr: nil, + wantRegion: "us-east-1", + wantErr: false, }, { - name: "openshift-velero-plugin-s3-auto-region-test-2", - bucket: "openshift-velero-plugin-s3-auto-region-test-2", - region: "us-west-1", - wantErr: false, + name: "region discovery for eu-central-1", + bucket: "eu-bucket", + mockRegion: "eu-central-1", + mockErr: nil, + wantRegion: "eu-central-1", + wantErr: false, }, { - name: "openshift-velero-plugin-s3-auto-region-test-3", - bucket: "openshift-velero-plugin-s3-auto-region-test-3", - region: "eu-central-1", - wantErr: false, - }, - { - name: "openshift-velero-plugin-s3-auto-region-test-4", - bucket: "openshift-velero-plugin-s3-auto-region-test-4", - region: "sa-east-1", - wantErr: false, - }, - { - name: "velero-6109f5e9711c8c58131acdd2f490f451", // oadp prow aws bucket name - bucket: "velero-6109f5e9711c8c58131acdd2f490f451", - region: "us-east-1", - wantErr: false, + name: "region discovery fails", + bucket: "non-existent-bucket", + mockRegion: "", + mockErr: errors.New("bucket not found"), + wantRegion: "", + wantErr: true, }, } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + // Save original and restore after test + originalFunc := GetBucketRegionFunc + defer func() { GetBucketRegionFunc = originalFunc }() + + // Set up mock + GetBucketRegionFunc = func(bucket string) (string, error) { + if bucket != tt.bucket { + t.Errorf("GetBucketRegion called with wrong bucket: got %v, want %v", bucket, tt.bucket) + } + return tt.mockRegion, tt.mockErr + } + got, err := GetBucketRegion(tt.bucket) if (err != nil) != tt.wantErr { t.Errorf("GetBucketRegion() error = %v, wantErr %v", err, tt.wantErr) return } - if got != tt.region { - t.Errorf("GetBucketRegion() = %v, want %v", got, tt.region) + if got != tt.wantRegion { + t.Errorf("GetBucketRegion() = %v, want %v", got, tt.wantRegion) } }) } @@ -62,21 +71,44 @@ func TestBucketRegionIsDiscoverable(t *testing.T) { tests := []struct { name string bucket string + mockRegion string + mockErr error discoverable bool }{ { - name: "openshift-velero-plugin-s3-auto-region-test-1 region is discoverable", - bucket: "openshift-velero-plugin-s3-auto-region-test-1", + name: "bucket region is discoverable", + bucket: "discoverable-bucket", + mockRegion: "us-east-1", + mockErr: nil, discoverable: true, }, { - name: "bucketNamesOverSixtyThreeCharactersAndNowItIsAboutTimeToTestThisFunction is an invalid aws bucket name so region is not discoverable", + name: "bucket region is not discoverable due to error", + bucket: "non-existent-bucket", + mockRegion: "", + mockErr: errors.New("bucket not found"), + discoverable: false, + }, + { + name: "invalid bucket name - region not discoverable", bucket: "bucketNamesOverSixtyThreeCharactersAndNowItIsAboutTimeToTestThisFunction", + mockRegion: "", + mockErr: errors.New("invalid bucket name"), discoverable: false, }, } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + // Save original and restore after test + originalFunc := GetBucketRegionFunc + defer func() { GetBucketRegionFunc = originalFunc }() + + // Set up mock + GetBucketRegionFunc = func(bucket string) (string, error) { + return tt.mockRegion, tt.mockErr + } + if got := BucketRegionIsDiscoverable(tt.bucket); got != tt.discoverable { t.Errorf("BucketRegionIsDiscoverable() = %v, want %v", got, tt.discoverable) }