Skip to content

Commit

Permalink
OADP-3038 Update region discovery error message.
Browse files Browse the repository at this point in the history
Signed-off-by: Tiger Kaovilai <[email protected]>
  • Loading branch information
kaovilai committed Feb 15, 2024
1 parent 20ee343 commit 8830d06
Show file tree
Hide file tree
Showing 4 changed files with 143 additions and 5 deletions.
11 changes: 8 additions & 3 deletions controllers/bsl.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/go-logr/logr"
oadpv1alpha1 "github.com/openshift/oadp-operator/api/v1alpha1"
"github.com/openshift/oadp-operator/pkg/common"
"github.com/openshift/oadp-operator/pkg/storage/aws"
velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -286,9 +287,13 @@ func (r *DPAReconciler) validateAWSBackupStorageLocation(bslSpec velerov1.Backup
}

// BSL region is required when s3ForcePathStyle is true OR BackupImages is true
// TODO: Update logic when (region auto-discovery) https://github.com/openshift/openshift-velero-plugin/pull/223 is merged
if (bslSpec.Config == nil || len(bslSpec.Config[Region]) == 0) && (bslSpec.Config != nil && bslSpec.Config[S3ForcePathStyle] == "true" || dpa.BackupImages()) {
return fmt.Errorf("region for AWS backupstoragelocation cannot be empty when s3ForcePathStyle is true or when backing up images")
// s3ForcePathStyle is true requires region because some velero processes requires region to be set and is not auto-discoverable when s3ForcePathStyle is true
// imagestream backup in openshift-velero-plugin now uses the same method to discover region as the rest of the velero codebase
// after https://github.com/openshift/openshift-velero-plugin/pull/223.
// Sometimes, the region is not discoverable and the user has to set it manually.
if (bslSpec.Config == nil || len(bslSpec.Config[Region]) == 0) &&
(bslSpec.Config != nil && bslSpec.Config[S3ForcePathStyle] == "true" || !aws.BucketRegionIsDiscoverable(bslSpec.ObjectStorage.Bucket)) {
return fmt.Errorf("region for AWS backupstoragelocation not automatically discoverable. Please set the region in the backupstoragelocation config")
}

//TODO: Add minio, noobaa, local storage validations
Expand Down
5 changes: 3 additions & 2 deletions controllers/bsl_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/go-logr/logr"
"github.com/google/go-cmp/cmp"
oadpv1alpha1 "github.com/openshift/oadp-operator/api/v1alpha1"
"github.com/openshift/oadp-operator/pkg/storage/aws"
velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
Expand Down Expand Up @@ -1183,7 +1184,7 @@ func TestDPAReconciler_ValidateBackupStorageLocations(t *testing.T) {
Provider: "aws",
StorageType: velerov1.StorageType{
ObjectStorage: &velerov1.ObjectStorageLocation{
Bucket: "bucket",
Bucket: aws.DiscoverableBucket,
Prefix: "prefix",
},
},
Expand Down Expand Up @@ -1339,7 +1340,7 @@ func TestDPAReconciler_ValidateBackupStorageLocations(t *testing.T) {
},
StorageType: velerov1.StorageType{
ObjectStorage: &velerov1.ObjectStorageLocation{
Bucket: "bucket",
Bucket: aws.DiscoverableBucket,
Prefix: "prefix",
},
},
Expand Down
48 changes: 48 additions & 0 deletions pkg/storage/aws/s3.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package aws

import (
"context"

"errors"

"github.com/aws/aws-sdk-go/aws/endpoints"
"github.com/aws/aws-sdk-go/aws/session"
"github.com/aws/aws-sdk-go/service/s3/s3manager"
// "github.com/pkg/errors" don't need stack trace, not inside velero-plugin
)

func BucketRegionIsDiscoverable(bucket string) bool {
_, err := GetBucketRegion(bucket)
return err == nil
}

// GetBucketRegion returns the AWS region that a bucket is in, or an error
// if the region cannot be determined.
// copied from https://github.com/openshift/openshift-velero-plugin/pull/223/files#diff-da482ef606b3938b09ae46990a60eb0ad49ebfb4885eb1af327d90f215bf58b1
func GetBucketRegion(bucket string) (string, error) {
var region string

session, err := session.NewSession()
if err != nil {
// return "", errors.WithStack(err) // don't need stack trace, not inside velero-plugin
return "", err
}

for _, partition := range endpoints.DefaultPartitions() {
for regionHint := range partition.Regions() {
region, _ = s3manager.GetBucketRegion(context.Background(), session, bucket, regionHint)

// we only need to try a single region hint per partition, so break after the first
break
}

if region != "" {
return region, nil
}
}

return "", errors.New("unable to determine bucket's region")
}

// For tests to use when discoverable bucket is required.
const DiscoverableBucket string = "openshift-velero-plugin-s3-auto-region-test-1"
84 changes: 84 additions & 0 deletions pkg/storage/aws/s3_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
package aws

import (
"testing"
)

// from https://github.com/openshift/openshift-velero-plugin/pull/223/files#diff-4f17f1708744bd4d8cb7a4232212efa0e3bfde2b9c7b12e3a23dcc913b9fc2ec
func TestGetBucketRegion(t *testing.T) {
type args struct {
bucket string
}
tests := []struct {
name string
bucket string
want string
wantErr bool
}{
{
name: "openshift-velero-plugin-s3-auto-region-test-1",
bucket: "openshift-velero-plugin-s3-auto-region-test-1",
want: "us-east-1",
wantErr: false,
},
{
name: "openshift-velero-plugin-s3-auto-region-test-2",
bucket: "openshift-velero-plugin-s3-auto-region-test-2",
want: "us-west-1",
wantErr: false,
},
{
name: "openshift-velero-plugin-s3-auto-region-test-3",
bucket: "openshift-velero-plugin-s3-auto-region-test-3",
want: "eu-central-1",
wantErr: false,
},
{
name: "openshift-velero-plugin-s3-auto-region-test-4",
bucket: "openshift-velero-plugin-s3-auto-region-test-4",
want: "sa-east-1",
wantErr: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := GetBucketRegion(tt.bucket)
if (err != nil) != tt.wantErr {
t.Errorf("GetBucketRegion() error = %v, wantErr %v", err, tt.wantErr)
return
}
if got != tt.want {
t.Errorf("GetBucketRegion() = %v, want %v", got, tt.want)
}
})
}
}

func TestBucketRegionIsDiscoverable(t *testing.T) {
type args struct {
bucket string
}
tests := []struct {
name string
args args
want bool
}{
{
name: "openshift-velero-plugin-s3-auto-region-test-1 region is discoverable",
args: args{bucket: "openshift-velero-plugin-s3-auto-region-test-1"},
want: true,
},
{
name: "bucketNamesOverSixtyThreeCharactersAndNowItIsAboutTimeToTestThisFunction is an invalid aws bucket name so region is not discoverable",
args: args{bucket: "bucketNamesOverSixtyThreeCharactersAndNowItIsAboutTimeToTestThisFunction"},
want: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := BucketRegionIsDiscoverable(tt.args.bucket); got != tt.want {
t.Errorf("BucketRegionIsDiscoverable() = %v, want %v", got, tt.want)
}
})
}
}

0 comments on commit 8830d06

Please sign in to comment.