-
Notifications
You must be signed in to change notification settings - Fork 30
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
feat: Account for custom S3 compatible storages (add optional parameters to AWS adapter) #11
base: master
Are you sure you want to change the base?
Conversation
207cf06
to
45170a3
Compare
V4 Signature for AWS Custom ServiceURL and Region for S3 compatible storage other than AWS Added and used some parameters Update AWSSDK.S3 to latest version possible (see aws/aws-sdk-net#2540 (comment))
45170a3
to
6474ae1
Compare
I just forced push a correction. It should be ready for merge @alanedwardes. Please check that the required null meme type is also compatible for AWS S3. |
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.
Hi, thanks for the contribution!
I am not sure whether this code makes sense in this repository, considering it is very application specific for your use case - I also must maintain it going forwards, but will not be able to test it.
Headers = new Dictionary<string, string> | ||
{ | ||
{"Content-Type", BlobConstants.UploadMimeType} | ||
} |
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 S3 supports headers, if the API in use does not support headers, it's not S3 compatible
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.
It does support headers, but it's UploadMimeType
that breaks it.
So I was asking if we could remove it if not necessary for AWS.
Why are use using it in the first place? I found this example that's use content type Host
like me, even with AWS.
This small PR would open to many cloud providers which are compatible with S3. I'm sure the community will understand you give support mostly for AWS, and they'll contribute if other cloud providers bring problems (like I did).
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.
You said you couldn't test it, but maybe you could test it on AWS S3.
It does not make sense in actual use as the default config would suffice, but it can assure you it works 👍🏻.
Something like:
S3_SERVICE_URL = "https://s3.eu-west-1.amazonaws.com"
S3Region = "eu-west-1";
S3AccessKey = "XXX";
S3AccessSecret = "XXX";
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.
Another use case that could be useful for people with this. It allows to use an AWS S3 bucket that's outside the stack, even in another region 😉
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.
Hello @alanedwardes, have you been able to try testing as I suggested?
const string LfsBucket = "estranged-lfs-test"; | ||
const string S3AccessKeyId = ""; | ||
const string S3AccessKeySecret = ""; | ||
const string S3Region = ""; | ||
const string S3ServiceURL = ""; | ||
if (!string.IsNullOrWhiteSpace(S3ServiceURL) && !string.IsNullOrWhiteSpace(S3Region) && !string.IsNullOrWhiteSpace(S3AccessKeyId) && !string.IsNullOrWhiteSpace(S3AccessKeySecret)) | ||
{ | ||
services.AddLfsS3Adapter(new S3BlobAdapterConfig { Bucket = LfsBucket }, new AmazonS3Client(S3AccessKeyId, S3AccessKeySecret, new AmazonS3Config { ServiceURL = S3ServiceURL, AuthenticationRegion = S3Region, SignatureVersion = "V4" })); | ||
} | ||
else | ||
{ | ||
services.AddLfsS3Adapter(new S3BlobAdapterConfig { Bucket = LfsBucket }, new AmazonS3Client()); | ||
} |
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 point of these being packages on NuGet is so that this kind of granular customisation can happen outside of this repository (e.g. in your own application). I think this is too application specific to be useful in the sample application.
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.
You are just using the default values, which only restrict to AWS S3.
It opens the configuration with optional parameters without breaking the existing AWS one.
With this solution, anyone could just clone the repo and is ready to go (required only to change the parameters to fit its cloud provider).
AWSSDK.S3 has been fix
AWS fix the bug I reported. I updated to the latest SDK version. |
This is nice. One feature I would have liked to have was the ability to use S3 compatible storage providers, such as Cloudflare R2. |
V4 Signature for AWS
Custom ServiceURL and Region for S3 compatible storage other than AWS (OVH, Google, MinIO, Scaleway etc.)
Added and used some parameters
Update AWSSDK.S3 to the latest version possible (see aws/aws-sdk-net#2540 (comment))
Updated Readme with more details