S3 integration: please add overloads which do not require explicit configuration

[AlexMedia]

Thanks to the AWSSDK.Extensions.NETCore.Setup package, the .NET SDKs for the AWS ecosystem can take a lot of the heavy lifting of configuring AWS clients out of a developer's hands. For example, the client can autodiscover credentials, and can read other configuration values using Amazon-defined standards.

I'm using it in a project I'm working with right now, and it's a breeze. I am looking to use Imageflow with S3 as a backing store for all images uploaded. However, the API design is currently a little limited.

For example, when I create an instance of S3ServiceOptions I have to explicitly provide an AccessKey and a SecretKey. The MapPrefix calls further down the line can be rather specific as well. In the end, the S3Service builds its own instance of AmazonS3Client to do the retrieval of data from the storage bucket.

I would like to propose the following changes to be made to S3ServiceOptions:

Constructor:

• Provide an additional constructor without parameters, so that I am not forced to specify credentials.

Add the following method:

public S3ServiceOptions MapPrefix(string prefix, Func<IAmazonS3> amazonS3ClientFactory, string bucket, string blobPrefix, bool ignorePrefixCase, bool lowercaseBlobPath)

The S3Service could then leverage the Func<IAmazonS3> to create the instance it needs to operate.

With these changes, I can provide my own instance of IAmazonS3 when Imageflow needs it, without having to do any of the heavy lifting with regards to configuration.

[lilith]

Thank you so much for the feedback! Would you be willing to make a PR? If not I can probably get to it within a few weeks.

On Mon, Jan 11, 2021, 4:35 PM Alex notifications@github.com wrote:

Thanks to the AWSSDK.Extensions.NETCore.Setup package, the .NET SDKs for the AWS ecosystem can take a lot of the heavy lifting of configuring AWS clients out of a developer's hands. For example, the client can autodiscover credentials, and can read other configuration values using Amazon-defined standards.

I'm using it in a project I'm working with right now, and it's a breeze. I am looking to use Imageflow with S3 as a backing store for all images uploaded. However, the API design is currently a little limited.

For example, when I create an instance of S3ServiceOptions I have to explicitly provide an AccessKey and a SecretKey. The MapPrefix calls further down the line can be rather specific as well. In the end, the S3Service builds its own instance of AmazonS3Client to do the retrieval of data from the storage bucket.

I would like to propose the following changes to be made to S3ServiceOptions:

Constructor:

• Provide an additional constructor without parameters, so that I am not forced to specify credentials.

Add the following method:

public S3ServiceOptions MapPrefix(string prefix, Func amazonS3ClientFactory, string bucket, string blobPrefix, bool ignorePrefixCase, bool lowercaseBlobPath)

The S3Service could then leverage the Func to create the instance it needs to operate.

With these changes, I can provide my own instance of IAmazonS3 when Imageflow needs it, without having to do any of the heavy lifting with regards to configuration.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/imazen/imageflow-dotnet-server/issues/43, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA2LH2NHC2GACIPSTANRR3SZODM5ANCNFSM4V6LUDUQ .

[AlexMedia]

I'll give it a try :)

[lilith]

Released as 0.5.4

[lilith]

Thank you so much for the perfect PR!

[AlexMedia]

Happy to help! Thank you for building such a nice product in the first place. 😀

[AlexMedia]

I don't see version 0.5.4 on Nuget yet, latest I see there is 0.5.3 and 0.5.3-build174. Will you be releasing 0.5.4 soon?

[lilith]

Oops, I made a mistake during release. Try 0.5.5 instead.

[edmacdonald]

@AlexMedia how are you using the factory to access the singleton created by the AWSSDK.Extensions.NETCore.Setup methods?

services.AddAWSService<IAmazonS3>();

[AlexMedia]

I construct an instance of AWSOptions like this:

var awsOptions = Configuration.GetAWSOptions();

And I then use its CreateServiceClient method to get an instance of IAmazonS3, like so:

var s3Configuration = new S3ServiceOptions().
    MapPrefix(path,
        () => awsOptions.CreateServiceClient<IAmazonS3>(),
        storageBucketName, pathPrefix, false, false);

[edmacdonald]

Thanks @AlexMedia for your quick response. While you were contemplating your PR, had you considered injecting the s3client? The factory is cool for scenarios where you need a different config/creds for different buckets, but in the simple case, the singleton created using services.AddAWSService<IAmazonS3>(); would do the trick.

[AlexMedia]

I did consider that but since IAmazonS3 inherits from IDisposable (while the call to AddImageflowS3Service registers the S3Service as a singleton) in my view a factory method would be better suited.

[edmacdonald]

So no particular technical hurdles? Creating the client on each call will mean profile/credential resolution is done every time. This is why Amazon's guidance (and helper method) is to use a singleton. I think I'll dig into this a little, and if I open an issue or PR, I'll tag you for your input if you don't mind.

In the meantime, thanks for your help, I'm going to adapt your suggestion a little to get at the singleton.

var s3Client = new S3ServiceOptions().CreateServiceClient<IAmazonS3>();
services.AddSingleton(s3Client);
    MapPrefix(path,
        () => s3Client,
        storageBucketName, pathPrefix, false, false);

[AlexMedia]

I must've overlooked Amazon's suggestion re. using it as a singleton. I haven't tried it either: as soon as I saw IDisposable, I built it as a factory.

