oadp-1.5: Fix S3 bucket region unit tests by adding mocking support (#2052)

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)

pkg/common/common_test.go

@@ -1,13 +1,16 @@
 package common
 
 import (
+	"errors"
 	"reflect"
 	"testing"
 
 	velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	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()

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.

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)
 			}