-
Notifications
You must be signed in to change notification settings - Fork 142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set hinting region to use for GetBucketRegion() #210
Set hinting region to use for GetBucketRegion() #210
Conversation
Signed-off-by: Tiger Kaovilai <[email protected]>
2183997
to
627b11d
Compare
// configures anonymous credentials | ||
WithAnonymousCredentials(). | ||
// configures region for GetBucketRegion to query from | ||
WithRegion("us-east-1"). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused here.
Line 159 calls function manager.GetBucketRegion
to determine the bucket's region. Do we need to set the default value as us-east-1
before that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The region in config here is only set for getting bucket region. Essentially so aws can form bucketname.s3.<regionHint>.amazonaws.com
where regionHint was previously in aws-sdk-go-v1 set to the first region retuned here.
It has to be in config before it can call GetBucketRegion otherwise you would get an error like this.
time="2024-07-12T17:30:17Z" level=error msg="Failed to determine bucket's region bucket: XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX, error: operation error S3: HeadBucket, resolve auth scheme: resolve endpoint: endpoint rule error, Invalid region: region was not a valid DNS name." backup-storage-location=openshift-adp/ts-velero-test-1 cmd=/plugins/velero-plugin-for-aws controller=backup-storage-location logSource="/opt/app-root/src/velero-plugin-for-aws/velero-plugin-for-aws/object_store.go:153" pluginName=velero-plugin-for-aws
This regionCfg is only used when if s3URL == "" && region == ""
and is thrown away after correct region is obtained.
Alternatively I can set the region I believe in latter steps, such as in newS3client or as part of GetBucketRegion()
opts.
71d4a94
to
f62c75c
Compare
Before fix
After fix
|
Signed-off-by: Tiger Kaovilai <[email protected]>
f62c75c
to
156fb10
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Fixes: vmware-tanzu/velero#8022
Adds back region hinting removed in https://github.com/vmware-tanzu/velero-plugin-for-aws/pull/177/files#diff-7ba4f8953b70f82b66f6ba15ea753c9bb847d2e01173baaef65574f60e57d86cL40-L41
Per following
https://github.com/aws/aws-sdk-go-v2/blob/4599f78694cabb6853addabc6f92cb197fdb5647/feature/s3/manager/bucket_region.go#L45-L47
AWS will make a request to
bucketname.s3.<regionHint>.amazonaws.com/
Where region hint is obtained via config which in local dev machine could be loaded from
~/.aws/credentials
so unit tests if any won't fail but in a container/k8s environment where cred file is not available it may not work.Related to fixes at openshift/oadp-operator#1470
Signed-off-by: Tiger Kaovilai [email protected]
test image:
Test image builds