I advise against using your suggestion: the factory method will dispose of the class as soon as it's done. In your case the singleton would get disposed of immediately, that might cause the instance of IAmazonS3 to end up in a broken state.

On Tue, 23 Mar 2021, 18:18 Ed MacDonald, @.***> wrote:

So no particular technical hurdles? Creating the client on each call will mean profile/credential resolution is done every time. This is why Amazon's guidance (and helper method) is to use a singleton. I think I'll dig into this a little, and if I open an issue or PR, I'll tag you for your input if you don't mind.

In the meantime, thanks for your help, I'm going to adapt your suggestion a little to get at the singleton.

var s3Client = new S3ServiceOptions().CreateServiceClient();services.AddSingleton(s3Client); MapPrefix(path, () => s3Client, storageBucketName, pathPrefix, false, false);

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/imazen/imageflow-dotnet-server/issues/43#issuecomment-805082062, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHHTXAZ2RBAOCH27IQJDGDTFDENXANCNFSM4V6LUDUQ .

[edmacdonald]

By adding it to the DI container, a reference is maintained and it won't get disposed. If I removed services.AddSingleton(s3Client);, then you're right, it would be gone before I had a chance to use it.

[AlexMedia]

That is not what IDisposable does...

Dispose frees up resources used by the class. For example, if a class opened a socket it should be closed once Dispose is called. Once Dispose has been called on an instance you can no longer be sure that it'll function correctly.

On Tue, 23 Mar 2021, 18:30 Ed MacDonald, @.***> wrote:

By adding it to the DI container, a reference is maintained and it won't get disposed. If I removed services.AddSingleton(s3Client);, then you're right, it would be gone before I had a chance to use it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/imazen/imageflow-dotnet-server/issues/43#issuecomment-805091182, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHHTXFNMCGDHJJ3SUR5XTDTFDF5DANCNFSM4V6LUDUQ .

[edmacdonald]

Ahhh... Thanks for the heads up. I am familiar with what IDiposable is for, I just wasn't aware it was being disposed of within the implementation. I see the using now. Adding to the container fixed the garbage collector disposing of it, and allowed me to get a page full of items to render, but I'm seeing the error now. I'll look for another way.

[lilith]

The docs do say it can be reused, as the client is just a factory itself. https://aws.amazon.com/blogs/developer/object-lifecycles/ It's not supposed to be heavy but it does hold a significant tree of objects.

We could change the implementation to only call the amazonS3ClientFactory once and make the plugin disposable, but it might limit flexibility in unknown ways. Please share any thoughts/feedback.

[edmacdonald]

I would suggest that in the simplest case, the user should only have to do this:

services.AddAWSService<IAmazonS3>();

This is all I need to do for AWS services used in other code, and it works across all environments. Credentials are resolved using the SDK store, credential files, env vars, or IAM instance profiles. I never have a need to manage them manually.

The plugin could call the following to get an s3 client that will work properly in the majority of cases:

container.GetRequiredService<IAmazonS3>();

This is quite flexible. If you need to manually manage your credentials, you can do that too, and register the s3 client any way you wish.

services.AddSingleton<IAmazonS3>(new AmazonS3Client(GetMyAwsCreds()));

This would also eliminate the need for all the credential management in the plugin. I can't use the parameterless constructor S3ServiceOptions() because it sets AnonymousAWSCredentials(). It's the only reason I need to use the factory, and it appears to be the reason the factory was introduced.

[lilith]

Different endpoints need different credentials sometimes.

On Fri, Mar 26, 2021, 12:32 PM Ed MacDonald @.***> wrote:

I would suggest that in the simplest case, the user should only have to do this:

services.AddAWSService();

This is all I need to do for AWS services used in other code, and it works across all environments. Credentials are resolved using the SDK store, credential files, env vars, or IAM instance profiles. I never have a need to manage them manually.

The plugin could call the following to get an s3 client that will work properly in the majority of cases:

container.GetRequiredService();

This is quite flexible. If you need to manually manage your credentials, you can do that too, and register the s3 client any way you wish.

services.AddSingleton(new AmazonS3Client(GetMyAwsCreds()));

This would also eliminate the need for all the credential management in the plugin. I can't use the parameterless constructor S3ServiceOptions() because it sets AnonymousAWSCredentials(). It's the only reason I need to use the factory, and it appears to be the reason the factory was introduced.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/imazen/imageflow-dotnet-server/issues/43#issuecomment-808433384, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA2LH7CPTB7D7Z5R3E5TYTTFTHNFANCNFSM4V6LUDUQ .

[edmacdonald]

For that case you would create a client for each set of credentials, and pass the appropriate one to each prefix mapping.

I was contemplating doing just that, but decided the better way to do it in my case would be to use one set of credentials, and provide cross-account access to the buckets/objects needed.

[edmacdonald]

@lilith I created a draft PR of what this might look like. For the purposes of discussion, I didn't worry about breaking existing clients.

[lilith]

Thanks! I'll look at this when I'm back in the office.

On Mon, Mar 29, 2021, 4:54 PM Ed MacDonald @.***> wrote:

@lilith https://github.com/lilith I created a draft PR of what this might look like. For the purposes of discussion, I didn't worry about breaking existing clients.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/imazen/imageflow-dotnet-server/issues/43#issuecomment-809773469, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA2LHZ76KAXXMXLIUVJ5M3TGEAKNANCNFSM4V6LUDUQ .